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>
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>
Two of the three findings; the third is answered in the PR thread.
**A replaced client could still enqueue.** The one-shot sweep only rejected what
was already queued, but the outgoing `InspectorClient` disconnects
asynchronously and can enqueue during its own teardown — and that late entry
would be rendered by a factory bound to the REPLACEMENT client, reading its
resource and answering through a different server. Requests now belong to a
session: each constructed client gets one, closing it rejects that session's
entries AND refuses anything it queues afterwards, and another session's
entries are untouched.
**The advertisement no longer has to guess.** Whether a sandbox exists is only
known once `/api/config` resolves, and the answer is baked into the client at
construction — guessing "available" over-claims, guessing "unavailable" strands
the whole session on the native form despite having a sandbox. The connect path
now awaits that fetch (already in flight since mount, so no human ever waits;
it only orders a deep-link auto-connect racing the same page load), after which
an absent `sandboxUrl` means confirmed-absent.
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 22. Three findings, all in code I wrote, and the first two are both
cases of a stated rationale being wrong rather than a line being wrong.
**`refreshingPersist` skipped the refresh on failure, and I had argued for
that explicitly.** The doc and its test both said "a persist that threw did
not write, so there is nothing new to describe". That is false for this write
order: both persistence paths write the secret store *before* the file, so a
rejected disk write can follow a `set` that has already upgraded
`secrets.json` from plaintext to encrypted. The wrapper then left the footer
describing a file that no longer exists in that form — the stale-descriptor
bug it exists to prevent, reached through its own error path.
It refreshes in a `finally` now. The asymmetry is what settles it: refreshing
after a failure that changed nothing costs one idempotent GET, while not
refreshing after a failure that changed something leaves a security statement
wrong until reload. The test asserting the old behavior is inverted, with the
reasoning recorded so it is not "corrected" back.
**The KDF parameters were validated for well-formedness, not for cost.**
scrypt's work is proportional to `N * r * p`, and all three come off disk — so
a file declaring a modest `N` with `p: 1_000_000` burns unbounded CPU on every
read *and* on merely describing the store for `/api/config`, which is a page
load. Anyone who can write that file can already read its secrets, so this is
not a privilege boundary; it is a footgun and a hang, and the fix is free
because this build writes exactly the constants it now enforces as maxima.
**Two secret-store mutations still bypassed the refresh.** The pagination
toggle resends the server's rehydrated secrets, and deletion sweeps them —
both are writes, and a write is what performs the pending upgrade. Routed
through the same wrapper as the rest.
Full `npm run ci` green, staged tree verified first.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three of round 21's four findings say the same thing: **the UI half of round 20
was never applied, and I reported it as done.**
The edit script aborted partway on a regex error, after writing one file and
before writing the other two. So `ServerConfigModal` gained the footer prop
but `App.tsx` never passed it — the footer was `undefined` in production and
rendered nothing, visible only to the unit test that passes a descriptor
directly — the extra refresh call sites never landed, and the footer's doc
comment still claimed two secret-entry dialogs while the code had three.
`npm run ci` passed throughout, because every test I added exercises the piece
that *did* land. This is the same failure as round 17's uncommitted files with
a different cause: I verified intent (green gate, clean typecheck) instead of
the artifact. Each edit now asserts it changed the file, and the result is
grepped afterwards — the check costs a second and is the only one that
describes what is actually on disk.
Landed now, and verified in the files rather than inferred:
- `App.tsx` passes `secretStorage` to `ServerConfigModal`, so the disclosure
the previous commit claimed to add actually renders.
- Every secret-bearing write refreshes the descriptor — config submit and both
import flows, not just the two settings saves. Any of them can perform the
pending plaintext-to-encrypted upgrade, after which the footer was reporting
"Plaintext file" until reload.
- The footer's doc names all three dialogs. The wrong count there was not a
description of the bug, it *was* the bug: the omitted dialog was the one
taking secrets with no disclosure.
Also, the fourth finding: **the staging name was reusable across restarts.**
`recoverOrphanedSnapshots` deliberately leaves an orphan when a live
`secrets.json` also exists, and a pid-only name repeats (pid 1 on every
container start) — so the next claim would `rename` straight over that orphan
and permanently discard secrets it may uniquely hold. A per-attempt nonce
makes a staging path unrepeatable.
Full `npm run ci` green, staged tree verified first.
Signed-off-by: cliffhall <cliff@futurescale.com>
**The task-augmented inbound branch bypassed the routing.** A server→client
`elicitation/create` carrying `params.task` answers immediately with a
`CreateTaskResult` and settles the TASK when the user answers, so it cannot
ride `enqueuePendingElicitation` — and it was therefore never offered to an
app, even with a valid `_meta.ui.resourceUri`. It now makes the same attempt,
completing the task with the app's result and falling back to the native queue
otherwise; an abort fails the task rather than reopening it natively. The two
task-settling closures are extracted so both answer routes settle it
identically.
**The web client advertised the capability without a sandbox.**
`useInitialConfig` leaves `sandboxUrl` undefined while `/api/config` is in
flight and when the sandbox controller could not start, but the renderer was
supplied unconditionally — so the client claimed it could host an app in
exactly the window where it could not. The option is now gated on a resolved
sandbox URL: such a connection behaves like the CLI/TUI, native queue and no
claim made.
**Concurrent modals each owned the keyboard.** Every open `Modal.Root`
installed its own focus trap, Escape handler and overlay, so the traps fought
and one Escape could dismiss more than one pending request. Only the topmost
owns them now; the rest stay mounted, keeping their bridges and handshakes,
but inert to the keyboard.
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
One inline and three of the four suppressed. Two of the four are cases where
an earlier fix of mine was applied to one branch and not its twin.
**The footer went stale after the write that changed it.** `useInitialConfig`
fetched once, and `secretStorage` is the one field it carries that describes
state *this app mutates*: the first save under a newly-set passphrase
re-encrypts a pre-existing plaintext file. The backend re-derives the
descriptor per request — deliberately, and there is a test for that — but the
client had no way to ask again, so the footer said "Plaintext file" for the
rest of the session about a file that no longer was. Exactly the misreport
this band exists to prevent, arrived at from the client side.
The hook now exposes `refresh()`, and both persist paths that can carry a
secret call it: server settings (OAuth client secret, stdio `env:` values)
and client settings (the enterprise IdP client secret). The refresh tracks
its own liveness through a ref, since unlike the mount fetch it has no effect
teardown to cancel against.
**The plaintext branch skipped the payload validation the encrypted branch
does.** Round 12 tightened the encrypted envelope check and left this one
behind, so `{ "encryption": "none", "secrets": [] }` — trivial to hand-write —
described a healthy plaintext store whose next save `readMap` refuses. Both
branches validate now.
**`secretStoreGetStrict`'s doc contradicted the code it was written for.** It
said falling back to `get` is right "for the in-memory and file stores —
neither can fail in a way that masquerades as absence". `FileSecretStore.get`
does exactly that masquerade, which is why round 9 gave that store its own
`getStrict`. The fallback stays (the seam is optional and doubles must
compile) but the comment no longer claims it is equivalent for a store that
can fail.
**The spec documented the performance bug rather than the fix.** It still said
`getMany` decrypts once per server, which was the shape round 13 replaced —
and describing the earlier bug as the design is worse than saying nothing.
The remaining suppressed comment is the hand-off's TOCTOU delete: the
cross-process guarantee, decided against for this PR and tracked in #2082.
Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
Eight findings from the Copilot review, all accepted.
Ownership across connections was the substantive one, in three parts. A
mid-session transport close reaches teardown only through
`clearAndAnnouncePendingPeerRequests`, whose emptiness check looked at the
native queues alone — so a lone app elicitation survived a dropped connection
with its modal still open; the check now counts the active set too. The web
bridge factory resolves its client at call time, so an entry queued by a
replaced `InspectorClient` could rebuild against the *next* one and answer
through a different server; the host now drops every entry synchronously on
the swap (`AppElicitationController.failAll`) rather than racing core's
awaited teardown. And the request id was only monotonic within one client, so
each replacement client's first request was `app-elicitation-1` — settling
that id could resolve the wrong server's request. It now carries a
per-instance prefix.
Result validation now parses the whole value with `ElicitResultSchema` before
the requested-schema check, instead of inspecting `action` by hand: a
`{ action: "decline", content: { x: {} } }` used to pass, though the standard
result permits only primitives and string arrays in `content`.
The dialog's accessible name now includes the request id and the prompt — two
concurrent elicitations can be for the same app URI, and the previous name
made those indistinguishable, which is what the comment beside it claimed to
prevent.
`observeAppCapabilities` takes a minimal structural `onmessage` contract
(generic over the message type, since a handler typed for a narrower message
is not assignable to one typed for `unknown`), removing the double cast at the
call site.
`smoke:web:elicit` now fails on an uncaught page error after a successful
drive, and captures the async half of that class off the console channel, as
its two sibling smokes do — it could otherwise print OK over a broken bundle.
Adds the tests each fix needs, including the two the review asked for
directly: that `advertiseElicitation` reaches the `AppBridge` constructor, and
that it is absent by default.
Signed-off-by: cliffhall <cliff@futurescale.com>
Let a server hand a form `elicitation/create` to an MCP App and return the
app's ordinary `ElicitResult`, with the native elicitation form as the
fallback on every failure.
No second extension, no custom method, and no custom result shape: the only
new wire surface is a nested `elicitation` flag on the existing
`io.modelcontextprotocol/ui` extension on each side, plus
`_meta.ui.resourceUri` on the request. App rendering is selected only when
all four gates hold — client `elicitation.form`, client MCP Apps MIME type,
the nested `elicitation` setting on BOTH peers, and a valid absolute `ui://`
URI on the request.
Only the web client advertises the nested client-side setting, and only
because it has a sandbox renderer: supplying
`InspectorClientOptions.appElicitation` is what opts a client in, so CLI and
TUI keep advertising the MIME type without ever claiming they can resolve an
elicitation through an app.
Routing lives in the one funnel both entry points already use
(`enqueuePendingElicitation`), so the inbound handler and the MRTR driver
cannot diverge. Ownership is request-scoped — a per-request id keys the
renderer, iframe and bridge — so two concurrent elicitations can never
resolve through each other's bridges. An explicit `decline`/`cancel` is a
completed elicitation and goes back to the server; everything else (absent or
malformed metadata, resource/sandbox/bridge failure, an app with no
elicitation capability, a timeout, an invalid result or one that fails the
requested schema) falls back.
The Inspector speaks the ext-apps#733 / SEP-3118 wire protocol but cannot yet
consume its helpers — the released `@modelcontextprotocol/ext-apps` (1.7.5)
predates that PR. `core/mcp/appElicitation.ts` and
`AppRenderer/requestAppElicitation.ts` mirror it exactly and are marked for
deletion once a release containing it ships. One consequence needed its own
seam: ext-apps 1.7.5 parses the view's `ui/initialize` through a schema that
strips `elicitation`, so an app that correctly advertises it would look like
one that did not, silently turning every negotiated elicitation into a
fallback. `AppRenderer/appCapabilities.ts` records the raw frame instead, and
prefers the bridge's own value once it carries the key.
`AppRenderer` now takes an `AppRenderSource` union rather than a `Tool`, so
the same renderer and bridge factory serve an App tool and an elicitation
without either faking the other's shape.
Adds the public fixture (`app_choose_option` + a self-contained
`ui://demo/choose-option.html` app covering accept/decline/cancel), its two
showcase configs, and `smoke:web:elicit`, which drives the negotiated path
end to end in headless Chromium and then the same tool against a server that
never advertised the capability, asserting the native form takes it.
Closes#1854
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
`runWithCommandAuthRecovery` rethrows anything that isn't an
`AuthRecoveryRequiredError`, and 17 of its 18 call sites were bare
`void runWithCommandAuthRecovery(...)` — so any non-auth failure of a
user-initiated list reload (Retry, Load next page, the pagination-mode
toggle) became an unhandled rejection in the browser.
Neither gate that should catch it could: `void` is the sanctioned
suppression for `no-floating-promises`, and `smoke:web:browser` — which
hard-fails on exactly this signal — never connects to a server.
Add `runCommandInBackground`, a fire-and-forget wrapper that owns the
rejection, and route every background command through it, so a new call
site cannot reintroduce the gap by omission. An optional `errorTitle`
picks the reporting: omitted where the list panel already renders the
failure with a Retry (a toast would duplicate it), passed where nothing
else records it — `logging/setLevel` and the tasks refresh.
Signed-off-by: cliffhall <cliff@futurescale.com>
#1954 wired the list-load error off the aggregate managed stores only.
With `paginatedLists` on, those stores deliberately skip their all-page
walk, so their error is permanently null — while the paged stores that
actually drive the sidebar had no error state at all and `loadPage` had
a try/finally with no catch. A failing list showed an empty panel with
no alert and no Retry, and the connect-time `void loadPage(undefined)`
left an unhandled rejection.
- Give PagedTools/Prompts/ResourcesState observable error state: an
`errorChange` event plus `getError()`, set in a new catch in
`loadPage`, cleared on the next success and on disconnect. The
rejection is still re-thrown, matching ManagedListState — callers'
auth-recovery wrappers key off it to detect a 401.
- Catch the connect-time load's rejection (`.catch(() => {})`) now that
the failure is recorded as state rather than lost.
- Select the error by mode in `usePaginatedList`, the same way `items`
already is, and read it in App.tsx from the pagination model.
- Rename `useManagedListError` to `useListError`: both store families
expose an identical `errorChange`, so the hook declares that one-event
contract itself instead of importing ManagedListEventMap.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
#1961 keyed the sidebar's React children by source position, but everything
downstream still identified a tool by `tool.name`. With a `tools/list` that
repeats a name, both copies highlighted at once, the click guard made the
other copy a no-op, and `tools.find((t) => t.name === …)` always resolved to
the first — so a later copy rendered but could never be opened.
Thread the row key through selection instead:
- `utils/toolUtils.ts` now owns `toolRowKey` (moved out of ToolControls) and
adds `findToolByRowKey`, which compares computed keys rather than parsing
one so the two can't drift.
- `ToolControls`: `selectedName` → `selectedKey`; the highlight and the click
guard compare keys, and `onSelectTool` emits the row key.
- `ToolsScreen`: `ToolsUiState.selectedToolName` → `selectedToolKey`, with
both lookups going through `findToolByRowKey`. Renamed through
`screenUiState.ts` and `App.tsx`'s `onToolsUiChange`.
`onCallTool` still sends `selectedTool.name` — the duplicated name is the
genuine wire identity; only the UI identity needed to be unique.
Also prefixes the excluded (SEP-2243) rows' keys: they render into the same
`Stack` as the list rows but index within their own list, so an excluded tool
sharing a name and index with a listed one collided in the key space.
Closes#2001
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
