Fix gh aw logs discarding all processed runs due to variable shadowing - #49332
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw logs discarding all processed runs due to variable shadowing
PR TriageCategory: bug (log-collection fix) Rationale: PR is marked Labels applied:
|
There was a problem hiding this comment.
Pull request overview
Fixes gh aw logs dropping processed runs because of loop-scoped variable shadowing.
Changes:
- Preserves accumulated runs across batches.
- Adds injectable test seams and a regression test.
- Adds a patch changeset.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_orchestrator_download.go |
Corrects batch-result assignment. |
pkg/cli/logs_orchestrator_unit_test.go |
Verifies multi-batch accumulation. |
.changeset/fix-logs-processed-runs-shadowing.md |
Documents the user-facing fix. |
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: 0
- Review effort level: Balanced
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49332 does not have the implementation label and has only 59 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
Ran the
Remaining follow-up before merge: let the current |
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The fix is correct and the approach is clean.
- The root cause (
:=shadowing the outerprocessedRunsaccumulator) is accurately identified and the fix (pre-declare + plain=) is the right minimal change. - The test seam pattern (
logsFetchWorkflowRunBatch/logsProcessWorkflowRunBatchpackage vars witht.Cleanuprestore) follows the existing convention and the regression test faithfully reproduces the bug. batchTimedOutrename cleanly resolves the name collision.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 12.9 AIC · ⊞ 5.3K
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🎉 Great work! This PR looks ready for review. The variable shadowing bug is clearly identified and fixed, with a well-designed regression test that validates the batch accumulation across loop iterations. The changeset documentation is precise, and the refactoring (introducing test seams via A few observations for maintainers:
The PR is small, focused, and ready to merge.
|
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 86/100 — Excellent
📊 Metrics (7 tests)
|
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Approve — no blocking issues
Correct, minimal, well-tested fix for the := shadowing bug that discarded processedRuns on every batch.
Review notes
- Fix: pre-declaring
batchProcessed,allRunsConsumed,batchTimedOutand assigning with=correctly restores accumulation into the outerprocessedRuns. Verified no other:=shadowing of this pattern exists elsewhere in the file. - Test seam:
logsFetchWorkflowRunBatch/logsProcessWorkflowRunBatchpackage-level var indirection follows the existingforecastListWorkflowRunsPaginatedconvention in this package — consistent style, no other direct callers offetchWorkflowRunBatch/processWorkflowRunBatchbypass it. - Regression test
TestCollectProcessedWorkflowRunsAccumulatesBatchesreproduces the bug pre-fix and passes post-fix. Not.Parallel()in this file, so the shared package-level var swap-and-restore viat.Cleanupis safe from cross-test races. - Renaming
timedOuttobatchTimedOutavoids the naming collision with the outer-scopetimedOut, reducing future shadowing risk in this same function.
A background grumpy-coder sub-agent pass and my own independent review of the diff found no additional correctness, concurrency, or maintainability issues within the changed lines.
🔎 Code quality review by PR Code Quality Reviewer · auto · 54.7 AIC · ⊞ 7.8K
Comment /review to run again
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving. The root cause is correctly identified and fixed, and the regression test faithfully reproduces the original failure.
📋 Key Themes & Highlights
Key Themes
- Root cause fixed, not just the symptom — the
:=shadowing is eliminated by pre-declaring variables before the loop and using=assignment - Regression test covers the exact failure mode —
TestCollectProcessedWorkflowRunsAccumulatesBatcheswould have caught this before the bad code shipped - Test seams follow the established
forecastListWorkflowRunsPaginatedpattern — consistent and easy to navigate
Positive Highlights
- ✅ Informative inline comment explaining why
=is used (not:=) — prevents future contributors from reintroducing the bug - ✅
batchTimedOutrename avoids a collision with the outertimedOut; clearer scope separation - ✅ Changeset entry is accurate and user-facing
- ✅ Test uses
t.Cleanupto restore the package-level vars — safe for parallel future tests
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.7 AIC · ⊞ 7K
Comment /matt to run again
|
🎉 This pull request is included in a new release. Release: |
The daily token-audit workflow reported zero runs despite
gh aw logsvisibly downloading artifacts for ~100 runs over 7 minutes. The collection window was fine —gh aw logswas silently dropping every processed run.In
collectProcessedWorkflowRuns, the batch results were assigned with:=inside theforbody. Since the loop body is its own scope, this declared a newprocessedRunsthat shadowed the function-level accumulator, so each batch's results were discarded at the end of the iteration:The knock-on effects:
len(processedRuns)stayed at 0, so the count cap never tripped and the loop paginated through every available run, then exited viahandleEmptyProcessedRunswithNo workflow runs with artifacts found matching the specified criteriaand.runs == []in JSON output.Changes
pkg/cli/logs_orchestrator_download.go— pre-declare the batch outputs and assign with=so runs accumulate across iterations. The batch flag is renamed tobatchTimedOutto avoid colliding with thetimedOutalready declared earlier in the same block.logsFetchWorkflowRunBatchandlogsProcessWorkflowRunBatchpackage vars, following the existingforecastListWorkflowRunsPaginatedpattern, so the collection loop can be exercised without the GitHub API.pkg/cli/logs_orchestrator_unit_test.go—TestCollectProcessedWorkflowRunsAccumulatesBatchesdrives two batches through the loop and asserts all three runs survive. It reproduces the bug on the pre-fix code (should have 3 item(s), but has 0)..github/workflows/agentic-token-audit.mdis upstream-managed (source: githubnext/agentic-ops@…) and is deliberately untouched; the defect was in the CLI.Worth noting for reviewers: the skip paths in
shouldSkipProcessedWorkflowRunare verbose-gated, which is why the failure surfaced as a plausible-looking "empty window" rather than an error. No linter in.golangci.ymlcurrently catches this class of shadowing.Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.