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
7 changes: 7 additions & 0 deletions pkg/workflow/frontmatter_extraction_yaml.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ func (c *Compiler) extractTopLevelYAMLSection(frontmatter map[string]any, key st

// Check if value is a map that we should order alphabetically
if valueMap, ok := value.(map[string]any); ok {
// For the "on" section, exclude gh-aw-specific keys that are processed
// separately and must not appear in the compiled GitHub Actions workflow.
// The "needs" key (on.needs) controls job dependency wiring and is invalid
// as a top-level on: trigger key in GitHub Actions.
if key == "on" {
valueMap = excludeMapKeys(valueMap, "needs")
}
// Use OrderMapFields for alphabetical sorting (empty priority list = all alphabetical)
orderedValue := OrderMapFields(valueMap, []string{})
// Wrap the ordered value with the key using MapSlice
Expand Down
59 changes: 59 additions & 0 deletions pkg/workflow/on_needs_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package workflow
import (
"os"
"path/filepath"
"strings"
"testing"

"github.com/github/gh-aw/pkg/testutil"
Expand Down Expand Up @@ -52,6 +53,15 @@ Run with on.needs
var lock map[string]any
require.NoError(t, yaml.Unmarshal(lockBytes, &lock), "compiled lock file should be valid YAML")

// Verify on.needs is NOT emitted into the compiled on: section
onSection, ok := lock["on"].(map[string]any)
require.True(t, ok, "compiled workflow should contain on section")
assert.NotContains(t, onSection, "needs", "on.needs must not appear as a key in the compiled on: section")

// Verify on.needs is not in the raw compiled text (even as a comment)
lockContent := string(lockBytes)
assert.False(t, containsNeedsInOnSection(lockContent), "on.needs must not appear in the compiled on: section text")

jobs, ok := lock["jobs"].(map[string]any)
require.True(t, ok, "compiled workflow should contain jobs map")

Expand All @@ -63,3 +73,52 @@ Run with on.needs
require.True(t, ok, "compiled workflow should contain activation job")
assert.Contains(t, activation["needs"], "secrets_fetcher", "activation should depend on on.needs job")
}

// containsNeedsInOnSection checks whether the "needs:" key appears in the "on:" section
// of a compiled workflow YAML string, including a commented-out "# needs:" line.
func containsNeedsInOnSection(yamlContent string) bool {
inOnSection := false
onIndent := 0
childIndent := -1
for _, line := range strings.Split(yamlContent, "\n") {
trimmed := strings.TrimLeft(line, " \t")
// Track when we enter/leave the on: section
if trimmed == "on:" || strings.HasPrefix(trimmed, `"on":`) {
inOnSection = true
onIndent = len(line) - len(trimmed)
childIndent = -1
continue
}

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.

[/tdd] containsNeedsInOnSection parses raw YAML text to detect needs: inside on:. A needs: key at job level (e.g. jobs.activation.needs) could also match if the heuristic mis-tracks the section boundary — producing a false positive. The structured map check on line 55 already verifies the fix at the parsed-YAML level; the raw-text check adds fragility without extra safety.

💡 Suggested improvement

Either remove containsNeedsInOnSection (the structured assert on line 55 is sufficient), or tighten it so it cannot match needs: lines deeply indented inside jobs:. A comment explaining why both checks coexist would also help future readers.

@copilot please address this.

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.

Addressed in 06e7928. I kept the raw-text regression check, but tightened it to only consider direct child keys within on: so it catches commented-out # needs: lines without matching unrelated nested/job-level needs: entries.

// Leave on: section when we hit a top-level key
if inOnSection && len(line) > 0 && line[0] != ' ' && line[0] != '\t' && !strings.HasPrefix(trimmed, "#") {
inOnSection = false
}
if !inOnSection {
continue
}
indent := len(line) - len(trimmed)
if indent <= onIndent {
continue
}
if trimmed == "" || strings.HasPrefix(trimmed, "#") {
normalized := strings.TrimSpace(strings.TrimPrefix(trimmed, "#"))
if childIndent != -1 && indent == childIndent && strings.HasPrefix(normalized, "needs:") {
return true
}
continue
}
if childIndent == -1 {
childIndent = indent
}
if indent != childIndent {
continue
}
normalized := strings.TrimSpace(strings.TrimPrefix(trimmed, "#"))
// Check for needs: inside on: section as a direct child key, even if it
// has been commented out.
if strings.HasPrefix(normalized, "needs:") {
return true
}
}
return false
}
28 changes: 28 additions & 0 deletions pkg/workflow/yaml_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,34 @@ func TestMarshalWithFieldOrder_OrdersNestedEnvWithSecretsRecursively(t *testing.
assertOrder(" ALPHA_STEP:", " ZETA_STEP:")
}

func TestExtractTopLevelYAMLSectionExcludesOnNeeds(t *testing.T) {
compiler := NewCompiler()

frontmatter := map[string]any{
"on": map[string]any{
"needs": []any{"custom_job"},
"workflow_dispatch": nil,
},
}

result := compiler.extractTopLevelYAMLSection(frontmatter, "on")

if strings.Contains(result, "needs:") {
t.Errorf("on.needs must not appear in compiled on: section, but found it in:\n%s", result)
}
if !strings.Contains(result, "workflow_dispatch") {
t.Errorf("workflow_dispatch should be present in the compiled on: section:\n%s", result)
}
// Verify the original frontmatter["on"] map is not mutated
onMap, ok := frontmatter["on"].(map[string]any)
if !ok {
t.Fatal("frontmatter[\"on\"] should remain a map[string]any")
}
if _, hasNeeds := onMap["needs"]; !hasNeeds {
t.Error("extractTopLevelYAMLSection must not mutate the original frontmatter[\"on\"] map")
}
}

func TestExtractTopLevelYAMLSectionWithOrdering(t *testing.T) {
compiler := NewCompiler()

Expand Down
Loading