Enable threat detection on token audit/report workflows#42520
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. PR #42520 only modifies workflow files (.github/workflows/agentic-token-trend-audit.md, .github/workflows/agentic-token-trend-audit.lock.yml, .github/workflows/daily-token-consumption-report.md, .github/workflows/daily-token-consumption-report.lock.yml). Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42520 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Enables gh-aw-detection for two token aggregation workflows so their cross-run synthesized outputs are threat-scanned before any publication steps, bringing them in line with other detection-enabled workflows in this repo.
Changes:
- Added
features.gh-aw-detection: trueto the workflow frontmatter for the daily token consumption report and the agentic token trend audit. - Recompiled the corresponding
.lock.ymlartifacts, resulting in the expected detection wiring (feature flag propagation viaGH_AW_INFO_FEATURESand thethreat-detect-based detection flow).
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/daily-token-consumption-report.md | Enables gh-aw-detection in workflow frontmatter for the daily token aggregation report. |
| .github/workflows/daily-token-consumption-report.lock.yml | Recompiled workflow now propagates features and includes the standard threat-detect detection steps/artifacts. |
| .github/workflows/agentic-token-trend-audit.md | Enables gh-aw-detection in workflow frontmatter for the on-demand token trend audit. |
| .github/workflows/agentic-token-trend-audit.lock.yml | Recompiled workflow now propagates features and includes the standard threat-detect detection steps/artifacts. |
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: 0
- Review effort level: Low
There was a problem hiding this comment.
Review: Enable threat detection on token audit/report workflows
Good change overall. Enabling gh-aw-detection on these high-aggregation report paths closes a real detection gap, and the migration from the harness-based agentic execution to the dedicated threat-detect binary is a clean simplification (−115/+55 lines).
What looks good:
GH_AW_INFO_FEATURESenv var correctly reflects the new feature flag in both compiled lock filesif: always() && steps.detection_guard.outputs.run_detection == 'true'guards are correctly applied to bothInstall threat-detect binaryandExecute threat detection with AWF- Writable
--mount /tmp/gh-aw/threat-detection:/tmp/gh-aw/threat-detection:rwcorrectly added so the binary can writedetection_result.json Upload threat detection artifactnow also capturesdetection_result.json(not just the log), which improves post-run debuggability- Conclusion step correctly migrated from a JS github-script to a shell script (
conclude_threat_detection.sh), consistent with the binary-based approach - Upstream-managed workflows (
agentic-token-audit.md,agentic-token-optimizer.md) correctly left untouched per the source-managed workflow policy
One finding (non-blocking):
Two inline comments flag the missing timeout-minutes: 20 on the new execution step in both lock files. The old CLI execution steps had this explicit timeout; without it, a hung threat-detect process runs until the job-level cap.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 49.6 AIC · ⌖ 6.84 AIC · ⊞ 4.8K
| # Copilot CLI tool arguments (sorted): | ||
| timeout-minutes: 20 | ||
| if: always() && steps.detection_guard.outputs.run_detection == 'true' | ||
| continue-on-error: true |
There was a problem hiding this comment.
Missing timeout-minutes on detection execution step.
The previous Execute GitHub Copilot CLI step had timeout-minutes: 20. The new Execute threat detection with AWF step omits this, so if threat-detect hangs the job runs until the job-level 45-minute cap (line 1619) is hit.
Consider restoring:
timeout-minutes: 20@copilot please address this.
| # - Write(/tmp/*) | ||
| timeout-minutes: 20 | ||
| if: always() && steps.detection_guard.outputs.run_detection == 'true' | ||
| continue-on-error: true |
There was a problem hiding this comment.
Missing timeout-minutes on detection execution step.
The previous Execute Claude Code CLI step had timeout-minutes: 20. The new Execute threat detection with AWF step omits this, so if threat-detect hangs the job will run until the 45-minute job cap is hit.
Consider restoring:
timeout-minutes: 20@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — two reliability/security concerns must be addressed before merge
The source changes (features: gh-aw-detection: true in both .md files) are correct. The compiled lock files carry the right structural shift to the new threat-detect binary. However the compilation introduces a step-timeout regression on both detection jobs and raises two questions about the generated output.
🔴 Blocking issues
1. Missing timeout-minutes on both "Execute threat detection with AWF" steps (HIGH)
The old detection steps explicitly set timeout-minutes: 20. The new compiled steps have no step-level timeout and neither detection job has a job-level timeout. A hung threat-detect process will consume a runner for up to 6 hours. This is a reliability regression in both lock files.
2. Unexplained raw.githubusercontent.com in the agentic-token-trend-audit detection sandbox (MEDIUM)
This domain was not in the prior allowlist for this workflow. Please document why it is needed; if it is not, remove it to keep the sandbox network footprint minimal.
🟡 Non-blocking — needs confirmation
detection_result.json artifact exposure (MEDIUM)
The new artifact upload includes detection_result.json alongside detection.log. If this file contains excerpts from analyzed workflow outputs (as evidence/citations), those excerpts will persist 90 days as a downloadable artifact. Confirm the file contains only scores/labels, or add a retention cap.
🔎 Code quality review by PR Code Quality Reviewer · 117.6 AIC · ⌖ 7.36 AIC · ⊞ 1.6K
Comment /review to run again
| # Copilot CLI tool arguments (sorted): | ||
| timeout-minutes: 20 | ||
| if: always() && steps.detection_guard.outputs.run_detection == 'true' | ||
| continue-on-error: true |
There was a problem hiding this comment.
Missing step-level timeout: the old Execute GitHub Copilot CLI detection step had timeout-minutes: 20; the new step drops it entirely, leaving only the 6-hour GitHub default job ceiling.
💡 Impact and suggested fix
If threat-detect or the AWF subprocess hangs (e.g., waiting on an LLM API call that stalls), this job will consume a runner for up to 6 hours before GitHub kills it, blocking the detection job queue and burning credits. continue-on-error: true does not help with a hung step.
The detection job also has no timeout-minutes at the job level, so there is no outer guard.
Suggested fix — add the timeout back to the generated step:
- name: Execute threat detection with AWF
id: detection_agentic_execution
if: always() && steps.detection_guard.outputs.run_detection == 'true'
continue-on-error: true
timeout-minutes: 20Or set a timeout-minutes on the detection job itself if the compiler cannot emit per-step timeouts for shell steps.
| # - Write(/tmp/*) | ||
| timeout-minutes: 20 | ||
| if: always() && steps.detection_guard.outputs.run_detection == 'true' | ||
| continue-on-error: true |
There was a problem hiding this comment.
Missing step-level timeout: same timeout regression as in agentic-token-trend-audit.lock.yml — the old Execute Claude Code CLI step had timeout-minutes: 20; the new step has none.
💡 Impact
A hung threat-detect process will keep the runner occupied until the 6-hour job default. The detection job has no job-level timeout-minutes either, so there is no outer guard.
Add timeout-minutes: 20 (or appropriate value) to this step, or set it at the job level.
| GH_AW_MAX_AI_CREDITS="${GH_AW_MAX_AI_CREDITS:-400}" | ||
| printf '%s\n' "{\"\$schema\":\"https://github.com/github/gh-aw-firewall/releases/download/v0.27.15/awf-config.schema.json\",\"network\":{\"allowDomains\":[\"api.business.githubcopilot.com\",\"api.enterprise.githubcopilot.com\",\"api.github.com\",\"api.githubcopilot.com\",\"api.individual.githubcopilot.com\",\"github.com\",\"host.docker.internal\",\"registry.npmjs.org\",\"telemetry.enterprise.githubcopilot.com\"]},\"apiProxy\":{\"enabled\":true,\"enableTokenSteering\":true,\"maxRuns\":500,\"maxAiCredits\":${GH_AW_MAX_AI_CREDITS},\"maxCacheMisses\":5},\"container\":{\"imageTag\":\"0.27.15\"}}" > "${RUNNER_TEMP}/gh-aw/awf-config.json" | ||
| GH_AW_MAX_AI_CREDITS="${{ vars.GH_AW_DEFAULT_DETECTION_MAX_AI_CREDITS || '400' }}" | ||
| printf '%s\n' "{\"\$schema\":\"https://github.com/github/gh-aw-firewall/releases/download/v0.27.15/awf-config.schema.json\",\"network\":{\"allowDomains\":[\"api.business.githubcopilot.com\",\"api.enterprise.githubcopilot.com\",\"api.github.com\",\"api.githubcopilot.com\",\"api.individual.githubcopilot.com\",\"github.com\",\"host.docker.internal\",\"raw.githubusercontent.com\",\"registry.npmjs.org\",\"telemetry.enterprise.githubcopilot.com\"]},\"apiProxy\":{\"enabled\":true,\"enableTokenSteering\":true,\"maxRuns\":500,\"maxAiCredits\":${GH_AW_MAX_AI_CREDITS},\"maxCacheMisses\":5},\"container\":{\"imageTag\":\"0.27.15\"}}" > "${RUNNER_TEMP}/gh-aw/awf-config.json" |
There was a problem hiding this comment.
Unexplained raw.githubusercontent.com added to detection sandbox allowlist: this domain was not in the previous detection allowlist for this workflow and is not mentioned in the PR description.
💡 Why this matters
The detection sandbox for agentic-token-trend-audit previously had a narrow allowlist (Copilot API, GitHub API, npm). Adding raw.githubusercontent.com allows the sandboxed threat-detect process to fetch arbitrary public GitHub content — including content derived from attacker-controlled commits.
If the threat-detection prompt or analyzed data can influence the binary to fetch URLs, this domain widens the potential exfiltration surface.
Please confirm:
- Why does
threat-detect --engine copilotneedraw.githubusercontent.com? - Is this required for the binary's own install or operation, or is it an artifact of expanding the main agent's allowlist into the detection step?
| if: always() && steps.detection_guard.outputs.run_detection == 'true' | ||
| uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 | ||
| with: | ||
| name: detection |
There was a problem hiding this comment.
detection_result.json now persisted as artifact without confirmed redaction: the old artifact upload included only detection.log; the new upload adds detection_result.json from the threat-detect binary.
💡 Why this matters
Both agentic-token-trend-audit and daily-token-consumption-report ingest cross-run workflow outputs that can contain API responses, token consumption summaries, and other operational data. If threat-detect embeds excerpts of analyzed content in detection_result.json (as evidence or citations — common in threat detection tools), that data will persist as a downloadable workflow artifact.
Artifacts are retained by default for 90 days and are readable by anyone with repo access.
Please confirm:
- Does
detection_result.jsoncontain excerpts of analyzed workflow outputs, or only scores/labels? - If it contains content excerpts, is there a redaction step before upload, or should the artifact retention be reduced?
Review Summary 🧠Requesting changes on one blocking issue; two observations are non-blocking. 🚨 Blocking — timeout regression in compiled lock filesBoth Fix path: Update the compiler template for detection-enabled workflows to carry 📋 Non-blocking observations
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture, /codebase-design, and /grill-with-docs — requesting changes on one concrete regression, with two lower-priority observations.
📋 Key Themes & Highlights
Blocking
timeout-minutesregression (both lock files): The old Copilot/Claude detection execution steps both hadtimeout-minutes: 20. The newExecute threat detection with AWFsteps drop this guard. AWF config caps AI credits and runs, but not wall-clock time — a stuckthreat-detectprocess can hold a runner for up to 6 hours. This needs to be addressed in the compiler template for detection-enabled workflows.
Non-blocking observations
-
conclude_threat_detection.shearly-exit gap: WhenRUN_DETECTION=false, the script skips the missing-file guard but still callsthreat-detect conclude --result-file <path>. If the result file doesn't exist (detection never ran), this call fails. Since the step hascontinue-on-error: true, it silently errors rather than no-oping. Consider adding an earlyexit 0whenRUN_DETECTION != trueat the top of the script. -
Upstream-managed workflow gap: The PR body notes that
agentic-token-audit.mdandagentic-token-optimizer.mdneed detection enabled viagh aw updateonce upstream resolves. This gap has no linked tracking issue and could be forgotten. Consider creating a follow-up issue before merging.
Positive Highlights
- ✅ Clean, well-described PR: the what/why/follow-up structure is clear
- ✅
install_threat_detect_binary.shhas solid SHA256 checksum verification with retry logic - ✅ Replacing the inline JS parsing with
conclude_threat_detection.shsimplifies the step significantly - ✅ Moving detection results to structured
detection_result.json+ log artifact is a good debuggability improvement - ✅ The
--mount /tmp/gh-aw/threat-detection:/tmp/gh-aw/threat-detection:rwmount is correctly added so the container can write the result file
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 103.5 AIC · ⌖ 7.79 AIC · ⊞ 6.5K
Comment /matt to run again
| continue-on-error: true | ||
| run: | | ||
| bash "${RUNNER_TEMP}/gh-aw/actions/install_threat_detect_binary.sh" v0.2.2 | ||
| - name: Execute threat detection with AWF |
There was a problem hiding this comment.
[/improve-codebase-architecture] The old "Execute GitHub Copilot CLI" step carried timeout-minutes: 20 as a wall-clock safety guard; the replacement "Execute threat detection with AWF" step drops it entirely.
💡 Impact and suggestion
Without a step-level timeout, a hung threat-detect process can pin the runner for up to the 6-hour default job timeout. maxRuns and maxAiCredits in the AWF config guard API credits, not wall-clock time.
Since this is a compiled lock file, the fix belongs in the compiler template for detection-enabled workflows. A value of timeout-minutes: 20 (matching the previous guard) or longer if the new binary needs more time would restore the regression.
Alternatively, confirm that threat-detect carries its own internal timeout sufficient to bound runner occupancy — and document that in the PR or a follow-up comment.
@copilot please address this.
| continue-on-error: true | ||
| run: | | ||
| bash "${RUNNER_TEMP}/gh-aw/actions/install_threat_detect_binary.sh" v0.2.2 | ||
| - name: Execute threat detection with AWF |
There was a problem hiding this comment.
[/improve-codebase-architecture] Same regression as in agentic-token-trend-audit.lock.yml: the new "Execute threat detection with AWF" step is missing the timeout-minutes: 20 guard that the old "Execute Claude Code CLI" step had.
💡 Details
Both token-aggregation workflows now share this gap. The daily-token-consumption-report workflow ingests cross-run synthesized data, so a detection hang here would block the publishing path for up to 6 hours.
The fix should land in the shared compiler template for detection-enabled workflows (not manually in the lock file), and applied to both lock files on next make recompile.
@copilot please address this.
| if: always() && steps.detection_guard.outputs.run_detection == 'true' | ||
| uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 | ||
| with: | ||
| name: detection |
There was a problem hiding this comment.
[/codebase-design] Both agentic-token-trend-audit and daily-token-consumption-report upload their detection artifact under the same name detection, making it harder to identify which workflow produced a given artifact when debugging across runs.
💡 Suggested improvement
Use a workflow-scoped name such as detection-token-trend-audit and detection-daily-report, mirroring the tracker-id naming convention used elsewhere in the repo. The if-no-files-found: ignore guard is already correct — this is purely a discoverability concern.
This change also belongs in the compiler template rather than the lock file directly.
@copilot please address this.
| expires: 1d | ||
|
|
||
| - shared/otlp.md | ||
| features: |
There was a problem hiding this comment.
[/grill-with-docs] The features: block is placed inside the YAML frontmatter but after the imports: block, immediately before the closing ---. In agentic-token-trend-audit.md, the same block is placed between tools: and steps:. The compiler likely tolerates both orderings, but a canonical position (e.g., always after tools:, before steps:) would improve consistency across workflow source files and make detection-enabled workflows easier to grep.
💡 Suggestion
Consider picking one canonical position for features: in the frontmatter and applying it consistently. The tools: → features: → steps: ordering used in agentic-token-trend-audit.md is the more natural grouping since both tools: and features: are compile-time declarations.
@copilot please address this.
🤖 PR Triage
Score breakdown: Impact 22/50 · Urgency 12/30 · Quality 3/20 Rationale: Closes threat-detection gap on token aggregation workflows. Currently blocked by
|
|
🎉 This pull request is included in a new release. Release: |
This change closes the detection gap on token aggregation workflows by enabling
gh-aw-detectionin workflow frontmatter where it was missing, so summarized cross-run outputs are scanned before publication.What changed
features.gh-aw-detection: trueto:.github/workflows/daily-token-consumption-report.md.github/workflows/agentic-token-trend-audit.md.github/workflows/daily-token-consumption-report.lock.yml.github/workflows/agentic-token-trend-audit.lock.ymlWhy this matters
Follow-up needed (source-managed workflows)
agentic-token-audit.mdandagentic-token-optimizer.mdare upstream-managed (source:frontmatter) and were intentionally not edited directly in-repo.gh aw update) once upstream resolution succeeds.