Skip to content

fix(audit): gracefully skip non-zip artifacts instead of crashing #20294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Mar 10, 2026
Merged

fix(audit): gracefully skip non-zip artifacts instead of crashing #20294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81a03d1
Initial plan
Copilot Mar 10, 2026
d4d9eaf
fix(audit): skip non-zip artifacts (e.g. .dockerbuild) instead of fai…
Copilot Mar 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion pkg/cli/logs_download.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,15 @@ func listArtifacts(outputDir string) ([]string, error) {
return artifacts, nil
}

// isNonZipArtifactError reports whether the output from gh run download indicates
// that the failure was caused by one or more non-zip artifacts (e.g. .dockerbuild files).
// Such artifacts cannot be extracted as zip archives and should be skipped rather than
// failing the entire download.
func isNonZipArtifactError(output []byte) bool {
s := string(output)
return strings.Contains(s, "zip: not a valid zip file") || strings.Contains(s, "error extracting zip archive")
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

isNonZipArtifactError currently returns true for any output containing "error extracting zip archive". That substring can also occur for genuinely broken/partial zip downloads (e.g. unexpected EOF), not just non-zip artifacts, so this change may silently downgrade real failures to warnings and continue with incomplete data. Consider tightening the detection to only match the specific non-zip signature (e.g. "zip: not a valid zip file" / other known non-zip text), or rename the helper + warning to reflect that any extraction error is treated as non-fatal.

Suggested change
return strings.Contains(s, "zip: not a valid zip file") || strings.Contains(s, "error extracting zip archive")
return strings.Contains(s, "zip: not a valid zip file")

Copilot uses AI. Check for mistakes.
}

// downloadRunArtifacts downloads artifacts for a specific workflow run
func downloadRunArtifacts(runID int64, outputDir string, verbose bool, owner, repo, hostname string) error {
logsDownloadLog.Printf("Downloading run artifacts: run_id=%d, output_dir=%s, owner=%s, repo=%s", runID, outputDir, owner, repo)
Expand Down Expand Up @@ -612,6 +621,10 @@ func downloadRunArtifacts(runID int64, outputDir string, verbose bool, owner, re
cmd := workflow.ExecGH(ghArgs...)
output, err := cmd.CombinedOutput()

// skippedNonZipArtifacts is set when gh run download fails due to non-zip artifacts
// (e.g., .dockerbuild files). In that case we warn and continue with what was downloaded.
var skippedNonZipArtifacts bool

if err != nil {
// Stop spinner on error
if !verbose {
Expand Down Expand Up @@ -645,7 +658,27 @@ func downloadRunArtifacts(runID int64, outputDir string, verbose bool, owner, re
if strings.Contains(err.Error(), "exit status 4") {
return errors.New("GitHub CLI authentication required. Run 'gh auth login' first")
}
return fmt.Errorf("failed to download artifacts for run %d: %w (output: %s)", runID, err, string(output))
// Check if the error is due to non-zip artifacts (e.g., .dockerbuild files).
// The gh CLI fails when it encounters artifacts that are not valid zip archives.
// We warn and continue with any artifacts that were successfully downloaded.
if isNonZipArtifactError(output) {
// Show a concise warning; the raw output may be verbose so truncate it.
msg := string(output)
if len(msg) > 200 {
msg = msg[:200] + "..."
}
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Some artifacts could not be extracted (not a valid zip archive) and were skipped: "+msg))
skippedNonZipArtifacts = true
} else {
return fmt.Errorf("failed to download artifacts for run %d: %w (output: %s)", runID, err, string(output))
}
}

if skippedNonZipArtifacts && fileutil.IsDirEmpty(outputDir) {
// All artifacts were non-zip (none could be extracted) so nothing was downloaded.
// Treat this the same as a run with no artifacts — the audit will rely solely on
// workflow logs rather than artifact content.
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

In the skippedNonZipArtifacts && IsDirEmpty(outputDir) branch, the function returns ErrNoArtifacts without attempting to download workflow run logs. The earlier "no valid artifacts" branch explicitly downloads logs so audits can still diagnose failures. For runs where all artifacts are non-zip (empty dir after gh run download), we should also call downloadWorkflowRunLogs (and optionally clean up the empty dir if that produces nothing) before returning ErrNoArtifacts.

This issue also appears on line 664 of the same file.

Suggested change
// workflow logs rather than artifact content.
// workflow logs rather than artifact content.
// Attempt to download workflow run logs so audits can still diagnose failures,
// matching the behavior of runs with no valid artifacts.
if err := downloadWorkflowRunLogs(runID, outputDir, verbose, owner, repo, hostname); err != nil {
// Log the error but don't fail the process; logs may be unavailable (expired/deleted).
if verbose {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to download workflow run logs: %v", err)))
}
}

Copilot uses AI. Check for mistakes.
return ErrNoArtifacts
}

// Stop spinner with success message
Expand Down
48 changes: 48 additions & 0 deletions pkg/cli/logs_download_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,54 @@ func TestErrNoArtifacts(t *testing.T) {
}
}

func TestIsNonZipArtifactError(t *testing.T) {
tests := []struct {
name string
output string
expected bool
}{
{
name: "zip: not a valid zip file",
output: "error downloading github~gh-aw~39RTHX.dockerbuild: error extracting zip archive: zip: not a valid zip file",
expected: true,
},
{
name: "error extracting zip archive",
output: "error downloading some-artifact: error extracting zip archive: unexpected EOF",
expected: true,
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

The table case "error extracting zip archive" expects true for an "unexpected EOF" extraction failure. That broadens the behavior beyond skipping non-zip artifacts (it will also treat corrupted/partial zip downloads as non-fatal). If the intent is only to skip non-zip formats, update the test (and helper) to require a non-zip-specific signature; otherwise consider renaming the helper/test to match the broader semantics.

Suggested change
expected: true,
expected: false,

Copilot uses AI. Check for mistakes.
},
{
name: "both patterns present",
output: "error extracting zip archive: zip: not a valid zip file",
expected: true,
},
{
name: "unrelated error",
output: "exit status 1: some other failure",
expected: false,
},
{
name: "empty output",
output: "",
expected: false,
},
{
name: "no artifacts found",
output: "no valid artifacts found",
expected: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := isNonZipArtifactError([]byte(tt.output))
if result != tt.expected {
t.Errorf("isNonZipArtifactError(%q) = %v, want %v", tt.output, result, tt.expected)
}
})
}
}

func TestListWorkflowRunsWithPagination(t *testing.T) {
// Test that listWorkflowRunsWithPagination properly adds beforeDate filter
// Since we can't easily mock the GitHub CLI, we'll test with known auth issues
Expand Down
Loading