From b7a73e7da38bf9cf86aa1e8e103c6d926c650a2f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 21:36:40 -0400 Subject: [PATCH] fix(scripts): address Copilot review round 2 on #2147 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: cliffhall --- scripts/lib/pty.mjs | 54 +++++++++++++++---- scripts/lib/pty.test.mjs | 87 ++++++++++++++++++++++++++----- scripts/lib/render-smoke.mjs | 63 +++++++++++++++++++--- scripts/lib/render-smoke.test.mjs | 40 +++++++++++++- scripts/smoke-tui.mjs | 23 +++++--- 5 files changed, 226 insertions(+), 41 deletions(-) diff --git a/scripts/lib/pty.mjs b/scripts/lib/pty.mjs index e6ec2df8..d311ba68 100644 --- a/scripts/lib/pty.mjs +++ b/scripts/lib/pty.mjs @@ -109,39 +109,71 @@ export function ptyCommand({ command, args = [], flavor }) { } /** - * Probe the local `script(1)` for its flavor. + * Probe the local `script(1)`: is it there, and what does it say about itself? * - * @param {(cmd: string, args: string[]) => { stdout?: string, stderr?: string }} [runner] + * **`spawnSync` does not throw on ENOENT** — it *returns* `{ error }` with + * empty output. So a `try`/`catch` alone cannot tell "no `script` on this box" + * from "a `script` that printed nothing", and collapsing both to `""` is + * actively harmful: on linux the empty output falls through to the + * `util-linux` guess, and `smoke:tui` then hard-fails spawning a binary that + * does not exist, instead of taking the documented skip. Hence the `available` + * flag rather than a bare string. The `catch` stays for a runner that really + * does throw (an injected one, or a future spawn shape). + * + * A non-zero exit is NOT unavailability: BSD `script` has no `--version` and + * answers `illegal option -- -` plus its usage on stderr — which is exactly the + * evidence `scriptFlavorFor` reads. Only `error` means "could not run it". + * + * @param {(cmd: string, args: string[]) => { stdout?: string, stderr?: string, error?: Error }} [runner] * Injected for tests; defaults to a real `spawnSync`. - * @returns {string} Combined output, or "" if `script` could not be run at all. + * @returns {{ available: boolean, output: string }} */ export function probeScriptVersion( runner = (cmd, args) => spawnSync(cmd, args, { encoding: "utf8", timeout: 5000 }), ) { try { - const r = runner("script", ["--version"]) ?? {}; - return `${r.stdout ?? ""}${r.stderr ?? ""}`; + const r = runner("script", ["--version"]); + // `error` covers ENOENT, EACCES and the timeout; a missing result at all + // means the runner told us nothing, which is not evidence of a working one. + if (!r || r.error) return { available: false, output: "" }; + return { available: true, output: `${r.stdout ?? ""}${r.stderr ?? ""}` }; } catch { - return ""; + return { available: false, output: "" }; } } /** - * Resolve a PTY wrapper for this machine, or null if none is available. + * Resolve a PTY wrapper for this machine, or say why there isn't one. + * + * The two ways it can be unavailable are worth telling apart in the skip + * message — "you have no `script(1)`" and "nobody has verified this platform's + * invocation" send the reader somewhere different — so the failure carries a + * `reason` rather than being a bare `null` the caller has to narrate. * * @param {object} [opts] * @param {string} [opts.platform] - * @param {() => string} [opts.probe] - * @returns {{ flavor: string, wrap: (spec: { command: string, args?: string[] }) => { command: string, args: string[] } } | null} + * @param {() => { available: boolean, output: string }} [opts.probe] + * @returns {{ ok: true, flavor: string, wrap: (spec: { command: string, args?: string[] }) => { command: string, args: string[] } } + * | { ok: false, reason: string }} */ export function resolvePtyWrapper({ platform = process.platform, probe = probeScriptVersion, } = {}) { - const flavor = scriptFlavorFor({ platform, versionOutput: probe() }); - if (!flavor) return null; + const { available, output } = probe(); + if (!available) { + return { ok: false, reason: "no `script(1)` on PATH" }; + } + const flavor = scriptFlavorFor({ platform, versionOutput: output }); + if (!flavor) { + return { + ok: false, + reason: `no verified \`script(1)\` invocation for ${platform}`, + }; + } return { + ok: true, flavor, wrap: ({ command, args = [] }) => ptyCommand({ command, args, flavor }), }; diff --git a/scripts/lib/pty.test.mjs b/scripts/lib/pty.test.mjs index b1c0b4f2..ceb1fbcb 100644 --- a/scripts/lib/pty.test.mjs +++ b/scripts/lib/pty.test.mjs @@ -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`, ); } diff --git a/scripts/lib/render-smoke.mjs b/scripts/lib/render-smoke.mjs index f58c3c11..468d89f8 100644 --- a/scripts/lib/render-smoke.mjs +++ b/scripts/lib/render-smoke.mjs @@ -23,6 +23,40 @@ import { spawn } from "node:child_process"; +/** Durations, in ms. Every one is a hard floor as well as a default. */ +export const DEFAULTS = Object.freeze({ + timeoutMs: 15_000, + surviveMs: 2_000, + exitGraceMs: 5_000, + drainMs: 500, +}); + +/** + * Coerce a caller-supplied duration to a usable one, mirroring + * `child-cleanup.mjs`'s `normalizeGraceMs`. + * + * Call sites read these from the environment, and `Number()` there has two + * holes that `??` does not cover: `Number("")` is `0` and `Number("typo")` is + * `NaN`. `setTimeout` treats both as "fire immediately" — which for `surviveMs` + * means the survival check resolves before the child could possibly have died, + * silently restoring the exact first-paint false green this module exists to + * prevent, on a run that still prints OK. Falling back is therefore not + * defensive tidiness; it is the difference between a gate and a decoration. + * + * A consequence worth stating: `surviveMs: 0` is NOT honored as "skip the + * survival wait". There is no legitimate caller for that here, and reading it + * literally is indistinguishable from the typo. + * + * @param {unknown} value + * @param {number} fallback + * @returns {number} + */ +export function normalizeMs(value, fallback) { + return typeof value === "number" && Number.isFinite(value) && value > 0 + ? value + : fallback; +} + /** * Tail of a child's output, for quoting in a diagnostic. Slicing a terminal * stream can land mid-CSI-sequence, so drop through the first newline rather @@ -46,11 +80,16 @@ export function outputTail(output, limit = 800) { * (`stdio` is fixed — see below). * @param {string} opts.marker Substring whose appearance in the * child's output counts as first paint. - * @param {number} [opts.timeoutMs] Budget for first paint (default 15s). + * @param {number} [opts.timeoutMs] Budget for first paint (default 15s; + * non-finite/non-positive falls back). * @param {number} [opts.surviveMs] How long after first paint the child - * must stay alive (default 2s). - * @param {number} [opts.exitGraceMs] SIGTERM → SIGKILL grace (default 5s). - * @param {number} [opts.drainMs] Post-exit pipe-drain cap (default 500ms). + * must stay alive (default 2s; + * non-finite/non-positive falls back — + * 0 is a typo, never "skip the check"). + * @param {number} [opts.exitGraceMs] SIGTERM → SIGKILL grace (default 5s; + * non-finite/non-positive falls back). + * @param {number} [opts.drainMs] Post-exit pipe-drain cap (default 500ms; + * non-finite/non-positive falls back). * @param {(child: import("node:child_process").ChildProcess) => void} [opts.onSpawn] * Called synchronously with the child, before any waiting — the caller's * teardown handle on every failure path. @@ -72,13 +111,21 @@ export function runRenderSmoke({ args = [], spawnOptions = {}, marker, - timeoutMs = 15_000, - surviveMs = 2_000, - exitGraceMs = 5_000, - drainMs = 500, + timeoutMs: requestedTimeoutMs, + surviveMs: requestedSurviveMs, + exitGraceMs: requestedExitGraceMs, + drainMs: requestedDrainMs, onSpawn = () => {}, warn = (m) => console.warn(m), }) { + // Normalized rather than defaulted, so an env-derived 0/NaN cannot reach a + // `setTimeout` — see `normalizeMs`. Done once, before anything reads them, + // so the interpolated durations in the messages below can't read `NaNms`. + const timeoutMs = normalizeMs(requestedTimeoutMs, DEFAULTS.timeoutMs); + const surviveMs = normalizeMs(requestedSurviveMs, DEFAULTS.surviveMs); + const exitGraceMs = normalizeMs(requestedExitGraceMs, DEFAULTS.exitGraceMs); + const drainMs = normalizeMs(requestedDrainMs, DEFAULTS.drainMs); + return new Promise((resolve) => { const child = spawn(command, args, { ...spawnOptions, diff --git a/scripts/lib/render-smoke.test.mjs b/scripts/lib/render-smoke.test.mjs index cae3f7fd..0c335915 100644 --- a/scripts/lib/render-smoke.test.mjs +++ b/scripts/lib/render-smoke.test.mjs @@ -1,7 +1,12 @@ import { strict as assert } from "node:assert"; import test from "node:test"; -import { outputTail, runRenderSmoke } from "./render-smoke.mjs"; +import { + DEFAULTS, + normalizeMs, + outputTail, + runRenderSmoke, +} from "./render-smoke.mjs"; const MARKER = "MCP Servers"; @@ -105,6 +110,39 @@ test("scans stderr as well as stdout for the marker", async () => { assert.equal(r.code, 0, `expected pass, got: ${r.message}`); }); +// `Number("")` is 0 and `Number("typo")` is NaN, and `??` catches neither +// because both are non-nullish. `setTimeout` fires immediately on either, so an +// un-normalized `surviveMs` resolves the survival check before the child could +// possibly have died — restoring the exact first-paint false green, on a run +// that still prints OK. This is the guard on the guard. +test("normalizeMs rejects the values a bad env var actually produces", () => { + assert.equal(normalizeMs(Number(""), 2000), 2000); + assert.equal(normalizeMs(Number("typo"), 2000), 2000); + assert.equal(normalizeMs(undefined, 2000), 2000); + assert.equal(normalizeMs(-1, 2000), 2000); + assert.equal(normalizeMs(Infinity, 2000), 2000); + assert.equal(normalizeMs("3000", 2000), 2000, "a string is not a duration"); + // 0 is a typo, never "skip the check" — there is no caller who wants that. + assert.equal(normalizeMs(0, 2000), 2000); + // A real value passes through untouched. + assert.equal(normalizeMs(1234, 2000), 1234); +}); + +test("a zero survival window falls back rather than passing instantly", async () => { + // End to end through the real state machine: with surviveMs honored as 0 the + // paint-then-exit stub would settle OK before the child's exit was seen. + const r = await runRenderSmoke({ + ...opts, + ...paintThenExit(1), + surviveMs: Number(""), + }); + assert.equal(r.code, 1, `expected failure, got: ${r.message}`); + assert.match( + r.message, + new RegExp(`${DEFAULTS.surviveMs}ms survival window`), + ); +}); + test("outputTail drops a leading partial line but keeps short output whole", () => { assert.equal(outputTail("short", 800), "short"); // Over the limit: the slice can land mid-escape-sequence, so it starts after diff --git a/scripts/smoke-tui.mjs b/scripts/smoke-tui.mjs index ec8314d7..b3192c66 100644 --- a/scripts/smoke-tui.mjs +++ b/scripts/smoke-tui.mjs @@ -41,17 +41,26 @@ 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"; +import { DEFAULTS, normalizeMs, 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); +// `??` does not cover the two holes that matter here: an env var set to the +// empty string parses as 0, and a typo as NaN, and `setTimeout` fires +// immediately on both. For SURVIVE_MS that would silently restore the +// first-paint false green on a run that still prints OK, so every duration goes +// through `normalizeMs` — which is also where the defaults live, so this file +// and the module cannot drift on what they are. +const envMs = (name, fallback) => + normalizeMs(Number(process.env[name]), fallback); + +const TIMEOUT_MS = envMs("SMOKE_TUI_TIMEOUT_MS", DEFAULTS.timeoutMs); // 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); +const SURVIVE_MS = envMs("SMOKE_TUI_SURVIVE_MS", DEFAULTS.surviveMs); +const EXIT_GRACE_MS = envMs("SMOKE_TUI_EXIT_GRACE_MS", DEFAULTS.exitGraceMs); // 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/; @@ -85,10 +94,10 @@ if (!existsSync(launcher)) { // 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) { +if (!pty.ok) { skip( - `no pseudoterminal available on ${process.platform} — the Ink TUI needs raw mode, ` + - `which requires a real terminal fd (see scripts/lib/pty.mjs)`, + `${pty.reason} — the Ink TUI needs raw mode, which requires a real ` + + `terminal fd (see scripts/lib/pty.mjs)`, ); }