| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
cliffhall
|
d57801d7b3 |
feat(core): report connection state on a request timeout (#2318)
A request timeout used to surface as a bare `Request timed out`. Every fact needed to explain it was already in-process: what the client was still waiting on, when it last heard back, and the state of the standalone GET notification stream. The SDK's timeout now rejects with all three, and the same snapshot is shown live in Connection Info and on the Network entry for the stream. - core/mcp/connectionDiagnostics.ts: the snapshot type, its one-line description, and `annotateRequestTimeout`, which rebuilds the SDK's per-request timeout with the state at that moment. - InspectorClient decorates the SDK Client's `request` (the one funnel every verb goes through) so the annotation covers the whole surface; tracks outstanding requests with their send time, the last response, and the most recent long-lived transport stream; exposes `getConnectionDiagnostics()` and `connectionDiagnosticsChange`. - fetchTracking watches a long-lived stream through a tee'd clone, counting SSE events and reporting the close, via a new `updateStream` callback wired through the node transport, the remote session (`fetch_stream_update`), and the browser transport. - FetchRequestLogState patches `entry.stream`; the Network entry's placeholder reports the event count and open/closed state. - Connection Info gains a Connection Activity section, ticking while open (`useTickingClock`). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APKSYHGxXBUpnR7ErFiLJn Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
63623b754b |
feat: support the Skills extension (SEP-2640) with digest verification
Adds detection, enumeration, and conformance checking for the Skills extension (`io.modelcontextprotocol/skills`) — phases 1 and 2 of #2234, which is everything its Acceptance list names. Skills is a *server*-declared extension, read off the connecting server's `capabilities.extensions`, so it is deliberately absent from `ADVERTISABLE_EXTENSIONS` — that registry is what the Inspector advertises and the user toggles, and an entry there would be a meaningless toggle. `skills/list` and `skills/get` are consumer-owned extension methods that neither era codec defines, so the SDK's era gate skips them and they go out as ordinary `client.request` calls with explicit result schemas. The raw-wire channel modern `tasks/*` needs is not used, and the Skills tab is not era-gated: a legacy-era server that declares the extension is serving it. The checks are the point, not the list view. `core/mcp/skills.ts` reports each obligation SEP-2640 states — the name/path invariant, the digest format, the 512-entry and 16 MiB limits, and `resources: "dynamic"`, which means integrity cannot be verified at all. Digest verification hashes the fetched bytes with WebCrypto and returns a mismatch with both digests attached rather than throwing, because showing a mismatch loudly is the whole value proposition. Files are fetched on demand: SEP-2640 is explicit that a `resources/read` of a `SKILL.md` is not a load and confers no standing, so none of the SEP's host machinery is implemented. The `skills-http` fixture serves four skills over two pages, three of them deliberately non-conforming — without those the verification code is untestable. Phase 3 (CLI, TUI, `resources/directory/read`, paged mode) is #2248. Closes #2234 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
546dacffe2 |
chore: address Copilot review round 2 (#2192)
- `useEmaIdpLoginState` attributed a read to the wrong issuer. A bare `EmaIdpLoginState` carries the previous issuer's answer over to a new one until a fresh read lands — and if that read fails, indefinitely, so the settings surface reports "logged in" for an IdP nobody authenticated against. Round 1's `.catch()` made that worse by design, and the test added with it asserted the wrong behaviour outright. State is now the pair (issuer, value) and `loginState` derives from it, so a mismatch reports "none". A monotonic read token — the device `useInitialConfig` already uses — drops completions that were overtaken, including a read still in flight when `logout` clears the session; issuer keying cannot catch those, since both carry the same issuer. Both guards are pinned by tests verified to fail without them. Deriving the value also removed the last synchronous commit on that path, so the `set-state-in-effect` suppression is gone — one fewer than the PR started with. - The draft-seeding change had no test that could tell it from the effect it replaced: the regression is a committed frame, and RTL flushes passive effects before any assertion on `result.current`. Both hooks now have a commit-level trace recorded from inside the tree, asserting the previous target's (or session's) draft is never committed for the new one. Both were verified to fail against the effect implementation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
7e6d6c0bc1 |
chore: address Copilot review round 1 (#2192)
- `useEmaIdpLoginState`: the effect-triggered `refresh()` was discarded with `void`, but it does not own its failures — `getEmaIdpLoginState` awaits `storage.getIdpSession`, which rejects when the backend is unreachable. Terminate with `.catch()` and leave `loginState` untouched, as `logout` already does. Covered by a new test that also asserts no unhandled rejection escapes. - Correct the history: the plugin was *client-local*, declared by both `clients/web` and `clients/tui`. The root could reach neither, since Node resolution walks up rather than down — that, not web-exclusivity, is why `core/react/` went unlinted. - Correct the enforcement claim: `clients/tui` registers the plugin but deliberately enables only `rules-of-hooks` and `exhaustive-deps`, so `set-state-in-effect` and `refs` hold for `clients/web` and `core/react/` only. Says so in AGENTS.md and in the Copilot mirror, which would otherwise have reviewers cite rules the TUI gate does not run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
9ced0fd150 |
chore: lint core/react with eslint-plugin-react-hooks (#2192)
`eslint-plugin-react-hooks` was a `clients/web` devDependency only, and each client's `eslint .` is scoped to its own directory — so the whole hook rule set had never looked at `core/react/`, a directory of nothing but React hooks. "Neither surface has JSX" was the stated reason and was never a good one: the rules judge hooks, not JSX. Add the plugin at the root and apply it in a `core/react/**` block (scoped there rather than to all of `core/**`, since the rules key off the `use` prefix), with `exhaustive-deps` promoted to `error` as `clients/web` does. Resolve the 15 findings it then reports: - `useSettingsDraft` / `useClientSettingsDraft` seed their draft during render instead of in a `targetId`/`opened`-keyed effect, so the previous target's values never reach the DOM — the same rule `clients/web` spells `useValueChange`. Their "latest value" refs move from render-time writes into an effect; every reader (the debounce timer, `flush`, `onChange`) runs after a commit, so a post-commit write is what they need. - The three async loaders keep their effects behind justified inline disables: `set-state-in-effect` follows a call into an async function without modelling the `await`, verified against a minimal hook whose only setState is in a continuation. - `useInitialConfig`'s cleanup bump keeps reading `generation.current` at teardown — the rule's "copy it into a variable" advice would restore a stale request token and let a superseded response commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
8f3619f618 |
docs(core): correct the useStoreSnapshot caller contract
Copilot review round 2. The contract claimed an unstable `read`/`whenAbsent` trips React's "The result of getSnapshot should be cached" error. It does not. Measured on React 19: an inline `read` closure throws nothing, logs nothing, and forces no extra render pass. React's dev check calls getSnapshot twice within a single render, where the closure is unchanged, so the cache answers both calls identically and the warning never fires. The real cost is quiet — a fresh value every render, defeating downstream useMemo/React.memo/effect deps — which is a better reason to state the contract than an error that never arrives. Also stop requiring module scope for every fallback. A primitive `whenAbsent` (`false`, `undefined`, `"disconnected"`) is stable under Object.is and is passed inline throughout these hooks, so the rule contradicted the code it was describing. Only object/array fallbacks need a constant, which is what NO_TOOLS and NO_PAGINATION are. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
f6105773f0 |
fix(core): key the snapshot cache on every input it derives from
Copilot review round 1. The cache was keyed on the store and its revision alone, so a changed `read` was answered with the previous reader's output, and with a null store — where the revision is pinned at 0 — a changed `whenAbsent` was ignored outright. Key on `event`, `read` and `whenAbsent` too. This makes an unstable argument defeat the cache rather than silently serve a value derived from the previous one, so React fails it with "The result of getSnapshot should be cached" at first render. That is the intended failure mode and is now documented as such: a loud error beats a wrong list nobody can account for. Also correct the docs, which described the helper as universal. That misread useListError as a violation: its snapshot is the stored Error instance, already referentially stable, so it subscribes with useSyncExternalStore directly and needs no cache. The rule is read-during-render; the helper is for getters that return a fresh value per read. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
50502269f4 |
fix(core): read state stores during render instead of re-syncing in an effect
Every state-subscription hook in core/react/ seeded local React state from its store prop and re-synced it inside a useEffect. That shape paints one frame of the previous store's data when the prop changes (switching servers), and loses any event dispatched between the render that took the snapshot and the effect that attached the listener. Convert all 16 to useSyncExternalStore through a new shared useStoreSnapshot, following useManagedListError (#1954). useSyncExternalStore needs a referentially stable snapshot, and these getters hand back a defensive copy, so TypedEventTarget now keeps a per-event-type dispatch counter (getEventRevision) that the snapshot is cached against. A contents comparison would not do: MessageLogState folds a response into its request entry by mutating that entry in place, which a shallow compare reports as unchanged. The bump lives on the dispatchEvent override so no dispatch — including a hand-rolled CustomEvent — can bypass it. useInspectorClient is included though the issue did not list it; its nine subscribed slices made the stale frame most visible. Its lastError stays useState (accumulated from events, no getter to read back) but resets on client swap during render rather than in an effect. The settings-draft and async fetch hooks are deliberately untouched: the former hold genuinely local editable state, the latter are I/O rather than store subscriptions. Also answers the issue's open question. react-hooks/set-state-in-effect never fired here because eslint-plugin-react-hooks has never linted core/react/ at all — it is a clients/web devDependency and the root config loads no React plugin. Corrected the config comment that gave "no JSX" as the reason; the gap itself is #2192, measured at 15 findings, none of them in the hooks converted here. Closes #1955 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
Cliff Hall
|
8b9839cc93 |
Merge pull request #2087 from modelcontextprotocol/v2/fix/1914-surface-post-add-refresh-failure
fix(web): surface a failed post-write server-list reload |
||
|
cliffhall
|
b33532c024 |
feat: allow complex JSON values in request _meta
Closes #1910 `_meta` was typed `Record<string, string>` end to end in the Inspector — on disk as a `{ key, value }[]` pair array, in the Server Settings form as key/value rows, and in every request verb's per-call metadata parameter. Nothing in the MCP spec restricts a `_meta` value to a string, and the SDK models the field as a `z.looseObject`, so the restriction was ours alone. Widen the whole path to `RequestMetadata` (`Record<string, JsonValue>`): - Per-server default metadata is a JSON **object** on disk and in memory. The pre-#1910 pair array is still read (`normalizeStoredMetadata`) so an existing `mcp.json` keeps working; it is never written back, so a save migrates the entry. - The Server Settings → Request Metadata section is a JSON editor (`JsonObjectInput`) instead of key/value rows — rows cannot express an object value. It keeps the typed text as the display source and emits only parsed JSON, the same draft split `SchemaJsonField` uses, so the box stays typeable; invalid text is flagged inline and not applied. - The CLI stops flattening `--metadata` / `--tool-metadata` through `metaValueToString`. Those values were already JSON-parsed, so `trace={"id":"abc"}` used to reach the wire as the *string* `{"id":"abc"}` and now reaches it as an object. - `servers/show` redaction replaces a sensitive key's whole value rather than a string, so a structured value cannot leak through its members. - `progressTokenOf` narrows the reserved `progressToken` key back to `string | number` before it reaches the SDK; any other JSON still travels inside `_meta` but wires up no progress callback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
fd21d0d528 |
fix(web): don't roll back an optimistic setting a failed reload didn't undo
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> |
||
|
cliffhall
|
4379d17f76 |
fix(web): surface a failed post-write server-list reload
`useServers`' mutators awaited `refresh()`, whose failure was swallowed into the hook's `error` state rather than thrown. `ServerConfigModal` renders `submitError` only for something `onSubmit` threw, so a successful `POST /api/servers` followed by a failed `GET /api/servers` closed the Add Server modal as though the whole thing had worked — no inline error, no toast, and no new row, since the list read is what would have produced it. That is the silent failure reported in #1914; #1848 removed the trigger (a keyring-less container 500'ing the list read) but not the swallow, so any other cause reproduces it. Split the list read into `loadServers` and give the mutators their own `refreshAfterWrite`, which records the failure in `error` as before and then rethrows. `refreshInternal` is unchanged for the mount effect and the SSE loop — neither has anywhere to put a rejection. The thrown message says the write landed, because it did; only reading the list back failed. A bare "could not add server" would send the user straight into a retry that trips the duplicate-id check, which is the dead end the original report described. Every consumer of these mutators already handles a rejection: ServerConfigModal and ServerRemoveConfirmModal into an inline error, useSettingsDraft and the pagination toggle into a toast, and both import flows into their per-entry outcome list. Closes #1914 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> |
||
|
cliffhall
|
de84b67046 |
fix(auth): stop blaming the passphrase for a corrupt payload, and refresh the stale docs
Round 23 produced **no inline comments** — nothing wrong in the code it had not already seen. The five suppressed ones are all in unchanged code: one real misdiagnosis and four pieces of drift. **A decrypted-but-corrupt payload was reported as a wrong passphrase.** By the time `JSON.parse` runs, GCM has verified the tag — so the passphrase is *proven correct*, and a `SecretFileKeyMismatchError` there tells the user to restore a secret they never lost. `asSecretMap` was already outside that catch for exactly this reason; `JSON.parse` was not, so the same class of mistake survived one line above the fix for it. The decrypt now has its own narrow catch and everything after it reports a corrupt file. The test fixture could not express this case — it `JSON.stringify`s its payload, so it can only produce valid JSON. Split into a raw-plaintext variant so a fixture can carry bytes that are authentic under the passphrase and still unparseable, which is the only shape that reaches the branch. The rest is documentation that had drifted from the code it describes: - `handOff`'s doc still described the re-read-and-compare protocol that the atomic claim replaced two rounds ago — an obsolete concurrency invariant is worse than none, because the next reader takes it as the contract. - The spec still described that same hand-off, and still called Client and Server Settings "the only two dialogs that accept a secret". Both now match the code, and the dialog count is called out as load-bearing: the one that was missing from it was the one taking secrets with no disclosure at all. - The `void load()` in `refresh` carries the justification the repo's no-floating-promises rule requires — `load` owns its failures, and `refresh` is deliberately synchronous because `refreshingPersist` calls it from a `finally`. Full `npm run ci` green, staged tree verified first. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
52087f7739 |
fix(auth): address Copilot review round 16
Three findings. The first is the third consecutive round in which my previous fix introduced the next defect, all in the same ~15 lines of `useInitialConfig`. **The request token did not subsume the unmount guard, and I said it did.** Bumping the generation on effect teardown invalidates loads already *in flight*; a `refresh()` called after unmount claims the next token, is therefore current, and commits into a dead hook. That is a live path — the callers fire `refresh()` from an async settings persist that can outlive `App`. Mounted state is tracked separately again and `stale()` checks both. The test for this was also wrong, in the way that matters: it called `refresh()` after unmount but never settled the fetch, so it could not observe the commit it existed to prevent. It settles the promise now. **A tmpfs mount was reported as durable.** `docker run --tmpfs /home/node/.mcp-inspector` puts a real entry in mountinfo, so a mount-table lookup answered "durable", selected the file store, and published `durable: true` for a file that disappears when the container stops — the exact false promise the in-memory fallback exists to avoid, reached by trusting the mount table over the filesystem type. `tmpfs`/`ramfs` are excluded now, read from the field after the ` - ` separator rather than by position, since the optional fields before it are variable in number. **A malformed envelope was diagnosed as a changed passphrase.** A truncated payload, a wrong-length IV or tag, or an out-of-range KDF parameter all threw `SecretFileKeyMismatchError`, whose 503 tells the user to restore a passphrase that cannot repair a structurally broken file. The read path now runs the same `encryptedEnvelopeProblem` check the descriptor path already ran, and reserves the key-mismatch error for an envelope that is well-formed and fails authentication — which is the case where that advice is right. Two existing tests asserted the old behavior and encoded the bug; both updated, and a new one pins that a well-formed envelope with the wrong key still gets the passphrase advice. The remaining suppressed comment is the hand-off's check-to-delete race, tracked in #2082. Full `npm run ci` green. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
0ab542cbee |
fix(auth): order /api/config loads so an older response cannot win
Round 15, one finding — in the refresh path added one commit ago. `refresh()` fires after each secret-bearing save, and nothing ordered the responses. Two debounced settings saves landing close together issue two refreshes; if the earlier request resolves last, it commits the descriptor from *before* the write and the footer reverts to "Plaintext file" for a file that is now encrypted. Reverting a security statement to a stale value is the worst direction for this particular field to be wrong in, and it needed no adverse network conditions — just two saves in quick succession. Loads now claim a monotonic token and may commit only while they still hold the current one. That subsumes the liveness ref the previous commit added: bumping the token on effect teardown covers unmount *and* a `baseUrl` / `authToken` change, and bumping it on every load covers the overlap. The test drives it by holding the `fetch` promises and resolving them out of order — newer first, then older — which fails against the previous implementation and passes here. The remaining suppressed comment is the hand-off's check-to-delete race, which is the cross-process guarantee tracked in #2082. Full `npm run ci` green. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
0c52172379 |
fix(auth): address Copilot review round 14
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>
|
||
|
cliffhall
|
fdeffa1f00 |
fix(auth): address Copilot review round 6 on the secret-store PR
Four findings, all suppressed, all real. Three are the same mistake in different places: a state that was never established being reported as a confident answer. - **`readOnDiskEncryption` conflated "no file" with "unreadable file".** Both answered `null`, and the descriptor falls back to the configured write policy on `null` — so a passphrase-configured install reported "File (encrypted)" about a file whose envelope had never parsed, and which `set` was in fact about to refuse rather than overwrite. It now returns a four-state result, and only `absent` falls back to the policy (honestly: the first write really will create it that way). `unreadable` produces `encryptionUnknown` on the descriptor, `plaintext` is omitted entirely so nothing can read a default out of it, and the label, caveat, tone and footer all carry "unreadable" rather than guessing a mode. - **The loose-mode warning overstated exposure for an encrypted file.** "Anyone who can read it can read the secrets in it" is false when the file is ciphertext — a reader gets the bytes and still needs the passphrase. Both the caveat and the footer tooltip now branch on encryption: the passphrase becomes the only protection, which is the real consequence. Overstating it is not harmless, because a warning that cries wolf is discounted on the occasion it is literally true. - **The keychain hand-off could invert its own keychain-wins rule.** `SecretStore.get` answers `null` both for a missing entry and for a keychain it could not read, and the migration *writes* on `null` — so a transient read failure would overwrite a newer keychain value with the older file copy. Added an optional `getStrict` (throws instead of swallowing) implemented by `KeyringSecretStore`, with `secretStoreGetStrict` falling back to `get` for stores whose reads cannot fail. A throw now aborts the hand-off with the file left in place, so the next run retries. Writing the tests turned up one more, in code the review had passed: `parseOwner` recorded a garbled pid as `-1`, and `process.kill(-1, 0)` is a *wildcard* asking about every process this user can signal — it answers "alive" almost always, which would have made a lock with a corrupt stamp permanently unstealable. Only a positive pid is usable now; anything else is treated as no pid at all and the mtime decides. Tests: the four envelope states kept distinct; the descriptor refusing to guess, with the genuine no-file case as the control; strict-read abort without a write and with the file preserved, plus a store whose `get` and `getStrict` disagree so the test proves which one the migration consults; encryption-aware wording asserted in both directions in the caveat and the band; three unusable owner stamps and a wildcard pid; and the descriptor shapes that must now be rejected — neither half of the encryption question answered, or both. Full `npm run ci` green — 5,750 tests under the coverage gate. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
decc673a5d |
fix(auth): address Copilot review round 5, and land the impl its tests need
Repairs origin as well as answering the review.
|
||
|
cliffhall
|
b143dd6cbd |
fix(auth): address Copilot review round 3 on the secret-store PR
Nine findings (six inline, three suppressed), all taken. Two were serious. **The memory fallback made the migrations destructive.** Both migrations — mcp.json in the remote server, client.json in `node-persistence` — lift a plaintext secret off disk, write it to the secret store, and then delete it from the file. That trade is safe only while the store is at least as durable as the disk, which held by construction while `InMemorySecretStore` was a test double. Making it a production fallback broke it: on an unmounted container a plain `GET /api/servers` would have moved a user's existing secrets into RAM and lost them at exit — destroying data on a read. `SecretStore` gains an optional `isDurable()`, absent meaning durable, and both migrations withhold the disk delete when it answers false (the value is still loaded, so the session behaves normally). The production fallback is now a named `SessionSecretStore` rather than the bare `InMemorySecretStore`, so the suite's many doubles keep standing in for a *working keychain* — which is what they are there to be — and only code that deliberately chooses RAM in production says so. **Round 2's per-call descriptor never reached anyone.** Both web servers resolved it once at startup and baked it into `initialConfig`, which `/api/config` serves unchanged — so the freshness fix was inert for its only consumer. The route now takes a `secretStorageResolver` and calls it per request. The lock needed three more fixes, and they compound: - **Release removed whatever directory was at the path.** A holder whose work outran the stale threshold would delete the *next* owner's lock, letting a third writer into a section two processes believed they held. Ownership is now stamped with a random token and release only removes a lock still carrying it. - **The timeout fell through to an unlocked write** — under contention, the one moment another writer is provably active. It now throws `SecretFileLockTimeoutError`, which `set` is allowed to do. - **The first write of every process bypassed the lock**, because the secrets directory does not exist yet and `mkdir` failed ENOENT. The parent is created first. First writes are exactly when two processes are most likely to start together. Fixing the timeout exposed a fourth: with stale (10s) below timeout, a waiter stole from a holder that was merely *slow* rather than dead — reinstating the lost update. A holder now heartbeats its lock, so "stale" means the holder is gone, and the timeout is what a live-but-slow holder produces. Lock timings are injectable so the tests can exercise contention, staleness, and timeout without spending the real intervals. Also: the loose-permission state is carried into `SecretStorageInfo` as `looseMode` and rendered as the caveat (it outranks the encryption caveat — a file others can read is a live exposure, while "unencrypted" is a property of a file only its owner can open), rather than living in a startup log the browser cannot see; and `usableSecretStorage` validates `pendingEncryption` and `looseMode` as the optional booleans/numbers they are (`pendingEncryption: "false"` is truthy and would have printed advice that is the opposite of the truth) and rejects file-only fields on a keychain or memory descriptor. Tests: both migrations against a session store with a durable control alongside, so the guard is visibly the difference; `/api/config` serving a changing resolver and falling back without one; lock ownership after a steal, the timeout under a live holder, no-steal from a slow holder, and a locked first write; `looseMode` through the descriptor, the caveat ordering, and the tone; and five more rejected payload shapes. Full `npm run ci` green — 5,704 tests under the coverage gate. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
3452a63101 |
fix(auth): address Copilot review on the secret-store PR
Six findings, all taken.
- The descriptor reported the *write policy*, not the file. Adding
`MCP_INSPECTOR_SECRET_KEY` to an install that already has a plaintext
file flipped `store.encrypted` immediately, so the banner and footer
claimed "File (encrypted)" while the existing secrets sat readable
until the next `set`. `FileSecretStore.readOnDiskEncryption()` now
reads the envelope (never the payload, so it needs no passphrase) and
the descriptor is built from that. The transitional state gets its own
`pendingEncryption` flag, which changes the advice without softening
the verdict — "set a passphrase" is the wrong thing to tell someone who
already has, so the caveat names the pending write instead.
- The secrets file ignored the configured storage directory. A container
mounting only that directory was judged unmounted, fell back to
`memory`, and lost secrets the user had arranged to keep.
`defaultSecretFilePath()` now follows `MCP_STORAGE_DIR` — the variable
that already relocates OAuth tokens and `client.json` — between the
explicit `MCP_INSPECTOR_SECRET_FILE` and the `~/.mcp-inspector`
default. (The issue comment called it `MCP_INSPECTOR_STORAGE_DIR`; no
such variable exists.)
- `version` was never checked before the plaintext branch, so a
`{ version: 2, encryption: "none" }` file was read and silently
rewritten as version 1, discarding whatever the newer format added —
the same destroy-to-satisfy-an-additive-request case the encrypted
branch already refuses. Checked first now, so the answer no longer
depends on which encryption mode the newer writer chose.
- The doc comment claimed a per-file salt made "a short, memorable value
safe to use". It does not: the salt defeats precomputation, not
guessing, and the scrypt cost is deliberately a few milliseconds
because it runs on every read and write. Both the comment and the
README now say to use a high-entropy value, generated rather than
chosen.
- `usableSecretStorage` validated only `kind`, so a partial
`{ kind: "file" }` was cast through and — because a missing `plaintext`
is falsy — rendered as the quiet, neutral *encrypted* file. That is the
confident half-answer the guard exists to reject, produced by the
absence of information rather than by any claim the backend made.
`reason`, `durable`, and each kind's own required fields are checked.
- The footer's inline `Tooltip` carried four static props; extracted to a
`CaveatTooltip` constant per the `.withProps()` rule.
Tests cover each: the policy/file disagreement in both directions and the
no-file-yet fallback, `MCP_STORAGE_DIR` and its precedence against
`MCP_INSPECTOR_SECRET_FILE`, the version refusal (asserting the bytes
survive it) split from the unrecognized-cipher refusal it used to be
conflated with, `readOnDiskEncryption` including the keyless and
unparseable cases, five new rejected descriptor shapes, and the changed
caveat. Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
bc408f5869 |
feat(auth): file-backed and in-memory SecretStore for hosts with no keychain
Closes #1950 Before this, a box with no reachable OS keychain — the published container (no D-Bus session, #1848), Android/Termux (no platform binary, #1905), a minimal Linux install without libsecret — could not persist an OAuth client secret or a stdio `env:` value at all. `KeyringSecretStore` degraded into a store that silently holds nothing: `get` returned null, `delete` no-opped, and `set` hard-failed with a 503. That is the correct failure behavior, but it left the documented, supported way to run the Inspector with no secret persistence whatsoever. Adds the two missing implementations and, more importantly, the policy that picks one and the surfaces that say which was picked. - `FileSecretStore` — one `0600` JSON document at `~/.mcp-inspector/secrets.json`, encrypted with AES-256-GCM whenever `MCP_INSPECTOR_SECRET_KEY` is set (scrypt, per-file random salt). The whole map is encrypted as a unit rather than value-by-value so the account names — `${serverId}:${field}` — are hidden too; a per-value scheme would leave a readable index of which servers you hold a client secret for. A file that can no longer be decrypted reads as empty (matching the interface's read tolerance) but *refuses to be written*, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - `secret-store-selection.ts` — the policy. `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright; otherwise probe the keychain and, if unreachable, fall back loudly. The fallback is `memory` in a container whose secrets directory is not on a mount, and `file` otherwise — so mounting the volume the README already recommends for the catalog flips the same run to durable storage with no configuration, and an unmounted container gets the honest answer rather than a file `--rm` will discard. Cached per process so the banner, `/api/config`, and the store doing the writing cannot disagree. - Automatic, but never silent. The downgrade prints at startup, rides `GET /api/config`, and is named in a permanent 32px footer at the bottom of both dialogs that accept a secret. A startup banner is seen once by whoever started the process; a toast is seen once; a dismissible banner is the thing a user dismisses before doing the work it describes. The plaintext and memory cases render their caveat inline in a warning tone — a warning you must hover to discover is not one. - `SecretStoreUnavailableError` is now the base every store's `set` throws, so the routes translate one condition into a 503 instead of matching `KeychainUnavailableError` by name. Docs: the container section of the root README gains a table of which store you get and how to make secrets survive, plus the `MCP_INSPECTOR_SECRET_KEY` upgrade/mismatch rules. Tests: the existing interface contract block is extracted to `secretStoreContract.ts` and run against all three implementations, so none can drift on the availability contract. `FileSecretStore` is tested against a real temp filesystem — mode, cross-instance round-trip, that the ciphertext does not contain the plaintext, the read-modify-write race, and every refusal path. Full `npm run ci` passes, per-file gate included. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
de60983ccc |
fix: surface list-fetch failures in paginated mode (#1998)
#1954 wired the list-load error off the aggregate managed stores only. With `paginatedLists` on, those stores deliberately skip their all-page walk, so their error is permanently null — while the paged stores that actually drive the sidebar had no error state at all and `loadPage` had a try/finally with no catch. A failing list showed an empty panel with no alert and no Retry, and the connect-time `void loadPage(undefined)` left an unhandled rejection. - Give PagedTools/Prompts/ResourcesState observable error state: an `errorChange` event plus `getError()`, set in a new catch in `loadPage`, cleared on the next success and on disconnect. The rejection is still re-thrown, matching ManagedListState — callers' auth-recovery wrappers key off it to detect a 401. - Catch the connect-time load's rejection (`.catch(() => {})`) now that the failure is recorded as state rather than lost. - Select the error by mode in `usePaginatedList`, the same way `items` already is, and read it in App.tsx from the pagination model. - Rename `useManagedListError` to `useListError`: both store families expose an identical `errorChange`, so the hook declares that one-event contract itself instead of importing ManagedListEventMap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
07a7b0beb5 |
fix(web): accept CRLF-delimited SSE frames on the servers events stream
The `/api/servers/events` reader in `useServers` searched for `\n\n` and consumed exactly two characters as the separator. SSE permits CRLF line endings, so a `event: change\r\n\r\n` frame stayed buffered forever: `isSseDataFrame()` was never reached and no background refresh fired, leaving the server list stale until a manual refresh or an LF-delimited event arrived. Frame boundaries are now located with a CRLF-aware matcher that reports the separator's real length, and frame lines split on either terminator. A bare CR is deliberately not accepted as a terminator: it cannot be distinguished from the first half of a CRLF straddling a chunk boundary, so a buffer ending in `…\r\n\r` would read as a completed frame and the `\n` arriving next would open a bogus one. Two regressions cover it, both verified failing against the old parser. Each uses a referentially stable `fetchFn` and holds the frame behind a promise released after the disk write — with an inline `fetchFn` the hook re-subscribes every render and that re-subscription's own mount refresh picks the edit up regardless, so the test would pass unfixed. Closes #2006 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
fe5b622fd1 |
fix: salvage a list result's valid entries when one is malformed (#1909)
The SDK validates a list result as a whole, so a single non-conforming entry
rejected the entire response and the panel showed "Couldn't load resources"
with zero items — the exact opposite of what a debugging tool should do with a
server that is 90% right. Reported against a PHP server whose empty
`annotations` object reaches the wire as `[]` rather than `{}`.
`listAll*` now falls back to a lenient re-walk when — and only when — the SDK
rejects a result it received (`InvalidResult` / `UnsupportedResultType`), keeps
the entries that validate, and reports the rest. A conforming server takes the
strict, cache-aware path exactly as before.
Salvaging is not accepting: the dropped entries are named with their reason in
a warning above the list, and the Protocol entry is still marked rejected, so
the wire truth survives. When every entry validates, the original error is
rethrown rather than swallowed — the rejection was about something the
per-item pass cannot explain.
`isClientDecodeRejection` moves to the new module and is now shared with
managedListState, which had the only copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
Cliff Hall
|
9e074aa6f2 |
Merge pull request #1946 from modelcontextprotocol/v2/fix/firefox-sse-stream-hang
fix: prime SSE streams so Firefox resolves the events fetch |
||
|
cliffhall
|
a6f233eb6a |
fix: read the list error via useSyncExternalStore; clear correlation maps
Two fixes from Copilot's review of #1953. useManagedListError re-synced React state from the `state` prop inside a useEffect, which renders one frame carrying the PREVIOUS store's error after `state` changes (switching servers) before the effect corrects it — the pattern AGENTS.md forbids. useSyncExternalStore reads the snapshot during render, so a store swap lands in the same frame, and it also closes the window where an error recorded between render and subscribe was missed. The snapshot returns the stored Error instance (or null), so it is referentially stable as the API requires. The `markResponseRejected` correlation maps were never cleared. Ids of requests that never got a response — a timeout, a dropped connection — are the only entries trackResponse can't remove, so they accumulated across reconnects. Cleared on the start-clean connect path, per the convention documented on resetSessionState (one route out, `onerror` with no `onclose`, tears down nothing). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
f201032137 |
fix: surface list-fetch failures instead of swallowing them
A list load that failed — a transport error, or a result the SDK's era codec rejects as invalid — reported nothing. The connect-time refresh is fired with no caller to await it (`void this.refresh()`), so its rejection became an unhandled rejection: the header said "Connected", the panel rendered an empty list indistinguishable from a server that has none, and the failed exchange showed in the Protocol panel as a clean success. Two surfaces now tell the truth: - ManagedListState records the failure as observable state (`getError` + an `errorChange` event) and still re-throws, so App's auth-recovery wrapper keeps working. The four `useManaged*` hooks expose it via a shared `useManagedListError`, and the Tools/Prompts/Resources sidebars render a `ListLoadError` alert with the reason and a Retry. - A response the client refused is attributed back to its Protocol entry (`markResponseRejected` → `responseRejected` → `MessageEntry.clientError`), which now renders an Error status and a "Rejected by the Inspector" alert rather than the green resultType badge alone. The SDK gives no request id with a decode failure, so the id is recovered by correlation; see the comment on `markResponseRejected` for why that is exact. Also stops `listAllTools` from dropping a failing excluded-tools walk on the floor — it stays non-fatal but is logged. Closes #1953 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
9644ac7fe8 |
fix: prime SSE streams so Firefox resolves the events fetch
Firefox does not hand a streaming `fetch()` response to JS until the first *body* byte arrives; Chromium resolves the promise as soon as the headers do. Both SSE endpoints flushed headers immediately and then stayed silent until there was something to report, which deadlocked `/api/mcp/events`: `RemoteClientTransport.openEventStream()` awaits that fetch *before* the MCP client sends `initialize`, so no `initialize` -> no event to report -> no body byte -> the fetch never resolves. The web UI hung on "Connecting..." forever with nothing in the UI, the console, or the node logs. Write an inert `:` SSE comment the instant each stream opens. Conforming parsers ignore a comment line, so this unblocks the read without inventing a wire event and needs no client-side protocol change. `X-Content-Type-Options: nosniff` was also proposed on the issue but does not fix it -- verified against Firefox 153, the fetch still never resolves with the header set and no body byte. The delay is not MIME sniffing. The priming write is deliberately ordered *after* each handler registers its consumer (the session event consumer; the file-watch subscriber plus `ensureWatcher()`). Callers treat the arrival of the stream's first bytes as proof they are subscribed, so priming first would hand out that proof across an `await` and drop an edit made in the gap. `/api/servers/events` gets the same treatment: its fetch was equally stuck on Firefox until the first real change event, which for an unedited `mcp.json` never comes. Its client-side reader counted any `\n\n` as a change, so the inert frame would have fired a spurious background re-fetch on every connection -- it now skips frames carrying no `event:`/`data:` field. The transport's own `parseSSE` already drops comment frames correctly and needed no change. Verified end to end in real Firefox against a live streamable-HTTP test server: before, stuck on "Connecting..." past 25s; after, initialize -> notifications/initialized -> tools/list, connected. Closes #1858 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU |
||
|
Cliff Hall
|
8aba3122c5 |
refactor(web): consolidate the three GET /api/config hooks into one (#1770)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
* refactor(web): consolidate the three GET /api/config hooks into one (#1643) useSandboxUrl, useServerListWritable, and useInspectorVersion each fetched the same static /api/config payload separately and read one field from it — three requests on load for one payload. Replace them with a single useInitialConfig hook that fetches once and exposes { version, sandboxUrl, writable, loading } with the same tolerant parsing/defaults (writable defaults true; version/sandboxUrl undefined when absent; unmount-safe). - Add core/react/useInitialConfig.ts; wire App.tsx to it. - Remove the three former single-field hooks and their tests. - Fold the fetch-branch matrix into one useInitialConfig test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): address review nits on useInitialConfig tests (#1643) - "keeps writable true for any non-false value" now sends real falsy-but- not-false values a nonconforming backend could actually emit (null / 0 / "no") instead of `writable: undefined`, which was indistinguishable from an absent key and duplicated the omitted-field test. - Soften the post-unmount comment to say the test exercises the isCancelled() guard branches for coverage rather than implying it can detect their removal (React 18 dropped the setState-after-unmount warning). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs(web): fix contradicting unmount-guard comment; it.each writable cases (#1643) Second-review follow-ups: - core/react/useInitialConfig.ts: correct the isCancelled() rationale — React 18 no longer warns on setState-after-unmount; the guard exists to stop a stale payload (after unmount or a baseUrl/authToken re-run) overwriting current state. The old wording contradicted the corrected test comment. - useInitialConfig.test.tsx: convert the writable-non-false loop to it.each so each value (null / 0 / "no") reports independently. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): use vitest %j specifier in it.each title; tidy hook docblock (#1643) Third-review follow-ups (both title/comment-only): - useInitialConfig.test.tsx: `%p` is a Jest specifier vitest doesn't support, so all three cases rendered the literal `%p`. Switch to `%j` — titles now render null / 0 / "no" distinctly (verified: `writable=null`, `writable=0`, `writable="no"`). - useInitialConfig.ts: extend the class docblock to say a response is dropped after unmount OR a re-fetch, matching the corrected inline guard comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
373f346e5f |
chore: close quality-gate coverage gaps (#1689) (#1766)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
Closes #1689. Typecheck cli/tui src, add a core/ format+lint gate, and gate core/json + core/client coverage. See PR #1766 for the full review history. |
||
|
Cliff Hall
|
e70a11360c |
Tools: x-mcp-header annotation tooling + -32602 unknown-tool rendering (#1632) (#1735)
* Tools: x-mcp-header annotation tooling + -32602 unknown-tool rendering (#1632) Card 9/10 of the SDK-v2 + New Spec stack. Adds SEP-2243 `x-mcp-header` tooling to the Tools tab and adjusts result rendering for SDK v2's unknown-tool behavior, and bumps the SDK to 2.0.0-beta.5. - **`core/json/xMcpHeader.ts`**: a faithful port of the SDK's (non-public) `scanXMcpHeaderDeclarations` — validates every SEP-2243 constraint (primitive types incl. `number`, RFC 9110 token, case-insensitive uniqueness, statically-reachable `properties`-only chain) and derives the mirrored `Mcp-Param-{Name}` params for a kept tool. - **Excluded tools**: the SDK silently drops tools with invalid annotations from `tools/list`. InspectorClient now re-lists the RAW (unfiltered) list and scans it to recompute the excluded set with reasons — gated to the modern non-stdio era (matching the SDK's gate), recomputed on every aggregate refresh, surfaced via a new `excludedToolsChange` event + `getExcludedTools()` and `useInspectorClient().excludedTools`. The Tools sidebar lists them struck-through under an "Excluded (SEP-2243)" divider with the reason on hover. - **Mirrored args**: ToolDetailPanel shows a "Mirrored request headers" section listing each arg → `Mcp-Param-{Name}` (with the browser-omits note). - **-32602 rendering**: SDK v2 rejects an unknown-tool call with `-32602` instead of an `isError` result, which previously rendered nothing. New ToolCallErrorPanel renders a thrown tool-call error, with a targeted "Unknown Tool" hint for `-32602`. Adds unit + Storybook + integration coverage (a real modern server with an invalid-header tool fixture) for every path. `npm run ci`-equivalent checks green. Closes #1632 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test-servers: x-mcp-header modern showcase config + preset (#1632) Register the `invalid_header_tool` fixture as a preset and add `xmcpheader-modern-http.json` — a modern HTTP showcase serving echo, a valid-header `get_weather`, and the invalid-header tool — so the Tools-tab x-mcp-header tooling (mirrored headers, excluded-tools surface, -32602 rendering) can be exercised manually. Documented in the README alongside the other per-SEP showcase configs. Smoke-tested end to end in the web client. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: add #1632 proof screenshots to pr-screenshots End-to-end web-client verification of the x-mcp-header Tools tooling against the modern showcase server: excluded-tools surface + reason, mirrored-headers section, and the -32602 unknown-tool error panel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Respond to review: disambiguate -32602, doc/search nits (#1632) Addresses the review on PR #1735: 1. -32602 "Unknown Tool" mislabel: `-32602` is generic Invalid params, so a known tool called with bad arguments hit the same code and rendered under the definitive "Unknown Tool" heading. Replace `isUnknownToolError` with `classifyToolCallError(code, message)` → "unknown-tool" only when the message names it (not found / unknown tool / …), else "invalid-params" (new "Invalid Parameters" heading + schema hint), else "generic". 2. Double pagination walk: expand the `listAllTools` comment to document that the excluded-tools recompute is a deliberately un-cached second raw walk (doubles list round-trips on modern non-stdio, runs even on a cache-served aggregate) — accepted for a debugging tool, no-op on legacy/stdio. 3. Doc nit: `scanXMcpHeaderDeclarations` no longer claims to visit "$ref targets" — reworded to note `$ref` is never followed (refs resolve into the unreachable `$defs` body). 4. Search nit: the excluded-list filter now matches name AND title, matching the main tools filter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tighten unknown-tool matcher to be tool-scoped (#1632 re-review nit) The re-review noted the -32602 disambiguation was symmetric: broad markers ("does not exist", "unrecognized") could inversely mislabel a genuine invalid-params message (e.g. "property 'region' does not exist") as "Unknown Tool". Scope the not-found/does-not-exist family to require the word "tool" in the same clause (matching the SDK's "Tool <name> not found"); keep "unknown tool" / "no such tool" as standalone unambiguous markers. Adds a tool-less-"does not exist" test asserting it classifies as invalid-params. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Add trigger_invalid_params showcase tool + Invalid Parameters screenshot (#1632) Adds a `trigger_invalid_params` spec-error trigger to the modern showcase server: the injector answers its `tools/call` with a real `-32602 Invalid params` JSON-RPC error whose message does NOT name an unknown tool. This lets the "Invalid Parameters" branch of the tool-call error panel (added in the review response) be exercised live, distinct from the unknown-tool `-32602`. - `SPEC_ERROR_TRIGGERS` gains the `trigger_invalid_params` entry (HTTP 200, in-band -32602) + preset registration. - Enable `injectSpecErrors` on `xmcpheader-modern-http.json` and add the tool. - README + pr-screenshots README document it; adds the live "Invalid Parameters" proof screenshot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
320daf2f67 |
Resources: subscriptions/listen model on modern servers (era fork) (#1630) (#1730)
Fork resource subscriptions by protocol era: legacy keeps resources/subscribe; modern (2026-07-28) models subscriptions as a filter over one long-lived subscriptions/listen stream with reconnect-by-re-listen (capped backoff), an optimistic Connecting→Listening header badge, and a fix so the browser proxy no longer blocks listen() ~60s. Closes #1630. |
||
|
Cliff Hall
|
b3b04c6939 |
Thread cacheMode through managed list refresh via SDK cache-aware verbs (#1721) (#1723)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
* Thread cacheMode through managed list refresh via SDK cache-aware verbs (#1721) Manual/auto refresh of tools/resources/prompts now forces a cache-bypassing round trip on modern servers, delivering the raw-request cacheMode control deferred from #1628. The InspectorClient list/read path uses the SDK's single-page `client.request` verbs (for pagination debugging), which never consult the SDK response cache — so `cacheMode` was inert there. This adds a separate aggregate path that routes through the SDK's cache-aware high-level verbs, where `cacheMode` is honored: - InspectorClient: new `listAllTools/Prompts/Resources/ResourceTemplates( { cacheMode, metadata })` that call `client.listTools()` etc. (all-page aggregate + SEP-2243 x-mcp-header exclusion + cache consult/write). A shared `getCacheableRequestOptions` / `aggregateListParams` back all four. - InspectorClientProtocol: the four `listAll*` methods added. - ManagedListState: delegates page walking to the SDK verb (config `fetchPage` loop → `fetchAll`); `refresh(metadata?, cacheMode?)` threads cacheMode. A `list_changed` auto-refresh bypasses the cache (`cacheMode: "refresh"`). - Managed hooks: user-initiated `refresh()` passes `cacheMode: "refresh"`, so the existing per-tab Refresh buttons become genuinely cache-bypassing — no App wiring change. Connect-time load stays default (`'use'`). The single-page `listTools(cursor)` primitive is unchanged; the paged hooks still surface per-page `nextCursor`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Single-page list pagination: sidebar toggle + server setting (#1721) Delivers the deferred single-page fetch UI: Tools/Resources/Prompts can be loaded one page at a time instead of auto-aggregating every page — a defensive option for servers with very large lists. - New `singlePageLists` server setting (persisted, server-wide), plumbed through ServerConfig/InspectorServerSettings, serverList convert/default, the node + remote settings paths, and the Server Settings form ("Fetch Lists One Page at a Time"). - New `ListPaginationControls` element (a "Single page" switch + "Load next page" button + "N pages loaded" status) rendered in each list sidebar; the toggle edits the setting live (optimistic) and persists it. - The paged state stores (PagedTools/Prompts/ResourcesState) now own loading: they auto-load page 1 on connect in single-page mode and track nextCursor + page count as observable state; the managed stores skip their all-page walk in single-page mode (so nothing is over-fetched) while still lighting the list-changed indicator. `usePaginatedList` selects the display source and pagination controls per mode — pure, no data-loading in React effects. - Refresh is mode-aware: reload page 1 in single-page mode, refetch the whole aggregate otherwise. Sidebars widened 340→360 so the controls don't crowd list entries. - Test server: `test-servers/configs/pagination-http.json` (12 tools/resources/ prompts, page size 4 → 3 pages each) exercises the flow end to end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Add pr-screenshots proving single-page pagination (#1721) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Address @claude review: template load, double-click guard, auth recovery (#1721) - Fix: resource templates never loaded on connect in single-page mode. The connect-time all-page skip now applies only to lists with a paged counterpart (new `deferInSinglePage` flag on ManagedListConfig; false for resource templates), so templates still aggregate on connect regardless of the setting. - Guard `PagedTools/Prompts/ResourcesState.loadPage` against a concurrent load (fast double-click on "Load next page") so the same page can't be appended twice; the in-flight load owns the cursor and the second call is a no-op. - Thread ambient auth recovery through the single-page refresh, Load-next-page, and mode-switch loads (they previously bypassed it) — usePaginatedList's onRefresh/onLoadMore now return their promises so App can wrap them. - Drop the misleading "aggregated list" wording from the paged-store docs. Tests added for each; npm run coverage green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Review round 2: revert optimistic toggle when persisting the setting fails (#1721) Non-blocking review follow-up. If updateServerSettings rejects, the optimistic `singlePageListsOverride` is now cleared and the live client setting rolled back (the effect only clears the override when the persisted value changes, which doesn't happen on failure), so the UI and client reflect what's on disk rather than the failed edit. Added an App test for the revert path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Fix single-page Refresh not clearing list-changed indicator + UI polish (#1721) Round-3 review fix (blocking): in single-page mode a `list_changed` lights the list-changed indicator, but the single-page Refresh reloads page 1 of the paged store and bypassed `useManaged*.refresh()`, so the indicator was never cleared. Expose `clearListChanged` from useManagedTools/Prompts/Resources and call it in the single-page branch of onRefreshTools/Prompts/Resources. Tests added. UI polish (user feedback on the sidebar control): - Rename the switch label "Single page" → "Paginated". - Put "Load next page" on the same row as the switch, space-between (switch fully left, button fully right). - Move the "N pages loaded" status to its own line below the row, centered. - Hide the whole control once the list is known to be a single page (single-page mode, page 1 loaded, no nextCursor) — nothing to paginate. pr-screenshots refreshed to the new layout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Rename the singlePageLists setting to paginatedLists for clarity (#1721) `singlePageLists` read backwards from the "Paginated" switch label (it sounds like paginated=false). Rename the server setting to `paginatedLists` with the same value meaning — true = paginated/one-page-at-a-time, false = aggregate-all — and follow through on the related identifiers for consistency: - Setting field `paginatedLists` across ServerConfig / InspectorServerSettings, serverList convert/default, node + remote settings paths, and the form/modal callbacks (onPaginatedListsChange). - `usePaginatedList` / `ListPaginationControls` prop `singlePage` → `paginated`, `onSinglePageChange` → `onPaginatedChange`. - `ManagedListConfig.deferInSinglePage` → `deferWhenPaginated`. - App state (paginatedListsOverride, onTogglePaginatedLists, …) and all tests/fixtures + mode-referring comments. "single-page" wording that denotes the SDK's one-page `client.request` verb (inspectorClient) is intentionally left as-is. Purely a rename — no behavior change; unit + integration + cli + tui + coverage all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Format App.test.tsx (prettier) after the paginatedLists rename (#1721) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
e026051b47 |
Era-aware connection model & capability gating (#1626) (#1697)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
* Era-aware connection model & capability gating (#1626) Make protocol era (SEP §7.8) a first-class, per-server connection dimension, orthogonal to the transport type, and drive the SDK Client's versionNegotiation from it. Config - Add `ServerProtocolEra` ("legacy" | "auto" | "modern"), `DEFAULT_PROTOCOL_ERA` (legacy, per the SDK's don't-auto-probe guidance), `MODERN_PROTOCOL_VERSION`, and `eraToVersionNegotiation()` in core/mcp/types.ts. - Persist `protocolEra` on `StoredMCPServer` / `InspectorServerSettings`, round-tripping through serverList.ts (omitting the default on disk) and the `/api/servers` read + write validators. InspectorClient / state - Thread `versionNegotiation` through `InspectorClientOptions` (default legacy), replacing the hardcoded pin. After connect, read `getProtocolEra()` / `getDiscoverResult()` / `getNegotiatedProtocolVersion()`, expose them via getters, dispatch `protocolEraChange` / `discoverResultChange`, and clear on disconnect. `useInspectorClient` surfaces `protocolEra` / `discoverResult`. Web UI - ServerSettingsForm: a Protocol Era selector bound to the per-server setting. - ConnectionInfoContent: era-aware — Era badge, Session ("Sessionless" on modern), and a Discovery section (supported versions + extensions) from the `server/discover` result. CLI/TUI also map the mcp.json `protocolEra` into versionNegotiation so the field applies uniformly. Version negotiation stays fully client-side; modern connect works over the existing remote tunnel and disconnect is sessionless (the SDK skips DELETE when there is no session id). Mcp-* header fidelity is left to the History/Network card. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Address @claude review: era docstrings, era validation, stdio session label, AGENTS.md rule - Fix stale docstrings/comments in inspectorClient.ts, useInspectorClient.ts, inspectorClientEventTarget.ts, and ConnectionInfoContent.tsx: getProtocolEra() returns "legacy" (not undefined) on a plain legacy connect; it is undefined only when not connected. - Validate protocolEra at the storedFieldsToInspectorSettings read boundary (new shared isProtocolEra guard) so a hand-edited mcp.json read directly by the CLI/TUI drops an unknown era instead of passing it to eraToVersionNegotiation. Keeps the exhaustive switch intact. - ConnectionInfoContent: the Session row is HTTP-only — show "N/A (stdio)" for stdio instead of the misleading "Session-based". - AGENTS.md: add a rule against unjustified `as unknown as` double casts, and drop the one such cast introduced in the serverList test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Address re-review: dedupe isProtocolEra guard in remote server Replace the local isProtocolEra in remote/node/server.ts with the shared guard now exported from serverList.ts (already imported here), so the era literal set lives in one place and can't drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
cliffhall
|
f9f79fff54 |
Address @claude review: format core/, typed output-validation error, document progress heuristic
- Run Prettier on the 60 core/ files this PR touched — the deterministic import rewrite had collapsed multi-line imports and left throws unwrapped; core/ is in no format gate so it slipped past CI. Now Prettier-clean. - Default-path output-schema violation now throws ProtocolError(ProtocolErrorCode.InvalidParams, …) instead of a bare Error, matching the prior v1 contract (SDK threw a typed McpError with a code) so downstream `instanceof`/`.code` branching keeps working. - Document the "assumes serial construction" invariant on pollTaskToolCall's progress-subscription revival (the `_progressHandlers` new-key heuristic). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
cliffhall
|
19a6df7ad3 |
SDK v2 migration (behavior-neutral, versionNegotiation: 'legacy') (#1624)
Migrate all three clients + core + test-servers from
@modelcontextprotocol/sdk@1.29 to SDK v2 (2.0.0-beta.4), keeping behavior
identical against legacy (2025-11-25) servers. Foundation for the SDK V2 +
New Spec stack.
Packages
- Split @modelcontextprotocol/sdk -> @modelcontextprotocol/client (Client,
transports, OAuth, error taxonomy, protocol types) + core (Zod schemas);
stdio -> client/stdio, Ajv -> client/validators/ajv. Test-servers ->
@modelcontextprotocol/server (+ server-legacy/sse). Deterministic import
rewrite (schemas -> core, everything else -> client); the codemod misroutes
client code to /server so it was not used for path routing.
- new Client(..., { versionNegotiation: { mode: 'legacy' } }) — nothing
2026-era goes on the wire.
Runtime (core/mcp/inspectorClient.ts)
- Error taxonomy: McpError -> ProtocolError, ErrorCode -> ProtocolErrorCode;
RequestTimeout -> SdkError/SdkErrorCode.
- Handlers take method strings; task methods (tasks/*, notifications/tasks/*)
use the 3-arg custom form (excluded from v2's spec-method set).
- Legacy tasks kept working (per project requirement): requestor tasks re-
implemented over raw client.request + explicit deprecated schemas;
experimental.tasks.callToolStream replaced by a local pollTaskToolCall
async generator (with _progressHandlers revival so task-execution progress
still reaches the caller). Receiver (server-initiated) tasks: the v2 Client
validates a sampling/elicit handler result and rejects the legacy { task }
response with -32602 — installReceiverTaskResponseBypass swaps the wrapped
entry in the private _requestHandlers map for the task-augmented branch so
{ task } reaches the wire (fixes server-initiated sampling-via-task).
- Pagination: list verbs use raw client.request({method}, List*ResultSchema)
for single-page behavior (v2's client.listX() auto-aggregate all pages).
Web / auth / apps
- Rename local JsonSchemaType -> InspectorFormSchema + add toFormSchema()
structural narrowing at the SDK-schema -> form boundary.
- ext-apps still peers on SDK v1: one compat boundary with documented casts
(remove when ext-apps#702 lands a v2 peer).
- OAuth provider saveClientInformation reconciled with v2's
OAuthClientInformationContext; InvalidClientMetadataError re-homed as a
local class (server-legacy/auth is not browser-safe).
- vitest.shared.mts dedupes @modelcontextprotocol/client+core to one copy so
vi.mock intercepts the SUT's import.
Accepted v2 behavior changes (per #1624 / spec §9.2): unknown-tool call
rejects with -32602; cancellation is stream-abort (no guaranteed
notifications/cancelled frame); output-schema strictness restored via the
Inspector's own Ajv check on the default path.
npm run ci green: validate (web 3087 / cli 212 / tui 274 / launcher 5),
coverage (>=90 per-file, all clients), smoke (launcher/cli/web), storybook
(410).
Closes #1624
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
cliffhall
|
650e5bd020 |
feat(web): show the Inspector version in the lower-right corner (#1639)
Display the single-source version (root package.json) in the web UI's lower-right corner, in grey — matching the CLI and TUI. The browser can't read the filesystem, so the version flows backend → browser via GET /api/config: - Add `version?: string` to InitialConfigPayload; the web backend (webServerConfigToInitialPayload) reads it once via the shared readInspectorVersion() and tags it onto the payload. - Add a `useInspectorVersion` hook mirroring useSandboxUrl/useServerListWritable. - Add a presentational `VersionBadge` element rendered fixed lower-right, grey, non-interactive; styling lives in a `versionBadge` Text theme variant (no inline styles, --inspector-* tokens). - Wire it into App.tsx. Verified live: /api/config serves `version: "2.0.0"` and the badge renders `v2.0.0` fixed at the lower-right in grey. Tests: useInspectorVersion (full branch coverage), VersionBadge (unit + Storybook), web-server-config payload. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw |
||
|
cliffhall
|
ba48e06ffd |
fix(auth): guard OAuth-storage async paths against unhandled rejections
Review follow-ups on the RemoteOAuthStorage web migration: - useEmaIdpLoginState.logout(): add a .catch to the now-async clearEmaIdpSession call so a storage failure no longer surfaces as an unhandled rejection; leave loginState unchanged on failure (the session was not actually cleared). Add a test asserting no unhandled rejection. - createOAuthStore: attach a no-op catch to the eager persistLoadPromise so a hydration read that fails before any load() awaits it does not become an unhandled rejection; real awaiters still observe the rejection via the shared reference. - storage-remote.test: replace `as unknown as typeof fetch` double-casts with the typed vi.fn<typeof fetch>() pattern the same PR established. - remoteOAuthStorage: correct the doc comment to describe the cache-of-one accurately instead of implying a keyed map. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
Bob Dickinson
|
a24091484c | fix(auth): await OAuth storage load() to fix remote OAuth races | ||
|
Bob Dickinson
|
13dafe457b |
Guided-auth removal, OAuth smoke testing, and TUI/CLI support (#1531)
* refactor(web): update OAuth E2E tests to use new authentication method - Replaced `runGuidedAuth` with `authenticate` in the InspectorClient OAuth end-to-end tests to support quick authentication with CIMD pre-registration. - Updated comments to reflect the change in authentication method and its support for HTTP test metadata URLs. - Ensured that the tests continue to validate the expected authorization URL after the change. Additionally, imported `ensureCimdClientRegistration` in the OAuthManager to streamline client registration during the authentication process. * Guided-auth removal, OAuth smoke testing, and TUI/CLI support * Added CLI OAuth runner flags tests * Added clarifying comment on port 0 binding * README notes on TUI oauth callback port, prettier fixes for CLI tests * CIMD validation uses inline like EMA IdP * Fixed TUI test failures from merge * test: bring 5 OAuth files over the 90 per-file coverage gate The guided-auth-removal work added/touched several files that fell below the repo's >=90 per-file coverage gate (npm run coverage). CI doesn't run the gate (it's local-only), so validate + test:integration passed while these gaps slipped through. Bring them all back over the line: - core/auth/node/runner-oauth-callback.ts: test bad-URL/non-http-scheme throws, no-port->80 default, and IPv6 hostname bracketing. Annotate the provably-dead guards (empty host, out-of-range port, String(err) fallback, pathname "/" fallback) -- all rejected by new URL() upstream -- with justified v8 ignore comments instead of lowering the gate. - core/auth/cimd.ts: test the AS-doesn't-support-CIMD early return. - core/react/useClientSettingsDraft.ts: test the functional-updater path, the stale-onChange-after-close prev===null guard, and the timer-fires-after-close value===null guard. - ClientSettingsForm.tsx: test the CIMD checkbox toggle and CIMD URL clear. - clients/tui/src/utils/oauthDisplay.ts: test static/dcr registration kinds, the expired IdP session, and the configured-scope/empty-scope branches of formatScopes. npm run coverage now passes (exit 0) across web, cli, tui, launcher. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: cliffhall <cliff@futurescale.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Bob Dickinson
|
5de4e911e3 |
feat(web,core): enterprise-managed authorization (EMA) (#1509) (#1512)
Adds enterprise-managed authorization (EMA): browser-side IdP login + RFC 8693 token-exchange and JWT-bearer resource-token mint, per-server EMA opt-in in Server Settings, IdP config in Client Settings, and IdP clientSecret keychain storage. Includes the v2/main merge and review follow-ups (400 on malformed client config; normalizeIdpIssuer import consolidation). |
||
|
Cliff Hall
|
97b1a7fbff |
feat(web,core): import client configs and registry server.json (#1348) (#1511)
* feat(web,core): import client configs and registry server.json (#1348) Wire up the two server-import affordances that were previously todoNoop: - Import config: source picker (Claude Desktop / Cursor / Cline / VS Code) read on the backend via a new GET /api/import-source, plus a file-upload fallback. Per-server review with import/skip on new servers and overwrite/skip/rename on id conflicts, then a per-server outcome summary. - Import server.json: parse the MCP registry single-server format (npm/pypi/oci packages + remotes, env vars, package/runtime args, URL template vars) into a runnable config, with a file picker and debounced live validation; Add Server is disabled until validation passes. Core gains an isomorphic import strategy layer (core/mcp/import) with a strategy registry, client-config + server.json parsers, a strategy-agnostic merge, and a dependency-injected source resolver. useServers exposes importSource(). Modal headers are bold and the redundant server.json section header is folded into the modal title. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): collapse the server.json textarea into a File Contents disclosure Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): auto-collapse File Contents disclosure after content loads Once content is pasted or a file is loaded, collapse the File Contents disclosure after ~1s with Mantine's built-in accordion animation, signaling the input was accepted and surfacing the validation/env-var sections. The disclosure becomes controlled by the modal; clearing the textarea re-opens it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): flash File Contents hover highlight before auto-collapse Before the File Contents disclosure auto-collapses, paint it with its hover background for 250ms, then run the collapse over a 325ms animation, so the collapse reads as intentional. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): rename "Import server.json" to "Import from registry config" Rename the Add Servers menu entry and the modal header. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): rename "Import config" to "Import client config" Rename the Add Servers menu entry and the modal header. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(web,core): move import orchestration into core helpers + web hooks Make the import logic reusable by CLI/TUI and keep the modals display-only: - core/mcp/import: add parseClientConfig (mcpServers→servers fallback), resolveServerId, selectServerJsonOption, and buildServerConfigForSelection, so the full import (parse → select → build → merge) is callable from any environment. - core/storage: extract the pure validateStoreId into a Node-free store-id module (store-io re-exports it) so the isomorphic import layer can validate ids without pulling Node deps into the browser bundle. - clients/web/hooks: useServerJsonImport and useImportClientConfig own all the wizard/parse/debounce/apply state; ServerImportConfigModal and ServerImportJsonModal are now thin display shells over the dumb panel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): rename "Import client config" to "Import from client config" Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): remove the Cancel button from the registry import modal The modal header X (and Escape) still dismiss it; drop the redundant Cancel button (and the panel onCancel prop). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): hide validation results + name override until content is present In the registry import modal, the Validation Results section and the Server Name override field stay hidden until something has been pasted/loaded into the textarea. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): make Validation Results a collapsible disclosure Wrap the registry import modal's validation results in a Mantine accordion disclosure (default open), matching the File Contents section. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): Server Name disclosure with read-only derived name + override Make Server Name a collapsible disclosure containing a read-only "From configuration" field (the id derived from the server.json name) and the editable Override field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): scroll to and highlight a freshly-added server After a manual add/clone or an import (client config / registry), the new server's card scrolls into view and draws an animated border (Mantine Border Animate); clicking anywhere on the card dismisses it. - App tracks highlightedServerId; addServerHighlighted sets it (edits and conflict-overwrites don't). Threaded through InspectorView -> ServerListScreen -> ServerCard. - ServerCard scrolls into view on highlight and wraps in <BorderAnimate>; onClick clears the highlight. - Pin @gfazioli/mantine-border-animate@2.0.3 (Mantine 8 compatible; 2.0.4+ requires Mantine 9). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web): highlighted card fills its cell and its controls work on first click - The BorderAnimate wrapper is display:flex, so the card was shrinking to content width while the border spanned the cell. Stretch the card with w=100% (and the wrapper). - Always render BorderAnimate and toggle show/animate instead of conditionally wrapping: swapping the element type on clear remounted the card, swallowing the click that triggered it (e.g. the connect toggle fired clear but not connect, needing a second click). Verified: one click now connects + clears. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web,core): address review — OCI env, untrack fixture, rename validation - Untrack the stray inspector-v1-multi.server.json (accidentally committed) and gitignore root *.server.json fixtures. - core/mcp/import/serverJson: forward declared env vars into OCI containers via '-e KEY' docker args (they previously only set the docker CLI process env); add a test. Clarify buildServerConfig (destructure env out instead of spread-then-delete) and document value-vs-default env handling. - useImportClientConfig: validate 'rename' targets inline (invalid charset / collision with existing, additions, or other renames) and gate the Import button on canImport; surface the error on the field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): highlight all servers from a batch import, not just the last Track highlighted servers as a set of ids instead of one. addServerHighlighted accumulates each id (a multi-server client-config import highlights them all); opening an add/import modal starts a fresh batch. Clicking a card clears only that card's highlight; only the first highlighted card scrolls into view. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
91f02fd6db |
feat(web): catalog vs read-only session launch model + apply --header (#1481, #1483) (#1496)
* feat(web): catalog vs read-only session launch model + apply --header (#1481, #1483) Per specification/v2_catalog_launch_config.md, `mcp-inspector --web` now selects the backend's server-list source and a session-wide writability, instead of seeding an inert `initialMcpConfig` the UI never read: - `--catalog <path>` (and `MCP_CATALOG_PATH`) → active WRITABLE catalog (seed/CRUD/watch), like the default catalog. - `--config <path>` → READ-ONLY session file: served in the UI but never written, seeded, or plaintext-secret-migrated — safe to point at a foreign config (e.g. claude_desktop_config.json). - ad-hoc `--server-url`/command (+ `--header`) → one server held IN MEMORY (no file written); `--header` is lifted onto the entry so the seeded connection actually uses it (closes the #1483 warn-only no-op). - nothing → default catalog (writable), unchanged. Backend (`createRemoteApp`): new `writable` + `initialServers` options. GET /api/servers branches three ways (in-memory / read-only file with no seed+no migrate / writable catalog); the four mutating routes return 403 when read-only; `/api/config` carries `writable`; the file watcher is skipped for in-memory sessions. `--catalog`/`--config` are mutually exclusive and neither combines with an ad-hoc target or `--header`; `--header` requires an ad-hoc HTTP/SSE server (clear errors otherwise — no silent no-op). UI: `useServerListWritable` reads `writable` from /api/config; the Servers screen hides add/edit/clone/remove/reorder/settings and shows a read-only banner when false. All new props default to writable=true (back-compatible). Docs: spec gaps G2/G3/G4 updated; launcher README documents the flags. Verified end-to-end against the built launcher: --config (writable:false, file byte-stable, mutations 403), ad-hoc --header (in-memory, header lifted), and --catalog (writable:true, POST persists). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(web): address review — Mantine <Code> in read-only banner, clarify watcher comment - Replace raw <code> with Mantine <Code> in the read-only-session Alert (repo convention: prefer Mantine primitives over raw HTML). - Add a comment near `watchable` clarifying that a read-only `--config` file is still watched on purpose (read-only blocks UI writes, not external observation; `writable` does not gate watching). Deferred (per reviewer): consolidating useSandboxUrl + useServerListWritable into one useConfig hook to avoid the duplicate /api/config fetch — optional follow-up. The redundant ad-hoc header type-guard is kept as defensive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
b4716a1405 |
feat(core): surface mid-session transport failures via lastError (#1489)
Closes #1323. Adds an `error`-event-driven `lastError` on useInspectorClient and a mid-session failure toast in App.tsx; the transport onerror is scoped to non-handshake (status !== connecting) so handshake failures stay on the connect() rejection path. Follow-up: #1490. |
||
|
Cliff Hall
|
d5f12b2a82 |
feat(web): reorderable ServerCards — drag-and-drop + keyboard (#1369) (#1447)
* feat(web): reorderable ServerCards — drag-and-drop + keyboard (#1369) Let users control the order of servers on the Servers screen instead of hand-editing mcp.json. Each ServerCard gets a grip handle (in the header, left of the name) that drives an @dnd-kit sortable; pointer and keyboard sensors both work, with live-region announcements for screen readers. - Backend: PUT /api/servers/order rewrites mcp.json in the supplied order inside the write lock, reusing the atomic-write + watcher-notify path. Registered before /:id so "order" isn't captured as a server id. Rejects with 409 when the id set no longer matches disk and 400 on duplicate / malformed bodies, so a reorder racing an external edit can't drop or duplicate an entry. - Hook: useServers.reorderServers(orderedIds) — optimistic local reorder, PUT, refresh-to-disk-truth on failure. - UI: SortableServerCard wrapper owns the dnd-kit concerns; ServerListScreen wraps the grid in DndContext + SortableContext (rect strategy) with pointer + keyboard sensors and announcements. Reorder math and a11y copy extracted to reorderIds.ts / serverReorder.ts for direct unit testing. Renders plain cards (no grip) when no onReorder is wired. - Wired onServerReorder through InspectorView -> App.tsx. Tests: pure-helper units, hook (persist / optimistic / 409 revert), 9 backend integration tests (incl. id-set-mismatch rejection and the route-registration guard), ServerCard/ServerListScreen units, and a real-browser Storybook keyboard-reorder play (the dnd-kit keyboard sensor needs layout rects that happy-dom doesn't provide). Adds @dnd-kit/core, @dnd-kit/sortable, @dnd-kit/utilities. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web): notify on reorder failure; skip reorder glue when not reorderable Address PR #1447 review: - App.tsx: catch reorderServers rejection (409 from a racing external edit, or a network error) and show a red toast, matching the error-handling pattern used by every other mutation — previously the rejection was discarded with `void` and the drag silently bounced back. - ServerListScreen: only build handleDragEnd / announcements when `reorderable`, so the four-closure announcements object isn't allocated every render when no onReorder is wired. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
b0a024269e |
feat(web): list-changed handling — indicator, pull-on-demand default, and per-server auto-refresh opt-in (#1402) (#1443)
* feat(web): wire the listChanged indicator across all screens (#1402) The per-screen list-changed indicator was hardcoded false in InspectorView for Tools, Apps, Prompts, and Resources, so it could never light up. Source it from the managed-state layer instead. Each ManagedXState now tracks a listChanged flag: set true when its */list_changed notification arrives (the existing auto-refresh leaves it set), exposed via getListChanged()/a listChangedChange event, and cleared by clearListChanged() or on disconnect. The useManagedX hooks surface the flag and clear it on a user-initiated refresh (the indicator's Refresh button = acknowledge). App threads the three flags to InspectorView, which maps them to the screens — Apps shares the tools flag since apps are a filtered view of tools. Tests cover set/clear/disconnect at the state-manager layer, the hook subscription + refresh-clears behavior, and the InspectorView routing (including Apps sharing the tools flag and no cross-wiring between screens). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(core): close clear-after-await race in managed-list refresh (#1402 review) The useManagedX refresh() callbacks cleared the listChanged flag AFTER awaiting the fetch. If a */list_changed arrived mid-fetch, the state re-set the flag (and auto-refreshed), then the trailing clear wiped that genuinely-new signal. Clear BEFORE the await instead: the user's refresh acknowledges only the change in hand, and a mid-fetch change re-sets the flag and survives. Adds a race regression test on the tools hook. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web): drop duplicate Refresh on the Apps screen (#1402) AppControls uniquely had a standalone toolbar Refresh button AND the ListChangedIndicator on a separate row, so once the indicator could actually appear (this PR), the Apps screen showed two Refresh buttons. Match the other three controls (Tools/Prompts/Resources): the indicator is the single header-right refresh affordance. Updates AppControls and AppsScreen tests to assert exactly one Refresh (and none when the list hasn't changed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(core): stop auto-invoking */list on list_changed; user pulls via Refresh (#1402) With auto-refresh on, the list was already current by the time the list-changed indicator appeared, making its Refresh button moot. Now the managed Tools/Prompts/Resources state managers only flag the change on */list_changed (they no longer auto-refresh); the list stays as-is until the user clicks Refresh, which fetches the new list and clears the indicator. The connect-time initial load is unchanged. Resource templates (no own indicator, part of the Resources screen) also stop auto-refreshing; onRefreshResources now refreshes both resources and templates, since a single resources/list_changed covers both. Tasks are left auto-refreshing — they have no list-changed indicator and need live status updates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(core): correct ManagedResourceTemplatesState header comments (#1402 review) The class no longer subscribes to resourceTemplatesListChanged; update the stale file/class docstrings that still claimed it syncs on that notification to reflect connect + statusChange only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): add per-server 'Auto Refresh on List Changed Notifications' option (#1402) New Options section in ServerSettingsForm with a single checkbox. When checked, the managed Tools/Prompts/Resources (and resource templates) state auto-refreshes on */list_changed notifications; when unchecked (default) the notification only lights the list-changed indicator and the user pulls via Refresh. Plumbing: new optional autoRefreshOnListChanged on InspectorServerSettings and StoredMCPServer, round-tripped through serverList (lifted with a false default, persisted only when true, added to the exhaustive field-key map); surfaced from InspectorClient via a new getServerSettings() on the protocol; the state managers read it live at notification time to decide auto-refresh vs flag-only. Resource templates re-subscribe to their list_changed but only auto-refresh when the option is on (they have no indicator of their own). Tests cover the round-trip (true persists, false omitted), the form section (render/checked/toggle), and each state manager's on/off branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(server): persist autoRefreshOnListChanged through validateSettings (#1402) The PUT /api/servers write path runs settings through validateSettings, which rebuilds the value from explicitly-named fields and silently drops unknown keys — so autoRefreshOnListChanged never reached disk (the serverList round-trip test passed because it calls inspectorSettingsToStoredFields directly, bypassing this path). Validate it as an optional boolean and include it in the built value; false is omitted on disk by inspectorSettingsToStoredFields as before. Adds integration tests over the real PUT path: true persists, false is omitted, and a non-boolean is rejected (400). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): default the Server Settings modal to the Options section (#1402) Open 'Options' instead of 'Custom Headers' when the Server Settings modal first opens, so the new Auto Refresh toggle is the first thing surfaced. Updates the modal default-expanded section, the modal test's initial-state assertions, the three header tests (now expand 'Custom Headers' first since it's no longer open by default), and the story defaults. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: drop accidentally-committed Inspector history dump; gitignore the class 83076441 swept an exported Inspector message-history JSON (manual-testing artifact at the repo root) into the tree via git add -A. Untrack it and extend the existing repo-root debug-artifact .gitignore block (alongside /*.png) to cover /inspector-history-*.json so these can't be re-added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
9b558afee1 |
feat(web): display negotiated MCP protocol version on ServerCard (#1324) (#1442)
* feat(web): show negotiated MCP protocol version on ServerCard (#1324) Add optional protocolVersion to ConnectionState and render it as a dimmed "MCP <version>" label at the end of the transport-badge/ description row on ServerCard, shown only when connected. The value is plumbed through useInspectorClient as part of #1324. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(core,web): surface negotiated protocolVersion end-to-end (#1324) Plumb the MCP protocol version negotiated during initialize from the InspectorClient through to the ServerCard so the active connected card displays "MCP <version>". The SDK Client doesn't expose protocolVersion; it stamps it onto the transport via setProtocolVersion. MessageTrackingTransport now captures it (and forwards to the base transport when present), so it works for stdio too — whose base transport has no setProtocolVersion. - core: MessageTrackingTransport captures + exposes protocolVersion; InspectorClient stores it, exposes getProtocolVersion(), dispatches a protocolVersionChange event, and clears it on disconnect. - core: useInspectorClient subscribes and returns protocolVersion. - web: App reads it from the hook (drops the hard-coded "2025-06-18") and InspectorView splices it onto the active server's connection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(web): decouple initializeResult from protocolVersion (#1324 review) Gate the initializeResult memo on serverInfo only, not protocolVersion. initializeResult also drives the connected ViewHeader and Connection Info modal, so requiring the version risked hiding the whole connected surface in the edge case where it's absent. protocolVersion now flows through as the optional field it already is everywhere downstream ('' reads as unknown; the ServerCard label and modal value both tolerate it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(web): drop redundant connected-check in InspectorView splice (#1324 review) initializeResult is derived from connectionStatus in App's memo, so a truthy initializeResult.protocolVersion already implies a live connection — the extra connectionStatus === 'connected' guard in the splice was redundant. ServerCard keeps its own status guard (dumb- component robustness). No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
ed6eda7492 |
feat(web): wire up MCP task support — Tasks screen, Run-as-task, status toasts (#1422) (#1424)
Wires the ported Tasks screen + core task subsystem through App.tsx: sticky Clear Completed, cancel/refresh error toasts, core-side per-task progress correlation, Run-as-task on the Tools screen (gated on server support), per-server Task TTL, and live task-status toasts. |
||
|
Cliff Hall
|
8f6a2e0f78 |
feat(web): render pending sampling/elicitation requests (#1407) (#1411)
* feat(web): render pending sampling/elicitation requests (#1407) Server-initiated sampling and elicitation requests never reached the v2 web UI: the InspectorClient queued them and returned a Promise that only resolved on a user response, but nothing subscribed to the pending-change events or rendered the (already-built) request components. A tool that triggered one spun the Execute button forever. Wire the queues to the UI: - Add usePendingClientRequests() (core/react) subscribing to pendingSamplesChange / pendingElicitationsChange and returning the live arrays. Expose getPendingSamples() / getPendingElicitations() on InspectorClientProtocol (and the test fake) so hooks can read them. - Add PendingClientRequestModal, an App-level modal that surfaces one queued request at a time (sampling-first) via the existing SamplingRequestPanel / ElicitationFormPanel / ElicitationUrlPanel. It is non-dismissable so the blocked call only resolves via an explicit action. - Wire it in App.tsx: responding/rejecting resolves the client's handler Promise, which unblocks the originating call and clears the spinner. Also fold in two issues surfaced while verifying: - SchemaForm rendered an `array` + `items.enum` (multi-select enum) as a JSON textarea because only `items.anyOf` had a branch; add a MultiSelect branch with enumNames label support. - ToolsScreen's result lives in App and outlived the screen's local selection (reset on tab-switch unmount), leaving a result with no selected tool. Clear the result on unmount for a clean slate on return. Prompts/Resources gate their preview on selection, so they are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(elicitation): support explicit decline; honest queue label; review polish Address @claude review on #1411: - Add an explicit "Decline" action to ElicitationFormPanel so the spec's decline (refusal) is distinguishable from cancel (dismissal); the JSDoc claimed accept/decline/cancel but only accept/cancel were wired. Wire the modal body to emit { action: "decline" }. - Replace the misleading "1 of N" queue label (the leading 1 never changed since the modal always shows the head) with an "N pending" hint, hidden when nothing else is queued. - Make the dead useMemo honest: derive the head request inside the memo from the source arrays so it actually caches (the inline activeElicitation had a fresh identity every render). - Document the URL elicitation accept-on-open semantic with a comment pointing at the follow-up (#1415). - Add an App-level round-trip test driving pendingSamplesChange end-to-end (modal opens, respond() fires, empty event closes it). Follow-ups filed: #1415 (URL two-step completion flow), #1416 (form required- field submit gate + double-submit guard). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(elicitation): include untouched schema defaults in submitted content The schema form displays a field's default via resolveValue(), but onChange only writes a field into `values` once the user edits it. So an elicitation submitted with default-only fields left untouched dropped them entirely — v2 sent just the manually-edited fields while v1 sent defaults too (reported with the everything server: firstLine/integer/number/enum defaults missing). Add collectSchemaDefaults() to jsonUtils (recurses nested objects, omits fields with no default) and seed ElicitationFormModalBody's values state with it via a lazy initializer. The modal body is keyed by request id, so it re-seeds per request; user edits still override seeded defaults via the existing onChange merge. Tests: collectSchemaDefaults unit tests (field types, nested, empty) in jsonUtils.test; a modal test asserting an untouched-default form still submits its defaults in content. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(forms): seed schema defaults for tool-call and app-launch forms too Extend the elicitation defaults fix (collectSchemaDefaults) to the other two SchemaForm consumers that own form state and submit it: ToolsScreen (tool execute args) and AppsScreen (app launch args). Both reset formValues to {} on selection, so default-only fields the user never edited were dropped from the submitted args — same root cause as the elicitation modal. Seed formValues from the selected tool/app inputSchema defaults on select. User edits still override via the existing onFormChange merge. (InlineElicitationRequest has no consumers wired up, so it's left as-is.) Tests: ToolsScreen sends a tool's default on Execute without editing; AppsScreen sends an app's default on Open App without editing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(elicitation): gate form submit on required fields; guard double-submit (#1416) Fold the #1416 hardening into this PR: - Gate the elicitation form's Submit button on required fields. Promote the hasMissingRequiredFields helper out of AppDetailPanel into jsonUtils (shared) and use it in ElicitationFormPanel so Submit is disabled until every required field has a value. - Guard against double-submit. Add an optional `busy` prop to the sampling, elicitation-form, and elicitation-url panels that disables their responding actions. PendingClientRequestModal's bodies use a new ref-guarded useRespondOnce() hook: the first action dispatches and locks the panel; later clicks no-op. This stops a fast double-click from calling respond()/reject() twice (which throws and produced an unhandled rejection). Copy URL stays enabled — it doesn't resolve the request. Tests: hasMissingRequiredFields unit tests; ElicitationFormPanel submit-gate and busy-lock tests; a modal double-click-resolves-once test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(tools): cap detail/result panels at viewport and scroll internally The Tools screen's detail and result columns sized to their content and bled past the bottom of the viewport on long output (e.g. the env dump or a long elicitation result). Cap each column at the screen's available height and let its panel scroll internally instead. - ToolsScreen: drop `align: flex-start` and wrap each column in a stretched `ContentPane` (Flex column, capped by `mah` = viewport − header − xl*2). The pane's definite height is what lets the card's inner ScrollArea know how far to shrink — a bare `mah` on the card does not (the card sizes to content, so the scroll region renders full-height and overflow:hidden just clips it, unreachable). Mirrors the proven Prompts/Resources preview panes. - ToolResultPanel: pinned header + inner ScrollArea (flex 0 1 auto, mih 0). - ToolDetailPanel: pinned title/annotations header, scrolling body (description + form + progress), pinned Execute footer. Audited the other screens: Prompts/Resources already use this pattern, ServerList and the four list panels (Logging/Network/History/Tasks) already cap via ScrollArea.Autosize. Only AppsScreen's app-input form had the same gap — fixed it the same way: AppDetailPanel now scrolls internally and the card uses variant="preview" so the running app's iframe and a long form both stay within the viewport. Verified in-browser against the everything server: the get-env result now stops at the viewport bottom with margin and scrolls within the card (clientHeight 502 vs scrollHeight 1931), header pinned. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(remote): use sync pino destinations in logging tests to fix teardown race CI flaked with an uncaught `ENOENT: ... /tmp/inspector-log-unknown-level-*/app.log` that failed an unrelated later test. The remote-logging integration tests create `pino.destination(logFile)` loggers (async sonic-boom streams) and rmSync the temp dir in finally/afterEach without closing the stream — a buffered background flush then fires after the dir is gone and throws an uncaught error, which Vitest counts as a run failure even though all tests pass. Make those destinations `sync: true` so the write lands during the request instead of on a later flush, removing the race. Test-only; unrelated to this PR's feature work but it was blocking the PR's CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
5fb2b503f4 |
feat(apps): render running MCP Apps end-to-end (#1370) (#1403)
* feat(apps): render running MCP Apps end-to-end (#1370) Wire the v2 Apps screen so clicking Open App actually runs an MCP App. Previously App.tsx stubbed the three load-bearing pieces (sandbox URL, bridge factory, action handlers), so the iframe loaded about:blank and nothing crossed the bridge. - Add useSandboxUrl hook (core/react) to read sandboxUrl from GET /api/config and thread it down as sandboxPath; render an "MCP Apps unavailable" state instead of a blank iframe when absent. - Add createAppBridgeFactory: builds a host-side ext-apps AppBridge over the active client's SDK client, reads the tool's UI resource on the sandbox handshake and pushes it into the proxy, handles openLinks, and connects a PostMessageTransport. - AppRenderer: pass the tool to the bridge factory; buffer tool input/result and release them once the view signals `initialized` (input before result, latest-wins). - Move the AppRenderer handle ref to App.tsx so onOpenApp issues tools/call with the form values and delivers input + result to the running app via the bridge; onCloseApp tears the bridge/iframe down. - Stories: themed sandbox placeholder (visible in light/dark), an echo-bridge running story, and an unavailable story. New unit tests for the factory, the buffering, useSandboxUrl, and the empty state. The listChanged indicator badge wiring is tracked separately in #1402. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(apps): make MCP Apps render in dev — single bridge + result passthrough Two bugs kept apps (e.g. NightRider's MIDI Pattern Editor) stuck on an empty shell even though the host fetched the resource and called the tool correctly. 1. StrictMode double-mounted the bridge. The dev-only effect setup→cleanup→ setup built TWO host bridges/transports, double-loaded the sandbox, and raced the app's ui/initialize handshake (it timed out, so `initialized` never fired and buffered input/result were never flushed). AppRenderer now defers bridge disposal to a microtask and, when the effect re-runs synchronously with identical inputs, cancels the disposal and reuses the live bridge — so a single bridge survives the double-invoke. The pending input/result buffer is no longer cleared on a transient cleanup. 2. The SDK rejected the server's result. callTool() validates structuredContent against the tool's outputSchema and throws on a mismatch — but an MCP App is a passthrough: the host must forward whatever the server returned to the running view (which legacy hosts render fine). InspectorClient.callTool gains a `skipOutputValidation` option that routes through client.request() (no wrapper validation); onOpenApp uses it. Verified end-to-end against a real app via Playwright: the editor now renders its grid + pattern data identically to the v1 inspector. Tests: AppRenderer StrictMode single-bridge/no-dispose case; a get_temp_extra test-server fixture (returns an extra structuredContent property) with integration tests covering both the throwing default path and the skipOutputValidation passthrough. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(apps): warn (toast) when an app's result violates its outputSchema The skipOutputValidation passthrough keeps schema-mismatched apps rendering, but that silently hid a real spec problem from server developers. Now the host still renders, but surfaces the mismatch. - InspectorClient.callTool runs the SDK's own Ajv output validation on the skipOutputValidation path NON-fatally and returns the message as `outputValidationError` on the ToolCallInvocation (and the toolCallResultChange event); the result is still delivered. - App.tsx onOpenApp shows a yellow toast when outputValidationError is set, explaining strict MCP clients may not render the app. Tests: integration cases asserting outputValidationError is set for a schema-violating result (get_temp_extra) and unset for a valid one (get_temp). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(apps): summarize output-schema warning in toast + details modal The previous toast dumped the full (very long) validation error and overflowed. Now the toast shows a one-line summary with a "View validation details" link that opens a modal containing the full error in a read-only, scrollable textarea. - New OutputValidationModal group component (+ story + tests). - App.tsx: toast message is a summary + link; clicking it opens the modal populated with the tool name and the full outputValidationError. Verified via Playwright against the NightRider MIDI editor: toast is concise, link opens the modal with the scrollable details. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(apps): address PR review — theme stability, onError, validation polish Responds to the automated PR review on #1403. - Theme toggle no longer rebuilds a running app's bridge. createAppBridgeFactory reads the resolved theme from the DOM (Mantine's data-mantine-color-scheme) at bridge-build time instead of capturing isDark in the factory deps, so the factory identity is stable across theme flips. (#1) - Wire AppRenderer's onError through AppsScreen -> InspectorView -> App.tsx, surfacing bridge/runtime failures (no client, late rejection, failed tools/call) as a red toast instead of a silent blank iframe. (#2) - Extract validateToolOutput into a pure, unit-tested helper covering the matching / extra-property / missing-structuredContent / error / bad-schema cases (the no-structuredContent branch can't be produced via the SDK server, which rejects it before sending). (#4) - useSandboxUrl aborts its in-flight fetch on unmount (cancelled flag) to avoid setState after unmount. (#5) - Schema-mismatch toast: autoClose:false (details modal is one click away). (note) - AppRenderer: add a state-machine comment block documenting the bridge lifecycle refs. (#6) - inspectorClient: note why the `as CallToolResult` casts are safe. (note) Tests: AppsScreen onError surface; validateToolOutput unit suite; factory theme read from DOM. validate + test:storybook green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(apps): clarify theme attribute + useSandboxUrl fetch comments Re-review follow-ups (non-blocking): - createAppBridgeFactory: note that Mantine's data-mantine-color-scheme is only ever light/dark (auto is resolved before paint), so no `auto` branch is needed and matchMedia only covers an absent attribute. - useSandboxUrl: correct the doc to say it fetches on mount AND re-fetches on baseUrl/authToken change (idempotent), not "once on mount". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
d57801d7b3 | ||
|
|
63623b754b | ||
|
|
546dacffe2 | ||
|
|
7e6d6c0bc1 | ||
|
|
9ced0fd150 | ||
|
|
8f3619f618 | ||
|
|
f6105773f0 | ||
|
|
50502269f4 | ||
|
|
8b9839cc93 |
||
|
|
b33532c024 | ||
|
|
fd21d0d528 | ||
|
|
4379d17f76 | ||
|
|
de84b67046 | ||
|
|
52087f7739 | ||
|
|
0ab542cbee | ||
|
|
0c52172379 | ||
|
|
fdeffa1f00 | ||
|
|
decc673a5d | ||
|
|
b143dd6cbd | ||
|
|
3452a63101 | ||
|
|
bc408f5869 | ||
|
|
de60983ccc | ||
|
|
07a7b0beb5 | ||
|
|
fe5b622fd1 | ||
|
|
9e074aa6f2 |
||
|
|
a6f233eb6a | ||
|
|
f201032137 | ||
|
|
9644ac7fe8 | ||
|
|
8aba3122c5 |
||
|
|
373f346e5f |
||
|
|
e70a11360c |
||
|
|
320daf2f67 |
||
|
|
b3b04c6939 |
||
|
|
e026051b47 |
||
|
|
f9f79fff54 | ||
|
|
19a6df7ad3 | ||
|
|
650e5bd020 | ||
|
|
ba48e06ffd | ||
|
|
a24091484c | ||
|
|
13dafe457b |
||
|
|
5de4e911e3 |
||
|
|
97b1a7fbff |
||
|
|
91f02fd6db |
||
|
|
b4716a1405 |
||
|
|
d5f12b2a82 |
||
|
|
b0a024269e |
||
|
|
9b558afee1 |
||
|
|
ed6eda7492 |
||
|
|
8f6a2e0f78 |
||
|
|
5fb2b503f4 |