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>
This commit is contained in:
cliffhall 2026-08-26 21:20:52 -04:00
commit bd7297da88

View file

@ -973,7 +973,7 @@ 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 <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.
- `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.
**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.