[NIT] The comment // else: matching SHA is a no-op by design is helpful but slightly inconsistent with Go style (comments above the code they describe, not as trailing-else). Minor style point with no functional impact.
[NIT] Minor alignment inconsistency in the ReviewRequest struct: Body has no trailing spaces before the backtick tag, while Event and CommitID have extra whitespace for alignment. gofmt normalises struct field alignment automatically, so this is harmless, but worth noting in case the file is not being run through gofmt as part of the build.
[NIT] Double blank line between TestAdapter_PostReview_EventTranslation and TestAdapter_PostReview_CommitID. Go convention (enforced by gofmt) uses a single blank line between top-level declarations. This also appears at the end of the TestPostReview_EmptyCommitID_OmittedFromPayload function in gitea/client_test.go (double blank before TestGetPullRequest_Non200).
[NIT] Inconsistent alignment: CommitID string has extra whitespace padding (string json:...) that doesn't match the adjacent Event ReviewEventfield. Likely a gofmt artefact from editing — should be cleaned up sogo vet/gofmt` stays clean.
[NIT] The doc comment for AllowInsecureHTTP() has an overly long line: 'For tests, use AllowInsecureHTTPForTest (defined in a _test.go file in the same package) which bypasses the env gate.' This wraps beyond 80 chars and doesn't follow the conventional line-length style seen elsewhere in the file, though this is cosmetic only.
[NIT] The HTTP rejection error message says 'use HTTPS or set AllowInsecureHTTP option'. For users who call AllowInsecureHTTP() and see it silently fall back (env var not set), this message may be confusing — they did set the option but it was ignored. A slightly more helpful message might mention the REVIEW_BOT_ALLOW_INSECURE env var. Minor UX nit.
[NIT] The warn log 'AllowInsecureHTTP ignored: set REVIEW_BOT_ALLOW_INSECURE=1 to enable' fires silently in tests that run in environments where the env var isn't set. Since NewClient is called in many tests (e.g. TestNewClient_DefaultBaseURL, TestParseRetryAfter_* etc.) without AllowInsecureHTTP(), this won't fire there. But any caller using AllowInsecureHTTP() in a test without the env var will produce a slog.Warn to whatever slog default is configured. The AllowInsecureHTTPForTest bypass prevents this for test servers, so this is acceptable — just worth noting that the warn will fire in non-test environments where someone misconfigures the option.
[MINOR] redactURL sets u.Fragment = "" unconditionally. Fragments are not sent over the wire in HTTP requests (RFC 7230), so stripping them is harmless but adds noise. Not a bug, but the fragment stripping is unnecessary for this use case (URL in error messages for HTTP requests).
[MINOR] The doc comment for AllowInsecureHTTP() says 'For tests, use AllowInsecureHTTPForTest (defined in export_test.go)' but export_test.go is actually in the package itself (package github), not a separate file the doc comment navigation implies. This is fine technically but the phrasing slightly misrepresents the location — it's defined in a _test.go file in the same package. Minor wording issue only.
[NIT] TestNoInsecureOption_RejectsUppercaseHTTP and TestNoInsecureOption_RejectsMixedCaseHTTP don't start an httptest.Server — they just pass 'HTTP://example.com/test' directly. These tests don't need a real server (good), but using an unreachable external host in test code is a minor smell. Consider using 'HTTP://127.0.0.1:1/test' or similar to make it obvious no real connection is attempted. Not a bug since the scheme check fires before any TCP connection.
[NIT] The redactURL function has a dead code path: if u.User != nil { u.User = nil }. The nil check is unnecessary because assigning nil to a nil pointer is a no-op. This works correctly but is slightly misleading. Could simplify to u.User = nil.
[NIT] The doc comment for AllowInsecureHTTP() says 'silently ignored' but the code actually logs a slog.Warn — it's not silent. The comment should say 'ignored with a warning log' to match actual behavior.
[NIT] The HTTP scheme check in doRequest parses the URL twice when the URL is also valid for the request — once here in the guard and implicitly again by http.NewRequestWithContext. This is a minor inefficiency in the hot path but not a correctness issue.
[MINOR] The env-gate silently ignores AllowInsecureHTTP (logs a warning but doesn't return an error) when REVIEW_BOT_ALLOW_INSECURE is absent. This is a deliberate design choice documented in the PR, but it's a subtle failure mode: a developer who calls AllowInsecureHTTP() expecting HTTP to work will instead get a 'refusing HTTP request' error from doRequest with no obvious connection to the silent warning at construction time. Returning an error from NewClient or panicking at construction time would make the misconfiguration immediately obvious. This is a design trade-off rather than a bug, but worth noting.