- 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
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.
- 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 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
