fix: detection job missing engine.env job dependencies in needs list

docs/adr/44202-detection-job-engine-env-needs-scan.md

@@ -0,0 +1,51 @@
+# ADR-44202: Detection Job Needs Derived from Engine Env Expressions
+
+**Date**: 2026-07-08
+**Status**: Draft
+**Deciders**: pelikhan, Copilot SWE Agent
+
+---
+
+### Context
+
+The generated `detection` job inherits environment variables from the engine configuration used for threat detection. Some workflows set those values with expressions such as `${{ needs.select_model.outputs.model }}`, where the expression is only valid if the detection job directly depends on the referenced custom job.
+
+Before this change, the detection job always used `needs: [agent, activation]` and did not scan its effective engine environment for `needs.<job>.outputs.*` references. As a result, expressions that resolved correctly in the `agent` and `conclusion` jobs silently evaluated to empty strings in the detection job at runtime. The detection job also supports a `safe-outputs.threat-detection.engine` override, so dependency inference must follow the effective detection engine env rather than always reading the top-level engine config.
+
+### Decision
+
+`buildDetectionJob` will derive additional direct dependencies from the effective detection engine env. The compiler starts with the required built-in dependencies (`agent` and `activation`), scans the env values for `needs.<customJob>.outputs.*` references, appends the referenced custom jobs, and deduplicates the resulting `needs` list.
+
+When `safe-outputs.threat-detection.engine.env` is present, it overrides the top-level `engine.env` for this analysis, matching the execution path already used by the detection job. If the env references a built-in job that cannot become a direct detection dependency, the compiler emits the same warning pattern already used by the main agent job so the workflow author sees the misconfiguration at compile time.
+
+### Alternatives Considered
+
+#### Alternative 1: Leave the detection job dependency list unchanged
+
+We could keep the detection job limited to `agent` and `activation` and document that `engine.env` `needs.*` expressions are unsupported there. This was rejected because the detection job already inherits those env values, so leaving the dependency gap in place would preserve a silent runtime failure mode for otherwise valid workflow configurations.
+
+#### Alternative 2: Reuse the top-level engine env scan without honoring the detection override
+
+We could mirror the existing main-job logic by scanning only `data.EngineConfig.Env`. This was rejected because it would compute dependencies from the wrong configuration whenever `safe-outputs.threat-detection.engine.env` overrides the top-level env, which can both miss required jobs and add unnecessary ones.
+
+### Consequences
+
+#### Positive
+
+- Detection-job env expressions that reference custom job outputs now resolve correctly at runtime.
+- Detection-job dependency inference now matches the effective threat-detection engine configuration instead of a possibly stale top-level env.
+- Built-in-job references in detection env expressions now surface as compile-time warnings instead of failing silently.
+
+#### Negative
+
+- The detection job may wait for additional custom jobs before starting, which can lengthen workflow execution when authors reference more upstream outputs.
+- Workflows that accidentally relied on the previous empty-string behavior will now observe the intended dependency ordering and warning output.
+
+#### Neutral
+
+- The change is limited to detection-job dependency inference and warning behavior; it does not change how agent output artifacts or detection steps are executed once the job starts.
+- Regression coverage now includes override, deduplication, built-in warning, and compiled-lock assertions for the detection job path.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/workflow/compiler_jobs_test.go

@@ -4071,3 +4071,236 @@ func TestBuildMainJobEngineEnvActivationNoFalseWarning(t *testing.T) {
 	assert.Equal(t, initialWarnings, compiler.GetWarningCount(),
 		"no warning should be emitted for activation which is already a direct agent dependency")
 }
+
+// TestBuildDetectionJobEngineEnvBuiltinWarning verifies that detection-job engine.env
+// references to built-in jobs that are not direct detection dependencies emit a compiler warning.
+func TestBuildDetectionJobEngineEnvBuiltinWarning(t *testing.T) {
+	compiler := NewCompiler()
+	compiler.stepOrderTracker = NewStepOrderTracker()
+
+	workflowData := &WorkflowData{
+		Name:        "Test Workflow",
+		AI:          "copilot",
+		RunsOn:      "runs-on: ubuntu-latest",
+		Permissions: "permissions:\n  contents: read",
+		EngineConfig: &EngineConfig{
+			ID: "copilot",
+		},
+		SafeOutputs: &SafeOutputsConfig{
+			ThreatDetection: &ThreatDetectionConfig{
+				EngineConfig: &EngineConfig{
+					ID: "copilot",
+					Env: map[string]string{
+						"BUILTIN_JOB_REF": "${{ needs.safe_outputs.outputs.anything }}",
+					},
+				},
+			},
+			Jobs: map[string]*SafeJobConfig{"create-issue": {}},
+		},
+	}
+
+	initialWarnings := compiler.GetWarningCount()
+	_, err := compiler.buildDetectionJob(workflowData)
+	require.NoError(t, err, "buildDetectionJob should succeed")
+
+	assert.Equal(t, initialWarnings+1, compiler.GetWarningCount(),
+		"built-in jobs that cannot become direct detection dependencies should emit a warning")
+}
+
+// TestBuildDetectionJobEngineEnvNeedsExpression verifies that the detection job scans the
+// effective detection engine env, so safe-outputs.threat-detection.engine overrides the
+// top-level engine env when resolving custom job dependencies.
+func TestBuildDetectionJobEngineEnvNeedsExpression(t *testing.T) {
+	compiler := NewCompiler()
+	compiler.stepOrderTracker = NewStepOrderTracker()
+
+	workflowData := &WorkflowData{
+		Name:        "Test Workflow",
+		AI:          "copilot",
+		RunsOn:      "runs-on: ubuntu-latest",
+		Permissions: "permissions:\n  contents: read",
+		EngineConfig: &EngineConfig{
+			ID: "copilot",
+			Env: map[string]string{
+				"IGNORED_MODEL": "${{ needs.ignored_job.outputs.model }}",
+			},
+		},
+		Jobs: map[string]any{
+			"ignored_job": map[string]any{
+				"runs-on": "ubuntu-latest",
+				"steps": []any{
+					map[string]any{"run": `echo "model=ignored" >> "$GITHUB_OUTPUT"`},
+				},
+			},
+			"select_model": map[string]any{
+				"runs-on": "ubuntu-latest",
+				"needs":   "pre_activation",
+				"outputs": map[string]any{
+					"model": "${{ steps.pick.outputs.model }}",
+				},
+				"steps": []any{
+					map[string]any{
+						"id":  "pick",
+						"run": `echo "model=claude-sonnet-4.6" >> "$GITHUB_OUTPUT"`,
+					},
+				},
+			},
+		},
+		SafeOutputs: &SafeOutputsConfig{
+			ThreatDetection: &ThreatDetectionConfig{
+				EngineConfig: &EngineConfig{
+					ID: "copilot",
+					Env: map[string]string{
+						"COPILOT_MODEL": "${{ needs.select_model.outputs.model }}",
+					},
+				},
+			},
+			Jobs: map[string]*SafeJobConfig{
+				"create-issue": {},
+			},
+		},
+	}
+
+	job, err := compiler.buildDetectionJob(workflowData)
+	require.NoError(t, err, "buildDetectionJob should succeed")
+	require.NotNil(t, job, "detection job should be created")
+
+	// The detection job must use the threat-detection override env, not the top-level env.
+	assert.Contains(t, job.Needs, "select_model",
+		"detection job must directly depend on select_model referenced in threat-detection.engine.env")
+	assert.NotContains(t, job.Needs, "ignored_job",
+		"detection job must ignore top-level engine.env references when threat-detection.engine overrides env")
+	assert.Contains(t, job.Needs, string(constants.AgentJobName),
+		"detection job must still depend on agent")
+	assert.Contains(t, job.Needs, string(constants.ActivationJobName),
+		"detection job must still depend on activation")
+}
+
+// TestBuildDetectionJobEngineEnvNeedsNotDuplicated verifies that a job referenced in
+// engine.env is not duplicated in the detection job's needs list.
+func TestBuildDetectionJobEngineEnvNeedsNotDuplicated(t *testing.T) {
+	compiler := NewCompiler()
+	compiler.stepOrderTracker = NewStepOrderTracker()
+
+	workflowData := &WorkflowData{
+		Name:        "Test Workflow",
+		AI:          "copilot",
+		RunsOn:      "runs-on: ubuntu-latest",
+		Permissions: "permissions:\n  contents: read",
+		EngineConfig: &EngineConfig{
+			ID: "copilot",
+			Env: map[string]string{
+				"VALUE_A": "${{ needs.custom_job.outputs.result }}",
+				"VALUE_B": "${{ needs.custom_job.outputs.other }}",
+			},
+		},
+		Jobs: map[string]any{
+			"custom_job": map[string]any{
+				"runs-on": "ubuntu-latest",
+				"steps": []any{
+					map[string]any{"run": "echo result=hello >> $GITHUB_OUTPUT"},
+				},
+			},
+		},
+		SafeOutputs: &SafeOutputsConfig{
+			ThreatDetection: &ThreatDetectionConfig{},
+			Jobs:            map[string]*SafeJobConfig{"create-issue": {}},
+		},
+	}
+
+	job, err := compiler.buildDetectionJob(workflowData)
+	require.NoError(t, err, "buildDetectionJob should succeed")
+	require.NotNil(t, job, "detection job should be created")
+
+	count := 0
+	for _, need := range job.Needs {
+		if need == "custom_job" {
+			count++
+		}
+	}
+	assert.Equal(t, 1, count, "custom_job should appear exactly once in detection needs")
+}
+
+// TestBuildDetectionJobEngineEnvNeedsIntegration is an end-to-end integration test that
+// compiles a workflow where engine.env references a custom job output, and verifies that the
+// compiled lock file includes the custom job as a direct dependency of the detection job.
+func TestBuildDetectionJobEngineEnvNeedsIntegration(t *testing.T) {
+	tmpDir := testutil.TempDir(t, "detection_engine_env_needs_test")
+
+	frontmatter := `---
+on:
+  issues:
+    types: [opened]
+  permissions: {}
+permissions:
+  contents: read
+  issues: read
+engine:
+  id: copilot
+  env:
+    IGNORED_MODEL: ${{ needs.ignored_job.outputs.model }}
+strict: false
+jobs:
+  ignored_job:
+    runs-on: ubuntu-latest
+    outputs:
+      model: ${{ steps.pick.outputs.model }}
+    steps:
+      - id: pick
+        run: echo "model=ignored" >> "$GITHUB_OUTPUT"
+  select_model:
+    runs-on: ubuntu-latest
+    needs: pre_activation
+    outputs:
+      model: ${{ steps.pick.outputs.model }}
+    steps:
+      - id: pick
+        run: echo "model=claude-sonnet-4.6" >> "$GITHUB_OUTPUT"
+safe-outputs:
+  create-issue: {}
+  threat-detection:
+    engine:
+      id: copilot
+      env:
+        COPILOT_MODEL: ${{ needs.select_model.outputs.model }}
+---
+
+# Test Detection Needs
+
+This workflow tests that engine.env needs expressions create detection job dependencies.
+`
+
+	testFile := filepath.Join(tmpDir, "detection-engine-env-needs.md")
+	require.NoError(t, os.WriteFile(testFile, []byte(frontmatter), 0644), "write test file")
+
+	compiler := NewCompiler()
+	require.NoError(t, compiler.CompileWorkflow(testFile), "compile workflow")
+
+	lockFile := filepath.Join(tmpDir, "detection-engine-env-needs.lock.yml")
+	content, err := os.ReadFile(lockFile)
+	require.NoError(t, err, "read lock file")
+
+	var lockFileYAML map[string]any
+	require.NoError(t, yaml.Unmarshal(content, &lockFileYAML), "parse lock file yaml")
+
+	jobsNode, ok := lockFileYAML["jobs"].(map[string]any)
+	require.True(t, ok, "lock file should contain jobs map")
+
+	detectionNode, ok := jobsNode["detection"].(map[string]any)
+	require.True(t, ok, "lock file should contain detection job")
+
+	rawNeeds, ok := detectionNode["needs"].([]any)
+	require.True(t, ok, "detection job should render needs as a YAML list")
+
+	detectionNeeds := make([]string, 0, len(rawNeeds))
+	for _, need := range rawNeeds {
+		needStr, ok := need.(string)
+		require.True(t, ok, "detection job needs entries should be strings")
+		detectionNeeds = append(detectionNeeds, needStr)
+	}
+
+	assert.Contains(t, detectionNeeds, "select_model",
+		"detection job must list select_model in needs when referenced via threat-detection.engine.env")
+	assert.NotContains(t, detectionNeeds, "ignored_job",
+		"detection job must not inherit top-level engine.env dependencies when threat-detection.engine overrides env")
+}

pkg/workflow/threat_detection_job.go

@@ -3,8 +3,13 @@ package workflow
 
 import (
 	"fmt"
+	"os"
+	"slices"
+	"strings"
 
+	"github.com/github/gh-aw/pkg/console"
 	"github.com/github/gh-aw/pkg/constants"
+	"github.com/github/gh-aw/pkg/sliceutil"
 )
 
 // buildDetectionJob creates a separate detection job that runs after the agent job.
@@ -68,6 +73,53 @@ func (c *Compiler) buildDetectionJob(data *WorkflowData) (*Job, error) {
 	// Detection job depends on agent job and activation job (for trace ID)
 	needs := []string{string(constants.AgentJobName), string(constants.ActivationJobName)}
 
+	// Scan the effective detection engine env values for needs.<customJob>.outputs.*
+	// expressions and add the referenced custom jobs as direct dependencies of the
+	// detection job. safe-outputs.threat-detection.engine overrides the top-level
+	// engine config for detection execution, so its env map must win here as well.
+	detectionEngineConfig := data.EngineConfig
+	if data.SafeOutputs != nil && data.SafeOutputs.ThreatDetection != nil && data.SafeOutputs.ThreatDetection.EngineConfig != nil {
+		detectionEngineConfig = data.SafeOutputs.ThreatDetection.EngineConfig
+	}
+	if detectionEngineConfig != nil && len(detectionEngineConfig.Env) > 0 {
+		var engineEnvBuilder strings.Builder
+		for _, envValue := range detectionEngineConfig.Env {
+			engineEnvBuilder.WriteByte('\n')
+			engineEnvBuilder.WriteString(envValue)
+		}
+		engineEnvContent := engineEnvBuilder.String()
+		hasNeedsReference := strings.Contains(engineEnvContent, "needs.")
+		if len(data.Jobs) > 0 {
+			engineEnvJobs := c.getReferencedCustomJobs(engineEnvContent, data.Jobs)
+			for _, jobName := range engineEnvJobs {
+				if isBuiltinJobName(jobName) {
+					continue
+				}
+				if !slices.Contains(needs, jobName) {
+					needs = append(needs, jobName)
+					threatLog.Printf("Added custom job '%s' to detection needs because it's referenced in engine.env", jobName)
+				}
+			}
+		}
+		if hasNeedsReference {
+			for _, builtinJobName := range sliceutil.SortedKeys(constants.KnownBuiltInJobNames) {
+				if slices.Contains(needs, builtinJobName) {
+					continue
+				}
+				if strings.Contains(engineEnvContent, fmt.Sprintf("needs.%s.", builtinJobName)) {
+					warningMsg := fmt.Sprintf(
+						"engine.env references built-in job '%s' in a detection-job needs expression. "+
+							"Built-in jobs are managed by the compiler and cannot be added as direct detection dependencies; "+
+							"this expression will silently evaluate to an empty string at runtime.",
+						builtinJobName,
+					)
+					fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg))
+					c.IncrementWarningCount()
+				}
+			}
+		}
+	}
+
 	// Determine runs-on: use threat detection override if set, otherwise ubuntu-latest.
 	// The detection job runs on a fresh runner separate from the agent job, so it does
 	// not need the same custom runner as safe-outputs.