fix: canonicalize mangled tool names before permission inspection (follow-up to #10230) - #10285
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5f3267363
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let tool_owners: Vec<(&str, Option<String>)> = tools | ||
| .iter() | ||
| .map(|t| (t.name.as_ref(), get_tool_owner(t))) | ||
| .collect(); |
There was a problem hiding this comment.
Because tools here is the provider-visible list after prepare_tools_and_prompt filters out tools that are not is_tool_visible_to_model, app-only MCP tools are missing from tool_owners. If a provider emits a mangled call for one of those tools (for example db.drop_table while policy/hooks deny the canonical db__drop_table), this block leaves the raw name unchanged, but ExtensionManager::resolve_tool later recovers against the unfiltered cached extension tools and can execute it. That leaves the permission/hook bypass in place for hidden/app-only tools; canonicalization needs to use the same dispatchable tool set, or dispatch recovery must not run for tools that were not inspected.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale because it has not had recent activity for 23 days. What happens next?
Thank you for your contribution! 🚀 |
b5f3267 to
8b66c08
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b66c0870e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| 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(); |
There was a problem hiding this comment.
When GOOSE_STATE_MACHINE=1, responses bypass categorize_tool_requests: ops_llm.rs persists provider tool requests unchanged, and ops_toolcalling.rs later retains only exact matches from known_tools. Consequently, the documented developer.shell response is reported as unavailable instead of becoming shell, so this fix works only in the legacy loop. Apply equivalent canonicalization before state-machine approval and dispatch, with coverage for that path.
AGENTS.md reference: AGENTS.md:L21-L23
Useful? React with 👍 / 👎.
…t at dispatch PR aaif-goose#10230 recovered model-mangled tool names (GLM/Minimax) only inside ExtensionManager::resolve_tool, deep in the dispatch path. PermissionInspector and PreToolUse hooks run earlier, on the raw ToolRequest, and check policy (never_allow/ask_before) against tool_call.name directly. A mangled name that recovery later maps to a real, possibly policy-restricted tool could dodge those checks entirely: inspection sees the unrecognized mangled name while dispatch executes the canonical one underneath. Fix: canonicalize tool_call.name in categorize_tool_requests, immediately after parsing the provider response and before inspect_tools/dispatch ever run on it. This is also where the existing schema-argument-coercion exact lookup already happens, so recovery and coercion share one lookup. This also closes two related gaps from review on aaif-goose#10230: - Unprefixed platform extensions (e.g. "developer", advertised tools have no "__" prefix, owner only in metadata) can now recover "developer.shell" to "shell" via owner-metadata matching in recover_mangled_tool_name. - Tools appended outside the extension manager (recipe__final_output, platform__manage_schedule) are covered for free, since the merged tool list categorize_tool_requests already receives includes them. recover_mangled_tool_name is now pub(crate) and takes (name, owner) pairs instead of bare names, so both call sites — pre-inspection canonicalization and ExtensionManager::resolve_tool's existing defense-in-depth recovery for callers that bypass Agent — share one implementation and one set of ambiguity guarantees.
8b66c08 to
d9944c9
Compare
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
Reviewed after rebasing this onto main (01859e2) myself.
The fix is the right shape. #10230 put mangled-name recovery inside ExtensionManager::resolve_tool, which runs after PermissionInspector and the PreToolUse hook — so a never_allow on db__drop_table did not match a model-emitted db.drop_table, policy passed, and dispatch then recovered and ran the tool the policy existed to block. Canonicalizing in categorize_tool_requests puts one rewrite point upstream of inspection, hooks and dispatch, rather than patching each consumer. Recovery keeps its unique-match-or-refuse guarantee and only runs when exact lookup has already failed, so advertised names are never rewritten and the normal path is unchanged.
The rebase needed three fixes for upstream drift into PR-added code:
- import conflict in
reply_parts.rs(both sides kept) rmcp::model::Metawas renamed upstream toMetaObject; updated the new test to match the existing call sites in the same fileResolvedTool.tool_nameno longer exists (it isactual_tool_nameafter #10747); dropped the stale assertion, keptactual_tool_nameandextension_name
Verified locally at the rebased commit: cargo fmt --all --check clean, cargo clippy --all-targets -- -D warnings clean, cargo test -p goose 1770 passed / 15 failed — and those same 15 fail identically on a clean main worktree (jsonwebtoken CryptoProvider and config-path issues in my local env), so zero new failures and +6 net passing tests. Full CI is green on the rebased head.
Approving. Thanks for flagging the P1 in your own merged work openly rather than quietly — that is the right instinct.
One thing that should not get lost: issue #9486 is still CLOSED while the bypass it tracks is live in main. That needs reopening or a fresh tracking issue independent of this PR landing.
Reviewed with agent assistance (rebase, build/test verification) on behalf of @michaelneale.
* origin/main: fix: bound retry command diagnostics (#11365) Disable thinking for tool call labels (#11207) fix(review): contain REVIEW.md discovery (#11367) fix(acp): preserve tool result audience metadata (#11375) test(providers): isolate environment-proxy test in its own binary (#11262) fix(security): bind Foundry API keys to request origin (#11347) fix: canonicalize mangled tool names before permission inspection (follow-up to #10230) (#10285) feat(providers): add Lynkr as a declarative OpenAI-compatible provider (#11372) fix: sanitize Pi imported output (#10990) fix(otel): honor RUST_LOG directives for logs (#11360) feat(skills): add web-search and browser-use built-in skills (#11233) feat(dictation): add model-native audio transcription provider (#10589) feat(aws_bedrock): route OpenAI GPT-5.6 (sol/terra/luna) via Bedrock … (#10502) refactor(goose-local-inference): move mlx deps under macos (#11328) feat(providers): add PleumRouter declarative provider (#10479)
* origin/main: (50 commits) chore(deps): bump the ui-minor-and-patch group across 1 directory with 53 updates (#11386) fix(security): bound call graph traversal (#11193) fix: pin arrayref to known-good commit (#11389) feat(providers): add SayGM as declarative OpenAI-compatible provider (#11267) feat(providers): custom provider cost fields drive cost tracking (config-declared pricing fallback) (#11220) fix(deps): repair dangling syn reference in Cargo.lock (#11385) fix(flake): add cudaforge hash for git dependency (#10910) feat: auto-focus chat input when user starts typing (#11184) fix(security): fail closed on invalid Codex ACP mode (#11362) fix(mcp): keep stdio extensions alive across worker exits (#10364) feat(ui): collapse scheduled job sessions into accordion in chat history (#11265) fix: bound retry command diagnostics (#11365) Disable thinking for tool call labels (#11207) fix(review): contain REVIEW.md discovery (#11367) fix(acp): preserve tool result audience metadata (#11375) test(providers): isolate environment-proxy test in its own binary (#11262) fix(security): bind Foundry API keys to request origin (#11347) fix: canonicalize mangled tool names before permission inspection (follow-up to #10230) (#10285) feat(providers): add Lynkr as a declarative OpenAI-compatible provider (#11372) fix: sanitize Pi imported output (#10990) ... # Conflicts: # crates/goose/src/agents/state_machine/tests/hooks_lifecycle.rs
* origin/main: (59 commits) chore(deps): bump the ui-minor-and-patch group across 1 directory with 53 updates (#11386) fix(security): bound call graph traversal (#11193) fix: pin arrayref to known-good commit (#11389) feat(providers): add SayGM as declarative OpenAI-compatible provider (#11267) feat(providers): custom provider cost fields drive cost tracking (config-declared pricing fallback) (#11220) fix(deps): repair dangling syn reference in Cargo.lock (#11385) fix(flake): add cudaforge hash for git dependency (#10910) feat: auto-focus chat input when user starts typing (#11184) fix(security): fail closed on invalid Codex ACP mode (#11362) fix(mcp): keep stdio extensions alive across worker exits (#10364) feat(ui): collapse scheduled job sessions into accordion in chat history (#11265) fix: bound retry command diagnostics (#11365) Disable thinking for tool call labels (#11207) fix(review): contain REVIEW.md discovery (#11367) fix(acp): preserve tool result audience metadata (#11375) test(providers): isolate environment-proxy test in its own binary (#11262) fix(security): bind Foundry API keys to request origin (#11347) fix: canonicalize mangled tool names before permission inspection (follow-up to #10230) (#10285) feat(providers): add Lynkr as a declarative OpenAI-compatible provider (#11372) fix: sanitize Pi imported output (#10990) ...
3 participants
Problem
Follow-up to #10230, which was merged with two review findings still open (#10230 (review) and inline comments after merge). I want to be upfront that this PR ships with an unresolved P1 already in
main— this addresses it directly.P1 — recovery bypasses permission/hook policy checks
#10230 recovers a model-mangled tool name (GLM/Minimax) only inside
ExtensionManager::resolve_tool, deep in the dispatch path. ButPermissionInspector(crates/goose/src/permission/permission_inspector.rs:143) and thePreToolUsehook (crates/goose/src/agents/agent.rs:1062-1092, insideAgent::dispatch_tool_call) both run earlier, checking policy (never_allow/ask_before, blocking hooks) directly againsttool_call.nameon the raw, unrecovered request.Concretely: if a user has
never_allowondb__drop_table, and a model emits the mangleddb.drop_table, permission inspection doesn't recognize the mangled name (no rule matches it) and lets it through — then dispatch recovers it to the realdb__drop_tableand executes the very tool the policy was meant to block.P2 — unprefixed platform extensions still unrecovered
recover_mangled_tool_nameonly tried the "__" separator mangling. Platform extensions withunprefixed_tools: true(developer,analyze,summon,code_execution,skills— seecrates/goose/src/agents/platform_extensions/mod.rs) advertise tools with no prefix at all; the owner lives only in tool metadata. GLM's own documented reproduction in the original issue,developer.shell, was still unrecovered after #10230.P2 — tools outside the extension manager not covered
recipe__final_outputandplatform__manage_scheduleare appended byAgent::list_toolsoutside the extension manager (crates/goose/src/agents/agent.rs:1449-1459). Recovery only ran insideExtensionManager::resolve_tool, which never sees these two tools, so a mangledrecipe.final_outputhad no path to recovery.Fix
Canonicalize at the source, not at the sink.
categorize_tool_requests(crates/goose/src/agents/reply_parts.rs) already does an exact-name lookup against the full tool list (Agent::list_toolsoutput — extension-manager tools plus the two special ones) immediately after parsing the provider response, for schema-argument coercion. This is upstream of everything: permission inspection, hooks, and dispatch all consume its output (remaining_requests) unchanged.tool_call.nameto the canonical form before coercion runs. One rewrite point closes the P1 gap for every downstream consumer at once — no per-inspector or per-hook patching needed.recover_mangled_tool_nameto accept(name, owner)pairs (owner fromget_tool_owner, from tool metadata) instead of bare names, adding an owner-metadata match branch:developer.shellnow matchesshellwhenshell's metadata owner isdeveloper. The existing separator/prefix matching (from fix: recover malformed tool calls from GLM/Minimax models instead of rejecting them #10230) is unchanged.recipe__final_output/platform__manage_schedulerecovery falls out for free:categorize_tool_requests's tool list already includes them (unlikeExtensionManager's own cache), and they use the sameowner__toolconvention the separator-mangling check already handles.recover_mangled_tool_nameis nowpub(crate), called from bothreply_parts.rs(pre-inspection canonicalization) andExtensionManager::resolve_tool(unchanged, now a defense-in-depth safety net for any caller that bypassesAgent, e.g. tests or direct API consumers) — one implementation, one set of ambiguity guarantees, not two divergent copies.Testing
Every new behavior was written test-first and confirmed failing before the fix:
test_recover_mangled_tool_name_unprefixed_extension— owner-metadata matching, including a wrong-owner-must-not-match case and an ambiguity case (two different unprefixed extensions both owning a same-named tool).test_recover_mangled_tool_name_non_extension_manager_tools—recipe.final_output/platform.manage_schedulerecover via the plain separator check.test_resolve_tool_recovers_unprefixed_platform_extension_name— end-to-end throughresolve_tool, using a mock extension named literallydevelopersois_unprefixed_extensionlooks it up in the realPLATFORM_EXTENSIONSregistry rather than a fake stand-in — this is GLM's exact documented reproduction from the original issue.categorize_tool_requests_canonicalizes_mangled_unprefixed_tool_name/_non_extension_manager_tool_name— confirm the rewrite happens at thecategorize_tool_requestsboundary, i.e. before permission inspection and hooks would ever see the request.categorize_tool_requests_leaves_unrecoverable_name_untouched— no matching tool at all: request passes through unchanged, not-found handling downstream is unaffected.Results:
cargo test -p goose --lib: 1314 pass; the same 5 pre-existing failures as on a cleanmain(jsonwebtoken/insta environment issues, unrelated to this change — verified identical before this PR)cargo clippy -p goose --all-targets -- -D warnings: cleancargo fmt: applied