diff --git a/crates/buzz-agent/src/agent.rs b/crates/buzz-agent/src/agent.rs index ca618a7421..4af2bdb90b 100644 --- a/crates/buzz-agent/src/agent.rs +++ b/crates/buzz-agent/src/agent.rs @@ -20,9 +20,9 @@ //! * **`builtin.rs` / `load_skill`.** `Agent::with_config` loads zero //! extensions and `build_agent` only adds the `mcpServers` the harness //! declares, so goose's `skills` platform extension is never loaded. Instead -//! `build_agent` registers `load_skill` as a goose *frontend* extension and -//! [`serve_frontend_tool`] answers the calls in-process (goose yields a -//! `FrontendToolRequest` and blocks the stream until we respond). +//! `build_agent` registers `load_skill` as a goose *platform* extension +//! backed by an `McpClientTrait` (see [`crate::builtin_client`]), so goose +//! dispatches it on its ordinary tool path. //! * **`AGENTS.md` hints / skill index.** Goose's loader keys off //! `GOOSE_HINTS_FILENAME` (`.goosehints`); the old code walked the directory //! chain for `AGENTS.md` plus `~/AGENTS.md`. Every repo here ships the @@ -140,7 +140,6 @@ pub async fn run_turn( wire_tx: &WireSender, cancel: CancellationToken, hook_extension: Option<&str>, - skills: &[crate::hints::SkillEntry], ) -> (Result, TurnTokens) { let mut tokens = TurnTokens::default(); let mut next_message = Message::user().with_text(prompt_to_text(&prompt)); @@ -161,7 +160,6 @@ pub async fn run_turn( &cancel, &mut tokens, hook_extension, - skills, ) .await { @@ -226,7 +224,6 @@ async fn drive_stream( cancel: &CancellationToken, tokens: &mut TurnTokens, hook_extension: Option<&str>, - skills: &[crate::hints::SkillEntry], ) -> Result { let session_config = SessionConfig { id: session_id.to_string(), @@ -308,7 +305,6 @@ async fn drive_stream( &mut compacted, &mut reflections, &mut open_tool_calls, - skills, ) .await { @@ -397,7 +393,6 @@ async fn handle_event( compacted: &mut bool, reflections: &mut usize, open_tool_calls: &mut Vec, - skills: &[crate::hints::SkillEntry], ) -> Option { match event { AgentEvent::Message(msg) => { @@ -408,17 +403,6 @@ async fn handle_event( open_tool_calls.push(req.id.clone()); } - // Frontend tool calls (`load_skill`) never arrive as - // `ToolRequest`: goose strips them from the normal flow - // (`reply_parts.rs` `categorize_tools`) and yields this - // dedicated variant instead, then blocks the reply stream on - // `tool_result_rx` until we answer. Answer it or the turn - // hangs forever. - if let MessageContent::FrontendToolRequest(req) = content { - open_tool_calls.push(req.id.clone()); - serve_frontend_tool(req, agent, skills).await; - } - if let MessageContent::ToolResponse(resp) = content { open_tool_calls.retain(|id| id != &resp.id); @@ -585,68 +569,6 @@ async fn emit_content(content: &MessageContent, session_id: &str, wire_tx: &Wire } } -/// Serve a goose *frontend* tool call. -/// -/// Goose never dispatches frontend tools itself. `handle_frontend_tool_request` -/// (`goose/src/agents/tool_execution.rs:175`) yields the -/// `FrontendToolRequest` message we are handling right now and then **blocks -/// the reply stream** on `tool_result_rx.recv()` until the embedder calls -/// `agent.handle_tool_result(id, ..)`. That has two consequences: -/// -/// * every `FrontendToolRequest` event MUST be answered — returning without -/// sending a result hangs the turn forever, so even an unrecognised tool -/// name gets an error result rather than silence; -/// * exactly one answer per event — a stray extra send would be picked up by -/// the recv of the *next* frontend call and mis-pair results. -async fn serve_frontend_tool( - req: &goose_provider_types::conversation::message::FrontendToolRequest, - agent: &Arc, - skills: &[crate::hints::SkillEntry], -) { - let result = match &req.tool_call { - Ok(call) if call.name == crate::builtin::LOAD_SKILL_TOOL => { - let args = call - .arguments - .clone() - .map(serde_json::Value::Object) - .unwrap_or(serde_json::Value::Null); - crate::builtin::call_load_skill(&args, skills).await - } - // Only `load_skill` is registered, so this arm is unreachable today — - // but goose is already blocked waiting for this id, so answer anyway. - Ok(call) => crate::builtin::error_result(&format!( - "unknown frontend tool {:?}; only {} is served in-process", - call.name, - crate::builtin::LOAD_SKILL_TOOL - )), - // An Err here means goose did NOT block on the result channel — its - // yield only happens inside the `Ok` branch (`tool_execution.rs:181`). - // Sending would leave a stray result in the queue that mis-pairs the - // NEXT frontend call (the recv trusts the id coming off the channel). - Err(e) => { - tracing::warn!("unparseable frontend tool call {}: {e}", req.id); - return; - } - }; - - let content: Vec = result - .content - .iter() - .map(|c| rmcp::model::Content::text(c.as_text_lossy())) - .collect(); - - agent - .handle_tool_result( - req.id.clone(), - Ok(if result.is_error { - rmcp::model::CallToolResult::error(content) - } else { - rmcp::model::CallToolResult::success(content) - }), - ) - .await; -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/buzz-agent/src/builtin_client.rs b/crates/buzz-agent/src/builtin_client.rs new file mode 100644 index 0000000000..0324d3e480 --- /dev/null +++ b/crates/buzz-agent/src/builtin_client.rs @@ -0,0 +1,119 @@ +//! Buzz's in-process tools, exposed to Goose as a platform extension. +//! +//! Goose has two ways to take a tool the embedder implements in Rust: +//! +//! * [`ExtensionConfig::Frontend`] — Goose *advertises* the tool but refuses to +//! run it. It returns "Frontend tool execution required" and blocks until the +//! embedder calls `handle_tool_result`. Dispatch is strictly sequential, has +//! no timeout, and the result channel carries no request id, so a single +//! missed result wedges the agent for the rest of the session and the cancel +//! token will not free it. +//! * [`ExtensionConfig::Platform`] + `extension_manager.add_client` with an +//! [`McpClientTrait`] — Goose dispatches it through the ordinary tool path, +//! so concurrency, per-request timeouts and `notifications/cancelled` all +//! work, and there is nothing for us to correlate. +//! +//! This uses the second. It is the shape Maple settled on for its own +//! in-process developer and skills tools. +//! +//! The only tool here is `load_skill`. The system prompt carries the skill +//! *index* (names and descriptions, from [`crate::hints`]); bodies are pulled +//! on demand so a large skill library costs nothing per turn. + +use std::sync::Arc; + +use async_trait::async_trait; +use goose::agents::mcp_client::McpClientTrait; +use goose::agents::{ExtensionConfig, ToolCallContext}; +use rmcp::model::{CallToolResult, Content, InitializeResult, JsonObject, ListToolsResult, Tool}; +use rmcp::service::ServiceError as Error; +use tokio_util::sync::CancellationToken; + +use crate::hints::SkillEntry; + +/// Extension key and display name. Goose prefixes tool names with the +/// extension name (`{extension}__{tool}`) unless the extension is registered +/// as unprefixed, so this shows up in the model's tool list. +pub const BUILTIN_EXTENSION_NAME: &str = "buzz"; + +/// Goose-side declaration for the extension backed by [`BuiltinClient`]. +pub fn builtin_extension_config() -> ExtensionConfig { + ExtensionConfig::Platform { + name: BUILTIN_EXTENSION_NAME.to_string(), + description: "Buzz built-in tools".to_string(), + display_name: Some("Buzz".to_string()), + bundled: Some(true), + available_tools: vec![crate::builtin::LOAD_SKILL_TOOL.to_string()], + } +} + +/// In-process MCP client serving [`crate::builtin`]'s tools. +pub struct BuiltinClient { + skills: Vec, +} + +impl BuiltinClient { + pub fn new(skills: Vec) -> Self { + Self { skills } + } + + fn load_skill_tool() -> Tool { + let def = crate::builtin::load_skill_def(); + let schema = match def.input_schema { + serde_json::Value::Object(map) => Arc::new(map), + _ => Arc::new(serde_json::Map::new()), + }; + Tool::new(def.name, def.description, schema) + } +} + +#[async_trait] +impl McpClientTrait for BuiltinClient { + async fn list_tools( + &self, + _session_id: &str, + _next_cursor: Option, + _cancel_token: CancellationToken, + ) -> Result { + Ok(ListToolsResult { + tools: vec![Self::load_skill_tool()], + next_cursor: None, + meta: None, + }) + } + + async fn call_tool( + &self, + _ctx: &ToolCallContext, + name: &str, + arguments: Option, + _cancel_token: CancellationToken, + ) -> Result { + if name != crate::builtin::LOAD_SKILL_TOOL { + return Ok(CallToolResult::error(vec![Content::text(format!( + "unknown tool: {name}" + ))])); + } + + let args = arguments + .map(serde_json::Value::Object) + .unwrap_or(serde_json::Value::Null); + let result = crate::builtin::call_load_skill(&args, &self.skills).await; + + let content: Vec = result + .content + .iter() + .map(|c| Content::text(c.as_text_lossy())) + .collect(); + + Ok(if result.is_error { + CallToolResult::error(content) + } else { + CallToolResult::success(content) + }) + } + + fn get_info(&self) -> Option<&InitializeResult> { + None + } +} diff --git a/crates/buzz-agent/src/hints.rs b/crates/buzz-agent/src/hints.rs index 90833aa3db..cedb5872af 100644 --- a/crates/buzz-agent/src/hints.rs +++ b/crates/buzz-agent/src/hints.rs @@ -241,9 +241,15 @@ fn build_hints_section_impl(cwd: &Path, home: Option<&Path>) -> (String, Vec, - /// Skills discovered at session start; the bodies `load_skill` serves. - skills: Vec, /// Name of the MCP extension carrying `_Stop`/`_PostCompact`, if any. /// See [`crate::hooks`] for why we dispatch these ourselves. hook_extension: Option, @@ -97,15 +96,7 @@ async fn build_agent( cwd: &str, system_prompt: Option<&str>, mcp_servers: &[McpServerStdio], -) -> Result< - ( - Arc, - String, - Option, - Vec, - ), - AgentError, -> { +) -> Result<(Arc, String, Option), AgentError> { let session_manager = Arc::new(SessionManager::instance()); let permission_manager = PermissionManager::instance(); @@ -201,32 +192,24 @@ async fn build_agent( .await; } - // `load_skill` runs in-process. The prompt carries only skill names and - // descriptions; the body is pulled on demand, which is the whole point of - // the index above. Registered as a goose *frontend* tool: goose advertises - // it, then hands the call back to us rather than dispatching it itself - // (`agent.rs:1167`). + // `load_skill` runs in-process, registered the way Maple does it: a + // platform extension backed by an `McpClientTrait`, NOT + // `ExtensionConfig::Frontend`. Goose advertises frontend tools but refuses + // to dispatch them -- it blocks on an uncorrelated result channel until the + // embedder calls `handle_tool_result`, with no timeout and no cancellation, + // so one missed result wedges the session. A platform client goes through + // goose's ordinary tool path instead. See `crate::builtin_client`. if !skills.is_empty() { - let def = crate::builtin::load_skill_def(); - let tool = rmcp::model::Tool::new( - def.name.clone(), - def.description.clone(), - match def.input_schema { - serde_json::Value::Object(map) => std::sync::Arc::new(map), - _ => std::sync::Arc::new(serde_json::Map::new()), - }, - ); - let ext = ExtensionConfig::Frontend { - name: "buzz_builtin".to_string(), - description: String::new(), - tools: vec![tool], - instructions: None, - bundled: None, - available_tools: Vec::new(), - }; - if let Err(e) = agent.add_extension(ext, &session.id).await { - tracing::warn!("load_skill unavailable: {e}"); - } + agent + .extension_manager + .add_client( + crate::builtin_client::BUILTIN_EXTENSION_NAME.to_string(), + crate::builtin_client::builtin_extension_config(), + std::sync::Arc::new(crate::builtin_client::BuiltinClient::new(skills)), + None, + None, + ) + .await; } let agent = Arc::new(agent); @@ -259,7 +242,7 @@ async fn build_agent( tracing::info!(extension = %ext, "lifecycle hooks available"); } - Ok((agent, session.id, hook_extension, skills)) + Ok((agent, session.id, hook_extension)) } /// Keep the model's hands off the lifecycle hooks. @@ -472,7 +455,7 @@ async fn session_new(app: &Arc, id: Value, params: Value, wire_tx: &WireSen .await; } - let (agent, goose_session_id, hook_extension, skills) = + let (agent, goose_session_id, hook_extension) = match build_agent(&app.cfg, &p.cwd, p.system_prompt.as_deref(), &p.mcp_servers).await { Ok(v) => v, Err(e) => { @@ -514,7 +497,6 @@ async fn session_new(app: &Arc, id: Value, params: Value, wire_tx: &WireSen model_override: None, pending_model: None, hook_extension, - skills, accumulated_input_tokens: 0, accumulated_output_tokens: 0, }, @@ -609,7 +591,7 @@ async fn session_prompt(app: &Arc, id: Value, params: Value, wire_tx: &Wire let cancel = CancellationToken::new(); // Single-flight per session, and capture the agent handle. - let (agent, goose_session_id, pending_model, hook_extension, skills) = { + let (agent, goose_session_id, pending_model, hook_extension) = { let mut sessions = app.sessions.lock().await; let Some(s) = sessions.get_mut(&p.session_id) else { return wire::send( @@ -639,7 +621,6 @@ async fn session_prompt(app: &Arc, id: Value, params: Value, wire_tx: &Wire s.goose_session_id.clone(), s.pending_model.take(), s.hook_extension.clone(), - s.skills.clone(), ) }; @@ -678,7 +659,6 @@ async fn session_prompt(app: &Arc, id: Value, params: Value, wire_tx: &Wire wire_tx, cancel, hook_extension.as_deref(), - &skills, ) .await; diff --git a/crates/buzz-agent/tests/skills.rs b/crates/buzz-agent/tests/skills.rs index af792cbbb6..2de2a91dce 100644 --- a/crates/buzz-agent/tests/skills.rs +++ b/crates/buzz-agent/tests/skills.rs @@ -1,12 +1,10 @@ //! Proves `load_skill` and `AGENTS.md` hints survive the goose swap. //! //! Both were briefly lost in the port. `load_skill` is registered as a goose -//! *frontend* tool, which means goose advertises it to the model but refuses to -//! dispatch it — it returns "Frontend tool execution required" and waits for -//! the embedder to call `handle_tool_result` -//! (`goose/src/agents/agent.rs:1167`). If that wiring is wrong the turn does -//! not fail, it **hangs forever**, so the timeout in `await_response` is the -//! real assertion here. +//! *platform* extension backed by an `McpClientTrait` (the shape Maple uses), +//! so goose dispatches it on its ordinary tool path. Goose namespaces such +//! tools as `{extension}__{tool}`, which the system prompt has to match — a +//! prompt naming a tool that is not in the tool list is a silent failure. use std::io::{BufRead, BufReader, Write}; use std::net::TcpListener; @@ -96,13 +94,13 @@ fn spawn_provider() -> (String, mpsc::Receiver<(String, Vec)>) { call_count += 1; let sse = if call_count == 1 { - // Ask for the skill. If our frontend-tool plumbing is broken, + // Ask for the skill by its namespaced name. // goose never resolves this and the turn hangs. let c = json!({ "id":"c","object":"chat.completion.chunk","created":1,"model":"fake-model", "choices":[{"index":0,"delta":{"role":"assistant","tool_calls":[{ "index":0,"id":"call_1","type":"function", - "function":{"name":"load_skill","arguments":"{\"name\":\"widget-maker\"}"} + "function":{"name":"buzz__load_skill","arguments":"{\"name\":\"widget-maker\"}"} }]},"finish_reason":null}] }); let d = json!({ @@ -233,7 +231,7 @@ fn agents_md_and_skill_index_reach_the_model_and_load_skill_resolves() { .unwrap_or_else(|| panic!("session/new failed: {r}")) .to_string(); - // The turn only returns if load_skill was served. A broken frontend-tool + // The turn only returns if load_skill was dispatched. A broken tool // path hangs here rather than erroring. let r = h.call( "session/prompt", @@ -266,7 +264,7 @@ fn agents_md_and_skill_index_reach_the_model_and_load_skill_resolves() { "skill BODY was inlined — the point of load_skill is that it is not:\n{first}" ); assert!( - tool_lists[0].iter().any(|t| t == "load_skill"), + tool_lists[0].iter().any(|t| t == "buzz__load_skill"), "load_skill not advertised: {:?}", tool_lists[0] );