fix(monitor): surface warn-mode detection failures after gh-aw #52400 - #880
Conversation
gh-aw's compiler now marks `Install threat-detect binary` continue-on-error in warn mode, so a failed download of this repo's released binary leaves the whole detection job green. The monitor previously scanned only failure-status runs and only read job conclusions, so it silently missed this class. Step conclusions cannot distinguish these: the Actions runner rewrites a continue-on-error step's Result to Succeeded (keeping the failure only in the workflow-expression `outcome`, which REST does not expose). The conclusion step's warning markers in the job log are the real evidence, so add a bounded tail read (last 200 lines, at most 30 jobs per run) alongside scanning success-status runs. Also add spec U-05a documenting the host-side rule that an acquisition failure MUST be tolerated no more strictly than the configured detection-failure policy, and MUST NOT synthesize a `safe` verdict. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Extends detection monitoring to identify warn-mode failures hidden by green jobs and documents acquisition-failure policy.
Changes:
- Scans successful runs and inspects bounded job-log tails.
- Reports hard versus soft failures.
- Adds host acquisition-failure requirements.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/detection-failure-monitor.md |
Adds soft-failure detection and reporting. |
specs/usage-spec.md |
Defines acquisition-failure behavior by policy mode. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
| 1. Build the candidate run set from two bounded listings, both restricted to runs | ||
| whose `created_at` (or `updated_at`) is within the **last 24 hours**, most | ||
| recent first, and neither paginated further: | ||
| - a. runs with `status: failure` — at most the 50 most recent (these can | ||
| hard-fail or soft-fail), | ||
| - b. runs with `status: success` — at most the 30 most recent (these can only | ||
| soft-fail: a green run whose detection job never got a verdict is exactly | ||
| what a broken binary install looks like now). |
| the download, then surface an unrecoverable acquisition failure as a warning | ||
| with the `agent_failure` outcome (per U-20) rather than as a job failure — the | ||
| absent detector is the same non-fatal condition its conclusion step already | ||
| tolerates. In strict mode the acquisition failure MUST fail the job, because |
| - `threat-detect binary not found on PATH` (the binary never installed), | ||
| - a `::warning::` line naming threat detection, or a `⚠️` threat-detection | ||
| warning banner, | ||
| - `reason=agent_failure`, `reason=parse_error`, or `reason=threat_detected`, | ||
| - `conclusion=warning`. |
…ision Three fixes from the PR review: 1. github-mcp-server v1.9.0's list_workflow_runs `status` enum only accepts lifecycle values (queued/in_progress/completed/requested/waiting), not result values. Switch to listing `status: completed` runs (80 most recent) and partition by each run's `conclusion` in-agent. 2. The broad soft-failure markers (`::warning::`, `⚠️ `, `reason=threat_detected`, `conclusion=warning`) would misclassify a legitimate warn-mode threat verdict as a detector failure: parse_threat_detection_results.cjs emits all four via setDetectionFailure("threat_detected", ...) with mustFail=false. Restrict the marker list to genuine tooling failures: `threat-detect binary not found on PATH`, `reason=agent_failure`, `reason=parse_error`. 3. U-05a as written conflicted with U-20's clause that hard-fails agent_failure/parse_error when the execution step also failed. Since a soft-failed install lets the execution step run and fail on a missing binary, a warn-mode host that followed both rules would still hard-fail on acquisition failure. Extend U-05a to require the host to prevent that execution-outcome propagation — either by skipping the execution step or by guarding it with the same missing-binary check conclude_threat_detection.sh uses today — so U-20's exception does not trigger. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>
|
Thanks — all three land, and all three are fixed in 1ad2c1c. Comment 1 (MCP Comment 3 (misclassifying real threat verdicts). Also confirmed. Comment 2 (U-05a / U-20 conflict). Real conflict, and thanks for catching it. Given I did NOT loosen U-20 itself — the mandatory hard-fail on a genuinely failed execution step is still the right rule; U-05a just prevents an acquisition failure from becoming a failed-execution-step outcome. Open to reframing this as an explicit U-20 exception if you prefer that shape instead. |
Summary
gh-aw PR #52400 added
continue-on-error: trueto theInstall threat-detect binarystep in warn mode. In gh-aw's ~104 warn-mode workflows, a failed download of our released binary now leaves thedetectionjob green.detection-failure-monitor.mdonly scannedstatus: failureruns and only read job conclusions, so a broken/missing release asset — squarely our responsibility — would never be reported.The detector contract (result JSON, exit codes,
conclude) is unchanged. Our smokes pinsafe-outputs.threat-detection.continue-on-error: false, andresolveThreatDetectionContinueOnErroremits nocontinue-on-errorin strict mode, so a broken release binary still fails our smokes. Only the monitor's blind spot needs closing.Why the fix reads logs instead of step conclusions
The Actions runner's
ExecutionContext.ApplyContinueOnErrordoesOutcome = Result; Result = Succeeded, and REST exposes only the rewrittenresult. So every step of a soft-failed detection job reportsconclusion: "success"; a step-conclusion rule can never match. A scan of ~40 live gh-aw runs found zero green-job/failed-step pairs, consistent with that. The real evidence is the conclusion step's warning markers in the job log, so the check is a bounded tail (last 200 lines, at most 30 jobs per monitor run).Changes
.github/workflows/detection-failure-monitor.md— also scanstatus: successruns (30 most recent, 24h window); for green detection jobs, tail-read the last 200 lines of the job log for conclusion-step markers (threat-detect binary not found on PATH,::warning::,⚠️,reason=agent_failure|parse_error|threat_detected,conclusion=warning); recordfailure_kind: jobvsstep; extend the report bullet and JSON schema withfailure_kind.specs/usage-spec.md— new U-05a documenting the host-side rule that an acquisition failure MUST be tolerated no more strictly than the configured detection-failure policy, MUST surface asagent_failurein warn mode, MUST fail the job in strict mode, and MUST NOT synthesize asafeverdict.No recompile needed
detection-failure-monitor.lock.ymlembeds the source via{{#runtime-import .github/workflows/detection-failure-monitor.md}}and the frontmatter is unchanged; CI has no lock-freshness check.Open item
In warn mode,
conclude'smustFailis only set for tooling failures when the execution step also failed, so eventhreat_detectedexits 0 andsafe_outputsproceeds. That's upstream gh-aw policy, deliberately not changed here.