From a1897c1600321637c66bec1d8ef094e8597e3e7f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 15:47:17 +0000 Subject: [PATCH 1/6] test: serialize client-metro subprocess-stub test to end contention flakes src/__tests__/client-metro.test.ts stubs npx and the package managers on PATH and spawns a real Metro dev server per case, so each case waits real subprocess time (~570-910ms measured). Run in the unit-core project at ~7x file parallelism it contends for CPU with every other stub-spawning file; a starved spawn pushes production down a generic failure path that returns a different error than the assertion expects. This is the same contention flake #1362 serialized runtime-hints.test.ts and apple/core/index.test.ts for, but this file was not included in that batch (observed failing a full test:unit run while passing 20/20 in isolation and green on a re-run). Add it to SUBPROCESS_STUB_TESTS so it joins the fileParallelism:false, maxWorkers:1 subprocess-stub project, so at most one real-stub-spawning file spawns at a time. Injecting the spawn budget is not the lever here: the fake dev server becomes ready fast (no waited-out timeout to inject), the per-case cost is a genuine subprocess spawn, and each case is already under the 2.5s slow-test budget so the gate never flags it. Removing the cost would mean mocking the very spawn/args/package-manager-detection path the file exists to verify, and an exec-options DI seam is forbidden by the CI gate (AGENTS.md). Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK --- vitest.config.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/vitest.config.ts b/vitest.config.ts index d2fd6638c..42cfd2841 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,7 +1,7 @@ import { defineConfig } from 'vitest/config'; import slowTestGateReporter from './scripts/vitest-slow-test-reporter.ts'; -// Tests that stub a real binary (adb/xcrun) by mutating process.env.PATH and +// Tests that stub a real binary (adb/xcrun/npx) by mutating process.env.PATH and // then spawn it, so each case waits real subprocess/retry/poll time. Run at the // unit suite's default ~7x file parallelism they contend for CPU and their stub // spawns get starved past an internal budget, so production takes a generic @@ -14,6 +14,8 @@ const SUBPROCESS_STUB_TESTS = [ 'src/platforms/android/__tests__/{app-lifecycle-install,app-lifecycle-open,device-input-state,input-actions,notifications,settings}.test.ts', 'src/daemon/__tests__/runtime-hints.test.ts', 'src/platforms/apple/core/__tests__/index.test.ts', + // Stubs npx + the package managers on PATH and spawns a real Metro dev server per case. + 'src/__tests__/client-metro.test.ts', ]; export default defineConfig({ @@ -44,7 +46,7 @@ export default defineConfig({ }, }, { - // The subprocess-stub tests stub adb/xcrun by mutating process.env + // The subprocess-stub tests stub adb/xcrun/npx by mutating process.env // (PATH, AGENT_DEVICE_TEST_ARGS_FILE) and wait real subprocess/retry/poll // time, so the group runs serialized with per-file isolation — the same // execution contract the pre-split android index.test.ts aggregation From c2be4230ca44cc2a39cae8e52233f97c4d0f0b15 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 16:32:04 +0000 Subject: [PATCH 2/6] test: make the vitest suite hermetic against ambient daemon env MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A machine actually running agent-device — including this repo's own remote dev containers — exports AGENT_DEVICE_DAEMON_BASE_URL and AGENT_DEVICE_DAEMON_AUTH_TOKEN pointing at a live daemon. Production flag-default resolution folds those into every command's input and connection config (resolveConfigBackedFlagDefaults -> readEnvFlagDefaults, plus the daemon client's own env fallbacks in daemon-client-lifecycle), so a configured host silently diverges from CI, which runs with them unset: - command-tools and cloud-connect-profile assert exact command input / profile shapes and gain phantom daemonBaseUrl/daemonAuthToken keys. - daemon-client and daemon-client-lifecycle take the remote-daemon path ("Remote daemon is unavailable") instead of the local one they exercise. 28 tests across those four files fail deterministically on such a host — fast, in isolation, not contention — while passing on CI. Add a shared setup file that deletes the two ambient daemon-connection vars before each test file, wired into every vitest project alongside the existing process-memo reset, so a configured host matches CI. Tests that need these set assign their own value or pass an explicit env object, which runs after this setup module loads and is therefore unaffected. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK --- src/__tests__/hermetic-env-setup.ts | 23 +++++++++++++++++++++++ vitest.config.ts | 25 ++++++++++++++++++++----- 2 files changed, 43 insertions(+), 5 deletions(-) create mode 100644 src/__tests__/hermetic-env-setup.ts diff --git a/src/__tests__/hermetic-env-setup.ts b/src/__tests__/hermetic-env-setup.ts new file mode 100644 index 000000000..654603849 --- /dev/null +++ b/src/__tests__/hermetic-env-setup.ts @@ -0,0 +1,23 @@ +// Unit tests must be hermetic with respect to the host's daemon-connection +// environment. A machine actually running agent-device — including this repo's +// own remote dev containers — exports AGENT_DEVICE_DAEMON_BASE_URL and +// AGENT_DEVICE_DAEMON_AUTH_TOKEN pointing at a live daemon. Production +// flag-default resolution folds those into every command's input and connection +// config (resolveConfigBackedFlagDefaults -> readEnvFlagDefaults, and the daemon +// client's own env fallbacks), so a configured host silently diverges from CI: +// tests that assert an exact command input/config shape gain phantom +// daemonBaseUrl/daemonAuthToken keys, and daemon-client tests take the remote +// path ("Remote daemon is unavailable") instead of the local one they exercise. +// +// CI runs with these unset. Delete them here so a configured host matches CI. +// Tests that genuinely need them assign their own value or pass an explicit env +// object; that happens inside the test, after this module has loaded, so this +// scrub does not interfere. +const AMBIENT_DAEMON_ENV_VARS = [ + 'AGENT_DEVICE_DAEMON_BASE_URL', + 'AGENT_DEVICE_DAEMON_AUTH_TOKEN', +] as const; + +for (const name of AMBIENT_DAEMON_ENV_VARS) { + delete process.env[name]; +} diff --git a/vitest.config.ts b/vitest.config.ts index 42cfd2841..bc9c79d73 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -42,7 +42,10 @@ export default defineConfig({ // job (scripts/maestro-conformance), like the layering guard. ], exclude: SUBPROCESS_STUB_TESTS, - setupFiles: ['src/__tests__/process-memo-setup.ts'], + setupFiles: [ + 'src/__tests__/hermetic-env-setup.ts', + 'src/__tests__/process-memo-setup.ts', + ], }, }, { @@ -54,7 +57,10 @@ export default defineConfig({ test: { name: 'subprocess-stub', include: SUBPROCESS_STUB_TESTS, - setupFiles: ['src/__tests__/process-memo-setup.ts'], + setupFiles: [ + 'src/__tests__/hermetic-env-setup.ts', + 'src/__tests__/process-memo-setup.ts', + ], fileParallelism: false, isolate: true, maxWorkers: 1, @@ -64,21 +70,30 @@ export default defineConfig({ test: { name: 'provider-integration', include: ['test/integration/provider-scenarios/**/*.test.ts'], - setupFiles: ['src/__tests__/process-memo-setup.ts'], + setupFiles: [ + 'src/__tests__/hermetic-env-setup.ts', + 'src/__tests__/process-memo-setup.ts', + ], }, }, { test: { name: 'interaction-contract', include: ['test/integration/interaction-contract/**/*.test.ts'], - setupFiles: ['src/__tests__/process-memo-setup.ts'], + setupFiles: [ + 'src/__tests__/hermetic-env-setup.ts', + 'src/__tests__/process-memo-setup.ts', + ], }, }, { test: { name: 'output-economy', include: ['test/output-economy/**/*.test.ts'], - setupFiles: ['src/__tests__/process-memo-setup.ts'], + setupFiles: [ + 'src/__tests__/hermetic-env-setup.ts', + 'src/__tests__/process-memo-setup.ts', + ], }, }, ], From 8b7a72935e3a4ca105d978724162d3021052b758 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 17:52:19 +0000 Subject: [PATCH 3/6] test: add a regression guard for the hermetic-env setup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI runs with AGENT_DEVICE_DAEMON_* unset, so it can never exercise the scrub that hermetic-env-setup.ts performs — deleting the setup file or forgetting to wire a project would leave the whole suite green. Add a guard that closes that gap two ways: - A static check that every configured vitest project lists the setup file in its setupFiles (catches an unwired project). - A real vitest child, launched with both daemon vars set, running a probe fixture that proves a wired project sees them scrubbed — plus a negative control (same vars, setup disabled) proving the probe actually detects the leak, so a green result is the setup working, not a no-op. A control var the setup must not touch proves the child inherited the injected environment. The two children run concurrently to stay within the unit wall-clock budget, and the file joins the serialized subprocess-stub project since it spawns real vitest processes. Ignore the fixture config's default export in fallow — vitest loads it by path rather than importing it, the same class as the other tool-config default exports already listed there. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK --- .fallowrc.json | 2 +- .../hermetic-env-guard/hermetic-env-probe.ts | 13 +++ .../hermetic-env-guard/vitest.config.ts | 21 +++++ src/__tests__/hermetic-env-guard.test.ts | 84 +++++++++++++++++++ vitest.config.ts | 2 + 5 files changed, 121 insertions(+), 1 deletion(-) create mode 100644 src/__tests__/__fixtures__/hermetic-env-guard/hermetic-env-probe.ts create mode 100644 src/__tests__/__fixtures__/hermetic-env-guard/vitest.config.ts create mode 100644 src/__tests__/hermetic-env-guard.test.ts diff --git a/.fallowrc.json b/.fallowrc.json index c1961e772..813d1a734 100644 --- a/.fallowrc.json +++ b/.fallowrc.json @@ -65,7 +65,7 @@ }, { "comment": "Tool config default exports, loaded by the tool rather than imported.", - "file": "{tsdown.config.ts,website/rspress.config.ts,test/skillgym/skillgym.config.ts,test/skillgym/suites/*.ts}", + "file": "{tsdown.config.ts,website/rspress.config.ts,test/skillgym/skillgym.config.ts,test/skillgym/suites/*.ts,src/__tests__/__fixtures__/hermetic-env-guard/vitest.config.ts}", "exports": [ "default" ] diff --git a/src/__tests__/__fixtures__/hermetic-env-guard/hermetic-env-probe.ts b/src/__tests__/__fixtures__/hermetic-env-guard/hermetic-env-probe.ts new file mode 100644 index 000000000..59df83140 --- /dev/null +++ b/src/__tests__/__fixtures__/hermetic-env-guard/hermetic-env-probe.ts @@ -0,0 +1,13 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; + +// Run only by hermetic-env-guard.test.ts, which spawns vitest against this +// fixture with the daemon vars + a control var set in the child environment. +test('the wired setup scrubs ambient daemon env before the test runs', () => { + // Control var the setup must NOT touch: proves the child truly inherited the + // injected environment, so the two absences below are scrubbing — not a child + // that never received the vars in the first place. + assert.equal(process.env.HERMETIC_GUARD_CONTROL, 'present'); + assert.equal(process.env.AGENT_DEVICE_DAEMON_BASE_URL, undefined); + assert.equal(process.env.AGENT_DEVICE_DAEMON_AUTH_TOKEN, undefined); +}); diff --git a/src/__tests__/__fixtures__/hermetic-env-guard/vitest.config.ts b/src/__tests__/__fixtures__/hermetic-env-guard/vitest.config.ts new file mode 100644 index 000000000..53e84abdd --- /dev/null +++ b/src/__tests__/__fixtures__/hermetic-env-guard/vitest.config.ts @@ -0,0 +1,21 @@ +import { fileURLToPath } from 'node:url'; +import path from 'node:path'; +import { defineConfig } from 'vitest/config'; + +const here = path.dirname(fileURLToPath(import.meta.url)); + +// A minimal standalone project rooted at this fixture dir. It wires the REAL +// hermetic-env setup file (so deleting or breaking that file fails the guard), +// unless HERMETIC_GUARD_DISABLE_SETUP=1 — the negative control that proves the +// probe is actually sensitive to the ambient vars. Driven by +// hermetic-env-guard.test.ts. +export default defineConfig({ + test: { + root: here, + include: ['hermetic-env-probe.ts'], + setupFiles: + process.env.HERMETIC_GUARD_DISABLE_SETUP === '1' + ? [] + : [path.resolve(here, '../../hermetic-env-setup.ts')], + }, +}); diff --git a/src/__tests__/hermetic-env-guard.test.ts b/src/__tests__/hermetic-env-guard.test.ts new file mode 100644 index 000000000..2ee97469c --- /dev/null +++ b/src/__tests__/hermetic-env-guard.test.ts @@ -0,0 +1,84 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import { spawn } from 'node:child_process'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import vitestConfig from '../../vitest.config.ts'; + +// hermetic-env-setup.ts scrubs ambient AGENT_DEVICE_DAEMON_* vars so a host that +// actually runs agent-device matches CI, where they are unset. CI can never +// exercise that scrub — it starts clean — so without this guard, deleting the +// setup file or forgetting to wire a project would leave the whole suite green. +// This test closes that gap two ways: a static check that every project wires +// the setup, and a real vitest child, launched with the vars set, that proves a +// wired project sees them scrubbed (with a negative control proving the probe is +// sensitive to the vars in the first place). + +const HERMETIC_ENV_SETUP = 'src/__tests__/hermetic-env-setup.ts'; +const here = path.dirname(fileURLToPath(import.meta.url)); +const repoRoot = path.resolve(here, '../..'); +const fixtureConfig = path.join(here, '__fixtures__', 'hermetic-env-guard', 'vitest.config.ts'); + +function runProbe( + extraEnv: Record, +): Promise<{ status: number | null; output: string }> { + return new Promise((resolve, reject) => { + const child = spawn('npx', ['vitest', 'run', '--config', fixtureConfig], { + cwd: repoRoot, + env: { ...process.env, ...extraEnv }, + }); + let output = ''; + child.stdout.on('data', (chunk: Buffer) => (output += chunk.toString())); + child.stderr.on('data', (chunk: Buffer) => (output += chunk.toString())); + child.on('error', reject); + child.on('close', (status) => resolve({ status, output })); + }); +} + +type ProjectShape = { test?: { name?: string; setupFiles?: readonly string[] } }; + +test('every vitest project wires the hermetic-env setup', () => { + const projects = (vitestConfig.test?.projects ?? []) as unknown as ReadonlyArray; + assert.ok(projects.length > 0, 'expected configured vitest projects'); + for (const project of projects) { + const name = project.test?.name ?? '(unnamed)'; + const setupFiles = project.test?.setupFiles ?? []; + assert.ok( + setupFiles.includes(HERMETIC_ENV_SETUP), + `project "${name}" must wire ${HERMETIC_ENV_SETUP} in setupFiles`, + ); + } +}); + +test('a wired vitest child scrubs ambient daemon env before tests run', async () => { + // Both daemon vars set, plus a control var the setup must not touch — its + // presence in the probe proves the child inherited the injected environment, + // so a scrubbed daemon var is the setup working, not an env that never arrived. + const dirtyEnv = { + AGENT_DEVICE_DAEMON_BASE_URL: 'https://guard.example.test', + AGENT_DEVICE_DAEMON_AUTH_TOKEN: 'hermetic-guard-token', + HERMETIC_GUARD_CONTROL: 'present', + }; + + // Run the wired probe and the unwired negative control concurrently: each is a + // real vitest cold start (~1.4s), so serial spawns would exceed the unit + // wall-clock budget for no reason — they are independent processes. + const [wired, unwired] = await Promise.all([ + runProbe(dirtyEnv), + runProbe({ ...dirtyEnv, HERMETIC_GUARD_DISABLE_SETUP: '1' }), + ]); + + assert.equal( + wired.status, + 0, + `expected the wired probe to pass with the vars scrubbed:\n${wired.output}`, + ); + + // Negative control: same vars, setup NOT wired — the probe must fail, proving + // it genuinely detects the leak (so the pass above is the setup, not a no-op). + assert.notEqual( + unwired.status, + 0, + `expected the probe to fail when the setup is not wired (leak must be detectable):\n${unwired.output}`, + ); +}); diff --git a/vitest.config.ts b/vitest.config.ts index bc9c79d73..86b14b7e1 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -16,6 +16,8 @@ const SUBPROCESS_STUB_TESTS = [ 'src/platforms/apple/core/__tests__/index.test.ts', // Stubs npx + the package managers on PATH and spawns a real Metro dev server per case. 'src/__tests__/client-metro.test.ts', + // Spawns a real vitest child process to prove the hermetic-env setup is wired. + 'src/__tests__/hermetic-env-guard.test.ts', ]; export default defineConfig({ From a1aba8b76351e2b5d79cdb0d2665009eb8493ded Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 18:08:00 +0000 Subject: [PATCH 4/6] fix(test): route the hermetic-env guard's vitest child through runCmd AGENTS.md hard rule: TypeScript process execution must go through src/utils/exec.ts, never raw spawn/spawnSync. The guard's runProbe spawned the vitest child with node:child_process spawn directly, bypassing the shared timeout, process-tree cleanup, normalized failure, and diagnostics behavior. Replace it with runCmd invoking the repo-local vitest CLI (node node_modules/vitest/vitest.mjs) with allowFailure: true and a bounded timeoutMs, asserting the returned exitCode. allowFailure surfaces the negative control's non-zero exit as data instead of throwing, so that assertion keeps working; the two probes still run concurrently to stay under the unit budget. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK --- src/__tests__/hermetic-env-guard.test.ts | 33 ++++++++++++------------ 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/src/__tests__/hermetic-env-guard.test.ts b/src/__tests__/hermetic-env-guard.test.ts index 2ee97469c..e45674bc5 100644 --- a/src/__tests__/hermetic-env-guard.test.ts +++ b/src/__tests__/hermetic-env-guard.test.ts @@ -1,8 +1,8 @@ import { test } from 'vitest'; import assert from 'node:assert/strict'; -import { spawn } from 'node:child_process'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; +import { runCmd } from '../utils/exec.ts'; import vitestConfig from '../../vitest.config.ts'; // hermetic-env-setup.ts scrubs ambient AGENT_DEVICE_DAEMON_* vars so a host that @@ -18,21 +18,22 @@ const HERMETIC_ENV_SETUP = 'src/__tests__/hermetic-env-setup.ts'; const here = path.dirname(fileURLToPath(import.meta.url)); const repoRoot = path.resolve(here, '../..'); const fixtureConfig = path.join(here, '__fixtures__', 'hermetic-env-guard', 'vitest.config.ts'); +// The repo-local vitest CLI, invoked through node so no PATH/npx lookup is involved. +const vitestCli = path.join(repoRoot, 'node_modules', 'vitest', 'vitest.mjs'); +const PROBE_TIMEOUT_MS = 60_000; -function runProbe( +async function runProbe( extraEnv: Record, -): Promise<{ status: number | null; output: string }> { - return new Promise((resolve, reject) => { - const child = spawn('npx', ['vitest', 'run', '--config', fixtureConfig], { - cwd: repoRoot, - env: { ...process.env, ...extraEnv }, - }); - let output = ''; - child.stdout.on('data', (chunk: Buffer) => (output += chunk.toString())); - child.stderr.on('data', (chunk: Buffer) => (output += chunk.toString())); - child.on('error', reject); - child.on('close', (status) => resolve({ status, output })); +): Promise<{ exitCode: number; output: string }> { + // AGENTS.md hard rule: TypeScript process execution goes through src/utils/exec.ts, + // not raw spawn. allowFailure keeps the negative control's non-zero exit as data. + const result = await runCmd(process.execPath, [vitestCli, 'run', '--config', fixtureConfig], { + cwd: repoRoot, + env: { ...process.env, ...extraEnv }, + allowFailure: true, + timeoutMs: PROBE_TIMEOUT_MS, }); + return { exitCode: result.exitCode, output: `${result.stdout}${result.stderr}` }; } type ProjectShape = { test?: { name?: string; setupFiles?: readonly string[] } }; @@ -61,7 +62,7 @@ test('a wired vitest child scrubs ambient daemon env before tests run', async () }; // Run the wired probe and the unwired negative control concurrently: each is a - // real vitest cold start (~1.4s), so serial spawns would exceed the unit + // real vitest cold start (~1.4s), so serial runs would exceed the unit // wall-clock budget for no reason — they are independent processes. const [wired, unwired] = await Promise.all([ runProbe(dirtyEnv), @@ -69,7 +70,7 @@ test('a wired vitest child scrubs ambient daemon env before tests run', async () ]); assert.equal( - wired.status, + wired.exitCode, 0, `expected the wired probe to pass with the vars scrubbed:\n${wired.output}`, ); @@ -77,7 +78,7 @@ test('a wired vitest child scrubs ambient daemon env before tests run', async () // Negative control: same vars, setup NOT wired — the probe must fail, proving // it genuinely detects the leak (so the pass above is the setup, not a no-op). assert.notEqual( - unwired.status, + unwired.exitCode, 0, `expected the probe to fail when the setup is not wired (leak must be detectable):\n${unwired.output}`, ); From 6a373137c758655ea81626eef2ead5b69a3a2a26 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 19:12:12 +0000 Subject: [PATCH 5/6] fix(test): make the hermetic-env guard's probe cleanup timely and complete Follow-up to review. Two process-tree cleanup gaps in the guard's runProbe: - It gave the nested vitest a 60s timeout while the enclosing test used vitest's default (5s) timeout, so a hung probe would time the parent test out first and leave the child running (orphaned). - runCmd only process-group-kills when detached:true; without it a timeout kills only the direct child, so vitest worker descendants could survive. Run the probe detached so a timeout kills the whole process group (the vitest child and its workers), and set the child timeout (20s) strictly below an explicit enclosing test timeout (40s) so a hang is reaped by runCmd before vitest abandons the parent test. The repo-local node/vitest invocation, allowFailure, exit-code assertions, and the concurrent positive/negative probes are unchanged. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK --- src/__tests__/hermetic-env-guard.test.ts | 73 ++++++++++++++---------- 1 file changed, 42 insertions(+), 31 deletions(-) diff --git a/src/__tests__/hermetic-env-guard.test.ts b/src/__tests__/hermetic-env-guard.test.ts index e45674bc5..cf5058ecf 100644 --- a/src/__tests__/hermetic-env-guard.test.ts +++ b/src/__tests__/hermetic-env-guard.test.ts @@ -20,17 +20,24 @@ const repoRoot = path.resolve(here, '../..'); const fixtureConfig = path.join(here, '__fixtures__', 'hermetic-env-guard', 'vitest.config.ts'); // The repo-local vitest CLI, invoked through node so no PATH/npx lookup is involved. const vitestCli = path.join(repoRoot, 'node_modules', 'vitest', 'vitest.mjs'); -const PROBE_TIMEOUT_MS = 60_000; +// The child (probe) timeout must fire strictly before the enclosing test timeout: a hung +// probe is then process-group-killed by runCmd (see detached below) and reaped here, +// rather than the parent test timing out first and leaving the vitest worker tree orphaned. +const PROBE_TIMEOUT_MS = 20_000; +const TEST_TIMEOUT_MS = PROBE_TIMEOUT_MS * 2; async function runProbe( extraEnv: Record, ): Promise<{ exitCode: number; output: string }> { // AGENTS.md hard rule: TypeScript process execution goes through src/utils/exec.ts, - // not raw spawn. allowFailure keeps the negative control's non-zero exit as data. + // not raw spawn. detached:true makes runCmd's timeout kill the whole process group + // (the vitest child *and* its worker descendants), not just the direct child; + // allowFailure keeps the negative control's non-zero exit as data rather than throwing. const result = await runCmd(process.execPath, [vitestCli, 'run', '--config', fixtureConfig], { cwd: repoRoot, env: { ...process.env, ...extraEnv }, allowFailure: true, + detached: true, timeoutMs: PROBE_TIMEOUT_MS, }); return { exitCode: result.exitCode, output: `${result.stdout}${result.stderr}` }; @@ -51,35 +58,39 @@ test('every vitest project wires the hermetic-env setup', () => { } }); -test('a wired vitest child scrubs ambient daemon env before tests run', async () => { - // Both daemon vars set, plus a control var the setup must not touch — its - // presence in the probe proves the child inherited the injected environment, - // so a scrubbed daemon var is the setup working, not an env that never arrived. - const dirtyEnv = { - AGENT_DEVICE_DAEMON_BASE_URL: 'https://guard.example.test', - AGENT_DEVICE_DAEMON_AUTH_TOKEN: 'hermetic-guard-token', - HERMETIC_GUARD_CONTROL: 'present', - }; +test( + 'a wired vitest child scrubs ambient daemon env before tests run', + async () => { + // Both daemon vars set, plus a control var the setup must not touch — its + // presence in the probe proves the child inherited the injected environment, + // so a scrubbed daemon var is the setup working, not an env that never arrived. + const dirtyEnv = { + AGENT_DEVICE_DAEMON_BASE_URL: 'https://guard.example.test', + AGENT_DEVICE_DAEMON_AUTH_TOKEN: 'hermetic-guard-token', + HERMETIC_GUARD_CONTROL: 'present', + }; - // Run the wired probe and the unwired negative control concurrently: each is a - // real vitest cold start (~1.4s), so serial runs would exceed the unit - // wall-clock budget for no reason — they are independent processes. - const [wired, unwired] = await Promise.all([ - runProbe(dirtyEnv), - runProbe({ ...dirtyEnv, HERMETIC_GUARD_DISABLE_SETUP: '1' }), - ]); + // Run the wired probe and the unwired negative control concurrently: each is a + // real vitest cold start (~1.4s), so serial runs would exceed the unit + // wall-clock budget for no reason — they are independent processes. + const [wired, unwired] = await Promise.all([ + runProbe(dirtyEnv), + runProbe({ ...dirtyEnv, HERMETIC_GUARD_DISABLE_SETUP: '1' }), + ]); - assert.equal( - wired.exitCode, - 0, - `expected the wired probe to pass with the vars scrubbed:\n${wired.output}`, - ); + assert.equal( + wired.exitCode, + 0, + `expected the wired probe to pass with the vars scrubbed:\n${wired.output}`, + ); - // Negative control: same vars, setup NOT wired — the probe must fail, proving - // it genuinely detects the leak (so the pass above is the setup, not a no-op). - assert.notEqual( - unwired.exitCode, - 0, - `expected the probe to fail when the setup is not wired (leak must be detectable):\n${unwired.output}`, - ); -}); + // Negative control: same vars, setup NOT wired — the probe must fail, proving + // it genuinely detects the leak (so the pass above is the setup, not a no-op). + assert.notEqual( + unwired.exitCode, + 0, + `expected the probe to fail when the setup is not wired (leak must be detectable):\n${unwired.output}`, + ); + }, + TEST_TIMEOUT_MS, +); From 32f5546dcf9b82e2885286d0299d954ae70121d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Jul 2026 05:47:35 +0000 Subject: [PATCH 6/6] test: replace the child-process hermetic-env guard with an in-process test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous guard spawned a real vitest child to exercise the setup through vitest's setupFiles machinery, which dragged in process-group cleanup, timeout ordering, a fixture config + probe, and a fallow suppression — a pile of scaffolding around a scrub that is two delete statements. Prove the same contract in-process instead: - Behavior: set the daemon vars, vi.resetModules(), re-import hermetic-env-setup, and assert they are gone. This exercises the real import-time scrub on any host (CI included, where the vars are otherwise absent) and fails if it regresses. - Wiring: assert every configured vitest project lists the setup in setupFiles. Runs in ~5ms in unit-core with no subprocess, so it also drops out of SUBPROCESS_STUB_TESTS and removes the fixtures and the fallow ignore entry. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK --- .fallowrc.json | 2 +- .../hermetic-env-guard/hermetic-env-probe.ts | 13 --- .../hermetic-env-guard/vitest.config.ts | 21 ---- src/__tests__/hermetic-env-guard.test.ts | 96 ------------------- src/__tests__/hermetic-env-setup.test.ts | 41 ++++++++ vitest.config.ts | 2 - 6 files changed, 42 insertions(+), 133 deletions(-) delete mode 100644 src/__tests__/__fixtures__/hermetic-env-guard/hermetic-env-probe.ts delete mode 100644 src/__tests__/__fixtures__/hermetic-env-guard/vitest.config.ts delete mode 100644 src/__tests__/hermetic-env-guard.test.ts create mode 100644 src/__tests__/hermetic-env-setup.test.ts diff --git a/.fallowrc.json b/.fallowrc.json index 813d1a734..c1961e772 100644 --- a/.fallowrc.json +++ b/.fallowrc.json @@ -65,7 +65,7 @@ }, { "comment": "Tool config default exports, loaded by the tool rather than imported.", - "file": "{tsdown.config.ts,website/rspress.config.ts,test/skillgym/skillgym.config.ts,test/skillgym/suites/*.ts,src/__tests__/__fixtures__/hermetic-env-guard/vitest.config.ts}", + "file": "{tsdown.config.ts,website/rspress.config.ts,test/skillgym/skillgym.config.ts,test/skillgym/suites/*.ts}", "exports": [ "default" ] diff --git a/src/__tests__/__fixtures__/hermetic-env-guard/hermetic-env-probe.ts b/src/__tests__/__fixtures__/hermetic-env-guard/hermetic-env-probe.ts deleted file mode 100644 index 59df83140..000000000 --- a/src/__tests__/__fixtures__/hermetic-env-guard/hermetic-env-probe.ts +++ /dev/null @@ -1,13 +0,0 @@ -import { test } from 'vitest'; -import assert from 'node:assert/strict'; - -// Run only by hermetic-env-guard.test.ts, which spawns vitest against this -// fixture with the daemon vars + a control var set in the child environment. -test('the wired setup scrubs ambient daemon env before the test runs', () => { - // Control var the setup must NOT touch: proves the child truly inherited the - // injected environment, so the two absences below are scrubbing — not a child - // that never received the vars in the first place. - assert.equal(process.env.HERMETIC_GUARD_CONTROL, 'present'); - assert.equal(process.env.AGENT_DEVICE_DAEMON_BASE_URL, undefined); - assert.equal(process.env.AGENT_DEVICE_DAEMON_AUTH_TOKEN, undefined); -}); diff --git a/src/__tests__/__fixtures__/hermetic-env-guard/vitest.config.ts b/src/__tests__/__fixtures__/hermetic-env-guard/vitest.config.ts deleted file mode 100644 index 53e84abdd..000000000 --- a/src/__tests__/__fixtures__/hermetic-env-guard/vitest.config.ts +++ /dev/null @@ -1,21 +0,0 @@ -import { fileURLToPath } from 'node:url'; -import path from 'node:path'; -import { defineConfig } from 'vitest/config'; - -const here = path.dirname(fileURLToPath(import.meta.url)); - -// A minimal standalone project rooted at this fixture dir. It wires the REAL -// hermetic-env setup file (so deleting or breaking that file fails the guard), -// unless HERMETIC_GUARD_DISABLE_SETUP=1 — the negative control that proves the -// probe is actually sensitive to the ambient vars. Driven by -// hermetic-env-guard.test.ts. -export default defineConfig({ - test: { - root: here, - include: ['hermetic-env-probe.ts'], - setupFiles: - process.env.HERMETIC_GUARD_DISABLE_SETUP === '1' - ? [] - : [path.resolve(here, '../../hermetic-env-setup.ts')], - }, -}); diff --git a/src/__tests__/hermetic-env-guard.test.ts b/src/__tests__/hermetic-env-guard.test.ts deleted file mode 100644 index cf5058ecf..000000000 --- a/src/__tests__/hermetic-env-guard.test.ts +++ /dev/null @@ -1,96 +0,0 @@ -import { test } from 'vitest'; -import assert from 'node:assert/strict'; -import path from 'node:path'; -import { fileURLToPath } from 'node:url'; -import { runCmd } from '../utils/exec.ts'; -import vitestConfig from '../../vitest.config.ts'; - -// hermetic-env-setup.ts scrubs ambient AGENT_DEVICE_DAEMON_* vars so a host that -// actually runs agent-device matches CI, where they are unset. CI can never -// exercise that scrub — it starts clean — so without this guard, deleting the -// setup file or forgetting to wire a project would leave the whole suite green. -// This test closes that gap two ways: a static check that every project wires -// the setup, and a real vitest child, launched with the vars set, that proves a -// wired project sees them scrubbed (with a negative control proving the probe is -// sensitive to the vars in the first place). - -const HERMETIC_ENV_SETUP = 'src/__tests__/hermetic-env-setup.ts'; -const here = path.dirname(fileURLToPath(import.meta.url)); -const repoRoot = path.resolve(here, '../..'); -const fixtureConfig = path.join(here, '__fixtures__', 'hermetic-env-guard', 'vitest.config.ts'); -// The repo-local vitest CLI, invoked through node so no PATH/npx lookup is involved. -const vitestCli = path.join(repoRoot, 'node_modules', 'vitest', 'vitest.mjs'); -// The child (probe) timeout must fire strictly before the enclosing test timeout: a hung -// probe is then process-group-killed by runCmd (see detached below) and reaped here, -// rather than the parent test timing out first and leaving the vitest worker tree orphaned. -const PROBE_TIMEOUT_MS = 20_000; -const TEST_TIMEOUT_MS = PROBE_TIMEOUT_MS * 2; - -async function runProbe( - extraEnv: Record, -): Promise<{ exitCode: number; output: string }> { - // AGENTS.md hard rule: TypeScript process execution goes through src/utils/exec.ts, - // not raw spawn. detached:true makes runCmd's timeout kill the whole process group - // (the vitest child *and* its worker descendants), not just the direct child; - // allowFailure keeps the negative control's non-zero exit as data rather than throwing. - const result = await runCmd(process.execPath, [vitestCli, 'run', '--config', fixtureConfig], { - cwd: repoRoot, - env: { ...process.env, ...extraEnv }, - allowFailure: true, - detached: true, - timeoutMs: PROBE_TIMEOUT_MS, - }); - return { exitCode: result.exitCode, output: `${result.stdout}${result.stderr}` }; -} - -type ProjectShape = { test?: { name?: string; setupFiles?: readonly string[] } }; - -test('every vitest project wires the hermetic-env setup', () => { - const projects = (vitestConfig.test?.projects ?? []) as unknown as ReadonlyArray; - assert.ok(projects.length > 0, 'expected configured vitest projects'); - for (const project of projects) { - const name = project.test?.name ?? '(unnamed)'; - const setupFiles = project.test?.setupFiles ?? []; - assert.ok( - setupFiles.includes(HERMETIC_ENV_SETUP), - `project "${name}" must wire ${HERMETIC_ENV_SETUP} in setupFiles`, - ); - } -}); - -test( - 'a wired vitest child scrubs ambient daemon env before tests run', - async () => { - // Both daemon vars set, plus a control var the setup must not touch — its - // presence in the probe proves the child inherited the injected environment, - // so a scrubbed daemon var is the setup working, not an env that never arrived. - const dirtyEnv = { - AGENT_DEVICE_DAEMON_BASE_URL: 'https://guard.example.test', - AGENT_DEVICE_DAEMON_AUTH_TOKEN: 'hermetic-guard-token', - HERMETIC_GUARD_CONTROL: 'present', - }; - - // Run the wired probe and the unwired negative control concurrently: each is a - // real vitest cold start (~1.4s), so serial runs would exceed the unit - // wall-clock budget for no reason — they are independent processes. - const [wired, unwired] = await Promise.all([ - runProbe(dirtyEnv), - runProbe({ ...dirtyEnv, HERMETIC_GUARD_DISABLE_SETUP: '1' }), - ]); - - assert.equal( - wired.exitCode, - 0, - `expected the wired probe to pass with the vars scrubbed:\n${wired.output}`, - ); - - // Negative control: same vars, setup NOT wired — the probe must fail, proving - // it genuinely detects the leak (so the pass above is the setup, not a no-op). - assert.notEqual( - unwired.exitCode, - 0, - `expected the probe to fail when the setup is not wired (leak must be detectable):\n${unwired.output}`, - ); - }, - TEST_TIMEOUT_MS, -); diff --git a/src/__tests__/hermetic-env-setup.test.ts b/src/__tests__/hermetic-env-setup.test.ts new file mode 100644 index 000000000..22ad6f8b4 --- /dev/null +++ b/src/__tests__/hermetic-env-setup.test.ts @@ -0,0 +1,41 @@ +import { afterEach, test, vi } from 'vitest'; +import assert from 'node:assert/strict'; +import vitestConfig from '../../vitest.config.ts'; + +const HERMETIC_ENV_SETUP = 'src/__tests__/hermetic-env-setup.ts'; +const AMBIENT_DAEMON_VARS = [ + 'AGENT_DEVICE_DAEMON_BASE_URL', + 'AGENT_DEVICE_DAEMON_AUTH_TOKEN', +] as const; + +type ProjectShape = { test?: { name?: string; setupFiles?: readonly string[] } }; + +// Wiring: the scrub only helps if every project loads it as a setup file. CI runs with the +// vars unset, so a dropped wiring is otherwise invisible — assert it structurally instead. +test('every vitest project wires the hermetic-env setup', () => { + const projects = (vitestConfig.test?.projects ?? []) as unknown as ReadonlyArray; + assert.ok(projects.length > 0, 'expected configured vitest projects'); + for (const project of projects) { + const name = project.test?.name ?? '(unnamed)'; + const setupFiles = project.test?.setupFiles ?? []; + assert.ok( + setupFiles.includes(HERMETIC_ENV_SETUP), + `project "${name}" must wire ${HERMETIC_ENV_SETUP} in setupFiles`, + ); + } +}); + +// Behavior: re-import a fresh copy of the setup module with the daemon vars set, so the scrub +// is exercised on any host (CI included, where the vars are otherwise absent). +afterEach(() => { + for (const name of AMBIENT_DAEMON_VARS) delete process.env[name]; +}); + +test('importing hermetic-env-setup scrubs the ambient daemon connection vars', async () => { + for (const name of AMBIENT_DAEMON_VARS) process.env[name] = 'leaked-from-host'; + vi.resetModules(); + await import('./hermetic-env-setup.ts'); + for (const name of AMBIENT_DAEMON_VARS) { + assert.equal(process.env[name], undefined, `${name} must be scrubbed when the setup loads`); + } +}); diff --git a/vitest.config.ts b/vitest.config.ts index 86b14b7e1..bc9c79d73 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -16,8 +16,6 @@ const SUBPROCESS_STUB_TESTS = [ 'src/platforms/apple/core/__tests__/index.test.ts', // Stubs npx + the package managers on PATH and spawns a real Metro dev server per case. 'src/__tests__/client-metro.test.ts', - // Spawns a real vitest child process to prove the hermetic-env setup is wired. - 'src/__tests__/hermetic-env-guard.test.ts', ]; export default defineConfig({