diff --git a/docs/adr/44202-detection-job-engine-env-needs-scan.md b/docs/adr/44202-detection-job-engine-env-needs-scan.md new file mode 100644 index 00000000000..29e0ba1c3de --- /dev/null +++ b/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..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..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.* diff --git a/pkg/workflow/compiler_jobs_test.go b/pkg/workflow/compiler_jobs_test.go index bc71175ec6d..5473fb5e01c 100644 --- a/pkg/workflow/compiler_jobs_test.go +++ b/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") +} diff --git a/pkg/workflow/threat_detection_job.go b/pkg/workflow/threat_detection_job.go index 0bbaf00d188..f01ad31333a 100644 --- a/pkg/workflow/threat_detection_job.go +++ b/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..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.