* chore: close a batch of quality-gate & tidy gaps (#1785 #1787 #1789 #1790 #1767) 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore: address PR #1792 review findings - #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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore: handle PR #1792 round-3 follow-ups in-place (format-coverage guard, uniform globs, theme gate) 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore: address PR #1792 round-4 findings (guard hardening, docs, exclusion list) - 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore: address PR #1792 round-5 findings (airtight guard + doc/config sync) - verify-format-coverage: assert the root `validate` chain invokes each client's `validate` (cd <dir> && 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
166ed38e7f
commit
4defa32ac6
1 changed files with 1221 additions and 220 deletions
12
AGENTS.md
12
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`), 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/` 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 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: '^_'`).
|
||||
- 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) `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, #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.
|
||||
|
|
@ -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`/`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
|
||||
|
|
|
|||
Loading…
Reference in a new issue