fix: inject detection guard condition into custom steps and improve test ordering assertions

Copilot · pelikhan · web-flow · commit 92d06e0fabe3 · 2026-04-03T13:49:11.000Z
- Inject detectionStepCondition into custom threat detection steps (pre/post) unless the user already provides an if: condition. This ensures custom steps are skipped when detection_guard says run_detection is false. - Update test to verify post-step ordering relative to id: detection_agentic_execution (stable engine step marker) instead of just "Setup threat detection" - Add TestCustomThreatDetectionStepsGuardCondition to explicitly test condition injection and preservation Agent-Logs-Url: https://github.com/github/gh-aw/sessions/db33799b-37fd-4489-96df-09ea24078ed9 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
diff --git a/pkg/workflow/threat_detection.go b/pkg/workflow/threat_detection.go
@@ -3,6 +3,7 @@ package workflow
 import (
 	"encoding/json"
 	"fmt"
+	"maps"
 	"strings"
 
 	"github.com/github/gh-aw/pkg/constants"
@@ -567,10 +568,21 @@ await main();`
 }
 
 // buildCustomThreatDetectionSteps builds YAML steps from user-configured threat detection steps.
+// It injects the detection guard condition into each step unless an explicit if: condition is
+// already set, ensuring custom steps only run when the detection_guard determines that detection
+// should proceed and preventing unexpected side effects in runs with no agent outputs to analyze.
 func (c *Compiler) buildCustomThreatDetectionSteps(steps []any) []string {
 	var result []string
 	for _, step := range steps {
 		if stepMap, ok := step.(map[string]any); ok {
+			// Inject the detection guard condition unless the user already provided an if: condition.
+			if _, hasIf := stepMap["if"]; !hasIf {
+				// Clone the map to avoid mutating the original config.
+				injected := make(map[string]any, len(stepMap)+1)
+				maps.Copy(injected, stepMap)
+				injected["if"] = detectionStepCondition
+				stepMap = injected
+			}
 			if stepYAML, err := ConvertStepToYAML(stepMap); err == nil {
 				result = append(result, stepYAML)
 			}
diff --git a/pkg/workflow/threat_detection_test.go b/pkg/workflow/threat_detection_test.go
@@ -447,15 +447,16 @@ func TestThreatDetectionStepsOrdering(t *testing.T) {
 		stepsString := strings.Join(steps, "")
 
 		postStepPos := strings.Index(stepsString, "Custom Post Scan")
-		setupStepPos := strings.Index(stepsString, "Setup threat detection")
+		// Use the engine execution step ID as the stable marker for the engine step boundary
+		engineStepPos := strings.Index(stepsString, "id: detection_agentic_execution")
 		uploadStepPos := strings.Index(stepsString, "Upload threat detection log")
 		concludeStepPos := strings.Index(stepsString, "Parse and conclude threat detection")
 
 		if postStepPos == -1 {
 			t.Error("Expected to find 'Custom Post Scan' step")
 		}
-		if setupStepPos == -1 {
-			t.Error("Expected to find 'Setup threat detection' step")
+		if engineStepPos == -1 {
+			t.Error("Expected to find 'id: detection_agentic_execution' engine step")
 		}
 		if uploadStepPos == -1 {
 			t.Error("Expected to find 'Upload threat detection log' step")
@@ -464,9 +465,9 @@ func TestThreatDetectionStepsOrdering(t *testing.T) {
 			t.Error("Expected to find 'Parse and conclude threat detection' step")
 		}
 
-		// Verify ordering: post-steps should come after setup and before upload/conclude
-		if postStepPos < setupStepPos {
-			t.Errorf("Custom post-steps should come after 'Setup threat detection'. Got post-step at position %d, setup at position %d", postStepPos, setupStepPos)
+		// Verify ordering: post-steps should come after the engine execution step
+		if postStepPos < engineStepPos {
+			t.Errorf("Custom post-steps should come after engine execution step. Got post-step at position %d, engine at position %d", postStepPos, engineStepPos)
 		}
 		if postStepPos > uploadStepPos {
 			t.Errorf("Custom post-steps should come before 'Upload threat detection log'. Got post-step at position %d, upload at position %d", postStepPos, uploadStepPos)
@@ -501,7 +502,7 @@ func TestThreatDetectionStepsOrdering(t *testing.T) {
 
 		preStepPos := strings.Index(stepsString, "Custom Pre Step")
 		postStepPos := strings.Index(stepsString, "Custom Post Step")
-		setupStepPos := strings.Index(stepsString, "Setup threat detection")
+		engineStepPos := strings.Index(stepsString, "id: detection_agentic_execution")
 		uploadStepPos := strings.Index(stepsString, "Upload threat detection log")
 
 		if preStepPos == -1 {
@@ -510,13 +511,16 @@ func TestThreatDetectionStepsOrdering(t *testing.T) {
 		if postStepPos == -1 {
 			t.Error("Expected to find 'Custom Post Step'")
 		}
+		if engineStepPos == -1 {
+			t.Error("Expected to find 'id: detection_agentic_execution' engine step")
+		}
 
-		// pre-steps before setup, post-steps after setup but before upload
-		if preStepPos > setupStepPos {
-			t.Errorf("Pre-steps should come before 'Setup threat detection'. Got pre=%d, setup=%d", preStepPos, setupStepPos)
+		// pre-steps before engine, post-steps after engine but before upload
+		if preStepPos > engineStepPos {
+			t.Errorf("Pre-steps should come before engine execution step. Got pre=%d, engine=%d", preStepPos, engineStepPos)
 		}
-		if postStepPos < setupStepPos {
-			t.Errorf("Post-steps should come after 'Setup threat detection'. Got post=%d, setup=%d", postStepPos, setupStepPos)
+		if postStepPos < engineStepPos {
+			t.Errorf("Post-steps should come after engine execution step. Got post=%d, engine=%d", postStepPos, engineStepPos)
 		}
 		if postStepPos > uploadStepPos {
 			t.Errorf("Post-steps should come before 'Upload threat detection log'. Got post=%d, upload=%d", postStepPos, uploadStepPos)
@@ -528,6 +532,43 @@ func TestThreatDetectionStepsOrdering(t *testing.T) {
 	})
 }
 
+func TestCustomThreatDetectionStepsGuardCondition(t *testing.T) {
+	compiler := NewCompiler()
+
+	t.Run("injects detection guard condition when no if: present", func(t *testing.T) {
+		steps := []any{
+			map[string]any{
+				"name": "No If Step",
+				"run":  "echo hello",
+			},
+		}
+		result := compiler.buildCustomThreatDetectionSteps(steps)
+		stepsStr := strings.Join(result, "")
+		if !strings.Contains(stepsStr, detectionStepCondition) {
+			t.Errorf("Expected detection guard condition to be injected, got:\n%s", stepsStr)
+		}
+	})
+
+	t.Run("preserves user-provided if: condition", func(t *testing.T) {
+		userCondition := "always()"
+		steps := []any{
+			map[string]any{
+				"name": "User If Step",
+				"if":   userCondition,
+				"run":  "echo hello",
+			},
+		}
+		result := compiler.buildCustomThreatDetectionSteps(steps)
+		stepsStr := strings.Join(result, "")
+		if strings.Contains(stepsStr, detectionStepCondition) {
+			t.Error("Expected detection guard condition NOT to be injected when user provides if:")
+		}
+		if !strings.Contains(stepsStr, userCondition) {
+			t.Errorf("Expected user if: condition %q to be preserved, got:\n%s", userCondition, stepsStr)
+		}
+	})
+}
+
 func TestBuildDetectionEngineExecutionStepWithThreatDetectionEngine(t *testing.T) {
 	compiler := NewCompiler()
 
