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

fix(scripts): address Copilot review round 2 on #2147

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

87
scripts/lib/pty.test.mjs
View file

@ -121,28 +121,76 @@ test("shellQuote survives an embedded single quote", () => {
});
test("probeScriptVersion combines both streams and never throws", () => {
assert.equal(
assert.deepEqual(
probeScriptVersion(() => ({ stdout: "out ", stderr: "err" })),
"out err",
{ available: true, output: "out err" },
);
// BSD `script --version` errors; spawnSync can also return a bare object.
assert.equal(
// A non-zero exit is NOT unavailability. BSD `script` has no `--version` and
// answers with `illegal option -- -` plus its usage on stderr — which is the
// very evidence the flavor fallback reads, so it must survive the probe.
assert.deepEqual(
probeScriptVersion(() => ({
status: 1,
stdout: "",
stderr: "script: illegal option -- -\nusage: script [-aeFkpqr] ...",
})),
{
available: true,
output: "script: illegal option -- -\nusage: script [-aeFkpqr] ...",
},
);
// A runner that throws outright (injected, or a future spawn shape).
assert.deepEqual(
probeScriptVersion(() => {
throw new Error("ENOENT");
}),
"",
{ available: false, output: "" },
);
assert.equal(
probeScriptVersion(() => ({})),
"",
// Told nothing at all is not evidence of a working `script`.
assert.deepEqual(
probeScriptVersion(() => undefined),
{
available: false,
output: "",
},
);
});
test("resolvePtyWrapper wraps with the resolved flavor, or reports none", () => {
// `spawnSync` REPORTS ENOENT in `result.error` — it does not throw — so a
// try/catch alone reads a missing binary as "ran, printed nothing". On linux
// that empty output falls through to the util-linux guess, and `smoke:tui`
// hard-fails spawning a binary that does not exist instead of skipping.
test("probeScriptVersion treats result.error as unavailable, not as empty output", () => {
const enoent = Object.assign(new Error("spawnSync script ENOENT"), {
code: "ENOENT",
});
assert.deepEqual(
probeScriptVersion(() => ({ error: enoent, stdout: "", stderr: "" })),
{ available: false, output: "" },
);
// The timeout shape reports the same way.
assert.equal(
probeScriptVersion(() => ({ error: new Error("ETIMEDOUT") })).available,
false,
);
});
test("resolvePtyWrapper reports a missing script(1) rather than guessing", () => {
// linux would otherwise be classified util-linux off the empty output.
const r = resolvePtyWrapper({
platform: "linux",
probe: () => ({ available: false, output: "" }),
});
assert.equal(r.ok, false);
assert.match(r.reason, /on PATH/);
});
test("resolvePtyWrapper wraps with the resolved flavor, or reports why not", () => {
const wrapper = resolvePtyWrapper({
platform: "linux",
probe: () => "script from util-linux 2.39.3",
probe: () => ({ available: true, output: "script from util-linux 2.39.3" }),
});
assert.equal(wrapper.ok, true);
assert.equal(wrapper.flavor, "util-linux");
assert.deepEqual(wrapper.wrap({ command: "node", args: ["a.js"] }), {
command: "script",
@ -151,7 +199,14 @@ test("resolvePtyWrapper wraps with the resolved flavor, or reports none", () =>
// `args` is optional at the call site.
assert.deepEqual(wrapper.wrap({ command: "node" }).args[1], "'node'");
assert.equal(resolvePtyWrapper({ platform: "win32", probe: () => "" }), null);
// A present `script` on a platform whose invocation nobody verified is a
// DIFFERENT failure from a missing one, and the reason must say which.
const unsupported = resolvePtyWrapper({
platform: "win32",
probe: () => ({ available: true, output: "" }),
});
assert.equal(unsupported.ok, false);
assert.match(unsupported.reason, /no verified .* invocation for win32/);
});
// The platforms this repo claims a verified `script(1)` invocation for. Any
@ -166,12 +221,16 @@ test("resolvePtyWrapper resolves on the platforms we claim to support", () => {
// smoke failure.
const wrapper = resolvePtyWrapper();
if (SUPPORTED_PLATFORMS.has(process.platform)) {
assert.ok(wrapper, `no PTY wrapper resolved on ${process.platform}`);
assert.equal(
wrapper.ok,
true,
`no PTY wrapper on ${process.platform}: ${wrapper.reason ?? ""}`,
);
assert.equal(wrapper.wrap({ command: "node" }).command, "script");
} else {
assert.equal(
wrapper,
null,
wrapper.ok,
false,
`${process.platform} is not in SUPPORTED_PLATFORMS but resolved a wrapper`,
);
}