diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 9259214a0..5fdc748db 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -28,8 +28,10 @@ jobs: # clients/launcher, so this single step sets up every client. run: npm install - - name: Validate (format, lint, build, fast tests) - # Each client self-validates: format:check + lint + build + test (no + - name: Validate (coverage guards, format, lint, typecheck, build, fast tests) + # Runs the two coverage guards first (verify:format-coverage + + # verify:typecheck-coverage), then validate:core, then each client's + # self-validation: format:check + lint + typecheck + build + test (no # coverage instrumentation — fast). This also builds every client bundle # (web dist, cli/tui/launcher) that the smokes below need. The heavier # per-file coverage gate runs in its own step below. Unit tests run in diff --git a/AGENTS.md b/AGENTS.md index 9b9ac1b77..08944062d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -217,6 +217,7 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - **Test placement: side-by-side by default, `src/test/` only for what can't be co-located.** These look like competing conventions but aren't — the split is: *tests live beside their source, **except** tests for the repo-root `core/` package (which lives outside `clients/web/`) and shared test scaffolding — both of which live under `src/test/`, with `core/` tests mirroring the `core/` layout and integration tests under `src/test/integration/`.* - **Side-by-side (`.test.tsx` next to the source) — the default for web's own `src/` code.** Components, hooks, `lib/`, `utils/`. This is the overwhelming majority; a web-owned test living under `src/test/` instead of beside its source is a bug (fixed one such straggler, `downloadFile.test.ts`, in #1776). - **`src/test/` — the three things that *cannot* be co-located:** (1) tests of the repo-root **`core/`** package (`src/test/core/…`, mirroring the `core/` folder layout — `core/` physically lives at `/core` outside `clients/web/`, is consumed via the `@inspector/core` alias, and has no test harness of its own, so co-locating would pollute the shared isomorphic package with web-only test infra); (2) the **`integration`** vitest project (`src/test/integration/…`, node env, 30s — placement *is* the manifest, see above); (3) **shared test infrastructure** (`renderWithMantine.tsx`, `setup.ts`, `fixtures/`, `scrollAreaStoryAssertions.ts`) — not tests *of* a source file, so nothing to sit beside. + - **The above is web only.** The Node clients (**cli, tui, launcher**) keep **all** their tests in a top-level **`__tests__/`** dir, not beside their source — their `tsconfig.json` excludes `**/*.test.*` and their `tsconfig.test.json` includes `__tests__/**/*` (plus, for launcher, its root `vitest.config.ts`), so a co-located `src/**/*.test.*` lands in **no** tsconfig project and fails `npm run verify:typecheck-coverage` (#1791). Put a new cli/tui/launcher test under `__tests__/`. - Use `renderWithMantine` from `src/test/renderWithMantine.tsx` to render components — it wraps in `MantineProvider` with the project theme. It sets `env="test"` so Mantine renders transitions synchronously (no internal `setTimeout`); this prevents a `Transition`/`Modal` timer from firing after happy-dom tears down `window` at end-of-run and failing the whole run with an uncaught `ReferenceError: window is not defined` (#1760). **Always render through `renderWithMantine`; do not hand-roll a bare `MantineProvider` in a test** (that reintroduces the leak class). To exercise a **forced color scheme** (e.g. the `useComputedColorScheme` dark branch) pass the `colorScheme` option — `renderWithMantine(ui, { colorScheme: "dark" })` — instead of hand-rolling a `defaultColorScheme="dark"` provider (#1786). Only when a test must assert *mid-flight* transition state (e.g. a `data-anim="out"` cell during an exit crossfade) use `renderWithMantineTransitions` (real transitions). Such a test can leak the #1760 class because waiting for one cell to unmount does **not** settle a concurrent *enter* (a completed enter leaves no DOM signal to `waitFor`), so the helper **automatically drains the in-flight animation after the test**. The rule for using it: pass `settleMs` derived from the component's real animation duration — its `Transition` `duration`/`exitDuration` plus any `enterDelay`/`exitDelay` plus rAF slack — e.g. `renderWithMantineTransitions(ui, { settleMs: HEADER_ANIM_MS + 200 })` (so the window can't silently become insufficient when that duration changes); do **not** also use `vi.useFakeTimers()` in the same test (the auto-settle no-ops under fake timers — it warns, but anything the test left pending on the *real* clock is then unprotected, so the test depends on which clock was installed at teardown); and if the test unmounts the tree itself, use the `unmount()` the helper returns (it drops that tree from the settle's liveness check, while still draining — a bare mid-body `cleanup()` on a still-armed tree would trip the check). The mechanism behind all three — why the drain is `act`-wrapped, the fake-timer hazard, the `afterEach`-before-`cleanup()` ordering and its `container.isConnected` self-checks, and the exported `settleTransitions(ms)` for manual mid-body settling — is documented at length on the helper in `renderWithMantine.tsx`; read there before changing it. ### Responding to Code Reviews @@ -228,9 +229,10 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i ### 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 **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). +- 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 **`verify:typecheck-coverage`** (the #1791 guard — asserts every tracked `.ts`/`.tsx`/`.mts`/`.cts` in each gated Node client, plus the non-client first-party TS like `core/` and `test-servers/src`, lands in a tsconfig project), then **`test:scripts`** (the guard's own parser unit tests, `node --test`), 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`. + - **cli and tui now typecheck their `src` (#1689).** Their `build`/`test` run through esbuild (no type check), so each has a `typecheck` script 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 `__tests__` dirs are typechecked too (#1791).** The src-only `tsconfig.json` excludes `**/*.test.*`, so each of cli, tui, and launcher carries a **`tsconfig.test.json`** — extending the build config, `noEmit`, including `__tests__/**/*` (only the tests root the project; tsc pulls in the `src` they import, and the src-only config already validates all of `src` without the test-only aliases) and adding the test-only path aliases that resolve what vitest resolves via `vitest.shared.mts`. The alias set differs per client: **cli's is the widest** (`@modelcontextprotocol/inspector-test-server` → `test-servers/src`, the `@inspector/core/*` deep paths, express/vitest — cli is the only one importing the test-server package); **tui's** carries only the `@inspector/core/*` + react/vitest redirects; **launcher's** has **no** `paths` at all — it's a plain `rootDir: "."` sibling of the build config (whose `rootDir: ./src` is what rejects the tests). Each client's `typecheck` script runs **both** projects (`tsc -p tsconfig.json && tsc -p tsconfig.test.json`) so running it standalone means the same thing everywhere (launcher's `build` also `tsc`s `src`, but `typecheck` doesn't rely on that). cli additionally carries `@types/express` (devDep) so the transitively-aliased test-server source typechecks, mirroring `clients/web` (cli's `tsconfig.test.json` also names `test-servers/src/server-composable.ts` explicitly — a bin entry the barrel doesn't import, so nothing else gives it a tsc pass). The client **config files** are typechecked too: cli's/tui's (`vitest.config.ts`, `tsup.config.ts`, tui `dev.ts`) are folded into each src `tsconfig.json`'s `include`; launcher's `vitest.config.ts` goes in its `tsconfig.test.json` instead (again the `rootDir: ./src` reason). Note the gate checks mock **implementations and return types** (typing a `vi.fn()` against a real signature keeps its `mockResolvedValue`/impl in sync) but **not** `toHaveBeenCalledWith(...)` arguments — vitest types those to accept anything regardless of the mock's type parameter. **`npm run verify:typecheck-coverage`** (`scripts/verify-typecheck-coverage.mjs`, run as the second step of `validate` right after `verify:format-coverage`) is the durable guard for this invariant: it runs each client's `typecheck` projects with `tsc --listFilesOnly`, unions them, and fails on any tracked `.ts`/`.tsx`/`.mts`/`.cts` that lands in no project — for every gated Node client, which it discovers from disk (each `clients/*` is enrolled through its `typecheck` script's projects, or — for a `tsc -b` client like `clients/web` with no `typecheck` script — through its `tsconfig.json` `references`), so a new client is covered without editing the guard — the typecheck analog of `verify:format-coverage`, since a project only reaches the files its `include` names plus their transitive imports, so a new top-level file (launcher especially, whose build `rootDir: ./src` rejects package-root files) can otherwise fall out silently. Like its sibling it also asserts the gate is *wired* (each client's typecheck pass is reachable from its `validate` — its `typecheck` script for cli/tui/launcher, or a real `tsc -b` for web — and the root chain runs each client's `validate`), so it can't stay green while measuring a pass nothing invokes. It asserts the same of **`test:scripts`** — its own parser tests — on three axes: reachable from the root `validate`, a **non-empty** tracked `scripts/**/*.{test,spec}.*` set, and **every one of those files matched by a glob harvested across the scripts reachable from `test:scripts`** (so a delegating `test:scripts` still measures correctly). The third axis exists because `node --test` silently *skips* a file its glob misses and still exits 0 — a rename to `*.spec.mjs` would shrink the suite with a green run. Beyond the clients it also covers, **deny-by-default**, the first-party TS no client owns — everything tracked outside `clients/*` (`test-servers/src/**`, the root `vitest.shared.mts`, **all of `core/`**, and any new top-level TS location) must land in the *global* union of client projects (cli aliases the test-server source; web's enrolled projects include `core/`). So a `core` `*.tsx` web's `include` doesn't reach, or an unimported `test-servers/src` bin entry, can't ship uncompiled-but-unchecked. The one "listed but unchecked" tier the guard structurally can't see — a per-file `// @ts-nocheck` — is owned by a different gate: `@typescript-eslint/ban-ts-comment` rejects it across every surface (`lint:core`, `lint:shared`, and each client's `eslint .`). The guard's own pure parsers (`scripts/lib/npm-scripts.mjs` + the exported helpers of `verify-typecheck-coverage.mjs`, whose execution is behind a `main()` so importing it for tests doesn't run it) are **unit-tested** — `npm run test:scripts` (node's built-in `node --test`, in `validate`; the root has no vitest harness by design) runs table-driven cases, one per rule the guard's parsers encode, and the guard itself enforces that this stays wired (above). - 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). diff --git a/README.md b/README.md index 652a9b41a..058fb77ea 100644 --- a/README.md +++ b/README.md @@ -146,6 +146,8 @@ Each client self-validates from its own folder; the root scripts chain them. The | `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 test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs` + the exported helpers of `verify-typecheck-coverage.mjs`), one case per rule they encode. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | +| `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs 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/cli/__tests__/cli.test.ts b/clients/cli/__tests__/cli.test.ts index 832cb58a5..d50cd861c 100644 --- a/clients/cli/__tests__/cli.test.ts +++ b/clients/cli/__tests__/cli.test.ts @@ -23,7 +23,7 @@ import { createListRootsTool, createTestServerInfo, } from "@modelcontextprotocol/inspector-test-server"; -import type { MCPServerConfig } from "@modelcontextprotocol/inspector-core/mcp/index.js"; +import type { MCPServerConfig } from "@inspector/core/mcp/index.js"; describe("CLI Tests", () => { describe("Basic CLI Mode", () => { @@ -452,7 +452,7 @@ describe("CLI Tests", () => { // the config file into the connection's settings. const sawHeader = server .getRecordedRequests() - .some((r) => r.headers["x-custom-token"] === "secret-123"); + .some((r) => r.headers?.["x-custom-token"] === "secret-123"); expect(sawHeader).toBe(true); } finally { await server.stop(); @@ -493,7 +493,7 @@ describe("CLI Tests", () => { expectCliSuccess(result); const tokens = server .getRecordedRequests() - .map((r) => r.headers["x-custom-token"]); + .map((r) => r.headers?.["x-custom-token"]); expect(tokens).toContain("from-cli"); expect(tokens).not.toContain("from-disk"); } finally { diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index f18367be6..70738d784 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -13,6 +13,10 @@ import { } from "../src/cliOAuth.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; import { createInterface } from "node:readline/promises"; +import { + makeFakeCliOAuthClient, + makeFakeServerSettings, +} from "./helpers/oauth-test-fakes.js"; // `confirmStepUpFromStdin` (the default step-up confirmer) reads a line from // stdin via node:readline/promises. Mock the module so the default path can be @@ -103,7 +107,7 @@ describe("cliOAuth", () => { expect( isStandardOAuthStepUp( { reason: "insufficient_scope", requiredScopes: ["weather:read"] }, - {}, + makeFakeServerSettings(), ), ).toBe(true); }); @@ -112,7 +116,7 @@ describe("cliOAuth", () => { expect( isStandardOAuthStepUp( { reason: "insufficient_scope", requiredScopes: ["weather:read"] }, - { enterpriseManaged: true }, + makeFakeServerSettings({ enterpriseManaged: true }), ), ).toBe(false); }); @@ -220,7 +224,7 @@ describe("cliOAuth", () => { error, new MutableRedirectUrlProvider(), { hostname: "127.0.0.1", port: 6276, pathname: "/oauth/callback" }, - {}, + makeFakeServerSettings(), { confirmStepUp: async () => false, isTTY: true }, ), ).rejects.toThrow("Step-up authorization declined."); @@ -250,7 +254,7 @@ describe("cliOAuth", () => { error, new MutableRedirectUrlProvider(), { hostname: "127.0.0.1", port: 6276, pathname: "/oauth/callback" }, - {}, + makeFakeServerSettings(), { confirmStepUp: async () => true, isTTY: true }, ); @@ -338,7 +342,7 @@ describe("cliOAuth", () => { OAUTH_HTTP_CONFIG, new MutableRedirectUrlProvider(), { hostname: "127.0.0.1", port: 6276, pathname: "/oauth/callback" }, - { enterpriseManaged: true }, + makeFakeServerSettings({ enterpriseManaged: true }), fn, { confirmStepUp: async () => true, isTTY: true }, ); @@ -375,7 +379,7 @@ describe("cliOAuth", () => { standardStepUpError(), new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, - {}, + makeFakeServerSettings(), INTERACTIVE, ); @@ -395,7 +399,7 @@ describe("cliOAuth", () => { standardStepUpError(), new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, - {}, + makeFakeServerSettings(), INTERACTIVE, ); @@ -412,7 +416,7 @@ describe("cliOAuth", () => { standardStepUpError(), new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, - {}, + makeFakeServerSettings(), INTERACTIVE, ), ).rejects.toThrow("Step-up authorization declined."); @@ -431,7 +435,7 @@ describe("cliOAuth", () => { standardStepUpError(), new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, - {}, + makeFakeServerSettings(), INTERACTIVE, ), ).rejects.toThrow("Step-up authorization declined."); @@ -455,7 +459,7 @@ describe("cliOAuth", () => { standardStepUpError(), new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, - {}, + makeFakeServerSettings(), INTERACTIVE, ), ).rejects.toThrow("Step-up authorization declined."); @@ -482,7 +486,7 @@ describe("cliOAuth", () => { standardStepUpError(), new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, - {}, + makeFakeServerSettings(), INTERACTIVE, ); @@ -503,7 +507,7 @@ describe("cliOAuth", () => { standardStepUpError(), new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, - {}, + makeFakeServerSettings(), { isTTY: false }, ); expect(runSpy).toHaveBeenCalled(); @@ -522,7 +526,7 @@ describe("cliOAuth", () => { standardStepUpError(), new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, - {}, + makeFakeServerSettings(), { isTTY: true, stepUpPromptTimeoutMs: 20 }, ), ).rejects.toMatchObject({ @@ -556,7 +560,7 @@ describe("cliOAuth", () => { standardStepUpError(), new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, - {}, + makeFakeServerSettings(), INTERACTIVE, ); let settled = false; @@ -596,11 +600,10 @@ describe("cliOAuth", () => { ), ) .mockResolvedValueOnce(undefined); - const client = { + const client = makeFakeCliOAuthClient({ connect, - disconnect: vi.fn(), checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(true), - }; + }); await connectInspectorWithOAuth( client, @@ -626,11 +629,10 @@ describe("cliOAuth", () => { ), ) .mockResolvedValueOnce(undefined); - const client = { + const client = makeFakeCliOAuthClient({ connect, - disconnect: vi.fn(), checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(false), - }; + }); await connectInspectorWithOAuth( client, @@ -654,11 +656,11 @@ describe("cliOAuth", () => { .mockRejectedValueOnce(new Error("Connection failed for server (401)")) .mockResolvedValueOnce(undefined); // A rejecting disconnect exercises the `.catch(() => {})` guard. - const client = { + const client = makeFakeCliOAuthClient({ connect, disconnect: vi.fn().mockRejectedValue(new Error("disconnect failed")), checkAuthChallengeSatisfied: vi.fn(), - }; + }); await connectInspectorWithOAuth( client, @@ -679,11 +681,10 @@ describe("cliOAuth", () => { const connect = vi .fn() .mockRejectedValue(new Error("some unrelated failure")); - const client = { + const client = makeFakeCliOAuthClient({ connect, - disconnect: vi.fn(), checkAuthChallengeSatisfied: vi.fn(), - }; + }); await expect( connectInspectorWithOAuth( @@ -698,11 +699,10 @@ describe("cliOAuth", () => { it("rethrows when the server config is not OAuth-capable", async () => { const connect = vi.fn().mockRejectedValue(new Error("nope (401)")); - const client = { + const client = makeFakeCliOAuthClient({ connect, - disconnect: vi.fn(), checkAuthChallengeSatisfied: vi.fn(), - }; + }); await expect( connectInspectorWithOAuth( @@ -721,11 +721,10 @@ describe("cliOAuth", () => { reason: "token_expired", }), ); - const client = { + const client = makeFakeCliOAuthClient({ connect, - disconnect: vi.fn(), checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(false), - }; + }); await expect( connectInspectorWithOAuth( @@ -751,11 +750,10 @@ describe("cliOAuth", () => { ), ) .mockResolvedValueOnce(undefined); - const client = { + const client = makeFakeCliOAuthClient({ connect, - disconnect: vi.fn(), checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(true), - }; + }); await connectInspectorWithOAuth( client, @@ -775,11 +773,10 @@ describe("cliOAuth", () => { const connect = vi .fn() .mockRejectedValue(new Error("Connection failed for server (401)")); - const client = { + const client = makeFakeCliOAuthClient({ connect, - disconnect: vi.fn(), checkAuthChallengeSatisfied: vi.fn(), - }; + }); await expect( connectInspectorWithOAuth( @@ -1101,11 +1098,10 @@ describe("cliOAuth", () => { const connect = vi.fn().mockRejectedValue(err); await expect( connectInspectorWithOAuth( - { + makeFakeCliOAuthClient({ connect, - disconnect: vi.fn(), checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(false), - }, + }), OAUTH_HTTP_CONFIG, new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, diff --git a/clients/cli/__tests__/helpers/fixtures.ts b/clients/cli/__tests__/helpers/fixtures.ts index 457986a5c..abb25f519 100644 --- a/clients/cli/__tests__/helpers/fixtures.ts +++ b/clients/cli/__tests__/helpers/fixtures.ts @@ -3,7 +3,7 @@ import * as path from "path"; import * as os from "os"; import * as crypto from "crypto"; import { getTestMcpServerCommand } from "@modelcontextprotocol/inspector-test-server"; -import type { MCPServerConfig } from "@modelcontextprotocol/inspector-core/mcp/index.js"; +import type { MCPServerConfig } from "@inspector/core/mcp/index.js"; /** * Sentinel value for tests that don't need a real server diff --git a/clients/cli/__tests__/helpers/oauth-test-fakes.ts b/clients/cli/__tests__/helpers/oauth-test-fakes.ts new file mode 100644 index 000000000..b53e15703 --- /dev/null +++ b/clients/cli/__tests__/helpers/oauth-test-fakes.ts @@ -0,0 +1,75 @@ +import { vi } from "vitest"; +import { + DEFAULT_MAX_FETCH_REQUESTS, + DEFAULT_TASK_TTL_MS, +} from "@inspector/core/mcp/types.js"; +import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; +import type { CliOAuthClient } from "../../src/cliOAuth.js"; + +/** + * Typed mock factories for the CLI OAuth tests. They exist so a test can supply + * only the surface a given code path exercises while `tsc` still sees a + * complete `CliOAuthClient` / `InspectorServerSettings` — avoiding a spray of + * `as unknown as` casts (see #1791 and the AGENTS.md `as`-cast policy). + */ + +/** + * A full {@link CliOAuthClient} whose every method is a `vi.fn()` resolving to a + * benign value. Pass `overrides` to swap in the method(s) a test drives (e.g. a + * `connect` that rejects with `AuthRecoveryRequiredError`). + */ +export function makeFakeCliOAuthClient( + overrides: Partial = {}, +): CliOAuthClient { + // The defaults are typed against the real CliOAuthClient signatures (not bare + // vi.fn()s) so a default with a wrong implementation/return would be rejected, + // the same way the tui App spies are typed. Note this does NOT extend to + // `overrides`: a caller passing a bare `vi.fn()` gets `Mock<(...args) => any>`, + // assignable to any member, so an override's implementation is unchecked — the + // factory guarantees the object's shape, not each override's signature. (And + // since `exactOptionalPropertyTypes` is off, `Partial<…>` even admits an + // explicit `undefined` for a required member.) + return { + connect: vi.fn().mockResolvedValue(undefined), + disconnect: vi + .fn() + .mockResolvedValue(undefined), + authenticate: vi + .fn() + .mockResolvedValue(undefined), + beginInteractiveAuthorization: vi + .fn() + .mockResolvedValue(undefined), + completeOAuthFlow: vi + .fn() + .mockResolvedValue(undefined), + checkAuthChallengeSatisfied: vi + .fn() + .mockResolvedValue(false), + ...overrides, + }; +} + +/** + * A full {@link InspectorServerSettings} with representative defaults for its + * required fields (the timeouts at 0 = "SDK default", `taskTtl` / + * `maxFetchRequests` at their product defaults, empty lists elsewhere). Pass + * `overrides` for the field(s) under test (e.g. `{ enterpriseManaged: true }`); + * as a `Partial<…>` with `exactOptionalPropertyTypes` off it also admits an + * explicit `undefined` for a required field, so don't rely on it to reject one. + */ +export function makeFakeServerSettings( + overrides: Partial = {}, +): InspectorServerSettings { + return { + headers: [], + metadata: [], + env: [], + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: DEFAULT_TASK_TTL_MS, + maxFetchRequests: DEFAULT_MAX_FETCH_REQUESTS, + roots: [], + ...overrides, + }; +} diff --git a/clients/cli/__tests__/oauth-interactive.test.ts b/clients/cli/__tests__/oauth-interactive.test.ts index 0508c5ec2..db1cad720 100644 --- a/clients/cli/__tests__/oauth-interactive.test.ts +++ b/clients/cli/__tests__/oauth-interactive.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, afterEach } from "vitest"; +import { describe, it, expect, afterEach, vi } from "vitest"; import { rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -21,6 +21,7 @@ import { withCliAuthRecoveryRetry, } from "../src/cliOAuth.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; +import { makeFakeServerSettings } from "./helpers/oauth-test-fakes.js"; const oauthTestStatePath = join( tmpdir(), @@ -216,15 +217,9 @@ describe("CLI interactive OAuth (integration)", () => { const getTempTool = tools.tools.find((tool) => tool.name === "get_temp"); expect(getTempTool).toBeDefined(); - let stepUpPrompted = false; - const originalWrite = process.stderr.write.bind(process.stderr); - process.stderr.write = ((chunk, ...rest) => { - const text = typeof chunk === "string" ? chunk : String(chunk); - if (text.includes("Proceed with step-up authorization?")) { - stepUpPrompted = true; - } - return originalWrite(chunk, ...rest); - }) as typeof process.stderr.write; + const stderrSpy = vi + .spyOn(process.stderr, "write") + .mockImplementation(() => true); try { const result = await withCliAuthRecoveryRetry( @@ -232,7 +227,7 @@ describe("CLI interactive OAuth (integration)", () => { { type: "streamable-http", url: `${serverUrl}/mcp` }, redirectUrlProvider, callbackUrlConfig, - {}, + makeFakeServerSettings(), () => client.callTool(getTempTool!, { city: "NYC", @@ -241,10 +236,15 @@ describe("CLI interactive OAuth (integration)", () => { { confirmStepUp: async () => true, isTTY: true }, ); + const stepUpPrompted = stderrSpy.mock.calls.some( + ([chunk]) => + typeof chunk === "string" && + chunk.includes("Proceed with step-up authorization?"), + ); expect(stepUpPrompted).toBe(true); expect(result.success).toBe(true); } finally { - process.stderr.write = originalWrite; + stderrSpy.mockRestore(); await client.disconnect(); } }, 30_000); diff --git a/clients/cli/package-lock.json b/clients/cli/package-lock.json index ccecd55f5..d9a12d310 100644 --- a/clients/cli/package-lock.json +++ b/clients/cli/package-lock.json @@ -26,6 +26,7 @@ }, "devDependencies": { "@eslint/js": "^9.39.4", + "@types/express": "^5.0.6", "@types/node": "^24.12.4", "@vitest/coverage-v8": "^4.1.0", "eslint": "^9.39.4", @@ -1815,6 +1816,17 @@ "tslib": "^2.4.0" } }, + "node_modules/@types/body-parser": { + "version": "1.19.6", + "resolved": "https://registry.npmjs.org/@types/body-parser/-/body-parser-1.19.6.tgz", + "integrity": "sha512-HLFeCYgz89uk22N5Qg3dvGvsv46B8GLvKKo1zKG4NybA8U2DiEO3w9lqGg29t/tfLRJpJ6iQxnVw4OnB7MoM9g==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/connect": "*", + "@types/node": "*" + } + }, "node_modules/@types/chai": { "version": "5.2.3", "resolved": "https://registry.npmjs.org/@types/chai/-/chai-5.2.3.tgz", @@ -1826,6 +1838,16 @@ "assertion-error": "^2.0.1" } }, + "node_modules/@types/connect": { + "version": "3.4.38", + "resolved": "https://registry.npmjs.org/@types/connect/-/connect-3.4.38.tgz", + "integrity": "sha512-K6uROf1LD88uDQqJCktA4yzL1YYAK6NgfsI0v/mTgyPKWsX1CnJ0XPSDhViejru1GcRkLWb8RlzFYJRqGUbaug==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/node": "*" + } + }, "node_modules/@types/deep-eql": { "version": "4.0.2", "resolved": "https://registry.npmjs.org/@types/deep-eql/-/deep-eql-4.0.2.tgz", @@ -1840,6 +1862,38 @@ "dev": true, "license": "MIT" }, + "node_modules/@types/express": { + "version": "5.0.6", + "resolved": "https://registry.npmjs.org/@types/express/-/express-5.0.6.tgz", + "integrity": "sha512-sKYVuV7Sv9fbPIt/442koC7+IIwK5olP1KWeD88e/idgoJqDm3JV/YUiPwkoKK92ylff2MGxSz1CSjsXelx0YA==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/body-parser": "*", + "@types/express-serve-static-core": "^5.0.0", + "@types/serve-static": "^2" + } + }, + "node_modules/@types/express-serve-static-core": { + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/@types/express-serve-static-core/-/express-serve-static-core-5.1.2.tgz", + "integrity": "sha512-d3KvEXBSo/lOAMc2u6fkyDHBvetBHeqD7wm/AcXfLpSOQwlmG9D/aQ0SFswVjv05p7ullQS7Mjohj6/VdbZuTg==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/node": "*", + "@types/qs": "*", + "@types/range-parser": "*", + "@types/send": "*" + } + }, + "node_modules/@types/http-errors": { + "version": "2.0.5", + "resolved": "https://registry.npmjs.org/@types/http-errors/-/http-errors-2.0.5.tgz", + "integrity": "sha512-r8Tayk8HJnX0FztbZN7oVqGccWgw98T/0neJphO91KkmOzug1KkofZURD4UaD5uH8AqcFLfdPErnBod0u71/qg==", + "dev": true, + "license": "MIT" + }, "node_modules/@types/json-schema": { "version": "7.0.15", "resolved": "https://registry.npmjs.org/@types/json-schema/-/json-schema-7.0.15.tgz", @@ -1857,6 +1911,41 @@ "undici-types": "~7.18.0" } }, + "node_modules/@types/qs": { + "version": "6.15.1", + "resolved": "https://registry.npmjs.org/@types/qs/-/qs-6.15.1.tgz", + "integrity": "sha512-GZHUBZR9hckSUhrxmp1nG6NwdpM9fCunJwyThLW1X3AyHgd9IlHb6VANpQQqDr2o/qQp6McZ3y/IA2rVzKzSbw==", + "dev": true, + "license": "MIT" + }, + "node_modules/@types/range-parser": { + "version": "1.2.7", + "resolved": "https://registry.npmjs.org/@types/range-parser/-/range-parser-1.2.7.tgz", + "integrity": "sha512-hKormJbkJqzQGhziax5PItDUTMAM9uE2XXQmM37dyd4hVM+5aVl7oVxMVUiVQn2oCQFN/LKCZdvSM0pFRqbSmQ==", + "dev": true, + "license": "MIT" + }, + "node_modules/@types/send": { + "version": "1.2.1", + "resolved": "https://registry.npmjs.org/@types/send/-/send-1.2.1.tgz", + "integrity": "sha512-arsCikDvlU99zl1g69TcAB3mzZPpxgw0UQnaHeC1Nwb015xp8bknZv5rIfri9xTOcMuaVgvabfIRA7PSZVuZIQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/node": "*" + } + }, + "node_modules/@types/serve-static": { + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/@types/serve-static/-/serve-static-2.2.0.tgz", + "integrity": "sha512-8mam4H1NHLtu7nmtalF7eyBH14QyOASmcxHhSfEoRyr0nP/YdoesEtU+uSRvMe96TW/HPTtkoKqQLl53N7UXMQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/http-errors": "*", + "@types/node": "*" + } + }, "node_modules/@typescript-eslint/eslint-plugin": { "version": "8.61.1", "resolved": "https://registry.npmjs.org/@typescript-eslint/eslint-plugin/-/eslint-plugin-8.61.1.tgz", diff --git a/clients/cli/package.json b/clients/cli/package.json index 221215ff0..eb37684fb 100644 --- a/clients/cli/package.json +++ b/clients/cli/package.json @@ -16,7 +16,7 @@ ], "scripts": { "build": "tsup", - "typecheck": "tsc --noEmit -p tsconfig.json", + "typecheck": "tsc --noEmit -p tsconfig.json && tsc --noEmit -p tsconfig.test.json", "validate": "npm run format:check && npm run lint && npm run typecheck && npm run test", "test": "vitest run", "test:watch": "vitest", @@ -47,6 +47,7 @@ }, "devDependencies": { "@eslint/js": "^9.39.4", + "@types/express": "^5.0.6", "@types/node": "^24.12.4", "@vitest/coverage-v8": "^4.1.0", "eslint": "^9.39.4", diff --git a/clients/cli/tsconfig.json b/clients/cli/tsconfig.json index 522a3ebe2..31e268fdb 100644 --- a/clients/cli/tsconfig.json +++ b/clients/cli/tsconfig.json @@ -20,6 +20,6 @@ "@inspector/core/*": ["../../core/*"] } }, - "include": ["src/**/*"], - "exclude": ["node_modules", "**/*.test.ts", "**/*.test.tsx", "build"] + "include": ["src/**/*", "vitest.config.ts", "tsup.config.ts"], + "exclude": ["node_modules", "**/*.test.*", "build"] } diff --git a/clients/cli/tsconfig.test.json b/clients/cli/tsconfig.test.json new file mode 100644 index 000000000..22a45ff2a --- /dev/null +++ b/clients/cli/tsconfig.test.json @@ -0,0 +1,33 @@ +{ + // Typecheck the __tests__ dir (the src-only tsconfig.json excludes tests). + // Mirrors clients/web/tsconfig.test.json: the test-server barrel is aliased + // to its source and the module paths below resolve what vitest resolves via + // vitest.shared.mts's projectResolve, so tsc validates the tests against the + // same graph the runner executes. See #1791. + "extends": "./tsconfig.json", + "compilerOptions": { + "types": ["node", "express"], + "paths": { + "@inspector/core/*": ["../../core/*"], + "@modelcontextprotocol/inspector-test-server": [ + "../../test-servers/src/index.ts" + ], + "express": ["./node_modules/@types/express"], + "vitest": ["./node_modules/vitest"] + } + }, + // Only the tests root the project; tsc pulls in the `src` they import. The + // src-only tsconfig.json already validates all of `src` (without the + // test-only aliases), so listing it here too would just check it twice. + // + // `server-composable.ts` is listed explicitly because it's a `test-servers` + // bin entry nothing imports — the barrel alias above pulls in the other 13 + // `test-servers/src` files transitively, but not this one, and its own build + // (`test-servers:build`) runs `tsc --noCheck`, so without this line it would + // get no tsc pass at all yet still be emitted into `test-servers/build`. + "include": [ + "__tests__/**/*", + "../../test-servers/src/server-composable.ts" + ], + "exclude": ["node_modules", "build"] +} diff --git a/clients/launcher/package.json b/clients/launcher/package.json index f0602565a..f39eecdd0 100644 --- a/clients/launcher/package.json +++ b/clients/launcher/package.json @@ -15,11 +15,12 @@ "build": "tsc", "postbuild": "node scripts/make-executable.js", "lint": "eslint .", + "typecheck": "tsc --noEmit -p tsconfig.json && tsc --noEmit -p tsconfig.test.json", "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" + "validate": "npm run format:check && npm run lint && npm run typecheck && npm run build && npm run test" }, "dependencies": { "commander": "^13.1.0" diff --git a/clients/launcher/tsconfig.json b/clients/launcher/tsconfig.json index 4b705d67e..3e0379729 100644 --- a/clients/launcher/tsconfig.json +++ b/clients/launcher/tsconfig.json @@ -9,5 +9,10 @@ "skipLibCheck": true }, "include": ["src/**/*"], - "exclude": ["node_modules", "build"] + // Exclude tests so `build` (an emitting `tsc`) never compiles a co-located + // `src/**/*.test.*` into `build/` — which ships in the published tarball. It + // also means such a test lands in no tsconfig project, so verify:typecheck- + // coverage catches it, matching cli/tui. (Tests live in `__tests__/`, checked + // via tsconfig.test.json.) + "exclude": ["node_modules", "build", "**/*.test.*"] } diff --git a/clients/launcher/tsconfig.test.json b/clients/launcher/tsconfig.test.json new file mode 100644 index 000000000..19cbf0765 --- /dev/null +++ b/clients/launcher/tsconfig.test.json @@ -0,0 +1,16 @@ +{ + // Typecheck the __tests__ dir. The build config (`tsconfig.json`) emits with + // `rootDir: ./src`, so the tests can't join it — this is a `noEmit` sibling + // rooted at the package so it can reach both `__tests__` and the `src` the + // tests import. See #1791. + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true, + "rootDir": ".", + "types": ["node"] + }, + // `vitest.config.ts` lives here (not the src project) because the build + // config's `rootDir: ./src` rejects it — same reason as the tests. + "include": ["__tests__/**/*", "vitest.config.ts"], + "exclude": ["node_modules", "build"] +} diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index 5fd2c5e39..d70b9c274 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -48,21 +48,38 @@ const h = vi.hoisted(() => { const openUrl = vi.fn().mockResolvedValue(undefined); // Shared OAuth-related spies so a test can configure resolve/reject and // assert calls regardless of which per-server FakeClient instance App built. + // Each spy is typed against the real InspectorClient method signature so its + // implementation and `mockResolvedValue` / return payloads stay in sync with + // the client (this is what keeps a stale `{ kind: "satisfied" }` literal from + // narrowing `handleAuthChallenge`'s return). Note vitest does NOT type-check + // `toHaveBeenCalledWith(...)` arguments against the mock's signature, so those + // assertions stay runtime-only. The FakeClient wrappers below forward the same + // `Parameters<…>` tuple, which spreads cleanly (a tuple, not `unknown[]`). const clientSpies = { - authenticate: vi.fn( - async (): Promise => "https://auth.example/start", + authenticate: vi.fn( + async () => new URL("https://auth.example/start"), + ), + clearOAuthTokens: vi.fn( + async () => {}, + ), + completeOAuthFlow: vi.fn( + async () => {}, + ), + getOAuthState: vi.fn( + async () => undefined, + ), + callTool: vi.fn(), + checkAuthChallengeSatisfied: vi.fn< + InspectorClient["checkAuthChallengeSatisfied"] + >(async () => false), + handleAuthChallenge: vi.fn( + async () => ({ kind: "satisfied" }), ), - clearOAuthTokens: vi.fn(), - completeOAuthFlow: vi.fn(async (): Promise => {}), - getOAuthState: vi.fn(async () => undefined), - callTool: vi.fn(), - checkAuthChallengeSatisfied: vi.fn(async () => false), - handleAuthChallenge: vi.fn(async () => ({ kind: "satisfied" as const })), }; // Captured options from the most recent callbackServer.start(), so a test can // drive the onCallback / onError handlers the OAuth flows register. interface CallbackOpts { - onCallback: (p: { code: string }) => Promise | void; + onCallback: (p: { code: string; iss?: string }) => Promise | void; onError: (p: { error?: string; error_description?: string }) => void; } const cb: { opts: CallbackOpts | null } = { opts: null }; @@ -124,16 +141,24 @@ const h = vi.hoisted(() => { | "sse" | "streamable-http", ); - authenticate = (...a: unknown[]) => clientSpies.authenticate(...a); - clearOAuthTokens = (...a: unknown[]) => clientSpies.clearOAuthTokens(...a); - completeOAuthFlow = (...a: unknown[]) => - clientSpies.completeOAuthFlow(...a); - getOAuthState = (...a: unknown[]) => clientSpies.getOAuthState(...a); - callTool = (...a: unknown[]) => clientSpies.callTool(...a); - checkAuthChallengeSatisfied = (...a: unknown[]) => - clientSpies.checkAuthChallengeSatisfied(...a); - handleAuthChallenge = (...a: unknown[]) => - clientSpies.handleAuthChallenge(...a); + authenticate = (...a: Parameters) => + clientSpies.authenticate(...a); + clearOAuthTokens = ( + ...a: Parameters + ) => clientSpies.clearOAuthTokens(...a); + completeOAuthFlow = ( + ...a: Parameters + ) => clientSpies.completeOAuthFlow(...a); + getOAuthState = (...a: Parameters) => + clientSpies.getOAuthState(...a); + callTool = (...a: Parameters) => + clientSpies.callTool(...a); + checkAuthChallengeSatisfied = ( + ...a: Parameters + ) => clientSpies.checkAuthChallengeSatisfied(...a); + handleAuthChallenge = ( + ...a: Parameters + ) => clientSpies.handleAuthChallenge(...a); readResource = vi.fn(async () => ({ result: { contents: [{ uri: "file://x", text: "hello" }] }, })); @@ -263,6 +288,7 @@ vi.mock("../src/utils/openUrl.js", () => ({ })); import App from "../src/App.js"; +import type { InspectorClient } from "@inspector/core/mcp/index.js"; import type { TuiServer } from "../src/tui-servers.js"; import { AuthRecoveryRequiredError, @@ -581,7 +607,9 @@ beforeEach(() => { h.clientInstances.length = 0; h.runner.override = null; h.clientSpies.authenticate.mockReset(); - h.clientSpies.authenticate.mockResolvedValue("https://auth.example/start"); + h.clientSpies.authenticate.mockResolvedValue( + new URL("https://auth.example/start"), + ); h.clientSpies.clearOAuthTokens.mockReset(); h.clientSpies.completeOAuthFlow.mockReset(); h.clientSpies.completeOAuthFlow.mockResolvedValue(undefined); diff --git a/clients/tui/__tests__/AuthTab.test.tsx b/clients/tui/__tests__/AuthTab.test.tsx index d12f16f39..86d6b18eb 100644 --- a/clients/tui/__tests__/AuthTab.test.tsx +++ b/clients/tui/__tests__/AuthTab.test.tsx @@ -26,13 +26,17 @@ const sampleOAuthState: OAuthConnectionState = { client: { clientId: "abc123", registrationKind: "dcr", + hasClientSecret: false, }, tokens: { access_token: "tok-abcdefghijklmnopqrstuvwxyz", token_type: "Bearer", }, authorizationServerMetadata: { + issuer: "https://auth.example.com", authorization_endpoint: "https://auth.example.com/authorize", + token_endpoint: "https://auth.example.com/token", + response_types_supported: ["code"], }, configuredScope: "read write", }; diff --git a/clients/tui/__tests__/HistoryTab.test.tsx b/clients/tui/__tests__/HistoryTab.test.tsx index 72c53dc82..4710402d5 100644 --- a/clients/tui/__tests__/HistoryTab.test.tsx +++ b/clients/tui/__tests__/HistoryTab.test.tsx @@ -28,14 +28,27 @@ const PAGE_DOWN = `${ESC}[6~`; const ts = new Date("2024-01-01T12:34:56Z"); -const entry = (over: Partial): MessageEntry => - ({ - id: "id", - timestamp: ts, - direction: "request", - message: { jsonrpc: "2.0", id: 1, method: "ping" }, - ...over, - }) as unknown as MessageEntry; +// `message` stays fully typed so a malformed field in a well-formed fixture is +// caught. The two intentionally-malformed messages below (a response with +// neither result nor error; a notification with no method) exercise HistoryTab's +// defensive branches and cannot be represented by the strict JSONRPCMessage +// union, so they carry a local `MALFORMED` cast at their own call sites rather +// than loosening the whole helper. +const entry = (over: Partial): MessageEntry => ({ + id: "id", + timestamp: ts, + direction: "request", + message: { jsonrpc: "2.0", id: 1, method: "ping" }, + ...over, +}); + +// Cast for the two intentionally-malformed wire messages only, scoped to a named +// helper so `entry()`'s `message` stays gate-checked for every well-formed +// fixture. See the comment on `entry` above. +const MALFORMED = (m: { + jsonrpc: "2.0"; + id?: number; +}): MessageEntry["message"] => m as MessageEntry["message"]; // One entry exercising each label / direction / detail branch. const reqWithResponse = entry({ @@ -63,7 +76,7 @@ const respError = entry({ const respPlain = entry({ id: "m4", direction: "response", - message: { jsonrpc: "2.0", id: 5 }, + message: MALFORMED({ jsonrpc: "2.0", id: 5 }), }); const notification = entry({ id: "m5", @@ -73,7 +86,7 @@ const notification = entry({ const unknownEntry = entry({ id: "m6", direction: "notification", - message: { jsonrpc: "2.0" }, + message: MALFORMED({ jsonrpc: "2.0" }), }); const allMessages: MessageEntry[] = [ diff --git a/clients/tui/__tests__/helpers/oauth-test-fakes.ts b/clients/tui/__tests__/helpers/oauth-test-fakes.ts new file mode 100644 index 000000000..efff339e1 --- /dev/null +++ b/clients/tui/__tests__/helpers/oauth-test-fakes.ts @@ -0,0 +1,31 @@ +import { + DEFAULT_MAX_FETCH_REQUESTS, + DEFAULT_TASK_TTL_MS, +} from "@inspector/core/mcp/types.js"; +import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; + +/** + * A full {@link InspectorServerSettings} with representative defaults for its + * required fields (the timeouts at 0 = "SDK default", `taskTtl` / + * `maxFetchRequests` at their product defaults, empty lists elsewhere). Lets a + * test supply only the field(s) under test (e.g. `{ enterpriseManaged: true }`) + * while `tsc` still sees a complete object, avoiding `as unknown as` casts (see + * #1791 and the AGENTS.md `as`-cast policy). As a `Partial<…>` with + * `exactOptionalPropertyTypes` off, `overrides` also admits an explicit + * `undefined` for a required field, so don't rely on it to reject one. + */ +export function makeFakeServerSettings( + overrides: Partial = {}, +): InspectorServerSettings { + return { + headers: [], + metadata: [], + env: [], + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: DEFAULT_TASK_TTL_MS, + maxFetchRequests: DEFAULT_MAX_FETCH_REQUESTS, + roots: [], + ...overrides, + }; +} diff --git a/clients/tui/__tests__/tuiOAuth.test.ts b/clients/tui/__tests__/tuiOAuth.test.ts index 125c57f4b..92da7d7c9 100644 --- a/clients/tui/__tests__/tuiOAuth.test.ts +++ b/clients/tui/__tests__/tuiOAuth.test.ts @@ -5,19 +5,20 @@ import { stepUpConfirmMessage, stepUpInsufficientScopeMessage, } from "../src/utils/tuiOAuth.js"; +import { makeFakeServerSettings } from "./helpers/oauth-test-fakes.js"; describe("tuiOAuth", () => { it("detects standard OAuth step-up", () => { expect( isStandardOAuthStepUp( { reason: "insufficient_scope", requiredScopes: ["weather:read"] }, - { enterpriseManaged: false }, + makeFakeServerSettings({ enterpriseManaged: false }), ), ).toBe(true); expect( isStandardOAuthStepUp( { reason: "insufficient_scope", requiredScopes: ["weather:read"] }, - { enterpriseManaged: true }, + makeFakeServerSettings({ enterpriseManaged: true }), ), ).toBe(false); }); @@ -26,13 +27,13 @@ describe("tuiOAuth", () => { expect( isStepUpConfirmation( { reason: "insufficient_scope", requiredScopes: ["env:read"] }, - { enterpriseManaged: true }, + makeFakeServerSettings({ enterpriseManaged: true }), ), ).toBe(true); expect( isStepUpConfirmation( { reason: "insufficient_scope", requiredScopes: ["weather:read"] }, - { enterpriseManaged: false }, + makeFakeServerSettings({ enterpriseManaged: false }), ), ).toBe(true); }); diff --git a/clients/tui/package.json b/clients/tui/package.json index 005801f47..7799b1f7d 100644 --- a/clients/tui/package.json +++ b/clients/tui/package.json @@ -18,7 +18,7 @@ "scripts": { "dev": "vite-node --config vitest.config.ts dev.ts", "build": "tsup", - "typecheck": "tsc --noEmit -p tsconfig.json", + "typecheck": "tsc --noEmit -p tsconfig.json && tsc --noEmit -p tsconfig.test.json", "validate": "npm run format:check && npm run lint && npm run typecheck && npm run build && npm run test", "test": "vitest run", "test:coverage": "vitest run --coverage", diff --git a/clients/tui/tsconfig.json b/clients/tui/tsconfig.json index 1d5ac9cac..31997f4e8 100644 --- a/clients/tui/tsconfig.json +++ b/clients/tui/tsconfig.json @@ -25,6 +25,13 @@ "react/jsx-runtime": ["./node_modules/@types/react/jsx-runtime"] } }, - "include": ["index.ts", "tui.tsx", "src/**/*"], - "exclude": ["node_modules", "**/*.test.ts", "**/*.test.tsx", "build"] + "include": [ + "index.ts", + "tui.tsx", + "src/**/*", + "dev.ts", + "vitest.config.ts", + "tsup.config.ts" + ], + "exclude": ["node_modules", "**/*.test.*", "build"] } diff --git a/clients/tui/tsconfig.test.json b/clients/tui/tsconfig.test.json new file mode 100644 index 000000000..a1cd82c9b --- /dev/null +++ b/clients/tui/tsconfig.test.json @@ -0,0 +1,21 @@ +{ + // Typecheck the __tests__ dir (the src-only tsconfig.json excludes tests). + // The tui tests don't import the test-server package, so this only re-adds + // the `@inspector/core/*` deep-path resolution and the react redirects the + // base config already carries, extended to include __tests__. See #1791. + "extends": "./tsconfig.json", + "compilerOptions": { + "paths": { + "@inspector/core/*": ["../../core/*"], + "react": ["./node_modules/@types/react"], + "react/jsx-runtime": ["./node_modules/@types/react/jsx-runtime"], + "vitest": ["./node_modules/vitest"] + } + }, + // Only the tests root the project; tsc pulls in the `src` they import. The + // src-only tsconfig.json already validates `src` (and index.ts / tui.tsx) + // without the test-only aliases, so listing them here too would just check + // them twice. + "include": ["__tests__/**/*"], + "exclude": ["node_modules", "build"] +} diff --git a/core/mcp/remote/createRemoteTransport.ts b/core/mcp/remote/createRemoteTransport.ts index 5e720c530..96803d8b2 100644 --- a/core/mcp/remote/createRemoteTransport.ts +++ b/core/mcp/remote/createRemoteTransport.ts @@ -26,7 +26,7 @@ export interface RemoteTransportFactoryOptions { * connecting to the given remote server. * * @example - * import { API_SERVER_ENV_VARS } from '@modelcontextprotocol/inspector-core/mcp/remote'; + * import { API_SERVER_ENV_VARS } from '@inspector/core/mcp/remote/constants.js'; * const createTransport = createRemoteTransport({ * baseUrl: 'http://localhost:3000', * authToken: process.env[API_SERVER_ENV_VARS.AUTH_TOKEN], diff --git a/package.json b/package.json index dcd710d5f..45ed8a9eb 100644 --- a/package.json +++ b/package.json @@ -40,7 +40,9 @@ "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 verify:format-coverage && npm run validate:core && npm run validate:web && npm run validate:cli && npm run validate:tui && npm run validate:launcher", + "verify:typecheck-coverage": "node scripts/verify-typecheck-coverage.mjs", + "test:scripts": "node --test \"scripts/**/*.test.mjs\"", + "validate": "npm run verify:format-coverage && npm run verify:typecheck-coverage && npm run test:scripts && 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}\"", diff --git a/scripts/lib/npm-scripts.mjs b/scripts/lib/npm-scripts.mjs new file mode 100644 index 000000000..fbb616de5 --- /dev/null +++ b/scripts/lib/npm-scripts.mjs @@ -0,0 +1,89 @@ +// Shared npm-script reachability helpers used by both coverage guards +// (`verify-format-coverage.mjs`, `verify-typecheck-coverage.mjs`). Extracted so +// the wiring logic both depend on — "is this gate actually run?" — can't drift +// between them (the same rationale `scripts/lib/prod-web-server.mjs` was +// extracted under). + +/** + * Split a shell-ish command string into args, honoring single and double quotes + * (a quoted arg becomes one token with the quotes stripped). Used by both + * coverage guards so they parse a script's args — quoting is the house style + * here (a quoted prettier glob, `tsc -p "tsconfig.json"`) — the same way. + */ +export function tokenize(command) { + const tokens = []; + const re = /"([^"]*)"|'([^']*)'|(\S+)/g; + let m; + while ((m = re.exec(command)) !== null) { + tokens.push(m[1] ?? m[2] ?? m[3]); + } + return tokens; +} + +/** + * Names of scripts transitively reachable from `entry` by following `npm run + * ` references within a single manifest's `scripts`, plus npm's implicit + * `pre`/`post` lifecycle hooks (npm runs those around `` + * without an explicit `npm run`, so a gate moved into e.g. `prevalidate` is + * still reached). A gate harvested from a script that nothing reachable from + * `entry` invokes gates nothing, so callers restrict to this set to assert "CI + * actually runs this", not merely "the script exists". + */ +export 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); + // npm runs pre/post implicitly around . + for (const hook of [`pre${name}`, `post${name}`]) + if (typeof scripts?.[hook] === "string") queue.push(hook); + const cmd = scripts?.[name]; + if (typeof cmd !== "string") continue; + for (const m of cmd.matchAll(runRef)) queue.push(m[1]); + } + return reached; +} + +/** The command strings of every script reachable from the root `validate`. */ +function rootReachedCommands(rootScripts) { + return [...reachableScripts(rootScripts)] + .map((n) => rootScripts?.[n]) + .filter((c) => typeof c === "string"); +} + +/** + * Whether the root `validate` chain runs a client's `validate` — either + * `cd && npm run validate` or `npm --prefix run + * validate`. Without this a per-client gate would still be harvested from that + * client's own `validate` and count as coverage even after the root chain + * stopped running it — the "gate silently stops gating" failure, one level up. + */ +export function rootRunsClientValidate(rootScripts, clientDir) { + // Anchored on both ends: an optional leading `./` (`cd ./clients/x` genuinely + // runs the client) and a boundary after the dir, so a prefix-sibling + // (`clients/x` vs `clients/x-next`) can't satisfy the check for the shorter + // name; and a boundary after `validate` so `run validate:fast` (a *different* + // script that may skip typecheck) doesn't count as running `validate`. A bare + // `includes`/`\b` on either would let a sibling silently vouch for a dropped + // client. Quotes are stripped first so `cd "clients/x"` still matches. Both + // `cd && npm run validate` and `npm --prefix run validate` count. + const escaped = clientDir.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const dirRe = new RegExp(`(?:cd|--prefix) \\.?/?${escaped}(?=$|[\\s&;])`); + return rootReachedCommands(rootScripts).some((c) => { + const stripped = c.replace(/["']/g, ""); // both halves see quotes stripped + return dirRe.test(stripped) && /\brun validate(?=$|[\s&;])/.test(stripped); + }); +} + +/** + * Whether `scriptName` is reachable from the root `validate`. A guard can't + * assert it is *itself* run (an unrun guard runs no check), but the two coverage + * guards can each assert the *other* is still wired — so dropping either from + * `validate` is caught by its sibling. Only deleting both slips through. + */ +export function rootReachesScript(rootScripts, scriptName) { + return reachableScripts(rootScripts, "validate").has(scriptName); +} diff --git a/scripts/lib/npm-scripts.test.mjs b/scripts/lib/npm-scripts.test.mjs new file mode 100644 index 000000000..a9408a6e1 --- /dev/null +++ b/scripts/lib/npm-scripts.test.mjs @@ -0,0 +1,125 @@ +// Table-driven tests for the shared npm-script reachability helpers. Each case +// pins a rule the #1799 review took a round to get right — the comment names it. +// Run via `npm run test:scripts` (node:test; the root has no vitest harness). + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + reachableScripts, + rootReachesScript, + rootRunsClientValidate, + tokenize, +} from "./npm-scripts.mjs"; + +test("tokenize: quoting (r17)", () => { + const cases = [ + [ + "tsc --noEmit -p tsconfig.json", + ["tsc", "--noEmit", "-p", "tsconfig.json"], + ], + [`tsc -p "tsconfig.test.json"`, ["tsc", "-p", "tsconfig.test.json"]], + [`tsc -p 'tsconfig.test.json'`, ["tsc", "-p", "tsconfig.test.json"]], + [ + `prettier --check "core/**/*.ts"`, + ["prettier", "--check", "core/**/*.ts"], + ], + [" spaced out ", ["spaced", "out"]], + ["", []], + ]; + for (const [input, expected] of cases) + assert.deepEqual(tokenize(input), expected, input); +}); + +test("reachableScripts: follows `npm run` refs and pre/post hooks (r10)", () => { + const scripts = { + validate: "npm run lint && npm run build", + lint: "eslint .", + build: "tsc", + prebuild: "echo pre", // implicit hook npm runs around `build` + postbuild: "echo post", + unrelated: "noop", + }; + const reached = reachableScripts(scripts, "validate"); + assert.ok(reached.has("validate")); + assert.ok(reached.has("lint")); + assert.ok(reached.has("build")); + assert.ok(reached.has("prebuild"), "pre hook"); + assert.ok(reached.has("postbuild"), "post hook"); + assert.ok(!reached.has("unrelated")); +}); + +test("reachableScripts: a `prevalidate`-hosted typecheck is reachable (r10)", () => { + const scripts = { + validate: "npm run build && npm run test", + prevalidate: "npm run typecheck", + typecheck: "tsc --noEmit", + build: "tsc", + test: "vitest run", + }; + assert.ok(reachableScripts(scripts, "validate").has("typecheck")); +}); + +test("rootRunsClientValidate: forms that count vs. don't", () => { + // `cmd` lives in `vt`, reached from `validate` via `npm run vt`. + const runs = (cmd) => + rootRunsClientValidate({ validate: "npm run vt", vt: cmd }, "clients/tui"); + // Counts: + assert.ok(runs("cd clients/tui && npm run validate"), "plain cd"); + assert.ok(runs("cd ./clients/tui && npm run validate"), "leading ./ (r15)"); + assert.ok(runs(`cd "clients/tui" && npm run validate`), "quoted dir (r17)"); + assert.ok( + runs(`cd clients/tui && npm run "validate"`), + "quoted script name (r18)", + ); + assert.ok( + runs("cd clients/tui && npm run build && npm run validate"), + "extra step", + ); + assert.ok(runs("npm --prefix clients/tui run validate"), "--prefix (r19)"); + // Does NOT count: + assert.ok( + !runs("cd clients/tui-next && npm run validate"), + "prefix-sibling must not match the shorter name (r16)", + ); + assert.ok( + !runs("cd clients/tui && npm run validate:fast"), + "`run validate:fast` is a different script (r20)", + ); + assert.ok(!runs("cd clients/tui && npm run build"), "no validate"); +}); + +test("rootRunsClientValidate: only counts a reachable script (r5)", () => { + // A cd-validate call in a script nothing reachable from `validate` runs must + // NOT count — that's the whole reason the reachability restriction exists. + assert.ok( + !rootRunsClientValidate( + { + validate: "npm run something-else", + orphan: "cd clients/tui && npm run validate", + }, + "clients/tui", + ), + "orphan script isn't reachable from validate", + ); + // Reached via one hop of indirection counts. + assert.ok( + rootRunsClientValidate( + { + validate: "npm run validate:tui", + "validate:tui": "cd clients/tui && npm run validate", + }, + "clients/tui", + ), + "reached via validate:tui", + ); +}); + +test("rootReachesScript: sibling-guard vouching", () => { + const scripts = { + validate: "npm run verify:format-coverage && npm run validate:web", + "validate:web": "cd clients/web && npm run validate", + }; + assert.ok(rootReachesScript(scripts, "verify:format-coverage")); + assert.ok(rootReachesScript(scripts, "validate:web")); + assert.ok(!rootReachesScript(scripts, "verify:typecheck-coverage")); +}); diff --git a/scripts/verify-format-coverage.mjs b/scripts/verify-format-coverage.mjs index 13fca8299..8963c8c24 100644 --- a/scripts/verify-format-coverage.mjs +++ b/scripts/verify-format-coverage.mjs @@ -17,6 +17,12 @@ import { readFileSync } from "node:fs"; import { execFileSync } from "node:child_process"; import path from "node:path"; import { fileURLToPath } from "node:url"; +import { + reachableScripts, + rootReachesScript, + rootRunsClientValidate, + tokenize, +} from "./lib/npm-scripts.mjs"; const repoRoot = path.resolve( path.dirname(fileURLToPath(import.meta.url)), @@ -49,39 +55,6 @@ const MANIFESTS = [ "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; -} - -/** - * 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 @@ -159,15 +132,8 @@ 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), - ), + (dir) => !rootRunsClientValidate(rootPkg.scripts, dir), ); } @@ -200,6 +166,19 @@ function trackedSourceFiles() { return out.split("\n").filter(Boolean); } +// Vouch for the sibling guard: a guard can't detect being unrun itself, but the +// two coverage guards can each assert the other is still wired into `validate`, +// so dropping either is caught here. Only deleting both slips through. +const rootScripts = JSON.parse( + readFileSync(path.join(repoRoot, "package.json"), "utf8"), +).scripts; +if (!rootReachesScript(rootScripts, "verify:typecheck-coverage")) { + console.error( + "verify:format-coverage — the root `validate` no longer runs `verify:typecheck-coverage` (its sibling guard). Restore it.", + ); + process.exit(1); +} + const unreachedClients = clientsUnreachedFromRoot(); if (unreachedClients.length > 0) { console.error( diff --git a/scripts/verify-typecheck-coverage.mjs b/scripts/verify-typecheck-coverage.mjs new file mode 100644 index 000000000..20d6ab2cc --- /dev/null +++ b/scripts/verify-typecheck-coverage.mjs @@ -0,0 +1,896 @@ +#!/usr/bin/env node +// Durable guard for the "every tracked source file gets a `tsc` pass" invariant +// that #1791 established for the Node clients — every `clients/*`, enrolled via +// its `typecheck` script's projects (cli, tui, launcher) or, for a client with +// no `typecheck` script (clients/web), via its `tsconfig.json` `references`; a +// new client is picked up from disk automatically (nodeClients). Either way a +// `tsc -b` solution config is reduced to the real projects it references +// (resolveLeafProjects), so coverage and the non-inertness check both follow it. +// A tsconfig +// project only typechecks the files its `include`/`files` name plus whatever +// those transitively import — so a new top-level `.ts` (a fresh config file, a +// new test helper) can silently fall outside every project and get no +// type-checking, exactly the hole that surfaced twice during #1791's review +// (`launcher/__tests__`, then `launcher/vitest.config.ts`). launcher is the most +// exposed: its build config's `rootDir: "./src"` actively rejects anything at +// the package root. +// +// This is the typecheck-coverage analog of `verify:format-coverage`: for each +// client it runs every project its `typecheck` script names with `tsc +// --listFilesOnly` (the accurate measure — it includes import-reached files), +// unions the result, and asserts every tracked first-party `.ts`/`.tsx`/`.mts`/ +// `.cts` under the client is in that union. It also covers, deny-by-default, the +// tracked TS that lives OUTSIDE any client — the shared surface (`test-servers/ +// src`, `vitest.shared.mts`), all of `core/` (covered by web's enrolled `tsc -b` +// projects), plus anything at a new top-level location — requiring each to land +// in the global union of client projects. Exits non-zero, listing the offenders, +// on any miss. +// +// Like its sibling it also asserts the gate is actually WIRED: each client's +// `typecheck` must be reachable from that client's `validate`, and the root +// `validate` chain must invoke each client's `validate` — otherwise the guard +// could measure a `typecheck` script that CI no longer runs and stay green while +// nothing is typechecked ("gate silently stops gating"). +// +// Source of truth is the `typecheck` scripts themselves — this parser reads the +// `-p`/`--project`/`-b`/`--build` args (and the implicit `./tsconfig.json` a +// bare `tsc` resolves) out of every script reachable from `typecheck`, so +// adding/removing a project is reflected here with no second list to keep in sync. + +import { execFileSync } from "node:child_process"; +import { readdirSync, readFileSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; +import { + reachableScripts, + rootReachesScript, + rootRunsClientValidate, + tokenize, +} from "./lib/npm-scripts.mjs"; + +const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "..", +); + +// Clients this guard deliberately does NOT gate, with the reason. The escape +// hatch for a genuinely-ungateable client. Empty today: `clients/web` used to be +// here (it has no `typecheck` script — its `build` runs `tsc -b`) but is now +// ENROLLED through its `tsconfig.json` `references`, so its package-root files +// are checked rather than whole-tree-exempted (an exemption wider than the gate +// it defers to — #1799 review r24). +const EXEMPT = new Map(); + +// TypeScript source extensions this guard requires a tsc pass for. Matches its +// sibling's TS set (`verify-format-coverage.mjs` — `.mts` is already the idiom +// for shared config here, e.g. `vitest.shared.mts`, so a client `.mts`/`.cts` +// must be gated too). Ambient declaration files are excluded: an unreferenced +// `*.d.{ts,mts,cts}` shim (e.g. web's `vitest.shims.d.ts`) is type-only and not +// a real gap. There are no client `.mts`/`.cts` today; this pre-empts one. +export const isRequiredSource = (rel) => + /\.(ts|tsx|mts|cts)$/.test(rel) && !/\.d\.(ts|mts|cts)$/.test(rel); + +// Match `tsc` by token basename so a path-invoked binary (`node_modules/.bin/ +// tsc`, `./node_modules/.bin/tsc.cmd`) counts, not just the bare `tsc` token. +export const isTsc = (t) => /(?:^|[\\/])tsc(?:\.(?:cmd|exe|ps1))?$/.test(t); + +// A flag that makes a `tsc` pass list files without type-checking them (so it +// gates nothing). Case-insensitive — tsc's own option parsing is. Shared by the +// `typecheck`-script path and the reference (`tsc -b`) path. +export const isDisablingFlag = (t) => /^--(noCheck|listFilesOnly)$/i.test(t); + +/** + * Whether a tracked test file is matched by one of the `test:scripts` command's + * globs. Delegates to node's own `path.matchesGlob` (22.5.0+; the repo floor is + * `>=22.19.0`) rather than hand-translating the glob to a RegExp — `node --test` + * is the thing whose matching we're modeling, so measuring with node's matcher + * is exact for every form it accepts (`**` incl. zero depth, `*`, `?` as exactly + * one non-separator char, brace alternation, character classes) instead of + * false-failing on whichever form the translator hadn't covered yet. + */ +export const matchesTestGlob = (file, glob) => path.matchesGlob(file, glob); + +// `node --test` flags that make it execute only PART of the suite its globs +// match, while still exiting 0. This is `--noCheck` for the test runner: the +// gate runs, but gates less than it appears to. +const NARROWING_TEST_FLAGS = new Set([ + "--test-shard", + "--test-only", + "--test-name-pattern", + "--test-skip-pattern", +]); + +// `node --test` flags whose VALUE is a separate token — never a positional glob. +const VALUE_TAKING_TEST_FLAGS = new Set([ + "--test-coverage-include", + "--test-coverage-exclude", + "--test-reporter", + "--test-reporter-destination", + "--test-name-pattern", + "--test-skip-pattern", + "--test-shard", + // Threshold flags: numeric values, so harvesting one is inert rather than a + // suppression (a number can't be a BROADER glob) — listed for completeness. + "--test-coverage-lines", + "--test-coverage-branches", + "--test-coverage-functions", +]); + +/** + * The path/glob arguments `test:scripts` hands `node --test`, harvested across + * every script reachable FROM it — not just its own literal string. A delegating + * `test:scripts` ("npm run test:scripts:lib && npm run test:scripts:guard", the + * way other scripts here split) genuinely runs every test, and reading the one + * string would instead harvest `npm`/`run`/`test:scripts:lib` and report each + * test file as unmatched with unfollowable advice. Reachability also picks up a + * `pretest:scripts` hook for free. + * + * Scoped per COMMAND SEGMENT to the ones that actually invoke `node --test`, + * exactly as `typecheckProjects` gates on `tokens.some(isTsc)` before harvesting + * `-p` args. Without that scoping a glob belonging to a *neighbouring* command + * is attributed to the test runner — and since `.some()` only needs ONE glob to + * match, a broader one silently suppresses a real miss rather than adding an + * inert pattern. A `pretest:scripts` of `prettier --check "scripts/**\/*.mjs"` + * is enough to do it: that glob matches a renamed `*.spec.mjs`, so the rename + * `node --test` skips would pass the check. Reachability widened what's read; + * this keeps what's *harvested* to the runner's own arguments. + * + * Within a kept segment only the POSITIONAL args count. `--test-coverage-include + * "scripts/**\/*.mjs"` is the same suppression one level in — a glob-valued flag + * broader than the test glob, which would vouch for the very file the runner + * skips (and it's the obvious next addition here, since `scripts/` sits outside + * the ≥90 coverage gate). A leading `./` is normalized off because `git ls-files` + * emits none, so `./scripts/…` — the common npm-script idiom, which `node --test` + * accepts — would otherwise match nothing and blame every file for a rename that + * never happened (`rootRunsClientValidate` normalizes it for the same reason). + */ +const testRunnerSegments = (scripts) => + [...reachableScripts(scripts, "test:scripts")] + .map((n) => scripts?.[n]) + .filter((c) => typeof c === "string") + .flatMap((c) => c.split(/&&|\|\||;/)) + .map((segment) => tokenize(segment)) + .filter((tokens) => tokens.includes("--test")); // only `node --test` names test files + +export const testScriptGlobs = (scripts) => + testRunnerSegments(scripts) + .flatMap((tokens) => + tokens.filter( + (t, i) => + !t.startsWith("-") && + t !== "node" && + !VALUE_TAKING_TEST_FLAGS.has(tokens[i - 1]), + ), + ) + .map((g) => g.replace(/^\.\//, "")); + +/** + * Suite-narrowing flags the `test:scripts` runner is given. The other three axes + * ask whether the tests are RUN, whether there are any, and whether the runner + * is told about each — none asks what it then DOES with them. `--test-shard 1/2` + * keeps every glob intact and executes half the suite, exiting 0; `--test-only` + * and the name/skip patterns are the same shape. Three of these are already in + * `VALUE_TAKING_TEST_FLAGS` — the guard knew them well enough to drop their + * values, but not that they shrink the run. + * + * Known boundary: COMPLEMENTARY shards (`--test-shard 1/2` and `2/2` in two + * reachable scripts) do run the whole suite, and this flags them anyway. Fail- + * safe, and nobody shards a sub-100ms suite; reconstructing shard arithmetic to + * allow it would cost more than the case is worth. + */ +export const testScriptNarrowingFlags = (scripts) => + testRunnerSegments(scripts) + .flat() + // `--flag=value` and `--flag value` both report as the bare flag name. + .map((t) => t.split("=")[0]) + .filter((t) => NARROWING_TEST_FLAGS.has(t)); + +/** + * Whether a tracked `scripts/` file is one of this guard's own test files, by + * CONTENT rather than name: a JS file that registers tests with `node:test` is + * a test whatever it's called. + * + * Deciding it by name would key the required set off the same pattern the + * `test:scripts` glob uses, so a rename escaping BOTH is invisible to every axis + * — a dot→hyphen slip (`npm-scripts.test.mjs` → `npm-scripts-test.mjs`) drops + * the file from `testFiles` entirely, so nothing asks whether the runner is told + * about it, and the surviving file satisfies the non-empty axis. Round 30 fixed + * "renamed to a form the GLOB misses" by broadening the name pattern; a fifth + * name pattern only moves that boundary again. `node --test` decides what a test + * is by whether it registers tests, so read that instead of guessing. + */ +// The extension half, alone: `main()` needs it BEFORE reading a file, and the +// predicate needs it as part of its contract. One definition, two positions. +export const isJsFile = (file) => /\.(c|m)?js$/.test(file); + +export const isScriptsTestFile = (file, source) => + isJsFile(file) && + // Node's actual criterion is REGISTERING tests, not mentioning the module: a + // shared helper importing `node:test`'s `mock` registers none, and telling it + // to rename itself to `*.test.mjs` would only make node run an empty file. + /(?:\bfrom|\brequire\(\s*)\s*["']node:test["']/.test(source) && + /\b(?:test|it|describe|suite)\s*\(/.test(source); + +/** + * Gate-integrity problems with `test:scripts` — this guard's OWN parser tests — + * given the root `scripts` and the tracked `scripts/**` test files. Vouched for + * the same way the guard vouches for a `tsc` pass, on three axes: it must be run + * from `validate`, its file set must be non-empty, and every one of those files + * must be matched by a glob the runner is actually given — because `node --test` + * SKIPS a file its glob misses and still exits 0, so a rename to `*.spec.mjs` + * would shrink the suite with a green run. + * + * Pure, and separate from `main()`, so each branch below is reachable from the + * test suite: every previous round put the newest branch inside `main()`, where + * mutation testing found it untested three rounds running. + */ +export function testScriptProblems(scripts, testFiles) { + if (!rootReachesScript(scripts, "test:scripts")) + return [ + "the root `validate` no longer runs `test:scripts` — the guard's own parser tests run nowhere; restore it.", + ]; + const problems = []; + if (testFiles.length === 0) + problems.push( + "no `scripts/**/*.test.*` files are tracked — the guard's parser tests are gone.", + ); + const globs = testScriptGlobs(scripts); + if (globs.length === 0) { + // A bare `node --test` (auto-discovery from the cwd) names no glob, and + // `.some()` over an empty list would blame every file individually with + // advice none of them can follow. Say the actual problem once instead. + problems.push( + '`test:scripts` names no path/glob — this guard can\'t tell which files `node --test` runs. Give it an explicit glob (e.g. `node --test "scripts/**/*.test.mjs"`).', + ); + return problems; + } + for (const flag of testScriptNarrowingFlags(scripts)) + problems.push( + `\`test:scripts\` passes \`${flag}\` — \`node --test\` then runs only part of the matched suite and still exits 0, so the parsers this guard gates are partly unrun. Drop it.`, + ); + for (const f of testFiles) + if (!globs.some((g) => matchesTestGlob(f, g))) + problems.push( + `${f}: not matched by the \`test:scripts\` glob — \`node --test\` won't run it (a rename to a form it skips). Rename it back to \`*.test.mjs\`.`, + ); + return problems; +} + +/** + * The `references` paths declared in the tsconfig at repo-relative `tsconfigRel` + * (a `tsc -b` solution config), or `[]` if it has none / isn't readable. Paths + * are as written (relative to that tsconfig's own directory). + */ +export function parseTsconfigReferences(raw) { + try { + // Tolerate JSONC — block AND line comments + trailing commas (tsconfig + // allows all; block comments are in fact the style of every other tsconfig + // here). Block comments are stripped first so a `//` inside one doesn't + // survive; a `//` inside a string value (e.g. an `https://` URL) is a + // theoretical false strip this guard's tsconfigs never hit. + const cfg = JSON.parse( + raw + .replace(/\/\*[\s\S]*?\*\//g, "") + .replace(/\/\/.*$/gm, "") + .replace(/,(\s*[}\]])/g, "$1"), + ); + return Array.isArray(cfg.references) + ? cfg.references.map((r) => r?.path).filter((p) => typeof p === "string") + : []; + } catch { + return []; + } +} + +export function tsconfigReferences(tsconfigRel) { + try { + return parseTsconfigReferences( + readFileSync(path.join(repoRoot, tsconfigRel), "utf8"), + ); + } catch { + return []; // unreadable file (e.g. a directory / missing path) + } +} + +/** + * The `references` in a client's root `tsconfig.json`. Non-empty for a `tsc -b` + * client (like `clients/web`, which has no `typecheck` script) — this guard + * enrolls it through these instead of exempting the whole tree. + */ +export function clientTsconfigReferences(clientDir) { + return tsconfigReferences(path.posix.join(clientDir, "tsconfig.json")); +} + +/** + * How the client's `validate` runs `tsc -b` (the pass that typechecks a + * reference client): `"ok"` (a real `tsc -b`), `"neutered"` (a `tsc -b` carrying + * `--noCheck`/`--listFilesOnly` — lists files but checks nothing, the same hole + * the `typecheck`-script path rejects), or `"none"` (no `tsc -b` at all). + */ +export function tscBuildStatus(scripts) { + let status = "none"; + for (const name of reachableScripts(scripts, "validate")) { + const cmd = scripts?.[name]; + if (typeof cmd !== "string") continue; + for (const segment of cmd.split(/&&|\|\||;/)) { + const tokens = tokenize(segment); + if (!tokens.some(isTsc)) continue; + if (!tokens.some((t) => t === "-b" || t === "--build")) continue; + if (tokens.some(isDisablingFlag)) status = "neutered"; + else return "ok"; // a real, checking `tsc -b` + } + } + return status; +} + +/** + * The Node clients this guard covers (#1791): every `clients/` dir with a + * readable `package.json` is required to be **enrolled** — either it declares a + * `typecheck` script (measured through that script's projects) or its root + * `tsconfig.json` has `references` (a `tsc -b` solution like `clients/web`, + * measured through those reference projects) — **or** be in {@link EXEMPT}. + * Anything else is a gate-integrity failure. Enumerated from **disk** so it's + * fail-closed on every axis: a new client is auto-required (a hardcoded list + * wouldn't pick it up), a client can't be dropped by a root-chain edit (the + * chain isn't the source), renaming the `typecheck` script doesn't silently drop + * the client (it hard-fails — no longer enrolled, not exempt), and a `clients/*` + * dir holding tracked TS but no manifest hard-fails too. Returns + * `{ clients, problems }`. + */ +function nodeClients() { + const clientsDir = path.join(repoRoot, "clients"); + const clients = []; + const problems = []; + let entries; + try { + entries = readdirSync(clientsDir, { withFileTypes: true }); + } catch { + return { clients, problems }; // `clients/` missing — the caller's bail fires. + } + for (const entry of entries.sort((a, b) => a.name.localeCompare(b.name))) { + if (!entry.isDirectory()) continue; + const dir = `clients/${entry.name}`; + if (EXEMPT.has(dir)) continue; + let scripts; + try { + scripts = JSON.parse( + readFileSync(path.join(clientsDir, entry.name, "package.json"), "utf8"), + ).scripts; + } catch { + // No readable manifest. If the dir still holds tracked TS its source gets + // no tsc pass — a gate hole; otherwise it just isn't a client. + if (trackedSourceFiles(dir).length > 0) + problems.push( + `${dir}: holds tracked TypeScript but has no readable \`package.json\` — its source gets no tsc pass. Add a client manifest with a \`typecheck\` (or exempt it).`, + ); + continue; + } + if ( + typeof scripts?.typecheck === "string" || + clientTsconfigReferences(dir).length > 0 + ) + clients.push(dir); + else + problems.push( + `${dir}: declares no \`typecheck\` script, has no \`tsconfig.json\` \`references\`, and isn't in the EXEMPT set — it gets no tsc pass. Add a \`typecheck\` (or exempt it with a reason).`, + ); + } + // A stale EXEMPT key would otherwise be asserted in the success line while + // naming a dir that no longer exists — the exemption outliving its subject. + const present = new Set( + entries.filter((e) => e.isDirectory()).map((e) => `clients/${e.name}`), + ); + for (const dir of EXEMPT.keys()) + if (!present.has(dir)) + problems.push( + `${dir}: listed in EXEMPT but is not a \`clients/*\` directory — remove the stale exemption.`, + ); + return { clients, problems }; +} + +/** + * The tsconfig projects a client's `typecheck` names, harvested from **every** + * script reachable from `typecheck` (not just the one string) so a delegating + * `typecheck` (`npm run typecheck:src && …`) still counts — matching how + * `verify-format-coverage.mjs` harvests globs across reachable scripts. Splits + * each script on `&&`/`||`/`;` so a flag on one command doesn't leak onto + * another. Each `tsc` command's project comes from `-p`/`--project` (or a + * `-b`/`--build` path); a `tsc` command with **no** project flag resolves the + * implicit `./tsconfig.json` (tsc's own default), so that idiomatic form counts + * too. Returns `{ projects, neutered }`: `neutered` names any project whose own + * command carries `--noCheck`/`--nocheck` or `--listFilesOnly` (matched + * case-insensitively — tsc's option parsing is) — a pass that lists files + * without type-checking them, which would otherwise satisfy the guard while + * checking nothing. The config-file form (`noCheck` set in the tsconfig) is + * caught separately by {@link projectDisablesChecking}. + * + * A harvested `tsc -b` **solution config** (`"files": []` + `references`) lists + * nothing itself; {@link projectFiles} expands it to its references, so this + * form is measured whether it reaches here (a `typecheck: "tsc -b"`) or the + * dedicated reference path (`clients/web`, which declares no `typecheck`). + * + * Minor limitations, all unreachable with the plain `-p --noEmit` passes here: + * the implicit-`./tsconfig.json` fallback assumes **no file operands** (`tsc + * ` ignores the config and checks only that file, but would be credited + * the whole config's file list); the `--noCheck`/`--listFilesOnly` detection + * ignores a following boolean, so the contrived explicit `--noCheck false` + * (checking *on*) is still treated as disabling; and the `&&`/`||`/`;` split + * runs before tokenizing, so a quoted operator inside an arg would split + * mid-token (project paths carry none of those). + */ +export function typecheckProjects(scripts) { + const projects = []; + const neutered = []; + const isFlag = (t) => t.startsWith("-"); + const isProjectFlag = (t) => ["-p", "--project", "-b", "--build"].includes(t); + for (const name of reachableScripts(scripts, "typecheck")) { + const cmd = scripts?.[name]; + if (typeof cmd !== "string") continue; + for (const segment of cmd.split(/&&|\|\||;/)) { + const tokens = tokenize(segment); + if (!tokens.some(isTsc)) continue; // only tsc commands name projects + const disabling = tokens.find(isDisablingFlag); + // A project path follows `-p`/`--project`/`-b`/`--build`; a tsc command + // with none uses the implicit `./tsconfig.json` (tsc's own default). + const named = []; + for (let i = 0; i < tokens.length; i++) + if (isProjectFlag(tokens[i]) && tokens[i + 1] && !isFlag(tokens[i + 1])) + named.push(tokens[i + 1]); + if (named.length === 0) named.push("tsconfig.json"); + for (const project of named) { + if (disabling) neutered.push({ project, flag: disabling }); + else projects.push(project); + } + } + } + return { projects, neutered }; +} + +/** + * Whether the tsconfig `project` sets `noCheck` (which disables type-checking as + * thoroughly as the CLI flag, but can't be seen in the `typecheck` script + * string). `tsc --showConfig` emits the merged compilerOptions, surfacing a + * `noCheck` from the config or its `extends` chain. Best-effort: on any error + * (e.g. an unreadable config, already reported elsewhere) it returns false. + */ +function projectDisablesChecking(clientDir, project) { + try { + const out = execFileSync( + "npx", + ["--no-install", "tsc", "-p", project, "--showConfig"], + { cwd: path.join(repoRoot, clientDir), encoding: "utf8" }, + ); + return JSON.parse(out)?.compilerOptions?.noCheck === true; + } catch { + return false; + } +} + +/** + * Repo-relative POSIX paths of the files ONE project (no reference expansion) + * typechecks. Absolute paths outside the repo root (lib.d.ts) and anything under + * `node_modules` are dropped; the aliased `core/` + `test-servers/` sources stay + * in the set but are harmless — the set is only ever queried with client-relative + * paths. Cached: `resolveLeafProjects` and `projectFiles` both list a project. + */ +const rawFilesCache = new Map(); +function rawProjectFiles(clientDir, project) { + const key = `${clientDir}|${project}`; + const cached = rawFilesCache.get(key); + if (cached) return cached; + const absClient = path.join(repoRoot, clientDir); + let stdout; + try { + stdout = execFileSync( + "npx", + ["--no-install", "tsc", "-p", project, "--listFilesOnly"], + { cwd: absClient, encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }, + ); + } catch (err) { + // `--listFilesOnly` doesn't type-check, but a config error (an unreadable or + // malformed tsconfig) still exits non-zero while printing the resolved file + // list; keep stdout so a broken config doesn't mask a coverage gap. Echo the + // diagnostic — since this guard runs before any client's own `typecheck`, + // it's the first place a bad `-p` config surfaces, and without the reason + // the resulting "file in no project" report is misleading. tsc prints config + // errors (`error TS…`) to stdout, so scan both streams for them. + stdout = typeof err.stdout === "string" ? err.stdout : ""; + const streams = + stdout + "\n" + (typeof err.stderr === "string" ? err.stderr : ""); + const diagnostic = streams + .split("\n") + .filter((l) => /error TS\d+/.test(l)) + .join("\n") + .trim(); + console.warn( + `verify:typecheck-coverage — \`tsc -p ${project}\` (in ${clientDir}) exited non-zero:\n${diagnostic || "(no diagnostic captured)"}\n`, + ); + } + const covered = new Set(); + for (const line of stdout.split("\n")) { + const abs = line.trim(); + if (!abs) continue; + const rel = path.relative(repoRoot, abs); + if (rel.startsWith("..") || rel.includes("node_modules")) continue; + covered.add(rel.split(path.sep).join("/")); + } + rawFilesCache.set(key, covered); + return covered; +} + +/** + * The repo-relative tsconfig FILE a `clientDir`-relative `project` entry names. + * A directory-form entry (`{ "path": "./packages/a" }`, or `tsc -p src`) means + * `/tsconfig.json` — tsc's own rule. + */ +export function projectConfigFile(clientDir, project) { + const projectRel = path.posix.join(clientDir, project); + return projectRel.endsWith(".json") + ? projectRel + : path.posix.join(projectRel, "tsconfig.json"); +} + +/** + * A `references` entry `ref` (written relative to `fromConfigFile`'s own + * directory) as a `clientDir`-relative project path, the form the rest of the + * graph walk uses. + */ +export function refToProject(clientDir, fromConfigFile, ref) { + return path.posix.relative( + clientDir, + path.posix.join(path.posix.dirname(fromConfigFile), ref), + ); +} + +/** + * The leaf tsconfig projects `project` resolves to (paths relative to + * `clientDir`): itself if it lists files (or has no `references`), else its + * `references` expanded recursively. A `tsc -b` **solution config** (`{"files": + * [], "references": […]}`) lists nothing under `--listFilesOnly`, so this is how + * it's reduced to the real projects — and doing it here (not just inside + * `projectFiles`) is what lets BOTH coverage and the non-inertness check follow + * the same graph, so a `noCheck` in a *referenced* project is caught no matter + * which enrollment path harvested the solution. + */ +export function resolveLeafProjects(clientDir, project, seen = new Set()) { + if (seen.has(project)) return []; + seen.add(project); + // Lists files → a real leaf. (An empty set is a solution config, or a config + // that errored — either way the reference expansion below is the right next + // step: a broken config yields no references too.) + if (rawProjectFiles(clientDir, project).size > 0) return [project]; + const configFile = projectConfigFile(clientDir, project); + const refs = tsconfigReferences(configFile); + if (refs.length === 0) return [project]; // no files, no refs — itself + return refs.flatMap((ref) => + resolveLeafProjects( + clientDir, + refToProject(clientDir, configFile, ref), + seen, + ), + ); +} + +/** Repo-relative files a project covers, following a solution config's references. */ +function projectFiles(clientDir, project) { + const covered = new Set(); + for (const leaf of resolveLeafProjects(clientDir, project)) + for (const f of rawProjectFiles(clientDir, leaf)) covered.add(f); + return covered; +} + +/** + * The leaf projects (across `projects`, solutions expanded) that genuinely + * type-check — i.e. minus any whose tsconfig sets `noCheck`, each of which is + * pushed to `integrity`. Shared by both enrollment paths so a `noCheck` in a + * referenced project is caught whether the solution was harvested from a + * `typecheck` script or is the reference-path client's own `tsconfig.json`. + */ +function checkingLeaves(clientDir, projects, integrity) { + const checking = []; + // Dedup across all harvested projects (a leaf can be reached from two of them + // — `resolveLeafProjects` only dedups within one) so a shared leaf isn't + // reported twice, nor `--showConfig`-spawned twice. + const seen = new Set(); + for (const project of projects) + for (const leaf of resolveLeafProjects(clientDir, project)) { + if (seen.has(leaf)) continue; + seen.add(leaf); + if (projectDisablesChecking(clientDir, leaf)) + integrity.push( + `${clientDir}: \`${leaf}\` sets \`noCheck\` in its tsconfig — that project lists files without type-checking them, so it gates nothing.`, + ); + else checking.push(leaf); + } + return checking; +} + +/** Tracked first-party TS (`.ts`/`.tsx`/`.mts`/`.cts`) under a client (excludes build output). */ +function trackedSourceFiles(clientDir) { + const out = execFileSync( + "git", + ["ls-files", "*.ts", "*.tsx", "*.mts", "*.cts"], + { cwd: path.join(repoRoot, clientDir), encoding: "utf8" }, + ); + return out + .split("\n") + .filter(Boolean) + .map((f) => path.posix.join(clientDir, f)) + .filter(isRequiredSource) + .filter((f) => !f.includes("/build/") && !f.includes("/dist/")); +} + +/** + * Tracked first-party TS that is not under a `clients/*` dir (which the + * per-client pass covers) — the shared surface no client owns (`test-servers/src`, + * the root `vitest.shared.mts`), all of `core/` (which web's enrolled `tsc -b` + * projects cover — measured, not hand-modeled), plus anything new at a fresh + * top-level location. Deny-by-default: a new such file is required without + * editing this guard, matching the repo-wide reach of the sibling + * `verify-format-coverage`. Each must get a tsc pass from SOME client project + * (cli aliases the test-server source; web's app/test projects include `core/`; + * each client's `vitest.config.ts` imports `vitest.shared.mts`) — checked against + * the GLOBAL union of client projects, which is why a new unimported + * `test-servers/src` bin entry or a `core` `*.tsx` web doesn't reach is caught. + */ +function trackedNonClientSource() { + const out = execFileSync( + "git", + ["ls-files", "*.ts", "*.tsx", "*.mts", "*.cts"], + { cwd: repoRoot, encoding: "utf8" }, + ); + return out + .split("\n") + .filter(Boolean) + .filter(isRequiredSource) + .filter((f) => !f.includes("/build/") && !f.includes("/dist/")) + .filter((f) => !f.startsWith("clients/")); +} + +/** + * The remediation footer for a set of gate-integrity problems, or `null` when + * none of them is about the typecheck wiring. Not every phase-1 problem is: the + * sibling-guard vouch (`verify:format-coverage`) and the `test:scripts` axes are + * about OTHER gates, and appending `--noCheck`/`tsc -b` advice under one of them + * sends the reader after the wrong thing. The criterion is "is this about the + * typecheck pass at all", not "does it carry its own per-line advice" — several + * typecheck problems do carry their own and still want the footer. Client + * enrollment ("declares no `typecheck` script …") is therefore NOT excluded: the + * footer's first clause is exactly its fix. + * + * Pure and outside `main()` on purpose — this is the third consecutive round in + * which the newest branch in `main()` turned out to be the one mutation testing + * couldn't reach, including the branch this function replaces. + */ +export function integrityAdvice(problems, nonTypecheckProblems) { + const isTypecheck = (f) => !nonTypecheckProblems.includes(f); + return problems.some(isTypecheck) + ? "\nRestore the `typecheck` wiring (client `validate` → `typecheck`, root `validate` → each client), and drop any `--noCheck`/`--listFilesOnly`/`noCheck` from the typecheck pass." + : null; +} + +/** + * Run the guard: enumerate clients, check gate integrity (phase 1), then file + * coverage (phase 2). Prints its verdict and `process.exit(1)`s on any failure. + * Called only when this file is executed directly — importing it (for tests) + * gives access to the pure helpers above without running any of this. + */ +export function main() { + const rootPkg = JSON.parse( + readFileSync(path.join(repoRoot, "package.json"), "utf8"), + ); + const { clients: CLIENTS, problems: enrollmentProblems } = nodeClients(); + + // ------------------------------------------------------------------------- + // Phase 1 — gate integrity: is each client's `typecheck` actually run, and + // does it actually type-check? Reported (and exited) before the file-coverage + // pass so a mis-wired / inert gate isn't buried under a flood of consequent + // "in no tsconfig project" lines. Records, per client, the projects that + // genuinely type-check, for phase 2. Boundary: does NOT detect shell-level + // failure suppression (`… || true`, `; exit 0`). + // ------------------------------------------------------------------------- + // A client that declares no `typecheck` and isn't exempt is a gate-integrity + // failure (seeded so a renamed/removed `typecheck` is loud, not a silent + // drop). If that leaves nothing enrolled AND surfaced no such problem, the + // enumeration itself is broken (a moved `clients/` dir) — fail, don't no-op. + const integrity = [...enrollmentProblems]; + const checkingProjects = new Map(); + if (CLIENTS.length === 0 && integrity.length === 0) { + console.error( + "verify:typecheck-coverage — found no `clients/*` dir to check. The guard would check nothing; fix the enumeration.", + ); + process.exit(1); + } + + // Vouch for the sibling guard — a guard can't detect being unrun itself, but the + // two can each assert the other is still wired into `validate`, so dropping + // either is caught here (only deleting both slips through). + const siblingVouchIssues = []; + if (!rootReachesScript(rootPkg.scripts, "verify:format-coverage")) { + siblingVouchIssues.push( + "the root `validate` no longer runs `verify:format-coverage` (its sibling guard) — restore it.", + ); + } + integrity.push(...siblingVouchIssues); + + const testScriptIssues = testScriptProblems( + rootPkg.scripts, + execFileSync("git", ["ls-files", "scripts"], { + cwd: repoRoot, + encoding: "utf8", + }) + .split("\n") + .filter(Boolean) + // Extension first, and the read guarded: a file in the index but not the + // worktree (`rm` without `git rm`, an ordinary mid-work state) would + // otherwise abort phase 1 with a raw ENOENT stack, masking every real + // finding — the round-6 precedent, where a failed `tsc` echoes its + // diagnostic instead of dying. + .filter(isJsFile) + .filter((f) => { + let src; + try { + src = readFileSync(path.join(repoRoot, f), "utf8"); + } catch (err) { + console.warn( + `verify:typecheck-coverage — skipping tracked-but-unreadable ${f}: ${err.message}`, + ); + return false; + } + return isScriptsTestFile(f, src); + }), + ); + integrity.push(...testScriptIssues); + + for (const clientDir of CLIENTS) { + checkingProjects.set(clientDir, []); + if (!rootRunsClientValidate(rootPkg.scripts, clientDir)) { + integrity.push( + `${clientDir}: the root \`validate\` chain no longer runs \`cd ${clientDir} && npm run validate\` (or \`npm --prefix ${clientDir} run validate\`) — its typecheck isn't invoked by CI.`, + ); + continue; + } + const scripts = JSON.parse( + readFileSync(path.join(repoRoot, clientDir, "package.json"), "utf8"), + ).scripts; + + // Reference (`tsc -b`) client — no `typecheck` script; measured through its + // `tsconfig.json` `references`, and wired iff its `validate` runs a real + // (checking) `tsc -b`. + if (typeof scripts?.typecheck !== "string") { + const status = tscBuildStatus(scripts); + if (status !== "ok") { + integrity.push( + status === "neutered" + ? `${clientDir}: its \`validate\`'s \`tsc -b\` carries \`--noCheck\`/\`--listFilesOnly\` — it lists files without type-checking them, so its references are gated by nothing.` + : `${clientDir}: no \`typecheck\` script and its \`validate\` never runs \`tsc -b\` — its \`tsconfig.json\` references are typechecked by nothing.`, + ); + continue; + } + checkingProjects.set( + clientDir, + checkingLeaves( + clientDir, + clientTsconfigReferences(clientDir), + integrity, + ), + ); + continue; + } + + if (!reachableScripts(scripts, "validate").has("typecheck")) { + integrity.push( + `${clientDir}: \`typecheck\` is not reachable from its \`validate\` — the typecheck it measures gates nothing.`, + ); + continue; + } + const { projects, neutered } = typecheckProjects(scripts); + for (const { project, flag } of neutered) + integrity.push( + `${clientDir}: its \`typecheck\` runs \`-p ${project}\` with \`${flag}\` — that pass lists files without type-checking them, so it gates nothing.`, + ); + // Fire only when nothing was harvested at all — not when projects WERE named + // but every one is neutered (command flag) or config-disabled (`projects` was + // non-empty in that case; those get their own lines above). + if (projects.length === 0 && neutered.length === 0) + integrity.push( + `${clientDir}: its \`typecheck\` names no \`-p \` — nothing is typechecked.`, + ); + // Resolve each harvested project to its leaves (a `tsc -b` solution expands), + // and disable-check each leaf — so a `noCheck` in a referenced project counts. + checkingProjects.set( + clientDir, + checkingLeaves(clientDir, projects, integrity), + ); + } + + if (integrity.length > 0) { + console.error( + `verify:typecheck-coverage — ${integrity.length} gate-integrity issue(s): a typecheck gate is not run, or runs but checks nothing:\n`, + ); + for (const f of integrity) console.error(" " + f); + // NOT `enrollmentProblems`: "declares no `typecheck` script …" IS about the + // typecheck pass, and the footer's first clause is its canonical fix. + const advice = integrityAdvice(integrity, [ + ...siblingVouchIssues, + ...testScriptIssues, + ]); + if (advice) console.error(advice); + process.exit(1); + } + + // --------------------------------------------------------------------------- + // Phase 2 — file coverage: every tracked source file lands in a checking project. + // --------------------------------------------------------------------------- + let totalChecked = 0; + const failures = []; + const globalCovered = new Set(); + for (const clientDir of CLIENTS) { + const covered = new Set(); + for (const project of checkingProjects.get(clientDir)) + for (const f of projectFiles(clientDir, project)) { + covered.add(f); + globalCovered.add(f); + } + + const tracked = trackedSourceFiles(clientDir); + totalChecked += tracked.length; + for (const f of tracked) + if (!covered.has(f)) failures.push(`${f} — in no tsconfig project`); + } + + // Non-client first-party TS (shared source, plus anything at a new top-level + // location) gets no client of its own; require each to land in the GLOBAL union + // of client projects, so it's deny-by-default rather than an allowlist that a + // new location could fall outside. + const otherTracked = trackedNonClientSource(); + totalChecked += otherTracked.length; + const nonClientMisses = []; + for (const f of otherTracked) + if (!globalCovered.has(f)) { + nonClientMisses.push(f); + failures.push(`${f} — in no client's tsconfig project`); + } + + if (failures.length > 0) { + console.error( + `verify:typecheck-coverage — ${failures.length} tracked source file(s) get no \`tsc\` pass:\n`, + ); + for (const f of failures) console.error(" " + f); + console.error( + "\nAdd the file to a client's `tsconfig.json` / `tsconfig.test.json` `include` (a top-level config the build config's `rootDir` rejects goes in the test project).", + ); + if (failures.some((f) => /\.test\./.test(f))) + console.error( + "For a co-located test, instead move it to `__tests__/` — adding it to the src `include` would make the build emit it.", + ); + if (nonClientMisses.some((f) => f.startsWith("test-servers/"))) + console.error( + "For a shared-source file no client imports (e.g. a `test-servers/src` bin entry), name it in a client `tsconfig.test.json`'s `include`, as `server-composable.ts` is.", + ); + if (nonClientMisses.some((f) => f.startsWith("core/"))) + console.error( + "For a `core/` file web's `tsc -b` doesn't reach, widen the web project that owns it — `tsconfig.test.json` for `core/**/__tests__/**`, else `tsconfig.app.json` — not a cli/tui project, which shouldn't own `core`.", + ); + if (nonClientMisses.some((f) => f.startsWith("scripts/"))) + console.error( + "For root tooling under `scripts/`, keep it `.mjs` (the convention there — prettier-gated via `format:check:scripts`, and `.mjs` isn't in this guard's required set), or give `scripts/` its own tsconfig project and enroll it here.", + ); + console.error("See AGENTS.md."); + process.exit(1); + } + + const exemptNote = [...EXEMPT.entries()] + .map(([dir, reason]) => `${dir} exempt: ${reason}`) + .join("; "); + console.log( + `verify:typecheck-coverage — OK: all ${totalChecked} tracked source files ` + + `(${CLIENTS.length} clients + non-client) get a tsc pass` + + (exemptNote ? ` (${exemptNote}).` : "."), + ); +} + +// Run only when executed directly (`node scripts/verify-typecheck-coverage.mjs`); +// importing this file (tests) exposes the pure helpers without running the guard. +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) + main(); diff --git a/scripts/verify-typecheck-coverage.test.mjs b/scripts/verify-typecheck-coverage.test.mjs new file mode 100644 index 000000000..22151c254 --- /dev/null +++ b/scripts/verify-typecheck-coverage.test.mjs @@ -0,0 +1,427 @@ +// Table-driven tests for verify-typecheck-coverage's pure parsers. Importing the +// module exposes these without running the guard (its execution is behind +// `main()`, called only when the file is run directly). Each case pins a rule a +// #1799 review round found. Run via `npm run test:scripts`. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + matchesTestGlob, + isDisablingFlag, + isRequiredSource, + isTsc, + parseTsconfigReferences, + projectConfigFile, + refToProject, + integrityAdvice, + isScriptsTestFile, + testScriptGlobs, + testScriptNarrowingFlags, + testScriptProblems, + tscBuildStatus, + typecheckProjects, +} from "./verify-typecheck-coverage.mjs"; + +test("isRequiredSource: TS extensions, ambient .d.ts excluded (r7)", () => { + for (const f of ["a.ts", "a.tsx", "a.mts", "a.cts"]) + assert.ok(isRequiredSource(f), f); + for (const f of ["a.js", "a.d.ts", "a.d.mts", "a.d.cts", "a.json"]) + assert.ok(!isRequiredSource(f), f); +}); + +test("isTsc: matches by basename incl. path-invoked (r18 regression)", () => { + for (const t of [ + "tsc", + "node_modules/.bin/tsc", + "./node_modules/.bin/tsc.cmd", + ]) + assert.ok(isTsc(t), t); + for (const t of ["vitest", "prettier", "tscx", "atsc"]) + assert.ok(!isTsc(t), t); +}); + +test("isDisablingFlag: case-insensitive (r18)", () => { + for (const t of [ + "--noCheck", + "--nocheck", + "--listFilesOnly", + "--LISTFILESONLY", + ]) + assert.ok(isDisablingFlag(t), t); + for (const t of ["--noEmit", "-p", "--project", "noCheck"]) + assert.ok(!isDisablingFlag(t), t); +}); + +test("typecheckProjects: harvests -p / --project / -b, implicit tsconfig.json (r13)", () => { + const { projects, neutered } = typecheckProjects({ + typecheck: + "tsc --noEmit -p tsconfig.json && tsc --noEmit -p tsconfig.test.json", + }); + assert.deepEqual(projects, ["tsconfig.json", "tsconfig.test.json"]); + assert.equal(neutered.length, 0); + + // A bare `tsc` (no project flag) resolves the implicit ./tsconfig.json. + assert.deepEqual(typecheckProjects({ typecheck: "tsc --noEmit" }).projects, [ + "tsconfig.json", + ]); + + // Path-invoked binary still counts (r18). + assert.deepEqual( + typecheckProjects({ typecheck: "node_modules/.bin/tsc -p tsconfig.json" }) + .projects, + ["tsconfig.json"], + ); + + // A quoted project path (r17). + assert.deepEqual( + typecheckProjects({ typecheck: `tsc -p "tsconfig.test.json"` }).projects, + ["tsconfig.test.json"], + ); + + // `--project` long form, and `-b`/`--build` project paths (r13). + const proj = (cmd) => typecheckProjects({ typecheck: cmd }).projects; + assert.deepEqual(proj("tsc --noEmit --project tsconfig.json"), [ + "tsconfig.json", + ]); + assert.deepEqual(proj("tsc -b tsconfig.json"), ["tsconfig.json"]); + assert.deepEqual(proj("tsc --build tsconfig.json"), ["tsconfig.json"]); + assert.deepEqual(proj("tsc -b"), ["tsconfig.json"]); // implicit fallback +}); + +test("typecheckProjects: neutered by --noCheck / --listFilesOnly (r10)", () => { + const { projects, neutered } = typecheckProjects({ + typecheck: + "tsc --noEmit -p tsconfig.json --noCheck && tsc --noEmit -p tsconfig.test.json", + }); + assert.deepEqual(projects, ["tsconfig.test.json"]); + assert.deepEqual(neutered, [{ project: "tsconfig.json", flag: "--noCheck" }]); +}); + +test("typecheckProjects: delegating typecheck, ignores non-tsc segments (r15)", () => { + const { projects } = typecheckProjects({ + typecheck: "npm run typecheck:src && npm run typecheck:test", + "typecheck:src": "tsc --noEmit -p tsconfig.json", + "typecheck:test": "tsc --noEmit --project tsconfig.test.json", + }); + assert.deepEqual(projects.sort(), ["tsconfig.json", "tsconfig.test.json"]); +}); + +test("tscBuildStatus: ok / neutered / none (r25)", () => { + const status = (build) => + tscBuildStatus({ validate: "npm run build", build }); + assert.equal(status("tsc -b && vite build"), "ok"); + assert.equal(status("tsc --build"), "ok"); + assert.equal(status("tsc -b --noCheck && vite build"), "neutered"); + assert.equal(status("vite build"), "none"); + assert.equal(status("tsc --noEmit -p tsconfig.json"), "none"); // -b required +}); + +test("parseTsconfigReferences: JSONC tolerance (r17-nit2 block comments)", () => { + const refs = (raw) => parseTsconfigReferences(raw); + assert.deepEqual(refs('{ "references": [{ "path": "./a" }] }'), ["./a"]); + assert.deepEqual( + refs('/* solution */\n{ "references": [{ "path": "./a" }] }'), + ["./a"], + ); + assert.deepEqual(refs('{ "references": [{ "path": "./a" }] } // trailing'), [ + "./a", + ]); + assert.deepEqual(refs('{ "references": [{ "path": "./a" },] }'), ["./a"]); // trailing comma + assert.deepEqual(refs('{ "files": [] }'), []); // no references + assert.deepEqual(refs("{ not json"), []); // malformed + assert.deepEqual(refs('{ "references": [{ "prepend": true }] }'), []); // no path +}); + +test("matchesTestGlob: the guard's contract, not node's glob engine", () => { + // Only the two properties the guard actually relies on — the rest of node's + // glob semantics are node's to test, which is the point of delegating to it. + const g = "scripts/**/*.test.mjs"; + assert.ok(matchesTestGlob("scripts/verify-typecheck-coverage.test.mjs", g)); // zero-depth ** + assert.ok(matchesTestGlob("scripts/lib/npm-scripts.test.mjs", g)); // nested + assert.ok(!matchesTestGlob("scripts/lib/npm-scripts.spec.mjs", g)); // the probe-B rename +}); + +test("testScriptGlobs: harvests across delegation (r31 finding 1 / r32 finding 2)", () => { + // Direct form — the glob itself, with `node` and flags dropped. + assert.deepEqual( + testScriptGlobs({ "test:scripts": 'node --test "scripts/**/*.test.mjs"' }), + ["scripts/**/*.test.mjs"], + ); + // Delegating form — BOTH child globs, not the literal npm/run/ tokens. + const delegating = testScriptGlobs({ + "test:scripts": "npm run test:scripts:lib && npm run test:scripts:guard", + "test:scripts:lib": 'node --test "scripts/lib/**/*.test.mjs"', + "test:scripts:guard": 'node --test "scripts/*.test.mjs"', + }); + assert.ok(delegating.includes("scripts/lib/**/*.test.mjs")); + assert.ok(delegating.includes("scripts/*.test.mjs")); + // A pre hook is reached too (npm runs it implicitly). + assert.ok( + testScriptGlobs({ + "test:scripts": "node --test scripts/a.test.mjs", + "pretest:scripts": "node --test scripts/b.test.mjs", + }).includes("scripts/b.test.mjs"), + ); + // An unreachable script contributes nothing. + assert.deepEqual( + testScriptGlobs({ + "test:scripts": "node --test scripts/a.test.mjs", + other: "node --test scripts/z.test.mjs", + }), + ["scripts/a.test.mjs"], + ); +}); + +test("testScriptGlobs: only `node --test` segments contribute (r33 finding 1)", () => { + // A reachable NON-runner command's glob must not be attributed to the runner: + // `scripts/**/*.mjs` matches a renamed `*.spec.mjs`, so harvesting it would + // make the probe-B rename pass while `node --test` silently ran 6 fewer tests. + assert.deepEqual( + testScriptGlobs({ + "test:scripts": 'node --test "scripts/**/*.test.mjs"', + "pretest:scripts": 'prettier --check "scripts/**/*.mjs"', + }), + ["scripts/**/*.test.mjs"], + ); + // Same within one command: only the `--test` segment's args are harvested. + assert.deepEqual( + testScriptGlobs({ + "test:scripts": + 'prettier --check "scripts/**/*.mjs" && node --test "scripts/**/*.test.mjs"', + }), + ["scripts/**/*.test.mjs"], + ); +}); + +test("testScriptGlobs: flag values and ./ prefixes (r34 findings 1 & 2)", () => { + // A glob-valued `--test-*` flag is NOT a positional arg. `scripts/**/*.mjs` + // matches a renamed `*.spec.mjs`, so harvesting it would vouch for the very + // file the runner skips — the r33 suppression one level in. + assert.deepEqual( + testScriptGlobs({ + "test:scripts": + 'node --test --experimental-test-coverage --test-coverage-include "scripts/**/*.mjs" "scripts/**/*.test.mjs"', + }), + ["scripts/**/*.test.mjs"], + ); + // A leading `./` is normalized off — `node --test` accepts it, `git ls-files` + // never emits it, so keeping it would blame every file for a rename that + // never happened. + assert.deepEqual( + testScriptGlobs({ + "test:scripts": 'node --test "./scripts/**/*.test.mjs"', + }), + ["scripts/**/*.test.mjs"], + ); + // …and the normalized glob really does match what `git ls-files` emits. + assert.ok( + testScriptProblems( + { + validate: "npm run test:scripts", + "test:scripts": 'node --test "./scripts/**/*.test.mjs"', + }, + ["scripts/lib/npm-scripts.test.mjs"], + ).length === 0, + ); +}); + +test("testScriptGlobs: empty when no glob is named (r33 finding 2)", () => { + // The condition the "`test:scripts` names no path/glob" branch keys off — a + // bare `node --test` auto-discovers, so the guard can't tell what it runs. + assert.deepEqual(testScriptGlobs({ "test:scripts": "node --test" }), []); + assert.deepEqual(testScriptGlobs({}), []); +}); + +test("testScriptProblems: all three axes (r33 finding 2)", () => { + const WIRED = { + validate: "npm run test:scripts", + "test:scripts": 'node --test "scripts/**/*.test.mjs"', + }; + const only = (p) => (assert.equal(p.length, 1, p.join("\n")), p[0]); + + // Green: wired, non-empty, every file glob-matched. + assert.deepEqual(testScriptProblems(WIRED, ["scripts/a.test.mjs"]), []); + + // Axis 1 — not reachable from `validate`. Reported alone: with the tests + // unrun, the other two axes are moot. + assert.match( + only( + testScriptProblems( + { validate: "echo hi", "test:scripts": WIRED["test:scripts"] }, + ["scripts/a.test.mjs"], + ), + ), + /no longer runs `test:scripts`/, + ); + + // Axis 2 — no tracked test files at all. + assert.match( + only(testScriptProblems(WIRED, [])), + /no `scripts\/\*\*\/\*\.test\.\*` files are tracked/, + ); + + // Axis 3 — a file `node --test` would skip (the probe-B rename). + assert.match( + only(testScriptProblems(WIRED, ["scripts/a.spec.mjs"])), + /^scripts\/a\.spec\.mjs: not matched by the `test:scripts` glob/, + ); + + // The empty-harvest branch: ONE message naming the real problem, not one + // unfollowable blame per file. (`if (false)`-mutating it yields 2 messages.) + assert.match( + only( + testScriptProblems( + { validate: "npm run test:scripts", "test:scripts": "node --test" }, + ["scripts/a.test.mjs", "scripts/b.test.mjs"], + ), + ), + /names no path\/glob/, + ); +}); + +test("isScriptsTestFile: discovery by content, not name (r36 finding 1)", () => { + const TEST_SRC = 'import { test } from "node:test";\ntest("x", () => {});\n'; + // The name is irrelevant — a dot→hyphen rename escapes every name pattern, + // and that is exactly the rename that silently dropped 6 of 24 tests. + assert.ok(isScriptsTestFile("scripts/lib/npm-scripts.test.mjs", TEST_SRC)); + assert.ok(isScriptsTestFile("scripts/lib/npm-scripts-test.mjs", TEST_SRC)); + assert.ok(isScriptsTestFile("scripts/tokenize-tests.mjs", TEST_SRC)); + assert.ok(isScriptsTestFile("scripts/a.test.js", TEST_SRC)); + assert.ok( + isScriptsTestFile( + "scripts/a.test.cjs", + "const { test } = require('node:test');\ntest('x', () => {});\n", + ), + ); + // A shared helper importing `node:test`'s `mock` registers no tests — telling + // it to rename itself to `*.test.mjs` would make node run an empty file. + assert.ok( + !isScriptsTestFile( + "scripts/lib/test-mocks.mjs", + 'import { mock } from "node:test";\nexport const resetAll = () => mock.reset();\n', + ), + ); + // A guard script that merely mentions testing is not a test. + assert.ok( + !isScriptsTestFile( + "scripts/verify-typecheck-coverage.mjs", + "// runs the test:scripts glob\nimport path from 'node:path';\n", + ), + ); + // Non-JS never counts, whatever it contains. + assert.ok(!isScriptsTestFile("scripts/notes.md", TEST_SRC)); + assert.ok(!isScriptsTestFile("scripts/a.test.ts", TEST_SRC)); +}); + +test("testScriptNarrowingFlags: flags that shrink the run (r35 finding 1)", () => { + const flags = (cmd) => testScriptNarrowingFlags({ "test:scripts": cmd }); + const GLOB = '"scripts/**/*.test.mjs"'; + // `--test-shard 1/2` keeps every glob intact and runs half the suite, exit 0. + assert.deepEqual(flags(`node --test --test-shard 1/2 ${GLOB}`), [ + "--test-shard", + ]); + assert.deepEqual(flags(`node --test --test-only ${GLOB}`), ["--test-only"]); + assert.deepEqual(flags(`node --test --test-name-pattern zzz ${GLOB}`), [ + "--test-name-pattern", + ]); + assert.deepEqual(flags(`node --test --test-skip-pattern=zzz ${GLOB}`), [ + "--test-skip-pattern", + ]); // `=` form + // Not narrowing: the plain form, and a coverage flag that only reports. + assert.deepEqual(flags(`node --test ${GLOB}`), []); + assert.deepEqual( + flags( + `node --test --experimental-test-coverage --test-coverage-include "scripts/**/*.mjs" ${GLOB}`, + ), + [], + ); + // A narrowing flag on a NON-runner segment isn't the runner's (r33 gate). + assert.deepEqual( + testScriptNarrowingFlags({ + "test:scripts": `node --test ${GLOB}`, + "pretest:scripts": "some-tool --test-only", + }), + [], + ); + // Reported as a gate-integrity problem, with the globs still harvested. + const wired = { + validate: "npm run test:scripts", + "test:scripts": `node --test --test-shard 1/2 ${GLOB}`, + }; + assert.deepEqual(testScriptGlobs(wired), ["scripts/**/*.test.mjs"]); + const problems = testScriptProblems(wired, ["scripts/a.test.mjs"]); + assert.equal(problems.length, 1); + assert.match( + problems[0], + /--test-shard.*runs only part of the matched suite/, + ); +}); + +test("integrityAdvice: typecheck footer only for typecheck issues (r35 finding 2 / nit 3)", () => { + const TYPECHECK = "clients/cli: the root `validate` chain no longer runs …"; + const SIBLING = + "the root `validate` no longer runs `verify:format-coverage` …"; + const TESTS = "scripts/a.spec.mjs: not matched by the `test:scripts` glob …"; + // Only typecheck-wiring issues → advise. + assert.match( + integrityAdvice([TYPECHECK], []), + /Restore the `typecheck` wiring/, + ); + // Every issue is a non-typecheck one → stay silent. The sibling-guard vouch + // and client-enrollment problems count here too, not just `test:scripts`. + assert.equal(integrityAdvice([TESTS], [TESTS]), null); + assert.equal(integrityAdvice([SIBLING], [SIBLING]), null); + assert.equal(integrityAdvice([SIBLING, TESTS], [SIBLING, TESTS]), null); + // Client enrollment ("declares no `typecheck` script …") IS about the + // typecheck pass — the footer's first clause is its fix — so it advises. + const ENROLLMENT = + "clients/tui: declares no `typecheck` script, has no `tsconfig.json` `references` …"; + assert.match( + integrityAdvice([ENROLLMENT], [SIBLING, TESTS]), + /Restore the `typecheck` wiring/, + ); + // A mix still advises — the typecheck issue is real. + assert.match( + integrityAdvice([SIBLING, TYPECHECK, TESTS], [SIBLING, TESTS]), + /Restore the `typecheck` wiring/, + ); +}); + +test("projectConfigFile: directory-form entry means /tsconfig.json (r26)", () => { + assert.equal( + projectConfigFile("clients/cli", "tsconfig.test.json"), + "clients/cli/tsconfig.test.json", + ); + assert.equal( + projectConfigFile("clients/cli", "packages/a"), + "clients/cli/packages/a/tsconfig.json", + ); + assert.equal( + projectConfigFile("clients/cli", "."), + "clients/cli/tsconfig.json", + ); +}); + +test("refToProject: refs resolve against the REFERRING config's dir (r26)", () => { + // A ref is relative to the tsconfig that declares it, not to clientDir. + assert.equal( + refToProject( + "clients/web", + "clients/web/tsconfig.json", + "./tsconfig.app.json", + ), + "tsconfig.app.json", + ); + assert.equal( + refToProject( + "clients/web", + "clients/web/sub/tsconfig.json", + "../other.json", + ), + "other.json", + ); + assert.equal( + refToProject("clients/web", "clients/web/sub/tsconfig.json", "./deep"), + "sub/deep", + ); +});