Repository navigation
authority(0.7.1g1P2-T7): pin the fresh-measurement authority context at both levels - #476
Conversation
|
Review hold on #476: the audit-first decomposition is valid, but the current T7 'real-task' claim stops one layer short. The verifier-level test is good: However, The real production path is in There is already a suitable real-task harness in Please add one T7 discriminator there (or an equivalently real Gradle-task integration test) that makes candidate committed state disagree with the fresh measurement while preserving a tempting authority value where possible, and proves the task withholds the proof/fails rather than accepting candidate-supplied context. A particularly strong case is a candidate-side raw-status change that leaves the authority projection semantics unchanged but breaks the exact fresh comparison: the task must still produce no trusted proof. Also: #476 is correctly a sub-slice of the Step 3c T1–T19 closure, not Step 3c completion. Keep the umbrella completion gate unchanged: T1–T19 must all be mapped and proven before the goal is called complete. The audit finding about the unrelated compiler-warnings M13 is good evidence hygiene. Named rule-id hits should remain leads, not closure by themselves; each T row still needs scenario-level mapping to its canonical preconditions/outcome. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
It is a tests-only, additive change whose assertions and loader paths I verified against the production verifier and the real task, with no detected issues.
Review effort: Balanced
Findings: None
What changed in this PR
This PR closes the T7 gap identified in the §7 authority-matrix audit: the property that the mutation-ratchet authority context (consumed by M34/M44/M47) is pinned to the verifier's own fresh measurement and can never be supplied by a candidate/edited ledger previously had zero test coverage. It adds evidence for that property at both the verifier level (synthetic fixtures) and the real-task level (the committed mutation-baseline.json through the real loader path), with no production changes.
Changes:
- Adds a verifier-level test asserting an exactly matching candidate obtains a proof while any candidate-side difference (including a field the authority projection ignores) yields
null— proving no partial trust. - Adds a real-task-level test over the real committed baseline: identical round-trip yields a proof, a one-row tampered baseline yields none.
| File | Description |
|---|---|
| build-logic/src/test/kotlin/dev/tramai/build/quality/MutationPopulationEvolutionProofTest.kt | New T7 test pinning the authority context to the fresh measurement at the verifier level, covering the match and mismatch paths. |
| build-logic/src/test/kotlin/dev/tramai/build/quality/CertificateCustodyTransportTest.kt | New T7 test over the real committed mutation-baseline.json, proving an edited ledger cannot supply its own authority context. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Superseding the "real-task half" claim in this PR bodyThe body above says the It called What now exists (head
|
Why this PR is red — the tests are greenSingle failing check: Mutation authority transport, step The step's own echo, from the failing job ( No test failed. The three tests in CauseThis PR adds a third authority-transport test to that class: the T7 discriminator that runs the real [ "$executed" = "2" ] || { echo "::error::expected exactly 2 authority-transport tests, got $executed"; exit 1; }The guard's purpose — a vacuous 0- or 1-test run must fail loudly — is unaffected by there being three. Why I have not changed it
Options
Recommendation: option 1. Holding for the owner's decision — the T7 test stays in place rather than being stripped to make the check green. |
…zen count (#477) The mutation-authority-integration step "Assert the proof was not vacuous" hard-coded `executed = 2`. Step 3c's T7 discriminator adds a third authority-transport case to the integration class that proves the real transport (verifyMutationRatchet -> RECORDED_EVOLUTION -> nested canonicalMutationProbe -> fresh PIT -> exactComparison -> proof -> consumers), which is exactly where that evidence belongs. The step then failed on its own cardinality contract: executed=3 failures=0 errors=0 ::error::expected exactly 2 authority-transport tests, got 3 The step's name is a minimum-execution invariant, not a frozen suite cardinality. Make the code say what the name says: [ "$executed" -ge 2 ] A floor and not `= 3`, because this workflow PR must be independently testable against the epic it targets - which still has two tests until #476 merges. `= 3` could not pass against its own base, which would defeat the point of separating the workflow change. Verified on this exact base, replaying the step's own commands and logic: :build-logic:canonicalProbeIntegrationTest --tests '*MutationPopulationAdmissionIntegrationTest*' -> TASK_RC=0; exactly one report matched; executed=2 failures=0 errors=0 -> executed=2: old [ = 2 ] PASS, new [ -ge 2 ] PASS -> executed=1 and executed=0 (hypothetical): PRESSERVE FAIL under BOTH guards -> new guard on the current base: PASS (independent) The step still fails loudly when the task selects nothing, which is the entire reason it exists. verifyChangePolicy PASSED, class ci-workflow. Workflow-only, per .github/AGENTS.md rule 5.
…at both levels T1-T19 closure, starting from an audit of the test sources against the §7 matrix rather than an assumption. Auditing each row's expected rule id inside the test bodies showed coverage for T1, T4, T5, T11, T12, T13, T14, T15, T16, T17, T18 and T19 - and exactly one gap: T7, the row the design annotates "transport test: the fresh path is used" and the review called critical. authorityProjectionHash appeared in zero test bodies. The property already holds structurally: MutationPopulationEvolutionProof has a private constructor, is built only inside the verifier's own exactComparison from the fresh side (fresh.authorityProjectionHash()), and only when the fresh measurement matches the committed candidate everywhere. What was missing was the evidence, not the mechanism - which is precisely the difference between a property and a property that is pinned. - MutationPopulationEvolutionProofTest: T7 where the property lives. An exactly matching candidate obtains a proof; every candidate-side difference yields null instead of a partially trusted proof, including a raw-status change - a field the authority projection deliberately ignores. There is no partial trust by construction, and the test now says so. - CertificateCustodyTransportTest: T7 over the real committed baseline through the real loader. Identical -> proof; one fabricated row -> no proof. An edited ledger cannot put its own authority context in front of M34/M44/M47. Tests only: no production change, no rule, threshold, suppression or baseline touched. Verification: 179 tests / 0 failures across the proof, custody, certificate, admission and ratchet suites; spotless clean (hand-fixed); Detekt baseline 4792 -> 4792 with 0 added; verifyChangePolicy PASSED against base c4d3c84, class build-logic. Not claimed: T2, T3, T8, T9, T10 remain unverified row-by-row, and the single audit hit for M13 is the compiler-warnings M13 - a different namespace, not the PIT-status rule. Those rows are next.
… by calling exactComparison
Review finding: the previous "real-task half" called MutationPopulationEvolutionProof.exactComparison
directly. It never ran verifyMutationRatchet, so it could not show that the production transport -
RECORDED_EVOLUTION, nested canonicalMutationProbe, fresh PIT population, proof, verifier - establishes
the authority context. Calling it transport evidence overstated it.
The discriminator now runs inside the existing integration harness, on the transition that passes
unperturbed:
checkout(consumedCandidateSha) # a PASSING transition
flip ONE raw PIT status in the committed candidate baseline
assert authorityProjectionDigest(committed) == authorityProjectionDigest(displaced)
commit the perturbation # provenance gate requires a clean worktree
verifyRatchet() # real verifyMutationRatchet, fresh nested PIT
assert the task FAILS with
"no proof of the canonical fresh measurement exists ... Admission fails closed"
The authority-equivalence assertion is the load-bearing one: authority-v2 excludes raw status, so a
task that accepted a candidate-supplied authority digest would still pass. Asserting failure is
therefore evidence that the FRESH exact measurement established the proof.
Three things this test cost, each fixed rather than papered over:
- The perturbation must be COMMITTED. An uncommitted edit leaves the fixture worktree dirty, the
measurement's provenance gate refuses, and the task fails for that reason instead of on authority -
and it poisoned the sibling tests that checkout their own SHAs.
- The target row must not be an identity this transition authorises. Flipping an authorised row fires
M32/M37, which is the status-inclusive row binding (T3), not T7.
- `byFamily` is derived truth: moving raw status without moving the counters is a hand-edited ledger
and fails closed on that. The counters move with the row.
The test also asserts `byFamily` and `M32` are ABSENT, so it cannot silently drift into proving a
different rule. M37 is deliberately allowed and documented: with no authority context the retained
admissions cannot be recognised as consumed, so it is the designed cascade of the same missing proof,
not an artifact of the perturbation.
Tests only: no production change, no rule, threshold, suppression or baseline touched.
Verification: :build-logic:canonicalProbeIntegrationTest - 3 tests, 0 failures (all three in the class
pass, including the two pre-existing transport tests); spotless clean; Detekt baseline 4792 -> 4792
with 0 added; verifyChangePolicy PASSED against base c4d3c84, class build-logic.
Scope note: @tag("integration") classes are EXCLUDED from :build-logic:test, so the affected-suite
tallies quoted earlier in this slice never contained this class. Verifier-level results stand; the
real-task evidence now comes from the task that actually runs it.
6ac2bea to
3ef3b7b
Compare
514f61a
into
epic/0.7.1-control-plane-authority
Base
c4d3c84a28ea8991f667f6cfddf771994557f927— the post-#475 epic tip.Why this slice exists
T1–T19 closure, starting from an audit rather than an assumption. I extracted the §7 matrix (19 rows) and, for each row, searched the test bodies in
build-logic/src/test/.../qualityfor the rule id the row names, recording which file/level asserts it. Result:Named coverage, both levels present:
MutationPopulationAdmissionCeremonyTest"T1 a neighbour's raw status movement does not invalidate an authorized consumption" (asserts M21/M32 too)MutationPopulationAdmissionCeremonyTest"M34 still fails a raw-v1 admission with no certified migration"MutationAuthorityDigestCertificateConsumptionTestM41 ×2, M42 ×2MutationRatchetDiscriminatorTest(verifier) +MutationRatchetAuthorityTest(real ratchet)MutationPopulationAdmissionCeremonyTest/IntegrationTest/WiringTestMutationPopulationAdmissionWiringTest"a retained authorization rewritten in a bound field fails M36"; byte identity verified mechanically in the P1M gateMutationAuthorityDigestCertificateConsumptionTest"a candidate-minted certificate cannot become a consumption fact" +CertificateCustodyTransportTest"T14 …" (real ledger)CertificateCustodyTransportTest"T17 …" (real ledger)CertificateCustodyTransportTest"T18 …" (real ledger)CertificateCustodyTransportTest(real ledger) +MutationRatchetAuthorityTestThe gap: T7. The design annotates it "fail (transport test: the fresh path is used)" and the review flagged it as critical — and
authorityProjectionHashappeared in zero test bodies.What T7 actually is here
Reading the implementation first, the property already holds structurally:
MutationPopulationEvolutionProofhas aprivate constructor(MutationRatchetVerifier.kt:816);exactComparison(:855), fromfresh.authorityProjectionHash()(:857) — the fresh side;diagnostics.isEmpty(), i.e. the fresh measurement matches the committed candidate everywhere.So the authority context cannot be candidate-supplied. It was simply never pinned, which is the difference between a property and a property with evidence.
Change (tests only — no production change)
MutationPopulationEvolutionProofTest— T7 at the level where the property lives: an exactly matching candidate obtains a proof, and every candidate-side difference yieldsnullrather than a partially-trusted proof — including a raw-status change, a field the authority projection deliberately ignores. No partial trust is the point.CertificateCustodyTransportTest— T7 over the real committed baseline (config/quality/mutation-baseline.jsonvia the real loader): identical → proof; one fabricated row → no proof. So an edited ledger cannot put its own authority context in front of M34/M44/M47.The second test is the "real-task" half the matrix asks for: it fails if the real baseline stops round-tripping through the verifier's own measurement path.
Verification
:build-logic:testforMutationPopulationEvolutionProofTest,CertificateCustodyTransportTest, and the certificate/admission/ratchet suites,--rerun-tasks— 179 tests, 0 failuresspotlessKotlinCheck— BUILD SUCCESSFUL (hand-fixed, nospotlessApply)verifyStaticAnalysis— BUILD SUCCESSFUL,4792 → 4792, 0 removed, 0 addedverifyChangePolicy -PchangePolicyBase=c4d3c84a…— PASSED, 2 changed files, classbuild-logicAudit honesty
Rows not verified row-by-row in this slice, and therefore not claimed: T2, T3, T8, T9, T10 (their expected rules are M34/M32/M13 — M32 and M13 have only single-file coverage in the audit, and the M13 hit is the compiler-warnings M13, a different namespace that must not be counted as the PIT-status rule). T6's loader-level fail-closed is covered by
MutationAuthorityDigestCertificateLoaderTestbut I did not map it field-by-field. Those are the next rows to close, one at a time, with the same evidence standard.