Skip to content

Commit 5c6512e

Browse files
authored
fix(test-classifier): resolve repo from origin (not gh's parent) + harden comment-id capture (#64)
On a FORK, `gh repo view` resolves to the PARENT repo (a navapbc/ai-chatbot checkout reports vercel/chatbot), so the dispatcher looked up the wrong PR, ran the suite against the wrong diff, posted to the wrong repo (review-comment 422 'path could not be resolved' → issue-comment fallback on the upstream), and recorded a Testing Events row attributed to the wrong slug. Fix: resolve the owner/name slug from the `origin` remote URL (HTTPS or SSH), cached, with an AI_REVIEW_REPO=owner/name override. Thread it through all four gh call sites (--pr lookup, auto-discovery, comment posting, metrics row) via `-R <slug>` so they all agree on the fork. Also harden the comment-id capture: on a 422, `gh ... --jq` can print the raw error JSON to stdout, which slipped past the old non-empty guard and got stored as comment_id — poisoning the metrics row (the 'Validation Failed' text seen in the Sheet). Add ai_review::valid_comment_capture requiring a NUMERIC id; reject otherwise. Both this and the repo fix independently prevent the corruption. Docs: add a prominent WARNING that AI_RUN_SUITE=1 (OBSERVED) runs the change's code on your machine with no sandbox — only use it on branches you trust, never an untrusted --pr; plus a troubleshooting row for the fork/AI_REVIEW_REPO case.
1 parent 60df1ac commit 5c6512e

2 files changed

Lines changed: 76 additions & 11 deletions

File tree

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

Lines changed: 60 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,42 @@ require_gh_cli() {
216216
fi
217217
}
218218

219+
# Resolve the owner/name slug of the repo the PR lives in, from the `origin`
220+
# remote — NOT `gh repo view`. On a fork, `gh repo view` resolves to the PARENT
221+
# repo (e.g. a navapbc/ai-chatbot checkout reports vercel/chatbot), so posting
222+
# and the metrics row would target the wrong repo. `origin` is the fork you are
223+
# actually working in and whose PR number you passed. Override with AI_REVIEW_REPO
224+
# (owner/name) for the rare cross-remote case. Cached after first resolution.
225+
AI_REVIEW_REPO_SLUG=""
226+
ai_review::repo_slug() {
227+
if [[ -n "${AI_REVIEW_REPO_SLUG}" ]]; then
228+
printf '%s' "${AI_REVIEW_REPO_SLUG}"; return 0
229+
fi
230+
if [[ -n "${AI_REVIEW_REPO:-}" ]]; then
231+
AI_REVIEW_REPO_SLUG="${AI_REVIEW_REPO}"
232+
printf '%s' "${AI_REVIEW_REPO_SLUG}"; return 0
233+
fi
234+
local url
235+
url="$(git remote get-url origin 2>/dev/null || true)"
236+
if [[ -z "${url}" ]]; then
237+
echo "ERROR: no 'origin' remote — cannot determine which repo the PR lives in." >&2
238+
echo " Set one (git remote add origin <url>) or export AI_REVIEW_REPO=owner/name." >&2
239+
return 1
240+
fi
241+
# Normalize both forms to owner/name:
242+
# https://github.com/owner/name(.git) git@github.com:owner/name(.git)
243+
local slug="${url}"
244+
slug="${slug#*github.com[:/]}" # strip scheme/host up to owner
245+
slug="${slug%.git}" # strip trailing .git
246+
if [[ ! "${slug}" =~ ^[^/]+/[^/]+$ ]]; then
247+
echo "ERROR: could not parse owner/name from origin URL: ${url}" >&2
248+
echo " Export AI_REVIEW_REPO=owner/name to set it explicitly." >&2
249+
return 1
250+
fi
251+
AI_REVIEW_REPO_SLUG="${slug}"
252+
printf '%s' "${AI_REVIEW_REPO_SLUG}"
253+
}
254+
219255
ai_review::discover_pr_context() {
220256
# If --against was passed, it wins for the diff range — no base-ref lookup
221257
# needed. But we still record --pr (when given) as the PR number so comment
@@ -241,8 +277,10 @@ ai_review::discover_pr_context() {
241277
# If --pr was given, look up that PR's base ref.
242278
if [[ -n "${PR_NUMBER}" ]]; then
243279
require_gh_cli "PR number was specified via --pr"
280+
local repo_slug
281+
repo_slug="$(ai_review::repo_slug)" || exit 1
244282
local base
245-
base="$(gh pr view "${PR_NUMBER}" --json baseRefName --jq '.baseRefName' 2>/dev/null || true)"
283+
base="$(gh pr view "${PR_NUMBER}" -R "${repo_slug}" --json baseRefName --jq '.baseRefName' 2>/dev/null || true)"
246284
if [[ -z "${base}" ]]; then
247285
echo "ERROR: could not look up PR #${PR_NUMBER} via gh CLI." >&2
248286
echo " Verify the PR number exists and you have access to it." >&2
@@ -266,9 +304,12 @@ ai_review::discover_pr_context() {
266304
fi
267305

268306
# Let gh extract the fields with --jq (same idiom as the --pr branch above),
269-
# rather than scraping the raw JSON with brittle regexes.
270-
AI_REVIEW_PR_NUMBER="$(gh pr view --json number --jq '.number' 2>/dev/null || true)"
271-
AI_REVIEW_PR_BASE="$(gh pr view --json baseRefName --jq '.baseRefName' 2>/dev/null || true)"
307+
# rather than scraping the raw JSON with brittle regexes. Pin to origin's slug
308+
# so a fork checkout discovers its OWN PR, not the parent repo's.
309+
local repo_slug
310+
repo_slug="$(ai_review::repo_slug)" || exit 1
311+
AI_REVIEW_PR_NUMBER="$(gh pr view -R "${repo_slug}" --json number --jq '.number' 2>/dev/null || true)"
312+
AI_REVIEW_PR_BASE="$(gh pr view -R "${repo_slug}" --json baseRefName --jq '.baseRefName' 2>/dev/null || true)"
272313

273314
if [[ -z "${AI_REVIEW_PR_NUMBER}" ]] || [[ -z "${AI_REVIEW_PR_BASE}" ]]; then
274315
echo "ERROR: 'gh pr view' could not find an open PR for the current branch." >&2
@@ -585,7 +626,7 @@ post_issue_comment_to_github() {
585626
"repos/${repo_slug}/issues/${pr_number}/comments" \
586627
--method POST \
587628
--field body="${body}" --jq '"\(.id)\t\(.created_at)"' 2>/dev/null || true)"
588-
if [[ -z "$resp" || "$resp" == $'\t' ]]; then
629+
if ! ai_review::valid_comment_capture "$resp"; then
589630
echo "ERROR: 'gh api' issue-comment call failed." >&2
590631
echo " Check your gh auth status and that your token has 'pull-requests: write'" >&2
591632
echo " (or 'issues: write')." >&2
@@ -595,6 +636,17 @@ post_issue_comment_to_github() {
595636
POSTED_COMMENT_CREATED="${resp#*$'\t'}"
596637
}
597638

639+
# Validate an "<id>\t<created_at>" capture from a comment-POST response. A real
640+
# success has a NUMERIC id. On an API error (e.g. a 422), `gh ... --jq` may print
641+
# the raw error JSON to stdout, which would otherwise be stored verbatim as the
642+
# comment_id and poison the metrics row — so require a numeric leading field.
643+
ai_review::valid_comment_capture() {
644+
local resp="$1"
645+
[[ -n "$resp" && "$resp" != $'\t' ]] || return 1
646+
local id="${resp%%$'\t'*}"
647+
[[ "$id" =~ ^[0-9]+$ ]]
648+
}
649+
598650
# Post ONE classification comment to the PR. Args: PR number, comment body.
599651
#
600652
# Posts as a file-level pull-request REVIEW comment (subject_type=file) so the
@@ -611,10 +663,7 @@ post_comment_to_github() {
611663
require_gh_cli "--post-comment was specified"
612664

613665
local repo_slug
614-
if ! repo_slug="$(gh repo view --json nameWithOwner --jq '.nameWithOwner' 2>/dev/null)"; then
615-
echo "ERROR: could not determine repo from gh CLI." >&2
616-
exit 1
617-
fi
666+
repo_slug="$(ai_review::repo_slug)" || exit 1
618667

619668
# Anchor path: a file in the PR diff. Prefer a changed test file if one is
620669
# present, since the comment is about test failures, but any changed file
@@ -651,7 +700,7 @@ post_comment_to_github() {
651700
--field commit_id="${commit_id}" \
652701
--field path="${anchor_path}" \
653702
--field subject_type=file --jq '"\(.id)\t\(.created_at)"' 2>/dev/null || true)"
654-
if [[ -n "$resp" && "$resp" != $'\t' ]]; then
703+
if ai_review::valid_comment_capture "$resp"; then
655704
POSTED_COMMENT_ID="${resp%%$'\t'*}"
656705
POSTED_COMMENT_CREATED="${resp#*$'\t'}"
657706
echo "[test-classifier] Review comment posted. Awaiting the developer's 👍/👎 reaction (and a reply reason on a 👎)."
@@ -733,7 +782,7 @@ submit_metrics_row() {
733782
fi
734783

735784
local repo_slug
736-
repo_slug="$(gh repo view --json nameWithOwner --jq '.nameWithOwner' 2>/dev/null || echo "")"
785+
repo_slug="$(ai_review::repo_slug 2>/dev/null || echo "")"
737786

738787
# The classifier may emit multiple classifications; the Testing Events row is
739788
# per-comment, so collapse to one representative verdict by most-actionable

testing/classifier/docs/LOCAL_TEST_CLASSIFIER.md

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,21 @@ and uses the same `--unpushed` scope rule. This is the ergonomics layer over
3232
OBSERVED with `AI_RUN_SUITE=1` to have the agent actually locate, install, and
3333
run your suite and triage the real failures — the same behavior CI uses.
3434

35+
> [!WARNING]
36+
> **OBSERVED (`AI_RUN_SUITE=1`) runs the change's code on your machine — there
37+
> is no sandbox.** The agent installs dependencies (running each package's
38+
> install/postinstall scripts) and executes the test suite **directly in your
39+
> checkout**, with your shell, your environment, and your credentials. It also
40+
> leaves build artifacts behind (`node_modules/`, downloaded browsers, etc.).
41+
>
42+
> Only use `AI_RUN_SUITE=1` on a branch **you trust** — typically your own
43+
> work-in-progress. Do **not** point it at an untrusted or unreviewed PR
44+
> (e.g. `--pr N` for someone else's contribution): installing and running that
45+
> branch's code is arbitrary code execution on your laptop. For untrusted
46+
> changes, use the default **INFERRED** mode (omit `AI_RUN_SUITE`), which only
47+
> reasons over the diff and never executes anything, or let CI run OBSERVED in
48+
> its disposable runner.
49+
3550
---
3651

3752
## Prerequisites
@@ -214,6 +229,7 @@ run (Path B) is still the recorded, metrics-feeding pass; this is your preview.
214229
| `AI_REVIEW_TOOL … not set` | Export `AI_REVIEW_TOOL=claude` (or `codex`/`copilot`); see `README.md`. |
215230
| Classification seems to miss recent edits | `--unpushed` excludes *unstaged* changes. `git add` or commit them first. |
216231
| Everything lands in INFERRED | That's the default. Add `AI_RUN_SUITE=1` to run the suite (OBSERVED). Suites needing services may still fall back to INFERRED, by design. |
232+
| Posted to / recorded the wrong repo (e.g. the upstream of your fork) | The repo is resolved from your `origin` remote. On a fork whose `origin` isn't the repo the PR lives in, export `AI_REVIEW_REPO=owner/name` to set it explicitly. |
217233

218234
---
219235

0 commit comments

Comments
 (0)