[NIT] stringPtr helper is defined at the bottom of pr_test.go but is a common test utility. Since the test file is in package github (not package github_test), it's only visible within the package's test files. This is fine, but if other test files in the package need pointer helpers in the future, it could conflict. A convention note: per project conventions, no single-letter names outside loop indices — s parameter in stringPtr is acceptable here as it's conventional for this helper.
[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).
[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.
[MINOR] The 'try array first, fall back to object' JSON parsing strategy is a pragmatic workaround for the GitHub API's inconsistent response shape. The approach is well-commented, but an empty JSON array [] will unmarshal successfully into a nil []entry (not an empty struct), so the guard at line 100 (checking Name/Path/Type all empty) is actually only reached when the fallback object parse succeeds — it cannot be triggered by the successful array parse. The logic is correct, but the comment 'An empty array ([]) is valid' could be more precise: the entries variable will be nil (not zero-len) when the array is empty, and len(nil) == 0, so make([]vcs.ContentEntry, 0) would be returned instead of nil. This is a behavior difference compared to the populated-array path which returns a non-nil slice.
[NIT] result is allocated with make([]vcs.ContentEntry, len(entries)) which is fine, but it means when entries has len 0 (the empty array case), a zero-length non-nil slice is returned. The doc comment for ListContents doesn't document nil-vs-empty semantics the way GetPullRequestFiles does ('Returns nil (not an empty slice) when...'). Minor inconsistency in documented behavior across sibling methods.
[NIT] strings.NewReplacer is slightly overkill for two simple character replacements. strings.ReplaceAll(strings.ReplaceAll(encoded, "\n", ""), "\r", "") would be marginally clearer and avoids allocating a Replacer object for every call. Minor style preference — the current code is correct.
[NIT] TestGetFileContentAtRef_* and TestGetFileContent_* tests are duplicated between github/files_test.go and github/pr_test.go (e.g. TestGetFileContentAtRef_HappyPath, TestGetFileContentAtRef_EmptyRef, etc. appear in both files). Since both are in package github, this will cause a compile error due to duplicate function names. Looking more carefully at the diff, the pr_test.go file in the diff includes these functions but the files_test.go also does — if both files exist in the same package with the same function names, the package will not compile. This needs to be resolved by removing the duplicates from one file.
[MINOR] GetFileContent is a thin wrapper that just calls GetFileContentAtRef. The doc comment 'Delegates to GetFileContentAtRef with the provided ref' is accurate, but this method exists only to satisfy the vcs.FileReader interface (if that's what it is). If the interface has the exact same signature as GetFileContentAtRef, consider whether GetFileContent should be the primary implementation and GetFileContentAtRef the derivative (or vice versa). As written, the public API surface is slightly confusing — callers see two methods with nearly identical signatures.
[NIT] The test TestDoRequest_RetryAfterDoesNotMutateBackoff skips in short mode with t.Skip("skipping slow retry test in short mode") but the test sets a Retry-After: 1 header which causes a ~1 second delay. This is intentional for the actual retry test, but the 'does not mutate' property could be verified without the real delay by using RetryAfter: 0 or a past date. Minor since the skip guard is appropriate.
[NIT] TestDoRequest_RetryAfterDoesNotMutateBackoff accesses c.retryBackoff directly (a private field) from within the github package test — this works because the test is in package github (white-box test). This is intentional and correct given the test file is in the same package.
[MINOR] After handleResponse returns done=false (the 429 case that will be retried), resp is already closed by handleResponse's defer. The subsequent resp.StatusCode == http.StatusTooManyRequests check on line 258 reads from a closed response, but this is safe because resp is a struct value (not a stream) and StatusCode was already populated. However, the design where handleResponse owns the response lifecycle (via defer) but the caller still needs to inspect resp.StatusCode afterward is a subtle coupling. The caller reads resp.StatusCode on line 258 after handleResponse has already closed the body — this works but is fragile. Consider returning the status code from handleResponse or restructuring to make the ownership clearer.
[MINOR] The retry condition attempt < maxAttempts-1 is evaluated after handleResponse returns done=false. This means on the final attempt (attempt==2), a 429 response falls through to return nil, lastErr correctly. However, the continue on a 429 means the loop increments attempt and hits the backoff sleep at the top — but backoff has only 2 entries for 3 attempts. The indexing backoff[attempt-1] for attempt=1 gives backoff[0] and for attempt=2 gives backoff[1], which is correct. But the guard attempt < maxAttempts-1 (i.e., attempt < 2) means we only continue for attempts 0 and 1, never attempt 2. On attempt 2 a 429 hits return nil, lastErr. This is correct behavior but the interaction between the backoff slice length and maxAttempts is implicit and could silently break if maxAttempts is changed without updating the default backoff.
[NIT] The timer pattern uses timer.Stop() after <-timer.C with a comment 'no-op after fire; kept for symmetry'. The standard Go idiom from the docs recommends this for correctness in case of concurrent resets, but for a non-reset timer this is indeed a no-op. The comment explains the intent — this is fine as written.
[MINOR] The dual-unmarshal fallback for array vs. object in ListContents is correct, but the error variable shadowing with err and err2 is slightly awkward. The outer err from array unmarshal is passed into the error string but err2 is wrapped with %w — this means only the object-unmarshal error is unwrappable. Consider wrapping both or using a different format. Low impact since callers only check for nil.
[NIT] In doRequest, after the handleResponse call, there's a check if resp.StatusCode == http.StatusTooManyRequests but resp could theoretically have been closed by handleResponse. The body is closed, but the resp.StatusCode field is still accessible on the struct. This is correct and safe in Go's net/http — just worth being aware of.
[MINOR] After c.httpClient.Do(req) returns an error, the function returns immediately with a wrapped transport error. However, the body is not closed because resp is nil on transport error — this is correct, but deserves a comment for future readers since handleResponse uses defer for body close, it's not obvious why there's no defer here.
[MINOR] The handleResponse method is defined on *Client but uses no client state (c receiver is unused). This could be a package-level function. Minor since it doesn't affect correctness, but slightly misleading about the dependency on client state.