Skip to content

test(cli): suppress detached MCP warmers in adapter fixtures - #2433

Merged
lilyshen0722 merged 1 commit into
mainfrom
ci-doctor/warm-fixtures
Oct 23, 2026
Merged

lilyshen0722 merged 1 commit into
mainfrom
ci-doctor/warm-fixtures

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

What changed

  • Put a .warm.lock in the per-file Jest MCP home so adapter tests cannot launch an unrelated detached MCP warmer into that disposable fixture.
  • Put the same lock in the five explicit temporary homes in the Claude, Pi, and Codex adapter tests that could still launch a warmer. These fixtures continue to select the same cached or installed MCP command; the lock only suppresses due → kickWarm.
  • Keep the warm-due behavior covered by 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 ENOTEMPTY failures. 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

  • Node 22 targeted adapter run with --runInBand --detectOpenHandles: 7 suites passed, 202 tests passed, 1 skipped.
  • Ten complete parallel CLI runs (--maxWorkers=4) with a node:child_process.spawn preload that traces mcp-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 named spawn export.
  • The two macOS Seatbelt smoke files were excluded from the traced runs because the preload changes their sandbox behavior; GitHub CI runs them without the preload. One traced run that included the Claude Seatbelt smoke had zero warmers but failed its sandbox output assertion, so it is not counted in the ten passing runs.
  • git diff --check passed.
  • Exact-head GitHub Tests run 39845534538 on c8a0b4222f39d1727eb3abeec2fe1a37b716321b: all split jobs, the required Test & Coverage gate, 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.

Co-authored-by: CI Doctor <WyJjaS1kb2N0b3IiLCJkZWZhdWx0Il0@agents.commonly.invalid>
Agent-Model: codex/gpt-6-sol/medium

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
lilyshen0722 marked this pull request as ready for review October 23, 2026 03:37

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, due is !lockPresent(home) && warmLooksWanted(...), and lockPresent is an existence check only. STALE_LOCK_MS (10 min) applies only when mcp-warm-child.mjs acquires 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

@lilyshen0722
lilyshen0722 added this pull request to the merge queue Oct 23, 2026
Merged via the queue into main with commit 141b47f Oct 23, 2026
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant