From 7ede1d77f65261d1a77a4eee0db2b943cd5557fc Mon Sep 17 00:00:00 2001 From: Roy Han Date: Mon, 2 Mar 2026 16:26:41 -0800 Subject: [PATCH 01/14] feedback diagnostics --- codex-rs/core/src/feedback_diagnostics.rs | 308 ++++++++++++++++++ codex-rs/core/src/lib.rs | 1 + codex-rs/tui/src/bottom_pane/feedback_view.rs | 225 +++++++++++-- ...view__tests__feedback_view_bad_result.snap | 4 + ...edback_view__tests__feedback_view_bug.snap | 4 + ...iew__tests__feedback_view_good_result.snap | 4 + ...back_view__tests__feedback_view_other.snap | 4 + ...ew__tests__feedback_view_safety_check.snap | 4 + ...ck_view_with_connectivity_diagnostics.snap | 17 + codex-rs/tui/src/chatwidget.rs | 2 + ..._tests__feedback_upload_consent_popup.snap | 10 +- 11 files changed, 556 insertions(+), 27 deletions(-) create mode 100644 codex-rs/core/src/feedback_diagnostics.rs create mode 100644 codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_with_connectivity_diagnostics.snap diff --git a/codex-rs/core/src/feedback_diagnostics.rs b/codex-rs/core/src/feedback_diagnostics.rs new file mode 100644 index 00000000000..b6b01ebad51 --- /dev/null +++ b/codex-rs/core/src/feedback_diagnostics.rs @@ -0,0 +1,308 @@ +use std::collections::HashMap; +use std::fs; +use std::io; +use std::path::Path; +use std::path::PathBuf; + +use tempfile::Builder; +use tempfile::TempDir; +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, +} + +pub struct FeedbackDiagnosticsAttachment { + _dir: TempDir, + path: PathBuf, +} + +impl FeedbackDiagnosticsAttachment { + pub fn path(&self) -> &Path { + &self.path + } +} + +impl FeedbackDiagnostics { + pub fn collect_from_env() -> Self { + Self::collect_from_pairs(std::env::vars()) + } + + pub 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 write_temp_attachment(&self) -> io::Result> { + let Some(text) = self.attachment_text() else { + return Ok(None); + }; + + let dir = Builder::new() + .prefix("codex-connectivity-diagnostics-") + .tempdir()?; + let path = dir.path().join(FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME); + fs::write(&path, text)?; + + Ok(Some(FeedbackDiagnosticsAttachment { _dir: dir, path })) + } +} + +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 std::ffi::OsStr; + use std::fs; + + use super::FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME; + use super::FeedbackDiagnostic; + use super::FeedbackDiagnostics; + use super::sanitize_url_for_display; + + #[test] + fn collect_from_pairs_returns_empty_when_no_diagnostics_are_present() { + let diagnostics = FeedbackDiagnostics::collect_from_pairs(Vec::<(String, String)>::new()); + + assert_eq!(diagnostics, FeedbackDiagnostics::default()); + assert_eq!(diagnostics.attachment_text(), None); + } + + #[test] + fn collect_from_pairs_reports_proxy_env_vars_in_fixed_order() { + let diagnostics = FeedbackDiagnostics::collect_from_pairs([ + ("HTTPS_PROXY", "https://secure-proxy.example.com:443"), + ("HTTP_PROXY", "proxy.example.com:8080"), + ("ALL_PROXY", "socks5h://all-proxy.example.com:1080"), + ]); + + 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(), + ], + }], + } + ); + } + + #[test] + fn collect_from_pairs_reports_invalid_proxy_values_without_echoing_them() { + let diagnostics = + FeedbackDiagnostics::collect_from_pairs([("HTTP_PROXY", "not a valid\nproxy")]); + + 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()], + }], + } + ); + } + + #[test] + fn collect_from_pairs_reports_non_default_openai_base_url() { + let diagnostics = FeedbackDiagnostics::collect_from_pairs([( + "OPENAI_BASE_URL", + "https://example.com/v1", + )]); + + assert_eq!( + diagnostics, + FeedbackDiagnostics { + diagnostics: 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()], + }], + } + ); + } + + #[test] + fn collect_from_pairs_ignores_default_openai_base_url() { + let diagnostics = FeedbackDiagnostics::collect_from_pairs([( + "OPENAI_BASE_URL", + "https://api.openai.com/v1/", + )]); + + assert_eq!(diagnostics, FeedbackDiagnostics::default()); + } + + #[test] + fn collect_from_pairs_reports_invalid_openai_base_url_without_echoing_it() { + let diagnostics = + FeedbackDiagnostics::collect_from_pairs([("OPENAI_BASE_URL", "not a valid\nurl")]); + + assert_eq!( + diagnostics, + FeedbackDiagnostics { + diagnostics: vec![FeedbackDiagnostic { + headline: "OPENAI_BASE_URL is set and may affect connectivity.".to_string(), + details: vec!["OPENAI_BASE_URL = invalid value".to_string()], + }], + } + ); + } + + #[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())); + } + + #[test] + fn write_temp_attachment_persists_sanitized_text() { + let diagnostics = FeedbackDiagnostics::collect_from_pairs([ + ( + "HTTP_PROXY", + "https://user:password@proxy.example.com:8443?secret=1", + ), + ("OPENAI_BASE_URL", "https://example.com/v1?token=secret"), + ]); + + let attachment = diagnostics + .write_temp_attachment() + .expect("attachment should be written") + .expect("attachment should be present"); + let contents = + fs::read_to_string(attachment.path()).expect("attachment should be readable"); + + assert_eq!( + attachment.path().file_name(), + Some(OsStr::new(FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME)) + ); + assert_eq!( + contents, + "Connectivity diagnostics\n\n- Proxy environment variables are set and may affect connectivity.\n - HTTP_PROXY = https://proxy.example.com:8443\n- OPENAI_BASE_URL is set and may affect connectivity.\n - OPENAI_BASE_URL = https://example.com/v1" + .to_string() + ); + } +} diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index a7bcb58f35d..cb6684e6cf5 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -37,6 +37,7 @@ pub mod exec_env; mod exec_policy; pub mod external_agent_config; pub mod features; +pub mod feedback_diagnostics; mod file_watcher; mod flags; pub mod git_info; diff --git a/codex-rs/tui/src/bottom_pane/feedback_view.rs b/codex-rs/tui/src/bottom_pane/feedback_view.rs index 4144c87f97a..c5defbd625a 100644 --- a/codex-rs/tui/src/bottom_pane/feedback_view.rs +++ b/codex-rs/tui/src/bottom_pane/feedback_view.rs @@ -1,6 +1,9 @@ use std::cell::RefCell; use std::path::PathBuf; +use codex_core::feedback_diagnostics::FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME; +use codex_core::feedback_diagnostics::FeedbackDiagnostics; +use codex_core::feedback_diagnostics::FeedbackDiagnosticsAttachment; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyModifiers; @@ -19,6 +22,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; @@ -48,6 +53,7 @@ pub(crate) struct FeedbackNoteView { category: FeedbackCategory, snapshot: codex_feedback::CodexLogSnapshot, rollout_path: Option, + diagnostics: FeedbackDiagnostics, app_event_tx: AppEventSender, include_logs: bool, feedback_audience: FeedbackAudience, @@ -58,11 +64,17 @@ pub(crate) struct FeedbackNoteView { complete: bool, } +struct PreparedFeedbackAttachments { + attachment_paths: Vec, + _diagnostics_attachment: Option, +} + impl FeedbackNoteView { pub(crate) fn new( category: FeedbackCategory, snapshot: codex_feedback::CodexLogSnapshot, rollout_path: Option, + diagnostics: FeedbackDiagnostics, app_event_tx: AppEventSender, include_logs: bool, feedback_audience: FeedbackAudience, @@ -71,6 +83,7 @@ impl FeedbackNoteView { category, snapshot, rollout_path, + diagnostics, app_event_tx, include_logs, feedback_audience, @@ -87,11 +100,7 @@ impl FeedbackNoteView { } else { Some(note.as_str()) }; - let log_file_paths = if self.include_logs { - self.rollout_path.iter().cloned().collect::>() - } else { - Vec::new() - }; + let attachments = self.prepare_feedback_attachments(); let classification = feedback_classification(self.category); let mut thread_id = self.snapshot.thread_id.clone(); @@ -100,7 +109,7 @@ impl FeedbackNoteView { classification, reason_opt, self.include_logs, - &log_file_paths, + &attachments.attachment_paths, Some(SessionSource::Cli), ); @@ -216,21 +225,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, }; @@ -243,23 +252,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, }; @@ -333,6 +345,77 @@ 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()]), + Line::from(vec![gutter()]), + 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)); + if self.diagnostics.is_empty() { + let options = RtOptions::new(width) + .initial_indent(Line::from(vec![gutter(), " ".dim()])) + .subsequent_indent(Line::from(vec![gutter(), " ".dim()])); + return word_wrap_lines(["No connectivity diagnostics detected.".dim()], options); + } + + 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.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 + } + + fn prepare_feedback_attachments(&self) -> PreparedFeedbackAttachments { + let mut attachment_paths = if self.include_logs { + self.rollout_path.iter().cloned().collect::>() + } else { + Vec::new() + }; + let diagnostics_attachment = if self.include_logs { + match self.diagnostics.write_temp_attachment() { + Ok(attachment) => attachment, + Err(err) => { + tracing::warn!(error = %err, "failed to write diagnostics attachment"); + None + } + } + } else { + None + }; + if let Some(attachment) = diagnostics_attachment.as_ref() { + attachment_paths.push(attachment.path().to_path_buf()); + } + + PreparedFeedbackAttachments { + attachment_paths, + _diagnostics_attachment: diagnostics_attachment, + } + } } fn gutter() -> Span<'static> { @@ -512,7 +595,7 @@ pub(crate) fn feedback_upload_consent_params( let mut header_lines: Vec> = vec![ Line::from("Upload logs?".bold()).into(), Line::from("").into(), - Line::from("The following files will be sent:".dim()).into(), + Line::from("If you choose Yes, the following files may be sent:".dim()).into(), Line::from(vec![" • ".into(), "codex-logs.log".into()]).into(), ]; if let Some(path) = rollout_path.as_deref() @@ -520,6 +603,14 @@ pub(crate) fn feedback_upload_consent_params( { header_lines.push(Line::from(vec![" • ".into(), name.into()]).into()); } + header_lines.push( + Line::from(vec![ + " • ".into(), + FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME.into(), + " (if connectivity diagnostics are detected)".dim(), + ]) + .into(), + ); super::SelectionViewParams { footer_hint: Some(standard_popup_hint_line()), @@ -527,7 +618,7 @@ pub(crate) fn feedback_upload_consent_params( super::SelectionItem { name: "Yes".to_string(), description: Some( - "Share the current Codex session logs with the team for troubleshooting." + "Share the current Codex session logs and any generated diagnostics attachment." .to_string(), ), actions: vec![yes_action], @@ -536,7 +627,7 @@ pub(crate) fn feedback_upload_consent_params( }, super::SelectionItem { name: "No".to_string(), - description: Some("".to_string()), + description: Some("Send feedback without any attached files.".to_string()), actions: vec![no_action], dismiss_on_select: true, ..Default::default() @@ -554,6 +645,9 @@ mod tests { use super::*; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; + use pretty_assertions::assert_eq; + use std::ffi::OsStr; + use std::fs; fn render(view: &FeedbackNoteView, width: u16) -> String { let height = view.desired_height(width); @@ -593,6 +687,7 @@ mod tests { category, snapshot, None, + FeedbackDiagnostics::default(), tx, true, FeedbackAudience::External, @@ -634,6 +729,90 @@ 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 snapshot = codex_feedback::CodexFeedback::new().snapshot(None); + let diagnostics = FeedbackDiagnostics::collect_from_pairs([ + ("HTTP_PROXY", "proxy.example.com:8080"), + ("OPENAI_BASE_URL", "https://example.com/v1"), + ]); + let view = FeedbackNoteView::new( + FeedbackCategory::Bug, + snapshot, + None, + diagnostics, + tx, + false, + FeedbackAudience::External, + ); + let rendered = render(&view, 60); + + insta::assert_snapshot!("feedback_view_with_connectivity_diagnostics", rendered); + } + + #[test] + fn prepare_feedback_attachments_skips_diagnostics_without_logs() { + let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let snapshot = codex_feedback::CodexFeedback::new().snapshot(None); + let diagnostics = FeedbackDiagnostics::collect_from_pairs([( + "OPENAI_BASE_URL", + "https://example.com/v1", + )]); + let view = FeedbackNoteView::new( + FeedbackCategory::Other, + snapshot, + None, + diagnostics, + tx, + false, + FeedbackAudience::External, + ); + + let attachments = view.prepare_feedback_attachments(); + assert_eq!(attachments.attachment_paths, Vec::::new()); + assert!(attachments._diagnostics_attachment.is_none()); + } + + #[test] + fn prepare_feedback_attachments_includes_diagnostics_with_logs() { + let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let snapshot = codex_feedback::CodexFeedback::new().snapshot(None); + let diagnostics = FeedbackDiagnostics::collect_from_pairs([( + "OPENAI_BASE_URL", + "https://example.com/v1", + )]); + let view = FeedbackNoteView::new( + FeedbackCategory::Other, + snapshot, + None, + diagnostics, + tx, + true, + FeedbackAudience::External, + ); + + let attachments = view.prepare_feedback_attachments(); + assert_eq!(attachments.attachment_paths.len(), 1); + assert_eq!( + attachments.attachment_paths[0].file_name(), + Some(OsStr::new(FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME)) + ); + assert!( + attachments._diagnostics_attachment.is_some(), + "diagnostics attachment should stay alive until upload completes" + ); + let contents = fs::read_to_string(&attachments.attachment_paths[0]) + .expect("diagnostics attachment should be readable"); + assert_eq!( + contents, + "Connectivity diagnostics\n\n- OPENAI_BASE_URL is set and may affect connectivity.\n - OPENAI_BASE_URL = https://example.com/v1" + ); + } + #[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_bad_result.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap index 465f0f9c4f3..97ac7280e4a 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap @@ -1,9 +1,13 @@ --- source: tui/src/bottom_pane/feedback_view.rs +assertion_line: 698 expression: rendered --- ▌ Tell us more (bad result) ▌ +▌ Connectivity diagnostics +▌ No connectivity diagnostics detected. +▌ ▌ (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/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap index a0b5660135b..06f1966f5ec 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap @@ -1,9 +1,13 @@ --- source: tui/src/bottom_pane/feedback_view.rs +assertion_line: 712 expression: rendered --- ▌ Tell us more (bug) ▌ +▌ Connectivity diagnostics +▌ No connectivity diagnostics detected. +▌ ▌ (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/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap index 73074d61faa..cce926b6e3e 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap @@ -1,9 +1,13 @@ --- source: tui/src/bottom_pane/feedback_view.rs +assertion_line: 705 expression: rendered --- ▌ Tell us more (good result) ▌ +▌ Connectivity diagnostics +▌ No connectivity diagnostics detected. +▌ ▌ (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/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap index 80e4ffeffe1..640177c94d2 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap @@ -1,9 +1,13 @@ --- source: tui/src/bottom_pane/feedback_view.rs +assertion_line: 719 expression: rendered --- ▌ Tell us more (other) ▌ +▌ Connectivity diagnostics +▌ No connectivity diagnostics detected. +▌ ▌ (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/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap index 52148d0e863..2f0c19a7ac7 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap @@ -1,9 +1,13 @@ --- source: tui/src/bottom_pane/feedback_view.rs +assertion_line: 726 expression: rendered --- ▌ Tell us more (safety check) ▌ +▌ Connectivity diagnostics +▌ No connectivity diagnostics detected. +▌ ▌ (optional) Share what was refused and why it should have b Press enter to confirm or esc to go back 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 78e8fcc6690..cc484fa5936 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1251,6 +1251,7 @@ impl ChatWidget { ) { // Build a fresh snapshot at the time of opening the note overlay. let snapshot = self.feedback.snapshot(self.thread_id); + let diagnostics = codex_core::feedback_diagnostics::FeedbackDiagnostics::collect_from_env(); let rollout = if include_logs { self.current_rollout_path.clone() } else { @@ -1260,6 +1261,7 @@ impl ChatWidget { category, snapshot, rollout, + diagnostics, self.app_event_tx.clone(), include_logs, self.feedback_audience, 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..2b70ae1122f 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 @@ -1,14 +1,16 @@ --- source: tui/src/chatwidget/tests.rs +assertion_line: 6401 expression: popup --- Upload logs? - The following files will be sent: + If you choose Yes, the following files may be sent: • codex-logs.log + • codex-connectivity-diagnostics.txt (if connectivity diagnostics are dete -› 1. Yes Share the current Codex session logs with the team for - troubleshooting. - 2. No +› 1. Yes Share the current Codex session logs and any generated diagnostics + attachment. + 2. No Send feedback without any attached files. Press enter to confirm or esc to go back From c690e66706a063b6a336e731bec63ea25c3a8d4c Mon Sep 17 00:00:00 2001 From: Roy Han Date: Mon, 2 Mar 2026 18:21:10 -0800 Subject: [PATCH 02/14] ui edits --- codex-rs/tui/src/bottom_pane/feedback_view.rs | 4 ++-- ..._chatwidget__tests__feedback_upload_consent_popup.snap | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/feedback_view.rs b/codex-rs/tui/src/bottom_pane/feedback_view.rs index c5defbd625a..820344eeb16 100644 --- a/codex-rs/tui/src/bottom_pane/feedback_view.rs +++ b/codex-rs/tui/src/bottom_pane/feedback_view.rs @@ -595,7 +595,7 @@ pub(crate) fn feedback_upload_consent_params( let mut header_lines: Vec> = vec![ Line::from("Upload logs?".bold()).into(), Line::from("").into(), - Line::from("If you choose Yes, the following files may be sent:".dim()).into(), + Line::from("The following files will be sent:".dim()).into(), Line::from(vec![" • ".into(), "codex-logs.log".into()]).into(), ]; if let Some(path) = rollout_path.as_deref() @@ -618,7 +618,7 @@ pub(crate) fn feedback_upload_consent_params( super::SelectionItem { name: "Yes".to_string(), description: Some( - "Share the current Codex session logs and any generated diagnostics attachment." + "Share the current Codex session logs with the team for troubleshooting." .to_string(), ), actions: vec![yes_action], 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 2b70ae1122f..a959b74a15e 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 @@ -5,12 +5,12 @@ expression: popup --- Upload logs? - If you choose Yes, the following files may be sent: + The following files will be sent: • codex-logs.log • codex-connectivity-diagnostics.txt (if connectivity diagnostics are dete -› 1. Yes Share the current Codex session logs and any generated diagnostics - attachment. - 2. No Send feedback without any attached files. +› 1. Yes Share the current Codex session logs with the team for + troubleshooting. + 2. No Press enter to confirm or esc to go back From 4c92bd483e18ac372c0a7355e27b93729fffddd8 Mon Sep 17 00:00:00 2001 From: Roy Han Date: Mon, 2 Mar 2026 18:38:01 -0800 Subject: [PATCH 03/14] test condense --- codex-rs/core/src/feedback_diagnostics.rs | 168 ++++++++---------- codex-rs/tui/src/bottom_pane/feedback_view.rs | 69 +++---- 2 files changed, 98 insertions(+), 139 deletions(-) diff --git a/codex-rs/core/src/feedback_diagnostics.rs b/codex-rs/core/src/feedback_diagnostics.rs index b6b01ebad51..f298a4fde3c 100644 --- a/codex-rs/core/src/feedback_diagnostics.rs +++ b/codex-rs/core/src/feedback_diagnostics.rs @@ -177,96 +177,108 @@ mod tests { use super::sanitize_url_for_display; #[test] - fn collect_from_pairs_returns_empty_when_no_diagnostics_are_present() { - let diagnostics = FeedbackDiagnostics::collect_from_pairs(Vec::<(String, String)>::new()); - - assert_eq!(diagnostics, FeedbackDiagnostics::default()); - assert_eq!(diagnostics.attachment_text(), None); - } - - #[test] - fn collect_from_pairs_reports_proxy_env_vars_in_fixed_order() { + fn collect_from_pairs_reports_sanitized_diagnostics_and_attachment() { let diagnostics = FeedbackDiagnostics::collect_from_pairs([ - ("HTTPS_PROXY", "https://secure-proxy.example.com:443"), - ("HTTP_PROXY", "proxy.example.com:8080"), - ("ALL_PROXY", "socks5h://all-proxy.example.com:1080"), + ( + "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(), - ], - }], + 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()], + }, + ], } ); - } - #[test] - fn collect_from_pairs_reports_invalid_proxy_values_without_echoing_them() { - let diagnostics = - FeedbackDiagnostics::collect_from_pairs([("HTTP_PROXY", "not a valid\nproxy")]); + let attachment = diagnostics + .write_temp_attachment() + .expect("attachment should be written") + .expect("attachment should be present"); + let contents = + fs::read_to_string(attachment.path()).expect("attachment should be readable"); 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()], - }], - } + attachment.path().file_name(), + Some(OsStr::new(FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME)) ); - } - - #[test] - fn collect_from_pairs_reports_non_default_openai_base_url() { - let diagnostics = FeedbackDiagnostics::collect_from_pairs([( - "OPENAI_BASE_URL", - "https://example.com/v1", - )]); - assert_eq!( - diagnostics, - FeedbackDiagnostics { - diagnostics: 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()], - }], - } + contents, + "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_default_openai_base_url() { - let diagnostics = FeedbackDiagnostics::collect_from_pairs([( - "OPENAI_BASE_URL", - "https://api.openai.com/v1/", - )]); - - assert_eq!(diagnostics, FeedbackDiagnostics::default()); + 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); + assert!( + diagnostics + .write_temp_attachment() + .expect("empty diagnostics should not write attachment") + .is_none() + ); + } } #[test] - fn collect_from_pairs_reports_invalid_openai_base_url_without_echoing_it() { - let diagnostics = - FeedbackDiagnostics::collect_from_pairs([("OPENAI_BASE_URL", "not a valid\nurl")]); + 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: "OPENAI_BASE_URL is set and may affect connectivity.".to_string(), - details: vec!["OPENAI_BASE_URL = invalid value".to_string()], - }], + 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] @@ -277,32 +289,4 @@ mod tests { assert_eq!(sanitized, Some("https://example.com:8443/v1".to_string())); } - - #[test] - fn write_temp_attachment_persists_sanitized_text() { - let diagnostics = FeedbackDiagnostics::collect_from_pairs([ - ( - "HTTP_PROXY", - "https://user:password@proxy.example.com:8443?secret=1", - ), - ("OPENAI_BASE_URL", "https://example.com/v1?token=secret"), - ]); - - let attachment = diagnostics - .write_temp_attachment() - .expect("attachment should be written") - .expect("attachment should be present"); - let contents = - fs::read_to_string(attachment.path()).expect("attachment should be readable"); - - assert_eq!( - attachment.path().file_name(), - Some(OsStr::new(FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME)) - ); - assert_eq!( - contents, - "Connectivity diagnostics\n\n- Proxy environment variables are set and may affect connectivity.\n - HTTP_PROXY = https://proxy.example.com:8443\n- OPENAI_BASE_URL is set and may affect connectivity.\n - OPENAI_BASE_URL = https://example.com/v1" - .to_string() - ); - } } diff --git a/codex-rs/tui/src/bottom_pane/feedback_view.rs b/codex-rs/tui/src/bottom_pane/feedback_view.rs index 820344eeb16..569a703ff79 100644 --- a/codex-rs/tui/src/bottom_pane/feedback_view.rs +++ b/codex-rs/tui/src/bottom_pane/feedback_view.rs @@ -647,7 +647,6 @@ mod tests { use crate::app_event_sender::AppEventSender; use pretty_assertions::assert_eq; use std::ffi::OsStr; - use std::fs; fn render(view: &FeedbackNoteView, width: u16) -> String { let height = view.desired_height(width); @@ -753,64 +752,40 @@ mod tests { } #[test] - fn prepare_feedback_attachments_skips_diagnostics_without_logs() { - let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::(); - let tx = AppEventSender::new(tx_raw); - let snapshot = codex_feedback::CodexFeedback::new().snapshot(None); + fn prepare_feedback_attachments_only_includes_diagnostics_when_logs_are_enabled() { let diagnostics = FeedbackDiagnostics::collect_from_pairs([( "OPENAI_BASE_URL", "https://example.com/v1", )]); - let view = FeedbackNoteView::new( - FeedbackCategory::Other, - snapshot, - None, - diagnostics, - tx, - false, - FeedbackAudience::External, - ); - - let attachments = view.prepare_feedback_attachments(); - assert_eq!(attachments.attachment_paths, Vec::::new()); - assert!(attachments._diagnostics_attachment.is_none()); - } + let make_view = |include_logs| { + let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let snapshot = codex_feedback::CodexFeedback::new().snapshot(None); + FeedbackNoteView::new( + FeedbackCategory::Other, + snapshot, + None, + diagnostics.clone(), + tx, + include_logs, + FeedbackAudience::External, + ) + }; - #[test] - fn prepare_feedback_attachments_includes_diagnostics_with_logs() { - let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::(); - let tx = AppEventSender::new(tx_raw); - let snapshot = codex_feedback::CodexFeedback::new().snapshot(None); - let diagnostics = FeedbackDiagnostics::collect_from_pairs([( - "OPENAI_BASE_URL", - "https://example.com/v1", - )]); - let view = FeedbackNoteView::new( - FeedbackCategory::Other, - snapshot, - None, - diagnostics, - tx, - true, - FeedbackAudience::External, - ); + let without_logs = make_view(false).prepare_feedback_attachments(); + assert_eq!(without_logs.attachment_paths, Vec::::new()); + assert!(without_logs._diagnostics_attachment.is_none()); - let attachments = view.prepare_feedback_attachments(); - assert_eq!(attachments.attachment_paths.len(), 1); + let with_logs = make_view(true).prepare_feedback_attachments(); + assert_eq!(with_logs.attachment_paths.len(), 1); assert_eq!( - attachments.attachment_paths[0].file_name(), + with_logs.attachment_paths[0].file_name(), Some(OsStr::new(FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME)) ); assert!( - attachments._diagnostics_attachment.is_some(), + with_logs._diagnostics_attachment.is_some(), "diagnostics attachment should stay alive until upload completes" ); - let contents = fs::read_to_string(&attachments.attachment_paths[0]) - .expect("diagnostics attachment should be readable"); - assert_eq!( - contents, - "Connectivity diagnostics\n\n- OPENAI_BASE_URL is set and may affect connectivity.\n - OPENAI_BASE_URL = https://example.com/v1" - ); } #[test] From 8e2a09c5d9b8e4bb318687077708ef853f3c8c07 Mon Sep 17 00:00:00 2001 From: Roy Han Date: Mon, 2 Mar 2026 18:55:03 -0800 Subject: [PATCH 04/14] test fix --- ...x_tui__chatwidget__tests__feedback_upload_consent_popup.snap | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 a959b74a15e..56466513bae 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 @@ -11,6 +11,6 @@ expression: popup › 1. Yes Share the current Codex session logs with the team for troubleshooting. - 2. No + 2. No Send feedback without any attached files. Press enter to confirm or esc to go back From 4c21949517e145042177f8b08e326395e4493278 Mon Sep 17 00:00:00 2001 From: Roy Han Date: Tue, 3 Mar 2026 10:49:10 -0800 Subject: [PATCH 05/14] selective diagnostic display --- codex-rs/tui/src/bottom_pane/feedback_view.rs | 45 +++++++++++++++---- codex-rs/tui/src/bottom_pane/mod.rs | 1 + ...view__tests__feedback_view_bad_result.snap | 4 +- ...edback_view__tests__feedback_view_bug.snap | 4 +- ...iew__tests__feedback_view_good_result.snap | 4 +- ...back_view__tests__feedback_view_other.snap | 4 +- ...ew__tests__feedback_view_safety_check.snap | 4 +- codex-rs/tui/src/chatwidget.rs | 16 ++++++- ..._good_result_note_without_diagnostics.snap | 11 +++++ codex-rs/tui/src/chatwidget/tests.rs | 17 ++++++- 10 files changed, 83 insertions(+), 27 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap diff --git a/codex-rs/tui/src/bottom_pane/feedback_view.rs b/codex-rs/tui/src/bottom_pane/feedback_view.rs index 569a703ff79..b5ac6827d2a 100644 --- a/codex-rs/tui/src/bottom_pane/feedback_view.rs +++ b/codex-rs/tui/src/bottom_pane/feedback_view.rs @@ -351,21 +351,19 @@ impl FeedbackNoteView { let mut lines = vec![ Line::from(vec![gutter(), title.bold()]), Line::from(vec![gutter()]), - Line::from(vec![gutter(), "Connectivity diagnostics".bold()]), ]; - lines.extend(self.diagnostics_lines(width)); + if should_show_feedback_connectivity_details(self.category, &self.diagnostics) { + 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)); - if self.diagnostics.is_empty() { - let options = RtOptions::new(width) - .initial_indent(Line::from(vec![gutter(), " ".dim()])) - .subsequent_indent(Line::from(vec![gutter(), " ".dim()])); - return word_wrap_lines(["No connectivity diagnostics detected.".dim()], options); - } - let headline_options = RtOptions::new(width) .initial_indent(Line::from(vec![gutter(), " - ".into()])) .subsequent_indent(Line::from(vec![gutter(), " ".into()])); @@ -418,6 +416,13 @@ impl FeedbackNoteView { } } +pub(crate) fn should_show_feedback_connectivity_details( + category: FeedbackCategory, + diagnostics: &FeedbackDiagnostics, +) -> bool { + category != FeedbackCategory::GoodResult && !diagnostics.is_empty() +} + fn gutter() -> Span<'static> { "▌ ".cyan() } @@ -751,6 +756,28 @@ mod tests { 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::collect_from_pairs([("HTTP_PROXY", "proxy.example.com:8080")]); + + 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 prepare_feedback_attachments_only_includes_diagnostics_when_logs_are_enabled() { let diagnostics = FeedbackDiagnostics::collect_from_pairs([( diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 02c7faeebb3..63185a7250e 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -101,6 +101,7 @@ mod selection_popup_common; mod textarea; mod unified_exec_footer; pub(crate) use feedback_view::FeedbackNoteView; +pub(crate) use feedback_view::should_show_feedback_connectivity_details; /// How long the "press again to quit" hint stays visible. /// diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap index 97ac7280e4a..e0704738973 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap @@ -1,12 +1,10 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 698 +assertion_line: 699 expression: rendered --- ▌ Tell us more (bad result) ▌ -▌ Connectivity diagnostics -▌ No connectivity diagnostics detected. ▌ ▌ (optional) Write a short description to help us further diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap index 06f1966f5ec..6a1dd7395f0 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap @@ -1,12 +1,10 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 712 +assertion_line: 713 expression: rendered --- ▌ Tell us more (bug) ▌ -▌ Connectivity diagnostics -▌ No connectivity diagnostics detected. ▌ ▌ (optional) Write a short description to help us further diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap index cce926b6e3e..8efcbcb2b3b 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap @@ -1,12 +1,10 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 705 +assertion_line: 706 expression: rendered --- ▌ Tell us more (good result) ▌ -▌ Connectivity diagnostics -▌ No connectivity diagnostics detected. ▌ ▌ (optional) Write a short description to help us further diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap index 640177c94d2..165f0c256b2 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap @@ -1,12 +1,10 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 719 +assertion_line: 720 expression: rendered --- ▌ Tell us more (other) ▌ -▌ Connectivity diagnostics -▌ No connectivity diagnostics detected. ▌ ▌ (optional) Write a short description to help us further diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap index 2f0c19a7ac7..301049c3ef8 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap @@ -1,12 +1,10 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 726 +assertion_line: 727 expression: rendered --- ▌ Tell us more (safety check) ▌ -▌ Connectivity diagnostics -▌ No connectivity diagnostics detected. ▌ ▌ (optional) Share what was refused and why it should have b diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index bec6da5825c..16c32960ac5 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1254,10 +1254,19 @@ impl ChatWidget { &mut self, category: crate::app_event::FeedbackCategory, include_logs: bool, + ) { + let diagnostics = codex_core::feedback_diagnostics::FeedbackDiagnostics::collect_from_env(); + self.show_feedback_note(category, include_logs, diagnostics); + } + + fn show_feedback_note( + &mut self, + category: crate::app_event::FeedbackCategory, + include_logs: bool, + diagnostics: codex_core::feedback_diagnostics::FeedbackDiagnostics, ) { // Build a fresh snapshot at the time of opening the note overlay. let snapshot = self.feedback.snapshot(self.thread_id); - let diagnostics = codex_core::feedback_diagnostics::FeedbackDiagnostics::collect_from_env(); let rollout = if include_logs { self.current_rollout_path.clone() } else { @@ -1283,6 +1292,11 @@ impl ChatWidget { } pub(crate) fn open_feedback_consent(&mut self, category: crate::app_event::FeedbackCategory) { + let diagnostics = codex_core::feedback_diagnostics::FeedbackDiagnostics::collect_from_env(); + if !crate::bottom_pane::should_show_feedback_connectivity_details(category, &diagnostics) { + self.show_feedback_note(category, false, diagnostics); + return; + } let params = crate::bottom_pane::feedback_upload_consent_params( self.app_event_tx.clone(), category, diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap new file mode 100644 index 00000000000..c2776882f70 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap @@ -0,0 +1,11 @@ +--- +source: tui/src/chatwidget/tests.rs +assertion_line: 6441 +expression: popup +--- +▌ Tell us more (good result) +▌ +▌ +▌ (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/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index a720559b5c3..813909c14e8 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -6436,13 +6436,26 @@ 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(), + )); let popup = render_bottom_popup(&chat, 80); assert_snapshot!("feedback_upload_consent_popup", popup); } +#[tokio::test] +async fn feedback_good_result_skips_attachment_prompt() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + + chat.open_feedback_consent(crate::app_event::FeedbackCategory::GoodResult); + + let popup = render_bottom_popup(&chat, 80); + assert_snapshot!("feedback_good_result_note_without_diagnostics", 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; From 90ee93ba45ca2cc4360a840be3ee120d76e5702c Mon Sep 17 00:00:00 2001 From: Roy Han Date: Tue, 3 Mar 2026 11:03:46 -0800 Subject: [PATCH 06/14] snapshot cleanup --- codex-rs/tui/src/bottom_pane/feedback_view.rs | 6 ++---- ...ane__feedback_view__tests__feedback_view_bad_result.snap | 3 +-- ...ottom_pane__feedback_view__tests__feedback_view_bug.snap | 3 +-- ...ne__feedback_view__tests__feedback_view_good_result.snap | 3 +-- ...tom_pane__feedback_view__tests__feedback_view_other.snap | 3 +-- ...e__feedback_view__tests__feedback_view_safety_check.snap | 3 +-- ...ests__feedback_good_result_note_without_diagnostics.snap | 3 +-- 7 files changed, 8 insertions(+), 16 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/feedback_view.rs b/codex-rs/tui/src/bottom_pane/feedback_view.rs index b5ac6827d2a..d7bc483fbb0 100644 --- a/codex-rs/tui/src/bottom_pane/feedback_view.rs +++ b/codex-rs/tui/src/bottom_pane/feedback_view.rs @@ -348,11 +348,9 @@ impl FeedbackNoteView { 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()]), - Line::from(vec![gutter()]), - ]; + let mut lines = vec![Line::from(vec![gutter(), title.bold()])]; if should_show_feedback_connectivity_details(self.category, &self.diagnostics) { + lines.push(Line::from(vec![gutter()])); lines.push(Line::from(vec![ gutter(), "Connectivity diagnostics".bold(), diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap index e0704738973..ffa916de325 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap @@ -1,11 +1,10 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 699 +assertion_line: 703 expression: rendered --- ▌ Tell us more (bad result) ▌ -▌ ▌ (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/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap index 6a1dd7395f0..1b318ecb7f6 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap @@ -1,11 +1,10 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 713 +assertion_line: 717 expression: rendered --- ▌ Tell us more (bug) ▌ -▌ ▌ (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/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap index 8efcbcb2b3b..fa6e430ebdf 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap @@ -1,11 +1,10 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 706 +assertion_line: 710 expression: rendered --- ▌ Tell us more (good result) ▌ -▌ ▌ (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/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap index 165f0c256b2..7feafcfd3d6 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap @@ -1,11 +1,10 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 720 +assertion_line: 724 expression: rendered --- ▌ Tell us more (other) ▌ -▌ ▌ (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/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap index 301049c3ef8..a22ef5d5dc9 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap @@ -1,11 +1,10 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 727 +assertion_line: 731 expression: rendered --- ▌ Tell us more (safety check) ▌ -▌ ▌ (optional) Share what was refused and why it should have b Press enter to confirm or esc to go back diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap index c2776882f70..9eb4b6385b8 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap @@ -1,11 +1,10 @@ --- source: tui/src/chatwidget/tests.rs -assertion_line: 6441 +assertion_line: 6456 expression: popup --- ▌ Tell us more (good result) ▌ -▌ ▌ (optional) Write a short description to help us further Press enter to confirm or esc to go back From be545df21112c4e2f143a6dd896aa8d3939c495e Mon Sep 17 00:00:00 2001 From: Roy Han Date: Tue, 3 Mar 2026 11:12:57 -0800 Subject: [PATCH 07/14] snapshot cleanup --- ...tom_pane__feedback_view__tests__feedback_view_bad_result.snap | 1 - ...ui__bottom_pane__feedback_view__tests__feedback_view_bug.snap | 1 - ...om_pane__feedback_view__tests__feedback_view_good_result.snap | 1 - ...__bottom_pane__feedback_view__tests__feedback_view_other.snap | 1 - ...m_pane__feedback_view__tests__feedback_view_safety_check.snap | 1 - ...et__tests__feedback_good_result_note_without_diagnostics.snap | 1 - 6 files changed, 6 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap index ffa916de325..465f0f9c4f3 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bad_result.snap @@ -1,6 +1,5 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 703 expression: rendered --- ▌ Tell us more (bad result) diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap index 1b318ecb7f6..a0b5660135b 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_bug.snap @@ -1,6 +1,5 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 717 expression: rendered --- ▌ Tell us more (bug) diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap index fa6e430ebdf..73074d61faa 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_good_result.snap @@ -1,6 +1,5 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 710 expression: rendered --- ▌ Tell us more (good result) diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap index 7feafcfd3d6..80e4ffeffe1 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_other.snap @@ -1,6 +1,5 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 724 expression: rendered --- ▌ Tell us more (other) diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap index a22ef5d5dc9..52148d0e863 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__feedback_view__tests__feedback_view_safety_check.snap @@ -1,6 +1,5 @@ --- source: tui/src/bottom_pane/feedback_view.rs -assertion_line: 731 expression: rendered --- ▌ Tell us more (safety check) diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap index 9eb4b6385b8..4ad1965411a 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap @@ -1,6 +1,5 @@ --- source: tui/src/chatwidget/tests.rs -assertion_line: 6456 expression: popup --- ▌ Tell us more (good result) From 52867f984e6cd340db2e54ab16224dfd2ec30675 Mon Sep 17 00:00:00 2001 From: Roy Han Date: Tue, 3 Mar 2026 11:19:02 -0800 Subject: [PATCH 08/14] restore other log sending --- codex-rs/tui/src/bottom_pane/feedback_view.rs | 38 ++++++++++++------- codex-rs/tui/src/chatwidget.rs | 5 +-- ...s__feedback_good_result_consent_popup.snap | 14 +++++++ ..._good_result_note_without_diagnostics.snap | 9 ----- ..._tests__feedback_upload_consent_popup.snap | 3 +- codex-rs/tui/src/chatwidget/tests.rs | 12 ++++-- 6 files changed, 50 insertions(+), 31 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_consent_popup.snap delete mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap diff --git a/codex-rs/tui/src/bottom_pane/feedback_view.rs b/codex-rs/tui/src/bottom_pane/feedback_view.rs index d7bc483fbb0..797ce1a70c1 100644 --- a/codex-rs/tui/src/bottom_pane/feedback_view.rs +++ b/codex-rs/tui/src/bottom_pane/feedback_view.rs @@ -392,7 +392,9 @@ impl FeedbackNoteView { } else { Vec::new() }; - let diagnostics_attachment = if self.include_logs { + let diagnostics_attachment = if self.include_logs + && should_show_feedback_connectivity_details(self.category, &self.diagnostics) + { match self.diagnostics.write_temp_attachment() { Ok(attachment) => attachment, Err(err) => { @@ -570,6 +572,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({ @@ -606,14 +609,15 @@ pub(crate) fn feedback_upload_consent_params( { header_lines.push(Line::from(vec![" • ".into(), name.into()]).into()); } - header_lines.push( - Line::from(vec![ - " • ".into(), - FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME.into(), - " (if connectivity diagnostics are detected)".dim(), - ]) - .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()), @@ -782,12 +786,12 @@ mod tests { "OPENAI_BASE_URL", "https://example.com/v1", )]); - let make_view = |include_logs| { + let make_view = |category, include_logs| { let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::(); let tx = AppEventSender::new(tx_raw); let snapshot = codex_feedback::CodexFeedback::new().snapshot(None); FeedbackNoteView::new( - FeedbackCategory::Other, + category, snapshot, None, diagnostics.clone(), @@ -797,11 +801,11 @@ mod tests { ) }; - let without_logs = make_view(false).prepare_feedback_attachments(); + let without_logs = make_view(FeedbackCategory::Other, false).prepare_feedback_attachments(); assert_eq!(without_logs.attachment_paths, Vec::::new()); assert!(without_logs._diagnostics_attachment.is_none()); - let with_logs = make_view(true).prepare_feedback_attachments(); + let with_logs = make_view(FeedbackCategory::Other, true).prepare_feedback_attachments(); assert_eq!(with_logs.attachment_paths.len(), 1); assert_eq!( with_logs.attachment_paths[0].file_name(), @@ -811,6 +815,14 @@ mod tests { with_logs._diagnostics_attachment.is_some(), "diagnostics attachment should stay alive until upload completes" ); + + let good_result_with_logs = + make_view(FeedbackCategory::GoodResult, true).prepare_feedback_attachments(); + assert_eq!( + good_result_with_logs.attachment_paths, + Vec::::new() + ); + assert!(good_result_with_logs._diagnostics_attachment.is_none()); } #[test] diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 16c32960ac5..779d6d6d873 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1293,14 +1293,11 @@ impl ChatWidget { pub(crate) fn open_feedback_consent(&mut self, category: crate::app_event::FeedbackCategory) { let diagnostics = codex_core::feedback_diagnostics::FeedbackDiagnostics::collect_from_env(); - if !crate::bottom_pane::should_show_feedback_connectivity_details(category, &diagnostics) { - self.show_feedback_note(category, false, diagnostics); - return; - } let params = crate::bottom_pane::feedback_upload_consent_params( self.app_event_tx.clone(), category, self.current_rollout_path.clone(), + crate::bottom_pane::should_show_feedback_connectivity_details(category, &diagnostics), ); 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..8ba56f905cb --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_consent_popup.snap @@ -0,0 +1,14 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: popup +--- + Upload logs? + + The following files will be sent: + • codex-logs.log + +› 1. Yes Share the current Codex session logs with the team for + troubleshooting. + 2. No Send feedback without any attached files. + + Press enter to confirm or esc to go back diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap deleted file mode 100644 index 4ad1965411a..00000000000 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__feedback_good_result_note_without_diagnostics.snap +++ /dev/null @@ -1,9 +0,0 @@ ---- -source: tui/src/chatwidget/tests.rs -expression: popup ---- -▌ Tell us more (good result) -▌ -▌ (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/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 56466513bae..922bb69be7a 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 @@ -1,13 +1,12 @@ --- source: tui/src/chatwidget/tests.rs -assertion_line: 6401 expression: popup --- Upload logs? The following files will be sent: • codex-logs.log - • codex-connectivity-diagnostics.txt (if connectivity diagnostics are dete + • 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 813909c14e8..70a2ef2311c 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -6440,6 +6440,7 @@ async fn feedback_upload_consent_popup_snapshot() { chat.app_event_tx.clone(), crate::app_event::FeedbackCategory::Bug, chat.current_rollout_path.clone(), + true, )); let popup = render_bottom_popup(&chat, 80); @@ -6447,13 +6448,18 @@ async fn feedback_upload_consent_popup_snapshot() { } #[tokio::test] -async fn feedback_good_result_skips_attachment_prompt() { +async fn feedback_good_result_consent_popup_omits_connectivity_diagnostics() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; - chat.open_feedback_consent(crate::app_event::FeedbackCategory::GoodResult); + 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(), + false, + )); let popup = render_bottom_popup(&chat, 80); - assert_snapshot!("feedback_good_result_note_without_diagnostics", popup); + assert_snapshot!("feedback_good_result_consent_popup", popup); } #[tokio::test] From 32db9eb2373dfbdf9db85ade5a550456a4d6d963 Mon Sep 17 00:00:00 2001 From: Roy Han Date: Tue, 3 Mar 2026 11:34:37 -0800 Subject: [PATCH 09/14] remove no message --- codex-rs/tui/src/bottom_pane/feedback_view.rs | 1 - ...__chatwidget__tests__feedback_good_result_consent_popup.snap | 2 +- ...x_tui__chatwidget__tests__feedback_upload_consent_popup.snap | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/feedback_view.rs b/codex-rs/tui/src/bottom_pane/feedback_view.rs index 797ce1a70c1..cc4de0d7192 100644 --- a/codex-rs/tui/src/bottom_pane/feedback_view.rs +++ b/codex-rs/tui/src/bottom_pane/feedback_view.rs @@ -634,7 +634,6 @@ pub(crate) fn feedback_upload_consent_params( }, super::SelectionItem { name: "No".to_string(), - description: Some("Send feedback without any attached files.".to_string()), actions: vec![no_action], dismiss_on_select: true, ..Default::default() 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 index 8ba56f905cb..cc3d8e37559 100644 --- 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 @@ -9,6 +9,6 @@ expression: popup › 1. Yes Share the current Codex session logs with the team for troubleshooting. - 2. No Send feedback without any attached files. + 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 922bb69be7a..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 @@ -10,6 +10,6 @@ expression: popup › 1. Yes Share the current Codex session logs with the team for troubleshooting. - 2. No Send feedback without any attached files. + 2. No Press enter to confirm or esc to go back From 9ec4fa6a991fb06bd2bd94bc70b4bd8a6b675a3d Mon Sep 17 00:00:00 2001 From: Roy Han Date: Tue, 3 Mar 2026 12:16:12 -0800 Subject: [PATCH 10/14] move logic from core to feedback crate --- codex-rs/Cargo.lock | 2 ++ codex-rs/core/src/lib.rs | 1 - codex-rs/feedback/Cargo.toml | 2 ++ codex-rs/{core => feedback}/src/feedback_diagnostics.rs | 3 ++- codex-rs/feedback/src/lib.rs | 2 ++ codex-rs/tui/src/bottom_pane/feedback_view.rs | 6 +++--- codex-rs/tui/src/chatwidget.rs | 8 +++++--- 7 files changed, 16 insertions(+), 8 deletions(-) rename codex-rs/{core => feedback}/src/feedback_diagnostics.rs (99%) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 62d2ffae3b0..f5fe5be307b 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1997,8 +1997,10 @@ dependencies = [ "codex-protocol", "pretty_assertions", "sentry", + "tempfile", "tracing", "tracing-subscriber", + "url", ] [[package]] diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index a4e78e9f366..ef988f1c52c 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -37,7 +37,6 @@ pub mod exec_env; mod exec_policy; pub mod external_agent_config; pub mod features; -pub mod feedback_diagnostics; mod file_watcher; mod flags; pub mod git_info; diff --git a/codex-rs/feedback/Cargo.toml b/codex-rs/feedback/Cargo.toml index 73803af86a3..f674bc7151b 100644 --- a/codex-rs/feedback/Cargo.toml +++ b/codex-rs/feedback/Cargo.toml @@ -8,8 +8,10 @@ license.workspace = true anyhow = { workspace = true } codex-protocol = { workspace = true } sentry = { version = "0.46" } +tempfile = { workspace = true } tracing = { workspace = true } tracing-subscriber = { workspace = true } +url = { workspace = true } [dev-dependencies] pretty_assertions = { workspace = true } diff --git a/codex-rs/core/src/feedback_diagnostics.rs b/codex-rs/feedback/src/feedback_diagnostics.rs similarity index 99% rename from codex-rs/core/src/feedback_diagnostics.rs rename to codex-rs/feedback/src/feedback_diagnostics.rs index f298a4fde3c..48fdbe998b4 100644 --- a/codex-rs/core/src/feedback_diagnostics.rs +++ b/codex-rs/feedback/src/feedback_diagnostics.rs @@ -167,10 +167,11 @@ fn sanitize_proxy_value(raw: &str) -> Option { #[cfg(test)] mod tests { - use pretty_assertions::assert_eq; use std::ffi::OsStr; use std::fs; + use pretty_assertions::assert_eq; + use super::FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME; use super::FeedbackDiagnostic; use super::FeedbackDiagnostics; diff --git a/codex-rs/feedback/src/lib.rs b/codex-rs/feedback/src/lib.rs index a4fe6cc1859..123ad2c03b5 100644 --- a/codex-rs/feedback/src/lib.rs +++ b/codex-rs/feedback/src/lib.rs @@ -21,6 +21,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"; diff --git a/codex-rs/tui/src/bottom_pane/feedback_view.rs b/codex-rs/tui/src/bottom_pane/feedback_view.rs index cc4de0d7192..dc790ea7615 100644 --- a/codex-rs/tui/src/bottom_pane/feedback_view.rs +++ b/codex-rs/tui/src/bottom_pane/feedback_view.rs @@ -1,9 +1,9 @@ use std::cell::RefCell; use std::path::PathBuf; -use codex_core::feedback_diagnostics::FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME; -use codex_core::feedback_diagnostics::FeedbackDiagnostics; -use codex_core::feedback_diagnostics::FeedbackDiagnosticsAttachment; +use codex_feedback::feedback_diagnostics::FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME; +use codex_feedback::feedback_diagnostics::FeedbackDiagnostics; +use codex_feedback::feedback_diagnostics::FeedbackDiagnosticsAttachment; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyModifiers; diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 779d6d6d873..14a0a721464 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1255,7 +1255,8 @@ impl ChatWidget { category: crate::app_event::FeedbackCategory, include_logs: bool, ) { - let diagnostics = codex_core::feedback_diagnostics::FeedbackDiagnostics::collect_from_env(); + let diagnostics = + codex_feedback::feedback_diagnostics::FeedbackDiagnostics::collect_from_env(); self.show_feedback_note(category, include_logs, diagnostics); } @@ -1263,7 +1264,7 @@ impl ChatWidget { &mut self, category: crate::app_event::FeedbackCategory, include_logs: bool, - diagnostics: codex_core::feedback_diagnostics::FeedbackDiagnostics, + diagnostics: codex_feedback::feedback_diagnostics::FeedbackDiagnostics, ) { // Build a fresh snapshot at the time of opening the note overlay. let snapshot = self.feedback.snapshot(self.thread_id); @@ -1292,7 +1293,8 @@ impl ChatWidget { } pub(crate) fn open_feedback_consent(&mut self, category: crate::app_event::FeedbackCategory) { - let diagnostics = codex_core::feedback_diagnostics::FeedbackDiagnostics::collect_from_env(); + let diagnostics = + codex_feedback::feedback_diagnostics::FeedbackDiagnostics::collect_from_env(); let params = crate::bottom_pane::feedback_upload_consent_params( self.app_event_tx.clone(), category, From 198d39d1f6060f11ba89bbc379b1a834d690ef6c Mon Sep 17 00:00:00 2001 From: Roy Han Date: Tue, 3 Mar 2026 14:41:08 -0800 Subject: [PATCH 11/14] auto attach diagnostics under unified feedback snapshot --- codex-rs/Cargo.lock | 1 - codex-rs/feedback/Cargo.toml | 1 - codex-rs/feedback/src/feedback_diagnostics.rs | 60 +-------- codex-rs/feedback/src/lib.rs | 37 ++++- codex-rs/tui/src/bottom_pane/feedback_view.rs | 127 +++++++----------- codex-rs/tui/src/chatwidget.rs | 18 ++- 6 files changed, 91 insertions(+), 153 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index e2ea5a6fe1d..8c0920c65d2 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1995,7 +1995,6 @@ dependencies = [ "codex-protocol", "pretty_assertions", "sentry", - "tempfile", "tracing", "tracing-subscriber", "url", diff --git a/codex-rs/feedback/Cargo.toml b/codex-rs/feedback/Cargo.toml index f674bc7151b..43d572f895a 100644 --- a/codex-rs/feedback/Cargo.toml +++ b/codex-rs/feedback/Cargo.toml @@ -8,7 +8,6 @@ license.workspace = true anyhow = { workspace = true } codex-protocol = { workspace = true } sentry = { version = "0.46" } -tempfile = { workspace = true } tracing = { workspace = true } tracing-subscriber = { workspace = true } url = { workspace = true } diff --git a/codex-rs/feedback/src/feedback_diagnostics.rs b/codex-rs/feedback/src/feedback_diagnostics.rs index 48fdbe998b4..5497fe0579e 100644 --- a/codex-rs/feedback/src/feedback_diagnostics.rs +++ b/codex-rs/feedback/src/feedback_diagnostics.rs @@ -1,11 +1,5 @@ use std::collections::HashMap; -use std::fs; -use std::io; -use std::path::Path; -use std::path::PathBuf; -use tempfile::Builder; -use tempfile::TempDir; use url::Url; const DEFAULT_OPENAI_BASE_URL: &str = "https://api.openai.com/v1"; @@ -31,23 +25,16 @@ pub struct FeedbackDiagnostic { pub details: Vec, } -pub struct FeedbackDiagnosticsAttachment { - _dir: TempDir, - path: PathBuf, -} - -impl FeedbackDiagnosticsAttachment { - pub fn path(&self) -> &Path { - &self.path +impl FeedbackDiagnostics { + pub fn new(diagnostics: Vec) -> Self { + Self { diagnostics } } -} -impl FeedbackDiagnostics { pub fn collect_from_env() -> Self { Self::collect_from_pairs(std::env::vars()) } - pub fn collect_from_pairs(pairs: I) -> Self + fn collect_from_pairs(pairs: I) -> Self where I: IntoIterator, K: Into, @@ -125,20 +112,6 @@ impl FeedbackDiagnostics { Some(lines.join("\n")) } - - pub fn write_temp_attachment(&self) -> io::Result> { - let Some(text) = self.attachment_text() else { - return Ok(None); - }; - - let dir = Builder::new() - .prefix("codex-connectivity-diagnostics-") - .tempdir()?; - let path = dir.path().join(FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME); - fs::write(&path, text)?; - - Ok(Some(FeedbackDiagnosticsAttachment { _dir: dir, path })) - } } pub fn sanitize_url_for_display(raw: &str) -> Option { @@ -167,12 +140,8 @@ fn sanitize_proxy_value(raw: &str) -> Option { #[cfg(test)] mod tests { - use std::ffi::OsStr; - use std::fs; - use pretty_assertions::assert_eq; - use super::FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME; use super::FeedbackDiagnostic; use super::FeedbackDiagnostics; use super::sanitize_url_for_display; @@ -211,21 +180,12 @@ mod tests { } ); - let attachment = diagnostics - .write_temp_attachment() - .expect("attachment should be written") - .expect("attachment should be present"); - let contents = - fs::read_to_string(attachment.path()).expect("attachment should be readable"); - assert_eq!( - attachment.path().file_name(), - Some(OsStr::new(FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME)) - ); - assert_eq!( - contents, + 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() + ) ); } @@ -240,12 +200,6 @@ mod tests { ] { assert_eq!(diagnostics, FeedbackDiagnostics::default()); assert_eq!(diagnostics.attachment_text(), None); - assert!( - diagnostics - .write_temp_attachment() - .expect("empty diagnostics should not write attachment") - .is_none() - ); } } diff --git a/codex-rs/feedback/src/lib.rs b/codex-rs/feedback/src/lib.rs index 3ba17c5377e..5623dd4f095 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; @@ -90,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() @@ -99,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()), @@ -201,17 +204,27 @@ 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 save_to_temp_file(&self) -> io::Result { let dir = std::env::temp_dir(); let filename = format!("codex-feedback-{}.log", self.thread_id); @@ -226,7 +239,7 @@ 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<()> { @@ -320,7 +333,19 @@ impl CodexLogSnapshot { })); } - for path in extra_log_files { + if include_logs + && classification != "good_result" + && let Some(text) = self.feedback_diagnostics.attachment_text() + { + envelope.add_item(EnvelopeItem::Attachment(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) => { diff --git a/codex-rs/tui/src/bottom_pane/feedback_view.rs b/codex-rs/tui/src/bottom_pane/feedback_view.rs index 9b0cdfdde4c..a6dbbaa6d8f 100644 --- a/codex-rs/tui/src/bottom_pane/feedback_view.rs +++ b/codex-rs/tui/src/bottom_pane/feedback_view.rs @@ -3,7 +3,6 @@ use std::path::PathBuf; use codex_feedback::feedback_diagnostics::FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME; use codex_feedback::feedback_diagnostics::FeedbackDiagnostics; -use codex_feedback::feedback_diagnostics::FeedbackDiagnosticsAttachment; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyModifiers; @@ -51,9 +50,8 @@ 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, - diagnostics: FeedbackDiagnostics, app_event_tx: AppEventSender, include_logs: bool, feedback_audience: FeedbackAudience, @@ -64,17 +62,11 @@ pub(crate) struct FeedbackNoteView { complete: bool, } -struct PreparedFeedbackAttachments { - attachment_paths: Vec, - _diagnostics_attachment: Option, -} - impl FeedbackNoteView { pub(crate) fn new( category: FeedbackCategory, - snapshot: codex_feedback::CodexLogSnapshot, + snapshot: codex_feedback::FeedbackSnapshot, rollout_path: Option, - diagnostics: FeedbackDiagnostics, app_event_tx: AppEventSender, include_logs: bool, feedback_audience: FeedbackAudience, @@ -83,7 +75,6 @@ impl FeedbackNoteView { category, snapshot, rollout_path, - diagnostics, app_event_tx, include_logs, feedback_audience, @@ -100,7 +91,11 @@ impl FeedbackNoteView { } else { Some(note.as_str()) }; - let attachments = self.prepare_feedback_attachments(); + let attachment_paths = if self.include_logs { + self.rollout_path.iter().cloned().collect::>() + } else { + Vec::new() + }; let classification = feedback_classification(self.category); let mut thread_id = self.snapshot.thread_id.clone(); @@ -109,7 +104,7 @@ impl FeedbackNoteView { classification, reason_opt, self.include_logs, - &attachments.attachment_paths, + &attachment_paths, Some(SessionSource::Cli), None, ); @@ -350,7 +345,10 @@ impl FeedbackNoteView { 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.diagnostics) { + if should_show_feedback_connectivity_details( + self.category, + self.snapshot.feedback_diagnostics(), + ) { lines.push(Line::from(vec![gutter()])); lines.push(Line::from(vec![ gutter(), @@ -371,7 +369,7 @@ impl FeedbackNoteView { .subsequent_indent(Line::from(vec![gutter(), " ".into()])); let mut lines = Vec::new(); - for diagnostic in self.diagnostics.diagnostics() { + for diagnostic in self.snapshot.feedback_diagnostics().diagnostics() { lines.extend(word_wrap_lines( [Line::from(diagnostic.headline.clone())], headline_options.clone(), @@ -386,35 +384,6 @@ impl FeedbackNoteView { lines } - - fn prepare_feedback_attachments(&self) -> PreparedFeedbackAttachments { - let mut attachment_paths = if self.include_logs { - self.rollout_path.iter().cloned().collect::>() - } else { - Vec::new() - }; - let diagnostics_attachment = if self.include_logs - && should_show_feedback_connectivity_details(self.category, &self.diagnostics) - { - match self.diagnostics.write_temp_attachment() { - Ok(attachment) => attachment, - Err(err) => { - tracing::warn!(error = %err, "failed to write diagnostics attachment"); - None - } - } - } else { - None - }; - if let Some(attachment) = diagnostics_attachment.as_ref() { - attachment_paths.push(attachment.path().to_path_buf()); - } - - PreparedFeedbackAttachments { - attachment_paths, - _diagnostics_attachment: diagnostics_attachment, - } - } } pub(crate) fn should_show_feedback_connectivity_details( @@ -652,8 +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; - use std::ffi::OsStr; fn render(view: &FeedbackNoteView, width: u16) -> String { let height = view.desired_height(width); @@ -693,7 +662,6 @@ mod tests { category, snapshot, None, - FeedbackDiagnostics::default(), tx, true, FeedbackAudience::External, @@ -739,16 +707,24 @@ mod tests { fn feedback_view_with_connectivity_diagnostics() { let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::(); let tx = AppEventSender::new(tx_raw); - let snapshot = codex_feedback::CodexFeedback::new().snapshot(None); - let diagnostics = FeedbackDiagnostics::collect_from_pairs([ - ("HTTP_PROXY", "proxy.example.com:8080"), - ("OPENAI_BASE_URL", "https://example.com/v1"), + 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, - diagnostics, tx, false, FeedbackAudience::External, @@ -760,8 +736,11 @@ mod tests { #[test] fn should_show_feedback_connectivity_details_only_for_non_good_result_with_diagnostics() { - let diagnostics = - FeedbackDiagnostics::collect_from_pairs([("HTTP_PROXY", "proxy.example.com:8080")]); + 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), @@ -781,48 +760,32 @@ mod tests { } #[test] - fn prepare_feedback_attachments_only_includes_diagnostics_when_logs_are_enabled() { - let diagnostics = FeedbackDiagnostics::collect_from_pairs([( - "OPENAI_BASE_URL", - "https://example.com/v1", - )]); - let make_view = |category, include_logs| { + fn feedback_snapshot_override_controls_connectivity_details() { + let 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 make_view = |category| { let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::(); let tx = AppEventSender::new(tx_raw); - let snapshot = codex_feedback::CodexFeedback::new().snapshot(None); + let snapshot = codex_feedback::CodexFeedback::new() + .snapshot(None) + .with_feedback_diagnostics(diagnostics.clone()); FeedbackNoteView::new( category, snapshot, None, - diagnostics.clone(), tx, - include_logs, + true, FeedbackAudience::External, ) }; - let without_logs = make_view(FeedbackCategory::Other, false).prepare_feedback_attachments(); - assert_eq!(without_logs.attachment_paths, Vec::::new()); - assert!(without_logs._diagnostics_attachment.is_none()); - - let with_logs = make_view(FeedbackCategory::Other, true).prepare_feedback_attachments(); - assert_eq!(with_logs.attachment_paths.len(), 1); - assert_eq!( - with_logs.attachment_paths[0].file_name(), - Some(OsStr::new(FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME)) - ); + assert!(render(&make_view(FeedbackCategory::Bug), 60).contains("Connectivity diagnostics")); assert!( - with_logs._diagnostics_attachment.is_some(), - "diagnostics attachment should stay alive until upload completes" - ); - - let good_result_with_logs = - make_view(FeedbackCategory::GoodResult, true).prepare_feedback_attachments(); - assert_eq!( - good_result_with_logs.attachment_paths, - Vec::::new() + !render(&make_view(FeedbackCategory::GoodResult), 60) + .contains("Connectivity diagnostics") ); - assert!(good_result_with_logs._diagnostics_attachment.is_none()); } #[test] diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 14a0a721464..6b78676637c 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1255,19 +1255,16 @@ impl ChatWidget { category: crate::app_event::FeedbackCategory, include_logs: bool, ) { - let diagnostics = - codex_feedback::feedback_diagnostics::FeedbackDiagnostics::collect_from_env(); - self.show_feedback_note(category, include_logs, diagnostics); + 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, - diagnostics: codex_feedback::feedback_diagnostics::FeedbackDiagnostics, + snapshot: codex_feedback::FeedbackSnapshot, ) { - // Build a fresh snapshot at the time of opening the note overlay. - let snapshot = self.feedback.snapshot(self.thread_id); let rollout = if include_logs { self.current_rollout_path.clone() } else { @@ -1277,7 +1274,6 @@ impl ChatWidget { category, snapshot, rollout, - diagnostics, self.app_event_tx.clone(), include_logs, self.feedback_audience, @@ -1293,13 +1289,15 @@ impl ChatWidget { } pub(crate) fn open_feedback_consent(&mut self, category: crate::app_event::FeedbackCategory) { - let diagnostics = - codex_feedback::feedback_diagnostics::FeedbackDiagnostics::collect_from_env(); + 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(), - crate::bottom_pane::should_show_feedback_connectivity_details(category, &diagnostics), + crate::bottom_pane::should_show_feedback_connectivity_details( + category, + snapshot.feedback_diagnostics(), + ), ); self.bottom_pane.show_selection_view(params); self.request_redraw(); From a4b4f0d47e328873d3f362cd8380841b063ac4cc Mon Sep 17 00:00:00 2001 From: Roy Han Date: Tue, 3 Mar 2026 15:25:55 -0800 Subject: [PATCH 12/14] always full snapshot --- codex-rs/feedback/src/lib.rs | 96 +++++++++++++++---- codex-rs/tui/src/bottom_pane/mod.rs | 1 - codex-rs/tui/src/chatwidget.rs | 5 +- ...s__feedback_good_result_consent_popup.snap | 1 + codex-rs/tui/src/chatwidget/tests.rs | 4 +- 5 files changed, 81 insertions(+), 26 deletions(-) diff --git a/codex-rs/feedback/src/lib.rs b/codex-rs/feedback/src/lib.rs index 5623dd4f095..b1f957eca9d 100644 --- a/codex-rs/feedback/src/lib.rs +++ b/codex-rs/feedback/src/lib.rs @@ -244,13 +244,11 @@ impl FeedbackSnapshot { 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; @@ -324,25 +322,43 @@ impl FeedbackSnapshot { } 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, - })); + }); } - if include_logs - && classification != "good_result" - && let Some(text) = self.feedback_diagnostics.attachment_text() - { - envelope.add_item(EnvelopeItem::Attachment(Attachment { + if include_logs && let Some(text) = self.feedback_diagnostics.attachment_text() { + 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 { @@ -357,22 +373,19 @@ impl FeedbackSnapshot { 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 } } @@ -457,7 +470,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; @@ -487,4 +505,44 @@ 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_include_connectivity_diagnostics_for_good_result() { + 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 = 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 = + snapshot.feedback_attachments(true, std::slice::from_ref(&extra_path), Some(vec![1])); + + assert_eq!( + attachments + .iter() + .map(|attachment| attachment.filename.as_str()) + .collect::>(), + vec![ + "codex-logs.log", + FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME, + extra_filename.as_str() + ] + ); + assert_eq!(attachments[0].buffer, vec![1]); + assert_eq!( + attachments[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[2].buffer, b"rollout".to_vec()); + assert_eq!( + OsStr::new(attachments[2].filename.as_str()), + OsStr::new(extra_filename.as_str()) + ); + fs::remove_file(extra_path).expect("extra attachment should be removed"); + } } diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 63185a7250e..02c7faeebb3 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -101,7 +101,6 @@ mod selection_popup_common; mod textarea; mod unified_exec_footer; pub(crate) use feedback_view::FeedbackNoteView; -pub(crate) use feedback_view::should_show_feedback_connectivity_details; /// How long the "press again to quit" hint stays visible. /// diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 6b78676637c..316184d3c43 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1294,10 +1294,7 @@ impl ChatWidget { self.app_event_tx.clone(), category, self.current_rollout_path.clone(), - crate::bottom_pane::should_show_feedback_connectivity_details( - category, - snapshot.feedback_diagnostics(), - ), + !snapshot.feedback_diagnostics().is_empty(), ); 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 index cc3d8e37559..4529d6d478a 100644 --- 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 @@ -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 70a2ef2311c..b7dbdcb4099 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -6448,14 +6448,14 @@ async fn feedback_upload_consent_popup_snapshot() { } #[tokio::test] -async fn feedback_good_result_consent_popup_omits_connectivity_diagnostics() { +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(), - false, + true, )); let popup = render_bottom_popup(&chat, 80); From b4db5e3bd1c583cefa6824ddcd19be6e729c0b0a Mon Sep 17 00:00:00 2001 From: Roy Han Date: Tue, 3 Mar 2026 16:00:50 -0800 Subject: [PATCH 13/14] align consistency of diagnostic presence --- codex-rs/feedback/src/lib.rs | 57 +++++++++++++++++++++++++++++++++- codex-rs/tui/src/chatwidget.rs | 4 ++- 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/codex-rs/feedback/src/lib.rs b/codex-rs/feedback/src/lib.rs index b1f957eca9d..29fc7e5829e 100644 --- a/codex-rs/feedback/src/lib.rs +++ b/codex-rs/feedback/src/lib.rs @@ -225,6 +225,14 @@ impl FeedbackSnapshot { 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); @@ -352,7 +360,7 @@ impl FeedbackSnapshot { }); } - if include_logs && let Some(text) = self.feedback_diagnostics.attachment_text() { + 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(), @@ -545,4 +553,51 @@ mod tests { ); fs::remove_file(extra_path).expect("extra attachment should be removed"); } + + #[test] + fn feedback_attachments_omit_connectivity_diagnostics_when_none_detected() { + let snapshot = CodexFeedback::new().snapshot(None); + + let attachments = snapshot.feedback_attachments(true, &[], Some(vec![1])); + + assert_eq!( + attachments + .iter() + .map(|attachment| attachment.filename.as_str()) + .collect::>(), + vec!["codex-logs.log"] + ); + assert_eq!(attachments[0].buffer, vec![1]); + } + + #[test] + fn feedback_diagnostics_attachment_text_matches_upload_conditions() { + let snapshot_without_diagnostics = CodexFeedback::new().snapshot(None); + assert_eq!( + snapshot_without_diagnostics.feedback_diagnostics_attachment_text(true), + None + ); + assert_eq!( + snapshot_without_diagnostics.feedback_diagnostics_attachment_text(false), + None + ); + + let snapshot_with_diagnostics = + snapshot_without_diagnostics.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()], + }, + ])); + assert_eq!( + snapshot_with_diagnostics.feedback_diagnostics_attachment_text(false), + None + ); + assert_eq!( + snapshot_with_diagnostics.feedback_diagnostics_attachment_text(true), + Some( + "Connectivity diagnostics\n\n- OPENAI_BASE_URL is set and may affect connectivity.\n - OPENAI_BASE_URL = https://example.com/v1".to_string() + ) + ); + } } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 316184d3c43..3c200131564 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1294,7 +1294,9 @@ impl ChatWidget { self.app_event_tx.clone(), category, self.current_rollout_path.clone(), - !snapshot.feedback_diagnostics().is_empty(), + snapshot + .feedback_diagnostics_attachment_text(true) + .is_some(), ); self.bottom_pane.show_selection_view(params); self.request_redraw(); From 473fc7642888ce575c37b1fa8dd088c2d3c19175 Mon Sep 17 00:00:00 2001 From: Roy Han Date: Tue, 3 Mar 2026 16:16:07 -0800 Subject: [PATCH 14/14] test condensation --- codex-rs/feedback/src/lib.rs | 68 +++++-------------- codex-rs/tui/src/bottom_pane/feedback_view.rs | 29 -------- 2 files changed, 18 insertions(+), 79 deletions(-) diff --git a/codex-rs/feedback/src/lib.rs b/codex-rs/feedback/src/lib.rs index 29fc7e5829e..0bd76936aa1 100644 --- a/codex-rs/feedback/src/lib.rs +++ b/codex-rs/feedback/src/lib.rs @@ -515,23 +515,26 @@ mod tests { } #[test] - fn feedback_attachments_include_connectivity_diagnostics_for_good_result() { + 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 = CodexFeedback::new() + 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 = - snapshot.feedback_attachments(true, std::slice::from_ref(&extra_path), Some(vec![1])); + let attachments_with_diagnostics = snapshot_with_diagnostics.feedback_attachments( + true, + std::slice::from_ref(&extra_path), + Some(vec![1]), + ); assert_eq!( - attachments + attachments_with_diagnostics .iter() .map(|attachment| attachment.filename.as_str()) .collect::>(), @@ -541,63 +544,28 @@ mod tests { extra_filename.as_str() ] ); - assert_eq!(attachments[0].buffer, vec![1]); + assert_eq!(attachments_with_diagnostics[0].buffer, vec![1]); assert_eq!( - attachments[1].buffer, + 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[2].buffer, b"rollout".to_vec()); + assert_eq!(attachments_with_diagnostics[2].buffer, b"rollout".to_vec()); assert_eq!( - OsStr::new(attachments[2].filename.as_str()), + OsStr::new(attachments_with_diagnostics[2].filename.as_str()), OsStr::new(extra_filename.as_str()) ); - fs::remove_file(extra_path).expect("extra attachment should be removed"); - } - - #[test] - fn feedback_attachments_omit_connectivity_diagnostics_when_none_detected() { - let snapshot = CodexFeedback::new().snapshot(None); - - let attachments = snapshot.feedback_attachments(true, &[], Some(vec![1])); + let attachments_without_diagnostics = CodexFeedback::new() + .snapshot(None) + .feedback_attachments(true, &[], Some(vec![1])); assert_eq!( - attachments + attachments_without_diagnostics .iter() .map(|attachment| attachment.filename.as_str()) .collect::>(), vec!["codex-logs.log"] ); - assert_eq!(attachments[0].buffer, vec![1]); - } - - #[test] - fn feedback_diagnostics_attachment_text_matches_upload_conditions() { - let snapshot_without_diagnostics = CodexFeedback::new().snapshot(None); - assert_eq!( - snapshot_without_diagnostics.feedback_diagnostics_attachment_text(true), - None - ); - assert_eq!( - snapshot_without_diagnostics.feedback_diagnostics_attachment_text(false), - None - ); - - let snapshot_with_diagnostics = - snapshot_without_diagnostics.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()], - }, - ])); - assert_eq!( - snapshot_with_diagnostics.feedback_diagnostics_attachment_text(false), - None - ); - assert_eq!( - snapshot_with_diagnostics.feedback_diagnostics_attachment_text(true), - Some( - "Connectivity diagnostics\n\n- OPENAI_BASE_URL is set and may affect connectivity.\n - OPENAI_BASE_URL = https://example.com/v1".to_string() - ) - ); + 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 a6dbbaa6d8f..f9bbfe24368 100644 --- a/codex-rs/tui/src/bottom_pane/feedback_view.rs +++ b/codex-rs/tui/src/bottom_pane/feedback_view.rs @@ -759,35 +759,6 @@ mod tests { ); } - #[test] - fn feedback_snapshot_override_controls_connectivity_details() { - let 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 make_view = |category| { - let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::(); - let tx = AppEventSender::new(tx_raw); - let snapshot = codex_feedback::CodexFeedback::new() - .snapshot(None) - .with_feedback_diagnostics(diagnostics.clone()); - FeedbackNoteView::new( - category, - snapshot, - None, - tx, - true, - FeedbackAudience::External, - ) - }; - - assert!(render(&make_view(FeedbackCategory::Bug), 60).contains("Connectivity diagnostics")); - assert!( - !render(&make_view(FeedbackCategory::GoodResult), 60) - .contains("Connectivity diagnostics") - ); - } - #[test] fn issue_url_available_for_bug_bad_result_safety_check_and_other() { let bug_url = issue_url_for_category(