fix(tools): raise clear TypeError when on_invoke_tool gets non-ToolContext#3685
fix(tools): raise clear TypeError when on_invoke_tool gets non-ToolContext#3685wustwyh wants to merge 1 commit into
Conversation
…ntext Calling a @function_tool's on_invoke_tool(ctx, ...) with a ctx that is not a RunContextWrapper (e.g. None when unit-testing) currently fails with a chained AttributeError that masks the real mistake. The wrapper's failure handler dereferences the bad ctx again, producing a confusing stack trace. Validate ctx at the top of _FailureHandlingFunctionToolInvoker.__call__ before the try block. The happy path is untouched, and the error now propagates cleanly with an actionable message. Fixes openai#3681.
![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: 36c4c74730
ℹ️ 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 bound_invoker | ||
|
|
||
| async def __call__(self, ctx: ToolContext[Any], input: str) -> Any: | ||
| if not isinstance(ctx, RunContextWrapper): |
There was a problem hiding this comment.
For a regular @function_tool, passing a plain RunContextWrapper is still a non-ToolContext direct invocation, but this guard treats it as valid; _on_invoke_tool_impl then immediately reads ctx.tool_name and the handled-error path can read ctx.run_config, so callers still get the same confusing/masked AttributeError instead of the clear TypeError. If base wrappers are only needed for agent.as_tool(), gate that exception on the bound FunctionTool; otherwise require ToolContext here.
Useful? React with 👍 / 👎.
2 participants
Summary
Fixes #3681.
Calling a
@function_tool'son_invoke_tool(ctx, input_json)with actxthat is not a run context (most commonlyNonewhen unit-testing a tool directly) currently fails far from the cause with a cryptic chainedAttributeError:The wrapper's failure handler then dereferences the same bad
ctxagain (context.run_config), masking the original cause behind a second exception.What changed
Validate the context once, at the top of
_FailureHandlingFunctionToolInvoker.__call__(the publicon_invoke_toolentry point), before thetryblock. The check acceptsRunContextWrapper(which covers bothToolContextand the base wrapper used byagent.as_tool()), and raises a preciseTypeErrorfor anything else.The happy path is untouched (one
isinstancecheck), and the error now propagates cleanly instead of being routed through the failure formatter.Tests
Added
tests/test_function_tool.py::test_on_invoke_tool_rejects_non_tool_context, which asserts:Noneraises the clearTypeError.TypeError.ToolContextstill works and returns the expected result.Notes
RunContextWrapperrather than strictlyToolContext, because the same wrapper is used byagent.as_tool(), whose invoker legitimately runs with a baseRunContextWrapper. RequiringToolContextwould break that path.