tkmind/tkmind_go
1
0
Fork 0
Code Issues Pull Requests Actions 5 Packages Projects Releases Wiki Activity

fix: canonicalize mangled tool names before permission inspection (follow-up to #10230) (#10285)

Browse Source
This commit is contained in:
Herley
2026-08-19 22:15:03 +00:00
committed by GitHub
parent 5e6b79fb96
commit f2e6e9ed05
2 changed files with 222 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/goose/src/agents/extension_manager.rs
+99 -7
View File
@@ -282,9 +282,12 @@ pub fn get_tool_owner(tool: &Tool) -> Option<String> {
.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<Item = &'a str>,
tools: impl Iterator<Item = (&'a str, Option<&'a str>)>,
) -> Option<String> {
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<String>)> = 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![]);
crates/goose/src/agents/reply_parts.rs
+123
View File
@@ -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<ToolRequest>, Vec<ToolRequest>, 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<String>)> = 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<ToolRequest> = 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