From e15dd8abee4adf919e94325e15d0a3296702ba58 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 22:13:35 -0400 Subject: [PATCH] fix(scripts): make the test stubs drain, and the raw-mode guard testable (#2147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: cliffhall --- scripts/lib/render-smoke.mjs | 31 +++++++++++--- scripts/lib/render-smoke.test.mjs | 70 ++++++++++++++++++++++++++++++- scripts/smoke-tui.mjs | 21 +++++----- 3 files changed, 103 insertions(+), 19 deletions(-) diff --git a/scripts/lib/render-smoke.mjs b/scripts/lib/render-smoke.mjs index f156f123..bff51b24 100644 --- a/scripts/lib/render-smoke.mjs +++ b/scripts/lib/render-smoke.mjs @@ -23,7 +23,11 @@ import { spawn } from "node:child_process"; -/** Durations, in ms. Every one is a hard floor as well as a default. */ +/** + * Fallback durations, in ms. These are defaults, NOT minimums: `normalizeMs` + * accepts any finite positive value, and the tests deliberately pass shorter + * ones. What it rejects is the shape a bad env var produces, not smallness. + */ export const DEFAULTS = Object.freeze({ timeoutMs: 15_000, surviveMs: 2_000, @@ -93,6 +97,14 @@ export function outputTail(output, limit = 800) { * @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. + * @param {{ pattern: RegExp, reason: string }} [opts.forbidOutput] + * Text that must NOT appear in an otherwise-passing run. `smoke:tui` uses it + * for Ink's raw-mode error: survival already implies its absence *today*, + * because that throw is fatal — but if Ink ever degraded it to a warning, a + * TUI with no keyboard input would survive happily and pass. Checked here + * rather than at the call site so it runs against the fully drained output + * and is reachable from a test. Use a non-global RegExp: `test()` on a `/g` + * pattern advances `lastIndex` and alternates. * @param {(message: string) => void} [opts.warn] * @returns {Promise<{ code: 0 | 1, message: string, output: string }>} * On the normal path, resolves only after the child's streams have `close`d — @@ -115,6 +127,7 @@ export function runRenderSmoke({ surviveMs: requestedSurviveMs, exitGraceMs: requestedExitGraceMs, drainMs: requestedDrainMs, + forbidOutput, onSpawn = () => {}, warn = (m) => console.warn(m), }) { @@ -162,11 +175,17 @@ export function runRenderSmoke({ const finish = (code, message) => { if (finished) return; finished = true; - resolve({ - code, - message: typeof message === "function" ? message() : message, - output, - }); + let verdict = code; + let text = typeof message === "function" ? message() : message; + // Only ever turns a pass into a failure. A run that already failed keeps + // its own diagnostic: the crash reason is the more useful of the two, and + // forbidden output is usually a *symptom* of it rather than a separate + // finding. + if (verdict === 0 && forbidOutput?.pattern.test(output)) { + verdict = 1; + text = `${forbidOutput.reason}\n${outputTail(output)}`; + } + resolve({ code: verdict, message: text, output }); }; // Settle the verdict, then wait for the child to actually close before diff --git a/scripts/lib/render-smoke.test.mjs b/scripts/lib/render-smoke.test.mjs index e3b12f1b..a77b5468 100644 --- a/scripts/lib/render-smoke.test.mjs +++ b/scripts/lib/render-smoke.test.mjs @@ -15,8 +15,17 @@ function stub(body) { return { command: process.execPath, args: ["-e", body] }; } +// `process.exitCode`, never `process.exit()`. Writing to a *pipe* is +// asynchronous, and `process.exit()` tears the process down without draining — +// so `console.log(MARKER); process.exit(1)` can lose the marker outright. That +// is not hypothetical: probing the drain race for this PR, a 4MB write followed +// by `process.exit()` arrived at the parent with the marker missing entirely. +// A stub that intermittently never paints would fail the very assertion meant +// to pin the paint-then-die path, and would fail it as "exited before +// rendering" — the neighbouring branch. Setting `exitCode` and letting the loop +// drain still exits immediately after the write; nothing holds it open. const paintThenExit = (code) => - stub(`console.log(${JSON.stringify(MARKER)}); process.exit(${code});`); + stub(`console.log(${JSON.stringify(MARKER)}); process.exitCode = ${code};`); const paintThenLive = stub( `console.log(${JSON.stringify(MARKER)}); setInterval(() => {}, 1000);`, ); @@ -96,7 +105,9 @@ test("a paint landing just under the deadline is not failed by the render timer" test("fails when the child exits before painting", async () => { const r = await runRenderSmoke({ ...opts, - ...stub(`console.error("boom"); process.exit(3);`), + // `exitCode`, not `exit()` — see `paintThenExit`. A forced exit can + // truncate the stderr write, making the `boom` assertion below flaky. + ...stub(`console.error("boom"); process.exitCode = 3;`), }); assert.equal(r.code, 1); assert.match(r.message, /before rendering/); @@ -181,6 +192,61 @@ test("a zero survival window falls back rather than passing instantly", async () assert.doesNotMatch(r.message, /\b0ms survival window/); }); +// `forbidOutput` is `smoke:tui`'s guard for the acceptance criterion that the +// raw-mode error must not appear in a passing run. It lives here rather than at +// the call site precisely so it can be driven both ways: the real PTY happy +// path only ever exercises the pattern NOT matching, so a regression that broke +// this check would stay green forever. +test("forbidOutput fails a run that would otherwise have passed", async () => { + const r = await runRenderSmoke({ + ...opts, + ...stub( + `console.log(${JSON.stringify(MARKER)}); ` + + `console.error("Raw mode is not supported on the current process.stdin"); ` + + `setInterval(() => {}, 1000);`, + ), + forbidOutput: { + pattern: /Raw mode is not supported/, + reason: "survived but logged the raw-mode error", + }, + }); + assert.equal(r.code, 1, `expected failure, got: ${r.message}`); + assert.match(r.message, /survived but logged the raw-mode error/); + // The offending output is quoted, so the reader sees the evidence. + assert.match(r.message, /Raw mode is not supported/); +}); + +test("forbidOutput leaves a passing run alone when it does not match", async () => { + const r = await runRenderSmoke({ + ...opts, + ...paintThenLive, + forbidOutput: { + pattern: /Raw mode is not supported/, + reason: "should not be reached", + }, + }); + assert.equal(r.code, 0, `expected pass, got: ${r.message}`); + assert.doesNotMatch(r.message, /should not be reached/); +}); + +// A crash reason is more useful than "and it also printed X", which is usually +// a symptom of the same crash rather than a separate finding. +test("forbidOutput does not overwrite an existing failure's diagnostic", async () => { + const r = await runRenderSmoke({ + ...opts, + ...stub( + `console.error("Raw mode is not supported"); process.exitCode = 1;`, + ), + forbidOutput: { + pattern: /Raw mode is not supported/, + reason: "should not replace the crash reason", + }, + }); + assert.equal(r.code, 1); + assert.match(r.message, /before rendering/); + assert.doesNotMatch(r.message, /should not replace the crash reason/); +}); + 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 b3192c66..7f17a288 100644 --- a/scripts/smoke-tui.mjs +++ b/scripts/smoke-tui.mjs @@ -152,6 +152,16 @@ try { USERPROFILE: work, }, }, + // 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 it to a warning, a TUI with no keyboard + // input would survive happily, and this smoke is the only thing watching. + forbidOutput: { + pattern: RAW_MODE_ERROR, + reason: + `TUI rendered and survived, but still logged the raw-mode error — ` + + `the pseudoterminal did not take effect (flavor: ${pty.flavor})`, + }, warn: (m) => console.warn(m.replace(/^render-smoke/, "smoke:tui")), }); } finally { @@ -163,17 +173,6 @@ try { removeSafe(work, { label: "smoke:tui" }); } -// 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})`, - ); -} - if (result.code === 0) { console.log(`smoke:tui OK — ${result.message}`); process.exit(0);