Skip to content

feat(web): catalog vs read-only session launch model + apply --header (#1481, #1483)#1496

Open
cliffhall wants to merge 2 commits into
v2/mainfrom
feat/1481-launcher-config-web-catalog
Open

feat(web): catalog vs read-only session launch model + apply --header (#1481, #1483)#1496
cliffhall wants to merge 2 commits into
v2/mainfrom
feat/1481-launcher-config-web-catalog

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #1481. Closes #1483.

Implements the full catalog/session launch model from specification/v2_catalog_launch_config.md for the web client. Previously mcp-inspector --web resolved an initialMcpConfig that the v2 UI never read (the default* fields of /api/config have no consumer), so --server-url/--config showed nothing and --header only printed a warning.

Behaviour

Launch Server list source writable UI
mcp-inspector --web default catalog ~/.mcp-inspector/mcp.json true full CRUD
--catalog <path> / MCP_CATALOG_PATH that file (seed/CRUD/watch) true full CRUD
--config <path> that file, read-only (never written/seeded/migrated) false shown, CRUD hidden
ad-hoc --server-url/command (+ --header) one server held in memory (no file) false shown, connects with headers, CRUD hidden

--catalog/--config are mutually exclusive; neither combines with an ad-hoc target or --header; --header requires an ad-hoc HTTP/SSE server. Every illegal combination exits with a clear error — no silent no-op remains (the #1483 acceptance criterion).

The read-only design deliberately protects a foreign config passed via --config (e.g. claude_desktop_config.json): the backend never writes, seeds, or migrates plaintext secrets from it.

Changes

  • Backend (core/mcp/remote/node/server.ts): writable + initialServers options on RemoteServerOptions; three-way GET /api/servers (in-memory / read-only-file-no-seed-no-migrate / writable); 403 guard on POST/PUT/DELETE/order; writable on /api/config; watcher skipped for in-memory sessions.
  • Web layer: writable/initialServers threaded through WebServerConfig → both createRemoteApp call sites; run-web flag resolution rewritten (adds --catalog/MCP_CATALOG_PATH, makes --config read-only, seeds ad-hoc in-memory with --header, conflict matrix).
  • Frontend: useServerListWritable hook (reads /api/config); App → InspectorView → ServerListScreen thread writable; 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 + flag behaviour table; launcher README documents the flags.

Scope / follow-ups (out of scope, separate issues)

--server selection / defaultServerId + auto-connect (#1183), UC2 import (#1348/#1435), and the CLI/TUI resolveServerList + writable unification (#1482/#1432).

Verification

  • npm run validate (web format/lint/build/coverage + cli/tui + launcher smoke) and npm run test:storybook both green.
  • New tests: run-web resolution matrix + conflicts; backend read-only 403 + in-memory serve + file byte-stability after GET (no migration) + no-seed-on-missing; /api/config writable; events no-broadcast for in-memory; useServerListWritable; ServerCard/ServerListScreen writable=false gating.
  • Manual end-to-end against the built launcher: --configwritable:false, file byte-identical, mutations 403; ad-hoc --header → in-memory server with headers; --catalogwritable:true, POST persists to the file.

🤖 Generated with Claude Code

…#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>
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jun 16, 2026
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 1m 46s —— View job


Code Review: catalog vs read-only session launch model (#1481, #1483)

  • Read backend changes (core/mcp/remote/node/server.ts)
  • Read web layer (run-web.ts, web-server-config.ts)
  • Read frontend (useServerListWritable, App, ServerListScreen, ServerCard)
  • Review tests
  • Post review

Overall this is a well-structured, carefully-documented PR. The three-way server-list model (in-memory / read-only-file / writable-catalog) is cleanly factored, the comments explain the why (foreign-config safety, no-migration-on-read, 403-not-404 to avoid id leaks), the conflict matrix closes the #1483 "silent no-op" gap, and the test coverage is genuinely thorough (byte-stability, no-seed, 403-uniformity, in-memory serve, events no-broadcast). Nothing here blocks merge. Below are observations grouped by severity.

Correctness / design — minor

  1. Duplicate /api/config fetch. App.tsx now calls both useSandboxUrl and useServerListWritable, each issuing its own GET /api/config (same URL, same auth header) on mount. Harmless and the PR explicitly mirrors the existing hook shape, but two round-trips for one payload is avoidable. A future cleanup could expose a single useConfig() hook returning { sandboxUrl, writable }. Not worth blocking on. clients/web/src/App.tsx:541-552

  2. --config watcher still active for read-only files. For an in-memory ad-hoc list watchable is false (correct), but a read-only --config file is still watched (inMemoryServers is undefined → watchable true), so external edits to the foreign file broadcast change events and reload the UI. This is reasonable (read-only blocks UI writes, not external observation), but it's an implicit behavior worth a one-line comment near the watchable definition so a future reader doesn't assume read-only ⇒ no watch. core/mcp/remote/node/server.ts:333-339

  3. Redundant (defensive) header type-guard. In the ad-hoc branch, the entry builder re-checks config.type !== "stdio" && config.type !== undefined, but the earlier guard already process.exit(1)s when hasHeaders && (stdio | undefined). So at that point hasHeaders already implies http/sse. Harmless and arguably defensive, but the condition reads as if both states are reachable. clients/web/server/run-web.ts (entry construction).

Style / conventions — minor

  1. Native <code> in the read-only Alert. Per the repo's React conventions (prefer Mantine primitives over raw HTML), the inline <code>--config</code> / <code>--catalog</code> in ServerListScreen.tsx:142-146 would be better as Mantine's <Code> component, which also picks up theme styling. Small nit. clients/web/src/components/screens/ServerListScreen/ServerListScreen.tsx:140-147

Things verified as correct ✅

  • The 403 guard (requireWritable) is checked first in every mutating route — before body parse / existence checks — so a read-only backend never leaks whether an id exists (returns 403, not 404). Test covers this. core/mcp/remote/node/server.ts:1463
  • In-memory headers survive normalizeMcpServers (opts.header is a Record<string, string>, passes isStringRecord), so --header actually reaches the connect path.
  • Read-only GET path returns the file/in-memory list verbatim with no seed and no plaintext-secret migration (migration is a write) — byte-stability is explicitly tested.
  • useServerListWritable defaults writable=true on absent field / network error / pre-resolution, so legacy backends and the default catalog keep full CRUD — back-compatible as claimed.
  • All new component props default to writable=true, so existing callers/stories are unaffected.
  • Export correctly stays enabled in read-only sessions (it's a read, not a mutation).

Suggestion (optional)

The duplicate-fetch item (#1) is the only one I'd consider acting on, and only as a follow-up. Consolidate the two /api/config hooks →
· feat/1481-launcher-config-web-catalog

…larify 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>
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks for the review! Addressed the two worthwhile nits in 7e619ac:

  • SSE transport support #4 (raw <code>): switched the read-only banner to Mantine <Code> (repo convention).
  • Request logging #2 (watcher comment): added a comment near watchable clarifying that a read-only --config file is still watched on purpose — read-only blocks UI-driven writes, not external observation, so an external edit should still refresh the UI.

Deferred / left as-is (agree these aren't blockers):

  • Progress notifications #1 (duplicate /api/config fetch): left as the optional follow-up you suggested — consolidating useSandboxUrl + useServerListWritable into one useConfig() hook touches useSandboxUrl's consumers/tests, so it's cleaner as its own change.
  • Completion requests #3 (defensive header type-guard): kept intentionally defensive so a future logic change can't let headers land on a stdio entry.

@claude review

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 34s —— View job


Re-review: review fixes in 7e619ac4


Both fixes look good — clean, minimal, and exactly scoped to the feedback. Nothing further to address.

#4 — raw <code> → Mantine <Code>
ServerListScreen.tsx now imports Code from @mantine/core and uses it for both --config and --catalog in the read-only Alert. This is the right primitive (inline by default, theme-styled) and matches the repo convention of preferring Mantine components over raw HTML. Import is added to the existing destructured @mantine/core import rather than a new statement — tidy.

#2 — watcher comment
The added comment at core/mcp/remote/node/server.ts:333-339 accurately documents the intended behavior: watchable = !inMemoryServers, so a read-only --config file is still watched and external edits still refresh the UI. The distinction it draws (read-only blocks UI-driven writes, not external observation; writable does not gate watching) matches the code, and it removes the trap I flagged where a future reader might assume read-only ⇒ no watch.

Deferrals — agreed:

  • Progress notifications #1 (duplicate /api/config fetch): Reasonable to leave as a follow-up — consolidating into a useConfig() hook touches useSandboxUrl's consumers and tests, so it's cleaner as its own change.
  • Completion requests #3 (defensive header type-guard): Keeping it defensive is a fair call; it guards against a future refactor letting headers land on a stdio entry, at no real cost.

No other issues introduced by this commit. Both changes are documentation/styling only, with no behavioral impact, so the existing test coverage remains valid. 👍
· feat/1481-launcher-config-web-catalog

@cliffhall cliffhall requested review from BobDickinson and olaservo and removed request for BobDickinson and olaservo June 16, 2026 12:32
@cliffhall

cliffhall commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

Config launch

npx mcp-inspector --config ~/Desktop/mcp.json

config-mode

Catalog launch

npx mcp-inspector --catalog ~/Desktop/mcp.json

catalog-mode

Server URL launch

npx mcp-inspector --server-url http://127.0.0.1:3000/mcp
server-url-mode

Server URL launch with header

npx mcp-inspector --server-url http://127.0.0.1:3000/mcp --header this:that --header toodles:poodles
server-url-header-mode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

1 participant