feat(review): run the reviewed repository's own commands behind a container (#9556) (#9723)

* feat(review): run the reviewed repository's own commands behind a container (#9556)

A review executes the code it is reviewing. `build-test` runs whatever the
reviewed repository's `package.json` names — `npm ci` with its `preinstall` and
`postinstall` scripts, the build, the suite — and `test-efficacy` runs that
suite again once per baseline, control, mutant, hunk probe and revert. Both did
it as the invoking identity, and both handed the PR's code `process.env`
entire: on CI that carries `OPENAI_API_KEY` and `GH_TOKEN`. Reading them is one
line in a `postinstall`, and it needs none of the git-config machinery the
pipeline's threat findings are built on.

The boundary goes around the executions, not around the review agent. Wrapping
the agent was tried first and is the wrong shape: its secrets do not survive
the container's env allowlist, its `timeout` reaps the host-side client rather
than the container, its CLI version stops matching the runner's — and after all
of that the mount is the whole checkout, so `<repo>/.git` stays writable
anyway.

Three decisions the argv encodes, each measured rather than assumed:

- **The mount is the review temp dir, not the tree the command runs in.** The
  dependency farm links OUT of every tree — `exposeDependencies` points each
  package in the probe tree's `node_modules` at the review worktree's copy, 1
  722 of them on a live CI review. Mounting one tree would leave every link
  dangling. Every tree the pipeline builds is a sibling under `.qwen/tmp`, so
  one mount covers both ends while `<repo>/.git` stays outside it.
- **The environment is an allowlist**, not the inherited one.
- **The network is per command kind.** An install needs the registry; a build
  and a suite do not, and `--network none` keeps loopback so a suite that
  stands up a local fixture server still runs.

One ephemeral container per command. A long-lived one per phase would save
about 1–2% of the 540-second efficacy budget and would re-introduce exactly the
cross-run state #9221 spent rounds closing.

Off by default: containerising a build by surprise changes what native modules
compile against. `review.sandbox` is `off` | `auto` | `required`, read through
`operatorReviewSettings` — which skips the workspace scope, so a repository
cannot ship a `.qwen/settings.json` that switches off the containment existing
to contain it. `QWEN_REVIEW_SANDBOX` outranks it so CI can require containment
without depending on a settings file the runner may not carry.

Each of the three decisions is pinned by a test that goes red when that
decision alone is reverted.

* chore(review): regenerate settings.schema.json for review.sandbox

The generated JSON Schema is checked in and CI diffs it against a fresh run
(`npm run generate:settings-schema`). Adding `review.sandbox` to
`settingsSchema.ts` without regenerating left the two out of step, which is
what the "settings.schema.json is out of date" gate is for.

The second red check, `Post Coverage Comment`, failed at "Download coverage
reports artifact" — a consequence of the test job dying before it uploaded
one, not an independent failure.

* fix(review): make `required` actually refuse, and fix four wiring bugs the first cut shipped

Five Criticals from the review, all of them real, and the first two would have
made the feature not work at all.

- **`required` failed open.** `sandboxVerdict` produced a `refused` verdict and
  nothing consumed it: both call sites tested `kind !== 'container'` and fell
  through to the direct spawn with the full environment. Refusal is now decided
  ONCE, at the top of each phase, before anything executes — which is also the
  only place that can cover the route that never reaches a spawn: a repo whose
  toolchain cannot be scoped is handed to the AGENT's own shell, and a gate at
  the spawn would leave that wide open under the very policy forbidding it.
- **`SANDBOX` was a shortcut past the policy.** The first cut returned `direct`
  when the session was already sandboxed, reasoning that the outer boundary is
  the one the operator asked for. Wrong for this property: the CLI's own
  sandbox constrains the filesystem and hands the child `process.env` entire,
  and stripping the secrets is half of what `required` promises.
- **The probe suite baked in the host's Node path.** `process.execPath` does
  not exist inside the image, so every sandboxed probe would exit 127 and map
  baseline, control, every mutant, every hunk and the revert to inconclusive —
  zero evidence exactly when containment is on. It uses the image's `node` now;
  the vitest path resolves because it lives under the mount.
- **The mount root took the first `.qwen/tmp`, not the deepest.** A review run
  from inside another review's worktree nests them, and the first occurrence
  widens the mount to the outer temp dir — pulling `<repo>/.git` and every
  sibling checkout in, which is the one property the mount exists for.
- **No UID/GID mapping.** The default image runs as root, so the container's
  writes into the mounted trees were root-owned and every later host-side
  cleanup — the install-timeout `rmSync`, `discardWorktree`, the sweeps — hit
  EACCES, accumulating residue across reviews.

Two things the fixes themselves needed, found by checking them rather than by
being told:

- **Refusing with `toolchain: 'unsupported'` would have caused the regression it
  was closing.** That value has a documented meaning — the brief reads it as
  "build-test could not scope this repo, install and build it yourself" — so a
  refusal routed into it would have sent the agent to run the reviewed code by
  hand, unsandboxed. It is a distinct `refused` now, with a brief rule that says
  the evidence is unavailable and must NOT be reconstructed by hand.
- **A bare `--user uid:gid` resets `$HOME` to `/`**, which the mapped user
  cannot write, so npm fails before the install starts — `utils/sandbox.ts`
  copies the host's `$HOME` for exactly this. The container gets a writable
  HOME inside the mount, with the npm cache under it.

The duplicated mount-root arithmetic became one exported `mountRootFor`, which
is both how the copies stopped drifting and how the nested case got a test.
Every fix above is pinned by a test that goes red when that fix alone is
reverted.

* fix(review): stop the reviewed repository from deciding its own containment (#9556)

Round 2. The sharpest finding is that the guarantee this PR advertises was
true of one route and false through another.

**A repository could switch off the containment that exists to contain it.**
`operatorReviewSettings` skips the workspace scope precisely so a
`.qwen/settings.json` cannot set review policy — but the env layer that
outranked it is repository-controlled too: `loadEnvironment` walks up from cwd
and applies `<repo>/.qwen/.env`, from the very checkout under review, admitted
by default because folder trust starts off. `QWEN_REVIEW_SANDBOX=off` in a
committed `.env` disabled it. Three siblings were worse, because they have no
ordering to fall back on: `QWEN_REVIEW_SANDBOX_IMAGE` chooses the image the
reviewed code runs *inside*; `SANDBOX_SET_UID_GID=false` puts the container
back to root; `DOCKER_HOST` chooses which daemon answers, so `required` reads
as satisfied and whatever that daemon returns is scored as evidence.

`environment.ts` gains `isFileSourcedEnvKey`, and containment now reads only
the operator's settings or a real process variable. The policy additionally
only ever tightens, so even a genuine env value cannot lower a settings
`required` — which, as a mutation showed, is what actually protects the policy;
the file-source check is what protects the other three.

**`required` still failed open where the mount could not be built.** The gate
asked "did a runtime answer", never "can this phase be contained": with a
healthy daemon and a cwd outside `.qwen/tmp` — a `/review` of a local checkout
— the command fell through to the direct spawn with the full environment and a
report indistinguishable from a contained run. It asks the second question now.

**A refusal on `--resume` destroyed the run it was asked to continue.**
Returning a report let the handler's unconditional write overwrite the
in-flight one, and the refusal carries no run identity, so every later resume
failed the identity check even after the runtime recovered. It throws on a
continuation, which is the invariant the `!adapter` branch states in its own
words.

**The HOME added last round was itself cross-run state.** It lived on the
shared mount, and `sh -lc` sources `$HOME/.profile` while npm reads
`$HOME/.npmrc` — so one run's postinstall could plant what the next review's
install executes, with the network on. That contradicted this module's own
`--rm` "isolation by construction" claim, and it arrived with the fix for the
`$HOME` problem rather than in the original. HOME is a tmpfs now: discarded
with the container, never on the host. The npm cache stays on the mount, and
the comment says plainly that npm's integrity check is what stands between a
poisoned cache and a bad install.

**The mount root was lexical.** `resolve` never touches the filesystem, so a
symlink at or above `.qwen/tmp` — committable as mode 120000 — would have
widened a read-write bind mount to wherever it pointed. Every other creating or
destroying path in this pipeline refuses that; this one does now too.

Also: the new tests were platform-fragile in three places (Windows has no
`process.getuid`, and the documented `SANDBOX_SET_UID_GID` opt-out could fail
them on a developer's box), and the secret-leak check asserted against whatever
the runner happened to export rather than a planted canary. Both fixed, and the
mount-root tests now build real directories, which is how the symlink refusal
got pinned at all.

Every fix here is pinned by a test that goes red when that fix alone is
reverted — except the policy's file-source check, which a mutation showed is
redundant with the tightening rule, and which is documented as defence in depth
rather than claimed as load-bearing.

* fix(review): close the hand-off arm, and three fixes that were wrong at their edges

Round 3. Six Criticals, all of them real, and two were introduced by round 2's
own fixes.

- **The hand-off arm of the original blocker was still open.** The phase gate
  refuses when containment is impossible — but a repo the npm adapter cannot
  scope (yarn/pnpm/bun, no lockfile) reaches `unsupportedReport` with the gate
  satisfied: a runtime answered and the tree is mountable. That report tells the
  agent to install and build with its own shell, which nothing here contains. An
  inapplicable adapter is now a refusal under `required`, not a hand-off.
- **The mount-failure refusal fired under `auto` too**, and its message
  hardcoded `required`. Under `auto` the contract is "contain it when that is
  possible", so an unmountable tree falls back to the direct spawn — refusing
  there would have taken build/test and efficacy evidence away from every local
  review the moment a daemon happened to be running. Mine, from round 2.
- **`--workdir` got the lexical path while the mount got the realpath.** Round
  2 made `mountRootFor` canonicalise; the workdir did not follow, so on any
  layout where the two spellings differ — `/var` against `/private/var` is the
  everyday one — the container was handed a directory it does not have and
  every command would fail before starting. Also mine, from round 2.
- **The daemon scrub missed the indirection selectors.** `DOCKER_CONFIG`,
  `CONTAINERS_CONF` and friends name a config file that in turn names the
  daemon, the registries and the runtime: scrubbing the direct selectors and
  leaving these moves the same steering one level down.
- **`isFileSourcedEnvKey` matched case-sensitively.** Windows env lookup is
  case-insensitive, so a `.env` committed as `docker_host=…` reaches the child
  exactly as `DOCKER_HOST` would while the exact-case test answers "not from a
  file" about a value that is. Same class as `sanitizedGitEnv`'s case fold.

The gitfile finding stays deferred with its reasoning in the thread — it is not
a hole this PR opens, and closing it belongs to the identity gates rather than
to the sandbox.

Two mutations came back green on the first pass and were the useful part of
this round: the `DOCKER_CONFIG` scrub had no test because the fixture pinned one
key rather than the set, and the operator's `SANDBOX_SET_UID_GID=false` opt-out
had no test at all — both uid tests asserted the flag was PRESENT. The scrub
test now asserts the whole set, and the opt-out has its own.

* fix(review): the hand-off gate was dead code, and three more edges

Round 4.

- **Last round's hand-off gate never ran.** It tested `!applicable`, and
  `applicable` is the filtered adapter ARRAY returned by
  `selectToolchainAdapter` — never falsy. The gate shipped green and closed
  nothing. It is judged on the RESULT now (`toolchain === 'unsupported'`),
  which also covers the second route to a hand-off — an adapter that applies
  and cannot scope, from inside the npm one — and the predicate is exported and
  tested rather than living inline where no test could see it.
- **The boxed farm dangled under a symlinked ancestor.** Round 3 made the mount
  and `--workdir` canonical; `exposeDependencies` still built its link targets
  from the lexical root, so on the everyday macOS `/tmp` → `/private/tmp`
  layout every farm link resolved to a path the container does not have. The
  phase would then report "every file was red or collected nothing" — a wiring
  failure published as a statement about the PR's own suite. Canonicalised on
  the sandboxed path only; the direct path keeps the caller's spelling.
- **A timed-out boxed run leaked its container.** `--rm` fires only on a
  self-exit, and the deadline kills the runtime CLIENT — so a suite whose own
  trap ignores the forwarded signal keeps running with the review temp dir
  writable, past the budget and past the end of the review. Containers get a
  unique `--name`, and both spawn sites `rm -f` it when the deadline fires.
- **The daemon scrub deleted case-sensitively.** Round 3 taught
  `isFileSourcedEnvKey` to fold case on Windows and left the deletion exact —
  so a `docker_host` written by a repo `.env` was correctly detected and then
  not removed.

Two mutations came back green again, and both were the round's real lesson: the
hand-off refusal had no test (which is how its dead-code predecessor shipped),
and the farm canonicalisation still has none — it needs a symlinked-ancestor
fixture with a live runtime, which this machine cannot provide, and it is
listed with the other integration gaps rather than claimed.

* fix(review): put the hand-off conversion at the one exit, and make the reaper reachable

Round 5. The first finding is the same one for the third time, and the third
time is the one worth explaining: I had been guarding routes rather than the
exit.

- **Attempt one** tested `!applicable` — the filtered adapter ARRAY, never
  falsy — and was dead code.
- **Attempt two** wrapped the two `adapter.run` returns and missed the
  `!adapter` branch's own `unsupported` report.
- **Now** the conversion sits at the single place a report can reach a caller,
  and it is an exported `applyHandOffPolicy` rather than a branch inside a long
  function, so a test can reach it without a live container runtime. Both
  previous attempts failed the same way — a guard placed where no test could
  see it, in a function with more exits than the author was holding in mind.

**The container reaper added last round was unreachable, not wrong.**
`spawnSync` sends its `killSignal` at the deadline and then WAITS for the child
to exit, so an attached runtime client that forwards SIGTERM to a workload
whose own trap ignores it never returns — and the `killContainer` after it never
runs. The boxed spawns use `killSignal: 'SIGKILL'` now: the client cannot ignore
it, the call returns, and the container is reaped by name at the daemon, which
is where the deadline had to be enforced.

**The scrub missed the proxy family.** `HTTP_PROXY`/`HTTPS_PROXY`/`ALL_PROXY`/
`NO_PROXY` (and their lowercase spellings) are honoured by both clients for
every daemon call, so a repo-shipped one interposes on the connection the
direct selectors were scrubbed to protect.

One test was removed rather than added: a `expect(killContainer).toBeTypeOf(
'function')` I wrote to "document" the SIGKILL reasoning. It asserts nothing and
would have read as coverage; the reasoning belongs in the comment where it now
lives alone.

The `killSignal` choice is reasoned but not pinned — it needs a live runtime and
a TERM-ignoring workload — and joins the integration gaps already listed in the
PR body rather than being claimed.

* fix(review): the third continuation exit destroyed the report it was continuing

Round 6, two Criticals: the deferred gitfile one, and this.

Round 5's single-exit conversion runs on `--resume` answers too. A resumed
report whose toolchain is `unsupported`, under a policy that tightened between
the first call and the continuation, was replaced by a fresh refusal — which
the handler writes unconditionally, over the report the call was asked to
continue. That refusal carries no run identity, so every later `--resume` fails
the identity check and the round redoes install, build and every suite.

"A continuation must never answer with a FRESH report" is enforced by a throw
at the refusal gate and at `!adapter`. This conversion was added after both and
did not have it. It does now.

The trigger is ordinary rather than adversarial: the policy is read per call,
so an operator raising it — or a workflow's `env:` — between call one and the
resume is enough, on exactly the unscopeable repo shapes (yarn/pnpm/bun) that
reach a hand-off at all.

**The first test I wrote for this passed without the fix.** It drove
`runBuildTest` with an incomplete argument object and asserted `.toThrow()`;
the throw it saw was `--timeout must be a finite number of seconds`, from
validation long before the code under test. A mutation caught it. Making the
fixture faithful means reproducing the whole resume-identity apparatus, so the
decision is a predicate instead — `resumeWouldDestroyReport`, tested directly,
with the wiring left as one visible line. Same shape as round 5's
`applyHandOffPolicy`, and for the same reason: a guard a test cannot reach is a
guard that has already been wrong twice here.

* fix(review): reap the container on every abnormal exit, not only the timeout

Round 7, one Critical.

`build-test` reaped the boxed container when `spawnTimedOut(r)`, which is true
for ETIMEDOUT and false for a `maxBuffer` overflow — and a reviewed command
writing 64 MB to one stream is a postinstall away. The client dies with
ENOBUFS, the reap is skipped, and the container keeps the review temp dir
mounted read-write past the per-command deadline and past the end of the
review.

The sibling in `test-efficacy` already reaped on the broader `r.error ||
r.signal`. That the two had drifted to different conditions is how one came to
miss a case the other caught, so they now share one exported predicate:
`status === null` — exactly "the client did not exit normally", covering
ETIMEDOUT, ENOBUFS and signal kills in one condition rather than a list of
causes to keep in sync. A normal exit needs no reaping (`--rm` has fired), and
a client that never spawned has no container, where the reap is a silent no-op
by `killContainer`'s construction.

* fix(review): scrub the container client env by provenance, not by name

The scrub list named the daemon selectors, then the proxy family, and still
missed DOCKER_API_VERSION — which selects no daemon at all. It just makes
every client call fail, and under `sandbox: auto` a failing availability
probe reads as "no runtime here", so a repository that ships one value in
its .env turns containment off without a word.

The class has no last entry. Replace the list with the rule it was trying to
approximate: a key the loader wrote from a file the reviewed checkout
supplies does not reach the process that decides whether containment
happened. The client needs no repository-provided environment for anything,
and the workload's environment is an allowlist either way.

Deleting restores rather than approximates: the loader records a key as
file-sourced only where the real environment had nothing, so a file value
never shadows an inherited one and the client keeps the operator's own
DOCKER_HOST, PATH and HOME.

* fix(review): drop --user on rootless runtimes, and take the base-side rerun into the box

Two blockers from round 9.

R9-1. `--user uid:gid` is right on a rootful engine and wrong on a rootless
one. Rootless podman — its default install — and rootless docker run the
engine inside the invoking user's namespace, so container uid 0 IS the
invoking host user and every other container uid lands on a subuid near
100000. The availability probe answers fine, the phase gate passes, and then
every boxed command meets a tree it does not own: `npm ci` cannot create
node_modules, and what the container does create comes out unsweepable — the
cross-run residue `--user` exists to prevent. Detect rootlessness from the
runtime's own `info` document and drop the flag there, where the container's
root is already the invoking user and the flag has nothing left to do.

Unknown answers rootful. The two wrong guesses are not symmetric: guessing
rootful on a rootless host breaks the run loudly, guessing rootless on a
rootful one silently runs reviewed code as real uid 0 on a writable mount.

R2-21. test-delta kept a private copy of build-test's run(). The copy was
correct until build-test's grew a container, and then the two sides of the
measurement stopped being comparable: the PR side in the image with an env
allowlist and no network, the base side on the host with both. A test that
reads an env var or opens a socket flips on one side, and the file whose only
job is to say which side a failure belongs to says "the PR's". The duplicate
is deleted rather than re-synchronised, and `required` now refuses the
base-side rerun instead of running it on the host.

A refusal returns an empty report, so the brief now says in as many words
that an empty `netNew` beside such a note is the absence of a measurement,
not the absence of a regression.

* fix(review): refuse mount roots the -v grammar cannot spell, and stop the suite inheriting the operator's policy

R10-1. `-v src:dst` has exactly one separator, so a checkout at `/…/my:repo`
produces a spec docker answers with `invalid spec … too many colons`.
`mountRootFor` called that root mountable, which bypassed both designed
degradations: under `auto` every install/build/test command surfaced a raw
mount error the report attributed to the PR instead of falling back to the
direct spawn, and under `required` the phase gate passed and the refusal that
should have explained it never happened. Classify such a root as unmountable
and it rejoins the path every other unmountable root already takes.

Measured rather than assumed, including the suggested alternative: `--mount
type=bind` fails on the same colon AND on a comma that `-v` takes without
complaint, so it would trade one unspellable path for two.

On Windows this refuses every absolute path, deliberately: a drive letter is
a colon, and a mount whose source and target are the same path cannot be a
Windows path at all. Containment is not available there, and saying so is
what gives `auto` its fallback and `required` its refusal.

Hermeticity, which this PR broke. The phase gates read the operator's own
`review.sandbox`, so a maintainer who turns the feature on and runs the suite
watched 101 review tests report that setting back at them instead of what
they measure. Both routes are closed: the shared setup drops the environment
variables, and the two suites whose gates resolve settings isolate `QWEN_HOME`
the way they already isolate the host git config — measured at 101 → 0 from
each route independently. test-delta's gate is injectable for the same reason
its `exec` is.

Also from the round-10 list, all self-inflicted: the `--user` test pinned the
documented opt-out it asserts against, `runtimeIsRootless`'s unknown case is
now structural (an empty document carries no marker) rather than a literal a
mutant can flip, and the dead `resetContainerRuntimeProbe`/`force` pair is
gone. Two doc corrections: `containerPathFor` names the null it actually
produces, and the brief's `refused` bullet covers all three routes that
produce it rather than one.

* test(review): gate the mount-root cases Windows cannot be asked

Round 10 refused every absolute Windows path from `mountRootFor` — a drive
letter is a colon — and said so as intended behaviour. It is; what that reply
missed is that this file then asserts the opposite. Three cases here turn on a
root being MOUNTABLE, which is the one question Windows has no answer to: the
comma control added last round, the deepest-temp-dir selection, and the
symlink refusal's honest half. All three assert non-null against real
absolute paths, and the colon case also builds `my:repo`, a component name
Win32 rejects outright.

`test_windows` is merge_group-only and a required check, so nothing here
would have gone red until the merge queue itself.

Gated off win32 with the convention this directory already uses in 37 places,
and the shipped semantics pinned by a win32-only case rather than left to the
comment that describes them.

* test(review): make the Windows pin actually reach the check it pins

The win32 case added last round named a path that does not exist, so
`mountRootFor` returned null out of its realpath catch and the assertion held
whether or not the drive-letter check was there at all — probe-verified: the
layout is absent and the answer is null anyway. It pinned nothing.

Build the layout first and assert it exists, so the catch is closed and the
colon is the only thing left that can produce the null.

Two more from the same list. The uid cases returned early instead of
skipping, which reports PASSED with zero assertions on the lane where the
condition bites — the reading "this held" for something never checked. And
the install-network assertion looked only for a bare `none`, blind to the
joined `--network=none` a refactor could switch to; it now rejects the flag in
either shape.

* test(review): skip the second uid case instead of returning, and restore its stub in finally

The sibling got `it.skipIf` last round and this one did not, so on a lane
where `getuid` is undefined it still reports PASSED having asserted nothing —
which reads as "this held" for something never checked.

Its `SANDBOX_SET_UID_GID` stub was also cleared on the last line rather than
in a `finally`. There is no `unstubEnvs` in the vitest config and no
file-level hook that would catch it, so a failing assertion left the stub set
for whatever ran next — one red test quietly becoming two.

* test(review): drop the comment last round's edit left stranded

Wrapping the uid case in try/finally moved its opt-out note above the stub
and left the original copy inside the try, so the same three lines now appear
twice a few lines apart. Delete the stranded one.

* test(review): pin the four decisions this feature is sold on

Four cells the suite reached only by accident of the machine it ran on, each
carrying a live mutant on a property the PR description states as a
guarantee. All four functions already took their ambient dependency as an
injectable parameter, so this is assertions, not seams.

- sandboxPolicy: strictest-wins in BOTH directions, and a file-sourced value
  counting for nothing. "A repository cannot switch off the containment that
  exists to contain it" was described in a comment and asserted nowhere;
  env-overrides-settings, a looser strictest, and a dropped file-sourced guard
  all shipped green.
- sandboxVerdict: `auto` with nothing answering runs DIRECTLY. Every other
  case here either has a runtime or is `required`, so a mutant refusing
  instead — turning `auto` into `required` on every machine without docker —
  survived.
- refuseUnsandboxedPhase: the PASS path. Every other assertion about this gate
  is a refusal, so unconditional refusal under `required` — every review on a
  perfectly good host — survived.
- containerPathFor: it feeds `--workdir` at both spawn sites, and had no test
  at all. A lexical spelling names a directory the container does not have;
  the parent fallback is what lets a probe tree be named before it is built.

Mutation-checked, one at a time: seven mutants, seven reds.

* fix(review): read the policy setting the way an operator writes it

`"Required"` in settings.json — or a stray trailing space — matched no policy,
resolved to `off`, and disabled the containment the operator had just asked
for. Silently. The environment value was already normalised; the settings
value was not, and the asymmetry fell on the wrong side: settings is the
documented place to turn this ON, since the environment can only tighten. So
the unnormalised half was the half operators actually use, and a fail-open on
the one setting whose whole purpose is to fail closed.

Reported as a suggestion in rounds 8, 9 and 16 and deferred each time under
the critical-only posture. Measured this round: both `"Required"` and
`"required "` returned `off`.

Also pins two shapes the new tests left open. `sandboxPolicy`'s settings
default is the production path every real caller takes, and with it swapped
for `{}` the whole settings half stopped being consulted while every
assertion — each passing settings explicitly — stayed green; it is now driven
through an isolated settings file. And the `SANDBOX_SET_UID_GID` opt-out
parses case- and space-insensitively, which nothing asserted.

The remaining deferral, a fixture literal pasted twice, is duplication rather
than a defect and is left alone.

* test(review): finish the policy table instead of one row of it

Last round's normalisation fix was asserted only against `required`
spellings, which is the shape a mutant keyed on that one value walks
straight through — leaving an operator's `"Auto"` resolving to `off`, the
same silent downgrade one rung lower. Assert every policy on both sides.

Two adjacent cells with it. A garbled ENVIRONMENT value must be dropped on
its own rather than taking the operator's setting down with it: the
environment is the half a repository can reach, so garbage there must never
answer for the half it cannot. And `sandboxPolicy`'s env default is the twin
of the settings default pinned last round — every assertion here hands it an
env literal, so `env = {}` as the default stops the environment half being
read at all and nothing notices.

Three mutants, three reds.

* test(review): exercise the reap and the runtime probe, and stop leaking a fixture

Three of the four recorded this round; the fourth is duplication, not a
defect, and is left alone for the third time.

`killContainer` is the whole answer to a container that outlived the client's
deadline, and nothing anywhere ran it — a garbled argv, or a dropped `-f`,
shipped green, and what survives is a container holding the review tree open
past the end of the run. It now takes its spawn as a parameter, so the argv
and the swallow-and-continue contract are both asserted.

`containerRuntime`'s probe had the same hole with a memo and a real daemon
call in the way. The decision is now separated from both: `firstAnsweringRuntime`
is order and nothing else, which is the content — a client installed but not
running must never shadow one that is.

And the `containerPathFor` case left a temp tree behind on every run. In a
change about not leaving residue behind, that one is just embarrassing.

packages/cli/src/commands/review/build-test.test.ts

@@ -5,6 +5,7 @@
  */
 
 import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+import { isolateOperatorReviewSettings } from './lib/test-utils.js';
 import {
   mkdtempSync,
   mkdirSync,
@@ -17,7 +18,9 @@ import {
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import {
+  applyHandOffPolicy,
   run,
+  resumeWouldDestroyReport,
   runBuildTest,
   type BuildTestReport,
   trimOutput,
@@ -38,12 +41,20 @@ vi.mock('node:fs', async (importOriginal) => {
   return { ...mock, default: mock };
 });
 
+let reviewSettingsIsolation: ReturnType<typeof isolateOperatorReviewSettings>;
+
 beforeEach(() => {
   // Plenty of disk by default, so this suite behaves the same on a nearly-full
   // machine as on an empty one — the low-disk cases below opt in explicitly.
   statfsSyncMock.mockReturnValue({ bavail: 16 * 1024 ** 3, bsize: 1 });
+  // ...and the same for the operator's review policy: with `required` set in
+  // their own settings the phase gate refuses every run here, correctly, and
+  // 82 of this file's tests report that instead of what they measure.
+  reviewSettingsIsolation = isolateOperatorReviewSettings();
 });
 
+afterEach(() => reviewSettingsIsolation?.dispose());
+
 const PKGS: WorkspacePackage[] = [
   { dir: 'packages/core', name: '@x/core', scripts: ['build'], deps: [] },
   { dir: 'packages/webui', name: '@x/webui', scripts: ['build'], deps: [] },
@@ -4483,3 +4494,67 @@ describe('runBuildTest', () => {
     });
   });
 });
+
+describe('applyHandOffPolicy', () => {
+  const handOff = {
+    toolchain: 'unsupported' as const,
+    affected: [],
+    buildSet: [],
+    widenedWith: [],
+    install: null,
+    build: [],
+    test: [],
+    timedOut: [],
+    ok: true,
+    note: 'build-test could not scope this repo',
+  };
+
+  it('converts a hand-off to a refusal under `required`', () => {
+    // The hand-off tells the agent to install and build with its own shell,
+    // which nothing here contains. This conversion has been wrong twice — once
+    // as a precondition that was never true, once as a wrapper on two of the
+    // three routes that produce it — so what it produces is pinned here rather
+    // than left to the call site.
+    const got = applyHandOffPolicy(handOff, 'required');
+    expect(got.toolchain).toBe('refused');
+    // NOT `ok`, or a reader treats it as a clean hand-off and does by hand
+    // exactly what the policy refused.
+    expect(got.ok).toBe(false);
+    expect(got.note).toContain('do not run the commands by hand');
+    expect(got.build).toEqual([]);
+    expect(got.test).toEqual([]);
+  });
+
+  it('leaves a hand-off alone under the other policies', () => {
+    for (const policy of ['off', 'auto'] as const) {
+      expect(applyHandOffPolicy(handOff, policy)).toBe(handOff);
+    }
+  });
+
+  it('refuses to convert on a --resume, which would destroy the report', () => {
+    // The invariant the other two continuation exits enforce with a throw:
+    // "a continuation must never answer with a FRESH report". This conversion
+    // was added after both and returns one — which the handler writes over the
+    // report the call was asked to continue, and that refusal carries no run
+    // identity, so every later resume fails the identity check. A policy
+    // tightened between the first call and the resume is enough to trigger it,
+    // on the unscopeable repo shapes that reach a hand-off in the first place.
+    expect(resumeWouldDestroyReport(handOff, true, 'required')).toBe(true);
+    // A fresh call converts normally — that is the whole point of the
+    // conversion.
+    expect(resumeWouldDestroyReport(handOff, false, 'required')).toBe(false);
+    // And a resume of a real run is never touched.
+    expect(
+      resumeWouldDestroyReport(
+        { ...handOff, toolchain: 'npm' },
+        true,
+        'required',
+      ),
+    ).toBe(false);
+  });
+
+  it('never converts a real run', () => {
+    const real = { ...handOff, toolchain: 'npm' as const };
+    expect(applyHandOffPolicy(real, 'required')).toBe(real);
+  });
+});

packages/cli/src/commands/review/build-test.ts

@@ -42,6 +42,24 @@ import { spawnSync } from 'node:child_process';
 import { existsSync, readFileSync, statSync, writeFileSync } from 'node:fs';
 import { join, resolve } from 'node:path';
 import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
+import {
+  boxedRunLeftContainer,
+  containerCommand,
+  containerName,
+  containerPathFor,
+  handOffRefused,
+  killContainer,
+  sandboxPolicy,
+  mountRootFor,
+  refuseUnsandboxedPhase,
+  reviewSandboxImage,
+  runtimeIsRootless,
+  runtimeClientEnv,
+  sandboxVerdict,
+  type CommandKind,
+  type SandboxPolicy,
+  type ContainerRuntime,
+} from './lib/sandboxed-exec.js';
 import {
   DEFAULT_COMMAND_TIMEOUT_S,
   DEFAULT_WHOLE_CALL_BUDGET_S,
@@ -110,7 +128,14 @@ export interface CommandResult {
 
 export interface BuildTestReport {
   /** The scoped toolchain that ran, or `unsupported` when selection was unsafe. */
-  toolchain: 'npm' | 'unsupported';
+  /**
+   * `refused` is not a kind of repository — it is the absence of a run.
+   * `unsupported` means "this command could not scope your repo, go run the
+   * build yourself", which is a real instruction the brief acts on; routing a
+   * sandbox refusal into it would send the agent to run the reviewed code by
+   * hand with its own shell, which is the exact thing the policy forbade.
+   */
+  toolchain: 'npm' | 'unsupported' | 'refused';
   /** Workspace dirs the diff changed. */
   affected: string[];
   /** What was built, dependencies first — after any widening. */
@@ -310,10 +335,58 @@ export function buildRunEnv(
  * set is measured HERE, off the raw text, and survives a trim that drops the
  * FAIL lines it was parsed from.
  */
+/**
+ * The container argv for one reviewed-repository command, or null to run it
+ * directly.
+ *
+ * Null covers three cases and they are not the same thing: the policy is off
+ * (today's behaviour), no runtime answered under `auto`, or this command's cwd
+ * is not inside a review temp dir — which is the case for a `/review` of a
+ * local checkout, where the tree under test IS the user's own working copy and
+ * there is no `.qwen/tmp` sibling layout to mount. The `required` policy is
+ * NOT handled here: refusing is the caller's decision, because only the caller
+ * knows what evidence it is about to mark unavailable.
+ */
+function containerised(
+  command: string,
+  cwd: string,
+  kind: CommandKind,
+): {
+  file: string;
+  args: string[];
+  name: string;
+  runtime: ContainerRuntime;
+} | null {
+  const verdict = sandboxVerdict();
+  if (verdict.kind !== 'container') return null;
+  const tmpDir = mountRootFor(cwd);
+  if (tmpDir === null) return null;
+  // The CANONICAL spelling, matching the mount: the bind mount is created from
+  // the root's realpath, so a lexical `--workdir` names a directory the
+  // container does not have and every command fails before it starts.
+  const workdir = containerPathFor(cwd);
+  if (workdir === null) return null;
+  const name = containerName();
+  return {
+    ...containerCommand(command, {
+      cwd: workdir,
+      tmpDir,
+      kind,
+      name,
+      runtime: verdict.runtime,
+      rootless: runtimeIsRootless(verdict.runtime),
+      image: reviewSandboxImage(),
+    }),
+    name,
+    runtime: verdict.runtime,
+  };
+}
+
 export function run(
   command: string,
   cwd: string,
   timeoutMs: number,
+  kind: CommandKind = 'test',
 ): CommandResult {
   const started = Date.now();
   // spawnSync validates `timeout` as an unsigned integer: the adapters'
@@ -322,16 +395,50 @@ export function run(
   // with no report, or zero, which arms no kill timer at all. Coerce once
   // at the one boundary every command crosses.
   const deadlineMs = Math.max(1, Math.round(timeoutMs));
-  const r = spawnSync(command, {
-    cwd,
-    shell: true,
-    encoding: 'utf8',
-    timeout: deadlineMs,
-    maxBuffer: 64 * 1024 * 1024,
-    // A build that asks a question is a build that hangs until the deadline.
-    stdio: ['ignore', 'pipe', 'pipe'],
-    env: buildRunEnv(),
-  });
+  // This is the reviewed repository's own command — `npm ci` with whatever
+  // install scripts the PR committed, its build, its suite — so it is the
+  // thing #9556 is about. `containerised` returns null when the run is not
+  // sandboxed, and the direct spawn below is unchanged for that case.
+  const boxed = containerised(command, cwd, kind);
+  const r = boxed
+    ? spawnSync(boxed.file, boxed.args, {
+        cwd,
+        encoding: 'utf8',
+        timeout: deadlineMs,
+        maxBuffer: 64 * 1024 * 1024,
+        // SIGKILL, not the default SIGTERM, and only on the boxed branch.
+        // `spawnSync` sends its `killSignal` at the deadline and then WAITS for
+        // the child to exit — so an attached runtime client that forwards the
+        // signal and keeps waiting on a workload whose own trap ignores it
+        // never returns, and the `killContainer` below is never reached. That
+        // is what made the round-4 machinery unreachable rather than wrong.
+        // SIGKILL cannot be ignored, so the client dies, the call returns, and
+        // the container is then reaped BY NAME at the daemon — which is where
+        // the deadline had to be enforced all along.
+        killSignal: 'SIGKILL',
+        stdio: ['ignore', 'pipe', 'pipe'],
+        // NOT `buildRunEnv()`: the container gets an allowlist instead (see
+        // `containerEnv`), and this env is the RUNTIME CLIENT's — the caller's
+        // PATH and nothing from the review, minus the daemon-selecting
+        // variables a repository could have shipped in its own `.env`.
+        env: runtimeClientEnv(),
+      })
+    : spawnSync(command, {
+        cwd,
+        shell: true,
+        encoding: 'utf8',
+        timeout: deadlineMs,
+        maxBuffer: 64 * 1024 * 1024,
+        // A build that asks a question is a build that hangs until the deadline.
+        stdio: ['ignore', 'pipe', 'pipe'],
+        env: buildRunEnv(),
+      });
+  if (boxed && boxedRunLeftContainer(r.status)) {
+    // The deadline killed the CLIENT; the container outlives it — see the
+    // `--name` comment in `containerCommand`. Reach the daemon instead, then
+    // report the timeout exactly as before.
+    killContainer(boxed.runtime, boxed.name);
+  }
   // `spawnSync` sets `error.code === 'ETIMEDOUT'` when the deadline fired — that is
   // the authoritative signal. The `SIGTERM`/null-status pair is only a fallback: it
   // also matches an external SIGTERM (a container stop), and it misses a non-default
@@ -614,7 +721,53 @@ function previousReport(out: string | undefined): BuildTestReport {
   return parsed as BuildTestReport;
 }
 
-export function runBuildTest(args: BuildTestArgs): BuildTestReport {
+/**
+ * A report that says the phase ran nothing, and why.
+ *
+ * Module scope because two callers need it: the phase gate inside the run, and
+ * the hand-off conversion at the exit.
+ */
+function refusedReport(why: string): BuildTestReport {
+  return {
+    toolchain: 'refused',
+    affected: [],
+    buildSet: [],
+    widenedWith: [],
+    install: null,
+    build: [],
+    test: [],
+    timedOut: [],
+    // NOT `ok: true`. `unsupportedReport`'s hand-off is `ok` because nothing
+    // was found wrong; here something WAS — the phase could not be run under
+    // the policy in force — and a reader that treats this as a clean hand-off
+    // would go do by hand exactly what the policy just refused.
+    ok: false,
+    note:
+      `no build or test evidence: ${why}. This phase would have had to run ` +
+      `the reviewed repository's own commands, which is what the policy ` +
+      `forbids — so it ran nothing rather than running them unsandboxed. Do ` +
+      `not read this as a passing build, and do not run the commands by hand ` +
+      `to fill the gap.`,
+  };
+}
+
+/**
+ * The hand-off is an EXECUTION too, and it is the one that leaves this
+ * process: `unsupportedReport` tells the agent to install and build with its
+ * own shell — see the `toolchain: "unsupported"` rule in the brief — and that
+ * shell is contained by nothing here. The phase gate cannot catch it, because
+ * the gate passes exactly when a runtime answered and the tree is mountable,
+ * which is when a repo the adapters cannot scope still reaches the hand-off.
+ *
+ * At the ONE exit every report crosses, and that placement is the point. The
+ * first attempt tested a precondition (`!applicable` — the filtered adapter
+ * ARRAY, never falsy) and was dead code. The second wrapped the two
+ * `adapter.run` returns and missed the `!adapter` branch's own `unsupported`
+ * report. Both were the same mistake at different addresses: guarding routes
+ * one at a time in a function with several. There is exactly one place a
+ * report can reach a caller, so the conversion belongs there.
+ */
+function runBuildTestUnguarded(args: BuildTestArgs): BuildTestReport {
   // yargs `type: 'number'` coerces `--timeout abc` to NaN rather than
   // rejecting it; NaN defeats every budget-floor comparison and reaches
   // spawnSync as an invalid deadline — ERR_OUT_OF_RANGE with no report.
@@ -739,6 +892,33 @@ export function runBuildTest(args: BuildTestArgs): BuildTestReport {
     previous,
     exec: args.exec ?? run,
   };
+  // BEFORE anything is executed or handed off. Under `review.sandbox: required`
+  // with no container runtime answering, this phase must produce no build/test
+  // evidence rather than produce it by running the reviewed repository's code
+  // unsandboxed. It sits here and not at the spawn because one route never
+  // reaches a spawn at all: a repo this adapter cannot scope is handed to the
+  // AGENT's own shell (`unsupportedReport`), which would otherwise run the
+  // install and the suite with nothing consulted.
+  const refusal = refuseUnsandboxedPhase(root);
+  if (refusal && args.resume) {
+    // THROW on a continuation, never return. The handler writes whatever this
+    // returns to `--out`, which on a resume is the very report the call was
+    // asked to continue — so returning the refusal below would overwrite a
+    // partial run's install, builds and finished suites, and the refusal
+    // report carries no run identity, so every later `--resume` would fail the
+    // identity check ("records no run identity") even after a runtime came
+    // back. One transient probe failure would cost the round its whole
+    // build-test chain. This is the invariant the `!adapter` branch below
+    // states in its own words; a policy refusal is subject to it too.
+    throw new Error(
+      `refusing to continue this run: ${refusal}. The report at ${args.out} ` +
+        `is left as it was — re-run without --resume once the policy can be ` +
+        `satisfied, or lower review.sandbox.`,
+    );
+  }
+  if (refusal) {
+    return refusedReport(refusal);
+  }
   const { adapter, applicable } = selectToolchainAdapter(
     root,
     toolchainAdapters,
@@ -905,3 +1085,72 @@ export const buildTestCommand: CommandModule = {
     }
   },
 };
+
+/**
+ * Turn a hand-off into a refusal when the policy forbids one.
+ *
+ * Exported and separate from `runBuildTest` so the conversion — the half that
+ * has been wrong twice, first as a dead precondition and then as a wrapper on
+ * two of the three routes — is reachable by a test without a live container
+ * runtime. What stays unpinned is only that `runBuildTest` calls it, which is
+ * one visible line rather than a branch hiding in a long function.
+ */
+/**
+ * Whether converting this report would destroy the run it was asked to
+ * continue.
+ *
+ * A predicate for the same reason `applyHandOffPolicy` is one: the conversion
+ * it guards returns a report, the handler writes whatever is returned, and a
+ * fresh refusal carries no run identity — so on a `--resume` it replaces the
+ * in-flight report and every later resume fails the identity check. The other
+ * two continuation exits enforce that invariant with a throw; this one was
+ * added after both and did not.
+ */
+export function resumeWouldDestroyReport(
+  report: BuildTestReport,
+  resume: boolean,
+  policy: SandboxPolicy = sandboxPolicy(),
+): boolean {
+  return resume && handOffRefused(report.toolchain, policy);
+}
+
+export function applyHandOffPolicy(
+  report: BuildTestReport,
+  policy: SandboxPolicy = sandboxPolicy(),
+): BuildTestReport {
+  return handOffRefused(report.toolchain, policy)
+    ? refusedReport(
+        `review.sandbox is "required" and no toolchain adapter could scope ` +
+          `this repository, so the only remaining route was to hand its ` +
+          `install, build and test commands to an agent shell this policy ` +
+          `cannot contain`,
+      )
+    : report;
+}
+
+export function runBuildTest(args: BuildTestArgs): BuildTestReport {
+  const report = runBuildTestUnguarded(args);
+  // The THIRD continuation exit, and the one the invariant had not reached.
+  // "A continuation must never answer with a FRESH report" is enforced by a
+  // throw at the refusal gate and at `!adapter`; this conversion was added
+  // after both and returns a report of its own, which the handler writes
+  // unconditionally — so a policy that tightened between the first call and
+  // the resume would replace the in-flight report with an identity-less
+  // refusal, and every later `--resume` would fail the identity check. That
+  // costs the round its whole build-test chain over a setting change.
+  //
+  // The trigger is ordinary: the policy is read per call, so an operator
+  // raising it — or a workflow's `env:` — between call one and the resume is
+  // enough, on the unscopeable repo shapes (yarn/pnpm/bun) that reach a
+  // hand-off in the first place.
+  if (resumeWouldDestroyReport(report, args.resume === true)) {
+    throw new Error(
+      `refusing to continue this run: this repository's toolchain cannot be ` +
+        `scoped, and review.sandbox is now "required", so continuing would ` +
+        `replace the report at ${args.out} with a refusal that records no run ` +
+        `identity — killing the resume chain. Re-run without --resume under ` +
+        `the new policy.`,
+    );
+  }
+  return applyHandOffPolicy(report);
+}

packages/cli/src/commands/review/lib/agent-briefs.ts

@@ -577,8 +577,9 @@ Read the JSON it prints:
 
 - \`toolchain: "npm"\` → use its \`build[]\` / \`test[]\` results. A failure in a file **the diff changed** is a **Critical** (\`Source: [build]\` or \`[test]\`); a failure in a file it did **not** touch is pre-existing — say so, do not file it against this PR. A non-empty \`timedOut\`, or a failed \`install\`, is environment/infrastructure — informational, never a Critical. On \`ok: true\`, name the workspaces built and the commands run; a return that names no command is a whiff. Report the TEST coverage from \`testScope\`, never from assumption. \`testScope.workspaces\` lists exactly the suites that ran — say "tests scoped to <list> — the changed workspaces and their declared dependents that define a test script". \`testScope.notRun\`, when present, names suites the whole-call budget stopped before they ran — say they did not run, never fold them into the coverage. When \`testScope.caveat\` is present, the scope may be incomplete — quote the caveat and say exactly that. A green run is a claim about those suites only — do not phrase it as the whole suite passing.
 - **A suite left unrun is not a suite that passed — continue the run.** \`testScope.notRun\` names suites the whole-call budget could not reach, and a \`test[]\` entry with \`"clamped": true\` is a suite the budget started too late and killed (its deadline was shortened, so its timeout says nothing about the suite). A third shape ends before any suite — a single-package repo whose budget ran out before its one suite has an empty \`test[]\`, no \`testScope\`, and \`"endedBeforeTests": true\` (the report's own stamp), with the \`note\` naming the unrun suite; read them before calling the dimension finished. That third shape cannot be continued — a continuation has no recorded scope to read, and a \`--resume\` on it answers "ended before its test phase" and points at a fresh run — so report the dimension UNFINISHED and do not spend a continuation on it. The first two mean the dimension is unfinished AND continuable: re-run the SAME \`build-test\` command with \`--resume\` — it skips install and build, runs only what is left, and merges into the same report file. The ${SHELL_TOOL_MAX_TIMEOUT_MS / 1000}-second ceiling is per CALL, so this is the only way a repo whose suites do not fit one call ever finishes them (measured on this repo: \`packages/cli\` alone needs 401s, and install + builds + \`packages/core\` had already spent 285s). Keep resuming while work is left, up to ${MAX_RESUME_CALLS} continuations; then report what the run has, with \`notRun\` disclosed.
-- **When any \`test[]\` command failed (exit non-zero, not a timeout), MEASURE which failures are the PR's before ruling by path.** The path rule above misclassifies in both directions — an environment-flaky test in a touched file gets filed as a Critical it did not cause, and a PR that breaks a test in an UNTOUCHED file gets waved through as pre-existing. The measurement is two commands: \`qwen review base-tree --plan <plan> --worktree <worktree> --out <plan dir>/qwen-review-pr-<n>-base-tree.json\` (builds the merge base beside the worktree). **Read \`available\` before using \`path\`** — a tree that was created but did NOT build populates \`path\` too, and a base that failed to build says nothing whatsoever about the PR, so measuring against it turns an infrastructure failure into a list of Criticals. \`available: false\` (local/lightweight review, no merge base, a base that would not compile) means the path rule stands — say so and stop here, and \`qwen review test-delta --report <the build-test report you wrote> --baseline <the base-tree report's path field> --pr-worktree <this worktree> --out <plan dir>/qwen-review-pr-<n>-test-delta.json\`. Read its verdict: a file in \`netNew\` fails on the PR side only — **that is the Critical**, whatever file the diff touches; a file in \`shared\` fails on base too — **pre-existing by measurement**, never filed, whatever file the diff touches; an \`unparsed\` entry, a timed-out base rerun, a base rerun that FAILED without naming any failing file (it did not measure the base — an unbuilt tree, a missing install, a workspace absent at base), or a command the whole-command budget could not fit attributes nothing — the report names each with its own reason; fall back to the path rule for those and say the delta could not rule. Compare failing FILE SETS, never counts: a flaky suite fails different test NAMES on two runs of the same tree, so counts are noise and the set difference is the signal.
+- **When any \`test[]\` command failed (exit non-zero, not a timeout), MEASURE which failures are the PR's before ruling by path.** The path rule above misclassifies in both directions — an environment-flaky test in a touched file gets filed as a Critical it did not cause, and a PR that breaks a test in an UNTOUCHED file gets waved through as pre-existing. The measurement is two commands: \`qwen review base-tree --plan <plan> --worktree <worktree> --out <plan dir>/qwen-review-pr-<n>-base-tree.json\` (builds the merge base beside the worktree). **Read \`available\` before using \`path\`** — a tree that was created but did NOT build populates \`path\` too, and a base that failed to build says nothing whatsoever about the PR, so measuring against it turns an infrastructure failure into a list of Criticals. \`available: false\` (local/lightweight review, no merge base, a base that would not compile) means the path rule stands — say so and stop here, and \`qwen review test-delta --report <the build-test report you wrote> --baseline <the base-tree report's path field> --pr-worktree <this worktree> --out <plan dir>/qwen-review-pr-<n>-test-delta.json\`. Read its verdict: a file in \`netNew\` fails on the PR side only — **that is the Critical**, whatever file the diff touches; a file in \`shared\` fails on base too — **pre-existing by measurement**, never filed, whatever file the diff touches; an \`unparsed\` entry, a timed-out base rerun, a base rerun that FAILED without naming any failing file (it did not measure the base — an unbuilt tree, a missing install, a workspace absent at base), or a command the whole-command budget could not fit attributes nothing — the report names each with its own reason; fall back to the path rule for those and say the delta could not rule. **An EMPTY \`entries\` is never itself a verdict** \u2014 when the report comes back with no entries at all, its \`note\` is the whole result, and several of the things it can say there (the base tree was never built, the build-test report was unreadable, containment was \`required\` and unavailable so the base-side rerun did not happen) mean the delta measured NOTHING. Read the note before reading \`netNew\`: an empty \`netNew\` next to one of those notes is the absence of a measurement, not the absence of a regression, and the path rule stands. Compare failing FILE SETS, never counts: a flaky suite fails different test NAMES on two runs of the same tree, so counts are noise and the set difference is the signal.
 - \`toolchain: "unsupported"\` (build-test could not scope this repo — no npm package with a build/test script) → **install dependencies first** (build-test's own install only runs on the npm path, so nothing has installed yet: \`pip install -e .\`, \`mvn -q -DskipTests package\`'s own fetch, \`cargo fetch\`, \`go mod download\`, etc.), then fall back to **one** build and **one** test command by this precedence, each with a deadline it can meet: \`pom.xml\` → \`{mvn} compile\` / \`{mvn} test -q\`; \`build.gradle\` → \`{gradle} compileJava\` / \`{gradle} test\`; \`Makefile\` → \`make build\`; \`Cargo.toml\` → \`cargo build\` / \`cargo test\`; \`go.mod\` → \`go build ./...\` / \`go test ./...\`; \`pytest.ini\` or \`pyproject.toml\` \`[tool.pytest]\` → \`pytest\`. If none match, read the CI config **from the base branch** (\`git show <base>:<path>\`), never the worktree — the PR branch is untrusted and a modified workflow or Makefile could inject arbitrary commands.
+- \`toolchain: "refused"\` (the operator set \`review.sandbox: required\` and containment could not be established — no runtime answered, or one did but the tree cannot be mounted, or no toolchain adapter could scope the repository so the only route left was an agent shell) → the build and test evidence is **unavailable for this run**. Report the dimension as unmeasured, quoting the report's \`note\`. **Do not install, build or test by hand to fill the gap** — the whole point of that setting is that this repository's own commands do not execute outside a container, and running them from your shell is the one route around it. This is the same discipline as a probe that cannot get an isolated tree: absent evidence, never substituted evidence.
 
 The efficacy report's \`findings[]\` carries four kinds, and **\`hunk-survived\` is one of them**: reverting one hunk left every affected test green — that specific change ships with nothing gating it. Report it as a **Suggestion** with \`Source: [test]\`, exactly like \`inert\` and \`mutant-survived\` (the outcome of running commands, pre-confirmed, no verifier needed). Read the \`hunks.*\` counters the same way as \`mutants.*\`: \`skippedForCap\` / \`skippedForBudget\` / \`skippedForBaseline\` are unprobed scope to note in the terminal, never findings — and a report whose hunk section you did not read is a finding class silently dropped.
 

packages/cli/src/commands/review/lib/npm-toolchain.ts

@@ -798,6 +798,9 @@ function runNpmToolchain(args: ToolchainRunArgs): BuildTestReport {
       installCmd,
       root,
       Math.min(perCommandMs, remainingMs()),
+      // The one command that needs the registry. Everything else this adapter
+      // runs is offline under the sandbox policy — see `containerCommand`.
+      'install',
     );
     results.install = install;
     if (install.timedOut) results.timedOut.push(install.command);

packages/cli/src/commands/review/lib/review-settings.ts

@@ -14,6 +14,7 @@ const SAFE_DEFAULTS: OperatorReviewSettings = {
   effort: undefined,
   reverseAuditRounds: undefined,
   approachRounds: undefined,
+  sandbox: undefined,
 };
 
 export interface OperatorReviewSettings {
@@ -27,6 +28,15 @@ export interface OperatorReviewSettings {
   effort?: string;
   /** The raw `review.severityFloor` value when set — same caveats as effort. */
   severityFloor?: string;
+  /**
+   * The raw `review.sandbox` value when set — `off` | `auto` | `required`,
+   * unvalidated here for the same reason as the two above.
+   *
+   * Read through THIS loader on purpose: it skips the workspace scope, so a
+   * repository cannot ship a `.qwen/settings.json` that switches off the
+   * containment which exists to contain that repository's own code.
+   */
+  sandbox?: string;
   /**
    * The operator's reverse-audit round ceiling, when they set a real one.
    *
@@ -108,6 +118,7 @@ export function operatorReviewSettings(): OperatorReviewSettings {
       typeof review?.severityFloor === 'string'
         ? review.severityFloor
         : undefined,
+    sandbox: typeof review?.sandbox === 'string' ? review.sandbox : undefined,
     reverseAuditRounds:
       typeof rounds === 'number' && Number.isInteger(rounds) && rounds > 0
         ? rounds

packages/cli/src/commands/review/lib/sandboxed-exec.test.ts

@@ -0,0 +1,922 @@
+/**
+ * @license
+ * Copyright 2026 Qwen Team
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+// The reviewed repository's own commands, and the boundary they run behind
+// (#9556). What these pin is not that a container starts — that needs a
+// runtime and belongs to an integration harness — but the three decisions the
+// argv encodes: what is mounted, what crosses in the environment, and what
+// happens when there is no runtime at all.
+
+import { describe, it, expect, afterEach, vi } from 'vitest';
+import { join, sep } from 'node:path';
+import {
+  existsSync,
+  mkdirSync,
+  writeFileSync,
+  mkdtempSync,
+  realpathSync,
+  rmSync,
+  symlinkSync,
+} from 'node:fs';
+import { tmpdir } from 'node:os';
+import type { spawnSync } from 'node:child_process';
+import { isolateOperatorReviewSettings } from './test-utils.js';
+import * as environment from '../../../config/environment.js';
+import {
+  containerCommand,
+  firstAnsweringRuntime,
+  killContainer,
+  containerPathFor,
+  hasRootlessMarker,
+  readInfoDocument,
+  runtimeIsRootless,
+  boxedRunLeftContainer,
+  CONTAINER_HOME,
+  containerEnv,
+  containerName,
+  handOffRefused,
+  mountRootFor,
+  refuseUnsandboxedPhase,
+  reviewSandboxImage,
+  runtimeClientEnv,
+  sandboxPolicy,
+  sandboxVerdict,
+} from './sandboxed-exec.js';
+
+describe('sandboxPolicy', () => {
+  it('lets the environment outrank the setting, and defaults to off', () => {
+    // CI has to be able to require containment without depending on a
+    // settings file the runner may not carry.
+    expect(sandboxPolicy({ QWEN_REVIEW_SANDBOX: 'required' }, {})).toBe(
+      'required',
+    );
+    expect(
+      sandboxPolicy({ QWEN_REVIEW_SANDBOX: 'auto' }, { sandbox: 'off' }),
+    ).toBe('auto');
+    expect(sandboxPolicy({}, { sandbox: 'required' })).toBe('required');
+    // Today every review runs the reviewed code directly; turning that into a
+    // container by default would change what native modules compile against
+    // on machines nobody asked.
+    expect(sandboxPolicy({}, {})).toBe('off');
+    // A garbled value is not a policy — it falls through rather than being
+    // guessed at.
+    expect(sandboxPolicy({ QWEN_REVIEW_SANDBOX: 'yes' }, {})).toBe('off');
+  });
+});
+
+describe('values a repository must not be able to set', () => {
+  // `loadEnvironment` walks up from cwd and applies `<repo>/.qwen/.env` —
+  // repository content, from the checkout under review, admitted by default
+  // because folder trust starts off. So `process.env` is NOT the operator's
+  // alone, and every containment decision read from it needs to know which
+  // half it came from. The policy is additionally protected by only ever
+  // tightening; these three have no such ordering to fall back on.
+
+  it('ignores a repo-shipped image override — the image IS the code', () => {
+    vi.stubEnv('QWEN_REVIEW_SANDBOX_IMAGE', 'attacker.example/rogue:1');
+    const spy = vi
+      .spyOn(environment, 'isFileSourcedEnvKey')
+      .mockImplementation((k) => k === 'QWEN_REVIEW_SANDBOX_IMAGE');
+    try {
+      expect(reviewSandboxImage()).not.toContain('attacker.example');
+    } finally {
+      spy.mockRestore();
+      vi.unstubAllEnvs();
+    }
+  });
+
+  // Skipped, not returned early: an early return reports PASSED with zero
+  // assertions, which reads on the Windows lane as "this held" when nothing
+  // was checked.
+  it.skipIf(process.getuid === undefined)(
+    'ignores a repo-shipped SANDBOX_SET_UID_GID=false',
+    () => {
+      // Left honoured, a committed opt-out puts the container back to root and
+      // leaves root-owned residue the host pipeline cannot sweep.
+      vi.stubEnv('SANDBOX_SET_UID_GID', 'false');
+      const spy = vi
+        .spyOn(environment, 'isFileSourcedEnvKey')
+        .mockImplementation((k) => k === 'SANDBOX_SET_UID_GID');
+      try {
+        const { args } = containerCommand('npm ci', {
+          cwd: join(sep, 'repo', '.qwen', 'tmp', 'review-pr-9'),
+          tmpDir: join(sep, 'repo', '.qwen', 'tmp'),
+          kind: 'install',
+          runtime: 'docker',
+          image: 'example/image:tag',
+          rootless: false,
+          name: 'qwen-review-test',
+        });
+        expect(args).toContain('--user');
+      } finally {
+        spy.mockRestore();
+        vi.unstubAllEnvs();
+      }
+    },
+  );
+
+  it('drops a repo-shipped DOCKER_HOST from the runtime client env', () => {
+    // It decides WHICH daemon answers: a repository that ships one points the
+    // availability probe and every `docker run` at a daemon it controls, so
+    // `required` reads as satisfied and whatever that daemon returns is scored
+    // as build, test and probe evidence.
+    // By PROVENANCE, not by name. The list was the first design and it lost
+    // twice — it named the daemon selectors and missed the proxy family, then
+    // named those and missed `DOCKER_API_VERSION`, which selects nothing and
+    // merely makes every call fail, turning `auto` containment off because the
+    // probe reads a broken client as "no runtime". So the fixture includes a
+    // key nobody would think to enumerate, and expects it gone too.
+    const selectors = [
+      'DOCKER_API_VERSION',
+      'PATH',
+      'SOMETHING_NOBODY_ENUMERATED',
+      'HTTP_PROXY',
+      'HTTPS_PROXY',
+      'ALL_PROXY',
+      'NO_PROXY',
+      'DOCKER_HOST',
+      'DOCKER_CERT_PATH',
+      'DOCKER_TLS_VERIFY',
+      'DOCKER_CONTEXT',
+      'CONTAINER_HOST',
+      'DOCKER_CONFIG',
+      'CONTAINERS_CONF',
+      'CONTAINERS_REGISTRIES_CONF',
+      'CONTAINERS_STORAGE_CONF',
+    ];
+    for (const key of selectors) vi.stubEnv(key, 'from-the-repo');
+    const spy = vi
+      .spyOn(environment, 'isFileSourcedEnvKey')
+      .mockImplementation((k) => selectors.includes(k));
+    try {
+      const scrubbed = runtimeClientEnv();
+      for (const key of selectors) expect(scrubbed[key]).toBeUndefined();
+    } finally {
+      spy.mockRestore();
+      vi.unstubAllEnvs();
+    }
+    // An OPERATOR's own DOCKER_HOST — a remote engine, colima, rootless — is
+    // untouched; only the file-sourced one is dropped. Without this half the
+    // scrub could be "delete everything" and still ship green.
+    vi.stubEnv('DOCKER_HOST', 'unix:///run/user/1000/docker.sock');
+    try {
+      expect(runtimeClientEnv()['DOCKER_HOST']).toBe(
+        'unix:///run/user/1000/docker.sock',
+      );
+    } finally {
+      vi.unstubAllEnvs();
+    }
+  });
+});
+
+describe('the operator opt-out still works', () => {
+  it.skipIf(process.getuid === undefined)(
+    'honours SANDBOX_SET_UID_GID=false when it is the operator’s own',
+    () => {
+      // Both uid tests assert `--user` is PRESENT; without this one the
+      // documented opt-out could stop working and nothing would say so.
+      vi.stubEnv('SANDBOX_SET_UID_GID', 'false');
+      try {
+        const { args } = containerCommand('npm ci', {
+          cwd: join(sep, 'repo', '.qwen', 'tmp', 'review-pr-9'),
+          tmpDir: join(sep, 'repo', '.qwen', 'tmp'),
+          kind: 'install',
+          runtime: 'docker',
+          image: 'example/image:tag',
+          rootless: false,
+          name: 'qwen-review-test',
+        });
+        expect(args).not.toContain('--user');
+      } finally {
+        vi.unstubAllEnvs();
+      }
+    },
+  );
+});
+
+describe('sandboxVerdict', () => {
+  it('does not let an already-sandboxed session satisfy `required`', () => {
+    // The first cut returned `direct` here, reasoning that the outer boundary
+    // is the one the operator asked for. That is wrong for the property this
+    // module is about: the CLI's own sandbox constrains the filesystem and the
+    // network and hands the child `process.env` ENTIRE — and stripping the
+    // secrets is half of what `required` promises. So `SANDBOX` is not a
+    // shortcut past the policy; the runtime probe still decides.
+    const got = sandboxVerdict(
+      'required',
+      { SANDBOX: 'qwen-code-abc123' },
+      () => null,
+    );
+    expect(got.kind).toBe('refused');
+    expect(got.kind === 'refused' && got.reason).toContain(
+      'does not satisfy "required"',
+    );
+  });
+
+  it('turns `required` with no runtime into a refusal a phase can act on', () => {
+    // The refusal has to reach something that stops the phase. Left to "the
+    // caller", no caller acted and `required` ran the reviewed code
+    // unsandboxed with the full environment — the policy meant nothing.
+    const tree = join(sep, 'repo', '.qwen', 'tmp', 'review-pr-9');
+    const mounted = () => tree;
+    const verdict = sandboxVerdict('required', {}, () => null);
+    expect(
+      refuseUnsandboxedPhase(tree, verdict, mounted, 'required'),
+    ).toContain('no container runtime');
+    // ...and a verdict that is not a refusal never stops one.
+    expect(
+      refuseUnsandboxedPhase(
+        tree,
+        sandboxVerdict('off', {}, () => null),
+        mounted,
+      ),
+    ).toBe(null);
+    expect(
+      refuseUnsandboxedPhase(
+        tree,
+        sandboxVerdict('auto', {}, () => 'docker'),
+        mounted,
+        'auto',
+      ),
+    ).toBe(null);
+  });
+
+  it('refuses a phase a healthy runtime still cannot contain', () => {
+    // "A runtime answered" is not containment. A `/review` of a local checkout
+    // has no `.qwen/tmp` layout to mount, so the command falls through to the
+    // direct spawn — with the full environment, and a report indistinguishable
+    // from a contained run. The policy's question is whether THIS phase can be
+    // contained, and the mount is the half that fails while the daemon is fine.
+    const contained = sandboxVerdict('required', {}, () => 'docker');
+    expect(contained.kind).toBe('container');
+    const local = join(sep, 'home', 'me', 'myrepo');
+    expect(
+      refuseUnsandboxedPhase(local, contained, () => null, 'required'),
+    ).toContain('cannot be mounted');
+
+    // ...and ONLY under `required`. Under `auto` the contract is "contain it
+    // when that is possible", so an unmountable tree falls back to the direct
+    // spawn — refusing there would take the build/test and efficacy evidence
+    // away from every local review the moment a daemon happened to be running.
+    expect(refuseUnsandboxedPhase(local, contained, () => null, 'auto')).toBe(
+      null,
+    );
+  });
+
+  it('discloses rather than hides that the reviewed code ran as you', () => {
+    const got = sandboxVerdict('off', {}, () => null);
+    expect(got.kind).toBe('direct');
+    expect(got.kind === 'direct' && got.disclose).toContain('ran as you');
+  });
+});
+
+describe('the decisions this module exists to make', () => {
+  // Cells the surrounding suite reached only by accident of the machine it ran
+  // on. Each is a pure function with its ambient dependency already
+  // injectable, so pinning them costs nothing and leaves no mutant alive on
+  // the properties this feature is sold on.
+
+  // Swept, like the `mountRootFor` block's: a suite about not leaving residue
+  // behind has no business leaving a temp tree per run.
+  const made: string[] = [];
+  const fixture = (prefix: string): string => {
+    const dir = realpathSync(mkdtempSync(join(tmpdir(), prefix)));
+    made.push(dir);
+    return dir;
+  };
+  afterEach(() => {
+    for (const dir of made.splice(0))
+      rmSync(dir, { recursive: true, force: true });
+  });
+
+  it('lets the environment tighten the policy and never loosen it', () => {
+    // "A repository cannot switch off the containment that exists to contain
+    // it" is the whole claim. It rests on two halves, and both are asserted
+    // here rather than described: strictest-wins in BOTH directions, and a
+    // value the loader wrote from a file counting for nothing.
+    expect(sandboxPolicy({ QWEN_REVIEW_SANDBOX: 'required' }, {})).toBe(
+      'required',
+    );
+    // env stricter than settings → env
+    expect(
+      sandboxPolicy({ QWEN_REVIEW_SANDBOX: 'required' }, { sandbox: 'auto' }),
+    ).toBe('required');
+    // settings stricter than env → settings. The direction that matters: an
+    // operator's opt-in cannot be undone by an environment variable.
+    expect(
+      sandboxPolicy({ QWEN_REVIEW_SANDBOX: 'off' }, { sandbox: 'required' }),
+    ).toBe('required');
+    // ...and a FILE-SOURCED env value is not read at all, so a repository
+    // shipping `QWEN_REVIEW_SANDBOX` in its own `.env` cannot even tighten,
+    // let alone loosen.
+    expect(
+      sandboxPolicy(
+        { QWEN_REVIEW_SANDBOX: 'off' },
+        { sandbox: 'required' },
+        () => true,
+      ),
+    ).toBe('required');
+    expect(
+      sandboxPolicy(
+        { QWEN_REVIEW_SANDBOX: 'required' },
+        { sandbox: 'off' },
+        () => true,
+      ),
+    ).toBe('off');
+  });
+
+  it('reads either side the way an operator would write it', () => {
+    // The env half was normalised and the settings half was not, and the
+    // asymmetry fell the wrong way: settings.json is the documented place to
+    // turn this ON, so `"Required"` — or a stray trailing space — matched no
+    // policy, resolved to `off`, and disabled the control without a word.
+    // Fail-open on the one setting whose entire purpose is to fail closed.
+    expect(sandboxPolicy({}, { sandbox: 'Required' })).toBe('required');
+    expect(sandboxPolicy({}, { sandbox: 'required ' })).toBe('required');
+    expect(sandboxPolicy({ QWEN_REVIEW_SANDBOX: 'Required' }, {})).toBe(
+      'required',
+    );
+    expect(sandboxPolicy({ QWEN_REVIEW_SANDBOX: ' required ' }, {})).toBe(
+      'required',
+    );
+    // Every policy, not just the strict one: normalisation keyed on the value
+    // it was reported against would leave an operator's `"Auto"` resolving to
+    // `off` — the same silent downgrade one rung lower.
+    expect(sandboxPolicy({}, { sandbox: 'Auto' })).toBe('auto');
+    expect(sandboxPolicy({ QWEN_REVIEW_SANDBOX: 'Auto' }, {})).toBe('auto');
+    expect(sandboxPolicy({}, { sandbox: 'OFF' })).toBe('off');
+
+    // A value that is not a policy at all still resolves to `off` — the
+    // normalisation widens spelling, not the set of accepted values.
+    expect(sandboxPolicy({}, { sandbox: 'requiredish' })).toBe('off');
+    // ...and an unreadable ENVIRONMENT value is dropped on its own rather than
+    // taking the operator's setting down with it. This is the cell that
+    // matters: the environment is the half a repository can reach, so garbage
+    // there must never be able to answer for the half it cannot.
+    expect(
+      sandboxPolicy(
+        { QWEN_REVIEW_SANDBOX: 'yes-please' },
+        { sandbox: 'required' },
+      ),
+    ).toBe('required');
+  });
+
+  it('reads the process environment when no env is passed', () => {
+    // The other production default, and the twin of the settings one below:
+    // every assertion in this block hands `sandboxPolicy` an env literal, so
+    // `env = {}` as the default stops the environment half from being read at
+    // all and nothing here notices.
+    vi.stubEnv('QWEN_REVIEW_SANDBOX', 'required');
+    try {
+      expect(sandboxPolicy(undefined, {})).toBe('required');
+    } finally {
+      vi.unstubAllEnvs();
+    }
+  });
+
+  it('reads the operator settings file when no settings are passed', () => {
+    // The production shape: every real caller takes the default. With the
+    // default replaced by `{}` the whole settings half stops being consulted
+    // and every other assertion here — which passes settings explicitly —
+    // stays green.
+    const isolation = isolateOperatorReviewSettings();
+    try {
+      writeFileSync(
+        join(isolation.home, 'settings.json'),
+        JSON.stringify({ review: { sandbox: 'required' } }),
+      );
+      expect(sandboxPolicy({})).toBe('required');
+    } finally {
+      isolation.dispose();
+    }
+  });
+
+  it('falls back to running directly under `auto` when nothing answers', () => {
+    // The cell that makes `auto` usable on a machine without a runtime — and
+    // the one a mutant turning it into a refusal would sail through, because
+    // every other test either has a runtime or is not `auto`.
+    const verdict = sandboxVerdict('auto', {}, () => null);
+    expect(verdict.kind).toBe('direct');
+    // `required` with the same absent runtime is the opposite answer, so this
+    // is about the policy and not about the probe.
+    expect(sandboxVerdict('required', {}, () => null).kind).toBe('refused');
+  });
+
+  it('passes the phase when `required` is actually satisfiable', () => {
+    // Every other assertion about this gate is a refusal. Without the pass
+    // path, a mutant refusing unconditionally under `required` — which would
+    // make the feature refuse every review on a perfectly good host — leaves
+    // the suite green.
+    const verdict = { kind: 'container', runtime: 'docker' } as const;
+    expect(
+      refuseUnsandboxedPhase(
+        join(sep, 'repo', '.qwen', 'tmp', 'review-pr-9'),
+        verdict,
+        () => join(sep, 'repo', '.qwen', 'tmp'),
+        'required',
+      ),
+    ).toBeNull();
+    // ...and the same satisfiable verdict against a root that cannot be
+    // mounted still refuses, so the null above is the pass and not a hole.
+    expect(
+      refuseUnsandboxedPhase(
+        join(sep, 'elsewhere', 'review-pr-9'),
+        verdict,
+        () => null,
+        'required',
+      ),
+    ).toContain('cannot be mounted');
+  });
+
+  it('takes the first runtime whose daemon answers, in order', () => {
+    // Order is the content: a client installed but not running must never
+    // shadow one that is. With the loop reversed or short-circuited on the
+    // first NAME rather than the first ANSWER, `auto` silently picks a runtime
+    // that cannot run anything and the phase degrades to direct.
+    expect(firstAnsweringRuntime(() => true)).toBe('docker');
+    expect(firstAnsweringRuntime((rt) => rt === 'podman')).toBe('podman');
+    expect(firstAnsweringRuntime(() => false)).toBeNull();
+  });
+
+  it('reaps by name, with the flag that makes it a kill', () => {
+    // The reap runs after a deadline already cost the phase its result, so
+    // nothing downstream would notice a garbled argv — and what survives is a
+    // container holding the review tree open past the end of the run. `-f`,
+    // because a container that ignored the client's signal is exactly the one
+    // this is for.
+    const calls: Array<[string, readonly string[]]> = [];
+    const spawn = ((file: string, args: readonly string[]) => {
+      calls.push([file, args]);
+      return { status: 0 } as ReturnType<typeof spawnSync>;
+    }) as unknown as typeof spawnSync;
+    killContainer('podman', 'qwen-review-1-abc-0', spawn);
+    expect(calls).toEqual([['podman', ['rm', '-f', 'qwen-review-1-abc-0']]]);
+
+    // Best-effort by construction: a reap that throws must not become a second
+    // failure on top of the timeout that is already being reported.
+    const throwing = (() => {
+      throw new Error('no daemon');
+    }) as unknown as typeof spawnSync;
+    expect(() =>
+      killContainer('docker', 'qwen-review-1-abc-1', throwing),
+    ).not.toThrow();
+  });
+
+  it('spells the workdir canonically, and survives a tree not built yet', () => {
+    // This feeds `--workdir` at both spawn sites: a lexical spelling names a
+    // directory the container does not have, and every command fails before
+    // it starts.
+    const root = fixture('qwen-workdir-');
+    const real = join(root, 'review-pr-9');
+    mkdirSync(real);
+    expect(containerPathFor(real)).toBe(realpathSync(real));
+
+    // A path reached through a link resolves to the canonical spelling the
+    // mount actually carries.
+    const link = join(root, 'link-to-tree');
+    symlinkSync(real, link);
+    expect(containerPathFor(link)).toBe(realpathSync(real));
+
+    // The probe tree is NAMED before it is created; its parent exists, and
+    // the fallback canonicalises that and re-attaches the leaf.
+    const unborn = join(link, 'not-created-yet');
+    expect(containerPathFor(unborn)).toBe(
+      join(realpathSync(real), 'not-created-yet'),
+    );
+  });
+});
+
+describe('containerCommand', () => {
+  const tmpDir = join(sep, 'repo', '.qwen', 'tmp');
+  const base = {
+    tmpDir,
+    runtime: 'docker' as const,
+    image: 'example/image:tag',
+    name: 'qwen-review-test',
+    rootless: false,
+  };
+
+  // Skipped rather than returned early, for the same reason as the sibling
+  // above: an early return reports PASSED with nothing asserted.
+  it.skipIf(process.getuid === undefined || process.getgid === undefined)(
+    'drops --user on a rootless runtime and keeps it on a rootful one',
+    () => {
+      // Rootless engines map the container's uid onto a host SUBUID, so naming
+      // the host uid here hands the container process an identity that owns
+      // nothing in the tree it is mounted on: `npm ci` cannot create
+      // `node_modules`, and whatever it does create comes out unsweepable. The
+      // container's root already IS the invoking user there, so the flag's job
+      // is done without it. On a rootful engine it is still the only thing
+      // between the reviewed code and real uid 0 on the mount.
+      //
+      // The documented opt-out is a real thing an operator exports, and it
+      // removes the very flag this asserts — without pinning it off, this test
+      // reports a failure of the code in a shell where the code is correct.
+      // Restored in a `finally`, so a failing assertion below fails THIS test
+      // instead of leaking the stub into whichever test runs next.
+      vi.stubEnv('SANDBOX_SET_UID_GID', '');
+      try {
+        const cwd = join(tmpDir, 'review-pr-9');
+        const rootful = containerCommand('npm ci', {
+          ...base,
+          cwd,
+          kind: 'install',
+        });
+        expect(rootful.args).toContain('--user');
+        // Optional-called because the skipIf guard above cannot narrow these
+        // for the compiler; the test does not run where they are undefined.
+        expect(rootful.args[rootful.args.indexOf('--user') + 1]).toBe(
+          `${process.getuid?.()}:${process.getgid?.()}`,
+        );
+
+        const rootless = containerCommand('npm ci', {
+          ...base,
+          cwd,
+          kind: 'install',
+          rootless: true,
+        });
+        expect(rootless.args).not.toContain('--user');
+        // Everything else is the SAME run — dropping --user must not quietly take
+        // the mount, the tmpfs HOME or the image with it.
+        expect(rootless.args).toContain('--volume');
+        expect(rootless.args).toContain('--tmpfs');
+        expect(rootless.args.at(-4)).toBe(base.image);
+
+        // The opt-out is read case- and space-insensitively, so an operator
+        // who exports `False` gets the documented behaviour rather than a
+        // flag they thought they had turned off.
+        vi.stubEnv('SANDBOX_SET_UID_GID', ' False ');
+        expect(
+          containerCommand('npm ci', { ...base, cwd, kind: 'install' }).args,
+        ).not.toContain('--user');
+      } finally {
+        vi.unstubAllEnvs();
+      }
+    },
+  );
+
+  it('answers rootful when the runtime will not say', () => {
+    // The unknown case must land on the LOUD side: keeping `--user` breaks a
+    // rootless run visibly, dropping it on a rootful engine runs the reviewed
+    // code as real uid 0 on a writable mount and says nothing. An empty
+    // document is how "could not tell" reaches the predicate.
+    expect(runtimeIsRootless('podman', () => '')).toBe(false);
+    expect(
+      runtimeIsRootless(
+        'podman',
+        () => '{"host":{"security":{"rootless":true}}}',
+      ),
+    ).toBe(true);
+    // ...and a runtime that is not on this machine produces exactly that empty
+    // document rather than throwing out of the argv builder.
+    expect(readInfoDocument('qwen-no-such-runtime' as never)).toBe('');
+  });
+
+  it("reads rootlessness out of either runtime's info document", () => {
+    // The negative case is a LIVE rootful docker's actual `info` output
+    // (docker 29.5.2), not a hand-written stub: the marker search only holds
+    // if the word genuinely does not occur in a rootful document, and a stub
+    // written by the same hand that wrote the matcher cannot show that.
+    expect(
+      hasRootlessMarker(
+        '{"SecurityOptions":["name=apparmor","name=seccomp,profile=builtin","name=cgroupns"],"ServerVersion":"29.5.2","OperatingSystem":"Ubuntu 24.04.4 LTS"}',
+      ),
+    ).toBe(false);
+    // docker spells it as a security option...
+    expect(
+      hasRootlessMarker(
+        '{"SecurityOptions":["name=seccomp,profile=builtin","name=rootless","name=cgroupns"]}',
+      ),
+    ).toBe(true);
+    // ...podman as a field under Host.Security, which is why this searches the
+    // document rather than one runtime's schema path.
+    expect(
+      hasRootlessMarker(
+        '{"host":{"security":{"rootless":true,"seccompEnabled":true}}}',
+      ),
+    ).toBe(true);
+    expect(hasRootlessMarker('{"host":{"security":{"rootless":false}}}')).toBe(
+      false,
+    );
+    // Go marshals an exported field under its own name unless a tag renames
+    // it, so the capitalised spelling is a real shape, not a defensive guess.
+    expect(hasRootlessMarker('{"Host":{"Security":{"Rootless":true}}}')).toBe(
+      true,
+    );
+  });
+
+  it('mounts the review temp dir, not the tree the command runs in', () => {
+    // The dependency farm links OUT of every tree: each package in the probe
+    // tree's `node_modules` points at the review worktree's copy (1 722 of
+    // them on a live CI review). Mounting the probe tree alone would leave
+    // every one of those dangling and no probe would resolve a dependency.
+    const probeTree = join(tmpDir, 'review-pr-9-probe');
+    const { file, args } = containerCommand('npm test', {
+      ...base,
+      cwd: probeTree,
+      kind: 'test',
+    });
+
+    expect(file).toBe('docker');
+    const mount = args[args.indexOf('--volume') + 1];
+    expect(mount).toBe(`${tmpDir}:${tmpDir}`);
+    expect(mount).not.toContain('-probe');
+    // ...and the command still RUNS in the tree.
+    expect(args[args.indexOf('--workdir') + 1]).toBe(probeTree);
+    // `<repo>/.git` — the filter/fsmonitor/replace surface — is outside it.
+    expect(mount.startsWith(join(sep, 'repo', '.git'))).toBe(false);
+  });
+
+  it('gives the network to an install and to nothing else', () => {
+    const cwd = join(tmpDir, 'review-pr-9');
+    const install = containerCommand('npm ci', {
+      ...base,
+      cwd,
+      kind: 'install',
+    });
+    // Both argv shapes: the separated form this builds, and the joined
+    // `--network=none` a refactor could switch to, which `not.toContain('none')`
+    // alone would not see.
+    expect(install.args.some((a) => a.startsWith('--network'))).toBe(false);
+    expect(install.args).not.toContain('none');
+
+    for (const kind of ['build', 'test'] as const) {
+      const r = containerCommand('npm run build', { ...base, cwd, kind });
+      expect(r.args[r.args.indexOf('--network') + 1]).toBe('none');
+    }
+  });
+
+  it('hands the reviewed code an env allowlist, never the inherited one', () => {
+    // This is the finding the design is built on: both call sites used to give
+    // the PR's code `process.env`, which on CI carries the review's model and
+    // GitHub credentials. A `postinstall` reading them is one line.
+    const env = containerEnv('/cache-in-mount');
+    expect(env).toEqual([
+      'CI=1',
+      'npm_config_yes=true',
+      'QWEN_SKIP_PREPARE=1',
+      // HOME is a tmpfs path, NOT under the mount: `sh -lc` sources
+      // `$HOME/.profile` and npm reads `$HOME/.npmrc`, so a HOME on the shared
+      // mount lets one run plant what the next one executes — across `--rm`
+      // and across reviews.
+      `HOME=${CONTAINER_HOME}`,
+      'npm_config_cache=/cache-in-mount',
+    ]);
+
+    // A PLANTED canary, not whatever the runner's shell happens to export:
+    // the regression this guards against is `containerCommand` forwarding the
+    // inherited environment, and on a box exporting no TOKEN/KEY/SECRET the
+    // old form shipped green.
+    vi.stubEnv('OPENAI_API_KEY', 'canary-should-not-cross');
+    vi.stubEnv('GH_TOKEN', 'canary-should-not-cross');
+    try {
+      const { args } = containerCommand('npm ci', {
+        ...base,
+        cwd: join(tmpDir, 'review-pr-9'),
+        kind: 'install',
+      });
+      const passed = args.filter((_, i) => args[i - 1] === '--env');
+      expect(passed.some((e) => e.includes('canary-should-not-cross'))).toBe(
+        false,
+      );
+      // ...and the WIRING, not just `containerEnv` called with a literal: HOME
+      // must be the tmpfs the argv also declares, or the mapped uid has no
+      // writable home and npm fails before the install starts.
+      expect(passed).toContain(`HOME=${CONTAINER_HOME}`);
+      const tmpfs = args[args.indexOf('--tmpfs') + 1];
+      expect(tmpfs.startsWith(`${CONTAINER_HOME}:`)).toBe(true);
+    } finally {
+      vi.unstubAllEnvs();
+    }
+  });
+
+  // Windows has no `process.getuid`, so the flag is correctly absent there —
+  // and the merge queue runs this file on Windows.
+  it.skipIf(process.getuid === undefined)(
+    'maps the host uid so its writes stay removable from the host',
+    () => {
+      // The container writes into a mount the HOST then cleans up — `node_modules`
+      // after an install timeout, the tree at `discardWorktree`, the sweeps. Root
+      // in the container makes every one of those EACCES, and the residue
+      // accumulates across reviews: the cross-run-state class #9221 closed.
+      // The documented opt-out (`SANDBOX_SET_UID_GID=false`) is something a
+      // developer's own shell may carry, which would fail this spuriously; it
+      // is stubbed rather than assumed.
+      vi.stubEnv('SANDBOX_SET_UID_GID', 'true');
+      try {
+        const { args } = containerCommand('npm ci', {
+          ...base,
+          cwd: join(tmpDir, 'review-pr-9'),
+          kind: 'install',
+        });
+        const user = args[args.indexOf('--user') + 1];
+        expect(user).toBe(`${process.getuid?.()}:${process.getgid?.()}`);
+      } finally {
+        vi.unstubAllEnvs();
+      }
+    },
+  );
+
+  it('names the container so a deadline can reach it', () => {
+    // `--rm` fires only when the container exits on its own, and a `spawnSync`
+    // timeout kills the runtime CLIENT: measured on docker 29.1.3, an attached
+    // client forwards the signal and waits, so a workload whose own trap
+    // ignores it keeps running with this mount writable — past the budget and
+    // past the end of the review. Without a name there is nothing to aim at.
+    const { args } = containerCommand('npm test', {
+      ...base,
+      cwd: join(tmpDir, 'review-pr-9-probe'),
+      kind: 'test',
+    });
+    expect(args[args.indexOf('--name') + 1]).toBe(base.name);
+    // ...and two calls never collide, or one run's cleanup would reach
+    // another's container.
+    expect(containerName()).not.toBe(containerName());
+  });
+
+  it('runs one ephemeral container per command', () => {
+    // Not one long-lived container per phase: that would be cheaper by about
+    // a percent of the efficacy budget and would re-introduce the cross-run
+    // state this pipeline spent rounds closing.
+    const { args } = containerCommand('npm test', {
+      ...base,
+      cwd: join(tmpDir, 'review-pr-9-probe'),
+      kind: 'test',
+    });
+    expect(args[0]).toBe('run');
+    expect(args).toContain('--rm');
+  });
+
+  it('passes the command to a shell inside, not to the host', () => {
+    const { args } = containerCommand('npm ci && npm test', {
+      ...base,
+      cwd: join(tmpDir, 'review-pr-9'),
+      kind: 'install',
+    });
+    expect(args.slice(-3)).toEqual(['sh', '-lc', 'npm ci && npm test']);
+  });
+});
+
+describe('handOffRefused', () => {
+  it('turns the agent-shell hand-off into a refusal under `required`', () => {
+    // `unsupportedReport` tells the agent to install and build with its own
+    // shell — contained by nothing here — and the phase gate cannot catch it,
+    // because that gate passes exactly when a runtime answered and the tree is
+    // mountable, which is when a repo the adapters cannot scope still reaches
+    // the hand-off.
+    expect(handOffRefused('unsupported', 'required')).toBe(true);
+    // Under the other policies a hand-off is what it has always been.
+    expect(handOffRefused('unsupported', 'auto')).toBe(false);
+    expect(handOffRefused('unsupported', 'off')).toBe(false);
+    // ...and a real run is never converted.
+    expect(handOffRefused('npm', 'required')).toBe(false);
+    expect(handOffRefused('refused', 'required')).toBe(false);
+  });
+});
+
+describe('boxedRunLeftContainer', () => {
+  it('reaps for every abnormal exit, not just the timeout', () => {
+    // The first cut reaped on `spawnTimedOut` alone. A `maxBuffer` overflow —
+    // a reviewed command writing 64 MB to one stream, which a postinstall can
+    // do — kills the client with ENOBUFS and no timeout, so the container kept
+    // the review temp dir mounted read-write past the end of the review. The
+    // two call sites had also drifted to different conditions, which is how one
+    // came to miss a case the other caught.
+    expect(boxedRunLeftContainer(null)).toBe(true); // ETIMEDOUT, ENOBUFS, signal
+    // A normal exit needs no reaping — `--rm` has already fired.
+    expect(boxedRunLeftContainer(0)).toBe(false);
+    expect(boxedRunLeftContainer(1)).toBe(false);
+  });
+});
+
+describe('mountRootFor', () => {
+  // Real directories, because the function is no longer lexical: it realpaths
+  // the root and refuses a redirected ancestor, and a fixture of invented
+  // paths would pin the arithmetic while missing both.
+  const made: string[] = [];
+  const tmp = () => {
+    const dir = mkdtempSync(join(tmpdir(), 'qwen-mount-'));
+    made.push(dir);
+    return dir;
+  };
+  afterEach(() => {
+    for (const dir of made.splice(0))
+      rmSync(dir, { recursive: true, force: true });
+  });
+
+  // Every absolute Windows path carries a colon — the drive letter — so the
+  // check below refuses all of them there. That is the shipped behaviour (see
+  // `mountRootFor`, and the win32 test at the end of this block), but it makes
+  // "which root is mountable" a question Windows cannot be asked, and these
+  // three cases exist only to ask it. `test_windows` is merge_group-only, so
+  // an ungated assertion here would first go red inside the merge queue.
+  const itWhereRootsCanMount = it.skipIf(process.platform === 'win32');
+
+  itWhereRootsCanMount('refuses a root the -v grammar cannot spell', () => {
+    // `-v src:dst` has exactly one separator. A checkout at `/…/my:repo` makes
+    // the spec `…/my:repo/.qwen/tmp:…/my:repo/.qwen/tmp`, which docker rejects
+    // as "too many colons" — measured, not assumed. Saying "mountable" about
+    // that root sends every command in the phase into a raw mount error
+    // instead of the fallback (`auto`) or the refusal (`required`) already
+    // written for roots that cannot be mounted.
+    const root = tmp();
+    const colon = join(root, 'my:repo', '.qwen', 'tmp', 'review-pr-9');
+    mkdirSync(colon, { recursive: true });
+    expect(mountRootFor(colon)).toBeNull();
+    // The comparison case, so this is a statement about the colon and not
+    // about a deep path: docker takes a comma in a `-v` spec without complaint.
+    const comma = join(root, 'my,repo', '.qwen', 'tmp', 'review-pr-9');
+    mkdirSync(comma, { recursive: true });
+    expect(mountRootFor(comma)).not.toBeNull();
+  });
+
+  itWhereRootsCanMount('takes the DEEPEST temp dir, not the first', () => {
+    // A review run from inside another review's worktree — this pipeline's own
+    // dogfood geometry — nests one `.qwen/tmp` inside another. First-occurrence
+    // search widens the mount to the OUTER temp dir, which pulls `<repo>/.git`
+    // and every sibling checkout into the container and defeats the one
+    // property the mount exists for.
+    const root = tmp();
+    const inner = join(
+      root,
+      '.qwen',
+      'tmp',
+      'checkouts',
+      'myrepo',
+      '.qwen',
+      'tmp',
+    );
+    mkdirSync(join(inner, 'review-pr-1-probe'), { recursive: true });
+
+    expect(mountRootFor(join(inner, 'review-pr-1-probe'))).toBe(
+      realpathSync(inner),
+    );
+  });
+
+  itWhereRootsCanMount(
+    'refuses a root reached through a symlink instead of mounting it',
+    () => {
+      // `resolve` never touches the filesystem, so a link at or above
+      // `.qwen/tmp` — committable as mode 120000, materialised by a fresh clone
+      // — would silently widen a read-write bind mount to wherever it points.
+      // Every other creating or destroying path in this pipeline refuses that.
+      const root = tmp();
+      const elsewhere = tmp();
+      mkdirSync(join(elsewhere, 'tmp', 'review-pr-1'), { recursive: true });
+      mkdirSync(join(root, '.qwen'), { recursive: true });
+      symlinkSync(join(elsewhere, 'tmp'), join(root, '.qwen', 'tmp'));
+
+      expect(mountRootFor(join(root, '.qwen', 'tmp', 'review-pr-1'))).toBe(
+        null,
+      );
+
+      // ...and the same layout without the link is mounted normally, so the
+      // refusal is about the redirect and not about the shape.
+      const honest = tmp();
+      mkdirSync(join(honest, '.qwen', 'tmp', 'review-pr-1'), {
+        recursive: true,
+      });
+      expect(mountRootFor(join(honest, '.qwen', 'tmp', 'review-pr-1'))).toBe(
+        realpathSync(join(honest, '.qwen', 'tmp')),
+      );
+    },
+  );
+
+  it.skipIf(process.platform !== 'win32')(
+    'refuses every absolute path on Windows, where the drive letter is a colon',
+    () => {
+      // The other side of the same coin, and the reason the three above are
+      // skipped rather than deleted: containment is unavailable on Windows —
+      // this mount uses one path as both source and target, and the container
+      // side has no `C:` — so refusing is the honest answer, not a casualty.
+      const root = tmp();
+      const tree = join(root, '.qwen', 'tmp', 'review-pr-1');
+      mkdirSync(tree, { recursive: true });
+      // The layout has to EXIST first. Named at a path that does not, this
+      // returns null out of the realpath catch and says nothing at all about
+      // the drive letter — the first version of this test did exactly that,
+      // and a build with the colon check deleted passed it.
+      expect(existsSync(tree)).toBe(true);
+      expect(mountRootFor(tree)).toBe(null);
+    },
+  );
+
+  it('is null outside a temp dir, so a local checkout is never mounted', () => {
+    // `/review` of a local checkout has no sibling layout: the tree under test
+    // IS the user's working copy.
+    const root = tmp();
+    expect(mountRootFor(join(root, 'myrepo'))).toBe(null);
+  });
+});
+
+describe('reviewSandboxImage', () => {
+  it('is overridable, because one image cannot carry every toolchain', () => {
+    expect(reviewSandboxImage({ QWEN_REVIEW_SANDBOX_IMAGE: 'mine:1' })).toBe(
+      'mine:1',
+    );
+    expect(reviewSandboxImage({})).toContain('sandbox');
+  });
+});

packages/cli/src/commands/review/lib/sandboxed-exec.ts

@@ -0,0 +1,763 @@
+/**
+ * @license
+ * Copyright 2026 Qwen Team
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/**
+ * Running the reviewed PR's own code behind a container boundary (#9556).
+ *
+ * A review executes the code it is reviewing. `build-test` runs the commands
+ * the reviewed repository's `package.json` names — `npm ci` with its
+ * `preinstall`/`postinstall` scripts, the build, the suite — and
+ * `test-efficacy` runs that suite again once per baseline, control, mutant,
+ * hunk probe and revert. Both did it as the invoking identity, in that
+ * identity's environment: measured at the two call sites, the PR's code was
+ * handed `process.env` entire, which on CI carries `OPENAI_API_KEY` and
+ * `GH_TOKEN`. Reading them is one line in a `postinstall`, and it needs none
+ * of the git-config machinery the review pipeline's threat findings are built
+ * on.
+ *
+ * So the boundary goes around the EXECUTIONS, not around the review agent.
+ * Wrapping the agent was tried first and is the wrong shape: its secrets do
+ * not survive the container's env allowlist, its `timeout` reaps the host-side
+ * client rather than the container, its CLI version stops matching the
+ * runner's — and after all of it the mount is the whole checkout, so
+ * `<repo>/.git` (the `filter.*`, `core.fsmonitor` and `refs/replace` surface)
+ * stays writable anyway. Wrapping the commands costs none of that and closes
+ * more.
+ *
+ * Three properties do the work, and each is a decision this module encodes
+ * rather than a default it inherits:
+ *
+ * 1. **The mount is the review temp dir, not the tree the command runs in.**
+ *    The dependency farm links OUT of each tree: `exposeDependencies` points
+ *    every package in the probe tree's `node_modules` at the review
+ *    worktree's copy (measured on a live CI review: 1 722 links). Mounting
+ *    one tree would leave all of them dangling. Every tree the pipeline
+ *    builds — the review worktree, `-probe`, `-base`, every `-scratch-*` —
+ *    is a sibling under `.qwen/tmp`, so one mount covers both ends of every
+ *    link while `<repo>/.git` stays outside it.
+ * 2. **The environment is an allowlist**, not the inherited one. The PR's
+ *    code gets the npm knobs the pipeline sets deliberately and nothing else.
+ * 3. **The network is per command kind.** An install needs the registry; a
+ *    build and a suite do not. `--network none` keeps loopback, so a suite
+ *    that stands up a local fixture server still runs.
+ */
+
+import { spawnSync } from 'node:child_process';
+import { realpathSync } from 'node:fs';
+import { basename, dirname, join, resolve, sep } from 'node:path';
+import { operatorReviewSettings } from './review-settings.js';
+import { REVIEW_TMP_DIR } from './paths.js';
+import { redirectedAncestor } from './worktree.js';
+import { CUSTOM_SANDBOX_IMAGE_ENV_VAR } from '../../../utils/processUtils.js';
+import { isFileSourcedEnvKey } from '../../../config/environment.js';
+
+/**
+ * The fallback when neither override names an image: the published sandbox
+ * image for this CLI line. Pinned by tag rather than digest on purpose — a
+ * digest would go stale in a file nobody updates, and the override exists for
+ * anyone who needs reproducibility.
+ */
+const DEFAULT_IMAGE = 'ghcr.io/qwenlm/qwen-code/sandbox:latest';
+
+/** Container runtimes this module knows how to drive, in preference order. */
+const RUNTIMES = ['docker', 'podman'] as const;
+export type ContainerRuntime = (typeof RUNTIMES)[number];
+
+/**
+ * What the operator asked for.
+ *
+ * - `off` — run the PR's commands directly, as every review does today.
+ * - `auto` — use a container when one is available, run directly when not.
+ * - `required` — refuse to run the PR's commands unsandboxed. The caller
+ *   reports the affected evidence unavailable; it does not abandon the review.
+ */
+export type SandboxPolicy = 'off' | 'auto' | 'required';
+
+const POLICIES: readonly string[] = ['off', 'auto', 'required'];
+
+/**
+ * The policy for this run.
+ *
+ * `QWEN_REVIEW_SANDBOX` wins, so CI can require containment without depending
+ * on a settings file the runner may not carry. Below it, `review.sandbox` is
+ * read through {@link operatorReviewSettings}, which loads the operator scopes
+ * ONLY — a repository must not be able to switch off the containment that
+ * exists to contain it, and `.qwen/settings.json` is repository content the
+ * review reads.
+ *
+ * The default is `off`: today every review runs the PR's code directly, and
+ * turning that into a container by default would change what `npm ci` builds
+ * (native modules against a different libc) on machines nobody asked. CI opts
+ * in explicitly; a local operator opts in when they want it.
+ */
+export function sandboxPolicy(
+  env: NodeJS.ProcessEnv = process.env,
+  // Injected so a test can pin the settings half without a settings file on
+  // disk deciding the outcome.
+  settings: { sandbox?: string } = operatorReviewSettings(),
+  fileSourced: (key: string) => boolean = isFileSourcedEnvKey,
+): SandboxPolicy {
+  // The env layer is READ, but it may only ever tighten, and only when the
+  // value is a real process variable.
+  //
+  // Both halves are load-bearing, and the first cut had neither. `process.env`
+  // is not the operator's alone: `loadEnvironment` walks up from cwd and
+  // applies `<repo>/.qwen/.env` — repository content, from the very checkout
+  // under review, admitted by default because folder trust starts off. Letting
+  // that outrank the setting made the guarantee this module advertises
+  // ("a repository cannot switch off the containment that exists to contain
+  // it") true of `settings.json` and false in practice: `QWEN_REVIEW_SANDBOX=off`
+  // in a committed `.env` disabled it. So a file-sourced value is ignored here,
+  // and even a genuine process variable can only raise the policy, never lower
+  // it — a CI workflow's `env:` block requiring containment still works, while
+  // nothing reachable by the reviewed repository can take it away.
+  const raw = env['QWEN_REVIEW_SANDBOX']?.trim().toLowerCase();
+  const fromEnv =
+    raw && POLICIES.includes(raw) && !fileSourced('QWEN_REVIEW_SANDBOX')
+      ? (raw as SandboxPolicy)
+      : undefined;
+  // Normalised the SAME way as the env value above. It was not, and the
+  // asymmetry fell on the wrong side: `"Required"` — or a trailing space — in
+  // settings.json matched no policy, resolved to `off`, and silently disabled
+  // the containment the operator had just asked for. Settings is the
+  // documented way to turn this on (the environment can only tighten), so the
+  // unnormalised half was the half operators actually use.
+  const setting = settings.sandbox?.trim().toLowerCase();
+  const fromSettings =
+    setting && POLICIES.includes(setting)
+      ? (setting as SandboxPolicy)
+      : undefined;
+  const strictest = (a: SandboxPolicy, b: SandboxPolicy) =>
+    POLICIES.indexOf(a) >= POLICIES.indexOf(b) ? a : b;
+  if (fromEnv && fromSettings) return strictest(fromEnv, fromSettings);
+  return fromEnv ?? fromSettings ?? 'off';
+}
+
+let probed: ContainerRuntime | null | undefined;
+
+/**
+ * The first runtime whose daemon actually answers, or null.
+ *
+ * `<rt> info` rather than presence on `PATH`: a docker client with no
+ * reachable daemon is the shape that otherwise fails deep inside the first
+ * command, after an install has already burned minutes — the same reason
+ * `qwen-autofix.yml` preflights the daemon before its sandboxed agent starts.
+ */
+export function containerRuntime(): ContainerRuntime | null {
+  if (probed !== undefined) return probed;
+  probed = firstAnsweringRuntime(daemonAnswers);
+  return probed;
+}
+
+/**
+ * The decision, separated from the memo and the spawn so it can be asked.
+ *
+ * Order is the whole content: `RUNTIMES` is tried in sequence and the first
+ * one whose daemon answers wins, so a client installed but not running never
+ * shadows one that is.
+ */
+export function firstAnsweringRuntime(
+  answers: (runtime: ContainerRuntime) => boolean,
+): ContainerRuntime | null {
+  for (const runtime of RUNTIMES) {
+    if (answers(runtime)) return runtime;
+  }
+  return null;
+}
+
+/** `<rt> info` — a real round trip to the daemon, not presence on `PATH`. */
+function daemonAnswers(runtime: ContainerRuntime): boolean {
+  const r = spawnSync(runtime, ['info'], {
+    stdio: 'ignore',
+    timeout: 30_000,
+    env: runtimeClientEnv(),
+  });
+  return !r.error && r.status === 0;
+}
+
+/**
+ * Whether the runtime maps container uids through a USER NAMESPACE.
+ *
+ * Rootless podman (its default install mode) and rootless docker run the whole
+ * engine inside the invoking user's namespace: container uid 0 is the invoking
+ * host user, and every other container uid lands on a host SUBUID around
+ * 100000. So the `--user uid:gid` below, which is exactly right on a rootful
+ * engine, is exactly wrong here — the container process becomes a stranger to
+ * the host-created tree it is mounted on. `npm ci` cannot create `node_modules`
+ * inside it (host uid owns it at mode 755, the container is "other"), so the
+ * review reports an install failure with no evidence at all — strictly worse
+ * than the direct path this feature replaced. And what the container DOES
+ * create in the mount comes out subuid-owned, which the host sweeps cannot
+ * remove: the cross-run residue `--user` is here to prevent.
+ *
+ * Dropping `--user` under rootless is not a weakening. The container's root IS
+ * the invoking user on the host — the same uid `--user` was naming, reached the
+ * way this engine reaches it — so the files land host-owned and sweepable, and
+ * nothing gains a host privilege the operator did not already have.
+ *
+ * Unknown answers rootful, which keeps `--user`. The two failure directions are
+ * not symmetric: guessing rootful on a rootless host breaks the run loudly,
+ * while guessing rootless on a ROOTFUL one silently runs the reviewed code as
+ * real uid 0 with this mount writable, and leaves root-owned residue behind.
+ * A probe that cannot answer must not pick the silent one.
+ */
+export function runtimeIsRootless(
+  runtime: ContainerRuntime,
+  read: (rt: ContainerRuntime) => string = cachedInfoDocument,
+): boolean {
+  return hasRootlessMarker(read(runtime));
+}
+
+/**
+ * The runtime's `info` document, or an empty string if it could not be had.
+ *
+ * Empty is what makes the unknown case answer rootful WITHOUT a decision: an
+ * empty document carries no marker, so the predicate above says false on its
+ * own, and there is no second code path holding a literal that a mutant could
+ * flip the other way. The direction matters — see `runtimeIsRootless`.
+ */
+export function readInfoDocument(runtime: ContainerRuntime): string {
+  try {
+    const r = spawnSync(runtime, ['info', '--format', '{{json .}}'], {
+      encoding: 'utf8',
+      timeout: 30_000,
+      maxBuffer: 8 * 1024 * 1024,
+      stdio: ['ignore', 'pipe', 'pipe'],
+      env: runtimeClientEnv(),
+    });
+    if (r.error || r.status !== 0) return '';
+    return String(r.stdout ?? '');
+  } catch {
+    return '';
+  }
+}
+
+/** One `info` spawn per runtime per process, not one per command. */
+function cachedInfoDocument(runtime: ContainerRuntime): string {
+  const cached = infoDocuments.get(runtime);
+  if (cached !== undefined) return cached;
+  const doc = readInfoDocument(runtime);
+  infoDocuments.set(runtime, doc);
+  return doc;
+}
+
+/**
+ * The whole `info` document is searched rather than one schema path, because
+ * the two runtimes spell this in unrelated places — docker as a
+ * `SecurityOptions` entry, podman as `Host.Security.Rootless` — and a
+ * per-runtime template that a version rename breaks would fail to the rootful
+ * answer without saying so. Verified against a live rootful docker: the word
+ * does not occur anywhere in its `info` document, so a marker hit is a
+ * positive statement, not an accident of some unrelated field.
+ */
+export function hasRootlessMarker(info: string): boolean {
+  return (
+    info.includes('"name=rootless"') ||
+    info.includes('"rootless":true') ||
+    info.includes('"Rootless":true')
+  );
+}
+
+const infoDocuments = new Map<ContainerRuntime, string>();
+
+/** What a caller must do with one command. */
+export type SandboxVerdict =
+  | { kind: 'direct'; disclose?: string }
+  | { kind: 'container'; runtime: ContainerRuntime }
+  | { kind: 'refused'; reason: string };
+
+/**
+ * Decide once, for this run, how the PR's commands are to be executed.
+ *
+ * `SANDBOX` set means this process is ALREADY inside the CLI's own sandbox
+ * (`sandbox.ts` sets it to the seatbelt profile or the container name), so a
+ * container here would be a container inside a container: the outer boundary
+ * is the one the operator asked for, and this returns `direct`.
+ */
+export function sandboxVerdict(
+  policy: SandboxPolicy = sandboxPolicy(),
+  env: NodeJS.ProcessEnv = process.env,
+  // Injected so a test's outcome does not depend on whether the machine
+  // running it happens to have a daemon — this decides whether the reviewed
+  // code is contained, and a test that answers differently on two machines
+  // pins nothing.
+  probe: () => ContainerRuntime | null = containerRuntime,
+): SandboxVerdict {
+  // NOTE: `SANDBOX` being set is NOT a shortcut past the policy. The first cut
+  // returned `direct` for it, reasoning that the outer boundary is the one the
+  // operator asked for — which is wrong for the property this module is about.
+  // The CLI's own sandbox constrains the filesystem and the network; it hands
+  // the child `process.env` entire, secrets included, and stripping those is
+  // half of what `required` promises. An already-sandboxed session that also
+  // wants an env allowlist gets it the same way everyone else does: the probe
+  // below finds a runtime or it does not, and `required` refuses if it does
+  // not. The disclosure survives on the direct path, where it is useful.
+  if (policy === 'off') {
+    return {
+      kind: 'direct',
+      disclose:
+        'the PR’s own build and test commands ran as you, in your environment — ' +
+        'set review.sandbox to "auto" or "required" to run them in a container',
+    };
+  }
+  const runtime = probe();
+  if (runtime) return { kind: 'container', runtime };
+  if (policy === 'required') {
+    return {
+      kind: 'refused',
+      reason:
+        'review.sandbox is "required" and no container runtime answered ' +
+        `(tried ${RUNTIMES.join(', ')})` +
+        (env['SANDBOX']
+          ? ' — this session is itself sandboxed, which constrains the ' +
+            'filesystem but still hands the PR’s commands this process’s ' +
+            'environment, so it does not satisfy "required"'
+          : ''),
+    };
+  }
+  return {
+    kind: 'direct',
+    disclose: env['SANDBOX']
+      ? 'no container runtime answered; the PR’s commands ran inside this session’s own sandbox, which does not strip its environment'
+      : 'no container runtime answered, so the PR’s commands ran directly',
+  };
+}
+
+/**
+ * The one place a `required` refusal becomes an outcome.
+ *
+ * `sandboxVerdict` can say `refused`, and the first cut left acting on it to
+ * "the caller" — where no caller acted, so `required` ran the reviewed code
+ * unsandboxed with the full environment and the policy meant nothing. It has
+ * to be decided ONCE, at the top of a phase, before anything executes:
+ *
+ * - the two spawn sites cannot refuse usefully — by the time they are reached
+ *   the phase has committed to producing a verdict, and a per-command refusal
+ *   would read as a build failure rather than as absent evidence;
+ * - one of them is not even on the path. When the toolchain cannot be scoped
+ *   (a yarn/pnpm/bun repo, no `package-lock.json` — `unsupportedReport`), the
+ *   pipeline hands the install/build/test to the AGENT's own shell, which
+ *   never passes through `run()` at all. A gate at the spawn would leave that
+ *   route wide open under the very policy that forbids it.
+ *
+ * Returns the reason when the phase must not execute the reviewed repository's
+ * code, or null when it may.
+ */
+export function refuseUnsandboxedPhase(
+  // The tree this phase would execute in. Required, because "is a runtime
+  // running" is the wrong question — see below.
+  root: string,
+  verdict: SandboxVerdict = sandboxVerdict(),
+  mountRoot: (cwd: string) => string | null = mountRootFor,
+  policy: SandboxPolicy = sandboxPolicy(),
+): string | null {
+  if (verdict.kind === 'refused') return verdict.reason;
+  if (verdict.kind === 'direct') return null;
+  // Only `required` turns an unmountable tree into a refusal. Under `auto` the
+  // contract is "contain it when that is possible" — a `/review` of a local
+  // checkout has no layout to mount, and refusing there would take the
+  // build/test and efficacy evidence away from every local review the moment a
+  // daemon happened to be running. The first cut of this branch refused
+  // regardless of policy and said "required" in a message `auto` could reach.
+  if (policy !== 'required') return null;
+  // A runtime answering is not containment. The first cut asked only whether
+  // one did, and every route the container cannot actually serve still ran the
+  // reviewed code with the full environment under `required`: a `/review` of a
+  // local checkout has no `.qwen/tmp` layout to mount, so `mountRootFor`
+  // returns null, the command falls through to the direct spawn, and the
+  // report is indistinguishable from a contained run. The question the policy
+  // asks is whether THIS phase can be contained, and the mount is the half
+  // that can fail while the runtime is healthy.
+  if (mountRoot(root) === null) {
+    return (
+      'review.sandbox is "required" and this tree cannot be mounted: it is ' +
+      `not under a review temp dir (${root}), which is the layout the ` +
+      'container boundary is built on — a review of a local checkout has no ' +
+      'such layout, so its commands cannot be contained'
+    );
+  }
+  return null;
+}
+
+/** Whether one command needs the network. */
+export type CommandKind = 'install' | 'build' | 'test';
+
+/**
+ * The environment the PR's code is given inside the container.
+ *
+ * An allowlist rather than the inherited environment, which is the point:
+ * today both call sites hand it `process.env`, and on CI that carries the
+ * review's model and GitHub credentials. `CI` and the npm knobs are the ones
+ * the pipeline sets on purpose (`buildRunEnv`), so they are the ones that
+ * cross.
+ */
+export const CONTAINER_HOME = '/qwen-review-home';
+
+export function containerEnv(cacheDir: string): string[] {
+  return [
+    'CI=1',
+    'npm_config_yes=true',
+    'QWEN_SKIP_PREPARE=1',
+    // `HOME` explicitly, because forcing a uid resets it to `/` in these
+    // images — `utils/sandbox.ts` copies the host's for the same reason — and
+    // `/` is not writable by the mapped user, so npm's first write fails
+    // before the install starts.
+    //
+    // And it points at a TMPFS, not at the mount. The first cut put it under
+    // the mount and shared it across every command of every tree: `sh -lc` is
+    // a login shell that sources `$HOME/.profile`, and npm reads
+    // `$HOME/.npmrc`, so one run's postinstall could plant both and the NEXT
+    // review's install — network on — would source and read them. That is
+    // cross-run execution wearing this module's own `--rm` "isolation by
+    // construction" claim, and it was introduced by the fix for the `$HOME`
+    // problem rather than found in the original. A tmpfs is discarded with the
+    // container and never touches the host, so the claim is true again.
+    `HOME=${CONTAINER_HOME}`,
+    // The npm cache stays on the mount, deliberately: it is what keeps an
+    // install from re-downloading ~1 700 packages every review, it holds no
+    // rc file or profile, and npm verifies each entry's integrity hash on
+    // read. That verification is what stands between a poisoned cache and a
+    // bad install — worth naming rather than implying the cache is inert.
+    `npm_config_cache=${cacheDir}`,
+  ];
+}
+
+/**
+ * The directory to mount for a command running in `cwd`, or null when `cwd` is
+ * not one of the pipeline's trees.
+ *
+ * Not the tree: the dependency farm links out of every tree into the review
+ * worktree's `node_modules`, so a per-tree mount leaves every link dangling.
+ * Every tree the pipeline builds is a sibling under the review temp dir, so
+ * that directory covers both ends of every link while `<repo>/.git` stays
+ * outside it.
+ *
+ * `lastIndexOf`, because a review run from inside another review's worktree —
+ * this pipeline's own dogfood geometry — nests one `.qwen/tmp` inside another,
+ * and the FIRST occurrence would widen the mount to the outer temp dir,
+ * pulling `<repo>/.git` and every sibling checkout in with it. Tree names
+ * cannot contain a separator (scratch labels flatten to `[A-Za-z0-9._-]`), so
+ * the deepest occurrence is always the tree's own parent.
+ *
+ * Null for a cwd outside any temp dir — a `/review` of a local checkout, where
+ * the tree under test IS the user's working copy and there is no sibling
+ * layout to mount.
+ */
+export function mountRootFor(cwd: string): string | null {
+  const resolved = resolve(cwd);
+  const marker = `${sep}${REVIEW_TMP_DIR}${sep}`;
+  const at = resolved.lastIndexOf(marker);
+  if (at < 0) return null;
+  const root = resolved.slice(0, at + marker.length - 1);
+  // A LEXICAL root is not a safe mount target. `resolve` never touches the
+  // filesystem, so a symlink at or above `.qwen/tmp` — committable as mode
+  // 120000 and materialised by a fresh clone — silently widens a read-write
+  // bind mount to wherever it points. Every other creating or destroying path
+  // in this pipeline refuses that (`runCleanup`, `releaseWorktree`,
+  // `resetScratchTree`); the mount is the one place a redirect would hand the
+  // reviewed code a directory nobody chose.
+  try {
+    if (redirectedAncestor(root, dirname(resolve(root, '..', '..'))) !== null) {
+      return null;
+    }
+    const real = realpathSync(root);
+    // `-v src:dst` separates its fields with `:`, so a root that contains one
+    // cannot be spelled in that grammar at all: docker answers `invalid spec
+    // ... too many colons` and every command in the phase hard-fails with a
+    // raw mount error. Under `auto` those land as build/test failures the
+    // report attributes to the PR; under `required` the gate passes and the
+    // refusal that should have explained it never happens. Both designed
+    // degradations are bypassed because this said "mountable" about a root
+    // that is not.
+    //
+    // Refusing it here puts such a checkout back on the path every other
+    // unmountable root already takes. That is the whole fix: the `-v` grammar
+    // has exactly one separator, and `:` in a repository path — legal, if
+    // rare — is the only way to write it.
+    //
+    // On Windows this refuses EVERY absolute path, and that is the right
+    // answer rather than a casualty of it: a drive letter is a colon, and the
+    // mount this builds uses one path as both source and target, which a
+    // Windows path cannot be — the container side has no `C:`. So containment
+    // is not available there, and saying so gives `auto` its direct fallback
+    // and `required` its refusal instead of the runtime's parse error on every
+    // single command.
+    if (real.includes(':')) return null;
+    return real;
+  } catch {
+    return null;
+  }
+}
+
+/**
+ * Whether a hand-off report must become a refusal.
+ *
+ * The hand-off — `toolchain: "unsupported"`, which the brief reads as "install
+ * and build it yourself" — is an execution that leaves this process for an
+ * agent's own shell, contained by nothing here. Under `required` that is the
+ * one route the phase gate cannot catch, because the gate passes exactly when
+ * a runtime answered and the tree is mountable, which is when a repo the
+ * adapters cannot scope still reaches the hand-off.
+ *
+ * A predicate, and exported, because the first attempt at this lived inline as
+ * `!applicable` — the filtered adapter ARRAY, never falsy — and shipped as dead
+ * code that no test could see.
+ */
+export function handOffRefused(
+  toolchain: string,
+  policy: SandboxPolicy = sandboxPolicy(),
+): boolean {
+  return toolchain === 'unsupported' && policy === 'required';
+}
+
+/**
+ * The path a tree has INSIDE the container.
+ *
+ * The bind mount is created from the root's realpath, so a tree named by a
+ * path that differs from its canonical spelling — `/var` against
+ * `/private/var` on macOS is the everyday case — is present in the container
+ * under the canonical name only. Handing `--workdir` the lexical spelling
+ * then names a directory the container does not have, and every command
+ * fails before it starts.
+ *
+ * Null when neither the tree NOR its parent can be canonicalised — a tree
+ * whose directory went away under a cancelled run. Not the same question as
+ * `mountRootFor`'s: that one decides whether a root can be mounted at all,
+ * this one only spells a path that already lives under one.
+ */
+export function containerPathFor(cwd: string): string | null {
+  const resolved = resolve(cwd);
+  try {
+    return realpathSync(resolved);
+  } catch {
+    // Not created yet — a probe tree named before it is built. Its PARENT is,
+    // and the mount uses the parent's canonical spelling, so canonicalise that
+    // and re-attach the leaf.
+    try {
+      return join(realpathSync(dirname(resolved)), basename(resolved));
+    } catch {
+      return null;
+    }
+  }
+}
+
+export interface ContainerCommandOptions {
+  /** Where the command runs — a tree under `tmpDir`. */
+  cwd: string;
+  /** The container's name, so a timeout can reach it — see `containerName`. */
+  name: string;
+  /** The review temp dir (`<repo>/.qwen/tmp`): the mount, and the farm's far end. */
+  tmpDir: string;
+  kind: CommandKind;
+  runtime: ContainerRuntime;
+  image: string;
+  /**
+   * Whether `runtime` maps uids through a user namespace — see
+   * `runtimeIsRootless`. Passed IN rather than probed here so the argv this
+   * builds stays a pure function of its inputs, and so both call sites answer
+   * the question once per review instead of once per command.
+   */
+  rootless: boolean;
+}
+
+/**
+ * The argv that runs `command` in a container, for `spawnSync` WITHOUT a
+ * shell — the command itself still reaches a shell, inside.
+ */
+let containerSeq = 0;
+
+/** A name no other run of this pipeline can collide with. */
+export function containerName(): string {
+  return `qwen-review-${process.pid}-${Date.now().toString(36)}-${containerSeq++}`;
+}
+
+/**
+ * Whether a boxed spawn left its container behind.
+ *
+ * `status === null` is exactly "the client did not exit normally", and it is
+ * one condition rather than a list of causes on purpose: the first cut reaped
+ * on `spawnTimedOut` alone, which is true for ETIMEDOUT and false for a
+ * `maxBuffer` overflow — and a reviewed command writing 64 MB to stdout is a
+ * postinstall away. The two call sites had drifted to different conditions,
+ * which is how one of them came to miss a case the other caught.
+ *
+ * A normal exit needs no reaping: `--rm` has already fired. A client that
+ * never spawned has no container, and the reap is a silent no-op.
+ */
+export function boxedRunLeftContainer(status: number | null): boolean {
+  return status === null;
+}
+
+/**
+ * Kill a container the deadline could not.
+ *
+ * Best-effort by construction: this runs after a timeout has already cost the
+ * phase its result, so a failure here must not become a second one. What it
+ * must not do is nothing — see `containerCommand`'s `--name` comment for what
+ * survives otherwise.
+ */
+export function killContainer(
+  runtime: ContainerRuntime,
+  name: string,
+  spawn: typeof spawnSync = spawnSync,
+): void {
+  try {
+    spawn(runtime, ['rm', '-f', name], {
+      stdio: 'ignore',
+      timeout: 30_000,
+      env: runtimeClientEnv(),
+    });
+  } catch {
+    // Nothing to add: the caller is already reporting the timeout.
+  }
+}
+
+export function containerCommand(
+  command: string,
+  opts: ContainerCommandOptions,
+): { file: string; args: string[] } {
+  const args = [
+    'run',
+    '--rm',
+    '--init',
+    // A NAME, so the deadline has something to aim at. `--rm` fires only when
+    // the container exits on its own, and a `spawnSync` timeout kills the
+    // runtime CLIENT — measured on docker 29.1.3, an attached client forwards
+    // the signal and waits rather than dying, and a workload whose own trap
+    // ignores it keeps running with this mount writable, past the budget and
+    // past the end of the review. On a persistent runner that is one orphan
+    // per malicious review, holding the tree other agents are reading.
+    '--name',
+    opts.name,
+    // One ephemeral container per command, deliberately. A long-lived one per
+    // phase would be cheaper by a few hundred milliseconds a run — against a
+    // 540-second budget, one to two percent — and would re-introduce exactly
+    // the cross-run state this pipeline has spent rounds closing: a tracked
+    // file one run rewrote, an ignored plant a sweep honoured. `--rm` is
+    // isolation by construction rather than by hygiene.
+    '--volume',
+    `${opts.tmpDir}:${opts.tmpDir}`,
+    '--workdir',
+    opts.cwd,
+  ];
+  if (opts.kind !== 'install') args.push('--network', 'none');
+  // The container writes into a mount the HOST pipeline then has to clean up —
+  // `node_modules` after an install timeout, the tree itself at `discardWorktree`,
+  // the sweeps. The default image runs as root (`node:22-slim`, no `USER`), so
+  // without this every one of those hits EACCES and the residue accumulates
+  // across reviews: the cross-run-state class this pipeline has spent rounds
+  // closing. `utils/sandbox.ts` maps the same pair for the same hazard on the
+  // same image lineage, and honours the same opt-out.
+  //
+  // Known limit, stated rather than papered over: `--user` with a bare
+  // uid:gid leaves that uid absent from the container's `/etc/passwd`, so a
+  // tool that calls `getpwuid` (rather than reading `$HOME`) still sees an
+  // unknown user. `utils/sandbox.ts` avoids that by starting as root and
+  // `useradd`-ing the host's ids — machinery this does not need, because what
+  // runs here is a shell command, not the CLI whose `os.userInfo()` requires
+  // the entry. If a toolchain turns out to need it, that is the shape to copy.
+  const uid = process.getuid?.();
+  const gid = process.getgid?.();
+  if (
+    uid !== undefined &&
+    gid !== undefined &&
+    // Rootless engines remap this uid to a host subuid, which is the one shape
+    // where naming it is worse than not — see `runtimeIsRootless`.
+    !opts.rootless &&
+    // The opt-out is the operator's, not the repository's: a committed
+    // `SANDBOX_SET_UID_GID=false` would put the container back to root and
+    // leave root-owned residue the host cannot sweep.
+    !(
+      !isFileSourcedEnvKey('SANDBOX_SET_UID_GID') &&
+      process.env['SANDBOX_SET_UID_GID']?.toLowerCase().trim() === 'false'
+    )
+  ) {
+    args.push('--user', `${uid}:${gid}`);
+  }
+  // `mode=1777` so the mapped uid owns what it writes there: a tmpfs mounts
+  // root-owned by default, and `--user` would then be unable to write its own
+  // HOME — the very failure this HOME exists to prevent.
+  args.push('--tmpfs', `${CONTAINER_HOME}:rw,mode=1777`);
+  for (const entry of containerEnv(join(opts.tmpDir, '.npm-cache'))) {
+    args.push('--env', entry);
+  }
+  args.push(opts.image, 'sh', '-lc', command);
+  return { file: opts.runtime, args };
+}
+
+/**
+ * The image the reviewed repository's commands run in.
+ *
+ * Defaults to the CLI's own sandbox image, which already carries a Node
+ * toolchain — the same image `qwen --sandbox` uses, so a repository that
+ * builds under one builds under the other. `QWEN_REVIEW_SANDBOX_IMAGE`
+ * overrides it for a repository whose toolchain needs more (a JDK, a Python,
+ * a specific Node major), which is the case this default cannot cover and
+ * should not pretend to.
+ */
+export function reviewSandboxImage(
+  env: NodeJS.ProcessEnv = process.env,
+): string {
+  // File-sourced overrides are ignored for the same reason the policy ignores
+  // them, and this one is sharper: the image IS the code the reviewed
+  // repository's commands run inside, so a `.env` committed in that repository
+  // naming its own image would be arbitrary execution wearing the containment's
+  // name.
+  const pick = (key: string) =>
+    isFileSourcedEnvKey(key) ? undefined : env[key]?.trim();
+  return (
+    pick('QWEN_REVIEW_SANDBOX_IMAGE') ||
+    pick(CUSTOM_SANDBOX_IMAGE_ENV_VAR) ||
+    DEFAULT_IMAGE
+  );
+}
+
+/**
+ * The environment the container RUNTIME CLIENT is spawned with.
+ *
+ * `DOCKER_HOST` and its TLS companions decide which daemon answers — so a
+ * repository that ships one in `.qwen/.env` points both the availability probe
+ * and every `docker run` at a daemon it controls: `required` reads as
+ * satisfied, the mount is handed over, and whatever that daemon returns is
+ * scored as build, test and probe evidence. An operator's own `DOCKER_HOST`
+ * (a remote engine, colima, rootless) is untouched — only the file-sourced
+ * ones are dropped.
+ */
+export function runtimeClientEnv(
+  env: NodeJS.ProcessEnv = process.env,
+): NodeJS.ProcessEnv {
+  const scrubbed = { ...env };
+  // EVERY file-sourced key, not a list of the dangerous ones.
+  //
+  // The list was the first design and it lost twice: it named the daemon
+  // selectors and missed the proxy family, then named those and missed
+  // `DOCKER_API_VERSION` — a value that does not select a daemon at all, it
+  // just makes every call to one fail, which under `auto` turns containment
+  // off silently because the availability probe reads a broken client as "no
+  // runtime". The class is not "variables that point somewhere else", it is
+  // "variables a repository can set that change what this client does", and
+  // that has no last entry: an incompatible API version, a proxy, a config
+  // path, a `PATH` naming a different `docker` binary.
+  //
+  // The client does not need repository-provided environment for anything. So
+  // the rule is provenance, not name: what the loader wrote from a file the
+  // reviewed checkout supplies does not reach the process that decides whether
+  // containment happened.
+  //
+  // Deleting is the right restore, not an approximation of one: the loader
+  // records a key as file-sourced only where the real environment had nothing
+  // (`isEffectivelyUnset` in config/environment.ts), so a file value never
+  // shadows an inherited one and dropping it returns the variable to exactly
+  // its pre-load state. The scrub therefore cannot cost the client a `PATH` or
+  // `HOME` from the operator's shell — those are set, so they are never
+  // file-sourced. What it does cost is a value the operator kept ONLY in a
+  // `.env`, which `isFileSourcedEnvKey` cannot tell from the repository's own;
+  // that one must move to their shell. Conservative on the right side.
+  for (const key of Object.keys(scrubbed)) {
+    if (isFileSourcedEnvKey(key)) delete scrubbed[key];
+  }
+  return scrubbed;
+}

packages/cli/src/commands/review/lib/test-utils.ts

@@ -47,6 +47,37 @@ export function isolateHostGitConfig(): {
   };
 }
 
+/**
+ * Redirect the review settings the phase gates read away from the operator's
+ * own — the same shape as `isolateHostGitConfig`, for the same reason.
+ *
+ * `review.sandbox` is a setting a maintainer turns on for their OWN reviews,
+ * and the gates then correctly refuse to run anything uncontained. Under
+ * `required` that refusal is the right answer to give a review and the wrong
+ * answer to give a fixture: 101 tests across this directory stop measuring
+ * what they are about and start reporting the operator's preference back at
+ * them. `QWEN_HOME` is the lever because policy is the STRICTEST of settings
+ * and environment, so no environment value can loosen a settings-side opt-in.
+ *
+ * Call in beforeEach and `dispose()` in afterEach.
+ */
+export function isolateOperatorReviewSettings(): {
+  home: string;
+  dispose: () => void;
+} {
+  const home = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-review-home-')));
+  const saved = process.env['QWEN_HOME'];
+  process.env['QWEN_HOME'] = home;
+  return {
+    home,
+    dispose() {
+      if (saved === undefined) delete process.env['QWEN_HOME'];
+      else process.env['QWEN_HOME'] = saved;
+      rmSync(home, { recursive: true, force: true });
+    },
+  };
+}
+
 /** Seed the report `parse-args` tees, so the effort fallback has something to read. */
 export function seedParseArgs(dir: string, effort: unknown): void {
   mkdirSync(join(dir, dirname(PARSE_ARGS_REPORT)), { recursive: true });

packages/cli/src/commands/review/lib/toolchain.ts

@@ -5,6 +5,7 @@
  */
 
 import type { BuildTestReport, CommandResult } from '../build-test.js';
+import type { CommandKind } from './sandboxed-exec.js';
 
 export interface ToolchainRunArgs {
   root: string;
@@ -29,7 +30,18 @@ export interface ToolchainRunArgs {
    * which is every call that is not a continuation.
    */
   previous?: BuildTestReport;
-  exec: (command: string, cwd: string, timeoutMs: number) => CommandResult;
+  /**
+   * `kind` decides the containment policy when the reviewed repository's
+   * commands run sandboxed (#9556): only an install is given the network.
+   * Optional, and it defaults to the RESTRICTIVE side, so an adapter that
+   * does not pass it cannot silently grant egress.
+   */
+  exec: (
+    command: string,
+    cwd: string,
+    timeoutMs: number,
+    kind?: CommandKind,
+  ) => CommandResult;
 }
 
 export interface ReviewToolchainAdapter {

packages/cli/src/commands/review/test-delta.test.ts

@@ -110,6 +110,9 @@ describe('runTestDelta', () => {
       baseline,
       timeout: 60,
       now,
+      // Hermetic: the real gate reads the operator's own settings, and this
+      // helper's tests are about attribution, not about containment.
+      refuse: () => null,
       exec:
         typeof baseOutput === 'function'
           ? // Pass cwd through: swallowing it made the baseline-dir assertion
@@ -126,6 +129,56 @@ describe('runTestDelta', () => {
   });
   afterEach(() => rmSync(dir, { recursive: true, force: true }));
 
+  it('refuses the base-side rerun under `required` instead of running it on the host', () => {
+    // The PR side now runs in a container with an env allowlist and no
+    // network. Running the base side on the host anyway does not just break
+    // the operator's `required` — it makes the two sides incomparable, and
+    // this file exists to compare them. So under `required` with no usable
+    // containment the answer is "not measured", never a delta between two
+    // differently-shaped runs.
+    vi.stubEnv('QWEN_REVIEW_SANDBOX', 'required');
+    let ran = 0;
+    try {
+      const r = runTestDelta({
+        report: writeReport([cmd({ output: ' FAIL  src/new.test.ts > x' })]),
+        baseline,
+        timeout: 60,
+        exec: (command) => {
+          ran += 1;
+          return cmd({ command, output: '' });
+        },
+      });
+      // The rerun did not happen at all — asserting only on the note would
+      // pass just as well with the host run still going ahead behind it.
+      expect(ran).toBe(0);
+      expect(r.entries).toEqual([]);
+      expect(r.netNew).toEqual([]);
+      // ...and it says so in the words that stop `netNew: []` from being read
+      // as "no regression".
+      expect(r.note).toContain('NOTHING was attributed');
+    } finally {
+      vi.unstubAllEnvs();
+    }
+  });
+
+  it('measures the base side normally when no policy demands containment', () => {
+    // The other half of the gate: without `required` nothing changes, so the
+    // refusal above cannot be a blanket "test-delta stopped working".
+    let ran = 0;
+    const r = runTestDelta({
+      report: writeReport([cmd({ output: ' FAIL  src/new.test.ts > x' })]),
+      baseline,
+      timeout: 60,
+      refuse: () => null,
+      exec: (command) => {
+        ran += 1;
+        return cmd({ command, output: '' });
+      },
+    });
+    expect(ran).toBe(1);
+    expect(r.entries).toHaveLength(1);
+  });
+
   it('attributes a PR-only failure as netNew and a both-sides failure as shared', () => {
     const r = runWith(
       [
@@ -563,6 +616,7 @@ describe('the CLI option contract', () => {
 
     const report = runTestDelta({
       ...parsed,
+      refuse: () => null,
       // The base side prints the SAME failure under its own root.
       exec: (command) => ({
         command,

packages/cli/src/commands/review/test-delta.ts

@@ -39,17 +39,15 @@
 // rescued summary lines. A file this cannot parse is disclosed, never guessed.
 
 import type { CommandModule } from 'yargs';
-import { spawnSync } from 'node:child_process';
 import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs';
 import { dirname, resolve } from 'node:path';
 import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
 import {
-  buildRunEnv,
-  spawnTimedOut,
-  trimOutput,
+  run as runCommand,
   type BuildTestReport,
   type CommandResult,
 } from './build-test.js';
+import { refuseUnsandboxedPhase } from './lib/sandboxed-exec.js';
 import { failingFilesOf } from './lib/failing-files.js';
 import { TEST_COMMAND_RE } from './lib/npm-toolchain.js';
 
@@ -144,6 +142,15 @@ export interface TestDeltaArgs {
   timeout: number;
   /** Test seam — production spawns the real command. */
   exec?: (command: string, cwd: string, timeoutMs: number) => BaseRunResult;
+  /**
+   * Containment gate, injectable for the same reason `exec` is: it resolves
+   * the operator's OWN policy from settings and environment, so without a seam
+   * every test in this file changes its answer on a machine where the operator
+   * opted into `required` — 21 of 31 of them, measured. Tests that are about
+   * delta arithmetic pass a gate that never refuses; the one test that is
+   * about the gate drives the real chain.
+   */
+  refuse?: (root: string) => string | null;
   /** Injectable clock, for tests only — the budget math cannot be driven to
    *  its cutoff in real time. Matches `test-efficacy`'s seam; without it a
    *  test has to reassign the global `Date.now`. */
@@ -162,50 +169,13 @@ export interface TestDeltaArgs {
  * attributed to this PR by "measurement". Parse the raw text, report the
  * bounded one.
  */
-export interface BaseRunResult extends CommandResult {
-  /** Parsed from the untrimmed output. Absent from a seam that predates this. */
-  failingFiles?: string[];
-}
-
-// Mirrors build-test's run() on the three properties its comments call out as
-// deliberate — reviewed live when this reimplementation diverged on all three:
-// stdin ignored (a rerun that asks a question hangs to the deadline), timeout
-// read from error.code with the SIGTERM/null-status fallback (the substring
-// form misses a maxBuffer kill, which would flow into the base-green Critical
-// path), and trimmed output (a failing monorepo suite is hundreds of KB that
-// would otherwise land verbatim in the report Agent 7 reads).
-function run(command: string, cwd: string, timeoutMs: number): BaseRunResult {
-  const started = Date.now();
-  const r = spawnSync(command, {
-    shell: true,
-    cwd,
-    encoding: 'utf8',
-    timeout: timeoutMs,
-    env: buildRunEnv(process.env),
-    maxBuffer: 64 * 1024 * 1024,
-    // build-test's, deliberately: "a build that asks a question is a build that
-    // hangs until the deadline" — and this reruns those same commands.
-    stdio: ['ignore', 'pipe', 'pipe'],
-  });
-  // The sibling's predicate, not a weaker re-derivation: an external SIGTERM
-  // (container stop, cancelled CI job) sets neither an ETIMEDOUT message nor
-  // an exit code, so the substring form reported timedOut:false with empty
-  // output and fed straight into the base-green path.
-  const timedOut = spawnTimedOut(r);
-  const raw = `${r.stdout ?? ''}${r.stderr ?? ''}`;
-  return {
-    command,
-    exitCode: timedOut ? null : (r.status ?? null),
-    seconds: Math.round((Date.now() - started) / 1000),
-    timedOut,
-    failingFiles: timedOut ? [] : failingFilesOf(raw, cwd),
-    // Bounded like build-test's: this lands in `entries[].base.output`, which
-    // is JSON.stringify'd to --out, and the verdict fields sit AFTER it — an
-    // untrimmed megabyte pushes exactly what the command produces past any
-    // reader's truncation.
-    output: trimOutput(raw),
-  };
-}
+/**
+ * Nothing of its own any more: `failingFiles` moved onto `CommandResult` when
+ * build-test grew the untrimmed capture, and this re-declared it. Kept as the
+ * name the injectable `exec` seam has always been spelled with rather than
+ * churning every caller for an alias.
+ */
+export type BaseRunResult = CommandResult;
 
 /**
  * Whole-command budget, mirroring test-efficacy's. `--timeout` is PER COMMAND,
@@ -220,7 +190,23 @@ const TOTAL_BUDGET_MS = 540_000;
 const DEFAULT_TIMEOUT_S = 300;
 
 export function runTestDelta(args: TestDeltaArgs): TestDeltaReport {
-  const exec = args.exec ?? run;
+  // The base-side rerun crosses the SAME containment boundary as the PR side.
+  //
+  // It used to have its own `run()`, a careful copy of build-test's — and the
+  // copy was correct right up until build-test's grew a container. Then the two
+  // sides stopped being comparable: the PR side ran in the image with an env
+  // allowlist and no network, the base side on the host with the full
+  // environment and the full network. A test that reads an env var or opens a
+  // socket then fails on one side and passes on the other for a reason that has
+  // nothing to do with the diff, and this file's whole job is to say which side
+  // a failure belongs to. It would have said "the PR's" — a manufactured
+  // Critical — or, on the other flip, waved a real regression through as
+  // pre-existing.
+  //
+  // So the duplicate is gone rather than re-synchronised: one `run`, one place
+  // where the boundary is decided, and no way for the two sides to drift again.
+  // `kind` defaults to 'test', which is the restrictive shape (no network).
+  const exec = args.exec ?? runCommand;
   const baseline = resolve(args.baseline);
   const empty = (note: string): TestDeltaReport => ({
     entries: [],
@@ -243,6 +229,19 @@ export function runTestDelta(args: TestDeltaArgs): TestDeltaReport {
       `the base tree ${baseline} does not exist — run \`qwen review base-tree\` first`,
     );
   }
+  // `required` means no reviewed-repository command runs outside a container,
+  // and that has to include this one. The base tree holds base-commit content,
+  // so this is not the PR reaching the host — it is the operator's setting
+  // meaning what it says at every phase rather than at most of them. Refusing
+  // is also the honest answer for the measurement itself: with containment
+  // unavailable the PR side either refused too or ran somewhere else, and a
+  // delta between two differently-shaped runs is worse than no delta.
+  const refusal = (args.refuse ?? refuseUnsandboxedPhase)(baseline);
+  if (refusal) {
+    return empty(
+      `the base-side rerun did not happen, so NOTHING was attributed — ${refusal}`,
+    );
+  }
 
   // Failed for real: a timeout is an infrastructure result and reruns as one.
   const failed = (report.test ?? []).filter(

packages/cli/src/commands/review/test-efficacy.integration.test.ts

@@ -33,7 +33,10 @@ import {
   splitDiffIntoHunks,
   testEfficacyCommand,
 } from './test-efficacy.js';
-import { isolateHostGitConfig } from './lib/test-utils.js';
+import {
+  isolateHostGitConfig,
+  isolateOperatorReviewSettings,
+} from './lib/test-utils.js';
 
 type Handler = (args: {
   report: string;
@@ -47,6 +50,7 @@ const runHandler = testEfficacyCommand.handler as unknown as Handler;
 let repo: string;
 let outside: string;
 let gitIsolation: ReturnType<typeof isolateHostGitConfig>;
+let reviewSettingsIsolation: ReturnType<typeof isolateOperatorReviewSettings>;
 
 function git(cwd: string, ...args: string[]): string {
   return execFileSync('git', args, { cwd, encoding: 'utf8' });
@@ -163,6 +167,10 @@ process.stdout.write(JSON.stringify({
 }
 
 beforeEach(() => {
+  // The operator's own `review.sandbox` reaches the phase gate here too — see
+  // isolateOperatorReviewSettings; 19 of this file's tests report their
+  // refusal instead of their measurement without it.
+  reviewSettingsIsolation = isolateOperatorReviewSettings();
   repo = mkdtempSync(join(tmpdir(), 'efficacy-iso-'));
   outside = mkdtempSync(join(tmpdir(), 'efficacy-outside-'));
   // Isolate the fixtures from the user's git environment (shared helper —
@@ -245,6 +253,7 @@ afterEach(() => {
   rmSync(repo, { recursive: true, force: true });
   rmSync(outside, { recursive: true, force: true });
   gitIsolation.dispose();
+  reviewSettingsIsolation?.dispose();
 });
 
 describe('fixture git-config isolation', () => {

packages/cli/src/commands/review/test-efficacy.ts

@@ -66,6 +66,21 @@ import { probeWorktreePath } from './lib/paths.js';
 // stale-sweep-then-remove step (its rationale lives there, with the helper), and
 // `exposeDependencies` followed it when `scratch-tree` needed the same
 // dependency farm for the verifier's own probe tree.
+import { shellQuotePath } from './lib/shell-quote.js';
+import {
+  boxedRunLeftContainer,
+  containerCommand,
+  containerName,
+  containerPathFor,
+  killContainer,
+  mountRootFor,
+  refuseUnsandboxedPhase,
+  reviewSandboxImage,
+  runtimeIsRootless,
+  runtimeClientEnv,
+  sandboxVerdict,
+  type ContainerRuntime,
+} from './lib/sandboxed-exec.js';
 import {
   discardWorktree,
   exposeDependencies,
@@ -1516,6 +1531,45 @@ export function probeCleanupFailureDetail(
  * repository out into the tree, which is the hazard the residue probe's
  * identity gate exists for. Refusing is the only answer that is neither.
  */
+/**
+ * The container argv for one probe-suite run, or null to spawn it directly.
+ *
+ * Same three null cases as `build-test`'s: policy off, no runtime under
+ * `auto`, or a tree that is not under a review temp dir (a `/review` of a
+ * local checkout, where there is no `.qwen/tmp` layout to mount).
+ */
+function probeContainer(
+  command: string,
+  probeTree: string,
+): {
+  file: string;
+  args: string[];
+  name: string;
+  runtime: ContainerRuntime;
+} | null {
+  const verdict = sandboxVerdict();
+  if (verdict.kind !== 'container') return null;
+  const tmpDir = mountRootFor(probeTree);
+  if (tmpDir === null) return null;
+  // Canonical, matching the mount — see the twin in `build-test.ts`.
+  const workdir = containerPathFor(probeTree);
+  if (workdir === null) return null;
+  const name = containerName();
+  return {
+    ...containerCommand(command, {
+      cwd: workdir,
+      tmpDir,
+      kind: 'test',
+      name,
+      runtime: verdict.runtime,
+      rootless: runtimeIsRootless(verdict.runtime),
+      image: reviewSandboxImage(),
+    }),
+    name,
+    runtime: verdict.runtime,
+  };
+}
+
 function restoreProbeTreeTracked(probeTree: string): string | null {
   if (!existsSync(join(probeTree, '.git'))) {
     return `${probeTree} carries no .git, so there is no commit to put it back to`;
@@ -1682,22 +1736,75 @@ function runProbeSuite(
   // own test code: a suite that plants or replaces a module in `node_modules`
   // would otherwise decide every later run's verdict. Re-linking costs about a
   // second per run against the budget's minutes.
-  const exposed = exposeDependencies(probeTree, dependencyRoot, {
+  // The farm's link TARGETS must be spelled the way the mount is. The mount and
+  // `--workdir` are canonical (`mountRootFor` realpaths), while
+  // `exposeDependencies` builds targets from the argument it is given — so
+  // under a symlinked ancestor (macOS `/tmp` → `/private/tmp` is the everyday
+  // one) every link dangles INSIDE the container, and the phase reports "every
+  // file was red or collected nothing": a wiring failure published as a
+  // statement about the PR's own suite. Canonicalise what crosses the
+  // boundary, and only there — the direct path keeps the caller's spelling.
+  let farmRoot = dependencyRoot;
+  if (sandboxVerdict().kind === 'container') {
+    try {
+      farmRoot = realpathSync(dependencyRoot);
+    } catch {
+      // Unresolvable: the farm below reports what it could not link.
+    }
+  }
+  const exposed = exposeDependencies(probeTree, farmRoot, {
     rebuild: true,
   });
-  const r = spawnSync(
-    process.execPath,
-    [findVitestBin(dependencyRoot), 'run', '--reporter=json', ...probes],
-    {
-      cwd: probeTree,
-      encoding: 'utf8',
-      timeout,
-      // Vitest's JSON reporter on a large suite easily exceeds spawnSync's
-      // 1 MiB default stdout buffer, which returns ENOBUFS and turns every
-      // probe `inconclusive`. Match the 64 MiB ceiling the gh wrapper uses.
-      maxBuffer: 64 * 1024 * 1024,
-    },
-  );
+  // The reviewed repository's own suite, run once per baseline / control /
+  // mutant / hunk probe / revert — the second of the two places a review
+  // executes the code it is reviewing (#9556). Sandboxed it is a container
+  // per run, offline, with an env allowlist instead of this process's own;
+  // unsandboxed it is the direct spawn this has always been, and the caller
+  // has already disclosed that.
+  // `node` off the IMAGE's PATH, not `process.execPath`: the host's interpreter
+  // path (`/usr/bin/node` here, `/opt/hostedtoolcache/…` on a GitHub runner) is
+  // neither mounted nor present in the image, so baking it in exits 127 and
+  // maps every probe — baseline, control, each mutant, each hunk, the revert —
+  // to inconclusive, blaming the runner's output for a wiring error. The vitest
+  // bin path DOES resolve, because it lives under the mounted temp dir.
+  const suite = `node ${shellQuotePath(
+    findVitestBin(dependencyRoot),
+  )} run --reporter=json ${probes.map(shellQuotePath).join(' ')}`;
+  const boxed = probeContainer(suite, probeTree);
+  const r = boxed
+    ? spawnSync(boxed.file, boxed.args, {
+        cwd: probeTree,
+        encoding: 'utf8',
+        timeout,
+        // SIGKILL, not the default SIGTERM, and only on the boxed branch.
+        // `spawnSync` sends its `killSignal` at the deadline and then WAITS for
+        // the child to exit — so an attached runtime client that forwards the
+        // signal and keeps waiting on a workload whose own trap ignores it
+        // never returns, and the `killContainer` below is never reached. That
+        // is what made the round-4 machinery unreachable rather than wrong.
+        // SIGKILL cannot be ignored, so the client dies, the call returns, and
+        // the container is then reaped BY NAME at the daemon — which is where
+        // the deadline had to be enforced all along.
+        killSignal: 'SIGKILL',
+        maxBuffer: 64 * 1024 * 1024,
+        // The RUNTIME CLIENT's environment, minus the daemon-selecting
+        // variables a repository could have shipped in its own `.env` — the
+        // container's own environment is the allowlist in `containerEnv`.
+        env: runtimeClientEnv(),
+      })
+    : spawnSync(
+        process.execPath,
+        [findVitestBin(dependencyRoot), 'run', '--reporter=json', ...probes],
+        {
+          cwd: probeTree,
+          encoding: 'utf8',
+          timeout,
+          // Vitest's JSON reporter on a large suite easily exceeds spawnSync's
+          // 1 MiB default stdout buffer, which returns ENOBUFS and turns every
+          // probe `inconclusive`. Match the 64 MiB ceiling the gh wrapper uses.
+          maxBuffer: 64 * 1024 * 1024,
+        },
+      );
   // `r.error` is set — and `r.status` is null — when the process never ran
   // (vitest entry missing or unresolvable) or was killed (the timeout above
   // fires SIGTERM). Ignoring it reports those as "the runner produced no
@@ -1709,6 +1816,12 @@ function runProbeSuite(
   // SIGTERM", which is a less useful sentence about the same event. The reason
   // tag is derived from the whole result either way, so it does not depend on
   // which message wins.
+  if (boxed && boxedRunLeftContainer(r.status)) {
+    // The deadline killed the CLIENT; the container outlives it — `--rm` fires
+    // only on a self-exit. Reach the daemon before reporting, or a
+    // TERM-ignoring suite keeps this mount writable past the end of the review.
+    killContainer(boxed.runtime, boxed.name);
+  }
   if (r.error)
     throw new ProbeRunFailure(r.error.message, runnerFailureReason(r));
   if (r.signal) {
@@ -2424,7 +2537,23 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise<void> {
     }
   };
 
-  if (probes.length > 0 && revert.length > 0) {
+  // BEFORE the probe tree is even created. Every run this phase makes executes
+  // the reviewed repository's suite, so under `review.sandbox: required` with no
+  // container runtime the honest outcome is no efficacy evidence — not evidence
+  // bought by running that suite unsandboxed. Refusing here rather than at the
+  // spawn keeps the report's vocabulary intact: the phase produced nothing, and
+  // says why, instead of a run of probes each blaming the runner.
+  // The probe tree this phase WOULD build, named before it exists — the gate
+  // has to answer before anything is created, and `probeWorktreePath` is a
+  // pure path function.
+  const sandboxRefusal = refuseUnsandboxedPhase(probeWorktreePath(worktree));
+  if (sandboxRefusal) {
+    noteMutants(
+      `mutation probes did not run: ${sandboxRefusal}. Every probe executes ` +
+        `the reviewed repository's own test suite, which is what the policy ` +
+        `forbids unsandboxed — read the absence as unmeasured, not as covered.`,
+    );
+  } else if (probes.length > 0 && revert.length > 0) {
     // The probe reverts the PR's source to base and runs the tests against it —
     // in its OWN disposable worktree, checked out at the PR head and discarded
     // wholesale when the probe finishes. The shared worktree the other review

packages/cli/src/config/environment.ts

@@ -171,6 +171,45 @@ export function resetEnvironmentTrackingForTesting(): void {
   lastReloadSnapshotSeeded = false;
 }
 
+/**
+ * True when `key`'s current value in `process.env` was written by a FILE the
+ * loader read — a `.env` on the way up from cwd, or a settings `env` block —
+ * rather than by the process's actual environment.
+ *
+ * The distinction matters wherever a value decides something the file's author
+ * must not decide. `<repo>/.qwen/.env` is repository content: it is read from
+ * the checkout under review, and folder trust defaults off, so a fresh runner
+ * admits it. A setting that a repository is deliberately barred from making
+ * through `settings.json` (see `operatorReviewSettings`, which skips the
+ * workspace scope) is barred for nothing if the same value can arrive through
+ * the env layer that outranks it.
+ *
+ * Callers that consult this are saying: an operator may set this, a repository
+ * may not. The operator's routes remain their settings file and their real
+ * shell environment — including a workflow's `env:` block, which is a process
+ * variable and not file-sourced.
+ */
+export function isFileSourcedEnvKey(key: string): boolean {
+  if (dotEnvSourcedKeys.has(key) || settingsEnvSourcedKeys.has(key)) {
+    return true;
+  }
+  // Case-INSENSITIVELY on Windows, where env lookup is: a `.env` committed as
+  // `docker_host=…` writes that spelling into the tracking set and reaches the
+  // child exactly as `DOCKER_HOST` would, so an exact-case membership test
+  // answers "not from a file" about a value that is. `config/shared-env-keys.ts`
+  // folds case for the same reason, and this file's own callers ask a security
+  // question rather than a bookkeeping one.
+  if (process.platform !== 'win32') return false;
+  const lower = key.toLowerCase();
+  for (const tracked of dotEnvSourcedKeys) {
+    if (tracked.toLowerCase() === lower) return true;
+  }
+  for (const tracked of settingsEnvSourcedKeys) {
+    if (tracked.toLowerCase() === lower) return true;
+  }
+  return false;
+}
+
 /**
  * Collects environment variables from user-level `.env` files and returns
  * them as a plain dictionary **without** mutating `process.env`.

packages/cli/src/config/settingsSchema.ts

@@ -727,6 +727,21 @@ const SETTINGS_SCHEMA = {
           'Append the attribution footer naming the model and CLI version (e.g. "_— qwen3-coder via Qwen Code /review (v0.21.2)_") to review bodies and inline comments posted to GitHub. Disable to post reviews without VISIBLE AI attribution: no footer, and no "**[Critical]**"/"**[Suggestion]**" severity markers on posted comments and body lists. Unattributed posts stay identifiable in the raw source: each posted comment carries an invisible severity marker ("<!-- qwen-review critical -->") and the review body carries a ledger marker ("<!-- qwen-review-ledger ... -->") — anything reading comment bodies (GitHub API automation, the workflows this setting couples to) still recognizes a /review artifact, and presubmit duplicate detection recognizes the reviewing account\'s earlier posts by the severity marker, though unattributed posts from other accounts escape it. Another consequence: qwen-autofix\'s Critical-only mode (engaged after round 5, or earlier when a counting window\'s diff-growth budget trips) no longer recognizes the posted findings as Critical and defers them. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot set review policy for its reviewers.',
         showInDialog: true,
       },
+      sandbox: {
+        type: 'enum',
+        label: 'Sandbox the reviewed code: review',
+        category: 'General',
+        requiresRestart: false,
+        default: 'off',
+        description:
+          'Run the REVIEWED repository\'s own commands — `npm ci` with its install scripts, the build, the test suite, and every mutation probe — inside a container instead of directly as you. A review executes the code it is reviewing, and today those commands inherit the review process\'s whole environment (on CI that includes the model and GitHub credentials). "auto" uses a container when docker or podman answers and runs directly when neither does; "required" refuses to run them unsandboxed, which makes the evidence that depends on execution (build/test findings, mutation verdicts, `Source: [probe]`) unavailable for that run rather than ending the review; "off" is today\'s behaviour and stays the default, because containerising a build by surprise changes what native modules compile against. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot switch off the containment that exists to contain it.',
+        showInDialog: true,
+        options: [
+          { value: 'off', label: 'Off (run the reviewed code directly)' },
+          { value: 'auto', label: 'Auto (container when one is available)' },
+          { value: 'required', label: 'Required (never run it unsandboxed)' },
+        ],
+      },
       effort: {
         type: 'enum',
         label: 'Default effort: review',

packages/cli/test-setup.ts

@@ -19,6 +19,16 @@ if (process.env['QWEN_SERVE_NO_PERSISTENT_REGISTRATION'] === undefined) {
   process.env['QWEN_SERVE_NO_PERSISTENT_REGISTRATION'] = '1';
 }
 
+// The review sandbox policy is the OPERATOR's setting for their own reviews,
+// and this suite must not inherit it. A maintainer who turns the feature on
+// and then runs `npm test` would otherwise watch the review tests refuse to
+// run — 101 of them, measured — because the phase gates correctly do what the
+// setting says. Deleting rather than pinning to a value, so `sandboxPolicy`'s
+// "strictest of environment and settings" rule is left alone and a test that
+// wants a policy still stubs one.
+delete process.env['QWEN_REVIEW_SANDBOX'];
+delete process.env['SANDBOX_SET_UID_GID'];
+
 import './src/test-utils/customMatchers.js';
 
 // Lowlight is loaded asynchronously in production to keep it out of the

packages/vscode-ide-companion/schemas/settings.schema.json

@@ -231,6 +231,15 @@
           "type": "boolean",
           "default": true
         },
+        "sandbox": {
+          "description": "Run the REVIEWED repository's own commands — `npm ci` with its install scripts, the build, the test suite, and every mutation probe — inside a container instead of directly as you. A review executes the code it is reviewing, and today those commands inherit the review process's whole environment (on CI that includes the model and GitHub credentials). \"auto\" uses a container when docker or podman answers and runs directly when neither does; \"required\" refuses to run them unsandboxed, which makes the evidence that depends on execution (build/test findings, mutation verdicts, `Source: [probe]`) unavailable for that run rather than ending the review; \"off\" is today's behaviour and stays the default, because containerising a build by surprise changes what native modules compile against. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot switch off the containment that exists to contain it. Options: off, auto, required",
+          "enum": [
+            "off",
+            "auto",
+            "required"
+          ],
+          "default": "off"
+        },
         "effort": {
           "description": "Default effort for /review when --effort is not given. \"auto\" keeps the built-in rule (high for PRs, medium for local changes). An explicit --effort still wins; an effective --comment still forces high and --fix still floors at medium. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot set review policy for its reviewers. Options: auto, low, medium, high",
           "enum": [