Skip to content

feat(daemon-cli): mcpdo — experimental connection CLI client (#1432) - #1783

Open
BobDickinson wants to merge 62 commits into
v2/mainfrom
v2/mcpi-client
Open

BobDickinson wants to merge 62 commits into
v2/mainfrom
v2/mcpi-client

Conversation

@BobDickinson

@BobDickinson BobDickinson commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1432

Summary

Adds clients/daemon-cli, an experimental connection CLI published as the mcpdo bin: connect to an MCP server once, then run many commands against that named connection (ssh-agent style). Connections are held by an implicit local Unix-socket daemon that mcpdo starts on demand and talks to over token-authenticated NDJSON.

mcpdo connect test-stdio --config path/to/mcp.json
mcpdo tools/list
mcpdo tools/call echo message:=hi
mcpdo --conn other tools/list      # or --connection
mcpdo logging/tail                 # long-lived stream; Ctrl-C to stop
mcpdo connections/list && mcpdo daemon status
eval "$(mcpdo private)"            # optional per-shell private daemon
  • Daemon security model: per-daemon bearer token always required (generated at startup, published 0600 as daemon.token, or supplied via env for private mode), 0700 socket dirs under $TMPDIR/mcp-conn-<uid>/, socket-path length validated up front, O_EXCL pid lock with dead-pid reclaim (no takeover of a live daemon), 1 MiB NDJSON request-line cap, daemon stderr to a 0600 daemon.log.
  • Output safety: terminal-bound text (results, elicitation prompts, daemon errors) is control-character sanitized; OSC 8 URIs validated; --format json stays verbatim.
  • Stdio correctness: connect always sends an absolute cwd (defaults to the caller's), bare command names are resolved against the caller's PATH client-side, the default-inherited environment (PATH/HOME/SHELL…) is snapshotted from the caller's shell rather than the daemon's, and the daemon chdirs away from its spawn directory.
  • Auth: shared oauth.json with the other Inspector clients; connect-time OAuth on this CLI (--relogin, --stored-auth-only); elicitation bridging for form/URL prompts (non-interactive callers — --format json or no TTY — get the elicitation parked and answer it via elicitation/respond; URL mode never auto-accepts).
  • Era support: negotiates legacy/modern via core InspectorClient; --era legacy|auto|modern on connect.
  • Reuses clients/cli handlers / error-handler / OAuth helpers via a temporary build-time @inspector/cli alias — chore(mcpi): replace the temporary @inspector/cli source alias with a real shared surface #2461 tracks promoting that surface to a shared area.
  • Wired into monorepo validate / build / coverage / verify:bundle-externals; documented in AGENTS.md, clients/daemon-cli/README.md, and specification/v2_cli_v2.md. Adds a top-level skills/mcpdo end-user skill (teaches an agent to drive mcpdo), distinct from the .claude/skills/ repo procedures.

Packaging

Published by this PR (maintainer-approved): root bin.mcpdo → clients/daemon-cli/build/mcp-bin.js; files adds clients/daemon-cli/build and skills/mcpdo. Adds ~199 KB compressed (~770 KB unpacked, 16%) to the tarball. The daemon is inert unless mcpdo is invoked. The bin was renamed from mcpi to mcpdo to avoid the existing unrelated mcpi npm package.

Naming

Reviewer-visible rename since the last review: the client moved from clients/mcpi to clients/daemon-cli, the bin is mcpdo, and user-facing vocabulary moved from "session" to "connection" (connections/list|show|use, --connection/--conn) — modern MCP is session-less and the daemon-held thing's lifecycle is the connection's.

Test plan

  • npm run coverage:daemon-cli — 259 tests, per-file coverage gate ≥90 on all four dimensions (only the two true bootstraps src/mcp-bin.ts / src/daemon/run.ts excluded)
  • npm run verify:bundle-externals (daemon-cli enrolled, 4 bundles)
  • npm run local:gate from repo root — green on macOS
  • Manual: connect/tools/resources/logging-tail against test-servers over stdio + HTTP, OAuth + EMA connects, shared and mcpdo private daemons

@BobDickinson BobDickinson added the v2 Issues and PRs for v2 label Jul 25, 2026
Base automatically changed from v2/cli-improvements to v2/main July 26, 2026 20:55
@cliffhall cliffhall linked an issue Aug 17, 2026 that may be closed by this pull request
BobDickinson added a commit that referenced this pull request Sep 14, 2026
Adds a per-connection override for the elicitation capability mcpi
advertises to a server, mirroring the existing --era mechanism:

- InspectorServerSettings.elicitCapability ("off"|"url"|"form"|"both",
  default "both") persists on disk as elicitCapability, omitted when it
  equals the default, and round-trips through serverList.ts the same
  way protocolEra does.
- mcpi connect gains --elicit <mode>, validated the same way as --era,
  with a withElicitOverride() helper mirroring withEraOverride() (incl.
  synthesizing bare-defaults settings for ad-hoc targets).
- createSessionClient() now derives the InspectorClient elicit option
  from serverSettings.elicitCapability via elicitCapabilityToClientOption()
  instead of the old Phase-1 hardcoded { url: true, form: true }.

This lets a caller that cannot handle an interactive elicitation prompt
(a script, an agent) opt out entirely so the server sees no elicitation
capability and can fall back to its own alternative, instead of every
elicitation request being auto-declined.

Also updates clients/mcpi/README.md with an "Elicitation support"
section (previously undocumented, despite already-shipped URL/form
prompt rendering) and the --elicit flag, and refreshes the stale
"Sampling / elicitation CLI: Still TUI/web" to-do row in
specification/v2_cli_v2.md.

Manually verified end-to-end against the modern-mrtr-http test server:
--elicit off makes the server itself reject the mid-round input request
("capabilities do not declare the required capability"); --elicit both
(default) succeeds and reaches the interactive/auto-decline prompt path
as before.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author

mcpi work summary — PR #1783 (2026-09-13 → 2026-09-14)

Branch: v2/mcpi-client in /Users/bob/Documents/GitHub/inspector-trees/v2-mcpi-client
Repo: modelcontextprotocol/inspector
PR: #1783 — all pushed; CI (build, coverage) green as of 7fd4af59.

Organized by what actually changed functionally, not commit order.


1. Modern protocol-era support (era + skills primitives)

Gave mcpi first-class awareness of MCP's protocol eras (legacy vs. modern/
task-capable) and filled in missing skills primitives:

  • --era override on connect (f2fc1a2c): force which protocol era an
    ad-hoc session negotiates as, instead of only auto-detecting.
  • sessions/show replaces initialize (9550b32c): the session-info RPC
    now reports era details directly (protocol version, task support, etc.)
    instead of the old bare initialize response.
  • protocolEra surfaced everywhere (380dd3e7): every session listing
    (sessions/list, not just sessions/show) now reports era at a glance.
  • tasks/update (22d5c9f4): implemented to resume paused "modern"
    (task-capable) MCP tasks, with success/error-path tests.
  • skills/list and skills/get (40b4f441): implemented the RPCs
    (previously stubbed/missing), supporting positional and --uri argument
    forms, plus a --verify flag on skills/list.
  • Docs (2a61f427): documented --era, sessions/show, and
    tasks/update end-to-end.

2. Elicitation features (legacy URL-mode and modern/MRTR form-mode)

Built out MCP's elicitation flow, covering both eras' mechanisms:

  • URL-mode (legacy elicitation) (ac7e4bf1): when a server elicits via a
    URL, mcpi prompts to confirm/open it and waits for completion.
  • Form-mode (modern/MRTR structured elicitation) (258d789f): when a
    server elicits structured form data (JSON-schema-driven, per the newer
    request-response/MRTR-style pattern), mcpi walks the user through each
    field interactively with a review step before submitting.
  • --elicit capability override (98a41510): lets a caller declare
    elicitation support explicitly, for ad-hoc/non-standard clients.

3. Making mcpi agent-friendly

Everything else — reframing and hardening mcpi so an AI agent driving it
non-interactively gets the same guarantees a human at a terminal gets:

  • Packaging (29193232): bundled mcpi into the published
    @modelcontextprotocol/inspector npm package so it actually ships.
  • mcpi agent-help + skills/mcpi/SKILL.md (9ecd2647): a discoverable,
    self-contained reference for agents on how to drive mcpi non-interactively.
  • OAuth without a TTY (0096e2c7): OAuth's URL-prompt-and-wait flow no
    longer requires an interactive terminal; message reframed for an
    agent-attended flow ("The user needs to navigate to this link to
    authenticate: <url>"). Added clean SIGINT/SIGTERM cancellation so a user
    (or agent) can break out of the ~15-minute OAuth wait if they decide not to
    auth or auth fails, instead of it being a hard, uninterruptible block. Also
    addressed the daemon idle-timeout interacting with long OAuth waits.
    Live-tested with a real, non-TTY OAuth flow.
  • Non-TTY elicitation (7cf45384): removed the TTY gate on elicitation
    entirely — both URL-mode and form-mode now work non-interactively, since
    the underlying readline-based prompting was never actually TTY-dependent,
    just gated by policy. Closed the one real risk this exposed (stdin EOF/close
    could hang readline.question() forever) by racing every prompt against a
    "stdin closed" signal. Live end-to-end tested against a real MCP test
    server, including a piped-EOF instant-decline case and a live-FIFO
    simulated-agent-relayed-answer case.
  • Final non-TTY audit + SIGINT cleanup (7fd4af59): audited all
    remaining isTTY gates; confirmed auth/clear --all and
    requireExplicitSession()'s explicit-session requirement are intentional
    (see MRU note below), fixed a stale doc comment, and extended clean
    SIGINT/SIGTERM cancellation from the two streaming RPCs to the general
    rpc path so Ctrl-C during any blocking call (e.g. tools/call, an
    elicitation wait) cancels cleanly instead of killing the process.

Key design note (MRU): the daemon is a single shared process, so MRU
("most recently used" session) state is global, not per-terminal.
requireExplicitSession() gates on stdin, not stdout, so a human piping
output (mcpi tools/list | jq) still gets MRU convenience; a truly
non-interactive caller (agent/script/CI) must pass --session/@name
explicitly, since there's no live human to catch a wrong guess.
MCP_ALLOW_DEFAULT_SESSION=1 opts back into MRU for scripts that want it.


Non-functional maintenance (excluded from the above as "not changes")

These kept the branch buildable/green but didn't change behavior:

  • e79dea7f, fd64afff — restored build:dev tooling/build config after a
    v2/main merge broke it.
  • 54d00b91 — brought mcpi's validate scripts into parity with the rest of
    the repo's guards.
  • aabc19fa, 12353bea — closed CI coverage/build gaps (including one
    caused by the agent-help commit itself shipping without tests) — pure
    test-coverage backfill, no functional change.

@cliffhall cliffhall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mergeability verdict: ❌ Not mergeable — changes requested

CI is green and the feature works end to end. I connected, listed and called tools, and checked sessions and daemon status against a stdio test server. The test suite is large and mostly good. Four things block the merge:

  1. Private-mode daemons can be taken over, and live sessions orphaned (security, reproduced).
  2. Server-controlled terminal escape sequences reach the user's terminal raw (security, reproduced).
  3. Stdio connect resolves relative commands against the daemon's cwd, not the caller's, so it can run a different file than the one the user named (reproduced).
  4. Repo-rule violations: dependency placement, coverage-gate exclusions, no DCO signoff on any of the 30 commits, and a test that fails on stock macOS so local:gate cannot go green on a Mac.

There is also a scope question maintainers need to decide explicitly, not by drift: this PR now publishes a global mcpi bin and a background daemon to every installer of @modelcontextprotocol/inspector, while the PR description still says it does not.

Everything below was checked in a clean worktree of 22c97b09 (npm install && npm run build, then validate:guards, coverage:mcpi and verify:bundle-externals) on macOS 15, with an isolated MCP_STORAGE_DIR / MCP_INSPECTOR_DAEMON_DIR.


1. Security

Most of the risk comes from what mcpi adds on top of the one-shot CLI: a detached, long-lived process that accepts connect requests carrying an arbitrary serverConfig (including a stdio command) over a Unix socket, and then spawns that command. The one-shot CLI's exposure ends when the process exits. The daemon's does not: it stays up for as long as any session is open, because the idle timer only arms at zero sessions.

The stated trust model is same-UID filesystem trust (shared mode), plus an IPC token in private mode. The findings below are measured against that model.

1a. 🔴 A wrong or missing token replaces a live private daemon (blocker)

ensureDaemon() (clients/mcpi/src/daemon/ensure.ts) treats "socket reachable but ping failed" as "stale socket". It unlinks the socket and spawns a new daemon. ping also fails on daemon_auth_failed, so any caller holding the wrong token, or no token, deletes the live daemon's socket and installs its own daemon in its place.

Reproduced:

# user starts a private daemon
MCP_INSPECTOR_DAEMON_TOKEN=goodtoken mcpi connect --session a node server.js   → daemon 90433

# any same-UID process without the token
mcpi connect --session evil node server.js                                      → spawns daemon 90441 (NO token)

# the legitimate user, still presenting goodtoken
MCP_INSPECTOR_DAEMON_TOKEN=goodtoken mcpi sessions/list
Sessions (1):
* `@evil` (MRU) — node server.js [legacy]

Three consequences:

  • Private mode's guarantee is void. The user's token-bearing client is silently served by an unauthenticated daemon that someone else started. The replacement's @evil session is now the user's MRU default, so the user's next bare mcpi tools/call … goes to a server the other party chose. The token exists to separate same-UID callers; this is the one thing it currently fails to do.
  • Orphaned processes. The original daemon, 90433 above, and its stdio server keep running with no reachable socket. They never exit, because idle reaping is only armed at zero sessions. The same happens with a wrong token (MCP_INSPECTOR_DAEMON_TOKEN=wrong also produced a second daemon). A typo therefore leaks processes.
  • Lock-out. With a wrong token, the legitimate user then gets daemon_auth_failed against the replacement.

Fix: on daemon_auth_failed (or any structured error reply), ensureDaemon must fail loudly and leave the socket alone. Only a socket that refuses connections (ECONNREFUSED / ENOENT) is stale. daemon.lock is written but never used as a lock. Make it one: store pid plus start time, use an O_EXCL create or proper-lockfile, which is already a root dependency, and check liveness with process.kill(pid, 0) before unlinking anything.

1b. 🔴 Terminal escape injection from server-controlled text (blocker)

The human formatter (clients/mcpi/src/session/format-human.ts, the default --format text) writes server-supplied strings straight to the terminal: tool results, descriptions, resource text, elicitation messages, and URIs embedded in OSC 8 hyperlinks (clients/cli/src/style.ts). I found no control-character stripping anywhere under clients/mcpi/src.

Reproduced with the echo test tool (od -c of stdout):

E c h o :   h i 033 ] 5 2 ; c ; c H d u Z W Q = \a 033 ] 0 ; S P O O F E D - T I T L E \a

That is an OSC 52 clipboard write and an OSC 0 title change, both delivered intact from the server. Depending on the terminal, the same channel allows clipboard poisoning (the next paste into a shell), hiding or overwriting earlier output (CSI cursor moves and erases), and spoofing link targets. A URI containing \a also breaks out of the OSC 8 wrapper. The one-shot CLI emits JSON, where these bytes are escaped, so this is new exposure introduced by this PR. It matters more because the skill in this PR targets agents reading the output.

Fix: sanitize every server-derived string before styling it. Replace C0/C1 controls other than \n and \t, plus DEL, with visible escapes such as \x1b → ␛ or \u001b. Validate or percent-encode URIs before putting them in OSC 8. --format json is already safe.

1c. 🟠 Stdio commands resolve against the daemon's cwd, not the caller's

The daemon is spawned without cwd, so it inherits the directory of whichever mcpi invocation first started it. connect does not default --cwd to the caller's process.cwd() (clients/mcpi/src/session/mcp.ts, serverOptions.cwd). A relative stdio target is therefore resolved in someone else's directory:

(daemon started from the repo root; lsof cwd → /…/mcp-inspector-pr1783)
cd test-servers/build && mcpi connect --session rel node ./test-server-stdio.js
{"error":{"code":"error","message":"Connection closed"}}

Here it failed with an opaque error, but only because the file did not exist in the daemon's cwd. If a file with the same name exists there, mcpi silently runs that one. mcpi connect node ./server.js in project B would execute project A's ./server.js. That is a correctness bug with a real security edge. PATH has the same staleness problem: every later session inherits the first shell's PATH, whether that came from nvm, a venv or anything else.

Fix: the front end should always send an absolute cwd, defaulting to process.cwd(), and should resolve relative command paths before sending. It should also consider forwarding the caller's PATH. The daemon should chdir to its own directory, or to /, at startup so it never pins an arbitrary working directory.

1d. 🟠 The daemon dies silently, and the socket-path limit is unchecked

ensure.ts spawns the daemon with stdio: "ignore", so every startup failure is invisible. The client waits 10s and reports a generic daemon_start_timeout. I hit this at once: my scratch MCP_INSPECTOR_DAEMON_DIR produced a 140-byte socket path, and listen() fails above macOS's 104-byte sun_path limit (108 on Linux). The daemon exited, left a stale daemon.lock behind (mode 0644, because the chmod never ran), and the user saw only a timeout.

The private-mode layout, $HOME/.mcp-inspector/private/<uuid>/daemon.sock, uses 72 fixed bytes, leaving ~32 bytes for $HOME on macOS. This is also why a test fails locally (see §2c).

Fix: validate the socket path length up front with a clear error, and shorten the private layout, e.g. a short id, or $TMPDIR/mcpi-<uid>/<short> with a 0700 dir. Send the daemon's stderr to daemon.log in the daemon dir with mode 0600, and have daemon_start_timeout include its tail.

1e. 🟡 Hardening (not blocking on their own)

  • Shared-mode directory permissions. ensureDaemonDir() creates ~/.mcp-inspector with the default umask (0755 here). The socket is chmod 0600 only after listen(), which leaves a short window. Create the dir 0700 and bind inside it. Then the socket's own mode never matters, and BSD's inconsistent enforcement of socket permissions stops mattering too.
  • An exec service outside agent sandboxes. This deserves a paragraph because the PR ships a skill aimed at agents. In shared mode, any same-UID process that can connect() to ~/.mcp-inspector/daemon.sock can have the daemon spawn any command. Same-UID is nominally the same privilege, but agent sandboxes (Claude Code's sandbox, Codex, containers that bind-mount $HOME, Flatpak) often restrict exec and filesystem access and not Unix-socket connects. A daemon started outside a sandbox, by the human, becomes an unsandboxed exec endpoint for any agent inside one. My suggestion: always require a token, including in shared mode, stored in a 0600 file in the 0700 daemon dir. That gives no extra protection against a plain same-UID process, which can read the file anyway. It does give sandbox policies a file-read denial to rely on, which is the control they actually have. It also retires the tokenless path that 1a exploits.
  • No request-size limit on the NDJSON reader (readline over the socket). A single unbounded line grows daemon memory without limit. Cap the line length.
  • Non-TTY elicitation answered by an agent (7cf4538). This is a real product decision, not a bug. Form-mode elicitation is meant to put a question to the user, and this makes it routine for an agent to answer on the user's behalf. That may be fine for an inspector, but please record it as a decision (spec doc plus README), and keep URL-mode elicitation requiring an explicit human action. skills/mcpi/SKILL.md also still says "running non-interactively (no TTY, scripted, or --format json) auto-declines", which that commit made false. Only --format json declines now.
  • core/auth/node/runner-interactive-oauth.ts now installs process-wide SIGINT/SIGTERM listeners for the length of the OAuth wait. They are correctly removed in finally, but this is shared core/ and also runs under the TUI, which owns Ctrl-C through Ink. Please confirm that TUI Ctrl-C during an OAuth wait still behaves as intended, or scope the handler to callers that opt in.

Good things worth keeping: timingSafeEqual token comparison, 0600 socket and lock, 0700 private dirs, randomBytes(32) tokens, the POSIX-safe single-quoting in mcpi private, idle self-reaping, and stdio children exiting when their stdin closes. I SIGKILLed the daemon, and its test server exited with no orphans.


2. Repo-rule compliance (AGENTS.md)

2a. 🔴 Dependency placement

  • clients/mcpi/package.json re-declares root-owned runtime dependencies: @modelcontextprotocol/{client,core,server,server-legacy}, ajv, atomically, @napi-rs/keyring, pino, undici, zod, commander and open. This breaks "a client declares only what that client alone consumes … clients/cli and clients/launcher therefore declare no runtime dependencies". It re-creates exactly the second copy that #1896 exists to prevent (the 3,387-line clients/mcpi/package-lock.json). server/server-legacy are not runtime dependencies of a client at all.
  • The re-declaration is also load-bearing, which is why this matters beyond neatness. tsup auto-externalizes what the client's manifest declares, and mcpi's external list omits undici, zod, ajv and atomically. Deleting the manifest entries today would inline undici and reproduce #2067 (Dynamic require of "assert" is not supported). Fix: delete the client runtime deps and name every root runtime dependency that core/ reaches in clients/mcpi/tsup.config.ts external, mirroring clients/cli/tsup.config.ts and its comments.
  • AGENTS.md still says "must also be named in all three bundler external lists (clients/{cli,tui}/tsup.config.ts, clients/web/tsup.runner.config.ts)". With a fourth bundler this rule changes, so update AGENTS.md in the same change, per the maintenance rule.

2b. 🔴 Coverage gate: whole files waved out

clients/mcpi/vitest.config.ts excludes src/daemon/ipc-glue.ts and src/daemon/stream-client.ts from the ≥90 per-file gate as "hard-to-stabilize accept/stream races". The rule is explicit: "A genuinely-unreachable branch is annotated at the source, never waved through by lowering the gate", and a race is fixed with an awaited condition, never with headroom or exclusion (#1596). ipc-glue.ts is the socket accept loop and the elicitation line-consumer, the most security-relevant code in the client. It is the file that most needs gating. Only the true bootstraps (mcp-bin.ts, daemon/run.ts) qualify for exclusion, as with clients/cli's src/index.ts. The AGENTS.md edit that documents these exclusions should be dropped along with them.

2c. 🔴 A test fails locally, so local:gate cannot pass on macOS

coverage:mcpi → daemon-private.test.ts > ensureDaemon spawns a token-gated daemon from env fails on stock macOS: 1 failed | 220 passed, with Timed out waiting for session daemon. The test sets HOME to os.tmpdir()/…, which on macOS is /var/folders/…/T/. The resulting socket path is 142 bytes (§1d). CI passes only because Linux's /tmp is short. local:gate is the mandatory pre-push command, and the PR's own test plan still has npm run ci unchecked. Fixing §1d fixes this too.

2d. 🔴 DCO

None of the 30 commits carries Signed-off-by (git log --format='%(trailers:key=Signed-off-by)' is empty for every one). AGENTS.md: "sign off every commit (git commit -s — the DCO check is a hard merge gate with no partial credit)". A rebase with --signoff fixes it. It is probably best done together with the rebase in §2f.

2e. 🟠 Docs and structure

  • The PR description is stale and contradicts the code. It says "Not in the published tarball (files allowlist unchanged)" and "No root bin.mcpi". 2919323 adds "mcpi": "./clients/mcpi/build/mcp-bin.js" to root bin, and adds clients/mcpi/build and skills/mcpi to files. It also still says "Depends on #1782" (merged) and "Retarget … after #1782 merges". Please rewrite it; reviewers and the release notes will read it.
  • The new top-level skills/ directory is missing from the AGENTS.md / README Project Structure trees. Its relationship to .claude/skills/ needs one line (end-user skill shipped in the tarball vs. repo procedures). Otherwise the next agent will try to run verify:skills rules against it, or move it.
  • Branch name v2/mcpi-client lacks the type/issue segment (v2/feat/1432-mcpi-client). This is minor and not worth a new branch now.

2f. 🟠 Freshness and size

The branch is 124 commits behind v2/main. That includes #2374's Skills registry and -32021 changes and the SDK-v2 client-extension work, which touch the same skills/list / skills/get / era surfaces mcpi wraps. mergeStateStatus says CLEAN, but green CI on a stale base proves little for this surface. Please rebase with --signoff and re-run local:gate. At 16.8k added lines, with two merge commits and features accreted over two months (elicitation, EMA, tasks/update, era, packaging), this is hard to review as a whole. At minimum, the packaging change (2919323) should be split into its own PR so it gets its own decision (see §3).

2g. 🟡 Architecture: the @inspector/cli reach-in

The build-time alias from clients/mcpi into clients/cli/src (handlers, error-handler, OAuth navigation) makes one client's private source another client's API. clients/cli refactors can now break mcpi with no signal in the cli's own gate. fd64aff and aabc19f are both exactly this kind of breakage. The code comments say "temporary"; please file the tracking issue now (move handlers/, error-handler, and cli-oauth-navigation into core/, or a shared Node-runner area) and link it from the tsup comment. Temporary without an issue tends to become permanent.


3. Should it ship in the published package? Should it be containerized?

Shipping. Publishing adds a second global bin and a long-lived background daemon to every npm i -g @modelcontextprotocol/inspector install, under a name maintainers haven't signed off on. (mcpi is also an existing, unrelated npm package, a Minecraft-Pi API, which is harmless for a bin but will confuse search and npx mcpi.) The issue and spec still call this experimental. I'd keep it out of the tarball until the security items above are fixed and a maintainer signs off on the bin name, then publish it in a dedicated PR. That was the original plan in this PR's description, and I think it was right.

Containerizing the daemon: should not, and mostly could not usefully. The daemon's whole job needs host resources: the OS keychain (@napi-rs/keyring), the shared oauth.json store, the user's browser for OAuth, a loopback OAuth callback port, and above all local stdio servers that exist to touch the user's files and tools. Putting the daemon in a container breaks keyring and OAuth, turns every stdio server into a mount-and-PATH configuration problem, and on macOS and Windows adds a Linux VM dependency (Docker Desktop, Podman). It also secures the wrong thing. The daemon itself is small, trusted first-party code. The risky parts are (a) the socket as an exec endpoint, which §1a and §1e fix in code, and (b) the MCP servers it runs, which are untrusted third-party code. That is the same risk every MCP host takes, and containers are the right tool for it.

What I'd recommend instead:

  1. Fix the socket boundary in code (§1a, §1e). That is the risk the daemon adds.
  2. Make server isolation opt-in, per session. Document the recipe that already works today with no code: mcpi connect docker run -i --rm --network none -v "$PWD:/work:ro" <image>. Then consider a first-class --sandbox on connect that wraps the stdio command: docker/podman run -i everywhere, with lighter native options later (bwrap on Linux, sandbox-exec profiles on macOS). That puts isolation where the untrusted code is, lets users choose it per server, and costs nothing when unused.
  3. Treat HTTP/SSE targets as needing no process isolation. Their risk is the terminal-output and elicitation surface (§1b, §1e), which sanitization covers.

Summary of requested changes

# Change Severity
1a ensureDaemon: never unlink or replace on auth failure; turn daemon.lock into a real pid lock 🔴 security
1b Sanitize control characters in all server-derived text output; validate OSC 8 URIs 🔴 security
1c Send an absolute cwd (default process.cwd()) with stdio connects; chdir the daemon away 🟠 security/correctness
1d Socket-path length check, shorter private layout, daemon stderr to a 0600 log 🟠
1e 0700 daemon dir; token always required (file-backed); cap NDJSON line length; fix SKILL.md elicitation wording; confirm TUI Ctrl-C 🟡
2a Drop client runtime deps; complete the external list; update AGENTS.md's "three lists" rule 🔴 rules
2b Gate ipc-glue.ts / stream-client.ts (fix races, v8 ignore only truly unreachable lines) 🔴 rules
2c Make daemon-private.test.ts pass on macOS (falls out of 1d) 🔴 rules
2d --signoff every commit 🔴 rules
2e/2f Rebase on v2/main, rewrite the PR description, document skills/, split out packaging 🟠
2g File the tracking issue for the @inspector/cli reach-in 🟡

Happy to re-review once the 🔴 items are in. The session model itself works well and I'd like to see it land.

BobDickinson and others added 2 commits September 22, 2026 22:25
Small, mcpi-motivated additions to shared code, kept separate so the
client itself is reviewable on its own:

- clients/cli handlers: expose method metadata (method-types) and a
  reusable run-method entry point for out-of-process callers; unit
  tests for the mocked run-method paths
- clients/cli/src/cli-oauth-navigation.ts: allow callers to supply
  their own browser-open/navigation hooks
- core/auth/node/runner-interactive-oauth.ts: SIGINT/SIGTERM-aware
  wait so Ctrl-C during an interactive OAuth flow cleans up the
  callback server (removed in finally); test in clients/web test tree
- core/mcp/serverList.ts, core/mcp/types.ts: server-list helpers and
  types shared by cli and mcpi

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Add clients/mcpi, an experimental session-oriented CLI: connect once,
then run many MCP commands against a named session held open by an
implicit local Unix-socket daemon (ssh-agent style). Not part of the
published package; runs from a repo checkout (npm link).

Highlights:

- Session daemon (auto-spawned, idle self-reaping) with NDJSON IPC,
  token-gated private mode (`mcpi private`), MRU session selection
- Full command surface via shared clients/cli handlers: tools,
  resources, prompts, skills, tasks, completions, logging, sampling,
  elicitation (interactive form prompts and agent-answerable modes)
- OAuth support including stored-token reuse, interactive browser
  flows, and enterprise-managed auth (EMA): --ema connect flag,
  auth/ema-status|login|logout, per-session Auth reporting with
  disk-truth reads in sessions/show
- Era detection/reporting (legacy vs 2025-11-25) per session
- Human and JSON output formats; agent-focused skills/mcpi/SKILL.md
- Spec: specification/v2_cli_v2.md; docs in clients/mcpi/README.md
- Tests: 221 unit/integration tests, per-file coverage gates wired
  into the repo quality gate (coverage:mcpi, validate:mcpi)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
BobDickinson and others added 7 commits September 23, 2026 11:03
1a — no daemon takeover: a socket that accepts connections is owned by a
live daemon; any ping failure (auth, timeout, protocol) now fails loudly
instead of unlinking the socket and respawning over it. daemon.lock is a
real O_EXCL pid lock with dead-pid reclaim, closing the probe/unlink/bind
race between two starting daemons.

1b — terminal escape sanitization: every server-controlled string is
sanitized before reaching the terminal in text mode (new
session/sanitize.ts: C0/C1 controls except \n\t become visible
stand-ins). Wired into the human formatter, the ndjson stderr summary,
elicitation prompts (message/url/schema — never protocol ids), and
daemon-client error messages. --format json stays verbatim (JSON already
escapes controls).

1c — stdio cwd correctness: --cwd is resolved to an absolute path at the
caller; stdio connects with no cwd default to the client's cwd
(catalog/--cwd still win); the daemon chdirs to its own dir on startup so
its inherited cwd is inert.

1d — no silent daemon death: socket paths are validated against sun_path
limits up front with an actionable error; private daemon dirs moved to
the short $TMPDIR/mcpi-<uid>/<id>/ layout (0700, fits the macOS limit);
daemon stderr goes to a 0600 daemon.log whose tail is quoted in
start-timeout errors.

1e — hardening: daemon dir created 0700; a token is now always required —
generated when the environment doesn't supply one and published to a
0600 daemon.token beside the socket for clients to read, retiring the
unauthenticated request path; NDJSON request lines are capped at 1 MiB;
SKILL.md/README/spec updated to record the elicitation decision (only
--format json auto-declines; URL mode never auto-accepts); the OAuth
runner's process-wide SIGINT/SIGTERM handlers are now opt-in
(handleSignals) so the TUI keeps Ctrl-C ownership under Ink, with CLI and
mcpi opting in.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
clients/mcpi declared root-owned runtime dependencies, re-creating the
second copy the dependency-placement rule (#1896) exists to prevent, and
the re-declaration was load-bearing: tsup auto-externalized from the
client manifest, so the external list was incomplete.

- clients/mcpi/package.json now declares no runtime dependencies (same
  steady state as clients/cli and clients/launcher); the 3,387-line
  lockfile shrinks to devDeps only.
- clients/mcpi/tsup.config.ts names every root runtime dependency that
  core/ (or the bundled one-shot CLI source) reaches, mirroring
  clients/cli/tsup.config.ts; verify:bundle-externals passes against the
  built output.
- A scoped override pins sucrase's nested commander to ^13: with no
  top-level commander declared, npm otherwise hoists sucrase's
  commander@4 into clients/mcpi/node_modules where it shadows the root
  commander@13 on the walk-up (helpCommand crash at startup).
- AGENTS.md's "three lists" rule is now four (clients/{cli,mcpi,tui}
  tsup configs + web's runner config); the no-runtime-deps steady state
  names mcpi; sdk-watch's checklist string updated to match.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…review 2b)

Remove the "hard-to-stabilize accept/stream races" coverage exclusions
for src/daemon/ipc-glue.ts and src/daemon/stream-client.ts; only true
bootstraps (src/mcp-bin.ts, src/daemon/run.ts) stay outside the gate.

New __tests__/daemon-ipc-glue.test.ts exercises the per-connection
wiring deterministically with an in-memory Duplex (no accept races):
the elicitation channel round trip, non-answer lines, double-pending
rejection, disconnect/destroyed-socket rejection, the mid-handle
destroyed guard, and single-shot stream cleanup on socket error.
daemon-stream.test.ts gains default socket-path/timeout + explicit
token coverage and a post-end frame-ignore case.

Writing those tests surfaced a real bug: readline re-emits socket
errors on the interface, so a client RST would have crashed the daemon
with an unhandled 'error' event. acceptDaemonConnection now attaches an
rl error listener; the socket error handler keeps owning teardown.

Both files clear >=90 on all four dimensions (ipc-glue 99/95/95/100,
stream-client 96/92/93/97); mcpi suite 250/250.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…es (review 2e, 2g)

AGENTS.md and README gain the skills/ entry in the project tree
(distinct from .claude/skills/); the temporary @inspector/cli alias
notes in AGENTS.md, clients/mcpi/README.md and tsup.config.ts now link
the tracking issue #2461.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dio servers (review §3)

The daemon token gates who can command the daemon, not what a spawned
server can do. Record the zero-code recipe — wrapping the stdio command
in `docker run -i` — as the way to isolate an untrusted server, per the
review recommendation.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… (review 1c follow-up)

The daemon inherits the environment of whichever mcpi invocation first
spawned it, so a bare command name like `node` was looked up in that
stale PATH — a different nvm version or venv could supply a different
binary than the caller's shell would. The connect front end now
resolves bare names (no path separator) to an absolute path using the
caller's PATH before the config crosses the IPC boundary, so the
daemon spawns exactly the caller's binary and no environment is
forwarded. Unresolvable names pass through unchanged so the daemon's
spawn error stays the user-visible failure; commands with a separator
still resolve against the pinned session cwd.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…bundle into the package

Maintainer-approved decisions on the #1783 review thread:

- Bin name: `mcpdo` (conflict-free on npm; `mcpi` collides with an
  unrelated package). Root `bin` now installs it and `files` ships
  `clients/daemon-cli/build` and `skills/mcpdo`, so
  `npm i -g @modelcontextprotocol/inspector` provides the experimental
  client (~200 KB compressed addition).
- Internal name: `clients/daemon-cli` (role-based, like cli/tui/web/
  launcher), insulated from future bin renames. Root scripts are now
  build:/validate:/coverage:daemon-cli.
- Vocabulary: the daemon holds named live connections, not resumable
  sessions, so the session wording over-promised and collided with MCP
  transport terminology. Commands are now `connections/list|show|use`;
  `connect`/`disconnect` stay top-level lifecycle verbs. The global flag
  is `--connection <name>` with `--conn` as a documented shorthand
  (argv-level alias, one option registration). Env opt-in renamed to
  MCP_ALLOW_DEFAULT_CONNECTION; daemon dirs move to
  $TMPDIR/mcp-conn-<uid>/. IdP *session* wording is kept where it names
  the enterprise IdP login session (a different concept).
- Shared cli helpers consumed only by mcpdo follow suit
  (annotateServerEntriesWithConnections, CONNECTION_RPC_METHODS, and the
  servers/list `connection` annotation field).

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson BobDickinson changed the title feat(mcpi): experimental session CLI client (#1432) feat(daemon-cli): mcpdo — experimental connection CLI client (#1432) Sep 23, 2026
@BobDickinson

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — reproducing 1a–1c made these easy to fix with confidence. Everything below is on the branch; local:gate is green on macOS (the §2c failure is gone). Two headline changes since your review, both maintainer-approved: the client is renamed (mcpi → mcpdo, clients/mcpi → clients/daemon-cli, "session" → "connection" vocabulary), and packaging is folded back into this PR (details under §3).

§1 Security

  • 1a — fixed. ensureDaemon now fails loudly on any structured error reply (including daemon_auth_failed) and never unlinks the socket; only ECONNREFUSED/ENOENT is treated as stale. daemon.lock is a real O_EXCL pid+starttime lock with a kill(pid, 0) liveness check before any reclaim. Your repro sequence now errors instead of replacing the daemon.
  • 1b — fixed. All server-derived terminal-bound text goes through a sanitizer (C0/C1 + DEL → visible escapes, \n/\t preserved); OSC 8 URIs are validated before wrapping. Covered by tests including your OSC 52/OSC 0 payloads. --format json unchanged.
  • 1c — fixed. connect always sends an absolute cwd, defaulting to the caller's process.cwd(); the daemon chdirs to its own directory at startup. For PATH we went a step further than forwarding: bare command names are resolved client-side against the caller's PATH (which-style) and sent absolute, so no environment crosses the socket at all.
  • 1d — fixed. Socket path length is validated up front against the platform sun_path limit with a clear error; the layout is shortened to $TMPDIR/mcp-conn-<uid>/<id>/ (0700); daemon stderr goes to a 0600 daemon.log and start-timeout errors include its tail.
  • 1e — all taken. Daemon dir created 0700 before bind; token always required in every mode (0600 daemon.token in the 0700 dir — adopted your reasoning: it gives sandbox policies a file-read denial to enforce and retires the tokenless path from 1a); 1 MiB NDJSON request-line cap; SKILL.md elicitation wording corrected and the non-TTY-agent-may-answer decision is recorded in specification/v2_cli_v2.md (URL mode still requires an explicit answer); the OAuth SIGINT/SIGTERM handlers were a regression introduced by this PR's own first commit — they're now opt-in (handleSignals), so TUI Ctrl-C behavior is unchanged.

§2 Repo rules

  • 2a — fixed. Client runtime deps removed; every root runtime dependency core/ reaches is named in clients/daemon-cli/tsup.config.ts external, mirroring clients/cli; AGENTS.md's "three lists" rule updated to four.
  • 2b — fixed. ipc-glue.ts and stream-client.ts are in the ≥90 per-file gate; the accept/stream races were fixed with awaited conditions, and only genuinely-unreachable lines carry annotated v8 ignore. Only the two true bootstraps remain excluded. The AGENTS.md exclusion note is gone.
  • 2c — fixed (fell out of 1d). local:gate green on stock macOS.
  • 2d — fixed. Rebased; every commit carries Signed-off-by.
  • 2e — done. PR description rewritten to match the code (including packaging); skills/ documented in the AGENTS.md and README structure trees with the .claude/skills/ distinction. Agreed on leaving the branch name.
  • 2f — done, with one deviation. Rebased onto current v2/main and re-ran local:gate. Packaging was initially split to a separate branch as you suggested, then folded back after an explicit maintainer decision to ship it in this PR — see §3.
  • 2g — done. Tracking issue chore(mcpi): replace the temporary @inspector/cli source alias with a real shared surface #2461 filed for promoting the @inspector/cli reach-in surface to a shared area; linked from the tsup comment.

§3 Decisions

  • Shipping: maintainer-approved to bundle in this PR, under the new conflict-free bin name mcpdo (your mcpi-collision point drove the rename). Data point: the addition is ~199 KB compressed / ~770 KB unpacked (~16% of the tarball), and the daemon is inert unless the bin is invoked. Without bundling there was no reasonable install story for the experimental client (build-from-source + npm link).
  • Containerizing: agree — no. Your opt-in per-connection isolation recipe (docker run -i --rm --network none …) is now documented in clients/daemon-cli/README.md; a first-class --sandbox flag is deferred as a possible follow-up.

Ready for re-review whenever you are.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The request-size security limit is bypassable, and several correctness and required lint-enforcement issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity · 4 Medium severity · 4 Low severity

Open (10)
What changed in this PR

Adds mcpdo, an experimental connection-oriented MCP CLI backed by a token-authenticated Unix-socket daemon.

Changes:

  • Implements persistent MCP connections, commands, OAuth, elicitation, streaming, and safe output formatting.
  • Adds extensive tests and integrates the client into build, validation, coverage, and packaging.
  • Documents the new client and ships an agent-facing mcpdo skill.
File Description
AGENTS.md Documents the new client and repository rules.
README.md Adds mcpdo to the project overview.
clients/​cli/​__tests__/​method-types.test.ts Updates shared method-list tests.
clients/​cli/​__tests__/​run-method-mocks.test.ts Updates reusable handler mocks.
clients/​cli/​__tests__/​servers-list.test.ts Tests reusable server-list behavior.
clients/​cli/​src/​cli-oauth-navigation.ts Exposes shared OAuth navigation.
clients/​cli/​src/​cliOAuth.ts Supports reusable OAuth flows.
clients/​cli/​src/​handlers/​consume-outcome.ts Updates shared outcome handling.
clients/​cli/​src/​handlers/​method-types.ts Defines connection-compatible methods.
clients/​cli/​src/​handlers/​run-method.ts Exposes shared MCP method execution.
clients/​cli/​src/​handlers/​servers-list.ts Generalizes server catalog loading.
clients/​cli/​src/​style.ts Exposes CLI styling helpers.
clients/​daemon-cli/​README.md Documents installation and usage.
clients/​daemon-cli/​__tests__/​agent-help.test.ts Tests agent-help output.
clients/​daemon-cli/​__tests__/​authorize.test.ts Tests authorization behavior.
clients/​daemon-cli/​__tests__/​connection-stored-auth.test.ts Tests stored-auth commands.
clients/​daemon-cli/​__tests__/​daemon-connections.test.ts Tests connection lifecycle.
clients/​daemon-cli/​__tests__/​daemon-coverage.test.ts Covers daemon edge cases.
clients/​daemon-cli/​__tests__/​daemon-ipc-glue.test.ts Tests IPC framing and handling.
clients/​daemon-cli/​__tests__/​daemon-paths.test.ts Tests daemon filesystem paths.
clients/​daemon-cli/​__tests__/​daemon-private.test.ts Tests private-daemon authentication.
clients/​daemon-cli/​__tests__/​daemon-stream.test.ts Tests streaming IPC.
clients/​daemon-cli/​__tests__/​dispatch.test.ts Tests RPC and stream dispatch.
clients/​daemon-cli/​__tests__/​elicitation-bridge.test.ts Tests daemon elicitation bridging.
clients/​daemon-cli/​__tests__/​elicitation-client.test.ts Tests elicitation client transport.
clients/​daemon-cli/​__tests__/​elicitation-prompt.test.ts Tests interactive elicitation.
clients/​daemon-cli/​__tests__/​ema-commands.test.ts Tests EMA commands.
clients/​daemon-cli/​__tests__/​ema.test.ts Tests EMA authentication logic.
clients/​daemon-cli/​__tests__/​form-prompt.test.ts Tests form prompting and validation.
clients/​daemon-cli/​__tests__/​form-schema.test.ts Tests elicitation schema parsing.
clients/​daemon-cli/​__tests__/​format-connection.test.ts Tests connection output formatting.
clients/​daemon-cli/​__tests__/​helpers/​mcp-runner.ts Adds daemon CLI test harness.
clients/​daemon-cli/​__tests__/​hoist-connection.test.ts Tests connection argument rewriting.
clients/​daemon-cli/​__tests__/​mcp-auth-coverage.test.ts Covers MCP authentication branches.
clients/​daemon-cli/​__tests__/​mcp-connection.test.ts Tests CLI connection workflows.
clients/​daemon-cli/​__tests__/​mcp-coverage.test.ts Covers command-routing edge cases.
clients/​daemon-cli/​__tests__/​parse-tool-args.test.ts Tests tool argument parsing.
clients/​daemon-cli/​__tests__/​resolve-command.test.ts Tests executable resolution.
clients/​daemon-cli/​__tests__/​sanitize.test.ts Tests terminal sanitization.
clients/​daemon-cli/​eslint.config.js Configures daemon-client linting.
clients/​daemon-cli/​package-lock.json Locks daemon-client dependencies.
clients/​daemon-cli/​package.json Defines scripts and package metadata.
clients/​daemon-cli/​src/​connection/​authorize.ts Implements connect-time OAuth.
clients/​daemon-cli/​src/​connection/​dispatch.ts Dispatches daemon RPCs and streams.
clients/​daemon-cli/​src/​connection/​elicitation-prompt.ts Implements elicitation prompts.
clients/​daemon-cli/​src/​connection/​ema.ts Implements enterprise-managed auth.
clients/​daemon-cli/​src/​connection/​form-prompt.ts Collects form elicitation input.
clients/​daemon-cli/​src/​connection/​form-schema.ts Parses elicitation schemas.
clients/​daemon-cli/​src/​connection/​format-connection.ts Formats command output.
clients/​daemon-cli/​src/​connection/​format-human.ts Provides human-readable formatting.
clients/​daemon-cli/​src/​connection/​mcp.ts Defines the mcpdo command surface.
clients/​daemon-cli/​src/​connection/​parse-tool-args.ts Parses tool-call arguments.
clients/​daemon-cli/​src/​connection/​private-env.ts Creates private-daemon shell exports.
clients/​daemon-cli/​src/​connection/​resolve-command.ts Resolves caller-side executables.
clients/​daemon-cli/​src/​connection/​sanitize.ts Sanitizes terminal-bound data.
clients/​daemon-cli/​src/​connection/​stored-auth.ts Manages persisted authentication.
clients/​daemon-cli/​src/​daemon/​auth.ts Implements daemon token authentication.
clients/​daemon-cli/​src/​daemon/​client.ts Implements request-response IPC.
clients/​daemon-cli/​src/​daemon/​connections.ts Manages persistent MCP connections.
clients/​daemon-cli/​src/​daemon/​elicitation-bridge.ts Bridges elicitation over IPC.
clients/​daemon-cli/​src/​daemon/​ensure.ts Starts and discovers the daemon.
clients/​daemon-cli/​src/​daemon/​framing.ts Encodes and parses IPC frames.
clients/​daemon-cli/​src/​daemon/​index.ts Exports daemon APIs.
clients/​daemon-cli/​src/​daemon/​ipc-glue.ts Accepts and processes socket clients.
clients/​daemon-cli/​src/​daemon/​paths.ts Defines daemon paths and limits.
clients/​daemon-cli/​src/​daemon/​protocol.ts Defines the IPC protocol.
clients/​daemon-cli/​src/​daemon/​run.ts Boots the daemon process.
clients/​daemon-cli/​src/​daemon/​server.ts Implements daemon lifecycle and routing.
clients/​daemon-cli/​src/​daemon/​stream-client.ts Implements streaming IPC clients.
clients/​daemon-cli/​src/​mcp-bin.ts Boots the mcpdo executable.
clients/​daemon-cli/​tsconfig.json Configures source type-checking.
clients/​daemon-cli/​tsconfig.test.json Configures test type-checking.
clients/​daemon-cli/​tsup.config.ts Builds CLI and daemon bundles.
clients/​daemon-cli/​vitest.config.ts Configures tests and coverage.
clients/​tui/​package-lock.json Refreshes the TUI dependency lock.
clients/​web/​package-lock.json Refreshes the web dependency lock.
clients/​web/​src/​test/​core/​auth/​runner-interactive-oauth.test.ts Tests OAuth signal cancellation.
core/​auth/​node/​runner-interactive-oauth.ts Adds optional signal handling.
core/​mcp/​serverList.ts Persists elicitation capability settings.
core/​mcp/​types.ts Defines elicitation capability types.
package.json Wires build, validation, packaging, and bin entry.
scripts/​install-clients.mjs Adds daemon-client installation.
scripts/​lib/​workflow-gate.test.mjs Updates gate coverage assertions.
scripts/​sdk-watch.mjs Includes daemon bundle externals guidance.
scripts/​verify-bundle-externals.mjs Verifies the new multi-entry bundle.
scripts/​verify-format-coverage.mjs Enrolls daemon-client formatting.
scripts/​verify-test-timeouts.mjs Enrolls daemon-client timeouts.
scripts/​verify-test-timeouts.test.mjs Updates timeout guard tests.
skills/​mcpdo/​SKILL.md Adds agent-facing usage guidance.
specification/​v2_catalog_launch_config.md Links the as-built CLI specification.
specification/​v2_cli_tui_launcher.md Documents the additional client surface.
specification/​v2_cli_v2.md Specifies the implemented connection CLI.
Files not reviewed (3)
  • clients/daemon-cli/package-lock.json: Generated file
  • clients/tui/package-lock.json: Generated file
  • clients/web/package-lock.json: Generated file

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread clients/daemon-cli/src/daemon/ensure.ts
Comment thread clients/daemon-cli/src/daemon/ipc-glue.ts Outdated
Comment thread clients/daemon-cli/eslint.config.js
Comment thread clients/daemon-cli/src/connection/elicitation-prompt.ts
Comment thread clients/daemon-cli/src/connection/form-prompt.ts
Comment thread clients/daemon-cli/src/connection/resolve-command.ts
Comment thread AGENTS.md Outdated
Comment thread clients/daemon-cli/package.json Outdated
Comment thread skills/mcpdo/SKILL.md Outdated
Comment thread specification/v2_cli_v2.md Outdated
- ensure: a losing concurrent starter re-reads the winner's published
  daemon.token instead of polling its own dead token into a bogus
  daemon_start_timeout (explicit/private tokens still fail loud); test
- ipc-glue: enforce the 1 MiB line cap per newline-delimited segment so a
  terminated oversized line can't reset the counter past the check, and
  ignore lines after rejection; unit + e2e regression tests
- elicitation: parse the form schema raw and sanitize server-controlled
  strings at render points only, so responses carry the server's own
  keys/values
- form-prompt: reject non-finite numbers ("Infinity" is not a valid JSON
  number)
- resolve-command: honor an empty PATH entry as the current directory
  (POSIX) and return absolute paths for relative entries
- lint: add the type-aware no-floating-promises pass and --max-warnings 0,
  matching clients/cli
- docs: AGENTS.md external-lists brace path mcpdo -> daemon-cli; SKILL.md
  connect example uses --config; spec no longer advertises unregistered
  `initialize`

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unvalidated terminal hyperlinks, an unsafe private-daemon parent directory, and potentially truncated stream output must be corrected before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 4 High severity

Open (4)
Resolved since last review (10)
Files not reviewed (3)
  • clients/daemon-cli/package-lock.json: Generated file
  • clients/tui/package-lock.json: Generated file
  • clients/web/package-lock.json: Generated file
Previously missed (3)

In code that hasn't changed since last review

Medium severity Add elicitCapability persistence and fallback coverage

core/​mcp/​serverList.ts:74

The new persisted setting adds valid/invalid read branches and default-omission write behavior, but the existing comprehensive serverList.test.ts suite has no elicitCapability case. Add coverage for all accepted literals, an unknown hand-edited value falling back to the default, and round-trip/default omission so this shared catalog behavior cannot regress unnoticed.

Medium severity Verify the published mcpdo executable and artifacts

package.json:22

This publishes a second executable, but scripts/pack-and-verify.mjs still checks only the installed mcp-inspector bin and web/launcher artifacts. A missing clients/daemon-cli/build, broken bin.mcpdo target, or omitted skills/mcpdo directory would therefore pass the repository's published-tarball verification. Enroll the new files and run the installed mcpdo --help in that check.

Low severity Rename the tracking entry to Connection CLI umbrella

specification/​v2_catalog_launch_config.md:515

The PR explicitly renames the user-facing concept from “session” to “connection,” but this updated row still calls #1432 the “Session CLI umbrella.” Use “Connection CLI umbrella” so the tracking table matches the as-built terminology.

Comment thread clients/daemon-cli/src/connection/dispatch.ts
Comment thread clients/daemon-cli/src/connection/elicitation-prompt.ts Outdated
Comment thread clients/daemon-cli/src/connection/format-human.ts Outdated
Comment thread clients/daemon-cli/src/daemon/paths.ts
- dispatch: chain stream writes and await the chain before returning, so
  mcp-bin's process.exit can't truncate a pending stdout write on piped or
  backpressured output; write errors stay non-fatal as before
- sanitize: isSafeLinkTarget scheme allowlist (https/http) for OSC 8
  hyperlinks; format-human and URL-mode elicitation render every other
  scheme (file:, custom protocol handlers) as plain text
- paths: fail closed unless the predictable $TMPDIR/mcp-conn-<uid> root is
  a real directory owned by the current user, and tighten a loose mode
  fatally instead of best-effort — a shared-/tmp user can no longer plant
  the root (dir or symlink) and keep write control over socket/token paths

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
BobDickinson and others added 9 commits September 28, 2026 14:05
Stage-2 enabling slice for the behavior eval — pure deterministic tooling,
no new model-dependent cases yet.

- Optional `server` field on behavior cases: {url} points the hermetic
  catalog entry at a running HTTP fixture; the composed (config-file) form
  is written to disk and served through a new eval-owned stdio launcher
  (loadConfig -> resolveConfig -> TestServerStdio) — the composable
  framework's own path, not an extension of test-server-stdio's
  purpose-built default entrypoint. The harness injects
  transport:{type:"stdio"}; specs claiming another transport are rejected.
- `phases` matcher key: ordered cross-stream regex phases over one
  invocation's recorded stdin/stdout/stderr, via a global-timeline segment
  mapping (matches span chunk boundaries; ordering enforced across
  streams); distinct diagnostics for each failure mode.
- validateServerSpec exported and wired into validateBehaviorCase so bad
  specs fail at case load, before any model run.
- 11 new tests incl. a launcher integration test driving a raw ndjson
  JSON-RPC handshake (skips when test-servers/build is absent); suite
  775 -> 786.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Composed specs may declare transport streamable-http: the harness starts
  TestServerHttp IN-PROCESS (resolveConfig -> start(), free port) and the
  catalog entry points at its URL; teardown stops it. OAuth rides on the
  same instance via the spec's oauth block (requireAuth: true is the
  enforcement knob — asserted with AS metadata + 401/WWW-Authenticate on
  unauthenticated initialize).
- Multi-server: `servers` name->spec map (names are catalog entry names,
  visible to the agent via servers/list); `server` stays as single-entry
  sugar. Per-name config files. validateCaseServers: mutual exclusion,
  non-empty map, name charset, labeled per-entry diagnostics.
- No MRU strictness rule: non-TTY agents + no MCP_ALLOW_DEFAULT_CONNECTION
  means the daemon-cli itself rejects implicit targeting, so every
  successful targeting call names its connection — connection matchers stay
  decidable with any number of entries.
- makeBehaviorEnv/teardown now async; teardown's daemon stop uses async
  spawn — a sync child wait while an in-process fixture is live deadlocks
  the event loop (found via no-model smoke, which then validated
  connect -> tools/call add -> {"result":5} through shim + daemon +
  in-process HTTP fixture).
- Suite 786 -> 791.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…letes sign-in

Agents drive mcpdo over pipes, where the blocking interactive OAuth flow is
hostile: the auth URL sits invisible in a buffered foreground pipe, and a
timeout kill tears down the loopback callback listener the URL points at,
staling the link (observed live in a Claude Code session).

On connect, when stdin AND stderr are non-TTY (and MCP_AUTO_OPEN_ENABLED is
not "true"), mcpdo now:

- spawns a detached helper (hidden auth/complete-signin subcommand; params as
  JSON over stdin, never argv) that owns the callback listener and token
  exchange, bounded by the flow's own 15-minute callback wait;
- registers the connection in the daemon as a pending intent entry
  (ConnectParams.pendingOnAuthRequired): a never-connected client whose
  terminal status makes the first real op complete the connection through
  the existing revive path once tokens land;
- exits 0 immediately with pendingAuth: true and the authorize URL in the
  normal output payload (authUrl in JSON; relay-worded block in human
  output). The URL cannot ride the error envelope: error-path redaction
  strips URL query strings, which is exactly where client_id/PKCE/state
  live.

Repeat connects while a helper is waiting reuse its URL via a pid+expiry
validated 0600 marker file in the daemon dir — minting a second flow would
collide on the fixed callback port and stale the user's held link.
connections/show recomputes auth from disk, so it doubles as the sign-in
poll; list/show/connect surface the pending state. TTY and
--stored-auth-only behavior is unchanged.

Validated end-to-end against the composable OAuth fixture (requireAuth +
DCR): non-TTY connect → exit 0 with URL → consent click → helper stored
tokens → tools/call revived and succeeded with pendingAuth cleared.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Non-TTY / --format json callers can no longer answer a mid-rpc
elicitation prompt, so the daemon now parks the call at the first
elicitation and returns an elicitationPending payload (exit 0) with
the elicitation id, mode, message, form schema or URL, and expiry.

A new 'elicitation/respond <id>' command answers it: form fields as
key:=value pairs or one JSON object (accept), --done for URL-mode
self-report, --decline (form only), or --cancel. Each respond returns
the final rpc result or the next parked round, giving a stateless
request/response loop that covers multi-round elicitation on both
legacy and modern servers (task-augmented SEP-2663 flows already
worked via tasks/* and are untouched).

One parked call per connection: new rpcs are refused with an
elicitation_pending error citing the respond command. Parked entries
expire after 10 minutes; disconnect and daemon stop cancel them
upstream so servers see a clean elicitation cancel.

Interactive TTY sessions keep the existing inline prompts.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…imulator

The secure-add behavior case composes a requireAuth+DCR streamable-http
server and measures the full headless auth flow: non-TTY connect exits 0
with the sign-in link, and the agent is expected to relay it and finish
the tools/call once access is granted.

The harness plays the human: cases opting in with autoConsent get a
watcher that polls the shim transcript for /oauth/authorize URLs and
approves each once (GET consent page, POST approve, follow the redirect
to the detached auth helper's loopback callback), after which the
agent's next call revives the connection.

Also: missed samples now dump a compact transcript (argv, exit, first
300 chars of each stream) as the failure diagnostic, and CASE_MATCH
filters cases by prompt substring for one-case iteration (labeled a dev
probe, not a measurement).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…fixture tool

collect_elicitation can only exercise the elicitation wire: the caller
composes the message and schema, so an agent can trivially satisfy it.
Measuring elicitation BEHAVIOR needs a tool whose elicitation is
intrinsic — the elicited fields deliberately absent from the input
schema, so the only way to a result is answering the mid-call request.

New submit_ticket preset: takes only a summary, elicits contact
name/email over legacy elicitation/create, and returns a ticket id
derived from the accepted content (decline/cancel: ticket not filed).

The helpdesk eval case gives the agent the contact facts in a natural
prompt and asserts the parked tools/call (exit 0, elicitationPending)
followed by an elicitation/respond returning the ticket number. No
harness user-simulator involved — the agent itself is the answerer.

First probe (claude, 3 runs): 3/3 with the current SKILL.md — the
pending output's in-band answer guidance is sufficient.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SKILL.md body rewrite (description frontmatter unchanged):
- command summary opens with servers/list; canonical flow spelled out
  (servers/list -> connect -> @entry <command>)
- new 'How to think about mcpdo' section: connections extend the toolset,
  answer capability questions with them, don't auto-connect, inspect via
  commands not the filesystem
- new 'The catalog' section: writable ~/.mcp-inspector/mcp.json,
  --catalog / MCP_CATALOG_PATH, servers/* vs connections/*, edit by file
- Conventions: connection self-healing and the [legacy]/[modern] era tag
- 'Auth': non-TTY connect exits 0 with pendingAuth + authUrl; relay the
  link, then retry the command with short sleeps (measured: 'wait for the
  user' wording made agents end their turn — 0/3; retry wording 3/3)
- 'Elicitations': parking + elicitation/respond, decline/cancel/--done,
  TTL and one-parked-call limit

Four regression behavior cases appended to evals.json (each guards a fixed
skill gap): connections-awareness, catalog-discovery, explicit-connection
(two-server catalog, matcher pins the connection), helpdesk-decline
(asserted via stdoutMatch since boolean flags don't surface in parsed
argv). All baselined 3/3 pre-edit; post-edit full suite: trigger 8/8 at
100%, behavior 8/8 at 3/3 incl. secure-add 2/3 -> 3/3.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- `agent-help` (default, or explicit `--skill`) now prints the SKILL.md
  body with frontmatter stripped, so an agent without the skill installed
  can pull guidance usable exactly as if the skill had loaded.
- `--instructions` prints a short always-on snippet to append to a
  project's CLAUDE.md/AGENTS.md, so agents treat open mcpdo connections
  as part of their toolset on every turn (skills load only on demand).
- `--skill-path` (replaces `--path`) prints the installable SKILL.md
  location for skill runtimes. Flags are mutually exclusive.
- SKILL.md gains a "Make it always-on (optional)" bullet pointing at
  `agent-help --instructions`.
- Test hygiene from ed06480: fix `createStyle` calls to the real
  `(ansi: boolean)` signature and a prefer-const slip; neither changed
  runtime behavior, both now caught by full validate.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e artifacts

Harness fixes (both produced silent never-authorized misses):
- Concurrent samples' detached auth helpers all defaulted to callback
  port 6276 and collided (EADDRINUSE, or consent redirected to a dead
  helper). Each sample now sets
  MCP_OAUTH_CALLBACK_URL=http://127.0.0.1:0/oauth/callback — ephemeral
  ports are already supported end-to-end by the product.
- The consent clicker scanned each stream chunk separately, so an
  authorize URL split across chunks was never detected. It now scans
  joined per-stream text (streamText), like the rest of the matchers;
  unit test covers a mid-URL split. Clicks are now logged.

Runner UX:
- AGENT defaults to "all": trigger + behavior suites run for claude and
  copilot back to back (AGENT=claude|copilot still selects one).
- On a behavior miss the sample's hermetic env dir is preserved under
  ~/.cache/mcpdo-skill-eval/failures/ (raw agent stream, shim transcript,
  case, catalog, server configs) and the miss report prints its path.
- Miss diagnostics show per-call timing offsets.

Validated: full AGENT=all suite green — both agents 8/8 trigger @100%
and 8/8 behavior @3/3, including the secure-add OAuth case.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The behavior-evaluation shell policy permits unrestricted host command execution, and one evaluation can report false positives without validating requested inputs.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)

Comment thread scripts/skill-eval-mcpdo.mjs
Comment thread clients/daemon-cli/evals/evals.json
BobDickinson and others added 4 commits September 29, 2026 08:51
CI coverage fell below the 90% per-file gates in four files touched by
recent auth/elicitation work. Cover the gaps:

- auth-helper: stdio configs (no marker), non-Error flow failures,
  stdin error/timeout, stdout EPIPE guard, helper spawn failure, and
  noise lines (blank/malformed/unknown events) before the auth URL.
- authorize: non-EMA connect failures rethrown unchanged; caller-provided
  makeNavigation overrides the CLI default navigation.
- format-human: colorLevel warning/debug/notice buckets and empty-URI
  passthrough via formatStreamEventHuman.
- elicitation-park: waitForElicitation with an already-pending frame;
  forClient/cancelForConnection ignoring non-matching entries.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…er's

Review finding (PR #1783): the behavior evals' shell allow-list looked like
a containment boundary but is not one — approval patterns prefix-match
(`mcpdo x && anything` passes) and `mcpdo connect` launches arbitrary
stdio commands by design. The agent under test is a nondeterministic model
with shell access; its actions are untrusted.

- runPrompt no longer spreads process.env into the agent: agentEnv() picks
  process basics (PATH, HOME, locale, proxies) plus only the agent's own
  auth/config vars (ANTHROPIC_/CLAUDE_ for claude, GITHUB_/GH_/COPILOT_
  for copilot). Exported credentials for anything else stay out.
- The behaviorAgentArgs doc now states the real model: approval scoping is
  drift reduction for a cooperating model, env is minimized, and hard
  isolation is the runner's job (container/VM/dedicated user of choice —
  there is no portable OS sandbox worth shipping here).

Validated: harness unit tests (93) green; live helpdesk 3/3 and secure-add
OAuth 1/1 on both agents under the minimal env.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review finding (PR #1783): the accept-path helpdesk case measured only the
call sequence — any summary and any contact details produced
elicitationPending then TCK-. Its stated point is mapping known facts into
the requested schema, so assert them: `summary` on the tools/call and
`contact_name`/`contact_email` on the elicitation/respond. valuesMatch
is spelling-agnostic (key:=value, JSON positional, --tool-args-json all
land in parsed args; plain key=value is rejected by the CLI itself).

Re-baselined 3/3 on both claude and copilot.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Post-format edit slipped past a re-check before push; format:check:scripts
now clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Concurrent OAuth startup, unsafe authorization hyperlinks, misleading recovery commands, and colliding fixture ticket IDs remain unresolved.

Review effort: Balanced
Findings: 2 High severity

Open (2)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Validate OAuth URLs before rendering OSC 8 hyperlinks

clients/​daemon-cli/​src/​connection/​format-connection.ts:278

authUrl originates from OAuth metadata controlled by the remote server, but this path emits it directly as an OSC 8 hyperlink. That bypasses the scheme allowlist used for every other server-supplied link, allowing values such as file: or custom protocol handlers to invoke a local handler when clicked. Gate this with isSafeLinkTarget and render unsafe targets as plain text, as format-human.ts already does.

Comment thread clients/daemon-cli/src/connection/auth-helper.ts
Two review findings (PR #1783):

- The pending-marker check and helper spawn were not atomic: two
  concurrent non-TTY connects for the same server could both pass the
  check and spawn helpers that contend for the OAuth callback port.
  A per-server lock file (wx-exclusive create) now reserves the flow
  before spawning; the loser polls for the winner's marker (published
  before the helper reports its URL) and reuses it. Stale locks from a
  crashed reserver are stolen after the URL wait window, so a crash
  cannot wedge sign-in.

- connect's sign-in block emitted the server-controlled OAuth authUrl
  as an OSC 8 hyperlink unconditionally. It now goes through the same
  isSafeLinkTarget scheme allowlist as every other server-supplied
  link; unsafe targets render as plain text.

Validated: full validate green, per-file coverage thresholds met, live
secure-add OAuth eval passing on both claude and copilot.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author

Re the latest Copilot review overview:

  • "Validate OAuth URLs before rendering OSC 8 hyperlinks" (body-only finding, no inline thread): valid — fixed in 4b47db4. connect's sign-in block now gates the server-controlled authUrl through the same isSafeLinkTarget scheme allowlist as every other server-supplied link; unsafe targets render as plain text. Covered by tests.
  • "Misleading recovery commands" and "colliding fixture ticket IDs" (mentioned in the overview sentence only): no corresponding finding, thread, or location was included in any review, so there isn't enough information to act on these. If they resurface with specifics, happy to address.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

OAuth helper coordination contains two stale-file races and an unref’ed polling timer that can lose concurrent sign-in flows.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (2)

Comment thread clients/daemon-cli/src/connection/auth-helper.ts
Comment thread clients/daemon-cli/src/connection/auth-helper.ts
…l atomic

Address Copilot review round 3 on the sign-in reservation code:

- waitForPendingAuthUrl: don't unref the poll timer — for a reservation
  loser it can be the only live handle, and an unref'ed timer let Node
  exit mid-wait without ever printing the authorization URL.
- readLivePendingAuthMarker: stop deleting stale/dead markers at read
  time; the unlink-by-pathname raced a just-spawned helper's fresh
  marker (TOCTOU). Stale markers are inert and writers replace them
  with rm+wx.
- tryReserveAuthFlow: steal stale locks via an atomic rename-claim to a
  per-pid path so concurrent stealers cannot both win, with a
  post-rename staleness recheck. POSIX has no compare-and-delete; the
  rename makes the claim exclusive, which is what prevents double
  helper spawns.

Tests updated for the no-delete-on-read semantics plus a claim-
contention back-off case.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

RPC cancellation, caller-environment propagation, and evaluation transcript handling contain unresolved correctness defects.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Resolving only the executable does not preserve the invoking shell's stdio environment.…

clients/​daemon-cli/​src/​connection/​mcp.ts:547

Resolving only the executable does not preserve the invoking shell's stdio environment. StdioClientTransport is constructed in the persistent daemon, so its inherited PATH, HOME, SHELL, etc. come from whichever shell first spawned that daemon; a later shell can resolve the right executable here while the server and any subprocesses still receive stale values. Snapshot the SDK's inherited environment client-side at connect time and merge the configured env over it before sending serverConfig.

Low severity The PR description says JSON mode auto-declines form elicitation, but this implementation…

clients/​daemon-cli/​src/​connection/​dispatch.ts:110

The PR description says JSON mode auto-declines form elicitation, but this implementation deliberately parks it and returns elicitationPending; the new elicitation test and shipped skill document the parked workflow too. Update the PR description to match the actual JSON contract so users do not assume the server receives a decline automatically.

Comment thread scripts/skill-eval-mcpdo.mjs
BobDickinson and others added 2 commits September 29, 2026 12:05
Resolves the AGENTS.md dependency-rules conflict: takes v2/main's
server-legacy devDependency rewrite (#2519/#2520) and keeps this
branch's daemon-cli additions (four externalized clients; daemon-cli in
the no-runtime-deps list).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…pt corruption

Address Copilot review round 4:

- readTranscript (eval harness): a malformed record before the final
  line now throws with its line number instead of being silently
  dropped, matching the function's stated contract — only a torn final
  line (a kill artifact) is tolerated.
- connect: snapshot the SDK's default-inherited environment (PATH,
  HOME, SHELL, ...) from the calling shell and merge it under any
  configured env before the config crosses to the daemon. The transport
  otherwise evaluates getDefaultEnvironment() inside the persistent
  daemon, handing servers the environment of whichever shell first
  spawned it. Extracted the cwd/command/env pinning into an exported
  pinStdioConfigToCaller, which now also treats a type-less config as
  stdio (stdio is the implicit default).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author

Re the two body-only "previously missed" items and the headline in the latest review:

  • Stale environment for stdio servers (mcp.ts): valid — fixed in 5b92cff. connect now snapshots the SDK's default-inherited environment (PATH/HOME/SHELL/…) from the calling shell and merges it under any configured env before the config crosses to the daemon, completing the existing absolute-cwd/command handoff. The pinning is extracted into pinStdioConfigToCaller with unit tests, and now also treats a type-less config as stdio (the implicit default).
  • PR description vs JSON elicitation behavior (dispatch.ts): the description was stale — updated. Non-interactive callers (--format json or no TTY) get the elicitation parked and answer via elicitation/respond; nothing is auto-declined.
  • "RPC cancellation" (headline only): mentioned in the overview sentence but appears nowhere as a finding, so there is nothing actionable to evaluate.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Form validation mishandles valid Unicode lengths, ticket identifiers can collide, and an architecture document remains inconsistent.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity · 1 Low severity

Open (4)
Resolved since last review (1)

Comment thread test-servers/src/test-server-fixtures.ts Outdated
Comment thread clients/daemon-cli/src/connection/form-prompt.ts
Comment thread clients/daemon-cli/src/connection/form-schema.ts
Comment thread specification/v2_cli_tui_launcher.md
…ng, spec doc

Address Copilot review round 5:

- form-schema/form-prompt: minLength/maxLength are measured in Unicode
  code points per JSON Schema, not UTF-16 units — a valid astral-char
  default (or answer) was rejected / trapped in the re-prompt loop.
  Shared codePointLength() applied at all four bound checks.
- test-server-fixtures: submit_ticket ids now hash the full submission
  (djb2) instead of summary/email lengths, which collided whenever only
  contact_name changed; comment made honest about the 4-digit space.
- specification/v2_cli_tui_launcher.md: 'Shared core consumption' and
  the summary now count daemon-cli among the core consumers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Subscription notifications can be lost, OAuth marker cleanup races with replacement flows, and pending connections report an unnegotiated protocol era.

Review effort: Balanced
Findings: 2 High severity

Open (2)
Resolved since last review (4)

Comment thread clients/cli/src/handlers/run-method.ts
Comment thread clients/daemon-cli/src/connection/auth-helper.ts Outdated
…marker cleanup

Address Copilot review round 6:

- resources/subscribe: the resourceUpdated listener now attaches before
  the subscribe handshake and buffers matching updates until the
  consumer's start() — a server notifying immediately after (or with)
  its subscribe response no longer loses that update in the window
  before the stream starts. The subscribe-failure path detaches the
  listener; ipc-glue already guarantees every stream outcome is started
  and stopped, so no leak on vanished callers.
- auth helper exit: marker cleanup now verifies the on-disk marker's
  pid is this helper's before deleting (removeOwnPendingAuthMarker) —
  past the 15-minute TTL a replacement flow's fresh marker at the same
  pathname would otherwise be deleted out from under its callers. The
  residual read-to-rm window is documented; losing it costs one extra
  sign-in prompt, never a wrong URL.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

IPC Unicode corruption, cross-connection elicitation ID collisions, and incorrect shipped guidance must be resolved.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Low severity Update docs for parked elicitation behavior

clients/​daemon-cli/​README.md:175

This section still describes the former auto-decline behavior. The implemented dispatch path sets parkElicitations for JSON and no-TTY callers, returns an elicitation-pending payload, and resumes through elicitation/respond, as the PR summary also states. Update these instructions so agents do not expect an automatic decline or try to answer an inline prompt that is never opened.

…uto-decline

The README still said --format json auto-declines forms and every other
non-TTY caller gets a real prompt. Actual behavior since the parking
change: non-interactive callers (--format json or no TTY) get the
elicitation parked, the RPC returns an elicitationPending payload, and
the answer comes via elicitation/respond (auto-cancel after 10 minutes).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author

Re the latest review (no findings, one body-only item):

  • README parked-elicitation docs: valid — fixed in 03bb675. The elicitation section now describes the actual behavior: non-interactive callers (--format json or no TTY) get the elicitation parked, the RPC returns an elicitationPending payload, and the answer comes via elicitation/respond (form key:=value/JSON, --done, --decline, --cancel; auto-cancel after 10 minutes). The --elicit off bullet no longer claims requests are auto-declined.
  • "IPC Unicode corruption" and "cross-connection elicitation ID collisions" are mentioned only in the overview headline; no finding for either appears anywhere in the review, so there is nothing actionable to evaluate.

This branch has not been deployed

No deployments
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

Development

Successfully merging this pull request may close these issues.

Inspector mcpi client

3 participants