| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
cliffhall
|
621b712229 |
Merge remote-tracking branch 'origin/v2/main' into v2/feat/2294-skills-catalog-budget-settings
Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
eb41185c37 |
feat: make the skills verification catalog budget configurable per server (#2294)
Adds a Skills section to Server Settings with Maximum Number of Skills and Maximum Catalog Size fields, defaulting to SKILL_MAX_CATALOG_SKILLS (256) and SKILL_MAX_CATALOG_BYTES (64 MiB). The values persist as skillCatalogMaxSkills / skillCatalogMaxBytes on the server's mcp.json entry (omitted at the default), are validated as positive integers by the /api/servers routes, and are read by verifySkills through getServerSettings(), so the CLI's --verify and the TUI Skills pane both honor them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P6i1HBSdSS9kL4npZ2dDSe Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
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
|
cf856c740c |
fix: do not await the discarded stream's cancel in /api/fetch (#2319 round 26)
Third and last site of the same class. `ReadableStream.cancel()` adopts the underlying source's cancel promise, which is permitted never to settle, so awaiting it kept the handler pending forever — and on an unbounded request there is no timer to release it either. The cancel is started and the handler returns, so the JSON response and the `finally` complete regardless. The `void` carries the justification for which of AGENTS.md's permitted cases it is: the callee owns its failures through its own `catch`, and the caller cannot await. Test: a `/stream-hostile` upstream that sends event-stream headers and one frame and then never closes, asserting the route still answers 200 with no body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
1e501fa5cb |
fix: redact sensitive query values from timeout diagnostics (#2319 round 23)
The timeout message interpolated the raw request URL, and createFetchTracker records that message verbatim into the Network log and the persisted session. An OAuth endpoint's query string can carry a `code`, an `access_token` or a `client_secret` — exactly the fields fetchTracking already redacts everywhere else it records a URL — so a stalled signed endpoint would have written its secret to both. Redacted with the existing `redactUrlQuery`, in the OAuthRequestTimeoutError constructor rather than at each display site, so both the message and the public `url` field are covered by construction and no future consumer can reintroduce it. The route's own proxied-timeout message and the URL in its 504 marker get the same treatment, since both cross back to the browser and are recorded there. The path and every non-sensitive parameter survive, so the endpoint stays identifiable — which was the point of naming it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
e234343702 |
fix: cancellable compat probes and a validated budget (#2319 round 16)
Two real bugs and two stale rationales.
The OIDC fallback probes were issued with `{ headers }` and no signal,
so a caller that aborted after the initial 404 got a timeout reported
in place of its own cancellation, while the probe ran on to its own
deadline. The probes now carry the caller's signal — via the shared
`callerSignalOf`, so the precedence rule has one definition — a
pre-aborted caller is not probed at all, and an abort is rethrown
rather than folded back into the preceding response.
`Math.max(0, Math.round(timeoutMs))` let NaN and Infinity through.
`setTimeout(fn, NaN)` fires immediately, and a delay above 2**31-1
overflows a 32-bit signed int and falls back to 1ms — so a caller
asking for a very large budget, or one whose arithmetic produced NaN,
got an immediate OAuth timeout. `normalizeBudget` falls back to the
default on non-finite and clamps to the timer's range; the route's
envelope-supplied deadline gets the same clamp, but ignores a
non-finite value outright since a malformed deadline reads safest as
no deadline.
The wire-marker docblock and the matching route test still said a
backend deadline is the only one on the transport-internal discovery
path, which round 11 changed. Both now give the real reason the marker
matters: both ends run the same budget, and it is the backend winning
the race — a backgrounded tab throttling setTimeout — that must not
decide whether the error names its endpoint.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
de6960ec5a |
fix: make the proxy deadline per-request, not unconditional (#2319 round 13)
The server-side timer added in round 9 applied to every /api/fetch request. But that route carries MCP traffic as well as OAuth work, so a Streamable HTTP tool call that legitimately withholds its response headers past the budget was aborted there — reintroducing on the backend exactly the regression exemptMcpEndpoint prevents on the client, and reporting it as an OAuth timeout besides. The caller now decides. `withOAuthRequestTimeout` stamps its budget on the RequestInit it hands down, `createRemoteFetch` reads it back and puts it in the /api/fetch envelope, and the route applies a timer only when one is present — so an exempt MCP request, which never gets an init of its own, is bounded by nothing. The carrier is a WeakMap keyed on the init rather than a header: serializeRequest copies request headers verbatim into the payload and the route re-sends them to the authorization server, so a marker header would leak to a third party. It needs no cast onto RequestInit and is collected with the object. The `fetchTimeoutMs` option from round 9 is gone — the deadline now lives in exactly one place, the client wrapper, which is also what makes the 504 marker honest: only a request the OAuth wrapper bounded can produce one. Ten tests: the stamping and its rounding, nothing stamped on an exempt call or an init that never went through the wrapper, the envelope carrying the budget (and not a header), its omission on both exempt and direct calls, and at the route the 504 marker, no deadline without one, and a non-numeric deadline ignored. The no-deadline case is verified by mutation: an unconditional timer makes it fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
baefc96806 |
fix: carry the proxy deadline's identity, correct the rounding note (#2319 round 9)
A deadline enforced by the backend could not arrive as an OAuthRequestTimeoutError: /api/fetch serialized it as a 500 and createRemoteFetch turned that into a plain Error, so every instanceof check downstream was false — including the one that lets a stalled probe escape withRfc8414OidcCompat. That matters most where no client-side wrapper exists to have fired first: the discovery the SDK runs from inside the transport. The route now answers 504 with a typed marker (OAUTH_TIMEOUT_WIRE_CODE plus url and timeoutMs) and createRemoteFetch reconstructs the error from it, guarded by a shape check so an upstream's own JSON cannot forge one. The route's deadline becomes a `fetchTimeoutMs` option defaulting to the shared constant, so the backstop can be proven to fire in milliseconds instead of thirty seconds of wall clock — and because a very slow authorization server is a legitimate reason to want a longer one. The rounding rationale was wrong: setTimeout accepts a fractional delay and truncates it (verified on Node 26); ERR_OUT_OF_RANGE is AbortSignal.timeout's behaviour, which this wrapper never calls. The rounding stays, for the reason that is actually true — the budget is interpolated into the message and exposed as timeoutMs, and a performance.now() budget would otherwise report 1000.4000000953674ms. Also drops a sentence in the discarded-stream test that contradicted the qualification above it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
a452305204 |
fix: cancel the discarded proxy stream, refresh revocation docs (#2319 round 8)
The /api/fetch route classifies an event-stream or NDJSON response as a stream and then answers with JSON and no body — so nobody downstream owns the upstream stream and nothing will ever read it, while the `finally` clears the only deadline on the way out. It is now cancelled explicitly, best-effort, the way `releaseBody` in oidcDiscoveryCompat already does. Measured, undici reclaims an unread body here on its own, so this is belt and braces rather than a demonstrated leak — kept because relying on that is an implementation detail of the fetch beneath us, not a contract. The route test and the source comment both say so rather than implying a stronger claim. Forwarding the signal also made revocation.ts's rationale stale: it still described a proxy that drops `init.signal` and a backend that cannot cancel. Both comments there, and the matching one in revocation.test.ts, now describe the post-#2319 path — the signal cancels, the race guarantees, and what the race is still for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
d848300077 |
fix: enforce the OAuth deadline across the proxy hop (#2319 round 5)
Two findings, both real. The race bounded only the caller's promise on the browser path. `createRemoteFetch` dropped the signal, so an abort — including the deadline's own — left the POST to /api/fetch in flight; the backend never saw its request cancelled and its outbound fetch ran on with neither signal nor timeout, holding a handler and an authorization-server socket per timed-out attempt. `createRemoteFetch` now forwards the caller's signal (init.signal when present and not undefined, else a Request's own), and the /api/fetch route composes `c.req.raw.signal` with a server-side deadline matching the client budget, cleared in `finally` so a streaming response is not severed. A probe's own timeout was swallowed by `withRfc8414OidcCompat`, which returned the preceding 404 and discarded the endpoint name the deadline exists to provide. A deadline the Inspector imposed is its own error, not the server's answer, so it escapes now; every other probe failure still falls back as documented. Nothing regresses: every candidate the loop probes is one the SDK's own buildDiscoveryUrls emits, so a stall there is one the SDK would have hit itself, unbounded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
Cliff Hall
|
5ba2fadd3b |
Merge branch 'v2/main' into v2/feat/2144-oauth-revocation | ||
|
cliffhall
|
85c5206895 |
fix: cancel a Streamable HTTP call by closing its stream (#2140)
The Cancel button POSTed `notifications/cancelled` on every transport. On a 2026-07-28 Streamable HTTP connection that is the wrong signal — the spec makes closing the request's SSE response stream the cancellation signal there and says the notification is neither required nor expected — so a spec-compliant server acknowledged it 202 and dropped it while the tool ran to completion. The SDK already implements the fork, off `transport.hasPerRequestStream`. Every Inspector connection is wrapped in `MessageTrackingTransport`, which did not forward that flag, so the SDK read `undefined` and took the stdio branch on every client — the CLI and TUI included, whose real `StreamableHTTPClientTransport` advertises it correctly. The web client needed three more links: its browser transport answers for the upstream transport that lives on the backend, so it advertises the flag for `streamable-http` and applies the SDK's `requestSignal` to its `POST /api/mcp/send`; and `/api/mcp/send`, held open for the whole call, forwards the browser's disconnect to the upstream `transport.send`. The test fixture's `toWebRequest` built a Web Request with no `signal`, so no fixture server could observe a disconnect at all — without that, a test asserting the cancel reaches the server passes vacuously. stdio and SSE keep `notifications/cancelled`: they multiplex every request over one shared channel and have no per-request stream to close. So does the legacy era, which predates the mechanism. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
65fd2b9e35 |
feat(auth): revoke OAuth tokens at the authorization server on clear (#2144)
Clearing the Inspector's OAuth state deleted the local copy of the tokens and stopped there. The access token — and the refresh token, which is long-lived by design — stayed valid at the authorization server until they expired on their own, so a day of connect/disconnect iteration left it holding grants for sessions that ended hours ago. RFC 7009 §1 describes exactly that case, and nothing in the repo constructed a revocation request, so a server author implementing `revocation_endpoint` had no way to exercise it with the tool that exists to exercise this. core/auth/revocation.ts builds and sends the request; the three clear paths call it, so the behavior is written once — web's "Clear OAuth state and disconnect" (both the active and stored-only paths), the TUI's Auth tab, and the CLI's --relogin. It runs BEFORE the local clear, because the token, the client credentials and the discovered `revocation_endpoint` all live in the store the clear is about to empty. The request names the refresh token when there is one: §2.1 asks the AS to also invalidate the access tokens issued under the same grant, so one request covers both halves. Best-effort by construction. Every path returns a TokenRevocationOutcome rather than throwing: no advertised endpoint means nothing is sent and that authorization server behaves exactly as before, and a network error, a non-2xx, a 5s timeout or an unreadable store is reported and nothing more. Forgetting the tokens is what the user asked for. `lost_authorization_state` recovery deliberately skips it — it clears a half-finished flow to retry it, and an authorization that never completed has no grant to revoke. Client authentication resolves the preregistered slot first, mirroring BaseOAuthClientProvider.clientInformation; reading only the dynamic slot would send no authentication for a server configured with `oauth.clientId`. The Basic credential is byte-identical to the SDK's applyBasicAuth so this request and the token request cannot present the same secret differently. Opt out per server with `oauth.revokeOnClear` (default on, only `false` written to disk) or per CLI run with --no-revoke. Read at clear time rather than connect time, so toggling needs no reconnect. Turning it off is a testing affordance: a client that walks away still holding live tokens is a case a server author may want to reproduce. test-servers now advertises `revocation_endpoint` and serves POST /oauth/revoke, faithful on §2.2 (an unknown token is a success) and §2.1 (revoking a refresh token invalidates the access tokens under the same grant), with oauth-revocation-http.json / oauth-no-revocation-http.json as showcase configs. 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> |
||
|
Cliff Hall
|
6af849e217 |
Merge branch 'v2/main' into v2/fix/2096-stdio-env-cwd-revert | ||
|
cliffhall
|
d9b5f4f8a7 |
fix(web): carry a landed stdio env/cwd onto the config it connects with
The Server Settings modal edits a stdio server's `env` and `cwd` as settings, while the transport reads them off the *config* — so #2091's tracker, which resolves settings through the last write known to have reached disk, left those two behind: `server.config` still came straight off a `servers` entry that stops advancing once a list read fails. A save that landed during such a window kept spawning the child process with the pre-save environment, while the modal (re-seeded from the tracker) showed the new value. Credentials live in `env`, so the symptom is the inspected server failing to authorize rather than a visibly lost edit. Adds the settings→config mapping to `core/mcp/serverList.ts` — the one place both sides of the wire can share — and routes the `/api/servers` PUT write-through and the web client's connect path through it, so the value persisted and the value spawned cannot be derived differently. Closes #2096 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
|
e2fb78d999 |
feat(oauth): make the refresh_token grant opt-out per server
The Inspector always declared `refresh_token` in its OAuth client metadata. The SDK's `determineScope()` then appends `offline_access` whenever the authorization server advertises it and the client declares that grant, and `startAuthorization()` appends `prompt=consent` whenever `offline_access` is in scope. Against Microsoft Entra ID that forced consent prompt routes a non-admin user into the admin-consent workflow and fails with AADSTS90094 — even after a tenant admin has consented. Adds a per-server "Request refresh token" setting (default on) that drops the grant from the client metadata, breaking that chain at its source. Persisted as `oauth.requestRefreshToken` and written only when off, so entries that never touch it keep a byte-stable round-trip. Web edits it in Server Settings → Authorization; the CLI and TUI pick it up from the same per-server settings via `buildRunnerClientAuthOptions`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
fafc8367b8 |
fix(web): move the app origin off 6276, bound the publish route
Copilot review round 2 on #2100, plus a collision the review did not find. The default app-origin port was 6276 — which is already RUNNER_OAUTH_CALLBACK_DEFAULT_PORT, the CLI/TUI loopback OAuth callback. That port is fixed precisely so an OAuth app can pre-register http://127.0.0.1:6276/oauth/callback, which makes it the one listener here that cannot move when something takes it first. A running --web would hold it and break a later --cli/--tui OAuth flow at its registered redirect URI, and docs/mcp-app-review.md has those two running side by side. This listener has a documented dynamic fallback, so this listener yields: the default is now 6278 (6277 skipped — v1's retired proxy port, which the migration guide tells people to stop forwarding). A test asserts the two defaults stay distinct. Review comments: - POST /api/app-document now applies hono's bodyLimit BEFORE parsing. The 8MiB check ran after c.req.json() had buffered and parsed the whole request, and bounded only `html` — so bulk in `csp`, or in keys the route never reads, was fully materialized before the 413. - `csp` is validated as an HTTP header value and length-bounded. The controller hands it to res.writeHead() verbatim, where Node throws ERR_INVALID_CHAR synchronously — inside the handler for a later, unrelated request, outside this route's error handling. - The app-origin port now propagates everywhere the other two do: the CLI --print-handoff port-forward command (and its test), the Dockerfile's ENV and EXPOSE, and the remote-review SSH recipe in docs/mcp-app-review.md. Without it an app declaring _meta.ui.domain is unreachable through every documented remote workflow. 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
|
fa441da85f |
fix(web): harden the app-origin paths and correct three docs
Copilot review round 1 on #2100. - POST /api/app-document: `null` and `[1,2]` are valid JSON, so the destructure that followed threw OUTSIDE the parse try/catch and turned a malformed body into a 500 instead of the documented 400. - app-origin controller: the error handler cleared `server` but not `origin`, and `publish` gates on `origin` — so an error after a successful listen kept minting URLs on a dead port and the browser never took the srcdoc fallback. Clear both, and drop the now-unfetchable documents. - sandbox proxy: drop a stale `srcdoc` before navigating to a dedicated origin (srcdoc wins over src), and restore the opaque-origin posture — strip `allow-same-origin`, reset `innerOrigin` — before assigning `srcdoc` on the fallback path. Not reachable today (each render remounts the iframe), but the cost of being wrong about that later is an isolation boundary. - publishAppDocument tests: replace two `as unknown as Response` casts with real `Response`s. - docs: `frame-ancestors` admits the proxy AND the Inspector page (every ancestor is checked); the shared helper falls back to loopback, not `'none'`; the showcase origin is `127.0.0.1`, the default bind address. - tests: null/array body, post-listen error, and both factories receiving a working `publishAppDocument`. 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
|
ec40cf5531 |
feat(web): honor _meta.ui.domain with a dedicated app origin
An MCP App is rendered by handing its HTML to the sandbox proxy as `srcdoc`, in a frame sandboxed without `allow-same-origin` — the #1565 isolation model. That gives the app document an opaque origin, so every request it makes carries `Origin: null` and no CORS / OAuth-callback / API-key allowlist can admit it. `_meta.ui.domain` is the spec field a server uses to ask its host for a stable, dedicated origin, and it was being dropped on the floor. The field is explicitly host-dependent and the Inspector owns no domain infrastructure, so it treats any non-empty value as a request rather than an address, and answers with a real loopback origin on its own listener (MCP_APP_ORIGIN_PORT, default 6276). One shared origin, path-keyed per document: that delivers what the field is for without minting a port or DNS name per app, and is therefore not a per-app isolation boundary. The inner frame is granted `allow-same-origin` on this path only, which is what makes the origin real. It is not a #1565 regression: the listener is on its own port, so the app stays cross-origin to both the proxy and the Inspector. The proxy refuses the grant if the URL's origin equals its own, and the grant is never reachable from the server-supplied `sandbox` string, which is still stripped. The per-app CSP is delivered as a real response header there rather than only as a <meta>. Every failure path falls back to the default srcdoc render with a console warning naming the field, rather than blanking the app. Also fixes a pre-existing `tsc -b` break on v2/main (useServers.test.tsx passed `metadata: []` where `RequestMetadata` is expected), which has had CI red since #2087 and blocks the pre-push gate. 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
|
531cf84e60 |
Merge branch 'v2/main' into v2/fix/2067-proxy-undici
One conflict, in `clients/tui/__tests__/App.test.tsx`: both sides added an `opts` capture to the test harness's `FakeClient` for different fields — `defaultMetadata` on v2/main (#1910), `environment.fetch` here (#2067) — so the resolution is the union of the two, keeping both rationales and both tests. Everything else auto-merged. `npm run ci` passes on the result. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
9a084f49e5 |
fix(proxy): proxy /api/fetch, and detect entry-inlined externals
Addresses the Copilot review on #2099. - `/api/fetch` used the bare global `fetch`. That route is the browser's only way out to the network — the web client's `environment.fetch` is `createRemoteFetch()`, which forwards OAuth discovery and token requests there — so a corporate-proxy user could connect to a server but never authorize against it, and the PR's "OAuth is proxied now" claim held for the CLI and TUI but not for web. It now uses the same proxy-aware base fetch, with an integration test driving a real forwarding proxy (confirmed failing without the fix). - `verify:bundle-externals` only recognized an inlining that produced a `<pkg>-HASH.js` chunk. A *statically* imported package is folded into `index.js` and emits no chunk, so it would have passed. Detection now keys off esbuild's `// <path>` module banners, covering both shapes. Because that depends on unminified output, a bundle with no banners now fails as such rather than reporting clean — enabling `minify` cannot silently retire the check. Tests cover the entry-inlined case, the same-prefix case (`undici-types` must not read as `undici`), and the no-banner case. - The pre-push pipeline summaries in AGENTS.md, README.md, and .github/copilot-instructions.md still described `verify:build-gate → smoke` and omitted the new gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
56d8644032 |
fix: stop leaking metadata values into diagnostics, and salvage per key
Two credential leaks, the same class already fixed for the client logger: `serverList` printed the malformed entry verbatim — a legacy row missing `key` still holds whatever the user configured — and the CLI overflow error echoed the whole `key=value` pair into stderr and CI logs. Both now name the key and the offending *type*; a test plants a `sk-live-…` and asserts it appears nowhere while the key still does. `normalizeStoredMetadata` now also drops a value that parses but cannot be sent. A hand-edited catalog is the reachable source: it is parsed server-side, so `1e400` really does become `Infinity` there, and the next write would store `null`. Filtered per key, matching the per-item salvage this file already does elsewhere, so one bad value does not cost the user the rest. Deliberately *not* added to the `/api/servers` guard, which review also suggested. Writing that test is what showed why: a client's own `JSON.stringify` turns `Infinity` into `null` before the request is sent, so the wire cannot carry the bad case at all — and the check made things worse, rejecting the whole metadata field where the normalizer drops one key. The guard stays a container check, with a comment recording both points. 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
|
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
|
af3d940e78 |
fix(auth): stop a config-only PUT deleting stored secrets (#2084)
Round 24 produced no inline comments again. Of the three suppressed items, one is a pre-existing data-loss bug that predates #1950 entirely. **Editing a server's config deletes its stored OAuth client secret — from the keychain, not only from the stores this PR adds.** Filed as #2084 and fixed here, since the review surfaced it and these lines were already changing. `useServers.updateServer` deliberately sends `{ id, config }` with no `settings`, so a config-only save cannot wipe persisted settings. The route reads that as `preserve` and re-derives settings from the *on-disk* entry — which by #1356's design no longer holds the secrets. The submitted secret set is therefore empty, `expectedSecretFields` always lists the OAuth slot, and the reconcile deleted a value the user never touched. The rename branch was given `mergeRenameKeychainSecrets` for exactly this reason; the in-place branch never got the equivalent. The first fix was too broad — skip obsolete deletion whenever the intent is `preserve` — and an existing test caught it immediately: stdio `env` is part of `config`, so a config-only PUT that drops a key really has retired that secret. Obsolete detection is per-field now: - **`env:` fields** are implied by the entry's shape, so the new entry no longer expecting one is proof it was retired. - **The OAuth slot** is implied by nothing, so absence counts as "cleared" only when the caller spoke about settings at all. Both directions are tested, and the pre-existing env-key reconcile test still passes. Also: - `refreshingPersist` moves to `lib/`. The combinator computes nothing, but what it exists to do is sequence side effects, which is the `lib` half of the repo's split; I judged it the other way on the grounds that the function is a pure higher-order one, and the rule is about what a module is *for*. - `SecretStorageInfo.plaintext` is documented as file-only and *omitted* elsewhere rather than "always false", which is what the producer does and what `usableSecretStorage` enforces — the old wording invited a consumer to build the exact shape the web client discards. Full `npm run ci` green, staged tree verified first. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
15e9260484 |
fix(auth): make the durability guard provenance-aware, and harden the claim
Round 20, and almost all of it is fallout from round 19's two changes. Both were right in direction and wrong in the detail, in the same way: each solved its case by treating a broad thing as a proxy for a narrow one. **The durability guard wrote newly entered secrets to disk in plaintext.** Preserving legacy plaintext for a session-scoped store is correct — stripping it moves the only durable copy into RAM. Using *the whole submitted entry* as the proxy for "legacy" was not: it turned `MCP_INSPECTOR_SECRET_STORE=memory` into "write every new secret to `mcp.json` in the clear", while the footer for that very store says secrets are written nowhere. A security regression introduced by a data-loss fix, and the same overshoot in `client.json`. Provenance decides now. A secret field is kept on disk only when it was already there, unchanged, in the file being overwritten; a newly entered or changed value is stripped and lives in the session store, which is exactly what the footer promises. Where provenance cannot be established — an unparseable prior file — the value is treated as new, because writing a secret to disk on a guess is the failure that matters. **The atomic claim was not as safe as its comments said.** Four ways: - A crash between the claim and the copy left only `secrets.json.migrating-<pid>`. The next run checked the canonical path, found nothing, selected the keychain, and every stored credential vanished with no warning. Orphans are now adopted at startup. - Restoring used `rename`, which on POSIX **replaces** its destination — so the restore path could overwrite a `secrets.json` a concurrent writer had recreated, destroying the later successful write that claiming exists to protect. The comment beside it asserted the opposite. `link` + `unlink` is the no-clobber primitive this needed. - Every claim failure was read as a lost race. `EACCES`/`EROFS` leave the file exactly where it is, and returning quietly selected the keychain while file-backed secrets sat there invisible. Only `ENOENT` is benign now. - A failed cleanup left a plaintext-capable snapshot on disk with a silent catch, while the app reported the keychain as the only store. It names the path now. **A third secret-entry surface had no disclosure at all.** `ServerConfigModal` takes stdio `env` values, which are extracted into the store exactly as Server Settings' are — and the footer's own doc comment claimed there were only two such dialogs. It has the footer now, and every server write that can carry a secret (config submit, both import flows) refreshes the descriptor, not just the settings save. Full `npm run ci` green, staged tree verified first. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
5ef3572578 |
fix(auth): stop losing preserved plaintext, and claim the hand-off file atomically
Round 19 found real data loss rather than process noise, in two places. **An unrelated settings edit destroyed the secret the durability gate had just preserved.** The GET migration withholds its strip for a session-scoped store, so `mcp.json` stays the durable copy — but the POST/PUT paths always wrote the stripped shape. The GET also returns the rehydrated secret to the settings form, which resends the whole object when the user changes anything, so editing a timeout wrote the stripped entry, moved the only durable copy into RAM, and lost it at exit. Every operation reported success. The client config had the identical shape: the read-path guard was there, the write path had none, and saving a CIMD URL did the same thing to the IdP client secret. Both write paths now agree with their read paths: while the store cannot outlive the process, the file stays the durable copy. **The hand-off's delete is no longer a check-to-delete race.** Re-reading immediately before `rm` caught writes that had already landed and was blind to the one arriving between the check and the delete — destroying a *later*, already-successful write, which is worse than the optimistic-write residual and, unlike it, fixable without a lock. The file is claimed by an atomic rename before anything reads it, so the migration operates on a snapshot nobody else can reach and a writer that recreates `secrets.json` is simply untouched. Claiming introduced a failure mode the existing tests caught immediately: a migration that aborts must not leave the secrets stranded at the staging path. The snapshot is restored on any incomplete outcome, and when the live path has been recreated in the meantime it is left in place with its location logged — an operator-visible orphan is recoverable; clobbering someone's write is not. Also: - **`setMany`**, the write-side twin of `getMany`. `set` costs a whole-file read-decrypt-encrypt-write-verify cycle per call, and the settings form resends a server's full `env` map on any edit, so a 10-variable stdio server spent ~30 scrypt derivations saving an unrelated change. One cycle now; the keychain keeps its parallel individual writes. - **A doc that contradicted its function.** `readOnDiskEncryption` still said it "never decrypts, so it needs no passphrase" — round 12 made it authenticate precisely because structure is not access. Full `npm run ci` green, staged tree verified before running it. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
78a65f7c20 |
fix(auth): address Copilot review round 17
Three findings. The first is the fourth consecutive one in the refresh path, so it is answered by making that path testable rather than by patching it again. **The refresh-after-persist wiring was covered by nothing.** It lived as two inline `await …; refresh()` pairs in `App.tsx`, which is deliberately outside the coverage `include` — so the hook's tests and the modals' tests could both pass while the callback joining them was broken, leaving the security footer stale after exactly the write that changed it. Three of the last four rounds found defects in this path; none of them could have been caught by a test, because there was nowhere for one to live. Extracted to `utils/refreshingPersist`, which is inside the gate and has its own tests: refresh runs after the persist resolves (not before — refreshing first re-reads the descriptor the pending write is about to invalidate), arguments pass through unchanged (the server-settings caller is `(id, settings)`, and a transparent wrapper is the difference between saving the entry and dropping it), and a failed persist refreshes nothing while propagating the error. **A tmpfs-style false promise, in the path fallback.** With neither `HOME` nor `USERPROFILE` set — an ordinary service environment — `defaultSecretFilePath` returned `.mcp-inspector/secrets.json`, a *relative* path, while `FileSecretStoreOptions.filePath` and the `SecretStorageInfo.path` the footer offers to copy both promise an absolute one. Resolved like the two override branches above it. **A warning that was usually false.** The session-store notice fired on every `/api/servers` read whenever the store was in-memory — including for the default empty catalog, where "plaintext values are left on disk" is not true of anything, repeated on every list refresh. It is now emitted lazily, at most once per server instance, and only when the loop actually preserves a non-empty secrets set. A log line that is usually untrue is one people learn to skip, which costs it the occasion it matters. The remaining suppressed comment is the hand-off's check-to-delete race, tracked in #2082. Full `npm run ci` green. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
26d2f58784 |
fix(auth): address Copilot review round 13
No new inline comments this round, but two of the four suppressed ones were real defects rather than restatements — both in code I added, both silently wrong in the safe-looking direction. **A symlinked secrets directory read as unmounted.** `isOnMountPoint` walks to the nearest existing ancestor *lexically* and then string-prefixes it against mountinfo, so `~/.mcp-inspector -> /data/inspector` with `/data` mounted matched no mount point. The container then selected the session-only store on a box that had explicitly arranged for persistence, and nothing said so — the mount was there, the volume was there, the secrets evaporated on exit. Resolved through symlinks now, tolerantly: a path that cannot be resolved falls back to the lexical comparison, which is what this did before and is correct whenever no symlink is involved. The `statSync` fallback branch already followed links, so this also makes the two paths agree. **The bulk read did not fix the case it was written for.** `getMany` batched *one server's* fields, but both rehydration callers iterate servers — so a catalog of 20 servers holding one secret each still paid 20 serialized scrypt derivations. The same ~450ms stall the seam was introduced to remove, reached one server at a time instead of one field at a time. Measuring the wrong unit and then optimizing it is worse than not optimizing, because the number in the commit message says it is fixed. The seam is cross-server now — `getMany(requests: SecretBulkRequest[])` — and both callers pass the whole catalog in one call. `FileSecretStore` decrypts once for the entire rehydration; the keychain fallback still issues independent parallel `get`s, which is what it wants. A test asserts one derivation for four servers, next to the existing one asserting one for three fields, so the property is pinned at both axes. The other two suppressed comments are the cross-process guarantee — `mutate`'s residual and the hand-off's TOCTOU delete — which remain decided against for this PR and tracked in #2082. Full `npm run ci` green. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
339aea3729 |
fix(auth): replace the secrets-file lock with optimistic verify-and-retry, and address review round 9
Two changes, interleaved in the same files because both are about the same read-modify-write path. ## The lock is gone (maintainer decision on the round-5/7/8 finding) Three consecutive review rounds found a real race in the `mkdir` lock, and the last one is not patchable with what Node exposes: claiming a stale lock needs compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`). Without it, `canSteal` inspects one lock but `rename` acts on whatever is at that path *later*, so a waiter that lost the first race can move the winner's fresh lock aside and enter alongside it. Rather than patch it a fourth time, writers now collide and the loser notices. `mutate` reads `M0`, applies the mutation to get `M1`, writes it, then reads back `M2`; if they differ, someone wrote in between, so it re-applies onto what they left and retries, bounded. The comparison is over the **whole map**, and that is the load-bearing detail. Checking only the entry we touched passes in exactly the case that loses data — ours is present, and it is the *other* writer's that is gone. With A and B from the same `M0`: A writes `MA`, B writes `MB` and clobbers A; B verifies and correctly sees `MB`; A verifies, sees `M2 !== MA`, re-applies onto `MB`, writes again. Both survive, and the writer that was clobbered is the one that repairs it. Two things this deliberately does not claim: - It is **not mutual exclusion**. A process that crashes between its write and its verifying read cannot notice it was clobbered, and nothing else will. That window is strictly narrower than the lock's — which lost updates with every participant alive — and needs no primitive Node lacks. Stated in the code rather than left to be rediscovered in review. - Non-convergence **fails loudly**. After five lost rounds `set` throws rather than returning, because returning would reintroduce the silent loss with extra steps. `delete` stays silent, per the interface contract. `serialize` is now keyed on the resolved path **process-wide** rather than per instance. Two `FileSecretStore`s on one file are ordinary — the resolved store holds one and `absorbFileSecretsIntoKeyring` builds another — and a per-instance queue does not order them against each other at all. The new convergence test caught this on its first run: two instances racing `set` lost an entry. In-process is now correct by construction; the verify covers what it cannot see, a second Inspector process. The hand-off's read → copy → delete was the lock's other user. Its delete is now conditional: the file is re-read immediately before removal and compared against what was copied, and it declines to delete when they differ. Leaving the file costs nothing (keychain-wins makes the next run idempotent) while deleting on a stale read costs a secret. ## Round 9 (two inline, two suppressed) - **`KeyringSecretStore.getStrict` let raw binding errors escape.** Both plaintext migrations keep their source file only when they catch `SecretStoreUnavailableError`, so a transient keychain failure 500'd `GET /api/servers` instead of abandoning the migration. Wrapped, as `set` already does. - **`FileSecretStore` had no strict read at all**, so `secretStoreGetStrict` fell back to the tolerant `get` and mapped every read failure to `null` — leaving the exact inversion the seam was introduced to prevent open on the store it was introduced for. - **`readOnDiskEncryption` reported "encrypted" for an envelope it cannot open.** Naming the cipher is not the same as being openable: a file with no `kdf`/`data`, or a `data` that is not `iv.tag.ciphertext`, produced the quiet reassuring footer for a file whose next save was guaranteed to fail. - **`/api/config` kept a stale descriptor when the resolver returned `undefined`**, because the conditional spread left the startup value standing. Assigned unconditionally now; `c.json` omits the key. Full `npm run ci` green. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
37faf1be88 |
fix(auth): address Copilot review round 8 (four inline findings)
The lock finding from this round is deliberately not addressed here — see below. - **My round-6 strict-read fix was inert in production.** `DeferredSecretStore` is what `defaultSecretStore()` returns, and it did not forward `getStrict` — so `secretStoreGetStrict` fell through to the tolerant `get` for every real caller, and the strictness existed only in tests that injected a concrete store. Forwarded now, along with `getMany`. - **The same tolerant-read bug remained in both pre-existing migrations.** I added `getStrict` for the new hand-off in round 6 and never applied it to the mcp.json and client.json migrations, which have the identical shape: decide from `get`, then write on `null`. A transient keychain read failure therefore let an older plaintext value overwrite a newer keychain one — and the disk copy was then stripped, so the newer value was gone. Both use the strict read now; the throw is a `SecretStoreUnavailableError`, which both call sites already handle as "abandon the migration, keep the file". - **The permission check made a false statement on Windows.** `stat` returns synthetic bits there, and interpreting them yields a confident falsehood either way — "0666 — not owner-only" about a file the ACL may restrict, or "owner-only" about one it does not. The ACL is the real answer and this does not inspect it, so Windows now reports `unknown`. - **Rehydration cost one full decryption per field.** `get` reads and decrypts the entire file, and both rehydration paths ask field by field, serialized behind the store's own queue. Measured on this machine, scrypt at N=16384 is ~23ms per derivation, so a 20-secret encrypted catalog spent **~450ms of pure key derivation on every `GET /api/servers`** — a visible stall, not a micro-optimization. Added an optional `getMany` seam: `FileSecretStore` decrypts once per server, everything else falls back to the parallel per-field `get` that already suited it. A test counts derivations directly: 1 for a three-field `getMany`, 3 for the three `get`s it replaced. Also broadens a log message that still said "Keychain unavailable" for a store that may not be a keychain. Full `npm run ci` green — 5,762 tests under the coverage gate. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
b143dd6cbd |
fix(auth): address Copilot review round 3 on the secret-store PR
Nine findings (six inline, three suppressed), all taken. Two were serious. **The memory fallback made the migrations destructive.** Both migrations — mcp.json in the remote server, client.json in `node-persistence` — lift a plaintext secret off disk, write it to the secret store, and then delete it from the file. That trade is safe only while the store is at least as durable as the disk, which held by construction while `InMemorySecretStore` was a test double. Making it a production fallback broke it: on an unmounted container a plain `GET /api/servers` would have moved a user's existing secrets into RAM and lost them at exit — destroying data on a read. `SecretStore` gains an optional `isDurable()`, absent meaning durable, and both migrations withhold the disk delete when it answers false (the value is still loaded, so the session behaves normally). The production fallback is now a named `SessionSecretStore` rather than the bare `InMemorySecretStore`, so the suite's many doubles keep standing in for a *working keychain* — which is what they are there to be — and only code that deliberately chooses RAM in production says so. **Round 2's per-call descriptor never reached anyone.** Both web servers resolved it once at startup and baked it into `initialConfig`, which `/api/config` serves unchanged — so the freshness fix was inert for its only consumer. The route now takes a `secretStorageResolver` and calls it per request. The lock needed three more fixes, and they compound: - **Release removed whatever directory was at the path.** A holder whose work outran the stale threshold would delete the *next* owner's lock, letting a third writer into a section two processes believed they held. Ownership is now stamped with a random token and release only removes a lock still carrying it. - **The timeout fell through to an unlocked write** — under contention, the one moment another writer is provably active. It now throws `SecretFileLockTimeoutError`, which `set` is allowed to do. - **The first write of every process bypassed the lock**, because the secrets directory does not exist yet and `mkdir` failed ENOENT. The parent is created first. First writes are exactly when two processes are most likely to start together. Fixing the timeout exposed a fourth: with stale (10s) below timeout, a waiter stole from a holder that was merely *slow* rather than dead — reinstating the lost update. A holder now heartbeats its lock, so "stale" means the holder is gone, and the timeout is what a live-but-slow holder produces. Lock timings are injectable so the tests can exercise contention, staleness, and timeout without spending the real intervals. Also: the loose-permission state is carried into `SecretStorageInfo` as `looseMode` and rendered as the caveat (it outranks the encryption caveat — a file others can read is a live exposure, while "unencrypted" is a property of a file only its owner can open), rather than living in a startup log the browser cannot see; and `usableSecretStorage` validates `pendingEncryption` and `looseMode` as the optional booleans/numbers they are (`pendingEncryption: "false"` is truthy and would have printed advice that is the opposite of the truth) and rejects file-only fields on a keychain or memory descriptor. Tests: both migrations against a session store with a durable control alongside, so the guard is visibly the difference; `/api/config` serving a changing resolver and falling back without one; lock ownership after a steal, the timeout under a live holder, no-steal from a slow holder, and a locked first write; `looseMode` through the descriptor, the caveat ordering, and the tone; and five more rejected payload shapes. Full `npm run ci` green — 5,704 tests under the coverage gate. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
bc408f5869 |
feat(auth): file-backed and in-memory SecretStore for hosts with no keychain
Closes #1950 Before this, a box with no reachable OS keychain — the published container (no D-Bus session, #1848), Android/Termux (no platform binary, #1905), a minimal Linux install without libsecret — could not persist an OAuth client secret or a stdio `env:` value at all. `KeyringSecretStore` degraded into a store that silently holds nothing: `get` returned null, `delete` no-opped, and `set` hard-failed with a 503. That is the correct failure behavior, but it left the documented, supported way to run the Inspector with no secret persistence whatsoever. Adds the two missing implementations and, more importantly, the policy that picks one and the surfaces that say which was picked. - `FileSecretStore` — one `0600` JSON document at `~/.mcp-inspector/secrets.json`, encrypted with AES-256-GCM whenever `MCP_INSPECTOR_SECRET_KEY` is set (scrypt, per-file random salt). The whole map is encrypted as a unit rather than value-by-value so the account names — `${serverId}:${field}` — are hidden too; a per-value scheme would leave a readable index of which servers you hold a client secret for. A file that can no longer be decrypted reads as empty (matching the interface's read tolerance) but *refuses to be written*, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - `secret-store-selection.ts` — the policy. `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright; otherwise probe the keychain and, if unreachable, fall back loudly. The fallback is `memory` in a container whose secrets directory is not on a mount, and `file` otherwise — so mounting the volume the README already recommends for the catalog flips the same run to durable storage with no configuration, and an unmounted container gets the honest answer rather than a file `--rm` will discard. Cached per process so the banner, `/api/config`, and the store doing the writing cannot disagree. - Automatic, but never silent. The downgrade prints at startup, rides `GET /api/config`, and is named in a permanent 32px footer at the bottom of both dialogs that accept a secret. A startup banner is seen once by whoever started the process; a toast is seen once; a dismissible banner is the thing a user dismisses before doing the work it describes. The plaintext and memory cases render their caveat inline in a warning tone — a warning you must hover to discover is not one. - `SecretStoreUnavailableError` is now the base every store's `set` throws, so the routes translate one condition into a 503 instead of matching `KeychainUnavailableError` by name. Docs: the container section of the root README gains a table of which store you get and how to make secrets survive, plus the `MCP_INSPECTOR_SECRET_KEY` upgrade/mismatch rules. Tests: the existing interface contract block is extracted to `secretStoreContract.ts` and run against all three implementations, so none can drift on the availability contract. `FileSecretStore` is tested against a real temp filesystem — mode, cross-instance round-trip, that the ciphertext does not contain the plaintext, the read-modify-write race, and every refusal path. Full `npm run ci` passes, per-file gate included. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
e58c80440b |
fix: guard the abort listener against a double invocation
Copilot review round 1. The helper's contract is exactly-once teardown and the test asserted it must not lean on Hono's own re-entry guard, but neither the implementation nor the test actually enforced that: the listener had no local guard and the test aborted only once. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
3dfac1c3db |
fix: register SSE abort cleanup before the priming write
Both SSE handlers installed their subscriber, primed the stream so Firefox's fetch() resolves (#1858), and only then registered onAbort. Hono's onAbort has no already-aborted replay and abort() fires only the subscribers registered at that moment, so a client disconnecting while the priming write was in flight fired nothing: the session event consumer / server-list subscriber stayed installed, the mcp.json watcher was never stopped, and the handler parked forever on a promise nothing would resolve. Add primeAndHoldSseStream, which registers one listener before the first await — it both cleans up and releases the hold, collapsing the previous duplicate onAbort registrations. Priming still happens after the subscriber is installed, since callers treat the stream's first bytes as proof they are subscribed. Closes #1999 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
Cliff Hall
|
4e4aa49db1 |
Merge pull request #2037 from modelcontextprotocol/v2/feat/1906-auth-token-url-overrides
feat(auth): Auth URL and Token URL overrides in Server Settings |
||
|
cliffhall
|
d830213796 |
fix: address round-2 review — stale disconnect, lint-project gap, coverage
Copilot round 2 (PR #2039) surfaced no new visible comments but six suppressed ones. Four were actionable. `clients/tui/src/App.tsx` — a stale-completion race in the new banner: start disconnecting server A, switch to B, then let A's disconnect reject, and A's failure appears under B. Rapid same-server retries had the same shape, and the stale attempt is the one most likely to reject. Claim a monotonic attempt token before awaiting and publish only if it is still current and the server has not changed; `handleConnect` bumps the same counter so a connect retires an in-flight disconnect. The rejection is still owned when superseded — just not published. The regression test drives a deferred rejection across a server switch, and fails without the guard. `tsconfig.lint.json` — the root type-aware block matches `test-servers/src/**/*.tsx` but the parser project omitted that extension, so such a file would have failed with "not found in any of the provided project(s)" instead of being linted. Added, and AGENTS.md now states the invariant: the include must stay a superset of the lint globs. `clients/web/server/{server,vite-hono-plugin}.ts` — the `open()` rejection branch was untested, and duplicated. Both were fixed by removing the duplication: `openBrowser` now lives beside `printServerBanner` in `web-server-config.ts`, with one implementation to test. Two copies of a one-line `.catch` is two chances for one to drift back to a bare `open(url)`. The dev plugin is not booted in tests — it returns early under VITEST by design — but it calls the same tested helper. `core/mcp/remote/node/server.ts` — `closeAbortedStream`'s rejecting-close path, the entire reason the helper exists, had no test. Exported it (as this file already does for `requestIdForSendWait` / `mcpParamHeadersOnly`) and covered both legs, asserting no unhandledRejection fires. Also corrects AGENTS.md, which said each client lists "both" tsconfig projects — true for cli/tui/launcher, but web lists four leaf projects. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
519ecf49ee |
chore(lint): enable no-floating-promises in all five ESLint scopes
A floated promise is invisible at review time — the call reads like an awaited one minus four characters — and the unhandled rejection it produces surfaces in a different test, in a different file, as a stack pointing at SDK internals. In #1947 two un-held `callTool` calls made `npm run ci` unpassable and took a full investigation to attribute; the fix was two lines. `@typescript-eslint/no-floating-promises` catches the class outright, so enable it at `error` everywhere. The rule is type-aware and `tseslint.configs.recommended` is not, so each scope's config gains a parser project. The four clients name their own tsconfig projects (both of them — `src` and `__tests__`/`server` live in different ones). The root scope has no tsconfig at all, so add `tsconfig.lint.json`: a lint-only project covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts`, emitting nothing and gating nothing. It sets `moduleResolution: bundler` deliberately — under NodeNext `core/`'s extensionless imports fail to resolve, every import degrades to `any`, and the rule silently stops seeing promises at all. Measured surface, and the fixes: - core (2) — `stream.close()` in two Hono `onAbort` listeners. Hono invokes subscribers with a bare `subscriber()`, so a promise returned from the listener is floated one layer up; `closeAbortedStream` owns the rejection where it can actually be owned. - tui (5) — `handleDisconnect` had no catch of its own and its only caller is a key handler that cannot await, so a failing disconnect was a live unhandled rejection; it now surfaces the message the way `handleConnect` does. The rest are `void` at callers that genuinely cannot await, over callees that already own their failures. - web (92) — 83 are Storybook `expect(...)`, which is instrumented and returns a promise, now awaited (`scrollAreaStoryAssertions` is async for the same reason). Two tests were asserting against a provider call they never awaited. `open(url)` in both web servers now warns instead of escaping a synchronous callback. - cli, launcher — already clean. Type-aware linting costs time: `clients/web`'s `eslint .` goes from ~8s to ~19s, inside `validate`. Narrowing the loaded projects is the lever if that needs reducing later; dropping the rule is not. Closes #1959 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
364d8c9dbc |
feat(auth): per-server authorization and token URL overrides
Adds two optional per-server fields — `oauth.authorizationUrl` and `oauth.tokenUrl` — that override the endpoints authorization-server metadata discovery resolved, so a server can be pointed at a development or staging authorization server without changing what it advertises. SDK v2 routes both endpoints through the discovered metadata document and neither through OAuthClientProvider, so the override is applied by wrapping the client's base fetch and patching the metadata document in flight (core/auth/endpointOverrides.ts). Wrapping the base fetch — rather than only the auth fetch — also covers the discovery the SDK runs from inside the transport on the 401/refresh path. The fields round-trip through mcp.json, the /api/servers routes, the runner (CLI/TUI), and the Server Settings → Authorization panel, where a value that is not an absolute http(s) URL is flagged inline; at runtime such a value is dropped with a single warning rather than failing the connection. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
e6e4600a73 |
fix: validate authorizationParams off disk; note the EMA exception (#2018)
Copilot round 5 — three of four findings implemented.
- `cleanAuthorizationParams` validates the field as it comes off disk. Only the
web client's /api/servers route checked the shape; the CLI and TUI read
mcp.json directly, so `authorizationParams: "oops"` enumerated as the
character-indexed pairs 0=o, 1=o, … and `{ audience: 5 }` reached
URLSearchParams.set, which stringifies it — either way the Inspector sent
query parameters the user never wrote. Now validated in shared code so all
three clients enforce the documented string→string contract, warning on each
rejection. Follows `cleanRoots`, which guards the same class for `roots`.
- The form's description now states that these parameters are not sent while
Enterprise-managed authorization is on. The EMA leg authorizes against the
enterprise IdP — a different authorization server — and deliberately sends
none of them, so the copy was presenting a configured-but-inert value as
active.
- The /api/servers malformed-`oauth` warning listed an expected shape omitting
`onInsufficientScope`, which the validator accepts and the docs describe, so
it told users a valid config was malformed.
Ten new cases, all verified to fail against the pre-fix sources.
Declined: recording the authorize URL in the Network tab (repeat of round 2) —
that URL is reached by a top-level browser navigation rather than a tracked
fetch, so it has never appeared there for any authorization request; adding a
synthetic entry changes every OAuth flow and is a product decision for a
maintainer, not a change to make inside this PR.
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
b25ece8ded |
feat: per-server custom OAuth authorization-request parameters (#2018)
Real deployments gate IdP selection, prompt behavior, or audience on a query
parameter the OAuth/OIDC core specs never standardized (Keycloak's
`kc_idp_hint`, OIDC's `login_hint` / `prompt` / `acr_values`, Auth0's
`audience`), and none of them could be driven from the Inspector.
The Inspector never builds the authorize URL — the SDK's `auth()` does, and it
exposes no hook for extra parameters — so the merge happens at the one seam
that sees the finished URL: `BaseOAuthClientProvider.redirectToAuthorization`.
It lives in `core/`, so web, CLI, and TUI all inherit it.
- `core/auth/authorizationParams.ts`: the reserved-key list (`client_id`,
`code_challenge`, `code_challenge_method`, `redirect_uri`, `resource`,
`response_type`, `scope`, `state`), the key validator the form renders
inline, and the merge itself. Reserved keys are dropped with a warning
rather than silently overriding PKCE / the state binding / RFC 8707;
blank-key rows are skipped; the caller's URL is never mutated.
- Applied to the authorization request only — nothing wires it into the token
request.
- Persisted as `oauth.authorizationParams` (a string record) on the per-server
config beside `scopes`, edited as key/value rows in Server Settings ->
Authorization ("Additional authorization parameters"), with an inline error
per offending row plus a section-level warning.
- The enterprise-managed (EMA) leg redirects to a *different* authorization
server (the enterprise IdP), so it goes through the explicit
`redirectToExternalAuthorization` entry point, which skips the merge — a
`kc_idp_hint` or `audience` meant for the MCP server's AS must never be
appended to the IdP's authorize URL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
Cliff Hall
|
9e074aa6f2 |
Merge pull request #1946 from modelcontextprotocol/v2/fix/firefox-sse-stream-hang
fix: prime SSE streams so Firefox resolves the events fetch |
||
|
cliffhall
|
7a74b5eba1 |
fix: forward the negotiated protocol version to the remote backend transport
The web client's SDK Client owns the `initialize` handshake, so the `setProtocolVersion()` call it makes after initialize lands on RemoteClientTransport — a pass-through to the Node backend, not the real HTTP transport. The backend's StreamableHTTPClientTransport therefore never learned the negotiated version and stamped no `Mcp-Protocol-Version` on any request after initialize, starting with `notifications/initialized`. A stateful server that requires the header rejects that notification with HTTP 400. RemoteClientTransport now records the version and rides it on every `/api/mcp/send`; the backend applies it to its transport before the send, so that request and everything the transport issues afterwards (the standalone SSE GET, the session DELETE) carry the header. The version is validated against a token pattern on the backend so a client can't push arbitrary bytes into an upstream header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 |
||
|
cliffhall
|
9644ac7fe8 |
fix: prime SSE streams so Firefox resolves the events fetch
Firefox does not hand a streaming `fetch()` response to JS until the first *body* byte arrives; Chromium resolves the promise as soon as the headers do. Both SSE endpoints flushed headers immediately and then stayed silent until there was something to report, which deadlocked `/api/mcp/events`: `RemoteClientTransport.openEventStream()` awaits that fetch *before* the MCP client sends `initialize`, so no `initialize` -> no event to report -> no body byte -> the fetch never resolves. The web UI hung on "Connecting..." forever with nothing in the UI, the console, or the node logs. Write an inert `:` SSE comment the instant each stream opens. Conforming parsers ignore a comment line, so this unblocks the read without inventing a wire event and needs no client-side protocol change. `X-Content-Type-Options: nosniff` was also proposed on the issue but does not fix it -- verified against Firefox 153, the fetch still never resolves with the header set and no body byte. The delay is not MIME sniffing. The priming write is deliberately ordered *after* each handler registers its consumer (the session event consumer; the file-watch subscriber plus `ensureWatcher()`). Callers treat the arrival of the stream's first bytes as proof they are subscribed, so priming first would hand out that proof across an `await` and drop an edit made in the gap. `/api/servers/events` gets the same treatment: its fetch was equally stuck on Firefox until the first real change event, which for an unedited `mcp.json` never comes. Its client-side reader counted any `\n\n` as a change, so the inert frame would have fired a spurious background re-fetch on every connection -- it now skips frames carrying no `event:`/`data:` field. The transport's own `parseSSE` already drops comment frames correctly and needed no change. Verified end to end in real Firefox against a live streamable-HTTP test server: before, stuck on "Connecting..." past 25s; after, initialize -> notifications/initialized -> tools/list, connected. Closes #1858 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU |
||
Sam Morrow
|
5d91d4acd0 |
fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call (#1847)
* fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call (#1846) A strict modern (2026-07-28) HTTP server (e.g. the GitHub MCP Server) rejects a tools/call whose argument carries an `x-mcp-header` annotation when the matching `Mcp-Param-*` request header is missing (-32020 "header mismatch"). The Inspector never sent that header. The SDK only mirrors inside `client.callTool()` (and skips it entirely in a browser environment), but the Inspector routes tools/call through `client.request()` for manual MRTR driving (#1704), so the SDK never mirrors for us — on any transport. On top of that, the remote (web) transport dropped per-request headers before they reached the backend's upstream fetch. - core/json/xMcpHeader.ts: port the SDK's non-public `buildMcpParamHeaders` + value encoding (Base64 sentinel for non-ASCII/whitespace/empty values) and add `mcpParamHeadersForTool`. - InspectorClient.attemptToolCall: on a modern connection, build the headers from the tool's declarations + call arguments and attach them to the tools/call request options. `Protocol.request` forwards `headers` to the transport and preserves them across MRTR retry legs, so the direct StreamableHTTP transport applies them. Fixes CLI/TUI. - RemoteClientTransport.requestSend + /api/mcp/send + RemoteSendRequest: forward per-send headers and apply them to the backend's upstream transport.send. The browser can't set cross-origin headers, but the Node backend can — where the Inspector's real request is issued. Fixes web. Verified end-to-end against the live GitHub MCP Server (modern HTTP). Closes #1846 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd * review: harden backend header allowlist, fix breadcrumbs, trim comments Address Copilot review on #1847: - server.ts: restrict client-supplied per-send headers to the `Mcp-Param-` prefix (string values only) via `mcpParamHeadersOnly` before applying them upstream, so the per-send channel can't inject arbitrary headers (e.g. Authorization). `settings.headers` remains the channel for configured upstream headers. Covered by new unit tests. - Fix stale `#1810` breadcrumbs → `#1846` in the two new tests. - Trim the verbose mirroring comments in inspectorClient/types/remote transport. - Drop the redundant base64-on-the-wire integration test (encoding is unit- tested; wire delivery is proven by the verbatim test), remove two unneeded test casts, and cover the boolean `true` branch. Not changed (declined with reason on the PR): buildMcpParamHeaders remains a faithful port of the SDK's helper and does not re-validate values against the declared JSON Schema type — arg types are enforced by schema validation on both sides, and matching the SDK's conforming wire output is the module's goal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd * review: mirror Mcp-Param-* on the task-augmented tools/call too Address the maintainer review on #1847: - `callToolStream` (the "Run as task" path) built its own request options and never mirrored, so a task-augmented `tools/call` on an annotated tool was still rejected by a strict modern server with -32020. It now mirrors like the plain path. - Extract the two blocks both `tools/call` entry points had duplicated — `convertStringToolArgs` (the string-arg coercion) and `applyMirroredParamHeaders` (the SEP-2243 mirroring) — so the paths can't drift again. That duplication is why the task path was missed. - Regression test: `callToolStream` against the modern `get_weather` server asserts the POST carries `Mcp-Param-City` (verified failing without the fix). - README: the modern-network showcase claimed mirroring is skipped in the browser so the web client gets -32020, and that CLI/TUI mirror. Both are now wrong — the Inspector mirrors itself, on every client. AGENTS: note that xMcpHeader.ts now also builds the wire headers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UkhhCHryVp5H3dqEnhwZ7t * docs: correct the stale "browser omits Mcp-Param-*" copy Both statements predate this PR and are now the opposite of true, per the wire capture on #1847 (proxy-observed upstream headers, web leg included). - ToolDetailPanel: the mirrored-headers note claimed the browser omits `Mcp-Param-*`. It now says the web client's headers are applied by the Node backend that issues the upstream request. - createRemoteFetch: the header comment claimed an `x-mcp-header`-annotated tool is "uncallable from the web client against a strict modern server" — the exact wrong conclusion to reach while debugging a -32020. It now records that the Inspector builds the headers itself, and that they do NOT travel this `/api/fetch` proxy path: they ride the transport's per-send `headers` through `/api/mcp/send`. Copy only — no test asserted the old note text (ToolDetailPanel.test.tsx matches the section title only). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UkhhCHryVp5H3dqEnhwZ7t --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: cliffhall <cliff@futurescale.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd |
||
|
Cliff Hall
|
40aaf47705 |
test: deterministic remote-server teardown to kill #1667 afterEach hang (#1763)
* test: deterministic remote-server teardown to kill #1667 afterEach hang The remote integration tests close their Hono HTTP server in afterEach with `server.close(cb)`, which waits for all keep-alive sockets to go idle before firing the callback. undici's global-fetch connection pool holds sockets open, and under the full parallel/instrumented `npm run coverage` load they may not go idle before the 30s hook timeout — intermittently failing the dead-transport tests in remote-auth-branches.test.ts. The crash-on-startup / dead-session tests are the worst case: they never disconnect, so the session and its dead subprocess transport linger when the server closes. Force the sockets closed with `server.closeAllConnections()` alongside close() so the callback fires deterministically regardless of parallel load, in both remote-auth-branches.test.ts and its sibling connect-crash.test.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * server: unref the abandoned-session sweep timer The transport-onclose path arms a 30s setTimeout to GC a session whose client never opens the events stream. That sweep is best-effort cleanup, not work the process must stay alive for — unref it so a pending timer can't keep the event loop (or a test worker) alive for 30s after everything else has finished. Also narrow closeAllConnections() behind an `in` guard in the two remote teardown helpers: ServerType is Server | Http2Server and the method is http1-only, so the direct call failed tsc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: extract shared closeHarnessServer teardown helper Address review feedback on #1763: the deterministic close()+closeAllConnections teardown (and its multi-line rationale) was duplicated across remote-auth-branches.test.ts and connect-crash.test.ts. Extract it to a shared closeHarnessServer(server) helper so the logic and its comment live in one place. 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
|
99ef731746 |
Refactor / justify codebase-wide 'as unknown as' casts (#1690) (#1756)
* core: refactor serverList.ts object↔Record casts into typed helper (#1690) Remove the four pre-existing `as unknown as Record<string, unknown>` / `as unknown as MCPServerConfig` casts in core/mcp/serverList.ts: - stripInspectorFields: rewrite as clone + `delete` over INSPECTOR_FIELD_KEYS. Every Inspector-extension key is optional on StoredMCPServer, so the result is still (a subtype of) MCPServerConfig with no cast. - oauth-strip in extractSecretsFromStored: `oauth` is optional, so a plain `delete stripped.oauth` is legal — drop the cast. - mcpConfigToServerEntries: route the one genuinely-needed widening through a single documented `toRecord(value: object)` helper. Taking `object` makes the single `as Record<string, unknown>` legal (no TS2352), so the double cast disappears from the call site. Net: zero `as unknown as` in serverList.ts; the one remaining structural cast lives inside the audited toRecord helper. Behavior-neutral — existing coverage (79 serverList tests) unchanged; full `npm run ci` passes. Closes #1690 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * core+web+test-servers: eliminate/justify remaining `as unknown as` casts (#1690) Codebase-wide follow-on to the serverList refactor: audit every `as unknown as` double cast outside tests and either remove it or ensure it carries a justifying comment (the review bar is "no *unjustified* double cast"). Removed (widened via a shared helper or narrowed to a single cast): - Promote the serverList `toRecord(value: object)` widening helper to core/json/jsonUtils.ts and reuse it at core/mcp/node/config.ts (normalize loop), core/mcp/remote/node/server.ts (pino level dispatch), and clients/web/src/App.tsx (window global lookup). - inspectorClient.rawWireRequest: type the frame as `JSONRPCRequest` and narrow only `params` with a single structural cast (the SDK's `_meta`-typed params is what blocked direct assignment). - toolOutputValidation: SDK `outputSchema` → `JsonSchemaType` is a single cast. - test-server-fixtures: the optional `task` handle is reachable with a single cast (target field is optional). Justified in place (genuinely unavoidable — private SDK internals, partial interface stubs, third-party/nominal bridges): the inspectorClient private-map accesses, resourceContext + tokenAuthProvider partial `OAuthClientProvider` stubs, the ext-apps peer bridge, modern/legacy `_requestHandlers` bypasses, and the happy-dom `matchMedia` test mock. Behavior-neutral. Full `npm run ci` passes (validate, ≥90 coverage gate, smoke, Storybook). Refs #1690 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * address @claude review nits: consistent cast justifications + drop reflow noise (#1690) - test-servers/src/modern-tasks.ts: add an inline justification above the `_requestHandlers` double cast, mirroring the sibling in composable-test-server.ts (was leaning on the function JSDoc only). - Annotate the partial-`AppBridge` mock double casts in the story fixtures (AppsScreen ×2, AppRenderer, InspectorView) so no non-test-file double cast is unjustified. - inspectorClient.ts: revert unrelated Prettier reflow hunks (protocolVersion, task-content array, timeout-reject wrap, reconnect guard) so the diff is the single rawWireRequest cast change only. Behavior-neutral (comment/format-only). `npm run ci` green apart from a known flaky redaction test whose assertion collided with a random request id. Refs #1690 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * actually revert the reflow noise this time (#1690) The reflow reverts in cc187ea were silently undone by a `prettier --write` I ran on the same file right after, so the four Prettier-only hunks from 6a3afa4a were still in the diff (caught in re-review). Revert them for real now — and without re-running Prettier on the file: - core/mcp/inspectorClient.ts: restore the protocolVersion line, task-content array, timeout-reject call, and reconnect guard to their origin/v2/main form, so the file's diff is only the single rawWireRequest cast change. - test-servers/src/modern-tasks.ts: restore the ModernTaskStatus union to its one-line form (the leading-`|` reflow was also Prettier churn), leaving only the new `_requestHandlers` justification comment. Formatting-only; `npm run validate` green. Refs #1690 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
|
fe295396af |
Extensions: advertised-extensions toggle in Server Settings (Phase 2 of #1633) (#1743)
* Extensions: advertised-extensions toggle in Server Settings (Phase 2 of #1633) Adds the debugging knob from #1633: users choose which extensions the Inspector advertises to a server, and a server may register different tools in response. Builds on Phase 1's registry + advertisedExtensions option (#1738). Settings + persistence (core): - InspectorServerSettings.advertisedExtensions + StoredMCPServer field, wired through serverList's read/write converters and INSPECTOR_FIELD_KEY_MAP (omit-when-empty, byte-stable round-trip). - validateSettings (remote PUT route): isBooleanRecord guard + carry a non-empty map; unknown/malformed rejected. - App.tsx threads savedSettings.advertisedExtensions into the client options. UI: - ServerSettingsForm renders an "Advertised Extensions" group of checkboxes driven by ADVERTISABLE_EXTENSIONS; a per-server override wins over the registry default. Modal folds a toggle into settings.advertisedExtensions. Test server + proof: - composable-test-server extensionGatedTools: a tool registered disabled and enabled on `initialized` iff the client declared the mapped extension (getClientCapabilities). Legacy stateful leg. Threaded through load-config/resolve-config; advertised-extensions-http.json showcase + README. - Integration test connects legacy with/without the tasks extension advertised and asserts the gated tool appears/disappears in tools/list — the acceptance criterion, proven end to end, not asserted. Tests: serverList round-trip (non-empty/absent/empty), ServerSettingsForm toggle rows, ServerSettingsModal fold, validateSettings accept/reject, and the integration gating test. npm run ci green (per-file ≥90 gate held). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Extensions: add #1739 proof screenshots (advertised-extensions toggle) End-to-end smoke test in the web client against the legacy advertised-extensions-http.json test server (port 3220): connecting with the Tasks extension advertised (default) lists echo + get_weather, unchecking the Advertised Extensions → Tasks toggle and reconnecting lists only echo — the server's tool registration changing on the client-declared extension. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Extensions: reconverge advertisedExtensions to no-override on default (review) Address Phase 2 review observation #1: toggling an extension back to its registry default now deletes the override key (and drops the whole map to undefined when it empties), so the on-disk `advertisedExtensions` reconverges to the byte-stable "no override" state instead of persisting a redundant `{ key: <default> }`. Matches the omit-when-default policy the other settings use. Only a value that differs from the default is persisted. Adds a modal test asserting the reconvergence (re-checking the sole disabling override drops advertisedExtensions to undefined). 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
|
ec185f1053 |
Logging: per-request log level on modern servers (era fork) (#1629) (#1724)
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
* Logging: per-request log level on modern servers (era fork) (#1629) On modern (2026-07-28) servers `logging/setLevel` and the session-scoped level are gone: the client opts into logs per request via the `io.modelcontextprotocol/logLevel` `_meta` key, and the SDK does not attach it automatically, so server logs are silently absent until stamped. Core (`InspectorClient`): - `setModernLogLevel(level | undefined)` / `getModernLogLevel()` store the opt-in level; `mergeMeta` stamps it on every outgoing request, gated on the negotiated modern era so legacy connections are untouched. - Cleared on disconnect so it can't leak into the next connection. Web UI: - `LogControls` forks on `protocolEra`: legacy keeps the `logging/setLevel` selector + Set button; modern shows a per-request opt-in Select (Off + levels, applied immediately) with copy explaining logs arrive on the originating request's stream. - Threaded `protocolEra` / `modernLogLevel` / `onSetModernLogLevel` through LoggingScreen and InspectorView from App. Tests: unit coverage for the modern LogControls/LoggingScreen fork; integration coverage proving the `logLevel` `_meta` is stamped on a real modern server once opted in (and not on legacy). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test-servers: logging era-fork showcase configs + proof screenshots (#1629) Add two manual-testing configs and README docs for the logging era fork: - logging-legacy-http.json: plain streamable-HTTP server with `logging: true` and a `send_notification` tool. The Logs tab shows the legacy session-scoped "Set Active Level" selector + Set button; calling the tool streams logs. - logging-modern-http.json: same, with `transport.modern: true`. Connect with Protocol Era = Modern and the Logs tab shows the per-request opt-in control; the client stamps `_meta["io.modelcontextprotocol/logLevel"]` on every request (visible in the Network tab request body). pr-screenshots/ documents the verified behavior end-to-end against both real servers: the legacy control with logs streaming, the modern per-request control, and the modern tools/call request body carrying the stamped logLevel _meta. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test-servers: send_notification emits via per-request notifier so modern logs stream (#1629) The `send_notification` tool sent its `notifications/message` through the global `server.server.notification()`. On the modern (2026-07-28) stateless leg that isn't tied to the in-flight request, so the SDK answered the tool call with plain JSON and dropped the log — nothing reached the Inspector's Logs panel. Prefer the per-request `extra.sendNotification` (rebuilt from `ctx.mcpReq.notify`): scoped to the current request, it makes the SDK upgrade the response to SSE and stream the log back on the originating request's stream. Falls back to the global notifier (legacy path / no per-request context). Verified end-to-end: opting in via the Logs-tab "Log Level per Request" control and calling send_notification now streams the entry on the modern server; legacy behavior unchanged (server-notification integration tests green). README documents the modern logging showcase and the caveat that this test server streams whenever a tool emits — the SDK beta strips the per-request envelope from the handler context, so the tool can't gate on the client's logLevel opt-in (the client-side `_meta` stamp #1629 owns is still fully visible in the Network tab). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test-servers: send_notification uses SDK request-scoped log() → spec-faithful modern gating (#1629) The previous fix routed `send_notification` through `extra.sendNotification` (`ctx.mcpReq.notify`), which streams on the modern leg but emits unconditionally — the log showed up even when the client hadn't opted in via the per-request `logLevel`. The SDK already provides the correct primitive: `ctx.mcpReq.log(level, data, logger)` (added in `McpServer.buildContext`). It applies the era-correct threshold gating itself — on the modern (2026-07-28) leg it reads the per-request `logLevel` opt-in from the request envelope and drops the message when the client didn't opt in or the level is below the requested severity; on legacy it honors the session level from `logging/setLevel` — and emits through `notify`, so the modern response still upgrades to SSE and the log rides the originating request's stream. Surface it as `extra.log` (`toHandlerExtra` maps `ctx.mcpReq.log`) and have `send_notification` prefer it, falling back to the global notifier for callers without per-request context. Verified end to end: opting in via the Logs-tab "Log Level per Request" control stamps `logLevel` in the request `_meta` and streams the log; setting it back to Off omits the key and the server gates the log (plain JSON response, no notification). Legacy server-notification integration tests still green. (No SDK change needed — the earlier apparent gap was our test server using the wrong low-level API.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * web+core: add "Log Level per Request" as a per-server setting, defaulting to on (#1629) Adds the modern per-request log level as a persisted Server Settings option (Options → "Log Level per Request", below Protocol Era), so a server can opt into logs by default instead of the Logs-tab control starting at Off. - types.ts: `ModernLogLevel` (`"off"` | the 8 levels), `DEFAULT_MODERN_LOG_LEVEL` = "debug" (default-on so a modern connection surfaces server logs out of the box), `MODERN_LOG_LEVELS`, and an `isModernLogLevel` guard. Field added to `InspectorServerSettings` and `StoredMCPServer`. - serverList.ts: round-trip the field (lift on read, omit-on-default on write, drop unknown literals) mirroring `protocolEra`; add to the persisted field allowlist. - remote/node/server.ts: validateSettings accepts/validates it and the normalize path drops a malformed literal. - inspectorClient.ts: the constructor seeds `modernLogLevel` from the setting (`"off"` → not opted in), so a modern connection stamps the level from the first request without UI interaction. mergeMeta still gates on the modern era. - ServerSettingsForm/Modal: a Select (Off + 8 levels) with a modern-era description. App seeds the live Logs-tab control from the setting on connect so the two stay in sync. Tests: form Select (default/onChange/off/reflect), modal handler, serverList round-trips, route validation (accept/reject/drop), and constructor seeding (seeded level stamped from the first request; "off" opts out). The prior modern-era "stamps once set" test is updated for the new default-on behavior. Coverage gate green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Fix CI + review: format ServerSettingsForm, drop stray network export, broaden ignore - Prettier-format ServerSettingsForm.tsx (the format:check failure that broke CI — my pre-push `npm run format` from the repo root was a silent no-op since there is no root format script; formatting is per-client). - Remove `inspector-network-logging-modern-2026-07-19T15-11-27.013Z.json`, a Network-tab export debugging artifact accidentally committed in ca2aa526 (flagged by review). - Broaden the .gitignore debugging-artifacts section with `/inspector-network-*.json` so future network exports are ignored (the existing `/inspector-history-*.json` didn't match the network export naming). Verified with the full `npm run ci` (validate → coverage → smoke → storybook), green across all clients. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: add modern-era log delivery + gating e2e tests (#1629) The modern-era suite covered the client-side `_meta` logLevel stamp but not that a log actually flows back — the legacy suite already has that (`should receive server-initiated notifications via SSE/streamable-http`), so the modern side was the gap. Add two live e2e tests against a modern (`createMcpHandler`) server serving the `send_notification` tool with `logging: true`: - opted-in (server setting = a level): calling the tool delivers a `notifications/message` over the request's SSE stream, with the right level/logger/data. - not opted in (setting "off"): the tool still returns a result, but the server gates the log — no `notifications/message` is received. This exercises the full path end to end: the client seeding + `_meta` opt-in, and the server's `send_notification` → `ctx.mcpReq.log` era-correct gating. Verified with the full `npm run ci` (green across all clients). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * web: hide the modern "Log Level per Request" setting on legacy connections (#1629) The per-request log level is meaningless on a legacy server (which uses logging/setLevel), so hide it in Server Settings when the era is legacy: - protocolEra pinned to "legacy" → hidden (no connection needed), or - protocolEra "auto" that negotiated legacy at connect time → hidden. It stays visible for a pinned "modern" server, and for an "auto" server that is either not yet connected (era unknown) or resolved to modern. Threads the negotiated era down: App passes `negotiatedEra` to ServerSettingsModal only when the settings target is the live-connected server (`connectionStatus === "connected" && settingsModalTargetId === activeServerId`); the modal forwards it to ServerSettingsForm, which gates the control on `configuredEra === "modern" || (configuredEra === "auto" && negotiatedEra !== "legacy")`. Tests: visibility matrix on the form (legacy hidden; auto+legacy hidden; auto+unconnected and auto+modern and modern shown) and a modal hide case; the existing modern-log tests now base on a modern-era settings object. Verified in the browser (legacy hides, switching to modern reveals it reactively) and with the full `npm run ci` (green across all clients). 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
|
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> |
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
621b712229 | ||
|
|
eb41185c37 | ||
|
|
d57801d7b3 | ||
|
|
cf856c740c | ||
|
|
1e501fa5cb | ||
|
|
e234343702 | ||
|
|
de6960ec5a | ||
|
|
baefc96806 | ||
|
|
a452305204 | ||
|
|
d848300077 | ||
|
|
5ba2fadd3b |
||
|
|
85c5206895 | ||
|
|
65fd2b9e35 | ||
|
|
6af849e217 |
||
|
|
d9b5f4f8a7 | ||
|
|
e2fb78d999 | ||
|
|
fafc8367b8 |
||
|
|
fa441da85f |
||
|
|
ec40cf5531 |
||
|
|
531cf84e60 | ||
|
|
9a084f49e5 | ||
|
|
56d8644032 | ||
|
|
b33532c024 | ||
|
|
af3d940e78 | ||
|
|
15e9260484 | ||
|
|
5ef3572578 | ||
|
|
78a65f7c20 | ||
|
|
26d2f58784 | ||
|
|
339aea3729 | ||
|
|
37faf1be88 | ||
|
|
b143dd6cbd | ||
|
|
bc408f5869 | ||
|
|
e58c80440b | ||
|
|
3dfac1c3db | ||
|
|
4e4aa49db1 |
||
|
|
d830213796 | ||
|
|
519ecf49ee | ||
|
|
364d8c9dbc | ||
|
|
e6e4600a73 | ||
|
|
b25ece8ded | ||
|
|
9e074aa6f2 |
||
|
|
7a74b5eba1 | ||
|
|
9644ac7fe8 | ||
|
|
5d91d4acd0 |
||
|
|
40aaf47705 |
||
|
|
99ef731746 |
||
|
|
fe295396af |
||
|
|
320daf2f67 |
||
|
|
ec185f1053 |
||
|
|
b3b04c6939 |