Honor GH_AW_DETECTION_CONTINUE_ON_ERROR in detection setup to prevent false job failures - #49415
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates threat-detection setup so missing inputs can warn instead of failing.
Changes:
- Adds continue-on-error handling to required-file checks.
- Reads the detection policy during setup.
- Adds regression tests for warning and strict modes.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/setup_threat_detection.test.cjs |
Tests missing agent-output behavior. |
actions/setup/js/setup_threat_detection.cjs |
Applies continue-on-error during setup. |
actions/setup/js/file_helpers.test.cjs |
Tests warning-mode file checks. |
actions/setup/js/file_helpers.cjs |
Adds optional warning behavior for missing files. |
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: 3
- Review effort level: Balanced
| * @returns {Promise<void>} | ||
| */ | ||
| async function main() { | ||
| const continueOnError = (process.env.GH_AW_DETECTION_CONTINUE_ON_ERROR || "true").toLowerCase() !== "false"; |
| if (continueOnError) { | ||
| core.warning(`${ERR_SYSTEM}: ❌ ${fileDescription} not found at: ${filePath}. Continuing because GH_AW_DETECTION_CONTINUE_ON_ERROR=true`); | ||
| } else { | ||
| core.setFailed(`${ERR_SYSTEM}: ❌ ${fileDescription} not found at: ${filePath}`); |
| // The artifact contains /tmp/gh-aw/agent_output.json which becomes /tmp/gh-aw/threat-detection/agent_output.json | ||
| const agentOutputPath = path.join(threatDetectionDir, AGENT_OUTPUT_FILENAME); | ||
| if (!checkFileExists(agentOutputPath, threatDetectionDir, "Agent output file", true)) { | ||
| if (!checkFileExists(agentOutputPath, threatDetectionDir, "Agent output file", true, continueOnError)) { |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49415 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes. The fix direction is correct, but three issues need resolution before the behavior is clean.
📋 Key Themes
Issues to address
-
core.errorfires unconditionally before thecontinueOnErrorbranch (file_helpers.cjsline 67) — warn mode still produces an error annotation, which is the same observable failure signal the PR is trying to suppress. See existing comment on line 80 offile_helpers.cjs. -
GH_AW_DETECTION_CONTINUE_ON_ERRORis not injected into the generated workflow step (setup_threat_detection.cjsline 28) — the env var will be absent at runtime unless explicitly forwarded. See existing comment. -
Only the
agent_output.jsoncheck is patched (setup_threat_detection.cjs) — the download-check at lines 98–101 still callscore.setFailedunconditionally, so jobs can still false-fail on the other required input. See existing comment on line 77. -
New test asserts the broken
core.errorside-effect (file_helpers.test.cjsline 130) — this makes the test a false-green that validates the bug, not the fix.
Positive highlights
- ✅ Correct default semantics: env var defaults to
"true", matching the conclude-phase behavior. - ✅ Case-insensitive parsing of the env var.
- ✅ Both enabled and disabled paths covered in
setup_threat_detection.test.cjs. - ✅ Clean early-return on warn-mode missing file.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 28.7 AIC · ⊞ 7K
Comment /matt to run again
| const result = checkFileExists(filePath, tempDir, "Test file", true, true); | ||
| expect(result).toBe(false); | ||
| expect(mockCore.errorCalls.some(msg => msg.includes("Test file not found"))).toBe(true); | ||
| expect(mockCore.warningCalls.some(msg => msg.includes("Continuing because GH_AW_DETECTION_CONTINUE_ON_ERROR=true"))).toBe(true); |
There was a problem hiding this comment.
[/diagnosing-bugs] This assertion encodes the broken behavior — it expects core.error to fire even in continueOnError=true mode, making it a false-green test that passes only while the bug exists.
💡 What to fix
Line 67 of file_helpers.cjs calls core.error(...) unconditionally before the continueOnError branch. Once that call is guarded (see companion comment), this assertion must be inverted:
// After fixing file_helpers.cjs:
expect(mockCore.errorCalls).toHaveLength(0); // no error annotation in warn mode
expect(mockCore.warningCalls.some(msg => msg.includes("Test file not found"))).toBe(true);Leaving the errorCalls assertion as-is means the test will start failing after the correct fix is applied, or worse, give false confidence that warn mode is clean when it is not.
@copilot please address this.
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (5 tests)
|
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: Honor GH_AW_DETECTION_CONTINUE_ON_ERROR in detection setup
The intent is correct, but three blocking issues prevent the change from fully achieving its goal. All three are captured in inline comments — this summary consolidates them.
Blocking issues:
-
core.error(...)fires unconditionally in warn mode (file_helpers.cjs~line 67): incontinueOnError=truepath the step still emits an error annotation before thecore.warningbranch. This can causeconclusion: failureeven withoutsetFailed. Thecore.errorcall should be guarded the same way ascore.setFailed. -
Patch/bundle check ignores
continueOnError(setup_threat_detection.cjs~lines 98–101):core.setFailedis called unconditionally whenHAS_PATCH=truebut no patch files are found — an identical gap to the one this PR aims to fix. -
GH_AW_DETECTION_CONTINUE_ON_ERRORis never injected into the generated step: the env var is read inmain()butbuildThreatDetectionAnalysisStepdoes not emit it. If the caller does not expose it, the default"true"in|| "true"silently masks misconfiguration.
Non-blocking observations:
- New tests are well-structured and cover agent-output happy/sad paths.
- The parametric
continueOnErroraddition tocheckFileExistsis the right abstraction.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 22.7 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Request Changes
The core fix does not actually work in production: GH_AW_DETECTION_CONTINUE_ON_ERROR is never wired into the "Setup threat detection" step's env block, so the new continue-on-error branch always falls back to its hardcoded default and cannot be configured. Two additional consistency issues (unconditional patch/bundle failure, error+warning double-annotation) undermine the stated goal of aligning setup-phase and conclude-phase failure semantics.
💡 Themes
- Critical wiring gap:
buildThreatDetectionAnalysisStepinpkg/workflow/threat_detection_steps.gonever emitsGH_AW_DETECTION_CONTINUE_ON_ERRORinto the "Setup threat detection" step's env, unlikebuildDetectionConclusionStepwhich does this correctly for the later step. Verified against generated.lock.ymlfiles (e.g.ab-testing-advisor.lock.yml) — the env block only hasWORKFLOW_NAME,WORKFLOW_DESCRIPTION,HAS_PATCH. This means the new unit tests pass (they setprocess.envdirectly) but the real GitHub Actions env never populates the variable, so the fix is inert for actual workflow runs and users can't opt into strict (fail-fast) mode as intended. - Inconsistent policy application: the patch/bundle existence check in
setup_threat_detection.cjsstill callscore.setFailedunconditionally, while the agent-output check was updated to respectcontinueOnError. Same function, same "required downloaded input" category, different failure behavior. - Misleading log output:
checkFileExistsstill emits an unconditionalcore.error(...)before choosing warn vs. fail, so warn mode produces both an error annotation and a warning annotation for the same event.
Tests added are reasonable for the JS-level API but don't cover the Go step-generation layer, which is where the actual defect lives — that's why the missing env wiring wasn't caught.
🔎 Code quality review by PR Code Quality Reviewer · auto · 62.1 AIC · ⊞ 7.8K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/setup_threat_detection.cjs:98
This patch/bundle existence check still calls core.setFailed unconditionally, ignoring the new continueOnError policy applied a few lines above to the agent-output check.
<details>
<summary>💡 Details</summary>
Line 77 now gates the missing agent_output.json failure on , but this block at line 98-101:
still hard-fails regardless of . If the stated goal is that missing required setup inputs should degrade to warnings in warn mode, this check is inconsistent: a missing agent output file is…
actions/setup/js/file_helpers.cjs:67
In warn mode this still emits an unconditional core.error(...) annotation before the new warning, so "continue on error" still surfaces as a CI error annotation, not just a warning.
<details>
<summary>💡 Details</summary>
Line 67 (core.error("❌ " + fileDescription + " not found at: " + filePath);) runs unconditionally before the continueOnError branch that decides between core.warning and core.setFailed. So in warn mode the log/UI ends up with both an "error" annotation (from line 6…
PR Triage
|
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot Please address the outstanding review feedback, refresh the branch, and run the
|
…ontinueOnError in patch check, emit env var in setup step Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Great work! 🚀 This PR addresses a critical bug where detection jobs were failing despite What's solid here:
The PR looks ready for review and merge. The implementation aligns setup-phase error handling with conclude-phase semantics, fixing the false job failures described in the issue. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
All three review issues addressed in this commit:
|
|
🎉 This pull request is included in a new release. Release: |
Detection jobs could still conclude as
failurewhenGH_AW_DETECTION_CONTINUE_ON_ERROR=trueif setup failed beforedetection_result.jsonexisted (notably whenagent_output.jsonwas missing). This change aligns setup-phase failure behavior with conclude-phase continue-on-error semantics so missing detection inputs degrade to warnings in warn mode.Behavioral alignment: setup phase now respects continue-on-error
setup_threat_detection.cjsnow readsGH_AW_DETECTION_CONTINUE_ON_ERROR(case-insensitive, defaulttrue) and propagates it into required input checks.Failure-path correction in shared file existence helper
checkFileExists(...)infile_helpers.cjsnow accepts an optionalcontinueOnErrorflag.continueOnError=true→ emit warning, do not callcore.setFailed.continueOnError=false→ preserve existing hard-fail behavior.Targeted regression coverage
setFailedagent_output.jsonin setup + continue mode enabled/disabled behavior split.Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
run: https://github.com/github/gh-aw/actions/runs/30676859249