fix(ios): stop shipping runner unit tests in the npm package - #1594
Merged
Conversation
The Apple runner ships as source in dist/apple/runner, and packaging strips #if AGENT_DEVICE_RUNNER_UNIT_TESTS blocks — but tests outside such blocks shipped whole and compiled on every user's machine. Two files leaked six tests this way (RunnerTests+LifecycleCacheTests, RunnerTests+SnapshotTraversalIdentityTests). Wrap the strays and close the class: packaging now fails if any XCTest-shaped method (func test*) survives stripping, with testCommand in RunnerTests.swift as the only allowlisted entrypoint.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
Member
Author
|
Sentinel re-review at |
thymikee
added
the
ready-for-human
|
thymikee
added a commit
that referenced
this pull request
Aug 4, 2026
Rebasing onto main pulled in #1594's two new test cases in this file, added independently of this branch's migration, still using raw fs.mkdtempSync(path.join(os.tmpdir(), ...)). Git's line-based merge found no textual conflict with this branch's removal of the os import (the changes touch non-overlapping regions), so it silently produced a file that doesn't typecheck. Migrated both to mkdtempForTestSync for consistency with the rest of the file, caught by CI's Typecheck, Fallow Code Quality, and FreeRange checks re-running against the pushed commit.
This was referenced Aug 4, 2026
thymikee
added a commit
that referenced
this pull request
Aug 5, 2026
Rebasing onto main pulled in #1594's two new test cases in this file, added independently of this branch's migration, still using raw fs.mkdtempSync(path.join(os.tmpdir(), ...)). Git's line-based merge found no textual conflict with this branch's removal of the os import (the changes touch non-overlapping regions), so it silently produced a file that doesn't typecheck. Migrated both to mkdtempForTestSync for consistency with the rest of the file, caught by CI's Typecheck, Fallow Code Quality, and FreeRange checks re-running against the pushed commit.
thymikee
added a commit
that referenced
this pull request
Aug 5, 2026
* fix: stop the unit suite from leaking temp directories ~650 test call sites across the unit suite create scratch directories via fs.mkdtemp(path.join(os.tmpdir(), ...)) or shared factories (makeSessionStore) with no cleanup, ever. Over time this accumulated 1.16M+ orphaned directories in the real system tmpdir, slow enough to make tools that enumerate $TMPDIR at startup (e.g. opencode) take 1-2 minutes to launch. Rather than migrate every call site, redirect os.tmpdir() itself for the lifetime of the whole `vitest run` invocation: scripts/vitest-tmpdir-global-setup.ts wires in as vitest's globalSetup/globalTeardown, points TMPDIR at one /tmp-rooted directory (verified: env mutations here propagate to every forked worker, confirmed empirically), and removes it in one recursive rm after every worker across every project finishes. Since os.tmpdir() reads TMPDIR on every call, this covers all ~650 call sites without touching any of them. Rooted at /tmp rather than nested inside the current (already deep, on macOS) os.tmpdir(): that broke real AF_UNIX socket tests (runner-usbmux.test.ts) by pushing socket paths past the 104-byte sun_path limit. A per-file afterAll hook was tried first but proved unreliable — 5 of 7 workers in one run never ran it before their process was torn down; the global setup/teardown pair (one process, confirmed single execution) is the mechanism that's actually guaranteed to run once. Also adds: - scripts/check-tmpdir-leaks.ts: CI/local guard asserting no agent-device-test-run-* directory survives a run (a leftover one means a worker was killed before cleanup could run). - src/__tests__/test-utils/tmp-dir.ts: documented mkdtempForTest / mkdtempForTestSync helpers, the discoverable way to get a scratch dir going forward (mirrors the src/utils/exec.ts pattern for node:child_process). - scripts/check-test-tmpdir-helper.ts: ratchet guard capping raw fs.mkdtemp/mkdtempSync call sites in test files at today's count (632); it can only shrink as call sites migrate to the helper. * fix: make check-tmpdir-leaks scan the same root the fix actually uses check-tmpdir-leaks.ts was scanning os.tmpdir() for leftover run directories, but vitest-tmpdir-global-setup.ts creates them under a hard-coded /tmp. On macOS those are different paths (TMPDIR is a deep per-user /var/folders/.../T/ directory) — the guard could never find a leak on the exact platform the original leak happened on, only on Linux CI where os.tmpdir() already is /tmp. Export TEST_RUN_TMP_ROOT and TEST_RUN_TMP_PREFIX from the global-setup module and import them in the leak check instead of recomputing a path that can drift. Switched from a fixed pid-based directory name to fs.mkdtempSync so a same-named leftover from a prior killed run (or, on a shared machine, another user) can't collide with a live run. Also fixes two stale comments (in this file and ci.yml) that still described the per-file afterAll hook design that was abandoned in favor of the global setup/teardown pair, and notes the check only covers vitest runs, not the node --test lanes (test:smoke, test:integration:node). Verified live: with the old code the guard reported no leaks even with a real orphaned /tmp/agent-device-test-run-* directory present (left by a command that got killed mid-run); with this fix it correctly found and reported it. * simplify: drop the tmpdir ratchet guard, keep the leak check local-only Two guards were more than this needed: - check:test-tmpdir-helper (ratchet on raw fs.mkdtemp call counts) protects nothing a bug could actually trigger — the leak is already fixed architecturally regardless of call-site count, so this was pure style/discoverability nudging. Dropped the script and its check:tooling/CI wiring; kept mkdtempForTest/mkdtempForTestSync in tmp-dir.ts as the documented option without enforcing it. - check:tmpdir-leaks in CI added little: GitHub-hosted runners are destroyed after each job, so a leftover directory there is harmless by construction, and a worker getting killed mid-run would already surface as a job failure some other way. Its real value is local, on the long-lived dev machines where the original leak actually accumulated — kept it wired into check:unit, dropped the CI step. * fix: don't flag a concurrent vitest run's tmpdir as a leak check-tmpdir-leaks.ts reported every agent-device-test-run-* directory as a leak, but a concurrent vitest run in another worktree legitimately keeps its own directory present until its own teardown finishes. On a machine that regularly runs several worktrees at once, that made check:unit fail on unrelated in-progress work. Embed the owning process's pid in the directory name (still random- suffixed via mkdtempSync, so same-pid reuse across separate runs can't collide) and have the leak check skip any directory whose pid is still alive (process.kill(pid, 0)) — only directories whose owning process already exited without running its globalTeardown are real leaks. Split the pure logic into check-tmpdir-leaks-model.ts (findLeakedRunDirectories, with an injectable liveness check for testing) so it has a real regression suite, including the concurrent-run case, instead of only being exercised by hand. * refactor: migrate raw fs.mkdtemp call sites to mkdtempForTest(Sync) Migrates 629 raw fs.mkdtemp(Sync)(path.join(os.tmpdir(), PREFIX)) call sites across 168 test files to the mkdtempForTest / mkdtempForTestSync helpers (src/__tests__/test-utils/tmp-dir.ts), so there's one documented, discoverable way to get a scratch dir in a test — cleanup already didn't depend on the call-site shape (the global TMPDIR redirect covers any of them), this is purely for consistency and discoverability, same reasoning as src/utils/exec.ts for node:child_process. Existing manual per-test cleanup (fs.rm in finally/afterEach/onTestFinished blocks) is untouched — the global teardown is a fallback for killed workers, not a replacement for tests cleaning up after themselves. Migrated with a one-off AST-based codemod (oxc-parser, since regex mismatched multi-line calls and complex prefix expressions like `options?.tempPrefix ?? 'default-'`) rather than by hand across 168 files. The codemod isn't included — it doesn't need to survive this commit. Caught and fixed one real bug in it during review: a small number of files declare a second import statement later in the file, after some of the matched call sites, which broke a naive "insert after the textually-last ImportDeclaration" placement; fixed to insert after the top contiguous import block instead, plus a self-check that re-parses every generated file before writing it. Also fixes 5 fallow dead-code findings the branch introduced: the vitest globalSetup functions (setup/teardown) are only referenced by the config-string path vitest.config.ts hands to globalSetup, invisible to static analysis — suppressed with the documented convention. isProcessAlive didn't need to be exported (nothing outside the module uses it). And dropped a barrel re-export of the new helpers from test-utils/index.ts: nothing actually imports through the barrel (matching the existing makeSessionStore convention, which is imported directly from store-factory.ts everywhere despite also being barrel-exported), so the re-export was genuinely dead. Documents the convention in docs/agents/testing.md. Verified: full unit suite (5308 tests) passes except the one pre-existing, unrelated package-exports.test.ts failure; typecheck, lint, and format all clean; fallow audit clean against the PR base. * fix: correct fallow suppression token and drop unused barrel re-export These were meant to be part of 397cdd6 (verified locally before that commit) but didn't actually get staged — caught by CI's Fallow Code Quality check re-running against the pushed commit, which still had the plural 'unused-exports' token (fallow expects singular 'unused-export') and the dead barrel re-export. * fix: migrate the two mkdtemp call sites the rebase silently reintroduced Rebasing onto main pulled in #1594's two new test cases in this file, added independently of this branch's migration, still using raw fs.mkdtempSync(path.join(os.tmpdir(), ...)). Git's line-based merge found no textual conflict with this branch's removal of the os import (the changes touch non-overlapping regions), so it silently produced a file that doesn't typecheck. Migrated both to mkdtempForTestSync for consistency with the rest of the file, caught by CI's Typecheck, Fallow Code Quality, and FreeRange checks re-running against the pushed commit. * test: pin Vitest tmpdir lifecycle * fix: preserve the Swift cache across test runs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The Apple runner ships as source in
dist/apple/runner(~431 kB, ~16% of npm unpacked) and is compiled by xcodebuild on the user's machine. Packaging strips#if AGENT_DEVICE_RUNNER_UNIT_TESTSblocks — but unit tests written outside such blocks ship whole and compile on every user's first build. Nothing enforced the wrapping convention, so every runner PR could quietly leak its tests into the package (e.g. #1588's newRunnerTests+TextEntryPolicyTests.swiftwould ship un-stripped today).What
RunnerTests+LifecycleCacheTests.swiftand 1 inRunnerTests+SnapshotTraversalIdentityTests.swiftsat outside any conditional (the first file wraps some tests, which is how these slipped by).package-apple-runner-source.mjsnow fails if any XCTest-shaped method (func test*) survives stripping. Sole allowlisted exception: the runner's command-loop entrypointtestCommandinRunnerTests.swift. The script runs inprepackand the size-report CI, so a leak now fails packaging with a message pointing at the fix (wrap it in #if AGENT_DEVICE_RUNNER_UNIT_TESTS).No pbxproj changes needed — the project uses file-system-synchronized groups.
Verification
pnpm build:xcuitest:iossucceeds both without and withAGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS=1(shipped config and CI unit lane).test-without-building(3/3; CI builds with the flag on, so its-only-testinglist is unaffected).testCommandremains discoverable; shipped runner source 430.9 → 428.3 kB.pnpm check:affected --rungreen; packaging vitest suite 5/5.Note for #1588
Once this merges, #1588 will need its new
RunnerTests+TextEntryPolicyTests.swiftwrapped in#if AGENT_DEVICE_RUNNER_UNIT_TESTS— the gate will fail packaging otherwise (that's it working as intended).