Surface external threat-detection engine failures in warn-mode fallback#44605
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
PR Triage - Run 29041860272. Category: bug | Risk: Low | Score: 48/100 | Action: defer. DRAFT. Small fix (68+/2-) surfacing threat-detection engine error details in warn-mode. Undraft and let CI pass to re-evaluate for auto_merge.
|
🤖 PR Triage — Run §29061171191
Surface external threat-detection engine failures in warn-mode fallback. DRAFT, 2 files, 68 additions. Low risk. Defer until undrafted.
|
There was a problem hiding this comment.
Pull request overview
This pull request improves diagnosability of warn-mode threat detection failures by enhancing the conclude_threat_detection.sh wrapper to surface the detector’s last THREAT_DETECTION_STATUS: line when detection_result.json is missing, and by adding test coverage for this missing-result behavior.
Changes:
- Updated the conclude wrapper to look for a sibling
detection.logand include the lastTHREAT_DETECTION_STATUS:line in the surfaced warning/error message when results are missing. - Improved fallback messaging to distinguish between “log missing” vs “log present but no status line”.
- Added a focused Go test verifying detector status lines and execution outcome are surfaced while preserving warn-mode contract outputs.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/sh/conclude_threat_detection.sh | Enhances missing-result fallback messaging by incorporating detector status from detection.log. |
| pkg/workflow/threat_detection_conclude_script_test.go | Adds test coverage ensuring warn-mode output surfaces detector status and preserves conclusion/reason outputs. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
|
|
||
| if [ "${continue_on_error}" = "true" ]; then | ||
| echo "::warning::Detection result file not found at: ${RESULT_FILE} (execution outcome: ${DETECTION_AGENTIC_EXECUTION_OUTCOME:-unknown}); continuing because GH_AW_DETECTION_CONTINUE_ON_ERROR=true" | ||
| echo "::warning::${result_message}; continuing because GH_AW_DETECTION_CONTINUE_ON_ERROR=true" |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44605 does not have the 'implementation' label and has only 49 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Surface external threat-detection engine failures in warn-mode fallback
The change is focused and correct. The shell logic is well-structured and the new test exercises the primary code path.
One non-blocking observation: the new branches for "log exists but has no status line" and "log file not found" are untested. While the existing test matrix gives reasonable confidence, adding coverage for those two branches would make future regressions immediately visible.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 10.7 AIC · ⌖ 4.29 AIC · ⊞ 4.8K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (3 tests)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — requesting changes on two issues.
📋 Key Themes & Highlights
Key Themes
- Missing branch coverage: only the happy-path branch (log exists + status line found) is tested; the two other new branches (log exists but no status, log missing) have no coverage.
grepwithout-F: the prefix match could behave unexpectedly if the prefix ever contains regex metacharacters.
Positive Highlights
- ✅ Three-tier fallback is well-structured and covers all reachable log states.
- ✅ Existing test extended cleanly;
detectorStatusLineconstant promotes reuse. - ✅ Preserves the existing gate contract (
conclusion=warning,reason=agent_failure) — non-blocking behaviour is unchanged.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 27.9 AIC · ⌖ 4.55 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| func TestConcludeThreatDetectionScript_InvokesThreatDetectConclude(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| scriptPath := filepath.Join("..", "..", "actions", "setup", "sh", "conclude_threat_detection.sh") |
There was a problem hiding this comment.
[/tdd] Only one of three new branches is tested — the "log exists but has no status line" and "log not found" paths are uncovered.
💡 Suggested additions
func TestConcludeThreatDetectionScript_MissingResultLogExistsNoStatus(t *testing.T) {
// Write a log with no THREAT_DETECTION_STATUS line
os.WriteFile(detectionLog, []byte("some other output\n"), 0600)
// ... run script, assert message contains "did not include THREAT_DETECTION_STATUS:"
}
func TestConcludeThreatDetectionScript_MissingResultNoLog(t *testing.T) {
// Do not write detection.log
// ... run script, assert message contains "detection log not found at"
}Without these, a regression in either branch (e.g., wrong variable reference) would go undetected.
@copilot please address this.
| DETECTION_STATUS_PREFIX="THREAT_DETECTION_STATUS:" | ||
| continue_on_error="${GH_AW_DETECTION_CONTINUE_ON_ERROR:-true}" | ||
| continue_on_error="$(echo "${continue_on_error}" | tr '[:upper:]' '[:lower:]')" | ||
|
|
There was a problem hiding this comment.
[/diagnosing-bugs] grep without -F treats THREAT_DETECTION_STATUS: as a regex pattern — : and any future special characters in the prefix could cause unexpected matches or failures.
💡 Fix
detection_status="$(grep -F "${DETECTION_STATUS_PREFIX}" "${DETECTION_LOG_FILE}" | tail -n 1 || true)"Use -F (fixed-string) to match literally and avoid unintended regex interpretation.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — security-adjacent issues must be fixed before merge
The diagnostic improvement is worthwhile, but the PR introduces a trust-boundary violation by embedding log file content directly into GitHub Actions workflow annotations and output files without sanitization.
Blocking themes
-
Workflow command injection (critical) —
detection_statusfrom the external detector's log is interpolated verbatim into::warning::.... A log line containing%0A::set-env::or raw LF will cause the runner to process injected workflow commands. Both%-encoding and newline stripping are required before anyecho ::...::emission. -
GITHUB_OUTPUTcorruption (high) — Multiline or newline-embedded content indetection_statusflows intoresult_message, which can break thekey=valueformat expected byGITHUB_OUTPUT, allowing attacker-controlled lines to override downstream step outputs.tr -d '\r\n'on the grep result is required. -
grepwithout-F(medium) —DETECTION_STATUS_PREFIXis used as a regex, not a fixed string. Usegrep -Fto prevent future metacharacter accidents.
Test coverage for the continue_on_error=false (error exit) path with the new status message is also missing but is not individually blocking.
🔎 Code quality review by PR Code Quality Reviewer · 32.4 AIC · ⌖ 4.76 AIC · ⊞ 5.4K
Comment /review to run again
| echo "::warning::${result_message}; continuing because GH_AW_DETECTION_CONTINUE_ON_ERROR=true" | ||
| echo "conclusion=warning" >> "${GITHUB_OUTPUT}" | ||
| echo "success=false" >> "${GITHUB_OUTPUT}" | ||
| echo "reason=agent_failure" >> "${GITHUB_OUTPUT}" |
There was a problem hiding this comment.
Workflow command injection: detection_status from an external log file flows unsanitized into a ::warning:: annotation and can inject arbitrary workflow commands.
💡 Details and fix
A detector log line like:
THREAT_DETECTION_STATUS: foo%0A::add-mask::mysecret
will cause the runner to process ::add-mask::mysecret as a real workflow command — masking or leaking arbitrary values. On older runners ::set-env:: could corrupt runner environment state.
The existing reviewer comment addresses % encoding but raw LF/CR bytes in the log file have the same effect. Sanitize before embedding:
detection_status="$(printf '%s' "${detection_status}" | tr -d '\r\n' | sed 's/%/%25/g')"
result_message="$(printf '%s' "${result_message}" | tr -d '\r\n' | sed 's/%/%25/g')"Apply this to every value derived from log file content before it reaches any echo ::...:: or >> "${GITHUB_OUTPUT}" write.
| if [ -f "${DETECTION_LOG_FILE}" ]; then | ||
| detection_status="$(grep "${DETECTION_STATUS_PREFIX}" "${DETECTION_LOG_FILE}" | tail -n 1 || true)" | ||
| fi | ||
|
|
There was a problem hiding this comment.
Multiline detection_status corrupts GITHUB_OUTPUT: if the log file contains multiple THREAT_DETECTION_STATUS: lines (or a line with embedded newlines), the grep+tail result can still be multi-line, breaking the key=value format written to GITHUB_OUTPUT.
💡 Details and fix
tail -n 1 only helps if grep returns clean single lines. A log line with an embedded literal \n (e.g., from ANSI sequences or log formatting) survives through grep | tail. The result is then interpolated into:
echo "conclusion=warning" >> "${GITHUB_OUTPUT}"...but result_message containing a newline will inject an attacker-controlled extra line into GITHUB_OUTPUT, potentially overriding downstream step outputs.
Fix: collapse detection_status to a single line before use:
detection_status="$(grep -F "${DETECTION_STATUS_PREFIX}" "${DETECTION_LOG_FILE}" | tail -n 1 | tr -d '\r\n')"Note the addition of -F (fixed string) to prevent the prefix from being treated as a regex, and tr -d '\r\n' to strip any embedded line endings.
| if [ -f "${DETECTION_LOG_FILE}" ]; then | ||
| detection_status="$(grep "${DETECTION_STATUS_PREFIX}" "${DETECTION_LOG_FILE}" | tail -n 1 || true)" | ||
| fi | ||
|
|
There was a problem hiding this comment.
grep without -F flag is a latent correctness bug: grep "${DETECTION_STATUS_PREFIX}" treats the prefix as a regex pattern. While the current literal THREAT_DETECTION_STATUS: is harmless, the variable is not readonly and a future change or env override introducing regex metacharacters (e.g., ., *, [) would silently broaden the match and capture unintended log lines into the warning.
💡 Fix
detection_status="$(grep -F "${DETECTION_STATUS_PREFIX}" "${DETECTION_LOG_FILE}" | tail -n 1 || true)"Using -F (fixed-string mode) makes the match deterministic regardless of the prefix value.
Warn-mode threat detection could fall back to a generic
agent_failurewhen the external detector exited without producingdetection_result.json, leaving the real failure mode buried in logs. In the reported run, the detector emittedTHREAT_DETECTION_STATUS: reason=engine_error exit=2, but the conclude step only surfaced “failed to produce results.”What changed
detection.logwhendetection_result.jsonis missing.THREAT_DETECTION_STATUS:line in the warning/error message.Why this matters
conclusion=warning,reason=agent_failure) while making the alert materially more actionable.Coverage