fix: guard conclude_threat_detection.sh so threat-detect never runs when RUN_DETECTION=false#47019
Conversation
…N_DETECTION=false Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes false failures when threat detection is skipped.
Changes:
- Adds an early skipped-detection exit.
- Adds regression coverage for
RUN_DETECTION=false.
Show a summary per file
| File | Description |
|---|---|
actions/setup/sh/conclude_threat_detection.sh |
Prevents invoking unavailable threat-detect. |
pkg/workflow/threat_detection_conclude_script_test.go |
Tests the skipped path. |
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: Medium
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47019 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (46 additions across 2 files, none in tracked business logic paths). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
The early-exit guard is correct: RUN_DETECTION!=true now exits before reaching the threat-detect conclude call, preventing the exit-127 failure on skipped-detection runs.
The test is well-structured — it removes threat-detect from PATH and asserts both GITHUB_OUTPUT and GITHUB_ENV are set to skipped on exit 0.
Non-blocking note: The missing success/reason outputs on the skipped path (flagged in the existing inline comment) are valid but consistent with how downstream consumers treat conclusion=skipped.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 13.3 AIC · ⌖ 4.36 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting two small improvements before merge.
📋 Key Themes & Highlights
Issues Found
- Missing outputs on skipped path (existing comment at line 15):
success,reason, andGH_AW_DETECTION_REASONare not emitted on theskippedpath, leaving downstream consumers of those outputs with unset values. - Partial test coverage (new comment at test line 133): Only
RUN_DETECTION=falseis tested; the unset-variable default should also be covered to lock in the guard's default behaviour.
Positive Highlights
- ✅ Root cause fix is correct and minimal — the early-exit guard directly prevents the exit-127 failure.
- ✅ The refactoring of the downstream missing-result block is clean: removing the now-redundant
RUN_DETECTION=truecheck simplifies the logic. - ✅ Test exercises the right scenario (no result file, no
threat-detectin PATH) and checks bothGITHUB_OUTPUTandGITHUB_ENV.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 20.1 AIC · ⌖ 4.5 AIC · ⊞ 6.6K
Comment /matt to run again
🧪 Test Quality Sentinel Report❌ Test Quality Score: 90/100 — Excellent (but FAILED due to violation)
📊 Metrics (4 tests)
🚨 Coding ViolationMissing mandatory build tag File These are integration tests (bash script execution with fixtures), so add: All Go test files in gh-aw must declare a build tag ( Test Quality Analysis✅ All 4 tests are design/behavioral tests with strong edge-case coverage:
Each test uses explicit Verdict❌ FAILED. Implementation tests: 0% (threshold: 30%), but mandatory
|
There was a problem hiding this comment.
❌ Test Quality Sentinel: 90/100. Coding guideline violation: missing mandatory //go:build integration tag on line 1 of pkg/workflow/threat_detection_conclude_script_test.go. While test quality is excellent (100% design tests, 100% edge-case coverage), all Go test files must declare a build tag. Review flagged violation in the comment above.
There was a problem hiding this comment.
COMMENT — fix is correct, two test gaps need attention
The shell fix itself is sound: the early-exit guard correctly short-circuits before is invoked and the refactored downstream block is logically equivalent to the original. The existing review thread (line 15) already flags the missing success/reason/GH_AW_DETECTION_REASON outputs on the skipped path — that is the primary design question.
Findings
- (line 72, medium) — Test does not assert the state of
success/reason/GH_AW_DETECTION_REASONoutputs on the skipped path. The missing-outputs defect will never be caught by the new test. - (line 44, medium) — No test covers the default/unset
RUN_DETECTIONcase (${RUN_DETECTION:-false}default), which is the most common real-world caller scenario.
🔎 Code quality review by PR Code Quality Reviewer · 33.5 AIC · ⌖ 4.6 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/threat_detection_conclude_script_test.go:72
Test does not assert the state of success, reason, or GH_AW_DETECTION_REASON on the skipped path, so the missing-outputs defect will never be caught by this test.
<details>
<summary>💡 Suggested additions</summary>
After the existing conclusion=skipped assertions, add checks to codify the intended contract:
// If skipped path intentionally omits these, assert they are absent:
if strings.Contains(string(outputData), "success=") {
t.Fatalf("unexpected success= in GITHUB_O…
</details>
<details><summary>pkg/workflow/threat_detection_conclude_script_test.go:44</summary>
**No test covers the default/unset `RUN_DETECTION` case**, which should also skip per `${RUN_DETECTION:-false}`.
<details>
<summary>💡 Suggested addition</summary>
Add a second sub-test or table-driven case that omits `RUN_DETECTION` from the environment entirely and asserts the same `conclusion=skipped` behavior. The `:-false` default is the most likely real-world value (the env var simply isn't set in many callers), and it is completely untested.
</details>
</details>
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
…rve output contract Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. The skipped path now emits |
…aviour Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Added The outstanding
Next maintainer action: Re-trigger CI (close/reopen the PR or push a commit), then merge once green. |
|
@copilot please run the Unresolved review feedback:
Run: https://github.com/github/gh-aw/actions/runs/29861145850 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
No new unresolved feedback to address. The only non-outdated unresolved thread ( The CI run linked (29861145850) is the PR Sous Chef scheduler on
Next maintainer action: Re-trigger CI (close/reopen the PR), then merge once green. |
|
@copilot please run the Unresolved review threads (newest first):
Run: {run_url} Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Next maintainer action: Re-trigger CI by closing and reopening the PR (or pushing a commit). Once CI is green the Matt Pocock Skills bot will re-review and the CHANGES_REQUESTED state will resolve, unblocking the merge. |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29873842709 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Branch is current at
Exact maintainer action: Close and reopen the PR (or push any commit) to trigger CI on HEAD |
|
@copilot please run the Unresolved review threads (newest first):
Run: https://github.com/github/gh-aw/actions/runs/29877220652 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
The GitHub API shows 4 review threads total (
No code changes needed. Branch is current at Smallest next maintainer action: Close and reopen the PR to trigger CI on HEAD |
|
@copilot please run the Unresolved review threads to address first:
Run: https://github.com/github/gh-aw/actions/runs/29881378283 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
All three threads flagged by PR Sous Chef are already resolved in the GitHub API:
No code changes needed. Current state on HEAD
Smallest next maintainer action: Re-trigger CI by closing and reopening the PR (or pushing any commit), then merge once green. |
|
@copilot please run the Unresolved review signals:
Run: https://github.com/github/gh-aw/actions/runs/29883079375 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Both blocking reviews are stale — they were submitted by the Review Review No code changes are needed. The reviews will be replaced by fresh bot reviews once CI runs on the current HEAD.
Smallest maintainer action: Close and reopen the PR to trigger CI on HEAD |
When
RUN_DETECTION=false(detection skipped),conclude_threat_detection.shfell through to the unconditionalthreat-detect concludecall on line 41. Sincethreat-detectis not installed on the skipped-detection path, this exited 127, marking otherwise-successful runs as failed.Changes
actions/setup/sh/conclude_threat_detection.sh— add early-exit guard: whenRUN_DETECTION != true, emitconclusion=skippedtoGITHUB_OUTPUT/GITHUB_ENVand exit 0 before reaching thethreat-detectinvocation. Refactored the downstream missing-result block to drop its now-redundantRUN_DETECTION=truecheck.pkg/workflow/threat_detection_conclude_script_test.go— addTestConcludeThreatDetectionScript_SkippedWhenRunDetectionFalse: runs the script withRUN_DETECTION=false, no result file, andthreat-detectabsent from PATH; asserts exit 0 andconclusion=skippedoutputs.threat-detectwhen detection is skipped → exit 127 false-failur [Content truncated due to length] #47016Run: https://github.com/github/gh-aw/actions/runs/29816999996
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Run: https://github.com/github/gh-aw/actions/runs/29849545379
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/29858066392
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Run: https://github.com/github/gh-aw/actions/runs/29861145850
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Run: https://github.com/github/gh-aw/actions/runs/29883079375
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.