Repository navigation
fix: retry 403 secondary rate limits in GitHubClient resilience - #141
Conversation
A 403 carrying a server-advised retry signal (Retry-After, or x-ratelimit-remaining: 0 with x-ratelimit-reset) is a GitHub secondary rate limit and is now classified retryable, so withResilience backs off and retries it instead of failing immediately. A bare 403 stays non-retryable so genuine permission denials do not loop. 429 and 5xx behavior is unchanged. Also removes a broken plugin entry from the Claude settings and gitignores the AI context cache. Closes #139 Signed-off-by: C. Spencer Beggs <spencer@savvyweb.systems>
![savvy-web-bot[bot]](https://avatars.githubusercontent.com/in/1855974?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Current Commit: f71de9c
Summary
Clean, minimal fix. The change is correctly scoped: GitHub secondary rate limits surface as 403s with Retry-After or x-ratelimit-remaining: 0 + x-ratelimit-reset, and this PR makes exactly that distinction — retrying those while leaving bare 403 permission denials as immediate failures.
What's Good
-
Correct classification logic.
isRetryable(status, retryAfterMs)neatly captures the three cases (429 always, 5xx always, 403 only with a retry signal). TheretryAfterMs !== undefinedgate is the right predicate — it's exactly whatparseRetryAfterMsreturns when a retry signal is present. -
Eliminates double computation. The old code called
parseRetryAfterMs(error)independently forretryableandretryAfterMs. Now it's computed once inwrapErrorand the result flows into both fields — cleaner and consistent. -
Solid test coverage. Three unit tests cover all 403 variants at the
wrapErrorlevel (Retry-After, x-ratelimit-remaining:0, bare 403). Two integration tests verify the resilience layer actually retries a 403+Retry-After to success and stops immediately on a bare 403. The critical paths are all exercised. -
Changeset included. Patch bump with accurate description.
-
Housekeeping is appropriate. Removing the broken
vitest-agent-reporterplugin entry and gitignoring.claude/cache/are clean, uncontroversial changes.
Checks
| Check | Status |
|---|---|
| Tests | ✅ success |
| Code Quality | ✅ success |
| Conventional Commits | ✅ success |
| PR Title Validation | ✅ success |
| Markdown | ✅ success |
Nit
One inline comment on the new multi-line JSDoc block — non-blocking, see the inline note.
| /** | ||
| * Whether a failed response should be retried. `429` and any `5xx` are always | ||
| * retryable. A `403` is retryable only when it carries a server-advised retry | ||
| * delay (GitHub secondary rate limit) — a bare `403` is a genuine permission | ||
| * denial and must fail fast rather than loop. | ||
| */ | ||
| const isRetryable = (status: number, retryAfterMs: number | undefined): boolean => | ||
| status === 429 || status >= 500 || (status === 403 && retryAfterMs !== undefined); |
There was a problem hiding this comment.
Nit: CLAUDE.md convention says "never write multi-line comment blocks — one short line max." The WHY here is genuinely non-obvious so a comment is warranted, but it could be condensed to one line, e.g.:
// A 403 is retryable only with a server-advised retry signal (GitHub secondary rate limit).Non-blocking — the explanation is valuable either way.
Summary
GitHubClient's resilience layer previously classified only429and>= 500responses as retryable. GitHub secondary rate limits frequently surface as403with aRetry-Afterheader, so those responses failed immediately instead of backing off.A
403is now treated as retryable only when it carries a server-advised retry signal — aRetry-Afterheader, orx-ratelimit-remaining: 0plusx-ratelimit-reset. A bare403(genuine permission denial) stays non-retryable so we don't loop on real auth failures.429/5xxbehavior is unchanged. The fix reuses the existingparseRetryAfterMs/withResiliencebackoff machinery.Changes
src/layers/GitHubClientLive.ts— replace status-onlyisRetryableStatuswithisRetryable(status, retryAfterMs);wrapErrorcomputes the retry delay once and widens the retryable set to include 403 secondary rate limits.src/layers/GitHubClientLive.test.ts— tests for both retried and non-retried 403 paths (error-wrapping + resilience integration).layers.md,errors-and-schemas.md,services.md,testing-strategy.md) anddocs/08-resilient-github-api.mdupdated to describe the new classification..claude/settings.json; gitignored the AI context cache.Test plan
pnpm vitest runonGitHubClientLive.test.tsandresilience.test.ts— all passpnpm run typecheck— cleanCloses #139
Signed-off-by: C. Spencer Beggs spencer@savvyweb.systems