rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases 6 Wiki Activity

feat(github): implement PRReader + FileReader client (#80) #93

Closed
rodin wants to merge 16 commits from review-bot-issue-80 into feature/github-support
pull from: review-bot-issue-80
merge into: rodin:feature/github-support
Conversation 78 Commits 16 Files Changed 8
+2215

16 Commits

Author SHA1 Message Date
claw b380e7fcae refactor(github): extract handleResponse for safe defer body close
PR Ready Gate / clear-labels (pull_request) Successful in 1s
Details
CI / test (pull_request) Successful in 17s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 40s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m16s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m44s
Details 
Address review findings #1 and #2: the response body was closed explicitly
rather than via defer, which could leak if future code paths were added.

Extract handleResponse helper method that uses defer resp.Body.Close() to
guarantee cleanup. This avoids the loop-defer antipattern (defer inside a
for loop accumulates defers until function exit) by isolating the body
handling into its own function scope.
 2026-05-12 20:47:59 -07:00
claw 30798ff023 fix: address sonnet review MINOR findings (#2916)
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 18s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 46s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 59s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m6s
Details 
- client.go: fix misleading timer.Stop() comment (finding #1)
- pr.go: document all-or-nothing semantics for GetCommitStatuses
  when check-runs endpoint fails after statuses succeed (finding #2)
- files.go: include both array and object unmarshal errors in
  ListContents fallback error message (finding #3)
- pr.go: expand mapCheckRunStatus comment to explain non-blocking
  policy decision (finding #4)
 2026-05-12 20:28:52 -07:00
claw 6e8e744816 fix(github): address self-review findings from 1194bc75
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 17s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 51s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m22s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m36s
Details 
- Handle io.ReadAll error on error body read (client.go:265)
- Remove unused State field from commitStatusResponse (pr.go)
- Guard via slice access in defaultCheckRedirect (client.go:117)
- Move GetFileContentAtRef from pr.go to files.go (logical home)
 2026-05-12 19:40:30 -07:00
claw 1194bc758c fix(github): address review findings from rounds 2884/2885/2887
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 18s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 40s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m18s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m44s
Details 
- Fix response body limit check: read maxResponseBytes+1 and use > to
  distinguish exactly-at-limit from truncated (sonnet finding #1)
- Reject HTTPS→HTTP redirects outright instead of stripping auth and
  following; prevents plaintext metadata leakage (sonnet #2, security #1)
- Sanitize newlines in APIError.Error to prevent log injection from
  upstream response bodies (security #2)
- Add nil-return documentation to GetCommitStatuses (sonnet #3)
- Gate TestDoRequest_429RetryAfterHTTPDate behind testing.Short (sonnet #6)
- Add tests for redirect policy, exact-at-limit body, and error sanitization
 2026-05-12 19:29:06 -07:00
claw 80af5037b2 fix(github): address review findings from round 2880/2883
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 24s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 43s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m16s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m21s
Details 
Sonnet MINOR #1: Stop timer after <-timer.C fires for idiomatic cleanup.
Sonnet MINOR #2: Document that empty array from contents API is valid (empty dir).
Sonnet MINOR #3: Document that GetPullRequestFiles returns nil for no files.
Sonnet NIT #4: Strengthen SetHTTPClient/SetRetryBackoff docs to clarify test-only intent.
Sonnet NIT #5: Document GetCommitStatuses fail-fast behavior.
Sonnet NIT #6: Document double-slash collapsing in escapePath.
Security MINOR #1: Document redirect policy responsibility when providing custom client.
Security MINOR #2: Reduce maxErrorBodyBytes from 64KB to 4KB to limit sensitive data exposure.
 2026-05-12 18:41:44 -07:00
claw 5b2fa0b9af refactor(github): address review findings from round 2872
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 17s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 36s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m31s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m53s
Details 
- client.go: clarify timer drain comment (finding #1)
- client.go: rename t -> retryAt for time.Time clarity (finding #2)
- pr.go: remove dead _ string parameter from mapCheckRunStatus (finding #3)
- files.go: add inline comment explaining zero-value guard (finding #4)

Findings #5 (NIT, no code change) and #6 (NIT, defer vs t.Cleanup
in t.Run closures) pushed back — see PR comment.
 2026-05-12 18:16:43 -07:00
claw 491df7cb1f fix(github): address review findings from rounds 2867/2870
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 18s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 41s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m20s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m43s
Details 
- Extract duplicated CheckRedirect lambda to defaultCheckRedirect function
  (sonnet #1: eliminate duplication between NewClient and SetHTTPClient)
- Remove unnecessary int64 cast in response size check (sonnet #3)
- Validate fallback unmarshal in ListContents to reject zero-value entries
  (sonnet #5: prevent accepting unexpected JSON formats silently)
- Rename strPtr to stringPtr for consistency (sonnet #6)
- Add doc comment about APIError.Error body exposure (security #3)

Deferred to separate issues:
- #95: Reject cross-host redirects entirely (security #1)
- #96: Add safeguards for AllowInsecureHTTP (security #2)
 2026-05-12 17:30:24 -07:00
claw 1fcc0b738a fix(github): address MINOR/NIT findings from review #2866
PR Ready Gate / clear-labels (pull_request) Successful in 1s
Details
CI / test (pull_request) Successful in 18s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m30s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m8s
Details 
- SetHTTPClient(nil): preserve CheckRedirect auth-stripping policy
  instead of restoring a plain http.Client that loses cross-host
  protection.

- Authorization header: add comment documenting why Bearer scheme is
  correct (OAuth2 standard, works for both classic PATs and
  fine-grained tokens).

- Retry-After parsing: support HTTP-date format (RFC 7231) in addition
  to integer seconds. GitHub only sends integers today, but the
  implementation is now spec-compliant.

- escapePath dot-segment removal: document the behavior in public API
  doc comments for ListContents and GetFileContentAtRef so callers are
  aware without reading the internal helper.
 2026-05-12 17:13:07 -07:00
claw fce5f2d184 fix(github): address review findings on client.go
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 17s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 40s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m23s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m15s
Details 
- Use net/url.Parse for HTTPS scheme check (case-insensitive)
- Guard SetHTTPClient against nil (restores default 30s client)
- Rename 'url' param to 'reqURL' in doRequest/doGet for clarity
- Return error when response exceeds maxResponseBytes instead of
  silently truncating

Finding #1 (Bearer auth scheme) intentionally kept: GitHub REST API
officially supports and recommends Bearer for all token types.
See: https://docs.github.com/en/rest/authentication/authenticating-to-the-rest-api
 2026-05-12 16:55:32 -07:00
claw af72c64b7f fix(github): correct ListContents error wrapping and move HTTPS guard before retry loop
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 17s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 42s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m11s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m11s
Details
2026-05-12 16:48:39 -07:00
claw 1bc3f206ba fix: address review findings from rounds 2843-2846
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 17s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 41s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m13s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m23s
Details 
- Remove redundant timer.Stop() after timer fires (Sonnet #1, GPT #2)
- Remove unused TotalCount field from checkRunsResponse (Sonnet #2)
- Improve escapePath doc comment to explain deliberate silent stripping (Sonnet #3)
- Fix ListContents to handle both array (directory) and object (single file)
  responses from GitHub Contents API (GPT #3)
- Add HTTPS enforcement: refuse to send credentials over non-HTTPS URLs
  unless AllowInsecureHTTP() option is passed (Security #1)
- Replace constant-value test with actual behavior test for response
  body limiting (Sonnet #6)
- Run gofmt for consistent formatting (Sonnet #4)
- Add tests for HTTPS enforcement and ListContents single-file handling
 2026-05-12 16:39:01 -07:00
claw c10bb72117 fix: address self-review NIT findings on PR #93
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 22s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 37s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m9s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m17s
Details 
- Add timer.Stop() on happy path in retry loop (idiomatic)
- Add concurrency caveat to Client doc comment for SetHTTPClient/SetRetryBackoff
- Add explicit 'stale'/'waiting' cases to mapCheckRunStatus
 2026-05-12 16:25:32 -07:00
claw ae91c8aef5 fix: address review findings from rounds 2834-2838
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 17s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 49s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m6s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m19s
Details 
- Unexport RetryBackoff, add SetRetryBackoff method (#17286)
- Rename http field to httpClient to avoid shadowing (#17289)
- Group const blocks into single declaration (#17291)
- Fix CheckRedirect to compare against previous hop, not first (#17302)
- Strip auth header on protocol downgrade https→http (#17297)
- Add maxPages safeguard to pagination loops (#17299, #17300)
- Document mapCheckRunStatus unused second parameter (#17287, #17303)
 2026-05-12 16:11:58 -07:00
claw 75f65fbf5d fix: address MINOR review findings on PR #93 (round 2)
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 17s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 38s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m28s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m50s
Details 
- Add User-Agent header to all requests (gpt-review-bot)
- Limit successful response body to 10 MiB via io.LimitReader (security-review-bot)
- Add CheckRedirect to strip Authorization on cross-host redirects (security-review-bot)
- Fix decodeBase64Content to strip both \r and \n (gpt-review-bot)
- Document that transport errors are not retried (sonnet-review-bot)
- Update package doc to reflect current scope (no review submission yet)
- Add tests for User-Agent, empty-token auth skip, CRLF base64, CheckRedirect
 2026-05-12 16:00:09 -07:00
claw 5b43afc6d4 fix: address review feedback on PR #93
PR Ready Gate / clear-labels (pull_request) Successful in 1s
Details
CI / test (pull_request) Successful in 23s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 45s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m48s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m7s
Details 
- Fix Retry-After slice mutation: copy c.RetryBackoff before modifying
  to prevent permanent mutation of the shared slice (sonnet#1, security#1)
- Cap Retry-After to 120s maximum to prevent excessive sleeps (security#2)
- Guard auth header: only set Authorization when token is non-empty (gpt#2)
- Fix GetFileContent doc comment to match actual behavior (sonnet#3, gpt#1)
- Remove dead 'in_progress/queued' case in mapCheckRunStatus (sonnet#4)
- Add testing.Short() guard to slow retry test (sonnet#5)
- Reject dot-segments in escapePath to prevent path traversal (security#3)
- Add regression tests for non-mutation and escapePath safety
 2026-05-12 15:43:45 -07:00
claw d1ef1e21e5 feat(github): implement PRReader + FileReader client (#80)
CI / test (pull_request) Successful in 18s
Details
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 34s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m45s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m56s
Details 
Implement the GitHub API client with PRReader and FileReader interface
conformance for both github.com and GitHub Enterprise.

New files:
- github/client.go: Client struct, NewClient with configurable base URL,
  HTTP helpers with 429 retry and Retry-After support
- github/pr.go: GetPullRequest, GetPullRequestDiff (per-request Accept
  header), GetPullRequestFiles (paginated, populates Patch field),
  GetFileContentAtRef (base64 decode), GetCommitStatuses (merges commit
  statuses + check runs with conclusion mapping)
- github/files.go: GetFileContent (delegates to GetFileContentAtRef),
  ListContents, escapePath, decodeBase64Content helpers

Type changes:
- vcs/types.go: Add Patch field to ChangedFile struct

Tests cover: happy path, 404, 401, 429+retry, malformed response,
pagination, binary files, check run conclusion mapping, base64 decoding.

Compile-time checks:
  var _ vcs.PRReader = (*Client)(nil)
  var _ vcs.FileReader = (*Client)(nil)

Exit criteria met:
- go test ./github/... passes (all methods)
- NewClient with empty baseURL uses https://api.github.com
- NewClient with GHE URL targets correctly
- GetFileContent delegates to GetFileContentAtRef with empty ref
- GetPullRequestFiles paginates and populates Patch field
- GetCommitStatuses merges both commit statuses and check-runs
 2026-05-12 15:18:55 -07:00