M8a: quality & release gates (NFR-002/005/009 + CI/CD)#8
Conversation
First of three M8 PRs (M8 = conformance + interop + samples + gates). Adds the
pure-.NET quality and release gates — no external runtime.
- Credentials.TestSupport: [FrTag] coverage attribute + RequirementIds (34 FR + 9 NFR).
- Credentials.ArchitectureTests: no-draft-type surface (F3/NFR-005), static no-Newtonsoft
reference closure (NFR-002), empty-<summary> catcher (NFR-009), net10/async gates
(NFR-001/004), and the FrCoverage gate (every PRD §8 requirement has a tagged test).
- Credentials.RoundTripTests: FR-003 byte fidelity per issued securing family.
- Credentials.ConsumerProbe + tools/check-no-newtonsoft-closure.sh: authoritative
package-level NFR-002 transitive-closure check.
- PublicAPI analyzers (RS0016/17) + committed PublicAPI.{Shipped,Unshipped}.txt; ApiCompat
tool (.config) + tools/check-api-compat.sh (skip-logged pre-release).
- [FrTag] tags across the suite + new FR-004 (lazy projection memoization) and FR-021
(status-list set/clear round-trip) tests.
- .github/workflows/ci.yml + release.yml.
Adversarial pass fixed two hollow-gate findings: the ConsumerProbe stale-same-version-cache
false-pass (now a unique per-run version via an env-var MSBuild property) and the static
closure walk's used-reference blind spot (documented + recursion broadened; the package
probe is authoritative). FrCoverage hardened to ignore commented-out tags.
Build 0-warning under TreatWarningsAsErrors; 338 tests green (was 318).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
moisesja
left a comment
There was a problem hiding this comment.
Review — M8a quality & release gates
Reviewed the full diff. This is genuinely strong infrastructure work: the MetadataLoadContext approach to inspect Credentials.Rdfc's surface without pulling its Newtonsoft transitive is the right call, the ConsumerProbe stale-same-version-cache false-pass is a real and non-trivial finding that was correctly diagnosed and fixed, and the DeferredToLaterPr mechanism is honest rather than silently-green. No correctness blockers; 318→338 with the new tests targeted where they matter. Not approving outright only because of the small items below — none of which need to block merge.
Worth fixing
release.yml— quote the version interpolation. Build (L149) and pack (L164) use-p:CredentialsVersion=${{ steps.version.outputs.version }}unquoted.versionderives from the pushed tag (${GITHUB_REF_NAME#v}). Tag-push is normally maintainer-only so the risk is low, but a tag name with shell metacharacters/spaces would word-split or inject MSBuild args. Wrap in quotes:-p:CredentialsVersion="${{ steps.version.outputs.version }}".
Worth confirming (likely fine)
.snupkgreaches nuget.org. The pack step is labelled.nupkg + .snupkgand the artifact upload globs*.*nupkg, but the push step only globs*.nupkg. This is probably fine becausedotnet nuget pushauto-pushes an adjacent.snupkgof the same name — but please confirm symbols actually land on nuget.org on the first real release, since the glob doesn't make it explicit.
Minor
RoundTripFidelityTests.ProofCountreturns 0/1, not a count. It only checksTryGetPropertyValue("proof", …), so aproofthat is a 2-element array still returns1and the "exactly one proof" assertion passes. Won't catch an accidental multi-proof emission. Tighten to count array elements if that's the intent.LibrarySurface.Configuration(.Parent!.Name) assumesbin/<Config>/net10.0/. The!suppresses a real NRE that would surface from a static initializer under non-standard output paths (dotnet test --output, publish). Fine in CI; a defensive fallback would save a confusing crash locally.- CI job graph.
no-newtonsoftandsemverare parallel peers with noneeds: build-test, and each triggers its own pack/rebuild. Fine for wall-clock, but a single build break shows up as three independent red jobs rather than one root cause. Optionalneeds: build-testwould sharpen the signal.
Happy to push fixes for any of these if you'd like — say the word.
Generated by Claude Code
… gate CI jobs Addresses the PR #8 review (all non-blocking): - release.yml: quote -p:CredentialsVersion="${{...}}" in the build and pack steps so a tag with shell metacharacters/spaces cannot word-split or inject MSBuild args; document that the *.nupkg push glob auto-publishes the adjacent .snupkg symbols. - RoundTripFidelityTests.ProofCount: count proof array elements (a 2-proof array now returns 2), so the "exactly one proof" assertion catches an accidental multi-proof emission, not just presence. - LibrarySurface: guard the bin/<Config>/<tfm> assumption (no more `!`-suppressed NRE from a static initializer) and try both Debug/Release when locating the Rdfc artifact under a non-standard output. - ci.yml: no-newtonsoft + semver jobs `needs: build-test`, so a compile break is one root-cause job. 338 tests green; build 0-warning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the review — all five addressed in b94b953 (validated each against the code first; all were accurate, no misreads): Worth fixing
Worth confirming
Minor
338 tests green, build 0-warning. Ready for another look. |
Captures the workflow correction — opening a PR starts its review cycle; don't stack the next PR on an unreviewed base. Wait for review/merge before the next unit of work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
First of three M8 PRs (M8 = conformance + interop + samples + gates). M8a lands the pure-.NET quality and release gates — no external runtime — closing the package / surface / doc halves of NFR-002, NFR-005, NFR-009 empirically and wiring CI/CD.
Verification:
dotnet build -c Releaseis 0-warning underTreatWarningsAsErrors; 338 tests green (Core 145 + DI 154 + Architecture 9 + RoundTrip 9 + Rdfc 21; was 318).What's in it
Credentials.TestSupport—[FrTag]coverage attribute +RequirementIds(the §8 set: 34 FR + 9 NFR).Credentials.ArchitectureTests(metadata-only viaMetadataLoadContext):DataProofsDotnettype on the public surface (F3 / NFR-005) — return/param/field/property/generic-arg/base/interface vectors;<summary>catcher (NFR-009) complementing CS1591-as-error;[FrTag]-tagged test (NFR-007 deferred to M8c, logged).Credentials.RoundTripTests— FR-003 byte-fidelity per issued family (verbatim received bytes; DI JCS+RDFC byte-stable, oneproof; JOSE/SD-JWT verbatim compact + signed==source; COSE verbatim; H1 special-char fidelity).Credentials.ConsumerProbe+tools/check-no-newtonsoft-closure.sh— authoritative package-level NFR-002 closure check (dotnet list --include-transitive).PublicAPI.{Shipped,Unshipped}.txt(749/26/16);Microsoft.DotNet.ApiCompat.Tool+tools/check-api-compat.sh(skip-logged pre-release).[FrTag]tagging across the suite + new FR-004 (lazy projection memoization) and FR-021 (status-list set/clear round-trip) tests..github/workflows/ci.yml(ubuntu+windows matrix +no-newtonsoft+semverjobs) andrelease.yml(tag-driven pack + gate + push under a protectednuget-releaseenvironment).Adversarial pass — two real findings fixed
Each gate was attacked to confirm it has teeth. Surface, semver (RS0016), doc (CS1591 + empty-summary), and FrCoverage (missing/unknown id) all failed correctly when defeated. Fixed:
0.1.0served stale same-version package metadata from NuGet's cache, so an injected Newtonsoft dependency was invisible (always reported the first run). Fixed via a unique per-run version delivered as an env-var MSBuild property (consistent across restore/build/dotnet list). Verified it now flags the violation.Next: PR-B (samples + api-coverage), then PR-C (conformance + interop).
🤖 Generated with Claude Code