-
Notifications
You must be signed in to change notification settings - Fork 7.9k
feat: dynamic tools injection #9539
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
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d62269ea2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # codex-rs/tui/src/chatwidget.rs
# Conflicts: # codex-rs/tui2/src/chatwidget.rs
# Conflicts: # codex-rs/app-server-protocol/src/protocol/v2.rs # codex-rs/app-server/README.md # codex-rs/app-server/src/codex_message_processor.rs
# Conflicts: # codex-rs/app-server-protocol/src/protocol/v2.rs # codex-rs/app-server/src/codex_message_processor.rs # codex-rs/core/src/thread_manager.rs
|
Will fix the merge ofc |
dylan-hurd-oai
left a comment
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.
1 suggestion for specs, otherwise really like the approach!
| fn dynamic_tool_to_openai_tool( | ||
| tool: &DynamicToolSpec, | ||
| ) -> Result<ResponsesApiTool, serde_json::Error> { | ||
| let input_schema = parse_tool_input_schema(&tool.input_schema)?; |
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'm a bit worried that RESERVED_DYNAMIC_TOOL_NAMES will drift - should we check for conflicts/duplicates at runtime, or add a test to assert that all built-in tool names are covered by the list?
Summary
Add dynamic tool injection to thread startup in API v2, wire dynamic tool calls through the app server to clients, and plumb responses back into the model tool pipeline.
Flow (high level)
dynamic_toolsinto the model tool list for that thread (validation is done here).DynamicToolCallRequestevent.item/tool/call, waits for the client’s response, then submits aDynamicToolResponseback to core.function_call_outputin the next model request so the model can continue.What changed
item/tool/call(request/response) for dynamic tool execution.