| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
cliffhall
|
d57801d7b3 |
feat(core): report connection state on a request timeout (#2318)
A request timeout used to surface as a bare `Request timed out`. Every fact needed to explain it was already in-process: what the client was still waiting on, when it last heard back, and the state of the standalone GET notification stream. The SDK's timeout now rejects with all three, and the same snapshot is shown live in Connection Info and on the Network entry for the stream. - core/mcp/connectionDiagnostics.ts: the snapshot type, its one-line description, and `annotateRequestTimeout`, which rebuilds the SDK's per-request timeout with the state at that moment. - InspectorClient decorates the SDK Client's `request` (the one funnel every verb goes through) so the annotation covers the whole surface; tracks outstanding requests with their send time, the last response, and the most recent long-lived transport stream; exposes `getConnectionDiagnostics()` and `connectionDiagnosticsChange`. - fetchTracking watches a long-lived stream through a tee'd clone, counting SSE events and reporting the close, via a new `updateStream` callback wired through the node transport, the remote session (`fetch_stream_update`), and the browser transport. - FetchRequestLogState patches `entry.stream`; the Network entry's placeholder reports the event count and open/closed state. - Connection Info gains a Connection Activity section, ticking while open (`useTickingClock`). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APKSYHGxXBUpnR7ErFiLJn Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
9a8488681e |
feat: Skills extension phase 3 — CLI, TUI, directory reads, frontmatter check
Closes #2248. Completes SEP-2640 support across all three clients, and closes the one obligation #2234 deliberately left open. - `checkSkillFrontmatterMatch` compares a served SKILL.md's own YAML frontmatter against the entry the listing advertised, field by field. No digest can cover this: a digest is taken over the bytes the server served, so it proves the file was not altered in transit and says nothing about whether the listing described it honestly. Needs a real YAML parser; `yaml` was already a root dependency, so this adds no package — but it does newly put it on core/'s import graph, hence the three bundler `external` lists. - `resources/directory/read`: result schemas defined against the normative text, `InspectorClient.readResourceDirectory`, a Directory section on the Skills screen, and `--method resources/directory/read` in the CLI. The client refuses the call locally when the server did not declare `directoryRead`, which is the SEP's MUST NOT. A child the directory lists but the entry does not is marked `not listed` rather than merged into the manifest — the SEP calls a directory result a live observation and forbids treating it as extending the manifest. - CLI: `skills/list`, `skills/get` and `resources/directory/read`, plus `--verify` — one NDJSON report per skill, a summary on stderr, exit 7 on a violation. Reads are sequential; a conforming manifest may declare 512 entries. - TUI: a Skills pane, shown only when the server declares the extension. Each row carries its conformance verdict as a glyph as well as a colour, since the pane is read over ssh and through script(1). Enter verifies. - `verifySkills` re-throws `AuthRecoveryRequiredError` rather than recording it per file: it says the session's authorization expired, so absorbing it would report N identical read failures and swallow the error the TUI and the web commands key off to reauthorize. Two open questions settled, both in code comments where they will be found: `skills/get` carries no caching attributes because SEP-2640 leaves the question open in as many words, so requiring them would fail a conforming server; and there is no `PagedSkillsState` because every consumer of this list is a whole-catalog verdict — computed over page one of three, "this server's skills conform" is not merely partial but wrong. The fixture grows two skills, both for checks that were otherwise undemonstrable: `lying-listing` (listing and file disagree, digest still verifies) and `stale-manifest` (serves a file its manifest does not declare). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
3b4eb447c9 |
fix: walk past an empty-string cursor in the legacy task refresh (#2220)
`ManagedRequestorTasksState.refresh()` drives the legacy `tasks/list` walk itself, and both of its cursor tests were truthiness checks — so the fixed adapter was never reached. `while (cursor)` ended the walk on a `""` `nextCursor`, leaving the user-visible task list on page one, and `cursor ? [...this.tasks, ...page] : page` would have replaced the accumulated list rather than extended it had the walk continued. Both now test `=== undefined`. The other four paged/managed stores already did (`pagedToolsState`, `pagedPromptsState`, `pagedResourcesState`, `pagedResourceTemplatesState`, `pagedRequestorTasksState`, `managedSkillsState`); this was the last walk on the truthiness side. Covered by a new case in `managedRequestorTasksState.test.ts` that paginates `"" → "c2" → end` and asserts the cursor each call received, so a walk that terminated correctly but re-requested page one still fails. Reverting either guard on its own fails exactly that case. Reported by Copilot on #2277. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EN2UfrkQZvtrviMtqURRm4 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
6a08c77c92 |
fix: address Copilot review round 3 on #2251
- skills.ts: decide root containment on NORMALIZED URIs. A raw prefix check passed `skill://acme/billing/refunds/../other.md`, which starts with the advertised root but resolves outside it — a traversal the conformance report was reporting as clean. - skills.ts: `normalizeSkillUri` also rejects a relative string, so `demo/SKILL.md` is `malformed-uri` rather than a skill path, and an opaque-path URI (`skill:demo/..`), which the parser leaves un-normalized and on which containment cannot be decided. - skills.ts: `missing-description` is an error. SEP-2640 requires it, so an absent one must not read as "0 errors" in the conformance summary. - managedSkillsState: call `markResponseRejected` for a decode rejection, as every managed list does. An invalid `skills/list` result was showing in the Protocol tab as a clean success. - SkillsScreen: per-row attempt token. The manifest key cannot tell two verifications of the SAME row apart, so a double click (or a row button pressed during "Verify all") let an older read finish last and overwrite the newer verdict. - SkillsScreen: disable "Verify all" while a batch runs. The concurrency cap is per invocation, so repeated clicks stacked pools — 4, then 8, then 12. - SkillsScreen: include the finding index in each issue alert's key. Three identical URIs produce two `duplicate-resource` findings with the same code and URI, and React was free to drop the extras — hiding findings in exactly the malformed input this view exists to inspect. - test-servers/skills.ts: register `skills/list` and `skills/get` through the PUBLIC `setRequestHandler` with explicit param schemas. The private `_requestHandlers` map is now reached only to wrap `resources/read`, which has to chain onto the SDK's handler rather than replace it — the one thing the public API cannot express, and the comment now says so instead of citing the tasks fixture. - test-servers/skills.ts: raise `-32602` for an unknown `skills/get` URI. A plain Error mapped to a generic server failure, making the fixture non-conforming outside its three documented bad cases. - clients/web/README.md: the paragraph named four contracts while still saying the smoke "drives all three". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
bfeebd440d |
fix: address Copilot review round 2 on #2251
- SkillsScreen: key verdicts and React elements by manifest ROW INDEX,
not URI. The checker deliberately tolerates a duplicated URI so it can
report `duplicate-resource`; a URI key collided those rows into one
verdict, so verifying either updated both and "Verify all" raced two
different digest/size declarations into the same slot.
- SkillsScreen: hold the invalidation generation in React state keyed by
the manifest, not a ref bumped during render. `useValueChange` runs its
callback during render and requires setState-only purity — an abandoned
render left the ref incremented and silently discarded a live
verification.
- SkillsScreen: title a size disagreement "Size mismatch" and render its
reason. `verifySkillResource` catches it before hashing, so the alert
was showing "actual undefined" under "Digest mismatch".
- SkillsScreen: pass `contents` to ContentViewer so a base64 SKILL.md
renders. The text-block form substituted "" and painted a blank preview
for a file verification had just read correctly.
- skills.ts: add `malformed-size` for a size that is not a non-negative
safe integer, and exclude such values from the 16 MiB total. A negative
one could pull the sum back under the limit and hide a violation.
- managedSkillsState: make the overlap guard per-session instead of a
boolean. A disconnect during an in-flight walk left it set, so the
reconnect's own load no-oped and was never retried — permanently, if
the stale request never settled. A stale `finally` can no longer clear
the live session's guard either.
- skillsSchemas: require the `{ skill }` envelope for `skills/get`. The
accepted SEP settles it, and normalizing an inline entry would let a
non-conforming response past the one place that could report it.
- #2248 and the PR description: corrected — they said
`ReadResourceDirectoryResultSchema` was already declared, which round 1
removed. #2248 now owns defining it, and records the frontmatter
cross-check gap too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
da8cc166ce |
fix: address Copilot review round 1 on #2251
- skills.ts: report a manifest that omits the skill's own SKILL.md (an empty list included), duplicate URIs, entries outside the skill root, and a missing size. `Conforms` was reachable for manifests that break invariants SEP-2640 states. - skills.ts: cross-check the declared byte length before hashing. A size that disagrees fails verification on its own — the digest is taken over the bytes the server served, so agreeing with it says nothing about whether the manifest describes them. - skills.ts: copy the view instead of slicing its backing store in `sha256Digest`. `SharedArrayBuffer.prototype.slice()` returns another SharedArrayBuffer, which `crypto.subtle.digest` rejects — the cast claimed to handle the exact input that would have thrown. No cast now. - skills.ts: state the one obligation NOT checked here — that an entry's frontmatter matches the fetched SKILL.md's. The digest cannot cover it, and closing it needs a YAML parser, so it is tracked on #2248. - managedSkillsState: cap the walk at LIST_MAX_PAGES. The repeated-cursor guard only catches a server stuck on one cursor; endlessly unique ones walked forever. Raises rather than truncating, like the salvage walk. - managedSkillsState: gate every write on a session generation, so a walk that resolves after a disconnect or destroy cannot repopulate a cleared store or deliver the previous session's skills into the next. - inspectorClient: send a cursor when it is `!== undefined`, not when it is truthy. An opaque cursor may be `""`, and dropping it re-requested page one — which the store then reported as a repeated-cursor failure. - SkillsScreen: key verdict invalidation on the manifest (URI + digests + sizes), not the URI alone. A Refresh that changed the manifest left a green badge attached to a digest nothing had checked. - SkillsScreen: epoch-guard every read continuation, so a fetch that resolves after the selection moved on cannot write into the new one. - SkillsScreen: bound "Verify all" to 4 concurrent reads. A conforming manifest may hold 512 files. - skillsSchemas: drop the guessed `resources/directory/read` result schema. Nothing calls it, so an unverified shape could sit wrong indefinitely; phase 3 adds it against the normative text. - skills-http.json: declare the extension bare. It advertised `directoryRead: true` with no handler, so Connection Info reported "Supported" for a method that answers -32601. - types.ts: restore the Tasks doc comment the Skills interface displaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
63623b754b |
feat: support the Skills extension (SEP-2640) with digest verification
Adds detection, enumeration, and conformance checking for the Skills extension (`io.modelcontextprotocol/skills`) — phases 1 and 2 of #2234, which is everything its Acceptance list names. Skills is a *server*-declared extension, read off the connecting server's `capabilities.extensions`, so it is deliberately absent from `ADVERTISABLE_EXTENSIONS` — that registry is what the Inspector advertises and the user toggles, and an entry there would be a meaningless toggle. `skills/list` and `skills/get` are consumer-owned extension methods that neither era codec defines, so the SDK's era gate skips them and they go out as ordinary `client.request` calls with explicit result schemas. The raw-wire channel modern `tasks/*` needs is not used, and the Skills tab is not era-gated: a legacy-era server that declares the extension is serving it. The checks are the point, not the list view. `core/mcp/skills.ts` reports each obligation SEP-2640 states — the name/path invariant, the digest format, the 512-entry and 16 MiB limits, and `resources: "dynamic"`, which means integrity cannot be verified at all. Digest verification hashes the fetched bytes with WebCrypto and returns a mismatch with both digests attached rather than throwing, because showing a mismatch loudly is the whole value proposition. Files are fetched on demand: SEP-2640 is explicit that a `resources/read` of a `SKILL.md` is not a load and confers no standing, so none of the SEP's host machinery is implemented. The `skills-http` fixture serves four skills over two pages, three of them deliberately non-conforming — without those the verification code is untestable. Phase 3 (CLI, TUI, `resources/directory/read`, paged mode) is #2248. Closes #2234 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
b33532c024 |
feat: allow complex JSON values in request _meta
Closes #1910 `_meta` was typed `Record<string, string>` end to end in the Inspector — on disk as a `{ key, value }[]` pair array, in the Server Settings form as key/value rows, and in every request verb's per-call metadata parameter. Nothing in the MCP spec restricts a `_meta` value to a string, and the SDK models the field as a `z.looseObject`, so the restriction was ours alone. Widen the whole path to `RequestMetadata` (`Record<string, JsonValue>`): - Per-server default metadata is a JSON **object** on disk and in memory. The pre-#1910 pair array is still read (`normalizeStoredMetadata`) so an existing `mcp.json` keeps working; it is never written back, so a save migrates the entry. - The Server Settings → Request Metadata section is a JSON editor (`JsonObjectInput`) instead of key/value rows — rows cannot express an object value. It keeps the typed text as the display source and emits only parsed JSON, the same draft split `SchemaJsonField` uses, so the box stays typeable; invalid text is flagged inline and not applied. - The CLI stops flattening `--metadata` / `--tool-metadata` through `metaValueToString`. Those values were already JSON-parsed, so `trace={"id":"abc"}` used to reach the wire as the *string* `{"id":"abc"}` and now reaches it as an object. - `servers/show` redaction replaces a sensitive key's whole value rather than a string, so a structured value cannot leak through its members. - `progressTokenOf` narrows the reserved `progressToken` key back to `string | number` before it reaches the SDK; any other JSON still travels inside `_meta` but wires up no progress callback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
de60983ccc |
fix: surface list-fetch failures in paginated mode (#1998)
#1954 wired the list-load error off the aggregate managed stores only. With `paginatedLists` on, those stores deliberately skip their all-page walk, so their error is permanently null — while the paged stores that actually drive the sidebar had no error state at all and `loadPage` had a try/finally with no catch. A failing list showed an empty panel with no alert and no Retry, and the connect-time `void loadPage(undefined)` left an unhandled rejection. - Give PagedTools/Prompts/ResourcesState observable error state: an `errorChange` event plus `getError()`, set in a new catch in `loadPage`, cleared on the next success and on disconnect. The rejection is still re-thrown, matching ManagedListState — callers' auth-recovery wrappers key off it to detect a 401. - Catch the connect-time load's rejection (`.catch(() => {})`) now that the failure is recorded as state rather than lost. - Select the error by mode in `usePaginatedList`, the same way `items` already is, and read it in App.tsx from the pagination model. - Rename `useManagedListError` to `useListError`: both store families expose an identical `errorChange`, so the hook declares that one-event contract itself instead of importing ManagedListEventMap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
fe5b622fd1 |
fix: salvage a list result's valid entries when one is malformed (#1909)
The SDK validates a list result as a whole, so a single non-conforming entry
rejected the entire response and the panel showed "Couldn't load resources"
with zero items — the exact opposite of what a debugging tool should do with a
server that is 90% right. Reported against a PHP server whose empty
`annotations` object reaches the wire as `[]` rather than `{}`.
`listAll*` now falls back to a lenient re-walk when — and only when — the SDK
rejects a result it received (`InvalidResult` / `UnsupportedResultType`), keeps
the entries that validate, and reports the rest. A conforming server takes the
strict, cache-aware path exactly as before.
Salvaging is not accepting: the dropped entries are named with their reason in
a warning above the list, and the Protocol entry is still marked rejected, so
the wire truth survives. When every entry validates, the original error is
rethrown rather than swallowed — the rejection was about something the
per-item pass cannot explain.
`isClientDecodeRejection` moves to the new module and is now shared with
managedListState, which had the only copy.
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>
|
||
|
cliffhall
|
fad8480376 |
fix: only attribute a decode rejection to a Protocol entry
markResponseRejected was called for ANY refresh failure. The id it recovers is "the last response received for this method", which is the failing exchange only when a response actually arrived and was refused while decoding. A transport drop, a timeout, or an abort produces no response frame at all — the last-answered id then still points at an EARLIER successful call, so marking it stamped "Rejected by the Inspector" onto an exchange that worked. That is the same class of lie this issue exists to remove. A server-sent JSON-RPC error is excluded for a different reason: the id would be right, but the failure is the server's and its entry already renders as an error from the error frame. Gated on SdkErrorCode.InvalidResult / UnsupportedResultType — decisions the client made about a frame in hand. The failure is still recorded as list state either way; only the Protocol attribution is gated. Caught in Copilot's review of #1953. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
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 |
||
|
Cliff Hall
|
373f346e5f |
chore: close quality-gate coverage gaps (#1689) (#1766)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
Closes #1689. Typecheck cli/tui src, add a core/ format+lint gate, and gate core/json + core/client coverage. See PR #1766 for the full review history. |
||
|
Cliff Hall
|
2259b1a3e1 |
Tasks: io.modelcontextprotocol/tasks extension on modern servers (era fork) (#1731)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
* Tasks: modern io.modelcontextprotocol/tasks extension (era fork core) Core + test-server implementation of the SEP-2663 tasks extension for modern (2026-07-28) connections, forked from the legacy 2025-11-25 tasks path. SDK v2 removed all tasks support AND era-gates the tasks/* spec methods out of the modern era on both the client (outbound) and server (inbound) sides, and its codec rejects a resultType:"task" result outright. So the Inspector drives the extension itself: task creation rides tools/call (a modern method) with the resultType:"task" frame rewritten at the transport to a CallToolResult carrying the handle in _meta; tasks/get/update/cancel go over a raw-wire request channel that bypasses the era gate (full modern envelope, string ids, consumed by the transport). input_required tasks reuse the existing pending-request modal via fulfilInputRequests, answered with tasks/update. No tasks/list, no tasks/result. Tab + run-as-task gate on the negotiated extension; the managed task store's refresh re-polls known handles on modern (no server list). Test server serves the extension via an Express interceptor (SDK can't) + a shared poll-driven runtime. Full integration coverage of both eras. Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: tests + coverage (raw-wire, schemas, era integration) Adds unit coverage for modernTaskSchemas and the raw-wire request channel (transport-null guard, send failure, timeout, error response, teardown), plus end-to-end era integration tests (create/poll/input-required/update/cancel, synchronous passthrough, unknown-id error, modern store re-poll). Fixes the App and constructor-capabilities tests for the always-on tasks extension. Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: docs + share test servers per era to de-flake coverage Document the tasks era-fork test configs in README and the new core/test-server files in AGENTS.md. Share one modern + one legacy test server across the era integration block (started lazily, stopped in afterAll) — starting a fresh Express server per test made connection negotiation flaky under the heavy concurrent coverage run. Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Restore pr-screenshots deleted inadvertently The logging/subscriptions PR screenshots + README were staged-deleted in the working tree before this branch; restore them from v2/main so this PR doesn't remove other PRs' artifacts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: offer Run-as-task for any tool + PR screenshots On modern (SEP-2663) task creation is server-directed — the client declares the extension once and any tool may return a task — so the Tools screen now offers 'Run as task' for every tool on a modern connection (threaded via a modernTasks prop through InspectorView → ToolsScreen → ToolDetailPanel), rather than gating on the legacy per-tool taskSupport. Verified end-to-end in the web UI (remote proxy): create → poll tasks/get → input_required elicitation → tasks/update → inline result, plus the unchanged legacy tasks/list/result flow. Adds proof screenshots. Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: address two follow-ups (task-input note + ordinary-path auto-poll) 1. The pending-request note now distinguishes a modern task round from an MRTR round: a new 'task-input-required' PendingRequestOrigin threads through fulfilInputRequests → the pending-request modal → MrtrOriginNote, which shows 'submitted via a tasks/update request (SEP-2663), not a retry' instead of the MRTR retry copy. 2. The ordinary (non-run-as-task) modern callTool path now auto-polls an unsolicited task handle: attemptToolCall detects the rewritten handle in the result _meta and drives it to termination (get → input_required/update → inline result), dispatching task events so the Tasks tab + toasts track it and the tool result resolves to the task's final result. The poll loop is shared with the streaming path via extracted helpers (submitModernTaskInput / modernTaskTerminalOutcome / modernTaskPollInterval). Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: address review (negotiated envelope version, input-round cap, clarifying comment) + fresh smoke screenshots Review of #1731: 1. withModernTaskEnvelope now stamps the NEGOTIATED protocol version (getProtocolVersion() ?? MODERN_PROTOCOL_VERSION) so the envelope matches the MCP-Protocol-Version header instead of a hardcoded constant. 2. A never-completing input_required task is now bounded by MRTR_MAX_ROUNDS in the shared submitModernTaskInput (threaded through both poll loops), so a non-conformant server can't re-prompt the user forever. New modern_loop_task test-server tool + integration test cover the cap. 3. ToolDetailPanel: comment clarifying that per-tool taskSupport:"forbidden" is deliberately ignored on modern (task creation is server-directed under SEP-2663). (The abort-during-poll observation is addressed in the PR reply: the cancel path uses tasks/cancel and disconnect rejects the in-flight raw request, so the poll stops; matches legacy.) Fresh web-UI smoke screenshots (modern connected, task completed, input_required with the corrected tasks/update note, input completed). Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: fix uncancelable input-required loop + sticky cancel Two bug fixes reported during manual testing of the tasks era-fork (#1631): 1. A modern task paused at `input_required` (e.g. `modern_loop_task`, which never advances) could not be cancelled: the poll loop blocked on the pending-request modal, so `cancelRequestorTask` sent `tasks/cancel` but the modal kept re-prompting. Now each modern poll loop registers a per-task AbortController (`registerTaskInputAbort`), `cancelRequestorTask` aborts it to reject the pending elicitation and unblock the poll, and the embedded input requests are tagged with their owning task id (`RELATED_TASK_META_KEY`) so the pending-request UI's Cancel action cancels the TASK rather than sending a "cancel" answer the server would re-prompt on. 2. On the legacy server, cancelling a task briefly showed CANCELLED then flipped to COMPLETED when the in-flight tool call resolved. Cancellation is a deliberate terminal decision, so `ManagedRequestorTasksState` now remembers cancelled task ids and ignores any later non-cancelled status update. Also: move the compact task id onto its own line in the embedded monitoring sidebar so the status badge / Cancel Task control don't wrap; DRY the duplicated task poll-interval logic into a shared `taskPollInterval` helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Refresh tasks PR screenshots + capture both bug-fix scenarios Recapture the five tasks flow screenshots against the current build and add two demonstrating the post-review fixes: modern_loop_task cancelled from the pending-request modal (uncancelable-loop fix) and a legacy task staying CANCELLED after the in-flight call resolves (sticky-cancel fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Address re-review: sticky cancel on legacy refresh + App cancel-routing test Re-review of the cancelable-loop / sticky-cancel fix raised two non-blocking hardening items: 1. The legacy tasks/list refresh() rebuilt the list from the server, honoring only dismissedTaskIds — so a cooperative-cancel server that re-lists a user-cancelled task as completed could un-stick the cancel on a manual Refresh. The refresh path now pins a cancelledTaskIds task to "cancelled" (keeping it visible-as-cancelled, matching the live-merge handlers) rather than dropping it. Covered by a new state test. 2. Added an App.test.tsx case locking in the user-facing half of the fix: Cancel on a task-input-required elicitation calls cancelRequestorTask (not respond). 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> |
||
|
Cliff Hall
|
320daf2f67 |
Resources: subscriptions/listen model on modern servers (era fork) (#1630) (#1730)
Fork resource subscriptions by protocol era: legacy keeps resources/subscribe; modern (2026-07-28) models subscriptions as a filter over one long-lived subscriptions/listen stream with reconnect-by-re-listen (capped backoff), an optimistic Connecting→Listening header badge, and a fix so the browser proxy no longer blocks listen() ~60s. Closes #1630. |
||
|
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
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
* 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> |
||
|
cliffhall
|
f9f79fff54 |
Address @claude review: format core/, typed output-validation error, document progress heuristic
- Run Prettier on the 60 core/ files this PR touched — the deterministic import rewrite had collapsed multi-line imports and left throws unwrapped; core/ is in no format gate so it slipped past CI. Now Prettier-clean. - Default-path output-schema violation now throws ProtocolError(ProtocolErrorCode.InvalidParams, …) instead of a bare Error, matching the prior v1 contract (SDK threw a typed McpError with a code) so downstream `instanceof`/`.code` branching keeps working. - Document the "assumes serial construction" invariant on pollTaskToolCall's progress-subscription revival (the `_progressHandlers` new-key heuristic). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
cliffhall
|
19a6df7ad3 |
SDK v2 migration (behavior-neutral, versionNegotiation: 'legacy') (#1624)
Migrate all three clients + core + test-servers from
@modelcontextprotocol/sdk@1.29 to SDK v2 (2.0.0-beta.4), keeping behavior
identical against legacy (2025-11-25) servers. Foundation for the SDK V2 +
New Spec stack.
Packages
- Split @modelcontextprotocol/sdk -> @modelcontextprotocol/client (Client,
transports, OAuth, error taxonomy, protocol types) + core (Zod schemas);
stdio -> client/stdio, Ajv -> client/validators/ajv. Test-servers ->
@modelcontextprotocol/server (+ server-legacy/sse). Deterministic import
rewrite (schemas -> core, everything else -> client); the codemod misroutes
client code to /server so it was not used for path routing.
- new Client(..., { versionNegotiation: { mode: 'legacy' } }) — nothing
2026-era goes on the wire.
Runtime (core/mcp/inspectorClient.ts)
- Error taxonomy: McpError -> ProtocolError, ErrorCode -> ProtocolErrorCode;
RequestTimeout -> SdkError/SdkErrorCode.
- Handlers take method strings; task methods (tasks/*, notifications/tasks/*)
use the 3-arg custom form (excluded from v2's spec-method set).
- Legacy tasks kept working (per project requirement): requestor tasks re-
implemented over raw client.request + explicit deprecated schemas;
experimental.tasks.callToolStream replaced by a local pollTaskToolCall
async generator (with _progressHandlers revival so task-execution progress
still reaches the caller). Receiver (server-initiated) tasks: the v2 Client
validates a sampling/elicit handler result and rejects the legacy { task }
response with -32602 — installReceiverTaskResponseBypass swaps the wrapped
entry in the private _requestHandlers map for the task-augmented branch so
{ task } reaches the wire (fixes server-initiated sampling-via-task).
- Pagination: list verbs use raw client.request({method}, List*ResultSchema)
for single-page behavior (v2's client.listX() auto-aggregate all pages).
Web / auth / apps
- Rename local JsonSchemaType -> InspectorFormSchema + add toFormSchema()
structural narrowing at the SDK-schema -> form boundary.
- ext-apps still peers on SDK v1: one compat boundary with documented casts
(remove when ext-apps#702 lands a v2 peer).
- OAuth provider saveClientInformation reconciled with v2's
OAuthClientInformationContext; InvalidClientMetadataError re-homed as a
local class (server-legacy/auth is not browser-safe).
- vitest.shared.mts dedupes @modelcontextprotocol/client+core to one copy so
vi.mock intercepts the SUT's import.
Accepted v2 behavior changes (per #1624 / spec §9.2): unknown-tool call
rejects with -32602; cancellation is stream-abort (no guaranteed
notifications/cancelled frame); output-schema strictness restored via the
Inspector's own Ajv check on the default path.
npm run ci green: validate (web 3087 / cli 212 / tui 274 / launcher 5),
coverage (>=90 per-file, all clients), smoke (launcher/cli/web), storybook
(410).
Closes #1624
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
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> |
||
Bob Dickinson
|
2488ca5e38 |
feat: port CLI, TUI, and launcher from v1.5 into v2 (#1452)
Ports the CLI, TUI, and launcher from v1.5 into v2 behind a single `mcp-inspector` entrypoint (launcher → web/cli/tui). Includes the `mcp-inspector --web --dev` launcher fix (Vite alias resolution + prod-server double-start), the root `postinstall` install cascade with simplified CI, and spec/AGENTS docs (no-workspaces rationale, Development setup). Closes #1246. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
53dc4549be |
fix(core): trace dropped fetch body update when entry rotates out (#1390) (#1480)
* fix(core): trace dropped fetch body update when entry rotates out (#1390) FetchRequestLogState.onFetchRequestBodyUpdate silently dropped a deferred response-body update when the matching entry had already been evicted by log rotation (>= maxFetchRequests newer requests appended between the append and the fire-and-forget body read). The user saw an empty Response Body with no way to distinguish "body never came back" from "body dropped after eviction". Plumb an optional pino logger into FetchRequestLogState (defaulting to the silent logger) and emit a logger.debug at the early-return site so the drop is traceable. The web client wires the existing remote logger from createWebEnvironment into the state manager. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(core): emit dropped-body trace at warn level so it is observable (#1390) Review feedback: the web remote logger (createRemoteLogger) defaults to level 'info' with no override at the environmentFactory call site, so a logger.debug at the drop site is filtered out by both the pino level gate and transmit.level — the diagnostic never reaches /api/log and the drop stays invisible in the shipped web app, defeating the issue's goal. Promote the trace to logger.warn (>= info, so it transmits by default). A response body lost to log rotation is unexpected data loss, not routine diagnostic noise, so warn is the appropriate severity. Update the comment to explain the level choice and switch the test stub from debug to warn, extracting a small typed makeWarnLogger helper per review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): configurable Network Log Size per server + body-dropped toast (#1390) Turn the otherwise-invisible "response body dropped after rotation" case into an actionable signal: 1. Per-server `maxFetchRequests` setting (default 1000, 0 = unlimited), editable as "Network Log Size" in the first (Options) accordion section of the Server Settings form. Plumbed through the full settings pipeline: InspectorServerSettings + StoredMCPServer types, the serverList disk converters (read defaults to 1000, write omits the default to keep diffs minimal — 0 is a meaningful non-default), and the node server's GET-sanitize + write-side validateSettings (non-negative number). Applied live to the active connection on settings-modal close via a new FetchRequestLogState.setMaxFetchRequests (shrinking trims immediately), and at connect time for the next session. 2. FetchRequestLogState now emits a `fetchRequestBodyDropped` event, but only for genuine rotation drops (log at capacity with a finite cap) — not for benign post-clear stragglers or unlimited logs, which previously also hit the warn path. App shows a deduped, persistent toast (stable per-server id, no auto-close) explaining what happened and the likely causes, with a link that opens this server's settings on the Options section (modal remounts via key) so the user can raise the limit if it's just a high-traffic server. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web): don't treat a cleared Network Log Size field as unlimited (#1390) Review feedback: clearing the NumberInput emits "", and `parseInt("") || 0` coerced that to 0 (unlimited). Since the value applies live on modal close, a user who cleared the field to retype and then closed the modal would silently switch the log to unlimited. Fall back to the current value on an empty/NaN parse and reserve 0 for an explicit numeric entry. Adds a cleared-input test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
721dae08c6 |
feat: apply auto-refresh setting live + diff-aware list-changed indicator (#1444) (#1445)
* feat: apply auto-refresh setting live + diff-aware list-changed indicator (#1444) Two follow-ups to #1402. 1. Live setting application — InspectorClient gains setServerSettings() (on the protocol + fake); App pushes edited settings onto the live client when the Server Settings modal closes for the connected server, so toggling auto-refresh-on-list-changed takes effect without a reconnect (the managed state reads getServerSettings() at notification time). 2. Diff-aware indicator — in flag-only mode, on */list_changed the managed Tools/Prompts/Resources state now fetches the new list and compares it to the current one, lighting the indicator only when it actually differs. The displayed list stays put until the user pulls via Refresh (pull-on-demand preserved). Servers like everything re-send identical lists on list_changed; this suppresses the false-positive indicator. Refactors each manager's refresh into fetch + apply helpers shared with the peek path. Trade-off: item 2 reintroduces a fetch on */list_changed (to compare), but unlike the pre-#1402 behavior it never replaces the displayed list under the user — only the indicator reflects a real change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(core): make the diff-aware peek clear the indicator on revert (#1444 review) peekForChange now sets the list-changed flag to whether the server list differs from the displayed one (setListChanged(differs)) instead of only ever setting it true. So a later notification that reverts the server back to the displayed list clears the indicator — nothing left to pull. Adds a clear-on-revert regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(core): consolidate managed-list state managers onto a shared base (#1444) The four per-primitive list managers (tools, prompts, resources, resource templates) were near-identical copies of the same fetch/apply/peek/connect/ disconnect/list_changed machinery, a standing drift risk. Extract a generic ManagedListState<T, M> base parameterized by a config (change event, list_changed notification, capability key, page fetcher, and whether the list drives an indicator). Each manager is now a ~40-line subclass: its config plus a typed getter alias. Public contracts are unchanged — same per-manager *Change / listChangedChange events and get*/refresh/getListChanged/clearListChanged methods — so the hooks and all existing tests pass untouched. Templates opt out of the indicator (supportsIndicator: false): a list_changed auto-refreshes only when the server opts in, else does nothing. Net -245 lines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(core): coalesce bursts of list_changed peeks (#1444 review) A rapid burst of */list_changed notifications fired one concurrent paginated fetch per notification. Guard peekForChange with an in-flight flag: while a peek is fetching, a new notification queues a single re-run instead of launching a concurrent fetch, and the running peek loops once more on completion to capture the latest server state. Collapses a burst into sequential peeks. Adds a coalescing test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(core): debounce list_changed bursts into a single fetch (#1444) The in-flight guard only coalesced fetches that actually overlapped — when a server (e.g. everything) emits a burst of */list_changed spaced just far enough apart that each list call returns before the next notification, we still fired one list call per notification. Add a real debounce in the ManagedListState base: a notification (re)starts a timer and the refresh/peek runs once the burst settles, collapsing N notifications into one fetch. The in-flight coalescing guard stays as a second line of defense for a post-debounce notification landing during a slow paginated fetch. The delay is injectable (DEFAULT_LIST_CHANGED_DEBOUNCE_MS = 250; subclasses take an optional constructor arg) so tests run with 0 for fast, determinist ic behavior. Timer is cleared on disconnect, unsubscribe, and destroy. Adds debounce + in-flight regression tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(core): extend the in-flight guard to the auto-refresh path (#1444 review) The overlap guard was only on the peek path; a burst-during-a-slow auto- refresh could let a slower older fetch clobber a newer list (applyItems is last-write-wins). Lift the guard from peekForChange up to runListChanged so it wraps both the auto-refresh and peek actions: while one is fetching, a new post-debounce notification queues a single re-run. Adds an auto-refresh coalescing test alongside the peek one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(core): light the indicator without fetching when auto-refresh is off (#1444) Per maintainer feedback: with auto-refresh off, a list_changed should not issue any list call. The diff-aware peek (fetch-to-compare) meant one tools/list went out per settled burst even with auto-refresh off, which the maintainer doesn't want. Revert flag-only mode to lighting the indicator blindly on a (debounced) list_changed — no network — and the user pulls the new list via Refresh. Trade-off (accepted): the indicator lights even when the server re-sent an identical list (the everything-server case), since we no longer fetch to compare. Keeps the debounce (one indicator light per burst), the auto-refresh path + its in-flight guard, the live setServerSettings, and the ManagedListState consolidation. Removes peekForChange and its tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(core): scrub stale peek docstrings after the blind-indicator revert (#1445 review) The diff-aware revert removed peekForChange's body but left its docstring, which orphaned onto destroy() and described fetch-to-compare/clear-on-revert behavior that no longer exists. Replace it with a one-line destroy() doc, and update clearListChanged's doc (peek -> blind light). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(core): finish scrubbing stale peek references after the revert (#1445 review) Round-7's scrub missed four more peek/fetch-to-compare mentions in managedListState.ts (supportsIndicator, fetchAll, the debounce-default and onListChanged comments) plus six 'async peek' comments in the state tests. Update them all to the blind-indicator semantics. Doc/comment-only; no behavior change. grep -i peek over core/mcp/state now returns nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
b0a024269e |
feat(web): list-changed handling — indicator, pull-on-demand default, and per-server auto-refresh opt-in (#1402) (#1443)
* feat(web): wire the listChanged indicator across all screens (#1402) The per-screen list-changed indicator was hardcoded false in InspectorView for Tools, Apps, Prompts, and Resources, so it could never light up. Source it from the managed-state layer instead. Each ManagedXState now tracks a listChanged flag: set true when its */list_changed notification arrives (the existing auto-refresh leaves it set), exposed via getListChanged()/a listChangedChange event, and cleared by clearListChanged() or on disconnect. The useManagedX hooks surface the flag and clear it on a user-initiated refresh (the indicator's Refresh button = acknowledge). App threads the three flags to InspectorView, which maps them to the screens — Apps shares the tools flag since apps are a filtered view of tools. Tests cover set/clear/disconnect at the state-manager layer, the hook subscription + refresh-clears behavior, and the InspectorView routing (including Apps sharing the tools flag and no cross-wiring between screens). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(core): close clear-after-await race in managed-list refresh (#1402 review) The useManagedX refresh() callbacks cleared the listChanged flag AFTER awaiting the fetch. If a */list_changed arrived mid-fetch, the state re-set the flag (and auto-refreshed), then the trailing clear wiped that genuinely-new signal. Clear BEFORE the await instead: the user's refresh acknowledges only the change in hand, and a mid-fetch change re-sets the flag and survives. Adds a race regression test on the tools hook. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web): drop duplicate Refresh on the Apps screen (#1402) AppControls uniquely had a standalone toolbar Refresh button AND the ListChangedIndicator on a separate row, so once the indicator could actually appear (this PR), the Apps screen showed two Refresh buttons. Match the other three controls (Tools/Prompts/Resources): the indicator is the single header-right refresh affordance. Updates AppControls and AppsScreen tests to assert exactly one Refresh (and none when the list hasn't changed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(core): stop auto-invoking */list on list_changed; user pulls via Refresh (#1402) With auto-refresh on, the list was already current by the time the list-changed indicator appeared, making its Refresh button moot. Now the managed Tools/Prompts/Resources state managers only flag the change on */list_changed (they no longer auto-refresh); the list stays as-is until the user clicks Refresh, which fetches the new list and clears the indicator. The connect-time initial load is unchanged. Resource templates (no own indicator, part of the Resources screen) also stop auto-refreshing; onRefreshResources now refreshes both resources and templates, since a single resources/list_changed covers both. Tasks are left auto-refreshing — they have no list-changed indicator and need live status updates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(core): correct ManagedResourceTemplatesState header comments (#1402 review) The class no longer subscribes to resourceTemplatesListChanged; update the stale file/class docstrings that still claimed it syncs on that notification to reflect connect + statusChange only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): add per-server 'Auto Refresh on List Changed Notifications' option (#1402) New Options section in ServerSettingsForm with a single checkbox. When checked, the managed Tools/Prompts/Resources (and resource templates) state auto-refreshes on */list_changed notifications; when unchecked (default) the notification only lights the list-changed indicator and the user pulls via Refresh. Plumbing: new optional autoRefreshOnListChanged on InspectorServerSettings and StoredMCPServer, round-tripped through serverList (lifted with a false default, persisted only when true, added to the exhaustive field-key map); surfaced from InspectorClient via a new getServerSettings() on the protocol; the state managers read it live at notification time to decide auto-refresh vs flag-only. Resource templates re-subscribe to their list_changed but only auto-refresh when the option is on (they have no indicator of their own). Tests cover the round-trip (true persists, false omitted), the form section (render/checked/toggle), and each state manager's on/off branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(server): persist autoRefreshOnListChanged through validateSettings (#1402) The PUT /api/servers write path runs settings through validateSettings, which rebuilds the value from explicitly-named fields and silently drops unknown keys — so autoRefreshOnListChanged never reached disk (the serverList round-trip test passed because it calls inspectorSettingsToStoredFields directly, bypassing this path). Validate it as an optional boolean and include it in the built value; false is omitted on disk by inspectorSettingsToStoredFields as before. Adds integration tests over the real PUT path: true persists, false is omitted, and a non-boolean is rejected (400). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): default the Server Settings modal to the Options section (#1402) Open 'Options' instead of 'Custom Headers' when the Server Settings modal first opens, so the new Auto Refresh toggle is the first thing surfaced. Updates the modal default-expanded section, the modal test's initial-state assertions, the three header tests (now expand 'Custom Headers' first since it's no longer open by default), and the story defaults. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: drop accidentally-committed Inspector history dump; gitignore the class 83076441 swept an exported Inspector message-history JSON (manual-testing artifact at the repo root) into the tree via git add -A. Untrack it and extend the existing repo-root debug-artifact .gitignore block (alongside /*.png) to cover /inspector-history-*.json so these can't be re-added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
ed6eda7492 |
feat(web): wire up MCP task support — Tasks screen, Run-as-task, status toasts (#1422) (#1424)
Wires the ported Tasks screen + core task subsystem through App.tsx: sticky Clear Completed, cancel/refresh error toasts, core-side per-task progress correlation, Run-as-task on the Tools screen (gated on server support), per-server Task TTL, and live task-status toasts. |
||
|
Cliff Hall
|
9e494544e7 |
fix(state): gate list fetches on server capability (#1350) (#1395)
The four list-fetching state managers fired their list RPC against every server on connect regardless of advertised capabilities, so a server that doesn't implement a given primitive replied -32601 "Method not found", surfacing in the console on every connect. Mirrors the `tasks`-capability gate added for ManagedRequestorTasksState in #1349. - managedToolsState → gate on capabilities.tools - managedPromptsState → gate on capabilities.prompts - managedResourcesState → gate on capabilities.resources - managedResourceTemplatesState → gate on capabilities.resources (the spec defines no separate resourceTemplates capability; resources/templates/list is part of the resources surface) When the capability is absent each manager sets an empty list, dispatches its change event, and returns — the right semantics for "this server doesn't support X." Tests: existing flow tests now construct their FakeInspectorClient with the relevant capability so the live list path is still exercised; added a "refresh skips listX when capability absent" and a "connect against an X-less server doesn't fire listX" test per manager. Updated the useManagedX peers and resourceSubscriptionsState (which drives a ManagedResourcesState refresh) to advertise the capability too. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
5c5e5ab754 |
feat(network): show auth response bodies with masked secrets + reveal toggle (#1386) (#1387)
* feat(auth): inject MCP_INSPECTOR_API_TOKEN into served index.html (#1378) Reloading the web client at the bare URL (no `?MCP_INSPECTOR_API_TOKEN=…` query string) with empty sessionStorage made every `/api/*` request 401 — the browser had no way to recover the backend's auth token. Embed the token into `index.html` on every page load so the browser no longer depends on the query string surviving navigation: - New shared helper `clients/web/server/inject-auth-token.ts` embeds `<script>window.__INSPECTOR_API_TOKEN__ = "…"</script>` (escaped against `</script>` injection; no-op when auth is dangerously omitted). - Dev: the Vite plugin injects via `transformIndexHtml`. - Prod: the Hono server injects on the `/` route. - `App.tsx` `getAuthToken()` now reads the injected global first, then the query string, then sessionStorage (both fallbacks preserved). - Shared global name lives in `INSPECTOR_API_TOKEN_GLOBAL` (`core/mcp/remote/constants.ts`). Tests: helper unit coverage + an integration test exercising the real prod server's `/` → `/api/*` flow (injected token authenticates; missing token 401s). AGENTS.md documents the token-recovery order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(auth): wire OAuth authorization-code flow into App.tsx (#1379) OAuth-protected MCP servers could not be connected to from the v2 web client: the core OAuth pipeline exists, but App.tsx never invoked it, so a connect attempt 401'd and surfaced "Remote send failed (401): … Missing Authorization header" as a toast. Wire the two missing entry points (all core primitives already in place): - Auto-trigger on 401: in onToggleConnection's catch, detect an upstream 401 (isUnauthorizedError) and call client.authenticate(), which runs discovery + DCR (backend-proxied) and redirects the page to the auth server via BrowserNavigation. The initiating server id is persisted to sessionStorage first, since the OAuth `state` carries only mode+authId and the full-page redirect wipes React state. - /oauth/callback handler: a mount effect that, once `servers` hydrate, parses the callback params, recovers the pending server, rebuilds its InspectorClient, runs completeOAuthFlow(code) (PKCE verifier + DCR client info survive in BrowserOAuthStorage), replaceState("/") so a reload can't replay the single-use code, then connect(). An `error=` callback toasts instead of retrying. connect() already attaches the OAuth provider to the transport (inspectorClient.ts), so once tokens land in BrowserOAuthStorage the outbound request carries the bearer token. Extracted the pure pieces (constants + isUnauthorizedError) to src/utils/oauthFlow.ts with unit tests. Verified end-to-end in a real browser against the MCP SDK demo OAuth server: Connect -> redirect -> auto-approve -> callback -> Connected, with the access token shown in the Connection Info modal (#1377). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): persist OAuth pre-redirect Network log across the redirect (#1384) After #1379, the Network tab showed only the post-redirect auth HTTP (discovery re-run + POST /token); the pre-redirect discovery and the DCR POST /register that run during authenticate() were lost when the page navigated to the auth server. Root causes: 1. Ordering — BrowserNavigation set `window.location.href` before the client's `saveSession` event fired (OAuthManager calls onBeforeOAuthRedirect *after* auth() already navigated), so the save raced the unload and was dropped. Fix: BrowserNavigation now runs a synchronous `beforeNavigate` hook immediately before assigning location.href; App wires it through createWebEnvironment to flush the active fetch log to RemoteInspectorClient Storage (keyed by the authId parsed from the auth URL) via a keepalive POST that outlives the unload. 2. Illegal invocation — RemoteInspectorClientStorage defaulted to `this.fetchFn = globalThis.fetch` and called `this.fetchFn(...)`, which re-binds `this` and makes native fetch throw "Illegal invocation" (swallowed by the catch). This silently broke *all* session save/load. Fix: default to a wrapper that preserves the global receiver. 3. Restore race — hydrateFetchRequests replaced the list, so a load that resolved after the resuming connect appended live entries would clobber them. Fix: merge restored (older) entries ahead of live ones, dedupe by id. saveSession also now uses keepalive: true. Verified end-to-end against the MCP SDK demo OAuth server: the connected page's Network tab shows the full handshake — pre-redirect discovery + DCR /register plus post-redirect discovery + /token as `auth`, alongside `transport`. Added unit tests for the beforeNavigate ordering and the hydrate merge/dedupe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(network): show auth response bodies with masked secrets + reveal toggle (#1386) Auth-category Network entries showed request body/headers/status but never the response body (rendered "(empty)"), because buildEffectiveAuthFetch deliberately skipped capturing it to avoid surfacing access_token / refresh_token. That hid the most useful thing to inspect when debugging OAuth — the token exchange. Capture auth response bodies, but mask sensitive OAuth fields by default behind a click-to-reveal toggle so they aren't exposed at a glance during a screen-share: - inspectorClient: wire updateResponseBody on the auth fetcher. - src/utils/maskSecrets.ts: maskSecretsInBody() masks access_token, refresh_token, id_token, client_secret (case-insensitive, nested) in JSON bodies; reports whether anything was masked. Non-JSON / secret-free bodies pass through untouched. - NetworkEntry BodyPreview: when a body has masked fields, render it masked with a Reveal/Hide toggle (copy honors the shown view). Masking is a UI concern; the raw entry is unchanged so reveal shows the real value. access_token / refresh_token live in the post-redirect /token response, which is never written to the session-restore files (#1384); only pre-redirect bodies (public discovery, DCR /register) persist, so no bearer token hits disk. Verified end-to-end: the /token response shows masked by default (access_token: "••••••••"), Reveal exposes the raw value, and discovery / the public DCR /register response (no secret) render with no toggle. Added util + component unit tests and a story play function. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): inject token into prod SPA fallback + prime sessionStorage + no-store (#1378) Addresses code-review feedback on the token-injection PR: - Prod `server.ts` SPA deep-link fallback (e.g. `/oauth/callback`) previously served the raw index.html off disk via serveStatic, bypassing injection — so a bookmark/reload at a non-`/` route with empty sessionStorage would 401. Route the SPA fallback through the same `serveIndexHtml` (inject) helper; real static assets (paths with a dot) still serve verbatim. Dev already injected on every HTML serve via Vite `transformIndexHtml`. - `getAuthToken()` now persists the injected `window.__INSPECTOR_API_TOKEN__` to sessionStorage (not just the URL-param branch), priming the backstop for any later navigation that loses the global. - Injected HTML responses now send `Cache-Control: no-store`, so a page carrying a token isn't cached and served stale after a restart regenerates the token. Integration tests added: SPA fallback (`/oauth/callback`) carries the token, `Cache-Control: no-store` on injected HTML, real assets served verbatim, and unknown `/api` routes 404 rather than falling through to the HTML shell. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): address #1383 review — split OAuth/connect toasts, tighten 401 match (#1379) Code-review feedback on the OAuth-wiring PR: - Callback effect: split completeOAuthFlow vs connect() into separate try/catch blocks. A token-exchange failure now reads "OAuth token exchange failed … Please try connecting again." (the single-use code is spent and the URL was cleared, so a reload can't retry); a post-OAuth connect failure reads "Failed to connect" since OAuth actually succeeded and re-clicking Connect reuses the persisted tokens. - isUnauthorizedError: anchor the message fallback on the transport's `failed …(401)` wording instead of a bare `(401)`, so an unrelated `(401)` spliced into an error message can't trip the OAuth flow. Added a test. - Documented that clearing the pending id + URL before the server lookup is intentional (deleted/renamed server mid-flow → require a fresh Connect). Also merges the squash-merged #1382 base from v2/main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(auth): address #1385 review — clarify double-save + keepalive cap + test fetch default (#1384) Code-review feedback on the OAuth Network-log persistence PR: - Documented the double-save: `FetchRequestLogState`'s `saveSession` listener is the backstop; `BrowserNavigation`'s `beforeNavigate` hook is the primary flush for the redirect case. Notes the listener may lose the navigation race and is harmless when it duplicates (last-writer-wins, identical payload). - Reworded the keepalive comment in `RemoteInspectorClientStorage.saveSession`: the 64KB cap is general (the method is also reachable from the listener with the full session log), so a long session could exceed it and drop silently — acceptable since the persisted log is best-effort, not load-bearing. - Added a regression test that constructs `RemoteInspectorClientStorage` without a `fetchFn`, stubs `globalThis.fetch`, and asserts the default wrapper calls it (locks in the "Illegal invocation" fix, which callers otherwise swallow). Optional items (logger.warn on swallowed save errors; setupClientForServer dep churn) acknowledged on the PR, not changed. Also merges the squash-merged #1383 base from v2/main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(network): address #1387 review — form-encoded masking, cleaner flag, a11y (#1386) Code-review feedback on the auth-response-body masking PR: - Extended masking to form-urlencoded bodies (finding #1): the token *request* is `application/x-www-form-urlencoded` and carries `code` / `code_verifier` / `client_secret` / `refresh_token`. `maskSecretsInBody` now masks those in form bodies too (preserving formatting; placeholder not percent-encoded). `code`/`code_verifier`/`client_assertion` are form-only sensitive keys — deliberately NOT masked in JSON, where `code` is usually an error/status code. - Replaced the double-stringify `hasSecrets` heuristic with an explicit masked-flag propagated out of `maskNode` (finding #2) — robust if the transform ever grows non-identity behavior, and one fewer serialization. - Reset reveal state on body change via `key={body}` remount instead of a setState-in-effect (finding #3; avoids the cascading-render lint rule). - a11y (finding #4): `aria-label` on the Reveal/Hide button and `aria-live` on the hidden/revealed status text. - Security tripwire comment near the `saveSession` listener (finding #6): captured auth bodies are unmasked at source (masking is UI-only), so any new post-token-exchange persistence path must redact first. Tests: form-encoded masking (token + refresh requests), JSON `code` NOT masked, non-object JSON pass-through, default-fetch wrapper. Story play + NetworkEntry tests updated for the new aria-labels and the now-masked form request body. Finding #5 (pretty-print asymmetry) is already handled by ContentViewer's JSON formatting; #4-aria and #6 are the doc/a11y bits. Also merges the squash-merged #1385 base from v2/main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(network): address #1387 third-pass review — content-type masking, wholesale mask, dedup (#1386) Third-pass code-review feedback on the auth-body masking PR: - maskSecretsInBody now takes the body's content-type (finding #1): `*json*` → JSON masking, form-urlencoded → form masking, any other known type (HTML/plaintext/XML) → no masking; absent/unknown → sniff as before. Removes the implicit "non-JSON ⇒ form" guess for error pages etc. NetworkEntry passes the request/response `content-type` header through to BodyPreview. - Mask any non-empty value under a sensitive key wholesale (finding #2): a non-standard object/array value under e.g. `access_token` is replaced rather than recursed into, so it can't leak. Empty-string still not flagged. - Extracted `isSensitiveKey(set, key)` to dedupe the JSON/form key checks (finding #3). - Reworded `MaskResult.masked` doc to cover the form (non-pretty-printed) case (finding #6). Tests: non-string-value-under-sensitive-key wholesale mask; repeated form param; empty form value not flagged; content-type honored (JSON type skips form masking, text/plain skips masking, explicit form type masks). Finding #5 (story re-hide) skipped per reviewer — unit test already covers re-mask. Finding #4 (form edge cases) added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(network): address #1387 fourth-pass nits — content-type contract, key, form test (#1386) Fourth-pass review (LGTM) — minor, non-blocking items: - Documented the content-type matching contract on `maskSecretsInBody`: substring match (`*json*` / `*x-www-form-urlencoded*`), and we trust the wire's own label (a mislabeled body renders raw — acceptable for the screen-share threat model) (finding #1). - Key `<BodyPreview>` by content-type + body (not body alone) so a header-only change would also reset the reveal state (finding #3; not reachable today). - Storybook `AuthSuccess`: assert revealed count `>=` reveal-button count to mirror the `hidden >= 2` check, so adding a non-masked body later can't drift the assertion silently (finding #4). - Added a `NetworkEntry` unit test for form-encoded request-body masking (code/code_verifier) — previously exercised only via the Storybook play. Finding #2 (silent decodeURIComponent fallback on malformed keys) acknowledged as a known quirk outside the threat model; no change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(network): address #1387 fifth-pass — memoize masking, mask DCR mgmt token (#1386) Fifth-pass review (LGTM): - Memoized `maskSecretsInBody` in `BodyPreview` (recommended item #1): a Reveal/Hide click no longer re-parses + re-walks the body; cost is once per mount, and the `key` remount re-runs it on body/content-type change. Guarded so a too-large body is never parsed (the hook runs unconditionally, with the size check inside the memo). - Added `registration_access_token` (RFC 7592 DCR management credential, same bearer class) to the masked key set, and a confidential-client `/register` response fixture test (#5) asserting client_secret + registration_access_token masked, client_id/metadata visible. - Tightened the `isMaskableValue` doc to state the exact contract: non-null, non-empty-string values are masked wholesale (#3). Items #2 (transport parse cost — capped by the memo) and #4 (test-only MASK_PLACEHOLDER export) left as-is per the review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(network): clarify body-update emits list event only (#1387 sixth pass) (#1386) Sixth-pass review (LGTM, all nits). Added a comment on `onFetchRequestBodyUpdate` noting it re-emits only `fetchRequestsChange`, not the per-entry `fetchRequest` event — list-reading consumers pick up the body on the next render; a future per-entry subscriber would need its own event. Other items left as-is per the review: full-string `BodyPreview` key (#1, micro-perf only if profiled), NaN under a sensitive key (#3, consistent with the mask-non-null-non-empty contract), `&`-only form separator (#4, OAuth never emits `;`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
98192fd86e |
fix(auth): persist OAuth pre-redirect Network log across the redirect (#1384) (#1385)
* feat(auth): inject MCP_INSPECTOR_API_TOKEN into served index.html (#1378) Reloading the web client at the bare URL (no `?MCP_INSPECTOR_API_TOKEN=…` query string) with empty sessionStorage made every `/api/*` request 401 — the browser had no way to recover the backend's auth token. Embed the token into `index.html` on every page load so the browser no longer depends on the query string surviving navigation: - New shared helper `clients/web/server/inject-auth-token.ts` embeds `<script>window.__INSPECTOR_API_TOKEN__ = "…"</script>` (escaped against `</script>` injection; no-op when auth is dangerously omitted). - Dev: the Vite plugin injects via `transformIndexHtml`. - Prod: the Hono server injects on the `/` route. - `App.tsx` `getAuthToken()` now reads the injected global first, then the query string, then sessionStorage (both fallbacks preserved). - Shared global name lives in `INSPECTOR_API_TOKEN_GLOBAL` (`core/mcp/remote/constants.ts`). Tests: helper unit coverage + an integration test exercising the real prod server's `/` → `/api/*` flow (injected token authenticates; missing token 401s). AGENTS.md documents the token-recovery order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(auth): wire OAuth authorization-code flow into App.tsx (#1379) OAuth-protected MCP servers could not be connected to from the v2 web client: the core OAuth pipeline exists, but App.tsx never invoked it, so a connect attempt 401'd and surfaced "Remote send failed (401): … Missing Authorization header" as a toast. Wire the two missing entry points (all core primitives already in place): - Auto-trigger on 401: in onToggleConnection's catch, detect an upstream 401 (isUnauthorizedError) and call client.authenticate(), which runs discovery + DCR (backend-proxied) and redirects the page to the auth server via BrowserNavigation. The initiating server id is persisted to sessionStorage first, since the OAuth `state` carries only mode+authId and the full-page redirect wipes React state. - /oauth/callback handler: a mount effect that, once `servers` hydrate, parses the callback params, recovers the pending server, rebuilds its InspectorClient, runs completeOAuthFlow(code) (PKCE verifier + DCR client info survive in BrowserOAuthStorage), replaceState("/") so a reload can't replay the single-use code, then connect(). An `error=` callback toasts instead of retrying. connect() already attaches the OAuth provider to the transport (inspectorClient.ts), so once tokens land in BrowserOAuthStorage the outbound request carries the bearer token. Extracted the pure pieces (constants + isUnauthorizedError) to src/utils/oauthFlow.ts with unit tests. Verified end-to-end in a real browser against the MCP SDK demo OAuth server: Connect -> redirect -> auto-approve -> callback -> Connected, with the access token shown in the Connection Info modal (#1377). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): persist OAuth pre-redirect Network log across the redirect (#1384) After #1379, the Network tab showed only the post-redirect auth HTTP (discovery re-run + POST /token); the pre-redirect discovery and the DCR POST /register that run during authenticate() were lost when the page navigated to the auth server. Root causes: 1. Ordering — BrowserNavigation set `window.location.href` before the client's `saveSession` event fired (OAuthManager calls onBeforeOAuthRedirect *after* auth() already navigated), so the save raced the unload and was dropped. Fix: BrowserNavigation now runs a synchronous `beforeNavigate` hook immediately before assigning location.href; App wires it through createWebEnvironment to flush the active fetch log to RemoteInspectorClient Storage (keyed by the authId parsed from the auth URL) via a keepalive POST that outlives the unload. 2. Illegal invocation — RemoteInspectorClientStorage defaulted to `this.fetchFn = globalThis.fetch` and called `this.fetchFn(...)`, which re-binds `this` and makes native fetch throw "Illegal invocation" (swallowed by the catch). This silently broke *all* session save/load. Fix: default to a wrapper that preserves the global receiver. 3. Restore race — hydrateFetchRequests replaced the list, so a load that resolved after the resuming connect appended live entries would clobber them. Fix: merge restored (older) entries ahead of live ones, dedupe by id. saveSession also now uses keepalive: true. Verified end-to-end against the MCP SDK demo OAuth server: the connected page's Network tab shows the full handshake — pre-redirect discovery + DCR /register plus post-redirect discovery + /token as `auth`, alongside `transport`. Added unit tests for the beforeNavigate ordering and the hydrate merge/dedupe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): inject token into prod SPA fallback + prime sessionStorage + no-store (#1378) Addresses code-review feedback on the token-injection PR: - Prod `server.ts` SPA deep-link fallback (e.g. `/oauth/callback`) previously served the raw index.html off disk via serveStatic, bypassing injection — so a bookmark/reload at a non-`/` route with empty sessionStorage would 401. Route the SPA fallback through the same `serveIndexHtml` (inject) helper; real static assets (paths with a dot) still serve verbatim. Dev already injected on every HTML serve via Vite `transformIndexHtml`. - `getAuthToken()` now persists the injected `window.__INSPECTOR_API_TOKEN__` to sessionStorage (not just the URL-param branch), priming the backstop for any later navigation that loses the global. - Injected HTML responses now send `Cache-Control: no-store`, so a page carrying a token isn't cached and served stale after a restart regenerates the token. Integration tests added: SPA fallback (`/oauth/callback`) carries the token, `Cache-Control: no-store` on injected HTML, real assets served verbatim, and unknown `/api` routes 404 rather than falling through to the HTML shell. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): address #1383 review — split OAuth/connect toasts, tighten 401 match (#1379) Code-review feedback on the OAuth-wiring PR: - Callback effect: split completeOAuthFlow vs connect() into separate try/catch blocks. A token-exchange failure now reads "OAuth token exchange failed … Please try connecting again." (the single-use code is spent and the URL was cleared, so a reload can't retry); a post-OAuth connect failure reads "Failed to connect" since OAuth actually succeeded and re-clicking Connect reuses the persisted tokens. - isUnauthorizedError: anchor the message fallback on the transport's `failed …(401)` wording instead of a bare `(401)`, so an unrelated `(401)` spliced into an error message can't trip the OAuth flow. Added a test. - Documented that clearing the pending id + URL before the server lookup is intentional (deleted/renamed server mid-flow → require a fresh Connect). Also merges the squash-merged #1382 base from v2/main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(auth): address #1385 review — clarify double-save + keepalive cap + test fetch default (#1384) Code-review feedback on the OAuth Network-log persistence PR: - Documented the double-save: `FetchRequestLogState`'s `saveSession` listener is the backstop; `BrowserNavigation`'s `beforeNavigate` hook is the primary flush for the redirect case. Notes the listener may lose the navigation race and is harmless when it duplicates (last-writer-wins, identical payload). - Reworded the keepalive comment in `RemoteInspectorClientStorage.saveSession`: the 64KB cap is general (the method is also reachable from the listener with the full session log), so a long session could exceed it and drop silently — acceptable since the persisted log is best-effort, not load-bearing. - Added a regression test that constructs `RemoteInspectorClientStorage` without a `fetchFn`, stubs `globalThis.fetch`, and asserts the default wrapper calls it (locks in the "Illegal invocation" fix, which callers otherwise swallow). Optional items (logger.warn on swallowed save errors; setupClientForServer dep churn) acknowledged on the PR, not changed. Also merges the squash-merged #1383 base from v2/main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
4641339cc3 |
feat(network): add Network screen surfacing backend fetch log (#1355) (#1363)
* feat(network): add Network screen surfacing backend fetch log (#1355) Routes `FetchRequestLogState` to a new Network tab so users can inspect HTTP-layer request/response headers, status, duration, and category (auth vs transport) of every fetch the backend makes for an active session. Adds `NetworkControls` + `NetworkStreamPanel` + `NetworkEntry` groups, a `monoBreak` Text theme variant for long header values, and wires onClearNetwork / onExportNetwork (JSON download) from `App.tsx`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(network): include request / response bodies in search filter Headers were already covered but bodies were not, so searches for strings that live inside the JSON-RPC payload (e.g. "jsonrpc", "initialize", a tool name) silently returned no matches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(network): always render Response Body section, note streaming Streaming responses (text/event-stream, application/x-ndjson, POST to /mcp) are intentionally not body-captured by the fetch tracker, so the section was silently omitted. Now whenever a response was received we render the section, with either the body, a "(empty)" note, or a "Streaming response — body not captured" note keyed off content-type. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(network): capture response bodies via async body-update events Two related fixes: 1. The fetch tracker was marking every streamable-HTTP POST /mcp call as "not captured" because the response content-type is text/event-stream. In practice those streams are bounded — the server sends the JSON-RPC reply (sometimes preceded by progress events) and closes — so we can clone the body and read it. Only GET + SSE / ndjson is the unbounded long-lived channel that has to stay uncaptured. 2. Reading the body inline blocked the transport from processing progress notifications until the entire stream finished, breaking the `resetTimeoutOnProgress` integration test. The body is now read in the background and dispatched via a new `fetchRequestBodyUpdate` event that the FetchRequestLogState patches into the matching entry by id. Plumbing wires through node/transport, the remote SSE channel (RemoteSession, RemoteClientTransport), and the remote event types. Also hides the Network tab when the active server is stdio (no HTTP traffic to surface) and bumps the inline body preview limit to 100 KB so typical tools/list responses render rather than tripping the "too large" notice. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(network): show orange SSE badge for long-lived stream entries Adds a small "SSE" badge to the right side of the NetworkEntry header (left of the HTTP status badge) when the response is a long-lived streaming channel (GET + text/event-stream or application/x-ndjson) — the only case where the body genuinely isn't captured. Makes the "not captured" placeholder less surprising at a glance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(network): address PR review findings - Stop capturing OAuth response bodies (drop updateResponseBody from the effectiveAuthFetch tracker). Restores pre-PR behavior for auth-category entries so live access_token / refresh_token values can't leak into the Network tab body preview during a screen-share. Headers + status are still tracked. - Extract `isLongLivedStreamResponse(method, contentType)` to core/mcp/fetchTracking so the rule lives in one place; consumed by the tracker and the NetworkEntry UI placeholder. - Document the intent of NetworkEntry's `useEffect` (the list-level toggle is authoritative; per-entry overrides are intentionally discarded on each list toggle). - Rename MAX_INLINE_BODY_BYTES → MAX_INLINE_BODY_CHARS (`.length` is UTF-16 code units, not bytes) and update the user-facing notice. - Switch NetworkStreamPanel's search from joined-string `.includes` to per-field `.some(.includes)` so a term can't span field boundaries. Regression test added. - Add aria-label to ListToggle ("Expand all" / "Collapse all") — both an a11y improvement and a stable selector that lets the panel test stop fishing the button out by text-content negation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(network): use ListToggle aria-label in panel + modal tests Cleanup spotted in PR review follow-up: - HistoryListPanel + ServerSettingsModal previously fished the ListToggle out by empty text content because the toggle had no accessible name. Now that ListToggle exposes "Expand all" / "Collapse all", both tests can use getByRole + name and the stale comments go away. - ListToggle's own tests now assert the aria-label contract so a future refactor can't silently drop it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(network): expose category toggle state via aria-pressed The category UnstyledButtons in NetworkControls previously communicated their toggled state only via background color. Adds aria-pressed={active} so assistive tech can discover the on/off state without visuals, with a test that asserts the attribute tracks the visibleCategories prop. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
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> |
||
|
Cliff Hall
|
db1916cd34 |
feat(servers): persist server list to ~/.mcp-inspector/mcp.json (#1343) (#1349)
* feat(servers): persist server list to ~/.mcp-inspector/mcp.json (#1343) Replaces the hardcoded SEED_SERVERS in App.tsx with a file-backed list at ~/.mcp-inspector/mcp.json. Full CRUD round-trips through new /api/servers routes; first launch writes the two existing seeds if the file is absent. Backend (core): - store-io: getDefaultMcpConfigPath() — sibling of getDefaultStorageDir() - core/mcp/serverList.ts: pure converters MCPConfig <-> ServerEntry[], DEFAULT_SEED_CONFIG, and normalizeServerType (moved from node/config.ts to keep serverList.ts Node-free) - core/mcp/remote/node/server.ts: GET/POST/PUT/:id/DELETE/:id /api/servers routes, plus RemoteServerOptions.mcpConfigPath defaulting to getDefaultMcpConfigPath(). Ids validated with validateStoreId. Frontend: - core/react/useServers.ts: { servers, loading, error, refresh, addServer, updateServer, removeServer } over the new endpoints - App.tsx: drops SEED_SERVERS, wires useServers, disconnects on remove of the active server, follows id on rename of the active server - ServerConfigModal: Add/Edit/Clone form (stdio: command/args/env/cwd; sse/streamable-http: url/headers as one-per-line text) - ServerRemoveConfirmModal: confirm-on-remove with id + transport summary Tests (52 new): - unit: getDefaultMcpConfigPath env-var permutations, serverList round-trip, hook flows against real Hono app.fetch (no TCP), modal forms + validation - integration: /api/servers routes against a real createRemoteApp + tmp file - stories with play functions for both new modals Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): address PR #1349 review (correctness + nits) Bugs: - normalizeServerType: validate against the allowed set and default unknown / non-string types to "stdio" (matches the missing-type case). Prevents a hand-edited {"type":"websocket"} from leaking past `as ServerType` and surprising downstream narrowing. - onConfirmRemove: when removing the active server, also destroy() the 9 state managers and null them + the client out so they GC immediately instead of waiting for the next server switch. - /api/servers mutating routes: serialize POST/PUT/DELETE through an in-process write-lock so concurrent read-modify-write cycles can't lose updates. New integration test fires 25 parallel POSTs and asserts every entry lands. - readMcpConfig comment: update to match actual behavior (valid-JSON file with no `mcpServers` → empty; invalid JSON → 500 from the outer try, so corruption surfaces rather than silently emptying the list). New integration tests pin the 500 path on both GET and POST. Nits: - Add --inspector-surface-subtle token, use it in ServerRemoveConfirmModal instead of the raw Mantine var. - Rename useServers buildHeaders boolean param to includeJsonBody. - Add a comment on the env-value vs header-value trim asymmetry (env values preserve trailing whitespace; header values follow RFC 7230). Tests: 1630 unit + 404 integration + 306 storybook all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(ServerConfigModal): null currentTarget in functional setState updater React nulls SyntheticEvent.currentTarget after the synchronous handler returns. Reading e.currentTarget.value inside `setForm((f) => ...)` worked under happy-dom (which keeps currentTarget around) but threw "Cannot read properties of null (reading 'value')" on the first keystroke in a real browser. Capture the value before the functional updater closes over it. All seven onChange handlers in ServerConfigModal had the bug. Extended the AddEmpty storybook play to type into Server ID and Command — the real-Chromium runner now exercises this path so a future regression fails the test suite instead of the user. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(remote-server): surface stderr + transport_error when subprocess crashes during startup A stdio subprocess that spawns successfully but exits before the browser opens the SSE events stream used to vanish into a bare "Session not found" 404. The fix has three parts: 1. RemoteSession.markTransportDead always pushes the transport_error event (via pushEvent's queue) rather than dropping it when no consumer is attached. The queued event drains when the consumer eventually arrives. 2. /api/mcp/connect's transport.onclose handler no longer deletes the session immediately when no consumer is attached. Instead it holds the session (with queued stderr + transport_error) for a 30s grace window so the events endpoint can drain it. A timeout sweeps if no consumer ever connects. 3. /api/mcp/events detects isTransportDead after attaching the consumer (which just drained the queue), yields once to flush, then closes the stream and deletes the session. End result: the user sees the actual error (e.g. "Cannot find module" from stderr + the transport-closed reason) instead of a confusing 404. Tests: - New unit tests (clients/web/src/test/integration/mcp/remote/remote-session.test.ts) cover the queueing behavior, live delivery, and clearEventConsumer signaling under all transport-state permutations. - New integration test (connect-crash.test.ts) spawns a node subprocess that writes to stderr and exits, then verifies the events stream delivers both the stderr line and the transport_error event before closing — the exact scenario from PR #1349. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(state): gate listRequestorTasks on the server's `tasks` capability ManagedRequestorTasksState was calling tasks/list on every connect. Servers that don't advertise the `tasks` capability (the common case) reply with -32601 "Method not found", which then surfaces in the inspector's console for every connect. Skip the call when the server doesn't declare `tasks` — empty tasks list is the right semantics. Existing tests opt into capabilities so the live-path coverage is preserved; two new tests pin the gated behavior (direct refresh + connect-event-triggered refresh). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(App): clear activeServerId on disconnect so other server cards re-enable ServerCard dims any card whose id differs from `activeServer` (and marks it inert), so cards stay non-interactive after a disconnect until the user reconnects elsewhere. activeServerId was set on connect but never reset on disconnect, leaving the whole screen locked to the just- disconnected card. Subscribe to InspectorClient's `disconnect` event and clear activeServerId there. Covers all three disconnect paths uniformly: - user toggles the connected card off - user clicks the header Disconnect button - mid-session transport failure (subprocess exit / network drop) Event-based instead of effect-based on connectionStatus to avoid the first-render-clobbers-new-id trap (status starts as "disconnected" for a freshly-created client before connect() runs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
6678313373 |
Expose live resource subscriptions via a hook (#1325) (#1326)
* Expose live resource subscriptions via a hook (#1325) Add ResourceSubscriptionsState that mirrors InspectorClient's subscribed URIs as InspectorResourceSubscription[], resolving each URI against the managed resources list (so the subscription tile shows server-supplied name/title) and stamping lastUpdated on notifications/resources/updated. Pair it with useResourceSubscriptions and wire App.tsx so the Resources screen reflects subscribe/unsubscribe actions in real time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address review notes on resourceSubscriptionsState - Soften the onResourceUpdated comment: the client's dispatch is already guarded by subscribedResources.has(uri), so the re-check is true defense-in-depth rather than guarding a known hazard. - Use this.getSubscriptions() in the statusChange handler so every emit goes through the defensive-copy path. - Document the deliberate fallback to a synthetic Resource when a previously-listed resource is removed from the managed list while the user is still subscribed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Reflect subscribed state in preview + compact subscribed tile labels - Wire `isSubscribed` on the ReadResourceState passed to ResourcesScreen by deriving it from the live subscriptions list in App.tsx. The ResourcePreviewPanel's SubscribeButton already flips its label to "Unsubscribe" when subscribed; without this derivation isSubscribed was always false and the button looked stuck on "Subscribe". - In ResourceSubscribedItem, display only the last URI path segment (truncated with ellipsis if it still overflows) and surface the full URI via a hover tooltip. Keeps the Subscriptions pleat readable when resources have long path-style URIs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Auto-read resource on sidebar click Clicking a resource in the URIs accordion now triggers onReadResource in addition to setting the selection, so the preview panel jumps straight to the pending → ok render path. Removes the unreachable "Click to read this resource" placeholder, since the loader / preview / error states always cover the rendered output for a selected resource. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Render markdown resources via react-markdown + fix preview overflow - Add react-markdown / remark-gfm dependencies. - ContentViewer accepts an optional mimeType prop; when it matches text/markdown (or text/x-markdown), text blocks render via react-markdown wrapped in a `.markdown-content` container. Non-markdown text now uses the existing Code "wrapping" variant so long lines stay inside the panel. - ResourcePreviewPanel infers `text/markdown` from a `.md` / `.markdown` URI suffix when the server didn't supply a mimeType, and threads the effective mime through to ContentViewer per content item. - ResourcesScreen flex children get miw=0 so a long unwrappable line in the resource body can no longer push the preview past the viewport's right edge. - App.css gains a `.markdown-content` ruleset constraining nested pre, table, code, and img elements to the container's width. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Contain resource preview: sticky header/footer, scrollable body Restructure ResourcePreviewPanel into a fixed-height column with the resource title + URI pinned to the top and the timestamp / annotations / subscribe-refresh actions pinned to the bottom. The content viewer area in the middle now owns its own ScrollArea, so a long markdown body scrolls within the panel instead of pushing the subscribe button below the viewport. In ResourcesScreen, the selectedResource branch (and the template branch's right pane) now hosts the panel inside a PreviewPane Flex column with the screen's max-height, and renderReadState wraps in a FillDetailCard sized to fill that column. The legacy outer ScrollArea.Autosize is removed for these panes since scrolling is internal now. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Auto-hide template after read; size preview to content with cap - After the user clicks Read Resource on the template form, handleReadResource now clears selectedTemplateUri so the screen swaps from the template form to the resource preview. Previously both panels stayed mounted side-by-side. - The preview no longer hard-fills the viewport. The Card uses a new "preview" theme variant (overflow: hidden) and is content-sized, capped at SCROLL_MAX_HEIGHT. ResourcePreviewPanel's flex column marks the header / meta / footer rows as `flex: 0 0 auto` and the ContentScroll as `flex: 0 1 auto` with `mih: 0`, so: - short content → card hugs it, footer sits right under the body - long content → card caps at viewport, ContentScroll shrinks and scrolls internally, footer stays pinned at the cap - ScrollArea / Group imports in ResourcesScreen pruned to match the simpler single-pane layout. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Cap template form panel at 40% width A bare text input + Read Resource button stretched across the whole main content area looks awkward on wide displays. Apply maw=40% to the PreviewPane in the template branch so the form keeps a reasonable form-field width regardless of viewport. The preview branch is unaffected — resource bodies still get the full main area for content. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Live completion/complete suggestions on resource template inputs Wires the server's `completions` capability through to the resource template form so each variable's input becomes an Autocomplete that fires `completion/complete` (debounced, 300ms) on every keystroke and surfaces the returned values as a dropdown — mirroring v1's behavior. - core/mcp/inspectorClientProtocol.ts: surface getCompletions on the protocol so non-runtime callers (state managers, hooks, tests) can depend on it. FakeInspectorClient gains a vi.fn-backed stub. - ResourceTemplatePanel: accepts onCompleteArgument + completionsSupported. When both are present it renders Mantine Autocomplete instead of TextInput, debounces keystrokes via per-arg timers, aborts in-flight requests on the next keystroke, and disables client-side filtering (the server already filtered for the typed prefix). - ResourcesScreen / InspectorView: thread the props through; the screen-level callback re-injects the active template's URI as the `ref: "ref/resource"` so the panel-level callback stays ref-free. - App.tsx: wires onCompleteArgument to inspectorClient.getCompletions and derives completionsSupported from `capabilities?.completions`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
0ead1deea6 |
feat(core): port v1.5 InspectorClient + transports + auth subsystem (#1302) (#1306)
* wip(core): bulk-copy v1.5 InspectorClient runtime files (#1302) Verbatim ports from origin/v1.5/main into v2/main core/: - logging/ (2 files) - storage/ + storage/adapters/ (4 files) - auth/ + auth/browser/ + auth/node/ + auth/remote/ (16 files) - mcp/config.ts + mcp/node/ (4 files, stdio transport) - mcp/remote/ + mcp/remote/node/ (12 files, HTTP/SSE transport) - mcp/inspectorClient.ts (~2169 LOC), messageTrackingTransport, fetchTracking, oauthManager, index, state/index Total: ~7,800 LOC of source files, copied unmodified via `git show origin/v1.5/main:<path> > <dest>`. Also: - Add deps: pino, hono, @hono/node-server, atomically, zustand, @types/node - tsconfig.app.json: extend paths to resolve new node-only deps from core/, add "node" to types, disable erasableSyntaxOnly (v1.5 code uses parameter properties — TODO to re-enable in follow-up) Compiles down to 47 type errors, all in three categories: - 24 TS2305: v2 dropped v1.5 type exports (InspectorClientOptions, CreateTransport, InspectorClientEnvironment, AppRendererClient, SamplingCreateMessage class, ElicitationCreateMessage class, jsonUtils helpers, OAuth events). Per user direction, will re-add these to v2's diverged files (types.ts, samplingCreateMessage.ts, etc.) so the ported code stays verbatim. - 15 TS7006: implicit-any params in v1.5 code (v1.5 tsconfig was looser than v2's noImplicitAny). Need explicit annotations. - 8 other TS errors: ../package.json import, Transport|null narrowing, hono Context generic mismatch, one unused parameter. Tests not yet ported (19 files, ~11,170 LOC). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * wip(core): integrate ported v1.5 InspectorClient + port tests (#1302) Source integration (typecheck + build now pass): - Re-add v1.5 types/classes dropped during v2 evolution: - types.ts: CreateTransport*, AppRendererClient, InspectorClient{Environment,Options} - samplingCreateMessage.ts / elicitationCreateMessage.ts: classes + keep v2's InspectorPending{Sampling,Elicitation} interface - inspectorClientEventTarget.ts: oauth* event keys - json/jsonUtils.ts: convertParameterValue / convertToolParameters / convertPromptArguments + ParameterSchema - inspectorClient.ts: replace `../package.json` import (v2 has no core/package.json) with a hardcoded identity constant - remote/node/server.ts: cast hono Context for streamSSE (hono's generic typing tightened since v1.5) - oauth-callback-server.ts: drop unused hostname field - tsconfig: enable @types/node, add paths for new bare-module deps (pino, zustand, hono, atomically) in app + test configs - vite.config.ts: mirror those module aliases for the unit project (which runs from repoRoot) Tests ported (19 files, ~11K LOC) under clients/web/src/test/core/: - All core/__tests__/auth/* (7 files) - inspectorClient.test.ts + 4 OAuth variants - mcp/oauthManager.test.ts, jsonUtils.test.ts - transport, remote-transport, remote-server-config, storage-adapters - helpers/oauth-client-fixtures.ts Bulk-rewrote `from "../mcp/…"` → `from "@inspector/core/mcp/…"` etc. Current state: - typecheck: 0 errors - build: passes (vite v8, 788 modules) - unit tests: 1014 / 1034 pass (937 pre-existing + 77 new core tests) - 20 failures in 13 files; failure modes: - 13 file-load failures needing the v1.5 `test-servers/` package (@modelcontextprotocol/inspector-test-server) which isn't in v2 yet or real fs/network not available in happy-dom - 7 assertion failures in discovery.test.ts + state-machine.test.ts (SDK 1.29 vs 1.25 API drift — mocks don't match new shape) Coverage gate not yet checked against new files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * wip(core): port test-servers, more types, partial test integration (#1302) - Port v1.5 test-servers/ package (12 files, ~5K LOC) under repo-root/test-servers/, wire @modelcontextprotocol/inspector-test-server alias in both vite.config.ts (regex form) and tsconfig.test.json. - Add deps: express ^5, yaml ^2, @types/express ^5. - Re-export InspectorClientOptions/Environment/Transport* from inspectorClient.ts so v1.5 tests that import them from that module path keep resolving. - Switch InspectorClientEventMap to use SamplingCreateMessage / ElicitationCreateMessage classes (matches v1.5 event payload contracts) while keeping the InspectorPending{Sampling,Elicitation} interfaces in the *.ts modules for v2 consumers that still use them. - Fix verbatim-module-syntax issue in test-servers and drop unused stdio config field. - Adjust inspectorClient.test.ts task types to TaskWithOptionalCreatedAt (SDK 1.29 tightened Task shape vs v1.5's 1.25). - Drop spurious maxMessages/maxFetchRequests/maxStderrLogEvents from remote-transport.test.ts (never on InspectorClientOptions). State: - typecheck: 0 errors - build: passes - unit tests: 1099 / 1270 pass - 171 failures in 10 test files, almost all integration tests that need real HTTP/fs/network (happy-dom can't host them). Next: add a node-env vitest project to run these properly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(core): port v1.5 InspectorClient + transports + auth subsystem (#1302) Bulk-port of the v1.5 core runtime (~7,800 LOC source + ~16K LOC tests + ~5K LOC test-servers/) into v2. Files copied verbatim from origin/v1.5/main via `git show >`; only minimal adapters added on the v2 side. Subsystems landed ----------------- - core/logging/, core/storage/, core/auth/ (browser + node + remote) - core/mcp/ runtime: inspectorClient.ts (2169 LOC), oauthManager, messageTrackingTransport, fetchTracking, config, index - core/mcp/node/ (stdio transport) - core/mcp/remote/ + remote/node/ (HTTP/SSE transport + Hono server) - test-servers/ package (composable MCP test servers, fixtures, harness) v2 adapter layer ---------------- - types.ts: append CreateTransport*, AppRendererClient, InspectorClient{Environment,Options} (v2 had pruned these — v1.5 needs them) - samplingCreateMessage / elicitationCreateMessage: keep v1.5 classes AND v2's Inspector Pending* interfaces in the same files - inspectorClientEventTarget: add oauth* event keys; switch pending sampling/elicitation payloads to the classes (matches v1.5 contracts) - json/jsonUtils: append convertParameterValue/convertToolParameters/ convertPromptArguments + ParameterSchema - inspectorClient.ts: re-export Options/Environment/Transport* so tests that import these from `inspectorClient.js` keep resolving; replace `../package.json` import with a hardcoded identity (v2 has no core/package.json) - remote/node/server.ts: cast hono Context for streamSSE (hono generic tightened since v1.5) - oauth-callback-server: drop unused hostname field - tsconfig.app.json / tsconfig.test.json: add @types/node + paths for pino/zustand/hono/@hono/node-server/atomically/express/yaml, disable erasableSyntaxOnly (parameter-property syntax from v1.5) - vite.config.ts: regex-form module aliases (so the package's exports field handles subpaths like `hono/streaming`, `zustand/middleware`), alias for @modelcontextprotocol/inspector-test-server → test-servers/src Tests ----- - 19 v1.5 test files copied under clients/web/src/test/core/, imports rewired to @inspector/core/* and @modelcontextprotocol/inspector-test-server - 1004 unit tests pass; 298 storybook tests pass; coverage gate passes Known follow-ups (out-of-scope here, tracked in TODOs) ------------------------------------------------------ - 13 integration test files are excluded from the unit project (spawn real HTTP/stdio servers; run real OAuth flows; mock SDK auth → happy-dom CORS). Needs a node-env vitest project. Files listed under `test.projects[0].test.exclude`. - Source files exercised only by those integration tests are excluded from the coverage gate pending the node-env project. Listed under `test.coverage.exclude` with comment. - Manual smoke tests against real stdio / HTTP / OAuth server per the AC on #1302 still need verification locally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs/fix: address #1306 review (AGENTS tree, #1307 links, @types/node, .substr) - AGENTS.md: expand the project-structure tree to include all v1.5-ported subsystems (core/auth/{browser,node,remote}, core/storage, core/logging, core/mcp/{node,remote/{,node}}, core/mcp/state) plus the new top-level test-servers/ directory (with a note about its @modelcontextprotocol/ inspector-test-server alias). - vite.config.ts: reference #1307 from both the test exclude block and the coverage exclude block so the next person editing either sees the marching order for shrinking the lists. - package.json: restore @types/node to ^24 (an earlier `npm install --save-dev @types/node@^22` accidentally pinned it lower than the repo was using; nothing here required ^22). - core/mcp/fetchTracking.ts: replace deprecated .substr(2, 9) with .slice(2, 11) to match the convention in core/mcp/inspectorClient.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
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> |
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
d57801d7b3 | ||
|
|
9a8488681e | ||
|
|
3b4eb447c9 | ||
|
|
6a08c77c92 | ||
|
|
bfeebd440d | ||
|
|
da8cc166ce | ||
|
|
63623b754b | ||
|
|
b33532c024 | ||
|
|
de60983ccc | ||
|
|
fe5b622fd1 | ||
|
|
fad8480376 | ||
|
|
f201032137 | ||
|
|
373f346e5f |
||
|
|
2259b1a3e1 |
||
|
|
320daf2f67 |
||
|
|
b3b04c6939 |
||
|
|
f9f79fff54 | ||
|
|
19a6df7ad3 | ||
|
|
dc31c28102 |
||
|
|
2488ca5e38 |
||
|
|
53dc4549be |
||
|
|
721dae08c6 |
||
|
|
b0a024269e |
||
|
|
ed6eda7492 |
||
|
|
9e494544e7 |
||
|
|
5c5e5ab754 |
||
|
|
98192fd86e |
||
|
|
4641339cc3 |
||
|
|
85a4b4c057 |
||
|
|
db1916cd34 |
||
|
|
6678313373 |
||
|
|
0ead1deea6 |
||
|
|
e3e70470b5 |