[NIT] The TestYAMLDeeplyNestedRejection comment says 'Each levelN mapping adds +1 depth (MappingNode → MappingValueNode → value)' but the trace shows MappingValueNode contributes depth via both Key and Value children. The actual depth accumulation might differ from the comment's description depending on how goccy/go-yaml structures the AST for nested maps. The test uses 25 levels with a 'comfortable margin' which compensates, but the depth trace comment may be imprecise.
[MINOR] The Retry-After override mutates the backoff slice in-place. When c.RetryBackoff is non-nil (set by tests or callers), backoff is a reference to the same slice. This means a Retry-After header on attempt 0 will permanently modify c.RetryBackoff[0] on the caller's struct, which is surprising shared-state mutation. Either copy the slice at the start of doRequest, or use a local variable for the overridden delay rather than mutating the slice.
[NIT] TestDoRequest_429RetryAfterHeader waits 1 full second (Retry-After: 1) making it a slow test. The test comment acknowledges this, but consider using testing.Short() to skip it or documenting it as intentionally slow.
[NIT] strPtr helper is defined in pr_test.go but the same helper could be needed in other test files. Since all files are in package github (internal tests), this is fine as-is but worth noting for future maintainability.
[MINOR] GetFileContent's signature includes a ref parameter but the doc comment says 'Delegates to GetFileContentAtRef with an empty ref', which contradicts the actual implementation (it passes ref through). The doc comment is wrong and should say 'Delegates to GetFileContentAtRef with the provided ref'.
[MINOR] mapCheckRunStatus has a dead code path: case "in_progress", "queued" will never be reached because conclusion == nil is checked first (returning "pending"), and when conclusion is non-nil it cannot be "in_progress" or "queued" (those are status values, not conclusion values in the GitHub API). This case is unreachable and misleading.
[MINOR] RetryBackoff is an exported field on Client, which exposes an internal implementation detail and creates a testing surface that's somewhat awkward — callers might accidentally rely on mutation semantics. A more idiomatic approach would be a functional option (e.g. WithRetryBackoff) or keeping it unexported with a test helper. That said, the comment explicitly marks it as a test hook, which mitigates this concern.
[MINOR] In mapCheckRunStatus, the cases "in_progress" and "queued" are in the switch *conclusion block, but conclusion is non-nil when this switch is reached. GitHub's API sets conclusion to null for in-progress/queued check runs, not to the strings "in_progress" or "queued" — those are status field values. So these cases are dead code and will never be reached. The nil-conclusion check at the top already handles the pending case correctly.
[NIT] The GetFileContent signature accepts a ref parameter and delegates to GetFileContentAtRef, making GetFileContent essentially an alias. The doc comment says 'Fetches a file from the default branch' but it actually passes through whatever ref is given. If the interface requires GetFileContent to always use the default branch, the ref parameter shouldn't be accepted (or it should be ignored). If it accepts a ref, the doc comment is misleading.
[NIT] The http field name shadows the net/http package import within method bodies. While Go resolves this correctly (field access via c.http vs package via http.MethodGet), using a field name that matches an imported package name reduces readability. Consider renaming to httpClient or hc.
[MINOR] The errorAs function is a hand-rolled reimplementation of errors.As. The comment says it's to 'avoid import cycle issues', but there are no obvious import cycles here — errors is a stdlib package with no dependencies. This should simply use errors.As(err, target) directly. The custom implementation handles the Unwrap() error interface but doesn't handle Unwrap() []error (multi-errors from errors.Join), so it's also less complete than the stdlib version.
[MINOR] The Retry-After handling mutates the backoff slice in-place: backoff[attempt] = time.Duration(seconds) * time.Second. When c.RetryBackoff is non-nil (e.g. in tests), this modifies the caller's slice, which is surprising and could cause test pollution if the same slice is reused. A local copy should be made before mutation, or the mutation should only apply to the local backoff variable (which it does when RetryBackoff is nil since a new slice is allocated, but not when it's non-nil).
[NIT] The section separator comments (// --- PRReader ---, // --- FileReader ---, // --- Reviewer ---, // --- Identity ---) are not idiomatic Go. The standard approach is to let the godoc comments on each method group naturally, or split into multiple files by concern. That said, this is a minor style issue.
[NIT] The comment // Wait: first hunk has pos 1(@@ hdr), 2(" line1"), 3("-old"), 4("+new") is a left-over from the author working through the arithmetic. The 'Wait:' prefix makes it look like a mistake in the comment itself rather than an explanatory note. It should be rephrased or removed.
[NIT] The maxPosition method is a one-liner that just indexes the map and is only called from Translate. It adds a layer of indirection without adding clarity or encapsulation. Inlining pm.maxPositions[file] directly in Translate would be marginally clearer, though this is entirely a style preference.
[NIT] The comment says '+++ b/ is checked before +++ /dev/null — the two prefixes are non-overlapping... so ordering is independent' but then immediately says 'Checking the common case first for clarity.' The comment is slightly contradictory: if ordering is independent, the justification doesn't need the non-overlapping note. The comment could be simplified to just explain intent (common case first for readability).
[MINOR] The unmarshalYAMLWithDepthLimit function does a two-pass approach: first parser.ParseBytes then yaml.NewDecoder(...).Decode(out). This means the raw bytes are parsed twice. The second decode with yaml.Strict() also re-parses from a bytes.NewReader(data). For the file size constraint (64KB max), this is acceptable, but worth noting. A minor concern is that the two passes could theoretically behave differently if the library has any parse-then-decode divergence. However, this is standard practice for AST-pre-validation patterns and is not a bug.