diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 06430dc31..a700b3b9e 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -7,8 +7,10 @@ name: Claude Auto Review # enforcement lives in claude-review-guard.yml, whose verdict arm fails while the newest verdict # is changes-requested, so a substantive finding becomes a red check instead of a comment that # auto-merge outruns (the #3470-#3473 train shipped six findings in one night that way; one was -# real). It no-ops cleanly until the CLAUDE_CODE_OAUTH_TOKEN repo secret is set, and on fork PRs -# (which do not receive secrets), so neither case shows a failed check. +# real). Since #3650 the job also fails ITSELF when the run submitted no verdict, with the cause in +# its log and the transcript attached -- a green here means a verdict exists, and the guard is the +# second line. It no-ops cleanly until the CLAUDE_CODE_OAUTH_TOKEN repo secret is set, and on fork +# PRs (which do not receive secrets), so neither case shows a failed check. on: pull_request: types: [opened, synchronize, reopened] @@ -37,7 +39,19 @@ jobs: with: fetch-depth: 1 + # #3650: the verdict check at the bottom counts the bot's formal reviews submitted AFTER this + # instant, so a verdict left by an earlier run on the same PR cannot vouch for this one. Read + # once, here, before the action installs anything -- GitHub stamps submitted_at in the same + # ISO-8601 UTC shape (2026-09-18T22:00:00Z), so the comparison below is a plain string one. + - name: Open the verdict window + id: window + if: ${{ env.CLAUDE_CODE_OAUTH_TOKEN != '' }} + run: echo "start=$(date -u +%FT%TZ)" >> "$GITHUB_OUTPUT" + + # The step NAME is read by claude-review-guard.yml (REVIEW_STEP) to tell a clean no-op from a + # run that said nothing; renaming it blinds the guard. The id is for the steps below. - name: Claude review + id: review if: ${{ env.CLAUDE_CODE_OAUTH_TOKEN != '' }} uses: anthropics/claude-code-action@v1 with: @@ -78,5 +92,139 @@ jobs: does not enable, and no gate needs it. The guard's rule is newest-verdict-wins: on a re-review after new commits, review the NEW diff and submit a fresh verdict, and a clean fresh verdict clears an earlier changes-requested by itself. + # #3650: the prompt above says the branch is checked out and asks for a correctness, + # parity and security review -- an invitation to read code -- while the allowlist used to + # permit only the four gh verbs and the inline-comment tool. Every Read, Grep, Glob and + # git call the reviewer reached for was a permission denial, and the swallowed runs' own + # result blocks put the ratio on record: 50 turns / 18 denials, 39 / 21, 18 / 17, each + # ending subtype=success with NOTHING posted. After enough denials the session ends + # without ever reaching the verdict protocol, so a paid run leaves no trace (three PRs, + # about thirteen runs, one night). Read-only tools are enough: the reviewer needs to open + # the files the diff touches, find a symbol's other callers and read a parity twin, and + # nothing about reviewing needs a write. Nothing here can edit, commit, push or post + # outside the gh verbs already listed; the git verbs are the read-only three. claude_args: | - --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr review:*)" + --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr review:*),Read,Grep,Glob,Bash(git diff:*),Bash(git log:*),Bash(git show:*)" + + # #3650: a review that posted nothing used to finish GREEN. The prompt mandates one formal + # review per run -- changes-requested on a substantive finding, a "LGTM" comment review + # otherwise -- so on this repo a run that submitted no verdict has broken its contract every + # time; a clean review is never silent, and a zero here is never legitimate. This step turns + # that into the job's own colour, with the diagnosis in the log, so the guard (#2229/#3492, + # still the REQUIRED check, still enforcing newest-verdict-wins and the drift arm) becomes the + # second line rather than the first. Counted: reviews by the bot, submitted inside this run's + # window, with a NON-EMPTY body. The body test is load-bearing: the inline-comment tool files + # each comment inside a review of its own with an empty body (#3647 carried four bot reviews, + # two of them bodiless carriers), and both verdict shapes must carry one (gh and the API both + # refuse a bodiless comment/changes-requested review) -- so "posted inline notes, never a + # verdict" is the #3470 shape and fails here, as it should. + # Two runs end with zero verdicts and both are defects on this repo, told apart by whether + # Claude ran at all: the action exits SUCCESS in seconds without running when this file + # differs from the default branch's copy (cause 1 in the guard's header), and then it sets no + # execution_file and writes no transcript. Expected on a PR that edits this file; a repo-wide + # outage otherwise -- the guard grades which, because it can read the PR's file list from a + # workflow that is free to change. The step's own outcome is left to speak for itself when it + # is not success (a failed step already reds the job; a cancelled one is a superseded push). + # A gh lookup failure is NOT a verdict on the review (#2309): it warns and stands down rather + # than forcing a paid re-run of the whole job to clear a transient API error -- the guard's + # own tally, a separate and free-to-rerun workflow, stays the enforcement. + - name: Verify the review posted a verdict + if: ${{ always() && env.CLAUDE_CODE_OAUTH_TOKEN != '' }} + env: + GH_TOKEN: ${{ github.token }} + R: ${{ github.repository }} + PR: ${{ github.event.pull_request.number }} + SINCE: ${{ steps.window.outputs.start }} + REVIEW_OUTCOME: ${{ steps.review.outcome }} + # Set by the action only after Claude actually ran; empty when it refused at validation. + EXECUTION_FILE: ${{ steps.review.outputs.execution_file }} + # Author of every artifact the review leaves behind (the action's bot_name default). + REVIEW_BOT: claude[bot] + run: | + set -euo pipefail + summary() { echo "$*" >> "$GITHUB_STEP_SUMMARY"; } + + if [ "$REVIEW_OUTCOME" != "success" ]; then + echo "::notice title=Verdict check skipped::The review step ended '$REVIEW_OUTCOME'; its own"\ + "outcome carries the story, so this step has nothing to add." + exit 0 + fi + + transcript="${EXECUTION_FILE:-$RUNNER_TEMP/claude-execution-output.json}" + + errfile=$(mktemp 2>/dev/null || echo /dev/null) + if ! matched=$(gh api --paginate "repos/$R/pulls/$PR/reviews?per_page=100" \ + --jq ".[] | select(.user.login == env.REVIEW_BOT + and .submitted_at != null + and .submitted_at >= env.SINCE + and ((.body // \"\") | length) > 0) + | \"\\(.state)\\t\\(.submitted_at)\\t\\(.html_url)\"" 2>"$errfile"); then + err=$(cat "$errfile" 2>/dev/null || true) + [ "$errfile" != /dev/null ] && rm -f "$errfile" || true + echo "::warning title=Verdict check could not read the PR's reviews::gh api"\ + "repos/$R/pulls/$PR/reviews failed: ${err:-no stderr}. A lookup failure is not a"\ + "review verdict (#2309), so the review is UNCONFIRMED here; the guard's own tally"\ + "decides, or read the PR by eye." + summary "- Verdict check: lookup failed; review UNCONFIRMED (#2309)." + exit 0 + fi + [ "$errfile" != /dev/null ] && rm -f "$errfile" || true + + count=$(printf '%s' "$matched" | grep -c . || true) + echo "verdict reviews by $REVIEW_BOT on PR #$PR since $SINCE: $count" + if [ -n "$matched" ]; then printf '%s\n' "$matched"; fi + + if [ "$count" -gt 0 ]; then + summary "### Claude review posted $count verdict review(s) since $SINCE" + exit 0 + fi + + summary '### Claude review posted NO verdict' + + if [ -z "$EXECUTION_FILE" ] && [ ! -s "$transcript" ]; then + echo "::error title=Claude never ran::claude-code-action exited success without running"\ + "Claude -- no execution file, no transcript. That is what it does when this branch's"\ + ".github/workflows/claude-review.yml differs from the default branch's copy (#2229,"\ + "cause 1). Expected on a PR that edits this file; on any other PR it means the file"\ + "has drifted and EVERY PR in the repo is going unreviewed -- see the guard's verdict"\ + "on this PR. Either way, do not read this PR as reviewed." + summary '- Claude never ran (workflow validation refused). Not reviewed.' + exit 1 + fi + + echo "::error title=Review ran and posted no verdict::The review ran and posted no"\ + "verdict: $REVIEW_BOT submitted no formal review on PR #$PR since $SINCE, and the"\ + "prompt promises one every run even when clean. Real money was spent and the output"\ + "vanished (#3650; the #2229 failure shape). Do NOT read this PR as reviewed. The"\ + "transcript is attached to this run as the claude-review-transcript artifact; its"\ + "result block follows." + summary '- The review ran and submitted no formal review. Not reviewed (#3650).' + if [ -s "$transcript" ]; then + echo "--- result block of $transcript ---" + jq -c '.[-1] | {type, subtype, is_error, num_turns, permission_denials_count, + total_cost_usd, duration_ms, + denied: [.permission_denials[]? | .tool_name]}' "$transcript" \ + 2>/dev/null || true + echo "--- tail of $transcript ---" + tail -c 4000 "$transcript" || true + echo + else + echo "(no transcript at $transcript)" + fi + exit 1 + + # #3650: the per-turn transcript is what proves WHY a run said nothing, and it used to die + # with the runner -- the denial-ratio evidence above had to be inferred from result blocks in + # the step log. Seven days is long enough to diagnose the next swallowed run in one click and + # short enough that nothing accumulates. The reviewer's tools are read-only over a public + # tree and gh reads of public PR data, so the transcript holds nothing that is not already + # public; keep the allowlist that way, because this artifact is readable by anyone who can + # read the repo. Missing file (the action refused to run, or was skipped) is not an error. + - name: Retain the review transcript + if: ${{ always() && env.CLAUDE_CODE_OAUTH_TOKEN != '' }} + uses: actions/upload-artifact@v6 + with: + name: claude-review-transcript + path: ${{ steps.review.outputs.execution_file || format('{0}/claude-execution-output.json', runner.temp) }} + if-no-files-found: ignore + retention-days: 7