fix(github): address MINOR/NIT findings from review #2866
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m30s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m8s
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m30s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m8s
- 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.
This commit is contained in:
@@ -263,6 +263,84 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestDoRequest_429RetryAfterHTTPDate(t *testing.T) {
|
||||
attempts := 0
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
attempts++
|
||||
if attempts == 1 {
|
||||
// Use HTTP-date format (RFC 7231) — a time 2 seconds in the future.
|
||||
future := time.Now().Add(2 * time.Second).UTC()
|
||||
w.Header().Set("Retry-After", future.Format(http.TimeFormat))
|
||||
w.WriteHeader(429)
|
||||
w.Write([]byte(`{"message":"rate limit"}`))
|
||||
return
|
||||
}
|
||||
w.WriteHeader(200)
|
||||
w.Write([]byte(`{"ok":true}`))
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
|
||||
|
||||
start := time.Now()
|
||||
body, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||
elapsed := time.Since(start)
|
||||
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if string(body) != `{"ok":true}` {
|
||||
t.Errorf("unexpected body: %s", body)
|
||||
}
|
||||
if attempts != 2 {
|
||||
t.Errorf("expected 2 attempts, got %d", attempts)
|
||||
}
|
||||
// HTTP-date was ~2s in the future; by the time client processes it,
|
||||
// time.Until gives ~1-2s. Verify it's meaningfully delayed (not instant).
|
||||
if elapsed < 500*time.Millisecond {
|
||||
t.Errorf("expected meaningful delay from HTTP-date Retry-After, got %v", elapsed)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDoRequest_429RetryAfterHTTPDateInPast(t *testing.T) {
|
||||
attempts := 0
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
attempts++
|
||||
if attempts == 1 {
|
||||
// Use a time in the past — should result in zero/immediate retry.
|
||||
past := time.Now().Add(-10 * time.Second).UTC()
|
||||
w.Header().Set("Retry-After", past.Format(http.TimeFormat))
|
||||
w.WriteHeader(429)
|
||||
w.Write([]byte(`{"message":"rate limit"}`))
|
||||
return
|
||||
}
|
||||
w.WriteHeader(200)
|
||||
w.Write([]byte(`{"ok":true}`))
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second})
|
||||
|
||||
start := time.Now()
|
||||
_, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||
elapsed := time.Since(start)
|
||||
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if attempts != 2 {
|
||||
t.Errorf("expected 2 attempts, got %d", attempts)
|
||||
}
|
||||
// Past date should override the 5s backoff to ~0
|
||||
if elapsed > 500*time.Millisecond {
|
||||
t.Errorf("expected near-instant retry for past HTTP-date, got %v", elapsed)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDoRequest_SetsUserAgentHeader(t *testing.T) {
|
||||
var gotUA string
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
@@ -392,4 +470,7 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
|
||||
if c.httpClient.Timeout != 30*time.Second {
|
||||
t.Errorf("expected 30s timeout, got %v", c.httpClient.Timeout)
|
||||
}
|
||||
if c.httpClient.CheckRedirect == nil {
|
||||
t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user