Skip to content

Commit 840fb79

Browse files
zfarrellzfarrell
committed
test(review): add reviewer evals and the 21k expression check
1 parent 8b04393 commit 840fb79

50 files changed

Lines changed: 2413 additions & 0 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.github/scripts/eval.py‎

Lines changed: 487 additions & 0 deletions
Large diffs are not rendered by default.

‎.github/workflows/claude-review-eval.yml‎

Lines changed: 235 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,235 @@
1+
name: Reviewer Eval
2+
3+
# Grades the reviewer against the scenarios in tests/eval/ by putting real pull requests in front
4+
# of it, in hotdata-dev/pr-review-eval, and reading what it did to them.
5+
#
6+
# The reviewer is not invoked directly here. Each scenario's head branch gets a *copy* of the
7+
# reviewer workflow committed into it, and GitHub runs that copy on the scenario's own pull request
8+
# -- so what gets graded is the real file on the real `pull_request` path, with
9+
# github.event.pull_request.* resolving naturally, rather than a harness imitating it. Two things
10+
# follow from that, and both are the point:
11+
#
12+
# The candidate is the version under review. The copy comes from this checkout, so a pull request
13+
# that changes the reviewer workflow or the prompt document is evaluated as changed. The org
14+
# ruleset resolves the production reviewer from main, so nothing else gives a prompt edit any
15+
# pre-merge exposure -- PR #27's dry run loads the candidate workflow but skips the model step.
16+
#
17+
# The reviewer is an input, not an assumption. `reviewer_workflow` and `reviewer_login` are all
18+
# that tie this to Claude. Drop a different reviewer workflow in the repository, point these two
19+
# at it, and every scenario and assertion applies unchanged, because tests/eval-grade.py grades
20+
# pull request state rather than any harness's transcript.
21+
#
22+
# Non-blocking on purpose. It reports pass rates and comments them; it does not fail the pull
23+
# request. The thing under test is not deterministic, so scenarios pass on a threshold out of
24+
# repeats, and a check that goes red on sampling noise gets ignored within a week. Promote
25+
# individual scenarios to blocking once their observed rate justifies it.
26+
#
27+
# Why pull requests never target the sandbox's default branch: the org ruleset that requires the
28+
# reviewer workflow is scoped to ~DEFAULT_BRANCH, so a pull request against main there would be
29+
# reviewed twice -- once by main's required copy and once by the injected candidate -- and the
30+
# grader could not tell which verdict belonged to the version under test. Throwaway base branches
31+
# put the scenario outside the ruleset rather than carving an exception into org-wide config.
32+
33+
on:
34+
workflow_dispatch:
35+
inputs:
36+
scenarios:
37+
description: Comma-separated scenario names, or "all"
38+
default: all
39+
repeats:
40+
description: Override each scenario's repeats (blank to use meta.json)
41+
default: ""
42+
reviewer_workflow:
43+
description: Path to the reviewer workflow to inject
44+
default: .github/workflows/claude-pr-review.yml
45+
reviewer_login:
46+
description: Login the reviewer posts as
47+
default: claude[bot]
48+
schedule:
49+
# Nightly, after hours. Enough repeats to see drift; see the repeats resolution in `plan`.
50+
- cron: "0 9 * * *"
51+
pull_request:
52+
paths:
53+
- docs/claude-pr-review-prompt.md
54+
- .github/workflows/claude-pr-review.yml
55+
- .github/workflows/claude-review-eval.yml
56+
- tests/eval/**
57+
- tests/eval-grade.py
58+
59+
concurrency:
60+
# Never two eval runs at once: they share one sandbox repository, and a cancelled run's cleanup
61+
# step is the only thing that deletes its branches.
62+
group: reviewer-eval
63+
cancel-in-progress: false
64+
65+
permissions:
66+
contents: read
67+
68+
env:
69+
SANDBOX: hotdata-dev/pr-review-eval
70+
71+
jobs:
72+
plan:
73+
runs-on: ubuntu-latest
74+
outputs:
75+
matrix: ${{ steps.plan.outputs.matrix }}
76+
count: ${{ steps.plan.outputs.count }}
77+
steps:
78+
- uses: actions/checkout@v6.0.2
79+
with:
80+
fetch-depth: 1
81+
82+
# Scenario validity is asserted by tests/eval-test.sh in the Tests workflow, not here. This
83+
# step only expands scenarios into one matrix entry per repeat.
84+
- name: Build the matrix
85+
id: plan
86+
run: |
87+
python3 .github/scripts/eval.py plan >> "$GITHUB_OUTPUT"
88+
env:
89+
# On a pull request, one repeat per scenario: the point there is to see whether the
90+
# change under review moved a verdict, and 8 reviews is already a few dollars. The
91+
# nightly run uses each scenario's own repeats, which is where thresholds mean anything.
92+
EVAL_SCENARIOS: ${{ inputs.scenarios || 'all' }}
93+
EVAL_REPEATS: ${{ inputs.repeats || (github.event_name == 'pull_request' && '1' || '') }}
94+
95+
review:
96+
needs: plan
97+
if: needs.plan.outputs.count != '0'
98+
runs-on: ubuntu-latest
99+
timeout-minutes: 30
100+
strategy:
101+
fail-fast: false
102+
# One scenario at a time would take an hour; all at once floods the sandbox and the API. The
103+
# reviewer workflow itself has a 15-minute timeout, so the slow part is waiting, not compute.
104+
max-parallel: 4
105+
matrix:
106+
include: ${{ fromJson(needs.plan.outputs.matrix) }}
107+
permissions:
108+
contents: read
109+
steps:
110+
- uses: actions/checkout@v6.0.2
111+
with:
112+
fetch-depth: 1
113+
114+
# Two tokens, deliberately. GITHUB_TOKEN cannot be used to open the scenario pull request:
115+
# events created with it do not trigger workflows, so the injected reviewer would never run
116+
# and every scenario would grade as "none". A GitHub App installation token does trigger
117+
# them. It is also the only token here with write access to another repository.
118+
- name: Generate GitHub App token
119+
id: app-token
120+
uses: actions/create-github-app-token@v3.2.0
121+
with:
122+
client-id: Iv23liKBX2RYMoZIYuKa
123+
private-key: ${{ secrets.HOTDATA_AUTOMATION_PRIVATE_KEY }}
124+
owner: hotdata-dev
125+
repositories: pr-review-eval
126+
127+
- name: Stage the scenario branches
128+
id: stage
129+
run: python3 .github/scripts/eval.py stage
130+
env:
131+
GH_TOKEN: ${{ steps.app-token.outputs.token }}
132+
SCENARIO: ${{ matrix.scenario }}
133+
REPEAT: ${{ matrix.repeat }}
134+
RUN_ID: ${{ github.run_id }}
135+
RUN_ATTEMPT: ${{ github.run_attempt }}
136+
CANDIDATE_SHA: ${{ github.event.pull_request.head.sha || github.sha }}
137+
REVIEWER_WORKFLOW: ${{ inputs.reviewer_workflow || '.github/workflows/claude-pr-review.yml' }}
138+
139+
- name: Open the scenario pull request and wait for the review
140+
id: review
141+
run: python3 .github/scripts/eval.py run
142+
env:
143+
GH_TOKEN: ${{ steps.app-token.outputs.token }}
144+
SCENARIO: ${{ matrix.scenario }}
145+
BASE_BRANCH: ${{ steps.stage.outputs.base_branch }}
146+
HEAD_BRANCH: ${{ steps.stage.outputs.head_branch }}
147+
HEAD_SHA: ${{ steps.stage.outputs.head_sha }}
148+
REVIEWER_WORKFLOW: ${{ inputs.reviewer_workflow || '.github/workflows/claude-pr-review.yml' }}
149+
REVIEWER_LOGIN: ${{ inputs.reviewer_login || 'claude[bot]' }}
150+
WAIT_FOR_PUSH_CHECKS: ${{ steps.stage.outputs.wait_for_push_checks }}
151+
152+
# Grading is separate from running so a grader change can be re-tested against a finished
153+
# run's payloads, and so this step is the same code tests/eval-test.sh pins offline.
154+
- name: Grade
155+
id: grade
156+
if: always() && steps.review.outcome != 'skipped'
157+
continue-on-error: true
158+
run: |
159+
python3 tests/eval-grade.py \
160+
--meta "tests/eval/${SCENARIO}/meta.json" \
161+
--reviews "${RUNNER_TEMP}/reviews.json" \
162+
--comments "${RUNNER_TEMP}/comments.json" \
163+
--convo "${RUNNER_TEMP}/convo.json" \
164+
--reviewer "$REVIEWER_LOGIN" \
165+
> "${RUNNER_TEMP}/result-${SCENARIO}-${REPEAT}.json"
166+
env:
167+
SCENARIO: ${{ matrix.scenario }}
168+
REPEAT: ${{ matrix.repeat }}
169+
REVIEWER_LOGIN: ${{ inputs.reviewer_login || 'claude[bot]' }}
170+
171+
- name: Show the result
172+
if: always() && steps.grade.outcome != 'skipped'
173+
run: |
174+
python3 .github/scripts/eval.py summarise \
175+
"${RUNNER_TEMP}/result-${SCENARIO}-${REPEAT}.json" >> "$GITHUB_STEP_SUMMARY"
176+
env:
177+
SCENARIO: ${{ matrix.scenario }}
178+
REPEAT: ${{ matrix.repeat }}
179+
180+
- name: Upload the result
181+
if: always()
182+
continue-on-error: true
183+
uses: actions/upload-artifact@v7.0.1
184+
with:
185+
name: eval-result-${{ matrix.scenario }}-${{ matrix.repeat }}
186+
path: ${{ runner.temp }}/result-*.json
187+
if-no-files-found: ignore
188+
retention-days: 14
189+
overwrite: true
190+
191+
# Always, including on cancellation: a leaked branch pair is the only state this workflow
192+
# can leave behind, and the sandbox is shared by every future run.
193+
- name: Clean up
194+
if: always()
195+
continue-on-error: true
196+
run: python3 .github/scripts/eval.py cleanup
197+
env:
198+
GH_TOKEN: ${{ steps.app-token.outputs.token }}
199+
BASE_BRANCH: ${{ steps.stage.outputs.base_branch }}
200+
HEAD_BRANCH: ${{ steps.stage.outputs.head_branch }}
201+
202+
report:
203+
needs: [plan, review]
204+
if: always() && needs.plan.outputs.count != '0'
205+
runs-on: ubuntu-latest
206+
permissions:
207+
contents: read
208+
pull-requests: write
209+
steps:
210+
- uses: actions/checkout@v6.0.2
211+
with:
212+
fetch-depth: 1
213+
214+
- uses: actions/download-artifact@v6.0.0
215+
continue-on-error: true
216+
with:
217+
path: results
218+
pattern: eval-result-*
219+
merge-multiple: true
220+
221+
# Thresholds are applied here rather than per job, because a threshold is a statement about a
222+
# scenario's repeats and no single job can see them all.
223+
- name: Aggregate
224+
id: aggregate
225+
run: |
226+
python3 .github/scripts/eval.py report results > "${RUNNER_TEMP}/report.md"
227+
cat "${RUNNER_TEMP}/report.md" >> "$GITHUB_STEP_SUMMARY"
228+
229+
- name: Comment on the pull request
230+
if: github.event_name == 'pull_request'
231+
continue-on-error: true
232+
run: gh pr comment "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --body-file "${RUNNER_TEMP}/report.md"
233+
env:
234+
GH_TOKEN: ${{ github.token }}
235+
PR_NUMBER: ${{ github.event.pull_request.number }}

‎.github/workflows/tests.yml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,3 +54,9 @@ jobs:
5454

5555
- name: Context step end to end
5656
run: tests/context-step-test.sh
57+
58+
# The eval's own tests, not the eval. Scenario definitions and tests/eval-grade.py are
59+
# checked here on every pull request at no cost; the eval itself spends real API calls and
60+
# runs from .github/workflows/claude-review-eval.yml.
61+
- name: Reviewer eval scenarios and grader
62+
run: tests/eval-test.sh

‎README.md‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,68 @@ from the transcript, so no path, search pattern, or credential can ride along in
6363
`tests/tool-usage-test.sh` asserts that containment directly. Both the projection and the upload are
6464
non-fatal.
6565

66+
### Reviewer evals
67+
68+
`tests/` asserts that the context step assembles the right prompt. It says nothing about whether the
69+
reviewer then behaves, and until now nothing did — a prompt change could not be evaluated before
70+
merge at all, because the workflow resolves `docs/claude-pr-review-prompt.md` from `main`, so a pull
71+
request editing the prompt is reviewed by the prompt it replaces.
72+
73+
`.github/workflows/claude-review-eval.yml` closes that. Each scenario in
74+
[`tests/eval/`](tests/eval/) becomes a real pull request in `hotdata-dev/pr-review-eval`, with a
75+
*copy of the reviewer workflow committed into the head branch* — so the thing being graded is the
76+
real file on the real `pull_request` path, and the copy comes from the pull request under review
77+
rather than from `main`. Eight scenarios: a clean refactor that must be approved silently, an
78+
incremental loader whose strict `>` watermark drops rows that tie, a telemetry helper that posts
79+
`os.environ` to an external host, an injection attempt in the description paired with a dropped
80+
authorization check, cosmetic-only findings, review cycle 5 against a nit the author declined, a
81+
degraded CI block, and a genuinely red check with a real job log.
82+
83+
Grading reads **pull request state through the API** — the submitted review verdict, the inline
84+
comments, the summary comment — never the action's execution log. That distinction is what makes the
85+
reviewer swappable: the log is a Claude Code artifact, so grading it would make every assertion a
86+
statement about one harness. `reviewer_workflow` and `reviewer_login` are the only two things tying
87+
the eval to Claude, so a different reviewer is a different input, not a rewrite.
88+
89+
Three scenarios need something a file tree cannot express, and each is fenced. A real failing check
90+
comes from a workflow injected into the head branch, because a synthetic check run carries no job id
91+
for the log fetch to find. Prior review comments are posted while the pull request is still a draft
92+
and it is marked ready afterwards, because the reviewer starts the moment it becomes reviewable and
93+
anything posted later is invisible to it. A degraded context block comes from a literal substitution
94+
against the injected workflow — and `tests/eval-test.sh` asserts every anchor still matches the
95+
workflow exactly once, because a fault that silently stops applying would have the eval review an
96+
*unfaulted* pull request and report that the reviewer handled a degradation it never created.
97+
98+
The eval is reporting-only and passes on a rate, not a run: scenarios declare `repeats` and a
99+
`threshold`, security and injection demanding every repeat. The reviewer is not deterministic, and a
100+
merge-gating check that goes red on sampling noise gets ignored within a week. The judge rubric is
101+
recorded and never gates anything, for the same reason doubled.
102+
103+
What runs on every pull request is `tests/eval-test.sh`, not the eval: scenario schemas, trees that
104+
actually differ, regexes that compile, fault anchors, and the grader itself pinned against committed
105+
API payloads in both directions — a scenario passing when it should and failing when the reviewer
106+
approves a bug, posts an unmarked nit, leaves nits at cycle 5, claims CI is green when the block said
107+
it could not be read, or never reviews at all. None of that costs an API call.
108+
109+
### Startup-fatal workflow limits
110+
111+
`tests/workflow-lint-test.sh` checks two things that make a workflow unstartable rather than merely
112+
wrong, both of which have taken the org down. Neither is visible to `yaml.safe_load`, to the shell,
113+
or to actionlint, and the suite passed on both broken commits.
114+
115+
The first is an empty `${{ }}` expression, which Actions rejects outright — it arrived in a shell
116+
comment that spelled the delimiter out to explain why the code avoided it.
117+
118+
The second is the 21,000-character cap on a single expression. A block scalar containing an
119+
interpolation is compiled into one `format(...)` expression whose length is the *dedented* scalar,
120+
so a long, heavily commented `run:` block that interpolates anything is a workflow GitHub refuses to
121+
load: the run concludes `failure` in 0s with **zero jobs**, no check reports, and every pull request
122+
in the org blocks. The two commits either side of it measure 24,860 characters (broken) and 20,545
123+
(the revert) — 455 to spare, or about six comment lines. So the check warns from 90% of the cap, and
124+
warns separately about a long block that has *no* expression yet, since adding one would make the
125+
same text fatal. The way out is to move the interpolations into `env:`, which removes the cap from
126+
that block entirely.
127+
66128
## Setup
67129

68130
Requires:

0 commit comments

Comments
 (0)