Skip to content

fix(code_execution): handle model quirks with tool calls #6352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alexhancock merged 1 commit into from
Jan 13, 2026
Merged

fix(code_execution): handle model quirks with tool calls #6352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions crates/goose/src/agents/code_execution_extension.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,8 @@ impl CodeExecutionClient {
- WRONG: Multiple execute_code calls, each with one tool
- RIGHT: One execute_code call with a script that calls all needed tools

IMPORTANT: All tool calls are SYNCHRONOUS. Do NOT use async/await.
Copy link
Collaborator

@michaelneale michaelneale Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrm - I guess this is true - are we sure?
Also, I wonder if there is some other way to enforce this than prompting, but I think this is is ok @alexhancock

Copy link
Collaborator

@alexhancock alexhancock Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this part of the code @rabi . Isn't the builtin function we provide async, and therefor don't any calls the model writes to functions which call a tool need to be awaited?

Copy link
Contributor Author

@rabi rabi Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems tools are exposed to the model as synchronous JavaScript functions via the Boa JS engine. create_tool_function wraps each tool using blocking_recv() which blocks until the async Rust handler responds. So, model-generated code I think should use const x = tool({...}) not await tool({...}).

Copy link
Collaborator

@alexhancock alexhancock Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense @rabi. I had misremembered. Merged


Workflow:
1. Use the read_module tool to discover tools and signatures
2. Write ONE script that imports and calls ALL tools needed for the task
Expand Down Expand Up @@ -572,12 +574,16 @@ impl CodeExecutionClient {
.and_then(|a| a.get("terms"))
.ok_or("Missing required parameter: terms")?;

let terms_vec = if let Some(s) = terms.as_str() {
vec![s.to_string()]
} else if let Some(arr) = terms.as_array() {
let terms_vec = if let Some(arr) = terms.as_array() {
arr.iter()
.filter_map(|v| v.as_str().map(String::from))
.collect()
} else if let Some(s) = terms.as_str() {
if s.starts_with('[') && s.ends_with(']') {
serde_json::from_str::<Vec<String>>(s).unwrap_or_else(|_| vec![s.to_string()])
} else {
vec![s.to_string()]
}
} else {
return Err("Parameter 'terms' must be a string or array of strings".to_string());
};
Expand Down Expand Up @@ -830,9 +836,11 @@ impl McpClientTrait for CodeExecutionClient {
Search for tools by name or description across all available modules.

USAGE:
- Single term: search_modules with terms="file"
- Multiple terms: search_modules with terms=["git", "shell"]
- Regex patterns: search_modules with terms="sh.*", regex=true
- Single term: terms="github" (just a plain string)
- Multiple terms: terms=["git", "shell"] (a JSON array, NOT a string)
- Regex patterns: terms="sh.*", regex=true

IMPORTANT: Do NOT stringify arrays. Use terms=["a","b"] not terms="[\"a\",\"b\"]"
Copy link
Collaborator

@michaelneale michaelneale Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have found saying NOT can use up attention and hurt performance, wonder if we need that so explicitly (if there is a way to feedback errors then that can help)


Returns matching servers and tools with descriptions.
Use this when you don't know which module contains the tool you need.
Expand Down
21 changes: 17 additions & 4 deletions crates/goose/src/agents/extension_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,17 +1022,30 @@ impl ExtensionManager {
tool_call: CallToolRequestParam,
cancellation_token: CancellationToken,
) -> Result<ToolCallResult> {
// Some models strip the tool prefix, so auto-add it for known code_execution tools
Copy link
Collaborator

@michaelneale michaelneale Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this special casing for code execution as it needs to prefix it, but in other cases - they work?

Copy link
Contributor Author

@rabi rabi Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think code_execution tools (execute_code, read_module, search_modules) are the only tools the model calls directly when code_execution extension is enabled. Model is expected to call the tools with the extension prefix, but I suspect when all visible tools share the same prefix and the instruction is to use without prefix, some models strip them.

let tool_name_str = tool_call.name.to_string();
let prefixed_name = if !tool_name_str.contains("__") {
let code_exec_tools = ["execute_code", "read_module", "search_modules"];
if code_exec_tools.contains(&tool_name_str.as_str())
&& self.extensions.lock().await.contains_key("code_execution")
{
format!("code_execution__{}", tool_name_str)
} else {
tool_name_str
}
} else {
tool_name_str
};

// Dispatch tool call based on the prefix naming convention
let (client_name, client) =
self.get_client_for_tool(&tool_call.name)
self.get_client_for_tool(&prefixed_name)
.await
.ok_or_else(|| {
ErrorData::new(ErrorCode::RESOURCE_NOT_FOUND, tool_call.name.clone(), None)
})?;

// rsplit returns the iterator in reverse, tool_name is then at 0
let tool_name = tool_call
.name
let tool_name = prefixed_name
.strip_prefix(client_name.as_str())
.and_then(|s| s.strip_prefix("__"))
.ok_or_else(|| {
Expand Down