Skip to content

feat(launchpad): escalate-only enforcement and total-refutation status (#118 STEP 6) - #266

Open
serina-mcfall wants to merge 4 commits into
feat/review-agent-adjudication-noncefrom
feat/review-agent-adjudication-escalate
Open

feat(launchpad): escalate-only enforcement and total-refutation status (#118 STEP 6)#266
serina-mcfall wants to merge 4 commits into
feat/review-agent-adjudication-noncefrom
feat/review-agent-adjudication-escalate

Conversation

@serina-mcfall

@serina-mcfall serina-mcfall commented Aug 21, 2026

Copy link
Copy Markdown

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 the stages manifest. Stacked on the not-yet-merged STEP 4 branch (#264).

Related issue

Refs #118

Issue type

Task


Agent provenance

Field Value
Harness / provider Claude Code
Model claude-opus-5
Session reference N/A - the harness exposes no shareable run id
Initiating human @serina-mcfall

Objective

Extend launchpad/review-agent/run_adjudication.py so 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.py
  • launchpad/review-agent/test_run_adjudication.py

Approach and rejected alternatives

Three branches: no re-rating (absent, or equal to reported_severity) leaves the finding unchanged; a legal differing re-rating becomes severity, carries severity_reason, and is recorded in adjudication.downgrades only if it is a genuine fall; an out-of-ladder rating is refused, forcing UNPROVEN at the guaranteed-legal reported_severity and never recording a downgrade.

total_refutation drives the manifest's own status, checked ahead of and overriding the complete branch. A zero-findings document still gets complete — conflating "nothing to refute" with "everything refuted" would banner every genuinely clean PR, which is why that case is singled out.

stage_complete names each condition as its own boolean rather than an and chain, so STEP 6's flag cannot be silently dropped when a later step adds a third condition.

Rejected: relying on verdicts.validate downstream 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 a finding_id set-equality reassertion runs inside adjudicate() before it returns. It uses a local walk rather than calling verdicts._finding_ids, because a bug shared between the producer and its own belt-and-braces check would prove nothing.

Verification

Command run:

cd launchpad/review-agent
python3 -m unittest discover -s . -p 'test_*.py'

Raw output:

...........................................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 219 tests in 0.860s

OK

The inherited stages defect, 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:

=== #266: the inherited bypass, after propagation ===
  OBJECT adjudication (was exit 0)         exit=1 stages=EMPTY
  OBJECT preflight blocked (was dropped)   exit=1 stages=EMPTY
  CONTROL list preflight blocked           exit=0 stages=['preflight', 'adjudication']

The total-refutation flag in all three directions, including the mixed case:

C refute-all    total_refutation: True   status: total_refutation   counts before/after: [2] [2]
D zero-findings total_refutation: False  status: complete
E MIXED (1 REFUTED, 1 CONFIRMED) total_refutation: False status: ['complete']
  • Tests or checks were run and the raw output is pasted above
  • The diff is confined to the scope of the linked issue
  • No secrets, keys, tokens or hostnames were added to tracked files

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 Blocker fallback at the out-of-ladder branch is unreachable through main() today. findings.validate refuses an out-of-ladder input severity before 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.

downgrades has 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 UNPROVEN rather than being coerced to something plausible, and an upgrade is never recorded as a downgrade. Severity is imported from review.py and 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 severity had no type check before an in test against SEVERITY_ORDER, so an unhashable value — a list or dict, reachable today through make_replay_judge on a malformed recording — crashed the whole run with TypeError instead of failing that one finding closed. Reproduced before fixing.

Containment findings are confirmed to acquire no verdict and to leave the containment block byte-identical, so this step cannot quietly adjudicate something the contract says is passed through.

Escalations

  1. This PR introduces nothing that blocks it. Its one confirmed finding was inherited from feat(launchpad): nonce check and stages manifest (#118 STEP 4) #264 and is fixed there; the merge on this branch carries that fix up. The second reported finding was refuted on adjudication.
  2. STEP 6's plan text contradicts itself about who owns the agreed-with out-of-ladder case — prose says this step, done-when says STEP 3, and the code follows the done-when. Filed as task: reconcile STEP 6's prose with its own done-when on the agreed-with out-of-ladder severity #286 rather than decided here, because amending a plan mid-chain is not this PR's call.
  3. review-final has 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.

#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>
@ciaran-slow

Copy link
Copy Markdown

Review pipeline — PR #266

Stages run: review-code, review-tests, review-a11y, review-adjudicate, review-final.
Plan read first: launchpad/plans/2026-08-13-issue-118-adjudication.md, STEP 6 at :599.
Diffed against this PR's own base (feat/review-agent-adjudication-nonce, #264).

Not applicable, declared: review-a11y — out of scope for #118 per the plan's LEFT OUT. check-ledger.sh — plan uses STEP N, not ### Task N:; no .superpowers/sdd/ ledger; vacuity guard would exit 1.

Ran the suite: Ran 52 tests, OK. Then drove every clause of STEP 6's done-when through adjudicate() with injected judges:

refute every finding    → total_refutation=True, stage status "total_refutation", counts unchanged, validates clean
zero findings           → total_refutation=False, stage status "complete"
judge returns "Info"    → finding becomes UNPROVEN at reported "High", reason names the refusal, not in downgrades
bare SEVERITY_ORDER[f]  → succeeds on every finding of every output above
downgrade High→Low      → downgrades = [{finding_id, from:"High", to:"Low", reason}]
upgrade High→Blocker    → downgrades = [] (an upgrade is not a downgrade)

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.


Findings

1. Medium — the out-of-ladder guard does not fire on the case the plan describes in exactly these words

launchpad/review-agent/run_adjudication.py:487

if proposed_severity is None or proposed_severity == reported_severity:
    # No re-rating: unchanged from STEP 3/4's behaviour.
    return verdict, reported_severity, None

This equality short-circuit runs before the ladder check at :490. So when the judge agrees with an out-of-ladder severity — or omits severity entirely — reported_severity is returned verbatim, never tested against review.SEVERITY_ORDER, with no severity_reason and the verdict left as-is.

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'…")   # correct

The second call is STEP 6's done-when clause and it works. The first is the case the plan wrote out in advance, at :611-616:

"AN OUT-OF-LADDER EFFECTIVE SEVERITY IS REFUSED, not published. The guard fires on the value that will actually be emitted — the re-rating where there is one, reported_severity where the judge agreed and there is none — not only on a re-rating that differs. A guard watching only re-ratings never sees a finding that ARRIVED at "Info" and was agreed with, and copies it into severity untouched."

That is a description of :487. The plan named this defect, prescribed the fix, and the implementation is the version the plan rejected.

The Blocker fallback exists but is unreachable in the scenario it was written for. :495-497 does implement it:

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 reported_severity to be out-of-ladder — and whenever that is true, the judge either agrees (short-circuited at :487) or proposes something else. Only the "proposes something else" path gets there. The layer is half-present.

Concrete failure: a finding with severity: "Info" reaches _apply_severity_rerating and the judge agrees with it. The finding is emitted CONFIRMED at severity: "Info" with severity_reason: None. Downstream, verdicts.validate does catch the illegal severity — so the document is rejected rather than published — but the plan's stated point is that this is the wrong place for the catch: "this stage is where bad values are created, and a producer relying on its consumer's default has moved the failure rather than removed it." The failure mode becomes "the whole document is rejected with a validator message" instead of "that one finding becomes UNPROVEN with a reason naming the refusal", which is a materially worse operator experience and loses the other findings' verdicts with it.

Not reachable through main() today. STEP 3's findings.validate refuses an out-of-ladder input severity before any judge runs, which is why this is rated Medium rather than higher. It is reachable from any other caller of _apply_severity_relating that does not pre-validate — STEP 10's controls calling it directly, as my probe did, being the obvious one.

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 reported_severity is legally in-ladder" — the case that works — and explicitly hands the sibling case to STEP 3: "the SIBLING case — a fixture whose finding ARRIVES with an out-of-ladder severity value already — is STEP 3's job, not this control's." That is correct for input arriving illegal. It leaves the agreed-with case in neither list, so no control was ever written for it, even though the plan's prose requires the guard to cover it.

Fix: move the ladder test ahead of the equality short-circuit — check the effective severity (the proposed value if there is one, else reported_severity) against review.SEVERITY_ORDER first, and only then decide whether a re-rating occurred. Add the missing control: _apply_severity_rerating("x", "Info", "CONFIRMED", "Info", …) must return UNPROVEN at Blocker with a reason.

2. Low — missing by:agent label

No labels; the body carries an Agent provenance block. launchpad/AGENTS.md §5 rule 3.

gh pr edit 266 --repo launchpad-26/buzz --add-label by:agent


What I looked for and did not find

  • Containment findings poisoning total_refutation. The likeliest way to break this step. Containment findings carry no verdict, so if the tally walked them, all(v == "REFUTED") would be False on any PR with a containment catch and total refutation would become unreportable. Probed with a delimiter_forge containment finding alongside fully-refuted report findings: total_refutation: True, stage status total_refutation. Correctly excluded, matching verdicts.py's _iter_findings, which I checked separately on feat(launchpad): verdict contract in code (#118 STEP 2) #261.
  • Containment findings acquiring a verdict. Probed the same document: the containment finding's keys are exactly ['entry_point', 'evidence', 'kind', 'severity'] — no verdict field added, as the plan requires. And the whole containment block is byte-identical to the input under json.dumps(..., sort_keys=True), as are pr, merge_base_sha and head_sha. STEP 3's pass-through guarantee survives STEP 6's changes.
  • findings_count drifting after re-rating. Probed: every report's findings_count still equals len(findings) after a refute-all run, and the document validates clean. Nothing is removed, which is STEP 6's first requirement.
  • An upgrade being logged as a downgrade. Probed High→Blocker: downgrades stays empty and the severity is applied. The direction test at :519 uses SEVERITY_ORDER[proposed] > SEVERITY_ORDER[reported], which is correct given the ladder's index ordering.
  • A downgrade recorded by a later sweep. It is appended inside _apply_severity_rerating at the moment the re-rating is applied, per the plan's "a sweep is a second place the two could disagree." The downgrades list is mutated in place and the docstring says so.
  • A missing severity_reason on a legal re-rating. :513-516 generates one when the judge gives none, because verdicts.validate requires it in either direction. So a judge that re-rates without explaining itself cannot produce an invalid document.
  • The failed-closed path re-rating anyway. _run_judge_safely never includes a severity key in either of its two failure returns, so a judge whose output failed closed cannot also move the severity. The docstring states this as "failing closed means failing closed on both", and the code matches.
  • Tests that cannot fail. All 52. Literals throughout; the injected judges are one-line lambdas returning fixed dicts, which is the right shape — the assertion is on the runner's behaviour, not on the judge's.

Carried forward from earlier in the stack

Neither is new to this PR; both are re-reachable through the code it touches, and both are counted once elsewhere.

On CI coverage

run_controls.py's CONTROLS list still names no test_*.py, so these 52 tests run in no CI job. As on #261/#263/#264, that is a stated decision deferred to STEP 10 (plan:741), not filed as a finding here.

Merge readiness

A reader of #118 STEP 6 would find the step's headline behaviours correct and, in the places that matter most, correct for stated reasons rather than by accident. Nothing is removed. Downgrades are recorded at the moment of application with from/to/reason. Total refutation reaches the stages status so #119's banner fires, and the zero-findings case is excluded so it does not fire on every clean PR. The bare-subscript control the plan asked for — deliberately without #119's .get default — passes on every output I generated.

They would find one guard placed one branch too late, in the one spot the plan described in advance and in detail. It is not reachable through the CLI, and verdicts.validate catches the consequence, so nothing publishes wrong today — but the layer the plan called defence in depth is currently one layer, and the missing control is why nobody noticed.

Merge order: base is #264's branch. The stack is #261#263#264#266#267.

What I could not check: whether STEP 10's controls will call _apply_severity_rerating directly. If they do, they will hit finding 1's path, which is how I hit it.

Independence and tools

Independent of the code under review: I did not write it. Not independent across pipeline stages — one context ran the reviewers, the adjudicator and the final pass. All findings self-adjudicated; treat that as a limit.

Tools actually held and used: Bash (git, git show, git worktree, gh, python3 for the probes), Read, Edit, Write. No Grep or Glob tool was available in this session.

Nothing found at Blocker or High that is new to this PR.

CONFIRMED	Medium	launchpad/review-agent/run_adjudication.py:487	equality short-circuit runs before the ladder check, so an agreed-with out-of-ladder severity is published unguarded — the case plan:611-616 names verbatim
CONFIRMED	Low	PR #266 (labels)	missing required by:agent label

Handed 2 findings new to this PR, confirmed 2, refuted 0. Two findings carried forward from #261/#263/#265 are listed above and not re-counted here. Eight candidates were REFUTED by probe and are recorded rather than dropped silently. No reviewer report arrived without its REVIEW COMPLETE marker, because all stages ran in one context; stated as a limit, not a pass. I did not author any of the code under review.

ADJUDICATION COMPLETE

REVIEW COMPLETE


Per launchpad/AGENTS.md §5 rule 1 — an agent drafts and raises, never approves or clears. This is a report, not an approval; the merge decision is @ciaran-slow's.

@ciaran-slow ciaran-slow added the by:agent Filed or authored by an AI agent, not a human label Aug 21, 2026
@ciaran-slow ciaran-slow self-assigned this Aug 21, 2026
@ciaran-slow

Copy link
Copy Markdown

Carried-forward severity updated: the stages finding is now a Blocker

@ciaran-slow has promoted finding 1 on #264 from High to Blocker. My review above lists it in the Carried forward table at High — that row is now Blocker.

This PR inherits it. Both guard sites are unchanged from #264, only shifted:

  • launchpad/review-agent/run_adjudication.py:327if not isinstance(stages, list): return
  • launchpad/review-agent/run_adjudication.py:674… if isinstance(input_stages_raw, list) else []

Re-verified through the CLI on the chain tip: an adjudication entry inside an object container adjudicates with exit 0 instead of being refused, and a preflight entry with status: "blocked" is silently discarded so the document publishes as complete.

So this PR is blocking until #264's fix lands and propagates. Do not patch it here — the fix belongs on #264's branch, or the stack ends up with two divergent versions of the same guard.

Nothing else in my review of this PR changes. The reasoning and the re-verification are on #264: #264 (comment)

The verdict block in my review above stands as written — it covers only findings new to this PR, and this defect is counted once, on #264.

@ciaran-slow ciaran-slow left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. Inherited Blocker from #264 — non-list stages silently discarded, at :327 and :674 here. 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.

  2. Medium, :487 — the out-of-ladder guard does not fire on the case the plan describes verbatim. The equality short-circuit if 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_severity is 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 into severity untouched." The Blocker fallback at :495 exists 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 through main() 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 ciaran-slow left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. Inherited Blocker from #264 — non-list stages silently discarded, at :327 and :674 here. 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.

  2. Medium, :487 — the out-of-ladder guard does not fire on the case the plan describes verbatim. The equality short-circuit if 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_severity is 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 into severity untouched." The Blocker fallback at :495 exists 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 through main() 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.

@serina-mcfall

Copy link
Copy Markdown
Author

Independent adjudication of the change-request findings

Adjudicated 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 e79da04fb; no post-review push. Suite verified: Ran 52 tests ... OK here, Ran 190 tests ... OK directory-wide.

Bottom line: nothing this PR introduces blocks it. One finding is inherited and belongs on #264; the other is refuted.

Findings

1. run_adjudication.py:327 and :674 — inherited non-list stages — CONFIRMED. Fix on #264, not here.

Both lines reproduce. _check_not_already_adjudicated returns early at :327-328; :674 substitutes [], so nothing is carried forward and nothing is refused.

No upstream guard — findings.validate does not type-check stages, so the malformed shape passes it cleanly:

A: NOT REFUSED. output stages = [{"name": "adjudication", "status": "total_refutation", ...}]

And the evidence-loss half, through the real process:

$ ... d["stages"] = {"preflight": {"name":"preflight","status":"blocked","reason":"fetch failed"},
                     "adj": {"name":"adjudication","status":"complete"}}
  subprocess.run([sys.executable, "run_adjudication.py", "--judge", "stub"], input=json.dumps(d))
exit: 0 | stderr:
stages: [{"name": "adjudication", "status": "complete", "reason": null}]

A blocked pre-flight entry is gone and the document publishes as complete at exit 0. That is the worse half — not the missed re-run, but silent loss of a stage that recorded the pipeline failing.

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 _check_not_already_adjudicated body plus surrounding lines (#264 250-278 vs here 319-347) compares equal line-for-line, and the manifest site's comment block plus both statements (#264 469-474 vs here 670-675) likewise. The only difference in the second region is the following comment block, which this PR rewrote for the new total_refutation status — the guard lines themselves are untouched. Nothing drifted.

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. run_adjudication.py:487 — out-of-ladder guard on an agreed-with severity — REFUTED.

The mechanical claim is true; the finding is not. The line is where the review says, and the probe reproduces exactly:

_apply_severity_rerating("x","Info","CONFIRMED","Info","judge says",[])  -> ('CONFIRMED', 'Info', None)
_apply_severity_rerating("y","High","CONFIRMED","Info","judge says",[])  -> ('UNPROVEN', 'High', "judge returned an out-of-ladder severity 'Info' ... falling back to the reported severity 'High'")

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. _apply_severity_rerating has exactly one caller, adjudicate() at :634. findings.validate runs before the judge loop and refuses an out-of-ladder input severity:

out-of-ladder input refused: input document fails findings.validate:
  report 'secrets-and-access' finding[0]: severity 'Info' is not a key of SEVERITY_ORDER

findings.py:214 is the guard, and it is already tested end-to-endIllegalInputSeverityTests.test_real_process_illegal_severity_exits_nonzero_no_stdout asserts non-zero exit and empty stdout for exactly this input. Nothing can construct the reviewer's scenario except a direct unit call.

(b) The limitation is documented, in three places. The module docstring at :70-73 states the short-circuit verbatim ("No severity key, or one equal to the finding's own reported_severity: unchanged from STEP 3/4"), and at :90-95 states the layering and its reliance explicitly: severity falls back to reported_severity "(guaranteed legal at this point by STEP 3's input validation) or, purely as a second layer should that guarantee ever be bypassed, to "Blocker"". The test module docstring at :33-35 names the hand-off to IllegalInputSeverityTests.

(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 (plan:609-616) accurately. But the same step's done-when — the criterion the plan designates as the test — assigns this exact case elsewhere, with a reason (plan:644-650):

"the SIBLING case — a fixture whose finding ARRIVES with an out-of-ladder severity value already — is STEP 3's job, not this control's: run_adjudication.py exits non-zero on it before any judge runs, so it is asserted there and not repeated here as a per-finding UNPROVEN case, since there is no legal reported_severity for such a finding to have been emitted WITH"

That last clause is an argument against the prose's demand: emitting UNPROVEN at a Blocker fallback means inventing a severity for a finding that never had a legal one. The implementation matches the done-when. Reading only the prose paragraph reaches the review's conclusion; reading the step to its end does not.

On STEP 10 reachability, which the review offers as the reason it will surface anyway: STEP 10's list does name "as a reported_severity on the input that the judge agrees with", but its stated assertion is "an out-of-ladder EFFECTIVE severity refused rather than published" — and a non-zero exit with empty stdout satisfies "refused rather than published", which is where STEP 6's done-when says the case is asserted and which the existing test already asserts. That STEP 10 must fail could not be verified, and under default-to-refuted the ambiguity resolves against the finding.

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 Blocker fallback at :499 is consequently reachable only via "judge proposes something else illegal over an already-illegal reported severity". One sentence of the plan should be reconciled with its own done-when. Nothing to change in this diff.

Spot-check of the reviewer's positive STEP 6 claims — four checked, all hold, plus one case it did not test

Zero findings vs refute-all — the "would banner every clean PR" case, and the highest-value one:

C refute-all  total_refutation: True   status: total_refutation   counts before/after: [2] [2]   ids unchanged: True
D zero-findings total_refutation: False  status: complete
E MIXED (1 REFUTED, 1 CONFIRMED) total_refutation: False  status: complete   <-- added, not reported

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:

containment byte-identical: True   |  containment finding keys: ['entry_point','evidence','kind','severity']
pr/merge_base/head identical: True True True   |   total_refutation with containment present: True

No verdict key added to the containment finding, the block byte-identical under sort_keys=True, and a containment finding alongside fully-refuted report findings does not suppress the flag.

The bare SEVERITY_ORDER[f["severity"]] subscript — holds on every output generated, and the input document was not mutated.

No defect found in the part declared sound.

Verdict

state severity file:line summary
CONFIRMED High launchpad/review-agent/run_adjudication.py:327 Non-list stages skips re-run detection and discards every prior entry incl. a blocked pre-flight, publishing complete at exit 0 (also :674) — inherited from #264, byte-identical, fix there
REFUTED Low launchpad/review-agent/run_adjudication.py:487 Agreed-with out-of-ladder severity is unreachable via the only caller (findings.validate refuses it, tested end-to-end), documented in three places, and STEP 6's own done-when assigns the case to STEP 3; residual is a plan prose/done-when contradiction, not a code defect

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 by:agent label, is already resolved.

This PR is blocked by inheritance, not by anything it introduced.

🤖 Adjudicated by Claude Code (claude-opus-5) for @serina-mcfall. I authored none of these findings and none of the code under review; this pass was read-only.

serina-mcfall added a commit that referenced this pull request Aug 21, 2026
…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 benmitchell11 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@serina-mcfall

Copy link
Copy Markdown
Author

The requested change has landed — flagging for re-review

@ciaran-slow's CHANGES_REQUESTED on this PR named one inherited Blocker: non-list stages silently discarded, with the instruction to "fix on #264's branch and let it propagate." That has happened.

The fix: 57941045c"refuse a present-but-malformed stages manifest" — on feat/review-agent-adjudication-nonce (#264), merged forward through #266 (ec4dd4356) and #267 (965c2325b).

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 stages carrying a blocked pre-flight entry now exits 1 and is refused. 230 tests OK in a clean worktree.

Also resolved since that review: the cross-step notes drift @ciaran-slow flagged (four steps agreeing the field exists, none connecting them) — addressed on both ends, at 4a2bf04ba on #263 (documenting the deferral where the runner discards the key) and 05a960478 on #265 (adjudicator.md no longer mandating a channel the runner drops).

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 — _apply_severity_rerating's no-re-rating branch returned reported_severity without checking it, so a value that arrived illegal and that the judge agreed with was published untouched:

_apply_severity_rerating("x", "Info", "CONFIRMED", "Info", None, [])
  -> ('CONFIRMED', 'Info', None)

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 ADJUDICATION.md promises the guarantee holds on the effective severity. Fixed in c73a9ba73 on #266's branch — UNPROVEN, severity falls back to Blocker, reason names the refusal, nothing added to downgrades. 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.

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 check runs — an earlier failure and a later re-run that passes (e.g. 04:57:41 failure, 05:13:52 success on 965c2325b). Filtering the rollup on "any failure" reports these PRs as red when the latest run per check name is green. Worth taking the newest run, not any run.

🤖 Posted by Claude Code for @serina-mcfall. Claude-only pass on the severity fix — no independent cross-model check on that specific commit.

@serina-mcfall

Copy link
Copy Markdown
Author

Requested changes are done — but I am not the right party to certify it

Disclosure first: I wrote one of the commits on this branch (ec4dd4356, the propagation merge). Status report, not an independent review.

@ciaran-slow's change-request (2026-08-21T03:57Z) is stale — the head has moved twice since, to c73a9ba73. Both findings are addressed:

  • The inherited stages Blocker at :327/:674. Closed by propagation from feat(launchpad): nonce check and stages manifest (#118 STEP 4) #264 rather than by a second copy of the guard — StagesShapeError is present at this head. Re-verified through the real CLI here: the object-container bypass now exits 1 and the blocked pre-flight is no longer discarded, while the well-formed list control still passes with both entries in order. That was the right way to fix it; two divergent copies of one guard would have been worse than the bug.
  • The out-of-ladder Medium at :487. Addressed by c73a9ba73 — "guard the effective severity, not only a re-rating."

Worth saying plainly about that second one: an independent adjudication pass had refuted it. The reasoning was that findings.validate refuses an out-of-ladder input severity before the judge loop with an end-to-end test, the behaviour is documented in three places, and STEP 6's own done-when assigns the case to STEP 3 with a stated reason. So it was filed as a plan-clarity item (#286), not a code defect.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

by:agent Filed or authored by an AI agent, not a human

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants