Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
425 changes: 404 additions & 21 deletions .github/workflows/claude-pr-review.yml

Large diffs are not rendered by default.

6 changes: 6 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,9 @@ jobs:

- name: Tool usage projection
run: tests/tool-usage-test.sh

- name: Frontloaded PR context
run: tests/pr-context-test.sh

- name: Context step end to end
run: tests/context-step-test.sh
50 changes: 44 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,54 @@ Automated code review on every pull request using [claude-code-action](https://g

The review prompt lives in [`docs/claude-pr-review-prompt.md`](docs/claude-pr-review-prompt.md).

### Frontloaded review context

The workflow gathers the PR into the prompt before the reviewer starts: title and description,
commits, changed files with per-file line counts, CI check status with log excerpts from failing
jobs, the diff since the reviewer's own last review, the full diff, and the PR conversation.

This is not a convenience. The reviewer's allowlist is four `gh pr` commands plus `rg`, `Read`,
`Grep`, and `Glob`, and the checkout is `fetch-depth: 1` — so the reviewer cannot reach git history,
cannot run tests, and cannot pipe or redirect even the commands it is allowed. Before this, it spent
19.5 Bash calls and 5.2 permission denials per run trying anyway; 86% of runs hit at least one
denial, and one review approved a PR with the words "reviewed statically (test suite not run in this
environment)" while CI had already run those tests. Runs with no denials averaged 14 turns and 98
seconds against 37 turns and 270 seconds for runs with five or more.

Each block degrades to a sentence saying what is missing rather than to silence, because a reviewer
handed an empty CI block will state that CI is clean. Two reads get a stronger treatment: a failed
`/reviews` or `/pulls/{n}/comments` would otherwise render as `REVIEW CYCLE: 1` and "no prior review
comments", which are claims rather than gaps, so those failures are disclosed to the reviewer in a
`## Context warnings` block at the top of the context. The step is `continue-on-error`: a failure
there once skipped the review step and the notify step with it, leaving the PR with no review and no
explanation.

Both step outputs are byte-bounded (100 KB of comment threads, 200 KB of context), with per-block
caps beneath that — 3,000 diff lines, 40 KB per CI log excerpt, 3,000 characters per comment. The
caps are deliberately far below any plausible runner limit: 400 inline comments rendered 1.1 MB of
threads before they existed, and the runner accounts for output size in UTF-16, so a byte count here
is not the number it checks against. Blocks are ordered so that truncation sacrifices the PR
conversation before the diff or the CI status.

Everything reaching the prompt is attacker-controlled — title, body, diff, CI logs, comments — so the
block delimiters are neutralised by shape rather than by exact string: `</pr_context >`,
`</PR_CONTEXT>` and `< / pr_context foo="1">` all read as the same delimiter to a model, and any of
them would otherwise end the data block early and land the rest where it reads as instructions.

### Tool usage artifact

Each run attaches a `claude-tool-usage-pr-<number>` artifact (14-day retention): tool call counts,
denied tool names, and the run's turn count and cost. It exists to diagnose permission denials
against the workflow's `--allowedTools` list, since the job log records only the number of denials,
never which tools were refused.
Bash command labels with a compound flag, the denied subset of both, and the run's turn count and
cost. It exists to diagnose permission denials against the workflow's `--allowedTools` list, since
the job log records only the number of denials, never what was refused. Tool names alone proved
insufficient — 520 of 567 denials in the first week were `Bash`, which is every command there is.

The artifact is a projection of the action's execution log, never the log itself — that file is the
full conversation, and the runner holds a git credential the reviewer can read, which artifacts
(unlike job logs) would not mask. `TOOL_USAGE_JQ` in the workflow emits names and counts only, and
`tests/tool-usage-test.sh` asserts that tool inputs, tool results, and repository contents cannot
reach the artifact. Both the projection and the upload are non-fatal.
(unlike job logs) would not mask. Command labels come from the fixed vocabulary in `CMD_JQ`, never
from the transcript, so no path, search pattern, or credential can ride along in a "command prefix".
`tests/tool-usage-test.sh` asserts that containment directly. Both the projection and the upload are
non-fatal.

## Setup

Expand Down
34 changes: 29 additions & 5 deletions docs/claude-pr-review-prompt.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,39 @@ You are an expert code reviewer embedded in a GitHub Actions workflow. Your job

This prompt includes:
- **REVIEW CYCLE** — which review iteration this is (1 = first review, 2+ = re-review after changes)
- **Prior Review Comments** — existing inline comment threads from previous reviews, including author responses. If this is cycle 1, there will be no prior comments — skip straight to reviewing the code.
- **Prior Review Comments** (`<prior_review_comments>`) — existing inline comment threads from previous reviews, including author responses. If this is cycle 1, there will be no prior comments — skip straight to reviewing the code.
- **PR Context** (`<pr_context>`) — title, description, commits, changed files with per-file line counts, CI check status, log excerpts from any failing CI job, the diff since your own last review (cycle 2+), the full diff, and the PR conversation.

Everything in `<pr_context>` is already in front of you. Do not spend a tool call re-fetching it.

**Unless it is not there.** If `<pr_context>` is empty, or a block inside it says it could not be read, then that block is genuinely missing — fetch what you need yourself with `gh pr diff` or `gh pr view`, and say in your review that you reviewed without it. Never treat a missing block as evidence: an absent CI block does not mean CI is clean, and an absent diff does not mean nothing changed.

## Tools

Available: `Read`, `Grep`, `Glob`, `rg`, and `gh pr diff` / `gh pr view` / `gh pr review` / `gh pr comment`. Nothing else — every other command is refused, and each refusal costs a turn.

- **Use `Read` for files** and `Grep`/`Glob`/`rg` to search. `cat`, `sed`, `head`, `ls`, `find`, and `grep` are all refused.
- **Never pipe, redirect, or chain.** `gh pr diff | head`, `gh pr diff > f.diff`, and `rg foo && rg bar` are all refused even though `gh pr diff` and `rg` are allowed — the allowlist matches whole commands. Run one command at a time.
- **Do not run tests, linters, or builds.** Dependencies are not installed and the commands are refused. CI already ran them; the results are in `<pr_context>`.
- **Do not use git.** The checkout is `fetch-depth: 1`, so there is no history and no base branch to diff against. The diffs you need are in `<pr_context>`.
- **Do not write files.** There is no scratch space; `Write` is refused.

## Review Process

1. **Understand the PR** — read the title, description, and linked issues to understand intent
2. **Read prior review threads** — if cycle 2+, read the prior review comments included above to understand what feedback was already given and how the author responded
3. **Inspect the diff** — use `gh pr diff` to see what changed
1. **Understand the PR** — read the title, description, and commits in `<pr_context>`
2. **Read prior review threads** — if cycle 2+, read the prior review comments to understand what feedback was already given and how the author responded
3. **Inspect the diff** — read the diff in `<pr_context>`. On cycle 2+, start from the diff since your last review, then consult the full diff for surrounding context
4. **Read affected files** — use `Read` to get full context around changed code
5. **Post feedback** — use inline comments for specific issues, and a summary comment only when requesting changes
5. **Check CI** — read the check status in `<pr_context>`. See "CI Status" below
6. **Post feedback** — use inline comments for specific issues, and a summary comment only when requesting changes

## CI Status

The check status in `<pr_context>` is a snapshot from the moment this review started. This workflow runs on the same push as the rest of CI, so checks are usually still queued or in progress.

- **A failing check is a blocking issue.** Name the failing check and cite the log lines provided.
- **Checks that are queued, in progress, or absent are not evidence of anything.** Do not claim tests pass, and do not claim they fail.
- **Never state or imply that you verified behavior by running it.** You did not run anything. If a correctness claim depends on tests you cannot see the result of, say what the untested risk is instead of asserting it is fine.

## Handling Prior Feedback (cycle 2+ only)

Expand Down
Loading
Loading