From ffca49b82620bdbd2a9e738377a0155141095288 Mon Sep 17 00:00:00 2001 From: "Maple (gastown)" Date: Tue, 28 Apr 2026 21:20:25 +0000 Subject: [PATCH 1/2] feat(github): add fetchPullRequestForBranch helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a small Octokit helper that looks up the PR associated with a (repo, branch) pair using an existing GitHub App installation token. Used exclusively by the manual 'Refresh PR info' mutation — the common case is webhook-driven. Intentionally no caching or in-flight dedup since the caller is throttled to once per 10s per session. - Returns most recently updated PR whose head ref matches, preferring open PRs when multiple exist. - Maps merged_at != null to a 'merged' state. - Throws GitHubRateLimitError on 403/429 or x-ratelimit-remaining=0, surfacing the parsed reset timestamp so callers can report it. - Returns null on 404 (repo no longer accessible / deleted). Bead 1 of 5 in the 'Associated PR for cloud-agent-next session' convoy. --- .../integrations/platforms/github/adapter.ts | 120 ++++++++- .../fetch-pull-request-for-branch.test.ts | 245 ++++++++++++++++++ .../integrations/platforms/github/adapter.ts | 28 ++ 3 files changed, 392 insertions(+), 1 deletion(-) create mode 100644 apps/web/src/lib/integrations/platforms/github/fetch-pull-request-for-branch.test.ts diff --git a/apps/web/src/lib/integrations/platforms/github/adapter.ts b/apps/web/src/lib/integrations/platforms/github/adapter.ts index e016e584f1..cd8a2f70ae 100644 --- a/apps/web/src/lib/integrations/platforms/github/adapter.ts +++ b/apps/web/src/lib/integrations/platforms/github/adapter.ts @@ -1,7 +1,7 @@ import { Octokit } from '@octokit/rest'; import { createAppAuth } from '@octokit/auth-app'; import { exchangeWebFlowCode } from '@octokit/oauth-methods'; -import { logExceptInTest } from '@/lib/utils.server'; +import { logExceptInTest, warnExceptInTest } from '@/lib/utils.server'; import crypto from 'crypto'; import type { InstallationToken } from '@/lib/integrations/core/types'; @@ -647,6 +647,124 @@ function isHttpError(error: unknown): error is { status: number; message: string ); } +export type AssociatedPullRequest = { + number: number; + htmlUrl: string; + state: 'open' | 'closed' | 'merged'; + title: string; + headSha: string; + updatedAt: string; // ISO +}; + +/** + * Thrown when GitHub returns a rate-limit response. The caller can surface + * `resetAt` to the user so they know when to retry. + */ +export class GitHubRateLimitError extends Error { + public readonly resetAt: Date; + constructor(resetAt: Date) { + super(`GitHub rate limited until ${resetAt.toISOString()}`); + this.name = 'GitHubRateLimitError'; + this.resetAt = resetAt; + } +} + +function getResponseHeader(error: unknown, name: string): string | undefined { + if (typeof error !== 'object' || error === null) return undefined; + const response = (error as { response?: { headers?: Record } }).response; + const headers = response?.headers; + if (!headers) return undefined; + const value = headers[name] ?? headers[name.toLowerCase()]; + return typeof value === 'string' ? value : undefined; +} + +function parseRateLimitResetAt(error: unknown): Date { + const resetHeader = getResponseHeader(error, 'x-ratelimit-reset'); + const resetSeconds = resetHeader ? Number(resetHeader) : NaN; + if (Number.isFinite(resetSeconds) && resetSeconds > 0) { + return new Date(resetSeconds * 1000); + } + // Fall back to "retry in 60s" if the header is missing/invalid, so callers + // always have a usable Date to show. + return new Date(Date.now() + 60_000); +} + +function isRateLimitError(error: unknown): boolean { + if (!isHttpError(error)) return false; + if (error.status === 403 || error.status === 429) return true; + const remaining = getResponseHeader(error, 'x-ratelimit-remaining'); + return remaining === '0'; +} + +/** + * Look up the pull request associated with a `(repo, branch)` pair using an + * installation token. Returns the most recently updated PR whose head ref + * matches `branch`, preferring `open` PRs when multiple exist. + * + * This helper is only invoked from the manual "Refresh PR info" mutation; the + * webhook path updates the DB directly. Intentionally no caching or dedup — + * the mutation is throttled server-side to once per 10s per session. + * + * @returns The associated PR, or `null` if no PR matches (or the repo is no + * longer accessible to this installation). + * @throws {GitHubRateLimitError} when GitHub rate-limits the request. + */ +export async function fetchPullRequestForBranch(params: { + installationId: number; + owner: string; + repo: string; + branch: string; + appType: GitHubAppType; +}): Promise { + const { installationId, owner, repo, branch, appType } = params; + + const tokenData = await generateGitHubInstallationToken(String(installationId), appType); + const octokit = new Octokit({ auth: tokenData.token }); + + try { + const { data: prs } = await octokit.pulls.list({ + owner, + repo, + head: `${owner}:${branch}`, + state: 'all', + per_page: 10, + sort: 'updated', + direction: 'desc', + }); + + if (prs.length === 0) { + return null; + } + + const chosen = prs.find(pr => pr.state === 'open') ?? prs[0]; + + const state: AssociatedPullRequest['state'] = + chosen.merged_at != null ? 'merged' : chosen.state === 'open' ? 'open' : 'closed'; + + return { + number: chosen.number, + htmlUrl: chosen.html_url, + state, + title: chosen.title, + headSha: chosen.head.sha, + updatedAt: chosen.updated_at, + }; + } catch (error) { + if (isRateLimitError(error)) { + throw new GitHubRateLimitError(parseRateLimitResetAt(error)); + } + if (isHttpError(error) && error.status === 404) { + warnExceptInTest('[fetchPullRequestForBranch] Repo not accessible or deleted', { + owner, + repo, + branch, + }); + return null; + } + throw error; + } +} + /** * Get repository details including whether it's empty. * Used to validate target repo before migration. diff --git a/apps/web/src/lib/integrations/platforms/github/fetch-pull-request-for-branch.test.ts b/apps/web/src/lib/integrations/platforms/github/fetch-pull-request-for-branch.test.ts new file mode 100644 index 0000000000..bdaa2d35cf --- /dev/null +++ b/apps/web/src/lib/integrations/platforms/github/fetch-pull-request-for-branch.test.ts @@ -0,0 +1,245 @@ +/** + * Tests for `fetchPullRequestForBranch` in `adapter.ts`. + * + * The adapter module is globally mocked via the jest `moduleNameMapper` entry + * for `@/lib/integrations/platforms/github/adapter`. We bypass that by importing + * via a relative path, then mock `@octokit/rest` and the installation-token + * generation so we never hit real GitHub. + */ + +process.env.GITHUB_APP_ID = 'test-app-id'; +process.env.GITHUB_APP_PRIVATE_KEY = 'test-private-key'; +process.env.GITHUB_LITE_APP_ID = 'test-lite-app-id'; +process.env.GITHUB_LITE_APP_PRIVATE_KEY = 'test-lite-private-key'; + +const mockPullsList = jest.fn(); + +jest.mock('@octokit/rest', () => ({ + Octokit: jest.fn().mockImplementation(() => ({ + pulls: { list: mockPullsList }, + })), +})); + +jest.mock('@octokit/auth-app', () => ({ + createAppAuth: jest.fn(() => async () => ({ token: 'mock-token', expiresAt: '2099-01-01' })), +})); + +import { fetchPullRequestForBranch, GitHubRateLimitError } from './adapter'; + +type ListResponse = { + data: Array<{ + number: number; + html_url: string; + state: 'open' | 'closed'; + title: string; + updated_at: string; + merged_at: string | null; + head: { sha: string }; + }>; +}; + +function okResponse(data: ListResponse['data']): ListResponse { + return { data }; +} + +function httpError(status: number, headers: Record = {}) { + return Object.assign(new Error(`HTTP ${status}`), { + status, + response: { headers }, + }); +} + +beforeEach(() => { + mockPullsList.mockReset(); +}); + +describe('fetchPullRequestForBranch', () => { + const params = { + installationId: 42, + owner: 'acme', + repo: 'widgets', + branch: 'feature/login', + appType: 'standard' as const, + }; + + it('returns the single open PR when the API returns one result', async () => { + mockPullsList.mockResolvedValueOnce( + okResponse([ + { + number: 7, + html_url: 'https://github.com/acme/widgets/pull/7', + state: 'open', + title: 'Add login page', + updated_at: '2026-04-20T10:00:00Z', + merged_at: null, + head: { sha: 'abc123' }, + }, + ]) + ); + + const result = await fetchPullRequestForBranch(params); + + expect(result).toEqual({ + number: 7, + htmlUrl: 'https://github.com/acme/widgets/pull/7', + state: 'open', + title: 'Add login page', + headSha: 'abc123', + updatedAt: '2026-04-20T10:00:00Z', + }); + + expect(mockPullsList).toHaveBeenCalledWith({ + owner: 'acme', + repo: 'widgets', + head: 'acme:feature/login', + state: 'all', + per_page: 10, + sort: 'updated', + direction: 'desc', + }); + }); + + it('prefers an open PR over a more-recently-updated closed one', async () => { + mockPullsList.mockResolvedValueOnce( + okResponse([ + { + number: 9, + html_url: 'https://github.com/acme/widgets/pull/9', + state: 'closed', + title: 'Old attempt', + updated_at: '2026-04-22T10:00:00Z', + merged_at: null, + head: { sha: 'closedsha' }, + }, + { + number: 7, + html_url: 'https://github.com/acme/widgets/pull/7', + state: 'open', + title: 'Current attempt', + updated_at: '2026-04-20T10:00:00Z', + merged_at: null, + head: { sha: 'opensha' }, + }, + ]) + ); + + const result = await fetchPullRequestForBranch(params); + + expect(result).toMatchObject({ number: 7, state: 'open', headSha: 'opensha' }); + }); + + it('returns merged state when merged_at is set', async () => { + mockPullsList.mockResolvedValueOnce( + okResponse([ + { + number: 11, + html_url: 'https://github.com/acme/widgets/pull/11', + state: 'closed', + title: 'Merged work', + updated_at: '2026-04-22T10:00:00Z', + merged_at: '2026-04-22T09:59:00Z', + head: { sha: 'mergedsha' }, + }, + ]) + ); + + const result = await fetchPullRequestForBranch(params); + + expect(result?.state).toBe('merged'); + }); + + it('falls back to the first result when no open PR exists', async () => { + mockPullsList.mockResolvedValueOnce( + okResponse([ + { + number: 2, + html_url: 'https://github.com/acme/widgets/pull/2', + state: 'closed', + title: 'Later closed', + updated_at: '2026-04-22T10:00:00Z', + merged_at: null, + head: { sha: 'sha-2' }, + }, + { + number: 1, + html_url: 'https://github.com/acme/widgets/pull/1', + state: 'closed', + title: 'Earlier closed', + updated_at: '2026-04-20T10:00:00Z', + merged_at: null, + head: { sha: 'sha-1' }, + }, + ]) + ); + + const result = await fetchPullRequestForBranch(params); + + expect(result?.number).toBe(2); + expect(result?.state).toBe('closed'); + }); + + it('returns null when no PRs match', async () => { + mockPullsList.mockResolvedValueOnce(okResponse([])); + + const result = await fetchPullRequestForBranch(params); + + expect(result).toBeNull(); + }); + + it('returns null on 404 (repo no longer accessible)', async () => { + mockPullsList.mockRejectedValueOnce(httpError(404)); + + const result = await fetchPullRequestForBranch(params); + + expect(result).toBeNull(); + }); + + it('throws GitHubRateLimitError on 403 with rate-limit reset header', async () => { + const resetEpochSeconds = 1_800_000_000; + mockPullsList.mockRejectedValueOnce( + httpError(403, { + 'x-ratelimit-remaining': '0', + 'x-ratelimit-reset': String(resetEpochSeconds), + }) + ); + + await expect(fetchPullRequestForBranch(params)).rejects.toMatchObject({ + name: 'GitHubRateLimitError', + resetAt: new Date(resetEpochSeconds * 1000), + }); + }); + + it('throws GitHubRateLimitError on 429', async () => { + const resetEpochSeconds = 1_800_000_100; + mockPullsList.mockRejectedValueOnce( + httpError(429, { 'x-ratelimit-reset': String(resetEpochSeconds) }) + ); + + const error = await fetchPullRequestForBranch(params).catch(e => e); + expect(error).toBeInstanceOf(GitHubRateLimitError); + if (error instanceof GitHubRateLimitError) { + expect(error.resetAt).toEqual(new Date(resetEpochSeconds * 1000)); + } + }); + + it('throws GitHubRateLimitError when x-ratelimit-remaining is 0 even without 403/429', async () => { + const resetEpochSeconds = 1_800_000_200; + // Status 200 is nonsensical with this error shape, but the helper should + // still recognise the rate-limit signal from the header. + mockPullsList.mockRejectedValueOnce( + httpError(500, { + 'x-ratelimit-remaining': '0', + 'x-ratelimit-reset': String(resetEpochSeconds), + }) + ); + + await expect(fetchPullRequestForBranch(params)).rejects.toBeInstanceOf(GitHubRateLimitError); + }); + + it('re-throws unexpected errors unchanged', async () => { + const boom = new Error('network exploded'); + mockPullsList.mockRejectedValueOnce(boom); + + await expect(fetchPullRequestForBranch(params)).rejects.toBe(boom); + }); +}); diff --git a/apps/web/src/tests/setup/__mocks__/lib/integrations/platforms/github/adapter.ts b/apps/web/src/tests/setup/__mocks__/lib/integrations/platforms/github/adapter.ts index 1535a4eddb..6fc9ba576a 100644 --- a/apps/web/src/tests/setup/__mocks__/lib/integrations/platforms/github/adapter.ts +++ b/apps/web/src/tests/setup/__mocks__/lib/integrations/platforms/github/adapter.ts @@ -68,3 +68,31 @@ export async function updateCheckRun( ): Promise { return; } + +export type AssociatedPullRequest = { + number: number; + htmlUrl: string; + state: 'open' | 'closed' | 'merged'; + title: string; + headSha: string; + updatedAt: string; +}; + +export class GitHubRateLimitError extends Error { + public readonly resetAt: Date; + constructor(resetAt: Date) { + super(`GitHub rate limited until ${resetAt.toISOString()}`); + this.name = 'GitHubRateLimitError'; + this.resetAt = resetAt; + } +} + +export async function fetchPullRequestForBranch(_params: { + installationId: number; + owner: string; + repo: string; + branch: string; + appType: GitHubAppType; +}): Promise { + return null; +} From 85437c613a6f5738d09e570f1630186208f40cb6 Mon Sep 17 00:00:00 2001 From: "Maple (gastown)" Date: Tue, 28 Apr 2026 21:26:38 +0000 Subject: [PATCH 2/2] fix(github): only classify 403 as rate limit when message indicates it Non-rate-limit 403 responses (e.g. installation missing pull request permission) were being wrapped as GitHubRateLimitError, which would incorrectly tell users to retry instead of surfacing the permission problem. Now only treat 403 as rate-limited when the x-ratelimit headers or the error message ('rate limit', 'secondary rate limit', 'abuse') indicate so. 429 remains unambiguously rate-limited, and x-ratelimit-remaining: 0 still triggers the rate-limit path at any status. --- .../integrations/platforms/github/adapter.ts | 26 ++++++++++++++-- .../fetch-pull-request-for-branch.test.ts | 30 +++++++++++++++++-- 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/apps/web/src/lib/integrations/platforms/github/adapter.ts b/apps/web/src/lib/integrations/platforms/github/adapter.ts index cd8a2f70ae..87e1ae93f8 100644 --- a/apps/web/src/lib/integrations/platforms/github/adapter.ts +++ b/apps/web/src/lib/integrations/platforms/github/adapter.ts @@ -689,11 +689,33 @@ function parseRateLimitResetAt(error: unknown): Date { return new Date(Date.now() + 60_000); } +function getErrorMessage(error: unknown): string { + if (typeof error !== 'object' || error === null) return ''; + const message = (error as { message?: unknown }).message; + return typeof message === 'string' ? message : ''; +} + function isRateLimitError(error: unknown): boolean { if (!isHttpError(error)) return false; - if (error.status === 403 || error.status === 429) return true; + // 429 is unambiguously rate limiting. + if (error.status === 429) return true; + // `x-ratelimit-remaining: 0` signals the primary rate limit is exhausted + // regardless of status. const remaining = getResponseHeader(error, 'x-ratelimit-remaining'); - return remaining === '0'; + if (remaining === '0') return true; + // 403 is overloaded: it can mean rate/abuse limiting OR a plain permission + // denial (e.g. installation lacks pull request access). Only treat 403 as + // rate-limited when the message indicates so, so that genuine permission + // failures are surfaced to the caller. + if (error.status === 403) { + const message = getErrorMessage(error).toLowerCase(); + return ( + message.includes('rate limit') || + message.includes('secondary rate limit') || + message.includes('abuse') + ); + } + return false; } /** diff --git a/apps/web/src/lib/integrations/platforms/github/fetch-pull-request-for-branch.test.ts b/apps/web/src/lib/integrations/platforms/github/fetch-pull-request-for-branch.test.ts index bdaa2d35cf..4fce84ca75 100644 --- a/apps/web/src/lib/integrations/platforms/github/fetch-pull-request-for-branch.test.ts +++ b/apps/web/src/lib/integrations/platforms/github/fetch-pull-request-for-branch.test.ts @@ -42,8 +42,12 @@ function okResponse(data: ListResponse['data']): ListResponse { return { data }; } -function httpError(status: number, headers: Record = {}) { - return Object.assign(new Error(`HTTP ${status}`), { +function httpError( + status: number, + headers: Record = {}, + message?: string +) { + return Object.assign(new Error(message ?? `HTTP ${status}`), { status, response: { headers }, }); @@ -236,6 +240,28 @@ describe('fetchPullRequestForBranch', () => { await expect(fetchPullRequestForBranch(params)).rejects.toBeInstanceOf(GitHubRateLimitError); }); + it('throws GitHubRateLimitError on 403 with "secondary rate limit" message', async () => { + mockPullsList.mockRejectedValueOnce( + httpError(403, {}, 'You have exceeded a secondary rate limit.') + ); + + await expect(fetchPullRequestForBranch(params)).rejects.toBeInstanceOf(GitHubRateLimitError); + }); + + it('re-throws non-rate-limit 403 (permission denied) unchanged', async () => { + // GitHub returns 403 when an installation lacks the required permission + // (e.g. pull request read access). We must NOT wrap this as a rate limit, + // or callers will incorrectly tell users to retry. + const permissionError = httpError( + 403, + {}, + 'Resource not accessible by integration' + ); + mockPullsList.mockRejectedValueOnce(permissionError); + + await expect(fetchPullRequestForBranch(params)).rejects.toBe(permissionError); + }); + it('re-throws unexpected errors unchanged', async () => { const boom = new Error('network exploded'); mockPullsList.mockRejectedValueOnce(boom);