mirror of
https://github.com/openai/codex.git
synced 2026-10-30 19:18:49 +00:00
Return standalone web search failures to the model (#58147)
## Why Standalone web search errors currently abort the turn as fatal failures, preventing the model from responding to them. ## What changed Return client setup errors to the model and represent failed search requests as unsuccessful tool outputs. Failed requests still emit search completion events and retain external-context handling. Limit error messages to 1,024 bytes, truncating at a UTF-8 character boundary with a truncation marker. ## Testing Add coverage for failure status reporting and UTF-8-safe error truncation. Extend workspace routing tests to verify that a rejected redirect returns a bounded diagnostic to the model, allows the turn to complete, emits search lifecycle events, and marks thread memory as polluted. GitOrigin-RevId: 9fb308990afa2ce0db5d9beed9bc66d9c1500418
This commit is contained in:
@@ -1,4 +1,4 @@
|
||||
//! Standalone search uses the discovered workspace origin and credentials.
|
||||
//! Standalone search uses the discovered workspace origin and cannot follow redirects.
|
||||
|
||||
use std::time::Duration;
|
||||
|
||||
@@ -14,10 +14,12 @@ use codex_app_server_protocol::TurnStatus;
|
||||
use codex_app_server_protocol::UserInput;
|
||||
use codex_config::types::AuthCredentialsStoreMode;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::ThreadId;
|
||||
use core_test_support::responses;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use tempfile::TempDir;
|
||||
use test_case::test_case;
|
||||
use tokio::io::copy_bidirectional;
|
||||
use tokio::net::TcpStream;
|
||||
use tokio::task::JoinSet;
|
||||
@@ -30,13 +32,20 @@ use wiremock::matchers::method;
|
||||
use wiremock::matchers::path;
|
||||
|
||||
use super::super::https_mock::tls_listener;
|
||||
use super::super::memory_reset::init_state_db;
|
||||
use super::search_request;
|
||||
use super::wait_for_web_search_completed;
|
||||
use super::wait_for_web_search_started;
|
||||
|
||||
const READ_TIMEOUT: Duration = Duration::from_secs(60);
|
||||
const SEARCH_PATH: &str = "/backend-api/codex/alpha/search";
|
||||
|
||||
#[test_case(Some("us_cr"); "us_cr_routes_search")]
|
||||
#[test_case(None; "unconstrained_rejects_redirect_and_bounds_error_output")]
|
||||
#[tokio::test]
|
||||
async fn standalone_web_search_uses_workspace_routing() -> Result<()> {
|
||||
async fn standalone_web_search_uses_workspace_routing(
|
||||
routing_override: Option<&str>,
|
||||
) -> Result<()> {
|
||||
let bootstrap = MockServer::start().await;
|
||||
let routed = responses::start_mock_server().await;
|
||||
let home = TempDir::new()?;
|
||||
@@ -70,7 +79,7 @@ async fn standalone_web_search_uses_workspace_routing() -> Result<()> {
|
||||
"accounts": [{
|
||||
"id": "selected",
|
||||
"workspace_backend_origin": routed_origin,
|
||||
"account_routing_override": "us_cr",
|
||||
"account_routing_override": routing_override.unwrap_or("NO_CONSTRAINT"),
|
||||
}],
|
||||
})))
|
||||
.expect(1)
|
||||
@@ -86,14 +95,26 @@ async fn standalone_web_search_uses_workspace_routing() -> Result<()> {
|
||||
.mount(&bootstrap)
|
||||
.await;
|
||||
|
||||
let response = if routing_override.is_some() {
|
||||
ResponseTemplate::new(200).set_body_json(json!({
|
||||
"output": "Routed search result",
|
||||
}))
|
||||
} else {
|
||||
ResponseTemplate::new(307)
|
||||
.insert_header("location", format!("{routed_origin}/redirected-search"))
|
||||
.set_body_string(format!("Search diagnostic: {}", "λ".repeat(/*n*/ 16_384)))
|
||||
};
|
||||
Mock::given(method("POST"))
|
||||
.and(path(SEARCH_PATH))
|
||||
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
|
||||
"output": "Routed search result",
|
||||
})))
|
||||
.respond_with(response)
|
||||
.expect(1)
|
||||
.mount(&routed)
|
||||
.await;
|
||||
Mock::given(path("/redirected-search"))
|
||||
.respond_with(ResponseTemplate::new(200))
|
||||
.expect(0)
|
||||
.mount(&routed)
|
||||
.await;
|
||||
|
||||
let call_id = "routed-search";
|
||||
let model_responses = vec![
|
||||
@@ -116,6 +137,7 @@ async fn standalone_web_search_uses_workspace_routing() -> Result<()> {
|
||||
"chatgpt_base_url = \"{}/backend-api\"",
|
||||
bootstrap.uri()
|
||||
))
|
||||
.with_root_config("memories.disable_on_external_context = true")
|
||||
.with_model_provider("openai-custom")
|
||||
.with_provider_name("OpenAI")
|
||||
.with_provider_base_url(&format!("{}/backend-api/codex", bootstrap.uri()))
|
||||
@@ -149,6 +171,13 @@ async fn standalone_web_search_uses_workspace_routing() -> Result<()> {
|
||||
.build_initialized_with_timeout(READ_TIMEOUT)
|
||||
.await?;
|
||||
let thread = app.start_thread(ThreadStartParams::default()).await?.thread;
|
||||
let thread_id = ThreadId::from_string(&thread.id)?;
|
||||
let state_db = init_state_db(home.path()).await?;
|
||||
// A new thread can acquire its SQLite row on the first turn.
|
||||
assert!(matches!(
|
||||
state_db.get_thread_memory_mode(thread_id).await?.as_deref(),
|
||||
None | Some("enabled")
|
||||
));
|
||||
let completed = timeout(
|
||||
READ_TIMEOUT,
|
||||
app.start_turn_and_wait_for_completion(TurnStartParams {
|
||||
@@ -164,16 +193,41 @@ async fn standalone_web_search_uses_workspace_routing() -> Result<()> {
|
||||
assert_eq!(completed.turn.status, TurnStatus::Completed);
|
||||
let requests = model_requests.requests();
|
||||
assert_eq!(requests.len(), 2);
|
||||
assert_eq!(
|
||||
requests[1].function_call_output(call_id)["output"],
|
||||
json!([{"type": "input_text", "text": "Routed search result"}])
|
||||
);
|
||||
if routing_override.is_some() {
|
||||
assert_eq!(
|
||||
requests[1].function_call_output(call_id)["output"],
|
||||
json!([{"type": "input_text", "text": "Routed search result"}])
|
||||
);
|
||||
} else {
|
||||
let error = requests[1]
|
||||
.function_call_output_text(call_id)
|
||||
.context("redirect rejection should be reported to the model")?;
|
||||
assert!(error.len() <= 1024, "{} bytes", error.len());
|
||||
assert!(error.contains("http 307 Temporary Redirect"), "{error}");
|
||||
assert!(error.contains("Search diagnostic: λ"), "{error}");
|
||||
assert_eq!(
|
||||
state_db.get_thread_memory_mode(thread_id).await?.as_deref(),
|
||||
Some("polluted")
|
||||
);
|
||||
let started = timeout(READ_TIMEOUT, wait_for_web_search_started(&mut app)).await??;
|
||||
let completed =
|
||||
timeout(READ_TIMEOUT, wait_for_web_search_completed(&mut app)).await??;
|
||||
assert_eq!(started.item.id(), call_id);
|
||||
assert_eq!(completed.item.id(), call_id);
|
||||
}
|
||||
|
||||
let search = search_request(&routed, SEARCH_PATH).await?;
|
||||
assert_eq!(search.headers["authorization"], "Bearer access-chatgpt");
|
||||
assert_eq!(search.headers["chatgpt-account-id"], "selected");
|
||||
assert_eq!(search.headers["x-openai-internal-codex-residency"], "us");
|
||||
assert_eq!(search.headers["x-openai-account-routing-override"], "us_cr");
|
||||
assert_eq!(
|
||||
search
|
||||
.headers
|
||||
.get("x-openai-account-routing-override")
|
||||
.map(|value| value.to_str())
|
||||
.transpose()?,
|
||||
routing_override
|
||||
);
|
||||
bootstrap.verify().await;
|
||||
routed.verify().await;
|
||||
Ok::<_, anyhow::Error>(app)
|
||||
|
||||
@@ -5,12 +5,21 @@ use codex_protocol::models::FunctionCallOutputPayload;
|
||||
use codex_protocol::models::ResponseInputItem;
|
||||
|
||||
pub(crate) struct SearchOutput {
|
||||
output: String,
|
||||
payload: FunctionCallOutputPayload,
|
||||
}
|
||||
|
||||
impl SearchOutput {
|
||||
pub(crate) fn new(output: String) -> Self {
|
||||
Self { output }
|
||||
pub(crate) fn new(result: Result<String, String>) -> Self {
|
||||
let payload = match result {
|
||||
Ok(text) => FunctionCallOutputPayload::from_content_items(vec![
|
||||
FunctionCallOutputContentItem::InputText { text },
|
||||
]),
|
||||
Err(error) => FunctionCallOutputPayload {
|
||||
success: Some(false),
|
||||
..FunctionCallOutputPayload::from_text(error)
|
||||
},
|
||||
};
|
||||
Self { payload }
|
||||
}
|
||||
}
|
||||
|
||||
@@ -20,7 +29,7 @@ impl ToolOutput for SearchOutput {
|
||||
}
|
||||
|
||||
fn success_for_logging(&self) -> bool {
|
||||
true
|
||||
self.payload.success.unwrap_or(true)
|
||||
}
|
||||
|
||||
fn contains_external_context(&self) -> bool {
|
||||
@@ -30,45 +39,27 @@ impl ToolOutput for SearchOutput {
|
||||
fn to_response_item(&self, call_id: &str, _payload: &ToolPayload) -> ResponseInputItem {
|
||||
ResponseInputItem::FunctionCallOutput {
|
||||
call_id: call_id.to_string(),
|
||||
output: FunctionCallOutputPayload::from_content_items(vec![
|
||||
FunctionCallOutputContentItem::InputText {
|
||||
text: self.output.clone(),
|
||||
},
|
||||
]),
|
||||
output: self.payload.clone(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use codex_extension_api::ToolPayload;
|
||||
use codex_protocol::models::FunctionCallOutputContentItem;
|
||||
use codex_protocol::models::FunctionCallOutputPayload;
|
||||
use codex_protocol::models::ResponseInputItem;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
use super::SearchOutput;
|
||||
use super::ToolOutput;
|
||||
|
||||
#[test]
|
||||
fn emits_plaintext_function_call_output() {
|
||||
let output = SearchOutput::new("search output".to_string());
|
||||
|
||||
assert_eq!(
|
||||
output.to_response_item(
|
||||
"call-1",
|
||||
&ToolPayload::Function {
|
||||
arguments: "{}".to_string(),
|
||||
},
|
||||
),
|
||||
ResponseInputItem::FunctionCallOutput {
|
||||
call_id: "call-1".to_string(),
|
||||
output: FunctionCallOutputPayload::from_content_items(vec![
|
||||
FunctionCallOutputContentItem::InputText {
|
||||
text: "search output".to_string(),
|
||||
},
|
||||
]),
|
||||
}
|
||||
);
|
||||
fn reports_search_status_for_logging_and_downstream_handling() {
|
||||
for (result, success) in [
|
||||
(Ok("found".into()), None),
|
||||
(Err("failed".into()), Some(false)),
|
||||
] {
|
||||
let output = SearchOutput::new(result);
|
||||
assert_eq!(output.payload.success, success);
|
||||
assert_eq!(output.success_for_logging(), success.unwrap_or(true));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -103,6 +103,6 @@ async fn config_changes_preserve_the_threads_established_routing() {
|
||||
};
|
||||
assert!(matches!(
|
||||
tools[0].handle(call).await,
|
||||
Err(FunctionCallError::Fatal(message)) if message == thread_bootstrap
|
||||
Err(FunctionCallError::RespondToModel(message)) if message == thread_bootstrap
|
||||
));
|
||||
}
|
||||
|
||||
@@ -51,6 +51,7 @@ pub(crate) const WEB_NAMESPACE: &str = "web";
|
||||
pub(crate) const RUN_TOOL_NAME: &str = "run";
|
||||
const WEB_RUN_DESCRIPTION: &str = include_str!("../web_run_description.md");
|
||||
const RESULTS_PAYLOAD_BYTES_METRIC: &str = "codex.web_search.results.payload_bytes";
|
||||
const MAX_SEARCH_ERROR_BYTES: usize = 1024;
|
||||
|
||||
pub(crate) struct WebSearchTool {
|
||||
pub(crate) session_id: String,
|
||||
@@ -150,7 +151,7 @@ impl WebSearchTool {
|
||||
Ok(SearchClient::new(transport, endpoint, api_auth))
|
||||
})
|
||||
.await
|
||||
.map_err(|err| FunctionCallError::Fatal(err.to_string()))?;
|
||||
.map_err(|err| FunctionCallError::RespondToModel(search_error_message(err)))?;
|
||||
let request = SearchRequest {
|
||||
id: self.session_id.clone(),
|
||||
model: call.model.clone(),
|
||||
@@ -179,12 +180,10 @@ impl WebSearchTool {
|
||||
}),
|
||||
))
|
||||
.await;
|
||||
let response = client
|
||||
.search(&request, extra_headers)
|
||||
.await
|
||||
.map_err(|err| FunctionCallError::Fatal(err.to_string()))?;
|
||||
let output = response.output;
|
||||
let results = response.results;
|
||||
let (output, results) = match client.search(&request, extra_headers).await {
|
||||
Ok(response) => (Ok(response.output), response.results),
|
||||
Err(err) => (Err(search_error_message(err)), None),
|
||||
};
|
||||
if let Some(results) = results.as_ref()
|
||||
&& let Some(metrics) = codex_otel::global()
|
||||
&& let Ok(payload) = serde_json::to_vec(results)
|
||||
@@ -226,6 +225,16 @@ impl WebSearchTool {
|
||||
}
|
||||
}
|
||||
|
||||
fn search_error_message(error: impl std::fmt::Display) -> String {
|
||||
let mut message = error.to_string();
|
||||
if message.len() > MAX_SEARCH_ERROR_BYTES {
|
||||
const SUFFIX: &str = "… [truncated]";
|
||||
message.truncate(message.floor_char_boundary(MAX_SEARCH_ERROR_BYTES - SUFFIX.len()));
|
||||
message.push_str(SUFFIX);
|
||||
}
|
||||
message
|
||||
}
|
||||
|
||||
fn search_request_headers(originator: Option<&str>, turn_metadata: Option<&str>) -> HeaderMap {
|
||||
let mut headers = HeaderMap::new();
|
||||
if let Some(turn_metadata) = turn_metadata
|
||||
@@ -326,10 +335,22 @@ mod tests {
|
||||
|
||||
use super::command_action;
|
||||
use super::parse_commands;
|
||||
use super::search_error_message;
|
||||
use super::search_request_headers;
|
||||
use codex_core::X_CODEX_TURN_METADATA_HEADER;
|
||||
use codex_extension_api::FunctionCallError;
|
||||
|
||||
#[test]
|
||||
fn search_errors_preserve_short_messages_and_bound_utf8_output() {
|
||||
let fits = "x".repeat(/*n*/ 1024);
|
||||
assert_eq!(search_error_message(&fits), fits);
|
||||
|
||||
let oversized = "🦀".repeat(/*n*/ 512);
|
||||
let message = search_error_message(&oversized);
|
||||
assert!(message.len() <= 1024);
|
||||
assert!(oversized.starts_with(message.strip_suffix("… [truncated]").unwrap()));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn search_request_headers_forward_thread_originator_and_turn_metadata() {
|
||||
let headers = search_request_headers(Some("chatgpt_cca"), Some("turn-metadata"));
|
||||
|
||||
Reference in New Issue
Block a user