fix: real CLI-probe tests leak scratch state into the vitest temp root (#9039) - #9050
Merged
Merged
Conversation
…mp root (#9039) Real kilo/opencode/npm child processes spawned by provider-readiness probes wrote their own scratch/cache state (a session dir, Node's module compile cache) straight into whatever TMPDIR they inherited, which since #9032 is PortOS's run-scoped vitest temp root. Two fixes: - commandExists.js's probe() and npmGlobalBin.js's npm-prefix probe now pin TMPDIR/TMP/TEMP to a throwaway per-spawn scratch dir, removed once the probe settles, so a real binary's own scratch state never reaches the caller's TMPDIR at all (production benefit too, not just tests). - providerRuntimeInstaller.js's runtime status sweep and codexOssSupport.js's --oss probe now skip the real PATH scan/spawn under the test runner unless a caller explicitly injects its own findCommand/probeCommand/run — so an incidental route/service test that never asked to probe a real CLI stops shelling out to whatever happens to be on the developer's PATH. Every intentional real-CLI-probe test already injects its own deps and is unaffected. Also fixed agentRunReconciler.test.js's own path-traversal fixture, which wrote one directory outside its RUNS_DIR and never swept it — that was never a third-party leak, just a missed cleanup. Shrinks KNOWN_THIRD_PARTY_CLI_SCRATCH in runTempRoot.js to the one residual, rare entry (node-compile-cache) that no longer correlates with any CLI spawn in the trace that produced it.
… be created Local codex review of the #9039 fix caught a real regression: mkdtempSync() ran synchronously before the promise chain, so a full disk or unwritable tmpdir would throw out of probe() instead of resolving to null like every other probe failure — breaking the "commandExists/commandOutput never reject" contract every caller relies on.
…Command Local codex review (round 2) of the #9039 fix caught an asymmetry: an explicit skipRealSpawn: true combined with only a custom findCommand still fell through to the REAL commandOutput default for probeCommand, so a caller relying on skipRealSpawn alone to suppress real spawning could still shell out. Both fallbacks now gate on skip consistently.
Local codex review (round 3) flagged that a skipped test-runner probe wrote its synthetic "not probed" result into the same TTL cache a real probe uses, so a later call in the same module instance that injects real findCommand/probeCommand deps (e.g. a hasCli()-gated integration test sharing the cache) would read the stale skipped result back instead of running its own real probe. Matches the "NOT PROBED, deliberately not cached" contract codexOssSupport.js already uses for the same reason.
This was referenced Sep 28, 2026
atomantic
added a commit
that referenced
this pull request
Sep 28, 2026
node-compile-cache still turned up rarely in the run-scoped vitest temp root even after #9050 isolated the one confirmed real-CLI spawn (npmGlobalBin.js's `npm prefix -g`) into its own TMPDIR. The cache is process-wide (NODE_COMPILE_CACHE env, or a CLI calling module.enableCompileCache() on itself), not tied to one call site, so no single spawn helper's isolation could be proven to cover every child that might inherit the run's TMPDIR. Set NODE_DISABLE_COMPILE_CACHE=1 tree-wide for the test run instead (server/vitest.config.js, main process and worker env) - honored since Node v22.1, within this repo's supported range. Remove the now-unneeded KNOWN_THIRD_PARTY_CLI_SCRATCH allowlist entry from server/test/runTempRoot.js so a recurrence fails the run again. Document the new env var in envExampleDrift.test.js's INHERITED_ENV allowlist.
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.
Summary
Fixes the leak #9039 tracked, by fixing its actual root causes rather than
just growing the allowlist.
server/lib/commandExists.js's sharedprobe()(used by every--versionCLI capability check) and
server/lib/npmGlobalBin.js'snpm prefix -gprobe now pin
TMPDIR/TMP/TEMPto a throwaway per-spawn scratch dir,removed once the probe settles.
kilo,opencode(both self-extractingNode-launcher binaries) and
npmitself all write their own scratch/cachestate (a session dir, Node's own module compile cache) into whatever
TMPDIRthey inherit on every invocation — this keeps that state out of thecaller's real
TMPDIRentirely, in production too, not just under test.server/services/providerRuntimeInstaller.js's runtime-status sweep andserver/services/codexOssSupport.js's--ossprobe now skip the real PATHscan/spawn under the test runner unless a caller explicitly injects its own
findCommand/probeCommand/run— so an incidental route/service testthat never asked to probe a real CLI stops shelling out to whatever happens
to be on the developer's own PATH. Every genuinely intentional
real-CLI-probe test already injects its own deps (confirmed by inspection —
none of
providerRuntimeInstaller.test.js's orcodexOssSupport.test.js'scases rely on the default) and is completely unaffected by this gate.
server/services/agentRunReconciler.test.js's path-traversal test writesone fixture deliberately outside its
RUNS_DIR(that's the point of thetest) but never swept it in
afterAll— not a third-party leak at all,just a missed cleanup. Fixed.
KNOWN_THIRD_PARTY_CLI_SCRATCHinserver/test/runTempRoot.jsshrinks fromfour names to one:
node-compile-cachestill reappears very rarely (oncein ~2,500 files locally) with no real CLI spawn in the trace that produced
it, so it stays allowlisted rather than pinned on an unconfirmed cause.
kilo,opencode, andescapeare fully removed — their root causes arefixed, not just muted.
Three rounds of local
provider:codexreview each caught a real issue,fixed in follow-up commits: a synchronous
mkdtempSyncthrow that couldhave broken the "probe never rejects" contract, an asymmetric
skipRealSpawngate that still let a real spawn through theprobeCommanddefault, and a skipped probe answer that would have sat in the TTL cache
where a later real-probe call could read it back instead of re-probing.
Test plan
cd server && node_modules/.bin/vitest run— full suite, twice, exits 0with no
⚠️ test temp leakline forkilo/opencode/escape, on amachine with both CLIs installed on
PATH(per the issue's acceptancecriteria).
npm run pregate— green.server/lib/commandExists.test.js,server/lib/npmGlobalBin.test.js,server/services/providerRuntimeInstaller.test.js,server/services/providerPrerequisites.test.js,server/services/codexOssSupport.test.js,server/services/agentRunReconciler.test.js,server/routes/providers.composite.test.js.Closes #9039