Skip to content

The smokes rebuild test-servers only when it is MISSING, so a stale fixture silently tests the wrong thing #2111

Description

@cliffhall

The trap

Five scripts/*.mjs consumers build test-servers/build only when the output is absent:

File Guard
scripts/smoke-cli.mjs:71 if (existsSync(testServer) && existsSync(httpTestServerModule)) return;
scripts/smoke-tui.mjs:46 if (existsSync(testServer)) return;
scripts/smoke-web-elicitation.mjs:102 if (existsSync(composableServer)) return;
scripts/lib/mcp-app-flow.mjs (ensureComposableTestServer) if (existsSync(entry)) return entry;
scripts/pack-and-verify.mjs:154 if (existsSync(testServer) && existsSync(composableServer)) return;

Presence is not freshness. Once test-servers/build exists — which it does on any machine that has run the smokes once — a change to test-servers/src is never picked up by a smoke. The smoke keeps driving the previously-built fixture.

Why it matters more than it sounds

The failure is silent and misattributed. It does not present as "your fixture is stale"; it presents as a product bug.

Hit live while merging v2/main into #2107. smoke:web:app phase 2 (#2056) failed with:

smoke:web:app FAILED — app declaring _meta.ui.domain was not served from the dedicated
app origin — no frame at /app-document/<id> (frames: …, about:srcdoc)

which reads as "the dedicated app-origin path is broken". Two wrong hypotheses were investigated and measured out first — a port collision with a running Inspector on 6274/6275, then an explicit MCP_SANDBOX_PORT/MCP_APP_ORIGIN_PORT override — before the real cause surfaced: the merge had changed test-servers/src/preset-registry.ts, build/ already existed, so the smoke was driving the pre-merge fixture that had no _meta.ui.domain support. npx tsc -p test-servers --noCheck and it passed.

Any merge or rebase touching test-servers/src reproduces this, and the more recently the fixture changed the more convincingly the failure impersonates a real regression in the feature under test.

The asymmetry that makes this clearly a defect

The tests already build it unconditionally — clients/web/package.json:

"pretest":          "npm run test-servers:build",
"test:coverage":    "npm run test-servers:build && vitest run …",
"test:integration": "npm run test-servers:build && vitest run --project=integration",
"test-servers:build": "tsc -p ../../test-servers --noCheck"

So the same fixture has two different freshness policies depending on which consumer reaches it, and the weaker one is on the checks that spawn real servers and are hardest to debug when wrong. There is no stated reason for the split — the guard looks like an optimization nobody priced.

Proposed remedy — deterministic, not guidance

Guidance in AGENTS.md is the wrong instrument here: the failure gives no signal that points at staleness, so a reader has to already suspect it to go look up the rule. Make it impossible instead.

Build unconditionally, from one shared helper. Measured on this repo:

wall time
tsc -p test-servers --noCheck (full emit) ~1.0s
same, with "incremental": true and up to date ~0.7s

Against smokes that take tens of seconds each — and a pack:verify that does a real npm install from the registry — that is free. It is also exactly what the test scripts already pay without anyone noticing.

Concretely:

  1. Extract the five near-identical ensureTestServer copies into one scripts/lib/ensure-test-servers.mjs. They have already drifted (different entry points checked, different labels, pack-and-verify uses npx tsc while the others use resolveNodeBin for the Windows .cmd reason in verify:typecheck-coverage cannot run on Windows — execFileSync('npx', …) is ENOENT, so validate/ci fail at step one #1939) — consolidating fixes that too.
  2. Drop the existsSync early return; always run the build.
  3. Optionally add "incremental": true to test-servers/tsconfig.json so the up-to-date case is a no-op rather than a re-emit.

Two things to check when doing it:

  • --noCheck emit does not clean. A deleted src file leaves its stale .js behind, and unconditional emit does not fix that — only the absent-output path (or an explicit clean) does. Decide whether that matters before calling the invariant closed; tsc -b with composite would handle it properly, at the cost of needing project-graph config.
  • Keep the failure message actionable. The existing "could not build the test servers … run npm run test-servers:build from clients/web" text is good and should survive the extraction.

Scope

Tooling only — no client or core/ behavior change. Verified by: touch a test-servers/src file, run npm run smoke:web:app twice, and confirm the second run reflects the edit without a manual tsc.

Triage

Priority Low (maintainer call — it is a papercut with a known one-line workaround, and it only bites on a merge that happens to touch the fixtures). Rubric for the record: severity 3 (a check silently measures the wrong thing and reports it as a product failure), urgency 1 (nothing blocked on it), +1 milestone — total 5.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

choreMaintenance: deps, build tooling, CI, cleanup — no user-facing behavior changev2Issues and PRs for v2

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions