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
55 changes: 51 additions & 4 deletions pkg/workflow/compiler_yaml.go
Original file line number Diff line number Diff line change
Expand Up @@ -312,14 +312,61 @@ func (c *Compiler) generateWorkflowBody(yaml *strings.Builder, data *WorkflowDat
// clearing its whitespace does not change the parsed content. This removes the bulk
// of yamllint trailing-spaces and empty-lines noise from generated lock files without
// touching lines that carry real content.
//
// This implementation avoids strings.Split/strings.Join to reduce allocations: it scans
// the input byte-by-byte and builds the result with a single pre-allocated strings.Builder.
func normalizeBlankLines(yamlContent string) string {
lines := strings.Split(yamlContent, "\n")
for i, line := range lines {
var b strings.Builder
b.Grow(len(yamlContent))

// lastNonBlankEnd tracks the builder length immediately after writing the last
// non-blank line (including its trailing newline). It starts at 0 and is only
// advanced when a substantive line is written, so it stays 0 when all lines
// are whitespace-only or the input is empty. Every line — blank or not — still
// gets a '\n' written to b, so b.Len() and lastNonBlankEnd may diverge when
// there are trailing blank lines.
lastNonBlankEnd := 0
pos := 0
for pos < len(yamlContent) {
// Find the end of the current line.
end := strings.IndexByte(yamlContent[pos:], '\n')
var line string
if end == -1 {
line = yamlContent[pos:]
} else {
line = yamlContent[pos : pos+end]
}

if strings.TrimSpace(line) == "" {
lines[i] = ""
// Whitespace-only line: emit as a truly empty line.
b.WriteByte('\n')
// lastNonBlankEnd is NOT updated here so that trailing blank lines
// (including a blank final "line" produced by a file that ends with
// "\n\n" or by whitespace-only text after the last real line) are
// excluded from the returned slice.
} else {
b.WriteString(line)
b.WriteByte('\n')
lastNonBlankEnd = b.Len()
}

if end == -1 {
break
}
pos += end + 1
}

// When lastNonBlankEnd is still 0 there were no non-blank lines at all
// (empty input or all-whitespace). Return a single newline, which matches
// the original strings.TrimRight(…, "\n") + "\n" behaviour for that case.
// NOTE: b.String()[:0] must NOT be used here; the early return is intentional.
if lastNonBlankEnd == 0 {
return "\n"
}
return strings.TrimRight(strings.Join(lines, "\n"), "\n") + "\n"
// Slice the builder string to drop trailing blank lines. b.String() copies
// the builder's internal buffer into a new string once; the slice avoids a
// second copy that a separate strings.Builder trim would incur.
return b.String()[:lastNonBlankEnd]

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.

[/diagnosing-bugs] No regression tests for the rewritten normalizeBlankLines — a non-trivial rewrite with subtle edge-case handling (empty input, all-whitespace, no trailing newline) is exactly where a unit test suite pays off.

💡 Suggested test cases
func TestNormalizeBlankLines(t *testing.T) {
    tests := []struct {
        name  string
        input string
        want  string
    }{
        {"empty string", "", "\n"},
        {"single blank line", "\n", "\n"},
        {"all whitespace line", "   \n", "\n"},
        {"no trailing newline", "hello", "hello\n"},
        {"trailing blank lines stripped", "a\n\nb\n\n\n", "a\n\nb\n"},
        {"blank lines in middle preserved", "a\n\nb\n", "a\n\nb\n"},
        {"whitespace-only lines cleared", "a\n   \nb\n", "a\n\nb\n"},
    }
    for _, tc := range tests {
        t.Run(tc.name, func(t *testing.T) {
            got := normalizeBlankLines(tc.input)
            if got != tc.want {
                t.Errorf("got %q, want %q", got, tc.want)
            }
        })
    }
}

Without tests, a future refactor or Go version change could silently break the behaviour that was carefully preserved in the comments.

@copilot please address this.

}

func (c *Compiler) generateYAML(data *WorkflowData, markdownPath string) (string, []string, []string, error) {
Expand Down
30 changes: 30 additions & 0 deletions pkg/workflow/compiler_yaml_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1783,3 +1783,33 @@ Test prompt.
t.Fatal("gh-aw-manifest must not duplicate agent_image_runner metadata")
}
}

func TestNormalizeBlankLines(t *testing.T) {
tests := []struct {
name string
input string
want string
}{
// Empty or all-whitespace input: return a single newline to match the
// original strings.TrimRight(…, "\n") + "\n" behaviour — the caller
// always expects a trailing newline even when there is no real content.
{"empty string", "", "\n"},
{"single blank line", "\n", "\n"},
{"all whitespace line", " \n", "\n"},
{"no trailing newline", "hello", "hello\n"},
{"trailing blank lines stripped", "a\n\nb\n\n\n", "a\n\nb\n"},
{"blank lines in middle preserved", "a\n\nb\n", "a\n\nb\n"},
{"whitespace-only lines cleared", "a\n \nb\n", "a\n\nb\n"},
{"single non-blank line", "key: value\n", "key: value\n"},
{"multiple trailing blank lines", "a\n\n\n\n", "a\n"},
{"only whitespace lines", " \n \n", "\n"},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
got := normalizeBlankLines(tc.input)
if got != tc.want {
t.Errorf("normalizeBlankLines(%q) = %q, want %q", tc.input, got, tc.want)
}
})
}
}
8 changes: 8 additions & 0 deletions pkg/workflow/template_injection_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ func mayContainInlineExpression(s string) bool {
}

func findRunValue(keyPart string) (string, bool) {
// Fast pre-check: every regex alternative embeds "run:" (unquoted),
// "run\":" (double-quoted close), or "run':" (single-quoted close), so
// skip the regex entirely when none of those substrings is present.
if !strings.Contains(keyPart, "run:") &&
!strings.Contains(keyPart, `run":`) &&
!strings.Contains(keyPart, "run':") {
return "", false
}
loc := runKeyPattern.FindStringIndex(keyPart)

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.

[/diagnosing-bugs] The pre-check strings.Contains(keyPart, "run:") is sound for the known regex forms, but there are no tests that assert the fast-path produces identical results to the regex alone. A regression test covering the quoted ("run":) and flow-style ({run:) forms would lock in correctness.

💡 Suggested test
func TestFindRunValueFastPath(t *testing.T) {
    cases := []struct {
        input  string
        wantOk bool
    }{
        {`run: echo hello`, true},
        {`"run": echo hello`, true},
        {`{run: echo hello}`, true},
        {`step: build`, false},
        {`runner: ubuntu`, false}, // "run" present but not "run:"
    }
    for _, c := range cases {
        _, ok := findRunValue(c.input)
        if ok != c.wantOk {
            t.Errorf("findRunValue(%q) ok=%v, want %v", c.input, ok, c.wantOk)
        }
    }
}

@copilot please address this.

if loc == nil {
return "", false
Expand Down
66 changes: 66 additions & 0 deletions pkg/workflow/template_injection_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1394,6 +1394,33 @@ func TestScanRunContentExpressions(t *testing.T) {
wantHasUnsafe: false,
wantHasDisallowed: true,
},
{
name: "unsafe expression in double-quoted run key",
yaml: `jobs:
test:
steps:
- "run": echo "${{ github.event.issue.title }}"`,
wantHasUnsafe: true,
wantHasDisallowed: true,
},
{
name: "unsafe expression in single-quoted run key",
yaml: `jobs:
test:
steps:
- 'run': echo "${{ github.event.issue.title }}"`,
wantHasUnsafe: true,
wantHasDisallowed: true,
},
{
name: "allowed expression in double-quoted run key",
yaml: `jobs:
test:
steps:
- "run": node ${{ runner.temp }}/actions/foo.cjs`,
wantHasUnsafe: false,
wantHasDisallowed: false,
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -1593,3 +1620,42 @@ func TestDetectHeredocDelimiter(t *testing.T) {
})
}
}

func TestFindRunValueFastPath(t *testing.T) {
cases := []struct {
input string
wantOk bool
wantVal string
}{
// Unquoted key forms
{`run: echo hello`, true, "echo hello"},
{` run: echo hello`, true, "echo hello"},
// Flow-style YAML: the trailing "}" is part of the raw value returned by
// findRunValue (everything after "run:"). Template-injection callers scan
// that raw string for ${{...}} expressions, so the brace does no harm.
{`{run: echo hello}`, true, "echo hello}"},
// Double-quoted key
{`"run": echo hello`, true, "echo hello"},
// Single-quoted key
{`'run': echo hello`, true, "echo hello"},
// Mixed-quote forms
{`"run': echo hello`, true, "echo hello"},
{`'run": echo hello`, true, "echo hello"},
// Non-matching cases
{`step: build`, false, ""},
{`runner: ubuntu`, false, ""},
{`runner:`, false, ""},
{`name: run something`, false, ""},
}
for _, c := range cases {
t.Run(c.input, func(t *testing.T) {
val, ok := findRunValue(c.input)
if ok != c.wantOk {
t.Errorf("findRunValue(%q) ok=%v, want %v", c.input, ok, c.wantOk)
}
if ok && val != c.wantVal {
t.Errorf("findRunValue(%q) val=%q, want %q", c.input, val, c.wantVal)
}
})
}
}
Loading