threat-detection: fix strict-mode conclude failure writing no outputs - #50856
Conversation
…exit 1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (81 additions detected). |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe 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 Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Fixes strict-mode threat-detection failures so downstream reporting correctly identifies tooling errors.
Changes:
- Writes failure outputs and environment variables before exiting.
- Adds strict and continue-on-error regression tests.
- Includes an unrelated agentic-workflows router update.
Show a summary per file
| File | Description |
|---|---|
actions/setup/sh/conclude_threat_detection.sh |
Emits failure metadata for missing results. |
pkg/workflow/threat_detection_conclude_script_test.go |
Tests output and environment propagation. |
.github/skills/agentic-workflows/SKILL.md |
Adds an unrelated designer mapping entry. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| - `.github/aw/debug-agentic-workflow.md` | ||
| - `.github/aw/dependabot.md` | ||
| - `.github/aw/deployment-status.md` | ||
| - `.github/aw/designer-mappings.md` |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Test Quality Sentinel 🧪PR: #50856 — "threat-detection: fix strict-mode conclude failure writing no outputs" SummaryThis PR adds 2 new integration tests that directly verify the behavioral fix: ensuring outputs are written to All new tests are high-value design tests with clear assertions covering both error and success paths. No mock libraries or forbidden patterns detected. Recommendation: Approve. Coverage AnalysisNew Tests (2)
File: Test Assertions BreakdownTest 1 — Strict Mode (exit 1 path)
Test 2 — Continue-on-Error (exit 0 path)
Production Changes CoverageAll 11 new lines in
Quality Signals✅ Design contracts: Both tests verify user-visible output behavior Notes
Recommendation✅ APPROVE — Score: 100/100. All new tests are high-value design tests with comprehensive behavioral coverage. Zero implementation tests.
|
![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 Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor suggestion.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed: the script now writes
conclusion,success, andreasonto$GITHUB_OUTPUTand exportsGH_AW_DETECTION_CONCLUSION/GH_AW_DETECTION_REASONto$GITHUB_ENVbeforeexit 1, matching the.cjspath from #48940. - Regression test coverage: two new tests pin the fixed behaviour for both strict-mode and continue-on-error paths. The strict-mode test is particularly valuable since it validates the previously-absent output writes.
- Minor gap:
TestConcludeThreatDetectionScript_MissingResultContinueOnErrorExportsEnvonly assertsGITHUB_ENV; a note linking it to the existingMissingResultContinueOnErrortest (which coversGITHUB_OUTPUT) would make coverage intent obvious.
Positive Highlights
- ✅ Symmetric fix — continue-on-error path gets
GITHUB_ENVexports for consistency, not just strict mode - ✅ Guard
[ -n "${GITHUB_ENV:-}" ]is appropriately defensive for local/test contexts - ✅ Tests follow the existing script-test pattern cleanly
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 21.5 AIC · ⊞ 7.1K
Comment /matt to run again
| t.Fatalf("expected GH_AW_DETECTION_CONCLUSION=warning in GITHUB_ENV, got: %s", envText) | ||
| } | ||
| if !strings.Contains(envText, "GH_AW_DETECTION_REASON=agent_failure") { | ||
| t.Fatalf("expected GH_AW_DETECTION_REASON=agent_failure in GITHUB_ENV, got: %s", envText) |
There was a problem hiding this comment.
[/tdd] The ContinueOnErrorExportsEnv test validates GITHUB_ENV exports but does not assert GITHUB_OUTPUT is also populated in this path — the pre-existing MissingResultContinueOnError test may cover it, but a comment linking them (or adding assertions here) would make coverage intent explicit.
💡 Suggestion
Add a quick GITHUB_OUTPUT assertion or a comment referencing the prior test, e.g.:
// GITHUB_OUTPUT assertions covered by TestConcludeThreatDetectionScript_MissingResultContinueOnErrorThis keeps the test self-documenting for future maintainers.
@copilot please address this.
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. All 2 new tests are high-value design tests with 0% implementation tests (well below the 30% threshold). Comprehensive behavioral coverage of both strict-mode and continue-on-error paths with no forbidden mock patterns detected.
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The fix is correct and well-structured. The strict-mode path now writes conclusion=failure, success=false, and reason=agent_failure to GITHUB_OUTPUT before exit 1, preventing downstream steps from misclassifying infra failures as threat detections. Both branches also export GH_AW_DETECTION_CONCLUSION/GH_AW_DETECTION_REASON to GITHUB_ENV matching the .cjs path. Two new tests cover the changes. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.7 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Request changes
The $GITHUB_OUTPUT fix is correct and matches the existing warn-mode pattern, but the added $GITHUB_ENV exports in both branches are dead code that do not fix anything and should be removed.
Themes
conclude_threat_detection.shis the last step of the detection job (seethreat_detection_job.go/threat_detection_external.go), and downstream jobs only ever consume its results vianeeds.detection.outputs.detection_conclusion/detection_reason, which are wired fromsteps.detection_conclusion.outputs.*($GITHUB_OUTPUT) — never from$GITHUB_ENV.$GITHUB_ENVonly affects later steps in the same job, and none exist after this step.- The real consumers of the
GH_AW_DETECTION_CONCLUSION/GH_AW_DETECTION_REASONenv var names (generate_footer.cjs,handle_detection_runs.cjs,notify_comment_error.cjs,push_to_pull_request_branch.cjs, etc.) get them from the safe-outputs job's own env, populated from the detection job'soutputs.*— not from this script's$GITHUB_ENV. - The new Go tests lock in this no-op behavior rather than testing something that changes production behavior, which will make future cleanup look like a regression.
- The core fix (writing
conclusion=failure,success=false,reason=agent_failureto$GITHUB_OUTPUTin strict mode beforeexit 1) correctly addresses the described bug and is a good, minimal change on its own.
🔎 Code quality review by PR Code Quality Reviewer · auto · 87.5 AIC · ⊞ 7.8K
Comment /review to run again
| echo "reason=agent_failure" >> "${GITHUB_OUTPUT}" | ||
| if [ -n "${GITHUB_ENV:-}" ]; then | ||
| echo "GH_AW_DETECTION_CONCLUSION=warning" >> "${GITHUB_ENV}" | ||
| echo "GH_AW_DETECTION_REASON=agent_failure" >> "${GITHUB_ENV}" |
There was a problem hiding this comment.
The new $GITHUB_ENV exports here are a no-op: this is the last step of the detection job, and env vars never cross job boundaries anyway.
💡 Why this does not fix the described bug
Downstream jobs consume detection results exclusively via needs.detection.outputs.detection_conclusion / detection_reason, which are wired in pkg/workflow/threat_detection_job.go from steps.detection_conclusion.outputs.* (i.e. $GITHUB_OUTPUT), not from $GITHUB_ENV. $GITHUB_ENV only affects subsequent steps within the same job, and this conclude step is the last step before exit 0/exit 1 in this job — there is no later step in the detection job that reads these env vars either.
The $GITHUB_OUTPUT writes added alongside this (conclusion/success/reason) are the actual fix; the $GITHUB_ENV block is dead code that adds a false impression of cross-job propagation and increases maintenance surface without changing observable behavior.
# This block has no observable effect in either the warn or strict branch:
if [ -n "${GITHUB_ENV:-}" ]; then
echo "GH_AW_DETECTION_CONCLUSION=warning" >> "${GITHUB_ENV}"
echo "GH_AW_DETECTION_REASON=agent_failure" >> "${GITHUB_ENV}"
fiRecommend removing both $GITHUB_ENV blocks (here and at line 51) unless there is a concrete downstream consumer within the same job that actually reads them — if there is, that dependency should be documented and tested directly, not just asserted via a new unit test that locks in a no-op.
| echo "success=false" >> "${GITHUB_OUTPUT}" | ||
| echo "reason=agent_failure" >> "${GITHUB_OUTPUT}" | ||
| if [ -n "${GITHUB_ENV:-}" ]; then | ||
| echo "GH_AW_DETECTION_CONCLUSION=failure" >> "${GITHUB_ENV}" |
There was a problem hiding this comment.
Same dead-code issue as the warn-mode branch above: this $GITHUB_ENV export is a no-op right before exit 1.
💡 Details
This is the strict-mode branch, and it exits the process immediately after this block via exit 1. There is no subsequent step in this job that reads $GITHUB_ENV, and downstream jobs never see it since env vars do not cross job boundaries in GitHub Actions — only the $GITHUB_OUTPUT values (already correctly added right above) are consumed via needs.detection.outputs.*.
Keeping this creates two independent "sources of truth" for the same conclusion that can silently diverge in future edits (e.g. someone changes the $GITHUB_OUTPUT value but forgets the parallel $GITHUB_ENV value, or vice versa), and it invites a future contributor to wrongly assume some downstream code depends on GH_AW_DETECTION_CONCLUSION/GH_AW_DETECTION_REASON env vars being set by this script, when in reality all real consumers of those env var names (generate_footer.cjs, handle_detection_runs.cjs, notify_comment_error.cjs, etc.) get them from the safe_outputs job env, populated from job outputs — not from this script.
Recommend dropping both $GITHUB_ENV blocks.
| func TestConcludeThreatDetectionScript_MissingResultStrictModeWritesOutputs(t *testing.T) { | ||
| scriptPath := filepath.Join("..", "..", "actions", "setup", "sh", "conclude_threat_detection.sh") | ||
| outputFile := filepath.Join(t.TempDir(), "github_output.txt") | ||
| envFile := filepath.Join(t.TempDir(), "github_env.txt") | ||
| missingResult := filepath.Join(t.TempDir(), "missing_detection_result.json") | ||
|
|
||
| cmd := exec.Command("bash", scriptPath, missingResult) | ||
| cmd.Env = append(os.Environ(), | ||
| "RUN_DETECTION=true", | ||
| "DETECTION_AGENTIC_EXECUTION_OUTCOME=failure", | ||
| "GH_AW_DETECTION_CONTINUE_ON_ERROR=false", | ||
| "GITHUB_OUTPUT="+outputFile, | ||
| "GITHUB_ENV="+envFile, | ||
| ) | ||
|
|
||
| out, err := cmd.CombinedOutput() | ||
| if err == nil { | ||
| t.Fatalf("script should exit non-zero in strict mode on missing result: %s", out) | ||
| } | ||
|
|
||
| outputData, readErr := os.ReadFile(outputFile) | ||
| if readErr != nil { | ||
| t.Fatalf("failed to read GITHUB_OUTPUT: %v", readErr) | ||
| } | ||
| outputText := string(outputData) | ||
| if !strings.Contains(outputText, "conclusion=failure") { | ||
| t.Fatalf("expected conclusion=failure in GITHUB_OUTPUT, got: %s", outputText) | ||
| } | ||
| if !strings.Contains(outputText, "success=false") { | ||
| t.Fatalf("expected success=false in GITHUB_OUTPUT, got: %s", outputText) | ||
| } | ||
| if !strings.Contains(outputText, "reason=agent_failure") { | ||
| t.Fatalf("expected reason=agent_failure in GITHUB_OUTPUT, got: %s", outputText) | ||
| } | ||
|
|
||
| envData, readErr := os.ReadFile(envFile) | ||
| if readErr != nil { | ||
| t.Fatalf("failed to read GITHUB_ENV: %v", readErr) | ||
| } | ||
| envText := string(envData) | ||
| if !strings.Contains(envText, "GH_AW_DETECTION_CONCLUSION=failure") { |
There was a problem hiding this comment.
These new tests lock in behavior (the $GITHUB_ENV writes) that has no observable effect in production, giving false confidence that a real bug is being covered.
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Great work on this fix! 🎯 This PR elegantly resolves the strict-mode bug where infrastructure failures in What looks good:
This is ready for maintainer review. The fix aligns perfectly with the acceptance criteria from the issue, and the test coverage validates both the happy path and error scenarios.
|
|
@copilot run pr-finisher skill |
|
@copilot resolve the merge conflicts on this branch. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…tection-output # Conflicts: # actions/setup/sh/conclude_threat_detection.sh # pkg/workflow/threat_detection_conclude_script_test.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Maintainer triage: this PR is blocked by merge conflicts. Please run Run: https://github.com/github/gh-aw/actions/runs/31113572782
|
|
@copilot resolve the merge conflicts in this pull request |
Ran the pr-finisher pass. Addressed review feedback by removing the unrelated skill-router change and the no-op GITHUB_ENV additions, merged latest main to resolve conflicts, and pushed 71e67ad.
Resolved the merge conflicts by merging latest main and keeping the upstream |
|
🎉 This pull request is included in a new release. Release: |
Strict-mode (
GH_AW_DETECTION_CONTINUE_ON_ERROR=false) failures inconclude_threat_detection.shexited non-zero without writing any$GITHUB_OUTPUT, leavingdetection_reasonempty downstream. This caused infra/tooling failures to render to reviewers as[!CAUTION] agentic threat detectedinstead of a system error — the exact confusion #48940 fixed for the.cjspath, but never covered here.Script fix (
actions/setup/sh/conclude_threat_detection.sh)conclusion=failure,success=false,reason=agent_failureto$GITHUB_OUTPUTbeforeexit 1, sodetection_reasonis populated and correctly classified as a tooling failure downstream.GH_AW_DETECTION_CONCLUSION/GH_AW_DETECTION_REASONto$GITHUB_ENV, matchingsetDetectionFailureinparse_threat_detection_results.cjsfor consistency with later steps in the job.Test coverage (
pkg/workflow/threat_detection_conclude_script_test.go)GH_AW_DETECTION_CONCLUSION/GH_AW_DETECTION_REASONto$GITHUB_ENV.Run: https://github.com/github/gh-aw/actions/runs/31113572782> Generated by 👨🍳 PR Sous Chef · gpt54 · 17.2 AIC · ⊞ 8.3K · ◷