Clients directory, docs update, launcher rework

[BobDickinson]

Clients directory

• Moved clients into /clients directory

Docs Update

• Reworked README.md in root and each project (root is overview, client has usage instructions for client)

Launcher rework

• Created new simple launcher that only determines which client to launch and calls its runner with all other argv
  • Mapped to root bin mcp-inspector
  • Not sure this provides any value other than backward compatibility (consider removing / just launch each client individually)
• Each client now configures itself (using shared config logic in core)
• All clients behave identically whether launched from launcher or directly
• No more CLI preprocessing of some config elements and spawning web app with partial config in env
• Web app now creates Vite and Hono servers via SDK instead of spawning runtimes and passing config in env
• Orderly shutdown implemented (easier with a single process)
• Bottom line: Single Node app regardless of how launched, no spawning, no env var stuffing

Notes

Current dir layout is:

inspector/
├── .github/              # CI workflows, issue/PR templates
├── .husky/               # Git hooks
├── clients/
│   ├── cli/              # CLI client
│   ├── launcher/         # mcp-inspector binary (--web, --cli, --tui)
│   ├── tui/              # Terminal UI client
│   └── web/              # Web UI client
├── configs/              # Sample MCP configs (mcp.json, mcpapps.json)
├── core/                 # Shared InspectorClient, state, auth, react
├── docs/                 # Architecture and config docs
├── scripts/              # Version check, update-version, etc.
├── test-servers/         # Composable MCP test servers and fixtures

Considerations:

I considered client (vs clients) - convention seems to be pluralize when multiple instances (similar to other directories in this project, and things like packages from the TypeScript SDK)

Launcher isn't exactly a client, but I'm not sure if we're going to end up keeping it, so seems fine to put it there now

I left core at the top level in part becasue I see uses cases for external projects (including some of my own) to use Inspector Core as way to embed inspector-like functionality (so it will be used by our own clients, but not just our own clients).

[BobDickinson]

@claude review

[claude]

Claude finished @BobDickinson's task in 3m 8s —— View job

---

PR Review: Clients directory, docs update, launcher rework

Todo List:

• Gather context and understand the PR
• Read key changed files
• Review launcher implementation
• Review client changes
• Review docs/README changes
• Review CI/build config changes
• Post comprehensive review feedback

---

Summary

This is a solid, well-scoped refactor. The key architectural win—eliminating the spawn-based multi-process approach in favor of a single-process model with in-process runWeb/runCli/runTui calls—is a genuine improvement. Config parsing is now unified in core/, and shutdown is orderly. The docs and README restructuring is clean and useful.

A few issues worth addressing below, ranging from medium to minor.

---

Issues

🔴 Fragile isMain detection in server.ts

clients/web/src/server.ts:142-146

const isMain =
  typeof process.argv[1] === "string" &&
  (process.argv[1].endsWith("server.js") ||
    process.argv[1].endsWith("server.cjs"));

This uses a suffix string match, which is fragile—any file ending in server.js would match incorrectly. The other index files (clients/web/src/index.ts, clients/cli/src/index.ts, clients/tui/index.ts) all use the correct pattern:

resolve(process.argv[1]) === resolve(__filename)

This should use that same canonical path comparison. Fix this →

---

🟡 Stale dependencies in root package.json and clients/cli/package.json

package.json:67-71

The root package.json still lists spawn-rx, shell-quote, ts-node, node-fetch (and @types/shell-quote in devDependencies) as dependencies. After the launcher rework eliminated process spawning, none of these appear to be imported anywhere in the source tree.

Similarly, clients/cli/package.json lists spawn-rx and express as runtime dependencies, but neither is imported in the CLI source (cli.ts, index.ts, error-handler.ts). The express dependency is actually used by test-servers/, not the CLI, and spawn-rx is not referenced anywhere in the CLI's source or tests.

These should be cleaned up to avoid bloating the installed package size and causing confusion for contributors. Fix this →

---

🟡 Double sandbox close in dev mode

clients/web/src/vite-hono-plugin.ts:36-46

The Vite plugin registers both an httpServer.on("close") handler AND overrides server.close() to shut down the sandbox:

server.httpServer.on("close", () => {
  sandboxController.close().catch(...)  // first close
});

const originalClose = server.close.bind(server);
server.close = async () => {
  await sandboxController.close();     // second close
  return originalClose();
};

When server.close() is called, it closes the sandbox and then the HTTP server, which fires the httpServer.close event, triggering another sandboxController.close(). The sandbox controller's close() method only guards against calling when server is already null (after the first close sets it null), but the second call races with cleanup. One of these two mechanisms should be removed—likely the httpServer.on("close") listener since server.close() is the explicit teardown path. Fix this →

---

🟡 MCP_INITIAL_ARGS splits on space, breaking paths with spaces

clients/web/src/web-server-config.ts:172

const args = process.env.MCP_INITIAL_ARGS?.split(" ") ?? [];

This simple space-split doesn't handle quoted arguments (e.g., a server arg like "--path=/my dir/server"). This is the standalone Docker/env-var entrypoint path, so it's a scenario that matters for container deployments. Either use a shell-quote library for proper parsing, or document that MCP_INITIAL_ARGS does not support arguments containing spaces.

---

🟢 Empty key not validated in CLI's local parseKeyValuePair

clients/cli/src/cli.ts:209-231

The local parseKeyValuePair (for JSON-typed tool/prompt/metadata args) doesn't reject empty keys:

const key = parts[0];  // could be "" for input like "=value"
// ...no check for !key

The shared version in core/mcp/node/config.ts:40 correctly checks !key. The local version should add if (!key || ...). Fix this →

---

🟢 URL transport auto-detection defaults to "sse" for unknown paths

core/mcp/node/config.ts:172-177

transportType = u.pathname.endsWith("/mcp")
  ? "streamable-http"
  : u.pathname.endsWith("/sse")
    ? "sse"
    : "sse";          // unknown path → sse (not streamable-http)

For URLs that don't match either /mcp or /sse, the fallback is "sse". Given that Streamable HTTP is the newer/recommended transport in the MCP spec and is preferred over SSE for new servers, this default seems backwards. Consider defaulting to "streamable-http" for unrecognized paths, or at minimum documenting the rationale.

---

🟢 resolveSandboxPort logic duplicated

The sandbox port resolution logic (check MCP_SANDBOX_PORT, fall back to SERVER_PORT, then 0) is implemented twice:

• clients/web/src/sandbox-controller.ts:29-41 — resolveSandboxPort() export
• clients/web/src/web-server-config.ts:233-245 — inline in buildWebServerConfigFromEnv()

web.ts:137 calls resolveSandboxPort() for the CLI path, but buildWebServerConfigFromEnv() duplicates the same logic inline for the standalone path. The standalone path should call resolveSandboxPort() to stay DRY.

---

Minor Notes

• clients/cli/package.json is missing a "type": "module" field—it relies on root workspace inheritance. The web and tui packages explicitly set it. Worth being consistent.
• clients/tui/tui.tsx:124-151: The parseCallbackUrl function contains unreachable return values after program.error() calls (which throw). TypeScript requires them for control flow. A minor style nit—these could be marked with // never reached, program.error() throws or use as never casts, but it's not a bug.
• Dockerfile is missing a test-servers workspace from the COPY package*.json stage—but since test-servers aren't needed at runtime, this is intentional and fine.

---

Overall Assessment

The architectural direction is correct and well-executed. The most important fixes are the isMain detection in server.ts and cleaning up the stale dependencies. The double sandbox close and the MCP_INITIAL_ARGS space-splitting are worth addressing before merge. Everything else is minor polish.
| Branch: launcher-and-docs

[BobDickinson]

Implemented all Claude review feedback, with the following exceptions:

• MCP_INITIAL_ARGS handling: All MCP_INITIAL_XXX were vestigial, removed.

• URL transport auto-detection defaults to "sse" for unknown paths: If transport isn't specified and can't be inferred from URL, we now error (instead of guessing at a default).

• clients/cli/package.json is missing a "type": This was hallucinated (the type was set properly to module)

[cliffhall]

Phew, that was a lot. Just a few notes.