diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index d9ee0f608bf..8c0920c65d2 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1997,6 +1997,7 @@ dependencies = [ "sentry", "tracing", "tracing-subscriber", + "url", ] [[package]] diff --git a/codex-rs/feedback/Cargo.toml b/codex-rs/feedback/Cargo.toml index 73803af86a3..43d572f895a 100644 --- a/codex-rs/feedback/Cargo.toml +++ b/codex-rs/feedback/Cargo.toml @@ -10,6 +10,7 @@ codex-protocol = { workspace = true } sentry = { version = "0.46" } tracing = { workspace = true } tracing-subscriber = { workspace = true } +url = { workspace = true } [dev-dependencies] pretty_assertions = { workspace = true } diff --git a/codex-rs/feedback/src/feedback_diagnostics.rs b/codex-rs/feedback/src/feedback_diagnostics.rs new file mode 100644 index 00000000000..5497fe0579e --- /dev/null +++ b/codex-rs/feedback/src/feedback_diagnostics.rs @@ -0,0 +1,247 @@ +use std::collections::HashMap; + +use url::Url; + +const DEFAULT_OPENAI_BASE_URL: &str = "https://api.openai.com/v1"; +const OPENAI_BASE_URL_ENV_VAR: &str = "OPENAI_BASE_URL"; +pub const FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME: &str = "codex-connectivity-diagnostics.txt"; +const PROXY_ENV_VARS: &[&str] = &[ + "HTTP_PROXY", + "http_proxy", + "HTTPS_PROXY", + "https_proxy", + "ALL_PROXY", + "all_proxy", +]; + +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct FeedbackDiagnostics { + diagnostics: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct FeedbackDiagnostic { + pub headline: String, + pub details: Vec, +} + +impl FeedbackDiagnostics { + pub fn new(diagnostics: Vec) -> Self { + Self { diagnostics } + } + + pub fn collect_from_env() -> Self { + Self::collect_from_pairs(std::env::vars()) + } + + fn collect_from_pairs(pairs: I) -> Self + where + I: IntoIterator, + K: Into, + V: Into, + { + let env = pairs + .into_iter() + .map(|(key, value)| (key.into(), value.into())) + .collect::>(); + let mut diagnostics = Vec::new(); + + let proxy_details = PROXY_ENV_VARS + .iter() + .filter_map(|key| { + let value = env.get(*key)?.trim(); + if value.is_empty() { + return None; + } + + let detail = match sanitize_proxy_value(value) { + Some(sanitized) => format!("{key} = {sanitized}"), + None => format!("{key} = invalid value"), + }; + Some(detail) + }) + .collect::>(); + if !proxy_details.is_empty() { + diagnostics.push(FeedbackDiagnostic { + headline: "Proxy environment variables are set and may affect connectivity." + .to_string(), + details: proxy_details, + }); + } + + if let Some(value) = env.get(OPENAI_BASE_URL_ENV_VAR).map(String::as_str) { + let trimmed = value.trim(); + if !trimmed.is_empty() && trimmed.trim_end_matches('/') != DEFAULT_OPENAI_BASE_URL { + let detail = match sanitize_url_for_display(trimmed) { + Some(sanitized) => format!("{OPENAI_BASE_URL_ENV_VAR} = {sanitized}"), + None => format!("{OPENAI_BASE_URL_ENV_VAR} = invalid value"), + }; + diagnostics.push(FeedbackDiagnostic { + headline: "OPENAI_BASE_URL is set and may affect connectivity.".to_string(), + details: vec![detail], + }); + } + } + + Self { diagnostics } + } + + pub fn is_empty(&self) -> bool { + self.diagnostics.is_empty() + } + + pub fn diagnostics(&self) -> &[FeedbackDiagnostic] { + &self.diagnostics + } + + pub fn attachment_text(&self) -> Option { + if self.diagnostics.is_empty() { + return None; + } + + let mut lines = vec!["Connectivity diagnostics".to_string(), String::new()]; + for diagnostic in &self.diagnostics { + lines.push(format!("- {}", diagnostic.headline)); + lines.extend( + diagnostic + .details + .iter() + .map(|detail| format!(" - {detail}")), + ); + } + + Some(lines.join("\n")) + } +} + +pub fn sanitize_url_for_display(raw: &str) -> Option { + let trimmed = raw.trim(); + if trimmed.is_empty() { + return None; + } + + let Ok(mut url) = Url::parse(trimmed) else { + return None; + }; + let _ = url.set_username(""); + let _ = url.set_password(None); + url.set_query(None); + url.set_fragment(None); + Some(url.to_string().trim_end_matches('/').to_string()).filter(|value| !value.is_empty()) +} + +fn sanitize_proxy_value(raw: &str) -> Option { + if raw.contains("://") { + return sanitize_url_for_display(raw); + } + + sanitize_url_for_display(&format!("http://{raw}")) +} + +#[cfg(test)] +mod tests { + use pretty_assertions::assert_eq; + + use super::FeedbackDiagnostic; + use super::FeedbackDiagnostics; + use super::sanitize_url_for_display; + + #[test] + fn collect_from_pairs_reports_sanitized_diagnostics_and_attachment() { + let diagnostics = FeedbackDiagnostics::collect_from_pairs([ + ( + "HTTPS_PROXY", + "https://user:password@secure-proxy.example.com:443?secret=1", + ), + ("http_proxy", "proxy.example.com:8080"), + ("all_proxy", "socks5h://all-proxy.example.com:1080"), + ("OPENAI_BASE_URL", "https://example.com/v1?token=secret"), + ]); + + assert_eq!( + diagnostics, + FeedbackDiagnostics { + diagnostics: vec![ + FeedbackDiagnostic { + headline: + "Proxy environment variables are set and may affect connectivity." + .to_string(), + details: vec![ + "http_proxy = http://proxy.example.com:8080".to_string(), + "HTTPS_PROXY = https://secure-proxy.example.com".to_string(), + "all_proxy = socks5h://all-proxy.example.com:1080".to_string(), + ], + }, + FeedbackDiagnostic { + headline: "OPENAI_BASE_URL is set and may affect connectivity.".to_string(), + details: vec!["OPENAI_BASE_URL = https://example.com/v1".to_string()], + }, + ], + } + ); + + assert_eq!( + diagnostics.attachment_text(), + Some( + "Connectivity diagnostics\n\n- Proxy environment variables are set and may affect connectivity.\n - http_proxy = http://proxy.example.com:8080\n - HTTPS_PROXY = https://secure-proxy.example.com\n - all_proxy = socks5h://all-proxy.example.com:1080\n- OPENAI_BASE_URL is set and may affect connectivity.\n - OPENAI_BASE_URL = https://example.com/v1" + .to_string() + ) + ); + } + + #[test] + fn collect_from_pairs_ignores_absent_and_default_values() { + for diagnostics in [ + FeedbackDiagnostics::collect_from_pairs(Vec::<(String, String)>::new()), + FeedbackDiagnostics::collect_from_pairs([( + "OPENAI_BASE_URL", + "https://api.openai.com/v1/", + )]), + ] { + assert_eq!(diagnostics, FeedbackDiagnostics::default()); + assert_eq!(diagnostics.attachment_text(), None); + } + } + + #[test] + fn collect_from_pairs_reports_invalid_values_without_echoing_them() { + let invalid_proxy = "not a valid\nproxy"; + let invalid_base_url = "not a valid\nurl"; + let diagnostics = FeedbackDiagnostics::collect_from_pairs([ + ("HTTP_PROXY", invalid_proxy), + ("OPENAI_BASE_URL", invalid_base_url), + ]); + + assert_eq!( + diagnostics, + FeedbackDiagnostics { + diagnostics: vec![ + FeedbackDiagnostic { + headline: + "Proxy environment variables are set and may affect connectivity." + .to_string(), + details: vec!["HTTP_PROXY = invalid value".to_string()], + }, + FeedbackDiagnostic { + headline: "OPENAI_BASE_URL is set and may affect connectivity.".to_string(), + details: vec!["OPENAI_BASE_URL = invalid value".to_string()], + }, + ], + } + ); + let attachment_text = diagnostics + .attachment_text() + .expect("invalid diagnostics should still render attachment text"); + assert!(!attachment_text.contains(invalid_proxy)); + assert!(!attachment_text.contains(invalid_base_url)); + } + + #[test] + fn sanitize_url_for_display_strips_credentials_query_and_fragment() { + let sanitized = sanitize_url_for_display( + "https://user:password@example.com:8443/v1?token=secret#fragment", + ); + + assert_eq!(sanitized, Some("https://example.com:8443/v1".to_string())); + } +} diff --git a/codex-rs/feedback/src/lib.rs b/codex-rs/feedback/src/lib.rs index ee8e98c39ee..0bd76936aa1 100644 --- a/codex-rs/feedback/src/lib.rs +++ b/codex-rs/feedback/src/lib.rs @@ -13,6 +13,8 @@ use anyhow::Result; use anyhow::anyhow; use codex_protocol::ThreadId; use codex_protocol::protocol::SessionSource; +use feedback_diagnostics::FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME; +use feedback_diagnostics::FeedbackDiagnostics; use tracing::Event; use tracing::Level; use tracing::field::Visit; @@ -21,6 +23,8 @@ use tracing_subscriber::filter::Targets; use tracing_subscriber::fmt::writer::MakeWriter; use tracing_subscriber::registry::LookupSpan; +pub mod feedback_diagnostics; + const DEFAULT_MAX_BYTES: usize = 4 * 1024 * 1024; // 4 MiB const SENTRY_DSN: &str = "https://ae32ed50620d7a7792c1ce5df38b3e3e@o33249.ingest.us.sentry.io/4510195390611458"; @@ -88,7 +92,7 @@ impl CodexFeedback { .with_filter(Targets::new().with_target(FEEDBACK_TAGS_TARGET, Level::TRACE)) } - pub fn snapshot(&self, session_id: Option) -> CodexLogSnapshot { + pub fn snapshot(&self, session_id: Option) -> FeedbackSnapshot { let bytes = { let guard = self.inner.ring.lock().expect("mutex poisoned"); guard.snapshot_bytes() @@ -97,9 +101,10 @@ impl CodexFeedback { let guard = self.inner.tags.lock().expect("mutex poisoned"); guard.clone() }; - CodexLogSnapshot { + FeedbackSnapshot { bytes, tags, + feedback_diagnostics: FeedbackDiagnostics::collect_from_env(), thread_id: session_id .map(|id| id.to_string()) .unwrap_or("no-active-thread-".to_string() + &ThreadId::new().to_string()), @@ -199,17 +204,35 @@ impl RingBuffer { } } -pub struct CodexLogSnapshot { +pub struct FeedbackSnapshot { bytes: Vec, tags: BTreeMap, + feedback_diagnostics: FeedbackDiagnostics, pub thread_id: String, } -impl CodexLogSnapshot { +impl FeedbackSnapshot { pub(crate) fn as_bytes(&self) -> &[u8] { &self.bytes } + pub fn feedback_diagnostics(&self) -> &FeedbackDiagnostics { + &self.feedback_diagnostics + } + + pub fn with_feedback_diagnostics(mut self, feedback_diagnostics: FeedbackDiagnostics) -> Self { + self.feedback_diagnostics = feedback_diagnostics; + self + } + + pub fn feedback_diagnostics_attachment_text(&self, include_logs: bool) -> Option { + if !include_logs { + return None; + } + + self.feedback_diagnostics.attachment_text() + } + pub fn save_to_temp_file(&self) -> io::Result { let dir = std::env::temp_dir(); let filename = format!("codex-feedback-{}.log", self.thread_id); @@ -224,18 +247,16 @@ impl CodexLogSnapshot { classification: &str, reason: Option<&str>, include_logs: bool, - extra_log_files: &[PathBuf], + extra_attachment_paths: &[PathBuf], session_source: Option, logs_override: Option>, ) -> Result<()> { use std::collections::BTreeMap; - use std::fs; use std::str::FromStr; use std::sync::Arc; use sentry::Client; use sentry::ClientOptions; - use sentry::protocol::Attachment; use sentry::protocol::Envelope; use sentry::protocol::EnvelopeItem; use sentry::protocol::Event; @@ -309,16 +330,46 @@ impl CodexLogSnapshot { } envelope.add_item(EnvelopeItem::Event(event)); + for attachment in + self.feedback_attachments(include_logs, extra_attachment_paths, logs_override) + { + envelope.add_item(EnvelopeItem::Attachment(attachment)); + } + + client.send_envelope(envelope); + client.flush(Some(Duration::from_secs(UPLOAD_TIMEOUT_SECS))); + Ok(()) + } + + fn feedback_attachments( + &self, + include_logs: bool, + extra_attachment_paths: &[PathBuf], + logs_override: Option>, + ) -> Vec { + use sentry::protocol::Attachment; + + let mut attachments = Vec::new(); + if include_logs { - envelope.add_item(EnvelopeItem::Attachment(Attachment { + attachments.push(Attachment { buffer: logs_override.unwrap_or_else(|| self.bytes.clone()), filename: String::from("codex-logs.log"), content_type: Some("text/plain".to_string()), ty: None, - })); + }); } - for path in extra_log_files { + if let Some(text) = self.feedback_diagnostics_attachment_text(include_logs) { + attachments.push(Attachment { + buffer: text.into_bytes(), + filename: FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME.to_string(), + content_type: Some("text/plain".to_string()), + ty: None, + }); + } + + for path in extra_attachment_paths { let data = match fs::read(path) { Ok(data) => data, Err(err) => { @@ -330,22 +381,19 @@ impl CodexLogSnapshot { continue; } }; - let fname = path + let filename = path .file_name() .map(|s| s.to_string_lossy().to_string()) .unwrap_or_else(|| "extra-log.log".to_string()); - let content_type = "text/plain".to_string(); - envelope.add_item(EnvelopeItem::Attachment(Attachment { + attachments.push(Attachment { buffer: data, - filename: fname, - content_type: Some(content_type), + filename, + content_type: Some("text/plain".to_string()), ty: None, - })); + }); } - client.send_envelope(envelope); - client.flush(Some(Duration::from_secs(UPLOAD_TIMEOUT_SECS))); - Ok(()) + attachments } } @@ -430,7 +478,12 @@ impl Visit for FeedbackTagsVisitor { #[cfg(test)] mod tests { + use std::ffi::OsStr; + use std::fs; + use super::*; + use feedback_diagnostics::FeedbackDiagnostic; + use pretty_assertions::assert_eq; use tracing_subscriber::layer::SubscriberExt; use tracing_subscriber::util::SubscriberInitExt; @@ -460,4 +513,59 @@ mod tests { pretty_assertions::assert_eq!(snap.tags.get("model").map(String::as_str), Some("gpt-5")); pretty_assertions::assert_eq!(snap.tags.get("cached").map(String::as_str), Some("true")); } + + #[test] + fn feedback_attachments_gate_connectivity_diagnostics() { + let extra_filename = format!("codex-feedback-extra-{}.jsonl", ThreadId::new()); + let extra_path = std::env::temp_dir().join(&extra_filename); + fs::write(&extra_path, "rollout").expect("extra attachment should be written"); + + let snapshot_with_diagnostics = CodexFeedback::new() + .snapshot(None) + .with_feedback_diagnostics(FeedbackDiagnostics::new(vec![FeedbackDiagnostic { + headline: "OPENAI_BASE_URL is set and may affect connectivity.".to_string(), + details: vec!["OPENAI_BASE_URL = https://example.com/v1".to_string()], + }])); + + let attachments_with_diagnostics = snapshot_with_diagnostics.feedback_attachments( + true, + std::slice::from_ref(&extra_path), + Some(vec![1]), + ); + + assert_eq!( + attachments_with_diagnostics + .iter() + .map(|attachment| attachment.filename.as_str()) + .collect::>(), + vec![ + "codex-logs.log", + FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME, + extra_filename.as_str() + ] + ); + assert_eq!(attachments_with_diagnostics[0].buffer, vec![1]); + assert_eq!( + attachments_with_diagnostics[1].buffer, + b"Connectivity diagnostics\n\n- OPENAI_BASE_URL is set and may affect connectivity.\n - OPENAI_BASE_URL = https://example.com/v1".to_vec() + ); + assert_eq!(attachments_with_diagnostics[2].buffer, b"rollout".to_vec()); + assert_eq!( + OsStr::new(attachments_with_diagnostics[2].filename.as_str()), + OsStr::new(extra_filename.as_str()) + ); + let attachments_without_diagnostics = CodexFeedback::new() + .snapshot(None) + .feedback_attachments(true, &[], Some(vec![1])); + + assert_eq!( + attachments_without_diagnostics + .iter() + .map(|attachment| attachment.filename.as_str()) + .collect::>(), + vec!["codex-logs.log"] + ); + assert_eq!(attachments_without_diagnostics[0].buffer, vec![1]); + fs::remove_file(extra_path).expect("extra attachment should be removed"); + } } diff --git a/codex-rs/tui/src/bottom_pane/feedback_view.rs b/codex-rs/tui/src/bottom_pane/feedback_view.rs index 33368faa45b..f9bbfe24368 100644 --- a/codex-rs/tui/src/bottom_pane/feedback_view.rs +++ b/codex-rs/tui/src/bottom_pane/feedback_view.rs @@ -1,6 +1,8 @@ use std::cell::RefCell; use std::path::PathBuf; +use codex_feedback::feedback_diagnostics::FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME; +use codex_feedback::feedback_diagnostics::FeedbackDiagnostics; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyModifiers; @@ -19,6 +21,8 @@ use crate::app_event::FeedbackCategory; use crate::app_event_sender::AppEventSender; use crate::history_cell; use crate::render::renderable::Renderable; +use crate::wrapping::RtOptions; +use crate::wrapping::word_wrap_lines; use codex_protocol::protocol::SessionSource; use super::CancellationEvent; @@ -46,7 +50,7 @@ pub(crate) enum FeedbackAudience { /// both logs and rollout with classification + metadata. pub(crate) struct FeedbackNoteView { category: FeedbackCategory, - snapshot: codex_feedback::CodexLogSnapshot, + snapshot: codex_feedback::FeedbackSnapshot, rollout_path: Option, app_event_tx: AppEventSender, include_logs: bool, @@ -61,7 +65,7 @@ pub(crate) struct FeedbackNoteView { impl FeedbackNoteView { pub(crate) fn new( category: FeedbackCategory, - snapshot: codex_feedback::CodexLogSnapshot, + snapshot: codex_feedback::FeedbackSnapshot, rollout_path: Option, app_event_tx: AppEventSender, include_logs: bool, @@ -87,7 +91,7 @@ impl FeedbackNoteView { } else { Some(note.as_str()) }; - let log_file_paths = if self.include_logs { + let attachment_paths = if self.include_logs { self.rollout_path.iter().cloned().collect::>() } else { Vec::new() @@ -100,7 +104,7 @@ impl FeedbackNoteView { classification, reason_opt, self.include_logs, - &log_file_paths, + &attachment_paths, Some(SessionSource::Cli), None, ); @@ -217,21 +221,21 @@ impl BottomPaneView for FeedbackNoteView { impl Renderable for FeedbackNoteView { fn desired_height(&self, width: u16) -> u16 { - 1u16 + self.input_height(width) + 3u16 + self.intro_lines(width).len() as u16 + self.input_height(width) + 2u16 } fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { if area.height < 2 || area.width <= 2 { return None; } + let intro_height = self.intro_lines(area.width).len() as u16; let text_area_height = self.input_height(area.width).saturating_sub(1); if text_area_height == 0 { return None; } - let top_line_count = 1u16; // title only let textarea_rect = Rect { x: area.x.saturating_add(2), - y: area.y.saturating_add(top_line_count).saturating_add(1), + y: area.y.saturating_add(intro_height).saturating_add(1), width: area.width.saturating_sub(2), height: text_area_height, }; @@ -244,23 +248,26 @@ impl Renderable for FeedbackNoteView { return; } - let (title, placeholder) = feedback_title_and_placeholder(self.category); + let intro_lines = self.intro_lines(area.width); + let (_, placeholder) = feedback_title_and_placeholder(self.category); let input_height = self.input_height(area.width); - // Title line - let title_area = Rect { - x: area.x, - y: area.y, - width: area.width, - height: 1, - }; - let title_spans: Vec> = vec![gutter(), title.bold()]; - Paragraph::new(Line::from(title_spans)).render(title_area, buf); + for (offset, line) in intro_lines.iter().enumerate() { + Paragraph::new(line.clone()).render( + Rect { + x: area.x, + y: area.y.saturating_add(offset as u16), + width: area.width, + height: 1, + }, + buf, + ); + } // Input line let input_area = Rect { x: area.x, - y: area.y.saturating_add(1), + y: area.y.saturating_add(intro_lines.len() as u16), width: area.width, height: input_height, }; @@ -334,6 +341,56 @@ impl FeedbackNoteView { let text_height = self.textarea.desired_height(usable_width).clamp(1, 8); text_height.saturating_add(1).min(9) } + + fn intro_lines(&self, width: u16) -> Vec> { + let (title, _) = feedback_title_and_placeholder(self.category); + let mut lines = vec![Line::from(vec![gutter(), title.bold()])]; + if should_show_feedback_connectivity_details( + self.category, + self.snapshot.feedback_diagnostics(), + ) { + lines.push(Line::from(vec![gutter()])); + lines.push(Line::from(vec![ + gutter(), + "Connectivity diagnostics".bold(), + ])); + lines.extend(self.diagnostics_lines(width)); + } + lines + } + + fn diagnostics_lines(&self, width: u16) -> Vec> { + let width = usize::from(width.max(1)); + let headline_options = RtOptions::new(width) + .initial_indent(Line::from(vec![gutter(), " - ".into()])) + .subsequent_indent(Line::from(vec![gutter(), " ".into()])); + let detail_options = RtOptions::new(width) + .initial_indent(Line::from(vec![gutter(), " - ".dim()])) + .subsequent_indent(Line::from(vec![gutter(), " ".into()])); + let mut lines = Vec::new(); + + for diagnostic in self.snapshot.feedback_diagnostics().diagnostics() { + lines.extend(word_wrap_lines( + [Line::from(diagnostic.headline.clone())], + headline_options.clone(), + )); + for detail in &diagnostic.details { + lines.extend(word_wrap_lines( + [Line::from(detail.clone())], + detail_options.clone(), + )); + } + } + + lines + } +} + +pub(crate) fn should_show_feedback_connectivity_details( + category: FeedbackCategory, + diagnostics: &FeedbackDiagnostics, +) -> bool { + category != FeedbackCategory::GoodResult && !diagnostics.is_empty() } fn gutter() -> Span<'static> { @@ -485,6 +542,7 @@ pub(crate) fn feedback_upload_consent_params( app_event_tx: AppEventSender, category: FeedbackCategory, rollout_path: Option, + include_connectivity_diagnostics_attachment: bool, ) -> super::SelectionViewParams { use super::popup_consts::standard_popup_hint_line; let yes_action: super::SelectionAction = Box::new({ @@ -521,6 +579,15 @@ pub(crate) fn feedback_upload_consent_params( { header_lines.push(Line::from(vec![" • ".into(), name.into()]).into()); } + if include_connectivity_diagnostics_attachment { + header_lines.push( + Line::from(vec![ + " • ".into(), + FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME.into(), + ]) + .into(), + ); + } super::SelectionViewParams { footer_hint: Some(standard_popup_hint_line()), @@ -537,7 +604,6 @@ pub(crate) fn feedback_upload_consent_params( }, super::SelectionItem { name: "No".to_string(), - description: Some("".to_string()), actions: vec![no_action], dismiss_on_select: true, ..Default::default() @@ -555,6 +621,8 @@ mod tests { use super::*; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; + use codex_feedback::feedback_diagnostics::FeedbackDiagnostic; + use pretty_assertions::assert_eq; fn render(view: &FeedbackNoteView, width: u16) -> String { let height = view.desired_height(width); @@ -635,6 +703,62 @@ mod tests { insta::assert_snapshot!("feedback_view_safety_check", rendered); } + #[test] + fn feedback_view_with_connectivity_diagnostics() { + let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let diagnostics = FeedbackDiagnostics::new(vec![ + FeedbackDiagnostic { + headline: "Proxy environment variables are set and may affect connectivity." + .to_string(), + details: vec!["HTTP_PROXY = http://proxy.example.com:8080".to_string()], + }, + FeedbackDiagnostic { + headline: "OPENAI_BASE_URL is set and may affect connectivity.".to_string(), + details: vec!["OPENAI_BASE_URL = https://example.com/v1".to_string()], + }, + ]); + let snapshot = codex_feedback::CodexFeedback::new() + .snapshot(None) + .with_feedback_diagnostics(diagnostics); + let view = FeedbackNoteView::new( + FeedbackCategory::Bug, + snapshot, + None, + tx, + false, + FeedbackAudience::External, + ); + let rendered = render(&view, 60); + + insta::assert_snapshot!("feedback_view_with_connectivity_diagnostics", rendered); + } + + #[test] + fn should_show_feedback_connectivity_details_only_for_non_good_result_with_diagnostics() { + let diagnostics = FeedbackDiagnostics::new(vec![FeedbackDiagnostic { + headline: "Proxy environment variables are set and may affect connectivity." + .to_string(), + details: vec!["HTTP_PROXY = http://proxy.example.com:8080".to_string()], + }]); + + assert_eq!( + should_show_feedback_connectivity_details(FeedbackCategory::Bug, &diagnostics), + true + ); + assert_eq!( + should_show_feedback_connectivity_details(FeedbackCategory::GoodResult, &diagnostics), + false + ); + assert_eq!( + should_show_feedback_connectivity_details( + FeedbackCategory::BadResult, + &FeedbackDiagnostics::default() + ), + false + ); + } + #[test] fn issue_url_available_for_bug_bad_result_safety_check_and_other() { let bug_url = issue_url_for_category( diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_with_connectivity_diagnostics.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_with_connectivity_diagnostics.snap new file mode 100644 index 00000000000..ec1322ce845 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_with_connectivity_diagnostics.snap @@ -0,0 +1,17 @@ +--- +source: tui/src/bottom_pane/feedback_view.rs +assertion_line: 749 +expression: rendered +--- +▌ Tell us more (bug) +▌ +▌ Connectivity diagnostics +▌ - Proxy environment variables are set and may affect +▌ connectivity. +▌ - HTTP_PROXY = http://proxy.example.com:8080 +▌ - OPENAI_BASE_URL is set and may affect connectivity. +▌ - OPENAI_BASE_URL = https://example.com/v1 +▌ +▌ (optional) Write a short description to help us further + +Press enter to confirm or esc to go back diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 51f39c1f2a8..3c200131564 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1255,8 +1255,16 @@ impl ChatWidget { category: crate::app_event::FeedbackCategory, include_logs: bool, ) { - // Build a fresh snapshot at the time of opening the note overlay. let snapshot = self.feedback.snapshot(self.thread_id); + self.show_feedback_note(category, include_logs, snapshot); + } + + fn show_feedback_note( + &mut self, + category: crate::app_event::FeedbackCategory, + include_logs: bool, + snapshot: codex_feedback::FeedbackSnapshot, + ) { let rollout = if include_logs { self.current_rollout_path.clone() } else { @@ -1281,10 +1289,14 @@ impl ChatWidget { } pub(crate) fn open_feedback_consent(&mut self, category: crate::app_event::FeedbackCategory) { + let snapshot = self.feedback.snapshot(self.thread_id); let params = crate::bottom_pane::feedback_upload_consent_params( self.app_event_tx.clone(), category, self.current_rollout_path.clone(), + snapshot + .feedback_diagnostics_attachment_text(true) + .is_some(), ); self.bottom_pane.show_selection_view(params); self.request_redraw(); diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_consent_popup.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_consent_popup.snap new file mode 100644 index 00000000000..4529d6d478a --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_consent_popup.snap @@ -0,0 +1,15 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: popup +--- + Upload logs? + + The following files will be sent: + • codex-logs.log + • codex-connectivity-diagnostics.txt + +› 1. Yes Share the current Codex session logs with the team for + troubleshooting. + 2. No + + Press enter to confirm or esc to go back diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_upload_consent_popup.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_upload_consent_popup.snap index cc3d8e37559..4529d6d478a 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_upload_consent_popup.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_upload_consent_popup.snap @@ -6,6 +6,7 @@ expression: popup The following files will be sent: • codex-logs.log + • codex-connectivity-diagnostics.txt › 1. Yes Share the current Codex session logs with the team for troubleshooting. diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index a720559b5c3..b7dbdcb4099 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -6436,13 +6436,32 @@ async fn feedback_selection_popup_snapshot() { async fn feedback_upload_consent_popup_snapshot() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; - // Open the consent popup directly for a chosen category. - chat.open_feedback_consent(crate::app_event::FeedbackCategory::Bug); + chat.show_selection_view(crate::bottom_pane::feedback_upload_consent_params( + chat.app_event_tx.clone(), + crate::app_event::FeedbackCategory::Bug, + chat.current_rollout_path.clone(), + true, + )); let popup = render_bottom_popup(&chat, 80); assert_snapshot!("feedback_upload_consent_popup", popup); } +#[tokio::test] +async fn feedback_good_result_consent_popup_includes_connectivity_diagnostics_filename() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + + chat.show_selection_view(crate::bottom_pane::feedback_upload_consent_params( + chat.app_event_tx.clone(), + crate::app_event::FeedbackCategory::GoodResult, + chat.current_rollout_path.clone(), + true, + )); + + let popup = render_bottom_popup(&chat, 80); + assert_snapshot!("feedback_good_result_consent_popup", popup); +} + #[tokio::test] async fn reasoning_popup_escape_returns_to_model_popup() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(Some("gpt-5.1-codex-max")).await;