-
Notifications
You must be signed in to change notification settings - Fork 19.9k
fix(anthropic): execute bash + file tools via tool node #33960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CodSpeed Performance ReportMerging #33960 will not alter performanceComparing Summary
Footnotes
|
* properly register shell tool w/ tool node * inherit this improvement in claude shell tool middleware as well * allow for configuration of tool name (which allows for claude impl to use `bash` as required by the anthropic api)
| execution_policy: BaseExecutionPolicy | None = None, | ||
| redaction_rules: tuple[RedactionRule, ...] | list[RedactionRule] | None = None, | ||
| tool_description: str | None = None, | ||
| tool_name: str = SHELL_TOOL_NAME, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: inline the tool name perhaps? it's only used in one place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have it as an arg here so that we can override it in the anthropic implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i meant to not use SHELL_TOOL_NAME and instead tool_name = "shell" , a global variable for one use case feels a bit odd
| AgentState, | ||
| ModelRequest, | ||
| ModelResponse, | ||
| _ModelRequestOverrides, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to make this public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question, I think no for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel comfortable importing this in a partner lib
* use `override` instead of directly patching things on `ModelRequest` * rely on `ToolNode` for execution of tools related to said middleware, using `wrap_model_call` to inject the relevant claude tool specs + allowing tool node to forward them along to corresponding langchain tool implementations * making the same change for the native shell tool middleware * allowing shell tool middleware to specify a name for the shell tool (negative diff then for claude bash middleware) long term I think the solution might be to attach metadata to a tool to map the provider spec to a langchain implementation, which we could also take some lessons from on the MCP front.
overrideinstead of directly patching things onModelRequestToolNodefor execution of tools related to said middleware, usingwrap_model_callto inject the relevant claude tool specs + allowing tool node to forward them along to corresponding langchain tool implementationslong term I think the solution might be to attach metadata to a tool to map the provider spec to a langchain implementation, which we could also take some lessons from on the MCP front.