[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] The timer pattern timer.Stop() // no-op after fire; kept for symmetry in the case <-timer.C branch is slightly misleading. Stop() after a channel receive is indeed a no-op since the channel has already fired, but the comment could be clearer: timer.Stop() after <-timer.C does nothing and could simply be omitted. This is a readability nit, not a correctness issue.
[MINOR] After a successful response read, resp.Body.Close() is called after io.ReadAll but not in a defer. If the code path between resp.Body.Close() and the size check were to grow (e.g., someone adds an early return), the body could leak. The pattern defer resp.Body.Close() immediately after c.httpClient.Do(req) is the idiomatic Go pattern per the style patterns (defer for resource cleanup). Currently safe but brittle.
[MINOR] The error body path also calls resp.Body.Close() explicitly rather than via defer. Same concern: if a future edit inserts an early return between reading the body and closing it, the body leaks. Structuring this as defer resp.Body.Close() right after Do() would be more robust. The current approach requires correctly handling both success and error paths manually.
[MINOR] The HTMLURL field name in checkRunsResponse deviates from the Acronym Capitalization pattern (style.md §2) which mandates all-caps acronyms: it should be HTMLURL for an unexported struct field following htmlURL convention, but since this is an unexported type used only for JSON unmarshaling, the real concern is cosmetic. However HTMLURL (two consecutive all-caps acronyms) is actually the correct form per Go conventions — this is not a real issue. Noting for completeness: the field name is correct.
[NIT] The derefString helper is a small utility that could theoretically live in a shared internal utilities package if it's needed elsewhere. For now it's fine in pr.go but if the codebase grows, consider an internal/ package. Not a current issue.
[MINOR] The escapePath function silently removes dot-segments and empty segments rather than returning an error. The doc comment acknowledges this is intentional and explains the rationale. However the behavior (caller gets a different path than requested without error) could lead to subtle bugs where callers don't notice path traversal has been cleaned — for example escapePath("../secret") returns "secret" silently. Since this is documented behavior and the convention explicitly calls it out, this is a design trade-off rather than a bug, but it warrants attention if paths come from user input.
[NIT] The table-driven TestGetCommitStatuses_CheckRunConclusions test creates a new httptest.Server for each subtest case rather than parameterizing the handler. This works but is slightly heavier than necessary. A single server whose handler reads from tt via closure capture (the subtests are sequential since t.Run without t.Parallel() runs sequentially) would be cleaner and follow the table-driven pattern more closely. Minor style concern.
[MINOR] The mapCheckRunStatus mapping treats cancelled, skipped, and neutral as "success" (non-blocking). This is a policy decision that should be documented more prominently — cancelled in particular could reasonably be mapped to failure in some contexts. The comment // non-blocking is brief; a note explaining the design rationale (e.g. 'these do not indicate a blocking failure per GitHub's check suite semantics') would help future maintainers.
[MINOR] The fallback logic for ListContents when the array unmarshal fails uses the error from the object unmarshal (err2) as the returned error, discarding the original array unmarshal error. This could produce a confusing error message if the response was genuinely a malformed array rather than a single-file object. The original err might be more informative in the pure malformed-JSON case.
[NIT] stringPtr helper is defined in pr_test.go (package github) but TestGetCommitStatuses_CheckRunConclusions also uses it. Since tests are in the same package, this is fine, but the helper could be placed in a shared test helper file to avoid potential future duplication if more test files are added to the package.
[NIT] The APIError.Body field is exported (Body string) which means callers can directly access potentially sensitive upstream response content. The doc comment warns against this, but since APIError is a public type, there's no enforcement. This is acceptable given the documentation but worth noting as a conscious design trade-off.
[NIT] The doRequest method signature uses a positional accept string parameter rather than a functional option or an options struct. For internal use only (called from doGet and GetPullRequestDiff) this is fine, but if the API surface grows (e.g. needing custom headers), this approach will require signature changes. No action needed now, but a comment noting it's intentionally internal would be helpful.
[MINOR] The timer.Stop() call after <-timer.C is a no-op (timer already fired) and the comment acknowledges this. While harmless, it's slightly misleading. More importantly, timer.Stop() is not called on the ctx.Done() path before returning — it is called, which is correct. The no-op case is fine to keep for symmetry but the comment 'releases runtime resources promptly' is inaccurate since the timer has already fired and its resources are already released.
[MINOR] In GetCommitStatuses, when the check-runs endpoint fails, the function returns nil, err but has already accumulated commit statuses in result. The doc comment says 'If the commit statuses endpoint fails... the function returns immediately without attempting the check-runs endpoint' but doesn't document what happens when the check-runs endpoint fails after statuses succeed. This is a partial-result scenario that callers cannot distinguish from a total failure. Consider either returning the partial result or documenting this behavior explicitly.
[NIT] The fallback for {} (empty object) guards against single.Name == "" && single.Path == "" && single.Type == "". A partial response with only type set (e.g. {"type":"file"}) would pass this guard and be returned as a valid entry with empty name/path. This edge case is unlikely from GitHub but the guard could be tightened to single.Name == "" alone since Name is the minimal required field.