Proactively ignore .dockerbuild artifacts in logs download#24386
Proactively ignore .dockerbuild artifacts in logs download#24386
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/554338e6-cd25-4c05-b344-3b29e2abdac5 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the workflow run artifact download flow to avoid .dockerbuild artifacts (non-zip) aborting gh run download, by pre-listing artifacts via the GitHub API and selectively downloading only valid artifacts.
Changes:
- Add detection for
.dockerbuildartifacts and pre-list artifact names viagh api. - When
.dockerbuildartifacts are present, skip them and download remaining artifacts individually by name. - Add unit tests for the
.dockerbuildartifact name predicate.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_download.go | Adds artifact pre-listing and conditional per-artifact downloads to bypass .dockerbuild extraction failures. |
| pkg/cli/logs_download_test.go | Adds tests for .dockerbuild artifact name detection. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/cli/logs_download.go:701
- In the individual-download (.dockerbuild) branch, returning ErrNoArtifacts solely because the output directory is still empty can misclassify “all downloads failed” as “run has no artifacts”. Since ErrNoArtifacts is used by callers to skip/soft-handle runs, it would be safer to distinguish “no downloadable artifacts exist” from “downloads attempted but failed” (e.g., return a real error when at least one download attempt failed).
if err := downloadArtifactsByName(runID, outputDir, downloadableNames, verbose, owner, repo, hostname); err != nil {
return err
}
if fileutil.IsDirEmpty(outputDir) {
// Downloads were attempted but none succeeded; treat as no artifacts.
return ErrNoArtifacts
}
- Files reviewed: 2/2 changed files
- Comments generated: 2
| cmd := workflow.ExecGH(args...) | ||
| cmdOutput, cmdErr := cmd.CombinedOutput() | ||
| if cmdErr != nil { | ||
| logsDownloadLog.Printf("Failed to download artifact %q: %v (%s)", name, cmdErr, string(cmdOutput)) | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to download artifact %q: %v", name, cmdErr))) | ||
| } | ||
| // Non-fatal: continue downloading other artifacts | ||
| } else { |
There was a problem hiding this comment.
downloadArtifactsByName currently treats every per-artifact failure as non-fatal and always returns nil. In the .dockerbuild path, if all downloads fail, downloadRunArtifacts returns ErrNoArtifacts, which can incorrectly mask real failures (e.g., auth/permission issues, transient network errors) as “no artifacts” and cause callers to skip runs. Consider making authentication/permission failures fatal (consistent with the bulk path) and/or having downloadArtifactsByName return an aggregated error (or at least the last error) when nothing was successfully downloaded.
This issue also appears on line 695 of the same file.
| output, err := cmd.Output() | ||
| if err != nil { |
There was a problem hiding this comment.
listRunArtifactNames uses cmd.Output(), so if gh api fails the returned error won’t include stderr/API error details. Elsewhere in this codebase API calls typically use workflow.RunGHCombined to capture stderr for diagnostics (e.g., pkg/cli/logs_github_api.go:37). Consider switching to CombinedOutput/RunGHCombined and including the output in the wrapped error so failures to pre-list artifacts are actionable when troubleshooting.
| output, err := cmd.Output() | |
| if err != nil { | |
| output, err := cmd.CombinedOutput() | |
| if err != nil { | |
| trimmedOutput := strings.TrimSpace(string(output)) | |
| if trimmedOutput != "" { | |
| return nil, fmt.Errorf("failed to list artifacts for run %d: %w: %s", runID, err, trimmedOutput) | |
| } |
gh run downloadaborts when it encounters.dockerbuildartifacts (not valid zip archives), potentially missing other valid artifacts downstream. The existing workaround only retried a hardcoded set of "critical" artifacts.Approach
Instead of reacting to the error, proactively list all artifacts via the GitHub API before downloading, filter out
.dockerbuildentries, and download the remainder individually.Changes
listRunArtifactNames— queriesrepos/.../actions/runs/{id}/artifactswith--paginateto enumerate artifact names before any download beginsisDockerBuildArtifact— predicate:strings.HasSuffix(name, ".dockerbuild")downloadArtifactsByName— downloads a filtered list of artifacts individually viagh run download --name <name>; failures are non-fatal per artifactdownloadRunArtifacts— now partitions artifact names into.dockerbuildvs downloadable before downloading:.dockerbuildartifacts found: warns and callsdownloadArtifactsByNamefor the rest — bulk download is never attempted