inspector/.github/copilot-instructions.md
Cliff Hall 834fd2d77d
chore(deps): migrate to eslint 10 to clear the brace-expansion DoS advisory (#1900)
* chore(deps): migrate to eslint 10 to clear the brace-expansion DoS advisory (#1838)

eslint 9 pulled `minimatch@3` → `brace-expansion@1.1.16`, which carries
GHSA-mh99-v99m-4gvg (high, DoS via unbounded expansion length). It appeared in
all five lockfiles — the root plus each of the four clients, none of which is an
npm workspace, so all five had to move together.

Deferred from the pre-2.0.0 sweep because it is dev-scope, not attacker-
controlled, and a major bump across five packages was the wrong thing to attempt
immediately before an irreversible publish. This is that work.

## What moved

- `eslint` `^9.39.4/5` → `^10.8.0` and `@eslint/js` → `^10.0.1`, in all five
  manifests.
- `eslint-plugin-react-hooks` `^7.0.1` → `^7.1.1` in web and tui. Required, not
  cosmetic: 7.0.1's peer range stops at `^9.0.0`, so installing it alongside
  eslint 10 fails `ERESOLVE`.
- Everything else already declared eslint 10 support and stayed put:
  `typescript-eslint@^8.56.1` (`^8.57 || ^9 || ^10`),
  `eslint-plugin-react-refresh@^0.5.2` (`^9 || ^10`),
  `eslint-plugin-storybook@^10.2.19` (`>=8`).

## Config migration

None was needed. All five configs were already flat (`eslint.config.js` with
`defineConfig`/`globalIgnores` from `eslint/config`), which is the format v10
keeps and the only one it still supports. The root config's `core/` and shared-
surface blocks — including the `_`-prefix `argsIgnorePattern` /
`varsIgnorePattern` / `caughtErrorsIgnorePattern` — are unchanged and still gate
`lint:core` and `lint:shared`.

No rule was disabled, downgraded, or scoped away to make the new eslint pass.
v10 surfaced 21 real violations across three rules; all 21 are fixed in the code.

### `preserve-caught-error` (new in `eslint:recommended`) — 15 sites

A `throw new Error(msg)` inside a `catch` silently drops the original error.
Each now passes `{ cause }`, so the underlying failure survives into the chain.
Messages are unchanged, so nothing that asserts on them moves. 12 in `core/`,
2 in `clients/cli`, 1 in `test-servers/src`.

### `no-useless-assignment` (new in `eslint:recommended`) — 2 sites

`clients/cli/src/cli.ts`'s `optionArgs` and `test-servers/src/test-server-oauth`
's `valid` were initialized and then unconditionally overwritten on every path.
Both are now declared without the dead initializer; TypeScript's definite-
assignment analysis covers the remaining paths.

### `react-hooks/set-state-in-effect` — 8 sites (web)

These were false negatives the plugin fixed in 7.1.0, not a rule we newly opted
into: `set-state-in-effect` has been `error` in `flat/recommended` since 7.0.1,
and the web config has always extended it. Every one was the same anti-pattern —
resetting or re-syncing local state from a prop inside a `useEffect`, which
paints the stale value for one frame and then renders again to correct it.

All eight now use a new `useValueChange(value, onChange)` hook
(`clients/web/src/hooks/useValueChange.ts`), which is React's documented
"adjusting state during render": compare against the previous render's value
with `Object.is` and call back *during* render, so React discards the
in-progress output before it reaches the DOM.

- `MrtrConversation`, `ProtocolEntry`, `TaskCard` — mirror `isListExpanded`.
- `NetworkEntry` — same mirror, plus the "Reveal in Network" force-open. The
  reveal's rAF scroll stays an effect (real external-system work); only the
  `setIsExpanded(true)` moved. `isExpanded` is now seeded from
  `isListExpanded || revealed`, since a render-time sync fires on change and so
  cannot cover an entry that mounts already revealed.
- `PromptArgumentsForm`, `ResourceTemplatePanel` — reset on template/prompt
  switch.
- `ServerConfigModal` — reset on open. Keyed on `opened ? initial : undefined`,
  which collapses "opened flipped" and "initial changed while open" into one
  value; `initial` is already memoized, so it cannot thrash.

`useValueChange` has its own test file, and `ServerConfigModal` gains an
open→edit→close→reopen test (the close leg was previously untested, which is
what left the new guard uncovered).

## Advisory delta

`npm audit`, per package, before → after:

| Package             | before                     | after                      |
| ------------------- | -------------------------- | -------------------------- |
| root                | 9 (1 low, 6 mod, 2 high)   | 8 (1 low, 6 mod, 1 high)   |
| `clients/web`       | 19 (3 low, 6 mod, 6 high, 4 crit) | 17 (3 low, 6 mod, 4 high, 4 crit) |
| `clients/cli`       | 6 (2 low, 4 high)          | 4 (2 low, 2 high)          |
| `clients/tui`       | 4 (1 low, 3 high)          | 2 (1 low, 1 high)          |
| `clients/launcher`  | 3 (3 high)                 | 1 (1 high)                 |

`brace-expansion` is gone from all five — `eslint@10` resolves `minimatch@10` →
`brace-expansion@5.0.9`. Dropping `@eslint/eslintrc` (removed in v10) also took
`js-yaml` with it, clearing GHSA-52cp-r559-cp3m (high) from all four clients as
a bonus. The remainder are unrelated and tracked separately.

`npm run ci` passes end to end — validate, the per-file ≥90 coverage gate, the
build gate, all five smokes (including `smoke:tui`, run locally on a real TTY),
and 462 Storybook tests.

Also documents the effect rule in AGENTS.md and mirrors it into
`.github/copilot-instructions.md`, since it is now something a reviewer cites.

Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: state useValueChange's referential-stability requirement (#1838)

Copilot review on #1900: the hook compares with `Object.is`, so an object or
array literal rebuilt in the component body compares unequal every render.
Because the callback is what updates state, that is not merely a wasted render —
it is an infinite loop.

Every current caller passes a primitive (`boolean`, `string`) or an already-
memoized value, so nothing changes behaviorally. But the hazard was implicit,
and the next caller is the one it would bite.

Documented on the hook itself, and one clause added to the AGENTS.md rule with
its mirror in `.github/copilot-instructions.md`. No runtime guard: the
requirement is the same one a `useEffect` dependency array carries, and here the
failure is loud and immediate rather than silent.

`npm run ci` passes.

Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(cli): stop asserting the --tool-args-json catch binding is an Error (#1838)

Copilot review round 2 on #1900: the `--tool-args-json` parse handler formatted
its message with `(e as Error).message`. A `catch` binding is `unknown`, so that
cast is an assertion rather than a check — a thrown non-Error would render
`--tool-args-json is not valid JSON: undefined`, which points the user at
nothing.

`JSON.parse` does only throw `SyntaxError` today, so this is not a live bug. But
the cast is the kind AGENTS.md asks us not to leave unjustified, the same
handler was already being touched here to add `{ cause: e }`, and every other
site in this PR uses the guarded form. Now it does too.

`npm run ci` passes.

Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: require useValueChange's onChange to be render-pure (#1838)

Copilot review round 3 on #1900: `onChange` is called during render, so it is
bound by render-phase purity — and the web client mounts under `StrictMode`,
which deliberately double-renders in development. A callback doing anything
external would run an unpredictable number of times, and concurrent React can
abandon an in-progress render entirely.

Every current caller already passes a callback that only calls `setState`, so
nothing changes. But "runs during render" is the whole point of the hook and the
constraint that follows from it was left implicit.

Stated on the hook, with the same clause in the AGENTS.md rule and its
`.github/copilot-instructions.md` mirror. All three now point at `NetworkEntry`
as the worked example of the split: the reveal's force-open is a state update
and lives in `useValueChange`, while its `requestAnimationFrame` scroll stays a
`useEffect`.

`npm run ci` passes.

Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-01 20:13:30 -04:00

11 KiB

Copilot review instructions — MCP Inspector

AGENTS.md is the source of truth. This file is a review-focused distillation of it, kept in sync by hand — see "Keep documentation files up to date" in AGENTS.md. Where the two disagree, AGENTS.md wins, and the drift is a bug worth flagging in review.

The Inspector ships as one package with three clients (Web, CLI, TUI) over a shared core/, consumed via the @inspector/core build-time alias. v2 is not an npm workspace: the root and each clients/* keep their own package.json and node_modules.

TypeScript

  • Never use any. Not in types, not in casts, not in generics.
  • Never suppress errors to satisfy the linter or compiler — no disabling no-unused-vars / no-explicit-any in config, and no // @ts-nocheck or // @ts-ignore (@typescript-eslint/ban-ts-comment rejects these across every surface).
  • Avoid double casts (as unknown as T). They erase all type safety and usually mean the real type is being worked around. Prefer a type guard, a narrower single cast, or fixing the underlying type. If genuinely unavoidable (a documented gap in a third-party type, or bridging structurally-identical shapes TS can't relate), it must carry an inline comment justifying why it's safe and why nothing better exists. An unjustified as unknown as is not acceptable in review.
  • Prefer inference, type guards, and precise annotations over assertions.
  • An _ prefix is the intentionally-unused marker (argsIgnorePattern / varsIgnorePattern / caughtErrorsIgnorePattern).

React and UI (web client)

The web client is built from presentational ("dumb") components — they take data and callbacks as props and hold display logic only. No data fetching or client state inside them; state comes from the @inspector/core hooks wired near the top of the tree. A component that reaches for a store or fetches directly is a review finding.

Styling is Mantine-first, in this strict order of preference: component props → theme variants → CSS classes (last resort).

  • Never use inline styles.
  • Never use raw color literals — no hex (#ddd), no rgba(). Use the --inspector-* CSS custom properties from App.css :root (e.g. c: 'var(--inspector-text-primary)'). If no token fits, add one to :root first.
  • Avoid div and bare HTML for layout. Use Mantine Box, Group, Stack, Flex, Paper.
  • Never add a CSS class when the styles can be component props or a theme variant. Flat CSS properties (margin, padding, background, border, color, font-size) belong in the theme (src/theme/<Component>.ts, via Component.extend()). App.css may contain only what the theme cannot express: @keyframes, pseudo-selectors (:hover, :focus), cross-component hover relationships, nested child selectors for third-party HTML output, and styles for native elements (img, iframe).
  • When a theme variant needs a class for nested/pseudo selectors, assign it via classNames in the theme extension — never a manual className in JSX for theme-styled components.

The .withProps() rule

Declare a named subcomponent constant via .withProps() whenever an inline Mantine element carries two or more static props. This applies to single-use elements too — "it's only used once" is not an exemption.

  • Static = a literal configuring styling, layout, or behavior: size="sm", c="dimmed", fw={500}, gap="xs", justify="space-between", variant="light", withBorder, readOnly, striped.
  • Not counted: dynamic props (value, on*, children, key, ref, anything whose value is a variable) — pass these at the call site; and per-instance content/accessibility literals (label, description, placeholder, title, aria-label, role) — these never by themselves trigger extraction.
const CardContent = Group.withProps({
  flex: 1,
  align: "flex-start",
  justify: "space-between",
  wrap: "nowrap",
});

Legitimate exceptions (each stays inline, with a one-line comment saying why):

  • Box — does not support .withProps(). Use Group/Stack/Flex/Text/Paper/UnstyledButton/Image instead, chosen by purpose. A Box that genuinely needs a non-flex primitive (component="iframe", display="grid") stays inline.
  • Accordion — a compound, multiple-discriminated generic; .withProps() loses its JSX call signature and fails to type.
  • Headless, non-factory() components such as Transition — no Styles API, so no .withProps static at all.
  • data-* attributes — not part of a component's typed props, so excess-property-checked out of a withProps literal. Pass at the call site.
  • Anything that isn't a Mantine factory component — a react-icons glyph, another library's component, or a first-party plain export function.

State and effects

  • Never reset or re-sync local state from a prop inside a useEffect. useEffect(() => setX(prop), [prop]) paints the stale value first and renders twice; it is an error under react-hooks/set-state-in-effect.
  • Use useValueChange(value, onChange) (src/hooks/useValueChange.ts) — React's documented "adjusting state during render" pattern. It does not fire on the first render; seed the state with useState. The comparison is Object.is, so pass a referentially stable value — a primitive key (id/name/URI) or a memoized one, never a fresh object literal.
  • The onChange runs during render, so it must be pure — setState and nothing else. No fetches, DOM writes, logging, ref mutation, or parent callbacks; a render can be replayed or abandoned.
  • Effects remain correct for real external-system synchronization (DOM measurement, rAF, subscriptions, timers).

Theme files vs. element components

Both exist and do different jobs. Theme files (src/theme/<Component>.ts) customize a Mantine primitive app-wide. Element components (src/components/elements/) add domain semantics on top of primitives.

  • Element components import from @mantine/core, not from src/theme/ — the theme layer is applied transparently by the provider.
  • Never push domain-specific variant logic into theme files (annotation types, transport types, …). Domain variants belong to the element component that owns those semantics.

Where code goes (web client)

utils = functions that compute; lib = things that instantiate, adapt, or touch the environment. If it does I/O or wraps a subsystem it's lib; if it's a pure transform it's utils.

  • src/utils/ — pure, side-effect-free. Also: pure shared domain types and their constructors; diagnostic console.warn/error does not count as a side effect; type-only imports from @inspector/core, and re-exporting pure functions/constants from core, are both fine.
  • src/lib/ — infrastructure, integration, stateful adapters: composes subsystems, wraps the core runtime, touches DOM / window / sessionStorage, or produces side effects.
  • Cross-directory imports go one way: lib → utils, never the reverse.
  • src/types/ is only for ambient .d.ts module stubs — not a home for new domain types.
  • ⚠️ The coverage include is a whitelist naming components / hooks / theme / lib / utils / server (plus the core/* runtime). A module placed outside those directories silently falls out of the ≥90% gate. Flag new top-level files or new grab-bag directories.

Tests and the coverage gate

  • All new or modified code needs tests. The per-file gate is ≥ 90% on all four dimensions — lines, statements, functions, and branches — enforced in CI for clients/{web,cli,tui,launcher} and the gated core/ runtime.
  • Never lower the gate to accommodate an unreachable branch. Annotate at the source with a justified /* v8 ignore … -- <reason> */. Acceptable reasons: happy-dom-inherent paths (Mantine portal mounts, useMediaQuery fallbacks, typeof window SSR guards), React StrictMode effect-replay blocks, and provably-dead defensive guards. Anything else is a missing test.
  • Suppress expected error output from the console in tests that exercise error paths.

Test placement

  • Web: side-by-side by default — <Name>.test.tsx next to the source. A web-owned test under src/test/ instead of beside its source is a bug. src/test/ is only for what can't be co-located: tests of the repo-root core/ package (src/test/core/…, mirroring core's layout), the integration project (src/test/integration/… — placement is the manifest), and shared test infrastructure.
  • CLI / TUI / launcher: all tests live in a top-level __tests__/ directory. A co-located src/**/*.test.* lands in no tsconfig project and fails verify:typecheck-coverage.

Rendering components in tests

  • Always render through renderWithMantine from src/test/renderWithMantine.tsx. Never hand-roll a bare MantineProvider — that reintroduces a real failure class where a Transition/Modal timer fires after happy-dom tears down window and fails the entire run.
  • For a forced color scheme, pass the option — renderWithMantine(ui, { colorScheme: "dark" }) — rather than a hand-rolled defaultColorScheme provider.
  • Only when asserting mid-flight transition state, use renderWithMantineTransitions, passing settleMs derived from the component's real animation duration. Do not combine it with vi.useFakeTimers(), and use the unmount() it returns if the test unmounts the tree itself.

Gates and PR hygiene

  • npm run format before committing; npm run ci before pushing (validate → coverage → verify:build-gate → smoke → Storybook). npm run validate is the fast inner-loop check and is not a substitute — it runs test, not test:coverage, so it does zero coverage gating.
  • Every PR references an issue, first body line Closes #<ISSUE_NUMBER>.
  • Every PR carries exactly one version label, v1 or v2, matching its base branch.
  • Update the relevant README.md / AGENTS.md when a change adds, removes, renames, or repurposes a file or folder, changes the structure or tech stack, or introduces a command, dependency, or architectural pattern.

What to prioritize in review

  1. Correctness and security — this backend spawns local processes and proxies outbound requests, so anything touching auth, origin validation, host binding, or the proxy's SSRF controls deserves close reading.
  2. Type-safety violations (any, suppressions, unjustified double casts).
  3. Missing or thin tests against the ≥90% four-dimension gate, and modules placed outside the gated directories.
  4. Mantine convention violations — inline styles, raw colors, unnecessary CSS classes, missing .withProps() extraction.
  5. Docs that contradict the change.