diff --git a/pkg/workflow/error_aggregation_test.go b/pkg/workflow/error_aggregation_test.go index 238d78d0c19..c07dc1afd0d 100644 --- a/pkg/workflow/error_aggregation_test.go +++ b/pkg/workflow/error_aggregation_test.go @@ -30,7 +30,7 @@ func TestNewErrorCollector(t *testing.T) { collector := NewErrorCollector(tt.failFast) require.NotNil(t, collector, "Collector should be created") assert.Equal(t, tt.failFast, collector.failFast, "Fail-fast setting should match") - assert.False(t, collector.HasErrors(), "New collector should have no errors") + assert.Equal(t, 0, collector.Count(), "New collector should have no errors") assert.Equal(t, 0, collector.Count(), "New collector should have zero count") }) } @@ -45,7 +45,7 @@ func TestErrorCollectorAdd_FailFast(t *testing.T) { result := collector.Add(err1) require.Error(t, result, "Should return error immediately in fail-fast mode") assert.Equal(t, err1, result, "Should return the exact error") - assert.False(t, collector.HasErrors(), "Should not collect errors in fail-fast mode") + assert.Equal(t, 0, collector.Count(), "Should not collect errors in fail-fast mode") // Second error should also be returned immediately result = collector.Add(err2) @@ -62,7 +62,7 @@ func TestErrorCollectorAdd_Aggregate(t *testing.T) { // Add errors should not return them result := collector.Add(err1) require.NoError(t, result, "Should not return error in aggregate mode") - assert.True(t, collector.HasErrors(), "Should have errors") + assert.Greater(t, collector.Count(), 0, "Should have errors") assert.Equal(t, 1, collector.Count(), "Should have 1 error") result = collector.Add(err2) @@ -79,7 +79,7 @@ func TestErrorCollectorAdd_NilError(t *testing.T) { result := collector.Add(nil) require.NoError(t, result, "Should handle nil error") - assert.False(t, collector.HasErrors(), "Should not have errors") + assert.Equal(t, 0, collector.Count(), "Should not have errors") assert.Equal(t, 0, collector.Count(), "Should have zero count") } diff --git a/pkg/workflow/safe_outputs_runs_on_test.go b/pkg/workflow/safe_outputs_runs_on_test.go index 0f41a023c6f..542651028e0 100644 --- a/pkg/workflow/safe_outputs_runs_on_test.go +++ b/pkg/workflow/safe_outputs_runs_on_test.go @@ -153,38 +153,6 @@ This is a test workflow.` } } -func TestFormatSafeOutputsRunsOnEdgeCases(t *testing.T) { - compiler := NewCompiler() - - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - expectedRunsOn string - }{ - { - name: "nil safe outputs config", - safeOutputs: nil, - expectedRunsOn: "runs-on: " + constants.DefaultActivationJobRunnerImage, - }, - { - name: "safe outputs config with nil runs-on", - safeOutputs: &SafeOutputsConfig{ - RunsOn: "", - }, - expectedRunsOn: "runs-on: " + constants.DefaultActivationJobRunnerImage, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - runsOn := compiler.formatSafeOutputsRunsOn(tt.safeOutputs) - if runsOn != tt.expectedRunsOn { - t.Errorf("Expected runs-on to be %q, got %q", tt.expectedRunsOn, runsOn) - } - }) - } -} - func TestUnlockJobUsesRunsOn(t *testing.T) { frontmatter := `--- on: diff --git a/pkg/workflow/safe_outputs_runtime.go b/pkg/workflow/safe_outputs_runtime.go index 1880d7043dc..04155866f94 100644 --- a/pkg/workflow/safe_outputs_runtime.go +++ b/pkg/workflow/safe_outputs_runtime.go @@ -15,18 +15,6 @@ var safeOutputsRuntimeLog = logger.New("workflow:safe_outputs_runtime") // (runner images) for safe-outputs jobs and detect feature usage patterns // that affect job configuration. -// formatSafeOutputsRunsOn formats the runs-on value from SafeOutputsConfig for job output. -// Falls back to the default activation job runner image when not explicitly set. -func (c *Compiler) formatSafeOutputsRunsOn(safeOutputs *SafeOutputsConfig) string { - if safeOutputs == nil || safeOutputs.RunsOn == "" { - safeOutputsRuntimeLog.Printf("Safe outputs runs-on not set, using default: %s", constants.DefaultActivationJobRunnerImage) - return "runs-on: " + constants.DefaultActivationJobRunnerImage - } - - safeOutputsRuntimeLog.Printf("Safe outputs runs-on: %s", safeOutputs.RunsOn) - return "runs-on: " + safeOutputs.RunsOn -} - // formatFrameworkJobRunsOn returns the runs-on value for framework/generated jobs // (activation, pre-activation, safe-outputs, unlock, APM, etc.). // diff --git a/pkg/workflow/safe_outputs_test.go b/pkg/workflow/safe_outputs_test.go index 0d2655e48a2..456a5224fc5 100644 --- a/pkg/workflow/safe_outputs_test.go +++ b/pkg/workflow/safe_outputs_test.go @@ -7,7 +7,6 @@ import ( "strings" "testing" - "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/stringutil" ) @@ -699,52 +698,6 @@ func TestGenerateSafeOutputsConfig(t *testing.T) { // FormatSafeOutputsRunsOn Tests // ======================================== -// TestFormatSafeOutputsRunsOn tests the formatSafeOutputsRunsOn function -func TestFormatSafeOutputsRunsOn(t *testing.T) { - compiler := NewCompiler() - - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - expectedRunsOn string - }{ - { - name: "nil safe outputs returns default", - safeOutputs: nil, - expectedRunsOn: "runs-on: " + constants.DefaultActivationJobRunnerImage, - }, - { - name: "empty runs-on returns default", - safeOutputs: &SafeOutputsConfig{RunsOn: ""}, - expectedRunsOn: "runs-on: " + constants.DefaultActivationJobRunnerImage, - }, - { - name: "custom runs-on", - safeOutputs: &SafeOutputsConfig{RunsOn: "ubuntu-latest"}, - expectedRunsOn: "runs-on: ubuntu-latest", - }, - { - name: "self-hosted runs-on", - safeOutputs: &SafeOutputsConfig{RunsOn: "self-hosted"}, - expectedRunsOn: "runs-on: self-hosted", - }, - { - name: "windows-latest runs-on", - safeOutputs: &SafeOutputsConfig{RunsOn: "windows-latest"}, - expectedRunsOn: "runs-on: windows-latest", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := compiler.formatSafeOutputsRunsOn(tt.safeOutputs) - if result != tt.expectedRunsOn { - t.Errorf("formatSafeOutputsRunsOn() = %q, want %q", result, tt.expectedRunsOn) - } - }) - } -} - // ======================================== // BuildWorkflowMetadataEnvVarsWithTrackerID Tests // ======================================== diff --git a/pkg/workflow/safe_scripts.go b/pkg/workflow/safe_scripts.go index b14eb100130..1d2b0b42724 100644 --- a/pkg/workflow/safe_scripts.go +++ b/pkg/workflow/safe_scripts.go @@ -75,20 +75,6 @@ func parseSafeScriptsConfig(scriptsMap map[string]any) map[string]*SafeScriptCon return result } -// extractSafeScriptsFromFrontmatter extracts safe-scripts configuration from frontmatter. -func extractSafeScriptsFromFrontmatter(frontmatter map[string]any) map[string]*SafeScriptConfig { - if safeOutputs, exists := frontmatter["safe-outputs"]; exists { - if safeOutputsMap, ok := safeOutputs.(map[string]any); ok { - if scripts, exists := safeOutputsMap["scripts"]; exists { - if scriptsMap, ok := scripts.(map[string]any); ok { - return parseSafeScriptsConfig(scriptsMap) - } - } - } - } - return make(map[string]*SafeScriptConfig) -} - // isSafeScriptName returns true if the script name is safe for use as a filename component. // It rejects names that contain path separators or ".." sequences that could lead to // path traversal when the generated filename is passed to require() at runtime. diff --git a/pkg/workflow/safe_scripts_test.go b/pkg/workflow/safe_scripts_test.go index 8a6f58e07f1..92d13a833fb 100644 --- a/pkg/workflow/safe_scripts_test.go +++ b/pkg/workflow/safe_scripts_test.go @@ -68,39 +68,6 @@ func TestParseSafeScriptsConfigNilMap(t *testing.T) { } // TestExtractSafeScriptsFromFrontmatter verifies extraction from frontmatter -func TestExtractSafeScriptsFromFrontmatter(t *testing.T) { - frontmatter := map[string]any{ - "safe-outputs": map[string]any{ - "scripts": map[string]any{ - "my-handler": map[string]any{ - "description": "A custom handler", - // Users write only the body — no module.exports or main declaration needed - "script": "return async (m) => ({ success: true });", - }, - }, - }, - } - - result := extractSafeScriptsFromFrontmatter(frontmatter) - - require.Len(t, result, 1, "Should have one script") - script, exists := result["my-handler"] - require.True(t, exists, "Should have my-handler script") - assert.Equal(t, "A custom handler", script.Description, "Description should match") -} - -// TestExtractSafeScriptsFromFrontmatterEmpty verifies empty result when no scripts -func TestExtractSafeScriptsFromFrontmatterEmpty(t *testing.T) { - frontmatter := map[string]any{ - "safe-outputs": map[string]any{ - "create-issue": map[string]any{}, - }, - } - - result := extractSafeScriptsFromFrontmatter(frontmatter) - assert.Empty(t, result, "Should return empty map when no scripts") -} - // TestBuildCustomSafeOutputScriptsJSON verifies JSON generation for script env var func TestBuildCustomSafeOutputScriptsJSON(t *testing.T) { data := &WorkflowData{ diff --git a/pkg/workflow/threat_detection.go b/pkg/workflow/threat_detection.go index 5d0620f7dda..6fb5a457677 100644 --- a/pkg/workflow/threat_detection.go +++ b/pkg/workflow/threat_detection.go @@ -172,13 +172,6 @@ func (c *Compiler) buildDetectionJobSteps(data *WorkflowData) []string { return steps } -// buildInlineDetectionSteps is kept for backward compatibility but no longer inlines detection -// into the agent job. Detection is now handled by the separate detection job. -// Deprecated: use buildDetectionJobSteps instead. -func (c *Compiler) buildInlineDetectionSteps(data *WorkflowData) []string { - return c.buildDetectionJobSteps(data) -} - // buildPullAWFContainersStep creates a step that pre-pulls AWF (agent workflow firewall) // container images in the detection job. The detection engine runs inside AWF, which uses // three containers (squid, agent, api-proxy). Pre-pulling avoids on-demand pulls at runtime. diff --git a/pkg/workflow/threat_detection_file_access_test.go b/pkg/workflow/threat_detection_file_access_test.go index fe2f560447a..6a933b3853b 100644 --- a/pkg/workflow/threat_detection_file_access_test.go +++ b/pkg/workflow/threat_detection_file_access_test.go @@ -38,7 +38,7 @@ func TestThreatDetectionSteps_UseFilePathReferences(t *testing.T) { compiler := createTestCompiler(t) data := createTestWorkflowData(t, &ThreatDetectionConfig{}) - steps := compiler.buildInlineDetectionSteps(data) + steps := compiler.buildDetectionJobSteps(data) stepsString := strings.Join(steps, "") tests := []struct { @@ -97,11 +97,8 @@ func TestThreatDetectionSteps_IncludeBashReadTools(t *testing.T) { compiler := createTestCompiler(t) data := createTestWorkflowData(t, &ThreatDetectionConfig{}) - steps := compiler.buildInlineDetectionSteps(data) + steps := compiler.buildDetectionJobSteps(data) stepsString := strings.Join(steps, "") - - // Detection uses bash: ["*"] — the compiled YAML lists "Bash" (unrestricted) rather - // than individual Bash() entries. assert.Contains(t, stepsString, "Bash", "threat detection should include unrestricted Bash tool") } diff --git a/pkg/workflow/threat_detection_test.go b/pkg/workflow/threat_detection_test.go index 7d4b5d3cda6..b146061e3ba 100644 --- a/pkg/workflow/threat_detection_test.go +++ b/pkg/workflow/threat_detection_test.go @@ -153,96 +153,6 @@ func TestParseThreatDetectionConfig(t *testing.T) { } } -func TestBuildInlineDetectionSteps(t *testing.T) { - compiler := NewCompiler() - - tests := []struct { - name string - data *WorkflowData - expectNil bool - expectSteps bool - }{ - { - name: "threat detection disabled (nil) should return nil", - data: &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - ThreatDetection: nil, - }, - }, - expectNil: true, - expectSteps: false, - }, - { - name: "threat detection enabled should create inline steps", - data: &WorkflowData{ - RunsOn: "runs-on: ubuntu-latest", - SafeOutputs: &SafeOutputsConfig{ - ThreatDetection: &ThreatDetectionConfig{}, - }, - }, - expectNil: false, - expectSteps: true, - }, - { - name: "threat detection with custom steps should create inline steps", - data: &WorkflowData{ - RunsOn: "runs-on: ubuntu-latest", - SafeOutputs: &SafeOutputsConfig{ - ThreatDetection: &ThreatDetectionConfig{ - Steps: []any{ - map[string]any{ - "name": "Custom step", - "run": "echo 'custom validation'", - }, - }, - }, - }, - }, - expectNil: false, - expectSteps: true, - }, - { - name: "nil safe outputs should return nil", - data: &WorkflowData{ - SafeOutputs: nil, - }, - expectNil: true, - expectSteps: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - steps := compiler.buildInlineDetectionSteps(tt.data) - - if tt.expectNil && steps != nil { - t.Errorf("Expected nil steps, got %d lines", len(steps)) - } - if !tt.expectNil && steps == nil { - t.Errorf("Expected non-nil steps, got nil") - } - - if tt.expectSteps { - joined := strings.Join(steps, "") - // Verify key detection step components - if !strings.Contains(joined, "detection_guard") { - t.Error("Expected inline steps to contain detection_guard step") - } - if !strings.Contains(joined, "detection_conclusion") { - t.Error("Expected inline steps to contain detection_conclusion step") - } - // The combined conclusion step should call parse_threat_detection_results.cjs - if !strings.Contains(joined, "parse_threat_detection_results.cjs") { - t.Error("Expected inline steps to reference parse_threat_detection_results.cjs") - } - if !strings.Contains(joined, "Threat Detection") { - t.Error("Expected inline steps to contain threat detection comment separator") - } - } - }) - } -} - func TestThreatDetectionDefaultBehavior(t *testing.T) { compiler := NewCompiler() @@ -296,7 +206,7 @@ func TestThreatDetectionInlineStepsDependencies(t *testing.T) { } // Build inline detection steps - steps := compiler.buildInlineDetectionSteps(data) + steps := compiler.buildDetectionJobSteps(data) if steps == nil { t.Fatal("Expected inline detection steps to be created") } @@ -334,7 +244,7 @@ func TestThreatDetectionCustomPrompt(t *testing.T) { }, } - steps := compiler.buildInlineDetectionSteps(data) + steps := compiler.buildDetectionJobSteps(data) if steps == nil { t.Fatal("Expected inline detection steps to be created") } @@ -437,7 +347,7 @@ func TestThreatDetectionStepsOrdering(t *testing.T) { }, } - steps := compiler.buildInlineDetectionSteps(data) + steps := compiler.buildDetectionJobSteps(data) if len(steps) == 0 { t.Fatal("Expected non-empty steps") @@ -591,7 +501,7 @@ func TestThreatDetectionStepsIncludeUpload(t *testing.T) { }, } - steps := compiler.buildInlineDetectionSteps(data) + steps := compiler.buildDetectionJobSteps(data) if len(steps) == 0 { t.Fatal("Expected non-empty steps") diff --git a/pkg/workflow/workflow_errors.go b/pkg/workflow/workflow_errors.go index 2e63d1ffc29..20841f42cd6 100644 --- a/pkg/workflow/workflow_errors.go +++ b/pkg/workflow/workflow_errors.go @@ -215,11 +215,6 @@ func (c *ErrorCollector) Add(err error) error { return nil } -// HasErrors returns true if any errors have been collected -func (c *ErrorCollector) HasErrors() bool { - return len(c.errors) > 0 -} - // Count returns the number of errors collected func (c *ErrorCollector) Count() int { return len(c.errors)