diff --git a/docs/adr/46360-fix-safe-items-count-population.md b/docs/adr/46360-fix-safe-items-count-population.md new file mode 100644 index 00000000000..bbcc47e60dd --- /dev/null +++ b/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.* diff --git a/pkg/cli/logs_download.go b/pkg/cli/logs_download.go index 3bc9b8eeb09..85c2c7dac06 100644 --- a/pkg/cli/logs_download.go +++ b/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: "-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 { diff --git a/pkg/cli/logs_download_test.go b/pkg/cli/logs_download_test.go index 85105b0b848..22d7848b713 100644 --- a/pkg/cli/logs_download_test.go +++ b/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: diff --git a/pkg/cli/logs_models.go b/pkg/cli/logs_models.go index 55ff634d21b..5df081f60e3 100644 --- a/pkg/cli/logs_models.go +++ b/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 diff --git a/pkg/cli/logs_run_processor.go b/pkg/cli/logs_run_processor.go index 2db3503a941..ac3a18b1c2a 100644 --- a/pkg/cli/logs_run_processor.go +++ b/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 } diff --git a/pkg/cli/logs_run_processor_test.go b/pkg/cli/logs_run_processor_test.go index e76f907cfe6..9dd5ad90628 100644 --- a/pkg/cli/logs_run_processor_test.go +++ b/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") +} diff --git a/pkg/cli/logs_summary_test.go b/pkg/cli/logs_summary_test.go index 02876d4761c..c4b51cd29b2 100644 --- a/pkg/cli/logs_summary_test.go +++ b/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)) + } +}