Skip to content

shell_parser: drop stale capacity==1 debug_assert in parse_atom#31191

Merged
Jarred-Sumner merged 2 commits into
mainfrom
farm/4bf77864/shell-parse-atom-capacity-assert
May 22, 2026
Merged

shell_parser: drop stale capacity==1 debug_assert in parse_atom#31191
Jarred-Sumner merged 2 commits into
mainfrom
farm/4bf77864/shell-parse-atom-capacity-assert

Conversation

@robobun
Copy link
Copy Markdown
Collaborator

@robobun robobun commented May 21, 2026

Fuzzilli found a debug-build panic in the shell parser:

panic: assertion failed: atoms.capacity() == 1 (src/shell_parser/parse.rs:1932:17)

Minimal repro (debug build):

await Bun.$`echo hello`;

Root cause

parse_atom creates its atom list with ArenaVec::with_capacity_in(1, ..) and, when exactly one atom was pushed, asserts atoms.capacity() == 1. In the Zig original this verified the std.heap.stackFallback(@sizeOf(SimpleAtom)) slot was sufficient and no heap allocation occurred.

Since 303cd28, ArenaVec is BabyVec, whose with_capacity_in routes through grow_to:

let new_cap = (self.cap as usize * 2).max(at_least).max(4);

so the initial capacity is always 4. The assertion therefore fires on every single-atom token in debug builds — i.e. on essentially any Bun.$ invocation.

Fix

Remove the assertion. There is no stack fallback in the Rust port, so the check guards no invariant; the len() == 1 match arm already establishes the only property the code relies on.

Covered by the existing test/js/bun/shell/parse.test.ts (parse\echo foo`et al.), which crashed underbun-debug` before this change and passes after.

BabyVec::with_capacity_in(1) routes through grow_to which floors the
allocation at 4 slots, so atoms.capacity() is never 1. The assertion
was a stack-fallback check carried over from the Zig port and does not
guard any invariant in the Rust implementation. Every single-atom parse
(e.g. `echo foo`) tripped it in debug builds since 303cd28
introduced BabyVec as the ArenaVec backing.
@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented May 21, 2026

Updated 2:49 PM PT - May 21st, 2026

@robobun, your commit f86c738b42c1d050c54dc05a2ffa1e86ffd697c5 passed in Build #56710! 🎉


🧪   To try this PR locally:

bunx bun-pr 31191

That installs a local version of the PR into your bun-31191 executable, so you can run:

bun-31191 --bun

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0faff96b-3552-4e7e-bf06-7b5ba515995f

📥 Commits

Reviewing files that changed from the base of the PR and between ad4fac0 and f86c738.

📒 Files selected for processing (1)
  • test/js/bun/shell/parse.test.ts

Walkthrough

Simplifies the single-atom branch in Parser::parse_atom by removing a redundant debug_assert! and adds tests verifying parsing of a lone command (ls) and a tilde argument (echo ~).

Changes

Parse atom debug assertion cleanup

Layer / File(s) Summary
Atom parsing simplification
src/shell_parser/parse.rs
The atoms.len() == 1 branch in parse_atom is refactored to remove the surrounding debug_assert!(atoms.capacity() == 1) wrapper while preserving the returned Simple atom value.
Single-atom tests
test/js/bun/shell/parse.test.ts
Adds test("single atom") asserting that parse\ls`produces a command with a single simple atom and thatparse`echo `representsas atildeatom inname_and_args`.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: removing a stale debug assertion in parse_atom that checked capacity == 1.
Description check ✅ Passed The description covers the required sections: what the PR does (removes stale assertion), root cause analysis, and fix explanation with test coverage details.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. fix(baby_vec): with_capacity_in respects exact capacity #31189 - Alternative fix for the same atoms.capacity() == 1 panic: changes BabyVec::with_capacity_in to respect exact capacity instead of removing the assertion
  2. Hardening: input validation and bounds tightening across 28 subsystems (round 2) #31175 - Broad hardening PR that also removes the same debug_assert!(atoms.capacity() == 1) in parse_atom

🤖 Generated with Claude Code

@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented May 21, 2026

Re: duplicates —

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the removed debug_assert is provably always-false given BabyVec's min-capacity-4 growth policy, and it guarded no invariant in the Rust port.

Extended reasoning...

Overview

This PR removes a single debug_assert!(atoms.capacity() == 1) from Parser::parse_atom in src/shell_parser/parse.rs and adds a regression test for single-atom parsing (ls, echo ~) in test/js/bun/shell/parse.test.ts.

Correctness

I verified the root cause directly: ArenaVec is aliased to BabyVec (src/bun_alloc/lib.rs:280), and BabyVec::with_capacity_in(1, ..) calls grow_to(1) which computes new_cap = (0).max(1).max(4) = 4 (src/bun_alloc/baby_vec.rs:344-346). The assertion therefore fires on every single-atom token in debug builds. The assertion was a vestige of the Zig stack-fallback allocator and guards no invariant here — the len() == 1 match arm already establishes everything the code relies on. Release builds are unaffected since debug_assert! compiles out.

Security risks

None. This is a pure deletion of a debug-only assertion with no control-flow, data-handling, or boundary changes.

Level of scrutiny

Low. A one-line removal of a provably-stale debug assertion, plus an additive test that follows existing patterns in the same file. The CI failures reported by robobun are unrelated build-infrastructure issues (clang -no-pie linker warnings and scripts/build/ci.ts build failures) that affect the base branch, not this diff.

Other factors

No CODEOWNERS entry covers src/shell_parser/. No prior human review comments are outstanding. The bug-hunting system found no issues.

@Jarred-Sumner Jarred-Sumner merged commit dbdf818 into main May 22, 2026
79 checks passed
@Jarred-Sumner Jarred-Sumner deleted the farm/4bf77864/shell-parse-atom-capacity-assert branch May 22, 2026 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants