🐛 Quote an edited flow-unsafe scalar inside a flow collection#189
Conversation
A value assigned into a flow collection (`[...]`/`{...}`) that contained a flow
indicator was emitted as a bare plain scalar, so `doc["seq"][0] = "a,b"` produced
`[a,b, 2]`, which reparses as two entries (or breaks the parse for `[`/`{`/`: `).
The round-trip emitter now tracks how many flow collections are open and quotes a
plain scalar carrying a flow indicator (`,` `[` `]` `{` `}`, or a `: ` / trailing
`:`) while inside one. This only affects an edited-in value: a loaded plain
scalar never contains a bare flow indicator (the scanner would not produce one),
so an unmodified document still re-emits byte-for-byte.
frenck
added
the
bugfix
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a round-trip emitter bug where a value edited into a flow collection ([...]/{...}) that contains a flow indicator was emitted as a bare plain scalar. For example, doc["seq"][0] = "a,b" produced [a,b, 2], which reparses as two entries; a [/{/: would break the parse entirely. This sits in src/roundtrip/ (the rich AST + emitter path), complementing the fast dumps path, which already quoted such scalars in flow context via has_flow_indicator.
The emitter now tracks how many flow collections are open (flow_depth) and double-quotes a plain scalar carrying a flow indicator while inside one. I verified this only affects edited-in values: flow_depth is incremented/decremented symmetrically only in emit_flow_sequence/emit_flow_mapping (the sole sites emitting [/{), with no intervening early returns, and unmodified documents still re-emit verbatim, so byte-for-byte fidelity is preserved. The : /trailing-: branches of plain_unsafe_in_flow are defensive (such values are already quoted at assignment time by needs_quoting), which is consistent with the emitter treating quoting as its own correctness boundary.
Changes:
- Added a
flow_depthcounter toRoundTripEmitter, maintained around flow-collection emission. - Added a new
emit_scalararm that double-quotes a plain scalar viaplain_unsafe_in_flowwhen inside a flow collection. - Added a parametrized test verifying edited-in flow-unsafe scalars round-trip as a single string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/roundtrip/emit.rs |
Adds flow_depth tracking and the plain_unsafe_in_flow guard so edited-in flow-unsafe plain scalars are quoted; core of the fix. |
tests/roundtrip/test_edge_cases.py |
Adds a parametrized test asserting values with ,[]{}/: /trailing : round-trip correctly inside flow collections. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #189 +/- ##
=======================================
Coverage 92.37% 92.38%
=======================================
Files 40 40
Lines 11580 11595 +15
=======================================
+ Hits 10697 10712 +15
Misses 883 883 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Breaking change
None. This only changes how an edited-in scalar is emitted inside a flow collection (it is now quoted when it must be); an unmodified document still re-emits byte-for-byte.
Proposed change
A value assigned into a flow collection (
[...]/{...}) that contained a flow indicator was emitted as a bare plain scalar, sodoc["seq"][0] = "a,b"produced[a,b, 2], which reparses as two entries (and[/{/:break the parse outright).The round-trip emitter now tracks how many flow collections are open and quotes a plain scalar carrying a flow indicator (
,[]{}, or a:/ trailing:) while inside one. This only affects an edited-in value: a loaded plain scalar never contains a bare flow indicator (the scanner would not produce one), so an unmodified document still re-emits byte-for-byte.Type of change
Additional information
Checklist
uv run pytestpasses locally. A pull request cannot be merged unless CI is green.uv run ruff check .anduv run ruff format --check .pass.cargo fmt --checkandcargo clippy --all-targets -- -D warningspass.If the change is user-facing:
docs/is added or updated, anddocs/verify_examples.pystill passes.