Review found the net still had the exact hole it was written to close.
The afterEach is synchronous, so a queued rAF callback cannot run until it
returns — at which point it registers a fresh setTimeout AFTER the sweep has
already drained. On a file's last test that timer survives environment
teardown. That is the rAF race described in the previous commit message, left
open by the fix for it.
Track animation frames too, and cancel them BEFORE sweeping timers. Order is
the fix: JS is single-threaded and nothing between the two loops yields, so no
frame callback can run in between. Mutation-checked — without the cancel pass
the new ordering test fails (1 failed | 6 passed); with it, 7 pass.
Also from review:
- renderWithMantine.tsx still documented the disproven env="test" behavior. It
is the file contributors are told to use, so leaving it would have preserved
the wrong belief exactly where it does most damage. That was the third copy of
this claim in the repo; all three now corrected.
- Dropped an unjustified `as unknown as` in the test helper, which broke the
repo's own double-cast rule. Inference keeps the handle type clearTimeout
accepts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
A window.setTimeout that outlives its test file fires after happy-dom disposes
that file's window; React's dispatchSetState then throws an uncaught
ReferenceError: window is not defined. Vitest attributes it to whichever file
was running, so a single leak fails the ENTIRE run with every test passing —
landing on an innocent file under a step named "Enforce per-file coverage gate."
Seen twice in CI, both on docs-only branches.
Not a Mantine bug, and not a badly-written test. Both hooks do clear on unmount
(useTransition's clearAllTimeouts, useLockScroll's effect cleanup). It is a rAF
race: clearAllTimeouts cancels the *pending* rAF, but the transition schedules
rAF -> rAF -> setTimeout, so if the inner callback is already in flight when the
unmount lands, cancelAnimationFrame is a no-op and that callback then schedules
a timer after cleanup has run. Nothing owns it. Needs a loaded machine, which is
why it only ever appeared in CI.
Track every timer and clear whatever is still outstanding after cleanup(). The
ordering is load-bearing: legitimate unmount cleanups get their turn first, so
only true leaks are dropped. Fake timers pass through untracked, which is
correct — a fake timer cannot outlive the environment.
Also corrects the claim, in AGENTS.md and in setup.ts's own comment, that
env="test" prevents this. It does not: env is read only by Transition.mjs at its
render branch, while useTransition runs before that check (hooks cannot be
conditional) and still schedules real timers. Measured — opening a <Modal>
through renderWithMantine schedules three 200ms timers. Believing that guarantee
is why this went unexamined.
The regression test is mutation-verified: with the net disabled it fails, with
it restored it passes. It deliberately does not assert that Mantine schedules no
timers — it does, and that is normal; the contract is that none survive.
Closes#1984
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
AGENTS.md requires review-relevant changes to land in the Copilot mirror in the
same PR, and the catalog-isolation rules are review-enforceable: a new web smoke
that spawns its own server, or a teardown that removes a work dir without first
awaiting stopChild, is something a reviewer should flag.
Mirrored as one line under "Gates and PR hygiene" rather than the AGENTS.md
paragraph. The mirror is read on every review and its own rule is "a
distillation, not a copy" — the mechanism (why the 409 appeared, why only the
catalog and not HOME is redirected, how the contract is tested) belongs in
AGENTS.md, while what a reviewer needs is the invariant and the two shapes that
violate it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Both round-3 review points, both mine.
1. The docblock claimed deleting `removeSafe` would leave "both the unit suite
and all three smokes green" — written before the test existed and never
updated, so it contradicted the test added in the same PR. Only the smokes
would stay green; the unit tests are precisely what fails on it.
2. `assert.ok(hasExited(child))` after the await proved only that the child was
dead by the time teardown returned. It passes just as happily if the catalog
were removed *first*, or while an un-awaited `stopChild` was still pending —
so it did not lock down the #1801 ordering it claimed to.
Sample the directory from the child's own `exit` handler instead, registered
before teardown so it runs ahead of stopChild's listener and observes the
world at the instant the child dies. `removeSafe` cannot have run by then: it
is sequenced after the promise stopChild resolves from that very event.
Mutation-checked against both defects the old assertion missed — removing before
stopping, and dropping the await. Each now fails the test; neither did before.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot was right that the previous test proved only that `removeSafe` can
delete a directory — it never drove `stop()`, so deleting the cleanup call
would have left the unit suite AND all three smokes green. That is exactly the
regression these tests exist to catch, and they did not catch it.
Extract the teardown into an exported `teardownWebServer({ child, catalogDir,
label })`, which `stop()` now delegates to, and drive it against a stand-in
child process (a real `node -e setInterval` that stays alive until signalled)
rather than a mock. Three cases:
- it removes the catalog dir it was given (asserted on the dir itself, not a
spy, so the assertion cannot drift from the behavior);
- it awaits the child's exit before removing — the #1801 race in one
assertion, since losing the await is otherwise invisible;
- an already-dead child is not an error, which is the failure path a smoke
takes when it calls fail() after the launcher has crashed.
Mutation-checked: with the `removeSafe` call deleted, 2 of the 3 now fail
(previously: 0). AGENTS.md updated to match.
Signed-off-by is now on every commit (#1978 DCO); the requirement itself is
undocumented and tracked in #1979.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses the two review comments that still applied to HEAD (the third, the
stop() race, was already fixed in a8022138).
- AGENTS.md still described the prod-web-server helper as shared by only
smoke:web and smoke:web:browser, and said nothing about catalog isolation.
It is the source of truth for these smoke mechanics, so record the invariant
there: the temp catalog per run and why, that only the catalog (not HOME) is
redirected, that teardown needs both halves of child-cleanup.mjs and is
therefore async for every caller, and where the contract is unit-tested.
- The `label` JSDoc claimed the option prefixes the temp dir. It does not —
that prefix is fixed at `smoke-web-catalog-` (deliberately, so every run's
dir is greppable); `label` only prefixes teardown warnings.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses both Copilot review comments on #1978.
1. `stop()` removed the catalog dir immediately after a bare `child.kill()`,
which only *delivers* SIGTERM. That is precisely the #1801 race
`child-cleanup.mjs:11-17` documents, and it says both halves are needed:
`stopChild()` closes the race on the normal path, `removeSafe()` makes the
residual case harmless. I had used only the second. `smoke-web-app.mjs`
already used `stopChild` for its MCP test-server child, so the helper was
inconsistent with its own sibling. `stop()` is now async — it awaits
`stopChild` and then removes — and all three callers await it
(`smoke-web.mjs`'s `fail()` became async to match, so every call site had to
`await fail(...)` or execution would run past it instead of exiting).
2. The isolation contract had no test. The smokes exit the process right after
teardown, so a regression reintroducing the shared catalog — or dropping
cleanup — would leave them all green. Split the two pure pieces out
(`createTempCatalog`, `buildWebServerEnv`) and cover them in a sibling
`prod-web-server.test.mjs`: a unique dir per run, no pre-created catalog
file, the dir is removable, and an inherited `MCP_CATALOG_PATH` (or
`MCP_AUTO_OPEN_ENABLED`) cannot beat the explicit one — the spread-then-assign
ordering that guard depends on. `startProdWebServer` itself stays untested by
design; it spawns a real launcher, which is the smokes' job.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
`startProdWebServer` spread `process.env` without setting `MCP_CATALOG_PATH`,
so `smoke:web`, `smoke:web:browser`, and `smoke:web:app` all ran against the
developer's real `~/.mcp-inspector/mcp.json` — unlike `smoke:cli` / `smoke:tui`,
which have always driven a temp `--catalog`.
That made the web smokes both destructive and non-deterministic.
`smoke:web:app`'s deep link persists a `deep-link` server row, so a second run
finds it already on disk, racing hydration: the deep-link effect misses it in
`servers`, POSTs `addServer` anyway, and the backend answers 409. The app
swallows that 409 by design (App.tsx:2842) so the smoke still passed, but it
reported a spurious non-fatal console error that was really just residue from
the previous run.
Give the helper its own temp catalog dir and clean it up in `stop()` via
`removeSafe`, so cleanup can never turn a passing smoke red. Isolation lives in
the shared helper rather than per script, since every web smoke needs it.
Only the catalog is redirected — other per-user state under `~/.mcp-inspector`
stays shared, since isolating it means redirecting HOME wholesale, which would
also move the npx and Playwright caches these smokes depend on.
Verified: `smoke:web:app` twice in a row is now clean both times (previously
the second run reported the 409), the real catalog's mtime is unchanged, and no
temp dirs leak. CI never saw this — a fresh HOME per run made every CI run look
like a first run.
Closes#1977
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Took v2/main's project-layout line and quality-gate table (adding
verify:dep-lockstep), keeping this branch's migration-guide mention and
prettier table alignment.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
README: kept v2/main's showcase table including the new structured-output
row. AGENTS: kept v2/main's newer type-label and triage rules, rendered in
this branch's markdown formatting.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Both sides added a showcase config row and section after the pagination
entry; kept both, structured output first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Two gaps from the Copilot review on #1975:
- The `Large` story asserted nothing, so dropping the section's `mah` (or
applying it to the wrong element) would still pass CI. Its play function
now measures the real ScrollArea viewport: the visible height stays at
the cap and the overflow is scrollable rather than clipped.
- Nothing exercised the `list_items` fixture, so a preset-registration
typo would leave the documented showcase broken with a green suite. A
new integration test loads `structured-output-http.json` through
loadConfig/resolveConfig — covering the file, the registry entry, and
the fixture — connects, calls the tool, and asserts both the summary
block and the nested structuredContent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
Review round 2. Three findings, all real.
1. Precision. Mantine's `NumberInput` returns the raw *string* rather than
a number once the value reaches `Number.MAX_SAFE_INTEGER` (see its
`isValidNumber` guard), precisely because JS cannot hold it. Parsing
that turned `90071992547409910` into `90071992547409904` and would
have sent the server a number the user never typed. An inspector must
not misreport what it transmits, so such input now reports no value —
which is also what this field did before #1888, so nothing regresses.
Only the integer part overflows, so long decimals stay parsed.
2. `AppDetailPanel` has the same reuse pattern as `ToolDetailPanel`:
`AppsScreen.handleSelect` swaps `selectedAppName` + `formValues` in
place without remounting, so switching between apps with a same-named
number field could retain an equal-valued draft. It now passes
`resetKey` too.
3. An exponent was never reachable: `NumberInput` masks input through
`NumericFormat`, which rejects `e`. Dropped that claim from the doc
comment rather than implying `"1e"` is a case being handled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84
- A named volume created by an image predating the ownership fix keeps its
root ownership if it is non-empty, since Docker only applies the image
directory's ownership to an *empty* volume. Verified both halves: an empty
pre-existing volume repairs itself on the first run of the fixed image,
while one holding a file stays root-owned and still EACCESes. Document the
distinction and the one-shot `chown` repair (verified to restore writes).
- "pair it with a known MCP_INSPECTOR_API_TOKEN" was bad advice for a wider
publication: the same paragraph notes `GET /` discloses the token, so a
custom one is harvested exactly as a generated one is. Point at a real
access-control boundary instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a
Review follow-up. The draft/value re-sync compares parsed numbers, so it
cannot see a reset to an *equal* value. `ToolDetailPanel` is not keyed by
tool, so its fields are reused across selections: type `-` (draft `"-"`,
value `undefined`), switch to a tool with a same-named number field and
no default, and the value is `undefined` on both sides — no divergence is
detected and the stale `-` stays in the box for the new tool.
Add a `resetKey` prop carrying the identity of whatever the form edits,
and vary the number field's React key with it so no in-progress text can
outlive the entity it was typed into. `ToolDetailPanel` passes the tool
name; nested object forms inherit it. The schema object is no substitute
— callers rebuild it every render, so its identity is unstable.
The regression test drives the switch with `fireEvent` rather than
`user.click`: a real click also blurs the input, and Mantine sanitizes an
incomplete value on blur, which masked the defect entirely. A paired test
asserts a *stable* `resetKey` does not remount, since remounting on every
render would wipe the draft and reinstate #1888.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84
Copilot review round 2, four findings, all factual errors in the docs added by
the previous commit:
- `yaml` is in the root `dependencies`, not `devDependencies`, and its only
importer is `test-servers/src/load-config.ts` — not `core/` or the root
tooling, as the `vitest.shared.mts` comment claimed.
- The rule as written told a reader to put anything reached through root-owned
code — `core/` included — in `devDependencies`. That is wrong and would break
the published package silently: the client builds externalize npm packages, so
a published install resolves them from the root manifest, where
devDependencies are absent. Placement (which manifest) and classification
(which section) are now stated as separate questions, the second following
from who consumes the package at runtime.
`yaml`'s runtime classification is left alone and called out instead: moving it
changes what ships, which is not a docs change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Closes#1964.
- README's Docker recipes dropped v1's `127.0.0.1:` prefix, so
`-p 6274:6274` published the Inspector on every host interface. The
container's `HOST=0.0.0.0` governs the container's interfaces, not the
host's, so `DANGEROUSLY_BIND_ALL_INTERFACES` never covered this. Verified
against a real container: from a LAN peer, `GET /` served the injected
`MCP_INSPECTOR_API_TOKEN`, and that token then unlocked `/api/*` with no
`Origin` header (server.ts:241 allows origin-less requests). Restore the
prefix on both recipes, on the port-remap examples, and in the web README.
- Adding a server failed inside the container the moment you mounted a
volume to keep it. Docker seeds a named volume's ownership from the
image's directory at the mount point, and creates it `root:root` when
that directory is absent — so the non-root `node` user got
`EACCES ... open '/home/node/.mcp-inspector/mcp.json.tmp-*'`. Create the
dir in the image, owned by `node`, and document the volume recipe (plus
the bind-mount `--user` caveat) — without a volume the catalog lives in
the writable layer and `--rm` throws it away.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a
Per the maintenance rules, a change to the dependency layout has to land in the
docs with it. Adds the placement rule in three places, at three altitudes:
- README (Setup): the rule plus the concrete drift it prevents.
- AGENTS.md: a "Dependency placement" section — the MCP SDK packages are root
only, the v1 SDK is not ours and must not become a dependency, and anything
reached solely through root-owned code with no manifest (`test-servers/src`,
`core/`) is a root devDependency aliased to the repo root in
`vitest.shared.mts`.
- .github/copilot-instructions.md: the distilled, reviewer-citable form, so a
diff re-adding one of these to a client manifest is flagged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
A tool declaring an `outputSchema` returns its real payload in
`structuredContent`; the `content[]` blocks usually only summarize it. v1
rendered that payload as its own inspectable JSON section — v2's
ToolResultPanel only ever walked `result.content`, so the payload was
dropped with no label, section, or hint that it existed.
Adds a `StructuredOutputPanel` group: a collapsible "Structured Output"
box (bordered like the existing "Resource Links" box) rendering the
payload as pretty-printed, syntax-highlighted, copyable JSON, bounded so
a large payload scrolls within itself. ToolResultPanel renders it below
the content blocks — and also when `content` is empty (where "No results
yet" was simply wrong) and below the alert on an error result, so it is
never silently dropped.
Adds a `list_items` test-server preset (nested payload, the issue's repro
shape) and a `structured-output-http.json` showcase config, documented in
the README.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
`SchemaForm` drove Mantine's `NumberInput` directly off the parent's
numeric value. `NumberInput` reports the raw string while the text does
not parse, so pressing `.` after `1` produced `"1."`, which the handler
collapsed to `undefined` — and because the control is fully controlled,
the `value` prop immediately rewrote the box back to `1`. The decimal
point was discarded on every keystroke, making `1.5` unreachable. A
trailing zero, a lone leading `-`, and an exponent failed the same way.
Introduce `SchemaNumberInput`, which holds the in-progress text as the
source of truth for what is displayed while the parent still only ever
sees a `number | undefined`. Draft and value re-sync only when they
genuinely diverge, via `useValueChange` (adjust-state-during-render)
rather than an effect, so an external reset still rewrites the box but
an in-progress `"1."` is left alone.
Also pass `allowDecimal` from the schema, so an `integer` field rejects
the decimal point instead of accepting a value its schema forbids —
matching what the TUI already does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84
The four v2 SDK packages (`client`, `core`, `server`, `server-legacy`) plus
`ext-apps` were declared in the root manifest *and* again in clients/web, cli,
and tui, so each client installed its own copy. Node resolution walks up and the
root install is on every client's chain — and the root manifest is already the
documented source of truth for what ships — so the per-client entries were
duplicates of a decision made at the root, and they had already drifted: before
this change the tree carried ext-apps 1.7.4 at the root and 1.7.5 under
clients/web, dragging in two copies of the v1 `@modelcontextprotocol/sdk`
(1.29.0 and 1.30.0) through ext-apps' peer dependency.
Nothing imports the v1 SDK; it is not in any manifest of ours, only a peer of
ext-apps, so consolidating ext-apps is the only lever over it. After a clean
install there is now exactly one copy of each, at the root. `npm update` then
moves ext-apps to 1.7.5 and its SDK peer to 1.30.0, whose widened
`@hono/node-server` range (`^1.19.9 || ^2.0.5`) also drops a nested duplicate of
that package.
`express` needed a real home to make this work. `test-servers/src` imports it,
but no manifest declared it: it was reaching `clients/cli/node_modules` only as
a peer of `express-rate-limit` under `@modelcontextprotocol/server-legacy`, so
removing that entry took express with it and every cli test that spawns a test
server failed to resolve it. It is now a root devDependency — test-servers is
root-owned code with no manifest of its own — and `vitest.shared.mts` resolves
it from the root, joining `yaml`, which was already the precedent for exactly
this case. The clients/web duplicate is dropped; its only reference is a
type-only import covered by `@types/express`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Copilot review round 4: every await before the `connect` dispatch is a window
for a `disconnect()` or a transport close to overtake the connect, and the
listen round-trip widened it. Announcing anyway restarts every managed list
refresh against a session being torn down or already dead.
The dispatch is now gated on `status === "connected" && !disconnecting` — the
second because an explicit teardown claims ownership before it awaits
`client.close()`, leaving the status untouched until that block finishes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Copilot review round 3 (no new code findings, three stale-comment ones):
- `openModernListenStreamOnConnect`'s generation guard was justified by a
`connect` listener racing it — which the previous commit made impossible by
dispatching `connect` after this call. The guard is still right, for reasons
that survive the reorder: `statusChange` has already fired, and a concurrent
`subscribeToResource` or `disconnect()` (whose `resetSubscriptionStream` bumps
the generation) can supersede it.
- `reconcileModernStreamStateAfterFailedRefresh`'s JSDoc still described its
branches as "nothing subscribed" vs "URIs live"; they are the empty vs
non-empty *filter* now, which differ exactly on the listChanged-only path.
- The superseded-failure test carried the same stale `connect` rationale.
Comments only; no behavior change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
The guard restated `primaryShade.light` as a literal 7, so re-pinning the
theme's primary shade would have made it fail even with the preview
annotations correctly applied — a guard that has to be edited alongside a
legitimate theme change just trains people to edit it.
Derive the shade from `theme.primaryShade`, handling both of Mantine's
forms (a bare number and `{ light, dark }`) and falling back to Mantine's
own default of 6 when a theme pins none.
Verified by temporarily re-pinning the theme to `{ light: 8, dark: 8 }`:
the guard still passes, where the hard-coded 7 would have failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84
Copilot review round 2: opening the stream after `dispatchTypedEvent("connect")`
left a real gap. The managed list states start their initial `refresh()` from
that event, so `tools/list` could go out ahead of `subscriptions/listen` — and a
list the server changed in that window would notify nobody, leaving the UI
stale with no way to notice.
The handler registration, the initial logging level, and the stream open now all
run before the `connect` dispatch, so no consumer can act on the connection
until the notification channel is established (or its retry armed). Costs one
listen round-trip on a modern connect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Since Storybook 10.3, @storybook/addon-vitest provisions the preview
annotations itself — and *skips* doing so when it finds a setup file
calling setProjectAnnotations. So .storybook/vitest.setup.ts was not
merely redundant, it was actively opting the project out of the
automatic path, which the addon printed a notice about on every run.
Remove the file and the `setupFiles` entry from the `storybook` vitest
project.
A green suite doesn't prove the automatic provisioning works: without
the preview annotations, stories would render outside MantineProvider
and without App.css, and would very likely still pass. Add
src/test/PreviewAnnotations.stories.tsx, which asserts from a play
function that the Mantine theme variables and the App.css tokens are
actually present in the rendered document, so an unthemed render fails
loudly. Verified by emptying `decorators` in preview.tsx and confirming
the story goes red.
The a11y addon's axe runner can't be introspected from a play function,
so it was checked out-of-band: a story with a real violation turned the
suite red with the setup file already removed, confirming
@storybook/addon-a11y/preview is applied automatically.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84
Copilot review: `openModernListenStreamOnConnect`'s catch reconciled
unconditionally, unlike the subscribe/unsubscribe call sites. The `connect`
event is dispatched before the stream opens, so a listener subscribing to a
resource can start and acknowledge a newer refresh while this one is still
awaiting its `listen()` — and reconciling anyway would arm a reconnect that
tears down a healthy stream. Apply the same generation test.
Also consolidates the tests' private-member access onto the file's existing
`internals()` helper (one justified double cast, hoisted and documented) and
makes the failure stub bump the generation the way the real method does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
On the modern (2026-07-28) era every server→client notification rides the
`subscriptions/listen` stream, but the Inspector only ever opened that stream
from a resource-subscribe click. A tools-only server advertising
`tools.listChanged` therefore had no path to an open stream, so
`notifications/tools/list_changed` could never arrive — even though
`buildSubscriptionFilter()` already modelled the opt-in.
The trigger now matches the filter it builds: the stream opens (and stays open,
and reconnects) whenever the built filter carries anything — subscribed URIs or
an enabled list-change opt-in the server advertises. That mirrors the SDK's own
`ClientOptions.listChanged` auto-open, which opens on a non-empty config ∩
capability intersection. `connect()` opens it once the handlers are registered;
a failure there is handed to the reconnect machinery rather than failing the
connect.
`ResourceSubscriptionStreamState.active` deliberately keeps its narrower
meaning — "at least one URI is subscribed" — because it drives the Subscriptions
section's badge, which has nothing to report for a listChanged-only stream. That
also preserves the invariant the rest of the file is written against: an empty
subscribed set is never announced alongside an `active` stream.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Two pieces of hardening on the Claude Code workflow, plus a comment for a
finding that needed no change.
Same-repo gating. `Get PR details` now emits an `is_fork` output, and the
PR-head checkout and the `Run Claude Code` step are both gated on it. A fork
PR is declined outright — no checkout, no agent run, with the reason written
to the step summary — rather than falling through to a "metadata-only" review
of the base tree, which would trade an untrusted-code problem for a
wrong-tree one. A deleted fork (`head.repo` null) counts as a fork.
The head checkout also drops its `repository:` input: only a same-repo head
reaches that step now, and checkout's default `github.repository` is a value
no PR can influence.
SHA pins. actions/checkout, actions/github-script, and claude-code-action
were all on mutable major tags in a job that holds ANTHROPIC_API_KEY and
grants the agent Bash. Each is now pinned to a full commit SHA with the
trailing `# vX.Y.Z` comment Dependabot reads, so upgrade automation is
unaffected. Pinned to the v7 / v9 SHAs so this does not regress the bumps in
the open Dependabot PR #1922.
The failure fallback Copilot flagged turns out not to exist: `Checkout
repository` carries no status-check function, so GitHub applies an implicit
`success()` and skips it after a failed lookup regardless of the `outcome`
comparison. Its condition is now the explicit `== 'skipped'` and says so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
From Copilot's fifteenth pass of #1962. `installDirs` filtered on the presence
of a lockfile, so a missing one silently removed that install from the
comparison rather than failing. For the root — the install every shared source
resolves from — that meant the guard could report success from client locks
alone, which is the deny-by-default contract inverted.
Enrolment is now by `package.json` (an install we are meant to compare), with
the root always enrolled, and a missing lockfile is a named, loud failure. A
`clients/` directory with no `package.json` is not an install and is skipped,
so a stray scratch dir isn't asked for a lockfile it should never have.
Three regression cases: a missing client lockfile in a fixture that IS skewed
(so failing open would show as a pass), a missing ROOT lockfile, and a stray
`clients/` dir that must not be enrolled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's fourteenth pass of #1962. `hasReadableLockShape` and the
diagnostic it drives both promise "lockfileVersion 2+", but the check only
looked for a `packages` table with a root entry. A file declaring v1 while
carrying such a table was accepted, produced an empty version map, and could
make a real skew read as aligned once only one other holder remained — the
same fail-open the shape check was added to close, through the half of the
contract that wasn't enforced.
The declared version is now verified: a finite number, 2 or greater. Absent,
non-numeric, and declared-v1-with-packages cases are all in the table.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's thirteenth pass of #1962. This PR added `verify:dep-lockstep`
to the vouch loop in `verify-format-coverage.mjs` without a test for it. The
existing tests exercise `rootReachesScript` in isolation and the opposite
direction (dep-lockstep vouching for format-coverage), so a typo in a sibling's
name here would have left `test:scripts` green while that guard silently
stopped being enforced — the same "a gate that stops gating" failure the
vouch cycle exists to prevent, one level up.
Three cases, reusing the fixture-repo approach: dropping either sibling from
`validate` exits 1 naming that sibling, and with both wired no sibling is
reported missing. The last asserts on the *reason* rather than the exit
status, since the fixture has no client manifests to harvest globs from and
so fails later for an unrelated reason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's twelfth pass of #1962. A `/// <reference types="x" />` pulls in
a package's declarations exactly like an import does, but TypeScript reports
it in `preProcessFile`'s `typeReferenceDirectives`, not `importedFiles` — and
only the latter was read. A shared source using one could therefore load the
root's copy of a package alongside a client's without it ever entering the
candidate set, so its skew would pass. Same silent-miss direction as the
earlier syntax gaps, through a channel the parser switch didn't cover because
I only read one of its two outputs.
`typeReferencePackageNames` maps a directive to both candidates, since the
name is the *type* name and not the package: `node` resolves to `@types/node`,
while a package shipping its own declarations resolves to itself. Returning
both over-approximates, the safe direction — whichever isn't installed drops
out downstream. Scoped names mangle `@scope/pkg` → `@types/scope__pkg`,
TypeScript's convention. `path` references name a file, not a package, and are
ignored.
The real candidate set is unchanged at 17: no shared source uses a triple-slash
reference today. As with the other syntax fixes, this closes the channel
against the one that arrives later.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's eleventh pass of #1962, and the most consequential kind of bug
for a gate: it failed *open*.
`topLevelLockVersions` returns an empty map for anything without a v2+
`packages` table — a lockfileVersion 1 file, or a malformed one. `main()` fed
that straight into the comparison, so such an install contributed no holders
at all, and a genuine skew among the remaining installs was reported as
aligned. The guard would have said OK to exactly what it exists to catch.
`hasReadableLockShape` now gates every discovered lockfile before any
comparison, requiring a `packages` object carrying the `""` root entry npm
always writes — "has a packages key" is not enough. An unreadable one is named
and exits 1, telling you to regenerate it. A JSON parse failure likewise
reports the file rather than dying on a bare SyntaxError with no path in it.
The empty-map behavior of the pure helper is kept and its test now says why it
is safe: the shape check rejects those inputs before they can reach
`findSkew`.
Three regression cases: a v1 lockfile in a fixture that IS skewed (so failing
open would show as a pass), a `packages` table with no root entry, and the
unit table for the predicate. Fixtures now write the `""` root entry, matching
real npm output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's tenth pass of #1962. The suite covered only pure helpers, so a
regression in source enumeration, install discovery, lockfile loading, the
sibling-guard vouch, or the nonzero exit on real skew would have left it
green — and the negative cases had only ever been run by hand, once per review
round.
Five end-to-end cases build a throwaway repo and run the guard as a
subprocess: aligned installs exit 0; a skewed one exits 1 naming the package
and every holder while leaving the agreeing package unreported; a package
imported only by `test-servers/src` still fails (so dropping the second shared
tree can't pass); a shared source matching no tracked file fails; and a
`validate` chain that stopped running `verify:format-coverage` fails the vouch
even when the versions are fine.
The fixture must be realpath'd. On macOS `tmpdir()` is `/var/…`, a symlink to
`/private/var/…`, and the guard runs `main()` only when `import.meta.url` (the
resolved path) matches `process.argv[1]` — so launching it via the unresolved
path loads the module and does nothing, exiting 0 silently. That is how all
five first "passed" as no-ops, and the comment records it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
