feat(review-agent): refuse a PR comment set carrying more than one verdict block - #1762
Merged
Conversation
STEP 1 of the #287 plan. Cites PR #261 (5364185647/5364261676) and PR #264 (5364221899/5364504768) as the real production evidence behind taking the last complete, closed, well-formed verdict block by comment order, with no new supersedes-marker syntax. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Reuses pr_body_check.py's FENCE_OPEN/FENCE_CLOSE run-length matching for the fence boundary, inverted on blockquote disposition: a quoted fence disqualifies rather than counts. Captures the info string so ```verdict is told apart from an unrelated fence, and reports closed/unclosed explicitly. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Note: parse_rows/ParsedRow/MalformedRow (the STEP 3 implementation) landed in the prior STEP 2 commit alongside verdict_blocks.py's locator, since both were written into the same file before the first commit. This commit is only the STEP 3 control script; the implementation diff is not isolated to its own commit. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…#287 STEP 4) RUNS HERE. fetch_and_locate() borrows fetch.py's --paginate --slurp incantation and UNREADABLE/CAP_PER_ENTRY_POINT model, applied per surface (issue vs review comments) rather than to one joined string, keeping each comment's id, created_at, and per-comment block position intact. Run live against #261 and #264: PR #261 issue surface -> blocks tagged comment_id 5364185647, 5364261676 PR #264 issue surface -> blocks tagged comment_id 5364221899, 5364504768 An invalid PR number reports state=absent, distinct from zero comments. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
resolve() applies the six ordered branches from ADJUDICATION.md's #287 section: unreadable fetch -> same-comment multi-block -> any other malformed/unclosed -> zero found -> one well-formed -> accept the highest (created_at, comment_id) across different comments, reporting the rest superseded. Verified live: PR #261 resolves to comment 5364261676 (5364185647 superseded); PR #264 resolves to comment 5364504768, the Blocker promotion (5364221899 superseded). Registers the two new network controls in run_controls.py. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…(STEP 6) test_verdict_resolution.py: zero blocks, one block, two well-formed closed blocks in one comment (same-comment refuse, not the malformed catch-all), two blocks across two different comments, a blockquoted/indented lookalike fence, and an unreadable/absent comment fetch -- one test class per shape, nine test methods total. The two-different-comments shape uses PR #261 and #264 as real fixtures (fixtures/verdict_blocks/, PROVENANCE.md + generate.py + recordings/ FALSIFIABILITY.md, mirroring fixtures/adjudication/'s convention), replayed through the real pipeline via pr_comments.py's new from_items() rather than hand-written stand-ins -- fetch_and_locate() is refactored to call the same helper, so live and recorded paths share one code path. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…287 STEP 7) Note: resolve_verdict() itself (the STEP 7 entry point) was already written into verdict_resolution.py in the STEP 5 commit, since STEP 7 depends on STEP 5's return shape and both were authored together in that file before STEP 5's first commit. This commit adds the STEP 7 control (check_resolve_verdict_contract.py) that STEP 7's own done-when requires: a smoke-level guard asserting resolve_verdict's signature, the Resolution/ ResolvedBlock/BlockLocation dataclass shapes, and the OUTCOMES constant stay stable, plus that the docstring names both candidate future callers (#119's banner path, #426's pre-review packet) and states neither calls it today. Registers the new no-network control in run_controls.py. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…es (#287) review-tests findings on task/287-verdict-block-refusal: HIGH: no test exercised the accept-branch sort key's comment_id tie-break when two well-formed blocks in different comments share an identical created_at. Added TieBreakTests, with insertion order deliberately reversed (higher comment_id first) so a stable sort on created_at alone can't pass by accident. Verified this is a real catch: dropping comment_id from resolve()'s sort key made this test fail (accepted comment 100 instead of 200); restoring the key made it pass again. MEDIUM: the malformed-row-anywhere branch's only prior test lived inside check_verdict_resolution.py's needs_network=True main(), despite not needing network itself. Added the equivalent case (MalformedRowAnywhereTests) to this network-free suite, which already runs the same shape for same-comment-refuse and unreadable-fetch-refuse. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…ace checks (#287) review-code findings on task/287-verdict-block-refusal: LOW: pr_comments.degrade() (STEP 4's stated alternate way to force "forced-unreadable") was never called from anywhere. Wired it into a real --degrade CLI flag on pr_comments.py's _main (mirroring contain.py/ run_dimensions.py's own --degrade shape: fetch first, then apply forced- state overrides), plus a new check_pr_comments.py control that exercises degrade() directly against a live PR #261 fetch. LOW: verdict_resolution.py redeclared DEFAULT_REPO as a literal instead of importing pr_comments.DEFAULT_REPO (which itself imports fetch.DEFAULT_REPO for the same reason) -- inconsistent with this file's own stated principle for VERDICTS/SEVERITY_ORDER. Now imported, not redeclared. LOW: check_pr_comments.py's STEP 4 live control only asserted on results["issue"]; results["review"] was fetched but never checked, so the control couldn't actually prove PR #261/#264 have EXACTLY the two known blocks. Added readable + zero-blocks assertions for the review surface on both PRs. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…blocks (#287) review-final findings on task/287-verdict-block-refusal: HIGH #1: _evaluate() discarded verdict_blocks.parse_rows's real MalformedRow objects on any malformed row, so branch 3's refusal could only say the fixed generic sentence for every cause. _evaluate now returns the actual MalformedRow list; branch 3 threads each block's own .reason string(s) onto a new BlockLocation.reason field, so a refusal names the specific row and specific problem (field count, unknown verdict, unknown severity), not just "comment id, surface, position". Verified: a malformed-row refusal now carries "not enough fields"/"need 4 or more" in its reason, while its well-formed sibling location's reason stays "". HIGH #2: both surfaces (issue + review/inline) were merged into one (created_at, comment_id) ordering, so a later inline code comment carrying a well-formed one-row fence could silently outrank -- and get reported as superseding -- a real, complete issue-comment block. Neither #261 nor #264 (the only real evidence Option B rests on) ever used the review surface. Added branch 4: any well-formed, closed block on the "review" surface now refuses the whole set outright -- never accepted, never folded into the ordering, never silently dropped. Mutation-verified: removing this branch made both new ReviewSurfaceNeverAuthoritativeTests cases fail (accepted instead of refused); restoring it passes again. MEDIUM #3: added a reconciliation sentence to ADJUDICATION.md's #287 section -- "more than one block" refuses for same-comment/malformed sets, and only resolves by ordering (not a guess) when every candidate is individually well-formed in different issue-surface comments. MEDIUM #4: dropped LocatedBlock.info and MalformedRow.raw (produced, never read anywhere -- grep-confirmed). Propagated LocatedBlock.start_line/ end_line onto BlockLocation instead of leaving start_line unread, so a refusal can point at the actual fence line within a comment. ADJUDICATION.md also gains the #287 scope decision itself (finding #2): both surfaces are fetched for detection, only the issue-comment surface can supply an authoritative block -- resolving the plan's OPEN item on review- line comment scope. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…ace names (#287) review-final MEDIUM #5: pr_comments.py calls the two comment surfaces "issue"/"review"; fetch.py (same directory, same two GitHub endpoints) calls them "pr_issue_comments"/"pr_review_comments" -- the vocabulary contain.py/run_dimensions.py's own --degrade flags already use. An operator who knows that convention and tried --degrade pr_issue_comments=absent here got an uncaught ValueError traceback instead of a normal refusal. Added _SURFACE_ALIASES/_normalize_surface so degrade() accepts either spelling (short names stay canonical everywhere else -- SURFACE_ENDPOINTS keys, TaggedBlock.surface, the results dict resolve() consumes -- rather than a wider rename for cosmetic gain), and wrapped _main's --degrade loop so a genuinely unknown surface/state still exits cleanly via parser.error (exit 2) instead of an uncaught traceback. Verified both spellings reach the identical forced state against a live PR #261 fetch. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
- branch 4's refusal now names the whole evaluated set (both the review- surface block and any real issue-comment block sitting alongside it), mirroring branch 3 -- a refusal naming only the objectionable block hid a real block in the same set, against DoD bullet 2 (Medium #4) - drop BlockLocation.start_line/end_line -- produced, never read, and the docstring claiming they were read was itself the defect (Medium #3) - module docstring's branch 5 said "zero well-formed blocks"; the check is "zero blocks", already-well-formed being guaranteed by branches 2-4 not having triggered (Low #7) - pr_comments.py's --degrade now accepts "empty" alongside fetch.UNREADABLE's three states, since a surface with zero comments is READABLE and forcing that shape shouldn't need a real PR that happens to have none (Low #6) - plan and ADJUDICATION.md's #287 sections no longer contradict the code on whether ordering is "merged across both surfaces" -- it isn't, since #287's own review-surface-refusal branch removes review-surface blocks from contention first; the plan's OPEN item on comment-surface scope is marked resolved with the reasoning (High #1) Findings 6 (resolve_verdict has no caller) and 7 (a commit message) from the first review-final pass, and this pass's own re-raised "no gating ledger" finding, are deliberately not addressed here -- see the PR description for why each is accepted rather than fixed on this branch. Filed #1761 for the third comment surface (pulls/{pr}/reviews) gap this pass also found. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…-block-refusal Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com> # Conflicts: # launchpad/review-agent/run_controls.py
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed commit 1543e903036ba11bc1346f5d2081b507b280df37 against merge base dea96f837690703b1298ab3f0e49e1b82fecb8cf.
Incomplete
This review is INCOMPLETE and must not be read as a full pass:
- no dimension was actually reviewed: the pipeline ran the 'default_reviewer' stub reviewer, which reports every dimension clean without reading it (a real dimension reviewer is #116)
Containment
No containment findings.
Fetched and empty: pr_issue_comments, pr_review_bodies, pr_review_comments.
Automated containment covers the delimiter boundary and unambiguous injection tells only. It does not cover injection phrased as ordinary, unremarkable prose. The absence of a containment finding is not evidence that this pull request contains no injection attempt.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a parser and resolution rule so the review-agent's consuming side never guesses when a PR's comment set carries more than one machine-parseable
```verdictblock. Two real production cases motivated this (PR #261, #264): each had two blocks from the same reviewer in two separate comments, where the second silently superseded a row in the first, with nothing in the format saying which was authoritative. Addsverdict_blocks.py,pr_comments.py, andverdict_resolution.pyunderlaunchpad/review-agent/, plus the decision record inADJUDICATION.md.Related issue
Closes #287
Issue type
Task
Agent provenance
Objective
A parser + resolution rule (
verdict_resolution.resolve_verdict) that reduces one PR's full comment set to zero-or-one authoritative```verdictblock, refusing rather than guessing whenever the set is ambiguous.Impacted components
launchpad/review-agent/verdict_blocks.pylaunchpad/review-agent/pr_comments.pylaunchpad/review-agent/verdict_resolution.pylaunchpad/review-agent/ADJUDICATION.mdlaunchpad/review-agent/test_verdict_resolution.pylaunchpad/review-agent/check_verdict_blocks.pylaunchpad/review-agent/check_verdict_rows.pylaunchpad/review-agent/check_pr_comments.pylaunchpad/review-agent/check_verdict_resolution.pylaunchpad/review-agent/check_resolve_verdict_contract.pylaunchpad/review-agent/run_controls.pylaunchpad/review-agent/fixtures/verdict_blocks/launchpad/plans/2026-08-27-issue-287-verdict-block-refusal.mdApproach and rejected alternatives
Chose Option B (no supersedes-marker syntax; a reviewer amends by re-posting the complete block, and the parser deterministically takes the last complete/closed/well-formed one by
(created_at, comment_id)) over Option A (require reviewers to emit a new explicit "supersedes" marker before a second block is accepted as an amendment). Rejected Option A because neither real production double-block case (#261, #264) used any such marker — both were full restatements — so Option A would require a reviewer-side format change with no existing evidence anyone emits it, where Option B needs none. This was surfaced as a real fork and the human (Serina) chose Option B explicitly rather than my picking silently.During review, also decided: a well-formed block found on the inline/review-comment surface can never be authoritative (always refuses) rather than being merged into the ordering — the plan's original design merged both surfaces, but review-final's first pass found a concrete silent-data-loss scenario (a later-created_at inline snippet could silently outrank and supersede a real, complete issue-comment block). Narrowed the scope in the safer direction rather than inventing a new marker for that case too, since both real production examples only ever used the issue-comment surface.
Verification
Command run:
Raw output:
The two SKIPs are pre-existing and unrelated to this change (they only run inside the actual publish workflow).
check_pr_comments.pyandcheck_verdict_resolution.pyboth run live against the real GitHub API and reproduce PR #261/#264's actual comment resolution.Not verified
Did not run the desktop, web, or mobile test suites — this change touches only
launchpad/review-agent/, a pure-Python CLI tool with no relationship to those surfaces. Did not independently re-verify thatpulls/{pr}/reviews(a third GitHub comment-bearing surface, distinct from the two this PR scans) carries no verdict blocks beyond the sample of PRs review-final checked (#261, #264, #267, and the 40 most recent PRs in this repo) — filed as follow-up #1761 rather than exhaustively checked. Did not run this against any PR outsidelaunchpad-26/buzz.Security implications
None. This is a read-only parser over PR comment text already fetched via the existing
gh apicredential path (no new scopes, no write calls); it produces a resolution value for a future consumer to act on and does not itself publish, approve, or gate anything.Escalations
verdict_resolutionnever scans the third GitHub comment-bearing surface (pulls/{pr}/reviews, i.e. a review submitted with a body). No real verdict block uses this surface today, so filed as a follow-up rather than blocking this merge.verdict_resolution.resolve_verdictas the entry point either would need if it starts reading reviewer verdicts from PR comments. Neither wires it in today; this PR only builds the parser, per the issue's own scope.review-tests/review-docsroster auto-routing gap (.review/*/rostercan't route the new fixture JSON files and names a nonexistentreview-docsagent) — pre-existing infrastructure outside this branch's file scope, first disclosed byreview-code's pass on this branch.review-plan(1 Blocker/3 High/4 Medium/2 Low, applied),review-code+review-tests(3 Low/1 High/1 Medium, applied), and two independentreview-finalpasses (2 High/4 Medium/1 Low, then 2 High/3 Medium/2 Low, both fully applied) before this PR opened. The firstreview-finalpass caught a real silent-data-loss defect (see Approach above) that no earlier gate found.