feat(cli): session-oriented mcpi CLI with daemon (#1432)

[BobDickinson]

Closes #1432

Summary

Implements the session-oriented Inspector CLI from #1432, as documented in specification/v2_cli_v2.md:

• New mcpi entrypoint (bin.mcpi → clients/cli/build/mcp-bin.js) with an implicit local session daemon (IPC). Connect once, run many MCP subcommands against a named session, then disconnect — ssh-agent-style private binding via eval "$(mcpi private)".
• Frozen one-shot path unchanged: mcp-inspector --cli still connects → one --method → disconnect, never starts the daemon. Both paths share handlers/run-method.ts + InspectorClient.
• Session surface: catalog (servers/list|show), session lifecycle (connect / disconnect / sessions/*), auth store (auth/list, auth/clear), daemon control, and MCP methods including streams (logging/tail, resources/subscribe), tasks, and roots.
• Human text (default) and pretty JSON (--format json, no { result } envelope); TTY styling / OSC 8; connect-time OAuth with --relogin / --stored-auth-only.

Known gaps stay in the spec To-do (mid-session auth over IPC, Windows named pipes, daemon lock hardening, etc.) — not claimed as done here.

Review follow-ups (Claude on this PR)

Addressed in 5fbd3b9e:

1. Daemon idle leak (Medium) — Arm the idle timer at daemon start and after a failed connect that leaves the registry empty, so ensureDaemon from e.g. mcpi tools/list with no sessions still self-reaps (~60s). Covered by a session-less start test.
2. stringifyMeta mangling (Low) — Non-string metadata values (objects/arrays from key={"a":1}) are now JSON.stringify'd instead of String(v) → "[object Object]", in both session and one-shot paths.
3. --relogin on stdio (Low/UX) — Documented, not rejected: the flag means “ignore stored OAuth for this connect; interactive login only if auth is required.” For stdio / no URL-keyed store entry it is a deliberate no-op (nothing to clear). Help text, README, and v2_cli_v2.md updated accordingly.
4. Global bin name (Nit) — Renamed session bin from mcp → mcpi (MCP Inspector) in root + clients/cli package.json, docs, examples, and Commander .name(), to avoid claiming a generic PATH name.

Deferred (already in spec To-do / out of scope for this base PR):

• Completion requests #3 ensureDaemon unlink on failed ping vs auth mismatch — rare private-token edge case; leave for later hardening.
• add readme #6 Coverage gate for ipc-glue.ts / stream-client.ts — already tracked in the spec To-do.

Test plan

• npm run validate (web / cli / tui / launcher)
• clients/cli unit suite + per-file ≥90 coverage gate (test:coverage), with the exclusions listed in the spec
• Session smoke: mcpi connect <server> --config … → tools/list → tools/call … → disconnect → daemon stop
• OAuth: connect to an HTTP server that requires auth; TTY browser/OSC 8 UX and token reuse on reconnect
• mcpi connect … --relogin, auth/list, auth/clear <url>, auth/clear --all --yes
• eval "$(mcpi private)" isolates the daemon from the default socket
• One-shot mcp-inspector --cli … still works for the same catalog entry
• Re-validated after review fixes + mcpi rename

[BobDickinson]

Note: This is a first pass. There are noted open / future issues in the spec. We're trying to land a base PR to build from.

@claude review

[claude]

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

---

Review: session-oriented mcp CLI with daemon (#1727)

Read the spec and the full daemon + session surface. This is a well-structured first pass — I've kept the review to things not already listed in the spec To-do, since the known gaps (mid-session auth, singleton lock, Windows pipes, per-session mutex, etc.) are explicitly out of scope for this base PR.

What's good: clean split of one-shot vs. session sharing handlers/run-method.ts + InspectorClient; constant-time IPC token compare; 0600 socket/lock; config (incl. secrets) sent over IPC after listen rather than on argv; token via env not argv; NDJSON framing is simple and readable; strong test coverage with sensible, documented gate carve-outs.

---

1. (Medium) Daemon leaks when a spawn never establishes a session

SessionRegistry.armIdleTimer() is only ever called from disconnect() when the session count hits 0 (sessions.ts:226). It is not armed at daemon startup. But ensureDaemon() auto-spawns a detached, unref'd daemon from four call sites — connect, disconnect, sessions/use, and every rpc/stream (dispatch.ts:35). Any of these that spawns the daemon but never records a successful connect leaves a daemon with sessions.size === 0 and no armed idle timer, so it runs forever.

Concrete repro (nothing connected yet):

mcp tools/list        # → error "No open sessions", but a daemon is now running
mcp sessions/use foo  # → "session not found", daemon lingers
mcp disconnect        # → "No open sessions", daemon lingers

The user sees an error and has no idea a background daemon is now pinned to the event loop indefinitely (only daemon stop, or a later connect+disconnect cycle, reaps it). On a shared/CI box these accumulate. The spec's "idle exit ~60s after last disconnect" implicitly assumes a connect happened first.

Suggested fix: arm the idle timer once at DaemonServer.start() (and/or after any request completes with sessions.size === 0) so a session-less daemon self-reaps. Fix this →

---

2. (Low) stringifyMeta mangles non-string metadata values

session/mcp.ts:799 maps every metadata/tool-metadata value through String(v). A --metadata k={"a":1} (parsed to an object by parseKeyValue) becomes the literal "[object Object]", and arrays get comma-joined — the structure is lost before it reaches the server. MethodArgs.metadata is typed Record<string,string>, so if only strings are ever intended, consider validating/erroring on non-strings; otherwise JSON.stringify non-primitives (or thread JsonValue through end-to-end).

3. (Low) ensureDaemon unlinks a reachable-but-unresponsive socket

ensure.ts:111-122: if the socket is reachable but ping throws, the code fs.unlinkSync(socketPath) and spawns a replacement. If the ping failed because the running daemon required a token the caller doesn't have (rather than being truly stale), this yanks the socket path from under a live daemon, orphaning it. Edge case (private dirs are unique per mcp private, so token drift on the same dir is unlikely), but the unlink is destructive — worth distinguishing an auth failure from a stale socket before removing.

4. (Low/UX) --relogin is a silent no-op for stdio servers

mcp.ts:318 only clears stored auth when "url" in serverConfig. Functionally correct (stdio has no OAuth), but --relogin on a stdio entry is silently ignored rather than rejected — mildly surprising.

5. (Nit) Global bin name mcp

package.json:22 registers a top-level mcp bin. That's a very generic name to claim on a user's PATH for a global install and is prone to collision with other tooling. The README's npx --package=… mcp form sidesteps it, but a global npm i -g grabs mcp outright — worth a conscious call-out (or a note in docs about the collision risk).

6. (Nit) Concurrency-critical code sits outside the coverage gate

ipc-glue.ts and stream-client.ts are excluded from the ≥90 per-file gate (documented, and it's in the To-do). Just flagging that these two files hold the trickiest logic in the PR (accept/half-close/abort races), so the exclusion is where I'd most want the gate — good to prioritize the "stable race matrix" follow-up.

---

Nothing here is a blocker for landing a base to build from. #1 is the one I'd fix before merge (or add to the To-do explicitly), since it's an easy-to-hit lingering-process leak rather than a documented gap.

• Gather context (diff, spec, key files)
• Review daemon IPC layer (protocol, framing, server, sessions)
• Review session front-end (mcp.ts, dispatch, auth, formatting)
• Review shared handlers + one-shot path
• Review security considerations (socket perms, auth store)
• Post consolidated review
  · branch v2/cli-v2-sessions

[BobDickinson]

OK, Cursor updated the PR description with responses to review comments, which isn't ideal. But I think generally we're good now.

[cliffhall]

This seems like a toe-stubber (different command line for one client). Can the launcher not be the entry point and hand off to the mcpi?