| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
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> |
||
|
Cliff Hall
|
36b48e61a2 |
feat(web): build dist on demand for prod --web + end-to-end CI smoke (#1486) (#1497)
* feat(web): build dist on demand for prod --web + end-to-end CI smoke (#1486) Prod `mcp-inspector --web` serves static assets from clients/web/dist, which only exists after a build. In a fresh checkout it was absent and the prod path served a broken page (ENOENT on index.html); the launcher smoke test only covered --help, so this path was unverified in CI. - ensure-web-build.ts: build dist on demand (vite build via build:client) when missing, or exit with an actionable error if the build can't run. - run-web.ts: invoke ensureWebBuild before startHonoServer in the prod path (--dev runs Vite directly and skips it). - scripts/smoke-web.mjs + smoke:web: start --web against built dist and assert GET / returns 200 with the injected __INSPECTOR_API_TOKEN__. Wired into validate:launcher and a new CI step. - Docs: launcher README + AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web): surface spawn error in build-on-demand; clarify smoke comment Address PR #1497 review: - runViteBuild now logs result.error.message (e.g. ENOENT when npm isn't on PATH) before returning a null status, so the spawn-failure cause surfaces instead of only the generic actionable error. The build seam gains a `log` param; added a test covering the ENOENT path. - Reword scripts/smoke-web.mjs header so it no longer implies the readiness wait covers a cold build-on-demand vite build (it's sized for a pre-built dist, which validate/CI guarantee). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
16c96d44c3 | ||
|
|
29e3ce95b8 | ||
|
|
742e82515a |
||
|
|
36b48e61a2 |