Uh oh!
There was an error while loading. Please reload this page.
Restore detection step timeout and --rootless AWF install on the external threat-detect path - #54627
Conversation
…etection Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Restores the external threat-detection path’s 20-minute execution limit and rootless AWF installation.
Changes:
- Emits
timeout-minutesalongsideGH_AW_TIMEOUT_MINUTES. - Installs AWF according to the detection job’s rootless profile.
- Adds regression tests, regenerated locks, and a patch changeset.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/threat_detection_external.go | Restores timeout and rootless installation. |
pkg/workflow/threat_detection_test.go | Adds regression coverage; one comment needs correction. |
.changeset/detection-step-timeout-and-rootless-awf-install.md | Adds release note; runtime wording needs correction. |
.github/workflows/*.lock.yml (all changed lock files) | Regenerates detection steps with rootless AWF and a 20-minute timeout. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 239/239 changed files
- Comments generated: 2
- Review effort level: Balanced
| "gh-aw": patch | ||
| --- | ||
| Restore two properties of the threat-detection job on the external `threat-detect` path. The detection execution step is again bounded by a step-level `timeout-minutes` (aligned with the `GH_AW_TIMEOUT_MINUTES` value it already exported), so a stall before the binary reaches its own timeout logic no longer runs up to the 360 minute GitHub default. The detection job's AWF binary install now passes `--rootless` in the same cases as the agent job, matching how `awf` is actually invoked in that job. |
| // TestBuildInstallAWFForExternalDetectorStepUsesRootless verifies that the detection | ||
| // job installs the AWF binary in the same mode used to invoke it, matching the agent job. |
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (63 additions detected).
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. The pre-fetched diff for PR #54627 contains only regenerated .lock.yml workflow files (generated, out of scope per ponytail-review skill) and a .changeset markdown doc (not code). The actual Go source changes are not present in the capped diff, leaving no code surface to review for over-engineering.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
COMMENT — I don't see a blocking regression in the changed lines.
The only problems I found are wording drift that was already called out in prior review comments.
- The implementation change in
pkg/workflow/threat_detection_external.golooks internally consistent: the external detector now derives bothtimeout-minutesand the AWF install mode from the detection job's own synthetic workflow data. generateAWFInstallationStepalready suppresses--rootlessfor privileged profiles such asdocker-sudo-iptablesandcloud-hypervisor, so the new call site does not force rootless universally.- The lockfile churn matches the intended generated output.
- Remaining issues are non-blocking wording inaccuracies in the new changeset/test comments, and those are already covered by existing Copilot review comments on this PR.
I skipped sub-agent input because grumpy-coder is not available in this environment.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 12.2 AIC · ⌖ 8.23 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The changes are correct and consistent across all affected .lock.yml files. Both fixes (--rootless for AWF install and timeout-minutes: 20 on the detection step) are uniformly applied with no omissions or inconsistencies visible in the diff. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20.1 AIC · ⌖ 8.94 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — two minor issues in the new tests; overall the fix is correct and well-motivated.
📋 Key Themes & Highlights
Issues Found
Hardcoded timeout value in test (
threat_detection_test.go:3629) — the literal"20"should be derived fromconstants.DefaultAgenticWorkflowTimeout(or the env var neutralised witht.Setenv) so an enterpriseGH_AW_DEFAULT_TIMEOUT_MINUTESoverride doesn't silently break the assertion.Misleading doc comment (
threat_detection_test.go:3638) — "matching the agent job" re-introduces the same inaccuracy already flagged in the changeset: the detection job always uses its own independent default Docker/rootless profile, never inheritingsandbox.agent.runtimefrom the main workflow.
Positive Highlights
- ✅ Root cause properly addressed:
nil→getAgentConfig(detectionData)ensures install mode aligns with invocation mode. - ✅
timeout-minutesis sourced fromresolveStepTimeoutValue(threatDetectionData), keeping it aligned withGH_AW_TIMEOUT_MINUTESautomatically. - ✅ Both fixes have targeted unit tests and a clear changeset entry.
- ✅ The comment in
buildInstallAWFForExternalDetectorStepexplains exactly whynilwas wrong, which is excellent.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 65.3 AIC · ⌖ 9.95 AIC · ⊞ 7.6K
Comment /matt to run again
| } | ||
| joined := strings.Join(steps, "") | ||
| if !strings.Contains(joined, " timeout-minutes: 20\n") { |
There was a problem hiding this comment.
[/diagnosing-bugs] The assertion hardcodes "20" instead of deriving the value through the same resolution path as the production code. If GH_AW_DEFAULT_TIMEOUT_MINUTES is set in the test environment, resolveStepTimeoutValue returns a different value and this assertion silently fails — masking a real regression.
💡 Suggested fix
Isolate the env var or derive the expected value programmatically:
// Option 1 – neutralise the env overridet.Setenv(compilerenv.DefaultTimeoutMinutes, "")
// Option 2 – derive from the same constantfallback:=int(constants.DefaultAgenticWorkflowTimeout/time.Minute)
expected:=fmt.Sprintf(" timeout-minutes: %d\n", fallback)
if!strings.Contains(joined, expected) { ... }@copilot please address this.
There was a problem hiding this comment.
Addressed in bbb721c: the test now derives expectedTimeout via resolveStepTimeoutValue(data) and checks both timeout-minutes and GH_AW_TIMEOUT_MINUTES against that resolved value.
| } | ||
| // TestBuildInstallAWFForExternalDetectorStepUsesRootless verifies that the detection | ||
| // job installs the AWF binary in the same mode used to invoke it, matching the agent job. |
There was a problem hiding this comment.
[/diagnosing-bugs] The test comment says the rootless install matches "the agent job", but buildThreatDetectionWorkflowData explicitly creates a separate default Docker/rootless profile — it never inherits sandbox.agent.runtime from the main workflow. This wording is the same as the misleading phrase in the changeset and may re-introduce confusion when this test is read in isolation.
Consider: "verifies that the detection job installs AWF in rootless mode, matching how awf is invoked in the same job" — which is accurate and echoes the PR description.
@copilot please address this.
There was a problem hiding this comment.
Addressed in bbb721c: the test comment now states that the install mode matches how awf is invoked in the detection job, rather than comparing it to the main agent job.
gh-aw-bot
commented
Aug 21, 2026
Branch refresh was requested. Please run the Run: https://github.com/github/gh-aw/actions/runs/32534300670
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Ran the |
Moving the threat-detection job to the external
threat-detectimplementation dropped two properties of the detection job: the execution step lost itstimeout-minutes(raising the effective ceiling from 20 to the GitHub default of 360, with no frontmatter lever to restore it), and the AWF install lost--rootlesswhile the agent job kept it.Changes
buildExternalDetectorExecutionStepnow emitstimeout-minutes:on the execution step, resolved from the same detectionWorkflowDatathat producesGH_AW_TIMEOUT_MINUTES, so the two stay aligned. The env var is only honoured once the binary is running; a stall before that (image pull, AWF startup, headless server never becoming ready) was previously unbounded.buildInstallAWFForExternalDetectorSteppassednilas the agent sandbox config, sogenerateAWFInstallationStepskipped--rootlesseven though the detection job invokes the rootlessawfcommand. It now passes the detection job's own agent config, making install mode match invocation..lock.ymlfiles and a patch changeset.Resulting detection-job diff:
Note the detection job does not inherit
sandbox.agent.runtimefrom the main workflow — it always resolves to the default docker (rootless) profile — so the install flag now tracks how AWF is actually invoked in that job rather than the agent job's runtime.