-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Dynamic web_search inclusion #5972
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
base: main
Are you sure you want to change the base?
Conversation
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.
ℹ️ 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".
| payload, | ||
| }; | ||
|
|
||
| if !invocation | ||
| .turn | ||
| .tools_config | ||
| .is_tool_allowed(invocation.tool_name.as_str()) | ||
| { | ||
| let message = format!("tool {} is disabled for this session", invocation.tool_name); | ||
| let otel = invocation.turn.client.get_otel_event_manager(); | ||
| otel.tool_result( | ||
| invocation.tool_name.as_str(), | ||
| &invocation.call_id, | ||
| invocation.payload.log_payload().as_ref(), | ||
| Duration::ZERO, | ||
| false, | ||
| &message, | ||
| ); | ||
| return Err(FunctionCallError::RespondToModel(message)); |
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.
Keep disabled-tool handling consistent
The new guard right above the dispatcher returns Err(FunctionCallError::RespondToModel(...)) when a tool is disabled. In our pipeline, any Err(_) that reaches ToolCallRuntime::handle_tool_call is immediately promoted to CodexErr::Fatal, which aborts the turn (handle_tool_call maps every non-Fatal error to CodexErr::Fatal). Previously, non-fatal tool errors went through the match below and became Ok(Self::failure_response(...)), letting the model receive a structured failure response without blowing up the turn. With this branch in place, toggling web search off will cause any stray web_search call to terminate the entire turn with a fatal error instead of a normal tool failure. We should reuse the same failure_response path (or otherwise keep the error in the Ok branch) so the turn continues gracefully.
Useful? React with 👍 / 👎.
Adds a slash command to toggle web_search on and off using allowed_tools to preserve token caching