threat-detection: fix GITHUB_STEP_SUMMARY unwritable in AWF sandbox (v0.4.0 exit 2) - #50717
Conversation
- Add ThreatDetectionStepSummaryPath constant pointing inside the rw-mounted ThreatDetectionDir - Touch the file before AWF runs in 'Ensure threat-detection directory and log' step - Pass --step-summary flag to threat-detect (flag beats env for this variable since the runner re-injects GITHUB_STEP_SUMMARY after step env: is applied) - Add 'Append detection step summary' host step to cat the file into real $GITHUB_STEP_SUMMARY after execution, no-op when empty - Include step-summary.md in detection artifact upload - conclude runs on the host where GITHUB_STEP_SUMMARY is writable; --step-summary flag is intentionally NOT added there - Add tests for all new invariants including the conclude asymmetry - Recompile 275 lock files Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ 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. |
|
✅ 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. |
There was a problem hiding this comment.
Pull request overview
Fixes #50659 by routing threat-detection summaries through a sandbox-writable file and appending them from the host.
Changes:
- Adds a dedicated detection summary path.
- Passes
--step-summary, uploads the file, and appends it host-side. - Adds coverage and recompiles workflow lock files.
Show a summary per file
| File | Description |
|---|---|
pkg/constants/constants.go |
Defines the summary path. |
pkg/workflow/threat_detection_steps.go |
Prepares and appends the summary. |
pkg/workflow/threat_detection_external.go |
Wires the flag and artifact upload. |
pkg/workflow/threat_detection_isolation_test.go |
Tests sandbox and conclude behavior. |
.github/workflows/*.lock.yml (275 files) |
Applies the recompiled workflow changes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 267/267 changed files
- Comments generated: 2
- Review effort level: Balanced
| " run: |\n", | ||
| " mkdir -p /tmp/gh-aw/threat-detection\n", | ||
| " touch /tmp/gh-aw/threat-detection/detection.log\n", | ||
| fmt.Sprintf(" touch %s\n", constants.ThreatDetectionStepSummaryPath), |
| // The step-summary file must also be included in the artifact upload so it is preserved | ||
| // even if the append step is skipped. | ||
| if !strings.Contains(detectionSection, "/tmp/gh-aw/threat-detection/step-summary.md") { | ||
| t.Error("External detector path must include step-summary.md in artifact upload") | ||
| } |
|
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 (6 tests)
Verdict
|
![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 fix is correct and well-structured. Redirecting threat-detect output via --step-summary to a temp file, then appending to $GITHUB_STEP_SUMMARY outside the sandbox is the right approach. Pattern is consistently applied across all affected lock files: touch step-summary.md before the sandboxed step, --step-summary flag prevents touching unmounted _runner_file_commands/, if [ -s ... ] guard is clean, and including the file in artifact upload aids debugging. No issues found.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 29 AIC · ⊞ 5.4K
|
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. 🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
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 /diagnosing-bugs and /tdd — the fix is correct and well-reasoned; two test-coverage gaps flagged.
📋 Key Themes & Highlights
Key Themes
- Test path constants not used — New test assertions hardcode
/tmp/gh-aw/threat-detection/step-summary.mdrather than usingconstants.ThreatDetectionStepSummaryPath. A future path change would break the compiled output silently while the tests keep passing. if: always()not asserted — The append step carriesif: always()in the implementation, which is load-bearing (summary must appear even when detection fails), but no test guards it.
Positive Highlights
- ✅ Root-cause diagnosis is precise: runner re-injects
GITHUB_STEP_SUMMARYafterenv:, so env-var override is ineffective;--step-summaryflag is the correct fix. - ✅ Asymmetric design (flag on execute, not on conclude) is clearly documented and actively guarded by
TestExternalDetectorConcludeDoesNotReceiveStepSummaryFlag. - ✅ Defense-in-depth: step-summary included in artifact upload so the content survives even if the append step is skipped.
- ✅ Pre-execution
touchpreventscaterrors on an absent file.
| // The real GITHUB_STEP_SUMMARY is not writable inside the AWF sandbox; using a flag | ||
| // rather than an env var is the correct mechanism (the runner re-injects the env var | ||
| // after step-level env: is applied, overriding any override). | ||
| if !strings.Contains(detectionSection, "--step-summary /tmp/gh-aw/threat-detection/step-summary.md") { |
There was a problem hiding this comment.
[/tdd] Test assertions hardcode the path string instead of using constants.ThreatDetectionStepSummaryPath — if the constant value changes, these checks silently diverge from the implementation.
💡 Suggested fix
The existing TestExternalDetectorPath already uses constants.DefaultThreatDetectVersion and constants.ShellMcpServersJsonPath — apply the same pattern here:
if !strings.Contains(detectionSection, "--step-summary "+constants.ThreatDetectionStepSummaryPath) {
t.Errorf("External detector path must pass --step-summary %s", constants.ThreatDetectionStepSummaryPath)
}
if !strings.Contains(detectionSection, "touch "+constants.ThreatDetectionStepSummaryPath) {
t.Errorf("External detector path must touch %s", constants.ThreatDetectionStepSummaryPath)
}This keeps the constant as the single source of truth so a future path rename propagates automatically.
@copilot please address this.
|
|
||
| // A host-side append step must copy the detection step summary to $GITHUB_STEP_SUMMARY | ||
| // after execution. conclude runs on the host where GITHUB_STEP_SUMMARY is writable. | ||
| if !strings.Contains(detectionSection, "Append detection step summary") { |
There was a problem hiding this comment.
[/tdd] The test verifies the step name and cat command but not the if: always() condition — if someone drops that guard, the summary will not appear when the detection step fails (the very scenario this whole fix is meant to cover).
💡 Suggested addition
Add an assertion alongside the existing append-step checks:
// The append step must run even when the detection step fails.
if !strings.Contains(detectionSection, "Append detection step summary") {
t.Error(...)
}
// Check if: always() is present to ensure it runs on failure too.
appendIdx := strings.Index(detectionSection, "Append detection step summary")
if appendIdx != -1 {
appendBlock := detectionSection[appendIdx:]
nextStep := strings.Index(appendBlock[1:], "- name:")
if nextStep != -1 {
appendBlock = appendBlock[:nextStep+1]
}
if !strings.Contains(appendBlock, "if: always()") {
t.Error("Append detection step summary must have 'if: always()' so it runs even when detection fails")
}
}@copilot please address this.
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.
Solid, narrowly-scoped fix for the AWF sandbox GITHUB_STEP_SUMMARY write failure — using --step-summary into the already rw-mounted dir plus a host-side append step is the right mechanism, and the conclude-step asymmetry is correctly guarded by a dedicated test. No blocking correctness/security bugs found; a few non-blocking consistency nits below.
💡 Review themes
- The new "Append detection step summary" step uses
if: always()instead of the shareddetectionStepConditiongate used by every other step in this pipeline — inconsistent, though currently harmless since the file-existence check makes it a no-op when detection was skipped. - One new test assertion (artifact-upload inclusion of the step-summary path) duplicates a substring already guaranteed by an earlier assertion in the same test, so it provides no real additional coverage of the artifact-upload step specifically.
- Minor escaping inconsistency: the new
touchline for the step-summary path doesn't useshellEscapeArg, while sibling lines referencing the same constant do.
None of these block merge; the core fix (constant, --step-summary flag wiring, artifact inclusion, and the conclude-step non-regression test) is correct and well-tested.
| func (c *Compiler) buildDetectionStepSummaryAppendStep() []string { | ||
| return []string{ | ||
| " - name: Append detection step summary\n", | ||
| " if: always()\n", |
There was a problem hiding this comment.
This step uses if: always() instead of the shared detectionStepCondition gate used by every sibling step in this job — an inconsistency that lets it run even when detection was skipped.
💡 Details
Every other step in the external-detector pipeline (setup, touch, install, execution, artifact upload) is gated on detectionStepCondition = "always() && steps.detection_guard.outputs.run_detection == '''true'''". This new append step instead just uses if: always(), so it unconditionally runs even when the guard decided detection should not run at all.
It happens to be harmless today because [ -s /tmp/gh-aw/threat-detection/step-summary.md ] will be false when the file was never touched (the touch step is gated), but it breaks the established pattern and adds a pointless extra step execution/log entry on every skipped run. Use the same detectionStepCondition here for consistency.
fmt.Sprintf(" if: %s
", detectionStepCondition),|
|
||
| // The step-summary file must also be included in the artifact upload so it is preserved | ||
| // even if the append step is skipped. | ||
| if !strings.Contains(detectionSection, "/tmp/gh-aw/threat-detection/step-summary.md") { |
There was a problem hiding this comment.
This new assertion at line 241 is redundant — it re-checks a substring already required by the earlier assertion at line 220, so it can never catch a regression in the artifact-upload step specifically.
💡 Details
The stated intent of this check is "the step-summary file must also be included in the artifact upload" (see comment at lines 239-240), but the assertion just does:
if !strings.Contains(detectionSection, "/tmp/gh-aw/threat-detection/step-summary.md") {That exact substring is already guaranteed present by the assertion at line 220 (--step-summary /tmp/gh-aw/threat-detection/step-summary.md) and lines 226/235. Since strings.Contains checks the whole detectionSection, not the artifact-upload step specifically, this test will still pass even if the path is completely missing from the path: | block of the upload step, as long as it appears anywhere else in the job (which it always will given the other assertions). Scope the check to the upload step — e.g. extract the step by name ("Upload threat detection artifact") the same way the test already isolates the conclude step in TestExternalDetectorConcludeDoesNotReceiveStepSummaryFlag — then assert the path appears within that slice.
| " run: |\n", | ||
| " mkdir -p /tmp/gh-aw/threat-detection\n", | ||
| " touch /tmp/gh-aw/threat-detection/detection.log\n", | ||
| fmt.Sprintf(" touch %s\n", constants.ThreatDetectionStepSummaryPath), |
There was a problem hiding this comment.
Inconsistent escaping: this new touch line interpolates the path constant raw, while the sibling execution and append steps wrap the same constant with shellEscapeArg.
💡 Details
ThreatDetectionStepSummaryPath is a hardcoded Go constant today, so the missing escaping here is not currently exploitable. But threat_detection_external.go wraps this exact same constant with shellEscapeArg in two places (the --step-summary flag and the cat/-s checks), establishing a convention that any reference to this path in a shell command should be escaped. This line breaks that convention:
fmt.Sprintf(" touch %s\n", constants.ThreatDetectionStepSummaryPath),Inconsistent escaping of the "same" value across the codebase is a common source of future injection bugs if someone later parameterizes the path (e.g. per-workflow overrides) and copies this line as a template instead of the escaped ones. Wrap it with shellEscapeArg for consistency:
fmt.Sprintf(" touch %s\n", shellEscapeArg(constants.ThreatDetectionStepSummaryPath)),
|
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 resolve the remaining review feedback, refresh this branch if needed, then run the
Run: https://github.com/github/gh-aw/actions/runs/31062758914
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in b239516. Updates made:
Validation run:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
threat-detectv0.4.0 writes an artifact-inventory table toGITHUB_STEP_SUMMARYon startup; if the write fails it exits 2 before running any detection. Inside the AWF sandbox_runner_file_commands/is intentionally not mounted, so the write always fails — and the runner re-injects the realGITHUB_STEP_SUMMARYenv var after step-levelenv:is applied, making any env-var override ineffective. The correct fix is a--step-summaryflag pointing into the already rw-mountedThreatDetectionDir.Changes
pkg/constants/constants.go— NewThreatDetectionStepSummaryPath = "/tmp/gh-aw/threat-detection/step-summary.md"(inside the existing rw-mounted dir; no new mount needed).threat_detection_steps.go— "Ensure threat-detection directory and log" step now alsotouchesstep-summary.mdbefore AWF runs. NewbuildDetectionStepSummaryAppendStepis wired after the artifact upload in the external-detector path:threat_detection_external.go—threat-detectinvocation gains--step-summary /tmp/gh-aw/threat-detection/step-summary.md;buildUploadDetectionArtifactStepincludes the file so it survives even if the append step is skipped; newbuildDetectionStepSummaryAppendStepfunction.conclude_threat_detection.sh/buildExternalDetectorConcludeStep—concluderuns on the host whereGITHUB_STEP_SUMMARYis writable;--step-summaryis intentionally not added there.threat_detection_isolation_test.go— Assertions added toTestExternalDetectorPathfor--step-summaryflag, pre-executiontouch, append step content, and artifact inclusion. NewTestExternalDetectorConcludeDoesNotReceiveStepSummaryFlagguards the conclude asymmetry against future "cleanup".275 lock files recompiled.
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/31062758914> Generated by 👨🍳 PR Sous Chef · gpt54 · 20.1 AIC · ⊞ 8.3K · ◷
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 · ◷