-
Notifications
You must be signed in to change notification settings - Fork 456
fix: detection job missing engine.env job dependencies in needs list #44202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
86dfb87
f356b40
12289ee
6f67435
6018d58
bd99f2e
3da131e
5763a6e
8cb0c14
3e22ac1
007677d
6db7942
71b34bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4071,3 +4071,160 @@ 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") | ||
| } | ||
|
|
||
| // TestBuildDetectionJobEngineEnvNeedsExpression verifies that when engine.env values contain | ||
| // needs.<customJob>.outputs.* expressions, the referenced custom job is added as a direct | ||
| // dependency of the detection job (mirrors the same fix applied to the agent job). | ||
| 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{ | ||
| "COPILOT_MODEL": "${{ needs.select_model.outputs.model }}", | ||
| }, | ||
| }, | ||
| Jobs: map[string]any{ | ||
| "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{}, | ||
| 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 directly depend on select_model because engine.env | ||
| // references its outputs; without this, needs.select_model would be undefined. | ||
| assert.Contains(t, job.Needs, "select_model", | ||
| "detection job must directly depend on select_model referenced in engine.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: | ||
| COPILOT_MODEL: ${{ needs.select_model.outputs.model }} | ||
| strict: false | ||
| jobs: | ||
| 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: {} | ||
| --- | ||
|
|
||
| # 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") | ||
|
|
||
| yamlStr := string(content) | ||
|
|
||
| // The detection job must directly depend on select_model | ||
| detectionSection := extractJobSection(yamlStr, "detection") | ||
| require.NotEmpty(t, detectionSection, "detection job section should be present in lock file") | ||
|
|
||
| assert.Contains(t, detectionSection, "select_model", | ||
| "detection job must list select_model in its needs (referenced via engine.env)") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in bd99f2e. The integration test now parses the compiled lock YAML and asserts |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,8 @@ package workflow | |
|
|
||
| import ( | ||
| "fmt" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| "github.com/github/gh-aw/pkg/constants" | ||
| ) | ||
|
|
@@ -68,6 +70,29 @@ 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 engine.env values for needs.<customJob>.outputs.* expressions and add the | ||
| // referenced custom jobs as direct dependencies of the detection job. | ||
| // The detection job inherits the engine.env mapping from the main engine config, so | ||
| // it must also inherit the corresponding job dependencies to ensure those expressions | ||
| // evaluate correctly at runtime (mirrors the same scan done for the agent job). | ||
| if data.EngineConfig != nil && len(data.EngineConfig.Env) > 0 && len(data.Jobs) > 0 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] The agent job warns users when 💡 Suggestion: mirror the built-in warning from compiler_main_job.goAfter the dedup loop (~line 93), add the same built-in-name guard already in for _, builtinJobName := range sliceutil.SortedKeys(constants.KnownBuiltInJobNames) {
if slices.Contains(needs, builtinJobName) {
continue
}
if strings.Contains(engineEnvBuilder.String(), fmt.Sprintf("needs.%s.", builtinJobName)) {
c.IncrementWarningCount()
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(
fmt.Sprintf("engine.env references built-in job %q in a detection-job needs expression; will silently resolve to empty at runtime", builtinJobName)))
}
}This keeps detection-job behaviour consistent with the agent job and surfaces misconfigurations at compile time rather than at runtime. @copilot please address this. |
||
| var engineEnvBuilder strings.Builder | ||
| for _, envValue := range data.EngineConfig.Env { | ||
| engineEnvBuilder.WriteByte('\n') | ||
| engineEnvBuilder.WriteString(envValue) | ||
| } | ||
| engineEnvJobs := c.getReferencedCustomJobs(engineEnvBuilder.String(), 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) | ||
| } | ||
| } | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in bd99f2e. |
||
|
|
||
| // 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. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The integration test asserts that
detectionsection contains the stringselect_model, but it doesn't verify theneeds:key specifically. A false-positive could occur ifselect_modelappeared elsewhere in the detection section (e.g., in a step env var or output value).💡 Suggestion: tighten the assertion
Use a
needs:prefix check to confirm the dependency is in the right place:or, since the YAML
needslist is rendered inline:This is a minor robustness concern; the current test is still meaningful for regression detection.
@copilot please address this.