[yamllint-fixer] Reduce yamllint trailing-spaces noise in generated lock files#43580
[yamllint-fixer] Reduce yamllint trailing-spaces noise in generated lock files#43580github-actions[bot] wants to merge 4 commits into
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
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 |
Summary
Reduces yamllint noise in generated
*.lock.ymlfiles by fixing root causes in the GoYAML generator code.
yamllint config used:
{extends: default, rules: {line-length: disable, document-start: disable, truthy: {check-keys: false}, comments: {require-starting-space: true, min-spaces-from-content: 1}}}Results
Warning breakdown
Changes
pkg/workflow/frontmatter_extraction_yaml.gocommentOutProcessedFieldsInOnSection: when commenting out processed frontmatterfields in the
on:section, blank lines inside multi-line blocks (e.g. an empty linewithin a commented-out
steps:script) were emitted as#— a hash followed by atrailing space. The commented line is now
strings.TrimRight-trimmed, so blank linesbecome a bare
#. This accounts for the entire reduction (issue-monster alone droppedfrom 41 trailing-spaces to 2).
indentYAMLLines: whitespace-only lines were re-emitted verbatim, carrying thesurrounding indentation as trailing whitespace. They now emit a bare newline. (Defensive;
no committed lock file relied on the old behavior.)
pkg/workflow/frontmatter_extraction_yaml_test.go: updatedTestIndentYAMLLinestoassert the new blank-line normalization.
The remaining 12 trailing-spaces warnings are trailing whitespace inside user-authored
content (jq scripts, agent prompt text) carried through literal block scalars — trimming
those in the generator would alter user content, so they are intentionally left alone.
The larger
comments-indentationandindentationcategories stem fromgoccy/go-yamlnot indenting sequences (yamllint's
indent-sequences: truedefault) in theon:block.Fixing those requires a broad marshaling change that reformats every sequence in every lock
file, so it is deliberately out of scope for this surgical PR.
Notes
.lock.ymlfiles are not included in this PR.Run
gh aw compileafter merging to regenerate them with the improvements.Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
models.devSee Network Configuration for more information.