From 6025705fac92802078fb1f1daf6104b10b8eefd0 Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Thu, 8 Oct 2026 15:13:44 +0100 Subject: [PATCH 01/15] Review bot: read every non-generated diff, and check that it did On Deco354/factory-farm-em#29 all seven approvals (2026-10-06..08) said they had read only part of the PR; none read `analysis/summary.py`, which computes its headline numbers, and `@claude` reviews of the unread files then found a bug the approvals had missed. 15 of 134 approvals across the consuming repos since 2026-09-01 admit unread code (#11). 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. All seven #29 runs tried a per-file diff, were refused, and approved after reading one to three files. Sonnet 5 (the action's default until late September) opened the saved file in 2 of 2 sampled runs, Sonnet 5.5 in 1 of 9. - Write per-file diffs: `git diff` of the pull_request merge commit against its base parent (checkout is now fetch-depth 2), one file per changed path under .claude-review/diffs/. generated_paths become exclude pathspecs (whitespace-separated git globs; a slashless pattern matches at any depth), so generated files get no diff and are listed by name and size only. Binary files are listed, not diffed. - The prompt lists every diff file and asks for each to be read to its last line; it no longer points at `gh pr diff` or the skim rule. - Measure which diffs the review read, from the execution file: a diff is read only when successful Read calls covered every line. - If any are unread, a second action run resumes the review's session (`--resume` with the action's `session_id` output) to read the rest and file one more verdict for the whole PR. continue-on-error, so it cannot sink the first verdict; the verify step checks the latest verdict as before, unchanged. - Report coverage in the job summary; anything still unread gets a PR comment with a ready-to-paste `@claude` prompt (posted with GITHUB_TOKEN, so it triggers nothing). Coverage is report-only by decision: tying the verdict to it would block too many PRs. Not done: pinning the model, re-enabling Task (PR #73's stall), demoting approvals after the fact. Also in CLAUDE.md: the step list gains the new steps and loses the contradiction guard removed on 2026-09-30, and the pipeline repo is no longer described as running its own copies (it calls @v1 since its #169). --- .github/workflows/claude-pr-review.yml | 376 +++++++++++++++++++++++-- CLAUDE.md | 54 ++-- README.md | 39 ++- 3 files changed, 427 insertions(+), 42 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index fa4a221..dc84198 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -23,17 +23,20 @@ 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/**" or "uv.lock". 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. 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 +129,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 +143,102 @@ 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`. + read -r -a pats <<<"${GENERATED_PATHS}" + 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" + + 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,36 +257,66 @@ 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"). 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 with the + Read tool. 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. To read + other 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. BLOCK - if [ -n "$GENERATED_PATHS" ]; then + if [ "$DIFF_COUNT" -gt 0 ]; then + read -r -d '' T <<'BLOCK' || true + Changed files (__COUNT__). Read EVERY diff below, each one to its + last line, before you submit a verdict — not a sample, and not only + the files that look riskiest. Read returns 2,000 lines at a time by + default: for a longer diff, call it again with `offset` and `limit` + until you reach the end. 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 [ -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}}" 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' @@ -347,6 +479,206 @@ jobs: # 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 agent's successful + # Read calls covered every one of its lines. Read returns 2,000 lines + # per call unless given offset/limit, and 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. + - 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' + 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" + + cat >"$STATE/coverage.jq" <<'JQ' + # Input (-s): one execution file per review pass, each an array of + # SDK messages. $m: the manifest, one {path, diff, lines} per diff. + # Output: the manifest with `read` set on every entry. + def covers($n): # input: [[first, last], ...] line ranges read + ($n == 0) or + (reduce sort[] as $r (0; if $r[0] <= . + 1 then ([., $r[1]] | max) else . end) >= $n); + (add // []) as $msgs + | [ $msgs[] | select(.type == "user") | .message.content[]? + | select(.type == "tool_result" and (.is_error // false)) | .tool_use_id ] as $failed + | [ $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 + | ([(.input.offset // 1 | tonumber), 1] | max) as $first + | { file: (if ($f | startswith("/")) then $f else "\(env.GITHUB_WORKSPACE)/\($f)" end), + range: [$first, $first + (.input.limit // 2000 | tonumber) - 1] } ] as $reads + | $m | map(. as $e | . + { read: ([ $reads[] | select(.file == $e.diff) | .range ] | covers($e.lines)) }) + JQ + + 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" + echo "The review read $((total - unread)) of ${total} diffs to the end." + if [ "$unread" -eq 0 ]; then + exit 0 + fi + list=$(jq -r '.[] | select(.read | not) | "- `\(.path)`: `\(.diff)` (\(.lines) lines)"' "$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 each of them now, every line, with the Read tool (2,000 lines + at a time by default; use `offset` and `limit` to page through a + longer one), plus 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, and say in it which + files this pass added. 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 + # allowlists identical — 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.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" + ${{ 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 + { + echo "### Review coverage" + echo + echo "Read **$((total - unread)) of ${total}** non-generated diffs to the end${passes}." + 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..6e0e33a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -58,25 +58,39 @@ 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). +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. **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 +6. **Measure which diffs the review read** — from the execution file: a diff + counts as read only when successful Read calls covered all its lines. +7. **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. +8. **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). +9. **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. +10. **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 +114,11 @@ 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 follow-up step repeats the first pass's `claude_args` allowlist.** + 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 +135,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..799e849 100644 --- a/README.md +++ b/README.md @@ -17,13 +17,19 @@ 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: a follow-up pass reads any it + skipped, and anything still unread is named in a PR comment with a + ready-to-paste `@claude` prompt. Coverage is report-only — it never fails + the check or changes the verdict; 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 +130,8 @@ 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 committed generated data out of the review — whitespace-separated git +globs such as `outputs/**` or `uv.lock`), `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 +240,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 +287,31 @@ admin involved — this is exactly how animal-welfare-data-pipeline was set up ## Changelog +- **v1, 2026-10-08** — 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 live values, `outputs/**` and `uv.lock`, already are). A step + counts the lines each diff's Read calls covered; 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. 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). 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 / From fc59ec5a6f6981ed506fecfdf950292d74913d5e Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Thu, 8 Oct 2026 15:18:31 +0100 Subject: [PATCH 02/15] Review bot: keep the coverage measurement report-only if it breaks claude[bot]'s review of #12 noted that "Measure which diffs the review read" runs under set -euo pipefail, so an unexpected jq or cp failure would fail the job - against the report-only rule the PR documents. It is now continue-on-error, and the follow-up pass also requires it to have succeeded, so a half-written step can never start a follow-up with an empty prompt. --- .github/workflows/claude-pr-review.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index dc84198..3c84467 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -488,7 +488,9 @@ jobs: # 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. + # 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: >- @@ -496,6 +498,7 @@ jobs: 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 }} @@ -590,6 +593,7 @@ jobs: 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 != '' From 8bbedcb152e2410cb3f3da887f0b23fae673852c Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Thu, 8 Oct 2026 19:23:10 +0100 Subject: [PATCH 03/15] Review bot: read every diff first, tests and docs included On Deco354/factory-farm-em#40 (2026-10-08, 2 runs) the first pass read 12 of 24 diffs, most important first, and skipped all tests and docs. The prompt's older text invited that: "focus on ... the changed code only" and "you do not need [the tests]". The prompt now asks for every diff to be read up front, names tests, docs and configs as in scope, and limits the reporting bar and the CI paragraph to what they were for. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/claude-pr-review.yml | 37 ++++++++++++++++++-------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 3c84467..2f75613 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -271,6 +271,14 @@ jobs: # 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. cat >>"$P" <<'BLOCK' Review this pull request. The PR branch is already checked out in the current working directory, and the diff of each changed file @@ -285,12 +293,16 @@ jobs: if [ "$DIFF_COUNT" -gt 0 ]; then read -r -d '' T <<'BLOCK' || true - Changed files (__COUNT__). Read EVERY diff below, each one to its - last line, before you submit a verdict — not a sample, and not only - the files that look riskiest. Read returns 2,000 lines at a time by - default: for a longer diff, call it again with `offset` and `limit` - until you reach the end. Then read the surrounding source wherever - a hunk needs context. + 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 2,000 lines at a time by default: for a + longer diff, call it again with `offset` and `limit` until you + reach the end. Only then read the surrounding source wherever a + hunk needs context. BLOCK T="${T//__COUNT__/${DIFF_COUNT}}" printf '\n%s\n\n' "$T" >>"$P" @@ -321,7 +333,8 @@ jobs: 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 @@ -356,10 +369,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 From 4987b9c51e1537857d6d95798f29bac78d4953e1 Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Thu, 8 Oct 2026 20:02:27 +0100 Subject: [PATCH 04/15] Review bot: hold the verdict until every diff is read Prompt wording 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 wordings, and each follow-up pass, handed the list of unread diffs, read all of them. A PreToolUse hook, passed through the action's settings input, now refuses gh pr review while any diff is unread and names those diffs. It refuses at most twice, and lets the verdict through whenever it cannot tell. It controls when the verdict is posted, never what it says, and never fails the check. coverage.jq moves to "Write per-file diffs" so the hook and the coverage steps share one definition of read. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/claude-pr-review.yml | 126 ++++++++++++++++++++----- CLAUDE.md | 26 +++-- README.md | 16 +++- 3 files changed, 134 insertions(+), 34 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 2f75613..045d911 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -234,6 +234,29 @@ jobs: 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. + 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` set on every entry. + def covers($n): # input: [[first, last], ...] line ranges read + ($n == 0) or + (reduce sort[] as $r (0; if $r[0] <= . + 1 then ([., $r[1]] | max) else . end) >= $n); + (add // []) as $msgs + | [ $msgs[] | select(.type == "user") | .message.content[]? + | select(.type == "tool_result" and (.is_error // false)) | .tool_use_id ] as $failed + | [ $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 + | ([(.input.offset // 1 | tonumber), 1] | max) as $first + | { file: (if ($f | startswith("/")) then $f else "\(env.GITHUB_WORKSPACE)/\($f)" end), + range: [$first, $first + (.input.limit // 2000 | tonumber) - 1] } ] as $reads + | $m | map(. as $e | . + { read: ([ $reads[] | select(.file == $e.diff) | .range ] | covers($e.lines)) }) + 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 @@ -445,6 +468,79 @@ 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 '.[] | select(.read | not) | "- `\(.path)`: `\(.diff)` (\(.lines) lines)"') || 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 + each of these diffs now, to its last line, with the Read tool: all + of them in one turn if you can, using `offset` and `limit` past + 2,000 lines. 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. + 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 @@ -463,6 +559,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 @@ -525,32 +624,13 @@ jobs: # keep this pass's copy for the count over both passes. cp "$EXEC_FILE" "$STATE/exec-1.json" - cat >"$STATE/coverage.jq" <<'JQ' - # Input (-s): one execution file per review pass, each an array of - # SDK messages. $m: the manifest, one {path, diff, lines} per diff. - # Output: the manifest with `read` set on every entry. - def covers($n): # input: [[first, last], ...] line ranges read - ($n == 0) or - (reduce sort[] as $r (0; if $r[0] <= . + 1 then ([., $r[1]] | max) else . end) >= $n); - (add // []) as $msgs - | [ $msgs[] | select(.type == "user") | .message.content[]? - | select(.type == "tool_result" and (.is_error // false)) | .tool_use_id ] as $failed - | [ $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 - | ([(.input.offset // 1 | tonumber), 1] | max) as $first - | { file: (if ($f | startswith("/")) then $f else "\(env.GITHUB_WORKSPACE)/\($f)" end), - range: [$first, $first + (.input.limit // 2000 | tonumber) - 1] } ] as $reads - | $m | map(. as $e | . + { read: ([ $reads[] | select(.file == $e.diff) | .range ] | covers($e.lines)) }) - JQ - 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" - echo "The review read $((total - unread)) of ${total} diffs to the end." + 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 @@ -659,10 +739,14 @@ jobs: 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:" diff --git a/CLAUDE.md b/CLAUDE.md index 6e0e33a..905332f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,32 +63,39 @@ each other via `$GITHUB_OUTPUT`: 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). + takes no path argument (#11). Also writes `coverage.jq`, the one + definition of "read": successful Read calls covered all of a diff's + lines. 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. **Run Claude Code Review** — `anthropics/claude-code-action@v1` with a +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. 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. -6. **Measure which diffs the review read** — from the execution file: a diff - counts as read only when successful Read calls covered all its lines. -7. **Review the diffs the first pass did not read** — only when some are +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. -8. **Report review coverage** — job summary over both passes; anything still +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). -9. **Verify a review verdict was posted** — the action reports success even +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 — 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. -10. **Request human review** — on `COMMENTED` or `NOT_REVIEWED`: apply +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. @@ -117,6 +124,9 @@ reintroduce. - **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.** Change both together. - **`show_full_output: true` must already be on `main`** before a failure diff --git a/README.md b/README.md index 799e849..dd04461 100644 --- a/README.md +++ b/README.md @@ -26,10 +26,11 @@ On every pull request, claude[bot]: 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. A coverage - check counts which diffs it actually read: a follow-up pass reads any it - skipped, and anything still unread is named in a PR comment with a - ready-to-paste `@claude` prompt. Coverage is report-only — it never fails - the check or changes the verdict; + 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 or changes the verdict; 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. @@ -302,7 +303,12 @@ admin involved — this is exactly how animal-welfare-data-pipeline was set up (`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 live values, `outputs/**` and `uv.lock`, already are). A step + globs (both live values, `outputs/**` and `uv.lock`, already are). 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; it refuses at most twice. A step counts the lines each diff's Read calls covered; 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. From 87cb7ca178c6e6b77bde72a8bee46df9e3345fa4 Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Thu, 8 Oct 2026 20:07:56 +0100 Subject: [PATCH 05/15] README: coverage never gates the verdict (the follow-up can file a new one) From the selftest review of 4987b9c: "never changes the verdict" was wrong, because the follow-up pass files a verdict of its own. Co-Authored-By: Claude Opus 5.5 --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index dd04461..bc384b9 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,8 @@ On every pull request, claude[bot]: 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 or changes the verdict; + 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. From 56015339838f8390272f6dc0c6331f87f0f7036f Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Fri, 9 Oct 2026 10:32:04 +0100 Subject: [PATCH 06/15] Review bot: count the lines Read returned; keep process out of the summary Read stops at its 25,000-token cap without an error: on 2026-10-09 a 2,000-line diff came back as lines 1-935 (truncatedByTokenCap). The coverage count trusted the call's default 2,000-line limit, so it scored that diff as read, the gate let the verdict through and no follow-up ran. It now takes each range from the result's startLine/numLines, falling back to offset/limit only when a result has no structured form. #40's largest diff was 543 lines, so the live runs never hit this. The prompt, the gate's message and the follow-up prompt no longer say Read returns 2,000 lines at a time. The review body is told to describe the change, not how it was reviewed: all three gated approvals on factory-farm-em#40 opened with "I read every diff ... to its last line". Inline comments go on the changed file's own lines, not the diff file's. --- .github/workflows/claude-pr-review.yml | 82 +++++++++++++++++++------- CLAUDE.md | 6 +- README.md | 22 ++++--- 3 files changed, 79 insertions(+), 31 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 045d911..f61027b 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -235,7 +235,14 @@ jobs: 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 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. @@ -247,13 +254,27 @@ jobs: (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 - | ([(.input.offset // 1 | tonumber), 1] | max) as $first + | $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: [$first, $first + (.input.limit // 2000 | tonumber) - 1] } ] as $reads + range: $range } ] as $reads | $m | map(. as $e | . + { read: ([ $reads[] | select(.file == $e.diff) | .range ] | covers($e.lines)) }) JQ @@ -322,9 +343,10 @@ jobs: 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 2,000 lines at a time by default: for a - longer diff, call it again with `offset` and `limit` until you - reach the end. Only then read the surrounding source wherever a + does not do. Read returns a long diff a page at a time (at most + 2,000 lines, fewer when the lines are long, and it says where it + stopped): call it again with `offset` and `limit` until you reach + the last line. Only then read the surrounding source wherever a hunk needs context. BLOCK T="${T//__COUNT__/${DIFF_COUNT}}" @@ -411,14 +433,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: @@ -526,10 +561,13 @@ jobs: cat <<'MSG' This is not an error, and nothing is wrong with the command. Read each of these diffs now, to its last line, with the Read tool: all - of them in one turn if you can, using `offset` and `limit` past - 2,000 lines. 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. + of them in one turn if you can. Read returns a long diff a page at + a time, so keep calling it with `offset` and `limit` until you + reach the 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 @@ -595,9 +633,9 @@ jobs: # 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 agent's successful - # Read calls covered every one of its lines. Read returns 2,000 lines - # per call unless given offset/limit, and a #29 run read lines 1-80 of + # 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 @@ -643,15 +681,19 @@ jobs: __LIST__ - Read each of them now, every line, with the Read tool (2,000 lines - at a time by default; use `offset` and `limit` to page through a - longer one), plus any surrounding source a hunk needs. Leave an - inline comment for each specific issue, as before. + Read each of them now, every line, with the Read tool (it returns a + long diff a page at a time; keep calling it with `offset` and + `limit` until you reach the last line), plus 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, and say in it which - files this pass added. Run ONE of these — nothing else: + 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: diff --git a/CLAUDE.md b/CLAUDE.md index 905332f..ec513ed 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,8 +64,10 @@ each other via `$GITHUB_OUTPUT`: 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": successful Read calls covered all of a diff's - lines. + 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 diff --git a/README.md b/README.md index bc384b9..5ff92a8 100644 --- a/README.md +++ b/README.md @@ -310,15 +310,19 @@ admin involved — this is exactly how animal-welfare-data-pipeline was set up 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; it refuses at most twice. A step - counts the lines each diff's Read calls covered; 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. 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). No input renamed; callers need no - change. + 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". 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). 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 / From 40db5eea5c51d776cdf4fb29a55f7dac9651f15b Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Fri, 9 Oct 2026 10:38:42 +0100 Subject: [PATCH 07/15] README: date the changelog entry 2026-10-09 (it cites a 10-09 finding) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 5ff92a8..d6cbccc 100644 --- a/README.md +++ b/README.md @@ -289,7 +289,7 @@ admin involved — this is exactly how animal-welfare-data-pipeline was set up ## Changelog -- **v1, 2026-10-08** — the review reads every non-generated diff, and is +- **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 From c89d87197d7b692338709c636bd91938bca65a6e Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Fri, 9 Oct 2026 11:07:27 +0100 Subject: [PATCH 08/15] Review bot: name the unread lines when holding the verdict; page ~800 lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate's refusal and the follow-up prompt now list each unread diff's unread line ranges ("not read yet: line 4890"), not just "not read to the end". On factory-farm-em#40 (2026-10-09, 2 runs) Sonnet 5 paged to line 4889 and to 4883 of a 4,890-line diff, re-read the same range when told only the file name, was held back twice, and needed the follow-up pass and a second approval each time. The prompt and both messages ask for long diffs about 800 lines per call, starting at the first unread line: Sonnet 5.5 asked for 2,000, 1,400 and 1,100 lines of that diff, each over Read's 25,000-token cap, before 900 worked. The prompt also points at the "diff lines" count, not +/−. --- .github/workflows/claude-pr-review.yml | 71 ++++++++++++++++++-------- CLAUDE.md | 3 +- README.md | 5 +- 3 files changed, 56 insertions(+), 23 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index f61027b..ac92074 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -247,10 +247,14 @@ jobs: # 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` set on every entry. - def covers($n): # input: [[first, last], ...] line ranges read - ($n == 0) or - (reduce sort[] as $r (0; if $r[0] <= . + 1 then ([., $r[1]] | max) else . end) >= $n); + # 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 @@ -275,7 +279,25 @@ jobs: 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 | . + { read: ([ $reads[] | select(.file == $e.diff) | .range ] | covers($e.lines)) }) + | $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" @@ -323,6 +345,12 @@ jobs: # 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. cat >>"$P" <<'BLOCK' Review this pull request. The PR branch is already checked out in the current working directory, and the diff of each changed file @@ -343,11 +371,12 @@ jobs: 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 a long diff a page at a time (at most - 2,000 lines, fewer when the lines are long, and it says where it - stopped): call it again with `offset` and `limit` until you reach - the last line. Only then read the surrounding source wherever a - hunk needs context. + 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" @@ -550,7 +579,7 @@ jobs: # 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 '.[] | select(.read | not) | "- `\(.path)`: `\(.diff)` (\(.lines) lines)"') || exit 0 + jq -r -f "$state/unread.jq") || exit 0 [ -n "$list" ] || exit 0 echo $((held + 1)) >"$state/gate-held" { @@ -560,10 +589,10 @@ jobs: echo cat <<'MSG' This is not an error, and nothing is wrong with the command. Read - each of these diffs now, to its last line, with the Read tool: all - of them in one turn if you can. Read returns a long diff a page at - a time, so keep calling it with `offset` and `limit` until you - reach the last line. Leave an inline comment for each significant + 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 @@ -672,7 +701,7 @@ jobs: if [ "$unread" -eq 0 ]; then exit 0 fi - list=$(jq -r '.[] | select(.read | not) | "- `\(.path)`: `\(.diff)` (\(.lines) lines)"' "$STATE/coverage.json") + list=$(jq -r -f "$STATE/unread.jq" "$STATE/coverage.json") echo "$list" read -r -d '' T <<'BLOCK' || true @@ -681,11 +710,11 @@ jobs: __LIST__ - Read each of them now, every line, with the Read tool (it returns a - long diff a page at a time; keep calling it with `offset` and - `limit` until you reach the last line), plus any surrounding source - a hunk needs. Leave an inline comment for each specific issue, as - before. + 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. diff --git a/CLAUDE.md b/CLAUDE.md index ec513ed..d5e3569 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,7 +76,8 @@ each other via `$GITHUB_OUTPUT`: 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. It refuses at most twice, and lets the verdict through + 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 diff --git a/README.md b/README.md index d6cbccc..3150998 100644 --- a/README.md +++ b/README.md @@ -309,7 +309,10 @@ admin involved — this is exactly how animal-welfare-data-pipeline was set up 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; it refuses at most twice. A step + 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 From 4cefbcb7a0fa45f5a3150ec0a166d50a62f4f081 Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Fri, 9 Oct 2026 13:28:46 +0100 Subject: [PATCH 09/15] TEMP effort test: --effort high on both passes [skip ci] --- .github/workflows/claude-pr-review.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index ac92074..6267a87 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -656,6 +656,7 @@ jobs: 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 @@ -774,6 +775,7 @@ jobs: --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 From 5e4c6919bdd2ff83a194873148351abafb7c9c89 Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Fri, 9 Oct 2026 13:34:13 +0100 Subject: [PATCH 10/15] TEMP effort test: main's workflow (no per-file diffs) + --effort high [skip ci] --- .github/workflows/claude-pr-review.yml | 563 ++----------------------- 1 file changed, 28 insertions(+), 535 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 6267a87..81b1856 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -23,20 +23,17 @@ on: default: "" generated_paths: description: >- - Whitespace-separated git glob pattern(s) for committed generated - data, e.g. "outputs/**" or "uv.lock". 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. Empty: every changed text file is - reviewed. + 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. type: string required: false default: "" generated_paths_note: description: >- - 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. + 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. type: string required: false default: "" @@ -129,10 +126,7 @@ jobs: - name: Checkout repository uses: actions/checkout@v4 with: - # 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 + fetch-depth: 1 # 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). @@ -143,168 +137,6 @@ 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`. - read -r -a pats <<<"${GENERATED_PATHS}" - 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 @@ -323,92 +155,41 @@ 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. cat >>"$P" <<'BLOCK' Review this pull request. The PR branch is already checked out in - the current working directory, and the diff of each changed file - is saved as its own file, listed below. Read the diffs with the - Read tool. 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. To read - other 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. 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. 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 [ -s "$STATE/generated.md" ]; then + if [ -n "$GENERATED_PATHS" ]; then read -r -d '' T <<'BLOCK' || true - 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. + 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. BLOCK note="" if [ -n "$GENERATED_PATHS_NOTE" ]; then note=" ${GENERATED_PATHS_NOTE}"; fi T="${T//__PATHS__/${GENERATED_PATHS}}" T="${T//__NOTE__/${note}}" - 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" + printf '\n%s\n' "$T" >>"$P" fi cat >>"$P" <<'BLOCK' - Report only high-signal problems in the changed files (this bar is - for what you report, not for what you read): + Focus on high-signal problems in the changed code only: - Bugs, logic errors, and unhandled edge cases - Security issues - Clear violations of guidelines in any relevant CLAUDE.md (root or a @@ -443,12 +224,10 @@ 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 - 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. + 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. BLOCK T="${T//__CHECK__/${REQUIRED_CHECK}}" else @@ -462,27 +241,14 @@ 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 - of the changed file: its own path and line number, not the diff - file's). + for each specific issue (one comment per issue, on the relevant line). 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: @@ -532,82 +298,6 @@ 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 @@ -626,9 +316,6 @@ 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 @@ -661,200 +348,6 @@ jobs: # 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 - # allowlists identical — 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 From f47e495bb1444eec6280ae3a23cc3fa30748edde Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Fri, 9 Oct 2026 13:38:34 +0100 Subject: [PATCH 11/15] Revert the temporary effort-test commits (4cefbcb, 5e4c691) --- .github/workflows/claude-pr-review.yml | 563 +++++++++++++++++++++++-- 1 file changed, 534 insertions(+), 29 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 81b1856..ac92074 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -23,17 +23,20 @@ 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/**" or "uv.lock". 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. 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 +129,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 +143,168 @@ 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`. + read -r -a pats <<<"${GENERATED_PATHS}" + 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 +323,92 @@ 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. 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 with the + Read tool. 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. To read + other 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. BLOCK - if [ -n "$GENERATED_PATHS" ]; then + if [ "$DIFF_COUNT" -gt 0 ]; 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. + 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 [ -s "$STATE/generated.md" ]; then + read -r -d '' T <<'BLOCK' || true + 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}}" 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 +443,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 +462,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 +532,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 +626,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 @@ -343,11 +656,203 @@ jobs: 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 + # allowlists identical — 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" + ${{ 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 From af20b0f1514d70e05a18c98f6c16c3e85ef7f4a4 Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Fri, 9 Oct 2026 13:48:17 +0100 Subject: [PATCH 12/15] Review bot: run both passes at --effort high; fix the prompt's tool list Sonnet 5.5 defaults to medium effort and read 9-13 of 25 diffs before its first verdict attempt on factory-farm-em#40; at high it read 24 of 25, as Sonnet 5 did. The prompt named Grep and Glob, which Claude Code no longer has, and claimed every other Bash command is denied, which read-only commands no longer are. --- .github/workflows/claude-pr-review.yml | 39 ++++++++++++++++++++------ CLAUDE.md | 13 ++++++--- README.md | 17 +++++++++-- 3 files changed, 54 insertions(+), 15 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index ac92074..67fa5b0 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -351,16 +351,26 @@ jobs: # 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 had no Grep or Glob tool since at least + # Claude Code 2.1.293 (factory-farm-em#40, 2026-10-08); a call is + # refused with "search file contents with `grep` via the Bash tool + # instead". On 2.1.295, read-only Bash commands (`grep`, `sed -n`, + # `tail`) run despite the `gh pr`-only allowlist, while `python3 -c` + # is still refused. Diffs are asked for through Read because the + # coverage step counts only Read calls: Sonnet 5 read parts of a + # diff with `sed -n` and `tail` on #40 (2026-10-09, run + # 37914095042), lines that then showed as unread. cat >>"$P" <<'BLOCK' Review this pull request. The PR branch is already checked out in the current working directory, and the diff of each changed file - is saved as its own file, listed below. Read the diffs with the - Read tool. 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. To read - other 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. + 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 @@ -653,9 +663,20 @@ 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. 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 @@ -754,7 +775,8 @@ jobs: # what is still unread. # # `with:` mirrors "Run Claude Code Review". Keep the two claude_args - # allowlists identical — the comments on the first explain each entry. + # 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: >- @@ -774,6 +796,7 @@ jobs: --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 diff --git a/CLAUDE.md b/CLAUDE.md index d5e3569..9bb187f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -82,8 +82,13 @@ each other via `$GITHUB_OUTPUT`: 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. + `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 @@ -130,8 +135,8 @@ reintroduce. 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.** - Change both together. +- **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. diff --git a/README.md b/README.md index 3150998..25d81ef 100644 --- a/README.md +++ b/README.md @@ -323,9 +323,20 @@ admin involved — this is exactly how animal-welfare-data-pipeline was set up 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". 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). No input renamed; callers need no change. + 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 / From 5792475f349f369cfece53e707d6182a22b20ccd Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Fri, 9 Oct 2026 13:52:37 +0100 Subject: [PATCH 13/15] Review bot: cite the documented causes in the tool and effort comments --- .github/workflows/claude-pr-review.yml | 29 +++++++++++++++++--------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 67fa5b0..b832add 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -352,15 +352,22 @@ jobs: # 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 had no Grep or Glob tool since at least - # Claude Code 2.1.293 (factory-farm-em#40, 2026-10-08); a call is - # refused with "search file contents with `grep` via the Bash tool - # instead". On 2.1.295, read-only Bash commands (`grep`, `sed -n`, - # `tail`) run despite the `gh pr`-only allowlist, while `python3 -c` - # is still refused. Diffs are asked for through Read because the - # coverage step counts only Read calls: Sonnet 5 read parts of a - # diff with `sed -n` and `tail` on #40 (2026-10-09, run - # 37914095042), lines that then showed as unread. + # 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, and the diff of each changed file @@ -672,7 +679,9 @@ jobs: # 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. + # 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" From 6b3f2219c04550ce3b6ea1064d700c5255a186ef Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Fri, 9 Oct 2026 14:37:08 +0100 Subject: [PATCH 14/15] Review bot docs: generated_paths leaves files out entirely; not for lockfiles Under this PR a generated_paths file gets no diff, so the review cannot see it at all; before, its hunks were in the diff it was told to skim. A lockfile's changes are small (factory-farm-em's uv.lock: 2-56 lines) and are the only place indirect dependency and source changes show, so the input description, README and caller template stop suggesting uv.lock, and the template's 'skim' comment is corrected. Mirrored in sentfutures/.github#4; factory-farm-em#44 stops withholding its uv.lock. --- .github/workflows/claude-pr-review.yml | 12 +++++++----- README.md | 11 ++++++++--- callers/claude-pr-review.caller.yml | 4 +++- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index b832add..989491c 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -24,11 +24,13 @@ on: generated_paths: description: >- Whitespace-separated git glob pattern(s) for committed generated - data, e.g. "outputs/**" or "uv.lock". 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. Empty: every changed text file is - reviewed. + data, e.g. "outputs/**". 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: "" diff --git a/README.md b/README.md index 25d81ef..021db83 100644 --- a/README.md +++ b/README.md @@ -132,8 +132,9 @@ the GitHub web editor. Most customization belongs in `extra_instructions`: ``` All inputs: `required_check`, `generated_paths` + `generated_paths_note` -(leave committed generated data out of the review — whitespace-separated git -globs such as `outputs/**` or `uv.lock`), `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 @@ -304,7 +305,11 @@ admin involved — this is exactly how animal-welfare-data-pipeline was set up (`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 live values, `outputs/**` and `uv.lock`, already are). The + 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 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: From d2dc44aff7cefd4b227fc3da5149d4e80b5c0f3c Mon Sep 17 00:00:00 2001 From: Declan McKenna Date: Fri, 9 Oct 2026 14:43:21 +0100 Subject: [PATCH 15/15] Review bot: read every line of a multi-line generated_paths A plain `read -a` stops at the first newline, so a YAML | block with one pattern per line withheld only the first pattern and silently reviewed the rest (devops#12 selftest note). `read -d ''` reads the whole value; the prompt shows the patterns on one line. --- .github/workflows/claude-pr-review.yml | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 989491c..f79a272 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -24,8 +24,9 @@ on: generated_paths: description: >- Whitespace-separated git glob pattern(s) for committed generated - data, e.g. "outputs/**". A pattern with no slash matches at any - depth, as in .gitignore. Matching files are left out of the + 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 @@ -184,8 +185,12 @@ jobs: # 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`. - read -r -a pats <<<"${GENERATED_PATHS}" + # .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}") @@ -413,7 +418,9 @@ jobs: 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\n' "$T" >>"$P" cat "$STATE/generated.md" >>"$P"