diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 6f0c77cb..9f25e588 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -106,8 +106,9 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo - **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. - **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. - **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. -- **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. +- **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, as is **`smoke:web:tabs`** (#2148), the connected-flow smoke for the Tools / Resources / Prompts tabs: those are ordinary React and Mantine rather than the sandbox, so it is deliberately **outside `ENGINE_SMOKES`** and pins `chromium` on **both** halves of its npm script — install and launch. Flag a supposedly single-engine script that reads ambient `SMOKE_BROWSER` (an ambient value must not be able to redirect a gated tier), or one whose install and launch engines can disagree. +- **A browser-driven smoke asserts `data-*` attributes, never visible copy** (#2148). Each screen exposes a readiness contract — `data-app-status` for Apps, and `data-tool-count` / `data-call-status` and their Resources / Prompts equivalents for the core tabs, tabulated in `clients/web/README.md`. Copy-based waits fail on the next rewording, and as an opaque timeout rather than a mismatch. Two things to flag: a new smoke matching on text or a heading, and an assertion that stops at an **RPC status** where the issue asked for something to *render* — `data-read-status="ok"` flips the moment `resources/read` resolves and stays true with the preview panel deleted, so the rendered-panel testid must be asserted alongside it. Each attribute name is pinned by a screen unit test so a rename fails there, not in a five-minute gate. Shared drive steps belong in `scripts/lib/` (the deep link lives in `deep-link-connect.mjs`): `parseDeepLink` *ignores* a link it cannot validate rather than reporting one, so a copied-and-drifted link fails as a silent connect timeout. - **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_. - **Lint has no warning tier.** Every `lint` script runs `--max-warnings 0`, so a warning fails `validate` exactly as an error does (#2085) — a `warn`-level `react-hooks/exhaustive-deps` finding otherwise let a stale-closure bug pass the pre-push gate and reach review. Flag a PR that silences a finding to make the gate pass (widening a `globalIgnores`, dropping a rule, or an inline disable with no justification comment); the fix is the defect, not the message. A rule meant to be enforced should be set to `error` rather than left at `warn` and carried by the flag. - **Every PR references an issue**, first body line `Closes #`. diff --git a/clients/web/README.md b/clients/web/README.md index 74114676..934b799c 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -109,9 +109,11 @@ Treat them as a public contract, for the same reason as the Apps ones below: | `data-resource-count` | on `resources-screen` | Entries from `resources/list`. | | `data-template-count` | on `resources-screen` | Entries from `resources/templates/list` — a **separate** call that can fail on its own, so it is reported separately. | | `data-read-status` | on `resources-screen` | `idle` → `pending` → `ok` / `error` for the current `resources/read`. | +| `data-testid="resource-preview"` | preview panel | The read resource's **rendered** contents. Assert this *and* `data-read-status`: the status flips as soon as the RPC resolves, so it alone stays true with the panel removed. | | `data-testid="prompts-screen"` | Prompts root | The element carrying the two attributes below. | | `data-prompt-count` | on `prompts-screen`| Entries from `prompts/list`. | | `data-get-status` | on `prompts-screen`| `idle` → `pending` → `ok` / `error` for the current `prompts/get`. | +| `data-testid="prompt-messages"` | messages panel | The fetched prompt's **rendered** messages — the `prompts/get` counterpart of `resource-preview`, and asserted alongside `data-get-status` for the same reason. | Why attributes rather than text: a smoke that waited on a label fails the next time the label is reworded, which is noise rather than signal — and it fails as diff --git a/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.test.tsx b/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.test.tsx index 23364e8b..d359b9ba 100644 --- a/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.test.tsx +++ b/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.test.tsx @@ -64,3 +64,14 @@ describe("PromptMessagesDisplay", () => { ).not.toBeInTheDocument(); }); }); + +// The `data-testid` the headless tab smoke waits on (#2148). It asserts the +// messages RENDERED, which `data-get-status="ok"` alone does not — that flips +// as soon as `prompts/get` resolves. Pinned here so a rename fails loudly in a +// unit test rather than as an opaque timeout in a five-minute gate. +describe("automation contract (#2148)", () => { + it("exposes a stable data-testid on the panel", () => { + renderWithMantine(); + expect(screen.getByTestId("prompt-messages")).toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.tsx b/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.tsx index 3947d6d8..9bff4a72 100644 --- a/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.tsx +++ b/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.tsx @@ -67,7 +67,10 @@ export function PromptMessagesDisplay({ onClose, }: PromptMessagesDisplayProps) { return ( - + // `data-testid` so a driver can wait on the messages having actually + // RENDERED, not merely on `prompts/get` having returned (#2148) — see the + // ResourcePreviewPanel counterpart for why the RPC status is not enough. + {onClose && ( diff --git a/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.test.tsx b/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.test.tsx index 35cafff8..b8283ed9 100644 --- a/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.test.tsx +++ b/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.test.tsx @@ -566,3 +566,22 @@ describe("ResourcePreviewPanel", () => { }); }); }); + +// The `data-testid` the headless tab smoke waits on (#2148) — see the +// PromptMessagesDisplay counterpart: `data-read-status="ok"` flips as soon as +// `resources/read` resolves, so only this proves the preview rendered. +describe("automation contract (#2148)", () => { + it("exposes a stable data-testid on the panel", () => { + renderWithMantine( + , + ); + expect(screen.getByTestId("resource-preview")).toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx b/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx index 39630e89..5988ed0d 100644 --- a/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx +++ b/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx @@ -222,7 +222,12 @@ export function ResourcePreviewPanel({ const sourceToggleable = isSourceToggleable(mimeType); return ( - + // `data-testid` so a driver can wait on the preview having actually + // RENDERED, not merely on `resources/read` having returned (#2148). The + // read status flips as soon as the RPC resolves, so a smoke stopping there + // would stay green with this panel removed. A `data-*` attribute is not + // part of a component's typed props, so it is passed at the call site. + {onClose && ( diff --git a/package.json b/package.json index 1d04a8fc..b52516b0 100644 --- a/package.json +++ b/package.json @@ -73,7 +73,7 @@ "smoke:web:browser": "node scripts/install-smoke-browser.mjs && node scripts/smoke-web-browser.mjs", "smoke:web:app": "node scripts/install-smoke-browser.mjs && node scripts/smoke-web-app.mjs", "smoke:web:elicit": "node scripts/install-smoke-browser.mjs && node scripts/smoke-web-elicitation.mjs", - "smoke:web:tabs": "node scripts/install-smoke-browser.mjs && node scripts/smoke-web-tabs.mjs", + "smoke:web:tabs": "node scripts/install-smoke-browser.mjs chromium && node scripts/smoke-web-tabs.mjs chromium", "smoke:web:engine": "node scripts/run-engine-smokes.mjs", "smoke:web:chromium": "node scripts/run-engine-smokes.mjs chromium", "smoke:web:firefox": "node scripts/run-engine-smokes.mjs firefox", diff --git a/scripts/run-engine-smokes.test.mjs b/scripts/run-engine-smokes.test.mjs index b0cebe58..cd28009e 100644 --- a/scripts/run-engine-smokes.test.mjs +++ b/scripts/run-engine-smokes.test.mjs @@ -90,6 +90,23 @@ describe("every engine tier consumes ENGINE_SMOKES", () => { } }); + it("the Chromium-only tab smoke pins its engine on BOTH halves", () => { + // `smoke:web:tabs` is not engine-sensitive and deliberately stays out of + // ENGINE_SMOKES — but "Chromium-only" is a claim the script has to keep. + // Without the literal engine it followed ambient `SMOKE_BROWSER`, so + // `SMOKE_BROWSER=firefox npm run ci` would have run it under Firefox + // (Copilot, #2148). Both halves are pinned because they must AGREE: an + // install of one engine and a launch of another surfaces as a confusing + // "Executable doesn't exist" rather than as the wiring error it is. + assert.match( + scripts["smoke:web:tabs"], + /install-smoke-browser\.mjs chromium .*smoke-web-tabs\.mjs chromium$/, + ); + // And it must not be in ENGINE_SMOKES, or the Firefox tier would pick it + // up regardless of what its own script pins. + assert.ok(!ENGINE_SMOKES.includes("smoke-web-tabs.mjs")); + }); + it("each gated tier names its engine explicitly rather than reading the env", () => { // An ambient SMOKE_BROWSER must not be able to redirect a gate: without the // literal engine, `SMOKE_BROWSER=firefox npm run ci` would run Firefox twice diff --git a/scripts/smoke-web-tabs.mjs b/scripts/smoke-web-tabs.mjs index 7ad6a94b..448a5b70 100644 --- a/scripts/smoke-web-tabs.mjs +++ b/scripts/smoke-web-tabs.mjs @@ -30,12 +30,17 @@ * only summarize it, so a result panel that renders text alone looks * correct while dropping the data. * 2. **Resources** — the list renders, a resource is read, and the preview - * populates. Templates are asserted present (the RFC 6570 fixture, #1919), - * which is what proves `resources/templates/list` reached the screen — - * that list is fetched by a *different* call than `resources/list` and can - * fail on its own. - * 3. **Prompts** — the list renders and `simple_prompt` is fetched (selecting - * an argument-less prompt auto-fetches), and the messages render. + * panel renders its contents. Templates are asserted present (the RFC 6570 + * fixture, #1919), which is what proves `resources/templates/list` reached + * the screen — that list is fetched by a *different* call than + * `resources/list` and can fail on its own. + * 3. **Prompts** — the list renders, `simple_prompt` is fetched (selecting an + * argument-less prompt auto-fetches), and the messages panel renders. + * + * The read/get stages assert the RPC status **and** the rendered panel, not the + * status alone. Each `data-*-status` flips the moment its call resolves, so a + * smoke stopping there would stay green with the panel deleted — which is the + * opposite of what a connected-flow smoke is for. * * Deliberately **one script over one browser launch** rather than one per tab: * launching the browser and booting the server dominate the cost, and @@ -79,11 +84,8 @@ import { resolve } from "node:path"; import { startProdWebServer } from "./lib/prod-web-server.mjs"; import { stopChild } from "./lib/child-cleanup.mjs"; -import { - attachPageDiagnostics, - loadBrowser, - resolveBrowserName, -} from "./lib/headless-browser.mjs"; +import { attachPageDiagnostics, loadBrowser } from "./lib/headless-browser.mjs"; +import { resolveRequestedBrowser } from "./install-smoke-browser.mjs"; import { buildConnectDeepLink, connectViaDeepLink, @@ -92,14 +94,20 @@ import { startMcpAppServer } from "./lib/mcp-app-flow.mjs"; const repoRoot = resolve(import.meta.dirname, ".."); -// Resolved before anything starts, so an unsupported SMOKE_BROWSER fails -// immediately rather than after a web server and an MCP server are up. Every -// message carries the engine so a failure names which run it came from — even -// though this smoke is Chromium-only in every gate, it can still be pointed -// elsewhere by hand. +// The engine comes from an explicit ARGUMENT first, and only then from +// `SMOKE_BROWSER`. That precedence is the load-bearing part: `smoke:web:tabs` +// passes `chromium` to both this and the installer, so an ambient +// `SMOKE_BROWSER=firefox npm run ci` cannot silently redirect a gated tier that +// claims to be Chromium-only — the same invariant `run-engine-smokes.test.mjs` +// pins for the engine tiers (Copilot, #2148). Running another engine by hand is +// still one argument away: `node scripts/smoke-web-tabs.mjs firefox`. +// +// Resolved before anything starts, so an unsupported value fails immediately +// rather than after a web server and an MCP server are up. Every message +// carries the engine, so a failure names which run it came from. let BROWSER; try { - BROWSER = resolveBrowserName(); + BROWSER = resolveRequestedBrowser(process.argv.slice(2), process.env); } catch (err) { console.error( `smoke:web:tabs FAILED — ${err instanceof Error ? err.message : String(err)}`, @@ -279,10 +287,16 @@ try { .getByRole("button", { name: RESOURCE, exact: true }) .click({ timeout: 30_000 }); await waitForStage(page, { - selector: '[data-testid="resources-screen"][data-read-status="ok"]', + // Both the RPC status AND the rendered preview. The status flips the + // moment `resources/read` resolves, so stopping there would stay green + // with the preview panel removed entirely (Copilot, #2148) — and #2148 + // asks for the content to *render*, not merely for the call to return. + selector: + '[data-testid="resources-screen"][data-read-status="ok"]' + + ' [data-testid="resource-preview"]', screen: '[data-testid="resources-screen"]', attrs: ["data-read-status"], - what: `reading \`${RESOURCE}\` did not succeed`, + what: `reading \`${RESOURCE}\` did not render a preview`, }); // ── Prompts ──────────────────────────────────────────────────────────── @@ -299,10 +313,14 @@ try { .getByRole("button", { name: new RegExp(`^${PROMPT}`) }) .click({ timeout: 30_000 }); await waitForStage(page, { - selector: '[data-testid="prompts-screen"][data-get-status="ok"]', + // As with the resource preview: the messages panel must have rendered, + // not merely `prompts/get` returned (Copilot, #2148). + selector: + '[data-testid="prompts-screen"][data-get-status="ok"]' + + ' [data-testid="prompt-messages"]', screen: '[data-testid="prompts-screen"]', attrs: ["data-get-status"], - what: `getting \`${PROMPT}\` did not succeed`, + what: `getting \`${PROMPT}\` did not render its messages`, }); };