diff --git a/crates/buzz-agent/src/lib.rs b/crates/buzz-agent/src/lib.rs index 8db185ae50..6e4fdbc43b 100644 --- a/crates/buzz-agent/src/lib.rs +++ b/crates/buzz-agent/src/lib.rs @@ -189,7 +189,7 @@ async fn build_agent( ) .await .map_err(|e| AgentError::Llm(format!("session create: {e}")))?; - let session_id = session.id; + let session_id = session.id.clone(); // Provider/model names come from the `GOOSE_*` variables `Config::from_env` // projected out of the `BUZZ_AGENT_*` ones; goose's registry owns base-url @@ -275,11 +275,32 @@ async fn build_agent( // global ones, plugin-installed skills and its own builtins, and supports // `args` templating we never had. // - // Registered by *name* rather than by handing over a client: `skills` is - // in goose's `PLATFORM_EXTENSIONS` table, so `add_extension` runs its - // factory. It is declared `unprefixed_tools: true` there, which is why the - // tool the model sees is plain `load_skill` and not `skills__load_skill`. - let skills = goose::skills::discover_skills(Some(std::path::Path::new(cwd))); + // Registered by handing over a *client* rather than by name, and that is + // the whole point: goose's platform factory + // (`platform_extensions/mod.rs:219-221`) calls plain `SkillsClient::new`, + // which leaves goose's own bundled skills switched on. Those are + // `goose-doc-guide` and `web-search` (`skills/builtins/`) — neither is a + // Buzz skill, and `web-search` advertises a capability Buzz does not + // provide, so every Buzz agent would offer the model a tool-shaped promise + // to shell out to `uvx ddgs`. `with_builtin_skills(false)` is only + // reachable off the constructor, so buzz builds the client itself and + // registers it with `add_client`. + // + // What that route costs: `add_client` does not consult + // `PLATFORM_EXTENSIONS`, so nothing here inherits the table's + // `unprefixed_tools: true`. It does not have to — `is_unprefixed_extension` + // (`extension_manager.rs:392-400`) keys off the *config*, and this passes + // the same `ExtensionConfig::Platform { name: "skills" }` the factory route + // does, so the lookup still hits the table entry and the model still sees a + // bare `load_skill`. Verified by driving both routes: same tool name, same + // filesystem skills, builtins gone. + // + // The prompt index is filtered to match. A skill listed in the index but + // absent from the client is a dead `load_skill` reference. + let skills: Vec<_> = goose::skills::discover_skills(Some(std::path::Path::new(cwd))) + .into_iter() + .filter(|s| s.source_type != goose::custom_requests::SourceType::BuiltinSkill) + .collect(); if !skills.is_empty() { let ext = ExtensionConfig::Platform { name: goose::skills::EXTENSION_NAME.to_string(), @@ -288,15 +309,37 @@ async fn build_agent( bundled: Some(true), available_tools: Vec::new(), }; - if let Err(e) = agent.add_extension(ext, &session_id).await { - // Unlike an MCP server, a missing skills extension is not fatal: - // the agent still has every other tool. Losing it silently would - // be worse than losing it loudly, hence the warning. - tracing::warn!(error = %e, "skills extension unavailable"); - } else { - prompt - .add_extra("skills", crate::skills::skill_index(&skills)) - .await; + // The factory receives a context carrying the session, because + // `SkillsClient::new` reads `session.working_dir` for discovery and + // falls back to the *process* cwd without it — which for a + // desktop-spawned agent is not the nest. + let mut ctx = agent.extension_manager.get_context().clone(); + ctx.extension_manager = Some(Arc::downgrade(&agent.extension_manager)); + ctx.session = Some(Arc::new(session.clone())); + match goose::skills::SkillsClient::new(ctx) { + Ok(client) => { + let client = client.with_builtin_skills(false); + let info = goose::agents::mcp_client::McpClientTrait::get_info(&client).cloned(); + agent + .extension_manager + .add_client( + goose::skills::EXTENSION_NAME.to_string(), + ext, + Arc::new(client), + info, + None, + ) + .await; + prompt + .add_extra("skills", crate::skills::skill_index(&skills)) + .await; + } + Err(e) => { + // Unlike an MCP server, a missing skills extension is not + // fatal: the agent still has every other tool. Losing it + // silently would be worse than losing it loudly. + tracing::warn!(error = %e, "skills extension unavailable"); + } } } diff --git a/crates/buzz-agent/tests/skills.rs b/crates/buzz-agent/tests/skills.rs index 5e270685d3..c865639919 100644 --- a/crates/buzz-agent/tests/skills.rs +++ b/crates/buzz-agent/tests/skills.rs @@ -19,7 +19,17 @@ mod approve; /// Fake OpenAI provider that asks for `load_skill` on the first turn, then /// answers normally. Sends every observed `system` prompt and tool list back. -fn spawn_provider() -> (String, mpsc::Receiver<(String, Vec)>) { +type Observed = (String, Vec, String); + +/// `(system prompt, tool names, whole request body)` for each provider call. +fn spawn_provider() -> (String, mpsc::Receiver) { + spawn_provider_requesting("widget-maker") +} + +/// As [`spawn_provider`], but the first turn asks for `skill_name`. Used to +/// drive `load_skill` at a name the client is expected *not* to know. +fn spawn_provider_requesting(skill_name: &str) -> (String, mpsc::Receiver) { + let skill_name = skill_name.to_string(); let listener = TcpListener::bind("127.0.0.1:0").expect("bind"); let addr = listener.local_addr().expect("addr"); let (tx, rx) = mpsc::channel(); @@ -92,7 +102,7 @@ fn spawn_provider() -> (String, mpsc::Receiver<(String, Vec)>) { .collect() }) .unwrap_or_default(); - let _ = tx.send((system, tools)); + let _ = tx.send((system, tools, req.to_string())); } call_count += 1; @@ -103,7 +113,7 @@ fn spawn_provider() -> (String, mpsc::Receiver<(String, Vec)>) { "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":"load_skill","arguments": format!("{{\"name\":\"{skill_name}\"}}")} }]},"finish_reason":null}] }); let d = json!({ @@ -257,9 +267,11 @@ fn agents_md_and_skill_index_reach_the_model_and_load_skill_resolves() { let mut systems = Vec::new(); let mut tool_lists = Vec::new(); - while let Ok((sys, tools)) = seen.recv_timeout(Duration::from_millis(500)) { + let mut bodies = Vec::new(); + while let Ok((sys, tools, body)) = seen.recv_timeout(Duration::from_millis(500)) { systems.push(sys); tool_lists.push(tools); + bodies.push(body); } assert!(!systems.is_empty(), "provider was never called"); @@ -285,4 +297,83 @@ fn agents_md_and_skill_index_reach_the_model_and_load_skill_resolves() { systems.len() >= 2, "expected a second round after the tool result" ); + + // The turn completing is not evidence the skill loaded. A `load_skill` + // answering "Skill not found." also reaches `end_turn` with a second + // round, so `stopReason` alone passes on a broken skills path. Assert on + // the tool result the model was actually handed. + let second = &bodies[1]; + assert!( + second.contains("Step 2: cut the flange."), + "skill body never reached the model as a tool result:\n{second}" + ); + assert!( + !second.contains("not found"), + "load_skill failed to resolve the skill:\n{second}" + ); +} + +/// goose's `SkillsClient` ships two skills compiled into the crate — +/// `goose-doc-guide` and `web-search` (`goose/src/skills/builtins/`). Neither +/// is a Buzz skill, and `web-search` tells the model to shell out to `uvx +/// ddgs`, a capability Buzz does not provide. buzz-agent on `main` had no such +/// thing, so registering the client with builtins on would have widened every +/// Buzz agent's advertised surface as a side effect of the goose swap. +/// +/// This pins both halves: absent from the prompt index, and unresolvable +/// through the tool. Index-only would pass while the tool still served them. +#[test] +fn goose_builtin_skills_are_not_offered_to_buzz_agents() { + let (base_url, seen) = spawn_provider_requesting("web-search"); + let home = tempfile::tempdir().expect("home"); + let ws = workspace(); + let mut h = Harness::start(&base_url, home.path()); + + h.call("initialize", json!({"protocolVersion": 2})); + let r = h.call( + "session/new", + json!({"cwd": ws.path().to_str().unwrap(), "mcpServers": []}), + ); + let sid = r["result"]["sessionId"] + .as_str() + .unwrap_or_else(|| panic!("session/new failed: {r}")) + .to_string(); + + let r = h.call( + "session/prompt", + json!({"sessionId": sid, "prompt": [{"type":"text","text":"search the web"}]}), + ); + assert_eq!(r["result"]["stopReason"], "end_turn", "turn stalled: {r}"); + + let mut systems = Vec::new(); + let mut bodies = Vec::new(); + while let Ok((sys, _tools, body)) = seen.recv_timeout(Duration::from_millis(500)) { + systems.push(sys); + bodies.push(body); + } + assert!(!systems.is_empty(), "provider was never called"); + + let first = &systems[0]; + for builtin in ["web-search", "goose-doc-guide"] { + assert!( + !first.contains(builtin), + "goose builtin {builtin:?} is advertised in the system prompt:\n{first}" + ); + } + // The filesystem skill still has to be there — an empty index would pass + // the assertions above for the wrong reason. + assert!( + first.contains("widget-maker"), + "filesystem skills were lost along with the builtins:\n{first}" + ); + + assert!( + bodies.len() >= 2, + "expected a second round carrying the tool result" + ); + let second = &bodies[1]; + assert!( + second.contains("not found"), + "load_skill served a goose builtin:\n{second}" + ); }