Skip to content

fix(tool_transform): hoist $defs to schema root when ArgTransform introduces them - #4101

Merged
jlowin merged 2 commits into
PrefectHQ:mainfrom
SarthakB11:fix/issue-4093
May 7, 2026
Merged

jlowin merged 2 commits into
PrefectHQ:mainfrom
SarthakB11:fix/issue-4093

Conversation

@SarthakB11

Copy link
Copy Markdown
Contributor

TransformedTool._apply_single_transform calls get_cached_typeadapter(transform.type).json_schema() and merges the result into the per-property new_schema with new_schema.update(type_schema). TypeAdapter.json_schema() returns a self-contained schema with $defs at its top level, so the update plants those defs inside the property node while the $ref values still point at #/$defs/... (the schema root). Any consumer that resolves refs raises KeyError.

_create_forwarding_transform already hoists the parent tool's pre-existing $defs, but when the parent declares the parameter as a plain type (e.g. dict | None), parent_defs is empty and the hoist branch never fires. _apply_single_transform now pops $defs off the TypeAdapter schema before merging into the property schema and returns them alongside; the caller folds them into parent_defs so they land at the schema root via the same path that already handles parent defs (including compress_schema's unused-def pruning).

class Filter(BaseModel):
    field: str
    op: str

@base.tool
def search(filters: dict | None = None) -> list[str]: ...

base.add_tool_transform(
    "search",
    arg_transforms={"filters": ArgTransform(type=list[Filter])},
)
# Before: $defs lands inside the property node; refs to #/$defs/Filter resolve to nothing.
# After:  $defs hoists to the schema root; refs resolve cleanly.

Closes #4093.

  • If I used an LLM, it followed the repo's contributing conventions (not generic output)

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@marvin-context-protocol marvin-context-protocol Bot added bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. server Related to FastMCP server implementation or server-side functionality. labels May 5, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/fastmcp/tools/tool_transform.py Outdated
Comment on lines +690 to +691
if extracted_defs:
parent_defs.update(extracted_defs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@SarthakB11

SarthakB11 commented May 5, 2026 •

Copy link
Copy Markdown
Contributor Author

Good catch, pushed eaf3a15. The hoist now raises ValueError on key collision when the colliding schemas differ; identical re-introductions are a no-op. Added a regression test (test_arg_transform_type_raises_on_defs_name_collision) that plants a different schema under $defs.Foo on the parent and confirms the transform raises with the expected message. Full tests/tools/tool_transform/ (114 tests) passes.

@jlowin jlowin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jlowin
jlowin merged commit b8597f9 into PrefectHQ:main May 7, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. server Related to FastMCP server implementation or server-side functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ArgTransform(type=...) places $defs inside the property node instead of hoisting to schema root

2 participants