Make PR review verdicts deterministic and prior findings audited - #129
Merged
Merged
Conversation
## Problem Three connected failures in the shared PR-review action, all observed on ConductorOne/baton-axiomatic PR 249 after the Claude Code 2.1.280 / claude-opus-5-5 upgrade (#126): 1. The agent stopped submitting the formal gh pr review verdict — a full day of runs posted zero reviews, leaving PRs with a quiet summary comment and no blocking signal. The ductone sister repo hit the same regression with Claude Code 2.1.187 and fixed it by moving verdict submission into CI. 2. The agent does not emit the <!-- review-state --> marker reliably, so state detection failed closed: every run fell back to full review mode. 3. The prompt's 'skip anything already raised' rule plus 82 unresolvable stale threads (the Actions token is denied resolveReviewThread in every context tested, both orgs, public and private repos) let the reviewer dedupe everything into 'no new issues' — and a resolved thread was treated as if the underlying issue were fixed, which is not necessarily true. ## Change - submit-verdict-review.py (new, ported from ductone/github-workflows, judge/approve mode stripped): CI reads '**Blocking Issues: N**' from the HEAD-bound sticky summary and submits --request-changes (N>0) or --comment (N==0). Baseline mode only — this reviewer never approves. Fails closed (nonzero) when no bound verdict exists, so a broken review is a loud red check instead of silent green. - stamp-review-state.py (new, ported + adapted): CI stamps the full {last_reviewed_sha, base_sha, workflow_ref} marker with git HEAD after a successful agent step, so incremental mode and the verdict gate no longer depend on the model emitting the marker. - resolve-outdated-threads.py: also writes .github/prior-findings.json (every bot finding, resolved or not, with thread state), and stops retrying resolveReviewThread after the first 'Resource not accessible by integration' denial instead of burning ~30s per thread. - base-pr-review.md: mandatory prior-findings audit (Step 3) — thread state is not evidence of code state; each prior finding gets a still present / fixed / obsolete verdict derived from the current code, reported in a new 'Prior Findings Re-check' summary section; Blocking Issues counts confirmed still-present priors so a PR with an unfixed blocking issue stays blocked. Wall-clock budget: post a provisional summary before going deep, bounded sub-agent fan-out. Verdict duty moved to CI; the agent no longer runs gh pr review. - action.yml: wire the stamp + submit steps after a successful Claude step; harden claude_args (--setting-sources user --strict-mcp-config so the reviewed repo's agents/MCP/skills cannot hijack the review; hard-deny ScheduleWakeup/Cron* loop tools; drop Bash(gh pr review:*) and Skill from the allow-list). - _gh.py (new, ported): shared resilient GitHub REST/GraphQL helper used by the two new scripts. No permission changes: the workflow's existing pull-requests: write token submits reviews as before; the resolveReviewThread denial is worked around (read-only prior findings) rather than fixed with new credentials. ## Verification - python3 -m unittest discover -s .github/actions/pr-review/scripts -p 'test_*.py' — 44 tests pass (19 new: verdict parsing never approves, SHA binding, marker stamping fields, prior-findings collection, permission-denial short-circuit). - action.yml and pr-review.yaml parse as valid YAML. - End-to-end behavior cannot be exercised from this PR (the ruleset-required workflow runs from main); first post-merge run on a connector PR is the live check. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Addresses the gate review on #129 (all three P1s, both P2s, plus the inherited retry defect): P1-1 (stale summary laundering): the stamper and submitter now require the summary comment to have been created/updated at or after REVIEW_RUN_STARTED_AT (captured in the first action step, before any review work). A successful Claude step is no longer treated as evidence a final summary exists: stale comments fail closed and are never re-stamped into looking current, and a missing summary fails submission. Entry-point regressions cover successful-no-summary, stale summary, and foreign-workflow summary. P1-2 (PR-title count injection): the verdict is parsed from exactly one canonical count row ('**Blocking Issues: N** | **Suggestions: M** | **Threads Resolved: R**', line-anchored, closing bold required). Zero rows, multiple rows, malformed values ('0-2'), unclosed bold, and count-shaped text in the PR title / quotes / code blocks are all rejected. Both false-negative and false-positive title cases are tested through the submission entry point. P1-3 (provisional accepted as final): provisional summaries (the exact provisional line) are refused by both entry points — a successful-but-provisional-only run fails as incomplete and never submits either verdict; fetch-pr-context.py skips provisional comments when selecting review state, so provisional output can never advance last_reviewed_sha (state selection extracted as testable extract_review_state); the prompt now requires omitting the review-state marker from provisional posts. P2-4 (SHA-only no-op): the stamper canonicalizes the entire marker {last_reviewed_sha, base_sha, workflow_ref} — a marker with the right SHA but missing/wrong base or workflow fields is repaired, with a round-trip test through context extraction. P2-5 (no live-head binding): submission re-fetches the live PR head immediately before posting and fails closed if it moved during the run, and the review is submitted via POST /pulls/{n}/reviews with an explicit commit_id (the reviewed SHA) instead of gh pr review, which cannot carry a commit. A head-change-between-review-and-submit regression is covered. Retry defect (_gh.py, inherited from the ductone helper): a server Retry-After / rate-limit cooldown that exceeds the remaining budget now stops with the outage error instead of being shortened; the per-request timeout is bounded by the remaining budget and the budget is checked before every attempt. Also: the human-authored spoof fixture in the prior-findings test now carries a finding prefix, so the author filter (not the prefix filter) is what excludes it. Verification: python3 -m unittest discover -s .github/actions/pr-review/scripts -p 'test_*.py' — 73 tests pass (29 new/updated, including entry-point regressions for every gate). action.yml parses as valid YAML. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The r2 gate review reproduced a false clean review against the real submit entry point: a fresh/final/current-workflow summary whose official count row was malformed (0-2) — or entirely absent — plus a fenced example containing a canonical zero row was accepted and submitted as 'No blocking issues found.' COUNT_ROW_PATTERN scanned every line, so a fenced row could stand in for a missing/malformed official verdict; the existing code-block test only passed because a real row plus a fenced row tripped the duplicate guard. The verdict is now accepted only from exactly one canonical count row in its prescribed top-level position — the first non-empty line after the summary heading — with fenced code blocks stripped before parsing. A missing or malformed official row rejects the summary instead of searching examples or later sections for a replacement; duplicate-row, malformed-value, unclosed bold, and PR-title protections are unchanged. New entry-point regressions assert nonzero exit and zero POSTs for (a) no real row plus a fenced canonical row and (b) a malformed real row plus a fenced canonical row; parser-level tests cover a sole fenced row, a fenced row alongside a real row (real row stays authoritative), and an out-of-position row. Verification: python3 -m unittest discover -s .github/actions/pr-review/scripts -p 'test_*.py' — 77 tests pass. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The r3 gate review reproduced three false-clean variants against the real entry points, all rooted in _strip_code_fences() toggling on any line whose stripped prefix was three backticks: (a) a four-backtick block containing a triple-backtick line — the scanner toggled out early and exposed the fenced count as the metadata row; (b) a ' ```example' line treated as a closer although a closing fence may only have trailing whitespace; (c) a ~~~markdown fence never stripped at all, exposing a fake heading and count. parse_blocking_count() now operates only over top-level lines determined with CommonMark fence rules: openers and closers use backticks or tildes; a closer must use the same character, be at least the opening length, and have only whitespace after it; a backtick fence's info string may not contain a backtick. The owning heading is only ever searched among top-level lines, and the verdict still requires exactly one canonical count row as the first non-empty line after it — fence deletion can no longer manufacture an authoritative metadata position. Entry-point negative controls for all three reproduced variants assert exit 1 and zero review POSTs; parser-level tests cover the same boundaries plus the retained r2 cases. Verification: python3 -m unittest discover -s .github/actions/pr-review/scripts -p 'test_*.py' — 83 tests pass. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Two r4 findings:
P1 (leading tab accepted before a closing fence): the closer computed
indentation with lstrip(' ') (spaces only) but the delimiter with strip()
(tabs too), so a TAB + triple-backtick line closed the block and exposed
the fenced count as top-level metadata — reproduced as a false clean POST.
The closer is now an anchored grammar: 0-3 LITERAL leading spaces (a
leading tab is 4 columns, i.e. content), the matching delimiter repeated at
least the opening length, and only [ \t]* afterward. Entry-point cases for
tab and mixed space/tab indentation assert exit 1 and zero POSTs.
P2 (test-oracle weakness): the six r3 regression tests stayed green with
the broken scanner restored — their fixtures placed the malicious fence
AFTER the Review Summary section, so the positional guard rejected the
exposed row regardless of scanner correctness. The fixtures now place the
fence in the metadata slot (or the fake heading ahead of the real summary
for the tilde variant), so a naive toggling scanner WOULD promote the
fenced row into the official position. Mutation-verified locally: the r3
naive-toggling scanner fails all 10 fence/tab tests, and the r4 tab-closer
bug fails exactly the 4 tab tests; the fixed scanner passes all 87.
Verification: python3 -m unittest discover -s .github/actions/pr-review/scripts
-p 'test_*.py' — 87 tests pass; both mutants above fail as named.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
jugonzalez12
approved these changes
Sep 24, 2026
gontzess
added a commit
that referenced
this pull request
Sep 24, 2026
## Summary - Publish a new full report at review completion instead of repeatedly editing a buried completed summary. - Keep completed reports separate from model-owned working output. Preserve the previous report on incomplete or failed attempts. - Link the explicit commit-bound formal review to the new report, then mark publication complete and collapse only older owned output while retaining its body and permalink. The baseline policy is unchanged: blockers produce `REQUEST_CHANGES`; zero blockers produce neutral `COMMENT`, never `APPROVE`. Model/action versions, named-secret wiring, permissions and logging boundaries are unchanged. ## Publication safety Publication is an ordered multi-request operation, not an atomic transaction. Reports remain pending until their required formal review exists. Same-attempt retries reconcile the exact report, reviewed commit and submitted result; they do not recreate deleted/dismissed historical reviews or retire a newer report. Ambiguous creation responses are reconciled after a single transport attempt rather than blindly retried. Completion retains the original reviewed snapshot. Older reportless attempts cannot reclaim output already consumed by a later completed review; actual attempt-start time, not numeric run IDs, determines obsolescence. A formal review may already exist if the subsequent completion-marker update fails. The report remains pending, prior useful output remains, and a same-attempt retry can finish the transition. Cleanup failures are non-destructive warnings. New comment/review creation provides normal GitHub activity; inbox/email delivery still depends on subscription settings. ## Verification - Focused action suite: **134 tests passed**. - Independent mutation audit: **89 added/changed tests** have recorded breaking mutations; removed change-detector tests are excluded. - Stateful publisher smoke: **38 checks passed**, including response-loss recovery, replay, explicit commit/event/link binding and preservation before replacement. - Independent source review accepted the corrected candidate, including replay ownership, Unicode headings and original-base preservation. - Action YAML parses successfully. - Final-candidate consumer validation confirmed a new full report, explicit reviewed-commit link, matching commit-bound `REQUEST_CHANGES`, and post-success collapse of the previous report and exact working comment. - A stale-head rerun failed before the agent ran: no new comment or host-owned verdict, and the existing completed report remained byte-identical. - GitHub-rendered HTML confirmed the report links and collapsed prior body. An authenticated browser session was unavailable, so no visual or notification-delivery proof is claimed. - Cancellation experiments raced publication; a cancelled run is not evidence that no review was published. The failed-attempt preservation claim uses the separate stale-head refusal above. Retains the review-quality and prior-finding safeguards delivered in #129. ### Rewritten-history regression follow-up Real temporary Git histories and mocked GitHub comparison boundaries cover changed-base rebases, same-base squash/amend/force-push comparisons, unavailable prior commits, retained bot findings, and stale checkouts. The existing full-review fallback policy is unchanged. New controls were mutation-verified, including under an inherited `PR_HEAD_SHA` to prevent a masked checkout test. Previously local transport response-loss tests are now included in the branch. No reviewer runtime logic changed in this follow-up. Current GitHub checks are self-review skips; the test counts above are local execution evidence, not claims that skipped checks ran tests. --------- Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
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.
Problem
After the reviewer upgrade, substantive connector review could stop at “no new issues” while unfixed prior findings remained. The agent also could not reliably publish the machine-readable state or formal review needed to make that conclusion visible.
Changes
ANTHROPIC_API_KEYsecret and an optional single-line summary heading. Reserved heading values cannot be embedded in an override.Validation
A controlled live recovery check reused the same comment ID after an interrupted markerless summary, performed a full review without provisional state, and stamped the completed result without creating another summary.