diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index fa4a221..f79a272 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -23,17 +23,23 @@ on: default: "" generated_paths: description: >- - Path pattern(s) for committed generated data the reviewer should - skim rather than analyze, e.g. "outputs/**". Empty: the skim rule is - omitted from the prompt. + Whitespace-separated git glob pattern(s) for committed generated + data, e.g. "outputs/**"; spaces or newlines, so a YAML `|` block + with one pattern per line works. A pattern with no slash matches at + any depth, as in .gitignore. Matching files are left out of the + review: no diff is written for them, and the prompt lists them by + name and size only. Meant for bulk machine output nobody reads line + by line, not lockfiles: a lockfile's diff is usually small, and it + is the only place indirect dependency and source changes show. + Empty: every changed text file is reviewed. type: string required: false default: "" generated_paths_note: description: >- - One-sentence repo context appended to the generated-data skim rule - (e.g. a pointer to the CLAUDE.md section explaining why the data is - committed). Ignored when generated_paths is empty. + One-sentence repo context appended to the generated-files section + of the prompt (e.g. a pointer to the CLAUDE.md section explaining + why the data is committed). Ignored when generated_paths is empty. type: string required: false default: "" @@ -126,7 +132,10 @@ jobs: - name: Checkout repository uses: actions/checkout@v4 with: - fetch-depth: 1 + # 2, not 1: on pull_request this checks out GitHub's merge commit, + # and "Write per-file diffs" diffs it against its first parent (the + # base). Depth 2 fetches both parents and nothing older. + fetch-depth: 2 # The action reports the job as successful the moment the agent process # ends, even when it posted no verdict (see the verify step at the end). @@ -137,6 +146,172 @@ jobs: id: review-start run: echo "ts=$(date -u -d '60 seconds ago' +%Y-%m-%dT%H:%M:%SZ)" >> "$GITHUB_OUTPUT" + # The review cannot see a large PR's diff through `gh pr diff`. The + # whole diff reaches the agent as ONE tool result, and above Claude + # Code's tool-output limit it gets only "Output too large (150.9KB). + # Full output saved to: …" plus a 2 KB preview. `gh pr diff` takes no + # path argument, so it cannot ask for one file instead. On + # Deco354/factory-farm-em#29 (2026-10-06..08, 25 files, ~150 KB of + # diff) all seven runs tried a per-file diff, were refused ("accepts at + # most 1 arg(s)"), never opened the saved file, read one to three + # files and approved — while `analysis/summary.py`, which computes the + # PR's headline numbers, went unread (issue #11). So the diff is split + # here, one file per changed path under .claude-review/diffs/, which + # the Read tool opens and pages like any other file. The tool + # allowlist is unchanged. + # + # git rather than splitting `gh pr diff`: no API call, and no ceiling — + # GitHub's diff endpoint refuses very large PRs, and the pipeline + # repo's `outputs/**` PRs are the large ones. generated_paths become + # exclude pathspecs, so a generated file never gets a diff to read. + - name: Write per-file diffs + id: diffs + env: + GENERATED_PATHS: ${{ inputs.generated_paths }} + run: | + set -euo pipefail + # actions/checkout on pull_request checks out the merge commit, whose + # first parent is the base: HEAD^1..HEAD is exactly what merging + # would change. Any other checkout has no base to diff against here. + if ! git rev-parse -q --verify 'HEAD^2' >/dev/null; then + echo "::error::HEAD is not the pull_request merge commit, so there is no base to diff against. The caller must trigger on pull_request (see callers/claude-pr-review.caller.yml)." + exit 1 + fi + out="${GITHUB_WORKSPACE}/.claude-review" + state="${RUNNER_TEMP}/claude-review" + rm -rf "$out" "$state" + mkdir -p "$out/diffs" "$state" + + # generated_paths -> git glob pathspecs. `read -a`, not an unquoted + # $GENERATED_PATHS, so `outputs/**` is never expanded against the + # checkout. A pattern with no slash matches at any depth, as in + # .gitignore: `uv.lock` also matches `pkg/uv.lock`. `-d ''` reads + # the whole value, so a YAML `|` block with one pattern per line + # works too; a plain `read` stops at the first newline, and the + # other patterns were silently reviewed (devops#12 selftest, + # 2026-10-09). It returns 1 at the end of input, hence `|| true`. + read -r -d '' -a pats <<<"${GENERATED_PATHS}" || true + gen=() + for p in "${pats[@]}"; do + gen+=(":(glob)${p}") + case "$p" in */*) ;; *) gen+=(":(glob)**/${p}") ;; esac + done + excl=() + for g in "${gen[@]}"; do excl+=(":(exclude,${g#:(}"); done + + # `git diff --numstat -z` record: "addeddeletedpath"; for a + # rename the path is empty and the old and new paths follow as two + # more NUL-terminated fields. Binary files count "-". + split() { + add="${1%%$'\t'*}"; local rest="${1#*$'\t'}" + del="${rest%%$'\t'*}"; path="${rest#*$'\t'}" + } + + : >"$state/generated.md" + if [ "${#gen[@]}" -gt 0 ]; then + while IFS= read -r -d '' rec; do + split "$rec" + printf -- "- \`%s\` (+%s/-%s)\n" "$path" "$add" "$del" >>"$state/generated.md" + done < <(git diff --no-renames --numstat -z HEAD^1 HEAD -- "${gen[@]}") + fi + + n=0 + : >"$state/manifest.jsonl"; : >"$state/files.md"; : >"$state/binary.md" + git diff -M --numstat -z HEAD^1 HEAD -- . "${excl[@]}" >"$state/numstat" + while IFS= read -r -d '' rec; do + split "$rec" + from="" + if [ -z "$path" ]; then + IFS= read -r -d '' from + IFS= read -r -d '' path + fi + if [ "$add" = "-" ]; then + printf -- "- \`%s\`\n" "$path" >>"$state/binary.md" + continue + fi + diff="$out/diffs/${path}.diff" + mkdir -p "$(dirname "$diff")" + git --literal-pathspecs diff -M HEAD^1 HEAD -- ${from:+"$from"} "$path" >"$diff" + lines=$(( $(wc -l <"$diff") )) + n=$((n + 1)) + jq -nc --arg p "$path" --arg d "$diff" --argjson l "$lines" \ + '{path: $p, diff: $d, lines: $l}' >>"$state/manifest.jsonl" + label="+${add}/-${del}, ${lines} diff lines" + if [ -n "$from" ]; then label="renamed from \`${from}\`, ${label}"; fi + printf -- "- \`%s\` (%s): \`%s\`\n" "$path" "$label" "$diff" >>"$state/files.md" + done <"$state/numstat" + + # What "read" means for these diffs, used by the verdict gate during + # the review and by the coverage steps after it: the lines successful + # Read calls returned cover all of a diff's lines. Returned, not + # asked for: Read stops at its 25,000-token cap without an error. On + # 2026-10-09 (Claude Code 2.1.295) a 2,000-line diff came back as + # lines 1-935, marked truncatedByTokenCap, and a count that trusted + # the call's default 2,000-line limit scored it as read. The result's + # own startLine/numLines say what the agent saw. A result without + # them (an older format) falls back to the call's offset/limit. + cat >"$state/coverage.jq" <<'JQ' + # Input (-s): one or more arrays of SDK messages, i.e. a pass's + # execution file or the session transcript as an array. + # $m: the manifest, one {path, diff, lines} per diff. + # Output: the manifest with `read` and `unread` (the line ranges + # not yet read, [[first, last], ...]) set on every entry. + def gaps($n): # input: [[first, last], ...] line ranges read + reduce sort[] as $r ({next: 1, out: []}; + (if $r[0] > .next and .next <= $n + then .out += [[.next, ([$r[0] - 1, $n] | min)]] else . end) + | .next = ([.next, $r[1] + 1] | max)) + | .out + (if .next <= $n then [[.next, $n]] else [] end); + (add // []) as $msgs + | [ $msgs[] | select(.type == "user") | .message.content[]? + | select(.type == "tool_result" and (.is_error // false)) | .tool_use_id ] as $failed + # Each tool result's structured form, by tool_use_id: the execution + # file calls it tool_use_result, the transcript toolUseResult, and + # each user message carries one tool result. + | ( [ $msgs[] | select(.type == "user") + | ((.tool_use_result // .toolUseResult) | objects) as $r + | .message.content[]? | select(.type == "tool_result") + | { key: .tool_use_id, value: $r } ] | from_entries ) as $shown + | [ $msgs[] | select(.type == "assistant") | .message.content[]? + | select(.type == "tool_use" and .name == "Read") + | select(.id as $id | any($failed[]; . == $id) | not) + | (.input.file_path // "") as $f + | $shown[.id] as $r + | ( if ($r.file.numLines? | type) == "number" then + ($r.file.startLine // 1 | tonumber) as $first + | [$first, $first + $r.file.numLines - 1] + elif $r == null then + ([(.input.offset // 1 | tonumber), 1] | max) as $first + | [$first, $first + (.input.limit // 2000 | tonumber) - 1] + else empty end ) as $range + | { file: (if ($f | startswith("/")) then $f else "\(env.GITHUB_WORKSPACE)/\($f)" end), + range: $range } ] as $reads + | $m | map(. as $e + | ([ $reads[] | select(.file == $e.diff) | .range ] | gaps($e.lines)) as $g + | . + { read: ($g == []), unread: $g }) + JQ + # One line per unread diff, for the gate's refusal and the follow-up + # prompt: the unread line ranges, so a review that paged one line + # short of the end is told exactly that. Sonnet 5 on + # Deco354/factory-farm-em#40 (2026-10-09, 2 runs) read to line 4889 + # and to 4883 of a 4,890-line diff, was told only "not read to the + # end", re-read the same range, and was held back twice. + cat >"$state/unread.jq" <<'JQ' + def span: if .[0] == .[1] then "\(.[0])" else "\(.[0])–\(.[1])" end; + .[] | select(.read | not) + | "- `\(.path)`: `\(.diff)` (\(.lines) lines; not read yet: " + + (if .unread == [[1, .lines]] then "all of it" + elif .unread == [[.unread[0][0], .unread[0][0]]] then "line \(.unread[0][0])" + else "lines " + ([.unread[:8][] | span] | join(", ")) + + (if (.unread | length) > 8 then ", …" else "" end) end) + + ")" + JQ + + echo "${n} diff(s) to review:"; cat "$state/files.md" + if [ -s "$state/generated.md" ]; then echo "Generated, not reviewed:"; cat "$state/generated.md"; fi + if [ -s "$state/binary.md" ]; then echo "Binary, no text diff:"; cat "$state/binary.md"; fi + echo "count=${n}" >>"$GITHUB_OUTPUT" + echo "state=${state}" >>"$GITHUB_OUTPUT" # The review prompt is assembled here, not inline in the action's # `with:`, so callers can swap the repo-specific sections through @@ -155,41 +330,111 @@ jobs: GENERATED_PATHS: ${{ inputs.generated_paths }} GENERATED_PATHS_NOTE: ${{ inputs.generated_paths_note }} EXTRA_INSTRUCTIONS: ${{ inputs.extra_instructions }} + DIFF_COUNT: ${{ steps.diffs.outputs.count }} + STATE: ${{ steps.diffs.outputs.state }} run: | + # Bash 5.2 turns an unquoted `&` in a ${T//pat/rep} replacement into + # the matched text; a note or file name containing `&` must survive. + shopt -u patsub_replacement 2>/dev/null || true P="$(mktemp)" printf 'REPO: %s\nPR NUMBER: %s\n\n' "$REPO" "$PR_NUMBER" >>"$P" + # The diff is NOT read through `gh pr diff` any more — see "Write + # per-file diffs" for why. The list of diff files is the review's + # scope, and "every diff, to its last line" is measured afterwards + # (see "Measure which diffs the review read"). + # + # Read-all-first, and naming tests/docs/configs as in scope, are + # from Deco354/factory-farm-em#40 (2026-10-08, 2 runs). There, with + # "Read EVERY diff" but "focus on … the changed code only" and + # "you do not need [the tests]" further down, Sonnet 5.5 read the + # source diffs a few at a time, most important first. It skipped + # all 6 test files and both docs, 12 of 24 diffs in each run, then + # approved and said it had read them. + # + # "About 800 lines at a time", and "the diff lines count": on #40 + # (2026-10-09) Sonnet 5.5 asked for 2,000, 1,400 and 1,100 lines of + # a 4,890-line diff, each page over Read's 25,000-token cap, before + # 900 worked; Sonnet 5 paged by the file's +4884 count and stopped + # six lines short of the diff's end. + # + # Tools: the review has no Grep or Glob tool. Claude Code 2.1.117's + # native build, which the action installs, replaced both "by + # embedded `bfs` and `ugrep` available through the Bash tool" + # (changelog). A Grep call is refused with "search file contents + # with `grep` via the Bash tool instead" (#40, 2026-10-08). Naming + # either tool in --allowedTools would restore both (docs, + # tools-reference); `grep` through Bash does the same job without + # touching the allowlist. Claude Code also runs a built-in set of + # read-only commands "without a permission prompt in every mode" + # (docs, permissions: `cat`, `head`, `tail`, `grep`, `find`, + # read-only `git`, …; "not configurable"). So those run despite the + # `gh pr`-only allowlist, while `python3 -c` is still refused. Diffs + # are asked for through Read because coverage counts only Read + # calls: Sonnet 5 read parts of a diff with `sed -n` and `tail` on + # #40 (2026-10-09, run 37914095042), and those lines then showed as + # unread. cat >>"$P" <<'BLOCK' Review this pull request. The PR branch is already checked out in - the current working directory. Inspect the changes with - `gh pr diff`. To read individual files, use the Read, Grep, and - Glob tools — do NOT use git, cat, sed, grep, or find via Bash: only - the `gh pr` subcommands below are permitted, and any other Bash - command is denied and just wastes turns. + the current working directory, and the diff of each changed file + is saved as its own file, listed below. Read the diffs, and any + other file you need, with the Read tool, not through Bash. Do not + run `gh pr diff`: on a large PR its output is too long to be shown + to you and arrives as a 2 KB preview. There is no Grep or Glob + tool: to search file contents, run `grep` through Bash. Use Bash + for nothing else but the `gh pr` subcommands below; most other + commands are denied and just waste turns. + BLOCK + + if [ "$DIFF_COUNT" -gt 0 ]; then + read -r -d '' T <<'BLOCK' || true + Changed files (__COUNT__). Start by reading EVERY diff below, each + one to its last line — not a sample, and not only the files that + look riskiest. Issue those Read calls together, in one turn if you + can. Tests, docs, and config files are part of the review, not + just source code: a test can assert the wrong behaviour, a config + can set a wrong value, and a doc can promise something the code + does not do. Read returns at most 2,000 lines per call, and a page + over about 25,000 tokens fails or comes back cut short. So read a + long diff about 800 lines at a time (`offset` 1, 801, 1601, …) + until you reach its last line: the "diff lines" count in the list, + not its +/− counts. Only then read the surrounding source wherever + a hunk needs context. BLOCK + T="${T//__COUNT__/${DIFF_COUNT}}" + printf '\n%s\n\n' "$T" >>"$P" + cat "$STATE/files.md" >>"$P" + else + printf '\n%s\n' "This PR changes no non-generated text files, so there is no diff to read." >>"$P" + fi - if [ -n "$GENERATED_PATHS" ]; then + if [ -s "$STATE/generated.md" ]; then read -r -d '' T <<'BLOCK' || true - Some PRs commit generated data under `__PATHS__` — machine-written - output, not hand-authored code, and a single change can add - thousands of lines.__NOTE__ - Run `gh pr diff --name-only` first; if any changed paths match - those patterns, skim past those hunks in the full diff without - analyzing their content, and just note the file/line count in - your summary. Spend your review effort on the non-generated - paths only. + These changed files match the repository's generated-data patterns + (`__PATHS__`) — machine-written output, not hand-authored code.__NOTE__ + They are not part of your review and no diff is saved for them: + name them with their line counts in your summary and move on. BLOCK note="" if [ -n "$GENERATED_PATHS_NOTE" ]; then note=" ${GENERATED_PATHS_NOTE}"; fi - T="${T//__PATHS__/${GENERATED_PATHS}}" + # One line, space-separated, even when the input is a `|` block. + read -r -d '' -a pats <<<"${GENERATED_PATHS}" || true + T="${T//__PATHS__/${pats[*]}}" T="${T//__NOTE__/${note}}" - printf '\n%s\n' "$T" >>"$P" + printf '\n%s\n\n' "$T" >>"$P" + cat "$STATE/generated.md" >>"$P" + fi + + if [ -s "$STATE/binary.md" ]; then + printf '\n%s\n\n' "Binary files changed (no text diff to read):" >>"$P" + cat "$STATE/binary.md" >>"$P" fi cat >>"$P" <<'BLOCK' - Focus on high-signal problems in the changed code only: + Report only high-signal problems in the changed files (this bar is + for what you report, not for what you read): - Bugs, logic errors, and unhandled edge cases - Security issues - Clear violations of guidelines in any relevant CLAUDE.md (root or a @@ -224,10 +469,12 @@ jobs: read -r -d '' T <<'BLOCK' || true You cannot run tests and you cannot read CI results — do not try, the calls are denied and will only waste turns. You do not need - them: this repository's `__CHECK__` check runs the suite against - this commit, and its result is enforced by the repository's branch - rules independently of your review. Review the code on its merits - and do not caveat your review over not having seen the tests. + the results: this repository's `__CHECK__` check runs the suite + against this commit, and its result is enforced by the repository's + branch rules independently of your review. Review the code on its + merits and do not caveat your review over not having seen test + results. That is about results only: the test files in the diff + are part of your review, like every other diff. BLOCK T="${T//__CHECK__/${REQUIRED_CHECK}}" else @@ -241,14 +488,27 @@ jobs: T="${T//__PR__/${PR_NUMBER}}" printf '\n%s\n' "$T" >>"$P" + # "Do not describe how you reviewed": with the per-file diffs and + # the verdict gate, all three approvals on Deco354/factory-farm-em#40 + # (2026-10-08) opened with "I read every diff in this PR to its last + # line" or similar, our instructions echoed back as workflow jargon + # the PR author cannot act on. Coverage is measured, not + # self-reported: the job summary and the warning comment carry it. read -r -d '' T <<'BLOCK' || true Leave feedback with `mcp__github_inline_comment__create_inline_comment` - for each specific issue (one comment per issue, on the relevant line). + for each specific issue (one comment per issue, on the relevant line + of the changed file: its own path and line number, not the diff + file's). Then submit exactly ONE formal review verdict for PR #__PR__. First write your summary to a file named `review-body.md` in the repository root using the Write tool (this keeps the command below free of shell quoting). + Write it for the PR's author, about the change and what you found + in it. Do not describe how you reviewed: never mention the diff + files, how many files or lines you read, or any check on your + reading. Those are internal to this workflow, which reports + coverage separately. Your verdict MUST agree with your own summary. Before running the command, re-read what you wrote: @@ -298,6 +558,82 @@ jobs: echo "${delim}" } >>"$GITHUB_OUTPUT" + # The prompt alone does not get every diff read. On + # Deco354/factory-farm-em#40 (2026-10-08) the first pass read 12, 12 + # and 7 of 24 diffs under three prompt wordings. The third told it to + # "Start by reading EVERY diff", and it still wrote "I did not read + # every diff" and approved. Yet each follow-up pass, handed the list + # of what was unread, read all of it. So this hands it that list + # *before* the verdict. + # + # A PreToolUse hook refuses `gh pr review` while any listed diff is + # unread, by the same rule as "Measure which diffs the review read", + # and names what is left. The action merges its `settings` input into + # ~/.claude/settings.json, so the hook stays installed for the + # follow-up pass too. + # + # The hook refuses at most twice per job, then lets the verdict + # through, and it lets it through whenever it cannot tell (no + # transcript, a jq error). It decides when the verdict is posted, + # never what it says, and it never fails the check: coverage stays + # report-only (#11). continue-on-error because without the hook the + # review runs as it did before. + - name: Hold the verdict until every diff is read + id: gate + if: steps.diffs.outputs.count != '0' + continue-on-error: true + env: + STATE: ${{ steps.diffs.outputs.state }} + run: | + set -euo pipefail + cat >"$STATE/verdict-gate.sh" <<'GATE' + #!/bin/bash + # PreToolUse hook on the review agent's Bash calls. Exit 2 refuses + # the call and shows stderr to the agent; any other exit lets it run. + set -o pipefail + state=$(dirname "$0") + input=$(cat) || exit 0 + cmd=$(jq -r '.tool_input.command // ""' <<<"$input") || exit 0 + [[ "$cmd" =~ ^[[:space:]]*gh[[:space:]]+pr[[:space:]]+review([[:space:]]|$) ]] || exit 0 + held=$(cat "$state/gate-held" 2>/dev/null || echo 0) + [ "$held" -lt 2 ] || exit 0 + transcript=$(jq -r '.transcript_path // ""' <<<"$input") || exit 0 + [ -f "$transcript" ] || exit 0 + GITHUB_WORKSPACE="${GITHUB_WORKSPACE:-$(jq -r '.cwd // ""' <<<"$input")}" + export GITHUB_WORKSPACE + # The transcript is JSONL: one message per line. Skip a line that + # does not parse rather than give up on the whole count. + list=$(jq -R 'fromjson? // empty' "$transcript" | jq -s . | + jq -s --slurpfile m "$state/manifest.jsonl" -f "$state/coverage.jq" | + jq -r -f "$state/unread.jq") || exit 0 + [ -n "$list" ] || exit 0 + echo $((held + 1)) >"$state/gate-held" + { + echo "Not yet: your review has not read these diffs to the end, and its verdict has to cover every listed diff:" + echo + printf '%s\n' "$list" + echo + cat <<'MSG' + This is not an error, and nothing is wrong with the command. Read + the unread lines listed for each diff now, with the Read tool: all + of them in one turn if you can. Start each call at the first unread + line (`offset`) and read about 800 lines per call until you reach + the diff's last line. Leave an inline comment for each significant + issue you find in them. Update review-body.md so it covers them, + then run your `gh pr review` command again. This check is internal + to the review workflow: do not mention it, or the diff files, in + review-body.md. + MSG + } >&2 + exit 2 + GATE + rm -f "$STATE/gate-held" + jq -n --arg cmd "bash \"${STATE}/verdict-gate.sh\"" \ + '{hooks: {PreToolUse: [{matcher: "Bash", hooks: [{type: "command", command: $cmd, timeout: 60}]}]}}' \ + >"$STATE/settings.json" + jq -e . "$STATE/settings.json" >/dev/null + echo "settings=${STATE}/settings.json" >>"$GITHUB_OUTPUT" + - name: Run Claude Code Review id: claude-review uses: anthropics/claude-code-action@v1 @@ -316,6 +652,9 @@ jobs: # bot names instead of '*' if you want to restrict which bots trigger it. allowed_bots: "*" prompt: ${{ steps.prompt.outputs.prompt }} + # The verdict gate's hook (see "Hold the verdict until every diff is + # read"). Empty when that step was skipped or failed: no hook. + settings: ${{ steps.gate.outputs.settings }} # The action denies comment-posting tools by default; grant the inline # comment tool plus the gh subcommands the review needs, or it reviews # but cannot post its findings. `Write` lets the review compose its @@ -340,13 +679,221 @@ jobs: # summary channel, and across nine high-traffic PRs the review had # never once posted a plain comment. Keeping it only offered a # weaker exit than the three verdicts. + # --effort high: Claude Code defaults Sonnet 5.5, the action's + # default model since late September, to `medium` effort, and + # Sonnet 5 to `high` (docs, model-config, "Default effort levels"). + # On factory-farm-em#40 (2026-10-09, 2 runs each), Sonnet 5.5 read + # 9–13 of 25 diffs before its first verdict attempt at the + # default, and 24 of 25 with `high`, as Sonnet 5 did, so the + # verdict gate went back to being a backstop. It cost about $0.07 + # and 25 s more per run there. `high` alone does not fix #11: on + # main's workflow, without per-file diffs, it still approved #40 + # after reading 0 and 5 of 25 diffs. A `model` input without + # effort support ignores the flag: no warning or error (Haiku 4.5, + # 2.1.295, tested locally 2026-10-09). claude_args: | --allowedTools "Write,mcp__github_inline_comment__create_inline_comment,Bash(gh pr review:*),Bash(gh pr diff:*),Bash(gh pr view:*)" --disallowedTools "Task" + --effort high ${{ inputs.model != '' && format('--model {0}', inputs.model) || '' }} # See https://github.com/anthropics/claude-code-action/blob/main/docs/usage.md # or https://code.claude.com/docs/en/cli-reference for available options + # Did the review read every diff? The prompt asks it to, but the agent + # decides, so measure it from the execution file rather than trusting + # the summary: a diff counts as read only when the lines the agent's + # successful Read calls returned cover every one of its lines (see + # coverage.jq in "Write per-file diffs"). A #29 run read lines 1-80 of + # a 461-line file and moved on. Report-only by decision (issue #11, + # 2026-10-08: tying the verdict to coverage would block too many PRs): + # this never fails the check or touches the verdict. It decides whether + # the follow-up pass below runs, and "Report review coverage" re-counts + # over both passes. continue-on-error keeps that true if this script + # itself breaks (an execution file jq cannot parse, say): the follow-up + # and the report then skip, and the verdict is checked as usual. + - name: Measure which diffs the review read + id: coverage + if: >- + always() && !cancelled() && + steps.claude-review.outcome == 'success' && + steps.claude-review.outputs.execution_file != '' && + steps.diffs.outputs.count != '0' + continue-on-error: true + env: + STATE: ${{ steps.diffs.outputs.state }} + EXEC_FILE: ${{ steps.claude-review.outputs.execution_file }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + set -euo pipefail + shopt -u patsub_replacement 2>/dev/null || true + # The follow-up pass writes its execution file to the same path, so + # keep this pass's copy for the count over both passes. + cp "$EXEC_FILE" "$STATE/exec-1.json" + + jq -s --slurpfile m "$STATE/manifest.jsonl" -f "$STATE/coverage.jq" \ + "$STATE/exec-1.json" >"$STATE/coverage.json" + total=$(jq length "$STATE/coverage.json") + unread=$(jq '[.[] | select(.read | not)] | length' "$STATE/coverage.json") + echo "unread=${unread}" >>"$GITHUB_OUTPUT" + held=$(cat "$STATE/gate-held" 2>/dev/null || echo 0) + echo "The review read $((total - unread)) of ${total} diffs to the end. The verdict gate held its verdict back ${held} time(s)." + if [ "$unread" -eq 0 ]; then + exit 0 + fi + list=$(jq -r -f "$STATE/unread.jq" "$STATE/coverage.json") + echo "$list" + + read -r -d '' T <<'BLOCK' || true + A check of your Read calls found that your review of PR #__PR__ did + not read these diffs to the end: + + __LIST__ + + Read the unread lines listed for each of them now, with the Read + tool: start each call at the first unread line (`offset`) and read + about 800 lines per call until you reach the diff's last line. Also + read any surrounding source a hunk needs. Leave an inline comment + for each specific issue, as before. + + Then submit exactly ONE more verdict, for the whole PR: what you + reviewed before plus these files, under the same rules as before. + Write the summary to `review-body.md` first. Open it with one line + saying it replaces your earlier review of this commit, then cover + the whole PR. As before, write about the change and what you found, + not about this check or the diff files. Run ONE of these — nothing + else: + - Significant issues found anywhere in the PR: + `gh pr review __PR__ --request-changes --body-file review-body.md` + - Correct, no significant issues: + `gh pr review __PR__ --approve --body-file review-body.md` + - The decision is not yours to make even in principle, as before: + `gh pr review __PR__ --comment --body-file review-body.md` + BLOCK + T="${T//__PR__/${PR_NUMBER}}" + T="${T//__LIST__/${list}}" + delim="PROMPT_$(od -An -N8 -tx1 /dev/urandom | tr -d ' \n')" + { + echo "followup_prompt<<${delim}" + printf '%s\n' "$T" + echo "${delim}" + } >>"$GITHUB_OUTPUT" + + # A second pass, only when the first left diffs unread. It resumes the + # same session — the action's `session_id` output "can be used with + # --resume to continue this conversation" — so the agent keeps what it + # already read and its first verdict in context, and is asked to + # finish. Its verdict is then the latest bodied claude review, which + # the verify step below already checks, unchanged. + # + # Why not post an `@claude` comment and let the mention handler do it: + # a comment made with the job's GITHUB_TOKEN starts no workflow + # (GitHub's recursion guard), and the review agent deliberately has no + # comment tool. continue-on-error: a follow-up that fails must not take + # the first pass's verdict down with it; "Report review coverage" says + # what is still unread. + # + # `with:` mirrors "Run Claude Code Review". Keep the two claude_args + # identical apart from --resume: the comments on the first explain + # each entry. + - name: Review the diffs the first pass did not read + id: claude-followup + if: >- + always() && !cancelled() && + steps.coverage.outcome == 'success' && + steps.coverage.outputs.unread != '' && + steps.coverage.outputs.unread != '0' && + steps.claude-review.outputs.session_id != '' + continue-on-error: true + uses: anthropics/claude-code-action@v1 + with: + claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + show_full_output: true + allowed_bots: "*" + prompt: ${{ steps.coverage.outputs.followup_prompt }} + claude_args: | + --resume ${{ steps.claude-review.outputs.session_id }} + --allowedTools "Write,mcp__github_inline_comment__create_inline_comment,Bash(gh pr review:*),Bash(gh pr diff:*),Bash(gh pr view:*)" + --disallowedTools "Task" + --effort high + ${{ inputs.model != '' && format('--model {0}', inputs.model) || '' }} + + # Report-only, like the measurement: what the review read over both + # passes, in the job summary. When diffs are still unread — the + # follow-up missed them, failed, or had no session to resume — it also + # says so on the PR, where the author will see it, with a ready-to-paste + # `@claude` prompt modelled on the one that reviewed #29's unread files + # (2026-10-08) and found a bug seven approvals had missed. The comment + # is posted with the job's own token, so its `@claude` text starts + # nothing; a human pastes it. Never fails the check. + - name: Report review coverage + if: always() && !cancelled() && steps.coverage.outcome == 'success' + env: + GH_TOKEN: ${{ github.token }} + STATE: ${{ steps.diffs.outputs.state }} + PR_NUMBER: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + FOLLOWUP_OUTCOME: ${{ steps.claude-followup.outcome }} + FOLLOWUP_EXEC: ${{ steps.claude-followup.outputs.execution_file }} + run: | + execs=("$STATE/exec-1.json") + if [ "$FOLLOWUP_OUTCOME" != "skipped" ] && [ -n "$FOLLOWUP_EXEC" ] && [ -f "$FOLLOWUP_EXEC" ]; then + execs+=("$FOLLOWUP_EXEC") + fi + if ! jq -s --slurpfile m "$STATE/manifest.jsonl" -f "$STATE/coverage.jq" \ + "${execs[@]}" >"$STATE/coverage.json"; then + echo "::warning::Could not count the review's coverage over both passes; see \"Measure which diffs the review read\" for the first pass." + exit 0 + fi + total=$(jq length "$STATE/coverage.json") + unread=$(jq '[.[] | select(.read | not)] | length' "$STATE/coverage.json") + case "$FOLLOWUP_OUTCOME" in + success) passes=" over two passes" ;; + failure) passes=" (the follow-up pass failed)" ;; + *) passes="" ;; + esac + held=$(cat "$STATE/gate-held" 2>/dev/null || echo 0) + { + echo "### Review coverage" + echo + echo "Read **$((total - unread)) of ${total}** non-generated diffs to the end${passes}." + if [ "$held" -gt 0 ]; then + echo "The verdict gate held the verdict back ${held} time(s) for diffs not yet read." + fi + if [ "$unread" -gt 0 ]; then + echo + echo "Not read:" + jq -r '.[] | select(.read | not) | "- `\(.path)`"' "$STATE/coverage.json" + fi + } >>"$GITHUB_STEP_SUMMARY" + if [ "$unread" -eq 0 ]; then + echo "✓ The review read all ${total} non-generated diffs${passes}." + exit 0 + fi + + echo "::warning::The review did not read ${unread} of ${total} non-generated diffs to the end${passes}, so its verdict does not cover them. A comment on the PR names them, with an @claude prompt to review them." + short="${HEAD_SHA:0:7}" + names=$(jq -r '[.[] | select(.read | not) | "`\(.path)`"] | join(", ")' "$STATE/coverage.json") + body="$STATE/coverage-warning.md" + { + echo "> [!WARNING]" + echo "> **The automated review of ${short} did not read ${unread} of ${total} changed files**, so its verdict does not cover them:" + jq -r '.[] | select(.read | not) | "> - `\(.path)`"' "$STATE/coverage.json" + echo + echo "To have Claude review them, post this as a new comment:" + echo + echo '```' + echo "@claude Please review these files in this PR, which the automated review of ${short} did not read: ${names}." + echo + echo "**This is a review only. Don't push commits or edit files.** Reply on this PR with your findings. For each one, give the \`file:line\`, a concrete input or state that produces a wrong result, and why it's wrong. Then list what you read in full." + echo '```' + } >"$body" + if gh api -X POST "repos/${{ github.repository }}/issues/${PR_NUMBER}/comments" \ + -F "body=@${body}" >/dev/null; then + echo "✓ Posted the coverage warning on the PR." + else + echo "::warning::Could not post the coverage warning on the PR; the job summary still lists the files." + fi + # Turn the action's false-green into a hard failure: confirm a claude[bot] # review for THIS head commit was submitted during this run, and fail the # check loudly if not (the action exposes no "verdict posted" signal). A diff --git a/CLAUDE.md b/CLAUDE.md index c9709b3..9bb187f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -58,25 +58,54 @@ each other via `$GITHUB_OUTPUT`: 2. **Record review start time** — backdated 60s; the verify step only counts a verdict submitted after this, so a stale review at the same SHA is ignored. -3. **Compose review prompt** — bash assembles the prompt from fixed quoted - heredocs plus the `workflow_call` inputs (`extra_instructions`, - `required_check`, `generated_paths*`). The reviewing rules and the - verdict rules are fixed text; only the repo-specific sections are inputs. -4. **Run Claude Code Review** — `anthropics/claude-code-action@v1` with a +3. **Write per-file diffs** — `git diff` of the `pull_request` merge commit + against its first parent (checkout is `fetch-depth: 2`), one file per + changed path under `.claude-review/diffs/`; `generated_paths` (git globs) + get no diff. The review must not depend on `gh pr diff`: above Claude + Code's tool-output limit its output arrives as a 2 KB preview, and it + takes no path argument (#11). Also writes `coverage.jq`, the one + definition of "read": the lines successful Read calls *returned* (the + result's `startLine`/`numLines`) cover all of a diff's lines. Not the + call's `limit`: Read stops early at its 25,000-token cap, without an + error. +4. **Compose review prompt** — bash assembles the prompt from fixed quoted + heredocs, the list of diff files, and the `workflow_call` inputs + (`extra_instructions`, `required_check`, `generated_paths*`). The + reviewing rules and the verdict rules are fixed text; only the + repo-specific sections are inputs. +5. **Hold the verdict until every diff is read** — writes a Claude Code + PreToolUse hook, passed to the action as its `settings` input. While + any diff is unread, the hook refuses `gh pr review` and names the + unread diffs and their unread line ranges (`unread.jq`, also used by + the follow-up prompt). It refuses at most twice, and lets the verdict through + whenever it cannot tell. Prompt wording alone got 7–12 of 24 diffs read + on factory-farm-em#40. +6. **Run Claude Code Review** — `anthropics/claude-code-action@v1` with a narrow tool allowlist (Write, the inline-comment MCP tool, and - `gh pr review|diff|view`) and `Task` disallowed. The review **cannot read - CI** and the prompt tells it not to try. -5. **Verify a review verdict was posted** — the action reports success even + `gh pr review|diff|view`), `Task` disallowed, and `--effort high` + (Claude Code defaults Sonnet 5.5 to `medium`, which read about half the + diffs before trying to approve). Claude Code itself also lets read-only + Bash commands (`grep`, `sed -n`, `tail`) through, whatever the + allowlist says, and has no Grep or Glob tool. The prompt therefore asks + for `grep` to search and Read for everything else: coverage counts only + Read. The review **cannot read CI** and the prompt tells it not to try. +7. **Measure which diffs the review read** — from the execution file. +8. **Review the diffs the first pass did not read** — only when some are + unread: a second action run resumes the first session + (`--resume `), reads the rest, and files one more verdict for + the whole PR. `continue-on-error`, so it can never sink the first verdict. +9. **Report review coverage** — job summary over both passes; anything still + unread gets a `::warning::` and a PR comment with a ready-to-paste + `@claude` prompt (posted with `GITHUB_TOKEN`, so it triggers nothing). +10. **Verify a review verdict was posted** — the action reports success even with no verdict, so this step reads the reviews API itself: last *bodied* - claude review for this head SHA since the start time. `APPROVED` / - `CHANGES_REQUESTED` pass; `COMMENTED` (no binary verdict) fails the check; - `DISMISSED` is a notice; no verdict fails and dumps the agent's last turn. - For an approval with `required_check` set, the **contradiction guard** - polls the Actions API (needs the caller's `actions: read`; without it, - warn and skip) and fails the check if that job failed on this commit. -6. **Request human review** — on `COMMENTED`, `NOT_REVIEWED`, or a - contradiction: apply `escalation_label` and request `escalate_to` - reviewers, minus the PR author (GitHub 422s) and anyone already pending. + claude review for this head SHA since the start time — the follow-up's, + when one ran. `APPROVED` / `CHANGES_REQUESTED` pass; `COMMENTED` (no + binary verdict) fails the check; `DISMISSED` is a notice; no verdict fails + and dumps the agent's last turn. +11. **Request human review** — on `COMMENTED` or `NOT_REVIEWED`: apply + `escalation_label` and request `escalate_to` reviewers, minus the PR + author (GitHub 422s) and anyone already pending. `claude-mention.yml` is the near-stock action with `contents: write` so `@claude` can push commits when asked; it is deliberately broader than the @@ -100,6 +129,14 @@ reintroduce. - **A new input** needs a description, a safe empty default, and README documentation in the same PR. Verdict rules, the allowlist, and the verification logic are intentionally *not* inputs. +- **Coverage is report-only.** The measurement, the follow-up pass and the + warning comment never fail the check or gate the verdict — decided + 2026-10-08 (#11): tying the verdict to coverage would block too many PRs. + The verdict gate's hook decides only *when* the verdict goes out. It + must keep its cap on refusals and keep letting the verdict through + whenever it errors. +- **The follow-up step repeats the first pass's `claude_args`** (allowlist + and `--effort`). Change both together. - **`show_full_output: true` must already be on `main`** before a failure you need to diagnose; PRs editing the workflow self-skip. @@ -116,8 +153,9 @@ reintroduce. moving them breaks every install until the skill is updated. - The README **Changelog** records every behavioral change to `v1` with its date and the incident behind it. Add an entry with the change. -- `animal-welfare-data-pipeline` (the origin repo) still runs its own copies - of these workflows until it migrates to a caller; fixes belong in both. +- `animal-welfare-data-pipeline` (the origin repo) calls both shared + workflows at `@v1` since 2026-09-21 (its #169); it has no copies left to + keep in sync. - This repo must stay **public**: outside-org consumers resolve `uses: sentfutures/devops/...` cross-owner. diff --git a/README.md b/README.md index 0eb4066..021db83 100644 --- a/README.md +++ b/README.md @@ -17,13 +17,21 @@ logic each encode a real incident from that period — the inline comments in On every pull request, claude[bot]: -1. reads the diff and leaves **inline comments** on specific lines; +1. reads the diff of **every changed file** — each saved as its own diff + file, with your `generated_paths` left out — and leaves **inline + comments** on specific lines; 2. files exactly one **verdict** — approve, or request changes — with a summary that must agree with itself (a "minor nit" summary files as an approval, never as a limbo comment); 3. is **checked up on**: a verification step confirms a verdict for the current commit actually landed, and fails the `review / claude-review` - check if the bot looked but wouldn't commit to a verdict; + check if the bot looked but wouldn't commit to a verdict. A coverage + check counts which diffs it actually read. The bot's verdict is held + back (at most twice) until it has read every diff, and a follow-up pass + reads any it still skipped. Anything left unread after that is named in + a PR comment with a ready-to-paste `@claude` prompt. Coverage is + report-only — it never fails the check, and a verdict is never + withheld or downgraded for unread files; 4. **escalates to humans** when it can't supply a verdict: the `needs-human-review` label plus a review request to the maintainers named in your caller. @@ -124,7 +132,9 @@ the GitHub web editor. Most customization belongs in `extra_instructions`: ``` All inputs: `required_check`, `generated_paths` + `generated_paths_note` -(skim committed generated data instead of analyzing it), `escalate_to`, +(leave bulk committed generated data out of the review — whitespace-separated +git globs such as `outputs/**`; not for lockfiles, whose small diffs are +where dependency changes show), `escalate_to`, `escalation_label` (default `needs-human-review`), `extra_instructions`, `model`. Each is documented at the top of `claude-pr-review.yml`. What is *not* an input — the verdict rules, the tool allowlist, the verification @@ -233,6 +243,7 @@ Three tiers, in order of how often they should happen: | `gh api` 403s in the verify or escalation steps | The caller's `permissions:` block was trimmed — restore it exactly as in `callers/claude-pr-review.caller.yml`. | | Bot never runs on a PR | Fork PRs are skipped by design — comment `@claude please review this PR`. Also check the PR event types in your caller match the template's. | | No `review / claude-review` check appears on the PR at all (or a required one sits at "Expected — waiting for status"), and the Actions tab shows the run as `startup_failure` — "This run likely failed because of a workflow file issue" | The workflow failed before any job started, so nothing could report; open the run for the one-line reason. Seen so far: (a) the caller's `permissions:` block granted less than the called job declared — fixed 2026-09-29 by making the job inherit the caller's grant, so a caller on `@v1` can no longer hit this; (b) a PR branch carrying an old copy of the caller. GitHub reads the workflow from the PR's *merge ref*, which it does not refresh after an automatic base change, so rebase the branch onto the default branch — that push refreshes it. | +| A PR comment from github-actions says "The automated review of `` did not read N of M changed files" | The first pass skipped those diffs and the follow-up pass did not finish them (the job summary's "Review coverage" and the "Review the diffs the first pass did not read" log say why). The verdict stands but does not cover those files: post the comment's `@claude` prompt, or review them yourself. If it keeps happening, open an issue here with the run link. | | `review / claude-review` is red with "no binary verdict" | Working as designed: the bot looked and wouldn't decide; the PR now carries `needs-human-review` and a human review request. A human reviews, then dismisses or supersedes. | | Review summary says it could not read `statusCheckRollup` / CI ("Resource not accessible by integration"), or caveats its verdict on not having seen the tests | The review is told it cannot read CI and must not try or caveat (the prompt block in `claude-pr-review.yml`). If that wording is back, the prompt on `v1` has regressed (it happened 2026-09-17 to 21): roll `v1` back per `release.yml`'s header and open an issue here. Nothing to fix in your caller — `required_check` is prompt context only. | @@ -279,6 +290,58 @@ admin involved — this is exactly how animal-welfare-data-pipeline was set up ## Changelog +- **v1, 2026-10-09** — the review reads every non-generated diff, and is + checked for it (#11). On Deco354/factory-farm-em#29 (25 files, ~150 KB of + diff) all seven approvals between 2026-10-06 and 10-08 said they had read + only part of the PR, and none had read `analysis/summary.py`, which + computes its headline numbers; `@claude` reviews of the unread files then + found a bug the approvals had missed. Across the five consuming repos, 15 + of 134 approvals since 2026-09-01 admitted unread code. Cause: `gh pr diff` + was the review's only view of the diff; above Claude Code's tool-output + limit it arrives as a 2 KB preview plus a saved file, it takes no path + argument, and nothing told the review to open the file. Sonnet 5 (the + action's default until late September) opened it in 2 of 2 sampled runs, + Sonnet 5.5 in 1 of 9. Now the workflow writes one diff per changed file + (`git diff` of the merge commit; checkout is `fetch-depth: 2`) and lists + them in the prompt; `generated_paths` files get no diff and are listed by + name and size only, so `generated_paths` is now whitespace-separated git + globs (both values in use on 2026-10-09, `outputs/**` and `uv.lock`, + already are). Withheld now means unseen, not skimmed, so lockfiles are no + longer suggested for it: factory-farm-em's `uv.lock` changes ran 2–56 + lines, and it is the only place indirect dependency and source changes + show; Deco354/factory-farm-em#44 stops withholding it. The + list alone was not enough: in a replay of #29 (factory-farm-em#40) the + first pass read 12, 12 and 7 of 24 diffs under three prompt wordings, + skipping every test and doc. So a Claude Code hook, passed through the + action's `settings` input, refuses the review's `gh pr review` while any + diff is unread and names what is left, down to the unread lines (Sonnet 5 + twice paged to a few lines short of a 4,890-line diff's end and, told + only "not read to the end", was held back twice); it refuses at most + twice. The prompt asks for long diffs about 800 lines at a time. A step + counts the lines each diff's Read calls returned — not the lines they + asked for: Read stops at 25,000 tokens without an error, and on + 2026-10-09 a 2,000-line diff came back as lines 1–935. If any diff is + unread, a second action run resumes the review's session to read the + rest and file one more verdict for the whole PR, which the verify step + checks as before. Anything still unread gets a PR comment with a + ready-to-paste `@claude` prompt. Coverage is report-only by decision: + tying the verdict to it would block too many PRs. The review body is + told to describe the change, not how it was reviewed: the gated + approvals on #40 opened with "I read every diff in this PR to its last + line". Both passes run at `--effort high`. Claude Code defaults Sonnet + 5.5 to `medium` effort, and Sonnet 5 to `high`, so effort changed along + with the action's default model. On #40, Sonnet 5.5 read 9–13 of 25 + diffs before its first verdict attempt at `medium`, and 24 of 25 at + `high`. `high` alone is not enough: on the old workflow it still + approved #40 after reading 0 and 5 of 25 diffs. The prompt no longer + names the Grep and Glob tools, which Claude Code no longer has. It asks + for `grep` through Bash to search, and Read for everything else: Claude + Code now lets read-only Bash commands such as `sed -n` through despite + the allowlist, and coverage counts only Read. Cost: a full read is what + September's reviews cost (18–24 turns and $0.49–0.99 on + 2,000–6,000-line PRs, against ~10 turns and ~$0.13 for the partial + reads); on #40, with a 4,890-line test file added, about $1.00. No input + renamed; callers need no change. - **v1, 2026-09-30** — the post-approval CI cross-check is removed, and `required_check` is now prompt context only. The verify step had polled the named job for a fixed 90s after an approval and failed `review / diff --git a/callers/claude-pr-review.caller.yml b/callers/claude-pr-review.caller.yml index c586412..2a1054d 100644 --- a/callers/claude-pr-review.caller.yml +++ b/callers/claude-pr-review.caller.yml @@ -43,7 +43,9 @@ jobs: # extra_instructions: | # Also flag: # - Broken internal links in changed HTML - # Optional — skim committed generated data instead of analyzing it: + # Optional — leave bulk generated data (e.g. committed run outputs) out + # of the review entirely; it is listed by name and size only. Not for + # lockfiles: their diffs are small and show dependency changes. # generated_paths: "outputs/**" # generated_paths_note: "See CLAUDE.md for why this data is committed." secrets: