Skip to content

fix(yamlfmt): expand flow collections under the key's real column - #619

Merged
kehoecj merged 1 commit into
Boeing:feat/3.0from
snowyukitty:fix/yaml-flow-indent-in-sequence
Jul 31, 2026
Merged

fix(yamlfmt): expand flow collections under the key's real column#619
kehoecj merged 1 commit into
Boeing:feat/3.0from
snowyukitty:fix/yaml-flow-indent-in-sequence

Conversation

@snowyukitty

Copy link
Copy Markdown

Found while testing the release candidate as asked in #551, and rebased onto
37b2b6c.

The bug

cfv format --fix rewrites valid YAML into YAML that no longer parses, and
reports success while doing it.

$ cat relabel.yaml
relabel_configs:
  - values: ["first_long_element_value_aaaaaaaaaaaaaaaa", "second_long_element_value_bbbbbbbbbbbb", "third_long_element_cccccc"]
    action: keep

$ cfv format --fix relabel.yaml
    ✓ relabel.yaml

$ cfv check relabel.yaml
    × relabel.yaml
        error: syntax: line 3: could not find expected ':'

The rewritten file is:

relabel_configs:
  - values:
    [
      "first_long_element_value_aaaaaaaaaaaaaaaa",
      ...
    ]
    action: keep

values sits at column 4 and its expanded value is put at column 4 as well, so
it is no longer that key's value.

This shape — a key inside a block sequence item whose value is a long flow
collection — is common in Prometheus and Promtail relabel_configs, GitHub
Actions matrices, and Kubernetes manifests. I hit it by running
cfv format --fix over the config files of an unrelated repository.

Cause

expandLongFlowSequences takes the expansion indent from the line's
TokIndent, which stops before the sequence indicator. For
- values: [...] that gives 2, while the key actually starts at column 4, so
the bracket lands exactly on the key instead of under it. Outside a sequence
item the two agree, which is why the existing fixtures pass.

The fix tracks the key token while walking back to the line indent and lays the
expansion out from the key's real column, so anything sitting between the indent
and the key is accounted for rather than just - . The result matches Prettier
byte for byte:

relabel_configs:
  - values:
      [
        "first_long_element_value_aaaaaaaaaaaaaaaa",
        "second_long_element_value_bbbbbbbbbbbb",
        "third_long_element_cccccc",
      ]
    action: keep

Both [ and { were affected.

Tests

  • pkg/formatter/yamlfmt/testdata/flow_expand_in_sequence.{input,expected}.yaml
    covers [ and { inside a sequence item. TestIdempotency picks the golden
    file up automatically, so the same fixture also pins stability across passes.
  • cmd/cfv/testdata/format_yaml_flow_in_sequence.txtar pins it end to end:
    format, then cfv check must still pass, then formatting again must be a
    no-op.

Both fail on feat/3.0 — the txtar case fails with × relabel.yaml, which is
the corruption itself.

The golden file is byte-for-byte equal to prettier --parser yaml on the same
input, checked with Prettier 3, so it encodes the reference layout rather than
my preference.

Verification

Check Result
go test ./... (Linux, golang:1.26) identical to feat/3.0 — same 4 pre-existing failures, nothing new
golangci-lint run on the changed packages 0 issues
go vet ./..., gofmt -s -l -e . clean
Formatter fixtures for all nine formats pass
Prettier 3 parity on the new fixture byte-for-byte identical
Idempotence and validity sweep over 1,124 third-party config files 0 violations

These all produced unparseable output before and are valid and idempotent after:
a value carrying an anchor (key: &anc [...]), a flow nested in a flow, a
multi-document file, a CRLF file, a quoted key, and a doubly nested sequence.
Empty collections, comments inside a collection, tags, explicit keys, and
brackets or commas inside quoted strings were checked too.

The four failures common to both branches are Test_CLIWithUnreadableFile,
Test_FormatFixUnwritableDirectory, Test_fsFinderWalkDirError and
TestFetchAndCacheUnwritableDir; each asserts that an unreadable or unwritable
path errors, which does not hold when the suite runs as root in a container.

Two things left out

Prettier first tries the whole collection on its own line and only explodes it
element per element if that still does not fit:

  - labels:
      ["one_value_that_fits_on_its_own_line_once_it_is_no_longer_after_the_key"]

cfv always explodes. That is a layout difference rather than a correctness
one and it would move a lot of golden files, so it is not in this PR.

Separately, and unrelated to flow collections: when a sequence indicator is
followed by more than one space, the item's sibling keys get re-indented to a
different column and the document stops parsing.

$ printf 'items:\n  -   key: short\n      other: x\n' > t.yaml && cfv format --fix t.yaml && cfv check t.yaml
    × t.yaml
        error: syntax: line 1: did not find expected '-' indicator

It reproduces on feat/3.0 with no flow collection involved and is untouched
here — happy to report it properly on #551 if that helps.

A flow collection expanded past the print width took its indentation from the line's TokIndent, which stops before the sequence indicator. Inside a block sequence item the key starts further right, so 'key: [...]' put the bracket on the key's own column, where it is no longer that key's value: 'cfv format --fix' rewrote valid documents into unparseable ones while reporting success. Both '[' and '{' were affected.

Lay the expansion out from the key's own column instead, which also covers anything else sitting between the line indent and the key. The result matches Prettier byte for byte.
@snowyukitty
snowyukitty requested a review from a team as a code owner July 31, 2026 18:53
@kehoecj kehoecj added bug Something isn't working OSS Community Contribution Contributions from the OSS Community waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Jul 31, 2026

@kehoecj kehoecj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@snowyukitty Thank you for the PR and fix! From your findings, I've updated my parity and functional suite to include validation after the formatting.

@kehoecj
kehoecj merged commit 6330b79 into Boeing:feat/3.0 Jul 31, 2026
9 checks passed
@kehoecj

kehoecj commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

@snowyukitty Validated both of the other issues on "Two things left out" - those are valid as well. Please submit them as issues if you don't mind

@snowyukitty

Copy link
Copy Markdown
Author

Filed both: #622 for the sequence-indicator corruption and #623 for the flow-collection layout difference. Each has a reproduction verified against 6330b79.

Glad the parity suite picked up the post-format validation — that was the check that surfaced this one in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working OSS Community Contribution Contributions from the OSS Community waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants