Three findings, all valid.
**The read/get stages asserted an RPC status, not a render.** #2148 asks
for the resource content and the prompt messages to *render*, and
`data-read-status` / `data-get-status` flip the moment the call resolves
— so the smoke stayed green with the panel deleted. `ResourcePreviewPanel`
and `PromptMessagesDisplay` now carry `data-testid="resource-preview"` /
`"prompt-messages"`, and each stage waits on the status **and** the
rendered panel. Verified by breaking each testid: the smoke fails with
`reading `events` did not render a preview — last observed:
data-read-status="ok"`, which is the finding stated back exactly.
**`smoke:web:tabs` followed ambient `SMOKE_BROWSER`**, so
`SMOKE_BROWSER=firefox npm run ci` would have run the supposedly
Chromium-only smoke under Firefox — contradicting both this PR's stated
scope and the gated-tier invariant in `run-engine-smokes.test.mjs`. The
npm script now pins `chromium` on **both** halves (install and launch)
and the smoke resolves through `resolveRequestedBrowser(argv, env)`, where
an explicit argument wins over the environment; a new case in that same
test file pins it, including that the smoke stays out of `ENGINE_SMOKES`.
**The AGENTS rules were not mirrored into `.github/copilot-instructions.md`.**
Correct, and worse than an omission: that file's engine bullet asserts the
headless web smokes are "engine-parameterized, not Chromium-only", which
this PR made false. It now names `smoke:web:tabs` as the deliberate
exception, and carries the genuinely review-citable new rule — assert
`data-*`, never copy, and never stop at an RPC status where the ask was
that something render.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
The web client had exactly one connected-flow smoke and it was the Apps
tab. `smoke:web:browser` proves the bundle boots and paints but never
connects; `smoke:web:app` / `smoke:web:elicit` connect but only drive the
Apps surface. So the three highest-traffic tabs were unexercised through
the prod bundle end to end — everything covering them (unit tests,
Storybook, the integration project) mocks or bypasses the prod server, so
a regression living only in the built bundle or in the prod Hono
backend's wiring of a tab passed all of them.
`scripts/smoke-web-tabs.mjs` drives all three against one server on one
browser launch (~5s added wall-clock): Tools runs `list_items` and
asserts its `structuredContent` section rendered (#1908), Resources reads
`events` with the template list asserted separately — `resources/list`
and `resources/templates/list` are different calls and either can fail
alone — and Prompts fetches the argument-less `simple_prompt`.
Every assertion is on a `data-*` attribute, never on copy, mirroring the
`data-app-status` precedent: each screen now carries a readiness contract
(`data-tool-count` / `data-call-status` and its Resources / Prompts
equivalents), tabulated in the web README and pinned by screen unit tests
so a rename fails loudly there rather than as an opaque timeout here.
The deep-link connect step is extracted to `lib/deep-link-connect.mjs`
rather than copied a third time. That matters because `parseDeepLink`
ignores a link it cannot validate rather than reporting one, so a drifted
copy fails as an opaque connect timeout in whichever smoke was not
updated. `smoke:web:elicit`'s hand-rolled copy is retired onto it.
Chromium-only and deliberately not in `ENGINE_SMOKES`: the engine tiers
exist for the MCP Apps sandbox's genuinely divergent primitives, and
these tabs are ordinary React and Mantine. Network, Protocol, Logs and
Tasks remain uncovered — a stated gap, not an oversight.
Closes#2148
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 5 — four findings, all in the suppressed block.
1+4. The regression stubs did `console.log(MARKER); process.exit(1)`.
Writing to a PIPE is asynchronous and `process.exit()` tears the
process down without draining, so the marker can be lost outright —
and then the stub that pins the paint-then-die path fails as "exited
before rendering", the neighbouring branch and the wrong diagnostic
for the thing it guards.
Worth recording that I met this mechanism last round and did not join
the dots: probing the drain race, a 4MB write followed by
`process.exit()` reached the parent with the marker missing
entirely. That was noted as a reason the repro failed, while the same
construct stayed in the stubs. Both now set `process.exitCode` and
let the loop drain — still exiting immediately after the write — with
a comment, since `process.exit()` is the more obvious spelling and
will read as a simplification to the next person. Same fix for the
`console.error("boom")` stub, whose assertion had the same exposure.
2. DEFAULTS claimed "every one is a hard floor as well as a default",
which contradicts `normalizeMs` (any finite positive value passes)
and the suite two files over, which passes 400ms against a 2000ms
default. It rejects the shape a bad env var produces, not smallness.
3. The raw-mode forbidden-output check was unreachable from any test:
the real PTY happy path only exercises the pattern NOT matching, so
a regression breaking it would stay green forever — an
acceptance-criterion guard with nothing guarding it, which is the
recursive form of this PR's own complaint.
Now a `runRenderSmoke` option (`forbidOutput`), so it runs inside the
tested state machine and against the fully drained output.
`smoke:tui` passes its pattern in and drops the post-hoc check. It
can only turn a pass into a failure, never rewrite an existing
failure's message: a crash reason is more useful than "and it also
printed X", which is usually a symptom of that crash. Three tests —
matching, not matching, and that asymmetry.
Verified: test:scripts 213/213; `smoke:tui OK — rendered "MCP Servers" at
360ms and was still running 2000ms later`; and the no-PTY shim still
fails through the refactored path.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4.
`exit` can be observed before the child's last output drains — `done()`
already keeps reading until `close` for exactly that reason — so `onData`
can run after the verdict is settled. Recording `markerAt` there is still
right: the message is a thunk rendered at `finish()`, and a marker still
sitting in the pipe when `exit` fired was painted all the same. Arming a
survival timer is not. It could not change the result (`done` is a no-op
once settled), but it holds the event loop open for the whole window,
which under `node --test` is a suite lingering seconds for nothing.
That implies something the comment did not say, and it had already bitten
this PR: if `markerAt` can be set after the exit was observed, any wording
derived from observation ORDER is unstable. The old message read
"rendered at Nms and then exited Mms later", where M was
`since() - markerAt` evaluated at finish — not the paint-to-exit gap at
all, and incoherent whenever the marker surfaced during the drain. The
test asserted on /survival window/, which the losing side of that race
could drop. It could have flaked, and the flake would have looked like
the bug this PR fixes.
So the message now makes no ordering claim and reports both times as the
observations they are, with `exitAt` captured in the exit handler rather
than recomputed later. Assertions moved to the race-stable phrase, plus a
test that runs the paint-then-die stub 12x asserting verdict AND wording
are identical every time — one pass proves nothing about a race.
The ordering could not be forced on darwin, so the guard is reasoned
rather than reproduced, and the reply says so. Two attempts and what they
actually showed: a 4MB payload with the marker last never gets there,
because `process.exit()` discards pending stdout writes (the "before
rendering" branch is correctly taken); marker on stderr with a graceful
exitCode was observed first in all 30 runs, none lingering near the
window. `done()` exists precisely because exit and close are separate
events, so treating their order as guaranteed was worth removing anyway.
Also (suppressed comment): the README's "Two more do the same" clause now
lists four suites, not two, after this PR added render-smoke and pty.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3.
`done()` is first-caller-wins, and the render deadline stayed armed after
first paint. So a marker landing near `timeoutMs` let that timer fire
*inside* the survival window and settle the run as
child did not render "MCP Servers" within 15000ms
followed by an output tail that visibly contains "MCP Servers". A TUI that
is merely slow fails intermittently, with a diagnostic naming the one
thing that demonstrably did happen — the same wrong-attribution class this
PR exists to remove, and one that reads as flake.
First paint has happened; its deadline no longer has a question to ask, so
`onData` clears it at the moment the marker is observed.
Also hoists `renderTimer`'s declaration beside `surviveTimer`'s. It was a
`const` below both closures that clear it, safe only because `onData` and
`done` run from events rather than the synchronous body. That already held
for `done`; this fix adds a second reader, so the ordering is now stated
rather than relied upon.
The test paints at 250ms with timeoutMs 400 and surviveMs 400, so the
render deadline expires while the survival window is open — the exact
shape. Verified it fails against the module with the clearTimeout removed
and passes with it: a wider timeoutMs would have been green either way,
which is why the numbers are tight.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings, both of which quietly undo the PR.
1. `SMOKE_TUI_SURVIVE_MS` was unvalidated. `??` guards only nullish, so
an env var set to the empty string reaches `Number("")` -> 0 and a typo
reaches NaN; `setTimeout` fires immediately on both. The survival check
would then resolve before the child could possibly have died, printing
"still running 0ms later" and asserting nothing — the exact first-paint
false green this PR removes, restored by an empty environment variable,
on a green run.
Fixed at the module rather than only at the env-parse site, so every
caller is covered: `normalizeMs` (same shape as child-cleanup.mjs's
`normalizeGraceMs`, matching an existing policy rather than inventing a
second one) applied to all four durations before anything reads them,
which also stops the messages interpolating "NaNms". Defaults moved to
an exported DEFAULTS so the smoke and the module cannot drift.
`surviveMs: 0` is deliberately NOT honored as "skip the wait": no
caller wants that, and it is indistinguishable from the typo.
2. (suppressed comment) `spawnSync` REPORTS ENOENT in `result.error` — it
does not throw — so `probeScriptVersion`'s try/catch read a missing
binary as "ran, printed nothing". On linux that empty output fell
through to the util-linux guess, and the smoke then hard-failed
spawning a binary that does not exist instead of taking the documented
skip.
`probeScriptVersion` now returns `{ available, output }`, and
`resolvePtyWrapper` returns `{ ok: true, flavor, wrap }` or
`{ ok: false, reason }` — the two unavailability causes deserve
different skip messages, since "no script(1) on PATH" and "nobody has
verified this platform's invocation" send the reader elsewhere. A
non-zero exit is explicitly not unavailability: BSD `script` has no
`--version` and answers with its usage, which is the evidence the
flavor fallback reads.
Tests for both, since each finding lives entirely in a branch no happy
path reaches: the normalizeMs table (empty string, typo, undefined, -1,
Infinity, a numeric string, 0), an end-to-end run proving a
`surviveMs: Number("")` paint-then-die stub still fails, the
`result.error` ENOENT and timeout shapes, and both `resolvePtyWrapper`
failure reasons.
Verified: `SMOKE_TUI_SURVIVE_MS="" npm run smoke:tui` reports "still
running 2000ms later"; with PATH blanked, resolvePtyWrapper returns
`{ok: false, reason: "no `script(1)` on PATH"}` rather than guessing.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
- pty.mjs: narrow the BSD argv set to {darwin, freebsd}. Grouping every
BSD together was wrong — NetBSD's `script` takes the command through
`-c`, so the argv form fails at `script` startup, which this smoke would
then report as a TUI crash. Rather than add an unverified `-c` branch
for netbsd/openbsd, they fall through to `null` and the smoke skips with
the reason: a guessed invocation turns a clean skip into a false failure
while looking like support.
- pty.test.mjs: "supported unless Windows" would fail `test:scripts` on
aix/sunos/netbsd/openbsd, where a null wrapper is correct and the smoke
skips by design. State the policy once (SUPPORTED_PLATFORMS) and branch
on it, keeping the real-probe teeth that catch a `script` missing from
PATH. Pin netbsd/openbsd/aix/sunos to null in scriptFlavorFor's tests
so "unsupported" reads as the default, not a win32 special case.
- render-smoke.mjs: the @returns contract claimed a close guarantee the
code does not give — both drain give-up branches resolve after only a
warning, so output can be truncated and the child may still hold the
work dir. Document the bounded fallback and name the required cleanup
shape (removeSafe, not rmSync), since a caller "tightening" cleanup on
the old wording would reintroduce #1801 exactly where it doesn't hold.
- AGENTS.md, clients/launcher/README.md: drop the pre-#2111 claim that the
smokes build test-servers "on demand if it is missing". ensureTestServers
rebuilds unconditionally, once per process — presence is not freshness.
- .github/copilot-instructions.md: mirror the survival rule, per the
same-PR mirroring requirement. One bullet, generalized past this fix:
a smoke must assert the thing survives, not that it started, and the
started-then-died branch belongs in a scripts/lib test against a stub.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
`smoke:tui` spawned the launcher with `stdio: ["ignore", …]`, waited for
"MCP Servers" in the output, printed OK, and SIGTERMed. It never looked at
the child again.
The child was already dead. Ink mounts `useInput`, `useInput` needs raw
mode, and raw mode is a property of the file descriptor — with stdin on
/dev/null the TUI painted one frame and exited 1 about 40ms later with
"Raw mode is not supported on the current process.stdin". Measured here:
first frame at 396ms, exit 1 at 433ms. The smoke won that race and
reported success, so what it asserted was *first paint*, not survival —
contradicting its own header ("boots and renders without crashing").
This is not environment-specific: `stdio: ["ignore", …]` makes the child's
stdin /dev/null whether or not the developer's terminal is a TTY, so this
smoke has never verified a running TUI on any machine. And the race cut
both ways — an `exit` processed first failed the run, which reads as flake
rather than as the standing defect.
Two pieces:
- `scripts/lib/pty.mjs` gives the child a real terminal via `script(1)`,
no dependency. The three flavors are not interchangeable (BSD takes
argv; util-linux takes one shell string plus `-e`; busybox that string
without `-e`), so the invocation is built and unit-tested rather than
written inline. A platform with no `script(1)` skips loudly instead of
running without one, which would be a guaranteed failure rather than a
weaker check.
- `scripts/lib/render-smoke.mjs` is the fix: the child must still be
running `surviveMs` (default 2s) after first paint. Without that, a
future harness change reintroduces the same false green and nothing
notices — which is why it lives in a module `test:scripts` drives
against a stub that paints the marker and immediately exits. Today's
harness passes that stub; this one fails it, with a diagnostic that
distinguishes "died after painting" from "died before painting".
The smoke additionally fails if the raw-mode error appears in a passing
run, so a future Ink that degrades to a warning instead of throwing can't
ship a TUI with no keyboard input past this gate.
Verified end to end: with a `script` shim that allocates no pty, the smoke
now fails with "it painted one frame, it did not run"; with the real one
the TUI is still running 2s after its first frame and never logs the
raw-mode error.
The `process.env.CI` skip stays, but its comment is corrected: the PTY
removes the technical blocker it used to cite, so the smoke is local-only
by decision now, not by capability. Whether it joins GitHub CI is out of
scope here (#2146 is keeping the other local-only smokes out on purpose).
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
smoke-web-elicitation.mjs told the reader to run
`SMOKE_BROWSER=webkit npm run smoke:web:app` -- the App smoke, not the
elicitation smoke the file documents. Following it exercises a different
path entirely, and it does not fail while doing so: a real smoke runs and
passes, so nothing signals that you never tested what you were reading
about.
This is a direct cost of my own decision two rounds ago to make the two
App headers word-for-word identical so they could not drift. The shared
PROSE should be identical; the example command is the one line that must
not be, and copy-paste does not distinguish them. Both headers now name
their own smoke, and both point at npm run smoke:web:webkit for all three.
Guarded, since the class is invisible at runtime by construction:
run-engine-smokes.test.mjs now asserts no smoke header tells the reader to
run a sibling's command, and that every `npm run` it does name is a real
script. Both mutation-verified -- restoring the exact bug fails the first,
and naming a nonexistent script fails the second.
Addresses the suppressed comment on Copilot review 5031065042 of #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three findings, all from Copilot's suppressed block.
1. The tests never checked that loadBrowser launches the engine it was
asked for. The failure-path stand-in rejects identically for every
engine, so loadBrowser(root, "firefox") could have called
chromium.launch() and still produced a correctly Firefox-labelled
error. The smokes could not catch it either: the Firefox gate runs
after Chromium is already installed, so a mis-dispatched launch would
SUCCEED and report Firefox coverage that never happened.
Added a success-path test with distinct per-engine spies asserting the
requested engine is invoked, that nothing else is, and that its handle
is what comes back. Mutation-verified: hard-coding playwright.chromium
fails it.
2. The launch-failure message told the reader to run
`npx playwright install --with-deps <engine>`, which is wrong now that
the npm scripts run from the repo root rather than clients/web -- I
introduced that when I removed the `cd`. Playwright is pinned in
clients/web, so a bare root-level npx can fetch a different version and
install a browser revision the pinned one still cannot launch. It now
points at `npm run smoke:web:<engine>` first, and scopes the raw
command to clients/web with the reason.
That message named smoke:web:webkit, which did not exist -- only the
chromium and firefox scripts did. Added it, and a test asserting every
supported engine has one, since a remedy nobody can run is worse than
no remedy. Mutation-verified.
3. AGENTS.md said npm run ci "mirrors" the workflow and then, two
sentences later, that it is "a superset, not a mirror". It now leads
with strict superset, which is the topology this PR is documenting.
The README and AGENTS.md remediation wording is corrected the same way as
the runtime message, so a reader gets the same advice wherever they find
it.
Addresses Copilot review 5030668906 on #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
I claimed ENGINE_SMOKES was the single home for "which smokes are
engine-sensitive". It was not: npm run smoke still hard-coded the same
three, so a fourth entry would have reached the Firefox and on-demand runs
and silently skipped the DEFAULT Chromium run -- which is the one GitHub
CI executes. The exact drift the list was introduced to remove, left in
the one tier that matters most.
npm run smoke now ends in smoke:web:chromium, which goes through the
runner like the other two tiers.
Two things fall out of that:
- The Chromium tier passes its engine as a literal argument, so an
ambient SMOKE_BROWSER can no longer redirect it. Without that,
SMOKE_BROWSER=firefox npm run ci would have run Firefox twice and never
exercised Chromium at all -- a gate silently testing something other
than what it claims.
- run-engine-smokes.test.mjs now asserts that no tier names an individual
smoke, that both gated tiers name their engine explicitly, and that the
on-demand entry point is the one that follows the environment. This is
the durable half: a tier running fewer smokes is invisible at runtime,
because it still passes. Mutation-verified -- restoring the old hard-
coded chain fails the test.
Docs updated to state the wider claim rather than the one that was true
of only two tiers.
Addresses the suppressed comment on Copilot review 5026921914 of #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
The three headers still said the non-Chromium engines were on demand and
that 'nothing runs it for you'. Both became false when Firefox moved into
npm run ci; I updated the README and AGENTS.md for that change and did not
carry it into the files themselves.
Each header now names the three tiers separately rather than collapsing
them into a CI/not-CI binary: GitHub CI is Chromium only, the local
pre-push gate is Chromium and Firefox, and WebKit is on demand.
That distinction is the whole point of the arrangement, so a header that
blurs it is worse than one that is merely out of date.
Addresses Copilot review 5026731741 on #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
Dropping the CI job left cross-engine checking as something you had to
remember to run, which in practice means nobody runs it. This moves it
into npm run ci, so it happens once per push, in front of a human who can
still reason about the change -- rather than once per CI run, on every
branch, to re-derive an answer we already had.
scripts/run-engine-smokes.mjs is what makes that possible cross-platform:
SMOKE_BROWSER has to reach three child processes, and a POSIX VAR=x npm
run prefix does not work under Windows cmd.exe, which npm uses there.
Setting it in the child env from Node does. smoke:web:firefox passes the
engine as an ARGUMENT rather than relying on the ambient variable, so a
stray SMOKE_BROWSER cannot silently redirect the gate to another engine
-- verified: an explicit argument beats it.
It also gives the smoke list one home. It was previously an && chain in
package.json beside a second chain in the workflow, so adding a fourth
smoke meant remembering both. ENGINE_SMOKES is now the list, with a test
pinning its contents and its order -- a smoke going missing is invisible
at runtime, because a shorter list is still a passing run.
Firefox only. WebKit fails the App smokes for unidentified reasons and
must not be in a gate.
This makes npm run ci a strict superset of GitHub CI rather than a mirror.
That was already true of smoke:tui, which self-skips on CI; Firefox is the
second such step. The direction that matters still holds: passing
npm run ci locally means CI's gates will pass.
Signed-off-by: cliffhall <cliff@futurescale.com>
The job was trialled and did not earn its place. It was genuinely cheap —
~2 minutes, running in parallel with the 15-minute build job, so zero
added wall-clock — but across a dozen runs Firefox never once disagreed
with Chromium. It was paying runner time and carrying a real flake
surface (playwright install --with-deps runs apt-get update, which fails
whenever a third-party repo in the runner image breaks; that is what the
one red run was) to re-confirm a result already in hand.
Everything that made cross-engine checking possible stays: SMOKE_BROWSER,
the shared headless-browser helper, the installer, and the tests. So
"SMOKE_BROWSER=firefox npm run smoke:web:engine" is there for the moment
it is worth running — before touching the sandbox, the CSP builder or the
proxy page — without taxing every push to re-derive the same answer.
The workflow is now byte-identical to v2/main again; the publish jobs go
back to needing only build.
Docs say what is true rather than what was intended, and AGENTS.md
records the reasoning so the job is not re-added on the general argument
that cross-engine coverage is good — that argument was already accepted,
and the on-demand path is what serves it. Re-add it on evidence: a real
cross-engine regression a gate would have caught.
Also swept the now-stale "matrix" language out of the smokes and the
resolver, including the empty-SMOKE_BROWSER rationale, which no longer
leans on a matrix key that does not exist.
Signed-off-by: cliffhall <cliff@futurescale.com>
#2132 was closed without the investigation going anywhere, so every
'pending #2132' pointer in the tree was about to send a reader to a
closed issue whose own diagnosis had already been retracted.
The docs now say what is actually known, and nothing more: the two App
smokes fail under Playwright's WebKit for unidentified reasons; it does
not reproduce in real Safari; an isolated repro of the suspected
mechanism did not reproduce it under Playwright's WebKit either; and
chasing it further was judged not worth the effort. A reader hitting it
locally is told to treat it as unexplained rather than as a defect.
The matrix decision is unchanged and never depended on the cause — the
engine fails, so it is not gated.
No behavior change.
Signed-off-by: cliffhall <cliff@futurescale.com>
An isolated repro of the mechanism I described — a server writing three
SSE frames then going silent with the connection held open, read via
fetch + getReader — delivers every frame in all three engines, WebKit
included. So 'the SSE stream's last message is not delivered' is not a
supported description of the failure.
The keep-alive experiment that appeared to confirm it was one
observation, and the failure is not even stable in shape: two consecutive
webkit runs failed at different stages (ready wait vs connect wait),
which a deterministic buffering bug would not do.
What is unchanged: the App smokes really do fail under Playwright's
WebKit, which is why the engine stays out of the matrix; that reason
never depended on the mechanism. The parseSSE cross-chunk defect also
stands, being read off the code rather than inferred from an experiment.
No behavior change.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings from Copilot's suppressed block.
1. resolveBrowserName treated a present-but-empty SMOKE_BROWSER as unset
and returned chromium. The test that covered it even documented WHY
that is dangerous — GitHub Actions renders an undefined matrix key as
"" rather than omitting the variable — and then asserted the unsafe
behavior. A misspelled matrix key would run chromium inside a job named
for another engine and report green: the same false-coverage outcome
the unrecognized-value branch exists to prevent, reached by a typo
instead of a bad value.
Only an absent variable now selects the default. Empty throws, naming
the likely cause and the actual remedy ('unset it', which is not the
same as setting it to nothing). The workflow always renders a real
value, so nothing in CI is affected.
2. install-smoke-browser.mjs had no tests. Its I/O half runs on every
smoke and fails loudly; its argument half does not — every caller
passes nothing or the literal 'chromium', so the precedence rule is
asserted by nothing at runtime. It matters: pack:verify passes chromium
BECAUSE it launches chromium, and if the environment won,
SMOKE_BROWSER=webkit npm run pack:verify would install one engine and
launch another, surfacing as a misleading "Executable doesn't exist".
resolveRequestedBrowser is now exported and pure; execution moved
behind main() with the entrypoint guard verify-typecheck-coverage.mjs
uses, so importing the module for tests cannot trigger an install.
Both mutation-verified: restoring either behavior fails the tests that
claim to cover it.
Addresses the suppressed comments on Copilot's fifth review of #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings from Copilot's suppressed block.
1. The launch-failure message is #2086's acceptance criterion — a missing
browser must name THAT engine and its own install command — and it had
no test. The smokes cannot supply one: install-smoke-browser fetches
the binary first, so a passing smoke says nothing about what a failing
one prints, and that message is the deliverable. loadBrowser now takes
an injectable loadPlaywright purely to open that seam.
The test asserts per engine that the message leads with that engine,
names --with-deps <that engine> and no other, and preserves the
underlying cause. Verified by mutation: hard-coding 'chromium' in the
message fails it. It also pins the two branches apart — an unresolvable
package must say
> @modelcontextprotocol/inspector@2.3.0 postinstall
> node scripts/install-clients.mjs
[install-clients] Installing dependencies for clients/web...
up to date, audited 583 packages in 614ms
198 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
[install-clients] Installing dependencies for clients/cli...
up to date, audited 237 packages in 496ms
69 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
[install-clients] Installing dependencies for clients/tui...
up to date, audited 314 packages in 518ms
102 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
[install-clients] Installing dependencies for clients/launcher...
up to date, audited 160 packages in 449ms
50 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
up to date, audited 281 packages in 3s
108 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities and must NOT say ,
which fetches binaries and cannot fix a missing package.
The unsupported-engine test now also asserts loadPlaywright was never
called, pinning the ORDER rather than only the message: validating
first is what makes the error name the real mistake.
2. prod-web-server.mjs's header still described the prefix this PR removed. Corrected, keeping why
deriving paths from import.meta.url is still right, and adding the
elicitation smoke to the list of callers it had also outgrown.
Addresses the suppressed comments on Copilot's review of #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
#2132 was filed asserting that a Safari user opening an MCP App gets a
permanently 'loading' widget. That was never tested. A manual check in
real Safari renders the App normally, so the claim is unsupported and is
removed from all six places it reached.
The failure under Playwright's WebKit is real and still keeps that engine
out of the CI matrix. What is not established is that any shipping
browser behaves this way — and Playwright ships its own WebKit build
whose networking stack is not Safari's, which is exactly the layer the
failure lives in.
headless-browser.mjs already carried the caveat that a green WebKit run
is not a Safari guarantee. The same sentence forbids the inverse
inference, which is the one that was made; it now says so explicitly, and
AGENTS.md carries the rule: reproduce in the real browser before claiming
user impact in one.
No behavior change.
Signed-off-by: cliffhall <cliff@futurescale.com>
The three smoke headers and one AGENTS.md bullet still said CI runs all
three engines. That was written before pointing the smokes at WebKit
found #2132; the matrix was then narrowed to Firefox and these were not
brought along, so they claimed a gate that does not exist. Each now says
what CI actually enforces — Chromium in the build job's `npm run smoke`,
Firefox in the Sandbox smokes matrix — and where WebKit stands.
smoke-web-browser.mjs additionally notes that IT passes in WebKit; the
two App smokes are the ones that do not, and a reader of this file should
not infer otherwise from a bare '#2132' reference.
The fail-fast: false rationale no longer cites a WebKit-vs-Firefox
example it cannot currently produce, and says why the flag is still right
for a one-entry matrix.
Addresses Copilot review on #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
Every browser-driven check in this repo launched Chromium and nothing else.
That is fine for most of the web client, whose behavior is React and Mantine.
It is not fine for the MCP Apps sandbox, which is built out of the primitives
that genuinely diverge between engines: a CSP <meta> injected as the first
<head> child of a srcdoc document, a nested sandboxed iframe, a
Permissions-Policy allow attribute, and postMessage origin discipline across
those two frames. Nothing else covers that surface — sandbox-csp.test.ts
asserts which policy STRING is built (environment-independent by
construction), and no Storybook story reaches the sandbox at all, since all
three App stories use a data: placeholder iframe and a mock bridge.
The engine is now a parameter:
SMOKE_BROWSER=webkit npm run smoke:web:app
SMOKE_BROWSER=firefox npm run smoke:web:engine # all three smokes
Unset means chromium, so `npm run ci` and `npm run smoke` are unchanged. An
unrecognized value is an error rather than a fallback — falling back would
report a green Chromium run under a job labelled "webkit", claiming coverage
that never ran — and a launch failure names the engine that failed and its own
`playwright install` invocation.
Selection, launching, and the fatal-vs-benign page-diagnostics split now live
in one shared scripts/lib/headless-browser.mjs. The three smokes had each
hand-rolled the diagnostics split and two had their own copy of loadChromium;
pack:verify goes through the same helper, pinned to Chromium explicitly since
it is a packaging check.
The npm scripts drop their `cd clients/web && npx playwright install chromium`
prefix for scripts/install-smoke-browser.mjs. The engine is a variable now, and
a ${VAR:-default} expansion does not expand under Windows' cmd.exe; npx is also
a .cmd shim there that a shell-free spawn cannot start, so this resolves the
Playwright CLI through resolveNodeBin — the #1939 problem solved the #1939 way.
CI gains a `Sandbox smokes (<engine>)` matrix job rather than matrixing `build`,
which also runs validate, the coverage gate, two verify gates and Storybook —
all engine-independent. Both publish jobs now need it.
WebKit is supported by the tooling but deliberately NOT in the CI matrix yet:
pointing the smokes at it immediately found #2132, a pre-existing Safari
incompatibility in the web client's SSE transport that strands the last message
on /api/mcp/events, so an MCP App never leaves "loading". That is a product bug
affecting real Safari users, with its own diagnosis and a verified fix, and it
is not a matrix change. Adding `webkit` here is a one-word diff once it lands.
An engine is either green in CI or absent from it; a continue-on-error job would
report coverage nobody is held to.
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 4. Two leftovers from moving the build into the shared helper, both
invisible here because `scripts/` is prettier-gated but not ESLint-gated,
so an unused binding fails nothing:
- smoke-web-elicitation.mjs imported `existsSync` solely for the local
early return that is gone.
- pack-and-verify.mjs kept `composableServer` and its `composableServerPath`
import for the same reason; the helper now asserts that entry via
`requires: ["composable"]`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 3's suppressed finding, and it corrects a claim of mine that was
simply wrong. The header said a deleted source file's stale `.js` produces
"a loud failure rather than a silent one — an import of a removed module
fails at the consumer". It does not: the stale file is still there, so the
post-build existence checks pass against it and anything still importing
that module keeps silently running the old code. That is the SAME failure
class this PR exists to close, described as though it were the opposite.
Rewritten in the helper header and AGENTS.md to say the unconditional build
does not cover deletion, and to make `rm -rf test-servers/build` after a
delete or rename an instruction rather than an aside — which is now a real
clean, since the tsbuildinfo was pinned inside build/ in 8d19346.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 2's suppressed finding, and a fair one: smoke-web-app.mjs reaches the
build through `startMcpAppServer` rather than calling the helper itself, so
its header survived the sweep still claiming `test-servers/build` is "built
on demand if missing" — the exact policy this PR removes. It is also the
smoke that found the defect, so the header now says so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings from review round 1, both real.
The default incremental cache path derives from `rootDir`, so it landed at
`test-servers/tsconfig.tsbuildinfo` — beside the config, OUTSIDE `build/`.
That survives the `rm -rf test-servers/build` this project documents as the
remedy for a deleted source file, leaving tsc convinced its outputs are up
to date and emitting nothing. Pinned to `./build/tsconfig.tsbuildinfo`, and
verified that removing the output dir now forces a full re-emit.
The build is memoized per repo root; the existence assertion was memoized
with it. A later caller naming an entry the first did not would be handed a
path tsc never emitted, then fail with exactly the opaque
ERR_MODULE_NOT_FOUND the helper exists to replace. The assertion now runs on
every call.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Five `scripts/*.mjs` consumers each carried their own `ensureTestServer()`
that returned early when `test-servers/build` was already on disk. Presence
is not freshness: once that directory exists — which it does on any machine
that has run the smokes once — an edit to `test-servers/src` was never
picked up again, and the smoke went on driving the previously-built fixture.
The failure is silent and misattributed. It does not read as "your fixture
is stale"; it reads as a product bug in whatever the smoke is checking.
`scripts/lib/ensure-test-servers.mjs` is now the one place that build is
produced for a script consumer, and it builds unconditionally — memoized
once per process per repo root, so pack-and-verify's two asks stay one tsc
pass while a fresh run always rebuilds. Callers name the entries they
depend on and get one actionable failure message, replacing five that had
drifted. pack-and-verify also stops shelling out to `npx tsc`, which is
what made it the one script unrunnable on Windows (#1939).
`test-servers/tsconfig.json` gains `incremental` so the up-to-date case is
cheap: 3.0s cold, 1.6s incremental here.
Unconditional emit is still not a clean — `--noCheck` leaves the stale .js
of a deleted src file behind — which is unchanged from before and recorded
in the helper's header and AGENTS.md.
Closes#2111
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
One conflict, scripts/smoke-web-app.mjs: #2056 added a second phase to the
smoke (the same app declaring _meta.ui.domain, asserting it renders from the
dedicated app origin rather than an opaque one) while this branch extracted
the smoke's flow into scripts/lib/mcp-app-flow.mjs.
Resolved by keeping both: phase 2 now runs through the shared helper too,
which grows three knobs it needs and pack:verify does not —
- expectDeepLink: phase 2 re-navigates the SAME page, so the token gate is
already proven and a rejected re-navigate would fail at the connect wait
regardless;
- what: so a phase-2 ready timeout doesn't read as a phase-1 one;
- extraDiagnostics: phase 2 appends the frame list, which is what separates
"never rendered" from "rendered at the wrong origin".
Each is covered by a new unit test, since neither consumer exercises a
failure branch.
pack:verify deliberately drives phase 1 only. The dedicated origin is served
by the same runner the packaging assertions already cover and reads nothing
new from disk (app-origin-controller.ts has no file reads), so a second
browser launch there would buy no packaging signal.
Verified on the merged tree: smoke:web:app passes both phases (dedicated
origin http://127.0.0.1:6278/app-document/<id>), and pack:verify passes end
to end against the real tarball.
Signed-off-by: cliffhall <cliff@futurescale.com>
`frame-ancestors` is checked against every ancestor, not just the parent.
A published app document is framed by the sandbox proxy, which is framed
by the Inspector page — so admitting only the proxy's origin blocked the
frame outright (it rendered as a chrome-error frame that never reached
the bridge). `appDocumentEmbedders` now admits both.
That failure was invisible to every unit test, so `smoke:web:app` grows a
second phase: the same demo app declaring `_meta.ui.domain`, driven
through the same connect → open → ready chain, then asserting the two
facts that only hold on the dedicated path — the inner frame was
navigated to a published document, and its own `location.origin` is a
real http origin rather than "null". Both phases end at
`data-app-status="ready"`, so nothing short of reading the frame's origin
can tell them apart.
Adds `test-servers/configs/mcp-app-domain-http.json` (and a `domain`
param on the `mcp_app_demo_widget` preset) as the showcase server for
both the smoke and hand testing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Nothing covered the intersection #1859 fell through. `smoke:web:app` drives
the App path but against the repo build tree; `pack:verify` runs against the
installed tarball but only asserted the sandbox proxy page *exists*. So a
packaging regression that ships the file present-but-unreachable — a rename
with a stale reader, a wrong position relative to clients/web/build, a future
"files"/.npmignore edit — passes both.
Extract the drive-the-App-flow half of the smoke into
scripts/lib/mcp-app-flow.mjs and call it from both, rather than copying it:
the deep-link shape is the part that rots silently, and a drifted link fails
as an opaque timeout rather than a mismatch. pack:verify then drives
connect -> open app -> data-app-status="ready" in headless Chromium against
the installed `--web` boot it already starts.
The client under test comes from the install; the App test server stays a
repo fixture.
Also isolates pack:verify's `--web` catalog (the #1977 treatment the smokes
already have). Untidy while the step only did `GET /`; the App deep link
persists a server row, which would make it destructive and non-deterministic
against the developer's real ~/.mcp-inspector/mcp.json.
Unit tests cover what neither consumer can check itself — both CSRF gates and
the appArgs encoding on the deep link, and driveAppFlow's failure branches
against a stand-in page, all dead code from a happy-path smoke's view.
Verified: pack:verify passes end to end against the real tarball, and fails
at the new step alone when sandbox-controller.ts is pointed at a renamed
proxy page (the tarball and on-disk assertions both still pass — the exact
gap this closes).
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses review round 7 on #2099 (three suppressed comments).
- Proxy variables are now cleared for the WHOLE of `transport.test.ts` and
`cli.test.ts`, not just around the proxy tests. Measured: undici's
`EnvHttpProxyAgent` captures its proxy URLs when it is CONSTRUCTED and re-reads
only NO_PROXY per request, so with the process-wide memo, any earlier test that
connects on a machine with an ambient proxy would pin the agent to it and a
later test setting HTTP_PROXY would be talking to an agent that is not
listening. Clearing up front means nothing can build the singleton before the
proxy tests do, and restoring only in the file-level afterAll means the memo
cannot be left bound to a closed test proxy.
Both suites verified green under a hostile ambient env
(`HTTP_PROXY`/`http_proxy`/`HTTPS_PROXY` pointed at a dead port). In fairness
this is hardening rather than a fixed failure: the concrete cross-test break
did not reproduce with the current test order — but it depends on test order
and on the developer's environment, which is exactly what this removes.
- The guard's failure message said a package "is declared external but was
inlined" even for a candidate that came only from the root dependencies —
i.e. #2067's own shape, where the missing `external` entry IS the bug, so the
message contradicted the config it was asking the reader to check. It now
words the failure by candidate source, and says to add the entry.
The capture-at-construction behavior is documented on `createProxyFetch`, since
it is the reason the suites are structured this way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses review round 4 on #2099 — three posted comments and three suppressed
ones, all valid.
- A rebuilt Response reported `url: ""`, `redirected: false`, and
`type: "default"` even after undici followed a redirect: a fetch-contract
regression visible only to proxy users, and one `core/auth/endpointOverrides.ts`
explicitly guards against by declining to rebuild at all. This adapter has no
such choice — the response arrives from the wrong realm — so it now reinstates
the three with `defineProperty`.
- Normalizing a `Request` input dropped every option the Request itself carried
beyond method/headers/body/signal. Most visibly `redirect`: a
`new Request(url, { redirect: "manual" })` started following redirects instead
of surfacing them. All of them are now carried through, with an explicit
`init` still winning per the fetch contract.
Both are covered by tests driving a real redirect through a real proxy, and
both were confirmed to fail without the fix.
- `@modelcontextprotocol/ext-apps` was added only to the CLI's `external` list.
AGENTS.md requires every root-declared runtime dependency in all three, since
which client reaches it is a function of what `core/` imports rather than of
the client's own code. Added to the web runner and TUI lists too.
- The `BUNDLED_CLIENTS` test compared the hard-coded list against a hard-coded
copy of itself, so a new tsup-bundled client would have left it green while
going unchecked. It now discovers clients from disk (a client is tsup-bundled
iff it ships a tsup config) and compares. Verified it fails when a stub client
is added.
- AGENTS.md's project tree still described `core/mcp/node/` as only the stdio
transport factory; it now carries `proxyFetch.ts` and its role.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses the suppressed review comment on #2099, which was the sharpest of the
three rounds: the guard stopped looking at a package the moment its `external`
entry was removed — so deleting `"undici"` from the web config would have made
the pre-fix bundle pass. That is self-referential, and #2067 was precisely a
MISSING declaration, so the guard covered its regression but not the bug.
Candidates are now the union of two independently-derived lists: each client's
`external` array, and the root manifest's `dependencies` — root-declared by
rule, reached through `core/`, and required to be external in every client
bundle. Removing a package from an `external` list can no longer remove it from
scrutiny.
That immediately surfaced one latent instance of the same rule violation:
`@modelcontextprotocol/ext-apps` is root-declared and reached through
`core/mcp/apps.ts`, but was absent from the CLI's `external` list and was being
inlined into `index.js` — dragging the transitive v1 SDK and zod-to-json-schema
in with it. It is ESM, so it was not failing the way `undici` did; what it
violated was the rule. Adding it drops the CLI bundle from 590KB to 518KB.
It is also a real example of the shape the previous revision could not see: it
was inlined into the entry, emitting no `<pkg>-HASH.js` chunk at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses review round 2 on #2099.
- The /api/fetch proxy test set only uppercase HTTP_PROXY, so an ambient
lowercase `http_proxy` (which undici's EnvHttpProxyAgent prefers) or a NO_PROXY
could route the request elsewhere and fail the assertion. All six proxy vars
are now saved and cleared for the whole /api/fetch suite and restored in
afterAll — suite-level rather than per-test because the agent is memoized
process-wide once built. Verified the test still passes under a deliberately
hostile ambient env (`http_proxy=http://127.0.0.1:9 NO_PROXY='*'`).
- `verify:bundle-externals` reported success when `parseExternals` returned
nothing, so an ordinary tsup refactor — hoisting the array to a constant, or a
line break after `external:` — would silently disable the guard. It now fails
closed: every bundled client has externals, so "none parsed" means the parser
lost track of the config. Verified against exactly that refactor. The
per-client verdict moved into a pure `evaluateClient()` so all three failure
modes are unit-tested, and the failure epilogue is now printed only for a real
inlining, since "add the package to `external`" is the wrong instruction for
the other two.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses the Copilot review on #2099.
- `/api/fetch` used the bare global `fetch`. That route is the browser's only
way out to the network — the web client's `environment.fetch` is
`createRemoteFetch()`, which forwards OAuth discovery and token requests
there — so a corporate-proxy user could connect to a server but never
authorize against it, and the PR's "OAuth is proxied now" claim held for the
CLI and TUI but not for web. It now uses the same proxy-aware base fetch, with
an integration test driving a real forwarding proxy (confirmed failing without
the fix).
- `verify:bundle-externals` only recognized an inlining that produced a
`<pkg>-HASH.js` chunk. A *statically* imported package is folded into
`index.js` and emits no chunk, so it would have passed. Detection now keys off
esbuild's `// <path>` module banners, covering both shapes. Because that
depends on unminified output, a bundle with no banners now fails as such
rather than reporting clean — enabling `minify` cannot silently retire the
check. Tests cover the entry-inlined case, the same-prefix case
(`undici-types` must not read as `undici`), and the no-banner case.
- The pre-push pipeline summaries in AGENTS.md, README.md, and
.github/copilot-instructions.md still described `verify:build-gate → smoke`
and omitted the new gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Corporate-proxy users could not connect to any remote MCP server from v2.
Two independent defects, both reported in #2067.
1. The web and TUI bundles inlined undici. It was declared in the root and
`clients/cli` manifests but named in no client's tsup `external` list, and
tsup auto-externalizes only what the *nearest* manifest declares — so
esbuild inlined 1.05MB of it into `clients/{web,tui}/build/`. undici is
CommonJS, so the emitted chunk's `require("assert")` hit esbuild's ESM
`__require` shim and threw `Dynamic require of "assert" is not supported`.
The rejected `import()` surfaced as "the `undici` package is not available",
telling users to install a package that was already present — and could not
have helped, because the specifier had been rewritten to a relative chunk at
build time.
2. `withProxyDispatcher` injected a *userland* undici `EnvHttpProxyAgent` into
Node's *built-in* fetch. That couples two different copies of undici at the
dispatcher handler interface, which is not stable across majors: Node 22
embeds undici 6, Node 24 embeds 7, Node 26 embeds 8. A userland undici 8
agent handed a built-in undici 7 handler is rejected with
`fetch failed: invalid onRequestStart method`, and the request never leaves
the process. So fixing the bundling alone would only have converted the
web/TUI failure into the CLI's.
Both halves of the pair now come from userland undici — its `fetch` and its
`EnvHttpProxyAgent` — which keeps the handler inside one copy and works
unchanged from the Node 22.19 engine floor through Node 26. Deleting the
wrapper in favor of Node's native `NODE_USE_ENV_PROXY` was not an option:
measured, it is unsupported on 22.19 (the floor) and only lands mid-22.x.
Because undici's `Response` is a different class from `globalThis.Response`,
the proxied fetch re-wraps each response as a genuine global `Response`,
streaming preserved. Without that, `res instanceof Response` is false for
callers that test it — including the SDK's OAuth error formatter, which would
degrade every message to `Raw body: [object Response]`.
The proxy now sits at the bottom of the fetch stack (`environment.fetch` for
the Node clients, and the `createTransportNode` default for the web backend,
its one caller that supplies no `fetchFn`) rather than wrapping the transport's
fetch. Proxying means *substituting* the fetch, not decorating one, so wrapping
a supplied `fetchFn` would discard it; installing it underneath lets
InspectorClient's own wrappers compose over it. A side effect worth having:
OAuth discovery and token requests go through `environment.fetch` too, and were
never proxied before.
Guarding it:
- `undici` is added to all three tsup `external` lists and removed from
`clients/cli/package.json`, so it is root-declared only, per the placement
rule in AGENTS.md. The two installs had already drifted (root 8.5.0 vs cli
8.9.0).
- New `npm run verify:bundle-externals` fails if any package a client declares
external was inlined into that client's bundle anyway. It reads the *built
output*, not the config — those two disagreed for four releases — and derives
what to check from each client's own `external` array, so a newly
externalized package is covered without editing the guard. Verified against
the real pre-fix artifact, which it flags.
- The proxy tests now drive a real forwarding proxy rather than a mock. That
matters: defect 2 was a runtime interface mismatch, which no mock can catch
because no mock ever dispatches.
AGENTS.md's must-not-bundle rule (added by #2082) is extended with why undici
slipped past it, and notes the `node -e` trap — probing the bundled chunk with
`node -e` falsely succeeds, because it exposes a global `require` that
satisfies esbuild's guard.
Closes#2067
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot's suppressed round-3 comment is right. The attempt-counted loop read
the output and then slept, so nothing ever observed the last interval: an
announcement arriving at 29.9s of a 30s budget was reported 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.
Replace it with a deadline loop that re-reads state after every wait, the last
one included, and clamp the final wait to the deadline so the budget is a real
bound rather than one poll longer.
The regression test sets pollMs === timeoutMs, which makes the distinction
deterministic rather than a race: 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 the announcement. The child announces
immediately, ~2s inside the window, so a loaded runner can't flip it. Verified
both directions — the test fails against the old loop and passes against the
new one — and `npm run test:scripts` was repeated to confirm it is stable under
the full 116-test parallel run (an earlier margin-based version of this test
was not).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Two review findings from round 2, both correct.
1. The regression suite killed its child only after the assertions. If
`assert.rejects` or either assertion failed, execution never reached the
kill, leaving a 60-second child attached to the runner — a suite guarding a
process leak would leak one itself, precisely on the run where the
regression had come back. Teardown is now registered from `onSpawn` via
`t.after`, so it runs on every path out of every case. That is the helper's
own ownership rule applied to the tests; killing an already-exited child is
a no-op, so the early-exit cases pay nothing for it.
2. Document the new shared helper, per the maintenance rules. AGENTS.md's
scripts/lib tree entry now carries announced-child.mjs and what its contract
buys, and the smoke:web:app paragraph records why the spawn/readiness step
moved out of the smoke (same reason teardownWebServer left prod-web-server's
stop()). README's test:scripts row names the new suite alongside
resolve-node-bin's.
Not mirrored into .github/copilot-instructions.md: that file is a distillation
of the rules a reviewer cites against a diff, and deliberately excludes the
project-structure tree and tooling inventory.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot's review is right that the fix was unproven: smoke:web:app's happy path
always receives the announcement, so `npm run ci` never exercised a child that
stays alive *through* the readiness timeout — exactly the 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.
Extract the spawn/readiness ownership into scripts/lib/announced-child.mjs
(the same move prod-web-server.mjs made for teardownWebServer, and for the same
reason), and add scripts/lib/announced-child.test.mjs under `test:scripts`.
The tests drive real `node -e` children rather than spies, so the assertion is
that a real process is reachable and killable, not that a mock was called: the
timeout case asserts the child was published before the wait, is still alive
when the throw lands, and is actually stopped by kill. The early-exit and
spawn-failure paths are covered alongside it, plus the both-streams scan.
smoke-web-app.mjs keeps its behavior — startMcpServer() now delegates the
spawn and publishes the child via `onSpawn`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
1. clients/web/.npmignore's explanatory comment still said the root "files"
allowlist restricts publishing to build/ and dist/ and that everything else
under clients/web "stays out regardless". #1934 added clients/web/static to
that allowlist, so the claim was false — in a file that exists precisely
because a packaging contract was misread once. Name static/ and say why it
ships (read from disk at runtime by sandbox-controller.ts, at a path
relative to the runner dir).
2. Replace the four unjustified `as unknown as Response` doubles in
useServers.test.tsx with real Responses over real ReadableStreams: a
null-body Response for the `!res.body` guard, a stream whose pull throws for
the reader-rejects case, and pull-driven streams for the multi-frame and
priming-comment cases (pull runs once per read, so the counting and the
blocking second read behave exactly as the hand-rolled reader doubles did).
The doubles are now structurally type-checked against the Response API.
3. scripts/smoke-web-app.mjs orphaned the MCP test server on a readiness
timeout: startMcpServer() returned the child only once the announcement
line matched, so a child that was alive but never announced left `mcpServer`
unassigned, shutdown() could not stop it, and process.exit(1) left it
holding its port. Publish the child to `mcpServer` the moment it is spawned,
so every throw path is covered by teardown.
Closes#2000
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Eight findings from the Copilot review, all accepted.
Ownership across connections was the substantive one, in three parts. A
mid-session transport close reaches teardown only through
`clearAndAnnouncePendingPeerRequests`, whose emptiness check looked at the
native queues alone — so a lone app elicitation survived a dropped connection
with its modal still open; the check now counts the active set too. The web
bridge factory resolves its client at call time, so an entry queued by a
replaced `InspectorClient` could rebuild against the *next* one and answer
through a different server; the host now drops every entry synchronously on
the swap (`AppElicitationController.failAll`) rather than racing core's
awaited teardown. And the request id was only monotonic within one client, so
each replacement client's first request was `app-elicitation-1` — settling
that id could resolve the wrong server's request. It now carries a
per-instance prefix.
Result validation now parses the whole value with `ElicitResultSchema` before
the requested-schema check, instead of inspecting `action` by hand: a
`{ action: "decline", content: { x: {} } }` used to pass, though the standard
result permits only primitives and string arrays in `content`.
The dialog's accessible name now includes the request id and the prompt — two
concurrent elicitations can be for the same app URI, and the previous name
made those indistinguishable, which is what the comment beside it claimed to
prevent.
`observeAppCapabilities` takes a minimal structural `onmessage` contract
(generic over the message type, since a handler typed for a narrower message
is not assignable to one typed for `unknown`), removing the double cast at the
call site.
`smoke:web:elicit` now fails on an uncaught page error after a successful
drive, and captures the async half of that class off the console channel, as
its two sibling smokes do — it could otherwise print OK over a broken bundle.
Adds the tests each fix needs, including the two the review asked for
directly: that `advertiseElicitation` reaches the `AppBridge` constructor, and
that it is absent by default.
Signed-off-by: cliffhall <cliff@futurescale.com>
Let a server hand a form `elicitation/create` to an MCP App and return the
app's ordinary `ElicitResult`, with the native elicitation form as the
fallback on every failure.
No second extension, no custom method, and no custom result shape: the only
new wire surface is a nested `elicitation` flag on the existing
`io.modelcontextprotocol/ui` extension on each side, plus
`_meta.ui.resourceUri` on the request. App rendering is selected only when
all four gates hold — client `elicitation.form`, client MCP Apps MIME type,
the nested `elicitation` setting on BOTH peers, and a valid absolute `ui://`
URI on the request.
Only the web client advertises the nested client-side setting, and only
because it has a sandbox renderer: supplying
`InspectorClientOptions.appElicitation` is what opts a client in, so CLI and
TUI keep advertising the MIME type without ever claiming they can resolve an
elicitation through an app.
Routing lives in the one funnel both entry points already use
(`enqueuePendingElicitation`), so the inbound handler and the MRTR driver
cannot diverge. Ownership is request-scoped — a per-request id keys the
renderer, iframe and bridge — so two concurrent elicitations can never
resolve through each other's bridges. An explicit `decline`/`cancel` is a
completed elicitation and goes back to the server; everything else (absent or
malformed metadata, resource/sandbox/bridge failure, an app with no
elicitation capability, a timeout, an invalid result or one that fails the
requested schema) falls back.
The Inspector speaks the ext-apps#733 / SEP-3118 wire protocol but cannot yet
consume its helpers — the released `@modelcontextprotocol/ext-apps` (1.7.5)
predates that PR. `core/mcp/appElicitation.ts` and
`AppRenderer/requestAppElicitation.ts` mirror it exactly and are marked for
deletion once a release containing it ships. One consequence needed its own
seam: ext-apps 1.7.5 parses the view's `ui/initialize` through a schema that
strips `elicitation`, so an app that correctly advertises it would look like
one that did not, silently turning every negotiated elicitation into a
fallback. `AppRenderer/appCapabilities.ts` records the raw frame instead, and
prefers the bridge's own value once it carries the key.
`AppRenderer` now takes an `AppRenderSource` union rather than a `Tool`, so
the same renderer and bridge factory serve an App tool and an elicitation
without either faking the other's shape.
Adds the public fixture (`app_choose_option` + a self-contained
`ui://demo/choose-option.html` app covering accept/decline/cancel), its two
showcase configs, and `smoke:web:elicit`, which drives the negotiated path
end to end in headless Chromium and then the same tool against a server that
never advertised the capability, asserting the native form takes it.
Closes#1854
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3, all three real:
- `require.resolve.paths()` also appends Node's GLOBAL FOLDERS
($HOME/.node_modules, $HOME/.node_libraries, $PREFIX/lib/node) and any
NODE_PATH entries, so round 2's lookup could satisfy a missing repo
install from a GLOBALLY installed typescript/vite — measuring the
programs with the wrong compiler instead of producing the actionable
"run npm install" this helper promises. Filter to the node_modules
directories on fromDir's own ancestor chain.
- The quoting predicate missed characters cmd.exe still parses, notably
parentheses (`C:\Temp(1)`), and quoting cannot suppress `%VAR%` at all
— expansion happens before quotes are processed. Extracted to
lib/win-shell-args.mjs with a broad predicate and a hard error on `%`
rather than a silently different path. `platform` is a parameter so the
Windows behavior is exercised on the POSIX machines that run the suite.
- AGENTS.md claimed pack-and-verify spawns via `process.execPath`; it does
not, and cannot — its children are `npm` and the installed `.bin` shim.
State the exception and why.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01387r3MotGk52hrkfjzHFsN
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2 (suppressed comments), all three real:
- `require.resolve("<pkg>/package.json")` is governed by the package's
`exports`, and Node keeps no special case for `./package.json` — so a
package declaring an `exports` map without it threw
ERR_PACKAGE_PATH_NOT_EXPORTED even though its bin was installed and
spawnable, a false "not installed" pointing at `npm install` for a
package already on disk. Walk `resolve.paths()` instead, which never
consults `exports`. The docblock claiming the opposite is corrected.
- `shell: true` (needed at all on Windows to start the `.cmd` shims) makes
Node hand `cmd.exe` one space-joined string, so an argument holding a
path with a space splits in two. Every generated path in
pack-and-verify lives under `tmpdir()`, which sits beneath the user
profile. Quote them via `shellArgs()` at all four spawn sites, the
command path included — the hazard predates this PR, so fix the class.
- Document the new module: `scripts/lib/resolve-node-bin.mjs` in AGENTS.md's
scripts/ tree, its tests in the README's `test:scripts` row.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01387r3MotGk52hrkfjzHFsN
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review on #1997:
- A string-form `bin` is npm's shorthand for ONE command, named after the
package, so ignoring `binName` let a typo silently resolve the package's
only executable while bypassing the documented "declares no such bin"
failure. Match it against the manifest's unscoped name instead.
- A declared bin whose file is absent (partial install) was returned
unchecked. `process.execPath <missing>` still spawns and exits 1 with
empty stdout, which `rawProjectListing` records as "no diagnostic
captured" — reproducing the bogus every-file-uncovered report this
helper exists to eliminate. Throw here, where the remedy is actionable.
Also restores `rawProjectListing`'s docblock, which the new `tscEntry`
block had orphaned above itself.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01387r3MotGk52hrkfjzHFsN
Signed-off-by: cliffhall <cliff@futurescale.com>
Upstream #1965 moved the tsc --listFilesOnly machinery into
scripts/lib/tsc-program.mjs (shared with the new verify:dep-lockstep),
still spawning `npx` — which is ENOENT on Windows. Resolve the conflict
by taking upstream's factoring and porting the #1939 fix into the lib:
rawProjectListing and verify-typecheck-coverage's --showConfig pass now
spawn process.execPath with the cached, hard-failing tscEntry resolver
(exported from tsc-program.mjs). Fixture copies in both main tests pick
up the new lib import graph. verify:dep-lockstep now runs on Windows
too.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two findings from the Copilot review of #1965, both real.
1. The candidates were flattened to package names before the version
comparison, so a third install holding a different copy failed the guard
even when no program loads that copy beside another — and the diagnostic
named an install that never took part. `crossInstallPackages` now returns
the co-occurrences themselves (name → program → install → entry paths) and
`findSkew` compares per program, across only the installs that met there.
2. Folding a nested copy onto its outermost install lost the path that
identifies which copy was loaded, and the version was then read from the
install's top-level entry — absent or differently versioned, either way
letting a real pair pass. `classifyModulePath` now also returns the
`entryPath` npm keys the lockfile by, `topLevelLockVersions` becomes
`lockVersionsByPath`, and each holder is priced from the entry the program
resolved. A resolved copy with no lockfile entry is now a hard failure
rather than a silently dropped holder.
The failure output prints `in <program>` with each holder's install path
beside its version, so a nested copy is distinguishable from a top-level one.
Signed-off-by: cliffhall <cliff@futurescale.com>
`verify:dep-lockstep` derived its candidate set from the packages the shared
first-party sources named *directly*, so a package whose declarations reach a
program only through another package's `.d.ts` was invisible to it — it could
skew across installs and recreate the two-copy blow-up #1896 was about while
the guard stayed green. `@modelcontextprotocol/sdk` was the live example: never
written in first-party code, yet 16 of its `.d.ts` files land in clients/web's
test program.
Replace the derivation with the measured one: list every client tsconfig
project with `tsc --listFilesOnly`, map each resolved `node_modules` file to
its owning install and package, and keep the packages that reach ONE program
from TWO installs. That is exactly the set that can put two structurally
distinct copies of a type in front of one checker. The rest of the guard is
unchanged — top-level lockfile comparison, deny-by-default, `TOLERATED_SKEW` as
a name allowlist scoped to within-a-major.
The listing machinery moves to `scripts/lib/tsc-program.mjs`, shared with
`verify:typecheck-coverage`, so the two guards can't disagree about what a
program contains. It is deliberately not cached to disk between the two
processes: a stale fingerprint would make a guard measure a program that no
longer exists and pass on a real miss. The second pass costs ~14s.
`TOLERATED_SKEW` is now empty. `jose` and `@modelcontextprotocol/ext-apps` are
root-only since #1970 and cannot skew; `react` (whose types are `@types/react`,
resolved from one install) and `hono` never reach one program from two
installs. Each would be a rationale for a skew that cannot occur.
Closes#1965
Signed-off-by: cliffhall <cliff@futurescale.com>
On Windows npx/npm are .cmd shims a shell-free execFileSync/spawnSync
cannot start (ENOENT since the CVE-2024-27980 hardening), so
verify:typecheck-coverage died at validate's second step — doubly
silently, echoing "(no diagnostic captured)" per project and then
reporting all 918 tracked files as getting no tsc pass — and
verify:build-gate, the smoke test-server bootstraps, and pack:verify's
npm pack failed the same way.
Add scripts/lib/resolve-node-bin.mjs: resolves the JS entry behind a
package's bin from <pkg>/package.json (deep resolution is blocked by
Vite 8's exports map) and spawns it via process.execPath — the same
walk npx --no-install did, cross-platform and shell-free. Switch the
six npx call sites to it; the npm pack call gets the existing
shell-on-win32 idiom instead (npm has no in-tree package to resolve).
An unresolvable tsc is now a hard "cannot measure" error with
actionable stderr, pinned by a new main() regression test.
Closes#1939
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both round-3 review points, both mine.
1. The docblock claimed deleting `removeSafe` would leave "both the unit suite
and all three smokes green" — written before the test existed and never
updated, so it contradicted the test added in the same PR. Only the smokes
would stay green; the unit tests are precisely what fails on it.
2. `assert.ok(hasExited(child))` after the await proved only that the child was
dead by the time teardown returned. It passes just as happily if the catalog
were removed *first*, or while an un-awaited `stopChild` was still pending —
so it did not lock down the #1801 ordering it claimed to.
Sample the directory from the child's own `exit` handler instead, registered
before teardown so it runs ahead of stopChild's listener and observes the
world at the instant the child dies. `removeSafe` cannot have run by then: it
is sequenced after the promise stopChild resolves from that very event.
Mutation-checked against both defects the old assertion missed — removing before
stopping, and dropping the await. Each now fails the test; neither did before.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot was right that the previous test proved only that `removeSafe` can
delete a directory — it never drove `stop()`, so deleting the cleanup call
would have left the unit suite AND all three smokes green. That is exactly the
regression these tests exist to catch, and they did not catch it.
Extract the teardown into an exported `teardownWebServer({ child, catalogDir,
label })`, which `stop()` now delegates to, and drive it against a stand-in
child process (a real `node -e setInterval` that stays alive until signalled)
rather than a mock. Three cases:
- it removes the catalog dir it was given (asserted on the dir itself, not a
spy, so the assertion cannot drift from the behavior);
- it awaits the child's exit before removing — the #1801 race in one
assertion, since losing the await is otherwise invisible;
- an already-dead child is not an error, which is the failure path a smoke
takes when it calls fail() after the launcher has crashed.
Mutation-checked: with the `removeSafe` call deleted, 2 of the 3 now fail
(previously: 0). AGENTS.md updated to match.
Signed-off-by is now on every commit (#1978 DCO); the requirement itself is
undocumented and tracked in #1979.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
