[MINOR] The timer in the retry loop is not stopped in the happy path (when <-timer.C fires). After time.NewTimer fires via <-timer.C, the timer is already expired and GC will collect it, but the idiomatic Go pattern is to always call timer.Stop() after a select to be safe and clear. Pattern from concurrency.md: prefer explicit resource cleanup. Low risk here since the timer has already fired, but idiomatic code calls Stop() regardless.
[NIT] The struct field http *http.Client shadows the net/http package name within the struct definition. While Go allows this and the compiler handles it correctly, it's a minor readability concern. A name like httpClient or hc would avoid the shadowing.
[MINOR] GetFileContent is defined in files.go but it's a pure delegation to GetFileContentAtRef which lives in pr.go. Both implement the FileReader interface, but splitting the delegation from the implementation across files is mildly confusing. Both could live in files.go or the delegation could be removed and GetFileContentAtRef used directly. Minor organization issue.
[NIT] strPtr helper is defined in pr_test.go but files_test.go is in the same package and could theoretically need it. Both are in package github (white-box tests), so there's no duplication issue here. However, there is a duplicate strPtr in neither file — only pr_test.go has it. Fine as-is.
[MINOR] RetryBackoff is an exported field on Client, which makes it part of the public API surface. This is used as a testing escape hatch ('Set to shorter durations in tests'), but the pattern documented in the codebase for HTTP client injection is SetHTTPClient(). Exposing RetryBackoff publicly means callers can set arbitrary production retry delays, which may be intentional but conflates test and production configuration. A SetRetryBackoff method or keeping it package-internal with a test-only setter via export_test.go would be cleaner, though the current approach is pragmatic.
[NIT] Three separate const declarations could be grouped into a single const (...) block per the style pattern, though this is a pure style nit and not a correctness issue.
[MINOR] mapCheckRunStatus ignores the second parameter (status string) entirely — it's named _. The function signature accepts the status for potential future use, but the unused parameter with a blank name could be confusing. If the status field (e.g. 'in_progress', 'queued') is never needed, the parameter should be removed from the signature; if it may be needed, a comment explaining why it's currently unused would help. The current behavior maps nil conclusion to 'pending' regardless of status, which may be correct, but 'stale' and 'waiting' conclusions (which GitHub also uses) would silently fall through to 'pending' rather than being explicit.
[MINOR] When c.http.Do(req) returns an error (network failure, context cancellation), the function returns immediately without retrying. For transient network errors, a retry could be valuable. The current behavior is reasonable for the stated scope (only retry on 429), but the comment says 'It respects the Retry-After header when present' without mentioning the no-retry-on-transport-error behavior. This is a documentation gap rather than a bug.
[NIT] The mapCheckRunStatus function accepts a second parameter _ string (the status field) but ignores it. The function signature signature documents intent to use status but doesn't. In practice, status (e.g., 'in_progress', 'queued') could provide additional fidelity when conclusion is nil. The ignored parameter adds dead weight to the function signature. Either use it or remove it.
[NIT] GetFileContent doc says 'Delegates to GetFileContentAtRef with the provided ref' — this is accurate but the function adds no value over calling GetFileContentAtRef directly. This is presumably required to satisfy a FileReader interface. If that's the case, a comment noting it satisfies the interface would help readers understand why this thin wrapper exists.
[NIT] strPtr is defined in pr_test.go (package github). The same helper (or a very similar one) might be useful in other test files. Consider moving it to a shared test helper file (e.g., helpers_test.go) within the package to avoid potential duplication if other test files need it.
[MINOR] The exported RetryBackoff field is an exposed implementation detail intended primarily for testing. The doc comment says 'Set to shorter durations in tests', but exposing it publicly allows any caller to mutate it after construction. A cleaner approach for the production API would be a WithRetryBackoff([]time.Duration) option or making it unexported and providing a test helper. That said, the test TestDoRequest_RetryAfterDoesNotMutateBackoff correctly verifies the internal copy-on-use behavior, so the current design is safe — just not idiomatic for a public field that is primarily a test hook. Per configuration pattern #2 (Options Struct), test-only configuration hooks should typically not be part of the public API surface.
[MINOR] The SetHTTPClient method is documented as 'intended for testing to inject mock transports', making it a test-only escape hatch on the public API. Per the package design pattern for internal/ packages, test-only hooks that are not part of the intended public contract ideally belong in export_test.go or a test helper. However, since this is a new package and the project may not yet have an export_test.go pattern established, this is a minor concern.
[MINOR] The unmarshalYAMLWithDepthLimit function performs a two-pass decode (AST parse then full decode). The second pass with yaml.Strict() will also parse the YAML from scratch, so the file bytes are parsed twice. This is intentional per the comment but worth noting: if the goccy/go-yaml Strict() decoder also does alias resolution internally, the depth protection from the first pass only guards the structural AST walk, not the decoder's internal expansion. This appears acceptable given the design, but should be validated that yaml.Strict() doesn't recurse unboundedly on crafted alias chains during decode.
[NIT] The PR description acknowledges that updating CONVENTIONS.md and implementation in the same PR deviates from the documented two-step process. The justification (pre-approval in issue #57) is reasonable, but it sets a precedent for bypassing the process. A brief inline comment in CONVENTIONS.md explaining the deviation and referencing issue #91 for the audit trail would make this cleaner for future maintainers.
[NIT] The AnchorNode case increments depth for the anchor definition. The comment explains this is intentional and asymmetric. However, this means an anchored value at the top level that nests 10 levels will consume 11 depth budget (1 for anchor + 10 for content), and when aliased at depth 5, the alias expands to depth 5+1+10=16. The comment says 'combined budget is halved' which isn't quite accurate — it's more nuanced. The comment could be clearer, but this doesn't affect correctness.
[MINOR] The validated map uses ast.Node (interface) as a key, which relies on pointer equality for interface comparison. This works correctly for pointer types like *ast.MappingNode, but if any ast.Node implementation is a value type (non-pointer), two structurally identical nodes could hash to different entries or the same entry incorrectly. In practice, all goccy/go-yaml AST node types appear to be pointer types, so this is low risk, but it's an implicit assumption worth a comment.