fix(logs): total_turns and total_safe_items always 0 in summary#43410
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage — Run §28715668077
Small targeted fix (+7/-1 Go, +9 Go, +137 test lines, 3 files). Low risk. Recommend undraft → expedited review.
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect logs summary totals where total_turns and total_safe_items were reported as 0 despite non-zero values being present in usage activity artifacts and/or cached summaries.
Changes:
- Prevents the orchestrator from overwriting backfilled
Run.Turnswith0when log-derivedMetrics.Turnsis unavailable. - Re-applies usage activity backfill on cache hits to self-heal stale cached
run_summary.jsonentries with zeroTurns/SafeItemsCount. - Adds unit tests covering the cache-hit backfill guard and the intended turns precedence behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_orchestrator.go | Avoids clobbering backfilled Turns with Metrics.Turns==0 in the main orchestrator path. |
| pkg/cli/logs_run_processor.go | On cache hits, re-loads usage activity summary to backfill stale zero values for Turns / SafeItemsCount. |
| pkg/cli/logs_usage_activity_test.go | Adds unit tests for stale-cache backfill and turns precedence rules. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Low
| // result.Metrics.Turns is 0 when only the usage artifact was downloaded | ||
| // (no events.jsonl / .log files), so only overwrite when the log-based | ||
| // count is actually available to avoid discarding the backfilled value. | ||
| if result.Metrics.Turns > 0 { | ||
| run.Turns = result.Metrics.Turns |
There was a problem hiding this comment.
Fixed in the prior commit — applyMetricsTurnsToRun() helper is now called by both DownloadWorkflowLogs and DownloadWorkflowLogsFromStdin, removing the unconditional assignment from the stdin path.
| if result.Run.Turns == 0 || result.Run.SafeItemsCount == 0 { | ||
| if usageActivitySummary, _ := loadUsageActivitySummary(runOutputDir); usageActivitySummary != nil { | ||
| applyUsageActivitySummaryToResult(usageActivitySummary, &result, true) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in the prior commit — backfillCacheHitIfNeeded() now logs a warning via logsOrchestratorLog.Printf when loadUsageActivitySummary returns an error and verbose is true.
| // This mirrors the cache-hit path in logs_run_processor.go: if values are zero, | ||
| // reload the usage activity summary and apply the backfill. | ||
| if result.Run.SafeItemsCount == 0 || result.Run.Turns == 0 { | ||
| if usageActivitySummary, _ := loadUsageActivitySummary(runDir); usageActivitySummary != nil { | ||
| applyUsageActivitySummaryToResult(usageActivitySummary, &result, true) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in the prior commit — backfillCacheHitIfNeeded() was extracted as a package-level helper; tests now call it directly instead of duplicating inline production logic.
| // This mirrors the relevant lines in logs_orchestrator.go: | ||
| // run := result.Run | ||
| // if result.Metrics.Turns > 0 { run.Turns = result.Metrics.Turns } | ||
| run := result.Run | ||
| if result.Metrics.Turns > 0 { | ||
| run.Turns = result.Metrics.Turns | ||
| } |
There was a problem hiding this comment.
Fixed in the prior commit — applyMetricsTurnsToRun() was extracted as a package-level helper; tests now call it directly instead of mirroring the conditional assignment inline.
…nd cache healing ADR-43410 documents the design decisions in PR #43410: - Backfill-wins-when-metrics-is-zero precedence rule in the orchestrator - Lazy cache healing strategy for stale run_summary.json entries
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 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
|
There was a problem hiding this comment.
Review: fix(logs): total_turns and total_safe_items always 0 in summary
The two independent bugs are correctly diagnosed and fixed.
Bug 1 — Orchestrator clobbers backfilled Turns (logs_orchestrator.go): The guard if result.Metrics.Turns > 0 preserves backfilled values when no events.jsonl/.log files were downloaded. This aligns the orchestrator with applyUsageActivitySummaryToResult which already uses result.Run.Turns == 0 as its own guard (line 103).
Bug 2 — Cache-hit path skips backfill (logs_run_processor.go): Re-applying the backfill after loading a cached entry is safe because applyUsageActivitySummaryToResult is idempotent — both the Turns guard (line 103) and SafeItemsCount guard (line 169) are zero-checks, so non-zero values are never overwritten.
Tests: The five new unit tests in logs_usage_activity_test.go give precise coverage of both fix paths and their boundary conditions, and the test logic directly mirrors the production code patterns.
No issues found — correctness, idempotency contract, and guard conditions all check out. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 37.8 AIC · ⌖ 6.04 AIC · ⊞ 4.9K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (11 tests)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on test robustness.
📋 Key Themes & Highlights
Key Themes
- Test-to-production coupling: Four of the five new tests inline the production logic they mean to guard rather than calling through it. If the guard condition (
||vs&&) or the surrounding orchestrator block changes, those tests will silently pass while the real bug reappears. - Silenced error path: The cache-hit backfill uses
_to discard theloadUsageActivitySummaryerror, making it harder to diagnose corruption in the activity summary file. - Missing
||boundary test:TestCacheHitDoesNotOverwriteNonZeroValuesonly tests the fully-non-zero case; it does not verify the partial-zero cases (Turns=0, SafeItemsCount=5and vice versa) that are the most interesting semantics of the||guard.
Positive Highlights
- ✅ Minimal, surgical fixes — each bug is resolved with a single well-placed guard
- ✅ Clear PR description with root-cause analysis for both bugs
- ✅ Five tests added for a 16-line production change — strong intent
- ✅ Self-healing stale-cache design is safe and idempotent (
applyUsageActivitySummaryToResultis already a no-op when values are non-zero) - ✅ Correct precedence: log-derived
Metrics.Turnsstill wins when available
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 79.6 AIC · ⌖ 9.48 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| // This mirrors the cache-hit path in logs_run_processor.go: if values are zero, | ||
| // reload the usage activity summary and apply the backfill. | ||
| if result.Run.SafeItemsCount == 0 || result.Run.Turns == 0 { |
There was a problem hiding this comment.
[/tdd] These cache-hit tests inline the guard condition from logs_run_processor.go instead of calling through the production code path — if the guard changes in production (e.g. || becomes &&), these tests will still pass.\n\n
💡 Suggested improvement
\n\nExtract the cache-hit backfill logic into a package-level helper and call it from both the production code and the tests:\n\ngo\n// logs_run_processor.go\nfunc healStaleCacheEntry(result *DownloadResult, runOutputDir string) {\n if result.Run.Turns == 0 || result.Run.SafeItemsCount == 0 {\n if summary, _ := loadUsageActivitySummary(runOutputDir); summary != nil {\n applyUsageActivitySummaryToResult(summary, result, true)\n }\n }\n}\n\n\nThen the tests call healStaleCacheEntry(&result, runDir) — testing the real guard, not a copy. Guard changes will break tests rather than silently diverge.\n\nThere was a problem hiding this comment.
Addressed in the prior commit — backfillCacheHitIfNeeded() was extracted as a package-level helper and tests call it directly rather than inlining the guard logic.
| } | ||
|
|
||
| // Guard condition is false (both >0), so neither value should change. | ||
| assert.Equal(t, 14, result.Run.Turns, "non-zero cached Turns must not be overwritten by the cache-hit backfill guard") |
There was a problem hiding this comment.
[/tdd] TestCacheHitDoesNotOverwriteNonZeroValues doesn't exercise the case where only one of the two fields is zero — the guard uses ||, so this partial-zero scenario is the critical path.\n\n
💡 Missing test case
\n\nAdd two sub-cases:\n\n1.Turns=0, SafeItemsCount=5 — guard triggers; only Turns gets backfilled, SafeItemsCount stays 5\n2. Turns=14, SafeItemsCount=0 — guard triggers; only SafeItemsCount gets backfilled, Turns stays 14\n\nThese cover the || semantics and would catch a regression if the guard were changed to &&.\n\nThere was a problem hiding this comment.
Added TestCacheHitBackfillsPartialZeroTurns and TestCacheHitBackfillsPartialZeroSafeItemsCount in the latest commit to cover both partial-zero branches of the || guard. Each test verifies that only the zero field gets backfilled while the non-zero field remains unchanged.
| // were saved before safe-outputs or turn backfill was introduced. | ||
| // applyUsageActivitySummaryToResult is a no-op when values are already | ||
| // non-zero, so this is always safe to call. | ||
| if result.Run.Turns == 0 || result.Run.SafeItemsCount == 0 { |
There was a problem hiding this comment.
[/diagnosing-bugs] The guard condition Turns == 0 || SafeItemsCount == 0 re-loads and applies the usage activity summary even when only one value is stale — which is fine — but silently swallows the loadUsageActivitySummary error via _. If the activity summary file is corrupted, the backfill silently skips without logging, making future diagnosis harder.\n\n
💡 Suggested improvement
\n\ngo\nif result.Run.Turns == 0 || result.Run.SafeItemsCount == 0 {\n if usageActivitySummary, err := loadUsageActivitySummary(runOutputDir); err != nil {\n logsRunProcessorLog.Printf("cache-hit backfill: failed to load activity summary: %v", err)\n } else if usageActivitySummary != nil {\n applyUsageActivitySummaryToResult(usageActivitySummary, &result, true)\n }\n}\n\n\nThis preserves the no-op behaviour when the file is absent (err will be a not-found error, already handled) while surfacing corruption.\n\nThere was a problem hiding this comment.
Addressed in the prior commit — backfillCacheHitIfNeeded() now captures the error and logs a warning via logsOrchestratorLog.Printf when verbose is true, consistent with the non-cache path.
| // This mirrors the relevant lines in logs_orchestrator.go: | ||
| // run := result.Run | ||
| // if result.Metrics.Turns > 0 { run.Turns = result.Metrics.Turns } | ||
| run := result.Run |
There was a problem hiding this comment.
[/tdd] TestMetricsTurnsZeroDoesNotOverwriteBackfilledTurns and TestMetricsTurnsNonZeroOverridesBackfilledTurns copy-paste the orchestrator's conditional assignment inline rather than calling into it — the same fragility as the cache-hit tests above.\n\n
💡 Why this matters
\n\nThe orchestrator code block these tests guard spans lines 579–588 oflogs_orchestrator.go. If the assignment block gains additional fields (e.g. AvgTimeBetweenTurns logic changes), the inline test copy won't reflect that.\n\nAs with the cache-hit path, extracting to a helper (e.g. applyMetricsToRun) and testing the helper directly would give stronger coverage with less duplication.\n\nThere was a problem hiding this comment.
Addressed in the prior commit — applyMetricsTurnsToRun() was extracted as a package-level helper and tests call it directly rather than mirroring the conditional assignment inline.
There was a problem hiding this comment.
REQUEST_CHANGES — Bug 1 fix is incomplete; one code path remains broken.
Blocking issues
Missing fix in DownloadWorkflowLogsFromStdin
The PR fixes run.Turns = result.Metrics.Turns → guarded assignment in DownloadWorkflowLogs (~line 585) but leaves the identical line unguarded in DownloadWorkflowLogsFromStdin (~line 1175). The --stdin path will still report total_turns: 0 for usage-only artifact downloads. See inline comment.
Two tests that don't guard the production fix
TestMetricsTurnsZeroDoesNotOverwriteBackfilledTurns and TestMetricsTurnsNonZeroOverridesBackfilledTurns copy-paste the guard condition inline instead of calling through production code. They pass regardless of whether the orchestrator fix is present, providing false confidence. See inline comment for remediation options.
🔎 Code quality review by PR Code Quality Reviewer · 191.3 AIC · ⌖ 5.53 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/logs_orchestrator.go:1175
Same Turns-clobber bug exists in DownloadWorkflowLogsFromStdin: run.Turns = result.Metrics.Turns overwrites any backfilled value with 0, exactly like the bug fixed at line 585.
<details>
<summary>Bug description and suggested fix</summary>
DownloadWorkflowLogsFromStdin (here, line ~1175) is a parallel code path to DownloadWorkflowLogs (fixed at line ~585). The PR applied the guard only to DownloadWorkflowLogs. Any run processed through the stdin URL mode still reports `total_t…
pkg/cli/logs_usage_activity_test.go:458
These two tests exercise inlined logic, not production code: if the guard in logs_orchestrator.go is reverted, both tests still pass.
<details>
<summary>Why this matters</summary>
TestMetricsTurnsZeroDoesNotOverwriteBackfilledTurns and TestMetricsTurnsNonZeroOverridesBackfilledTurns inline the same if result.Metrics.Turns > 0 condition directly in the test body instead of calling through a function that exercises the real production path. They are proofs-of-concept for the logic, …
|
/souschef |
|
✅ PR Sous Chef finished PR sous-chef nudges. processed=5; skipped_checks_running=0; skipped_last_comment_from_sous_chef=0; skipped_cooldown=0; nudged=0; branch_update_attempts=0; formatter_pushes=0; merge_main_scheduled=0 |
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…gging - Add applyMetricsTurnsToRun() helper called by both DownloadWorkflowLogs and DownloadWorkflowLogsFromStdin, fixing the unconditional Turns=0 clobber in the stdin path (mirrors the fix from the regular path) - Add backfillCacheHitIfNeeded() helper that includes verbose error logging for loadUsageActivitySummary failures instead of silently ignoring them - Update tests to call the real helper functions instead of duplicating inline production logic Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Addressed all four review threads in commit
Local |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All unresolved review feedback has been addressed:
Local |
|
🎉 This pull request is included in a new release. Release: |
logstool summary consistently reportstotal_turns: 0andtotal_safe_items: 0even when the underlying artifact data contains non-zero values. Two independent bugs cause this.Bug 1 — Orchestrator clobbers backfilled
Turns(logs_orchestrator.go)applyUsageActivitySummaryToResultcorrectly backfillsresult.Run.Turnsfromactivity/summary.json(e.g.session.turns = 34). The orchestrator then unconditionally overwrites it:Fix: only assign from metrics when the log-derived count is actually available:
Bug 2 — Cache-hit path skips backfill (
logs_run_processor.go)The cache-hit path returned
run_summary.jsonas-is. If a cache entry was written before thesafe_outputs/session.turnsbackfill existed (same CLI version = no invalidation),SafeItemsCountandTurnsstay 0 indefinitely.Fix: after loading from cache, re-apply the backfill when either value is zero.
applyUsageActivitySummaryToResultis a no-op when values are already non-zero, so this is safe unconditionally and self-heals old cache entries.Tests
Five new unit tests cover both fixes and their guard conditions (non-zero cached values are not overwritten; log-derived
Metrics.Turnsstill wins when present).