| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
cliffhall
|
58e53d5a92 |
fix: state every test-gate timeout instead of inheriting a library default
Three of the six Vitest projects ran on Vitest's own testTimeout: 5000 and five on its hookTimeout/teardownTimeout: 10000, and all 788 Testing Library waits in the web unit project ran on asyncUtilTimeout: 1000. None of those numbers was chosen here, and all of them were sized for an idle machine rather than for three or four concurrent worktree sessions each running a full local:gate on eight cores. #2292, #2278, #2250, #1942 and #1742 were each that class, diagnosed one site at a time. This is the complement of #1596, not a retreat from it: every fix that issue produced stands, and no race is hidden here. What changes is that a correct, deterministic test is no longer cut off by a budget nobody picked. - vitest.shared.mts exports frozen TIMEOUTS / INTEGRATION_TIMEOUTS; all six projects spread one of them, clients/launcher importing the shared module for the first time to do so. - Both web projects configure Testing Library's asyncUtilTimeout. The storybook one needs its own setup file, deliberately outside .storybook/ and free of setProjectAnnotations so the automatic preview-annotation provisioning (#1898) is untouched. - 28 per-suite restatements of the integration project's own budget are gone, so raising the shared value now moves every site under it. The handful that genuinely differ — cross-process lock contention, a full OAuth round trip — keep their budget as a named constant that says why. - AppRenderer's waitFor no longer asks for exactly its enclosing budget, which it could never win (#2292's shape); cli's e2e child timer drops below the project budget so the bound carrying the diagnostic is the one that fires. - The transition auto-settle's slack term is named RAF_SLACK_MS and raised; the term derived from HEADER_ANIM_MS is untouched. The two answer to different things and must not be scaled as one number. - The 19 Playwright budgets scattered across three web smokes become four named constants in scripts/lib/browser-timeouts.mjs. scripts/verify-test-timeouts.mjs resolves each project through Vitest itself and asserts the budgets, the asyncUtilTimeout sites, and that retry stays unset. It was observed red against the pre-change config: unit/tui/launcher at testTimeout 5000, five projects at hookTimeout 10000, both asyncUtilTimeout sites unmet. Closes #2323 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
0b64460097 |
chore(scripts): rename the local pre-push gate off ci, and guard it out of CI (#2146)
The root `ci` script was never what runs in GitHub CI, and the name worked
against us three ways: it collides with the built-in `npm ci` (which does
something else entirely and is one keystroke away), it invites the
cross-engine passes into a workflow by looking like the thing a workflow
should call, and it blurs "GitHub CI" and "the local gate" in every doc
that names it.
- Rename `ci` → `local:gate` and `ci:storybook` → `local:storybook`, with
no back-compat alias: `npm run ci` now fails with npm's missing-script
error, which prints the available scripts.
- Add `scripts/lib/workflow-gate.mjs` + its `test:scripts` suite, which
runs over the repo's real `.github/workflows/**` and fails on an
invocation of a `local:*` script, of a non-Chromium engine pass (derived
from SUPPORTED_BROWSERS), or of `smoke:web:engine`, or on `SMOKE_BROWSER`
set to anything but a literal `chromium`. It does not forbid
`npm run smoke`, `smoke:web:chromium` or `smoke:tui` — those belong in CI
and are there today. Comments are not scanned, so the workflow can still
explain itself. This is the durable half; the rest is prose.
- Document the two-tier model once, in the root README ("Two tiers: GitHub
CI and the local gate"), and point the other docs at it rather than
restating the split.
- Sweep the ~40 stale references across README, AGENTS.md,
.github/copilot-instructions.md, the client READMEs, the roadmap doc and
the script headers. AGENTS.md and copilot-instructions.md change
together, per the mirror rule.
No gate changes what it actually runs.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
66b3274fa6 |
docs(smoke): align the smoke headers with the pre-push gate wiring
The three headers still said the non-Chromium engines were on demand and that 'nothing runs it for you'. Both became false when Firefox moved into npm run ci; I updated the README and AGENTS.md for that change and did not carry it into the files themselves. Each header now names the three tiers separately rather than collapsing them into a CI/not-CI binary: GitHub CI is Chromium only, the local pre-push gate is Chromium and Firefox, and WebKit is on demand. That distinction is the whole point of the arrangement, so a header that blurs it is worse than one that is merely out of date. Addresses Copilot review 5026731741 on #2133. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
8a7ade7b08 |
ci: drop the Firefox smoke job; keep the engines as an on-demand tool
The job was trialled and did not earn its place. It was genuinely cheap — ~2 minutes, running in parallel with the 15-minute build job, so zero added wall-clock — but across a dozen runs Firefox never once disagreed with Chromium. It was paying runner time and carrying a real flake surface (playwright install --with-deps runs apt-get update, which fails whenever a third-party repo in the runner image breaks; that is what the one red run was) to re-confirm a result already in hand. Everything that made cross-engine checking possible stays: SMOKE_BROWSER, the shared headless-browser helper, the installer, and the tests. So "SMOKE_BROWSER=firefox npm run smoke:web:engine" is there for the moment it is worth running — before touching the sandbox, the CSP builder or the proxy page — without taxing every push to re-derive the same answer. The workflow is now byte-identical to v2/main again; the publish jobs go back to needing only build. Docs say what is true rather than what was intended, and AGENTS.md records the reasoning so the job is not re-added on the general argument that cross-engine coverage is good — that argument was already accepted, and the on-demand path is what serves it. Re-add it on evidence: a real cross-engine regression a gate would have caught. Also swept the now-stale "matrix" language out of the smokes and the resolver, including the empty-SMOKE_BROWSER rationale, which no longer leans on a matrix key that does not exist. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
fb9e146043 |
docs(smoke): drop the #2132 references now that the issue is closed
#2132 was closed without the investigation going anywhere, so every 'pending #2132' pointer in the tree was about to send a reader to a closed issue whose own diagnosis had already been retracted. The docs now say what is actually known, and nothing more: the two App smokes fail under Playwright's WebKit for unidentified reasons; it does not reproduce in real Safari; an isolated repro of the suspected mechanism did not reproduce it under Playwright's WebKit either; and chasing it further was judged not worth the effort. A reader hitting it locally is told to treat it as unexplained rather than as a defect. The matrix decision is unchanged and never depended on the cause — the engine fails, so it is not gated. No behavior change. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
822919f3ef |
docs(smoke): state the actual CI browser coverage, not the intended one
The three smoke headers and one AGENTS.md bullet still said CI runs all three engines. That was written before pointing the smokes at WebKit found #2132; the matrix was then narrowed to Firefox and these were not brought along, so they claimed a gate that does not exist. Each now says what CI actually enforces — Chromium in the build job's `npm run smoke`, Firefox in the Sandbox smokes matrix — and where WebKit stands. smoke-web-browser.mjs additionally notes that IT passes in WebKit; the two App smokes are the ones that do not, and a reader of this file should not infer otherwise from a bare '#2132' reference. The fail-fast: false rationale no longer cites a WebKit-vs-Firefox example it cannot currently produce, and says why the flag is still right for a one-entry matrix. Addresses Copilot review on #2133. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
ef4259a544 |
test(smoke): run the headless web smokes in Firefox and WebKit, not just Chromium (#2086)
Every browser-driven check in this repo launched Chromium and nothing else.
That is fine for most of the web client, whose behavior is React and Mantine.
It is not fine for the MCP Apps sandbox, which is built out of the primitives
that genuinely diverge between engines: a CSP <meta> injected as the first
<head> child of a srcdoc document, a nested sandboxed iframe, a
Permissions-Policy allow attribute, and postMessage origin discipline across
those two frames. Nothing else covers that surface — sandbox-csp.test.ts
asserts which policy STRING is built (environment-independent by
construction), and no Storybook story reaches the sandbox at all, since all
three App stories use a data: placeholder iframe and a mock bridge.
The engine is now a parameter:
SMOKE_BROWSER=webkit npm run smoke:web:app
SMOKE_BROWSER=firefox npm run smoke:web:engine # all three smokes
Unset means chromium, so `npm run ci` and `npm run smoke` are unchanged. An
unrecognized value is an error rather than a fallback — falling back would
report a green Chromium run under a job labelled "webkit", claiming coverage
that never ran — and a launch failure names the engine that failed and its own
`playwright install` invocation.
Selection, launching, and the fatal-vs-benign page-diagnostics split now live
in one shared scripts/lib/headless-browser.mjs. The three smokes had each
hand-rolled the diagnostics split and two had their own copy of loadChromium;
pack:verify goes through the same helper, pinned to Chromium explicitly since
it is a packaging check.
The npm scripts drop their `cd clients/web && npx playwright install chromium`
prefix for scripts/install-smoke-browser.mjs. The engine is a variable now, and
a ${VAR:-default} expansion does not expand under Windows' cmd.exe; npx is also
a .cmd shim there that a shell-free spawn cannot start, so this resolves the
Playwright CLI through resolveNodeBin — the #1939 problem solved the #1939 way.
CI gains a `Sandbox smokes (<engine>)` matrix job rather than matrixing `build`,
which also runs validate, the coverage gate, two verify gates and Storybook —
all engine-independent. Both publish jobs now need it.
WebKit is supported by the tooling but deliberately NOT in the CI matrix yet:
pointing the smokes at it immediately found #2132, a pre-existing Safari
incompatibility in the web client's SSE transport that strands the last message
on /api/mcp/events, so an MCP App never leaves "loading". That is a product bug
affecting real Safari users, with its own diagnosis and a verified fix, and it
is not a matrix change. Adding `webkit` here is a one-word diff once it lands.
An engine is either green in CI or absent from it; a continue-on-error job would
report coverage nobody is held to.
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
16c96d44c3 |
fix(smoke): await the server's exit before removing its catalog, and test it
Addresses both Copilot review comments on #1978. 1. `stop()` removed the catalog dir immediately after a bare `child.kill()`, which only *delivers* SIGTERM. That is precisely the #1801 race `child-cleanup.mjs:11-17` documents, and it says both halves are needed: `stopChild()` closes the race on the normal path, `removeSafe()` makes the residual case harmless. I had used only the second. `smoke-web-app.mjs` already used `stopChild` for its MCP test-server child, so the helper was inconsistent with its own sibling. `stop()` is now async — it awaits `stopChild` and then removes — and all three callers await it (`smoke-web.mjs`'s `fail()` became async to match, so every call site had to `await fail(...)` or execution would run past it instead of exiting). 2. The isolation contract had no test. The smokes exit the process right after teardown, so a regression reintroducing the shared catalog — or dropping cleanup — would leave them all green. Split the two pure pieces out (`createTempCatalog`, `buildWebServerEnv`) and cover them in a sibling `prod-web-server.test.mjs`: a unique dir per run, no pre-created catalog file, the dir is removable, and an inherited `MCP_CATALOG_PATH` (or `MCP_AUTO_OPEN_ENABLED`) cannot beat the explicit one — the spread-then-assign ordering that guard depends on. `startProdWebServer` itself stays untested by design; it spawns a real launcher, which is the smokes' job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
29e3ce95b8 |
chore(smoke): run the web smokes against a throwaway catalog
`startProdWebServer` spread `process.env` without setting `MCP_CATALOG_PATH`, so `smoke:web`, `smoke:web:browser`, and `smoke:web:app` all ran against the developer's real `~/.mcp-inspector/mcp.json` — unlike `smoke:cli` / `smoke:tui`, which have always driven a temp `--catalog`. That made the web smokes both destructive and non-deterministic. `smoke:web:app`'s deep link persists a `deep-link` server row, so a second run finds it already on disk, racing hydration: the deep-link effect misses it in `servers`, POSTs `addServer` anyway, and the backend answers 409. The app swallows that 409 by design (App.tsx:2842) so the smoke still passed, but it reported a spurious non-fatal console error that was really just residue from the previous run. Give the helper its own temp catalog dir and clean it up in `stop()` via `removeSafe`, so cleanup can never turn a passing smoke red. Isolation lives in the shared helper rather than per script, since every web smoke needs it. Only the catalog is redirected — other per-user state under `~/.mcp-inspector` stays shared, since isolating it means redirecting HOME wholesale, which would also move the npx and Playwright caches these smokes depend on. Verified: `smoke:web:app` twice in a row is now clean both times (previously the second run reported the 409), the real catalog's mtime is unchanged, and no temp dirs leak. CI never saw this — a fresh HOME per run made every CI run look like a first run. Closes #1977 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
Cliff Hall
|
742e82515a |
ci: add headless-browser boot smoke for the web client (#1615) (#1768)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
* ci: add headless-browser boot smoke for the web client (#1615) smoke:web only asserts GET / serves the SPA HTML with the injected token; it never runs the React app, so a Node built-in reaching the browser bundle (which crashes the app to a blank page at runtime, e.g. #1612) slips through. Add smoke:web:browser, which boots the same prod --web server and runs the bundle in headless Chromium (Playwright, already a clients/web devDependency), asserting the app renders its first frame (the "Add Servers" control) with no uncaught page errors — in particular no `Module "node:*" has been externalized`. This catches "Node code reached the browser bundle" as a class, not one import at a time. Wire it into the root smoke chain (and thus npm run ci / GitHub CI), and move the Playwright install/cache steps ahead of the smoke step so the new smoke reuses the cache. Update AGENTS.md and README. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * ci: address review on the headless-browser boot smoke - Don't fail on benign console.error (CDN font loads, React warnings) — the #1612 class arrives as an uncaught pageerror. console errors are now diagnostics, hard-failing only when they carry the externalization signature. - Broaden the externalization regex to non-prefixed built-ins ("fs" as well as "node:fs") and label the signature on both the pageerror and console channels. - Rename the port override to SMOKE_WEB_BROWSER_PORT so it doesn't collide with smoke:web's SMOKE_WEB_PORT. - Add a settle window (networkidle + 500ms) so a late-evaluating lazy chunk that throws is caught, and race the render against launcher death so a mid-load crash reports the real cause instead of a render timeout. - Print an actionable hint when Playwright/chromium can't launch (missing --with-deps system libs). - Extract the shared spawn/readiness boilerplate into scripts/lib/prod-web-server.mjs so smoke-web and smoke-web-browser can't drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * ci: fix playwright resolution in the browser smoke (CI failure) The bare `import("playwright")` resolved relative to the script's own directory (scripts/), not the cwd — so `cd clients/web` in the npm script never made it resolvable. It only passed locally because an ancestor node_modules happened to carry playwright; CI has none, so it failed with "Cannot find package 'playwright'". Resolve it explicitly from clients/web via createRequire, independent of cwd and ancestor dirs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * ci: address second review on the prod-web-server helper - waitForReady now records the last non-ok status and includes it in the timeout message, so a server that boots but persistently answers 500 (broken dist / injection failure) reports "server did not start … (last response: HTTP 500)" instead of a bare timeout that points at the wrong thing. Keeps polling on non-ok (a warming server may answer 503). - Add a child 'error' handler so a spawn-level failure surfaces as the smoke's FAILED line instead of an uncaught raw stack; readiness and the child-exit race both report it. - Drop the unused isExited/exitCode/child exports from the helper API. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * ci: reframe browser smoke around uncaught pageerror; fix stop() edge Third-review findings. 1. The `Module "…" has been externalized` string can't fire against a Vite 8 *prod* bundle — verified end-to-end against the installed vite@8.0.0: a node: import reaching the browser graph is a BUILD-time warning, and the shipped stub is a silent `module.exports = {}`. The real #1612 manifestation is the first *call* into that empty stub throwing a TypeError at module init (confirmed: injecting a used `node:fs` import fails the smoke with "readFileSync is not a function"; an unused one ships {} and is harmless). So the signature machinery was dead against the bundle it runs on — dropped it. The smoke now asserts exactly what it verifies: the prod bundle boots and paints with no uncaught page error. Reworded the script header, the AGENTS.md bullet, and the README row to match. Console errors stay diagnostic-only. 2. The helper's child 'error' handler set `exited = true`, but Node emits 'error' for a failed kill/send too (child still alive) — so `stop()` (guarded on `exited`) would skip the SIGTERM and orphan the launcher on the port. Track the error separately (`childError`); `bootFailed()` = exited || childError, while `stop()` still SIGTERMs a live child. Docs: fixed the stale AGENTS.md claim that `cd clients/web` makes `import("playwright")` resolve (createRequire does, cwd-independently; the cd is only for `npx playwright install`), and noted the shared scripts/lib/prod-web-server.mjs helper on both smoke bullets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * ci: also hard-fail the async half of the crash class (fourth review) page.on("pageerror") fires for uncaught SYNCHRONOUS exceptions only. The async twin of the #1612 shape — the same empty-stub TypeError reached through an await/.then(), or a failed dynamic import (this app lazy-loads chunks) — lands on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module`, which were diagnostic-only, so the smoke would have passed. Hard-fail console errors matching /^Uncaught\b|Failed to fetch dynamically imported module/; every other console error stays a printed non-fatal note. The prefixes are unambiguous (a font/CDN miss reads "Failed to load resource: net::ERR_…"; React warnings never start with "Uncaught"), so this can't reintroduce the flake the earlier round fixed. Verified: an unhandled rejection at module init now fails the smoke where it previously wouldn't. Also prod-scope the AGENTS.md note that the "externalized" string isn't a runtime message (under `npm run dev` Vite's stub is a Proxy that console.warns it), and document both the sync and async failure signals. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: reword smoke success line + README to two-channel wording (fifth review) After the async hard-failure signature, the assertion spans both the pageerror (sync) and console (unhandled rejection / failed dynamic import) channels. The OK console line and the README smoke row still said "page error"; match the two-channel wording already in the AGENTS.md bullet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * ci: correct the playwright-package-resolution hint (sixth review) The catch around requireFromWeb("playwright") advised `playwright install --with-deps chromium`, but that installs browser binaries, not the npm package — and the npm script already ran `playwright install` a step earlier. A failure there means the package isn't resolvable from clients/web, so the actionable fix is `npm install` at the repo root (the postinstall cascade installs clients/web's devDependencies). The chromium.launch() catch keeps its --with-deps advice (correct for the missing-system-libraries case). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
58e53d5a92 | ||
|
|
0b64460097 | ||
|
|
66b3274fa6 | ||
|
|
8a7ade7b08 | ||
|
|
fb9e146043 | ||
|
|
822919f3ef | ||
|
|
ef4259a544 | ||
|
|
16c96d44c3 | ||
|
|
29e3ce95b8 | ||
|
|
742e82515a |