fix: use [!WARNING] banner for threat detection engine failures instead of [!CAUTION] - #48940
Conversation
…findings When the threat detection engine fails to produce results (reason: agent_failure or parse_error), render a [!WARNING] banner titled "threat detection engine error" instead of the [!CAUTION] "agentic threat detected" banner used for real security findings. This prevents alert fatigue and false urgency when the detection tool itself crashes, making it clear to reviewers that the banner signals a tooling failure rather than a genuine security threat. Changes: - threat_detection_warning.cjs: add isToolingFailureReason() helper that returns true for agent_failure and parse_error - messages_run_status.cjs: getDetectionWarningMessage() uses [!WARNING]/detectionEngineError template for tooling failures and keeps [!CAUTION]/detectionWarning template for actual threats - generate_footer.cjs: getExpiredEntityCautionAlert() applies the same distinction using the imported isToolingFailureReason() - Tests updated and extended to cover all banner variants Closes #48922 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Distinguishes threat-detection tooling failures from genuine security findings.
Changes:
- Classifies
agent_failureandparse_erroras tooling failures. - Renders tooling failures with
[!WARNING]. - Adds coverage for classification and banner rendering.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/threat_detection_warning.cjs |
Adds tooling-failure classification. |
actions/setup/js/threat_detection_warning.test.cjs |
Tests reason classification. |
actions/setup/js/messages_run_status.cjs |
Selects warning or caution messages. |
actions/setup/js/messages.test.cjs |
Tests footer message behavior. |
actions/setup/js/generate_footer.cjs |
Updates expired-entity alerts. |
actions/setup/js/generate_footer.test.cjs |
Tests both tooling-failure reasons. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Medium
| const isEngineError = isToolingFailureReason(ctx.reason); | ||
| if (isEngineError) { | ||
| const defaultTemplate = `> [!WARNING]\n> threat detection engine error\n> The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.\n> ${getThreatDetectedMarkerTemplate()}\n>\n> <details>\n> <summary>Details</summary>\n>\n> {reason_text}\n>\n> Review the [workflow run logs]({run_url}) for details.\n> </details>`; | ||
| return renderConfiguredMessage("detectionEngineError", defaultTemplate, { ...ctx, reasonText, threatKinds: normalizeThreatKinds(ctx.reason) }); |
| function isToolingFailureReason(reason) { | ||
| const normalized = String(reason || "").trim(); | ||
| return normalized === "agent_failure" || normalized === "parse_error"; |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48940 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. ✅ |
|
✅ 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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look good. The separation of [!WARNING] for tooling failures (agent_failure, parse_error) vs [!CAUTION] for actual threat findings is a meaningful UX improvement. The isToolingFailureReason() helper is correctly implemented, well-documented, and has comprehensive test coverage.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.7 AIC · ⌖ 5.37 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design — requesting changes for two pre-existing comments plus one new finding.
📋 Key Themes & Highlights
Key Themes
- Incomplete coverage (pre-existing comment #3676223067):
push_to_pull_request_branch.cjsstill hardcodes[!CAUTION]for the samewarningconclusion, leaving a parallel code path un-fixed. - Unreachable config key (pre-existing comment #3676223001):
detectionEngineErroris not a supported key inSafeOutputMessagesConfig, so the user-customizable path is silently dead. - Duplicate template strings (new inline comment on
generate_footer.cjs:135): identical warning template exists in two files with no shared constant; any wording change requires two edits.
Positive Highlights
- ✅ Clean
isToolingFailureReasonpredicate — well-named, single-responsibility, easy to extend. - ✅ Good test coverage with descriptive names that read as specifications.
- ✅ Clear before/after table in the PR description makes the intent obvious.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 22.8 AIC · ⌖ 4.94 AIC · ⊞ 7K
Comment /matt to run again
| } | ||
| const detectionReason = process.env.GH_AW_DETECTION_REASON || ""; | ||
| const reasonText = getDetectionReasonText(detectionReason); | ||
| if (isToolingFailureReason(detectionReason)) { |
There was a problem hiding this comment.
[/codebase-design] The inline template string here is a verbatim copy of the one in messages_run_status.cjs. Any future wording change to the warning banner must be applied in two places. Since threat_detection_warning.cjs is already imported by both files, extracting the shared template there would keep the wording in sync automatically.
💡 Suggested approach
In threat_detection_warning.cjs, export a constant:
const TOOLING_FAILURE_WARNING_TEMPLATE =
`> [!WARNING]
> threat detection engine error
> The threat detection engine encountered an error...`;
module.exports = { ..., TOOLING_FAILURE_WARNING_TEMPLATE };Both callers import and interpolate it, removing the duplication.
@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
📊 Metrics (9 tests)
|
|
🎉 This pull request is included in a new release. Release: |
When the threat detection engine crashes (
agent_failure) or produces unparseable output (parse_error), the resulting banner was visually identical to a genuine security finding — both showed[!CAUTION] agentic threat detected. Reviewers had no signal to distinguish a broken tooling job from an actual threat, causing alert fatigue and masking the real infrastructure bug.Changes
threat_detection_warning.cjs— addsisToolingFailureReason(reason)returningtrueforagent_failureandparse_errormessages_run_status.cjs—getDetectionWarningMessage()now branches onisToolingFailureReason:[!WARNING]/threat detection engine error/detectionEngineErrormessage key (user-customizable)[!CAUTION]/agentic threat detectedgenerate_footer.cjs—getExpiredEntityCautionAlert()applies the same distinction (kept inline to avoid bundler/test issues withmessages_footer.cjs)Before / After
threat_detected[!CAUTION] agentic threat detected[!CAUTION] agentic threat detected(unchanged)agent_failure[!CAUTION] agentic threat detected[!WARNING] threat detection engine errorparse_error[!CAUTION] agentic threat detected[!WARNING] threat detection engine error