diff --git a/pkg/workflow/frontmatter_extraction_yaml.go b/pkg/workflow/frontmatter_extraction_yaml.go index 630dc2a396a..d43e6cdcf55 100644 --- a/pkg/workflow/frontmatter_extraction_yaml.go +++ b/pkg/workflow/frontmatter_extraction_yaml.go @@ -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 diff --git a/pkg/workflow/on_needs_integration_test.go b/pkg/workflow/on_needs_integration_test.go index f6f8ad0962e..f7b193664a5 100644 --- a/pkg/workflow/on_needs_integration_test.go +++ b/pkg/workflow/on_needs_integration_test.go @@ -5,6 +5,7 @@ package workflow import ( "os" "path/filepath" + "strings" "testing" "github.com/github/gh-aw/pkg/testutil" @@ -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") @@ -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 + } + // 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 +} diff --git a/pkg/workflow/yaml_test.go b/pkg/workflow/yaml_test.go index 651e13ff2a8..07834aa4526 100644 --- a/pkg/workflow/yaml_test.go +++ b/pkg/workflow/yaml_test.go @@ -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()