mirror of
https://github.com/hanzoai/dev.git
synced 2026-10-25 03:32:32 +00:00
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:
@@ -451,26 +451,26 @@ impl ExecCell {
|
|||||||
));
|
));
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
let trimmed_output = Self::truncate_lines_middle(
|
// Wrap first so that truncation is applied to on-screen lines
|
||||||
&raw_output.lines,
|
// rather than logical lines. This ensures that a small number
|
||||||
display_limit,
|
// of very long lines cannot flood the viewport.
|
||||||
raw_output.omitted,
|
|
||||||
);
|
|
||||||
|
|
||||||
let mut wrapped_output: Vec<Line<'static>> = Vec::new();
|
let mut wrapped_output: Vec<Line<'static>> = Vec::new();
|
||||||
let output_wrap_width = layout.output_block.wrap_width(width);
|
let output_wrap_width = layout.output_block.wrap_width(width);
|
||||||
let output_opts =
|
let output_opts =
|
||||||
RtOptions::new(output_wrap_width).word_splitter(WordSplitter::NoHyphenation);
|
RtOptions::new(output_wrap_width).word_splitter(WordSplitter::NoHyphenation);
|
||||||
for line in trimmed_output {
|
for line in &raw_output.lines {
|
||||||
push_owned_lines(
|
push_owned_lines(
|
||||||
&word_wrap_line(&line, output_opts.clone()),
|
&word_wrap_line(line, output_opts.clone()),
|
||||||
&mut wrapped_output,
|
&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(
|
lines.extend(prefix_lines(
|
||||||
wrapped_output,
|
trimmed_output,
|
||||||
Span::from(layout.output_block.initial_prefix).dim(),
|
Span::from(layout.output_block.initial_prefix).dim(),
|
||||||
Span::from(layout.output_block.subsequent_prefix),
|
Span::from(layout.output_block.subsequent_prefix),
|
||||||
));
|
));
|
||||||
@@ -597,3 +597,99 @@ const EXEC_DISPLAY_LAYOUT: ExecDisplayLayout = ExecDisplayLayout::new(
|
|||||||
PrefixedBlock::new(" └ ", " "),
|
PrefixedBlock::new(" └ ", " "),
|
||||||
5,
|
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}",
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user