Skip to content

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

Merged
cliffhall merged 7 commits into
v2/mainfrom
v2/fix/2147-smoke-tui-pty
Aug 27, 2026
Merged

fix(scripts): make smoke:tui assert the TUI survives its first frame (#2147)#2158
cliffhall merged 7 commits into
v2/mainfrom
v2/fix/2147-smoke-tui-pty

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #2147

The problem

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 paints one frame and exits 1 with Raw mode is not supported on the current process.stdin. Measured on this branch's parent commit:

[plain] first frame at 599ms
[plain] EXITED code=1 sig=null at 644ms
[plain] at 6000ms: alive=false rawModeError=true

The smoke won that ~45ms race and reported OK. So what it asserted was first paint, not survival — contradicting its own header ("boots and renders without crashing"). And 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. The race also cut both ways — an exit processed first fails the run, which reads as flake rather than as the standing defect.

Under a pseudoterminal, same spawn:

[pty] first frame at 374ms
[pty] at 6000ms: alive=true rawModeError=false

So the TUI is fine; the harness was what broke it.

The change

scripts/lib/pty.mjs — gives the child a real terminal via script(1), no dependency added. The three flavors are not interchangeable, which is why the invocation is built and unit-tested rather than written inline at the call site:

flavor invocation notes
BSD (macOS) script -q /dev/null <cmd> <args…> argv, no shell
util-linux script -qec "<cmd args…>" /dev/null one shell string; -e propagates the child's exit code
busybox script -qc "<cmd args…>" /dev/null no -e

The -c flavors take a single command string, so every word is quoted on the way in — the smoke passes an mkdtemp path, and a space in it would otherwise split into two arguments. Flavor comes from probing script --version first (Alpine is why: "linux" does not imply util-linux, and busybox rejects -e), falling back to the platform. A platform with no script(1) (Windows) skips loudly rather than running without one — that is a guaranteed failure, not a weaker check, and node-pty is the portable answer if it ever needs to run there.

scripts/lib/render-smoke.mjs — the actual fix. The child must still be running surviveMs (default 2s) after first paint. All of the existing teardown care is preserved: the close-not-exit wait, the re-armed drain deadline, the SIGTERM→SIGKILL grace, and the #1801 ordering that removes the work dir only after the child has closed.

It is a module rather than inline code because a smoke only ever exercises its own happy path. The branch that matters most here — paint, then die — is unreachable from a fixed TUI, so test:scripts drives it against node -e stubs instead. Today's harness passes the paint-then-exit-1 stub; this one fails it, and the diagnostic distinguishes "died after painting" from "died before painting" so the reader isn't sent to the wrong defect. Same reasoning as announced-child.mjs (#2000) and ensure-test-servers.mjs (#2111).

The smoke additionally fails if the raw-mode error appears in an otherwise-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.

Verification

Real smoke, real TUI, real PTY:

$ npm run smoke:tui
smoke:tui OK — rendered "MCP Servers" at 600ms and was still running 2000ms later

Negative check — a script shim on PATH that allocates no pty and just execs its argument, i.e. exactly the old behavior:

$ PATH="$FAKEBIN:$PATH" node scripts/smoke-tui.mjs; echo "EXIT=$?"
smoke:tui FAILED — child rendered "MCP Servers" at 808ms and then exited (code 1) 483ms
later, inside the 2000ms survival window — it painted one frame, it did not run
    at commitPassiveMountOnFiber (…/react-reconciler.development.js:12979:13)
    …
Error running MCP Inspector: sonic boom is not ready yet
EXIT=1

npm run test:scripts — 204 tests, 19 of them new across pty.test.mjs and render-smoke.test.mjs. npm run ci green.

No screenshots: this changes a build/verify script, not the web UI or the TUI's own rendering. The terminal output above is the proof of functionality.

Acceptance criteria

  • smoke:tui fails if the TUI exits non-zero after rendering, and a test proves it — render-smoke.test.mjs, "fails when the child paints the marker and then exits non-zero". The zero-exit twin is covered too, since busybox script reports 0 for a crashed child and the verdict therefore cannot rest on the exit code.
  • The raw-mode error no longer appears in the child's output on a passing run — and is now asserted explicitly, not merely implied.
  • The process.env.CI skip's comment is corrected. The skip stays; the PTY removes the technical blocker it used to cite, so it is local-only by decision now, not by capability. Whether it joins GitHub CI is deliberately out of scope here (Optimizing and documenting repo-hygiene processes: rename the local pre-push gate off the name ci, and guard it out of CI #2146 is keeping the other local-only smokes out on purpose), and that is now what the comment says.

Docs updated in the same change: AGENTS.md, README.md, and clients/launcher/README.md.

…2147)

`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>
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Aug 27, 2026
@cliffhall
cliffhall requested a balanced review from Copilot August 27, 2026 01:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates smoke:tui to run under a pseudoterminal and verify the TUI survives beyond its first rendered frame.

Changes:

  • Adds PTY invocation and process-survival helpers.
  • Adds regression tests for PTY flavors and post-render crashes.
  • Documents the strengthened smoke behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
scripts/smoke-tui.mjs Uses the new PTY and survival harness.
scripts/lib/render-smoke.mjs Implements render-and-survive process checks.
scripts/lib/render-smoke.test.mjs Tests rendering, crashes, timeouts, and spawning.
scripts/lib/pty.mjs Builds platform-specific script(1) wrappers.
scripts/lib/pty.test.mjs Tests PTY flavor detection and quoting.
README.md Documents the stronger smoke gate.
clients/launcher/README.md Describes TUI smoke survival behavior.
AGENTS.md Records the PTY and survival-test architecture.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread scripts/lib/pty.mjs Outdated
Comment thread scripts/lib/pty.test.mjs Outdated
Comment thread scripts/lib/render-smoke.mjs Outdated
Comment thread AGENTS.md Outdated
Comment thread AGENTS.md
- pty.mjs: narrow the BSD argv set to {darwin, freebsd}. Grouping every
  BSD together was wrong — NetBSD's `script` takes the command through
  `-c`, so the argv form fails at `script` startup, which this smoke would
  then report as a TUI crash. Rather than add an unverified `-c` branch
  for netbsd/openbsd, they fall through to `null` and the smoke skips with
  the reason: a guessed invocation turns a clean skip into a false failure
  while looking like support.

- pty.test.mjs: "supported unless Windows" would fail `test:scripts` on
  aix/sunos/netbsd/openbsd, where a null wrapper is correct and the smoke
  skips by design. State the policy once (SUPPORTED_PLATFORMS) and branch
  on it, keeping the real-probe teeth that catch a `script` missing from
  PATH. Pin netbsd/openbsd/aix/sunos to null in scriptFlavorFor's tests
  so "unsupported" reads as the default, not a win32 special case.

- render-smoke.mjs: the @returns contract claimed a close guarantee the
  code does not give — both drain give-up branches resolve after only a
  warning, so output can be truncated and the child may still hold the
  work dir. Document the bounded fallback and name the required cleanup
  shape (removeSafe, not rmSync), since a caller "tightening" cleanup on
  the old wording would reintroduce #1801 exactly where it doesn't hold.

- AGENTS.md, clients/launcher/README.md: drop the pre-#2111 claim that the
  smokes build test-servers "on demand if it is missing". ensureTestServers
  rebuilds unconditionally, once per process — presence is not freshness.

- .github/copilot-instructions.md: mirror the survival rule, per the
  same-PR mirroring requirement. One bullet, generalized past this fix:
  a smoke must assert the thing survives, not that it started, and the
  started-then-died branch belongs in a scripts/lib test against a stub.

Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review round 1 — all 5 addressed (bd7297da)

Mirroring the inline replies here, since pushing the fixes marks those threads outdated and hides them.

# Finding Response
1 openbsd/netbsd don't take the BSD argv form Fixed by narrowing. BSD_PLATFORMS is now {darwin, freebsd}; netbsd/openbsd resolve to null and the smoke skips.
2 Test assumes every non-Windows platform is supported Fixed. SUPPORTED_PLATFORMS = {darwin, freebsd, linux}, assertion branches on membership.
3 @returns overstates the close guarantee Fixed. Documented as bounded, and named the required cleanup shape.
4 Stale "builds test-servers on demand if missing" Fixed in both AGENTS.md and clients/launcher/README.md.
5 Rule not mirrored into copilot-instructions.md Fixed. One bullet added.

On #1 — you were right that grouping every BSD together is wrong, but I didn't add the -c branch, because I can't verify it. darwin is measured on this machine; freebsd is where darwin's implementation comes from. NetBSD and OpenBSD are neither, and a guessed invocation is strictly worse than none: it fails at script startup, which this smoke would report as a TUI crash — the exact misattribution the PR exists to remove — while looking like support. They now fall through to the existing loud skip, with scriptFlavorFor tests pinning them to null so nobody re-adds them by pattern-matching the platform name. Adding a verified -c flavor is a small change for whoever can run it on the box.

On #2 — this got worse after fixing #1, since two more platforms now resolve to null. "Non-Windows ⇒ supported" would have failed test:scripts on a machine where smoke:tui is behaving exactly as designed; a test that turns a correct skip into a red run is the wrong shape. The real-probe teeth are kept — a script gone from PATH still fails there rather than surfacing later as an opaque smoke failure.

On #3 — naming the cleanup shape is the load-bearing part. A caller that "tightened" removeSafe to rmSync on the strength of the old wording would have reintroduced #1801 on precisely the paths where the guarantee doesn't hold, and only intermittently.

On #4 — more than a wording nit in context: "on demand if it is missing" invites exactly the optimization #2111 removed, and the resulting failure is a smoke run against a fixture that no longer matches test-servers/src, which reads as a product bug rather than a staleness one. grep confirms those were the only two instances.

On #5 — agreed, and it's the same failure mode as the PR itself: a guard nothing enforces. Kept to one bullet, generalized past this fix ("any new smoke whose only assertion is that a process printed something once"), with AGENTS.md staying the source of truth for the detail.

Verification: npm run ci green end to end (CI_EXIT=0), test:scripts 204/204, and smoke:tui OK — rendered "MCP Servers" at 363ms and was still running 2000ms later. The negative check still fails as it should: with a script shim that allocates no PTY, smoke:tui FAILED — … it painted one frame, it did not run.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/lib/pty.mjs:124

  • spawnSync reports ENOENT and timeout failures in result.error; it normally does not throw. This currently turns a missing script binary into "", after which Linux is classified as util-linux, so smoke:tui hard-fails while spawning script instead of taking the documented “no PTY available” skip. Return a distinct unavailable result when r.error is present (and on an actual throw), have resolvePtyWrapper map that to null, and cover the result.error shape in the tests.
    const r = runner("script", ["--version"]) ?? {};
    return `${r.stdout ?? ""}${r.stderr ?? ""}`;

Comment thread scripts/smoke-tui.mjs Outdated
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>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review round 2 — both addressed (b7a73e7d)

Mirrored here because pushing marks the inline thread outdated, and because one of the two was in the collapsed "Suppressed comments" block and has no thread to reply to at all.

Both findings share a shape worth naming: each one silently un-does this PR while leaving the smoke printing OK.

1. SMOKE_TUI_SURVIVE_MS was unvalidated (inline)

?? 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, so the survival check would resolve before the child could possibly have died — printing still running 0ms later and asserting nothing. The first-paint false green, restored by an empty environment variable, on a green run.

Fixed at the module rather than only at the env-parse site, so any caller is covered:

export function normalizeMs(value, fallback) {
  return typeof value === "number" && Number.isFinite(value) && value > 0
    ? value
    : fallback;
}

All four durations are normalized before anything reads them (which also stops the messages interpolating NaNms), and the defaults moved into an exported DEFAULTS so smoke-tui.mjs and the module can't drift. Thanks for the child-cleanup.mjs pointer — normalizeGraceMs is the same shape for the same reason, and matching an existing policy beat inventing a second one.

Deliberate, and documented on the helper: surviveMs: 0 is not honored as "skip the survival wait". No caller wants that, and it is indistinguishable from the typo.

2. spawnSync reports ENOENT in result.error (suppressed)

Confirmed against the real API — spawnSync("nope") returns { error: ENOENT, status: null } and does not throw, so the try/catch read a missing binary as "ran, printed nothing". On linux that empty output then fell through to the util-linux guess, and smoke:tui 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 reason is not decoration: "no script(1) on PATH" and "no verified script(1) invocation for <platform>" send the reader somewhere different, and after round 1 both are reachable.

One thing the fix had to be careful not to do: a non-zero exit is not unavailability. BSD script has no --version and answers script: illegal option -- - plus its usage on stderr — which is exactly the evidence the flavor fallback reads. Only error means "could not run it". There's a test pinning that shape so a later tightening to status === 0 doesn't break macOS.

Verification

Both findings live entirely in branches no happy path reaches, so each got tests: 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.

Behavior checked for real, not just in tests:

$ SMOKE_TUI_SURVIVE_MS="" npm run smoke:tui
smoke:tui OK — rendered "MCP Servers" at 451ms and was still running 2000ms later   # not 0ms

$ PATH=/var/empty node -e '…resolvePtyWrapper()…'
{"ok":false,"reason":"no `script(1)` on PATH"}                                       # not a util-linux guess

npm run ci green end to end (CI_EXIT=0); smoke:tui OK — rendered "MCP Servers" at 371ms and was still running 2000ms later.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comment thread scripts/lib/render-smoke.mjs
…#2147)

Copilot review round 3.

`done()` is first-caller-wins, and the render deadline stayed armed after
first paint. So a marker landing near `timeoutMs` let that timer fire
*inside* the survival window and settle the run as

    child did not render "MCP Servers" within 15000ms

followed by an output tail that visibly contains "MCP Servers". A TUI that
is merely slow fails intermittently, with a diagnostic naming the one
thing that demonstrably did happen — the same wrong-attribution class this
PR exists to remove, and one that reads as flake.

First paint has happened; its deadline no longer has a question to ask, so
`onData` clears it at the moment the marker is observed.

Also hoists `renderTimer`'s declaration beside `surviveTimer`'s. It was a
`const` below both closures that clear it, safe only because `onData` and
`done` run from events rather than the synchronous body. That already held
for `done`; this fix adds a second reader, so the ordering is now stated
rather than relied upon.

The test paints at 250ms with timeoutMs 400 and surviveMs 400, so the
render deadline expires while the survival window is open — the exact
shape. Verified it fails against the module with the clearTimeout removed
and passes with it: a wider timeoutMs would have been green either way,
which is why the numbers are tight.

Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review round 3 — addressed (747f5989)

One finding, and a good one. Mirrored here since pushing marks the inline thread outdated.

The first-paint deadline stayed armed after the marker

done() is first-caller-wins, so a paint landing near timeoutMs let the render timer fire inside the survival window and settle the run as:

child did not render "MCP Servers" within 15000ms

…followed by an output tail that visibly contains MCP Servers. A TUI that is merely slow fails intermittently, with a diagnostic naming the one thing that demonstrably did happen. That is the same wrong-attribution class this PR exists to remove — and it reads as flake, which is how the original defect survived too.

The fix is one line in onData, at the moment the marker is observed:

// Disarm the first-paint deadline. It is first-caller-wins with `done`, so
// leaving it armed means a paint landing near `timeoutMs` lets it fire
// *during* the survival window and settle the run as "did not render
// <marker>" — quoting an output tail that visibly contains the marker.
// First paint has happened; its deadline no longer has a question to ask.
clearTimeout(renderTimer);

renderTimer's declaration also moved up beside surviveTimer's. It was a const sitting below both closures that clear it — safe today only because onData and done run from events rather than the synchronous body. That already held for done; this fix adds a second reader, so the ordering is now stated in a comment rather than relied upon.

Verification

The value of this fix is entirely in a branch that only appears under timing, so I checked the test actually catches it rather than assuming:

✖ a paint landing just under the deadline is not failed by the render timer   # clearTimeout removed
✔ a paint landing just under the deadline is not failed by the render timer   # with the fix

It paints at 250ms with timeoutMs: 400 and surviveMs: 400, so the render deadline expires while the survival window is still open — the exact shape. A more generous timeoutMs would have been green either way, which is why the numbers are deliberately tight.

Worth saying this was reachable in the real smoke, not just in the abstract: SMOKE_TUI_TIMEOUT_MS defaults to 15s against a ~400ms first paint, so the margin is large — but a loaded machine is precisely when a smoke runs slow and when an intermittent red gets written off.

npm run ci green (CI_EXIT=0); smoke:tui OK — rendered "MCP Servers" at 421ms and was still running 2000ms later. test:scripts 209/209.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

README.md:493

  • The suite count is now stale: this clause lists mcp-app-flow, render-smoke, pty, and ensure-test-servers—four additional suites, not two. Change “Two more” to “Four more.”
| `npm run test:scripts`              | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Two more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                       |

Comment thread scripts/lib/render-smoke.mjs
…#2147)

Copilot review round 4.

`exit` can be observed before the child's last output drains — `done()`
already keeps reading until `close` for exactly that reason — so `onData`
can run after the verdict is settled. Recording `markerAt` there is still
right: the message is a thunk rendered at `finish()`, and a marker still
sitting in the pipe when `exit` fired was painted all the same. Arming a
survival timer is not. It could not change the result (`done` is a no-op
once settled), but it holds the event loop open for the whole window,
which under `node --test` is a suite lingering seconds for nothing.

That implies something the comment did not say, and it had already bitten
this PR: if `markerAt` can be set after the exit was observed, any wording
derived from observation ORDER is unstable. The old message read
"rendered at Nms and then exited Mms later", where M was
`since() - markerAt` evaluated at finish — not the paint-to-exit gap at
all, and incoherent whenever the marker surfaced during the drain. The
test asserted on /survival window/, which the losing side of that race
could drop. It could have flaked, and the flake would have looked like
the bug this PR fixes.

So the message now makes no ordering claim and reports both times as the
observations they are, with `exitAt` captured in the exit handler rather
than recomputed later. Assertions moved to the race-stable phrase, plus a
test that runs the paint-then-die stub 12x asserting verdict AND wording
are identical every time — one pass proves nothing about a race.

The ordering could not be forced on darwin, so the guard is reasoned
rather than reproduced, and the reply says so. Two attempts and what they
actually showed: a 4MB payload with the marker last never gets there,
because `process.exit()` discards pending stdout writes (the "before
rendering" branch is correctly taken); marker on stderr with a graceful
exitCode was observed first in all 30 runs, none lingering near the
window. `done()` exists precisely because exit and close are separate
events, so treating their order as guaranteed was worth removing anyway.

Also (suppressed comment): the README's "Two more do the same" clause now
lists four suites, not two, after this PR added render-smoke and pty.

Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review round 4 — both addressed (4af38c71)

Mirrored here because pushing marks the inline thread outdated, and because one of the two was in the collapsed "Suppressed comments" block, which has no thread to reply to.

1. A survival timer could be armed after the verdict was settled (inline)

exit can be observed before the child's last output drains — done() already keeps reading until close for exactly that reason — so onData can run after settled. Applied as framed: record markerAt, don't arm the timer.

markerAt = since();
clearTimeout(renderTimer);
// `exit` can beat the last of the child's buffered output … Recording
// `markerAt` is still right (the deferred message reads it, and a marker that
// only surfaced in the drain was still painted). Arming a timer is not …
if (settled) return;

Keeping the assignment matters for the stated reason: the message is a thunk rendered at finish(), so a marker still in the pipe when exit fired is accounted for by the time it is read.

The part the comment implies but doesn't say — and it had already bitten this PR. If markerAt can be set after the exit was observed, then any wording derived from observation order is unstable. The old message read:

child rendered "…" at 250ms and then exited (code 1) 40ms later

where 40ms was since() - markerAt evaluated at finish() — not the paint-to-exit gap at all, and incoherent whenever the marker surfaced during the drain. My test asserted on /survival window/, which the losing side of that race could drop. It could have flaked, and the flake would have looked like the bug this PR fixes.

The message now makes no ordering claim and reports both times as the observations they are, with exitAt captured in the exit handler rather than recomputed later:

child rendered "MCP Servers" and then exited (code 1) inside the 2000ms survival window — it painted one frame, it did not run (first paint seen at 421ms, exit at 464ms)

Assertions moved to /it painted one frame, it did not run/, stable across both orders, plus a test running the paint-then-die stub 12× asserting verdict and wording are identical every time. One pass proves nothing about a race.

On reproducing it — I could not force the ordering on darwin, and would rather say so than imply otherwise:

  • 4MB payload with the marker last: doesn't get there. process.exit() discards pending stdout writes, so the marker never arrives and the before rendering branch is correctly taken (sawMarkerInOutput: false).
  • Marker on stderr with a graceful process.exitCode = 1, 30 runs: observed first all 30 times; no run lingered near the window (max 75ms against 1500ms).

So the guard is reasoned-and-costless rather than reproduced. done() exists precisely because exit and the stream close are separate events, so treating their order as guaranteed was an assumption worth removing regardless.

2. Stale suite count in the README (suppressed)

Correct — my insertion made it stale. The clause introduces "Two more do the same:" and then lists mcp-app-flow, render-smoke, pty, and ensure-test-servers. Changed to "Four more".

Verification

npm run ci green (CI_EXIT=0); smoke:tui OK — rendered "MCP Servers" at 370ms and was still running 2000ms later; test:scripts 210/210.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

scripts/lib/render-smoke.test.mjs:19

  • These regression stubs force process.exit() immediately after writing the marker to piped stdout. Node may terminate before asynchronous stdout is flushed, so the child can intermittently look like it exited before painting and fail the very assertion intended to pin the post-paint path. Set process.exitCode and let Node drain the write naturally; the child still exits immediately after the marker is delivered.

This issue also appears on line 99 of the same file.

const paintThenExit = (code) =>
  stub(`console.log(${JSON.stringify(MARKER)}); process.exit(${code});`);

scripts/lib/render-smoke.mjs:26

  • “Hard floor” contradicts normalizeMs: any positive duration, including values below these defaults, is accepted (the tests use 400ms). These values are fallback defaults, not minimums, so the comment currently misdocuments the configuration contract.
/** Durations, in ms. Every one is a hard floor as well as a default. */

scripts/smoke-tui.mjs:174

  • The new “raw-mode warning must fail” branch is not exercised by the added tests: render-smoke.test.mjs covers survival outcomes but has no forbidden-output behavior, while the real PTY happy path only covers the regex not matching. A regression that removes or breaks this acceptance-criterion check would therefore stay green. Move the forbidden-output verdict into a testable helper (or add it as a runRenderSmoke option) and drive both matching and non-matching output from a stub.
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})`,
  );

scripts/lib/render-smoke.test.mjs:99

  • This forced exit can truncate the preceding piped stderr write, making the test intermittently miss boom even though the helper correctly drains everything the child actually emitted. Let Node exit naturally with the requested status so the diagnostic assertion is deterministic.
    ...stub(`console.error("boom"); process.exit(3);`),

…ble (#2147)

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 <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review round 5 — all 4 addressed

"generated no new comments" — but the Suppressed comments (4) block had four, all real. Posting here since none of them has a thread to reply to.

1 & 4. process.exit() truncates piped writes — my stubs were racy

Both 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.

This one lands with some force, because I hit exactly this mechanism last round and didn't join the dots. Probing the drain race in round 4, a 4MB write followed by process.exit() reached the parent with the marker missing entirely (sawMarkerInOutput: false). I noted it as an interesting reason my repro attempt failed — and left the same construct sitting in the stubs that pin the paint-then-die path. Intermittently that stub never paints, so the test fails as "exited before rendering": the neighbouring branch, and the wrong diagnostic for the thing it is guarding.

Both now set process.exitCode and let the loop drain. The child still exits immediately after the write — nothing holds the loop open — and the comment records why, since process.exit() is the more obvious spelling and will look like a simplification to the next reader:

// `process.exitCode`, never `process.exit()`. Writing to a *pipe* is
// asynchronous, and `process.exit()` tears the process down without draining …
// 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 …

Same fix for the console.error("boom") stub, whose diagnostic assertion had the same exposure.

2. "hard floor" misdocumented the contract

Correct, and it contradicted the tests two files over — normalizeMs accepts any finite positive value, and the suite passes 400ms against a 2000ms default. It rejects the shape a bad env var produces, not smallness. Reworded:

/**
 * 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.
 */

3. The raw-mode check was unreachable from any test

Also correct, and the sharpest of the four: the real PTY happy path only ever exercises the pattern not matching, so a regression breaking that check would stay green forever — an acceptance-criterion guard with nothing guarding it, which is the recursive version of this PR's whole complaint.

Taken as suggested, as a runRenderSmoke option rather than a call-site helper, so it also runs against the fully drained output:

if (verdict === 0 && forbidOutput?.pattern.test(output)) {
  verdict = 1;
  text = `${forbidOutput.reason}\n${outputTail(output)}`;
}

smoke:tui passes its raw-mode pattern in and drops the post-hoc check. One deliberate asymmetry, documented: 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 same crash.

Three tests, covering both directions plus that asymmetry:

  • forbidOutput fails a run that would otherwise have passed — paints, survives, logs the raw-mode text → fails, with the offending output quoted.
  • forbidOutput leaves a passing run alone when it does not match.
  • forbidOutput does not overwrite an existing failure's diagnostic.

Verification

test:scripts 213/213. Real smoke still green — smoke:tui OK — rendered "MCP Servers" at 511ms and was still running 2000ms later — and the negative check still fails through the refactored path:

smoke:tui FAILED — child rendered "MCP Servers" and then exited (code 1) inside the
2000ms survival window — it painted one frame, it did not run
(first paint seen at 473ms, exit at 772ms)

npm run ci green.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

@cliffhall
cliffhall merged commit 833ad40 into v2/main Aug 27, 2026
3 checks passed
@cliffhall
cliffhall deleted the v2/fix/2147-smoke-tui-pty branch August 27, 2026 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

smoke:tui reports success for a TUI that crashed: it asserts first paint, not survival

2 participants