fix(yml): keep folded block scalars stable across encode round trips - #244
Open
AshGodfrey wants to merge 6 commits into
Open
fix(yml): keep folded block scalars stable across encode round trips#244AshGodfrey wants to merge 6 commits into
AshGodfrey wants to merge 6 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Contributor
📊 Test Coverage ReportCurrent Coverage: Coverage Change: 📈 +.1% (improved) Coverage by Package
📋 Detailed Coverage by Function (click to expand)
Generated by GitHub Actions |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
gopkg.in/yaml.v3 emits an extra line break immediately before a more-indented line inside a folded scalar. Applying an overlay is a decode/mutate/encode round trip, so every apply grew such a scalar by one blank line, and the break became part of the decoded string. Re-style affected folded scalars as literal blocks before returning from ApplyTo and ApplyToStrict. Literal blocks reproduce their value verbatim and round trip unchanged, so this removes the trigger without forking the frozen yaml.v3 emitter. Explicitly tagged scalars combine TaggedStyle with FoldedStyle, so the style is matched and replaced bitwise to preserve the tag.
AshGodfrey
force-pushed
the
ash/overlay-stabilize-folded-scalars
branch
from
August 24, 2026 13:26
2a68c54 to
4a6a06e
Compare
The yaml.v3 emitter defect is not specific to overlays: any decode/encode round trip grows a folded scalar that contains a more-indented line, so a document repeatedly unmarshalled and marshalled through this package accumulates blank lines with no overlay involved. Move the stabilizer to yml.StabilizeFoldedScalars and call it immediately before encoding, alongside the existing resetNodeStylesForYAML pass in marshaller, when rewriting localized files, and when serializing an overlay document. overlay.ApplyTo and ApplyToStrict keep calling it so consumers that encode the applied tree themselves stay covered. Exporting it from yml also lets consumers with their own encoders call it directly.
AshGodfrey
changed the title
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="yml/foldedscalar.go">
<violation number="1" location="yml/foldedscalar.go:43">
P2: When the input is an alias or a subtree whose folded scalar is reachable only through `Alias`, this traversal never stabilizes the target, so repeated encode/decode cycles can still accumulate blank lines. Traverse alias targets with a visited set to avoid recursing indefinitely through YAML alias cycles.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| node.Style = node.Style&^yaml.FoldedStyle | yaml.LiteralStyle | ||
| } | ||
|
|
||
| for _, child := range node.Content { |
There was a problem hiding this comment.
P2: When the input is an alias or a subtree whose folded scalar is reachable only through Alias, this traversal never stabilizes the target, so repeated encode/decode cycles can still accumulate blank lines. Traverse alias targets with a visited set to avoid recursing indefinitely through YAML alias cycles.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At yml/foldedscalar.go, line 43:
<comment>When the input is an alias or a subtree whose folded scalar is reachable only through `Alias`, this traversal never stabilizes the target, so repeated encode/decode cycles can still accumulate blank lines. Traverse alias targets with a visited set to avoid recursing indefinitely through YAML alias cycles.</comment>
<file context>
@@ -0,0 +1,46 @@
+ node.Style = node.Style&^yaml.FoldedStyle | yaml.LiteralStyle
+ }
+
+ for _, child := range node.Content {
+ StabilizeFoldedScalars(child)
+ }
</file context>
Author
There was a problem hiding this comment.
Not reachable in practice, so leaving the traversal as is.
An anchor definition is an ordinary node in the tree — &anchor >- is a scalar node sitting in its parent's Content, carrying an Anchor value — so the existing walk already visits and stabilizes it. An alias node adds no new content; it points back at that same node, and the encoder emits *anchor for it. A document such as:
a: &anchor >-
one
more indented
b: *anchoris a fixed point after 30 encode/decode cycles under this implementation. Added as TestStabilizeFoldedScalars_HandlesAnchoredScalars in 37c7fb9 so it stays that way.
The only shape where a folded scalar is reachable solely through Alias is a bare alias node handed in as the root of a subtree whose anchor is defined outside it — and encoding that subtree emits *anchor with no anchor definition, i.e. invalid YAML regardless of styling. Recursing into Alias would also need a visited set to stay safe on hand-built cyclic trees, which is complexity for a case a decoded document cannot produce.
Format and ToString on a nil *Overlay encoded as "null"; stabilizing the update payloads dereferenced the receiver first and panicked. Guard it, and cover anchored scalars while here: an anchor definition is an ordinary node in the tree, so stabilizing it covers every alias that references it.
2ynn
reviewed
Aug 24, 2026
2ynn
left a comment
2ynn
left a comment
There was a problem hiding this comment.
It looks like there is one yaml.Marshal call that is still not covered in oq/format.go:
https://github.com/speakeasy-api/openapi/blob/main/oq/format.go#L313 so specs going through openapi spec query still get inserted blank lines.
I would also suggest adding test coverage for > and >+
| doc := "title: Original\n" + foldedWithMoreIndentedLine | ||
| want := decodeDescription(t, doc) | ||
|
|
||
| for i := range 30 { |
There was a problem hiding this comment.
nit: i don't think 30 iterations is necessary (multiple instances)
Suggested change
| for i := range 30 { | |
| for i := range 3 { |
| } | ||
|
|
||
| // Marshal back to YAML | ||
| yml.StabilizeFoldedScalars(&node) |
There was a problem hiding this comment.
Would be worth adding a test fixture with a more-indented string in https://github.com/speakeasy-api/openapi/tree/main/openapi/testdata/localize
| } | ||
|
|
||
| // An overlay carries folded scalars of its own, in the update payloads it applies. | ||
| func TestFormatSurvivesRepeatedRoundTrips(t *testing.T) { |
There was a problem hiding this comment.
This test name is slightly misleading as it invokes ToString, not Format. I think this test should exercise both of these methods (like it's done in TestFormatToleratesNilOverlay)
| func TestMarshal_FoldedScalar_SurvivesRepeatedRoundTrips(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
nit:
Suggested change
| ctx := context.Background() | |
| ctx := t.Context() |
TristanSpeakEasy
requested changes
Aug 25, 2026
TristanSpeakEasy
left a comment
TristanSpeakEasy
left a comment
Member
There was a problem hiding this comment.
The core workaround looks sound, but one user-facing encode boundary is still uncovered: oq.FormatYAML calls yaml.Marshal(wrapper) directly in oq/format.go, so output from openapi spec query --format yaml can still gain the extra blank line. This was also identified by @2ynn. Please stabilize the wrapper/node before that marshal and add a focused query-format regression test.
Validation:
mise test ./yml ./overlay ./openapi: passed (1,405 tests; 1 pre-existing skip).go test ./oq: passed, but does not cover the affected folded-scalar case.go test ./commands/overlay: passed (no test files).- Probed
>,>-, and>+: all preserve their decoded value after stabilization. - PR CI: 12 successful, 1 skipped, 0 failing.
Requesting changes for the remaining query serialization path.
FormatYAML marshals graph nodes with its own encoder rather than going through the core model, so `openapi spec query --format yaml` was the one user-facing encode boundary still inserting a blank line into a folded scalar that contains a more-indented line. Stabilize the wrapper before marshalling it. As elsewhere, this restyles in place: the nodes belong to the loaded document, and only their representation changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Cover all three chomping indicators. yaml.v3 does not record chomping in Node.Style, it re-derives one from the value's trailing newlines, so `>` becomes `|` and `>+` with trailing blank lines becomes `|+`. - Exercise Overlay.Format as well as ToString; the round-trip test named after Format only ever called ToString. - Add a more-indented folded description to the localize input fixture, so the expected-output files pin that path too. Reverting the stabilizer in localize.go fails these fixtures. - Drop the repeat counts from 30 to 3. The emitter inserts its break on the first encode, so three passes catch it just as well. - Use t.Context() in the openapi round-trip test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
gopkg.in/yaml.v3emits an extra line break immediately before a more-indented line inside a folded (>-) scalar. The break lands inside the scalar, so it becomes part of the decoded string rather than cosmetic whitespace: any decode/mutate/encode round trip grows such a scalar by one blank line, and a document run through many round trips accumulates roughly one blank line per pass in descriptions nothing ever targeted.Minimal reproduction, no overlays involved:
Folded scalars without a more-indented line, and literal (
|-) blocks, are already stable.The emitter bug can't be fixed at its source:
go-yaml/yamlis archived, andgopkg.in/yaml.v3is frozen at v3.0.1.What changed
yml.StabilizeFoldedScalarsre-styles folded scalars containing a more-indented line as literal blocks, which reproduce their value verbatim and round trip unchanged. It is called immediately before encoding, at every point in this repository that writes a node tree back out:marshaller.CoreModel.Marshal, alongside the existingresetNodeStylesForYAMLpass — this coversopenapi.Marshal,swagger.Marshal, and everything else that goes through the core modelopenapi.Localize, when rewriting references in a copied external fileOverlay.Format/Overlay.ToString, for folded scalars in an overlay's own update payloadsOverlay.ApplyTo/ApplyToStrict, so consumers that encode the applied tree with their own encoder are covered without having to changeIt is exported so consumers with their own encode paths can call it directly.
This removes the trigger rather than patching the emitter, so it needs no fork or
replacedirective. The decoded value is unchanged — the node already holds the folded value — so only the on-disk representation differs.Testing
go test ./...passes, including the pre-existing expected-output fixtures.New tests:
yml: unit coverage for detection, explicit!!strtags, styles that must be left alone, nil-node guards, andTestFoldedScalarGrowsWithoutStabilizer, which asserts the unpatched emitter still misbehaves so it fails ifyaml.v3is ever fixed and the workaround can be removedopenapi: 30Unmarshal/Marshalround trips of a document with an affected description, asserting the decoded value never changesoverlay: 30 applies viaApplyToandApplyToStrict, plus 30ToStringround trips of an overlay whose update payload holds an affected scalarVerified end to end against three builds of this module — the released version, the first commit of this branch, and its head — exercising four round-trip paths on a document whose description contains a more-indented table row:
openapi.Unmarshal→Marshal, ×30ApplyTo+ caller's own encoder, ×30Overlay.ToString, ×30openapi.Localize, ×10Note for reviewers
Affected scalars change representation from
>-to|the first time a document is re-encoded after this lands. That is a one-time diff; output is stable afterwards.