Skip to content
Merged
5 changes: 2 additions & 3 deletions pkg/cli/bootstrap_profile_manifest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 ",
Expand All @@ -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",
Expand All @@ -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()

Expand Down
1 change: 1 addition & 0 deletions pkg/cli/compile_guard_policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ permissions:
contents: read
engine: copilot
tools:
bash: false
github:
min-integrity: none
---
Expand Down
11 changes: 5 additions & 6 deletions pkg/workflow/compiler_safe_outputs_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"math"
"os"
"path/filepath"
"strconv"
"strings"
"testing"

Expand Down Expand Up @@ -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 {
Expand Down
26 changes: 8 additions & 18 deletions pkg/workflow/notify_comment_conclusion_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions pkg/workflow/notify_comment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
},
}
Expand Down Expand Up @@ -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"},
},
}
Expand Down Expand Up @@ -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"},
},
}
Expand Down
19 changes: 8 additions & 11 deletions pkg/workflow/safe_outputs_config_global.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package workflow

import (
"math"
"strconv"
"strings"

"github.com/github/gh-aw/pkg/logger"
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/workflow/safe_outputs_config_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 4 additions & 2 deletions pkg/workflow/safe_outputs_import_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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")
}
Expand Down
30 changes: 25 additions & 5 deletions pkg/workflow/step_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"maps"
"reflect"
"sort"
"strconv"

"github.com/github/gh-aw/pkg/logger"
)
Expand All @@ -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
Comment thread
github-actions[bot] marked this conversation as resolved.
TimeoutMinutes int `yaml:"timeout-minutes,omitempty"`
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down
29 changes: 22 additions & 7 deletions pkg/workflow/step_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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,
},
},
}
Expand Down Expand Up @@ -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,
Expand All @@ -228,7 +228,7 @@ func TestMapToStep(t *testing.T) {
want: &WorkflowStep{
Name: "Test step",
Run: "npm test",
ContinueOnError: "false",
ContinueOnError: templatableBoolPtr("false"),
},
wantErr: false,
},
Expand Down Expand Up @@ -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,
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
Loading