From 997a80020f5bb129c6804e8046d09fb0231751c6 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Tue, 18 Aug 2026 21:22:14 +0000 Subject: [PATCH] Report diagnostic upload failures (#39287) ## Why Submitting a diagnostic report could appear successful without confirming that Sentry accepted the upload, leaving callers unable to detect transport or HTTP failures. ## What changed - Send report envelopes through the route-aware HTTP client and await the response. - Reject redirects, propagate transport and non-success HTTP responses, and include the full error chain in upload JSON-RPC errors. - Add structured logs for upload attempts, successes, and failures without including report contents. ## Testing Added coverage for successful uploads, rejected responses, blocked redirects, transport failures, and app-server JSON-RPC error reporting. GitOrigin-RevId: 07b5cfccd7a65f35d51a720537d2cd8962ed6cdb --- codex-rs/Cargo.lock | 3 + .../request_processors/feedback_processor.rs | 28 ++- .../app-server/tests/suite/v2/feedback.rs | 48 ++++ codex-rs/app-server/tests/suite/v2/mod.rs | 1 + codex-rs/feedback/Cargo.toml | 3 + codex-rs/feedback/src/lib.rs | 224 ++++++++++++++++-- 6 files changed, 280 insertions(+), 27 deletions(-) create mode 100644 codex-rs/app-server/tests/suite/v2/feedback.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 47df1e8c56..d1ccdc8632 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3179,14 +3179,17 @@ name = "codex-feedback" version = "0.0.0" dependencies = [ "anyhow", + "codex-http-client", "codex-login", "codex-protocol", "log", "mime_guess", "pretty_assertions", "sentry", + "tokio", "tracing", "tracing-subscriber", + "wiremock", ] [[package]] diff --git a/codex-rs/app-server/src/request_processors/feedback_processor.rs b/codex-rs/app-server/src/request_processors/feedback_processor.rs index ee4381c0f1..e6a805af6d 100644 --- a/codex-rs/app-server/src/request_processors/feedback_processor.rs +++ b/codex-rs/app-server/src/request_processors/feedback_processor.rs @@ -261,19 +261,24 @@ impl FeedbackRequestProcessor { } let session_source = self.thread_manager.session_source(); + let http_client_factory = self.config.http_client_factory(); + let runtime_handle = tokio::runtime::Handle::current(); let upload_result = tokio::task::spawn_blocking(move || { let tags = (!upload_tags.is_empty()).then_some(&upload_tags); - snapshot.upload_feedback(FeedbackUploadOptions { - classification: &classification, - reason: reason.as_deref(), - tags, - include_logs, - extra_attachments: &extra_attachments, - extra_attachment_paths: &attachment_paths, - session_source: Some(session_source), - logs_override: sqlite_feedback_logs, - }) + runtime_handle.block_on(snapshot.upload_feedback( + FeedbackUploadOptions { + classification: &classification, + reason: reason.as_deref(), + tags, + include_logs, + extra_attachments: &extra_attachments, + extra_attachment_paths: &attachment_paths, + session_source: Some(session_source), + logs_override: sqlite_feedback_logs, + }, + &http_client_factory, + )) }) .await; @@ -286,7 +291,8 @@ impl FeedbackRequestProcessor { } }; - upload_result.map_err(|err| internal_error(format!("failed to upload feedback: {err}")))?; + upload_result + .map_err(|err| internal_error(format!("failed to upload feedback: {err:#}")))?; Ok(FeedbackUploadResponse { thread_id }) } diff --git a/codex-rs/app-server/tests/suite/v2/feedback.rs b/codex-rs/app-server/tests/suite/v2/feedback.rs new file mode 100644 index 0000000000..d2e21dc5d0 --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/feedback.rs @@ -0,0 +1,48 @@ +use std::time::Duration; + +use anyhow::Result; +use app_test_support::TestAppServer; +use codex_app_server_protocol::RequestId; +use pretty_assertions::assert_eq; +use serde_json::json; +use tokio::time::timeout; +use wiremock::Mock; +use wiremock::MockServer; +use wiremock::ResponseTemplate; +use wiremock::matchers::method; + +#[tokio::test] +async fn feedback_upload_reports_transport_failure_as_json_rpc_error() -> Result<()> { + let proxy = MockServer::start().await; + Mock::given(method("CONNECT")) + .respond_with(ResponseTemplate::new(503)) + .expect(1) + .mount(&proxy) + .await; + let proxy_uri = proxy.uri(); + let mut app_server = TestAppServer::builder() + .with_env_overrides(&[ + ("HTTPS_PROXY", Some(proxy_uri.as_str())), + ("https_proxy", Some(proxy_uri.as_str())), + ("NO_PROXY", Some("")), + ("no_proxy", Some("")), + ]) + .build_initialized() + .await?; + + let request_id = app_server + .send_raw_request( + "feedback/upload", + Some(json!({ "classification": "bug", "includeLogs": false })), + ) + .await?; + let error = timeout( + Duration::from_secs(15), + app_server.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + + assert_eq!(error.error.code, -32603); + assert!(error.error.message.contains("failed to upload feedback")); + Ok(()) +} diff --git a/codex-rs/app-server/tests/suite/v2/mod.rs b/codex-rs/app-server/tests/suite/v2/mod.rs index 75a7952eec..5bec9aaa12 100644 --- a/codex-rs/app-server/tests/suite/v2/mod.rs +++ b/codex-rs/app-server/tests/suite/v2/mod.rs @@ -31,6 +31,7 @@ mod experimental_api; mod experimental_feature_list; mod external_agent_config; mod external_agent_import_sync; +mod feedback; mod fs; mod git_attribution; mod guardian_v2; diff --git a/codex-rs/feedback/Cargo.toml b/codex-rs/feedback/Cargo.toml index 7cfdc9327d..8a01696952 100644 --- a/codex-rs/feedback/Cargo.toml +++ b/codex-rs/feedback/Cargo.toml @@ -9,6 +9,7 @@ workspace = true [dependencies] anyhow = { workspace = true } +codex-http-client = { workspace = true } codex-login = { workspace = true } codex-protocol = { workspace = true } mime_guess = { workspace = true } @@ -19,6 +20,8 @@ tracing-subscriber = { workspace = true } [dev-dependencies] log = { workspace = true } pretty_assertions = { workspace = true } +tokio = { workspace = true, features = ["macros", "rt"] } +wiremock = { workspace = true } [lib] doctest = false diff --git a/codex-rs/feedback/src/lib.rs b/codex-rs/feedback/src/lib.rs index e4c0a10042..dad90f56a6 100644 --- a/codex-rs/feedback/src/lib.rs +++ b/codex-rs/feedback/src/lib.rs @@ -9,9 +9,14 @@ use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; use std::time::Duration; +use std::time::Instant; +use anyhow::Context; use anyhow::Result; use anyhow::anyhow; +use codex_http_client::ClientRouteClass; +use codex_http_client::HttpClientFactory; +use codex_http_client::RouteAwareClientPool; use codex_login::AuthEnvTelemetry; use codex_protocol::ThreadId; use codex_protocol::protocol::SessionSource; @@ -420,25 +425,35 @@ impl FeedbackSnapshot { } /// Upload feedback to Sentry with optional attachments. - pub fn upload_feedback(&self, options: FeedbackUploadOptions<'_>) -> Result<()> { - use std::str::FromStr; - use std::sync::Arc; + pub async fn upload_feedback( + &self, + options: FeedbackUploadOptions<'_>, + http_client_factory: &HttpClientFactory, + ) -> Result<()> { + self.upload_feedback_with_dsn(options, http_client_factory, SENTRY_DSN) + .await + } + + async fn upload_feedback_with_dsn( + &self, + options: FeedbackUploadOptions<'_>, + http_client_factory: &HttpClientFactory, + dsn: &str, + ) -> Result<()> { + use std::str::FromStr; - use sentry::Client; use sentry::ClientOptions; use sentry::protocol::Envelope; use sentry::protocol::EnvelopeItem; use sentry::protocol::Event; use sentry::protocol::Level; - use sentry::transports::DefaultTransportFactory; use sentry::types::Dsn; - // Build Sentry client - let client = Client::from_config(ClientOptions { - dsn: Some(Dsn::from_str(SENTRY_DSN).map_err(|e| anyhow!("invalid DSN: {e}"))?), - transport: Some(Arc::new(DefaultTransportFactory {})), - ..Default::default() - }); + let started_at = Instant::now(); + let dsn = Dsn::from_str(dsn).map_err(|error| anyhow!("invalid DSN: {error}"))?; + let upload_url = dsn.envelope_api_url(); + let sentry_options = ClientOptions::default(); + let sentry_auth = dsn.to_auth(Some(sentry_options.user_agent.as_ref())); let tags = self.upload_tags( options.classification, @@ -477,18 +492,80 @@ impl FeedbackSnapshot { } envelope.add_item(EnvelopeItem::Event(event)); - for attachment in self.feedback_attachments( + let attachments = self.feedback_attachments( options.include_logs, options.extra_attachments, options.extra_attachment_paths, options.logs_override, - ) { + ); + let attachment_count = attachments.len(); + for attachment in attachments { envelope.add_item(EnvelopeItem::Attachment(attachment)); } - client.send_envelope(envelope); - client.flush(Some(Duration::from_secs(UPLOAD_TIMEOUT_SECS))); - Ok(()) + let mut body = Vec::new(); + envelope + .to_writer(&mut body) + .context("failed to serialize feedback upload")?; + + tracing::info!( + thread_id = %self.thread_id, + classification = options.classification, + include_logs = options.include_logs, + attachment_count, + payload_bytes = body.len(), + "uploading feedback to Sentry" + ); + + let mut status = None; + let result: Result<()> = async { + let client_pool = RouteAwareClientPool::new_without_redirects( + http_client_factory.clone(), + ClientRouteClass::Other, + ); + let response = client_pool + .post(upload_url.as_str()) + .header("X-Sentry-Auth", sentry_auth.to_string()) + .body(body) + .timeout(Duration::from_secs(UPLOAD_TIMEOUT_SECS)) + .send() + .await + .context("failed to upload feedback to Sentry")?; + let response_status = response.status(); + status = Some(response_status.as_u16()); + response + .error_for_status() + .context("failed to upload feedback to Sentry")?; + anyhow::ensure!( + response_status.is_success(), + "Sentry rejected feedback upload with HTTP status {response_status}" + ); + Ok(()) + } + .await; + + match &result { + Ok(()) => tracing::info!( + thread_id = %self.thread_id, + classification = options.classification, + include_logs = options.include_logs, + status, + elapsed_ms = started_at.elapsed().as_millis(), + "feedback uploaded to Sentry" + ), + Err(error) => { + tracing::warn!( + thread_id = %self.thread_id, + classification = options.classification, + include_logs = options.include_logs, + status, + elapsed_ms = started_at.elapsed().as_millis(), + error = %format!("{error:#}"), + "feedback upload failed" + ) + } + } + result } fn upload_tags( @@ -709,9 +786,16 @@ mod tests { use super::*; use crate::FeedbackDiagnostic; + use codex_http_client::OutboundProxyPolicy; use pretty_assertions::assert_eq; use tracing_subscriber::layer::SubscriberExt; use tracing_subscriber::util::SubscriberInitExt; + use wiremock::Mock; + use wiremock::MockServer; + use wiremock::ResponseTemplate; + use wiremock::matchers::header_exists; + use wiremock::matchers::method; + use wiremock::matchers::path; #[test] fn ring_buffer_drops_front_when_full() { @@ -787,6 +871,114 @@ mod tests { pretty_assertions::assert_eq!(snap.tags.get("cached").map(String::as_str), Some("true")); } + async fn upload_test_feedback(feedback: &CodexFeedback, dsn: &str) -> Result<()> { + feedback + .snapshot(/*session_id*/ None) + .upload_feedback_with_dsn( + FeedbackUploadOptions { + classification: "bug", + reason: Some("private feedback"), + tags: None, + include_logs: true, + extra_attachments: &[], + extra_attachment_paths: &[], + session_source: Some(SessionSource::Cli), + logs_override: Some(b"private log contents".to_vec()), + }, + &HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault), + dsn, + ) + .await + } + + #[tokio::test] + async fn feedback_upload_waits_for_successful_sentry_response() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/api/42/envelope/")) + .and(header_exists("X-Sentry-Auth")) + .respond_with(ResponseTemplate::new(200)) + .expect(1) + .mount(&server) + .await; + + let dsn = format!("http://public@{}/42", server.address()); + upload_test_feedback(&CodexFeedback::new(), &dsn) + .await + .expect("successful Sentry response should complete feedback upload"); + } + + #[tokio::test] + async fn feedback_upload_reports_rejected_sentry_response_without_exposing_feedback() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/api/42/envelope/")) + .respond_with(ResponseTemplate::new(503)) + .expect(1) + .mount(&server) + .await; + + let feedback = CodexFeedback::new(); + let dsn = format!("http://public@{}/42", server.address()); + + let error = upload_test_feedback(&feedback, &dsn) + .await + .expect_err("rejected Sentry responses must fail feedback uploads"); + + let error = format!("{error:#}"); + assert!(error.contains("503")); + assert!(!error.contains("private feedback")); + } + + #[tokio::test] + async fn feedback_upload_does_not_forward_private_data_to_redirect_target() { + let sentry_server = MockServer::start().await; + let redirect_target = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/api/42/envelope/")) + .respond_with( + ResponseTemplate::new(307) + .insert_header("Location", format!("{}/capture", redirect_target.uri())), + ) + .expect(1) + .mount(&sentry_server) + .await; + let dsn = format!("http://public@{}/42", sentry_server.address()); + let error = upload_test_feedback(&CodexFeedback::new(), &dsn) + .await + .expect_err("redirected feedback uploads must be rejected"); + + assert!(error.to_string().contains("307")); + assert!( + redirect_target + .received_requests() + .await + .expect("redirect target should record requests") + .is_empty() + ); + } + + #[tokio::test] + async fn feedback_upload_reports_transport_failures() { + let listener = std::net::TcpListener::bind("127.0.0.1:0") + .expect("unused local address should be available"); + let address = listener + .local_addr() + .expect("listener should have an address"); + drop(listener); + + let dsn = format!("http://public@{address}/42"); + let error = upload_test_feedback(&CodexFeedback::new(), &dsn) + .await + .expect_err("transport failures must fail feedback uploads"); + + assert!( + error + .downcast_ref::() + .is_some_and(codex_http_client::RouteAwareRequestError::is_connect) + ); + } + #[test] fn feedback_attachments_gate_connectivity_diagnostics() { let extra_filename = format!("codex-feedback-extra-{}.jsonl", ThreadId::new());