[MINOR] Timer leak on the happy path of the retry loop. When timer.C fires normally (case <-timer.C), timer.Stop() is never called. The timer has already fired so this is not a resource leak in the traditional sense, but idiomatic Go calls timer.Stop() on all branches to release the internal runtime resources promptly. The fix: case <-timer.C: timer.Stop() or use defer timer.Stop() before the select.
[NIT] In GetCommitStatuses, if the commit statuses endpoint returns an error (e.g., 404), the function returns immediately without attempting the check-runs endpoint. Depending on the intended semantics, callers may expect partial results or a combined error. The current behavior (fail fast) is reasonable but undocumented.
[MINOR] The GetPullRequestFiles pagination loop does not return an empty non-nil slice when there are no files (the first page returns 0 items), it returns nil. This is idiomatic Go for slices but may surprise callers who do len(files) == 0 vs a nil check. Not a bug, just worth documenting or ensuring callers handle nil consistently.
[MINOR] Silent empty-array handling: when the GitHub Contents API returns an empty JSON array [], json.Unmarshal into []entry succeeds with len(entries) == 0 and the function returns an empty slice without error. This is probably the correct behavior for an empty directory, but it's undocumented and could silently mask an unexpected server response. A comment clarifying this intent would be valuable.
[NIT] The escapePath function silently drops empty segments produced by consecutive slashes (e.g., a//b becomes a/b). The existing documentation mentions dot-segment removal but not double-slash collapsing. Minor doc gap.
[NIT] The concurrency safety doc comment says "SetHTTPClient and SetRetryBackoff must not be called concurrently with requests" but these are exported methods with no enforcement (no mutex, no atomic). This is an acceptable design for a test-support method, but the comment could be stronger: e.g., "These methods are intended for test setup only and must be called before any goroutines issue requests."
[NIT] The unmarshalYAMLWithDepthLimit function takes maxDepth int as a parameter but always calls checkYAMLDepth with the package-level constant MaxYAMLNodes directly rather than accepting it as a parameter. This asymmetry is mildly inconsistent — either both limits should be constants used directly, or both should be parameters. Low impact since callers always pass MaxYAMLDepth anyway.
[NIT] The deviation comment <!-- Deviation from step 1+3 for go-yaml migration: see #91 for rationale. --> is an HTML comment that won't render in most Markdown viewers, but it does serve as an audit trail. Consider whether a visible note (e.g., a footnote row in the table or a sentence after the table) would be more discoverable for future reviewers. Minor concern only.
[NIT] TestYAMLEmptyFileRejection has a redundant if err != nil guard on the second condition: if err != nil && !strings.Contains(...). Since the preceding if err == nil { t.Error(...); return } (implicit — actually the test doesn't return on the first error, it falls through) — actually the first check is if err == nil { t.Error(...) } without a return, so err could still be nil here. This means the strings.Contains check is guarded correctly with err != nil, but the test won't t.Error the wrong message if err == nil (it will have already errored on the first check). The pattern is slightly unusual compared to the rest of the file which uses early returns. Consistent style would be to add a return after the first t.Error, then drop the err != nil guard.
[MINOR] The timer drain comment '// Timer already fired; Stop() is a no-op here.' is slightly misleading. When the timer fires and we receive from timer.C, Stop() returns false but calling it is still harmless — however the comment may cause confusion since we never call Stop() in this branch. More importantly, after receiving from timer.C, Stop() genuinely IS a no-op (the timer has already fired), but the code never calls Stop() here at all, which is correct. The comment is about something that isn't happening. This is fine but the comment could be clearer or removed.
[MINOR] The shadow variable t on } else if t, err := http.ParseTime(ra); err == nil { shadows the outer t parameter name from the function signature — but this function has no t parameter, so there is no actual shadowing issue here. However, using t as a local variable for a time.Time is confusing since t conventionally means *testing.T in Go. Consider renaming to parsedTime or retryAt for clarity.
[NIT] In TestGetCommitStatuses_CheckRunConclusions, each table entry creates a new httptest server inside a subtest. This is correct but could use t.Cleanup(srv.Close) instead of defer srv.Close() per the testing patterns (defer runs at function end, t.Cleanup runs at subtest end). In practice, defer works correctly here since the server is created inside the t.Run closure function body, but using t.Cleanup is more idiomatic per the documented patterns.
[MINOR] The ListContents function silently accepts {} (empty JSON object) as a valid response — the fallback from array parse failure to single-object parse succeeds for {}, and the single.Name == "" && single.Path == "" && single.Type == "" guard catches this case correctly. However, json.Unmarshal(body, &entries) where body is {} (an object, not array) will return an error, and the fallback will parse it as an empty entry — the zero-value check does catch this. This is correct, but the logic is subtle. A brief inline comment explaining that the zero-value check guards against {} or unexpected object shapes would help future readers.
[MINOR] The mapCheckRunStatus function signature takes a second _ string parameter that is explicitly ignored and documented as intentionally unused. Per the convention patterns, this is acceptable as documentation, but it would be cleaner to either remove the parameter entirely (if the function is only called internally) or keep it for forward-compatibility with a comment. Since it's unexported and only called in one place, removing the dead parameter would be cleaner.
[NIT] The APIError.Body field is exported, meaning callers can access it directly. However, the Error() method truncates to 200 bytes while the Body field contains the full text. This asymmetry is intentional (truncation is for human-readable error messages) but could surprise callers who log err.Error() thinking they see the full body. The existing doc comment addresses this adequately; no code change needed, just noting the asymmetry.
[MINOR] The CheckRedirect lambda is duplicated verbatim in both NewClient and SetHTTPClient(nil). Extract it to a package-level function (e.g., defaultCheckRedirect) to eliminate the duplication and ensure both code paths stay in sync when the policy changes.