feat(launchpad): nonce check and stages manifest (#118 STEP 4) - #264
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>
Review pipeline — PR #264Stages run: Not applicable, declared: Ran the suite: I drove every one of STEP 4's done-when clauses through the real CLI over stdin rather than through the test helpers, so the refusals are proven observable end-to-end. All pass, with three genuinely distinct reason strings: Every refusal printed no document. That is the step's core requirement and it holds. Findings1. High — a
|
Severity promoted: High → Blocker (finding 1)@ciaran-slow has promoted the non-list I proposed High and set out the argument both ways; the call was never mine to make. What the promotion rests onThe plan's requirement is unconditional (
Probe B1 is an input carrying an
A step that exists because it must not trust its producer cannot be excused by trusting its producer. Promotion is the consistent reading. Re-verified against the whole stack, just nowBoth guard sites are unchanged downstream — only the line numbers move:
And the bypass still reproduces through the CLI on the chain tip ( The third line is the one that costs something in production: a What this changes about merging#264, #266 and #267 all carry it. A Blocker on #264 is therefore a Blocker on everything stacked above it, because the code is identical and the defect reproduces on the tip. The fix belongs on this branch and propagates up the stack; it should not be patched separately in #266 or #267. Revised stack readiness:
The fixOne line at each of the two sites: raise instead of returning or defaulting when That also resolves finding 2 (a Suggested control, in the style of the existing nonce tests which assert on stderr text rather than exit code alone: feed This block supersedes the one in my earlier review comment on this PR. Two blocks now exist across the two comments; if a gate scrapes all comments rather than one report, this is the authoritative one — it is the later of the two and the only one carrying the Blocker. Ranking is otherwise unchanged. ADJUDICATION COMPLETE REVIEW COMPLETE |
ciaran-slow
left a comment
There was a problem hiding this comment.
Comment review recording the pipeline result. Not an approval and not a change-request — the merge decision is unchanged by this.
Reviewed via the full pipeline — detail in my two comments on this PR. Ran 40 tests ... OK.
I drove every clause of STEP 4's done-when through the real CLI over stdin rather than through the test helpers, and the core of the step is right: three nonce refusals with three genuinely distinct reason strings, the "both 2 and 3 → report as mixed document" precedence implemented as specified rather than left to code order, no document printed on any refusal, no invented nonce, no sloppy prefix matching, and a pre-flight entry preserved in order. stage_complete naming each condition as its own boolean rather than an and chain that could silently drop STEP 6's flag is the opposite of the defect I was hunting.
One Blocker — must be fixed before this merges.
:258 and :474 — a non-list stages is silently discarded. Both sites treat a malformed container as an absent one: the re-run guard returns, and the manifest builder substitutes []. Probed through the CLI:
stages = [{"name":"adjudication",…}] → exit 1, correctly refused
stages = {"0":{"name":"adjudication",…}} → exit 0, ADJUDICATED ← guard bypassed
stages = {"p":{"name":"preflight","status":"blocked",…}}
→ exit 0, stages=['adjudication'], complete
The third case is what costs something: a blocked pre-flight — #116's fork-PR-secrets-withheld case — is dropped and the document publishes as a clean, complete review, because #119 only banners a non-complete status. The issue's fifth-criterion failure, reached through a shape defect no verdict-side check looks at.
The plan's rule is unconditional: "a second one on input is a re-run against an already-adjudicated document and exits non-zero." And this step's own premise is that it must not trust its producer, so "#117 never emits that shape" is not available as a defence — that is the reasoning the step exists to reject.
Fix: raise instead of returning/defaulting when stages is present but not a list — a StagesShapeError alongside the existing error types, which main already turns into exit 1 with no document. Absent stays legal. That also resolves the Low at :261 (a stages entry with a non-string name) in the same change.
#266 and #267 inherit this — both guard sites are unchanged, only shifted (:327/:674 and :388/:873), and the bypass reproduces on the chain tip. Please fix on this branch and let it propagate rather than patching downstream.
ciaran-slow
left a comment
There was a problem hiding this comment.
Change-request review. The finding(s) below are the author's to resolve before this merges — full detail, probes and line citations are in my pipeline comment on this PR.
Reviewed via the full pipeline — detail in my two comments on this PR. Ran 40 tests ... OK.
I drove every clause of STEP 4's done-when through the real CLI over stdin rather than through the test helpers, and the core of the step is right: three nonce refusals with three genuinely distinct reason strings, the "both 2 and 3 → report as mixed document" precedence implemented as specified rather than left to code order, no document printed on any refusal, no invented nonce, no sloppy prefix matching, and a pre-flight entry preserved in order. stage_complete naming each condition as its own boolean rather than an and chain that could silently drop STEP 6's flag is the opposite of the defect I was hunting.
One Blocker — must be fixed before this merges.
:258 and :474 — a non-list stages is silently discarded. Both sites treat a malformed container as an absent one: the re-run guard returns, and the manifest builder substitutes []. Probed through the CLI:
stages = [{"name":"adjudication",…}] → exit 1, correctly refused
stages = {"0":{"name":"adjudication",…}} → exit 0, ADJUDICATED ← guard bypassed
stages = {"p":{"name":"preflight","status":"blocked",…}}
→ exit 0, stages=['adjudication'], complete
The third case is what costs something: a blocked pre-flight — #116's fork-PR-secrets-withheld case — is dropped and the document publishes as a clean, complete review, because #119 only banners a non-complete status. The issue's fifth-criterion failure, reached through a shape defect no verdict-side check looks at.
The plan's rule is unconditional: "a second one on input is a re-run against an already-adjudicated document and exits non-zero." And this step's own premise is that it must not trust its producer, so "#117 never emits that shape" is not available as a defence — that is the reasoning the step exists to reject.
Fix: raise instead of returning/defaulting when stages is present but not a list — a StagesShapeError alongside the existing error types, which main already turns into exit 1 with no document. Absent stays legal. That also resolves the Low at :261 (a stages entry with a non-string name) in the same change.
#266 and #267 inherit this — both guard sites are unchanged, only shifted (:327/:674 and :388/:873), and the bypass reproduces on the chain tip. Please fix on this branch and let it propagate rather than patching downstream.
Independent adjudication of the change-request findingsAdjudicated in a fresh context, separate from the pipeline that produced the findings — which states its own limitation plainly: one context ran the reviewers, the adjudicator and the final pass, so its findings were self-adjudicated. This is the PR that matters most in the stack. The Head Findings1. Both lines reproduce verbatim: the re-run guard No upstream guard. Both candidate validators were checked specifically: All three probes reproduce through the real CLI over stdin, plus two more: Case C is the data-loss one and C2 is its control. Nothing downstream objects: Not documented as a limitation — the reverse. The docstring at Why the severity moved. The review promoted its own finding High -> Blocker in a second comment, which is a reviewer grading its own work; the rating has been re-derived from the failure scenario instead. Both "breaks a stated rule" arguments were checked and neither holds:
Also weighing toward High: the harm scenario needs two unbuilt components. Nothing in the repository produces a top-level What keeps it at High rather than Medium: reachable today through the real CLI's untrusted stdin, at two independent sites, with a docstring asserting a guarantee the code does not provide. A maintainer will not guess wrong about it — they will read a comment that is false. High blocks the merge under this project's rules, so nothing operational changes. The fix must still land on this branch. Only the label's honesty changes. The proposed fix is right and unaltered: raise a 2.
Low because a non-string Spot-check of the reviewer's positive claims — five checked, all five holdBecause the findings were self-adjudicated, the parts declared sound were probed too:
No defect found in the region declared sound. Cross-PR dedupe — one root cause, and the fix belongs hereDiffed the three refs directly rather than trusting the table. The two guard sites are byte-identical on all three branches; only line numbers move as the file grows:
The base Fix on this branch. The stack is a true chain (#266's base is One note for the reviewer harnessTwo verdict blocks were emitted across two comments, the second superseding the first (High -> Blocker). The second comment names the hazard itself. A gate scraping comments sees two authoritative-looking blocks with nothing to tell them apart — worth fixing in the harness rather than in the code. Verdict
Handed 2 (3 in the review block). 2 confirmed, 0 refuted, 1 resolved (the 🤖 Adjudicated by Claude Code ( |
…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>
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>
benmitchell11
left a comment
There was a problem hiding this comment.
Independent pass (not the pipeline that left the earlier reviews on this PR), with extra attention to the nonce check per this PR's own subject.
Checked out feat/review-agent-adjudication-nonce and ran the suite: Ran 207 tests ... OK in a clean worktree.
The Blocker from the standing review (non-list stages silently discarded) is fixed on this branch's tip (57941045c), and I reproduced the exact bypass scenarios from that review against the real CLI myself rather than trusting the fix commit's message:
stages = {"0": {"name":"adjudication",...}} -> exit 1, refused (was: exit 0, guard bypassed)
stages = {"p": {"name":"preflight","status":"blocked",...}} -> exit 1, refused (was: exit 0, blocked pre-flight dropped, published as complete)
stages = [{"name":"adjudication",...}] (still-legal list case) -> exit 1, correctly refused as a re-run
_input_stages (run_adjudication.py:211-251) now raises StagesShapeError for a non-list container, a non-dict entry, or an entry with a non-string name, and both call sites (_check_not_already_adjudicated and the manifest builder) route through this one function instead of each having its own inline isinstance check — closing the "two independent readers, two independent ways to get it wrong" shape the original bug had. Regression tests exist for both scenarios (test_run_adjudication.py:811-880).
Nonce check itself (_verify_nonce, :270-327) — read through it looking specifically for a bypassable condition, per the task brief:
- Empty/falsy top-level
nonce(including"") is refused before any comparison runs. - Empty
reportsor any report with an unparseablecompletion_markertriggers "absent provenance" — can't skip verification by supplying zero reports. - A non-string
noncethat happens to be truthy (e.g. an int) can't accidentally match: the compared value is always the string parsed out of a report'scompletion_marker, so a type mismatch alone forces "mismatched envelope." - Precedence (mixed document over mismatched envelope) is implemented as an explicit branch order matching the plan's stated rule, not left to happen to fall out of it.
_check_not_already_adjudicatedruns before nonce verification (a re-run is refused as a structural defect independent of whether the nonce happens to check out), and nonce verification runs beforefindings.validate(provenance gated before content) — both orderings are the ones the module docstring claims, confirmed by readingadjudicate()itself, not just the docstring.
I didn't find a bypass. Nothing else new to add beyond the standing review's own findings, which now read as resolved on this branch.
Not verified: no live-model dependency in this step.
The requested change has landed — flagging for re-review@ciaran-slow's The fix: Independently confirmed, not assumed: @benmitchell11 re-ran the CLI probe on the chain tip rather than trusting the merge carried cleanly, and reported an object-shaped Also resolved since that review: the cross-step One further finding, fixed just now: @benmitchell11's pass also caught an out-of-ladder gap that both earlier review passes on this stack missed — Reproduced before fixing. This is a plan-conformance gap rather than only defence-in-depth: STEP 6's own done-when names this exact case, and Not claiming this clears the review — the merge decision is @ciaran-slow's. This is the evidence trail so re-review doesn't require re-deriving what changed. A note on reading CI on this stack: each commit here carries two 🤖 Posted by Claude Code for @serina-mcfall. Claude-only pass on the severity fix — no independent cross-model check on that specific commit. |
Requested changes are done — but I am not the right party to certify itDisclosure first: I wrote the commit that fixes this ( @ciaran-slow's change-request (2026-08-21T03:57Z) is stale — the head moved after it. What landed:
Re-verified through the real CLI at this head: an One deliberate departure from the review: the severity was adjudicated down from Blocker to High. Both "breaks a stated rule" arguments turned out not to hold — the plan's "cannot inherit a guarantee it did not watch being made" sits in the nonce paragraph, and the What this needs: a re-review from @ciaran-slow rather than a clearance from me — I should not dismiss findings against my own fix. Re-requested. This branch is the root cause for #266 and #267; both carry the fix by propagation rather than a second copy. |
0967862
into
feat/review-agent-adjudication-run
Summary
Implements STEP 4 of
launchpad/plans/2026-08-13-issue-118-adjudication.md: nonce verification and thestagesmanifest. The top-levelnonceis checked against every report's completion marker with three distinct ordered refusals, and the output carries every inputstagesentry plus exactly one newadjudicationentry. Stacked on the not-yet-merged STEP 3 branch (#263).Related issue
Refs #118
Issue type
Task
Agent provenance
Objective
Extend
launchpad/review-agent/run_adjudication.pywith nonce provenance checking and thestagesmanifest #119 reads.Impacted components
launchpad/review-agent/run_adjudication.pylaunchpad/review-agent/test_run_adjudication.pyApproach and rejected alternatives
Three refusals in a fixed order, because one document can satisfy more than one at once:
absent provenance, thenmixed document, thenmismatched envelope. Mixed wins over mismatched when both apply — a mixed document is the larger fact and a header mismatch is its consequence._verify_nonceruns beforefindings.validate, deliberately reordered from STEP 3's sequence. Rejected the original ordering after proving it made the three refusals unreachable through the real CLI:findings.validateindependently rejects a marker/header disagreement with one generic per-report message, identical whether the reports disagree with each other or merely with the top-level key, so the operator would only ever see the generic text. Verified by reverting the ordering on a scratch copy and confirming both categories produce byte-identical messages.One exception to that ordering, found by review and fixed: a document whose
reportskey is missing, non-list or empty defers straight tofindings.validate, because calling that shape "absent provenance" buries a more specific and more useful message.For the malformed
stagesfix, rejected correcting the twoisinstance(..., list)tests inline at each site. Both readers previously treated a malformed container as absent, which is how one shape defect became two independent failures — so the rule lives in one_input_stagesfunction that both call. A second copy of a rule is a second chance to disagree with it.Verification
Command run:
Raw output:
Every malformed-
stagesshape driven through the real CLI over stdin, with both controls:Confirmation that no upstream validator covers this key, which is why the guard has to be here:
Not verified
#119 does not exist, so "the manifest #119 reads" is asserted against the plan's stated rule rather than against a consumer.
PublishIncompleteRuleTestsapplies #119's own rule ("any status other thancompletebanners the whole review") as a local assertion, which is the best available and is not the same as integration.#116's pre-flight does not emit a
stagesentry yet, so theblockedpre-flight case — the one that costs the most when it is dropped — is exercised with hand-written input, not with a real producer's output.stage_complete's incomplete branch is unreachable today._run_judge_safelyalways returns a legal verdict, soevery_finding_has_verdictis always True by the time it is read. The branch is tested by direct construction, not throughmain().No live model. Same as STEP 3: every verdict comes from the stub or an injected judge.
Security implications
This step is where a document's provenance is checked, so the failure mode is accepting an unprovenanced or spliced document as genuine. Three properties hold: the nonce is passed through and never generated, no prefix or lookalike match is accepted (a marker nonce equal to the top-level minus its last character is refused, and an
adjudication_noncekey is not adopted), and no document is printed on any refusal.The
stagesfix closes a real data-loss path. Astagesvalue that was present but not a list was read as absent at both sites, which bypassed the re-run guard and silently discarded every entry already recorded. Ablockedpre-flight — #116's fork-PR-secrets-withheld case — disappeared and the document published as a clean, complete review, because #119 only banners a non-completestatus. Neitherfindings.validatenorverdicts.validateinspectsstages, and thestagesmanifest is explicitly an output #117 does not produce, so there was no upstream guarantee to inherit. Absent stays legal; present-but-malformed is now refused with exit 1 and no document.Escalations
stagesdefect was contested and settled at High, not Blocker. The two "breaks a stated rule" arguments did not survive checking: the plan's "cannot inherit a guarantee it did not watch being made" sits in the nonce paragraph, and thestagesparagraph callsstagesthe output task: the parallel review dimensions that produce findings #117 does not produce. High still blocks the merge, so nothing operational turns on it — recorded so the rating is not taken as inherited.review-finalhas not been run for this branch. The plan's GATES section schedules it at STEP 12.🤖 Drafted by Claude Code (
claude-opus-5) for @serina-mcfall.