Split long CLI logs/reporting functions into focused helpers - #53279
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors oversized CLI log collection and reporting functions into focused helpers while preserving behavior and output.
Changes:
- Splits artifact download, recovery, and fallback logic into helpers.
- Extracts run aggregation and report section construction.
- Separates MCP server health processing and finalization.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_download.go |
Decomposes artifact and workflow-log download flows. |
pkg/cli/logs_report.go |
Extracts run conversion, aggregation, and report sections. |
pkg/cli/audit_expanded.go |
Splits MCP server health construction into helpers. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
|
✅ Ponytail Reviewer completed successfully! Ponytail review: this is a lint-driven (golint-custom 60-line limit), behavior-preserving refactor. Every extracted helper (resolveCachedArtifacts, planArtifactDownload, enumerateDownloadableArtifacts, classifyBulkDownloadError, recoverBulkDownloadArtifacts, logsAggregate methods, buildRunData/newRunData, appendMCPServerDetails, etc.) exists to keep its parent function under the 60-line cap or to dedupe genuine repeated logic (e.g. downloadWorkflowRunLogsForDiagnostics merges two duplicated blocks). No speculative abstractions, unused flexibility, or hand-rolled stdlib equivalents found. Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ 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.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES
This refactor is mostly mechanical, but it introduced a real behavior change in artifact handling: a usage-only request can now return ErrNoArtifacts for runs that contain only non-downloadable .dockerbuild artifacts, even when the requested usage artifact is already present on disk. That turns a cache hit into a hard failure.
Blocking themes
- Cached-artifact short-circuiting now depends on
findMissingFilterEntries, but the later individual-download path still treats an emptydownloadableNamesset asErrNoArtifactswithout distinguishing “filter already satisfied” from “nothing exists”. - This is exactly the kind of regression a behavior-preserving refactor is supposed to avoid; please add a focused test around cached usage-only runs /
.dockerbuild-only residual artifacts.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 15.5 AIC · ⌖ 7.08 AIC · ⊞ 6.9K
Comment /review to run again
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (823 new lines across 📄 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: Split long CLI logs/reporting functions into focused helpers
This is a clean, well-structured refactoring that splits large functions into focused helpers. Logic is faithfully preserved — no behavioral regressions detected.
Correctness verified:
recoverBulkDownloadArtifactsalways callsmarkArtifactDownloaded, which is safe because it is only reachable frombulkDownloadArtifactsafterclassifyBulkDownloadErrorhandles fatal errors — the oldif err == nil || skippedNonZip || skippedCaseCollisionguard is now implicit in the call chain.- Spinner lifecycle is correct: individual path stops the spinner before delegating; bulk path stops on error inside
bulkDownloadArtifactsand callsStopWithMessagein the parent on success. logsAggregatestruct correctly initialisesengineCountsasmake(map[string]int)at the single call site.- The
fetchWorkflowRunLogsArchiveextraction cleanly separates download from extraction, improving testability.
✅ LGTM — approved.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 49.5 AIC · ⌖ 8.01 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — one correctness issue, one doc-contract nit. Requesting changes.
📋 Key Themes & Highlights
Key Themes
-
Spinner lifecycle gap (
pkg/cli/logs_download.goline 192):bulkDownloadArtifactsstops the spinner only whengh run downloaditself errors. If the gh command succeeds butrecoverBulkDownloadArtifactsreturns an error (e.g. marker write or artifact retry failure),bulkDownloadArtifactsreturns the error, the caller returns early, andStopWithMessageat line 199 is never reached — leaving the spinner running. This is a new path introduced by the refactor. -
Implicit contract on
classifyBulkDownloadError(pkg/cli/logs_download.goline 514): theerrparameter is used only forisPermissionError; the rest of the logic readsoutput []byte. This non-obvious split deserves a doc comment clarification.
Positive Highlights
- ✅ Excellent extraction of
fetchWorkflowRunLogsArchive— the([]byte, bool, error)triple cleanly separates "not found / expired" from real errors. - ✅
logsAggregatestruct with method receivers is a big improvement over 20 loose accumulator locals. - ✅
copyUsageAwInfoToRunRootis now independently testable — good deepening opportunity taken. - ✅
classifyRunFailureas anlogsAggregatemethod makes the side-effects on the aggregate explicit. - ✅ The PR description gives an accurate, file-by-file account of every extracted function — excellent author communication.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 103.2 AIC · ⌖ 9.79 AIC · ⊞ 7.7K
Comment /matt to run again
| } else { | ||
| logsDownloadLog.Printf("Could not list artifacts (will use bulk download): %v", listErr) | ||
| // No .dockerbuild artifacts detected (or listing failed) — use efficient bulk download. | ||
| if err := bulkDownloadArtifacts(ctx, opts, spinner, downloadableNames); err != nil { |
There was a problem hiding this comment.
[/codebase-design] bulkDownloadArtifacts stops the spinner when the gh run download command itself fails (line 459), but if the command succeeds and recoverBulkDownloadArtifacts subsequently errors (e.g. retryCaseCollisionArtifacts → downloadArtifactsByName returns an error), the spinner is never stopped. The caller at line 192 returns that error immediately, bypassing the StopWithMessage at line 199.
💡 Suggested fix
In downloadRunArtifacts, stop the spinner before the early return on bulk-download error:
if err := bulkDownloadArtifacts(ctx, opts, spinner, downloadableNames); err != nil {
if !opts.verbose {
spinner.Stop()
}
return err
}SpinnerWrapper.Stop() is idempotent, so calling it again on the normal success path is safe.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 69d61e1: downloadRunArtifacts now stops the spinner before returning any bulk-download recovery error.
| // classifyBulkDownloadError inspects a failed gh run download invocation and reports | ||
| // whether the failure can be recovered from by retrying artifacts individually. | ||
| // A non-nil error means the failure is fatal for this run. | ||
| func classifyBulkDownloadError(ctx context.Context, opts downloadArtifactsOptions, output []byte, err error) (skippedNonZip bool, skippedCaseCollision bool, fatal error) { |
There was a problem hiding this comment.
[/codebase-design] classifyBulkDownloadError's err parameter is used only for isPermissionError(err); the rest of the classification relies solely on output []byte. This is a non-obvious contract — the caller must pass the original err even though the function name implies it works entirely from output text.
💡 Suggestion
Strengthen the doc comment to make the contract explicit:
// classifyBulkDownloadError inspects a failed gh run download invocation.
// err must be the non-nil error returned by cmd.CombinedOutput() and is used
// exclusively to detect authentication failures. output is the combined
// stdout+stderr from the same invocation.Alternatively, consider testing isPermissionError at the call site in bulkDownloadArtifacts before delegating to classifyBulkDownloadError, keeping the two classification concerns visibly separate.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 69d61e1: the doc comment now states that err comes from CombinedOutput for authentication detection and output is the matching combined stdout/stderr.
|
@copilot this PR is ready for the next finishing pass. Please address these items, newest first:
Run: https://github.com/github/gh-aw/actions/runs/31996014734
|
|
Hey Here's one concrete thing that would strengthen this PR:
If you'd like a hand adding those tests, you can assign this prompt to your coding agent:
|
Addressed in 69d61e1. The branch is refreshed with latest |
|
@copilot this PR is ready for the next finishing pass. Please address these items, newest first:
Run: https://github.com/github/gh-aw/actions/runs/31997601784
|
Reviewed and confirmed addressed by 69d61e1: the cache-hit behavior is preserved and the usage-only/ |
PR TriageCategory: refactor | Risk: medium | Score: 48/100
Recommended action: fast_track (CI green, well-documented)
|
|
@copilot this PR is ready for the next finishing pass. Please address these items, newest first:
Run: https://github.com/github/gh-aw/actions/runs/32002564106
|
Confirmed addressed by 69d61e1: the cache-hit behavior and usage-only/ |
|
@copilot this PR is ready for the next finishing pass. Please address these items, newest first:
Run: https://github.com/github/gh-aw/actions/runs/32004940993
|
Verified both threads are addressed by |
|
@copilot this PR is ready for the next finishing pass. Please address these items, newest first:
Run: https://github.com/github/gh-aw/actions/runs/32019589269
|
Confirmed the remembered cache-hit issue remains addressed by |
|
🎉 This pull request is included in a new release. Release: |
make golint-custom(largefunc, 60-line limit) flagged a cluster of very long functions in the CLI log collection and reporting paths, notably two 321-line functions. This is a behavior-preserving refactor of the five functions listed in the issue; output shape and error semantics are unchanged.pkg/cli/logs_download.godownloadWorkflowRunLogs(64 → ~25): extractedfetchWorkflowRunLogsArchive, which builds the API endpoint and classifies not-found/expired/auth errors, returning anokflag for the non-critical "no logs" case.downloadRunArtifacts(321 → ~45): split into cache resolution (resolveCachedArtifacts), planning (planArtifactDownload→enumerateDownloadableArtifacts,planIncrementalDownload), execution (downloadArtifactsIndividually,bulkDownloadArtifactswithbuildBulkDownloadArgs,classifyBulkDownloadError,recoverBulkDownloadArtifacts,retryCaseCollisionArtifacts), and post-processing (finalizeArtifactDownload). The repeated "download logs for diagnostics, then clean up an empty dir" block is nowdownloadWorkflowRunLogsForDiagnostics.ensureUsageAwInfoFallback(73 → ~30): extractedcopyUsageAwInfoToRunRoot,resolveActivationArtifactNames,flattenActivationFallback.The main function now reads as a pipeline:
pkg/cli/logs_report.gobuildLogsData(321 → ~30): the ~25 loose accumulator locals became alogsAggregatestruct withaccumulateRunTotals,accumulateChainMetrics,classifyRunFailure(returns the failure kind and bumps the matching rollup), andsummary. Per-run conversion moved tobuildRunData/newRunDatawithextractRunEngineInfoandapplyAwInfoToRunData; the cross-run sections moved tobuildLogsSections.pkg/cli/audit_expanded.gobuildMCPServerHealth(96 → ~33): extractedappendMCPServerDetails,appendMissingFailedServers, andfinalizeMCPServerHealth(rollups, sort, summary string).Scope
buildSessionAnalysis(77 lines, same file asbuildMCPServerHealth) was left alone — it is not part of this slice._Run: https://github.com/github/gh-aw/actions/runs/31997601784_> Generated by 👨🍳 PR Sous Chef · gpt54 · 18 AIC · ⌖ 6.85 AIC · ⊞ 8.8K · ◷
make golint-custom(largefunc, 60-line limit) flagged a cluster of very long functions in the CLI log collection and reporting paths, notably two 321-line functions. This is a behavior-preserving refactor of the five functions listed in the issue; output shape and error semantics are unchanged.pkg/cli/logs_download.godownloadWorkflowRunLogs(64 → ~25): extractedfetchWorkflowRunLogsArchive, which builds the API endpoint and classifies not-found/expired/auth errors, returning anokflag for the non-critical "no logs" case.downloadRunArtifacts(321 → ~45): split into cache resolution (resolveCachedArtifacts), planning (planArtifactDownload→enumerateDownloadableArtifacts,planIncrementalDownload), execution (downloadArtifactsIndividually,bulkDownloadArtifactswithbuildBulkDownloadArgs,classifyBulkDownloadError,recoverBulkDownloadArtifacts,retryCaseCollisionArtifacts), and post-processing (finalizeArtifactDownload). The repeated "download logs for diagnostics, then clean up an empty dir" block is nowdownloadWorkflowRunLogsForDiagnostics.ensureUsageAwInfoFallback(73 → ~30): extractedcopyUsageAwInfoToRunRoot,resolveActivationArtifactNames,flattenActivationFallback.The main function now reads as a pipeline:
pkg/cli/logs_report.gobuildLogsData(321 → ~30): the ~25 loose accumulator locals became alogsAggregatestruct withaccumulateRunTotals,accumulateChainMetrics,classifyRunFailure(returns the failure kind and bumps the matching rollup), andsummary. Per-run conversion moved tobuildRunData/newRunDatawithextractRunEngineInfoandapplyAwInfoToRunData; the cross-run sections moved tobuildLogsSections.pkg/cli/audit_expanded.gobuildMCPServerHealth(96 → ~33): extractedappendMCPServerDetails,appendMissingFailedServers, andfinalizeMCPServerHealth(rollups, sort, summary string).Scope
buildSessionAnalysis(77 lines, same file asbuildMCPServerHealth) was left alone — it is not part of this slice._Run: https://github.com/github/gh-aw/actions/runs/31997601784_> Generated by 👨🍳 PR Sous Chef · gpt54 · 18 AIC · ⌖ 6.85 AIC · ⊞ 8.8K · ◷
@copilotthis PR is ready for the next finishing pass.Please address these items, newest first:
pr-finisherskill after the fixes and summarize the outcome._Run: https://github.com/github/gh-aw/actions/runs/32002564106_> Generated by 👨🍳 PR Sous Chef · gpt54 · 9.55 AIC · ⌖ 8.36 AIC · ⊞ 8.8K · ◷