fix(acp): register MCP extensions when resuming a session (#7806)
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -789,19 +789,7 @@ impl GooseAcpAgent {
|
||||
sacp::Error::internal_error().data(format!("Failed to set provider: {}", e))
|
||||
})?;
|
||||
|
||||
for mcp_server in args.mcp_servers {
|
||||
let config = match mcp_server_to_extension_config(mcp_server) {
|
||||
Ok(c) => c,
|
||||
Err(msg) => {
|
||||
return Err(sacp::Error::invalid_params().data(msg));
|
||||
}
|
||||
};
|
||||
let name = config.name().to_string();
|
||||
if let Err(e) = agent.add_extension(config, &goose_session.id).await {
|
||||
return Err(sacp::Error::internal_error()
|
||||
.data(format!("Failed to add MCP server '{}': {}", name, e)));
|
||||
}
|
||||
}
|
||||
Self::add_mcp_extensions(&agent, args.mcp_servers, &goose_session.id).await?;
|
||||
|
||||
let session = GooseAcpSession {
|
||||
agent,
|
||||
@@ -841,6 +829,27 @@ impl GooseAcpAgent {
|
||||
Ok(provider)
|
||||
}
|
||||
|
||||
async fn add_mcp_extensions(
|
||||
agent: &Agent,
|
||||
mcp_servers: Vec<McpServer>,
|
||||
session_id: &str,
|
||||
) -> Result<(), sacp::Error> {
|
||||
for mcp_server in mcp_servers {
|
||||
let config = match mcp_server_to_extension_config(mcp_server) {
|
||||
Ok(c) => c,
|
||||
Err(msg) => {
|
||||
return Err(sacp::Error::invalid_params().data(msg));
|
||||
}
|
||||
};
|
||||
let name = config.name().to_string();
|
||||
if let Err(e) = agent.add_extension(config, session_id).await {
|
||||
return Err(sacp::Error::internal_error()
|
||||
.data(format!("Failed to add MCP server '{}': {}", name, e)));
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn on_load_session(
|
||||
&self,
|
||||
cx: &JrConnectionCx<AgentToClient>,
|
||||
@@ -875,6 +884,8 @@ impl GooseAcpAgent {
|
||||
sacp::Error::internal_error().data(format!("Failed to set provider: {}", e))
|
||||
})?;
|
||||
|
||||
Self::add_mcp_extensions(&agent, args.mcp_servers, &session_id).await?;
|
||||
|
||||
let conversation = goose_session.conversation.ok_or_else(|| {
|
||||
sacp::Error::internal_error()
|
||||
.data(format!("Session {} has no conversation data", session_id))
|
||||
|
||||
@@ -243,7 +243,7 @@ pub async fn run_load_model<C: Connection>() {
|
||||
assert_eq!(output.text, "2");
|
||||
|
||||
let session_id = session.session_id().0.to_string();
|
||||
let (_, models) = conn.load_session(&session_id).await;
|
||||
let (_, models) = conn.load_session(&session_id, vec![]).await;
|
||||
assert_eq!(&*models.unwrap().current_model_id.0, "o4-mini");
|
||||
}
|
||||
|
||||
@@ -497,6 +497,61 @@ pub async fn run_prompt_image_attachment<C: Connection>() {
|
||||
expected_session_id.assert_matches(&session.session_id().0);
|
||||
}
|
||||
|
||||
pub async fn run_load_session_mcp<C: Connection>() {
|
||||
let expected_session_id = ExpectedSessionId::default();
|
||||
let prompt = "Use the get_code tool and output only its result.";
|
||||
let mcp = McpFixture::new(Some(expected_session_id.clone())).await;
|
||||
let mcp_url = mcp.url.clone();
|
||||
|
||||
// Two rounds of tool call + tool result: one for new session, one for loaded session.
|
||||
let openai = OpenAiFixture::new(
|
||||
vec![
|
||||
(
|
||||
prompt.to_string(),
|
||||
include_str!("../test_data/openai_tool_call.txt"),
|
||||
),
|
||||
(
|
||||
format!(r#""content":"{FAKE_CODE}""#),
|
||||
include_str!("../test_data/openai_tool_result.txt"),
|
||||
),
|
||||
(
|
||||
prompt.to_string(),
|
||||
include_str!("../test_data/openai_tool_call.txt"),
|
||||
),
|
||||
(
|
||||
format!(r#""content":"{FAKE_CODE}""#),
|
||||
include_str!("../test_data/openai_tool_result.txt"),
|
||||
),
|
||||
],
|
||||
expected_session_id.clone(),
|
||||
)
|
||||
.await;
|
||||
|
||||
let mcp_servers = vec![McpServer::Http(McpServerHttp::new("mcp-fixture", &mcp_url))];
|
||||
|
||||
let config = TestConnectionConfig {
|
||||
mcp_servers: mcp_servers.clone(),
|
||||
..Default::default()
|
||||
};
|
||||
let mut conn = C::new(config, openai).await;
|
||||
let (mut session, _) = conn.new_session().await;
|
||||
expected_session_id.set(session.session_id().0.to_string());
|
||||
|
||||
// First prompt: tool should work in the new session.
|
||||
let output = session.prompt(prompt, PermissionDecision::Cancel).await;
|
||||
assert_eq!(output.text, FAKE_CODE, "tool call failed in new session");
|
||||
|
||||
// Load the same session with MCP servers re-specified.
|
||||
let session_id = session.session_id().0.to_string();
|
||||
let (mut loaded_session, _) = conn.load_session(&session_id, mcp_servers).await;
|
||||
|
||||
// Second prompt: tool should work in the loaded session.
|
||||
let output = loaded_session
|
||||
.prompt(prompt, PermissionDecision::Cancel)
|
||||
.await;
|
||||
assert_eq!(output.text, FAKE_CODE, "tool call failed in loaded session");
|
||||
}
|
||||
|
||||
pub async fn run_prompt_mcp<C: Connection>() {
|
||||
let expected_session_id = ExpectedSessionId::default();
|
||||
let mcp = McpFixture::new(Some(expected_session_id.clone())).await;
|
||||
|
||||
+1
@@ -299,6 +299,7 @@ pub trait Connection: Sized {
|
||||
async fn load_session(
|
||||
&mut self,
|
||||
session_id: &str,
|
||||
mcp_servers: Vec<McpServer>,
|
||||
) -> (Self::Session, Option<SessionModelState>);
|
||||
fn auth_methods(&self) -> &[AuthMethod];
|
||||
fn reset_openai(&self);
|
||||
|
||||
+2
-1
@@ -12,7 +12,7 @@ use goose::permission::permission_confirmation::PrincipalType;
|
||||
use goose::permission::{Permission, PermissionConfirmation};
|
||||
use goose::providers::base::Provider;
|
||||
use goose_test_support::TEST_MODEL;
|
||||
use sacp::schema::{AuthMethod, SessionModelState, ToolCallStatus};
|
||||
use sacp::schema::{AuthMethod, McpServer, SessionModelState, ToolCallStatus};
|
||||
use std::sync::Arc;
|
||||
use tokio::sync::Mutex;
|
||||
|
||||
@@ -180,6 +180,7 @@ impl Connection for ClientToProviderConnection {
|
||||
async fn load_session(
|
||||
&mut self,
|
||||
_session_id: &str,
|
||||
_mcp_servers: Vec<McpServer>,
|
||||
) -> (ClientToProviderSession, Option<SessionModelState>) {
|
||||
unimplemented!("TODO: implement load_session in ACP provider")
|
||||
}
|
||||
|
||||
+5
-1
@@ -250,13 +250,17 @@ impl Connection for ClientToAgentConnection {
|
||||
async fn load_session(
|
||||
&mut self,
|
||||
session_id: &str,
|
||||
mcp_servers: Vec<McpServer>,
|
||||
) -> (ClientToAgentSession, Option<SessionModelState>) {
|
||||
self.updates.lock().unwrap().clear();
|
||||
let work_dir = tempfile::tempdir().unwrap();
|
||||
let session_id = sacp::schema::SessionId::new(session_id.to_string());
|
||||
let response = self
|
||||
.cx
|
||||
.send_request(LoadSessionRequest::new(session_id.clone(), work_dir.path()))
|
||||
.send_request(
|
||||
LoadSessionRequest::new(session_id.clone(), work_dir.path())
|
||||
.mcp_servers(mcp_servers),
|
||||
)
|
||||
.block_task()
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
@@ -6,8 +6,9 @@ use common_tests::fixtures::run_test;
|
||||
use common_tests::{
|
||||
run_config_mcp, run_fs_read_text_file_true, run_fs_write_text_file_false,
|
||||
run_fs_write_text_file_true, run_initialize_doesnt_hit_provider, run_load_model,
|
||||
run_model_list, run_model_set, run_permission_persistence, run_prompt_basic,
|
||||
run_prompt_codemode, run_prompt_image, run_prompt_image_attachment, run_prompt_mcp,
|
||||
run_load_session_mcp, run_model_list, run_model_set, run_permission_persistence,
|
||||
run_prompt_basic, run_prompt_codemode, run_prompt_image, run_prompt_image_attachment,
|
||||
run_prompt_mcp,
|
||||
};
|
||||
|
||||
#[test]
|
||||
@@ -43,6 +44,12 @@ fn test_provider_load_model() {
|
||||
run_test(async { run_load_model::<ClientToProviderConnection>().await });
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[ignore = "TODO: implement load_session in ACP provider"]
|
||||
fn test_provider_load_session_mcp() {
|
||||
run_test(async { run_load_session_mcp::<ClientToProviderConnection>().await });
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_provider_model_list() {
|
||||
run_test(async { run_model_list::<ClientToProviderConnection>().await });
|
||||
|
||||
@@ -4,9 +4,9 @@ use common_tests::fixtures::server::ClientToAgentConnection;
|
||||
use common_tests::{
|
||||
run_config_mcp, run_fs_read_text_file_true, run_fs_write_text_file_false,
|
||||
run_fs_write_text_file_true, run_initialize_doesnt_hit_provider,
|
||||
run_initialize_without_provider, run_load_model, run_model_list, run_model_set,
|
||||
run_permission_persistence, run_prompt_basic, run_prompt_codemode, run_prompt_image,
|
||||
run_prompt_image_attachment, run_prompt_mcp,
|
||||
run_initialize_without_provider, run_load_model, run_load_session_mcp, run_model_list,
|
||||
run_model_set, run_permission_persistence, run_prompt_basic, run_prompt_codemode,
|
||||
run_prompt_image, run_prompt_image_attachment, run_prompt_mcp,
|
||||
};
|
||||
|
||||
#[test]
|
||||
@@ -79,6 +79,11 @@ fn test_prompt_image_attachment() {
|
||||
run_test(async { run_prompt_image_attachment::<ClientToAgentConnection>().await });
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_load_session_mcp() {
|
||||
run_test(async { run_load_session_mcp::<ClientToAgentConnection>().await });
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_prompt_mcp() {
|
||||
run_test(async { run_prompt_mcp::<ClientToAgentConnection>().await });
|
||||
|
||||
Reference in New Issue
Block a user