Fix: Review gate could confidently reject a diff the PR never contained - #617
Conversation
… swap the diff The review action's own session setup re-fetches the base branch at limited depth after every workflow guard has passed. When the base advanced since checkout, that fetch force-moves origin/<base> and shallow-grafts it, so the session's merge-base lookup fails and an improvised two-dot diff reports other merged PRs' changes as this PR's reverts (measured on PR #614, runs 31497107005 and 31504414058 — one produced a confident REJECT citing files the PR never touched). - prepare.py resolves the merge base while history is intact, records it in skip-decision.json and the step output, and fails closed (infrastructure error, not a verdict; no output files, so no patch-id/verdict marker can be cached) when none resolves - the session prompt pins the SHA (git diff <sha> HEAD walks no ancestry, immune to the graft) and forbids diffing against origin/<base> by name in any form, for orchestrator and specialists - the ensure-merge-base step retries with a full-depth base fetch and exits 1 instead of warning - separate gap fixed: the PR title/description now reach the session deterministically at the top of discussion-context.txt (both broken runs issued no gh command and told specialists no description existed) Discriminating test: a repo whose origin/main is severed from HEAD's history makes prepare.main() abort with no files written; without the fix it exits 0 and the pipeline proceeds diffless.
- prompt: stop promising full history — when the PR is up to date with its base the graft lands on HEAD's own ancestry and blame/log truncate silently; teach detection via cat .git/shallow (already in allowedTools) and demote premise-audit conclusions crossing the boundary - prompt: the STEP 1 specialist checklist now hands down discussion-context.txt (the #614 failure was the orchestrator telling specialists no description existed) - ensure step: the repair fetch deepens with --unshallow when a shallow boundary still exists (a plain fetch into a shallow repo stops at the boundary) - prepare.py: surface git's stderr on the fail-closed merge-base path - tests: prepare's git subprocesses now run under the same scrubbed git config as the fixture's (kills a global-diff-config flake class); structure tests cover the new prompt and repair properties
- the pr-description item can never clear, downgrade, or pre-empt a finding — it is the reviewed artifact and is rewritable after a REJECT; clearing power belongs to discussion items only (orchestrator context paragraph + STEP 2, with a structure test) - item bodies in discussion-context.txt are indented four spaces; only the pipeline writes at column zero, so a body imitating an '[issue-comment] maintainer ...' envelope line is visibly body text (fence header states the rule; forgery test added) - untrusted-data notice now names the PR description and the envelope-imitation rule - the graft caveat covers git log's commit listing, not just path-limited log/blame - the ensure step reports the repair fetch's own stderr in the fail-closed annotation instead of blaming history corruption for an auth/network/renamed-ref failure
Review round 3 caught a fail-open introduced in round 1: telling the
session to report a failed pinned diff 'in the diagnostics section'
routes it into prose no script reads — the session submits empty
findings and validate.py computes empty findings as APPROVE, greening an
unreviewed PR. A fabricated finding instead would compute as a REJECT
the patch-id skip re-asserts forever. Neither is acceptable, so:
- the session writes review-result.json as {"infrastructure_failure":
"<why>"}; validate.py checks the key before any schema validation and
exits non-zero with no verdict — nothing posted, no marker cached, the
Stop hook satisfied by the parsed file
- specialists on a failed pinned diff still write empty findings files
(releasing the hook) and escalate in their returned summary
- the graft caveat (cat .git/shallow) and the description-cannot-clear
rule now reach the specialist checklist and the fence header the
specialists actually read, not just the orchestrator paragraphs
- the initial --unshallow fetch is guarded so a transient failure reaches
the annotated fail-closed exit instead of dying bare under bash -e
- findings.schema.json gains an optional infrastructure_failure field; validate.py fails closed when any specialist file carries it — the escalation no longer depends on the orchestrator relaying prose from a subagent summary (two empty findings files otherwise compute APPROVE on a PR nobody reviewed) - the orchestrator's one blessed fallback for a failed pinned diff is gh pr diff (same merge-base diff, computed server-side); specialists run no gh and go straight to the field - collapse newlines in the ensure step's captured merge-base stderr so a multi-line git message cannot truncate the annotation before its 'NOT a verdict' sentence - prepare.py enforces the published hex-or-empty contract on the merge-base SHA it exports (a stray newline would inject extra GITHUB_OUTPUT entries) - docs: failure-mode count (five), specialist channel, blessed fallback
…fetch Review round 5 polish (no High/Medium findings remained): - the findings schema rejects infrastructure_failure alongside findings (self-contradiction: either the review happened or it didn't), so the channel cannot discard a run over a subordinate failure the specialist reviewed through - specialist infra reports preempt later validation errors, keeping the precise cause as the last error line the workflow annotates - a failed PR-description/discussion fetch emits a ::warning:: annotation instead of a plain log line (direction unchanged: fail-toward-review)
The pinned-SHA and description additions pushed the inline prompt scalar
to ~23.6k chars. Any ${{ }}-bearing scalar compiles into one format()
expression with a hard 21000-char source cap, and an over-cap file
reports 'Invalid workflow file' — no job runs, and merging it would
brick the required check on every PR until an admin merge (caught by
this branch's throwaway push workflow, run 31515805871).
The prompt now travels as a step OUTPUT: a compose step writes the
template (a quoted heredoc — no expression, no cap), substitutes the
four dynamic values (each a tiny env expression; MERGE_BASE stays
hex-enforced by prepare.py) via placeholder replacement, and the session
step's prompt: input is the one small expression reading that output.
Structure tests retarget to the template and two new guards keep the
class dead: the template must contain no inline ${{ }}, and a sweep
fails any expression-bearing scalar in any workflow at 19000 chars with
a named reason instead of a push-time parse error.
Its runs (31515807299, 31516462128) verified on ubuntu: the graft reproduction with pinned-SHA immunity, the real ensure-step block repairing a shallow clone and failing closed (annotated) on disjoint history, and the real prepare.py recording merge_base on a healthy checkout and aborting with no output files on a severed one. It also caught the 21000-char expression-cap regression before merge.
This comment has been minimized.
This comment has been minimized.
…step in tests Review gate findings (both MEDIUM, both in the compose step): - the post-substitution assert covered only MERGE_BASE and PR_NUMBER; it now loops over the same tuple the substitution uses, so a renamed or typo'd placeholder cannot ship literally into the prompt - the compose step now has subprocess-executed tests (same pattern as the validate-step cases): a real run must substitute every placeholder, emit a well-formed multiline GITHUB_OUTPUT heredoc, and refuse an empty substitution value rather than composing 'git diff HEAD'
This comment has been minimized.
This comment has been minimized.
…ma null - reword the seven occurrences of a banned word across validate.py, tests, and docs (schema-declared / sanctioned fallback) - the ensure-merge-base step gets real bash -e execution tests against real git fixtures (shallow-repairable, healthy-complete, and severed-with-dead-remote — the last asserting the single annotated infrastructure error names the repair fetch's own failure), mirroring the compose step's execution tests - infrastructure_failure is nullable like every optional sibling field, and the schema's findings-must-be-empty conditional triggers only on a real string — a model's null-for-absence reads as absence
This comment has been minimized.
This comment has been minimized.
Reconciles this branch's merge-base pinning with main's #616 (unknown-key stripping + log-text sanitizer): - validate.py: keep the infra-preempt block; route its prints (which interpolate model-written text) through main's _sanitize_log_text choke point - prompt template: port main's seven-keys-only STEP 1 sentence, amended to name infrastructure_failure as the one additional legal top-level key (this branch's cannot-review channel) - structure tests: keep both suites; main's prompt-key-list test reads the compose-step template, where the prompt now lives - test_validate.py: main's file plus this branch's appended infrastructure-failure sections
This comment has been minimized.
This comment has been minimized.
…hema too Review finding: the schema's if/then fired on any string value including '' and whitespace, while infrastructure_failure_message() treats those as no-report — so a blank key alongside real findings failed the file at schema validation (safe direction, but it discards a lens's genuine findings over a key that says nothing). The if-condition now requires substance (pattern \\S), matching the reader's strip-based definition exactly; parametrized test covers '' and whitespace
This comment has been minimized.
This comment has been minimized.
Review finding: the production self-check re-walked the same keys tuple the substitution consumed, so a placeholder renamed or added in the template without updating the tuple would ship its raw token into the prompt of a required check — the generic scan existed only in a test suite that is not among the required contexts. The step now fails closed on any leftover __[A-Z_]+__ token itself, and a mutation test renames one template token to prove the guard fires
|
✅ Looks good This PR fixes a real production bug in the review pipeline itself: the merge gate could confidently REJECT (or APPROVE) a PR based on the wrong diff, because Given this is exactly the kind of guard/safety-shaped change where the real risk lives in unverified premises about existing code, both specialists ran a full premise audit rather than just reading the new logic: merge-base pinning happening before the graft, This review's own environment reproduced the exact failure this PR fixes: this checkout's One MINOR item, tucked below, and no HIGH/MEDIUM findings survived merge — nothing filtered out as invalid. Minor findings
Finding dispositions
Review diagnosticsThis session's own Posted by the claude-review check — the review of this PR's diff at patch-id |
1 participant
What's broken
The
claude-reviewmerge gate can review the wrong diff and publish a confident verdict about it. On PR #614 (docs-only:.github/PULL_REQUEST_TEMPLATE.md+ oneCLAUDE.mdline), run 31504414058 REJECTed with findings about "unexplained partial reverts" ofSources/TBDApp/Terminal/ChildReaper.swiftand terminal-idempotency code — files that appear nowhere in the PR's diff. Run 31497107005 approved only because the model noticed the breakage and improvised around it. Both sessions also reported having no PR description available.Why it happens
The workflow's own guards all held.
actions/checkout(fetch-depth: 0) fetched full history; the "Ensure a merge-base" step verified merge base8b9b9b8in both broken runs;prepare.pycomputed the patch-id from the correct three-dot diff seconds later. The damage happens after every guard, inside theclaude-code-actionstep: the action's session setup re-fetches the base branch at limited depth (it logsRestoring .claude, ... from origin/mainthen a fetch ofmain -> FETCH_HEAD). When main advanced between checkout and session start — minutes apart, on a repo with heavy merge traffic — that fetch force-movesrefs/remotes/origin/mainto the new tip and records it in.git/shallow: a shallow graft that severs the ref's ancestry in a previously complete clone.From then on, inside the session:
git merge-base origin/main HEAD→ nothing;git diff origin/main...HEAD→fatal: no merge basegit diff origin/main HEAD"succeeds" — against a newer main — and reports every other PR merged in the interval as if this PR reverted it (run 31504414058, 14:58:13: the stat listed 14 files, 12 of them main-side)Reproduced locally and on an ubuntu runner:
git fetch --depth=1 origin maininto a complete clone after the remote moved grafts it exactly this way. A SHA-addressed diff (git diff <merge-base-sha> HEAD) survives, because it walks no ancestry and both endpoints are local objects.Worse,
prepare.pyhad recorded the patch-id of the correct diff, so the fabricated REJECT was cached against the correct diff's identity —gh pr update-branchthen re-asserted it in 8s without reviewing.The missing PR description is a separate gap with the same fingerprint: nothing hands the description to the session deterministically, and in both runs the session never issued a single
ghcommand — the orchestrator told its specialists no description existed.When it broke
Latent since the fan-out gate (and the single-session gate before it) started running review sessions through
claude-code-actionwith a mid-step base re-fetch; it fires whenever the base branch advances between checkout and session start, so it surfaced as merge traffic increased. First observed misverdict: PR #614, 2026-08-11.What this PR does
Pins the diff basis.
prepare.pyresolvesgit merge-base origin/<base> HEADwhile history is intact, records it inskip-decision.jsonand the step output, and computes the patch-id over it (same trees as the three-dot diff, so patch-ids stay comparable with existing markers). The session prompt hands the literal SHA to the orchestrator and to each specialist, and forbids diffing againstorigin/<base>by name in any form — three-dot, two-dot, or two-argument.Fails closed on a missing merge base — as infrastructure, not a verdict.
prepare.pyaborts non-zero without writing any output file, so the job fails before the session and no patch-id/verdict marker is ever recorded — an aborted run cannot cache a verdict, and the next run reviews fresh. The "Ensure a merge-base" step's::warning::branch becomes repair-then-exit 1(with an--unshallow-aware retry fetch, since a plain fetch into a still-shallow repo stops at the boundary).Tells the session the truth about history. When the PR is up to date with its base, the graft lands on HEAD's own ancestry and
git blame/git log <path>truncate silently; the prompt now teaches detection (cat .git/shallow, already in allowedTools) and demotes premise-audit conclusions that would cross the boundary.Hands the PR description to the session deterministically.
prepare.pyfetches title/body/author in its existing GraphQL call and renders them as the first item ofdiscussion-context.txt's untrusted-data fence — bypassing the bot filter, with its own 8000-char cap, never shed by the whole-block cap, with an explicit "(the PR has no description)" item when the body is empty. An empty file now unambiguously means the fetch failed. The STEP 1 specialist checklist passes the file down.Gives the session a fail-closed "I cannot review" channel. If the pinned diff itself errors mid-session, the session writes
review-result.jsonas{"infrastructure_failure": "<why>"};validate.pychecks that key before any schema validation and exits with no verdict. The alternatives are both named failure modes: empty findings compute as APPROVE (an unreviewed PR goes green), and a fabricated finding computes as a REJECT the patch-id skip re-asserts on every re-run.Bounds what the new description context can do. The
[pr-description]item can never clear, downgrade, or pre-empt a finding (it's the reviewed artifact, rewritable after a REJECT) — stated in the orchestrator paragraphs, the STEP 1 specialist checklist, and the fence header the specialists actually read. Item bodies are indented four spaces so a body imitating an[issue-comment] maintainer …envelope line is visibly body text; only the pipeline writes at column zero.Moves the prompt out of GitHub's 21000-char expression cap. The additions pushed the inline
prompt:scalar to ~23.6k chars, and any${{ }}-bearing scalar compiles into oneformat()expression with a hard 21000-char source cap — the file goes "Invalid workflow file" and, merged, that would brick the required check on every PR until an admin merge (caught by this branch's throwaway push workflow, run 31515805871). The prompt now travels as a compose-step output (runtime values are uncapped): a quoted-heredoc template with__PLACEHOLDER__tokens, four small env expressions, string substitution, and a one-expressionprompt:input. Two new structure tests keep the class dead (template must carry no inline${{; a sweep fails any expression-bearing scalar in any workflow at 19000 chars with a named reason).No spec: this is a bug fix — it restores the gate to its documented theory (review the PR's own diff, fail closed on infrastructure) rather than revising it.
Assumptions
git merge-basereturning an ancestor of HEAD meansgit diff <mb> HEADandgit log <mb>..HEADstay graft-immune (HEAD's own ancestry can only be grafted at or above the merge base when the branch is behind it — the diff endpoints are still local objects either way).pull_request_target), the fix proves itself on the first PR reviewed after merge, not on this one.Evidence & verification
8b9b9b8; session diagnostics reportorigin/main"fetched shallowly (depth 1)"), 31504414058 (same guard pass; session'sgit log --oneline origin/mainreturned exactly one commit, merge-base empty, two-dot stat showing 12 main-side files).git fetch --depth=1of a moved ref into a complete clone).python3 -m pytest .github/workflows/claude-review-v2/tests/(the "Review scripts tests" CI job): 311 passed. The new no-merge-base test discriminates: with only the fail-closed abort removed, it fails — the old code exits 0, writesskip-decision.json, and proceeds diffless. Five fresh-context/code-review highrounds ran over the branch; every finding (1 HIGH, 4 MEDIUM, 12 MINOR across rounds) was addressed; round 5 surfaced no High/Medium.on: pushworkflow on this branch (added, verified, then removed — a PR cannot test its own review workflow, sincepull_request_targetruns main's copy) passed on a real ubuntu runner (runs 31515807299, 31516462128): the graft reproduction with pinned-SHA immunity, the real extracted "Ensure a merge-base" step block against repairable-shallow / grafted / unrepairable-severed repos (annotated fail-closed exit confirmed), and the realprepare.pyon healthy (recordsmerge_base) and severed (aborts, writes nothing) checkouts. It also caught the expression-cap regression before merge.__PLACEHOLDER__leftovers, well-formed multiline$GITHUB_OUTPUT.🔗 open in tbd