diff --git a/codex-rs/cli/src/doctor/output.rs b/codex-rs/cli/src/doctor/output.rs index 70f1c814af..a6142503de 100644 --- a/codex-rs/cli/src/doctor/output.rs +++ b/codex-rs/cli/src/doctor/output.rs @@ -146,7 +146,7 @@ fn write_check_row(out: &mut String, check: &DoctorCheck, options: HumanOutputOp if options.show_details { for detail in detail_lines(check, options) { - write_detail_line(out, detail, options); + write_detail_line(out, detail, status, options); } } } @@ -161,7 +161,12 @@ fn write_note_row(out: &mut String, note: &DoctorNote, options: HumanOutputOptio ); } -fn write_detail_line(out: &mut String, detail: HumanDetail, options: HumanOutputOptions) { +fn write_detail_line( + out: &mut String, + detail: HumanDetail, + status: DisplayStatus, + options: HumanOutputOptions, +) { match detail { HumanDetail::Row { label, @@ -173,11 +178,11 @@ fn write_detail_line(out: &mut String, detail: HumanDetail, options: HumanOutput let value = if let Some(expected) = expected { format!( "{} {}", - detail_value(&value, options), + detail_value(&value, status, options), dim(&format!("(expected {expected})"), options) ) } else { - detail_value(&value, options) + detail_value(&value, status, options) }; let _ = writeln!( out, @@ -193,7 +198,7 @@ fn write_detail_line(out: &mut String, detail: HumanDetail, options: HumanOutput out, " {} {}", detail_label(&spacer, options), - detail_value(&value, options) + detail_value(&value, status, options) ); } HumanDetail::Bullet(value) => { @@ -201,7 +206,7 @@ fn write_detail_line(out: &mut String, detail: HumanDetail, options: HumanOutput out, " {} {}", very_dim(if options.ascii { "-" } else { "·" }, options), - dim(&highlight_actions(&value, options), options) + dim(&highlight_actions(&value, status, options), options) ); } HumanDetail::Remedy(value) => { @@ -210,7 +215,7 @@ fn write_detail_line(out: &mut String, detail: HumanDetail, options: HumanOutput out, " {} {}", orange(marker, options), - highlight_actions(&value, options) + highlight_actions(&value, status, options) ); } } @@ -321,7 +326,7 @@ fn style_description( status: DisplayStatus, options: HumanOutputOptions, ) -> String { - let highlighted = highlight_actions(description, options); + let highlighted = highlight_actions(description, status, options); match status { DisplayStatus::Ok | DisplayStatus::Idle => dim(&highlighted, options), DisplayStatus::Update => amber(&highlighted, options), @@ -805,7 +810,7 @@ fn separator(options: HumanOutputOptions) -> String { } } -fn highlight_actions(text: &str, options: HumanOutputOptions) -> String { +fn highlight_actions(text: &str, status: DisplayStatus, options: HumanOutputOptions) -> String { if !options.color_enabled { return text.to_string(); } @@ -813,33 +818,40 @@ fn highlight_actions(text: &str, options: HumanOutputOptions) -> String { let mut out = String::new(); let mut parts = text.split('`'); if let Some(first) = parts.next() { - out.push_str(&highlight_flags(first, options)); + out.push_str(&highlight_plain_text(first, status, options)); } let mut in_code = true; for part in parts { if in_code { - out.push_str(&cyan(part, options)); + let highlighted = if status == DisplayStatus::Fail && looks_copyable(part) { + red(part, options) + } else { + cyan(part, options) + }; + out.push_str(&highlighted); } else { - out.push_str(&highlight_flags(part, options)); + out.push_str(&highlight_plain_text(part, status, options)); } in_code = !in_code; } out } -fn highlight_flags(text: &str, options: HumanOutputOptions) -> String { +fn highlight_plain_text(text: &str, status: DisplayStatus, options: HumanOutputOptions) -> String { text.split_inclusive(char::is_whitespace) .map(|token| { let trimmed = token.trim_end(); let suffix = &token[trimmed.len()..]; let bare = trimmed.trim_end_matches([',', '.', ':', ';', ')']); let punctuation = &trimmed[bare.len()..]; - if bare.starts_with("--") { - let highlighted = cyan(bare, options); - format!("{highlighted}{punctuation}{suffix}") + let highlighted = if status == DisplayStatus::Fail && looks_copyable(bare) { + red(bare, options) + } else if bare.starts_with("--") || looks_copyable(bare) { + cyan(bare, options) } else { - token.to_string() - } + bare.to_string() + }; + format!("{highlighted}{punctuation}{suffix}") }) .collect() } @@ -1051,47 +1063,60 @@ fn detail_label(text: &str, options: HumanOutputOptions) -> String { color256(text, /*code*/ 240, options) } -fn detail_value(text: &str, options: HumanOutputOptions) -> String { +fn detail_value(text: &str, status: DisplayStatus, options: HumanOutputOptions) -> String { if !options.color_enabled { return text.to_string(); } - style_detail_text(text, options) + style_detail_text(text, status, options) } -fn style_detail_text(text: &str, options: HumanOutputOptions) -> String { +fn style_detail_text(text: &str, status: DisplayStatus, options: HumanOutputOptions) -> String { let mut out = String::new(); let mut parts = text.split('`'); if let Some(first) = parts.next() { - out.push_str(&style_detail_plain_text(first, options)); + out.push_str(&style_detail_plain_text(first, status, options)); } let mut in_code = true; for part in parts { if in_code { - out.push_str(&cyan(part, options)); + let highlighted = if status == DisplayStatus::Fail && looks_copyable(part) { + red(part, options) + } else { + cyan(part, options) + }; + out.push_str(&highlighted); } else { - out.push_str(&style_detail_plain_text(part, options)); + out.push_str(&style_detail_plain_text(part, status, options)); } in_code = !in_code; } out } -fn style_detail_plain_text(text: &str, options: HumanOutputOptions) -> String { +fn style_detail_plain_text( + text: &str, + status: DisplayStatus, + options: HumanOutputOptions, +) -> String { text.split_inclusive(char::is_whitespace) - .map(|token| style_detail_token(token, options)) + .map(|token| style_detail_token(token, status, options)) .collect() } -fn style_detail_token(token: &str, options: HumanOutputOptions) -> String { +fn style_detail_token(token: &str, status: DisplayStatus, options: HumanOutputOptions) -> String { let trimmed = token.trim_end(); let suffix = &token[trimmed.len()..]; let bare = trimmed.trim_end_matches([',', '.', ':', ';', ')']); let punctuation = &trimmed[bare.len()..]; - let styled = style_detail_bare_token(bare, options); + let styled = style_detail_bare_token(bare, status, options); format!("{styled}{punctuation}{suffix}") } -fn style_detail_bare_token(bare: &str, options: HumanOutputOptions) -> String { +fn style_detail_bare_token( + bare: &str, + status: DisplayStatus, + options: HumanOutputOptions, +) -> String { if bare.is_empty() { return String::new(); } @@ -1109,6 +1134,9 @@ fn style_detail_bare_token(bare: &str, options: HumanOutputOptions) -> String { if bare == "ok" { return green(bare, options); } + if status == DisplayStatus::Fail && looks_copyable(bare) { + return red(bare, options); + } if bare.starts_with("--") || looks_copyable(bare) { return cyan(bare, options); } @@ -1147,13 +1175,24 @@ fn color256(text: &str, code: u8, options: HumanOutputOptions) -> String { } fn looks_copyable(text: &str) -> bool { - text.starts_with("http://") + if text.starts_with("http://") || text.starts_with("https://") || text.starts_with("wss://") || text.starts_with("~/") || text.starts_with('/') || text.starts_with("./") || text.starts_with("../") + { + return true; + } + + if cfg!(target_os = "windows") { + return text.starts_with(r"\\") // unc and win32 paths + || text.starts_with(r"\??\") // NT DOS paths + || matches!(text.as_bytes(), [drive, b':', b'/' | b'\\', ..] if drive.is_ascii_alphabetic()); // standard drive absolute C:/ and D:\ paths + } + + false } #[cfg(test)] @@ -1760,6 +1799,7 @@ Run codex doctor without --summary for detailed diagnostics. fn detail_value_colors_inline_statuses_and_low_signal_values() { let rendered = detail_value( "npm: no · commit unknown · integrity ok · ~/code/codex/target/debug/codex · ", + DisplayStatus::Ok, detailed_color_unicode_options(), ); @@ -1924,4 +1964,41 @@ Run codex doctor without --summary for detailed diagnostics. ); assert!(rendered.contains("\u{1b}[")); } + + #[test] + fn copyable_items_use_check_status() { + let description = + "/tmp/logs_2.sqlite, ~/goals_1.sqlite integrity check; try --summary or `codex doctor`"; + let details = r"see ./logs, ../goals. C:\logs, D:/goals. c:\logs, \\server\share\logs, \\?\C:\logs, \\?\UNC\server\share\logs, \\.\pipe\codex, \??\C:\logs, \DosDevices\C:\logs, \rooted. http://localhost:8080: https://example.com; wss://example.com) and `/tmp/my data/logs_2.sqlite`"; + let mut rendered = String::new(); + + for status in [CheckStatus::Ok, CheckStatus::Warning, CheckStatus::Fail] { + let check = DoctorCheck::new("state.paths", "state", status, description) + .detail(format!("locations: {details}")); + write_check_row(&mut rendered, &check, detailed_color_unicode_options()); + assert_eq!( + style_description( + description, + display_status(&check), + detailed_no_color_unicode_options() + ), + description + ); + assert_eq!( + detail_value( + details, + display_status(&check), + detailed_no_color_unicode_options() + ), + details + ); + } + + let snapshot_name = if cfg!(windows) { + "copyable_items_color_windows" + } else { + "copyable_items_color" + }; + insta::assert_snapshot!(snapshot_name, rendered.replace('\u{1b}', "")); + } } diff --git a/codex-rs/cli/src/doctor/snapshots/codex__doctor__output__tests__copyable_items_color.snap b/codex-rs/cli/src/doctor/snapshots/codex__doctor__output__tests__copyable_items_color.snap new file mode 100644 index 0000000000..c6c290c76b --- /dev/null +++ b/codex-rs/cli/src/doctor/snapshots/codex__doctor__output__tests__copyable_items_color.snap @@ -0,0 +1,10 @@ +--- +source: cli/src/doctor/output.rs +expression: "rendered.replace('\\u{1b}', \"\")" +--- + [38;5;10m✓[39m state [2m[38;5;117m/tmp/logs_2.sqlite[39m, [38;5;117m~/goals_1.sqlite[39m integrity check; try [38;5;117m--summary[39m or [38;5;117mcodex doctor[39m[0m + [38;5;240mlocations [39m see [38;5;117m./logs[39m, [38;5;117m../goals[39m. C:\logs, D:/goals. c:\logs, \\server\share\logs, \\?\C:\logs, \\?\UNC\server\share\logs, \\.\pipe\codex, \??\C:\logs, \DosDevices\C:\logs, \rooted. [38;5;117mhttp://localhost:8080[39m: [38;5;117mhttps://example.com[39m; [38;5;117mwss://example.com[39m) and [38;5;117m/tmp/my data/logs_2.sqlite[39m + [38;5;214m⚠[39m state [38;5;117m/tmp/logs_2.sqlite[39m, [38;5;117m~/goals_1.sqlite[39m integrity check; try [38;5;117m--summary[39m or [38;5;117mcodex doctor[39m + [38;5;240mlocations [39m see [38;5;117m./logs[39m, [38;5;117m../goals[39m. C:\logs, D:/goals. c:\logs, \\server\share\logs, \\?\C:\logs, \\?\UNC\server\share\logs, \\.\pipe\codex, \??\C:\logs, \DosDevices\C:\logs, \rooted. [38;5;117mhttp://localhost:8080[39m: [38;5;117mhttps://example.com[39m; [38;5;117mwss://example.com[39m) and [38;5;117m/tmp/my data/logs_2.sqlite[39m + [38;5;196m✗[39m state [38;5;196m/tmp/logs_2.sqlite[39m, [38;5;196m~/goals_1.sqlite[39m integrity check; try [38;5;117m--summary[39m or [38;5;117mcodex doctor[39m + [38;5;240mlocations [39m see [38;5;196m./logs[39m, [38;5;196m../goals[39m. C:\logs, D:/goals. c:\logs, \\server\share\logs, \\?\C:\logs, \\?\UNC\server\share\logs, \\.\pipe\codex, \??\C:\logs, \DosDevices\C:\logs, \rooted. [38;5;196mhttp://localhost:8080[39m: [38;5;196mhttps://example.com[39m; [38;5;196mwss://example.com[39m) and [38;5;196m/tmp/my data/logs_2.sqlite[39m diff --git a/codex-rs/cli/src/doctor/snapshots/codex__doctor__output__tests__copyable_items_color_windows.snap b/codex-rs/cli/src/doctor/snapshots/codex__doctor__output__tests__copyable_items_color_windows.snap new file mode 100644 index 0000000000..09c927698e --- /dev/null +++ b/codex-rs/cli/src/doctor/snapshots/codex__doctor__output__tests__copyable_items_color_windows.snap @@ -0,0 +1,10 @@ +--- +source: cli/src/doctor/output.rs +expression: "rendered.replace('\\u{1b}', \"\")" +--- + [38;5;10m✓[39m state [2m[38;5;117m/tmp/logs_2.sqlite[39m, [38;5;117m~/goals_1.sqlite[39m integrity check; try [38;5;117m--summary[39m or [38;5;117mcodex doctor[39m[0m + [38;5;240mlocations [39m see [38;5;117m./logs[39m, [38;5;117m../goals[39m. [38;5;117mC:\logs[39m, [38;5;117mD:/goals[39m. [38;5;117mc:\logs[39m, [38;5;117m\\server\share\logs[39m, [38;5;117m\\?\C:\logs[39m, [38;5;117m\\?\UNC\server\share\logs[39m, [38;5;117m\\.\pipe\codex[39m, [38;5;117m\??\C:\logs[39m, \DosDevices\C:\logs, \rooted. [38;5;117mhttp://localhost:8080[39m: [38;5;117mhttps://example.com[39m; [38;5;117mwss://example.com[39m) and [38;5;117m/tmp/my data/logs_2.sqlite[39m + [38;5;214m⚠[39m state [38;5;117m/tmp/logs_2.sqlite[39m, [38;5;117m~/goals_1.sqlite[39m integrity check; try [38;5;117m--summary[39m or [38;5;117mcodex doctor[39m + [38;5;240mlocations [39m see [38;5;117m./logs[39m, [38;5;117m../goals[39m. [38;5;117mC:\logs[39m, [38;5;117mD:/goals[39m. [38;5;117mc:\logs[39m, [38;5;117m\\server\share\logs[39m, [38;5;117m\\?\C:\logs[39m, [38;5;117m\\?\UNC\server\share\logs[39m, [38;5;117m\\.\pipe\codex[39m, [38;5;117m\??\C:\logs[39m, \DosDevices\C:\logs, \rooted. [38;5;117mhttp://localhost:8080[39m: [38;5;117mhttps://example.com[39m; [38;5;117mwss://example.com[39m) and [38;5;117m/tmp/my data/logs_2.sqlite[39m + [38;5;196m✗[39m state [38;5;196m/tmp/logs_2.sqlite[39m, [38;5;196m~/goals_1.sqlite[39m integrity check; try [38;5;117m--summary[39m or [38;5;117mcodex doctor[39m + [38;5;240mlocations [39m see [38;5;196m./logs[39m, [38;5;196m../goals[39m. [38;5;196mC:\logs[39m, [38;5;196mD:/goals[39m. [38;5;196mc:\logs[39m, [38;5;196m\\server\share\logs[39m, [38;5;196m\\?\C:\logs[39m, [38;5;196m\\?\UNC\server\share\logs[39m, [38;5;196m\\.\pipe\codex[39m, [38;5;196m\??\C:\logs[39m, \DosDevices\C:\logs, \rooted. [38;5;196mhttp://localhost:8080[39m: [38;5;196mhttps://example.com[39m; [38;5;196mwss://example.com[39m) and [38;5;196m/tmp/my data/logs_2.sqlite[39m