From 1d9df2b695c20961f01f0db9d2aa85770e4211f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Mon, 31 Aug 2026 09:52:31 +0200 Subject: [PATCH 1/2] feat(provider-review): expose the neutral review facade --- .../src/lib/github-pr-review/retry.test.ts | 141 +- apps/web/src/lib/github-pr-review/retry.ts | 35 +- .../lib/provider-review/github-bridge.test.ts | 1466 +++++++++++++++++ .../src/lib/provider-review/github-bridge.ts | 1077 ++++++++++++ .../src/routers/github-pr-review-router.ts | 83 +- .../routers/provider-review-router.test.ts | 796 +++++++++ .../web/src/routers/provider-review-router.ts | 667 ++++++++ apps/web/src/routers/root-router.ts | 2 + packages/trpc/src/mobile.ts | 2 + 9 files changed, 4229 insertions(+), 40 deletions(-) create mode 100644 apps/web/src/lib/provider-review/github-bridge.test.ts create mode 100644 apps/web/src/lib/provider-review/github-bridge.ts create mode 100644 apps/web/src/routers/provider-review-router.test.ts create mode 100644 apps/web/src/routers/provider-review-router.ts diff --git a/apps/web/src/lib/github-pr-review/retry.test.ts b/apps/web/src/lib/github-pr-review/retry.test.ts index d5a687c4b4..ae583eb6ca 100644 --- a/apps/web/src/lib/github-pr-review/retry.test.ts +++ b/apps/web/src/lib/github-pr-review/retry.test.ts @@ -4,16 +4,17 @@ import { TRPCError } from '@trpc/server'; const getGitHubUserAccessToken = jest.fn(); +const createGitHubPrReviewOctokit = jest.fn((token: string) => ({ __token: token })); jest.mock('@/lib/integrations/platforms/github/user-token-client', () => ({ getGitHubUserAccessToken: (...args: unknown[]) => getGitHubUserAccessToken(...args), })); jest.mock('./client', () => ({ - createGitHubPrReviewOctokit: (token: string) => ({ __token: token }), + createGitHubPrReviewOctokit: (token: string) => createGitHubPrReviewOctokit(token), })); -import { withGitHubUserTokenRetry } from './retry'; +import { withGitHubReviewIdentity, withGitHubUserTokenRetry } from './retry'; function connected(token: string, authorizationId: string, credentialVersion: number) { return { @@ -34,6 +35,7 @@ function http401() { beforeEach(() => { getGitHubUserAccessToken.mockReset(); + createGitHubPrReviewOctokit.mockReset().mockImplementation(token => ({ __token: token })); }); describe('withGitHubUserTokenRetry', () => { @@ -117,3 +119,138 @@ describe('withGitHubUserTokenRetry', () => { expect(call).not.toHaveBeenCalled(); }); }); + +describe('scoped GitHub review identity', () => { + const identity = { accountId: 'u1', authorizationId: 'auth_1', actorId: '101' }; + + it('rejects another account before its protected call', async () => { + getGitHubUserAccessToken.mockResolvedValue(connected('t1', 'auth_1', 1)); + const effects: string[] = []; + await expect( + withGitHubReviewIdentity(identity, () => + withGitHubUserTokenRetry({ + kiloUserId: 'u2', + call: async () => { + effects.push('wrong-account'); + return 'sent'; + }, + }) + ) + ).rejects.toMatchObject({ code: 'CONFLICT', message: 'review_identity_or_revision_changed' }); + expect(effects).toEqual([]); + }); + + it('isolates overlapping scopes across awaits and leaves concurrent direct calls unscoped', async () => { + getGitHubUserAccessToken.mockImplementation(async userId => + connected(userId, `auth_${userId}`, 1) + ); + createGitHubPrReviewOctokit.mockImplementation(token => ({ + __token: token, + users: { + getAuthenticated: async () => { + if (token === 'u3') throw new Error('Direct calls must not query actor identity'); + return { data: { id: token === 'u1' ? 101 : 102 } }; + }, + }, + })); + const effects: string[] = []; + const call = async (client: unknown) => { + const token = (client as { __token: string }).__token; + effects.push(token); + return token; + }; + const firstGate = Promise.withResolvers(); + const secondGate = Promise.withResolvers(); + const start = (userId: string, actorId: string, gate: Promise) => + withGitHubReviewIdentity( + { accountId: userId, authorizationId: `auth_${userId}`, actorId }, + async () => { + await gate; + const result = await withGitHubUserTokenRetry({ kiloUserId: userId, call }); + await expect( + withGitHubUserTokenRetry({ kiloUserId: userId === 'u1' ? 'u2' : 'u1', call }) + ).rejects.toMatchObject({ code: 'CONFLICT' }); + return result; + } + ); + const first = start('u1', '101', firstGate.promise); + const second = start('u2', '102', secondGate.promise); + expect(await withGitHubUserTokenRetry({ kiloUserId: 'u3', call })).toBe('u3'); + firstGate.resolve(); + expect(await first).toBe('u1'); + secondGate.resolve(); + expect(await second).toBe('u2'); + expect(await withGitHubUserTokenRetry({ kiloUserId: 'u3', call })).toBe('u3'); + expect(effects).toEqual(['u3', 'u1', 'u2', 'u3']); + }); + + it('retains unscoped rotation to a replacement authorization after a scoped rejection', async () => { + getGitHubUserAccessToken.mockResolvedValue(connected('t2', 'auth_2', 2)); + const effects: string[] = []; + await expect( + withGitHubReviewIdentity(identity, () => + withGitHubUserTokenRetry({ + kiloUserId: 'u1', + call: async () => { + effects.push('scoped'); + return 'sent'; + }, + }) + ) + ).rejects.toMatchObject({ code: 'CONFLICT' }); + getGitHubUserAccessToken + .mockResolvedValueOnce(connected('t1', 'auth_1', 1)) + .mockResolvedValueOnce(connected('t2', 'auth_2', 2)); + expect( + await withGitHubUserTokenRetry({ + kiloUserId: 'u1', + call: async client => { + const token = (client as typeof client & { __token: string }).__token; + if (token === 't1') throw http401(); + effects.push(token); + return 'completed'; + }, + }) + ).toBe('completed'); + expect(effects).toEqual(['t2']); + }); + + it.each([false, true])( + 'handles identity lookup 401 through normal rotation: rejected=%s', + async rejected => { + getGitHubUserAccessToken + .mockResolvedValueOnce(connected('t1', 'auth_1', 1)) + .mockResolvedValueOnce(connected('t2', 'auth_1', 2)) + .mockResolvedValueOnce({ status: 'disconnected', reason: 'revoked' }); + createGitHubPrReviewOctokit.mockImplementation(token => ({ + __token: token, + users: { + getAuthenticated: async () => { + if (token === 't1' || rejected) throw http401(); + return { data: { id: 101 } }; + }, + }, + })); + const effects: string[] = []; + const result = withGitHubReviewIdentity(identity, () => + withGitHubUserTokenRetry({ + kiloUserId: 'u1', + call: async () => { + effects.push('confirmed-actor'); + return 'completed'; + }, + }) + ); + if (rejected) { + await expect(result).rejects.toMatchObject({ + code: 'PRECONDITION_FAILED', + message: 'GitHub connection is no longer valid — reconnect', + }); + expect(effects).toEqual([]); + } else { + expect(await result).toBe('completed'); + expect(effects).toEqual(['confirmed-actor']); + } + } + ); +}); diff --git a/apps/web/src/lib/github-pr-review/retry.ts b/apps/web/src/lib/github-pr-review/retry.ts index f98874d950..addfee6fc3 100644 --- a/apps/web/src/lib/github-pr-review/retry.ts +++ b/apps/web/src/lib/github-pr-review/retry.ts @@ -1,5 +1,6 @@ import 'server-only'; +import { AsyncLocalStorage } from 'node:async_hooks'; import { TRPCError } from '@trpc/server'; import type { Octokit } from '@octokit/rest'; @@ -19,6 +20,23 @@ export type CurrentCredential = Pick< 'authorizationId' | 'credentialVersion' >; +type GitHubReviewIdentity = Readonly<{ + accountId: string; + authorizationId: string; + actorId: string; +}>; +const reviewIdentity = new AsyncLocalStorage(); + +// Only the neutral bridge opts in. Legacy callers keep their existing token retry behavior. +export function withGitHubReviewIdentity(identity: GitHubReviewIdentity, call: () => T): T { + const { accountId, authorizationId, actorId } = identity; + return reviewIdentity.run({ accountId, authorizationId, actorId }, call); +} + +export function hasGitHubReviewIdentity(accountId: string): boolean { + return reviewIdentity.getStore()?.accountId === accountId; +} + function throwTrpcFromClassification(classified: ClassifiedGitHubError): never { throw new TRPCError({ code: classified.code, @@ -60,6 +78,19 @@ export async function withGitHubUserTokenRetry(args: { kiloUserId: string; call: (octokit: Octokit) => Promise; }): Promise { + const expected = reviewIdentity.getStore(); + const conflict = () => + new TRPCError({ code: 'CONFLICT', message: 'review_identity_or_revision_changed' }); + if (expected && expected.accountId !== args.kiloUserId) throw conflict(); + const invoke = async (credential: CurrentCredential, octokit: Octokit) => { + if (expected) { + if (credential.authorizationId !== expected.authorizationId) throw conflict(); + const { data: actor } = await octokit.users.getAuthenticated(); + if (!Number.isSafeInteger(actor.id) || actor.id <= 0 || String(actor.id) !== expected.actorId) + throw conflict(); + } + return args.call(octokit); + }; const first = await getGitHubUserAccessToken(args.kiloUserId, { op: 'fetch' }); if (first.status !== 'connected') { throwTrpcFromClassification({ @@ -69,7 +100,7 @@ export async function withGitHubUserTokenRetry(args: { } const firstOctokit = createGitHubPrReviewOctokit(first.credential.token); try { - return await args.call(firstOctokit); + return await invoke(first.credential, firstOctokit); } catch (error) { // A TRPCError is an already-classified failure (e.g. a GraphQL errors[] // entry, or a deliberate BAD_REQUEST) — surface it unchanged. @@ -91,7 +122,7 @@ export async function withGitHubUserTokenRetry(args: { } const secondOctokit = createGitHubPrReviewOctokit(rotate.credential.token); try { - return await args.call(secondOctokit); + return await invoke(rotate.credential, secondOctokit); } catch (secondError) { if (secondError instanceof TRPCError) throw secondError; if (isHttp401(secondError)) { diff --git a/apps/web/src/lib/provider-review/github-bridge.test.ts b/apps/web/src/lib/provider-review/github-bridge.test.ts new file mode 100644 index 0000000000..84b0c5be4a --- /dev/null +++ b/apps/web/src/lib/provider-review/github-bridge.test.ts @@ -0,0 +1,1466 @@ +import { TRPCError } from '@trpc/server'; +import type { User } from '@kilocode/db/schema'; +import { user_terms_acceptances } from '@kilocode/db/schema'; +import { + githubPrReviewRouter, + PR_REVIEW_GRAPHQL_DOCUMENTS, +} from '@/routers/github-pr-review-router'; +import { repositoryResourceKey } from '@kilocode/app-shared/code-review/repository-identity'; +import { providerReviewRouter } from '@/routers/provider-review-router'; +import { createGitHubReviewBridge } from './github-bridge'; +import { getGitHubUserAccessToken } from '@/lib/integrations/platforms/github/user-token-client'; +import { createGitHubPrReviewOctokit } from '@/lib/github-pr-review/client'; +import { getAllIntegrationsForOwner } from '@/lib/integrations/db/platform-integrations'; + +jest.mock('@/lib/trpc/init', () => { + const t = jest.requireActual('@trpc/server').initTRPC.create(); + return { baseProcedure: t.procedure, createTRPCRouter: t.router }; +}); +jest.mock('@/lib/config.server', () => ({})); +jest.mock('@/lib/tokens', () => ({})); +jest.mock('@/routers/organizations/utils', () => ({ ensureOrganizationAccess: jest.fn() })); +jest.mock('@/lib/integrations/db/platform-integrations', () => ({ + getAllIntegrationsForOwner: jest.fn(), +})); +jest.mock('@/lib/provider-review/gitlab-authorization', () => ({})); +jest.mock('@/lib/provider-review/gitlab-read', () => ({})); +jest.mock('@/lib/provider-review/bitbucket-read', () => ({})); +jest.mock('@/lib/provider-review/gitlab-write', () => ({})); +jest.mock('@/lib/provider-review/bitbucket-write', () => ({})); +jest.mock('@/lib/integrations/platforms/github/user-token-client', () => ({ + getGitHubUserAccessToken: jest.fn(), +})); +jest.mock('@/lib/github-pr-review/client', () => ({ createGitHubPrReviewOctokit: jest.fn() })); +jest.mock('@/lib/drizzle', () => ({ + db: { + select: () => ({ + from: (table: unknown) => ({ + where: () => ({ + limit: async () => { + if (table === user_terms_acceptances) { + await gates.terms(); + return termsAccepted ? [{ id: 'terms' }] : []; + } + return row ? [row] : []; + }, + }), + }), + }), + }, +})); +jest.mock('@kilocode/db/operation-ledger', () => ({ + admitOperation: async (_db: unknown, input: any) => { + await gates.admission(); + if (row) + return { + admission: row.status === 'completed' ? 'duplicate_settled' : 'duplicate_reconcile_pending', + row, + }; + row = { + id: 'ledger-row', + intent: input.intent, + resource_key: input.resourceKey, + status: 'admitted', + canonical_result: null, + }; + return { admission: 'admitted', row }; + }, + settleOperation: async (_db: unknown, input: any) => { + row.status = input.status; + row.canonical_result = input.canonicalResult; + }, + recordOperationAcceptance: async (_db: unknown, input: any) => { + row.canonical_result = input.canonicalResult; + }, + markReconcilePending: async () => { + row.status = 'reconcile_pending'; + return row; + }, +})); + +const ctx = { user: { id: 'oauth/caller' } as User }; +const direct = githubPrReviewRouter.createCaller(ctx); +const facade = providerReviewRouter.createCaller(ctx); +const address = { owner: 'Team', repo: 'Repo', number: 7 }; +const operationKey = '55555555-5555-4555-8555-555555555555'; +const headSha = 'a'.repeat(40); +const baseSha = 'b'.repeat(40); +const repo = { + id: 42, + full_name: 'Team/Repo', + default_branch: 'trunk', + allow_merge_commit: true, + allow_squash_merge: true, + allow_rebase_merge: false, + allow_auto_merge: true, + allow_update_branch: true, + permissions: { push: true, admin: false }, +}; +const octokit = { + users: { getAuthenticated: jest.fn() }, + pulls: { + get: jest.fn(), + listFiles: jest.fn(), + createReview: jest.fn(), + createReviewComment: jest.fn(), + createReplyForReviewComment: jest.fn(), + getReview: jest.fn(), + getReviewComment: jest.fn(), + merge: jest.fn(), + updateBranch: jest.fn(), + }, + repos: { + get: jest.fn(), + compareCommits: jest.fn(), + getContent: jest.fn(), + listCommitStatusesForRef: jest.fn(), + }, + checks: { listForRef: jest.fn() }, + issues: { listComments: jest.fn() }, + git: { deleteRef: jest.fn() }, + paginate: jest.fn(), + request: jest.fn(), +}; +const gates = { terms: jest.fn(), admission: jest.fn() }; +let row: any; +let termsAccepted: boolean; +let writes: unknown[]; +let pull: any; +let inbox: any[]; + +beforeEach(() => { + jest.resetAllMocks(); + jest.mocked(createGitHubPrReviewOctokit).mockReturnValue(octokit as any); + row = null; + termsAccepted = true; + writes = []; + inbox = []; + pull = { + node_id: 'PR_7', + number: 7, + title: 'Review me', + body: 'Description', + user: { login: 'author', avatar_url: 'https://avatars.example/author' }, + state: 'open', + draft: false, + merged: false, + base: { ref: 'trunk', sha: baseSha, repo }, + head: { ref: 'feature', sha: headSha, repo }, + commits: 1, + changed_files: 0, + additions: 0, + deletions: 0, + mergeable: true, + mergeable_state: 'clean', + }; + jest.mocked(getGitHubUserAccessToken).mockResolvedValue({ + status: 'connected', + credential: { + connected: true, + token: 'fixture-token', + expiresAtEpochMs: Date.now() + 3600000, + githubLogin: 'reviewer', + authorizationId: 'authorization-1', + credentialVersion: 1, + }, + }); + jest.mocked(getAllIntegrationsForOwner).mockResolvedValue([]); + octokit.users.getAuthenticated.mockResolvedValue({ + data: { id: 99, login: 'reviewer', name: 'Reviewer' }, + }); + octokit.repos.get.mockResolvedValue({ data: repo }); + octokit.repos.compareCommits.mockResolvedValue({ data: { merge_base_commit: { sha: baseSha } } }); + octokit.pulls.get.mockImplementation(async () => ({ data: pull })); + octokit.pulls.listFiles.mockResolvedValue({ data: [] }); + octokit.paginate.mockResolvedValue([]); + octokit.issues.listComments.mockResolvedValue({ data: [] }); + octokit.request.mockImplementation(async (_route, input) => ({ + data: { + data: input.variables?.q + ? { search: { nodes: inbox, pageInfo: { hasNextPage: false, endCursor: null } } } + : { + repository: { + pullRequest: { + reviewDecision: 'APPROVED', + reviewThreads: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }, + }, + }, + viewer: { login: 'reviewer' }, + }, + }, + })); + octokit.pulls.createReview.mockImplementation(async input => { + writes.push(input); + return { + data: { + id: 81, + node_id: 'REVIEW_81', + state: + input.event === 'APPROVE' + ? 'APPROVED' + : input.event === 'REQUEST_CHANGES' + ? 'CHANGES_REQUESTED' + : 'COMMENTED', + }, + }; + }); + octokit.pulls.createReviewComment.mockImplementation(async input => { + writes.push(input); + return { data: { id: 82, node_id: 'COMMENT_82' } }; + }); + octokit.pulls.getReview.mockResolvedValue({ + data: { id: 81, node_id: 'REVIEW_81', state: 'COMMENTED' }, + }); +}); +afterEach(() => expect(getAllIntegrationsForOwner).not.toHaveBeenCalled()); + +it.each([0, 2])( + 'AC10 direct reads, facade reads, inbox, paste and restored drafts ignore %s installations', + async count => { + jest.mocked(getAllIntegrationsForOwner).mockResolvedValue( + Array.from({ length: count }, (_, index) => ({ + id: `unrelated-${index}`, + platform: 'github', + })) as any + ); + const old = await direct.getPullRequest(address); + expect(old).toMatchObject({ number: 7, headSha, repo: { viewerLogin: 'reviewer' } }); + expect(old).not.toHaveProperty('identity'); + const overview = await facade.getReview({ review: address }); + expect(overview.identity).toEqual({ + repository: { + provider: 'github', + instanceUrl: 'https://github.com', + repositoryId: '42', + fullName: 'Team/Repo', + defaultBranch: 'trunk', + }, + authorization: { + kind: 'githubUser', + accountId: ctx.user.id, + authorizationId: 'authorization-1', + }, + reviewId: 'PR_7', + number: '7', + canonicalUrl: 'https://github.com/Team/Repo/pull/7', + }); + expect(overview.authorization.actor).toMatchObject({ id: '99', login: 'reviewer' }); + inbox = [ + { + number: 7, + title: 'Assigned review', + isDraft: false, + updatedAt: '2026-08-30T00:00:00Z', + author: null, + repository: { name: 'Repo', owner: { login: 'Team' } }, + }, + ]; + expect(await direct.listInbox({})).toEqual({ + items: [ + { + ...address, + title: 'Assigned review', + isDraft: false, + updatedAt: '2026-08-30T00:00:00Z', + author: null, + }, + ], + nextCursor: null, + }); + expect(await facade.listInbox({})).toMatchObject({ + items: [{ identity: overview.identity, title: 'Assigned review' }], + scope: { kind: 'actor', actor: { id: '99' } }, + }); + expect( + await facade.resolveUrl({ url: 'http://www.github.com/Team/Repo/pull/7/files#diff' }) + ).toEqual(overview.identity); + const saved = JSON.parse( + '{"owner":"Team","repo":"Repo","number":7,"accountId":"oauth/caller","futureField":{"unknown":true}}' + ); + expect((await facade.getReview({ review: saved })).identity).toEqual(overview.identity); + } +); +it('AC10 normalizes absent provider and default metadata without guessing a branch', async () => { + const oldRepo = { ...repo, default_branch: undefined }; + octokit.repos.get.mockResolvedValue({ data: oldRepo }); + pull.base = { ...pull.base, sha: undefined, repo: oldRepo }; + const overview = await facade.getReview({ + review: { repository: { fullName: 'Team/Repo' }, number: 7 }, + }); + expect(overview.identity.repository.defaultBranch).toBeNull(); + expect(overview.revision).toEqual({ + headSha, + baseSha: null, + startSha: null, + targetHeadSha: null, + }); + expect(overview.checks).toEqual({ status: 'none', checks: [] }); +}); +it.each(['not_connected', 'revoked'] as const)( + 'AC10 preserves %s user errors even with installation access', + async reason => { + jest + .mocked(getAllIntegrationsForOwner) + .mockResolvedValue([{ platform: 'github', id: 'installed' }] as any); + jest.mocked(getGitHubUserAccessToken).mockResolvedValue({ status: 'disconnected', reason }); + const oldError = await direct.getPullRequest(address).catch(error => error); + await expect(facade.getReview({ review: address })).rejects.toMatchObject({ + code: oldError.code, + message: oldError.message, + }); + expect(await facade.getAuthorization({})).toEqual({ + status: 'not_connected', + reason, + authorization: null, + actor: null, + }); + expect(writes).toEqual([]); + } +); +it.each([403, 404, 429, 503])( + 'AC10 preserves GitHub HTTP %s error classification', + async status => { + octokit.pulls.get.mockRejectedValue({ status }); + const oldError = await direct.getPullRequest(address).catch(error => error); + await expect(facade.getReview({ review: address })).rejects.toMatchObject({ + code: oldError.code, + message: oldError.message, + }); + } +); +it('AC10 retries a rejected user token through the existing rotation path', async () => { + octokit.pulls.get.mockRejectedValueOnce({ status: 401 }); + const overview = await facade.getReview({ review: address }); + expect(overview.title).toBe('Review me'); + expect(getGitHubUserAccessToken).toHaveBeenCalledWith(ctx.user.id, { + op: 'rotate', + staleAuthorizationId: 'authorization-1', + staleCredentialVersion: 1, + }); +}); +it('AC10 fences restored authorization and account changes before a write', async () => { + const overview = await facade.getReview({ review: address }); + const identity = structuredClone(overview.identity); + if (identity.authorization.kind !== 'githubUser') throw new Error('fixture'); + identity.authorization.authorizationId = 'previous-authorization'; + await expect(facade.getReview({ review: identity })).rejects.toMatchObject({ code: 'CONFLICT' }); + await expect( + facade.getReview({ review: { ...address, accountId: 'other-account' } }) + ).rejects.toMatchObject({ code: 'FORBIDDEN' }); + expect(writes).toEqual([]); +}); +it('AC4–AC5 keeps empty files, checks, discussions and closed reviews readable', async () => { + pull.state = 'closed'; + const overview = await facade.getReview({ review: address }); + expect(overview.state).toBe('closed'); + expect( + await facade.listFiles({ review: overview.identity, revision: overview.revision }) + ).toMatchObject({ items: [], nextCursor: null, authorization: { actor: { id: '99' } } }); + expect( + await facade.listChecks({ review: overview.identity, revision: overview.revision }) + ).toMatchObject({ checks: { status: 'none', checks: [] } }); + expect(await facade.listDiscussions({ review: overview.identity })).toMatchObject({ + items: [], + nextCursor: null, + }); +}); +it('AC5 binds renamed old-side context to the immutable merge base', async () => { + octokit.pulls.listFiles.mockResolvedValue({ + data: [ + { + filename: 'new.ts', + previous_filename: 'old.ts', + status: 'renamed', + additions: 1, + deletions: 1, + patch: '@@ -1 +1 @@\n-old\n+new', + }, + ], + }); + octokit.repos.getContent.mockImplementation(async input => ({ + data: + input.owner === 'Team' && + input.repo === 'Repo' && + input.ref === baseSha && + input.path === 'old.ts' + ? 'old-value\nsecond' + : 'WRONG REVISION', + })); + const overview = await facade.getReview({ review: address }); + const files = await facade.listFiles({ review: overview.identity, revision: overview.revision }); + expect(files.items[0]).toMatchObject({ + oldPath: 'old.ts', + newPath: 'new.ts', + revision: overview.revision, + }); + const context = await facade.getFileContext({ + review: overview.identity, + context: { file: files.items[0], side: 'old', startLine: 1, lineCount: 1 }, + }); + expect(context).toMatchObject({ + lines: ['old-value'], + content: 'available', + canonicalUrl: `https://github.com/Team/Repo/blob/${baseSha}/old.ts`, + }); + await expect( + facade.listFiles({ + review: overview.identity, + revision: { ...overview.revision, headSha: 'c'.repeat(40) }, + }) + ).rejects.toMatchObject({ code: 'CONFLICT' }); +}); +it('AC10 replays a direct GitHub submission through the facade without changing ledger bytes', async () => { + const legacy = { + ...address, + operationKey, + event: 'COMMENT' as const, + body: 'Keep this summary', + commitSha: headSha, + }; + expect(await direct.submitReview(legacy)).toEqual({ + reviewId: 81, + nodeId: 'REVIEW_81', + state: 'COMMENTED', + }); + const savedKey = row.resource_key; + const overview = await facade.getReview({ review: address }); + const input = { + review: overview.identity, + actorId: '99', + revision: overview.revision, + operationKey, + input: { + action: 'submitReview' as const, + body: 'Keep this summary', + choice: 'comment' as const, + }, + }; + expect(await facade.act(input)).toMatchObject({ + result: { status: 'confirmed', reference: { id: '81' } }, + }); + expect(row.resource_key).toBe(savedKey); + expect(writes).toHaveLength(1); + expect(await facade.getOperationStatus(input)).toMatchObject({ result: { status: 'confirmed' } }); + expect(writes).toHaveLength(1); +}); +it('AC6 keeps the legacy Terms gate before any provider effect', async () => { + termsAccepted = false; + const overview = await facade.getReview({ review: address }); + await expect( + facade.act({ + review: overview.identity, + actorId: '99', + revision: overview.revision, + operationKey, + input: { action: 'comment', body: 'Retain my draft' }, + }) + ).rejects.toMatchObject({ code: 'PRECONDITION_FAILED', message: 'terms_required' }); + expect(writes).toEqual([]); + expect(row).toBeNull(); +}); +it('AC6 preserves the ambiguous marker and never repeats a lost submission', async () => { + octokit.pulls.createReview.mockImplementation(async input => { + writes.push(input); + throw { status: 503 }; + }); + const overview = await facade.getReview({ review: address }); + const input = { + review: overview.identity, + actorId: '99', + revision: overview.revision, + operationKey, + input: { action: 'comment' as const, body: 'One effect' }, + }; + await expect(facade.act(input)).rejects.toMatchObject({ + code: 'CONFLICT', + message: "Couldn't confirm — check the PR before retrying.", + }); + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: 'unresolved', retry: 'reconcile' }, + }); + await expect(facade.act(input)).rejects.toMatchObject({ + code: 'CONFLICT', + message: "Couldn't confirm — check the PR before retrying.", + }); + expect(writes).toHaveLength(1); +}); +it('AC6 refuses a changed submitted intent instead of replaying its receipt', async () => { + await direct.submitReview({ + ...address, + operationKey, + event: 'COMMENT', + body: 'Original', + commitSha: headSha, + }); + const overview = await facade.getReview({ review: address }); + await expect( + facade.act({ + review: overview.identity, + actorId: '99', + revision: overview.revision, + operationKey, + input: { action: 'comment', body: 'Changed' }, + }) + ).rejects.toMatchObject({ code: 'CONFLICT', message: 'operation_key_reuse_mismatch' }); + expect(writes).toHaveLength(1); +}); +it('AC6 rejects a stale unadmitted revision and leaves no write or ledger row', async () => { + const overview = await facade.getReview({ review: address }); + await expect( + facade.act({ + review: overview.identity, + actorId: '99', + revision: { ...overview.revision, headSha: 'c'.repeat(40) }, + operationKey, + input: { action: 'comment', body: 'Original selection' }, + }) + ).rejects.toMatchObject({ code: 'CONFLICT' }); + expect(writes).toEqual([]); + expect(row).toBeNull(); +}); +it('AC6 does not admit a new operation from its status query', async () => { + const overview = await createGitHubReviewBridge(ctx).getReview(address); + expect( + await facade.getOperationStatus({ + review: overview.identity, + actorId: '99', + revision: overview.revision, + operationKey, + input: { action: 'comment', body: 'Not sent' }, + }) + ).toMatchObject({ result: { status: 'rejected', code: 'operation_not_admitted' } }); + expect(writes).toEqual([]); + expect(row).toBeNull(); +}); +it.each([ + 'https://github.com/Team/Repo/issues/7', + 'https://github.com/Team/Repo/pull/0', + 'https://github.com/Team/Repo/pull/7/arbitrary', +])('AC10 rejects invalid GitHub entry %s', async url => { + await expect(facade.resolveUrl({ url })).rejects.toBeInstanceOf(TRPCError); + expect(octokit.pulls.get).not.toHaveBeenCalled(); +}); +it('AC4 keeps pending and error commit statuses distinct', async () => { + octokit.paginate.mockImplementation(async method => + method === octokit.repos.listCommitStatusesForRef + ? [ + { context: 'pending-build', state: 'pending', target_url: null, updated_at: null }, + { context: 'failed-build', state: 'error', target_url: null, updated_at: null }, + ] + : [] + ); + expect((await facade.getReview({ review: address })).checks).toMatchObject({ + status: 'reported', + checks: [ + { name: 'pending-build', state: 'pending' }, + { name: 'failed-build', state: 'failed' }, + ], + }); +}); +it('AC10 never confirms a legacy ledger row without a provider receipt', async () => { + await direct.submitReview({ + ...address, + operationKey, + event: 'COMMENT', + body: 'Original', + commitSha: headSha, + }); + row.canonical_result = { futureField: true }; + const overview = await facade.getReview({ review: address }); + expect( + await facade.getOperationStatus({ + review: overview.identity, + actorId: '99', + revision: overview.revision, + operationKey, + input: { action: 'comment', body: 'Original' }, + }) + ).toMatchObject({ result: { status: 'unresolved', retry: 'reconcile' } }); + expect(writes).toHaveLength(1); +}); +it('AC6 rejects fields the selected GitHub action cannot preserve', async () => { + const overview = await facade.getReview({ review: address }); + await expect( + facade.act({ + review: overview.identity, + actorId: '99', + revision: overview.revision, + operationKey, + input: { action: 'comment', body: 'Original', squash: true }, + }) + ).rejects.toMatchObject({ code: 'BAD_REQUEST' }); + expect(writes).toEqual([]); +}); +it('AC7 denies an unavailable merge grant without disabling reads', async () => { + octokit.repos.get.mockResolvedValue({ data: { ...repo, permissions: { push: false } } }); + octokit.pulls.merge.mockImplementation(async input => { + writes.push(input); + return { data: { merged: true, sha: 'merged' } }; + }); + const overview = await facade.getReview({ review: address }); + expect(overview.authorization.capabilities.read.permission).toBe('allowed'); + await expect( + facade.act({ + review: overview.identity, + actorId: '99', + revision: overview.revision, + operationKey, + input: { action: 'merge', method: 'squash' }, + }) + ).rejects.toMatchObject({ code: 'FORBIDDEN' }); + expect(writes).toEqual([]); +}); + +async function actionInput(input: Parameters[0]['input']) { + const overview = await facade.getReview({ review: address }); + return { + review: overview.identity, + revision: overview.revision, + actorId: overview.authorization.actor.id, + operationKey, + input, + }; +} + +function mockGitHubActions(validReceipt = true) { + const original = octokit.request.getMockImplementation(); + if (!original) throw new Error('Missing GraphQL fixture'); + const state = { resolved: false, reacted: false }; + const pageInfo = { hasNextPage: false, endCursor: null }; + const docs = PR_REVIEW_GRAPHQL_DOCUMENTS; + octokit.request.mockImplementation(async (route, request) => { + const variables = request.variables?.input; + const query = request.query; + if (query === docs.REVIEW_THREADS_QUERY) { + return { + data: { + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo, + nodes: [ + { + id: 'THREAD_11', + isResolved: state.resolved, + isOutdated: false, + subjectType: 'LINE', + path: 'new.ts', + line: 4, + diffSide: 'RIGHT', + comments: { + pageInfo, + nodes: [ + { + databaseId: 11, + id: 'COMMENT_11', + body: 'Original comment', + createdAt: '2026-08-30T00:00:00Z', + author: null, + reactionGroups: [ + { + content: 'THUMBS_UP', + reactors: { totalCount: 1 }, + viewerHasReacted: state.reacted, + }, + ], + }, + ], + }, + }, + ], + }, + }, + }, + }, + }, + }; + } + if (query === docs.RESOLVE_THREAD_MUTATION || query === docs.UNRESOLVE_THREAD_MUTATION) { + const resolve = query === docs.RESOLVE_THREAD_MUTATION; + writes.push({ action: resolve ? 'resolve' : 'reopen', ...variables }); + if (validReceipt) state.resolved = resolve; + return { + data: { + data: { + [resolve ? 'resolveReviewThread' : 'unresolveReviewThread']: { + thread: { + id: validReceipt ? 'THREAD_11' : 'OTHER_THREAD', + isResolved: state.resolved, + }, + }, + }, + }, + }; + } + if (query === docs.ADD_REACTION_MUTATION || query === docs.REMOVE_REACTION_MUTATION) { + const add = query === docs.ADD_REACTION_MUTATION; + writes.push({ action: add ? 'react' : 'unreact', ...variables }); + if (validReceipt) state.reacted = add; + return { + data: { + data: { + [add ? 'addReaction' : 'removeReaction']: { + reaction: { content: validReceipt ? 'THUMBS_UP' : 'HEART' }, + }, + }, + }, + }; + } + if (query === docs.ENABLE_AUTO_MERGE_MUTATION || query === docs.DISABLE_AUTO_MERGE_MUTATION) { + const enable = query === docs.ENABLE_AUTO_MERGE_MUTATION; + writes.push({ action: enable ? 'enable' : 'disable', ...variables }); + if (validReceipt) + pull.auto_merge = enable ? { merge_method: variables.mergeMethod.toLowerCase() } : null; + return { + data: { + data: { + [enable ? 'enablePullRequestAutoMerge' : 'disablePullRequestAutoMerge']: { + pullRequest: { id: validReceipt ? 'PR_7' : 'OTHER_PR' }, + }, + }, + }, + }; + } + return original(route, request); + }); + octokit.pulls.createReplyForReviewComment.mockImplementation(async input => { + writes.push(input); + return { data: { id: 83, node_id: 'COMMENT_83' } }; + }); + return state; +} + +it.each([ + ['comment', undefined, 'COMMENT'], + ['approve', undefined, 'APPROVE'], + ['requestChanges', undefined, 'REQUEST_CHANGES'], + ['submitReview', 'comment', 'COMMENT'], + ['submitReview', 'approve', 'APPROVE'], + ['submitReview', 'requestChanges', 'REQUEST_CHANGES'], +] as const)('AC10 maps %s/%s to the unchanged %s ledger intent', async (action, choice, event) => { + await direct.submitReview({ + ...address, + operationKey, + event, + body: 'Summary', + commitSha: headSha, + }); + const savedKey = row.resource_key; + const input = await actionInput({ action, body: 'Summary', ...(choice ? { choice } : {}) }); + expect(await facade.act(input)).toMatchObject({ + result: { status: 'confirmed', reference: { id: '81' } }, + }); + expect(writes).toEqual([ + { owner: 'Team', repo: 'Repo', pull_number: 7, event, commit_id: headSha, body: 'Summary' }, + ]); + expect(row.resource_key).toBe(savedKey); +}); + +it('AC6 maps both batch sides and ranges without changing legacy fingerprint ordering', async () => { + const input = await actionInput({ action: 'submitReview', choice: 'approve', body: 'Summary' }); + input.input.comments = [ + { + itemId: 'old', + body: 'Old range', + position: { + revision: input.revision, + oldPath: 'old.ts', + newPath: 'new.ts', + side: 'old', + line: 4, + startLine: 2, + startSide: 'old', + native: { provider: 'github' }, + }, + }, + { + itemId: 'new', + body: 'New line', + position: { + revision: input.revision, + oldPath: null, + newPath: 'added.ts', + side: 'new', + line: 8, + native: { provider: 'github' }, + }, + }, + ]; + await direct.submitReview({ + ...address, + operationKey, + event: 'APPROVE', + body: 'Summary', + commitSha: headSha, + comments: [ + { path: 'new.ts', line: 4, side: 'LEFT', startLine: 2, startSide: 'LEFT', body: 'Old range' }, + { path: 'added.ts', line: 8, side: 'RIGHT', body: 'New line' }, + ], + }); + expect(await facade.act(input)).toMatchObject({ result: { status: 'confirmed' } }); + expect(writes).toHaveLength(1); +}); + +it.each(['comment', 'inlineComment', 'reply'] as const)( + 'AC6 recovers the recorded %s receipt without another write', + async action => { + mockGitHubActions(); + const input = await actionInput({ action, body: 'One effect' }); + if (action === 'inlineComment') + input.input.position = { + revision: input.revision, + oldPath: 'old.ts', + newPath: 'new.ts', + side: 'old', + line: 4, + startLine: 2, + startSide: 'old', + native: { provider: 'github' }, + }; + if (action === 'reply') + input.input.target = { provider: 'github', kind: 'comment', id: 'COMMENT_11', url: null }; + expect(await facade.act(input)).toMatchObject({ result: { status: 'confirmed' } }); + const receiptId = action === 'comment' ? 81 : action === 'inlineComment' ? 82 : 83; + octokit.pulls.getReviewComment.mockResolvedValue({ + data: { id: receiptId, node_id: `COMMENT_${receiptId}` }, + }); + row.status = 'reconcile_pending'; + row.canonical_result.futureField = 'ignored'; + const recorded = structuredClone(row); + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: 'confirmed', reference: { id: String(receiptId) } }, + }); + expect(row).toEqual(recorded); + expect(writes).toHaveLength(1); + if (action === 'inlineComment') + expect(writes[0]).toMatchObject({ + path: 'new.ts', + line: 4, + side: 'LEFT', + start_line: 2, + start_side: 'LEFT', + commit_id: headSha, + }); + if (action === 'reply') + expect(writes[0]).toMatchObject({ comment_id: 11, body: 'One effect', pull_number: 7 }); + } +); + +it.each([404, 503, 'wrong-id', 'absent-reference'] as const)( + 'AC6 keeps unavailable receipt %s unresolved', + async failure => { + const input = await actionInput({ action: 'comment', body: 'One effect' }); + await facade.act(input); + row.status = 'reconcile_pending'; + if (failure === 'absent-reference') row.canonical_result = { futureField: true }; + else if (failure === 'wrong-id') + octokit.pulls.getReview.mockResolvedValue({ data: { id: 999, node_id: 'OTHER' } }); + else octokit.pulls.getReview.mockRejectedValue({ status: failure }); + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: 'unresolved', retry: 'reconcile' }, + }); + expect(writes).toHaveLength(1); + expect(row.status).toBe('reconcile_pending'); + } +); + +it('AC10 retains the casing of a legacy ledger address after canonical normalization', async () => { + await direct.submitReview({ + owner: 'team', + repo: 'repo', + number: 7, + operationKey, + event: 'COMMENT', + body: 'Summary', + commitSha: headSha, + }); + const savedKey = row.resource_key; + const input = await actionInput({ action: 'comment', body: 'Summary' }); + expect(await facade.act(input)).toMatchObject({ result: { status: 'confirmed' } }); + expect(await facade.getOperationStatus(input)).toMatchObject({ result: { status: 'confirmed' } }); + expect(row.resource_key).toBe(savedKey); + expect(writes).toHaveLength(1); + await expect( + facade.act({ ...input, input: { action: 'comment', body: 'Different' } }) + ).rejects.toMatchObject({ code: 'CONFLICT', message: 'operation_key_reuse_mismatch' }); + expect(writes).toHaveLength(1); +}); + +it.each([ + ['merge', 'keep'], + ['squash', 'delete'], + ['rebase', 'fail'], +] as const)('AC7 maps %s merge and %s deletion through the old path', async (method, deletion) => { + octokit.repos.get.mockResolvedValue({ data: { ...repo, allow_rebase_merge: true } }); + octokit.pulls.merge.mockImplementation(async input => { + writes.push(input); + return { data: { merged: true, sha: 'merged' } }; + }); + octokit.git.deleteRef.mockImplementation(async input => { + writes.push(input); + if (deletion === 'fail') throw new Error('Deletion failed'); + return { data: {} }; + }); + const input = await actionInput({ + action: 'merge', + method, + commitTitle: 'Title', + commitMessage: 'Message', + }); + input.input.deletion = { + effect: deletion === 'keep' ? 'keep' : 'delete', + repositoryKey: repositoryResourceKey(ctx.user.id, input.review), + branch: 'feature', + expectedHeadSha: headSha, + }; + const result = await facade.act(input); + expect(result).toMatchObject({ + result: { status: deletion === 'fail' ? 'partial' : 'confirmed' }, + }); + expect(writes).toEqual([ + { + owner: 'Team', + repo: 'Repo', + pull_number: 7, + merge_method: method, + sha: headSha, + commit_title: 'Title', + commit_message: 'Message', + }, + ...(deletion === 'keep' ? [] : [{ owner: 'Team', repo: 'Repo', ref: 'heads/feature' }]), + ]); + if (deletion === 'fail') + expect(result.result).toMatchObject({ + items: [ + { effect: 'merge', result: { status: 'confirmed' } }, + { effect: 'deleteBranch', result: { status: 'unresolved' } }, + ], + }); + const effects = structuredClone(writes); + expect(await facade.getOperationStatus(input)).toEqual(result); + expect(writes).toEqual(effects); +}); + +it.each([true, false])('AC7 recovers a lost merge response only when merged=%s', async merged => { + const input = await actionInput({ action: 'merge', method: 'squash' }); + octokit.pulls.merge.mockImplementation(async value => { + writes.push(value); + throw { status: 503 }; + }); + await expect(facade.act(input)).rejects.toMatchObject({ code: 'CONFLICT' }); + pull.state = 'closed'; + pull.merged = merged; + const pending = structuredClone(row); + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: merged ? 'confirmed' : 'unresolved' }, + }); + expect(await facade.act(input)).toMatchObject({ + result: { status: merged ? 'confirmed' : 'unresolved' }, + }); + expect(row).toEqual(pending); + expect(writes).toHaveLength(1); +}); + +describe.each([ + ['resolveThread', { action: 'resolve', threadId: 'THREAD_11' }], + ['reopenThread', { action: 'reopen', threadId: 'THREAD_11' }], + ['addReaction', { action: 'react', subjectId: 'COMMENT_11', content: 'THUMBS_UP' }], + ['removeReaction', { action: 'unreact', subjectId: 'COMMENT_11', content: 'THUMBS_UP' }], + ['enableAutoMerge', { action: 'enable', pullRequestId: 'PR_7', mergeMethod: 'SQUASH' }], + ['disableAutoMerge', { action: 'disable', pullRequestId: 'PR_7' }], +] as const)('AC6–AC7 %s mapping and recovery', (action, expected) => { + it.each([true, false])('confirms only a matching receipt: %s', async validReceipt => { + const state = mockGitHubActions(validReceipt); + state.resolved = action === 'reopenThread'; + state.reacted = action === 'removeReaction'; + if (action === 'disableAutoMerge') pull.auto_merge = { merge_method: 'squash' }; + const input = await actionInput({ action }); + if (action === 'resolveThread' || action === 'reopenThread') + input.input.target = { provider: 'github', kind: 'thread', id: 'THREAD_11', url: null }; + if (action === 'addReaction' || action === 'removeReaction') { + input.input.target = { provider: 'github', kind: 'comment', id: 'COMMENT_11', url: null }; + input.input.reaction = 'THUMBS_UP'; + } + if (action === 'enableAutoMerge') input.input.method = 'squash'; + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: 'unresolved' }, + }); + expect(writes).toEqual([]); + expect(await facade.act(input)).toMatchObject({ + result: { status: validReceipt ? 'confirmed' : 'unresolved' }, + }); + expect(writes).toEqual([expected]); + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: validReceipt ? 'confirmed' : 'unresolved' }, + }); + expect(writes).toEqual([expected]); + }); +}); + +it.each(['both', 'source-only', 'target-only', 'unrelated'] as const)( + 'AC7 confirms branch update only with both original ancestors: %s', + async ancestry => { + const input = await actionInput({ action: 'updateBranch' }); + octokit.pulls.updateBranch.mockImplementation(async value => { + writes.push(value); + return { data: { message: 'Updating' } }; + }); + expect(await facade.act(input)).toMatchObject({ + result: { status: 'accepted', retry: 'reconcile' }, + }); + const updatedHead = 'c'.repeat(40); + pull.head.sha = updatedHead; + octokit.repos.compareCommits.mockImplementation(async value => ({ + data: { + merge_base_commit: { + sha: + value.head === updatedHead && + ((value.base === headSha && ['both', 'source-only'].includes(ancestry)) || + (value.base === baseSha && ['both', 'target-only'].includes(ancestry))) + ? value.base + : 'd'.repeat(40), + }, + }, + })); + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: ancestry === 'both' ? 'confirmed' : 'unresolved' }, + }); + expect(writes).toEqual([ + { owner: 'Team', repo: 'Repo', pull_number: 7, expected_head_sha: headSha }, + ]); + } +); + +it('AC7 rejects the implicit auto-merge method when the repository disables it', async () => { + mockGitHubActions(); + octokit.repos.get.mockResolvedValue({ data: { ...repo, allow_merge_commit: false } }); + const input = await actionInput({ action: 'enableAutoMerge' }); + await expect(facade.act(input)).rejects.toMatchObject({ + code: 'BAD_REQUEST', + message: 'merge_method_not_available', + }); + expect(writes).toEqual([]); +}); + +it('AC6 reports a rejected ledger operation as terminal rather than unknown', async () => { + const input = await actionInput({ action: 'comment', body: 'No permission' }); + octokit.pulls.createReview.mockImplementation(async value => { + writes.push(value); + throw { status: 403 }; + }); + await expect(facade.act(input)).rejects.toMatchObject({ code: 'FORBIDDEN' }); + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: 'rejected', retry: 'never' }, + }); + expect(writes).toHaveLength(1); +}); + +it.each(['repositoryKey', 'branch', 'expectedHeadSha'] as const)( + 'AC7 rejects changed deletion %s without a merge', + async field => { + const input = await actionInput({ action: 'merge', method: 'squash' }); + input.input.deletion = { + effect: 'delete', + repositoryKey: repositoryResourceKey(ctx.user.id, input.review), + branch: 'feature', + expectedHeadSha: headSha, + }; + input.input.deletion[field] = 'other'; + await expect(facade.act(input)).rejects.toMatchObject({ code: 'CONFLICT' }); + expect(writes).toEqual([]); + expect(row).toBeNull(); + } +); + +it.each(['unapprove', 'removeChangeRequest', 'deleteBranch'] as const)( + 'AC10 refuses %s without inventing a legacy procedure', + async action => { + const input = await actionInput({ action }); + await expect(facade.act(input)).rejects.toMatchObject({ code: 'BAD_REQUEST' }); + expect(writes).toEqual([]); + expect(row).toBeNull(); + } +); + +it.each(['PENDING', 'COMMENTED', undefined])( + 'AC6 never confirms approval from review state %s', + async state => { + const input = await actionInput({ action: 'approve' }); + octokit.pulls.createReview.mockImplementation(async value => { + writes.push(value); + return { data: { id: 81, node_id: 'REVIEW_81', state } }; + }); + expect(await facade.act(input)).toMatchObject({ + result: { status: 'unresolved', retry: 'reconcile' }, + }); + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: 'unresolved', retry: 'reconcile' }, + }); + expect(writes).toHaveLength(1); + } +); + +it.each([null, 'unknown'])( + 'AC10 cannot recover a branch update from unavailable target revision %s', + async targetHeadSha => { + const input = await actionInput({ action: 'updateBranch' }); + input.revision.targetHeadSha = targetHeadSha; + pull.head.sha = 'c'.repeat(40); + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: 'unresolved', retry: 'reconcile' }, + }); + expect(writes).toEqual([]); + expect(row).toBeNull(); + } +); + +it('AC7 fences a head change during branch-update status reads', async () => { + const input = await actionInput({ action: 'updateBranch' }); + pull.head.sha = 'c'.repeat(40); + octokit.repos.compareCommits.mockImplementation(async value => { + if (value.base === headSha) pull.head.sha = 'd'.repeat(40); + return { data: { merge_base_commit: { sha: value.base } } }; + }); + await expect(facade.getOperationStatus(input)).rejects.toMatchObject({ code: 'CONFLICT' }); + expect(writes).toEqual([]); +}); + +it('AC7 does not confirm an uncertain merge against a different head', async () => { + const input = await actionInput({ action: 'merge', method: 'squash' }); + octokit.pulls.merge.mockImplementation(async value => { + writes.push(value); + throw { status: 503 }; + }); + await expect(facade.act(input)).rejects.toMatchObject({ code: 'CONFLICT' }); + pull.state = 'closed'; + pull.merged = true; + pull.head.sha = 'c'.repeat(40); + const pending = structuredClone(row); + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: 'unresolved', retry: 'reconcile' }, + }); + expect(await facade.act(input)).toMatchObject({ + result: { status: 'unresolved', retry: 'reconcile' }, + }); + expect(row).toEqual(pending); + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: 'unresolved', retry: 'reconcile' }, + }); + expect(writes).toHaveLength(1); +}); + +it.each(['head', 'target', 'merge-base'] as const)( + 'AC5 rejects %s drift during file retrieval without relabeling the selection', + async changed => { + const overview = await facade.getReview({ review: address }); + const selected = structuredClone(overview.revision); + octokit.pulls.listFiles.mockImplementation(async () => { + if (changed === 'head') pull.head.sha = 'c'.repeat(40); + if (changed === 'target') pull.base.sha = 'c'.repeat(40); + if (changed === 'merge-base') + octokit.repos.compareCommits.mockResolvedValue({ + data: { merge_base_commit: { sha: 'c'.repeat(40) } }, + }); + return { + data: [ + { + filename: 'file.ts', + status: 'modified', + additions: 1, + deletions: 1, + patch: '@@ -1 +1 @@\n-old\n+new', + }, + ], + }; + }); + await expect( + facade.listFiles({ review: overview.identity, revision: selected }) + ).rejects.toMatchObject({ + code: 'CONFLICT', + message: 'review_identity_or_revision_changed', + }); + expect(overview.revision).toEqual(selected); + expect(writes).toEqual([]); + } +); + +it('AC4 preserves check identity after insertion, reordering, and state changes', async () => { + let runs = [ + { + id: 11, + name: 'build', + status: 'in_progress', + conclusion: null, + app: { name: 'CI' }, + details_url: 'https://github.com/Team/Repo/actions/runs/11', + }, + { + id: 22, + name: 'build', + status: 'completed', + conclusion: 'success', + app: { name: 'CI' }, + details_url: 'https://github.com/Team/Repo/actions/runs/22', + }, + { + id: 33, + name: 'lint', + status: 'completed', + conclusion: 'success', + app: { name: 'Linter' }, + details_url: null, + }, + ]; + let statuses = [{ context: 'legacy', state: 'pending', target_url: null, updated_at: null }]; + octokit.paginate.mockImplementation(async method => + method === octokit.checks.listForRef ? runs : statuses + ); + const before = (await facade.getReview({ review: address })).checks.checks; + runs = [ + { + ...runs[0], + id: 44, + name: 'new-check', + details_url: 'https://github.com/Team/Repo/actions/runs/44', + }, + runs[2], + runs[1], + { ...runs[0], status: 'completed', conclusion: 'success' }, + ]; + statuses = [{ ...statuses[0], state: 'success' }]; + const after = (await facade.getReview({ review: address })).checks.checks; + for (const check of before) + expect(after.find(current => current.id === check.id)).toMatchObject({ + name: check.name, + detailsUrl: check.detailsUrl, + state: 'passed', + }); + expect(new Set(after.map(check => check.id)).size).toBe(5); +}); + +it.each([null, 'https://example.com/shared-check'])( + 'AC4 distinguishes checks with repeated metadata and URL %s', + async detailsUrl => { + const run = { name: 'build', status: 'completed', app: null, details_url: detailsUrl }; + let runs = [ + { ...run, id: 11, conclusion: 'failure' }, + { ...run, id: 22, conclusion: 'success' }, + ]; + let statuses = [ + { + context: 'build', + state: 'pending', + target_url: detailsUrl, + updated_at: '2026-08-30T00:00:00Z', + }, + { + context: 'build', + state: 'success', + target_url: detailsUrl, + updated_at: '2026-08-31T00:00:00Z', + }, + ]; + octokit.paginate.mockImplementation(async method => + method === octokit.checks.listForRef ? runs : statuses + ); + const before = (await facade.getReview({ review: address })).checks.checks; + expect(new Set(before.map(check => check.id)).size).toBe(3); + expect(before.map(check => check.state)).toEqual(['failed', 'passed', 'passed']); + runs = [runs[1], { ...runs[0], conclusion: 'success' }]; + statuses = [...statuses].reverse(); + const after = (await facade.getReview({ review: address })).checks.checks; + expect(after.map(check => check.id)).toEqual([before[1].id, before[0].id, before[2].id]); + expect(after.every(check => check.state === 'passed')).toBe(true); + const legacy = await direct.listChecks({ + owner: address.owner, + repo: address.repo, + ref: headSha, + }); + expect(legacy.checkRuns).toHaveLength(3); + expect(legacy.checkRuns.every(check => !('id' in check))).toBe(true); + } +); + +it.each(['other-head', 'open', 'same-head'] as const)( + 'AC7 keeps overlapping same-key merge recovery read-only for %s', + async outcome => { + const input = await actionInput({ action: 'merge', method: 'squash' }); + const entered = Promise.withResolvers(); + const release = Promise.withResolvers(); + gates.admission.mockImplementationOnce(async () => { + entered.resolve(); + await release.promise; + }); + const delayed = facade.act(input).then( + value => ({ value }), + error => ({ error }) + ); + await entered.promise; + octokit.pulls.merge.mockImplementationOnce(async value => { + writes.push(value); + throw { status: 503 }; + }); + await expect(facade.act(input)).rejects.toMatchObject({ code: 'CONFLICT' }); + expect(row.status).toBe('reconcile_pending'); + if (outcome !== 'open') { + pull.state = 'closed'; + pull.merged = true; + } + if (outcome === 'other-head') pull.head.sha = 'c'.repeat(40); + octokit.pulls.merge.mockImplementation(async value => { + writes.push(value); + return { data: { merged: true, sha: 'merged' } }; + }); + release.resolve(); + const result = await delayed; + if (outcome === 'same-head') { + expect(result).toMatchObject({ value: { result: { status: 'confirmed' } } }); + expect(row.status).toBe('completed'); + } else { + expect(result).toMatchObject({ + error: { code: 'CONFLICT', message: "Couldn't confirm — check the PR before retrying." }, + }); + expect(row.status).toBe('reconcile_pending'); + expect(await facade.getOperationStatus(input)).toMatchObject({ + result: { status: 'unresolved' }, + }); + } + expect(writes).toHaveLength(1); + if (outcome === 'open') { + // Unscoped legacy callers retain their existing same-head takeover behavior. + await expect( + direct.mergePullRequest({ + ...address, + operationKey, + method: 'squash', + deleteBranch: false, + expectedHeadSha: headSha, + }) + ).resolves.toMatchObject({ merged: true }); + expect(writes).toHaveLength(2); + } + } +); + +async function replacementCredential(authorizationId: string, actorId: number) { + const initial = await getGitHubUserAccessToken(ctx.user.id, { op: 'fetch' }); + if (initial.status !== 'connected') throw new Error('Missing credential fixture'); + const replacement = { + ...initial, + credential: { + ...initial.credential, + token: 'replacement-fixture', + authorizationId, + credentialVersion: 2, + }, + }; + const replacementOctokit = { + ...octokit, + users: { getAuthenticated: async () => ({ data: { id: actorId, login: 'reviewer' } }) }, + pulls: { + ...octokit.pulls, + createReview: async (input: unknown) => { + writes.push({ input, actorId }); + return { data: { id: 81, node_id: 'REVIEW_81', state: 'COMMENTED' } }; + }, + }, + }; + jest + .mocked(createGitHubPrReviewOctokit) + .mockImplementation( + token => (token === replacement.credential.token ? replacementOctokit : octokit) as any + ); + return { initial, replacement }; +} + +it.each([ + ['terms', 'authorization'], + ['terms', 'actor'], + ['terms', 'same-identity'], + ['admission', 'authorization'], + ['admission', 'actor'], + ['admission', 'same-identity'], +] as const)('AC6 fences %s await against %s replacement', async (stage, change) => { + const input = await actionInput({ action: 'comment', body: 'Admitted actor only' }); + const { replacement } = await replacementCredential( + change === 'authorization' ? 'authorization-2' : 'authorization-1', + change === 'actor' ? 100 : 99 + ); + const entered = Promise.withResolvers(); + const release = Promise.withResolvers(); + gates[stage].mockImplementationOnce(async () => { + entered.resolve(); + await release.promise; + }); + const result = facade.act(input); + await entered.promise; + jest.mocked(getGitHubUserAccessToken).mockResolvedValue(replacement); + release.resolve(); + if (change === 'same-identity') { + expect(await result).toMatchObject({ + result: { status: 'confirmed', reference: { id: '81' } }, + }); + expect(writes).toEqual([ + { + input: { + owner: 'Team', + repo: 'Repo', + pull_number: 7, + event: 'COMMENT', + commit_id: headSha, + body: 'Admitted actor only', + }, + actorId: 99, + }, + ]); + } else { + await expect(result).rejects.toMatchObject({ + code: 'CONFLICT', + message: 'review_identity_or_revision_changed', + }); + expect(writes).toEqual([]); + expect(row.status).toBe('failed'); + } +}); + +it.each(['authorization', 'actor', 'same-identity'] as const)( + 'AC6 fences the rotated credential for %s', + async change => { + const input = await actionInput({ action: 'comment', body: 'Rotate safely' }); + const { initial, replacement } = await replacementCredential( + change === 'authorization' ? 'authorization-2' : 'authorization-1', + change === 'actor' ? 100 : 99 + ); + jest + .mocked(getGitHubUserAccessToken) + .mockImplementation(async (_userId, op) => (op.op === 'rotate' ? replacement : initial)); + octokit.pulls.createReview.mockRejectedValue({ status: 401 }); + const result = facade.act(input); + if (change === 'same-identity') { + expect(await result).toMatchObject({ + result: { status: 'confirmed', reference: { id: '81' } }, + }); + expect(writes).toEqual([ + { + input: { + owner: 'Team', + repo: 'Repo', + pull_number: 7, + event: 'COMMENT', + commit_id: headSha, + body: 'Rotate safely', + }, + actorId: 99, + }, + ]); + } else { + await expect(result).rejects.toMatchObject({ + code: 'CONFLICT', + message: 'review_identity_or_revision_changed', + }); + expect(writes).toEqual([]); + expect(row.status).toBe('failed'); + } + } +); diff --git a/apps/web/src/lib/provider-review/github-bridge.ts b/apps/web/src/lib/provider-review/github-bridge.ts new file mode 100644 index 0000000000..49e877d82c --- /dev/null +++ b/apps/web/src/lib/provider-review/github-bridge.ts @@ -0,0 +1,1077 @@ +import 'server-only'; + +import { z } from 'zod'; +import { and, eq } from 'drizzle-orm'; +import { TRPCError } from '@trpc/server'; +import { operation_ledgers } from '@kilocode/db/schema'; +import { + normalizeLegacyGitHubReviewRepository, + repositoryResourceKey, + type GitHubUserAuthorization, +} from '@kilocode/app-shared/code-review/repository-identity'; +import { + ReviewActionSchema, + ReviewCapabilitiesSchema, + ReviewRevisionSchema, + reviewActionAvailability, + parseReviewCursor, + reviewPageKey, + reviewResourceKey, + type ReviewActor, + type ReviewCapability, + type ReviewCursor, + type ReviewFile, + type ReviewFileContext, + type ReviewInbox, + type ReviewIntent, + type ReviewMutationResult, + type ReviewOverview, + type ReviewPage, + type ReviewPosition, + type ReviewThread, +} from '@kilocode/app-shared/provider-review'; +import type { TRPCContext } from '@/lib/trpc/init'; +import { db } from '@/lib/drizzle'; +// Old GitHub procedures, DTOs, and ledger bytes stay behind this bridge until old +// clients/records disappear and the 30-day ledger window expires. +import { + fetchGitHubReviewChecks, + githubPrReviewRouter, + prLedgerResourceKey, +} from '@/routers/github-pr-review-router'; +import { buildChecksResult } from '@/lib/github-pr-review/mappers'; +import type { PrLedgerIntent } from '@kilocode/app-shared/pr-review'; +import type { GitHubPrReviewOverview } from '@/lib/github-pr-review/dtos'; +import { withGitHubReviewIdentity, withGitHubUserTokenRetry } from '@/lib/github-pr-review/retry'; +import { getGitHubUserAccessToken } from '@/lib/integrations/platforms/github/user-token-client'; +import { + AutoMergeMethodSchema, + CommentPositionSchema, + MergeMethodSchema, + ReactionContentSchema, +} from '@/lib/github-pr-review/mutations'; + +export const GitHubReviewAddressSchema = z.object({ + owner: z.string().regex(/^[A-Za-z0-9_.-]+$/), + repo: z.string().regex(/^[A-Za-z0-9_.-]+$/), + number: z.number().int().positive().safe(), +}); +type Address = z.infer; +const sha = z.string().regex(/^(?:[a-f0-9]{40}|[a-f0-9]{64})$/i); +const repositorySchema = z.object({ + id: z.number().int().positive().safe(), + full_name: z.string().regex(/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/), + default_branch: z.string().nullable().optional(), +}); +const pullSchema = z.object({ + node_id: z.string().min(1), + number: z.number().int().positive(), + head: z.object({ sha, repo: repositorySchema.nullish() }), + base: z.object({ sha: sha.optional(), repo: repositorySchema }), +}); +const reconnect = () => + new TRPCError({ + code: 'PRECONDITION_FAILED', + message: 'GitHub connection is no longer valid — reconnect', + }); +const conflict = () => + new TRPCError({ code: 'CONFLICT', message: 'review_identity_or_revision_changed' }); + +function author(value: GitHubPrReviewOverview['author']): ReviewActor | null { + // Old author DTOs contain only login/avatar. Keep the qualified login fallback until + // old clients/records disappear and the 30-day ledger window expires. + return value + ? { + provider: 'github', + instanceUrl: 'https://github.com', + id: `login:${value.login.toLowerCase()}`, + login: value.login, + displayName: null, + avatarUrl: z.url({ protocol: /^https$/ }).safeParse(value.avatarUrl).success + ? value.avatarUrl + : null, + } + : null; +} +function address(overview: ReviewOverview): Address { + const [owner, repo] = overview.identity.repository.fullName.split('/'); + return GitHubReviewAddressSchema.parse({ owner, repo, number: Number(overview.identity.number) }); +} +function sameRevision(expected: ReviewIntent['revision'], actual: ReviewIntent['revision']) { + if ( + JSON.stringify(ReviewRevisionSchema.parse(expected)) !== + JSON.stringify(ReviewRevisionSchema.parse(actual)) + ) + throw conflict(); +} +function capabilities(value: GitHubPrReviewOverview) { + const base: ReviewCapability = { + support: 'supported', + version: 'available', + license: 'available', + permission: 'allowed', + restrictions: [], + explanation: '', + recovery: 'none', + evidenceUrl: null, + expectedHeadProtection: 'none', + }; + return ReviewCapabilitiesSchema.parse( + Object.fromEntries( + ReviewActionSchema.options.map(action => { + const capability = { ...base, restrictions: [] as string[] }; + if (['unapprove', 'removeChangeRequest'].includes(action)) { + capability.support = 'unknown'; + capability.explanation = 'review_state_action_not_exposed'; + capability.recovery = 'openProvider'; + } + if ( + ['approve', 'requestChanges', 'submitReview'].includes(action) && + value.state !== 'open' + ) + capability.restrictions.push(value.state); + if ( + ['merge', 'deleteBranch', 'updateBranch', 'enableAutoMerge', 'disableAutoMerge'].includes( + action + ) + ) { + capability.permission = value.repo.viewerCanPush ? 'allowed' : 'forbidden'; + if (!value.repo.viewerCanPush) { + capability.explanation = 'permission_required'; + capability.recovery = 'openProvider'; + } + if (value.state !== 'open') capability.restrictions.push(value.state); + } + if (action === 'merge' || action === 'enableAutoMerge') { + if (value.draft) capability.restrictions.push('draft'); + if (value.mergeable === false) capability.restrictions.push('conflict'); + if (value.mergeable === null) capability.restrictions.push('mergeability_unknown'); + if ( + action === 'merge' && + !['clean', 'unstable', 'has_hooks'].includes(value.mergeableState ?? '') + ) + capability.restrictions.push(value.mergeableState ?? 'mergeability_unknown'); + } + if (action === 'deleteBranch' && value.isCrossRepo) + capability.restrictions.push('cross_repository_source'); + if (action === 'updateBranch' && !value.repo.allowUpdateBranch) + capability.restrictions.push('branch_update_not_allowed'); + if (action === 'enableAutoMerge' && !value.repo.allowAutoMerge) + capability.restrictions.push('auto_merge_not_allowed'); + if (action === 'disableAutoMerge' && !value.autoMerge) + capability.restrictions.push('auto_merge_not_enabled'); + if (action === 'inlineComment') capability.expectedHeadProtection = 'revisionAttachment'; + if (action === 'merge') capability.expectedHeadProtection = 'atomicSource'; + return [action, capability]; + }) + ) + ); +} + +export function createGitHubReviewBridge(ctx: TRPCContext) { + const caller = githubPrReviewRouter.createCaller(ctx); + const call = (work: Parameters>[0]['call']) => + withGitHubUserTokenRetry({ kiloUserId: ctx.user.id, call: work }); + + async function credential() { + const result = await getGitHubUserAccessToken(ctx.user.id, { op: 'fetch' }); + if (result.status !== 'connected') throw reconnect(); + return result.credential; + } + async function authorization(expected?: GitHubUserAuthorization) { + if (expected && expected.accountId !== ctx.user.id) throw conflict(); + const first = await credential(); + if (expected && expected.authorizationId !== first.authorizationId) throw conflict(); + const user = z + .object({ + id: z.number().int().positive(), + login: z.string().min(1), + name: z.string().nullish(), + avatar_url: z.url().nullish(), + }) + .parse(await call(async octokit => (await octokit.users.getAuthenticated()).data)); + if ((await credential()).authorizationId !== first.authorizationId) throw conflict(); + return { + authorization: { + kind: 'githubUser' as const, + accountId: ctx.user.id, + authorizationId: first.authorizationId, + }, + actor: { + provider: 'github' as const, + instanceUrl: 'https://github.com', + id: String(user.id), + login: user.login, + displayName: user.name ?? null, + avatarUrl: z.url({ protocol: /^https$/ }).safeParse(user.avatar_url).success + ? (user.avatar_url ?? null) + : null, + }, + }; + } + async function getAuthorization() { + const result = await getGitHubUserAccessToken(ctx.user.id, { op: 'fetch' }); + if (result.status === 'disconnected') + return { + status: 'not_connected' as const, + reason: result.reason, + authorization: null, + actor: null, + }; + if (result.status !== 'connected') + throw new TRPCError({ code: 'SERVICE_UNAVAILABLE', message: 'temporarily_unavailable' }); + return { status: 'connected' as const, reason: null, ...(await authorization()) }; + } + async function metadata(input: Address, auth: Awaited>) { + const value = await call(async octokit => { + const repo = repositorySchema.parse((await octokit.repos.get(input)).data); + const pull = pullSchema.parse( + (await octokit.pulls.get({ ...input, pull_number: input.number })).data + ); + return { repo, pull }; + }); + if ( + value.repo.full_name.toLowerCase() !== `${input.owner}/${input.repo}`.toLowerCase() || + value.pull.number !== input.number || + value.pull.base.repo.id !== value.repo.id + ) + throw conflict(); + const normalized = normalizeLegacyGitHubReviewRepository({ + accountId: ctx.user.id, + repository: { + fullName: value.repo.full_name, + repositoryId: String(value.repo.id), + defaultBranch: value.repo.default_branch, + }, + authorization: auth.authorization, + }); + if (normalized.kind !== 'resolved') throw conflict(); + return { + ...value, + identity: { + ...normalized.reference, + number: String(input.number), + reviewId: value.pull.node_id, + canonicalUrl: `https://github.com/${value.repo.full_name}/pull/${input.number}`, + }, + }; + } + async function getReview( + input: Address, + expected?: GitHubUserAuthorization + ): Promise { + input = GitHubReviewAddressSchema.parse(input); + const auth = await authorization(expected); + const value = await caller.getPullRequest(input); + const meta = await metadata(input, auth); + if (value.headSha !== meta.pull.head.sha || value.prNodeId !== meta.identity.reviewId) + throw conflict(); + // Old provider payloads can omit base/default metadata. Preserve explicit unavailable + // values until old clients/records disappear and the 30-day ledger window expires. + const baseSha = meta.pull.base.sha + ? sha.parse( + await call( + async octokit => + ( + await octokit.repos.compareCommits({ + ...input, + base: meta.pull.base.sha ?? '', + head: value.headSha, + }) + ).data.merge_base_commit.sha + ) + ) + : null; + const source = meta.pull.head.repo; + const checks = await getChecksFor(input, value.headSha); + await authorization(auth.authorization); + return { + identity: meta.identity, + title: value.title, + bodyMarkdown: value.bodyMarkdown, + author: author(value.author), + state: value.state, + draft: value.draft, + revision: { + headSha: value.headSha, + baseSha, + startSha: null, + targetHeadSha: meta.pull.base.sha ?? null, + }, + source: { + repository: source + ? { + provider: 'github', + instanceUrl: 'https://github.com', + repositoryId: String(source.id), + fullName: source.full_name, + defaultBranch: source.default_branch ?? null, + } + : null, + branch: value.headRef, + }, + target: { repository: meta.identity.repository, branch: value.baseRef }, + authorization: { + actor: auth.actor, + credentialKind: 'githubUser', + capabilities: capabilities(value), + writeLimits: { requestMaxBytes: Number.MAX_SAFE_INTEGER, bodyMaxBytes: null }, + }, + providerState: { provider: 'github', decision: value.reviewDecision }, + checks, + counts: { + commits: value.counts.commits, + files: value.counts.changedFiles, + additions: value.counts.additions, + deletions: value.counts.deletions, + }, + merge: { + methods: (['merge', 'squash', 'rebase'] as const) + .filter( + method => + ({ + merge: value.repo.allowMergeCommit, + squash: value.repo.allowSquashMerge, + rebase: value.repo.allowRebaseMerge, + })[method] + ) + .map(id => ({ id, label: id })), + squash: null, + autoMerge: value.autoMerge, + task: null, + }, + }; + } + async function getChecksFor(input: Address, ref: string): Promise { + const raw = await fetchGitHubReviewChecks(ctx.user.id, { + owner: input.owner, + repo: input.repo, + ref, + }); + const { checkRuns } = buildChecksResult(raw); + if (!checkRuns.length) return { status: 'none', checks: [] }; + // The mapper retains run order and one status per context in first-seen order. + // A status context keeps its identity when a newer status replaces its value. + const ids = [ + ...raw.checkRuns.map( + check => `check-run:${z.number().int().positive().safe().parse(check.id)}` + ), + ...new Set(raw.commitStatuses.map(status => `status:${status.context}`)), + ]; + return { + status: 'reported', + checks: checkRuns.map((check, index) => ({ + id: ids[index], + name: check.name, + required: null, + detailsUrl: check.detailsUrl, + state: + check.status === 'queued' || check.status === 'pending' + ? 'pending' + : check.status === 'in_progress' + ? 'running' + : check.conclusion === 'success' + ? 'passed' + : check.conclusion === 'skipped' || check.conclusion === 'neutral' + ? 'skipped' + : check.conclusion === 'cancelled' + ? 'cancelled' + : [ + 'failure', + 'error', + 'timed_out', + 'action_required', + 'startup_failure', + ].includes(check.conclusion ?? '') + ? 'failed' + : 'unknown', + })), + }; + } + function page( + overview: ReviewOverview, + surface: 'files' | 'threads', + cursor?: ReviewCursor | null + ) { + const scope = { + resourceKey: reviewResourceKey(ctx.user.id, overview.identity), + surface, + queryKey: 'all', + revision: overview.revision, + }; + return { + token: cursor ? parseReviewCursor(cursor, scope).token : undefined, + next: (token: string | null): ReviewCursor | null => + token === null ? null : { scopeKey: reviewPageKey(scope), token }, + }; + } + async function unchanged(overview: ReviewOverview) { + if (overview.identity.authorization.kind !== 'githubUser') throw conflict(); + const current = await getReview(address(overview), overview.identity.authorization); + if ( + reviewResourceKey(ctx.user.id, current.identity) !== + reviewResourceKey(ctx.user.id, overview.identity) + ) + throw conflict(); + sameRevision(overview.revision, current.revision); + } + async function listInbox(cursor?: ReviewCursor | null): Promise { + const auth = await authorization(); + const scope = { + resourceKey: JSON.stringify([ctx.user.id, auth.authorization, auth.actor.id]), + surface: 'inbox' as const, + queryKey: 'review-requested', + revision: null, + }; + const result = await caller.listInbox({ + cursor: cursor ? parseReviewCursor(cursor, scope).token : undefined, + }); + const items = []; + for (const item of result.items) + items.push({ + identity: (await metadata(item, auth)).identity, + title: item.title, + author: author(item.author), + state: 'open' as const, + draft: item.isDraft, + updatedAt: item.updatedAt, + }); + await authorization(auth.authorization); + return { + items, + nextCursor: result.nextCursor + ? { scopeKey: reviewPageKey(scope), token: result.nextCursor } + : null, + scope: { kind: 'actor', actor: auth.actor }, + }; + } + async function listFiles( + overview: ReviewOverview, + cursor?: ReviewCursor | null + ): Promise> { + const paging = page(overview, 'files', cursor); + const result = await caller.listFiles({ + ...address(overview), + cursor: paging.token ? z.number().int().positive().parse(Number(paging.token)) : undefined, + }); + await unchanged(overview); + return { + items: result.files.map(file => ({ + id: file.path, + oldPath: file.status === 'added' ? null : (file.previousPath ?? file.path), + newPath: file.status === 'removed' ? null : file.path, + revision: overview.revision, + status: + file.status === 'removed' + ? 'deleted' + : file.status === 'added' || file.status === 'renamed' || file.status === 'copied' + ? file.status + : 'modified', + patch: file.patch, + content: file.patchMissing ? 'unavailable' : 'available', + additions: file.additions, + deletions: file.deletions, + canonicalUrl: `${overview.identity.canonicalUrl}/files`, + })), + nextCursor: paging.next(result.nextCursor === null ? null : String(result.nextCursor)), + }; + } + async function getFileContext( + overview: ReviewOverview, + input: { + file: Pick; + side: 'old' | 'new'; + startLine: number; + lineCount: number; + } + ): Promise { + sameRevision(input.file.revision, overview.revision); + let cursor: ReviewCursor | null = null; + let found = false; + do { + const files = await listFiles(overview, cursor); + found = files.items.some( + file => file.oldPath === input.file.oldPath && file.newPath === input.file.newPath + ); + cursor = files.nextCursor; + } while (!found && cursor); + if (!found) throw conflict(); + const path = input.side === 'old' ? input.file.oldPath : input.file.newPath; + const ref = input.side === 'old' ? overview.revision.baseSha : overview.revision.headSha; + const repository = + input.side === 'old' ? overview.target.repository : overview.source.repository; + if (!path) throw new TRPCError({ code: 'BAD_REQUEST', message: 'file_side_unavailable' }); + const result: ReviewFileContext = { + revision: input.file.revision, + path, + side: input.side, + startLine: input.startLine, + lines: [], + totalLines: null, + content: 'unavailable', + canonicalUrl: overview.identity.canonicalUrl, + }; + if (!ref || !repository) return result; + result.canonicalUrl = `https://github.com/${repository.fullName}/blob/${ref}/${path.split('/').map(encodeURIComponent).join('/')}`; + const [owner, repo] = repository.fullName.split('/'); + try { + const lines = await caller.getFileLines({ + owner, + repo, + path, + ref, + startLine: input.startLine, + endLine: input.startLine + input.lineCount - 1, + }); + await unchanged(overview); + return lines.lines.some(line => line.includes('\0')) + ? { ...result, content: 'binary' } + : { ...result, ...lines, content: 'available' }; + } catch (error) { + if (error instanceof TRPCError && error.code === 'NOT_FOUND') return result; + throw error; + } + } + async function listDiscussions( + overview: ReviewOverview, + cursor?: ReviewCursor | null + ): Promise> { + const paging = page(overview, 'threads', cursor); + const result = await caller.listReviewThreads({ ...address(overview), cursor: paging.token }); + await unchanged(overview); + const comment = (value: (typeof result.conversation)[number]) => ({ + id: String(value.commentId), + reference: { + provider: 'github' as const, + kind: 'comment' as const, + id: value.nodeId, + url: `${overview.identity.canonicalUrl}#discussion_r${value.commentId}`, + }, + author: author(value.author), + bodyMarkdown: value.bodyMarkdown, + createdAt: value.createdAt, + reactions: value.reactions.map(reaction => ({ ...reaction, id: reaction.content })), + }); + // Old threads omit immutable original revisions and rename paths. Do not attach them + // to the current head; retain null positions until old payloads/records and the 30-day window expire. + const threads: ReviewThread[] = result.threads.map(thread => ({ + id: thread.threadId, + reference: { + provider: 'github', + kind: 'thread', + id: thread.threadId, + url: overview.identity.canonicalUrl, + }, + subjectType: thread.subjectType === 'LINE' ? 'line' : 'file', + file: null, + position: null, + diffHunk: thread.diffHunk, + resolved: thread.isResolved, + outdated: thread.isOutdated, + comments: { items: thread.comments.map(comment), nextCursor: null }, + capabilities: overview.authorization.capabilities, + })); + threads.push( + ...result.conversation.map(value => ({ + id: value.nodeId, + reference: { + provider: 'github' as const, + kind: 'comment' as const, + id: value.nodeId, + url: `${overview.identity.canonicalUrl}#issuecomment-${value.commentId}`, + }, + subjectType: 'conversation' as const, + file: null, + position: null, + diffHunk: null, + resolved: null, + outdated: null, + comments: { items: [comment(value)], nextCursor: null }, + capabilities: {}, + })) + ); + return { items: threads, nextCursor: paging.next(result.nextCursor) }; + } + + async function runOperation( + overview: ReviewOverview, + intent: ReviewIntent, + operationKey: string, + statusOnly = false + ): Promise { + if ( + intent.accountId !== ctx.user.id || + intent.actorId !== overview.authorization.actor.id || + reviewResourceKey(ctx.user.id, intent.review) !== + reviewResourceKey(ctx.user.id, overview.identity) + ) + throw conflict(); + const input = intent.input; + const fields: Partial> = { + comment: ['body'], + inlineComment: ['body', 'position'], + reply: ['body', 'target'], + submitReview: ['body', 'choice', 'comments'], + approve: ['body'], + requestChanges: ['body'], + merge: ['method', 'commitTitle', 'commitMessage', 'deletion'], + resolveThread: ['target'], + reopenThread: ['target'], + addReaction: ['target', 'reaction'], + removeReaction: ['target', 'reaction'], + updateBranch: [], + enableAutoMerge: ['method', 'commitTitle', 'commitMessage'], + disableAutoMerge: [], + }; + const allowed = fields[input.action]; + if (!allowed || Object.keys(input).some(key => key !== 'action' && !allowed.includes(key))) + throw new TRPCError({ code: 'BAD_REQUEST', message: 'invalid_action_fields' }); + function freshWrite() { + sameRevision(intent.revision, overview.revision); + const capability = overview.authorization.capabilities[input.action]; + if (reviewActionAvailability(capability) !== 'available') + throw new TRPCError({ + code: capability.permission === 'forbidden' ? 'FORBIDDEN' : 'PRECONDITION_FAILED', + message: capability.explanation || capability.restrictions[0] || 'action_not_available', + }); + if ( + ['merge', 'enableAutoMerge'].includes(input.action) && + !overview.merge.methods.some( + method => method.id === (input.method ?? 'merge').toLowerCase() + ) + ) + throw new TRPCError({ code: 'BAD_REQUEST', message: 'merge_method_not_available' }); + } + async function authorizeWrite() { + if (overview.identity.authorization.kind !== 'githubUser') throw conflict(); + const current = await authorization(overview.identity.authorization); + if (current.actor.id !== intent.actorId) throw conflict(); + } + const common = { ...address(overview), operationKey }; + const reference = { + provider: 'github' as const, + kind: 'review' as const, + id: overview.identity.reviewId, + url: overview.identity.canonicalUrl, + }; + const confirmed = (ref = reference): ReviewMutationResult => ({ + status: 'confirmed', + reference: ref, + retry: 'never', + reconciliation: 'complete', + }); + const unresolved = (reason: string): ReviewMutationResult => ({ + status: 'unresolved', + reason, + reference, + retry: 'reconcile', + reconciliation: 'required', + }); + const position = (value?: ReviewPosition) => { + if (!value || value.native.provider !== 'github') + throw new TRPCError({ code: 'BAD_REQUEST', message: 'invalid_position' }); + sameRevision(value.revision, intent.revision); + return CommentPositionSchema.parse({ + path: value.newPath ?? value.oldPath, + line: value.line, + side: value.side === 'old' ? 'LEFT' : 'RIGHT', + startLine: value.startLine, + startSide: + value.startSide === undefined ? undefined : value.startSide === 'old' ? 'LEFT' : 'RIGHT', + }); + }; + async function ledger( + intentName: PrLedgerIntent, + legacy: Record, + execute: () => Promise + ) { + const [row] = await db + .select() + .from(operation_ledgers) + .where( + and( + eq(operation_ledgers.kilo_user_id, ctx.user.id), + eq(operation_ledgers.domain, 'pr'), + eq(operation_ledgers.operation_key, operationKey) + ) + ) + .limit(1); + // Old ledger addresses retain caller casing, unlike canonical repository metadata. + // Keep those bytes until old clients/records and the 30-day ledger window expire. + const savedAddress = row?.resource_key.match( + /^([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)#([1-9]\d*)::[a-f0-9]{16}$/ + ); + if ( + savedAddress && + `${savedAddress[1]}/${savedAddress[2]}`.toLowerCase() === + overview.identity.repository.fullName.toLowerCase() && + savedAddress[3] === overview.identity.number + ) { + legacy.owner = savedAddress[1]; + legacy.repo = savedAddress[2]; + } + if ( + row && + (row.intent !== intentName || row.resource_key !== prLedgerResourceKey(intentName, legacy)) + ) + throw new TRPCError({ code: 'CONFLICT', message: 'operation_key_reuse_mismatch' }); + // Unsettled merge retries share read-only recovery. Legacy reconciliation can + // settle a different merged head or dispatch another write under this key. + if (row && intentName === 'merge' && !['completed', 'no_op', 'failed'].includes(row.status)) + return overview.state === 'merged' && overview.revision.headSha === intent.revision.headSha + ? normalizeResult({ merged: true, branchDeleted: false }) + : unresolved('merge_not_confirmed'); + if (statusOnly) { + if (!row) + return { + status: 'rejected' as const, + code: 'operation_not_admitted', + explanation: 'operation_not_admitted', + retry: 'same-key' as const, + reconciliation: 'not-needed' as const, + }; + if (row.status === 'completed' || row.status === 'no_op') + return normalizeResult(row.canonical_result); + if (row.status === 'failed') + return { + status: 'rejected' as const, + code: 'operation_failed', + explanation: 'operation_failed', + retry: 'never' as const, + reconciliation: 'not-needed' as const, + }; + const referenceKey = intentName === 'submit_review' ? 'reviewId' : 'commentId'; + const providerId = z + .number() + .int() + .positive() + .safeParse(row.canonical_result?.[referenceKey]); + if (!providerId.success) return unresolved('provider_receipt_unavailable'); + try { + const actualId = await call(async octokit => { + const response = + intentName === 'submit_review' + ? await octokit.pulls.getReview({ + ...address(overview), + pull_number: common.number, + review_id: providerId.data, + }) + : await octokit.pulls.getReviewComment({ + owner: common.owner, + repo: common.repo, + comment_id: providerId.data, + }); + return response.data.id; + }); + return actualId === providerId.data + ? normalizeResult(row.canonical_result) + : unresolved('provider_receipt_unavailable'); + } catch (error) { + if (error instanceof TRPCError && ['NOT_FOUND', 'BAD_GATEWAY'].includes(error.code)) + return unresolved('provider_receipt_unavailable'); + throw error; + } + } + if (!row) freshWrite(); + await authorizeWrite(); + return normalizeResult(await execute()); + } + function normalizeResult(raw: unknown): ReviewMutationResult { + const parsed = z + .object({ + commentId: z.number().int().positive().optional(), + reviewId: z.number().int().positive().optional(), + state: z.string().optional(), + merged: z.boolean().optional(), + branchDeleted: z.boolean().optional(), + }) + .safeParse(raw); + // Old ledger results can lack receipt fields. Keep them unresolved until old + // clients/records disappear and the 30-day ledger window expires. + if (!parsed.success) return unresolved('provider_receipt_unavailable'); + const result = parsed.data; + if (input.action === 'merge' && result.merged !== true) + return unresolved('merge_not_confirmed'); + if ( + input.action !== 'merge' && + (['inlineComment', 'reply'].includes(input.action) ? !result.commentId : !result.reviewId) + ) + return unresolved('provider_receipt_unavailable'); + if (!['merge', 'inlineComment', 'reply'].includes(input.action)) { + const choice = input.action === 'submitReview' ? input.choice : input.action; + const expectedState = + choice === 'approve' + ? 'APPROVED' + : choice === 'requestChanges' + ? 'CHANGES_REQUESTED' + : 'COMMENTED'; + if (result.state !== expectedState) return unresolved('review_state_unconfirmed'); + } + if (result.merged && input.deletion?.effect === 'delete' && result.branchDeleted !== true) + return { + status: 'partial', + items: [ + { + itemId: 'merge', + effect: 'merge', + result: { + status: 'confirmed', + reference, + retry: 'never', + reconciliation: 'complete', + }, + }, + { + itemId: 'deleteBranch', + effect: 'deleteBranch', + result: { + status: 'unresolved', + reference, + reason: 'branch_deletion_unconfirmed', + retry: 'reconcile', + reconciliation: 'required', + }, + }, + ], + retry: 'unfinished-only', + reconciliation: 'required', + }; + return { + status: 'confirmed', + reference: result.commentId + ? { ...reference, kind: 'comment', id: String(result.commentId) } + : result.reviewId + ? { ...reference, id: String(result.reviewId) } + : reference, + retry: 'never', + reconciliation: 'complete', + }; + } + if (input.action === 'inlineComment') { + const legacy = { + ...common, + ...position(input.position), + body: z.string().min(1).parse(input.body), + commitSha: intent.revision.headSha, + }; + return ledger('create_review_comment', legacy, () => caller.createReviewComment(legacy)); + } + if (['comment', 'submitReview', 'approve', 'requestChanges'].includes(input.action)) { + const choice = input.action === 'submitReview' ? input.choice : input.action; + const legacy = { + ...common, + event: + choice === 'approve' + ? ('APPROVE' as const) + : choice === 'requestChanges' + ? ('REQUEST_CHANGES' as const) + : ('COMMENT' as const), + body: input.body, + commitSha: intent.revision.headSha, + comments: input.comments?.map(value => ({ ...position(value.position), body: value.body })), + }; + return ledger('submit_review', legacy, () => caller.submitReview(legacy)); + } + if (input.action === 'merge') { + if ( + input.deletion && + (input.deletion.repositoryKey !== repositoryResourceKey(ctx.user.id, overview.identity) || + input.deletion.branch !== overview.source.branch || + input.deletion.expectedHeadSha !== intent.revision.headSha) + ) + throw conflict(); + const legacy = { + ...common, + method: MergeMethodSchema.parse(input.method), + commitTitle: input.commitTitle, + commitMessage: input.commitMessage, + deleteBranch: input.deletion?.effect === 'delete', + expectedHeadSha: intent.revision.headSha, + }; + return ledger('merge', legacy, () => caller.mergePullRequest(legacy)); + } + let targetThread: ReviewThread | undefined; + let targetComment: ReviewThread['comments']['items'][number] | undefined; + if (input.target) { + if (input.target.provider !== 'github') throw conflict(); + let cursor: ReviewCursor | null = null; + do { + const discussions = await listDiscussions(overview, cursor); + targetThread = discussions.items.find( + thread => + thread.reference.kind === input.target?.kind && thread.reference.id === input.target.id + ); + targetComment = discussions.items + .flatMap(thread => thread.comments.items) + .find(comment => comment.reference.id === input.target?.id); + cursor = discussions.nextCursor; + } while (!targetThread && !targetComment && cursor); + if (!targetThread && !targetComment) + throw new TRPCError({ code: 'NOT_FOUND', message: 'discussion_target_not_found' }); + } + if (input.action === 'reply') { + if (!targetComment) throw new TRPCError({ code: 'BAD_REQUEST', message: 'comment_required' }); + const legacy = { + ...common, + body: z.string().min(1).parse(input.body), + commentId: Number(targetComment.id), + }; + return ledger('reply_comment', legacy, () => caller.replyToComment(legacy)); + } + if (statusOnly) { + switch (input.action) { + case 'resolveThread': + case 'reopenThread': + if (!targetThread || targetThread.reference.kind !== 'thread') throw conflict(); + return targetThread.resolved === (input.action === 'resolveThread') + ? confirmed() + : unresolved('thread_state_unconfirmed'); + case 'addReaction': + case 'removeReaction': { + if (!targetComment) throw conflict(); + const content = ReactionContentSchema.parse(input.reaction); + const reacted = targetComment.reactions.some( + reaction => reaction.content === content && reaction.viewerHasReacted + ); + return reacted === (input.action === 'addReaction') + ? confirmed() + : unresolved('reaction_state_unconfirmed'); + } + case 'enableAutoMerge': + return overview.merge.autoMerge?.method.toUpperCase() === + AutoMergeMethodSchema.parse(input.method?.toUpperCase() ?? 'MERGE') + ? confirmed() + : unresolved('auto_merge_state_unconfirmed'); + case 'disableAutoMerge': + return overview.state === 'open' && overview.merge.autoMerge === null + ? confirmed() + : unresolved('auto_merge_state_unconfirmed'); + case 'updateBranch': { + const source = overview.source.repository; + const ancestors = [intent.revision.headSha, intent.revision.targetHeadSha]; + // Old revisions can omit the target head. Do not infer an update from a changed head. + // Keep this fallback until old clients/records and the 30-day ledger window expire. + if (!source || ancestors.some(value => !sha.safeParse(value).success)) + return unresolved('branch_update_unconfirmed'); + const [owner, repo] = source.fullName.split('/'); + try { + for (const ancestor of ancestors) { + const base = sha.parse(ancestor); + const mergeBase = await call( + async octokit => + ( + await octokit.repos.compareCommits({ + owner, + repo, + base, + head: overview.revision.headSha, + }) + ).data.merge_base_commit.sha + ); + if (mergeBase !== base) return unresolved('branch_update_unconfirmed'); + } + await unchanged(overview); + return confirmed(); + } catch (error) { + if (error instanceof TRPCError && ['NOT_FOUND', 'BAD_GATEWAY'].includes(error.code)) + return unresolved('branch_update_unconfirmed'); + throw error; + } + } + default: + return unresolved('provider_outcome_unknown'); + } + } + freshWrite(); + await authorizeWrite(); + switch (input.action) { + case 'resolveThread': + case 'reopenThread': { + if (!targetThread || targetThread.reference.kind !== 'thread') throw conflict(); + const result = + input.action === 'resolveThread' + ? await caller.resolveThread({ threadId: targetThread.id }) + : await caller.unresolveThread({ threadId: targetThread.id }); + return result.threadId === targetThread.id && + result.isResolved === (input.action === 'resolveThread') + ? confirmed() + : unresolved('thread_state_unconfirmed'); + } + case 'addReaction': + case 'removeReaction': { + if (!targetComment) throw conflict(); + const legacy = { + commentNodeId: targetComment.reference.id, + content: ReactionContentSchema.parse(input.reaction), + }; + const result = + input.action === 'addReaction' + ? await caller.addReaction(legacy) + : await caller.removeReaction(legacy); + return result.content === legacy.content + ? confirmed() + : unresolved('reaction_state_unconfirmed'); + } + case 'updateBranch': + await caller.updateBranch({ + ...address(overview), + expectedHeadSha: intent.revision.headSha, + }); + return { + status: 'accepted', + reference, + task: null, + retry: 'reconcile', + reconciliation: 'pending', + }; + case 'enableAutoMerge': { + const result = await caller.enableAutoMerge({ + ...address(overview), + prNodeId: overview.identity.reviewId, + method: AutoMergeMethodSchema.parse(input.method?.toUpperCase() ?? 'MERGE'), + commitTitle: input.commitTitle, + commitMessage: input.commitMessage, + }); + return result.prNodeId === overview.identity.reviewId + ? confirmed() + : unresolved('auto_merge_state_unconfirmed'); + } + case 'disableAutoMerge': { + const result = await caller.disableAutoMerge({ + ...address(overview), + prNodeId: overview.identity.reviewId, + }); + return result.prNodeId === overview.identity.reviewId + ? confirmed() + : unresolved('auto_merge_state_unconfirmed'); + } + default: + throw new TRPCError({ code: 'BAD_REQUEST', message: 'action_not_available' }); + } + } + return { + getAuthorization, + getReview, + listInbox, + listFiles, + getFileContext, + listDiscussions, + runOperation( + overview: ReviewOverview, + intent: ReviewIntent, + operationKey: string, + statusOnly = false + ) { + const authorization = overview.identity.authorization; + if (authorization.kind !== 'githubUser') throw conflict(); + return withGitHubReviewIdentity( + { + accountId: authorization.accountId, + authorizationId: authorization.authorizationId, + actorId: intent.actorId, + }, + () => runOperation(overview, intent, operationKey, statusOnly) + ); + }, + }; +} diff --git a/apps/web/src/routers/github-pr-review-router.ts b/apps/web/src/routers/github-pr-review-router.ts index 4f98317582..b37189bf14 100644 --- a/apps/web/src/routers/github-pr-review-router.ts +++ b/apps/web/src/routers/github-pr-review-router.ts @@ -37,7 +37,11 @@ import { INBOX_PAGE_SIZE, REVIEW_THREADS_PAGE_SIZE, } from '@/lib/github-pr-review/dtos'; -import { throwTrpcFromGraphQlErrors, withGitHubUserTokenRetry } from '@/lib/github-pr-review/retry'; +import { + hasGitHubReviewIdentity, + throwTrpcFromGraphQlErrors, + withGitHubUserTokenRetry, +} from '@/lib/github-pr-review/retry'; import { getGitHubUserAccessToken } from '@/lib/integrations/platforms/github/user-token-client'; import { AutoMergeMethodSchema, @@ -1275,6 +1279,9 @@ async function reconcileMergePrRow(args: { | { kind: 'stale_head' } | { kind: 'unresolved' }; + // The neutral bridge requires read-only recovery even when admission races its preflight. + // Unscoped legacy callers retain their original reconciliation behavior. + const readOnly = hasGitHubReviewIdentity(args.userId); let reconcile: MergeReconcileState = { kind: 'unresolved' }; try { reconcile = await withGitHubUserTokenRetry({ @@ -1286,6 +1293,12 @@ async function reconcileMergePrRow(args: { pull_number: args.number, }); const pr = prResp.data; + if ( + readOnly && + !(pr.state === 'closed' && pr.merged === true && pr.head?.sha === args.expectedHeadSha) + ) { + return { kind: 'unresolved' } satisfies MergeReconcileState; + } if (pr.state === 'closed' && pr.merged === true) { return { kind: 'merged', @@ -1469,6 +1482,38 @@ async function fetchOverviewGraphQl( } } +// Keep native identities available to the neutral bridge without changing the legacy DTO. +export async function fetchGitHubReviewChecks( + kiloUserId: string, + input: z.infer +) { + return withGitHubUserTokenRetry({ + kiloUserId, + call: async octokit => { + const checkRunsPromise = octokit.paginate(octokit.checks.listForRef, { + owner: input.owner, + repo: input.repo, + ref: input.ref, + per_page: 100, + }); + const statusesPromise = octokit.paginate(octokit.repos.listCommitStatusesForRef, { + owner: input.owner, + repo: input.repo, + ref: input.ref, + per_page: 100, + }); + // Wait for both requests before throwing the first error; neither rejection escapes. + const [checkRunsResult, statusesResult] = await Promise.allSettled([ + checkRunsPromise, + statusesPromise, + ]); + if (checkRunsResult.status === 'rejected') throw checkRunsResult.reason; + if (statusesResult.status === 'rejected') throw statusesResult.reason; + return { checkRuns: checkRunsResult.value, commitStatuses: statusesResult.value }; + }, + }); +} + export const githubPrReviewRouter = createTRPCRouter({ getPullRequest: baseProcedure.input(GetPullRequestInput).query(async ({ ctx, input }) => { const overview = await withGitHubUserTokenRetry({ @@ -1518,41 +1563,7 @@ export const githubPrReviewRouter = createTRPCRouter({ }), listChecks: baseProcedure.input(ListChecksInput).query(async ({ ctx, input }) => { - return withGitHubUserTokenRetry({ - kiloUserId: ctx.user.id, - call: async octokit => { - // Start both paginate calls before awaiting so the checks retrieval - // runs in parallel. - const checkRunsPromise = octokit.paginate(octokit.checks.listForRef, { - owner: input.owner, - repo: input.repo, - ref: input.ref, - per_page: 100, - }); - const statusesPromise = octokit.paginate(octokit.repos.listCommitStatusesForRef, { - owner: input.owner, - repo: input.repo, - ref: input.ref, - per_page: 100, - }); - - const [checkRunsResult, statusesResult] = await Promise.allSettled([ - checkRunsPromise, - statusesPromise, - ]); - - // Rethrow the first rejection in order (check runs, then commit - // statuses). Promise.allSettled — not Promise.all — is used so a second - // rejection after the first never becomes an unhandled rejection. - if (checkRunsResult.status === 'rejected') throw checkRunsResult.reason; - if (statusesResult.status === 'rejected') throw statusesResult.reason; - - return buildChecksResult({ - checkRuns: checkRunsResult.value as never, - commitStatuses: statusesResult.value as never, - }); - }, - }); + return buildChecksResult(await fetchGitHubReviewChecks(ctx.user.id, input)); }), listFiles: baseProcedure.input(ListFilesInput).query(async ({ ctx, input }) => { diff --git a/apps/web/src/routers/provider-review-router.test.ts b/apps/web/src/routers/provider-review-router.test.ts new file mode 100644 index 0000000000..0ad6cee5a7 --- /dev/null +++ b/apps/web/src/routers/provider-review-router.test.ts @@ -0,0 +1,796 @@ +import { isDeepStrictEqual } from 'node:util'; +import { TRPCError } from '@trpc/server'; +import type { PlatformIntegration, User } from '@kilocode/db/schema'; +import { + ReviewActionSchema, + type ReviewOverview, + type ReviewIntent, +} from '@kilocode/app-shared/provider-review'; +import { reviewCapabilityFixtures } from '@kilocode/app-shared/provider-review/fixtures'; +import { providerReviewRouter } from './provider-review-router'; +import { getAllIntegrationsForOwner } from '@/lib/integrations/db/platform-integrations'; +import { ensureOrganizationAccess } from './organizations/utils'; +import { authorizeGitLabReview } from '@/lib/provider-review/gitlab-authorization'; +import { authorizeBitbucketReview } from '@/lib/provider-review/bitbucket-authorization'; +import * as gitlab from '@/lib/provider-review/gitlab-read'; +import * as bitbucket from '@/lib/provider-review/bitbucket-read'; +import { runGitLabReviewOperation } from '@/lib/provider-review/gitlab-write'; +import { runBitbucketReviewOperation } from '@/lib/provider-review/bitbucket-write'; +import { createGitHubReviewBridge } from '@/lib/provider-review/github-bridge'; +import { + GitLabInteractiveError, + type GitLabInteractiveOperations, +} from '@/lib/integrations/platforms/gitlab/interactive-client'; +import { BitbucketInteractiveClientError } from '@/lib/integrations/platforms/bitbucket/interactive-client'; + +jest.mock('@/lib/trpc/init', () => { + const t = jest.requireActual('@trpc/server').initTRPC.create(); + return { baseProcedure: t.procedure, createTRPCRouter: t.router }; +}); +jest.mock('@/lib/drizzle', () => ({ db: {} })); +jest.mock('@/lib/config.server', () => ({})); +jest.mock('@/lib/tokens', () => ({})); +jest.mock('@/lib/integrations/platforms/github/user-token-client', () => ({})); +jest.mock('@/lib/github-pr-review/client', () => ({})); +jest.mock('@/routers/organizations/utils', () => ({ ensureOrganizationAccess: jest.fn() })); +jest.mock('@/lib/integrations/db/platform-integrations', () => ({ + getAllIntegrationsForOwner: jest.fn(), +})); +jest.mock('@/lib/integrations/gitlab-service', () => ({ getGitLabIntegration: jest.fn() })); +jest.mock('@/lib/provider-review/github-bridge', () => ({ + ...jest.requireActual('@/lib/provider-review/github-bridge'), + createGitHubReviewBridge: jest.fn(), +})); +jest.mock('@/lib/provider-review/gitlab-authorization', () => ({ + ...jest.requireActual('@/lib/provider-review/gitlab-authorization'), + authorizeGitLabReview: jest.fn(), +})); +jest.mock('@/lib/provider-review/bitbucket-authorization', () => ({ + ...jest.requireActual('@/lib/provider-review/bitbucket-authorization'), + authorizeBitbucketReview: jest.fn(), +})); +jest.mock('@/lib/provider-review/gitlab-read', () => ({ + getGitLabReview: jest.fn(), + getGitLabChecks: jest.fn(), + listGitLabInbox: jest.fn(), + listGitLabFiles: jest.fn(), + getGitLabFileContext: jest.fn(), + listGitLabDiscussions: jest.fn(), +})); +jest.mock('@/lib/provider-review/bitbucket-read', () => ({ + getBitbucketReview: jest.fn(), + getBitbucketChecks: jest.fn(), + listBitbucketInbox: jest.fn(), + listBitbucketFiles: jest.fn(), + getBitbucketFileContext: jest.fn(), + listBitbucketDiscussions: jest.fn(), +})); +jest.mock('@/lib/provider-review/gitlab-write', () => ({ runGitLabReviewOperation: jest.fn() })); +jest.mock('@/lib/provider-review/bitbucket-write', () => ({ + runBitbucketReviewOperation: jest.fn(), +})); + +const userId = 'oauth/caller'; +const orgId = '11111111-1111-4111-8111-111111111111'; +const integrationId = '22222222-2222-4222-8222-222222222222'; +const repositoryId = '33333333-3333-4333-8333-333333333333'; +const workspaceUuid = '44444444-4444-4444-8444-444444444444'; +const operationKey = '55555555-5555-4555-8555-555555555555'; +const caller = providerReviewRouter.createCaller({ user: { id: userId } as User }); +let overview: ReviewOverview; +let integrations: PlatformIntegration[]; +let effects: ReviewIntent[]; +let statusReads: number; + +function fixture(platform: 'gitlab' | 'bitbucket', type: 'user' | 'org') { + const owner = { type, id: type === 'user' ? userId : orgId }; + const repository = + platform === 'gitlab' + ? { + provider: platform, + instanceUrl: 'https://gitlab.example/Enterprise', + repositoryId: '42', + fullName: 'Group/Sub/Repo', + defaultBranch: null, + } + : { + provider: platform, + instanceUrl: 'https://bitbucket.org', + repositoryId, + workspaceUuid, + fullName: 'team/repo', + defaultBranch: null, + }; + const identity = { + repository, + authorization: { kind: 'ownerIntegration' as const, owner, integrationId }, + reviewId: platform === 'gitlab' ? '77' : '7', + number: '7', + canonicalUrl: `${repository.instanceUrl}/${repository.fullName}/${platform === 'gitlab' ? '-/merge_requests' : 'pull-requests'}/7`, + }; + overview = { + identity, + title: 'Authorized review', + bodyMarkdown: null, + author: null, + state: 'closed', + draft: false, + revision: { + headSha: 'a'.repeat(40), + baseSha: platform === 'gitlab' ? 'b'.repeat(40) : null, + startSha: platform === 'gitlab' ? 'c'.repeat(40) : null, + targetHeadSha: platform === 'bitbucket' ? 'b'.repeat(40) : null, + }, + source: { repository, branch: 'feature' }, + target: { repository, branch: 'trunk' }, + authorization: { + actor: { + provider: platform, + instanceUrl: repository.instanceUrl, + id: 'provider-actor', + displayName: 'Integration actor', + login: null, + avatarUrl: null, + }, + credentialKind: platform === 'gitlab' ? 'gitlabPat' : 'bitbucketWorkspaceToken', + capabilities: reviewCapabilityFixtures(platform), + writeLimits: { requestMaxBytes: 256000, bodyMaxBytes: null }, + }, + providerState: + platform === 'gitlab' + ? { + provider: platform, + approvals: { approved: null, required: null, remaining: null, actorIds: [] }, + requestedChanges: { + actorIds: [], + blocksMerge: null, + blockingCapability: reviewCapabilityFixtures(platform).requestChanges, + }, + } + : { provider: platform, participants: [], expectedHeadProtection: 'none' }, + checks: { status: 'none', checks: [] }, + counts: { commits: 0, files: 0, additions: 0, deletions: 0 }, + merge: { methods: [], squash: null, autoMerge: null, task: null }, + }; + integrations = [ + { + id: integrationId, + platform, + platform_account_id: workspaceUuid, + integration_status: 'active', + suspended_at: null, + auth_invalid_at: null, + owned_by_user_id: type === 'user' ? userId : null, + owned_by_organization_id: type === 'org' ? orgId : null, + metadata: { gitlab_instance_url: repository.instanceUrl }, + repositories: [ + { + id: platform === 'gitlab' ? 42 : repositoryId, + name: 'Repo', + full_name: repository.fullName, + private: true, + }, + ], + } as PlatformIntegration, + ]; + return { provider: platform, owner, integrationId, repository }; +} +type FixtureAuthorization = + | Parameters[0] + | Parameters[0]; + +function authorizeFixture(platform: 'gitlab' | 'bitbucket', input: FixtureAuthorization) { + if ( + platform !== overview.identity.repository.provider || + input.userId !== userId || + !isDeepStrictEqual(input.authorization, overview.identity.authorization) || + ('repository' in input + ? !isDeepStrictEqual(input.repository, overview.identity.repository) + : input.instanceUrl !== overview.identity.repository.instanceUrl) + ) + throw new TRPCError({ code: 'FORBIDDEN', message: 'fixture_authorization_mismatch' }); + return { + ...input, + actor: overview.authorization.actor, + credentialKind: overview.authorization.credentialKind, + scopes: ['api', 'pullrequest'], + }; +} + +beforeEach(() => { + jest.clearAllMocks(); + effects = []; + statusReads = 0; + fixture('gitlab', 'user'); + jest + .mocked(getAllIntegrationsForOwner) + .mockImplementation(async owner => + integrations.filter(integration => + owner.type === 'user' + ? integration.owned_by_user_id === owner.id + : integration.owned_by_organization_id === owner.id + ) + ); + jest.mocked(ensureOrganizationAccess).mockResolvedValue('member'); + jest.mocked(createGitHubReviewBridge).mockImplementation(() => { + throw new Error('Forbidden GitHub dispatch'); + }); + jest + .mocked(authorizeGitLabReview) + .mockImplementation(async input => authorizeFixture('gitlab', input) as any); + jest + .mocked(authorizeBitbucketReview) + .mockImplementation(async input => authorizeFixture('bitbucket', input) as any); + const read = ( + platform: 'gitlab' | 'bitbucket', + auth: FixtureAuthorization, + identity: ReviewIntent['review'], + revision: ReviewIntent['revision'] + ) => { + authorizeFixture(platform, auth); + if ( + !isDeepStrictEqual(identity, overview.identity) || + !isDeepStrictEqual(revision, overview.revision) + ) + throw new TRPCError({ code: 'CONFLICT', message: 'fixture_resource_or_revision_mismatch' }); + return { ...overview, identity, revision }; + }; + jest + .mocked(gitlab.getGitLabReview) + .mockImplementation(async (auth, repository, number) => + read( + 'gitlab', + auth, + { ...overview.identity, repository, number: String(number) }, + overview.revision + ) + ); + jest + .mocked(bitbucket.getBitbucketReview) + .mockImplementation(async (auth, number) => + read( + 'bitbucket', + auth, + { ...overview.identity, repository: auth.repository, number: String(number) }, + overview.revision + ) + ); + const inboxFor = ( + platform: 'gitlab' | 'bitbucket', + auth: FixtureAuthorization, + repository: ReviewOverview['identity']['repository'] + ) => { + read(platform, auth, { ...overview.identity, repository }, overview.revision); + return { + items: [], + nextCursor: null, + scope: { kind: 'repository' as const, actor: overview.authorization.actor, repository }, + }; + }; + jest + .mocked(gitlab.listGitLabInbox) + .mockImplementation(async (auth, input) => + inboxFor('gitlab', auth, input?.repository as ReviewOverview['identity']['repository']) + ); + jest + .mocked(bitbucket.listBitbucketInbox) + .mockImplementation(async auth => inboxFor('bitbucket', auth, auth.repository)); + for (const [platform, files, discussions, checks, context] of [ + [ + 'gitlab', + gitlab.listGitLabFiles, + gitlab.listGitLabDiscussions, + gitlab.getGitLabChecks, + gitlab.getGitLabFileContext, + ], + [ + 'bitbucket', + bitbucket.listBitbucketFiles, + bitbucket.listBitbucketDiscussions, + bitbucket.getBitbucketChecks, + bitbucket.getBitbucketFileContext, + ], + ] as const) { + jest.mocked(files).mockImplementation(async (auth, identity, revision) => { + read(platform, auth, identity, revision); + return { items: [], nextCursor: null }; + }); + jest.mocked(discussions).mockImplementation(async (auth, identity) => { + read(platform, auth, identity, overview.revision); + return { items: [], nextCursor: null }; + }); + jest + .mocked(checks) + .mockImplementation( + async (auth, identity, revision) => read(platform, auth, identity, revision).checks + ); + jest.mocked(context).mockImplementation(async (auth, identity, input) => { + read(platform, auth, identity, input.file.revision); + return { + ...input.file, + path: input.side === 'old' ? 'old.ts' : 'new.ts', + side: input.side, + startLine: input.startLine, + lines: [], + totalLines: null, + content: 'binary', + canonicalUrl: identity.canonicalUrl, + }; + }); + } + const run = async ( + platform: 'gitlab' | 'bitbucket', + auth: FixtureAuthorization, + request: { intent: ReviewIntent }, + statusOnly?: boolean + ) => { + read(platform, auth, request.intent.review, request.intent.revision); + if (statusOnly) statusReads++; + else effects.push(request.intent); + return { + status: 'unresolved' as const, + reason: statusOnly ? 'pending_task' : 'provider_receipt', + reference: { + provider: platform, + kind: 'review' as const, + id: request.intent.review.reviewId, + url: request.intent.review.canonicalUrl, + }, + retry: 'reconcile' as const, + reconciliation: 'required' as const, + }; + }; + jest + .mocked(runGitLabReviewOperation) + .mockImplementation((auth, request, statusOnly) => run('gitlab', auth, request, statusOnly)); + jest + .mocked(runBitbucketReviewOperation) + .mockImplementation((auth, request, statusOnly) => run('bitbucket', auth, request, statusOnly)); +}); +afterEach(() => expect(createGitHubReviewBridge).not.toHaveBeenCalled()); + +it.each([ + ['gitlab', 'user'], + ['gitlab', 'org'], + ['bitbucket', 'org'], +] as const)( + 'AC4–AC7 routes every %s/%s surface with its authorized actor', + async (platform, owner) => { + const scope = fixture(platform, owner); + const review = overview.identity; + expect(await caller.getAuthorization(scope)).toMatchObject({ + status: 'connected', + actor: overview.authorization.actor, + }); + expect( + await caller.resolveUrl({ url: review.canonicalUrl, owner: scope.owner, integrationId }) + ).toEqual(review); + expect(await caller.getReview({ review })).toEqual(overview); + expect(await caller.listInbox(scope)).toMatchObject({ + items: [], + scope: { actor: overview.authorization.actor, repository: review.repository }, + }); + expect(await caller.listFiles({ review, revision: overview.revision })).toEqual({ + items: [], + nextCursor: null, + authorization: overview.authorization, + }); + expect(await caller.listChecks({ review, revision: overview.revision })).toEqual({ + checks: { status: 'none', checks: [] }, + authorization: overview.authorization, + }); + expect(await caller.listDiscussions({ review })).toMatchObject({ + items: [], + authorization: overview.authorization, + }); + expect( + await caller.getFileContext({ + review, + context: { + file: { oldPath: 'old.ts', newPath: 'new.ts', revision: overview.revision }, + side: 'old', + startLine: 5, + lineCount: 10, + }, + }) + ).toMatchObject({ + content: 'binary', + path: 'old.ts', + startLine: 5, + canonicalUrl: review.canonicalUrl, + authorization: overview.authorization, + }); + const actions = ReviewActionSchema.options.filter(action => action !== 'read'); + for (const action of actions) { + const input = { + review, + revision: overview.revision, + actorId: 'provider-actor', + operationKey, + input: { action }, + }; + const reference = { + provider: platform, + kind: 'review', + id: review.reviewId, + url: review.canonicalUrl, + }; + expect(await caller.act(input)).toMatchObject({ + result: { status: 'unresolved', reason: 'provider_receipt', reference }, + authorization: overview.authorization, + }); + expect(await caller.getOperationStatus(input)).toMatchObject({ + result: { status: 'unresolved', reason: 'pending_task', reference }, + }); + } + expect(statusReads).toBe(17); + expect(effects).toEqual( + actions.map(action => ({ + accountId: userId, + actorId: 'provider-actor', + review, + revision: overview.revision, + input: { action }, + })) + ); + } +); +it.each([ + ['gitlab', 'user'], + ['gitlab', 'org'], + ['bitbucket', 'org'], +] as const)( + 'AC10 resolves old %s/%s references and ignores unrelated saved fields', + async (platform, type) => { + const scope = fixture(platform, type); + const saved = JSON.parse( + JSON.stringify({ + repository: { provider: platform, fullName: scope.repository.fullName }, + authorization: { kind: 'ownerIntegration', owner: scope.owner }, + number: 7, + futureField: 'ignored', + }) + ); + expect((await caller.getReview({ review: saved })).identity).toEqual(overview.identity); + expect(overview.identity.repository.defaultBranch).toBeNull(); + expect( + await caller.resolveUrl({ url: overview.identity.canonicalUrl, owner: scope.owner }) + ).toEqual(overview.identity); + } +); +describe('uncached GitLab project resolution', () => { + let project: Record; + const showProject = jest.fn(); + + beforeEach(() => { + showProject.mockReset().mockImplementation(async (projectId: string | number) => { + if (projectId !== overview.identity.repository.fullName) + throw new Error('Unexpected GitLab project lookup'); + return project; + }); + integrations[0].repositories = null; + project = { + id: 42, + path_with_namespace: 'Group/Sub/Repo', + web_url: 'https://gitlab.example/Enterprise/Group/Sub/Repo', + default_branch: null, + }; + jest.mocked(authorizeGitLabReview).mockImplementation( + async input => + ({ + ...authorizeFixture('gitlab', input), + instanceUrl: input.instanceUrl, + client: (projectId?: string) => { + if (projectId !== overview.identity.repository.fullName) + throw new Error('Unexpected GitLab client scope'); + return { + execute: async (operation: (api: GitLabInteractiveOperations) => Promise) => ({ + status: 200 as const, + headers: {}, + data: await operation({ + Projects: { show: showProject }, + } as GitLabInteractiveOperations), + }), + }; + }, + }) as any + ); + }); + + it.each([ + ['user', false], + ['org', false], + ['user', true], + ['org', true], + ] as const)('AC4 resolves an accessible %s project with archived=%s', async (owner, archived) => { + const scope = fixture('gitlab', owner); + integrations[0].repositories = archived + ? [{ id: 99, name: 'Other', full_name: 'Group/Other', private: true }] + : []; + project.archived = archived; + project.default_branch = 'release/next'; + overview.identity.repository.defaultBranch = 'release/next'; + expect( + await caller.resolveUrl({ + url: `${overview.identity.canonicalUrl}/diffs`, + owner: scope.owner, + integrationId, + }) + ).toEqual(overview.identity); + expect(showProject).toHaveBeenCalledTimes(1); + expect(showProject).toHaveBeenCalledWith('Group/Sub/Repo'); + }); + + it('AC10 resolves an uncached old reference without a numeric project ID', async () => { + expect( + await caller.getReview({ + review: { + repository: { provider: 'gitlab', fullName: 'Group/Sub/Repo' }, + number: 7, + }, + }) + ).toEqual(overview); + }); + + it('AC4 resolves an uncached repository-scoped empty inbox', async () => { + expect( + await caller.listInbox({ + provider: 'gitlab', + repository: { provider: 'gitlab', fullName: 'Group/Sub/Repo' }, + }) + ).toEqual({ + items: [], + nextCursor: null, + scope: { + kind: 'repository', + actor: overview.authorization.actor, + repository: overview.identity.repository, + }, + }); + }); + + it.each([ + { path_with_namespace: 'Group/Other/Repo' }, + { + path_with_namespace: 'Group/Other/Repo', + web_url: 'https://gitlab.example/Enterprise/Group/Other/Repo', + }, + { web_url: 'https://other.example/Enterprise/Group/Sub/Repo' }, + { web_url: 'https://gitlab.example/Other/Group/Sub/Repo' }, + { web_url: 'https://gitlab.example/Enterprise/Group/Other/Repo' }, + ])('AC10 rejects a mismatched lookup identity: %j', async change => { + Object.assign(project, change); + await expect(caller.resolveUrl({ url: overview.identity.canonicalUrl })).rejects.toMatchObject({ + code: 'NOT_FOUND', + message: 'not_found', + }); + expect(gitlab.getGitLabReview).not.toHaveBeenCalled(); + }); + + it.each([0, -1, 1.5, '42'])('AC10 rejects a nonnumeric or invalid project ID: %s', async id => { + project.id = id; + await expect(caller.resolveUrl({ url: overview.identity.canonicalUrl })).rejects.toMatchObject({ + code: 'SERVICE_UNAVAILABLE', + message: 'invalid_response', + }); + expect(gitlab.getGitLabReview).not.toHaveBeenCalled(); + }); + + it.each([ + ['forbidden', 'FORBIDDEN'], + ['not_found', 'NOT_FOUND'], + ['reconnect_required', 'PRECONDITION_FAILED'], + ] as const)('AC4 preserves the project lookup rejection %s', async (code, expected) => { + showProject.mockRejectedValueOnce(new GitLabInteractiveError(code)); + await expect(caller.resolveUrl({ url: overview.identity.canonicalUrl })).rejects.toMatchObject({ + code: expected, + message: code, + }); + expect(gitlab.getGitLabReview).not.toHaveBeenCalled(); + }); + + it('AC4 retries a temporary lookup failure with the same pasted URL', async () => { + showProject.mockRejectedValueOnce(new GitLabInteractiveError('temporarily_unavailable')); + const input = { url: overview.identity.canonicalUrl }; + await expect(caller.resolveUrl(input)).rejects.toMatchObject({ + code: 'SERVICE_UNAVAILABLE', + message: 'temporarily_unavailable', + }); + await expect(caller.resolveUrl(input)).resolves.toEqual(overview.identity); + }); + + it.each([ + { url: 'https://other.example/Enterprise/Group/Sub/Repo/-/merge_requests/7' }, + { url: 'https://gitlab.example/Other/Group/Sub/Repo/-/merge_requests/7' }, + { url: 'https://gitlab.example/Enterprise/Group/Sub/Repo/issues/7' }, + { owner: { type: 'user' as const, id: 'other' } }, + { integrationId: '66666666-6666-4666-8666-666666666666' }, + ])('AC10 rejects unauthorized uncached URL input: %j', async change => { + await expect( + caller.resolveUrl({ url: overview.identity.canonicalUrl, ...change }) + ).rejects.toBeInstanceOf(TRPCError); + expect(authorizeGitLabReview).not.toHaveBeenCalled(); + expect(showProject).not.toHaveBeenCalled(); + }); + + it('AC10 requires an explicit integration when uncached URL resolution is ambiguous', async () => { + integrations.push({ ...integrations[0], id: '66666666-6666-4666-8666-666666666666' }); + await expect(caller.resolveUrl({ url: overview.identity.canonicalUrl })).rejects.toMatchObject({ + code: 'CONFLICT', + message: 'integration_ambiguous', + }); + expect(authorizeGitLabReview).not.toHaveBeenCalled(); + expect(showProject).not.toHaveBeenCalled(); + await expect( + caller.resolveUrl({ url: overview.identity.canonicalUrl, integrationId }) + ).resolves.toEqual(overview.identity); + }); +}); + +it.each(['account', 'review', 'repository', 'url', 'actor'] as const)( + 'AC10 rejects changed %s identity before an effect', + async field => { + const input = { + review: structuredClone(overview.identity), + revision: overview.revision, + actorId: 'provider-actor', + operationKey, + input: { action: 'comment' as const, body: 'Preserved' }, + }; + if (field === 'account' && input.review.authorization.kind === 'ownerIntegration') + input.review.authorization.owner = { type: 'user', id: 'other' }; + if (field === 'review') input.review.reviewId = 'another'; + if (field === 'repository') input.review.repository.repositoryId = '43'; + if (field === 'url') input.review.canonicalUrl = 'https://evil.example/review'; + if (field === 'actor') input.actorId = 'other'; + await expect(caller.act(input)).rejects.toBeInstanceOf(TRPCError); + expect(effects).toEqual([]); + } +); +it('AC4 preserves read access with unavailable write grants', async () => { + const scope = fixture('bitbucket', 'org'); + overview.authorization.capabilities.approve.permission = 'forbidden'; + overview.authorization.capabilities.approve.recovery = 'replaceToken'; + jest.mocked(runBitbucketReviewOperation).mockResolvedValue({ + status: 'rejected', + code: 'insufficient_permissions', + explanation: 'replaceToken', + retry: 'never', + reconciliation: 'not-needed', + }); + expect((await caller.getReview({ review: overview.identity })).title).toBe('Authorized review'); + expect(await caller.listInbox(scope)).toMatchObject({ items: [] }); + expect( + await caller.act({ + review: overview.identity, + revision: overview.revision, + actorId: 'provider-actor', + operationKey, + input: { action: 'approve' }, + }) + ).toMatchObject({ + result: { status: 'rejected', retry: 'never' }, + authorization: { + capabilities: { approve: { permission: 'forbidden', recovery: 'replaceToken' } }, + }, + }); +}); +it.each([ + 'https://evil.example/Group/Sub/Repo/-/merge_requests/7', + 'https://gitlab.example/Other/Group/Sub/Repo/-/merge_requests/7', + 'https://gitlab.example/Enterprise/Group/Sub/Repo/issues/7', + 'https://gitlab.example/Enterprise/Group/Sub/Repo/-/merge_requests/0', + 'https://user:secret@gitlab.example/Enterprise/Group/Sub/Repo/-/merge_requests/7', +])('AC10 rejects invalid or unauthorized pasted URL %s without provider calls', async url => { + await expect(caller.resolveUrl({ url })).rejects.toBeInstanceOf(TRPCError); + expect(authorizeGitLabReview).not.toHaveBeenCalled(); +}); +it('AC10 rejects ambiguous integrations instead of selecting a different connection', async () => { + integrations.push({ ...integrations[0], id: '66666666-6666-4666-8666-666666666666' }); + await expect(caller.resolveUrl({ url: overview.identity.canonicalUrl })).rejects.toMatchObject({ + code: 'CONFLICT', + message: 'integration_ambiguous', + }); + expect(authorizeGitLabReview).not.toHaveBeenCalled(); +}); +it('AC4 rejects Personal Bitbucket and denied organization access', async () => { + await expect(caller.listInbox({ provider: 'bitbucket' })).rejects.toMatchObject({ + code: 'FORBIDDEN', + message: 'bitbucket_requires_organization', + }); + const scope = fixture('gitlab', 'org'); + jest + .mocked(ensureOrganizationAccess) + .mockRejectedValue(new TRPCError({ code: 'UNAUTHORIZED', message: 'membership_required' })); + await expect(caller.listInbox(scope)).rejects.toMatchObject({ + code: 'UNAUTHORIZED', + message: 'membership_required', + }); + expect(authorizeGitLabReview).not.toHaveBeenCalled(); +}); +it.each([ + ['reconnect_required', 'PRECONDITION_FAILED'], + ['forbidden', 'FORBIDDEN'], + ['conflict', 'CONFLICT'], + ['temporarily_unavailable', 'SERVICE_UNAVAILABLE'], +] as const)('AC4 preserves GitLab recovery class %s', async (code, expected) => { + jest.mocked(gitlab.getGitLabReview).mockRejectedValue(new GitLabInteractiveError(code)); + await expect(caller.getReview({ review: overview.identity })).rejects.toMatchObject({ + code: expected, + message: code, + }); +}); +it('AC4 preserves a later Bitbucket page failure and cursor without inventing empty success', async () => { + const scope = fixture('bitbucket', 'org'); + jest + .mocked(bitbucket.listBitbucketInbox) + .mockRejectedValue(new BitbucketInteractiveClientError('provider_unavailable')); + await expect( + caller.listInbox({ ...scope, cursor: { scopeKey: 'bound-page', token: 'next' } }) + ).rejects.toMatchObject({ code: 'SERVICE_UNAVAILABLE', message: 'provider_unavailable' }); +}); +it('AC6 rejects an oversized serialized write before authorization', async () => { + await expect( + caller.act({ + review: overview.identity, + revision: overview.revision, + actorId: 'provider-actor', + operationKey, + input: { action: 'comment', body: 'x'.repeat(256000) }, + }) + ).rejects.toMatchObject({ code: 'PAYLOAD_TOO_LARGE' }); + expect(getAllIntegrationsForOwner).not.toHaveBeenCalled(); + expect(effects).toEqual([]); +}); +it('AC4 distinguishes no integration from an empty authorized inbox', async () => { + integrations = []; + expect(await caller.getAuthorization({ provider: 'gitlab' })).toEqual({ + status: 'not_connected', + reason: 'not_connected', + authorization: null, + actor: null, + }); +}); +it.each(['gitlab', 'bitbucket'])( + 'AC10 never downgrades an explicit %s legacy record to GitHub', + async provider => { + const saved = JSON.parse(JSON.stringify({ owner: 'Team', repo: 'Repo', number: 7, provider })); + await expect(caller.getReview({ review: saved })).rejects.toMatchObject({ + code: 'BAD_REQUEST', + }); + } +); +it('AC10 never drops malformed normalized identity through a legacy fallback', async () => { + const saved = JSON.parse( + JSON.stringify({ + owner: 'Team', + repo: 'Repo', + number: 7, + repository: { ...overview.identity.repository, provider: 'unknown' }, + }) + ); + await expect(caller.getReview({ review: saved })).rejects.toMatchObject({ code: 'BAD_REQUEST' }); +}); +it('AC6 rejects a foreign provider position at the public boundary', async () => { + await expect( + caller.act({ + review: overview.identity, + actorId: 'provider-actor', + revision: overview.revision, + operationKey, + input: { + action: 'inlineComment', + body: 'Keep my selection', + position: { + revision: overview.revision, + oldPath: 'old.ts', + newPath: 'new.ts', + side: 'new', + line: 2, + native: { provider: 'github' }, + }, + }, + }) + ).rejects.toMatchObject({ code: 'BAD_REQUEST' }); + expect(effects).toEqual([]); +}); diff --git a/apps/web/src/routers/provider-review-router.ts b/apps/web/src/routers/provider-review-router.ts new file mode 100644 index 0000000000..a7ab460b22 --- /dev/null +++ b/apps/web/src/routers/provider-review-router.ts @@ -0,0 +1,667 @@ +import 'server-only'; + +import { z } from 'zod'; +import { TRPCError } from '@trpc/server'; +import { CODE_REVIEW_PLATFORMS } from '@kilocode/app-shared/code-review'; +import type { + Owner, + RepositoryIdentity, +} from '@kilocode/app-shared/code-review/repository-identity'; +import { + ReviewCursorSchema, + ReviewIntentInputSchema, + ReviewRevisionSchema, + reviewResourceKey, + serializeReviewWriteRequest, + type ReviewIdentity, + type ReviewIntent, + type ReviewOverview, +} from '@kilocode/app-shared/provider-review'; +import { baseProcedure, createTRPCRouter, type TRPCContext } from '@/lib/trpc/init'; +import { ensureOrganizationAccess } from '@/routers/organizations/utils'; +import { getAllIntegrationsForOwner } from '@/lib/integrations/db/platform-integrations'; +import { normalizeGitLabInstanceUrl } from '@/lib/integrations/platforms/gitlab/instance-url'; +import { GitLabInteractiveError } from '@/lib/integrations/platforms/gitlab/interactive-client'; +import { BitbucketInteractiveClientError } from '@/lib/integrations/platforms/bitbucket/interactive-client'; +import { + authorizeGitLabReview, + GitLabProjectSchema, + gitLabResourceUrl, + parseGitLab, + type GitLabReviewAuthorization, +} from '@/lib/provider-review/gitlab-authorization'; +import { + authorizeBitbucketReview, + BitbucketUuidSchema, +} from '@/lib/provider-review/bitbucket-authorization'; +import { + getGitLabReview, + getGitLabChecks, + listGitLabInbox, + listGitLabFiles, + getGitLabFileContext, + listGitLabDiscussions, +} from '@/lib/provider-review/gitlab-read'; +import { + getBitbucketReview, + getBitbucketChecks, + listBitbucketInbox, + listBitbucketFiles, + getBitbucketFileContext, + listBitbucketDiscussions, +} from '@/lib/provider-review/bitbucket-read'; +import { runGitLabReviewOperation } from '@/lib/provider-review/gitlab-write'; +import { runBitbucketReviewOperation } from '@/lib/provider-review/bitbucket-write'; +import { + createGitHubReviewBridge, + GitHubReviewAddressSchema, +} from '@/lib/provider-review/github-bridge'; + +const id = z.string().min(1).max(4096); +const provider = z.enum(CODE_REVIEW_PLATFORMS); +const ownerSchema = z.discriminatedUnion('type', [ + z.object({ type: z.literal('user'), id }), + z.object({ type: z.literal('org'), id: z.uuid() }), +]); +const fullName = id.refine( + value => + value.split('/').length >= 2 && + value + .split('/') + .every( + part => + part && + part !== '.' && + part !== '..' && + !/[\\%?#]/.test(part) && + [...part].every( + character => character.charCodeAt(0) >= 32 && character.charCodeAt(0) !== 127 + ) + ) +); +// Old references omit provider, instance, integration and default branch. Resolve them +// through authorized lookup until old clients/records disappear and the 30-day ledger window expires. +const repositoryWire = z.object({ + provider: provider.default('github'), + fullName, + repositoryId: id.optional(), + instanceUrl: z.url().optional(), + defaultBranch: z.string().nullable().optional(), + workspaceUuid: id.optional(), +}); +const authorizationWire = z.discriminatedUnion('kind', [ + z.object({ kind: z.literal('githubUser'), accountId: id, authorizationId: id.optional() }), + z.object({ + kind: z.literal('ownerIntegration'), + owner: ownerSchema, + integrationId: z.uuid().optional(), + }), +]); +const number = z + .union([z.string().regex(/^[1-9]\d*$/), z.number().int().positive().safe()]) + .transform(String); +const reviewWire = z.object({ + repository: repositoryWire, + authorization: authorizationWire.optional(), + number, + reviewId: id.optional(), + canonicalUrl: z.url().optional(), +}); +// Old saved GitHub routes/drafts carry owner/repo/number and can contain unknown saved fields. +// Strip only those record additions until old clients/records and the 30-day ledger window expire. +const reviewInput = z.union([ + reviewWire, + GitHubReviewAddressSchema.extend({ + accountId: id.optional(), + provider: z.literal('github').optional(), + instanceUrl: z.literal('https://github.com').optional(), + repository: z.never().optional(), + authorization: z.never().optional(), + }).transform(value => ({ + repository: { provider: 'github' as const, fullName: `${value.owner}/${value.repo}` }, + authorization: value.accountId + ? { kind: 'githubUser' as const, accountId: value.accountId } + : undefined, + number: String(value.number), + })), +]); +const scopeInput = z.object({ + provider: provider.default('github'), + owner: ownerSchema.optional(), + integrationId: z.uuid().optional(), + instanceUrl: z.url().optional(), + repository: repositoryWire.optional(), +}); +const pageInput = z.object({ + review: reviewInput, + cursor: ReviewCursorSchema.nullish(), + direction: z.enum(['forward', 'backward']).optional(), +}); +const fileContextInput = z.object({ + file: z.object({ + oldPath: id.nullable(), + newPath: id.nullable(), + revision: ReviewRevisionSchema, + }), + side: z.enum(['old', 'new']), + startLine: z.number().int().positive().safe(), + lineCount: z.number().int().positive().max(500), + versionId: id.optional(), +}); +const operationInput = z.object({ + review: reviewInput, + actorId: id, + revision: ReviewRevisionSchema, + input: ReviewIntentInputSchema, + operationKey: z.uuid(), +}); +type Scope = z.infer; +type ReviewWire = z.infer; +function fail(code: ConstructorParameters[0]['code'], message: string): never { + throw new TRPCError({ code, message }); +} + +async function integrationFor(ctx: TRPCContext, input: Scope, url?: URL) { + const owner: Owner = input.owner ?? { type: 'user', id: ctx.user.id }; + if (owner.type === 'user' && owner.id !== ctx.user.id) fail('FORBIDDEN', 'owner_mismatch'); + if (input.provider === 'bitbucket' && owner.type !== 'org') + fail('FORBIDDEN', 'bitbucket_requires_organization'); + if (owner.type === 'org') await ensureOrganizationAccess(ctx, owner.id); + const integrations = await getAllIntegrationsForOwner(owner); + const candidates = integrations + .filter( + integration => + integration.platform === input.provider && + (!input.integrationId || input.integrationId === integration.id) + ) + .map(integration => { + // Old GitLab integration records omit the host. Retain its existing default until + // old clients/records disappear and the 30-day ledger window expires. + const metadata = z + .object({ gitlab_instance_url: z.string().optional() }) + .parse(integration.metadata ?? {}); + return { + integration, + instanceUrl: + input.provider === 'gitlab' + ? normalizeGitLabInstanceUrl(metadata.gitlab_instance_url) + : 'https://bitbucket.org', + }; + }) + .filter(candidate => { + if (input.instanceUrl && input.instanceUrl.replace(/\/+$/, '') !== candidate.instanceUrl) + return false; + if (!url) return true; + const base = new URL(candidate.instanceUrl); + return ( + url.origin === base.origin && + url.pathname.startsWith(`${base.pathname.replace(/\/+$/, '')}/`) + ); + }); + if (!candidates.length) fail('NOT_FOUND', 'integration_not_found'); + if (candidates.length !== 1) fail('CONFLICT', 'integration_ambiguous'); + const selected = candidates[0]; + if ( + selected.integration.integration_status !== 'active' || + selected.integration.suspended_at || + selected.integration.auth_invalid_at + ) + fail('PRECONDITION_FAILED', 'reconnect_required'); + return { + ...selected, + authorization: { + kind: 'ownerIntegration' as const, + owner, + integrationId: selected.integration.id, + }, + }; +} +async function repositoryFor( + input: z.infer, + selected: Awaited>, + auth?: GitLabReviewAuthorization +): Promise { + if ( + input.provider !== selected.integration.platform || + (input.instanceUrl && input.instanceUrl.replace(/\/+$/, '') !== selected.instanceUrl) + ) + fail('FORBIDDEN', 'repository_instance_mismatch'); + const matches = + selected.integration.repositories?.filter( + repository => repository.full_name === input.fullName + ) ?? []; + if (matches.length > 1) fail('CONFLICT', 'repository_ambiguous'); + let repositoryId = input.repositoryId ?? (matches[0] ? String(matches[0].id) : undefined); + let defaultBranch = input.defaultBranch ?? matches[0]?.default_branch ?? null; + if (!repositoryId && input.provider === 'gitlab' && auth) { + // Discovery can omit accessible projects, including archived projects. Resolve only this path. + const result = await auth + .client(input.fullName) + .execute(api => api.Projects.show(input.fullName)); + const project = parseGitLab(GitLabProjectSchema, result.data); + if ( + project.path_with_namespace !== input.fullName || + new URL(project.web_url).toString() !== gitLabResourceUrl(auth.instanceUrl, input.fullName) + ) + throw new GitLabInteractiveError('not_found'); + repositoryId = String(project.id); + defaultBranch = project.default_branch ?? null; + } + if (!repositoryId) fail('NOT_FOUND', 'repository_selection_required'); + const common = { + instanceUrl: selected.instanceUrl, + repositoryId, + fullName: input.fullName, + defaultBranch, + }; + return input.provider === 'bitbucket' + ? { + ...common, + provider: 'bitbucket', + repositoryId: BitbucketUuidSchema.parse(repositoryId), + workspaceUuid: BitbucketUuidSchema.parse( + input.workspaceUuid ?? selected.integration.platform_account_id + ), + } + : { ...common, provider: 'gitlab' }; +} +function assertResolved(input: ReviewWire, actual: ReviewIdentity, accountId: string) { + const expected = input.repository; + if ( + (input.reviewId && input.reviewId !== actual.reviewId) || + (input.canonicalUrl && input.canonicalUrl !== actual.canonicalUrl) || + (expected.repositoryId && expected.repositoryId !== actual.repository.repositoryId) || + (expected.workspaceUuid && expected.workspaceUuid !== actual.repository.workspaceUuid) + ) + fail('CONFLICT', 'review_identity_changed'); + if (input.authorization?.kind === 'githubUser') { + if ( + actual.authorization.kind !== 'githubUser' || + input.authorization.accountId !== accountId || + (input.authorization.authorizationId && + input.authorization.authorizationId !== actual.authorization.authorizationId) + ) + fail('CONFLICT', 'review_authorization_changed'); + } + reviewResourceKey(accountId, actual); +} +async function target(ctx: TRPCContext, input: ReviewWire) { + if (input.repository.provider === 'github') { + if (input.authorization?.kind === 'ownerIntegration') + fail('FORBIDDEN', 'github_user_authorization_required'); + if (input.authorization && input.authorization.accountId !== ctx.user.id) + fail('FORBIDDEN', 'account_mismatch'); + if (input.repository.instanceUrl && input.repository.instanceUrl !== 'https://github.com') + fail('FORBIDDEN', 'repository_instance_mismatch'); + const bridge = createGitHubReviewBridge(ctx); + const [owner, repo] = input.repository.fullName.split('/'); + if (input.repository.fullName.split('/').length !== 2) + fail('BAD_REQUEST', 'invalid_repository_path'); + const expected = input.authorization?.authorizationId + ? { + kind: 'githubUser' as const, + accountId: ctx.user.id, + authorizationId: input.authorization.authorizationId, + } + : undefined; + const overview = await bridge.getReview( + { owner, repo, number: Number(input.number) }, + expected + ); + assertResolved(input, overview.identity, ctx.user.id); + return { provider: 'github' as const, bridge, overview }; + } + if (input.authorization?.kind === 'githubUser') fail('FORBIDDEN', 'owner_integration_required'); + const selected = await integrationFor(ctx, { + provider: input.repository.provider, + owner: input.authorization?.owner, + integrationId: input.authorization?.integrationId, + instanceUrl: input.repository.instanceUrl, + }); + if (input.repository.provider === 'gitlab') { + const auth = await authorizeGitLabReview({ + userId: ctx.user.id, + authorization: selected.authorization, + instanceUrl: selected.instanceUrl, + }); + const repository = await repositoryFor(input.repository, selected, auth); + const overview = await getGitLabReview(auth, repository, input.number); + assertResolved(input, overview.identity, ctx.user.id); + return { provider: 'gitlab' as const, auth, overview }; + } + const repository = await repositoryFor(input.repository, selected); + const auth = await authorizeBitbucketReview({ + userId: ctx.user.id, + authorization: selected.authorization, + repository, + }); + const overview = await getBitbucketReview(auth, input.number); + assertResolved(input, overview.identity, ctx.user.id); + return { provider: 'bitbucket' as const, auth, overview }; +} + +// Translate only sanitized adapter codes. GitHub TRPCErrors and their retry markers pass unchanged. +const procedure = baseProcedure.use(async ({ next }) => { + const result = await next(); + if (result.ok) return result; + const error = result.error.cause; + if ( + !(error instanceof GitLabInteractiveError) && + !(error instanceof BitbucketInteractiveClientError) + ) + return result; + const code = error.code; + const trpcCode = ['not_connected', 'reconnect_required', 'authentication_rejected'].includes(code) + ? 'PRECONDITION_FAILED' + : [ + 'forbidden', + 'insufficient_permissions', + 'integration_mismatch', + 'workspace_mismatch', + 'repository_mismatch', + ].includes(code) + ? 'FORBIDDEN' + : code === 'not_found' + ? 'NOT_FOUND' + : code === 'conflict' + ? 'CONFLICT' + : code === 'rate_limited' + ? 'TOO_MANY_REQUESTS' + : ['invalid_request', 'unsafe_url', 'invalid_pagination', 'request_too_large'].includes( + code + ) + ? 'BAD_REQUEST' + : 'SERVICE_UNAVAILABLE'; + throw new TRPCError({ code: trpcCode, message: code }); +}); +async function operation( + ctx: TRPCContext, + input: z.infer, + statusOnly: boolean +) { + // No text truncation and no provider call precedes admission of the serialized request size. + if (input.review.repository.provider !== 'github') { + try { + serializeReviewWriteRequest(input); + } catch { + fail('PAYLOAD_TOO_LARGE', 'request_too_large'); + } + } + const positions = [ + input.input.position, + ...(input.input.comments?.map(comment => comment.position) ?? []), + ]; + for (const position of positions) { + if (!position) continue; + if (position.native.provider !== input.review.repository.provider) + fail('BAD_REQUEST', 'position_provider_mismatch'); + if ( + JSON.stringify(ReviewRevisionSchema.parse(position.revision)) !== + JSON.stringify(ReviewRevisionSchema.parse(input.revision)) + ) + fail('CONFLICT', 'position_revision_mismatch'); + } + const selected = await target(ctx, input.review); + const { overview } = selected; + if (input.actorId !== overview.authorization.actor.id) fail('CONFLICT', 'review_actor_changed'); + const intent: ReviewIntent = { + accountId: ctx.user.id, + review: overview.identity, + actorId: input.actorId, + revision: input.revision, + input: input.input, + }; + const request = { + userId: ctx.user.id, + distinctId: ctx.user.google_user_email ?? ctx.user.id, + operationKey: input.operationKey, + intent, + }; + const result = + selected.provider === 'github' + ? await selected.bridge.runOperation(overview, intent, input.operationKey, statusOnly) + : selected.provider === 'gitlab' + ? await runGitLabReviewOperation(selected.auth, request, statusOnly) + : await runBitbucketReviewOperation( + selected.auth, + { + ...request, + intent: { + ...intent, + revision: ReviewRevisionSchema.extend({ + targetHeadSha: id, + startSha: z.null(), + }).parse(intent.revision), + }, + }, + statusOnly + ); + return { result, authorization: overview.authorization }; +} + +export const providerReviewRouter = createTRPCRouter({ + getAuthorization: procedure.input(scopeInput).query(async ({ ctx, input }) => { + if (input.provider === 'github') return createGitHubReviewBridge(ctx).getAuthorization(); + let selected: Awaited>; + try { + selected = await integrationFor(ctx, input); + } catch (error) { + if (error instanceof TRPCError && error.code === 'NOT_FOUND') + return { + status: 'not_connected' as const, + reason: 'not_connected', + authorization: null, + actor: null, + }; + throw error; + } + if (input.provider === 'gitlab') { + const auth = await authorizeGitLabReview({ + userId: ctx.user.id, + authorization: selected.authorization, + instanceUrl: selected.instanceUrl, + }); + return { + status: 'connected' as const, + reason: null, + authorization: auth.authorization, + actor: auth.actor, + }; + } + if (!input.repository) + return { + status: 'repository_required' as const, + reason: 'select_repository', + authorization: selected.authorization, + actor: null, + }; + const auth = await authorizeBitbucketReview({ + userId: ctx.user.id, + authorization: selected.authorization, + repository: await repositoryFor(input.repository, selected), + }); + return { + status: 'connected' as const, + reason: null, + authorization: auth.authorization, + actor: auth.actor, + }; + }), + resolveUrl: procedure + .input( + z.object({ + url: z.url().max(8192), + owner: ownerSchema.optional(), + integrationId: z.uuid().optional(), + accountId: id.optional(), + }) + ) + .query(async ({ ctx, input }) => { + if (input.accountId && input.accountId !== ctx.user.id) fail('FORBIDDEN', 'account_mismatch'); + if (/\\|(?:\/|%2f)(?:\.|%2e){1,2}(?:\/|%2f)/i.test(input.url)) + fail('BAD_REQUEST', 'invalid_review_url'); + const url = new URL(input.url); + if (url.username || url.password) fail('BAD_REQUEST', 'invalid_review_url'); + const platform = ['github.com', 'www.github.com'].includes(url.hostname) + ? 'github' + : url.hostname === 'bitbucket.org' + ? 'bitbucket' + : 'gitlab'; + if (url.protocol !== 'https:' && !(platform === 'github' && url.protocol === 'http:')) + fail('BAD_REQUEST', 'invalid_review_url'); + if (platform === 'github') { + if (url.port) fail('BAD_REQUEST', 'invalid_review_url'); + const match = url.pathname.match( + /^\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)\/pull\/([1-9]\d*)(?:\/(?:files|commits|checks))?\/?$/ + ); + if (!match) fail('BAD_REQUEST', 'invalid_review_path'); + return ( + await createGitHubReviewBridge(ctx).getReview({ + owner: match[1], + repo: match[2], + number: Number(match[3]), + }) + ).identity; + } + const selected = await integrationFor( + ctx, + { provider: platform, owner: input.owner, integrationId: input.integrationId }, + url + ); + const path = url.pathname.slice( + new URL(selected.instanceUrl).pathname.replace(/\/+$/, '').length + ); + const match = path.match( + platform === 'gitlab' + ? /^\/(.+)\/-\/merge_requests\/([1-9]\d*)(?:\/(?:diffs|commits|pipelines))?\/?$/ + : /^\/([^/]+\/[^/]+)\/pull-requests\/([1-9]\d*)(?:\/(?:diff|commits|activity))?\/?$/ + ); + if (!match) fail('BAD_REQUEST', 'invalid_review_path'); + let name: string; + try { + name = fullName.parse(decodeURIComponent(match[1])); + } catch { + fail('BAD_REQUEST', 'invalid_repository_path'); + } + const resolved = await target(ctx, { + repository: { provider: platform, instanceUrl: selected.instanceUrl, fullName: name }, + authorization: selected.authorization, + number: match[2], + }); + return resolved.overview.identity; + }), + getReview: procedure + .input(z.object({ review: reviewInput })) + .query(async ({ ctx, input }) => (await target(ctx, input.review)).overview), + listInbox: procedure + .input( + scopeInput.extend({ + cursor: ReviewCursorSchema.nullish(), + direction: z.enum(['forward', 'backward']).optional(), + filter: z.enum(['reviewer', 'author']).optional(), + state: z.enum(['OPEN', 'MERGED', 'DECLINED', 'SUPERSEDED']).optional(), + }) + ) + .query(async ({ ctx, input }) => { + if (input.provider === 'github') return createGitHubReviewBridge(ctx).listInbox(input.cursor); + const selected = await integrationFor(ctx, input); + if (input.provider === 'gitlab') { + const auth = await authorizeGitLabReview({ + userId: ctx.user.id, + authorization: selected.authorization, + instanceUrl: selected.instanceUrl, + }); + const repository = input.repository + ? await repositoryFor(input.repository, selected, auth) + : undefined; + return listGitLabInbox(auth, { repository, filter: input.filter, cursor: input.cursor }); + } + if (!input.repository) fail('BAD_REQUEST', 'repository_selection_required'); + const repository = await repositoryFor(input.repository, selected); + const auth = await authorizeBitbucketReview({ + userId: ctx.user.id, + authorization: selected.authorization, + repository, + }); + return listBitbucketInbox(auth, { cursor: input.cursor, state: input.state }); + }), + listFiles: procedure + .input(pageInput.extend({ revision: ReviewRevisionSchema, versionId: id.optional() })) + .query(async ({ ctx, input }) => { + const selected = await target(ctx, input.review); + const { overview } = selected; + if ( + selected.provider === 'github' && + JSON.stringify(input.revision) !== JSON.stringify(overview.revision) + ) + fail('CONFLICT', 'review_revision_changed'); + if (selected.provider !== 'gitlab' && input.versionId) + fail('BAD_REQUEST', 'diff_version_not_available'); + const result = + selected.provider === 'github' + ? await selected.bridge.listFiles(overview, input.cursor) + : selected.provider === 'gitlab' + ? await listGitLabFiles( + selected.auth, + overview.identity, + input.revision, + input.cursor, + input.versionId + ) + : await listBitbucketFiles( + selected.auth, + overview.identity, + input.revision, + input.cursor + ); + return { ...result, authorization: overview.authorization }; + }), + getFileContext: procedure + .input(z.object({ review: reviewInput, context: fileContextInput })) + .query(async ({ ctx, input }) => { + const selected = await target(ctx, input.review); + const { overview } = selected; + if (selected.provider !== 'gitlab' && input.context.versionId) + fail('BAD_REQUEST', 'diff_version_not_available'); + const result = + selected.provider === 'github' + ? await selected.bridge.getFileContext(overview, input.context) + : selected.provider === 'gitlab' + ? await getGitLabFileContext(selected.auth, overview.identity, input.context) + : await getBitbucketFileContext(selected.auth, overview.identity, input.context); + return { ...result, authorization: overview.authorization }; + }), + listChecks: procedure + .input(z.object({ review: reviewInput, revision: ReviewRevisionSchema })) + .query(async ({ ctx, input }) => { + const selected = await target(ctx, input.review); + const { overview } = selected; + if ( + selected.provider === 'github' && + JSON.stringify(input.revision) !== JSON.stringify(overview.revision) + ) + fail('CONFLICT', 'review_revision_changed'); + const checks: ReviewOverview['checks'] = + selected.provider === 'github' + ? overview.checks + : selected.provider === 'gitlab' + ? await getGitLabChecks(selected.auth, overview.identity, input.revision) + : await getBitbucketChecks(selected.auth, overview.identity, input.revision); + return { checks, authorization: overview.authorization }; + }), + listDiscussions: procedure.input(pageInput).query(async ({ ctx, input }) => { + const selected = await target(ctx, input.review); + const { overview } = selected; + const result = + selected.provider === 'github' + ? await selected.bridge.listDiscussions(overview, input.cursor) + : selected.provider === 'gitlab' + ? await listGitLabDiscussions(selected.auth, overview.identity, input.cursor) + : await listBitbucketDiscussions(selected.auth, overview.identity, input.cursor); + return { ...result, authorization: overview.authorization }; + }), + act: procedure.input(operationInput).mutation(({ ctx, input }) => operation(ctx, input, false)), + getOperationStatus: procedure + .input(operationInput) + .query(({ ctx, input }) => operation(ctx, input, true)), +}); diff --git a/apps/web/src/routers/root-router.ts b/apps/web/src/routers/root-router.ts index 4028e7c114..bca27a16a8 100644 --- a/apps/web/src/routers/root-router.ts +++ b/apps/web/src/routers/root-router.ts @@ -46,6 +46,7 @@ import { mcpGatewayRouter } from '@/routers/mcp-gateway-router'; import { mcpGatewayAuthorizationsRouter } from '@/routers/mcp-gateway-authorizations-router'; import { modelPreferencesRouter } from '@/routers/model-preferences-router'; import { githubPrReviewRouter } from '@/routers/github-pr-review-router'; +import { providerReviewRouter } from '@/routers/provider-review-router'; import { moderationRouter } from '@/routers/moderation-router'; import { userExportsRouter } from '@/routers/user-exports-router'; import { quickChatRouter } from '@/routers/quick-chat-router'; @@ -96,6 +97,7 @@ export const rootRouter = createTRPCRouter({ mcpGatewayAuthorizations: mcpGatewayAuthorizationsRouter, modelPreferences: modelPreferencesRouter, githubPrReview: githubPrReviewRouter, + providerReview: providerReviewRouter, moderation: moderationRouter, userExports: userExportsRouter, quickChat: quickChatRouter, diff --git a/packages/trpc/src/mobile.ts b/packages/trpc/src/mobile.ts index 1e4b38a503..75929a5c51 100644 --- a/packages/trpc/src/mobile.ts +++ b/packages/trpc/src/mobile.ts @@ -15,6 +15,7 @@ import { modelsRouter } from '@/routers/models-router'; import { activeSessionsRouter } from '@/routers/active-sessions-router'; import { modelPreferencesRouter } from '@/routers/model-preferences-router'; import { githubPrReviewRouter } from '@/routers/github-pr-review-router'; +import { providerReviewRouter } from '@/routers/provider-review-router'; import { moderationRouter } from '@/routers/moderation-router'; import { kiloChatRouter } from '@/routers/kilo-chat-router'; import { quickChatRouter } from '@/routers/quick-chat-router'; @@ -42,6 +43,7 @@ const mobileRouter = createTRPCRouter({ activeSessions: activeSessionsRouter, modelPreferences: modelPreferencesRouter, githubPrReview: githubPrReviewRouter, + providerReview: providerReviewRouter, moderation: moderationRouter, kiloChat: kiloChatRouter, quickChat: quickChatRouter, From 6740bbbba73bf02782cf60af71cf878c8c564767 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Mon, 31 Aug 2026 11:32:20 +0200 Subject: [PATCH 2/2] fix(provider-review): reject unbound legacy merge rows --- .../lib/provider-review/github-bridge.test.ts | 28 +++++++++++++++++++ .../src/lib/provider-review/github-bridge.ts | 2 +- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/apps/web/src/lib/provider-review/github-bridge.test.ts b/apps/web/src/lib/provider-review/github-bridge.test.ts index 84b0c5be4a..7006b8c398 100644 --- a/apps/web/src/lib/provider-review/github-bridge.test.ts +++ b/apps/web/src/lib/provider-review/github-bridge.test.ts @@ -942,6 +942,34 @@ it.each([ expect(writes).toEqual(effects); }); +it.each(['completed', 'reconcile_pending'])( + 'AC7–AC10 rejects a %s legacy merge without resource binding', + async status => { + const input = await actionInput({ action: 'merge', method: 'squash' }); + row = { + id: 'legacy-merge-row', + intent: 'merge', + resource_key: null, + status, + canonical_result: { merged: true, branchDeleted: false }, + }; + pull.state = 'closed'; + pull.merged = true; + const saved = structuredClone(row); + + await expect(facade.act(input)).rejects.toMatchObject({ + code: 'CONFLICT', + message: 'operation_key_reuse_mismatch', + }); + await expect(facade.getOperationStatus(input)).rejects.toMatchObject({ + code: 'CONFLICT', + message: 'operation_key_reuse_mismatch', + }); + expect(row).toEqual(saved); + expect(octokit.pulls.merge).not.toHaveBeenCalled(); + } +); + it.each([true, false])('AC7 recovers a lost merge response only when merged=%s', async merged => { const input = await actionInput({ action: 'merge', method: 'squash' }); octokit.pulls.merge.mockImplementation(async value => { diff --git a/apps/web/src/lib/provider-review/github-bridge.ts b/apps/web/src/lib/provider-review/github-bridge.ts index 49e877d82c..51c1a20b9e 100644 --- a/apps/web/src/lib/provider-review/github-bridge.ts +++ b/apps/web/src/lib/provider-review/github-bridge.ts @@ -698,7 +698,7 @@ export function createGitHubReviewBridge(ctx: TRPCContext) { .limit(1); // Old ledger addresses retain caller casing, unlike canonical repository metadata. // Keep those bytes until old clients/records and the 30-day ledger window expire. - const savedAddress = row?.resource_key.match( + const savedAddress = row?.resource_key?.match( /^([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)#([1-9]\d*)::[a-f0-9]{16}$/ ); if (