[yamllint-fixer] Trim trailing whitespace and cap blank runs in inlined prompt content#46123
Conversation
The unified prompt creation step embeds markdown prompt content inside a `run: |` heredoc, which is a YAML block scalar. normalizeBlankLines deliberately preserves block-scalar payload verbatim (arbitrary scalars can carry semantically significant trailing whitespace and blank runs, e.g. shell line continuations), so trailing whitespace on prompt content lines and long blank runs survived into inlined lock files as yamllint trailing-spaces / empty-lines noise. Prompt content is markdown text the compiler owns, where trailing whitespace is never meaningful and long blank runs are just noise, so it is now cleaned at emission: trailing whitespace is trimmed per line and consecutive blank lines are capped at maxConsecutiveBlankLines, tracked across chunk boundaries. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ 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 analysis skipped. The PR only modifies production code (unified_prompt_step.go) and a testdata golden file. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46123 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (26 additions detected, threshold is 100). |
There was a problem hiding this comment.
Pull request overview
Normalizes inlined prompt content to reduce yamllint warnings in generated workflow lock files.
Changes:
- Trims trailing whitespace and caps consecutive blank lines across chunks.
- Updates the Copilot smoke-test golden output.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/unified_prompt_step.go |
Adds prompt-content normalization during YAML emission. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden |
Reflects removed trailing spaces. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/2 changed files
- Comments generated: 2
- Review effort level: Medium
|
|
||
| lines := strings.SplitSeq(chunk, "\n") | ||
| for line := range lines { | ||
| trimmed := strings.TrimRight(line, " \t") |
There was a problem hiding this comment.
These prompts are consumed by LLMs as plaintext strings, not rendered as HTML/Markdown. In that consumption context, two trailing spaces have no semantic effect — the agent receives raw text and acts on it directly, not via a Markdown renderer. Trimming is therefore safe for all user-prompt content, including imported workflow Markdown.
The PR description explicitly scopes this to prompt content (the heredoc payload of the generateUnifiedPromptCreationStep function) rather than block scalars in steps: bodies. The latter remain guarded by normalizeBlankLines' verbatim block-scalar preservation and TestNormalizeBlankLinesPreservesBlockScalarContent.
| if userBlankRun >= maxConsecutiveBlankLines { | ||
| continue | ||
| } | ||
| userBlankRun++ |
There was a problem hiding this comment.
Added in 75c338e — pkg/workflow/unified_prompt_step_test.go now has four focused tests:
TestGenerateUnifiedPromptCreationStep_TrailingWhitespace— verifies per-line trimmingTestGenerateUnifiedPromptCreationStep_BlankRunCapWithinChunk— caps 4 consecutive blank lines in one chunk to 2TestGenerateUnifiedPromptCreationStep_BlankRunCapAcrossChunks— verifies the counter persists across chunk boundariesTestGenerateUnifiedPromptCreationStep_BlankRunCapAdjacentToRuntimeImport— verifies correct handling when a runtime-import macro separates two blank-heavy chunks
There was a problem hiding this comment.
Review: LGTM
The fix is correct and well-scoped.
unified_prompt_step.go: userBlankRun is correctly initialized before the chunk loop and reset on both the runtime-import path and the non-blank content path, so cross-chunk blank runs are properly collapsed. Blank lines inside the heredoc are emitted without indentation prefix (correct, bare newlines inside a block scalar carry no trailing whitespace). Reuses existing maxConsecutiveBlankLines constant.
smoke-copilot.golden: only two trailing spaces removed, consistent with the fix.
No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 20.7 AIC · ⌖ 4.29 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — one finding on test coverage.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Clean, surgical fix: trimming and blank-run capping are applied only to compiler-owned prompt content, with no risk to user-authored block scalars
- ✅ The
userBlankRuncounter is correctly tracked across chunk boundaries — the tricky cross-chunk edge case is handled - ✅ Comment in the code clearly explains why
normalizeBlankLinescannot do this job and why it is safe to do here - ✅ Golden fixture updated, PR description explains the before/after metric table
One Issue
- Missing unit test (see inline comment): the new trimming and blank-capping logic in
generateUnifiedPromptCreationStephas no direct test. The golden fixture provides implicit coverage but won't catch a targeted regression in this code path.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 29.5 AIC · ⌖ 4.52 AIC · ⊞ 6.7K
Comment /matt to run again
| @@ -483,8 +494,20 @@ func (c *Compiler) generateUnifiedPromptCreationStep(yaml *strings.Builder, buil | |||
|
|
|||
| lines := strings.SplitSeq(chunk, "\n") | |||
| for line := range lines { | |||
There was a problem hiding this comment.
[/tdd] No test covers the new trimming and blank-line-capping behaviour — a future refactor could silently regress both properties.
💡 Suggested test skeleton
func TestGenerateUnifiedPromptCreationStep_TrimsTrailingWhitespace(t *testing.T) {
compiler := newTestCompiler()
var yaml strings.Builder
chunk := "line with trailing space \n\n\n\nexcess blank lines\nnormal line"
compiler.generateUnifiedPromptCreationStep(&yaml, nil, []string{chunk}, nil, testData())
out := yaml.String()
for _, line := range strings.Split(out, "\n") {
if strings.TrimRight(line, " \t") != line {
t.Errorf("trailing whitespace found: %q", line)
}
}
// at most maxConsecutiveBlankLines consecutive blank lines
blankRun := 0
for _, line := range strings.Split(out, "\n") {
if strings.TrimSpace(line) == "" {
blankRun++
if blankRun > maxConsecutiveBlankLines {
t.Errorf("blank run exceeds %d", maxConsecutiveBlankLines)
}
} else {
blankRun = 0
}
}
}Without this, the behaviour is only validated indirectly by the golden fixture update.
@copilot please address this.
There was a problem hiding this comment.
Added in 75c338e — see the four new tests in pkg/workflow/unified_prompt_step_test.go covering trailing-whitespace trimming, within-chunk blank-run capping, cross-chunk blank-run capping, and the runtime-import macro adjacency case.
There was a problem hiding this comment.
Review: COMMENT (non-blocking)
The implementation logic is correct — trimming trailing whitespace and capping blank runs at the emission site (rather than in normalizeBlankLines) is the right layering. One concern worth addressing before a future refactor:
Missing unit tests — the new trim-and-blank-run behavior has no dedicated test coverage; only golden-file output for two specific lines validates it. See the inline comment for specific scenarios to add.
🔎 Code quality review by PR Code Quality Reviewer · 42.8 AIC · ⌖ 4.38 AIC · ⊞ 5.6K
Comment /review to run again
| @@ -483,8 +494,20 @@ func (c *Compiler) generateUnifiedPromptCreationStep(yaml *strings.Builder, buil | |||
|
|
|||
| lines := strings.SplitSeq(chunk, "\n") | |||
| for line := range lines { | |||
There was a problem hiding this comment.
No dedicated unit tests for the new trim-and-blank-run logic: regressions will only surface through golden-file diffs that are easy to misread.
💡 Suggested additions
Add a test case to unified_prompt_creation_test.go covering at least:
- Trailing spaces/tabs stripped from content lines
- Exactly
maxConsecutiveBlankLinesblanks preserved,+1collapsed - A blank run straddling two adjacent chunks (counter persists across boundary)
- A runtime-import macro between two blank-heavy chunks (counter reset path)
The golden-file test only validates two specific trailing-space removals from the smoke fixture and does not cover any of these edge cases. A future refactor of the chunk loop could silently break the trimming logic or over-aggressively collapse blanks inside user prompts.
There was a problem hiding this comment.
Added in 75c338e — pkg/workflow/unified_prompt_step_test.go now has four tests covering all four of the cases listed: trailing spaces/tabs stripped, exactly maxConsecutiveBlankLines blanks preserved and +1 collapsed, blank run straddling two chunks, and a runtime-import macro between two blank-heavy chunks.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…user prompt chunks Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Summary
Fixes yamllint warnings in compiled lock files by cleaning user-prompt content during compilation: trailing whitespace is stripped from every line and runs of more than maxConsecutiveBlankLines consecutive blank lines are collapsed.
Problem
Inlined user-prompt chunks can contain trailing spaces/tabs and over-long blank runs. Because these chunks are written into a YAML heredoc block scalar, the existing normalizeBlankLines post-processor cannot touch them. The cleaning must happen at write time, inside generateUnifiedPromptCreationStep.
Changes
pkg/workflow/unified_prompt_step.go
pkg/workflow/unified_prompt_step_test.go (+112 lines)
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden
Scope
Testing
Four new unit tests cover the trimming and capping logic including cross-chunk and macro-adjacent edge cases. Existing golden-file test updated.