threat-detection: delegate all conclude branches to `threat-detect conclude` on the external path

[davidslater]

Created by GitHub Ace · View Session

Summary

On the external-detector path (features: gh-aw-detection: true), actions/setup/sh/conclude_threat_detection.sh re-implements the conclusion contract in bash and exits before ever invoking threat-detect conclude on the two branches where the detector's diagnostics matter most. The binary's equivalent branches are therefore dead code in production, and the two implementations have drifted.

Depends on the v0.4.0 pin (#50644).

Current code

actions/setup/sh/conclude_threat_detection.sh (46 lines total):

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

if [ ! -f "${RESULT_FILE}" ]; then
  ...
  if [ "${continue_on_error}" = "true" ]; then ... exit 0; fi
  echo "ERR_SYSTEM: ❌ ${result_message}"; exit 1
fi

threat-detect conclude --result-file "${RESULT_FILE}"   # happy path only

What is lost today

Gap	Effect	Upstream spec rule
Status-reason mapping bypassed	Every failure collapses to agent_failure. invalid_report_exhausted and output_write_error never map to parse_error, discarding the granularity #48940 keys severity off. Ironically the script does grep the THREAT_DETECTION_STATUS: line — only to interpolate it into a message string.	TD-20b
Diagnostics bypassed	No recursive result-directory listing, no detection-log line/byte stats, no echoed THREAT_DETECTION_STATUS: / THREAT_DETECTION_RESULT: marker lines	TD-20d
Verdict step summary bypassed	No summary block for skipped or engine-failure outcomes — so the new <!-- gh-aw-threat-engine-error --> marker and "Threat Detection Engine Failure" title from v0.4.0 are never emitted on exactly the outcomes they exist for	TD-20h, TD-20i
Hard-fail rule bypassed	conclude (and the inline .cjs path) hard-fail when DETECTION_AGENTIC_EXECUTION_OUTCOME=failure even in warn mode. The script warns and exits 0 → fails open. A failed detection execution can currently pass the gate on the external path where the inline path would block.	TD-20b
Skip branch violates env contract	Writes GITHUB_OUTPUT only, never exports GH_AW_DETECTION_CONCLUSION / GH_AW_DETECTION_REASON to GITHUB_ENV	U-18
continue_on_error normalization drift	Script uses ${...:-true} + lowercasing; detector uses != "false". Two implementations, subtly different.	—

Note the last row is a real divergence risk, and the hard-fail row is a security-relevant fail-open, not a cosmetic difference.

threat-detect conclude already covers every branch

Per upstream, conclude in v0.4.0 handles:

• RUN_DETECTION != "true" → conclusion=skipped, success=true, reason=, plus the skipped verdict summary block
• missing / unreadable / malformed result file → status-log reason mapping, full diagnostics, engine-error marker + summary block
• warn mode via GH_AW_DETECTION_CONTINUE_ON_ERROR != "false", with the DETECTION_AGENTIC_EXECUTION_OUTCOME=failure hard-fail override
• writes conclusion / reason / success to $GITHUB_OUTPUT and exports GH_AW_DETECTION_CONCLUSION / GH_AW_DETECTION_REASON to $GITHUB_ENV
• exits 0 (proceed) / 1 (block), matching the current gate contract
• defaults --detection-log to <result-file-dir>/detection.log — verified byte-identical to constants.ThreatDetectionLogPath and the script's ${RESULT_DIR}/detection.log
• defaults --step-summary to ambient $GITHUB_STEP_SUMMARY

Proposed change

Reduce the script to roughly:

RESULT_FILE="${1:-/tmp/gh-aw/threat-detection/detection_result.json}"
RESULT_DIR="$(dirname "${RESULT_FILE}")"
DETECTION_LOG_FILE="${DETECTION_LOG_FILE:-${RESULT_DIR}/detection.log}"

if ! command -v threat-detect >/dev/null 2>&1; then
  # the one failure conclude cannot handle itself
  ...
fi

threat-detect conclude \
  --result-file   "${RESULT_FILE}" \
  --detection-log "${DETECTION_LOG_FILE}"

Pass --detection-log explicitly even though the default matches, so the contract is robust to path-layout changes and visible in the compiled lock file.

Prerequisite already satisfied: delegating on every branch requires threat-detect on the host PATH in all cases. install_threat_detect_binary.sh installs to /usr/local/bin (or $HOME/.local/bin + $GITHUB_PATH in rootless mode) on the runner before the AWF run, so this holds. Keep the command -v guard as the sole remaining shell-side special case.

Open behavioral question to settle in this issue

threat-detect conclude has a fail-closed rule the .cjs does not:

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

i.e. in warn mode it still fails closed when the engine step also failed. setDetectionFailure in parse_threat_detection_results.cjs always warns and proceeds, per ADR 29031-threat-detection-resilience-in-warn-mode. Delegating makes this a behavior change for every workflow currently in warn mode, appearing silently the moment a workflow sets gh-aw-detection: true.

Two options, and upstream is happy either way — they just want the decision recorded:

1. Upstream drops mustFail to match ADR 29031 exactly.
2. gh-aw adopts it on the inline path too, and ADR 29031 is amended.

This decision must be made before merging. Option 2 is the safer security posture but is a behavior change on the inline path and requires an ADR amendment.

Acceptance criteria

• mustFail decision recorded (ADR amendment or upstream issue comment).
• Script delegates on all branches; only the command -v threat-detect guard remains.
• --detection-log passed explicitly.
• pkg/workflow/threat_detection_conclude_script_test.go updated for the new shape.
• Verify detection.log is actually written at constants.ThreatDetectionLogPath before the conclude step runs (the AWF log tee should already do this — confirm).
• make recompile.

References

• Parity: conclusion logic is duplicated in conclude_threat_detection.sh and threat-detect conclude, with divergent (and partly broken) semantics gh-aw-threat-detection#694
• Parity: parse_error is unreachable on the external path — THREAT_DETECTION_STATUS reasons are collapsed to agent_failure gh-aw-threat-detection#693 (reason mapping table)
• Parity: threat-detect conclude emits far less diagnostic output than parse_threat_detection_results.cjs gh-aw-threat-detection#692 (diagnostics)
• Upstream draft (for github/gh-aw): compiler-side changes needed for external threat-detect parity gh-aw-threat-detection#726 (item F)