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

@@ -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]]

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 })
     }
 

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(())
+}

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;

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

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::<codex_http_client::RouteAwareRequestError>()
+                .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());