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..ccade75136 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 @@ -352,28 +352,100 @@ async function resolveLocalPublicSource( platform: CodeReviewPlatform, url: string ): Promise { - if (platform === PLATFORM.GITHUB) { - const parsed = parseGitHubPullRequestUrl(url); - const pullRequest = await fetchPublicGitHubPullRequest(parsed); - validateOpenGitHubPullRequest(pullRequest); - return buildGitHubSource(pullRequest, undefined); + try { + if (platform === PLATFORM.GITHUB) { + const parsed = parseGitHubPullRequestUrl(url); + const pullRequest = await fetchPublicGitHubPullRequest(parsed); + validateOpenGitHubPullRequest(pullRequest); + return buildGitHubSource(pullRequest, undefined); + } + + const parsed = parseGitLabMergeRequestUrl(url); + if (new URL(parsed.origin).hostname !== 'gitlab.com') { + throw new TRPCError({ + code: 'BAD_REQUEST', + message: 'Local Code Reviewer jobs only support public gitlab.com merge requests.', + }); + } + + const mergeRequest = await fetchPublicGitLabMergeRequest(parsed); + validateOpenGitLabMergeRequest(mergeRequest); + return buildGitLabSource({ + mergeRequest, + projectPath: parsed.projectPath, + integrationId: undefined, + platformProjectId: mergeRequest.target_project_id ?? mergeRequest.project_id, + }); + } catch (error) { + // The local (DEBUG_SHOW_DEV_UI) path reads the pull request from the provider's + // public API instead of a connected integration. A provider failure there used + // to escape as a raw Error, so tRPC answered 500 and the client showed a generic + // failure. Translate it into an actionable client error; the connected path + // already returns typed TRPCErrors. Existing TRPCErrors (invalid URL, closed or + // draft pull request) pass through unchanged. + throw toLocalSourceError(platform, error); } +} - const parsed = parseGitLabMergeRequestUrl(url); - if (new URL(parsed.origin).hostname !== 'gitlab.com') { - throw new TRPCError({ - code: 'BAD_REQUEST', - message: 'Local Code Reviewer jobs only support public gitlab.com merge requests.', +// `Response.json()` rejects with a SyntaxError built outside this realm, so +// `instanceof SyntaxError` misses it; match the error name instead. A ZodError +// from the response schema is built in this realm and matches directly. +function isProviderResponseParseError(error: unknown): boolean { + if (error instanceof z.ZodError) return true; + return ( + typeof error === 'object' && + error !== null && + 'name' in error && + Reflect.get(error, 'name') === 'SyntaxError' + ); +} + +function toLocalSourceError(platform: CodeReviewPlatform, error: unknown): TRPCError { + if (error instanceof TRPCError) return error; + + const provider = platform === PLATFORM.GITHUB ? 'GitHub' : 'GitLab'; + // GitHub calls these pull requests; GitLab calls them merge requests. + const requestNoun = platform === PLATFORM.GITHUB ? 'pull request' : 'merge request'; + if (error instanceof ProviderFetchError) { + if (error.status === 404) { + return new TRPCError({ + code: 'NOT_FOUND', + message: `${provider} could not find that ${requestNoun}. Check the URL, or make sure the repository is public.`, + cause: error, + }); + } + // GitHub signals primary and secondary rate limits with 403; GitLab uses 429. + // A GitLab 403 is a permission error, not a rate limit, so it falls through. + if (error.status === 429 || (error.status === 403 && platform === PLATFORM.GITHUB)) { + return new TRPCError({ + code: 'TOO_MANY_REQUESTS', + message: `${provider} rate-limited the request. Try again in a few minutes.`, + cause: error, + }); + } + return new TRPCError({ + code: 'BAD_GATEWAY', + message: `${provider} returned an unexpected response for that ${requestNoun}.`, + cause: error, + }); + } + + // The provider answered, but with a body that is not valid JSON or does not + // match its documented shape. We reached it, so "could not reach" would + // misdescribe what happened. + if (isProviderResponseParseError(error)) { + return new TRPCError({ + code: 'BAD_GATEWAY', + message: `${provider} returned an unexpected response for that ${requestNoun}.`, + cause: error, }); } - const mergeRequest = await fetchPublicGitLabMergeRequest(parsed); - validateOpenGitLabMergeRequest(mergeRequest); - return buildGitLabSource({ - mergeRequest, - projectPath: parsed.projectPath, - integrationId: undefined, - platformProjectId: mergeRequest.target_project_id ?? mergeRequest.project_id, + // Network failure, timeout, or redirect. + return new TRPCError({ + code: 'BAD_GATEWAY', + message: `Could not reach ${provider} to read that ${requestNoun}. Try again.`, + cause: error, }); } diff --git a/apps/web/src/routers/code-reviews-router.test.ts b/apps/web/src/routers/code-reviews-router.test.ts index c7d052524d..c849546f1c 100644 --- a/apps/web/src/routers/code-reviews-router.test.ts +++ b/apps/web/src/routers/code-reviews-router.test.ts @@ -691,6 +691,196 @@ describe('personalReviewAgent.createManualReviewJob', () => { userId: testUser.id, }); }); + + // The local (DEBUG_SHOW_DEV_UI) path reads the pull request from the provider's + // public API. A provider failure there used to escape as a raw Error, so tRPC + // answered 500 and the client showed a generic failure. Each case below must + // resolve to an actionable client error code instead of INTERNAL_SERVER_ERROR. + it.each([ + { status: 404, expectedCode: 'NOT_FOUND', expectedMessage: 'could not find that pull request' }, + { + status: 403, + expectedCode: 'TOO_MANY_REQUESTS', + expectedMessage: 'rate-limited the request', + }, + { status: 500, expectedCode: 'BAD_GATEWAY', expectedMessage: 'unexpected response' }, + ])( + 'maps a public GitHub pull request fetch that fails with $status to $expectedCode', + async ({ status, expectedCode, expectedMessage }) => { + fetchSpy?.mockImplementation(async () => new Response('provider failure', { status })); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'github', + url: prUrl, + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string } + ); + + expect(rejection.code).toBe(expectedCode); + expect(rejection.code).not.toBe('INTERNAL_SERVER_ERROR'); + expect(rejection.message).toContain(expectedMessage); + } + ); + + it('maps a missing public GitLab merge request to an actionable GitLab error', async () => { + fetchSpy?.mockImplementation(async () => new Response('Not Found', { status: 404 })); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'gitlab', + url: 'https://gitlab.com/group/project/-/merge_requests/1', + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string } + ); + + expect(rejection.code).toBe('NOT_FOUND'); + expect(rejection.message).toContain('GitLab'); + expect(rejection.message).toContain('merge request'); + expect(rejection.message).not.toContain('pull request'); + }); + + it('maps a public GitLab rate limit to TOO_MANY_REQUESTS', async () => { + fetchSpy?.mockImplementation(async () => new Response('Too Many Requests', { status: 429 })); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'gitlab', + url: 'https://gitlab.com/group/project/-/merge_requests/1', + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string } + ); + + expect(rejection.code).toBe('TOO_MANY_REQUESTS'); + expect(rejection.message).toContain('rate-limited the request'); + }); + + // GitLab returns 429 for rate limits; a 403 is a permission error and must not + // be reported to the user as a rate limit. + it('does not map a public GitLab 403 to a rate-limit error', async () => { + fetchSpy?.mockImplementation(async () => new Response('Forbidden', { status: 403 })); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'gitlab', + url: 'https://gitlab.com/group/project/-/merge_requests/1', + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string } + ); + + expect(rejection.code).toBe('BAD_GATEWAY'); + expect(rejection.message).toContain('unexpected response'); + expect(rejection.message).not.toContain('rate-limited'); + }); + + // A provider that answers with an unparseable body was reached; the error must + // say so rather than claiming the provider was unreachable, and keep the cause. + it('reports an unparseable provider response as unexpected, not unreachable', async () => { + fetchSpy?.mockImplementation( + async () => + new Response('not json', { status: 200, headers: { 'Content-Type': 'application/json' } }) + ); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'github', + url: prUrl, + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string; cause?: unknown } + ); + + expect(rejection.code).toBe('BAD_GATEWAY'); + expect(rejection.message).toContain('unexpected response'); + expect(rejection.message).not.toContain('Could not reach'); + // The original parse error is kept as the cause rather than dropped. + expect((rejection.cause as Error | undefined)?.message).toContain('JSON'); + }); + + // A well-formed JSON body that does not match the provider's documented shape + // is also a reached-but-unusable response, not an unreachable provider. + it('reports a provider response that fails schema validation as unexpected', async () => { + fetchSpy?.mockImplementation( + async () => + new Response(JSON.stringify({ unexpected: true }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }) + ); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'github', + url: prUrl, + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string; cause?: unknown } + ); + + expect(rejection.code).toBe('BAD_GATEWAY'); + expect(rejection.message).toContain('unexpected response'); + expect(rejection.message).not.toContain('Could not reach'); + expect(rejection.cause).toBeDefined(); + }); + + it('maps a provider network failure to a client error instead of a 500', async () => { + const networkError = new TypeError('fetch failed'); + fetchSpy?.mockImplementation(async () => { + throw networkError; + }); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'github', + url: prUrl, + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string; cause?: unknown } + ); + + expect(rejection.code).toBe('BAD_GATEWAY'); + expect(rejection.message).toContain('Could not reach GitHub'); + expect(rejection.cause).toBe(networkError); + }); }); describe('review agent config REVIEW.md setting', () => {