Deep code review of a single open PR in nrwl/nx. Checks the PR out only inside an isolated sandbox, then runs four fixed reviewers: implementation (correctness, errors, types, performance), verification (tests, ticket grounding, comments, and docs), approach, and security. A reproduce-verifier executes a runnable repro only when verification identifies one. The skill saves a GitHub-flavored draft to ~/.nx-pr-reviews/<NUMBER>.md and never posts it. Claude reads/executes PR code only through the sandbox CLI; credentials never enter the sandbox.
Deep code review of a single open PR in nrwl/nx. Checks the PR out only inside an isolated sandbox, then runs four fixed reviewers: implementation (correctness, errors, types, performance), verification (tests, ticket grounding, comments, and docs), approach, and security. A reproduce-verifier executes a runnable repro only when verification identifies one. The skill saves a GitHub-flavored draft to ~/.nx-pr-reviews/<NUMBER>.md and never posts it. Claude reads/executes PR code only through the sandbox CLI; credentials never enter the sandbox.
Runs this repo's review agents against a remote PR in nrwl/nx. The PR is checked out inside an isolated sandbox (gVisor on Linux, the Docker VM on macOS), the agents are dispatched with the PR's scope passed to them explicitly (Step 5), and their output is collected into a draft suitable for posting on GitHub.
Drafts only. This skill never posts to GitHub. The draft is reading material for the reviewer; if they want any of it on the PR, they post it themselves (or ask in the session, e.g. via gh pr review --body-file).
Trust model — why the sandbox
A PR is untrusted code. The dividing line is execution, not reading: the host may freely read public PR/issue information, but must never run PR-authored code (install scripts, builds, tests, the linked-issue reproduction). This skill enforces that with a strict split:
Host (Claude + its credentials): reads GitHub metadata and the diff (gh pr view / gh pr diff / gh issue view), orchestrates the agents, and reads the checked-out code only through .claude/tools/sandbox read/grep/find. Claude's auth token never enters the sandbox.
The sandbox: holds the PR checkout and is the only place any PR code executes — dependency installs, builds, tests, and the issue reproduction all run via sandbox exec.
The CLI owns isolation, and nothing above it names a runtime.sandbox start probes the available backends, picks the boundary (gVisor on Linux, the VM on macOS), and refuses to start at all when it cannot get a real one. This is the one thing that used to be a variable here, and its failure mode was "no isolation, reported as success" — an unset RUNTIME_FLAG expanded to nothing, which is byte-identical to the correct macOS value. Do not reintroduce a runtime flag anywhere in this skill.
Consequences that the rest of this skill depends on:
Never check the PR out into the host working tree. The checkout lives only inside the sandbox and is destroyed by sandbox stop.
The review agents cannot use native Read/Grep/Glob for PR source (those only see the host FS). They read it through the CLI, which presents identical commands whether the checkout is isolated or local — so no agent is ever told a native source read is an option. Read is still fine for host-side files this skill writes (the charter, the dumped diff).
If you ever catch yourself about to run npm/pnpm/nx/a test/the repro on the host, stop — route it through sandbox exec instead. See Step 3.
Inputs
<NUMBER> — the PR number in nrwl/nx. Required.
Configuration (env-overridable)
SANDBOX_IMAGE — the toolchain image the checkout runs in. Default: nx-review-sandbox:latest (built by the setup-review-sandbox skill). Claude runs on the host, not in this image.
SANDBOX — the sandbox id, returned by sandbox start in Step 3. There is no default and no name to guess: it is minted per run.
TRIAGE_DIR — where drafts live. Default: ~/.nx-pr-reviews (outside the repo — so git clean never touches drafts and re-review history survives — and outside ~/.claude, so the skill never writes into Claude Code's own config dir)
REVIEW_NONINTERACTIVE — set by headless callers (review-prs, the review cron) to skip Step 8.5's grill. Unset in a normal session. Default: unset.
NX_REPO_PATH — path to the local clone of nrwl/nx this skill ships inside. Default: git rev-parse --show-toplevel. Used only by the Step 4.5 close-signal checks, which may run before the sandbox exists, and always with a fresh git fetch first. It is never used for the PR checkout and is never passed to an agent — agents read base state with sandbox read --ref base, which is fetched fresh every run and cannot be stale.
Step 1: Pre-flight
gh auth status
mkdir -p "$TRIAGE_DIR"# Probe the backends and report what is usable. This subsumes the old uname/docker# info/runsc checks: the CLI owns backend selection, so asking it is the only# answer that matches what `sandbox start` will actually do.
.claude/tools/sandbox doctor
# Bring the image up to date. Do NOT probe whether it exists and skip on a hit: an image# built from ANY older revision passes an existence check identically, so a missing# capability is invisible and shows up only as a review that is slower or quietly weaker.# Observed: an image predating the pnpm-store warming went unnoticed for two weeks and cost# ~25 min of package downloads on every review.## Just build. Docker's layer cache makes this the right default rather than an expensive one:# - nothing changed -> ~0.6 s, every layer cached (measured)# - Dockerfile/mise.toml -> rebuilds from the changed instruction# - pnpm-lock.yaml moved -> re-runs `pnpm fetch`, which is the point: it keeps the warm# store matching the lockfile reviews actually install from# Concurrent runs are safe with no lock of our own — review-prs drives up to five parallel# `/review-pr` panes, and BuildKit deduplicates identical concurrent builds (measured: a 20 s# step ran ONCE across 5 simultaneous builds, all finishing in ~21 s rather than 100 s).
bash "$(git rev-parse --show-toplevel)/tools/review-sandbox/build-image.sh"
doctor reports each backend and whether it can isolate. You do not act on the detail and you never pass a runtime flag anywhere — sandbox start re-derives it and refuses if it cannot get a real boundary. Read doctor only to give the user a useful message before that refusal happens.
Fail fast with a clear message if: gh isn't authed; doctor reports no usable backend; or the image build fails. For the last two, point the user at the setup-review-sandbox skill — it installs Docker + gVisor, which the build above deliberately does not.
The build prints one line on the fast path (sandbox image up to date), so a slow first run after a lockfile change is expected and self-explanatory rather than a mystery.
title, author.login, headRefOid (the head SHA), headRefName, baseRefName, url
isDraft — if true, exit early (don't review drafts)
Local dedup: if $TRIAGE_DIR/<NUMBER>.md exists, its frontmatter head_sha equals headRefOid, its pipeline_version equals the current PIPELINE_VERSION (see below), and its verdict is not failed, this PR was already reviewed at this commit — exit with no draft change; log "ALREADY_REVIEWED". A failed draft never blocks a retry. To deliberately re-review an unchanged PR, delete the draft file or just say so in the session.
PIPELINE_VERSION: 9 — the current review-criteria generation. A draft whose frontmatter has an older pipeline_version (or none) was produced by a weaker pipeline: re-review even at an unchanged head_sha, treating the old draft as a prior review (Step 4). Bump this constant whenever the review criteria change materially (new agents, new calibrations, new required sections) so stale drafts age out instead of being pinned forever by the SHA dedup.
Fetch the tracking ticket
Much of the work in this repo is tracked in Linear, not in GitHub issues. A PR whose only reference is NXC-1234 is not an unlinked PR — it is a PR whose bug report lives somewhere you have to go and read. Treating "no Fixes #N" as "no grounding available" throws away the problem statement, the acceptance criteria, and usually the reproduction, and it silently degrades the reproduce-verifier to guessing from the PR body.
Extract every NXC-\d+ from the PR body and commit messages (also accept a linear.app/... link), then fetch each one:
mcp__plugin_linear_linear__get_issue with id "NXC-1234"
Also pull its comments when the description is thin — a repro often arrives in a follow-up comment rather than the original report.
From each ticket, keep:
The problem statement — what is broken, for whom, under what conditions.
The reproduction, if it has one. This is the highest-value field on the ticket: it is what Step 5a.5's Level 1 should actually run, and it is usually more precise than anything reconstructable from the diff.
Acceptance criteria / definition of done, if stated.
Then classify the reproduction once, here, and carry it to Step 5a.5 as REPRO_CLASSIFICATION:
RUNNABLE — the ticket (or a linked GitHub issue) carries a concrete command or a repro repo.
MANUAL_ONLY — the trigger needs a live second Nx process, an interactive terminal, a real
connected workspace, or network the sandbox lacks.
NONE — no ticket, tracker unreachable, or the ticket has no reproduction.
Deriving it here rather than in the agent is the point: it is one read of material you already have
open, and the verifier otherwise spends its opening tool calls rediscovering the same answer.
Fails open. No Linear tools configured, not authenticated (headless and cron runs often are neither), or the ticket is unreadable ⇒ continue exactly as before and note it. Never block a review on the tracker.
Two boundaries, both load-bearing:
Internal content never reaches $REVIEW_BODY.nrwl/nx is public and tickets routinely carry customer names, embargoed detail, and internal planning. The ticket informs what you check; anything in the posted draft must stand on public evidence — the diff, the PR body, a linked GitHub issue, the repo's docs, or something this review executed. Same rule Step 5c applies to Polygraph sessions, and for the same reason.
Carry the problem, not the verdict. The bug report and its reproduction are grounding, and every agent may have them. A maintainer's comment concluding "the right fix is X" is a rationale, and it belongs with the Polygraph session in Step 5c — handing it to alternative-approach up front is what destroys that agent's independence.
Step 3: Check the PR out inside the sandbox
Start a long-lived, locked-down sandbox and check the PR out inside it — the fetch and everything after run there; nothing lands on the host working tree. One call does the whole thing, and it picks the isolation runtime itself.
# Clear host artifacts left by any EARLIER run of this PR. Several later steps# gate on these files merely existing, so a leftover silently changes this run's# behaviour (see Step 4) — and a stale /tmp/repro-<NUMBER>.cmd would be executed# in the sandbox and its result attributed to this review.# NOTE: /tmp/pr-<NUMBER>.json is deliberately NOT cleared here — Step 2 wrote it# one step ago and Step 8 still needs it for the draft frontmatter.# /tmp/pr-<NUMBER>.session.json IS cleared: Step 5c writes it later in this run, so# anything present now is a previous run's session record for this PR — and a stale# one would be read as this run's, downgrading findings against an outdated record.rm -f /tmp/pr-<NUMBER>.diff /tmp/pr-<NUMBER>.diff.tmp /tmp/pr-<NUMBER>.files \
/tmp/pr-<NUMBER>.review-charter.md /tmp/pr-<NUMBER>.review-context.md \
/tmp/pr-<NUMBER>-incremental.diff /tmp/pr-<NUMBER>.evidence /tmp/repro-<NUMBER>.cmd \
/tmp/pr-<NUMBER>.session.json
# One call: starts a locked-down sandbox (caps dropped, no privilege escalation,# bounded memory/cpu/pids, correct isolation runtime — chosen by the CLI, and# refused outright if it cannot get a real one), shallow-fetches this PR's head,# and adds the base ref as a second checkout. Both sides exist before any agent# is dispatched. Capture the id: it is minted per run and there is no name to guess.
SANDBOX=$(.claude/tools/sandbox start \
--image "$SANDBOX_IMAGE" \
--checkout https://github.com/nrwl/nx \
--ref pull/<NUMBER>/head \
--base <BASE_REF_NAME> | head -1)
.claude/tools/sandbox exec"$SANDBOX" -- git rev-parse HEAD # HEAD_SHA# Install HEAD once, here, before any agent is dispatched. `exec` would install it# on first use anyway, so this is not what makes it correct — it is what stops# several agents from racing the same install, which can corrupt node_modules.# The base side is deliberately NOT installed here: `read --ref base` answers the# usual base question without running anything, and most reviews never run# base-side at all. The first `exec --base` pays for it if one does.
.claude/tools/sandbox install "$SANDBOX"
This is the slowest step in the skill, but the image ships a warm pnpm store, so the install mostly links rather than downloads. It buys correctness as much as speed: a deterministic tree at the versions the PR pins. Do not skip it, even for docs-only changes.
If it is unexpectedly slow, the image predates the warm store — rebuild it via setup-review-sandbox.
Notes:
No host mounts — the checkout lives only inside the sandbox. All caps dropped, no privilege escalation, resources bounded. The CLI applies all of this; none of it is yours to pass.
Efficiency: the gh-only close-without-merge signals (Step 4.5, signals 1–4 and 6–8) need no sandbox. For a first review, you may run those cheap signals first and only start the sandbox if no strong close signal fired — a superseded/unnecessary PR then costs no sandbox. For a re-review, Step 4's incremental diff needs it, so start it before Step 4. Either way, once created it must be torn down in Step 9.
The image carries the repo toolchain (node/java/dotnet/rust/bun via mise) baked from mise.toml, and mise auto-installs the PR's pinned toolchain on first exec. It bakes nonode_modules — that is what the install step above is for.
tsc and eslint come from that install, so agents get the versions the PR pins rather than an arbitrary latest. Report the install outcome in the charter (Step 5).
exec puts the mise shims on PATH and lands in the right checkout for you. Do not add a cd or a PATH export of your own — written out per call site, that export was the thing that got forgotten, and a bash -lc without it fails with No version is set for shim: npm, an error with nothing to do with the PR.
The --depth 1 fetch gives full working trees at HEAD and base — enough for reading every changed and surrounding file.
Read base state with --ref base, never from a host clone. It is fetched fresh from the remote on every run, so it is always the PR's actual base. A maintainer's local clone can be weeks stale, which would silently answer "was this behavior already there?" against the wrong tree — the question calibration 6 exists to settle.
The sandbox reading protocol (used by every agent below)
Each agent already carries this protocol in its own definition; what follows is here so you can check a dispatch prompt against it, not to be pasted into the charter. The PR source is not on the host — it is reached only through the CLI, which presents identical commands whether the checkout is isolated or local:
Those verbs each read a single side. To answer "what differs between base and HEAD?", compare
them with git through exec — both sides are worktrees of one repo inside the sandbox, so
origin/<BASE_REF_NAME> resolves from the HEAD side and git compares tree hashes instead of walking
files:
Never compare the two sides with a recursive filesystem diff. The HEAD side is fully installed, so
diff -r walks a complete node_modules tree for minutes — measured at ~148s of CPU on a live
review — and piping through grep -v node_modules does not help, because the walk is the cost. A
read-only lane cannot run exec at all; a single file's base version is read <path> --ref base,
which needs no install and no comparison.
Hand the read-only lanes a narrowed id.sandbox view mints a second id onto the same checkout at a lower exec tier, so "this agent may read but not run things" is enforced by the sandbox rather than by instructions — agent frontmatter grants bare tool names (Bash), never per-verb patterns, so it cannot be expressed there:
READONLY_SANDBOX=$(.claude/tools/sandbox view "$SANDBOX" --exec none | head -1)
Give $READONLY_SANDBOX to alternative-approach, and $SANDBOX to the lanes that may need to run something.
If an agent must edit tracked files, apply a patch, or run a command known to rewrite sources, it creates its own tree — never mutating the shared checkout:
.claude/tools/sandbox worktree <SANDBOX> <AGENT> head
That returns a new sandbox id, already installed, which the agent uses in place of its original. Pass base instead of head only when the experiment must mutate the baseline. One agent owns one tree; never share or reuse another agent's. If it is refused, the dynamic check is unavailable — report that rather than working around it. Build output and ignored caches from ordinary non-rewriting commands are fine in the shared checkout; the prohibition is against changes to tracked source or refs.
The diff — the primary review surface — is fetched host-side (it's public PR info) and written to a host file the agents can Read directly:
Write-then-verify-then-move, rather than redirecting straight onto the final path. A bare > truncates the target beforegh runs, so a token expiry or a transient 5xx leaves a 0-byte file that every agent is then told is "the complete PR diff" — and because the changed-file list is fetched by a separategh call, agents can end up with a populated file list and an empty diff, which is exactly the shape the Step 5 verification is least able to catch. Cross-check wc -l < /tmp/pr-<NUMBER>.files against the changedFiles count already parsed in Step 2 before dispatching anyone.
Hard rule for every agent: never execute PR code on the host. Any command that runs the checkout — npm/pnpm install, nx …, a build, a test, the linked-issue reproduction — goes through .claude/tools/sandbox exec "$SANDBOX" -- <cmd>, never bare on the host.
Step 4: Gather incremental-review context (only if a prior review exists)
If $TRIAGE_DIR/<NUMBER>.md already exists and its verdict is not failed, this is a re-review triggered by new commits. Build context for the toolkit so it can be conversational instead of starting fresh.
(If the existing draft's verdict is failedand its ## Review draft body is empty or has no findings, the prior attempt produced nothing usable — skip this step and review fresh. Do NOT discard it merely because the token says failed: since Step 7 now sets failed when any single agent fails its EVIDENCE check, a failed draft can still contain other reviewers' real findings, and throwing those away loses the reconciliation this step exists for. The file's history is preserved by Step 8 either way.)
Read the existing triage file in full — the whole ## Review draft plus every entry under ## Prior reviews. This is for you, the orchestrator: Step 5b reconciliation is explicitly yours to do ("don't dispatch another agent — you already have all the context"), so you need the complete history to sort findings into Addressed / Still concerning / New. Extract:
The frontmatter head_sha (call it $PRIOR_SHA) and verdict.
The ## Review draft section (the most recent review). This becomes "the prior review."
The full ## Prior reviews section (older reviews, if any). All of them — no cap on history.
What you pass to the agents is a different, much smaller artifact — see step 4. Keep the two straight: full history in your head, distilled carry-forward on disk.
Compute the incremental diff inside the sandbox, writing it to a host file the agents can Read. $PRIOR_SHA isn't in the shallow checkout, so fetch it first — and branch on whether that fetch succeeded:
if .claude/tools/sandbox exec"$SANDBOX" -- git fetch -q --depth 1 origin "$PRIOR_SHA"; then
.claude/tools/sandbox exec"$SANDBOX" -- git diff "$PRIOR_SHA".."<HEAD_REF_OID>" \
> /tmp/pr-<NUMBER>-incremental.diff \
|| { echo"FATAL: failed to build incremental diff"; exit 1; }
elseecho"PRIOR_SHA <PRIOR_SHA> no longer on the remote — force-pushed; reviewing fresh"fi
A failed fetch means the author force-pushed and orphaned $PRIOR_SHA. Treat that as a fresh review: set HAS_PRIOR_CONTEXT=false, skip the incremental diff, skip the remaining steps below entirely, and note the force-push in the draft. Do not fall through with an empty incremental diff — an empty diff reads as "nothing changed since the last review" when in fact the entire branch was rewritten. (GitHub keeps force-pushed head SHAs fetchable for a long time, so this branch is rare — the common rebase case lands in step 3.)
Set HAS_PRIOR_CONTEXT=true only on the success path. Step 5 gates on that variable, never on the context file existing — file existence is not a safe signal, because a prior review of the same PR leaves one behind and it would silently narrow this run's scope to a stale delta. (Step 3 of the skill also clears these paths up front, so the two defenses are independent.)
Base-movement guard: do not trust the raw range after the merge base changes. Resolve both merge bases on the host because credentials never enter the sandbox:
OLD_MB=$(gh api "repos/nrwl/nx/compare/<BASE_REF_NAME>...$PRIOR_SHA" \
--jq .merge_base_commit.sha) \
|| { echo"FATAL: failed to resolve prior merge base"; exit 1; }
NEW_MB=$(gh api "repos/nrwl/nx/compare/<BASE_REF_NAME>...<HEAD_REF_OID>" \
--jq .merge_base_commit.sha) \
|| { echo"FATAL: failed to resolve current merge base"; exit 1; }
test -n "$OLD_MB" && test -n "$NEW_MB" \
|| { echo"FATAL: empty merge-base SHA"; exit 1; }
REPLAY_FALLBACK=false
If OLD_MB == NEW_MB, the raw $PRIOR_SHA..HEAD range contains only the branch endpoint delta. Count its changed paths and keep it as the incremental surface:
Write a context file at /tmp/pr-<NUMBER>.review-context.md (host-side — the agents Read it directly; it is our file, not PR code).
Distill; do not paste. Every byte here is read by every agent you dispatch, so its cost is multiplied by the whole fleet — on a PR with several prior attempts, pasting full bodies makes the carry-forward the single largest fixed charge in the run, larger for most agents than the diff they are meant to review. Worse, it is mostly inert: the bulk of a prior draft is that round's Reproduction / Approach / Performance / Security prose, which describes work already done and re-verified from scratch this round by the agents that own those dimensions. What an agent genuinely needs from history is short: what is still open, what was already fixed, and which trade-offs are settled so it does not re-litigate them.
Write this shape instead, and keep the whole file under ~80 lines:
# Re-review context
Attempt <N-1> reviewed `$PRIOR_SHA` and returned **<PRIOR_VERDICT>**. This is attempt <N>.
Earlier attempts: <onelineperattempt, oldestfirst — "attempt2 (1046ace) lgtm — daemonnowrejectsforeign-workspacemessages">.
## Open items — I re-checked these at HEAD; cite them, do not re-verify<EveryunresolvedCritical/ImportantfindingfromANY ,
' ;
—
— "
, " " <>". An agent that
reads a bare open item will go and re-open the same three files to check it; an agent that
reads your verified status will cite it and move on.>
Step 4.5: Close-without-merge check
Before running the toolkit, do a cheap pass to answer: "Should this PR be closed without merging?" Two flavors:
Superseded — master or another PR already addressed the goal.
Unnecessary — the change shouldn't be merged at all (no real bug, abandoned, out of scope, duplicate of rejected work).
Both save the toolkit's effort on PRs that won't merge anyway. Signals 1–4 detect supersession; signals 6–8 detect unnecessary; signal 5 detects an unconfirmed bug (it can push to blocked, never to a close). Run the gh-only signals here. Signal 5 depends on the reproduce-verifier and is finalized after Step 5a.5.
These signals close other people's work, so bias every judgment call toward the contributor: when a signal is ambiguous, treat it as not fired.
Supersession signals (gh-only, run now)
1. Mergeability. If master moved in the same files, the PR is stale.
Flag if mergeable == "CONFLICTING" or mergeStateStatus == "DIRTY".
2. Cross-references on linked issues. Has another merged PR referenced the same issue?
Parse closingIssuesReferences from the PR body + gh pr view (look for Fixes #N, Closes #N, Resolves #N). For each linked issue:
Pick the 2-3 most-touched distinctive files — skip monorepo hot files (package.json, lockfiles, migrations.json, versions.ts) that unrelated PRs touch constantly. Only flag a hit when the merged PR's title suggests the same goal as this one; same-file overlap alone is not competing work.
4. Target-state check. For small PRs (< 50 lines changed OR touches only package.json / versions.ts / migrations.json), peek at master to see if the target state is already there.
Confirm NX_REPO_PATH really is an nrwl/nx clone before trusting it — its default is git rev-parse --show-toplevel, so invoking the skill from some other repo would silently point this signal at that repo's master. Then refresh the remote-tracking ref and read each changed file:
if git -C "$NX_REPO_PATH" remote get-url origin 2>/dev/null | grep -q 'nrwl/nx'; then
git -C "$NX_REPO_PATH" fetch -q origin <BASE_REF_NAME>
git -C "$NX_REPO_PATH" show origin/<BASE_REF_NAME>:<path>
elseecho"NX_REPO_PATH is not an nrwl/nx clone — skipping signal 4 (would read the wrong repo's master)"fi
The if/else must actually gate the fetch+show. A … || { echo "skip"; } form prints the warning and then runs them anyway — and signal 4 can recommend closing a contributor's PR, so reading the target state from the wrong repo's master is a confident wrong closure. (Verified: the ||-only form reaches both commands.)
(If the sandbox already exists at this point, prefer sandbox read "$SANDBOX" <path> --ref base and skip the host clone entirely — it needs neither the origin check nor the fetch.)
Compare key lines against what the PR is trying to set. Example: if the PR changes "@foo/bar": "^1.0.0" → "^2.0.0" but master already has "^2.3.3", flag it. The fetch is not optional — this signal can recommend closing someone's PR, and a local clone that is weeks stale would answer "is the target state already on master?" from the wrong tree. (If the sandbox already exists at this point, read --ref base is equivalent and needs no fetch.)
For larger PRs, skip this — the toolkit will catch subtler issues.
Unnecessary signals
5. Bug not confirmable. Finalized after Step 5a.5. If the reproduce-verifier returns BUG_NOT_REPRODUCED_ON_BASELINE, treat that as inconclusive, not proof of a non-bug — many nx bugs are environment-specific (package manager, OS, node version), so a local non-repro proves little. Look for corroboration in the linked issue instead:
# Has a maintainer engaged with the issue?
gh issue view <ISSUE> --repo nrwl/nx --json comments --jq '[.comments[].author.login]'
A Linear ticket is corroboration, and usually stronger than a GitHub comment — it means the team tracked the work deliberately. Step 2 has already fetched it, so check it here before concluding the bug is unconfirmed. Firing this signal on a NXC-… PR purely because it has no GitHub issue would push a tracked, triaged piece of work toward blocked for the sole reason that its tracker is not GitHub.
If no nrwl-org member has confirmed the bug, no tracking ticket describes it, AND the PR body offers no rationale of its own (no root-cause explanation, no design-doc link), the right outcome is a question, not a closure: flag it, push the verdict toward blocked, and have the draft ask the author for a runnable reproduction. This signal never forces unnecessary.
6. Stale + abandoned + conflicted. All three together:
Last commit on the PR branch > 90 days ago: parse commits[-1].committedDate from gh pr view ... --json commits.
Has merge conflicts (signal 1 fired).
Has unanswered reviewer questions: most recent non-author comment is unanswered. Check via gh pr view <NUMBER> --json comments --jq '.comments | map({author: .author.login, at: .createdAt}) | last' — if the last commenter is not the author and the timestamp is > 30 days old, it's unanswered.
If all three fire, the PR is abandoned and unlikely to land. Any sign of recent author engagement (a comment within the last 30 days, even without new commits) resets this signal — prefer the stale-branch advisory instead.
7. Duplicate of recently-closed-without-merge PR. Search closed-but-not-merged PRs touching the same primary file in the last 6 months:
Filter to entries where mergedAt is null (closed without merging). Only flag when a closed PR has a clearly similar title or approach — not merely the same file — and note that the prior close may have been for fixable reasons (stale, author gave up), which weakens the signal.
8. No linked issue + speculative scope. All of:
No Fixes #N / Closes #N / Resolves #N reference in body or commits. A Linear reference (NXC-XXXX, or a linear.app/... link, whether phrased "Fixes" or "Relates to") counts as a linked issue — do NOT treat a Linear-only PR as unlinked; many nx PRs track work in Linear rather than GitHub. Step 2 has already fetched that ticket, so judge this signal on what the ticket actually says: a NXC-… whose ticket states a real problem satisfies the linked-issue check outright. Only a reference that resolves to nothing readable leaves the PR effectively unlinked.
The PR body doesn't explain why the change is needed — no motivation, no linked discussion. Judge the substance, not the length.
PR modifies > 100 lines OR touches public-API surface (packages/*/src/index.ts, files matching *.public.ts, anything under packages/*/index.ts).
Speculative refactors without a stated reason are usually closed. Advisory-strength signal — flag in the section, but don't on its own force a verdict.
Emit
If any signal fires, prepend a ### Close-without-merge check section to $REVIEW_BODY (above ### Reproduction verification):
### Close-without-merge check<pickthestrongestline — onlyoneverdict-line, butmultipleadvisorylinesOK:>- 🛑 **Likely superseded.**<reason, with linked PR numbers / file evidence>- 🛑 **Likely unnecessary.**<reason — namethesignal(s) thatfired:abandoned, duplicateof #N, etc.>- ⚠️ **Bug unconfirmed.** Couldn't reproduce the linked issue on master and found no maintainer confirmation — the draft should ask the author for a runnable repro.
- ⚠️ **Stale branch.** Merge conflicts with master on <N> files; author should rebase before review lands.
- ⚠️ **Speculative scope.** No linked issue and no stated motivation for a large change.
- ✅ No close signals — PR is current and well-scoped.
Verdict influence (Step 7):
Superseded (strong) → verdict superseded. "Strong" means ANY of: signal 2 fires (another merged PR closes the same issue), OR signals 3+4 both fire (same-file merged PR AND master already at/past the PR's target state). The section should include the specific superseding PR number(s) so whoever closes the PR has a concrete pointer to cite.
Unnecessary (strong) → verdict unnecessary. "Strong" means ANY of: signal 6 fires (stale + abandoned + conflicted, no recent author engagement), OR signal 7 fires (duplicate of declined work with clearly matching scope). Signal 5 is never part of this — an unconfirmed bug pushes toward blocked with an ask-the-author question, not toward a close.
Both fire → supersession wins (more specific framing, gives the author a concrete pointer).
Stale branch alone (only signal 1) → advisory; still run the toolkit, still pick a verdict normally.
Speculative scope alone (only signal 8) → advisory; note it in the review body, don't force a verdict.
Clean → no section emitted.
If all signals are cheap-negative, skip emitting the section entirely (no noise on healthy PRs).
Early exit on a strong close signal
If superseded (strong) or unnecessary (strong) fired, skip Steps 5 through 5b entirely (the four reviewers, reproduce-verifier, and reconciliation). The verdict precedence in Step 7 already decides the outcome, so agent findings can't change it. Set $REVIEW_BODY to just the close check and continue with Steps 6-10.
Step 4.7: Measure shared load-bearing claims ONCE, before dispatching
Some PRs turn on a single mechanical fact that every agent would otherwise re-derive
independently: what a module graph actually loads, what a changed lint config actually blocks,
whether a deleted user-facing message is still emitted somewhere else. These are expensive to
establish (install a toolchain, compile, instrument, run a matrix) and cheap to check once you have
the answer.
Left alone, the pipeline pays for that establishment once per agent, and every one of them reaches
the same answer. So: measure first, then hand the result to the agents as a claim to attack.
When this step applies
Only when the diff makes a mechanical, globally-relevant assertion that more than one agent's
dimension depends on. Signals that it does:
A comment or PR-body claim about module load order or laziness ("non-agentic runs never load
X", "this import is deferred").
A changed lint / CI / build config whose effect is the point of the change.
A removed log, warning, or error, justified as "the sink already reports it".
A claimed behavioral parity between two code paths ("the worker mirrors the classic loop").
A factual claim about an external dependency's behavior, especially across versions — "the
upstream package never reports X in any mode", "that field only exists from v21". One npm pack +
grep settles it; left unmeasured, every agent whose dimension touches it packs the same tarballs.
This shape hides because the claim is usually in a comment, so it reads as the comment-analyzer's
private business — but reachability claims of this kind set the severity of the whole PR, which
means the reproduce-verifier and the code reviewer need them too.
A change to a shared signature or call contract: a new parameter, a widened argument list, a
new option threaded through a function with several call sites.
That last one needs looking somewhere different. The four above it are claims the diff makes about
itself, so the diff contains the evidence; a signature change's expensive facts live in the code
around it. Measure these outside the changed lines:
Is the new argument genuinely inert for the callers that do not pass it? (Read the dependency's
own source for the falsy guard; do not assume.)
What arity does every call site actually pass? Enumerate them once.
Does any consumer reach the symbol through an untyped dynamic require/import, or across a
package boundary where the two sides version independently? That is what decides whether an
options-object refactor is even available, and it is invisible to a reader of the diff alone.
If the diff makes no such claim, skip this step entirely — most PRs will.
Finish this step before you dispatch anyone. A measurement taken after dispatch reaches nobody:
the charter is read once, at the start of each agent's run, so a late ## Established measurements
entry is invisible to every agent already working, and they each re-derive it. Observed: an
external-dependency version claim measured after dispatch was independently re-derived by three
agents at roughly 5-7k tokens each. If you think of a measurement mid-flight, you have two options —
neither is "write it to the charter and hope": either accept the duplication, or SendMessage the
specific agents whose dimension needs it. Prefer to catch it here by walking the trigger list above
once, deliberately, before the first Agent call.
How to do it
Keep the shared checkout immutable. Read it through sandbox read/grep directly. If
the measurement needs to create a harness, edit tracked files, or run source-rewriting tooling,
run .claude/tools/sandbox worktree "$SANDBOX" orchestrator-head head; it returns a new
sandbox id for an already-installed tree. Use a separately created
orchestrator-base base worktree if the baseline measurement also writes. Never copy or patch
files into the shared reference worktrees.
Use the prepared worktree's install — cd into that worktree first so mise resolves the
correct toolchain.
Prefer the method that reproduces the real build. For "is this import lazy?", transpile the
entry module with tsc --module commonjs and walk require() calls at column 0 of the emit
(indented ⇒ inside a function ⇒ lazy). Only TypeScript's own emit applies its real elision rules,
so a hand-written import parser over-approximates and a grep is simply wrong.
Measure the comparison points too — the base (--ref base / exec --base) and, on a re-review, the prior
SHA. If prior needs its own worktree, fetch it into /work/repo.git, add a uniquely named
the prior SHA, then run sandbox worktree "$SANDBOX" orchestrator-prior base; it
runs both required setup commands before returning. A number without its baseline cannot answer
"is this net-new?", which is the admission test's first question.
Measure the corollaries each dimension will ask for, not just the headline conclusion. This is
what decides whether the step actually suppresses duplication. An agent whose own question sits
one hop from your conclusion will rebuild the whole harness to answer that hop, and the
measurement buys nothing. Once a rig is standing, extra observations off it are nearly free, so
take them:
You measured that a timeout releases the event loop. Also record what the call returns
and what it logs on that path — those are the error-handling and comment-accuracy
dimensions' versions of the same experiment.
You measured that a request is torn down. Also record what the server saw — that answers
"did a stray write reach the wire?" for the security dimension.
You measured a cost. Also record it for the base and for the untouched sibling path, so the
performance dimension does not re-run it to get a comparison.
Ask, per dimension: what would this agent want to observe on the rig I already have standing?
Record the method, not just the number. The charter entry must let an agent re-run it.
Leave the rig standing, and say where it is. This is the highest-leverage part of the step and
the easiest to skip, because by the time you have your number the harness feels like scaffolding.
It is not — it is the expensive part, and every agent that wants to run anything will rebuild it
from scratch.
Observed on a single PR: six separate harnesses were built to do the same thing — load the shipped
implementation by transpiling its real source and push inputs through it. The orchestrator, the
silent-failure hunter, the code reviewer, the type analyzer, the test analyzer and the
reproduce-verifier each solved module resolution, each wrote the transpile boilerplate, and
several each hit the same cd-outside-the-mise-tree failure first.
So: when your measurement needed a harness, save it in the prepared orchestrator mutation
worktree at a stable path (<something>-probe.js in the rig sandbox — under the
worktree, or require() cannot resolve workspace modules), make its inputs a parameter rather
than a hard-coded list, and give the charter the literal command that runs it. Agents may run
that shared rig read-only; if they need to edit it, they copy it into their own prepared mutation
worktree first.
Reuse the plumbing, never the cases. The adversarial value of independent agents lives in
which inputs each one thinks to try; it lives not at all in who wrote the ts.transpileModule
call. Hand over the loader and the runner; let every agent bring its own matrix. Word the charter
entry that way explicitly — "here is a rig that executes the shipped code, bring your own inputs"
— because a rig presented alongside a case list invites agents to read the case list as the
territory and stop there.
The same applies to anything else you stood up that was expensive and is reusable: an installed
base-side dependency, an extracted tarball, a snapshot at /snap.
How to write it into the charter
Add an ## Established measurements section (see the Step 5 template). Frame every entry as a
measured claim the agent is invited to falsify — never as settled truth:
Measured, not asserted. Do NOT re-derive these from scratch; that duplication is the single
largest avoidable cost in this pipeline. DO challenge any of them if your own reading of the
code contradicts it — say so explicitly and show what you saw. A contradiction is a finding.
That phrasing is load-bearing. "Here is the answer" makes agents incurious; "here is my
measurement, break it if you can" keeps the adversarial value at a fraction of the cost. The
independence that matters — alternative-approach arriving uninformed about the author's reasoning — is untouched, because a mechanical measurement
is not a rationale. Keep giving them the measurement; keep withholding the Polygraph session until
Step 5c.
Record what you tested and found clean, not only what you found. A negative result is as
suppressive as a positive one and costs an extra line. If your matrix covered a case that looks like
the obvious place for this change to break — the shape a reader would reach for first — say that you
tested it and that it held. Otherwise every agent that has the same good instinct spends the same
tool calls confirming your silence. Observed working: a charter that recorded "the guard-shape
difference produces no divergence, including the case that difference would most plausibly expose"
drew zero re-tests from every agent, while the one measurement left out of the charter was re-derived
by three.
Never put a conclusion here that you did not personally run. Every reviewer trusts this section,
so one error fans out across the whole review.
Step 5: Run the review agents
First, write a review charter at /tmp/pr-<NUMBER>.review-charter.md (host-side) so the agents self-filter up front instead of generating findings that get trimmed later.
The charter carries only what is true of THIS run. The reading protocol, the proof-of-work contract, and the maintainer calibrations all live in the agent definitions now — an agent has them before it reads anything you write. Re-stating them here is how they drift: two copies of a rule, one of which is rebuilt from a template on every run. What cannot live in an agent is the part that changes per PR, and that is exactly what belongs below.
# Review charter## Toolchain (already installed — do not install your own)
The HEAD checkout is installed, so `tsc`, `eslint`, `jest` and the repo's own scripts are available
at the versions this PR pins. Do not install your own copies in the shared checkout — you would get
different versions and could corrupt `node_modules` for the agents running alongside you. A tree from
`sandbox worktree` is the exception, and it arrives already installed.
The base side is not installed until something needs to run there; the first `exec --base` handles it
for you. `read --ref base` needs no install at all.
<IFtheStep3installdidnotreportOK, REPLACEthefirstparagraphwiththeactualoutcome — forexample, "theinstallfailed, sodonotruntestsoreslint; restrictyourselftoreading" — ratherthanleavingagentstodiscoveritonefailedcommandatatime.>## The problem being solved<OMITunlessStep2fetchedatrackingticket (Linear `NXC-…`) orthePRlinksaGitHubissue.><Theproblemstatementand, ifthetickethasone, thereproduction — inthereporter'sterms, nottheauthor's.Thisiswhatthechangeismeanttofix; judgethechangeagainstit.Carrytheproblemandtherepro.LeaveOUTanycommentthatconcludeswhatthefixshouldbe, andanyinternaldetail (customernames, embargoedcontext) — itmustnever >## Orientation — where this change sits
Facts about the code **around** the diff, gathered once so you do not each spend your first several
tool calls rediscovering them. This is context, not conclusions: it says what the code is, never
whether the change is good. Verify anything you intend to lean on; correct it if it is wrong.
<Fillinfromreadsyouaredoinganywaybeforedispatch.Keepitto ~15lines.Include:- **Thechangedsymbols** — onelineeach:whatitdoes, exportedormodule-private.- **Whocallsthem** — thecallsites, withpaths, fromone `sandboxgrep <SANDBOX><symbol> packages`.
Note any reached through a dynamic `require`/`import`, across a package boundary, or a test-only path.
-**Base behavior** — what the same code did at the base revision (`read --ref base`), including
before/after types for changed exports.
-**Where it sits in the flow** — the entry point that reaches this code, and what gates it.
Leave out PR rationale and prior conclusions; call sites and base behavior are sufficient.>
## Established measurements<OMITTHISSECTIONENTIRELYunlessStep4.7ran.Whenitdid, pasteitsresultshere.>
Measured by the caller before you were dispatched — **not asserted, and not the author's word.**
Do NOT re-derive these from scratch — that duplication is the single largest avoidable cost in this
pipeline.
DO challenge any of them if your own reading of the code contradicts what is written here. Say so
explicitly and show what you saw — a contradiction between this section and the code is itself a
finding, and a valuable one. Reuse these as a _starting point_ for your own dimension's questions,
not as a place to stop.
<Foreachmeasurement:theclaim, themethod (specificenoughtore-run), andtheresult —
includingthebaseand, onare-review, thepriorSHA, so "isthisnet-new?" isanswerable.Wherethemeasurementwasclean, sayso — "testedX, nodivergence" — sonobodyre-testsyoursilence.>### Reusable rig<OMITunlessStep4.7leftaharnessorotherexpensivesetupstanding.Whenitdid, listeachone:thepath, theliteralcommandthatrunsit, andwhatitdoes.Stateplainlythattheinputsaretheagent'stochoose — therigexistssonobodyrewritestheplumbing, NOTsoeveryonereusesonecaselist.Examplewording:
<name>-probe.js in sandbox <RIG_SANDBOX> executes the SHIPPED implementation (it transpiles the
real source; it is not a reimplementation). Run it with:
.claude/tools/sandbox exec <RIG_SANDBOX> -- node <name>-probe.js <inputs>
Supply inputs your dimension cares about without editing this shared rig. If an edit is
unavoidable, get your own tree from `sandbox worktree` and copy it there first.
Also list any expensive setup that is reusable rather than re-creatable: a base-side dependency you
installed, an extracted tarball, a `/snap` snapshot.>
## What to report
You and the author share a goal: get this PR merged without letting bad code
in. A finding is not a rejection — it is the distance between the PR and merge,
stated precisely enough that the author can close it. That stance changes
nothing about rigor (the admission test below still gates every finding); it
changes what a finding must contain: the defect, the proof, and the way out.
Report **critical** and **important** findings, plus **strengths**. Concrete,
actionable nice-to-haves (a rename, a restructure, a missing cross-link) may go
in a terse **Suggestions** list — one line each; vague polish will be discarded.
### The two tiers Critical — something this PR produces is WRONG, now.
Important — nothing is wrong now, but the PR leaves something that
will be wrong later, or unguarded against becoming wrong.
Severity is **what happens to an affected user, never how many are affected.**
Windows-only, large-workspaces-only, one-rare-flag-only — name the condition in
the TRIGGER line and keep the tier. A Windows user hitting wrong output is
fully broken, and Windows is a supported platform, not an edge case.
**Critical** — any of:
1.**Wrong output, data loss, or a crash** on a supported path.
2.**A wrong or misleading error message.** Nx is a CLI: what it prints IS what
it produces. A message that routes someone to the wrong cause is wrong
output, not a wording problem.
3.**Docs that tell a reader to do something that does not work.** Same
argument — a page is a product surface.
4.**False coverage** — a test that cannot fail, or asserts the wrong thing.
Worse than no test: it certifies the bug as fixed and survives refactors.
5.**An exploitable source-to-sink path** on a default configuration.
6.**A breaking change** to a public API, generator schema, or executor option
with no migration.
7.**`claimed-fix`** — the PR does not fix what it says it fixes. The finding is
not the bug; it is that merging _closes the issue_, so the bug becomes
invisible and untracked.
There is no bounded version of wrong. If something the PR produces is wrong on a
reachable path, it is Critical — do not soften it because the path is narrow.
**Important** — any of:
1.**`widens`** — a real defect that predates the PR, whose reach the diff
extends. Important because the author did not cause it, not because the harm
is smaller. Holding a PR for a bug it did not write is contributor-hostile;
the root-cause fix belongs in its own PR.
2.**New user-facing surface with no docs** — a flag or option that works but is
undiscoverable.
3.**A comment the diff left false.** The misleading-error argument aimed at the
next maintainer instead of the user.
4.**A measurable, non-cliff performance regression.** A hang or a scaling cliff
is Critical; slower is Important.
**Missing tests are never a finding — Suggestions at most, including for the
behavior this PR changed.** Absence of a test is not a defect; a test that lies
is (Critical #4). Ask for the regression test in a one-line Suggestion and let
the maintainer decide whether to hold the PR for it.
Important still requires a **named mechanism** — the undocumented flag, the
comment that is now false, the number that regressed. "This feels risky" is
not an Important finding; it is not a finding.
**Widening that changes the KIND of harm is not `widens` — it is Critical.** If a
path that only ever saw internal values now takes user config, or a removed guard
makes an unreachable branch reachable, the buggy line is unchanged but the harm
is new. File it as a new defect and put the base evidence on the _reachability_,
not on the bug.
### Admission test (every Critical/Important finding)
Two failure modes dominate this pipeline's false positives: defects that were
already there before the PR, and defects nothing a real user does can reach.
Both read as legitimate findings, because both describe real code. So every
Critical/Important finding MUST carry these three lines, immediately under it:
NET-NEW: <base evidence — see below>
TRIGGER: <entry point → input → user-visible failure>
FIX: <the concrete change, 1-2 lines — see below>
**NET-NEW** must be one of:
-`base <path>:<line> — <what the base did instead>` — you read the base file
and the behavior differs. Quote it; a bare assertion is not evidence.
-`no base file` — the file is added by this PR.
-`widens <path>:<line>` — the defect predates the PR but the diff materially
extends it (new call site, new caller passing untrusted input, a guard
removed). Say what the diff changed about its reach.
-`claimed-fix` — the PR's stated purpose is to fix this exact behavior and it
does not. Name the ticket/PR-body claim.
If the same defect reproduces unchanged at `--ref base`, it is **pre-existing**
and it does not block this PR. The reviewer is deciding whether to merge _this
diff_, not whether the file is perfect. "The PR touched this function, so its
old bugs are in scope" is the specific mistake — touching a function does not
adopt it.
**It is still reported.** Emit it under a `PRE-EXISTING:` line instead of
dropping it — one per defect, no cap, in the same `file:line — defect` shape
plus the base evidence that proves it predates the diff:
PRE-EXISTING: <path>:<line> — <defect>. Present at base <path>:<line>.
The maintainer files follow-up tickets from these, so a bare "this is old" is
useless: the line must stand on its own once separated from the PR that
surfaced it. This is the one place a defect you are forbidden to block on still
reaches the reviewer intact — silently discarding it loses work nobody else is
positioned to redo.
**TRIGGER** must name a path a supported Nx workflow actually reaches: the
command or public API entry point, the input/config that gets there, and what
the user sees fail. Not a finding at Critical/Important if reaching it needs a
state the codebase never produces — an argument no caller passes, an env var no
supported flow sets, a dependency version outside the supported range, a
hand-edited internal file, or a `null` that every call site already excludes.
"A future caller might" is not a trigger; neither is "in theory". Demote those
to Suggestions, one line, and say what the unreachable precondition is.
Rarity is not the same as unreachability. A path a real user hits only on
Windows, only in a monorepo above some size, or only with a rarely-used flag
IS a trigger — name the condition. Cut the ones nothing reaches, not the ones
few people reach.
**FIX** names the concrete change: which function, what it should do instead —
one or two lines, sketch-level, not a patch. You already know the shape from
proving the TRIGGER; writing it down costs a sentence and turns the finding
from a verdict into a path to merge. Grade your own confidence: plain `FIX:`
when you are confident in the shape; `FIX (sketch):` when viable alternatives
exist or you have not traced every call site — a confidently wrong
prescription is worse than none. `FIX: unclear — <why>` is legal when the
right change hinges on a decision only the author or maintainer can make; name
that decision. Never invent a prescription to fill the line.
NET-NEW and TRIGGER are checked by the caller at trim time. A
Critical/Important finding that omits either, or whose NET-NEW cites no base
evidence, is demoted — so a real defect written up without them loses its
weight. A missing FIX never demotes a finding (the defect is real regardless);
the caller records the gap in `## Failures` instead.
When you endorse a debatable design decision (fail-open vs fail-closed,
normalization, escape hatches, compat trade-offs), say so explicitly in a
**Maintainer calls** line rather than folding it into an endorsement.
Your own definition carries the standing maintainer calibrations that bind your
dimension, and the proof-of-work contract. Both still apply; nothing here relaxes
them.
Substitute the real PR number for every<NUMBER> in the template. The one that matters most is /tmp/pr-<NUMBER>.diff, the primary review surface: leaving it literal points every agent at a nonexistent file, so no agent can produce a verifiable EVIDENCE line and the whole run degrades to all-agents-failed.
Also resolve the <IF …> / <OMIT …> / <For each …> placeholders in the template — the toolchain-unavailable branch, the ## Orientation body, and the ## Established measurements body. A charter shipped with an unresolved angle-bracket instruction tells every dispatched agent to follow an instruction meant for you.
Fill in ## Orientation on every review, and treat it as the cheapest thing in this step. Unlike Step 4.7, it is not gated on the diff making a claim — every diff has surrounding code, and on a first review that surrounding code is what each agent otherwise spends its opening tool calls reconstructing, arriving at the same answer separately. You are already reading most of it to write the charter and to pick a REVIEW TARGET. The rule that keeps it honest is the one in the template: call sites and base behavior in, rationale and conclusions out. A brief that says "foo() is called from these five places and previously returned null here" orients every dimension without touching what any of them is supposed to judge; a brief that says "the author chose X because Y" is the Polygraph session arriving early, and Step 5c exists precisely to keep that until last.
<EVIDENCE_FILE> is the one token that stays literal in the charter. It differs per agent (the reproduce-verifier keeps the full diff while the others may get the incremental one), so the charter deliberately defers it — "named in your dispatch prompt" — and each dispatch prompt resolves it to a real path. Substituting a single path into the charter would silently point some agents at a file they were never given.
Dispatch the review agents directly — NOT via the toolkit command
Scope is always passed explicitly. Every lane is dispatched with the diff and the sandbox id named in its prompt, and each agent definition refuses to discover scope from host git. That combination is what makes a clean review of nothing impossible: with the PR checked out in the sandbox and nothing on the host working tree, an agent that guessed at scope would find no changes and report "no issues" indistinguishably from a genuine pass.
Dispatch the toolkit's agents yourself instead, with the scope passed explicitly. Get the changed-file list first:
Same write-verify-move as the diff, and for the mirror-image reason: an empty .files with a
populated diff hands every agent a CHANGED FILES: heading followed by nothing. Abort if
wc -l < /tmp/pr-<NUMBER>.files does not equal the changedFiles count parsed in Step 2 — that
mismatch means one of the two gh calls silently returned a partial answer.
Pass the file list by path, not by value. Agents have Read and the list is a host file, so
pasting its contents into every prompt buys nothing and costs the whole list once per agent. The one
thing the paste bought — an agent noticing an empty scope — is already covered by the abort above,
which fires before any dispatch.
Choose the EVIDENCE surface
The proof-of-work line number each agent's definition requires is checked against one file, named
per dispatch as <EVIDENCE_FILE>:
First review, or no usable incremental diff → /tmp/pr-<NUMBER>.diff.
Re-review where REPLAY_FALLBACK=true -> /tmp/pr-<NUMBER>.diff.
Re-review where Step 4 set HAS_PRIOR_CONTEXT=trueandREPLAY_FALLBACK=falseandwc -l < /tmp/pr-<NUMBER>-incremental.diff is at least 40 → /tmp/pr-<NUMBER>-incremental.diff.
Pointing the proof at the incremental diff on a re-review does two things at once: it proves the
agent opened the surface that actually matters this round, and it stops agents grazing the full diff
for a quotable line. Below ~40 lines the far-half retry (see below) has too little room, so fall
back to the full diff.
The full diff stays available either way — as reference, not as the review target. Say which is
which; agents that are handed both without a hierarchy read both in full.
Then dispatch each agent with this prompt shape. All four lanes are project-local agent names, so
<SUBAGENT_TYPE> and <AGENT> are the same bare name — which is also what the evidence file paths
are keyed on.
Agent(
subagent_type="<SUBAGENT_TYPE>",
description="<AGENT> review of PR <NUMBER>",
prompt="""
Review PR <NUMBER> in nrwl/nx.
SCOPE — review exactly these changes. Do NOT run `git status` or `git diff` to discover scope:
the host working tree is clean and unrelated to this PR, so host git reports no changes. If you
find yourself with an empty file list, you have the wrong scope — re-read the inputs below.
- REVIEW TARGET: <EVIDENCE_FILE> (host file — read it with `Read`; this is what you review)
- CHANGED FILES: /tmp/pr-<NUMBER>.files (host file — one path per line; `Read` it)
- SANDBOX: <SANDBOX> (the checkout under review; reach it only with `.claude/tools/sandbox`)
- BASE_REF: <BASE_REF_NAME> (read base state with `sandbox read <SANDBOX> <path> --ref base`)
<ONLY IF <EVIDENCE_FILE> is the incremental diff, ADD:>
- FULL DIFF (reference only): /tmp/pr-<NUMBER>.diff — the whole PR against its base. Consult it to
understand context around a delta hunk; do NOT review it end to end. Prior rounds already reviewed
it, and this round's job is the delta.
Read /tmp/pr-<NUMBER>.review-charter.md (host file) FIRST. It carries this run's scope: the problem
being solved, orientation around the diff, the pre-installed toolchain, and any measurements already
established for you. Your own definition carries the reading protocol and the calibrations.
REQUIRED — open your report with the three proof-of-work lines your definition specifies, with
<EVIDENCE_FILE> as the file the line number refers to. A report without a verifying pair is
discarded and the agent recorded as failed — including one that found no issues.
<ONLY IF Step 4 set $HAS_PRIOR_CONTEXT=true, ADD:>
Also read /tmp/pr-<NUMBER>.review-context.md — a distilled carry-forward from prior reviews of this
PR: what is still open, what was already fixed, and which trade-offs are settled. Focus on what
changed since. Its open items were re-checked at HEAD by the caller before you were dispatched — cite
their recorded status rather than re-deriving it, and challenge one only if the code contradicts it.
It is deliberately NOT the full prior reviews — it lists only what carries forward, so never read it
as a statement that anything absent from it is fine.
"""
)
Dispatch these fixed lanes with the generic prompt above:
implementation-reviewer — correctness, errors/fallbacks, type/API contracts, and performance
verification-reviewer — tests, ticket grounding, comments, and docs
alternative-approach and security-reviewer run every review in Steps 5a and 5a.2.
The repo also owns comment-analyzer, docs-reviewer, performance-analyzer and security-analyzer — the deep single-dimension specialists whose beats the four lanes now cover in one pass each. They are not dispatched by default; reach for one only when a PR is dense enough in that dimension to be worth a dedicated pass, and say in the draft that you did.
Fixed review lanes
Every lane runs on every first review and re-review. A lane with no relevant surface returns a short *_SOUND result naming what it checked; it does not need a routing or discovery pass. This preserves coverage while eliminating duplicate reads inside the old specialist fleet.
Verify each agent actually reviewed something
A silent "looks good" from a reviewer that read nothing is the one outcome this pipeline must never produce. Every dispatched reviewer must prove it opened the artifact; one that cannot is a failure.
Demand a line number, not just a line. A filename is not evidence: the changed-file list is a host file every agent is told to Read, so an agent that opened nothing else can still cite one. Neither is a diff --git header (reconstructible from that list) nor — on a re-review — a bare code line (the prior-review context file quotes applied fixes, so the content of a + line is in the agent's sanctioned reading set even when its sandbox reads fail). The one thing an agent cannot produce without opening <EVIDENCE_FILE> is the line number of a +/- content line: line numbers appear in no prompt and in no prose. Require both:
REVIEWED: <N> changed files
EVIDENCE_LINE: <the line number in <EVIDENCE_FILE>, e.g. 214>
EVIDENCE_TEXT: <that exact line, verbatim — must begin with `+` or `-`, 20+ chars after the sign,
and not a `diff --git` / `index` / `---` / `+++` / `@@` line>
<EVIDENCE_FILE> is whichever surface you named as REVIEW TARGET for that agent — the full diff on a first review, the incremental diff on a re-review. Use the same file when you verify; checking a line number against a different file than the agent was pointed at fails every honest agent at once.
Verify by reading the diff yourself at that number. BOTH EVIDENCE_LINE and EVIDENCE_TEXT are agent-authored and untrusted — get them into shell variables only via the Write tool + $(cat …), never a bare LINE=<paste>. A bare assignment of the agent's line number is itself host RCE before any gate runs: LINE=1e touch /tmp/x # is bash assignment-prefix syntax — it sets LINE=1e and runstouch /tmp/x. So write both fields to files with the Write tool (no shell parses their bytes), then read them back with $(cat …). Run everything below as one shell invocation — the LINE=$(cat …) read and the case are a single block, because this harness does not persist shell variables between separate Bash calls (split them and $LINE is empty in the case, which fails an honest agent). It must emit exactly one token; do not paraphrase the checks into separate echo FAILED lines:
# Write /tmp/pr-<NUMBER>.line = EVIDENCE_LINE with the Write tool (no shell parses its bytes)# Write /tmp/pr-<NUMBER>.evidence = EVIDENCE_TEXT with the Write tool
LINE=$(cat /tmp/pr-<NUMBER>.line 2>/dev/null) # $(cat) never re-parses the bytes it reads
verdict=FAILED
case"$LINE"in''|*[!0-9]*) ;; # non-numeric → stays FAILED; sed must NOT run (see below)
*)
sed -n "${LINE}p" <EVIDENCE_FILE> > /tmp/pr-<NUMBER>.diffline
line=$(cat /tmp/pr-<NUMBER>.diffline) # $(…) strips the trailing newline sed adds
ev=$(cat /tmp/pr-<NUMBER>.evidence 2>/dev/null)
ifprintf'%s'"$line" | grep -qE '^[+-]' \
&& ! printf'%s'"$line" | grep -qE '^(diff --git|index |\+\+\+ |--- )' \
&& [ -n "$ev" ] && [ "$line" = "$ev" ]; then
verdict=VERIFIED
fi
;;
esacecho"$verdict"# the ONLY verdict — VERIFIED iff every check passed
Each element defeats a specific failure that real reviews of this skill actually hit — and every one was a bug in an earlier version of this very block:
One verdict variable, set only on the all-pass path. Do NOT write the checks as independent echo FAILED / echo VERIFIED lines. If they don't aggregate, an agent that forges EVIDENCE_LINE: 1 + the diff --git header (line 1 of every diff, and the first filename is in the prompt) makes the header-exclusion print FAILED while the final compare prints VERIFIED — a read-nothing pass. Verified: the non-aggregating form emits FAILED FAILED VERIFIED.
The case must GATE, not just warn. A bare case … ) echo FAILED ;; esac prints and falls through to sed, which is the dangerous part: GNU sed executes shell commands via its e command, so EVIDENCE_LINE = 1e touch /tmp/x # runs touch on the host — verified as real code execution inside the Linux container. EVIDENCE_LINE is agent-authored, so a non-numeric value reaching sed is host RCE, defeating the whole sandbox. Keeping sed inside the numeric *) branch is what closes it (pure digits can't carry an e command). This is the third time this mechanism reintroduced host execution; the integer gate is not optional.
Compare with $(cat …) and [ = ], not diff -q of the raw files.sed terminates its line with \n; the Writed evidence file usually does not, so a byte-exact diff -q FAILS an honest agent on the trailing newline — and Step 7 then flips the whole review to failed. $(…) strips trailing newlines from both sides. The contents go through cat and a quoted [ = ], never re-parsed by the shell, so no agent byte executes.
The line number is the core proof. It is in no prompt and in no prior-review prose, so an agent whose sandbox reads silently returned nothing cannot produce a valid one — the only defense that closes the re-review context-file leak. Headers and filenames are derivable from the prompt, which is why the ^[+-] and header-exclusion checks must actually gate.
[ -n "$ev" ] (the empty-evidence check): an empty pattern would otherwise match anything; a check for absence must not be default-open on absence.
^[+-] and the header exclusion together: a @@ hunk line is caught by ^[+-] (it starts with @); diff --git/index/+++/--- lines are caught by the header exclusion. Both are derivable from the file list, so only a real content line counts.
Both agent fields reach the shell only through Write + $(cat): EVIDENCE_LINE and EVIDENCE_TEXT are untrusted, and a bare LINE=<paste> executes the value via bash assignment-prefix syntax before the case gate. Round-tripping through a file keeps every agent byte out of any shell word.
One tolerated deviation: a markdown code-span wrapper. Agents recurrently return EVIDENCE_TEXT wrapped in backticks, sometimes also backslash-escaping the inner ones, despite the prompt saying not to. Strip an outer code span and any such escaping, then run the comparison unchanged. This concedes nothing: the line number is the proof of work, and the unwrapped text must still match the file byte-for-byte. Failing an honest agent over a formatting habit costs a whole re-review — Step 7 flips the verdict to failed, the one value that defeats Step 2's dedup. Record it as a protocol deviation in ## Failures, not as a failure.
This applies to endorsements too, and especially to them.APPROACH_SOUND, PERFORMANCE_SOUND, SECURITY_SOUND, DOCS_SOUND, and a NOT_ATTEMPTED reproduction all assert "I checked and found nothing" — a claim an agent that read nothing produces just as fluently, and which Steps 5a–5a.4 fold into Strengths as an affirmative statement that the dimension was audited. An endorsement must cost more evidence than a finding, not less. A *_SOUND verdict with no verified EVIDENCE line is recorded failed, never as a strength.
If EVIDENCE fails to verify, re-dispatch once — but never paste the answer. Restating the changed-file list is a no-op; the agent already had it. Pasting diff content into the retry prompt is worse than a no-op: it makes the retry's own check unfalsifiable, because the premise the whole mechanism rests on — the diff content is not in the prompt — becomes false exactly for the agent under suspicion. A retry that hands over the evidence launders a failed agent into a pass, and since verdict: failed only fires after two failures, it also means that verdict can essentially never fire.
Instead, keep the evidence out of reach and make the demand more specific:
Your previous EVIDENCE did not verify. Re-read <EVIDENCE_FILE> and give the EVIDENCE_LINE /
EVIDENCE_TEXT pair for a + or - line in the second half of the file (line number > <N/2>).
Verify exactly as above (same single-verdict block). Add the far-half check as an extra clause inside the numeric *) branch's if — e.g. && [ "$LINE" -gt <N/2> ] — where $LINE is already known to be pure digits. Do NOT put it before the case: there $LINE is unvalidated, so a [ "$LINE" -gt … ] on non-numeric input errors, and a standalone reject reintroduces the non-aggregating pattern the block exists to avoid. The line-number requirement already means an agent whose tools return nothing cannot pass; the far-half clause just stops it from replaying a number it kept from the first attempt.
If the second attempt also fails, record that agent as failed in the draft and in ## Failures (Step 8).
A failed agent is not a pass and not a silence — it changes the verdict. See Step 7: any dispatched reviewer that cannot prove it read the target is failed.
ملف SKILL.md هذا كبير جدا، لذلك يعرض SkillsMP القسم الاول فقط هنا.عرض على GitHub
exit
fi
If the merge bases differ, the raw range includes base-branch commits. Rebuild the incremental surface by replaying the prior PR patch onto the current merge base in a temporary Git index, then compare that expected tree with HEAD. Stream the trusted helper from the host into the sandbox; never execute a helper from the PR-controlled checkout. ${CLAUDE_SKILL_DIR} is substituted when the skill loads, so the input path does not depend on the current working directory:
if [ "$OLD_MB" != "$NEW_MB" ]; thenif .claude/tools/sandbox exec"$SANDBOX" -- bash -s -- \
"$OLD_MB""$PRIOR_SHA""$NEW_MB" \
< "${CLAUDE_SKILL_DIR}/scripts/replay-prior-patch.sh" \
> /tmp/pr-<NUMBER>-incremental.diff
then
PATCH_CHANGES=$(awk '/^diff --git / { count++ } END { print count + 0 }' \
/tmp/pr-<NUMBER>-incremental.diff) \
|| { echo"FATAL: failed to count replayed changes"; exit 1; }
case"$PATCH_CHANGES"in''|*[!0-9]*) echo"FATAL: invalid replayed-change count"; exit 1 ;;
esacelse
REPLAY_STATUS=$?
if [ "$REPLAY_STATUS" -eq 10 ]; then
REPLAY_FALLBACK=trueecho"Prior patch did not replay cleanly; reviewing the full PR diff"elseecho"FATAL: failed to rebuild incremental diff (exit $REPLAY_STATUS)"exit 1
fififi
The helper exits 10 only when --3way cannot replay the prior patch; every other nonzero exit is fatal. The temporary index preserves the prior author patch across non-overlapping base churn, including binary changes, modes, symlinks, unusual pathnames, and file-to-directory transitions. If the replay conflicts, the author may have resolved overlapping base changes manually. In that case, REPLAY_FALLBACK=true selects the full PR diff in Step 5 and keeps the prior review context; it never treats an uncomparable patch as an empty delta or assigns a numeric change count. If this block reports FATAL, stop the review because its evidence is invalid.
If REPLAY_FALLBACK=false and PATCH_CHANGES is zero, this was a base-movement-only push or an equivalent tree rewrite. Skip the remaining context-building and agent steps, re-verify the carry-forward yourself at HEAD, update the review body with "no author delta", then continue at Step 8 so history and cleanup still run. Any positive count continues to Step 5 regardless of line count; the existing evidence fallback handles a small but real author delta.
## Diff since last review (`$PRIOR_SHA..<HEAD>`)
<When `REPLAY_FALLBACK=false`:See /tmp/pr-<NUMBER>-incremental.diff for the author delta since the prior review.>
<When `REPLAY_FALLBACK=true`:Thepriorpatchdidnotreplaycleanlyonthecurrentbase, sothisattemptreviewsthefullPRdiff.Nonarrowerauthordeltaissafe.>
## Review focus
Focus on the named review target. The open items above are already re-checked; carry
their status into your report if your dimension owns one, but do not go and re-derive it. Do not
re-analyze unchanged code from scratch.
<Optionally:2-4specificquestionsthisroundshouldsettle, phrasedneutrally.>
Rules for the distillation:
Re-check open items once; move fixed ones to Already fixed.
When OLD_MB != NEW_MB, re-verify Already fixed items at HEAD too. Base movement can silently drop a landed fix. A dropped one goes back to Open items.
Never omit an unresolved finding; trim narrative first.
Preserve each finding's wording, location, and ask; omit old reproduction/approach/performance/security prose.
Carry facts, not a prior verdict's reasoning; keep focus questions neutral.
Full history remains in $TRIAGE_DIR/<NUMBER>.md; only this agent-facing digest is trimmed.
