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 has been commented out for some time 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. That matters: 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 — verified that the validator exits 1 with
missing valueotherwise, so blanking them would fail everybuildSqlServer: 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 has to be named explicitly. Confirmed againstValidator.cs:missing-symbolserrordependency-inconsistencyerrordelay-signederrorunsignederrorpackage-unsignederrordependency-inconsistencygates on every run. It is signing-independent, and mismatched inter-package dependency ranges are precisely what validating the whole drop in one invocation exists to catch.The two strong-name categories also gate on every run, because strong-name signing is the one signing type the pipeline applies unconditionally:
build-buildproj-step.ymldownloadsnetfxKeypair.snkand passes-p:SigningKeyPath=with noshouldSignPackageguard, unlike the two ESRP steps. Run 26251.2 confirms it — 59 of 59 implementation assemblies reportedSigning status: Signedon a non-official run, with zerodelay-signedand zerounsignedfindings. Leaving these official-only would have left the always-applied signing type unvalidated on PR builds, where a broken key download would silently flipPublicKeyTokenfrom23ec7fc2d6eaa4a5tonull— a breaking identity change — and not surface until an official run.package-unsignedstays official-only. NuGet package signing is an ESRP step genuinely conditioned onshouldSignPackage, so non-official packages carry no.signature.p7sby design and gating it everywhere would fail every PR build.Verified by isolating each token against a signed package and a stripped-signature copy:
erroronlypackage-unsignedpackage-unsignedAlso confirmed via
--fail-onhelp output that all five category tokens are accepted, and that gate matching is exact set membership (Program.Matches), sounsignedcannot collide withpackage-unsigned.Gating on
missing-symbolsis safe: I confirmed against non-official run 172931 that all six packages emit a.snupkg, that each is copied into its artifact, and that it lands beside its.nupkg— which is what the symbol resolver requires. That includesMicrosoft.Data.SqlClient, which packs viaNuspecFile.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.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.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.Testing
--jsonwell-formed66d6267e), verifying the widened strong-name gate. In progress; result to be posted when it completes. Run 26251.2 below already shows the findings this gate acts on: zerodelay-signedand zerounsignedacross all six packages, so it is expected to pass.d34d615d, covering thedependency-inconsistencygate and the script hardening. All six packages validated together against the versions published bycompute_versions: family wildcard7.1.0-preview3.26251.2plus theMicrosoft.SqlServer.Server=1.1.0-preview1.26251.2per-id override, gateerror+missing-symbols+dependency-inconsistency, 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. Zerodependency-inconsistencyfindings, so the family's inter-package ranges agree with the versions actually produced; every implementation assembly resolved a checksum-verified symbol-package PDB.-FailOn "error,missing-symbols,dependency-inconsistency"as a single quoted token and the script split it correctly into--fail-on error --fail-on missing-symbols --fail-on dependency-inconsistency. No Guardian Roslyn task was injected into the validation job.compute_versions: family wildcard7.1.0-preview3.26250.4plus theMicrosoft.SqlServer.Server=1.1.0-preview1.26250.4per-id override, JSON report written before gating. Summary: 6 inspected, 0 errors, 0 warnings, 6 info — allpackage-unsigned, correctly non-gating on a non-official run — gate passed. That run predates thedependency-inconsistencygate, which is what 26251.2 re-verifies.DotNetCoreCLIbuild task. Here that is only the PackageValidator tool build, which never ships, so Roslyn is disabled for this job; shipping-code Roslyn coverage in the six build jobs is unchanged.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).