Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions pkg/workflow/error_aggregation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change — using collector.Count() directly instead of the removed HasErrors() method makes the intent clearer and eliminates the need for a separate predicate method.

assert.Equal(t, 0, collector.Count(), "New collector should have zero count")
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two assertions both call collector.Count() with essentially the same expectation/message. Consider keeping a single assertion here to avoid redundant checks and reduce noise in the test output when it fails.

This issue also appears on line 82 of the same file.

Suggested change
assert.Equal(t, 0, collector.Count(), "New collector should have zero count")

Copilot uses AI. Check for mistakes.
})
}
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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")
}

Expand Down
32 changes: 0 additions & 32 deletions pkg/workflow/safe_outputs_runs_on_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
12 changes: 0 additions & 12 deletions pkg/workflow/safe_outputs_runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.).
//
Expand Down
47 changes: 0 additions & 47 deletions pkg/workflow/safe_outputs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"strings"
"testing"

"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/stringutil"
)

Expand Down Expand Up @@ -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
// ========================================
Expand Down
14 changes: 0 additions & 14 deletions pkg/workflow/safe_scripts.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing extractSafeScriptsFromFrontmatter keeps the package surface clean. The parseSafeScriptsConfig helper below is the right entry point for callers that already have the scripts map.

// 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.
Expand Down
33 changes: 0 additions & 33 deletions pkg/workflow/safe_scripts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Comment on lines 70 to 72
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment says “TestExtractSafeScriptsFromFrontmatter verifies extraction from frontmatter”, but the TestExtractSafeScriptsFromFrontmatter* tests were removed. Please remove or update the comment so it doesn’t suggest a missing test.

Copilot uses AI. Check for mistakes.
data := &WorkflowData{
Expand Down
7 changes: 0 additions & 7 deletions pkg/workflow/threat_detection.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 2 additions & 5 deletions pkg/workflow/threat_detection_file_access_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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(<cmd>) entries.
assert.Contains(t, stepsString, "Bash", "threat detection should include unrestricted Bash tool")
}

Expand Down
98 changes: 4 additions & 94 deletions pkg/workflow/threat_detection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down
5 changes: 0 additions & 5 deletions pkg/workflow/workflow_errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading