Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions pkg/cli/logs_github_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,12 @@ func listWorkflowRunsWithPagination(opts ListWorkflowRunsOptions) ([]WorkflowRun
return nil, 0, fmt.Errorf("invalid field in JSON query (exit code %d): %s", exitCode, string(output))
}

// Ignore 403 errors (e.g. rate limit exceeded) – treat as no results.
if strings.Contains(combinedMsgLower, "http 403") {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("GitHub API returned 403, skipping: "+strings.TrimSpace(outputMsg)))
Comment on lines +239 to +241
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

Ignoring any HTTP 403 here changes the semantics of listWorkflowRunsWithPagination from “couldn’t fetch runs” to “no runs exist”, which can silently mask real permission/configuration problems (403 Forbidden / resource not accessible) in other call sites (e.g. pkg/cli/logs_orchestrator.go and logs_download flows). Consider scoping this behavior to rate-limit cases only (match on "rate limit" / "secondary rate limit") or making it opt-in via a ListWorkflowRunsOptions flag so only the health check suppresses 403s, while other commands still surface an actionable error.

Suggested change
// Ignore 403 errors (e.g. rate limit exceeded) – treat as no results.
if strings.Contains(combinedMsgLower, "http 403") {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("GitHub API returned 403, skipping: "+strings.TrimSpace(outputMsg)))
// Ignore 403 errors only when they clearly indicate a rate limit condition
// (e.g. "rate limit" / "secondary rate limit"). Other 403s (e.g. missing
// permissions, misconfigured token) should surface as errors instead of
// being treated as "no results".
if strings.Contains(combinedMsgLower, "http 403") &&
(strings.Contains(combinedMsgLower, "rate limit") ||
strings.Contains(combinedMsgLower, "secondary rate limit") ||
strings.Contains(combinedMsgLower, "abuse detection")) {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("GitHub API returned 403 (rate limited), skipping: "+strings.TrimSpace(outputMsg)))

Copilot uses AI. Check for mistakes.
return nil, 0, nil
}

// Check for authentication errors.
// "exit status 1" is intentionally omitted: gh exits 1 for many non-auth
// errors (e.g. unsupported JSON fields), so matching it caused misleading
Expand Down
25 changes: 23 additions & 2 deletions pkg/cli/logs_github_api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ func TestListWorkflowRunsErrorHandling(t *testing.T) {
outputMsg string
wantInvalidField bool
wantAuth bool
want403 bool
}{
{
name: "unknown JSON field (capital U, as gh CLI emits)",
Expand Down Expand Up @@ -96,6 +97,18 @@ func TestListWorkflowRunsErrorHandling(t *testing.T) {
outputMsg: "not logged into any GitHub hosts",
wantAuth: true,
},
{
name: "HTTP 403 rate limit is ignored (not an error)",
errMsg: "exit status 1",
outputMsg: "HTTP 403: API rate limit exceeded for installation.",
want403: true,
},
{
name: "HTTP 403 Forbidden is ignored",
errMsg: "exit status 1",
outputMsg: "HTTP 403: Forbidden",
want403: true,
},
}

for _, tt := range tests {
Expand All @@ -109,7 +122,8 @@ func TestListWorkflowRunsErrorHandling(t *testing.T) {
strings.Contains(combinedMsgLower, "unknown json") ||
strings.Contains(combinedMsgLower, "field not found") ||
strings.Contains(combinedMsgLower, "no such field")
isAuth := !isInvalidField && (strings.Contains(combinedMsg, "exit status 4") ||
is403 := strings.Contains(combinedMsgLower, "http 403")
isAuth := !isInvalidField && !is403 && (strings.Contains(combinedMsg, "exit status 4") ||
strings.Contains(combinedMsg, "not logged into any GitHub hosts") ||
strings.Contains(combinedMsg, "To use GitHub CLI in a GitHub Actions workflow") ||
strings.Contains(combinedMsg, "authentication required") ||
Expand All @@ -118,14 +132,21 @@ func TestListWorkflowRunsErrorHandling(t *testing.T) {
if tt.wantInvalidField {
assert.True(t, isInvalidField, "expected invalid-field classification")
assert.False(t, isAuth, "invalid-field errors must not be classified as auth errors")
assert.False(t, is403, "invalid-field errors must not be classified as 403 errors")
}
if tt.wantAuth {
assert.False(t, isInvalidField, "auth errors must not be classified as invalid-field errors")
assert.True(t, isAuth, "expected auth classification")
}
if !tt.wantInvalidField && !tt.wantAuth {
if tt.want403 {
assert.True(t, is403, "expected 403 classification")
assert.False(t, isAuth, "403 errors must not be classified as auth errors")
assert.False(t, isInvalidField, "403 errors must not be classified as invalid-field errors")
}
if !tt.wantInvalidField && !tt.wantAuth && !tt.want403 {
assert.False(t, isInvalidField, "should not be invalid-field")
assert.False(t, isAuth, "should not be auth")
assert.False(t, is403, "should not be 403")
}
})
}
Expand Down
Loading