The supersede path calls GetTimelineReviewCommentIDForReview then EditComment.
On GitHub, EditComment targets /pulls/comments/{id} (for inline review comments),
but review bodies are not inline comments. The PATCH will return 404 and be
logged as a warning. The review itself posts correctly regardless.
This is a known limitation pending a future fix via the reviews PATCH endpoint.
Adds GetPullRequest, GetPullRequestDiff, GetPullRequestFiles,
GetCommitStatuses, GetFileContent, GetFileContentRef, ListContents,
GetAllFilesInPath, PostReview, ListReviews, DeleteReview,
GetAuthenticatedUser, RequestReviewer, EditComment, ListReviewComments,
ResolveComment, and GetTimelineReviewCommentIDForReview.
These mirror the gitea/ package methods but use GitHub REST API v3 paths
(/repos/{owner}/{repo}/... instead of /api/v1/repos/...) and handle
GitHub-specific behaviors:
- File content is base64-encoded in the JSON response
- Review event is APPROVE (not APPROVED)
- ResolveComment is a no-op (GitHub has no REST API for this)
- GetTimelineReviewCommentIDForReview returns the reviewID as-is
- Fix AllowInsecureHTTP doc comment: say '_test.go file in the same
package' instead of 'export_test.go' (MINOR #1)
- Remove dead u.Fragment = "" from redactURL: HTTP requests never
carry fragments over the wire per RFC 7230 §5.1 (MINOR #2)
- Use 127.0.0.1:1 in scheme-rejection tests to make intent clearer
that no network call should occur (NIT #3)
- Restore timer.Stop() no-op in case <-timer.C for symmetry with ctx.Done
- Add missing blank line between TestNoInsecureOption_RejectsHTTP and
TestNoInsecureOption_RejectsUppercaseHTTP
- Remove double blank line before TestAllowInsecureHTTP_WithoutEnvVar_Rejected
Resolves review comments from sonnet-review-bot on PR #113.
Address review feedback on PR #113:
- MAJOR (both reviews): Replace strings.HasPrefix(reqURL, "http://")
with url.Parse + strings.EqualFold for case-insensitive scheme
comparison per RFC 3986. Prevents bypass via HTTP:// or Http://.
- MINOR (security): Enhance redactURL to strip userinfo component
(user:pass@host) in addition to query params, preventing credential
leakage in error messages and logs.
- NIT (gpt): Remove redundant timer.Stop() after timer.C fires —
it's a no-op and the comment was misleading.
- Add tests for uppercase/mixed-case HTTP scheme rejection and
userinfo redaction.
Three-layer defense for the AllowInsecureHTTP client option:
1. Environment gate: AllowInsecureHTTP() requires REVIEW_BOT_ALLOW_INSECURE=1
env var. Without it, the option is silently ignored with a slog.Warn.
2. Warning log on activation: When the env gate IS satisfied, a slog.Warn
fires at client construction time so operators notice in production logs.
3. Test bypass: AllowInsecureHTTPForTest() skips the env gate entirely,
keeping test code clean (no t.Setenv needed in every test).
Additionally, doRequest now rejects HTTP URLs unless allowInsecureHTTP is set
on the client, providing defense-in-depth against credential leakage.
Closes#96
- Replace Unicode arrows (→) with ASCII (->) in error messages and
comments for log compatibility (gpt-review NITs #19626, #19628)
- Improve guard comment to clarify it exists for testability, not
runtime safety (sonnet-review NIT #19619)
- Add cross-reference comments noting intentional duplication between
gitea/client.go and github/client.go (sonnet-review #19618,
gpt-review #19625, #19627)
Pushed back on:
- internal/ package for dedup: structural overhead not warranted for
a single ~25-line function
- strings.EqualFold for scheme: Go's url.Parse normalizes schemes to
lowercase, making case-insensitive comparison unnecessary
Add defaultCheckRedirect to both GitHub and Gitea clients that rejects:
- HTTPS→HTTP protocol downgrades (prevents plaintext leakage)
- Cross-host redirects entirely (prevents consuming untrusted responses)
Same-host, same-or-upgraded-scheme redirects remain allowed.
Both NewClient constructors wire the policy, and SetHTTPClient(nil)
restores it. Callers providing a non-nil client are responsible for
configuring their own safe redirect policy.
Closes#95
- Add timer.Stop() to the timer.C branch to prevent timer leak on the
normal path (previously only called in ctx.Done branch)
- Rename struct field 'http' to 'httpClient' to avoid shadowing the
net/http import
Addresses self-review findings on PR #110.
- Add TODO comment on unused baseURL field explaining its intended
future use by higher-level exported methods
- Rewrite TestDoRequest_ContextCanceled to actually exercise the
timer-cancel path in the retry select (goroutine cancels context
while client is blocked in timer.C select)
- Change Retry-After: 1 to Retry-After: 0 in IntegerSeconds test
to avoid unnecessary 1s sleep during test runs
- Add doc note on SetRetryBackoff explaining that an empty non-nil
slice silently drops Retry-After delays
- Fix data race: copy retryBackoff slice per-request to prevent
concurrent doRequest calls from racing on shared state
- Fix parseRetryAfter: trim whitespace before parsing for robustness
- Fix parseRetryAfter: treat delta-seconds of 0 as valid per RFC 7231
- Add bounded read on success path (10 MB limit) for defense-in-depth
- Clean up TestDoRequest_429_RetryAfter_IntegerSeconds: remove dead
code block and commented-out reasoning
- Fix import ordering: context before errors (goimports compliance)
Implement the github package client with Retry-After header parsing that
supports both integer seconds (e.g. "Retry-After: 120") and HTTP-date
format (e.g. "Retry-After: Thu, 01 Dec 2025 16:00:00 GMT") per RFC 7231
§7.1.3.
Key design decisions:
- Use http.ParseTime which handles RFC 1123, RFC 850, and ASCTIME formats
- Cap maximum retry delay at 60s (maxRetryAfter) to prevent stalling
- If HTTP-date is in the past, use delay of 0 (retry immediately)
- Inject time.Now via c.now field for deterministic testing
Closes#94
Rodin
claw