fix(progress): cap text-mode rendered line length#88
Conversation
A user-supplied template can produce arbitrarily large content — e.g. a multi-line shell script embedded in a `check =` value, or a generated command with hundreds of args expanded inline — and text mode prints each render verbatim. With no cap, every prop update on such a job dumps thousands of chars into CI logs. Cap printable width at 4096 chars by default, ANSI-aware so escapes don't get split or counted toward the budget. Override with `CLX_TEXT_MAX_LEN`; set to 0 to disable truncation entirely.
Greptile SummaryThis PR caps text-mode rendered line length at 4096 printable characters (ANSI-aware via Confidence Score: 3/5Safe to merge after fixing the doc/behavior mismatch for unparseable CLX_TEXT_MAX_LEN values. One P1 finding: the documented behavior for unparseable env-var values contradicts the code, which could mislead users into thinking truncation is disabled when it is not. The truncation logic itself is correct and the four new tests are thorough. Score sits at 3 due to the P1 ceiling. src/progress/render.rs — specifically the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[render_text_mode called] --> B{output empty?}
B -- yes --> Z[return Ok]
B -- no --> C{contains clx:flex?}
C -- yes --> D[flex: truncate to terminal width]
C -- no --> E[use output as-is]
D --> F[truncate_text_mode_line]
E --> F
F --> G{max_len == 0?}
G -- yes --> H[return unchanged]
G -- no --> I{measure_text_width <= max_len?}
I -- yes --> H
I -- no --> J[console::truncate_str + ellipsis]
J --> K[final_output]
H --> K
K --> L{same as last output?}
L -- yes --> Z
L -- no --> M[write_line to terminal]
M --> Z
subgraph text_mode_max_len
N[read CLX_TEXT_MAX_LEN env var] --> O{parseable usize?}
O -- yes --> P[use parsed value]
O -- no / unset --> Q[use DEFAULT 4096]
end
Reviews (1): Last reviewed commit: "fix(progress): cap text-mode rendered li..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// `CLX_TEXT_MAX_LEN=0` (or any unparseable value) disables truncation. | ||
| fn text_mode_max_len() -> usize { | ||
| static CACHED: std::sync::OnceLock<usize> = std::sync::OnceLock::new(); | ||
| *CACHED.get_or_init(|| { | ||
| std::env::var("CLX_TEXT_MAX_LEN") | ||
| .ok() | ||
| .and_then(|s| s.parse::<usize>().ok()) | ||
| .unwrap_or(DEFAULT_TEXT_MODE_MAX_LEN) | ||
| }) |
There was a problem hiding this comment.
The doc comment says CLX_TEXT_MAX_LEN=0 or any unparseable value disables truncation, but unwrap_or(DEFAULT_TEXT_MODE_MAX_LEN) means an unparseable value (e.g. CLX_TEXT_MAX_LEN=off) silently falls back to the default 4096 cap instead. A user who sets CLX_TEXT_MAX_LEN=none or misspells the number would expect truncation disabled but would get the default cap applied with no warning.
Either fix the comment to match the code, or change the fallback so unparseable strings map to 0 (no-truncation) as documented.
| /// `CLX_TEXT_MAX_LEN=0` (or any unparseable value) disables truncation. | |
| fn text_mode_max_len() -> usize { | |
| static CACHED: std::sync::OnceLock<usize> = std::sync::OnceLock::new(); | |
| *CACHED.get_or_init(|| { | |
| std::env::var("CLX_TEXT_MAX_LEN") | |
| .ok() | |
| .and_then(|s| s.parse::<usize>().ok()) | |
| .unwrap_or(DEFAULT_TEXT_MODE_MAX_LEN) | |
| }) | |
| /// `CLX_TEXT_MAX_LEN=0` disables truncation. Any unparseable value falls back | |
| /// to the default (`DEFAULT_TEXT_MODE_MAX_LEN`). | |
| fn text_mode_max_len() -> usize { | |
| static CACHED: std::sync::OnceLock<usize> = std::sync::OnceLock::new(); | |
| *CACHED.get_or_init(|| { | |
| std::env::var("CLX_TEXT_MAX_LEN") | |
| .ok() | |
| .and_then(|s| s.parse::<usize>().ok()) | |
| .unwrap_or(DEFAULT_TEXT_MODE_MAX_LEN) | |
| }) | |
| } |
| fn truncate_text_mode_line(s: &str, max_len: usize) -> String { | ||
| if max_len == 0 || console::measure_text_width(s) <= max_len { | ||
| return s.to_string(); | ||
| } | ||
| console::truncate_str(s, max_len, "…").into_owned() | ||
| } |
There was a problem hiding this comment.
truncate_text_mode_line calls console::measure_text_width(s) to decide whether to truncate, and then console::truncate_str internally re-scans the string to find the cut point. For the common case (no truncation needed) this is just one extra pass, but for very long lines with many ANSI codes both passes traverse the entire string. This is a minor performance observation — not blocking.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to truncate text-mode output in src/progress/render.rs to prevent excessively large logs from runaway templates. It includes a configurable cap via environment variables, a truncation helper that handles ANSI escapes, and corresponding unit tests. Feedback highlights a discrepancy between the documentation and the implementation regarding unparseable environment variables, as well as a redundant text width measurement in the truncation logic.
| const DEFAULT_TEXT_MODE_MAX_LEN: usize = 4096; | ||
|
|
||
| /// Resolve the text-mode line cap from the env, falling back to the default. | ||
| /// `CLX_TEXT_MAX_LEN=0` (or any unparseable value) disables truncation. |
There was a problem hiding this comment.
The docstring states that any unparseable value disables truncation, but the implementation falls back to DEFAULT_TEXT_MODE_MAX_LEN (4096) if parsing fails (via unwrap_or on line 304). To align the code with the documentation, you could change the fallback to 0, or update the docstring to reflect that invalid values fall back to the default cap. Given that this is a safety feature, updating the docstring is likely the safer choice.
| if max_len == 0 || console::measure_text_width(s) <= max_len { | ||
| return s.to_string(); | ||
| } | ||
| console::truncate_str(s, max_len, "…").into_owned() |
There was a problem hiding this comment.
The call to console::measure_text_width(s) is redundant because console::truncate_str already performs this check internally. Removing it avoids an unnecessary extra pass over the string, which is beneficial when handling the large "pathological" inputs this PR aims to address.
| if max_len == 0 || console::measure_text_width(s) <= max_len { | |
| return s.to_string(); | |
| } | |
| console::truncate_str(s, max_len, "…").into_owned() | |
| if max_len == 0 { | |
| return s.to_string(); | |
| } | |
| console::truncate_str(s, max_len, "…").into_owned() |
1 participant
Summary
A user-supplied template can produce arbitrarily large content — e.g. a multi-line shell script embedded in a `check =` value, or a generated command with hundreds of args expanded inline — and text mode prints each render verbatim. With no cap, every prop update on such a job dumps thousands of chars into CI logs.
This cap was the missing piece after #86 — that PR collapsed duplicate lines and dropped UI escape codes from text mode, but the per-line size was still unbounded.
Behavior
Test plan
🤖 Generated with Claude Code
Note
Low Risk
Low risk change isolated to
ProgressOutput::Textrendering; main impact is potentially truncating very long template output (configurable/disableable viaCLX_TEXT_MAX_LEN).Overview
Adds a default 4096-character cap to rendered
ProgressOutput::Textlines to prevent user templates from dumping unbounded content into logs.Introduces ANSI-aware truncation with an ellipsis suffix via
truncate_text_mode_line, caches an optionalCLX_TEXT_MAX_LENoverride (set to0to disable), and applies the cap before the existing "skip duplicate line" check. Includes unit tests covering short/long input, the0escape hatch, and ANSI handling.Reviewed by Cursor Bugbot for commit a2a2444. Bugbot is set up for automated code reviews on this repo. Configure here.