Remove misleading total_tokens: 0 from logs summary

[Copilot]

logs summary always reported total_tokens: 0 because token counts require the agent artifact (not downloaded by default), while only AIC cost is available from the usage artifact. This created a confusing total_tokens: 0 / total_aic: 445.247 inconsistency.

Changes

• LogsSummary: Remove TotalTokens field entirely — always zero without --artifacts agent, never meaningful in the summary context
• buildLogsData: Drop totalTokens accumulation variable
• Compact/TSV renderers: Remove tokens= from summary output lines in both formats
• Tests: Remove all TotalTokens/total_tokens assertions from LogsSummary across 7 test files; replace field-existence checks that were asserting total_tokens is present with equivalent checks on fields that are actually populated

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

🧠 Matt Pocock Skills Reviewer failed during the skills-based review.

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #42722 does not have the 'implementation' label and has only 15 new lines of code in business logic directories (threshold: 100).

[Copilot]

Pull request overview

Removes the misleading total_tokens aggregate from gh aw logs summaries and related renderers/tests, since token totals are not available without downloading agent artifacts and were consistently reported as zero.

Changes:

• Removes TotalTokens from LogsSummary and stops accumulating totalTokens in buildLogsData.
• Updates compact and TSV summary lines to omit tokens=....
• Updates affected tests to stop asserting total_tokens presence/value in the logs summary.

Show a summary per file

File	Description
pkg/cli/logs_report.go	Drops LogsSummary.TotalTokens and removes token aggregation from buildLogsData.
pkg/cli/logs_format_compact.go	Removes tokens= from compact/verbose compact summary output.
pkg/cli/logs_format_tsv.go	Removes token count from TSV summary header line and updates format docs accordingly.
pkg/cli/mcp_schema_test.go	Updates schema validation fixture data to match LogsSummary without TotalTokens.
pkg/cli/logs_summary_file_test.go	Removes TotalTokens from summary file fixtures/assertions.
pkg/cli/logs_report_test.go	Updates console rendering test fixtures to omit TotalTokens.
pkg/cli/logs_json_test.go	Removes TotalTokens assertions from logs summary-related JSON tests.
pkg/cli/logs_json_clean_test.go	Updates “field exists” assertions to avoid total_tokens in summary.
pkg/cli/logs_empty_runs_test.go	Removes “summary total_tokens exists/is zero” expectations for empty runs.
pkg/cli/logs_ci_scenario_test.go	Updates required/expected summary fields lists to remove total_tokens.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Low

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 83/100 — Excellent

Analyzed 12 test(s) across 7 modified files: 10 design, 2 implementation, 0 violation(s).

📊 Metrics (12 modified tests)

Metric	Value
Analyzed	12 (Go: 12, JS: 0)
✅ Design	10 (83%)
⚠️ Implementation	2 (17%)
Edge/error coverage	9 (75%)
Duplicate clusters	0
Inflation	No (net −55 lines)
🚨 Violations	0

Test	File	Classification	Issues
TestLogsJSONOutputWithNoRuns	logs_ci_scenario_test.go	design_test	None
TestLogsJSONOutputStructure	logs_ci_scenario_test.go	design_test	None
TestSummaryFileWrittenWithNoRuns	logs_ci_scenario_test.go	design_test	None
TestBuildLogsDataEmptyRuns	logs_empty_runs_test.go	implementation_test	None
TestRenderLogsJSONEmptyRuns	logs_empty_runs_test.go	design_test	Weakened (see below)
TestJSONOutputNotCorruptedByStderr	logs_json_clean_test.go	design_test	None
TestStderrMessagesAfterJSON	logs_json_clean_test.go	design_test	None
TestBuildLogsData	logs_json_test.go	implementation_test	None
TestRenderLogsJSON	logs_json_test.go	design_test	None
TestRenderLogsConsoleUnified	logs_report_test.go	design_test	None
TestWriteSummaryFile	logs_summary_file_test.go	design_test	None
TestGeneratedSchemasValidateRealOutput	mcp_schema_test.go	design_test	None

⚠️ Flagged Tests (1)

TestRenderLogsJSONEmptyRuns (logs_empty_runs_test.go) — The test was simplified from checking for the presence of total_tokens to only verifying the summary is a map[string]any. The summary variable is now discarded (_), losing the ability to assert on specific field presence. While the removal of total_tokens is correct (it was always 0 and is now gone), the test no longer verifies any meaningful field in the JSON summary. Consider adding an assertion on a still-present summary field such as total_runs to maintain behavioral coverage, similar to how TestJSONOutputNotCorruptedByStderr was updated.

Verdict

✅ Passed. 17% implementation tests (threshold: 30%). All test changes are appropriate cleanup of total_tokens references that aligned tests with production code. Build tags (//go:build !integration) present on all files. No mock violations.

References:

• §28515924851

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🧪 Test quality analysis by Test Quality Sentinel · 49.3 AIC · ⌖ 18 AIC · ⊞ 6.8K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 83/100. 17% implementation tests (threshold: 30%). All test changes are appropriate cleanup of misleading total_tokens: 0 references. No guideline violations.

[github-actions]

Non-blocking observations

The fix is directionally correct — surfacing total_tokens: 0 alongside a non-zero total_aic was genuinely confusing. Two concerns worth addressing before this lands:

🔍 Findings summary

1. --artifacts agent regression (logs_report.go): TotalTokens is dropped entirely rather than made omitempty. When users run with --artifacts agent, per-run RunData.TokenUsage fields are non-zero — but the summary now never aggregates them. Using omitempty would hide the field in the default (zero) case while preserving it for artifact-download mode.

2. Silent TSV positional breakage (logs_format_tsv.go): The # N runs | T duration | ... header line loses one positional field. Scripts that parse this positionally will silently misread subsequent fields. No test covers the exact TSV summary header format.

🔎 Code quality review by PR Code Quality Reviewer · 112.2 AIC · ⌖ 9.59 AIC · ⊞ 1.6K
_{Comment /review to run again}

[github-actions]

--artifacts agent mode loses token summary: removing TotalTokens entirely drops the aggregate even when per-run token_usage fields are non-zero (populated by the agent artifact).

💡 Suggested fix

The PR's stated rationale is that the field is "always zero without --artifacts agent" — which is true. But when --artifacts agent IS used, run.TokenUsage (RunData.TokenUsage, line 109) is populated with real non-zero counts, and totalTokens += run.TokenUsage was the only place those were aggregated into the summary.

A less destructive fix: keep the field but suppress it when zero:

// In LogsSummary struct:
TotalTokens int `json:"total_tokens,omitempty" console:"header:Total Tokens,format:number,omitempty"`

This makes total_tokens absent from JSON in the default mode (matches the stated goal) while preserving the aggregate for users who download agent artifacts. The current change silently regresses --artifacts agent users who relied on the summary token count.

[github-actions]

Review: Remove misleading total_tokens: 0 from logs summary

The changes are correct and well-reasoned. The total_tokens field in LogsSummary was always 0 in normal usage because token data requires the --artifacts agent flag, while the logs command only downloads the usage artifact (providing total_aic). Emitting a persistent total_tokens: 0 alongside a real total_aic value was genuinely misleading.

What was reviewed

• logs_report.go — TotalTokens field removed from LogsSummary; totalTokens accumulator dropped from buildLogsData. The remaining per-run run.TokenUsage field on RunData is intentionally kept.
• logs_format_compact.go / logs_format_tsv.go — tokens= removed from summary output lines. TSV doc comment updated to match.
• 7 test files — TotalTokens/total_tokens assertions removed. Field-existence checks previously asserting total_tokens is present are correctly replaced with checks on fields that are actually populated.

Observations

1. EpisodeMetadata.TotalTokens (in logs_episode.go) is untouched — correct, episodes aggregate run.TokenUsage which CAN be non-zero when the agent artifact is present.
2. health_metrics.go and audit_cross_run*.go retain their TotalTokens fields — correct, different data paths unaffected by this change.
3. cost-management.md documents total_tokens as an episode field (episodes[].total_tokens), which is unaffected by this change.
4. total_aic has omitempty in JSON tags and would be 0 in the no-runs test, so its absence from the mandatory expectedFields list is intentional and correct.

No issues found. Clean, targeted removal with consistent test updates.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 56.4 AIC · ⌖ 6.52 AIC · ⊞ 4.9K

[pelikhan]

@copilot run pr-finisher skill
fix https://github.com/github/gh-aw/actions/runs/28515924920/job/84527557672

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs — one blocking issue: a test file was not updated alongside the others.

📋 Key Themes & Highlights

Blocking Issue

• logs_json_stderr_order_test.go not updated: This file still has 4 assertions checking for total_tokens (lines 106–107, 230–231, 236–237). Since the field was removed from LogsSummary, these will fail. It was not included in the PR's list of changed files.

Positive Highlights

• ✅ Root cause is correctly identified and fixed — TotalTokens was never meaningful without --artifacts agent; removing it is the right call.
• ✅ Per-run token_usage on RunData (line 109) is intentionally preserved — it remains useful when the agent artifact is downloaded.
• ✅ TSV format doc comment updated to match the new output format.
• ✅ Comprehensive sweep across 7 test files — only one was missed.
• ✅ Using omitempty on total_aic means it stays absent from JSON when zero, keeping the fix consistent.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 123.8 AIC · ⌖ 9.47 AIC · ⊞ 6.6K
_{Comment /matt to run again}

[gh-aw-bot]

@copilot please run the pr-finisher skill, address the unresolved review feedback, and rerun checks. Prioritize: 1) update logs_json_stderr_order_test.go to remove the remaining total_tokens assertions called out by Matt Pocock Skills Reviewer, 2) evaluate the PR Code Quality Reviewer notes about preserving summary total_tokens behavior for --artifacts agent and TSV header compatibility, and either fix or explicitly justify them, 3) fix the failing CGO test job at https://github.com/github/gh-aw/actions/runs/28515924920/job/84527557672.

Generated by 👨‍🍳 PR Sous Chef · 27.2 AIC · ⌖ 28.7 AIC · ⊞ 6.4K · ◷

[Copilot]

@copilot run pr-finisher skill
fix https://github.com/github/gh-aw/actions/runs/28515924920/job/84527557672

Fixed in ddd6ce3. I updated the leftover logs summary assertions and the compiler action-pin selection that let a stale cached actions/cache/restore@v4 override the newer embedded pin, which resolves the TestWasmGolden_RoundTrip failure locally. make fmt, make lint, make test-unit, make test, and make recompile passed; CI on this head still needs a maintainer re-trigger.

Done

[Copilot]

@copilot please run the pr-finisher skill, address the unresolved review feedback, and rerun checks. Prioritize: 1) update logs_json_stderr_order_test.go to remove the remaining total_tokens ass...

Addressed on this branch in e70416f. I kept summary.total_tokens omitted in the default zero-token case, restored the aggregate when real run token data exists, preserved the TSV summary field position with n/a tokens/actual token counts, and added regression coverage for the TSV summary line. The earlier logs_json_stderr_order_test cleanup and the CGO TestWasmGolden_RoundTrip fix are also on this head. Local make fmt, make lint, make test-unit, make test, make recompile, and make agent-report-progress all pass. CI for the new head is still unverified until a maintainer re-triggers it.

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.82.2