[NIT] In doGetLimited, the clamp comment says 'Clamp to prevent integer overflow when maxBytes == math.MaxInt64' but the guard `if maxSize < 0
[NIT] The doc comment for MaxDiffSize says 'Set to any negative value (or math.MaxInt64) to disable the limit.' This is slightly awkward API design — math.MaxInt64 is a magic value that happens to bypass the overflow protection, not an intentional disable signal. The docstring documents it correctly but it would be cleaner if the disable path were only a negative value. However, since the math.MaxInt64 bypass exists to defend against a subtle overflow in doGetLimited (where maxBytes+1 would wrap), documenting both is the right call for transparency.
[NIT] Struct field alignment: httpClient *http.Client is not aligned with the other fields (baseURL and token). gofmt will handle tab alignment within struct definitions, but the blank line between token and httpClient creates a visual group separation that might be intentional (separating identity fields from transport config). Either way this is fine.
[MINOR] timer.Stop() is called after <-timer.C has already fired, which is harmless but unnecessary. The canonical pattern for a one-shot timer in a select is: defer timer.Stop() before the select statement. In the current code, calling timer.Stop() after draining the channel is a no-op. This is not a bug, just slightly misleading.
[MINOR] The Retry-After override logic stores the delay at backoff[attempt] but the delay is consumed at backoff[attempt-1] on the next iteration (when attempt-1 == current_attempt). Since the current attempt is 0 when we first hit a 429, the delay gets stored at backoff[0] and consumed when attempt=1 reads backoff[0]. This happens to be correct, but the off-by-one relationship between where the delay is stored and where it is read is subtle and not explained in a comment. A comment clarifying that backoff[attempt] is the delay before attempt+1 would help future maintainers.
[NIT] The TestDoRequest_ContextCanceled test uses time.Sleep(50 * time.Millisecond) to create a timing window, which is a potential source of flakiness on very slow CI machines. A more robust approach would use a channel to signal that the select is about to be entered. However, since the retry backoff is 10 seconds and the sleep is 50ms, the window is generous enough to be reliable in practice.
[NIT] w.Write([]byte(...)) return values are silently ignored in test handlers. In tests this is fine, but adding //nolint comments or using fmt.Fprint might suppress any linter warnings about unchecked errors. Not a real issue.
[MINOR] The now field is unexported and directly mutated in tests (c.now = func() time.Time { ... }). Per the documented pattern in concurrency.md and the conventions (SetHTTPClient / SetRetryBackoff are "test setup only" with no synchronization), this is acceptable. However, unlike SetHTTPClient and SetRetryBackoff, there is no setter for now, so tests must access the unexported field directly — which requires white-box tests in the same package. Since tests are in package github (not package github_test), this works fine. Just noting the inconsistency: the two other test-setup fields have exported setters, but now does not.
[MINOR] The default branch in the VCS client switch (lines ~191-194) is unreachable: the provider has already been validated to be 'gitea' or 'github' by the switch at line ~112. The os.Exit(1) here is dead code. Consider removing it or replacing with panic('unreachable') to make the intent explicit, since a future refactor might add a new provider to the validation switch without updating the client switch.
[MINOR] The supersedeOldReviews function uses a type assertion client.(*gitea.Adapter) in what is supposed to be a provider-dispatch function. This leaks the concrete Gitea type into logic that should be abstracted by the vcs.Client interface. The comment acknowledges this is 'guaranteed to succeed', but it's a coupling smell: cmd/main.go now imports and depends on gitea.Adapter internals for a Gitea-specific code path. This is acceptable for now given the Gitea-specific EditComment/ResolveComment/GetTimelineReviewCommentIDForReview calls have no VCS-agnostic equivalent, but it should be noted as technical debt if additional providers are added.
[NIT] The --base-url flag comment says 'defaults to https://api.github.com' but the actual default applied by the flag is empty string, with the defaulting logic deferred to the client-init switch. The flag help text is technically correct but could confuse users inspecting --help output, since the flag's zero value appears as empty. Consider either setting the default directly in the flag registration (conditional on provider) or clarifying the help text.
[NIT] In ListReviews, the page-limit warning if page == maxReviewPages fires at the end of the last iteration but after the len(reviews) < reviewsPerPage break check. The comment correctly explains this only fires when the final page is full, but the warning fires before returning — the loop will exit naturally after this iteration without an explicit break. This is correct but slightly subtle; a break after the warning would make control flow clearer.
[NIT] json.NewEncoder(w).Encode(...) in the test handler ignores its return error. This is a minor inconsistency with the project's convention to check all error returns, although it is common practice in httptest handlers. Low priority but worth noting for consistency.
[NIT] In TestPostReview_HappyPath, the test handler ignores the error from io.ReadAll (body, _ := io.ReadAll(r.Body)). While unlikely to fail in a test server context, following the project convention of checking all error returns would be more consistent. The test would still exercise the same logic with body, err := io.ReadAll(r.Body); if err != nil { t.Fatalf(...) }.
[NIT] DeleteReview passes nil to doRequestWithBody, which results in no Content-Type header being set. This is correct behaviour per the comment, but it also means no body is sent at all. The DELETE endpoint for GitHub reviews does not require a body, so this is fine — but it's slightly unusual to call doRequestWithBody with nil when doGet (a GET-only wrapper) pattern exists. Consider whether a doDelete or doRequestNoBody wrapper would be clearer, or at minimum a brief doc comment on why doRequestWithBody(nil) is the right call here (a comment is already present, so this is purely cosmetic).
[NIT] The doRequest signature has accept string as a named parameter after reqURL string. Per the style patterns, prefer consistent parameter naming. Minor: the internal copy of backoff via append([]time.Duration(nil), c.retryBackoff...) is idiomatic but slightly obscure; slices.Clone (Go 1.21+) would be clearer if the project targets that version.
[NIT] w.Write(...) return values are ignored in test handlers throughout. This is standard for httptest handlers, but adding //nolint or accepting the convention is fine. Not a real issue.