Address sonnet-review feedback on PR #106:
- Document that the type assertion in supersedeOldReviews is guaranteed to
succeed given the caller's provider switch, with the !ok branch guarding
against future refactors (comment 18889).
- Clarify that vcsURL is only used in the Gitea path for constructing
review permalink URLs (comment 18890).
- Add note explaining why the page-limit warning in ListReviews only fires
when the final page is full, confirming the logic is intentional
(comment 18891).
- Remove dead code: findOwnReview (replaced by findAllOwnReviews)
- Check SetRetryBackoff return value in doJSONRequest tests
- Extract doWithRetry shared helper to eliminate ~100 lines of
duplicated 429-retry/backoff/Retry-After logic between doRequest
and doJSONRequest
- Fix import order: context before encoding/json (goimports)
- Add slog.Warn when ListReviews hits maxReviewPages limit
- Add 429 rate-limit retry logic to doJSONRequest (matching doRequest
behavior) so write operations (PostReview, DismissReview) properly
retry when rate-limited by GitHub
- Remove redundant explicit case for ReviewEventComment in
translateReviewEvent (default already handles it)
- Add ordering comment on --gitea-url alias registration explaining
the dependency on registration-before-parse evaluation order
- Add tests for doJSONRequest retry/exhaust behavior
- Document --gitea-url/--vcs-url last-one-wins behavior when both flags
are passed simultaneously (sonnet MINOR #1)
- Move doJSONRequest from github/reviews.go to github/client.go where
other HTTP helpers live (sonnet MINOR #2)
- Return joined error from supersedeOldReviews GitHub case instead of
silently swallowing DismissReview failures (sonnet MINOR #3)
- Fix evaluateCIStatus to distinguish 'all checks passed' from 'no
failures (N pending)' to avoid misleading status (gpt MINOR #2)
- Extract reviewsPerPage and maxReviewPages named constants for
ListReviews pagination (gpt NIT #3)
The empty case "gitea": body exits the switch and continues to the
Gitea-specific logic below. Replace the vague comment with an explicit
note about the fall-through intent, per self-review feedback.
- Replace panic() with fmt.Fprintf+os.Exit(1) in provider switch default
(repo convention: never panic)
- Remove spurious 'event' field from DismissReview payload (GitHub dismiss
endpoint only documents 'message')
- Change translateReviewEvent default to return 'COMMENT' as canonical
fallback instead of passing unknown events through to GitHub API
- Refactor supersedeOldReviews to use explicit switch/case with default
error for exhaustiveness
- Replace interface{} with any in github/reviews.go (Go 1.18+ idiom)
- Add default panic case to VCS client init switch
- Refactor supersedeOldReviews to return error instead of os.Exit(1)
- Remove spurious blank lines in formatter.go and formatter_test.go
- Add doc comment to DeleteReview explaining when to use vs DismissReview
- Sanitize extractSentinelName output to prevent log injection
- Add --provider flag (gitea|github) for VCS backend selection
- Add --base-url flag for GitHub API endpoint configuration
- Rename --gitea-url to --vcs-url with backward-compatible alias
- Replace direct gitea.Client usage with vcs.Client interface
- Create vcs.Client via factory switch based on --provider value
- Implement Reviewer + Identity interfaces on github.Client
- Add verdictToEvent() using canonical vcs.ReviewEvent types
- Remove review.GiteaEvent() (replaced by verdictToEvent)
- GitHub supersede uses DismissReview; Gitea keeps EditComment flow
- Add VCS_PROVIDER, VCS_BASE_URL, VCS_URL env var support
Closes#82
- 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).
Adds TestTranslate_HunkHeaderAtEnd covering the edge case where a
hunk-header is the last position in the file with no subsequent
new-file line. Mirrors TestBuildPositionToLineMap_DeletionAtEnd for
the hunk-header code path.
Addresses NIT from sonnet-review-bot on PR #104 (comment 18412).
BuildPositionToLineMap incremented position and updated maxPositions for
@@ hunk-header lines but did not store a map entry, causing Translate()
to return a hard error for any comment positioned at a hunk header.
Store sentinel value 0 for hunk-header positions (analogous to -1 for
deletions) and extend Translate() to fall through to the nearest
context/addition line below, matching the existing deletion-line
behavior.
Fixes#97
Pass 2 elements to SetRetryBackoff (matching maxRetryAttempts-1 = 2)
and check the error return. Previously passing 1 element silently
failed, causing tests to fall back to default {1s, 2s} backoffs.
Fixes self-review finding: 429Retry tests now run in <10ms instead
of ~1s.
Implement FileReader conformance on the GitHub client: GetFileContent,
ListContents, path helpers, base64 decode. Includes compile-time
conformance checks for both PRReader and FileReader.
Requires PR B (#102). Part 3 of 3 for #80.
- Add maxFileContentSize (10 MB) limit to decodeBase64Content to prevent
resource exhaustion from oversized file content (security MINOR)
- Fix reversed NewClient arg order in TestGetFileContentAtRef_DotSegmentError
(GPT MINOR + Sonnet NIT)
- Remove 'waiting' from mapCheckRunStatus conclusion cases since it is a
status value not a conclusion, update comment (GPT NIT)
- Add TestDecodeBase64Content_SizeLimit test
- escapePath now returns an error when paths contain dot-segments
(".", "..") instead of silently rewriting them. This prevents
subtle API misses where callers pass "foo/../bar" expecting to
hit "bar" but the old code produced "foo/bar".
- Uses path.Clean for canonical form after validation.
- CommitStatus.Description for check runs is now empty string
instead of the raw conclusion enum. The conclusion is already
captured in the Status field via mapCheckRunStatus; storing it
again in Description was semantically inconsistent with commit
statuses where Description carries a human-readable narrative.
- Removed unused derefString helper.
- Added tests for escapePath valid paths, dot-segment rejection,
and GetFileContentAtRef dot-segment error propagation.
- Separate maxPages into maxFilesPages and maxCheckRunPages constants
for clarity (sonnet MINOR #1)
- Add parallel to CheckRunConclusions subtests (sonnet MINOR #2)
- Add TestGetCommitStatuses_CheckRunsErrorAfterStatusesSucceed test
covering check-runs 500 after statuses succeed (sonnet MINOR #2)
- Expand mapCheckRunStatus doc comment with full mapping rules including
cancelled/skipped/neutral rationale and unknown value behavior
(sonnet MINOR #3, gpt MINOR #1)
- Expand GetPullRequest doc comment to mention error types returned
(sonnet NIT #4)
- Add inline comment on Description field clarifying it holds raw
conclusion value (gpt NIT #3)
- Implement GetFileContentAtRef on *Client to satisfy vcs.PRReader interface
- Add escapePath and decodeBase64Content helpers
- Fix conformance_test.go to properly import and qualify github.Client
(was using unqualified Client in package github_test)
Fixes CI failure: the PRReader interface requires GetFileContentAtRef
but it was missing from this PR (only present in the file-reader PR).
- MAJOR #1: Replace panic in doRequest with safe default fallback.
Validation now happens in SetRetryBackoff (returns error on invalid
length). doRequest gracefully falls back to default backoff if the
configured slice is somehow invalid.
- MINOR #2: SetRetryBackoff validates slice length at configuration
time, making the coupling between maxRetryAttempts and backoff
explicit and catching mismatches early with a clear error.
- MINOR #4: Reword oversized response error to remove '(truncated)'
which implied truncated data was returned when actually only an
error is returned.
- MINOR #5: Functional options kept as-is - idiomatic Go pattern
that allows future growth without breaking the API.
Address review feedback on PR #101:
1. Capture resp.StatusCode and Retry-After header *before* passing resp
to handleResponse, making ownership transfer explicit. Previously the
caller read resp.StatusCode after handleResponse had closed the body —
correct but fragile coupling.
2. Add panic guard ensuring backoff slice length equals maxAttempts-1.
Previously the relationship was implicit and could silently break if
maxAttempts were changed without updating the default backoff.
Add GitHub API client with configurable base URL and GHE support,
HTTP helpers with 429 retry and Retry-After handling.
Also adds Patch field to vcs.ChangedFile.
Part 1 of 3 for #80.
Add fmt.Errorf wrapping to the two remaining unwrapped error returns
in the adapter:
- GetPullRequest: 'get pull request: %w'
- PostReview (final client call): 'post review: %w'
This makes all error paths in the adapter consistent with the wrapping
pattern used by the diff-fetch and position-translation errors.
Addresses self-review findings #1 and #2 from b2eea502.
- position.go: Replace O(n) maxPosition scan with O(1) lookup by
tracking max position during map construction. Also eliminates
shadowing of the builtin max identifier (Go 1.21+).
- position.go: Add comment clarifying +++ prefix ordering intent.
- adapter.go: Document diff-fetch tradeoff in PostReview.
- adapter_test.go: Remove extra blank line between test functions.
- Remove unused error return from BuildPositionToLineMap (always nil)
- Add comment explaining intentional CommitID drop in PostReview
- Refactor TestAdapter_PostReview_WithComments to route by URL path
- Add TestAdapter_GetFileContent_RefRouting test
- Acknowledge maxPosition O(n) with code comment
- Remove redundant TestAdapter_CompileTimeCheck (compile-time var _ exists)
- Fix GetPullRequestFiles comment (Patch field is omitted, not 'set to empty')
- Acknowledge translateEvent fallback as intentional design
Implements the Gitea adapter (gitea.Adapter) that satisfies vcs.Client.
Key components:
- gitea/adapter.go: Adapter struct wrapping *Client with all vcs.Client methods
- gitea/position.go: BuildPositionToLineMap for diff-position → line translation
- gitea/adapter_test.go: Tests for all mapping methods and event translation
- gitea/position_test.go: Tests for position translation edge cases
Translation details:
- ReviewEvent: APPROVE → APPROVED (Gitea-native)
- PostReview: fetches diff, builds position map, translates each comment
- Deletion-targeted positions map to nearest non-deletion line below
- All field-mapping methods tested (GetPullRequest, GetPullRequestFiles,
ListReviews, GetCommitStatuses, ListContents)
Also:
- Added Base field to gitea.PullRequest struct
- Updated conformance tests to assert Adapter (not raw Client) satisfies vcs.Client
- Removed phase2 build tag from conformance tests
Closes#79
- Create vcs/util.go with GetAllFilesInPath and BuildLineToPositionMap
- Create vcs/util_test.go with comprehensive tests for both functions
- Remove review.ContentEntry type, replace with vcs.ContentEntry
- Remove review.GiteaClient interface, replace with vcs.FileReader
- Update review/repo_persona.go to use vcs.FileReader
- Update review/repo_persona_test.go to use vcs.ContentEntry
- Update cmd/review-bot/main.go adapter to implement vcs.FileReader
- Add Number and Base fields to vcs.PullRequest
- Add CommitStatus type to vcs/types.go
- Add GetFileContentAtRef to vcs.PRReader interface
- Add GetCommitStatuses to vcs.PRReader interface
- Add DismissReview to vcs.Reviewer interface
- Add stub implementations on gitea.Client for new interface methods
Closes#84, Closes#85, Closes#86
- Extract named HeadRef and UserInfo structs from anonymous structs
in PullRequest and Review (comments 16615, 16616)
- Change ReviewEventApprove value from "APPROVED" to "APPROVE" to
represent the action, not the state; document adapter translation
responsibility (comment 16621)
- Add doc comment on ReviewComment.CommitID noting optionality (16531)
- Move compile-time assertion from check.go (//go:build ignore) to
check_test.go with a "phase2" build tag — removes gitea adapter
import from the vcs package (comment 16622)
- check.go misleading comment was already fixed in prior commit (16532, 16539)
- Sha→SHA, typed ReviewEvent, duplicate package doc already resolved (16537, 16538, 16530)
Add vcs/interfaces.go and vcs/types.go as the foundation for multi-platform
VCS support. Interfaces are discovered from working gitea/client.go code,
not invented in a vacuum.
vcs/interfaces.go — role-based interfaces:
- PRReader: GetPullRequest, GetPullRequestDiff, GetPullRequestFiles
- FileReader: GetFileContent (path + ref), ListContents
- Reviewer: PostReview (ReviewRequest), ListReviews, DeleteReview
- Identity: GetAuthenticatedUser
- Client: all four composed
vcs/types.go — types extracted from gitea/:
- PullRequest, ChangedFile, ContentEntry, Review (identical to gitea/)
- ReviewComment: uses GitHub diff-position convention (Position int,
CommitID string) instead of Gitea's NewPosition int64
- ReviewRequest: new type wrapping Body, Event, Comments
vcs/check.go (//go:build ignore) — documents the gaps gitea.Client
must bridge in Phase 2:
1. PostReview signature mismatch (event+body+[]ReviewComment vs ReviewRequest)
2. GetFileContent missing ref parameter
3. ReviewComment type mismatch (NewPosition vs Position/CommitID)
No behavior changes. All existing tests pass.
- Goal: AI code reviews on GitHub with AI Core
- Feature inventory with API mapping
- Small interfaces (PRReader, FileReader, Reviewer, Identity)
- Test plan: unit (mock HTTP) + integration (real GitHub)
- 7 implementation phases with exit criteria
Issue: #76
claw
aweiker
Rodin