feat(review-agent): refuse a PR comment set carrying more than one verdict block

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/<n>/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 `<id>`", 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.

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())

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())