Repository navigation
[CI] ci: add @aiter-bot review (self-hosted GLM PR reviewer) - #5666
Merged
Merged
Conversation
Adds the orchestration layer behind the @aiter-bot review workflow: run_one (fetch -> headless GLM worker Step 1b-8 -> independent GLM refuter Step 7.7 -> seven gates -> collect) and publish (hold HIGH RISK for a human, post non-red, dedup by head SHA, skip merged/closed). It reuses the in-repo review-pr skill (wraps its fetch.sh and triage.py), does not reimplement it. Workflow updated to check out the repo and resolve the tooling relatively (no absolute paths). English-only; GLM backend is provisioned on the self-hosted runner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zufayu
marked this pull request as draft
September 20, 2026 03:31
Black + Ruff clean the .review-loop python (imports split, shebangs dropped, check=False on subprocess.run, with-open, f-strings). Register the box308 self-hosted-runner label in .github/actionlint.yaml so the workflow's runs-on passes actionlint. No behavior change; selftest 147/0.
Move the orchestration into .claude/skills/review-pr/ (where the skill, its fetch.sh and triage.py already live) instead of a new top-level .review-loop/, matching aiter's convention that skills own their scripts. Trim the thin shell wrappers: run_one/preflight call the python directly and fold the fetch wrapper (the applies.txt merge-target re-check) inline. Paths resolve via git rev-parse, so the scripts are location-independent. Workflow updated to the skill paths; publish runs _publish.py directly. No behavior change.
worker.md/refuter.md quote SKILL.md verbatim (Step 8 output rules; Step 7.7). check_prompts.py extracts each quoted block from SKILL.md by unique anchors and asserts it appears verbatim in the prompt — drift or a moved anchor exits non-zero. run_one.sh fails fast on drift before spending a review; preflight reports it. Verified: passes clean today, catches an injected one-word change.
- Scope AITER_BOT_TOKEN to the claim and publish steps only, off the job env, so the review agent (headless, tool-enabled, reads untrusted PR diffs) never sees the token — removes a prompt-injection exfiltration path. - publish gate gains success() so it cannot run after a failed review. - run_one cleans up the /tmp WORK dir and prunes its worktrees on success (kept on failure for debugging), so a long-lived runner does not fill up.
Non-red reviews auto-post as aiter-bot. A red (HIGH RISK) review is not auto- posted, but it is no longer silent: publish emits a GitHub Actions ::warning:: annotation (visible on the PR's checks) and prints the full card to the job log, so a maintainer sees it and decides, without auto-broadcasting a scary comment.
…W_AGENT) run_one hardcoded claude-glm. AITER_REVIEW_AGENT lets a box that hosts GLM itself use a local `claude` pointed at its on-box endpoint (no tunnel/wrapper), and AITER_REVIEW_REFUTER_AGENT can point the refuter at a different family.
fetch.sh Step 1 calls 'gh pr view --json ...,baseRefOid,headRefOid'. On an old gh (seen: 2.23.0) baseRefOid is an unknown JSON field, so the call errors and fetch aborts before any review runs — a silent runner failure that the preflight did not catch (it checked python/git/curl but never gh). Add two runtime checks: gh present, and gh recent enough to know baseRefOid, probed offline via 'gh pr view --help | grep -q baseRefOid' (no network, no token, no live PR). Fix on a red is: install a current gh (>= 2.24). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fetch.sh prints 'WORK=<dir>' where <dir> is mktemp under $TMPDIR. run_one
grepped 'WORK=/tmp/review-pr-...', so on a runner that redirects TMPDIR off a
full or read-only /tmp (e.g. a box whose root fs is full, pointing TMPDIR at a
data volume) the match failed after a *successful* fetch and run_one aborted
as 'no WORK dir' — a silent stop right at the fetch->worker handoff.
Match the WORK dir by its review-pr- basename regardless of parent path
('WORK=[^[:space:]]+/review-pr-...'), so the handoff works whatever TMPDIR is.
Caught running end to end on a self-hosted runner (root fs full, TMPDIR moved
to a data volume); the full pipeline then completed 7/7 gates green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fetch.sh created WORK with 'mktemp -d /tmp/review-pr-XXXXXX' and its day-old-
worktree GC matched only '/tmp/review-pr-...'. Both hardcode /tmp, so a runner
that must move scratch off /tmp — a self-hosted box whose root fs is full,
pointing TMPDIR at a data volume — cannot create WORK there, and any worktrees
it does create never get garbage-collected (they accumulate at ~182M each).
Honor $TMPDIR (default /tmp, so unchanged on a normal box): create WORK under
${TMPDIR:-/tmp}, and match worktrees by their review-pr- basename regardless
of parent path. Verified end to end on a self-hosted runner with TMPDIR moved
to a data volume: full pipeline completed 7/7 gates green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The workflow needs a self-hosted runner labeled box308, and preflight.sh checks the box, but nothing documented HOW to satisfy those checks: install the runner on a data volume (not $HOME/root — checkout + worktrees are large), inject box config via the runner .env, and keep the github token off the job env via GH_CONFIG_DIR so the headless review agent cannot read it. Capture that as a short, machine-agnostic contract that points back at preflight.sh for checkout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
re-review was redundant: the review step never used the gate's kind, and _publish.py already dedups by head SHA, so plain '@aiter-bot review' after a push (new head SHA) is a fresh pass on its own. Drop the second command and the kind plumbing — one command auto-covers first pass and a repeat pass after fixes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A single GLM request timeout (common on a shared inference box) killed the whole review: run_one ran the headless agent once and exited on non-zero. Wrap the worker and refuter in run_agent(), which retries up to AITER_REVIEW_RETRIES (default 3) with linear backoff and requires the output file to be non-empty before counting success. Transient GLM slowness no longer aborts a review; a genuinely down backend still fails cleanly after the attempts are exhausted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…LM owner) A review is worthless if the GLM backend is down, and a dead backend otherwise hung ~40s per agent call and died silently mid-review. run_one now deep-probes the on-box GLM (a real 1-token inference, not the shallow /health that stays green while inference is wedged) before any work and, if it fails, aborts in about a second with an ::error annotation and drops a .aiter-glm-down sentinel. The workflow gains a notify step that reads that sentinel and posts one comment: it tells whoever triggered the review that this was a backend outage (not their PR) and @-mentions the GLM deployment owner to go fix it. The owner is the repo variable AITER_GLM_OWNER, defaulting to honglie. The runner never tries to restart GLM itself — that belongs to the ATOM deployment, not the review bot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A failed review should land on whoever owns the failure, not on the PR author. run_one.sh now tags every abort with a class -- glm / atom / machine / flow / env -- via a fail() helper that writes .aiter-review-status. _notify.py is the single source of truth mapping class -> owner (repo-var overridable: glm=honglie, atom=the atom team, machine=huangxin, flow/env=the bot owner) and posts one comment @-mentioning them; the glm notice also carries a triage line escalating to the atom/machine owners, since a dead inference endpoint cannot always be told apart from outside. Hardened against drift: the class->owner map lives ONLY in _notify.py, the workflow step is a thin call to it, and check_prompts.py (run at every review start and in preflight) fails if run_one.sh emits a class _notify.py does not know. The runner only exposes backend problems; it never restarts GLM/ATOM. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The classified fail() paths route to an owner, but an unexpected non-zero exit that did NOT classify itself -- a crash in fetch/gates/collect, a set -e trip -- left no status file, so _notify.py saw nothing and the failure died silently with nobody notified. Add an EXIT trap: any non-zero exit without a status writes a flow-class status so it routes to the bot owner for triage. Now every way run_one can fail lands on a responsible person; success writes nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t, drop KILLED, bound & pre-gate Acting on the five verified findings the bot raised against this PR: - POST every verdict, including 🔴: the reviewer who triggered the review must see the report -- holding 🔴 in the job log meant the highest-risk cases showed nothing on the PR. It stays advisory; a warning annotation still flags 🔴. - Drop KILLED findings before the gates (_apply_refutation.py): the refuter killing a finding is its job (~28% base rate), but nothing removed it from the card, so triage.py's independent gate red-failed and lost the whole review. - Bound a hung review: timeout per agent call (AITER_AGENT_TIMEOUT, 20m) plus job timeout-minutes, so a GLM that wedges mid-review cannot pin the one runner until GitHub's 6h default. - Authorize before the expensive checkout: job-level if against the AITER_REVIEW_AUTHORIZED repo var, so an unlisted commenter no longer triggers a full-history clone on the self-hosted runner (public-repo spam vector). The description's over-promise (validation is opt-in/off by default; the refuter is same-family unless an Opus endpoint is set) and the prompt-injection containment note were fixed in the PR body. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fixes) The bot re-reviewed the PR after the first fixes and caught real bugs in them: - Token no longer leaks to the agent: run_one held the gh fallback token in a plain var handed only to fetch.sh's env, never exported, so the headless agent (--dangerously-skip-permissions on untrusted PR content) can't read it -- the containment the description claims is now true. - The owner-routing guarantee survives a timeout kill: /job-timeout kill with SIGTERM, which skips a plain EXIT trap, so a killed review left no status and nobody was notified. Trap TERM/INT too, fit the retry budget (2 attempts x 1000s x 2 agents = 66m) inside timeout-minutes 90, and have the notify step synthesize a flow status if the review step failed with none. - Authorization runs before the expensive checkout for real: the job now requires AITER_REVIEW_AUTHORIZED to be set and to list the commenter, so an unlisted user no longer starts a full-clone job on the runner (was only gated when the var happened to be set). - preflight probes the configured AITER_REVIEW_AGENT + ANTHROPIC_BASE_URL instead of hardcoding claude-glm, so a box provisioned per RUNNER-SETUP.md passes. Code and docs now agree that every verdict (incl. 🔴) is posted; description, workflow step name and header comment updated to match _publish.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Third pass caught lower-severity but real issues (verdict dropped 🔴 ->⚠️ ): - Worktree GC no longer deletes a user's worktree: fetch.sh's day-old cleanup matched any path containing /review-pr-, so 'git worktree remove --force' could discard a user worktree merely named review-pr-* with uncommitted work. Scope it to this box's scratch base (${TMPDIR:-/tmp}/review-pr-*) so only the review's own worktrees are touched. - _collect.py pointed at state.sh / pending.sh, which this PR never ships; replace both with the real git commands. - run_one.sh's retry comment said 'default 3' after the code moved to 2; align it. The description now states AITER_REVIEW_AUTHORIZED is required (unset = the bot ignores every comment, not a silent file-fallback) and that the runner account's git credential should be a dedicated read-only token, since the agent can read its files, not just its environment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Two-job workflow: a free ubuntu 'gate' job does the quote-aware trigger and allowlist check (reading authorized.txt from the default branch, or the repo var) BEFORE the self-hosted 'review' job checks anything out -- so an unlisted commenter never starts a job on the single box308, and the allowlist stays a maintainer-editable file (no admin-only repo var required). - authorized.txt now lists the real aiter review roster plus everyone who has reviewed a recent merged aiter PR (incl. external collaborators); bots excluded. - _notify.py owner defaults are the actual GitHub logins (glm=yhl-amd/Honglie, machine=gyohuangxin/Xin Huang, atom=valarLip), not display names that would @-mention nobody. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…) + pass repo to run_one The _on_exit EXIT trap short-circuited to a non-zero return on the success path (ec=0 makes [ "$ec" -ne 0 ] exit 1, && stops there), so run_one.sh exited 1 on every green review -> Publish (if: success()) never ran and the card was never posted, while Report-failed (if: always()) fired a false pipeline-failure notice. Add an explicit return 0. Also pass $GITHUB_REPOSITORY to run_one.sh so it reviews the repo the workflow runs in (was defaulting to ROCm/aiter), fixing fork/staging runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…aiter, so it checked the wrong repos PR and skipped fork/staging cards) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gyohuangxin
approved these changes
Sep 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why aiter-bot (and not just Copilot review)
SKILL.md/rules.md— aiter failure modes distilled from 200+ merged PRs (dispatch gate holes, gfx arch gating, tuned-config collisions, perf-claim scrutiny, AI-code tells)[inferred]Advisory and complementary — not a Copilot replacement.
What it is
Comment
@aiter-bot reviewon any PR and aiter-bot posts one review card. Push fixes and comment again for a fresh pass — the same commit is deduped, so it never spams. One command, nothing else to remember. Runs on a self-hosted runner, posts as the aiter-bot account, and is advisory — it never gates merge.How it works
flowchart TD A["aiter-team member comments:<br/><b>@aiter-bot review</b> on a PR"] --> B{"Gate: commenter on the<br/>allowlist? real trigger line?<br/>(quoted '>' lines ignored)"} B -- no --> Z["ignored (just a notice)"] B -- yes --> C["aiter-bot adds a 👀 reaction"] C --> D["self-hosted runner (box308)"] D --> E["fetch: PR diff, merge-target<br/>worktree, validation triage"] E --> F["GLM worker → review card<br/>(SKILL.md Steps 1b–8)"] F --> G["independent refuter (Step 7.7)<br/>— a second pass kills false positives"] G --> H["7 gates verify the card"] H --> V{"review verdict"} V -- "✓ no findings / ⚠ needs work" --> P["aiter-bot posts the card on the PR"] V -- "🔴 HIGH RISK" --> R["posted too (advisory) +<br/>a HIGH RISK warning<br/>annotation in the checks"] P -. "push fixes, comment<br/><b>@aiter-bot review</b> again<br/>(new head → fresh pass,<br/>same commit → deduped)" .-> AEach run posts one card: a one-line summary, a
Review/Validation/Perfverdict line, and ≤5 ranked findings (problem → impact → Author must… / Reviewer should ask…, each tagged[verified]/[inferred]). A🔴 HIGH RISKcard is posted too — it is exactly what the reviewer asked to see — and stays advisory (never a merge gate); a warning annotation also flags it in the checks.Example review
A real card aiter-bot posted for PR #5599 (a data-only config PR, so validation/perf are correctly
NOT RUN, with one[verified]process finding):Who can use it, & safety
AITER_REVIEW_AUTHORIZEDrepo variable (required): the job runs only for the comma-separated GitHub logins in that variable, checked at the job level before any checkout, so an unlisted commenter never starts a runner job. Until you set it, the bot ignores every comment — no spam, but no reviews either.authorized.txtin the skill dir seeds the initial list (zufayu) and is re-checked (exact match) inside the job; copy its logins into the variable at merge. A command in a quoted (>) line is ignored.issue_commentonly triggers from there). Thebox308runner and aiter-bot token are already provisioned — safe to merge and wire incrementally.permissions: contents: read; comments use the aiter-bot token, notGITHUB_TOKEN.REVIEW_AUTO_VALIDATE=0); enabling validation (opt-in, per-need) runs the PR's own test target on a locked GPU. Runs in the base (main) context; comment body read via env, not shell-interpolated.--dangerously-skip-permissions) and reads the PR diff, title and description as prompt input — untrusted content from any PR author, so indirect prompt injection is in scope. Containment relied on: the aiter-bot token is never in the review step's environment (only the claim / publish / notify steps carry it), the runner is a dedicated self-hosted box, and no PR code runs by default. Keep the runner's credentials and network egress minimal — the agent's blast radius is that box. Because the agent can read the runner account's files (not just its environment), that account's GitHub credential should be a dedicated read-only token (e.g. the aiter-botpublic_repoPAT), never a personal one.