chore(web): address Copilot review on the tab smoke
Three findings, all valid. **The read/get stages asserted an RPC status, not a render.** #2148 asks for the resource content and the prompt messages to *render*, and `data-read-status` / `data-get-status` flip the moment the call resolves — so the smoke stayed green with the panel deleted. `ResourcePreviewPanel` and `PromptMessagesDisplay` now carry `data-testid="resource-preview"` / `"prompt-messages"`, and each stage waits on the status **and** the rendered panel. Verified by breaking each testid: the smoke fails with `reading `events` did not render a preview — last observed: data-read-status="ok"`, which is the finding stated back exactly. **`smoke:web:tabs` followed ambient `SMOKE_BROWSER`**, so `SMOKE_BROWSER=firefox npm run ci` would have run the supposedly Chromium-only smoke under Firefox — contradicting both this PR's stated scope and the gated-tier invariant in `run-engine-smokes.test.mjs`. The npm script now pins `chromium` on **both** halves (install and launch) and the smoke resolves through `resolveRequestedBrowser(argv, env)`, where an explicit argument wins over the environment; a new case in that same test file pins it, including that the smoke stays out of `ENGINE_SMOKES`. **The AGENTS rules were not mirrored into `.github/copilot-instructions.md`.** Correct, and worse than an omission: that file's engine bullet asserts the headless web smokes are "engine-parameterized, not Chromium-only", which this PR made false. It now names `smoke:web:tabs` as the deliberate exception, and carries the genuinely review-citable new rule — assert `data-*`, never copy, and never stop at an RPC status where the ask was that something render. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
d710f90b90
commit
23088e9c69
9 changed files with 101 additions and 25 deletions
3
.github/copilot-instructions.md
vendored
3
.github/copilot-instructions.md
vendored
|
|
@ -106,8 +106,9 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.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 #<ISSUE_NUMBER>`.
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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(<PromptMessagesDisplay messages={messages} />);
|
||||
expect(screen.getByTestId("prompt-messages")).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -67,7 +67,10 @@ export function PromptMessagesDisplay({
|
|||
onClose,
|
||||
}: PromptMessagesDisplayProps) {
|
||||
return (
|
||||
<PanelStack>
|
||||
// `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.
|
||||
<PanelStack data-testid="prompt-messages">
|
||||
<HeaderRow>
|
||||
<HeaderLeft>
|
||||
{onClose && (
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
<ResourcePreviewPanel
|
||||
resource={textResource}
|
||||
contents={textContents}
|
||||
isSubscribed={false}
|
||||
onRefresh={vi.fn()}
|
||||
onSubscribe={vi.fn()}
|
||||
onUnsubscribe={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByTestId("resource-preview")).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -222,7 +222,12 @@ export function ResourcePreviewPanel({
|
|||
const sourceToggleable = isSourceToggleable(mimeType);
|
||||
|
||||
return (
|
||||
<PanelStack>
|
||||
// `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.
|
||||
<PanelStack data-testid="resource-preview">
|
||||
<HeaderRow>
|
||||
<HeaderLeft>
|
||||
{onClose && (
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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`,
|
||||
});
|
||||
};
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue