chore: three small cleanups flagged in the v2.2.0 merge review

[cliffhall]

Closes #2000

Three independent items Copilot raised while reviewing the v2.2.0 milestone merge (#1993). None is worth its own card; each is a few lines.

1. clients/web/.npmignore's comment contradicted the root manifest

#1934 added clients/web/static to the root package.json "files" allowlist, but the explanatory comment still said the allowlist "restricts publishing to those two directories" (build/ and dist/) and that everything else under clients/web "stays out regardless". That was false — in a file that exists because a packaging contract was misread once already.

The comment now names all three directories and says why static/ ships: static/sandbox_proxy.html is a committed source file read from disk at runtime by server/sandbox-controller.ts as <runner dir>/../static/sandbox_proxy.html, so it must ship at exactly that path (#1859).

2. Unjustified as unknown as Response in useServers.test.tsx

The issue named one site; the file carried four. All are replaced with the preferred fix rather than a justification comment — a real Response over a real ReadableStream, so each double is structurally type-checked against the Response API:

Case	Before	After
!res.body guard	{ ok: true, body: null }	new Response(null, { status: 200 }) — a Response built from null has a null .body
reader rejects mid-stream	read: () => Promise.reject(…)	a stream whose pull throws, so reader.read() rejects
two frames in one chunk	hand-rolled getReader() double	pull enqueues the chunk, then closes
priming comment + blocked second read	hand-rolled getReader() double	pull runs once per read, so the reads counting and the await secondRead park work identically

The last two keep their reads counters and their blocking semantics — pull is invoked once per read(), which is exactly what the doubles were emulating. This is the same pattern the #2006 CRLF tests in this file already use.

No test was changed in what it asserts; all 34 in the file still pass, and the file's coverage contribution is unchanged (the ≥90 gate passes).

3. scripts/smoke-web-app.mjs orphaned the test server on a readiness timeout

startMcpServer() returned { child, url } only once the announcement line matched. On the 30-second timeout — a child that is alive but never announces — it threw without handing the handle back, so mcpServer was unassigned, the caller's shutdown() could not stop it, and process.exit(1) left a live server possibly holding its port for a later run.

The child is now published to mcpServer the moment it is spawned, so teardown covers every throw path. The spawnError and exited paths never had the problem (the child is already gone there) but cost nothing to cover the same way.

Verification

npm run ci passes end to end from a clean install in the worktree — validate (incl. the three coverage/lockstep guards), the per-file ≥90 coverage gate, verify:build-gate, all smokes (smoke:web:app included), and the 115 Storybook play-function files.

No UI change, so no screenshots.

[Copilot]

Pull request overview

Addresses three cleanup items from the v2.2.0 merge review.

Changes:

• Corrects the web package allowlist documentation.
• Replaces unsafe test double casts with real Fetch API objects.
• Makes the MCP smoke-test server reachable during timeout teardown.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
clients/web/.npmignore	Documents all shipped web directories.
clients/web/src/test/core/react/useServers.test.tsx	Uses real Response and ReadableStream instances.
scripts/smoke-web-app.mjs	Publishes the child handle immediately for cleanup.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Done — implemented as suggested, including the extraction.

Why it needed doing: the fix was unproven. smoke:web:app always receives the announcement on its happy path, so nothing in npm run ci exercised a child that stays alive through the readiness timeout — the exact case that orphaned a live server holding its port. A regression there would be silent in CI and would surface only as a stray process on a later run.

What changed:

• New scripts/lib/announced-child.mjs owns the spawn + readiness wait. Its contract is the invariant: the child is handed to onSpawn synchronously, before any waiting, so every throw path leaves the caller holding a stoppable handle. This mirrors the teardownWebServer extraction in prod-web-server.mjs, made for the same "the smoke exits immediately, so it cannot detect its own regression" reason.
• New scripts/lib/announced-child.test.mjs, picked up by test:scripts (and therefore validate; verify:typecheck-coverage enforces that the glob still matches it).
• startMcpServer() in smoke-web-app.mjs now delegates, publishing the child via onSpawn. Behavior is unchanged; the announced URL is still authoritative over the config port.

The tests drive real node -e children rather than spies, so the assertion is that a real process is reachable and killable rather than that a mock was called. The regression case uses a 300ms budget against a child sleeping 60s and asserts three things in order: the child was published before the wait, it is still alive when the throw lands (the bug's precondition — a dead child would prove nothing), and kill actually stops it. The early-exit path (asserting the child's own output reaches the message), the spawn-failure path, and the both-streams scan are covered alongside it.

npm run ci passes end to end from a clean install with these in place.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[cliffhall]

Round 2 responses

1. Register teardown before awaiting the rejection — Fixed — you are right, and the irony was the point: a suite guarding a process leak would have leaked a 60-second child of its own, on exactly the run where the regression had come back (a failing assertion is when that child is most likely to still be alive).

Teardown is now registered from onSpawn via t.after, so it runs on every path out of every case, not just the success path. That is the helper's own ownership rule applied to the tests — the child is claimed the moment it exists, before anything can throw. A killOnTeardown(t, capture) factory does both jobs, so the early-exit and spawn-failure cases get the same protection for free (killing an already-exited child is a no-op).

The explicit kill + waitForExit inside the timeout case stays, because it is an assertion there rather than cleanup — it is what proves the published handle is actually stoppable. The t.after is then a no-op on that path.

2. Document the new helper — Done. Both updated in this PR:

• AGENTS.md — the scripts/ tree entry now carries announced-child.mjs and states its contract (the child is published via onSpawn before the readiness wait, so teardown reaches it on every throw path — the timeout included, which is what orphaned a live server holding its port). The smoke:web:app paragraph records the extraction and why: the two mechanics it documents (both-streams scan, announced-URL-over-config-port) moved into the helper so the failure path became testable.
• README.md — the test:scripts row now names announced-child.test.mjs alongside resolve-node-bin.test.mjs, with what it proves.

Deliberately not mirrored into .github/copilot-instructions.md: AGENTS.md defines that file as a distillation of the rules a reviewer cites against a diff, and explicitly lists the project-structure tree and tooling inventory as not review-relevant. A new scripts/lib helper is inventory, not a rule.

npm run ci is green end to end with these in place (validate, the ≥90 coverage gate, verify:build-gate, all seven smokes, and the 115 Storybook files).

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/lib/announced-child.mjs:85

• The readiness output is not checked after the final delay, so an announcement arriving during the last polling interval (for example, at 29.9s of the documented 30s budget) is incorrectly reported as a timeout. The same window also misclassifies a final-interval spawn error or exit. Check once at the deadline before throwing, or use a deadline loop that evaluates state after every wait.

    await delay(pollMs);
  }
  throw new Error(

[cliffhall]

Round 3 response — the suppressed comment (announced-child.mjs:85, final polling interval) was right, and is fixed in 6b4bf68.

The attempt-counted loop read the output and then slept, so nothing ever observed the last interval: an announcement at 29.9s of a 30s budget read as a timeout, and a spawn error or early exit landing in that window was misattributed the same way. Inherited from the original smoke, but wrong either way.

It is now a deadline loop that re-reads state after every wait, the last one included, with the final wait clamped to the deadline so the budget is a real bound rather than one poll longer.

On the test for it — the first version I wrote announced at ~350ms of a 500ms/250ms budget, i.e. it relied on a 100ms margin. That passed standalone and then failed inside npm run test:scripts, where 116 tests run in parallel and node startup eats the margin — so it would have been a flake, not a guard. The committed version sets pollMs === timeoutMs instead, which makes the distinction structural rather than timed: the attempt-counted loop gets exactly one check, at t≈0 before the child has finished starting, then throws; the deadline loop re-reads after the wait and sees it. The child announces immediately, ~2s inside the window.

Verified both directions (fails against the old loop, passes against the new) and repeated test:scripts three times under the full parallel run to confirm stability.

Full npm run ci stages all green: validate, the ≥90 coverage gate, verify:build-gate, all seven smokes, and the 115 Storybook files.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.