chore(repo): sandbox CLI for review checkouts, owned review agents, correct diff ranges - #36584
Draft
AgentEnder wants to merge 6 commits into
Draft
chore(repo): sandbox CLI for review checkouts, owned review agents, correct diff ranges#36584AgentEnder wants to merge 6 commits into
AgentEnder wants to merge 6 commits into
Conversation
Adds `.claude/tools/sandbox`, a dependency-free Node CLI that gives the review pipeline one way to reach the code under review, whether that code is isolated in a container or sitting on this host. - Verbs: start / read / grep / find / exec / stop / list / prune / doctor. read, grep and find cover the whole reading protocol so read-only analysts never need `exec` just to search. - Backend selection is the CLI's job and fails closed. No skill or agent types an isolation flag again — the old RUNTIME_FLAG's failure mode was "no isolation, reported as success", because an unset var and the correct macOS value are byte-identical. - `start --local <path>` registers a host dir as a sandbox with a null backend, so local is just another backend to a reader. `exec` is refused there, and on Linux without gVisor, rather than degrading. - `read --ref` resolves the base revision, so "was this already true before the change?" needs no second checkout. - State lives in ~/.nx-sandboxes, outside the repo, so `git clean -xdf` cannot orphan multi-GB containers.
The review pipeline borrowed five agents from the pr-review-toolkit plugin. Plugin-owned agents cannot be edited, so every run patched them at runtime: the nine maintainer calibrations were copied verbatim into a /tmp charter each time, the EVIDENCE_LINE contract was restated in every dispatch prompt, and review-pr spent a paragraph shouting "do NOT run git status to discover scope" to override a plugin default. Eject them into .claude/agents and bake all of that in: - New: nx-code-reviewer, nx-comment-analyzer, nx-silent-failure-hunter, nx-test-analyzer, nx-type-design-analyzer. - Renamed with the nx- prefix so they cannot collide with the plugin's: alternative-approach, performance-analyzer, reproduce-verifier, security-analyzer. - nx-code-reviewer's "by default review unstaged changes from git diff" default is replaced with "scope is supplied by the caller; there is no fallback" — the host working tree is unrelated to the change under review, so that default was always the wrong answer. Every agent now reaches source only through the sandbox CLI, and no agent branches on transport. `docker exec`, the /work/nx paths and the "your native tools will silently find nothing" paragraph are gone. Whether the checkout is isolated is deliberately not observable, so there is no fallback path left to fall down — an agent is never told a native source read is an option at all.
…d agents review-pr now provisions the checkout with `sandbox start` and dispatches the repo's own `nx-*` agents, passing a `SANDBOX` id instead of a container name. Container paths leave the skill entirely: `sandbox exec` lands in the root of whichever side it runs against and selects the base side with `--base`, so there is no `/work/nx`, no `cd <dir>`, and no `PATH` export to forget at a call site. The `RUNTIME_FLAG` prose is deleted rather than ported. Isolation selection is the CLI's job now: on Linux it requires gVisor and refuses to degrade to runc, on macOS it treats the container VM as the boundary, and `sandbox doctor` reports what is actually in effect. The old variable's failure mode was "no isolation, reported as success" — unset, it expanded to nothing, byte-identical to the correct macOS value — and a decision that consequential does not belong in prose an agent can skip. `allowed-tools` narrows from six `docker *` grants to a single `.claude/tools/sandbox *`, so the skill can no longer reach the daemon directly. `PIPELINE_VERSION` 5 → 6: the file's own rule bumps it for a new agent set, so drafts written against the plugin-owned agents age out of the SHA dedup instead of being pinned forever. reproduce-issue and setup-review-sandbox pick up the agent rename.
…label Two follow-ups to owning the review agents. **Linear tool ids depend on how the server is installed.** The ticket fetch named only `mcp__plugin_linear_linear__*`, which is what the Linear plugin exposes; a plain MCP server install exposes `mcp__linear-server__*` instead. Since that varies per machine, grant both pairs in `allowed-tools` and tell Step 2 to use whichever the session actually exposes rather than hardcoding either. Neither is guaranteed to exist, and absence stays a fail-open case, not an error. **"The toolkit" no longer describes anything this skill runs.** The Step 5 agents are the repo's own `nx-*` set, so the label is relabeled to "the review agents" / "the Step 5 agents" throughout, and Step 5's heading with it. Two references stay, and are sharpened rather than removed: the `pr-review-toolkit` plugin really is still installed, and it ships both a `/pr-review-toolkit:review-pr` command and un-prefixed ancestors of every `nx-*` agent. Those are a live footgun — an un-prefixed `subagent_type` silently resolves to the plugin's agent, which reviews the host working tree and knows nothing about the sandbox, landing in exactly the empty-scope failure the surrounding section warns about. The section now names the collision and says to check the prefix.
`exec` was a boolean and `--local` was hard-refused, which papered over the fact that the local path was never implemented: `execIn` reached for `sb.backend`, which only container sandboxes have. Replaces the boolean with a tier — none < screened < full. Local defaults to screened, container to full, and `sandbox view <id>` mints a second id onto the same checkout at a narrower tier. A view can only narrow; escalating through one would make the tier meaningless, since anyone holding a screened id can mint views from it. The view is what makes "read but don't run things" enforceable. Agent frontmatter grants bare tool names (`Bash`), never per-verb patterns, so the constraint cannot be expressed there. Handing analysts a screened id and the reproduce-verifier the full one puts it in the one place that can hold it — and in container mode it genuinely binds, because the CLI is the analyst's only route to code that is not on the host. The screen rejects commands that obviously write — git worktree/index/ref writes, filesystem mutation, in-place sed/perl, package installs, `>`/`>>` — checking each segment of a compound command so `git log && git checkout main` is caught. It denies rather than allowlists on purpose: `screened` is used where the code is trusted, so the hazard is a review agent mutating what it was asked to review, not malicious code. An allowlist would reject every unanticipated-but-harmless command at a round-trip each. Rejections name the offending command and the read-only way to get the same answer, so the caller self-corrects: exec refused for sb_x — `git checkout`: it writes to the worktree, index or refs. To see a path at another revision: sandbox read <id> <path> --ref <sha> This is a guardrail, not a security boundary, and says so where it refuses.
Two range bugs let review agents report other people's work as the PR's, both found by re-reviewing PRs that had been rebased. **Step 4's incremental diff was unscoped.** `git diff $PRIOR_SHA..$HEAD` is the right operator for "what changed since I last looked" — both SHAs are heads of the same branch, so their merge base is useless — but it compares tree states, which assumes the branch grew by appending commits. On a rebase every commit that landed on master in between enters the range. On #36477 attempt 4 that was 24,236 lines across ~190 files for a PR touching 6; scoped to the PR's own paths, 417. The cost is the smaller half: this file is the agents' EVIDENCE_FILE, so an unscoped one lets an agent quote an unrelated merged PR and still pass its proof-of-work check. Adds the pathspec, plus a rebase detector — any file in the range that the PR does not touch proves a rebase, and that belongs in the draft. **Scoping by path is necessary and not sufficient.** For a file the PR touches and master also changed, the delta interleaves both authors with nothing marking whose is whose. On #36370 attempt 6 a docs page appeared in the scoped delta as a whole-page rewrite that was entirely master's; two of nine agents reported its dangling anchors as this PR's regression. They are broken on master too. Adds a line-set comparison against the PR's own merge-base diff, and routes a leaky delta back to the full diff rather than handing agents an unattributable surface. Line sets rather than hunk counts because the #36370 shape reads 1-vs-1 clean on counts while 39 of 41 added lines were master's. Agents now also get the authorship rule directly: a line is the author's only if it appears as an added line in the full diff, settleable in one grep. **Documents why the PR diff is fetched host-side at all.** The container's checkouts are `--depth 1`, so `BASE...HEAD` fails with "no merge base" and `BASE..HEAD` succeeds and is wrong — an inviting one-character "fix" that silently reviews everything merged into master since the fork. Names that trap, and pins $MERGE_BASE explicitly for the no-PR case. Moves the changed-file fetch up beside the diff, since Step 4's pathspec now needs it and the two must describe the same PR.
✅ Deploy Preview for nx-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for nx-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current Behavior
The PR review pipeline works, but only for a remote PR, and each skill reaches for the container by hand.
review-prandreproduce-issueeach build their owndocker runinvocations, so backend selection (docker / podman / colima / lima), isolation-runtime checks and resource limits are duplicated prose rather than one enforced implementation. There is no way to hand an agent a read-only handle to a checkout — agent frontmatter grants bare tool names (Bash), never per-verb patterns, so "read this code but do not run it" cannot be expressed.pr-review-toolkitplugin's agents, which review the host working tree. Against a sandboxed PR checkout that tree is unrelated and clean, so an agent that falls back togit statusgets an empty scope — and an agent that finds no files reports no issues. That is a confident clean review of nothing, indistinguishable from a real pass.$PRIOR_SHA..$HEAD) was unscoped. On a rebase, every commit that landed on master in between enters the range — measured on fix(core): maintain cacheability when an executor target default applies #36477 attempt 4: 24,236 lines across ~190 files for a PR touching 6. Because that file is the agents'EVIDENCE_FILE, an agent could quote an unrelated merged PR and still pass its proof-of-work check.Expected Behavior
1. A dependency-free
sandboxCLI (.claude/tools/sandbox) wrapping docker/podman/colima/lima with one uniform surface —start,exec,read,grep,find,view,stop,list,prune— over state in~/.nx-sandboxes. Backend selection is fail-closed: it refusesexecoutright when no isolation runtime is available rather than silently running untrusted code unconfined.execis a tier, not a boolean —none < screened < full. Local checkouts default toscreened, containers tofull, andsandbox view <id>mints a second id onto the same checkout at a narrower tier (narrowing only; escalating through a view would make the tier meaningless). This is what makes "read but don't run things" enforceable in the one place that can hold it: analysts get a screened id, the reproduce-verifier gets the full one. The screen denies rather than allowlists —screenedis used where code is trusted, so the hazard is a review agent mutating what it was asked to review, not malicious code — and every refusal names the offending command plus the read-only way to get the same answer. It says plainly that it is a guardrail, not a security boundary.2. The review agents are owned by this repo (
.claude/agents/nx-*). All 8 are ejected from the plugin, and the three analyzers that need code access read exclusively through the sandbox CLI, so paths and output are root-relative and identical whether the checkout is isolated or local — agents cannot tell the difference, and there is no host-path fallback to take when it is isolated. Scope is always passed explicitly; the un-prefixed plugin agents are called out as the empty-scope trap they are.3. The diff ranges are correct and their failure modes are documented.
grepsettles it.--depth 1, soBASE...HEADfails withno merge basewhileBASE..HEADsucceeds and is wrong. That one-character "fix" is inviting and silently reviews everything merged into master since the fork.4. Linear tool ids are accepted in both forms.
mcp__plugin_linear_linear__*(plugin install) andmcp__linear-server__*(plain MCP server) vary per machine, so both are granted and the skill uses whichever the session exposes. Absence stays a fail-open case, not an error.Scope note
Every changed file is under
.claude/— maintainer tooling for reviewing Nx PRs. No published package, no runtime code, and nothing in the Nx build graph is touched.Related Issue(s)
No tracking issue — internal review tooling. Supersedes the prose-only backend-agnostic work in draft PR #36480.