fix: pass owner/repo to gh run download for cross-repo audit#18603
fix: pass owner/repo to gh run download for cross-repo audit#18603
gh run download for cross-repo audit#18603Conversation
…kflowRunLogs for cross-repo support Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
…e, use consistent fmt.Sprintf Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
gh run download for cross-repo audit
There was a problem hiding this comment.
Pull request overview
Fixes cross-repo workflow run auditing/log download failures by ensuring the correct repository (and host where applicable) is used when downloading artifacts and workflow run logs.
Changes:
- Extend artifact/log download helpers to accept
owner,repo, andhostname, and use explicit repo-scoped API endpoints/CLI flags. - Thread
owner/repo/hostnamefromAuditWorkflowRuninto artifact download, and threadrepoOverridethrough concurrent downloads forgh aw logs --repo. - Update unit tests to match updated function signatures.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_download.go | Adds repo/host-aware endpoint construction for run logs and adds repo override support for gh run download. |
| pkg/cli/audit.go | Forwards parsed owner/repo/hostname into artifact download for cross-repo audits. |
| pkg/cli/logs_orchestrator.go | Threads repoOverride into concurrent artifact download and parses it for owner/repo. |
| pkg/cli/logs_parallel_test.go | Updates calls to downloadRunArtifactsConcurrent for the new signature. |
| pkg/cli/audit_test.go | Updates call to downloadRunArtifacts for the new signature. |
Comments suppressed due to low confidence (2)
pkg/cli/logs_download.go:518
- New behavior is introduced here (adding repo override flags to the
gh run downloadinvocation), but there’s no unit test that asserts the constructed gh arguments/behavior for cross-repo (and host) inputs. Consider extracting arg construction into a small pure helper and adding tests for owner/repo and hostname cases so regressions are caught without needing network/gh CLI access.
// Build gh run download command with optional repo override for cross-repo support
ghArgs := []string{"run", "download", strconv.FormatInt(runID, 10), "--dir", outputDir}
if owner != "" && repo != "" {
ghArgs = append(ghArgs, "-R", owner+"/"+repo)
}
pkg/cli/logs_orchestrator.go:552
repoOverrideis documented as[HOST/]owner/repo, but this parsing usesSplitN(repoOverride, "/", 2). For values likeghe.example.com/owner/repo, this will setdlOwnerto the host anddlRepotoowner/repo, which later produces an invalid repo string and breaks cross-host downloads. Parse 2- vs 3-segment forms explicitly (host/owner/repo) and thread the host through (or avoid splitting entirely by passing the full repoOverride togh run download --repo).
// Parse repoOverride into owner/repo once for cross-repo artifact download
var dlOwner, dlRepo string
if repoOverride != "" {
parts := strings.SplitN(repoOverride, "/", 2)
if len(parts) == 2 {
dlOwner = parts[0]
dlRepo = parts[1]
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/cli/logs_download.go
Outdated
| // Build gh run download command with optional repo override for cross-repo support | ||
| ghArgs := []string{"run", "download", strconv.FormatInt(runID, 10), "--dir", outputDir} | ||
| if owner != "" && repo != "" { | ||
| ghArgs = append(ghArgs, "-R", owner+"/"+repo) |
There was a problem hiding this comment.
downloadRunArtifacts now accepts hostname, but the gh run download command it builds never uses it. This means auditing/downloading artifacts for GHES or other non-default hosts will still try to use the default host and can fail. Include the hostname in the gh invocation (or use --repo with [HOST/]owner/repo) when hostname is set and not github.com.
This issue also appears on line 514 of the same file.
| // Build gh run download command with optional repo override for cross-repo support | |
| ghArgs := []string{"run", "download", strconv.FormatInt(runID, 10), "--dir", outputDir} | |
| if owner != "" && repo != "" { | |
| ghArgs = append(ghArgs, "-R", owner+"/"+repo) | |
| // Build gh run download command with optional repo/hostname override for cross-repo and multi-host support | |
| ghArgs := []string{"run", "download", strconv.FormatInt(runID, 10), "--dir", outputDir} | |
| if owner != "" && repo != "" { | |
| // When a non-default hostname is provided, qualify the repo with the host: HOST/owner/repo | |
| if hostname != "" && hostname != "github.com" { | |
| ghArgs = append(ghArgs, "-R", hostname+"/"+owner+"/"+repo) | |
| } else { | |
| ghArgs = append(ghArgs, "-R", owner+"/"+repo) | |
| } | |
| } else if hostname != "" && hostname != "github.com" { | |
| // If we don't have an explicit owner/repo but do have a non-default host, ensure gh targets that host | |
| ghArgs = append(ghArgs, "--hostname", hostname) |
There was a problem hiding this comment.
Fixed in f091cc6. downloadRunArtifacts now uses HOST/owner/repo format for -R when a non-default hostname is provided, matching the gh CLI's [HOST/]owner/repo convention.
| // downloadRunArtifactsConcurrent downloads artifacts for multiple workflow runs concurrently | ||
| func downloadRunArtifactsConcurrent(ctx context.Context, runs []WorkflowRun, outputDir string, verbose bool, maxRuns int) []DownloadResult { | ||
| func downloadRunArtifactsConcurrent(ctx context.Context, runs []WorkflowRun, outputDir string, verbose bool, maxRuns int, repoOverride string) []DownloadResult { | ||
| logsOrchestratorLog.Printf("Starting concurrent artifact download: runs=%d, outputDir=%s, maxRuns=%d", len(runs), outputDir, maxRuns) |
There was a problem hiding this comment.
This function signature change requires updating all call sites. There are still callers using the old 5-arg form under the integration build tag (e.g., pkg/cli/logs_orchestrator_test.go:22,41,74,...), which will fail to compile when integration tests are built. Update those call sites to pass the new repoOverride argument.
This issue also appears on line 544 of the same file.
There was a problem hiding this comment.
Fixed in f091cc6. All 14 call sites in logs_orchestrator_test.go (integration build tag) now pass the repoOverride argument. Verified with go build -tags integration ./pkg/cli/.
|
@copilot investigate integration test failures in this PR. |
…signature and add GHES hostname support to gh run download Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
The integration test failures were caused by the 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:
|
owner,repo,hostnameparameters todownloadRunArtifacts()anddownloadWorkflowRunLogs()audit.goto passowner,repo,hostnametodownloadRunArtifacts()logs_orchestrator.goto threadrepoOverridethroughdownloadRunArtifactsConcurrentaudit_test.goandlogs_parallel_test.gologs_orchestrator_test.go(add missingrepoOverridearg)downloadRunArtifactsto useHOST/owner/repofor GHESOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.