fix: use distinct XML marker for threat detection engine errors vs real threats - #49527
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…al threats Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49527 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Separates threat-engine failures from genuine security findings using a distinct XML marker and warning presentation.
Changes:
- Adds
<!-- gh-aw-threat-engine-error -->marker helpers. - Applies engine-error messaging to status output and review PR bodies.
- Updates marker expectations across related tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/threat_detection_warning.cjs |
Adds engine-error marker selection. |
actions/setup/js/messages_run_status.cjs |
Uses the new marker in warning messages. |
actions/setup/js/push_to_pull_request_branch.cjs |
Differentiates review PR messaging by reason. |
actions/setup/js/threat_detection_warning.test.cjs |
Tests marker helpers. |
actions/setup/js/messages.test.cjs |
Verifies engine-error message markers. |
actions/setup/js/generate_footer.test.cjs |
Updates footer marker expectations. |
actions/setup/js/update_activation_comment.test.cjs |
Updates the mocked warning output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Balanced
| isEngineError ? "> [!WARNING]" : "> [!CAUTION]", | ||
| isEngineError ? "> threat detection engine error" : "> agentic threat detected", | ||
| isEngineError | ||
| ? "> The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding." | ||
| : "> Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.", |
| const isEngineError = reason === "agent_failure" || reason === "parse_error"; | ||
| if (isEngineError) { | ||
| return `> [!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> <!-- gh-aw-threat-engine-error -->\n>\n> ${reasonText}`; |
| 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>`; | ||
| 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> ${getThreatEngineErrorMarkerTemplate()}\n>\n> <details>\n> <summary>Details</summary>\n>\n> {reason_text}\n>\n> Review the [workflow run logs]({run_url}) for details.\n> </details>`; |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation correctly separates threat-engine-error markers from real-threat markers across all code paths. Tests cover the full matrix of inputs. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 11.7 AIC · ⌖ 10 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is correct and well-tested; two minor maintainability issues noted.
📋 Key Themes & Highlights
Key Themes
- Duplicated condition in test helper —
update_activation_comment.test.cjsre-implementsisToolingFailureReason()inline rather than importing it, which will silently drift if new tooling-failure reasons are added. - Dual-path PR body construction —
push_to_pull_request_branch.cjsbranches onisEngineErrorfor the banner lines and callsgetThreatDetectedMarker(reason)(which branches internally), creating two independent paths that must stay in sync manually.
Positive Highlights
- ✅ Clean, focused fix — root cause (shared marker constant) addressed at the source in
threat_detection_warning.cjs - ✅
getThreatDetectedMarker(reason)now acts as a single routing point for the marker decision - ✅ Tests across four test files assert both the positive case (engine-error marker for tooling failures) and the negative case (
.not.toContainfor the real-threat marker) — good regression discipline - ✅ PR description includes a clear before/after mapping table
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 35.7 AIC · ⌖ 8.04 AIC · ⊞ 7.1K
Comment /matt to run again
| parse_error: "The threat detection results could not be parsed.", | ||
| }; | ||
| const reasonText = reasons[reason] || "The threat detection analysis could not be completed."; | ||
| const isEngineError = reason === "agent_failure" || reason === "parse_error"; |
There was a problem hiding this comment.
[/tdd] The isEngineError check here duplicates the logic from isToolingFailureReason() instead of importing it — if tooling-failure reasons ever expand, this test helper will silently diverge from production logic.
💡 Suggested fix
Import isToolingFailureReason from threat_detection_warning.cjs and use it in the test fixture instead of the inline literal check:
// replace
const isEngineError = reason === 'agent_failure' || reason === 'parse_error';
// with (at the top of createTestableFunction or its mock)
const { isToolingFailureReason } = require('./threat_detection_warning.cjs');
const isEngineError = isToolingFailureReason(reason);This keeps the test fixture in sync with production logic automatically.
@copilot please address this.
| isEngineError | ||
| ? "> The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding." | ||
| : "> Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.", | ||
| `> ${getThreatDetectedMarker(detectionReasonEnv)}`, |
There was a problem hiding this comment.
[/diagnosing-bugs] The new isEngineError branch selects the correct marker text but still calls getThreatDetectedMarker(detectionReasonEnv) on line 1314 — which already performs the same routing internally. The banner text and the marker are now computed independently, so a future change to one path won't automatically keep the other in sync.
💡 Suggested simplification
Since getThreatDetectedMarker(reason) already returns the right marker for both cases, there is no need to duplicate the isEngineError branching for the body lines above it. Consider extracting the PR body construction into a helper (e.g. in threat_detection_warning.cjs) so the banner, text, and marker are always derived from the same source of truth.
Alternatively, add a unit test for the PR body output in this file to lock in the expected marker for both agent_failure and threat_detected reasons.
@copilot please address this.
Test Quality Sentinel Report test
|
|
@copilot quick triage for this PR:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in
The failed PR Code Quality Reviewer run was a reviewer-agent exit on the previous head; CI for this new head is still stale and will need a maintainer re-trigger. |
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. @copilot quick triage for this PR:
|
Checked the remaining skills-based review items on the current head: the DRY marker-selection concern is already addressed via Validation on
The earlier PR Code Quality Reviewer failure was on the previous head and its job log did not show a code regression to repair here. Current CI on the latest head still needs a maintainer re-trigger before merge. |
PR TriageCategory: bug · Risk: medium · Priority: medium · Score: 68/100 (impact 30, urgency 22, quality 16) Recommended action: Fixes alert-fatigue bug: tooling failures used the same XML marker as real security threats. Ready for review (9 comments, 5 review comments), copilot CI check passed. High value for signal quality — fast-track.
|
PR Triage
Distinct XML marker prevents false-positive threat alerts across daily reports; 2 approvals.
|
|
@copilot quick triage for this PR:
|
Checked the remaining github-actions review items on
Validation on the current head:
No further code changes were needed in this pass. Remaining maintainer action before merge: re-trigger CI on the current head after reviewing the existing unrelated |
Tooling failures (
agent_failure,parse_error) emitted the same<!-- gh-aw-threat-detected -->XML marker as actual security findings, making them indistinguishable to automated scanners and causing alert fatigue across dozens of daily reports.Changes
threat_detection_warning.cjs— addedgetThreatEngineErrorMarker()/getThreatEngineErrorMarkerTemplate()returning<!-- gh-aw-threat-engine-error -->. ModifiedgetThreatDetectedMarker(reason)to delegate to the engine-error marker whenisToolingFailureReason(reason)is true.messages_run_status.cjs— the[!WARNING]engine-error banner template now embedsgetThreatEngineErrorMarkerTemplate()instead ofgetThreatDetectedMarkerTemplate().push_to_pull_request_branch.cjs— review PR bodies were unconditionally[!CAUTION] agentic threat detected; now gates onisToolingFailureReason()and uses[!WARNING] threat detection engine errorwith the correct marker for tooling failures.Tests — updated
generate_footer,messages,threat_detection_warning, andupdate_activation_commenttests to assert<!-- gh-aw-threat-engine-error -->for tooling failures and<!-- gh-aw-threat-detected -->exclusively for real threat verdicts.Before / after marker mapping:
threat_detected[!CAUTION]<!-- gh-aw-threat-detected --><!-- gh-aw-threat-detected -->agent_failure[!WARNING]<!-- gh-aw-threat-detected --><!-- gh-aw-threat-engine-error -->parse_error[!WARNING]<!-- gh-aw-threat-detected --><!-- gh-aw-threat-engine-error -->Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
run: https://github.com/github/gh-aw/actions/runs/30692749654