Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/release.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 4 additions & 7 deletions .github/workflows/smoke-claude.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -313,13 +313,13 @@ jobs:

1. **GitHub MCP Testing**: Review the last 2 merged pull requests in __GH_AW_GITHUB_REPOSITORY__
2. **MCP Scripts GH CLI Testing**: Use the `mcpscripts-gh` tool to query 2 pull requests from __GH_AW_GITHUB_REPOSITORY__ (use args: "pr list --repo __GH_AW_GITHUB_REPOSITORY__ --limit 2 --json number,title,author")
3. **Serena MCP Testing**:
3. **Serena MCP Testing**:
- Use the Serena MCP server tool `activate_project` to initialize the workspace at `__GH_AW_GITHUB_WORKSPACE__` and verify it succeeds (do NOT use bash to run go commands - use Serena's MCP tools)
- After initialization, use the `find_symbol` tool to search for symbols (find which tool to call) and verify that at least 3 symbols are found in the results
4. **Playwright Testing**: Use the playwright tools to navigate to <https://github.com> and verify the page title contains "GitHub" (do NOT try to install playwright - use the provided MCP tools)
5. **File Writing Testing**: Create a test file `/tmp/gh-aw/agent/smoke-test-copilot-__GH_AW_GITHUB_RUN_ID__.txt` with content "Smoke test passed for Copilot at $(date)" (create the directory if it doesn't exist)
6. **Bash Tool Testing**: Execute bash commands to verify file creation was successful (use `cat` to read the file back)
7. **Discussion Interaction Testing**:
7. **Discussion Interaction Testing**:
- Use the `github-discussion-query` mcp-script tool with params: `limit=1, jq=".[0]"` to get the latest discussion from __GH_AW_GITHUB_REPOSITORY__
- Extract the discussion number from the result (e.g., if the result is `{"number": 123, "title": "...", ...}`, extract 123)
- Use the `add_comment` tool with `discussion_number: <extracted_number>` to add a fun, playful comment stating that the smoke test agent was here
Expand Down
25 changes: 24 additions & 1 deletion pkg/workflow/unified_prompt_step.go
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,16 @@ func (c *Compiler) generateUnifiedPromptCreationStep(yaml *strings.Builder, buil
// 2. Write user prompt chunks (appended after built-in sections).
// All chunks are written into the same heredoc block (opened above or here)
// to minimise the number of delimiter lines in the compiled lock file.
//
// The heredoc payload is a YAML block scalar, so normalizeBlankLines preserves
// it verbatim (it must, since arbitrary block scalars can carry semantically
// significant trailing whitespace and blank runs). Prompt content is markdown
// text the compiler owns, where trailing whitespace is never meaningful and
// long blank runs are noise, so it is cleaned here instead: trailing whitespace
// is trimmed from every line and consecutive blank lines are capped at
// maxConsecutiveBlankLines. userBlankRun is tracked across chunks so a run that
// straddles a chunk boundary is still collapsed.
userBlankRun := 0
for chunkIdx, chunk := range userPromptChunks {
unifiedPromptLog.Printf("Writing user prompt chunk %d/%d", chunkIdx+1, len(userPromptChunks))

Expand All @@ -472,6 +482,7 @@ func (c *Compiler) generateUnifiedPromptCreationStep(yaml *strings.Builder, buil
inHeredoc = true
}
yaml.WriteString(" " + chunk + "\n")
userBlankRun = 0
continue
}

Expand All @@ -483,8 +494,20 @@ func (c *Compiler) generateUnifiedPromptCreationStep(yaml *strings.Builder, buil

lines := strings.SplitSeq(chunk, "\n")
for line := range lines {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Trailing spaces/tabs stripped from content lines
  2. Exactly maxConsecutiveBlankLines blanks preserved, +1 collapsed
  3. A blank run straddling two adjacent chunks (counter persists across boundary)
  4. 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 75c338epkg/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.

trimmed := strings.TrimRight(line, " \t")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 trimmed == "" {
// Collapse over-long blank runs; emit truly empty lines (no
// indentation) so they carry no trailing whitespace.
if userBlankRun >= maxConsecutiveBlankLines {
continue
}
userBlankRun++
Comment on lines +501 to +504

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 75c338epkg/workflow/unified_prompt_step_test.go now has four focused tests:

  • TestGenerateUnifiedPromptCreationStep_TrailingWhitespace — verifies per-line trimming
  • TestGenerateUnifiedPromptCreationStep_BlankRunCapWithinChunk — caps 4 consecutive blank lines in one chunk to 2
  • TestGenerateUnifiedPromptCreationStep_BlankRunCapAcrossChunks — verifies the counter persists across chunk boundaries
  • TestGenerateUnifiedPromptCreationStep_BlankRunCapAdjacentToRuntimeImport — verifies correct handling when a runtime-import macro separates two blank-heavy chunks

yaml.WriteByte('\n')
continue
}
userBlankRun = 0
yaml.WriteString(" ")
yaml.WriteString(line)
yaml.WriteString(trimmed)
yaml.WriteByte('\n')
}
}
Expand Down
112 changes: 112 additions & 0 deletions pkg/workflow/unified_prompt_step_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -606,3 +606,115 @@ func TestCollectPromptSections_PRCommentPushToPRBranchGuidance(t *testing.T) {
}
})
}

// TestGenerateUnifiedPromptCreationStep_TrailingWhitespace verifies that trailing
// whitespace is stripped from every user-prompt content line.
func TestGenerateUnifiedPromptCreationStep_TrailingWhitespace(t *testing.T) {
compiler := &Compiler{}
data := &WorkflowData{
ParsedTools: NewTools(map[string]any{}),
}
builtinSections := compiler.collectPromptSections(data)

// Chunk lines with assorted trailing whitespace
userPromptChunks := []string{"line one \nline two\t\nline three \t "}

var sb strings.Builder
compiler.generateUnifiedPromptCreationStep(&sb, builtinSections, userPromptChunks, nil, data)
output := sb.String()

assert.NotContains(t, output, "line one ", "trailing spaces should be stripped")
assert.NotContains(t, output, "line two\t", "trailing tab should be stripped")
assert.NotContains(t, output, "line three \t ", "mixed trailing whitespace should be stripped")
assert.Contains(t, output, " line one\n", "content should be present with indentation")
assert.Contains(t, output, " line two\n", "content should be present with indentation")
assert.Contains(t, output, " line three\n", "content should be present with indentation")
}

// TestGenerateUnifiedPromptCreationStep_BlankRunCapWithinChunk verifies that a run of
// more than maxConsecutiveBlankLines blank lines inside a single chunk is capped.
func TestGenerateUnifiedPromptCreationStep_BlankRunCapWithinChunk(t *testing.T) {
compiler := &Compiler{}
data := &WorkflowData{
ParsedTools: NewTools(map[string]any{}),
}
builtinSections := compiler.collectPromptSections(data)

// Four consecutive blank lines between content lines – should be collapsed to two.
userPromptChunks := []string{"# Start\n\n\n\n\n# End"}

var sb strings.Builder
compiler.generateUnifiedPromptCreationStep(&sb, builtinSections, userPromptChunks, nil, data)
output := sb.String()

assert.Contains(t, output, "# Start", "start marker should be present")
assert.Contains(t, output, "# End", "end marker should be present")

// At most maxConsecutiveBlankLines (2) blank lines should appear between them.
startIdx := strings.Index(output, "# Start")
endIdx := strings.Index(output, "# End")
require.Less(t, startIdx, endIdx)
between := output[startIdx:endIdx]
// Three or more consecutive newlines would indicate a blank run of >2.
assert.NotContains(t, between, "\n\n\n\n", "blank run should be capped at 2")
}

// TestGenerateUnifiedPromptCreationStep_BlankRunCapAcrossChunks verifies that a blank
// run straddling two adjacent chunks is still capped correctly.
func TestGenerateUnifiedPromptCreationStep_BlankRunCapAcrossChunks(t *testing.T) {
compiler := &Compiler{}
data := &WorkflowData{
ParsedTools: NewTools(map[string]any{}),
}
builtinSections := compiler.collectPromptSections(data)

// chunk1 ends with two blank lines; chunk2 starts with two blank lines.
// Across the boundary that would be four consecutive blanks; they should be collapsed.
userPromptChunks := []string{
"# Chunk1\n\n\n",
"\n\n# Chunk2",
}

var sb strings.Builder
compiler.generateUnifiedPromptCreationStep(&sb, builtinSections, userPromptChunks, nil, data)
output := sb.String()

startIdx := strings.Index(output, "# Chunk1")
endIdx := strings.Index(output, "# Chunk2")
require.Less(t, startIdx, endIdx)
between := output[startIdx:endIdx]
assert.NotContains(t, between, "\n\n\n\n", "blank run across chunk boundary should be capped at 2")
}

// TestGenerateUnifiedPromptCreationStep_BlankRunCapAdjacentToRuntimeImport verifies
// that blank-run tracking resets correctly around a runtime-import macro so blank lines
// on both sides of the macro are handled independently.
func TestGenerateUnifiedPromptCreationStep_BlankRunCapAdjacentToRuntimeImport(t *testing.T) {
compiler := &Compiler{}
data := &WorkflowData{
ParsedTools: NewTools(map[string]any{}),
}
builtinSections := compiler.collectPromptSections(data)

// chunk[0] ends with a long blank run, chunk[1] is a runtime-import macro,
// chunk[2] starts with a long blank run.
userPromptChunks := []string{
"# Before\n\n\n\n",
"{{#runtime-import docs/task.md}}",
"\n\n\n\n# After",
}

var sb strings.Builder
compiler.generateUnifiedPromptCreationStep(&sb, builtinSections, userPromptChunks, nil, data)
output := sb.String()

// Runtime-import macro must be present verbatim.
assert.Contains(t, output, "{{#runtime-import docs/task.md}}", "runtime-import macro must be emitted verbatim")

// Content markers must appear.
assert.Contains(t, output, "# Before", "before-marker should be present")
assert.Contains(t, output, "# After", "after-marker should be present")

// No run of four or more newlines (i.e., 3+ consecutive blank lines) anywhere.
assert.NotContains(t, output, "\n\n\n\n", "blank run should be capped throughout the output")
}
Loading