diff --git a/packages/cli/src/commands/review/build-test.test.ts b/packages/cli/src/commands/review/build-test.test.ts index 8299a633a3..d95b25a621 100644 --- a/packages/cli/src/commands/review/build-test.test.ts +++ b/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; + 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); + }); +}); diff --git a/packages/cli/src/commands/review/build-test.ts b/packages/cli/src/commands/review/build-test.ts index 51ccc6fb96..c953d8b8f6 100644 --- a/packages/cli/src/commands/review/build-test.ts +++ b/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); +} diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index d9e291d18e..20c69af275 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/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 — 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 --worktree --out /qwen-review-pr--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 --baseline --pr-worktree --out /qwen-review-pr--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 --worktree --out /qwen-review-pr--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 --baseline --pr-worktree --out /qwen-review-pr--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 :\`), 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. diff --git a/packages/cli/src/commands/review/lib/npm-toolchain.ts b/packages/cli/src/commands/review/lib/npm-toolchain.ts index 3f743310ef..c1b3805e78 100644 --- a/packages/cli/src/commands/review/lib/npm-toolchain.ts +++ b/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); diff --git a/packages/cli/src/commands/review/lib/review-settings.ts b/packages/cli/src/commands/review/lib/review-settings.ts index 94bada0d84..085ab4d179 100644 --- a/packages/cli/src/commands/review/lib/review-settings.ts +++ b/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 diff --git a/packages/cli/src/commands/review/lib/sandboxed-exec.test.ts b/packages/cli/src/commands/review/lib/sandboxed-exec.test.ts new file mode 100644 index 0000000000..54acca559a --- /dev/null +++ b/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 `/.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; + }) 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); + // `/.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 `/.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'); + }); +}); diff --git a/packages/cli/src/commands/review/lib/sandboxed-exec.ts b/packages/cli/src/commands/review/lib/sandboxed-exec.ts new file mode 100644 index 0000000000..33ebc339ad --- /dev/null +++ b/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 + * `/.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 `/.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 `/.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. + * + * ` 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; +} + +/** ` 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(); + +/** 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 `/.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 `/.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 (`/.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; +} diff --git a/packages/cli/src/commands/review/lib/test-utils.ts b/packages/cli/src/commands/review/lib/test-utils.ts index ecff6891d9..051e9d51e7 100644 --- a/packages/cli/src/commands/review/lib/test-utils.ts +++ b/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 }); diff --git a/packages/cli/src/commands/review/lib/toolchain.ts b/packages/cli/src/commands/review/lib/toolchain.ts index 15eb5ae3ef..f9aee11a42 100644 --- a/packages/cli/src/commands/review/lib/toolchain.ts +++ b/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 { diff --git a/packages/cli/src/commands/review/test-delta.test.ts b/packages/cli/src/commands/review/test-delta.test.ts index f272147f68..0a7ee4aabb 100644 --- a/packages/cli/src/commands/review/test-delta.test.ts +++ b/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, diff --git a/packages/cli/src/commands/review/test-delta.ts b/packages/cli/src/commands/review/test-delta.ts index 289609a4c7..af0e74877a 100644 --- a/packages/cli/src/commands/review/test-delta.ts +++ b/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( diff --git a/packages/cli/src/commands/review/test-efficacy.integration.test.ts b/packages/cli/src/commands/review/test-efficacy.integration.test.ts index 9066a351a8..815c156023 100644 --- a/packages/cli/src/commands/review/test-efficacy.integration.test.ts +++ b/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; +let reviewSettingsIsolation: ReturnType; 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', () => { diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 087f18e629..7b84d43245 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/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 { } }; - 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 diff --git a/packages/cli/src/config/environment.ts b/packages/cli/src/config/environment.ts index d17d3266b0..29a698b7f9 100644 --- a/packages/cli/src/config/environment.ts +++ b/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. `/.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`. diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index cb227a6540..cbd8e7bded 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/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 ("") and the review body carries a ledger marker ("") — 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', diff --git a/packages/cli/test-setup.ts b/packages/cli/test-setup.ts index f04babb595..a107ea98c4 100644 --- a/packages/cli/test-setup.ts +++ b/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 diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index 593e9d1016..bc3d0c3944 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/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": [