feat(launchpad): dedupe via an injectable second judge (#118 STEP 7) - #267
Conversation
Groups findings that describe the same defect in different words, possibly from different review dimensions (finding_id differs by construction since dimension is a hash input), into adjudication.duplicate_groups with a deterministic survivor and a duplicate_of back-reference on every non-survivor -- discoverable from either the finding or the top-level block. The Judge protocol (judge(finding, input_document) -> dict) is called once per finding, independently, so it structurally cannot see across findings to detect a duplicate. Rather than reshape that existing, already-tested protocol, dedupe gets its own separate injectable callable, dedupe_judge, called once after every finding is adjudicated over the full list -- mirroring how the primary judge already defaults to stub_judge to prove the harness before a real model exists. stub_dedupe_judge finds no duplicates by default: never merging incorrectly is safer than merging wrongly. Survivor selection (highest severity, then CONFIRMED > UNPROVEN > REFUTED, then lowest finding_id) is deterministic code, independent of whichever mechanism decides who is a duplicate of whom. verdicts.validate already rejects a duplicate_of naming an absent id or itself (STEP 2); confirmed here with tests, not reimplemented. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Review pipeline — PR #267 (and the whole-branch pass for the #118 stack)Stages run: Not applicable, declared: Ran the suite: Ran 63 tests, OK. STEP 7's done-when, driven through
|
| Defect | Severity | Reported on |
|---|---|---|
whitespace-only verdict_evidence passes the fail-closed guard and the contract check |
High | #261 (verdicts.py:268), #263 (run_adjudication.py:188) |
judge-supplied notes silently dropped; adjudicator.md:66 instructs the judge to use them |
High | #265 (the instruction), #263 (:258, the wire) |
non-list stages silently discarded — bypasses the re-run guard, can drop a blocked pre-flight |
High | #264 (:258, :474) |
| out-of-ladder severity published when the judge agrees with it | Medium | #266 (:487) |
All four are still present in this diff's code, reached through the same lines. Counted once each, at the PR that introduced them.
Whole-branch pass (review-final)
Eleven commits, c4eb8bbe8..b5e934904. Read as a range, not as HEAD~1.
1. Does the branch do what #118 asked?
Partly, and honestly so. STEPs 2, 3, 4, 6 and 7 are built here; STEP 1 merged as #256; STEP 5 is #265. STEPs 8–12 are not built — fixtures, recorded judge outputs, the control suite, the mutation proof, and the PR step. Every PR in the stack says Refs #118 rather than Closes #118, which is the correct signal under launchpad/AGENTS.md §6. Nobody is claiming the issue is done.
What a reader of #118 would find working today: a CLI that takes a #117 merged document on stdin, validates it against #117's own contract, verifies the nonce three ways, adjudicates every finding through an injected judge, re-rates severity under a guard, records downgrades, flags total refutation, groups duplicates, and prints one document to stdout. I drove all of that end-to-end rather than reading it.
2. Did the steps drift from each other?
One real drift, and it is the notes field. Traced across every commit in the range:
- STEP 1 (
ADJUDICATION.md:137, already merged) declares it: "array of free-text notes". - STEP 2 (
verdicts.py:93) carries it as a dataclass field withdefault_factory=list, and:208explicitly excludes it from validation. - STEP 5 (
adjudicator.md:66, feat(launchpad): adjudicator.md -- what the judge is told (#118 STEP 5) #265) makes it normative: a genuinely new defect "does not become a finding: record it inadjudication.notes". - STEPs 3–7 (
run_adjudication.py:864) hardcodenotes=[], and the judge protocol at:226-234enumerates the keys it honours —verdict,verdict_evidence,severity,severity_reason— withnotesabsent.
Four steps agree the field exists and one step tells the judge to use it; no step connects the two. Probed: a judge returning a notes key produces adjudication.notes: []. This is invisible per-diff by construction — each step is correct on its own terms — which is exactly the category this pass exists for.
A second, milder one: the same falsiness idiom stands in for non-emptiness at both verdicts.py:268 and run_adjudication.py:188. The steps are consistent with each other and both wrong against the contract, so it reads as a shared convention rather than a drift. Reported on #261/#263.
3. Did the plan's OPEN items stay open?
Checked all nine. Yes, all of them. Two were worth verifying rather than assuming:
- Whether
verdict_evidenceneeds a second mechanism.adjudicator.md:83-87restates the exposure and explicitly declines to solve it, naming why a keyword filter would be the wrong fix. Left open. - Whether severity may be lowered at all. The plan chose "yes, with a reason and a recorded downgrade" while flagging the opposite reading as defensible.
_apply_severity_reratingimplements exactly that, and thedowngradeslist is the artefact the plan said would let the choice be revisited "without rebuilding the stage". The plan decided; the code did not decide something further.
4. Was anything from LEFT OUT built anyway?
No. Verified each:
- Finding new defects — the runner never adds to
reports[].findings; the id set is asserted equal before printing. - Deciding whether the PR merges — no
approved,mergeableormerge_recommendationkey can exist;_find_forbidden_keyswalks the whole document for them. - Publishing — no publisher module exists on this branch, no
subprocess, no HTTP client, noghcall. The module reads stdin and writes stdout. - Adjudicating containment findings — probed: a containment finding emerges with keys exactly
['entry_point','evidence','kind','severity'], noverdictadded, and the wholecontainmentblock is byte-identical to the input. - Choosing the model — grepped the runner for every model vendor and family name: no match. The judge is injected.
5. Are steps reported BLOCKED genuinely still blocked?
No step in this stack was reported blocked.
6. Does the commit history tell the truth?
Yes, and unusually so. Seven feat/docs commits and four fix commits, each naming its step in the subject. The four fix commits are the author's own review findings closed inside the branch rather than papered over — f79aa3d64 (unhashable field values), 4d05bfc83 (valid-but-non-object JSON), 36348cc0d (nonce ordering), 2be90c629 (malformed reports shape), e79da04fb (unhashable judge severity). I read 36348cc0d's message expecting it to have broken STEP 3's validate-first guarantee and it had not: "findings.validate still runs before any finding reaches the judge loop — STEP 3's actual guarantee — just second now instead of first." The message reasons correctly about what the guarantee actually was.
7. Was every step actually gated?
The mechanical check does not apply — no SDD ledger, and the plan's headings are STEP N rather than ### Task N:, so check-ledger.sh exits on its vacuity guard rather than reporting anything. Stating that rather than pretending it passed.
What I can say: before this review, none of the nine open PRs had a single review or review comment on it — I checked the reviews and comments APIs on each. Five of the six #118 PRs now have one, and the author's own pre-PR review-code passes are recorded in the PR bodies with the fixing commit named. That is not the same as an independent gate per step, and the honest statement is that this stack has had one independent pass — this one — over five steps at once.
8. Does every field added to a shared contract have a consumer?
This is the question the skill says produced the largest defect cluster elsewhere, so I ran it properly rather than reasoning about it. No consumer exists for anything this stage emits, because #119 — the publisher — is not built: there is no publish, render or comment module anywhere under launchpad/review-agent/, and the only readers of adjudication.* in the tree are verdicts.py, run_adjudication.py and their own tests.
That would be a large finding if the plan had not already said so, field by field, in its OPEN section:
verdictandverdict_evidence: "computed here and dropped there… task: publish exactly one PR review comment, and re-review on push #119's generic path rendersdefectandfailureand nothing else." Named, with the reason the re-rating was put inseverityinstead (because task: publish exactly one PR review comment, and re-review on push #119 does sort by that).verdict_counts,downgrades,duplicate_groups: "This plan emits the raw material… and computes no metric. Whether task: record the five success signals over the first ten pull requests #121 or enh: agent workflow — PR review agent as a first-pass reviewer #109 owns the computation is not decided here."total_refutation: has a real consumer path by design — it drives thestagesstatus, which task: publish exactly one PR review comment, and re-review on push #119 already banners without needing a change.
So the producer-with-no-consumer cluster here is deliberate and documented, not an oversight — with one exception, and it is notes, which has no consumer and no producer. That is finding 2 on #265/#263 and it is the only member of this cluster I am reporting.
Triage of deferred and parked items
Nothing arrived deferred or parked from an earlier round — there were no prior reviews on any of these PRs. Triaging what this pass produced instead:
| Item | Call | Why |
|---|---|---|
whitespace verdict_evidence (#261/#263) |
must-fix before the stack merges | it defeats a fail-closed guarantee two modules state explicitly, and can publish a merge-blocking CONFIRMED with no evidence. Two lines. |
notes has no wire (#265/#263) |
must-fix before #265 merges | a normative instruction to use a channel that does not exist pushes text into verdict_evidence, the one field with no guard. |
non-list stages (#264) |
must-fix before the stack merges | a blocked pre-flight can be dropped and the document publishes as complete — the issue's own fifth-criterion failure, reached through a shape defect. One line at each of two sites. |
| out-of-ladder severity when the judge agrees (#266) | defer, with a control | unreachable through main() today. Defer the fix if you like, but add the missing control now, or the next refactor makes it reachable silently. |
the four by:agent labels |
defer — but do them in one sweep | pure metadata; costs one command per PR. |
| test suites not wired into CI | defer to STEP 10, and hold STEP 10 to it | stated deliberately at test_verdicts.py:9-13 with STEP 10 named as owner. Nothing currently enforces that STEP 10 does it, so this belongs in STEP 10's review as a blocking item. Note this is not the same situation as #260/#262, where nothing was stated. |
A correction to my own review of #261
Reading _build_duplicate_groups here changes one thing I wrote on #261. My finding there — that verdicts.validate accepts a duplicate_groups entry with an empty duplicates list and an unvalidated survivor — is still correct as a validator hole, and I re-probed it. But the failure scenario I gave said the runner might emit such a group. It cannot: :672-674 drops any group left with fewer than two distinct real finding_ids, and I probed that too. So that finding is reachable only from a hand-written or forged document, or from STEP 10's planned malformed-document controls — which makes it materially less urgent than I framed it. I have posted the correction on #261 rather than leaving the overstatement standing.
Merge readiness
Merge order is not optional here. The stack is #261 → #263 → #264 → #266 → #267, each based on its predecessor's branch. #265 is based on launchpad and can land anywhere, but it cites run_adjudication.py normatively, so landing it after #264 avoids a window where launchpad carries a document citing a function no file on launchpad defines.
What a reader of #118 would find true after this stack merges: five of its twelve steps built, each doing what its step specified, with the hard parts — nonce provenance, pass-through fidelity, escalate-only, deterministic dedupe — done correctly and for stated reasons. What they would find missing: STEPs 8–12, correctly not claimed; and four small defects, three of which are channels that do not carry what their documentation says they carry.
What I could not check: whether any of this behaves correctly under a real model. That is STEP 9's job by the plan's own design and every module says so. I tested the harness, the guards and the contract; I did not test a judge.
Independence and tools
Independent of the code under review: I did not write any of it and had no part in it. Not independent across pipeline stages — one context ran the reviewers, the adjudicator and this final pass, where the skills call for a fresh context per stage and a different model where possible. Concretely: the adjudicator did not refute a single reviewer finding here, and a genuinely independent adjudicator over the same reports would be expected to refute some. Read the severities as one reviewer's proposals, not as adjudicated verdicts. Nine candidate findings across the stack were refuted by probe, and I have recorded each on its PR rather than dropping it silently — that is the closest this arrangement gets to the real thing.
Tools actually held and used: Bash (git, git grep, git show, git log, git worktree, git merge-base, git cat-file, gh, gh api, python3 and subprocess for the probes), Read, Edit, Write. No Grep or Glob tool was available in this session — every search was git grep/grep through Bash, which is why the evidence is quoted as commands throughout.
Nothing found at Blocker. Nothing new to this PR above Low.
CONFIRMED Low PR #267 (labels) missing required by:agent label
Handed 1 finding new to this PR, confirmed 1, refuted 0. Four defects reachable in this diff are carried forward from #261, #263, #264, #265 and #266 and are not re-counted here; eight adversarial candidates against STEP 7 were REFUTED by probe and are recorded above. 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 reports readiness; it does not grant it. The merge decision is @ciaran-slow's.
Carried-forward severity updated: the
|
ciaran-slow
left a comment
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, and this PR also carried the review-final whole-branch pass over all 11 commits — detail in my two comments. Ran 63 tests ... OK.
No new defect in STEP 7 itself. All five done-when clauses pass, plus three adversarial cases the plan does not name: a dedupe judge that raises fails closed to []; one that groups a finding with itself has the group dropped; one that returns the same pair twice yields a single group. Two runs of the same input are byte-identical under json.dumps(sort_keys=True). _survivor_sort_key implements the plan's three-level rule as a plain sort key, so "the best one" is a rule rather than a judgement.
The whole-branch pass came back clean on most of what only it can see: the commit history tells the truth (four fix commits are the author's own review findings closed inside the branch, and 36348cc0d's message reasons correctly about what STEP 3's guarantee actually was); all nine of the plan's OPEN items stayed open; nothing from LEFT OUT was built — no publisher, no model named, containment findings passed through untouched.
One inherited Blocker, and nothing else.
Non-list stages silently discarded — :388 and :873 here, unchanged from #264, and I reproduced the bypass through the CLI on this exact branch: an adjudication entry inside an object container adjudicates at exit 0 instead of being refused, and a preflight entry with status: "blocked" is dropped so the document publishes as complete. Fix on #264's branch and let it propagate.
Two things from the branch pass worth carrying forward rather than acting on here:
- One real cross-step drift:
notes. Four steps agree the field exists and STEP 5 tells the judge to use it; no step connects them.ADJUDICATION.md:137declares it,verdicts.py:93carries it,adjudicator.md:66mandates it,run_adjudication.py:864hardcodes it empty. Invisible per-diff by construction. Resolution belongs on #263. - Producer-with-no-consumer is deliberate here, not an oversight. #119 is not built, so nothing reads anything this stage emits — but the plan's OPEN section names each field and says so, field by field. The only exception is
notes, which has no consumer and no producer, and that is the drift above.
STEPs 8–12 are correctly not claimed; every PR in the stack says Refs #118, not Closes.
ciaran-slow
left a comment
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, and this PR also carried the review-final whole-branch pass over all 11 commits — detail in my two comments. Ran 63 tests ... OK.
No new defect in STEP 7 itself. All five done-when clauses pass, plus three adversarial cases the plan does not name: a dedupe judge that raises fails closed to []; one that groups a finding with itself has the group dropped; one that returns the same pair twice yields a single group. Two runs of the same input are byte-identical under json.dumps(sort_keys=True). _survivor_sort_key implements the plan's three-level rule as a plain sort key, so "the best one" is a rule rather than a judgement.
The whole-branch pass came back clean on most of what only it can see: the commit history tells the truth (four fix commits are the author's own review findings closed inside the branch, and 36348cc0d's message reasons correctly about what STEP 3's guarantee actually was); all nine of the plan's OPEN items stayed open; nothing from LEFT OUT was built — no publisher, no model named, containment findings passed through untouched.
One inherited Blocker, and nothing else.
Non-list stages silently discarded — :388 and :873 here, unchanged from #264, and I reproduced the bypass through the CLI on this exact branch: an adjudication entry inside an object container adjudicates at exit 0 instead of being refused, and a preflight entry with status: "blocked" is dropped so the document publishes as complete. Fix on #264's branch and let it propagate.
Two things from the branch pass worth carrying forward rather than acting on here:
- One real cross-step drift:
notes. Four steps agree the field exists and STEP 5 tells the judge to use it; no step connects them.ADJUDICATION.md:137declares it,verdicts.py:93carries it,adjudicator.md:66mandates it,run_adjudication.py:864hardcodes it empty. Invisible per-diff by construction. Resolution belongs on #263. - Producer-with-no-consumer is deliberate here, not an oversight. #119 is not built, so nothing reads anything this stage emits — but the plan's OPEN section names each field and says so, field by field. The only exception is
notes, which has no consumer and no producer, and that is the drift above.
STEPs 8–12 are correctly not claimed; every PR in the stack says Refs #118, not Closes.
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 unchanged: The reported 63 is Bottom line: nothing this PR introduces blocks it. Both findings are inherited and homed on earlier branches. Because this PR was declared clean, the highest-value work here was attacking the positive claims — five reached, all held, two phrasings corrected, and six left unverified and named below. Findings1. No upstream guard: Probed through the real CLI on this branch: The control proves the guard works on the shape it expects. C is the worse half: Same root cause as #264, line-shifted only — verified here rather than taken on trust. At #264's head the identical guards sit at 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; one of them, the "Every container is type-checked" docstring, does not even exist on #264's branch). High blocks the merge, so the fix must still land before this chain does. This PR should not be blocked in its own right. The severity attaches to the root cause; #267 clears automatically when #264's fix propagates. Do not patch it here — two divergent copies of one guard is worse than the guard. 2. Verified at all four sites, including the one on another branch:
Probed: a judge returning The decisive test — is it a documented deferral? No. The plan's OPEN section was read in full: it names Medium rather than High: on this branch Home is #263, which introduced both the judge protocol and the Adversarial spot-checks of the "no new defect" claims(a) Is
Also checked, and not stated in the review: (b) Does a raising dedupe judge fail closed to
Caveat, recorded so it is not mistaken for total coverage: (c) Was anything from LEFT OUT built? — NO. Verified. One phrasing corrected.
What was NOT reached — silence here is not confirmation
Verdict
Handed 2. 2 confirmed, 0 refuted, 0 merged. One severity moved down. Both findings homed on earlier branches; neither is this PR's to fix. No total refutation here — both findings were confirmed, and every positive claim reached held up, with two phrasings corrected and six claims left unverified and named above. 🤖 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>
…scalate' into feat/review-agent-adjudication-dedupe Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Brings #264's `StagesShapeError` / `_input_stages` and #261/#263's `verdicts.is_nonempty_str` up to the chain tip. Both of this PR's findings were homed on earlier branches -- the `stages` Blocker on #264 and the `notes` drift on #263 -- so this branch is cleared by propagation rather than by any change of its own, which is what the adjudication asked for. Clean merge, no conflicts. 230 tests across launchpad/review-agent. `notes` remains empty and is now documented as deferred at STEP 6/7, with the unresolved tension against `adjudicator.md` (#265) stated in the code. #265 should not merge ahead of that decision. Refs #118 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…118 STEP 5) Prohibition 2 told the judge to "record it in `adjudication.notes`". That is a normative instruction against a channel that silently drops its input: `run_adjudication.py` hardcodes `notes=[]` and its judge protocol reads only `verdict`, `verdict_evidence`, `severity` and `severity_reason`. Probed on the STEP 3 branch -- a judge returning a `notes` key produces `adjudication.notes: []`, and the document validates clean against both contracts, so nothing catches the loss. `ADJUDICATION.md` declares the field and `verdicts.py` carries it, which is what made the instruction look supported. Nothing between the declaration and the producer connects them. Adjudicated across #263 and #267 as one cross-step drift. Resolved on this side rather than by plumbing `notes` through the protocol, because collecting and attributing notes -- per-finding or per-document, deduped, ordered -- is STEP 6/7's design, and building that surface here would pre-empt a decision those steps own. The deferral is now recorded from both sides: this document and `run_adjudication.py`'s module docstring. Also states the consequence the old wording left implicit: with the channel deferred, a new observation has nowhere to go from this stage, so `verdict_evidence` must not be stretched to carry it. That field is the reason for the verdict on the finding the judge was given, and it is the one field with no structural guard -- exactly the wrong place for overflow. Doc-only. 121 tests across launchpad/review-agent green on this branch. Refs #118 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
benmitchell11
left a comment
benmitchell11
left a comment
There was a problem hiding this comment.
Independent pass (not the pipeline that left the earlier reviews on this PR, including its review-final whole-branch pass).
Checked out feat/review-agent-adjudication-dedupe and ran the suite: Ran 230 tests ... OK in a clean worktree.
The inherited stages-shape Blocker is fixed here too, via 965c2325b ("Merge STEP 6 into STEP 7, carrying the stages-shape fix"). Re-ran the CLI probe myself on this exact branch (object-shaped stages with a blocked pre-flight entry) rather than assuming the merge carried it cleanly:
stages = {"p": {"name":"preflight","status":"blocked",...}} -> exit 1, refused
Confirmed fixed on the chain tip.
The out-of-ladder-agreement Medium from #266 is also still present here (same code, unchanged): _apply_severity_rerating("x", "Info", "CONFIRMED", "Info", None, []) still returns ('CONFIRMED', 'Info', None) on this branch. Same assessment as on #266 — not reachable via main() today, not requesting changes on this branch since it isn't this PR's own defect, but noting it doesn't disappear just because it's inherited.
Dedupe logic itself (_build_duplicate_groups/_survivor_sort_key/_run_dedupe_safely, :700-783) — read it directly rather than trusting the standing review's account:
_run_dedupe_safelyfails closed to[]on either a raised exception or a non-list return — confirmed by reading thetry/except+type-check, matches the claim._build_duplicate_groups'sclaimedset correctly prevents one finding from landing in two groups (first group wins, in the order the dedupe judge returned them) and drops any group left with fewer than two real, distinct finding_ids after filtering — covers self-duplication and the "same pair returned twice" case structurally, not just by the specific tests that exercise them._survivor_sort_key's tiebreak chain (severity rank, then verdict rank, thenfinding_id) meansmin()never hits a true tie, sincefinding_idis already deduplicated into the candidate list beforemin()runs — so the "byte-identical across repeated runs" claim (json.dumps(sort_keys=True)) is structurally justified, not just empirically observed to hold in the tests that check it.- Ordering in
adjudicate()(:895-945) is dedupe-after-verdict-and-severity-are-final, called exactly once with the whole finding list, never per-finding — matches the docstring's claim, and a duplicate is never removed fromfindings_out's count, confirmed by reading wherefindings_outis set (before the dedupe block runs at all).
Agree with the standing review's finding-count for STEP 7 itself: no new defect in the dedupe logic beyond what's inherited from earlier steps.
Not verified: no live-model dependency in this step; did not independently re-run the full review-final whole-branch pass over all 11 commits that the standing review describes (commit-history audit, OPEN-items-stayed-open check) — took that account at face value rather than re-deriving it, since it's assertions about history and scope rather than about code behavior.
STEP 6) benmitchell11's independent pass on #266/#267 found that _apply_severity_rerating's no-re-rating branch returned reported_severity without ever checking it, so an out-of-ladder value the judge agreed with (or said nothing about) was published untouched: _apply_severity_rerating("x", "Info", "CONFIRMED", "Info", None, []) -> ('CONFIRMED', 'Info', None) Reproduced before fixing. This is a plan-conformance gap, not merely defence in depth: STEP 6's own done-when names this exact case -- "a guard watching only re-ratings never sees a finding that ARRIVED at 'Info' and was agreed with, and copies it into `severity` untouched" -- and ADJUDICATION.md promises the guarantee holds on the EFFECTIVE severity, the re-rating where there is one and reported_severity where there is not. Fixed by checking reported_severity inside that branch, ahead of the return that used to copy it: verdict becomes UNPROVEN, severity falls back to Blocker (not something smaller -- this stage may not decide an unrateable finding is minor), severity_reason names the refusal, and nothing is added to downgrades since no legal value fell. Unreachable through main() today (STEP 3's findings.validate refuses an out-of-ladder input severity before any judge runs) and kept as a real branch regardless: adjudicate() is importable, and STEP 10's control suite is planned to feed this function malformed values directly. Proved the new tests can fail: removing the guard on a scratch copy fails exactly the two new sub-cases while the legal-path control still passes. 71 tests OK, run_controls 13/13, all seven suites OK. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
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 to
So this PR is cleared by propagation rather than by any change of its own — which is what the review asked for, and worth recording as the correct outcome rather than an absence of work. Still open and not blocking this PR: #265 holds the One thing an independent reviewer should press on that I cannot: the adversarial pass on this PR reported no new defect in STEP 7 itself, and six of its positive claims were confirmed while six were explicitly left unverified — including whether two runs are byte-identical, and whether the plan's nine OPEN items all stayed open. Those gaps are named rather than papered over, but they are gaps. What this needs: a re-review from @ciaran-slow rather than a clearance from me. Re-requested. CI is green. |
39c07cb
into
feat/review-agent-adjudication-escalate
4 participants
Summary
Implements STEP 7 of
launchpad/plans/2026-08-13-issue-118-adjudication.md: dedupe, discoverable from both ends. Findings describing the same defect in different words are grouped, with every duplicate also naming its survivor viaduplicate_of, and nothing is dropped. Stacked on the not-yet-merged STEP 6 branch (#266).Related issue
Refs #118
Issue type
Task
Agent provenance
Objective
Extend
launchpad/review-agent/run_adjudication.pywith cross-dimension dedupe, via a second injectable judge called once after every verdict is final.Impacted components
launchpad/review-agent/run_adjudication.pylaunchpad/review-agent/test_run_adjudication.pyApproach and rejected alternatives
The plan specifies the outcome shape but not the detection mechanism, so this is a decision made and recorded rather than inherited. The existing per-finding
Judgeprotocol has no visibility into other findings, so it structurally cannot detect a cross-finding duplicate.Rejected: reshaping
Judgeto receive all findings. That ripples through STEPs 3/4/6's existing code and tests for a conceptually separate question. Instead a second callable,dedupe_judge(adjudicated_findings, input_document) -> list[list[finding_id]], is called exactly once after severity re-rating is final. It defaults tostub_dedupe_judge, which finds nothing — the conservative direction, mirroring ADJUDICATION.md's UNPROVEN-over-REFUTED default.Nothing is dropped: a duplicate still receives its own verdict and stays in
reports[].findings. The survivor is chosen by a plain sort key — highest adjudicated severity, then CONFIRMED > UNPROVEN > REFUTED, then lowestfinding_id— so "the best one" is a rule rather than a judgement.Verification
Command run:
Raw output:
The 63 is
test_run_adjudicationalone, not the whole review-agent suite — the 230 run is the directory. Both are after merging #266, which carries #264'sstagesfix and #261/#263's evidence guard up to this branch.Totality of the survivor sort key, checked because a tie at all three levels would make the byte-identical-output guarantee order-dependent:
Not verified
No live model has ever deduped anything.
stub_dedupe_judgereturns[], so every group in every test comes from a hand-written injected judge. Whether a real model groups sanely — or groups aggressively enough to merge two genuinely distinct defects — is entirely unmeasured. That is the risk this step's conservative default is chosen against, not evidence against it.Byte-identical output across two runs was reasoned, not re-executed on this branch. The premise is now verified (the sort key is total,
duplicatesissorted(), groups append in the judge's order), and a committed test asserts it — but the independent re-measurement was not repeated after the #266 merge.Two adversarial dedupe cases were confirmed by inspection, not driven: a judge grouping a finding with itself, and one returning the same pair twice. Both follow from the
len(candidate_ids) < 2gate and theclaimedset.except Exceptionin the dedupe fail-closed path does not catchBaseException. A dedupe judge raisingSystemExitorKeyboardInterruptaborts the run rather than failing closed to[]. Same convention_run_judge_safelyalready uses, and arguably correct forSystemExit, but it is not total coverage."All nine plan OPEN items stayed open" was not audited item by item. The OPEN section was read in full for the
notesquestion only.Nothing reads any of this. #119 is not built, so
duplicate_groupsandduplicate_ofare producer-with-no-consumer — deliberate, and named field by field in the plan's OPEN section.Security implications
Dedupe is the one step that can make a finding less visible, so the failure mode is a real defect hidden behind a survivor. Three properties limit that: nothing is removed (a duplicate keeps its own verdict and its place in
reports[].findings), the grouping is discoverable from both directions so a reader cannot see a group without seeing its members, and survivor choice is a deterministic sort rather than a judgement.A badly-behaved dedupe judge fails closed: single-element groups are dropped, absent
finding_ids are filtered, a finding claimed by two groups is resolved by first claim, a non-list return becomes[], and an exception becomes[]. None of those reachduplicate_groupsorduplicate_of.Nothing from the plan's LEFT OUT section was built. Verified: the import block is
argparse, copy, json, sys, pathlib, typingplusfindings, review, verdicts— nosubprocess, no HTTP client, nogh; no publish, render or comment module exists in the directory; andgrep -rniE "claude|gpt|opus|sonnet|gemini|anthropic|openai|llama|mistral"over the module returns zero hits, so no model is named.Escalations
stagesshape on feat(launchpad): nonce check and stages manifest (#118 STEP 4) #264, thenotesdrift on feat(launchpad): run_adjudication.py -- the adjudication CLI (#118 STEP 3) #263 — and both are now fixed there and merged up. This branch is cleared by propagation.Judge; if that is the wrong shape, the cost of changing it grows with every step built on top.review-finalhas not been run for this branch. The plan's GATES section schedules it at STEP 12, which is where the whole-chain pass belongs — this PR carried an early whole-branch pass over its 11 commits, which is not the same thing.🤖 Drafted by Claude Code (
claude-opus-5) for @serina-mcfall.