From 184c9c555ec7b30bacde5a5bdfa92ff156315aa3 Mon Sep 17 00:00:00 2001 From: sgarg-oai Date: Thu, 29 Oct 2026 18:30:12 +0000 Subject: [PATCH] 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 --- .../tests/suite/v2/web_search_routing.rs | 76 ++++++++++++++++--- codex-rs/ext/web-search/src/output.rs | 55 ++++++-------- codex-rs/ext/web-search/src/routing_tests.rs | 2 +- codex-rs/ext/web-search/src/tool.rs | 35 +++++++-- 4 files changed, 117 insertions(+), 51 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/web_search_routing.rs b/codex-rs/app-server/tests/suite/v2/web_search_routing.rs index c378e1f1dd..d9c45ed7de 100644 --- a/codex-rs/app-server/tests/suite/v2/web_search_routing.rs +++ b/codex-rs/app-server/tests/suite/v2/web_search_routing.rs @@ -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) diff --git a/codex-rs/ext/web-search/src/output.rs b/codex-rs/ext/web-search/src/output.rs index bf305fc4ea..8187ca2979 100644 --- a/codex-rs/ext/web-search/src/output.rs +++ b/codex-rs/ext/web-search/src/output.rs @@ -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) -> 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)); + } } } diff --git a/codex-rs/ext/web-search/src/routing_tests.rs b/codex-rs/ext/web-search/src/routing_tests.rs index ec278d3b84..01fc407752 100644 --- a/codex-rs/ext/web-search/src/routing_tests.rs +++ b/codex-rs/ext/web-search/src/routing_tests.rs @@ -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 )); } diff --git a/codex-rs/ext/web-search/src/tool.rs b/codex-rs/ext/web-search/src/tool.rs index d7307bc2ae..4b92af27d0 100644 --- a/codex-rs/ext/web-search/src/tool.rs +++ b/codex-rs/ext/web-search/src/tool.rs @@ -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"));