modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 4

fix(scripts): make smoke:tui assert the TUI survives its first frame (#2147)

Browse source 
`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>
This commit is contained in:
cliffhall 2026-08-26 21:00:16 -04:00
commit b6baa30d7b
1 changed files with 757 additions and 192 deletions
Download patch file Download diff file Expand all files Collapse all files

290
scripts/smoke-tui.mjs
View file

@ -6,13 +6,28 @@
* the Ink app, so the launcher → TUI → core path and `--catalog` loading went
* unverified by the launcher smokes. This script launches the built launcher in
* `--tui` mode against a temp `--catalog`, waits for the app to render its first
* frame (the "MCP Servers" panel) within a timeout, then sends SIGTERM and
* exits.
* frame (the "MCP Servers" panel), waits again to confirm it is *still running*,
* then sends SIGTERM and exits.
*
* It asserts the TUI *boots and renders* without crashing — not full
* interaction (driving an Ink UI deterministically in CI is flaky, so this is
* intentionally a shallow render check). Exits non-zero on a crash-before-render
* or a render timeout.
* It asserts the TUI boots, renders, and keeps running — not full interaction
* (driving an Ink UI deterministically is flaky, so this is intentionally a
* shallow check). Exits non-zero on a crash before or after first paint, or on
* a render timeout.
*
* ## Why it runs under a pseudoterminal (#2147)
*
* It used to spawn the TUI with its stdin on `/dev/null` and settle OK the
* moment the marker appeared. Ink mounts `useInput`, `useInput` needs raw mode,
* and raw mode is a property of the file descriptor — so the TUI painted one
* frame and died ~40ms later with "Raw mode is not supported on the current
* process.stdin". The smoke won that race and reported OK, on every machine,
* TTY or not: what it actually asserted was *first paint*, not survival.
*
* Two changes, and the second is the fix: `scripts/lib/pty.mjs` gives the child
* a real terminal via `script(1)`, and `scripts/lib/render-smoke.mjs` requires
* it to outlive its first frame. Without the survival assertion a future
* harness change reintroduces exactly this false green and nothing notices —
* which is why that assertion lives in a tested module rather than here.
*
* Expects `clients/launcher/build` and `clients/tui/build` to be built first
* (the validate / CI ordering guarantees this). The bundled stdio test server
@ -20,39 +35,63 @@
* `scripts/lib/ensure-test-servers.mjs` for why presence isn't freshness.
*/
import { spawn } from "node:child_process";
import { mkdtempSync, existsSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { join, resolve } from "node:path";
import { removeSafe } from "./lib/child-cleanup.mjs";
import { ensureTestServers } from "./lib/ensure-test-servers.mjs";
import { resolvePtyWrapper } from "./lib/pty.mjs";
import { runRenderSmoke } from "./lib/render-smoke.mjs";
const repoRoot = resolve(import.meta.dirname, "..");
const launcher = join(repoRoot, "clients", "launcher", "build", "index.js");
const RENDER_MARKER = "MCP Servers";
const TIMEOUT_MS = Number(process.env.SMOKE_TUI_TIMEOUT_MS ?? 15000);
// How long past first paint the TUI must still be running. Generous relative to
// the failure it catches (the raw-mode crash landed ~40ms after the marker) —
// the cost of a longer window is only wall-clock on a passing run.
const SURVIVE_MS = Number(process.env.SMOKE_TUI_SURVIVE_MS ?? 2000);
const EXIT_GRACE_MS = Number(process.env.SMOKE_TUI_EXIT_GRACE_MS ?? 5000);
// Ink's own error text when it is asked for raw mode on a non-terminal fd.
const RAW_MODE_ERROR = /Raw mode is not supported/;
function fail(message) {
console.error(`smoke:tui FAILED — ${message}`);
process.exit(1);
}
// The Ink TUI requires a real TTY for raw-mode keyboard input. Headless CI has
// none, so the app throws "Raw mode is not supported" on mount and exits before
// it can render its first frame — making this boot/render check inherently a
// LOCAL-only smoke (its own design notes call driving Ink in CI flaky). Skip it
// under CI rather than fail spuriously: the TUI is still built and unit-tested
// there; only this terminal-dependent render check is local-only.
if (process.env.CI) {
console.log(
"smoke:tui SKIPPED — Ink needs a real TTY (raw mode), unavailable in CI; run it locally",
);
function skip(message) {
console.log(`smoke:tui SKIPPED — ${message}`);
process.exit(0);
}
// Kept out of GitHub CI by decision, not by capability. The PTY above removes
// the technical blocker this skip used to cite (a headless runner has no TTY),
// but whether this smoke joins CI is a separate call for the maintainers, and
// #2146 is deliberately keeping the other local-only smokes out. Making it
// *valid* stands on its own: it is a local-only gate, which is exactly where a
// false green is least likely to be caught by anything else.
if (process.env.CI) {
skip(
"local-only by decision (see the header); the TUI is built and unit-tested in CI",
);
}
if (!existsSync(launcher)) {
fail(`launcher build not found at ${launcher} — run \`npm run build\` first`);
}
// No `script(1)` (Windows). Running without a PTY is not a weaker check, it is
// a guaranteed failure — so say why rather than report a crash as a defect.
// `node-pty` is the portable answer if this ever needs to run there.
const pty = resolvePtyWrapper();
if (!pty) {
skip(
`no pseudoterminal available on ${process.platform} — the Ink TUI needs raw mode, ` +
`which requires a real terminal fd (see scripts/lib/pty.mjs)`,
);
}
// Rebuilt on every run — presence is not freshness (#2111).
let testServer;
try {
@ -76,183 +115,58 @@ writeFileSync(
}),
);
const child = spawn(
process.execPath,
[launcher, "--tui", "--catalog", catalogPath],
{
cwd: repoRoot,
// Redirect HOME so the TUI's storage never touches the real ~/.mcp-inspector.
// Pin MCP_OAUTH_CALLBACK_URL="" (empty reads as unset) so an ambient
// non-loopback value can't crash the TUI before render via the loopback
// callback guard — same class smoke-cli.mjs's SMOKE_BASE_ENV neutralizes.
env: {
...process.env,
MCP_OAUTH_CALLBACK_URL: "",
HOME: work,
USERPROFILE: work,
const { command, args } = pty.wrap({
command: process.execPath,
args: [launcher, "--tui", "--catalog", catalogPath],
});
let result;
try {
result = await runRenderSmoke({
command,
args,
marker: RENDER_MARKER,
timeoutMs: TIMEOUT_MS,
surviveMs: SURVIVE_MS,
exitGraceMs: EXIT_GRACE_MS,
spawnOptions: {
cwd: repoRoot,
// Redirect HOME so the TUI's storage never touches the real
// ~/.mcp-inspector. Pin MCP_OAUTH_CALLBACK_URL="" (empty reads as unset)
// so an ambient non-loopback value can't crash the TUI before render via
// the loopback callback guard — same class smoke-cli.mjs's
// SMOKE_BASE_ENV neutralizes.
env: {
...process.env,
MCP_OAUTH_CALLBACK_URL: "",
HOME: work,
USERPROFILE: work,
},
},
stdio: ["ignore", "pipe", "pipe"],
},
);
let output = "";
let settled = false;
let childExited = false;
let childClosed = false;
// How long to let the TUI wind down after SIGTERM before escalating to SIGKILL,
// and then before giving up on the close event entirely.
const EXIT_GRACE_MS = Number(process.env.SMOKE_TUI_EXIT_GRACE_MS ?? 5000);
// Once the child itself is gone, only the pipe drain remains, so the wait drops
// to this. `close` is bounded by whoever holds the stdio pipes, which can
// outlive the direct child: any descendant that inherited them keeps it pending.
// The TUI spawns nothing at boot today (it does not auto-connect), so this
// never fires — but that reason lives outside this file, so cap it here rather
// than inherit an unrelated process's lifetime if that ever changes.
//
// The cost of capping: on that path we remove the work dir while a descendant
// holding HOME=work may still be writing to it — the #1801 race, re-entered
// deliberately. That is only acceptable because cleanup() warns instead of
// throwing, so the worst case is a warning plus a leaked temp dir in tmpdir,
// never a red smoke. The two are load-bearing for each other.
const DRAIN_MS = 500;
function cleanup() {
warn: (m) => console.warn(m.replace(/^render-smoke/, "smoke:tui")),
});
} finally {
// Never turn a passing smoke into a failure over a leftover temp dir; the OS
// reclaims tmpdir anyway. On the normal path a warning should not fire, since
// removal waits for the child's `close` (see done()) — but the two give-up
// branches there call finish() without it, and a process may still hold the
// dir, so it is expected rather than anomalous on those.
// reclaims tmpdir anyway. runRenderSmoke resolves only after the child's
// `close`, so on the normal path the #1801 ENOTEMPTY race is already closed
// and a warning here should not fire — its give-up branches resolve without
// that guarantee, so it is expected rather than anomalous on those.
removeSafe(work, { label: "smoke:tui" });
}
// Tail of the child's output, for quoting in a diagnostic. Slicing an Ink
// stream can land mid-CSI-sequence, so drop through the first newline rather
// than lead with an escape-code fragment that mangles the line after it.
function outputTail(limit = 800) {
const tail = output.slice(-limit);
const nl = tail.indexOf("\n");
return output.length > limit && nl !== -1 ? tail.slice(nl + 1) : tail;
}
// `message` may be a string or a thunk. Callers that quote the child's output
// pass a thunk so it is rendered here — after the wait below, by which point
// `close` guarantees stdout/stderr have been fully drained. Building the string
// at call time instead can truncate the crash reason that matters most.
let finished = false;
function finish(code, message) {
if (finished) return;
finished = true;
cleanup();
const text = typeof message === "function" ? message() : message;
if (code === 0) {
console.log(`smoke:tui OK — ${text}`);
} else {
console.error(`smoke:tui FAILED — ${text}`);
}
process.exit(code);
}
function done(code, message) {
if (settled) return;
settled = true;
clearTimeout(timer);
if (childClosed) {
finish(code, message);
return;
}
// Wait for the child to actually exit before removing the work dir (#1801).
// The dir doubles as the TUI's HOME, so the Ink process is still writing into
// it when we signal; an rmSync racing those writes fails with ENOTEMPTY when
// a file lands after a directory has been read but before it is removed.
//
// Wait on `close`, not `exit`: a *spawn failure* emits `error` + `close` and
// never `exit`, so an `exit` wait would hang here until the give-up timer and
// then blame a SIGTERM the child never received. `close` also guarantees the
// stdio pipes are drained, which is what makes the thunked messages complete.
//
// This deliberately does NOT use lib/child-cleanup.mjs's `stopChild()` (which
// pack-and-verify.mjs uses against the same race): that helper resolves on the
// FIRST of exit/close, whereas this wait is entangled with the drain phase —
// it must keep waiting for `close` after `exit`, on a shorter re-armed
// deadline, so the quoted output is complete. Only `removeSafe()` is shared.
const forceKill = setTimeout(() => child.kill("SIGKILL"), EXIT_GRACE_MS);
// Deadline for the wait. Re-armed shorter once the child is gone, since from
// that point we are only draining pipes. Each message describes what was
// actually being waited on — the timer can fire for a child that already
// exited and was never signalled, so it must not claim otherwise.
let deadline;
const armDeadline = (ms, warning) => {
clearTimeout(deadline);
deadline = setTimeout(() => {
console.warn(`smoke:tui — ${warning}`);
finish(code, message);
}, ms);
};
armDeadline(
EXIT_GRACE_MS * 2,
`TUI did not close its output streams within ${EXIT_GRACE_MS * 2}ms; cleaning up anyway`,
// Belt and braces for the acceptance criterion this fix is measured against.
// Survival already implies it today, since the raw-mode throw is fatal — but
// if Ink ever degrades to a warning instead, a TUI with no keyboard input would
// otherwise pass silently, and this smoke is the only thing watching.
if (result.code === 0 && RAW_MODE_ERROR.test(result.output)) {
fail(
`TUI rendered and survived, but still logged the raw-mode error — the ` +
`pseudoterminal did not take effect (flavor: ${pty.flavor})`,
);
const drainOnly = () => {
clearTimeout(forceKill);
armDeadline(
DRAIN_MS,
`TUI exited but held its output streams open for ${DRAIN_MS}ms (a descendant may have inherited them); cleaning up anyway`,
);
};
if (childExited) drainOnly();
else child.once("exit", drainOnly);
child.once("close", () => {
clearTimeout(forceKill);
clearTimeout(deadline);
finish(code, message);
});
// Only signal a child that is still running: on the crash-before-render and
// spawn-failure paths there is nothing left to signal, and we are here purely
// to wait out the remaining `close`.
if (child.exitCode === null && child.signalCode === null && !child.killed) {
child.kill("SIGTERM");
}
}
function onData(chunk) {
output += chunk.toString();
if (output.includes(RENDER_MARKER)) {
done(0, `rendered "${RENDER_MARKER}" panel from --catalog within timeout`);
}
if (result.code === 0) {
console.log(`smoke:tui OK — ${result.message}`);
process.exit(0);
}
child.stdout.on("data", onData);
child.stderr.on("data", onData);
// Registered before done()'s own one-shot listener, so this always runs first —
// done() can trust `childClosed` even when called from inside a close handler.
child.on("close", () => {
childClosed = true;
});
child.on("exit", (code) => {
childExited = true;
if (settled) return;
// Exiting before the render marker appeared is a failure (crash on boot).
done(
1,
() =>
`TUI exited (code ${code}) before rendering "${RENDER_MARKER}"\n${outputTail()}`,
);
});
child.on("error", (err) => {
done(1, `failed to spawn TUI: ${err.message}`);
});
const timer = setTimeout(() => {
done(
1,
() =>
`TUI did not render "${RENDER_MARKER}" within ${TIMEOUT_MS}ms\n${outputTail()}`,
);
}, TIMEOUT_MS);
fail(result.message);