Propagate runner topology into detection AWF image pre-pull

[Copilot]

Threat detection on arc-dind was still effectively no-oping after the previous fix: the detection execution step ran with arc-dind topology, but the separate AWF image pre-pull step built a different minimal WorkflowData without RunnerConfig. That caused collectDockerImages to omit build-tools, so AWF sysroot startup failed with No such image while the detection job still reported green.

• Pull-step topology propagation

  • buildPullAWFContainersStep now uses detection workflow data that carries RunnerConfig, so arc-dind detection jobs pre-pull the build-tools image alongside the existing firewall images.

• Keep synthetic detection contexts in sync

  • Extracted a shared buildThreatDetectionWorkflowData(...) helper for minimal detection-job WorkflowData.
  • Reused it across detection builders so topology- and feature-dependent behavior does not drift between pull, inline, and external detector paths.

• Comment cleanup

  • Updated the stale pull-step comment to reflect that arc-dind detection may require a fourth AWF image (build-tools).

• Regression coverage

  • Added focused tests asserting:
    • arc-dind detection pull steps include ghcr.io/github/gh-aw-firewall/build-tools:...
    • non-arc-dind detection pull steps do not

detectionData := buildThreatDetectionWorkflowData(data, engineSetting)
detectionData.Tools = map[string]any{}

images := collectDockerImages(detectionData.Tools, detectionData, c.actionMode)

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #45274 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (96 additions across 4 files, threshold is 100).

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[Copilot]

Pull request overview

Propagates runner topology into synthetic threat-detection contexts so arc-dind jobs pre-pull required AWF images.

Changes:

• Centralizes detection WorkflowData construction.
• Adds arc-dind build-tools pre-pull regression tests.
• Updates one generated workflow’s AWF runtime metadata.

Show a summary per file

File	Description
pkg/workflow/threat_detection_external.go	Adds shared detection context construction.
pkg/workflow/threat_detection_inline_engine.go	Uses the shared context helper.
pkg/workflow/threat_detection_test.go	Tests topology-sensitive image pre-pulling.
.github/workflows/sighthound-security-scan.lock.yml	Updates generated AWF version and digests.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

The fix is correct and well-structured. buildThreatDetectionWorkflowData now consistently propagates RunnerConfig across all detection paths (pull, inline, external), and the new helper eliminates the drift that caused the arc-dind build-tools image to be omitted from the pre-pull step. Tests are focused and cover both the positive and negative cases.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 18.6 AIC · ⌖ 4.92 AIC · ⊞ 4.8K

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 100/100 — Excellent

Analyzed 2 test(s): 2 design, 0 implementation, 0 violation(s).

📊 Metrics (2 tests)

Metric	Value
Analyzed	2 subtests (Go: 2, JS: 0)
✅ Design	2 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	2 (100%)
Duplicate clusters	0
Inflation	No (40 test lines / ~132 prod lines ≈ 0.3:1)
🚨 Violations	0

Test	File	Classification	Issues
TestBuildPullAWFContainersStepPropagatesRunnerTopology / arc-dind includes build-tools image	pkg/workflow/threat_detection_test.go:1964	design_test / high_value	None
TestBuildPullAWFContainersStepPropagatesRunnerTopology / non-arc-dind excludes build-tools image	pkg/workflow/threat_detection_test.go:1964	design_test / high_value	None

Verdict

✅ Passed. 0% implementation tests (threshold: 30%). No guideline violations.

The single new test function TestBuildPullAWFContainersStepPropagatesRunnerTopology covers both branches of the runner topology propagation fix with descriptive failure messages and clear behavioral contracts. The //go:build !integration tag is present. No forbidden mocks, no inflation.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🧪 Test quality analysis by Test Quality Sentinel · 40.3 AIC · ⌖ 8.76 AIC · ⊞ 6.8K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 100/100. 0% implementation tests (threshold: 30%).

[github-actions]

Non-blocking observations on the refactor; the core topology-propagation fix looks correct.

### Findings summary

Medium — Mounts assignment clobbers instead of appends (threat_detection_external.go line 141): assigning Mounts directly is fragile; use append to avoid silently discarding mounts added by the helper in future.

Medium — Duplicated engine-default logic (threat_detection_external.go line 73): buildPullAWFContainersStep still applies its own "claude" fallback before calling the helper, which has its own identical fallback — two divergence points for a one-line rule.

Medium — Silent permissions propagation into pull step (threat_detection_external.go line 83): the shared helper copies Permissions/CachedPermissions into the image-pull WorkflowData where the old code did not, with no test coverage confirming this has no effect on collectDockerImages.

None of these block the stated fix, but the Mounts assignment is a trap for the next refactor.

🔎 Code quality review by PR Code Quality Reviewer · 50.2 AIC · ⌖ 4.7 AIC · ⊞ 5.4K
_{Comment /review to run again}

Comments that could not be inline-anchored

pkg/workflow/threat_detection_external.go:141

Mount assignment clobbers rather than merges: directly assigning Mounts will silently discard any mounts that buildThreatDetectionWorkflowData initialises in future refactors.

<details>
<summary>💡 Suggested fix</summary>

The helper currently sets no mounts, so this works today — but it's one helper-update away from a silent regression. Append instead:

threatDetectionData.SandboxConfig.Agent.Mounts = append(
    threatDetectionData.SandboxConfig.Agent.Mounts,
    constants.Thre…

</details>

<details><summary>pkg/workflow/threat_detection_external.go:73</summary>

**Duplicated engine-default logic will silently diverge**: `buildPullAWFContainersStep` computes an `engineSetting` with its own `&quot;claude&quot;` fallback and then passes the result into `buildThreatDetectionWorkflowData`, which applies the same fallback again — creating two independent places to update.

&lt;details&gt;
&lt;summary&gt;💡 Suggested fix&lt;/summary&gt;

Remove the fallback from `buildPullAWFContainersStep` and pass `&quot;&quot;` so the helper owns all default logic:

```go
engineSetting := &quot;&quot;
if data.SafeOutput…

</details>

<details><summary>pkg/workflow/threat_detection_external.go:83</summary>

**Silent behavior change: permissions now propagated into the image-pull step**: the old `buildPullAWFContainersStep` only copied `ActionCache` and `Features`. The shared helper also copies `Permissions` and `CachedPermissions` — a change not mentioned in the PR description and not covered by any new test.

&lt;details&gt;
&lt;summary&gt;💡 Why this matters&lt;/summary&gt;

If `collectDockerImages` or any downstream consumer of the constructed `WorkflowData` branches on `Permissions`/`CachedPermissions`, pull st…

</details>

[github-actions]

Skills-Based Review 🧠

Applied /tdd and /codebase-design — commenting with two targeted observations; no blocking issues.

📋 Key Themes & Highlights

Key Themes

• Test coverage gap: buildExternalDetectorExecutionStep was refactored to use the new factory but has no arc-dind topology regression test. The pre-pull and inline-engine paths both have topology tests; the external-detector path doesn't.
• Implicit invariant: The SandboxConfig.Agent.Mounts mutation at line 279 relies on the factory always returning a non-nil Agent. This is true today but undocumented — worth a comment or a guard.

Positive Highlights

• ✅ Clean extraction of buildThreatDetectionWorkflowData — eliminates three near-identical struct literals and makes topology propagation impossible to miss on future changes
• ✅ Regression tests for the pre-pull step follow the arc-dind / non-arc-dind pair pattern already established for the inline engine test
• ✅ PR description accurately describes root cause and fix scope

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 72.5 AIC · ⌖ 5.14 AIC · ⊞ 6.6K
_{Comment /matt to run again}

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Ran the pr-finisher pass in f59b121: local make fmt, make lint, make test-unit, make test, make recompile, and make agent-report-progress all passed. CI on this pushed HEAD is still unverified until a maintainer re-triggers it.

[gh-aw-bot]

@copilot please run the pr-finisher skill, address any unresolved review feedback, and rerun checks once the branch is up to date.

Generated by 👨‍🍳 PR Sous Chef · 9.23 AIC · ⌖ 10.8 AIC · ⊞ 7.5K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot please run the pr-finisher skill, address any unresolved review feedback, and rerun checks once the branch is up to date.

...

Addressed on HEAD 60526df: merged the branch up to main, covered the remaining trusted-automation mount-append feedback with regression tests, and reran make fmt, make lint, make test-unit, make test, make recompile, make agent-report-progress, plus final validation. CI on this head is still unverified until a maintainer re-triggers it.

[github-actions]

PR Triage (run 29275650494)\n\nCategory: bug | Risk: high | Score: 75/100 | Action: fast_track\n\nScore: Impact 38/50 + Urgency 25/30 + Quality 12/20\n\nFixes arc-dind threat detection breakage - AWF image pre-pull omitted build-tools, causing silent No Such Image failures on detection jobs. 5 files, +203/-83, regression tests included.

Generated by 🔧 PR Triage Agent · 181.5 AIC · ⌖ 7.71 AIC · ⊞ 5.6K · ◷

[gh-aw-bot]

pr-sous-chef
@copilot please run the pr-finisher skill, review the remaining non-blocking feedback, confirm whether the external-detector topology path also needs regression coverage, and rerun checks once the branch is up to date.

Generated by 👨🍳 PR Sous Chef

Generated by 👨‍🍳 PR Sous Chef · 14.5 AIC · ⌖ 5.74 AIC · ⊞ 7.5K · ◷
_{Comment /souschef to run again}

[Copilot]

pr-sous-chef
@copilot please run the pr-finisher skill, review the remaining non-blocking feedback, confirm whether the external-detector topology path also needs regression coverage, and rerun ch...

No further code changes were needed on 60526df.

The external-detector topology path already has dedicated regression coverage in pkg/workflow/threat_detection_test.go via TestBuildExternalDetectorExecutionStepPropagatesRunnerTopology (added in f59b121), alongside the pull-step and mount-append coverage already on this branch.

I reran the local finisher checks on the current head and they all passed: make fmt, make lint, make test-unit, make test, make recompile, and make agent-report-progress.

The branch is already up to date with main in this clone. CI on this head is still unverified until a maintainer re-triggers it; the latest GitHub workflow entries for 60526df are action_required with zero jobs, so there were no failed job logs to root-cause from this agent pass.