M8b: samples matrix + API-coverage gate#9
Conversation
Second of three M8 PRs. A first-class offline samples matrix + a gate proving the samples exercise the public surface. - Credentials.Samples.Shared: did:key key minting, FR-banner narrator, and the allowlist IIssuerTrustPolicy (the one shipped trust policy — lives in samples, not the library, FR-082). - 14 samples/* console projects (Program.RunAsync(TextWriter, IServiceProvider?)), offline, each throwing on an unexpected outcome: DataIntegrity (+ a capabilities query), DataIntegrityRdfc, Jose/Cose envelope, SdJwtVc, SdJwtPresentation (KB-JWT), Bbs2023 (IsAvailable-gated), Presentation DI/Jose, StatusList, Schema, IssuerTrust, Vcdm11 (foreign-1.1 verify + opt-out gate), FullPipeline (status+schema+trust composed). - Credentials.SampleSmokeTests: runs every sample in-process (14 facts) + drives api-coverage. - tools/api-coverage: MetadataLoadContext surface enumerator + coverlet diff. Type-level gate — 53 covered, 0 uncovered, 4 documented exemptions (api-coverage-exclusions.txt); also fails on a stale exclusion. + tools/run-api-coverage.sh, tools/coverage.runsettings. - ci.yml: api-coverage job (needs: build-test). Build 0-warning; 352 tests green (was 338). Adversarial pass: the api-coverage gate fails correctly when an uncovered public type is injected (not hollow). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
moisesja
left a comment
There was a problem hiding this comment.
Review — M8b samples matrix + API-coverage gate
Read the whole diff. Overall this is strong, disciplined work: no src/ production code is touched — it's purely additive (14 samples, shared helpers, smoke tests, the coverage tool, one CI job), so runtime risk to the library is ~zero. The samples are real tests, not decoration: each one asserts concrete outcomes and throws on any deviation (decision must be Accepted/Rejected, specific checks must be Passed/Failed, COSE/JWS shapes validated, 1.1 must not upgrade). The smoke tests delegating their assertions to the samples is a sound, documented design. The trust/status/1.1 samples correctly exercise both accept and reject paths.
That said, the entire point of this PR is a gate that must never false-green, so my concerns are concentrated there.
1. (Main concern) The gate silently treats "absent from cobertura" as "not gateable" — a false-green vector
tools/api-coverage/Program.cs:
if (!coveredRate.TryGetValue(name, out var rate)) { notGateable++; continue; }
if (rate > 0) covered.Add(name); else uncovered.Add(name);The classification of a public type as "not gateable" is inferred purely from its absence in the cobertura report — never from reflection. But the MetadataLoadContext side already knows the truth (type.IsInterface, type.IsEnum, IsAbstract && IsSealed, etc.) and throws that knowledge away.
Coverlet does emit line-rate=0 entries for normal classes, so the common case (a new public class with methods, uncovered) is correctly caught — and your adversarial injection test verifies exactly that path. The residual hole is no-IL concrete surface: a new public static class WellKnownFoo { public const string X = "…"; } or an abstract base with no method bodies produces no cobertura <class> entry → it's silently bucketed into notGateable and exempt-by-absence, with no entry in the exclusions file and no failure. notGateable is printed but never asserted on, so a drift upward is invisible.
Recommendation: classify gateability from reflection metadata, and treat "concrete class with executable members but absent from cobertura" as uncovered (fail), not not-gateable. That closes the gap and makes the "every gateable public type is exercised" claim actually airtight rather than airtight-for-the-cases-coverlet-happens-to-emit.
2. Confirm BBS actually runs on the CI runner
samples/Credentials.Sample.Bbs2023/Program.cs returns early (exit 0, narrates "skipped") when !Bbs2023Cryptosuite().IsAvailable. The smoke test still passes (non-empty narration), so if the native BBS library isn't present on ubuntu-latest, the bbs-2023 derive/verify path is never validated in CI and the green check is misleading. This matches the M5 gating pattern, so it may be intentional/consistent — but please confirm whether the CI runner has the native lib. If it doesn't, the BBS sample is a no-op in CI and that should be called out explicitly (or the lib provisioned in the job).
3. Hardcoded net10.0 in the coverage script is brittle
tools/run-api-coverage.sh:
BIN="$ROOT/tests/Credentials.SampleSmokeTests/bin/Debug/net10.0"When the TFM bumps, this path silently stops existing → the tool gets bad DLL paths → MetadataLoadContext fails rather than reporting a coverage gap. Derive the TFM (read it from the csproj, or glob bin/Debug/net*/) so a framework bump can't quietly break the gate.
Minor / non-blocking
- Double build in CI: the
api-coveragejobneeds: build-testbutrun-api-coverage.shruns its owndotnet build, so the solution is built twice. Correct, just wasteful. IsCompilerGenerated(Program.cs): the>d__and__+DisplayClassclauses are already subsumed byContains('<')(async state machines and closures both contain<). Harmless, but dead conditions.- DRY: the
provider = services ?? …; ownsProvider = services is null; try/finally disposeboilerplate is repeated in all 14 samples, andInMemorySchemaResolveris duplicated inSchemaandFullPipeline. Defensible if you want each sample self-contained as a teaching artifact, but a sharedSampleHosthelper would cut ~14 copies. - Cosmetics:
SampleSmokeTests.csprojhas… Vcdm11.csproj" /> </ItemGroup>on one line; ProjectReference paths mix\and/. Builds fine on both OSes.
Verdict
Not requesting changes — nothing here blocks merge and the library code is untouched. But item 1 goes to the integrity of the very guarantee this PR sells, and item 2 decides whether the BBS check is real or theater in CI. I'd like those two addressed (or explained) before this is the gate the rest of M8 leans on.
🤖 Reviewed by Claude Code
Generated by Claude Code
Addresses the PR #9 review: - (item 1, main) api-coverage gateability is now decided from REFLECTION, not from absence in cobertura: a concrete type with executable members that coverlet didn't emit is treated as UNCOVERED (fail), closing the no-IL false-green vector. Interfaces/enums/delegates/const-only static classes are not-gateable by metadata, and the not-gateable set is now PRINTED (auditable — an upward drift is visible, not silently swallowed). Adversarial re-check: a gateable uncovered type still fails. - (item 3) run-api-coverage.sh derives the output dir from the built Credentials.Core.dll instead of hardcoding net10.0, so a TFM bump reports a gap rather than crashing on a missing path. - (minor) IsCompilerGenerated simplified to the subsuming Contains('<'); SampleSmokeTests.csproj ProjectReferences reformatted (consistent separators, proper line breaks). Item 2 (BBS in CI) confirmed in the PR reply: NetCrypto 1.1.0 ships linux-x64 libzkryptium_ffi.so, so the bbs-2023 path runs for real on ubuntu-latest (not the skip branch). Gate still: 53 covered, 0 uncovered, 4 exempted. Build 0-warning; 352 tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — sharp review, especially item 1. All addressed in 1. (Main) Gateability now from reflection, not cobertura-absence — false-green closedYou're right that inferring "not gateable" from absence threw away what Adversarial re-check: an injected gateable-but-uncovered class still fails ( 2. BBS does run on CI (not theater)Confirmed: 3. TFM no longer hardcoded
Minor
352 tests green, 0-warning build, api-coverage gate green. Ready for another look. |
Second of three M8 PRs (after M8a, #8). A first-class, offline samples matrix demonstrating every role × securing form plus status/schema/trust/1.1, and an api-coverage gate proving the samples exercise the public surface.
Verification: build 0-warning under
TreatWarningsAsErrors; 352 tests green (338 + 14 sample smoke tests); all 14 samples run to their expected outcome; api-coverage gate passes (53 covered / 0 uncovered / 4 exempted).What's in it
Credentials.Samples.Shared—SampleKeys(in-memorydid:key),SampleNarrator(FR banners), andAllowlistIssuerTrustPolicy(the one shipped trust policy — per FR-082 it lives in samples, not the library).samples/*console projects, eachProgram.RunAsync(TextWriter, IServiceProvider?), offline, narrating the FRs it demonstrates and throwing on any unexpected outcome:ISecuringCapabilities/SecuringSelectorquery), DataIntegrityRdfc (eddsa-rdfc-2022), JoseEnvelope (vc+jwt), CoseEnvelope (vc+cose)IsAvailable-gated)AcceptVcdm11=falseopt-out gate), FullPipeline (status + schema + trust composed)Credentials.SampleSmokeTests— runs every sample in-process (14 facts); doubles as the api-coverage driver under coverlet (scoped to Core + DI).tools/api-coverage(+run-api-coverage.sh,coverage.runsettings,api-coverage-exclusions.txt) — diffs the public surface (viaMetadataLoadContext) against the samples' coverage; fails if any gateable public type is exercised by no sample. Type-level (calibrated: only error-path/options types needed exempting); the tool also fails on a stale exclusion that has become covered.ci.ymlgains anapi-coveragejob (needs: build-test).Notes / decisions
SecuringSelectorwas covered by adding a real capabilities query to the DataIntegrity sample rather than exempted.did:key-issued secured-1.1 fixture (public issuance is 2.0-only), so it verifies fully offline.Adversarial pass
The api-coverage gate fails correctly when an uncovered public type is injected into Core (verified — not hollow, the M8a-ConsumerProbe lesson applied).
Next: PR-C (conformance shim + interop vectors).
🤖 Generated with Claude Code