Skip to content

feat(github): classify GitHub rate-limit, abuse-detection, and auth responses - #221

Open
John-David Dalton (jdalton) wants to merge 1 commit into
mainfrom
jdalton/github-error-classification
Open

feat(github): classify GitHub rate-limit, abuse-detection, and auth responses#221
John-David Dalton (jdalton) wants to merge 1 commit into
mainfrom
jdalton/github-error-classification

Conversation

@jdalton

Copy link
Copy Markdown
Collaborator

What this adds

A GitHub API response that is throttled or rejected does not always look like an error. GitHub sends its primary rate limit as an HTTP 403 carrying x-ratelimit-remaining: 0, which has no distinguishing status code at all. Code that reads the body without first checking the status reads that response as "this repository has nothing to return" and reports a run as successful when in fact nothing was fetched.

This PR adds github/error-classification, a small pure module that looks at a status code, the response headers, and the body text, and tells you whether you are looking at a rate limit, GitHub's abuse detection, or an auth failure. It takes plain data and returns plain data. There is no fetch call, no logger, and no result type in it, so any caller can run it against whatever HTTP client it already uses.

The three conditions it recognizes are all blocking: they depend on your credential and on the clock, not on the resource you asked for. If you are looping over a hundred repositories and you hit one of them, every remaining repository will fail the same way. A caller should stop the loop rather than retry the request or quietly skip to the next repository. A plain permission denial is deliberately not in the set, because that one really is about the specific resource, so skipping it and carrying on is the right behaviour.

Why it belongs here rather than in a caller

socket-lib already owns the retry half of this problem. releases/github-retry-config holds GITHUB_RETRY_CONFIG and documents itself as covering "the transient-failure / rate-limit surface", and promises/retry holds pRetry. What was missing was the step before the retry decision: working out what a given response actually is. Without it, every caller that talks to the GitHub API has to rebuild the same status-and-header reading, and two copies of that logic will drift.

The exported surface is four values and three types, and every one of them is exercised by a test
Export What it is for
classifyGitHubErrorResponse(response) The main entry point. Takes { body, headers, status } and returns a classification, or undefined when the response is not one of the three blocking conditions.
getGitHubRateLimitWaitSeconds(headers) How many seconds until the limit resets, read from Retry-After or from x-ratelimit-reset.
getGitHubResponseHeader(headers, name) Reads a single header out of either header shape a caller might hold.
GITHUB_BLOCKING_ERROR_KINDS The frozen list of kinds the classifier can return, so a caller can build its own lookup table from it instead of hard-coding one and going stale when a fourth kind appears.
GitHubErrorKind, GitHubErrorClassification, GitHubResponseHeaders The accompanying types.

Two details worth calling out. First, the classifier accepts both a Fetch Headers object and the plain record that Node's HTTP layer produces, because callers in this codebase hold both shapes and neither should have to convert before asking a question. A Headers already matches names case-insensitively; a plain record does not, so the record path compares lowercased keys rather than trusting the caller to have normalized them.

Second, the Retry-After header is parsed by the existing http-request/headers helper rather than by new code. RFC 7231 allows that header to be either a number of seconds or an absolute HTTP date, and parseRetryAfterHeader already handles both. Reusing it means there is one parser for that header in this repository instead of two.

Order matters inside the classifier: abuse detection is checked before the primary rate limit because both arrive as HTTP 403

GitHub's secondary rate limit, which it calls abuse detection, arrives as a 403 with a body that mentions it. The primary rate limit also arrives as a 403. So does an ordinary permission denial. The status code alone cannot tell them apart, which is why the classifier reads the body and the x-ratelimit-remaining header as well.

The checks run most-specific first:

  1. 403 with secondary rate limit or abuse detection in the body becomes abuse-detection.
  2. 429, or 403 with x-ratelimit-remaining: 0, or 403 with rate limit in the body, becomes rate-limit.
  3. 401 becomes auth-failure, and is marked retryable: false because the same token never recovers by waiting.
  4. Anything else returns undefined, so the caller keeps its own handling of 404s, empty repositories, permission denials, and transient 5xx responses.

Testing

The new suite is 27 cases and covers the module at 100% of lines, statements, branches, and functions, which clears the repository's 99/99/95 bar.

Every detector was mutation-checked: broken on purpose, confirmed a named test went red, then restored

A test you have not watched fail is not evidence. Each detector below was disabled in turn, the suite was run, and the named test that caught it is recorded. The source was restored and confirmed byte-identical afterwards.

Mutation applied Named test that went red
x-ratelimit-remaining: 0 detector removed reads a 403 with x-ratelimit-remaining: 0 as a rate limit, reads a 403 with x-ratelimit-remaining: 0 as a rate limit from a header record
abuse-detection branch removed reads a 403 secondary rate limit as abuse detection, reads a 403 abuse-detection body as abuse detection, prefers abuse detection over the rate limit when both would match
401 branch removed reads a 401 as an auth failure that waiting cannot clear
429 no longer counts as a rate limit reads a 429 as a rate limit even with no body, carries the reset window on a rate limit
auth failure marked retryable: true reads a 401 as an auth failure that waiting cannot clear
every 403 treated as a rate limit returns undefined for a 403 permission denial with quota remaining
Retry-After ignored in favour of x-ratelimit-reset carries the reset window on a rate limit, prefers retry-after in seconds, accepts an HTTP-date retry-after
elapsed reset window no longer floored at zero floors an already-elapsed reset at zero

The first attempt at this is worth recording, because it caught a bad test. The x-ratelimit-remaining: 0 cases originally used a body that also said "rate limit", so deleting the header detector left the suite green — the body detector was silently covering for it. The fix was a SILENT_BODY fixture that says nothing about throttling, so a header test now proves the header detector fired. That is the exact shape of the bug this module exists to prevent, so it mattered that the test could actually see it.

Ran

  • node scripts/fleet/test.mts test/unit/github/error-classification.test.mts — 27 passed, harness exit 0.
  • Coverage over the new module via the repository's vitest config — 100% lines, statements, branches, and functions.
  • node scripts/fleet/lint.mts — passed, exit 0, after fixing the import-sort, function-sort, and explicit-undefined findings it raised.
  • node scripts/fleet/format.mts — exit 0, and the formatter's rewrites are included in this commit.
  • tsc --noEmit -p .config/fleet/tsconfig.check.json — exit 0.
  • node scripts/repo/make-api-md.mts and node scripts/fleet/gen/llms-txt.mts — regenerated rather than hand-edited, so docs/api.md and llms.txt pick the new subpath up automatically.
  • The eight mutation checks in the table above.

Did not run

  • The full pnpm run check --all suite to completion. A partial run showed check-dispatch-table-is-current already failing on a hook-bundle artifact that this branch does not touch; this branch is the default branch plus three files, so that finding is pre-existing rather than something introduced here.
  • The full test suite. This change adds a new module and does not modify any existing one, so nothing else has a path to it yet.

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