fix: detection job missing engine.env job dependencies in needs list#44202
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main and recompile |
|
❌ Design Decision Gate 🏗️ failed to deliver outputs during design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a workflow compilation gap where the auto-generated detection job did not inherit required needs dependencies implied by engine.env values referencing needs.<job>.outputs.*, leading to empty-string expression evaluation at runtime.
Changes:
- Extend
buildDetectionJobto scan engine env values forneeds.<customJob>.outputs.*references and append those custom jobs to the detection job’sneeds. - Add unit + integration-style tests to validate detection job
needsbehavior for engine env references. - Regenerate two compiled workflow lock files.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/threat_detection_job.go | Adds dependency inference for detection job based on engine.env needs.* references. |
| pkg/workflow/compiler_jobs_test.go | Adds tests intended to cover the new detection-job dependency behavior. |
| .github/workflows/lint-monster.lock.yml | Regenerated compiled workflow output (safe-outputs config snippet updated). |
| .github/workflows/eslint-monster.lock.yml | Regenerated compiled workflow output (safe-outputs config snippet updated). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Low
| if data.EngineConfig != nil && len(data.EngineConfig.Env) > 0 && len(data.Jobs) > 0 { | ||
| 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) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in bd99f2e. buildDetectionJob now scans the effective detection engine config, so safe-outputs.threat-detection.engine.env overrides the top-level engine env when computing detection needs.
| 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)") |
There was a problem hiding this comment.
Fixed in bd99f2e. The integration test now parses the compiled lock YAML and asserts jobs.detection.needs directly instead of relying on a substring match.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix correctly identifies the gap: the detection job inherited engine.env values (via detectionEngineConfig.Env) but did not scan those values for needs.(job).outputs.* expressions the way the agent job already did.
The implementation cleanly mirrors the pattern in compiler_main_job.go - same guard, same helper, same dedup logic. Tests cover the unit case, dedup case, and an end-to-end lock-file integration test.
The lock file changes (removing required_title_prefix from update_issue in the eslint-monster and lint-monster workflows) appear to be a correct side-effect of running make recompile, not a regression.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 41.1 AIC · ⌖ 5.78 AIC · ⊞ 4.8K
|
Warning The Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (182 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (3 tests)
📋 Test Analysis Details
Inflation note: 157 test additions vs. 25 production additions (6.3:1 ratio), exceeding the 2:1 threshold (−10 pts). Contextually appropriate given three tests covering unit, deduplication, and integration scenarios for a targeted algorithmic fix. Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please run the
|
…tection-needs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… deps Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 3e22ac1. I merged |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
007677d adds |
🤖 PR Triage
Rationale: Fixes silent runtime bug — Run §28924016278
|
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…tection-needs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
When
engine.envvalues referenceneeds.<job>.outputs.*expressions, the auto-generateddetectionjob inherits those env mappings but was not gaining the referenced jobs in itsneedslist — causing the expressions to silently resolve to empty strings at runtime. Theagentandconclusionjobs already had this fixed;detectionwas the remaining gap.Changes
pkg/workflow/threat_detection_job.go: After building the initialneeds: [agent, activation]list, scanengine.envvalues forneeds.<customJob>.outputs.*references usinggetReferencedCustomJobsand append the referenced custom jobs — mirroring the identical scan already done for the agent job incompiler_main_job.go.pkg/workflow/compiler_jobs_test.go: Three new tests covering the detection job fix:TestBuildDetectionJobEngineEnvNeedsExpression— custom job appears in detectionneedsTestBuildDetectionJobEngineEnvNeedsNotDuplicated— no duplicate entries when job is referenced multiple timesTestBuildDetectionJobEngineEnvNeedsIntegration— end-to-end lock file compilation confirms correct outputExample
For a workflow with:
Before this fix, the compiled detection job omitted
select_model:After: