feat(launchpad): land #118 STEPs 4/6/7/8 — nonce + stages, escalate-only, dedupe, fixtures - #566
Conversation
Extends run_adjudication.py's adjudicate() with two things it did not previously do: - _verify_nonce() checks the top-level `nonce` against every report's own completion marker and raises NonceVerificationError naming exactly one of three refusals, in fixed order: "absent provenance" (no top-level nonce, or a report's marker does not parse), "mixed document" (reports disagree with each other -- wins over the third case when both apply), and "mismatched envelope" (reports agree with each other but not the top-level key). Runs after #117's own findings.validate, never before, and never invents or accepts a caller-supplied nonce. - The output now carries a top-level `stages` array: every entry present on input plus exactly one new {name: "adjudication", status, reason} entry. AlreadyAdjudicatedError refuses a document whose `stages` already carries an `adjudication` entry, rather than silently overwriting it. `status` is "complete" only when every finding has a verdict and the nonce was established -- STEP 6's total-refutation flag isn't built yet, so it is not one of the two live conditions today, but the computation is structured so that flag slots in later without a rewrite. Because findings.validate() already checks every report's marker nonce against the top-level key, every fixture that exercises the three nonce refusals through main() end-to-end is *also* caught there first, with its own generic (and, between "mixed" and "mismatched", indistinguishable) message -- so the three refusals' own distinct reasons are proven directly against _verify_nonce with hand-built documents, not observable through the CLI today. Both are tested: the dedicated check is real defence in depth per ADJUDICATION.md's own stated reasoning, and main()'s "exits non-zero, prints no document" contract holds either way. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…STEP 4) adjudicate() ran #117's findings.validate first, which independently rejects a document whose report markers disagree with the top-level nonce -- but with one generic per-report message that does not distinguish "reports disagree with each other" (mixed document) from "reports agree with each other but not the top-level key" (mismatched envelope). Since every nonce problem findings.validate can see is a problem _verify_nonce can also see, findings.validate always won the race, so the three distinct refusals STEP 4's plan requires to be observable end-to-end were provably unreachable through main() -- only testable by calling _verify_nonce directly. Fixed by running _check_not_already_adjudicated and _verify_nonce before findings.validate. findings.validate still runs before any finding reaches the judge loop -- STEP 3's actual guarantee -- just second now instead of first. Strengthened NonceVerificationEndToEndTests to assert the specific reason text in main()'s stderr (mixed document / mismatched envelope / absent provenance), not just exit code and empty stdout, so a regression that reverts the ordering fails a test again. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…ovenance (#118 STEP 4) review-code found that checking _verify_nonce before findings.validate (the previous fix in this branch) has a side effect: a document whose reports key is missing, non-list, or empty has nothing for nonce verification to compare against, so _verify_nonce calls it "absent provenance" -- technically true, but it buries findings.validate's more specific message for exactly that shape defect ("missing required key 'reports'", "must not be empty", "expected an array"), pointing a maintainer at the wrong subsystem. Fixed by checking reports is a present, non-empty list before running _verify_nonce at all; when it isn't, defer straight to findings.validate, which already names the real problem. The _verify_nonce call in that branch is unreachable in practice (a malformed reports always fails findings.validate on one of those three grounds) but kept as a real call rather than asserted away, matching this module's existing "real branch, not assumed" discipline for stage_complete's nonce_established condition. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
#118 STEP 6) Extend run_adjudication.py's adjudicate() with the three STEP 6 behaviours: a judge's return dict may now carry a severity re-rating, guarded so an out-of-ladder value it produces is refused (UNPROVEN at reported_severity, never published) rather than copied through; a genuine downgrade is recorded into adjudication.downgrades at the moment it is applied, never by a later sweep; and total_refutation now surfaces in the stages manifest's own status ("total_refutation", not "complete") rather than only in adjudication's own boolean. Also adds a belt-and-braces finding_id set-equality check inside adjudicate() itself, raising before the document is ever printed. Verified against STEP 6's own done-when: a REFUTED-everything judge leaves findings/findings_count unchanged and flips the stage status; the same judge against zero findings stays "complete"; a judge returning "Info" over a legally in-ladder reported_severity is refused with a reason and still passes verdicts.validate; a bare review.SEVERITY_ORDER[...] subscript succeeds on every finding in every output; and a Blocker-to-Low downgrade is named in adjudication.downgrades with from/to/reason. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…118 STEP 6) review-code found that _run_judge_safely forwarded a judge's severity value with no type check, and _apply_severity_rerating's `proposed_severity not in review.SEVERITY_ORDER` raises TypeError on an unhashable value (a list or dict) instead of failing closed to UNPROVEN -- reachable today through make_replay_judge (a malformed --replay recording), confirmed by reproducing the crash before fixing it. Fixed by only forwarding severity/severity_reason when severity is a str, matching the type discipline verdict/verdict_evidence already get in the same function. A non-string severity is now treated as no re-rating at all rather than crashing the whole run. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Groups findings that describe the same defect in different words, possibly from different review dimensions (finding_id differs by construction since dimension is a hash input), into adjudication.duplicate_groups with a deterministic survivor and a duplicate_of back-reference on every non-survivor -- discoverable from either the finding or the top-level block. The Judge protocol (judge(finding, input_document) -> dict) is called once per finding, independently, so it structurally cannot see across findings to detect a duplicate. Rather than reshape that existing, already-tested protocol, dedupe gets its own separate injectable callable, dedupe_judge, called once after every finding is adjudicated over the full list -- mirroring how the primary judge already defaults to stub_judge to prove the harness before a real model exists. stub_dedupe_judge finds no duplicates by default: never merging incorrectly is safer than merging wrongly. Survivor selection (highest severity, then CONFIRMED > UNPROVEN > REFUTED, then lowest finding_id) is deterministic code, independent of whichever mechanism decides who is a duplicate of whom. verdicts.validate already rejects a duplicate_of naming an absent id or itself (STEP 2); confirmed here with tests, not reimplemented. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…STEP 4) Adjudicated on #264 as the root cause inherited unchanged by #266 and #267. A `stages` value that was present but not a list was treated as absent at both sites that read it: `_check_not_already_adjudicated` returned early, and the manifest builder substituted `[]`. Two failures from one shape defect, reproduced through the real CLI: stages = [{"name":"adjudication",...}] -> exit 1, correctly refused stages = {"0":{"name":"adjudication",...}} -> exit 0, guard bypassed stages = {"p":{"name":"preflight","status":"blocked",...}} -> exit 0, entry GONE, "complete" stages = 42 / "adjudication" / True -> exit 0, dropped The second is the expensive one. A `blocked` pre-flight -- #116's fork-PR-secrets-withheld case -- disappeared and the document published as a clean, complete review, because #119 only banners a non-`complete` status. That is #118's fifth criterion failing through a shape defect no verdict-side check looks at: `grep -n stages findings.py verdicts.py` returns nothing, so neither contract validator inspects this key at all. "#117 never emits that shape" is not available as a defence here. The `stages` manifest is explicitly an output #117 does NOT produce, so there is no upstream guarantee for this stage to inherit. Fix: one `_input_stages` reader, raising `StagesShapeError`, used by BOTH sites. One function rather than two corrected inline checks on purpose -- the two readers each had their own `isinstance(..., list)` test and each treated malformed as absent, which is how one defect became two independent failures. The same lesson as `verdicts.is_nonempty_str` on #261/#263: a second copy of a rule is a second chance to disagree with it. The comment claiming `_check_not_already_adjudicated` "already guarantees" no input entry is named `adjudication` is now actually true, and says why. Absent stays legal, and an explicit null reads as absent -- #117 emits no `stages` key at all, so a fix that refused absence would break every real document. Both are tested as controls, as is a well-formed `blocked` pre-flight surviving in order. Also closes the Low at the same site: a `stages` entry that is not an object, or whose `name` is not a string, is refused in the same change. 11 tests added, each failing before the change, including the refusal driven through the real process. 57 tests in test_run_adjudication (was 46 after merging #263), 207 across launchpad/review-agent. #266 and #267 carry these two guard sites byte-identically, only line-shifted. Fixing here rather than there so one change propagates up the chain instead of becoming three copies that can diverge. Refs #118 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Brings in #264's `StagesShapeError` / `_input_stages` fix and #261/#263's `verdicts.is_nonempty_str` guard. This PR's inherited Blocker is now closed by propagation rather than by a second copy of the guard, which is the point -- two divergent copies of one check is worse than the bug. Verified on this branch through the real CLI after the merge: an `adjudication` entry inside an object container exits 1 with no document (was exit 0), a `blocked` preflight in an object container exits 1 rather than being silently discarded (was published as `complete`), and the control -- a well-formed list carrying a `blocked` preflight -- still exits 0 with both entries in order. Conflicts were docstring and error-class additions only; both sides kept. 219 tests across launchpad/review-agent. Refs #118 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…scalate' into feat/review-agent-adjudication-dedupe Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Brings #264's `StagesShapeError` / `_input_stages` and #261/#263's `verdicts.is_nonempty_str` up to the chain tip. Both of this PR's findings were homed on earlier branches -- the `stages` Blocker on #264 and the `notes` drift on #263 -- so this branch is cleared by propagation rather than by any change of its own, which is what the adjudication asked for. Clean merge, no conflicts. 230 tests across launchpad/review-agent. `notes` remains empty and is now documented as deferred at STEP 6/7, with the unresolved tension against `adjudicator.md` (#265) stated in the code. #265 should not merge ahead of that decision. Refs #118 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
STEP 6) benmitchell11's independent pass on #266/#267 found that _apply_severity_rerating's no-re-rating branch returned reported_severity without ever checking it, so an out-of-ladder value the judge agreed with (or said nothing about) was published untouched: _apply_severity_rerating("x", "Info", "CONFIRMED", "Info", None, []) -> ('CONFIRMED', 'Info', None) Reproduced before fixing. This is a plan-conformance gap, not merely defence in depth: STEP 6's own done-when names this exact case -- "a guard watching only re-ratings never sees a finding that ARRIVED at 'Info' and was agreed with, and copies it into `severity` untouched" -- and ADJUDICATION.md promises the guarantee holds on the EFFECTIVE severity, the re-rating where there is one and reported_severity where there is not. Fixed by checking reported_severity inside that branch, ahead of the return that used to copy it: verdict becomes UNPROVEN, severity falls back to Blocker (not something smaller -- this stage may not decide an unrateable finding is minor), severity_reason names the refusal, and nothing is added to downgrades since no legal value fell. Unreachable through main() today (STEP 3's findings.validate refuses an out-of-ladder input severity before any judge runs) and kept as a real branch regardless: adjudicate() is importable, and STEP 10's control suite is planned to feed this function malformed values directly. Proved the new tests can fail: removing the guard on a scratch copy fails exactly the two new sub-cases while the legal-path control still passes. 71 tests OK, run_controls 13/13, all seven suites OK. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…ed (#118 STEP 8) (#450) * feat(launchpad): adjudication fixtures, four of five genuinely produced (#118 STEP 8) STEP 8's stated premise was dead and is corrected in the plan (struck through, not deleted) exactly as its own BUDGET section instructed: "#117's producer does not exist -- there is no run_dimensions.py anywhere" has not been true since #117 merged. Fifteen real recorded reviewer outputs live under recordings/, and test_recordings.py's own ReplayValidityTests already replayed them through build_document, so the harness this step needed was already in the tree. Measured before building, not assumed. Four of the five named behaviours are now genuinely produced by replaying real recorded output through the real producer: - paraphrase gives BOTH the three-report all-anchor-line document and the dedupe document -- all three dimensions independently reported the same defect at gate.rs:42 with three different finding_ids, since dimension is a hash input. One document isolates both behaviours; committing it twice, or inventing a second, would be the dishonest alternatives. - claim-vs-evidence gives the pr-anchored fixture. Its real output is two findings (anchors line and pr); kept whole per Serina's call rather than trimmed to the single-finding shape the plan first described, since a trim would be a real replay with a finding deleted by hand. - secrets-and-access plus a reviewer injected to raise gives the failed/clean/findings document -- the failed report comes from _collect_report/_failed_report, not from hand-written JSON. The containment fixture is the one genuine exception and the split is stated rather than blurred: no existing fixture trips the detectors (all eight checked, every one yields zero containment findings), so its surfaces are crafted and its containment block and states map are then produced by the real contain.render. Crafted input, real pipeline -- never described as recorded. Regenerating reproduces the committed bytes exactly, which is what makes the provenance claim checkable rather than asserted: every nonce derives from the relevant recording's own _provenance.seed via contain.make_nonce(seed=...), never freshly randomised. Verified: 18 new tests OK; each fixture accepted by findings.validate with zero violations; each accepted by run_adjudication.py at exit 0; containment fixture carries all three kinds and exactly seven states keys; regeneration byte-identical; run_controls 13/13 and all eight suites OK. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com> * docs(launchpad): drop a citation a reader cannot open (#118 STEP 8) review-code flagged, as context rather than a finding, that generate.py attributed the keep-the-two-finding-document decision to "Serina's call recorded in this repo's session notes" -- and no such artifact exists in the repo to check it against. That is precisely the uncheckable citation this plan's own conventions exist to prevent: its ALREADY TRUE section carries a standing rule that evidence is what `git show` returns from a named commit, adopted after three separate rounds of cross-issue claims that were true-then- falsified, never-true, and true-but-misattributed. A pointer to out-of-repo session notes is weaker than all three. Replaced with the argument itself, made from what the recording actually contains: trimming would turn a real replay into a real replay with a finding deleted by hand, and the two-finding document exercises a 'pr' anchor alongside a 'line' one rather than in isolation. That reasoning is checkable against the fixture; the attribution was not. Regenerated so the committed fixture carries the corrected note. 18 tests OK (including the byte-reproducibility check), controls 13/13. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com> --------- Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
PROVENANCE.md claimed byte reproducibility was "what makes 'real' and
'crafted surfaces, real pipeline' checkable claims rather than
assertions". It is not, on its own: it compares generate.py against
itself. Rewrite the generator to type finding content by hand,
regenerate, commit, and the new bytes agree with the new generator
perfectly while `real: true` and `source_recordings` have quietly become
false. Demonstrated against this directory, not imagined -- a generator
reading nothing under recordings/ passed all 18 tests.
The claim is true today: every replayed report's findings and outcome are
equal to the recording it names, checked across all 8 replayed reports,
0 mismatches. That is exactly why the guard is cheap to add now and
expensive to add after STEP 9 records judge outputs against these
fixtures.
Adds RealDocumentsReplayTheirNamedRecordingsTests, which reads the
recordings rather than the generator:
- every replayed report's `findings` and `outcome` must EQUAL the
recording named in `_fixture.source_recordings`
- every real document's `nonce` must be `contain.make_nonce(seed=...)`
over a seed that recording actually records -- the one field a
fabricated generator cannot get right by copying shapes, since
inventing a seed changes it
- named recordings must exist, and must actually be replayed, so
provenance cannot inflate in either direction
- a guard on the guard: zero real documents, or zero comparisons,
fails rather than passing vacuously
Adds ProvenanceNoteRecordsTheHonestySplitTests, for the same reason
test_recordings.py pins #117's sampling disclosure: the old
test_provenance_note_exists was os.path.isfile only, so truncating
PROVENANCE.md to a single header line -- taking the whole real-versus-
crafted accounting with it -- stayed green.
Proved each guard can fail, in scratch copies:
- fabricated seed, fixtures regenerated: 3 failures, one per real
document, while byte reproducibility still PASSED -- the blind spot
stated plainly
- fabricated finding content, fixtures regenerated: 5 failures, one
per replayed report
- PROVENANCE.md truncated to "# PROVENANCE": old test still passes,
new pin fails 8 times
Also corrects two sentences that asserted adjudicator behaviour the code
does not produce. The test comment and line-anchored-findings.json's own
note said the document "exercises adjudicate()'s dedupe path"; the
default stub_dedupe_judge groups nothing by design, so the real CLI emits
`duplicate_groups: []` with every `duplicate_of` null. It is a dedupe
CANDIDATE -- three distinct finding_ids describing one defect at
crates/buzz-relay/src/gate.rs:42 -- and asserting on the grouped output
is STEP 10's remit, not this fixture's.
257 tests OK (was 250), run_controls.py 13 passed 0 failed 0 skipped.
Three of the four fixtures regenerated byte-identically; only the one
whose note changed differs.
Reviewed by review-code, review-tests and review-adjudicate before this
commit. PR #450 merged STEP 8 with zero reviews; these were its first.
All three passes were Claude, so this is not the cross-model review the
final gate wants.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…#118) ADJUDICATION.md's `stages` section carried the wording #119's own plan superseded on 2026-08-14 (8d47f87) and named as a defect: the manifest is "for stages that produce no envelope of their own (#116's pre-flight, and this one)". #119's plan STEP 5 says the opposite, and says why -- built to the old definition the manifest names no dimension, so its condition (7) can never fire and a three-dimension run that produced two reports renders as COMPLETE. The revision landed seven days before STEP 4 was built. This plan's own OPEN section flagged it as "the item to re-check before STEP 4 is built rather than after"; no artefact records that re-check happening, and the contract went on asserting the dead definition. Amends the section rather than rewriting it: the superseded sentence is struck through, #119's corrected definition is quoted from its own plan, and the consequence is stated -- until per-dimension entries exist, condition (7) cannot fire and a run that loses a whole dimension can still render as complete. Also records why this stage must NOT be the one to fix it. Deriving entries from `reports[].dimension` would name only the dimensions that did report, so condition (7) still could never fire -- a report cannot testify to its own absence, which is the case (7) exists for. The expected set is known only to run_dimensions.list_dimensions(), which enumerates dimensions/*.py before dispatch, so producing the entries is #117's job. Filed separately. No behaviour change: this stage still adds exactly one `adjudication` entry and passes the rest through. 257 tests OK, run_controls.py 13 passed 0 failed 0 skipped. Found by review-final over the whole branch, which returned NOT READY on this and on the CI-reachability gap. The reviewer's proposed fix was to derive the entries in adjudicate(); that would defeat the condition it aimed to restore, which is why this commit corrects the contract and assigns the behaviour elsewhere instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Post-gate review — CI green, verdict still NOT READYNot an independent review. I ran the review pipeline on this branch and wrote the two fixes it produced, so this is a record of what was measured, not a second opinion. It is a comment, never an approval — matching #109's own escalate-never-approve design. CI, measured after it settledNewest run per check name. Seven real checks, all green. What green does not cover here
So the green That is the gap #270's exclusion paragraph would have caught, on a premise that does not hold — evidence posted there. VerdictNOT READY, unchanged by CI going green. Two things stand between this and non-draft:
Not blocking, filed rather than fixed: #565 ( Also still true, and worth a maintainer's eye rather than mine: the 🤖 Posted by Claude Code ( |
…118) Found by a cross-model (Codex) review pass, after four same-model passes over this code did not raise it. `adjudicate()` called the judge with the LIVE output finding and read `reported_severity` back out of that same object AFTERWARDS. The escalate-only guarantee is enforced by inspecting what the judge RETURNS, so handing it the mutable object those checks are about let it route around every one of them at once. Reproduced against a real fixture before fixing. A judge doing `finding["severity"] = "Low"` and returning only `{"verdict": "REFUTED", "verdict_evidence": ...}` turned three input Blockers into: verdict=REFUTED reported_severity=Low severity=Low reason=None downgrades: [] verdicts.validate: NO violations So it downgraded three Blockers to Low, left no downgrade record, and falsified `reported_severity` -- the field whose entire job is to record what was reported -- while passing the contract check. That defeats the one property #109 rests on: that this agent can only escalate, so a weak judge is a throughput and credibility cost rather than a security regression. The fix copies at the boundary rather than guarding fields one at a time: - `_run_judge_safely` passes `copy.deepcopy(finding)` - `_run_dedupe_safely` passes `copy.deepcopy(adjudicated_findings)` -- by then every finding carries its final verdict and severity, and nothing re-reads them afterwards - `reported_severity` is captured BEFORE the judge runs, not after; belt as well as braces, and it makes the guarantee readable at the call site Severity is not only about severity: `finding_id` is what the input/output set-equality check is keyed on, so an in-place edit there defeated that check too. Both are covered. Adds `JudgeCannotMutateWhatItIsJudgingTests` (4 tests), including a guard-the-guard case asserting the judge's REFUTED verdict is still honoured -- refusing to let a judge edit the finding is not the same as refusing its conclusion. Proved all four can fail: reverting the three changes in a scratch copy turns them red (1 failure, 3 errors). Scope, stated honestly: a judge is injected Python, not model output, so exploiting this needs a hostile or buggy judge implementation rather than a prompt injection. STEP 6's own premise is that the prohibitions hold in code regardless of what the judge does, and a guard the guarded component can step around is not one. 261 tests OK (was 257), run_controls.py 13 passed 0 failed 0 skipped. Codex also reported: the fixture-provenance oracle is itself mutable (edit the recordings, recompute ids, regenerate) -- not fixed here, see the PR discussion; `severity_reason` is forwarded without type validation and `main()` does not run `verdicts.validate` on its own output; and `_verify_nonce` accepts any self-consistent string rather than checking the 128-bit lowercase-hex shape. Those three are reported, not fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Second finding from the cross-model (Codex) pass.
`_run_judge_safely` gave `verdict`, `verdict_evidence` and `severity` a
type guard each, then forwarded `severity_reason` unchecked. A judge
returning `severity_reason={"approved": True}` put a forbidden `approved`
key into both the finding and its downgrade record:
severity_reason : {'approved': True}
downgrades[0] : {..., 'from': 'Blocker', 'to': 'Low',
'reason': {'approved': True}}
`verdicts.validate` does catch this -- 9 violations, including
"forbidden key present -- no field may carry an approval". But `main()`
never calls it, so the CLI printed the document and exited 0. This
module states the rule itself, for the finding-set integrity check: "a
stage that can print a lossy document and rely on a downstream
`verdicts.validate` call to catch it has already lost the document once."
So it is refused at the producer, beside the guard it was missing from.
An unusable reason fails closed on the RE-RATING too, not only on the
reason -- `_run_judge_safely`'s own docstring already says "failing
closed means failing closed on both". Forwarding a severity change while
dropping its justification would manufacture exactly the contract
violation the check exists to refuse, since `severity_reason` is required
whenever `severity != reported_severity`.
Deliberately unchanged: a judge supplying NO `severity_reason` key at
all. That is the existing missing-reason path, which generates a default
reason and stays legal; touching it here would be a behaviour change
Codex did not report and no done-when asks for.
Verified across four judge returns, all 0 violations after the fix:
nested dict reason -> re-rating refused, severity stays Blocker
blank/whitespace -> re-rating refused, severity stays Blocker
legal re-rating -> honoured, reason preserved
no reason key -> unchanged from before
Adds `UnusableSeverityReasonFailsClosedTests` (4 tests), including a
guard-the-guard case proving a legitimate re-rating still applies.
Reverting the guard in a scratch copy turns 3 of the 4 red; the fourth is
the legal-re-rating case, which correctly passes either way.
265 tests OK (was 261), run_controls.py 13 passed 0 failed 0 skipped.
Still reported and NOT fixed, both from the same Codex pass: the
fixture-provenance oracle is itself mutable (edit the committed
recordings, recompute ids, regenerate, and the guard added in 682900c
still passes -- it establishes agreement with the recordings, not their
authenticity); and `_verify_nonce` accepts any self-consistent string
rather than checking the 128-bit lowercase-hex shape `contain.make_nonce`
produces. Neither is fixed here because both are design calls about what
the oracle should be, not omissions from an existing guard.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Cross-model review — Codex, and it found what four Claude passes did notRun 2026-08-24 with This is the pass the standing cross-model rule asks for, and it justified itself immediately: it found a Blocker in the load-bearing invariant that 1. Blocker — a judge could rewrite the finding it was judging. FIXED in
|
) Escalates 5bb984b from a local fix to a stated rule, because the bug it fixed was not a coding slip -- it was a condition the escalate-only contract depended on and had never written down. All three prohibitions in ADJUDICATION.md's "Escalate, never approve" are enforced by inspecting what a judge RETURNS. That is only sound if the judge cannot reach the record directly, and it could: `adjudicate()` passed the live output finding and read `reported_severity` back out afterwards, so a judge doing `finding["severity"] = "Low"` published three Blockers as Low with an empty `downgrades` and NO `verdicts.validate` violation. Prohibitions 1 and 3 were both defeated without either being violated as written -- which is why no amount of care in the checks themselves would have caught it. Adds it as § 4 of that section, and states the rule for every stage rather than for this one: A callable injected into any stage of this pipeline receives immutable input, or a deep copy. Never the object the stage will go on to publish, and never the object its own guards will be evaluated against. Records that #117 already had this right and this stage diverged: `run_dimensions._run_reviewer_into` takes `document: str`, so a reviewer there can influence the run only through its validated return value. `adjudicate()` took a dict, and a dict handed to injected code is a shared mutable reference, not an argument. Both call sites READ as "call the injected thing with the thing it should look at", which is why four same-model review passes did not separate them. Two consequences stated because both were tempting and both are wrong: copying is the enforcement here rather than defence in depth (a guard that reads its subject after invoking the component it guards against is not a guard); and it is not only about severity, since `finding_id` is what the input/output set-equality check is keyed on. Also amends the section's opening line, which promised "three concrete prohibitions" and now introduces the structural condition they turned out to rest on. Nothing asserts that count in code -- checked. Filed #568 to audit the pipeline's other injection points against the rule and add a control that enforces it, rather than trusting review to notice next time. Recorded there, accurately: `preflight_fetch._read` hands its injected runner a mutable list but never re-reads it, so there is no defect at that site today -- it is one edit away from one. No behaviour change; documentation only. 265 tests OK, run_controls.py 13 passed 0 failed 0 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
tucktuck101
left a comment
tucktuck101
left a comment
There was a problem hiding this comment.
Review panel — two independent reviewers (Fable, gpt-5.6-sol xhigh), consolidated
Summary: This is strong work — both reviewers independently ran the full suite (265 tests OK), Fable mutation-tested five load-bearing guards (all go red on reversion) and found every STEP's done-when clauses met; codex reconstructed the merge result and ran adversarial probes. The panel split on verdict (Fable: comment; codex: not ready). The consolidator verified codex's two blockers — one is visible in the diff text, both were reproduced by probe — so the verdict is request changes. Both blockers are narrow and the fix shapes are obvious.
Blocking
- Injected callables receive the live, mutable
input_document— the mutation defense covers the wrong argument. The judge invocation isjudge(copy.deepcopy(finding), input_document)and the dedupe invocation isdedupe_judge(copy.deepcopy(adjudicated_findings), input_document)(run_adjudication.py:601,:800): the finding is copied, the document is not. The runner later re-readsstagesfrom that same object (:1043) and evaluates its integrity guard against it (:1095). Codex reproduced a judge appending anapproved: trueadjudication stage todocument— the returned output carried the injected approval-bearing entry and the caller's input object was mutated, violating both this PR's own new ADJUDICATION.md §142 rule ("immutable input, or a copy") andadjudicate()'s "Never mutatesinput_document" docstring. The regression tests (test_run_adjudication.py:1020,:1071) mutate only the copied first argument, so they cannot see this. Fix: deepcopy the document at the same boundary (or pass a read-only view), and add a test that mutates the second argument. - An otherwise-valid #117 finding carrying
approved: truesails through the CLI at exit 0.findings.validatepermits extra keys,run_dimensions.py:248doesn't strip them, adjudication deep-copies the whole finding into the output (run_adjudication.py:964), andmain()prints without ever callingverdicts.validateon its own output (:1165,:1181) — the one validator that carries the whole-document forbidden-key walk (verdicts.py:548). Codex probed the real CLI:findings.validate == [], exit 0,approvedemitted. Since #117 findings are model output influenced by author-controlled PR text, this is the exact channel "escalate, never approve" exists to close. The new:1124test covers a forbidden key inside a judge-returnedseverity_reason, not one already present in valid input. Fix: runverdicts.validateon the output inmain()before printing (fail-closed), or strip unknown keys at the ingestion boundary — and pin it with a test.
High
- CI runs none of the 265 tests. Both reviewers verified the controls workflow's only invocation is
python3 run_controls.py(.github/workflows/launchpad-review-agent-controls.yml:48), and both new suites say they're unregistered. Disclosed in the PR and escalated on #270 — carried here because for a PR landing enforcement boundaries, "the guard exists but nothing runs it" is a merge-readiness fact the approver must own consciously.
Medium
- The PR re-asserts contract text it (or the merged base) supersedes, in three places. (both reviewers, overlapping instances) (a)
run_adjudication.py:169-171re-introduces verbatim the supersededstagesdefinition this same PR strikes from ADJUDICATION.md — the corrected definition is atADJUDICATION.md:220; (b) against the merged base,run_adjudication.py:112/:119still saysadjudicator.mdtells the judge to populatenotes, while mergedadjudicator.md:72now says the opposite, andtest_run_adjudication.py:479pins the stale account; (c)test_run_adjudication.py:13-25's module docstring describes the nonce/validate ordering backwards — it claimsfindings.validateruns first, contradicting the code's "deliberately reordered" comment and the end-to-end tests themselves. All prose-only; in this codebase the prose is load-bearing, and (b) is the very drift class #265's merge was supposed to close. _verify_nonceaccepts valuescontain.make_nonce()could never produce. (both reviewers) CONTAINMENT.md:31 requires 128-bit lowercase hex;run_adjudication.py:412checks only truthiness + agreement, and codex confirmed a document using"attacker-chosen"exits 0 and emits that nonce. The tests'"N1"/"N2"marker nonces mean they can't pin the shape. Doesn't authenticate a forged document either way, but it fails the documented producer-shape check — fix here or file it explicitly with CONTAINMENT.md's owner.- STEP 8's amended plan still claims byte-regeneration makes provenance checkable (
2026-08-13-issue-118-adjudication.md:750), whilePROVENANCE.md:88correctly says it proves only generator determinism, and the STEP 12 clause at:919still requires reporting a now-false premise. State the narrower guarantee (agreement with mutable committed recordings) or pin an immutable oracle.
Low
- The dedupe priority test named "confirmed over unproven over refuted" (
test_run_adjudication.py:1578) never comparesUNPROVENvsREFUTED— codex swapped their ranks in memory and all 11DedupeTestsstayed green. Add the equal-severity case. - A non-dict entry inside
reportssurfaces as "absent provenance" instead offindings.validate's more specific message — the exact message-burying the deferral comment says it avoids. Fail-closed either way; messaging only. (Fable) run_adjudication.py:178-181says "STEP 6's total-refutation flag does not exist yet" — it lands in this same PR, ~120 lines up in the same docstring. (Fable)
What looks good (both reviewers, independently)
- The test suite is the strongest part: Fable reverted five guards (judge deepcopy, severity_reason fail-closed, total-refutation status, nonce verification, malformed-stages refusal) and every reversion went red — these tests genuinely pin behaviour.
- All four STEPs meet their done-when clauses (Fable's per-clause table checked all of STEP 4's eight, STEP 6's escalate-only + downgrade recording, STEP 7's two-ended duplicate metadata with claimed-set discipline, STEP 8's corrected four-document consolidation).
- Fixture regeneration is byte-identical to the committed files (verified locally); the crafted/real provenance split is stated in four places and test-pinned, with no blurring.
- Fail-closed direction is consistent everywhere: judge crash → UNPROVEN, dedupe garbage → no duplicates, malformed
stages→ refuse. - The
severity_reasonfix correctly fails closed on the re-rating too, not just the reason.
Verdict: request changes — blockers 1 and 2, plus the prose drift in finding 4 (a)–(c) while you're in the file. Everything else can ride along or become tracked follow-ups.
Panel: fable + gpt-5.6-sol (codex, xhigh), independent reviews consolidated by the lane; the consolidator verified both blockers (one in the diff text, both reproduced by codex probes) before issuing the verdict. Posted under Jeff's standing authorization.
…118) Found by re-running the trial merge after `launchpad` moved 88 commits ahead. The merge is still conflict-free and there is no file-level overlap -- which is exactly why this needed looking for: the two halves live in different files, so a clean merge cannot see it. `run_adjudication.py`'s docstring said: **This deferral is in tension with ``adjudicator.md`` (#265), which normatively tells a judge to "record it in ``adjudication.notes``".** `adjudicator.md` was amended on `launchpad` in `05a960478` ("stop mandating a notes channel the runner discards"). It now says the channel is deferred and a judge "must not rely on it", and records that its own earlier paragraph said otherwise. So the claim above is already false on the merge target: this branch would have landed a docstring describing a document in the same directory as saying the opposite of what it says. Rewritten to record that the tension is resolved, and in which direction, rather than deleting the paragraph -- the two facts that outlived the tension are the reason it existed: - a judge's only free-text outlet is still `verdict_evidence`, the field ADJUDICATION.md identifies as having no structural guard - "deferred to STEP 6/7" was already wrong on its own terms: both steps are in this branch and neither plumbed `notes` So the field is now described as what it is -- empty, with no step plumbing it. Plumbing it remains unowned. Calling it "deferred to STEP 6/7" made a live gap look like a scheduled one. Also loosens the test that pinned the stale sentence. `test_run_adjudication.py:507` asserted `notes.*(defer|STEP 6/7|left empty)`, which accepted the wrong wording -- so correcting the docstring meant editing a green test, making a fix look like a regression. It now asserts the FACT (the field is empty and the docstring says so) and additionally asserts the stale shape does NOT return, so the claim cannot silently regain a scheduled-sounding owner. A test that pins prose it cannot verify is a test that defends a stale claim. Proved the new one is load-bearing in both directions, in scratch copies: restoring the "deferred to STEP 6/7" wording turns it red, and deleting the notes paragraph entirely turns it red. 265 tests OK, run_controls.py 13 passed 0 failed 0 skipped. Trial merge onto `9e39bab36` conflict-free. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…ring document (#118) Both blockers from tucktuck101's review panel (Fable + gpt-5.6-sol) on PR #566. Both reproduced here before fixing, and both are correct. BLOCKER 1 -- the mutation defence covered the wrong argument. 994c85d added a rule to ADJUDICATION.md saying an injected callable gets "immutable input, or a deep copy", and 5bb984b copied the FINDING while passing `input_document` through live. That satisfied the sentence as written and left the hole it describes open. The runner re-reads `stages` from that document and evaluates its integrity guard against it, so: judge appends {"name":"approval",...,"approved":true} to document["stages"] -> output stages: 3 approval-bearing entries + adjudication -> caller's input_document mutated, against adjudicate()'s own "Never mutates input_document" docstring -> "approved" present in the emitted document The regression tests mutated only the copied first argument, so they could not see it -- the panel said so explicitly and was right. Now both arguments are copied at both judge boundaries, per call rather than once per run, so one finding's judge cannot alter what the next one sees. The rule in ADJUDICATION.md now says "in every argument" and records why that phrase is load-bearing. BLOCKER 2 -- an approval could arrive on the INPUT and be printed. `findings.validate` permits extra keys on a finding, #117 does not strip them, and this stage deep-copies whole findings into its output. So `approved: true` planted on an incoming finding reached stdout at exit 0: findings.validate -> [] (no violations) run_adjudication.py < tainted.json -> exit 0, "approved": true emitted `verdicts.validate` carries the whole-document forbidden-key walk and does catch it -- but `main()` never called it. This module's own rule applies: "a stage that can print a lossy document and rely on a downstream verdicts.validate call to catch it has already lost the document once." Now checked inside `adjudicate()`, beside the finding-set integrity check and for the same reason, via a new public `verdicts.forbidden_keys()` rather than a second copy of the walk. Raises `ForbiddenKeyError`; `main()` exits 1 having printed NOTHING to stdout -- a non-zero exit that still printed the document would leave the approval there for anything piping it. Refused, not stripped. Stripping emits a document that looks clean and silently discards the evidence that something upstream tried to plant an approval. The nonce failures already set the house behaviour for "cannot honour this input": non-zero, no document. Since #117 findings are model output shaped by author-controlled PR text, this was the exact channel "escalate, never approve" exists to close. Verified after the fix: judge document-mutation leaves output stages ["adjudication"], input unmutated, no "approved" anywhere; tainted input gives exit 1, 0 bytes on stdout, and a named violation on stderr. Adds 6 tests -- second-argument mutation for both judges, input-planted `approved`, a key the protocol cannot filter, the CLI's stdout emptiness, and a guard-the-guard case proving clean documents still pass. Reverting both fixes in a scratch copy turns 5 of them red (4 failures, 1 error); the guard-the-guard cases correctly pass either way. 271 tests OK (was 265), run_controls.py 13 passed 0 failed 0 skipped. Note for the panel: the ADJUDICATION.md §4 rule this violated was added in the same PR that violated it. Writing the rule and half-applying it in one branch is its own lesson, and §4 now carries it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Both blockers confirmed and fixed in
|
Merging this outside the review-final gate — a decision, not an oversightRecording this so the next reader sees a choice rather than a gap. What was skipped. Why, and why it is not a quality judgement about this branch. The verdict is unreachable here, for two independent reasons:
So there is no sequence of correct actions that produces a READY verdict for a partial landing. Writing a What this branch did get instead. The gate's purpose is to stop unreviewed work merging. That did not happen here:
Seven defects found and fixed, each with a regression test proven to fail without its fix. 271 tests OK, The gate design is now filed as #574 ( Not carried by this merge, all recorded in the comments above and deliberately left as decisions rather than defects:
@tucktuck101 — your panel's two blockers are fixed in |
2 participants
Summary
Lands #118 STEPs 4, 6, 7 and 8 on
launchpad— the nonce check andstagesmanifest, escalate-only enforcement with the total-refutation flag, dedupe via an injectable second judge, and the four adjudication fixtures with their provenance accounting.This PR exists because a stack of PRs all reported MERGED without any of this reaching
launchpad. STEPs 4/6/7/8 each had a PR — #264, #266, #267, #450 — and each merged into another feature branch. The chain collapsed dedupe → escalate → nonce → run → verdicts →launchpad, then #450 merged STEP 8 into thededupebranch afterdedupehad already merged forward, so the chain terminated. Only STEPs 1, 2, 3 and 5 actually landed.Measurable, if you want to confirm rather than take it:
launchpadrun_adjudication.py"stages"occurrencesnonceoccurrencestotal_refutationOpened as a draft deliberately.
review-finalover the whole branch returned NOT READY, and while both of its blocking findings have been acted on, one of them is not fixed — see § Review outcome. A draft rather than a manufactured READY verdict.Related issue
Refs #118
Issue type
Task
Agent provenance
Objective
Get STEPs 4/6/7/8 onto the protected base, reviewed, with STEP 8's previously-unreviewed content actually reviewed.
Impacted components
14 commits, 11 files, every commit DCO-signed. Trial-merged onto
launchpad's current tip withgit merge-tree --write-tree— conflict-free. The branch is 72 commits behindlaunchpadbut touches onlylaunchpad/review-agent/, which nothing else has edited.Review outcome
STEPs 4/6/7 were reviewed on their own PRs by @ciaran-slow and @benmitchell11, including one inherited Blocker (non-list
stagessilently discarded) that was fixed and then independently re-confirmed by re-running the CLI probe rather than trusting the merge.STEP 8 (#450) merged with zero reviews. It was reviewed for the first time on 2026-08-24 —
review-code,review-tests, then adjudication over both. Three findings confirmed:1. High — the provenance claim was unfalsifiable by its own suite. Fixed in
682900cf1.PROVENANCE.mdclaimed byte reproducibility was "what makes 'real' … checkable rather than merely asserted". It is not: it compares the generator against itself. A generator rewritten to hand-type finding content, reading nothing underrecordings/, passed all 18 tests. AddsRealDocumentsReplayTheirNamedRecordingsTests(every replayed report'sfindingsandoutcomemust equal the recording named in_fixture.source_recordings; every real document'snoncemust becontain.make_nonce(seed=…)over a seed that recording actually records) andProvenanceNoteRecordsTheHonestySplitTests.Proven to fail, in scratch copies:
PROVENANCE.mdtruncated to# PROVENANCEisfiletest passes; new pin fails 8 timesThe claim is true today — all 8 replayed reports match their named recording, 0 mismatches — which is exactly why the guard was cheap now and expensive after STEP 9 records judge outputs against these fixtures.
2. Medium — a dedupe overclaim. Fixed in
682900cf1.The test comment and
line-anchored-findings.json's own note said the document "exercisesadjudicate()'s dedupe path". The defaultstub_dedupe_judgegroups nothing by design, so the real CLI emitsduplicate_groups: []with everyduplicate_ofnull, on threeBlockerfindings at the identicalcrates/buzz-relay/src/gate.rs:42. Reworded to dedupe CANDIDATE, with the grouped-output assertion attributed to STEP 10.Then
review-finalover all 14 commits returned NOT READY on two Highs:3. High —
stagesasserted a definition #119 superseded. Contract fixed in8da3d0307; behaviour filed as #565.ADJUDICATION.mdcarried the wording #119's plan superseded at8d47f8764on 2026-08-14 and named as a defect — seven days before STEP 4 was built atdc4c4bf75, with #118's ownOPENreserving it as "the item to re-check before STEP 4 is built rather than after." No artefact records that re-check.The reviewer proposed deriving per-dimension entries in
adjudicate(). That was checked and rejected: it would name only the dimensions that did report, so #119's condition (7) still could never fire — a report cannot testify to its own absence. The expected set is known only torun_dimensions.list_dimensions(), so the behaviour is #117's. This PR corrects the contract, strikes the superseded sentence rather than deleting it, and states the live consequence plainly.4. High, NOT fixed here — no CI job runs any of the 257 tests.
Makefile:3claimsmake testis "the single entry point"; it runsrun_controls.py, which runs 13 control scripts and zero unittest tests. Verified:grep -rn "unittest\|discover" check_*.py suite.py Makefilereturns 2 incidental hits, neither an invocation; the only workflow runspython3 run_controls.pyat line 48. #270 excludedreview-agent/on the premise that #118 STEP 10 closes it — STEP 10 registers a control, which does not invoke unittest discovery. Evidence posted to #270 for its owner to correct. This PR does not close it, and it is why the PR is a draft rather than READY.Verification
Not verified
review-final— was Claude. The standing rule is that final review is cross-model, because three same-model passes on a sibling issue missed what one Codex pass caught immediately. That pass has not happened, and no verdict here should be read as if it had.CHANGES_REQUESTEDreviews on feat(launchpad): nonce check and stages manifest (#118 STEP 4) #264/feat(launchpad): escalate-only enforcement and total-refutation status (#118 STEP 6) #266/feat(launchpad): dedupe via an injectable second judge (#118 STEP 7) #267 cannot be resolved by reading this PR — a merge into a feature branch leaves no approval to read. Whether they are satisfied is @ciaran-slow's call, not mine.review-final, not corrected here; it is plan text, not contract or code.🤖 Drafted by Claude Code (
claude-opus-5) for @serina-mcfall.