fix(core): select built-in shell and apply_patch tools by their own type - #4952
fix(core): select built-in shell and apply_patch tools by their own type#4952subhashpolisetti wants to merge 3 commits into
Conversation
… type
A request configured with ShellTool or ApplyPatchTool advertises the tool as
{"type": "shell"} or {"type": "apply_patch"}, but tool_choice="shell" and
tool_choice="apply_patch" fell through to the named-function branch and selected
a function tool the same request never defines.
Map both strings to their Responses selector when the matching built-in tool is
configured, and keep the named-function behavior otherwise so a function tool of
the same name still resolves.
seratch
changed the title
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The selector mismatch is real, and fixing it in the shared Responses converter is appropriate.
Before merging, please preserve named-function and handoff selection when a matching callable target is configured alongside the built-in. For example, with both ShellTool and a function named shell, the released behavior selects the function, but this patch silently selects the built-in. The same issue applies to apply_patch. Give the existing callable target precedence and use the built-in selector when no such target exists.
Please add outgoing-request regression coverage for these coexistence cases and for selecting each built-in among multiple tools. The current tests cover functions only when the corresponding built-in is absent.
…lector Give a function tool or handoff of the same name precedence over the shell and apply_patch selectors, and use the built-in selector only when no such callable target is configured. Cover the coexistence cases and built-in selection among multiple tools at the outgoing request.
|
Thanks for the review. Updated to give callable targets precedence: Outgoing-request coverage is in |
![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: 235a984258
ℹ️ 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".
| return not any( | ||
| isinstance(tool, FunctionTool | CustomTool) and tool.name == tool_choice | ||
| for tool in tools or () |
There was a problem hiding this comment.
When a built-in coexists with a same-named CustomTool, namespaced FunctionTool, or deferred FunctionTool, this predicate suppresses the valid built-in selector. The fallback then always emits {"type":"function","name":...}; a custom tool is advertised as type custom, while namespace/deferred cases are rejected by _validate_named_function_tool_choice, so the request either targets a nonexistent function or raises instead of forcing the available built-in. Only an exposed top-level function/handoff should select the function form, and other target types need their own valid selector handling.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed the custom-tool case: a custom tool is advertised as custom, so the named-function selector cannot target it. Precedence is now limited to function tools and handoffs.
Deferred and namespaced function tools are unchanged. Both already raise from _validate_named_function_tool_choice on v0.22.2, so forcing the built-in there would change released behavior beyond this fix.
…s its name A custom tool is advertised as a custom type, so the named-function selector cannot target it. Treating it as a callable target suppressed a valid built-in selector and left the request pointing at a function the request never defines. Limit the precedence rule to function tools and handoffs, which the named selector can target, and cover the custom-tool case at the outgoing request.
2 participants
Summary
This pull request fixes
tool_choice="shell"andtool_choice="apply_patch"selecting a function tool the request never defines.An agent configured with
ShellToolorApplyPatchToolsends the tool as{"type": "shell"}or{"type": "apply_patch"}. Settingtool_choiceto the matching string falls through to the named-function branch, so the same request also carries{"type": "function", "name": "shell"}while no function tool by that name is present in it. The Responses API definesToolChoiceShellParamandToolChoiceApplyPatchParamfor these tools andModelSettings.tool_choiceaccepts the strings, but neither selector is reachable today:requiredcannot target one specific tool when others are configured, and passing the selector dict directly raisesTypeError.Both strings now map to their own selector when the matching built-in tool is configured. Gating on tool presence follows
ComputerTool, where the built-in selector applies only when that tool is present and the string otherwise stays an ordinary function name; without the gate a function tool namedshellorapply_patchwould silently stop being targetable.LocalShellToolis deliberately unchanged, since the Responses API defines nolocal_shellselector.Test plan
test_convert_tool_choice_builtin_shell_and_apply_patchasserts each string maps to its own selector when the matching tool is configured. It fails onmain, which returns the named-function form.test_convert_tool_choice_allows_function_named_shell_without_builtin_toolpins that a function tool of either name still resolves to the named-function form, mirroring the existingComputerTooltest.make format,make lint,make typecheckanduv run mypy --platform win32 srcare clean..agents/skills/code-change-verification/scripts/run.shdoes not complete in my environment: 14 tests intests/test_code_change_verification_runner.pyfail withTimed out waiting for a controlled process transition. They reproduce identically on unmodifiedmainhere and are unrelated to this change. Excluding that file, the suite reports 9671 passed, 32 skipped.Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR