[MAJOR] doRequestWithBody duplicates ~90% of doRequest's implementation. Both functions share identical backoff setup, HTTPS validation, retry loop, timer/select pattern, Retry-After parsing, and handleResponse invocation. The only difference is that doRequestWithBody constructs a bytes.NewReader(body) and sets Content-Type: application/json. This violates DRY and makes the retry logic a two-place maintenance burden. The fix is to refactor doRequest to accept an optional body (e.g. add an io.Reader or []byte parameter, or introduce a private doRequestInternal that both exported helpers delegate to). The pattern to follow is the existing Layered API pattern: doGet calls doRequest, and doRequestWithBody should also call a single underlying implementation.
[MINOR] The HTTP method is passed as a raw string literal "POST" to doRequestWithBody instead of using the net/http package constant http.MethodPost. The existing doGet wrapper uses http.MethodGet. For consistency, use http.MethodPost, http.MethodDelete, and http.MethodPut in the PostReview, DeleteReview, and DismissReview callers respectively.
[MINOR] translateGitHubReviewState maps "APPROVED" → "APPROVED" and "DISMISSED" → "DISMISSED" explicitly, which is identical to the default passthrough case. These two cases can be removed, leaving only the two non-trivial translations (CHANGES_REQUESTED and COMMENTED). This simplifies the function and makes the intent clearer: only translate states that differ from their canonical representation.
[NIT] The error-message disambiguation (hunk-header vs deletion) occurs after the forward-scan loop, but the check if lineNum == 0 is evaluated only after the loop exhausts. This is correct and readable, but the two error messages could equally be computed before the loop (since lineNum doesn't change). Minor style point only — no functional issue.
[NIT] The error-message discrimination between lineNum==0 and lineNum==-1 is checked after the walk loop, meaning the if lineNum == 0 branch is dead code for the hunk-header error path when there IS a subsequent line — that path returns early. The logic is still correct, but the structure is slightly misleading: the if/else at lines 53-57 would be clearer if written as two separate if lineNum == -1 / if lineNum == 0 fallbacks before the shared loop, or the error strings could simply be unified into one message since both cases have identical semantics from a caller perspective. Minor readability issue only.
[NIT] TestTranslate_HunkHeaderPosition_MultiHunk does not test the edge case where a hunk-header is the last position in the file (i.e., the error path for a hunk-header with no subsequent line). The deletion equivalent is covered by TestBuildPositionToLineMap_DeletionAtEnd but there is no corresponding test for the new hunk-header error path. Not a correctness issue since the code path is trivially reached from the shared loop, but worth noting for completeness.
[NIT] The dual-unmarshal fallback (try array, then object) silently discards the first error err by capturing it in the error message string. This is intentional and documented, but the variable shadowing of err from the outer doGet call is slightly confusing. The code is correct but worth a quick read to confirm err at line 83 (doGet error) vs err at the inner json.Unmarshal are distinct. They are, because the doGet error is already returned early.
[NIT] The entry struct type is defined inside ListContents but could be a package-level unexported type. This is minor since it's only used here, but it makes it harder to reuse in tests or if additional methods need similar parsing. Not a real problem given the current scope.
[NIT] json.NewEncoder(w).Encode(...) return values are ignored in several test handlers. The testing patterns document recommends checking all error returns per the repo conventions. In httptest handlers this is typically acceptable since test failures would manifest as downstream assertion errors, but it deviates slightly from the "check all error returns" convention.
[NIT] Several test functions use json.NewEncoder(w).Encode(...) without checking the error return. The encoding/json patterns documented in stdlib tests typically ignore this in test helpers since encoding a map[string]string cannot fail, but json.NewEncoder(w).Encode does return an error that is silently dropped. This is a common and accepted pattern in test HTTP handlers but worth noting.
[NIT] TestListContents_HappyPath is not parallelized (no t.Parallel()) while some other tests in this file are. This is inconsistent but not harmful — HTTP server tests are acceptable either way and the inconsistency is minor.
[MINOR] The empty-object guard (single.Name == "" && single.Path == "" && single.Type == "") catches unexpected shapes, but an empty JSON object {} would unmarshal successfully into entry{} and be rejected here — that's correct. However, the error message 'unexpected response format' is not wrapped (no %w), so there's no underlying error for callers to inspect. This is acceptable since there's no underlying error to wrap, but worth noting for consistency with the rest of the error handling in this file.
[MINOR] GetFileContent is a pure delegation wrapper that adds no behavior beyond calling GetFileContentAtRef. Per the documentation pattern, the comment 'Delegates to GetFileContentAtRef with the provided ref' is accurate but the function itself is only needed to satisfy the vcs.FileReader interface. This is fine, but the doc comment could note that it exists to satisfy the FileReader interface contract, which would explain why this thin wrapper exists rather than just exposing GetFileContentAtRef directly.
[NIT] The TestEscapePath_RejectsDotSegments test name implies rejection/error, but the actual behavior is silent removal (the dot segments are stripped, not rejected with an error). A name like TestEscapePath_RemovesDotSegments would more accurately describe the behavior documented in the function's comment.
[NIT] pr_test.go contains tests for methods defined in files.go (e.g., TestGetFileContentAtRef_*, TestGetCommitStatuses_*). Given that files.go has its own dedicated files_test.go, placing file-content tests in pr_test.go is inconsistent with the one-concern-per-file pattern. These would be better co-located with files_test.go.