test: serialize client-metro and harden the unit suite against ambient daemon env

[thymikee]

Summary

Three test-infrastructure changes:

1. Serialize client-metro.test.ts (a1897c1) — it stubs npx/package managers on PATH and spawns a real Metro dev server per case: the same real-subprocess contention profile Generalize subprocess stub tests to support multiple platforms #1362 serialized runtime-hints/apple-core-index for, but it was missed in that batch. Moves it into the subprocess-stub project (fileParallelism:false, maxWorkers:1). Injecting a budget isn't the lever here — the fake server becomes ready fast (no waited-out timeout), each case is already under the 2.5s slow-test budget, and removing the spawn would delete the behavior under test; an exec-options DI seam is CI-forbidden.

2. Make the vitest suite hermetic against ambient daemon env (c2be423) — a machine actually running agent-device (including this repo's own remote dev containers) exports AGENT_DEVICE_DAEMON_BASE_URL/AGENT_DEVICE_DAEMON_AUTH_TOKEN. Production flag-default resolution folds those into command inputs and connection config (resolveConfigBackedFlagDefaults → readEnvFlagDefaults, plus the daemon client's own env fallbacks), so on a configured host command-tools, cloud-connect-profile, daemon-client, and daemon-client-lifecycle (28 tests) fail deterministically — fast, in isolation, not contention — while passing on CI, which runs with the vars unset. A shared src/__tests__/hermetic-env-setup.ts, wired into every project alongside the existing memo-reset, deletes those two vars before each test file so a configured host matches CI. No production behavior changes.

3. Regression test for the hermetic-env setup (32f5546) — CI starts clean, so it can't catch a regression of (2): deleting the setup or forgetting to wire a project would leave the suite green. hermetic-env-setup.test.ts proves both in-process, no subprocess:

   • Behavior — set the daemon vars, vi.resetModules(), re-import hermetic-env-setup.ts, assert they are scrubbed. This exercises the real import-time scrub on any host (CI included, where the vars are otherwise absent) and fails if it regresses (verified by neutering the setup locally).
   • Wiring — assert every configured vitest project lists the setup in setupFiles.

   An earlier revision spawned a real vitest child to exercise the setupFiles machinery end-to-end; that dragged in process-group cleanup, timeout ordering, a fixture config/probe, and a fallow suppression for a two-line scrub, so it was replaced with the in-process test above (~5ms in unit-core).

Validation

• pnpm test:unit green ×2 (490 files / 4338 tests, exit 0); on a configured host the pre-change suite failed 28 tests across the four files in (2).
• client-metro runs serialized and passes; the hermetic-env test runs in-process in ~5ms (unit lane, no slow-gate entry).
• format, lint, typecheck, fallow (diff scope), and layering all pass locally.

Device evidence: N/A — test-only, no production code touched.

https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	1.8 MB	1.8 MB	0 B
JS gzip	578.2 kB	578.2 kB	0 B
npm tarball	692.9 kB	692.9 kB	0 B
npm unpacked	2.4 MB	2.4 MB	0 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	26.2 ms	25.8 ms	-0.3 ms
CLI --help	56.0 ms	55.7 ms	-0.4 ms

Top changed chunks: no changes in the largest emitted chunks.

[thymikee]

Review of c2be4230: one blocking validation gap. Moving client-metro.test.ts is correct—it is excluded from unit-core, included exactly once in the serialized subprocess-stub project, and all 24 checks are green. However, this PR also adds an all-project policy that deletes ambient AGENT_DEVICE_DAEMON_BASE_URL and AGENT_DEVICE_DAEMON_AUTH_TOKEN. CI starts with those variables unset, so deleting hermetic-env-setup.ts or failing to wire a project would still leave the suite green. Add a regression that launches a configured Vitest project in a child process with both variables set and proves a representative affected test sees the intended CI-like local configuration. Also update the PR summary/validation—or split the change—so this second behavior is reviewed explicitly. Device evidence is not applicable. ready-for-human is withheld pending that regression and accurate scope description.

[thymikee]

Addressed in 8b7a729; scope now spelled out in the PR description.

Regression (src/__tests__/hermetic-env-guard.test.ts) closes the "CI starts clean, so the guard can silently rot" gap two ways:

• Static — asserts every configured vitest project lists src/__tests__/hermetic-env-setup.ts in setupFiles. Catches an unwired project.
• Dynamic — launches a real vitest child with both AGENT_DEVICE_DAEMON_* set, against a probe fixture, and asserts the wired project sees them scrubbed. A negative control (same vars, setup disabled via a fixture toggle) must fail — proving the probe actually detects the leak rather than passing vacuously — and a control var the setup must not touch proves the child inherited the injected env. It joins the serialized subprocess-stub project since it spawns real vitest processes; the two children run concurrently to stay under the unit wall-clock budget (~1s, no slow-gate warning).

Scope — the summary now describes all three commits explicitly (serialize client-metro; ambient-daemon-env hermeticity; this regression). Kept on one branch rather than split, per the note to push follow-ups here — happy to split into a separate PR if you'd prefer the env work reviewed on its own.

---

Generated by Claude Code

[thymikee]

Re-review of 8b7a729: the prior validation/scope finding is resolved. The static wiring assertion plus wired child, setup-disabled negative control, and inherited control variable make the hermetic-env regression non-vacuous and revert-sensitive; all 24 checks are green. One new blocking finding remains: hermetic-env-guard.test.ts imports raw spawn from node:child_process. AGENTS.md explicitly requires all TypeScript process execution to use src/utils/exec.ts, and this implementation also bypasses the shared timeout, process-tree cleanup, normalized failure, and diagnostics behavior. Replace runProbe with runCmd using the repository-local Vitest invocation, allowFailure: true, and a bounded timeoutMs; combine stdout/stderr and assert the returned exitCode for the negative control. Preserve the current concurrent probes. Device evidence is N/A. ready-for-human remains withheld until this hard-rule breach is fixed.

[thymikee]

Fixed in a1aba8b. runProbe no longer imports raw spawn — it now goes through runCmd from src/utils/exec.ts, invoking the repo-local vitest CLI (node node_modules/vitest/vitest.mjs run --config <fixture>) with allowFailure: true and a bounded timeoutMs (60s ceiling; actual ~1.2s). It reads the returned exitCode and asserts it (0 for the wired probe, non-zero for the setup-disabled negative control — allowFailure surfaces that exit as data instead of throwing), and combines stdout/stderr into the failure message. The two probes still run concurrently via Promise.all.

Local: guard passes at ~0.7–1.2s (under the unit budget, no slow-gate entry); pnpm test:unit green (490 files / 4338 tests); lint, typecheck, and fallow clean.

---

Generated by Claude Code

[thymikee]

Blocking finding — the new hermetic-env guard still cannot guarantee timely process-tree cleanup. runProbe gives nested Vitest 60s but the enclosing test uses Vitest’s much shorter default timeout, so a hang fails the parent first while the child can keep running. Also, runCmd only kills the full process group when detached: true; without it, Vitest worker descendants may survive. Please run the probe detached, give it a child timeout shorter than an explicitly configured enclosing test timeout, and keep the current local Node/Vitest invocation, allowFailure, exit-code assertions, and concurrent positive/negative probes. All checks are green, but this exact head is not ready until that cleanup contract is fixed.

[thymikee]

Fixed in 6a37313. Both cleanup gaps addressed:

• Process-group kill — runProbe now passes detached: true, so runCmd's timeout path calls killProcessTree with the detached flag → process.kill(-pid, 'SIGKILL'), taking down the vitest child and its worker descendants rather than just the direct child.
• Timeout ordering — the child (probe) timeout is now PROBE_TIMEOUT_MS = 20s, and the enclosing test has an explicit timeout TEST_TIMEOUT_MS = 40s (passed as the third test() arg). The child timeout fires strictly first, so a hung probe is group-killed and reaped by runCmd before vitest abandons the parent test and orphans the tree.

Unchanged per your note: repo-local node node_modules/vitest/vitest.mjs invocation, allowFailure: true, exit-code assertions, and the concurrent positive/negative probes.

Local: guard passes ~0.7s (under budget, no slow-gate entry); pnpm test:unit green (490 files / 4338 tests); typecheck, lint, and fallow clean.

---

Generated by Claude Code

[thymikee]

Re-review clean at 6a37313. The probe now uses runCmd with detached: true, a 20s child timeout, and an explicit 40s enclosing test timeout, so a hung nested Vitest tree is killed and reaped before the parent test can time out. The local Node/Vitest invocation, concurrent positive/negative controls, allowFailure, and exit-code assertions remain intact. All 24 exact-head checks are green. No separately authorized cross-vendor review was performed.

[thymikee]

Re-review clean at 32f5546. The follow-up replaces the child-Vitest guard with an in-process test that seeds both daemon variables, resets modules, imports the real setup, and proves both values are deleted, while retaining the structural assertion that every Vitest project wires the setup. Obsolete fixtures and fallow suppression are removed. All 24 exact-head checks pass; existing ready-for-human remains appropriate. No separately authorized cross-vendor review was performed.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-23 07:56 UTC