[yamllint-fixer] Reduce yamllint noise in generated lock files (trailing spaces + blank lines)#43509
Conversation
Fix two root causes of yamllint warnings in generated *.lock.yml files: - header.go: strip trailing whitespace from ASCII logo comment lines so the generated header no longer emits trailing spaces. - compiler_yaml.go: normalize the assembled workflow YAML so whitespace-only lines become empty lines (no trailing spaces) and the file ends with exactly one trailing newline. A whitespace-only line is semantically a blank line inside YAML block scalars, so this does not change parsed content. Reduces yamllint warnings by ~85% (4340 -> 616). Regenerates wasm golden fixtures accordingly. Lock files are intentionally not included; run `gh aw compile` after merge to regenerate them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. Changed files include source files (compiler_yaml.go, header.go) and golden testdata snapshots, but no *_test.go or *.test.cjs test functions. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This PR reduces yamllint noise in generated *.lock.yml files by removing two main sources of false-positive whitespace lint: trailing spaces in the ASCII logo header comments and indentation-only “blank” lines emitted by various YAML block scalars.
Changes:
- Trim trailing spaces from each embedded ASCII logo line before emitting it as a
# ...comment line in generated headers. - Add a final YAML post-processing pass that converts whitespace-only lines to truly empty lines and ensures the output ends with exactly one trailing newline.
- Update wasm golden fixtures to reflect the new blank-line/newline normalization behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/header.go | Trims trailing whitespace in ASCII logo comment lines to avoid trailing-spaces warnings. |
| pkg/workflow/compiler_yaml.go | Adds normalizeBlankLines post-pass to clear whitespace-only lines and normalize final newline. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden | Golden output updated for whitespace-only line normalization. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden | Golden output updated for whitespace-only line normalization. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden | Golden output updated for whitespace-only line normalization. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden | Golden output updated for whitespace-only line normalization. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden | Golden output updated for whitespace-only line normalization. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden | Golden output updated for whitespace-only line normalization. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden | Golden output updated for whitespace-only line normalization. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden | Golden output updated for whitespace-only line normalization. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden | Golden output updated for whitespace-only line normalization. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/11 changed files
- Comments generated: 0
- Review effort level: Low
Draft ADR documenting the decision to normalize generated YAML whitespace via a single post-processing pass rather than fixing each generator source. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (128 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with suggestions; no blocking issues found.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps:
normalizeBlankLineshas no direct unit tests;TestGenerateWorkflowHeader_*don't assert absence of trailing spaces. The golden file suite covers the integration path, but a future refactor could break the contract silently. - Function naming:
normalizeBlankLinesdoes two things (strip whitespace-only lines and enforce a single trailing newline); the name only reflects one of them. - Inline comment duplication: The call-site comment at line 446–452 fully re-states the function godoc at lines 307–314; trimming it improves maintainability.
Positive Highlights
- ✅ Excellent root-cause analysis: tackling the generator instead of post-processing the lock files is the right approach.
- ✅ 85% reduction in yamllint warnings (4340 → 616) with targeted, minimal code changes.
- ✅ The invariant that whitespace-only lines are semantically blank in YAML block scalars is correctly identified and well-documented.
- ✅ Logo trimming in
header.gois clean: fully-blank logo lines are still emitted as#\n(not an empty line), preserving visual structure. - ✅ Golden fixtures updated consistently across all engine variants.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 64.7 AIC · ⌖ 7.45 AIC · ⊞ 6.6K
Comment /matt to run again
| lines[i] = "" | ||
| } | ||
| } | ||
| return strings.TrimRight(strings.Join(lines, "\n"), "\n") + "\n" |
There was a problem hiding this comment.
[/tdd] normalizeBlankLines has no direct unit tests — edge cases like an empty string input, no-trailing-newline input, multiple consecutive trailing newlines, and content lines with trailing spaces (which must not be touched) are not explicitly covered. The golden file tests verify the integration path but do not pinpoint which part breaks if this function is refactored.
💡 Suggested test skeleton
func TestNormalizeBlankLines(t *testing.T) {
tests := []struct {
name string
input string
want string
}{
{"empty string", "", "\n"},
{"already clean", "foo\n\nbar\n", "foo\n\nbar\n"},
{"whitespace-only line becomes empty", "foo\n \nbar\n", "foo\n\nbar\n"},
{"multiple trailing blank lines collapsed", "foo\n\n\n\n", "foo\n"},
{"content trailing spaces preserved", "foo \nbar\n", "foo \nbar\n"},
{"no trailing newline gets one added", "foo", "foo\n"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := normalizeBlankLines(tt.input)
if got != tt.want {
t.Errorf("got %q, want %q", got, tt.want)
}
})
}
}@copilot please address this.
| lines[i] = "" | ||
| } | ||
| } | ||
| return strings.TrimRight(strings.Join(lines, "\n"), "\n") + "\n" |
There was a problem hiding this comment.
[/diagnosing-bugs] The function name normalizeBlankLines understates what it does: it also ensures a single trailing newline. A reader looking for "trailing newline enforcement" won't find it by name, and the dual concern slightly complicates future changes.
💡 Naming suggestion
Consider normalizeWhitespace or normalizeYAMLOutput to signal both concerns, or split into two composable helpers — stripTrailingWhitespace and ensureTrailingNewline — so each can be tested and evolved independently.
If splitting is overkill, at minimum rename to normalizeGeneratedYAML to make the dual purpose discoverable.
@copilot please address this.
| // blank line inside YAML block scalars, so clearing it does not change the | ||
| // parsed content. Also ensure the file ends with exactly one trailing newline | ||
| // (a trailing blank line is flagged by yamllint as empty-lines). | ||
| yamlContent = normalizeBlankLines(yamlContent) |
There was a problem hiding this comment.
[/diagnosing-bugs] normalizeBlankLines is applied after replaceIssueNumberReferences but the duplication comment at line 446-452 repeats much of what the function's own godoc already says (lines 307-314). The inline comment is valuable — but it should be a short call-site note referencing the function, not a full re-explanation.
💡 Suggested trimmed comment
// Strip whitespace-only lines and enforce a single trailing newline.
// See normalizeBlankLines for details.
yamlContent = normalizeBlankLines(yamlContent)This keeps the call site readable while directing maintainers to the canonical godoc.
@copilot please address this.
| fmt.Fprintf(&header, "# %s\n", line) | ||
| // Strip trailing whitespace from each logo line so the generated comment | ||
| // lines do not carry trailing spaces (yamllint trailing-spaces). | ||
| if trimmed := strings.TrimRight(line, " \t"); trimmed == "" { |
There was a problem hiding this comment.
[/tdd] The existing TestGenerateWorkflowHeader_* tests don't assert that the generated header contains no lines with trailing spaces. If the logo ever gains a new padding style, the regression would slip through silently.
💡 Suggested assertion
Add a helper check in TestGenerateWorkflowHeader_VersionInDevBuild (or a dedicated test) that scans every line of the header for trailing spaces:
for i, line := range strings.Split(header, "\n") {
if strings.TrimRight(line, " \t") != line {
t.Errorf("line %d has trailing whitespace: %q", i+1, line)
}
}This directly regression-tests the bug fixed in this PR.
@copilot please address this.
There was a problem hiding this comment.
Review: yamllint noise reduction
The approach is sound and well-documented. Both changes are minimal, targeted, and achieve the 85% reduction stated in the PR. Golden files are updated correctly.
normalizeBlankLines — correctly handles the common case: whitespace-only lines are semantically blank in YAML block scalars, so stripping them has no effect on parsed content. The TrimRight(...,"\n") + "\n" idiom correctly normalises the trailing newline.
header.go — strings.TrimRight(line, " \t") correctly preserves leading spaces (logo alignment) while stripping trailing whitespace. The empty-line branch emits # rather than # to avoid introducing a new trailing-space.
No blocking issues found. The 51 remaining trailing-spaces (content lines with intentional markdown trailing spaces) and comments-indentation/indentation warnings are correctly deferred.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 50 AIC · ⌖ 5.93 AIC · ⊞ 4.9K
There was a problem hiding this comment.
REQUEST_CHANGES — 4 issues across correctness, test coverage, and implementation consistency. The core approach (blank-line normalization + logo trailing-space strip) is sound, but the doc comment overstates safety guarantees, both new code paths lack direct unit tests, and the trim-function choice in header.go is inconsistent with compiler_yaml.go.
Findings summary
Correctness / Safety claim
normalizeBlankLinesdoc comment asserts the transformation is always content-neutral in YAML block scalars. This is only true when whitespace-only lines don't exceed the block's detected indentation level — a constraint that happens to hold for current generated output but is not enforced and not stated.
Test gaps (two separate issues)
normalizeBlankLineshas zero direct unit tests. Edge cases (empty input, no trailing newline, multi-trailing-blank collapse) are uncovered. Golden files cover only the whitespace-only-line clearing path; the trailing-newline enforcement has no golden-level coverage.TestASCIILogoAlignment/TestLogoTrimmingdon't assert the absence of trailing whitespace on generated logo lines. Theheader.gofix can be silently reverted without failing any test.
Implementation asymmetry
header.gousesstrings.TrimRight(line, " \t")(ASCII-only).compiler_yaml.gousesstrings.TrimSpace(Unicode-aware). An editor-introduced non-breaking space inlogo.txtwould be missed by the header fix but caught by the normalizer.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 153.8 AIC · ⌖ 10.7 AIC · ⊞ 5.4K
Comment /review to run again
| // - every whitespace-only line is emitted as an empty line (no trailing spaces), and | ||
| // - the file ends with exactly one trailing newline (no trailing blank line). | ||
| // | ||
| // A whitespace-only line is semantically a blank line inside YAML block scalars, so |
There was a problem hiding this comment.
Incorrect safety claim in doc comment: the assertion that clearing whitespace-only lines is always content-neutral is false for YAML literal block scalars where a "blank" line carries whitespace exceeding the block's detected indentation.
💡 Detail
Per YAML 1.2 §8.1.1.2, in a literal block scalar (|), a line whose whitespace content exceeds the block's detected indentation contributes the extra spaces as literal content. For example, if a block has detected indentation 6 and a later blank-looking line has 10 spaces, the parsed scalar value contains (4 spaces) — not an empty line. After normalizeBlankLines zeroes it out, the parsed content silently changes.
Currently generated workflows use consistent indentation for these blocks so the bug doesn't trigger. But the comment tells future maintainers the transformation is universally safe when it is not. Narrow the claim:
// A whitespace-only line whose indentation does not exceed the block scalar's detected
// indentation level is semantically blank; clearing it does not change the parsed content.
// This holds for the indentation structure of currently-generated lock files.Or cite the YAML spec section so the constraint remains visible.
| // clearing its whitespace does not change the parsed content. This removes the bulk | ||
| // of yamllint trailing-spaces and empty-lines noise from generated lock files without | ||
| // touching lines that carry real content. | ||
| func normalizeBlankLines(yamlContent string) string { |
There was a problem hiding this comment.
normalizeBlankLines has no unit tests: a package-level function performing two distinct transformations on the final compiled output has zero direct tests.
💡 Detail
The function does two things: (1) clears whitespace-only lines, and (2) enforces a single trailing newline. Each has edge cases worth pinning:
- Empty input
""→ produces"\n"(one newline added) — is that the intended contract? - Input with no trailing newline (e.g.,
"foo") → produces"foo\n"— correct, but untested. - Input that is all-whitespace (e.g.,
" \n \n") → produces"\n"— edge case for empty-ish workflows. - Input ending with multiple blank lines → collapses to a single trailing newline — the claimed behavior.
The golden file diffs only show one of the two transformations in action (whitespace-only line clearing). The trailing-newline enforcement has no golden-level coverage because all existing golden fixtures already ended with exactly one newline.
A small table-driven unit test in compiler_yaml_test.go would catch regressions during any future refactor:
func TestNormalizeBlankLines(t *testing.T) {
cases := []struct{ in, want string }{
{"foo\n \nbar\n", "foo\n\nbar\n"},
{"foo\n", "foo\n"},
{"foo", "foo\n"},
{"foo\n\n\n", "foo\n"},
{"", "\n"},
}
for _, c := range cases {
got := normalizeBlankLines(c.in)
if got != c.want {
t.Errorf("normalizeBlankLines(%q) = %q, want %q", c.in, got, c.want)
}
}
}| fmt.Fprintf(&header, "# %s\n", line) | ||
| // Strip trailing whitespace from each logo line so the generated comment | ||
| // lines do not carry trailing spaces (yamllint trailing-spaces). | ||
| if trimmed := strings.TrimRight(line, " \t"); trimmed == "" { |
There was a problem hiding this comment.
ASCII-only TrimRight is asymmetric with normalizeBlankLines' Unicode-aware TrimSpace: header.go uses strings.TrimRight(line, " \t") but compiler_yaml.go uses strings.TrimSpace(line) == "", which handles all Unicode whitespace.
💡 Detail
If logo.txt ever gains a non-ASCII whitespace character on a trailing position (non-breaking space U+00A0, en-space, etc. — common pasted-in editors), strings.TrimRight(line, " \t") will leave it in place, and the yamllint trailing-spaces warning reappears without any indication of why.
normalizeBlankLines by contrast uses strings.TrimSpace which covers the full Unicode \p{Z} category.
Make the two consistent:
if trimmed := strings.TrimRightFunc(line, unicode.IsSpace); trimmed == "" {
header.WriteString("#\n")
} else {
fmt.Fprintf(&header, "# %s\n", trimmed)
}This requires adding "unicode" to the import block. Alternatively at minimum document why ASCII-only is sufficient (e.g., "logo.txt is source-controlled and verified to contain only ASCII").
|
@copilot please run the
|
PR finisher run complete. Summary:
Actions taken: Ran Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, Still needed: Human verification of |
|
@copilot please run the Unresolved review feedback to address:
|
…flowAddCommentTargetContract The contract test asserts "Process at most **5 PRs** per run." as a safety throttle. A prior workflow edit changed this to "Process all eligible PRs per run." without updating the test. Restore the cap so CI passes. Recompile pr-sous-chef.lock.yml accordingly. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Root cause: Fixed in fix(pr-sous-chef): restore 5-PR-per-run cap by restoring the 5-PR cap and recompiling |
…ciders Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Actions taken this run: Failing test (pelikhan's note): ADR gate: Finalized PR body link (needs maintainer): The GitHub API is blocked in this sandbox so the PR body cannot be updated programmatically. A maintainer should add the following line to the PR body before merging:
Hand-off: CI must be re-triggered (close/reopen or maintainer push) before merge. Maintainer also needs to add the ADR link to the PR body to satisfy the ADR gate. |
Investigated the failure in 85228302141. The reported assertion — needle On current HEAD (
A new CI run was triggered on |
|
🎉 This pull request is included in a new release. Release: |
Summary
Reduce yamllint noise in generated workflow YAML by normalizing blank lines and trimming trailing spaces in generated headers.
Changes
Production code
normalizeBlankLines(yamlContent string) stringinpkg/workflow/compiler_yaml.go.normalizeBlankLines()at the end ofgenerateYAML()after existing transformations, includingreplaceIssueNumberReferences.pkg/workflow/header.goto right-trim each ASCII logo line before emitting# <line>.#instead of# <spaces>.Workflow source
.github/workflows/pr-sous-chef.mdto fixTestPRSousChefWorkflowAddCommentTargetContract.Documentation
docs/adr/43509-normalize-generated-yaml-whitespace-via-postprocessing.md.Generated artifacts
pkg/workflow/testdata/TestWasmGolden_AllEngines/pkg/workflow/testdata/TestWasmGolden_CompileFixtures/.github/workflows/agentic-auto-upgrade.yml.github/workflows/agentic_commands.yml.github/workflows/agentics-maintenance.ymlWhy
Compile runs produced about 4,340 yamllint warnings, mostly from generated whitespace:
trailing-spacesempty-linesThis change removes those warning sources in generated output without changing YAML meaning.
Impact
.lock.ymlfiles will pick up this normalization aftergh aw compile.Alternatives considered
Results
Expected yamllint warning reduction from this change:
comments-indentationindentationTesting
pr-sous-chefworkflow contract by restoring the 5-PR-per-run limit.