fix: populate SafeItemsCount in run_summary.json so audits report accurate safe-output write counts

docs/adr/46360-fix-safe-items-count-population.md

@@ -0,0 +1,55 @@
+# ADR-46360: Fix SafeItemsCount Population in run_summary.json
+
+**Date**: 2026-07-18
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+Daily audit workflows (API Consumption, Safe Output Health) read `SafeItemsCount` from `run_summary.json` to measure how many safe-output items an agent run actually wrote to GitHub. Three independent bugs caused this field to always be `0`, forcing audits to fall back to `usage/activity/summary.json → safe_outputs.total_items`. This fallback masked the root cause and made audit data less reliable. The bugs were: (1) `WorkflowRun.SafeItemsCount` lacked a `json:"safe_items_count"` tag so it marshaled as PascalCase while readers expected snake_case; (2) the `safe-outputs-items` artifact was skipped by `flattenSingleFileArtifacts` because it contains two files, so `extractCreatedItemsFromManifest` never found either; (3) `backfillCacheHitIfNeeded` healed `SafeItemsCount` in memory but never called `saveRunSummary`, leaving the on-disk file stale for cached runs.
+
+### Decision
+
+We will fix all three root causes simultaneously: add the missing JSON struct tag to `WorkflowRun.SafeItemsCount`; introduce `flattenSafeOutputsItemsArtifact()` following the existing `flattenActivationArtifact` / `flattenAgentOutputsArtifact` pattern to move both files to the run root; and detect when `backfillCacheHitIfNeeded` changes `SafeItemsCount` (via snapshot-and-compare) and call `saveRunSummary` to persist the healed value. All three fixes are required together because any single fix leaves the other failure paths open.
+
+### Alternatives Considered
+
+#### Alternative 1: Treat usage/activity/summary.json as the authoritative source
+
+Rather than fixing `run_summary.json` population, audit tools could always read `SafeItemsCount` from `usage/activity/summary.json`. This eliminates the need to flatten artifacts or persist the backfill value.
+
+This was not chosen because it centralises audit logic on a different data path, requires every downstream consumer to know about the fallback, and obscures the signal that `run_summary.json` is incomplete. The existing fallback was already a workaround; making it permanent would entrench technical debt.
+
+#### Alternative 2: Compute SafeItemsCount from raw JSONL at query time
+
+Each report could count lines in `safe-output-items.jsonl` directly instead of relying on pre-computed fields in `run_summary.json`. This avoids JSON tagging and caching problems.
+
+This was not chosen because it breaks the existing contract where `run_summary.json` is a self-contained, fully resolved snapshot of a run's metrics. Recomputing at query time duplicates parsing logic across consumers and increases I/O for each report run. The bugs are well-understood and fixable at the source.
+
+#### Alternative 3: Add a dedicated artifact-search step instead of flattening
+
+Instead of flattening the `safe-outputs-items` subdirectory, `extractCreatedItemsFromManifest` could be updated to search one level deep for the relevant files.
+
+This was not chosen because the rest of the codebase consistently uses the flatten-to-root pattern for all multi-file artifacts (`activation`, `agent_outputs`). Changing `extractCreatedItemsFromManifest`'s search semantics would be a broader, riskier change and would deviate from the established convention.
+
+### Consequences
+
+#### Positive
+- `run_summary.json` now contains accurate `SafeItemsCount` values, eliminating the need for the audit fallback path.
+- `flattenSafeOutputsItemsArtifact()` follows the established pattern for artifact flattening, making the codebase consistent.
+- Cache hits that previously left `SafeItemsCount = 0` on disk now produce a correct, persisted value for downstream readers.
+- Three targeted tests lock in the correct behavior and prevent regression.
+
+#### Negative
+- The `omitempty` option on the JSON tag means runs with zero safe outputs emit no `safe_items_count` key; readers that do not handle the absent-key case gracefully may interpret absence as unknown rather than zero.
+- The snapshot-and-compare approach in `tryLoadCachedRunResult` triggers an additional `saveRunSummary` disk write for every cache hit where `SafeItemsCount` changes; this is a one-time write per affected run but introduces a new I/O path in the hot cache-load code path.
+
+#### Neutral
+- The `flattenSafeOutputsItemsArtifact` function mirrors `flattenActivationArtifact` and `flattenAgentOutputsArtifact` — consistent pattern, but the number of special-cased flatten helpers continues to grow; a future refactor may want to unify them.
+- Existing audit code that uses the `usage/activity/summary.json` fallback remains in place as a safety net; it is now dead code for correctly processed runs but still executes for older cached runs that have not been re-processed.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/logs_download.go

@@ -304,6 +304,23 @@ func flattenAgentOutputsArtifact(outputDir string, verbose bool) error {
 	return flattenArtifactTree(agentOutputsDir, agentOutputsDir, outputDir, "agent_outputs artifact", verbose)
 }
 
+// flattenSafeOutputsItemsArtifact flattens the safe-outputs-items artifact directory
+// structure. The safe-outputs-items artifact contains safe-output-items.jsonl and
+// temporary-id-map.json. After flattening, these files land at the run directory root
+// where extractCreatedItemsFromManifest and loadResolvedTemporaryIDTargets expect them.
+// The artifact may be prefixed in workflow_call context: "<hash>-safe-outputs-items".
+func flattenSafeOutputsItemsArtifact(outputDir string, verbose bool) error {
+	safeOutputsItemsDir := findArtifactDir(outputDir, constants.SafeOutputItemsArtifactName, "")
+	if safeOutputsItemsDir == "" {
+		// No safe-outputs-items artifact, nothing to flatten
+		return nil
+	}
+
+	logsDownloadLog.Printf("Flattening safe-outputs-items artifact directory: %s", safeOutputsItemsDir)
+
+	return flattenArtifactTree(safeOutputsItemsDir, safeOutputsItemsDir, outputDir, "safe-outputs-items artifact", verbose)
+}
+
 // downloadWorkflowRunLogs downloads and unzips workflow run logs using GitHub API
 func downloadWorkflowRunLogs(ctx context.Context, runID int64, outputDir string, verbose bool, owner, repo, hostname string) error {
 	logsDownloadLog.Printf("Downloading workflow run logs: run_id=%d, output_dir=%s, owner=%s, repo=%s", runID, outputDir, owner, repo)
@@ -930,6 +947,14 @@ func downloadRunArtifacts(ctx context.Context, opts downloadArtifactsOptions) er
 		return fmt.Errorf("failed to flatten agent_outputs artifact: %w", err)
 	}
 
+	// Flatten safe-outputs-items artifact if present.
+	// This artifact contains safe-output-items.jsonl and temporary-id-map.json.
+	// Flattening moves them to the run root so extractCreatedItemsFromManifest
+	// and loadResolvedTemporaryIDTargets can find them at their expected paths.
+	if err := flattenSafeOutputsItemsArtifact(opts.outputDir, opts.verbose); err != nil {
+		return fmt.Errorf("failed to flatten safe-outputs-items artifact: %w", err)
+	}
+
 	// Download and unzip workflow run logs unless caller requested usage-only mode.
 	if !isUsageOnlyArtifactFilter(opts.artifactFilter) {
 		if err := downloadWorkflowRunLogs(ctx, opts.runID, opts.outputDir, opts.verbose, opts.owner, opts.repo, opts.hostname); err != nil {

pkg/cli/logs_download_test.go

@@ -890,7 +890,76 @@ func TestFlattenActivationArtifact(t *testing.T) {
 	}
 }
 
-// TestCountParameterBehavior verifies that the count parameter limits matching results
+// TestFlattenSafeOutputsItemsArtifact verifies that the safe-outputs-items artifact
+// directory is correctly flattened so extractCreatedItemsFromManifest and
+// loadResolvedTemporaryIDTargets can find their files at the run directory root.
+// The artifact contains safe-output-items.jsonl and temporary-id-map.json.
+func TestFlattenSafeOutputsItemsArtifact(t *testing.T) {
+	tmpDir := testutil.TempDir(t, "test-flatten-safe-outputs-items-*")
+
+	// Simulate the directory structure created by `gh run download` for the safe-outputs-items artifact.
+	safeOutputsDir := filepath.Join(tmpDir, "safe-outputs-items")
+	if err := os.MkdirAll(safeOutputsDir, 0o755); err != nil {
+		t.Fatalf("Failed to create safe-outputs-items dir: %v", err)
+	}
+
+	manifestContent := `{"id":"item1","safe":true}` + "\n"
+	if err := os.WriteFile(filepath.Join(safeOutputsDir, "safe-output-items.jsonl"), []byte(manifestContent), 0o644); err != nil {
+		t.Fatalf("Failed to create safe-output-items.jsonl: %v", err)
+	}
+	mapContent := `{"map":{"tmp-1":"real-1"}}`
+	if err := os.WriteFile(filepath.Join(safeOutputsDir, "temporary-id-map.json"), []byte(mapContent), 0o644); err != nil {
+		t.Fatalf("Failed to create temporary-id-map.json: %v", err)
+	}
+
+	if err := flattenSafeOutputsItemsArtifact(tmpDir, false); err != nil {
+		t.Fatalf("flattenSafeOutputsItemsArtifact failed: %v", err)
+	}
+
+	// safe-output-items.jsonl must be at the root for extractCreatedItemsFromManifest.
+	manifestPath := filepath.Join(tmpDir, "safe-output-items.jsonl")
+	if !fileutil.FileExists(manifestPath) {
+		t.Error("safe-output-items.jsonl should be at the root output directory after flattening")
+	} else {
+		content, err := os.ReadFile(manifestPath)
+		if err != nil {
+			t.Fatalf("Failed to read safe-output-items.jsonl: %v", err)
+		}
+		if string(content) != manifestContent {
+			t.Errorf("safe-output-items.jsonl content mismatch: got %q, want %q", string(content), manifestContent)
+		}
+	}
+
+	// temporary-id-map.json must also be at the root for loadResolvedTemporaryIDTargets.
+	mapPath := filepath.Join(tmpDir, "temporary-id-map.json")
+	if !fileutil.FileExists(mapPath) {
+		t.Error("temporary-id-map.json should be at the root output directory after flattening")
+	} else {
+		content, err := os.ReadFile(mapPath)
+		if err != nil {
+			t.Fatalf("Failed to read temporary-id-map.json: %v", err)
+		}
+		if string(content) != mapContent {
+			t.Errorf("temporary-id-map.json content mismatch: got %q, want %q", string(content), mapContent)
+		}
+	}
+
+	// The safe-outputs-items/ subdirectory should have been removed.
+	if fileutil.DirExists(filepath.Join(tmpDir, "safe-outputs-items")) {
+		t.Error("safe-outputs-items/ directory should have been removed after flattening")
+	}
+}
+
+// TestFlattenSafeOutputsItemsArtifactMissing verifies that flattenSafeOutputsItemsArtifact
+// is a no-op (returns nil) when no safe-outputs-items artifact directory is present.
+func TestFlattenSafeOutputsItemsArtifactMissing(t *testing.T) {
+	tmpDir := testutil.TempDir(t, "test-flatten-safe-outputs-items-missing-*")
+
+	if err := flattenSafeOutputsItemsArtifact(tmpDir, false); err != nil {
+		t.Errorf("flattenSafeOutputsItemsArtifact should return nil when artifact is absent, got: %v", err)
+	}
+}
+
 // not the number of runs fetched when date filters are specified
 func TestCountParameterBehavior(t *testing.T) {
 	// This test documents the expected behavior:

pkg/cli/logs_models.go

@@ -77,7 +77,7 @@ type WorkflowRun struct {
 	MissingToolCount    int
 	MissingDataCount    int
 	NoopCount           int
-	SafeItemsCount      int
+	SafeItemsCount      int           `json:"safe_items_count,omitempty"` // Count of safe-output items actually written to GitHub
 	EffectiveTokens     int           // Cost-normalized token count computed from per-model multipliers
 	AvgTimeBetweenTurns time.Duration // Average time between consecutive LLM API calls (from per-turn timestamps when available)
 	LogsPath            string

pkg/cli/logs_run_processor.go

@@ -308,7 +308,21 @@ func tryLoadCachedRunResult(
 		Cached:                  true,
 	}
 	// Re-apply the usage activity backfill to heal stale cache entries.
+	// Capture the SafeItemsCount before backfill to detect whether the field was healed.
+	safeItemsBefore := result.Run.SafeItemsCount
 	backfillCacheHitIfNeeded(&result, runOutputDir, params.verbose)
+	// If the backfill populated SafeItemsCount (i.e. it was 0 before and is now non-zero),
+	// persist the healed value back to run_summary.json so downstream readers (e.g.
+	// the api-consumption-report) see the correct count without having to fall back to
+	// usage/activity/summary.json.
+	if result.Run.SafeItemsCount != safeItemsBefore {
+		healed := *summary
+		healed.Run = result.Run
+		healed.Metrics = result.Metrics
+		if err := saveRunSummary(runOutputDir, &healed, params.verbose); err != nil {
+			logsOrchestratorLog.Printf("Warning: failed to persist healed run summary for run %d: %v", result.Run.DatabaseID, err)
+		}
+	}
 	return &result, true
 }
 

pkg/cli/logs_run_processor_test.go

@@ -179,3 +179,44 @@ func TestTryLoadCachedRunResultUsesCacheWhenEvalsNotRequested(t *testing.T) {
 	require.NotNil(t, result)
 	assert.True(t, result.Cached)
 }
+
+// TestTryLoadCachedRunResultPersistsSafeItemsCountAfterBackfill verifies that when
+// tryLoadCachedRunResult heals a stale SafeItemsCount (0 → N) via backfillCacheHitIfNeeded,
+// the healed value is written back to run_summary.json on disk so downstream readers
+// (e.g. api-consumption-report) see the correct count without falling back to the
+// activity summary.
+func TestTryLoadCachedRunResultPersistsSafeItemsCountAfterBackfill(t *testing.T) {
+	runOutputDir := t.TempDir()
+
+	// Write a run_summary.json with SafeItemsCount=0 (stale cache).
+	summary := &RunSummary{
+		CLIVersion:  GetVersion(),
+		RunID:       200,
+		ProcessedAt: time.Now(),
+		Run: WorkflowRun{
+			DatabaseID:     200,
+			SafeItemsCount: 0,
+		},
+	}
+	require.NoError(t, saveRunSummary(runOutputDir, summary, false))
+
+	// Write a usage/activity/summary.json so backfill has something to pull from.
+	activityPath := filepath.Join(runOutputDir, "usage", "activity", "summary.json")
+	require.NoError(t, os.MkdirAll(filepath.Dir(activityPath), 0o755))
+	require.NoError(t, os.WriteFile(activityPath, []byte(`{
+		"schema":"usage-activity-summary/v1",
+		"safe_outputs":{"total_items":5,"items_by_type":{"create_issue":5}}
+	}`), 0o644))
+
+	result, ok := tryLoadCachedRunResult(context.Background(), WorkflowRun{DatabaseID: 200}, runOutputDir, concurrentRunDownloadParams{})
+	require.True(t, ok)
+	require.NotNil(t, result)
+
+	// In-memory value should be healed.
+	assert.Equal(t, 5, result.Run.SafeItemsCount, "in-memory SafeItemsCount should be backfilled")
+
+	// The on-disk run_summary.json must also reflect the healed value.
+	reloaded, ok := loadRunSummary(runOutputDir, false)
+	require.True(t, ok)
+	assert.Equal(t, 5, reloaded.Run.SafeItemsCount, "on-disk run_summary.json SafeItemsCount should be persisted after backfill")
+}

pkg/cli/logs_summary_test.go

@@ -395,3 +395,29 @@ func TestSaveAndLoadRunSummary_SafeItemsCount(t *testing.T) {
 		t.Errorf("SafeItemsCount not persisted: got %d, want 4", loaded.Run.SafeItemsCount)
 	}
 }
+
+// TestSafeItemsCountJSONKey verifies that WorkflowRun.SafeItemsCount serializes to
+// "safe_items_count" (snake_case) in JSON so downstream audit tools (e.g. api-consumption-
+// report) can read it directly from run_summary.json without a fallback.
+func TestSafeItemsCountJSONKey(t *testing.T) {
+	run := WorkflowRun{SafeItemsCount: 7}
+	data, err := json.Marshal(run)
+	if err != nil {
+		t.Fatalf("json.Marshal failed: %v", err)
+	}
+	// The JSON must contain the snake_case key, not the Go field name.
+	var m map[string]any
+	if err := json.Unmarshal(data, &m); err != nil {
+		t.Fatalf("json.Unmarshal failed: %v", err)
+	}
+	val, ok := m["safe_items_count"]
+	if !ok {
+		t.Errorf("expected JSON key 'safe_items_count' not found in %s", string(data))
+	}
+	if v, _ := val.(float64); int(v) != 7 {
+		t.Errorf("safe_items_count = %v, want 7", val)
+	}
+	if _, hasPascal := m["SafeItemsCount"]; hasPascal {
+		t.Errorf("unexpected PascalCase key 'SafeItemsCount' found in %s; must use snake_case", string(data))
+	}
+}