Conversation
…width YAML.stringify counted a sequence item as one unit of the space argument. The `- ` that starts an item is always two columns wide, so a collection that starts on the dash line had its later entries at a different column than its first entry for every space other than 2. The output failed to parse, or parsed to different data. The stringifier now records, for each open block collection, whether it is a mapping value (indent by one space unit) or a sequence item (indent by two columns), and newline() writes the sum.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughYAML stringification now tracks mapping-value and sequence-item indentation separately. Sequence items use fixed two-column indentation, while mapping values use configured spacing. Tests cover nested collections, anchors, shared references, and round trips. ChangesYAML stringification
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The YAML indentation fix is covered by passing tests, with no merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and found no bugs. Deferring only because packages/bun-types/bun.d.ts is CODEOWNER-gated, so a human sign-off on the JSDoc wording is still needed.
What was reviewed:
newline()for bothSpace::NumberandSpace::Str— sequence steps emit the 2-space constant, mapping steps emit the user's unit;indent_len()matches what each arm actually appends.- Confirmed no other site in
YAMLObject.rsstill treatsindentas ausize(grep for allindentuses). - New tests: exact-line assertions across widths 1–8 plus round-trip
parse(stringify(...))over 8 shapes × 10spacevalues, anchored/shared refs, and the"\t"string case.
Extended reasoning...
Overview
The PR replaces the stringifier's scalar indent: usize with a Vec<IndentStep> stack so that sequence-item nesting always contributes exactly 2 columns (the width of - ) while mapping-value nesting contributes one unit of the user's space argument. newline() is updated for both the numeric and string space arms, a new indent_len() helper sizes the capacity reservation, and a SEQUENCE_ITEM_INDENT constant names the 2-space width. Two lines of JSDoc are added to bun.d.ts, and test/js/bun/yaml/yaml.test.ts gains a describe block with exact-line assertions and parse↔stringify round-trips across widths 1–10 and space strings.
Security risks
None. This is pure output-formatting logic in the YAML stringifier; no untrusted-input parsing paths, allocations sized from external data, or FFI/auth surfaces are touched. The Vec<IndentStep> depth is bounded by the existing recursion/stack check.
Level of scrutiny
Moderate. The Rust change is ~40 lines and mechanical: two += 1/-= 1 pairs become push/pop, and newline() sums per-step widths instead of count * unit. I grepped YAMLObject.rs for every indent reference and confirmed all sites were migrated — nothing else reads the field. The Space::Str arm correctly emits literal spaces for SequenceItem steps (so a "\t" space string no longer taints the dash-alignment column), matching the numeric arm's semantics. ensure_unused_capacity is still called with the exact count that will be appended.
Other factors
Test coverage is strong by REVIEW.md's bar: exact toEqual line arrays (not toContain), a test.each matrix over 10 space values, boundary widths 1 and 10, string vs numeric, anchored/shared references, and round-trip verification against YAML.parse (empirical semantics check). The space=2 case is included as a control so unchanged output is asserted. The only reason not to approve outright is that packages/bun-types/ is listed in .github/CODEOWNERS, and the approval guidelines say not to approve over a CODEOWNER-gated path.
The sentence about sequence items belongs with a rewrite of the space text for string values, which this change does not touch.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 8:08 AM PT - Sep 12th, 2026
✅ @robobun, your commit bdbac4c47e369064d986ea39448d3dda76013d84 passed in 🧪 To try this PR locally: bunx bun-pr 42483That installs a local version of the PR into your bun-42483 --bun |
|
Follow-ups to the reviews:
|
There was a problem hiding this comment.
LGTM — focused fix with thorough round-trip coverage.
Checked that the IndentStep push/pop pairs are balanced on every path through stringify_unwrapped (including the ? early returns — the stack is owned by self and only grows/shrinks around the recursive call, so an error unwinds with the same depth it entered). Verified indent_len() and the Space::Str branch agree on total width (each SequenceItem contributes exactly SEQUENCE_ITEM_INDENT.len() in both), and that no other site read the old usize indent. Output at space 2 is byte-identical to before since both step kinds resolve to 2 columns there.
Extended reasoning...
Overview
This PR fixes Bun.YAML.stringify indentation for block collections nested inside sequence items when space ≠ 2. The old code tracked indentation as a single usize depth counter and multiplied it by the space width for every level, but the - dash prefix is always 2 columns wide regardless of space, so nested entries after the first would misalign and produce YAML that either fails to parse or parses to a different value. The fix replaces the counter with a Vec<IndentStep> stack where MappingValue contributes one space unit and SequenceItem contributes a fixed 2 columns. newline() walks the stack to compute the exact indentation. About 100 lines of new tests in the existing test/js/bun/yaml/yaml.test.ts cover exact-line output at widths 1–8, tab-string space, anchored collections, and a test.each round-trip matrix over ten space settings.
Security risks
None. This is pure serialization formatting logic — no untrusted input parsing, no allocation sizing driven by external data beyond what already existed, no FFI boundary changes. The Vec<IndentStep> grows with recursion depth, which is already bounded by the existing StackCheck.
Level of scrutiny
Low-to-moderate. The change is ~40 lines of Rust in a single function cluster (newline, new indent_len, two push/pop sites) plus tests. The transformation from indent += 1 / indent -= 1 to indent.push(...) / indent.pop() is mechanical and the two sites are the only mutation points. I confirmed via grep that no other code reads self.indent. The Space::Str branch correctly clones space_str before borrowing &self.indent, so there are no borrow conflicts. ensure_unused_capacity uses indent_len() which sums the exact byte count each step will append.
Other factors
Tests follow repo conventions: added to the existing module test file, use test.each for the parameterized matrix, assert with .toEqual on line arrays, and include a space: 2 control case so the suite still exercises the unchanged path. The PR description states 14 of 15 new tests fail on 1.4.3, satisfying the fails-without-fix requirement. Since the last reviewed push, only comment wording was shortened and an unrelated .d.ts doc addition was reverted — no logic changed. The github-actions inline comment at line 78 was followed by a commit that reworded exactly those lines, plausibly addressing it. No CODEOWNERS entry covers these paths.
|
Closing: #39961 merged on 2026-09-15 and fixes the same bug. It adds an Checked on
The question about a |
2 participants
Problem
Bun.YAML.stringify(value, null, space)writes wrong YAML for everyspaceother than 2 when a sequence item holds a collection. At 4,[[1, 2], 3]gives- - 1\n - 2\n- 3, which parses as[["1 - 2"], 3].[{ a: 1, b: 2 }]gives- a: 1\n b: 2:YAML Parse error: Unexpected token.src/runtime/api/YAMLObject.rs:481. A sequence item added one indent level, andnewline()wrotelevel * spacecolumns. The-before the first entry is always 2 columns wide.Fix
IndentStepfor each open block collection. A mapping value adds onespaceunit. A sequence item adds 2 columns.newline()writes the sum.space2 does not change. At other widths the indentation is the same as theyamlnpm package:k:\n - a: 1\n b: 2at 4.spacestring that is not all spaces ("\t") still gives invalid YAML when a mapping nests.test/js/bun/yaml/yaml.test.ts, new block "collections nested in a sequence item, at every indent width". 14 of the 15 new tests fail on 1.4.3 (space2 is the control). All oftest/js/bun/yaml/passes.Background
-line of a sequence item:- a: 1. Its other entries must start at the column of the first entry, which is the dash column plus 2.- - 1\n - 2reads as the string1 - 2, with no error.Notes
Reproduction (from the report, 15 of 18 lines print MISMATCH on 1.4.3, 1 of 18 with this change):
The line that still mismatches is
{ k: [{ a: 1, b: 2 }] }with"\t":k: \n\t- a: 1\n\t b: 2,Tab characters cannot be used as indentation. That is the mapping indent, which uses the string as it is. The two other"\t"cases have no mapping indent and now round-trip.Which layout. Three layouts are valid YAML for a collection in a sequence item when
spaceis not 2:[{ a: 1, b: 2 }]at 4- a: 1\n b: 2yaml2.9.1, Prettier 3.6.2 (--tab-width 4)- a: 1\n b: 2-\n a: 1\n b: 2This PR uses the first one. It keeps the output at
space2 as it is, and it works forspace1 without a second form. I compared the output withyaml2.9.1 for[[1,2],3],[{a:1,b:2}],{k:[{a:1,b:2}]},{k:{j:[[1,2],[3]]}},[[[1,2],3],4]and[{a:[1,2],b:{c:1,d:2}}]at 1, 2, 3, 4 and 8. The indentation is the same in all 30 (Bun still writes a space afterk:and no final newline).Output that changes but was valid before. A mapping value that is inside a sequence item was indented by 2
spaceunits from the dash. It is now indented by 2 columns plus 1spaceunit. Example at 4:[{ a: { b: 1 } }]was- a: \n b: 1and is now- a: \n b: 1. Both parse to the same value.Round-trip probe. 20,887 small shapes (exhaustive to depth 3 over scalars, strings that need quotes, empty collections) plus 3,000 seeded random values with shared references, plus a cyclic value, at
space1, 2, 3, 4, 5, 7, 8, 10 and space strings of 1, 2, 3, 4 and 10 spaces: 310,544 round trips. 1.4.3 fails 182,557 of them. This branch fails 0.Related. #41116 names this bug in its notes as out of scope and limits block scalars to
space2 or more because of it.[human-review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file