Merge pull request #22 from hotdata-dev/feat/frontload-review-context

feat(review): frontload PR context into the prompt

Author: zfarrell

.github/workflows/claude-pr-review.yml

@@ -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

.github/workflows/tests.yml

@@ -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
 

README.md

@@ -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)