Fix zero token usage in the logs MCP tool run schema - #60424
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 3 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"
- "github.com"
- "raw.githubusercontent.com"See Network Configuration for more information.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
🟡 Changes recommended
The new combined default bypasses the activation-artifact fallback for legacy runs, and zero-value serialization lacks regression coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes missing token-usage metrics in MCP logs output by ensuring usage artifacts are downloaded and zero values remain serialized.
Changes:
- Adds
usageto default and explicit artifact selections. - Always serializes
token_usageandaic. - Adds normalization tests and a patch changeset.
File summaries
| File | Description |
|---|---|
pkg/cli/mcp_tools_privileged.go |
Updates MCP logs artifact selection and documentation. |
pkg/cli/mcp_tools_privileged_test.go |
Tests artifact defaults and normalization. |
pkg/cli/logs_report.go |
Preserves zero-valued usage fields in JSON. |
.changeset/logs-mcp-token-usage-default-artifacts.md |
Documents the patch. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| func effectiveMCPLogsToolArtifacts(artifacts []string) []string { | ||
| if len(artifacts) == 0 { | ||
| return slices.Clone(defaultMCPLogsToolArtifacts) |
There was a problem hiding this comment.
Fixed in 2b71844: added isInfoWithOptionalUsageArtifactFilter, which treats info and info+usage filters the same for the activation-artifact fallback (used at the ErrNoArtifacts checks and the post-download aw_info.json check). Legacy runs missing both compact artifacts now fall back to the activation artifact as before.
| // TokenUsage is always emitted (even when 0) so consumers of the run list can | ||
| // discover the field and distinguish "no tokens recorded" from "field absent". | ||
| TokenUsage int `json:"token_usage" console:"header:Tokens,format:number,omitempty"` | ||
| AIC float64 `json:"aic"` |
There was a problem hiding this comment.
Added TestRunDataJSONIncludesZeroTokenUsageAndAIC in 2b71844, which marshals a zero-metric RunData and asserts both "token_usage":0 and "aic":0 are present in the output.
🏗️ ADR Required — draft added for PR #60424I enforced the design-decision gate for this PR because it adds more than 100 new lines in business-logic directories ( Evidence reviewed
Action taken
Next action for the authorPlease review and refine the draft ADR, especially the rationale and trade-offs, before merging this PR. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
|
![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 /diagnosing-bugs — solid regression fix and test coverage, but the generated schemas were not refreshed.
📋 Key Themes & Highlights
Key Themes
- Stale generated schema: dropping
omitemptyfromRunData.TokenUsage/AIC(pkg/cli/logs_report.go) changes the JSON Schemarequiredlist, butschemas/logs.schema.jsonandschemas/logs-jsonl.schema.jsonweren't regenerated.TestGeneratedOutputSchemasAreCurrentfails as-is; runningmake schemas(ormake recompile) reproduces and fixes the diff. Left an inline comment with the exact repro.
Positive Highlights
- ✅ Root cause is correctly diagnosed (missing
usageartifact download, not a downstream aggregation bug) and the fix addresses it directly rather than papering over symptoms. - ✅
effectiveMCPLogsToolArtifactsis well-covered: default case, explicit-set augmentation, and both no-op cases (usage,all) each have a dedicated test. - ✅
TestLogsToolDefaultsToUsageArtifactis a genuine regression guard that exercises the full tool registration + handler path, not just the helper function in isolation.
Requesting changes only for the stale schema files — regenerate and commit them and this is good to merge.
Warning
Firewall blocked 3 domains
The following domains were blocked by the firewall during workflow execution:
api.github.comgithub.comraw.githubusercontent.com
[!TIP]
api.github.com is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding api.github.com to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to api.github.com:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "api.github.com"
- "github.com"
- "raw.githubusercontent.com"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 111.2 AIC · ⌖ 15 AIC · ⊞ 10.4K
Comment /matt to run again
| ActionMinutes float64 `json:"action_minutes,omitempty" console:"header:Action Minutes,omitempty"` | ||
| // TokenUsage is always emitted (even when 0) so consumers of the run list can | ||
| // discover the field and distinguish "no tokens recorded" from "field absent". | ||
| TokenUsage int `json:"token_usage" console:"header:Tokens,format:number,omitempty"` |
There was a problem hiding this comment.
[/diagnosing-bugs] Removing omitempty here makes token_usage/aic required in the generated JSON Schema, but schemas/logs.schema.json and schemas/logs-jsonl.schema.json weren't regenerated — make schemas (or make recompile) still produces a diff adding "token_usage" and "aic" to the required array. TestGeneratedOutputSchemasAreCurrent fails on this branch.
💡 Verification
$ ./gh-aw json-schema logs > /tmp/new.json
$ diff /tmp/new.json schemas/logs.schema.json
1000,1001d999
< "token_usage",
< "aic",
Same delta for logs-jsonl.schema.json. Run make schemas (or make recompile) and commit the regenerated files.
@copilot please address this.
There was a problem hiding this comment.
Regenerated in 2b71844 via make schemas; TestGeneratedOutputSchemasAreCurrent passes now.
|
@copilot run pr-finishers skill |
…/aic Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the pr-finisher pass:
Nothing else actionable remains on my side; ready for maintainer review/merge. |
|
🎉 This pull request is included in a new release. Release: |
The
logsMCP tool registered its schema withartifacts: ["info"], which only downloadsaw_info.json. The compactusageartifact carryingtoken_usage.jsonl/agent_usage.jsonwas never fetched, so every run hadTokenUsage == 0and — thanks toomitempty— thetoken_usagekey disappeared from the JSON entirely. Any report averaging token usage over the run list computed exactly 0.Changes
pkg/cli/mcp_tools_privileged.gologstool are nowinfo,usage.effectiveMCPLogsToolArtifactsappends the compactusageset to explicit selections too (no-op whenallorusageis already present), so token usage survives whichever sets a report requests.token_usage/aicand whyusageis always downloaded.pkg/cli/logs_report.go—token_usageandaicare always serialized onRunData, so the field is discoverable and0unambiguously means "no tokens recorded" rather than "field absent".usageset. Changeset included.The
usageartifact is the compact conclusion artifact, so the extra download is cheap relative to the metric it restores.