threat-detection: drop macOS support from installer, add platform-mapping tests and compiler test coverage#50719
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR #50719 does not have the 'implementation' label and has only 13 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Attempts to make threat detection Linux-only and expand platform validation coverage.
Changes:
- Rejects macOS in the installer.
- Adds shell platform-mapping tests.
- Tests string-form macOS compiler validation.
Show a summary per file
| File | Description |
|---|---|
actions/setup/sh/install_threat_detect_binary.sh |
Removes macOS installation support. |
actions/setup/sh/install_threat_detect_binary_test.sh |
Adds platform-mapping tests. |
pkg/workflow/runs_on_validation_test.go |
Covers nested string-form macOS validation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
| ;; | ||
| Darwin) | ||
| install_darwin_binary | ||
| echo "ERROR: macOS is not a supported platform for threat-detect. Use a Linux runner for threat-detection jobs." |
There was a problem hiding this comment.
Fixed in 62ca07c. Darwin install support is restored with threat-detect-darwin-x64 and threat-detect-darwin-arm64, and the installer header now documents macOS binaries as unsigned/not notarized.
| bash -c ' | ||
| OS="$1" | ||
| ARCH="$2" | ||
| case "$OS" in |
There was a problem hiding this comment.
Fixed in 62ca07c. The test now sources actions/setup/sh/install_threat_detect_binary.sh and directly calls the production resolve_binary_name function instead of duplicating dispatch logic.
| set +o histexpand | ||
|
|
||
| # Tests for install_threat_detect_binary.sh OS/arch → asset-name mapping logic. | ||
| # Run: bash install_threat_detect_binary_test.sh |
There was a problem hiding this comment.
Fixed in 62ca07c. The new shell test is now wired into test-scripts in /home/runner/work/gh-aw/gh-aw/Makefile, so it runs in automated test targets.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The changes are clean and correct.
- installer: Removed the latent
install_darwin_binary()branch and replaced theDarwin)dispatch case with a clear early-exit error message. The error is actionable and matches the documented Linux-only platform support. - shell tests: The new test file covers all meaningful code paths (Linux x86_64, aarch64/arm64, Darwin rejection, unknown OS, unsupported arch). Tests inline the platform logic rather than sourcing the main script, keeping them self-contained and fast.
- Go test: The added test case correctly covers the string-form
"macos-latest"path invalidateRunsOn, complementing the existing object-form test.
No correctness, security, or reliability issues found.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.3 AIC · ⊞ 5.3K
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report
📊 Metrics (7 tests)
Test Classification📋 All Tests (7 total)
Observations✅ Strengths:
Verdict
|
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — requesting changes on two correctness issues and one test-architecture concern.
📋 Key Themes & Highlights
Key Issues
- Test logic diverges from production (
install_threat_detect_binary_test.shlines 16–38): The test re-implements the platformcaseblock inline rather than sourcing the real script. Tests pass even if the real logic changes. amd64alias mismatch (line 23 of test vs real script): The test expectsamd64to resolve correctly, but the realinstall_linux_binary()only matchesx86_64. One of them is wrong.- Exit codes not asserted in error-path tests (Tests 4, 5, 6): Only the stderr message is checked; a zero exit code would still pass the tests.
Positive Highlights
- ✅ Clean removal of the latent
install_darwin_binary()function with an actionable error message - ✅ Good coverage table in the PR description — all rejection paths documented
- ✅ Go test for the string-form
macos-latestcase closes a real validation gap - ✅ Comment header updated correctly to document Linux-only support
| exit 1 | ||
| ;; | ||
| esac | ||
| ' -- "$os" "$arch" |
There was a problem hiding this comment.
[/tdd] The test file duplicates the platform-selection logic in a bash -c heredoc rather than sourcing and calling the real script — so the tests verify a copy of the logic, not the actual implementation in install_threat_detect_binary.sh. If the two diverge (e.g. someone adds amd64 as an alias in the real script but not here), the tests will still pass while the production path silently breaks.
💡 Suggested approach
Source only the helper function from the real script, or extract the case block into a shared helper that both the installer and the test can call. A minimal alternative:
# At the top of the test file:
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")")" && pwd)"
source "${SCRIPT_DIR}/install_threat_detect_binary.sh"
# Then call resolve_binary_name() directly instead of re-implementing it inline.This ensures the tests break when the real logic changes.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 62ca07c by extracting and sourcing the production resolver. The test now validates resolve_binary_name from the real installer script.
| fail "Unknown OS did not produce expected error" "got: $error_output" | ||
| fi | ||
|
|
||
| # Test 6: unsupported Linux architecture fails with actionable message |
There was a problem hiding this comment.
[/tdd] Test 4 (Darwin rejection) calls resolve_binary_name "Darwin" "arm64" 2>&1 — piping stderr into the result variable — but the exit code is lost because it is captured via $(). There is no if resolve_binary_name ...; then fail ... guard, so if the subshell somehow exits 0 with no output the test would still pass. Consider also asserting [ $? -ne 0 ] to confirm the exit code is non-zero.
💡 Suggested pattern
darwin_error=$(resolve_binary_name "Darwin" "arm64" 2>&1)
darwin_exit=$?
if [ $darwin_exit -eq 0 ] || ! echo "$darwin_error" | grep -q "macOS is not a supported platform"; then
fail "Darwin did not produce the expected unsupported-platform error (exit=$darwin_exit)" "got: $darwin_error"
else
pass "Darwin -> unsupported platform error"
fi@copilot please address this.
There was a problem hiding this comment.
Addressed in 62ca07c. Failure-path assertions now explicitly check non-zero exit status plus expected error text via assert_fails_with.
| ARCH="$2" | ||
| case "$OS" in | ||
| Linux) | ||
| case "$ARCH" in |
There was a problem hiding this comment.
[/diagnosing-bugs] The amd64 alias is handled in the test (x86_64|amd64), but the real install_linux_binary() in install_threat_detect_binary.sh only matches x86_64. If a runner reports uname -m as amd64 the installer will hit the unsupported-arch error, yet the test claims that mapping works. Either add amd64 to the real script or remove it from the test to keep them in sync.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 62ca07c. The production resolver now accepts amd64 as a Linux alias, and the test exercises that mapping against the real function.
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
🔎 Code quality review by PR Code Quality Reviewer · auto · 70.4 AIC · ⊞ 7.8K
Comment /review to run again
| bash -c ' | ||
| OS="$1" | ||
| ARCH="$2" | ||
| case "$OS" in |
There was a problem hiding this comment.
This test duplicates the OS/arch dispatch logic instead of exercising the real script, so it can pass even if install_threat_detect_binary.sh diverges from this copy.
💡 Details
resolve_binary_name() reimplements the case "$OS" / case "$ARCH" logic inline in a nested bash -c script rather than sourcing the real file. If someone edits install_linux_binary() or the Darwin)/*) cases in install_threat_detect_binary.sh — e.g. adds a new arch alias, changes an error message, or reintroduces a bug — this test suite will keep passing because it only validates its own re-implementation, not the shipped script. That defeats the purpose of a regression test for exactly the platform-support change this PR makes.
Suggested fix: extract the dispatch logic into a sourceable function (e.g. move the case blocks into a function in the main script, guarded by if [ "${BASH_SOURCE[0]}" = "${0}" ] so it only executes when run directly), then source install_threat_detect_binary.sh in the test file and call the real function instead of a hand-copied duplicate.
There was a problem hiding this comment.
Addressed in 62ca07c. The test no longer reimplements OS/arch dispatch; it sources the installer and calls the shared resolve_binary_name helper directly.
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please address the latest review feedback, refresh this branch if it is behind
Run: https://github.com/github/gh-aw/actions/runs/31060975271
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please address the latest review feedback, refresh this branch if needed, then run the
Run: https://github.com/github/gh-aw/actions/runs/31065713897
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…s-support Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 62ca07c: restored Darwin installer mappings (including unsigned/notarized note), extracted a sourceable production resolver, updated shell tests to call the real resolver with exit-code assertions, and added that test to the automated |
macOS is not a supported platform for threat-detection jobs. The installer script had a latent
install_darwin_binary()branch that would attempt a download on Darwin runners — this is now removed. The compiler already rejectssafe-outputs.threat-detection.runs-on: macos-*viavalidateRunsOn, but the string-form case lacked a test.Changes
install_threat_detect_binary.sh— removedinstall_darwin_binary()and theDarwin)dispatch case; replaced with an explicit early-exit error:macOS is not a supported platform for threat-detect. Use a Linux runner for threat-detection jobs.Updated the# Platform support:header to document Linux-only support.install_threat_detect_binary_test.sh(new) — shell tests covering all Linux OS/arch → asset-name mappings and all rejection paths:Linuxx86_64threat-detect-linux-amd64Linuxaarch64/arm64threat-detect-linux-arm64DarwinLinuxruns_on_validation_test.go— added test case for the string form of macOS insafe-outputs.threat-detection.runs-on("macos-latest"); the existing test only covered the{group, labels}object form.Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/31060975271> Generated by 👨🍳 PR Sous Chef · gpt54 · 10.7 AIC · ⊞ 5.9K · ◷
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/31065713897> Generated by 👨🍳 PR Sous Chef · gpt54 · 10.6 AIC · ⊞ 8.3K · ◷