[MINOR] The Description field for check run statuses is populated with derefString(cr.Conclusion) — the raw conclusion value (e.g. "success", "failure", "skipped"). This is the same value that's being mapped to Status via mapCheckRunStatus. The doc comment says 'raw conclusion value (e.g. "success", "failure", "skipped")' but for commit statuses, Description is the human-readable description string (e.g. 'Build passed'). Using the conclusion as description is a semantic mismatch: for commit statuses Description carries a narrative message, while for check runs it carries a machine-readable conclusion. Consumers expecting a human-readable description field will get a raw enum string instead. Consider using cr.Status (e.g. 'completed', 'in_progress') or leaving it empty rather than duplicating the conclusion.
[NIT] The TestGetCommitStatuses_CheckRunConclusions table-driven test creates an httptest server inside each subtest via t.Parallel(). The tt variable is correctly used (Go 1.22+ loop variable semantics), and the test is well-structured. However, t.Parallel() is called inside subtests that close over tt — this is safe in modern Go but the test could use t.Run with the name directly from *tt.conclusion rather than a pre-computed name variable. Minor readability nit only.
[MINOR] mapCheckRunStatus maps "cancelled" and "skipped" to "success". While the comment explains this as 'non-blocking per GitHub check suite semantics', this is a loss of information at the vcs abstraction layer. Callers who need to distinguish between a genuinely passing check and a skipped/cancelled one have no way to do so. Since vcs.CommitStatus.Status is a string (not an enum), it would be safe to introduce additional values like "skipped" and "cancelled" if consumers need to distinguish them. This is a design tradeoff rather than a bug, but the mapping decision warrants discussion.
[NIT] Double blank line between TestSetHTTPClient_NilRestoresDefault and TestSetRetryBackoff_RejectsInvalidLength. Minor formatting — gofmt would normalize this to a single blank line.
[NIT] The defaultBackoff local variable is declared but serves only as a template for the else branch. The variable name and the copy-pattern are fine, but it could be simplified slightly: the if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 guard duplicates the invariant already enforced by SetRetryBackoff. A comment noting that the length check here is a defensive guard against direct struct construction (bypassing the setter) would clarify the intent.
[NIT] Transport errors from c.httpClient.Do(req) are returned immediately without setting lastErr, meaning if a transport error occurs on attempt > 0 after successful 429 retries, the function correctly returns the error — but if the intent is to return the last meaningful error in all cases, the transport error path bypasses lastErr. This is fine as-is since transport errors are not retried per the documented contract, but a brief inline comment noting 'transport errors are non-retryable, return immediately' would improve clarity.
[NIT] The defaultCheckRedirect function uses len(via) >= 10 to cap redirects, matching net/http's own default of 10. However, the stdlib's default CheckRedirect returns ErrUseLastResponse-based behavior; this returns a plain fmt.Errorf. This is functionally equivalent (both cause the redirect to fail) but the error message won't be recognized as a redirect limit error by callers using errors.Is. Since this is internal behavior and unlikely to be checked by callers, it's a minor style point only.
[NIT] The allowInsecureHTTP field is stored on both clientConfig and Client. While this is a deliberate copy (the option is read once at construction and stored for use in doRequest), adding a brief comment on the Client.allowInsecureHTTP field noting it is set from ClientOption at construction time would make the data flow clearer for future maintainers.
[MINOR] The SetHTTPClient and SetRetryBackoff methods are documented as 'test setup only' and 'must be called before any goroutines issue requests'. Consider either unexported test helpers via an export_test.go bridge (following the stdlib export_test.go pattern documented in the testing patterns) to avoid exposing these mutation methods on the public API surface, or at minimum adding a //nolint or similar signal. As-is, exported mutation methods on a type documented as 'safe for concurrent use' is a mild API design tension.
[NIT] Minor timer resource leak: when delay == 0, the timer is never created but timer.Stop() is only called after the select fires. This is fine. However, in the case <-timer.C branch, timer.Stop() is called after the channel has already fired — the comment acknowledges it's a no-op. This is correct per Go docs but the symmetry comment is slightly misleading; timer.Stop() after drain is genuinely a no-op and serves no purpose beyond visual symmetry. Not a bug.
[MINOR] The panic on backoff length mismatch is a deliberate 'catch misconfiguration in tests' guard, but panicking in a library function violates the repo convention ('Return errors; never panic'). The comment justifies it as a programming error caught early, but it means any test that calls SetRetryBackoff with the wrong length (e.g., during future refactoring that changes maxAttempts) will panic rather than getting a useful error. Consider returning an error from doRequest or validating at SetRetryBackoff time instead.
[MINOR] When delay == 0 (e.g., a past HTTP-date Retry-After), the timer block is skipped entirely due to the if delay > 0 guard, which is correct. However, if attempt-1 >= len(backoff) (impossible given the length check, but defensive readers will note it), delay stays 0 and is also skipped. The logic is correct but the attempt-1 < len(backoff) guard (line 245) is now dead code because the panic above guarantees len(backoff) == maxAttempts-1 == 2 and attempt ranges 1..2, so attempt-1 is always 0 or 1, always in bounds. The dead branch is harmless but slightly confusing.
[NIT] TestDoRequest_429ExhaustsRetries uses a direct type assertion err.(*APIError) rather than errors.As. While this works since doRequest returns *APIError unwrapped, using errors.As would be more idiomatic per the error-handling patterns and would remain correct if error wrapping is added later.
[MINOR] TestGetCommitStatuses_CheckRunConclusions creates a new httptest.Server inside each subtest iteration but does not call t.Parallel() on the subtests. Each subtest also closes its server with defer srv.Close() which is correct, but spawning 9 servers sequentially when they could run in parallel is a minor inefficiency. More importantly, there is no test for the check-runs endpoint returning a 404 or error after statuses succeed (the documented behavior where partial results are not returned).
[MINOR] The mapCheckRunStatus function maps cancelled, skipped, and neutral to success. The comment says 'non-blocking' but this could be surprising to callers who depend on CommitStatus.Status for gating decisions — a cancelled check that maps to 'success' might unblock a review when it shouldn't. This is a semantic/policy decision that should be documented more prominently in the function's doc comment (the current comment only appears inline in the switch case, not in the function doc).
[NIT] The doRequest method signature uses accept string as the last parameter rather than a more descriptive name. Since this is unexported and used only internally, acceptHeader would be clearer at the call sites, but this is purely stylistic.