fix(buzz-agent): keep goose's bundled skills out of Buzz agents

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");
+            }
         }
     }
 

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<String>)>) {
+type Observed = (String, Vec<String>, String);
+
+/// `(system prompt, tool names, whole request body)` for each provider call.
+fn spawn_provider() -> (String, mpsc::Receiver<Observed>) {
+    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<Observed>) {
+    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<String>)>) {
                             .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<String>)>) {
                     "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}"
+    );
 }