Skip to content

Commit f7d4fb8

Browse files
authored
feat(test-classifier): let the agent re-resolve the diff range (fix 'nothing to classify') (#75)
A user on a GitHub Enterprise host (github.cms.gov) got the PR lookup working via AI_REVIEW_REPO, but then hit 'No change under test (git diff origin/main HEAD) — nothing to classify'. Root cause: the dispatcher hardcodes the diff base as origin/<base> and the prompt told the agent to 'use it verbatim'. When the PR lives on a repo that is NOT the local origin (fork, enterprise host, or a stale/ absent origin/<base>), that range is wrong/empty — and 'verbatim' made the agent trust the emptiness instead of recovering. Reframe: the precomputed range is a STARTING POINT, and the agent (which has git + shell) re-resolves the real diff itself when it looks empty/wrong — exactly the deterministic recovery it was already improvising in practice. • dispatcher exports AI_REVIEW_PR_NUMBER / AI_REVIEW_PR_BASE / AI_REVIEW_REPO_SLUG so the prompt can reference them; AI_REVIEW_DIFF_RANGE stays as the start. • prompt: drop 'use it verbatim' → 'START there, verify, do NOT treat as gospel'; on empty/wrong range, fetch the base from the PR's OWN repo (gh repo view -R <slug> --json url → git fetch <url> <base>) and diff FETCH_HEAD HEAD. Works across remotes/hosts. Note the re-resolved base in the summary. • SKILL.md Step 2 updated to match. Does NOT change the happy path (origin/<base> still the default when correct). Still need a user on the fork/enterprise layout to confirm end-to-end; the change only ADDS a recovery path, so it can't regress runs that already worked.
1 parent 114c77c commit f7d4fb8

2 files changed

Lines changed: 49 additions & 9 deletions

File tree

testing/classifier/.skills/test-classifier/SKILL.md

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -196,15 +196,24 @@ marker, and stop. Passing tests are never listed.
196196

197197
## Step 2 — Collect the Diff
198198

199-
The dispatcher passes the change-under-test base ref via the
200-
`AI_REVIEW_AGAINST` environment variable. If unset, the dispatcher will have
201-
refused to run.
199+
The dispatcher passes a **candidate** range via `AI_REVIEW_DIFF_RANGE` (and the
200+
base ref via `AI_REVIEW_AGAINST`). Start there:
202201

203202
```bash
204-
git diff "$AI_REVIEW_AGAINST" HEAD --unified=5 # full content of the change
205-
git diff "$AI_REVIEW_AGAINST" HEAD --name-only # list of changed paths
203+
git diff $AI_REVIEW_DIFF_RANGE --name-only # list of changed paths
204+
git diff $AI_REVIEW_DIFF_RANGE --unified=5 # full content of the change
206205
```
207206

207+
**Verify it reflects the real change under test — it is a starting point, not
208+
gospel.** The precomputed base (often `origin/<base>`) can be wrong or empty when
209+
the PR lives on a different repo than the local `origin`, on a fork or enterprise
210+
host, or when `origin/<base>` is stale/absent. If the name-only diff is empty or
211+
clearly not this PR's change, re-resolve the base yourself before concluding
212+
there is nothing to classify: for a PR run use `AI_REVIEW_PR_NUMBER` /
213+
`AI_REVIEW_REPO_SLUG` / `AI_REVIEW_PR_BASE` to fetch the base from the PR's own
214+
repo and diff `FETCH_HEAD HEAD`; for a local `--unpushed` run fall back to the
215+
merge-base with the remote default branch. Note in the summary if you re-resolved.
216+
208217
The diff is your evidence for the central question: did this change *intend* to
209218
alter the behavior the failing test asserts on? An intended behavior change that
210219
the test wasn't updated for points toward `TEST_BUG`. A change that should NOT

testing/classifier/.skills/test-classifier/scripts/test-classifier-dispatcher.sh

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -401,12 +401,35 @@ in this repository at:
401401
402402
Follow the file at the path above.
403403
404-
Classify the failing tests for the change under test. The exact git range is in
405-
the AI_REVIEW_DIFF_RANGE env var (base→HEAD normally; base→index, i.e. committed
406-
+ staged, for a local --unpushed run). Use it verbatim:
404+
Classify the failing tests for the change under test. The dispatcher precomputed
405+
a candidate git range in the AI_REVIEW_DIFF_RANGE env var (base→HEAD normally;
406+
base→index for a local --unpushed run). START there:
407407
408-
git diff \$AI_REVIEW_DIFF_RANGE --unified=5
409408
git diff \$AI_REVIEW_DIFF_RANGE --name-only
409+
git diff \$AI_REVIEW_DIFF_RANGE --unified=5
410+
411+
IMPORTANT — verify the range actually reflects the change under test; do NOT
412+
treat AI_REVIEW_DIFF_RANGE as gospel. The precomputed base (often \`origin/<base>\`)
413+
can be WRONG or empty when the PR lives on a different repo than the local
414+
\`origin\`, on a fork, or on an enterprise host (e.g. github.cms.gov), or when
415+
\`origin/<base>\` is stale or absent. If \`git diff \$AI_REVIEW_DIFF_RANGE --name-only\`
416+
is EMPTY or obviously not this PR's change, RE-RESOLVE the base yourself before
417+
concluding there is nothing to classify:
418+
419+
• This is a PR run when AI_REVIEW_PR_NUMBER is set; its repo is in
420+
AI_REVIEW_REPO_SLUG and its base branch in AI_REVIEW_PR_BASE.
421+
• Fetch the PR's base from its own repo and diff HEAD against what you fetched:
422+
gh pr view "\$AI_REVIEW_PR_NUMBER" -R "\$AI_REVIEW_REPO_SLUG" --json baseRefName,headRefOid
423+
# fetch the base ref from the PR's repo (works across remotes/hosts):
424+
url="\$(gh repo view -R "\$AI_REVIEW_REPO_SLUG" --json url --jq .url)"
425+
git fetch "\$url" "\$AI_REVIEW_PR_BASE"
426+
git diff FETCH_HEAD HEAD --name-only
427+
git diff FETCH_HEAD HEAD --unified=5
428+
• For a local --unpushed run (no PR number), fall back to the merge-base with
429+
the remote default branch, or the last pushed commit, as the base.
430+
431+
Use whichever range genuinely captures the change under test. State in the JSON
432+
"summary" which base you used if you had to re-resolve it.
410433
411434
Follow the skill instructions in that SKILL.md file (path above) exactly:
412435
@@ -947,10 +970,18 @@ test_classifier::run() {
947970
ai_review::log "────────────────────────────────────────────────────────────"
948971

949972
export AI_REVIEW_AGAINST
973+
# Export the PR context so the agent can re-resolve the diff itself when the
974+
# precomputed range below is empty/wrong (forks, enterprise hosts, stale
975+
# origin/<base>). These may be empty for a local --unpushed run.
976+
export AI_REVIEW_PR_NUMBER="${AI_REVIEW_PR_NUMBER:-}"
977+
export AI_REVIEW_PR_BASE="${AI_REVIEW_PR_BASE:-}"
978+
export AI_REVIEW_REPO_SLUG="${AI_REVIEW_REPO_SLUG:-}"
950979

951980
# The git range the AI should diff, matching the dispatcher's own accounting:
952981
# --unpushed (INCLUDE_STAGED) → `--cached <base>` (committed + staged)
953982
# otherwise → `<base> HEAD` (committed range)
983+
# This is a STARTING POINT, not gospel — the prompt authorizes the agent to
984+
# re-resolve locally if it doesn't reflect the real change under test.
954985
if [[ "${AI_REVIEW_INCLUDE_STAGED:-0}" == "1" ]]; then
955986
AI_REVIEW_DIFF_RANGE="--cached ${AI_REVIEW_AGAINST}"
956987
else

0 commit comments

Comments
 (0)