Parity: conclusion logic is duplicated in conclude_threat_detection.sh and `threat-detect conclude`, with divergent (and partly broken) semantics

[davidslater]

Created by GitHub Ace · View Session

Part of the gh-aw threat-detection parity review (see tracking issue).

Problem

The gh-aw job-output contract (conclusion / success / reason + GH_AW_DETECTION_*) is implemented twice on the external detector path, and the two implementations disagree.

Layer 1 — actions/setup/sh/conclude_threat_detection.sh (gh-aw):

if [ "${RUN_DETECTION:-false}" != "true" ]; then ...skipped...; exit 0; fi

if [ ! -f "${RESULT_FILE}" ]; then
  ...
  if [ "${continue_on_error}" = "true" ]; then
    echo "::warning::${result_message}; continuing..."
    echo "conclusion=warning" >> "${GITHUB_OUTPUT}"
    echo "success=false"     >> "${GITHUB_OUTPUT}"
    echo "reason=agent_failure" >> "${GITHUB_OUTPUT}"
    exit 0
  fi
  echo "ERR_SYSTEM: ❌ ${result_message}"
  exit 1
fi

threat-detect conclude --result-file "${RESULT_FILE}"

Layer 2 — cmd/threat-detect/conclude.go, which reimplements the skip branch, the missing-file branch, the warn-mode branch and the output/env writes.

Divergences

1. Dead code. The shell short-circuits RUN_DETECTION != true and the missing-result-file case, so conclude.go's fs.ErrNotExist → agent_failure/ERR_SYSTEM branch and its skipped branch are effectively unreachable in production. They are tested but never exercised.

2. Strict mode drops the outputs. In the shell's continue_on_error=false missing-file branch, it exit 1s without writing conclusion or reason to $GITHUB_OUTPUT. Downstream, needs.detection.outputs.detection_reason is the empty string, so getDetectionWarningMessage() / getDetectionReasonText() fall through to the generic "The threat detection analysis could not be completed." and isToolingFailureReason("") is false — i.e. a strict-mode infrastructure failure would render as [!CAUTION] agentic threat detected, exactly the confusion gh-aw#48940 set out to fix.

3. Fail-closed divergence. conclude.go has a rule the gh-aw .cjs does not:

   mustFail := c.executionFailed && (reason == "agent_failure" || reason == "parse_error")

   In warn mode, gh-aw's setDetectionFailure always warns and proceeds, for every reason (this is the explicit intent of ADR 29031-threat-detection-resilience-in-warn-mode). We fail closed when the engine step also failed. This was a deliberate hardening choice on our side, but it is a behavior change for every workflow currently running warn mode, and it is not documented as such anywhere in gh-aw's docs. It needs to be either upstreamed into the inline path or reverted, not left as a silent difference that appears when a workflow flips to gh-aw-detection: true.

4. GH_AW_DETECTION_CONCLUSION / GH_AW_DETECTION_REASON are exported to $GITHUB_ENV by conclude.go but not by the shell's branches — inconsistent for any later step in the detection job.

Proposed work

1. Collapse to a single implementation: threat-detect conclude owns the whole contract (skip, missing file, unreadable file, malformed file, verdict). Reduce conclude_threat_detection.sh to a thin invocation, or drop it.
2. Fix (2) regardless — strict-mode infrastructure failure must still set reason=agent_failure and conclusion=failure.
3. Make an explicit, documented decision on (3). If we keep mustFail, it must be documented in specs/threat-detection-spec.md and raised with gh-aw so the inline path matches; if not, remove it.
4. Add conformance tests covering both layers' matrix: {RUN_DETECTION} × {result file present/absent/malformed} × {warn/strict} × {execution outcome}.

[davidslater]

Investigated this. Summary of findings, opened PR #738 for the actionable part in this repo:

Already fixed in this repo — cmd/threat-detect/conclude.go currently:

• Writes reason/conclusion to $GITHUB_OUTPUT on every fail path (missing/unreadable/malformed file, strict and warn mode) — divergence (2) does not reproduce here.
• Exports GH_AW_DETECTION_CONCLUSION/GH_AW_DETECTION_REASON to $GITHUB_ENV on every path, including skip — divergence (4) does not reproduce here.
• Documents the mustFail fail-closed rule and its intentional divergence from gh-aw's inline path in specs/threat-detection-spec.md (TD-20b, TD-21a) — divergence (3)'s "make an explicit documented decision" is satisfied on this side; the ADR-level upstream reconciliation with gh-aw's inline path is still open.

Confirmed still broken upstream — I fetched github/gh-aw's current actions/setup/sh/conclude_threat_detection.sh and it still has exactly the bugs described: the strict-mode missing-file branch does echo "ERR_SYSTEM..."; exit 1 without writing conclusion/reason, there's no GH_AW_DETECTION_* env export, and it short-circuits before ever calling threat-detect conclude for RUN_DETECTION != true and missing-file cases (dead-code divergence (1)). Since that script lives in github/gh-aw, not this repo, I can't fix it from here — this needs a corresponding change/issue in github/gh-aw. The simplest fix would be to make the shell script always delegate to threat-detect conclude (it already handles missing/unreadable files correctly), eliminating the duplicated logic entirely.

This PR (#738): expanded conclude_test.go to cover the remaining gaps in proposed work item (4) — the full {result state} x {warn/strict} x {execution outcome} matrix, including previously-untested combinations for malformed/unreadable files and confirming mustFail correctly excludes threat_detected.

Recommend keeping this issue open (or splitting) until the github/gh-aw side is addressed, since the core "duplicated and divergent" problem only fully resolves once that script is fixed or removed.