-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[codex] Enable standalone web search in code mode #26719
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ use http::HeaderMap; | |
| use url::Url; | ||
|
|
||
| use crate::history::recent_input; | ||
| use crate::output::EncryptedSearchOutput; | ||
| use crate::output::SearchOutput; | ||
| use crate::schema::commands_schema; | ||
|
|
||
| pub(crate) const WEB_NAMESPACE: &str = "web"; | ||
|
|
@@ -67,7 +67,7 @@ impl ToolExecutor<ToolCall> for WebSearchTool { | |
| } | ||
|
|
||
| fn exposure(&self) -> ToolExposure { | ||
| ToolExposure::DirectModelOnly | ||
| ToolExposure::Direct | ||
| } | ||
|
|
||
| fn supports_parallel_tool_calls(&self) -> bool { | ||
|
|
@@ -114,9 +114,7 @@ impl ToolExecutor<ToolCall> for WebSearchTool { | |
| .emit_completed(web_search_item(&call.call_id, command_action)) | ||
| .await; | ||
|
|
||
| Ok(Box::new(EncryptedSearchOutput::new( | ||
| response.encrypted_output, | ||
| ))) | ||
| Ok(Box::new(SearchOutput::new(response.output))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pseudo related to this, there is some token accounting for tool call output. previously this was encrypted so responses added a max_output_tokens field (because we couldn’t truncate encrypted content in the harness safely), but now that it’s not can/does this output go through the normal tool call accounting that tracks + truncates large results? EDIT: nvm, we get this for free |
||
| } | ||
| } | ||
|
|
||
|
|
||
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:
Should this PR also make standalone search honor memories.disable_on_external_context? This output is recorded as a normal FunctionCallOutput, which the external-context detector does not recognize, so plaintext web results remain eligible for memory generation even when that setting is enabled.
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.
adding a func on tool output to allow us to do this cleanly. It is bit out of the scope of this PR (since its creating something reusable for future external tool calls), so I've stacked a follow up PR to resolve this issue: #26821