Replace raw fmt.Fprintf verbose debug output with console.LogVerbose in token_usage.go#39247
Conversation
…oken_usage.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR standardizes verbose debug output in pkg/cli/token_usage.go by routing messages through console.LogVerbose() instead of writing directly to os.Stderr, so verbose output is consistently formatted and gated in one place.
Changes:
- Replaced direct
fmt.Fprintf(os.Stderr, ...)verbose messages withconsole.LogVerbose(verbose, ...)inanalyzeTokenUsageandanalyzeTokenUsageAICOnly. - Updated message construction to satisfy the
perfsprintguidance by using string concatenation for the single%s-only case.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/token_usage.go | Switches verbose token-usage discovery logs from raw stderr printing to console.LogVerbose() for consistent formatting and centralized gating. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 5
| fileInfo, _ := os.Stat(filePath) | ||
| if fileInfo != nil { | ||
| console.LogVerbose(verbose, fmt.Sprintf(" Found token usage file: %s (%d bytes)", filepath.Base(filePath), fileInfo.Size())) | ||
| } |
| agentFileInfo, _ := os.Stat(agentUsagePath) | ||
| if agentFileInfo != nil { | ||
| console.LogVerbose(verbose, fmt.Sprintf(" Found agent usage file: %s (%d bytes)", filepath.Base(agentUsagePath), agentFileInfo.Size())) | ||
| } |
| fileInfo, _ := os.Stat(filePath) | ||
| if fileInfo != nil { | ||
| console.LogVerbose(verbose, fmt.Sprintf(" Found token usage file: %s (%d bytes)", filepath.Base(filePath), fileInfo.Size())) | ||
| } |
| agentFileInfo, _ := os.Stat(agentUsagePath) | ||
| if agentFileInfo != nil { | ||
| console.LogVerbose(verbose, fmt.Sprintf(" Found agent usage file: %s (%d bytes)", filepath.Base(agentUsagePath), agentFileInfo.Size())) | ||
| } |
| if verbose { | ||
| fmt.Fprintf(os.Stderr, " Found usage JSONL files: %s\n", strings.Join(usageJSONLFiles, ", ")) | ||
| } | ||
| console.LogVerbose(verbose, " Found usage JSONL files: "+strings.Join(usageJSONLFiles, ", ")) |
|
@copilot review all comments and address unresolved review feedback.
|
|
Please refresh the branch and rerun checks, then summarize any remaining blockers.
|
|
Hey One gap to address before this is merge-ready:
If you'd like the agent to take care of it:
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39247 does not have the implementation label and has only 13 new lines of code in business logic directories (≤100 threshold). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The only changed file is pkg/cli/token_usage.go (production code). Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /improve-codebase-architecture — one non-blocking observation on a subtle behavioral change.
📋 Key Themes & Highlights
Key Theme
- Unconditional
os.Statsyscalls: moving the stat calls outside theif verboseguard means they now run on every invocation, even whenverbose=false. The file stat result is only consumed for the log message, so this adds work that was previously avoided.
Positive Highlights
- ✅ Correct goal:
console.LogVerboseis the right abstraction; rawfmt.Fprintf(os.Stderr, ...)with a manual verbose gate is exactly the duplication this helper was designed to eliminate. - ✅ Clean, consistent application across both
analyzeTokenUsageandanalyzeTokenUsageAICOnly. - ✅ String concatenation (vs.
fmt.Sprintf) for the%s-only JSONL message satisfies theperfsprintlinter cleanly. - ✅ Sensible variable rename
fileInfo→agentFileInfoto avoid shadowing at the broader scope.
Suggestion
A LogVerbosef(verbose bool, format string, args ...any) variant in pkg/console would let the verbose gate live inside the helper for all call sites — including ones that need to compute expensive arguments. That would be the long-term fix without reintroducing explicit if verbose wrappers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 183.1 AIC · ⌖ 13.5 AIC · ⊞ 27.9K
| if fileInfo != nil { | ||
| fmt.Fprintf(os.Stderr, " Found token usage file: %s (%d bytes)\n", filepath.Base(filePath), fileInfo.Size()) | ||
| } | ||
| fileInfo, _ := os.Stat(filePath) |
There was a problem hiding this comment.
[/zoom-out] os.Stat is now called unconditionally even when verbose=false — this is a subtle behavioral change from the original code where the stat was guarded by if verbose.
💡 Details and suggested fix
Since fileInfo is only used to format the log message, the syscall was previously skipped when not in verbose mode. The same pattern repeats at lines 461, 641, and 683.
The cleanest fix is to keep a verbose guard around both the os.Stat and the log call:
if verbose {
if fileInfo, _ := os.Stat(filePath); fileInfo != nil {
console.LogVerbose(verbose, fmt.Sprintf(" Found token usage file: %s (%d bytes)", filepath.Base(filePath), fileInfo.Size()))
}
}Alternatively, consider adding a LogVerbosef(verbose bool, format string, args ...any) helper to the console package. This would internalize the verbose gate so call sites never need the explicit if verbose wrapper — and it would allow lazy argument evaluation without adding syscalls in non-verbose paths.
There was a problem hiding this comment.
Five instances of the same regression: non-verbose paths now do more work than before this PR.
The PR goal was to standardize verbose output through console.LogVerbose, but the implementation inverts the performance contract on non-verbose paths. See the five existing inline comments for per-site details.
Root cause: console.LogVerbose takes a pre-formatted string, so callers must eagerly evaluate all arguments before the verbose guard runs. The old if verbose { ... } wrappers correctly deferred both syscalls and string construction. Restore those wrappers while keeping console.LogVerbose for the actual write:
if verbose {
fileInfo, _ := os.Stat(filePath)
if fileInfo != nil {
console.LogVerbose(verbose, fmt.Sprintf(" Found token usage file: %s (%d bytes)", filepath.Base(filePath), fileInfo.Size()))
}
}🔎 Code quality review by PR Code Quality Reviewer · 322.3 AIC · ⌖ 13.1 AIC · ⊞ 16.7K
Verbose debug messages in
pkg/cli/token_usage.gousedfmt.Fprintf(os.Stderr, ...)directly instead ofconsole.LogVerbose(), bypassing consistent styled output and redundantly re-implementing the verbose gate.Changes
analyzeTokenUsageandanalyzeTokenUsageAICOnly: replaced all 5if verbose { fmt.Fprintf(os.Stderr, ...) }blocks withconsole.LogVerbose(verbose, ...), removing the redundant explicit verbose checkos.Statsolely to populate the log message, moved the stat call outside the (now-removed)if verbosewrapperfmt.Sprintffor the single%s-only format (perperfsprintlinter)