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)`, ); }