# Review Pr > 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/.md and never posts it. Claude reads/executes PR code only through the sandbox CLI; credentials never enter the sandbox. - Skill: `gabrielmoreira/review-pr` (Agent Skill) - Install (CLI): `npx skillmds@latest add gabrielmoreira/review-pr` - Raw SKILL.md: https://api.skillmd.com/api/skills/gabrielmoreira/review-pr/raw - Safety review: pending (external: skill-scanner WARNING, skillspector CAUTION) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Security - Author: gabrielmoreira (https://skillmd.com/u/gabrielmoreira) - Updated: 2026-09-21 - Page: https://skillmd.com/skills/gabrielmoreira/review-pr --- # Deep PR Review (review-pr) 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 - `` — 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 ```bash 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. ## Step 2: Fetch the PR metadata ```bash gh pr view \ --repo nrwl/nx \ --json number,title,author,headRefOid,headRefName,baseRefName,url,isDraft,additions,deletions,changedFiles \ > /tmp/pr-.json ``` Parse out: - `title`, `author.login`, `headRefOid` (the head SHA), `headRefName`, `baseRefName`, `url` - `isDraft` — if true, exit early (don't review drafts) - **Local dedup:** if `$TRIAGE_DIR/.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. ```bash # 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-.cmd would be executed # in the sandbox and its result attributed to this review. # NOTE: /tmp/pr-.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-.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-.diff /tmp/pr-.diff.tmp /tmp/pr-.files \ /tmp/pr-.review-charter.md /tmp/pr-.review-context.md \ /tmp/pr--incremental.diff /tmp/pr-.evidence /tmp/repro-.cmd \ /tmp/pr-.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//head \ --base | 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. How slow depends on whether you are the first review in the shared host: the first install in a container copies the store out of the read-only image layer (~2.3 GB, minutes), and every install after it in that container hardlinks to what is already there (~12 s). So a cold host is slow once, not every time. If it is slow on a _warm_ host, 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. - **Reviews share the container, deliberately.** `start` does not create a container per review — it attaches to one shared host per image and gives this review `/work//{nx,base,mutations}`, worktrees of one shared repo. That is a disk and time optimisation, not a weakening: the boundary is still host vs. container, and isolation _between_ reviews was never claimed. It is why a review costs ~0.5 GB rather than ~4.9 GB — the pnpm store's copy-up out of the read-only image layer is ~2.3 GB, and a shared container pays it once instead of once per PR. Because the caps are now one shared budget rather than one per review, a runaway test in your PR can starve the reviews running beside it. - **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 **no** `node_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: ```bash .claude/tools/sandbox read [--range a,b] [--ref base] .claude/tools/sandbox grep [subdir] [--ref base] .claude/tools/sandbox find [subdir] [--ref base] .claude/tools/sandbox diff [--name-only] [-- ...] .claude/tools/sandbox exec [--base] -- ``` Those verbs each read a **single** side. To answer "what differs between base and HEAD?", use `diff`, which compares tree hashes instead of walking files: ```bash .claude/tools/sandbox diff --name-only .claude/tools/sandbox diff -- ``` **Do not spell the comparison out as `exec -- git diff origin/..HEAD`.** Reviews share one repo inside the container, so `origin/` is a single ref that the _next_ review's fetch fast-forwards — a review still running would silently start diffing against a base that moved, inflating its scope with commits its PR never touched. `diff` resolves the review's own immovable base ref, and being read-only it works from a `view` id that cannot `exec` at all. 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 single file's base version is `read --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: ```bash 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: ```bash .claude/tools/sandbox worktree 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: ```bash gh pr diff --repo nrwl/nx > /tmp/pr-.diff.tmp \ || { echo "FATAL: gh pr diff failed"; exit 1; } test -s /tmp/pr-.diff.tmp \ || { echo "FATAL: empty diff for a PR reporting changed files"; exit 1; } mv /tmp/pr-.diff.tmp /tmp/pr-.diff ``` Write-then-verify-then-move, rather than redirecting straight onto the final path. A bare `>` truncates the target _before_ `gh` 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 _separate_ `gh` 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-.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" -- `, never bare on the host. ## Step 4: Gather incremental-review context (only if a prior review exists) If `$TRIAGE_DIR/.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 `failed` **and 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.) 1. 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. 2. 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: ```bash if .claude/tools/sandbox exec "$SANDBOX" -- git fetch -q --depth 1 origin "$PRIOR_SHA"; then .claude/tools/sandbox exec "$SANDBOX" -- git diff "$PRIOR_SHA".."" \ > /tmp/pr--incremental.diff \ || { echo "FATAL: failed to build incremental diff"; exit 1; } else echo "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.) 3. **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: ```bash OLD_MB=$(gh api "repos/nrwl/nx/compare/...$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/..." \ --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: ```bash if [ "$OLD_MB" = "$NEW_MB" ]; then PATCH_CHANGES=$(awk '/^diff --git / { count++ } END { print count + 0 }' \ /tmp/pr--incremental.diff) \ || { echo "FATAL: failed to count incremental changes"; exit 1; } else .claude/tools/sandbox exec "$SANDBOX" -- \ git fetch -q --depth 1 origin "$OLD_MB" "$NEW_MB" \ || { echo "FATAL: failed to fetch merge bases"; exit 1; } 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: ```bash if [ "$OLD_MB" != "$NEW_MB" ]; then if .claude/tools/sandbox exec "$SANDBOX" -- bash -s -- \ "$OLD_MB" "$PRIOR_SHA" "$NEW_MB" \ < "${CLAUDE_SKILL_DIR}/scripts/replay-prior-patch.sh" \ > /tmp/pr--incremental.diff then PATCH_CHANGES=$(awk '/^diff --git / { count++ } END { print count + 0 }' \ /tmp/pr--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 ;; esac else REPLAY_STATUS=$? if [ "$REPLAY_STATUS" -eq 10 ]; then REPLAY_FALLBACK=true echo "Prior patch did not replay cleanly; reviewing the full PR diff" else echo "FATAL: failed to rebuild incremental diff (exit $REPLAY_STATUS)" exit 1 fi fi fi ``` 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. 4. Write a context file at `/tmp/pr-.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**: ```markdown # Re-review context Attempt reviewed `$PRIOR_SHA` and returned ****. This is attempt . Earlier attempts: . ## Open items — I re-checked these at HEAD; cite them, do not re-verify ". 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.> ## Already fixed — do not re-raise ## Settled maintainer calls — do not re-litigate ## Diff since last review (`$PRIOR_SHA..`) -incremental.diff for the author delta since the prior review.> ## 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. ``` 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/.md`; only this agent-facing digest is trimmed. ## 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. ```bash gh pr view --repo nrwl/nx --json mergeable,mergeStateStatus ``` 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: ```bash gh issue view --repo nrwl/nx --json timelineItems --jq '.timelineItems[] | select(.__typename == "CrossReferencedEvent") | select(.source.__typename == "PullRequest") | {pr: .source.number, state: .source.state, merged: .source.merged, mergedAt: .source.mergedAt, title: .source.title}' ``` Flag any other PR with `merged: true` — that PR may have fixed the same issue. **3. Same-file merged PRs since this PR opened.** Identify possibly-competing work. Get the PR's `createdAt` and `files[].path`, then: ```bash gh pr list --repo nrwl/nx --state merged --search " merged:>" --json number,title,mergedAt --limit 5 ``` 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: ```bash 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 git -C "$NX_REPO_PATH" show origin/: else echo "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" --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: ```bash # Has a maintainer engaged with the issue? gh issue view --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 --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: ```bash gh pr list --repo nrwl/nx --state closed --search " closed:>$(date -d '6 months ago' +%Y-%m-%d 2>/dev/null || date -v-6m +%Y-%m-%d)" --json number,title,closedAt,state,mergedAt --limit 10 ``` 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`): ```markdown ### Close-without-merge check - 🛑 **Likely superseded.** - 🛑 **Likely unnecessary.** - ⚠️ **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 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 whoe …(truncated)