- Add comment in DismissReview explaining why the Event field is required
by the GitHub API even though DISMISS is the only valid value (#18652).
- Change t.Errorf to t.Fatalf for method/path routing assertions in test
handlers so failures are immediately fatal instead of silently
continuing handler execution (#18653).
Address review findings from sonnet-review-bot (review 3086):
- PostReview now returns ErrConflictingCommitIDs when comments specify
different non-empty CommitIDs, since the GitHub API accepts only a
single commit_id per review. Previously the discrepancy was silently
ignored, using only the first commit's ID.
- Extract newTestClient into helpers_test.go to make cross-file sharing
between review_test.go and identity_test.go explicit.
Refs: #81
Address review feedback from round-3 sonnet review:
- PostReview doc comment now accurately describes vcs.ReviewEvent → GitHub
wire-format string cast and notes nil-Comments omitempty behavior.
- Rename 'expected' field to 'want' in TestTranslateGitHubReviewState to
match the project's established naming convention.
- Wrap ErrCannotDeleteSubmittedReview with operation context via fmt.Errorf
so callers get both sentinel identity and context (MINOR fix)
- Combine double iteration in PostReview into single loop (NIT)
- Remove extra trailing blank line in review_test.go (NIT)
- Clarify translateGitHubReviewState comment re: PENDING state (NIT)
- Update requestOptions.bodyFn comment to mention DELETE-with-body (NIT)
- MAJOR: Extract doRequestCore to eliminate doRequest/doRequestWithBody
duplication. Both now delegate to a shared implementation with the
retry/backoff logic in a single place.
- MINOR: Replace custom containsStr/containsSubstring helpers with
strings.Contains in review_test.go.
- MINOR: Use http.Method* constants (MethodPost, MethodDelete, MethodPut)
in review.go for consistency with doGet.
- MINOR: Remove redundant APPROVED/DISMISSED cases from
translateGitHubReviewState that were identical to the default passthrough.
- NIT: Clarify DeleteReview comment about COMMENTED being a GitHub API
state name.
- DismissReview Event field verified as required by GitHub API docs;
kept as-is.
Implement the remaining vcs.Client interface methods for github.Client:
Reviewer:
- PostReview: POST /repos/{owner}/{repo}/pulls/{number}/reviews
- ListReviews: GET /repos/{owner}/{repo}/pulls/{number}/reviews
with state translation (CHANGES_REQUESTED → REQUEST_CHANGES, etc.)
- DeleteReview: DELETE /repos/{owner}/{repo}/pulls/{number}/reviews/{id}
Returns ErrCannotDeleteSubmittedReview on 422
- DismissReview: PUT /repos/{owner}/{repo}/pulls/{number}/reviews/{id}/dismissals
Identity:
- GetAuthenticatedUser: GET /user
Infrastructure:
- Add doRequestWithBody helper for POST/PUT/DELETE with JSON bodies
- Update conformance_test.go: var _ vcs.Client = (*github.Client)(nil)
All unit tests pass including error cases (401, 404, 422, malformed).
claw