Two findings, both valid.
`onCallTool`, `onGetPrompt`, `onReadResource` and `onOpenApp` await
`handleCommandScopedAuthRecovery` from inside their catch blocks, and a
rejection thrown from a catch is not caught by that same catch — so it
escapes the handler. That helper awaits `checkAuthChallengeSatisfied` and
`pushRemoteAuthState`, both of which reach the backend and can reject, so
the bare `void` on those four could still leak.
Rather than patch the four and keep a comment enumerating which handlers
are safe, all seven wrappers now terminate the same way: a reporting
`.catch` via `reportDispatchFailure`. Whether a handler "owns its
failures" turned out not to be reliably knowable by reading — the
previous two attempts at that judgement were both wrong — so the code no
longer depends on getting it right. A handler that does surface its own
failure resolves and never reaches the catch, so nothing double-toasts.
The reorder callback lifted out of the JSX had no App-level coverage.
The `InspectorView` double grows a reorder control, with tests for both
forwarding the ordered ids and reporting a rejected reorder.
Adds the auth-recovery escape test the review asked for. All four new
rejection tests were confirmed to fail against the bare-`void` code, so
they pin the fixes rather than merely passing beside them.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three findings, all valid on verification.
`onToggleConnection` and `onDisconnect` do not own every rejection, so a
bare `void` on them turns an ordinary Connect/Disconnect click into a
global unhandled rejection with nothing shown to the user. The toggle has
three escape paths outside its own try/catch — awaiting
`initialConfigSettledRef`, constructing the client via
`setupClientForServer`, and the already-connected disconnect branch,
whose `try/finally` runs `finalizeExplicitDisconnect()` and then lets a
failed transport close propagate — and `onDisconnect` is that same
`try/finally` on its own. Both are now terminated with a `.catch` that
toasts the failure.
The blanket comment claiming all seven wrappers own their failures was
wrong. The five command handlers (`onCallTool`, `onGetPrompt`,
`onReadResource`, `onCancelTask`, `onOpenApp`) do each end in a `catch`,
so `void` stays correct for them; the comment now splits the two groups
and says which is which.
Adds the two tests the review asked for, covering the rejected-close path
through both the toggle and the explicit Disconnect. Each fails against
the previous `void` code, so they pin the fix rather than merely passing.
The `InspectorView` double grows a `disconnect` button, since the
standalone `onDisconnect` was otherwise unreachable from it.
Also restores the `PropOverrides` doc example, which the scripted
regrouping had rewritten inside its own JSDoc into a triple-nested shape
that would not type-check if copied.
Signed-off-by: cliffhall <cliff@futurescale.com>
`<InspectorView>` took ~130 flat props, which made ~170 of App.tsx's JSX
lines a prop wall rather than a component tree. It now takes 12 domain
props and the call site is 14 lines.
Two conventions keep the change reviewable. No field is renamed — every
field inside a bundle carries the exact name it had as a flat prop, so
this is a regrouping rather than a rename. And the bundles stop at
`InspectorView`: it destructures each one back into the same locals its
body already used and passes the same individual props down, so nothing
below the view knows they exist.
Three props have no single screen and are placed deliberately:
`onCompleteArgument` / `completionsSupported` are read by both the
Prompts and Resources screens, so they sit in `connection` rather than
being duplicated; `malformedListItems` is one array three screens filter
for their own entries, so it sits in `shell`; `erroredServerId` /
`connectedServerId` are connection outcomes the Servers screen renders,
so they sit in `connection`.
Every multi-line closure declared inline in the JSX is now a named
`useCallback` above the return, including `onServerReorder`'s `.catch`
and the seven `void`-discarding wrappers, which carry their
`no-floating-promises` justification once rather than at each call site.
The `InspectorView` double in App.test.tsx is now typed with the real
`InspectorViewProps` instead of a hand-written structural mirror, so it
cannot drift from the component it stands in for. Stories keep one
constant per bundle, since Storybook merges args only at the top level.
No behavior change.
Signed-off-by: cliffhall <cliff@futurescale.com>
Phase-2 step 4 of the App.tsx decomposition (#2155, under #2129/#2126).
Every command the screens issue against the live server moves into
`useServerCommands`: calling a tool, getting a prompt, reading and
subscribing to a resource, completing an argument, cancelling a task or
a call, setting either log level, refreshing or paging a list, and
flipping the pagination mode. All of them route through the
command-scoped auth recovery `useOAuthRecovery` publishes, which is why
this cluster cannot precede the OAuth one.
The three in-flight result panels move with them, behind a small
`useResultPanels` in the same file — split out only because of call
order, the same reason `useHandshakeTelemetry` sits beside
`useConnectionLifecycle`: `useOAuthRecovery` drops those panels on a
session reset and routes a step-up failure into whichever one issued
the command, and it runs before the commands hook.
The move is inert — no behavior change, #2095's per-server pagination
override included.
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>
Phase 2 step 3 of the App.tsx decomposition (#2154, under #2129/#2126).
Moves the whole connection cluster out of App.tsx and into
clients/web/src/hooks/useConnectionLifecycle.ts: setupClientForServer
(the single place a connection is configured), onToggleConnection,
onDisconnect, onReauthenticateFromBanner, the deep-link auto-connect
effect and its three phase latches, the connected/teardown/disconnect
effects, connectErrorMessage, and the session-scoped reset a disconnect
triggers. Handshake telemetry rides along as useHandshakeTelemetry,
split out only because useOAuthRecovery runs first and stamps the same
ref.
No behavior change. App.tsx goes 3,039 -> 2,412 lines.
Per #2129, resetSessionScopedUiState takes a narrow SessionResetSurface
- one reset() per owner - rather than every phase-1 setter, so the hook
does not end up depending on everything phase 1 extracted. The two
log-level operations stay App-owned because the connect path seeds the
same state.
Closes#2154
Signed-off-by: cliffhall <cliff@futurescale.com>
Step 5 of the phase-2 App.tsx decomposition (#2129, under #2126): the two
sandbox bridge factories, the renderer handle, and the app-rendered
elicitation controller move out of App.tsx into a hook of their own.
Nearly a leaf — it reads the active client and the resource listing and
calls nothing back — so `getListedResourceMeta`, `publishDocument` and
`listedResourcesRef` become internal to the hook rather than App.tsx
locals. The hook is called where `resources` is derived, which is what
lets it own the latest-listing ref write too.
`sandboxUrl` and `initialConfigSettledRef` deliberately stay in App.tsx:
whether the sandbox exists decides whether the client advertises the
nested MCP Apps `elicitation` capability, and that is a
connection-lifecycle question rather than an Apps one.
Inert by construction: no behavior change, and the extraction is covered
100% on all four dimensions plus the existing `smoke:web:app` and
`smoke:web:elicit`, in both Chromium and Firefox.
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>
- Publish `setupClientForServerRef` from a layout effect rather than during
render. React runs every layout effect before any passive effect of the same
commit, and the /oauth/callback effect is passive, so the seam is still
published before its first read — without the render-phase ref mutation the
compiler rule rejects. This was the one `react-hooks/refs` finding the PR
introduced, so its suppression is gone rather than deferred to #2161 under a
justification that did not describe it.
- Narrow the hook's `fetchLogRef` option to `FetchRequestSource`, the
`getFetchRequests` slice it actually consumes, mirroring
`InspectorClientEventSource` in useProgressToasts. That removes the test
harness's `as unknown as` cast, and the narrowed contract is real: it
immediately rejected the loosely-typed fixture, which is now a properly typed
`FetchRequestEntry`.
- Make the test harness hold both refs across renders and publish them from a
layout effect, as App.tsx does. The deferred-publication test previously
passed because `Probe` built a fresh ref each render and the rerender also
changed `servers`; it now asserts what the guard is actually for — a run that
cannot exchange must not spend the single-use code — and re-triggers on the
`servers` change that carries it in production.
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>
Phase 2 step 2 of the App.tsx decomposition (#2129, under #2126). App.tsx goes
from 4,267 to 3,178 lines; no behaviour change.
The new hook owns the step-up slot and its retry operation, the re-auth banner,
the deferred background-tab recovery, the redirect plumbing (webOAuthStorage,
sessionStorageAdapter, onBeforeOAuthRedirect, prepareOAuthRedirect), the
command-scoped recovery wrappers, the four OAuth effects, the abandoned-redirect
check, the /oauth/callback completion, the Connection Info OAuth details, and
the clear/finalize paths. All five once-only latches move with it unchanged.
Three seams could not be a straight move:
- useSessionRef no longer takes pendingStepUp/pendingReauth. They are this
hook's own state, so App.tsx would have had to hold them purely to hand them
back; instead the owner mirrors them into the ref in an effect of its own,
following the shape activeServerName already had.
- setupClientForServer is injected through a ref, because it reads
onBeforeOAuthRedirect and sessionStorageAdapter out of this hook and so
cannot be declared before it. The callback effect reads the ref before
burning its one-shot latch.
- The two clear-OAuth modal adapters and onReauthenticateFromBanner stay in
App.tsx: they bind App-owned state, and the latter drives a reconnect, which
step 3 owns.
Dropping ~1,100 lines took App.tsx under the React Compiler's bail-out, so the
compiler-backed react-hooks rules now analyse it and report nine pre-existing
findings that were previously invisible. Each is suppressed with a narrow
justified disable naming #2161, which is filed to resolve them; the one new
finding (exhaustive-deps on setReAuthBanner) is fixed rather than suppressed.
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>
Step 1 of phase 2 of the App.tsx decomposition, landed first and alone
because it is the seam the remaining clusters are extracted onto.
App.tsx kept six "latest value" refs so long-lived callbacks could read a
current value without listing it as a dependency and being re-created on
every change: `activeServerNameRef`, `activeServerIdRef`, `serversRef`,
`inspectorClientRef`, `pendingStepUpRef` and `pendingReauthRef`, plus the
three passive effects that synced them. `useSessionRef` replaces all nine
with one hook returning one stable ref onto a `SessionSnapshot`.
This is not a store: nothing renders off it, nothing subscribes to it, and
mutating it never schedules a render. It is not the prop-to-state sync
AGENTS.md forbids either — that rule is about `setState` in an effect, and
a ref write produces no render at all. It is exactly the pattern the file
already used, written once instead of six times.
What it buys the later steps is a *stable* dependency: a hook taking
`sessionRef` does not drag the other clusters into its argument list, so
`useOAuthRecovery`, `useConnectionLifecycle` and `useServerCommands` can
be extracted one PR at a time.
Behavior is unchanged:
- The snapshot is seeded from the first render's values, matching what
each `useRef(value)` seeded.
- `activeServerName` keeps its one non-uniform rule — it holds the last
server that *was* active and is never cleared, because a transport
crash clears `activeServerId` before the failure toast is raised. It is
derived from `servers`/`activeServerId` rather than taking the
`activeServer` memo, which is the same value by definition and is what
lets the hook be called ahead of that memo.
- The two pending-* syncs already ran ahead of the only effect that reads
them, so merging them changes no effect's view. The other four synced
below every effect that reads them, so hoisting that sync changes none
either.
- The sync effect is deliberately un-gated rather than carrying a
dependency list: re-assigning unchanged values costs nothing and
removes a place for a newly-added field to be forgotten.
`sessionRef` is declared ahead of its first reader because a hook return
is not provably stable to `react-hooks/exhaustive-deps` — consumers must
list it, and a later declaration would put every one of them in the
temporal dead zone. Listing a ref that never changes identity re-runs
nothing.
The inline `pendingStepUp` state shape is named `PendingStepUp` and moved
beside `StepUpSource` in `utils/stepUp.ts`, so the snapshot can name it
without importing from the component that owns the state.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MZdpUwhHf943FoprcLVzEm
Signed-off-by: cliffhall <cliff@futurescale.com>
Phase 1 of #2126. Six hooks move out of `clients/web/src/App.tsx`, taking
its component body from 4,891 to 4,267 lines with no behavior change:
- useThemeToggle — the light/dark toggle
- useTabUiState — the nine per-screen `ui` objects, the active tab,
the pinned-Protocol ids, and the session reset
- useInspectorStores — the twelve state managers as one create/destroy
lifecycle, their `core/react` hooks, and the
pagination-mode selection
- useExportActions — the eleven Clear / Export / Replay handlers
- useProgressToasts — `notifications/progress` toasts
- useTaskToasts — task-status toasts, the cancel confirmation, the
taskId -> progress map, and the in-flight
tool-call taskId ref
Holding the stores in one slot collapses `setupClientForServer`'s and
`onConfirmRemove`'s dependency arrays from twelve entries to two stable
callbacks; `destroyStores` reads the live set through a ref mirror rather
than the state it also writes.
All six hooks are at 100% on all four coverage dimensions, no `v8 ignore`
needed. Also refreshes the now-stale "~4.5k-line" description of App.tsx
in AGENTS.md and the coverage `include` comment.
Signed-off-by: cliffhall <cliff@futurescale.com>
Phase 0 of #2126. Lines 239-668 of clients/web/src/App.tsx were already at
module scope — helpers, formatters, constants, and the toast subcomponents,
none of which close over component state — so the whole block moves verbatim
into files. App.tsx drops from 5,303 to 4,892 lines with no behavior change.
Placement follows the lib-vs-utils rule in AGENTS.md: pure computation to
utils/, anything touching the environment to lib/.
EMPTY_SETTINGS and the two loose toast ids deliberately do NOT land in a new
top-level src/constants.ts, which the issue suggested: the web coverage
include is a whitelist naming components/hooks/theme/lib/utils/server, so a
module there would have fallen out of the >=90 gate silently. They go under
utils/ instead.
ReAuthBannerBar is in the block but not in the issue's table — it was added
after the issue was written (#2108). It moves to components/groups/ReAuthBanner/,
beside the banner it wraps.
Every new module is at 100% on all four coverage dimensions. Two v8 ignores,
each justified in place: getAuthToken's typeof-window guard (a happy-dom
inherent path) and replayProtocolRequest's provably-dead default case.
Signed-off-by: cliffhall <cliff@futurescale.com>
The rollback's explanation still described the removed app-wide boolean and
claimed the override was applied only for the active server. That is now the
live client's rule alone: the override is keyed by this write's server and is
only ever displayed while that server is active, so it is recorded whatever
is active by the time the rejection arrives.
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>
The displayed `paginatedLists` value falls back to the active `servers`
entry, and that list only advances when a `GET /api/servers` succeeds — so
once a list read fails it keeps describing disk as it was before the writes
made since. The optimistic override papered over that, but it was a single
app-wide slot cleared on every change of the *active* entry, so switching to
another server and back dropped it and the UI fell back to the stale entry.
Two things then disagreed, and the one the user could see was the wrong one:
the toggle read `off` while disk, the write tracker, and the client just
built from it all said `on`; and because the client skipped its connect-time
aggregate walk, the all-pages UI rendered an aggregate that was never
fetched, with no Load-next-page control to fill it.
Hold the override per server instead, in `usePaginatedListsOverride`, paired
with that server's entry as of when it was recorded and believed only while
the list still carries that same entry object — identity, not equality, so a
read reporting the value the override replaced supersedes it too. A server's
override then survives a switch away and back, and is still superseded by
the first successful read that rebuilds its entry.
The read cannot instead move to `lastPersistedSettings.resolve(id)`: that is
called during render, and it both reads refs and prunes superseded records.
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>
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>
Unrelated to #2108's diagnostics work, folded in at the maintainer's request
rather than tracked separately.
The sticky full-bleed bar cost the whole view a band of vertical space for what
is a notification about one server, and it pushed down the monitoring sidebar
that an OAuth failure now opens. Every corner is spoken for — the toast stack
owns bottom-right, the right edge is the sidebar whose toolbar an anchored card
covers, top-left is the Servers header — so it is centered instead, which is
the one placement that reads as addressed to the window rather than attached to
the wrong panel.
Deliberately WITHOUT an overlay. This is a notification, not a decision that
must be made now, and blocking the page would force a choice between reading
the requests that explain the failure and keeping the affordance — dismissing
is not free, since "Authorize again" also clears the stale OAuth state, which a
plain reconnect does not do.
Kept as the existing declarative component rather than routed through Mantine's
`notifications`: that module is mocked in App.test.tsx, so switching would have
broken the ten tests asserting on this banner's text and its action button,
including the lost-authorization-state coverage. The message now stacks above a
right-aligned action, which a ~420px card needs.
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 4 raised that the callback catch leaves `activeServerId` set: the
disconnect there emits only on a status change, and a rejected token exchange
throws inside `completeOAuthFlow` before the reconnect, so the client is still
at its initial "disconnected" and the listener that would clear it never fires.
That is accurate, and clearing it is nonetheless wrong. "Authorize again" on
the re-auth banner hands `clearServerOAuthState` the live client only when the
banner's server is the active one; releasing it here passes `null` instead and
the stale tokens are never cleared from the client holding them, which is the
one thing that recovery exists to do. Confirmed by trying it — the existing
lost-authorization-state test fails on `clearOAuthTokens` never being called.
So the retention is now asserted rather than incidental, with the reason at
both the code and the test, and the InspectorView double surfaces `activeServer`
so a later cleanup of the apparent leak fails loudly instead of quietly
breaking the recovery.
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 3.
Both catches report a failure without ending the attempt, and in each the
preceding rejection is an auth-recovery error — which deliberately holds the
status at "connecting" rather than moving it to "error". So the toggle spins
and the active-server lock is never released:
- the auth-recovery arm, when `checkAuthChallengeSatisfied` is what rejected
(the retrying `connect()` does reach "error", so only this half was stuck);
- the callback leg, whose `resumeAfterOAuth` carries a reconnect that can
reject the same way. Disconnect ahead of the EMA guard there, since a stuck
session is worth clearing whichever way the error classifies.
The fetch log survives a disconnect, so the Network diagnostics this issue is
about are unaffected. Both teardowns are asserted by the tests that already
cover those arms.
Also fixes a stale comment left by round 2: the arming-hooks type carries four
hooks, not three.
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 2.
The flag in the resume catch sat above the EMA guard, so an unconfigured
enterprise client was marked a failed connect — which contradicts the round-1
argument for leaving EMA unflagged in the two connect-path arms. Moved below
the guard so all three agree, and still above the classification fan-out so
every arm that is a real failure carries it.
Adds the two tests the round flagged as missing: an OAuth storage `load()`
that rejects (spied on `RemoteOAuthStorage.prototype`, so every other test
keeps the real storage), and a *rejecting* challenge check — the distinct
control-flow decision introduced last round, where a broken check is surfaced
rather than treated as an unsatisfied challenge and used as grounds to navigate
the page away. That one asserts no resume snapshot was written, which is what
`prepareOAuthRedirect` would have done first.
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 1.
The satisfied-challenge arm re-connects from inside the outer `catch` with two
unguarded awaits, so a rejection there escaped `onToggleConnection` entirely —
an unhandled rejection with no toast, no red border and no sidebar, which is
this issue's failure mode in its most invisible form. Surface it as the failed
connect attempt it is. A throw from `checkAuthChallengeSatisfied` lands there
too rather than falling through to the redirect: it is not the same as the
challenge being unsatisfied, and navigating the page away on the strength of an
error would bury it.
The provider-error and OAuth-storage-load arms return before a client is
rebuilt, so they raise the red border and deliberately not the sidebar — the
diagnostic is the provider's `error` param (already in the banner) or a local
storage failure, neither of which is in the network log. Said so at both sites
and on the test, which asserts the flag and nothing more on purpose.
Also drops the three `as unknown as` casts on the test-only arming hooks for a
single intersection cast that keeps the module's real shape checked, and turns
the armed connect rejection into a FIFO queue so a test can arm the two
consecutive connects the recovery retry makes.
Signed-off-by: cliffhall <cliff@futurescale.com>
The failure column was gated on the `"error"` connection status, which an
OAuth failure never reaches: the parent drives the auth legs itself and tears
the client down on failure, so the session settles at `"disconnected"`. The
sidebar therefore stayed shut on exactly the failure the Network tab exists to
explain — the case #2048 was closed on.
Gate it on `erroredServerId` instead, the parent's canonical "this server's
last connect attempt failed" flag, and set that flag on every OAuth
connect-attempt failure: the pre-redirect discovery/DCR leg, the callback
leg's token exchange, an OAuth-storage load failure, and a provider that
redirects back with an error instead of a code.
Nothing else was needed to make the requests reachable. `auth` fetches are
already tracked into the Network store, which does not clear on disconnect and
is restored from the session on the callback leg, so discovery, DCR and the
token exchange are all present. Tab selection stays content-gated rather than
hard-coded, so Network leads because it is the one that captured something.
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
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>
It does not distinguish "no entry for this URI" from "the list has not
arrived yet"; both yield no hints, which is what a host with no listing
carrier gets anyway. Say so at the lookup, with why blocking on list
readiness is worse: for a server advertising no `resources` capability,
or one whose list errored, that request never resolves, so the app would
never render at all.
Signed-off-by: cliffhall <cliff@futurescale.com>
The sandbox proxy's buildAllowAttribute() tests each permission key for
truthiness, so an untrusted `_meta.ui.permissions` bag could stand a
non-boolean in for a grant — `{ camera: "false" }`, a plausible way for a
server to mean "off", reads as on. Nothing screened it, because before
the `_meta.ui` fix no permissions reached the proxy at all.
Add approveSandboxPermissions alongside approveCspSources: keep only the
four keys the proxy knows, and only where the value is the spec's empty-
object marker or `true`; drop anything else (warning on a malformed
value, silently on an explicit `false`) and return undefined when nothing
survives. Both the notification and the hostCapabilities echo now carry
the approved set, so the app sees what was granted.
Also document what the listing lookup can and cannot see under
`paginatedLists`.
Signed-off-by: cliffhall <cliff@futurescale.com>
The read RESULT's own `_meta` is not a documented carrier for the sandbox
hints. ext-apps documents the `resources/list` entry's `_meta.ui` as the
static default a host reviews at connection time, with a read content
item's own `_meta.ui` taking precedence (McpUiAppResourceConfig). Swap
the fallback accordingly and thread the listing through a new optional
`getListedResourceMeta` dep, wired in App.tsx from the resources list.
Signed-off-by: cliffhall <cliff@futurescale.com>
Both lines changed the same two settings-write paths in App.tsx:
- #2089 (v2/main) rewrote the pagination toggle and the settings-draft
flush around `useLastPersistedSettings`, so each write announces itself
with `begin` and confirms with `landed`/`failed`.
- #1914 (this branch) made a failed post-write list reload reject with
`ServerListReloadError` — the PUT landed, only reading the list back
failed.
Resolved by treating a `ServerListReloadError` as a *landed* write in the
new model rather than a failed one: both paths now settle it exactly as
the success path does (so it becomes the rollback baseline and re-applies
itself when settled) and only the toast says the reload failed. Rolling
it back would put the UI, the live client and the modal's seed on a value
disk no longer holds — the very contradiction #2089 exists to prevent.
The two suites' new tests were interleaved by the conflict and are both
kept, plus one covering the combination: a modal save whose reload failed
is recorded as the landed write instead of rolled back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 10 on #2091. Connecting is where the whole connection is
configured, and it read `server.settings` straight off the list entry —
so every connect undid what the tracker exists to preserve. A save that
landed while list reads were failing, including one made from the
settings modal for a server that was not connected at the time, was
silently reverted by the next connect: headers, roots, timeouts, the
Network log size, all rebuilt from the frozen entry.
`setupClientForServer` now resolves through `lastPersistedSettings`,
which covers every construction path (connect, reconnect, OAuth resume)
since they all pass through it.
The round-9 test also claimed to cover the log size and did not: it
asserted the settings object and the roots only, so a regression that
stopped restoring `maxFetchRequests` would have passed. It now asserts
the tracked fetch-log instance on both sides of the rejection.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 9 on #2091, all three findings.
Every live-apply now goes through one `applyLiveServerSettings`: the
settings object, the Network log size, and the roots diff together. A
close with a pending debounce applies the draft to all three, so
reconciling only `setServerSettings` when that save rejected left the
server advertising roots that were never persisted and the log sized for
them.
The "this server's draft is unpersisted" flag moves from a single id in
`App.tsx` into the tracker, which already orders writes by issue and
keeps state per server. One id could be overwritten by a failure for a
different server, or cleared by a stale success, either of which made a
close apply a rejected draft.
And when nothing has ever landed for a server, close falls back to the
empty settings the toggle rollback uses rather than to the draft — the
old fallback re-applied the very values that had just failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 8 on #2091, both findings — the two remaining places the
stale `servers` entry and the unpersisted draft still leaked.
The modal draft was seeded straight from `servers`, and the draft is the
input to the *next* write: after a write landed whose list reload failed,
opening the modal and saving any unrelated field sent the superseded
value back to disk. It now seeds through the tracker, which falls back to
the entry when nothing has landed, so the ordinary case is unchanged.
`useSettingsDraft` also keeps the draft when a save rejects, and closing
the modal unconditionally pushed that draft into the live client — so the
client returned to the value that had just failed to reach disk, undoing
the rollback that reported the failure. The server whose last modal save
rejected is now remembered, and close applies what landed for it instead
(the roots diff and the fetch-log resize included). The flag clears on
the next save for that server that lands, and on the close that acts on
it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 7 on #2091, both findings.
Every settings continuation pushed through the `inspectorClient` it
closed over, and `onToggleConnection` rebuilds the client on each
(re)connect — so a write outliving a reconnect passed the server-id
guard and then wrote to a destroyed instance, while the replacement,
built from the stale list entry, kept the value the write replaced. The
live client is now mirrored in a ref alongside the active server id, and
all three continuations apply through it.
The modal save's failure path also stopped at taking itself out of
flight. A write that landed while it was pending had been told it was
not settled and applied nothing, so with the save failing there was
nothing left to apply the value that is actually on disk. It now
resolves and re-applies the baseline, the same as the toggle's failure
path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 6 on #2091. The modal writer recorded its write but threw
away the settled result, so the mixed-writer overlap was still broken:
the modal's flush is in flight with `paginatedLists: true` when the
sidebar toggle fails and rolls back to the stale entry's `false`, then
the save lands. It is the settled state, but nothing re-applied it — so
with the list reload also failing, disk held `true` while the override
and the live client sat on `false`.
Both writers now share the reconciliation as well as the record: a
settled write for the still-active server re-applies its value to the
override and the live client.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 5 on #2091, both findings.
The other overlap order was still wrong. Write 1 (`true`) is in flight
when write 2 (`false`) fails, so the rollback resolves to the stale
entry's `false` — correct at that instant, nothing had landed. Write 1
then lands `true` and only touched refs, so if its own list reload had
failed too, no render would ever correct the UI or the live client while
disk held `true`.
Writes now report both outcomes: `landed` returns whether this write is
the settled state for its server — recorded, with nothing later still in
flight — and `failed` takes a write out of flight so an earlier one that
lands afterwards settles. The toggle re-applies the settled value to the
override and the client, gated on its server still being active.
Serializing per server was the alternative and was not taken: it makes
the modal's debounced saves queue behind each other and changes the write
path to fix a UI-reconciliation concern, and it still could not order two
requests already in flight.
The override reset also moves from a `useEffect` to `useValueChange`,
which is what this repo requires for resetting state from render data —
the entry is referentially stable between reads, and keying on it alone
subsumes the persisted boolean and the server id.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
Review round 4 on #2091, three findings.
A rollback sets the override to a concrete value rather than clearing
it, so the effect that drops it could not key on the persisted boolean
alone: if a later read reported the value the override replaced — an
edit made outside the Inspector overtaking the write — neither that
boolean nor the server id changed and the UI stayed stuck on the
override. It now keys on the active `servers` entry too, which a
successful read always rebuilds.
The override is also a single app-wide boolean and the live client
belongs to whichever server is connected now, so a rejection arriving
after the user switched servers rendered one server's value on another.
The UI rollback and the client push are now applied only while the
failing write's server is still active; the toast is not gated.
The settings-modal writer had no coverage: `useSettingsDraft` is mocked
for `App.test.tsx`, so its `onPersist` never ran and dropping
`begin`/`landed` there would have gone unnoticed. A test now invokes the
callback the App handed the hook, lands a save, and asserts a failed
toggle rolls back to the modal's value.
Both new tests fail without their fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 3 on #2091. The baseline was still captured when the write
went out, so overlapping toggles reproduced the original bug through the
new machinery: toggle 1 PUTs `true` while its reload is pending, toggle 2
reads the baseline and sees `false`, toggle 1 lands `true`, toggle 2
fails — and its catch pushed the `false` it had closed over, contradicting
both disk and the hook.
`resolve` now reads the record and the list live and takes no fallback,
returning `undefined` when neither has anything to say, so calling it in
the failure handler answers for the state at failure time. Both the
value being written and the rollback go through it, each at the moment
it is needed. The hook says explicitly not to hoist the result.
The regression test drives the exact order — issue 1, issue 2, land 1,
fail 2 — with deferred promises rather than timing, and fails on the
issue-time capture.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 2 on #2091. Recording on completion made the *last to
finish* the winner, and `updateServerSettings` waits for a list read
after its PUT — so an older write with a slow read finishes last and
would overwrite the record with a value a newer write had already
replaced on disk. Both writers can overlap: two quick toggles, or the
settings modal's debounced flush.
A write is now announced with `begin` when it is issued and confirmed
with `landed` when it completes, and a confirmation is dropped if a
later-issued write for that server has already confirmed. The high-water
mark is kept apart from the record so a straggler cannot resurrect a
record a fresh list read dropped.
Issue order is a proxy for disk order rather than a proof of it — HTTP
promises nothing about two in-flight PUTs — but it is the best signal on
this side until the servers route returns a revision to order by. Noted
on the hook alongside the other consequence of that gap.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 1 on #2091. A single record slot let a save on one server
discard the baseline a later failed write on another still needed — the
settings modal opens for any server, connected or not, so that sequence
reproduces #2089 through the very mechanism meant to prevent it. Records
are now kept per server id.
The record also paired with the `servers` entry captured when the write
was *issued*, so a list read landing while the write was in flight was
mistaken for one that happened after it and discarded a live record. It
now pairs with the entry as of completion, read through a ref (mirrored
in an effect — writing a ref during render is a `react-hooks/refs`
error). The residual — a read whose response predates the write but
arrives after it — needs a revision or ETag `useServers` does not
expose, and is documented on the hook.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
The pagination toggle read its rollback baseline off the `servers` entry,
which only advances on a successful `GET /api/servers`. Once a list read
fails that entry describes disk as it was before every write made since,
so the #1721 rollback reverted to a value nothing on disk holds — and
clearing the optimistic override fell back to the same stale entry, so
the UI was wrong from both directions.
Remember the last settings write known to have landed (a new gated hook,
`useLastPersistedSettings`) and roll back to that instead, keyed to the
`servers` entry it was derived from so a fresh list read supersedes it.
Both settings writers — the toggle and the settings modal's debounced
persist — feed the one record.
Closes#2089
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review r3. Three findings, all the same root: a
ServerListReloadError rejects after the write succeeded, so any consumer
still reading "rejection" as "the write failed" now says something false.
- `useImportClientConfig.applyEntry` gave a successfully imported server a
red "Failed" badge. It now reports the intended status and puts the
reload message in `detail`, which renders in red beneath it either way —
so the failure stays visible without claiming the row wasn't created.
- The settings-draft toast said "Failed to save settings". It now branches
its title; this toast is the only signal there, since the modal has
usually closed by the time a flush rejects.
- An edit modal left open by a failed reload kept targeting the old id.
When the row landed under the new one, `configModalTarget` went
undefined and ServerConfigModal's own reset blanked the open form and
cleared the error it was showing. An edit/clone modal whose target has
left the list now closes — adjusted during render via `useValueChange`,
so no frame paints the blanked form. That also covers the ordinary case
of an external mcp.json edit removing the server being edited.
The reachability here is not remote: the write is what triggers the
backend's change broadcast, so the SSE-driven refresh that lands the
renamed row is the likely next event.
Also adds the regression test r3 asked for on the add path — the
highlight batch has to be marked from the catch for the same reason the
rename is. Both new App tests and the import one fail without their fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review r2.
`onConfigSubmit` moved `activeServerId` to the renamed id only after
`updateServer` resolved. Once a failed list reload rejects, the rename is
still on disk — so the active id stayed on the old one and the next
successful refresh left it pointing at a row that no longer exists. Apply
the rename from the catch when the rejection is a `ServerListReloadError`,
then rethrow so the modal still shows the error. `addServerHighlighted`
gets the same treatment: the row is on disk, so it belongs in the
highlight batch the next successful refresh renders.
`activeServerId` is not rendered anywhere, so the test reads it back
through the one handler that hands it to a spy — the pagination toggle's
`updateServerSettings(activeServerId, …)` — after letting the renamed row
land as a later refresh would. It fails without the catch-side call. The
new describe installs its own `useServers` return value and restores the
previous implementation afterwards, since earlier describes' overrides
outlive them (which is why the first draft's spy was never called).
Also replaced the `as unknown as` in the new test with a single
`EventTarget & {…}` intersection, per the TypeScript rules. The other 19
in this file are the same pattern and are left for a separate sweep.
The third comment — that the pagination toggle's rollback baseline goes
stale after a failed list read — is a real defect but pre-existing and
unchanged by this PR; the end state is identical before and after it.
Filed as #2089 rather than widening this fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review r1. `updateServerSettings` is also driven by the pagination
toggle, which updates optimistically and — on any rejection — reverts the
override and rolls the live client back to `prevSettings` (#1721). Making
the post-write reload throw handed that catch a rejection whose PUT had
*succeeded*, so it reverted the UI and the client to a value that no
longer matched disk.
Give the reload failure its own type, `ServerListReloadError`, and branch
on it at that call site: report it, leave the optimistic state alone. The
distinction is what the callers need — a failed write must roll back, a
failed read-back must not — and a type carries it without the call site
matching on a message.
The `useServers` mock in App.test.tsx now spreads `importOriginal()` so
the class is the genuine one; a stub would make the `instanceof` check
vacuously false and the branch untestable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
`useImportClientConfig` applies every addition and conflict sequentially, so
wrapping its per-entry `onAddServer`/`onUpdateServer` in `refreshingPersist`
issued one `/api/config` per imported server. On an encrypted file store each
of those authenticates the whole file with a scrypt derivation (N=16384), so a
20-server import paid 20 serialized KDFs and round trips for a descriptor that
cannot change more than once.
Refresh on close instead: the modal must be dismissed before the settings
footer that reads the descriptor can be reached, and closing also covers a
partially-failed batch.
`ServerImportJsonModal` is left wrapped — it adds a single server, so its one
refresh is already once per batch.
Closes#1950 (review round 26)
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>
Both `useCallback`s that wrap a persist call in `refreshingPersist` captured
`refreshInitialConfig` without listing it. `useInitialConfig`'s `refresh` is
not referentially stable — it is a `useCallback` over `load`, whose deps are
`[base, authToken, doFetch]` — so once any of those changes, the pagination
save and the server removal keep invoking the obsolete closure and the
secret-storage footer can go stale.
`react-hooks/exhaustive-deps` reported both, but the rule is warn-level and
`lint` does not fail on warnings, so `npm run ci` stayed green. These two were
the only warnings in the entire web client.
Closes#1950 (review round 25)
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>
The previous round's `await` did not do what it claimed. `setupClientForServer`
is synchronous and memoized, so a caller that awaited the config resumed with
the `sandboxUrl` captured by the render it STARTED in — undefined, on exactly
the load the wait was added for. The deep-link one-shot guard then prevented a
retry, so that connection silently lost the capability despite having a
sandbox. The OAuth-callback path did not wait at all.
Construction now reads a ref written every render, so it sees the current
value whichever entry point reached it, and the OAuth path waits like the
connect path does. The wait still matters — it is what makes an absent value
mean 'confirmed absent' rather than 'not known yet' — but the ref is what makes
the value read be the settled one.
Signed-off-by: cliffhall <cliff@futurescale.com>
