fix(token-usage): restore fleet-wide TokenUsage collection via non-empty copy guard and correct priority order#41823
Conversation
🤖 PR Triage
Score breakdown: Impact 20 · Urgency 8 · Quality 7 Active WIP for restoring fleet-wide
|
Two bugs caused TokenUsage=0 fleet-wide since ~June 20, 2026: 1. Empty-file overwrite in conclusion job (notify_comment.go) The buildUsageArtifactUploadSteps function used sequential '[ -f ]' (file-exists) copies with last-wins semantics. AWF v0.27.7+ creates an empty api-proxy-logs/token-usage.jsonl stub in the --audit-dir (firewall/audit/), which overrode the real data from firewall/logs/. Fix: change to '[ -s ]' (non-empty) and reorder so firewall/logs/ goes LAST (highest priority), with firewall/audit/ as a fallback before it. 2. Missing audit path in parse_token_usage.cjs The agent job's parse_token_usage.cjs only checked firewall-audit-logs/ and firewall/logs/ paths. If AWF writes token data to firewall/audit/, the agent job would skip it and not write agent_usage.json. Fix: add TOKEN_USAGE_AWF_AUDIT_PATH and include it in TOKEN_USAGE_PATHS. Changes: - pkg/workflow/notify_comment.go: -f -> -s for token-usage copies; reorder agent/detection copies so firewall/logs/ wins (goes last) - actions/setup/js/parse_token_usage.cjs: add TOKEN_USAGE_AWF_AUDIT_PATH to TOKEN_USAGE_PATHS so agent job reads from firewall/audit/ too - actions/setup/js/parse_token_usage.test.cjs: update tests for new path - pkg/workflow/notify_comment_test.go: verify -s check and copy priority - make recompile: regenerate all .lock.yml workflow artifacts Closes #41734 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Restores fleet-wide token usage collection in gh-aw workflows by preventing empty AWF audit stubs from overwriting real proxy-log token usage data and by ensuring the agent-side parser also considers the AWF audit directory location.
Changes:
- Update usage-artifact collection to use non-empty file guards (
[ -s ]) and enforcefirewall/logs/as the highest-priority (last) copy source. - Extend token-usage parsing to include the AWF audit-dir token usage path in addition to legacy and proxy-log paths.
- Regenerate workflow lockfiles and adjust tests to assert the new guards and ordering.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/notify_comment.go | Updates usage artifact collection commands to skip empty token-usage files and ensure proxy logs win by priority order. |
| pkg/workflow/notify_comment_test.go | Adds assertions for -s guards and verifies copy priority ordering in generated conclusion-job steps. |
| actions/setup/js/parse_token_usage.cjs | Adds AWF audit-dir token usage path and includes it in the set of candidate token usage paths. |
| actions/setup/js/parse_token_usage.test.cjs | Updates constant/path-list tests to cover the newly added AWF audit path. |
| .github/workflows/*.lock.yml | Regenerated lockfiles to reflect updated token-usage copy guards and priority ordering in compiled workflows. |
| .changeset/patch-restore-token-usage-collection.md | Adds a patch changeset documenting the restoration of token usage collection. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 258/258 changed files
- Comments generated: 1
- Review effort level: Low
| logsIdx := strings.Index(allSteps, "[ -s /tmp/gh-aw/sandbox/firewall/logs/api-proxy-logs/token-usage.jsonl ]") | ||
| auditIdx := strings.Index(allSteps, "[ -s /tmp/gh-aw/sandbox/firewall/audit/api-proxy-logs/token-usage.jsonl ]") | ||
| if logsIdx > 0 && auditIdx > 0 && logsIdx <= auditIdx { | ||
| t.Errorf("Expected firewall/logs token-usage copy to appear AFTER firewall/audit copy (logs = higher priority).\nGenerated steps:\n%s", allSteps) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hey Everything looks solid: the
|
|
✅ 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 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (25 additions detected, threshold is 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Go: 1 ( Verdict
|
PR Review Summary 🧠Applied 📋 Suggestions (5)
@copilot please address the review comments above.
|
![github-actions[bot]](https://avatars.mcszy.xyz/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose, /tdd, and /zoom-out — commenting with improvement suggestions on test robustness and cross-component consistency. No blocking correctness issues found.
📋 Key Themes & Highlights
Key Themes
- Test guard semantics: The ordering assertion in
notify_comment_test.gouses> 0where!= -1is the idiomatic and safer sentinel forstrings.Indexresults (line 1291). - Detection parity gap: The agent sandbox ordering is tested but the symmetric detection sandbox ordering check is missing.
- Bug 2 test coverage:
getReadableTokenUsagePathsandreadDedupedTokenUsageare not exercised with all three paths including the newTOKEN_USAGE_AWF_AUDIT_PATH. - Cross-component priority asymmetry: Shell copy uses last-wins (primary logs go last); JS dedup uses first-wins. These are opposite semantics for the same intent. Benign in practice today (AWF audit stub is always empty), but worth a clarifying comment.
Positive Highlights
- ✅ Root cause correctly identified and addressed at both sites (conclusion job + agent job)
- ✅
[ -f ]→[ -s ]is the right fix — clean, surgical, backward-compatible - ✅ Priority reorder is correct: legacy → awf-audit → proxy-logs (last-wins for shell copy)
- ✅ Inline comments in
notify_comment.goclearly explain the priority ordering rationale - ✅ 253 lock files regenerated via
make recompile— no manual drift - ✅ Changeset entry included for version tracking
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 104.5 AIC · ⌖ 9.64 AIC · ⊞ 6.6K
Comments that could not be inline-anchored
pkg/workflow/notify_comment_test.go:1291
[/diagnose] The guard logsIdx > 0 && auditIdx > 0 uses the wrong sentinel. strings.Index returns -1 for not-found, and -1 > 0 is false, so the 'not found' case happens to be safe here — but the idiomatic and self-documenting check is != -1. If the preceding Contains assertions are ever removed or weakened, this guard would silently skip the ordering check for any missing path.
<details>
<summary>💡 Suggested fix</summary>
if logsIdx != -1 && auditIdx != -1 && logsIdx <…
</details>
<details><summary>pkg/workflow/notify_comment_test.go:1297</summary>
**[/tdd]** The detection sandbox block received the same reorder fix (threat-detection paths also go legacy → audit → logs), but there is no ordering assertion for those paths. The test at line 1289–1293 only verifies the agent sandbox (`/sandbox/firewall/`) ordering. A symmetric check for `threat-detection/sandbox/firewall/logs/` appearing after `threat-detection/sandbox/firewall/audit/` would give the same regression protection for the detection block.
<details>
<summary>💡 Suggested additio…
</details>
<details><summary>actions/setup/js/parse_token_usage.test.cjs:54</summary>
**[/tdd]** This test verifies that the three constants are in the array (good!), but doesn't cover the behaviour when all three paths contain real data. The existing `readDedupedTokenUsage` dedup test at line 515 only exercises two paths (`TOKEN_USAGE_AUDIT_PATH` and `TOKEN_USAGE_PATH`); a three-way variant including `TOKEN_USAGE_AWF_AUDIT_PATH` would confirm the new path is correctly merged and deduplicated.
<details>
<summary>💡 Suggested addition</summary>
```js
test('readDedupedTokenUsage…
</details>
<details><summary>actions/setup/js/parse_token_usage.cjs:23</summary>
**[/zoom-out]** The shell copy in `notify_comment.go` uses **last-wins** semantics (so `firewall/logs/` goes last = highest priority). The JS code here uses **first-wins** dedup semantics in `readDedupedTokenUsage`: when the same `request_id` appears in multiple files, the first file in the array wins. With the current order `[legacy, awf-audit, primary-logs]`, `firewall/audit/` beats `firewall/logs/` for duplicate entries — the opposite of the shell fix's stated intent.
In practice this is be…
</details>
<details><summary>actions/setup/js/parse_token_usage.test.cjs:511</summary>
**[/tdd]** The `getReadableTokenUsagePaths` tests mock only `TOKEN_USAGE_AUDIT_PATH` and `TOKEN_USAGE_PATH`. There is no test for the scenario where _only_ `TOKEN_USAGE_AWF_AUDIT_PATH` has content (the other two are absent or zero-size) — which is the exact new code path Bug 2 was meant to fix. Without this test, a regression that silently drops the new path would go undetected.
<details>
<summary>💡 Suggested test</summary>
```js
test('returns only TOKEN_USAGE_AWF_AUDIT_PATH when it is the s…
</details>
There was a problem hiding this comment.
The core fixes are correct: [ -f ] → [ -s ] eliminates the empty-stub overwrite, the priority reordering makes firewall/logs/ authoritative in the shell artifact step, and the detection paths receive the same treatment. Two non-blocking concerns.
📋 Findings summary
Medium — Detection test coverage gap
pkg/workflow/notify_comment_test.go line 1297: The -s guard and ordering assertions added for the agent paths have no equivalent for the detection (threat-detection/) paths, even though those paths received the same fix. A regression in the detection half would pass the test suite silently. Suggested assertions in the inline comment.
Medium — TOKEN_USAGE_PATHS dedup order inverts shell priority
actions/setup/js/parse_token_usage.cjs line 22: readDedupedTokenUsage() uses first-found wins, so the current [legacy-audit, awf-audit, firewall/logs] order gives firewall/logs (authoritative) the lowest dedup priority — the opposite of the shell script, which explicitly puts firewall/logs last to make it win. Currently harmless because empty stubs are filtered out, but the code comment implies a scenario where both audit and logs paths could be non-empty. Inline comment has a suggested reorder.
Dropped (already flagged)
notify_comment_test.go line 1292: strings.Index guard > 0 should be >= 0 — already covered by an existing review comment.
🔎 Code quality review by PR Code Quality Reviewer · 92.4 AIC · ⌖ 7.57 AIC · ⊞ 5.2K
Comments that could not be inline-anchored
pkg/workflow/notify_comment_test.go:1297
Detection paths are not covered by any -s guard assertion or ordering assertion, unlike the agent paths directly above.
<details>
<summary>💡 Detail</summary>
The PR applies the same [ -f ] → [ -s ] guard and ordering fix to both the agent and detection copy blocks in notify_comment.go. The new test assertions cover the agent paths (lines 1282–1293), but the detection block has no equivalent checks:
- No assertion that
[ -s .../threat-detection/sandbox/firewall/logs/...]is used (v…
actions/setup/js/parse_token_usage.cjs:22
TOKEN_USAGE_PATHS order gives legacy/audit paths dedup priority over the authoritative firewall/logs path, inverting the priority model established by the shell script.
<details>
<summary>💡 Detail</summary>
readDedupedTokenUsage() keeps the first occurrence of each request_id, so the array order [TOKEN_USAGE_AUDIT_PATH, TOKEN_USAGE_AWF_AUDIT_PATH, TOKEN_USAGE_PATH] means:
- Legacy
firewall-audit-logs/entries win if arequest_idappears in multiple files - AWF audit entries…
|
🎉 This pull request is included in a new release. Release: |
TokenUsage=0fleet-wide since ~June 20 (AWF v0.27.7 rollout). Two bugs conspired: the conclusion job's sequentialcpoverwrote real proxy-log token data with an empty audit-dir stub, and the agent job'sparse_token_usage.cjsnever checked the AWF audit path at all.Bug 1 — Empty stub overwrites real data (conclusion job)
buildUsageArtifactUploadStepscopied token-usage files in last-wins order using[ -f ](true for empty files). AWF v0.27.7+ creates an emptyapi-proxy-logs/token-usage.jsonlunder--audit-dir(firewall/audit/), which silently zeroed out the valid data fromfirewall/logs/.Before (broken — audit dir stub wins):
After (fixed — proxy logs win, empty files skipped):
[ -s ]skips empty files;firewall/logs/goes last so it always wins when non-empty.Bug 2 — Missing audit path in agent job (
parse_token_usage.cjs)TOKEN_USAGE_PATHSonly includedfirewall-audit-logs/(legacy) andfirewall/logs/(primary). AddedTOKEN_USAGE_AWF_AUDIT_PATHforfirewall/audit/api-proxy-logs/token-usage.jsonlso the agent job also captures token data if AWF writes it there.Changes
pkg/workflow/notify_comment.go—[ -f ]→[ -s ]; reorder agent + detection copies sofirewall/logs/is highest priority (last)actions/setup/js/parse_token_usage.cjs— addTOKEN_USAGE_AWF_AUDIT_PATH; include inTOKEN_USAGE_PATHSactions/setup/js/parse_token_usage.test.cjs— update constant and path-list testspkg/workflow/notify_comment_test.go— assert-scheck is used andfirewall/logs/copy appears afterfirewall/audit/copy.github/workflows/*.lock.yml— regenerated (253 files)