Skip to content

Commit bd7297d

Browse files
committed
fix(scripts): address Copilot review round 1 on #2147
- 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>
1 parent b6baa30 commit bd7297d

6 files changed

Lines changed: 57 additions & 16 deletions

File tree

‎.github/copilot-instructions.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.ts`) custo
105105
- `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → Storybook; the Firefox step is local-only and not mirrored in GitHub CI). `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating.
106106
- **A dependency bump must land in every install that declares it.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this — it derives its candidates from what each `tsc` program actually resolves (`tsc --listFilesOnly`, keeping packages that reach one program from two installs), so a package reached only through another package's `.d.ts` counts too (#1965) — so a PR bumping a package the shared sources pull in should update the root **and every client that already lists it** — not every client unconditionally, since a package absent from an install can't skew and adding it there would be a spurious dependency.
107107
- **A test or smoke must not touch real user state.** The web smokes run against a throwaway catalog via the shared `scripts/lib/prod-web-server.mjs` helper, never the developer's `~/.mcp-inspector/mcp.json` (#1977); the cli/tui smokes drive a temp `--catalog`; `pack:verify`'s `--web` child sets its own `MCP_CATALOG_PATH` for the same reason (#2003 — its App deep link persists a server row). Anything that boots the web backend and then *navigates* it needs that isolation, not just the scripts named `smoke:*`. A new smoke spawning its own server, or teardown that removes a work dir without first awaiting `stopChild` (the #1801 race — `child-cleanup.mjs` exports both halves and both are required), should be flagged.
108+
- **A smoke must assert the thing survives, not that it started.** `smoke:tui` runs the Ink TUI under a pseudoterminal (`scripts/lib/pty.mjs`) and requires it to still be running `SMOKE_TUI_SURVIVE_MS` after its first frame (`scripts/lib/render-smoke.mjs`) — because it previously settled OK on first paint while the TUI was exiting 1 ~40ms later with `Raw mode is not supported`, and won that race on every machine (#2147). Flag anything that collapses the survival wait back into "resolve on the marker", and any new smoke whose only assertion is that a process printed something once. A smoke walks only its own happy path, so the branch that matters — started, then died — belongs in a `scripts/lib/*.test.mjs` driven against a stub (as with `announced-child.mjs` #2000 and `ensure-test-servers.mjs` #2111), not in the smoke.
108109
- **The headless web smokes are engine-parameterized, not Chromium-only.** `smoke:web:browser` / `smoke:web:app` / `smoke:web:elicit` take their engine from `SMOKE_BROWSER` (chromium — the default — firefox, webkit). **GitHub CI runs Chromium only; Firefox runs in the local pre-push gate** via `smoke:web:firefox` (#2086 — a CI job was trialled and removed for never disagreeing with Chromium across a dozen runs, and moved into `npm run ci` instead). They exist because the MCP Apps sandbox is built out of the primitives that diverge between engines: `srcdoc` CSP inheritance, nested sandboxed iframes, `Permissions-Policy`, cross-frame `postMessage`. Nothing else covers that — `sandbox-csp.test.ts` asserts a policy *string*, and no Storybook story reaches the sandbox at all. Flag a new browser-driven script that launches Playwright itself instead of going through `scripts/lib/headless-browser.mjs`, an unrecognized-engine path that falls back to Chromium rather than failing (it would claim coverage that never ran), or a launch-failure message naming the wrong engine. `pack:verify` is pinned to Chromium on purpose — it is a packaging check.
109110

110111
- **Build output is never a gate target.** Lint, format, and typecheck read first-party source only; everything a build writes (`clients/*/build`, `clients/web/dist`, `storybook-static`, `coverage`, `test-servers/build`, `core/**/{build,dist}`, `*.tsbuildinfo`) stays out via each scope's `globalIgnores`, `format` globs, and tsconfig `include`. Gating generated code reports defects in vendored third-party source that nobody can fix, and a rule promotion turns that warning into a `validate` failure (#2043). Flag a PR that adds a build location without ignoring it in the same change, that widens an ignore to silence a finding in first-party code, or that adds a build directory to a tsconfig `include` to make a generated `.d.ts` resolve. Note the coverage guards don't catch this — they assert source is _covered_, not that output is _excluded_.

‎AGENTS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -973,7 +973,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab
973973
- `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.
974974

975975
- **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.
976-
- `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 <temp>` **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.
976+
- `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 <temp>` **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 rebuild `test-servers/build` on **every run** — once per process, whether or not it already exists (#2111): presence is not freshness, and a smoke driving a stale fixture reports a product failure rather than a staleness one.
977977

978978
**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.
979979

‎clients/launcher/README.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,9 @@ through the built launcher artifact (beyond the `--help` checks in
9999
local-only (self-skips under `CI`), which is precisely where a false green
100100
goes unnoticed.
101101

102-
Both build `test-servers/build` on demand if it is missing.
102+
Both rebuild `test-servers/build` on **every run** — once per process, whether
103+
or not it already exists (#2111). Presence is not freshness: a smoke driving a
104+
stale fixture reports a product failure rather than a staleness one.
103105

104106
## Development
105107

‎scripts/lib/pty.mjs‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,19 @@
3131

3232
import { spawnSync } from "node:child_process";
3333

34-
/** Platforms whose `script` takes the BSD argv form. */
35-
const BSD_PLATFORMS = new Set(["darwin", "freebsd", "openbsd", "netbsd"]);
34+
/**
35+
* Platforms whose `script` takes the BSD argv form (`script -q FILE cmd args…`).
36+
*
37+
* Deliberately just these two. darwin is verified empirically here; freebsd is
38+
* where darwin's implementation comes from and shares its usage. **netbsd and
39+
* openbsd are excluded on purpose** — the BSDs do not agree on this, NetBSD's
40+
* `script` takes the command through `-c` — and nobody has run either here. A
41+
* guessed invocation is worse than none: it fails at `script` startup, which
42+
* this smoke would then report as a TUI crash. They fall through to `null` and
43+
* the smoke skips, saying so. Adding a `-c` flavor for them is a small change
44+
* for whoever can actually verify it on the box.
45+
*/
46+
const BSD_PLATFORMS = new Set(["darwin", "freebsd"]);
3647

3748
/**
3849
* Quote one argument for a POSIX shell command string.
@@ -70,8 +81,9 @@ export function scriptFlavorFor({ platform, versionOutput = "" }) {
7081
// own usage error in the captured output, which is diagnosable — silently
7182
// skipping is the outcome this issue exists to stop rewarding.
7283
if (platform === "linux") return "util-linux";
73-
// win32 and anything else: no `script(1)`. `node-pty` is the portable answer
74-
// if this smoke ever has to run there.
84+
// win32 (no `script(1)` at all), and any platform whose invocation nobody has
85+
// verified — aix, sunos, netbsd, openbsd. The caller skips loudly rather than
86+
// guessing. `node-pty` is the portable answer if this ever has to run there.
7587
return null;
7688
}
7789

‎scripts/lib/pty.test.mjs‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -40,18 +40,26 @@ test("scriptFlavorFor falls back to the platform when the probe says nothing", (
4040
// BSD `script` has no --version, so failing the probe is itself the signal.
4141
assert.equal(scriptFlavorFor({ platform: "darwin" }), "bsd");
4242
assert.equal(scriptFlavorFor({ platform: "freebsd" }), "bsd");
43+
// NOT every BSD: NetBSD takes the command through `-c`, and nobody has run
44+
// either it or OpenBSD here. Guessing builds an invocation that fails at
45+
// `script` startup, which this smoke would then report as a TUI crash.
46+
assert.equal(scriptFlavorFor({ platform: "netbsd" }), null);
47+
assert.equal(scriptFlavorFor({ platform: "openbsd" }), null);
4348
// An unidentifiable `script` on linux is overwhelmingly util-linux. Guess it:
4449
// a wrong guess fails loudly with `script`'s own usage error in the captured
4550
// output, whereas skipping is the silence this issue exists to stop.
4651
assert.equal(scriptFlavorFor({ platform: "linux" }), "util-linux");
4752
});
4853

49-
test("scriptFlavorFor returns null where there is no script(1)", () => {
54+
test("scriptFlavorFor returns null where there is no verified invocation", () => {
5055
assert.equal(scriptFlavorFor({ platform: "win32" }), null);
5156
assert.equal(
5257
scriptFlavorFor({ platform: "win32", versionOutput: "not a thing" }),
5358
null,
5459
);
60+
// Unsupported is the default, not a win32 special case.
61+
assert.equal(scriptFlavorFor({ platform: "aix" }), null);
62+
assert.equal(scriptFlavorFor({ platform: "sunos" }), null);
5563
});
5664

5765
test("ptyCommand builds the BSD argv form", () => {
@@ -146,15 +154,25 @@ test("resolvePtyWrapper wraps with the resolved flavor, or reports none", () =>
146154
assert.equal(resolvePtyWrapper({ platform: "win32", probe: () => "" }), null);
147155
});
148156

149-
test("resolvePtyWrapper resolves on this machine unless it is Windows", () => {
150-
// Not a tautology: it runs the real probe, so a `script` that has vanished
151-
// from PATH, or a platform nobody has tried, surfaces here rather than as an
152-
// opaque smoke failure.
157+
// The platforms this repo claims a verified `script(1)` invocation for. Any
158+
// other — win32, aix, sunos, netbsd, openbsd — resolves to null BY DESIGN and
159+
// makes `smoke:tui` skip, so asserting "supported unless Windows" would fail
160+
// `test:scripts` on a machine where the smoke is behaving exactly as intended.
161+
const SUPPORTED_PLATFORMS = new Set(["darwin", "freebsd", "linux"]);
162+
163+
test("resolvePtyWrapper resolves on the platforms we claim to support", () => {
164+
// Not a tautology: it runs the real probe against the real machine, so a
165+
// `script` that has vanished from PATH surfaces here rather than as an opaque
166+
// smoke failure.
153167
const wrapper = resolvePtyWrapper();
154-
if (process.platform === "win32") {
155-
assert.equal(wrapper, null);
156-
} else {
168+
if (SUPPORTED_PLATFORMS.has(process.platform)) {
157169
assert.ok(wrapper, `no PTY wrapper resolved on ${process.platform}`);
158170
assert.equal(wrapper.wrap({ command: "node" }).command, "script");
171+
} else {
172+
assert.equal(
173+
wrapper,
174+
null,
175+
`${process.platform} is not in SUPPORTED_PLATFORMS but resolved a wrapper`,
176+
);
159177
}
160178
});

‎scripts/lib/render-smoke.mjs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,16 @@ export function outputTail(output, limit = 800) {
5656
* teardown handle on every failure path.
5757
* @param {(message: string) => void} [opts.warn]
5858
* @returns {Promise<{ code: 0 | 1, message: string, output: string }>}
59-
* Resolves only after the child's streams have closed, so `output` is
60-
* complete and the caller may safely remove a directory the child was using.
59+
* On the normal path, resolves only after the child's streams have `close`d —
60+
* so `output` is complete and the caller may remove a directory the child was
61+
* using without re-entering the #1801 ENOTEMPTY race.
62+
*
63+
* That is **bounded, not guaranteed**. Both drain deadlines below resolve
64+
* *without* having observed `close`, after warning: the child may still hold
65+
* the dir, and `output` may be truncated mid-line. Cleanup at the call site
66+
* must therefore be best-effort (`removeSafe`, not `rmSync`) — the worst case
67+
* is a warning and a leaked temp dir, never a red smoke. Do not write a
68+
* caller that depends on the close having happened.
6169
*/
6270
export function runRenderSmoke({
6371
command,

0 commit comments

Comments
 (0)