Repository navigation
test(cli): suppress detached MCP warmers in adapter fixtures - #2433
Conversation
Co-authored-by: CI Doctor <WyJjaS1kb2N0b3IiLCJkZWZhdWx0Il0@agents.commonly.invalid> Agent-Model: codex/gpt-6-sol/medium
lilyshen0722
left a comment
There was a problem hiding this comment.
Unasked review of c8a0b4222f39d1727eb3abeec2fe1a37b716321b (tree fc55ac044b04b183496ef2a33d21aeb9c8f7e471) — unreviewed, base current, mergeStateStatus CLEAN. No GATE: line; none was requested. No findings.
The one quantitative risk in a global lock is comfortably clear. A single .warm.lock written once per run in jest.setup.cjs only suppresses the warmer while it is fresh, and STALE_LOCK_MS is 600 s (cli/src/lib/mcp-home.js:81, consumed at mcp-warm-child.mjs:81). The full CLI suite at this head runs in 11 s — a 54× margin — so the lock cannot age out mid-run and let a warm child back in. If the suite ever grows past ten minutes this stops being true, which is worth knowing but is not close today.
The lock is the real one, not an inert file: WARM_LOCK_NAME = '.warm.lock' with lockPathFor(home) joining it, so each fixture and the global home are genuinely locked.
Nothing depended on the warmer running in the global home. 1,304 tests pass across 71 suites, including mcp-home.test.mjs, which tests the lock semantics themselves — it builds its own temp homes rather than the setup's, which the green run confirms. And no test asserted the home was literally empty, which adding a file would have broken.
The comments were corrected to match the change, which is the part most PRs miss. Three places said the setup points COMMONLY_MCP_HOME at an "EMPTY dir"; that stopped being true the moment a lock file landed in it, and all three now say "no installed build". A fourth, in jest.setup.cjs, gains the reason the lock exists. A diff that invalidates its own surrounding prose and then fixes it is rarer than it should be.
My local run showed 2 failures and they are not this PR's — stated so nobody reads my numbers as a signal. The same two fail identically at the merge-base 3c578d6b4 (same 2 failed / 16 skipped / 1,304 passed / 1,322 total), and their message is "the cli contract suite could not load backend dependencies" — my scratch worktree has no backend/node_modules. CI on this PR reports "CLI, MCP and hosted runtime tests — pass (1m0s)", which is the verdict that counts.
lilyshen0722
left a comment
There was a problem hiding this comment.
Gated at head c8a0b422, re-confirmed as the head at filing time. Draft, CLEAN/MERGEABLE, every check green including Legacy test and coverage (9m30s) and the required Test & Coverage. Trial-merged against origin/main (3c578d6b, which now carries #2431): clean.
I measured the class rather than reading the diff for it. This PR exists because of an enumeration I filed on #2431 — 12 distinct temp homes across 7 adapter test files spawning real detached warmers that outlive teardown. The diff touches only three of those seven files plus jest.setup.cjs, so the question that matters is whether the other four are covered. They are: I re-instrumented kickWarm (mcp-home.js:528, hooking on spawnImpl === spawn so only production spawns are logged) and ran the whole adapter suite at this head.
Zero detached warmers. The ledger file is never created. Down from 12 homes across claude.credential-channel, claude.environment, goose, pi, qwen, codex and opencode. The four files this PR doesn't touch are covered by the jest.setup.cjs lock on the default COMMONLY_MCP_HOME, which is where their warmers were originating. The class is closed, not narrowed.
The suppression costs no assertion, and that is checkable rather than assumed: lockPresent (:554) is consumed at exactly one site, :595, to compute due, which gates only kickWarm. It never reaches the command prepareMcpSpawn returns. So a .warm.lock in a fixture cannot change what any of these tests assert — it only stops a child from being born.
And the behaviour the lock suppresses is still genuinely gated, in the place that tests it properly with an injected spy: mcp-home.test.mjs still passes "an empty home kicks exactly one warm and still runs the declaration", "a warm already running (lock present) is not started twice", "a fresh lock belongs to the live warm and is not stolen" and "a lock older than the stale window is reclaimed". Nothing about warm scheduling became untestable; it moved out of fixtures that were testing it by accident.
Full CLI suite at this head: 71 suites / 1304 tests pass. The only failure is mention-event-types.contract.test.mjs, the known local-harness artifact (it reads backend TypeScript through ts-node and my worktree has no backend/node_modules); CI passes it. Worth noting the pre-fix tree also emitted Jest's "A worker process has failed to exit gracefully" warning on this suite and this head does not — the leak that warning was pointing at is the thing being fixed.
The stale comments go too, which I'd have raised if they hadn't: three fixtures described the jest-setup home as "an EMPTY dir", and after this change it holds a lock file, so "no installed build" is now the accurate description of why those arms still see npx -y @commonlyai/mcp@latest.
One non-blocking note, and it is the only thing I'd want written down:
This fix is invisible to the suite's assertions. Nothing goes red if a future fixture creates a cli-mcp-home-* temp dir and omits the lock — the flake simply returns, intermittently, as an ENOTEMPTY in whichever test file Jest happens to be running next. That is the same property that let 12 of them accumulate unnoticed. The one-run instrumentation above is the cheap re-check (hook kickWarm on spawnImpl === spawn, append to a ledger, run jest __tests__/adapters, assert the ledger is absent), and it would make a reasonable afterAll guard if you ever want the suite to defend this itself. Not a gate condition — the class is closed today and the next regression is a test-hygiene flake, not a shipped defect.
@lily-shen clear to press — it is still a draft and needs marking ready first.
GATE: PASS
lilyshen0722
left a comment
There was a problem hiding this comment.
Lane review (cli/) at c8a0b42.
This is test-only: three adapter test files plus cli/jest.setup.cjs, and no cli/src change, so no version bump is owed.
What I checked:
- The lock suppresses the warmer for the whole run, however long. In
cli/src/lib/mcp-home.js,dueis!lockPresent(home) && warmLooksWanted(...), andlockPresentis an existence check only.STALE_LOCK_MS(10 min) applies only whenmcp-warm-child.mjsacquires the lock, and a parent that sees a lock never kicks a child. So the fixture lock can't age out mid-suite into a detached warmer. - The resolver behaviour under test is unchanged. The lock only gates
kickWarm. The warmed-home arms still resolve the home build, and the cold-path arm still materialises an exact spec. The edited comments describe the home accurately ("no installed build", not "empty"). - Warm-due behaviour stays covered in
mcp-home.test.mjs, which uses its own homes and an injected spawn, so the setup-level lock doesn't mask it. - The 10/10 traced parallel runs with zero warmer launches are the right instrument for a race that only shows up as an ENOTEMPTY flake.
No defects.
GATE: PASS
What changed
.warm.lockin the per-file Jest MCP home so adapter tests cannot launch an unrelated detached MCP warmer into that disposable fixture.due→kickWarm.mcp-home.test.mjs, which injects and asserts on the spawn implementation.Why
An earlier OpenCode fixture raced a detached MCP warmer against removal of its temporary home, causing CI
ENOTEMPTYfailures. Otto's follow-up trace found the same class in 12 temp homes across 7 adapter files; that file/home set is stable, while the number of spawned children varies with timing. This change closes the remaining fixture homes after #2431.Proof
--runInBand --detectOpenHandles: 7 suites passed, 202 tests passed, 1 skipped.--maxWorkers=4) with anode:child_process.spawnpreload that tracesmcp-warm-child.mjs: 10/10 passed; 68 suites and 1,301 tests passed per run; zero detached warmer launches in every run. The trace preload was verified to wrap the ESM namedspawnexport.git diff --checkpassed.c8a0b4222f39d1727eb3abeec2fe1a37b716321b: all split jobs, the requiredTest & Coveragegate, and legacy passed. Other reported PR checks passed or were expected skips.Related: TASK-007. This PR only changes test fixtures.
Otto's exact-head review 5621391385 ends
GATE: PASS. His independent production-spawn trace found zero detached warmers across the full adapter suite at this head.Follow-up risk: a future explicit
cli-mcp-home-*fixture could omit the lock without making an assertion fail. Reusing the spawn trace as a suite-level guard would make that regression visible; it is outside this fixture-only PR.