diff --git a/pkg/cli/bootstrap_profile_manifest_test.go b/pkg/cli/bootstrap_profile_manifest_test.go index c2baaaa08d4..9e366eb73ab 100644 --- a/pkg/cli/bootstrap_profile_manifest_test.go +++ b/pkg/cli/bootstrap_profile_manifest_test.go @@ -294,7 +294,7 @@ func TestParseManifestBootstrapAction(t *testing.T) { "description": " desc ", "default": " keepme ", "secret": " SECRET ", - "strategy": " strat ", + "strategy": " custom ", "message": " hello ", "mode": " existing ", "app-id-variable": " APP_ID ", @@ -315,7 +315,7 @@ func TestParseManifestBootstrapAction(t *testing.T) { Description: "desc", Default: " keepme ", Secret: "SECRET", - Strategy: "strat", + Strategy: "custom", Message: "hello", Mode: "existing", AppIDVariable: "APP_ID", @@ -337,7 +337,6 @@ func TestParseManifestBootstrapAction(t *testing.T) { } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() diff --git a/pkg/cli/compile_guard_policy_test.go b/pkg/cli/compile_guard_policy_test.go index 354373f1707..6f7d48d5266 100644 --- a/pkg/cli/compile_guard_policy_test.go +++ b/pkg/cli/compile_guard_policy_test.go @@ -32,6 +32,7 @@ permissions: contents: read engine: copilot tools: + bash: false github: min-integrity: none --- diff --git a/pkg/workflow/compiler_safe_outputs_config_test.go b/pkg/workflow/compiler_safe_outputs_config_test.go index 851cdb0bb76..f6378ef0882 100644 --- a/pkg/workflow/compiler_safe_outputs_config_test.go +++ b/pkg/workflow/compiler_safe_outputs_config_test.go @@ -7,6 +7,7 @@ import ( "math" "os" "path/filepath" + "strconv" "strings" "testing" @@ -3674,14 +3675,12 @@ func TestReportFailureAsIssueWithCategoriesFilter(t *testing.T) { require.NotNil(t, config, "SafeOutputsConfig should be created") if tt.expectBool != nil { - reportBool, ok := config.ReportFailureAsIssue.(bool) - require.True(t, ok, "ReportFailureAsIssue should be bool") - assert.Equal(t, *tt.expectBool, reportBool, "Boolean value should match") + require.NotNil(t, config.ReportFailureAsIssue, "ReportFailureAsIssue should be set") + assert.Equal(t, strconv.FormatBool(*tt.expectBool), config.ReportFailureAsIssue.String(), "Boolean value should match") } if tt.expectString != "" { - reportString, ok := config.ReportFailureAsIssue.(string) - require.True(t, ok, "ReportFailureAsIssue should be string") - assert.Equal(t, tt.expectString, reportString, "String value should match") + require.NotNil(t, config.ReportFailureAsIssue, "ReportFailureAsIssue should be set") + assert.Equal(t, tt.expectString, config.ReportFailureAsIssue.String(), "String value should match") } if len(tt.expectCategories) > 0 { diff --git a/pkg/workflow/notify_comment_conclusion_helpers.go b/pkg/workflow/notify_comment_conclusion_helpers.go index bf7e507a3b5..a06ce129da3 100644 --- a/pkg/workflow/notify_comment_conclusion_helpers.go +++ b/pkg/workflow/notify_comment_conclusion_helpers.go @@ -343,26 +343,16 @@ func buildAgentFailureReportingPolicyVars(data *WorkflowData) []string { envVars = append(envVars, fmt.Sprintf(" GH_AW_FAILURE_REPORT_AS_ISSUE: %q\n", strconv.FormatBool(enabled))) } shouldIncludeCategoryFilters := true - switch reportSetting := data.SafeOutputs.ReportFailureAsIssue.(type) { - case bool: - appendReportFailureEnvVar(reportSetting) - shouldIncludeCategoryFilters = reportSetting - case string: - reportExpression := reportSetting - switch reportExpression { - case "true": - appendReportFailureEnvVar(true) - case "false": - appendReportFailureEnvVar(false) - shouldIncludeCategoryFilters = false - default: - envVars = append(envVars, buildTemplatableBoolEnvVar("GH_AW_FAILURE_REPORT_AS_ISSUE", &reportExpression)...) - shouldIncludeCategoryFilters = false - } - case []any: + reportSetting := data.SafeOutputs.ReportFailureAsIssue.String() + switch reportSetting { + case "true": appendReportFailureEnvVar(true) + case "false": + appendReportFailureEnvVar(false) + shouldIncludeCategoryFilters = false default: - appendReportFailureEnvVar(true) + envVars = append(envVars, buildTemplatableBoolEnvVar("GH_AW_FAILURE_REPORT_AS_ISSUE", templatableBoolPtrToStringPtr(data.SafeOutputs.ReportFailureAsIssue))...) + shouldIncludeCategoryFilters = false } if shouldIncludeCategoryFilters { if len(data.SafeOutputs.ReportFailureAsIssueCategories) > 0 { diff --git a/pkg/workflow/notify_comment_test.go b/pkg/workflow/notify_comment_test.go index 346dac2817a..758c4555554 100644 --- a/pkg/workflow/notify_comment_test.go +++ b/pkg/workflow/notify_comment_test.go @@ -1169,7 +1169,7 @@ func TestConclusionJobCategoriesFilterQuoting(t *testing.T) { Name: "Test Workflow", SafeOutputs: &SafeOutputsConfig{ NoOp: &NoOpConfig{}, - ReportFailureAsIssue: true, + ReportFailureAsIssue: templatableBoolPtr("true"), ReportFailureAsIssueCategories: []string{"it's-a-category", "normal-category"}, }, } @@ -1198,7 +1198,7 @@ func TestConclusionJobCategoriesFilterQuoting(t *testing.T) { Name: "Test Workflow", SafeOutputs: &SafeOutputsConfig{ NoOp: &NoOpConfig{}, - ReportFailureAsIssue: true, + ReportFailureAsIssue: templatableBoolPtr("true"), ReportFailureAsIssueExcludedCategories: []string{"it's-excluded", "other-excluded"}, }, } @@ -1228,7 +1228,7 @@ func TestConclusionJobReportFailureAsIssueTemplatableExpression(t *testing.T) { Name: "Test Workflow", SafeOutputs: &SafeOutputsConfig{ NoOp: &NoOpConfig{}, - ReportFailureAsIssue: "${{ inputs.report-failure-as-issue }}", + ReportFailureAsIssue: templatableBoolPtr("${{ inputs.report-failure-as-issue }}"), ReportFailureAsIssueCategories: []string{"agent_failure"}, }, } diff --git a/pkg/workflow/safe_outputs_config_global.go b/pkg/workflow/safe_outputs_config_global.go index 79995336b6f..c37a9d2ca86 100644 --- a/pkg/workflow/safe_outputs_config_global.go +++ b/pkg/workflow/safe_outputs_config_global.go @@ -2,6 +2,7 @@ package workflow import ( "math" + "strconv" "strings" "github.com/github/gh-aw/pkg/logger" @@ -168,7 +169,8 @@ func (c *Compiler) extractGlobalConfigFields(outputMap map[string]any, config *S } } } - config.ReportFailureAsIssue = reportFailureAsIssue // Preserve original value for proper serialization + reportAsIssue := TemplatableBool("true") + config.ReportFailureAsIssue = &reportAsIssue config.ReportFailureAsIssueCategories = includedCategories config.ReportFailureAsIssueExcludedCategories = excludedCategories if len(includedCategories) > 0 && len(excludedCategories) > 0 { @@ -184,17 +186,12 @@ func (c *Compiler) extractGlobalConfigFields(outputMap map[string]any, config *S safeOutputsConfigLog.Printf("Failed to preprocess report-failure-as-issue field: %v (ignoring invalid value and leaving field unset)", err) } else { if reportFailureAsIssueStr, ok := outputMap["report-failure-as-issue"].(string); ok { - switch reportFailureAsIssueStr { - case "true": - config.ReportFailureAsIssue = true - case "false": - config.ReportFailureAsIssue = false - default: - config.ReportFailureAsIssue = reportFailureAsIssueStr - } - safeOutputsConfigLog.Printf("Report failure as issue: %v", config.ReportFailureAsIssue) + reportAsIssue := TemplatableBool(reportFailureAsIssueStr) + config.ReportFailureAsIssue = &reportAsIssue + safeOutputsConfigLog.Printf("Report failure as issue: %s", reportAsIssue.String()) } else if reportFailureAsIssueBool, ok := outputMap["report-failure-as-issue"].(bool); ok { - config.ReportFailureAsIssue = reportFailureAsIssueBool + reportAsIssue := TemplatableBool(strconv.FormatBool(reportFailureAsIssueBool)) + config.ReportFailureAsIssue = &reportAsIssue safeOutputsConfigLog.Printf("Report failure as issue: %t", reportFailureAsIssueBool) } } diff --git a/pkg/workflow/safe_outputs_config_types.go b/pkg/workflow/safe_outputs_config_types.go index 8b72ab96974..58197387f53 100644 --- a/pkg/workflow/safe_outputs_config_types.go +++ b/pkg/workflow/safe_outputs_config_types.go @@ -106,7 +106,7 @@ type SafeOutputsConfig struct { Mentions *MentionsConfig `yaml:"mentions,omitempty"` // Configuration for @mention filtering in safe outputs Footer *bool `yaml:"footer,omitempty"` // Global footer control - when false, omits visible footer from all safe outputs (XML markers still included) GroupReports bool `yaml:"group-reports,omitempty"` // If true, create parent "Failed runs" issue for agent failures (default: false) - ReportFailureAsIssue any `yaml:"report-failure-as-issue,omitempty"` // Controls failure issue creation: bool, templatable expression string, or []interface{} categories (parsed to ReportFailureAsIssueCategories/ExcludedCategories). Default: true + ReportFailureAsIssue *TemplatableBool `yaml:"report-failure-as-issue,omitempty"` // Controls failure issue creation: bool or templatable expression string. Default: true. Category arrays are parsed into ReportFailureAsIssueCategories/ExcludedCategories. ReportFailureAsIssueCategories []string `yaml:"-"` // Parsed failure categories for report-failure-as-issue (internal use only, included categories) ReportFailureAsIssueExcludedCategories []string `yaml:"-"` // Parsed excluded failure categories for report-failure-as-issue (internal use only, categories starting with "!") ReportFailedJobs *bool `yaml:"report-failed-jobs,omitempty"` // Controls whether to report failed non-builtin jobs as issues (default: true). Set to false to disable. diff --git a/pkg/workflow/safe_outputs_import_test.go b/pkg/workflow/safe_outputs_import_test.go index a67c6358bda..924362d8750 100644 --- a/pkg/workflow/safe_outputs_import_test.go +++ b/pkg/workflow/safe_outputs_import_test.go @@ -786,7 +786,8 @@ This workflow uses the imported meta configuration. assert.True(t, templatableBoolIsTrue(workflowData.SafeOutputs.Staged), "Staged should be imported and set to true") assert.Equal(t, map[string]string{"TEST_VAR": "test_value"}, workflowData.SafeOutputs.Env, "Env should be imported") assert.Equal(t, "${{ secrets.CUSTOM_TOKEN }}", workflowData.SafeOutputs.GitHubToken, "GitHubToken should be imported") - assert.Equal(t, "${{ inputs.report-failure-as-issue }}", workflowData.SafeOutputs.ReportFailureAsIssue, "ReportFailureAsIssue should be imported as templatable bool") + require.NotNil(t, workflowData.SafeOutputs.ReportFailureAsIssue, "ReportFailureAsIssue should be imported") + assert.Equal(t, "${{ inputs.report-failure-as-issue }}", workflowData.SafeOutputs.ReportFailureAsIssue.String(), "ReportFailureAsIssue should be imported as templatable bool") // Note: When main workflow has safe-outputs section, extractSafeOutputsConfig sets MaximumPatchSize default (4096) // before merge happens, so imported value is not used. User should specify max-patch-size in main workflow. assert.Equal(t, 4096, workflowData.SafeOutputs.MaximumPatchSize, "MaximumPatchSize defaults to 4096 when main has safe-outputs") @@ -860,7 +861,8 @@ This workflow has its own meta configuration that should take precedence. // Verify main workflow meta fields take precedence assert.Equal(t, []string{"main.example.com"}, workflowData.SafeOutputs.AllowedDomains, "AllowedDomains from main should take precedence") - assert.Equal(t, "${{ inputs.report-failure-as-issue }}", workflowData.SafeOutputs.ReportFailureAsIssue, "ReportFailureAsIssue from main should take precedence") + require.NotNil(t, workflowData.SafeOutputs.ReportFailureAsIssue, "ReportFailureAsIssue from main should be set") + assert.Equal(t, "${{ inputs.report-failure-as-issue }}", workflowData.SafeOutputs.ReportFailureAsIssue.String(), "ReportFailureAsIssue from main should take precedence") assert.Equal(t, "${{ secrets.MAIN_TOKEN }}", workflowData.SafeOutputs.GitHubToken, "GitHubToken from main should take precedence") assert.Equal(t, 2048, workflowData.SafeOutputs.MaximumPatchSize, "MaximumPatchSize from main should take precedence") } diff --git a/pkg/workflow/step_types.go b/pkg/workflow/step_types.go index 9b63c4ff593..13211b1485e 100644 --- a/pkg/workflow/step_types.go +++ b/pkg/workflow/step_types.go @@ -7,6 +7,7 @@ import ( "maps" "reflect" "sort" + "strconv" "github.com/github/gh-aw/pkg/logger" ) @@ -25,7 +26,7 @@ type WorkflowStep struct { Shell string `yaml:"shell,omitempty"` With map[string]any `yaml:"with,omitempty"` Env map[string]string `yaml:"env,omitempty"` - ContinueOnError any `yaml:"continue-on-error,omitempty"` // Can be bool or string expression + ContinueOnError *TemplatableBool `yaml:"continue-on-error,omitempty"` // Can be bool or string expression TimeoutMinutes int `yaml:"timeout-minutes,omitempty"` } @@ -67,7 +68,14 @@ func (s *WorkflowStep) ToMap() map[string]any { result["env"] = s.Env } if s.ContinueOnError != nil { - result["continue-on-error"] = s.ContinueOnError + switch s.ContinueOnError.String() { + case "true": + result["continue-on-error"] = true + case "false": + result["continue-on-error"] = false + default: + result["continue-on-error"] = s.ContinueOnError.String() + } } if s.TimeoutMinutes > 0 { result["timeout-minutes"] = s.TimeoutMinutes @@ -126,8 +134,16 @@ func MapToStep(stepMap map[string]any) (*WorkflowStep, error) { } } if continueOnError, ok := stepMap["continue-on-error"]; ok { - // Preserve the original type (bool or string) - step.ContinueOnError = continueOnError + switch value := continueOnError.(type) { + case bool: + templatableValue := TemplatableBool(strconv.FormatBool(value)) + step.ContinueOnError = &templatableValue + case string: + if value == "true" || value == "false" || isExpression(value) { + templatableValue := TemplatableBool(value) + step.ContinueOnError = &templatableValue + } + } } if timeoutMinutes, ok := stepMap["timeout-minutes"].(int); ok { step.TimeoutMinutes = timeoutMinutes @@ -153,10 +169,14 @@ func (s *WorkflowStep) Clone() *WorkflowStep { Run: s.Run, WorkingDirectory: s.WorkingDirectory, Shell: s.Shell, - ContinueOnError: s.ContinueOnError, TimeoutMinutes: s.TimeoutMinutes, } + if s.ContinueOnError != nil { + continueOnError := *s.ContinueOnError + clone.ContinueOnError = &continueOnError + } + if s.With != nil { clone.With = make(map[string]any, len(s.With)) maps.Copy(clone.With, s.With) diff --git a/pkg/workflow/step_types_test.go b/pkg/workflow/step_types_test.go index 19a1a031ed8..53b71130f6e 100644 --- a/pkg/workflow/step_types_test.go +++ b/pkg/workflow/step_types_test.go @@ -86,7 +86,7 @@ func TestWorkflowStep_ToMap(t *testing.T) { WorkingDirectory: "/path/to/dir", With: map[string]any{"key": "value"}, Env: map[string]string{"VAR": "val"}, - ContinueOnError: true, + ContinueOnError: templatableBoolPtr("true"), TimeoutMinutes: 10, }, want: map[string]any{ @@ -115,12 +115,12 @@ func TestWorkflowStep_ToMap(t *testing.T) { step: &WorkflowStep{ Name: "Test step", Run: "npm test", - ContinueOnError: "false", + ContinueOnError: templatableBoolPtr("false"), }, want: map[string]any{ "name": "Test step", "run": "npm test", - "continue-on-error": "false", + "continue-on-error": false, }, }, } @@ -201,7 +201,7 @@ func TestMapToStep(t *testing.T) { WorkingDirectory: "/path/to/dir", With: map[string]any{"key": "value"}, Env: map[string]string{"VAR": "val"}, - ContinueOnError: true, + ContinueOnError: templatableBoolPtr("true"), TimeoutMinutes: 10, }, wantErr: false, @@ -228,7 +228,7 @@ func TestMapToStep(t *testing.T) { want: &WorkflowStep{ Name: "Test step", Run: "npm test", - ContinueOnError: "false", + ContinueOnError: templatableBoolPtr("false"), }, wantErr: false, }, @@ -288,7 +288,7 @@ func TestWorkflowStep_Clone(t *testing.T) { Shell: "bash", With: map[string]any{"key": "value", "nested": map[string]any{"inner": "val"}}, Env: map[string]string{"VAR1": "val1", "VAR2": "val2"}, - ContinueOnError: true, + ContinueOnError: templatableBoolPtr("true"), TimeoutMinutes: 15, } @@ -308,6 +308,12 @@ func TestWorkflowStep_Clone(t *testing.T) { clone.Env["NEW_VAR"] = "new-val" _, exists = original.Env["NEW_VAR"] assert.False(t, exists, "Clone should deep copy Env map - modifying clone should not affect original") + + require.NotNil(t, clone.ContinueOnError, "Clone should copy ContinueOnError") + require.NotNil(t, original.ContinueOnError, "Original should retain ContinueOnError") + *clone.ContinueOnError = TemplatableBool("false") + assert.Equal(t, "true", original.ContinueOnError.String(), "Clone should deep copy ContinueOnError - modifying clone should not affect original") + assert.Equal(t, "false", clone.ContinueOnError.String(), "Clone should allow independent ContinueOnError updates") } func TestMapToStep_RoundTrip(t *testing.T) { @@ -385,6 +391,15 @@ func compareStepValues(a, b any) bool { } } return true + case *TemplatableBool: + bValue, ok := b.(*TemplatableBool) + if !ok { + return false + } + if aVal == nil || bValue == nil { + return aVal == nil && bValue == nil + } + return aVal.String() == bValue.String() default: return a == b } @@ -406,7 +421,7 @@ func compareSteps(a, b *WorkflowStep) bool { return false } - // Compare ContinueOnError (can be any type) + // Compare ContinueOnError (templatable bool) if !compareStepValues(a.ContinueOnError, b.ContinueOnError) { return false }