feat(cli): isolate ACP MCP per Session (PR5 γ)

[Sun-GLiang]

Summary

PR 5 γ from #3132, split out of #5222. This replaces the temporary rejection of client-provided stdio mcpServers with per-ACP-Session MCP configuration, process lifecycle, tool publication, grants, invocation, and cleanup. A Session-scoped Host capability binding prevents same-named servers on one ACP connection from leaking tools or grants across Sessions. The TUI publication path shares the extracted publication helper while Host remains the execution and authorization authority.

The Host protocol compatibility epoch advances 168 → 169 for the Session-scoped capability contract; compatible client/Host builds are required. A crashed MCP server withdraws its tools without blocking later prompts; Host registration slots are bounded per provider. This does not add HTTP/SSE/OAuth MCP management or session/load/session/resume support, which remains PR 6 scope.

Refs #3132
Refs #5222

Verification

• Rebased on main commit 64f5e83de after α/β merged.
• Full-workspace npm run build:test and npm run typecheck passed.
• npm run lint, npm run format:check, and the protocol epoch guard (168 → 169) passed. ASF header and CLI third-party notice checks passed on the earlier split head and were not rerun for this review update.
• 153 affected Runtime Host tests, 14 ACP Session MCP tests, and all five official-SDK child-process tests with a real Host and stdio MCP fixtures passed. These cover bounded registration, retirement ordering, crash withdrawal, permission/form continuation, parallel Session isolation, cancellation, full subscription capacity, and authoritative result reconciliation. The Core MCP grant test passed on the earlier split head.
• On the earlier split head, the full CLI dist suite reached 1148 passed and 3 skipped. Its two failures were the local managed-Host cold-start cases; both reproduced with the same connect_failed result on a clean 852a9748d control worktree. The full CLI suite was not rerun after this review update.
• Not rerun after this review update: full Runtime Host/Core/Desktop E2E suites or a new third-party Zed smoke test. Historical combined-PR evidence is documented separately and is not claimed as a γ-head rerun. The unrelated Side Chat E2E flake fix is not included.
• GitHub checks passed, including test, installed CLI validation on macOS/Linux/Windows, and the immutable source tarball build.

Review follow-up

• P1 epoch collision: rebased onto 64f5e83de and advanced the Host compatibility epoch to 169. The protocol epoch guard passes against that base.
• P2 unbounded Session registrations: limited each provider to 64 Session registration slots; replacing an existing slot remains allowed. Session retirement now runs in the capability mutation queue, closing the queued-replace race. Pre-creation publication means requiring an already-live Session ID would reject the legitimate ACP creation flow.
• P2 crashed MCP server: initial creation still requires every configured server to connect. Later prompts wait for the current tool snapshot to be published; a dead server's tools are withdrawn, and the Session can continue with remaining or built-in tools. Tests cover partial and complete withdrawal.
• Process cleanup: the MCP SDK 2.0.0 transport already escalates from SIGTERM to SIGKILL for its direct child. It does not guarantee cleanup of a wrapper's descendants. The ACP README now states that direct-child boundary explicitly; descendant cleanup remains a separate process-supervision concern.
• Interop notes: mcpServers is required by the ACP request type, and this PR intentionally accepts only stdio MCP servers. The ACP publication path normalizes server/tool IDs before Host registration; the general wire decoder still allows broader IDs that fail closed at grant storage.

Review focus

α (#5384) and β (#5385) are merged. This branch was rebased onto main commit 64f5e83de and contains three γ commits. Please scrutinize Session scope, close/retire/reconnect races, provider fallback, and Desktop/TUI compatibility. The original #5222 is unchanged.

AI use

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: Codex contributed the split implementation, tests, documentation, and this description. Retain the Generated-by: Codex trailer in the final squash commit. Independent human review is still required.

Checklist

• Tests cover the change and fail without it
• Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

• Yes — described under Summary above
• No

[Astro-Han]

Two-slice adversarial review (Host protocol + CLI/ACP client). Headline: the Session-isolation invariant holds with three layers of defense — binding selection requires providerId && sessionId double-match, assertRegistrationSession runs inside both prepareTool and callTool, and grants are constructed from the stored request's sessionId rather than trusting call-time input. Same-named servers across Sessions get distinct processes and disjoint grant slots; connection-wide and session-scoped registration overlap is rejected in both directions; unregister scope is server-side resolved so a client can't forge another scope. The authorization chain stays Host-owned end to end. Nice work.

Findings: 1 P1 + 3 P2 inline/below, plus P3s.

P2 — an MCP server crash permanently wedges the whole ACP Session (packages/cli/src/acp/session-mcp.ts:203-211). ready()/#assertConnected requires all configured servers connected; a server that dies mid-session flips to disconnected, and every subsequent session/prompt fails mcp_not_ready — including built-in tools that never touched MCP. ACP has no reconnect path (TUI has a manual reconnect; nothing in acp/ calls manager.reconnect()), and load/resume is PR-6 scope, so the client must close+recreate and loses session context. Fail-closed is the right security posture but the availability blast radius is too big for one disposable server. Options: bounded manager.reconnect() in ready(); or relax the prompt gate to "snapshot published" (the publication layer already withdraws a dead server's tools, so running without them doesn't break isolation); at minimum return a distinguishable error code so clients can prompt a session restart. ② path — a crash is a normal failure mode.

P2 — close/EOF doesn't reliably kill wrapper-spawned MCP servers (packages/mcp/src/index.ts transport close ~:3010; contract claimed in packages/cli/src/acp/README.md:133-134). StdioClientTransport.close() SIGTERMs only the direct child — no escalation, no process-group kill — but npx/uvx/python -m is the most common MCP launch shape, so grandchildren survive as orphans holding ports, files, and client-injected env. Not new machinery, but this PR writes "Session close terminates its processes" as an explicit contract and exposes it to the lower-trust ACP path. Fix centrally in @maka/mcp (POSIX detached + kill(-pid), or SIGTERM→timeout→SIGKILL; taskkill /T on Windows) — TUI benefits too.

P3s: retireSessions bypasses #activation.runMutation — a queued replace for a retired sessionId re-pins the slot until disconnect (memory only, client-capability-coordinator.ts:859-873); session/new hard-requires mcpServers array and rejects type outright — both defensible fail-closed choices, worth noting for interop; serverId wire charset is broader than the safeId grant-key charset — non-safe ids fail closed in grant storage, fine but worth a normalization note.

Note on the CLI dist suite: the two connect_failed cold-start failures reproducing on clean 852a9748d control are consistent with an unrelated flake — accepted.

[Astro-Han]

P1 — epoch collision, merge-blocker. main is already at 168 (167 was taken by #5468 removing turn.regenerate). This PR claims 167. Either merge resolution trips protocol-epoch-check.mjs: staying at 167 gives headEpoch < baseEpoch; bumping to 168 puts this PR's incompatible client-capability changes (sessionId, admission:'mcp') on an epoch base already saw → rejected. Rebase onto current main and take 169.

[Sun-GLiang]

Thanks, agreed. I rebased γ onto main at 64f5e83de and advanced the Host compatibility epoch to 169 in 2e37db52e. The protocol epoch guard reports 168 -> 169, and the GitHub test check now passes.

[Astro-Han]

P2 — sessionRegistrations has no bound, and sessionId is fully client-controlled. Every other dimension of this protocol has a hard cap (offers 32, tools 256, manifest 56KiB), but a connection holding canPublishClientCapabilities can loop client.capability.replace with fabricated sessionIds — each pins a ≤56KiB frozen manifest plus registration entry until disconnect, and retireSessions only cleans real session ids so forged ones never release. Host memory grows unboundedly within a connection. ③ attacker-controlled input. Fix: cap provider.sessionRegistrations.size (allowing replace on existing ids) or validate the sessionId is a live session.

[Sun-GLiang]

Agreed. client.capability.replace now caps each provider at 64 Session registration slots while still allowing replacement of an existing slot and reuse after unregister. Checking for an already-live Session would reject the intended ACP flow, which publishes capabilities before Host session.create. I also put retireSessions in the capability mutation queue, so a queued replacement cannot re-pin a retired slot. Both boundaries have regression tests in 2e37db52e.

[Astro-Han]

P2 — one dead server wedges the entire Session. After any configured server flips disconnected, every prompt fails here with mcp_not_ready — even for conversations that would only use built-in tools — and there is no reconnect path on the ACP side (manager.reconnect() exists but nothing in acp/ calls it). Suggest bounded reconnect here, or relax the gate to "current snapshot published" (a dead server's tools are already withdrawn by the publication layer), or at least a distinguishable error code.

[Sun-GLiang]

Agreed. Initial prepare() still requires every configured server to connect. For later prompts, ready() waits for the current capability publication to settle, so a crashed server's tools are withdrawn before the Session continues with its remaining or built-in tools. Regression tests cover both a partial withdrawal and the only server dying (complete withdrawal). The existing RequestError.data.code already carried mcp_not_ready; the availability fix removes the persistent failure path in 2e37db52e.

[Sun-GLiang]

@Astro-Han Thanks for the thorough review. I replied to the three inline findings above. Follow-up on the remaining points:

• Wrapper-spawned processes: I agree that descendants are not guaranteed to exit on Session close. One factual correction: the pinned @modelcontextprotocol/client 2.0.0 StdioClientTransport.close() already escalates from SIGTERM to SIGKILL for its direct child after a grace period. It does not kill a process group. I narrowed the ACP README's cleanup contract to direct-child transport cleanup and explicitly noted the launcher/descendant limitation. I have not added cross-platform process-tree supervision in this PR, so descendant cleanup remains an acknowledged limitation.
• P3 retirement race: retireSessions now runs in the capability mutation queue, and a regression test covers a queued replace followed by retirement.
• P3 interop/IDs: The current ACP NewSessionRequest requires an mcpServers array; this PR intentionally accepts only untagged stdio servers. The ACP publication path normalizes server/tool IDs before Host registration. The general wire decoder accepts a broader charset, which fails closed at grant decoding; I added that note to the PR description.

Commit 2e37db52e contains the changes. Local build, typecheck, lint, format, the protocol epoch guard, 153 affected Host tests, 14 ACP MCP tests, and five real Host/ACP child-process tests passed. All current GitHub checks are green.

[Astro-Han]

Re-reviewed 2e37db52. All findings from the previous round are resolved, and I verified the fixes rather than taking the replies at face value:

• Epoch: now 169 in the diff — but note main has since moved to 170, so this needs 171 on rebase (the PR is currently conflicting anyway; the epoch guard will catch it either way).
• Registration cap: MAX_SESSION_REGISTRATIONS_PER_PROVIDER=64 is correct — replacement of an existing sessionId bypasses the cap, unregister frees the slot immediately, and forged sessionIds are bounded to ~3.5MiB/provider of inert memory. Accepting publish-before-session.create is a reasonable trade for the ACP flow.
• ready() semantics: initial prepare() still fails closed, post-prepare server death now withdraws tools and lets the Session continue — verified against the SIGKILL regression tests covering partial and complete withdrawal.
• retireSessions now runs inside the capability mutation queue; releaseConnection ordering is safe in both directions.
• Isolation invariant re-verified at the new head: binding-time double match (providerId + sessionId), assertRegistrationSession on both prepareTool and callTool, grants built from stored registrations with runtime-derived sessionId, and session-scoped offers are structurally barred from call/turn affinity pools. The leak mechanism I described last round is closed.
• Wrapper-spawned descendants: documented limitation now matches the code (direct-child transport cleanup, SIGTERM→SIGKILL escalation, no process-group kill). Acceptable as declared.

One new finding from this round:

P2 — currentRegistrationRetired can wedge a live Session into permanent mcp_not_ready (② failure/recovery path). When a committed replace response is lost (handshake timeout or interrupted request), the client drops its local record of the new registration while the superseded one remains current. The Host then releases the superseded registration and sends registration_release; #settleReleasedCurrentRegistration routes that to provider.currentRegistrationRetired() → AcpSessionMcp.#retire → #close(false). The publisher's #closed flag makes every subsequent ready() throw mcp_not_ready with no re-publish path — yet the Host-side Session is still alive and the new registration is still committed. This contradicts the contract this PR establishes elsewhere ("server death → withdraw tools → prompt continues"): here the Session is permanently bricked even though it could legitimately continue with zero tools or re-publish a fresh registration. acp-session-mcp.test.ts asserts the wedged behavior for the genuine retirement case, which is correct — the gap is distinguishing "Host retired the Session" from "the released registration was merely superseded by a committed-but-unacked replace". Worth treating retire-as-close as scoped to the retired-Session case, or letting ready() settle to a withdrawn/empty-tools state instead of hard-closing.

P3s:

• retireSessions has no tombstone — a replace arriving after retire re-pins a slot for a removed Session (inert ≤3.5MiB memory, unreachable; arguably intentional for archived Sessions). Optional.
• retireSessions only bumps #revision when it deleted a sessionRegistration — a runWithSessionBindingPreview committed after retire can resurrect a retired Session's provider-wide #sessions entry. Pre-existing on main and strictly better here; one unconditional #revision += 1 per retired sessionId would close it.
• mcpUnavailable stamps operation: 'mcp.prepare' even for ready()/#assertOpen failures — diagnostics-only.
• session/new aborted after session.create commits leaves an owned Session holding MCP processes the client never learned about — same outcome-unknown retention shape as main, just with more held resources; recoverable via session.list/close or disconnect disposal.

Not approving this round only because of the P2; everything else from the prior review stands resolved. The epoch bump to 171 is mechanical on the required rebase.

[Sun-GLiang]

Thanks for the detailed review. I rebased the PR onto current Apache main (879e0a4), advanced the Host compatibility epoch from 171 to 172, and pushed the follow-up as 1f7c88c.

What changed:

• The exact lost-response-by-timeout/interruption path from P2 is not reachable in the current dispatcher: capability mutations use the connection-scoped failure path, which closes/clears the channel before the client finally block can treat a release as authoritative retirement. I did find a narrower production path with the same state-divergence consequence: client.capability.replace can commit, then the post-commit RootTurnCoordinator recovery can fail while the channel is still usable. The wrapper now logs a bounded diagnostic, requests Host drain, and returns the already-committed success result. A regression test exercises the production composition wrapper and verifies the replacement remains acknowledged.
• Session retirement now increments the coordinator revision for every retired Session, even when no scoped registration or stored Session state exists. This invalidates a provider-wide binding preview captured before retirement, so it cannot commit afterward and recreate the Session.
• Session MCP failures now report the actual operation: readiness failures use mcp.ready, while preparation failures use mcp.prepare.

I intentionally did not add a permanent pre-create tombstone. The pre-create registration is bounded by the existing per-provider Session limit and provider lifetime, and retirement now invalidates any in-flight preview. I also kept the documented session/new outcome-unknown retention behavior: without an authoritative create failure, unregistering could withdraw MCP tools from a Session that the Host actually created.

Verification:

• npm --workspace @maka/runtime-host run typecheck
• npm --workspace maka-agent run typecheck
• node --test packages/runtime-host/dist/tests/client-capability-session-scope.test.js (10/10)
• targeted production composition regression (1/1)
• node --test packages/cli/dist/tests/acp-session-mcp.test.js (14/14)
• npx biome format/lint on the touched source and test files
• node scripts/protocol-epoch-check.mjs --base upstream/main (171 -> 172)

The full npm run build:test successfully built the affected dependency chain, Runtime Host, and CLI, then stopped later in untouched @maka/ui code on existing API mismatch errors such as settledText/autoScroll/trailingAction.

[Astro-Han]

Re-reviewed 73be1ae8. The remaining items from the last round are resolved:

• Epoch: 176, correctly ahead of main's 175.
• P2 (registration-release wedge): the timeout path I described is structurally unreachable — capability mutations run through #requestOperation with timeoutScope: 'connection', so a lost response calls #fail and tears down the channel itself; no usable channel remains to receive a stale registration_release, and reconnect republishes. For the narrower divergence that is reachable (committed replace + failed post-commit recovery), the wrapper now logs a bounded diagnostic, requests Host drain, and still returns the committed success — so the client keeps the new registration instead of discarding it and mistaking the old one's release for retirement. That closes the wedge without weakening authoritative retirement.
• retireSessions revision: now bumped unconditionally per retired sessionId, so a pre-retirement binding preview can no longer commit afterward and resurrect the Session.
• mcp.ready/mcp.prepare operation naming now reflects the actual call site.

Verified again at this head: 64-slot cap semantics (replacement bypass, unregister frees), session-scoped offers still barred from call/turn affinity, grants built only from stored registrations with runtime sessionId, retireSessions inside the mutation queue, close awaits withdrawal. Declining the permanent tombstone is fine — the pre-create registration is bounded by the per-provider cap and retirement now invalidates in-flight previews.

Approving. Nice work — this is a large surface and the isolation contract holds.

[likun666661]

Review conclusion: the current implementation addresses the core ACP Session-scoped MCP isolation problem: per-Session MCP lifecycle, capability publication, tool selection, grants, invocation, reconnect, crash withdrawal, and retirement are covered. The earlier epoch collision, unbounded registration, dead-server session wedge, and lost registration response issues have been addressed.

Non-blocking note: cleanup guarantees cover the direct MCP child process; wrapper-spawned descendants may outlive the Session. This limitation is documented and can be handled separately.