[MINOR] The slog.Warn call for the 'option ignored' case concatenates strings at call time ("WithAllowInsecureHTTP option ignored: set "+envAllowInsecure+"=1 to enable"). This is a trivial allocation on an error path so it has no practical impact, but idiomatic slog usage would pass the env var name as a structured attribute rather than embedding it in the message string, e.g. slog.Warn("WithAllowInsecureHTTP option ignored", "hint", "set "+envAllowInsecure+"=1 to enable") or a literal string. This is purely stylistic.
[NIT] The testBypass field on clientConfig is an internal implementation detail that controls whether the env gate is bypassed. Since clientConfig is unexported and only accessible within the package, this is fine. However, the field comment // skip env gate (for tests only) could be strengthened to note that only WithAllowInsecureHTTPForTest (defined in export_test.go) should ever set this field, to make it clear no production code path should touch it.
[MINOR] hasHTTPSScheme uses strings.EqualFold for the scheme prefix check. Since "https://" contains only ASCII characters, EqualFold is correct but adds a small amount of unnecessary overhead compared to a direct strings.ToLower or a case-sensitive check (HTTP schemes per RFC 3986 are case-insensitive but in practice always lowercase in this codebase). This is purely a performance NIT on a non-hot path and does not affect correctness.
[MINOR] NewClient reads os.Getenv at construction time, which makes the env-gate check susceptible to a subtle race: if a caller sets the env var after program startup but before calling NewClient (rare but possible in CLI tools), the behavior is correct, but the deeper issue is that env reads in constructors make testing harder and can be surprising. The pattern is pragmatic here and t.Setenv handles it in tests, but worth noting that the test-bypass field (testBypass) in clientConfig is part of the exported clientConfig type — if clientConfig is ever exported or accessed via reflection, testBypass would be visible. Since it's unexported, this is fine as-is.
[NIT] The file uses package github (not package github_test), which is correct for the export_test.go pattern — it compiles only in test binaries and can access unexported types. This is well-documented in the comment. No issue; just confirming the pattern is applied correctly per the testing-advanced.md pattern #11.
[NIT] TestDoRequest_AllowsHTTPWithoutToken passes AllowInsecureHTTPForTest() even though the token is empty and the HTTPS check is conditioned on c.token != "". The test is slightly misleading: it would pass without AllowInsecureHTTPForTest() at all. Consider removing the option from this test to better document that no-token HTTP is allowed unconditionally, which is the actual behavior being tested.
[MINOR] AllowInsecureHTTPForTest() is exported and production code could accidentally call it. The function name and doc comment make the intent clear, but consider placing it in a test-only file (e.g., export_test.go or client_testing.go with a build tag) to make the 'tests only' constraint enforceable by the compiler rather than by convention. At minimum, the export_test.go pattern (pattern #11 in testing-advanced.md) would confine this symbol to test binaries. This is a minor issue since the doc comment is clear, but it does add a small API surface risk.
[NIT] NewClient now reads os.Getenv at construction time and calls slog.Warn, creating a side effect (log output) when constructing a client with AllowInsecureHTTP() but without the env var. The conventions doc says 'never panic; return errors' and the pattern docs suggest constructors should avoid surprising side effects. The silent-ignore + warn behavior is a deliberate design choice documented in the PR, but callers who pass AllowInsecureHTTP() will be silently downgraded to a restricted client with no feedback other than a log line — a future caller who forgets the env var in production will get unexpected 'refusing to send credentials' errors from doRequest that don't clearly indicate the option was silently ignored at construction. Returning an error from NewClient or using a different API shape (e.g., requiring the env-gate check to be explicit) would be cleaner, but this is a design tradeoff, not a bug.
[NIT] The doc comment on PostReview still says event should be "APPROVED" or "REQUEST_CHANGES", but the integration test and production code also pass "COMMENT". The comment is a pre-existing inaccuracy but the PR touches this function's doc block and is a good opportunity to fix it (e.g. event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT").
[NIT] The comment block and both var _ vcs.ReviewerSelfRequester = (*Adapter)(nil) and var _ vcs.ReviewSuperseder = (*Adapter)(nil) are declared in the same file but separated by the SupersedeReviews implementation. The compile-time assertion for ReviewerSelfRequester is near line 19, while the one for ReviewSuperseder is mid-file (line 233). Convention (per patterns/style.md and stdlib) is to place all var _ Interface = ... assertions together near the top of the file for discoverability.
[MINOR] doJSONRequest is added to client.go but is only used internally by review.go methods. Placing transport-layer helpers (doRequestCore, doRequestWithBody) together with a higher-level JSON serialisation helper makes the file slightly inconsistent. This is a minor organisation concern — a comment grouping the helpers or moving doJSONRequest to review.go would improve cohesion, but the current placement is not wrong.
[NIT] The comment // Backward-compatible alias: --gitea-url shares vcsURL's pointer (last flag wins). is accurate but slightly misleading: with flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...) the last parsed flag wins at parse time. If both --vcs-url and --gitea-url are supplied, the rightmost one on the command line takes effect. This is the correct and intended behaviour, but a small clarification in the comment ("last supplied on the command line wins") would help future readers.
[MINOR] When len(oldReviews) > 0 and the client does not implement vcs.ReviewSuperseder, the code logs an error (slog.Error) but does NOT call os.Exit(1). This silently continues after reporting a failure, which is inconsistent with every other slog.Error call in main() that is followed by os.Exit(1). The new review has already been posted at this point, so the old reviews will remain un-superseded without any caller-visible error. Either add os.Exit(1) or downgrade to slog.Warn if non-fatal is intentional.
[MINOR] The panic("unreachable: ...") in the default case of the VCS client switch violates the project convention "Return errors; never panic" (from CONVENTIONS.md). Although the validation at line 104 does make this branch genuinely unreachable at runtime, the convention is unconditional. A cleaner alternative is slog.Error(...); os.Exit(1) — consistent with every other fatal error path in this file — or the validation could be made to work through a single code path. The panic is acceptable as a defensive measure (and correctly named 'unreachable'), but it deviates from stated conventions.
[NIT] The duplication comment says Changes here must be mirrored there but there is no enforcement mechanism (no shared test, no lint rule, no CI check). This is a maintenance risk. A follow-up to extract this into a shared internal/ package would eliminate the drift risk entirely. The PR description acknowledges this is intentional due to package separation, but the internal/ pattern exists precisely for this use case.