Fix byte truncation to preserve command output tails #6476
Conversation
When commands produce few lines but very long lines (like cargo build), the previous byte truncation logic only preserved the head, completely hiding the tail. This caused important error messages at the end of output to be lost. Changes: - Modified truncate_formatted_exec_output() to detect when byte truncation is needed upfront - When byte truncation occurs, split bytes evenly between head and tail (5KB each) instead of using line-based slicing - This ensures error messages at the end (like cargo errors) are visible - Line-based truncation still works for outputs with many short lines Added comprehensive tests: - test_byte_truncation_preserves_tail: Verifies tail preservation with cargo-like output (few lines, very long) - test_line_truncation_still_works: Ensures line truncation unchanged - test_no_truncation_needed: Validates no truncation for short output Fixes openai#6415 Signed-off-by: 0xRaduan <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Thanks for the contribution. Before I assign this to a team member for review, please fix the failing tests and respond to the automated code review feedback. |
- Modified truncate_formatted_exec_output() to apply line truncation first when both limits exceeded - Account for 3-line marker overhead when calculating head/tail line budgets to ensure output never exceeds 256 lines - Removed unused MODEL_FORMAT_HEAD_LINES and MODEL_FORMAT_TAIL_LINES constants - Updated test expectations for new omitted line counts (147 vs 144 for 400 lines) - Added regression test for scenario where 6000 tiny lines (12KB) previously violated line limit 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
- Fixed format_exec_output_reports_omitted_lines_and_keeps_head_and_tail to account for 3-line marker overhead - Fixed format_exec_output_prefers_line_marker_when_both_limits_exceeded to use correct omitted count - Updated assertions to match new behavior where available content lines = 256 - 3 = 253 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Updated head lines from 1-128 to 1-126 (126 lines) - Updated tail lines from 273-400 to 274-400 (127 lines) - Updated omitted count from 144 to 147 (400 - 253 = 147) - Accounts for 3-line marker overhead in line truncation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
0xRaduan
force-pushed
the
claude/fix-byte-truncation-tail-011CUzauyoZNLSqRm4q2FQdd
branch
from
c761219 to
d014ece
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@etraut-openai - fixed CI + the Codex comment, please feel free to assign for a review |
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
does it make sense to move to codex-rs/core/tests/suite/truncation.rs?
2 participants
Problem
The original truncation logic was line-based only. When commands produced few lines with very long content (like
cargo buildwith long paths), the tail containing critical error messages was completely lost.Additionally, the initial fix introduced a regression where the byte-truncation path didn't enforce line limits. When output exceeded both limits (10KB bytes AND 256 lines), only the byte limit was enforced, allowing thousands of lines to slip through.
Summary
BEFORE PR: Line-based truncation only. Loses tails when lines are long.
AFTER PR: Byte-based head+tail truncation with line limit enforcement. Preserves tails AND respects line limits.
The PR ensures critical error messages at the end of command output (like cargo errors) are always visible to the model, while maintaining the 256-line cap for context efficiency.
Fixes #6415
Changes
Original commit (b38bdae):
truncate_formatted_exec_output()to detect when byte truncation is needed upfrontFollow-up fix (661d022):
truncate_formatted_exec_output()to apply line truncation first when both limits exceededMODEL_FORMAT_HEAD_LINESandMODEL_FORMAT_TAIL_LINESconstantsBehavior Change: BEFORE vs AFTER
Scenario 1: Few lines, very long content (e.g., cargo build with long paths)
Input: 5 lines, each ~5KB (total 25KB)
BEFORE PR:
AFTER PR:
Scenario 2: Many lines, tiny content (e.g., printing 1-6000)
Input: 6,000 lines of
"a\n"(2 bytes each = 12KB total)BEFORE PR:
AFTER PR: