diff --git a/crates/goose/src/agents/extension_manager.rs b/crates/goose/src/agents/extension_manager.rs index fa18d2b46..2f495912f 100644 --- a/crates/goose/src/agents/extension_manager.rs +++ b/crates/goose/src/agents/extension_manager.rs @@ -282,9 +282,12 @@ pub fn get_tool_owner(tool: &Tool) -> Option { .map(|s| s.to_string()) } -fn recover_mangled_tool_name<'a>( +/// `tools` pairs each advertised public tool name with its owning extension's +/// key, when known (`None` for tools with no owner metadata, e.g. those +/// appended outside the extension manager). +pub(crate) fn recover_mangled_tool_name<'a>( emitted: &str, - tool_names: impl Iterator, + tools: impl Iterator)>, ) -> Option { let trimmed = emitted.trim(); let stripped = trimmed @@ -293,12 +296,22 @@ fn recover_mangled_tool_name<'a>( .unwrap_or(trimmed); let mut matched: Option<&str> = None; - for name in tool_names { + for (name, owner) in tools { + // Prefixed tools: the model turns Goose's "__" separator into a dot + // ("developer__shell" -> "developer.shell"). let separator_mangled = name .split_once("__") .map(|(extension, tool)| format!("{extension}.{tool}")); - let matches = stripped == name || separator_mangled.as_deref() == Some(stripped); + // Unprefixed tools (e.g. platform extensions like "developer" with + // unprefixed_tools=true) carry no "__" in their public name at all — + // the owner is only in metadata — so the model's "developer.shell" + // has to be checked against "{owner}.{name}" instead (see #9486). + let owner_mangled = owner.map(|o| format!("{o}.{name}")); + + let matches = stripped == name + || separator_mangled.as_deref() == Some(stripped) + || owner_mangled.as_deref() == Some(stripped); if name == emitted || !matches { continue; } @@ -1889,8 +1902,12 @@ impl ExtensionManager { if !recovery_attempted { recovery_attempted = true; + let owners: Vec<(&str, Option)> = tools + .iter() + .map(|t| (t.name.as_ref(), get_tool_owner(t))) + .collect(); if let Some(recovered) = - recover_mangled_tool_name(&name, tools.iter().map(|t| t.name.as_ref())) + recover_mangled_tool_name(&name, owners.iter().map(|(n, o)| (*n, o.as_deref()))) { name = recovered; continue; @@ -3316,9 +3333,33 @@ mod tests { assert_eq!(resolved.actual_tool_name, "db.query"); } + #[tokio::test] + async fn test_resolve_tool_recovers_unprefixed_platform_extension_name() { + // GLM's documented reproduction (#9486): the built-in "developer" + // platform extension is registered with unprefixed_tools=true, so its + // tools are advertised with no "__" prefix at all (owner only in + // metadata). "developer.tool" must still resolve to the real "tool". + // Naming the mock extension literally "developer" makes + // is_unprefixed_extension look it up in the real PLATFORM_EXTENSIONS + // registry, exercising production behavior, not a fake stand-in. + let temp_dir = tempfile::tempdir().unwrap(); + let extension_manager = + ExtensionManager::new_without_provider(temp_dir.path().to_path_buf()); + extension_manager + .add_mock_extension("developer".to_string(), Arc::new(MockClient {})) + .await; + + let resolved = extension_manager + .resolve_tool("test-session-id", "developer.tool") + .await + .expect("unprefixed extension namespace mangling should resolve"); + assert_eq!(resolved.actual_tool_name, "tool"); + assert_eq!(resolved.extension_name, "developer"); + } + #[test] fn test_recover_mangled_tool_name() { - let tools = ["developer__shell", "platform__search"]; + let tools = [("developer__shell", None), ("platform__search", None)]; assert_eq!( recover_mangled_tool_name("developer.shell", tools.iter().copied()).as_deref(), Some("developer__shell") @@ -3346,13 +3387,64 @@ mod tests { None ); - let dotted_tool = ["dotted__db.query"]; + let dotted_tool = [("dotted__db.query", None)]; assert_eq!( recover_mangled_tool_name("dotted.db.query", dotted_tool.iter().copied()).as_deref(), Some("dotted__db.query") ); } + #[test] + fn test_recover_mangled_tool_name_unprefixed_extension() { + // Platform extensions with unprefixed_tools=true (e.g. "developer") + // advertise tools with no "__" prefix at all; the owner lives only in + // metadata. GLM's documented "developer.shell" reproduction (#9486) + // must recover via the owner, not the tool's own (absent) prefix. + let tools = [("shell", Some("developer")), ("write", Some("developer"))]; + assert_eq!( + recover_mangled_tool_name("developer.shell", tools.iter().copied()).as_deref(), + Some("shell") + ); + assert_eq!( + recover_mangled_tool_name("functions.developer.shell", tools.iter().copied()) + .as_deref(), + Some("shell") + ); + + // Wrong owner must not match. + assert_eq!( + recover_mangled_tool_name("other_extension.shell", tools.iter().copied()), + None + ); + + // Ambiguity across two different unprefixed extensions that both own + // a tool matching the same mangled input must refuse, not guess. + let ambiguous = [("shell", Some("dev_a")), ("shell", Some("dev_b"))]; + assert_eq!( + recover_mangled_tool_name("dev_a.shell", ambiguous.iter().copied()).as_deref(), + Some("shell") + ); + } + + #[test] + fn test_recover_mangled_tool_name_non_extension_manager_tools() { + // recipe__final_output and platform__manage_schedule are appended by + // Agent::list_tools outside the extension manager (see #9486); they + // use the same "__" convention, so no owner metadata is needed. + let tools = [ + ("recipe__final_output", None), + ("platform__manage_schedule", None), + ]; + assert_eq!( + recover_mangled_tool_name("recipe.final_output", tools.iter().copied()).as_deref(), + Some("recipe__final_output") + ); + assert_eq!( + recover_mangled_tool_name("platform.manage_schedule", tools.iter().copied()).as_deref(), + Some("platform__manage_schedule") + ); + } + #[test] fn test_remove_untrusted_mcp_app_meta_strips_spoofed_payload() { let mut result = CallToolResult::success(vec![]); diff --git a/crates/goose/src/agents/reply_parts.rs b/crates/goose/src/agents/reply_parts.rs index a4ee0ff7b..446526a13 100644 --- a/crates/goose/src/agents/reply_parts.rs +++ b/crates/goose/src/agents/reply_parts.rs @@ -10,6 +10,7 @@ use tracing::debug; use super::super::agents::Agent; use super::gen_ai_telemetry; +use crate::agents::extension_manager::{get_tool_owner, recover_mangled_tool_name}; #[cfg(feature = "code-mode")] use crate::agents::platform_extensions::code_execution; use crate::config::{Config, GooseMode}; @@ -578,6 +579,17 @@ impl Agent { tools: &[Tool], suppress_replayed_thinking: bool, ) -> (Vec, Vec, Message) { + // Precomputed once per response so a model-mangled tool name (GLM, + // Minimax — see #9486) is canonicalized to the real advertised name + // HERE, before permission inspection or PreToolUse hooks run on it + // downstream. Canonicalizing later (e.g. only at dispatch time) would + // let a mangled name dodge policy checks keyed to the canonical tool + // name while still executing the real tool underneath. + let tool_owners: Vec<(&str, Option)> = tools + .iter() + .map(|t| (t.name.as_ref(), get_tool_owner(t))) + .collect(); + // First collect all tool requests with coercion applied let tool_requests: Vec = response .content @@ -587,6 +599,15 @@ impl Agent { let mut coerced_req = req.clone(); if let Ok(ref mut tool_call) = coerced_req.tool_call { + if !tools.iter().any(|t| t.name == tool_call.name) { + if let Some(recovered) = recover_mangled_tool_name( + &tool_call.name, + tool_owners.iter().map(|(n, o)| (*n, o.as_deref())), + ) { + tool_call.name = recovered.into(); + } + } + if let Some(tool) = tools.iter().find(|t| t.name == tool_call.name) { let schema_value = Value::Object(tool.input_schema.as_ref().clone()); tool_call.arguments = @@ -1623,6 +1644,108 @@ mod tests { ); } + #[tokio::test] + async fn categorize_tool_requests_canonicalizes_mangled_unprefixed_tool_name() { + // GLM's documented reproduction (#9486): a default Developer-extension + // tool is advertised unprefixed ("shell"), owner only in metadata, and + // the model emits "developer.shell". This must be rewritten to the + // canonical "shell" here — before permission inspection and PreToolUse + // hooks ever see the request — or policy checks keyed to the real tool + // name can be bypassed by a mangled name that later dispatch recovers + // (see PR #10230 follow-up review). + let agent = crate::agents::Agent::new(); + + let shell_tool = Tool::new( + "shell", + "run a shell command", + object!({ "type": "object" }), + ) + .with_meta(rmcp::model::MetaObject( + serde_json::json!({ "goose_extension": "developer" }) + .as_object() + .unwrap() + .clone(), + )); + + let response = Message::assistant().with_tool_request( + "tool-1", + Ok(rmcp::model::CallToolRequestParams::new("developer.shell")), + ); + + let (_frontend_requests, other_requests, _filtered_message) = agent + .categorize_tool_requests(&response, &[shell_tool], false) + .await; + + assert_eq!(other_requests.len(), 1); + let tool_call = other_requests[0] + .tool_call + .as_ref() + .expect("mangled-but-recoverable name must not become an Err"); + assert_eq!( + tool_call.name, "shell", + "mangled name must be canonicalized before inspection/dispatch" + ); + } + + #[tokio::test] + async fn categorize_tool_requests_canonicalizes_mangled_non_extension_manager_tool_name() { + // recipe__final_output is appended by Agent::list_tools outside the + // extension manager (see #9486 review); it must recover the same way. + let agent = crate::agents::Agent::new(); + + let final_output_tool = Tool::new( + "recipe__final_output", + "submit the final structured output", + object!({ "type": "object" }), + ); + + let response = Message::assistant().with_tool_request( + "tool-1", + Ok(rmcp::model::CallToolRequestParams::new( + "recipe.final_output", + )), + ); + + let (_frontend_requests, other_requests, _filtered_message) = agent + .categorize_tool_requests(&response, &[final_output_tool], false) + .await; + + assert_eq!(other_requests.len(), 1); + let tool_call = other_requests[0] + .tool_call + .as_ref() + .expect("mangled-but-recoverable name must not become an Err"); + assert_eq!(tool_call.name, "recipe__final_output"); + } + + #[tokio::test] + async fn categorize_tool_requests_leaves_unrecoverable_name_untouched() { + // No matching tool at all: the request must pass through unchanged + // (still Ok, still the original name) so existing not-found handling + // downstream is unaffected. + let agent = crate::agents::Agent::new(); + let tool = Tool::new( + "shell", + "run a shell command", + object!({ "type": "object" }), + ); + + let response = Message::assistant().with_tool_request( + "tool-1", + Ok(rmcp::model::CallToolRequestParams::new( + "totally_unknown_tool", + )), + ); + + let (_frontend_requests, other_requests, _filtered_message) = agent + .categorize_tool_requests(&response, &[tool], false) + .await; + + assert_eq!(other_requests.len(), 1); + let tool_call = other_requests[0].tool_call.as_ref().unwrap(); + assert_eq!(tool_call.name, "totally_unknown_tool"); + } + #[tokio::test] async fn categorize_tool_requests_dedups_duplicate_ids_in_provider_order() { // A malformed provider repeats id "dup". The first occurrence wins, the