diff --git a/apps/mobile/src/components/pr-review/pr-review-comment-composer.test.tsx b/apps/mobile/src/components/pr-review/pr-review-comment-composer.test.tsx index ec50ea7e99..f79cb0ca9c 100644 --- a/apps/mobile/src/components/pr-review/pr-review-comment-composer.test.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-comment-composer.test.tsx @@ -1,3 +1,4 @@ +/* eslint-disable max-lines -- the composer suite covers the draft clear rules and the provider-arm anchored post in one cohesive file */ // Clear-rule coverage for the comment composer's durable draft. The composer // clears its draft on three committed outcomes — comment post, add-to-review, // and a confirmed discard — and keeps it on a dismissed-without-confirmation @@ -154,18 +155,6 @@ vi.mock('@/lib/pr-review/pending-review-provider', () => ({ })); vi.mock('@/lib/pr-review/use-pr-review-mutations', () => ({ - formatPendingCommentBody: (item: { - path: string; - line: number; - startLine?: number; - body: string; - }) => { - const location = - item.startLine !== undefined && item.startLine !== item.line - ? `${item.path}:L${item.startLine}–L${item.line}` - : `${item.path}:L${item.line}`; - return `${location}\n\n${item.body}`; - }, useCreateReviewCommentMutation: () => ({ mutateAsync: createCommentMocks.mutateAsync, isPending: createCommentMocks.isPending, @@ -269,6 +258,18 @@ describe('PrReviewCommentComposer draft clear rules', () => { footerProp(element, 'onCommentNow')?.(); await flushMicrotasks(); + // The GitHub arm keeps its exact pre-s6 variables: the position rides + // the flat input fields, never the provider `anchor` shape (c3). + expect(createCommentMocks.mutateAsync).toHaveBeenCalledWith({ + owner: 'octocat', + repo: 'hello', + number: 1, + body: 'hello', + path: 'src/a.ts', + line: 10, + side: 'RIGHT', + commitSha: 'a'.repeat(40), + }); expect(clearDraft).toHaveBeenCalledWith('u1', 'pr-comment:key'); }); @@ -303,7 +304,7 @@ describe('PrReviewCommentComposer draft clear rules', () => { describe('PrReviewCommentComposer provider arm (s6)', () => { // A GitLab MR with the same owner/repo/number triple as the GitHub - // fixtures: the folded draft key and the body-only post must differ from + // fixtures: the folded draft key and the anchored post must differ from // the GitHub arm in both bytes. const gitlabRef = { platform: 'gitlab' as const, projectPath: 'octocat/hello', mrIid: 1 }; const providerProps = { ...baseProps, prRef: gitlabRef }; @@ -320,7 +321,7 @@ describe('PrReviewCommentComposer provider arm (s6)', () => { vi.clearAllMocks(); }); - it('posts body-only through the provider arm, with the location anchored in the text', async () => { + it('posts the tapped diff position as the real anchor through the provider arm (c3)', async () => { createCommentMocks.mutateAsync.mockResolvedValueOnce({}); const element = mountProviderComposer(); typeBody(element, 'hello'); @@ -328,7 +329,25 @@ describe('PrReviewCommentComposer provider arm (s6)', () => { await flushMicrotasks(); expect(createCommentMocks.mutateAsync).toHaveBeenCalledWith({ - body: 'src/a.ts:L10\n\nhello', + body: 'hello', + anchor: { path: 'src/a.ts', side: 'RIGHT', line: 10 }, + }); + }); + + it('carries a multi-line range into the anchor (c3)', async () => { + createCommentMocks.mutateAsync.mockResolvedValueOnce({}); + // eslint-disable-next-line new-cap + const element = PrReviewCommentComposer({ + ...providerProps, + startLine: 8, + }); + typeBody(element, 'hello'); + footerProp(element, 'onCommentNow')?.(); + await flushMicrotasks(); + + expect(createCommentMocks.mutateAsync).toHaveBeenCalledWith({ + body: 'hello', + anchor: { path: 'src/a.ts', side: 'RIGHT', line: 10, startLine: 8 }, }); }); diff --git a/apps/mobile/src/components/pr-review/pr-review-comment-composer.tsx b/apps/mobile/src/components/pr-review/pr-review-comment-composer.tsx index 31bac6df50..1371ce4a39 100644 --- a/apps/mobile/src/components/pr-review/pr-review-comment-composer.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-comment-composer.tsx @@ -35,10 +35,7 @@ import { useFencedDraftLoad } from '@/lib/persist/use-draft-load'; import { buildSuggestionFence } from '@/lib/pr-review/build-suggestion-fence'; import { getDiffSelection } from '@/lib/pr-review/diff-selection-bridge'; import { usePendingReview } from '@/lib/pr-review/pending-review-provider'; -import { - formatPendingCommentBody, - useCreateReviewCommentMutation, -} from '@/lib/pr-review/use-pr-review-mutations'; +import { useCreateReviewCommentMutation } from '@/lib/pr-review/use-pr-review-mutations'; import { type ProviderPrRef, providerPrRefKey } from '@/lib/pr-review/provider-pr-ref'; type CommentComposerMode = @@ -51,10 +48,10 @@ type PrReviewCommentComposerProps = Readonly<{ number: number; /** * The provider ref (s6). Present on the GitLab/Bitbucket surface: the - * comment posts through `providerReview.addComment` with the location - * anchored in the body, and the durable comment draft key folds the ref - * identity so a same-numbered GitHub PR never shares this sheet's draft. - * Absent on GitHub, which keeps the exact pre-s6 write path. + * comment posts through `providerReview.addComment` with the tapped diff + * position as a real anchor (c3), and the durable comment draft key folds + * the ref identity so a same-numbered GitHub PR never shares this sheet's + * draft. Absent on GitHub, which keeps the exact pre-s6 write path. */ prRef?: ProviderPrRef; mode: CommentComposerMode; @@ -217,11 +214,20 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { return; } try { - // Provider arms post body-only: the inline position rides in the - // text, because the provider APIs have no comment position. + // Provider arms post the real diff position (c3): the sheet's + // path/side/line selection rides the `anchor` the provider router + // turns into an inline discussion / inline comment. await createComment.mutateAsync( prRef - ? { body: formatPendingCommentBody({ path, line, startLine, body }) } + ? { + body, + anchor: { + path, + side, + line, + ...(startLine !== undefined ? { startLine } : {}), + }, + } : { owner, repo, diff --git a/apps/mobile/src/components/pr-review/pr-review-submit.test.tsx b/apps/mobile/src/components/pr-review/pr-review-submit.test.tsx index d36c15ce72..db1c769d51 100644 --- a/apps/mobile/src/components/pr-review/pr-review-submit.test.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-submit.test.tsx @@ -27,6 +27,40 @@ vi.mock('@/lib/pr-review/use-pr-review-mutations', () => ({ isPending: false, error: submitMutationMock.error, }), + // Mirrors the real mapper (pure field routing, tested in + // use-pr-review-mutations.test.ts): anchored items ride the `comments` + // batch, an item without a side keeps the text-anchored body fallback. + buildProviderSubmitInput: ( + summary: string, + items: readonly { + path: string; + side?: 'LEFT' | 'RIGHT'; + line: number; + startLine?: number; + body: string; + }[] + ) => { + const comments: { + path: string; + side: 'LEFT' | 'RIGHT'; + line: number; + startLine?: number; + body: string; + }[] = []; + const folded: string[] = []; + for (const item of items) { + if (item.side === undefined) { + folded.push(`${item.path}:L${item.line}\n\n${item.body}`); + } else { + const { path, side, line, startLine, body } = item; + comments.push({ path, side, line, startLine, body }); + } + } + return { + body: [summary.trim(), ...folded].filter(part => part.length > 0).join('\n\n'), + comments, + }; + }, })); const footerPreferenceMock = vi.hoisted(() => ({ @@ -321,6 +355,96 @@ describe('PrReviewSubmit footer preference', () => { }); }); +// ── c3: provider arm submits anchored comments ─────────────────────── + +describe('PrReviewSubmit provider arm (c3)', () => { + const ITEM_FRESH_RANGE: PendingReviewItem = { + id: 'fresh-r', + path: 'src/r.ts', + side: 'LEFT', + line: 8, + startLine: 5, + body: 'R', + commitSha: 'head-1', + }; + + it('submits fresh pending items as a real anchored comments batch', async () => { + const renderer = mount(GITLAB_REF); + + act(() => { + addCommentFn?.(ITEM_FRESH_A); + addCommentFn?.(ITEM_FRESH_B); + addCommentFn?.(ITEM_STALE); + }); + act(() => { + submitOnPress(renderer)(); + }); + await flush(); + + // The tapped positions ride the `comments` batch (event + items, no + // summary typed); stale items are never sent and stay queued. + expect(submitMutationMock.mutateAsync).toHaveBeenCalledWith({ + event: 'comment', + comments: [ + { path: 'src/a.ts', side: 'RIGHT', line: 1, body: 'A' }, + { path: 'src/b.ts', side: 'RIGHT', line: 2, body: 'B' }, + ], + }); + expect(latestItems.map(item => item.id)).toEqual(['stale-c']); + }); + + it('carries a multi-line range into the comment item', async () => { + const renderer = mount(GITLAB_REF); + + act(() => { + addCommentFn?.(ITEM_FRESH_RANGE); + }); + act(() => { + submitOnPress(renderer, 'Submit review')(); + }); + await flush(); + + expect(submitMutationMock.mutateAsync).toHaveBeenCalledWith({ + event: 'comment', + comments: [{ path: 'src/r.ts', side: 'LEFT', line: 8, startLine: 5, body: 'R' }], + }); + }); + + it('posts the summary alone when nothing is queued (empty state, no comments key)', async () => { + footerPreferenceMock.prReviewFooter = true; + const renderer = mount(GITLAB_REF); + + act(() => { + submitOnPress(renderer, 'Submit review')(); + }); + await flush(); + + expect(submitMutationMock.mutateAsync).toHaveBeenCalledWith({ + event: 'comment', + body: '---\nReviewed via the [Kilo iOS app](https://apps.apple.com/app/id6761193135)', + }); + }); + + it('keeps the summary as the review body beside the anchored batch', async () => { + footerPreferenceMock.prReviewFooter = true; + const renderer = mount(GITLAB_REF); + + act(() => { + addCommentFn?.(ITEM_FRESH_A); + }); + act(() => { + submitOnPress(renderer, 'Submit review')(); + }); + await flush(); + + expect(submitMutationMock.mutateAsync).toHaveBeenCalledWith({ + event: 'comment', + body: '---\nReviewed via the [Kilo iOS app](https://apps.apple.com/app/id6761193135)', + comments: [{ path: 'src/a.ts', side: 'RIGHT', line: 1, body: 'A' }], + }); + }); +}); + // ── s6f: refused-submit wording ────────────────────────────────────── /** Every string the mounted tree rendered inside a Text element. */ diff --git a/apps/mobile/src/components/pr-review/pr-review-submit.tsx b/apps/mobile/src/components/pr-review/pr-review-submit.tsx index ccc554a260..123088cc14 100644 --- a/apps/mobile/src/components/pr-review/pr-review-submit.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-submit.tsx @@ -55,7 +55,7 @@ import { formatNumber } from '@/lib/format'; import { classifyPrReviewMutationError } from '@/lib/pr-review/classify-pr-review-query-state'; import { mutationErrorDisplay } from '@/lib/pr-review/mutation-error-display'; import { - buildProviderSubmitBody, + buildProviderSubmitInput, type ProviderReviewEventOption, useSubmitReviewMutation, } from '@/lib/pr-review/use-pr-review-mutations'; @@ -159,9 +159,10 @@ type PrReviewSubmitProps = Readonly<{ eyebrow: string; /** * The provider ref (s6). Present on the GitLab/Bitbucket surface: the - * submit posts one `providerReview.submitReview` event with the pending - * comments folded into the body, and the pending-comment edit route is - * the ref's own. Absent on GitHub, which keeps the exact pre-s6 path. + * submit posts one `providerReview.submitReview` event whose fresh pending + * comments ride the real inline `comments` batch (c3), and the + * pending-comment edit route is the ref's own. Absent on GitHub, which + * keeps the exact pre-s6 path. */ prRef?: ProviderPrRef; /** @@ -291,13 +292,17 @@ export function PrReviewSubmit(props: PrReviewSubmitProps) { } try { const body = bodyRef.current.trim(); - // Provider arms post one event; the summary and every fresh pending - // comment fold into the body (the provider event has no batch). + // Provider arms post one event with a real inline batch (c3): every + // fresh pending comment rides `comments` anchored at its tapped diff + // position, and the summary stays the review body. An item without an + // anchor keeps the text-anchored body fallback. + const provider = buildProviderSubmitInput(body, fresh); await submitReview.mutateAsync( prRef ? { event: EVENT_TO_PROVIDER[event], - body: buildProviderSubmitBody(body, fresh), + ...(provider.body.length > 0 ? { body: provider.body } : {}), + ...(provider.comments.length > 0 ? { comments: provider.comments } : {}), } : buildSubmitReviewInput({ owner, diff --git a/apps/mobile/src/i18n/locales/en.json b/apps/mobile/src/i18n/locales/en.json index 39f9edf0da..2aaf73a5e6 100644 --- a/apps/mobile/src/i18n/locales/en.json +++ b/apps/mobile/src/i18n/locales/en.json @@ -338,7 +338,9 @@ "banner": { "title": "Not available on this provider", "accessibility": "Not available on this provider: {{reason}}" - } + }, + "autoMergeUnavailable": "Auto-merge is not available", + "autoMergeUnavailableReason": "Bitbucket Cloud does not expose auto-merge in its API" }, "merge": { "merge": "Merge", diff --git a/apps/mobile/src/lib/pr-review/use-pr-review-mutations.test.ts b/apps/mobile/src/lib/pr-review/use-pr-review-mutations.test.ts index 7d77c13ff3..5a066b3fdd 100644 --- a/apps/mobile/src/lib/pr-review/use-pr-review-mutations.test.ts +++ b/apps/mobile/src/lib/pr-review/use-pr-review-mutations.test.ts @@ -17,6 +17,10 @@ // fingerprint pins below mirror the server's `gitlabFingerprintInput` / // `bitbucketFingerprintInput` exactly — a GitLab comment and a same-named // GitHub comment can never share a ledger key (identity rule 17). +// c3: the provider arms carry the REAL diff position — an `anchor` on +// addComment and a `comments` batch on submitReview — and the pinned +// fingerprints below mirror the server folding the anchor flat and the +// parsed batch into the ledger key. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -27,7 +31,7 @@ import { type ProviderPrRef, type ProviderPrTriple } from '@/lib/pr-review/provi import { useCreateReviewCommentMutation, useSubmitReviewMutation } from './use-pr-review-mutations'; const hoistedKeys = vi.hoisted(() => ({ - getKey: vi.fn(() => 'hoisted-op-key'), + getKey: vi.fn((_: string) => 'hoisted-op-key'), rotateKey: vi.fn(), })); @@ -330,6 +334,48 @@ describe('useCreateReviewCommentMutation (s6 gitlab arm)', () => { ); }); + it('sends the tapped diff position as the real anchor and folds it into the fingerprint (c3)', async () => { + providerAddCommentMutateMock.mockResolvedValueOnce({ done: true, replayed: false }); + useCreateReviewCommentMutation(GITLAB_REF); + + await lastCapturedOptions?.mutationFn?.({ + body: 'inline nit', + anchor: { path: 'src/a.ts', side: 'RIGHT', line: 12, startLine: 10 }, + }); + expect(providerAddCommentMutateMock).toHaveBeenCalledWith({ + ...GITLAB_IDENTITY, + body: 'inline nit', + anchor: { path: 'src/a.ts', side: 'RIGHT', line: 12, startLine: 10 }, + operationKey: 'hoisted-op-key', + }); + // Pinned bytes mirroring the server's gitlabFingerprintInput: the anchor + // folds FLAT into the s1 create_review_comment field order (body, path, + // line, side, startLine) — a retried anchored comment keeps its key. + expect(hoistedKeys.getKey).toHaveBeenCalledWith( + '{"resource":["gitlab","https://gl.example.com","group/sub/app",12],"body":"inline nit","path":"src/a.ts","line":12,"side":"RIGHT","startLine":10}' + ); + }); + + it('rotates the key when the same body moves to another line (changed intent)', async () => { + providerAddCommentMutateMock.mockResolvedValue({ done: true, replayed: false }); + useCreateReviewCommentMutation(GITLAB_REF); + + await lastCapturedOptions?.mutationFn?.({ + body: 'inline nit', + anchor: { path: 'src/a.ts', side: 'RIGHT', line: 12 }, + }); + const atLine12 = hoistedKeys.getKey.mock.calls[0]?.[0]; + await lastCapturedOptions?.mutationFn?.({ + body: 'inline nit', + anchor: { path: 'src/a.ts', side: 'RIGHT', line: 13 }, + }); + const atLine13 = hoistedKeys.getKey.mock.calls[1]?.[0]; + expect(atLine12).not.toBe(atLine13); + expect(atLine12).toBe( + '{"resource":["gitlab","https://gl.example.com","group/sub/app",12],"body":"inline nit","path":"src/a.ts","line":12,"side":"RIGHT"}' + ); + }); + it('keys a GitLab comment apart from the same GitHub comment and from the same MR on another instance', async () => { providerAddCommentMutateMock.mockResolvedValue({ done: true, replayed: false }); const onInstanceA = prIntentFingerprint('create_review_comment', { @@ -415,6 +461,25 @@ describe('useCreateReviewCommentMutation (s6 bitbucket arm)', () => { '{"resource":["bitbucket","acme","widgets",77],"body":"inline nit"}' ); }); + + it('sends the anchor through the seam with the workspace fingerprint (c3)', async () => { + providerAddCommentMutateMock.mockResolvedValueOnce({ done: true, replayed: false }); + useCreateReviewCommentMutation(BITBUCKET_REF); + + await lastCapturedOptions?.mutationFn?.({ + body: 'inline nit', + anchor: { path: 'src/a.ts', side: 'LEFT', line: 5 }, + }); + expect(providerAddCommentMutateMock).toHaveBeenCalledWith({ + ...BITBUCKET_IDENTITY, + body: 'inline nit', + anchor: { path: 'src/a.ts', side: 'LEFT', line: 5 }, + operationKey: 'hoisted-op-key', + }); + expect(hoistedKeys.getKey).toHaveBeenCalledWith( + '{"resource":["bitbucket","acme","widgets",77],"body":"inline nit","path":"src/a.ts","line":5,"side":"LEFT"}' + ); + }); }); describe('useSubmitReviewMutation (P1-A-08c wiring)', () => { @@ -550,6 +615,44 @@ describe('useSubmitReviewMutation (s6 provider arms)', () => { ); }); + it('gitlab: sends the anchored comments batch and folds it into the fingerprint (c3)', async () => { + scopeOverride = { ref: GITLAB_REF, organizationId: 'org-9' }; + providerSubmitReviewMutateMock.mockResolvedValueOnce({ done: true, replayed: false }); + useSubmitReviewMutation(GITLAB_REF); + + const comments = [ + { path: 'src/a.ts', side: 'RIGHT' as const, line: 10, body: 'first' }, + { path: 'src/b.ts', side: 'LEFT' as const, line: 2, startLine: 1, body: 'second' }, + ]; + await lastCapturedOptions?.mutationFn?.({ event: 'approve', body: 'LGTM', comments }); + expect(providerSubmitReviewMutateMock).toHaveBeenCalledWith({ + ...GITLAB_IDENTITY, + event: 'approve', + body: 'LGTM', + comments, + operationKey: 'hoisted-op-key', + }); + // Pinned bytes mirroring the server's submitReview fingerprint input: + // the batch serializes in the router's field order (path, side, line, + // startLine, body), so a retried batch keeps its ledger identity. + expect(hoistedKeys.getKey).toHaveBeenCalledWith( + '{"resource":["gitlab","https://gl.example.com","group/sub/app",12],"event":"approve","body":"LGTM","comments":[{"path":"src/a.ts","side":"RIGHT","line":10,"body":"first"},{"path":"src/b.ts","side":"LEFT","line":2,"startLine":1,"body":"second"}]}' + ); + }); + + it('gitlab: an empty comments batch is no batch — pre-c3 bytes unchanged (c3)', async () => { + scopeOverride = { ref: GITLAB_REF, organizationId: 'org-9' }; + providerSubmitReviewMutateMock.mockResolvedValueOnce({ done: true, replayed: false }); + useSubmitReviewMutation(GITLAB_REF); + + await lastCapturedOptions?.mutationFn?.({ event: 'approve', body: 'LGTM', comments: [] }); + const sent = providerSubmitReviewMutateMock.mock.calls[0]?.[0] as Record; + expect(sent).not.toHaveProperty('comments'); + expect(hoistedKeys.getKey).toHaveBeenCalledWith( + '{"resource":["gitlab","https://gl.example.com","group/sub/app",12],"event":"approve","body":"LGTM"}' + ); + }); + it('bitbucket: routes request-changes through the seam with the workspace fingerprint', async () => { scopeOverride = { ref: BITBUCKET_REF, organizationId: 'org-9' }; providerSubmitReviewMutateMock.mockResolvedValueOnce({ done: true, replayed: false }); @@ -621,7 +724,7 @@ describe('submit_review fingerprint (P1-A-08c changed-input)', () => { }); }); -describe('provider pending-comment body builders (s6)', () => { +describe('provider pending-comment body builders (s6, c3)', () => { it('formatPendingCommentBody anchors a single-line position like the pending list shows it', async () => { const { formatPendingCommentBody } = await import('./use-pr-review-mutations'); expect(formatPendingCommentBody({ path: 'src/a.ts', line: 10, body: 'note' })).toBe( @@ -631,18 +734,36 @@ describe('provider pending-comment body builders (s6)', () => { formatPendingCommentBody({ path: 'src/a.ts', line: 12, startLine: 10, body: 'range' }) ).toBe('src/a.ts:L10–L12\n\nrange'); }); - it('buildProviderSubmitBody folds the summary and fresh comments into one body', async () => { - const { buildProviderSubmitBody } = await import('./use-pr-review-mutations'); + it('buildProviderSubmitInput sends anchored items as a real comments batch (c3)', async () => { + const { buildProviderSubmitInput } = await import('./use-pr-review-mutations'); + expect( + buildProviderSubmitInput('Looks good overall.', [ + { path: 'src/a.ts', side: 'RIGHT' as const, line: 10, body: 'first' }, + { path: 'src/b.ts', side: 'LEFT' as const, line: 2, startLine: 1, body: 'second' }, + ]) + ).toEqual({ + body: 'Looks good overall.', + comments: [ + { path: 'src/a.ts', side: 'RIGHT', line: 10, body: 'first' }, + { path: 'src/b.ts', side: 'LEFT', line: 2, startLine: 1, body: 'second' }, + ], + }); + }); + it('buildProviderSubmitInput keeps the text-anchored body for items without an anchor (c3)', async () => { + const { buildProviderSubmitInput } = await import('./use-pr-review-mutations'); expect( - buildProviderSubmitBody('Looks good overall.', [ - { path: 'src/a.ts', line: 10, body: 'first' }, - { path: 'src/b.ts', line: 2, startLine: 1, body: 'second' }, + buildProviderSubmitInput('Summary.', [ + { path: 'src/a.ts', line: 10, body: 'folded' }, + { path: 'src/b.ts', side: 'RIGHT' as const, line: 2, body: 'anchored' }, ]) - ).toBe('Looks good overall.\n\nsrc/a.ts:L10\n\nfirst\n\nsrc/b.ts:L1–L2\n\nsecond'); + ).toEqual({ + body: 'Summary.\n\nsrc/a.ts:L10\n\nfolded', + comments: [{ path: 'src/b.ts', side: 'RIGHT', line: 2, body: 'anchored' }], + }); }); - it('buildProviderSubmitBody drops empty parts: a summary-only approve posts exactly the summary', async () => { - const { buildProviderSubmitBody } = await import('./use-pr-review-mutations'); - expect(buildProviderSubmitBody(' LGTM ', [])).toBe('LGTM'); - expect(buildProviderSubmitBody('', [])).toBe(''); + it('buildProviderSubmitInput drops empty parts: a summary-only approve posts exactly the summary', async () => { + const { buildProviderSubmitInput } = await import('./use-pr-review-mutations'); + expect(buildProviderSubmitInput(' LGTM ', [])).toEqual({ body: 'LGTM', comments: [] }); + expect(buildProviderSubmitInput('', [])).toEqual({ body: '', comments: [] }); }); }); diff --git a/apps/mobile/src/lib/pr-review/use-pr-review-mutations.ts b/apps/mobile/src/lib/pr-review/use-pr-review-mutations.ts index 2fbedfd5b1..cae7de5ae1 100644 --- a/apps/mobile/src/lib/pr-review/use-pr-review-mutations.ts +++ b/apps/mobile/src/lib/pr-review/use-pr-review-mutations.ts @@ -16,12 +16,12 @@ // uses the LATEST head SHA (per the S3 contract) regardless of what // SHA each item was queued under; a per-item 422 surfaces inline. // -// Provider arms (s6): a GitLab MR or Bitbucket PR posts through -// `providerReview.addComment` / `providerReview.submitReview`. Those -// procedures carry NO inline position and NO comment batch — the -// provider APIs have none — so the composer anchors the position in -// the body text and the submit sheet folds the queued comments into -// the review summary (see `formatPendingCommentBody`). The GitHub arms +// Provider arms (s6, anchored by c3): a GitLab MR or Bitbucket PR posts +// through `providerReview.addComment` / `providerReview.submitReview` with +// the REAL diff position (`anchor: { path, side, line, startLine? }`) and a +// real `comments` batch, so an inline comment lands on the line the user +// tapped. `formatPendingCommentBody` stays as the no-anchor fallback: a +// position-less body keeps the `path:L10–L20` text anchor. The GitHub arms // stay byte-identical: same procedures, same inputs, same fingerprints. // // P1-A-08c: both hooks hoist one operation key per intent, so retries of the @@ -181,15 +181,26 @@ type CreateReviewCommentInput = RouterInputs['githubPrReview']['createReviewComm export type SubmitReviewInput = RouterInputs['githubPrReview']['submitReview']; export type SubmitReviewComment = NonNullable[number]; -/** The provider arms accept a body only — no inline position, no batch. */ -type ProviderCommentBody = { body: string }; +/** + * The real diff position a provider comment anchors to (c3): the line the + * user tapped, with `startLine` marking the first line of a multi-line + * range ending at `line`. Absent, the comment is a top-level note. + */ +export type ProviderCommentAnchor = { + path: string; + side: 'LEFT' | 'RIGHT'; + line: number; + startLine?: number; +}; + +/** The provider arms accept a body plus the optional real diff anchor. */ +type ProviderCommentBody = { body: string; anchor?: ProviderCommentAnchor }; export type CreateReviewCommentVars = CreateReviewCommentInput | ProviderCommentBody; /** - * The body a provider comment carries: GitLab and Bitbucket have no inline - * comment position, so the composer (direct post) and the submit sheet - * (pending comments folded into the review summary) anchor the location in - * the text itself, in the same `path:L10–L20` format the pending list shows. + * The no-anchor fallback body: when the composer has no real diff position, + * the location rides in the text itself, in the same `path:L10–L20` format + * the pending list shows. */ export function formatPendingCommentBody(item: { path: string; @@ -204,24 +215,41 @@ export function formatPendingCommentBody(item: { return `${location}\n\n${item.body}`; } +/** One anchored item in a provider `submitReview` batch (c3). */ +export type ProviderSubmitReviewComment = ProviderCommentAnchor & { body: string }; + /** - * The body a provider `submitReview` carries: the summary, then every fresh - * pending comment anchored in the text (the provider event has no comment - * batch). Empty parts are dropped, so an approve with a summary alone posts - * exactly the summary and an approve with neither posts nothing. + * The provider `submitReview` intent the submit sheet builds: every pending + * item with a side rides the real `comments` batch; an item without one + * keeps the text-anchored body (the no-anchor fallback). Empty body parts + * are dropped, so an approve with a summary alone posts exactly the summary + * and an approve with neither posts nothing. Comment items keep the router's + * field order (path, side, line, startLine, body): the server hashes the + * parsed batch into the `submit_review` fingerprint, and a key drift rotates + * the dedupe identity. */ -export function buildProviderSubmitBody( +export function buildProviderSubmitInput( summary: string, items: readonly { path: string; + side?: 'LEFT' | 'RIGHT'; line: number; startLine?: number; body: string; }[] -): string { - return [summary.trim(), ...items.map(item => formatPendingCommentBody(item))] - .filter(part => part.length > 0) - .join('\n\n'); +) { + const comments: ProviderSubmitReviewComment[] = []; + const folded: string[] = []; + for (const item of items) { + if (item.side === undefined) { + folded.push(formatPendingCommentBody(item)); + } else { + const { path, side, line, startLine, body } = item; + comments.push({ path, side, line, ...(startLine !== undefined ? { startLine } : {}), body }); + } + } + const body = [summary.trim(), ...folded].filter(part => part.length > 0).join('\n\n'); + return { body, comments }; } /** The events the `providerReview.submitReview` input accepts. */ @@ -229,6 +257,7 @@ export type ProviderReviewEventOption = 'approve' | 'request_changes' | 'comment type ProviderSubmitReviewVars = { event: ProviderReviewEventOption; body?: string; + comments?: ProviderSubmitReviewComment[]; }; export type SubmitReviewVars = SubmitReviewInput | ProviderSubmitReviewVars; @@ -251,16 +280,26 @@ export function useCreateReviewCommentMutation(ref: ReviewWriteRef) { return result; } const vars = input as ProviderCommentBody; - const result = await trpcClient.providerReview.addComment.mutate({ + // The anchor rides the input AND the fingerprint: path/line/side/ + // startLine are s1 create_review_comment fields, so a retried + // anchored comment dedupes and a moved position starts a fresh + // intent. The anchor keys are exactly the fingerprint field names, + // so the spread folds them FLAT the way the server's + // gitlabFingerprintInput / bitbucketFingerprintInput does. Hoisted + // off the call so the extra fields compile against the router input + // type the client is typed against (provider-review-router.ts). + const addCommentInput = { ...providerWriteIdentity(scope), body: vars.body, + ...(vars.anchor ? { anchor: vars.anchor } : {}), operationKey: getKey( prIntentFingerprint( 'create_review_comment', - providerFingerprintInput(scope.ref, { body: vars.body }) + providerFingerprintInput(scope.ref, { body: vars.body, ...vars.anchor }) ) ), - }); + }; + const result = await trpcClient.providerReview.addComment.mutate(addCommentInput); rotateKey(); return result; } catch (error) { @@ -304,17 +343,26 @@ export function useSubmitReviewMutation(ref: ReviewWriteRef) { return result; } const vars = input as ProviderSubmitReviewVars; - const result = await trpcClient.providerReview.submitReview.mutate({ + // An empty batch is no batch: the key and the input omit `comments` + // together, so an event-only review keeps its pre-c3 bytes. The + // batch rides the fingerprint exactly as the server folds the parsed + // `comments` into the submit_review fingerprint. Hoisted off the + // call so the batch compiles against the router input type the + // client is typed against (provider-review-router.ts). + const comments = vars.comments?.length ? vars.comments : undefined; + const submitReviewInput = { ...providerWriteIdentity(scope), event: vars.event, ...(vars.body !== undefined && vars.body.length > 0 ? { body: vars.body } : {}), + ...(comments ? { comments } : {}), operationKey: getKey( prIntentFingerprint( 'submit_review', - providerFingerprintInput(scope.ref, { event: vars.event, body: vars.body }) + providerFingerprintInput(scope.ref, { event: vars.event, body: vars.body, comments }) ) ), - }); + }; + const result = await trpcClient.providerReview.submitReview.mutate(submitReviewInput); rotateKey(); return result; } catch (error) { diff --git a/apps/mobile/src/lib/universal-link-paths.js b/apps/mobile/src/lib/universal-link-paths.js index 7ebf68c6c3..3cd5db6d1d 100644 --- a/apps/mobile/src/lib/universal-link-paths.js +++ b/apps/mobile/src/lib/universal-link-paths.js @@ -20,5 +20,7 @@ export const UNIVERSAL_LINK_PATH_PATTERNS = [ '/organizations/.*/code-reviews', '/organizations/.*/code-reviews/.*', '/organizations/.*/overview', + '/pr-review/gitlab/.*', + '/pr-review/bitbucket/.*', '/pr-review/.*/.*/.*', ]; diff --git a/apps/web/public/.well-known/apple-app-site-association b/apps/web/public/.well-known/apple-app-site-association index 5fc6240f18..e92f38c743 100644 --- a/apps/web/public/.well-known/apple-app-site-association +++ b/apps/web/public/.well-known/apple-app-site-association @@ -21,6 +21,8 @@ { "/": "/organizations/*/code-reviews/review-md", "exclude": true }, { "/": "/organizations/*/code-reviews/*" }, { "/": "/organizations/*/overview" }, + { "/": "/pr-review/gitlab/**" }, + { "/": "/pr-review/bitbucket/**" }, { "/": "/pr-review/*/*/*" } ] } diff --git a/apps/web/src/lib/provider-review/bitbucket-write.test.ts b/apps/web/src/lib/provider-review/bitbucket-write.test.ts index 4a2ac6ea13..e83eb45208 100644 --- a/apps/web/src/lib/provider-review/bitbucket-write.test.ts +++ b/apps/web/src/lib/provider-review/bitbucket-write.test.ts @@ -43,7 +43,10 @@ const ORG_OWNER = { userId: 'user_1', }; -const WORKSPACE = { uuid: '12345678-1234-1234-1234-123456789012', slug: 'acme' }; +const WORKSPACE = { + uuid: '12345678-1234-1234-1234-123456789012', + slug: 'acme', +}; const HEAD_SHA = 'abc123def4567890'; @@ -97,7 +100,11 @@ beforeEach(() => { fetchMock.mockImplementation(async (url: string | URL) => { const parsed = new URL(url.toString()); if (url.toString().includes('token-service.example.com')) { - return jsonResponse({ status: 'available', token: 'at-mock-token', workspace: WORKSPACE }); + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); } if (parsed.pathname === '/2.0/user') { return jsonResponse({ uuid: '{current-user-uuid}' }); @@ -147,14 +154,119 @@ describe('addComment', () => { const calls = bitbucketCalls(); const post = calls.find(call => call.init.method === 'POST'); expect(post?.url.pathname).toBe('/2.0/repositories/acme/repo/pullrequests/12/comments'); - expect(JSON.parse(String(post?.init.body))).toEqual({ content: { raw: 'A review comment' } }); + expect(JSON.parse(String(post?.init.body))).toEqual({ + content: { raw: 'A review comment' }, + }); + }); + + it('a RIGHT anchor posts an inline comment anchored on the destination line', async () => { + await addComment({ + owner: ORG_OWNER, + workspace: 'acme', + repoSlug: 'repo', + prId: 12, + body: 'Inline on the new side', + anchor: { path: 'src/deploy.ts', side: 'RIGHT', line: 42 }, + }); + + const post = bitbucketCalls().find(call => call.init.method === 'POST'); + expect(JSON.parse(String(post?.init.body))).toEqual({ + content: { raw: 'Inline on the new side' }, + inline: { path: 'src/deploy.ts', to: 42 }, + }); + }); + + it('a LEFT anchor posts an inline comment anchored on the source line', async () => { + await addComment({ + owner: ORG_OWNER, + workspace: 'acme', + repoSlug: 'repo', + prId: 12, + body: 'Inline on the old side', + anchor: { path: 'src/deploy.ts', side: 'LEFT', line: 7 }, + }); + + const post = bitbucketCalls().find(call => call.init.method === 'POST'); + expect(JSON.parse(String(post?.init.body))).toEqual({ + content: { raw: 'Inline on the old side' }, + inline: { path: 'src/deploy.ts', from: 7 }, + }); + }); + + it('a RIGHT startLine range anchors the destination line, never an unrelated source line', async () => { + await addComment({ + owner: ORG_OWNER, + workspace: 'acme', + repoSlug: 'repo', + prId: 12, + body: 'This block', + anchor: { path: 'src/deploy.ts', side: 'RIGHT', line: 20, startLine: 10 }, + }); + + const post = bitbucketCalls().find(call => call.init.method === 'POST'); + // Bitbucket's from/to are source-side and destination-side line numbers, + // not a one-sided range: `from: startLine` would anchor an unrelated old + // line. The range anchors its end line on the tapped (destination) side. + expect(JSON.parse(String(post?.init.body))).toEqual({ + content: { raw: 'This block' }, + inline: { path: 'src/deploy.ts', to: 20 }, + }); + }); + + it('a LEFT startLine range anchors the source line, never an invented new-side line', async () => { + await addComment({ + owner: ORG_OWNER, + workspace: 'acme', + repoSlug: 'repo', + prId: 12, + body: 'This block', + anchor: { path: 'src/deploy.ts', side: 'LEFT', line: 20, startLine: 10 }, + }); + + const post = bitbucketCalls().find(call => call.init.method === 'POST'); + expect(JSON.parse(String(post?.init.body))).toEqual({ + content: { raw: 'This block' }, + inline: { path: 'src/deploy.ts', from: 20 }, + }); + }); + + it('classifies a provider 400 on an anchored comment as bad_request', async () => { + fetchMock.mockImplementation(async (url: string | URL) => { + const full = url.toString(); + if (full.includes('token-service.example.com')) { + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); + } + return new Response(null, { status: 400 }); + }); + + const error = await captureRejection( + addComment({ + owner: ORG_OWNER, + workspace: 'acme', + repoSlug: 'repo', + prId: 12, + body: 'x', + anchor: { path: 'src/deploy.ts', side: 'RIGHT', line: 999_999 }, + }) + ); + + expect(error.kind).toBe('bad_request'); + expect(error.retryable).toBe(false); }); it('maps a provider 403 to non-retryable forbidden', async () => { fetchMock.mockImplementation(async (url: string | URL) => { const full = url.toString(); if (full.includes('token-service.example.com')) { - return jsonResponse({ status: 'available', token: 'at-mock-token', workspace: WORKSPACE }); + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); } if (new URL(full).pathname === '/2.0/user') return jsonResponse({ uuid: '{u}' }); return new Response(null, { status: 403 }); @@ -247,7 +359,9 @@ describe('submitReview', () => { const put = bitbucketCalls().find( call => call.init.method === 'PUT' && call.url.pathname.includes('/participants/') ); - expect(JSON.parse(String(put?.init.body))).toEqual({ state: 'changes_requested' }); + expect(JSON.parse(String(put?.init.body))).toEqual({ + state: 'changes_requested', + }); }); it('maps comment to clearing the own approval state and posts the body', async () => { @@ -266,7 +380,9 @@ describe('submitReview', () => { ); expect(JSON.parse(String(put?.init.body))).toEqual({ state: null }); const post = bitbucketCalls().find(call => call.init.method === 'POST'); - expect(JSON.parse(String(post?.init.body))).toEqual({ content: { raw: 'Read this first' } }); + expect(JSON.parse(String(post?.init.body))).toEqual({ + content: { raw: 'Read this first' }, + }); }); it('refuses a comment review without a body before any provider call', async () => { @@ -283,6 +399,201 @@ describe('submitReview', () => { expect(error.kind).toBe('bad_request'); expect(bitbucketCalls()).toEqual([]); }); + + it('posts every inline comment before the review state and the summary comment', async () => { + const result = await submitReview({ + owner: ORG_OWNER, + workspace: 'acme', + repoSlug: 'repo', + prId: 12, + event: 'approve', + body: 'LGTM', + comments: [ + { path: 'a.ts', side: 'RIGHT', line: 3, body: 'first inline' }, + { + path: 'b.ts', + side: 'LEFT', + line: 9, + startLine: 4, + body: 'second inline', + }, + ], + }); + + expect(result).toEqual({ done: true, replayed: false }); + const effects = bitbucketCalls().filter( + call => call.init.method === 'POST' || call.init.method === 'PUT' + ); + expect(effects.map(call => `${String(call.init.method)} ${call.url.pathname}`)).toEqual([ + 'POST /2.0/repositories/acme/repo/pullrequests/12/comments', + 'POST /2.0/repositories/acme/repo/pullrequests/12/comments', + 'PUT /2.0/repositories/acme/repo/pullrequests/12/participants/%7Bcurrent-user-uuid%7D', + 'POST /2.0/repositories/acme/repo/pullrequests/12/comments', + ]); + expect(JSON.parse(String(effects[0].init.body))).toEqual({ + content: { raw: 'first inline' }, + inline: { path: 'a.ts', to: 3 }, + }); + expect(JSON.parse(String(effects[1].init.body))).toEqual({ + content: { raw: 'second inline' }, + // A LEFT range anchors its end line on the source side. + inline: { path: 'b.ts', from: 9 }, + }); + expect(JSON.parse(String(effects[3].init.body))).toEqual({ + content: { raw: 'LGTM' }, + }); + }); + + it('a comment event with a batch and no body still posts the inline comments and clears approval', async () => { + const result = await submitReview({ + owner: ORG_OWNER, + workspace: 'acme', + repoSlug: 'repo', + prId: 12, + event: 'comment', + comments: [{ path: 'a.ts', side: 'RIGHT', line: 3, body: 'inline only' }], + }); + + expect(result).toEqual({ done: true, replayed: false }); + const effects = bitbucketCalls().filter( + call => call.init.method === 'POST' || call.init.method === 'PUT' + ); + expect(effects).toHaveLength(2); + expect(JSON.parse(String(effects[0].init.body))).toEqual({ + content: { raw: 'inline only' }, + inline: { path: 'a.ts', to: 3 }, + }); + expect(JSON.parse(String(effects[1].init.body))).toEqual({ state: null }); + }); + + it('a mid-batch rejection after a committed comment reports the ambiguous retryable kind', async () => { + let posts = 0; + fetchMock.mockImplementation(async (url: string | URL) => { + const full = url.toString(); + if (full.includes('token-service.example.com')) { + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); + } + const parsed = new URL(full); + if (parsed.pathname === '/2.0/user') return jsonResponse({ uuid: '{current-user-uuid}' }); + if (parsed.pathname.endsWith('/comments')) { + posts += 1; + return posts === 1 + ? jsonResponse({ id: 101 }) + : jsonResponse({ error: { message: 'inline position invalid' } }, 400); + } + return jsonResponse({}); + }); + + const error = await captureRejection( + submitReview({ + owner: ORG_OWNER, + workspace: 'acme', + repoSlug: 'repo', + prId: 12, + event: 'approve', + body: 'LGTM', + comments: [ + { path: 'a.ts', side: 'RIGHT', line: 3, body: 'first' }, + { path: 'b.ts', side: 'RIGHT', line: 4, body: 'outside' }, + ], + }) + ); + + // The first inline comment already committed: a deterministic + // bad_request would settle the ledger row failed, and the client's + // key-rotating retry would re-post that comment as a duplicate. The + // retryable kind keeps the row reconcile_pending instead. + expect(error.kind).toBe('retryable'); + expect(error.retryable).toBe(true); + // The failure stops the batch: no participants write, no summary comment. + expect(bitbucketCalls().some(call => call.init.method === 'PUT')).toBe(false); + expect(posts).toBe(2); + }); + + it('a rejection on the first comment, with nothing committed, keeps the deterministic bad_request', async () => { + let posts = 0; + fetchMock.mockImplementation(async (url: string | URL) => { + const full = url.toString(); + if (full.includes('token-service.example.com')) { + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); + } + const parsed = new URL(full); + if (parsed.pathname === '/2.0/user') return jsonResponse({ uuid: '{current-user-uuid}' }); + if (parsed.pathname.endsWith('/comments')) { + posts += 1; + return jsonResponse({ error: { message: 'inline position invalid' } }, 400); + } + return jsonResponse({}); + }); + + const error = await captureRejection( + submitReview({ + owner: ORG_OWNER, + workspace: 'acme', + repoSlug: 'repo', + prId: 12, + event: 'approve', + body: 'LGTM', + comments: [ + { path: 'a.ts', side: 'RIGHT', line: 999_999, body: 'outside' }, + { path: 'b.ts', side: 'RIGHT', line: 4, body: 'second' }, + ], + }) + ); + + expect(error.kind).toBe('bad_request'); + expect(error.retryable).toBe(false); + // The batch stops at the refused comment: the second never posts. + expect(posts).toBe(1); + }); + + it('a participants-write rejection after the whole batch committed is a partial apply too', async () => { + fetchMock.mockImplementation(async (url: string | URL) => { + const full = url.toString(); + if (full.includes('token-service.example.com')) { + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); + } + const parsed = new URL(full); + if (parsed.pathname === '/2.0/user') return jsonResponse({ uuid: '{current-user-uuid}' }); + if (parsed.pathname.includes('/participants/')) { + return jsonResponse({ error: { message: 'forbidden' } }, 403); + } + return jsonResponse({ id: 101 }); + }); + + const error = await captureRejection( + submitReview({ + owner: ORG_OWNER, + workspace: 'acme', + repoSlug: 'repo', + prId: 12, + event: 'approve', + body: 'LGTM', + comments: [{ path: 'a.ts', side: 'RIGHT', line: 3, body: 'first' }], + }) + ); + + // The inline comment committed before the review state was refused: a + // failed settle would let the retry re-post it as a duplicate. + expect(error.kind).toBe('retryable'); + expect(error.retryable).toBe(true); + // The summary comment never posts. + expect( + bitbucketCalls().filter(call => String(call.url.pathname).endsWith('/comments')) + ).toHaveLength(1); + }); }); describe('resolveThread', () => { @@ -305,7 +616,11 @@ describe('resolveThread', () => { fetchMock.mockImplementation(async (url: string | URL) => { const full = url.toString(); if (full.includes('token-service.example.com')) { - return jsonResponse({ status: 'available', token: 'at-mock-token', workspace: WORKSPACE }); + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); } const parsed = new URL(full); if (parsed.pathname.endsWith('/comments/101')) return jsonResponse({ id: 101 }); @@ -330,7 +645,11 @@ describe('resolveThread', () => { fetchMock.mockImplementation(async (url: string | URL) => { const full = url.toString(); if (full.includes('token-service.example.com')) { - return jsonResponse({ status: 'available', token: 'at-mock-token', workspace: WORKSPACE }); + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); } const parsed = new URL(full); if (parsed.pathname === '/2.0/user') return jsonResponse({ uuid: '{u}' }); @@ -364,7 +683,11 @@ describe('resolveThread', () => { fetchMock.mockImplementation(async (url: string | URL) => { const full = url.toString(); if (full.includes('token-service.example.com')) { - return jsonResponse({ status: 'available', token: 'at-mock-token', workspace: WORKSPACE }); + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); } const parsed = new URL(full); if (parsed.pathname === '/2.0/user') return jsonResponse({ uuid: '{u}' }); @@ -391,7 +714,11 @@ describe('resolveThread', () => { fetchMock.mockImplementation(async (url: string | URL) => { const full = url.toString(); if (full.includes('token-service.example.com')) { - return jsonResponse({ status: 'available', token: 'at-mock-token', workspace: WORKSPACE }); + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); } const parsed = new URL(full); if (parsed.pathname === '/2.0/user') return jsonResponse({ uuid: '{u}' }); @@ -399,7 +726,13 @@ describe('resolveThread', () => { if (parsed.pathname.endsWith('/tasks')) { return jsonResponse({ pagelen: 100, - values: [{ id: 7, resolved_on: '2026-09-06T00:00:00.000Z', comment: { id: 101 } }], + values: [ + { + id: 7, + resolved_on: '2026-09-06T00:00:00.000Z', + comment: { id: 101 }, + }, + ], next: null, }); } @@ -422,7 +755,11 @@ describe('resolveThread', () => { fetchMock.mockImplementation(async (url: string | URL) => { const full = url.toString(); if (full.includes('token-service.example.com')) { - return jsonResponse({ status: 'available', token: 'at-mock-token', workspace: WORKSPACE }); + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); } const parsed = new URL(full); if (parsed.pathname.endsWith('/comments/101')) return jsonResponse({ id: 101 }); @@ -462,7 +799,11 @@ describe('resolveThread', () => { fetchMock.mockImplementation(async (url: string | URL) => { const full = url.toString(); if (full.includes('token-service.example.com')) { - return jsonResponse({ status: 'available', token: 'at-mock-token', workspace: WORKSPACE }); + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); } const parsed = new URL(full); if (parsed.pathname.endsWith('/comments/101')) return jsonResponse({ id: 101 }); @@ -470,7 +811,13 @@ describe('resolveThread', () => { return parsed.searchParams.get('page') === '2' ? jsonResponse({ pagelen: 100, - values: [{ id: 7, resolved_on: '2026-09-06T00:00:00.000Z', comment: { id: 101 } }], + values: [ + { + id: 7, + resolved_on: '2026-09-06T00:00:00.000Z', + comment: { id: 101 }, + }, + ], next: null, }) : jsonResponse({ @@ -551,11 +898,19 @@ describe('mergePullRequest', () => { fetchMock.mockImplementation(async (url: string | URL) => { const full = url.toString(); if (full.includes('token-service.example.com')) { - return jsonResponse({ status: 'available', token: 'at-mock-token', workspace: WORKSPACE }); + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); } const parsed = new URL(full); if (parsed.pathname.endsWith('/pullrequests/12')) { - return jsonResponse({ id: 12, state: 'MERGED', source: { commit: { hash: HEAD_SHA } } }); + return jsonResponse({ + id: 12, + state: 'MERGED', + source: { commit: { hash: HEAD_SHA } }, + }); } return jsonResponse({ pagelen: 50, values: [], next: null }); }); @@ -576,11 +931,19 @@ describe('mergePullRequest', () => { fetchMock.mockImplementation(async (url: string | URL) => { const full = url.toString(); if (full.includes('token-service.example.com')) { - return jsonResponse({ status: 'available', token: 'at-mock-token', workspace: WORKSPACE }); + return jsonResponse({ + status: 'available', + token: 'at-mock-token', + workspace: WORKSPACE, + }); } const parsed = new URL(full); if (parsed.pathname.endsWith('/pullrequests/12')) { - return jsonResponse({ id: 12, state: 'DECLINED', source: { commit: { hash: HEAD_SHA } } }); + return jsonResponse({ + id: 12, + state: 'DECLINED', + source: { commit: { hash: HEAD_SHA } }, + }); } return jsonResponse({ pagelen: 50, values: [], next: null }); }); diff --git a/apps/web/src/lib/provider-review/bitbucket-write.ts b/apps/web/src/lib/provider-review/bitbucket-write.ts index c4c557e41f..9a49d45b02 100644 --- a/apps/web/src/lib/provider-review/bitbucket-write.ts +++ b/apps/web/src/lib/provider-review/bitbucket-write.ts @@ -12,7 +12,11 @@ import 'server-only'; import { z } from 'zod'; -import type { ProviderReviewCapabilities } from '@kilocode/app-shared/provider-review'; +import type { + ProviderReviewCapabilities, + ProviderReviewInlineAnchor, + ProviderReviewInlineComment, +} from '@kilocode/app-shared/provider-review'; import { BITBUCKET_REVIEW_CAPABILITIES } from '@kilocode/app-shared/provider-review'; import { authorizeRepository, @@ -126,15 +130,40 @@ async function ownAccountId(access: BitbucketRepositoryAccess): Promise return user.uuid; } -/** Post a top-level comment on the pull request. */ +/** + * The Bitbucket `inline` block for one anchor: RIGHT anchors the destination + * line (`to`), LEFT the source line (`from`). `from`/`to` are source-side and + * destination-side line numbers, not a one-sided range — sending + * `from: startLine` for a RIGHT range would anchor an unrelated source line, + * and a LEFT range would invent a new-side line. So a `startLine` range + * anchors its end line on the tapped side; the range stays in the pending + * list and the ledger key, not in the provider position. + */ +function buildInlinePosition(anchor: ProviderReviewInlineAnchor): Record { + return anchor.side === 'RIGHT' + ? { path: anchor.path, to: anchor.line } + : { path: anchor.path, from: anchor.line }; +} + +/** + * Post a comment on the pull request. With an `anchor` this creates a real + * inline comment on the diff position; without one it posts a top-level + * comment, byte-identical to the previous behavior. + */ export async function addComment( - target: BitbucketPrTarget & { body: string } & BitbucketMutationInput + target: BitbucketPrTarget & { + body: string; + anchor?: ProviderReviewInlineAnchor; + } & BitbucketMutationInput ): Promise { const access = await targetAccess(target); try { await requestBitbucketJson(access, `${prPath(access, target.prId)}/comments`, { method: 'POST', - body: { content: { raw: target.body } }, + body: { + content: { raw: target.body }, + ...(target.anchor ? { inline: buildInlinePosition(target.anchor) } : {}), + }, }); return { done: true, replayed: false }; } catch (error) { @@ -144,7 +173,10 @@ export async function addComment( /** Reply inside an existing comment thread. */ export async function replyToComment( - target: BitbucketPrTarget & { commentId: string; body: string } & BitbucketMutationInput + target: BitbucketPrTarget & { + commentId: string; + body: string; + } & BitbucketMutationInput ): Promise { const parentId = Number(target.commentId); if (!Number.isInteger(parentId) || parentId <= 0) { @@ -162,23 +194,58 @@ export async function replyToComment( } } +/** + * The partial-apply reason: an inline comment committed before a later + * rejection, so the provider already holds effects a replayed batch would + * duplicate. The retryable kind keeps the router's ledger row + * reconcile_pending instead of settling it failed. + */ +const BITBUCKET_INLINE_PARTIAL_APPLY_REASON = + 'Bitbucket applied part of this review before the request failed. Check the pull request before retrying.'; + /** * Submit a review. `approve` → PUT participants/{account_id} with * `state: 'approved'`; `request_changes` → `state: 'changes_requested'`; * `comment` → clear the caller's own approval state. An optional body is - * posted as a comment alongside the review state. + * posted as a comment alongside the review state. An optional `comments` + * batch posts real inline comments on the diff BEFORE the review state and + * the summary comment, so a review carries GitHub-parity inline threads; + * once any inline comment has committed, every failure reports the + * retryable kind, so the router marks the ledger row reconcile_pending and + * a same-key retry never re-posts the committed comments as duplicates. */ export async function submitReview( target: BitbucketPrTarget & { event: 'approve' | 'request_changes' | 'comment'; body?: string; + comments?: ProviderReviewInlineComment[]; } & BitbucketMutationInput ): Promise { - if (target.event === 'comment' && !target.body) { + if (target.event === 'comment' && !target.body && !target.comments?.length) { throw new BitbucketReviewError('bad_request', 'A comment review needs a body.'); } const access = await targetAccess(target); + let inlineCommitted = false; try { + for (const comment of target.comments ?? []) { + try { + await requestBitbucketJson(access, `${prPath(access, target.prId)}/comments`, { + method: 'POST', + body: { + content: { raw: comment.body }, + inline: buildInlinePosition(comment), + }, + }); + } catch (error) { + // A rejection after an earlier comment committed is a partial + // apply: the deterministic kind would settle the ledger row failed + // and let a key-rotating retry re-post the committed comments. + throw inlineCommitted + ? new BitbucketReviewError('retryable', BITBUCKET_INLINE_PARTIAL_APPLY_REASON) + : error; + } + inlineCommitted = true; + } const accountId = await ownAccountId(access); const state = target.event === 'approve' @@ -199,7 +266,14 @@ export async function submitReview( } return { done: true, replayed: false }; } catch (error) { - throw classifyBitbucketError(error); + const classified = classifyBitbucketError(error); + // Once an inline comment is live, every later failure — mid-batch or + // the state/summary step — is a partial apply: reconcile, never settle + // failed (see BITBUCKET_INLINE_PARTIAL_APPLY_REASON). + if (inlineCommitted && !classified.retryable) { + throw new BitbucketReviewError('retryable', BITBUCKET_INLINE_PARTIAL_APPLY_REASON); + } + throw classified; } } diff --git a/apps/web/src/lib/provider-review/gitlab-write.test.ts b/apps/web/src/lib/provider-review/gitlab-write.test.ts index 1d72b67755..a293b4b376 100644 --- a/apps/web/src/lib/provider-review/gitlab-write.test.ts +++ b/apps/web/src/lib/provider-review/gitlab-write.test.ts @@ -48,11 +48,16 @@ jest.mock('@/lib/integrations/platforms/gitlab/instance-url', () => { ...actual, // No pinned address → requests keep the plain fetch transport these // assertions read; the bound transport is covered in gitlab-read.test.ts. - resolveGitLabUrlSafely: jest.fn(async (urlString: string) => ({ url: new URL(urlString) })), + resolveGitLabUrlSafely: jest.fn(async (urlString: string) => ({ + url: new URL(urlString), + })), }; }); -const OWNER: { type: 'user'; userId: string } = { type: 'user', userId: 'user_1' }; +const OWNER: { type: 'user'; userId: string } = { + type: 'user', + userId: 'user_1', +}; const INSTANCE_URL = 'https://gitlab.example.com'; const PROJECT_PATH = 'group/sub/repo'; @@ -88,7 +93,11 @@ function openMrFixture(headSha: string, extra: Record = {}) { source_branch: 'feature/deploy', target_branch: 'main', sha: headSha, - diff_refs: { base_sha: 'sha-base', head_sha: headSha, start_sha: 'sha-start' }, + diff_refs: { + base_sha: 'sha-base', + head_sha: headSha, + start_sha: 'sha-start', + }, web_url: `${INSTANCE_URL}/group/sub/repo/-/merge_requests/12`, author: { id: 1, username: 'alice', name: 'Alice' }, ...extra, @@ -106,7 +115,10 @@ function jsonResponse(data: unknown, status = 200): Response { function lastRequest(): { url: URL; init: RequestInit } { const last = fetchMock.mock.calls[fetchMock.mock.calls.length - 1]; - return { url: new URL(String(last[0])), init: (last[1] ?? {}) as RequestInit }; + return { + url: new URL(String(last[0])), + init: (last[1] ?? {}) as RequestInit, + }; } beforeEach(() => { @@ -121,7 +133,11 @@ beforeEach(() => { describe('addComment / replyToDiscussion', () => { it('posts a project note with the server-derived credentials', async () => { - const result = await addComment({ ...TARGET, body: 'Ship it', operationKey: 'op-1' }); + const result = await addComment({ + ...TARGET, + body: 'Ship it', + operationKey: 'op-1', + }); expect(result).toEqual({ done: true, replayed: false }); expect(mockCreateMRNote).toHaveBeenCalledWith( @@ -133,6 +149,14 @@ describe('addComment / replyToDiscussion', () => { ); }); + it('without an anchor keeps the note path and fetches no diff refs', async () => { + await addComment({ ...TARGET, body: 'Ship it' }); + + expect(mockCreateMRNote).toHaveBeenCalledTimes(1); + expect(mockFetchGitLabMergeRequest).not.toHaveBeenCalled(); + expect(fetchMock).not.toHaveBeenCalled(); + }); + it('replies inside a discussion thread', async () => { const result = await replyToDiscussion({ ...TARGET, @@ -151,9 +175,136 @@ describe('addComment / replyToDiscussion', () => { }); }); +describe('addComment with an anchor (diff discussion)', () => { + const discussionsPath = `/api/v4/projects/${encodeURIComponent(PROJECT_PATH)}/merge_requests/12/discussions`; + + function discussionRequest(): { url: URL; init: RequestInit } { + const call = fetchMock.mock.calls.find( + entry => new URL(String(entry[0])).pathname === discussionsPath + ); + if (!call) throw new Error('Expected a POST to the discussions endpoint.'); + return { + url: new URL(String(call[0])), + init: (call[1] ?? {}) as RequestInit, + }; + } + + it('RIGHT anchor creates a text-position discussion from the MR diff refs', async () => { + const result = await addComment({ + ...TARGET, + body: 'Guard the path', + anchor: { path: 'deploy/run.sh', side: 'RIGHT', line: 42 }, + }); + + expect(result).toEqual({ done: true, replayed: false }); + // An anchored comment is a real discussion, never a top-level note: + expect(mockCreateMRNote).not.toHaveBeenCalled(); + const { url, init } = discussionRequest(); + expect(init.method).toBe('POST'); + expect(url.pathname).toBe(discussionsPath); + expect(JSON.parse(String(init.body))).toEqual({ + body: 'Guard the path', + position: { + position_type: 'text', + base_sha: 'sha-base', + start_sha: 'sha-start', + head_sha: 'sha-head', + new_path: 'deploy/run.sh', + old_path: 'deploy/run.sh', + new_line: 42, + }, + }); + }); + + it('LEFT anchor positions on the old side with old_line', async () => { + await addComment({ + ...TARGET, + body: 'Deleted too early', + anchor: { path: 'deploy/run.sh', side: 'LEFT', line: 7 }, + }); + + const { init } = discussionRequest(); + const position = (JSON.parse(String(init.body)) as { position: Record }) + .position; + expect(position).toEqual({ + position_type: 'text', + base_sha: 'sha-base', + start_sha: 'sha-start', + head_sha: 'sha-head', + new_path: 'deploy/run.sh', + old_path: 'deploy/run.sh', + old_line: 7, + }); + }); + + it('a RIGHT startLine range anchors new_line alone: an old_line pair 400s on added lines', async () => { + await addComment({ + ...TARGET, + body: 'This block', + anchor: { path: 'a/b.ts', side: 'RIGHT', line: 20, startLine: 10 }, + }); + + const { init } = discussionRequest(); + const position = (JSON.parse(String(init.body)) as { position: Record }) + .position; + // GitLab reads an old_line beside new_line as one changed-line pair, so + // a range on added lines has no old-side counterpart and the position is + // rejected (400). The range anchors its end line on the new side. + expect(position).toEqual({ + position_type: 'text', + base_sha: 'sha-base', + start_sha: 'sha-start', + head_sha: 'sha-head', + new_path: 'a/b.ts', + old_path: 'a/b.ts', + new_line: 20, + }); + expect(position).not.toHaveProperty('old_line'); + }); + + it('refuses an anchor when the MR reports no diff refs, before any write', async () => { + mockFetchGitLabMergeRequest.mockResolvedValue({ + ...openMrFixture('sha-head'), + diff_refs: undefined, + }); + + const error = await captureRejection( + addComment({ + ...TARGET, + body: 'x', + anchor: { path: 'a.ts', side: 'RIGHT', line: 1 }, + }) + ); + + expect(error.kind).toBe('bad_request'); + expect(error.retryable).toBe(false); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('classifies a provider 400 (line outside the diff) as bad_request', async () => { + fetchMock.mockResolvedValue(jsonResponse({ message: '400 Bad request' }, 400)); + + const error = await captureRejection( + addComment({ + ...TARGET, + body: 'x', + anchor: { path: 'a.ts', side: 'RIGHT', line: 999_999 }, + }) + ); + + expect(error).toBeInstanceOf(GitLabReviewError); + expect(error.kind).toBe('bad_request'); + expect(error.retryable).toBe(false); + }); +}); + describe('submitReview', () => { it('approve posts the approval plus an optional summary note', async () => { - const result = await submitReview({ ...TARGET, event: 'approve', body: 'LGTM' }); + const result = await submitReview({ + ...TARGET, + event: 'approve', + body: 'LGTM', + }); expect(result).toEqual({ done: true, replayed: false }); const { url, init } = lastRequest(); @@ -203,6 +354,186 @@ describe('submitReview', () => { }); }); +describe('submitReview with an inline comment batch', () => { + function effectOrder(): string[] { + const order: string[] = []; + fetchMock.mockImplementation(async (url: string | URL) => { + const path = new URL(String(url)).pathname; + if (path.endsWith('/discussions')) order.push('discussion'); + else if (path.endsWith('/approve')) order.push('approve'); + return jsonResponse({ state: 'opened' }); + }); + mockCreateMRNote.mockImplementation(async () => { + order.push('note'); + }); + return order; + } + + function discussionBodies(): Array> { + return fetchMock.mock.calls + .filter(entry => new URL(String(entry[0])).pathname.endsWith('/discussions')) + .map(entry => JSON.parse(String((entry[1] as RequestInit).body))); + } + + it('posts every inline discussion before the approval and the summary note', async () => { + const order = effectOrder(); + + const result = await submitReview({ + ...TARGET, + event: 'approve', + body: 'LGTM', + comments: [ + { path: 'a.ts', side: 'RIGHT', line: 3, body: 'first inline' }, + { + path: 'b.ts', + side: 'LEFT', + line: 9, + startLine: 4, + body: 'second inline', + }, + ], + }); + + expect(result).toEqual({ done: true, replayed: false }); + expect(order).toEqual(['discussion', 'discussion', 'approve', 'note']); + expect(discussionBodies()).toEqual([ + { + body: 'first inline', + position: { + position_type: 'text', + base_sha: 'sha-base', + start_sha: 'sha-start', + head_sha: 'sha-head', + new_path: 'a.ts', + old_path: 'a.ts', + new_line: 3, + }, + }, + { + body: 'second inline', + position: { + position_type: 'text', + base_sha: 'sha-base', + start_sha: 'sha-start', + head_sha: 'sha-head', + new_path: 'b.ts', + old_path: 'b.ts', + old_line: 9, + }, + }, + ]); + }); + + it('a comment event with a batch and no body posts the discussions only', async () => { + const order = effectOrder(); + + const result = await submitReview({ + ...TARGET, + event: 'comment', + comments: [{ path: 'a.ts', side: 'RIGHT', line: 3, body: 'inline only' }], + }); + + expect(result).toEqual({ done: true, replayed: false }); + expect(order).toEqual(['discussion']); + expect(mockCreateMRNote).not.toHaveBeenCalled(); + }); + + it('a mid-batch rejection after a committed discussion reports the ambiguous retryable kind', async () => { + let discussions = 0; + fetchMock.mockImplementation(async (url: string | URL) => { + const path = new URL(String(url)).pathname; + if (path.endsWith('/discussions')) { + discussions += 1; + return discussions === 1 + ? jsonResponse({ id: 'disc-1' }) + : jsonResponse({ message: '400 line is not in diff' }, 400); + } + return jsonResponse({ state: 'opened' }); + }); + + const error = await captureRejection( + submitReview({ + ...TARGET, + event: 'approve', + body: 'LGTM', + comments: [ + { path: 'a.ts', side: 'RIGHT', line: 3, body: 'first' }, + { path: 'b.ts', side: 'RIGHT', line: 4, body: 'outside' }, + ], + }) + ); + + // The first discussion already committed: a deterministic bad_request + // would settle the ledger row failed, and the client's key-rotating + // retry would re-post that comment as a duplicate. The retryable kind + // keeps the row reconcile_pending instead. + expect(error.kind).toBe('retryable'); + expect(error.retryable).toBe(true); + // The failure stops the batch at the rejected item: no approval, no note. + expect(discussions).toBe(2); + expect( + fetchMock.mock.calls.some(entry => new URL(String(entry[0])).pathname.endsWith('/approve')) + ).toBe(false); + expect(mockCreateMRNote).not.toHaveBeenCalled(); + }); + + it('a rejection on the first item, with nothing committed, keeps the deterministic bad_request', async () => { + fetchMock.mockImplementation(async (url: string | URL) => { + const path = new URL(String(url)).pathname; + if (path.endsWith('/discussions')) { + return jsonResponse({ message: '400 line is not in diff' }, 400); + } + return jsonResponse({ state: 'opened' }); + }); + + const error = await captureRejection( + submitReview({ + ...TARGET, + event: 'approve', + body: 'LGTM', + comments: [ + { path: 'a.ts', side: 'RIGHT', line: 999_999, body: 'outside' }, + { path: 'b.ts', side: 'RIGHT', line: 4, body: 'second' }, + ], + }) + ); + + expect(error.kind).toBe('bad_request'); + expect(error.retryable).toBe(false); + // The batch stops at the refused item: the second discussion never posts. + expect( + fetchMock.mock.calls.filter(entry => + new URL(String(entry[0])).pathname.endsWith('/discussions') + ) + ).toHaveLength(1); + }); + + it('an approval rejection after the whole batch committed is a partial apply too', async () => { + fetchMock.mockImplementation(async (url: string | URL) => { + const path = new URL(String(url)).pathname; + if (path.endsWith('/approve')) { + return jsonResponse({ message: '403 Forbidden' }, 403); + } + return jsonResponse({ id: 'disc-1' }); + }); + + const error = await captureRejection( + submitReview({ + ...TARGET, + event: 'approve', + body: 'LGTM', + comments: [{ path: 'a.ts', side: 'RIGHT', line: 3, body: 'first' }], + }) + ); + + // The inline discussion committed before the approval was refused: a + // failed settle would let the retry re-post it as a duplicate. + expect(error.kind).toBe('retryable'); + expect(error.retryable).toBe(true); + expect(mockCreateMRNote).not.toHaveBeenCalled(); + }); +}); + describe('resolveThread / unresolveThread', () => { function discussionFixture(resolved: boolean) { return { @@ -337,7 +668,10 @@ describe('mergePullRequest', () => { state: 'merged', }); - const result = await mergePullRequest({ ...TARGET, expectedHeadSha: 'sha-head' }); + const result = await mergePullRequest({ + ...TARGET, + expectedHeadSha: 'sha-head', + }); expect(result).toEqual({ done: true, replayed: true }); expect(fetchMock).not.toHaveBeenCalled(); @@ -375,7 +709,10 @@ describe('enableAutoMerge', () => { openMrFixture('sha-head', { head_pipeline: { status: 'running' } }) ); - const result = await enableAutoMerge({ ...TARGET, expectedHeadSha: 'sha-head' }); + const result = await enableAutoMerge({ + ...TARGET, + expectedHeadSha: 'sha-head', + }); expect(result).toEqual({ done: true, replayed: false }); const { url, init } = lastRequest(); @@ -397,7 +734,10 @@ describe('enableAutoMerge', () => { openMrFixture('sha-head', { merge_when_pipeline_succeeds: true }) ); - const result = await enableAutoMerge({ ...TARGET, expectedHeadSha: 'sha-head' }); + const result = await enableAutoMerge({ + ...TARGET, + expectedHeadSha: 'sha-head', + }); expect(result).toEqual({ done: true, replayed: true }); expect(fetchMock).not.toHaveBeenCalled(); @@ -439,12 +779,12 @@ describe('enableAutoMerge', () => { openMrFixture('sha-head', { head_pipeline: { status: 'success' } }) ); - await expect( - enableAutoMerge({ ...TARGET, expectedHeadSha: 'sha-head' }) - ).rejects.toMatchObject({ - kind: 'bad_request', - message: GITLAB_AUTO_MERGE_NO_PIPELINE_REASON, - }); + await expect(enableAutoMerge({ ...TARGET, expectedHeadSha: 'sha-head' })).rejects.toMatchObject( + { + kind: 'bad_request', + message: GITLAB_AUTO_MERGE_NO_PIPELINE_REASON, + } + ); expect(fetchMock).not.toHaveBeenCalled(); }); }); @@ -491,7 +831,10 @@ describe('deleteBranch', () => { it('deletes the project branch', async () => { fetchMock.mockResolvedValue(new Response(null, { status: 204 })); - const result = await deleteBranch({ ...TARGET, branchName: 'feature/deploy' }); + const result = await deleteBranch({ + ...TARGET, + branchName: 'feature/deploy', + }); expect(result).toEqual({ done: true, replayed: false }); const { url, init } = lastRequest(); @@ -504,7 +847,10 @@ describe('deleteBranch', () => { it('treats an already-deleted branch as a replay', async () => { fetchMock.mockResolvedValue(jsonResponse({ message: '404 Branch Not Found' }, 404)); - const result = await deleteBranch({ ...TARGET, branchName: 'feature/gone' }); + const result = await deleteBranch({ + ...TARGET, + branchName: 'feature/gone', + }); expect(result).toEqual({ done: true, replayed: true }); }); diff --git a/apps/web/src/lib/provider-review/gitlab-write.ts b/apps/web/src/lib/provider-review/gitlab-write.ts index 1c75fb328f..4c3bebb5a1 100644 --- a/apps/web/src/lib/provider-review/gitlab-write.ts +++ b/apps/web/src/lib/provider-review/gitlab-write.ts @@ -12,7 +12,11 @@ */ import 'server-only'; -import type { ProviderReviewCapabilities } from '@kilocode/app-shared/provider-review'; +import type { + ProviderReviewCapabilities, + ProviderReviewInlineAnchor, + ProviderReviewInlineComment, +} from '@kilocode/app-shared/provider-review'; import { createMRNote, fetchGitLabMergeRequest, @@ -121,19 +125,144 @@ function mrPath(access: GitLabProjectAccess, mrIid: number): string { return `/api/v4/projects/${encodeURIComponent(access.projectPath)}/merge_requests/${mrIid}`; } -/** Post a top-level comment (project note) on the merge request. */ +/** + * The MR's diff refs, fetched server-side through the authorized access. A + * diff discussion positions against base/start/head, so an anchored comment + * can only be built from the revision the provider reports right now. + */ +type GitLabDiffRefs = { base_sha: string; head_sha: string; start_sha: string }; + +async function fetchMrDiffRefs( + access: GitLabProjectAccess, + mrIid: number +): Promise { + const mr = (await fetchGitLabMergeRequest({ + accessToken: access.accessToken, + projectId: access.projectPath, + mrIid, + instanceUrl: access.instanceUrl, + })) as GitLabMergeRequestDetail; + const refs = mr.diff_refs; + if (!refs?.base_sha || !refs.head_sha || !refs.start_sha) { + throw new GitLabReviewError( + 'bad_request', + 'The merge request has no diff positions to anchor a comment to.' + ); + } + return refs; +} + +/** + * The GitLab text position for one anchor: the current diff refs plus the + * anchored path/line. RIGHT anchors the new side (`new_line`), LEFT the old + * side (`old_line`). A `startLine` range is NOT sent as an `old_line` beside + * the `new_line`: GitLab reads that pair as one changed-line relation, so a + * range on added lines (no old-side counterpart) 400s. A range anchors its + * end line on the tapped side; the range stays in the ledger key and the + * pending list, not in the provider position. + */ +function buildTextPosition( + refs: GitLabDiffRefs, + anchor: ProviderReviewInlineAnchor +): Record { + const position: Record = { + position_type: 'text', + base_sha: refs.base_sha, + start_sha: refs.start_sha, + head_sha: refs.head_sha, + new_path: anchor.path, + old_path: anchor.path, + }; + if (anchor.side === 'RIGHT') { + position.new_line = anchor.line; + } else { + position.old_line = anchor.line; + } + return position; +} + +/** + * The partial-apply reason: an inline discussion committed before a later + * rejection, so the provider already holds effects a replayed batch would + * duplicate. The retryable kind keeps the router's ledger row + * reconcile_pending instead of settling it failed. + */ +const GITLAB_INLINE_PARTIAL_APPLY_REASON = + 'GitLab applied part of this review before the request failed. Check the merge request before retrying.'; + +/** + * Classify one submitReview failure. A position outside the diff (400) is a + * clean deterministic refusal only while nothing has committed: the failed + * settle lets the client rotate its operation key and a fresh intent re-posts + * the whole batch. Once any inline discussion is live every later failure — + * mid-batch or the approval/summary step — is a partial apply, so it reports + * the retryable kind and the router reconciles instead of replaying. + */ +function classifySubmitFailure(error: unknown, inlineCommitted: boolean): GitLabReviewError { + const classified = classifyGitLabError(error); + if (inlineCommitted && !classified.retryable) { + return new GitLabReviewError('retryable', GITLAB_INLINE_PARTIAL_APPLY_REASON); + } + return classified; +} + +/** + * Create one diff discussion per anchored comment on the merge request. + * GitLab rejects a position outside the diff (400), which classifyGitLabError + * surfaces as a non-retryable bad_request through the existing taxonomy — + * but only while nothing has committed: a rejection after an earlier + * discussion committed is a partial apply, reported through the retryable + * kind so the ledger row stays reconcile_pending and never replays the + * committed comments as duplicates. + */ +async function createInlineDiscussions( + access: GitLabProjectAccess, + mrIid: number, + anchored: Array<{ anchor: ProviderReviewInlineAnchor; body: string }> +): Promise { + const refs = await fetchMrDiffRefs(access, mrIid); + let committed = false; + for (const item of anchored) { + try { + await requestGitLabJson(access, `${mrPath(access, mrIid)}/discussions`, { + method: 'POST', + body: { body: item.body, position: buildTextPosition(refs, item.anchor) }, + }); + } catch (error) { + throw committed + ? new GitLabReviewError('retryable', GITLAB_INLINE_PARTIAL_APPLY_REASON) + : error; + } + committed = true; + } +} + +/** + * Post a comment on the merge request. With an `anchor` this creates a real + * diff discussion positioned in the MR's current diff; without one it posts + * a top-level project note, byte-identical to the previous behavior. + */ export async function addComment( - target: GitLabMrTarget & { body: string } & GitLabMutationInput + target: GitLabMrTarget & { + body: string; + anchor?: ProviderReviewInlineAnchor; + } & GitLabMutationInput ): Promise { const access = await targetAccess(target); try { - await createMRNote( - access.accessToken, - access.projectPath, - target.mrIid, - target.body, - access.instanceUrl - ); + if (target.anchor) { + await createInlineDiscussions(access, target.mrIid, [ + { anchor: target.anchor, body: target.body }, + ]); + } else { + await createMRNote( + access.accessToken, + access.projectPath, + target.mrIid, + target.body, + access.instanceUrl + ); + } return { done: true, replayed: false }; } catch (error) { throw classifyGitLabError(error); @@ -142,7 +271,10 @@ export async function addComment( /** Reply inside an existing discussion thread. */ export async function replyToDiscussion( - target: GitLabMrTarget & { discussionId: string; body: string } & GitLabMutationInput + target: GitLabMrTarget & { + discussionId: string; + body: string; + } & GitLabMutationInput ): Promise { const access = await targetAccess(target); try { @@ -160,19 +292,37 @@ export async function replyToDiscussion( /** * Submit a review. `approve` → POST /approve plus an optional summary note; * `comment` → note; `request_changes` is not a GitLab concept and is refused - * with the exact reason — never a silent fallback to another event. + * with the exact reason — never a silent fallback to another event. An + * optional `comments` batch posts real inline diff discussions BEFORE the + * approval/summary note, so a review carries GitHub-parity inline threads; + * once any discussion has committed, every failure reports the retryable + * kind, so the router marks the ledger row reconcile_pending and a same-key + * retry never re-posts the committed comments as duplicates. */ export async function submitReview( target: GitLabMrTarget & { event: 'approve' | 'comment' | 'request_changes'; body?: string; + comments?: ProviderReviewInlineComment[]; } & GitLabMutationInput ): Promise { if (target.event === 'request_changes') { throw new GitLabReviewError('bad_request', GITLAB_REQUEST_CHANGES_UNSUPPORTED_REASON); } const access = await targetAccess(target); + let inlineCommitted = false; try { + if (target.comments?.length) { + await createInlineDiscussions( + access, + target.mrIid, + target.comments.map(comment => ({ + anchor: comment, + body: comment.body, + })) + ); + inlineCommitted = true; + } if (target.event === 'approve') { await requestGitLabJson(access, `${mrPath(access, target.mrIid)}/approve`, { method: 'POST', @@ -194,12 +344,12 @@ export async function submitReview( target.body, access.instanceUrl ); - } else { + } else if (!target.comments?.length) { throw new GitLabReviewError('bad_request', 'A comment review needs a body.'); } return { done: true, replayed: false }; } catch (error) { - throw classifyGitLabError(error); + throw classifySubmitFailure(error, inlineCommitted); } } diff --git a/apps/web/src/routers/provider-review-router.test.ts b/apps/web/src/routers/provider-review-router.test.ts index a13ec61bd2..b54e69abcd 100644 --- a/apps/web/src/routers/provider-review-router.test.ts +++ b/apps/web/src/routers/provider-review-router.test.ts @@ -176,7 +176,11 @@ function admittedRow(overrides: Partial = {}): OperationLedg function admittingOnce(admission: string, rowOverrides: Partial = {}): void { mockAdmitOperation.mockImplementationOnce(async (_db: unknown, args: any) => ({ admission, - row: admittedRow({ intent: args.intent, resource_key: args.resourceKey, ...rowOverrides }), + row: admittedRow({ + intent: args.intent, + resource_key: args.resourceKey, + ...rowOverrides, + }), })); } @@ -212,9 +216,18 @@ beforeEach(() => { mockRecordOperationAcceptance.mockResolvedValue(null); gitlabRead.getMergeRequest.mockResolvedValue(summaryFixture()); gitlabWrite.addComment.mockResolvedValue({ done: true, replayed: false }); - gitlabWrite.mergePullRequest.mockResolvedValue({ done: true, replayed: false }); - gitlabWrite.enableAutoMerge.mockResolvedValue({ done: true, replayed: false }); - gitlabWrite.disableAutoMerge.mockResolvedValue({ done: true, replayed: false }); + gitlabWrite.mergePullRequest.mockResolvedValue({ + done: true, + replayed: false, + }); + gitlabWrite.enableAutoMerge.mockResolvedValue({ + done: true, + replayed: false, + }); + gitlabWrite.disableAutoMerge.mockResolvedValue({ + done: true, + replayed: false, + }); bitbucketWrite.addComment.mockResolvedValue({ done: true, replayed: false }); }); @@ -248,7 +261,10 @@ describe('providerReviewRouter inputs', () => { }); it('accepts the infinite-query direction discriminator on paged inputs', async () => { - gitlabRead.listChangedFiles.mockResolvedValue({ items: [], nextCursor: null }); + gitlabRead.listChangedFiles.mockResolvedValue({ + items: [], + nextCursor: null, + }); await expect( caller.listFiles({ ...gitlabBase, cursor: 'c1', direction: 'forward' }) ).resolves.toBeDefined(); @@ -269,7 +285,9 @@ describe('providerReviewRouter identity derivation', () => { gitlabRead.getMergeRequest.mockResolvedValue(summaryFixture()); await caller.getPullRequest({ ...gitlabBase, organizationId: ORG_ID }); expect(mockEnsureOrganizationAccess).toHaveBeenCalledWith( - expect.objectContaining({ user: expect.objectContaining({ id: USER_ID }) }), + expect.objectContaining({ + user: expect.objectContaining({ id: USER_ID }), + }), ORG_ID ); expect(gitlabRead.getMergeRequest).toHaveBeenCalledWith( @@ -297,7 +315,11 @@ describe('providerReviewRouter identity derivation', () => { new TRPCError({ code: 'FORBIDDEN', message: 'no access' }) ); await expect( - caller.addComment({ ...bitbucketBase, body: 'hi', operationKey: 'key-1' }) + caller.addComment({ + ...bitbucketBase, + body: 'hi', + operationKey: 'key-1', + }) ).rejects.toMatchObject({ code: 'FORBIDDEN' }); expect(bitbucketWrite.addComment).not.toHaveBeenCalled(); expect(mockAdmitOperation).not.toHaveBeenCalled(); @@ -305,7 +327,10 @@ describe('providerReviewRouter identity derivation', () => { it('passes instanceHint only as a hint to the authorization layer, with the server-derived owner', async () => { gitlabRead.getMergeRequest.mockResolvedValue(summaryFixture()); - await caller.getPullRequest({ ...gitlabBase, instanceHint: 'gitlab.example' }); + await caller.getPullRequest({ + ...gitlabBase, + instanceHint: 'gitlab.example', + }); // The hint arrives as the LAST positional argument of the read layer — // the layer matches it against the connected instance and refuses a // mismatch (gitlab-authorization.test.ts); the router never builds a @@ -319,14 +344,20 @@ describe('providerReviewRouter identity derivation', () => { }); it('never lets a page cursor steer which repository is read', async () => { - gitlabRead.listChangedFiles.mockResolvedValue({ items: [], nextCursor: null }); + gitlabRead.listChangedFiles.mockResolvedValue({ + items: [], + nextCursor: null, + }); // A cursor minted for another repository is still only an opaque page // pointer: the router forwards the INPUT's identity, and the provider // cursor codec (s2) refuses a cursor bound to a different identity. await caller.listFiles({ ...gitlabBase, cursor: Buffer.from( - JSON.stringify({ identity: 'gitlab-diff:other/repo#1', next: 'https://x/other%2Frepo' }) + JSON.stringify({ + identity: 'gitlab-diff:other/repo#1', + next: 'https://x/other%2Frepo', + }) ).toString('base64url'), }); expect(gitlabRead.listChangedFiles).toHaveBeenCalledWith( @@ -343,7 +374,11 @@ describe('providerReviewRouter identity derivation', () => { describe('providerReviewRouter ledger', () => { it('admits provider writes into the shared pr domain with a provider-tagged resource key', async () => { - await caller.addComment({ ...gitlabBase, body: 'hello', operationKey: 'key-1' }); + await caller.addComment({ + ...gitlabBase, + body: 'hello', + operationKey: 'key-1', + }); const expectedKey = providerLedgerResourceKey( 'create_review_comment', { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 7 }, @@ -375,7 +410,12 @@ describe('providerReviewRouter ledger', () => { const gitlabKey = providerLedgerResourceKey( 'create_review_comment', { platform: 'gitlab', projectPath: 'octocat/hello', mrIid: 1 }, - { platform: 'gitlab', projectPath: 'octocat/hello', number: 1, body: 'same text' } + { + platform: 'gitlab', + projectPath: 'octocat/hello', + number: 1, + body: 'same text', + } ); // The GitHub ledger identity (prLedgerResourceKey) is // `owner/repo#number::hash` — a plain string prefix. @@ -383,12 +423,21 @@ describe('providerReviewRouter ledger', () => { expect(gitlabKey.startsWith(githubStyle)).toBe(false); expect( gitlabKey.startsWith( - providerPrRefKey({ platform: 'gitlab', projectPath: 'octocat/hello', mrIid: 1 }) + providerPrRefKey({ + platform: 'gitlab', + projectPath: 'octocat/hello', + mrIid: 1, + }) ) ).toBe(true); const bitbucketKey = providerLedgerResourceKey( 'create_review_comment', - { platform: 'bitbucket', workspace: 'octocat', repoSlug: 'hello', prId: 1 }, + { + platform: 'bitbucket', + workspace: 'octocat', + repoSlug: 'hello', + prId: 1, + }, { platform: 'bitbucket', workspace: 'octocat', @@ -402,7 +451,11 @@ describe('providerReviewRouter ledger', () => { }); it('settles a completed write with the pr_operation_settled outbox event', async () => { - await caller.addComment({ ...gitlabBase, body: 'hello', operationKey: 'key-1' }); + await caller.addComment({ + ...gitlabBase, + body: 'hello', + operationKey: 'key-1', + }); expect(mockSettleOperation).toHaveBeenCalledWith( {}, expect.objectContaining({ @@ -428,7 +481,11 @@ describe('providerReviewRouter ledger', () => { canonical_result: { done: true, replayed: false }, }); await expect( - caller.addComment({ ...gitlabBase, body: 'hello', operationKey: 'key-1' }) + caller.addComment({ + ...gitlabBase, + body: 'hello', + operationKey: 'key-1', + }) ).resolves.toEqual({ done: true, replayed: true }); expect(gitlabWrite.addComment).not.toHaveBeenCalled(); }); @@ -439,8 +496,15 @@ describe('providerReviewRouter ledger', () => { row: admittedRow({ intent: 'merge' }), }); await expect( - caller.addComment({ ...gitlabBase, body: 'hello', operationKey: 'key-1' }) - ).rejects.toMatchObject({ code: 'CONFLICT', message: 'operation_key_reuse_mismatch' }); + caller.addComment({ + ...gitlabBase, + body: 'hello', + operationKey: 'key-1', + }) + ).rejects.toMatchObject({ + code: 'CONFLICT', + message: 'operation_key_reuse_mismatch', + }); expect(gitlabWrite.addComment).not.toHaveBeenCalled(); expect(mockSettleOperation).not.toHaveBeenCalled(); }); @@ -448,8 +512,15 @@ describe('providerReviewRouter ledger', () => { it('never re-executes an in-flight duplicate', async () => { admittingOnce('duplicate_in_flight'); await expect( - caller.addComment({ ...gitlabBase, body: 'hello', operationKey: 'key-1' }) - ).rejects.toMatchObject({ code: 'CONFLICT', message: 'operation_in_progress' }); + caller.addComment({ + ...gitlabBase, + body: 'hello', + operationKey: 'key-1', + }) + ).rejects.toMatchObject({ + code: 'CONFLICT', + message: 'operation_in_progress', + }); expect(gitlabWrite.addComment).not.toHaveBeenCalled(); }); @@ -458,8 +529,15 @@ describe('providerReviewRouter ledger', () => { new TRPCError({ code: 'PRECONDITION_FAILED', message: 'terms_required' }) ); await expect( - caller.addComment({ ...gitlabBase, body: 'hello', operationKey: 'key-1' }) - ).rejects.toMatchObject({ code: 'PRECONDITION_FAILED', message: 'terms_required' }); + caller.addComment({ + ...gitlabBase, + body: 'hello', + operationKey: 'key-1', + }) + ).rejects.toMatchObject({ + code: 'PRECONDITION_FAILED', + message: 'terms_required', + }); expect(mockAdmitOperation).not.toHaveBeenCalled(); expect(gitlabWrite.addComment).not.toHaveBeenCalled(); }); @@ -469,7 +547,11 @@ describe('providerReviewRouter ledger', () => { new GitLabReviewError('retryable', 'Could not reach GitLab. Please try again.') ); await expect( - caller.addComment({ ...gitlabBase, body: 'hello', operationKey: 'key-1' }) + caller.addComment({ + ...gitlabBase, + body: 'hello', + operationKey: 'key-1', + }) ).rejects.toMatchObject({ code: 'CONFLICT', message: "Couldn't confirm — check the merge request before retrying.", @@ -489,6 +571,283 @@ describe('providerReviewRouter ledger', () => { }); }); +// ----- inline anchors ------------------------------------------------------------- + +describe('providerReviewRouter inline anchors', () => { + const anchor = { + path: 'src/a.ts', + side: 'RIGHT' as const, + line: 42, + startLine: 40, + }; + + it('passes the anchor to the GitLab write and folds it into the fingerprint', async () => { + await caller.addComment({ + ...gitlabBase, + body: 'inline', + anchor, + operationKey: 'key-1', + }); + + expect(gitlabWrite.addComment).toHaveBeenCalledWith( + expect.objectContaining({ body: 'inline', anchor }) + ); + const expectedKey = providerLedgerResourceKey( + 'create_review_comment', + { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 7 }, + { + platform: 'gitlab', + projectPath: 'group/sub/repo', + instanceHint: undefined, + number: 7, + body: 'inline', + path: 'src/a.ts', + line: 42, + side: 'RIGHT', + startLine: 40, + } + ); + expect(mockAdmitOperation).toHaveBeenCalledWith( + {}, + expect.objectContaining({ resourceKey: expectedKey }) + ); + }); + + it('passes the anchor to the Bitbucket write and folds it into the fingerprint', async () => { + await caller.addComment({ + ...bitbucketBase, + body: 'inline', + anchor, + operationKey: 'key-1', + }); + + expect(bitbucketWrite.addComment).toHaveBeenCalledWith( + expect.objectContaining({ body: 'inline', anchor }) + ); + const expectedKey = providerLedgerResourceKey( + 'create_review_comment', + { + platform: 'bitbucket', + workspace: 'acme', + repoSlug: 'widgets', + prId: 12, + }, + { + platform: 'bitbucket', + workspace: 'acme', + repoSlug: 'widgets', + number: 12, + body: 'inline', + path: 'src/a.ts', + line: 42, + side: 'RIGHT', + startLine: 40, + } + ); + expect(mockAdmitOperation).toHaveBeenCalledWith( + {}, + expect.objectContaining({ resourceKey: expectedKey }) + ); + }); + + it('an anchored and an unanchored comment with the same body never share a ledger key', async () => { + const anchored = providerLedgerResourceKey( + 'create_review_comment', + { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 7 }, + { + platform: 'gitlab', + projectPath: 'group/sub/repo', + number: 7, + body: 'same', + path: 'src/a.ts', + line: 42, + side: 'RIGHT', + } + ); + const plain = providerLedgerResourceKey( + 'create_review_comment', + { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 7 }, + { + platform: 'gitlab', + projectPath: 'group/sub/repo', + number: 7, + body: 'same', + } + ); + expect(anchored).not.toEqual(plain); + }); + + it('without an anchor the write payload and the fingerprint bytes stay unchanged', async () => { + await caller.addComment({ + ...gitlabBase, + body: 'hello', + operationKey: 'key-1', + }); + + expect(gitlabWrite.addComment).toHaveBeenCalledWith(expect.objectContaining({ body: 'hello' })); + expect(gitlabWrite.addComment.mock.calls[0][0]).not.toHaveProperty('anchor'); + // The legacy bytes: path/line/side/startLine absent (undefined) still + // serialize identically, so older clients keep replaying correctly. + const legacyKey = providerLedgerResourceKey( + 'create_review_comment', + { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 7 }, + { + platform: 'gitlab', + projectPath: 'group/sub/repo', + instanceHint: undefined, + number: 7, + body: 'hello', + } + ); + expect(mockAdmitOperation).toHaveBeenCalledWith( + {}, + expect.objectContaining({ resourceKey: legacyKey }) + ); + }); + + it('refuses malformed anchors with BAD_REQUEST before any write', async () => { + for (const bad of [ + { path: 'a.ts', side: 'TOP', line: 1 }, + { path: 'a.ts', side: 'LEFT', line: 0 }, + { path: 'a.ts', side: 'LEFT', line: -3 }, + { path: '', side: 'LEFT', line: 1 }, + { path: 'a.ts', side: 'LEFT', line: 1.5 }, + { path: 'a.ts', side: 'LEFT', line: 1, startLine: 2 }, + { path: 'a.ts', side: 'LEFT', line: 1, extra: true }, + { side: 'LEFT', line: 1 }, + ]) { + await expect( + caller.addComment({ + ...gitlabBase, + body: 'x', + anchor: bad, + operationKey: 'k', + }) + ).rejects.toMatchObject({ code: 'BAD_REQUEST' }); + } + expect(gitlabWrite.addComment).not.toHaveBeenCalled(); + }); + + it('submitReview folds the comment batch into the write and the fingerprint', async () => { + gitlabWrite.submitReview.mockResolvedValueOnce({ + done: true, + replayed: false, + }); + const comments = [ + { path: 'a.ts', side: 'RIGHT' as const, line: 3, body: 'first' }, + { + path: 'b.ts', + side: 'LEFT' as const, + line: 9, + startLine: 4, + body: 'second', + }, + ]; + + await caller.submitReview({ + ...gitlabBase, + event: 'approve', + body: 'LGTM', + comments, + operationKey: 'key-1', + }); + + expect(gitlabWrite.submitReview).toHaveBeenCalledWith(expect.objectContaining({ comments })); + const expectedKey = providerLedgerResourceKey( + 'submit_review', + { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 7 }, + { + platform: 'gitlab', + projectPath: 'group/sub/repo', + instanceHint: undefined, + number: 7, + event: 'approve', + body: 'LGTM', + comments, + } + ); + expect(mockAdmitOperation).toHaveBeenCalledWith( + {}, + expect.objectContaining({ resourceKey: expectedKey }) + ); + }); + + it('Bitbucket submitReview carries the batch through the same path', async () => { + bitbucketWrite.submitReview.mockResolvedValueOnce({ + done: true, + replayed: false, + }); + const comments = [{ path: 'a.ts', side: 'RIGHT' as const, line: 3, body: 'first' }]; + + await caller.submitReview({ + ...bitbucketBase, + event: 'comment', + comments, + operationKey: 'key-1', + }); + + expect(bitbucketWrite.submitReview).toHaveBeenCalledWith( + expect.objectContaining({ event: 'comment', comments }) + ); + }); + + it('a submit without comments keeps the legacy fingerprint bytes', async () => { + gitlabWrite.submitReview.mockResolvedValueOnce({ + done: true, + replayed: false, + }); + await caller.submitReview({ + ...gitlabBase, + event: 'approve', + body: 'LGTM', + operationKey: 'key-1', + }); + + expect(gitlabWrite.submitReview.mock.calls[0][0]).not.toHaveProperty('comments'); + const legacyKey = providerLedgerResourceKey( + 'submit_review', + { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 7 }, + { + platform: 'gitlab', + projectPath: 'group/sub/repo', + instanceHint: undefined, + number: 7, + event: 'approve', + body: 'LGTM', + } + ); + expect(mockAdmitOperation).toHaveBeenCalledWith( + {}, + expect.objectContaining({ resourceKey: legacyKey }) + ); + }); + + it('refuses comment items and oversized batches with BAD_REQUEST before any write', async () => { + await expect( + caller.submitReview({ + ...gitlabBase, + event: 'comment', + comments: [{ path: 'a.ts', side: 'RIGHT', line: 1 }], + operationKey: 'k', + }) + ).rejects.toMatchObject({ code: 'BAD_REQUEST' }); + await expect( + caller.submitReview({ + ...gitlabBase, + event: 'comment', + comments: Array.from({ length: 101 }, (_, i) => ({ + path: 'a.ts', + side: 'RIGHT' as const, + line: i + 1, + body: 'x', + })), + operationKey: 'k', + }) + ).rejects.toMatchObject({ code: 'BAD_REQUEST' }); + expect(gitlabWrite.submitReview).not.toHaveBeenCalled(); + }); +}); + // ----- moved head blocks merge --------------------------------------------------- describe('providerReviewRouter merge head fence', () => { @@ -502,7 +861,10 @@ describe('providerReviewRouter merge head fence', () => { expectedHeadSha: 'a'.repeat(40), operationKey: 'key-merge', }) - ).rejects.toMatchObject({ code: 'CONFLICT', message: GITLAB_STALE_HEAD_REASON }); + ).rejects.toMatchObject({ + code: 'CONFLICT', + message: GITLAB_STALE_HEAD_REASON, + }); expect(mockSettleOperation).toHaveBeenCalledWith( {}, expect.objectContaining({ status: 'failed', outcomeCode: 'head_moved' }) @@ -511,7 +873,9 @@ describe('providerReviewRouter merge head fence', () => { }); it('reconciles a pending merge by re-reading through the owner-bound reader', async () => { - admittingOnce('duplicate_reconcile_pending', { status: 'reconcile_pending' }); + admittingOnce('duplicate_reconcile_pending', { + status: 'reconcile_pending', + }); gitlabRead.getMergeRequest.mockResolvedValueOnce(summaryFixture({ state: 'merged' })); await expect( caller.mergePullRequest({ @@ -539,7 +903,9 @@ describe('providerReviewRouter merge head fence', () => { }); it('a reconcile read showing a moved head settles failed confirmed_absent and refuses the merge', async () => { - admittingOnce('duplicate_reconcile_pending', { status: 'reconcile_pending' }); + admittingOnce('duplicate_reconcile_pending', { + status: 'reconcile_pending', + }); gitlabRead.getMergeRequest.mockResolvedValueOnce(summaryFixture({ headSha: 'b'.repeat(40) })); await expect( caller.mergePullRequest({ @@ -547,7 +913,10 @@ describe('providerReviewRouter merge head fence', () => { expectedHeadSha: 'a'.repeat(40), operationKey: 'key-merge', }) - ).rejects.toMatchObject({ code: 'CONFLICT', message: GITLAB_STALE_HEAD_REASON }); + ).rejects.toMatchObject({ + code: 'CONFLICT', + message: GITLAB_STALE_HEAD_REASON, + }); expect(gitlabWrite.mergePullRequest).not.toHaveBeenCalled(); expect(mockSettleOperation).toHaveBeenCalledWith( {}, @@ -555,14 +924,18 @@ describe('providerReviewRouter merge head fence', () => { status: 'failed', outcomeCode: 'head_moved', outboxEvent: expect.objectContaining({ - properties: expect.objectContaining({ reconcile_result: 'confirmed_absent' }), + properties: expect.objectContaining({ + reconcile_result: 'confirmed_absent', + }), }), }) ); }); it('a failed authoritative read stays reconcile-pending instead of settling absent', async () => { - admittingOnce('duplicate_reconcile_pending', { status: 'reconcile_pending' }); + admittingOnce('duplicate_reconcile_pending', { + status: 'reconcile_pending', + }); gitlabRead.getMergeRequest.mockRejectedValueOnce(new GitLabReviewError('not_found', 'gone')); await expect( caller.mergePullRequest({ @@ -624,7 +997,12 @@ describe('providerReviewRouter capabilities', () => { expectedHeadSha: 'a'.repeat(40), operationKey: 'key-am', }) - ).resolves.toEqual({ supported: true, reason: '', done: true, replayed: false }); + ).resolves.toEqual({ + supported: true, + reason: '', + done: true, + replayed: false, + }); expect(mockAdmitOperation).toHaveBeenCalledWith( {}, expect.objectContaining({ intent: 'enable_auto_merge' }) diff --git a/apps/web/src/routers/provider-review-router.ts b/apps/web/src/routers/provider-review-router.ts index e9458058d1..ffd556f782 100644 --- a/apps/web/src/routers/provider-review-router.ts +++ b/apps/web/src/routers/provider-review-router.ts @@ -92,6 +92,37 @@ const pageCursor = z.string().min(1).max(2048).optional(); // becomes retry-safe; when absent, the write runs unledgered (older clients). const operationKeySchema = z.string().min(1).max(128).optional(); +// The diff position an inline comment anchors to — the same shape the +// GitHub createReviewComment input carries (minus startSide/commitSha, which +// GitLab positions and Bitbucket inline blocks do not use). A `startLine` +// marks the first line of a multi-line range ending at `line`. +const inlineAnchorShape = { + path: z.string().min(1).max(1024), + side: z.enum(['LEFT', 'RIGHT']), + line: z.number().int().positive(), + startLine: z.number().int().positive().optional(), +}; +const startLineOrderIssue = { + message: 'startLine must be <= line', + path: ['startLine'], +}; + +const providerInlineAnchorInput = z + .object(inlineAnchorShape) + .strict() + .refine( + value => value.startLine === undefined || value.startLine <= value.line, + startLineOrderIssue + ); + +const providerInlineCommentInput = z + .object({ ...inlineAnchorShape, body: z.string().min(1).max(65_535) }) + .strict() + .refine( + value => value.startLine === undefined || value.startLine <= value.line, + startLineOrderIssue + ); + const gitlabIdentityShape = { platform: z.literal('gitlab'), organizationId: z.uuid().optional(), @@ -134,7 +165,10 @@ const providerIdentityInput = z.discriminatedUnion('platform', [ const GetPullRequestInput = providerRefInput({}); -const ListFilesInput = providerRefInput({ cursor: pageCursor, direction: infiniteQueryDirection }); +const ListFilesInput = providerRefInput({ + cursor: pageCursor, + direction: infiniteQueryDirection, +}); const ListDiscussionsInput = providerRefInput({ cursor: pageCursor, @@ -176,6 +210,12 @@ const GetFileLinesInput = providerRefInput({ const AddCommentInput = providerRefInput({ body: z.string().min(1).max(65_535), + // An optional diff anchor turns the comment into a real inline discussion + // (GitLab) / inline comment (Bitbucket); without it the write stays a + // top-level note. s1's create_review_comment fingerprint already folds + // path/line/side/startLine, so an anchored and an unanchored comment can + // never share a ledger key. + anchor: providerInlineAnchorInput.optional(), operationKey: operationKeySchema, }); @@ -203,6 +243,10 @@ const ReplyToCommentInput = z.discriminatedUnion('platform', [ const SubmitReviewInput = providerRefInput({ event: z.enum(['approve', 'request_changes', 'comment']), body: z.string().min(1).max(65_535).optional(), + // The inline batch a review submits BEFORE the summary note/approval. s1's + // submit_review fingerprint already folds `comments`, so a review with a + // different batch can never replay under the same key. + comments: z.array(providerInlineCommentInput).max(100).optional(), operationKey: operationKeySchema, }); @@ -273,7 +317,11 @@ async function gitlabOwner( ): Promise { if (input.organizationId) { await ensureOrganizationAccess(ctx, input.organizationId); - return { type: 'organization', organizationId: input.organizationId, userId: ctx.user.id }; + return { + type: 'organization', + organizationId: input.organizationId, + userId: ctx.user.id, + }; } return { type: 'user', userId: ctx.user.id }; } @@ -283,7 +331,11 @@ async function bitbucketOwner( input: { organizationId: string } ): Promise { await ensureOrganizationAccess(ctx, input.organizationId); - return { type: 'organization', organizationId: input.organizationId, userId: ctx.user.id }; + return { + type: 'organization', + organizationId: input.organizationId, + userId: ctx.user.id, + }; } function providerRef(input: { @@ -508,7 +560,11 @@ async function settleCompletedProviderRow( // The provider layer reports no external reference (no comment id, no // review id), so only the canonical evidence is preserved. await bestEffortLedgerWrite(() => - recordOperationAcceptance(db, { rowId: row.id, providerRef: null, canonicalResult }) + recordOperationAcceptance(db, { + rowId: row.id, + providerRef: null, + canonicalResult, + }) ); console.error( `Failed to settle completed provider PR operation ledger row: ${error instanceof Error ? error.message : String(error)}` @@ -648,11 +704,17 @@ async function executeProviderWrite>( /** Replays a terminal row: only `completed`/`no_op` may replay a canonical result. */ function replaySettledProviderRow(row: OperationLedgerRow): ReplayedResult { if (row.status === 'completed' || row.status === 'no_op') { - return { ...(row.canonical_result ?? {}), replayed: true } as ReplayedResult; + return { + ...(row.canonical_result ?? {}), + replayed: true, + } as ReplayedResult; } // A settled `failed` row cannot be recovered under the same key: surface a // non-retryable typed rejection so the client starts a fresh intent. - throw new TRPCError({ code: 'BAD_REQUEST', message: PROVIDER_REPLAY_FAILED_MESSAGE }); + throw new TRPCError({ + code: 'BAD_REQUEST', + message: PROVIDER_REPLAY_FAILED_MESSAGE, + }); } type ProviderLedgerMutationArgs = ProviderLedgerBase & { @@ -709,7 +771,10 @@ async function runProviderLedgerMutation( return replaySettledProviderRow(admission.row); case 'duplicate_in_flight': case 'duplicate_reconcile_in_progress': - throw new TRPCError({ code: 'CONFLICT', message: OPERATION_IN_PROGRESS_MESSAGE }); + throw new TRPCError({ + code: 'CONFLICT', + message: OPERATION_IN_PROGRESS_MESSAGE, + }); case 'takeover': case 'duplicate_reconcile_pending': return args.reconcile(admission.row); @@ -879,7 +944,10 @@ export const providerReviewRouter = createTRPCRouter({ getFileLines: baseProcedure.input(GetFileLinesInput).query(async ({ ctx, input }) => { if (input.endLine < input.startLine) { - throw new TRPCError({ code: 'BAD_REQUEST', message: 'endLine must be >= startLine' }); + throw new TRPCError({ + code: 'BAD_REQUEST', + message: 'endLine must be >= startLine', + }); } if (input.platform === 'gitlab') { const owner = await gitlabOwner(ctx, input); @@ -975,16 +1043,25 @@ export const providerReviewRouter = createTRPCRouter({ ); }), - /** Post a top-level comment. UGC-gated and ledgered like the GitHub path. */ + /** Post a comment. With an `anchor` it becomes a real inline discussion. */ addComment: baseProcedure.input(AddCommentInput).mutation(async ({ ctx, input }) => { await assertTermsAccepted(ctx.user.id); + const anchorFields = { + path: input.anchor?.path, + line: input.anchor?.line, + side: input.anchor?.side, + startLine: input.anchor?.startLine, + }; if (input.platform === 'gitlab') { const owner = await gitlabOwner(ctx, input); return runProviderMutation({ ctx, ref: providerRef(input), intent: 'create_review_comment', - fingerprintInput: gitlabFingerprintInput(input, { body: input.body }), + fingerprintInput: gitlabFingerprintInput(input, { + body: input.body, + ...anchorFields, + }), operationKey: input.operationKey, write: () => gitlabAddComment({ @@ -993,6 +1070,7 @@ export const providerReviewRouter = createTRPCRouter({ mrIid: input.mrIid, instanceHint: input.instanceHint, body: input.body, + ...(input.anchor ? { anchor: input.anchor } : {}), }), reconcileAmbiguous: true, }); @@ -1002,7 +1080,10 @@ export const providerReviewRouter = createTRPCRouter({ ctx, ref: providerRef(input), intent: 'create_review_comment', - fingerprintInput: bitbucketFingerprintInput(input, { body: input.body }), + fingerprintInput: bitbucketFingerprintInput(input, { + body: input.body, + ...anchorFields, + }), operationKey: input.operationKey, write: () => bitbucketAddComment({ @@ -1011,6 +1092,7 @@ export const providerReviewRouter = createTRPCRouter({ repoSlug: input.repoSlug, prId: input.prId, body: input.body, + ...(input.anchor ? { anchor: input.anchor } : {}), }), reconcileAmbiguous: true, }); @@ -1068,6 +1150,8 @@ export const providerReviewRouter = createTRPCRouter({ /** * Submit a review. GitLab has no request-changes event: the write layer * refuses it with the exact reason (BAD_REQUEST), never a silent fallback. + * An optional `comments` batch lands as real inline discussions before the + * review state/summary note. */ submitReview: baseProcedure.input(SubmitReviewInput).mutation(async ({ ctx, input }) => { await assertTermsAccepted(ctx.user.id); @@ -1080,6 +1164,7 @@ export const providerReviewRouter = createTRPCRouter({ fingerprintInput: gitlabFingerprintInput(input, { event: input.event, body: input.body, + comments: input.comments, }), operationKey: input.operationKey, write: () => @@ -1090,6 +1175,7 @@ export const providerReviewRouter = createTRPCRouter({ instanceHint: input.instanceHint, event: input.event, body: input.body, + ...(input.comments ? { comments: input.comments } : {}), }), reconcileAmbiguous: true, }); @@ -1102,6 +1188,7 @@ export const providerReviewRouter = createTRPCRouter({ fingerprintInput: bitbucketFingerprintInput(input, { event: input.event, body: input.body, + comments: input.comments, }), operationKey: input.operationKey, write: () => @@ -1112,6 +1199,7 @@ export const providerReviewRouter = createTRPCRouter({ prId: input.prId, event: input.event, body: input.body, + ...(input.comments ? { comments: input.comments } : {}), }), reconcileAmbiguous: true, }); @@ -1124,7 +1212,9 @@ export const providerReviewRouter = createTRPCRouter({ ctx, ref: providerRef(input), intent: 'resolve_thread', - fingerprintInput: gitlabFingerprintInput(input, { threadId: input.discussionId }), + fingerprintInput: gitlabFingerprintInput(input, { + threadId: input.discussionId, + }), operationKey: input.operationKey, write: () => gitlabResolveThread({ @@ -1144,7 +1234,9 @@ export const providerReviewRouter = createTRPCRouter({ ctx, ref: providerRef(input), intent: 'resolve_thread', - fingerprintInput: bitbucketFingerprintInput(input, { threadId: input.threadId }), + fingerprintInput: bitbucketFingerprintInput(input, { + threadId: input.threadId, + }), operationKey: input.operationKey, write: () => bitbucketResolveThread({ @@ -1165,7 +1257,9 @@ export const providerReviewRouter = createTRPCRouter({ ctx, ref: providerRef(input), intent: 'unresolve_thread', - fingerprintInput: gitlabFingerprintInput(input, { threadId: input.discussionId }), + fingerprintInput: gitlabFingerprintInput(input, { + threadId: input.discussionId, + }), operationKey: input.operationKey, write: () => gitlabUnresolveThread({ @@ -1183,7 +1277,9 @@ export const providerReviewRouter = createTRPCRouter({ ctx, ref: providerRef(input), intent: 'unresolve_thread', - fingerprintInput: bitbucketFingerprintInput(input, { threadId: input.threadId }), + fingerprintInput: bitbucketFingerprintInput(input, { + threadId: input.threadId, + }), operationKey: input.operationKey, write: () => bitbucketUnresolveThread({ diff --git a/packages/app-shared/src/provider-review/contracts.ts b/packages/app-shared/src/provider-review/contracts.ts index 988aeb99e9..5f9e523635 100644 --- a/packages/app-shared/src/provider-review/contracts.ts +++ b/packages/app-shared/src/provider-review/contracts.ts @@ -98,6 +98,24 @@ export type ProviderPrState = 'open' | 'closed' | 'merged'; /** Which side of a diff a comment or thread anchors to. */ export type ProviderPrDiffSide = 'LEFT' | 'RIGHT'; +/** + * The diff position one inline review comment anchors to. `line` is the + * anchor line on `side`; `startLine` marks the first line of a multi-line + * range (GitHub parity: GitLab diff discussions and Bitbucket inline + * comments both accept this shape). + */ +export type ProviderReviewInlineAnchor = { + path: string; + side: ProviderPrDiffSide; + line: number; + startLine?: number; +}; + +/** One inline comment inside a review submission batch. */ +export type ProviderReviewInlineComment = ProviderReviewInlineAnchor & { + body: string; +}; + /** * One PR/MR as the review screen renders it. Field shapes mirror what the * mobile tree consumes today from `githubPrReview` (title, author, state,