| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
cliffhall
|
3c6f12dc6c | Merge remote-tracking branch 'origin/v2/main' into v2/feat/1862-sandbox-public-url | ||
|
cliffhall
|
925461a043 |
fix(web): only use a public address on a fixed listener port (#1862)
Addresses Copilot review round 2. A reverse proxy routes a public address to one stable port, so neither MCP_SANDBOX_FULL_ADDRESS nor MCP_APP_ORIGIN_FULL_ADDRESS is used when the listener's port is OS-assigned: an explicit 0, a config-time collision resolved to 0, or the EADDRINUSE fallback. Validation warnings also stop echoing the URL path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzuzRPJv7M8wGe3wyw5sSg Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
4dc5eaa6bb |
fix(web): address Copilot review round 1 on public addresses (#1862)
- Never echo a refused value into a warning: credentials, query and fragment are dropped, and an unparseable value is not shown at all. - Do not adopt MCP_SANDBOX_FULL_ADDRESS / MCP_APP_ORIGIN_FULL_ADDRESS after the EADDRINUSE fallback: the reverse proxy still routes them to the pinned port, which another process holds. - Refuse a public app origin equal to any trusted ancestor in the controller, where the real (possibly bind-derived, possibly fallback-port) sandbox origin is known; config time could not see it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzuzRPJv7M8wGe3wyw5sSg Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
55db703196 |
feat: add --protocol-era launch flag for ad-hoc servers (#2208)
An ad-hoc --server-url / command launch always negotiated the legacy era, because protocolEra was only settable per server in mcp.json. A wrapper that mints a short-lived bearer token therefore had to write that token to disk to exercise a server under auto or modern. --protocol-era <legacy|auto|modern> now feeds settings.protocolEra in all three clients through a shared parseProtocolEra coercer: - CLI/TUI overlay it in loadServerEntries alongside --header, onto an ad-hoc target and onto a --catalog/--config entry for that run. - Web stamps it on the in-memory ad-hoc entry, which the backend lifts into settings like headers; as with --header it is rejected alongside --catalog/--config, but unlike --header it applies to stdio too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZ4rrvH6xaCfrst75gJh7x Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
12544d251c |
feat(web): configurable public addresses for the MCP Apps listeners (#1862)
Add MCP_SANDBOX_FULL_ADDRESS and MCP_APP_ORIGIN_FULL_ADDRESS so a reverse proxy or ingress deployment can advertise the sandbox proxy URL and the _meta.ui.domain app origin the browser actually reaches, instead of the bind-derived http://localhost:<port> ones. The process still listens on MCP_SANDBOX_PORT / MCP_APP_ORIGIN_PORT. Both values are validated in public-address.ts and refused (warned, bind- derived address kept) when malformed or when their origin collapses onto the Inspector UI's (or, for the app origin, the sandbox's) — the MCP Apps spec requires host != sandbox origin, and the single-hostname proxy layout is exactly the one that breaks it. Plain http under an https UI is warned. The sandbox proxy page's referrer check was a hardcoded http://localhost|127.0.0.1 pattern no https embedder could match. It now compares the referrer's parsed origin against an exact list injected by the controller, derived from the same filtered allow-list as its frame-ancestors header, with the old loopback check kept only as the no-list fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzuzRPJv7M8wGe3wyw5sSg Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
86e6774a9a |
fix: address Copilot review round 1 — move the Storybook optimizeDeps into a tested helper
The storybook project's optimizeDeps now comes from `getStorybookOptimizeDeps()` in `vite-base-config.ts`, beside the dev-server helper it mirrors, so the `force` flag and the base include/exclude it carries are asserted by `vite-base-config.test.ts` rather than living only in an inline object a config refactor could drop. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
5b9405fe8c |
fix: force the Storybook dep pre-bundle on every run instead of trusting the cache
Vite keys the Storybook project's dep cache on the lockfile and the config,
never on what the story graph imports. After a story or component gains an
import of an already-installed package, the cached bundle is still "valid",
Vite skips the scan, and the first request for the new package re-runs the
optimizer mid-run: the browser hash is bumped, a full-reload is sent that the
Vitest tester iframes ignore, and the `@storybook/react` renderer already
loaded in a live iframe keeps the old `?v=` for its lazy
`import("@storybook/react-dom-shim")`, so every story that iframe renders from
then on fails with "Failed to fetch dynamically imported module". The next run
is green because the cache has caught up.
`optimizeDeps.force: true` on the storybook project discards the cache up
front so the scanner crawls every story entry before the browser opens — the
cold path CI already takes on every PR. Measured: stale-cache 6/6 red on the
baseline, 6/6 green with the fix, 3/3 red reverted; ~0.8s per run.
Closes #2340
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
566990382a |
chore: address Copilot review round 3 on #2200
Two suppressed comments, both accurate. - Pin zod, zod/v4 and open to the root in vitest.shared.mts, and add matching paths to the three clients/web tsconfigs. The claim they replace — that these resolve unambiguously by walk-up — was wrong: both still sit at the top level of clients/web/node_modules as transitive copies (zod under eslint-plugin-react-hooks, open under Storybook), so an unpinned bare import from a web test resolved the client copy while core/ and the SDK packages resolved the root's. For open that is only wasteful; for zod it is a schema built by one copy and an instanceof check made by the other across the whole @modelcontextprotocol/* surface. zod is deduped as well, and zod/v4 gets its own entry because first-party code imports both specifiers and pinning one would split the package rather than collapse it. - Update the consumer comments that #2195 falsified. clients/web/vite.config.ts (three places) and server/start-vite-dev-server.ts all still said the shared aliases resolve pino/hono and friends from clients/web/node_modules; they now state the split — react and react-dom from the client install, everything root-declared from the repo root. - local-dev skill: replace the wrong 'resolves unambiguously' list with the distinction that actually matters. Deleting a declaration is not the same as the package resolving from the root, because chokidar, zod, open and react all sit in a client install transitively; check the install, not the manifest. ajv, commander and undici carry no pin because no client install has a copy. npm run local:gate passes from a cold tsc build. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
6011d4de03 |
fix(web): bound retained app documents by size; correct the cookie claim
Copilot review round 5 on #2100. - The store bounded entry COUNT only, and the route admits 8Mi code units per document with a one-hour TTL — ~256Mi retainable on documents a server under test chooses the size of. Added a 64Mi retained-size budget evicted oldest-first alongside the count bound, with a running total kept in step with the map through a single `drop` seam so it cannot drift. - The isolation claim was wrong about cookies. A separate port makes a separate ORIGIN, so DOM access, localStorage, sessionStorage and IndexedDB are isolated — but cookies are keyed by host and path, ignoring port, so an app served here shares the 127.0.0.1 jar with the Inspector and every other loopback service. Not a hole in the Inspector's own auth (the API token is a header plus a window global and sessionStorage, none of them cookies) and not closable without a distinct host this listener cannot mint, so it is stated plainly in both the module header and clients/web/README.md rather than left as a claim that is false for one of the three surfaces it names. - Removed a shell-escaping artifact I left in the migration guide. 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
|
bb3ba91b8a |
fix(web): guard the app-origin port and the trusted-origin grant
Copilot review round 3 on #2100. - MCP_APP_ORIGIN_PORT could equal CLIENT_PORT or MCP_SANDBOX_PORT. The app-origin listener starts BEFORE the web server binds, so that is not a race it loses — it wins the port and the Inspector dies with EADDRINUSE. `CLIENT_PORT=6278` was a valid config before this feature existed. It now resolves to a dynamic port with a warning: of the three listeners this is the only one that can move, and refusing to boot would cost the user the whole Inspector over a feature they may not be using. - The proxy's same-origin refusal covered only its own origin. An app served from the INSPECTOR's origin is no safer: same-origin with the top document means direct access to window.top, its DOM and its storage, and the cross-origin frame in between buys nothing. Both trusted origins are now refused before allow-same-origin is granted. - Replaced the `as unknown as number` on `server.address()` with a real narrowing of the string/null members of its return type. - MCP_APP_ORIGIN_PORT added to the v1→v2 migration guide's env table, its Docker recipe, and a troubleshooting entry for the symptom this actually produces (the app renders, but its own backend rejects `Origin: null`). - clients/web/README.md now states which failure is outside the fallback guarantee — a published document the browser cannot reach — and why there is no honest signal to fall back on. Docs and code no longer disagree. 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
|
fafc8367b8 |
fix(web): move the app origin off 6276, bound the publish route
Copilot review round 2 on #2100, plus a collision the review did not find. The default app-origin port was 6276 — which is already RUNNER_OAUTH_CALLBACK_DEFAULT_PORT, the CLI/TUI loopback OAuth callback. That port is fixed precisely so an OAuth app can pre-register http://127.0.0.1:6276/oauth/callback, which makes it the one listener here that cannot move when something takes it first. A running --web would hold it and break a later --cli/--tui OAuth flow at its registered redirect URI, and docs/mcp-app-review.md has those two running side by side. This listener has a documented dynamic fallback, so this listener yields: the default is now 6278 (6277 skipped — v1's retired proxy port, which the migration guide tells people to stop forwarding). A test asserts the two defaults stay distinct. Review comments: - POST /api/app-document now applies hono's bodyLimit BEFORE parsing. The 8MiB check ran after c.req.json() had buffered and parsed the whole request, and bounded only `html` — so bulk in `csp`, or in keys the route never reads, was fully materialized before the 413. - `csp` is validated as an HTTP header value and length-bounded. The controller hands it to res.writeHead() verbatim, where Node throws ERR_INVALID_CHAR synchronously — inside the handler for a later, unrelated request, outside this route's error handling. - The app-origin port now propagates everywhere the other two do: the CLI --print-handoff port-forward command (and its test), the Dockerfile's ENV and EXPOSE, and the remote-review SSH recipe in docs/mcp-app-review.md. Without it an app declaring _meta.ui.domain is unreachable through every documented remote workflow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
fa441da85f |
fix(web): harden the app-origin paths and correct three docs
Copilot review round 1 on #2100. - POST /api/app-document: `null` and `[1,2]` are valid JSON, so the destructure that followed threw OUTSIDE the parse try/catch and turned a malformed body into a 500 instead of the documented 400. - app-origin controller: the error handler cleared `server` but not `origin`, and `publish` gates on `origin` — so an error after a successful listen kept minting URLs on a dead port and the browser never took the srcdoc fallback. Clear both, and drop the now-unfetchable documents. - sandbox proxy: drop a stale `srcdoc` before navigating to a dedicated origin (srcdoc wins over src), and restore the opaque-origin posture — strip `allow-same-origin`, reset `innerOrigin` — before assigning `srcdoc` on the fallback path. Not reachable today (each render remounts the iframe), but the cost of being wrong about that later is an isolation boundary. - publishAppDocument tests: replace two `as unknown as Response` casts with real `Response`s. - docs: `frame-ancestors` admits the proxy AND the Inspector page (every ancestor is checked); the shared helper falls back to loopback, not `'none'`; the showcase origin is `127.0.0.1`, the default bind address. - tests: null/array body, post-listen error, and both factories receiving a working `publishAppDocument`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
8e8c1bbad9 |
test(web): prove the dedicated app origin end to end
`frame-ancestors` is checked against every ancestor, not just the parent. A published app document is framed by the sandbox proxy, which is framed by the Inspector page — so admitting only the proxy's origin blocked the frame outright (it rendered as a chrome-error frame that never reached the bridge). `appDocumentEmbedders` now admits both. That failure was invisible to every unit test, so `smoke:web:app` grows a second phase: the same demo app declaring `_meta.ui.domain`, driven through the same connect → open → ready chain, then asserting the two facts that only hold on the dedicated path — the inner frame was navigated to a published document, and its own `location.origin` is a real http origin rather than "null". Both phases end at `data-app-status="ready"`, so nothing short of reading the frame's origin can tell them apart. Adds `test-servers/configs/mcp-app-domain-http.json` (and a `domain` param on the `mcp_app_demo_widget` preset) as the showcase server for both the smoke and hand testing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
ec40cf5531 |
feat(web): honor _meta.ui.domain with a dedicated app origin
An MCP App is rendered by handing its HTML to the sandbox proxy as `srcdoc`, in a frame sandboxed without `allow-same-origin` — the #1565 isolation model. That gives the app document an opaque origin, so every request it makes carries `Origin: null` and no CORS / OAuth-callback / API-key allowlist can admit it. `_meta.ui.domain` is the spec field a server uses to ask its host for a stable, dedicated origin, and it was being dropped on the floor. The field is explicitly host-dependent and the Inspector owns no domain infrastructure, so it treats any non-empty value as a request rather than an address, and answers with a real loopback origin on its own listener (MCP_APP_ORIGIN_PORT, default 6276). One shared origin, path-keyed per document: that delivers what the field is for without minting a port or DNS name per app, and is therefore not a per-app isolation boundary. The inner frame is granted `allow-same-origin` on this path only, which is what makes the origin real. It is not a #1565 regression: the listener is on its own port, so the app stays cross-origin to both the proxy and the Inspector. The proxy refuses the grant if the URL's origin equals its own, and the grant is never reachable from the server-supplied `sandbox` string, which is still stripped. The per-app CSP is delivered as a real response header there rather than only as a <meta>. Every failure path falls back to the default srcdoc render with a console warning naming the field, rather than blanking the app. Also fixes a pre-existing `tsc -b` break on v2/main (useServers.test.tsx passed `metadata: []` where `RequestMetadata` is expected), which has had CI red since #2087 and blocks the pre-push gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
09e1564672 |
fix(auth): refuse on a stuck lock, capture identity at the mkdir seam (#2082)
Copilot review round 6 — five findings (two inline, three suppressed), all valid. **A stale lock that cannot be cleared caused unlocked writes.** `acquireLock` does not only *create* directories: on finding a stale one it removes it and retries, and that removal can fail — `ENOTEMPTY` for a lock with anything inside it, `EACCES`/`EROFS` for one we may not touch. Those surface as ordinary non-`ELOCKED` errors, which the probe read as "locks do not work here" and degraded on. So every Inspector on the box quietly bypassed the *same* stuck lock and raced its writes — while the release-failure message was telling the operator saves would keep failing until they cleared it. `isStuckOrHeld` now discriminates on **whether the lock directory exists** rather than on an errno taxonomy: if it is there, something holds it and we refuse; if it is not, we genuinely could not create one and degrading is the documented trade (#1848, #1905). Reading the state the decision is about avoids enumerating error codes per platform — and an enumeration is exactly what let `ENOTEMPTY` through. **Identity was captured after the acquire promise settled.** That span includes the library's own `utimes`/`stat` probe, so a waiter replacing our directory inside it would have us record *the winner's* identity as our own, after which the removal guard would accept and delete their lock. It is now recorded in the `mkdir` callback — the moment the directory becomes ours, and synchronous with respect to this process, so there is no window. **The expiry advice was still wrong for `EACCES`/`EPERM`/`EROFS`.** Since the guard `stat`s before removing, everything reaching the release catch is an `rmdir` that was refused — and stale takeover reclaims through that same `rmdir`. The `ENOENT` arm was unreachable for the same reason, so the branch was dead code: removed, and the advice is now unconditional. **`proper-lockfile` was missing from `NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE`.** A fourth externalization surface — the tsup lists configure the production bundles, not `vite dev`, whose dep scanner would otherwise walk its CJS/`graceful-fs`/`signal-exit` graph. Added, and `vite-base-config.test.ts` now asserts it along with `chokidar` and `@napi-rs/keyring`, which were also absent from that assertion. **A test was named for something it did not exercise.** "does not treat an unlistable directory as an empty one" was driving `ENOTDIR`, which is deliberately treated *as* the fresh-install case, so the actual new behaviour was untested. Split in two: one for `ENOTDIR`, and a real `EACCES` one using a `--x` directory — `readdir` denied, a known name still reachable, which is the asymmetry that makes "unlistable" different from "empty". Driven with a real mode rather than a stub, since `vi.spyOn` cannot redefine an ESM namespace export; skipped as root and on Windows, where the mode would not bite and the test would assert nothing. Both new tests verified by mutation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
b143dd6cbd |
fix(auth): address Copilot review round 3 on the secret-store PR
Nine findings (six inline, three suppressed), all taken. Two were serious. **The memory fallback made the migrations destructive.** Both migrations — mcp.json in the remote server, client.json in `node-persistence` — lift a plaintext secret off disk, write it to the secret store, and then delete it from the file. That trade is safe only while the store is at least as durable as the disk, which held by construction while `InMemorySecretStore` was a test double. Making it a production fallback broke it: on an unmounted container a plain `GET /api/servers` would have moved a user's existing secrets into RAM and lost them at exit — destroying data on a read. `SecretStore` gains an optional `isDurable()`, absent meaning durable, and both migrations withhold the disk delete when it answers false (the value is still loaded, so the session behaves normally). The production fallback is now a named `SessionSecretStore` rather than the bare `InMemorySecretStore`, so the suite's many doubles keep standing in for a *working keychain* — which is what they are there to be — and only code that deliberately chooses RAM in production says so. **Round 2's per-call descriptor never reached anyone.** Both web servers resolved it once at startup and baked it into `initialConfig`, which `/api/config` serves unchanged — so the freshness fix was inert for its only consumer. The route now takes a `secretStorageResolver` and calls it per request. The lock needed three more fixes, and they compound: - **Release removed whatever directory was at the path.** A holder whose work outran the stale threshold would delete the *next* owner's lock, letting a third writer into a section two processes believed they held. Ownership is now stamped with a random token and release only removes a lock still carrying it. - **The timeout fell through to an unlocked write** — under contention, the one moment another writer is provably active. It now throws `SecretFileLockTimeoutError`, which `set` is allowed to do. - **The first write of every process bypassed the lock**, because the secrets directory does not exist yet and `mkdir` failed ENOENT. The parent is created first. First writes are exactly when two processes are most likely to start together. Fixing the timeout exposed a fourth: with stale (10s) below timeout, a waiter stole from a holder that was merely *slow* rather than dead — reinstating the lost update. A holder now heartbeats its lock, so "stale" means the holder is gone, and the timeout is what a live-but-slow holder produces. Lock timings are injectable so the tests can exercise contention, staleness, and timeout without spending the real intervals. Also: the loose-permission state is carried into `SecretStorageInfo` as `looseMode` and rendered as the caveat (it outranks the encryption caveat — a file others can read is a live exposure, while "unencrypted" is a property of a file only its owner can open), rather than living in a startup log the browser cannot see; and `usableSecretStorage` validates `pendingEncryption` and `looseMode` as the optional booleans/numbers they are (`pendingEncryption: "false"` is truthy and would have printed advice that is the opposite of the truth) and rejects file-only fields on a keychain or memory descriptor. Tests: both migrations against a session store with a durable control alongside, so the guard is visibly the difference; `/api/config` serving a changing resolver and falling back without one; lock ownership after a steal, the timeout under a live holder, no-steal from a slow holder, and a locked first write; `looseMode` through the descriptor, the caveat ordering, and the tone; and five more rejected payload shapes. Full `npm run ci` green — 5,704 tests under the coverage gate. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
bc408f5869 |
feat(auth): file-backed and in-memory SecretStore for hosts with no keychain
Closes #1950 Before this, a box with no reachable OS keychain — the published container (no D-Bus session, #1848), Android/Termux (no platform binary, #1905), a minimal Linux install without libsecret — could not persist an OAuth client secret or a stdio `env:` value at all. `KeyringSecretStore` degraded into a store that silently holds nothing: `get` returned null, `delete` no-opped, and `set` hard-failed with a 503. That is the correct failure behavior, but it left the documented, supported way to run the Inspector with no secret persistence whatsoever. Adds the two missing implementations and, more importantly, the policy that picks one and the surfaces that say which was picked. - `FileSecretStore` — one `0600` JSON document at `~/.mcp-inspector/secrets.json`, encrypted with AES-256-GCM whenever `MCP_INSPECTOR_SECRET_KEY` is set (scrypt, per-file random salt). The whole map is encrypted as a unit rather than value-by-value so the account names — `${serverId}:${field}` — are hidden too; a per-value scheme would leave a readable index of which servers you hold a client secret for. A file that can no longer be decrypted reads as empty (matching the interface's read tolerance) but *refuses to be written*, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - `secret-store-selection.ts` — the policy. `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright; otherwise probe the keychain and, if unreachable, fall back loudly. The fallback is `memory` in a container whose secrets directory is not on a mount, and `file` otherwise — so mounting the volume the README already recommends for the catalog flips the same run to durable storage with no configuration, and an unmounted container gets the honest answer rather than a file `--rm` will discard. Cached per process so the banner, `/api/config`, and the store doing the writing cannot disagree. - Automatic, but never silent. The downgrade prints at startup, rides `GET /api/config`, and is named in a permanent 32px footer at the bottom of both dialogs that accept a secret. A startup banner is seen once by whoever started the process; a toast is seen once; a dismissible banner is the thing a user dismisses before doing the work it describes. The plaintext and memory cases render their caveat inline in a warning tone — a warning you must hover to discover is not one. - `SecretStoreUnavailableError` is now the base every store's `set` throws, so the routes translate one condition into a 503 instead of matching `KeychainUnavailableError` by name. Docs: the container section of the root README gains a table of which store you get and how to make secrets survive, plus the `MCP_INSPECTOR_SECRET_KEY` upgrade/mismatch rules. Tests: the existing interface contract block is extracted to `secretStoreContract.ts` and run against all three implementations, so none can drift on the availability contract. `FileSecretStore` is tested against a real temp filesystem — mode, cross-instance round-trip, that the ciphertext does not contain the plaintext, the read-modify-write race, and every refusal path. Full `npm run ci` passes, per-file gate included. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
d830213796 |
fix: address round-2 review — stale disconnect, lint-project gap, coverage
Copilot round 2 (PR #2039) surfaced no new visible comments but six suppressed ones. Four were actionable. `clients/tui/src/App.tsx` — a stale-completion race in the new banner: start disconnecting server A, switch to B, then let A's disconnect reject, and A's failure appears under B. Rapid same-server retries had the same shape, and the stale attempt is the one most likely to reject. Claim a monotonic attempt token before awaiting and publish only if it is still current and the server has not changed; `handleConnect` bumps the same counter so a connect retires an in-flight disconnect. The rejection is still owned when superseded — just not published. The regression test drives a deferred rejection across a server switch, and fails without the guard. `tsconfig.lint.json` — the root type-aware block matches `test-servers/src/**/*.tsx` but the parser project omitted that extension, so such a file would have failed with "not found in any of the provided project(s)" instead of being linted. Added, and AGENTS.md now states the invariant: the include must stay a superset of the lint globs. `clients/web/server/{server,vite-hono-plugin}.ts` — the `open()` rejection branch was untested, and duplicated. Both were fixed by removing the duplication: `openBrowser` now lives beside `printServerBanner` in `web-server-config.ts`, with one implementation to test. Two copies of a one-line `.catch` is two chances for one to drift back to a bare `open(url)`. The dev plugin is not booted in tests — it returns early under VITEST by design — but it calls the same tested helper. `core/mcp/remote/node/server.ts` — `closeAbortedStream`'s rejecting-close path, the entire reason the helper exists, had no test. Exported it (as this file already does for `requestIdForSendWait` / `mcpParamHeadersOnly`) and covered both legs, asserting no unhandledRejection fires. Also corrects AGENTS.md, which said each client lists "both" tsconfig projects — true for cli/tui/launcher, but web lists four leaf projects. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
519ecf49ee |
chore(lint): enable no-floating-promises in all five ESLint scopes
A floated promise is invisible at review time — the call reads like an awaited one minus four characters — and the unhandled rejection it produces surfaces in a different test, in a different file, as a stack pointing at SDK internals. In #1947 two un-held `callTool` calls made `npm run ci` unpassable and took a full investigation to attribute; the fix was two lines. `@typescript-eslint/no-floating-promises` catches the class outright, so enable it at `error` everywhere. The rule is type-aware and `tseslint.configs.recommended` is not, so each scope's config gains a parser project. The four clients name their own tsconfig projects (both of them — `src` and `__tests__`/`server` live in different ones). The root scope has no tsconfig at all, so add `tsconfig.lint.json`: a lint-only project covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts`, emitting nothing and gating nothing. It sets `moduleResolution: bundler` deliberately — under NodeNext `core/`'s extensionless imports fail to resolve, every import degrades to `any`, and the rule silently stops seeing promises at all. Measured surface, and the fixes: - core (2) — `stream.close()` in two Hono `onAbort` listeners. Hono invokes subscribers with a bare `subscriber()`, so a promise returned from the listener is floated one layer up; `closeAbortedStream` owns the rejection where it can actually be owned. - tui (5) — `handleDisconnect` had no catch of its own and its only caller is a key handler that cannot await, so a failing disconnect was a live unhandled rejection; it now surfaces the message the way `handleConnect` does. The rest are `void` at callers that genuinely cannot await, over callees that already own their failures. - web (92) — 83 are Storybook `expect(...)`, which is instrumented and returns a promise, now awaited (`scrollAreaStoryAssertions` is async for the same reason). Two tests were asserting against a provider call they never awaited. `open(url)` in both web servers now warns instead of escaping a synchronous callback. - cli, launcher — already clean. Type-aware linting costs time: `clients/web`'s `eslint .` goes from ~8s to ~19s, inside `validate`. Narrowing the loaded projects is the lever if that needs reducing later; dropping the rule is not. Closes #1959 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
adde9ccf94 |
fix: bind loopback by address and pin the sandbox port so containers work
Two defaults that each break the Inspector inside a container. #1951 — the web UI never loads in a VS Code dev container. `HOST` defaulted to the NAME `localhost`, and `server.listen(port, host)` resolves a name through `dns.lookup` and binds the SINGLE address it gets back. Since Node 17 the default result order is `verbatim`, so on a glibc Linux host — whose /etc/hosts maps localhost to both 127.0.0.1 and ::1 — that is `::1`. The server bound IPv6 loopback only and refused every IPv4 client, including the dev container port forwarder. It presents as an endless spinner rather than a connection error because the forwarder accepts on the host side before failing to connect inward. Default to the ADDRESS 127.0.0.1 instead; an explicit HOST=localhost is still honored as typed. #2008 — the Apps tab is unreachable in any container. The MCP Apps sandbox listened on an OS-assigned port, so it changed every run and could never be named in a devcontainer `forwardPorts`, a `docker run -p`, or a tunnel. Default it to a fixed 6275, mirroring CLIENT_PORT's fixed 6274, and retry once on an OS-assigned port (loudly) if it is taken — the collision a dynamic default never had. Verified in the reporter's exact image (mcr.microsoft.com/devcontainers/typescript-node:4-24-trixie): both listeners come up on 127.0.0.1 and answer 200 on 6274 and 6275/sandbox. Also EXPOSE 6275 and set MCP_SANDBOX_PORT in the image, and document both defaults in the web README, the migration guide, and the root README's Docker section. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
Cliff Hall
|
a09f2dcf1d |
fix: load @napi-rs/keyring lazily so an unsupported platform degrades instead of crashing (#1905) (#1948)
* fix: load @napi-rs/keyring lazily so an unsupported platform degrades `@napi-rs/keyring` ships one prebuilt binary per platform triple and throws on import where it has none. On Android / Termux there is no `@napi-rs/keyring-android-arm64`, so the static top-level import in `core/auth/node/secret-store.ts` threw during module evaluation and the Inspector exited at startup with "Cannot find native binding" — before any of the keychain-unavailable handling in that same file could run. Replaces the static import with a cached dynamic one. The outcome is cached, not just the module, so a box without a binary doesn't re-attempt (and re-throw) resolution on every secret operation — `expectedSecretFields` means that would otherwise be once per server per `GET /api/servers` — and so `set` can name the underlying cause in its `KeychainUnavailableError`. An unloadable package now folds into the same degradation contract as an unreachable keychain: `get` returns null, `delete` and `deleteAllForServer` no-op, and `set` throws `KeychainUnavailableError` (not double-wrapped — the load failure is already typed by the time the catch sees it). The class doc comment now enumerates all three ways "unavailable" can arrive, since each escaped the contract at some point. The shared `vi.mock` stub can't express this — it models a keyring that loads — so the new tests build on `vi.resetModules()` + `vi.doMock` with a throwing factory and re-import the module, covering the load failure, each method under it, and the load-once caching. Verified the dynamic import survives bundling in all three clients (cli, tui, and the web runner) rather than being inlined back to a static one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F * fix: steer a missing native binding to a reinstall hint `KeychainUnavailableError` gave the Linux libsecret / gnome-keyring advice for every cause, including the one where the keychain is fine and the @napi-rs/keyring platform package is what's missing — an unsupported triple (#1905) or npm's optional-deps bug dropping it on a supported one (npm/cli#4828, the Windows report on #1852). Detect the loader's "Cannot find native binding" phrasing and point those users at a reinstall / npx-cache clear instead. Ported from the superseded #1943, which fixed this alongside the same lazy-load change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 * fix: name a bad keyring namespace instead of "is not a constructor" The lazy import accepted whatever the resolution produced. The named exports arrive through CJS interop, so a resolution that stopped yielding them — a default-only export upstream, a bundler changing interop, a platform where named-export detection fails — would hand us `undefined`, and `new mod.AsyncEntry(...)` would throw a TypeError inside the very try that implements graceful degradation. Shape-check the resolved module and treat a bad shape as unavailable. Scope, because it is narrower than it first appears: this does NOT prevent a bad shape from emptying the secret list. `get` returns null either way — that is its read-tolerance contract, and the TypeError lands in the same catch a dead keychain does. What changes is the diagnosis. Without the check the only signal is `set` reporting "keyring.mod.AsyncEntry is not a constructor", which reads like an Inspector bug; with it, `set` names the real problem once, at the load boundary, in the same actionable 503 as every other unavailability. Catching the silent-empty-list case itself would need a round-trip against the unmocked package, which nothing in the suite does today. The member access sits inside the try because reading a missing export is not always a harmless undefined — a Proxy-backed namespace can throw, as vitest's module mocks do — and letting that escape would reject the cached promise, which is otherwise guaranteed to always resolve. Of the four new tests only the cause-message one fails without the guard; the rest pin the surrounding contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 * docs: correct the test header's count of unavailability entry points The shape check added a fourth way "unavailable" can arrive, and the enumeration in secret-store.ts was updated to match, but the test file's header still said three — so it read as an exhaustive list that silently omitted the case a whole describe below it covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 * fix: give the wrong-shape cause its own remediation hint The shape check added a cause that fell through to the libsecret advice — so a packaging mismatch told the user to install a keyring daemon, which cannot help, and on Windows or macOS points somewhere that does not exist. That is the same wrong-advice bug the native-binding branch was added to fix, reintroduced by the guard itself. Hint selection moves into `hintFor`, where libsecret is the explicit fallback rather than the default, and the shape failure becomes a distinct `KeyringModuleShapeError` matched by type — it is our own error, so there is no reason to re-parse text we wrote (the native-binding branch matches a string only because that message comes from napi-rs). Also retitles the wrong-shape test: it claimed the guard avoids "returning null secrets", but `get` returns null either way by the read-tolerance contract. The title now says what actually differs — `set` hard-fails — matching the correction already made in the comments. Both raised by Copilot review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 * fix: give both shape-check failure paths the packaging hint The shape check can fail two ways — the members are absent, or reading them throws — and only the first carried KeyringModuleShapeError. The catch path returned the raw error, so a Proxy-backed namespace throwing on access fell through to the libsecret advice: the same wrong-remedy bug as the previous two commits, one branch further along. Both paths now carry the type. KeyringModuleShapeError takes a detail string plus a standard `cause`, so the throwing path reads "its exports could not be read: <original>" and keeps the underlying failure legible while still earning the packaging hint. Three instances of one mistake in a row (a new cause added without deciding which remediation it inherits) says the shape was wrong, not just the branch: making libsecret an explicit fallback in `hintFor` did not stop me adding a second exit that skipped it. The tests now pin the invariant directly — one asserts both construction paths produce the same hint — so the next cause that forgets is a failing test rather than a reviewer's catch. Raised by Copilot review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
cea22d86b8 |
feat(web): enforce localhost-only binding; expand loopback origin allow-list (#1796)
Closes #1795. Enforce localhost-only binding in code (the "never 0.0.0.0" rule was only a spec note): refuse an all-interfaces HOST — across every spelling the OS binds as the wildcard (inet_aton decimal/octal/hex, IPv6, IPv4-mapped, zone-scoped, IDNA/Unicode) — unless DANGEROUSLY_BIND_ALL_INTERFACES=true (Docker's sanctioned opt-in). Fixes the reported stdio-connect 403 by expanding the default loopback origin allow-list to all three interchangeable forms. Hardening surfaced across an extended review: closes an IDNA/fullwidth wildcard bypass, a blank-ALLOWED_ORIGINS fail-open, and the CLI/TUI OAuth-callback wildcard bind (loopback-only now, RFC 8252); canonicalizes the origin allow-list the way a browser does; derives frame-ancestors from the real embedder list (IPv6-literal exclusion verified in headless Chromium); strictPort on all three bind points; and preserves the CLI exit-code map + JSON envelope through the mcp-inspector --cli launcher path. Shared host normalization lives in core/node/hostUrl.ts. Docs + tests + smokes throughout. 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
Cliff Hall
|
a82a70f4f5 |
refactor(web): sharpen the src/lib vs src/utils rule and move misfiled modules (#1784)
* refactor(web): sharpen src/lib vs src/utils rule and move misfiled modules (#1776) Codify the empirical rule the web client already followed loosely — utils = pure functions that compute; lib = things that instantiate, adapt, or touch the environment — in AGENTS.md and clients/web/README.md, and move the modules that sat on the wrong side of it. Moves (source + co-located test): - lib/sandbox-csp -> utils (pure CSP-string construction; type-only import) - utils/browserTabVisibility -> lib (registers visibilitychange listeners) - utils/oauthResume -> lib (reads/writes sessionStorage) - utils/clearServerOAuthState -> lib (drives live InspectorClient / OAuthStorage) Borderline audits (kept in place): pendingReauth (pure type declarations), deepLink / correlateTransportErrors / mcpNetworkHeaders / oauthUx / oauthFlow / oauthNetworkPhase (pure transforms / type-only core imports) stay in utils; downloadFile stays whole in lib (dominant side is DOM side effects). Also relocate the lone src/test/lib/downloadFile.test.ts straggler beside its source at src/lib/downloadFile.test.ts, and document the test-placement rule (side-by-side by default; src/test/ only for core/ tests, the integration project, and shared scaffolding). Pure move — no behavior change; no build/coverage config keys off the boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * refactor(web): address review — fix stale paths, move customHeaders, one-way arrows (#1776) Responds to the @claude review on PR #1784: 1. Stale path references (fixed): update the four in-repo pointers left at pre-move paths — specification/v2_auth_mid_session.md (oauthResume, browserTabVisibility → lib/), and the two load-bearing sandbox security comments (sandbox_proxy.html, sandbox-controller.ts → src/utils/sandbox-csp). 2. lib/types/customHeaders (moved): it's pure (types + pure transforms), the rule's clearest counterexample. Moved lib/types/customHeaders.{ts,test.ts} → utils/ (no external importers; only its own test) and removed the undocumented lib/types/ subdir. Documented that utils/ is the home for pure shared domain types + their constructors — no separate types/ bucket. 3. One-way arrows (item 3): declare OAuthResumeAuthKind in utils/pendingReauth and re-export it from lib/oauthResume, so the only cross-directory arrow now points lib → utils. Codified that invariant in AGENTS.md. Wording nit: AGENTS.md + README now state diagnostic logging (console.warn/error) doesn't count as a side effect for the utils rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 2 — stale customHeaders refs, rule wording (#1776) Doc-only follow-up to the round-2 @claude review on PR #1784 (no code changed): 1. Stale paths (same class as round-1 item 1): the customHeaders move left three references to the deleted lib/types/customHeaders.ts path in specification/ (v2_ux_interfaces_phase_0_4_inventory.md, v2_ux_interfaces_plan.md ×2) — updated to utils/customHeaders.ts. 2. customHeaders anchor: note in AGENTS.md that its shape is staged for ServerSettingsForm (per v2_ux_interfaces_plan.md), so its zero-importer state reads as intentional rather than rot; kept as the no-types/-bucket illustration and led the anchor list with load-bearing modules. 3. Type-only-import clause: split the conflated clause — a type-only import (pendingReauth) and re-exporting core's pure functions/constants (oauthUx/oauthFlow) are distinct reasons to stay in utils; what makes a module lib is wrapping core's stateful runtime. Fixed in both AGENTS.md and the web README. Cosmetic: re-padded the v2_auth_mid_session.md table row to column width. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 3 — scope the "no types/ bucket" claim (#1776) Doc-only follow-up to the round-3 @claude review on PR #1784 (no code changed): 1. "no separate types/ bucket" overreached: clients/web/src/types/ does exist (navigation.ts + react-syntax-highlighter .d.ts ambient stubs wired via tsconfig.app.json paths). Scoped the claim in both AGENTS.md and the web README to "no types/ sub-bucket INSIDE lib/utils", and noted src/types/ is for ambient .d.ts stubs, not domain types. 2. README example list drifted from AGENTS.md's round-2 load-bearing anchors — realigned it (inspectorTabs/mcpNetworkHeaders, dropped the zero-importer customHeaders from the headline list). 3. Clarified v2_ux_interfaces_plan.md:185 so the v2 path reads as the copy's destination, not as the v1.5 source location. Follow-up filed as #1785 (board Todo): src/types/navigation.ts InspectorTab is dead and duplicates utils/inspectorTabs.ts — kept out of this pure-move PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 4 — reconcile src/types/ claim with #1785 (#1776) Doc-only follow-up to the round-4 @claude review on PR #1784 (no code changed): The round-3 scope fix overshot: it asserted src/types/ holds "not domain types at all", but the dead navigation.ts InspectorTab is a plain-.ts domain type still living there, and specification/v2_ux_interfaces_plan.md:181-182 actively prescribes placing InspectorTab there — so AGENTS.md and that spec doc gave opposite guidance about the same directory. - AGENTS.md + web README: softened to "not the place for new domain types" and added a parenthetical that the one lingering domain type (dead navigation.ts) is tracked for removal in #1785; until then a pure domain type belongs in utils/, not src/types/. - v2_ux_interfaces_plan.md:181: marked the tab-enum placement guidance as superseded by #1785 (live type is InspectorTabId in utils/inspectorTabs.ts). Self-heals on merge of #1785. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 5 — fix garbled #1785 clause, restructure utils rule (#1776) Doc-only follow-up to the round-5 @claude review on PR #1784 (no code changed): 1. Grammar (item 1): the round-4 #1785 clause read "superseding [#1785] will remove it" — a participle with no subject. Reworded to "the dead navigation.ts InspectorTab, which #1785 will remove". 2. Readability (item 2): the src/utils/ bullet had accreted into a single ~1,400-char blob across five review rounds. Promoted the carve-outs to scannable sub-bullets (Domain types / Diagnostic logging / Importing from core) and lifted the src/types/ note out to its own line, since it's a statement about a sibling directory, not about utils. AGENTS.md only; the README's parallel sentence was already clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs+config: address review round 6 — correct the coverage-glob claim (#1776) Responds to the round-6 @claude review on PR #1784: 1. "no build/coverage glob" was factually wrong (should-fix): the web coverage include in clients/web/vite.config.ts DOES enumerate src/lib/** and src/utils/**. The conclusion (a move between them is coverage-neutral) survives, but the reason was wrong. Reworded both AGENTS.md and the web README to say nothing *enforces* the boundary — no path alias, and the include lists both dirs so a move is coverage-neutral — plus the important whitelist caveat: a module outside components/lib/utils/server falls out of the ≥90 gate entirely. 2. Removed a pre-existing duplicate glob in vite.config.ts (src/lib/**/*.{ts,tsx} appeared twice, lines 161 and 163). Inert (vitest de-dupes) but it sat three lines from the config the doc now describes. Nits: gave the README bullet the same round-5 sub-bullet restructure as AGENTS.md, and made its #1785 reference a real markdown link (bare #N doesn't autolink in rendered repo markdown). npm run ci green (config change re-verified through coverage + smokes + Storybook). 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
|
07d6316fba |
build(web): fail vite build on a browser-externalized Node built-in (#1769) (#1777)
* build(web): fail vite build on a browser-externalized Node built-in (#1769) Vite 8 (rolldown) externalizes a node:* / Node built-in that reaches the browser graph with a warning and ships a `module.exports = {}` stub — the build still succeeds, shipping a broken bundle. The runtime browser smoke (smoke:web:browser, #1615) only catches the subset where that stub is *called* at module init; a stub imported but never called ships silently and nothing gates it. Add a Vite build plugin that turns that specific browser-externalization warning into a hard `vite build` error, moving detection upstream into `npm run build` / `validate` and additionally catching the never-called case the runtime smoke can't see. - clients/web/server/browser-externalized-builtin-gate.ts: Vite-agnostic detection + error + per-build state machine (unit-tested to the ≥90 gate). - vite.config.ts: thin Vite wiring. A throw inside rolldown's onLog is swallowed (the one hook where it doesn't abort — verified against vite@8.0.0), so the gate records the warning in onLog and re-throws in buildEnd. Scoped to `vite build` (apply: 'build'); the Node runner build (tsup) is unaffected. - scripts/verify-build-gate.mjs + verify:build-gate (in npm run ci and the GitHub workflow): runs a real build with a node:fs probe and asserts it fails via the gate — the only check that catches the warning phrasing drifting in a future Vite bump and silently disabling the message-keyed gate. There is no stable log `code` on the rolldown warning (only message + plugin), so the gate keys off the documented message phrasing; the drift guard above is what keeps that safe across Vite versions. smoke:web:browser stays as the runtime backstop. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * build(web): harden the browser-externalized-builtin gate per review (#1769) Review follow-ups on PR #1777: - Per-build state: add gate.reset() called from the plugin's buildStart, so a `vite build --watch` rebuild doesn't inherit a prior build's recorded warning (the plugin instance is reused across rebuilds). Makes the docblock true. - buildEnd(error) now only asserts on the success path — rolldown also calls buildEnd(error) when the build already failed for another reason, and throwing then would mask that real error with the #1769 message. - verify-build-gate.mjs: drop the shell `rm -rf` cache-clear entirely (verified the gate fires reliably without it — the earlier "caching" symptom was the onLog-swallow behavior, not a stale cache); add a result.error spawn-failure check so a missing `npx` isn't misdiagnosed as "not via the gate"; add SIGINT/SIGTERM handlers so an interrupt during the multi-minute build still restores the mutated entry; add a progress log before the spawn. - Match the double-quote style of the sibling server/ files. - Docs: list browser-externalized-builtin-gate.ts in the AGENTS.md server/ block and the clients/web/README server/ list (with why a build helper lives under server/), and add verify-build-gate to the root README scripts/ line. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * build(web): round-2 review hardening for the #1769 build gate - Scope the gate to the browser (`client`) environment via applyToEnvironment, so a future SSR/node environment built from this config isn't failed for a legitimate node:* import — browser-only intent is now structural. - Report ALL externalized built-ins in one build, deduped, instead of only the first offender (N leaks no longer means N fix-and-rebuild cycles). browserExternalizedBuiltinError now takes string[]; gate uses a Set. - verify-build-gate.mjs: pin Vite with `npx --no-install` so the drift check can never silently run a registry-fetched Vite instead of the repo-pinned one (the whole point is version fidelity); append the probe instead of prepending (ES imports hoist, and appending won't demote a future leading directive); clarify the signal-handler comment (handlers are a backstop — a blocked spawnSync tears down via the process group first). - AGENTS.md: update both `ci` pipeline enumerations to include verify:build-gate and drop the now-false "Storybook is the only CI step left out" claim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * build(web): round-3 review polish for the #1769 build gate - Error message: name the transitive-dependency remedy (a resolve.alias to a browser shim) — "remove the import / gate behind the backend" doesn't apply when the node:* import lives in node_modules. - isBrowserExternalizedBuiltinLog is now a `message is string` type guard, so recordLog drops its redundant `!== undefined` clause while still typing the Set.add() — a real one-liner, not a coverage-only removal. - verify-build-gate.mjs: docblock now states WHY it builds the real config + entry (catches config-level regressions — plugin removed, onwarn suppression added — that a temp config would miss), so the minute-long build isn't later "optimized" into a temp entry and silently lose that coverage. Guard the entry read so a renamed main.tsx fails through fail() with an actionable message. On a botched restore, save the captured original to a sidecar .bak and point there instead of recommending `git checkout --`, which would discard uncommitted edits. - AGENTS.md: fix the muddled CI-steps sentence (GitHub CI runs the whole chain as separate steps, Storybook last) and note the applyToEnvironment client-only scoping in the gate prose. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * build(web): round-4 review polish — sharper verify-build-gate diagnostics (#1769) All in scripts/verify-build-gate.mjs's failure paths: - Split the "build SUCCEEDED" failure into three diagnoses keyed off the captured output, so it names the right file: warning present but build passed → plugin not applying (removed from `plugins`, `applyToEnvironment` env-name mismatch, or an `onwarn` suppression); node:fs present but phrase absent → phrasing drift, update the constant; neither → probe never reached the graph. The old message hard-coded "phrasing drifted", which misdirects for the config-regression class this script exists to catch. Added applyToEnvironment to the docblock's config-regression list too. - Bound the build with a 10-min spawnSync timeout (+ SIGKILL); a timeout sets result.error (ETIMEDOUT) and reports via the existing branch, instead of burning to the GitHub job's 360-min default with no output. - Guard the probe write so a read-only/EACCES checkout fails through fail() with an actionable message instead of escaping as a raw stack. - SIGTERM now exits 143 (was 130, which is SIGINT's code). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * build(web): round-5 review polish — self-consistent verify-build-gate (#1769) All in scripts/verify-build-gate.mjs: - Drift guard: assert the gate source file still contains the mirrored KNOWN_PHRASE literal, failing fast if the two drift. Without it, a stale KNOWN_PHRASE after a phrasing update would make the three-way diagnosis misreport a plugin-not-applying regression as a phrasing drift — the exact misdirection the diagnosis exists to avoid. - Make the .bak safety net reachable on the path that needs it most: restoreEntry now catches its own write failure and routes to a shared saveBackupAndFail helper, instead of letting the write escape the finally as a raw stack (which skipped the .bak fallback and left the probe injected in the entry). - Add the fourth "plugin isn't applying" cause to the diagnosis: a future Vite emitting the warning before the client env's buildStart reset clears it (same symptom as the other three). - Flag once, on the failing-check path, that clients/web/dist holds a probe build so a local debugger doesn't serve it unaware. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * build(web): round-6 review polish — verify-build-gate literal contracts (#1769) All in scripts/verify-build-gate.mjs; one theme — the literals the script asserts on can each go stale independently of what they mirror: - Success key: assert on the gate's distinctive thrown-error prefix "Build failed (#1769)", not a bare "#1769". The issue number also appears in vite.config.ts comments, so a code frame from an unrelated build error there could otherwise make the check report OK with the gate dead — the one direction it must never get wrong (a false pass hiding a dead gate). - Drift guard now anchors on the BROWSER_EXTERNALIZED_BUILTIN_PHRASE assignment (regex) instead of a whole-file substring match, so the old wording lingering in a comment can't mask a changed constant; and its readFileSync of the gate module is guarded so a moved file fails through fail() actionably. - Extract PROBE_MODULE ("node:fs") used to build both the PROBE and the diagnosis check + messages, so changing the probe module can't silently misroute a phrasing-drift failure into "probe never reached the graph". - Comment the load-bearing diagnosis order (phrase before module-name). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * build(web): round-7 review polish — drift-guard both mirrored literals (#1769) - Extract ERROR_PREFIX ("Build failed (#1769)") and drift-guard it against the gate module too, and use it as the success key. Previously only KNOWN_PHRASE was guarded; the prefix is mirrored across the same TS/.mjs boundary for the same reason, so rewording the gate's thrown error would make a correctly-firing gate report "the build broke for another reason" (benign direction, but the same misdirection the other guards close). - Write the restore-failure .bak under the OS temp dir, not next to the entry: a sidecar inside src/ is untracked and a `git add -A` (likely while recovering) would sweep it into a commit. - Branch-(b) remediation now says update BOTH the gate constant AND KNOWN_PHRASE, so a phrasing-drift fix lands in one pass instead of tripping the drift guard on the next run. - Note next to BROWSER_EXTERNALIZED_BUILTIN_PHRASE that it must stay a single string literal (the script's drift guard anchors a regex on the assignment). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * build(web): round-8 review polish — safe recovery path + structural onLog order (#1769) - saveBackupAndFail no longer claims a backup it may not have written: it tracks whether the write succeeded and only points the user at the .bak in that case (otherwise it says the probe is still injected and to restore from VCS). Uses mkdtempSync for a unique dir instead of a fixed tmp filename, so a pre-existing/foreign-owned /tmp file can't make the recovery message point at someone else's content. - Guard the restore-verification readFileSync too, routing a failure through saveBackupAndFail so the whole restore path is uniformly safe (a raw throw there would skip the backup net). - vite.config.ts: enforce: 'pre' on the gate plugin so it runs ahead of the normal-plugin group in the onLog chain — a plugin whose onLog returns false filters the log for later plugins, so trailing the array would let a future log-filtering plugin silently blind the gate. Makes "the gate sees every log" structural rather than array-position-dependent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * build(web): round-9 review polish — prove the gate fired on the probe (#1769) The success key confirmed the #1769 gate fired, but not that OUR probe tripped it. If the repo already leaked a Node built-in, the gate would fire on that and the script would report OK even if the probe was tree-shaken or entryPath went stale — branch (c)'s failure silently inverted into a pass. Add an assertion that PROBE_MODULE appears among the gate's embedded offender warnings. Unreachable inside `npm run ci` (validate's build:web fails first on a pre-existing leak), but this script is a documented standalone command — run exactly when debugging a leak, the case where this mattered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * build(web): round-10 review polish — roll back a partial probe write (#1769) The probe writeFileSync uses the default 'w' flag (truncates at open), so a mid-write ENOSPC/EIO could leave clients/web/src/main.tsx empty or half-written — and its catch called fail() directly, the one path on this script that could destroy the entry rather than merely dirty it (every other read/write is already routed into the recovery net). Call restoreEntry() in that catch to roll the partial write back (falling through to the .bak net if the restore itself can't write), and correct the comment that claimed a write failure happens "before any mutation". Also widen the enforce:'pre' comment to note it orders ahead of Vite's core plugins too (harmless — the gate defines no resolve/load/transform). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * build(web): round-11 review polish — don't let the rollback swallow the write error (#1769) Round 10's restoreEntry() in the probe-write catch swallowed the primary error: restoreEntry() on its own write failure calls saveBackupAndFail → fail → exit, so the "could not write the probe" diagnosis never ran when the rollback also failed — which is exactly the read-only-checkout / ENOSPC case (both writes fail at open). On a read-only checkout nothing was ever written, yet the developer was told the restore failed and pointed at a /tmp backup of an untouched file. Roll back inline instead: attempt the restore without exiting, track whether it succeeded, then always report the PRIMARY write error, appending "restore from version control" only when the rollback didn't take. Also pin the "Build failed (#1769)" error prefix as contiguous in a single string fragment (a comment symmetric with the phrase pin), since verify-build-gate.mjs drift-guards on it as ERROR_PREFIX. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * build(web): round-12 review polish — don't let the pin comment defeat its own guard (#1769) The round-11 pin comment quoted "Build failed (#1769)" verbatim, so the prefix appeared twice in the gate module and verify-build-gate.mjs's whole-file includes(ERROR_PREFIX) drift guard passed on the comment alone — rewording the thrown error would have kept the guard green while a correctly-firing gate reported "the build broke for another reason." Reworded the comment to not repeat the full lead (matching how the phrase pin avoids repeating its phrase); the literal now occurs exactly once, restoring the guard's meaning. Also make the probe-write recovery message factual instead of inferred: read the entry back and only warn "restore from version control" when it actually differs from the original. On a read-only checkout both writes fail at open, so the file is untouched and the old hedge needlessly implied damage. 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
|
73d9b181fe |
History: MRTR conversation grouping + modern message vocabulary (#1705)
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
* History: MRTR conversation grouping + modern message vocabulary Teach the Protocol/History view the modern (2026-07-28) message vocabulary, correlating it off the frames the transport already captures — no change to how MRTR is driven (manual driving is deferred to #1704). - Correlation helpers in protocolUtils: extractResultType, extractRequestState, extractSubscriptionId, isModernFrame, and groupProtocolEntries — clustering the contiguous rounds of one logical operation (original call -> input_required -> retried call with a new id -> final result) that share a requestState token. - New MrtrConversation group renders that cluster as one expandable unit with a status derived from the final round, reusing ProtocolEntry for each round. - ProtocolEntry: resultType badge (input required / complete), a "modern" frame badge for server/discover, subscriptions/listen, and the acknowledged notification, and a copyable subscriptionId. - ProtocolListPanel folds each section through the grouping pass and shows an era badge in the header. Era is threaded from the negotiated protocolEra (App -> InspectorView -> ProtocolScreen -> panel) and never inferred from frames, respecting the transcript-classification caveat. - Shared EraBadge element (+ eraUtils); ConnectionInfoContent reuses it. - test-servers/configs/modern-mrtr-http.json serves an elicitation tool over the modern leg for eyeballing a real MRTR round-trip. - Pre-bundle @modelcontextprotocol/client in optimizeDeps.include so the Storybook vitest project doesn't re-optimize mid-run once protocolUtils pulls the client barrel into the browser graph. Closes #1627 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test-servers: real MRTR tool + live-verified History grouping While driving the modern flow in the browser, the elicitation-based config proved unable to produce MRTR: the modern (2026-07-28) SDK server rejects `server.elicitInput` — server→client requests aren't allowed on that leg; a tool must instead return `inputRequired(...)`. So the shipped `modern-mrtr-http.json` (and its README claim) were wrong. - Add a genuine MRTR tool preset (`mrtr_confirm` / `createMrtrTool`): its handler returns `inputRequired({ inputRequests: { confirm: elicit(...) } })` on the first call and the final result once the client echoes the answer back on the retry. - Teach the composable test-server to support it: the tool wrapper forwards an `InputRequiredResult` verbatim (instead of coercing it to a CallToolResult), and `toHandlerExtra` surfaces `inputResponses`/`requestState` so a handler can branch between the original call and the retry. - Point `modern-mrtr-http.json` at the new preset; correct the README to describe the real mechanism. - Add an integration test that drives the full round-trip through InspectorClient (auto-fulfilling the embedded elicitation) and asserts the final result is only reachable after the input_required round + retry — regression cover that would have caught the original config bug. Verified live in the browser: the History view renders the round-trip as one grouped "MRTR conversation" (Round 1 input_required → Round 2 complete), keyed by the real requestState, under the Modern era badge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Address review notes: dedupe status badge, unique MRTR token, import alias Non-blocking review feedback on #1705: - ProtocolEntry: suppress the redundant green "OK" status badge when a `resultType` badge already conveys the outcome (a modern success shows only `complete`/`input required`). Errors/pending have no `resultType`, so their status badge still shows. - createMrtrTool: mint a unique `requestState` per original call (closure counter) so two same-action invocations don't fold into one conversation during manual eyeballing. Real SDK tokens are already unique per operation. - ProtocolEntry: align the `MessageEntry` import to the `@inspector/core` alias used by its siblings (was a deep relative path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Review round 2: green `complete` badge, lighter embedded MRTR container Non-blocking observations from the second review: - ProtocolEntry: `resultType` "complete" now renders green (was gray) so a modern success keeps the at-a-glance green success affordance a legacy "OK" had, now that the redundant OK badge is suppressed. `input_required` stays yellow. - MrtrConversation: in the narrow monitoring (embedded) column, drop the outer container border and tighten padding — the per-round entries are already bordered cards, so this avoids a busy border-in-border nesting. The full-width view keeps the bordered surface that sets a conversation apart. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Review round 3: module-scope MRTR counter; revert embedded border change - test-servers: hoist the MRTR `requestState` counter to module scope. The modern (2026-07-28) leg is stateless — `createMcpHandler` rebuilds the server (and every tool closure) per request — so the previous per-closure counter reset to 0 on every request and every token ended in `:1`, defeating its purpose. A module-level counter survives the per-request factory recreation and increments monotonically across the process, so same-action invocations get distinct tokens and don't fold into one conversation. (Review finding.) - MrtrConversation: revert the embedded borderless/tighter-padding change — per maintainer feedback from manual testing, dropping the border didn't improve the narrow-column visual, so the group keeps its border in all layouts. 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
|
19a6df7ad3 |
SDK v2 migration (behavior-neutral, versionNegotiation: 'legacy') (#1624)
Migrate all three clients + core + test-servers from
@modelcontextprotocol/sdk@1.29 to SDK v2 (2.0.0-beta.4), keeping behavior
identical against legacy (2025-11-25) servers. Foundation for the SDK V2 +
New Spec stack.
Packages
- Split @modelcontextprotocol/sdk -> @modelcontextprotocol/client (Client,
transports, OAuth, error taxonomy, protocol types) + core (Zod schemas);
stdio -> client/stdio, Ajv -> client/validators/ajv. Test-servers ->
@modelcontextprotocol/server (+ server-legacy/sse). Deterministic import
rewrite (schemas -> core, everything else -> client); the codemod misroutes
client code to /server so it was not used for path routing.
- new Client(..., { versionNegotiation: { mode: 'legacy' } }) — nothing
2026-era goes on the wire.
Runtime (core/mcp/inspectorClient.ts)
- Error taxonomy: McpError -> ProtocolError, ErrorCode -> ProtocolErrorCode;
RequestTimeout -> SdkError/SdkErrorCode.
- Handlers take method strings; task methods (tasks/*, notifications/tasks/*)
use the 3-arg custom form (excluded from v2's spec-method set).
- Legacy tasks kept working (per project requirement): requestor tasks re-
implemented over raw client.request + explicit deprecated schemas;
experimental.tasks.callToolStream replaced by a local pollTaskToolCall
async generator (with _progressHandlers revival so task-execution progress
still reaches the caller). Receiver (server-initiated) tasks: the v2 Client
validates a sampling/elicit handler result and rejects the legacy { task }
response with -32602 — installReceiverTaskResponseBypass swaps the wrapped
entry in the private _requestHandlers map for the task-augmented branch so
{ task } reaches the wire (fixes server-initiated sampling-via-task).
- Pagination: list verbs use raw client.request({method}, List*ResultSchema)
for single-page behavior (v2's client.listX() auto-aggregate all pages).
Web / auth / apps
- Rename local JsonSchemaType -> InspectorFormSchema + add toFormSchema()
structural narrowing at the SDK-schema -> form boundary.
- ext-apps still peers on SDK v1: one compat boundary with documented casts
(remove when ext-apps#702 lands a v2 peer).
- OAuth provider saveClientInformation reconciled with v2's
OAuthClientInformationContext; InvalidClientMetadataError re-homed as a
local class (server-legacy/auth is not browser-safe).
- vitest.shared.mts dedupes @modelcontextprotocol/client+core to one copy so
vi.mock intercepts the SUT's import.
Accepted v2 behavior changes (per #1624 / spec §9.2): unknown-tool call
rejects with -32602; cancellation is stream-abort (no guaranteed
notifications/cancelled frame); output-schema strictness restored via the
Inspector's own Ajv check on the default path.
npm run ci green: validate (web 3087 / cli 212 / tui 274 / launcher 5),
coverage (>=90 per-file, all clients), smoke (launcher/cli/web), storybook
(410).
Closes #1624
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Cliff Hall
|
0ddb79203a |
web: Wave 2 — spec-conformant MCP Apps host (rollup of #1565–#1572) (#1660)
* web: sandbox hardening — opaque-origin inner frame, srcdoc-only, frame-ancestors Harden the MCP Apps sandbox isolation model: - The inner (untrusted-widget) iframe is sandboxed WITHOUT `allow-same-origin` (default "allow-scripts allow-forms") and any server-supplied sandbox value has `allow-same-origin` stripped case-insensitively, so the widget runs under an opaque origin and cannot reach the proxy's DOM to bypass its own CSP via the parent realm. - Widget delivery is `srcdoc`-only (no document.write), which respects the opaque origin. - The relay accepts the inner frame's "null" origin with `event.source === inner.contentWindow` as the unforgeable identity check. - sandbox-controller.ts serves the proxy with `Content-Security-Policy: frame-ancestors http://127.0.0.1:* http://localhost:*`; fetch directives are deliberately omitted so a srcdoc iframe's inherited policy container does not override the per-app CSP. Updated the isolation-model comments and the sandbox-controller integration test asserts the frame-ancestors header (and the absence of fetch directives). Closes #1565 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * web: enforce per-app _meta.ui.csp in the app bridge + surface resource-read failures - createAppBridgeFactory now builds each app's `_meta.ui.csp` host-side via the sandbox-csp library (approveCspSources → buildSandboxCspPolicy) and wraps the untrusted widget HTML with wrapSandboxedHtml so the CSP <meta> is always the first <head> child before the HTML reaches the sandboxed iframe. The approved (post-filter) csp + permissions are echoed back via hostCapabilities.sandbox so the view sees what was granted. The csp is no longer sent inline through sendSandboxResourceReady — it is enforced via the wrapped meta. - A per-app copy of HOST_CAPABILITIES prevents the sandbox echo from mutating the shared constant. - New optional `onResourceError` dep on AppBridgeFactoryDeps routes resource read/post failures to the caller (also always console.error'd) instead of leaving a silent blank frame. Closes #1566 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * web: full hostContext delivery + live host-context-changed updates - createAppBridgeFactory now seeds the full hostContext via snapshotHostContext(iframe, HOST_AVAILABLE_DISPLAY_MODES): theme, styles (Mantine design tokens), displayMode ("inline"), availableDisplayModes, and containerDimensions — replacing the theme-only seed. The local currentTheme helper is dropped in favor of hostContext.ts. - AppRenderer gains `displayMode` and `containerRef` props and pushes live host-context changes via the SDK's sendHostContextChange (partial host-context-changed notifications), never a hand-maintained snapshot: - theme + styles via a MutationObserver on <html data-mantine-color-scheme>, - containerDimensions via a ResizeObserver on the host container (or the iframe fallback), gated on `initialized`, skipping 0x0 and value-equal repeats, plus a one-shot push on initialize once layout settles, - displayMode whenever the prop changes (Maximize/Restore), gated on init. Adds AppRenderer host-context tests (theme flip incl. resolved styles, displayMode push + init gate, container-dimensions on init and resize, observer teardown) and updates the factory + story mocks for the new hostContext shape and sendHostContextChange method. Closes #1567 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * web: honor ui/notifications/size-changed and ui/request-display-mode AppRenderer: - New `onSizeChange` prop forwards the view's ui/notifications/size-changed reports (via the bridge `sizechange` event). - New `onRequestDisplayMode` prop backs an `onrequestdisplaymode` bridge handler that returns the mode the host actually applied; with no handler the request is declined by returning the current host-side mode (falling back to inline). AppsScreen: - Adds a host-controlled RendererContainer (measured for containerDimensions) with an inner box sized by the view-reported height (`appHeight`), so host→view container size and view→host size-changed don't couple. - `handleSizeChange` records the reported height (ignored while maximized); `displayMode` is derived from the maximize toggle and pushed to the view; `handleRequestDisplayMode` filters against HOST_AVAILABLE_DISPLAY_MODES (declining e.g. "pip") and maps fullscreen↔inline onto the maximize state. Adds AppRenderer tests (size forwarding, request-display-mode apply/decline/ fallback) and AppsScreen tests (frame sizing on report, no-height ignore, fullscreen/restore/decline via request-display-mode). Story mocks gain sendHostContextChange/addEventListener. Closes #1568 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * web: honor ui/download-file with confirmation and http(s) allowlist createAppBridgeFactory now advertises the `downloadFile` host capability and attaches an `ondownloadfile` handler: - Requires a host-mediated confirmation (window.confirm) listing the requested file(s); a declined prompt or an empty payload returns isError without acting. - Inline EmbeddedResources (text or base64 blob) are written to disk via the shared downloadBlob object-URL anchor; base64 is decoded via base64ToBytes. - ResourceLinks are opened in a new tab, restricted to http(s) by the shared isHttpUrl allowlist (javascript:/data:/file:/malformed are rejected). - Labels shown in the confirmation are sanitized (control + format chars stripped, length clamped) so a server-supplied filename/URI can't inject newlines or reflow the prompt. - Partial-batch success is surfaced (isError only when nothing downloaded), with skipped items warned. Adds factory tests for the full download surface (text/blob/link, decline, empty, throw, non-http rejection, label sanitization/clamping, partial batch). Closes #1569 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * web: surface ui/message and app logging in panels below the running app AppRenderer: - New `onMessage` prop backs an `onmessage` bridge handler that surfaces the view's ui/message content and returns the spec-required empty result; with no handler the submission is declined (isError). - New `onLog` prop forwards MCP `notifications/message` log entries via the bridge `loggingmessage` event, honoring the advertised `logging` capability. AppsScreen: - Adds `messages`/`appLogs` state (cleared with the reported height on select/open/close/back via resetAppChannels) and `handleMessage`/`handleLog`. - Renders a "Messages from app" panel (data-testid="apps-messages", reusing ContentViewer) and a default-expanded collapsible "App logs" panel (data-testid="apps-logs", reusing LogLevelBadge) with logger names and a Clear button, both as pinned panels below the running widget. Adds AppRenderer tests (log forward + no-handler no-throw, ui/message route/empty-result/decline) and AppsScreen tests (message log render + empty result + clear-on-close, log panel expand/collapse/clear). Closes #1570 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * web: stage partial input — replay tool-input-partial before final tool input AppRenderer: - New `partialInputs` prop (ordered fragments), snapshotted into pendingPartials at bridge-build time (read via a ref so prop churn never rebuilds the iframe) and cleared on dispose. flushPending replays them via bridge.sendToolInputPartial BEFORE the complete tool-input, per spec. AppsScreen: - New `partialStages` state + `handleStagePartialInput` (snapshots current form values). resetAppChannels gains a `keepPartials` option so Open App preserves the staged fragments (they're consumed by the renderer) while select/close/ back clear them. - Adds a "Stage partial input" control (fielded apps only) with a staged count and "Clear staged", wired to AppRenderer's `partialInputs`. Adds AppRenderer tests (ordered replay before tool-input; none when omitted) and AppsScreen tests (stage/count/clear gated on fields; staged partials survive Open and are replayed). Closes #1571 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * web: app lifecycle status + machine-readable data attributes on the Apps screen AppRenderer: - New AppRendererStatus ("loading" | "ready" | "error") + onAppStatusChange callback. Fires "loading" at the start of every (re)build, "ready" on the view's initialized signal, and "error" on a factory throw/rejection. AppsScreen: - appStatus/appError state; handleAppError captures the reason locally and forwards to the parent onError. resetAppChannels clears both. - Surfaces the contract as data-testid="apps-form" carrying data-app-status and data-app-error, renders an error panel (data-testid="apps-error") in place of the blank frame, and tags the stage button data-testid="apps-stage". - AppDetailPanel's Open App button gains data-testid="open-app". App.tsx: - Wires the bridge factory's onResourceError to a red toast so a malformed/404 UI resource is no longer console-only (the renderer separately drives data-app-status, so a driver times out on never-reaching-ready and reads it). Documents the data-testid/data-* automation contract in clients/web/README.md. Adds AppRenderer status-transition tests and AppsScreen status/error-panel tests. Closes #1572 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review: warn when server sandbox drops allow-scripts + revert incidental lockfile churn Addresses the claude[bot] review on PR #1650: - sandbox_proxy.html: log a console.warn when a server-supplied `sandbox` value lacks `allow-scripts` after the `allow-same-origin` strip, so a fail-closed (app-never-runs) sandbox is diagnosable instead of silent. - Restore clients/{cli,launcher,tui,web}/package-lock.json to their v2/main state — the earlier churn (@emnapi/@napi-rs optional deps, a dropped version field) was incidental `npm install` regeneration unrelated to the sandbox change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review: document CSP-vs-permissions filtering asymmetry (PR #1652) Address the claude[bot] review's finding #2: add a comment explaining why `csp` is injection-filtered (approveCspSources) while `permissions` is passed through verbatim — permissions is a structured boolean object consumed only by the sandbox proxy's buildAllowAttribute() (fixed key→token mapping), so it never reaches the iframe sandbox/allow attribute as raw text. No logic change. Also merges v2/1565-sandbox-hardening (review fixes) up the stack. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * test: lock in per-app HOST_CAPABILITIES isolation (PR #1652 round 2) Add a test asserting the shared HOST_CAPABILITIES.sandbox stays undefined after a sandboxready run sets hostCapabilities.sandbox — guards against a silent regression if the per-app spread copy were ever dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review: gate theme observer on initialized + document seed/dep assumptions (PR #1654) Address the claude[bot] review: - Finding #1 (applied): gate the theme+styles MutationObserver on the view's `initialized` signal, matching the container/displayMode pushes — a theme flip in the construction→handshake window no longer races ui/initialize. The factory seeds the construction-time theme, and the first post-init flip carries the current value. Added tests: pre-init flip is dropped; the live flip test now completes the handshake first. - Finding #2 (applied, comment): note in snapshotHostContext that the `displayMode: "inline"` seed assumes inline-at-open (AppsScreen always opens inline; #1568's displayMode push carries transitions). - Finding #4 (applied, comment): explain why `containerRef` is a build-effect dep yet doesn't force a rebuild (sameInputs ignores it; read lazily). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review: guard 0-height size reports, distinct RendererFrame, shared payload type (PR #1656) Address the claude[bot] review: - #1 (applied): handleSizeChange ignores a non-positive height — a view's ResizeObserver can transiently fire 0 pre-layout/teardown, which would collapse the frame; mirrors AppRenderer's 0×0 skip. Added a test. - #2 (applied): reinstate a distinct `RendererFrame` constant for the inner content-sized box so its role reads clearly vs. the outer RendererContainer. - #3 (declined + comment): keep carrying `appHeight` across maximize→restore — clearing it would flash the frame to full-card height until the next size-changed; the carry-over restores at the prior size. Documented. - #4 (applied): type handleSizeChange's param as AppBridgeEventMap["sizechange"] so the payload shape has a single source of truth with AppRenderer's prop. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review: distinguish links, cap batch size, comment truncation (PR #1657) Address the claude[bot] review: - #1 (applied): the confirmation now reads "download or open N item(s)" and resource_links are prefixed "↗" in the summary, so a link that opens in a tab is distinguishable from an embedded file that saves to disk. - #2 (applied): cap a single ui/download-file batch at MAX_DOWNLOAD_ITEMS (20); an oversized batch is rejected (isError) before the prompt, with a warn. - #3 (declined): can't check window.open's return value to detect a blocked popup — we pass `noopener`, and per spec window.open returns null even on a successful noopener open, so null can't distinguish blocked from succeeded. - #4b (applied, comment): note the start-of-label truncation is intentional so a link's scheme+host stays visible for the trust decision. Adds tests: oversized batch rejected without confirming/acting; resource-link ↗ prefix in the summary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review: skip embedded resource with neither text nor blob (PR #1657 round 2) Address the round-2 observation: an untrusted EmbeddedResource carrying neither `blob` nor a string `text` previously produced `new Blob([undefined])` — a file containing the literal text "undefined", counted as success. Now such an item is skipped (returns false, like a rejected link), so isError reflects it. Added a test asserting no object URL is created and the batch reports isError. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review: type-honest formatLogData, cap channels, log-panel a11y (PR #1658) Address the claude[bot] review: - #1 (applied): formatLogData coalesces JSON.stringify(undefined) → "" so the `: string` return type is honest for a data-less log. - #2 (applied): cap retained messages/logs at MAX_APP_CHANNEL_ENTRIES (500) via appendCapped — a chatty widget no longer grows the panels/DOM without bound between Clear/close; oldest entries drop. - #3 (declined): keep the Messages panel without a Clear/collapse — messages are sparser and already cleared on close/switch; keeping it simple. - #4 (applied): the App-logs toggle now has aria-controls pointing at the Collapse region (id="apps-logs-region") for assistive tech. Adds tests: no-data log renders (coalesce branch), 501 logs cap at 500 (oldest dropped), aria-controls assertion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review: stable id key for Messages panel (PR #1658 round 2) Address the round-2 nit: give AppMessage a stable `id` (assigned via nextMessageIdRef in handleMessage) and key the Messages panel on it instead of the array index — so the appendCapped front-drop can't renumber keys once a run exceeds the cap, matching AppLogEntry's treatment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review: note partialStages intentionally survives handleOpen (PR #1659) Address the claude[bot] review's observation #2: add a comment at the handleOpen call site clarifying that partialStages is intentionally NOT cleared there — AppRenderer snapshots the fragments into its own pendingPartialsRef at build time, and the staging UI only renders while not running, so the surviving state is invisible until the next reset drains it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review: align error-panel docs with behavior + gate data-app-error on running (PR #1660) Address the claude[bot] review: - #1 (applied, docs): the error panel renders BELOW the (blank) frame, not in place of it — the renderer stays mounted so a factory error still shows the reason alongside the iframe (gating the frame off on error would drop the auto-launched iframe an integration test relies on). Reworded the README row and the test comment to say "below the frame"; added a code comment. - #2 (applied): gate data-app-error on `running` (like data-app-status) so the two attributes can't desync if a future path sets an error without running. - #3 (no change): resource-read failures staying at data-app-status="loading" (toast only) is intentional and documented. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
cliffhall
|
a26cbac609 |
review: harden version read + fix MonitoringScreen docstring
Address PR #1641 review feedback: - Add readInspectorVersionSafe() (non-throwing) in core/node/version.ts and use it in the web backend, so a failure to resolve the root manifest hides the (cosmetic) version badge instead of crashing the backend at startup. Covered by tests for both the resolved and unresolvable paths. - Update MonitoringScreen's docstrings: it now wraps every supplied screen in a ScreenStage (only the active tab mounts, cross-fading) rather than rendering just the active tab's node. The third fetch-consolidation suggestion (one useInitialConfig instead of three GET /api/config hooks) is tracked as a follow-up in #1643. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw |
||
|
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 |
||
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> |
||
|
Cliff Hall
|
c96db0402e |
test: raise per-file coverage gate to 90 across all clients (closes #1271) (#1532)
* test(web): audit branch-coverage outliers, raise per-file gate to 70 (#1271) The web per-file coverage gate had drifted red: the recently-landed EMA auth feature (core/auth/ema/*) and ClientSettings components shipped undertested, and the prior 50% branch floor let business-logic branches go untested. CI does not run the coverage gate, so this accumulated. Add tests to bring every failing file above the gate and push branch coverage well past 70 across the board, then raise the per-file branches threshold 50 -> 70 in clients/web/vite.config.ts. - EMA core (emaFlow, resourceContext, jwt, wire): 50-88% -> 86-100% branch - EMA OIDC/transport (idpOidc, tokenEndpoint, transportProvider): -> 97-100% - oauthManager / discovery: 73-83% -> 88-92% - ClientSettingsModal/Form, ConnectionInfoContent: 0-88% -> 96-100% - useClientSettingsDraft / useEmaIdpLoginState hooks: 20% -> 100% - TaskStatusBadge / SortableServerCard outliers: 50% -> 100% Coverable branches reached by mocking useComputedColorScheme (dark theme), dnd-kit useSortable (isDragging), and fetch/OIDC paths. Remaining sub-100 branches are genuinely-unreachable defensive guards or happy-dom-inherent (portal / useMediaQuery / typeof window) and documented in test comments. No source files changed. Global web branch coverage 88.73% -> 90.63%. AGENTS.md updated; cli/tui/launcher stay at 50 (out of scope). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: raise per-file coverage gate to 90/90/90/90 across all clients Follows the #1271 web branch-coverage audit (50 -> 70) by raising the whole per-file gate to >=90 on all four dimensions — lines, statements, functions, branches — for web, cli, tui, and launcher (core/ is measured under the web suite). A codebase-wide audit added real tests for every outlier and brought global coverage up across the board. Config: all four vitest/vite coverage configs set to 90/90/90/90. Two pure-type modules (core/mcp/import/types.ts, clients/web/server/ types.ts) added to the web coverage exclude list, matching the existing type-only exclusions. Tests: ~50 test files extended/created across core (auth, ema, mcp, remote, state, storage, react), web components/screens/server, and the cli/tui clients. Highlights: full EMA OAuth flow + transport coverage, inspectorClient/remote server backfill, every Mantine control's handler branches, and the cli unsupported-method / argv-fallback paths. Genuinely-unreachable branches are annotated at the source with justified `/* v8 ignore -- <reason> */` comments rather than lowering the gate: happy-dom-inherent paths (Mantine portal, useMediaQuery, typeof window SSR guards), React StrictMode effect replay, and provably-dead defensive guards (?? fallbacks the types make impossible, Select.onChange values outside the allowed list). No runtime logic changed; the only structural edits are two behavior-preserving refactors (ResourceTemplatePanel hoists the field value to a local so the ignore applies in JSX; ToolsScreen swaps a ternary for an if) and prettier line-wrapping. Also fixed a flaky real-timer debounce assertion in PromptArgumentsForm (asserted an exact sibling-typing interleaving that raced the 300ms debounce; now asserts the load-bearing fact — a non-empty fire-time sibling capture). AGENTS.md updated to describe the uniform 90 gate and the v8-ignore policy. All four client coverage gates, root validate, storybook (370), and smoke pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(web): make connect-crash SSE reader stop on the terminal event `readSseEvents` looped until the server closed the stream (`done`). The server closes the events stream immediately after writing the terminal `transport_error`, but under the heavy parallel scheduler pressure of a full `coverage` run (v8 instrumentation across all workers) that close can lag far enough behind the event to trip the 30s test timeout — a hang, not a wrong result. This made the test intermittently fail under `npm run coverage` (it passed in isolation and under CI's lighter `test:integration`). Stop reading as soon as the terminal `transport_error` event arrives (cancelling the reader) instead of waiting only for `done`. That event is always the last one the session queue emits, so nothing is lost, and the test no longer depends on stream-close timing. The `done` break is kept for any stream that closes first. Verified green across two consecutive full `npm run test:coverage` runs (3094 tests) where it previously hung, plus isolation (~0.5s). 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
|
36b48e61a2 |
feat(web): build dist on demand for prod --web + end-to-end CI smoke (#1486) (#1497)
* feat(web): build dist on demand for prod --web + end-to-end CI smoke (#1486) Prod `mcp-inspector --web` serves static assets from clients/web/dist, which only exists after a build. In a fresh checkout it was absent and the prod path served a broken page (ENOENT on index.html); the launcher smoke test only covered --help, so this path was unverified in CI. - ensure-web-build.ts: build dist on demand (vite build via build:client) when missing, or exit with an actionable error if the build can't run. - run-web.ts: invoke ensureWebBuild before startHonoServer in the prod path (--dev runs Vite directly and skips it). - scripts/smoke-web.mjs + smoke:web: start --web against built dist and assert GET / returns 200 with the injected __INSPECTOR_API_TOKEN__. Wired into validate:launcher and a new CI step. - Docs: launcher README + AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web): surface spawn error in build-on-demand; clarify smoke comment Address PR #1497 review: - runViteBuild now logs result.error.message (e.g. ENOENT when npm isn't on PATH) before returning a null status, so the spawn-failure cause surfaces instead of only the generic actionable error. The build seam gains a `log` param; added a test covering the ENOENT path. - Reword scripts/smoke-web.mjs header so it no longer implies the readiness wait covers a cold build-on-demand vite build (it's sized for a pre-built dist, which validate/CI guarantee). 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
|
a1f40a4222 |
fix(web): remove config-load warning filter, fix root cause (#1491) (#1494)
The `UNRESOLVED_IMPORT` warnings that `quiet-config-warnings.mjs` suppressed no longer occur, so the filter (and the build-time `onwarn` suppressions for the same three deps) are removed. Root cause: Vite's config bundler (Rolldown) statically walks the node-only dev backend (`core/mcp/remote/node/server.ts`) when it loads `vite.config.ts`, reaching `chokidar`, `atomically`, and `@napi-rs/keyring`. `core/` has no node_modules of its own, so at config-bundle time those deps resolve from `core/`'s parent chain — the repo-root `node_modules`. Before #1452 the deps lived only under `clients/web/node_modules` (a sibling, off the resolution chain), so Rolldown emitted `UNRESOLVED_IMPORT`. #1452 declared them in the repo-root `package.json` (for the CLI/TUI/launcher, which consume core/'s node code), installing them at the repo root and making them resolvable. Verified by hiding the root copy: the warning returns; restore it and the dev/build startup is clean. A dynamic import of the backend does not help — Rolldown follows dynamic imports during config bundling too. The fix is the deps being resolvable, so the suppression is just removed and the invariant is documented in `vite.config.ts`. - Remove `clients/web/server/quiet-config-warnings.mjs` + `.d.mts` and its test - Restore the `dev` script to plain `vite` (drop the `--import` hook) - Remove the now-dead `build.rollupOptions.onwarn` UNRESOLVED suppressions (never fire: the Hono plugin is `apply: 'serve'`, out of the build graph) - Document why config-bundle resolution succeeds in `vite.config.ts` - Update AGENTS.md project tree Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Bob Dickinson
|
2488ca5e38 |
feat: port CLI, TUI, and launcher from v1.5 into v2 (#1452)
Ports the CLI, TUI, and launcher from v1.5 into v2 behind a single `mcp-inspector` entrypoint (launcher → web/cli/tui). Includes the `mcp-inspector --web --dev` launcher fix (Vite alias resolution + prod-server double-start), the root `postinstall` install cascade with simplified CI, and spec/AGENTS docs (no-workspaces rationale, Development setup). Closes #1246. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Bob Dickinson
|
62d442b073 |
Revert "First pass at pulling the v1.5 CLI, TUI, and Launcher clients into v2."
This reverts commit
|
||
|
Bob Dickinson
|
467b01edd6 | First pass at pulling the v1.5 CLI, TUI, and Launcher clients into v2. | ||
|
Cliff Hall
|
8ebd55723f |
fix(dev): handle client disconnect in the dev proxy stream pump (#1418) (#1419)
The Hono dev-proxy middleware pumped streaming /api responses to the Node ServerResponse without handling client disconnect. When the browser dropped a stream mid-flight (SSE close, navigation, HMR reload) `res` was destroyed and the next write failed with ERR_STREAM_DESTROYED, which the pump logged as an error. It also had no `error` listener (a failed write could surface as an uncaught exception) and no `close` listener (it kept reading upstream after the client left). Make the pump disconnect-aware: - Listen for res `close`/`error`; either cancels the upstream reader (resolving the pending read so the loop exits) and flips a `clientGone` flag. - Treat ERR_STREAM_DESTROYED / EPIPE / ECONNRESET as a normal end — not logged. - Only log genuinely unexpected stream errors. - Await each write so a slow client applies backpressure instead of the proxy buffering the whole stream. Dev-only plugin (excluded from the coverage gate — it needs a real Vite server to exercise). Verified via npm run validate. Closes #1418 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
8be1680494 |
chore(web): silence benign config-load dep warnings in npm run dev (#1408)
* chore(web): silence benign config-load dep warnings in `npm run dev` `vite dev` printed three UNRESOLVED_IMPORT warnings at startup (chokidar, atomically, @napi-rs/keyring). They originate from Vite's config bundler (Rolldown): the Hono plugin statically imports the node-only dev backend (core/mcp/remote/node/server.ts), so loading vite.config.ts bundles it and Rolldown walks its node-only deps. Those deps are correctly externalized — the browser never loads them and Node resolves them at runtime — so the warnings are pure noise. They can't be filtered the usual ways: the static import is required (the monorepo has no node_modules under core/, so the aliases must resolve the deps at bundle time — ssrLoadModule / `--configLoader runner` both fail to resolve them), and the warnings are written directly to the process streams during config load, bypassing customLogger, logLevel, and rollupOptions.onwarn. Fix: a tiny `--import` hook (server/quiet-config-warnings.mjs) that drops only those specific benign warning writes, wired into the `dev` script. Scoped to `npm run dev` — build, tests, and prod are untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(web): harden dev warning filter per review - Tighten BENIGN_PAIRS to repo-relative path fragments (not basenames) so an unrelated future UNRESOLVED_IMPORT warning mentioning the same dep + a same-named file isn't silently dropped. Verified Rolldown prints the full repo-relative path. (review #1) - Add a note that the matcher tracks Rolldown's message format and degrades to a safe no-op (warnings return) if that format changes. (review #2) - Export isBenignWarning/BENIGN_PAIRS, guard the stream patching behind !VITEST, add a .d.mts so the pure helper is importable from TS, and add a unit test locking the signature pairs and the over-match guards. (review #3) 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
|
881ba4b5e8 |
feat(auth): inject MCP_INSPECTOR_API_TOKEN into served index.html (#1378) (#1382)
* feat(auth): inject MCP_INSPECTOR_API_TOKEN into served index.html (#1378) Reloading the web client at the bare URL (no `?MCP_INSPECTOR_API_TOKEN=…` query string) with empty sessionStorage made every `/api/*` request 401 — the browser had no way to recover the backend's auth token. Embed the token into `index.html` on every page load so the browser no longer depends on the query string surviving navigation: - New shared helper `clients/web/server/inject-auth-token.ts` embeds `<script>window.__INSPECTOR_API_TOKEN__ = "…"</script>` (escaped against `</script>` injection; no-op when auth is dangerously omitted). - Dev: the Vite plugin injects via `transformIndexHtml`. - Prod: the Hono server injects on the `/` route. - `App.tsx` `getAuthToken()` now reads the injected global first, then the query string, then sessionStorage (both fallbacks preserved). - Shared global name lives in `INSPECTOR_API_TOKEN_GLOBAL` (`core/mcp/remote/constants.ts`). Tests: helper unit coverage + an integration test exercising the real prod server's `/` → `/api/*` flow (injected token authenticates; missing token 401s). AGENTS.md documents the token-recovery order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): inject token into prod SPA fallback + prime sessionStorage + no-store (#1378) Addresses code-review feedback on the token-injection PR: - Prod `server.ts` SPA deep-link fallback (e.g. `/oauth/callback`) previously served the raw index.html off disk via serveStatic, bypassing injection — so a bookmark/reload at a non-`/` route with empty sessionStorage would 401. Route the SPA fallback through the same `serveIndexHtml` (inject) helper; real static assets (paths with a dot) still serve verbatim. Dev already injected on every HTML serve via Vite `transformIndexHtml`. - `getAuthToken()` now persists the injected `window.__INSPECTOR_API_TOKEN__` to sessionStorage (not just the URL-param branch), priming the backstop for any later navigation that loses the global. - Injected HTML responses now send `Cache-Control: no-store`, so a page carrying a token isn't cached and served stale after a restart regenerates the token. Integration tests added: SPA fallback (`/oauth/callback`) carries the token, `Cache-Control: no-store` on injected HTML, real assets served verbatim, and unknown `/api` routes 404 rather than falling through to the HTML shell. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
037225a9fb |
feat(servers): move oauthClientSecret and stdio env values to OS keychain (#1356) (#1360)
* feat(servers): move oauthClientSecret and stdio env values to OS keychain (#1356) mcp.json is designed to be tool-shareable (symlinked from Claude Desktop's config, pasted into bug reports, synced via dotfiles), so storing OAuth client secrets and stdio env values in plaintext there meant any of those flows could leak them. Lift both into the OS keychain via @napi-rs/keyring (active replacement for the archived keytar). The wire shape is unchanged: the GET /api/servers handler rehydrates from the keychain so browser code sees the same JSON; the on-disk file no longer contains the secret material. Includes an idempotent migration that lifts plaintext from older mcp.json files (or hand-edited ones) into the keychain on first read. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): tolerate keychain unavailability on non-set ops (#1356) CI on Linux failed because `@napi-rs/keyring` hard-errors without libsecret, and the route handlers were touching the keychain even for no-secret flows (defensive `deleteAllForServer` on POST/DELETE, sweep on PUT). The user's intent for the Linux fallback was "hard fail when a secret is actually involved" — not on every keychain touch. Make `KeyringSecretStore.get / delete / deleteAllForServer` silently degrade when the keychain is unavailable (return null / no-op); `set` remains the one operation that hard-fails with `KeychainUnavailableError`. The migration path catches the error and preserves the disk plaintext rather than losing the secret. Also inject `InMemorySecretStore` in `useServers.test.tsx` and `servers-events.test.ts` so tests never touch a developer's real keychain even when libsecret is present. Adds `KeyringSecretStore` tests (vi.mock'd native bindings) and an integration suite that simulates Linux-without-libsecret to verify the tolerance contract end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): close GET migration race; reverse keychain/disk order (#1356) Addresses code review: 1. **Concurrency**: GET /api/servers was reading + migrating outside the write lock, then writing inside it — a concurrent POST/PUT/DELETE between the read and the lock acquisition could be clobbered by the migration write. Also affected the first-launch seed write. Fix: take a fast unlocked peek; if the file is absent (seed branch) or carries plaintext to migrate (migration branch), re-read inside the write lock and decide there. Added `hasPlaintextSecrets()` as a pure predicate so the common no-migration path stays lock-free. 2. **Write ordering**: POST and PUT wrote disk first, then keychain. A keychain `set` failure mid-write left a half-configured entry on disk and trapped retries at 409. Reversed to keychain-first so a 503 leaves both stores in their pre-write state. PUT in-place now sets new fields, writes disk, then deletes obsolete fields (a failed disk write leaves orphan keychain entries — recoverable — rather than premature deletions of fields the user still expects to see). 3. **Tolerance simplification**: dropped the no-entry regex in `KeyringSecretStore.delete` — both "no entry" and "keychain unavailable" collapse to the same desired outcome (the entry isn't there anymore), so a uniform swallow is clearer. 4. **Parallelism**: `readKeychainEntriesFor` now `Promise.all`s the per-field `get` calls. macOS Keychain round-trips are 10-50ms; serializing 5+ env vars per server × N servers is a meaningful wall-clock cost on GET. 5. **Spec doc**: reflects the tolerance contract (only `set` hard-fails) and the keychain-before-disk write ordering rationale. New regression test in servers-route.test.ts covers the retry-after-503 contract directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(servers): parallel keychain writes + migration partial-state comment (#1356) Two nice-to-have observations from the re-review: - `writeKeychainEntriesFor` now `Promise.all`s its per-field sets, matching the read side. No ordering requirement among distinct (id, field) keys; on unavailability every set rejects and Promise.all surfaces the first rejection (still 503 from the route handler). - `migratePlaintextSecrets` catch block now spells out the partial-migration semantics: we return the original config so the disk plaintext stays intact, and the next successful GET retries with the keychain-wins branch absorbing already-set entries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(servers): parallel keychain deletes for symmetry (#1356) Picks up the symmetry observation from the third review: `deleteKeychainFields` now uses `Promise.all` like its read and write siblings. Distinct (id, field) deletes have no ordering requirement, and `secretStore.delete` is already a silent no-op on unavailability so Promise.all has no failure-mode surprise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
5f9bfdc490 |
feat(servers): hot-reload server list on external mcp.json edits (#1345) (#1357)
* feat(servers): hot-reload server list on external mcp.json edits (#1345) Watch ~/.mcp-inspector/mcp.json from the backend and push a change event on a new /api/servers/events SSE channel. The web client subscribes via fetch + ReadableStream (so the existing bearer-auth contract is preserved — EventSource can't send custom headers) and re-fetches on each event. The watcher is lazy: it starts on the first SSE subscriber and stops when the last one disconnects, so tests that never open the channel don't spin up a real fs watcher. Self-fires from the backend's own POST/PUT/DELETE are suppressed by capturing the post-write mtime and comparing it on each watcher event. Chokidar is used for cross-platform editor-save semantics (temp-file + rename produces unlink+add sequences that bare fs.watch handles poorly on macOS/Linux); awaitWriteFinish coalesces those into a single event. createRemoteApp now returns a close() to release the watcher, which the standalone server and the vite dev plugin chain into their existing close paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): address PR review on hot-reload (#1357) - writeMcpAndTrackMtime now stat()s the file before writing; if the mtime differs from the last value we recorded, an external editor slipped in between our writes and chokidar would coalesce both into one event whose mtime matches our own write — broadcast directly so peer subscribers learn about the external edit (review finding 1, 5). - Split useServers.refresh into a public refresh() (loading toggle) and an internal refreshInternal(background) the SSE handler calls; the list now stays visible during background refreshes instead of flashing skeleton state on every external mcp.json edit (finding 2). - Coalesce multiple \n\n frames landing in one decode chunk to a single refresh call so back-to-back broadcasts don't race two concurrent GETs whose setState order is unspecified (finding 3). - Clarify createRemoteApp's close() docblock about in-flight SSE stream callbacks (finding 7). - Add integration coverage for PUT/DELETE self-suppression and for the external-edit-between-backend-writes broadcast (finding 6). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): address pass-2 review on hot-reload (#1357) - Clarify writeMcpAndTrackMtime comment so a future reader can't mistake the broadcast for a content-preservation guarantee: the external edit's content may already have been read-around and overwritten depending on the interleave with readMcpConfig. - Drop the no-op GET in the PUT/DELETE self-suppression tests. The pre-stat branch is short-circuited by `lastWrittenMtimeMs === null` with or without it; matches the POST test's structure. - Add explicit no-flicker regression test: tracks every `loading` value across renders during an SSE-driven refresh and asserts none flipped back to true. Catches a future regression that reaches for setLoading(true) in the background path even under React batching. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
85a4b4c057 |
feat(servers): persist per-server settings to mcp.json (#1352) (#1353)
* feat(servers): persist per-server settings to mcp.json (#1352) - Add optional `settings` node alongside the transport config in each `~/.mcp-inspector/mcp.json` entry. Round-trips through `mcpConfigToServerEntries` / `serverEntriesToMcpConfig` and the `/api/servers` POST/PUT routes. - Drop `MCPServerConfig.headers` from `SseServerConfig` / `StreamableHttpServerConfig` and remove the headers textarea from `ServerConfigModal`. Custom headers are now entered only in `ServerSettingsForm` and reach the wire via `settings.headers`. - Wire `ServerSettingsModal` into `App.tsx`; saves dispatch through a new `useServers.updateServerSettings` mutator. `updateServer` carries the existing settings across config-only saves so they aren't dropped. - Apply settings on the *first* outbound request after a server loads — no need to open the settings form first: - `settings.headers` → SSE / streamable-http wire headers. - `settings.metadata` → `InspectorClientOptions.defaultMetadata`, merged into every outgoing `_meta` (call-time keys win). - `settings.requestTimeout` → `InspectorClientOptions.timeout`. - `settings.connectionTimeout` → Promise.race wrapper around connect(). - `settings.oauth*` → pre-seeded OAuth client credentials. - `RemoteConnectRequest` carries `settings` so the remote backend can source transport headers from the same source as the local path. - `oauthClientSecret` is persisted in `mcp.json` alongside stdio `env` values, both protected by the file's `0o600` permission. - Update `specification/v2_servers_file.md` with the new shape and the removal of the legacy headers field. Tests: round-trip in `serverList.test.ts`; persistence on POST/PUT in `servers-route.test.ts`; first-connect header injection in `transport.test.ts`; `defaultMetadata` merging in `inspectorClient.test.ts`; `useServers.updateServerSettings` + `updateServer` settings-preservation in `useServers.test.tsx`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): rebuild InspectorClient on every connect so settings refresh The previous onToggleConnection branch reused the existing InspectorClient on a same-server reconnect, which froze settings at the moment the client was first constructed. Editing headers, metadata, timeouts, or OAuth credentials in the settings form was therefore invisible until the user either switched servers or reloaded the page — surprising right after a visible save. Always call setupClientForServer on (re)connect so the latest target.settings flow through to the transport's headers / requestInit, to defaultMetadata, and to the OAuth manager. Also adds remoteClientTransport tests that pin the wire-level contract: settings round-trip through the /api/mcp/connect body when supplied, and the field is omitted when no settings are configured. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(history): stop wiping pendingRequestEntries inside the connect event State managers (ManagedToolsState, ManagedPromptsState, ManagedResourcesState, ManagedResourceTemplatesState, ManagedRequestorTasksState) handle the client's "connect" event by calling `void this.refresh()`. The synchronous portion of refresh runs through SDK Protocol.request → transport.send → MessageTrackingTransport.send → trackRequest, all before yielding to the network — so each list request gets tracked into MessageLogState's pendingRequestEntries inside the same connect event dispatch. MessageLogState's own "connect" listener was registered after those state managers (it's the last one constructed in App.tsx) and cleared pendingRequestEntries on every connect. Listener order meant the clear ran *after* the requests were tracked but before their responses arrived, leaving the history with unmatched "response" entries marked PENDING. The clear-on-disconnect (via statusChange → disconnected) already handles session-boundary cleanup, so clear-on-connect was purely defensive and now actively harmful. Drop the connect listener; keep the disconnect path. Tests: updated the unit test to assert the new behavior (connect does not clear); added an integration regression that wires a sibling connect listener firing listTools and asserts the response folds into the request entry instead of appearing as an orphan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: drop accidentally tracked debugging screenshot pending-requests.png was a local debugging artifact attached to a conversation, not source. Remove it from the tree (the working copy is kept so the user still has it locally) and rely on the existing untracked behavior going forward. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(servers): add e2e check that settings.headers reach upstream MCP Stitches the full v2 pipeline together in one assertion: browser-style RemoteClientTransport → POST /api/mcp/connect (settings in body) → backend createTransportNode applies settings.headers to requestInit → SDK StreamableHTTPClientTransport → upstream MCP test server Captures the upstream HTTP request via FetchRequestLogState and asserts both `X-Tenant` and `X-Trace` are present on the outbound POST. Catches any regression that breaks the wire between the form and the upstream server in a single integration test, instead of the previous coverage which exercised each hop in isolation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(ServerSettingsModal): invert ListToggle compact prop so icon matches state ListToggle's `compact` prop represents the list's *current* compact state: true means "currently compact" and renders the expand icon to advertise the action (open up). The settings modal was passing `compact={allExpanded}` — exactly inverted. When all sections were open it showed the expand icon (suggesting "open more") and when collapsed it showed the collapse icon, opposite of every other ListToggle caller in the app. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style(HistoryListPanel): match ServerListScreen Export button — rename + default variant Renames "Export JSON" to "Export" and switches from the subtle ToolbarButton to the same `Button variant="default"` used by ServerListControls so the two Export affordances look identical across screens. Also moves the button to the right of the ListToggle (mirroring the order on the server list screen) and disables it when there are no entries to export. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style(HistoryListPanel): move Clear button to top toolbar, match Export style Moves the Clear affordance out of the "History (N)" section header and up to the panel toolbar alongside Export, where it now uses the same `Button variant="default"` styling. Order in the toolbar is now: ListToggle · Clear · Export. Clear is disabled when there are no unpinned entries to remove (pinned entries are intentionally preserved). ToolbarButton helper is unused after this change and dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: ignore local debugging screenshots at repo root Stops `git add -A` from re-staging the `*.png` files Claude Code drops at the repo root from attached conversation screenshots. The pattern only matches the top-level (`/*.png`) so legitimate image assets in subdirectories (e.g. `clients/web/public/`) still get tracked when explicitly added. Also untracks the two screenshots that slipped through previously (initialize-request.png, pending-requests.png). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style(LogStreamPanel): match HistoryListPanel button styling Clear / Export / Copy All on the Logging screen were rendered with the subtle ToolbarButton helper while History's equivalents use `Button variant="default"`. Switch all three to the default variant so the toolbars look identical across screens and mirror the disabling behavior — each button is now disabled when `entries.length === 0`. ToolbarButton helper is unused after this change and dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): address PR review — debounce save, preserve-on-omit PUT, validate shape, lift connect timeout Round-up of fixes from https://github.com/modelcontextprotocol/inspector/pull/1353#issuecomment-4526861959: 1. ServerSettingsModal saves were firing on every keystroke (one PUT + full mcp.json refresh per character). App.tsx now debounces by 300ms and flushes on modal close so a burst of edits coalesces into one PUT and the form value can't flicker back mid-typing. 2. `PUT /api/servers/:id` with an omitted `settings` field used to silently wipe the persisted settings node. The route now treats the three intents distinctly: - field omitted → preserve existing on-disk settings - explicit null → clear the settings node - settings object → validate + apply `useServers.updateServer` is simplified to drop the client-side carry-through hack, and the test that documented the broken behavior is flipped + augmented with explicit-clear and bad-shape cases. 3. `connectionTimeout` Promise.race is moved from App.tsx into `InspectorClient.connect()`, reading `this.serverSettings?.connectionTimeout`. TUI/CLI consumers now get the same behavior; on timeout the client internally disconnects so the next connect starts clean. 4. Custom Headers hint in `ServerSettingsForm` notes that `Authorization` is owned by the OAuth flow when OAuth is configured — avoids the sharp edge where the SDK's authProvider silently overwrites a user-set Authorization header. 5. New `validateSettings` on the server route rejects malformed shapes (`settings: []`, `settings.headers: "oops"`, non-numeric timeouts, etc.) with 400 instead of letting them persist and crash the UI. 7 (nit). Renamed `settings` → `savedSettings` inside `setupClientForServer` so the variable holding the persisted settings doesn't shadow the later locally-derived values. Follow-ups opened separately: #1356 — Move oauthClientSecret out of mcp.json into the OS keychain (review #6). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): address PR review pass 2 — connect-timeout test, unmount cleanup, tagged intent, lenient settings read Pass-2 review at https://github.com/modelcontextprotocol/inspector/pull/1353#issuecomment-4527089705: (a) New integration test pins the new client-layer connect-timeout contract: a hanging stub transport with `connectionTimeout: 50` rejects with the descriptive error and lands the client in "error" status. Documents what TUI/CLI consumers should expect from the lifted Promise.race in InspectorClient.connect(). (b) useEffect cleanup clears `pendingSettingsTimerRef` on unmount so a route change or HMR mid-debounce can't fire one stale PUT against an unmounted component. (c) Refactor the PUT-route `settingsIntent` discriminator from `"preserve" | "clear" | { value }` to a tagged union with `kind`, and consume it with a `switch` for exhaustiveness — reads cleaner and makes adding a fourth intent painless. (d) `normalizeMcpServers` now runs `validateSettings` on the on-disk settings node and silently strips it if invalid, mirroring the existing lenient-on-read pattern from `normalizeServerType`. Closes the read/write asymmetry where a hand-edited malformed `settings` could survive on disk and get carried through preserve-on-PUT. New route test pins the strip-on-GET + no-propagate-on-preserve-PUT behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): pass-3 review — patch-style PUT, toast on close-flush fail Pass-3 review at https://github.com/modelcontextprotocol/inspector/pull/1353#issuecomment-4529276897. (b) `PUT /api/servers/:id` now treats both `config` and `settings` as optional patches: - field omitted → preserve the on-disk value - explicit body value → apply - settings: null → clear `updateServerSettings` stops reading `existing.config` from the in-memory `servers` snapshot, which closes the closure-capture hazard where a refresh between debounce-schedule and flush could silently revert a separate edit. The route is the single source of truth for transport config now. New tests cover settings-only PUT, rejected non-object config, and the empty-body no-op patch case. The previous "rejects a missing config" test is rewritten to document the new no-op-preserve semantics. (c) Inline comment near the PUT rebuild loop documents that writing the full mcpServers map back is what gives the route its desirable self-healing property for malformed settings on *other* entries — deliberate side-effect, not an accident of normalization. (d) `flushPendingSettings` surfaces failures via @mantine/notifications instead of swallowing them. The modal already closed by the time the flush PUT resolves, so a silent fail would leave the user thinking their last edits saved (especially painful for the OAuth client secret). The notifications wiring was already mounted in main.tsx. Pass-3 (a) — settings form being effectively uncontrolled mid-edit — is acknowledged in the reply: working as intended today; the proper fix is internal modal state when file-watching (#1345) forces the issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): pass-4 review — strip smuggled settings out of config Pass-4 review at https://github.com/modelcontextprotocol/inspector/pull/1353#issuecomment-4529861349. Main finding: `normalizeServerType` spreads unknown keys, so a body that nested `settings` inside `config` would smuggle a settings field onto the stored entry without ever passing through `validateSettings`. Strip the settings key off the incoming config in `buildStoredEntry` so `validateSettings` remains the single write path for the settings node — the "one source of truth" invariant pass 2 set up holds again. Two new tests pin the strip on both routes: - POST with `config.settings = { bogus }` lands an entry with no settings node on disk. - PUT with `config.settings = { bogus }` + `settings: null` clears the real settings *and* doesn't re-attach the bogus payload via the spread. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): pass-5 review — absorb late connect rejection, tighten validateSettings, log smuggle warning Pass-5 review at https://github.com/modelcontextprotocol/inspector/pull/1353#issuecomment-4529954490. Main finding: when the connect-time `Promise.race` lost to the timeout, the original `this.client.connect(this.transport)` promise was left pending. After `disconnect()` tore the transport down, real transports (SSE / streamable-http) would reject that promise later — with no handler attached, producing a Node `unhandledRejection` (and a vitest suite warning). Attach a no-op `.catch` right after creating the connect promise so Node sees the late rejection as handled. Nit (a): `validateSettings` was casting the raw object through to `InspectorServerSettings`, which let unknown keys ride along onto disk. Replaced the cast with an explicit pick-and-build over the known fields. Unknown stowaways now silently drop on the way through. New route test pins the behavior. Nit (b): `buildStoredEntry` now logs a `warn` via the route's pino logger when an incoming `config` carries a `settings` key. The strip is still defensive (it isn't a documented contract) but the warning lets a misconfigured client find out about the wasted field instead of debugging a silent drop. Nit (c) — the defensive `?? {}` fallback in `buildStoredEntry` — acknowledged in the review reply; left in place as belt-and-braces. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): pass-6 polish — scope absorber, thread id, drop empty OAuth, guard `in` Pass-6 review at https://github.com/modelcontextprotocol/inspector/pull/1353#issuecomment-4529979135. Reviewer signed off as ready; landing the four marginal cleanups since they're all small. (a) `connectPromise.catch(() => {})` moves inside the `if (connectTimeoutMs > 0)` branch. The await-only path propagates rejection cleanly through the outer try/catch — the absorber is only needed when the race leaves the original promise unattended. Code now reads as "absorber is tied to the race," which is the real invariant. (b) Thread `id` (POST id, PUT newId) through `buildStoredEntry` and include it in the smuggle warning's pino bindings. Two clients smuggling simultaneously are distinguishable in the log instead of indistinguishable. (c) `validateSettings` coerces empty-string OAuth fields to absent. The form emits `""` when the user clears an input, and an empty `oauthClientId` on disk would later be misread as "OAuth configured." New route test pins it. (d) `buildStoredEntry` adds a null/object guard before `"settings" in configObj`. The route layer pre-checks this on POST and PUT, but the helper's `unknown` parameter is a weak contract; the guard makes it safe to call from anywhere. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(spec): reflect settings persistence + patch-PUT contract in v2_servers_file The doc was authored when `mcp.json` only stored the server list. Bring it forward to match what shipped in #1352 / PR #1353: - Summary calls out that the file also stores per-server settings and links into the Per-server settings section. - On-disk format note explains that `settings` is an optional Inspector- specific extension that other MCP tools ignore. - New top paragraph on the Per-server settings section explains the basic-config / settings UI split rationale (simpler form for the common case, advanced settings tucked behind a second dialog). - UI bullet is rewritten to match the patch-style PUT semantics that emerged across the review cycle: settings-only and config-only PUTs preserve their counterpart inside the write lock, so neither modal can silently clobber the other half. The earlier "must re-send settings on PUT" hack is gone. - New bullets capture the save cadence (300ms debounce + flush-on-close + failure toast), the patch-PUT contract (omit-preserve / null-clear / rename), and the write-path / read-path validator gates. - Secret storage bullet links to #1356 (OS-keychain follow-up). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(spec): show a third entry with the optional settings node The On-disk format example demonstrated only basic stdio entries. Add a streamable-http entry with a populated `settings` node so readers can see the shape inline rather than having to scroll to the Per-server settings section. Inline comment in the block cross-links there for the full contract and notes that other MCP tools ignore the extension key. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
37a4d99575 |
Port v1.5 web dev backend (vite-hono-plugin + remote-server bootstrap) (#1321)
* feat(web): port v1.5 dev backend (vite-hono-plugin + sandbox + remote-server bootstrap) Ports the v1.5 in-process dev backend into v2 so `npm run dev` serves the Hono remote-server (`/api/*`) alongside the Vite-served browser app. Without this, the browser-side `createRemoteTransport` had nothing to talk to during local development. Adds under `clients/web/server/`: - vite-hono-plugin.ts (Hono middleware on the Vite dev server; gated to `apply: 'serve'` and skipped when `process.env.VITEST` is set so it stays inert in vitest/storybook runs) - sandbox-controller.ts (standalone HTTP server for the MCP Apps sandbox) - web-server-config.ts (env parsing + initial-config payload + banner; `autoOpen` defaults off under VITEST so test runs don't shell out to `open`) - server.ts (standalone Hono prod server for the future v2 launcher, #1246) - start-vite-dev-server.ts (in-process Vite starter for the future launcher) - vite-base-config.ts (shared optimizeDeps exclusions) Wires `honoMiddlewarePlugin(buildWebServerConfigFromEnv())` into `vite.config.ts` with `apply: 'serve'`. Adds the `open` dependency and a DOM lib + `@inspector/core/*` path alias to `tsconfig.node.json` so the dev-backend files type-check alongside vite.config.ts. Coverage: web-server-config.ts (100% lines), sandbox-controller.ts (91% lines), and vite-base-config.ts (100%) all clear the per-file gate. The Vite plugin, prod server, and dev-server starter are excluded from coverage as runtime glue exercised by `npm run dev` end-to-end (callers covered separately; their logic lives in core/mcp/remote/node, which integration tests already exercise). Closes #1320. Unblocks #1244 (Wire App.tsx) — its test plan can now run end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(web/server): address PR review feedback on the dev-backend port - sandbox-controller: resolve start() with empty values on listen error (e.g. EADDRINUSE on a fixed MCP_SANDBOX_PORT) instead of leaving the promise pending forever. Without this, the Vite plugin's `await sandboxController.start()` could hang the entire dev backend on a port collision. Banner now silently omits the sandbox line; getUrl() keeps returning null. Adds a regression test exercising the EADDRINUSE path against a real socket. - vite-hono-plugin: listen for `error` and `close` on the incoming request stream (in addition to `end`) so an aborted POST/PUT (browser navigates mid-upload) doesn't leak the middleware indefinitely. - vite-hono-plugin: add a `.catch()` to the initial `pump()` kickoff so a sync throw before the first `await reader.read()` can't surface as an unhandled rejection. - sandbox_proxy.html: drop the stale CSP comment — v1.5's sandbox HTTP server never set CSP either; this PR doesn't regress anything. Replace with the actual isolation model (iframe sandbox attribute + origin allowlist) so future readers don't assume a CSP header is enforced. - Hoist `WebServerHandle` to a shared `server/types.ts` so the future launcher port (#1246) doesn't need to pick one of two duplicate declarations. - Extract `resolveAutoOpen()` helper out of the triple-nested ternary in `buildWebServerConfigFromEnv` for legibility, with the three-way logic documented in one place. - Add an inline comment explaining why `webServerConfigToInitialPayload` falls back to streamable-http for unknown discriminators (forward-compat with future SDK transports) rather than throwing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
3c6f12dc6c | ||
|
|
925461a043 | ||
|
|
4dc5eaa6bb | ||
|
|
55db703196 | ||
|
|
12544d251c | ||
|
|
86e6774a9a | ||
|
|
5b9405fe8c | ||
|
|
566990382a | ||
|
|
6011d4de03 |
||
|
|
bb3ba91b8a |
||
|
|
fafc8367b8 |
||
|
|
fa441da85f |
||
|
|
8e8c1bbad9 |
||
|
|
ec40cf5531 |
||
|
|
09e1564672 | ||
|
|
b143dd6cbd | ||
|
|
bc408f5869 | ||
|
|
d830213796 | ||
|
|
519ecf49ee | ||
|
|
adde9ccf94 | ||
|
|
a09f2dcf1d |
||
|
|
cea22d86b8 |
||
|
|
a82a70f4f5 |
||
|
|
07d6316fba |
||
|
|
73d9b181fe |
||
|
|
19a6df7ad3 | ||
|
|
0ddb79203a |
||
|
|
a26cbac609 | ||
|
|
650e5bd020 | ||
|
|
13dafe457b |
||
|
|
c96db0402e |
||
|
|
36b48e61a2 |
||
|
|
91f02fd6db |
||
|
|
a1f40a4222 |
||
|
|
2488ca5e38 |
||
|
|
62d442b073 | ||
|
|
467b01edd6 | ||
|
|
8ebd55723f |
||
|
|
8be1680494 |
||
|
|
881ba4b5e8 |
||
|
|
037225a9fb |
||
|
|
5f9bfdc490 |
||
|
|
85a4b4c057 |
||
|
|
37a4d99575 |