chore(launchpad): land #118 STEPs 4, 6, 7 and 8 — stranded off launchpad - #567
Closed
serina-mcfall wants to merge 14 commits into
Closed
chore(launchpad): land #118 STEPs 4, 6, 7 and 8 — stranded off launchpad#567serina-mcfall wants to merge 14 commits into
serina-mcfall wants to merge 14 commits into
Conversation
Extends run_adjudication.py's adjudicate() with two things it did not previously do: - _verify_nonce() checks the top-level `nonce` against every report's own completion marker and raises NonceVerificationError naming exactly one of three refusals, in fixed order: "absent provenance" (no top-level nonce, or a report's marker does not parse), "mixed document" (reports disagree with each other -- wins over the third case when both apply), and "mismatched envelope" (reports agree with each other but not the top-level key). Runs after #117's own findings.validate, never before, and never invents or accepts a caller-supplied nonce. - The output now carries a top-level `stages` array: every entry present on input plus exactly one new {name: "adjudication", status, reason} entry. AlreadyAdjudicatedError refuses a document whose `stages` already carries an `adjudication` entry, rather than silently overwriting it. `status` is "complete" only when every finding has a verdict and the nonce was established -- STEP 6's total-refutation flag isn't built yet, so it is not one of the two live conditions today, but the computation is structured so that flag slots in later without a rewrite. Because findings.validate() already checks every report's marker nonce against the top-level key, every fixture that exercises the three nonce refusals through main() end-to-end is *also* caught there first, with its own generic (and, between "mixed" and "mismatched", indistinguishable) message -- so the three refusals' own distinct reasons are proven directly against _verify_nonce with hand-built documents, not observable through the CLI today. Both are tested: the dedicated check is real defence in depth per ADJUDICATION.md's own stated reasoning, and main()'s "exits non-zero, prints no document" contract holds either way. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…STEP 4) adjudicate() ran #117's findings.validate first, which independently rejects a document whose report markers disagree with the top-level nonce -- but with one generic per-report message that does not distinguish "reports disagree with each other" (mixed document) from "reports agree with each other but not the top-level key" (mismatched envelope). Since every nonce problem findings.validate can see is a problem _verify_nonce can also see, findings.validate always won the race, so the three distinct refusals STEP 4's plan requires to be observable end-to-end were provably unreachable through main() -- only testable by calling _verify_nonce directly. Fixed by running _check_not_already_adjudicated and _verify_nonce before findings.validate. findings.validate still runs before any finding reaches the judge loop -- STEP 3's actual guarantee -- just second now instead of first. Strengthened NonceVerificationEndToEndTests to assert the specific reason text in main()'s stderr (mixed document / mismatched envelope / absent provenance), not just exit code and empty stdout, so a regression that reverts the ordering fails a test again. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…ovenance (#118 STEP 4) review-code found that checking _verify_nonce before findings.validate (the previous fix in this branch) has a side effect: a document whose reports key is missing, non-list, or empty has nothing for nonce verification to compare against, so _verify_nonce calls it "absent provenance" -- technically true, but it buries findings.validate's more specific message for exactly that shape defect ("missing required key 'reports'", "must not be empty", "expected an array"), pointing a maintainer at the wrong subsystem. Fixed by checking reports is a present, non-empty list before running _verify_nonce at all; when it isn't, defer straight to findings.validate, which already names the real problem. The _verify_nonce call in that branch is unreachable in practice (a malformed reports always fails findings.validate on one of those three grounds) but kept as a real call rather than asserted away, matching this module's existing "real branch, not assumed" discipline for stage_complete's nonce_established condition. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
#118 STEP 6) Extend run_adjudication.py's adjudicate() with the three STEP 6 behaviours: a judge's return dict may now carry a severity re-rating, guarded so an out-of-ladder value it produces is refused (UNPROVEN at reported_severity, never published) rather than copied through; a genuine downgrade is recorded into adjudication.downgrades at the moment it is applied, never by a later sweep; and total_refutation now surfaces in the stages manifest's own status ("total_refutation", not "complete") rather than only in adjudication's own boolean. Also adds a belt-and-braces finding_id set-equality check inside adjudicate() itself, raising before the document is ever printed. Verified against STEP 6's own done-when: a REFUTED-everything judge leaves findings/findings_count unchanged and flips the stage status; the same judge against zero findings stays "complete"; a judge returning "Info" over a legally in-ladder reported_severity is refused with a reason and still passes verdicts.validate; a bare review.SEVERITY_ORDER[...] subscript succeeds on every finding in every output; and a Blocker-to-Low downgrade is named in adjudication.downgrades with from/to/reason. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…118 STEP 6) review-code found that _run_judge_safely forwarded a judge's severity value with no type check, and _apply_severity_rerating's `proposed_severity not in review.SEVERITY_ORDER` raises TypeError on an unhashable value (a list or dict) instead of failing closed to UNPROVEN -- reachable today through make_replay_judge (a malformed --replay recording), confirmed by reproducing the crash before fixing it. Fixed by only forwarding severity/severity_reason when severity is a str, matching the type discipline verdict/verdict_evidence already get in the same function. A non-string severity is now treated as no re-rating at all rather than crashing the whole run. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Groups findings that describe the same defect in different words, possibly from different review dimensions (finding_id differs by construction since dimension is a hash input), into adjudication.duplicate_groups with a deterministic survivor and a duplicate_of back-reference on every non-survivor -- discoverable from either the finding or the top-level block. The Judge protocol (judge(finding, input_document) -> dict) is called once per finding, independently, so it structurally cannot see across findings to detect a duplicate. Rather than reshape that existing, already-tested protocol, dedupe gets its own separate injectable callable, dedupe_judge, called once after every finding is adjudicated over the full list -- mirroring how the primary judge already defaults to stub_judge to prove the harness before a real model exists. stub_dedupe_judge finds no duplicates by default: never merging incorrectly is safer than merging wrongly. Survivor selection (highest severity, then CONFIRMED > UNPROVEN > REFUTED, then lowest finding_id) is deterministic code, independent of whichever mechanism decides who is a duplicate of whom. verdicts.validate already rejects a duplicate_of naming an absent id or itself (STEP 2); confirmed here with tests, not reimplemented. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…STEP 4) Adjudicated on #264 as the root cause inherited unchanged by #266 and #267. A `stages` value that was present but not a list was treated as absent at both sites that read it: `_check_not_already_adjudicated` returned early, and the manifest builder substituted `[]`. Two failures from one shape defect, reproduced through the real CLI: stages = [{"name":"adjudication",...}] -> exit 1, correctly refused stages = {"0":{"name":"adjudication",...}} -> exit 0, guard bypassed stages = {"p":{"name":"preflight","status":"blocked",...}} -> exit 0, entry GONE, "complete" stages = 42 / "adjudication" / True -> exit 0, dropped The second is the expensive one. A `blocked` pre-flight -- #116's fork-PR-secrets-withheld case -- disappeared and the document published as a clean, complete review, because #119 only banners a non-`complete` status. That is #118's fifth criterion failing through a shape defect no verdict-side check looks at: `grep -n stages findings.py verdicts.py` returns nothing, so neither contract validator inspects this key at all. "#117 never emits that shape" is not available as a defence here. The `stages` manifest is explicitly an output #117 does NOT produce, so there is no upstream guarantee for this stage to inherit. Fix: one `_input_stages` reader, raising `StagesShapeError`, used by BOTH sites. One function rather than two corrected inline checks on purpose -- the two readers each had their own `isinstance(..., list)` test and each treated malformed as absent, which is how one defect became two independent failures. The same lesson as `verdicts.is_nonempty_str` on #261/#263: a second copy of a rule is a second chance to disagree with it. The comment claiming `_check_not_already_adjudicated` "already guarantees" no input entry is named `adjudication` is now actually true, and says why. Absent stays legal, and an explicit null reads as absent -- #117 emits no `stages` key at all, so a fix that refused absence would break every real document. Both are tested as controls, as is a well-formed `blocked` pre-flight surviving in order. Also closes the Low at the same site: a `stages` entry that is not an object, or whose `name` is not a string, is refused in the same change. 11 tests added, each failing before the change, including the refusal driven through the real process. 57 tests in test_run_adjudication (was 46 after merging #263), 207 across launchpad/review-agent. #266 and #267 carry these two guard sites byte-identically, only line-shifted. Fixing here rather than there so one change propagates up the chain instead of becoming three copies that can diverge. Refs #118 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Brings in #264's `StagesShapeError` / `_input_stages` fix and #261/#263's `verdicts.is_nonempty_str` guard. This PR's inherited Blocker is now closed by propagation rather than by a second copy of the guard, which is the point -- two divergent copies of one check is worse than the bug. Verified on this branch through the real CLI after the merge: an `adjudication` entry inside an object container exits 1 with no document (was exit 0), a `blocked` preflight in an object container exits 1 rather than being silently discarded (was published as `complete`), and the control -- a well-formed list carrying a `blocked` preflight -- still exits 0 with both entries in order. Conflicts were docstring and error-class additions only; both sides kept. 219 tests across launchpad/review-agent. Refs #118 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…scalate' into feat/review-agent-adjudication-dedupe Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Brings #264's `StagesShapeError` / `_input_stages` and #261/#263's `verdicts.is_nonempty_str` up to the chain tip. Both of this PR's findings were homed on earlier branches -- the `stages` Blocker on #264 and the `notes` drift on #263 -- so this branch is cleared by propagation rather than by any change of its own, which is what the adjudication asked for. Clean merge, no conflicts. 230 tests across launchpad/review-agent. `notes` remains empty and is now documented as deferred at STEP 6/7, with the unresolved tension against `adjudicator.md` (#265) stated in the code. #265 should not merge ahead of that decision. Refs #118 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
STEP 6) benmitchell11's independent pass on #266/#267 found that _apply_severity_rerating's no-re-rating branch returned reported_severity without ever checking it, so an out-of-ladder value the judge agreed with (or said nothing about) was published untouched: _apply_severity_rerating("x", "Info", "CONFIRMED", "Info", None, []) -> ('CONFIRMED', 'Info', None) Reproduced before fixing. This is a plan-conformance gap, not merely defence in depth: STEP 6's own done-when names this exact case -- "a guard watching only re-ratings never sees a finding that ARRIVED at 'Info' and was agreed with, and copies it into `severity` untouched" -- and ADJUDICATION.md promises the guarantee holds on the EFFECTIVE severity, the re-rating where there is one and reported_severity where there is not. Fixed by checking reported_severity inside that branch, ahead of the return that used to copy it: verdict becomes UNPROVEN, severity falls back to Blocker (not something smaller -- this stage may not decide an unrateable finding is minor), severity_reason names the refusal, and nothing is added to downgrades since no legal value fell. Unreachable through main() today (STEP 3's findings.validate refuses an out-of-ladder input severity before any judge runs) and kept as a real branch regardless: adjudicate() is importable, and STEP 10's control suite is planned to feed this function malformed values directly. Proved the new tests can fail: removing the guard on a scratch copy fails exactly the two new sub-cases while the legal-path control still passes. 71 tests OK, run_controls 13/13, all seven suites OK. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…ed (#118 STEP 8) (#450) * feat(launchpad): adjudication fixtures, four of five genuinely produced (#118 STEP 8) STEP 8's stated premise was dead and is corrected in the plan (struck through, not deleted) exactly as its own BUDGET section instructed: "#117's producer does not exist -- there is no run_dimensions.py anywhere" has not been true since #117 merged. Fifteen real recorded reviewer outputs live under recordings/, and test_recordings.py's own ReplayValidityTests already replayed them through build_document, so the harness this step needed was already in the tree. Measured before building, not assumed. Four of the five named behaviours are now genuinely produced by replaying real recorded output through the real producer: - paraphrase gives BOTH the three-report all-anchor-line document and the dedupe document -- all three dimensions independently reported the same defect at gate.rs:42 with three different finding_ids, since dimension is a hash input. One document isolates both behaviours; committing it twice, or inventing a second, would be the dishonest alternatives. - claim-vs-evidence gives the pr-anchored fixture. Its real output is two findings (anchors line and pr); kept whole per Serina's call rather than trimmed to the single-finding shape the plan first described, since a trim would be a real replay with a finding deleted by hand. - secrets-and-access plus a reviewer injected to raise gives the failed/clean/findings document -- the failed report comes from _collect_report/_failed_report, not from hand-written JSON. The containment fixture is the one genuine exception and the split is stated rather than blurred: no existing fixture trips the detectors (all eight checked, every one yields zero containment findings), so its surfaces are crafted and its containment block and states map are then produced by the real contain.render. Crafted input, real pipeline -- never described as recorded. Regenerating reproduces the committed bytes exactly, which is what makes the provenance claim checkable rather than asserted: every nonce derives from the relevant recording's own _provenance.seed via contain.make_nonce(seed=...), never freshly randomised. Verified: 18 new tests OK; each fixture accepted by findings.validate with zero violations; each accepted by run_adjudication.py at exit 0; containment fixture carries all three kinds and exactly seven states keys; regeneration byte-identical; run_controls 13/13 and all eight suites OK. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com> * docs(launchpad): drop a citation a reader cannot open (#118 STEP 8) review-code flagged, as context rather than a finding, that generate.py attributed the keep-the-two-finding-document decision to "Serina's call recorded in this repo's session notes" -- and no such artifact exists in the repo to check it against. That is precisely the uncheckable citation this plan's own conventions exist to prevent: its ALREADY TRUE section carries a standing rule that evidence is what `git show` returns from a named commit, adopted after three separate rounds of cross-issue claims that were true-then- falsified, never-true, and true-but-misattributed. A pointer to out-of-repo session notes is weaker than all three. Replaced with the argument itself, made from what the recording actually contains: trimming would turn a real replay into a real replay with a finding deleted by hand, and the two-finding document exercises a 'pr' anchor alongside a 'line' one rather than in isolation. That reasoning is checkable against the fixture; the attribution was not. Regenerated so the committed fixture carries the corrected note. 18 tests OK (including the byte-reproducibility check), controls 13/13. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com> --------- Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
The four PRs carrying #118 STEPs 4, 6, 7 and 8 -- #264, #266, #267 and #450 -- each merged into their stacked parent branch rather than into launchpad. All four read MERGED on GitHub, and none of their work is reachable from launchpad: git merge-base --is-ancestor origin/feat/review-agent-adjudication-dedupe \ origin/launchpad -> false run_adjudication.py is 364 lines on launchpad and 1140 here. StagesShapeError (STEP 4) appears 7 times here and 0 there. The whole fixtures/adjudication tree from STEP 8 is absent from launchpad. This branch was 72 commits behind launchpad, so opening it directly would have reverted work that landed since -- the four observability current-state docs, pr_review_batch.py, adjudicator.md, and #505's privacy fix to note 315. Merging launchpad in first removes that: the resulting diff is 3,058 insertions and 49 deletions, where the unmerged branch would have shown 10,039 deletions. The merge is clean -- no conflicted files. Both sides verified present afterwards, and both suites pass on the result: launchpad/review-agent 250 tests OK launchpad/scripts 256 tests OK Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LuDy9K8WDtowz5e2Uz2wjz Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
serina-mcfall
marked this pull request as ready for review
August 24, 2026 02:06
Author
|
Closing in favour of #566, which landed 14 minutes earlier, comes straight off the dedupe branch, and is a superset (includes ADJUDICATION.md, which this PR omits). The merge-forward here was based on a two-dot diff and was not needed -- git merges from the merge base, so the stale branch was never going to revert anything. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lands #118 STEPs 4, 6, 7 and 8, which are currently stranded. Their four PRs each merged into a stacked parent branch rather than into
launchpad, so all four read MERGED while none of their work is reachable from the default branch.Related issue
Refs #118
Issue type
Task
Agent provenance
The problem this fixes
feat/review-agent-adjudication-runfeat/review-agent-adjudication-noncefeat/review-agent-adjudication-escalatefeat/review-agent-adjudication-dedupeNone of the four branches is an ancestor of
launchpad. The PR badges say merged because each merge really happened — into the wrong target. Merge status and reachability are different questions, and only the second says whether code shipped.What is missing from
launchpadtoday:launchpad/review-agent/fixtures/adjudication/— the whole STEP 8 tree, seven files — is absent.Why this branch merges
launchpadin firstThe chain was 72 commits behind
launchpad. Opening it directly would have reverted everything that landed meanwhile: the four observabilitycurrent-statedocs,pr_review_batch.py,test_pr_review_batch.py,adjudicator.md, and #505's privacy fix to note 315.launchpaddirectlyThe 49 remaining deletions are all in
plans/2026-08-13-issue-118-adjudication.md, which is the plan document being updated as STEPs completed.The merge was clean — zero conflicted files. I expected conflicts in
run_adjudication.pyand there were none;launchpad's 72 commits and the chain's 13 touch different things.Verification
Command run:
Raw output:
Both sides confirmed present on the result:
Not verified
just cihas not run. I ran two Python suites — 506 tests. Rust, desktop, mobile and the builds have not been exercised, and this diff does not touch them, but that is an argument rather than a measurement.A clean merge is not a correct merge. That is what #363 is about, and it is the residual risk here. I checked that both sides' files exist and that both suites pass; I did not review the 776 new lines of
run_adjudication.pyfor semantic conflicts with what landed onlaunchpadmeanwhile. Nobody has read the mergedrun_adjudication.pyend to end.There may be a fifth stranded branch.
feat/review-agent-adjudication-runis #264's base, so an earlier link may exist. I did not trace it. If its work is already onlaunchpadby another route this PR is complete; if not, a second landing is needed and this diff would not reveal it.Escalations
This lands the backlog; it does not fix the cause. The next stacked PR will merge into its parent again unless its base is set to
launchpadat open time. Worth a habit or a check, and it is not in this PR.Verify by ancestry after merging, not by the badge. The whole failure was invisible because four PRs showed green:
should exit 0 afterwards.
Land promptly. The merge commit pins this to
launchpadas of26920e5c3. Iflaunchpadmoves and this sits, it needs another merge-forward.I am not independent on the STEP content. I wrote or reviewed commits across feat(launchpad): verdict contract in code (#118 STEP 2) #261-feat(launchpad): dedupe via an injectable second judge (#118 STEP 7) #267 during this work, and I wrote this merge. This PR needs a reviewer who did neither.
Opened as a draft — no
review-finalpass has run on this branch, so there is no READY verdict to justify a non-draft PR.🤖 Drafted by Claude Code (
claude-opus-5) for @serina-mcfall.