fix(acp-server): restore stdio MCP server support in ACP session methods - #3168
fix(acp-server): restore stdio MCP server support in ACP session methods#3168kimi-agent-bot wants to merge 3 commits into
Conversation
…al in one transaction RuntimeUnitHost.registerRuntime counted staged entries that had already been removed (removeRuntime marks them inactive but keeps them in the transaction), so unbinding and re-binding the same runtime id in one transaction failed with 'registered twice in one transaction'. ACP session/close unbinds the session runtime and session/load re-binds it, so close -> load of the same session in one process always failed.
84da662 (MoonshotAI#2961) replaced the stdio branch of acpMcpServersToConfigRecord with a hard 'does not declare a runtime identity' error and made the session-overlay McpConnectionManager require an explicit runtime_id for stdio configs. The ACP wire protocol cannot express a runtime_id, so every session/new, session/load, and session/resume carrying a stdio MCP server (the ACP baseline transport, sent by default by clients such as Zed) has failed with -32603 since v0.37.0. Restore the pre-MoonshotAI#2961 stdio mapping and drop requireStdioRuntimeId from the session-overlay manager; like the baseline manager it falls back to the workspace 'local' runtime. The restored session/load lifecycle test depends on the runtimeUnitHost rebind fix in the previous commit.
🦋 Changeset detectedLatest commit: d794ca8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@codex review |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28350884b7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Related Issue
Resolve #3167
Problem
Since v0.37.0, every ACP
session/new,session/load, andsession/resumerequest carrying a stdio MCP server declaration fails with-32603 Internal error: ACP stdio MCP server <name> does not declare a runtime identity. Stdio is the implied baseline transport in ACP, so this breaks all ACP clients that forward stdio MCP servers — including Zed, whose context servers are stdio by default. Full analysis in the linked issue.What changed
The regression came in with #2961 (84da662), which (a) replaced the stdio branch of
acpMcpServersToConfigRecordwith a hard error and (b) made the session-overlayMcpConnectionManagerrequire an explicitruntime_idfor stdio configs — a value the ACP wire protocol cannot express. This PR reverts both enforcement points and fixes a second latent bug from the same refactor that the restored lifecycle test exposed:packages/acp-server/src/convert.ts— restore the pre-refactor(agent-core-v2): decouple workspace from session DI via runtime binding #2961 stdio branch: map entries without atypediscriminator to{ transport: 'stdio', command, args, env }.packages/agent-core-v2/src/workspace/workspaceMcp/workspaceMcpService.ts— droprequireStdioRuntimeId: truefrom the session-overlay manager only; like the baseline manager it now falls back to the workspace'local'runtime. (Fixing only convert.ts would just move the failure to engine connect time: "MCP stdio requires runtime_id and runtime binding".)packages/agent-core-v2/src/runtime/runtimeUnitHost.ts—registerRuntimeno longer counts staged entries that were already removed (removeRuntimemarks them inactive but keeps them in the transaction). Previously, unbinding and re-binding the same runtime id in one transaction threw "registered twice in one transaction", sosession/close→session/loadof the same session in one process always failed — regardless of MCP servers.commit()runs exactly once per transaction before any removal can happen, so skipping inactive entries in the duplicate check is safe.convert.test.tsmaps stdio servers again;lifecycle.test.tsassertssession/newandsession/loadactually connect an ACP-provided stdio MCP server; newruntimeUnitHost.test.tscase covers remove-then-re-register of the same runtime id.Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.Verification:
@moonshot-ai/acp-serverfull suite 146/146 passed;@moonshot-ai/agent-core-v2full suite 5522 passed / 1 skipped; typecheck of both packages clean; repooxlint --type-aware0 errors.