Skip to content

fix: correct three bugs in token minting and throttle classification - #63

Merged
jferrl merged 1 commit into
mainfrom
fix/correctness
Sep 17, 2026
Merged

jferrl merged 1 commit into
mainfrom
fix/correctness

Conversation

@jferrl

@jferrl jferrl commented Sep 17, 2026 •

Copy link
Copy Markdown
Owner

Summary

Three defects, each pinned by a test written to fail against the current code first. A 403 permission failure was reported to callers as rate limiting, after stalling up to 60 seconds. Named identifier types were rejected despite the generic constraint promising them. A short application token expiration minted a JWT that had already expired, because issuance is backdated 60 seconds for clock drift and only the upper bound was clamped.

Changes

  • A 403 counts as throttled only when it carries Retry-After, reports an exhausted budget, or says so in its message; Resource not accessible by integration is now terminal and unwrapped
  • resolveIssuer switches on the reflect kind, honouring the ~int64 | ~string tildes for named types while keeping the zero-value rejection
  • WithApplicationTokenExpiration rejects values at or below 90s, the backdate plus DefaultExpirySkew, and falls back to the 10 minute default
  • Caps the error body read at 64 KiB, corrects an 8m30s arithmetic error in three comments, aligns the ErrRateLimited and WithRetryOnThrottle godoc with the new 403 contract, fixes a README example using a now-rejected value, and ignores .claude/

Rationale

The 403 message is consulted because a secondary rate limit can arrive with a healthy primary budget and no Retry-After; classifying that as terminal would strip the ErrRateLimited that callers with their own backoff branch on. The expiration floor is the backdate plus the skew rather than the backdate alone, because in between the cache can never hold the token and every call re-signs — a remote round trip for the KMS, HSM and Vault signers NewApplicationTokenSourceFromSigner exists to serve. The 60 second backdating is unchanged: it is what keeps a 10 minute expiry under GitHub's ceiling on a fast clock.

Migration Notes

A 403 with rate-limit headers but a healthy budget and no rate-limit message is no longer retried and no longer wraps ErrRateLimited. WithApplicationTokenExpiration values at or below 90s now fall back to 10 minutes; the README previously documented 1 * time.Minute, which is affected.

One premise could not be verified against live traffic: that GitHub attaches rate-limit headers to terminal 403s with a healthy remaining count. It is verified against mocks and GitHub's documented client guidance only.

Each bug is pinned by a test written to fail against the current code
first.

A 403 was treated as rate limiting whenever X-RateLimit-Reset parsed.
GitHub attaches those headers to nearly every authenticated response, so
a terminal "Resource not accessible by integration" slept up to 60s,
retried, and returned wrapped in ErrRateLimited. A 403 now counts as
throttled only when it carries Retry-After, reports an exhausted budget,
or says so in its message. The message matters: a secondary rate limit
can arrive with a healthy primary budget and no Retry-After, and
classifying that as terminal would strip the ErrRateLimited callers
branch on.

The Identifier constraint is ~int64 | ~string, but resolveIssuer
type-switched on the exact dynamic type, so a named type such as
type AppID int64 was rejected as unsupported. It now switches on the
reflect kind, preserving the zero-value rejection.

Issuance is backdated 60s for clock drift, and only the upper bound was
clamped, so WithApplicationTokenExpiration(30 * time.Second) returned a
token that had expired 30s earlier. The lower bound is now the backdate
plus DefaultExpirySkew: below that the cache can never hold the token and
every call re-signs, which for a KMS, HSM or Vault signer is a remote
round trip per request. The backdating itself is unchanged, since it is
what keeps a 10 minute expiry under GitHub's ceiling on a fast clock.

Also caps the error body read at 64 KiB, corrects three comments claiming
a default JWT is usable for 9m30s when the backdating makes it 8m30s,
brings the ErrRateLimited and WithRetryOnThrottle godoc in line with the
new 403 contract, fixes a README example that used a value the new lower
bound rejects, and ignores the .claude scratch directory.
@codecov

codecov Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.94%. Comparing base (14058b0) to head (5c0a82e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
+ Coverage   96.50%   96.94%   +0.44%     
==========================================
  Files           4        4              
  Lines         286      295       +9     
==========================================
+ Hits          276      286      +10     
+ Misses         10        9       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jferrl
jferrl merged commit 2a37180 into main Sep 17, 2026
10 checks passed
@jferrl
jferrl deleted the fix/correctness branch September 18, 2026 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant