modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2
3,887 commits 1 branch 87 tags 30 MiB
Commit graph inspector/core/mcp/state/messageLogState.ts
Author SHA1 Message Date
cliffhall
f201032137 fix: surface list-fetch failures instead of swallowing them 
A list load that failed — a transport error, or a result the SDK's era
codec rejects as invalid — reported nothing. The connect-time refresh is
fired with no caller to await it (`void this.refresh()`), so its rejection
became an unhandled rejection: the header said "Connected", the panel
rendered an empty list indistinguishable from a server that has none, and
the failed exchange showed in the Protocol panel as a clean success.

Two surfaces now tell the truth:

- ManagedListState records the failure as observable state (`getError` +
  an `errorChange` event) and still re-throws, so App's auth-recovery
  wrapper keeps working. The four `useManaged*` hooks expose it via a
  shared `useManagedListError`, and the Tools/Prompts/Resources sidebars
  render a `ListLoadError` alert with the reason and a Retry.
- A response the client refused is attributed back to its Protocol entry
  (`markResponseRejected` → `responseRejected` → `MessageEntry.clientError`),
  which now renders an Error status and a "Rejected by the Inspector"
  alert rather than the green resultType badge alone. The SDK gives no
  request id with a decode failure, so the id is recovered by correlation;
  see the comment on `markResponseRejected` for why that is exact.

Also stops `listAllTools` from dropping a failing excluded-tools walk on
the floor — it stays non-fatal but is logged.

Closes #1953

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
 2026-08-08 16:48:48 -04:00
Cliff Hall
b3b04c6939
 		Thread cacheMode through managed list refresh via SDK cache-aware verbs (#1721) (#1723)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Details
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Details
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
Details 
* Thread cacheMode through managed list refresh via SDK cache-aware verbs (#1721)

Manual/auto refresh of tools/resources/prompts now forces a cache-bypassing
round trip on modern servers, delivering the raw-request cacheMode control
deferred from #1628.

The InspectorClient list/read path uses the SDK's single-page `client.request`
verbs (for pagination debugging), which never consult the SDK response cache —
so `cacheMode` was inert there. This adds a separate aggregate path that routes
through the SDK's cache-aware high-level verbs, where `cacheMode` is honored:

- InspectorClient: new `listAllTools/Prompts/Resources/ResourceTemplates(
  { cacheMode, metadata })` that call `client.listTools()` etc. (all-page
  aggregate + SEP-2243 x-mcp-header exclusion + cache consult/write). A shared
  `getCacheableRequestOptions` / `aggregateListParams` back all four.
- InspectorClientProtocol: the four `listAll*` methods added.
- ManagedListState: delegates page walking to the SDK verb (config `fetchPage`
  loop → `fetchAll`); `refresh(metadata?, cacheMode?)` threads cacheMode. A
  `list_changed` auto-refresh bypasses the cache (`cacheMode: "refresh"`).
- Managed hooks: user-initiated `refresh()` passes `cacheMode: "refresh"`, so
  the existing per-tab Refresh buttons become genuinely cache-bypassing — no
  App wiring change. Connect-time load stays default (`'use'`).

The single-page `listTools(cursor)` primitive is unchanged; the paged hooks
still surface per-page `nextCursor`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* Single-page list pagination: sidebar toggle + server setting (#1721)

Delivers the deferred single-page fetch UI: Tools/Resources/Prompts can be
loaded one page at a time instead of auto-aggregating every page — a defensive
option for servers with very large lists.

- New `singlePageLists` server setting (persisted, server-wide), plumbed through
  ServerConfig/InspectorServerSettings, serverList convert/default, the node +
  remote settings paths, and the Server Settings form ("Fetch Lists One Page at
  a Time").
- New `ListPaginationControls` element (a "Single page" switch + "Load next
  page" button + "N pages loaded" status) rendered in each list sidebar; the
  toggle edits the setting live (optimistic) and persists it.
- The paged state stores (PagedTools/Prompts/ResourcesState) now own loading:
  they auto-load page 1 on connect in single-page mode and track nextCursor +
  page count as observable state; the managed stores skip their all-page walk
  in single-page mode (so nothing is over-fetched) while still lighting the
  list-changed indicator. `usePaginatedList` selects the display source and
  pagination controls per mode — pure, no data-loading in React effects.
- Refresh is mode-aware: reload page 1 in single-page mode, refetch the whole
  aggregate otherwise. Sidebars widened 340→360 so the controls don't crowd
  list entries.
- Test server: `test-servers/configs/pagination-http.json` (12 tools/resources/
  prompts, page size 4 → 3 pages each) exercises the flow end to end.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* Add pr-screenshots proving single-page pagination (#1721)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* Address @claude review: template load, double-click guard, auth recovery (#1721)

- Fix: resource templates never loaded on connect in single-page mode. The
  connect-time all-page skip now applies only to lists with a paged counterpart
  (new `deferInSinglePage` flag on ManagedListConfig; false for resource
  templates), so templates still aggregate on connect regardless of the setting.
- Guard `PagedTools/Prompts/ResourcesState.loadPage` against a concurrent load
  (fast double-click on "Load next page") so the same page can't be appended
  twice; the in-flight load owns the cursor and the second call is a no-op.
- Thread ambient auth recovery through the single-page refresh, Load-next-page,
  and mode-switch loads (they previously bypassed it) — usePaginatedList's
  onRefresh/onLoadMore now return their promises so App can wrap them.
- Drop the misleading "aggregated list" wording from the paged-store docs.

Tests added for each; npm run coverage green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* Review round 2: revert optimistic toggle when persisting the setting fails (#1721)

Non-blocking review follow-up. If updateServerSettings rejects, the optimistic
`singlePageListsOverride` is now cleared and the live client setting rolled back
(the effect only clears the override when the persisted value changes, which
doesn't happen on failure), so the UI and client reflect what's on disk rather
than the failed edit. Added an App test for the revert path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* Fix single-page Refresh not clearing list-changed indicator + UI polish (#1721)

Round-3 review fix (blocking): in single-page mode a `list_changed` lights the
list-changed indicator, but the single-page Refresh reloads page 1 of the paged
store and bypassed `useManaged*.refresh()`, so the indicator was never cleared.
Expose `clearListChanged` from useManagedTools/Prompts/Resources and call it in
the single-page branch of onRefreshTools/Prompts/Resources. Tests added.

UI polish (user feedback on the sidebar control):
- Rename the switch label "Single page" → "Paginated".
- Put "Load next page" on the same row as the switch, space-between (switch
  fully left, button fully right).
- Move the "N pages loaded" status to its own line below the row, centered.
- Hide the whole control once the list is known to be a single page
  (single-page mode, page 1 loaded, no nextCursor) — nothing to paginate.

pr-screenshots refreshed to the new layout.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* Rename the singlePageLists setting to paginatedLists for clarity (#1721)

`singlePageLists` read backwards from the "Paginated" switch label (it sounds
like paginated=false). Rename the server setting to `paginatedLists` with the
same value meaning — true = paginated/one-page-at-a-time, false = aggregate-all
— and follow through on the related identifiers for consistency:

- Setting field `paginatedLists` across ServerConfig / InspectorServerSettings,
  serverList convert/default, node + remote settings paths, and the form/modal
  callbacks (onPaginatedListsChange).
- `usePaginatedList` / `ListPaginationControls` prop `singlePage` → `paginated`,
  `onSinglePageChange` → `onPaginatedChange`.
- `ManagedListConfig.deferInSinglePage` → `deferWhenPaginated`.
- App state (paginatedListsOverride, onTogglePaginatedLists, …) and all
  tests/fixtures + mode-referring comments.

"single-page" wording that denotes the SDK's one-page `client.request` verb
(inspectorClient) is intentionally left as-is. Purely a rename — no behavior
change; unit + integration + cli + tui + coverage all green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* Format App.test.tsx (prettier) after the paginatedLists rename (#1721)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-19 01:06:31 -04:00
Cliff Hall
dc31c28102
 		fix(core): make "error" the canonical terminal status on a mid-session crash (#1490) (#1493) 
* fix(core): make "error" the canonical terminal status on a mid-session crash (#1490)

On a real mid-session transport crash many SDK transports fire BOTH
`onclose` and `onerror` in a transport-dependent order. The `onclose`
guard (`if status !== "disconnected"`) let a trailing `onclose`
downgrade a crash's `"error"` status back to `"disconnected"`, so the
persistent terminal status differed by ordering:

- onerror-then-onclose → "disconnected"
- onclose-then-onerror → "error"

`onclose` now treats `"error"` as terminal (only `"connected"`/
`"connecting"` → `"disconnected"`), so `"error"` is the canonical resting
status in both orderings. It still emits the `disconnect` event exactly
once so session-teardown consumers fire identically either way.

Because the state managers gated list/message/subscription/task teardown
on `getStatus() === "disconnected"`, holding the status at `"error"`
would have left them stale in the onerror-first ordering — swapping one
ordering-dependence for another. Introduce `isTerminalStatus()`
(disconnected || error) and key every teardown guard off it so they
clear identically regardless of crash ordering.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(core): single disconnect event when explicitly disconnecting from "error" (#1490 re-review)

Addresses the non-blocking observation from the PR review: explicitly
calling disconnect() from a held-"error" status could fire the
`disconnect` event twice. close() can trigger the transport's onclose
synchronously; onclose (holding status at "error") fired `disconnect`,
then disconnect()'s own guard saw status still != "disconnected" and
fired it again — a timing-dependence on whether the SDK closes sync.

Add a `disconnecting` ownership flag: onclose defers its status set +
`disconnect` event to disconnect() while it is set, so the
explicit-disconnect path emits exactly one `disconnect` regardless of
sync/async close. Crash-path onclose (flag unset) is unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(core): deterministically pin the sync-close disconnect double-fire (#1490 re-review)

The real-stdio test asserts the single-disconnect invariant but may not
exercise the sync-close branch (the one that double-fired) if the SDK
stdio transport closes asynchronously. Add a test that overrides the SDK
client's close() to invoke the transport's onclose synchronously,
forcing the exact timing the `disconnecting` flag guards. Verified it
asserts 2 (fails) without the flag and 1 (passes) with it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-06-15 10:15:29 -04:00
Cliff Hall
85a4b4c057
 		feat(servers): persist per-server settings to mcp.json (#1352) (#1353) 
* feat(servers): persist per-server settings to mcp.json (#1352)

- Add optional `settings` node alongside the transport config in each
  `~/.mcp-inspector/mcp.json` entry. Round-trips through
  `mcpConfigToServerEntries` / `serverEntriesToMcpConfig` and the
  `/api/servers` POST/PUT routes.
- Drop `MCPServerConfig.headers` from `SseServerConfig` /
  `StreamableHttpServerConfig` and remove the headers textarea from
  `ServerConfigModal`. Custom headers are now entered only in
  `ServerSettingsForm` and reach the wire via `settings.headers`.
- Wire `ServerSettingsModal` into `App.tsx`; saves dispatch through a new
  `useServers.updateServerSettings` mutator. `updateServer` carries the
  existing settings across config-only saves so they aren't dropped.
- Apply settings on the *first* outbound request after a server loads —
  no need to open the settings form first:
  - `settings.headers` → SSE / streamable-http wire headers.
  - `settings.metadata` → `InspectorClientOptions.defaultMetadata`,
    merged into every outgoing `_meta` (call-time keys win).
  - `settings.requestTimeout` → `InspectorClientOptions.timeout`.
  - `settings.connectionTimeout` → Promise.race wrapper around connect().
  - `settings.oauth*` → pre-seeded OAuth client credentials.
- `RemoteConnectRequest` carries `settings` so the remote backend can
  source transport headers from the same source as the local path.
- `oauthClientSecret` is persisted in `mcp.json` alongside stdio `env`
  values, both protected by the file's `0o600` permission.
- Update `specification/v2_servers_file.md` with the new shape and the
  removal of the legacy headers field.

Tests: round-trip in `serverList.test.ts`; persistence on POST/PUT in
`servers-route.test.ts`; first-connect header injection in
`transport.test.ts`; `defaultMetadata` merging in
`inspectorClient.test.ts`; `useServers.updateServerSettings` +
`updateServer` settings-preservation in `useServers.test.tsx`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(servers): rebuild InspectorClient on every connect so settings refresh

The previous onToggleConnection branch reused the existing InspectorClient
on a same-server reconnect, which froze settings at the moment the client
was first constructed. Editing headers, metadata, timeouts, or OAuth
credentials in the settings form was therefore invisible until the user
either switched servers or reloaded the page — surprising right after a
visible save.

Always call setupClientForServer on (re)connect so the latest
target.settings flow through to the transport's headers / requestInit, to
defaultMetadata, and to the OAuth manager.

Also adds remoteClientTransport tests that pin the wire-level contract:
settings round-trip through the /api/mcp/connect body when supplied, and
the field is omitted when no settings are configured.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(history): stop wiping pendingRequestEntries inside the connect event

State managers (ManagedToolsState, ManagedPromptsState, ManagedResourcesState,
ManagedResourceTemplatesState, ManagedRequestorTasksState) handle the
client's "connect" event by calling `void this.refresh()`. The synchronous
portion of refresh runs through SDK Protocol.request → transport.send →
MessageTrackingTransport.send → trackRequest, all before yielding to the
network — so each list request gets tracked into MessageLogState's
pendingRequestEntries inside the same connect event dispatch.

MessageLogState's own "connect" listener was registered after those state
managers (it's the last one constructed in App.tsx) and cleared
pendingRequestEntries on every connect. Listener order meant the clear ran
*after* the requests were tracked but before their responses arrived,
leaving the history with unmatched "response" entries marked PENDING.

The clear-on-disconnect (via statusChange → disconnected) already handles
session-boundary cleanup, so clear-on-connect was purely defensive and now
actively harmful. Drop the connect listener; keep the disconnect path.

Tests: updated the unit test to assert the new behavior (connect does not
clear); added an integration regression that wires a sibling connect
listener firing listTools and asserts the response folds into the request
entry instead of appearing as an orphan.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore: drop accidentally tracked debugging screenshot

pending-requests.png was a local debugging artifact attached to a
conversation, not source. Remove it from the tree (the working copy is
kept so the user still has it locally) and rely on the existing untracked
behavior going forward.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(servers): add e2e check that settings.headers reach upstream MCP

Stitches the full v2 pipeline together in one assertion:

  browser-style RemoteClientTransport
    → POST /api/mcp/connect (settings in body)
    → backend createTransportNode applies settings.headers to requestInit
    → SDK StreamableHTTPClientTransport
    → upstream MCP test server

Captures the upstream HTTP request via FetchRequestLogState and asserts
both `X-Tenant` and `X-Trace` are present on the outbound POST. Catches
any regression that breaks the wire between the form and the upstream
server in a single integration test, instead of the previous coverage
which exercised each hop in isolation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(ServerSettingsModal): invert ListToggle compact prop so icon matches state

ListToggle's `compact` prop represents the list's *current* compact state:
true means "currently compact" and renders the expand icon to advertise
the action (open up). The settings modal was passing
`compact={allExpanded}` — exactly inverted. When all sections were open
it showed the expand icon (suggesting "open more") and when collapsed it
showed the collapse icon, opposite of every other ListToggle caller in
the app.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* style(HistoryListPanel): match ServerListScreen Export button — rename + default variant

Renames "Export JSON" to "Export" and switches from the subtle ToolbarButton
to the same `Button variant="default"` used by ServerListControls so the
two Export affordances look identical across screens. Also moves the
button to the right of the ListToggle (mirroring the order on the server
list screen) and disables it when there are no entries to export.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* style(HistoryListPanel): move Clear button to top toolbar, match Export style

Moves the Clear affordance out of the "History (N)" section header and
up to the panel toolbar alongside Export, where it now uses the same
`Button variant="default"` styling. Order in the toolbar is now:
ListToggle · Clear · Export. Clear is disabled when there are no
unpinned entries to remove (pinned entries are intentionally preserved).

ToolbarButton helper is unused after this change and dropped.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore: ignore local debugging screenshots at repo root

Stops `git add -A` from re-staging the `*.png` files Claude Code drops
at the repo root from attached conversation screenshots. The pattern
only matches the top-level (`/*.png`) so legitimate image assets in
subdirectories (e.g. `clients/web/public/`) still get tracked when
explicitly added.

Also untracks the two screenshots that slipped through previously
(initialize-request.png, pending-requests.png).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* style(LogStreamPanel): match HistoryListPanel button styling

Clear / Export / Copy All on the Logging screen were rendered with the
subtle ToolbarButton helper while History's equivalents use
`Button variant="default"`. Switch all three to the default variant so
the toolbars look identical across screens and mirror the disabling
behavior — each button is now disabled when `entries.length === 0`.

ToolbarButton helper is unused after this change and dropped.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(servers): address PR review — debounce save, preserve-on-omit PUT, validate shape, lift connect timeout

Round-up of fixes from https://github.com/modelcontextprotocol/inspector/pull/1353#issuecomment-4526861959:

1. ServerSettingsModal saves were firing on every keystroke (one PUT +
   full mcp.json refresh per character). App.tsx now debounces by 300ms
   and flushes on modal close so a burst of edits coalesces into one PUT
   and the form value can't flicker back mid-typing.

2. `PUT /api/servers/:id` with an omitted `settings` field used to
   silently wipe the persisted settings node. The route now treats the
   three intents distinctly:
     - field omitted  → preserve existing on-disk settings
     - explicit null  → clear the settings node
     - settings object → validate + apply
   `useServers.updateServer` is simplified to drop the client-side
   carry-through hack, and the test that documented the broken behavior
   is flipped + augmented with explicit-clear and bad-shape cases.

3. `connectionTimeout` Promise.race is moved from App.tsx into
   `InspectorClient.connect()`, reading `this.serverSettings?.connectionTimeout`.
   TUI/CLI consumers now get the same behavior; on timeout the client
   internally disconnects so the next connect starts clean.

4. Custom Headers hint in `ServerSettingsForm` notes that `Authorization`
   is owned by the OAuth flow when OAuth is configured — avoids the
   sharp edge where the SDK's authProvider silently overwrites a
   user-set Authorization header.

5. New `validateSettings` on the server route rejects malformed shapes
   (`settings: []`, `settings.headers: "oops"`, non-numeric timeouts,
   etc.) with 400 instead of letting them persist and crash the UI.

7 (nit). Renamed `settings` → `savedSettings` inside `setupClientForServer`
   so the variable holding the persisted settings doesn't shadow the
   later locally-derived values.

Follow-ups opened separately:
  #1356 — Move oauthClientSecret out of mcp.json into the OS keychain (review #6).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(servers): address PR review pass 2 — connect-timeout test, unmount cleanup, tagged intent, lenient settings read

Pass-2 review at https://github.com/modelcontextprotocol/inspector/pull/1353#issuecomment-4527089705:

(a) New integration test pins the new client-layer connect-timeout
    contract: a hanging stub transport with `connectionTimeout: 50`
    rejects with the descriptive error and lands the client in "error"
    status. Documents what TUI/CLI consumers should expect from the
    lifted Promise.race in InspectorClient.connect().

(b) useEffect cleanup clears `pendingSettingsTimerRef` on unmount so a
    route change or HMR mid-debounce can't fire one stale PUT against
    an unmounted component.

(c) Refactor the PUT-route `settingsIntent` discriminator from
    `"preserve" | "clear" | { value }` to a tagged union with `kind`,
    and consume it with a `switch` for exhaustiveness — reads cleaner
    and makes adding a fourth intent painless.

(d) `normalizeMcpServers` now runs `validateSettings` on the on-disk
    settings node and silently strips it if invalid, mirroring the
    existing lenient-on-read pattern from `normalizeServerType`. Closes
    the read/write asymmetry where a hand-edited malformed `settings`
    could survive on disk and get carried through preserve-on-PUT.
    New route test pins the strip-on-GET + no-propagate-on-preserve-PUT
    behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(servers): pass-3 review — patch-style PUT, toast on close-flush fail

Pass-3 review at https://github.com/modelcontextprotocol/inspector/pull/1353#issuecomment-4529276897.

(b) `PUT /api/servers/:id` now treats both `config` and `settings` as
    optional patches:
      - field omitted → preserve the on-disk value
      - explicit body value → apply
      - settings: null → clear
    `updateServerSettings` stops reading `existing.config` from the
    in-memory `servers` snapshot, which closes the closure-capture
    hazard where a refresh between debounce-schedule and flush could
    silently revert a separate edit. The route is the single source of
    truth for transport config now. New tests cover settings-only PUT,
    rejected non-object config, and the empty-body no-op patch case.
    The previous "rejects a missing config" test is rewritten to
    document the new no-op-preserve semantics.

(c) Inline comment near the PUT rebuild loop documents that writing
    the full mcpServers map back is what gives the route its desirable
    self-healing property for malformed settings on *other* entries —
    deliberate side-effect, not an accident of normalization.

(d) `flushPendingSettings` surfaces failures via @mantine/notifications
    instead of swallowing them. The modal already closed by the time
    the flush PUT resolves, so a silent fail would leave the user
    thinking their last edits saved (especially painful for the OAuth
    client secret). The notifications wiring was already mounted in
    main.tsx.

Pass-3 (a) — settings form being effectively uncontrolled mid-edit — is
acknowledged in the reply: working as intended today; the proper fix is
internal modal state when file-watching (#1345) forces the issue.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(servers): pass-4 review — strip smuggled settings out of config

Pass-4 review at https://github.com/modelcontextprotocol/inspector/pull/1353#issuecomment-4529861349.

Main finding: `normalizeServerType` spreads unknown keys, so a body
that nested `settings` inside `config` would smuggle a settings field
onto the stored entry without ever passing through `validateSettings`.
Strip the settings key off the incoming config in `buildStoredEntry`
so `validateSettings` remains the single write path for the settings
node — the "one source of truth" invariant pass 2 set up holds again.

Two new tests pin the strip on both routes:
  - POST with `config.settings = { bogus }` lands an entry with no
    settings node on disk.
  - PUT with `config.settings = { bogus }` + `settings: null` clears
    the real settings *and* doesn't re-attach the bogus payload via
    the spread.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(servers): pass-5 review — absorb late connect rejection, tighten validateSettings, log smuggle warning

Pass-5 review at https://github.com/modelcontextprotocol/inspector/pull/1353#issuecomment-4529954490.

Main finding: when the connect-time `Promise.race` lost to the timeout,
the original `this.client.connect(this.transport)` promise was left
pending. After `disconnect()` tore the transport down, real transports
(SSE / streamable-http) would reject that promise later — with no
handler attached, producing a Node `unhandledRejection` (and a vitest
suite warning). Attach a no-op `.catch` right after creating the
connect promise so Node sees the late rejection as handled.

Nit (a): `validateSettings` was casting the raw object through to
`InspectorServerSettings`, which let unknown keys ride along onto disk.
Replaced the cast with an explicit pick-and-build over the known
fields. Unknown stowaways now silently drop on the way through. New
route test pins the behavior.

Nit (b): `buildStoredEntry` now logs a `warn` via the route's pino
logger when an incoming `config` carries a `settings` key. The strip
is still defensive (it isn't a documented contract) but the warning
lets a misconfigured client find out about the wasted field instead
of debugging a silent drop.

Nit (c) — the defensive `?? {}` fallback in `buildStoredEntry` —
acknowledged in the review reply; left in place as belt-and-braces.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(servers): pass-6 polish — scope absorber, thread id, drop empty OAuth, guard `in`

Pass-6 review at https://github.com/modelcontextprotocol/inspector/pull/1353#issuecomment-4529979135. Reviewer signed off as ready; landing the four marginal cleanups since they're all small.

(a) `connectPromise.catch(() => {})` moves inside the
    `if (connectTimeoutMs > 0)` branch. The await-only path propagates
    rejection cleanly through the outer try/catch — the absorber is
    only needed when the race leaves the original promise unattended.
    Code now reads as "absorber is tied to the race," which is the
    real invariant.

(b) Thread `id` (POST id, PUT newId) through `buildStoredEntry` and
    include it in the smuggle warning's pino bindings. Two clients
    smuggling simultaneously are distinguishable in the log instead
    of indistinguishable.

(c) `validateSettings` coerces empty-string OAuth fields to absent.
    The form emits `""` when the user clears an input, and an empty
    `oauthClientId` on disk would later be misread as "OAuth configured."
    New route test pins it.

(d) `buildStoredEntry` adds a null/object guard before `"settings" in
    configObj`. The route layer pre-checks this on POST and PUT, but
    the helper's `unknown` parameter is a weak contract; the guard
    makes it safe to call from anywhere.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(spec): reflect settings persistence + patch-PUT contract in v2_servers_file

The doc was authored when `mcp.json` only stored the server list. Bring it
forward to match what shipped in #1352 / PR #1353:

- Summary calls out that the file also stores per-server settings and
  links into the Per-server settings section.
- On-disk format note explains that `settings` is an optional Inspector-
  specific extension that other MCP tools ignore.
- New top paragraph on the Per-server settings section explains the
  basic-config / settings UI split rationale (simpler form for the
  common case, advanced settings tucked behind a second dialog).
- UI bullet is rewritten to match the patch-style PUT semantics that
  emerged across the review cycle: settings-only and config-only
  PUTs preserve their counterpart inside the write lock, so neither
  modal can silently clobber the other half. The earlier "must
  re-send settings on PUT" hack is gone.
- New bullets capture the save cadence (300ms debounce + flush-on-close
  + failure toast), the patch-PUT contract (omit-preserve / null-clear /
  rename), and the write-path / read-path validator gates.
- Secret storage bullet links to #1356 (OS-keychain follow-up).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(spec): show a third entry with the optional settings node

The On-disk format example demonstrated only basic stdio entries. Add a
streamable-http entry with a populated `settings` node so readers can
see the shape inline rather than having to scroll to the Per-server
settings section. Inline comment in the block cross-links there for the
full contract and notes that other MCP tools ignore the extension key.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-24 19:23:26 -04:00
Cliff Hall
e3e70470b5
 		feat(core): port v1.5 hook layer (#1243) (#1300) 
* feat(core): port v1.5 hook layer (useInspectorClient + useManaged*/usePaged* + log hooks)

Wholesale port of v1.5's React hook layer and the EventTarget-based state machinery the hooks subscribe to (#1243). Hooks decouple from the concrete InspectorClient via a new `InspectorClientProtocol` interface so this layer can land before the runtime class is ported.

Added under `core/`:
- `core/react/` — useInspectorClient + 5 useManaged* + 5 usePaged* + 3 log hooks
- `core/mcp/state/` — 5 managed*State + 5 paged*State + messageLogState + stderrLogState + fetchRequestLogState
- `core/mcp/inspectorClientProtocol.ts`, `inspectorClientEventTarget.ts`, `typedEventTarget.ts`, `samplingCreateMessage.ts`, `sessionStorage.ts`
- v2-only wrapper types in `core/mcp/types.ts`: InspectorUrlElicitRequest, InspectorPendingRequest, InspectorRequestHistoryItem

Tests live under `clients/web/src/test/core/` (28 new test files, 264 new tests). Test fake: `core/mcp/__tests__/fakeInspectorClient.ts`.

Build wiring:
- `vite.config.ts`: dedupe react/react-dom so core/ files resolve them via clients/web/node_modules.
- `tsconfig.app.json` / `tsconfig.test.json`: paths map react/react-dom/vitest to clients/web/node_modules so `tsc -b` resolves bare imports from core/ source files (which sit above the package's node_modules). Exclude core/__tests__ from app config; include in test config.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* review: address PR #1300 review findings

Bugs:
- fetchRequestLogState: add .catch() on sessionStorage.loadSession so a corrupt
  or unreadable session doesn't surface as an unhandled rejection (matches the
  saveSession swallow pattern).
- messageLogState: sync pendingRequestEntries when clearMessages() evicts a
  pending request entry, so a later response with the same id falls through
  to the append branch instead of mutating an unreachable map entry. Add tests
  for both predicate and no-predicate paths.

Cleanups:
- Extract mergeTaskIntoList helper to core/mcp/state/mergeTaskIntoList.ts so
  managed/paged RequestorTasks state classes share one definition.
- Add a one-line JSDoc note on each PagedTools/Prompts/Resources/Templates
  state explaining why they deliberately do not subscribe to *ListChanged
  (cursor invalidation mid-pagination).
- Document on useInspectorClient that appRendererClient is read lazily on
  every render and is not subscribed (changes once at connect time).

Carry-overs from v1.5 deliberately left for the follow-up InspectorClient
runtime port:
- Refresh/status-change race in Managed*State.refresh() (status check is only
  at entry; a disconnect during await can re-clear before refresh resumes).
- nextCursor exposure asymmetry across usePaged* (only usePagedRequestorTasks
  surfaces it via state field; others return it from loadPage() only).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-13 17:43:07 -04:00