fix: populate SafeItemsCount in run_summary.json so audits report accurate safe-output write counts#46360
Conversation
🤖 PR Triage
Rationale: WIP draft with no files committed yet. Cannot assess scope or risk. Defer until ready for review.
|
…tput write metrics Three complementary fixes: 1. Add json:"safe_items_count,omitempty" tag to WorkflowRun.SafeItemsCount so it serializes with the snake_case key expected by downstream audit tools. 2. Add flattenSafeOutputsItemsArtifact() to move safe-output-items.jsonl and temporary-id-map.json from safe-outputs-items/ subdirectory to the run root where extractCreatedItemsFromManifest and loadResolvedTemporaryIDTargets look. 3. Persist healed SafeItemsCount back to run_summary.json after cache-hit backfill so downstream readers see the correct count without falling back to activity summary. Closes #46268 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes safe-output counts in cached run summaries and downloaded artifacts.
Changes:
- Adds the
safe_items_countJSON key. - Flattens safe-output artifacts and persists backfilled counts.
- Adds regression tests and changes the auto-upgrade schedule.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_models.go |
Adds JSON serialization metadata. |
pkg/cli/logs_download.go |
Flattens safe-output artifacts. |
pkg/cli/logs_download_test.go |
Tests artifact flattening. |
pkg/cli/logs_run_processor.go |
Persists backfilled counts. |
pkg/cli/logs_run_processor_test.go |
Tests cache persistence. |
pkg/cli/logs_summary_test.go |
Tests the JSON key. |
.github/workflows/agentic-auto-upgrade.yml |
Changes the weekly schedule. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Medium
| // 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 { |
| on: | ||
| schedule: | ||
| - cron: "11 4 * * 6" # Weekly (auto-upgrade) | ||
| - cron: "21 3 * * 5" # Weekly (auto-upgrade) |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
The three-bug diagnosis is accurate and the fixes are well-targeted. Tests are thorough.
Existing comments already flag:
- The
agentic-auto-upgrade.ymlschedule change appears unrelated to this fix; worth verifying it's intentional or generated. flattenSafeOutputsItemsArtifactonly helps fresh downloads; the stale-cache path bypassesdownloadRunArtifacts. The persistence fix inlogs_run_processor.goaddresses that correctly — but only if the artifact files are already at the expected path on disk.
Additional note on omitempty: SafeItemsCount int with json:"safe_items_count,omitempty" means a true zero is omitted from JSON, making a "never computed" entry and an "explicitly zero safe-outputs run" indistinguishable. Benign for current reads (both default to 0), but worth noting for future tooling.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 19.7 AIC · ⌖ 4.33 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — three independent root causes, each fixed surgically with regression tests. This is a well-diagnosed and cleanly executed bug fix.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Each root cause is independently identified with a clear before/after in the PR description
- ✅ Test coverage is thorough: JSON key contract, artifact flattening (present + absent), and cache-persist healing are all verified
- ✅
flattenSafeOutputsItemsArtifactmirrors the existingflattenActivationArtifactpattern — consistent codebase design - ✅ The heal-persistence guard (
SafeItemsCount != safeItemsBefore) is conservative and correct givenbackfillCacheHitIfNeeded's own guard
One Minor Note
SafeItemsCount now uses omitempty, which means runs with genuinely 0 safe outputs will omit the key from JSON entirely. Downstream readers that can't fall back would interpret absence as 0, which is behaviorally equivalent here — but worth keeping in mind if the field is later queried with an existence check rather than a null-safe read.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 24.5 AIC · ⌖ 4.36 AIC · ⊞ 6.7K
Comment /matt to run again
🧪 Test Quality Sentinel Report
📊 Metrics (4 tests)
|
| File | Test Lines | Prod Lines | Ratio | Justification |
|---|---|---|---|---|
| logs_download_test.go | +71 | +25 | 2.84:1 | Artifact I/O requires verification of file placement + content + cleanup |
| logs_run_processor_test.go | +41 | +14 | 2.93:1 | Disk persistence & backfill healing requires cross-layer verification |
| logs_summary_test.go | +26 | +1 | 26:1 | JSON serialization contract (single tag change) needs exhaustive testing |
Assessment: While ratios exceed 2:1, they are justified by:
- File I/O operations requiring explicit path and content verification
- Cross-layer testing (memory → disk → reload) for cache persistence
- Contract tests for JSON key naming (critical for downstream audit tools)
Inflation is acceptable given the nature of the changes.
Verdict
✅ Passed. 0% implementation tests (threshold: 30%).
Test design is strong. All 4 tests enforce genuine behavioral contracts:
- Artifact flattening — files discoverable at run root after download
- Graceful missing artifact — no errors when optional files absent
- Disk persistence — cached SafeItemsCount healed and persisted to disk
- JSON serialization — snake_case key for downstream audit tool compatibility
No mock libraries, no missing build tags, edge cases covered. The test-to-code ratios are high but justified by the complexity of file I/O and cross-layer validation.
Sentinel: Go tests passed structural validation. No blocking issues detected.
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.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
🧪 Test quality analysis by Test Quality Sentinel · 16.9 AIC · ⌖ 8.92 AIC · ⊞ 7K · ◷
Comment /review to run again
There was a problem hiding this comment.
Three correctness issues need fixing before merge.
Blocking findings
1. omitempty defeats the JSON fix (logs_models.go:80)
Adding json:"safe_items_count,omitempty" corrects the key name but breaks serialisation of zero — omitempty on int drops the field entirely when it is 0. Runs that produce no safe outputs will serialize without the key, reload as 0, and trigger the backfill on every cache hit indefinitely. Remove omitempty.
2. Over-broad heal guard can overwrite correct persisted values (logs_run_processor.go:164)
The guard result.Run.SafeItemsCount != safeItemsBefore fires when any change occurs, not only on zero→non-zero transitions. If the stored count was already non-zero and the backfill computes a different value, this silently replaces the authoritative persisted value. The guard should be safeItemsBefore == 0 && result.Run.SafeItemsCount != 0.
3. Heal write silently persists Metrics as a side-effect (logs_run_processor.go:167)
healed.Metrics = result.Metrics commits whatever token-usage state backfillCacheHitIfNeeded wrote, regardless of whether that data is better or worse than what was already on disk. This is undocumented, unguarded, and can regress previously-correct Metrics.
🔎 Code quality review by PR Code Quality Reviewer · 53.1 AIC · ⌖ 4.82 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/logs_models.go:80
omitempty on an int field silently drops safe_items_count from JSON when zero, making legitimate zero-item runs indistinguishable from stale cache entries — triggering unnecessary backfill I/O on every subsequent cache hit.
<details>
<summary>💡 Suggested fix</summary>
omitempty on int omits the key entirely when the value is 0. Any run that produced zero safe-output items serializes without safe_items_count; on reload the field deserializes back to 0, which is the exact s…
pkg/cli/logs_run_processor.go:164
The heal guard fires on any change to SafeItemsCount, not just the zero→non-zero transition the comment describes, risking silent overwrites of previously-correct persisted values.
<details>
<summary>💡 Detail and fix</summary>
The inline comment at line 157 explicitly documents the intent as "it was 0 before and is now non-zero", but the guard is result.Run.SafeItemsCount != safeItemsBefore. This also fires when the stored value was already non-zero (e.g., 5) and `backfillCacheHitIfN…
pkg/cli/logs_run_processor.go:167
healed.Metrics = result.Metrics silently persists backfilled token-usage data as a side-effect of the SafeItemsCount heal, potentially overwriting a previously-authoritative Metrics value with an inferred estimate.
<details>
<summary>💡 Detail</summary>
backfillCacheHitIfNeeded can modify result.Metrics (via backfillRunTokenUsageFromFirewall) as a side-effect. The heal write here is triggered solely by a SafeItemsCount change, but unconditionally commits whatever state `resu…
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (177 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot run pr-finisher skill |
|
🎉 This pull request is included in a new release. Release: |
run_summary.jsonalways wroteSafeItemsCount: 0, causing daily audits (API Consumption, Safe Output Health) to fall back tosummary.json → safe_outputs.total_items. Three independent bugs converged to produce this:Root causes & fixes
JSON key mismatch (
logs_models.go):WorkflowRun.SafeItemsCounthad no JSON tag → marshaled as"SafeItemsCount"(PascalCase). Downstream reads"safe_items_count"(snake_case) → field was always absent.Artifact not flattened (
logs_download.go):safe-outputs-itemscontains two files (safe-output-items.jsonl+temporary-id-map.json), soflattenSingleFileArtifactsskipped it.extractCreatedItemsFromManifestlooks at the run root, never finding either file. AddedflattenSafeOutputsItemsArtifact()mirroring the existingflattenActivationArtifactpattern.Cache persistence gap (
logs_run_processor.go):backfillCacheHitIfNeededhealedSafeItemsCountin-memory but never calledsaveRunSummary. The on-diskrun_summary.jsonstayed at 0 for all cached runs. Now detects when the backfill changed the value and re-persists.Tests added
TestSafeItemsCountJSONKey— asserts"safe_items_count"is present in marshaled JSON (not"SafeItemsCount")TestFlattenSafeOutputsItemsArtifact/TestFlattenSafeOutputsItemsArtifactMissing— flatten moves both files to run root; no-op when artifact absentTestTryLoadCachedRunResultPersistsSafeItemsCountAfterBackfill— on-diskrun_summary.jsonreflects the healed count after a cache hit