From 24d87e84a4a1d459de05e662afc4d4171f5eadfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sun, 20 Sep 2026 01:56:22 +0200 Subject: [PATCH] fix(web): map manual review provider failures to client errors https://github.com/Kilo-Org/cloud/pull/6405 --- .../manual-code-review-jobs.test.ts | 234 ++++++++++++++++++ .../code-reviews/manual-code-review-jobs.ts | 160 ++++++++++-- 2 files changed, 377 insertions(+), 17 deletions(-) create mode 100644 apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts diff --git a/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts b/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts new file mode 100644 index 0000000000..936197dff9 --- /dev/null +++ b/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts @@ -0,0 +1,234 @@ +import { TRPCError } from '@trpc/server'; +import { getHTTPStatusCodeFromError } from '@trpc/server/http'; + +const mockIsLocalCodeReviewDevelopmentEnabled = jest.fn(); +const mockGetAgentConfigForOwner = jest.fn(); +const mockAssertCouncilCreationAllowed = jest.fn(); +const mockCreateCodeReview = jest.fn(); +const mockTryDispatchPendingReviews = jest.fn(); +const mockGetAllIntegrationsForOwner = jest.fn(); +const mockGetValidGitLabToken = jest.fn(); +const mockFetchGitLabMergeRequest = jest.fn(); + +jest.mock('@/lib/config.server', () => ({ + isLocalCodeReviewDevelopmentEnabled: () => mockIsLocalCodeReviewDevelopmentEnabled(), +})); + +jest.mock('@/lib/agent-config/db/agent-configs', () => ({ + getAgentConfigForOwner: (...args: unknown[]) => mockGetAgentConfigForOwner(...args), +})); + +jest.mock('./core/council-entitlement', () => ({ + assertCouncilCreationAllowed: (...args: unknown[]) => mockAssertCouncilCreationAllowed(...args), +})); + +jest.mock('@/lib/integrations/db/platform-integrations', () => ({ + getAllIntegrationsForOwner: (...args: unknown[]) => mockGetAllIntegrationsForOwner(...args), + getIntegrationForOwner: jest.fn(), + updateIntegrationMetadataForOwner: jest.fn(), +})); + +jest.mock('@/lib/integrations/gitlab-service', () => ({ + getValidGitLabToken: (...args: unknown[]) => mockGetValidGitLabToken(...args), +})); + +jest.mock('@/lib/integrations/platforms/gitlab/adapter', () => ({ + fetchGitLabMergeRequest: (...args: unknown[]) => mockFetchGitLabMergeRequest(...args), +})); + +jest.mock('./db/code-reviews', () => ({ + createCodeReview: (...args: unknown[]) => mockCreateCodeReview(...args), + findActiveProviderPublishingReview: jest.fn(), +})); + +jest.mock('./dispatch/dispatch-pending-reviews', () => ({ + tryDispatchPendingReviews: (...args: unknown[]) => mockTryDispatchPendingReviews(...args), +})); + +import { createManualCodeReviewJob } from './manual-code-review-jobs'; + +const OWNER = { type: 'user' as const, id: 'user-1', userId: 'user-1' }; + +const GITHUB_PR_URL = 'https://github.com/owner/repo/pull/123'; +const GITLAB_MR_URL = 'https://gitlab.com/group/project/-/merge_requests/123'; + +function providerResponse(status: number, body: unknown): Response { + return { + ok: status >= 200 && status < 300, + status, + text: async () => JSON.stringify(body), + json: async () => body, + } as unknown as Response; +} + +function taskInput(overrides: Record = {}) { + return { + platform: 'github' as const, + url: GITHUB_PR_URL, + modelSlug: 'test-model', + ...overrides, + }; +} + +async function captureError(overrides: Record = {}): Promise { + try { + await createManualCodeReviewJob({ owner: OWNER, input: taskInput(overrides) }); + return null; + } catch (error) { + return error; + } +} + +beforeEach(() => { + jest.clearAllMocks(); + mockIsLocalCodeReviewDevelopmentEnabled.mockReturnValue(true); + mockGetAgentConfigForOwner.mockResolvedValue(null); + mockAssertCouncilCreationAllowed.mockResolvedValue(undefined); +}); + +describe('createManualCodeReviewJob provider failures', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + // Every provider round-trip failure must surface as a mapped tRPC error. + // Before the mapping landed these escaped as raw + // ProviderFetchError/TypeError/ZodError and tRPC answered + // INTERNAL_SERVER_ERROR (HTTP 500) — the finding's defect. A provider that is + // genuinely unreachable maps to 502/504, which is the correct gateway status + // and not the reported internal error. + const cases: Array<{ + name: string; + fetch: () => void; + code: TRPCError['code']; + }> = [ + { + name: 'a missing public pull request maps to NOT_FOUND', + fetch: () => + void jest + .spyOn(global, 'fetch') + .mockResolvedValue(providerResponse(404, { message: 'Not Found' })), + code: 'NOT_FOUND', + }, + { + name: 'a rate-limited GitHub maps to TOO_MANY_REQUESTS', + fetch: () => + void jest + .spyOn(global, 'fetch') + .mockResolvedValue(providerResponse(403, { message: 'API rate limit exceeded' })), + code: 'TOO_MANY_REQUESTS', + }, + { + name: 'an unreachable provider maps to BAD_GATEWAY', + fetch: () => + void jest.spyOn(global, 'fetch').mockRejectedValue(new TypeError('fetch failed')), + code: 'BAD_GATEWAY', + }, + { + name: 'a provider timeout maps to GATEWAY_TIMEOUT', + fetch: () => { + const timeout = new Error('The operation was aborted due to timeout'); + timeout.name = 'TimeoutError'; + void jest.spyOn(global, 'fetch').mockRejectedValue(timeout); + }, + code: 'GATEWAY_TIMEOUT', + }, + { + name: 'an unexpected provider shape maps to BAD_GATEWAY', + fetch: () => + void jest.spyOn(global, 'fetch').mockResolvedValue(providerResponse(200, { nope: true })), + code: 'BAD_GATEWAY', + }, + ]; + + it.each(cases)('$name', async ({ fetch, code }) => { + fetch(); + + const error = await captureError(); + + expect(error).toBeInstanceOf(TRPCError); + expect((error as TRPCError).code).toBe(code); + // The finding's defect was tRPC's unmapped INTERNAL_SERVER_ERROR / HTTP 500. + expect((error as TRPCError).code).not.toBe('INTERNAL_SERVER_ERROR'); + expect(getHTTPStatusCodeFromError(error as TRPCError)).not.toBe(500); + }); +}); + +describe('createManualCodeReviewJob connected GitLab failures', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('maps an unreadable merge request from the connected instance to a client error', async () => { + mockIsLocalCodeReviewDevelopmentEnabled.mockReturnValue(false); + mockGetAllIntegrationsForOwner.mockResolvedValue([ + { + id: 'integration-1', + platform: 'gitlab', + integration_status: 'active', + metadata: { gitlab_instance_url: 'https://gitlab.com' }, + repositories: [], + }, + ]); + mockGetValidGitLabToken.mockResolvedValue('token'); + // The GitLab adapter throws a plain Error whose message ends with the status. + mockFetchGitLabMergeRequest.mockRejectedValue(new Error('GitLab MR fetch failed: 404')); + + const error = await captureError({ platform: 'gitlab', url: GITLAB_MR_URL }); + + expect(error).toBeInstanceOf(TRPCError); + expect((error as TRPCError).code).toBe('NOT_FOUND'); + expect(getHTTPStatusCodeFromError(error as TRPCError)).not.toBe(500); + }); + + it('maps a connected GitLab timeout to GATEWAY_TIMEOUT, not BAD_GATEWAY', async () => { + mockIsLocalCodeReviewDevelopmentEnabled.mockReturnValue(false); + mockGetAllIntegrationsForOwner.mockResolvedValue([ + { + id: 'integration-1', + platform: 'gitlab', + integration_status: 'active', + metadata: { gitlab_instance_url: 'https://gitlab.com' }, + repositories: [], + }, + ]); + mockGetValidGitLabToken.mockResolvedValue('token'); + // The GitLab adapter runs its own request timeout and destroys the request + // with a plain Error named 'Error' and this message. + mockFetchGitLabMergeRequest.mockRejectedValue(new Error('GitLab request timed out')); + + const error = await captureError({ platform: 'gitlab', url: GITLAB_MR_URL }); + + expect(error).toBeInstanceOf(TRPCError); + expect((error as TRPCError).code).toBe('GATEWAY_TIMEOUT'); + expect(getHTTPStatusCodeFromError(error as TRPCError)).toBe(504); + }); +}); + +describe('createManualCodeReviewJob happy path', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('creates the job from a public pull request and dispatches pending reviews', async () => { + jest.spyOn(global, 'fetch').mockResolvedValue( + providerResponse(200, { + number: 123, + html_url: GITHUB_PR_URL, + title: 'Fix the thing', + state: 'open', + draft: false, + user: { login: 'octocat', id: 1 }, + base: { ref: 'main', repo: { full_name: 'owner/repo' } }, + head: { ref: 'feature', sha: 'abc123' }, + }) + ); + mockCreateCodeReview.mockResolvedValue('review-1'); + + await expect(createManualCodeReviewJob({ owner: OWNER, input: taskInput() })).resolves.toEqual({ + reviewId: 'review-1', + outputMode: 'kilo', + }); + expect(mockTryDispatchPendingReviews).toHaveBeenCalledWith(OWNER); + }); +}); diff --git a/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts b/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts index 0636d765f2..ef19320ae2 100644 --- a/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts +++ b/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts @@ -460,22 +460,33 @@ async function resolveConnectedGitLabSource( userId: owner.userId, ...(owner.type === 'org' ? { organizationId: owner.id } : {}), }); - const rawMergeRequest = await fetchGitLabMergeRequest({ - accessToken, - projectId: parsed.projectPath, - mrIid: parsed.mrIid, - instanceUrl, - }); - const mergeRequest = GitLabMergeRequestApiSchema.parse(rawMergeRequest); - validateOpenGitLabMergeRequest(mergeRequest); + let rawMergeRequest: unknown; + try { + rawMergeRequest = await fetchGitLabMergeRequest({ + accessToken, + projectId: parsed.projectPath, + mrIid: parsed.mrIid, + instanceUrl, + }); + } catch (error) { + // The adapter throws a plain Error whose message ends with the provider + // status; map it so an unreadable merge request is a client error, not a + // 500. See toProviderRequestError. + throw toProviderRequestError(error, PLATFORM.GITLAB); + } + const mergeRequest = GitLabMergeRequestApiSchema.safeParse(rawMergeRequest); + if (!mergeRequest.success) { + throw unreadableProviderResponse('GitLab'); + } + validateOpenGitLabMergeRequest(mergeRequest.data); return buildGitLabSource({ - mergeRequest, + mergeRequest: mergeRequest.data, projectPath: parsed.projectPath, integrationId: integration.id, platformProjectId: - mergeRequest.target_project_id ?? - mergeRequest.project_id ?? + mergeRequest.data.target_project_id ?? + mergeRequest.data.project_id ?? getGitLabRepositoryIdFromIntegration(integration, parsed.projectPath), }); } @@ -603,10 +614,19 @@ async function fetchPublicGitHubPullRequest( parsed: ParsedGitHubPullRequestUrl ): Promise { const url = `https://api.github.com/repos/${encodeURIComponent(parsed.owner)}/${encodeURIComponent(parsed.repo)}/pulls/${parsed.prNumber}`; - const data = await fetchJson(url, { - headers: { Accept: 'application/vnd.github+json' }, - }); - return GitHubPullRequestApiSchema.parse(data); + let data: unknown; + try { + data = await fetchJson(url, { + headers: { Accept: 'application/vnd.github+json' }, + }); + } catch (error) { + throw toProviderRequestError(error, PLATFORM.GITHUB); + } + const pullRequest = GitHubPullRequestApiSchema.safeParse(data); + if (!pullRequest.success) { + throw unreadableProviderResponse('GitHub'); + } + return pullRequest.data; } async function fetchGitHubPullRequest( @@ -634,8 +654,17 @@ async function fetchPublicGitLabMergeRequest( parsed: ParsedGitLabMergeRequestUrl ): Promise { const url = `https://gitlab.com/api/v4/projects/${encodeURIComponent(parsed.projectPath)}/merge_requests/${parsed.mrIid}`; - const data = await fetchJson(url, { headers: { Accept: 'application/json' } }); - return GitLabMergeRequestApiSchema.parse(data); + let data: unknown; + try { + data = await fetchJson(url, { headers: { Accept: 'application/json' } }); + } catch (error) { + throw toProviderRequestError(error, PLATFORM.GITLAB); + } + const mergeRequest = GitLabMergeRequestApiSchema.safeParse(data); + if (!mergeRequest.success) { + throw unreadableProviderResponse('GitLab'); + } + return mergeRequest.data; } async function fetchJson(url: string, init: RequestInit): Promise { @@ -652,6 +681,103 @@ async function fetchJson(url: string, init: RequestInit): Promise { return await response.json(); } +/** + * Pull the upstream HTTP status out of a provider failure. + * + * `ProviderFetchError` carries it directly. The GitLab adapter throws a plain + * `Error` whose message ends with the status (`GitLab MR fetch failed: 403`) — + * the same contract `classifyGitLabError` in `provider-review/gitlab-authorization` + * relies on. + */ +function providerErrorStatus(error: unknown): number | null { + if (error instanceof ProviderFetchError) return error.status; + if (error instanceof Error) { + const match = error.message.match(/:\s*(\d{3})\b/); + const status = match?.[1] ? Number(match[1]) : Number.NaN; + if (Number.isInteger(status) && status >= 400 && status < 600) return status; + } + return null; +} + +function providerLabel(platform: CodeReviewPlatform): 'GitHub' | 'GitLab' { + return platform === PLATFORM.GITLAB ? 'GitLab' : 'GitHub'; +} + +/** + * The GitLab adapter runs its own `req.setTimeout` and destroys the request with + * a plain `Error` named `Error` and the message below. Unlike `AbortSignal` + * timeouts it carries no `TimeoutError`/`AbortError` name, so recognize the + * message too — otherwise a connected-GitLab timeout falls through to + * BAD_GATEWAY instead of GATEWAY_TIMEOUT. + */ +const GITLAB_ADAPTER_TIMEOUT_MESSAGE = 'GitLab request timed out'; + +function isProviderTimeout(error: unknown): boolean { + if (!(error instanceof Error)) return false; + if (error.name === 'TimeoutError' || error.name === 'AbortError') return true; + return error.message === GITLAB_ADAPTER_TIMEOUT_MESSAGE; +} + +/** + * A provider answered, but the body did not match the expected shape. That is an + * upstream/gateway failure, not a fault in the caller's request — so it must be a + * 502, never an unmapped error that tRPC turns into a 500. + */ +function unreadableProviderResponse(provider: 'GitHub' | 'GitLab'): TRPCError { + return new TRPCError({ + code: 'BAD_GATEWAY', + message: `We couldn't read ${provider}'s response for that request. Try again in a moment.`, + }); +} + +/** + * Convert a provider round-trip failure into a client-visible tRPC error. + * + * The manual-review resolver talks to GitHub/GitLab while resolving the pull + * request. Those fetches raise raw errors (`ProviderFetchError`, a network + * `TypeError`, a timeout `DOMException`, or a schema `ZodError`). Left unmapped + * they reach tRPC as unknown errors and the mutation answers + * INTERNAL_SERVER_ERROR — a 500 for what is usually user input: a nonexistent or + * private pull request, a rate-limited API, or a provider outage. Map each onto + * the closest client error so the mutation never returns 500 for a provider + * round-trip. + */ +function toProviderRequestError(error: unknown, platform: CodeReviewPlatform): TRPCError { + if (error instanceof TRPCError) return error; + const provider = providerLabel(platform); + const noun = platform === PLATFORM.GITLAB ? 'merge request' : 'pull request'; + const status = providerErrorStatus(error); + + if (status === 404) { + return new TRPCError({ + code: 'NOT_FOUND', + message: `We couldn't find that ${provider} ${noun}. Check the URL and that you have access to it.`, + }); + } + if (status === 403 || status === 429) { + return new TRPCError({ + code: 'TOO_MANY_REQUESTS', + message: `${provider} is limiting requests right now. Try again in a few minutes.`, + }); + } + if (status !== null && status >= 400 && status < 500) { + return new TRPCError({ + code: 'BAD_REQUEST', + message: `${provider} rejected that request. Check the ${noun} URL and try again.`, + }); + } + if (isProviderTimeout(error)) { + return new TRPCError({ + code: 'GATEWAY_TIMEOUT', + message: `${provider} took too long to respond. Try again.`, + }); + } + return new TRPCError({ + code: 'BAD_GATEWAY', + message: `We couldn't reach ${provider} to read that ${noun}. Try again in a moment.`, + }); +} + function validateOpenGitHubPullRequest(pullRequest: GitHubPullRequestApi): void { if (pullRequest.state !== 'open') { throw new TRPCError({