[MINOR] The timer leak on the happy path is benign but slightly untidy. When <-timer.C fires, timer.Stop() returns false and the channel is already drained, so it's a no-op — the comment acknowledges this. Consider using timer.Reset pattern or noting it more explicitly, but this is purely cosmetic.
[NIT] strPtr is defined in pr_test.go but files_test.go and client_test.go are in the same package. If any other test file ever needs this helper, it will collide. It's already fine since both are in package github, but naming it something more descriptive (e.g., stringPtr) would match the codebase's convention of meaningful names over abbreviations per the style conventions.
[NIT] The fallback from array to single-object JSON unmarshal silently swallows the original array parse error. If the body is valid JSON but neither an array nor an object matching the entry struct (e.g., a JSON number), the second unmarshal will succeed with zero values and return a single empty entry. Consider checking that single.Name != "" or similar before accepting the fallback.
[MINOR] The mapCheckRunStatus function signature takes a second parameter _ string that is explicitly discarded. Since Go allows unused blank-identifier parameters, this is idiomatic, but the function could alternatively just take conclusion *string directly. The current form is acceptable but the doc comment explaining why the second param is ignored is good practice.
[MINOR] The int64 cast in if int64(len(body)) >= maxResponseBytes is unnecessary since maxResponseBytes is an untyped constant and len() returns int. Both sides of the comparison are int-typed. The cast is harmless but adds noise.
[MINOR] The ListContents fallback from array→object parse uses err2 for the object parse error but discards the original array parse error err. If both fail (e.g., the response is valid JSON but neither an array nor an object matching entry), only the object-unmarshal error is returned, which may be less informative. This is a minor diagnostics issue.
[MINOR] The maxPages constant (100) is shared between GetPullRequestFiles and the check-runs pagination in GetCommitStatuses. A PR with exactly 10,000 files (100 pages × 100 per page) would silently return incomplete results with no error or warning. The GitHub API itself caps PRs at 3000 files, so in practice this is fine, but the silent truncation at the limit is a latent correctness issue. Consider logging a warning or returning an error when the page cap is hit.
[NIT] The mapCheckRunStatus function signature accepts _ string (unused status field) and explains this in the comment. This is fine for now, but the stale and waiting conclusions are mapped to pending — waiting is not a GitHub API conclusion value (it's an internal concept). This won't cause bugs but may be dead code.
[MINOR] The response body size check uses >= instead of >, so a response of exactly maxResponseBytes (10 MiB) will return an error even though it was fully read and not truncated. The io.LimitReader reads at most N bytes, so len(body) == maxResponseBytes means the response may have been truncated — the intent seems correct, but the error message says 'exceeded' which is misleading for an exact match. This is consistent with the test (maxResponseBytes+1024), but the boundary condition is ambiguous. Consider using > maxResponseBytes - 1 or reading one extra byte as a sentinel to confirm truncation actually occurred.
[NIT] The timer in the retry loop has a comment // Timer already fired; Stop() is a no-op here. — this is slightly inaccurate. Stop() on an already-fired timer is not a no-op; it returns false and the channel has already been drained by the case <-timer.C: branch, so there's no leak. The comment could be clearer: // Channel already drained; no need to call Stop().
[NIT] The SetHTTPClient and SetRetryBackoff methods are exported as test-injection points but documented as unsafe for concurrent use with requests. The convention in the patterns (style.md) suggests using unexported helpers or the functional options pattern for test injection. The current design works but exposes mutation methods on what is documented as a concurrent-safe type. A //nolint or explicit doc note would make the intent clearer.
[MINOR] The mapCheckRunStatus function signature func mapCheckRunStatus(conclusion *string, _ string) string uses a blank identifier for the status parameter. While the comment explains why, a cleaner approach given the parameter is truly unused would be to just not include it and update the call site — or if the signature must stay for extensibility, at minimum name it (e.g., checkStatus string) so readers can understand what's being ignored. The blank _ with no name makes the call site mapCheckRunStatus(cr.Conclusion, cr.Status) harder to understand without reading the comment.
[NIT] The doRequest method signature uses url as a parameter name, which shadows the imported net/url package. While net/url is not imported in client.go (it's only in pr.go and files.go), this naming is a minor style concern since url as a local variable name is idiomatic but shadows the package name if ever imported. Not a bug here, just worth noting.
[NIT] TestDoRequest_429ExhaustsRetries type-asserts err.(*APIError) directly rather than using errors.As. Since GetPullRequest and other methods wrap errors with fmt.Errorf("...: %w", err), but doGet/doRequest return the *APIError directly (not wrapped), the direct assertion works here. But it's inconsistent with the project's own IsNotFound/IsUnauthorized helpers which use errors.As. Test code should prefer errors.As for resilience.
[MINOR] The comment // Timer already fired; Stop() is a no-op here. is misleading. time.NewTimer creates a timer that fires after the duration; when the select case <-timer.C fires, the timer has already expired so Stop() does return false, but the comment conflates 'fired' with 'Stop is no-op'. More importantly, there's no defer timer.Stop() before the select — while technically fine here because the goroutine either drains the channel or returns, the pattern is fragile and deviates from the canonical defer timer.Stop() idiom. The standard pattern is: timer := time.NewTimer(delay); defer timer.Stop(); select { case <-timer.C: ... case <-ctx.Done(): ... }. The current code avoids the leak correctly but via an atypical path.
[NIT] The maxPages = 100 constant limits to 10,000 files (100 pages × 100 per page). GitHub's API caps PRs at 3,000 changed files, so this bound is fine in practice. However, the constant is shared between GetPullRequestFiles and the check-runs pagination in GetCommitStatuses, which have different semantics. These could be separate constants with explanatory comments for clarity.
[MINOR] The fallback from array to object JSON parsing is an unusual approach. The error from the first json.Unmarshal (array attempt) is discarded silently. If the response is truly malformed JSON (not just an object vs array mismatch), only err2 from the single-entry unmarshal is surfaced. This is intentional per the comment but it means a corrupted array response that happens to partially parse as an object could go undetected. Consider checking if the body starts with [ vs { to choose the parsing path, which would be more explicit and preserve the original error on genuine parse failures.
[NIT] mapCheckRunStatus accepts a second parameter _ string that is intentionally unused. The function signature is slightly misleading for callers. Since this is unexported and only called from one place, simplifying to mapCheckRunStatus(conclusion *string) string would be cleaner. The current approach was likely chosen to match the struct fields but the unused parameter adds noise.