diff --git a/pkg/workflow/bundler_runtime_validation.go b/pkg/workflow/bundler_runtime_validation.go index 1bb73ff57a..bad1ac0d05 100644 --- a/pkg/workflow/bundler_runtime_validation.go +++ b/pkg/workflow/bundler_runtime_validation.go @@ -54,6 +54,8 @@ var bundlerRuntimeLog = logger.New("workflow:bundler_runtime_validation") // validateNoRuntimeMixing checks that all files being bundled are compatible with the target runtime mode // This prevents mixing nodejs-only scripts (that use child_process) with github-script scripts // Returns an error if incompatible runtime modes are detected +// Note: This function uses fail-fast error handling because runtime mode conflicts in dependencies +// need to be resolved one at a time, and showing multiple conflicting dependency chains would be confusing func validateNoRuntimeMixing(mainScript string, sources map[string]string, targetMode RuntimeMode) error { bundlerRuntimeLog.Printf("Validating runtime mode compatibility: target_mode=%s", targetMode) @@ -61,6 +63,7 @@ func validateNoRuntimeMixing(mainScript string, sources map[string]string, targe checked := make(map[string]bool) // Recursively validate the main script and its dependencies + // This uses fail-fast error handling because runtime conflicts need sequential resolution return validateRuntimeModeRecursive(mainScript, "", sources, targetMode, checked) } diff --git a/pkg/workflow/dispatch_workflow_test.go b/pkg/workflow/dispatch_workflow_test.go index ce8f91c321..7558340b38 100644 --- a/pkg/workflow/dispatch_workflow_test.go +++ b/pkg/workflow/dispatch_workflow_test.go @@ -457,3 +457,136 @@ No agentic-workflows tool is present. assert.Contains(t, err.Error(), "nonexistent", "Error should mention the workflow name") } + +// TestDispatchWorkflowMultipleErrors tests that multiple validation errors are aggregated +func TestDispatchWorkflowMultipleErrors(t *testing.T) { + compiler := NewCompilerWithVersion("1.0.0") + compiler.failFast = false // Enable error aggregation + + tmpDir := t.TempDir() + awDir := filepath.Join(tmpDir, ".github", "aw") + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + + err := os.MkdirAll(awDir, 0755) + require.NoError(t, err, "Failed to create aw directory") + err = os.MkdirAll(workflowsDir, 0755) + require.NoError(t, err, "Failed to create workflows directory") + + // Create a workflow WITHOUT workflow_dispatch + ciWorkflow := `name: CI +on: + push: + pull_request: +jobs: + test: + runs-on: ubuntu-latest + steps: + - run: echo "Running tests" +` + ciFile := filepath.Join(workflowsDir, "ci.lock.yml") + err = os.WriteFile(ciFile, []byte(ciWorkflow), 0644) + require.NoError(t, err, "Failed to write ci workflow") + + // Create dispatcher workflow that references multiple problematic workflows + dispatcherWorkflow := `--- +on: issues +engine: copilot +permissions: + contents: read +safe-outputs: + dispatch-workflow: + workflows: + - dispatcher # Self-reference + - ci # Missing workflow_dispatch + - nonexistent # Not found + max: 3 +--- + +# Dispatcher Workflow + +This workflow has multiple validation errors. +` + dispatcherFile := filepath.Join(awDir, "dispatcher.md") + err = os.WriteFile(dispatcherFile, []byte(dispatcherWorkflow), 0644) + require.NoError(t, err, "Failed to write dispatcher workflow") + + // Change to the aw directory + oldDir, err := os.Getwd() + require.NoError(t, err, "Failed to get current directory") + err = os.Chdir(awDir) + require.NoError(t, err, "Failed to change directory") + defer func() { _ = os.Chdir(oldDir) }() + + // Parse the dispatcher workflow + workflowData, err := compiler.ParseWorkflowFile("dispatcher.md") + require.NoError(t, err, "Failed to parse workflow") + + // Validate the workflow - should return multiple errors + err = compiler.validateDispatchWorkflow(workflowData, dispatcherFile) + require.Error(t, err, "Validation should fail with multiple errors") + + // Check that all three errors are present in the aggregated error + errStr := err.Error() + assert.Contains(t, errStr, "Found 3 dispatch-workflow errors:", "Should have error count header") + assert.Contains(t, errStr, "self-reference", "Should contain self-reference error") + assert.Contains(t, errStr, "dispatcher", "Should mention dispatcher workflow") + assert.Contains(t, errStr, "workflow_dispatch", "Should contain workflow_dispatch error") + assert.Contains(t, errStr, "ci", "Should mention ci workflow") + assert.Contains(t, errStr, "not found", "Should contain not found error") + assert.Contains(t, errStr, "nonexistent", "Should mention nonexistent workflow") +} + +// TestDispatchWorkflowMultipleErrorsFailFast tests fail-fast mode stops at first error +func TestDispatchWorkflowMultipleErrorsFailFast(t *testing.T) { + compiler := NewCompilerWithVersion("1.0.0") + compiler.failFast = true // Enable fail-fast mode + + tmpDir := t.TempDir() + awDir := filepath.Join(tmpDir, ".github", "aw") + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + + err := os.MkdirAll(awDir, 0755) + require.NoError(t, err, "Failed to create aw directory") + err = os.MkdirAll(workflowsDir, 0755) + require.NoError(t, err, "Failed to create workflows directory") + + // Create dispatcher workflow with multiple errors + dispatcherWorkflow := `--- +on: issues +engine: copilot +permissions: + contents: read +safe-outputs: + dispatch-workflow: + workflows: + - dispatcher # Self-reference (first error) + - nonexistent # Not found (second error) + max: 2 +--- + +# Dispatcher Workflow +` + dispatcherFile := filepath.Join(awDir, "dispatcher.md") + err = os.WriteFile(dispatcherFile, []byte(dispatcherWorkflow), 0644) + require.NoError(t, err, "Failed to write dispatcher workflow") + + // Change to the aw directory + oldDir, err := os.Getwd() + require.NoError(t, err, "Failed to get current directory") + err = os.Chdir(awDir) + require.NoError(t, err, "Failed to change directory") + defer func() { _ = os.Chdir(oldDir) }() + + // Parse the dispatcher workflow + workflowData, err := compiler.ParseWorkflowFile("dispatcher.md") + require.NoError(t, err, "Failed to parse workflow") + + // Validate the workflow - should fail fast with first error only + err = compiler.validateDispatchWorkflow(workflowData, dispatcherFile) + require.Error(t, err, "Validation should fail") + + // In fail-fast mode, only the first error should be returned + errStr := err.Error() + assert.Contains(t, errStr, "self-reference", "Should contain first error") + assert.NotContains(t, errStr, "Found 2", "Should not have multiple error header in fail-fast mode") +} diff --git a/pkg/workflow/dispatch_workflow_validation.go b/pkg/workflow/dispatch_workflow_validation.go index 4245c8a736..95fb146ce6 100644 --- a/pkg/workflow/dispatch_workflow_validation.go +++ b/pkg/workflow/dispatch_workflow_validation.go @@ -31,18 +31,29 @@ func (c *Compiler) validateDispatchWorkflow(data *WorkflowData, workflowPath str currentWorkflowName := getCurrentWorkflowName(workflowPath) dispatchWorkflowValidationLog.Printf("Current workflow name: %s", currentWorkflowName) + // Collect all validation errors using ErrorCollector + collector := NewErrorCollector(c.failFast) + for _, workflowName := range config.Workflows { dispatchWorkflowValidationLog.Printf("Validating workflow: %s", workflowName) // Check for self-reference if workflowName == currentWorkflowName { - return fmt.Errorf("dispatch-workflow: self-reference not allowed (workflow '%s' cannot dispatch itself)", workflowName) + selfRefErr := fmt.Errorf("dispatch-workflow: self-reference not allowed (workflow '%s' cannot dispatch itself)", workflowName) + if returnErr := collector.Add(selfRefErr); returnErr != nil { + return returnErr // Fail-fast mode + } + continue // Skip further validation for this workflow } // Find the workflow file in multiple locations fileResult, err := findWorkflowFile(workflowName, workflowPath) if err != nil { - return fmt.Errorf("dispatch-workflow: error finding workflow '%s': %w", workflowName, err) + findErr := fmt.Errorf("dispatch-workflow: error finding workflow '%s': %w", workflowName, err) + if returnErr := collector.Add(findErr); returnErr != nil { + return returnErr // Fail-fast mode + } + continue // Skip further validation for this workflow } // Check if any workflow file exists @@ -53,8 +64,12 @@ func (c *Compiler) validateDispatchWorkflow(data *WorkflowData, workflowPath str repoRoot := filepath.Dir(githubDir) workflowsDir := filepath.Join(repoRoot, ".github", "workflows") - return fmt.Errorf("dispatch-workflow: workflow '%s' not found in %s (tried .md, .lock.yml, and .yml extensions)", + notFoundErr := fmt.Errorf("dispatch-workflow: workflow '%s' not found in %s (tried .md, .lock.yml, and .yml extensions)", workflowName, workflowsDir) + if returnErr := collector.Add(notFoundErr); returnErr != nil { + return returnErr // Fail-fast mode + } + continue // Skip further validation for this workflow } // Validate that the workflow supports workflow_dispatch @@ -67,29 +82,49 @@ func (c *Compiler) validateDispatchWorkflow(data *WorkflowData, workflowPath str workflowFile = fileResult.lockPath workflowContent, readErr = os.ReadFile(fileResult.lockPath) // #nosec G304 -- Path is validated via isPathWithinDir in findWorkflowFile if readErr != nil { - return fmt.Errorf("dispatch-workflow: failed to read workflow file %s: %w", fileResult.lockPath, readErr) + fileReadErr := fmt.Errorf("dispatch-workflow: failed to read workflow file %s: %w", fileResult.lockPath, readErr) + if returnErr := collector.Add(fileReadErr); returnErr != nil { + return returnErr // Fail-fast mode + } + continue // Skip further validation for this workflow } } else if fileResult.ymlExists { workflowFile = fileResult.ymlPath workflowContent, readErr = os.ReadFile(fileResult.ymlPath) // #nosec G304 -- Path is validated via isPathWithinDir in findWorkflowFile if readErr != nil { - return fmt.Errorf("dispatch-workflow: failed to read workflow file %s: %w", fileResult.ymlPath, readErr) + fileReadErr := fmt.Errorf("dispatch-workflow: failed to read workflow file %s: %w", fileResult.ymlPath, readErr) + if returnErr := collector.Add(fileReadErr); returnErr != nil { + return returnErr // Fail-fast mode + } + continue // Skip further validation for this workflow } } else { // Only .md exists - needs to be compiled first - return fmt.Errorf("dispatch-workflow: workflow '%s' must be compiled first (run 'gh aw compile %s')", workflowName, fileResult.mdPath) + compileErr := fmt.Errorf("dispatch-workflow: workflow '%s' must be compiled first (run 'gh aw compile %s')", workflowName, fileResult.mdPath) + if returnErr := collector.Add(compileErr); returnErr != nil { + return returnErr // Fail-fast mode + } + continue // Skip further validation for this workflow } // Parse the workflow YAML to check for workflow_dispatch trigger var workflow map[string]any if err := yaml.Unmarshal(workflowContent, &workflow); err != nil { - return fmt.Errorf("dispatch-workflow: failed to parse workflow file %s: %w", workflowFile, err) + parseErr := fmt.Errorf("dispatch-workflow: failed to parse workflow file %s: %w", workflowFile, err) + if returnErr := collector.Add(parseErr); returnErr != nil { + return returnErr // Fail-fast mode + } + continue // Skip further validation for this workflow } // Check if the workflow has an "on" section onSection, hasOn := workflow["on"] if !hasOn { - return fmt.Errorf("dispatch-workflow: workflow '%s' does not have an 'on' trigger section", workflowName) + onSectionErr := fmt.Errorf("dispatch-workflow: workflow '%s' does not have an 'on' trigger section", workflowName) + if returnErr := collector.Add(onSectionErr); returnErr != nil { + return returnErr // Fail-fast mode + } + continue // Skip further validation for this workflow } // Check if workflow_dispatch is in the "on" section @@ -114,14 +149,20 @@ func (c *Compiler) validateDispatchWorkflow(data *WorkflowData, workflowPath str } if !hasWorkflowDispatch { - return fmt.Errorf("dispatch-workflow: workflow '%s' does not support workflow_dispatch trigger (must include 'workflow_dispatch' in the 'on' section)", workflowName) + dispatchErr := fmt.Errorf("dispatch-workflow: workflow '%s' does not support workflow_dispatch trigger (must include 'workflow_dispatch' in the 'on' section)", workflowName) + if returnErr := collector.Add(dispatchErr); returnErr != nil { + return returnErr // Fail-fast mode + } + continue // Skip further validation for this workflow } dispatchWorkflowValidationLog.Printf("Workflow '%s' is valid for dispatch (found in %s)", workflowName, workflowFile) } - dispatchWorkflowValidationLog.Printf("All %d workflows validated successfully", len(config.Workflows)) - return nil + dispatchWorkflowValidationLog.Printf("Dispatch workflow validation completed: error_count=%d, total_workflows=%d", collector.Count(), len(config.Workflows)) + + // Return aggregated errors with formatted output + return collector.FormattedError("dispatch-workflow") } // extractWorkflowDispatchInputs parses a workflow file and extracts the workflow_dispatch inputs schema diff --git a/pkg/workflow/repository_features_validation.go b/pkg/workflow/repository_features_validation.go index 3de406e98a..16686a5150 100644 --- a/pkg/workflow/repository_features_validation.go +++ b/pkg/workflow/repository_features_validation.go @@ -111,6 +111,9 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error repositoryFeaturesLog.Printf("Checking repository features for: %s", repo) + // Collect all validation errors using ErrorCollector + collector := NewErrorCollector(c.failFast) + // Check if discussions are enabled when create-discussion or add-comment with discussion: true is configured needsDiscussions := workflowData.SafeOutputs.CreateDiscussions != nil || (workflowData.SafeOutputs.AddComments != nil && @@ -128,10 +131,8 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error fmt.Fprintln(os.Stderr, console.FormatWarningMessage( fmt.Sprintf("Could not verify if discussions are enabled: %v", err))) } - return nil - } - - if !hasDiscussions { + // Continue checking other features even if this check fails + } else if !hasDiscussions { // Changed to warning instead of error per issue feedback // Strategy: Always try to create the discussion at runtime and investigate if it fails // The runtime create_discussion handler will provide better error messages if creation fails @@ -146,7 +147,7 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error if c.verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg)) } - // Don't return error - allow compilation to proceed and let runtime handle the actual attempt + // Don't add to error collector - this is a warning, not an error } } @@ -161,15 +162,19 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error fmt.Fprintln(os.Stderr, console.FormatWarningMessage( fmt.Sprintf("Could not verify if issues are enabled: %v", err))) } - return nil - } - - if !hasIssues { - return fmt.Errorf("workflow uses safe-outputs.create-issue but repository %s does not have issues enabled. Enable issues in repository settings or remove create-issue from safe-outputs", repo) + // Continue to return aggregated errors even if this check fails + } else if !hasIssues { + issueErr := fmt.Errorf("workflow uses safe-outputs.create-issue but repository %s does not have issues enabled. Enable issues in repository settings or remove create-issue from safe-outputs", repo) + if returnErr := collector.Add(issueErr); returnErr != nil { + return returnErr // Fail-fast mode + } } } - return nil + repositoryFeaturesLog.Printf("Repository features validation completed: error_count=%d", collector.Count()) + + // Return aggregated errors with formatted output + return collector.FormattedError("repository features") } // getCurrentRepository gets the current repository from git context (with caching) diff --git a/pkg/workflow/template_include_validation_test.go b/pkg/workflow/template_include_validation_test.go index 04dd33e0a1..2836a719bf 100644 --- a/pkg/workflow/template_include_validation_test.go +++ b/pkg/workflow/template_include_validation_test.go @@ -398,3 +398,72 @@ No indent }) } } + +// TestValidateNoIncludesInTemplateRegions_MultipleErrors tests that multiple errors are aggregated +func TestValidateNoIncludesInTemplateRegions_MultipleErrors(t *testing.T) { + // Input with multiple includes inside different template regions + input := `# Test Workflow + +{{#if github.event.issue.number}} +@include shared/issue-tools.md +Some content here. +{{/if}} + +{{#if github.actor}} +@import shared/actor-config.md +More content here. +{{/if}} + +{{#if github.repository}} +@include? shared/optional.md +Even more content. +{{/if}}` + + err := validateNoIncludesInTemplateRegions(input) + + // Should return aggregated error with all three violations + if err == nil { + t.Fatal("validateNoIncludesInTemplateRegions() expected error, got nil") + } + + errStr := err.Error() + + // Check that all three errors are present + if !strings.Contains(errStr, "issue-tools.md") { + t.Errorf("Error should contain first violation: issue-tools.md") + } + if !strings.Contains(errStr, "actor-config.md") { + t.Errorf("Error should contain second violation: actor-config.md") + } + if !strings.Contains(errStr, "optional.md") { + t.Errorf("Error should contain third violation: optional.md") + } + + // Check for newline-separated errors (errors.Join behavior) + errorLines := strings.Split(errStr, "\n") + if len(errorLines) < 3 { + t.Errorf("Expected at least 3 error lines, got %d", len(errorLines)) + } +} + +// TestValidateNoIncludesInTemplateRegions_SingleError tests single error behavior +func TestValidateNoIncludesInTemplateRegions_SingleError(t *testing.T) { + // Input with single include inside template region + input := `# Test Workflow + +{{#if github.event.issue.number}} +@include shared/tools.md +{{/if}}` + + err := validateNoIncludesInTemplateRegions(input) + + // Should return single error (not aggregated) + if err == nil { + t.Fatal("validateNoIncludesInTemplateRegions() expected error, got nil") + } + + errStr := err.Error() + if !strings.Contains(errStr, "tools.md") { + t.Errorf("Error should contain violation: tools.md") + } +} diff --git a/pkg/workflow/template_validation.go b/pkg/workflow/template_validation.go index 455662edb2..59583d1a24 100644 --- a/pkg/workflow/template_validation.go +++ b/pkg/workflow/template_validation.go @@ -30,6 +30,7 @@ package workflow import ( + "errors" "fmt" "regexp" "strings" @@ -56,6 +57,9 @@ func validateNoIncludesInTemplateRegions(markdown string) error { matches := templateRegionPattern.FindAllStringSubmatch(markdown, -1) templateValidationLog.Printf("Found %d template regions to validate", len(matches)) + // Collect all validation errors + var errs []error + for _, match := range matches { if len(match) < 2 { continue @@ -71,10 +75,17 @@ func validateNoIncludesInTemplateRegions(markdown string) error { trimmedLine := strings.TrimSpace(line) directive := parser.ParseImportDirective(trimmedLine) if directive != nil { - return fmt.Errorf("import directives cannot be used inside template regions ({{#if...}}{{/if}}): found '%s' at line %d within template block", directive.Original, lineNum+1) + importErr := fmt.Errorf("import directives cannot be used inside template regions ({{#if...}}{{/if}}): found '%s' at line %d within template block", directive.Original, lineNum+1) + errs = append(errs, importErr) } } } + // Return aggregated errors + if len(errs) > 0 { + templateValidationLog.Printf("Found %d template validation errors", len(errs)) + return errors.Join(errs...) + } + return nil }