[MINOR] The fallback from array→object JSON parsing uses err from the outer json.Unmarshal in the error return (fmt.Errorf("parse contents JSON: %w", err)), but err here is the array-parse error ('cannot unmarshal object into Go value of type []entry'), which is misleading — the real parse failure is err2. Should return err2 instead: fmt.Errorf("parse contents JSON: %w", err2).
[MINOR] The security check if !c.allowInsecureHTTP && req.URL.Scheme != "https" is performed inside the retry loop, meaning it will fail on every retry attempt rather than being checked once before the loop starts. Since the URL doesn't change between retries, this is wasteful and the error message is slightly misleading (it mentions req.URL.Host but the real issue is the scheme). Moving the check before the retry loop or to NewClient would be cleaner.
[MINOR] After a successful response is read, resp.Body.Close() is called directly after io.ReadAll. If io.ReadAll returns an error (e.g. partial read), the body is still closed via the subsequent line, which is fine. However, the pattern is slightly inconsistent with the error path below it — consider using defer resp.Body.Close() paired with a drain before close on the error path for symmetry. This is purely stylistic; the current approach is correct.
[NIT] The doc comment for Client says SetHTTPClient and SetRetryBackoff must not be called concurrently with requests, but these are public methods and there's no enforcement or noCopy guard. Given the concurrent-use note, a brief comment in SetHTTPClient and SetRetryBackoff reiterating the constraint would help (though this matches the stdlib's tls.Config immutable-after-use convention, so it's acceptable as-is).
[NIT] TestDoRequest_LimitsResponseBody allocates maxResponseBytes+1024 bytes (over 10 MiB) as a string for each test run. The comment acknowledges this but the fix isn't applied — consider using io.LimitedReader in the test handler or generating the body lazily to avoid the 10 MiB allocation in tests.
[NIT] The mapCheckRunStatus function signature takes an ignored second parameter _ string. The comment explains why, which is good. A minor alternative would be to not accept the parameter at all and have the caller omit it, but given this is a private helper called from one place with both values available, the current approach is defensible. The doc comment adequately explains the design choice.
[MINOR] After the timer fires, timer.Stop() is called redundantly. When <-timer.C succeeds, the timer has already expired and Stop() returns false with no effect. It's harmless but misleading — the pattern from the Go docs is to call Stop() only in the cancellation case. Consider removing the redundant timer.Stop() on the success branch, or document why it's there.
[NIT] The struct formatting has inconsistent spacing — baseURL and token fields have no space before the type, but httpClient has extra whitespace alignment. gofmt normally handles this, so it may be a display artifact, but worth verifying the file passes gofmt cleanly.
[MINOR] The checkRunsResponse.TotalCount field is parsed but never used. The pagination termination relies on len(checkResp.CheckRuns) < 100 rather than TotalCount. This is fine for correctness, but the unused field adds noise. Either use it or remove it.
[MINOR] escapePath silently removes .. and . segments rather than returning an error. For a security-sensitive operation (path traversal prevention), silently stripping these is arguably better than erroring, but the caller has no way to know the path was modified. The function is package-private so this is contained, but callers like GetFileContentAtRef will fetch a different path than requested without any indication. A comment explaining the deliberate behavior (not a bug) would improve maintainability.
[NIT] TestDoRequest_LimitsResponseBody tests a constant value rather than actual behavior. The comment acknowledges this limitation. This is acceptable as a documentation-style test, but it adds no real safety guarantee — if maxResponseBytes is set correctly but the io.LimitReader call is removed, the test would still pass. Consider removing it or replacing with a test that actually sends a response exceeding the limit.
[MINOR] The mapCheckRunStatus function maps cancelled, skipped, and neutral to success (non-blocking). This is a meaningful policy decision that warrants a more explicit comment explaining why these are treated as non-failures from the review bot's perspective, rather than just // non-blocking. The current comment doesn't explain the reasoning to future maintainers.
[NIT] strPtr helper is defined in pr_test.go but files_test.go is in the same package (package github). If files_test.go ever needs this helper, there would be a duplicate definition. Consider putting shared test helpers in a helpers_test.go file. Not a current problem since files_test.go doesn't use it, but worth considering for consistency.
[NIT] TestDoRequest_LimitsResponseBody doesn't actually test the limit behavior — it only checks the constant value. The comment acknowledges this. This is a weak test; consider removing it or replacing it with an actual test using a small limit (e.g., setting maxResponseBytes to a small value via a test helper, or accepting that this particular boundary isn't testable without refactoring).
[MINOR] The Client struct has inconsistent field alignment — httpClient is not aligned with baseURL and token. While gofmt doesn't enforce struct field alignment, the blank line between token and httpClient with the comment block is slightly inconsistent with Go conventions. More substantively: SetHTTPClient and SetRetryBackoff are exported methods that mutate the struct, breaking the documented concurrency guarantee ('A Client is safe for concurrent use by multiple goroutines'). These mutation methods are clearly test-only helpers, but they're exported without any documentation caveat about not calling them concurrently with requests.
[NIT] GetFileContent simply delegates to GetFileContentAtRef. The doc comment says 'Delegates to GetFileContentAtRef with the provided ref' which is accurate but redundant given that the implementation is a one-liner. The vcs.FileReader interface likely has a different signature than vcs.PRReader's method — this delegation is the right approach, but the doc comment could explain why this wrapper exists (to satisfy the FileReader interface which has a different signature from GetFileContentAtRef).
[MINOR] The maxPages = 100 constant caps pagination at 100 pages × 100 files = 10,000 files for PRs, and 100 pages × 100 check runs = 10,000 check runs. This constant is shared between two very different concerns (PR files and check runs). A PR with 10,000 files is pathological but possible in generated-code repos; silently truncating without returning an error or warning could cause incorrect reviews. Consider either documenting this limit explicitly or returning an error when the cap is hit.