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