fix: keep FunctionTool subclasses copyable - #4273
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 054b470310
ℹ️ 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".
| # Rebuild the instance state directly instead of re-running the constructor, so | ||
| # FunctionTool subclasses that define their own __init__ signature stay copyable. | ||
| copied_tool = object.__new__(type(self)) | ||
| copied_tool.__dict__.update(self.__dict__) |
There was a problem hiding this comment.
Rebind subclass invokers to the copied tool
When a FunctionTool subclass passes self._invoke into super().__init__ (the pattern this commit is enabling), this __dict__ copy carries over the already-bound method object, so copied_tool.on_invoke_tool.__self__ remains the original instance. After copy.copy() or tool_namespace() returns a copy, any copied custom state that is later changed on the copy, such as session/user/name or namespace-sensitive state, is ignored during invocation because the call still runs against the original tool. Rebind self-bound methods to copied_tool before returning the copy.
Useful? React with 👍 / 👎.
seratch
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The underlying bug is valid: tool_namespace() accepts FunctionTool instances, but the released __copy__ path calls dataclasses.replace(), so SDK-owned init=False subclasses such as ExecCommandTool and ViewImageTool fail before namespacing. This is worth fixing.
Before merge, please preserve invoker ownership in the new copy path. After __dict__.update(), a subclass configured with on_invoke_tool=self._invoke remains bound to the original instance; the current test returns only raw_input and cannot detect this. Rebind directly bound invokers to the copied instance, then add a regression where _invoke reads self.session, the copied tool receives a different session sentinel, and invocation proves that the copied state is used. Arbitrary slot-only or custom-allocation subclasses do not need to be supported.
FunctionTool.__copy__ rebuilt the tool with dataclasses.replace(), which calls the concrete class constructor with every dataclass field. Any FunctionTool subclass that defines its own __init__ signature therefore raised TypeError on copy.copy(), and tool_namespace() raised the same error because it copies each tool before attaching namespace metadata. The SDK's own sandbox exec_command and write_stdin tools hit this. Copy the instance state onto a fresh object of the same type and re-run __post_init__ instead, which keeps the existing rebind, schema copy, and timeout validation behavior for every FunctionTool.
054b470 to
ffe78c6
Compare
|
Done in ffe78c6. The copy now rebinds an invoker that was bound to the original instance: invoker = copied_tool.__dict__.get("on_invoke_tool")
invoker_func = getattr(invoker, "__func__", None)
if invoker_func is not None and getattr(invoker, "__self__", None) is self:
copied_tool.on_invoke_tool = invoker_func.__get__(copied_tool, type(copied_tool))It only rebinds when
57 passed, mypy clean on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffe78c6bcd
ℹ️ 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".
| invoker_func = getattr(invoker, "__func__", None) | ||
| if invoker_func is not None and getattr(invoker, "__self__", None) is self: | ||
| copied_tool.on_invoke_tool = invoker_func.__get__(copied_tool, type(copied_tool)) | ||
| copied_tool.__post_init__() |
There was a problem hiding this comment.
Avoid rerunning subclass post-init during copies
When a FunctionTool subclass defines __post_init__ to initialize or normalize its own init=False/runtime state, this call runs the subclass hook after line 605 has already copied the original instance dictionary, so the copied state can be reset or recomputed. For subclasses whose old dataclasses.replace path worked, the previous implementation restored non-FunctionTool attributes after post-init; after this change, copy.copy() and tool_namespace() can silently lose mutated subclass state. Call the base FunctionTool.__post_init__ on the copied object, or otherwise avoid polymorphically rerunning subclass initialization hooks during a shallow copy.
Useful? React with 👍 / 👎.
anujbolewar
left a comment
There was a problem hiding this comment.
The direct state rebuild is the right call for subclass init signatures, and rebinding the invoker to the copy avoids the copy operating on the original instance. The object.new plus post_init path does require post_init to be idempotent on already-populated fields — worth a quick check that subclasses whose post_init derives state from init-only arguments behave identically after a copy. Adding a subclass-with-custom-init regression test around the invoker rebinding would lock the trickiest part.
FunctionTool.__copy__rebuilt the tool withdataclasses.replace(self), which calls the concrete class constructor with every dataclass field. AFunctionToolsubclass that defines its own__init__signature therefore raisesTypeError: __init__() got an unexpected keyword argument 'name'oncopy.copy(), andtool_namespace()raises the same error because it copies each tool before attaching namespace metadata.The SDK's own sandbox tools hit this —
ExecCommandToolandWriteStdinToolinagents/sandbox/capabilities/tools/shell_tool.pyare@dataclass(init=False)FunctionToolsubclasses with keyword-only constructors, so neither can be copied or grouped into a namespace.Copy the instance state onto a fresh object of the same type and re-run
__post_init__instead. That keeps every effect of the old path — invoker rebinding, theallowed_callerscopy, theparams_json_schemadeep copy and strictification,output_json_schemanormalization, and timeout validation — while also carrying non-field attributes across, which the old manual loop handled separately.Tests cover
copy.copy()andtool_namespace()on a subclass with a custom constructor.