From a1f40a42227facdccdee5e2f1c9fc2b8cde83e04 Mon Sep 17 00:00:00 2001 From: Cliff Hall Date: Mon, 15 Jun 2026 11:37:06 -0400 Subject: [PATCH] fix(web): remove config-load warning filter, fix root cause (#1491) (#1494) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `UNRESOLVED_IMPORT` warnings that `quiet-config-warnings.mjs` suppressed no longer occur, so the filter (and the build-time `onwarn` suppressions for the same three deps) are removed. Root cause: Vite's config bundler (Rolldown) statically walks the node-only dev backend (`core/mcp/remote/node/server.ts`) when it loads `vite.config.ts`, reaching `chokidar`, `atomically`, and `@napi-rs/keyring`. `core/` has no node_modules of its own, so at config-bundle time those deps resolve from `core/`'s parent chain — the repo-root `node_modules`. Before #1452 the deps lived only under `clients/web/node_modules` (a sibling, off the resolution chain), so Rolldown emitted `UNRESOLVED_IMPORT`. #1452 declared them in the repo-root `package.json` (for the CLI/TUI/launcher, which consume core/'s node code), installing them at the repo root and making them resolvable. Verified by hiding the root copy: the warning returns; restore it and the dev/build startup is clean. A dynamic import of the backend does not help — Rolldown follows dynamic imports during config bundling too. The fix is the deps being resolvable, so the suppression is just removed and the invariant is documented in `vite.config.ts`. - Remove `clients/web/server/quiet-config-warnings.mjs` + `.d.mts` and its test - Restore the `dev` script to plain `vite` (drop the `--import` hook) - Remove the now-dead `build.rollupOptions.onwarn` UNRESOLVED suppressions (never fire: the Hono plugin is `apply: 'serve'`, out of the build graph) - Document why config-bundle resolution succeeds in `vite.config.ts` - Update AGENTS.md project tree Co-authored-by: Claude Opus 4.8 (1M context) --- AGENTS.md | 4 +- clients/web/package.json | 2 +- .../web/server/quiet-config-warnings.d.mts | 8 --- clients/web/server/quiet-config-warnings.mjs | 66 ------------------- .../test/server/quiet-config-warnings.test.ts | 54 --------------- clients/web/vite.config.ts | 58 ++++------------ 6 files changed, 14 insertions(+), 178 deletions(-) delete mode 100644 clients/web/server/quiet-config-warnings.d.mts delete mode 100644 clients/web/server/quiet-config-warnings.mjs delete mode 100644 clients/web/src/test/server/quiet-config-warnings.test.ts diff --git a/AGENTS.md b/AGENTS.md index ba65d2b8..13a72797 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,9 +16,7 @@ inspector/ │ │ │ # web-server-config.ts (env parsing + initial-config payload + banner), │ │ │ # sandbox-controller.ts (MCP Apps sandbox HTTP server), │ │ │ # inject-auth-token.ts (embeds the API token into served index.html), -│ │ │ # vite-base-config.ts (shared optimizeDeps exclusions), -│ │ │ # quiet-config-warnings.mjs (--import hook for `npm run dev`: drops benign -│ │ │ # node-only UNRESOLVED_IMPORT warnings Rolldown prints at config load) +│ │ │ # vite-base-config.ts (shared optimizeDeps exclusions) │ │ └── static/ # sandbox_proxy.html (served by sandbox-controller for MCP Apps tab) │ ├── cli/ # CLI client (tsup bundle, @inspector/core alias) │ ├── tui/ # TUI client (Ink + React, tsup bundle) diff --git a/clients/web/package.json b/clients/web/package.json index d3f6c459..f81ff2d9 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -11,7 +11,7 @@ ".": "./build/index.js" }, "scripts": { - "dev": "node --import ./server/quiet-config-warnings.mjs ./node_modules/vite/bin/vite.js", + "dev": "vite", "build": "tsc -b && vite build && npm run build:runner", "build:runner": "tsup --config tsup.runner.config.ts", "lint": "eslint .", diff --git a/clients/web/server/quiet-config-warnings.d.mts b/clients/web/server/quiet-config-warnings.d.mts deleted file mode 100644 index 2ba354db..00000000 --- a/clients/web/server/quiet-config-warnings.d.mts +++ /dev/null @@ -1,8 +0,0 @@ -/** Type declarations for the dev-only `quiet-config-warnings.mjs` hook so its - * pure helpers can be imported (and unit-tested) from TypeScript. */ - -/** (package-quote, repo-relative-source-path) pairs of benign warnings to drop. */ -export declare const BENIGN_PAIRS: ReadonlyArray; - -/** True when `text` is a benign node-only UNRESOLVED_IMPORT warning. */ -export declare function isBenignWarning(text: string): boolean; diff --git a/clients/web/server/quiet-config-warnings.mjs b/clients/web/server/quiet-config-warnings.mjs deleted file mode 100644 index 871360bb..00000000 --- a/clients/web/server/quiet-config-warnings.mjs +++ /dev/null @@ -1,66 +0,0 @@ -/** - * Drops the benign "UNRESOLVED_IMPORT" warnings Vite's config bundler (Rolldown) - * prints at `vite dev` startup for node-only backend deps that the Hono plugin - * pulls in (`chokidar`, `atomically`, `@napi-rs/keyring`). - * - * Why a stream filter and not a Vite hook: the dev backend - * (`core/mcp/remote/node/server.ts`) is statically imported by the Hono plugin, - * so loading vite.config.ts bundles it and Rolldown walks its node-only deps. - * Those deps are correctly externalized (the browser never loads them; Node - * resolves them at runtime), but Rolldown still warns. The warnings are printed - * directly to the process streams during config loading — before any Vite - * logger exists — so `customLogger`, `logLevel`, and `rollupOptions.onwarn` all - * miss them. Each warning (header + code frame) is emitted as a single write, - * so we drop any write whose text carries the unresolved-import signature for - * one of these known-benign (package, source-file) pairs. Installed via - * `node --import` ahead of the Vite CLI; only affects the dev script. - * - * This matcher is tied to Rolldown's current warning format (the `'pkg'` quote - * style and the repo-relative source path it prints). If these warnings ever - * reappear, check whether Rolldown changed its message format. The failure mode - * is safe: a format change just makes the filter a no-op (the warnings return), - * it never drops anything else. - */ - -// (package, source-file) pairs. The file is a repo-relative path fragment — not -// just a basename — so an unrelated future warning that happens to mention the -// same dep and a same-named file isn't silently dropped. -export const BENIGN_PAIRS = [ - ["'chokidar'", "core/mcp/remote/node/server.ts"], - ["'atomically'", "core/storage/store-io.ts"], - ["'@napi-rs/keyring'", "core/auth/node/secret-store.ts"], -]; - -export function isBenignWarning(text) { - if (!text.includes("UNRESOLVED_IMPORT")) return false; - return BENIGN_PAIRS.some( - ([dep, file]) => text.includes(dep) && text.includes(file), - ); -} - -function patch(stream) { - const original = stream.write.bind(stream); - stream.write = (chunk, encoding, callback) => { - const text = - typeof chunk === "string" - ? chunk - : Buffer.isBuffer(chunk) - ? chunk.toString("utf8") - : ""; - if (text && isBenignWarning(text)) { - // Pretend the write succeeded so callers/promises don't stall. - if (typeof encoding === "function") encoding(); - else if (typeof callback === "function") callback(); - return true; - } - return original(chunk, encoding, callback); - }; -} - -// Guard so importing this module (e.g. from a unit test for isBenignWarning) -// doesn't monkey-patch the test runner's streams. Under `npm run dev` VITEST is -// unset, so the patch installs as intended. -if (!process.env.VITEST) { - patch(process.stdout); - patch(process.stderr); -} diff --git a/clients/web/src/test/server/quiet-config-warnings.test.ts b/clients/web/src/test/server/quiet-config-warnings.test.ts deleted file mode 100644 index b325cde0..00000000 --- a/clients/web/src/test/server/quiet-config-warnings.test.ts +++ /dev/null @@ -1,54 +0,0 @@ -import { describe, it, expect } from "vitest"; -import { - isBenignWarning, - BENIGN_PAIRS, -} from "../../../server/quiet-config-warnings.mjs"; - -// A representative Rolldown config-load warning block (header + code frame), as -// captured from `vite dev` startup. -function warning(dep: string, file: string): string { - return ( - `${file} (8:36) [UNRESOLVED_IMPORT] Warning: Could not resolve ${dep} in ${file}\n` + - ` ╭─[ ${file}:8:37 ]\n` + - ` 8 │ import { readFile } from ${dep};\n` + - ` ╰─ Module not found, treating it as an external dependency\n` - ); -} - -describe("quiet-config-warnings / isBenignWarning", () => { - it("matches each benign node-only dep warning", () => { - for (const [dep, file] of BENIGN_PAIRS) { - expect(isBenignWarning(warning(dep, file))).toBe(true); - } - }); - - it("ignores non-UNRESOLVED output", () => { - expect(isBenignWarning("VITE v8.0.0 ready in 603 ms")).toBe(false); - expect( - isBenignWarning("Sandbox (MCP Apps): http://localhost:61146/sandbox"), - ).toBe(false); - }); - - it("does not drop an UNRESOLVED_IMPORT for a different (real) dependency", () => { - expect( - isBenignWarning( - "src/App.tsx (1:0) [UNRESOLVED_IMPORT] Warning: Could not resolve 'some-real-pkg' in src/App.tsx", - ), - ).toBe(false); - }); - - it("requires the matching source path, not just the package name", () => { - // Same dep, but in an unrelated file → not silenced (guards over-matching). - expect( - isBenignWarning( - "src/elsewhere.ts (1:0) [UNRESOLVED_IMPORT] Warning: Could not resolve 'chokidar' in src/elsewhere.ts", - ), - ).toBe(false); - }); - - it("uses repo-relative path fragments (not bare basenames)", () => { - for (const [, file] of BENIGN_PAIRS) { - expect(file).toContain("/"); - } - }); -}); diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index 511b6ed4..1c82e0db 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -32,58 +32,24 @@ export default defineConfig({ // `honoMiddlewarePlugin` is gated by `apply: 'serve'` so it only attaches // during `vite dev` / `vite preview` — vitest projects share this config // but never invoke `configureServer`, so the plugin stays inert there. + // + // The plugin statically imports the node-only dev backend + // (`core/mcp/remote/node/server.ts`), so Vite's config bundler (Rolldown) + // walks that chain when it loads this file and reaches node-only deps + // (`chokidar`, `atomically`, `@napi-rs/keyring`). Those resolve cleanly at + // config-bundle time because they're declared in the repo-root + // `package.json` and installed into the repo-root `node_modules`, which sits + // on `core/`'s module-resolution chain (core/ has no node_modules of its + // own). Keep them in the root manifest: drop one and Rolldown can no longer + // resolve it from core/, reviving the benign `UNRESOLVED_IMPORT` warnings + // that #1491 eliminated at the source (by removing the old stream filter and + // build-time onwarn suppressions rather than re-hiding the symptom). plugins: [react(), honoMiddlewarePlugin(buildWebServerConfigFromEnv())], // Shared optimizeDeps exclusions so node-only packages // (`@modelcontextprotocol/sdk/client/stdio.js`, `cross-spawn`, `which`) // consumed by the dev backend aren't scanned for browser pre-bundling. // Browser code reaches the node-side stack via the Hono plugin only. ...getViteBaseConfig(), - build: { - rollupOptions: { - // Loading vite.config.ts pulls `core/mcp/remote/node/server.ts` (via the - // Hono plugin) into Rollup's module cache. That chain reaches - // `core/storage/store-io.ts`, which imports node-only `atomically`. The - // module never lands in the browser bundle (it's unreachable from - // `index.html`), but Rollup's scanner still warns about the unresolved - // import. Silence it here so the build log stays clean. - onwarn(warning, defaultHandler) { - if ( - warning.code === 'UNRESOLVED_IMPORT' && - typeof warning.message === 'string' && - warning.message.includes("'atomically'") && - warning.message.includes('store-io.ts') - ) { - return; - } - // Same story for `chokidar`: the mcp.json file watcher in - // `core/mcp/remote/node/server.ts` is reachable only through the dev - // backend, never from the browser bundle. Rollup still scans the - // import statement and warns when it can't resolve it. - if ( - warning.code === 'UNRESOLVED_IMPORT' && - typeof warning.message === 'string' && - warning.message.includes("'chokidar'") && - warning.message.includes('server.ts') - ) { - return; - } - // `@napi-rs/keyring` is the native-binding keychain backend - // consumed by `core/auth/node/secret-store.ts` from the Hono - // `/api/servers` handlers. Same reasoning as the entries above: - // unreachable from the browser bundle, so the unresolved-import - // warning is noise. - if ( - warning.code === 'UNRESOLVED_IMPORT' && - typeof warning.message === 'string' && - warning.message.includes("'@napi-rs/keyring'") && - warning.message.includes('secret-store.ts') - ) { - return; - } - defaultHandler(warning); - }, - }, - }, resolve: { // NOTE: the unit vitest project (below) overrides this — see comment there. //