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 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.
Addresses intermittent 'unexpected end of JSON input' failures where the
LLM response body is truncated in transit between the proxy and client.
Root cause: network-level truncation where io.ReadAll returns partial data
(observed in 3/50 CI runs through HAI proxy). The response body reading
was already using io.ReadAll correctly, but transient network issues
between the proxy and client can still cause partial reads.
Changes:
- Add Content-Length validation in doRequest: detect when fewer bytes
arrive than the server declared, triggering a retry
- Add retry logic in Complete: retries once on retryable errors (body
read failures, content-length mismatches) with a 500ms backoff
- Add parse-level retry in main: if ParseResponse fails, re-requests
from the LLM once before giving up (defensive, since retries always
succeed per issue evidence)
- Improve ParseResponse error diagnostics: log raw vs cleaned lengths
and a preview of the cleaned content to aid future debugging
Does NOT retry on API errors (4xx/5xx) or structural issues — only
transient body read problems.
Closes#47
claw