Skip to content

Fix nil-pointer panic when GitHub returns a rate-limit error - #800

Merged
alyssajoyner merged 5 commits into
mainfrom
alyssa/fix-nil-pointer-dereference
Aug 20, 2026
Merged

Fix nil-pointer panic when GitHub returns a rate-limit error#800
alyssajoyner merged 5 commits into
mainfrom
alyssa/fix-nil-pointer-dereference

Conversation

@alyssajoyner

@alyssajoyner alyssajoyner commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

What

sanitizeGitHubError now flattens all go-github error types with a custom Is method (ErrorResponse, RateLimitError, AbuseRateLimitError, AcceptedError, RedirectionError) to a plain error, not just ErrorResponse.

Why

When GitHub rate-limits a query, go-github returns a *RateLimitError, whose Is method calls errors.As(target, &v). The SDK's guessErrorStatus classifies errors via errors.Is(err, (*url.Error)(nil)) (a typed-nil target), which routes into RateLimitError.Is and calls (*url.Error).Unwrap on a nil receiver, panicking. The panic is recovered as a plugin.requestFailureError, so it counts against the DatasourcePluginQuerySuccessRate SLO.

sanitizeGitHubError already guarded against this but only for ErrorResponse; all five types share the same unsafe Is. Flattening with fmt.Errorf("%s", ...) (not %w) drops the Is method while keeping the message, so downstream classification is unaffected.

Copilot AI lite review requested due to automatic review settings August 18, 2026 03:19
@alyssajoyner
alyssajoyner requested a review from a team as a code owner August 18, 2026 03:19
@cla-assistant

cla-assistant Bot commented Aug 18, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request prevents a nil-pointer panic during datasource error-source classification by guarding access to go-github’s embedded *http.Response before reading StatusCode. This keeps request failures from being misclassified via an SDK panic path and aligns the classification behavior with the intended error handling flow.

Changes:

  • Add a defensive check for resp.Response != nil before reading resp.StatusCode in addErrorSourceToError.
  • Add a regression test covering the “non-nil wrapper Response with nil inner *http.Response” scenario.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pkg/github/client/errorsourcehandling.go Avoids nil-pointer panic by guarding the embedded *http.Response before using StatusCode.
pkg/github/client/errorsourcehandling_test.go Adds a test to ensure the nil inner *http.Response case does not panic and preserves the error.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pkg/github/client/errorsourcehandling.go
@alyssajoyner alyssajoyner changed the title Fix a nil-pointer panic in datasource error classification Fix nil-pointer panic when GitHub returns a rate-limit error Aug 19, 2026
@alyssajoyner
alyssajoyner requested a review from aangelisc August 19, 2026 22:10
Comment thread pkg/github/client/errorsourcehandling.go Outdated
@alyssajoyner
alyssajoyner requested a review from aangelisc August 20, 2026 15:10
@alyssajoyner
alyssajoyner merged commit 41dc39c into main Aug 20, 2026
25 checks passed
@alyssajoyner
alyssajoyner deleted the alyssa/fix-nil-pointer-dereference branch August 20, 2026 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants