Refactor token usage CLI into focused modules#47088
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ 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. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel analysis skipped. |
There was a problem hiding this comment.
Pull request overview
Refactors token-usage analysis into focused modules while preserving existing behavior and APIs.
Changes:
- Separates models, parsing, AIC, steering, and subagent attribution.
- Retains orchestration and existing test coverage.
- Includes an unrelated generated workflow permission change.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/token_usage.go |
Retains orchestration and constants. |
pkg/cli/token_usage_models.go |
Houses data models and summary helpers. |
pkg/cli/token_usage_parser.go |
Handles usage-file discovery and parsing. |
pkg/cli/token_usage_aic.go |
Handles AI Credits calculation. |
pkg/cli/token_usage_steering.go |
Detects steering events. |
pkg/cli/token_usage_subagent.go |
Handles subagent model attribution. |
.github/workflows/smoke-call-workflow.lock.yml |
Changes a generated workflow permission. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Medium
| # Review the called workflow's job-level permissions in ./.github/workflows/smoke-workflow-call.lock.yml. | ||
| permissions: | ||
| actions: write | ||
| actions: read |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Refactor token usage CLI into focused modules
The decomposition is clean and the module boundaries are well-chosen. All logic moves faithfully — no behavioral regressions spotted in the token-aggregation, AIC computation, steering detection, or subagent attribution paths.
One blocking issue found:
correlateToolCallsWithTokenDelta in token_usage_subagent.go (line 192–195) is a dead-code stub that was introduced in this PR. Its body does nothing (_ = tokenUsageFile; return toolCalls) and it is never called. It should be removed before merging to avoid confusing future readers.
Minor observations (non-blocking):
findTokenUsageFile(parser) andfindAPIProxyEventsFile(steering) share near-identical directory-scan loops; a shared helper could reduce duplication, but that is pre-existing and outside this PR's scope.analyzeTokenUsageAICOnlyintoken_usage_aic.gostill contains an inlinedefer file.Close()pattern (vs the explicit close-error-propagation pattern used inprocessOneUsageJSONLFile); inconsistent but not a bug.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 41.2 AIC · ⌖ 4.47 AIC · ⊞ 4.9K
| func correlateToolCallsWithTokenDelta(toolCalls []MCPToolCall, tokenUsageFile string) []MCPToolCall { | ||
| _ = tokenUsageFile | ||
| return toolCalls | ||
| } |
There was a problem hiding this comment.
Unused stub function — dead code
correlateToolCallsWithTokenDelta is never called anywhere in the codebase and its body discards all arguments (_ = tokenUsageFile). This stub should either be removed or implemented and wired up before merging.
@copilot please address this.
Generated by the Design Decision Gate from PR #47088 diff.
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,125 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. 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 MatterADRs 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.
Skills-Based Review 🧠
Applied /codebase-design — the decomposition is excellent and the module boundaries are clear. Leaving a few targeted comments on consistency and correctness issues that surfaced during the split.
📋 Key Themes & Highlights
Key Themes
defer file.Close()inconsistency:token_usage_parser.gosilently ignores close errors whiletoken_usage_aic.gocaptures them correctly — easy to fix and worth making consistent.findTokenUsageFilewalk uses a shared variable for both the initial candidate and the walk result, requiring a stringly-typed inequality check to distinguish them.findAgentUsageFilein the same file uses the cleaner pattern.hasTokenDataalias: a single-use alias ofhasRawTokenDataadds naming noise without adding meaning.- Dead stub
correlateToolCallsWithTokenDelta: no-op function that discards its argument — should be aTODOcomment or removed. - Walk duplication across
_aic.goand_parser.go: both modules walkrunDir; a shared helper would make this a single responsibility. ModelRowssort omitsReasoningTokens: reasoning-heavy models are ranked lower than their actual cost warrants.
Positive Highlights
- ✅ Excellent module decomposition — each file has exactly one domain responsibility
- ✅ Orchestration in
token_usage.gois now clean and readable - ✅
proxyEventsEntry.eventName()is a nice, testable value method - ✅
TokenCoreMetricsas an embedded struct is a great use of Go composition - ✅
scanSteeringEntriestakes anio.Reader, making it trivially unit-testable
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 63.3 AIC · ⌖ 5.04 AIC · ⊞ 6.6K
Comment /matt to run again
| if err != nil { | ||
| return nil, fmt.Errorf("failed to open token usage file: %w", err) | ||
| } | ||
| defer file.Close() |
There was a problem hiding this comment.
[/codebase-design] file.Close() is not error-checked here, while token_usage_aic.go uses a deferred error-capturing closure for the same pattern — inconsistency across the new modules.
💡 Suggestion
Use the same pattern as processOneUsageJSONLFile:
defer func() {
if closeErr := file.Close(); closeErr != nil && err == nil {
err = fmt.Errorf("failed to close token usage file: %w", closeErr)
}
}()Or add an explicit //nolint:errcheck to signal the omission is intentional.
@copilot please address this.
| }); walkErr != nil && !errors.Is(walkErr, filepath.SkipAll) { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("filesystem error walking %s: %v", runDir, walkErr))) | ||
| } | ||
| if primary != filepath.Join(runDir, "sandbox", "firewall", "logs", tokenUsageJSONLPath) { |
There was a problem hiding this comment.
[/codebase-design] The findTokenUsageFile walk re-uses the primary variable as both the initial candidate path and the walk result, making the final check confusing and fragile.
💡 Suggestion
Use a dedicated found variable in the walk closure (like findAgentUsageFile does right below), then return it:
var walkFound string
filepath.Walk(runDir, func(path string, ...) error {
if info.Name() == "token-usage.jsonl" || info.Name() == "token_usage.jsonl" {
walkFound = path
return filepath.SkipAll
}
return nil
})
if walkFound != "" {
return walkFound
}This removes the stringly-typed comparison if primary != filepath.Join(...) and brings the pattern in line with findAgentUsageFile.
@copilot please address this.
| summary.TotalCacheReadTokens > 0 || | ||
| summary.TotalCacheWriteTokens > 0 || | ||
| entry.ReasoningTokens > 0 | ||
| hasTokenData := hasRawTokenData |
There was a problem hiding this comment.
[/codebase-design] hasTokenData is assigned hasRawTokenData and never modified — the intermediate variable adds no clarity and is easy to misread as a distinct concept.
💡 Suggestion
Remove hasTokenData and use hasRawTokenData directly:
if hasRawTokenData {
...
}
...
} else if hasRawTokenData {
populateAIC(summary)
}This keeps one name for one concept, which is the core of /codebase-design's deep-module principle.
@copilot please address this.
| return found | ||
| } | ||
|
|
||
| func correlateToolCallsWithTokenDelta(toolCalls []MCPToolCall, tokenUsageFile string) []MCPToolCall { |
There was a problem hiding this comment.
[/codebase-design] correlateToolCallsWithTokenDelta is a no-op stub that discards its tokenUsageFile argument with _ = tokenUsageFile — this is dead code that will confuse future readers and defeat static analysis.
💡 Suggestion
If this is a placeholder for future work, document it explicitly:
// TODO(#ISSUE): correlate tool calls with token delta when token-usage granularity is available.
func correlateToolCallsWithTokenDelta(toolCalls []MCPToolCall, _ string) []MCPToolCall {
return toolCalls
}Or, if it is never called (check with grep -r correlateToolCallsWithTokenDelta), remove it entirely to avoid dead-code confusion.
@copilot please address this.
| if err != nil { | ||
| return nil, fmt.Errorf("failed to open token usage file: %w", err) | ||
| } | ||
| defer file.Close() |
There was a problem hiding this comment.
[/codebase-design] findUsageJSONLFiles in token_usage_aic.go and findTokenUsageFile/findAgentUsageFile in token_usage_parser.go both walk the same runDir using the same pattern. The discovery logic is split across two modules with no shared abstraction.
💡 Suggestion
Consider extracting a single walkForFile(runDir, matchFn) helper (or re-using findUsageJSONLFiles as the base) so that the walk pattern lives in one place — consistent with the /codebase-design principle of having one module own one responsibility.
@copilot please address this.
| jTot := b.InputTokens + b.OutputTokens + b.CacheReadTokens + b.CacheWriteTokens | ||
| if iTot > jTot { | ||
| return -1 | ||
| } |
There was a problem hiding this comment.
[/codebase-design] ModelRows() sorts by token sum descending but ReasoningTokens is excluded from the sort key, silently under-counting cost for reasoning-heavy models.
💡 Suggestion
Include ReasoningTokens in the sort total to match how expensive reasoning models are ranked:
iTot := a.InputTokens + a.OutputTokens + a.CacheReadTokens + a.CacheWriteTokens + a.ReasoningTokensOr, if reasoning tokens are intentionally excluded, add a comment explaining why.
@copilot please address this.
There was a problem hiding this comment.
Review: Faithful refactor, one low-risk inconsistency noted
This is a clean mechanical decomposition — logic is moved verbatim, no behavioral changes are introduced. The fallback ordering, AIC semantics, steering integration, and subagent attribution all carry over correctly.
📋 Analysis summary
What was verified:
parseTokenUsageFileandparseAgentUsageFileintoken_usage_parser.goare byte-for-byte identical to the originalspopulateAIC,sumAICFromUsageJSONLFiles, andanalyzeTokenUsageAICOnlyintoken_usage_aic.gomatch the originals- Steering detection logic in
token_usage_steering.gois identical - Subagent attribution in
token_usage_subagent.gois identical TotalTokens,AvgDurationMs,ModelRowsintoken_usage_models.gomatch the originals- The
findTokenUsageFilewalk closure variable capture pattern (primaryreassigned via closure) is preserved correctly
One low-risk inconsistency flagged (inline comment): ModelRows() lacks the nil guard that populateAIC and augmentSubagentModelAttribution use, which could panic on a nil map value in test scenarios.
No blocking issues found.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 140.4 AIC · ⌖ 4.54 AIC · ⊞ 5.6K
Comment /review to run again
| TotalCacheWriteTokens int `json:"total_cache_write_tokens" console:"header:Cache Write,format:number"` | ||
| TotalRequests int `json:"total_requests" console:"header:Requests"` | ||
| TotalSteeringEvents int `json:"total_steering_events,omitempty" console:"header:Steering Events,format:number,omitempty"` | ||
| TotalDurationMs int `json:"total_duration_ms"` |
There was a problem hiding this comment.
Nil dereference risk in ModelRows(): usage is accessed without a nil guard and will panic if the map contains a nil value.
💡 Details
Sibling functions populateAIC and augmentSubagentModelAttribution both guard against nil entries (if usage == nil { continue }), but ModelRows does not. While current call sites never insert nil values, this is an inconsistency that will silently panic if a nil entry is ever added in tests or future code paths.
Suggested fix:
for model, usage := range s.ByModel {
if usage == nil {
continue
}
avgDur := 0
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🤖 PR Triage
Score breakdown: Impact 22 · Urgency 12 · Quality 11
|
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29861145850 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
I ran the PR-finisher pass and triaged all currently open review threads plus CI state.
Exact next maintainer action: please re-trigger PR checks on the current HEAD (close/reopen PR, |
|
@copilot please run the Unresolved review threads (newest first):
Run: https://github.com/github/gh-aw/actions/runs/29873842709 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
…output schema Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the Unresolved review threads (newest first):
Run: https://github.com/github/gh-aw/actions/runs/29885935834 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Completed a PR-finisher pass.
Smallest next maintainer action: re-trigger PR checks on current HEAD (close/reopen PR, |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29889445104 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Completed a PR-finisher pass.
Exact next maintainer action: re-trigger PR checks on current HEAD (close/reopen PR, |
pkg/cli/token_usage.gohad grown into a multi-responsibility file (~1.1k LOC), which made token parsing, AIC computation, steering detection, and subagent attribution hard to evolve safely. This change decomposes that logic into focused modules while preserving existing behavior and API surface.Module decomposition
token_usage_models.go: shared data models and summary helpers (TotalTokens,AvgDurationMs,ModelRows)token_usage_parser.go: token/agent usage file discovery and parsing, ambient-context extractiontoken_usage_aic.go: usage JSONL scanning and AI Credits derivation/populationtoken_usage_steering.go: proxy event parsing and steering-event classificationtoken_usage_subagent.go: subagent model-request extraction and request-vs-observed attributiontoken_usage.go: reduced to orchestration + constantsBehavioral continuity
token_usage.jsonl→agent_usage.json)Resulting structure
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Run: https://github.com/github/gh-aw/actions/runs/29861145850
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.