[yamllint-fixer] fix: trim trailing whitespace in generated YAML to reduce yamllint noise - #45620
Conversation
Reduces yamllint noise in generated *.lock.yml files by trimming trailing whitespace from script and prompt body lines at the YAML emission points in the generator, rather than carrying source-file trailing whitespace verbatim. Fixes four emission paths: - ConvertStepToYAML (on.steps / pre-activation job steps) - writeStepsSection (pre/post steps; previously computed a trimmed value but wrote the untrimmed line) - addCustomStepsAsIs / addCustomStepsWithRuntimeInsertion (custom steps) - generateUnifiedPromptCreationStep (built-in sections + user prompt chunks) Eliminates all 12 trailing-spaces warnings across the workflow set (16 -> 4). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45620 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (25 additions detected, threshold is 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![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 consistent and correct. Trimming trailing spaces/tabs before writing each line to the YAML builder is safe across all code paths — yamllint treats trailing whitespace as noise regardless of block-scalar context. The golden-file update confirms the output is clean.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 10.4 AIC · ⌖ 4.9 AIC · ⊞ 5K
There was a problem hiding this comment.
Pull request overview
Attempts to reduce yamllint warnings in generated lock files by trimming trailing whitespace during YAML compilation.
Changes:
- Trims generated step and custom-step lines.
- Trims built-in and user prompt heredocs.
- Updates the WASM golden fixture.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_yaml.go |
Trims pre/post-step output. |
pkg/workflow/compiler_yaml_runtime_setup.go |
Trims custom-step output. |
pkg/workflow/compiler_yaml_step_conversion.go |
Trims converted step YAML. |
pkg/workflow/unified_prompt_step.go |
Trims prompt content lines. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden |
Updates expected generated output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/5 changed files
- Comments generated: 8
- Review effort level: Medium
| // Trim trailing whitespace so script/block-scalar body lines carried | ||
| // verbatim from the source don't emit yamllint trailing-spaces warnings. | ||
| result.WriteString(" " + strings.TrimRight(line, " \t") + "\n") |
| if strings.HasPrefix(trimmed, " ") { | ||
| yaml.WriteString(" " + trimmed[2:] + "\n") | ||
| } else { | ||
| yaml.WriteString(" " + line + "\n") | ||
| yaml.WriteString(" " + trimmed + "\n") |
| // Simply add 6 spaces for job context indentation. | ||
| // Trim trailing whitespace so script body lines carried verbatim from the | ||
| // source markdown don't emit yamllint trailing-spaces warnings in the lock file. | ||
| yaml.WriteString(" " + strings.TrimRight(line, " \t") + "\n") |
| // Add the line with proper indentation, trimming trailing whitespace so | ||
| // script body lines don't emit yamllint trailing-spaces warnings. | ||
| yaml.WriteString(" " + strings.TrimRight(line, " \t") + "\n") |
| yaml.WriteString("\n") | ||
| } else { | ||
| yaml.WriteString(" " + nextLine + "\n") | ||
| yaml.WriteString(" " + strings.TrimRight(nextLine, " \t") + "\n") |
| // Trim trailing whitespace so prompt lines don't emit yamllint | ||
| // trailing-spaces warnings in the lock file. | ||
| yaml.WriteString(strings.TrimRight(line, " \t")) |
| contentLines := strings.SplitSeq(cleanedContent, "\n") | ||
| for line := range contentLines { | ||
| yaml.WriteString(" " + line + "\n") | ||
| yaml.WriteString(" " + strings.TrimRight(line, " \t") + "\n") |
| contentLines := strings.SplitSeq(cleanedContent, "\n") | ||
| for line := range contentLines { | ||
| yaml.WriteString(" " + line + "\n") | ||
| yaml.WriteString(" " + strings.TrimRight(line, " \t") + "\n") |
There was a problem hiding this comment.
Clean, focused fix. No blocking issues.
The most notable correction is in writeStepsSection (compiler_yaml.go): the original code computed trimmed but then emitted the untrimmed line — a pre-existing logic bug that this PR fixes correctly. All other changes are consistent applications of strings.TrimRight(line, " \t") at YAML emission points where trailing whitespace is never semantically meaningful.
🔎 Code quality review by PR Code Quality Reviewer · 9.95 AIC · ⌖ 4.28 AIC · ⊞ 5.6K
Comment /review to run again
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 25 AIC · ⌖ 4.37 AIC · ⊞ 6.7K
Comment /matt to run again
| for _, line := range lines { | ||
| if strings.TrimSpace(line) == "" { | ||
| result.WriteString("\n") | ||
| } else { |
There was a problem hiding this comment.
[/tdd] No unit test directly asserts that trailing whitespace is stripped from emitted lines — coverage relies solely on the golden fixture for smoke-copilot.
💡 Suggested test addition
A small table-driven test in compiler_yaml_step_conversion_test.go (or equivalent) would pin this invariant:
func TestConvertStepToYAML_TrimsTrailingWhitespace(t *testing.T) {
stepMap := map[string]any{
"run": "echo hello \n indented ",
}
got, err := ConvertStepToYAML(stepMap)
require.NoError(t, err)
for _, line := range strings.Split(got, "\n") {
assert.Equal(t, strings.TrimRight(line, " \t"), line,
"emitted line must have no trailing whitespace")
}
}This is a fast, self-contained regression guard that doesn't require recompiling all 256 workflows.
@copilot please address this.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Per reviewer feedback, trimming block-scalar content changes parsed values: - run/script bodies (\\ is a shell line continuation) - custom step with:/run blocks (heredoc payloads, multiline data) - prompt heredocs (two trailing spaces = Markdown hard break) Reverts all trimming changes introduced in abf2f9e and restores the smoke-copilot.golden fixture to its original state. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all eight review threads. All trimming changes have been reverted — block-scalar bodies ( |
|
🎉 This pull request is included in a new release. Release: |
Summary
Reduces yamllint noise in generated
*.lock.ymlfiles by fixing root causes in the GoYAML generator code. Source-file trailing whitespace (in embedded shell/jq/JS scripts and
prompt text) was carried verbatim into the compiled lock files, producing
trailing-spaceswarnings. The generator now trims trailing whitespace at the YAML emission points.
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
All 12
trailing-spaceswarnings are eliminated. The remainingcomments-indentation(3)and
empty-lines(1) warnings were intentionally left untouched — fixing them safely requiresriskier structural changes (re-indenting generated
on:-block comments and collapsing blanklines inside user prompt content, which could alter fenced code blocks). Those are better
handled as separate, focused changes.
Changes
All changes trim trailing whitespace (
\t) from each content line as it is written intothe lock file. Trailing whitespace is never semantically meaningful in the emitted contexts
(YAML keys/values,
run:/script:block-scalar shell/JS bodies, and LLM prompt text), andthere is existing precedent for this in
header.go.pkg/workflow/compiler_yaml_step_conversion.go—ConvertStepToYAML(shared by allengines; covers
on.steps/ pre-activation job steps such as thegithub-scriptbodies inissue-monster).pkg/workflow/compiler_yaml.go—writeStepsSectionalready computed a trimmed valuebut then wrote the untrimmed line; now emits the trimmed line (covers pre/post steps).
pkg/workflow/compiler_yaml_runtime_setup.go—addCustomStepsAsIsandaddCustomStepsWithRuntimeInsertion(customsteps:from shared includes, e.g. the jqscripts in
dataflow-pr-discussion-datasetanddeep-report).pkg/workflow/unified_prompt_step.go—generateUnifiedPromptCreationStepbuilt-insections and user prompt chunks (e.g. markdown prompt lines in
smoke-claude).pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden—regenerated golden fixture (2 lines: trailing spaces removed).
Verified:
go build ./...passes,go test ./pkg/workflow/passes, andgh aw compilerecompiles all 256 workflows with 0 errors.
Notes
.lock.ymlfiles are not included in this PR.Run
gh aw compileafter merging to regenerate them with the improvements.