Skip to content

Fix safe_items_count missing from cached runs when filtering by workflow_name#21491

Merged
pelikhan merged 3 commits intomainfrom
copilot/cli-tools-test-fix-safe-items-count
Mar 18, 2026
Merged

Fix safe_items_count missing from cached runs when filtering by workflow_name#21491
pelikhan merged 3 commits intomainfrom
copilot/cli-tools-test-fix-safe-items-count

Conversation

Copy link
Contributor

Copilot AI commented Mar 18, 2026

When workflow_name filtering is applied, safe_items_count is absent from all run objects and total_safe_items is 0 — even for runs that reported correct counts in unfiltered queries.

Root cause

In downloadRunArtifactsConcurrent, SafeItemsCount is computed and stored on result.Run, but the RunSummary persisted to disk was built with the original run variable (the raw API struct, SafeItemsCount=0):

// Before — saves SafeItemsCount=0 to cache
summary := &RunSummary{
    Run: run,  // raw API run, never had SafeItemsCount set
    ...
}

// After — saves the computed SafeItemsCount
summary := &RunSummary{
    Run: result.Run,  // processed copy with SafeItemsCount set
    ...
}

On first processing (unfiltered, no cache), result.Run.SafeItemsCount was used correctly in-memory for the report. But the cache was written with 0, so any subsequent call — including a workflow_name-filtered query that hit the cached artifacts — read back 0.

Changes

  • pkg/cli/logs_orchestrator.go: Use result.Run instead of run when constructing RunSummary, so SafeItemsCount is correctly persisted to the on-disk cache.
  • pkg/cli/logs_summary_test.go: Add TestSaveAndLoadRunSummary_SafeItemsCount as a regression test verifying the field round-trips through cache save/load.

💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.



✨ PR Review Safe Output Test - Run 23223776723

💥 [THE END] — Illustrated by Smoke Claude ·

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] [cli-tools-test] Fix missing safe_items_count in workflow_name filtered logs Fix safe_items_count missing from cached runs when filtering by workflow_name Mar 18, 2026
Copilot AI requested a review from pelikhan March 18, 2026 00:58
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💥 Automated smoke test review - all systems nominal!

💥 [THE END] — Illustrated by Smoke Claude


processedRun := ProcessedRun{
Run: run,
Run: result.Run,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix! Using result.Run instead of the raw run ensures SafeItemsCount (computed during artifact extraction) is persisted to the cache. The original code was silently discarding the computed value.

if loaded.Run.SafeItemsCount != 4 {
t.Errorf("SafeItemsCount not persisted: got %d, want 4", loaded.Run.SafeItemsCount)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid regression test! The test clearly documents the bug and verifies the fix by round-tripping through save/load. The SafeItemsCount: 4 value makes the assertion easy to follow.

@pelikhan pelikhan marked this pull request as ready for review March 18, 2026 01:31
Copilot AI review requested due to automatic review settings March 18, 2026 01:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a caching bug where SafeItemsCount was computed in-memory but not persisted into run_summary.json, causing subsequent cached/filtered (workflow_name) reads to show safe_items_count=0.

Changes:

  • Persist the computed SafeItemsCount into cached RunSummary by saving result.Run (processed) instead of the raw API run.
  • Add a regression unit test to ensure SafeItemsCount round-trips through saveRunSummary/loadRunSummary.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
pkg/cli/logs_orchestrator.go Writes processed WorkflowRun (with SafeItemsCount) into the on-disk run summary cache.
pkg/cli/logs_summary_test.go Adds a regression test ensuring SafeItemsCount is persisted and reloaded correctly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.


processedRun := ProcessedRun{
Run: run,
Run: result.Run,
@pelikhan
Copy link
Contributor

@copilot Review comments

…y using result.Run

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 18, 2026

@copilot Review comments

Fixed in 5b7e13b. The sed command accidentally changed both occurrences of Run: run. The ProcessedRun (in the outer DownloadWorkflowLogs loop) is now back to using run — the fully-enriched local copy with LogsPath, Duration, TokenUsage, etc. Only the RunSummary (inside the goroutine) uses result.Run, which is where the SafeItemsCount is stored.

@pelikhan pelikhan merged commit c98a911 into main Mar 18, 2026
53 checks passed
@pelikhan pelikhan deleted the copilot/cli-tools-test-fix-safe-items-count branch March 18, 2026 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[cli-tools-test] Bug: safe_items_count missing and total_safe_items is 0 when filtering logs by workflow_name

3 participants