Repository navigation
fix(tool_transform): hoist $defs to schema root when ArgTransform introduces them - #4101
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f743841809
ℹ️ 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 extracted_defs: | ||
| parent_defs.update(extracted_defs) |
There was a problem hiding this comment.
Avoid overwriting existing $defs when hoisting transforms
When an ArgTransform(type=...) introduces a $defs entry whose key already exists in the parent schema, this update silently replaces the parent definition while the already-copied parent properties still point at #/$defs/<name>. This happens with common Pydantic schemas such as two different BaseModel classes named Item from different modules, or two transformed args that each define a nested model with the same name; the resulting transformed tool schema documents/validates the earlier property against the wrong model. The hoist path needs to preserve both definitions by detecting collisions and rewriting refs, or otherwise avoid merging colliding defs blindly.
Useful? React with 👍 / 👎.
|
Good catch, pushed eaf3a15. The hoist now raises |
TransformedTool._apply_single_transformcallsget_cached_typeadapter(transform.type).json_schema()and merges the result into the per-propertynew_schemawithnew_schema.update(type_schema).TypeAdapter.json_schema()returns a self-contained schema with$defsat its top level, so theupdateplants those defs inside the property node while the$refvalues still point at#/$defs/...(the schema root). Any consumer that resolves refs raisesKeyError._create_forwarding_transformalready hoists the parent tool's pre-existing$defs, but when the parent declares the parameter as a plain type (e.g.dict | None),parent_defsis empty and the hoist branch never fires._apply_single_transformnowpops$defsoff theTypeAdapterschema before merging into the property schema and returns them alongside; the caller folds them intoparent_defsso they land at the schema root via the same path that already handles parent defs (includingcompress_schema's unused-def pruning).Closes #4093.