Repository navigation
Make execute_tool the public method AirflowToolset subclasses implement - #73938
Merged
Merged
Conversation
A subclass had to implement the private _execute_tool, so a method that was free to change was also the one every toolset depends on. It is now the public execute_tool, with ctx and tool keyword-only so that arguments can be added later without breaking subclasses. call_tool keeps pydantic-ai's positional signature, since pydantic-ai calls it that way. Also renames with_masking to ensure_masked, which says that a toolset that already masks its output comes back unchanged, and gives the masking helpers names that are easier to tell apart: _masked becomes _mask_call and _stripped becomes _mask_attributes. MCPToolset._server_once_resolved becomes _resolve_server.
Lee-W
approved these changes
Sep 30, 2026
Lee-W
left a comment
Lee-W
left a comment
Member
There was a problem hiding this comment.
This PR looks great. 2 suggestion (could be part of this PR or follow up)
- call_tool and execute_tool are not easy to distinguish. we'll need some doc or some other way so that users won't get confused
- their function signature is not the same. might be something confusing as well
Say in the class docstring which method a subclass writes, why overriding call_tool skips the masking, and why the two signatures are shaped differently.
Lee-W
approved these changes
Sep 30, 2026
Member
Author
|
Thanks. Both are now covered in the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses @Lee-W's review on #73897, which merged before the comments were in.
A toolset subclass had to implement
AirflowToolset._execute_tool: the method every toolset depends on had a private name, which says it is free to change (comment). It is now the publicexecute_tool, andctxandtoolare keyword-only so that arguments can be added later without breaking subclasses (comment).The keyword-only suggestion was made on
call_tool, but that signature has to stay as pydantic-ai defines it: pydantic-ai callscall_tool(name, tool_args, ctx, tool)positionally, inWrapperToolset,CombinedToolsetand its durable-execution toolsets. So it applies toexecute_tool, the method subclasses write, instead.Renames:
with_maskingensure_masked_masked_mask_call_maskis already the recursive walker inutils/masking.py._stripped_mask_attributes_strip, which calls it.MCPToolset._server_once_resolved_resolve_server_json_defaultkeeps its name, since it is whatdumps_maskedpasses asjson.dumps(default=...).None of these names has been in a release yet, so nothing needs deprecating.