fix(tui): limit user shell output by screen lines (#7448)
What - Limit the TUI "user shell" output panel by the number of visible screen lines rather than by the number of logical lines. - Apply middle truncation after wrapping, so a few extremely long lines cannot expand into hundreds of visible lines. - Add a regression test to guard this behavior. Why When the `ExecCommandSource::UserShell` tool returns a small number of very long logical lines, the TUI wraps those lines into many visual lines. The existing truncation logic applied `USER_SHELL_TOOL_CALL_MAX_LINES` to the number of logical lines *before* wrapping. As a result, a command like: - `Ran bash -lc "grep -R --line-number 'maskAssetId' ."` or a synthetic command that prints a single ~50,000‑character line, can produce hundreds of screen lines and effectively flood the viewport. The intended middle truncation for user shell output does not take effect in this scenario. How - In `codex-rs/tui/src/exec_cell/render.rs`, change the `ExecCell` rendering path for `ExecCommandSource::UserShell` so that: - Each logical line from `CommandOutput::aggregated_output` is first wrapped via `word_wrap_line` into multiple screen lines using the appropriate `RtOptions` and width from the `EXEC_DISPLAY_LAYOUT` configuration. - `truncate_lines_middle` is then applied to the wrapped screen lines, with `USER_SHELL_TOOL_CALL_MAX_LINES` as the limit. This means the limit is enforced on visible screen lines, not logical lines. - The existing layout struct (`ExecDisplayLayout`) continues to provide `output_max_lines`, so user shell output is subject to both `USER_SHELL_TOOL_CALL_MAX_LINES` and the layout-specific `output_max_lines` constraint. - Keep using `USER_SHELL_TOOL_CALL_MAX_LINES` as the cap, but interpret it as a per‑tool‑call limit on screen lines. - Add a regression test `user_shell_output_is_limited_by_screen_lines` in `codex-rs/tui/src/exec_cell/render.rs` that: - Constructs two extremely long logical lines containing a short marker (`"Z"`), so each wrapped screen line still contains the marker. - Wraps them at a narrow width to generate many screen lines. - Asserts that the unbounded wrapped output would exceed `USER_SHELL_TOOL_CALL_MAX_LINES` screen lines. - Renders an `ExecCell` for `ExecCommandSource::UserShell` at the same width and counts rendered lines containing the marker. - Asserts `output_screen_lines <= USER_SHELL_TOOL_CALL_MAX_LINES`, guarding against regressions where truncation happens before wrapping. This change keeps user shell output readable while ensuring it cannot flood the TUI, even when the tool emits a few extremely long lines. Tests - `cargo test -p codex-tui` Issue - Fixes #7447
This commit is contained in:
parent
71504325d3
commit
3395ebd96e
1 changed files with 106 additions and 10 deletions
|
|
@ -451,26 +451,26 @@ impl ExecCell {
|
|||
));
|
||||
}
|
||||
} else {
|
||||
let trimmed_output = Self::truncate_lines_middle(
|
||||
&raw_output.lines,
|
||||
display_limit,
|
||||
raw_output.omitted,
|
||||
);
|
||||
|
||||
// Wrap first so that truncation is applied to on-screen lines
|
||||
// rather than logical lines. This ensures that a small number
|
||||
// of very long lines cannot flood the viewport.
|
||||
let mut wrapped_output: Vec<Line<'static>> = Vec::new();
|
||||
let output_wrap_width = layout.output_block.wrap_width(width);
|
||||
let output_opts =
|
||||
RtOptions::new(output_wrap_width).word_splitter(WordSplitter::NoHyphenation);
|
||||
for line in trimmed_output {
|
||||
for line in &raw_output.lines {
|
||||
push_owned_lines(
|
||||
&word_wrap_line(&line, output_opts.clone()),
|
||||
&word_wrap_line(line, output_opts.clone()),
|
||||
&mut wrapped_output,
|
||||
);
|
||||
}
|
||||
|
||||
if !wrapped_output.is_empty() {
|
||||
let trimmed_output =
|
||||
Self::truncate_lines_middle(&wrapped_output, display_limit, raw_output.omitted);
|
||||
|
||||
if !trimmed_output.is_empty() {
|
||||
lines.extend(prefix_lines(
|
||||
wrapped_output,
|
||||
trimmed_output,
|
||||
Span::from(layout.output_block.initial_prefix).dim(),
|
||||
Span::from(layout.output_block.subsequent_prefix),
|
||||
));
|
||||
|
|
@ -597,3 +597,99 @@ const EXEC_DISPLAY_LAYOUT: ExecDisplayLayout = ExecDisplayLayout::new(
|
|||
PrefixedBlock::new(" └ ", " "),
|
||||
5,
|
||||
);
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use codex_core::protocol::ExecCommandSource;
|
||||
|
||||
#[test]
|
||||
fn user_shell_output_is_limited_by_screen_lines() {
|
||||
// Construct a user shell exec cell whose aggregated output consists of a
|
||||
// small number of very long logical lines. These will wrap into many
|
||||
// on-screen lines at narrow widths.
|
||||
//
|
||||
// Use a short marker so it survives wrapping intact inside each
|
||||
// rendered screen line; the previous test used a marker longer than
|
||||
// the wrap width, so it was split across lines and the assertion
|
||||
// never actually saw it.
|
||||
let marker = "Z";
|
||||
let long_chunk = marker.repeat(800);
|
||||
let aggregated_output = format!("{long_chunk}\n{long_chunk}\n");
|
||||
|
||||
// Baseline: how many screen lines would we get if we simply wrapped
|
||||
// all logical lines without any truncation?
|
||||
let output = CommandOutput {
|
||||
exit_code: 0,
|
||||
aggregated_output,
|
||||
formatted_output: String::new(),
|
||||
};
|
||||
let width = 20;
|
||||
let layout = EXEC_DISPLAY_LAYOUT;
|
||||
let raw_output = output_lines(
|
||||
Some(&output),
|
||||
OutputLinesParams {
|
||||
// Large enough to include all logical lines without
|
||||
// triggering the ellipsis in `output_lines`.
|
||||
line_limit: 100,
|
||||
only_err: false,
|
||||
include_angle_pipe: false,
|
||||
include_prefix: false,
|
||||
},
|
||||
);
|
||||
let output_wrap_width = layout.output_block.wrap_width(width);
|
||||
let output_opts =
|
||||
RtOptions::new(output_wrap_width).word_splitter(WordSplitter::NoHyphenation);
|
||||
let mut full_wrapped_output: Vec<Line<'static>> = Vec::new();
|
||||
for line in &raw_output.lines {
|
||||
push_owned_lines(
|
||||
&word_wrap_line(line, output_opts.clone()),
|
||||
&mut full_wrapped_output,
|
||||
);
|
||||
}
|
||||
let full_screen_lines = full_wrapped_output
|
||||
.iter()
|
||||
.filter(|line| line.spans.iter().any(|span| span.content.contains(marker)))
|
||||
.count();
|
||||
|
||||
// Sanity check: this scenario should produce more screen lines than
|
||||
// the user shell per-call limit when no truncation is applied. If
|
||||
// this ever fails, the test no longer exercises the regression.
|
||||
assert!(
|
||||
full_screen_lines > USER_SHELL_TOOL_CALL_MAX_LINES,
|
||||
"expected unbounded wrapping to produce more than {USER_SHELL_TOOL_CALL_MAX_LINES} screen lines, got {full_screen_lines}",
|
||||
);
|
||||
|
||||
let call = ExecCall {
|
||||
call_id: "call-id".to_string(),
|
||||
command: vec!["bash".into(), "-lc".into(), "echo long".into()],
|
||||
parsed: Vec::new(),
|
||||
output: Some(output),
|
||||
source: ExecCommandSource::UserShell,
|
||||
start_time: None,
|
||||
duration: None,
|
||||
interaction_input: None,
|
||||
};
|
||||
|
||||
let cell = ExecCell::new(call, false);
|
||||
|
||||
// Use a narrow width so each logical line wraps into many on-screen lines.
|
||||
let lines = cell.command_display_lines(width);
|
||||
|
||||
// Count how many rendered lines contain our marker text. This approximates
|
||||
// the number of visible output "screen lines" for this command.
|
||||
let output_screen_lines = lines
|
||||
.iter()
|
||||
.filter(|line| line.spans.iter().any(|span| span.content.contains(marker)))
|
||||
.count();
|
||||
|
||||
// Regression guard: previously this scenario could render hundreds of
|
||||
// wrapped lines because truncation happened before wrapping. Now the
|
||||
// truncation is applied after wrapping, so the number of visible
|
||||
// screen lines is bounded by USER_SHELL_TOOL_CALL_MAX_LINES.
|
||||
assert!(
|
||||
output_screen_lines <= USER_SHELL_TOOL_CALL_MAX_LINES,
|
||||
"expected at most {USER_SHELL_TOOL_CALL_MAX_LINES} screen lines of user shell output, got {output_screen_lines}",
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue