Fix zero token usage in the logs MCP tool run schema

docs/adr/60424-ensure-logs-tool-downloads-usage-artifacts.md

@@ -0,0 +1,50 @@
+# ADR-60424: Ensure Logs Tool Downloads Usage Artifacts
+
+**Date**: 2026-09-12
+**Status**: Draft
+**Deciders**: gh-aw maintainers
+
+---
+
+### Context
+
+The PR fixes a bug where the `logs` MCP tool reported `TokenUsage == 0` for every run because its default artifact selection downloaded only the `info` artifact set and never fetched the compact `usage` artifact containing `token_usage.jsonl` and `agent_usage.json`. The PR description and diff show that downstream reports average token usage from per-run records, so omitting usage artifacts silently produced incorrect fleet analytics and even removed the `token_usage` key entirely because of `omitempty`. The implementation changes the logs tool schema defaults, normalizes explicit artifact selections, and adjusts JSON serialization so consumers can distinguish `0` from an absent field. The architectural question is how the logs tool should guarantee availability of per-run token metrics without forcing callers to understand internal artifact dependencies.
+
+### Decision
+
+We will make the `logs` MCP tool always include the compact `usage` artifact set in its effective artifact selection and always serialize `token_usage` and `aic` on each run record. We decided to default the tool to `info,usage` and to append `usage` to explicit artifact selections unless the caller already requested `usage` or `all`, because token metrics are part of the tool's contract and should not disappear due to an incomplete artifact list. This keeps report consumers aligned with the tool schema and restores correct token-usage analytics with minimal download overhead.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep `usage` optional and require callers to request it explicitly
+
+This matched the previous behavior where the tool could be invoked with only `info` or another narrow artifact subset. It was considered because it gives callers maximal control over artifact downloads. It was not chosen because the PR evidence shows callers and downstream reports treated `token_usage` as a normal part of run data, so leaving `usage` optional caused silent data corruption rather than an explicit opt-in trade-off.
+
+#### Alternative 2: Infer token usage from other downloaded artifacts or omit the field when unavailable
+
+Another option would be to keep existing artifact behavior and either derive token usage from heavier artifacts or continue omitting `token_usage` and `aic` when the data is missing. This was considered because it avoids modifying artifact defaults. It was not chosen because the PR description states the compact `usage` artifact is the authoritative, cheap source of those metrics, and omitting the fields made consumers interpret missing data as zero or fail to discover the metric at all.
+
+#### Alternative 3: Change only JSON serialization so `token_usage: 0` is always present
+
+The diff also removes `omitempty` from `token_usage` and `aic`, so one possible narrower decision would be to serialize zero values without changing artifact selection. This was considered because it improves schema discoverability. It was not chosen on its own because the root problem is absent usage data; always serializing `0` without downloading `usage` would preserve an incorrect metric rather than restore real token counts.
+
+### Consequences
+
+#### Positive
+- Per-run `token_usage` and `aic` are reliably present, so fleet analytics and logs reports can aggregate real usage metrics again.
+- The logs tool contract becomes safer for callers because explicit artifact selections still retain the compact data needed for token accounting.
+- Regression tests now guard both default and explicit artifact-selection paths, reducing the chance of silently reintroducing zeroed token metrics.
+
+#### Negative
+- The logs tool now downloads the `usage` artifact even when a caller requested a narrower set, slightly reducing strict caller control over artifact selection.
+- The implementation adds artifact-normalization logic and special handling for `all`/`usage`, increasing behavior complexity around schema defaults.
+- Future artifact-set changes must preserve this implicit dependency or update the tool contract and tests accordingly.
+
+#### Neutral
+- The change does not alter the external CLI shape beyond artifact defaults and documented behavior; callers still pass artifact-set names in the same way.
+- Run records with legitimately unavailable usage data now emit `token_usage: 0`, making missing-versus-zero semantics explicit at the JSON layer.
+- The patch frames `usage` as a compact dependency of the logs tool rather than as an optional reporting enhancement.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/logs_artifact_set_test.go

@@ -416,6 +416,30 @@ func TestIsInfoOnlyArtifactFilter(t *testing.T) {
 	}
 }
 
+func TestIsInfoWithOptionalUsageArtifactFilter(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name     string
+		filter   []string
+		expected bool
+	}{
+		{name: "info only", filter: []string{"info"}, expected: true},
+		{name: "info plus usage", filter: []string{"info", "usage"}, expected: true},
+		{name: "usage plus info reversed order", filter: []string{"usage", "info"}, expected: true},
+		{name: "info plus another artifact", filter: []string{"info", "agent"}, expected: false},
+		{name: "usage only", filter: []string{"usage"}, expected: false},
+		{name: "non-info only", filter: []string{"agent"}, expected: false},
+		{name: "empty filter", filter: nil, expected: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			assert.Equal(t, tt.expected, isInfoWithOptionalUsageArtifactFilter(tt.filter))
+		})
+	}
+}
+
 func TestShouldDownloadWorkflowRunLogs(t *testing.T) {
 	t.Parallel()
 	tests := []struct {

pkg/cli/logs_download.go

@@ -57,6 +57,30 @@ func isInfoOnlyArtifactFilter(artifactFilter []string) bool {
 	return len(artifactFilter) == 1 && artifactFilter[0] == constants.InfoArtifactName.String()
 }
 
+// isInfoWithOptionalUsageArtifactFilter reports whether the artifact filter requests
+// only the info artifact, or the info artifact plus the compact usage artifact (the
+// default selection used by the logs MCP tool). Both shapes must fall back to the
+// activation artifact for aw_info.json when the compact artifacts are unavailable, e.g.
+// legacy runs that predate the info/usage artifacts.
+func isInfoWithOptionalUsageArtifactFilter(artifactFilter []string) bool {
+	if isInfoOnlyArtifactFilter(artifactFilter) {
+		return true
+	}
+	if len(artifactFilter) != 2 {
+		return false
+	}
+	hasInfo, hasUsage := false, false
+	for _, artifact := range artifactFilter {
+		switch artifact {
+		case constants.InfoArtifactName.String():
+			hasInfo = true
+		case constants.UsageArtifactName.String():
+			hasUsage = true
+		}
+	}
+	return hasInfo && hasUsage
+}
+
 func shouldDownloadWorkflowRunLogs(artifactFilter []string) bool {
 	if len(artifactFilter) == 0 {
 		return true
@@ -175,7 +199,7 @@ func downloadRunArtifacts(ctx context.Context, opts downloadArtifactsOptions) er
 
 	downloadableNames, individualDownload, done, err := planArtifactDownload(ctx, opts, shouldLogProgress)
 	if done || err != nil {
-		if errors.Is(err, ErrNoArtifacts) && isInfoOnlyArtifactFilter(opts.artifactFilter) {
+		if errors.Is(err, ErrNoArtifacts) && isInfoWithOptionalUsageArtifactFilter(opts.artifactFilter) {
 			return downloadActivationAwInfoFallback(ctx, opts)
 		}
 		return err
@@ -192,7 +216,7 @@ func downloadRunArtifacts(ctx context.Context, opts downloadArtifactsOptions) er
 			spinner.Stop()
 		}
 		if err := downloadArtifactsIndividually(ctx, opts, downloadableNames); err != nil {
-			if errors.Is(err, ErrNoArtifacts) && isInfoOnlyArtifactFilter(opts.artifactFilter) {
+			if errors.Is(err, ErrNoArtifacts) && isInfoWithOptionalUsageArtifactFilter(opts.artifactFilter) {
 				return downloadActivationAwInfoFallback(ctx, opts)
 			}
 			return err
@@ -254,7 +278,7 @@ func finalizeArtifactDownload(ctx context.Context, opts downloadArtifactsOptions
 		return err
 	}
 
-	if isInfoOnlyArtifactFilter(opts.artifactFilter) && !fileutil.FileExists(filepath.Join(opts.outputDir, "aw_info.json")) {
+	if isInfoWithOptionalUsageArtifactFilter(opts.artifactFilter) && !fileutil.FileExists(filepath.Join(opts.outputDir, "aw_info.json")) {
 		if err := downloadActivationAwInfoFallback(ctx, opts); err != nil {
 			return err
 		}

pkg/cli/logs_report.go

@@ -150,11 +150,13 @@ type RunData struct {
 	//                   turns were observed, or job metadata shows agent=success followed
 	//                   by a failed safe_outputs job.
 	// ""              – the run did not fail (success), or turn data was unavailable for classification.
-	FailureKind                string                 `json:"failure_kind,omitempty" console:"-"`
-	Duration                   string                 `json:"duration,omitempty" console:"header:Duration,omitempty"`
-	ActionMinutes              float64                `json:"action_minutes,omitempty" console:"header:Action Minutes,omitempty"`
-	TokenUsage                 int                    `json:"token_usage,omitempty" console:"header:Tokens,format:number,omitempty"`
-	AIC                        float64                `json:"aic,omitempty"`
+	FailureKind   string  `json:"failure_kind,omitempty" console:"-"`
+	Duration      string  `json:"duration,omitempty" console:"header:Duration,omitempty"`
+	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"`
+	AIC                        float64                `json:"aic"`
 	AmbientContext             *AmbientContextMetrics `json:"ambient_context,omitempty" console:"-"`
 	WorkingSet                 *WorkingSetMetrics     `json:"working_set,omitempty" console:"-"`
 	WSRF                       string                 `json:"-" console:"header:WSRF,omitempty"` // Working-Set Rebuild Factor, pre-formatted for table display

pkg/cli/logs_report_test.go

@@ -940,6 +940,47 @@ func TestAccessLogSummaryJSONUsesEmbeddedBaseFields(t *testing.T) {
 	}
 }
 
+// TestRunDataJSONIncludesZeroTokenUsageAndAIC is a regression guard for the bug where
+// TokenUsage/AIC used `omitempty` and silently dropped out of the JSON output whenever
+// a run genuinely recorded zero tokens, making "field absent" indistinguishable from
+// "no tokens recorded". Marshaling a zero-metric RunData must still surface both keys.
+func TestRunDataJSONIncludesZeroTokenUsageAndAIC(t *testing.T) {
+	run := RunData{
+		RunID:        1,
+		WorkflowName: "wf-1",
+		WorkflowPath: ".github/workflows/wf-1.md",
+		Status:       "completed",
+		TokenUsage:   0,
+		AIC:          0,
+	}
+
+	data, err := json.Marshal(run)
+	if err != nil {
+		t.Fatalf("Marshal() error = %v", err)
+	}
+
+	var got map[string]any
+	if err := json.Unmarshal(data, &got); err != nil {
+		t.Fatalf("Unmarshal() error = %v", err)
+	}
+
+	tokenUsage, ok := got["token_usage"]
+	if !ok {
+		t.Fatalf("expected token_usage key to be present in %s", string(data))
+	}
+	if tokenUsage != float64(0) {
+		t.Fatalf("expected token_usage = 0, got %v", tokenUsage)
+	}
+
+	aic, ok := got["aic"]
+	if !ok {
+		t.Fatalf("expected aic key to be present in %s", string(data))
+	}
+	if aic != float64(0) {
+		t.Fatalf("expected aic = 0, got %v", aic)
+	}
+}
+
 // TestBuildFirewallLogSummaryWithSharedHelper tests firewall log summary with shared helper
 func TestBuildFirewallLogSummaryWithSharedHelper(t *testing.T) {
 	processedRuns := []ProcessedRun{

pkg/cli/mcp_tools_privileged.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"slices"
 	"strconv"
 	"strings"
 	"time"
@@ -116,7 +117,29 @@ type logsArgs struct {
 	MaxStorageMB          int      `json:"max_storage,omitempty" jsonschema:"Maximum logs storage in MB after pruning non-essential cache data (0 means unlimited)."`
 	PruneOlderRuns        bool     `json:"prune_older_runs,omitempty" jsonschema:"Remove oldest completed runs when non-essential cache pruning cannot satisfy max_storage."`
 	MaxTokens             int      `json:"max_tokens,omitempty" jsonschema:"Deprecated: accepted for backward compatibility but ignored. Output is always written to a file."`
-	Artifacts             []string `json:"artifacts,omitempty" jsonschema:"Artifact sets to download (default: usage). Valid sets: all, activation, agent, detection, evals, experiment, firewall, github-api, graders, mcp, usage"`
+	Artifacts             []string `json:"artifacts,omitempty" jsonschema:"Artifact sets to download (default: info,usage). Valid sets: all, activation, agent, detection, evals, experiment, firewall, github-api, graders, info, mcp, usage. The compact usage set is always added so per-run token_usage is populated."`
+}
+
+// defaultMCPLogsToolArtifacts is the artifact selection used when the caller does
+// not request specific sets.  The compact "info" artifact supplies run metadata and
+// the compact "usage" artifact supplies per-run token usage (token_usage.jsonl /
+// agent_usage.json).  Without the usage set every run reports zero tokens, which
+// silently breaks fleet-analytics reports that aggregate token usage from the run list.
+var defaultMCPLogsToolArtifacts = []string{string(ArtifactSetInfo), string(ArtifactSetUsage)}
+
+// effectiveMCPLogsToolArtifacts resolves the artifact sets the logs tool downloads.
+// Callers that omit the parameter get the defaults; callers that request specific
+// sets always get the compact usage set added so token usage stays populated.
+func effectiveMCPLogsToolArtifacts(artifacts []string) []string {
+	if len(artifacts) == 0 {
+		return slices.Clone(defaultMCPLogsToolArtifacts)
+	}
+	for _, set := range artifacts {
+		if ArtifactSet(set) == ArtifactSetAll || ArtifactSet(set) == ArtifactSetUsage {
+			return artifacts
+		}
+	}
+	return append(append([]string(nil), artifacts...), string(ArtifactSetUsage))
 }
 
 func defaultMCPLogsToolTimeoutMinutesForCount(count int) int {
@@ -171,7 +194,7 @@ func registerLogsTool(server *mcp.Server, execCmd execCmdFunc, actor string, val
 	logsSchema, err := generateSchemaWithDefaults[logsArgs](map[string]any{
 		"count":      defaultMCPLogsToolCount,
 		"max_tokens": 12000,
-		"artifacts":  []string{"info"},
+		"artifacts":  defaultMCPLogsToolArtifacts,
 	})
 	if err != nil {
 		mcpLog.Printf("Failed to generate logs tool schema: %v", err)
@@ -213,7 +236,12 @@ When results are incomplete, the tool response also sets "partial": true and rep
 "continuation" cursor inline, so partial results can be detected without reading the file.
 
 The continuation field includes all necessary parameters (before_run_id, etc.) to resume fetching
-from where the previous request stopped due to timeout.`
+from where the previous request stopped due to timeout.
+
+Each run record includes a "token_usage" field (input+output tokens) and an "aic" field.
+Both are always present, so aggregating them across runs yields real fleet-level metrics.
+The compact "usage" artifact set is downloaded by default (and added to any explicit
+artifact selection) because it carries the per-run token usage data.`
 
 // newLogsToolHandler builds the handler for the logs tool.
 func newLogsToolHandler(execCmd execCmdFunc, actor string, validateActor bool) func(context.Context, *mcp.CallToolRequest, logsArgs) (*mcp.CallToolResult, any, error) {
@@ -382,8 +410,8 @@ func appendLogsFilterArgs(cmdArgs []string, args logsArgs) []string {
 	if args.PruneOlderRuns {
 		cmdArgs = append(cmdArgs, "--prune-older-runs")
 	}
-	if len(args.Artifacts) > 0 {
-		cmdArgs = append(cmdArgs, "--artifacts", strings.Join(args.Artifacts, ","))
+	if artifacts := effectiveMCPLogsToolArtifacts(args.Artifacts); len(artifacts) > 0 {
+		cmdArgs = append(cmdArgs, "--artifacts", strings.Join(artifacts, ","))
 	}
 	return cmdArgs
 }

pkg/cli/mcp_tools_privileged_test.go

@@ -255,13 +255,75 @@ func TestLogsToolPassesArtifactsArgument(t *testing.T) {
 	for i, arg := range capturedArgs {
 		if arg == "--artifacts" {
 			require.Less(t, i+1, len(capturedArgs), "--artifacts should have a value")
-			assert.Equal(t, "agent,firewall", capturedArgs[i+1], "logs tool should join artifact sets for the CLI")
+			assert.Equal(t, "agent,firewall,usage", capturedArgs[i+1], "logs tool should join artifact sets for the CLI and always add the usage set")
 			return
 		}
 	}
 	t.Fatal("expected --artifacts flag in command args")
 }
 
+func TestEffectiveMCPLogsToolArtifacts(t *testing.T) {
+	tests := []struct {
+		name      string
+		artifacts []string
+		expected  []string
+	}{
+		{
+			name:      "omitted artifacts use info and usage defaults",
+			artifacts: nil,
+			expected:  []string{"info", "usage"},
+		},
+		{
+			name:      "usage set is added so token usage stays populated",
+			artifacts: []string{"info"},
+			expected:  []string{"info", "usage"},
+		},
+		{
+			name:      "explicit usage set is preserved as-is",
+			artifacts: []string{"usage"},
+			expected:  []string{"usage"},
+		},
+		{
+			name:      "all set is preserved as-is",
+			artifacts: []string{"all"},
+			expected:  []string{"all"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, effectiveMCPLogsToolArtifacts(tt.artifacts))
+		})
+	}
+}
+
+// TestLogsToolDefaultsToUsageArtifact guards against the regression where the logs
+// tool downloaded only the "info" artifact, leaving token_usage at 0 for every run.
+func TestLogsToolDefaultsToUsageArtifact(t *testing.T) {
+	var capturedArgs []string
+	mockExecCmd := func(ctx context.Context, args ...string) *exec.Cmd {
+		capturedArgs = append([]string(nil), args...)
+		return exec.CommandContext(ctx, "sh", "-c", `printf '%s' "$1"`, "sh", `{"file_path":"/tmp/gh-aw/aw-mcp/logs/runs.json"}`)
+	}
+
+	server := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "1.0"}, nil)
+	err := registerLogsTool(server, mockExecCmd, "", false)
+	require.NoError(t, err, "registerLogsTool should succeed")
+
+	session := connectInMemory(t, server)
+	_, err = session.CallTool(context.Background(), &mcp.CallToolParams{
+		Name:      "logs",
+		Arguments: map[string]any{},
+	})
+	require.NoError(t, err, "logs tool should succeed")
+
+	artifactsIndex := slices.Index(capturedArgs, "--artifacts")
+	require.NotEqual(t, -1, artifactsIndex, "logs tool should pass --artifacts")
+	require.Less(t, artifactsIndex+1, len(capturedArgs), "--artifacts should have a value")
+	assert.Equal(t, "info,usage", capturedArgs[artifactsIndex+1],
+		"logs tool should download the usage artifact by default so token_usage is populated")
+}
+
 func TestLogsToolPassesGradersArgument(t *testing.T) {
 	var capturedArgs []string
 	mockExecCmd := func(ctx context.Context, args ...string) *exec.Cmd {

schemas/logs-jsonl.schema.json

@@ -3691,6 +3691,8 @@
             "workflow_path",
             "status",
             "classification",
+            "token_usage",
+            "aic",
             "created_at",
             "url",
             "logs_path",

schemas/logs.schema.json

@@ -997,6 +997,8 @@
               "workflow_path",
               "status",
               "classification",
+              "token_usage",
+              "aic",
               "created_at",
               "url",
               "logs_path",