diff --git a/launchpad/plans/2026-08-27-issue-287-verdict-block-refusal.md b/launchpad/plans/2026-08-27-issue-287-verdict-block-refusal.md new file mode 100644 index 00000000000..ac3a6964e9d --- /dev/null +++ b/launchpad/plans/2026-08-27-issue-287-verdict-block-refusal.md @@ -0,0 +1,298 @@ +Issue #287 — refuse a comment set carrying more than one verdict block +Stated size: no `Size` line on the issue — asked Serina directly → 30–60 minutes → cap: 8 steps + +ALREADY TRUE (verified against git and the live repo, not notes) + No code in `launchpad/review-agent/` parses PR comment bodies for a fenced `verdict` + block today. `grep -rn` across every `.py` file in that directory found only: + `fetch.py`'s `fetch_all()` retrieving comment surfaces for *containment* scanning + (untrusted-text detection, `contain.py`'s domain), and `verdicts.py` validating the + adjudication *stage's output document* structure — a different JSON document, not raw + comment text. There is no existing "parse a fenced block out of a GitHub comment" code + in this repository to extend. + + The ` ```verdict ` fence convention is real and already in production use. Pulled the + actual bodies of PR #261 and #264 via `gh api repos/launchpad-26/buzz/issues//comments + --paginate --slurp`: rows are tab-separated `VERDICTSEVERITYfile:line + description`. It is written down nowhere in ADJUDICATION.md, FINDINGS.md, CONTAINMENT.md, + or PUBLISHING.md — the format exists only as a convention reviewers already follow. + + The exact double-block scenario the issue describes is reproduced in the wild, not + hypothetical: + #261 — comment `5364185647` (01:45:48Z) and `5364261676` (01:58:30Z), same author + (`ciaran-slow`), 13 minutes apart. Both are full 4-row restatements; row 2's + severity moved Medium → Low between them. + #264 — comment `5364221899` (01:51:51Z) and `5364504768` (02:36:23Z), same author, + 45 minutes apart. Both are full 3-row restatements; row 1's severity moved + High → Blocker between them — the named promotion the issue cites. + In both real cases the later comment is a **complete restatement**, not a delta. No + supersedes marker exists anywhere in the two samples — this is the evidence behind the + Option-B decision below. + + A near-neighbour already exists, but outside this repository and for a different input + shape: `review-gate.sh`'s `cmd_verdict` (reached via the `serina-skills` plugin cache — + canonical home is `serina-mcfall/serina-skills`, not part of `block/buzz`; corrected + during review-plan, which found `~/.claude/skills/review-pr/review-gate.sh` doesn't + exist and the earlier `launchpad-26/skills` attribution was wrong) already refuses more + than one opening fence, distinguishes a present-and-empty block from an absent one, + requires the fence to close, and accepts rows with **4 or more** tab-separated fields + (`cut -f4-` joins field 4 onward as the description — corrected from an earlier + "4-tab rows" misreading). It operates on **one local file** (`review-gate.sh verdict + `) — a single already-adjudicated report on disk before push — not a set of + already-posted GitHub PR comments. Its multi-block check is a bare + `grep -c '^```verdict$'` with no blockquote/indent lookalike guard, because a single + local file written by one adjudicator in one pass doesn't need one. #287's input + (comments from potentially several authors, reachable by anyone who can comment on the + PR) does. + + `pr_body_check.py`'s `_strip_fences` (`launchpad/scripts/pr_body_check.py`) is this + repo's only existing CommonMark-run-length-aware fence parser, but review-plan measured + its blockquote handling directly and it does the **opposite** of what STEP 2 needs: + `_strip_fences` strips blockquote markers *before* matching, specifically so a fence + quoted with `> ` **is recognised** as real (its own docstring says so, and a live + probe confirmed `_strip_fences` consumes a `> ```verdict` line as a genuine fence). + STEP 2 needs the opposite disposition — a quoted fence must be *excluded*, not + recognised — so only `_strip_fences`'s `FENCE_OPEN`/`FENCE_CLOSE` run-length matching is + reusable; the blockquote disposition must be inverted, not copied. + + `fetch.py:140` (`fetch_all`) already knows the retrieval mechanics — `gh api --paginate + --slurp` against both `issues/{pr}/comments` and `pulls/{pr}/comments`, plus the + `UNREADABLE = ("absent", "oversized", "unparseable")` states and the + `CAP_PER_ENTRY_POINT` size cap — but its `Surface`/`_joined()` abstraction **flattens + every comment into one joined string** and discards comment id, author, and creation + time entirely. That is not reusable for #287, which needs per-comment boundaries to + resolve which block is "the last one" and to refuse a same-comment double-block. + #287 needs a new per-comment fetch that borrows `fetch.py`'s pagination and + unreadable-state handling, not its `Surface` type. + + #119 (this repo's other in-flight branch, PR #1460) touches `publish.py`, + `run_dimensions.py`, `check_publish_*.py`, the two `launchpad-review-agent-*.yml` + workflows, and — confirmed via the full `gh pr diff 1460 --name-only`, not the earlier + partial read of it — `launchpad/review-agent/ADJUDICATION.md`, `CONTAINMENT.md`, + `FINDINGS.md`, and `PUBLISHING.md` too. STEP 1 below edits `ADJUDICATION.md`, so there + **is** file overlap with #1460 on that one file. This plan is on its own branch + (`task/287-verdict-block-refusal`, off `origin/launchpad`, not stacked on #1460), so the + overlap is an ordinary same-file-two-branches situation resolved by a normal rebase at + merge time — not a logical dependency, and not a reason to wait on #1460. The earlier + claim of "no file overlap" was wrong and is corrected here rather than left standing. + + No plan file existed for #287 before this one (`launchpad/plans/` checked). + +DECISION RECORDED HERE, PER SERINA (issue asks for this explicitly; two readings were +surfaced and she chose) + Reading A: a later comment may amend an earlier block, but only via a new explicit + "supersedes" marker; anything without it is refused as ambiguous. + Reading B: no amendment concept — a reviewer must re-post the complete block, and the + parser deterministically takes the last complete, closed, well-formed block by comment + order; a second block that is anything other than that (malformed, unclosed, or not a + full row-set) is still refused. + → Serina chose B: no new marker syntax, matches both real double-block cases observed + above, and needs no reviewer-side change. + +STEP 1 Record the Option-B decision in ADJUDICATION.md. [independent] + Cite #261/#264 as the evidence. + done when: ADJUDICATION.md gains a section stating the rule from the DECISION + block above verbatim in substance, naming comment ids `5364185647`/`5364261676` + (#261) and `5364221899`/`5364504768`(#264) as the production evidence it rests on. + +STEP 2 Build the fenced-block locator. [independent] + Given one comment body's raw text, return every top-level ` ```verdict ` fence + with its start/end line, closed/unclosed state, and raw row text. Reuse + `pr_body_check.py`'s `FENCE_OPEN`/`FENCE_CLOSE` run-length-matching regexes for + the fence boundary itself, but invert its blockquote disposition: `_strip_fences` + strips `> ` before matching so a quoted fence *counts* as real (correct for its + own job — hiding quoted code from prose scanning); this locator's job is the + opposite, so a line matching `BLOCKQUOTE` must *disqualify* that fence rather than + have its marker stripped first. Also capture the info string (`_strip_fences` + only captures the backtick run, not what follows it) so ` ```verdict ` can be + distinguished from an unrelated fence, and report closed/unclosed explicitly + (`_strip_fences` has no such state — an unterminated fence there just runs to EOF). + done when: a control suite proves, on synthetic bodies: zero blocks → empty list; + one closed block → one entry; one unclosed block → flagged unclosed, not silently + dropped or silently treated as empty; a `> ```verdict` blockquoted fence → not + matched as a top-level block; a 4-space-indented ` ```verdict ` → not matched. + +STEP 3 Build the row parser. [needs 2] + Given one located block's raw row text, parse each line into `{verdict, severity, + location, description}`. A row needs **4 or more** tab-separated fields — not + exactly 4 — with fields 4 onward rejoined as `description` (mirrors + `review-gate.sh`'s `cut -f4-`; a description containing a literal tab is legal on + the emitter side and must not be misread as malformed). Validate `verdict` is one + of `verdicts.VERDICTS` (imported, not re-declared) and `severity` is one of + `review.SEVERITY_ORDER` (imported the same way `verdicts.py:25` does) — marking a + malformed row distinctly from a merely-empty block, not silently dropping or + coercing it. + done when: a control proves a well-formed 4-field row parses to the four named + fields; a <4-field row is flagged malformed with the row's own text in the + message; a 5-field row (tab inside the description) parses with fields 4–5 joined, + not flagged malformed; a row whose first field isn't in `verdicts.VERDICTS`, or + whose second field isn't in `review.SEVERITY_ORDER`, is flagged malformed rather + than silently accepted. + +STEP 4 Fetch one PR's full comment set and locate every block. [needs 2] ← RUNS HERE + `fetch.py`'s `Surface`/`_joined()` flattens every comment into one string and + discards comment id/author/time — not reusable here. Build a new per-comment + fetch against both `issues/{pr}/comments` and `pulls/{pr}/comments` + (`gh api --paginate --slurp`, same incantation `fetch.py` uses), keeping each + comment's `id`, `created_at`, and `user` intact, and reusing `fetch.py`'s + `UNREADABLE = ("absent", "oversized", "unparseable")` state model and + `CAP_PER_ENTRY_POINT` so a failed or oversized fetch is a distinguishable state, + not silently empty. Run STEP 2's locator over each comment's own, un-joined body, + each result tagged with its source comment id, surface (`issue` vs `review`), + `created_at`, and position within that comment. + done when: run against the real PRs #261 and #264 over the live API, output shows + exactly the two blocks found in each, tagged with comment ids matching + `5364185647`/`5364261676` (#261) and `5364221899`/`5364504768` (#264) — the first + point this plan produces something demonstrable against real data rather than + synthetic fixtures. A forced-unreadable run (e.g. an invalid PR number, or + `fetch.py`'s own `--degrade` pattern applied to this fetch) reports a distinct + `unreadable` state rather than "zero comments". + +STEP 5 Implement the Option-B resolution rule. [needs 1, 3, 4] + Over STEP 4's tagged, STEP 3-parsed blocks for one PR, in this order — order + matters, since two of these cases can match the same input and the first match + must win: + the comment fetch itself was unreadable (STEP 4's `absent`/`oversized`/ + `unparseable` states) → refuse as `unreadable`, + distinct from "none found"; never render the same + as a clean zero-block PR (mirrors CONTAINMENT.md's + "absence of evidence is never reported as evidence") + two+ blocks **within the same comment** → always refuse, regardless of + whether every block in it is individually closed + and well-formed. Two fences posted in one write + can't be a temporal amendment of each other, so + Option B's ordering rule never applies to this + shape — refuse and name both positions. This branch + must be checked **before** the accept-last branch + below, since a same-comment pair that is also + well-formed would otherwise match both + any other malformed case (a malformed row anywhere, or an unclosed block, in + any comment) → refuse, naming every block's + comment id, surface, and position — not only the + offending one + a well-formed, closed block found on the **review** (inline code-comment) + surface → refuse outright, naming the + whole evaluated set (not only the review-surface + block) — added post-review-final (see ADDENDUM + below): resolves this section's own earlier + "merged across both surfaces" wording and the OPEN + item on review-comment scope, in the direction of + never letting a partial inline annotation silently + outrank or silently supersede a real issue-comment + block + zero blocks → a distinguishable "none found" + result, not an error + one closed, well-formed block → accept it + two+ blocks, all closed and fully well-formed, in **different** comments, all + on the **issue-comment surface** (the review branch above already removed any + review-surface block from contention) → accept the block with the + highest `(created_at, comment_id)` pair — + `created_at` is only second-resolution, so + `comment_id` (monotonically increasing on GitHub) + is the deciding tie-break, not "position within a + comment" (that tie-break only ever applied to the + always-refused same-comment case, so it's dropped + rather than kept as dead code). Report every + earlier one as superseded, naming its comment id, + surface, and position + +ADDENDUM (post-build, after two review-final passes): this section originally said the +accept branch's ordering was "merged across both surfaces". Review-final's second pass +(finding 1, High) correctly caught that the shipped code instead refuses any well-formed +review-surface block outright — a stricter, evidence-backed answer to the OPEN item below, +not an oversight — and that this section hadn't been updated to say so. Fixed here rather +than left contradicting the code: only the issue-comment surface can supply an +authoritative block; a well-formed block on the review surface always refuses. See +ADJUDICATION.md's #287 section for the same decision recorded where a consumer would +actually read it. + done when: run against #261's real comment set → resolves to `5364261676`'s block, + reports `5364185647` as superseded; against #264 → resolves to `5364504768`'s + block (the Blocker promotion), reports `5364221899` as superseded; a synthetic + case with two **well-formed, closed** blocks in one comment → refused (not + accepted via the last-wins branch), naming both positions; a synthetic case with + one malformed row in the second of two otherwise-clean blocks in different + comments → refused, naming both locations; STEP 4's `unreadable` state → refused + as `unreadable`, distinguishable in the return value from the "none found" case. + +STEP 6 Add the issue's five named control shapes, plus STEP 5's sixth. [needs 5] + As their own automated suite: zero blocks, one block, two **well-formed, closed** + blocks in one comment (not a malformed pair — that would pass through the + malformed-catch-all instead of proving the same-comment branch specifically), two + blocks across two comments, a fenced block inside a quoted-or-indented context + that only looks like one, and an unreadable/absent comment fetch — using #261 and + #264 as real fixtures (recorded, not re-fetched live in the suite) plus synthetic + cases for the rest. + done when: the control script reports PASS on all six shapes; the #261/#264 + fixtures are recorded under this project's existing convention — `fixtures/ + adjudication/PROVENANCE.md` plus `fixtures/adjudication/generate.py` and + `fixtures/adjudication/recordings/FALSIFIABILITY.md` (not `testdata/README.md`, + which doesn't exist under `launchpad/review-agent/` — that convention belongs to + `launchpad/scripts/testdata/`) — so a future re-record can refresh them from the + live PRs. + +STEP 7 Expose one clear entry point documenting the consumer contract. [needs 5] + E.g. a `resolve_verdict(pr, ...) -> Resolution`-shaped function, with a docstring + stating the DoD's consumer requirement as a contract for whoever builds it next — + #119's banner path does not currently read PR comments at all (confirmed: its + scope is publishing a review it composes itself, not consuming other reviewers' + comments), and STEP 11 is unrelated (`check_step11.py` is the CI-workflow-trigger + control, not a comment consumer). There is no live consumer to migrate today, + which the issue itself states ("fixing this now is cheap: there is no consumer yet + to migrate") — so this step documents the contract rather than rewiring code that + doesn't exist. + done when: the module exports one importable entry point, its docstring names + the two candidate future callers (#119's banner path, #426's pre-review packet) + and states neither currently calls it, and a control asserts the function's + signature/return shape stays stable (a smoke-level regression guard, not a + behavioural one). + +PARALLEL STEP 1 (docs) and STEP 2 (locator) touch disjoint files and can run as parallel + subagents. STEP 3 (row parser) and STEP 4 (fetch + locate-across-comments) both need + STEP 2's output but can be written as separate functions/files and run in parallel with + each other; they only need to agree on STEP 2's return shape, not on each other's code. + STEPs 5, 6, 7 are strictly sequential from STEP 5 onward — 6 and 7 both read the whole + resolver STEP 5 produces, and 7's docstring should reflect what 6's fixtures actually + proved. Nothing here is dispatched by this plan; the decision to fan out belongs to + whoever executes it. + +GATES No automated verify gate fires on its own in this checkout (per the sibling #118 + plan's own note, still true — `.claude/settings.json` / `.claude/settings.local.json` + are both absent). Gates below are manual invocations before push, per + `run-reviewers-before-pushing-not-after`. + serina:review-code applies after STEPs 2–5 land (the parser and resolver). + serina:review-tests applies after STEP 6 (the control suite) — check specifically for + the five named shapes actually being distinct tests, not one test asserting all five. + serina:review-plan ran once against this file (independent dispatch, Opus, this + plan's own author excluded per the skill's requirement) and found 1 Blocker, 3 High, + 4 Medium, 2 Low — all applied to this revision. Verified against the live API and real + code rather than assumed; see the STEP/ALREADY-TRUE text above for what changed. + qa explore mode applies lightly: STEP 4 already is the "exercise the real interface" + step (run against live PRs #261/#264), so a separate qa pass mainly needs to try + additional real PRs beyond the two already used as fixtures, to catch a comment shape + neither #261 nor #264 happened to exhibit. + +BUDGET STEP 5 is the step most likely to eat the budget, more so after review-plan's + findings — it now has six ordered branches (unreadable-fetch / same-comment-refuse / + malformed-anywhere / zero / one / clean-multi-in-different-comments) where branch order + is itself load-bearing, not just branch content, and three of the plan's five real-PR- + or-forced-state assertions depend on getting it right. + +OPEN Not for a builder to decide silently: + RESOLVED (post-build, see STEP 5's ADDENDUM): whether GitHub *review-line* comments + (`pulls/{pr}/comments`, inline code comments) are in scope. Both surfaces are fetched + (STEP 4), but only the issue-comment surface can supply an authoritative block — a + well-formed block found on the review surface always refuses, never silently accepted, + never silently merged into the ordering, never silently dropped. Both real double-block + examples (#261, #264) only ever used the issue-comment surface, which is the evidence + this rests on. + Whether an actual consumer (#119's banner path or #426's pre-review packet) gets wired + to call STEP 7's entry point, and when — deliberately left to whichever of those issues + builds next, per the issue's own "no consumer yet to migrate." + +LEFT OUT Deliberately excluded, per the issue's own Out of scope section: + Changing how any individual reviewer's harness composes its output — this plan is only + about the consuming side. + Adjudicating whether the specific 2026-08-21 double-block reports were themselves + correct — they were; the defect is the format not distinguishing amendment from + duplication, which this plan fixes going forward, not retroactively. + A supersedes-marker syntax (Reading A) — decided against; see DECISION RECORDED above. + Rewiring any real consumer — none exists yet (see OPEN). diff --git a/launchpad/review-agent/ADJUDICATION.md b/launchpad/review-agent/ADJUDICATION.md index e0e79f24d64..323c34a1f60 100644 --- a/launchpad/review-agent/ADJUDICATION.md +++ b/launchpad/review-agent/ADJUDICATION.md @@ -320,3 +320,70 @@ requires). The pass-through above honours the *intent* of the table's row, but t table's own literal wording is not corrected by this document — that edit is left for whoever owns #120, since `CONTAINMENT.md` is a cross-cutting contract this document does not have unilateral authority to amend. + +## PR comment verdict blocks: refusing more than one (#287) + +A ` ```verdict ` fence posted in a PR comment (the row shape `review-gate.sh`'s +`cmd_verdict` already validates for a single local file — see that script for the row +grammar) is not unique to one comment on a PR. Reviewers re-post a corrected block after +noticing a mistake, and nothing before #287 distinguished a deliberate correction from +two independent, disagreeing verdicts left standing at once. + +**The rule: no amendment marker. The parser deterministically takes the last complete, +closed, well-formed block by comment order (highest `(created_at, comment_id)`), and +refuses anything that does not reduce to exactly one candidate that way** — a malformed +row, an unclosed block, or two-or-more blocks inside the *same* comment are all refused +outright, never resolved by picking one. This was Option B of two readings put to +Serina: Option A would have required a new explicit "supersedes" marker before a later +block could override an earlier one; Option A was not chosen. + +**Why B, not A — the evidence, not a preference.** Two real double-block PR threads were +pulled via `gh api repos/launchpad-26/buzz/issues//comments --paginate --slurp` and +read in full before this decision was recorded, not assumed: + +- **PR #261** — comment `5364185647` (2026-08-21T01:45:48Z) and comment `5364261676` + (2026-08-21T01:58:30Z), same author, 13 minutes apart. Both are full 4-row + restatements of the same finding set; the second comment's row 2 severity moved + Medium → Low from the first. +- **PR #264** — comment `5364221899` (2026-08-21T01:51:51Z) and comment `5364504768` + (2026-08-21T02:36:23Z), same author, 45 minutes apart. Both are full 3-row + restatements; the second comment's row 1 severity moved High → Blocker from the + first — the named promotion #287 cites. + +In neither real case does the later comment carry any marker referencing the earlier +one — no "supersedes", no "correction to comment ``", nothing machine-parseable. Both +are simply a complete re-post of the whole block, later in the comment stream. Requiring +a marker (Option A) would have made both of these real, already-happened corrections +retroactively unparseable, and would have needed a reviewer-side convention change no +reviewer today follows. Taking the later complete block by comment order (Option B) +resolves both cases exactly as the reviewer who wrote them intended, with no new syntax +to adopt. + +**Reconciling #287's own done-when bullets 1 and 3.** Bullet 1 says the parser "refuses +rather than guesses when it finds more than one block"; bullet 3 says it accepts the +last one across different comments, when every candidate is well-formed. These are not +in tension once the shapes are told apart: **more than one block always refuses when the +set is not reducible to a single well-formed candidate** — two-or-more blocks inside the +*same* comment, or any malformed row or unclosed block anywhere — and **only** resolves +deterministically by comment order, rather than refusing, when every candidate is +individually closed and well-formed and each lives in its own, different (issue-surface) +comment. "More than one block" is a guess only when the parser would have to pick +between genuinely ambiguous candidates; ordering a set of unambiguous, well-formed +restatements is not a guess, it is applying the recorded Option-B rule. + +**Scope decision: which surface can be authoritative (resolves this plan's OPEN item).** +Both the issue-comment surface and the review (inline code-comment) surface are fetched +and scanned for a ` ```verdict ` fence — narrowing detection to one surface would be +weaker than what #287 asks for, and `fetch.py` already distinguishes the two. But only +the **issue-comment surface** may supply the authoritative block. A well-formed, closed +` ```verdict ` fence found on the review surface is refused outright, never accepted and +never silently dropped: an inline code comment is normally a narrow, line-scoped +annotation, not a full review restatement, and nothing establishes that a reviewer ever +means one to stand as the authoritative verdict for the whole PR. Concretely, without +this rule, a reviewer's real, complete issue-comment block could be silently outranked by +an unrelated later inline annotation that happens to carry a well-formed one-row fence — +exactly the "pick the last one and silently drop everything the first block carried" +failure #287 exists to prevent, just relocated to a surface boundary instead of a +same-surface timestamp. Both real production examples (#261, #264) only ever used the +issue-comment surface, which is the entire evidentiary basis Option B itself rests on — +there is no comparable evidence a review-surface block was ever meant to be authoritative. diff --git a/launchpad/review-agent/check_pr_comments.py b/launchpad/review-agent/check_pr_comments.py new file mode 100644 index 00000000000..3af8e2f0193 --- /dev/null +++ b/launchpad/review-agent/check_pr_comments.py @@ -0,0 +1,123 @@ +"""#287 STEP 4 control: fetch + locate against the real, live GitHub API. + +Needs network (`gh api` against launchpad-26/buzz). This is the plan's own +`RUNS HERE` step: the first control in this plan that proves something +against real data, not synthetic fixtures. +""" + +from __future__ import annotations + +import sys + +from pr_comments import degrade, fetch_and_locate + +REPO = "launchpad-26/buzz" + +FAILURES: list[str] = [] + + +def check(label: str, condition: bool, detail: str = "") -> None: + if condition: + print(f"PASS {label}") + else: + FAILURES.append(label) + print(f"FAIL {label} {detail}") + + +def _check_pr(pr: int, expected_issue_ids: list[int]) -> None: + """Asserts on BOTH surfaces `fetch_and_locate` returns -- not only "issue". + Without checking "review" too, this control never actually proves PR + #261/#264 have EXACTLY the two known blocks: an unnoticed extra block on + the review-comment surface would sail through silently.""" + results = fetch_and_locate(pr, REPO) + + issue = results["issue"] + check(f"PR #{pr} issue surface is readable", issue.readable, f"state={issue.state} reason={issue.reason}") + ids = sorted(tb.comment_id for tb in issue.blocks) + check( + f"PR #{pr}: exactly the two known blocks on the issue surface, right comment ids", + ids == sorted(expected_issue_ids), + f"got {ids}", + ) + + review = results["review"] + check(f"PR #{pr} review (inline-comment) surface is readable", review.readable, f"state={review.state} reason={review.reason}") + check( + f"PR #{pr} review surface carries zero ```verdict blocks", + review.blocks == [], + f"got {review.blocks!r}", + ) + + +def test_pr_261() -> None: + _check_pr(261, [5364185647, 5364261676]) + + +def test_pr_264() -> None: + _check_pr(264, [5364221899, 5364504768]) + + +def test_invalid_pr_is_unreadable() -> None: + results = fetch_and_locate(999999999, REPO) + cf = results["issue"] + check( + "an invalid PR number reports an unreadable state, not zero comments", + not cf.readable and cf.state == "absent", + f"got state={cf.state} readable={cf.readable} blocks={cf.blocks}", + ) + + +def test_degrade_forces_a_distinguishable_unreadable_state() -> None: + """`degrade()` (STEP 4's stated alternate way to force "forced-unreadable", + alongside the invalid-PR-number path above) actually gets called from + somewhere -- this control, and the CLI's own `--degrade` flag.""" + results = fetch_and_locate(261, REPO) + degraded = degrade(results, "review=oversized") + check( + "degrade() forces the named surface into the requested state", + degraded["review"].state == "oversized" and not degraded["review"].readable, + f"got {degraded['review']}", + ) + check( + "degrade() leaves the other surface untouched", + degraded["issue"] is results["issue"], + ) + check( + "degrade()'s reason names the forcing spec", + "--degrade review=oversized" in degraded["review"].reason, + degraded["review"].reason, + ) + + +def test_degrade_accepts_fetch_pys_longer_surface_names() -> None: + """review-final MEDIUM #5: `fetch.py` (same directory, same two GitHub + endpoints) calls these surfaces `pr_issue_comments`/`pr_review_comments` + -- the vocabulary `contain.py`/`run_dimensions.py`'s own `--degrade` + flags already use. Both spellings must reach the identical state, not + just avoid crashing.""" + results = fetch_and_locate(261, REPO) + via_alias = degrade(results, "pr_review_comments=absent") + via_short = degrade(results, "review=absent") + check( + "the fetch.py-style alias forces the same state as the short name", + via_alias["review"].state == via_short["review"].state == "absent", + f"alias={via_alias['review'].state} short={via_short['review'].state}", + ) + + +def main() -> int: + test_pr_261() + test_pr_264() + test_invalid_pr_is_unreadable() + test_degrade_forces_a_distinguishable_unreadable_state() + test_degrade_accepts_fetch_pys_longer_surface_names() + + if FAILURES: + print(f"\n{len(FAILURES)} failed: {FAILURES}") + return 1 + print("\nall STEP 4 live control shapes passed") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/launchpad/review-agent/check_resolve_verdict_contract.py b/launchpad/review-agent/check_resolve_verdict_contract.py new file mode 100644 index 00000000000..29cb290551a --- /dev/null +++ b/launchpad/review-agent/check_resolve_verdict_contract.py @@ -0,0 +1,101 @@ +"""#287 STEP 7 control: `resolve_verdict`'s signature and `Resolution`'s shape. + +Smoke-level, not behavioural, per STEP 7's own `done when`: this does not +assert what any particular PR resolves to -- STEP 5's `check_verdict_ +resolution.py` and STEP 6's `test_verdict_resolution.py` already do that. +This asserts the ONE importable entry point and its return type's shape do +not silently drift out from under a future consumer that has not been +written yet (#119's banner path or #426's pre-review packet -- see +`resolve_verdict`'s own docstring). No network needed. +""" + +from __future__ import annotations + +import dataclasses +import inspect +import sys + +import verdict_resolution as vr + +FAILURES: list[str] = [] + + +def check(label: str, condition: bool, detail: str = "") -> None: + if condition: + print(f"PASS {label}") + else: + FAILURES.append(label) + print(f"FAIL {label} {detail}") + + +def test_entry_point_exists_and_is_callable() -> None: + check("verdict_resolution exports resolve_verdict", hasattr(vr, "resolve_verdict")) + check("resolve_verdict is callable", callable(getattr(vr, "resolve_verdict", None))) + + +def test_entry_point_docstring_names_both_candidate_callers() -> None: + doc = vr.resolve_verdict.__doc__ or "" + check("docstring names #119's banner path", "#119" in doc) + check("docstring names #426's pre-review packet", "#426" in doc) + check( + "docstring states neither currently calls it", + "neither calls this today" in doc or "no consumer yet" in doc, + doc, + ) + + +def test_entry_point_signature_stable() -> None: + sig = inspect.signature(vr.resolve_verdict) + names = list(sig.parameters.keys()) + check("resolve_verdict(pr, repo=...) -- exactly these two parameters", names == ["pr", "repo"], f"got {names}") + if "repo" in sig.parameters: + check( + "repo has a default value", + sig.parameters["repo"].default is not inspect.Parameter.empty, + ) + + +def test_resolution_shape_stable() -> None: + fields = {f.name for f in dataclasses.fields(vr.Resolution)} + expected = {"outcome", "reason", "accepted", "superseded", "refused_locations"} + check("Resolution carries exactly the documented fields", fields == expected, f"got {fields}") + + +def test_resolved_block_shape_stable() -> None: + fields = {f.name for f in dataclasses.fields(vr.ResolvedBlock)} + expected = {"location", "rows"} + check("ResolvedBlock carries exactly the documented fields", fields == expected, f"got {fields}") + + +def test_block_location_shape_stable() -> None: + fields = {f.name for f in dataclasses.fields(vr.BlockLocation)} + expected = {"comment_id", "surface", "position", "created_at", "reason"} + check("BlockLocation carries exactly the documented fields", fields == expected, f"got {fields}") + + +def test_outcomes_constant_stable() -> None: + check( + "OUTCOMES lists exactly the four documented outcomes", + set(vr.OUTCOMES) == {"unreadable", "refused", "none_found", "accepted"}, + f"got {vr.OUTCOMES}", + ) + + +def main() -> int: + test_entry_point_exists_and_is_callable() + test_entry_point_docstring_names_both_candidate_callers() + test_entry_point_signature_stable() + test_resolution_shape_stable() + test_resolved_block_shape_stable() + test_block_location_shape_stable() + test_outcomes_constant_stable() + + if FAILURES: + print(f"\n{len(FAILURES)} failed: {FAILURES}") + return 1 + print("\nall STEP 7 contract checks passed") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/launchpad/review-agent/check_verdict_blocks.py b/launchpad/review-agent/check_verdict_blocks.py new file mode 100644 index 00000000000..95ad0edb537 --- /dev/null +++ b/launchpad/review-agent/check_verdict_blocks.py @@ -0,0 +1,109 @@ +"""#287 STEP 2 control: the fenced-block locator, on synthetic comment bodies. + +No network. Every case in STEP 2's `done when` is its own function so a failure +names exactly which shape broke, not "the suite failed". +""" + +from __future__ import annotations + +import sys + +from verdict_blocks import locate_verdict_blocks + +FAILURES: list[str] = [] + + +def check(label: str, condition: bool, detail: str = "") -> None: + if condition: + print(f"PASS {label}") + else: + FAILURES.append(label) + print(f"FAIL {label} {detail}") + + +def test_zero_blocks() -> None: + body = "Just a normal comment with no fences at all.\n\nSecond paragraph." + blocks = locate_verdict_blocks(body) + check("zero blocks -> empty list", blocks == [], f"got {blocks!r}") + + +def test_one_closed_block() -> None: + body = "Some prose.\n\n```verdict\nCONFIRMED\tHigh\tfoo.py:1\tsomething wrong\n```\n\nMore prose." + blocks = locate_verdict_blocks(body) + check("one closed block -> one entry", len(blocks) == 1, f"got {len(blocks)}: {blocks!r}") + if blocks: + b = blocks[0] + check("closed block reports closed=True", b.closed is True) + check("closed block has an end_line", b.end_line is not None) + check( + "closed block captures its row text", + b.raw_rows == "CONFIRMED\tHigh\tfoo.py:1\tsomething wrong", + f"got {b.raw_rows!r}", + ) + + +def test_one_unclosed_block() -> None: + body = "Prose before.\n\n```verdict\nCONFIRMED\tHigh\tfoo.py:1\tsomething wrong\n" + blocks = locate_verdict_blocks(body) + check( + "unclosed block is flagged, not dropped or empty", + len(blocks) == 1 and blocks[0].closed is False and blocks[0].end_line is None, + f"got {blocks!r}", + ) + if blocks: + check( + "unclosed block still carries its row text", + blocks[0].raw_rows == "CONFIRMED\tHigh\tfoo.py:1\tsomething wrong", + f"got {blocks[0].raw_rows!r}", + ) + + +def test_blockquoted_fence_not_matched() -> None: + body = ( + "Quoting an earlier reviewer:\n\n" + "> ```verdict\n" + "> CONFIRMED\tHigh\tfoo.py:1\tsomething wrong\n" + "> ```\n\n" + "My own take follows in prose only." + ) + blocks = locate_verdict_blocks(body) + check( + "blockquoted ```verdict fence is not matched as top-level", + blocks == [], + f"got {blocks!r}", + ) + + +def test_indented_fence_not_matched() -> None: + body = "Prose.\n\n ```verdict\n CONFIRMED\tHigh\tfoo.py:1\tsomething wrong\n ```\n" + blocks = locate_verdict_blocks(body) + check( + "4-space-indented ```verdict fence is not matched", + blocks == [], + f"got {blocks!r}", + ) + + +def test_unrelated_fence_not_matched() -> None: + body = "```python\nprint('not a verdict block')\n```" + blocks = locate_verdict_blocks(body) + check("unrelated fenced code is not matched as a verdict block", blocks == [], f"got {blocks!r}") + + +def main() -> int: + test_zero_blocks() + test_one_closed_block() + test_one_unclosed_block() + test_blockquoted_fence_not_matched() + test_indented_fence_not_matched() + test_unrelated_fence_not_matched() + + if FAILURES: + print(f"\n{len(FAILURES)} failed: {FAILURES}") + return 1 + print("\nall STEP 2 control shapes passed") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/launchpad/review-agent/check_verdict_resolution.py b/launchpad/review-agent/check_verdict_resolution.py new file mode 100644 index 00000000000..e6dc5f874af --- /dev/null +++ b/launchpad/review-agent/check_verdict_resolution.py @@ -0,0 +1,135 @@ +"""#287 STEP 5 control: the Option-B resolution rule. + +Needs network for the #261/#264 live assertions; the remaining cases are +synthetic and construct `pr_comments.CommentFetch`/`TaggedBlock` directly so +they do not depend on the live API at all. +""" + +from __future__ import annotations + +import sys + +from pr_comments import CommentFetch, TaggedBlock +from pr_comments import fetch_and_locate as live_fetch_and_locate +from verdict_blocks import LocatedBlock +from verdict_resolution import resolve + +FAILURES: list[str] = [] + + +def check(label: str, condition: bool, detail: str = "") -> None: + if condition: + print(f"PASS {label}") + else: + FAILURES.append(label) + print(f"FAIL {label} {detail}") + + +def test_pr_261_live() -> None: + results = live_fetch_and_locate(261, "launchpad-26/buzz") + resolution = resolve(results) + check("PR #261 resolves to accepted", resolution.outcome == "accepted", resolution.reason) + if resolution.accepted: + check( + "PR #261 accepts comment 5364261676's block", + resolution.accepted.location.comment_id == 5364261676, + f"got {resolution.accepted.location}", + ) + check( + "PR #261 reports comment 5364185647 as superseded", + any(loc.comment_id == 5364185647 for loc in resolution.superseded), + f"got {resolution.superseded}", + ) + + +def test_pr_264_live() -> None: + results = live_fetch_and_locate(264, "launchpad-26/buzz") + resolution = resolve(results) + check("PR #264 resolves to accepted", resolution.outcome == "accepted", resolution.reason) + if resolution.accepted: + check( + "PR #264 accepts comment 5364504768's block (the Blocker promotion)", + resolution.accepted.location.comment_id == 5364504768, + f"got {resolution.accepted.location}", + ) + check( + "PR #264 reports comment 5364221899 as superseded", + any(loc.comment_id == 5364221899 for loc in resolution.superseded), + f"got {resolution.superseded}", + ) + + +def _well_formed_block(text: str) -> LocatedBlock: + return LocatedBlock(start_line=1, end_line=3, closed=True, raw_rows=text) + + +def _malformed_block() -> LocatedBlock: + return LocatedBlock(start_line=1, end_line=3, closed=True, raw_rows="not enough fields") + + +ROW = "CONFIRMED\tHigh\tfoo.py:1\tsomething" + + +def test_two_well_formed_blocks_same_comment_refused() -> None: + tb1 = TaggedBlock(_well_formed_block(ROW), comment_id=1, surface="issue", created_at="t1", position=0) + tb2 = TaggedBlock(_well_formed_block(ROW), comment_id=1, surface="issue", created_at="t1", position=1) + results = {"issue": CommentFetch(state="ok", blocks=[tb1, tb2]), "review": CommentFetch(state="ok")} + resolution = resolve(results) + check( + "two well-formed closed blocks in ONE comment -> refused, not accepted", + resolution.outcome == "refused", + f"got {resolution.outcome} / {resolution.reason}", + ) + check( + "refusal names both positions", + {loc.position for loc in resolution.refused_locations} == {0, 1}, + f"got {resolution.refused_locations}", + ) + + +def test_malformed_in_second_of_two_different_comments_refused() -> None: + tb1 = TaggedBlock(_well_formed_block(ROW), comment_id=1, surface="issue", created_at="t1", position=0) + tb2 = TaggedBlock(_malformed_block(), comment_id=2, surface="issue", created_at="t2", position=0) + results = {"issue": CommentFetch(state="ok", blocks=[tb1, tb2]), "review": CommentFetch(state="ok")} + resolution = resolve(results) + check( + "malformed row in second of two different-comment blocks -> refused", + resolution.outcome == "refused", + f"got {resolution.outcome} / {resolution.reason}", + ) + check( + "refusal names both locations", + {loc.comment_id for loc in resolution.refused_locations} == {1, 2}, + f"got {resolution.refused_locations}", + ) + + +def test_unreadable_fetch_refused_distinct_from_none_found() -> None: + results = { + "issue": CommentFetch(state="absent", reason="gh timed out"), + "review": CommentFetch(state="ok"), + } + resolution = resolve(results) + check( + "an unreadable comment fetch resolves to 'unreadable', not 'none_found'", + resolution.outcome == "unreadable", + f"got {resolution.outcome}", + ) + + +def main() -> int: + test_pr_261_live() + test_pr_264_live() + test_two_well_formed_blocks_same_comment_refused() + test_malformed_in_second_of_two_different_comments_refused() + test_unreadable_fetch_refused_distinct_from_none_found() + + if FAILURES: + print(f"\n{len(FAILURES)} failed: {FAILURES}") + return 1 + print("\nall STEP 5 control shapes passed") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/launchpad/review-agent/check_verdict_rows.py b/launchpad/review-agent/check_verdict_rows.py new file mode 100644 index 00000000000..944ea902473 --- /dev/null +++ b/launchpad/review-agent/check_verdict_rows.py @@ -0,0 +1,91 @@ +"""#287 STEP 3 control: the row parser, on synthetic row text. + +No network. Every case in STEP 3's `done when` is its own function. +""" + +from __future__ import annotations + +import sys + +from verdict_blocks import MalformedRow, ParsedRow, parse_rows + +FAILURES: list[str] = [] + + +def check(label: str, condition: bool, detail: str = "") -> None: + if condition: + print(f"PASS {label}") + else: + FAILURES.append(label) + print(f"FAIL {label} {detail}") + + +def test_well_formed_row() -> None: + rows = parse_rows("CONFIRMED\tHigh\tfoo.py:12\tsomething is wrong here") + check("one well-formed row parses to one result", len(rows) == 1, f"got {rows!r}") + if rows: + r = rows[0] + check("result is a ParsedRow, not malformed", isinstance(r, ParsedRow), f"got {r!r}") + if isinstance(r, ParsedRow): + check( + "all four fields parse correctly", + (r.verdict, r.severity, r.location, r.description) + == ("CONFIRMED", "High", "foo.py:12", "something is wrong here"), + f"got {r!r}", + ) + + +def test_short_row_is_malformed() -> None: + raw = "CONFIRMED\tHigh\tfoo.py:12" + rows = parse_rows(raw) + check("a <4-field row produces one result", len(rows) == 1, f"got {rows!r}") + if rows: + r = rows[0] + check("short row is flagged malformed", isinstance(r, MalformedRow), f"got {r!r}") + if isinstance(r, MalformedRow): + check("malformed message carries the row's own text", raw in r.reason, r.reason) + + +def test_five_field_row_joins_description() -> None: + raw = "CONFIRMED\tHigh\tfoo.py:12\tfirst half\tsecond half after an inner tab" + rows = parse_rows(raw) + check("a 5-field row produces one result", len(rows) == 1, f"got {rows!r}") + if rows: + r = rows[0] + check("5-field row is NOT malformed", isinstance(r, ParsedRow), f"got {r!r}") + if isinstance(r, ParsedRow): + check( + "fields 4-5 are joined with a tab into description", + r.description == "first half\tsecond half after an inner tab", + f"got {r.description!r}", + ) + + +def test_unknown_verdict_is_malformed() -> None: + raw = "MAYBE\tHigh\tfoo.py:12\tsomething" + rows = parse_rows(raw) + check("unknown verdict value flagged malformed", isinstance(rows[0], MalformedRow), f"got {rows!r}") + + +def test_unknown_severity_is_malformed() -> None: + raw = "CONFIRMED\tCatastrophic\tfoo.py:12\tsomething" + rows = parse_rows(raw) + check("unknown severity value flagged malformed", isinstance(rows[0], MalformedRow), f"got {rows!r}") + + +def main() -> int: + test_well_formed_row() + test_short_row_is_malformed() + test_five_field_row_joins_description() + test_unknown_verdict_is_malformed() + test_unknown_severity_is_malformed() + + if FAILURES: + print(f"\n{len(FAILURES)} failed: {FAILURES}") + return 1 + print("\nall STEP 3 control shapes passed") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/launchpad/review-agent/fixtures/verdict_blocks/PROVENANCE.md b/launchpad/review-agent/fixtures/verdict_blocks/PROVENANCE.md new file mode 100644 index 00000000000..1f46e4ce8ce --- /dev/null +++ b/launchpad/review-agent/fixtures/verdict_blocks/PROVENANCE.md @@ -0,0 +1,49 @@ +# PROVENANCE — what is real in this directory + +This directory holds #287 STEP 6's fixtures: the real double-block comment +sets from PR #261 and PR #264, named as the production evidence behind the +Option-B decision recorded in `ADJUDICATION.md`'s "PR comment verdict +blocks: refusing more than one (#287)" section. + +## What is here, and where it came from + +`recordings/pr-261-comments.json` and `recordings/pr-264-comments.json` each +hold exactly the two comments ADJUDICATION.md names for that PR — `id`, +`created_at`, `user.login`, and the full raw `body`, unmodified from what +`gh api repos/launchpad-26/buzz/issues//comments --paginate --slurp` +returned. No finding text, row, or severity in either file was typed by +hand: both are `gh api` output, filtered down to the two named comment ids +and re-serialised with `json.dumps(..., indent=2)`. + +| file | PR | comment ids kept | +|---|---|---| +| `recordings/pr-261-comments.json` | #261 | `5364185647`, `5364261676` | +| `recordings/pr-264-comments.json` | #264 | `5364221899`, `5364504768` | + +## How the test suite uses them + +`test_verdict_resolution.py` loads these two files directly (no network) and +runs them through `pr_comments.from_items` — the same per-comment tagging +`pr_comments.fetch_and_locate` performs on a live fetch, just without the +`gh api` call — followed by `verdict_resolution.resolve`. The real +`verdict_blocks.locate_verdict_blocks` locator and `verdict_resolution.resolve` +resolver run unmodified against this recorded, real input; nothing about the +resolution logic is faked for the test. + +## Regeneration + +`python3 generate.py` from this directory re-fetches both PRs live and +reproduces the committed files. It failed the first time this was run only +in the sense that it succeeded identically — `git diff` against the +hand-fetched originals this file's history started from was empty, which is +what makes "wrote pr-261-comments.json (18843 bytes)" a check that the +committed bytes are still exactly what the live API returns, not merely a +claim. + +Unlike `fixtures/adjudication/`'s generator, this one has no nonce to pin — +these are raw comment bodies, not a document built from a seeded pipeline — +so reproducibility here means the *filtered comment set* is stable, not that +every byte is deterministic across different points in time: if either +comment were ever edited on GitHub, a re-run would pick up the edit. That +has not happened as of this recording (2026-08-27); if it ever does, a +re-run and re-commit is the correct response, not a workaround. diff --git a/launchpad/review-agent/fixtures/verdict_blocks/generate.py b/launchpad/review-agent/fixtures/verdict_blocks/generate.py new file mode 100644 index 00000000000..86c3bafbbe2 --- /dev/null +++ b/launchpad/review-agent/fixtures/verdict_blocks/generate.py @@ -0,0 +1,82 @@ +#!/usr/bin/env python3 +"""Re-record #287's real double-block fixtures from the live PRs they came from. + +Needs network (`gh api` against launchpad-26/buzz). Writes exactly the two +comments named in ADJUDICATION.md's #287 section for each PR to +``recordings/pr--comments.json`` -- id, created_at, user.login, and body, +unmodified from what `gh api` returns. No content in either file is +hand-typed; running this script is the only way either file's bytes are +produced. See PROVENANCE.md. + +Run: python3 generate.py (from this directory, or anywhere -- it locates + launchpad/review-agent/ from its own path) +""" + +from __future__ import annotations + +import json +import os +import sys + +HERE = os.path.dirname(os.path.abspath(__file__)) +REVIEW_AGENT_DIR = os.path.dirname(os.path.dirname(HERE)) +if REVIEW_AGENT_DIR not in sys.path: + sys.path.insert(0, REVIEW_AGENT_DIR) + +import fetch # noqa: E402 + +REPO = "launchpad-26/buzz" +RECORDINGS_DIR = os.path.join(HERE, "recordings") + +#: (PR number, [comment ids to keep]) -- the exact pair named in +#: ADJUDICATION.md's #287 section for each PR. +TARGETS = { + 261: [5364185647, 5364261676], + 264: [5364221899, 5364504768], +} + + +def _fetch_comments(pr: int) -> list[dict]: + state, out, reason = fetch._gh( + ["api", "--paginate", "--slurp", f"repos/{REPO}/issues/{pr}/comments"] + ) + if state != "ok": + raise RuntimeError(f"PR #{pr}: fetch failed ({state}): {reason}") + pages = json.loads(out) + return [item for page in pages for item in page] + + +def record(pr: int, wanted_ids: list[int]) -> str: + items = _fetch_comments(pr) + by_id = {item["id"]: item for item in items} + missing = [cid for cid in wanted_ids if cid not in by_id] + if missing: + raise RuntimeError(f"PR #{pr}: comment id(s) not found in live fetch: {missing}") + kept = sorted( + ( + { + "id": by_id[cid]["id"], + "created_at": by_id[cid]["created_at"], + "user": {"login": by_id[cid]["user"]["login"]}, + "body": by_id[cid]["body"], + } + for cid in wanted_ids + ), + key=lambda c: c["id"], + ) + return json.dumps(kept, indent=2) + "\n" + + +def main() -> int: + os.makedirs(RECORDINGS_DIR, exist_ok=True) + for pr, wanted_ids in TARGETS.items(): + text = record(pr, wanted_ids) + path = os.path.join(RECORDINGS_DIR, f"pr-{pr}-comments.json") + with open(path, "w", encoding="utf-8") as handle: + handle.write(text) + print(f"wrote {os.path.relpath(path, HERE)} ({len(text)} bytes)") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/launchpad/review-agent/fixtures/verdict_blocks/recordings/FALSIFIABILITY.md b/launchpad/review-agent/fixtures/verdict_blocks/recordings/FALSIFIABILITY.md new file mode 100644 index 00000000000..fa15e30369b --- /dev/null +++ b/launchpad/review-agent/fixtures/verdict_blocks/recordings/FALSIFIABILITY.md @@ -0,0 +1,35 @@ +# FALSIFIABILITY — what would show this recording is not real + +`pr-261-comments.json` and `pr-264-comments.json` claim to be the actual, +unedited bodies of four real GitHub PR comments. That claim is checkable, +not just asserted: + +1. **Regeneration.** `python3 ../generate.py` re-fetches both PRs live via + `gh api` and reproduces these two files. If either file's committed bytes + ever silently diverged from a hand edit, `generate.py` writing fresh + bytes and `git diff` showing a change is exactly how that would be + caught — the same mechanism `fixtures/adjudication/`'s own + `test_adjudication_fixtures.py` regeneration check relies on, applied + here as a manual re-run rather than an automated one (no seed/nonce + scheme exists for raw comment bodies to pin against in CI the way an + adjudication document's nonce does). + +2. **Cross-check against the live control.** `check_verdict_resolution.py` + (#287 STEP 5, network-required) resolves PR #261 and PR #264 by calling + `pr_comments.fetch_and_locate` directly against the live API — it does + not read these recordings at all. `test_verdict_resolution.py` (#287 + STEP 6, no network) resolves the SAME two PRs by loading these + recordings through `pr_comments.from_items` instead. Both paths are + asserted to reach the identical outcome: PR #261 accepts comment + `5364261676` and reports `5364185647` superseded; PR #264 accepts + `5364504768` (the Blocker promotion) and reports `5364221899` + superseded. If a recording had been hand-edited to make a test pass — + trimming a row, closing an unclosed fence, changing a severity — the + live control and the fixture-based test would disagree, and disagreeing + is a red suite, not a silent divergence. + +Both files were produced from a single fetch to `gh api +repos/launchpad-26/buzz/issues/{261,264}/comments --paginate --slurp` on +2026-08-27, filtered to the two comment ids ADJUDICATION.md names for each +PR, with `id`/`created_at`/`user.login`/`body` kept verbatim. No row, verdict, +or severity value in either file was typed by hand. diff --git a/launchpad/review-agent/fixtures/verdict_blocks/recordings/pr-261-comments.json b/launchpad/review-agent/fixtures/verdict_blocks/recordings/pr-261-comments.json new file mode 100644 index 00000000000..d45b6822708 --- /dev/null +++ b/launchpad/review-agent/fixtures/verdict_blocks/recordings/pr-261-comments.json @@ -0,0 +1,18 @@ +[ + { + "id": 5364185647, + "created_at": "2026-08-21T01:45:48Z", + "user": { + "login": "ciaran-slow" + }, + "body": "## Review pipeline \u2014 PR #261\n\nStages run: `review-code`, `review-tests`, `review-a11y`, `review-adjudicate`, `review-final`.\nPlan read first: `launchpad/plans/2026-08-13-issue-118-adjudication.md`, STEP 2 at `:362`.\n\n**Not applicable, declared rather than faked:** `review-a11y` \u2014 the plan's LEFT OUT section puts accessibility out of scope for #118 and states why (a definition plus a CLI printing JSON; no UI). `check-ledger.sh` \u2014 the plan uses `STEP N`, not `### Task N:`, and no `.superpowers/sdd/` ledger exists; the checker exits 1 on its vacuity guard, so I walked the step graph by hand.\n\n**I ran the two mechanical clauses of STEP 2's done-when myself**, in a clean worktree at `77065b95b`:\n\n```\n$ python3 -c \"import verdicts\" \u2192 import OK\n$ python3 -c \"import verdicts, review;\n print(verdicts.SEVERITY_ORDER is review.SEVERITY_ORDER)\" \u2192 True\n$ python3 -m unittest test_verdicts \u2192 Ran 17 tests, OK\n```\n\nThe `import` clause matters more than it looks: the plan specifies it *instead of* `py_compile`, because `py_compile` compiles without resolving imports and would pass on a module whose `import review` cannot be satisfied. It resolves.\n\nEverything below came from probing the validator with mutated copies of the suite's own `make_well_formed_pair()` fixture, so the only variable in each case is the mutation.\n\n---\n\n### Findings\n\n#### 1. High \u2014 whitespace-only `verdict_evidence` satisfies \"non-empty\" at every guard in the pipeline\n\n`launchpad/review-agent/verdicts.py:268` and `launchpad/review-agent/run_adjudication.py:516` (the latter on the #263\u2192#267 chain)\n\nThe plan requires *\"`verdict_evidence` present and non-empty on all three\"* verdicts. Both places that enforce it test falsiness, and a whitespace string is truthy.\n\n`verdicts.py:268` \u2014 `if not finding.get(\"verdict_evidence\"):`. Probed:\n\n```\nmutation: reports[0].findings[0].verdict_evidence = \" \"\nverdicts.validate(input, output) \u2192 0 violations\n```\n\n`run_adjudication.py:516` \u2014 `if verdict not in verdicts.VERDICTS or not evidence:` \u2014 has the same shape, and its docstring claims the stronger property: *\"fail closed to `UNPROVEN` on anything unusable \u2014 a raised exception, a non-dict return, an illegal/missing `verdict`, or **empty `verdict_evidence`**.\"* `not \" \"` is `False`, so the whitespace passes and is forwarded verbatim into `safe_result[\"verdict_evidence\"]` at `:527`.\n\n**Concrete failure:** a judge returns `{\"verdict\": \"CONFIRMED\", \"verdict_evidence\": \"\\n\"}` \u2014 the shape a truncated model response or a stripped formatting step actually produces. `_run_judge_safely` accepts it rather than failing closed. `verdicts.validate` accepts it. The document publishes a **CONFIRMED** finding whose evidence is blank, and per `ADJUDICATION.md` a CONFIRMED Blocker is what blocks a merge. So a blank-evidence confirmation can block a merge with no stated reason \u2014 while the ADJUDICATION.md default both docstrings invoke by name, *\"returns unusable output yields UNPROVEN with a reason\"*, silently does not apply to the most likely form of degenerate output.\n\nNothing fails today: `stub_judge` always returns real prose. It goes live with `--replay`, which loads recorded verdicts straight from JSON files, and with any real judge \u2014 STEP 9 being the next planned step.\n\n**Fix, both sites:** test the stripped value \u2014 `if not str(evidence).strip():` in the runner, and `if not str(finding.get(\"verdict_evidence\") or \"\").strip():` in the validator. Two lines. Worth doing at both ends rather than one: the runner's guard is the fail-closed promise and the validator's is the contract, and the plan's own reasoning for re-running `findings.validate` on the output \u2014 *\"a stage that quietly breaks its input's own rules is the same defect as one that drops a finding\"* \u2014 argues for not relying on a single checkpoint.\n\n**One defect, two sites, counted once.** Reported here because this PR defines what \"non-empty\" means for the contract; cross-referenced from #263's review rather than filed twice.\n\n#### 2. Medium \u2014 a `duplicate_groups` entry with no duplicates never has its `survivor` validated at all\n\n`launchpad/review-agent/verdicts.py:425`\n\nThe survivor is only ever checked *indirectly*. `:433` loops over `duplicates`, and for each one confirms the finding exists and that its own `duplicate_of` equals `survivor`. So a bad survivor is normally caught through a duplicate pointing at it. With an empty `duplicates` list, that loop never runs and nothing else looks at `survivor`. Probed:\n\n```\nduplicate_groups = [{\"survivor\": \"no-such-id\", \"duplicates\": []}] \u2192 0 violations\nduplicate_groups = [{\"duplicates\": []}] (no survivor key) \u2192 0 violations\n```\n\nBoth validate clean. The second is worse than the first: the group has no survivor field whatsoever.\n\n**Concrete failure:** the runner emits `duplicate_groups: [{\"survivor\": \"abc123\", \"duplicates\": []}]` after a dedupe pass that grouped nothing \u2014 an off-by-one in the grouping logic, or a survivor whose duplicates were filtered out upstream. `verdicts.validate` reports the document clean. #119 then renders a duplicate group naming a finding_id that may not exist in the document, and there is no finding anywhere marked as its duplicate, so the group asserts a relationship with one end missing. The plan's stated intent is the opposite: *\"the grouping is in the output rather than in the stage's head\u2026 discoverable from the finding as well as from the block.\"* A group with no members is discoverable from neither.\n\n**Fix:** two checks in the loop at `:425`, before the `duplicates` handling \u2014 that `survivor` is a string present in `output_ids`, and that `duplicates` is non-empty. The plan's own done-when says a run that dedupes nothing emits *an empty `duplicate_groups` array*, so an empty group inside a non-empty array is never a legal shape and can be rejected outright.\n\n#### 3. Medium \u2014 a boolean passes every integer check on the count fields\n\n`launchpad/review-agent/verdicts.py:322` and `:324`\n\n`isinstance(True, int)` is `True` in Python \u2014 `bool` subclasses `int` \u2014 and `True == 1`. So both the type guard and the equality chain accept booleans. Probed:\n\n```\nadjudication.findings_in = True\nadjudication.findings_out = True\nevery report's findings_count = True (with one finding each)\nverdicts.validate(input, output) \u2192 0 violations\n```\n\nZero. The document asserts `\"findings_in\": true` and passes the contract check clean.\n\n**Concrete failure:** the plan's own STEP 10 is *\"one control per done-criterion\"*, and this PR's body records a Blocker already found and fixed in the same family \u2014 `validate()` raising `TypeError` on `severity: [\"Blocker\"]` \u2014 explicitly *\"which would have broken STEP 10's planned 'feed every field malformed' controls.\"* `findings_in: true` is exactly such a malformed value, and it is the one that does not raise and does not get reported. When STEP 10 feeds every field malformed, this is a control that will report the validator as accepting garbage.\n\nThe asymmetry is the evidence that strictness was intended: `total_refutation` is checked with `isinstance(declared_total_refutation, bool)` \u2014 a strict bool test that correctly rejects `1` \u2014 while the count fields next to it accept `True`. One of the two reflects the author's intent and it is not the looser one.\n\n**Fix:** `isinstance(x, int) and not isinstance(x, bool)` at both sites, or a small `_is_count()` helper used by all three, since the same test is needed in three places.\n\n#### 4. Low \u2014 missing `by:agent` label\n\nNo labels on this PR; its body carries an *Agent provenance* block. `launchpad/AGENTS.md` \u00a75 rule 3 requires `by:agent`.\n\n`gh pr edit 261 --repo launchpad-26/buzz --add-label by:agent`\n\n---\n\n### What I looked for and did not find\n\nFour things I expected to be defects and confirmed are not. Two were probed, not reasoned about.\n\n- **A missing top-level nonce sliding through.** `:463` compares `output_document.get(\"nonce\")` against `input_document.get(\"nonce\")`, so if *both* lack the key, `None != None` is `False` and the \"present\" half of the plan's requirement looks unenforced. It is enforced \u2014 by #117. Probed by deleting `nonce` from both documents: 3 violations, the first being `document: missing or empty top-level 'nonce'` from `findings_module.validate` at `:232`. The guard is real and upstream, exactly as the plan said it would be.\n- **Containment findings polluting `total_refutation`.** The plan requires containment findings to pass through with no verdict field, and `total_refutation` is `all(v == \"REFUTED\")` over `_iter_findings`. Had `_iter_findings` walked `containment.findings`, every verdict-less containment finding would force the flag to `False` and total refutation would be unreportable on any PR with a containment catch. It does not: `:138-150` reads `document[\"reports\"][*][\"findings\"]` only. Correctly scoped.\n- **`keys[-1]` on an empty mapping.** `:478` indexes `list(adjudication.keys())[-1]`, which would raise on `{}`. Unreachable: the `else` branch only runs when `\"completion_marker\" in adjudication`, so the list is non-empty, and the non-dict path at `:311` substitutes `{}` which fails that test first. No crash.\n- **Prohibition 1 implemented as a grep.** `_find_forbidden_keys` at `:184` walks the parsed structure instead, and the docstring gives the right reason: a grep over serialised text would also flag those words inside `verdict_evidence` prose, which prohibition 1 does not bind. Recursive over dicts and lists, so a forbidden key nested at any depth is caught.\n- **Self-duplication.** Probed a finding named as its own survivor and its own duplicate: caught, `duplicate_of names itself`.\n- **Tests that cannot fail.** All 17. Every fixture and expectation is a literal; the `make_*` helpers build dicts from literals with keyword overrides and compute nothing. `test_four_independent_violations_surface_at_once` is the right test for the \"returns EVERY violation\" requirement, and it asserts on the count rather than on a mock.\n- **An undeclared scope gap.** `validate`'s docstring at `:207-213` states plainly that `schema_version`, `verdict_counts` and `notes` are unchecked, why (STEP 1 assigns one-control-per-key to STEP 10), and the consequence: *\"A document with a fabricated `verdict_counts` or a wrong-typed `schema_version` passes this function with zero violations today.\"* STEP 2's done-when names none of the three. A limitation the code documents is not a finding, and this one is documented better than most.\n\n### On CI coverage \u2014 different from #260 and #262\n\nI reported on #260 and #262 that no CI job runs their test directories. The same is mechanically true here \u2014 `run_controls.py`'s `CONTROLS` list is hardcoded and names no `test_*.py`, and `suite.py` is #120's containment suite, not a discoverer \u2014 but this PR **states the decision and names its owner**, at `test_verdicts.py:9-13`:\n\n> *\"deliberately not wired into `run_controls.py`'s CONTROLS list \u2014 that is STEP 10's control suite, over the full adjudication surface (`run_adjudication.py` included), not this module in isolation.\"*\n\nThat is a real plan step (`:741`, `[needs 4, 6, 7, 9]`), not a hand-wave, so I am **not** filing it as a finding here. The distinction is worth stating because it does not hold on #260/#262, where nothing was said. What nothing currently enforces is that STEP 10 actually wires them \u2014 worth carrying into STEP 10's own review rather than blocking this.\n\n### Triage of deferred items\n\nNothing arrived deferred or parked; no prior reviews or comments on this PR. The PR body records one Blocker found and fixed by the author's own pre-PR review pass (`f79aa3d64`, `validate()` raising `TypeError` on non-string field values). I re-probed that family and it holds: `severity: [\"Blocker\"]` now yields a violation rather than a `TypeError`. Finding 3 is the surviving member of that same family, in the opposite direction \u2014 a value too permissive rather than one that crashes.\n\n### Merge readiness\n\nA reader of #118 STEP 2 would find a validator that does what the step asked, including the parts that are easy to half-do. It returns every violation rather than raising on the first. It takes both documents, and the docstring at `:215-221` reconstructs *why* the earlier one-document revision could not work \u2014 a count cannot distinguish a drop from an invention. Both-directions checking is genuinely both-directions in all four places the plan demanded it: downgrades, `total_refutation`, `duplicate_groups`, and the finding_id set. `SEVERITY_ORDER` is the same object as `review`'s, not a copy, which is the difference between a shared ladder and two that can drift.\n\nThey would also find three holes where a type or an emptiness test is looser than the contract it enforces, all in the same family: falsiness standing in for non-emptiness, `int` accepting `bool`, and a validation that only runs when a list is non-empty. Finding 1 is the one that matters, because it defeats a fail-closed guarantee that two modules state explicitly and because it can publish a merge-blocking verdict with no evidence. All three fixes are one to three lines.\n\nWhat I could not check: whether `verdict_counts` and `schema_version` are correct in practice, since nothing validates them yet by design. And I did not review `run_adjudication.py` here beyond the two lines finding 1 cites \u2014 that is #263's and #264's diff, reviewed separately.\n\n### Independence and tools\n\nIndependent of the code under review: I did not write it. **Not independent across pipeline stages** \u2014 one context ran the reviewers, the adjudicator and the final pass, where the skills call for a fresh context per stage. All four findings are self-adjudicated. Treat that as a limit on this report.\n\nTools actually held and used: `Bash` (git, `git grep`, `git worktree`, `gh`, `python3` for the probes), `Read`, `Edit`, `Write`. No `Grep` or `Glob` tool was available in this session.\n\nNothing found at Blocker.\n\n```verdict\nCONFIRMED\tHigh\tlaunchpad/review-agent/verdicts.py:268\twhitespace-only verdict_evidence passes \"non-empty\"; same bug at run_adjudication.py:516 defeats fail-closed\nCONFIRMED\tMedium\tlaunchpad/review-agent/verdicts.py:425\tduplicate_groups entry with empty duplicates never validates its survivor\nCONFIRMED\tMedium\tlaunchpad/review-agent/verdicts.py:322\tbool passes every int check on findings_in/findings_out/findings_count\nCONFIRMED\tLow\tPR #261 (labels)\tmissing required by:agent label\n```\n\nHanded 4 findings, confirmed 4, refuted 0, merged 1 pair \u2014 finding 1's validator and runner halves are one defect on one row. Four further candidates were **REFUTED by probe** and are recorded above rather than dropped silently: missing-nonce (caught upstream by `findings.validate`), containment findings in `total_refutation` (`_iter_findings` correctly excludes them), `keys[-1]` on an empty mapping (unreachable), and self-duplication (caught). 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.\n\nADJUDICATION COMPLETE\n\nREVIEW COMPLETE\n\n---\n\n*Per `launchpad/AGENTS.md` \u00a75 rule 1 \u2014 an agent drafts and raises, never approves or clears. This is a report, not an approval; the merge decision is @ciaran-slow's.*\n" + }, + { + "id": 5364261676, + "created_at": "2026-08-21T01:58:30Z", + "user": { + "login": "ciaran-slow" + }, + "body": "## Correction to finding 2 above \u2014 reachability, not the finding itself\n\nReviewing #267 (STEP 7) put the producer side of `duplicate_groups` in front of me, and it changes one thing I wrote.\n\n**What stands:** `verdicts.validate` does accept a `duplicate_groups` entry whose `duplicates` list is empty, and in that case nothing validates `survivor` at all. Re-probed just now, unchanged:\n\n```\nduplicate_groups = [{\"survivor\": \"no-such-id\", \"duplicates\": []}] \u2192 0 violations\nduplicate_groups = [{\"duplicates\": []}] (no survivor key) \u2192 0 violations\n```\n\n**What I got wrong:** my failure scenario said the runner might emit such a group *\"after a dedupe pass that grouped nothing\"*. It cannot. `_build_duplicate_groups` in #267 drops any group left with fewer than two distinct, real, unclaimed `finding_id`s:\n\n```python\nif len(candidate_ids) < 2:\n continue\n```\n\nI probed that too \u2014 a dedupe judge asked to group a finding with itself, and one returning the same pair twice, both yield a correct single group or none, never an empty one.\n\n**So the corrected reading:** this is a validator-only gap, reachable from a hand-written or forged document, or from STEP 10's planned \"feed every field malformed\" controls \u2014 not from the current producer. That makes it **materially less urgent than I framed it**. I would still fix it, because catching what a producer *might* do wrong is the validator's whole job and STEP 10 is going to feed it exactly this, but it is not a live path.\n\nFindings 1 and 3 are unaffected. Finding 1 in particular I re-verified end-to-end through `adjudicate()` on the chain tip while reviewing #266 and #267: a judge returning `{\"verdict\": \"CONFIRMED\", \"verdict_evidence\": \" \\n \"}` still yields a CONFIRMED verdict with whitespace evidence and `verdicts.validate` still reports 0 violations. That one is real and live.\n\nRevised severity for finding 2 only: **Medium \u2192 Low**.\n\n```verdict\nCONFIRMED\tHigh\tlaunchpad/review-agent/verdicts.py:268\twhitespace-only verdict_evidence passes \"non-empty\"; same bug at run_adjudication.py:516 defeats fail-closed\nCONFIRMED\tLow\tlaunchpad/review-agent/verdicts.py:425\tduplicate_groups entry with empty duplicates never validates its survivor \u2014 validator-only, current producer cannot emit it\nCONFIRMED\tMedium\tlaunchpad/review-agent/verdicts.py:322\tbool passes every int check on findings_in/findings_out/findings_count\nCONFIRMED\tLow\tPR #261 (labels)\tmissing required by:agent label\n```\n\nThis block supersedes the one in my earlier comment. Ranking is unchanged apart from finding 2 dropping below finding 3.\n\nADJUDICATION COMPLETE\n\nREVIEW COMPLETE\n" + } +] diff --git a/launchpad/review-agent/fixtures/verdict_blocks/recordings/pr-264-comments.json b/launchpad/review-agent/fixtures/verdict_blocks/recordings/pr-264-comments.json new file mode 100644 index 00000000000..3dc85e272da --- /dev/null +++ b/launchpad/review-agent/fixtures/verdict_blocks/recordings/pr-264-comments.json @@ -0,0 +1,18 @@ +[ + { + "id": 5364221899, + "created_at": "2026-08-21T01:51:51Z", + "user": { + "login": "ciaran-slow" + }, + "body": "## Review pipeline \u2014 PR #264\n\nStages run: `review-code`, `review-tests`, `review-a11y`, `review-adjudicate`, `review-final`.\nPlan read first: `launchpad/plans/2026-08-13-issue-118-adjudication.md`, STEP 4 at `:501`.\nDiffed against this PR's own base (`feat/review-agent-adjudication-run`, #263), so only STEP 4's own changes are in scope.\n\n**Not applicable, declared:** `review-a11y` \u2014 accessibility is out of scope for #118 per the plan's LEFT OUT. `check-ledger.sh` \u2014 plan uses `STEP N`, not `### Task N:`; no `.superpowers/sdd/` ledger; the checker exits 1 on its vacuity guard.\n\nRan the suite: `python3 -m unittest test_run_adjudication` \u2192 **Ran 40 tests, OK**.\n\nI drove every one of STEP 4's done-when clauses through the real CLI over stdin rather than through the test helpers, so the refusals are proven observable end-to-end. All pass, with three genuinely distinct reason strings:\n\n```\nno top-level nonce \u2192 exit 1 \"absent provenance: no top-level `nonce` is present\"\nreports disagree w/ each other\u2192 exit 1 \"mixed document: reports carry different nonces\u2026\"\nreports agree, header differs \u2192 exit 1 \"mismatched envelope: every report's completion marker carries\u2026\"\nboth 2 and 3 at once \u2192 exit 1 reported as mixed document \u2190 the plan's precedence rule\nreport with no marker \u2192 exit 1 \"absent provenance: at least one report carries no parseable\u2026\"\nmarker nonce a PREFIX of key \u2192 exit 1 mismatched envelope (no sloppy prefix match)\nlookalike `adjudication_nonce` \u2192 exit 1 absent provenance (no caller-supplied nonce accepted)\nmalformed JSON \u2192 exit 1 stdout empty\n```\n\nEvery refusal printed no document. That is the step's core requirement and it holds.\n\n---\n\n### Findings\n\n#### 1. High \u2014 a `stages` value that is not a list is silently discarded, which bypasses the re-run guard and can drop a `blocked` pre-flight status\n\n`launchpad/review-agent/run_adjudication.py:258` and `:474`\n\nTwo sites treat a malformed `stages` container as an absent one, silently:\n\n```python\n:257 stages = document.get(\"stages\")\n:258 if not isinstance(stages, list):\n:259 return # \u2190 the re-run guard gives up, says nothing\n\n:474 input_stages = copy.deepcopy(input_stages_raw) if isinstance(input_stages_raw, list) else []\n # \u2190 the manifest replaces it with empty, says nothing\n```\n\n**Probed through the CLI. The control case behaves correctly; the malformed container does not:**\n\n```\nstages = [{\"name\":\"adjudication\",\"status\":\"complete\"}] \u2192 exit 1, correctly refused\nstages = {\"0\":{\"name\":\"adjudication\",\"status\":\"complete\"}} \u2192 exit 0, ADJUDICATED\nstages = '[{\"name\":\"adjudication\",\"status\":\"complete\"}]' (str) \u2192 exit 0, ADJUDICATED\n```\n\nThe same adjudication entry, in a container of the wrong type, passes a guard whose entire purpose is to catch it.\n\n**And the data-loss case, which is the one that matters for the product:**\n\n```\nstages = {\"preflight\":{\"name\":\"preflight\",\"status\":\"blocked\",\n \"reason\":\"fork PR, secrets withheld\"}}\n\u2192 exit 0, output stages = ['adjudication'], status complete\n```\n\nThe pre-flight entry is gone. #119 treats any status other than `\"complete\"` as incomplete and banners it at the top of the body \u2014 that is the mechanism the plan relies on so *\"a totally-refuting run cannot publish as a clean review.\"* Here a `blocked` pre-flight \u2014 #116's fork-PR-secrets-withheld case, exactly the situation the entry exists to report \u2014 is dropped, and the document publishes as a clean, complete review with no banner. It reaches the issue's fifth-criterion failure (\"reported as a clean PR\") through a shape defect instead of through verdicts, which is why no verdict-side check catches it.\n\n**Why this is a defect and not merely a malformed input.** This step's own stated rationale is that it must not trust its producer: *\"a stage agnostic about its producer cannot inherit a guarantee it did not watch being made, and this one is downstream of an unmerged plan whose validator does not exist yet.\"* Under that posture, silently normalising a wrong-typed container is the one thing it may not do. And the module refuses malformed input loudly everywhere else \u2014 `main:313` refuses valid-but-non-object JSON specifically so `findings.validate`'s dict assumption is never exercised, and `_verify_nonce` has three separately-worded refusals. `stages` is the exception, in both places that read it.\n\n**The project has already written this rule down, twice.** `run_controls.py`'s docstring: *\"A control whose input is missing reports SKIP with a reason and **never** PASS\u2026 Absence of evidence is not evidence.\"* And `check-ledger.sh` states the same divergence deliberately: *\"a missing ledger is a FAIL, not a SKIP. 'No ledger exists' is not an absent input \u2014 it is positive evidence that nothing was gated, which is the exact condition being tested. The shared rule that matters is upheld: a missing input NEVER reports PASS.\"* A non-list `stages` is that shape precisely.\n\n**Fix, one line at each site:** raise rather than return/default when `stages` is present but not a list \u2014 a `StagesShapeError` alongside the existing `AlreadyAdjudicatedError` and `NonceVerificationError`, with a reason naming the type found. `main` already turns those into exit 1 with no document, so nothing else changes. Absent (`stages` key missing) stays legal; present-but-wrong-typed becomes a refusal.\n\n**I am rating this High rather than Blocker, and the argument for promoting it is real.** The plan says plainly: *\"It never overwrites an existing `adjudication` entry silently: a second one on input is a re-run against an already-adjudicated document and exits non-zero.\"* Probe B1 is an input carrying an `adjudication` entry that does not exit non-zero, which is a stated rule broken \u2014 the Blocker definition. What holds me at High is that it needs input today's only producer (#117) never emits, so nothing is wrong on a well-formed run. **That mitigation is exactly the reasoning this step was written to reject**, so if you read it as Blocker I would not argue.\n\n#### 2. Low \u2014 a `stages` entry whose `name` is not a string passes through untouched\n\n`launchpad/review-agent/run_adjudication.py:261`\n\n`entry.get(\"name\") == \"adjudication\"` is a bare equality test, so `{\"name\": {\"nested\": \"adjudication\"}}` is neither matched nor objected to. Probed: it passes straight into the output's `stages` array and the run reports `complete`.\n\nThis is much milder than finding 1 \u2014 a non-string name cannot impersonate an `adjudication` entry, so the guard is not bypassed this way. But #119 reads `{name, status, reason}` entries, and the output now carries one whose `name` is an object. `verdicts.validate` does not check `stages` at all (it is outside the nine `adjudication` keys, documented at `verdicts.py:207-213`), so nothing downstream of here objects either. Fixing finding 1 with a type-checked shape validation would cover this in the same change.\n\n#### 3. Low \u2014 missing `by:agent` label\n\nNo labels; the body carries an *Agent provenance* block. `launchpad/AGENTS.md` \u00a75 rule 3.\n\n`gh pr edit 264 --repo launchpad-26/buzz --add-label by:agent`\n\n---\n\n### What I looked for and did not find\n\n- **The nonce-before-`findings.validate` reordering being a STEP 3 regression.** `36348cc0d` moves `_verify_nonce` ahead of `findings.validate`, which reads at first like it breaks STEP 3's *\"the input is validated before a single finding is adjudicated.\"* It does not, and the commit message reasons it correctly: *\"findings.validate still runs before any finding reaches the judge loop \u2014 STEP 3's actual guarantee \u2014 just second now instead of first.\"* The reorder was necessary because `findings.validate` also rejects nonce mismatches, with one generic per-report message, so it always won the race and STEP 4's three distinct refusals were *provably unreachable through `main()`*. The tests were strengthened to assert the specific reason text so a revert fails. This is the right fix for the right reason.\n- **The follow-up special case being a papered-over symptom.** `2be90c629` defers to `findings.validate` when `reports` is missing, non-list or empty, so a shape defect is not reported as \"absent provenance\". The duplicated `findings.validate`-and-raise in both branches is slightly awkward, and the `else` branch's `_verify_nonce` call is unreachable in practice \u2014 but it is *kept as a real call rather than asserted away*, with the reasoning stated, matching the same discipline used for `stage_complete`'s conditions. Deliberate, documented, and not a finding.\n- **Prefix or substring matching on nonces.** Probed a marker nonce that is the top-level nonce minus its last character: correctly reported as a mismatched envelope. No sloppy `startswith`.\n- **A caller being able to inject provenance.** Probed a document with no `nonce` but a plausible `adjudication_nonce` key: refused as absent provenance. The step never substitutes one of its own, as the plan requires.\n- **`stage_complete` becoming unconditionally true.** `:476-487` names `nonce_established` as its own boolean even though `_verify_nonce` would already have raised, and names STEP 6's not-yet-built flag as a third condition rather than inlining an `and` chain that would silently drop it. `every_finding_has_verdict` is read back off `output_document` rather than tracked as a parallel counter \u2014 a check *on* the produced data instead of a second bookkeeping path that could drift. This is the opposite of the defect I was looking for.\n- **The all-forged-nonce run.** Not caught, and correctly named as out of reach from inside a document \u2014 the plan says so at `:534-538` and the module repeats it. Reporting it would be reporting the plan's own stated limitation.\n- **Tests that cannot fail.** All 40. Literals throughout; the nonce tests assert on specific reason substrings in stderr rather than on exit code alone, which is what makes the three refusals genuinely distinguishable by the suite.\n- **Order preservation.** Probed a pre-existing `preflight` entry in a well-formed list: preserved, in order, with `adjudication` appended after it.\n\n### On CI coverage\n\n`run_controls.py`'s `CONTROLS` list is hardcoded and names no `test_*.py`, so this suite runs in no CI job. As on #261 and #263, that is a **stated, deferred decision** assigned to STEP 10 (`plan:741`), so I am not filing it as a finding \u2014 noting only that nothing yet enforces STEP 10 actually doing it.\n\n### Merge readiness\n\nA reader of #118 STEP 4 would find the hard part done well. Three nonce refusals with three distinct reasons, the precedence rule between them implemented as the plan specified rather than left to code order, no document printed on any refusal, no invented nonce, and the pre-flight entry preserved in order. I drove all of it through the CLI rather than trusting the suite, and it held.\n\nThey would also find that the one input this step is explicitly built to distrust \u2014 a `stages` array from an unmerged upstream producer \u2014 is the one input it normalises silently instead of refusing, in both places it reads it. Finding 1 is a few lines and closes the re-run bypass and the dropped-status path together.\n\n**Merge order:** base is #263's branch. The stack is #261 \u2192 #263 \u2192 #264 \u2192 #266 \u2192 #267.\n\nWhat I could not check: whether #117 could ever emit a non-list `stages`. I read no path where it does, which is why finding 1 is rated High rather than Blocker \u2014 but the step's own design premise is not to depend on that answer.\n\n### Independence and tools\n\nIndependent of the code under review: I did not write it. **Not independent across pipeline stages** \u2014 one context ran the reviewers, the adjudicator and the final pass. All findings self-adjudicated; treat that as a limit.\n\nTools actually held and used: `Bash` (git, `git show`, `git worktree`, `gh`, `python3` and `subprocess` for the CLI probes), `Read`, `Edit`, `Write`. No `Grep` or `Glob` tool was available in this session.\n\nNothing found at Blocker, with finding 1 flagged as promotable and the argument given.\n\n```verdict\nCONFIRMED\tHigh\tlaunchpad/review-agent/run_adjudication.py:258\tnon-list `stages` silently discarded: bypasses the re-run guard and can drop a blocked pre-flight status (also :474)\nCONFIRMED\tLow\tlaunchpad/review-agent/run_adjudication.py:261\tstages entry with a non-string `name` passes through unchecked\nCONFIRMED\tLow\tPR #264 (labels)\tmissing required by:agent label\n```\n\nHanded 3 findings, confirmed 3, refuted 0, merged 0. Eight further candidates were **REFUTED by probe or inspection** and are recorded above 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.\n\nADJUDICATION COMPLETE\n\nREVIEW COMPLETE\n\n---\n\n*Per `launchpad/AGENTS.md` \u00a75 rule 1 \u2014 an agent drafts and raises, never approves or clears. This is a report, not an approval; the merge decision and the Blocker/High call on finding 1 are @ciaran-slow's.*\n" + }, + { + "id": 5364504768, + "created_at": "2026-08-21T02:36:23Z", + "user": { + "login": "ciaran-slow" + }, + "body": "## Severity promoted: High \u2192 **Blocker** (finding 1)\n\n@ciaran-slow has promoted the non-list `stages` finding to Blocker. Recording it here so the verdict block a gate reads matches the decision.\n\nI proposed High and set out the argument both ways; the call was never mine to make. `launchpad/AGENTS.md` \u00a75 rule 1 \u2014 an agent raises concerns and never clears them \u2014 and severity that decides whether a merge stops is a human's call.\n\n### What the promotion rests on\n\nThe plan's requirement is unconditional (`launchpad/plans/2026-08-13-issue-118-adjudication.md:547`):\n\n> *\"It never overwrites an existing `adjudication` entry silently: a second one on input is a re-run against an already-adjudicated document and exits non-zero.\"*\n\nProbe B1 is an input carrying an `adjudication` entry that does not exit non-zero. That is a stated rule broken, which is the Blocker definition rather than the High one. The mitigation I weighed \u2014 that it needs input #117 never emits \u2014 is the reasoning this step was explicitly written to reject:\n\n> *\"a stage agnostic about its producer cannot inherit a guarantee it did not watch being made, and this one is downstream of an unmerged plan whose validator does not exist yet.\"*\n\nA step that exists because it must not trust its producer cannot be excused by trusting its producer. Promotion is the consistent reading.\n\n### Re-verified against the whole stack, just now\n\nBoth guard sites are unchanged downstream \u2014 only the line numbers move:\n\n| branch | `if not isinstance(stages, list): return` | `\u2026 if isinstance(input_stages_raw, list) else []` |\n|---|---|---|\n| #264 | `:258` | `:474` |\n| #266 | `:327` | `:674` |\n| #267 | `:388` | `:873` |\n\nAnd the bypass still reproduces through the CLI on the chain tip (`pr/267`):\n\n```\nadjudication entry in a LIST exit=1 refused\nsame entry in an OBJECT exit=0 ADJUDICATED stages=[('adjudication','complete')]\npreflight BLOCKED in an OBJECT exit=0 ADJUDICATED stages=[('adjudication','complete')]\n```\n\nThe third line is the one that costs something in production: a `preflight` entry with `status: \"blocked\", reason: \"fork PR, secrets withheld\"` is discarded, and the document publishes as `complete`. #119 banners any status other than `\"complete\"`, so the banner never fires and the run reads as a clean review \u2014 the issue's own fifth-criterion failure, reached through a shape defect that no verdict-side check looks at.\n\n### What this changes about merging\n\n**#264, #266 and #267 all carry it.** A Blocker on #264 is therefore a Blocker on everything stacked above it, because the code is identical and the defect reproduces on the tip. The fix belongs on this branch and propagates up the stack; it should not be patched separately in #266 or #267.\n\nRevised stack readiness:\n\n| PR | Blocking on this finding? |\n|---|---|\n| #261, #263 | no \u2014 earlier than the `stages` code |\n| **#264** | **yes \u2014 fix here** |\n| **#266, #267** | **yes \u2014 inherited; clears when #264's fix propagates** |\n| #265 | no \u2014 documentation only |\n\n### The fix\n\nOne line at each of the two sites: raise instead of returning or defaulting when `stages` is present but not a list. A `StagesShapeError` alongside the existing `AlreadyAdjudicatedError` and `NonceVerificationError`, with a reason naming the type found \u2014 `main` already turns those into exit 1 with no document, so nothing else changes. An absent `stages` key stays legal; present-but-wrong-typed becomes a refusal.\n\nThat also resolves finding 2 (a `stages` entry whose `name` is not a string) in the same change, since a shape check would cover both.\n\nSuggested control, in the style of the existing nonce tests which assert on stderr text rather than exit code alone: feed `stages` as an object, as a string, and as a number, and assert exit 1 with a reason naming `stages` \u2014 plus the positive case, that an absent `stages` key still adjudicates.\n\n```verdict\nCONFIRMED\tBlocker\tlaunchpad/review-agent/run_adjudication.py:258\tnon-list `stages` silently discarded: bypasses the re-run guard and can drop a blocked pre-flight status (also :474)\nCONFIRMED\tLow\tlaunchpad/review-agent/run_adjudication.py:261\tstages entry with a non-string `name` passes through unchecked\nCONFIRMED\tLow\tPR #264 (labels)\tby:agent label \u2014 since added\n```\n\n**This block supersedes the one in my earlier review comment on this PR.** Two blocks now exist across the two comments; if a gate scrapes all comments rather than one report, this is the authoritative one \u2014 it is the later of the two and the only one carrying the Blocker. Ranking is otherwise unchanged.\n\nADJUDICATION COMPLETE\n\nREVIEW COMPLETE\n" + } +] diff --git a/launchpad/review-agent/pr_comments.py b/launchpad/review-agent/pr_comments.py new file mode 100644 index 00000000000..f763a49e0cc --- /dev/null +++ b/launchpad/review-agent/pr_comments.py @@ -0,0 +1,221 @@ +"""Fetch one PR's full comment set, per-comment, and locate every ```verdict block. + +Implements launchpad-26/buzz#287 STEP 4. `fetch.py`'s `Surface`/`_joined()` +flattens every comment into one joined string and discards comment id, +author, and creation time entirely -- not reusable here. STEP 5's +resolution rule needs per-comment boundaries, to refuse a same-comment +double-block, and `(created_at, comment_id)` ordering, to pick "the last +one" across different comments -- both erased by `_joined`. + +This module borrows `fetch.py`'s pagination incantation (`gh api --paginate +--slurp` against both `issues/{pr}/comments` and `pulls/{pr}/comments`) and +its `UNREADABLE` state model / `CAP_PER_ENTRY_POINT` cap, applied per +surface here rather than to one joined string, so a failed or oversized +fetch is a distinguishable state per surface, never silently "zero +comments". +""" + +from __future__ import annotations + +import json +from dataclasses import dataclass, field + +import fetch +from verdict_blocks import LocatedBlock, locate_verdict_blocks + +DEFAULT_REPO = fetch.DEFAULT_REPO + +#: The two comment surfaces #287's OPEN section keeps in scope. `fetch.py` +#: already distinguishes them; excluding one silently here would be a +#: narrower guard than the issue asks for (see the plan's OPEN section). +SURFACE_ENDPOINTS = { + "issue": "issues/{pr}/comments", + "review": "pulls/{pr}/comments", +} + +#: `fetch.py` (same directory, same two GitHub endpoints) calls these surfaces +#: `pr_issue_comments`/`pr_review_comments` -- the vocabulary `contain.py`/ +#: `run_dimensions.py`'s own `--degrade` flags already use. Without this map, +#: an operator who knows that convention and tries `--degrade +#: pr_issue_comments=absent` here hits an uncaught `ValueError` traceback +#: instead of the normal refusal a genuine typo would get. `_normalize_surface` +#: accepts either spelling; this module's own short names stay canonical +#: everywhere else (`SURFACE_ENDPOINTS` keys, `TaggedBlock.surface`, the +#: `results` dict `fetch_and_locate`/`resolve` pass around) rather than +#: renaming those, which would touch every caller for a purely cosmetic gain. +_SURFACE_ALIASES = { + "pr_issue_comments": "issue", + "pr_review_comments": "review", +} + + +def _normalize_surface(name: str) -> str: + return _SURFACE_ALIASES.get(name, name) + + +@dataclass +class TaggedBlock: + """One located ```verdict block, tagged with where it came from.""" + + block: LocatedBlock + comment_id: int + surface: str # "issue" | "review" + created_at: str + position: int # 0-indexed position of this block within its own comment + + +@dataclass +class CommentFetch: + """One surface's fetch outcome: readable ("ok") or one of fetch.UNREADABLE.""" + + state: str + reason: str = "" + blocks: list[TaggedBlock] = field(default_factory=list) + + @property + def readable(self) -> bool: + return self.state not in fetch.UNREADABLE + + +def _fetch_items(endpoint: str) -> tuple[str, list[dict], str]: + """Returns (state, items, reason). Mirrors `fetch._json_field` + `fetch._classify`, + applied to a paginated comment list rather than a single JSON field.""" + state, out, reason = fetch._gh(["api", "--paginate", "--slurp", endpoint]) + if state != "ok": + return state, [], reason + try: + pages = json.loads(out) + except json.JSONDecodeError as exc: + return "unparseable", [], f"malformed JSON: {exc}" + try: + items = [item for page in pages for item in page] + except TypeError as exc: + return "unparseable", [], f"unexpected shape: {exc}" + total_bytes = sum(len((item.get("body") or "").encode("utf-8")) for item in items) + if total_bytes > fetch.CAP_PER_ENTRY_POINT: + return ( + "oversized", + items, + f"{total_bytes} bytes exceeds the {fetch.CAP_PER_ENTRY_POINT}-byte cap; " + "refused rather than truncated", + ) + return "ok", items, "" + + +def fetch_and_locate(pr: int, repo: str = DEFAULT_REPO) -> dict[str, CommentFetch]: + """One CommentFetch per surface: "issue" and "review". + + Every ```verdict block in every comment on that surface is located and + tagged with its source comment's id, surface, created_at, and position + within that comment -- the shape STEP 5's resolution rule consumes. + """ + base = f"repos/{repo}" + result: dict[str, CommentFetch] = {} + for surface, template in SURFACE_ENDPOINTS.items(): + endpoint = f"{base}/{template.format(pr=pr)}" + state, items, reason = _fetch_items(endpoint) + if state != "ok": + result[surface] = CommentFetch(state=state, reason=reason) + continue + result[surface] = from_items(items, surface) + return result + + +def _tag_items(items: list[dict], surface: str) -> list[TaggedBlock]: + tagged: list[TaggedBlock] = [] + for item in items: + comment_id = item.get("id") + created_at = item.get("created_at") or "" + body = item.get("body") or "" + located = locate_verdict_blocks(body) + for position, block in enumerate(located): + tagged.append(TaggedBlock(block, comment_id, surface, created_at, position)) + return tagged + + +def from_items(items: list[dict], surface: str) -> CommentFetch: + """Build a readable ``CommentFetch`` from an already-fetched list of raw + comment dicts (``id``/``created_at``/``body``), running the real locator + over each body -- the same tagging ``fetch_and_locate`` performs, minus + the live ``gh api`` call. Lets a recorded fixture (see + ``fixtures/verdict_blocks/``) replay through the real locate/tag pipeline + rather than a hand-written stand-in for it, the same pattern + ``fetch.from_payload`` gives #117's own fixtures. + """ + return CommentFetch(state="ok", blocks=_tag_items(items, surface)) + + +def degrade(results: dict[str, CommentFetch], spec: str) -> dict[str, CommentFetch]: + """Force a surface into a degenerate state, mirroring `fetch.degrade`'s CLI shape: + ``degrade(results, "issue=absent")``. Accepts either this module's own short + surface name or `fetch.py`'s longer one (`_SURFACE_ALIASES`) -- the reason + string still names the spec exactly as given, unnormalized, so the caller + sees what they actually typed. + + State vocabulary is `fetch.UNREADABLE` plus ``"empty"`` -- narrower than + `fetch.degrade`'s (no ``"oversized"``-only-for-diffs equivalent needed + here beyond what `fetch.UNREADABLE` already covers), but ``"empty"`` is + included deliberately: a surface with zero comments is READABLE + (``CommentFetch.readable`` only excludes `fetch.UNREADABLE`), so this + forces the "zero blocks on this surface" shape without needing a real PR + that happens to have none. + """ + surface, _, state = spec.partition("=") + surface = _normalize_surface(surface) + if surface not in SURFACE_ENDPOINTS: + raise ValueError(f"unknown surface: {surface!r}") + if state not in fetch.UNREADABLE and state != "empty": + raise ValueError(f"unknown state: {state!r}") + results = dict(results) + results[surface] = CommentFetch(state=state, reason=f"forced by --degrade {spec}") + return results + + +def _main(argv: list[str] | None = None) -> int: + """CLI: ``python3 pr_comments.py --pr [--repo owner/repo] [--degrade + SURFACE=STATE ...]`` -- prints every tagged block found, mirroring + `contain.py`/`run_dimensions.py`'s own `--degrade` shape (fetch first, + then apply any forced-state overrides on top).""" + import argparse + + parser = argparse.ArgumentParser( + prog="pr_comments.py", + description="Fetch and locate every ```verdict block in one PR's comments.", + ) + parser.add_argument("--pr", type=int, required=True, help="pull request number to fetch live") + parser.add_argument("--repo", default=DEFAULT_REPO) + parser.add_argument( + "--degrade", + action="append", + default=[], + metavar="SURFACE=STATE", + help="force a surface into a degenerate state, e.g. review=absent", + ) + args = parser.parse_args(argv) + + results = fetch_and_locate(args.pr, args.repo) + try: + for spec in args.degrade: + results = degrade(results, spec) + except ValueError as exc: + # A normal refusal (usage error, exit 2), not an uncaught traceback -- + # the failure mode finding 5 (#287) was filed against. + parser.error(str(exc)) + + for surface, cf in results.items(): + if not cf.readable: + print(f"{surface}: UNREADABLE state={cf.state!r} reason={cf.reason!r}") + continue + print(f"{surface}: {len(cf.blocks)} block(s)") + for tb in cf.blocks: + print( + f" comment={tb.comment_id} created_at={tb.created_at} " + f"position={tb.position} closed={tb.block.closed}" + ) + return 0 + + +if __name__ == "__main__": + import sys + + sys.exit(_main(sys.argv[1:])) diff --git a/launchpad/review-agent/run_controls.py b/launchpad/review-agent/run_controls.py index 98a69e17dac..b2f44af7941 100644 --- a/launchpad/review-agent/run_controls.py +++ b/launchpad/review-agent/run_controls.py @@ -34,6 +34,11 @@ ("check_adjudication_mutations.py", False), ("check_publish_scope.py", True), ("check_publish_single.py", False), + ("check_verdict_blocks.py", False), + ("check_verdict_rows.py", False), + ("check_pr_comments.py", True), + ("check_verdict_resolution.py", True), + ("check_resolve_verdict_contract.py", False), ] diff --git a/launchpad/review-agent/test_verdict_resolution.py b/launchpad/review-agent/test_verdict_resolution.py new file mode 100644 index 00000000000..82159704cfc --- /dev/null +++ b/launchpad/review-agent/test_verdict_resolution.py @@ -0,0 +1,269 @@ +#!/usr/bin/env python3 +"""#287 STEP 6: the issue's five named control shapes, plus STEP 5's sixth. + +No network. `PR #261`/`PR #264` are real fixtures, recorded (not re-fetched +live here) under `fixtures/verdict_blocks/` -- see that directory's +PROVENANCE.md and `recordings/FALSIFIABILITY.md` for what is real and how +that claim is checked. Every other shape is synthetic, built directly from +`pr_comments.CommentFetch`/`TaggedBlock` and `verdict_blocks.LocatedBlock`. + +Six shapes, six distinct test methods -- not one method asserting all six, +per this plan's own GATES note that `serina:review-tests` checks specifically +for that. + +Run: python3 -m unittest test_verdict_resolution (from launchpad/review-agent/) + or: python3 test_verdict_resolution.py +""" + +from __future__ import annotations + +import json +import os +import unittest + +from pr_comments import CommentFetch, TaggedBlock, from_items +from verdict_blocks import LocatedBlock, locate_verdict_blocks +from verdict_resolution import resolve + +HERE = os.path.dirname(os.path.abspath(__file__)) +RECORDINGS_DIR = os.path.join(HERE, "fixtures", "verdict_blocks", "recordings") + +ROW = "CONFIRMED\tHigh\tfoo.py:1\tsomething is wrong" + + +def _load_recording(pr: int) -> list[dict]: + with open(os.path.join(RECORDINGS_DIR, f"pr-{pr}-comments.json"), encoding="utf-8") as handle: + return json.load(handle) + + +def _empty_fetch() -> CommentFetch: + return CommentFetch(state="ok", blocks=[]) + + +def _well_formed_located(text: str = ROW) -> LocatedBlock: + return LocatedBlock(start_line=1, end_line=3, closed=True, raw_rows=text) + + +def _malformed_located() -> LocatedBlock: + return LocatedBlock(start_line=1, end_line=3, closed=True, raw_rows="not enough fields") + + +class ZeroBlocksTests(unittest.TestCase): + """Shape 1: no ```verdict block anywhere -- "none found", not an error.""" + + def test_zero_blocks_resolves_to_none_found(self) -> None: + results = {"issue": _empty_fetch(), "review": _empty_fetch()} + resolution = resolve(results) + self.assertEqual(resolution.outcome, "none_found") + self.assertIsNone(resolution.accepted) + + +class OneBlockTests(unittest.TestCase): + """Shape 2: exactly one closed, well-formed block -- accepted.""" + + def test_one_well_formed_block_is_accepted(self) -> None: + tb = TaggedBlock(_well_formed_located(), comment_id=1, surface="issue", created_at="t1", position=0) + results = {"issue": CommentFetch(state="ok", blocks=[tb]), "review": _empty_fetch()} + resolution = resolve(results) + self.assertEqual(resolution.outcome, "accepted") + self.assertIsNotNone(resolution.accepted) + self.assertEqual(resolution.accepted.location.comment_id, 1) + self.assertEqual(len(resolution.accepted.rows), 1) + self.assertEqual(resolution.superseded, []) + + +class SameCommentDoubleBlockTests(unittest.TestCase): + """Shape 3: two WELL-FORMED, CLOSED blocks in one comment. + + Deliberately not a malformed pair -- a malformed pair would pass through + the catch-all malformed branch instead of proving the same-comment + branch specifically runs and wins ahead of the accept-last branch. + """ + + def test_two_well_formed_blocks_in_one_comment_are_refused(self) -> None: + tb1 = TaggedBlock(_well_formed_located(), comment_id=1, surface="issue", created_at="t1", position=0) + tb2 = TaggedBlock(_well_formed_located(), comment_id=1, surface="issue", created_at="t1", position=1) + results = {"issue": CommentFetch(state="ok", blocks=[tb1, tb2]), "review": _empty_fetch()} + resolution = resolve(results) + self.assertEqual(resolution.outcome, "refused") + self.assertIsNone(resolution.accepted) + self.assertEqual({loc.position for loc in resolution.refused_locations}, {0, 1}) + + +class DifferentCommentsRealFixtureTests(unittest.TestCase): + """Shape 4: two blocks across two DIFFERENT comments -- accept the last by + (created_at, comment_id), report the other superseded. Real fixtures: + the actual #261 and #264 double-block comment sets.""" + + def _resolve_recorded(self, pr: int): + items = _load_recording(pr) + return resolve({"issue": from_items(items, "issue"), "review": _empty_fetch()}) + + def test_pr_261_accepts_the_later_comment(self) -> None: + resolution = self._resolve_recorded(261) + self.assertEqual(resolution.outcome, "accepted") + self.assertEqual(resolution.accepted.location.comment_id, 5364261676) + self.assertEqual([loc.comment_id for loc in resolution.superseded], [5364185647]) + + def test_pr_264_accepts_the_blocker_promotion(self) -> None: + resolution = self._resolve_recorded(264) + self.assertEqual(resolution.outcome, "accepted") + self.assertEqual(resolution.accepted.location.comment_id, 5364504768) + self.assertEqual([loc.comment_id for loc in resolution.superseded], [5364221899]) + # The named promotion: row 1's severity moved High -> Blocker. + row1 = resolution.accepted.rows[0] + self.assertEqual(row1.severity, "Blocker") + + +class TieBreakTests(unittest.TestCase): + """The accept-branch sort key is (created_at, comment_id): created_at is + only second-resolution, so when two well-formed blocks in DIFFERENT + comments share an identical created_at, comment_id alone must break the + tie. Dropping comment_id from the sort key would leave this case + resolving arbitrarily (dict/list order) rather than deterministically to + the higher comment_id -- this is what proves it does not.""" + + def test_identical_created_at_breaks_tie_on_comment_id(self) -> None: + tb_lower = TaggedBlock( + _well_formed_located(), comment_id=100, surface="issue", created_at="2026-08-21T01:00:00Z", position=0 + ) + tb_higher = TaggedBlock( + _well_formed_located(), comment_id=200, surface="issue", created_at="2026-08-21T01:00:00Z", position=0 + ) + # Insertion order deliberately puts the higher comment_id FIRST, so a + # sort keyed on created_at alone (a stable sort) would pick the lower + # id -- the wrong answer -- rather than happening to pick the right + # one by list order. + results = { + "issue": CommentFetch(state="ok", blocks=[tb_higher, tb_lower]), + "review": _empty_fetch(), + } + resolution = resolve(results) + self.assertEqual(resolution.outcome, "accepted") + self.assertEqual(resolution.accepted.location.comment_id, 200) + self.assertEqual([loc.comment_id for loc in resolution.superseded], [100]) + + +class MalformedRowAnywhereTests(unittest.TestCase): + """Branch 3: any malformed row anywhere (here, in the second of two + otherwise-clean blocks in DIFFERENT comments) refuses the whole set, + naming every block's location -- not only the offending one. Network-free + twin of `check_verdict_resolution.py`'s equivalent live-control assertion, + per #287 STEP 5's MEDIUM finding: that assertion doesn't need network and + belongs in this suite too, not only inside a needs_network=True control.""" + + def test_malformed_row_in_second_of_two_different_comments_refused(self) -> None: + tb1 = TaggedBlock(_well_formed_located(), comment_id=1, surface="issue", created_at="t1", position=0) + tb2 = TaggedBlock(_malformed_located(), comment_id=2, surface="issue", created_at="t2", position=0) + results = {"issue": CommentFetch(state="ok", blocks=[tb1, tb2]), "review": _empty_fetch()} + resolution = resolve(results) + self.assertEqual(resolution.outcome, "refused") + self.assertIsNone(resolution.accepted) + self.assertEqual({loc.comment_id for loc in resolution.refused_locations}, {1, 2}) + + # review-final HIGH #1: the refusal must carry verdict_blocks.parse_rows's + # own specific reason for the offending row, not the fixed generic + # branch-level sentence alone -- and must NOT invent a reason for the + # well-formed sibling location that has nothing wrong with it. + by_comment = {loc.comment_id: loc for loc in resolution.refused_locations} + self.assertIn("not enough fields", by_comment[2].reason) + self.assertIn("need 4 or more", by_comment[2].reason) + self.assertEqual(by_comment[1].reason, "") + + +class ReviewSurfaceNeverAuthoritativeTests(unittest.TestCase): + """review-final HIGH #2: a well-formed, closed block on the "review" + (inline code-comment) surface must never be silently accepted, never + silently folded into cross-comment ordering, and never silently dropped + as if it were "none found". Both real fixtures (#261, #264) only ever + used the issue-comment surface -- see ADJUDICATION.md's #287 section.""" + + def test_review_only_block_is_refused_not_accepted_not_none_found(self) -> None: + tb = TaggedBlock(_well_formed_located(), comment_id=1, surface="review", created_at="t1", position=0) + results = {"issue": _empty_fetch(), "review": CommentFetch(state="ok", blocks=[tb])} + resolution = resolve(results) + self.assertEqual(resolution.outcome, "refused") + self.assertNotEqual(resolution.outcome, "accepted") + self.assertNotEqual(resolution.outcome, "none_found") + self.assertIsNone(resolution.accepted) + self.assertEqual([loc.comment_id for loc in resolution.refused_locations], [1]) + + def test_review_block_does_not_silently_lose_to_a_real_issue_block_either(self) -> None: + """A well-formed issue-surface block PLUS a later-created_at, + well-formed review-surface block: proves the fix does not silently + pick the "good" issue block while quietly discarding the review one + (that would be "silently ignoring it", which the fix must not do + either) -- the whole set is refused, and BOTH locations are named + (review-final Medium #4: a refusal naming only the objectionable + block would hide the real issue-comment block sitting in the same + set -- the partial picture DoD bullet 2 exists to prevent).""" + issue_tb = TaggedBlock( + _well_formed_located(), comment_id=1, surface="issue", created_at="2026-01-01T00:00:00Z", position=0 + ) + review_tb = TaggedBlock( + _well_formed_located(), comment_id=2, surface="review", created_at="2026-01-02T00:00:00Z", position=0 + ) + results = { + "issue": CommentFetch(state="ok", blocks=[issue_tb]), + "review": CommentFetch(state="ok", blocks=[review_tb]), + } + resolution = resolve(results) + self.assertEqual(resolution.outcome, "refused") + self.assertIsNone(resolution.accepted) + self.assertEqual([loc.comment_id for loc in resolution.refused_locations], [1, 2]) + by_id = {loc.comment_id: loc.reason for loc in resolution.refused_locations} + self.assertEqual(by_id[1], "") + self.assertNotEqual(by_id[2], "") + + +class QuotedAndIndentedLookalikeTests(unittest.TestCase): + """Shape 5: a fenced block inside a quoted-or-indented context that only + looks like a real verdict block -- must resolve as if it were not there.""" + + def test_blockquoted_fence_is_not_a_block(self) -> None: + body = ( + "Quoting an earlier reviewer's block:\n\n" + "> ```verdict\n" + f"> {ROW}\n" + "> ```\n\n" + "My own comment carries no verdict block of its own." + ) + located = locate_verdict_blocks(body) + self.assertEqual(located, []) + cf = from_items([{"id": 1, "created_at": "t1", "body": body}], "issue") + resolution = resolve({"issue": cf, "review": _empty_fetch()}) + self.assertEqual(resolution.outcome, "none_found") + + def test_indented_fence_is_not_a_block(self) -> None: + body = f"Prose.\n\n ```verdict\n {ROW}\n ```\n" + located = locate_verdict_blocks(body) + self.assertEqual(located, []) + cf = from_items([{"id": 1, "created_at": "t1", "body": body}], "issue") + resolution = resolve({"issue": cf, "review": _empty_fetch()}) + self.assertEqual(resolution.outcome, "none_found") + + +class UnreadableFetchTests(unittest.TestCase): + """Shape 6: an unreadable/absent comment fetch -- refused as "unreadable", + never rendered the same as a clean zero-block PR.""" + + def test_absent_surface_resolves_to_unreadable(self) -> None: + results = { + "issue": CommentFetch(state="absent", reason="gh timed out after 60s"), + "review": _empty_fetch(), + } + resolution = resolve(results) + self.assertEqual(resolution.outcome, "unreadable") + self.assertNotEqual(resolution.outcome, "none_found") + + def test_oversized_surface_resolves_to_unreadable(self) -> None: + results = { + "issue": _empty_fetch(), + "review": CommentFetch(state="oversized", reason="over the per-entry-point cap"), + } + resolution = resolve(results) + self.assertEqual(resolution.outcome, "unreadable") + + +if __name__ == "__main__": + unittest.main() diff --git a/launchpad/review-agent/verdict_blocks.py b/launchpad/review-agent/verdict_blocks.py new file mode 100644 index 00000000000..f4c4df89be8 --- /dev/null +++ b/launchpad/review-agent/verdict_blocks.py @@ -0,0 +1,206 @@ +"""Locate and parse fenced ```verdict blocks inside one PR comment's raw text. + +Implements launchpad-26/buzz#287 STEPs 2 and 3. See ADJUDICATION.md's +"PR comment verdict blocks: refusing more than one (#287)" section for the +Option-B rule this module is a building block of, and `verdict_resolution.py` +for the rule itself (STEP 5). + +STEP 2 — ``locate_verdict_blocks``. This module reuses +``launchpad/scripts/pr_body_check.py``'s ``FENCE_OPEN``/``FENCE_CLOSE`` +run-length-matching regexes for the fence boundary itself (CommonMark's rule +that a closing fence must be at least as long as the one that opened it, and +must close with the same character) — that matching logic is correct here +unchanged. What is NOT reused is `_strip_fences`'s blockquote disposition: +`_strip_fences` strips a `> ` prefix *before* matching, on purpose, so a fence +someone quoted is recognised as real (its docstring: "Without that, quoting +someone else's fenced output left its contents standing as prose"). This +module's job is the opposite — a PR comment quoting someone else's +` ```verdict ` block must NOT have that block treated as this commenter's +own verdict — so a fence line matching `BLOCKQUOTE` disqualifies that fence +from the returned list, without changing anything about how the underlying +run-length matching works. `_strip_fences` also only captures the backtick or +tilde run, never what follows it on the same line, so it cannot tell a +` ```verdict ` fence from a ` ```python ` one; this module captures that info +string and only ever returns blocks whose info string is the exact word +``verdict``. + +STEP 3 — ``parse_rows``. Mirrors `review-gate.sh`'s `cmd_verdict`: a row +needs 4 or more tab-separated fields, with everything from field 4 onward +rejoined as the description (a description containing a literal tab is legal +on the emitter side, per that script's own `cut -f4-`). ``verdict`` is +checked against ``verdicts.VERDICTS`` and ``severity`` against +``review.SEVERITY_ORDER`` — both imported, never redeclared, for the same +reason `verdicts.py` itself gives for importing `SEVERITY_ORDER` rather than +re-declaring it. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass + +from review import SEVERITY_ORDER +from verdicts import VERDICTS + +#: Same pattern as `pr_body_check.BLOCKQUOTE` — a run of one or more `>` markers, +#: each optionally preceded by up to 3 spaces, with one optional trailing space. +BLOCKQUOTE = re.compile(r"^(?: {0,3}>)+ ?") + +#: Unlike `pr_body_check.FENCE_OPEN`, this captures group 2: everything after the +#: backtick/tilde run on the opening line, so the info string (`verdict`, `python`, +#: or nothing) can be told apart. CommonMark's own indentation limit (0-3 leading +#: spaces) is unchanged, so a 4-space-indented fence still never matches. +FENCE_OPEN = re.compile(r"^ {0,3}(`{3,}|~{3,})(.*)$") + +#: Identical to `pr_body_check.FENCE_CLOSE` — a closing fence carries nothing but +#: its run and trailing whitespace. +FENCE_CLOSE = re.compile(r"^ {0,3}(`{3,}|~{3,})[ \t]*$") + +#: The one info string this locator returns blocks for. Case-sensitive and exact: +#: `` ```verdicts `` or `` ```verdict-old `` are unrelated fences, not this one. +_VERDICT_INFO = "verdict" + + +@dataclass +class LocatedBlock: + """One ` ```verdict ` fence found in a comment body, 1-indexed line numbers. + + No ``info`` field: every returned block's info string is already exactly + ``verdict`` (that's what qualifies it for the returned list at all -- see + ``locate_verdict_blocks``), so a stored copy would be produced and never + read for anything a caller couldn't already assume. + """ + + start_line: int + end_line: int | None # None when the fence never closes (runs to EOF) + closed: bool + raw_rows: str # the raw text between the fence lines; "" if none + + +def locate_verdict_blocks(text: str) -> list[LocatedBlock]: + """Every top-level ` ```verdict ` fence in ``text``, closed or not. + + A fence opened inside a blockquote is tracked (so its extent is understood + correctly, the same way `pr_body_check._strip_fences` tracks it) but is + NEVER returned, regardless of its info string or closed state — that is + the "disqualify" disposition STEP 2 requires, the mirror image of + `_strip_fences` recognising it. A fence opened outside a blockquote whose + info string is not exactly ``verdict`` is tracked the same way and also + never returned — it is a real fence, just not this one. + """ + blocks: list[LocatedBlock] = [] + lines = text.splitlines() + + fence_char: str | None = None + fence_len = 0 + fence_in_quote = False + start_line = 0 + info = "" + row_lines: list[str] = [] + + def flush(end_line: int | None, closed: bool) -> None: + if fence_in_quote: + return + if info.strip() != _VERDICT_INFO: + return + blocks.append(LocatedBlock(start_line, end_line, closed, "\n".join(row_lines))) + + i = 0 + n = len(lines) + while i < n: + line = lines[i] + lineno = i + 1 + + if fence_char is not None: + if fence_in_quote and not BLOCKQUOTE.match(line): + # CommonMark ends a block quote lazily: a line with no `>` marker + # closes it, and a fence opened inside keeps no memory of the + # container it opened in — it closes with the quote, unclosed. + flush(None, False) + fence_char = None + fence_in_quote = False + continue # re-evaluate this same line fresh, below + probe = BLOCKQUOTE.sub("", line) if fence_in_quote else line + closer = FENCE_CLOSE.match(probe) + if closer and closer.group(1)[0] == fence_char and len(closer.group(1)) >= fence_len: + flush(lineno, True) + fence_char = None + fence_in_quote = False + i += 1 + continue + row_lines.append(line) + i += 1 + continue + + quoted = bool(BLOCKQUOTE.match(line)) + probe = BLOCKQUOTE.sub("", line) if quoted else line + m = FENCE_OPEN.match(probe) + if m: + fence_char = m.group(1)[0] + fence_len = len(m.group(1)) + info = m.group(2) + fence_in_quote = quoted + start_line = lineno + row_lines = [] + i += 1 + + if fence_char is not None: + flush(None, False) + + return blocks + + +@dataclass +class ParsedRow: + verdict: str + severity: str + location: str + description: str + + +@dataclass +class MalformedRow: + """``reason`` alone carries the offending row's own text embedded in the + message (see the three call sites below) -- a separate ``raw`` field + would be produced and never read, so it isn't one.""" + + reason: str + + +def parse_rows(raw_rows: str) -> list[ParsedRow | MalformedRow]: + """Parse a located block's raw row text into ``ParsedRow``/``MalformedRow``. + + A blank line between rows is skipped, not flagged malformed — the same + disposition `review-gate.sh`'s `cmd_verdict` gives it (`[ -z "$row" ] && + continue`). + """ + results: list[ParsedRow | MalformedRow] = [] + for line in raw_rows.split("\n"): + if not line.strip(): + continue + fields = line.split("\t") + if len(fields) < 4: + results.append( + MalformedRow( + reason=f"{len(fields)} tab-separated field(s), need 4 or more: {line}", + ) + ) + continue + verdict, severity, location = fields[0], fields[1], fields[2] + description = "\t".join(fields[3:]) + if verdict not in VERDICTS: + results.append( + MalformedRow( + reason=f"verdict {verdict!r} not in {sorted(VERDICTS)}: {line}", + ) + ) + continue + if severity not in SEVERITY_ORDER: + results.append( + MalformedRow( + reason=f"severity {severity!r} not in {sorted(SEVERITY_ORDER)}: {line}", + ) + ) + continue + results.append(ParsedRow(verdict, severity, location, description)) + return results diff --git a/launchpad/review-agent/verdict_resolution.py b/launchpad/review-agent/verdict_resolution.py new file mode 100644 index 00000000000..fb9ecb424d8 --- /dev/null +++ b/launchpad/review-agent/verdict_resolution.py @@ -0,0 +1,248 @@ +"""Resolve one PR's comment set to zero-or-one authoritative ```verdict block. + +Implements launchpad-26/buzz#287 STEPs 5 and 7. The rule itself is +Option B, recorded in ADJUDICATION.md's "PR comment verdict blocks: +refusing more than one (#287)" section: no supersedes marker; the parser +deterministically takes the last complete, closed, well-formed block by +comment order, and refuses anything that does not reduce to exactly one +candidate that way. + +``resolve`` (STEP 5) takes `pr_comments.fetch_and_locate`'s per-surface +output and applies SEVEN branches, IN ORDER -- order is load-bearing, since +more than one branch can match the same input and the first match must +win: + + 1. any surface's comment fetch itself was unreadable -> "unreadable" + 2. two or more blocks within the SAME comment -> "refused" + 3. any other malformed case (a malformed row anywhere, + or an unclosed block, in any comment) -> "refused" + 4. a well-formed, closed block found on the "review" + (inline code-comment) surface -> "refused" -- + never silently accepted, never silently folded into the ordering below, + and never silently dropped as if it did not exist. Both #261 and #264 + (the only real production evidence Option B rests on) only ever used + the issue-comment surface; see ADJUDICATION.md's #287 section for the + scope decision this codifies -- only the issue-comment surface can + supply an authoritative block. + 5. zero blocks anywhere -> "none_found" + 6. exactly one closed, well-formed block -> "accepted" + 7. two or more closed, well-formed blocks, each in a + DIFFERENT comment -> "accepted", + picking the block with the highest ``(created_at, comment_id)`` pair + -- comment_id (monotonically increasing on GitHub) is the deciding + tie-break, since created_at is only second-resolution. By branch 4, + every block reaching here is already known to be on the issue surface. + +``resolve_verdict`` (STEP 7) is the one importable entry point that chains +`pr_comments.fetch_and_locate` and `resolve` for a live PR number -- see +its own docstring for the consumer contract. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field + +from pr_comments import CommentFetch, DEFAULT_REPO, TaggedBlock, fetch_and_locate +from verdict_blocks import MalformedRow, ParsedRow, parse_rows + +#: The four outcomes `resolve` can reach. Never any other string. +OUTCOMES = ("unreadable", "refused", "none_found", "accepted") + + +@dataclass(frozen=True) +class BlockLocation: + """Where one verdict block was found, and -- for a refusal -- why it + counts against the set. + + ``comment_id``/``surface``/``position`` (0-indexed within that comment) + satisfy STEP 5's naming requirement. ``reason`` carries `verdict_blocks. + parse_rows`'s own specific per-row reason string(s) when this location is + part of a malformed refusal (branch 3) -- "" everywhere else -- so a + refusal names the specific row and specific problem, never a single fixed + sentence standing in for every possible cause. + """ + + comment_id: int + surface: str + position: int + created_at: str = "" + reason: str = "" + + +@dataclass +class ResolvedBlock: + """The accepted block's own parsed content, alongside where it came from.""" + + location: BlockLocation + rows: list[ParsedRow] + + +@dataclass +class Resolution: + outcome: str # one of OUTCOMES + reason: str = "" + accepted: ResolvedBlock | None = None + superseded: list[BlockLocation] = field(default_factory=list) + refused_locations: list[BlockLocation] = field(default_factory=list) + + +def _location(tb: TaggedBlock, reason: str = "") -> BlockLocation: + return BlockLocation( + comment_id=tb.comment_id, + surface=tb.surface, + position=tb.position, + created_at=tb.created_at, + reason=reason, + ) + + +def _evaluate(tb: TaggedBlock) -> tuple[bool, list[ParsedRow], list[MalformedRow]]: + """(well_formed, parsed_rows, malformed_rows). well_formed requires closed + AND every row parses. ``malformed_rows`` carries `verdict_blocks. + parse_rows`'s own real ``MalformedRow`` objects -- never discarded -- + so branch 3 below can report their actual ``.reason`` strings. An + unclosed block has no trustworthy row text to parse (nothing between an + unterminated fence and EOF is a real row set), so it gets one synthetic + ``MalformedRow`` naming exactly that, rather than an empty malformed list + that would make it indistinguishable from "nothing wrong here yet". + """ + if not tb.block.closed: + return False, [], [MalformedRow(reason="block is never closed -- its fence has no matching closer")] + rows = parse_rows(tb.block.raw_rows) + malformed = [r for r in rows if isinstance(r, MalformedRow)] + if malformed: + return False, [], malformed + return True, [r for r in rows if isinstance(r, ParsedRow)], [] + + +def resolve(results: dict[str, CommentFetch]) -> Resolution: + """Apply the seven branches above to one PR's fetched-and-located comment set.""" + # Branch 1: the comment fetch itself was unreadable on any surface. Distinct + # from "none found" -- CONTAINMENT.md's "absence of evidence is never + # reported as evidence" -- so an unreadable fetch must never render the + # same as a clean zero-block PR. + unreadable_surfaces = [s for s, cf in results.items() if not cf.readable] + if unreadable_surfaces: + detail = "; ".join( + f"{s}: {results[s].state} ({results[s].reason})" for s in unreadable_surfaces + ) + return Resolution( + outcome="unreadable", + reason=f"comment fetch unreadable on surface(s): {detail}", + ) + + all_blocks: list[TaggedBlock] = [] + for cf in results.values(): + all_blocks.extend(cf.blocks) + + # Branch 2: two or more blocks within the SAME comment. Checked before the + # accept-last branch below, since a same-comment pair that is also + # individually well-formed would otherwise match both -- two fences posted + # in one write can't be a temporal amendment of each other, so Option B's + # ordering rule never applies to this shape. + by_comment: dict[tuple[str, int], list[TaggedBlock]] = {} + for tb in all_blocks: + by_comment.setdefault((tb.surface, tb.comment_id), []).append(tb) + same_comment_groups = [group for group in by_comment.values() if len(group) > 1] + if same_comment_groups: + locs = [_location(tb) for group in same_comment_groups for tb in group] + return Resolution( + outcome="refused", + reason="two or more ```verdict blocks were posted within the same comment", + refused_locations=locs, + ) + + # Branch 3: any other malformed case (a malformed row anywhere, or an + # unclosed block, in any comment) -- names every block's comment id, + # surface, and position, not only the offending one, and carries each + # malformed block's own specific reason string(s) (empty for a + # well-formed block named only because it shares the refused set). + evaluations = [(tb, *_evaluate(tb)) for tb in all_blocks] + if any(not well_formed for _, well_formed, _, _ in evaluations): + refused = [ + _location(tb, reason="; ".join(m.reason for m in malformed)) + for tb, _well_formed, _rows, malformed in evaluations + ] + return Resolution( + outcome="refused", + reason="a malformed row or an unclosed block was found in the comment set", + refused_locations=refused, + ) + + # Branch 4: a well-formed, closed block on the "review" (inline + # code-comment) surface. Refused outright -- never silently accepted, + # never silently merged into branch 7's cross-comment ordering (where a + # later-created_at inline annotation could otherwise silently outrank a + # real, complete issue-comment block), and never silently dropped as if + # it were not there (that would make it indistinguishable from branch 5's + # "none found"). See ADJUDICATION.md's #287 section. Names every block in + # the set, not only the review-surface one(s) -- same reasoning as branch + # 3: a refusal that shows only the objectionable block hides a real, + # complete issue-comment block sitting in the same set, which is the + # partial picture DoD bullet 2 exists to prevent. + review_well_formed = [ + tb for tb, well_formed, _rows, _malformed in evaluations + if well_formed and tb.surface == "review" + ] + if review_well_formed: + return Resolution( + outcome="refused", + reason=( + "a well-formed ```verdict block was found on the review " + "(inline code-comment) surface; only the issue-comment " + "surface can supply an authoritative block" + ), + refused_locations=[ + _location( + tb, + reason=( + "well-formed block on the review surface, which cannot " + "be authoritative" + ) + if tb.surface == "review" + else "", + ) + for tb, _well_formed, _rows, _malformed in evaluations + ], + ) + + # Branch 5: zero blocks found anywhere -- a distinguishable "none found", + # not an error. + if not evaluations: + return Resolution(outcome="none_found", reason="no ```verdict block in any comment") + + # Branches 6 and 7: every remaining block is closed, well-formed, on the + # issue-comment surface (by branch 4), and (by branch 2) the only block + # in its own comment -- so ordering by (created_at, comment_id) always + # resolves to a single winner, whether there is one block or several + # across different comments. + ordered = sorted(evaluations, key=lambda e: (e[0].created_at, e[0].comment_id)) + winner_tb, _winner_well_formed, winner_rows, _winner_malformed = ordered[-1] + superseded = [_location(tb) for tb, _wf, _rows, _mf in ordered[:-1]] + return Resolution( + outcome="accepted", + accepted=ResolvedBlock(location=_location(winner_tb), rows=winner_rows), + superseded=superseded, + ) + + +def resolve_verdict(pr: int, repo: str = DEFAULT_REPO) -> Resolution: + """The one entry point a future consumer calls: fetch PR ``pr``'s full + comment set, live, and resolve it to zero-or-one authoritative verdict + block per the Option-B rule this module's docstring states. + + **Consumer contract.** Two candidate future callers exist and neither + calls this today, per the #287 issue's own "fixing this now is cheap: + there is no consumer yet to migrate": #119's banner path (composes and + publishes its own review; does not currently read PR comments at all) + and #426's pre-review packet. Whichever wires this in owns checking + ``Resolution.outcome`` -- "unreadable" and "refused" both mean no + verdict may be treated as authoritative, "none_found" means no + adjudication has been posted yet, and only "accepted" carries a real + ``Resolution.accepted.rows`` list to act on. + + Return shape stability (smoke-level, not behavioural) is asserted by + `check_resolve_verdict_contract.py`. + """ + results = fetch_and_locate(pr, repo) + return resolve(results)