fix: skip detection job when engine disabled; include patches in agent artifact#22924
fix: skip detection job when engine disabled; include patches in agent artifact#22924
Conversation
When threat-detection is configured with engine: false and no custom steps, the detection job has nothing to run — the engine step is replaced with a comment, leaving detection.log empty. The parser correctly fails with 'No THREAT_DETECTION_RESULT found'. Fix: skip the entire detection job (and its dependencies in safe_outputs, conclusion, cache, and memory jobs) when EngineDisabled && len(Steps)==0. Recompile changeset.md which uses engine: false.
…abled When push-to-pull-request-branch is staged, usesPatchesAndCheckouts() returns false so patches weren't included in the agent artifact upload. But the detection job still needs them for security analysis (it checks HAS_PATCH and expects patch files in the downloaded artifact). Include aw-*.patch whenever threat detection is enabled, not just when the safe-output handler needs checkout. Recompile smoke-claude and smoke-codex to pick up the fix.
There was a problem hiding this comment.
Pull request overview
Compiler-side follow-up fixes for the threat detection workflow plumbing: fully omit the detection job when the threat detection engine is disabled with no custom steps, and ensure patch files are included in the agent artifact whenever threat detection is enabled.
Changes:
- Skip generating the detection job when
threat-detection: { engine: false }and no custom detection steps are configured. - Update downstream jobs (conclusion/safe outputs/memory/cache) to avoid referencing the detection job when it is skipped.
- Include
aw-*.patchin the unified agent artifact upload whenever threat detection is enabled.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/threat_detection.go | Skip building detection job entirely when engine is disabled and there are no custom steps. |
| pkg/workflow/notify_comment.go | Gate detection-related env vars and needs: dependency on detection job actually existing. |
| pkg/workflow/compiler_yaml_main_job.go | Add patch glob to agent artifact paths when threat detection needs patches. |
| pkg/workflow/compiler_safe_outputs_job.go | Align threat-detection-enabled gating with “job exists” semantics for step conditions. |
| pkg/workflow/compiler_safe_output_jobs.go | Treat engine-disabled-with-no-steps as “no detection job” when building jobs. |
| pkg/workflow/compiler_jobs.go | Prevent memory-management jobs from assuming detection job exists in the skipped scenario. |
| pkg/workflow/cache.go | Adjust cache restore/upload behavior to only treat threat detection as enabled when detection job will run. |
| .github/workflows/smoke-codex.lock.yml | Recompiled lockfile to include aw-*.patch in agent artifact upload paths. |
| .github/workflows/smoke-claude.lock.yml | Recompiled lockfile to include aw-*.patch in agent artifact upload paths. |
| .github/workflows/changeset.lock.yml | Recompiled lockfile removing detection job and related dependencies/conditions. |
Comments suppressed due to low confidence (2)
pkg/workflow/compiler_safe_output_jobs.go:36
- The comment describing when the detection job is skipped is now inaccurate: it says detection is only skipped when
ThreatDetectionis nil, but the code also skips whenengine: falseand there are no custom steps. Please update this comment to match the new behavior.
// The detection job runs after the agent job, downloads the agent artifact,
// and outputs detection_success and detection_conclusion for downstream jobs.
if threatDetectionEnabled {
detectionJob, err := c.buildDetectionJob(data)
pkg/workflow/threat_detection.go:565
- New behavior (skipping the detection job when engine is disabled and there are no custom steps) isn’t covered by tests. Please add a unit/integration-style compilation test that asserts buildDetectionJob returns nil and that downstream jobs/needs/env-vars don’t reference the detection job for
threat-detection: { engine: false }with no steps.
var steps []string
// Add setup action steps (same as agent job - installs the agentic engine)
setupActionRef := c.resolveActionReference("./actions/setup", data)
if setupActionRef != "" || c.actionMode.IsScript() {
// For dev mode (local action path), checkout the actions folder first
steps = append(steps, c.generateCheckoutActionsFolder(data)...)
steps = append(steps, c.generateSetupStep(setupActionRef, SetupActionDestination, false)...)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NOTE: Git patch generation has been moved to the safe-outputs MCP server | ||
| // The patch is now generated when create_pull_request or push_to_pull_request_branch | ||
| // tools are called, providing immediate error feedback if no changes are present. | ||
| if usesPatchesAndCheckouts(data.SafeOutputs) { | ||
| // Include patches in the artifact when: | ||
| // 1. Safe outputs needs them for checkout (non-staged create_pull_request/push_to_pull_request_branch) | ||
| // 2. Threat detection is enabled (detection job needs patches for security analysis, even when the | ||
| // safe-output handler is staged and doesn't need checkout itself) | ||
| threatDetectionNeedsPatches := data.SafeOutputs != nil && data.SafeOutputs.ThreatDetection != nil && | ||
| !(data.SafeOutputs.ThreatDetection.EngineDisabled && len(data.SafeOutputs.ThreatDetection.Steps) == 0) |
There was a problem hiding this comment.
Including patch files in the unified agent artifact when threat detection is enabled is important behavior but currently untested. Please add a compiler_yaml_main_job test that compiles a workflow where PR handlers are staged (so usesPatchesAndCheckouts() is false) while threat detection is enabled, and assert the upload-artifact paths include /tmp/gh-aw/aw-*.patch.
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Smoke Test Results — Run §23551015969\n\n| Test | Result |\n|------|--------|\n| 1. GitHub MCP | ✅ |\n| 2. MCP Scripts GH CLI | ✅ |\n| 3. Serena MCP | ❌ |\n| 4. Playwright | ✅ |\n| 5. Web Fetch | ✅ |\n| 6. File Writing | ✅ |\n| 7. Bash Tool | ✅ |\n| 8. Discussion Interaction | ✅ |\n| 9. Build gh-aw | ✅ |\n| 10. Discussion Creation | ✅ |\n| 11. Workflow Dispatch | ✅ |\n| 12. PR Review | ✅ |\n\nOverall: ❌ FAIL (Serena MCP tools not available)\n\nAuthor: @davidslater · Assignees:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Smoke test review of PR #22924: The fix correctly skips the detection job when engine: false with no custom steps. The duplicated EngineDisabled && len(Steps) == 0 predicate across 6+ files could be extracted into a shared helper to reduce drift risk.
📰 BREAKING: Report filed by Smoke Copilot
| !(data.SafeOutputs.ThreatDetection.EngineDisabled && len(data.SafeOutputs.ThreatDetection.Steps) == 0) | ||
|
|
||
| // Build the separate detection job. Detection runs by default for all safe-outputs workflows | ||
| // and is only skipped when ThreatDetection is nil (i.e. threat-detection: false was set). |
There was a problem hiding this comment.
The predicate data.SafeOutputs.ThreatDetection.EngineDisabled && len(data.SafeOutputs.ThreatDetection.Steps) == 0 is duplicated in at least 6 files. Consider extracting a helper like isDetectionJobEnabled(so *SafeOutputsConfig) bool in a shared location to prevent future drift.
|
Commit pushed:
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Smoke test results for run §23551016051
|
|
💥 Smoke test §23551015966 — PARTIAL (all non-skipped passed)
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude
| @@ -430,7 +430,8 @@ func generateCacheMemorySteps(builder *strings.Builder, data *WorkflowData) { | |||
| // Use actions/cache/restore for restore-only caches or when threat detection is enabled | |||
| // When threat detection is enabled, we only restore the cache and defer saving to a separate job after detection | |||
There was a problem hiding this comment.
The multi-line condition for threatDetectionEnabled looks correct. Consider extracting EngineDisabled && len(Steps) == 0 into a helper method like ThreatDetection.IsEffectivelyDisabled() for readability across the multiple call sites in this file.
| @@ -348,7 +348,8 @@ func (c *Compiler) buildQmdIndexingJobWrapper(data *WorkflowData) error { | |||
| // buildMemoryManagementJobs builds memory management jobs (push_repo_memory and update_cache_memory). | |||
| // These jobs handle artifact-based memory persistence to git branches and GitHub Actions cache. | |||
There was a problem hiding this comment.
Same pattern as cache.go — if this condition is used in 3+ places, a shared helper would reduce future divergence risk. The logic correctly handles the case where the engine is disabled with no custom steps.
Extract the negated condition into a method on ThreatDetectionConfig to eliminate De Morgan's law lint violations (QF1001) and improve readability across 8 call sites.
…tifact test - Extract IsDetectionJobEnabled(so *SafeOutputsConfig) as the single source of truth for whether the detection job should exist. - Replace all 8 inline checks across 7 files with the helper. - Add TestPatchIncludedInArtifactWhenThreatDetectionEnabled to verify aw-*.patch is included in the agent artifact when push-to-pr-branch is staged (usesPatchesAndCheckouts=false) but detection is enabled.
|
@copilot add integration tests in pkg/cli/workflows that validate these scnearios. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/3f8f847c-6b8d-42f8-a3c1-55edfa21c472
Added integration tests for both scenarios in a follow-up commit. The original PR was merged before the push completed, so I'll need a new PR to include these tests. The tests added:
Both tests pass. New workflow fixtures were added to |
Follow-up fixes for threat detection parser (#22832)
These are compiler-side fixes discovered while testing the threat detection parser fix from #22832.
Fix 1: Skip detection job when engine is disabled with no custom steps
When
threat-detection: { engine: false }is configured (e.g., changeset workflow), the compiler still emitted the full detection job. The engine step was replaced with a YAML comment, but setup, parse, upload, and conclusion steps all ran — causing the parser to correctly fail with "No THREAT_DETECTION_RESULT found" on the emptydetection.log.Now the entire detection job is skipped when
EngineDisabled && len(Steps) == 0. All dependent jobs (safe_outputs, conclusion, cache, memory) no longer reference the non-existent detection job.Files changed:
threat_detection.go,compiler_safe_output_jobs.go,compiler_safe_outputs_job.go,compiler_jobs.go,notify_comment.go,cache.goRecompiled:
changeset.lock.yml(detection job removed, -139 lines)Fix 2: Include
aw-*.patchin agent artifact when threat detection is enabledWhen
push-to-pull-request-branchhasstaged: true,usesPatchesAndCheckouts()returns false so patches were excluded from the agent artifact upload. But the detection job still downloads the artifact and expects patch files (becauseHAS_PATCH=true).Now
aw-*.patchis included in the artifact whenever threat detection is enabled, not just when the safe-output handler needs checkout.Files changed:
compiler_yaml_main_job.goRecompiled:
smoke-claude.lock.yml,smoke-codex.lock.ymlTesting
aw-*.patchin upload pathsChangeset
threat-detection.engineis disabled with no custom steps, and includeaw-*.patchfiles in agent artifacts whenever threat detection is enabled.✨ PR Review Safe Output Test - Run 23551015966