[yamllint-fixer] fix(workflow): reduce yamllint noise in generated lock files (16 → 3)#45372
Conversation
…ed YAML Extend normalizeBlankLines so the final lock-file assembly also trims trailing spaces/tabs from content lines and caps consecutive blank lines at two. Previously it only cleared whitespace-only lines, leaving trailing-spaces on embedded shell/GraphQL/JS scripts and prompt heredoc content, plus over-long blank runs, which yamllint flagged. Trailing whitespace is never significant in the block scalars gh-aw emits, so this changes no runtime behaviour. Reduces yamllint warnings on generated *.lock.yml from 16 to 3 (all remaining are comments-indentation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45372 does not have the 'implementation' label and has only 44 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Reduces yamllint warnings in generated workflow lock files through final-pass whitespace normalization.
Changes:
- Trims trailing whitespace and caps blank-line runs.
- Adds normalization tests.
- Regenerates the WASM golden fixture.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_yaml.go |
Expands YAML whitespace normalization. |
pkg/workflow/compiler_yaml_test.go |
Tests the expanded behavior. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden |
Reflects normalized trailing whitespace. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/3 changed files
- Comments generated: 2
- Review effort level: Medium
| // Trim trailing whitespace from every line. On whitespace-only lines this | ||
| // yields an empty line; on content lines it strips trailing spaces/tabs that | ||
| // yamllint flags as trailing-spaces. | ||
| trimmed := strings.TrimRight(line, " \t") |
| if blankRun < maxConsecutiveBlankLines { | ||
| b.WriteByte('\n') | ||
| blankRun++ | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped fix. The TrimRight approach correctly preserves leading indentation while stripping trailing whitespace, the blank-run cap logic is correct, and the new test cases cover all changed behaviours. Golden file delta confirms no semantic change. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 11.7 AIC · ⌖ 4.92 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — approving with minor test-quality suggestions.
📋 Key Themes & Highlights
Key Themes
- Root cause fixed:
normalizeBlankLinesnow trims trailing whitespace from all lines, not just whitespace-only ones — the correct fix rather than a symptom workaround. - Constant is well-named:
maxConsecutiveBlankLinesmakes the policy explicit and directly ties to the yamllint rule it satisfies. - Test coverage added: 6 new table-driven cases cover both new behaviours.
- Minor: one test name is misleading and two cases overlap in intent (see inline comments).
Positive Highlights
- ✅ Clean single-responsibility change: one function, one responsibility extended
- ✅ Golden file delta confirms zero semantic impact
- ✅
blankRunreset on non-blank line is correctly placed afterlastNonBlankEndupdate - ✅ PR description is thorough and includes before/after metrics
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 21.9 AIC · ⌖ 4.52 AIC · ⊞ 6.6K
Comment /matt to run again
| {"single non-blank line", "key: value\n", "key: value\n"}, | ||
| {"multiple trailing blank lines", "a\n\n\n\n", "a\n"}, | ||
| {"only whitespace lines", " \n \n", "\n"}, | ||
| {"content-line trailing spaces trimmed", "key: value \n", "key: value\n"}, |
There was a problem hiding this comment.
[/tdd] Test name "interior trailing spaces preserved content" is misleading — it reads as if trailing spaces are being kept, but the assertion shows they are trimmed.
💡 Suggested rename
{"indentation preserved when trailing spaces trimmed", " foo: bar \n", " foo: bar\n"},The intent (leading whitespace untouched, trailing whitespace removed) should be readable from the test name itself, per /tdd spec-naming conventions.
@copilot please address this.
| {"only whitespace lines", " \n \n", "\n"}, | ||
| {"content-line trailing spaces trimmed", "key: value \n", "key: value\n"}, | ||
| {"content-line trailing tabs trimmed", "a\t\nb\n", "a\nb\n"}, | ||
| {"interior trailing spaces preserved content", " foo: bar \n", " foo: bar\n"}, |
There was a problem hiding this comment.
[/tdd] Two test cases (long interior blank run capped at two and three interior blanks capped at two) both test runs > 2 blanks and are redundant. Replace with one boundary case and one over-boundary case per /tdd boundary-testing discipline.
💡 Cleaner table
{"two interior blanks kept (at limit)", "a\n\n\nb\n", "a\n\n\nb\n"},
{"three interior blanks capped to two", "a\n\n\n\nb\n", "a\n\n\nb\n"},This makes the boundary explicit and removes the redundant five-blank case.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — two critical correctness bugs must be resolved before merge
normalizeBlankLines operates on the raw assembled YAML string with no YAML structural awareness. Both new behaviours — trailing whitespace trimming and blank-line capping — silently corrupt YAML literal block scalar content.
Blocking issues
1. Trailing whitespace stripping inside block scalars (line 354) — Covered by existing thread. Inside a run: | step, every line is scalar content per YAML 1.2. Shell \ continuations, Markdown hard-breaks, and any intentional trailing whitespace are silently removed.
2. Blank-run cap corrupts block scalar bodies (line 366) — Covered by existing thread. Three or more consecutive blank lines inside a run: | step are valid content; collapsing them to two changes runtime behavior, contradicting the PR description.
3. Missing round-trip test — No test constructs a real YAML document with a literal block scalar, passes it through normalizeBlankLines, and asserts the parsed scalar value is unchanged. Both regressions above are undetected by CI.
🔎 Code quality review by PR Code Quality Reviewer · 30.6 AIC · ⌖ 4.48 AIC · ⊞ 5.4K
Comment /review to run again
| {"only whitespace lines", " \n \n", "\n"}, | ||
| {"content-line trailing spaces trimmed", "key: value \n", "key: value\n"}, | ||
| {"content-line trailing tabs trimmed", "a\t\nb\n", "a\nb\n"}, | ||
| {"interior trailing spaces preserved content", " foo: bar \n", " foo: bar\n"}, |
There was a problem hiding this comment.
No round-trip test validates that block-scalar content survives normalization. All new test cases use abstract strings — none verify that a real YAML document with a literal block scalar (e.g. run: |) emerges with an unchanged parsed scalar value.
💡 Suggested fix
Add a table-driven case that constructs a YAML string with a literal block scalar containing trailing spaces and 3+ consecutive blank lines, passes it through normalizeBlankLines, parses both the original and result with a YAML library (e.g. gopkg.in/yaml.v3), and asserts the scalar value is bit-for-bit identical. Without this, the correctness claim in the comment at compiler_yaml.go:318 is structurally untested and any regression in block-scalar handling will go undetected.
// Example skeleton:
input := "jobs:\n build:\n steps:\n - run: |\n echo hello \n\n\n\n echo world\n"
output := normalizeBlankLines(input)
// Parse both with yaml.v3 and compare the run: scalar value
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (6 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please run the
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 44ab9c3. The
Fixed in 44ab9c3. |
pelikhan
deleted the
fix/yamllint-trailing-spaces-blank-lines-886438b1b9b48448
branch
Uh oh!
There was an error while loading. Please reload this page.