From 1bcf7b5f366424dd69944520f8375f3968480ee7 Mon Sep 17 00:00:00 2001 From: Anso Date: Wed, 2 Sep 2026 12:15:02 +0000 Subject: [PATCH] feat(git): classify Git host rate-limit responses as their own error state (#1884) A Git host throttle response (a 429, or a sideband message naming a rate limit or abuse-detection mechanism) previously fell through to AUTH_FAILED or a generic GIT_ERROR depending on the host's exact wording, telling the operator to check a credential that was never the problem. Adds RATE_LIMITED to the transport-facing error model, mapped to HTTP 429, checked ahead of the auth-shaped branches in classifyGitFailure. Proven against a real git binary: git's smart-HTTP client never surfaces the HTTP response body in stderr, only the status line, so a host that throttles via a bare 403 stays indistinguishable from a rejected credential and correctly classifies as AUTH_FAILED; only an explicit 429, or a sideband remote: message naming the throttle, classifies as RATE_LIMITED. --- backend/src/__tests__/git-source-http.test.ts | 6 +- .../src/__tests__/git-support-matrix.test.ts | 5 +- ...it-transport-ratelimit.integration.test.ts | 109 ++++++++++++++++ backend/src/__tests__/git-transport.test.ts | 116 ++++++++++++++++++ backend/src/services/GitSourceService.ts | 1 + backend/src/services/git/errors.ts | 30 ++++- backend/src/utils/gitSourceHttp.ts | 2 + docs/features/git-transport-support.mdx | 1 - docs/git-transport-support.yaml | 8 +- 9 files changed, 268 insertions(+), 10 deletions(-) create mode 100644 backend/src/__tests__/git-transport-ratelimit.integration.test.ts diff --git a/backend/src/__tests__/git-source-http.test.ts b/backend/src/__tests__/git-source-http.test.ts index 106031ae..c0fbef53 100644 --- a/backend/src/__tests__/git-source-http.test.ts +++ b/backend/src/__tests__/git-source-http.test.ts @@ -36,6 +36,10 @@ describe('gitSourceStatus', () => { expect(gitSourceStatus('NETWORK_TIMEOUT')).toBe(504); }); + it('maps RATE_LIMITED to 429, not to the auth or generic status', () => { + expect(gitSourceStatus('RATE_LIMITED')).toBe(429); + }); + it('maps PLAN_FINGERPRINT_REQUIRED to 400', () => { expect(gitSourceStatus('PLAN_FINGERPRINT_REQUIRED')).toBe(400); }); @@ -58,7 +62,7 @@ describe('gitSourceStatus', () => { it('has exactly one explicit mapping for every GitSourceErrorCode', () => { const codes: GitSourceErrorCode[] = [ 'REPO_NOT_FOUND', 'AUTH_FAILED', 'REF_NOT_FOUND', 'REF_DELETED', 'UNSUPPORTED_REF', - 'SSH_HOST_KEY_FAILED', 'FILE_NOT_FOUND', 'NETWORK_TIMEOUT', 'GIT_ERROR', 'STALE_PLAN', + 'SSH_HOST_KEY_FAILED', 'FILE_NOT_FOUND', 'RATE_LIMITED', 'NETWORK_TIMEOUT', 'GIT_ERROR', 'STALE_PLAN', 'PLAN_FINGERPRINT_REQUIRED', 'PLAN_BLOCKED', 'LEGACY_PENDING', 'PLAN_UNAVAILABLE', 'OPERATION_IN_FLIGHT', ]; diff --git a/backend/src/__tests__/git-support-matrix.test.ts b/backend/src/__tests__/git-support-matrix.test.ts index 8683fc27..672ce75e 100644 --- a/backend/src/__tests__/git-support-matrix.test.ts +++ b/backend/src/__tests__/git-support-matrix.test.ts @@ -391,8 +391,9 @@ describe('git transport support matrix', () => { }); describe('rate limiting', () => { - it('is documented as a limitation, not a supported claim', () => { - expect(limitationIds.has('no-rate-limit-classification')).toBe(true); + it('is classified in the error model, not left as a documented limitation', () => { + expect(limitationIds.has('no-rate-limit-classification')).toBe(false); + expect(support.error_model.some((e) => e.code === 'RATE_LIMITED')).toBe(true); }); }); }); diff --git a/backend/src/__tests__/git-transport-ratelimit.integration.test.ts b/backend/src/__tests__/git-transport-ratelimit.integration.test.ts new file mode 100644 index 00000000..c75a3a3f --- /dev/null +++ b/backend/src/__tests__/git-transport-ratelimit.integration.test.ts @@ -0,0 +1,109 @@ +/** + * Real end-to-end proof that a Git host throttle response classifies as + * RATE_LIMITED, not AUTH_FAILED or GIT_ERROR. + * + * The other classifier fixtures (git-transport.test.ts) construct stderr + * strings by hand; this file proves the string a real git binary actually + * produces, since git does not surface an HTTP response body over the + * smart-HTTP protocol; only the status line reaches stderr. A server that + * intercepts every request before touching a real repository is enough: + * resolveRef fails at ls-remote, before any fetch would need real + * repository content. + * + * Soft-skips when the system git binary is unavailable, mirroring the other + * native-git integration suites (see __helpers__/externalDeps.ts). + */ +import { promises as fs, readFileSync } from 'fs'; +import https from 'https'; +import os from 'os'; +import path from 'path'; +import { afterAll, afterEach, beforeAll, describe, expect, it } from 'vitest'; +import { classifyGitFailure, isTransportFailure } from '../services/git/errors'; +import { nativeGitTransport } from '../services/git/nativeGitTransport'; +import { requireGitBinary } from './__helpers__/externalDeps'; + +const FIXTURES_DIR = path.resolve(__dirname, '..', '..', '..', 'e2e', 'fixtures'); + +/** Throttle wording a real host would put in the response body, which git never shows. */ +const THROTTLE_BODY = 'You have exceeded a secondary rate limit. Please wait a few minutes before you try again.'; + +/** Serve a fixed HTTP status to every request, regardless of path or method. */ +function serveStatus(status: number): Promise<{ url: string; close: () => void }> { + return new Promise((resolve, reject) => { + const server = https.createServer( + { + cert: readFileSync(path.join(FIXTURES_DIR, 'git-server.pem')), + key: readFileSync(path.join(FIXTURES_DIR, 'git-server.key')), + }, + (req, res) => { + res.statusCode = status; + res.end(THROTTLE_BODY); + }, + ); + server.on('error', reject); + server.listen(0, '127.0.0.1', () => { + const address = server.address(); + if (address === null || typeof address === 'string') { + reject(new Error('server did not bind')); + return; + } + resolve({ url: `https://127.0.0.1:${address.port}/repo.git`, close: () => server.close() }); + }); + }); +} + +describe.skipIf(!requireGitBinary())('native git transport rate-limit classification (real git, real TLS)', () => { + let prevExtraCaCerts: string | undefined; + const workspaces: string[] = []; + + beforeAll(() => { + prevExtraCaCerts = process.env.NODE_EXTRA_CA_CERTS; + process.env.NODE_EXTRA_CA_CERTS = path.join(FIXTURES_DIR, 'git-ca.pem'); + }); + + afterAll(() => { + if (prevExtraCaCerts === undefined) delete process.env.NODE_EXTRA_CA_CERTS; + else process.env.NODE_EXTRA_CA_CERTS = prevExtraCaCerts; + }); + + afterEach(async () => { + await Promise.all(workspaces.splice(0).map((w) => fs.rm(w, { recursive: true, force: true }))); + }); + + async function makeWorkspace(): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'sencho-git-ratelimit-ws-')); + workspaces.push(dir); + return dir; + } + + // Both statuses are served with the same throttle body, which is the + // point of the pair: git shows only the status line, so a host that + // signals a throttle as a bare 403 is indistinguishable from a rejected + // credential and must still classify as AUTH_FAILED. + it.each([ + [429, 'RATE_LIMITED'], + [403, 'AUTH_FAILED'], + ] as const)('classifies a real %d response from the host as %s', async (status, code) => { + const served = await serveStatus(status); + try { + const workspaceRoot = await makeWorkspace(); + const failure = await nativeGitTransport + .resolveRef({ repoUrl: served.url, ref: 'main', token: 'irrelevant-token', timeoutMs: 15_000, workspaceRoot }) + .then(() => null, (e: unknown) => e); + + expect(isTransportFailure(failure)).toBe(true); + if (!isTransportFailure(failure)) throw new Error('unreachable'); + expect(failure.reason).toBe('exit'); + if (failure.reason === 'exit') { + // Pins the real stderr shape the classifier's hand-written + // fixtures (git-transport.test.ts) assume: the status line + // reaches stderr, the served body never does. + expect(failure.stderr).toMatch(new RegExp(`requested url returned error:\\s*${status}\\b`, 'i')); + expect(failure.stderr).not.toMatch(/secondary rate limit/i); + } + expect(classifyGitFailure(failure).code).toBe(code); + } finally { + served.close(); + } + }); +}); diff --git a/backend/src/__tests__/git-transport.test.ts b/backend/src/__tests__/git-transport.test.ts index 578613df..a3e21835 100644 --- a/backend/src/__tests__/git-transport.test.ts +++ b/backend/src/__tests__/git-transport.test.ts @@ -219,6 +219,122 @@ describe('classifyGitFailure (native git stderr corpus)', () => { expect(c.code).toBe('UNSUPPORTED_REF'); }); + it.each([ + // The HTTP shapes are verified against a real git binary talking to a + // fixture server (git-transport-ratelimit.integration.test.ts): git + // reports the status line only, never the response body. + ['bare 429 from the host', "fatal: unable to access 'https://h/x.git/': The requested URL returned error: 429"], + ['429 with trailing text', "fatal: unable to access 'https://h/x.git/': The requested URL returned error: 429 Too Many Requests"], + // Text a host sends through the pack stream does reach stderr as a + // remote: line, unlike an HTTP response body. + ['remote sideband rate-limit message', "remote: You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\nfatal: the remote end hung up unexpectedly"], + ['remote sideband abuse-detection message', "remote: You have triggered an abuse detection mechanism.\nfatal: the remote end hung up unexpectedly"], + ])('classifies %s as RATE_LIMITED', (_label, stderr) => { + const c = classifyGitFailure({ + transportFailure: true as const, + reason: 'exit', + stderr, + exitCode: 128, + host: 'github.com', + hasToken: true, + }); + expect(c.code).toBe('RATE_LIMITED'); + expect(c.message).toMatch(/rate limited/i); + }); + + it('classifies an unambiguous rate limit as RATE_LIMITED even without a token', () => { + // Rule 3 (see the module header) takes precedence over rule 2's + // no-token private-repo masking: a throttle leaks nothing about repo + // existence, so it should not be reported as REPO_NOT_FOUND. + const c = classifyGitFailure({ + transportFailure: true as const, + reason: 'exit', + stderr: "remote: You have exceeded a secondary rate limit.\nfatal: the remote end hung up unexpectedly", + exitCode: 128, + host: 'github.com', + hasToken: false, + }); + expect(c.code).toBe('RATE_LIMITED'); + }); + + it('does not send a rate-limited operator to rotate a working credential', () => { + // A sideband throttle message can arrive alongside a 403 fatal line, + // which the auth branch below would otherwise claim. Both the code + // and the message are asserted: reporting RATE_LIMITED while still + // saying "check your token" would leave the operator with the same + // wrong action. + const c = classifyGitFailure({ + transportFailure: true as const, + reason: 'exit', + stderr: "remote: You have exceeded a secondary rate limit.\nfatal: unable to access 'https://github.com/o/r.git/': The requested URL returned error: 403", + exitCode: 128, + host: 'github.com', + hasToken: true, + }); + expect(c.code).toBe('RATE_LIMITED'); + expect(c.message).not.toMatch(/check your token/i); + }); + + it('leaves a bare 403 with no rate-limit wording as an auth failure', () => { + // Guards the other direction, and pins a real constraint: git does + // not surface an HTTP response body, so a host that signals a + // throttle as a bare 403 is indistinguishable from a rejected + // credential. Widening the rate-limit branch to cover every 403 + // would make a genuinely bad token read as a throttle to wait out. + const c = classifyGitFailure({ + transportFailure: true as const, + reason: 'exit', + stderr: "fatal: unable to access 'https://github.com/o/r.git/': The requested URL returned error: 403", + exitCode: 128, + host: 'github.com', + hasToken: true, + }); + expect(c.code).toBe('AUTH_FAILED'); + }); + + it('does not mistake a repository named "rate-limiter" for a rate-limit signal', () => { + // git's fatal line echoes the full repo URL verbatim, so an + // unscoped rate-limit word match would fire on the path itself. A + // genuinely bad token against a repo whose name happens to contain + // rate-limit wording must still classify as an auth failure. + const c = classifyGitFailure({ + transportFailure: true as const, + reason: 'exit', + stderr: "fatal: unable to access 'https://github.com/acme/rate-limiter.git/': The requested URL returned error: 403", + exitCode: 128, + host: 'github.com', + hasToken: true, + }); + expect(c.code).toBe('AUTH_FAILED'); + }); + + it('does not mistake an upload-pack progress counter for a 429 status', () => { + // Progress lines like "Counting objects: 100% (429/429)" reach + // stderr from the server sideband and can contain the literal digits + // 429 with no connection to an HTTP status at all. + const c = classifyGitFailure({ + transportFailure: true as const, + reason: 'exit', + stderr: "remote: Counting objects: 100% (429/429), done.\nfatal: the remote end hung up unexpectedly", + exitCode: 128, + host: 'github.com', + hasToken: false, + }); + expect(c.code).toBe('NETWORK_TIMEOUT'); + }); + + it('does not mistake an unrelated transient-error sideband for a rate limit', () => { + const c = classifyGitFailure({ + transportFailure: true as const, + reason: 'exit', + stderr: "remote: Internal server error, please retry later\nfatal: the remote end hung up unexpectedly", + exitCode: 128, + host: 'github.com', + hasToken: false, + }); + expect(c.code).toBe('NETWORK_TIMEOUT'); + }); + it('scrubs credentials from the generic fallback tail', () => { const c = classifyGitFailure({ transportFailure: true as const, diff --git a/backend/src/services/GitSourceService.ts b/backend/src/services/GitSourceService.ts index 1bcf1810..f4db48f1 100644 --- a/backend/src/services/GitSourceService.ts +++ b/backend/src/services/GitSourceService.ts @@ -68,6 +68,7 @@ export type GitSourceErrorCode = | 'UNSUPPORTED_REF' | 'SSH_HOST_KEY_FAILED' | 'FILE_NOT_FOUND' + | 'RATE_LIMITED' | 'NETWORK_TIMEOUT' | 'GIT_ERROR' | 'STALE_PLAN' diff --git a/backend/src/services/git/errors.ts b/backend/src/services/git/errors.ts index 5b6b33ce..7c162ecc 100644 --- a/backend/src/services/git/errors.ts +++ b/backend/src/services/git/errors.ts @@ -7,12 +7,17 @@ * (service -> git/*) and the classifier is unit-testable in isolation. The * service wraps the returned pair in its own GitSourceError. * - * Two behaviors are contractual and pinned by tests; do not change them: + * Three behaviors are contractual and pinned by tests; do not change them: * 1. Authentication failure WITH a supplied token reports AUTH_FAILED, * which the HTTP layer maps to 400, never 401, because the frontend's * global logout trips on any API-level 401. * 2. A 401/403-shaped refusal WITHOUT a token reports REPO_NOT_FOUND with - * a private-repo hint, mirroring GitHub's masking of private repos. + * a private-repo hint, mirroring GitHub's masking of private repos, + * unless rule 3 claims it first. + * 3. An unambiguous rate-limit signal (a 429 status, or a server sideband + * line naming a throttle) reports RATE_LIMITED ahead of rules 1 and 2: + * a throttle leaks nothing about repo existence, so the rule-2 masking + * does not apply to it even when no token was supplied. */ export type TransportFacingCode = @@ -21,6 +26,7 @@ export type TransportFacingCode = | 'SSH_HOST_KEY_FAILED' | 'REF_NOT_FOUND' | 'UNSUPPORTED_REF' + | 'RATE_LIMITED' | 'NETWORK_TIMEOUT' | 'GIT_ERROR'; @@ -157,6 +163,26 @@ export function classifyGitFailure( }; } + // Rate limiting is checked ahead of the auth-shaped branches below, + // including the WITHOUT-a-token masking rule: a throttle is a throttle + // regardless of credentials. Without this branch a throttled fetch + // reaches the auth branch's /\b40[13]\b/ and tells the operator to rotate + // a credential that is not the problem. + // + // Both patterns stay narrow, because git's fatal line echoes the repo URL + // verbatim and sideband lines (always prefixed "remote:") carry arbitrary + // server text: a bare \b429\b would also match an upload-pack progress + // counter ("Counting objects: 100% (429/429)") or a number in the URL, + // and an unanchored word match would fire on a repo named "rate-limiter". + // Sideband wording is limited to throttle-specific phrases, since a + // generic "retry later" also describes a transient 5xx. + if (/returned error:\s*429\b/.test(raw) || /^remote:.*(too many requests|rate[ -]limit|abuse detection)/m.test(raw)) { + return { + code: 'RATE_LIMITED', + message: `Rate limited by${dest}. Too many requests were sent in a short window. Wait a few minutes and retry.`, + }; + } + // Auth-shaped refusals. Native git phrases these two ways: with a token // it gets "Authentication failed for ''"; without one it cannot even // answer and reports the disabled terminal prompt. diff --git a/backend/src/utils/gitSourceHttp.ts b/backend/src/utils/gitSourceHttp.ts index 379ce21c..f32f15fa 100644 --- a/backend/src/utils/gitSourceHttp.ts +++ b/backend/src/utils/gitSourceHttp.ts @@ -34,6 +34,8 @@ export function gitSourceStatus(code: GitSourceErrorCode): number { case 'PLAN_UNAVAILABLE': case 'OPERATION_IN_FLIGHT': return 409; + case 'RATE_LIMITED': + return 429; case 'NETWORK_TIMEOUT': return 504; case 'GIT_ERROR': diff --git a/docs/features/git-transport-support.mdx b/docs/features/git-transport-support.mdx index 20d04c95..7d07bb1e 100644 --- a/docs/features/git-transport-support.mdx +++ b/docs/features/git-transport-support.mdx @@ -57,7 +57,6 @@ Every "Supported" row here is backed by a test that runs on every change to Senc ## Not supported -- **No rate-limit classification.** A Git host rate-limit response (for example GitHub's secondary rate limits) is not classified as its own error state. It surfaces as an authentication failure or a generic transport error depending on the host's exact response. Wait and retry; there is no dedicated rate-limit message or backoff guidance yet. - **No Git LFS.** Compose and env files tracked via Git LFS are rejected rather than silently fetched as pointer stubs. Commit plain files instead. - **No submodules.** Submodule contents are not fetched. Inputs and build contexts that reference submodule contents are refused with an actionable message; a warning is shown when .gitmodules is present. - **No sparse or partial clone.** Every fetch materializes the complete repository at the resolved commit (shallow, single-branch); there is no sparse or partial clone for large monorepos. diff --git a/docs/git-transport-support.yaml b/docs/git-transport-support.yaml index d2fedfeb..0fa585b1 100644 --- a/docs/git-transport-support.yaml +++ b/docs/git-transport-support.yaml @@ -549,10 +549,6 @@ claims: attestation: att-2026-09-01-github-pilot limitations: - - id: no-rate-limit-classification - title: No rate-limit classification - statement: A Git host rate-limit response (for example GitHub's secondary rate limits) is not classified as its own error state. It surfaces as an authentication failure or a generic transport error depending on the host's exact response. Wait and retry; there is no dedicated rate-limit message or backoff guidance yet. - - id: no-git-lfs title: No Git LFS statement: Compose and env files tracked via Git LFS are rejected rather than silently fetched as pointer stubs. Commit plain files instead. @@ -606,6 +602,10 @@ error_model: label: Commit not reachable on this host status: 400 meaning: A pinned commit SHA that the Git host will not serve because it is not advertised by any branch or tag tip. + - code: RATE_LIMITED + label: Rate limited + status: 429 + meaning: The Git host throttled the request (an HTTP 429, or a sideband message naming a rate limit or abuse-detection mechanism). Wait and retry; there is no automated backoff yet. - code: NETWORK_TIMEOUT label: Network timeout status: 504