[yamllint-fixer] Reduce yamllint trailing-spaces noise in generated lock files#43580
Conversation
Reduces yamllint trailing-spaces noise in generated *.lock.yml files by fixing two root causes in the Go generator: - commentOutProcessedFieldsInOnSection emitted "# " (hash + space) when commenting out blank lines inside multi-line blocks (e.g. an empty line in a commented-out steps script). TrimRight the commented line. - indentYAMLLines re-emitted whitespace-only lines verbatim, carrying the surrounding indentation as trailing whitespace. Emit a bare newline. trailing-spaces warnings drop from 51 to 12 (remaining are content inside user-authored jq/prompt blocks, not machine-generated). 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 #43580 does not have the 'implementation' label and has only 10 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR reduces yamllint trailing-whitespace warnings in generated *.lock.yml workflow files by normalizing how the YAML generator emits blank / whitespace-only lines during indentation and when commenting out processed on: fields.
Changes:
- Normalize whitespace-only lines in
indentYAMLLinesto emit a bare newline (avoids indentation becoming trailing whitespace). - Trim trailing spaces/tabs when constructing commented-out
on:lines, preventing#on empty lines inside multi-line blocks. - Update
TestIndentYAMLLinesexpectations to reflect the new blank-line normalization behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/frontmatter_extraction_yaml.go | Normalizes blank/whitespace-only line emission to avoid trailing whitespace in generated YAML and commented-out on: fields. |
| pkg/workflow/frontmatter_extraction_yaml_test.go | Adjusts unit test expectations to match the new blank-line behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes are correct, minimal, and well-tested. Both fixes cleanly address the root causes of trailing-whitespace generation:\n\n- indentYAMLLines: the else branch only fires for blank/whitespace-only lines, so non-blank content is unaffected.\n- commentOutProcessedFieldsInOnSection: strings.TrimRight safely removes trailing spaces from composed comment lines without altering content in the middle of a line.\n\nThe test update accurately reflects the new normalization behavior. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 13.2 AIC · ⌖ 5.91 AIC · ⊞ 4.9K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (1 test)
Modified Test Cases:
Verdict
Summary: The test modification updates both the description and expected output to validate the corrected behavior—blank lines in indented YAML are now emitted without trailing whitespace, fixing yamllint's 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.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 85/100. 0% implementation tests (threshold: 30%). No violations. The test modification is high-value: it validates the critical behavioral fix that blank lines in indented YAML are emitted without trailing whitespace, ensuring yamllint compliance.
Skills-Based Review 🧠Applied 📋 Key Themes & HighlightsKey Themes
Positive Highlights
@copilot please address the review comments above.
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 34.9 AIC · ⌖ 6.56 AIC · ⊞ 6.7K
Comment /matt to run again
| // TrimRight avoids emitting trailing whitespace when commenting out | ||
| // blank lines inside multi-line blocks (e.g. "# " on an empty script line), | ||
| // which yamllint flags as trailing-spaces. | ||
| commentedLine := strings.TrimRight(indentation+"# "+trimmed+commentReason, " \t") |
There was a problem hiding this comment.
[/tdd] The commentOutProcessedFieldsInOnSection fix drives the bulk of the improvement (41→2 on issue-monster) but there's no unit test exercising the blank-line-in-multi-line-block scenario to prevent regression.
💡 Suggested test skeleton
Add a case to an existing or new test for commentOutProcessedFieldsInOnSection that verifies blank lines inside a commented-out multi-line block don't produce trailing spaces:
// input: on-section YAML with a steps field containing a blank line mid-script
// expected: commented-out blank lines become bare "#" (no trailing space)
input := "on:\n push:\n steps: |\n echo hi\n\n echo bye\n"
// After commentOutProcessedFieldsInOnSection, blank lines should yield "#" not "# "Without this, a future refactor of the comment-building logic could silently reintroduce trailing spaces on blank lines.
@copilot please address this.
There was a problem hiding this comment.
No blocking issues — both changes are mechanically correct.
The blank-line normalization is safe: YAML parsers treat a whitespace-only line identically to a bare blank line inside block scalars, and all callers pass structured YAML (not raw block scalar content with significant inner spacing).
**The in ** is correct: every constant ends with a non-whitespace character, so will never silently truncate comment text. The fix correctly converts the blank-line case # → #.
One non-blocking observation below.
🔎 Code quality review by PR Code Quality Reviewer · 191.5 AIC · ⌖ 6.5 AIC · ⊞ 5.4K
Comment /review to run again
| indent: " ", | ||
| wantOut: "first: value\n\n second: value\n \n third: value", | ||
| wantOut: "first: value\n\n second: value\n\n third: value", | ||
| }, |
There was a problem hiding this comment.
Missing test for the primary fix (commentOutProcessedFieldsInOnSection blank-line case): the PR correctly adds a blank-line normalization test for indentYAMLLines, but the higher-impact fix — which produces the 39-warning reduction — has no corresponding unit test asserting that a blank line inside a commented-out block yields bare # rather than # .
💡 Suggested addition
Add a case to TestCommentOutProcessedFieldsInOnSection (in compiler_yaml_test.go):
{
name: "blank line inside on.steps emits bare # not # ",
input: "on:\n steps:\n - name: test\n run: |\n echo hello\n\n echo world\n",
// blank line inside steps block should become "#" not "# "
mustNotContain: []string{"# \n"},
mustContain: []string{"#\n"},
},This mirrors what the PR does for TestIndentYAMLLines and protects the main regression target from regressing silently.
| // TrimRight avoids emitting trailing whitespace when commenting out | ||
| // blank lines inside multi-line blocks (e.g. "# " on an empty script line), | ||
| // which yamllint flags as trailing-spaces. | ||
| commentedLine := strings.TrimRight(indentation+"# "+trimmed+commentReason, " \t") |
There was a problem hiding this comment.
Latent regression trap: strings.TrimRight is applied to the fully-assembled indentation + "# " + trimmed + commentReason string — not just to the blank-line fragment. Today this is harmless because every commentReason constant ends with a letter. But if any future commentReason is accidentally written with a trailing space or tab, TrimRight will silently truncate it with no compile-time or test-time signal.
💡 Safer alternative
Apply the trim only when the line itself is blank (trimmed == ""), making the intent explicit and leaving commentReason untouched:
commentedLine := indentation + "# " + trimmed + commentReason
if trimmed == "" {
commentedLine = strings.TrimRight(commentedLine, " \t")
}
result = append(result, commentedLine)This removes the implicit coupling between the trim and the commentReason value.
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 3816286. The branch was already up to date with |
|
@copilot please run the
|
No further code changes were needed beyond 3816286. I rechecked the branch against |
|
@copilot please run the skill, address unresolved review comments, and rerun checks once the branch is up to date.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
pelikhan
deleted the
fix-yamllint-trailing-spaces-generator-ebb9dc5c22330b13
branch
|
🎉 This pull request is included in a new release. Release: |
Uh oh!
There was an error while loading. Please reload this page.