diff --git a/AGENTS.md b/AGENTS.md index 6ab00bee..d951abac 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -973,7 +973,13 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - `smoke:web:elicit` (`scripts/smoke-web-elicitation.mjs`, #1854) is the app-rendered **elicitation** counterpart of `smoke:web:app`: same prod `--web` server and the same deep-link connect, but it then calls `app_choose_option` from the Tools tab, waits for `[data-testid="app-elicitation"][data-app-elicitation-status="ready"]`, clicks a choice **inside the sandboxed app** (two `frameLocator` hops — the trusted sandbox proxy, then the untrusted app), and asserts the app's standard `ElicitResult` comes back in the *tool result*, i.e. that it reached the server rather than merely the host. It then repeats against `app-elicitation-native-http.json` — the same tool and app on a server that never advertised the nested MCP Apps `elicitation` capability — and asserts the **native** elicitation dialog takes it and no app modal is rendered. That second half is the more valuable one: the failure this feature can produce is not "the app doesn't render" but "an app renders when it should not have been offered one", which strands every user of a server that never opted in. Set `SMOKE_SCREENSHOT_DIR` to capture PNGs of the three states (used for PR proof); unset, it asserts only. Two mechanics worth knowing: the main-view tabs are a Mantine `SegmentedControl`, so there is no `role="tab"` — the clickable element is the sibling `label[for$="-Tools"]`; and the prompt string also appears in the (hidden) Protocol-tab payload, so the fallback assertion is scoped to the dialog rather than a bare text lookup. - **The build gate for the browser-externalized-builtin class (#1769)** is the earlier, more complete companion to `smoke:web:browser`. A Vite plugin in `clients/web/vite.config.ts` (logic in `clients/web/server/browser-externalized-builtin-gate.ts`, unit-tested) turns Vite 8's _browser-externalization warning_ (`Module "node:*" has been externalized for browser compatibility`) into a hard `vite build` error, so a Node built-in in the browser graph now **fails `npm run build` / `validate`** instead of shipping a `{}` stub. This catches **both** the _called-at-init_ case (which `smoke:web:browser` also catches, but later/at runtime) **and** the _imported-but-never-called_ case (the `{}` stub that is invisible to the runtime smoke "by design" — see above). Because rolldown **swallows a throw inside `onLog`** (the one hook where a thrown error doesn't abort — verified against vite@8.0.0), the plugin _records_ the warning in `onLog` and re-throws in `buildEnd`. There is **no stable log `code`**, so the gate keys off the documented message phrasing; `npm run verify:build-gate` (`scripts/verify-build-gate.mjs`, in `npm run ci` and the GitHub workflow) runs a real build with a `node:fs` probe forced into `src/main.tsx` and asserts the build fails via the gate — the only check that catches the message phrasing **drifting** in a future Vite bump and silently disabling the gate. The gate is scoped to `vite build` (`apply: 'build'`) — never `vite dev` or the vitest projects — **and** to the browser (`client`) environment (`applyToEnvironment`), so a future SSR/node environment built from this config isn't failed for a legitimate `node:*` import; the Node runner build (tsup, `build:runner`) is a separate config where built-ins are legitimate. `smoke:web:browser` stays as the runtime backstop for crashes the build can't reason about. -- `smoke:cli` (`scripts/smoke-cli.mjs`) drives `mcp-inspector --cli` through the built launcher against the bundled stdio test server via a temp `--catalog`: it asserts `tools/list` returns the server's tools (real connect over stdio), the default writable catalog is seeded empty on first run, a missing read-only `--config` errors without seeding, and `--catalog` + `--config` is rejected. `smoke:tui` (`scripts/smoke-tui.mjs`) launches `mcp-inspector --tui --catalog ` and asserts the Ink app renders its first frame (the "MCP Servers" panel) within a timeout, then SIGTERMs it — a shallow boot/render check, not full interaction. **`smoke:tui` is local-only: it self-skips when `process.env.CI` is set**, because the Ink TUI needs a real TTY (raw mode) that headless CI lacks — so run it (via `npm run smoke`) on your own machine before pushing. Both build `test-servers/build` on demand if it's missing. +- `smoke:cli` (`scripts/smoke-cli.mjs`) drives `mcp-inspector --cli` through the built launcher against the bundled stdio test server via a temp `--catalog`: it asserts `tools/list` returns the server's tools (real connect over stdio), the default writable catalog is seeded empty on first run, a missing read-only `--config` errors without seeding, and `--catalog` + `--config` is rejected. `smoke:tui` (`scripts/smoke-tui.mjs`) launches `mcp-inspector --tui --catalog ` **under a pseudoterminal**, asserts the Ink app renders its first frame (the "MCP Servers" panel) within a timeout, then waits `SMOKE_TUI_SURVIVE_MS` (default 2s) and asserts it is **still running** before SIGTERMing it — a shallow boot/render/survival check, not full interaction. Both build `test-servers/build` on demand if it's missing. + + **The survival half is the assertion, and it is the whole point (#2147).** This smoke used to settle OK the instant the marker appeared and never look at the child again. It was spawning the TUI with `stdio: ["ignore", …]`, i.e. stdin on `/dev/null`; Ink mounts `useInput`, `useInput` needs raw mode, and raw mode is a property of the **file descriptor** — so the TUI painted one frame and died ~40ms later with `Raw mode is not supported on the current process.stdin`. The smoke won that ~40ms race and printed OK. What it asserted was *first paint*, while its own header claimed "boots and renders without crashing", and it had never once verified a running TUI **on any machine** — `stdio: ["ignore", …]` makes the child's stdin `/dev/null` whether or not the developer's own terminal is a TTY. The race also cut both ways: an `exit` processed first failed the run, which reads as flake rather than as the standing defect it was. + + Two pieces, and the order matters. **`scripts/lib/pty.mjs`** gives the child a real terminal via `script(1)` — no dependency, and the 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)` (Windows) **skips** rather than running without one, since that is a guaranteed failure and not a weaker check. **`scripts/lib/render-smoke.mjs`** is the fix: it requires the child to outlive its first frame. 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 (the old harness passes that stub; this one must not), rather than in a smoke that only ever walks its own happy path. Do **not** collapse the survival wait back into "resolve on the marker" to save two seconds. + + **`smoke:tui` is still local-only: it self-skips when `process.env.CI` is set** — now by decision rather than by capability. The PTY removes the technical blocker the skip used to cite; whether it joins GitHub CI is a separate call, and #2146 deliberately keeps the other local-only smokes out. So run it (via `npm run smoke`) on your own machine before pushing: a local-only gate is exactly where a false green is least likely to be caught by anything else. - Storybook play-function tests (`clients/web` `test:storybook`) run in headless Chromium via `@vitest/browser-playwright` (~10s). They are part of `npm run ci` (which installs Playwright chromium first); kept out of `validate` because they need the browser binary and are slower than the unit suite. ### The web smokes are engine-parameterized; Firefox is in the pre-push gate, not CI (#2086) diff --git a/README.md b/README.md index b2e05c2d..ddca719f 100644 --- a/README.md +++ b/README.md @@ -486,11 +486,11 @@ Each client self-validates from its own folder; the root scripts chain them. The | ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `npm run validate` | Runs the three durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:typecheck-coverage` (every one lands in a tsconfig project), `verify:dep-lockstep` (no dependency reaching one `tsc` program from two installs skews across them) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | | `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). | -| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus three headless-browser smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. All three take their engine from `SMOKE_BROWSER` — see [Supported browsers](#supported-browsers). | +| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web — the TUI one runs under a **pseudoterminal** and asserts the Ink app is still running two seconds after its first frame, not merely that it painted one, [#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)), plus three headless-browser smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. All three take their engine from `SMOKE_BROWSER` — see [Supported browsers](#supported-browsers). | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. | | `npm run verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// ` module banners, so it covers both shapes — a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. Part of `npm run ci`. | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | -| `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; 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. | +| `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. | | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | | `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. Runs in `validate`. | `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → Storybook. A strict superset of GitHub CI — `smoke:web:firefox` (and `smoke:tui`) run only here. | diff --git a/clients/launcher/README.md b/clients/launcher/README.md index e2fc94f4..c1d58311 100644 --- a/clients/launcher/README.md +++ b/clients/launcher/README.md @@ -87,10 +87,18 @@ through the built launcher artifact (beyond the `--help` checks in resolution paths (default-catalog seed-on-missing, read-only `--config` error-without-seed, `--catalog`/`--config` conflict). - `npm run smoke:tui` (`scripts/smoke-tui.mjs`) — launches - `mcp-inspector --tui --catalog ` and asserts the Ink app renders its - first frame within a timeout, then shuts it down (a shallow boot/render + `mcp-inspector --tui --catalog ` **under a pseudoterminal**, asserts the + Ink app renders its first frame within a timeout, then waits and asserts it is + **still running** before shutting it down (a shallow boot/render/survival check, not full interaction). + The second half is the assertion (#2147). Spawned with its stdin on + `/dev/null`, Ink cannot enter raw mode for `useInput`, so the TUI painted one + frame and exited 1 about 40ms later — and this smoke, which settled OK on the + first frame, won that race and reported success on every machine. It is + local-only (self-skips under `CI`), which is precisely where a false green + goes unnoticed. + Both build `test-servers/build` on demand if it is missing. ## Development diff --git a/scripts/lib/pty.mjs b/scripts/lib/pty.mjs new file mode 100644 index 00000000..27d45a5a --- /dev/null +++ b/scripts/lib/pty.mjs @@ -0,0 +1,136 @@ +/** + * Wrap a command so it runs attached to a pseudoterminal (#2147). + * + * `smoke:tui` spawns the Ink TUI with `stdio: ["ignore", …]`, which makes the + * child's stdin `/dev/null`. Ink mounts `useInput`, `useInput` needs raw mode, + * and raw mode is a property of the *file descriptor* — so the child threw + * "Raw mode is not supported on the current process.stdin" and exited 1 about + * 40ms after painting its first frame, on every machine, TTY or not. A parent + * cannot assert raw mode on a child's behalf; the fd has to be a terminal. + * + * `script(1)` allocates one, with no dependency to install. Measured here on + * darwin: without it the TUI paints at ~400ms and is dead at ~440ms; with it + * the same TUI is still running at 6s and never logs the raw-mode error. + * + * The invocation differs by flavor, which is why this is a module rather than + * a string literal at the call site: + * + * BSD (macOS) script -q /dev/null argv, no shell + * util-linux script -qec "" /dev/null ONE shell string + * busybox script -qc "" /dev/null as above, no -e + * + * The `-c` flavors take a single command *string* that the child shell parses, + * so every word has to be quoted on the way in — an unquoted temp path with a + * space would otherwise split into two arguments. + * + * `-e` (util-linux) makes `script` exit with the child's status rather than its + * own. busybox has no such flag, so under it a crashed child reports 0. That is + * survivable and deliberately not worked around: the smoke's assertion is the + * child's `exit` *event*, not its code — the code only sharpens the diagnostic. + */ + +import { spawnSync } from "node:child_process"; + +/** Platforms whose `script` takes the BSD argv form. */ +const BSD_PLATFORMS = new Set(["darwin", "freebsd", "openbsd", "netbsd"]); + +/** + * Quote one argument for a POSIX shell command string. + * + * Single quotes are literal for everything except a single quote itself, which + * has to leave the quoted run, emit an escaped quote, and re-enter it. + * + * @param {string} arg + * @returns {string} + */ +export function shellQuote(arg) { + return `'${String(arg).replaceAll("'", `'\\''`)}'`; +} + +/** + * Decide which `script` flavor to build for. + * + * The version output is consulted first because it is the only *evidence*: + * "linux" does not imply util-linux (Alpine ships busybox, whose `script` + * rejects `-e`). Platform is the fallback for the case where `script --version` + * tells us nothing, which is itself informative — BSD `script` has no + * `--version` and fails the probe. + * + * @param {object} opts + * @param {string} opts.platform `process.platform`. + * @param {string} [opts.versionOutput] Combined stdout+stderr of `script --version`. + * @returns {"bsd" | "util-linux" | "busybox" | null} null when unsupported. + */ +export function scriptFlavorFor({ platform, versionOutput = "" }) { + if (/util-linux/i.test(versionOutput)) return "util-linux"; + if (/busybox/i.test(versionOutput)) return "busybox"; + if (BSD_PLATFORMS.has(platform)) return "bsd"; + // Linux with an unidentifiable `script` is overwhelmingly util-linux; guess + // it rather than skip the smoke. A wrong guess fails loudly with `script`'s + // own usage error in the captured output, which is diagnosable — silently + // skipping is the outcome this issue exists to stop rewarding. + if (platform === "linux") return "util-linux"; + // win32 and anything else: no `script(1)`. `node-pty` is the portable answer + // if this smoke ever has to run there. + return null; +} + +/** + * Build the PTY-wrapped spawn arguments for a command. + * + * @param {object} opts + * @param {string} opts.command + * @param {string[]} [opts.args] + * @param {"bsd" | "util-linux" | "busybox"} opts.flavor + * @returns {{ command: string, args: string[] }} + */ +export function ptyCommand({ command, args = [], flavor }) { + if (flavor === "bsd") { + return { command: "script", args: ["-q", "/dev/null", command, ...args] }; + } + if (flavor === "util-linux" || flavor === "busybox") { + const line = [command, ...args].map(shellQuote).join(" "); + const flags = flavor === "util-linux" ? "-qec" : "-qc"; + return { command: "script", args: [flags, line, "/dev/null"] }; + } + throw new Error(`unknown script(1) flavor: ${flavor}`); +} + +/** + * Probe the local `script(1)` for its flavor. + * + * @param {(cmd: string, args: string[]) => { stdout?: string, stderr?: string }} [runner] + * Injected for tests; defaults to a real `spawnSync`. + * @returns {string} Combined output, or "" if `script` could not be run at all. + */ +export function probeScriptVersion( + runner = (cmd, args) => + spawnSync(cmd, args, { encoding: "utf8", timeout: 5000 }), +) { + try { + const r = runner("script", ["--version"]) ?? {}; + return `${r.stdout ?? ""}${r.stderr ?? ""}`; + } catch { + return ""; + } +} + +/** + * Resolve a PTY wrapper for this machine, or null if none is available. + * + * @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} + */ +export function resolvePtyWrapper({ + platform = process.platform, + probe = probeScriptVersion, +} = {}) { + const flavor = scriptFlavorFor({ platform, versionOutput: probe() }); + if (!flavor) return null; + return { + flavor, + wrap: ({ command, args = [] }) => ptyCommand({ command, args, flavor }), + }; +} diff --git a/scripts/lib/pty.test.mjs b/scripts/lib/pty.test.mjs new file mode 100644 index 00000000..21adbaad --- /dev/null +++ b/scripts/lib/pty.test.mjs @@ -0,0 +1,160 @@ +import { strict as assert } from "node:assert"; +import test from "node:test"; + +import { + ptyCommand, + probeScriptVersion, + resolvePtyWrapper, + scriptFlavorFor, + shellQuote, +} from "./pty.mjs"; + +// The flavors are not interchangeable: BSD `script` takes argv after the +// typescript file, util-linux takes ONE shell string before it, and busybox +// takes that string without `-e`. Getting it wrong is not a subtle failure — +// `script` refuses to start, and the smoke reports it as a TUI crash. +test("scriptFlavorFor prefers the version probe over the platform", () => { + assert.equal( + scriptFlavorFor({ + platform: "linux", + versionOutput: "script from util-linux 2.39.3", + }), + "util-linux", + ); + assert.equal( + scriptFlavorFor({ + platform: "linux", + versionOutput: "BusyBox v1.36.1 (2024-01-01) multi-call binary.", + }), + "busybox", + ); + // Alpine is the reason the probe exists at all: "linux" does not imply + // util-linux, and busybox's `script` rejects `-e`. + assert.notEqual( + scriptFlavorFor({ platform: "linux", versionOutput: "BusyBox v1.36.1" }), + "util-linux", + ); +}); + +test("scriptFlavorFor falls back to the platform when the probe says nothing", () => { + // BSD `script` has no --version, so failing the probe is itself the signal. + assert.equal(scriptFlavorFor({ platform: "darwin" }), "bsd"); + assert.equal(scriptFlavorFor({ platform: "freebsd" }), "bsd"); + // An unidentifiable `script` on linux is overwhelmingly util-linux. Guess it: + // a wrong guess fails loudly with `script`'s own usage error in the captured + // output, whereas skipping is the silence this issue exists to stop. + assert.equal(scriptFlavorFor({ platform: "linux" }), "util-linux"); +}); + +test("scriptFlavorFor returns null where there is no script(1)", () => { + assert.equal(scriptFlavorFor({ platform: "win32" }), null); + assert.equal( + scriptFlavorFor({ platform: "win32", versionOutput: "not a thing" }), + null, + ); +}); + +test("ptyCommand builds the BSD argv form", () => { + assert.deepEqual( + ptyCommand({ + command: "/bin/node", + args: ["app.js", "--x"], + flavor: "bsd", + }), + { + command: "script", + args: ["-q", "/dev/null", "/bin/node", "app.js", "--x"], + }, + ); +}); + +test("ptyCommand builds the util-linux shell-string form with -e", () => { + // -e is what makes `script` exit with the child's status instead of its own. + assert.deepEqual( + ptyCommand({ + command: "/bin/node", + args: ["app.js"], + flavor: "util-linux", + }), + { + command: "script", + args: ["-qec", "'/bin/node' 'app.js'", "/dev/null"], + }, + ); +}); + +test("ptyCommand omits -e for busybox, which has no such flag", () => { + const { args } = ptyCommand({ + command: "node", + args: ["app.js"], + flavor: "busybox", + }); + assert.equal(args[0], "-qc"); + assert.deepEqual(args, ["-qc", "'node' 'app.js'", "/dev/null"]); +}); + +test("ptyCommand quotes arguments for the shell-string flavors", () => { + // The smoke passes an mkdtemp path; a space in it would otherwise split into + // two arguments inside the shell `script -c` runs. + const { args } = ptyCommand({ + command: "/usr/local/my node/bin/node", + args: ["/tmp/a dir/catalog.json", "it's-fine"], + flavor: "util-linux", + }); + assert.equal( + args[1], + `'/usr/local/my node/bin/node' '/tmp/a dir/catalog.json' 'it'\\''s-fine'`, + ); +}); + +test("shellQuote survives an embedded single quote", () => { + assert.equal(shellQuote("it's"), `'it'\\''s'`); + assert.equal(shellQuote("plain"), "'plain'"); +}); + +test("probeScriptVersion combines both streams and never throws", () => { + assert.equal( + probeScriptVersion(() => ({ stdout: "out ", stderr: "err" })), + "out err", + ); + // BSD `script --version` errors; spawnSync can also return a bare object. + assert.equal( + probeScriptVersion(() => { + throw new Error("ENOENT"); + }), + "", + ); + assert.equal( + probeScriptVersion(() => ({})), + "", + ); +}); + +test("resolvePtyWrapper wraps with the resolved flavor, or reports none", () => { + const wrapper = resolvePtyWrapper({ + platform: "linux", + probe: () => "script from util-linux 2.39.3", + }); + assert.equal(wrapper.flavor, "util-linux"); + assert.deepEqual(wrapper.wrap({ command: "node", args: ["a.js"] }), { + command: "script", + args: ["-qec", "'node' 'a.js'", "/dev/null"], + }); + // `args` is optional at the call site. + assert.deepEqual(wrapper.wrap({ command: "node" }).args[1], "'node'"); + + assert.equal(resolvePtyWrapper({ platform: "win32", probe: () => "" }), null); +}); + +test("resolvePtyWrapper resolves on this machine unless it is Windows", () => { + // Not a tautology: it runs the real probe, so a `script` that has vanished + // from PATH, or a platform nobody has tried, surfaces here rather than as an + // opaque smoke failure. + const wrapper = resolvePtyWrapper(); + if (process.platform === "win32") { + assert.equal(wrapper, null); + } else { + assert.ok(wrapper, `no PTY wrapper resolved on ${process.platform}`); + assert.equal(wrapper.wrap({ command: "node" }).command, "script"); + } +}); diff --git a/scripts/lib/render-smoke.mjs b/scripts/lib/render-smoke.mjs new file mode 100644 index 00000000..96ff3a57 --- /dev/null +++ b/scripts/lib/render-smoke.mjs @@ -0,0 +1,225 @@ +/** + * Spawn a terminal app, wait for it to paint a marker, and then wait again to + * see whether it is still running (#2147). + * + * Extracted from `scripts/smoke-tui.mjs` for the same reason + * `announced-child.mjs` was extracted from `smoke-web-app.mjs` (#2000): a smoke + * script only ever exercises its own happy path, so the branch that matters + * most here — a child that paints the marker and *then* dies — is unreachable + * from the real TUI once it is fixed. Driving this from a test against a stub + * that paints and immediately exits 1 is what keeps the assertion honest. + * + * The defect it encodes against: `smoke:tui` used to resolve OK the instant the + * marker appeared and never looked at the child again. The TUI was exiting 1 + * about 40ms later, every run, on every machine, and the smoke won that race + * and printed OK. So it asserted "painted once", while its own header claimed + * "boots and renders without crashing". Worse, the race cut both ways: had the + * `exit` event been processed first, the same run would have failed, which + * reads as flake rather than as the standing defect it was. + * + * Hence `surviveMs`. First paint is not survival, and nothing shorter than an + * explicit "still alive N ms later" check can tell the two apart. + */ + +import { spawn } from "node:child_process"; + +/** + * 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 + * than lead with an escape-code fragment that mangles the line after it. + * + * @param {string} output + * @param {number} [limit] + * @returns {string} + */ +export function outputTail(output, limit = 800) { + const tail = output.slice(-limit); + const nl = tail.indexOf("\n"); + return output.length > limit && nl !== -1 ? tail.slice(nl + 1) : tail; +} + +/** + * @param {object} opts + * @param {string} opts.command Executable to spawn. + * @param {string[]} [opts.args] Arguments for it. + * @param {object} [opts.spawnOptions] Merged into the `spawn` options + * (`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.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). + * @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 {(message: string) => void} [opts.warn] + * @returns {Promise<{ code: 0 | 1, message: string, output: string }>} + * Resolves only after the child's streams have closed, so `output` is + * complete and the caller may safely remove a directory the child was using. + */ +export function runRenderSmoke({ + command, + args = [], + spawnOptions = {}, + marker, + timeoutMs = 15_000, + surviveMs = 2_000, + exitGraceMs = 5_000, + drainMs = 500, + onSpawn = () => {}, + warn = (m) => console.warn(m), +}) { + return new Promise((resolve) => { + const child = spawn(command, args, { + ...spawnOptions, + // stdin is `ignore` (i.e. /dev/null) deliberately, even under a PTY: BSD + // `script` refuses to start when its own stdin is a pipe + // ("tcgetattr/ioctl: Operation not supported on socket"). It reads the + // immediate EOF and forwards one ^D into the pty before the app mounts, + // which is harmless — verified against the real TUI, which survives it. + stdio: ["ignore", "pipe", "pipe"], + }); + // Publish before waiting, so every path below leaves the caller holding a + // stoppable handle rather than an orphan (the #2000 rule). + onSpawn(child); + + let output = ""; + let settled = false; + let finished = false; + let childExited = false; + let childClosed = false; + let markerAt = null; + /** @type {ReturnType | undefined} */ + let surviveTimer; + + const t0 = Date.now(); + const since = () => Date.now() - t0; + + const finish = (code, message) => { + if (finished) return; + finished = true; + resolve({ + code, + message: typeof message === "function" ? message() : message, + output, + }); + }; + + // Settle the verdict, then wait for the child to actually close before + // resolving (#1801). The caller's work dir doubles as the app's HOME, so it + // is still being written to when we signal; removing it while those writes + // land fails with ENOTEMPTY. Waiting on `close` rather than `exit` also + // covers a *spawn failure*, which emits `error` + `close` and never `exit` + // — and guarantees the pipes are drained, which is what makes the thunked + // messages below complete rather than truncated at the interesting line. + // + // Deliberately not `lib/child-cleanup.mjs`'s `stopChild()`: that resolves + // on the FIRST of exit/close, whereas this wait must keep going for `close` + // after `exit`, on a shorter re-armed deadline, for the drain. + const done = (code, message) => { + if (settled) return; + settled = true; + clearTimeout(renderTimer); + clearTimeout(surviveTimer); + if (childClosed) { + finish(code, message); + return; + } + const forceKill = setTimeout(() => child.kill("SIGKILL"), exitGraceMs); + + let deadline; + const armDeadline = (ms, warning) => { + clearTimeout(deadline); + deadline = setTimeout(() => { + warn(`render-smoke — ${warning}`); + finish(code, message); + }, ms); + }; + armDeadline( + exitGraceMs * 2, + `child did not close its output streams within ${exitGraceMs * 2}ms; giving up on the drain`, + ); + + // Once the child itself is gone only the pipe drain remains, so the wait + // drops to `drainMs`. `close` is bounded by whoever holds the stdio + // pipes, which can outlive the direct child: any descendant that + // inherited them keeps it pending. + const drainOnly = () => { + clearTimeout(forceKill); + armDeadline( + drainMs, + `child exited but held its output streams open for ${drainMs}ms (a descendant may have inherited them); giving up on the drain`, + ); + }; + if (childExited) drainOnly(); + else child.once("exit", drainOnly); + + child.once("close", () => { + clearTimeout(forceKill); + clearTimeout(deadline); + finish(code, message); + }); + + // Only signal a child that is still running: on the crash and + // spawn-failure paths there is nothing left to signal, and we are here + // purely to wait out the remaining `close`. + if ( + child.exitCode === null && + child.signalCode === null && + !child.killed + ) { + child.kill("SIGTERM"); + } + }; + + const onData = (chunk) => { + output += chunk.toString(); + if (markerAt !== null || !output.includes(marker)) return; + markerAt = since(); + // First paint is not the verdict — it only starts the clock on the + // question this smoke actually asks. + surviveTimer = setTimeout(() => { + done( + 0, + `rendered "${marker}" at ${markerAt}ms and was still running ${surviveMs}ms later`, + ); + }, surviveMs); + }; + + child.stdout.on("data", onData); + child.stderr.on("data", onData); + + // Registered before done()'s own one-shot listener, so this always runs + // first — done() can trust `childClosed` even when called from a handler. + child.on("close", () => { + childClosed = true; + }); + + child.on("exit", (code, signal) => { + childExited = true; + if (settled) return; + const how = signal ? `signal ${signal}` : `code ${code}`; + done(1, () => + markerAt === null + ? `child exited (${how}) before rendering "${marker}"\n${outputTail(output)}` + : `child rendered "${marker}" at ${markerAt}ms and then exited (${how}) ` + + `${since() - markerAt}ms later, inside the ${surviveMs}ms survival window — ` + + `it painted one frame, it did not run\n${outputTail(output)}`, + ); + }); + + child.on("error", (err) => { + done(1, `failed to spawn ${command}: ${err.message}`); + }); + + const renderTimer = setTimeout(() => { + done( + 1, + () => + `child did not render "${marker}" within ${timeoutMs}ms\n${outputTail(output)}`, + ); + }, timeoutMs); + }); +} diff --git a/scripts/lib/render-smoke.test.mjs b/scripts/lib/render-smoke.test.mjs new file mode 100644 index 00000000..cae3f7fd --- /dev/null +++ b/scripts/lib/render-smoke.test.mjs @@ -0,0 +1,116 @@ +import { strict as assert } from "node:assert"; +import test from "node:test"; + +import { outputTail, runRenderSmoke } from "./render-smoke.mjs"; + +const MARKER = "MCP Servers"; + +/** A stub terminal app, driven as a real child process rather than a spy. */ +function stub(body) { + return { command: process.execPath, args: ["-e", body] }; +} + +const paintThenExit = (code) => + stub(`console.log(${JSON.stringify(MARKER)}); process.exit(${code});`); +const paintThenLive = stub( + `console.log(${JSON.stringify(MARKER)}); setInterval(() => {}, 1000);`, +); + +const opts = { + marker: MARKER, + timeoutMs: 5000, + surviveMs: 400, + exitGraceMs: 500, + drainMs: 200, + warn: () => {}, +}; + +// The regression this module exists for. The old harness resolved OK on the +// marker and never looked again, so this exact stub — paint, then die — passed. +test("fails when the child paints the marker and then exits non-zero", async () => { + const r = await runRenderSmoke({ ...opts, ...paintThenExit(1) }); + assert.equal(r.code, 1, `expected failure, got: ${r.message}`); + assert.match(r.message, /code 1/); + assert.match(r.message, /survival window/); + // The diagnostic must not read as a crash *before* first paint — that was a + // different defect, and conflating them sends the reader to the wrong place. + assert.doesNotMatch(r.message, /before rendering/); +}); + +// Exiting 0 right after painting is the same failure: the app did not run. It +// is called out separately because `script(1)` under busybox reports 0 for a +// crashed child, so the verdict cannot rest on the exit code. +test("fails when the child paints the marker and then exits zero", async () => { + const r = await runRenderSmoke({ ...opts, ...paintThenExit(0) }); + assert.equal(r.code, 1, `expected failure, got: ${r.message}`); + assert.match(r.message, /it painted one frame, it did not run/); +}); + +test("passes when the child paints the marker and keeps running", async () => { + const r = await runRenderSmoke({ ...opts, ...paintThenLive }); + assert.equal(r.code, 0, `expected pass, got: ${r.message}`); + assert.match(r.message, /still running 400ms later/); +}); + +test("fails when the child exits before painting", async () => { + const r = await runRenderSmoke({ + ...opts, + ...stub(`console.error("boom"); process.exit(3);`), + }); + assert.equal(r.code, 1); + assert.match(r.message, /before rendering/); + assert.match(r.message, /code 3/); + // The tail is quoted after the drain, so the crash reason survives. + assert.match(r.message, /boom/); +}); + +test("fails when the child never paints, and kills it", async () => { + let spawned; + const r = await runRenderSmoke({ + ...opts, + ...stub(`setInterval(() => {}, 1000);`), + timeoutMs: 300, + onSpawn: (c) => (spawned = c), + }); + assert.equal(r.code, 1); + assert.match(r.message, /did not render/); + // The #2000 ownership rule: the child is published before the wait, so the + // timeout path leaves a stoppable handle rather than an orphan. + assert.ok(spawned, "onSpawn was never called"); + assert.notEqual( + spawned.exitCode === null && spawned.signalCode === null, + true, + ); +}); + +test("reports a spawn failure rather than throwing it uncaught", async () => { + const r = await runRenderSmoke({ + ...opts, + command: "/definitely/not/a/binary-2147", + args: [], + }); + assert.equal(r.code, 1); + assert.match(r.message, /failed to spawn/); +}); + +test("scans stderr as well as stdout for the marker", async () => { + // An Ink app writing to stderr is missed entirely by a stdout-only scan, + // which then times out with an empty diagnostic. + const r = await runRenderSmoke({ + ...opts, + ...stub( + `console.error(${JSON.stringify(MARKER)}); setInterval(() => {}, 1000);`, + ), + }); + assert.equal(r.code, 0, `expected pass, got: ${r.message}`); +}); + +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 + // the first newline instead of leading with a fragment. + const long = "x".repeat(50) + "\nkeep me"; + assert.equal(outputTail(long, 20), "keep me"); + // No newline in the tail at all: nothing to trim to, so return it as sliced. + assert.equal(outputTail("y".repeat(50), 10), "y".repeat(10)); +}); diff --git a/scripts/smoke-tui.mjs b/scripts/smoke-tui.mjs index f5a8cd9d..ec8314d7 100644 --- a/scripts/smoke-tui.mjs +++ b/scripts/smoke-tui.mjs @@ -6,13 +6,28 @@ * the Ink app, so the launcher → TUI → core path and `--catalog` loading went * unverified by the launcher smokes. This script launches the built launcher in * `--tui` mode against a temp `--catalog`, waits for the app to render its first - * frame (the "MCP Servers" panel) within a timeout, then sends SIGTERM and - * exits. + * frame (the "MCP Servers" panel), waits again to confirm it is *still running*, + * then sends SIGTERM and exits. * - * It asserts the TUI *boots and renders* without crashing — not full - * interaction (driving an Ink UI deterministically in CI is flaky, so this is - * intentionally a shallow render check). Exits non-zero on a crash-before-render - * or a render timeout. + * It asserts the TUI boots, renders, and keeps running — not full interaction + * (driving an Ink UI deterministically is flaky, so this is intentionally a + * shallow check). Exits non-zero on a crash before or after first paint, or on + * a render timeout. + * + * ## Why it runs under a pseudoterminal (#2147) + * + * It used to spawn the TUI with its stdin on `/dev/null` and settle OK the + * moment the marker appeared. Ink mounts `useInput`, `useInput` needs raw mode, + * and raw mode is a property of the file descriptor — so the TUI painted one + * frame and died ~40ms later with "Raw mode is not supported on the current + * process.stdin". The smoke won that race and reported OK, on every machine, + * TTY or not: what it actually asserted was *first paint*, not survival. + * + * Two changes, and the second is the fix: `scripts/lib/pty.mjs` gives the child + * a real terminal via `script(1)`, and `scripts/lib/render-smoke.mjs` requires + * it to outlive its first frame. Without the survival assertion a future + * harness change reintroduces exactly this false green and nothing notices — + * which is why that assertion lives in a tested module rather than here. * * Expects `clients/launcher/build` and `clients/tui/build` to be built first * (the validate / CI ordering guarantees this). The bundled stdio test server @@ -20,39 +35,63 @@ * `scripts/lib/ensure-test-servers.mjs` for why presence isn't freshness. */ -import { spawn } from "node:child_process"; import { mkdtempSync, existsSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; 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"; 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); +// 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); +// 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/; function fail(message) { console.error(`smoke:tui FAILED — ${message}`); process.exit(1); } -// The Ink TUI requires a real TTY for raw-mode keyboard input. Headless CI has -// none, so the app throws "Raw mode is not supported" on mount and exits before -// it can render its first frame — making this boot/render check inherently a -// LOCAL-only smoke (its own design notes call driving Ink in CI flaky). Skip it -// under CI rather than fail spuriously: the TUI is still built and unit-tested -// there; only this terminal-dependent render check is local-only. -if (process.env.CI) { - console.log( - "smoke:tui SKIPPED — Ink needs a real TTY (raw mode), unavailable in CI; run it locally", - ); +function skip(message) { + console.log(`smoke:tui SKIPPED — ${message}`); process.exit(0); } +// Kept out of GitHub CI by decision, not by capability. The PTY above removes +// the technical blocker this skip used to cite (a headless runner has no TTY), +// but whether this smoke joins CI is a separate call for the maintainers, and +// #2146 is deliberately keeping the other local-only smokes out. Making it +// *valid* stands on its own: it is a local-only gate, which is exactly where a +// false green is least likely to be caught by anything else. +if (process.env.CI) { + skip( + "local-only by decision (see the header); the TUI is built and unit-tested in CI", + ); +} + if (!existsSync(launcher)) { fail(`launcher build not found at ${launcher} — run \`npm run build\` first`); } + +// No `script(1)` (Windows). Running without a PTY is not a weaker check, it is +// 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) { + skip( + `no pseudoterminal available on ${process.platform} — the Ink TUI needs raw mode, ` + + `which requires a real terminal fd (see scripts/lib/pty.mjs)`, + ); +} + // Rebuilt on every run — presence is not freshness (#2111). let testServer; try { @@ -76,183 +115,58 @@ writeFileSync( }), ); -const child = spawn( - process.execPath, - [launcher, "--tui", "--catalog", catalogPath], - { - cwd: repoRoot, - // Redirect HOME so the TUI's storage never touches the real ~/.mcp-inspector. - // Pin MCP_OAUTH_CALLBACK_URL="" (empty reads as unset) so an ambient - // non-loopback value can't crash the TUI before render via the loopback - // callback guard — same class smoke-cli.mjs's SMOKE_BASE_ENV neutralizes. - env: { - ...process.env, - MCP_OAUTH_CALLBACK_URL: "", - HOME: work, - USERPROFILE: work, +const { command, args } = pty.wrap({ + command: process.execPath, + args: [launcher, "--tui", "--catalog", catalogPath], +}); + +let result; +try { + result = await runRenderSmoke({ + command, + args, + marker: RENDER_MARKER, + timeoutMs: TIMEOUT_MS, + surviveMs: SURVIVE_MS, + exitGraceMs: EXIT_GRACE_MS, + spawnOptions: { + cwd: repoRoot, + // Redirect HOME so the TUI's storage never touches the real + // ~/.mcp-inspector. Pin MCP_OAUTH_CALLBACK_URL="" (empty reads as unset) + // so an ambient non-loopback value can't crash the TUI before render via + // the loopback callback guard — same class smoke-cli.mjs's + // SMOKE_BASE_ENV neutralizes. + env: { + ...process.env, + MCP_OAUTH_CALLBACK_URL: "", + HOME: work, + USERPROFILE: work, + }, }, - stdio: ["ignore", "pipe", "pipe"], - }, -); - -let output = ""; -let settled = false; -let childExited = false; -let childClosed = false; - -// How long to let the TUI wind down after SIGTERM before escalating to SIGKILL, -// and then before giving up on the close event entirely. -const EXIT_GRACE_MS = Number(process.env.SMOKE_TUI_EXIT_GRACE_MS ?? 5000); -// Once the child itself is gone, only the pipe drain remains, so the wait drops -// to this. `close` is bounded by whoever holds the stdio pipes, which can -// outlive the direct child: any descendant that inherited them keeps it pending. -// The TUI spawns nothing at boot today (it does not auto-connect), so this -// never fires — but that reason lives outside this file, so cap it here rather -// than inherit an unrelated process's lifetime if that ever changes. -// -// The cost of capping: on that path we remove the work dir while a descendant -// holding HOME=work may still be writing to it — the #1801 race, re-entered -// deliberately. That is only acceptable because cleanup() warns instead of -// throwing, so the worst case is a warning plus a leaked temp dir in tmpdir, -// never a red smoke. The two are load-bearing for each other. -const DRAIN_MS = 500; - -function cleanup() { + warn: (m) => console.warn(m.replace(/^render-smoke/, "smoke:tui")), + }); +} finally { // Never turn a passing smoke into a failure over a leftover temp dir; the OS - // reclaims tmpdir anyway. On the normal path a warning should not fire, since - // removal waits for the child's `close` (see done()) — but the two give-up - // branches there call finish() without it, and a process may still hold the - // dir, so it is expected rather than anomalous on those. + // reclaims tmpdir anyway. runRenderSmoke resolves only after the child's + // `close`, so on the normal path the #1801 ENOTEMPTY race is already closed + // and a warning here should not fire — its give-up branches resolve without + // that guarantee, so it is expected rather than anomalous on those. removeSafe(work, { label: "smoke:tui" }); } -// Tail of the child's output, for quoting in a diagnostic. Slicing an Ink -// stream can land mid-CSI-sequence, so drop through the first newline rather -// than lead with an escape-code fragment that mangles the line after it. -function outputTail(limit = 800) { - const tail = output.slice(-limit); - const nl = tail.indexOf("\n"); - return output.length > limit && nl !== -1 ? tail.slice(nl + 1) : tail; -} - -// `message` may be a string or a thunk. Callers that quote the child's output -// pass a thunk so it is rendered here — after the wait below, by which point -// `close` guarantees stdout/stderr have been fully drained. Building the string -// at call time instead can truncate the crash reason that matters most. -let finished = false; -function finish(code, message) { - if (finished) return; - finished = true; - cleanup(); - const text = typeof message === "function" ? message() : message; - if (code === 0) { - console.log(`smoke:tui OK — ${text}`); - } else { - console.error(`smoke:tui FAILED — ${text}`); - } - process.exit(code); -} - -function done(code, message) { - if (settled) return; - settled = true; - clearTimeout(timer); - if (childClosed) { - finish(code, message); - return; - } - // Wait for the child to actually exit before removing the work dir (#1801). - // The dir doubles as the TUI's HOME, so the Ink process is still writing into - // it when we signal; an rmSync racing those writes fails with ENOTEMPTY when - // a file lands after a directory has been read but before it is removed. - // - // Wait on `close`, not `exit`: a *spawn failure* emits `error` + `close` and - // never `exit`, so an `exit` wait would hang here until the give-up timer and - // then blame a SIGTERM the child never received. `close` also guarantees the - // stdio pipes are drained, which is what makes the thunked messages complete. - // - // This deliberately does NOT use lib/child-cleanup.mjs's `stopChild()` (which - // pack-and-verify.mjs uses against the same race): that helper resolves on the - // FIRST of exit/close, whereas this wait is entangled with the drain phase — - // it must keep waiting for `close` after `exit`, on a shorter re-armed - // deadline, so the quoted output is complete. Only `removeSafe()` is shared. - const forceKill = setTimeout(() => child.kill("SIGKILL"), EXIT_GRACE_MS); - - // Deadline for the wait. Re-armed shorter once the child is gone, since from - // that point we are only draining pipes. Each message describes what was - // actually being waited on — the timer can fire for a child that already - // exited and was never signalled, so it must not claim otherwise. - let deadline; - const armDeadline = (ms, warning) => { - clearTimeout(deadline); - deadline = setTimeout(() => { - console.warn(`smoke:tui — ${warning}`); - finish(code, message); - }, ms); - }; - armDeadline( - EXIT_GRACE_MS * 2, - `TUI did not close its output streams within ${EXIT_GRACE_MS * 2}ms; cleaning up anyway`, +// 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})`, ); - - const drainOnly = () => { - clearTimeout(forceKill); - armDeadline( - DRAIN_MS, - `TUI exited but held its output streams open for ${DRAIN_MS}ms (a descendant may have inherited them); cleaning up anyway`, - ); - }; - if (childExited) drainOnly(); - else child.once("exit", drainOnly); - - child.once("close", () => { - clearTimeout(forceKill); - clearTimeout(deadline); - finish(code, message); - }); - // Only signal a child that is still running: on the crash-before-render and - // spawn-failure paths there is nothing left to signal, and we are here purely - // to wait out the remaining `close`. - if (child.exitCode === null && child.signalCode === null && !child.killed) { - child.kill("SIGTERM"); - } } -function onData(chunk) { - output += chunk.toString(); - if (output.includes(RENDER_MARKER)) { - done(0, `rendered "${RENDER_MARKER}" panel from --catalog within timeout`); - } +if (result.code === 0) { + console.log(`smoke:tui OK — ${result.message}`); + process.exit(0); } - -child.stdout.on("data", onData); -child.stderr.on("data", onData); - -// Registered before done()'s own one-shot listener, so this always runs first — -// done() can trust `childClosed` even when called from inside a close handler. -child.on("close", () => { - childClosed = true; -}); - -child.on("exit", (code) => { - childExited = true; - if (settled) return; - // Exiting before the render marker appeared is a failure (crash on boot). - done( - 1, - () => - `TUI exited (code ${code}) before rendering "${RENDER_MARKER}"\n${outputTail()}`, - ); -}); - -child.on("error", (err) => { - done(1, `failed to spawn TUI: ${err.message}`); -}); - -const timer = setTimeout(() => { - done( - 1, - () => - `TUI did not render "${RENDER_MARKER}" within ${TIMEOUT_MS}ms\n${outputTail()}`, - ); -}, TIMEOUT_MS); +fail(result.message);