diff --git a/pkg/workflow/compiler_validators.go b/pkg/workflow/compiler_validators.go index 00753b81fa2..9d11e624775 100644 --- a/pkg/workflow/compiler_validators.go +++ b/pkg/workflow/compiler_validators.go @@ -11,6 +11,7 @@ import ( "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/goccy/go-yaml" ) // validateExpressions checks expression safety and runtime-import file references @@ -318,6 +319,12 @@ func (c *Compiler) emitSandboxRuntimeWarnings(workflowData *WorkflowData, markdo } func (c *Compiler) emitGeneralToolWarnings(workflowData *WorkflowData, markdownPath string) { + if workflowData.SafeOutputs != nil && hasWorkflowDispatchInputs(workflowData.On) && workflowData.ConcurrencyJobDiscriminator == "" { + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", + "workflow_dispatch workflow has no concurrency.job-discriminator; the generated conclusion concurrency group is shared by all dispatches of this workflow. "+ + "Set a discriminator (for example, `${{ github.run_id }}`) to give each dispatch its own slot.")) + c.IncrementWarningCount() + } if workflowData.Concurrency != "" && strings.Contains(workflowData.Concurrency, "cancel-in-progress: true") && hasBotSelfCancelRisk(workflowData) { fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", "Custom workflow-level concurrency with cancel-in-progress: true may cause self-cancellation.\n"+ @@ -362,6 +369,23 @@ func (c *Compiler) emitGeneralToolWarnings(workflowData *WorkflowData, markdownP } } +func hasWorkflowDispatchInputs(onYAML string) bool { + var parsedData map[string]any + if err := yaml.Unmarshal([]byte(onYAML), &parsedData); err != nil { + return false + } + onMap, ok := parsedData["on"].(map[string]any) + if !ok { + return false + } + workflowDispatch, ok := onMap["workflow_dispatch"].(map[string]any) + if !ok { + return false + } + inputs, ok := workflowDispatch["inputs"].(map[string]any) + return ok && len(inputs) > 0 +} + func (c *Compiler) emitExperimentalFeatureWarnings(workflowData *WorkflowData) { c.emitExperimentalFeatureWarningsTo(workflowData, os.Stderr) } diff --git a/pkg/workflow/workflow_dispatch_concurrency_warning_test.go b/pkg/workflow/workflow_dispatch_concurrency_warning_test.go new file mode 100644 index 00000000000..811abd89dea --- /dev/null +++ b/pkg/workflow/workflow_dispatch_concurrency_warning_test.go @@ -0,0 +1,101 @@ +//go:build !integration + +package workflow + +import ( + "testing" + + "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/assert" +) + +func TestWorkflowDispatchConcurrencyWarning(t *testing.T) { + tests := []struct { + name string + on string + discriminator string + safeOutputs bool + expectWarning bool + }{ + { + name: "workflow dispatch without discriminator", + on: "on: workflow_dispatch", + safeOutputs: true, + expectWarning: false, + }, + { + name: "workflow dispatch with empty inputs without discriminator", + on: "on:\n workflow_dispatch:\n inputs: {}", + safeOutputs: true, + expectWarning: false, + }, + { + name: "workflow dispatch with inputs without discriminator", + on: "on:\n workflow_dispatch:\n inputs:\n target_repo:\n type: string", + safeOutputs: true, + expectWarning: true, + }, + { + name: "workflow dispatch with inputs without safe outputs", + on: "on:\n workflow_dispatch:\n inputs:\n target_repo:\n type: string", + expectWarning: false, + }, + { + name: "mixed workflow dispatch without discriminator", + on: "on:\n workflow_dispatch:\n schedule:\n - cron: '0 0 * * *'", + safeOutputs: true, + expectWarning: false, + }, + { + name: "mixed workflow dispatch with inputs without discriminator", + on: "on:\n workflow_dispatch:\n inputs:\n target_repo:\n type: string\n schedule:\n - cron: '0 0 * * *'", + safeOutputs: true, + expectWarning: true, + }, + { + name: "workflow dispatch with inputs and discriminator", + on: "on:\n workflow_dispatch:\n inputs:\n target_repo:\n type: string", + discriminator: "${{ github.run_id }}", + safeOutputs: true, + expectWarning: false, + }, + { + name: "nested workflow dispatch value without discriminator", + on: "on:\n workflow_run:\n workflows: [workflow_dispatch]\n types: [completed]", + safeOutputs: true, + expectWarning: false, + }, + { + name: "schedule without discriminator", + on: "on:\n schedule:\n - cron: '0 0 * * *'", + safeOutputs: true, + expectWarning: false, + }, + } + + const warning = "workflow_dispatch workflow has no concurrency.job-discriminator" + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + workflowData := &WorkflowData{ + On: tt.on, + ConcurrencyJobDiscriminator: tt.discriminator, + } + if tt.safeOutputs { + workflowData.SafeOutputs = &SafeOutputsConfig{} + } + + output := testutil.CaptureStderr(t, func() { + compiler.emitGeneralToolWarnings(workflowData, "test.md") + }) + + if tt.expectWarning { + assert.Contains(t, output, warning) + assert.Equal(t, 1, compiler.GetWarningCount()) + } else { + assert.NotContains(t, output, warning) + assert.Zero(t, compiler.GetWarningCount()) + } + }) + } +}