fix: library improvements - bug fixes, tests, and coverage badge

[tirthpatell]

Summary

• Fix race condition in getUserID() -- added RLock for concurrent-safe token reads
• Restore structured handleAPIError -- reverted premature dedup to createErrorFromResponse that lost error_data.details parsing
• Honor BaseURL/UserAgent config in NewHTTPClient -- previously hardcoded, now reads from config with fallbacks
• Reset rateLimited flag in RateLimiter.Reset() -- was missing, causing stale rate-limit state
• Fix mutex held during sleep in RateLimiter.Wait() -- released lock before select/sleep to unblock concurrent callers
• Downgrade ShouldWait() to RLock -- time check already guards stale flag, no write needed
• Add comprehensive unit tests across all API surfaces using httptest-based mocking
• Strengthen test assertions -- added missing parameter checks (auto_publish_text, search_type) and error type verification
• Add coverage badge to README via gist-backed shields.io endpoint, updated by CI on main pushes

Test plan

• go test ./... -short -race passes
• go vet ./... clean
• CI passes on all Go versions (1.21-1.24)
• Coverage badge populates after merge to main

[greptile-apps]

Greptile Summary

This PR delivers a set of targeted concurrency and correctness fixes across the rate-limiter, HTTP client, and auth utilities, accompanied by a comprehensive new test suite and a CI coverage badge.

Key changes:

• ratelimit.go – Reset() now clears rateLimited; ShouldWait() downgraded to RLock; Wait() refactored from a recursive pattern to an iterative for loop that releases the mutex before sleeping and uses originalResetTime comparison on wakeup to detect rate-limit deadline extensions.
• client_utils.go – getUserID() wraps the tokenInfo read in RLock; handleAPIError restored with error_data.details parsing, but its HTTP 4xx switch cases (401, 403, 429, 400, 422) are unreachable dead code — see inline comment.
• http_client.go – NewHTTPClient now reads BaseURL and UserAgent from config with fallbacks instead of always using hardcoded strings.
• New test files – httptest-based mocks cover all major API surfaces; test_helpers_test.go consolidates the noopLogger, testClient, and newTestHTTPClient helpers previously scattered across files.
• CI – -race flag added to the test step; coverage badge updated via gist-backed schneegans/dynamic-badges-action on merges to main.

Confidence Score: 4/5

• Safe to merge — all concurrency fixes are correct and the new tests are well-structured, with one minor dead-code inconsistency to clean up.
• The core bug fixes (iterative Wait() loop with originalResetTime guard, Reset() flag clearing, getUserID RLock, BaseURL/UserAgent config) are all implemented correctly and backed by tests run under -race. One point deducted for the latent inconsistency in handleAPIError where the unreachable 429 branch omits a MarkRateLimited call, creating a subtle trap for future refactoring.
• client_utils.go — the dead 4xx switch cases in handleAPIError, particularly the 429 path that silently skips MarkRateLimited.

Important Files Changed

Filename	Overview
ratelimit.go	Key fixes: Reset() now clears rateLimited, ShouldWait() downgraded to RLock, and Wait() refactored from a recursive to an iterative loop that releases the mutex before sleeping and re-checks originalResetTime on wakeup to detect extended rate-limit deadlines. These address the core concurrency concerns from the previous review round.
client_utils.go	getUserID() correctly adds RLock for concurrent-safe token reads. handleAPIError is restored with error_data.details parsing, but its 4xx switch cases (401, 403, 429, 400, 422) are unreachable dead code — Do() only ever returns (resp, nil) for status codes <400 — and the dead 429 path is missing MarkRateLimited, creating a latent inconsistency with createErrorFromResponse.
http_client.go	NewHTTPClient now correctly reads BaseURL and UserAgent from config with proper fallbacks to hardcoded defaults, fixing the previous hardcoded-only behavior.
.github/workflows/ci.yml	Adds -race flag to the test step (addressing the race-condition fixes in this very PR), plus a main-only coverage extraction and badge update via schneegans/dynamic-badges-action. The badge step is correctly gated to refs/heads/main and a single Go version to avoid duplicate gist writes.
test_helpers_test.go	Introduces shared testClient, newTestHTTPClient, jsonHandler helpers and consolidates the previously duplicated noopLogger type here; clean and well-structured foundation for all new test files.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[HTTPClient.Do] -->|ShouldWait?| B{rateLimiter.ShouldWait\nRLock: rateLimited && before resetTime}
    B -->|yes| C[rateLimiter.Wait\nacquire Lock]
    B -->|no| D[executeRequest loop]
    C --> E{resetTime passed?}
    E -->|yes| F[clear rateLimited\nreturn nil]
    E -->|no| G{rateLimited?}
    G -->|no| H[update lastRequestTime\nreturn nil]
    G -->|yes| I[capture originalResetTime\nUnlock\ntime.After waitTime]
    I --> J{ctx.Done or timer fires}
    J -->|ctx.Done| K[return ctx.Err]
    J -->|timer| L[re-acquire Lock\ncheck resetTime]
    L --> M{resetTime after\noriginalResetTime?}
    M -->|yes – rate limit extended| N[update originalResetTime\nrecalculate waitTime\nUnlock → loop]
    N --> I
    M -->|no – reset elapsed| O[rateLimited = false\nlastRequestTime = now\nUnlock return nil]
    O --> D
    D --> P{executeRequest}
    P -->|status < 400| Q[return resp nil]
    P -->|status >= 400| R[createErrorFromResponse\ncalls MarkRateLimited if 429]
    R --> S{isRetryableError?}
    S -->|yes – 429 or 5xx| T{retries left?}
    T -->|yes| D
    T -->|no| U[return nil wrappedErr]
    S -->|no – 4xx auth/validation| V[return nil err]
    Q --> W[Client checks statusCode != 200\ncalls handleAPIError for 2xx-3xx]

Loading

Comments Outside Diff (1)

1. client_utils.go, line 47-64 (link)

   HTTP 4xx switch cases in handleAPIError are unreachable dead code

   handleAPIError is only ever called by client methods after Do() has returned (resp, nil). Because executeRequest unconditionally returns an error for any statusCode >= 400 (via createErrorFromResponse), and Do() discards resp whenever executeRequest returns an error, resp.StatusCode is always in the range 200–399 by the time any client method reaches c.handleAPIError(resp).

   This means the case 401, 403:, case 429:, case 400, 422: branches can never be triggered.

   The 429 branch is particularly worth noting: createErrorFromResponse calls h.rateLimiter.MarkRateLimited(resetTime) when it builds a RateLimitError, but the dead case 429: here does not. If the calling convention ever changes (e.g., Do() is refactored to surface HTTP-error responses back to the caller), this path would silently skip rate-limiter notification — a subtle latent inconsistency.

   Consider either:

   • Removing the unreachable 4xx cases and keeping only the default branch (which is what actually fires for any non-200 2xx/3xx response), or
   • Adding a MarkRateLimited call to the 429 case so both error-creation paths stay in sync.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: client_utils.go
Line: 47-64

Comment:
**HTTP 4xx switch cases in `handleAPIError` are unreachable dead code**

`handleAPIError` is only ever called by client methods after `Do()` has returned `(resp, nil)`. Because `executeRequest` unconditionally returns an error for any `statusCode >= 400` (via `createErrorFromResponse`), and `Do()` discards `resp` whenever `executeRequest` returns an error, `resp.StatusCode` is **always** in the range 200–399 by the time any client method reaches `c.handleAPIError(resp)`.

This means the `case 401, 403:`, `case 429:`, `case 400, 422:` branches can never be triggered.

The 429 branch is particularly worth noting: `createErrorFromResponse` calls `h.rateLimiter.MarkRateLimited(resetTime)` when it builds a `RateLimitError`, but the dead `case 429:` here does not. If the calling convention ever changes (e.g., `Do()` is refactored to surface HTTP-error responses back to the caller), this path would silently skip rate-limiter notification — a subtle latent inconsistency.

Consider either:
- Removing the unreachable 4xx cases and keeping only the `default` branch (which is what actually fires for any non-200 2xx/3xx response), or
- Adding a `MarkRateLimited` call to the 429 case so both error-creation paths stay in sync.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 0819e67}