[NIT] seen[personaName] = true unconditionally — previously if !seen[personaName] { seen[personaName] = true } was equivalent since writing true to an already-true key is idempotent. The simplification is correct and cleaner. No issue, just confirming this is intentional.
[NIT] Stray blank line between the closing brace of doRequestWithBody and the start of doJSONRequest doc comment — there is also a missing blank line before the closing } of doRequestWithBody (the } is on the line immediately after return). Minor formatting inconsistency that gofmt would not catch but goimports might flag, and it reduces readability.
[MINOR] The vcsURL flag captures the env-var-resolved default via a pointer dereference (*vcsURL) when registering the --gitea-url alias. The comment explains this, but the approach is subtle and fragile: if someone ever moves the flag.StringVar call before flag.String completes or wraps it in a helper, it silently uses the zero-value default. A simpler alternative would be to compute the default value once into a local variable (giteaURLDefault := envOrDefault(...)) and use it for both registrations, eliminating the ordering dependency entirely.
[NIT] Extra blank line between the closing brace of extractSentinelName and the comment for findAllOwnReviews — minor, but inconsistent with the rest of the file's style.
[MAJOR] In PostReview, the commit_id is set from the first comment that has one: if payload.CommitID == "" && comment.CommitID != "". The GitHub PR Reviews API requires that all comments in a single review reference the same commit_id (passed at the top-level request, not per-comment). If comments have different CommitIDs (which is possible since vcs.ReviewComment has a per-comment CommitID field), later comments' CommitIDs are silently ignored. This should either be validated (return an error if CommitIDs differ) or documented clearly.
[NIT] The ListReviews page-limit warning fires inside the loop when page == maxReviewPages, but the loop condition is page <= maxReviewPages, so the final iteration runs after the warning. The log message 'results may be truncated' is correct, but it would be cleaner to check after the loop (if len(allReviews) == reviewsPerPage*maxReviewPages) rather than inside it.
[MINOR] The DismissReview method on the raw *gitea.Client (not the Adapter) returns errors.ErrUnsupported wrapped with context. However, the Gitea Adapter.DismissReview calls c.client.DeleteReview(...) (which actually deletes). There's no code path that calls gitea.Client.DismissReview directly since the Adapter overrides it, but the method still exists on the type, creating confusion. The stub comment 'This is a stub for the vcs.Reviewer interface; full implementation is Phase 2' is incorrect — the Adapter already provides the implementation. This dead/misleading method should be removed or clarified.
[MINOR] BuildLineToPositionMap does not handle the case where currentFile is empty (deleted files with +++ /dev/null or binary files). Unlike gitea.BuildPositionToLineMap, it doesn't have the +++ /dev/null check. If a diff contains a deleted file, the +++ b/ prefix won't appear, but the subsequent hunk/content lines could spuriously be attributed to the previous file since currentFile is never reset to "". This can result in incorrect position mappings.
[MINOR] Go version 1.26 is specified, but as of the PR creation date, Go 1.26 does not exist (latest stable is 1.22/1.23). This will either fail in CI or use a future version. This should be 1.23 or 1.22 (whichever matches go.mod). The go.mod specifies go 1.26.2 which is also invalid — this suggests the go.mod was written speculatively. CI will fail if the GitHub Actions runner cannot find Go 1.26.
[MINOR] GetCommitStatuses makes two separate API calls (commit statuses + check runs) and the documentation says 'If the check-runs endpoint fails after statuses were fetched successfully, the function returns an error (not a partial result)'. This is intentionally strict, but in practice GitHub check-runs require the checks:read permission which not all tokens have. A 403 on check-runs will cause the entire CI status check to fail even though commit statuses succeeded. Consider making check-runs failures non-fatal (log a warning and return only the commit statuses).
[MINOR] gitea/position.go and vcs/util.go both implement diff parsing with similar but subtly different logic (the gitea version maps position→line, the vcs version maps line→position). This duplication is risky. The gitea.BuildPositionToLineMap is only used by gitea.Adapter.PostReview to translate vcs positions back to Gitea line numbers. This round-trip (build line→pos in main.go, then pos→line in Adapter) adds complexity. Consider having the Adapter work directly with line numbers from the original diff parsing, or consolidating the logic.
[NIT] The BuildLineToPositionMap function increments position for deletion lines but does not reset newLine (correctly), however there's a subtle behavioral difference from gitea.ParseDiffNewLines (which was used before). The old code used diffRanges.Contains(f.File, f.Line) which only mapped lines present in the diff. The new code maps ALL new lines in context/addition lines. This is correct for GitHub's diff-position API but may cause changes in which lines are eligible for inline comments compared to the old behavior.
[MAJOR] In PostReview, when there are inline comments, the adapter fetches the diff and builds a position-to-line map, then calls posMap.Translate(c.Path, c.Position) where c.Position is a GitHub diff-position integer. However, the vcs.ReviewComment.Position field is documented as a diff-position, and BuildPositionToLineMap in vcs/util.go maps line_number→position (the inverse direction). The gitea adapter then translates position→line via BuildPositionToLineMap in gitea/position.go (which is the correct direction). This dual/parallel implementation of position maps (gitea.BuildPositionToLineMap returns position→line, vcs.BuildLineToPositionMap returns line→position) is confusing and creates a correctness risk. In main.go, vcs.BuildLineToPositionMap is used to build the comment's Position value, and then the Gitea adapter re-translates that position back to a line number. This double-translation could produce wrong results if the two parsers have any behavioral difference.
[MAJOR] In supersedeOldReviews for the GitHub path, errors.Join(errs...) returns a non-nil error even when errs is empty (it returns nil correctly), but the caller does os.Exit(1) on any non-nil error. If all DismissReview calls fail, this correctly exits. However, the bigger issue is that the function signature says it returns an error, but in the Gitea path it returns nil even when individual review supersede operations fail (they only log warnings and continue). This asymmetry means GitHub failures are fatal but Gitea failures are silently swallowed. Either both should be fatal or both should be best-effort — the inconsistency will surprise operators.
[MINOR] The self-request reviewer logic uses a type assertion client.(*gitea.Adapter) and then calls both client.GetAuthenticatedUser and giteaAdapter.Underlying().RequestReviewer. The GetAuthenticatedUser call is redundant — it could be called on giteaAdapter directly since Adapter implements Identity. More importantly, this is still provider-specific logic leaked into the main flow via type assertion rather than the provider switch. It would be cleaner to check *provider == "gitea" (which is already available) rather than relying on a type assertion.
[NIT] The --gitea-url alias registration comment says 'If a user passes both --vcs-url and --gitea-url, the last one on the command line takes effect'. However, since both --vcs-url and --gitea-url share the same pointer via StringVar, only the last flag parsed wins. This is the documented behavior but may be surprising to users who set GITEA_URL env var and also pass --vcs-url on the command line — the flag will win. This is standard flag package behavior but worth a brief note in the help text.
[NIT] The test case 'math.MaxInt64 treated as disabled' verifies the bypass works, but the test name 'treated as disabled' implies intended behavior rather than an edge-case workaround. A name like 'math.MaxInt64 bypasses limit via overflow protection' would be more precise. Minor naming issue only.