Pipelines | Validate every package the OneBranch build produces - #4655
Open
paulmedynski wants to merge 7 commits into
Open
Pipelines | Validate every package the OneBranch build produces#4655paulmedynski wants to merge 7 commits into
paulmedynski wants to merge 7 commits into
Conversation
paulmedynski
added
the
Area\Engineering
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The JSON-report PackageValidator run in validate-packages.ps1 doesn’t check its exit code, which can produce misleading/invalid report artifacts if the tool fails.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Re-enables and expands OneBranch package validation so that all produced NuGet packages are validated together (exercising cross-package rules), and gates releasing on that validation.
Changes:
- Adds a new
package_validationstage that downloads all produced package artifacts into one tree and runstools/PackageValidatoronce across the full set. - Adds official-only signature verification scripts for NuGet package signatures and Authenticode signatures of extracted binaries, with new Pester coverage.
- Makes release stages depend on
package_validationand removes the legacy single-packagevalidate-signed-package-job.yml.
File summaries
| File | Description |
|---|---|
| eng/pipelines/onebranch/steps/validate-packages-step.yml | New step template to build and run PackageValidator with version expectations and gating tokens. |
| eng/pipelines/onebranch/jobs/validate-packages-job.yml | New Windows job that downloads all package artifacts and runs validation + (official-only) signature checks. |
| eng/pipelines/onebranch/stages/build-stages.yml | Replaces commented-out validation with active package_validation stage wired to compute_versions outputs. |
| eng/pipelines/onebranch/stages/release-stages.yml | Adds package_validation to release-stage dependencies to prevent publishing unvalidated packages. |
| eng/pipelines/onebranch/scripts/validate-packages.ps1 | New script that runs PackageValidator twice (JSON report then gated human output). |
| eng/pipelines/onebranch/scripts/verify-package-signatures.ps1 | New script to run dotnet nuget verify --all across all .nupkg/.snupkg (official builds only). |
| eng/pipelines/onebranch/scripts/verify-assembly-signatures.ps1 | New script to expand .nupkg and verify Authenticode signatures for all .dll under extracted content (official builds only). |
| eng/pipelines/onebranch/scripts/tests/validate-packages.Tests.ps1 | New Pester tests covering expectation shaping, gating args, and exit-code handling. |
| eng/pipelines/onebranch/scripts/tests/verify-package-signatures.Tests.ps1 | New Pester tests covering per-package verification and failure aggregation. |
| eng/pipelines/onebranch/scripts/tests/verify-assembly-signatures.Tests.ps1 | New Pester tests covering expansion behavior, native binaries, stale extraction replacement, and failure aggregation. |
| eng/pipelines/onebranch/scripts/tests/README.md | Documents the newly added script test coverage areas. |
| eng/pipelines/onebranch/jobs/validate-signed-package-job.yml | Deleted legacy job that only validated the SqlClient package and had known gaps. |
| .github/instructions/onebranch-pipeline-design.instructions.md | Updates design doc to reflect the new stage/job/template structure and gating rules. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
paulmedynski
force-pushed
the
dev/paul/onebranch-validate-packages
branch
from
September 6, 2026 19:12
87262e6 to
e48f26a
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The FailOn gate tokens are currently passed/consumed in a way that collapses multiple tokens into a single comma-delimited value, which will break PackageValidator argument parsing and cause validation to fail.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Gate arguments are currently ineffective, and dependency-range inconsistencies are not configured to block releases.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
eng/pipelines/onebranch/jobs/validate-packages-job.yml:173
- The configured gates omit
dependency-inconsistency, even though that cross-package check is a Warning (tools/PackageValidator/src/Validator.cs:165-166) and therefore is not covered byerror. A package whose dependency range excludes the sibling version will only be reported and can still proceed to release, contrary to this stage's dependency-agreement purpose. Gate this category in both official and non-official runs.
eng/pipelines/onebranch/scripts/validate-packages.ps1:166
PowerShell@2passes the comma-joined value to this[string[]]parameter as one element ("error,missing-symbols"), so the loop emits a single unknown--fail-ontoken. As a result, even Error findings do not trip the validation gate and the stage can release invalid packages. Split each incoming element before constructing the repeated validator arguments.
foreach ($token in $FailOn) {
$gate += @("--fail-on", $token)
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Make the compute-versions stage the single source of every version the OneBranch build jobs consume, so nothing is re-derived downstream. - Remove the addRevision mode entirely. Package versions now have a single shape driven by the pipeline build number, and the 16-bit revision wrapping, the four-part package base handling, and the Build.BuildId plumbing are gone. - Move package version stamping out of PowerShell and into Versions.props. BuildSuffix now does what it always documented: it turns a stable base into a prerelease. Any version carrying a prerelease tag, from either source, is stamped with the build number; released versions are left untouched. - Publish SqlClient and SqlServer file versions from the compute-versions stage and pass them into the build jobs, which previously received a raw build number and re-derived the file version through MSBuild. build.proj gains opt-in FileVersion* arguments, so PR/CI and local builds are unchanged. - Fix SBOM metadata, which reported the pipeline run number as the version of a single hardcoded package name. Each build job now supplies the name and computed version of the package it produces, and jobs that publish no packages disable SBOM generation instead.
Re-enable the package validation stage, which had been commented out pending the version pre-computation that landed in the previous change, and widen it from one package to all six. - Add a package_validation stage that depends on all four build stages. Every package is downloaded into one tree and validated together so PackageValidator can apply its cross-package rules: the SqlClient family must share a single version, and their inter-package dependency ranges must agree. Validating one package at a time would silently skip all of those findings. - Assert the versions the compute-versions stage already published, rather than re-deriving them. The family version is applied as a wildcard expectation, so a mismatch in any one package is caught along with the case where every package is consistently wrong; Microsoft.SqlServer.Server overrides it by id. When SqlServer is not built its expectations are omitted entirely, because the validator rejects an expectation whose value is empty. - Gate on error and missing-symbols always, plus package-unsigned on official runs. missing-symbols is a warning and package-unsigned is info, so neither is covered by the error severity and both must be named explicitly. Non-official runs are deliberately unsigned, so gating them on package-unsigned would always fail. - Make the release stage depend on package validation, so a package that fails validation is never published. - Replace the SqlClient-only validate-signed-package-job, which checked one package, could not detect missing files, indexed extracted content positionally, and depended on a hardcoded sn.exe path. Authenticode and NuGet signature verification now cover every produced package. Step and job logic lives in scripts with Pester coverage rather than inline YAML, matching compute-versions and publish-symbols.
1ES auto-injects the RoslynAnalyzers task into any job containing a DotNetCoreCLI build task, and drives the build itself. The validation job's only compile is PackageValidator, a build-time tool that never ships, and the injected run is launched from the host where the container's dotnet does not exist, so it failed with exit code 17 and failed the job even though package validation passed.
Addresses review feedback on PR #4655. The JSON-report invocation of PackageValidator ignored its exit code, so a validator crash produced a misleading report artifact and let the gated run obscure the real cause. Capture and check the code before writing the success message or reaching the gate. FailOn arrives from the pipeline as a single comma-joined token, which PowerShell -File argument mode does not split into an array. Normalize the parameter by splitting, trimming, and dropping empties, and quote the YAML argument so the value is one token in every invocation mode.
paulmedynski
force-pushed
the
dev/paul/onebranch-validate-packages
branch
from
September 8, 2026 11:41
644228f to
e87b80a
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The gate currently allows dependency-range and strong-name regressions to pass validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
…dings Addresses review feedback on PR #4655. The error severity covers only error-severity findings, so the warning and info categories this job exists to catch were slipping through the gate. dependency-inconsistency is a warning, and mismatched family dependency ranges are precisely what validating the whole drop at once is meant to find, so gate it on every run. delay-signed is a warning and unsigned is info. Non-official builds have no access to the real strong-name key and are delay-signed by design, so gate those two on official runs only, alongside package-unsigned.
Addresses review feedback on PR #4655. The previous split assumed non-official builds cannot strong-name sign, but build-buildproj-step.yml downloads netfxKeypair.snk and passes SigningKeyPath unconditionally, so every OneBranch build signs with the real key. Run 26251.2 confirms it: 59 of 59 implementation assemblies reported Signed on a non-official run, with no delay-signed or unsigned findings. Gating delay-signed and unsigned only on official runs therefore left the one signing type the pipeline always applies unvalidated on PR builds, where a regression that drops the key would go unnoticed. Gate both everywhere. package-unsigned stays official-only. NuGet package signing is an ESRP step condition on shouldSignPackage, so non-official packages genuinely carry no .signature.p7s.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Failed report generation can publish an untrusted artifact, and the documented non-official gate differs from the implementation.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
eng/pipelines/onebranch/scripts/validate-packages.ps1:195
- When the reporting invocation exits nonzero,
Set-Contenthas already created an empty or partial file under$(JOB_OUTPUT). Throwing here does not remove it, so OneBranch may still publish the report even though this block explicitly considers it untrustworthy. Remove the file before throwing, and update the Pester failure test to assert it is absent.
eng/pipelines/onebranch/jobs/validate-packages-job.yml:183
- The PR description still says
delay-signedandunsignedare official-only, but this non-official branch gates both. The implementation and repository design document now agree that OneBranch always strong-name signs, so update the PR description's gating table and prose to reflect these two non-official gates; otherwise release operators are given the wrong policy.
- delay-signed
- unsigned
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-enables the OneBranch package validation stage and widens it from one package to all six.
The stage had been commented out behind this
@TODO:#4652 resolved that by publishing package and file versions from the
compute_versionsstage, so validation can now assert them directly.What it validates
tools/PackageValidatorruns once over all packages together, not once per package. Its most valuable rules are cross-package — the SqlClient family must share one version, and their inter-package dependency ranges must agree. Validating one package at a time silently skips every one of those findings.Expectations come from
compute_versionsrather than being re-derived. The family version is applied as a wildcard, so a mismatch in any single package is caught and so is the case where every package is consistently wrong;Microsoft.SqlServer.Serveroverrides it by id.When SqlServer isn't built its expectations are omitted entirely rather than passed empty — the validator rejects an expectation with an empty value, so blanking them would fail every
buildSqlServer: falserun.Gating
error,missing-symbols,dependency-inconsistency,delay-signed,unsignedpackage-unsignedThe
errortoken matches only error-severity findings, so every warning/info category the stage relies on is named explicitly:missing-symbols,dependency-inconsistencyanddelay-signedare warnings;unsignedandpackage-unsignedare info.Why the split. The pipeline applies three kinds of signing, and only one of them is unconditional:
SigningKeyPath, compile time)shouldSignPackageguarddelay-signed,unsigned)SigntoolSign)NuGetSign)package-unsignedis official-onlyStrong-name signing happens on every OneBranch run, so gating it everywhere catches a regression that drops the key — which would silently flip
PublicKeyTokenfrom23ec7fc2d6eaa4a5tonull, a breaking identity change. NuGet package signing genuinely only runs on official builds, sopackage-unsignedwould fire on every PR build and stays official-only. Confirmed by isolating that token: an unsigned package exits 0 undererroralone and 2 underpackage-unsigned, while a signed package exits 0 underpackage-unsigned.dependency-inconsistencygates everywhere. It is signing-independent, and mismatched inter-package dependency ranges are precisely what validating the whole drop in one invocation exists to catch.Gating on
missing-symbolsis safe because all six packages emit a.snupkg, each is copied into its artifact, and each lands beside its.nupkg— which is what the symbol resolver requires. That includesMicrosoft.Data.SqlClient, which packs viaNuspecFile.Signature verification
PackageValidator reports strong-name state and NuGet signature presence from metadata, cross-platform. Two additional steps verify that those signatures are trusted —
dotnet nuget verify --allandGet-AuthenticodeSignature— which requires the Windows trust store, so they run on official builds only.Release gating
release-stages.ymlnow depends onpackage_validation, so a package that fails validation is never published.Replaces validate-signed-package-job
Deleted. It validated only
Microsoft.Data.SqlClient, and its checks had real gaps: folder and TFM checks could only detect unexpected entries and never missing ones, the "expected hierarchy" check indexed extracted content positionally (so it could pass on wrong content), and strong-name verification depended on a hardcodedsn.exepath under a wildcard VS directory. Authenticode and NuGet signature verification now cover every produced package, and strong-name state comes from PackageValidator cross-platform.The validations it attempted but never reliably performed, along with everything else found missing while doing this work, are tracked in ADO #47876 — OneBranch package validation: add missing package-quality validations.
SDL Roslyn analysis
Disabled for the validation job. 1ES auto-injects a Guardian Roslyn task into any job containing a
DotNetCoreCLIbuild task; here that is only the PackageValidator tool build, which never ships. Roslyn coverage of shipping code in the six build jobs is unchanged.Scripts, not inline YAML
Step and job logic lives in
scripts/validate-packages.ps1,verify-package-signatures.ps1andverify-assembly-signatures.ps1, matchingcompute-versions.ps1andpublish-symbols.ps1.The validator is invoked twice over the same inputs. The first run writes the JSON report and never gates, so the artifact survives a failing run; the second renders the human-readable report and applies the gate, so findings appear in the build's own log rather than only in an artifact. Because the reporting run is ungated, any non-zero exit from it means the validator itself failed, and the script fails there rather than writing a misleading report and letting the gated run obscure the cause.
-FailOnis normalized by splitting on commas, so it behaves identically whether it arrives as a PowerShell array or as the single comma-joined token an Azure Pipelinesarguments:line produces.Testing
27 new Pester tests across three new test files (77 total in the suite, all passing) cover: wildcard vs per-id expectations, SqlServer omitted when unbuilt, a guard rejecting a half-supplied SqlServer version pair, comma-joined
-FailOnnormalization, report-written-before-gating, the reporting run being ungated, the reporting run failing before the gate is reached, exit-code handling (0 / 2 / other), all failures collected before throwing, native binaries underruntimes/included, and stale expansions being replaced.The assembly test builds real
.nupkgfiles and lets the script genuinely expand them; onlyGet-AuthenticodeSignatureis substituted, since it's Windows-only.PackageValidator CLI behaviour was verified end-to-end against a real published package: matching expectations exit 0, wrong versions exit 2 with per-assembly findings, an empty expectation exits 1, and
--jsonoutput is well-formed.Pipeline verification
66d6267e). In progress.compute_versions: family wildcard7.1.0-preview3.26251.2plus theMicrosoft.SqlServer.Server=1.1.0-preview1.26251.2per-id override, JSON report written before gating. Summary: 6 inspected, 0 errors, 0 warnings, 6 info — gate passed; the six info findings are allpackage-unsigned, correctly non-gating on a non-official run. All 59 implementation assemblies reportedSigning status: Signed, and every one resolved a checksum-verified symbol-package PDB.Follow-ups
Missing validations found while doing this are captured in ADO #47876 (16 items: TFM completeness, content manifest, nuspec metadata, deterministic build/SourceLink, API-compat baseline, restore smoke test, and more).