Conversation
|
Updated 4:26 AM PT - May 30th, 2026
❌ @robobun, your commit a3a7bb2 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 28978That installs a local version of the PR into your bun-28978 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded support for configuring the maximum number of lines of script output shown when using Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/Arguments.zig`:
- Around line 477-479: The code duplicates environment-variable fallback logic
for BUN_CONFIG_ELIDE_LINES in two places; consolidate by extracting a single
helper (e.g., getElideLinesFromEnv or parseElideLines) and call it from both
parsing locations instead of repeating the logic. Locate and update usages that
set ctx.bundler_options.elide_lines and the CLI `--elide-lines` parsing to read
from that helper so the env var handling
(bun.env_var.BUN_CONFIG_ELIDE_LINES.get()) and int casting (`@intCast`) are
implemented once and returned/assigned consistently.
- Line 478: The assignment using `@intCast` to set ctx.bundler_options.elide_lines
from BUN_CONFIG_ELIDE_LINES.get() can overflow on 32-bit platforms; change the
code to validate or clamp the u64 value before casting (e.g., compare against
maxInt(usize) or use `@min` to clamp) and then set ctx.bundler_options.elide_lines
to the safely-cast ?usize; update the site around
ctx.bundler_options.elide_lines and the call to BUN_CONFIG_ELIDE_LINES.get() to
perform this check/clamp instead of a direct `@intCast`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6deec515-7f94-491d-8513-32b5827d7558
📥 Commits
Reviewing files that changed from the base of the PR and between 49221ca and 2983c9dc04d9184487ff22570a1a8654996b847d.
📒 Files selected for processing (4)
docs/runtime/environment-variables.mdxsrc/cli/Arguments.zigsrc/env_var.zigtest/cli/run/filter-workspace.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@robobun adopt |
|
✅ Feature complete and 🟡 Build #59204: the only red lane is This PRs diff is 5 files — @alii — diff is green on every relevant lane; the single failure is a Windows-aarch64 IPC-timeout flake. Ready to merge (or re-run that one lane). |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/filter_run.zig`:
- Around line 559-563: The current check rejects any explicit --elide-lines
value in non-terminal output even when the value is 0 (which is a no-op). Change
the guard so that when not pretty_output you only fail for an explicit
CLI-supplied positive value: check ctx.bundler_options.elide_lines is non-null
AND ctx.bundler_options.elide_lines != 0 AND
ctx.bundler_options.elide_lines_from_cli_flag is true, and only then call
Output.prettyErrorln(...) and Global.exit(1); leave the path that allows
elide_lines == 0 to continue normally.
In `@test/cli/run/filter-workspace.test.ts`:
- Around line 570-653: Add a regression test that mirrors
"BUN_CONFIG_ELIDE_LINES is silently ignored in non-terminal environments" but
supplies the elide-lines setting via a bunfig.toml (run.elide-lines) instead of
the BUN_CONFIG_ELIDE_LINES env var; create a tempDirWithFiles workspace that
includes a bunfig.toml with run.elide-lines = 17, invoke spawnSync with env: {
...bunEnv } and stdout/stderr piped (same invocation as the existing test using
bunExe(), spawnSync, and bunEnv), and assert stderr does not contain the
"--elide-lines is only supported" message, stdout contains the log lines, and
exitCode is 0 so the bunfig-backed code path (run.elide-lines) is covered and
treated as a silent no-op in non-terminal environments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a637b11f-9be1-4a0d-9a47-a10c1704ec50
📥 Commits
Reviewing files that changed from the base of the PR and between 2983c9dc04d9184487ff22570a1a8654996b847d and 117da3084c5f3bc1273e329648155837558014ec.
📒 Files selected for processing (4)
src/cli.zigsrc/cli/Arguments.zigsrc/cli/filter_run.zigtest/cli/run/filter-workspace.test.ts
There was a problem hiding this comment.
All previously reported bugs have been addressed across the follow-up commits.
Extended reasoning...
Overview
The PR adds BUN_CONFIG_ELIDE_LINES as an env-var equivalent of --elide-lines, touching src/env_var.zig, src/cli.zig, src/cli/Arguments.zig, src/cli/filter_run.zig, src/bunfig.zig, docs, and tests.
Security risks
None — the new env var feeds into a usize display-count with no security surface.
Level of scrutiny
Medium. The changes are well-scoped feature additions to CLI argument parsing. All code paths have corresponding test coverage including the non-terminal/CI edge cases.
Other factors
I found several bugs in earlier rounds (missing CLI-origin flag, broken --elide-lines 0 in non-terminal, f64 boundary off-by-one in bunfig, Windows test branch mismatch). All were resolved in commits 117da30, e203746, and b333e0f/2cc006e. The diff and test file confirm the fixes are in place. No new bugs were found by the bug hunting system.
b333e0f to
1fdd3bb
Compare
There was a problem hiding this comment.
LGTM — all previously raised issues have been resolved.
Extended reasoning...
Overview
This PR adds BUN_CONFIG_ELIDE_LINES as an env-var equivalent of --elide-lines, wiring it through src/env_var.zig, src/cli/Arguments.zig, src/cli/filter_run.zig, src/bunfig.zig, docs, and tests. The scope is moderate: 7 files, all within the CLI/config layer.
Security risks
None. The feature only controls how many lines of script output are shown in terminal environments; it doesn't touch auth, crypto, or I/O paths.
Level of scrutiny
Medium. The logic is self-contained and the test coverage is thorough, including platform-specific branches (Windows non-terminal), env-var vs CLI precedence, and bunfig fallback. All the bugs I flagged across prior runs have been addressed.
Other factors
The one remaining nit (fractional floats in bunfig silently truncating) is filed as an inline comment on this review. It is cosmetic — values like 5.7 become 5 instead of emitting a parse error — and does not affect correctness for the common case.
761b247 to
ce72069
Compare
There was a problem hiding this comment.
LGTM — all previously raised issues have been resolved.
Extended reasoning...
Overview
This PR adds BUN_CONFIG_ELIDE_LINES as a global environment-variable equivalent of --elide-lines, touching src/env_var.zig, src/cli.zig, src/cli/Arguments.zig, src/cli/filter_run.zig, src/bunfig.zig, docs, and tests.
Security risks
No security-sensitive code is touched. The new env var is a non-privileged numeric setting, parsed safely via std.math.cast with a clamp rather than a raw @intCast.
Level of scrutiny
Moderate — this is a feature addition to CLI argument parsing and runtime behavior, not a sandbox or config-only change. The core logic change (distinguishing CLI-sourced values from env var / bunfig sources via elide_lines_from_cli_flag) has been iterated on multiple times.
Other factors
Every specific issue from prior review rounds has been addressed in the follow-up commits (fractional-value guard with value \!= @floor(value), upper-bound fix to >=, Windows test branch condition tightened to elideLines \!== undefined && elideLines > 0, bunfig test passing -c ./bunfig.toml, empty-string CLI flag falling through to env var, docs semantics corrected). The final code state is correct and the test coverage is thorough.
c64951f to
0e9c158
Compare
There was a problem hiding this comment.
LGTM — all previously raised issues have been addressed: the non-terminal no-op, the silent-ignore for non-CLI sources, the bunfig boundary and fractional-value rejection, the vacuous bunfig test, and the empty env-var fallback.
Extended reasoning...
Overview
Adds as an env-var equivalent of , wires up precedence (CLI > env var > bunfig), documents the new variable, and adds comprehensive tests. Touches , , , , , , and .
Security risks
None. No auth, crypto, or permission-sensitive code is involved. The new env var is a purely additive output-formatting option.
Level of scrutiny
Moderate. The change is self-contained within the CLI run/filter path, but it required several iterations to get the edge cases right (empty-string expansion, , bunfig float boundary, non-terminal CI behavior). All those edge cases are now covered by tests and have been verified against the current source.
Other factors
Five follow-up commits (, , , , ) addressed every issue raised in prior reviews (mine and CodeRabbit's). The bug-hunting system found no new issues. Test coverage is thorough: terminal vs. non-terminal, env-var vs. CLI precedence, empty flag, bunfig fractional rejection, and Windows-branch behavior.
|
@robobun rebase |
0e9c158 to
757c840
Compare
There was a problem hiding this comment.
LGTM — all previously identified bugs have been resolved.
Extended reasoning...
Overview
This PR adds BUN_CONFIG_ELIDE_LINES as an environment-variable equivalent of --elide-lines for bun run --filter, touching src/env_var.zig, src/cli/Arguments.zig, src/bunfig.zig, docs, and tests.
Security risks
None. The change reads a user-controlled env var and parses it as an unsigned integer with explicit bounds checking. No auth, crypto, or permission-sensitive paths are affected.
Level of scrutiny
The scope is narrow and self-contained: a new env var definition, a consolidated parse helper, one new bunfig validation block, a docs entry, and test additions. The logic changes are all in CLI/config parsing, not in any hot runtime path.
Other factors
Multiple rounds of review caught several real bugs (32-bit cast overflow, non-terminal error firing for env-var-sourced values, fractional bunfig values, empty-flag shadowing the env var, bounds-check off-by-one). All have been addressed in subsequent commits. The final diff reflects correct behavior: safe casting, proper float validation with value \!= @floor(value) and >= upper bound, and the empty-string fallback in parseElideLinesOption. Test coverage is thorough, including the Windows non-terminal path. No new bugs were flagged by the bug hunting system.
757c840 to
71ad395
Compare
There was a problem hiding this comment.
LGTM — all prior feedback was addressed: the non-terminal error guard was cleanly removed (making --elide-lines a graceful no-op when output is piped), bunfig.zig now validates fractional/negative/out-of-range values before @intFromFloat, the empty-string CLI flag correctly falls back to BUN_CONFIG_ELIDE_LINES, and 32-bit overflow is handled with std.math.cast.
Extended reasoning...
Overview
PR adds BUN_CONFIG_ELIDE_LINES as a global env-var equivalent of --elide-lines, touching src/env_var.zig, src/cli/Arguments.zig, src/bunfig.zig, docs, and tests. The CLI flag (or a non-empty expansion of it) takes precedence; an empty flag value falls back to the env var.
Prior bugs — all resolved
Multiple rounds of review produced five distinct bug reports from previous runs; every one is addressed in the final state:
- Non-terminal error on env-var path: The
--elide-lines is only supported in terminal environmentsguard infilter_run.zigwas removed entirely rather than gated by a_from_cli_flagboolean;--elide-linesis now a silent no-op whenpretty_outputis false (theredraw()early return handles it). --elide-lines 0erroring in CI: Subsumed by the guard removal above.- bunfig unsafe
@intFromFloat: The new guard checksvalue < 0,!isFinite,value != @floor(value), andvalue >= @floatFromInt(maxInt(usize))before converting. - 32-bit
@intCastpanic: Usesstd.math.cast(usize, value) orelse std.math.maxInt(usize). - Empty
--elide-lines ""shadowing env var:parseElideLinesOptionnormalises empty string tonullbefore theelse ifenv-var branch.
Security risks
None. No auth, crypto, or permission-sensitive code is touched.
Scrutiny level
Medium-complexity CLI/config change (5 files). All previously identified bugs were fixed and the test suite covers the new code paths. Appropriate to approve.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2bfd715 to
64c1881
Compare
There was a problem hiding this comment.
LGTM — all previously raised concerns (empty-flag env-var fallback, 32-bit cast safety, bunfig fractional/bounds validation, non-terminal guard for env-var path, Windows test expectations) have been addressed.
Extended reasoning...
Overview
This PR adds BUN_CONFIG_ELIDE_LINES as a global env-var alternative to --elide-lines when using bun run --filter. It touches src/env_var.zig (new env var definition), src/cli/Arguments.zig (refactored into parseElideLinesOption), src/bunfig.zig (validation for run.elide-lines), and tests/docs.
Security risks
None. This is a display-control feature with no auth, file system, or network implications.
Level of scrutiny
Medium — Zig integer-casting edge cases were a concern, but the implementation uses std.math.cast(usize, value) orelse std.math.maxInt(usize) and a thorough bunfig.zig guard (negative, non-finite, fractional, out-of-range with >= boundary). All edge cases identified in earlier review rounds were fixed.
Other factors
Three rounds of iterative fixes addressed every issue I raised: the env-var-in-non-terminal false-error, empty-string flag shadowing the env var, 32-bit overflow, fractional bunfig values, and the Windows test branch expecting the wrong outcome for --elide-lines 0. The bug hunting system found no new issues in the final state.
There was a problem hiding this comment.
LGTM — all prior feedback has been addressed and the feature is cleanly implemented.
Extended reasoning...
Overview
This PR adds BUN_CONFIG_ELIDE_LINES as a global env-var equivalent of --elide-lines across five files: docs, src/env_var.zig, src/cli/Arguments.zig, src/bunfig.zig, and the filter-workspace test.
Security risks
None. The new code reads a numeric env var and a TOML number; both paths are validated before any cast.
Level of scrutiny
Low. The change is additive and self-contained: a new env var declaration, a consolidated parse helper, TOML validation, documentation, and tests. No auth, crypto, or IPC paths are touched.
Other factors
All issues raised in previous review rounds have been resolved in subsequent commits: the terminal-only fatal error was removed (making --elide-lines and BUN_CONFIG_ELIDE_LINES silently no-ops in non-TTY contexts), the bunfig.zig bounds check uses >= and guards fractional values, the empty-string CLI fallback is handled correctly, and the Windows test branch was updated to match. No bugs were found by the bug-hunting system.
1ad2fe6 to
7839576
Compare
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the remaining inline note is a nit about test construction that doesn't affect correctness.
Extended reasoning...
Overview
This PR adds BUN_CONFIG_ELIDE_LINES as an env-var equivalent of --elide-lines for bun run --filter. It touches five files: a one-line declarative entry in src/bun_core/env_var.zig, a one-row docs table addition, a refactor in src/cli/Arguments.zig that consolidates two duplicate --elide-lines parsing blocks into a single parseElideLinesOption helper (with env-var fallback and safe std.math.cast clamping), input validation for run.elide-lines in src/cli/bunfig.zig (rejecting negative/non-finite/fractional/out-of-range floats before @intFromFloat), and four new tests in test/cli/run/filter-workspace.test.ts.
Security risks
None. This only affects how many lines of script output are displayed when running workspace scripts with --filter. No auth, crypto, network, file-permission, or untrusted-input handling is involved.
Level of scrutiny
Low-to-moderate. The PR went through several iterations and every substantive issue I (and CodeRabbit) raised earlier — the non-terminal error semantics, the misleading docs wording, the @intFromFloat panic on negative/oversized bunfig values, the missing fractional check, the > vs >= upper-bound off-by-one, the empty --elide-lines "" shadowing the env var, and the duplicated parsing logic — has been addressed and the inline comments are all resolved. The helper now runs after loadConfigWithCmdArgs, so CLI > env var > bunfig precedence is correct, and the bunfig guard is well-commented. CI build #46620 is green.
Other factors
The one remaining finding is a nit: the new fractional-rejection test passes -c and ./bunfig.toml as separate argv tokens, which (because -c is takes_value=.one_optional) doesn't consume the path and dispatches as AutoCommand rather than RunCommand. The test is not vacuous — the cwd auto-load still picks up bunfig.toml and the new validation in bunfig.zig fires exactly as asserted — only the in-test comment about the mechanism is inaccurate. That doesn't block merge.
|
@robobun rebase and get this mergable |
Split out from #18111 by @martinamps. Adds `BUN_CONFIG_ELIDE_LINES` as an environment-variable equivalent of `--elide-lines`, so the limit can be set once globally instead of per command. The CLI flag takes precedence when both are set. Closes #11465. Implemented against the Rust CLI (`src/runtime/cli/Arguments.rs`, `src/bun_core/env_var.rs`, `src/bunfig/bunfig.rs`) after the src port: - Consolidate the two duplicated `--elide-lines` parse blocks into a single `parse_elide_lines_option` helper, called after `load_config_with_cmd_args` so CLI + env var beat `run.elide-lines` in bunfig.toml. - Empty `--elide-lines ""` (e.g. `--elide-lines "$MAYBE_UNSET"`) falls through to the env var instead of shadowing it. - Env var value is clamped via `usize::try_from(...).unwrap_or(usize::MAX)` so 32-bit targets don't lose high bits above `usize::MAX`. - `bunfig.rs` rejects negative, non-finite, fractional, and out-of-range `run.elide-lines` numbers with `Expected a non-negative integer` instead of silently saturating/truncating via `as usize`. Tests cover CLI vs env-var precedence, the empty-string fallback, the bunfig fractional rejection, and the non-terminal no-op contract. The bunfig test uses `--config=./bunfig.toml` (`=` form) because clap's `.one_optional` does not consume a following argv element.
f387db6 to
a3a7bb2
Compare
There was a problem hiding this comment.
LGTM — small, well-tested env-var addition; all prior review feedback has been addressed in the Rust port.
Extended reasoning...
Overview
This PR adds BUN_CONFIG_ELIDE_LINES as an environment-variable equivalent of the --elide-lines CLI flag for bun run --filter. It touches five files: a one-line env-var declaration in env_var.rs, a small parse_elide_lines_option helper in Arguments.rs that consolidates two previously-duplicated parse blocks (CLI flag → env var fallback, with empty-string passthrough and usize::try_from clamping), input validation for run.elide-lines in bunfig.rs (rejecting negative/non-finite/fractional/out-of-range), four new tests in filter-workspace.test.ts, and a one-line docs entry.
Security risks
None. This controls how many lines of subprocess output are shown in the terminal during bun run --filter — a pure display preference with no auth, crypto, network, filesystem, or privilege implications.
Level of scrutiny
Low-to-medium. The change is small, mechanical, and follows established patterns (new! macro for env vars, existing CLI option parsing shape). Both removed elide-lines blocks and the single remaining call site are inside the same RunCommand | AutoCommand guard, and the consolidated call now correctly runs after load_config_with_cmd_args so CLI/env precedence over bunfig is preserved. The Rust as cast in bunfig.rs saturates rather than panicking, so the boundary concern from the earlier Zig version no longer applies.
Other factors
This PR has been through extensive iteration (8+ rounds of bot review since April), and every prior inline comment is marked resolved in the current diff: doc wording corrected to "maximum number of lines shown", empty --elide-lines falls through to the env var, bunfig validation added with n.fract() != 0.0 check, the bunfig test uses the --config= form, and 32-bit clamping uses usize::try_from().unwrap_or(usize::MAX). The bug-hunting system found no issues in this revision. Tests cover env-var precedence, CLI override, empty-flag fallback, and bunfig fractional rejection.
This branch has not been deployed
2 participants
Split out from #18111 by @martinamps.
Adds
BUN_CONFIG_ELIDE_LINESas an environment-variable equivalent of--elide-lines, so the limit can be set once globally instead of per command. The CLI flag takes precedence when both are set.Closes #11465