From 87061f1a9c30597d46dfb925bc285ef4fb7329a5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 23:48:22 -0400 Subject: [PATCH 1/6] chore: close a batch of quality-gate & tidy gaps (#1785 #1787 #1789 #1790 #1767) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bundles five small, related tidy/gate items into one PR: - #1785: delete the dead src/types/navigation.ts InspectorTab (0 importers; duplicated/drifted from utils/inspectorTabs InspectorTabId). src/types/ now holds only ambient .d.ts stubs. - #1787: add src/hooks/** to the web coverage `include` (it was silently outside the ≥90 gate) and add tests for the two untested hooks, useServerJsonImport and useImportClientConfig, bringing every hook to ≥90 on lines/statements/functions/branches. - #1789: widen clients/{cli,tui,launcher} format/format:check to their top-level configs; add root format:shared / format:check:shared covering test-servers/src/**, vitest.shared.mts, and the repo-root eslint.config.js. - #1790: pin prettier to an exact version (3.8.4, dropping the ^3.8.1 caret) in all five package.json files so the format gate's verdict can't shift with an in-range patch bump; lockfiles synced. - #1767: its root-file format+lint gap is closed here (format:shared + a new lint:shared via a second files block in the root eslint.config.js; web/server/ was already gated in #1788). Its larger cli/tui __tests__ typecheck piece (76 type errors) is split out to #1791. Prettier-normalized the newly-covered files (pure formatting). No runtime behavior change. `npm run ci` green. Closes #1785 Closes #1787 Closes #1789 Closes #1790 Closes #1767 Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- AGENTS.md | 8 +- README.md | 2 +- clients/cli/eslint.config.js | 14 +- clients/cli/package-lock.json | 2 +- clients/cli/package.json | 6 +- clients/cli/tsup.config.ts | 30 +- clients/cli/vitest.config.ts | 22 +- clients/launcher/eslint.config.js | 14 +- clients/launcher/package-lock.json | 2 +- clients/launcher/package.json | 6 +- clients/launcher/vitest.config.ts | 14 +- clients/tui/eslint.config.js | 22 +- clients/tui/package-lock.json | 2 +- clients/tui/package.json | 6 +- clients/tui/tsup.config.ts | 42 +- clients/web/package-lock.json | 2 +- clients/web/package.json | 2 +- .../src/hooks/useImportClientConfig.test.tsx | 384 ++++++++++++++++++ .../src/hooks/useServerJsonImport.test.tsx | 254 ++++++++++++ clients/web/src/types/navigation.ts | 8 - clients/web/vite.config.ts | 1 + eslint.config.js | 60 ++- package-lock.json | 2 +- package.json | 9 +- test-servers/src/modern-tasks.ts | 6 +- test-servers/src/test-server-fixtures.ts | 17 +- test-servers/src/test-server-http.ts | 3 +- vitest.shared.mts | 84 ++-- 28 files changed, 862 insertions(+), 162 deletions(-) create mode 100644 clients/web/src/hooks/useImportClientConfig.test.tsx create mode 100644 clients/web/src/hooks/useServerJsonImport.test.tsx delete mode 100644 clients/web/src/types/navigation.ts diff --git a/AGENTS.md b/AGENTS.md index 1ba109fd0..47421ef1f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -214,10 +214,10 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - after making the changes, respond to each review comment with what was done (or why it was ignored) ### Mandatory pre-push gate -- ALWAYS do `npm run format` before committing — the **root** `format` auto-fixes `core/` (`format:core`), the root `scripts/**/*.{mjs,js,cjs,ts}` (`format:scripts`), and every client's scope in one shot. `validate` runs `format:check` (the non-fixing variant, including `format:check:core` and `format:check:scripts`) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it. +- ALWAYS do `npm run format` before committing — the **root** `format` auto-fixes `core/` (`format:core`), the root `scripts/**/*.{mjs,js,cjs,ts}` (`format:scripts`), the root "shared" surface (`format:shared` — `test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`), and every client's scope in one shot. `validate` runs `format:check` (the non-fixing variant, including `format:check:core`, `format:check:scripts`, and `format:check:shared`) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it. - **`npm run ci` is the mandatory pre-push command** — it mirrors `.github/workflows/main.yml` (minus `npm install`): `validate` → `coverage` → `verify:build-gate` (the #1769 browser-externalized-builtin build gate) → `smoke` → Storybook play-function tests (installs Playwright chromium if needed). It now runs **`npm run coverage`**, the per-file ≥90 gate (lines/statements/functions/branches) that CI enforces — so `npm run ci` is a true superset of GitHub CI, and passing it locally means CI's gates will pass. Expect several minutes. **`npm run validate`** remains the fast inner-loop check during development (unit tests only — no coverage gate, no smoke, no Storybook), but it is **NOT** an acceptable substitute for `npm run ci` before pushing: `validate` runs `test`, not `test:coverage`, so it does **zero** coverage gating. Skipping the gate is how a push passes every fast local check and still fails CI (this exact gap broke PR #1601 on a function-coverage regression). - ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs the **`core/` gate first** (`validate:core`) and then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). - - **`validate:core` is the root-owned format + lint gate (#1689, widened in #1778).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/` or the root `scripts/` before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `format:check:scripts` (`prettier --check "scripts/**/*.{mjs,js,cjs,ts}"`, the root build/verify tooling — #1778; a brace glob so a future non-`.mjs` script is auto-included) + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`). Use `npm run format:core` / `npm run format:scripts` to auto-fix (both folded into the root `format`). The `scripts/` gate is prettier-only — the root has no eslint config for `.mjs`. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). + - **`validate:core` is the root-owned format + lint gate (#1689, widened in #1778 and #1767).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/`, the root `scripts/`, or the root "shared" surface before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `format:check:scripts` (`prettier --check "scripts/**/*.{mjs,js,cjs,ts}"`, the root build/verify tooling — #1778; a brace glob so a future non-`.mjs` script is auto-included) + `format:check:shared` + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`) + `lint:shared`. Use `npm run format:core` / `npm run format:scripts` / `npm run format:shared` to auto-fix (all folded into the root `format`). The **shared surface** (#1767) is `test-servers/src/**/*.ts`, the root `vitest.shared.mts`, and the root `eslint.config.js` — first-party code no client's `eslint .` / `prettier` reaches; it is both prettier-gated (`format:check:shared`) and eslint-gated (`lint:shared`, via a second `files` block in the root `eslint.config.js` scoped to Node globals). The `scripts/` gate is prettier-only — the root has no eslint config for `.mjs`. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). **prettier is pinned to an exact version** (not a caret) in all five `package.json`s (#1790) so the gate's verdict can't shift with an in-range patch bump. - **cli and tui now typecheck their `src` (#1689).** Their `build`/`test` run through esbuild (no type check), so each has a `typecheck` script (`tsc --noEmit -p tsconfig.json`) folded into `validate`. Their `tsconfig.json` matches `clients/web/tsconfig.app.json`'s module/lib *resolution* options — DOM lib, `moduleResolution: bundler`, and **no** `noUncheckedIndexedAccess` (web's app config does not extend `tsconfig.base`, so re-enabling it would surface `core/` issues web never gates) — so the imported `core/` sources are validated the same way web validates them. It does **not** mirror web's extra strictness flags (`noUnusedLocals`, `verbatimModuleSyntax`, ES2023 target, …), so cli/tui's own `src` is checked slightly more loosely than web's. `core/` itself still typechecks through web's `tsc -b`. - The one CLI nuance: `clients/cli`'s out-of-process `e2e.test.ts` spawns the built binary, so its `test` **builds first** via `pretest` (`test-servers:build && build`). To avoid building it twice, `clients/cli`'s `validate` folds that in — it is `format:check && lint && typecheck && test` with **no** separate `build` step (the other clients, whose tests don't spawn their bundle, keep an explicit `build`). `validate:web`/`validate:tui`/`validate:launcher` are the uniform `format:check && lint && (typecheck &&) build && test`. (#1778) `clients/web`'s `format`/`format:check` covers `src`, `server`, and its top-level configs (`*.{ts,js}` — `vite.config.ts`, `tsup.runner.config.ts`, `eslint.config.js`, …), not just `src`, so the Node backend and Vite/build config are prettier-gated too. - **`npm run coverage`** is the per-file ≥90 gate and is now part of `npm run ci` — never treat it as optional before a push. It supersedes the old standalone `test:integration` step: web's `test:coverage` runs the `unit` **and** `integration` projects under v8 instrumentation, so `coverage` both enforces the ≥90 gate and exercises the same web integration paths CI covers. @@ -251,11 +251,11 @@ The web client keeps two grab-bag directories under `clients/web/src`, split by - _Importing from `@inspector/core`._ Two forms are fine: a **type-only** import is not a subsystem dependency (`pendingReauth` is pure type declarations), and **re-exporting pure functions or constants** from core is not subsystem ownership either (`oauthUx`/`oauthFlow` re-export core copy/predicates). What makes a module `lib` is wrapping core's *stateful runtime*, not merely importing from it. - **`src/lib/`** — infrastructure / integration / stateful adapters. Modules that instantiate or compose subsystems, wrap the `@inspector/core` **runtime** (not just its types), touch the DOM / `window` / `sessionStorage`, or otherwise produce side effects. (Anchors: `environmentFactory` composes `InspectorClientEnvironment`; `remoteOAuthStorage` is an adapter class over `core/auth`; `oauthResume` reads/writes `sessionStorage`; `browserTabVisibility` registers `visibilitychange` listeners; `clearServerOAuthState` drives the live `InspectorClient` / `OAuthStorage`; `downloadFile` triggers browser downloads.) -The top-level **`src/types/`** is a sibling of both and is not the place for new domain types — it's essentially the home for ambient `.d.ts` module stubs (e.g. the `react-syntax-highlighter` shims wired through `tsconfig.app.json` `paths`). One plain-`.ts` domain type still lingers there, the dead `navigation.ts` `InspectorTab`, which [#1785](https://github.com/modelcontextprotocol/inspector/issues/1785) will remove; until then a pure domain type belongs in `utils/`, not `src/types/`. +The top-level **`src/types/`** is a sibling of both and is not the place for new domain types — it's now purely the home for ambient `.d.ts` module stubs (e.g. the `react-syntax-highlighter` shims wired through `tsconfig.app.json` `paths`). The last plain-`.ts` domain type there, the dead `navigation.ts` `InspectorTab`, was removed in #1785, so a pure domain type belongs in `utils/`, not `src/types/`. Cross-directory imports point **one way, `lib → utils`** (infra depends on pure helpers, never the reverse). Keep it that way: if a `utils/` module needs a type currently exported from a `lib/` module, declare the type in `utils/` and re-export it from `lib/` (as `pendingReauth` owns `OAuthResumeAuthKind` and `oauthResume` re-exports it), rather than importing "up" from `utils` into `lib`. -Nothing **enforces** the boundary: no path alias keys off it, and the coverage `include` in `clients/web/vite.config.ts` lists **both** `src/lib/**` and `src/utils/**`, so a move between them is coverage-neutral (this is why the refactor was gate-safe). It's a human-legible signal at import time, valuable in a codebase this test-heavy (the ≥90% per-file gate). Note that `include` is a **whitelist** — it names `components`/`lib`/`utils`/`server` (plus the `core/*` runtime), so a module placed **outside** those directories (`hooks/`, `theme/`, `types/`, `App.tsx`, or a brand-new grab-bag) falls out of the ≥90 gate entirely, silently. When adding a module, place it by the rule and keep it inside a gated directory; when it genuinely mixes both (e.g. `downloadFile` bundles DOM-side-effect helpers with a couple of pure ones), keep it whole on its dominant side (`lib`) rather than splitting hairs. +Nothing **enforces** the boundary: no path alias keys off it, and the coverage `include` in `clients/web/vite.config.ts` lists **both** `src/lib/**` and `src/utils/**`, so a move between them is coverage-neutral (this is why the refactor was gate-safe). It's a human-legible signal at import time, valuable in a codebase this test-heavy (the ≥90% per-file gate). Note that `include` is a **whitelist** — it names `components`/`hooks`/`lib`/`utils`/`server` (plus the `core/*` runtime; `hooks` was added in #1787), so a module placed **outside** those directories (`theme/`, `types/`, `App.tsx`, or a brand-new grab-bag) falls out of the ≥90 gate entirely, silently. When adding a module, place it by the rule and keep it inside a gated directory; when it genuinely mixes both (e.g. `downloadFile` bundles DOM-side-effect helpers with a couple of pure ones), keep it whole on its dominant side (`lib`) rather than splitting hairs. ## React instructions - UI Components diff --git a/README.md b/README.md index 077406f9b..04693f0a3 100644 --- a/README.md +++ b/README.md @@ -147,7 +147,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. | | `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). | -Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …), plus root `validate:core` / `format:core` for the shared `core/` package and `format:scripts` for the root `scripts/` tooling. Run `npm run format` before committing — the root `format` fixes `core/`, the root `scripts/`, and every client; `validate` runs the non-fixing `format:check` and fails CI on any unformatted file. +Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …), plus root `validate:core` / `format:core` for the shared `core/` package, `format:scripts` for the root `scripts/` tooling, and `format:shared` / `lint:shared` for the root "shared" surface (`test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`). Run `npm run format` before committing — the root `format` fixes `core/`, the root `scripts/`, the shared surface, and every client; `validate` runs the non-fixing `format:check` and fails CI on any unformatted file. For the full testing rules — the ≥90% per-file gate, where test files live, the unit vs. integration vs. storybook projects, and the `v8 ignore` policy — see [`AGENTS.md`](./AGENTS.md). diff --git a/clients/cli/eslint.config.js b/clients/cli/eslint.config.js index 1157168e4..9407f9975 100644 --- a/clients/cli/eslint.config.js +++ b/clients/cli/eslint.config.js @@ -1,18 +1,18 @@ -import js from '@eslint/js'; -import globals from 'globals'; -import tseslint from 'typescript-eslint'; -import { defineConfig, globalIgnores } from 'eslint/config'; +import js from "@eslint/js"; +import globals from "globals"; +import tseslint from "typescript-eslint"; +import { defineConfig, globalIgnores } from "eslint/config"; // The CLI is plain Node TypeScript (no React/browser), so this mirrors the web // client's flat config minus the React/Storybook plugins. export default defineConfig([ - globalIgnores(['build', 'coverage']), + globalIgnores(["build", "coverage"]), { - files: ['**/*.ts'], + files: ["**/*.ts"], extends: [js.configs.recommended, tseslint.configs.recommended], languageOptions: { ecmaVersion: 2022, - sourceType: 'module', + sourceType: "module", globals: globals.node, }, }, diff --git a/clients/cli/package-lock.json b/clients/cli/package-lock.json index 2760136d3..0198d9f4d 100644 --- a/clients/cli/package-lock.json +++ b/clients/cli/package-lock.json @@ -29,7 +29,7 @@ "@vitest/coverage-v8": "^4.1.0", "eslint": "^9.39.4", "globals": "^17.4.0", - "prettier": "^3.8.1", + "prettier": "3.8.4", "tsup": "^8.5.0", "typescript": "~5.9.3", "typescript-eslint": "^8.56.1", diff --git a/clients/cli/package.json b/clients/cli/package.json index 9ef0e9bd8..57bbe4f18 100644 --- a/clients/cli/package.json +++ b/clients/cli/package.json @@ -28,8 +28,8 @@ "test:cli-metadata": "vitest run metadata.test.ts", "pretest": "npm run test-servers:build && npm run build", "lint": "eslint .", - "format": "prettier --write src __tests__", - "format:check": "prettier --check src __tests__" + "format": "prettier --write src __tests__ \"*.{ts,js}\"", + "format:check": "prettier --check src __tests__ \"*.{ts,js}\"" }, "dependencies": { "@modelcontextprotocol/client": "2.0.0-beta.5", @@ -50,7 +50,7 @@ "@vitest/coverage-v8": "^4.1.0", "eslint": "^9.39.4", "globals": "^17.4.0", - "prettier": "^3.8.1", + "prettier": "3.8.4", "tsup": "^8.5.0", "typescript": "~5.9.3", "typescript-eslint": "^8.56.1", diff --git a/clients/cli/tsup.config.ts b/clients/cli/tsup.config.ts index d65d37ec3..bacd84730 100644 --- a/clients/cli/tsup.config.ts +++ b/clients/cli/tsup.config.ts @@ -1,32 +1,32 @@ -import { defineConfig } from 'tsup'; -import path from 'node:path'; -import { fileURLToPath } from 'node:url'; +import { defineConfig } from "tsup"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; const dirname = path.dirname(fileURLToPath(import.meta.url)); -const repoRoot = path.resolve(dirname, '../..'); +const repoRoot = path.resolve(dirname, "../.."); export default defineConfig({ - entry: ['src/index.ts'], - format: ['esm'], - outDir: 'build', + entry: ["src/index.ts"], + format: ["esm"], + outDir: "build", clean: true, // No source maps in the published bundle — they roughly double the on-disk // size and aren't needed at runtime (debug via `npm run dev` on the source). sourcemap: false, - target: 'node22', - platform: 'node', + target: "node22", + platform: "node", // Bundle core source; leave npm deps external. noExternal: [/^@inspector\/core/], external: [ - '@napi-rs/keyring', - '@modelcontextprotocol/client', - '@modelcontextprotocol/core', - 'commander', - 'pino', + "@napi-rs/keyring", + "@modelcontextprotocol/client", + "@modelcontextprotocol/core", + "commander", + "pino", ], esbuildOptions(options) { options.alias = { - '@inspector/core': path.join(repoRoot, 'core'), + "@inspector/core": path.join(repoRoot, "core"), }; }, }); diff --git a/clients/cli/vitest.config.ts b/clients/cli/vitest.config.ts index 4480e1bd3..d6afd7333 100644 --- a/clients/cli/vitest.config.ts +++ b/clients/cli/vitest.config.ts @@ -1,7 +1,7 @@ -import path from 'node:path'; -import { fileURLToPath } from 'node:url'; -import { defineConfig } from 'vitest/config'; -import { vitestSharedPaths } from '../../vitest.shared.mts'; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { defineConfig } from "vitest/config"; +import { vitestSharedPaths } from "../../vitest.shared.mts"; const dirname = path.dirname(fileURLToPath(import.meta.url)); const { projectResolve } = vitestSharedPaths(dirname); @@ -10,8 +10,8 @@ export default defineConfig({ resolve: projectResolve, test: { globals: false, - environment: 'node', - include: ['__tests__/**/*.test.ts'], + environment: "node", + include: ["__tests__/**/*.test.ts"], testTimeout: 15000, // The in-process runner (__tests__/helpers/cli-runner.ts) patches // process.std{out,err}.write to capture CLI output. Test files run in @@ -19,18 +19,18 @@ export default defineConfig({ // those global patches never overlap. `forks` is vitest's default, but pin // it explicitly so the capture isolation can't regress to a shared-thread // pool. See #1484. - pool: 'forks', + pool: "forks", coverage: { - provider: 'v8', - reporter: ['text', 'html', 'json-summary'], - include: ['src/**/*.ts'], + provider: "v8", + reporter: ["text", "html", "json-summary"], + include: ["src/**/*.ts"], exclude: [ // Binary bootstrap: shebang + `isMain` guard + `runCli()`/`process.exit` // wiring that only runs when launched as the real binary. Exercised by // the out-of-process layer (__tests__/e2e.test.ts + scripts/smoke-cli.mjs), // which spawns build/index.js and so can't be measured under in-process // coverage. Mirrors web's `**/index.{ts,tsx}` exclusion. - 'src/index.ts', + "src/index.ts", ], thresholds: { perFile: true, diff --git a/clients/launcher/eslint.config.js b/clients/launcher/eslint.config.js index 17a394f19..535689a1f 100644 --- a/clients/launcher/eslint.config.js +++ b/clients/launcher/eslint.config.js @@ -1,18 +1,18 @@ -import js from '@eslint/js'; -import globals from 'globals'; -import tseslint from 'typescript-eslint'; -import { defineConfig, globalIgnores } from 'eslint/config'; +import js from "@eslint/js"; +import globals from "globals"; +import tseslint from "typescript-eslint"; +import { defineConfig, globalIgnores } from "eslint/config"; // The launcher is plain Node TypeScript (no React/browser), so this mirrors // the web client's flat config minus the React/Storybook plugins. export default defineConfig([ - globalIgnores(['build', 'coverage']), + globalIgnores(["build", "coverage"]), { - files: ['**/*.{ts,mts}'], + files: ["**/*.{ts,mts}"], extends: [js.configs.recommended, tseslint.configs.recommended], languageOptions: { ecmaVersion: 2022, - sourceType: 'module', + sourceType: "module", globals: globals.node, }, }, diff --git a/clients/launcher/package-lock.json b/clients/launcher/package-lock.json index 55f0543cf..4c112ddc4 100644 --- a/clients/launcher/package-lock.json +++ b/clients/launcher/package-lock.json @@ -19,7 +19,7 @@ "@vitest/coverage-v8": "^4.1.0", "eslint": "^9.39.4", "globals": "^17.4.0", - "prettier": "^3.8.1", + "prettier": "3.8.4", "typescript": "~5.9.3", "typescript-eslint": "^8.56.1", "vitest": "^4.1.0" diff --git a/clients/launcher/package.json b/clients/launcher/package.json index 6a5f5ff76..cd5703668 100644 --- a/clients/launcher/package.json +++ b/clients/launcher/package.json @@ -15,8 +15,8 @@ "build": "tsc", "postbuild": "node scripts/make-executable.js", "lint": "eslint .", - "format": "prettier --write src __tests__", - "format:check": "prettier --check src __tests__", + "format": "prettier --write src __tests__ \"*.{ts,js}\"", + "format:check": "prettier --check src __tests__ \"*.{ts,js}\"", "test": "vitest run", "test:coverage": "vitest run --coverage", "validate": "npm run format:check && npm run lint && npm run build && npm run test" @@ -30,7 +30,7 @@ "@vitest/coverage-v8": "^4.1.0", "eslint": "^9.39.4", "globals": "^17.4.0", - "prettier": "^3.8.1", + "prettier": "3.8.4", "typescript": "~5.9.3", "typescript-eslint": "^8.56.1", "vitest": "^4.1.0" diff --git a/clients/launcher/vitest.config.ts b/clients/launcher/vitest.config.ts index b64920005..eef71846a 100644 --- a/clients/launcher/vitest.config.ts +++ b/clients/launcher/vitest.config.ts @@ -1,14 +1,14 @@ -import { defineConfig } from 'vitest/config'; +import { defineConfig } from "vitest/config"; export default defineConfig({ test: { globals: false, - environment: 'node', - include: ['__tests__/**/*.test.ts'], + environment: "node", + include: ["__tests__/**/*.test.ts"], coverage: { - provider: 'v8', - reporter: ['text', 'html', 'json-summary'], - include: ['src/**/*.ts'], + provider: "v8", + reporter: ["text", "html", "json-summary"], + include: ["src/**/*.ts"], exclude: [ // Binary bootstrap: shebang + commander wiring that dynamically imports // each client's build/index.js and dispatches via process.exit. It only @@ -16,7 +16,7 @@ export default defineConfig({ // launcher smokes (smoke:launcher / smoke:cli / smoke:tui / smoke:web). // Mirrors the cli/web `index.ts` exclusion. The pure arg-parsing logic // lives in parse-launcher-argv.ts, which is unit-tested and gated below. - 'src/index.ts', + "src/index.ts", ], thresholds: { perFile: true, diff --git a/clients/tui/eslint.config.js b/clients/tui/eslint.config.js index 64bf01aaa..b4f8a251a 100644 --- a/clients/tui/eslint.config.js +++ b/clients/tui/eslint.config.js @@ -1,8 +1,8 @@ -import js from '@eslint/js'; -import globals from 'globals'; -import reactHooks from 'eslint-plugin-react-hooks'; -import tseslint from 'typescript-eslint'; -import { defineConfig, globalIgnores } from 'eslint/config'; +import js from "@eslint/js"; +import globals from "globals"; +import reactHooks from "eslint-plugin-react-hooks"; +import tseslint from "typescript-eslint"; +import { defineConfig, globalIgnores } from "eslint/config"; // The TUI is Node + React (Ink) TypeScript. This mirrors the web client's flat // config minus the browser-only plugins (react-refresh / Storybook). @@ -15,19 +15,19 @@ import { defineConfig, globalIgnores } from 'eslint/config'; // component/hook surface that is itself the interim-excluded one tracked in // #1501. Enforcing them would require refactoring code out of scope here. export default defineConfig([ - globalIgnores(['build', 'coverage']), + globalIgnores(["build", "coverage"]), { - files: ['**/*.{ts,tsx}'], - plugins: { 'react-hooks': reactHooks }, + files: ["**/*.{ts,tsx}"], + plugins: { "react-hooks": reactHooks }, extends: [js.configs.recommended, tseslint.configs.recommended], languageOptions: { ecmaVersion: 2022, - sourceType: 'module', + sourceType: "module", globals: globals.node, }, rules: { - 'react-hooks/rules-of-hooks': 'error', - 'react-hooks/exhaustive-deps': 'warn', + "react-hooks/rules-of-hooks": "error", + "react-hooks/exhaustive-deps": "warn", }, }, ]); diff --git a/clients/tui/package-lock.json b/clients/tui/package-lock.json index 1f24c6f61..e805f79cf 100644 --- a/clients/tui/package-lock.json +++ b/clients/tui/package-lock.json @@ -34,7 +34,7 @@ "eslint-plugin-react-hooks": "^7.0.1", "globals": "^17.4.0", "ink-testing-library": "^4.0.0", - "prettier": "^3.8.1", + "prettier": "3.8.4", "tsup": "^8.5.0", "tsx": "^4.21.0", "typescript": "~5.9.3", diff --git a/clients/tui/package.json b/clients/tui/package.json index 67095d5dd..9d52b0c44 100644 --- a/clients/tui/package.json +++ b/clients/tui/package.json @@ -23,8 +23,8 @@ "test": "vitest run", "test:coverage": "vitest run --coverage", "lint": "eslint .", - "format": "prettier --write src __tests__ index.ts tui.tsx dev.ts", - "format:check": "prettier --check src __tests__ index.ts tui.tsx dev.ts" + "format": "prettier --write src __tests__ \"*.{ts,tsx,js}\"", + "format:check": "prettier --check src __tests__ \"*.{ts,tsx,js}\"" }, "dependencies": { "@modelcontextprotocol/client": "2.0.0-beta.5", @@ -53,7 +53,7 @@ "eslint-plugin-react-hooks": "^7.0.1", "globals": "^17.4.0", "ink-testing-library": "^4.0.0", - "prettier": "^3.8.1", + "prettier": "3.8.4", "tsup": "^8.5.0", "tsx": "^4.21.0", "typescript": "~5.9.3", diff --git a/clients/tui/tsup.config.ts b/clients/tui/tsup.config.ts index 7d4313076..92a637b37 100644 --- a/clients/tui/tsup.config.ts +++ b/clients/tui/tsup.config.ts @@ -1,37 +1,37 @@ -import { defineConfig } from 'tsup'; -import path from 'node:path'; -import { fileURLToPath } from 'node:url'; +import { defineConfig } from "tsup"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; const dirname = path.dirname(fileURLToPath(import.meta.url)); -const repoRoot = path.resolve(dirname, '../..'); +const repoRoot = path.resolve(dirname, "../.."); export default defineConfig({ - entry: ['index.ts'], - format: ['esm'], - outDir: 'build', + entry: ["index.ts"], + format: ["esm"], + outDir: "build", clean: true, // No source maps in the published bundle — they roughly double the on-disk // size and aren't needed at runtime (debug via `npm run dev` on the source). sourcemap: false, - target: 'node22', - platform: 'node', + target: "node22", + platform: "node", noExternal: [/^@inspector\/core/], external: [ - 'react', - 'ink', - 'ink-form', - 'ink-scroll-view', - 'open', - 'commander', - 'pino', - '@modelcontextprotocol/client', - '@modelcontextprotocol/core', - '@napi-rs/keyring', + "react", + "ink", + "ink-form", + "ink-scroll-view", + "open", + "commander", + "pino", + "@modelcontextprotocol/client", + "@modelcontextprotocol/core", + "@napi-rs/keyring", ], esbuildOptions(options) { options.alias = { - '@inspector/core': path.join(repoRoot, 'core'), + "@inspector/core": path.join(repoRoot, "core"), }; - options.jsx = 'automatic'; + options.jsx = "automatic"; }, }); diff --git a/clients/web/package-lock.json b/clients/web/package-lock.json index 027df5286..18e0d71cd 100644 --- a/clients/web/package-lock.json +++ b/clients/web/package-lock.json @@ -69,7 +69,7 @@ "globals": "^17.4.0", "happy-dom": "^20.9.0", "playwright": "^1.58.2", - "prettier": "^3.8.4", + "prettier": "3.8.4", "storybook": "^10.2.19", "tsup": "^8.5.1", "typescript": "~5.9.3", diff --git a/clients/web/package.json b/clients/web/package.json index 3628f6d51..be99aad26 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -91,7 +91,7 @@ "globals": "^17.4.0", "happy-dom": "^20.9.0", "playwright": "^1.58.2", - "prettier": "^3.8.1", + "prettier": "3.8.4", "storybook": "^10.2.19", "tsup": "^8.5.1", "typescript": "~5.9.3", diff --git a/clients/web/src/hooks/useImportClientConfig.test.tsx b/clients/web/src/hooks/useImportClientConfig.test.tsx new file mode 100644 index 000000000..b406d111c --- /dev/null +++ b/clients/web/src/hooks/useImportClientConfig.test.tsx @@ -0,0 +1,384 @@ +import { describe, it, expect, vi } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { + useImportClientConfig, + type UseImportClientConfigOptions, +} from "./useImportClientConfig"; +import type { ImportSourceResult } from "@inspector/core/mcp/import/index.js"; +import type { MCPConfig } from "@inspector/core/mcp/types.js"; + +function config(...ids: string[]): MCPConfig { + const mcpServers: MCPConfig["mcpServers"] = {}; + for (const id of ids) mcpServers[id] = { command: "node", args: [id] }; + return { mcpServers }; +} + +function foundSource(cfg: MCPConfig): ImportSourceResult { + return { type: "claude", found: true, config: cfg, searched: ["/path"] }; +} + +function setup(over: Partial = {}) { + const onFetchSource = + over.onFetchSource ?? vi.fn(async () => foundSource(config("alpha"))); + const onAddServer = over.onAddServer ?? vi.fn(async () => {}); + const onUpdateServer = over.onUpdateServer ?? vi.fn(async () => {}); + const opts: UseImportClientConfigOptions = { + opened: true, + existingIds: [], + onFetchSource, + onAddServer, + onUpdateServer, + ...over, + }; + const view = renderHook( + (p: UseImportClientConfigOptions) => useImportClientConfig(p), + { initialProps: opts }, + ); + return { ...view, onFetchSource, onAddServer, onUpdateServer }; +} + +describe("useImportClientConfig", () => { + it("starts in the select phase with nothing to import", () => { + const { result } = setup(); + expect(result.current.phase).toBe("select"); + expect(result.current.plan).toBeNull(); + expect(result.current.canImport).toBe(false); + expect(result.current.importCount).toBe(0); + }); + + it("setSelectedType records the choice", () => { + const { result } = setup(); + act(() => result.current.setSelectedType("claude")); + expect(result.current.selectedType).toBe("claude"); + }); + + it("pickSource loads a config and moves to review", async () => { + const { result } = setup({ + onFetchSource: vi.fn(async () => foundSource(config("alpha", "beta"))), + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + expect(result.current.phase).toBe("review"); + expect(result.current.plan?.additions).toHaveLength(2); + expect(result.current.importCount).toBe(2); + expect(result.current.canImport).toBe(true); + }); + + it("pickSource surfaces a fetch-reported error", async () => { + const { result } = setup({ + onFetchSource: vi.fn(async () => ({ + type: "claude", + found: false, + searched: [], + error: "backend blew up", + })), + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + expect(result.current.phase).toBe("select"); + expect(result.current.error).toBe("backend blew up"); + }); + + it("pickSource shows a not-found notice", async () => { + const { result } = setup({ + onFetchSource: vi.fn(async () => ({ + type: "claude", + found: false, + searched: ["/a", "/b"], + })), + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + expect(result.current.phase).toBe("select"); + expect(result.current.notice).toContain("No config found"); + }); + + it("pickSource handles an empty config as no servers", async () => { + const { result } = setup({ + onFetchSource: vi.fn(async () => foundSource(config())), + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + expect(result.current.phase).toBe("select"); + expect(result.current.error).toContain("No servers found"); + }); + + it("pickSource surfaces a thrown fetch error", async () => { + const { result } = setup({ + onFetchSource: vi.fn(async () => { + throw new Error("network down"); + }), + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + expect(result.current.phase).toBe("select"); + expect(result.current.error).toBe("network down"); + }); + + it("pickFile parses an uploaded config", async () => { + const { result } = setup(); + const raw = JSON.stringify({ mcpServers: { gamma: { command: "go" } } }); + const file = new File([raw], "claude.json"); + await act(async () => { + await result.current.pickFile(file); + }); + expect(result.current.phase).toBe("review"); + expect(result.current.plan?.additions[0].id).toBe("gamma"); + }); + + it("pickFile ignores a null file", async () => { + const { result } = setup(); + await act(async () => { + await result.current.pickFile(null); + }); + expect(result.current.phase).toBe("select"); + }); + + it("pickFile surfaces a parse error", async () => { + const { result } = setup(); + const file = new File(["{ not json"], "claude.json"); + await act(async () => { + await result.current.pickFile(file); + }); + expect(result.current.phase).toBe("select"); + expect(result.current.error).toBeTruthy(); + }); + + it("runImport adds new servers and reports outcomes", async () => { + const onAddServer = vi.fn(async () => {}); + const { result } = setup({ + onFetchSource: vi.fn(async () => foundSource(config("alpha", "beta"))), + onAddServer, + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + await act(async () => { + await result.current.runImport(); + }); + expect(result.current.phase).toBe("summary"); + expect(onAddServer).toHaveBeenCalledTimes(2); + expect(result.current.outcomes.every((o) => o.status === "added")).toBe( + true, + ); + }); + + it("runImport skips additions opted out", async () => { + const onAddServer = vi.fn(async () => {}); + const { result } = setup({ + onFetchSource: vi.fn(async () => foundSource(config("alpha", "beta"))), + onAddServer, + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + act(() => result.current.setAdditionAction("beta", "skip")); + expect(result.current.importCount).toBe(1); + await act(async () => { + await result.current.runImport(); + }); + expect(onAddServer).toHaveBeenCalledTimes(1); + expect(result.current.outcomes.find((o) => o.id === "beta")?.status).toBe( + "skipped", + ); + }); + + it("runImport reports an addition failure", async () => { + const onAddServer = vi.fn(async () => { + throw new Error("add failed"); + }); + const { result } = setup({ + onFetchSource: vi.fn(async () => foundSource(config("alpha"))), + onAddServer, + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + await act(async () => { + await result.current.runImport(); + }); + const outcome = result.current.outcomes[0]; + expect(outcome.status).toBe("failed"); + expect(outcome.detail).toBe("add failed"); + }); + + it("resolves conflicts by overwrite, skip, and rename", async () => { + const onAddServer = vi.fn(async () => {}); + const onUpdateServer = vi.fn(async () => {}); + const { result } = setup({ + existingIds: ["alpha", "beta", "gamma"], + onFetchSource: vi.fn(async () => + foundSource(config("alpha", "beta", "gamma")), + ), + onAddServer, + onUpdateServer, + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + expect(result.current.plan?.conflicts).toHaveLength(3); + // Default is skip for all three. + act(() => result.current.setResolution("alpha", "overwrite")); + act(() => result.current.setResolution("gamma", "rename")); + act(() => result.current.setRenameTo("gamma", "gamma-new")); + await act(async () => { + await result.current.runImport(); + }); + expect(onUpdateServer).toHaveBeenCalledWith( + "alpha", + "alpha", + expect.any(Object), + ); + expect(onAddServer).toHaveBeenCalledWith("gamma-new", expect.any(Object)); + const statuses = Object.fromEntries( + result.current.outcomes.map((o) => [o.id, o.status]), + ); + expect(statuses["alpha"]).toBe("overwritten"); + expect(statuses["beta"]).toBe("skipped"); + expect(statuses["gamma-new"]).toBe("renamed"); + }); + + it("flags invalid and colliding rename targets", async () => { + const { result } = setup({ + existingIds: ["alpha", "beta"], + onFetchSource: vi.fn(async () => foundSource(config("alpha", "beta"))), + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + // Empty rename target. + act(() => result.current.setResolution("alpha", "rename")); + act(() => result.current.setRenameTo("alpha", " ")); + expect(result.current.renameErrors["alpha"]).toContain("required"); + // Syntactically invalid. + act(() => result.current.setRenameTo("alpha", "bad id!")); + expect(result.current.renameErrors["alpha"]).toContain("letters"); + // Collides with the other existing id. + act(() => result.current.setRenameTo("alpha", "beta")); + expect(result.current.renameErrors["alpha"]).toContain("already in use"); + expect(result.current.canImport).toBe(false); + }); + + it("flags a rename target that collides with another rename", async () => { + const { result } = setup({ + existingIds: ["alpha", "beta"], + onFetchSource: vi.fn(async () => foundSource(config("alpha", "beta"))), + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + act(() => result.current.setResolution("alpha", "rename")); + act(() => result.current.setResolution("beta", "rename")); + act(() => result.current.setRenameTo("alpha", "shared")); + act(() => result.current.setRenameTo("beta", "shared")); + // Each rename sees the other's target already claimed. + expect(result.current.renameErrors["beta"]).toContain("already in use"); + expect(result.current.canImport).toBe(false); + }); + + it("renames to the original id when the rename target is blank", async () => { + const onAddServer = vi.fn(async () => {}); + const { result } = setup({ + existingIds: ["alpha"], + onFetchSource: vi.fn(async () => foundSource(config("alpha"))), + onAddServer, + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + act(() => result.current.setResolution("alpha", "rename")); + act(() => result.current.setRenameTo("alpha", " ")); + await act(async () => { + await result.current.runImport(); + }); + // A blank rename target falls back to the original id. + expect(onAddServer).toHaveBeenCalledWith("alpha", expect.any(Object)); + expect(result.current.outcomes[0].status).toBe("renamed"); + }); + + it("back returns to the select phase", async () => { + const { result } = setup(); + await act(async () => { + await result.current.pickSource("claude"); + }); + expect(result.current.phase).toBe("review"); + act(() => result.current.back()); + expect(result.current.phase).toBe("select"); + }); + + it("resets when the modal is re-opened", async () => { + const { result, rerender } = setup({ opened: false }); + const props = (opened: boolean): UseImportClientConfigOptions => ({ + opened, + existingIds: [], + onFetchSource: vi.fn(async () => foundSource(config("alpha"))), + onAddServer: vi.fn(async () => {}), + onUpdateServer: vi.fn(async () => {}), + }); + rerender(props(true)); + await act(async () => { + await result.current.pickSource("claude"); + }); + expect(result.current.phase).toBe("review"); + rerender(props(false)); + rerender(props(true)); + expect(result.current.phase).toBe("select"); + expect(result.current.plan).toBeNull(); + }); + + it("stringifies a non-Error thrown by pickSource", async () => { + const { result } = setup({ + onFetchSource: vi.fn(async () => { + throw "plain fetch failure"; + }), + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + expect(result.current.error).toBe("plain fetch failure"); + }); + + it("stringifies a non-Error thrown by pickFile", async () => { + const { result } = setup(); + const file = new File(["x"], "claude.json"); + vi.spyOn(file, "text").mockRejectedValue("plain read failure"); + await act(async () => { + await result.current.pickFile(file); + }); + expect(result.current.error).toBe("plain read failure"); + }); + + it("stringifies a non-Error thrown while applying an import", async () => { + const onAddServer = vi.fn(async () => { + throw "plain add failure"; + }); + const { result } = setup({ + onFetchSource: vi.fn(async () => foundSource(config("alpha"))), + onAddServer, + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + await act(async () => { + await result.current.runImport(); + }); + expect(result.current.outcomes[0]).toMatchObject({ + status: "failed", + detail: "plain add failure", + }); + }); + + it("runImport with no plan is a no-op", async () => { + const { result } = setup(); + await act(async () => { + await result.current.runImport(); + }); + expect(result.current.phase).toBe("select"); + expect(result.current.outcomes).toEqual([]); + }); +}); diff --git a/clients/web/src/hooks/useServerJsonImport.test.tsx b/clients/web/src/hooks/useServerJsonImport.test.tsx new file mode 100644 index 000000000..5a7b04f03 --- /dev/null +++ b/clients/web/src/hooks/useServerJsonImport.test.tsx @@ -0,0 +1,254 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { + useServerJsonImport, + type UseServerJsonImportOptions, +} from "./useServerJsonImport"; + +// A valid registry server.json with two runnable options; option 0 declares a +// required env var (with no default) and an optional one (with a default). +const VALID_SERVER_JSON = JSON.stringify({ + name: "io.github.acme/weather", + packages: [ + { + registryType: "npm", + identifier: "weather-mcp", + environmentVariables: [ + { name: "API_KEY", isRequired: true, description: "The key" }, + { name: "REGION", isRequired: false, default: "us" }, + ], + }, + { registryType: "pypi", identifier: "weather-py" }, + ], +}); + +function setup(over: Partial = {}) { + const onAddServer = over.onAddServer ?? vi.fn(async () => {}); + const opts: UseServerJsonImportOptions = { + opened: true, + existingIds: [], + onAddServer, + ...over, + }; + const view = renderHook( + (p: UseServerJsonImportOptions) => useServerJsonImport(p), + { initialProps: opts }, + ); + return { ...view, onAddServer }; +} + +/** Set the textarea content and flush the validation debounce. */ +function typeAndFlush( + result: { current: ReturnType }, + content: string, +) { + act(() => result.current.setRawText(content)); + act(() => { + vi.advanceTimersByTime(300); + }); +} + +describe("useServerJsonImport", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.runOnlyPendingTimers(); + vi.useRealTimers(); + }); + + it("starts empty with an info prompt and the Add button disabled", () => { + const { result } = setup(); + expect(result.current.draft.rawText).toBe(""); + expect(result.current.canAdd).toBe(false); + expect(result.current.validation).toEqual([ + { type: "info", message: "Paste server.json content to validate." }, + ]); + expect(result.current.packages).toBeUndefined(); + expect(result.current.envVars).toEqual([]); + }); + + it("validates pasted content and enables Add", () => { + const { result } = setup(); + typeAndFlush(result, VALID_SERVER_JSON); + expect(result.current.canAdd).toBe(true); + expect(result.current.defaultServerName).toBe("weather"); + expect(result.current.packages).toHaveLength(2); + expect(result.current.validation[0]).toMatchObject({ type: "success" }); + expect(result.current.validation[1]).toMatchObject({ type: "info" }); + // Env vars of the first option, with the optional one's default prefilled. + expect(result.current.envVars).toEqual([ + { name: "API_KEY", description: "The key", required: true, value: "" }, + { name: "REGION", description: undefined, required: false, value: "us" }, + ]); + }); + + it("surfaces a parse error for invalid JSON", () => { + const { result } = setup(); + typeAndFlush(result, "{ not json"); + expect(result.current.canAdd).toBe(false); + expect(result.current.validation[0].type).toBe("error"); + }); + + it("flags an invalid overridden id", () => { + const { result } = setup(); + typeAndFlush(result, VALID_SERVER_JSON); + act(() => result.current.setServerName("bad id!")); + expect(result.current.canAdd).toBe(false); + expect( + result.current.validation.some( + (v) => v.type === "error" && v.message.includes("Server id"), + ), + ).toBe(true); + }); + + it("warns on a duplicate id", () => { + const { result } = setup({ existingIds: ["weather"] }); + typeAndFlush(result, VALID_SERVER_JSON); + expect(result.current.canAdd).toBe(false); + expect( + result.current.validation.some( + (v) => v.type === "warning" && v.message.includes("already exists"), + ), + ).toBe(true); + }); + + it("selectPackage switches the active option and its env vars", () => { + const { result } = setup(); + typeAndFlush(result, VALID_SERVER_JSON); + act(() => result.current.selectPackage(1)); + expect(result.current.draft.selectedPackageIndex).toBe(1); + // The pypi option declares no env vars. + expect(result.current.envVars).toEqual([]); + }); + + it("setEnvVar overrides a declared env value", () => { + const { result } = setup(); + typeAndFlush(result, VALID_SERVER_JSON); + act(() => result.current.setEnvVar("API_KEY", "secret")); + expect( + result.current.envVars.find((v) => v.name === "API_KEY")?.value, + ).toBe("secret"); + }); + + it("re-opens the File Contents disclosure when cleared", () => { + const { result } = setup(); + typeAndFlush(result, VALID_SERVER_JSON); + act(() => result.current.setFileContentsOpen(false)); + expect(result.current.fileContentsOpen).toBe(false); + typeAndFlush(result, ""); + expect(result.current.fileContentsOpen).toBe(true); + }); + + it("flashes then auto-collapses the disclosure after content loads", () => { + const { result } = setup(); + act(() => result.current.setRawText(VALID_SERVER_JSON)); + expect(result.current.fileContentsOpen).toBe(true); + act(() => { + vi.advanceTimersByTime(1000); + }); + expect(result.current.fileContentsHighlight).toBe(true); + act(() => { + vi.advanceTimersByTime(250); + }); + expect(result.current.fileContentsHighlight).toBe(false); + expect(result.current.fileContentsOpen).toBe(false); + }); + + it("resets its state when the modal is re-opened", () => { + const { result, rerender } = setup({ opened: false }); + // Open, dirty the draft, then close and re-open — the draft must clear. + rerender({ opened: true, existingIds: [], onAddServer: vi.fn() }); + typeAndFlush(result, VALID_SERVER_JSON); + expect(result.current.draft.rawText).not.toBe(""); + rerender({ opened: false, existingIds: [], onAddServer: vi.fn() }); + rerender({ opened: true, existingIds: [], onAddServer: vi.fn() }); + expect(result.current.draft.rawText).toBe(""); + }); + + it("pickFile reads a file into the draft", async () => { + const { result } = setup(); + const file = new File([VALID_SERVER_JSON], "server.json"); + await act(async () => { + await result.current.pickFile(file); + }); + expect(result.current.draft.rawText).toBe(VALID_SERVER_JSON); + }); + + it("pickFile ignores a null file", async () => { + const { result } = setup(); + await act(async () => { + await result.current.pickFile(null); + }); + expect(result.current.draft.rawText).toBe(""); + }); + + it("pickFile surfaces a read error", async () => { + const { result } = setup(); + const file = new File(["x"], "server.json"); + vi.spyOn(file, "text").mockRejectedValue(new Error("read boom")); + await act(async () => { + await result.current.pickFile(file); + }); + expect( + result.current.validation.some((v) => v.message.includes("read boom")), + ).toBe(true); + }); + + it("submit persists the selected server and resolves true", async () => { + const onAddServer = vi.fn(async () => {}); + const { result } = setup({ onAddServer }); + typeAndFlush(result, VALID_SERVER_JSON); + let ok = false; + await act(async () => { + ok = await result.current.submit(); + }); + expect(ok).toBe(true); + expect(onAddServer).toHaveBeenCalledWith("weather", expect.any(Object)); + }); + + it("submit refuses invalid content", async () => { + const onAddServer = vi.fn(async () => {}); + const { result } = setup({ onAddServer }); + typeAndFlush(result, "{ not json"); + let ok = true; + await act(async () => { + ok = await result.current.submit(); + }); + expect(ok).toBe(false); + expect(onAddServer).not.toHaveBeenCalled(); + expect( + result.current.validation.some((v) => + v.message.includes("Fix the validation errors"), + ), + ).toBe(true); + }); + + it("submit refuses a duplicate id", async () => { + const onAddServer = vi.fn(async () => {}); + const { result } = setup({ existingIds: ["weather"], onAddServer }); + typeAndFlush(result, VALID_SERVER_JSON); + let ok = true; + await act(async () => { + ok = await result.current.submit(); + }); + expect(ok).toBe(false); + expect(onAddServer).not.toHaveBeenCalled(); + }); + + it("submit surfaces an onAddServer failure", async () => { + const onAddServer = vi.fn(async () => { + throw new Error("save boom"); + }); + const { result } = setup({ onAddServer }); + typeAndFlush(result, VALID_SERVER_JSON); + let ok = true; + await act(async () => { + ok = await result.current.submit(); + }); + expect(ok).toBe(false); + expect( + result.current.validation.some((v) => v.message.includes("save boom")), + ).toBe(true); + }); +}); diff --git a/clients/web/src/types/navigation.ts b/clients/web/src/types/navigation.ts deleted file mode 100644 index e60781015..000000000 --- a/clients/web/src/types/navigation.ts +++ /dev/null @@ -1,8 +0,0 @@ -export type InspectorTab = - | "tools" - | "apps" - | "prompts" - | "resources" - | "logs" - | "tasks" - | "protocol"; diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index 21cc39af6..668e651f4 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -169,6 +169,7 @@ export default defineConfig(({ command }) => { reporter: ["text", "html", "json-summary"], include: [ "src/components/**/*.{ts,tsx}", + "src/hooks/**/*.{ts,tsx}", "src/lib/**/*.{ts,tsx}", "src/utils/**/*.{ts,tsx}", "clients/web/server/**/*.{ts,tsx}", diff --git a/eslint.config.js b/eslint.config.js index 4db86dbe0..d29cbd721 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -3,13 +3,33 @@ import globals from "globals"; import tseslint from "typescript-eslint"; import { defineConfig, globalIgnores } from "eslint/config"; -// Root-level lint gate for the shared `core/` package. Each client's own -// `eslint .` is scoped to its own directory, so nothing reached `core/` before -// (#1689) — this config closes that gap. `core/` is isomorphic TypeScript -// (browser-side OAuth + Node backends + shared runtime), so both browser and -// Node globals apply; there is no JSX in `core/`, so no React plugin is needed. +// Root-level lint gate for first-party code that no client's own `eslint .` +// (each scoped to its own directory) reaches: the shared `core/` package +// (#1689) and the root-owned "shared" surface — `test-servers/src/**`, the +// root `vitest.shared.mts`, and this config file itself (#1767). `core/` is +// isomorphic TypeScript (browser-side OAuth + Node backends + shared runtime); +// the shared surface is Node-only tooling/tests. Neither has JSX, so no React +// plugin is needed. +const sharedRules = { + // An `_`-prefix is the explicit "intentionally unused" marker — + // interface-conformance params in fakes, destructuring-rest omissions, + // and reserved-for-later args. Honor it rather than deleting signal. + "@typescript-eslint/no-unused-vars": [ + "error", + { + argsIgnorePattern: "^_", + varsIgnorePattern: "^_", + caughtErrorsIgnorePattern: "^_", + }, + ], +}; + export default defineConfig([ - globalIgnores(["core/**/build/**", "core/**/dist/**"]), + globalIgnores([ + "core/**/build/**", + "core/**/dist/**", + "test-servers/build/**", + ]), { files: ["core/**/*.{ts,tsx}"], extends: [js.configs.recommended, tseslint.configs.recommended], @@ -21,18 +41,22 @@ export default defineConfig([ ...globals.browser, }, }, - rules: { - // An `_`-prefix is `core/`'s explicit "intentionally unused" marker — - // interface-conformance params in fakes, destructuring-rest omissions, - // and reserved-for-later args. Honor it rather than deleting signal. - "@typescript-eslint/no-unused-vars": [ - "error", - { - argsIgnorePattern: "^_", - varsIgnorePattern: "^_", - caughtErrorsIgnorePattern: "^_", - }, - ], + rules: sharedRules, + }, + { + files: [ + "test-servers/src/**/*.ts", + "vitest.shared.mts", + "eslint.config.js", + ], + extends: [js.configs.recommended, tseslint.configs.recommended], + languageOptions: { + ecmaVersion: 2022, + sourceType: "module", + globals: { + ...globals.node, + }, }, + rules: sharedRules, }, ]); diff --git a/package-lock.json b/package-lock.json index 22e5af663..903dd105e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -41,7 +41,7 @@ "@eslint/js": "^9.39.5", "eslint": "^9.39.5", "globals": "^17.7.0", - "prettier": "^3.8.4", + "prettier": "3.8.4", "typescript": "~5.9.3", "typescript-eslint": "^8.65.0" }, diff --git a/package.json b/package.json index afd8037d3..47e4549e2 100644 --- a/package.json +++ b/package.json @@ -41,13 +41,16 @@ "ci:storybook": "cd clients/web && npx playwright install chromium && npm run test:storybook", "verify:build-gate": "node scripts/verify-build-gate.mjs", "validate": "npm run validate:core && npm run validate:web && npm run validate:cli && npm run validate:tui && npm run validate:launcher", - "validate:core": "npm run format:check:core && npm run format:check:scripts && npm run lint:core", + "validate:core": "npm run format:check:core && npm run format:check:scripts && npm run format:check:shared && npm run lint:core && npm run lint:shared", "lint:core": "eslint \"core/**/*.{ts,tsx}\"", + "lint:shared": "eslint \"test-servers/src/**/*.ts\" vitest.shared.mts eslint.config.js", "format:core": "prettier --write \"core/**/*.{ts,tsx}\"", "format:check:core": "prettier --check \"core/**/*.{ts,tsx}\"", "format:scripts": "prettier --write \"scripts/**/*.{mjs,js,cjs,ts}\"", "format:check:scripts": "prettier --check \"scripts/**/*.{mjs,js,cjs,ts}\"", - "format": "npm run format:core && npm run format:scripts && cd clients/web && npm run format && cd ../cli && npm run format && cd ../tui && npm run format && cd ../launcher && npm run format", + "format:shared": "prettier --write \"test-servers/src/**/*.ts\" vitest.shared.mts eslint.config.js", + "format:check:shared": "prettier --check \"test-servers/src/**/*.ts\" vitest.shared.mts eslint.config.js", + "format": "npm run format:core && npm run format:scripts && npm run format:shared && cd clients/web && npm run format && cd ../cli && npm run format && cd ../tui && npm run format && cd ../launcher && npm run format", "validate:cli": "cd clients/cli && npm run validate", "validate:tui": "cd clients/tui && npm run validate", "validate:web": "cd clients/web && npm run validate", @@ -102,7 +105,7 @@ "@eslint/js": "^9.39.5", "eslint": "^9.39.5", "globals": "^17.7.0", - "prettier": "^3.8.1", + "prettier": "3.8.4", "typescript": "~5.9.3", "typescript-eslint": "^8.65.0" } diff --git a/test-servers/src/modern-tasks.ts b/test-servers/src/modern-tasks.ts index bc7a6d773..15531921e 100644 --- a/test-servers/src/modern-tasks.ts +++ b/test-servers/src/modern-tasks.ts @@ -41,7 +41,11 @@ const DEFAULT_POLL_INTERVAL_MS = 500; const SIMPLE_WORKING_POLLS = 2; type ModernTaskStatus = - "working" | "input_required" | "completed" | "failed" | "cancelled"; + | "working" + | "input_required" + | "completed" + | "failed" + | "cancelled"; interface ModernTaskEntry { taskId: string; diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index fff1d5652..a87311efa 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -1575,7 +1575,8 @@ export function createAddToolTool(): ToolDefinition { { description: params.description as string, inputSchema: params.inputSchema as - Record | undefined, + | Record + | undefined, }, async () => { return { @@ -1679,7 +1680,8 @@ export function createAddPromptTool(): ToolDefinition { { description: params.description as string | undefined, argsSchema: params.argsSchema as - Record | undefined, + | Record + | undefined, }, async () => { return { @@ -1823,7 +1825,9 @@ export function createSendProgressTool( // Extract progressToken from metadata const progressToken = extra?._meta?.progressToken as - string | number | undefined; + | string + | number + | undefined; // Send progress notifications let sent = 0; @@ -2264,9 +2268,12 @@ export function createTaskTool( handler: { createTask: async (args, extra) => { const message = (args as Record)?.message as - string | undefined; + | string + | undefined; const progressToken = extra._meta?.progressToken as - string | number | undefined; + | string + | number + | undefined; const task = await extra.taskStore.createTask({}); runTaskExecution({ task, diff --git a/test-servers/src/test-server-http.ts b/test-servers/src/test-server-http.ts index 692c8b4fb..bbbb47cef 100644 --- a/test-servers/src/test-server-http.ts +++ b/test-servers/src/test-server-http.ts @@ -210,7 +210,8 @@ export class TestServerHttp { private recordedRequests: RecordedRequest[] = []; private httpServer?: HttpServer; private transport?: - WebStandardStreamableHTTPServerTransport | SSEServerTransport; + | WebStandardStreamableHTTPServerTransport + | SSEServerTransport; private baseUrl?: string; private currentRequestHeaders?: Record; private currentLogLevel: string | null = null; diff --git a/vitest.shared.mts b/vitest.shared.mts index f0c25d724..060e099f2 100644 --- a/vitest.shared.mts +++ b/vitest.shared.mts @@ -4,82 +4,112 @@ * that client's node_modules. */ -import path from 'node:path'; -import { fileURLToPath } from 'node:url'; +import path from "node:path"; +import { fileURLToPath } from "node:url"; export function vitestSharedPaths(clientDir: string) { const dirname = path.resolve(clientDir); - const repoRoot = path.resolve(dirname, '../..'); + const repoRoot = path.resolve(dirname, "../.."); const sharedAliases = { - '@inspector/core': path.resolve(repoRoot, 'core'), - '@modelcontextprotocol/inspector-test-server': path.resolve( + "@inspector/core": path.resolve(repoRoot, "core"), + "@modelcontextprotocol/inspector-test-server": path.resolve( repoRoot, - 'test-servers/build/index.js', + "test-servers/build/index.js", ), }; const sharedDedupe = [ - 'react', - 'react-dom', - '@modelcontextprotocol/client', - '@modelcontextprotocol/core', + "react", + "react-dom", + "@modelcontextprotocol/client", + "@modelcontextprotocol/core", ]; const nodeModulesAliases = [ - { find: /^react$/, replacement: path.resolve(dirname, 'node_modules/react') }, + { + find: /^react$/, + replacement: path.resolve(dirname, "node_modules/react"), + }, { find: /^react\/jsx-runtime$/, - replacement: path.resolve(dirname, 'node_modules/react/jsx-runtime.js'), + replacement: path.resolve(dirname, "node_modules/react/jsx-runtime.js"), }, { find: /^react\/jsx-dev-runtime$/, - replacement: path.resolve(dirname, 'node_modules/react/jsx-dev-runtime.js'), + replacement: path.resolve( + dirname, + "node_modules/react/jsx-dev-runtime.js", + ), }, { find: /^react-dom$/, - replacement: path.resolve(dirname, 'node_modules/react-dom'), + replacement: path.resolve(dirname, "node_modules/react-dom"), }, { find: /^react-dom\/client$/, - replacement: path.resolve(dirname, 'node_modules/react-dom/client.js'), + replacement: path.resolve(dirname, "node_modules/react-dom/client.js"), }, - { find: /^pino$/, replacement: path.resolve(dirname, 'node_modules/pino') }, + { find: /^pino$/, replacement: path.resolve(dirname, "node_modules/pino") }, { find: /^pino\/browser\.js$/, - replacement: path.resolve(dirname, 'node_modules/pino/browser.js'), + replacement: path.resolve(dirname, "node_modules/pino/browser.js"), }, { find: /^hono$/, - replacement: path.resolve(dirname, 'node_modules/hono/dist/index.js'), + replacement: path.resolve(dirname, "node_modules/hono/dist/index.js"), }, { find: /^hono\/streaming$/, - replacement: path.resolve(dirname, 'node_modules/hono/dist/helper/streaming/index.js'), + replacement: path.resolve( + dirname, + "node_modules/hono/dist/helper/streaming/index.js", + ), }, { find: /^@hono\/node-server$/, - replacement: path.resolve(dirname, 'node_modules/@hono/node-server'), + replacement: path.resolve(dirname, "node_modules/@hono/node-server"), + }, + { + find: /^atomically$/, + replacement: path.resolve(dirname, "node_modules/atomically"), + }, + { + find: /^chokidar$/, + replacement: path.resolve(dirname, "node_modules/chokidar"), }, - { find: /^atomically$/, replacement: path.resolve(dirname, 'node_modules/atomically') }, - { find: /^chokidar$/, replacement: path.resolve(dirname, 'node_modules/chokidar') }, { find: /^@napi-rs\/keyring$/, - replacement: path.resolve(dirname, 'node_modules/@napi-rs/keyring'), + replacement: path.resolve(dirname, "node_modules/@napi-rs/keyring"), + }, + { + find: /^express$/, + replacement: path.resolve(dirname, "node_modules/express"), + }, + { + find: /^yaml$/, + replacement: path.resolve(repoRoot, "node_modules/yaml"), }, - { find: /^express$/, replacement: path.resolve(dirname, 'node_modules/express') }, - { find: /^yaml$/, replacement: path.resolve(repoRoot, 'node_modules/yaml') }, ]; const projectResolve = { alias: [ - ...Object.entries(sharedAliases).map(([find, replacement]) => ({ find, replacement })), + ...Object.entries(sharedAliases).map(([find, replacement]) => ({ + find, + replacement, + })), ...nodeModulesAliases, ], dedupe: sharedDedupe, }; - return { repoRoot, sharedAliases, sharedDedupe, nodeModulesAliases, projectResolve }; + return { + repoRoot, + sharedAliases, + sharedDedupe, + nodeModulesAliases, + projectResolve, + }; } /** Convenience for importers that only have import.meta.url. */ From 1ce3b64bb0c0a63e22ec0e855233d1fa77036283 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 26 Jul 2026 08:41:49 -0400 Subject: [PATCH 2/6] chore: address PR #1792 review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - #1789 gap: widen clients/web format to include .storybook and clients/ launcher format to include scripts/; reformat the 3 .storybook files (were non-conforming under the pinned prettier 3.8.4). Audited all 885 tracked source files — now fully prettier-clean. - Export VALIDATE_DEBOUNCE_MS / COLLAPSE_DELAY_MS / HIGHLIGHT_DURATION_MS from useServerJsonImport and use them in the test instead of hardcoded 300/1000/250 (so a tuned constant can't silently desync the test). - Assert the symmetric renameErrors["alpha"] alongside ["beta"] so a one-directional collision-check regression fails. - Comment the blank-rename test as exercising a UI-unreachable defensive branch (canImport gates submit). - Un-stale the two specification/v2_ux_interfaces_plan.md references to the now-removed navigation.ts InspectorTab. npm run ci green. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- clients/launcher/package.json | 4 +- clients/web/.storybook/main.ts | 15 ++-- clients/web/.storybook/preview.tsx | 74 +++++++++++-------- clients/web/.storybook/vitest.setup.ts | 6 +- clients/web/package.json | 4 +- .../src/hooks/useImportClientConfig.test.tsx | 9 ++- .../src/hooks/useServerJsonImport.test.tsx | 9 ++- clients/web/src/hooks/useServerJsonImport.ts | 6 +- specification/v2_ux_interfaces_plan.md | 4 +- 9 files changed, 74 insertions(+), 57 deletions(-) diff --git a/clients/launcher/package.json b/clients/launcher/package.json index cd5703668..ba0ffc0ef 100644 --- a/clients/launcher/package.json +++ b/clients/launcher/package.json @@ -15,8 +15,8 @@ "build": "tsc", "postbuild": "node scripts/make-executable.js", "lint": "eslint .", - "format": "prettier --write src __tests__ \"*.{ts,js}\"", - "format:check": "prettier --check src __tests__ \"*.{ts,js}\"", + "format": "prettier --write src __tests__ scripts \"*.{ts,js}\"", + "format:check": "prettier --check src __tests__ scripts \"*.{ts,js}\"", "test": "vitest run", "test:coverage": "vitest run --coverage", "validate": "npm run format:check && npm run lint && npm run build && npm run test" diff --git a/clients/web/.storybook/main.ts b/clients/web/.storybook/main.ts index 3f89d8e61..69b7bb220 100644 --- a/clients/web/.storybook/main.ts +++ b/clients/web/.storybook/main.ts @@ -1,18 +1,15 @@ -import type { StorybookConfig } from '@storybook/react-vite'; +import type { StorybookConfig } from "@storybook/react-vite"; const config: StorybookConfig = { - "stories": [ - "../src/**/*.mdx", - "../src/**/*.stories.@(js|jsx|mjs|ts|tsx)" - ], - "addons": [ + stories: ["../src/**/*.mdx", "../src/**/*.stories.@(js|jsx|mjs|ts|tsx)"], + addons: [ "@chromatic-com/storybook", "@storybook/addon-vitest", "@storybook/addon-a11y", "@storybook/addon-docs", - "@storybook/addon-onboarding" + "@storybook/addon-onboarding", ], - "framework": "@storybook/react-vite", + framework: "@storybook/react-vite", previewHead: (head) => ` ${head} @@ -20,4 +17,4 @@ const config: StorybookConfig = { `, }; -export default config; \ No newline at end of file +export default config; diff --git a/clients/web/.storybook/preview.tsx b/clients/web/.storybook/preview.tsx index a17e18d82..6b419a436 100644 --- a/clients/web/.storybook/preview.tsx +++ b/clients/web/.storybook/preview.tsx @@ -1,64 +1,74 @@ -import type { Preview } from '@storybook/react-vite' -import { MantineProvider, useMantineColorScheme, type CSSVariablesResolver } from '@mantine/core' -import { Notifications } from '@mantine/notifications' -import '@mantine/core/styles.css' -import '@mantine/notifications/styles.css' -import '../src/App.css' -import { theme } from '../src/theme/theme' -import { useEffect } from 'react' +import type { Preview } from "@storybook/react-vite"; +import { + MantineProvider, + useMantineColorScheme, + type CSSVariablesResolver, +} from "@mantine/core"; +import { Notifications } from "@mantine/notifications"; +import "@mantine/core/styles.css"; +import "@mantine/notifications/styles.css"; +import "../src/App.css"; +import { theme } from "../src/theme/theme"; +import { useEffect } from "react"; // eslint-disable-next-line react-refresh/only-export-components function ColorSchemeWrapper({ colorScheme, children, }: { - colorScheme: 'light' | 'dark' - children: React.ReactNode + colorScheme: "light" | "dark"; + children: React.ReactNode; }) { - const { setColorScheme } = useMantineColorScheme() + const { setColorScheme } = useMantineColorScheme(); useEffect(() => { - setColorScheme(colorScheme) - }, [colorScheme, setColorScheme]) + setColorScheme(colorScheme); + }, [colorScheme, setColorScheme]); - return <>{children} + return <>{children}; } const resolver: CSSVariablesResolver = () => ({ variables: {}, light: {}, dark: { - '--mantine-color-body': 'var(--mantine-color-dark-9)', + "--mantine-color-body": "var(--mantine-color-dark-9)", }, -}) +}); const preview: Preview = { globalTypes: { colorScheme: { - description: 'Mantine color scheme', + description: "Mantine color scheme", toolbar: { - title: 'Color Scheme', - icon: 'mirror', + title: "Color Scheme", + icon: "mirror", items: [ - { value: 'light', title: 'Light', icon: 'sun' }, - { value: 'dark', title: 'Dark', icon: 'moon' }, + { value: "light", title: "Light", icon: "sun" }, + { value: "dark", title: "Dark", icon: "moon" }, ], dynamicTitle: true, }, }, }, initialGlobals: { - colorScheme: 'light', + colorScheme: "light", }, decorators: [ (Story, context) => { - const isFullscreen = context.parameters?.layout === 'fullscreen' + const isFullscreen = context.parameters?.layout === "fullscreen"; return ( - - + + {isFullscreen ? ( -
+
) : ( @@ -66,7 +76,7 @@ const preview: Preview = { )} - ) + ); }, ], parameters: { @@ -80,7 +90,7 @@ const preview: Preview = { // `'error'` fails the Storybook play-function tests (part of `npm run ci`) // on any axe violation, so the zero-violation state this PR reached is // enforced going forward rather than being a point-in-time result. - test: 'error', + test: "error", // Stories render presentational components (and whole screens) in // isolation, outside the app's `AppShell` — which is what provides the // page landmarks (`
`, nav) in the real app. The `region` rule @@ -90,11 +100,11 @@ const preview: Preview = { // by the components under test. Disable it here so the a11y panel surfaces // only rules the components can actually own. config: { - rules: [{ id: 'region', enabled: false }], + rules: [{ id: "region", enabled: false }], }, }, - layout: 'centered', + layout: "centered", }, -} +}; -export default preview +export default preview; diff --git a/clients/web/.storybook/vitest.setup.ts b/clients/web/.storybook/vitest.setup.ts index 44922d55e..fd7ac45e5 100644 --- a/clients/web/.storybook/vitest.setup.ts +++ b/clients/web/.storybook/vitest.setup.ts @@ -1,7 +1,7 @@ import * as a11yAddonAnnotations from "@storybook/addon-a11y/preview"; -import { setProjectAnnotations } from '@storybook/react-vite'; -import * as projectAnnotations from './preview'; +import { setProjectAnnotations } from "@storybook/react-vite"; +import * as projectAnnotations from "./preview"; // This is an important step to apply the right configuration when testing your stories. // More info at: https://storybook.js.org/docs/api/portable-stories/portable-stories-vitest#setprojectannotations -setProjectAnnotations([a11yAddonAnnotations, projectAnnotations]); \ No newline at end of file +setProjectAnnotations([a11yAddonAnnotations, projectAnnotations]); diff --git a/clients/web/package.json b/clients/web/package.json index be99aad26..d2209e278 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -26,8 +26,8 @@ "test:integration:watch": "npm run test-servers:build && vitest --project=integration", "test-servers:build": "tsc -p ../../test-servers --noCheck", "lint": "eslint .", - "format": "prettier --write src server \"*.{ts,js}\"", - "format:check": "prettier --check src server \"*.{ts,js}\"", + "format": "prettier --write src server .storybook \"*.{ts,js}\"", + "format:check": "prettier --check src server .storybook \"*.{ts,js}\"", "preview": "vite preview", "storybook": "storybook dev -p 6006" }, diff --git a/clients/web/src/hooks/useImportClientConfig.test.tsx b/clients/web/src/hooks/useImportClientConfig.test.tsx index b406d111c..9900f283e 100644 --- a/clients/web/src/hooks/useImportClientConfig.test.tsx +++ b/clients/web/src/hooks/useImportClientConfig.test.tsx @@ -276,7 +276,10 @@ describe("useImportClientConfig", () => { act(() => result.current.setResolution("beta", "rename")); act(() => result.current.setRenameTo("alpha", "shared")); act(() => result.current.setRenameTo("beta", "shared")); - // Each rename sees the other's target already claimed. + // The collision check is symmetric — each rename sees the other's target + // already claimed, so both must be flagged (asserting only one would pass + // even if the check regressed to one-directional). + expect(result.current.renameErrors["alpha"]).toContain("already in use"); expect(result.current.renameErrors["beta"]).toContain("already in use"); expect(result.current.canImport).toBe(false); }); @@ -293,6 +296,10 @@ describe("useImportClientConfig", () => { }); act(() => result.current.setResolution("alpha", "rename")); act(() => result.current.setRenameTo("alpha", " ")); + // A blank rename also sets renameErrors, so canImport is false and the real + // UI blocks submit here — this exercises the defensive fallback at + // useImportClientConfig.ts (`res.renameTo.trim() || conflict.id`) + // programmatically, which the UI can't reach. await act(async () => { await result.current.runImport(); }); diff --git a/clients/web/src/hooks/useServerJsonImport.test.tsx b/clients/web/src/hooks/useServerJsonImport.test.tsx index 5a7b04f03..a310a2d84 100644 --- a/clients/web/src/hooks/useServerJsonImport.test.tsx +++ b/clients/web/src/hooks/useServerJsonImport.test.tsx @@ -2,6 +2,9 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { renderHook, act } from "@testing-library/react"; import { useServerJsonImport, + VALIDATE_DEBOUNCE_MS, + COLLAPSE_DELAY_MS, + HIGHLIGHT_DURATION_MS, type UseServerJsonImportOptions, } from "./useServerJsonImport"; @@ -44,7 +47,7 @@ function typeAndFlush( ) { act(() => result.current.setRawText(content)); act(() => { - vi.advanceTimersByTime(300); + vi.advanceTimersByTime(VALIDATE_DEBOUNCE_MS); }); } @@ -145,11 +148,11 @@ describe("useServerJsonImport", () => { act(() => result.current.setRawText(VALID_SERVER_JSON)); expect(result.current.fileContentsOpen).toBe(true); act(() => { - vi.advanceTimersByTime(1000); + vi.advanceTimersByTime(COLLAPSE_DELAY_MS); }); expect(result.current.fileContentsHighlight).toBe(true); act(() => { - vi.advanceTimersByTime(250); + vi.advanceTimersByTime(HIGHLIGHT_DURATION_MS); }); expect(result.current.fileContentsHighlight).toBe(false); expect(result.current.fileContentsOpen).toBe(false); diff --git a/clients/web/src/hooks/useServerJsonImport.ts b/clients/web/src/hooks/useServerJsonImport.ts index 6736ec6cf..5692140e3 100644 --- a/clients/web/src/hooks/useServerJsonImport.ts +++ b/clients/web/src/hooks/useServerJsonImport.ts @@ -16,19 +16,19 @@ import type { } from "../components/groups/ImportServerJsonPanel/ImportServerJsonPanel"; /** Debounce (ms) before a textarea edit re-triggers parse/validation. */ -const VALIDATE_DEBOUNCE_MS = 300; +export const VALIDATE_DEBOUNCE_MS = 300; /** * Delay (ms) after content is loaded/pasted before the File Contents disclosure * auto-collapses — long enough to read as "the content was accepted". */ -const COLLAPSE_DELAY_MS = 1000; +export const COLLAPSE_DELAY_MS = 1000; /** * How long (ms) to flash the disclosure with its hover background just before it * collapses, so the collapse reads as intentional rather than abrupt. */ -const HIGHLIGHT_DURATION_MS = 250; +export const HIGHLIGHT_DURATION_MS = 250; const EMPTY_DRAFT: InspectorServerJsonDraft = { rawText: "", diff --git a/specification/v2_ux_interfaces_plan.md b/specification/v2_ux_interfaces_plan.md index 9041de277..c9dd7431f 100644 --- a/specification/v2_ux_interfaces_plan.md +++ b/specification/v2_ux_interfaces_plan.md @@ -82,7 +82,7 @@ and should be replaced verbatim during the refactor: | `InspectorServerSettings` | Closest v1.5 analog is `InspectorClientOptions` (timeouts, OAuth, sampling/elicit/roots flags). Settings form should accept the relevant subset. | `core/mcp/types.ts` | | `InspectorRequestHistoryItem` | Subset of `MessageEntry` filtered to outbound requests. No new type needed. | `core/mcp/types.ts` | | `InspectorUrlElicitRequest` | URL-mode elicitation IS supported by v1.5 via `InspectorClientOptions.elicit: { url: true }`; the request/response shape lives in `core/mcp/elicitationCreateMessage.ts`. v2 should mirror that file's types rather than invent. | `core/mcp/elicitationCreateMessage.ts` | -| `InspectorTab` | No v1.5 analog — v2-only enum. Place under `clients/web/src/types/` or alongside `ViewHeader`. | new (v2-only) | +| `InspectorTab` | No v1.5 analog — v2-only enum. Superseded by `InspectorTabId` in `clients/web/src/utils/inspectorTabs.ts` (#1785). | new (v2-only) | ## Non-goals @@ -181,7 +181,7 @@ have no v1.5 equivalent): **Tab enum placement**: `InspectorTab` is a UI-routing concept, not an MCP or transport concept. Place it under `clients/web/src/types/navigation.ts` rather than in `core/mcp/types.ts`. Everything else lives in `core/mcp/types.ts`. -_(Superseded by [#1785](https://github.com/modelcontextprotocol/inspector/issues/1785): the live tab-id type is `InspectorTabId` in `clients/web/src/utils/inspectorTabs.ts`, and the `navigation.ts` `InspectorTab` here is dead and slated for removal — a pure UI domain type belongs in `utils/`, per the `src/lib` vs `src/utils` rule in AGENTS.md.)_ +_(Superseded by [#1785](https://github.com/modelcontextprotocol/inspector/issues/1785): the live tab-id type is `InspectorTabId` in `clients/web/src/utils/inspectorTabs.ts`, and the dead `navigation.ts` `InspectorTab` was removed — a pure UI domain type belongs in `utils/`, not `src/types/`, per the `src/lib` vs `src/utils` rule in AGENTS.md.)_ **Custom headers**: `clients/web/src/utils/customHeaders.ts` is a verbatim copy of the v1.5 module. It owns `CustomHeader` / `CustomHeaders` shape used by From b4d04240147d0d32b4de440684a753a971ae6b65 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 26 Jul 2026 08:46:36 -0400 Subject: [PATCH 3/6] docs: sync AGENTS.md format-surface description with the widened globs (#1792 review r2) 1ce3b64 added .storybook to clients/web's format glob and scripts/ to clients/launcher's, but AGENTS.md:222 still described the pre-fix scope. Bring it in line and note the non-recursive top-level glob is why nested dirs are named explicitly. Docs only. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 47421ef1f..eaf3e57d8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -219,7 +219,7 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs the **`core/` gate first** (`validate:core`) and then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). - **`validate:core` is the root-owned format + lint gate (#1689, widened in #1778 and #1767).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/`, the root `scripts/`, or the root "shared" surface before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `format:check:scripts` (`prettier --check "scripts/**/*.{mjs,js,cjs,ts}"`, the root build/verify tooling — #1778; a brace glob so a future non-`.mjs` script is auto-included) + `format:check:shared` + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`) + `lint:shared`. Use `npm run format:core` / `npm run format:scripts` / `npm run format:shared` to auto-fix (all folded into the root `format`). The **shared surface** (#1767) is `test-servers/src/**/*.ts`, the root `vitest.shared.mts`, and the root `eslint.config.js` — first-party code no client's `eslint .` / `prettier` reaches; it is both prettier-gated (`format:check:shared`) and eslint-gated (`lint:shared`, via a second `files` block in the root `eslint.config.js` scoped to Node globals). The `scripts/` gate is prettier-only — the root has no eslint config for `.mjs`. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). **prettier is pinned to an exact version** (not a caret) in all five `package.json`s (#1790) so the gate's verdict can't shift with an in-range patch bump. - **cli and tui now typecheck their `src` (#1689).** Their `build`/`test` run through esbuild (no type check), so each has a `typecheck` script (`tsc --noEmit -p tsconfig.json`) folded into `validate`. Their `tsconfig.json` matches `clients/web/tsconfig.app.json`'s module/lib *resolution* options — DOM lib, `moduleResolution: bundler`, and **no** `noUncheckedIndexedAccess` (web's app config does not extend `tsconfig.base`, so re-enabling it would surface `core/` issues web never gates) — so the imported `core/` sources are validated the same way web validates them. It does **not** mirror web's extra strictness flags (`noUnusedLocals`, `verbatimModuleSyntax`, ES2023 target, …), so cli/tui's own `src` is checked slightly more loosely than web's. `core/` itself still typechecks through web's `tsc -b`. - - The one CLI nuance: `clients/cli`'s out-of-process `e2e.test.ts` spawns the built binary, so its `test` **builds first** via `pretest` (`test-servers:build && build`). To avoid building it twice, `clients/cli`'s `validate` folds that in — it is `format:check && lint && typecheck && test` with **no** separate `build` step (the other clients, whose tests don't spawn their bundle, keep an explicit `build`). `validate:web`/`validate:tui`/`validate:launcher` are the uniform `format:check && lint && (typecheck &&) build && test`. (#1778) `clients/web`'s `format`/`format:check` covers `src`, `server`, and its top-level configs (`*.{ts,js}` — `vite.config.ts`, `tsup.runner.config.ts`, `eslint.config.js`, …), not just `src`, so the Node backend and Vite/build config are prettier-gated too. + - The one CLI nuance: `clients/cli`'s out-of-process `e2e.test.ts` spawns the built binary, so its `test` **builds first** via `pretest` (`test-servers:build && build`). To avoid building it twice, `clients/cli`'s `validate` folds that in — it is `format:check && lint && typecheck && test` with **no** separate `build` step (the other clients, whose tests don't spawn their bundle, keep an explicit `build`). `validate:web`/`validate:tui`/`validate:launcher` are the uniform `format:check && lint && (typecheck &&) build && test`. (#1778, #1789) `clients/web`'s `format`/`format:check` covers `src`, `server`, `.storybook`, and its top-level configs (`*.{ts,js}` — `vite.config.ts`, `tsup.runner.config.ts`, `eslint.config.js`, …), not just `src`, so the Node backend, Storybook config, and Vite/build config are prettier-gated too; `clients/launcher`'s covers `src`, `__tests__`, `scripts`, and its top-level configs (the `*.` top-level glob is non-recursive, so each nested dir — `.storybook`, `scripts` — is named explicitly). - **`npm run coverage`** is the per-file ≥90 gate and is now part of `npm run ci` — never treat it as optional before a push. It supersedes the old standalone `test:integration` step: web's `test:coverage` runs the `unit` **and** `integration` projects under v8 instrumentation, so `coverage` both enforces the ≥90 gate and exercises the same web integration paths CI covers. - **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `verify:build-gate` (the #1769 build gate — see below), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). GitHub CI runs this same chain as separate workflow steps, with the Storybook play-function tests last (see below). - `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below. From 55ad7ab388931ebc246bfffccf67f0d1c8e339c3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 26 Jul 2026 09:23:07 -0400 Subject: [PATCH 4/6] chore: handle PR #1792 round-3 follow-ups in-place (format-coverage guard, uniform globs, theme gate) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rather than defer these to tickets, fold them into this PR: - Obs 2: widen every client + root format glob to a uniform *.{ts,tsx,mts,cts,js,jsx,mjs,cjs} so a future .mts/.cjs/.tsx top-level file can't silently slip the gate. - Obs 1: add scripts/verify-format-coverage.mjs (wired into `validate`) — it parses the format:check globs out of every package.json, enumerates all tracked source files, and fails listing any not covered by a glob. The durable, standing version of #1789's one-shot audit; negative-tested. - Obs 3: bring src/theme/** under the coverage gate (21/23 files were already ≥90 transitively; add Paper.test.tsx for the one laggard, Paper.ts). src/App.tsx (~4.5k lines, ~42% branches) is converted from a silent omission into an explicit, documented exclusion (comment on the coverage `include` + AGENTS.md) — gating it is a dedicated effort, not a whitelist tweak. npm run ci green (the new guard reports 885/885 tracked source files gated). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- AGENTS.md | 2 +- clients/cli/package.json | 4 +- clients/launcher/package.json | 4 +- clients/tui/package.json | 4 +- clients/web/package.json | 4 +- clients/web/src/theme/Paper.test.tsx | 34 ++++++ clients/web/vite.config.ts | 7 ++ package.json | 13 ++- scripts/verify-format-coverage.mjs | 168 +++++++++++++++++++++++++++ 9 files changed, 225 insertions(+), 15 deletions(-) create mode 100644 clients/web/src/theme/Paper.test.tsx create mode 100644 scripts/verify-format-coverage.mjs diff --git a/AGENTS.md b/AGENTS.md index eaf3e57d8..578efa24b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -255,7 +255,7 @@ The top-level **`src/types/`** is a sibling of both and is not the place for new Cross-directory imports point **one way, `lib → utils`** (infra depends on pure helpers, never the reverse). Keep it that way: if a `utils/` module needs a type currently exported from a `lib/` module, declare the type in `utils/` and re-export it from `lib/` (as `pendingReauth` owns `OAuthResumeAuthKind` and `oauthResume` re-exports it), rather than importing "up" from `utils` into `lib`. -Nothing **enforces** the boundary: no path alias keys off it, and the coverage `include` in `clients/web/vite.config.ts` lists **both** `src/lib/**` and `src/utils/**`, so a move between them is coverage-neutral (this is why the refactor was gate-safe). It's a human-legible signal at import time, valuable in a codebase this test-heavy (the ≥90% per-file gate). Note that `include` is a **whitelist** — it names `components`/`hooks`/`lib`/`utils`/`server` (plus the `core/*` runtime; `hooks` was added in #1787), so a module placed **outside** those directories (`theme/`, `types/`, `App.tsx`, or a brand-new grab-bag) falls out of the ≥90 gate entirely, silently. When adding a module, place it by the rule and keep it inside a gated directory; when it genuinely mixes both (e.g. `downloadFile` bundles DOM-side-effect helpers with a couple of pure ones), keep it whole on its dominant side (`lib`) rather than splitting hairs. +Nothing **enforces** the boundary: no path alias keys off it, and the coverage `include` in `clients/web/vite.config.ts` lists **both** `src/lib/**` and `src/utils/**`, so a move between them is coverage-neutral (this is why the refactor was gate-safe). It's a human-legible signal at import time, valuable in a codebase this test-heavy (the ≥90% per-file gate). Note that `include` is a **whitelist** — it names `components`/`hooks`/`theme`/`lib`/`utils`/`server` (plus the `core/*` runtime; `hooks` and `theme` were added in #1787), so a module placed **outside** those directories (`types/`, `App.tsx`, or a brand-new grab-bag) falls out of the ≥90 gate entirely, silently. The one **deliberate, documented** exception is `src/App.tsx` — a ~4.5k-line composition root at ~42% branch coverage, whose omission is called out in a comment on the `include` array itself (gating it is a dedicated testing/decomposition effort, not a whitelist tweak) rather than left silent. When adding a module, place it by the rule and keep it inside a gated directory; when it genuinely mixes both (e.g. `downloadFile` bundles DOM-side-effect helpers with a couple of pure ones), keep it whole on its dominant side (`lib`) rather than splitting hairs. ## React instructions - UI Components diff --git a/clients/cli/package.json b/clients/cli/package.json index 57bbe4f18..b16990e31 100644 --- a/clients/cli/package.json +++ b/clients/cli/package.json @@ -28,8 +28,8 @@ "test:cli-metadata": "vitest run metadata.test.ts", "pretest": "npm run test-servers:build && npm run build", "lint": "eslint .", - "format": "prettier --write src __tests__ \"*.{ts,js}\"", - "format:check": "prettier --check src __tests__ \"*.{ts,js}\"" + "format": "prettier --write src __tests__ \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"", + "format:check": "prettier --check src __tests__ \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"" }, "dependencies": { "@modelcontextprotocol/client": "2.0.0-beta.5", diff --git a/clients/launcher/package.json b/clients/launcher/package.json index ba0ffc0ef..f0602565a 100644 --- a/clients/launcher/package.json +++ b/clients/launcher/package.json @@ -15,8 +15,8 @@ "build": "tsc", "postbuild": "node scripts/make-executable.js", "lint": "eslint .", - "format": "prettier --write src __tests__ scripts \"*.{ts,js}\"", - "format:check": "prettier --check src __tests__ scripts \"*.{ts,js}\"", + "format": "prettier --write src __tests__ scripts \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"", + "format:check": "prettier --check src __tests__ scripts \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"", "test": "vitest run", "test:coverage": "vitest run --coverage", "validate": "npm run format:check && npm run lint && npm run build && npm run test" diff --git a/clients/tui/package.json b/clients/tui/package.json index 9d52b0c44..005801f47 100644 --- a/clients/tui/package.json +++ b/clients/tui/package.json @@ -23,8 +23,8 @@ "test": "vitest run", "test:coverage": "vitest run --coverage", "lint": "eslint .", - "format": "prettier --write src __tests__ \"*.{ts,tsx,js}\"", - "format:check": "prettier --check src __tests__ \"*.{ts,tsx,js}\"" + "format": "prettier --write src __tests__ \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"", + "format:check": "prettier --check src __tests__ \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"" }, "dependencies": { "@modelcontextprotocol/client": "2.0.0-beta.5", diff --git a/clients/web/package.json b/clients/web/package.json index d2209e278..9be68f0a5 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -26,8 +26,8 @@ "test:integration:watch": "npm run test-servers:build && vitest --project=integration", "test-servers:build": "tsc -p ../../test-servers --noCheck", "lint": "eslint .", - "format": "prettier --write src server .storybook \"*.{ts,js}\"", - "format:check": "prettier --check src server .storybook \"*.{ts,js}\"", + "format": "prettier --write src server .storybook \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"", + "format:check": "prettier --check src server .storybook \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"", "preview": "vite preview", "storybook": "storybook dev -p 6006" }, diff --git a/clients/web/src/theme/Paper.test.tsx b/clients/web/src/theme/Paper.test.tsx new file mode 100644 index 000000000..e1ccd19d7 --- /dev/null +++ b/clients/web/src/theme/Paper.test.tsx @@ -0,0 +1,34 @@ +import { describe, it, expect } from "vitest"; +import { Paper } from "@mantine/core"; +import { renderWithMantine } from "../test/renderWithMantine"; + +// The theme's ThemePaper.extend is applied transitively when components render a +// Paper, but no single component exercises every variant — the `code`, +// `contained`, and `panel` branches of its classNames/styles callbacks were +// uncovered. Render each variant (plus the default) under the project theme so +// both callbacks run for every branch. See #1787 (theme/** brought under the +// coverage gate). + +const VARIANTS = ["code", "contained", "panel"] as const; + +describe("ThemePaper variants", () => { + it("renders the default variant (no variant-specific styles)", () => { + const { getByText } = renderWithMantine(default); + expect(getByText("default")).toBeTruthy(); + }); + + for (const variant of VARIANTS) { + it(`renders the "${variant}" variant`, () => { + const { getByText } = renderWithMantine( + {variant}, + ); + expect(getByText(variant)).toBeTruthy(); + }); + } + + it('applies the paper-code class only to the "code" variant', () => { + const { getByText } = renderWithMantine(c); + // classNames returns { root: "paper-code" } for the code variant. + expect(getByText("c").className).toContain("paper-code"); + }); +}); diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index 668e651f4..749dd3e85 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -167,9 +167,16 @@ export default defineConfig(({ command }) => { coverage: { provider: "v8", reporter: ["text", "html", "json-summary"], + // Whitelist of gated directories. NOTE: `src/App.tsx` is deliberately + // NOT listed — it is a ~4.5k-line composition root at ~42% branch + // coverage, so gating it is a dedicated testing/decomposition effort, + // not a whitelist tweak. Its omission is intentional and documented + // here (and in AGENTS.md) rather than silent. Every *other* src + // directory is gated; add new ones here as they appear. include: [ "src/components/**/*.{ts,tsx}", "src/hooks/**/*.{ts,tsx}", + "src/theme/**/*.{ts,tsx}", "src/lib/**/*.{ts,tsx}", "src/utils/**/*.{ts,tsx}", "clients/web/server/**/*.{ts,tsx}", diff --git a/package.json b/package.json index 47e4549e2..dcd710d5f 100644 --- a/package.json +++ b/package.json @@ -40,16 +40,17 @@ "ci": "npm run validate && npm run coverage && npm run verify:build-gate && npm run smoke && npm run ci:storybook", "ci:storybook": "cd clients/web && npx playwright install chromium && npm run test:storybook", "verify:build-gate": "node scripts/verify-build-gate.mjs", - "validate": "npm run validate:core && npm run validate:web && npm run validate:cli && npm run validate:tui && npm run validate:launcher", + "validate": "npm run verify:format-coverage && npm run validate:core && npm run validate:web && npm run validate:cli && npm run validate:tui && npm run validate:launcher", + "verify:format-coverage": "node scripts/verify-format-coverage.mjs", "validate:core": "npm run format:check:core && npm run format:check:scripts && npm run format:check:shared && npm run lint:core && npm run lint:shared", "lint:core": "eslint \"core/**/*.{ts,tsx}\"", - "lint:shared": "eslint \"test-servers/src/**/*.ts\" vitest.shared.mts eslint.config.js", + "lint:shared": "eslint \"test-servers/src/**/*.{ts,tsx,mts,cts}\" vitest.shared.mts eslint.config.js", "format:core": "prettier --write \"core/**/*.{ts,tsx}\"", "format:check:core": "prettier --check \"core/**/*.{ts,tsx}\"", - "format:scripts": "prettier --write \"scripts/**/*.{mjs,js,cjs,ts}\"", - "format:check:scripts": "prettier --check \"scripts/**/*.{mjs,js,cjs,ts}\"", - "format:shared": "prettier --write \"test-servers/src/**/*.ts\" vitest.shared.mts eslint.config.js", - "format:check:shared": "prettier --check \"test-servers/src/**/*.ts\" vitest.shared.mts eslint.config.js", + "format:scripts": "prettier --write \"scripts/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"", + "format:check:scripts": "prettier --check \"scripts/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"", + "format:shared": "prettier --write \"test-servers/src/**/*.{ts,tsx,mts,cts}\" vitest.shared.mts eslint.config.js", + "format:check:shared": "prettier --check \"test-servers/src/**/*.{ts,tsx,mts,cts}\" vitest.shared.mts eslint.config.js", "format": "npm run format:core && npm run format:scripts && npm run format:shared && cd clients/web && npm run format && cd ../cli && npm run format && cd ../tui && npm run format && cd ../launcher && npm run format", "validate:cli": "cd clients/cli && npm run validate", "validate:tui": "cd clients/tui && npm run validate", diff --git a/scripts/verify-format-coverage.mjs b/scripts/verify-format-coverage.mjs new file mode 100644 index 000000000..0376c1f43 --- /dev/null +++ b/scripts/verify-format-coverage.mjs @@ -0,0 +1,168 @@ +#!/usr/bin/env node +// Durable guard for the "every first-party source file is format-gated" invariant +// that #1789 established by a one-shot manual audit (PR #1792). A prettier +// `format`/`format:check` glob that stops covering a file fails silently — the +// file is simply skipped, not reported — so a nested dir (`.storybook/`, a +// client `scripts/`) or a new extension can re-open the gap unnoticed. This is +// the format-coverage analog of `verify:build-gate`: it enumerates every tracked +// source file and asserts each is matched by at least one `prettier --check` +// glob declared across the repo's `package.json`s. Exits non-zero, listing the +// offenders, on any miss. +// +// Source of truth is the `format:check*` scripts themselves — this parser reads +// the globs out of them, so widening/narrowing a glob is reflected here with no +// second list to keep in sync. + +import { readFileSync } from "node:fs"; +import { execFileSync } from "node:child_process"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "..", +); + +// File extensions prettier formats as first-party source here. Kept in sync with +// the union of the format globs; a file with one of these extensions that no +// glob matches is the failure this guard exists to catch. +const SOURCE_EXTENSIONS = [ + "ts", + "tsx", + "mts", + "cts", + "js", + "jsx", + "mjs", + "cjs", +]; + +// The `package.json`s whose `format:check*` scripts define the gate. The root +// carries the split `format:check:core|scripts|shared`; each client carries a +// single `format:check`. Paths are relative to the repo root; each glob resolves +// relative to its manifest's directory (prettier's cwd for that script). +const MANIFESTS = [ + ".", + "clients/web", + "clients/cli", + "clients/tui", + "clients/launcher", +]; + +/** Split a shell-ish command string into args, honoring double quotes. */ +function tokenize(command) { + const tokens = []; + const re = /"([^"]*)"|(\S+)/g; + let m; + while ((m = re.exec(command)) !== null) { + tokens.push(m[1] !== undefined ? m[1] : m[2]); + } + return tokens; +} + +/** Extract the path/glob args from every `prettier --check …` in a manifest. */ +function prettierCheckArgs(scripts) { + const args = []; + for (const value of Object.values(scripts ?? {})) { + if (typeof value !== "string" || !value.includes("prettier --check")) + continue; + // A manifest may chain `prettier --check …` inside a larger script; take the + // segment starting at each occurrence up to the next `&&`/`||`/`;`. + for (const segment of value.split(/&&|\|\||;/)) { + const trimmed = segment.trim(); + if (!trimmed.startsWith("prettier --check")) continue; + const tokens = tokenize(trimmed).slice(2); // drop `prettier` `--check` + for (const t of tokens) { + if (t.startsWith("-")) continue; // a flag, not a path + args.push(t); + } + } + } + return args; +} + +const GLOB_CHARS = /[*?{}[\]]/; + +/** Convert a prettier glob to an anchored RegExp over repo-relative POSIX paths. */ +function globToRegExp(glob) { + let re = ""; + for (let i = 0; i < glob.length; i++) { + const c = glob[i]; + if (c === "*") { + if (glob[i + 1] === "*") { + // `**` (optionally `**/`) crosses path separators. + if (glob[i + 2] === "/") { + re += "(?:.*/)?"; + i += 2; + } else { + re += ".*"; + i += 1; + } + } else { + re += "[^/]*"; // `*` stays within a segment + } + } else if (c === "?") { + re += "[^/]"; + } else if (c === "{") { + re += "(?:"; + } else if (c === "}") { + re += ")"; + } else if (c === ",") { + re += "|"; + } else if (".+^$()|\\/".includes(c)) { + re += "\\" + c; + } else { + re += c; + } + } + return new RegExp("^" + re + "$"); +} + +/** Build the set of coverage predicates from all manifests' format globs. */ +function buildMatchers() { + const matchers = []; + for (const manifestDir of MANIFESTS) { + const manifestPath = path.join(repoRoot, manifestDir, "package.json"); + const pkg = JSON.parse(readFileSync(manifestPath, "utf8")); + for (const arg of prettierCheckArgs(pkg.scripts)) { + const rel = manifestDir === "." ? arg : path.posix.join(manifestDir, arg); + if (GLOB_CHARS.test(arg)) { + const re = globToRegExp(rel); + matchers.push((f) => re.test(f)); + } else { + // A bare path: a directory prettier recurses into, or an exact file. + matchers.push((f) => f === rel || f.startsWith(rel + "/")); + } + } + } + return matchers; +} + +function trackedSourceFiles() { + const out = execFileSync( + "git", + ["ls-files", ...SOURCE_EXTENSIONS.map((e) => `*.${e}`)], + { cwd: repoRoot, encoding: "utf8" }, + ); + return out.split("\n").filter(Boolean); +} + +const matchers = buildMatchers(); +const files = trackedSourceFiles(); +const ungated = files.filter((f) => !matchers.some((m) => m(f))); + +if (ungated.length > 0) { + console.error( + `verify:format-coverage — ${ungated.length} tracked source file(s) are not covered by any prettier format glob:\n`, + ); + for (const f of ungated) console.error(" " + f); + console.error( + "\nAdd the file's directory (or a matching glob) to the relevant `format`/`format:check` script,", + ); + console.error("or widen the extension set. See AGENTS.md."); + process.exit(1); +} + +console.log( + `verify:format-coverage — OK: all ${files.length} tracked source files are format-gated.`, +); From 0fa0f1bfe24f15ff6b3513fc91e9203f8742c981 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 26 Jul 2026 09:36:50 -0400 Subject: [PATCH 5/6] chore: address PR #1792 round-4 findings (guard hardening, docs, exclusion list) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Harden verify-format-coverage: only harvest format:check globs from scripts transitively reachable from `validate` (via npm run refs), so an unwired/renamed format:check no longer counts as coverage. Negative-tested — renaming clients/web format:check to an unreferenced name now fails the guard (627 files flagged) instead of passing. - Name src/main.tsx and src/index.ts (browser + bin bootstraps) alongside src/App.tsx in the coverage-include comment and AGENTS.md, so the documented exclusion list is complete rather than claiming App.tsx is the only one. - Doc sync: document verify:format-coverage (AGENTS.md + README script table + scripts/ layout line), fix the `validate` ordering (verify:format- coverage now runs before validate:core) in AGENTS.md and README, and update the quoted globs to the uniform *.{ts,tsx,mts,cts,js,jsx,mjs,cjs} / test-servers *.{ts,tsx,mts,cts}. npm run ci green. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- AGENTS.md | 10 ++++----- README.md | 5 +++-- clients/web/vite.config.ts | 16 +++++++++------ scripts/verify-format-coverage.mjs | 33 ++++++++++++++++++++++++++++-- 4 files changed, 49 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 578efa24b..d09ba33bf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -214,12 +214,12 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - after making the changes, respond to each review comment with what was done (or why it was ignored) ### Mandatory pre-push gate -- ALWAYS do `npm run format` before committing — the **root** `format` auto-fixes `core/` (`format:core`), the root `scripts/**/*.{mjs,js,cjs,ts}` (`format:scripts`), the root "shared" surface (`format:shared` — `test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`), and every client's scope in one shot. `validate` runs `format:check` (the non-fixing variant, including `format:check:core`, `format:check:scripts`, and `format:check:shared`) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it. +- ALWAYS do `npm run format` before committing — the **root** `format` auto-fixes `core/` (`format:core`), the root `scripts/` tooling (`format:scripts`), the root "shared" surface (`format:shared` — `test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`), and every client's scope in one shot. Every format glob uses the uniform extension set `*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` (#1792) so a new-extension file can't slip the gate, and `npm run verify:format-coverage` (the first step of `validate`, #1792) fails if any tracked source file is left uncovered by a `format:check` glob. `validate` runs `format:check` (the non-fixing variant, including `format:check:core`, `format:check:scripts`, and `format:check:shared`) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it. - **`npm run ci` is the mandatory pre-push command** — it mirrors `.github/workflows/main.yml` (minus `npm install`): `validate` → `coverage` → `verify:build-gate` (the #1769 browser-externalized-builtin build gate) → `smoke` → Storybook play-function tests (installs Playwright chromium if needed). It now runs **`npm run coverage`**, the per-file ≥90 gate (lines/statements/functions/branches) that CI enforces — so `npm run ci` is a true superset of GitHub CI, and passing it locally means CI's gates will pass. Expect several minutes. **`npm run validate`** remains the fast inner-loop check during development (unit tests only — no coverage gate, no smoke, no Storybook), but it is **NOT** an acceptable substitute for `npm run ci` before pushing: `validate` runs `test`, not `test:coverage`, so it does **zero** coverage gating. Skipping the gate is how a push passes every fast local check and still fails CI (this exact gap broke PR #1601 on a function-coverage regression). -- ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs the **`core/` gate first** (`validate:core`) and then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). - - **`validate:core` is the root-owned format + lint gate (#1689, widened in #1778 and #1767).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/`, the root `scripts/`, or the root "shared" surface before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `format:check:scripts` (`prettier --check "scripts/**/*.{mjs,js,cjs,ts}"`, the root build/verify tooling — #1778; a brace glob so a future non-`.mjs` script is auto-included) + `format:check:shared` + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`) + `lint:shared`. Use `npm run format:core` / `npm run format:scripts` / `npm run format:shared` to auto-fix (all folded into the root `format`). The **shared surface** (#1767) is `test-servers/src/**/*.ts`, the root `vitest.shared.mts`, and the root `eslint.config.js` — first-party code no client's `eslint .` / `prettier` reaches; it is both prettier-gated (`format:check:shared`) and eslint-gated (`lint:shared`, via a second `files` block in the root `eslint.config.js` scoped to Node globals). The `scripts/` gate is prettier-only — the root has no eslint config for `.mjs`. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). **prettier is pinned to an exact version** (not a caret) in all five `package.json`s (#1790) so the gate's verdict can't shift with an in-range patch bump. +- ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs **`verify:format-coverage` first** (the #1792 guard — asserts every tracked source file is covered by a `format:check` glob), then the **`core/` gate** (`validate:core`), then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). + - **`validate:core` is the root-owned format + lint gate (#1689, widened in #1778 and #1767).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/`, the root `scripts/`, or the root "shared" surface before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `format:check:scripts` (`prettier --check "scripts/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}"`, the root build/verify tooling — #1778) + `format:check:shared` + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`) + `lint:shared`. Use `npm run format:core` / `npm run format:scripts` / `npm run format:shared` to auto-fix (all folded into the root `format`). The **shared surface** (#1767) is `test-servers/src/**/*.{ts,tsx,mts,cts}`, the root `vitest.shared.mts`, and the root `eslint.config.js` — first-party code no client's `eslint .` / `prettier` reaches; it is both prettier-gated (`format:check:shared`) and eslint-gated (`lint:shared`, via a second `files` block in the root `eslint.config.js` scoped to Node globals). The `scripts/` gate is prettier-only — the root has no eslint config for `.mjs`. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). **prettier is pinned to an exact version** (not a caret) in all five `package.json`s (#1790) so the gate's verdict can't shift with an in-range patch bump. - **cli and tui now typecheck their `src` (#1689).** Their `build`/`test` run through esbuild (no type check), so each has a `typecheck` script (`tsc --noEmit -p tsconfig.json`) folded into `validate`. Their `tsconfig.json` matches `clients/web/tsconfig.app.json`'s module/lib *resolution* options — DOM lib, `moduleResolution: bundler`, and **no** `noUncheckedIndexedAccess` (web's app config does not extend `tsconfig.base`, so re-enabling it would surface `core/` issues web never gates) — so the imported `core/` sources are validated the same way web validates them. It does **not** mirror web's extra strictness flags (`noUnusedLocals`, `verbatimModuleSyntax`, ES2023 target, …), so cli/tui's own `src` is checked slightly more loosely than web's. `core/` itself still typechecks through web's `tsc -b`. - - The one CLI nuance: `clients/cli`'s out-of-process `e2e.test.ts` spawns the built binary, so its `test` **builds first** via `pretest` (`test-servers:build && build`). To avoid building it twice, `clients/cli`'s `validate` folds that in — it is `format:check && lint && typecheck && test` with **no** separate `build` step (the other clients, whose tests don't spawn their bundle, keep an explicit `build`). `validate:web`/`validate:tui`/`validate:launcher` are the uniform `format:check && lint && (typecheck &&) build && test`. (#1778, #1789) `clients/web`'s `format`/`format:check` covers `src`, `server`, `.storybook`, and its top-level configs (`*.{ts,js}` — `vite.config.ts`, `tsup.runner.config.ts`, `eslint.config.js`, …), not just `src`, so the Node backend, Storybook config, and Vite/build config are prettier-gated too; `clients/launcher`'s covers `src`, `__tests__`, `scripts`, and its top-level configs (the `*.` top-level glob is non-recursive, so each nested dir — `.storybook`, `scripts` — is named explicitly). + - The one CLI nuance: `clients/cli`'s out-of-process `e2e.test.ts` spawns the built binary, so its `test` **builds first** via `pretest` (`test-servers:build && build`). To avoid building it twice, `clients/cli`'s `validate` folds that in — it is `format:check && lint && typecheck && test` with **no** separate `build` step (the other clients, whose tests don't spawn their bundle, keep an explicit `build`). `validate:web`/`validate:tui`/`validate:launcher` are the uniform `format:check && lint && (typecheck &&) build && test`. (#1778, #1789, #1792) `clients/web`'s `format`/`format:check` covers `src`, `server`, `.storybook`, and its top-level configs (the uniform `*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` glob — `vite.config.ts`, `tsup.runner.config.ts`, `eslint.config.js`, …), not just `src`, so the Node backend, Storybook config, and Vite/build config are prettier-gated too; `clients/launcher`'s covers `src`, `__tests__`, `scripts`, and its top-level configs (the `*.` top-level glob is non-recursive, so each nested dir — `.storybook`, `scripts` — is named explicitly). The `verify:format-coverage` guard (#1792) enforces that this coverage stays complete. - **`npm run coverage`** is the per-file ≥90 gate and is now part of `npm run ci` — never treat it as optional before a push. It supersedes the old standalone `test:integration` step: web's `test:coverage` runs the `unit` **and** `integration` projects under v8 instrumentation, so `coverage` both enforces the ≥90 gate and exercises the same web integration paths CI covers. - **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `verify:build-gate` (the #1769 build gate — see below), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). GitHub CI runs this same chain as separate workflow steps, with the Storybook play-function tests last (see below). - `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below. @@ -255,7 +255,7 @@ The top-level **`src/types/`** is a sibling of both and is not the place for new Cross-directory imports point **one way, `lib → utils`** (infra depends on pure helpers, never the reverse). Keep it that way: if a `utils/` module needs a type currently exported from a `lib/` module, declare the type in `utils/` and re-export it from `lib/` (as `pendingReauth` owns `OAuthResumeAuthKind` and `oauthResume` re-exports it), rather than importing "up" from `utils` into `lib`. -Nothing **enforces** the boundary: no path alias keys off it, and the coverage `include` in `clients/web/vite.config.ts` lists **both** `src/lib/**` and `src/utils/**`, so a move between them is coverage-neutral (this is why the refactor was gate-safe). It's a human-legible signal at import time, valuable in a codebase this test-heavy (the ≥90% per-file gate). Note that `include` is a **whitelist** — it names `components`/`hooks`/`theme`/`lib`/`utils`/`server` (plus the `core/*` runtime; `hooks` and `theme` were added in #1787), so a module placed **outside** those directories (`types/`, `App.tsx`, or a brand-new grab-bag) falls out of the ≥90 gate entirely, silently. The one **deliberate, documented** exception is `src/App.tsx` — a ~4.5k-line composition root at ~42% branch coverage, whose omission is called out in a comment on the `include` array itself (gating it is a dedicated testing/decomposition effort, not a whitelist tweak) rather than left silent. When adding a module, place it by the rule and keep it inside a gated directory; when it genuinely mixes both (e.g. `downloadFile` bundles DOM-side-effect helpers with a couple of pure ones), keep it whole on its dominant side (`lib`) rather than splitting hairs. +Nothing **enforces** the boundary: no path alias keys off it, and the coverage `include` in `clients/web/vite.config.ts` lists **both** `src/lib/**` and `src/utils/**`, so a move between them is coverage-neutral (this is why the refactor was gate-safe). It's a human-legible signal at import time, valuable in a codebase this test-heavy (the ≥90% per-file gate). Note that `include` is a **whitelist** — it names `components`/`hooks`/`theme`/`lib`/`utils`/`server` (plus the `core/*` runtime; `hooks` and `theme` were added in #1787), so a module placed **outside** those directories (`types/`, `App.tsx`, or a brand-new grab-bag) falls out of the ≥90 gate entirely, silently. The **deliberate, documented** top-level-file exceptions are `src/App.tsx` — a ~4.5k-line composition root at ~42% branch coverage (gating it is a dedicated testing/decomposition effort, not a whitelist tweak) — and the `src/main.tsx` / `src/index.ts` bootstraps (browser `createRoot` render and the bin `runWeb` re-export, the analog of `clients/cli`'s excluded `src/index.ts`). All three are called out in a comment on the `include` array itself rather than left silent. When adding a module, place it by the rule and keep it inside a gated directory; when it genuinely mixes both (e.g. `downloadFile` bundles DOM-side-effect helpers with a couple of pure ones), keep it whole on its dominant side (`lib`) rather than splitting hairs. ## React instructions - UI Components diff --git a/README.md b/README.md index 04693f0a3..a8b358ce7 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ inspector/ │ ├── react/ # React hooks over the state stores │ └── storage/ # File I/O helpers for the OAuth persist backends ├── test-servers/ # Composable MCP test servers + fixtures used by integration tests -├── scripts/ # Root build/verify tooling (install cascade, smokes, verify-build-gate, pack:verify) +├── scripts/ # Root build/verify tooling (install cascade, smokes, verify-build-gate, verify-format-coverage, pack:verify) ├── specification/ # Design/build specifications ├── AGENTS.md # Contribution rules for agents AND humans (see below) └── README.md # You are here @@ -140,10 +140,11 @@ Each client self-validates from its own folder; the root scripts chain them. The | Script | What it does | | --------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `npm run validate` | Runs `validate:core` (the shared `core/` `format:check` + `lint` gate) first, then per client: `format:check` + `lint` + **`typecheck`** (cli/tui only) + `build` + fast unit tests. The quick inner-loop check. | +| `npm run validate` | Runs `verify:format-coverage` (asserts every tracked source file is format-gated) first, then `validate:core` (the shared `core/` `format:check` + `lint` gate), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui only) + `build` + fast unit tests. The quick inner-loop check. | | `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). | | `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus a headless-Chromium boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests). | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. | +| `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | | `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. | | `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). | diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index 749dd3e85..3df74bd13 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -167,12 +167,16 @@ export default defineConfig(({ command }) => { coverage: { provider: "v8", reporter: ["text", "html", "json-summary"], - // Whitelist of gated directories. NOTE: `src/App.tsx` is deliberately - // NOT listed — it is a ~4.5k-line composition root at ~42% branch - // coverage, so gating it is a dedicated testing/decomposition effort, - // not a whitelist tweak. Its omission is intentional and documented - // here (and in AGENTS.md) rather than silent. Every *other* src - // directory is gated; add new ones here as they appear. + // Whitelist of gated directories. Deliberate top-level-file omissions + // (every src *directory* below is gated): + // • `src/App.tsx` — a ~4.5k-line composition root at ~42% branch + // coverage; gating it is a dedicated testing/decomposition effort, + // not a whitelist tweak. + // • `src/main.tsx` / `src/index.ts` — the browser and bin bootstraps + // (createRoot render / `runWeb` re-export), the analog of + // clients/cli's excluded `src/index.ts`. + // These omissions are intentional and documented here (and in AGENTS.md) + // rather than silent. Add new gated dirs here as they appear. include: [ "src/components/**/*.{ts,tsx}", "src/hooks/**/*.{ts,tsx}", diff --git a/scripts/verify-format-coverage.mjs b/scripts/verify-format-coverage.mjs index 0376c1f43..259381ef5 100644 --- a/scripts/verify-format-coverage.mjs +++ b/scripts/verify-format-coverage.mjs @@ -60,10 +60,39 @@ function tokenize(command) { return tokens; } -/** Extract the path/glob args from every `prettier --check …` in a manifest. */ +/** + * Names of scripts transitively reachable from `entry` by following `npm run + * ` references within a manifest. Used so we only trust a `format:check` + * glob that CI actually runs — a `prettier --check` script that nothing invokes + * from `validate` doesn't gate anything, and counting its globs would let the + * gate be silently unwired (the file still "matches a glob" that never runs). + */ +function reachableScripts(scripts, entry = "validate") { + const reached = new Set(); + const queue = [entry]; + const runRef = /npm run ([\w:-]+)/g; + while (queue.length > 0) { + const name = queue.shift(); + if (reached.has(name)) continue; + reached.add(name); + const cmd = scripts?.[name]; + if (typeof cmd !== "string") continue; + for (const m of cmd.matchAll(runRef)) queue.push(m[1]); + } + return reached; +} + +/** + * Extract the path/glob args from every `prettier --check …` in a manifest's + * scripts that is reachable from `validate`. Restricting to reachable scripts is + * what makes the guard assert "this file is checked by CI", not merely "some + * glob covers it". + */ function prettierCheckArgs(scripts) { + const reachable = reachableScripts(scripts); const args = []; - for (const value of Object.values(scripts ?? {})) { + for (const [name, value] of Object.entries(scripts ?? {})) { + if (!reachable.has(name)) continue; if (typeof value !== "string" || !value.includes("prettier --check")) continue; // A manifest may chain `prettier --check …` inside a larger script; take the From 332eb613b552f999686c52965928f1af1683db03 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 26 Jul 2026 09:47:16 -0400 Subject: [PATCH 6/6] chore: address PR #1792 round-5 findings (airtight guard + doc/config sync) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - verify-format-coverage: assert the root `validate` chain invokes each client's `validate` (cd && npm run validate), closing the last reachability edge — a client dropped from the root chain now fails the guard even though its own globs still exist. Negative-tested. - eslint.config.js: widen the shared `files` block to test-servers/src/**/*.{ts,tsx,mts,cts} to match the widened lint:shared glob, so a future .mts/.cts there is lint-gated, not just prettier-gated. - AGENTS.md: scope the "uniform extension set" claim to client globs (core stays {ts,tsx}, shared {ts,tsx,mts,cts}), with verify:format-coverage as the backstop — removing the self-contradiction with the line below. npm run ci green. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- AGENTS.md | 2 +- eslint.config.js | 2 +- scripts/verify-format-coverage.mjs | 40 ++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d09ba33bf..733691ff0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -214,7 +214,7 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - after making the changes, respond to each review comment with what was done (or why it was ignored) ### Mandatory pre-push gate -- ALWAYS do `npm run format` before committing — the **root** `format` auto-fixes `core/` (`format:core`), the root `scripts/` tooling (`format:scripts`), the root "shared" surface (`format:shared` — `test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`), and every client's scope in one shot. Every format glob uses the uniform extension set `*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` (#1792) so a new-extension file can't slip the gate, and `npm run verify:format-coverage` (the first step of `validate`, #1792) fails if any tracked source file is left uncovered by a `format:check` glob. `validate` runs `format:check` (the non-fixing variant, including `format:check:core`, `format:check:scripts`, and `format:check:shared`) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it. +- ALWAYS do `npm run format` before committing — the **root** `format` auto-fixes `core/` (`format:core`), the root `scripts/` tooling (`format:scripts`), the root "shared" surface (`format:shared` — `test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`), and every client's scope in one shot. Every **client** format glob uses the uniform extension set `*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` (#1792) so a new-extension file can't slip the gate; `core/` stays `{ts,tsx}` and the shared surface `{ts,tsx,mts,cts}` (their surfaces can't hold the other extensions), and `npm run verify:format-coverage` (the first step of `validate`, #1792) is the backstop — it fails if any tracked source file is left uncovered by a `format:check` glob regardless of which glob was expected to catch it. `validate` runs `format:check` (the non-fixing variant, including `format:check:core`, `format:check:scripts`, and `format:check:shared`) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it. - **`npm run ci` is the mandatory pre-push command** — it mirrors `.github/workflows/main.yml` (minus `npm install`): `validate` → `coverage` → `verify:build-gate` (the #1769 browser-externalized-builtin build gate) → `smoke` → Storybook play-function tests (installs Playwright chromium if needed). It now runs **`npm run coverage`**, the per-file ≥90 gate (lines/statements/functions/branches) that CI enforces — so `npm run ci` is a true superset of GitHub CI, and passing it locally means CI's gates will pass. Expect several minutes. **`npm run validate`** remains the fast inner-loop check during development (unit tests only — no coverage gate, no smoke, no Storybook), but it is **NOT** an acceptable substitute for `npm run ci` before pushing: `validate` runs `test`, not `test:coverage`, so it does **zero** coverage gating. Skipping the gate is how a push passes every fast local check and still fails CI (this exact gap broke PR #1601 on a function-coverage regression). - ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs **`verify:format-coverage` first** (the #1792 guard — asserts every tracked source file is covered by a `format:check` glob), then the **`core/` gate** (`validate:core`), then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). - **`validate:core` is the root-owned format + lint gate (#1689, widened in #1778 and #1767).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/`, the root `scripts/`, or the root "shared" surface before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `format:check:scripts` (`prettier --check "scripts/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}"`, the root build/verify tooling — #1778) + `format:check:shared` + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`) + `lint:shared`. Use `npm run format:core` / `npm run format:scripts` / `npm run format:shared` to auto-fix (all folded into the root `format`). The **shared surface** (#1767) is `test-servers/src/**/*.{ts,tsx,mts,cts}`, the root `vitest.shared.mts`, and the root `eslint.config.js` — first-party code no client's `eslint .` / `prettier` reaches; it is both prettier-gated (`format:check:shared`) and eslint-gated (`lint:shared`, via a second `files` block in the root `eslint.config.js` scoped to Node globals). The `scripts/` gate is prettier-only — the root has no eslint config for `.mjs`. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). **prettier is pinned to an exact version** (not a caret) in all five `package.json`s (#1790) so the gate's verdict can't shift with an in-range patch bump. diff --git a/eslint.config.js b/eslint.config.js index d29cbd721..dfa945d1b 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -45,7 +45,7 @@ export default defineConfig([ }, { files: [ - "test-servers/src/**/*.ts", + "test-servers/src/**/*.{ts,tsx,mts,cts}", "vitest.shared.mts", "eslint.config.js", ], diff --git a/scripts/verify-format-coverage.mjs b/scripts/verify-format-coverage.mjs index 259381ef5..13fca8299 100644 --- a/scripts/verify-format-coverage.mjs +++ b/scripts/verify-format-coverage.mjs @@ -147,6 +147,30 @@ function globToRegExp(glob) { return new RegExp("^" + re + "$"); } +/** + * Assert the root `validate` chain actually invokes each non-root manifest's + * `validate` (via `cd && npm run validate`). Without this, a client's + * globs would still be harvested from its own `validate` and count as coverage + * even if the root chain stopped running that client — the same "gate silently + * stops gating" failure as the reachable-script check, one level up. Returns the + * list of manifest dirs the root chain does NOT reach. + */ +function clientsUnreachedFromRoot() { + const rootPkg = JSON.parse( + readFileSync(path.join(repoRoot, "package.json"), "utf8"), + ); + const reachedNames = reachableScripts(rootPkg.scripts); + const reachedCommands = [...reachedNames] + .map((n) => rootPkg.scripts?.[n]) + .filter((c) => typeof c === "string"); + return MANIFESTS.filter((dir) => dir !== ".").filter( + (dir) => + !reachedCommands.some( + (c) => c.includes(`cd ${dir}`) && /npm run validate/.test(c), + ), + ); +} + /** Build the set of coverage predicates from all manifests' format globs. */ function buildMatchers() { const matchers = []; @@ -176,6 +200,22 @@ function trackedSourceFiles() { return out.split("\n").filter(Boolean); } +const unreachedClients = clientsUnreachedFromRoot(); +if (unreachedClients.length > 0) { + console.error( + `verify:format-coverage — the root \`validate\` chain does not invoke ${unreachedClients.length} client validation(s):\n`, + ); + for (const dir of unreachedClients) + console.error(` ${dir} (expected \`cd ${dir} && npm run validate\`)`); + console.error( + "\nA client whose `validate` the root chain never runs is not format-gated by CI,", + ); + console.error( + "even though its globs exist. Restore the `validate:` link in the root `validate`.", + ); + process.exit(1); +} + const matchers = buildMatchers(); const files = trackedSourceFiles(); const ungated = files.filter((f) => !matchers.some((m) => m(f)));