[yamllint-fixer] Reduce yamllint trailing-spaces noise in generated lock files#46699
Conversation
Custom steps (agent-job `steps:`) and pre-activation `on.steps` embed user-authored shell/jq/js content verbatim. When those source lines carry trailing whitespace, it survived into the generated `*.lock.yml`, which yamllint flags as trailing-spaces. Right-trim each content line in the three step-emission paths: - addCustomStepsAsIs / addCustomStepsWithRuntimeInsertion (agent job) - ConvertStepToYAML (on.steps in pre-activation job) - writeStepsSection (pre/post/pre-agent steps) Blank/whitespace-only lines were already collapsed to bare newlines; this extends the same treatment to trailing whitespace on content lines. Trailing whitespace is insignificant for the shell/script content these steps carry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46699 does not have the 'implementation' label and has only 22 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Reduces yamllint trailing-space warnings by trimming generated step lines.
Changes:
- Trims emitted custom and lifecycle step lines.
- Trims serialized
on.stepscontent.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_yaml_step_lifecycle.go |
Trims lifecycle step lines. |
pkg/workflow/compiler_yaml_step_conversion.go |
Trims converted step YAML. |
pkg/workflow/compiler_yaml_runtime_setup.go |
Trims custom runtime-step output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/workflow/compiler_yaml_runtime_setup.go:357
- The runtime-insertion path now mutates trailing whitespace inside custom step block scalars. This can change shell continuation behavior and the contents of heredocs or action
with:scripts, despite the source workflow being valid. Track whether the current line is structural YAML or block-scalar payload and preserve payload exactly rather than applyingTrimRightunconditionally.
// Add the line with proper indentation. Right-trim so trailing whitespace
// carried in from source step content doesn't survive into the generated
// YAML, which yamllint flags as trailing-spaces.
yaml.WriteString(" " + strings.TrimRight(line, " \t") + "\n")
pkg/workflow/compiler_yaml_runtime_setup.go:400
- Lines copied from the checkout step can also belong to block scalars (for example multiline
with.sparse-checkoutinput). Unconditionally trimming them changes the action input value. Apply the same block-scalar-aware preservation here as in the main custom-step loop; only structural YAML should be right-trimmed.
if nextTrimmed == "" {
yaml.WriteString("\n")
} else {
yaml.WriteString(" " + strings.TrimRight(nextLine, " \t") + "\n")
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Medium
| if strings.HasPrefix(trimmed, " ") { | ||
| yaml.WriteString(" " + trimmed[2:] + "\n") | ||
| } else { | ||
| yaml.WriteString(" " + line + "\n") | ||
| yaml.WriteString(" " + trimmed + "\n") |
| if trimmed := strings.TrimRight(line, " \t"); strings.TrimSpace(trimmed) == "" { | ||
| result.WriteString("\n") | ||
| } else { | ||
| result.WriteString(" " + line + "\n") | ||
| result.WriteString(" " + trimmed + "\n") |
| // Simply add 6 spaces for job context indentation. Right-trim so any | ||
| // trailing whitespace carried in from source step content (e.g. embedded | ||
| // shell/jq/js scripts) doesn't survive into the generated YAML, which | ||
| // yamllint flags as trailing-spaces. | ||
| yaml.WriteString(" " + strings.TrimRight(line, " \t") + "\n") |
🧪 Test Quality Sentinel Report✅ No Test Files Modified This PR ([yamllint-fixer] Reduce yamllint trailing-spaces noise in generated lock files) contains only production code changes to three files:
Since no test files were added or modified, the Test Quality Sentinel skipped analysis. No test quality assessment is needed for this change. 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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: trailing-spaces fix
The changes are correct and consistent. Each of the three emitter sites now right-trims content lines before writing them, eliminating the source of yamllint trailing-spaces warnings without affecting runtime behavior.
Minor inconsistency (non-blocking): compiler_yaml_step_lifecycle.go line 36 trims only " " (spaces), while compiler_yaml_runtime_setup.go and compiler_yaml_step_conversion.go both trim " \t" (spaces and tabs). Tabs as trailing whitespace in embedded scripts are unlikely but the inconsistency is worth aligning in a follow-up.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 11.4 AIC · ⌖ 4.34 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — one inconsistency to address before merging.
📋 Key Themes & Highlights
Issues Found
- Tab vs space inconsistency:
compiler_yaml_step_lifecycle.goline 36 trims only spaces (" "), while the other two changed files trim both spaces and tabs (" \t"). Tabs in embedded content lines would still trigger yamllinttrailing-spaces.
Positive Highlights
- ✅ Clear, accurate root-cause analysis in PR description
- ✅ Latent bug in
writeStepsSection(trimmed but then writing untrimmed) correctly identified and fixed - ✅ Comments are concise and explain why trimming is safe for shell/script content
- ✅ The
TrimRightapproach (preserving leading whitespace/indentation) is exactly right —TrimSpacewould break YAML structure
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 20.5 AIC · ⌖ 4.5 AIC · ⊞ 6.7K
Comment /matt to run again
| // Emit the right-trimmed line so trailing whitespace carried in from the | ||
| // source step content (e.g. embedded shell/jq/js scripts) doesn't survive | ||
| // into the generated YAML, which yamllint flags as trailing-spaces. | ||
| if strings.HasPrefix(trimmed, " ") { |
There was a problem hiding this comment.
[/diagnosing-bugs] The trimmed variable at line 36 is defined as strings.TrimRight(line, " ") — trimming spaces only — while the other two changed files use strings.TrimRight(line, " \t"). Any trailing tab character on a content line will survive into the generated YAML and still trigger a yamllint trailing-spaces warning.
💡 Suggested fix
Change line 36 from:
trimmed := strings.TrimRight(line, " ")to:
trimmed := strings.TrimRight(line, " \t")This makes the trim consistent with compiler_yaml_runtime_setup.go and compiler_yaml_step_conversion.go.
@copilot please address this.
|
@copilot run pr-finisher skill |
|
@copilot create/find and reuse helper to write / trim string in yaml |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The three emission functions trimmed trailing whitespace from every line including literal/folded block-scalar payloads (run:, script: bodies). This is semantically unsafe: a backslash followed by spaces becomes a bare backslash (line continuation), heredoc indentation can shift, and template literal content can change. The normalizeBlankLines pass already preserves block-scalar content byte-for-byte (documented invariant in compiler_yaml_normalize.go). Trimming at the emission point conflicts with that contract. The 8 trailing-spaces yamllint warnings that prompted this PR all originate from user-authored block-scalar content; they are accepted as a known limitation rather than risking a semantic change. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reverted all three emission-level trimming changes in commit |
|
@copilot yamllint issues in .md files when applicable |
…ross emitters Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… emitters
Introduces yamlBlockScalarState (update method) and appendYAMLLine in a new
compiler_yaml_line_writer.go so that the logic for "trim structural lines,
preserve block-scalar payload verbatim" lives in one place and is reused by:
- writeStepsSection (compiler_yaml_step_lifecycle.go)
- ConvertStepToYAML (compiler_yaml_step_conversion.go)
- addCustomStepsAsIs + addCustomStepsWithRuntimeInsertion
(compiler_yaml_runtime_setup.go)
The helper trims trailing whitespace from structural YAML lines (fixing the
yamllint trailing-spaces warnings) while writing block-scalar payload lines
verbatim so that semantically significant whitespace — shell backslash-newline
continuations, heredoc indentation, template literal spacing — is preserved.
Blank lines are always emitted as bare newlines regardless of block-scalar
state to avoid trailing-space violations in the generated YAML.
The yamlBlockScalarState.update method reuses the blockScalarHeaderIndentForLine
and countLeadingSpaces helpers from compiler_yaml_normalize.go, applying the
same detection logic that normalizeBlankLines already uses.
Tests added in compiler_yaml_test.go: TestYamlBlockScalarStateUpdate (6
sub-cases) and TestAppendYAMLLine (3 sub-cases).
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commits
|
|
@copilot recompile, add tests |
… warnings in lock files Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit Fixed trailing whitespace directly in the source
The affected lock files were recompiled. yamllint now reports 1 warning (down from 9) — only the known |
…uralTrailingSpaces Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ll continuation risk Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commits Tests added (both in
Recompile: |
|
Hey team 👋 — thanks for the yamllint trailing-spaces fix! The core technical change is solid: the new However, this PR mixes multiple concerns that would benefit from separation: 1. Diff Is Unfocused (6,132 lines across 34 files)The changes span three distinct goals:
Consider extracting non-yamllint-essential changes into a separate housekeeping PR. This would make the yamllint fix reviewable in isolation. 2. Test Coverage NotesThe new test file (136 lines) is good. Verify it covers:
What Would HelpThe technical direction is right—just needs tighter focus!
|
|
🎉 This pull request is included in a new release. Release: |
Summary
Reduces yamllint trailing-spaces noise in generated lock files by trimming trailing whitespace from structural YAML lines emitted by the compiler, while safely preserving trailing whitespace inside block scalar payloads where it is semantically significant.
Problem
The YAML compiler emitted lines verbatim from workflow Markdown sources, preserving any trailing whitespace. This caused yamllint trailing-spaces warnings in generated .lock.yml files. A naive trim-all approach is unsafe: trailing whitespace inside a block scalar run: script (backslash followed by spaces) must not be stripped, as it prevents the backslash from acting as a shell line-continuation character.
Changes
New: pkg/workflow/compiler_yaml_line_writer.go
Refactored emitters
All previously used ad-hoc yaml.WriteString patterns; now delegate to appendYAMLLine + yamlBlockScalarState.
Source file fixes
Tests: pkg/workflow/compiler_yaml_test.go
New table-driven tests: TestYamlBlockScalarStateUpdate, TestAppendYAMLLine, TestWriteStepsSection, TestAddCustomStepsAsIsTrimsStructuralTrailingSpaces.
Key invariant
Trailing whitespace after a backslash in a block-scalar run: script must not be trimmed: removing it would activate shell line-continuation, silently changing script behaviour.
Testing
All new tests are in pkg/workflow/compiler_yaml_test.go. Existing compiler tests continue to pass.