fix(acp): register MCP extensions when resuming a session#7806
fix(acp): register MCP extensions when resuming a session#7806codefromthecrypt merged 6 commits intoblock:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcdf56a218
ℹ️ 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".
|
/cc @codefromthecrypt -- thoughts? |
The ACP `on_load_session` path created a fresh agent without restoring previously-registered MCP extensions, leaving the resumed session with no tools. The REST API path already called `agent.load_extensions_from_session()` correctly — this wires the same call into the ACP path for feature parity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The ACP protocol's LoadSessionRequest includes an mcp_servers field where the client re-specifies which MCP servers to connect to. The previous fix (bcdf56a) restored extensions from persisted session state, but the protocol expects the client to provide the current server list on each session load (matching on_new_session behavior). Switch to using args.mcp_servers with the same add_extension loop as new sessions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…etup The MCP extension registration loop was duplicated verbatim in on_new_session and on_load_session. Extract it into a shared add_mcp_extensions method. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The helper method name is self-descriptive; the comment restated what was already clear from context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5db5aa9 to
92ed816
Compare
|
I've switched around the solution a bit, added a test, and confirmed it fixes the issue when I actually run it |
Add run_load_session_mcp test that creates a session with an MCP server, uses a tool, then loads the same session (re-specifying MCP servers per the ACP protocol) and verifies the tool still works. The Connection::load_session trait method now accepts mcp_servers so that tests can pass MCP servers on session resume, matching the LoadSessionRequest schema. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
92ed816 to
a41cbf0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a41cbf0b14
ℹ️ 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".
| sacp::Error::internal_error().data(format!("Failed to set provider: {}", e)) | ||
| })?; | ||
|
|
||
| Self::add_mcp_extensions(&agent, args.mcp_servers, &session_id).await?; |
There was a problem hiding this comment.
Calling add_mcp_extensions inside on_load_session can overwrite enabled_extensions in session metadata with only the agent’s currently loaded set (defaults + newly passed MCP servers), because Agent::add_extension persists state immediately while ACP load does not rehydrate prior session extensions first. In sessions that already had other persisted extensions, loading with a non-empty mcp_servers list will silently drop those saved extensions from storage, so they are lost on subsequent resumes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@chatgpt-codex-connector
I believe we want the new extensions (plus the current builtins etc) to be used instead of the original session ones.
eg: i start a localhost MCP server and start an ACP session. Later i start my mcp server again and resume the session over ACP. The latest mcp server port points to the current server, not the one in the session history.
There was a problem hiding this comment.
To use Codex here, create a Codex account and connect to github.
There was a problem hiding this comment.
agree with @matt2e this is a nuance that isn't well covered in spec, but at least we are consistent on.. if this needs to be revised for any future work we have one place to look.
Add the test to provider_test.rs (ignored, matching run_load_model) so the function is imported in both test binaries. This follows the existing pattern rather than using #[allow(dead_code)]. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03b75acf1f
ℹ️ 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".
There was a problem hiding this comment.
sorry I wanted to check first, yeah it seems like this will be a common concern (new, load, fork, resume).
thanks for using the fixtures!
* main: (270 commits) test(acp): align provider and server test parity (#7822) fix(acp): register MCP extensions when resuming a session (#7806) fix(goose): load .gitignore in prompt_manager for hint file filtering (#7795) fix: remap max_completion_tokens to max_tokens for OpenAI-compatible providers (#7765) fix(openai): preserve Responses API tool call/output linkage (#7759) chore(deps): bump @hono/node-server from 1.19.9 to 1.19.11 in /evals/open-model-gym/mcp-harness (#7687) fix: return ContextLengthExceeded when prompt exceeds effective KV cache size (#7815) feat: MCP Roots support (#7790) fix(google): use `includeThoughts/part.thought` for thinking handling (#7593) refactor: simplify tokenizer initialization — remove unnecessary Result wrapper (#7744) Fix model selector showing wrong model in tabs (#7784) Stop collecting goosed stderr after startup (#7814) fix: avoid word splitting by space for windows shell commands (#7781) (#7810) Simplify and make it not break on linux (#7813) Add preferred microphone selection (#7805) Remove dependency on posthog-rs (#7811) feat: load hints in nested subdirs (#7772) feat(acp): add read tool and delegate filesystem I/O to ACP clients (#7668) Support secret interpolation in streamable HTTP extension URLs (#7782) More logging for command injection classifier model training (#7779) ...
When resuming a session via ACP, register passed in MCP extensions like when a session is started.
Previously Goose ignored the extensions argument for resume session