Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion codex-rs/tui/src/bottom_pane/approval_overlay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::bottom_pane::list_selection_view::ListSelectionView;
use crate::bottom_pane::list_selection_view::SelectionItem;
use crate::bottom_pane::list_selection_view::SelectionViewParams;
use crate::diff_render::DiffSummary;
use crate::exec_command::render_for_approval_prefix_label;
use crate::exec_command::strip_bash_lc_and_escape;
use crate::history_cell;
use crate::key_hint;
Expand Down Expand Up @@ -462,7 +463,7 @@ fn exec_options(
proposed_execpolicy_amendment
.filter(|_| features.enabled(Feature::ExecPolicy))
.map(|prefix| {
let rendered_prefix = strip_bash_lc_and_escape(prefix.command());
let rendered_prefix = render_for_approval_prefix_label(prefix.command());
ApprovalOption {
label: format!(
"Yes, and don't ask again for commands that start with `{rendered_prefix}`"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
source: tui/src/chatwidget/tests.rs
expression: contents
---
Would you like to run the following command?

$ echo hello world

› 1. Yes, proceed (y)
2. Yes, and don't ask again for commands that start with `python - <<'PY'
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx... (truncated)` (p)
3. No, and tell Codex what to do differently (esc)

Press enter to confirm or esc to cancel
41 changes: 41 additions & 0 deletions codex-rs/tui/src/chatwidget/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2465,6 +2465,47 @@ async fn approval_modal_exec_without_reason_snapshot() -> anyhow::Result<()> {
Ok(())
}

// Snapshot test: approval modal with a proposed execpolicy prefix that is long enough
// to trigger truncation in the selection label.
#[tokio::test]
async fn approval_modal_exec_multiline_prefix_truncates_snapshot() -> anyhow::Result<()> {
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await;
chat.config.approval_policy.set(AskForApproval::OnRequest)?;

let long = format!("python - <<'PY'\n{}\nPY\n", "x".repeat(500));
let ev = ExecApprovalRequestEvent {
call_id: "call-approve-cmd-multiline-trunc".into(),
turn_id: "turn-approve-cmd-multiline-trunc".into(),
command: vec!["bash".into(), "-lc".into(), "echo hello world".into()],
cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")),
reason: None,
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![
"bash".into(),
"-lc".into(),
long,
])),
parsed_cmd: vec![],
};
chat.handle_codex_event(Event {
id: "sub-approve-multiline-trunc".into(),
msg: EventMsg::ExecApprovalRequest(ev),
});

let width = 100;
let height = chat.desired_height(width);
let mut terminal =
ratatui::Terminal::new(VT100Backend::new(width, height)).expect("create terminal");
terminal.set_viewport_area(Rect::new(0, 0, width, height));
terminal
.draw(|f| chat.render(f.area(), f.buffer_mut()))
.expect("draw approval modal (multiline prefix truncation)");
let contents = terminal.backend().vt100().screen().contents();
assert!(contents.contains("(truncated)"));
assert_snapshot!("approval_modal_exec_multiline_prefix_truncates", contents);

Ok(())
}

// Snapshot test: patch approval modal
#[tokio::test]
async fn approval_modal_patch_snapshot() -> anyhow::Result<()> {
Expand Down
27 changes: 27 additions & 0 deletions codex-rs/tui/src/exec_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ use codex_core::parse_command::extract_shell_command;
use dirs::home_dir;
use shlex::try_join;

const APPROVAL_LABEL_MAX_LEN: usize = 80;
const APPROVAL_LABEL_TRUNCATED_SUFFIX: &str = "... (truncated)";

pub(crate) fn escape_command(command: &[String]) -> String {
try_join(command.iter().map(String::as_str)).unwrap_or_else(|_| command.join(" "))
}
Expand All @@ -16,6 +19,21 @@ pub(crate) fn strip_bash_lc_and_escape(command: &[String]) -> String {
escape_command(command)
}

pub(crate) fn render_for_approval_prefix_label(command: &[String]) -> String {
let rendered = strip_bash_lc_and_escape(command);

// Approval choices are rendered inline in a list; ensure we never introduce
// multi-line labels due to heredocs or embedded newlines.
let collapsed = rendered.split_whitespace().collect::<Vec<_>>().join(" ");
let mut chars = collapsed.chars();
let prefix: String = chars.by_ref().take(APPROVAL_LABEL_MAX_LEN).collect();
if chars.next().is_some() {
format!("{}{APPROVAL_LABEL_TRUNCATED_SUFFIX}", prefix.trim_end())
} else {
prefix
}
}

/// If `path` is absolute and inside $HOME, return the part *after* the home
/// directory; otherwise, return the path as-is. Note if `path` is the homedir,
/// this will return and empty path.
Expand Down Expand Up @@ -67,4 +85,13 @@ mod tests {
let cmdline = strip_bash_lc_and_escape(&args);
assert_eq!(cmdline, "echo hello");
}

#[test]
fn approval_prefix_label_is_single_line_and_truncated() {
let long = format!("python - <<'PY'\n{}\nPY\n", "x".repeat(500));
let args = vec!["bash".into(), "-lc".into(), long];
let label = render_for_approval_prefix_label(&args);
assert!(!label.contains('\n'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I would just do the work to derive expected_label and then assert_eq!(expected_label, label) because then the failure of the assertion has a stronger signal and the difference should be pretty clear when the failure is printed.

That said, this is pretty low-risk, so you can also keep it as-is.

assert!(label.ends_with(APPROVAL_LABEL_TRUNCATED_SUFFIX));
}
}
3 changes: 2 additions & 1 deletion codex-rs/tui2/src/bottom_pane/approval_overlay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::bottom_pane::list_selection_view::ListSelectionView;
use crate::bottom_pane::list_selection_view::SelectionItem;
use crate::bottom_pane::list_selection_view::SelectionViewParams;
use crate::diff_render::DiffSummary;
use crate::exec_command::render_for_approval_prefix_label;
use crate::exec_command::strip_bash_lc_and_escape;
use crate::history_cell;
use crate::key_hint;
Expand Down Expand Up @@ -462,7 +463,7 @@ fn exec_options(
proposed_execpolicy_amendment
.filter(|_| features.enabled(Feature::ExecPolicy))
.map(|prefix| {
let rendered_prefix = strip_bash_lc_and_escape(prefix.command());
let rendered_prefix = render_for_approval_prefix_label(prefix.command());
ApprovalOption {
label: format!(
"Yes, and don't ask again for commands that start with `{rendered_prefix}`"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
source: tui2/src/chatwidget/tests.rs
expression: contents
---
Would you like to run the following command?

$ echo hello world

› 1. Yes, proceed (y)
2. Yes, and don't ask again for commands that start with `python - <<'PY'
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx... (truncated)` (p)
3. No, and tell Codex what to do differently (esc)

Press enter to confirm or esc to cancel
39 changes: 39 additions & 0 deletions codex-rs/tui2/src/chatwidget/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2123,6 +2123,45 @@ async fn approval_modal_exec_without_reason_snapshot() {
);
}

// Snapshot test: approval modal with a proposed execpolicy prefix that is long enough
// to trigger truncation in the selection label.
#[tokio::test]
async fn approval_modal_exec_multiline_prefix_truncates_snapshot() {
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await;
chat.config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);

let long = format!("python - <<'PY'\n{}\nPY\n", "x".repeat(500));
let ev = ExecApprovalRequestEvent {
call_id: "call-approve-cmd-multiline-trunc".into(),
turn_id: "turn-approve-cmd-multiline-trunc".into(),
command: vec!["bash".into(), "-lc".into(), "echo hello world".into()],
cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")),
reason: None,
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![
"bash".into(),
"-lc".into(),
long,
])),
parsed_cmd: vec![],
};
chat.handle_codex_event(Event {
id: "sub-approve-multiline-trunc".into(),
msg: EventMsg::ExecApprovalRequest(ev),
});

let width = 100;
let height = chat.desired_height(width);
let mut terminal =
ratatui::Terminal::new(VT100Backend::new(width, height)).expect("create terminal");
terminal.set_viewport_area(Rect::new(0, 0, width, height));
terminal
.draw(|f| chat.render(f.area(), f.buffer_mut()))
.expect("draw approval modal (multiline prefix truncation)");
let contents = terminal.backend().vt100().screen().contents();
assert!(contents.contains("(truncated)"));
assert_snapshot!("approval_modal_exec_multiline_prefix_truncates", contents);
}

// Snapshot test: patch approval modal
#[tokio::test]
async fn approval_modal_patch_snapshot() {
Expand Down
27 changes: 27 additions & 0 deletions codex-rs/tui2/src/exec_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ use codex_core::parse_command::extract_shell_command;
use dirs::home_dir;
use shlex::try_join;

const APPROVAL_LABEL_MAX_LEN: usize = 80;
const APPROVAL_LABEL_TRUNCATED_SUFFIX: &str = "... (truncated)";

pub(crate) fn escape_command(command: &[String]) -> String {
try_join(command.iter().map(String::as_str)).unwrap_or_else(|_| command.join(" "))
}
Expand All @@ -16,6 +19,21 @@ pub(crate) fn strip_bash_lc_and_escape(command: &[String]) -> String {
escape_command(command)
}

pub(crate) fn render_for_approval_prefix_label(command: &[String]) -> String {
let rendered = strip_bash_lc_and_escape(command);

// Approval choices are rendered inline in a list; ensure we never introduce
// multi-line labels due to heredocs or embedded newlines.
let collapsed = rendered.split_whitespace().collect::<Vec<_>>().join(" ");
let mut chars = collapsed.chars();
let prefix: String = chars.by_ref().take(APPROVAL_LABEL_MAX_LEN).collect();
if chars.next().is_some() {
format!("{}{APPROVAL_LABEL_TRUNCATED_SUFFIX}", prefix.trim_end())
} else {
prefix
}
}

/// If `path` is absolute and inside $HOME, return the part *after* the home
/// directory; otherwise, return the path as-is. Note if `path` is the homedir,
/// this will return and empty path.
Expand Down Expand Up @@ -67,4 +85,13 @@ mod tests {
let cmdline = strip_bash_lc_and_escape(&args);
assert_eq!(cmdline, "echo hello");
}

#[test]
fn approval_prefix_label_is_single_line_and_truncated() {
let long = format!("python - <<'PY'\n{}\nPY\n", "x".repeat(500));
let args = vec!["bash".into(), "-lc".into(), long];
let label = render_for_approval_prefix_label(&args);
assert!(!label.contains('\n'));
assert!(label.ends_with(APPROVAL_LABEL_TRUNCATED_SUFFIX));
}
}
Loading