Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f8539e73-de7e-4c00-a5bc-1fca31431d93 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f8539e73-de7e-4c00-a5bc-1fca31431d93 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f8539e73-de7e-4c00-a5bc-1fca31431d93 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey One note: this repository doesn't have a
The PR looks technically solid and ready for maintainer review once the draft flag is removed.
|
There was a problem hiding this comment.
Pull request overview
Improves gh run download resilience on macOS by detecting case-colliding zip extraction failures and continuing downloads instead of aborting the entire run’s artifact retrieval.
Changes:
- Tightened non-zip artifact detection to only match the invalid-zip signature (
zip: not a valid zip file). - Added case-collision extraction failure detection and, on bulk-download failure, retries artifacts individually to avoid blocking later artifacts.
- Updated/added unit tests for the new error classification behavior and case-collision signature matching.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_download.go | Adds case-collision error classification and a retry path that downloads artifacts individually after bulk extraction aborts. |
| pkg/cli/logs_download_test.go | Updates non-zip classification expectations and adds focused tests for case-collision detection. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
| // Best effort fallback when initial artifact listing was unavailable. | ||
| artifactNamesRetry, retryListErr := listRunArtifactNames(ctx, runID, owner, repo, hostname, verbose) | ||
| if retryListErr == nil { | ||
| for _, name := range artifactNamesRetry { | ||
| if isDockerBuildArtifact(name) { | ||
| continue | ||
| } | ||
| if artifactMatchesFilter(name, artifactFilter) { | ||
| retryNames = append(retryNames, name) | ||
| } | ||
| } |
There was a problem hiding this comment.
The warning says artifacts will be retried individually, but the fallback retry path silently does nothing if the initial artifact listing was unavailable and the subsequent listRunArtifactNames call fails (retryListErr != nil). In that scenario the command will proceed as if successful even though later artifacts may have been skipped. Consider surfacing a follow-up warning when retryListErr is non-nil and/or returning an error when you cannot obtain any names to retry after detecting a case-collision bulk failure.
| // Best effort fallback when initial artifact listing was unavailable. | |
| artifactNamesRetry, retryListErr := listRunArtifactNames(ctx, runID, owner, repo, hostname, verbose) | |
| if retryListErr == nil { | |
| for _, name := range artifactNamesRetry { | |
| if isDockerBuildArtifact(name) { | |
| continue | |
| } | |
| if artifactMatchesFilter(name, artifactFilter) { | |
| retryNames = append(retryNames, name) | |
| } | |
| } | |
| // Initial artifact listing was unavailable, so fetch names now for targeted retry. | |
| artifactNamesRetry, retryListErr := listRunArtifactNames(ctx, runID, owner, repo, hostname, verbose) | |
| if retryListErr != nil { | |
| return fmt.Errorf("bulk artifact download hit case-colliding entries and could not list artifacts for individual retry: %w", retryListErr) | |
| } | |
| for _, name := range artifactNamesRetry { | |
| if isDockerBuildArtifact(name) { | |
| continue | |
| } | |
| if artifactMatchesFilter(name, artifactFilter) { | |
| retryNames = append(retryNames, name) | |
| } |
🧪 Test Quality Sentinel ReportTest Quality Score: 100/100✅ Excellent test quality
Test Classification Details
Test Analysis Details📋 Per-test breakdown
|
|
✅ Smoke CI completed successfully! |
|
🤖 Smoke CI — run complete ✅ Run URL: https://github.com/github/gh-aw/actions/runs/24807347146 |
|
@copilot review all comments |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/44392393-c47d-4cad-aec4-3dc4c84b94b0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/44392393-c47d-4cad-aec4-3dc4c84b94b0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed and addressed the actionable review feedback. I updated the case-collision fallback to return an explicit error if artifact names cannot be listed for individual retry (instead of silently continuing). Also cleaned up the earlier lockfile drift in the same commit. Addressed in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
gh run downloadcan abort on macOS when an artifact zip contains paths that differ only by case (for exampleMEMORY.mdandmemory.md). This currently prevents later artifacts in the same run from being downloaded.Error classification: distinguish case-collision vs non-zip failures
zip: not a valid zip file).error extracting zip archive+file exists).Download flow resilience: continue after case-collision failures
Targeted test coverage
✅ Smoke CI passed — https://github.com/github/gh-aw/actions/runs/24807347146
> Generated by Smoke CI for issue #27893 · ● 486.9K · ◷