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 12 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.10% -1.62%
==========================================
Files 288 283 -5
Lines 44088 67609 +23521
==========================================
+ Hits 28532 42664 +14132
- Misses 15556 24945 +9389
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
| @@ -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"?
There was a problem hiding this comment.
Worth separating two things here, because they're independent knobs: AnalysisLevel chooses which rules are considered, not which findings break the build.
Severity is decided by each rule's author and enforced by Guardian's Post Analysis (Guardian Break) task, whose threshold defaults to Error only. So widening the rule set with latest-all cannot by itself break anything — warning- and info-level rules are collected into the SARIF for TSA/burn-down (AB#46614) and are simply not breaking.
Two further reasons to keep latest-all:
- The whole
PropertyGroupis gated onEnableAnalyzers, which defaults tofalse(build.proj). Developer and CI builds are entirely unaffected — I verifiedAnalysisLevelevaluates to8.0in a normal build andlatest-allonly under-p:EnableAnalyzers=true, i.e. the dedicated SDL analysis pass, where the breadth is the point. latest-allwas chosen deliberately as a superset of the Guardian task'sSdl.Recommended.Warning.ruleset(see the comparison table ineng/pipelines/onebranch/steps/roslyn-analyzers-buildproj-step.yml). Dropping tominimumwould land below the SDL recommended set and risks silently losing required CA3xxx/CA5xxx security rules — a compliance regression rather than just less noise.
There was a problem hiding this comment.
Recommended ruleset is fine, we can handle compiler warnings with suppression file.
| overrides the repo-wide TreatWarningsAsErrors=true set above. Genuine compiler errors still | ||
| fail the build. | ||
| --> | ||
| <TreatWarningsAsErrors>false</TreatWarningsAsErrors> |
There was a problem hiding this comment.
The repo-wide default is true, set at the top of this same file, and it stays in force for every normal build. This false lives inside the EnableAnalyzers block, which is off by default, so it applies only to the dedicated SDL analysis pass. Verified by property evaluation: TreatWarningsAsErrors is true in a normal build and false only under -p:EnableAnalyzers=true.
Setting it true in the analysis pass would be counterproductive:
- the compile halts at the first analyzer warning, truncating the SARIF log Guardian collects — fewer findings reported, not more;
- every finding gets recorded at SARIF
level: error, and since Guardian's break threshold is Error-only, the build would then fail on essentially everything; - it contradicts the Guardian
RoslynAnalyzers@3task itself, which passes/p:TreatWarningsAsErrors=falsefor exactly this reason (see the comparison table ineng/pipelines/onebranch/steps/roslyn-analyzers-buildproj-step.yml).
The comment now states all of that explicitly, including the "analysis pass only" scoping, so the next reader doesn't have to reconstruct it. Happy to discuss further if you still disagree.
There was a problem hiding this comment.
Compiler warnings can be suppressed with "suppressionFileForCompilerWarnings" option, I don't agree with the 3rd bullet point - we should follow 1es guidelines here.
In summary, I think we should:
- Run 'Recommended' ruleset
- Suppress C# compiler warnings via "suppressionFileForCompilerWarnings"
- Let errors be thrown for genuine analyzer findings / fix them ASAP (I'd like to see what exactly we get here - IMO there shouldn't be much to do if noisy compiler warnings are suppressed.)
- Continue on Error to allow running build to completion.
- Report results to TSA with Roslyn findings.
IMO, if we can handle the compiler warnings correctly, we can safely enable this workflow.
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
Disable EnforceCodeStyleInBuild in the analysis pass. The IDE* code-style rules carry no SDL value and add substantial volume to the SARIF logs. They can still be enabled locally for a deliberate code-style pass. Clarify why TreatWarningsAsErrors must remain false inside the EnableAnalyzers block. The repo-wide default of true still governs every normal build; promoting analyzer warnings to errors here would truncate the SARIF log Guardian collects and record every finding at SARIF level "error", which Guardian's Error-only break threshold would then fail the build on. The Guardian RoslynAnalyzers task passes /p:TreatWarningsAsErrors=false for the same reason. Document the Guardian break severity mechanism in both OneBranch pipelines: the analyzer tasks never break the build, Post Analysis does, the threshold is cumulative (Error < Warning < Note), and it can be set globally, per tool, or per job. OneBranch accepts only Error, Warning or Note, so inheriting the default requires omitting the key. Update the roslyn-analyzers-buildproj-step.yml comparison table, which still listed EnforceCodeStyleInBuild among the properties we enable.
CodeQL's "Analyze (csharp)" job has been failing since 1425d0d with NU1101 for every SqlClient package and NU1100 for Microsoft.NETFramework.ReferenceAssemblies.net481, both reporting that PackageSourceMapping excluded the available sources. That commit removed the packageSourceMapping block from this file on the assumption it was redundant. It was not. The <clear /> element inside <packageSources> discards only inherited SOURCES; packageSourceMapping is merged from ancestor NuGet.config files independently. This PR adds a mapping to the repo root config that points "*" at the governed source, and this project clears governed away, so the subtree was left with nuget.org in no mapping at all and governed mapped but undefined. Every restore therefore had zero eligible sources. Restore the mapping and add a <clear /> so the subtree no longer inherits the root mapping, plus a comment explaining why the section must not be deleted again. Local builds masked this because the NuGet global-packages cache is resolved before any source, so mapping is never exercised on a warm machine. Verified with a cold cache: restore succeeds with this change and reproduces the exact CI NU1100/NU1101 errors without it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Directory.Build.props:48
- The comment above
AnalysisLevelsays the analysis pass enables the IDE code-style analyzers, butEnforceCodeStyleInBuildis explicitly set tofalseimmediately below to keep IDExxxx rules off. This is internally inconsistent and could mislead future edits (e.g., someone may assume IDExxxx findings are included in the SARIF logs when they are not).
<!--
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.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Directory.Build.props:48
- The comment above AnalysisLevel says IDE code-style analyzers are enabled, but this PropertyGroup explicitly sets EnforceCodeStyleInBuild=false (which disables IDE code-style analyzers during build). This is internally contradictory and can mislead future changes to the analyzer configuration.
<!--
Turn on every rule the SDK ships, at the latest level, plus the IDE code-style analyzers.
-->
<AnalysisLevel>latest-all</AnalysisLevel>
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: