Enable SDL Roslyn analysis (incl. internal IA* analyzers) in the OneBranch build - #4464
Enable SDL Roslyn analysis (incl. internal IA* analyzers) in the OneBranch build#4464paulmedynski wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Updates OneBranch pipeline configuration to modernize the Windows build environment to Windows Server 2025 / VS 2026 containers and to make Roslyn Analyzers SDL run reliably on the new image by switching the task to auto-detected build replay.
Changes:
- Upgraded the OneBranch Windows container image to
ltsc2025/vse2026. - Set
WindowsHostVersion: { Version: 2025 }(and documented host-vs-container expectations) in both official and non-official OneBranch pipelines. - Switched Roslyn Analyzers to auto mode and added a preceding “throwaway”
dotnet buildso the task can detect/replay compilation from logs; removed the now-unusedpackageVersionpass-through to that analyzer template.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| eng/pipelines/onebranch/variables/onebranch-variables.yml | Bumps Windows container image to ltsc2025/vse2026 and adds a doc link for OneBranch images. |
| eng/pipelines/onebranch/steps/roslyn-analyzers-buildproj-step.yml | Moves Roslyn analyzers to auto-detected build replay with a preceding DotNetCoreCLI@2 build for log generation; drops unused packageVersion parameter. |
| eng/pipelines/onebranch/sqlclient-official.yml | Sets WindowsHostVersion to 2025 and adds detailed documentation about host/container relationship and CDPx PAT injection. |
| eng/pipelines/onebranch/sqlclient-non-official.yml | Same host-version and documentation updates as the official pipeline. |
| eng/pipelines/onebranch/jobs/build-buildproj-job.yml | Removes passing packageVersion into the roslyn analyzers step template (since it no longer accepts/needs it). |
paulmedynski
added
the
Area\Engineering
| parameters: | ||
| dependencyArguments: $(dependencyPackageArguments) | ||
| packageShortName: ${{ parameters.packageShortName }} | ||
| packageVersion: ${{ parameters.packageVersion }} |
There was a problem hiding this comment.
The Roslyn template no longer needs this value.
| # Windows jobs use this image. | ||
| - name: WindowsContainerImage | ||
| value: onebranch.azurecr.io/windows/ltsc2022/vse2022:latest | ||
| value: onebranch.azurecr.io/windows/ltsc2025/vse2026:latest |
There was a problem hiding this comment.
Enable the latest tooling and environment for Windows jobs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
eng/pipelines/onebranch/steps/roslyn-analyzers-buildproj-step.yml:98
- This comment refers to the OAuth token env var as
system_accesstoken, but the task is actually configured with the standardSYSTEM_ACCESSTOKENname right below. Updating the comment avoids confusion when troubleshooting log-access failures.
# analyzers. Requires OAuth token access (system_accesstoken) to read the build logs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4464 +/- ##
==========================================
- Coverage 64.71% 63.17% -1.55%
==========================================
Files 288 288
Lines 44088 67329 +23241
==========================================
+ Hits 28532 42533 +14001
- Misses 15556 24796 +9240
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
eng/pipelines/onebranch/steps/roslyn-analyzers-buildproj-step.yml:179
- The PR description says the SDL RoslynAnalyzers task was switched to auto mode (
userProvideBuildInfo: autoMsBuildInfo) where the task re-runs the detected build. The current template instead performs an explicitDotNetCoreCLIbuild and uses RoslynAnalyzers incopyLogsOnly: truemode. Please update the PR description (or the implementation) so they match, to avoid future maintainers debugging the wrong behavior.
- task: securedevelopmentteam.vss-secure-development-tools.build-task-roslynanalyzers.RoslynAnalyzers@3
displayName: 'Roslyn Analyzers (collect) - build.proj Build${{ parameters.packageShortName }}'
inputs:
copyLogsOnly: true
# Root to search for the *.csproj.*.sarif logs. The analysis build wrote them next to each
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
eng/pipelines/onebranch/steps/roslyn-analyzers-buildproj-step.yml:203
- The PR description says the RoslynAnalyzers task was switched to auto mode (autoMsBuildInfo) where it re-runs the detected build command. This template now runs RoslynAnalyzers@3 in
copyLogsOnly: truemode and performs the build itself via DotNetCoreCLI, so the implementation and PR description are out of sync. Please update the PR description to reflect the current approach (Copy Logs Only + integrated analyzers), or adjust the YAML to match the described auto-mode behavior.
# Step 2: Collect the analysis results. In Copy Logs Only mode the task does not build or re-run
# the compiler -- it just gathers and sanitizes the *.csproj.*.sarif logs produced by Step 1 and
# hands them to Guardian/SDL. No msBuildVersion / msBuildArchitecture / VS setup is involved, and
# no OAuth token is needed (nothing is queried from Azure DevOps).
- task: securedevelopmentteam.vss-secure-development-tools.build-task-roslynanalyzers.RoslynAnalyzers@3
displayName: 'Roslyn Analyzers (collect) - build.proj Build${{ parameters.packageShortName }}'
inputs:
copyLogsOnly: true
# Root to search for the *.csproj.*.sarif logs. The analysis build wrote them next to each
# project under the repo checkout; the collector globs this directory recursively.
logRootDirectory: '$(REPO_ROOT)'
46b5a04 to
b6762e4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
eng/pipelines/onebranch/steps/roslyn-analyzers-buildproj-step.yml:10
- The PR description says the RoslynAnalyzers step was switched to auto mode (autoMsBuildInfo) where the task re-runs the detected build command. The updated template here instead implements the "integrated analyzers + Copy Logs Only" flow (self-run DotNetCoreCLI build + copyLogsOnly collector) and explicitly states the task performs no build/re-run. Please update the PR description so it matches the actual implementation in this file.
# This template runs Roslyn Analyzers (SDL) against a build.proj target using the RoslynAnalyzers@3
# task from the Secure Development Team's SDL extension, in "Copy Logs Only" mode:
#
# https://eng.ms/docs/cloud-ai-platform/devdiv/one-engineering-system-1es/1es-mohanb/security-integration/guardian-wiki/sdl-azdo-extension/roslyn-analyzers-build-task
- roslyn-analyzers step: stop logging the generated NuGet.config (it contains the confidential internal feed URL) and log only the path; quote the config path for space-safety; verify SARIF via -Filter instead of enumerating every *.sarif; clarify packageSourceMapping (longest-prefix match pins Microsoft.Internal.*); correct the non-incremental note (no Clean; safety comes from bin redirection). - build.proj / src/Directory.Build.props: correct the isolated-build note and fix the SARIF version in comments (v2 -> v1).
f023c28 to
fb591ac
Compare
| @@ -119,6 +119,8266 @@ | |||
| "default" | |||
| ], | |||
| "createdDate": "2026-07-23 11:29:23Z" | |||
| }, | |||
There was a problem hiding this comment.
Additional Guardian baseline signatures to grandfather/suppress all exsiting Roslyn analysis issues. We will burn these down via AB#46614.
|
|
||
| <!-- Build Output ==================================================== --> | ||
| <PropertyGroup> | ||
| <ArtifactPath>$(RepoRoot)artifacts/</ArtifactPath> |
There was a problem hiding this comment.
Necessary for the isolated build support added to build.proj.
| # source, and the Guardian RoslynAnalyzers CLI binaries (Microsoft.Guardian.RoslynAnalyzers*.dll). | ||
| # Re-verify these claims if the task's major version changes. | ||
| # | ||
| # HOW IT WORKS (integrated analyzers + Copy Logs Only): |
There was a problem hiding this comment.
Bottom line: We can't use the RoslynAnalyzers@3 task in build-mode because it isn't compatible with orchestration projects like our build.proj. Instead, we must enable the analyzers ourselves in our projects and then point the RoslynAnalyzers@3 task to the generated SARIF files for it to collect and process.
This has the added benefit that we can now enable Roslyn analysis anywhere - developer builds, other pipelines, etc.
| # Exact mappings take precedence over the governed feed's wildcard, ensuring validation SNI | ||
| # packages are restored from this source. Both package IDs are externally produced and are | ||
| # therefore intentionally not eligible for the repository's local feed. | ||
| $packageSourceMapping = $xml.SelectSingleNode('//ns:packageSourceMapping', $nsm) |
There was a problem hiding this comment.
This pipeline template was updated for completeness, but no pipelines actually invoke it currently.
There was a problem hiding this comment.
How is it going to be used in future?
What's the idea here?
There was a problem hiding this comment.
I believe it is orphaned and we just didn't remove it, but that's outside the scope of this PR. There will be a cleanup pass when we remove the legacy PR/CI pipelines, and it will be removed then.
| -p:SkipDependencyPack=true | ||
| -p:BuildNumber="$(Build.BuildNumber)" | ||
| -p:PackageVersion${{ parameters.packageShortName }}="${{ parameters.packageVersion }}" | ||
| -p:IsolatedBuildPath="$(Agent.TempDirectory)/roslyn" |
There was a problem hiding this comment.
Here we see the new isolated build and analysis options being used.
| <add key="governed" value="https://sqlclientdrivers.pkgs.visualstudio.com/public/_packaging/sqlclient/nuget/v3/index.json" /> | ||
| <add key="local" value="packages/" /> | ||
| <!-- NuGet expands this environment variable before restoring internal analyzer packages. --> | ||
| <add key="internal-analyzers" value="%INTERNAL_ANALYZERS_FEED%" /> |
There was a problem hiding this comment.
This allows the Official pipelines to inject the internal feed URL.
| --> | ||
| <add key="local" value="packages/" /> | ||
| </packageSources> | ||
| <packageSourceMapping> |
There was a problem hiding this comment.
Why is it needed in this project?
There was a problem hiding this comment.
Whoops - I had a commit to remove this staged and forgot to push!
cheenamalhotra
left a comment
cheenamalhotra
left a comment
There was a problem hiding this comment.
Looking into results, most of scans are code related recommendations, I would say we should keep this one strictly for official build quality - and locally devs can enable as needed.
| <!-- | ||
| Turn on every rule the SDK ships, at the latest level, plus the IDE code-style analyzers. | ||
| --> | ||
| <AnalysisLevel>latest-all</AnalysisLevel> |
There was a problem hiding this comment.
Can we limit it to mandatory and critical only? Set to "Minimum"?
| IDE code-style (IDExxxx) rules ship in the SDK; their configuration (which rules fire, at what | ||
| severity, and the style preferences they check) lives in our .editorconfig files. | ||
| --> | ||
| <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild> |
There was a problem hiding this comment.
Should be skipped IMO, it's generating a lot of unwanted stuff. One can toggle it manually if needed.
| overrides the repo-wide TreatWarningsAsErrors=true set above. Genuine compiler errors still | ||
| fail the build. | ||
| --> | ||
| <TreatWarningsAsErrors>false</TreatWarningsAsErrors> |
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
Description
Adds Roslyn analysis to our C# projects so it can be explicitly performed as part of Official builds. Previously, we were relying entirely on an internal Azure Pipelines task (
RoslynAnalyzers@3) which was silently failing to perform any analysis.Roslyn analysis via "Copy Logs Only"
.NET Roslyn analyzers are compiler-integrated — they only run during the real
csccompilation.build.projis an orchestrator that compiles each package in a separate childdotnet buildvia<Exec>, so analyzer properties injected onto the outer command never cross into the real compiles. That is why the earlier auto/manual-mode attempts collected 0 SARIF. Instead, the analyzers are baked into the leaf builds and theRoslynAnalyzers@3task runs in Copy Logs Only mode:build.projgains anEnableAnalyzersflag that is forwarded into every leafdotnet build.src/Directory.Build.propsreacts by enabling the full analyzer set (AnalysisLevel=latest-all+ IDE style rules) and emitting one SARIF v1 log per project.ErrorLogintentionally omits,version=2, because the Guardian sanitizer deserializes SARIF v1 (a v2 log throws and is dropped).IsolatedBuildPathredirects each project'sbinand forces a full recompile) so it never disturbs real build output, then the task only collects/sanitizes the*.csproj.*.sariflogs. Because Copy Logs Only performs no build, it needs no MSBuild and is agnostic to the container's VS/MSBuild version. A guard step fails fast if a package produced no SARIF.Internal IA* analyzers (private ADO.Net pipeline only)
AnalysisLevel=latest-allcovers the CA* rules but not the internalMicrosoft.Internal.Analyzers("IA*") rules. That package is Microsoft-internal and confidential, so:InternalAnalyzers=true(gated inDirectory.Build.props/Directory.Packages.props,PrivateAssets=all, so it never ships).NuGet.configat runtime pointing at the internal feed, withpackageSourceMappingscopingMicrosoft.Internal.*to that feed so the confidential package never touches the public governed feed.internal-analyzers-variables-v1variable group, defined only in the private ADO.Net project. No public/private gate is needed because the OneBranch pipelines run only in that project.SDL baseline
.config/guardian/.gdnbaselines, merged with the base branch's non-Roslyn entries (835 total), so official builds only fail on new findings.Windows Server 2025 build image
ltsc2025/vse2026and setWindowsHostVersion: { Version: 2025 }so the host and container OS versions match.LinuxHostVersionis not set — Linux containers share the host kernel), plus the legacy CDPx PAT flag (EnableCDPxPAT: false).Testing
OneBranch builds - to confirm Roslyn behaviour:
sqlclient-non-official: 26203.5 — analyzers run across all packages, SARIF produced, per-package baselines generated. IA* verified loaded (IA5352 in the rule catalog; 0 active findings).sqlclient-non-official: 26203.6 — committed.gdnbaselinesloaded by Guardian Post-Analysis.sqlclient-non-official: 26204.3 -break=true, and it failed with non-suppressable APIScan errors, as expected.PR/CI builds - to confirm that NuGet.config changes are benign: