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

feat(github): add safeguards against accidental AllowInsecureHTTP use (#96) #113

Merged
aweiker merged 6 commits from review-bot-issue-96 into main 2026-05-13 20:21:42 +00:00
Conversation 49 Commits 6 Files Changed 3
+2 -1
2 changed files with 2 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 64c9d551ba - Show all commits
github/client.go
+1
View File
39
 @@ -319,6 +319,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
timer := time.NewTimer(delay) timer := time.NewTimer(delay)
sonnet-review-bot commented 2026-05-13 18:36:00 +00:00
Review
Copy Link
Copy Source

[MINOR] The diff removes timer.Stop() from the case <-timer.C: branch. When the timer fires normally, the timer's goroutine is already done, but calling timer.Stop() after it fires is a no-op and not harmful — the original code was actually correct in calling it (the resource is already freed, but it's a safe call). The real concern is the missing call on the successful timer path: after <-timer.C fires, the GC will eventually collect the timer, but timer.Stop() on the fired case is idiomatic cleanup. This is extremely minor since a fired timer has no goroutine leak, only a small GC delay. Consider adding timer.Stop() back on both branches for explicitness, or using defer timer.Stop() before the select.

**[MINOR]** The diff removes `timer.Stop()` from the `case <-timer.C:` branch. When the timer fires normally, the timer's goroutine is already done, but calling `timer.Stop()` after it fires is a no-op and not harmful — the original code was actually correct in calling it (the resource is already freed, but it's a safe call). The real concern is the missing call on the successful timer path: after `<-timer.C` fires, the GC will eventually collect the timer, but `timer.Stop()` on the fired case is idiomatic cleanup. This is extremely minor since a fired timer has no goroutine leak, only a small GC delay. Consider adding `timer.Stop()` back on both branches for explicitness, or using `defer timer.Stop()` before the select.
select { select {
case <-timer.C: case <-timer.C:
timer.Stop() // no-op after fire; kept for symmetry with the ctx.Done case
case <-ctx.Done(): case <-ctx.Done():
timer.Stop() timer.Stop()
return nil, ctx.Err() return nil, ctx.Err()
github/client_test.go
+1 -1
View File
5
 @@ -544,6 +544,7 @@ func TestNoInsecureOption_RejectsHTTP(t *testing.T) {
t.Errorf("unexpected error message: %v", err) t.Errorf("unexpected error message: %v", err)
} }
} }
func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) { func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) {
// Verify case-insensitive scheme check (RFC 3986). // Verify case-insensitive scheme check (RFC 3986).
c := NewClient("tok", "HTTP://example.com") c := NewClient("tok", "HTTP://example.com")
1
 @@ -568,7 +569,6 @@ func TestNoInsecureOption_RejectsMixedCaseHTTP(t *testing.T) {
} }
} }
func TestAllowInsecureHTTP_WithoutEnvVar_Rejected(t *testing.T) { func TestAllowInsecureHTTP_WithoutEnvVar_Rejected(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
t.Fatal("request should not have been sent") t.Fatal("request should not have been sent")
1