feat(launchpad): escalate-only enforcement and total-refutation status (#118 STEP 6) - #266
Conversation
#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>
Review pipeline — PR #266Stages run: Not applicable, declared: Ran the suite: Ran 52 tests, OK. Then drove every clause of STEP 6's done-when through All six pass. The zero-findings case is the one the plan singles out — conflating it with total refutation "would banner every genuinely clean PR and teach readers to skip the banner" — and it is handled correctly. Findings1. Medium — the out-of-ladder guard does not fire on the case the plan describes in exactly these words
if proposed_severity is None or proposed_severity == reported_severity:
# No re-rating: unchanged from STEP 3/4's behaviour.
return verdict, reported_severity, NoneThis equality short-circuit runs before the ladder check at Probed directly: _apply_severity_rerating("x", "Info", "CONFIRMED", "Info", "judge says", [])
→ ('CONFIRMED', 'Info', None) # out-of-ladder value published, no reason
_apply_severity_rerating("y", "High", "CONFIRMED", "Info", "judge says", [])
→ ('UNPROVEN', 'High', "judge returned an out-of-ladder severity 'Info'…") # correctThe second call is STEP 6's done-when clause and it works. The first is the case the plan wrote out in advance, at
That is a description of The fallback_severity = (
reported_severity if reported_severity in review.SEVERITY_ORDER else "Blocker"
)The plan's reason for that branch is "there is then no safe value to copy and this stage may not decide that an unrateable finding is a small one", and the docstring calls it "purely as a second layer should that guarantee ever be bypassed." But reaching it requires Concrete failure: a finding with Not reachable through Why it shipped, and this is worth stating: STEP 6's own done-when list covers "a judge injected to return severity 'Info' over a finding whose Fix: move the ladder test ahead of the equality short-circuit — check the effective severity (the proposed value if there is one, else 2. Low — missing
|
Carried-forward severity updated: the
|
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 52 tests ... OK.
I drove all six of STEP 6's done-when clauses through adjudicate() with injected judges and every one passes: refute-all sets total_refutation and the total_refutation stage status with counts unchanged; the zero-findings case correctly does not set it, which is the case the plan singles out because conflating them "would banner every genuinely clean PR"; an out-of-ladder re-rating becomes UNPROVEN at the reported severity with a reason and is correctly absent from downgrades; the deliberately-bare SEVERITY_ORDER[f["severity"]] subscript succeeds on every output; a genuine fall is recorded with from/to/reason; and an upgrade is not. I also confirmed containment findings neither acquire a verdict nor poison the refutation tally, and that the whole containment block plus pr/merge_base_sha/head_sha stay byte-identical.
One inherited Blocker, plus one Medium of this PR's own.
-
Inherited Blocker from #264 — non-list
stagessilently discarded, at:327and:674here. Unchanged from #264 and the bypass reproduces on the chain tip. Fix on #264's branch, not here — two divergent copies of the same guard is worse than the bug. -
Medium,
:487— the out-of-ladder guard does not fire on the case the plan describes verbatim. The equality short-circuitif proposed_severity is None or proposed_severity == reported_severity:runs before the ladder check, so when the judge agrees with an out-of-ladder severity,reported_severityis returned untested with no reason and the verdict unchanged. Probed:_apply_severity_rerating("x","Info","CONFIRMED","Info",…)→('CONFIRMED','Info',None), while the in-ladder sibling correctly returns UNPROVEN. The plan wrote this out in advance: "A guard watching only re-ratings never sees a finding that ARRIVED at 'Info' and was agreed with, and copies it intoseverityuntouched." TheBlockerfallback at:495exists but is unreachable in the one scenario it was written for. Fix: test the effective severity against the ladder before deciding whether a re-rating occurred. Not reachable throughmain()today, which is why it is Medium — but #118 STEP 10's control list already names this exact path, so it will surface there regardless.
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 52 tests ... OK.
I drove all six of STEP 6's done-when clauses through adjudicate() with injected judges and every one passes: refute-all sets total_refutation and the total_refutation stage status with counts unchanged; the zero-findings case correctly does not set it, which is the case the plan singles out because conflating them "would banner every genuinely clean PR"; an out-of-ladder re-rating becomes UNPROVEN at the reported severity with a reason and is correctly absent from downgrades; the deliberately-bare SEVERITY_ORDER[f["severity"]] subscript succeeds on every output; a genuine fall is recorded with from/to/reason; and an upgrade is not. I also confirmed containment findings neither acquire a verdict nor poison the refutation tally, and that the whole containment block plus pr/merge_base_sha/head_sha stay byte-identical.
One inherited Blocker, plus one Medium of this PR's own.
-
Inherited Blocker from #264 — non-list
stagessilently discarded, at:327and:674here. Unchanged from #264 and the bypass reproduces on the chain tip. Fix on #264's branch, not here — two divergent copies of the same guard is worse than the bug. -
Medium,
:487— the out-of-ladder guard does not fire on the case the plan describes verbatim. The equality short-circuitif proposed_severity is None or proposed_severity == reported_severity:runs before the ladder check, so when the judge agrees with an out-of-ladder severity,reported_severityis returned untested with no reason and the verdict unchanged. Probed:_apply_severity_rerating("x","Info","CONFIRMED","Info",…)→('CONFIRMED','Info',None), while the in-ladder sibling correctly returns UNPROVEN. The plan wrote this out in advance: "A guard watching only re-ratings never sees a finding that ARRIVED at 'Info' and was agreed with, and copies it intoseverityuntouched." TheBlockerfallback at:495exists but is unreachable in the one scenario it was written for. Fix: test the effective severity against the ladder before deciding whether a re-rating occurred. Not reachable throughmain()today, which is why it is Medium — but #118 STEP 10's control list already names this exact path, so it will surface there regardless.
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. Head at review time Bottom line: nothing this PR introduces blocks it. One finding is inherited and belongs on #264; the other is refuted. Findings1. Both lines reproduce. No upstream guard — And the evidence-loss half, through the real process: A Severity: High (adjudicated at the root cause on #264, where the promoted Blocker was moved down — the two "breaks a stated rule" arguments did not survive checking; details on that PR). High blocks the merge, so the practical consequence is unchanged: the fix must land before this chain merges. Byte-identical to #264, verified programmatically rather than by eye. The full Do not patch it here. Two divergent copies of one guard is worse than the bug. #264's tip is still unfixed at both sites; the chain propagates the fix upward on rebase. 2. The mechanical claim is true; the finding is not. The line is where the review says, and the probe reproduces exactly: So the code does what is claimed. Three things refute it as a defect: (a) An upstream guard prevents the failure scenario — and it is the guard the plan itself nominates.
(b) The limitation is documented, in three places. The module docstring at (c) The decisive point, which the review did not weigh: the plan contradicts itself, and the code follows the operative clause. The review quotes STEP 6's prose (
That last clause is an argument against the prose's demand: emitting UNPROVEN at a On STEP 10 reachability, which the review offers as the reason it will surface anyway: STEP 10's list does name "as a Residual, genuine but non-blocking (Low, and not a code change): STEP 6's prose and STEP 6's done-when disagree about whose job the agreed-with case is, and the Spot-check of the reviewer's positive STEP 6 claims — four checked, all hold, plus one case it did not testZero findings vs refute-all — the "would banner every clean PR" case, and the highest-value one: Correct in all three directions — no always-fires, no never-fires. The mixed case is the other way the flag could misfire and was not in the review; it behaves correctly. Containment pass-through and refutation tally: No The bare No defect found in the part declared sound. Verdict
Handed 2. 1 confirmed (inherited — belongs on #264), 1 refuted, 0 merged. One severity moved down to REFUTED. The reviewer's third item, the missing This PR is blocked by inheritance, not by anything it introduced. 🤖 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>
benmitchell11
left a comment
There was a problem hiding this comment.
Independent pass (not the pipeline that left the earlier reviews on this PR).
Checked out feat/review-agent-adjudication-escalate and ran the suite: Ran 219 tests ... OK in a clean worktree.
The inherited Blocker from #264 (non-list stages silently discarded) is fixed here, via the merge commit ec4dd4356 ("Merge STEP 4's stages-shape fix into STEP 6"). Confirmed StagesShapeError and the fixed _input_stages are present on this branch's tip and the test file carries the same regression tests. Not re-probing the CLI again here since it's the identical code from #264, already verified there.
The Medium finding (out-of-ladder guard doesn't fire when the judge agrees with an out-of-ladder severity) is still present. Reproduced independently:
>>> _apply_severity_rerating("x", "Info", "CONFIRMED", "Info", None, [])
('CONFIRMED', 'Info', None)review.SEVERITY_ORDER only has Blocker/High/Medium/Low — "Info" is out-of-ladder on both sides here. _apply_severity_rerating (run_adjudication.py:551-614) checks proposed_severity == reported_severity (:574) before checking either value against review.SEVERITY_ORDER, so an out-of-ladder value the judge merely agrees with sails through untouched, with no severity_reason and no flag — the ladder check at :578 only ever fires for a proposed value that differs from what was reported. I agree with the standing review's Medium severity call: findings.validate already refuses an illegal reported_severity before any judge runs (confirmed by reading the InputValidationError boundary in adjudicate()), so this isn't reachable through main() today — it's a defense-in-depth gap in a guard whose whole job is "don't trust what arrives," which is a real inconsistency with the rest of this module's own stated discipline, and it has no regression test (grepped test_run_adjudication.py for anything exercising reported_severity == proposed_severity on an out-of-ladder value — nothing there). Given #118 STEP 10 already reportedly has this on its control list per the standing review, I'm not requesting changes, but flagging that my own reproduction confirms it's still open on this branch's current tip, not just theoretically.
Spot-checked (not re-deriving from scratch, since the standing review already drove all six done-when clauses through adjudicate()): total_refutation zero-findings vs. refute-all distinction, and that containment/pr/merge_base_sha/head_sha pass through untouched — both consistent with what's claimed by reading the code directly.
Not verified: no live-model dependency in this step.
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>
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 one of the commits on this branch ( @ciaran-slow's change-request (2026-08-21T03:57Z) is stale — the head has moved twice since, to
Worth saying plainly about that second one: an independent adjudication pass had refuted it. The reasoning was that Fixing it anyway is defensible and arguably better — the guard now holds without depending on an upstream guarantee, which is the posture the step's own premise argues for. But it means the plan contradiction in #286 is now the only open thread from that finding, and #286 should be reconciled or closed rather than left dangling. What this needs: a re-review from @ciaran-slow rather than a clearance from me. Re-requested. CI is green. |
Summary
Implements STEP 6 of
launchpad/plans/2026-08-13-issue-118-adjudication.md: the escalate-only guard around judge-supplied severity re-rating, plus total-refutation status on thestagesmanifest. Stacked on the not-yet-merged STEP 4 branch (#264).Related issue
Refs #118
Issue type
Task
Agent provenance
Objective
Extend
launchpad/review-agent/run_adjudication.pyso a judge may re-rate severity, behind a guard that refuses an illegal rating and records a genuine fall.Impacted components
launchpad/review-agent/run_adjudication.pylaunchpad/review-agent/test_run_adjudication.pyApproach and rejected alternatives
Three branches: no re-rating (absent, or equal to
reported_severity) leaves the finding unchanged; a legal differing re-rating becomesseverity, carriesseverity_reason, and is recorded inadjudication.downgradesonly if it is a genuine fall; an out-of-ladder rating is refused, forcingUNPROVENat the guaranteed-legalreported_severityand never recording a downgrade.total_refutationdrives the manifest's ownstatus, checked ahead of and overriding thecompletebranch. A zero-findings document still getscomplete— conflating "nothing to refute" with "everything refuted" would banner every genuinely clean PR, which is why that case is singled out.stage_completenames each condition as its own boolean rather than anandchain, so STEP 6's flag cannot be silently dropped when a later step adds a third condition.Rejected: relying on
verdicts.validatedownstream to catch a lossy document. A stage that can print a lossy document and lean on a later check has already lost the document once, so afinding_idset-equality reassertion runs insideadjudicate()before it returns. It uses a local walk rather than callingverdicts._finding_ids, because a bug shared between the producer and its own belt-and-braces check would prove nothing.Verification
Command run:
Raw output:
The inherited
stagesdefect, re-driven through the real CLI on this branch after merging #264's fix — closed by propagation, not by a second copy of the guard:The total-refutation flag in all three directions, including the mixed case:
Not verified
No live model produced any re-rating. Every severity in every test is hand-written or from an injected judge, so nothing here shows whether a real adjudicator re-rates sanely — only that the guard around it behaves.
The
Blockerfallback at the out-of-ladder branch is unreachable throughmain()today.findings.validaterefuses an out-of-ladder inputseveritybefore the judge loop, so the only way to reach that fallback is a direct unit call. It is deliberate defence in depth behind a guarantee the docstring names, and it is tested by direct construction rather than end to end.A reported Medium at the out-of-ladder equality short-circuit was refuted, not fixed. The guard does not fire when a judge agrees with an out-of-ladder severity — but that input cannot be constructed through the only caller, the behaviour is documented in three places, and STEP 6's own done-when assigns the case to STEP 3 with a stated reason. What survives is a contradiction between STEP 6's prose and STEP 6's done-when, filed as #286. If the prose turns out to be the intent, this decision needs revisiting.
downgradeshas no consumer. #119 is not built, so nothing reads what this step records.Security implications
This is the step where a model's opinion is allowed to change a severity, so it is the one most able to weaken a review. The guard is built accordingly: a re-rating can only land if it is inside the four-value ladder, an illegal one forces
UNPROVENrather than being coerced to something plausible, and an upgrade is never recorded as a downgrade. Severity is imported fromreview.pyand never redeclared, so there is no second copy of the ladder to drift.A real High was found and fixed on this branch before this PR: a judge-returned
severityhad no type check before anintest againstSEVERITY_ORDER, so an unhashable value — a list or dict, reachable today throughmake_replay_judgeon a malformed recording — crashed the whole run withTypeErrorinstead of failing that one finding closed. Reproduced before fixing.Containment findings are confirmed to acquire no verdict and to leave the
containmentblock byte-identical, so this step cannot quietly adjudicate something the contract says is passed through.Escalations
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.