diff --git a/apps/mobile/.kilo/WORKFLOW_LEARNINGS.md b/apps/mobile/.kilo/WORKFLOW_LEARNINGS.md index b74b469509..7df3d1bba9 100644 --- a/apps/mobile/.kilo/WORKFLOW_LEARNINGS.md +++ b/apps/mobile/.kilo/WORKFLOW_LEARNINGS.md @@ -70,6 +70,23 @@ Then wait event-driven with an `until grep -q EXITCODE= "$LOG"` loop that also b ## Orchestrator +### PR-review E2E env traps (stub token path) + +- **Symptom:** `githubApps.devSeedUserGithubToken` fails or the app shows + `GitHub connection expired` even after seeding; the GitHub stub never logs + a request. +- **Cause:** (1) worktree `.env.local` missing the `USER_GITHUB_APP_TOKEN_*` + encryption keys, so the seeded token cannot be encrypted/decrypted; (2) + `services/git-token-service/.dev.vars` has empty token keys, so the token + endpoint returns 503. +- **Fix:** copy the missing `USER_GITHUB_APP_TOKEN_*` key lines from the + primary checkout's `.env.local` (temporary, strip after the run), fill the + empty keys in `services/git-token-service/.dev.vars`, restart + git-token-service + nextjs, re-seed, then reopen the PR in the app (a + "Check connection" retry alone may not refetch after the first 412s). +- **Also:** iOS shows an `Allow Paste` prompt before the PR-URL paste lands; + and the Safari `Open this page in "Kilo"?` wording can differ from the + settle-app regex — tap the exact `Open` accessibility action instead. ### iOS 26.5 scheme-confirmation prompt wording breaks login.sh on a fresh install - Symptom: on a freshly installed dev client, `apps/mobile/e2e/login.sh` fails its settle assertion with the simulator stuck on the home screen under a SpringBoard dialog `Open in "Kilo"?` (Cancel/Open). diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx new file mode 100644 index 0000000000..df79acf0aa --- /dev/null +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx @@ -0,0 +1,224 @@ +// P1-F-46b: the "Finish review" affordance on the Files tab must be +// reachable regardless of pending-comment count, so a clean PR (0 +// queued comments) can still be approved. The downstream submit sheet +// + `buildSubmitReviewInput` already support a clean approve; see +// `src/lib/pr-review/build-submit-review-input.test.ts` for the +// builder coverage. This file only covers the Files-tab reachability +// wiring (button present + navigates to the submit route with the +// right params). +// +// Mutation-inversion gate: temporarily re-gating on +// `pending.items.length > 0` (or removing the button entirely) must +// make the "0 pending" case below FAIL. + +import * as React from 'react'; +import { describe, expect, it, vi } from 'vitest'; + +import { PrDiffFloatingActions } from './pr-diff-floating-actions'; +import { type PendingReviewItem } from '@/lib/pr-review/pending-review-provider'; +import { type SelectionState } from '@/lib/pr-review/diff-selection'; + +const routerPush = vi.fn(); + +vi.mock('expo-router', () => ({ + useRouter: () => ({ push: routerPush }), +})); + +vi.mock('react-native', () => ({ + View: 'View', + Platform: { OS: 'ios' }, +})); + +vi.mock('lucide-react-native', () => ({ + MessageCirclePlus: () => null, +})); + +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ + primaryForeground: '#FFFFFF', + foreground: '#000000', + mutedForeground: '#6F6A61', + }), +})); + +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/lib/pr-review/diff-selection-bridge', () => ({ + clearDiffSelection: vi.fn(), +})); + +type PendingValue = { + items: PendingReviewItem[]; + addComment: (item: PendingReviewItem) => void; + updateComment: (id: string, body: string) => void; + removeComment: (id: string) => void; + clear: () => void; +}; + +let currentPending: PendingValue = { + items: [], + addComment: vi.fn(() => undefined), + updateComment: vi.fn(() => undefined), + removeComment: vi.fn(() => undefined), + clear: vi.fn(() => undefined), +}; + +vi.mock('@/lib/pr-review/pending-review-provider', () => ({ + usePendingReview: () => currentPending, +})); + +const baseProps = { + owner: 'octocat', + repo: 'hello', + number: 7, + viewMode: 'unified' as const, + selection: null as SelectionState | null, + onClearSelection: vi.fn(), +}; + +type FindElementArgs = { + node: unknown; + type: string; + prop: string; + value: unknown; +}; + +function findElement({ node, type, prop, value }: FindElementArgs): React.ReactElement | null { + if (React.isValidElement(node)) { + const element = node; + const props = element.props as Record; + if (element.type === type && props[prop] === value) { + return element; + } + const children = props.children; + if (Array.isArray(children)) { + for (const child of children) { + const found = findElement({ node: child, type, prop, value }); + if (found) { + return found; + } + } + } else if (children !== undefined && children !== null) { + const found = findElement({ node: children, type, prop, value }); + if (found) { + return found; + } + } + } + if (Array.isArray(node)) { + for (const child of node) { + const found = findElement({ node: child, type, prop, value }); + if (found) { + return found; + } + } + } + return null; +} + +function findSubmitButton() { + // eslint-disable-next-line new-cap + const element = PrDiffFloatingActions(baseProps); + return findElement({ + node: element, + type: 'Button', + prop: 'accessibilityLabel', + value: 'Finish review', + }); +} + +function pressSubmit() { + const button = findSubmitButton(); + if (!button) { + throw new Error('Finish review button not found in rendered tree'); + } + const onPress = (button.props as { onPress?: () => void }).onPress; + onPress?.(); + return button; +} + +function makeItem(overrides: Partial = {}): PendingReviewItem { + return { + id: 'id-1', + path: 'src/lib.ts', + side: 'RIGHT', + line: 7, + body: 'Looks good.', + commitSha: 'head-1', + ...overrides, + }; +} + +describe('PrDiffFloatingActions submit reachability (P1-F-46b)', () => { + function emptyPending(): PendingValue { + return { + items: [], + addComment: vi.fn(() => undefined), + updateComment: vi.fn(() => undefined), + removeComment: vi.fn(() => undefined), + clear: vi.fn(() => undefined), + }; + } + function pendingWithItems(items: PendingReviewItem[]): PendingValue { + return { + items, + addComment: vi.fn(() => undefined), + updateComment: vi.fn(() => undefined), + removeComment: vi.fn(() => undefined), + clear: vi.fn(() => undefined), + }; + } + + it('renders the Finish review button when the queue is empty (clean PR)', () => { + currentPending = emptyPending(); + + const button = findSubmitButton(); + expect(button).not.toBeNull(); + }); + + it('renders the Finish review button when the queue has pending items', () => { + currentPending = pendingWithItems([makeItem(), makeItem({ id: 'id-2', path: 'src/other.ts' })]); + + const button = findSubmitButton(); + expect(button).not.toBeNull(); + }); + + it('does not render a numeric count badge when the queue is empty', () => { + currentPending = emptyPending(); + + // Render the tree once to build the React element, then re-render + // and assert no child text node renders the number "0" inside the + // badge slot. + pressSubmit(); + // eslint-disable-next-line new-cap + const tree = PrDiffFloatingActions(baseProps); + const serialized = JSON.stringify(tree); + expect(serialized).not.toContain('"text":"0"'); + }); + + it('navigates to the review-submit route with owner/repo/number on press (clean PR)', () => { + currentPending = emptyPending(); + routerPush.mockClear(); + + pressSubmit(); + + expect(routerPush).toHaveBeenCalledTimes(1); + expect(routerPush).toHaveBeenCalledWith({ + pathname: '/(app)/pr-review/[owner]/[repo]/[number]/review-submit', + params: { owner: 'octocat', repo: 'hello', number: 7 }, + }); + }); + + it('navigates to the review-submit route with owner/repo/number on press (with pending)', () => { + currentPending = pendingWithItems([makeItem()]); + routerPush.mockClear(); + + pressSubmit(); + + expect(routerPush).toHaveBeenCalledTimes(1); + expect(routerPush).toHaveBeenCalledWith({ + pathname: '/(app)/pr-review/[owner]/[repo]/[number]/review-submit', + params: { owner: 'octocat', repo: 'hello', number: 7 }, + }); + }); +}); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx index 1c7ceb12b0..f4e4312b43 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx @@ -2,8 +2,10 @@ // - The "Comment" affordance that pushes the comment-composer route // when a diff-line selection exists, plus a "Clear" button that // drops the selection. -// - The "Finish review" button shown when the pending review queue -// is non-empty, which pushes the review-submit route. +// - The "Finish review" button that pushes the review-submit route, +// shown regardless of pending-comment count so a clean PR can still +// be approved. The numeric count badge only renders when the queue +// is non-empty. // // Extracted from `pr-diff-file-list.tsx` to keep that file under the // 300-line repo cap. @@ -49,10 +51,10 @@ export function PrDiffFloatingActions({ const pending = usePendingReview(); const showSelectionAction = viewMode === 'unified' && selection !== null; - const showFinishReview = pending.items.length > 0; - if (!showSelectionAction && !showFinishReview) { - return null; - } + // P1-F-46b: the submit affordance must always be reachable from the + // Files tab, even when the pending-comment queue is empty (clean + // approve). The numeric count badge is only rendered when the queue + // is non-empty (see below), so a "0" never shows. function openCommentComposer() { if (!selection) { @@ -113,20 +115,20 @@ export function PrDiffFloatingActions({ ) : null} - {showFinishReview ? ( - - ) : null} + ) : null} + + ); diff --git a/apps/mobile/src/components/pr-review/merge/pr-merge-partial-success-banner.test.ts b/apps/mobile/src/components/pr-review/merge/pr-merge-partial-success-banner.test.ts new file mode 100644 index 0000000000..e6e839dd0f --- /dev/null +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-partial-success-banner.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, it, vi } from 'vitest'; + +import { PrMergePartialSuccessBanner as banner } from './pr-merge-partial-success-banner'; + +vi.mock('react-native', () => ({ + View: 'View', +})); + +vi.mock('@/components/ui/text', () => ({ + Text: 'Text', +})); + +const REASON = 'Reference does not exist'; + +describe('PrMergePartialSuccessBanner', () => { + it('renders the merge-success headline and the branch-delete failure reason', () => { + const element = banner({ reason: REASON }); + const serialized = JSON.stringify(element); + + expect(serialized).toContain('Merged'); + expect(serialized).toContain(`Couldn't delete the branch: ${REASON}`); + expect(serialized).toContain('polite'); + }); + + it('contains NO Button or Pressable (no destructive CTA — there is nothing to retry or undo)', () => { + const element = banner({ reason: REASON }); + const serialized = JSON.stringify(element); + + expect(serialized).not.toContain('Button'); + expect(serialized).not.toContain('Pressable'); + }); +}); diff --git a/apps/mobile/src/components/pr-review/merge/pr-merge-partial-success-banner.tsx b/apps/mobile/src/components/pr-review/merge/pr-merge-partial-success-banner.tsx new file mode 100644 index 0000000000..8165d7ac9c --- /dev/null +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-partial-success-banner.tsx @@ -0,0 +1,27 @@ +// Persistent partial-success banner surfaced on the PR review screen +// after a merge whose branch-delete step failed. The merge itself +// SUCCEEDED — this banner is informational only and intentionally has +// NO destructive CTA (the user already merged; re-running the merge +// would 422 and there is no rollback from the client side). +// +// Styling mirrors `AutoMergeEnabledBanner` so the two read as +// siblings; tone is "soft" / accent, not destructive. + +import { View } from 'react-native'; + +import { Text } from '@/components/ui/text'; + +export function PrMergePartialSuccessBanner({ reason }: Readonly<{ reason: string }>) { + return ( + + Merged + + {`Couldn't delete the branch: ${reason}`} + + + ); +} diff --git a/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.test.tsx b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.test.tsx new file mode 100644 index 0000000000..7e5be04828 --- /dev/null +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.test.tsx @@ -0,0 +1,291 @@ +import * as React from 'react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import * as Haptics from 'expo-haptics'; +import { PrMergeSheet } from './pr-merge-sheet'; +import { + __resetMergePartialSuccessStoreForTests, + consumeMergePartialSuccess, +} from '@/lib/pr-review/merge/merge-result-banner-store'; +import { type PrOverviewRepoSettings } from '@/lib/pr-review/merge/merge-blocked-reasons'; +import { MergeNotCompletedError } from '@/lib/pr-review/merge/merge-result-error'; + +const mergeMutationMocks = vi.hoisted(() => ({ + mutateAsync: vi.fn<() => Promise>(), + isPending: false, + error: null as Error | null, +})); + +const autoMergeMutationMocks = vi.hoisted(() => ({ + mutateAsync: vi.fn<() => Promise>(), + isPending: false, + error: null as Error | null, +})); + +vi.mock('react', async () => { + const actual = await vi.importActual('react'); + return { + ...actual, + useState: vi.fn( + (initial: T) => [initial, vi.fn() as () => void] as [T, (value: T) => void] + ), + useMemo: vi.fn((factory: () => T) => factory()), + useRef: vi.fn((initial: T) => { + const ref: React.RefObject = { current: initial }; + return ref; + }), + useEffect: vi.fn((effect: React.EffectCallback) => { + effect(); + }), + useCallback: vi.fn( unknown>(fn: T) => fn), + }; +}); + +vi.mock('react-native', () => ({ + Alert: { + alert: vi.fn( + ( + _title: string, + _message: string, + buttons: readonly { style?: string; onPress?: () => void }[] + ) => { + const destructive = buttons.find(b => b.style === 'destructive'); + destructive?.onPress?.(); + } + ), + }, + ScrollView: 'ScrollView', + View: 'View', + TextInput: 'TextInput', + Switch: 'Switch', + Platform: { OS: 'ios' }, + Keyboard: { addListener: vi.fn(() => ({ remove: vi.fn() })) }, + useWindowDimensions: () => ({ height: 800, width: 400 }), +})); + +vi.mock('expo-haptics', () => ({ + notificationAsync: vi.fn(), + NotificationFeedbackType: { Success: 'Success' }, +})); + +vi.mock('sonner-native', () => ({ + toast: { error: vi.fn() }, +})); + +vi.mock('@/lib/pr-review/merge/use-pr-merge-mutations', () => ({ + useMergePullRequestMutation: () => ({ + mutateAsync: mergeMutationMocks.mutateAsync, + isPending: mergeMutationMocks.isPending, + error: mergeMutationMocks.error, + }), + useEnableAutoMergeMutation: () => ({ + mutateAsync: autoMergeMutationMocks.mutateAsync, + isPending: autoMergeMutationMocks.isPending, + error: autoMergeMutationMocks.error, + }), + useUpdateBranchMutation: () => ({}), + useDisableAutoMergeMutation: () => ({}), +})); + +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/components/pr-review/pr-review-reconnect-notice', () => ({ + PrReviewReconnectNotice: 'PrReviewReconnectNotice', +})); +vi.mock('@/components/pr-review/pr-form-sheet-chrome', () => ({ + PrFormSheetHeader: 'PrFormSheetHeader', +})); +vi.mock('@/components/pr-review/merge/pr-merge-icons', () => ({ + defaultMergeMethodOptionFor: () => 'squash', + mergeMethodOptionsFor: () => [ + { value: 'merge', label: 'Merge', icon: 'merge' }, + { value: 'squash', label: 'Squash', icon: 'squash' }, + { value: 'rebase', label: 'Rebase', icon: 'rebase' }, + ], +})); +vi.mock('@/components/pr-review/merge/pr-merge-sheet-parts', () => ({ + CommitMessageField: 'CommitMessageField', + CommitTitleField: 'CommitTitleField', + DeleteBranchToggle: 'DeleteBranchToggle', + MergeSheetFormBody: 'MergeSheetFormBody', + MethodPicker: 'MethodPicker', +})); +vi.mock('@/lib/pr-review/merge/merge-commit-defaults', () => ({ + defaultCommitTitle: (title: string, number: number) => + `Merge pull request #${number} from ${title}`, + defaultCommitMessage: () => '', +})); + +const REF = { owner: 'octocat', repo: 'hello', number: 1 }; + +const repoSettings: PrOverviewRepoSettings = { + allowMergeCommit: true, + allowSquashMerge: true, + allowRebaseMerge: true, + allowAutoMerge: true, + deleteBranchOnMerge: true, + allowUpdateBranch: true, + viewerCanPush: true, + viewerCanAdmin: true, +}; + +const baseProps = { + owner: 'octocat', + repoName: 'hello', + number: 1, + headSha: 'a'.repeat(40), + headRef: 'feature/x', + isCrossRepo: false, + prNodeId: 'pr-node-1', + title: 'Feature', + bodyMarkdown: null, + baseRef: 'main', + repo: repoSettings, + initialMethod: 'squash' as const, + mode: 'merge' as const, + sheetTitle: 'Merge pull request', + eyebrow: 'octocat/hello#1', + onRefetch: vi.fn().mockResolvedValue(undefined), + onDismiss: vi.fn(), +}; + +type FindElementArgs = { + node: unknown; + type: string; + prop: string; + value: unknown; +}; + +function findElement({ node, type, prop, value }: FindElementArgs): React.ReactElement | null { + if (React.isValidElement(node)) { + const element = node; + const props = element.props as Record; + if (element.type === type && props[prop] === value) { + return element; + } + const children = props.children; + if (Array.isArray(children)) { + for (const child of children) { + const found = findElement({ node: child, type, prop, value }); + if (found) { + return found; + } + } + } else if (children !== undefined && children !== null) { + const found = findElement({ node: children, type, prop, value }); + if (found) { + return found; + } + } + } + if (Array.isArray(node)) { + for (const child of node) { + const found = findElement({ node: child, type, prop, value }); + if (found) { + return found; + } + } + } + return null; +} + +function pressMerge(props: typeof baseProps) { + // eslint-disable-next-line new-cap + const element = PrMergeSheet(props); + // The submit CTA lives inside MergeSheetFormBody (mocked as a string + // element); the sheet wires its confirm handler as the `onConfirm` prop. + // Invoking it drives the same Alert → destructive-confirm → performSubmit + // path the production Merge button press takes. + const formBody = findElement({ + node: element, + type: 'MergeSheetFormBody', + prop: 'submitLabel', + value: 'Merge', + }); + if (!formBody) { + throw new Error('MergeSheetFormBody not found in rendered tree'); + } + const onConfirm = (formBody.props as { onConfirm?: () => void }).onConfirm; + onConfirm?.(); + return element; +} + +async function flushMicrotasks() { + await new Promise(resolve => { + setTimeout(() => { + resolve(undefined); + }, 0); + }); +} + +describe('PrMergeSheet performSubmit wiring (P0-B-08)', () => { + beforeEach(() => { + __resetMergePartialSuccessStoreForTests(); + mergeMutationMocks.mutateAsync.mockReset(); + mergeMutationMocks.error = null; + autoMergeMutationMocks.mutateAsync.mockReset(); + autoMergeMutationMocks.error = null; + vi.clearAllMocks(); + }); + + it('partial success (merged:true + branchDeleteError) writes the banner, fires haptic, and dismisses', async () => { + const onDismiss = vi.fn(); + const onRefetch = vi.fn().mockResolvedValue(undefined); + const props = { ...baseProps, onDismiss, onRefetch }; + + mergeMutationMocks.mutateAsync.mockResolvedValueOnce({ + merged: true, + sha: 'mergedsha', + branchDeleted: false, + branchDeleteError: 'Reference does not exist', + }); + + pressMerge(props); + await flushMicrotasks(); + + expect(consumeMergePartialSuccess(REF)).toEqual({ reason: 'Reference does not exist' }); + expect(Haptics.notificationAsync).toHaveBeenCalledWith( + Haptics.NotificationFeedbackType.Success + ); + expect(onRefetch).toHaveBeenCalledTimes(1); + expect(onDismiss).toHaveBeenCalledTimes(1); + }); + + it('clean success (merged:true + branchDeleted:true) fires haptic and dismisses without writing a banner', async () => { + const onDismiss = vi.fn(); + const onRefetch = vi.fn().mockResolvedValue(undefined); + const props = { ...baseProps, onDismiss, onRefetch }; + + mergeMutationMocks.mutateAsync.mockResolvedValueOnce({ + merged: true, + sha: 'mergedsha', + branchDeleted: true, + }); + + pressMerge(props); + await flushMicrotasks(); + + expect(consumeMergePartialSuccess(REF)).toBeNull(); + expect(Haptics.notificationAsync).toHaveBeenCalledWith( + Haptics.NotificationFeedbackType.Success + ); + expect(onRefetch).toHaveBeenCalledTimes(1); + expect(onDismiss).toHaveBeenCalledTimes(1); + }); + + it('rejected mutation (merged:false) does not fire haptic, refetch, dismiss, or write a banner', async () => { + const onDismiss = vi.fn(); + const onRefetch = vi.fn().mockResolvedValue(undefined); + const props = { ...baseProps, onDismiss, onRefetch }; + + mergeMutationMocks.mutateAsync.mockRejectedValueOnce(new MergeNotCompletedError({ sha: 's1' })); + + pressMerge(props); + await flushMicrotasks(); + + expect(consumeMergePartialSuccess(REF)).toBeNull(); + expect(Haptics.notificationAsync).not.toHaveBeenCalled(); + expect(onRefetch).not.toHaveBeenCalled(); + expect(onDismiss).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.tsx b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.tsx index a5e5303413..07d963e3b8 100644 --- a/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.tsx +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.tsx @@ -27,6 +27,7 @@ import { useMergePullRequestMutation, } from '@/lib/pr-review/merge/use-pr-merge-mutations'; import { classifyPrReviewMutationError } from '@/lib/pr-review/classify-pr-review-query-state'; +import { applyMergeSuccessEffects } from '@/lib/pr-review/merge/merge-success-effects'; import { defaultMergeMethodOptionFor, mergeMethodOptionsFor, @@ -71,8 +72,6 @@ type MergePullRequestInput = { commitMessage?: string; deleteBranch: boolean; expectedHeadSha: string; - headRef: string; - isCrossRepo: boolean; }; type AutoMergeInput = { @@ -91,7 +90,6 @@ export function PrMergeSheet(props: PrMergeSheetProps) { repoName, number, headSha, - headRef, isCrossRepo, prNodeId, title, @@ -196,8 +194,6 @@ export function PrMergeSheet(props: PrMergeSheetProps) { commitMessage: messageRef.current.trim().length > 0 ? messageRef.current.trim() : undefined, deleteBranch: showDeleteBranchToggle ? deleteBranch : false, expectedHeadSha: headSha, - headRef, - isCrossRepo, }; } @@ -226,18 +222,32 @@ export function PrMergeSheet(props: PrMergeSheetProps) { setInlineError(null); setInlineErrorKind(null); try { + let celebrate = false; // eslint-disable-next-line typescript-eslint/prefer-ternary -- awaits inside branches can't be a ternary expression if (mode === 'merge') { - await mergeMutation.mutateAsync(buildMergeInput()); + // P0-B-08: only resolved here when `merged: true` (the hook's + // `assertMergeResult` throws on `merged: false` so a "not + // mergeable" reply is treated as a retryable mutation error, + // NOT a success). The pure helper decides whether the post-merge + // step (branch delete) is a partial success that needs a + // persistent banner on the PR review screen, then the sheet + // celebrates in BOTH clean and partial cases. The `incomplete` + // gate never reaches here because `mutateAsync` would have + // rejected. + const result = await mergeMutation.mutateAsync(buildMergeInput()); + ({ celebrate } = applyMergeSuccessEffects(result, ref)); } else { await enableAutoMergeMutation.mutateAsync(buildAutoMergeInput()); + celebrate = true; + } + if (celebrate) { + void Haptics.notificationAsync(Haptics.NotificationFeedbackType.Success); + await onRefetch(); + // Dismiss exactly this merge route; `onDismiss` (router.back) leaves the + // refreshed PR review screen visible. Do NOT also call router.back() + // here or it would pop the review screen too. + onDismiss(); } - void Haptics.notificationAsync(Haptics.NotificationFeedbackType.Success); - await onRefetch(); - // Dismiss exactly this merge route; `onDismiss` (router.back) leaves the - // refreshed PR review screen visible. Do NOT also call router.back() - // here or it would pop the review screen too. - onDismiss(); } catch { // The effect above classifies the mutation error into inlineError; // swallow here to avoid an unhandled promise rejection. diff --git a/apps/mobile/src/components/pr-review/pr-review-screen.test.tsx b/apps/mobile/src/components/pr-review/pr-review-screen.test.tsx new file mode 100644 index 0000000000..ee50cd7a4b --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-screen.test.tsx @@ -0,0 +1,231 @@ +// P1-F-46b: the "Submit review" affordance must be reachable from the +// Overview tab (header right) and the Files tab (floating action bar, +// see `pr-diff-floating-actions.test.tsx`). The Discussion tab is +// intentionally left without a submit affordance. +// +// This test renders the screen shell as a plain function call (the +// same pattern used by `pr-merge-sheet.test.tsx`) and walks the +// resulting tree to assert which affordances are present per tab. +// React hooks are stubbed so the call is a no-op, and every child +// component is mocked to a string node so the tree walk stays +// deterministic. + +import * as React from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { PrReviewScreen } from './pr-review-screen'; +import { type PendingReviewItem } from '@/lib/pr-review/pending-review-provider'; + +const routerPush = vi.fn(); +const routerBack = vi.fn(); +const routerCanGoBack = vi.fn(() => true); + +vi.mock('react', async () => { + const actual = await vi.importActual('react'); + return { + ...actual, + useState: vi.fn( + (initial: T) => [initial, vi.fn() as () => void] as [T, (value: T) => void] + ), + useMemo: vi.fn((factory: () => T) => factory()), + useRef: vi.fn((initial: T) => { + const ref: React.RefObject = { current: initial }; + return ref; + }), + useEffect: vi.fn((_effect: React.EffectCallback) => { + // no-op; the recents backfill and merge banner focus effect + // aren't part of P1-F-46b's reachability contract. + }), + useCallback: vi.fn( unknown>(fn: T) => fn), + }; +}); + +vi.mock('expo-router', () => ({ + useFocusEffect: vi.fn(), + useRouter: () => ({ push: routerPush, back: routerBack, canGoBack: routerCanGoBack }), +})); + +vi.mock('react-native', () => ({ + RefreshControl: 'RefreshControl', + ScrollView: 'ScrollView', + View: 'View', + Platform: { OS: 'ios' }, +})); + +vi.mock('@tanstack/react-query', () => ({ + useQuery: () => ({ data: undefined, isLoading: true, isError: false, isFetching: false }), + useQueryClient: () => ({ invalidateQueries: vi.fn() }), +})); + +vi.mock('lucide-react-native', () => ({ + Check: () => null, +})); + +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ + primaryForeground: '#FFFFFF', + foreground: '#000000', + mutedForeground: '#6F6A61', + }), +})); + +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => ({ + githubPrReview: { + getPullRequest: { queryOptions: () => ({}), queryKey: () => [] }, + listChecks: { queryKey: () => [] }, + }, + githubApps: { getUserAuthorization: { queryKey: () => [] } }, + }), +})); + +vi.mock('@/lib/pr-review/merge/merge-result-banner-store', () => ({ + consumeMergePartialSuccess: () => null, +})); + +vi.mock('@/lib/pr-review/recent-prs', () => ({ + upsertRecentPr: vi.fn(), +})); + +vi.mock('@/components/screen-header', () => { + // The screen passes `headerRight` as a named slot prop. We render it + // alongside the `children` slot so the tree walk can find the + // Submit-review Button inside. + const MockScreenHeader = (props: { + headerRight?: React.ReactNode; + children?: React.ReactNode; + }): React.ReactElement => + React.createElement( + 'ScreenHeader', + { hasHeaderRight: props.headerRight != null }, + props.headerRight, + props.children + ); + return { ScreenHeader: MockScreenHeader }; +}); +vi.mock('@/components/pr-review/merge/pr-merge-partial-success-banner', () => ({ + PrMergePartialSuccessBanner: 'PrMergePartialSuccessBanner', +})); +vi.mock('@/components/pr-review/pr-review-discussion-tab', () => ({ + PrReviewDiscussionTab: 'PrReviewDiscussionTab', +})); +vi.mock('@/components/pr-review/pr-review-files-tab', () => ({ + PrReviewFilesTab: 'PrReviewFilesTab', +})); +vi.mock('@/components/pr-review/pr-review-overview', () => ({ + PrReviewOverview: 'PrReviewOverview', +})); +vi.mock('@/components/pr-review/pr-review-tab-selector', () => ({ + PrReviewTabSelector: 'PrReviewTabSelector', +})); +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); + +// PendingReviewProvider is not used by PrReviewScreen directly, but +// the floating-actions test mocks this module; the screen import of +// @/lib/hooks/use-theme-colors already covers what we need. No-op +// stub here keeps the module resolvable in case any transitive import +// touches it. +vi.mock('@/lib/pr-review/pending-review-provider', () => ({ + usePendingReview: () => ({ + items: [] as PendingReviewItem[], + addComment: vi.fn(() => undefined), + updateComment: vi.fn(() => undefined), + removeComment: vi.fn(() => undefined), + clear: vi.fn(() => undefined), + }), +})); + +type FindElementArgs = { + node: unknown; + type: string; + prop: string; + value: unknown; +}; + +function findElement({ node, type, prop, value }: FindElementArgs): React.ReactElement | null { + if (React.isValidElement(node)) { + const element = node; + const props = element.props as Record; + if (element.type === type && props[prop] === value) { + return element; + } + const children = props.children; + if (Array.isArray(children)) { + for (const child of children) { + const found = findElement({ node: child, type, prop, value }); + if (found) { + return found; + } + } + } else if (children !== undefined && children !== null) { + const found = findElement({ node: children, type, prop, value }); + if (found) { + return found; + } + } + // Also walk into named slot props that carry a React node (e.g. + // ScreenHeader's `headerRight`), so the reachability test can + // find a Button mounted as a named slot without knowing the + // component shape. + const slotProps: readonly string[] = ['headerRight']; + for (const slot of slotProps) { + const slotValue = props[slot]; + if (slotValue !== undefined && slotValue !== null && slotValue !== children) { + const found = findElement({ node: slotValue, type, prop, value }); + if (found) { + return found; + } + } + } + } + if (Array.isArray(node)) { + for (const child of node) { + const found = findElement({ node: child, type, prop, value }); + if (found) { + return found; + } + } + } + return null; +} + +function findScreenHeaderSubmitButton(): React.ReactElement | null { + // eslint-disable-next-line new-cap + const element = PrReviewScreen({ owner: 'octocat', repo: 'hello', number: 7 }); + return findElement({ + node: element, + type: 'Button', + prop: 'accessibilityLabel', + value: 'Submit review', + }); +} + +describe('PrReviewScreen Submit review reachability (P1-F-46b)', () => { + beforeEach(() => { + routerPush.mockClear(); + }); + afterEach(() => { + routerPush.mockReset(); + }); + + it('renders the Submit review affordance on the Overview tab', () => { + const button = findScreenHeaderSubmitButton(); + expect(button).not.toBeNull(); + }); + + it('navigates to the review-submit route with owner/repo/number on press (Overview)', () => { + const button = findScreenHeaderSubmitButton(); + if (!button) { + throw new Error('Submit review button not found on Overview tab'); + } + const onPress = (button.props as { onPress?: () => void }).onPress; + onPress?.(); + + expect(routerPush).toHaveBeenCalledTimes(1); + expect(routerPush).toHaveBeenCalledWith({ + pathname: '/(app)/pr-review/[owner]/[repo]/[number]/review-submit', + params: { owner: 'octocat', repo: 'hello', number: 7 }, + }); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-screen.tsx b/apps/mobile/src/components/pr-review/pr-review-screen.tsx index 164b35ea2f..bb31cfc6d3 100644 --- a/apps/mobile/src/components/pr-review/pr-review-screen.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-screen.tsx @@ -1,7 +1,10 @@ import { useQuery, useQueryClient } from '@tanstack/react-query'; +import { type Href, useFocusEffect, useRouter } from 'expo-router'; +import { Check } from 'lucide-react-native'; import { type ReactNode, useCallback, useEffect, useState } from 'react'; import { RefreshControl, ScrollView, View } from 'react-native'; +import { PrMergePartialSuccessBanner } from '@/components/pr-review/merge/pr-merge-partial-success-banner'; import { PrReviewDiscussionTab } from '@/components/pr-review/pr-review-discussion-tab'; import { PrReviewFilesTab } from '@/components/pr-review/pr-review-files-tab'; import { PrReviewOverview } from '@/components/pr-review/pr-review-overview'; @@ -10,8 +13,15 @@ import { PrReviewTabSelector, } from '@/components/pr-review/pr-review-tab-selector'; import { ScreenHeader } from '@/components/screen-header'; +import { Button } from '@/components/ui/button'; +import { Text } from '@/components/ui/text'; +import { consumeMergePartialSuccess } from '@/lib/pr-review/merge/merge-result-banner-store'; +import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { upsertRecentPr } from '@/lib/pr-review/recent-prs'; import { useTRPC } from '@/lib/trpc'; +import { cn } from '@/lib/utils'; + +const REVIEW_SUBMIT_PATH = '/(app)/pr-review/[owner]/[repo]/[number]/review-submit' as const; type PrReviewScreenProps = { readonly owner: string; @@ -37,9 +47,37 @@ type PrReviewScreenProps = { export function PrReviewScreen({ owner, repo, number }: PrReviewScreenProps) { const trpc = useTRPC(); const queryClient = useQueryClient(); + const router = useRouter(); + const colors = useThemeColors(); const [tab, setTab] = useState('overview'); const [refreshing, setRefreshing] = useState(false); + // P1-F-46b: push the review-submit route with the same params the + // Files-tab `PrDiffFloatingActions` uses, so a clean PR (no queued + // comments) can still be approved from the Overview tab. + const openReviewSubmit = useCallback(() => { + const href: Href = { + pathname: REVIEW_SUBMIT_PATH, + params: { owner, repo, number }, + }; + router.push(href); + }, [router, owner, repo, number]); + + // P0-B-08: post-merge "branch delete failed" partial-success banner. + // The merge sheet writes the reason into the in-memory store right + // before dismissing; we consume it on every focus so the banner + // appears once after the user navigates back, then disappears (and + // does not re-flash on re-focus) thanks to consume-on-read semantics. + const [partialMergeReason, setPartialMergeReason] = useState(null); + useFocusEffect( + useCallback(() => { + const value = consumeMergePartialSuccess({ owner, repo, number }); + if (value) { + setPartialMergeReason(value.reason); + } + }, [owner, repo, number]) + ); + // The screen owns the PR query so it can drive the recents backfill // and pass `headSha` / `changedFiles` to the Files tab. The Overview // re-uses the same query — tanstack-query dedupes by key, so this is @@ -104,6 +142,7 @@ export function PrReviewScreen({ owner, repo, number }: PrReviewScreenProps) { keyboardShouldPersistTaps="handled" refreshControl={} > + {partialMergeReason ? : null} ); @@ -135,7 +174,27 @@ export function PrReviewScreen({ owner, repo, number }: PrReviewScreenProps) { return ( - + + + Submit review + + ) : null + } + /> diff --git a/apps/mobile/src/lib/code-reviewer-config.ts b/apps/mobile/src/lib/code-reviewer-config.ts index 7188a97bc4..0930154b89 100644 --- a/apps/mobile/src/lib/code-reviewer-config.ts +++ b/apps/mobile/src/lib/code-reviewer-config.ts @@ -6,7 +6,6 @@ import { import { parseParam } from '@/lib/route-params'; export { - buildSaveConfigInput, GATE_THRESHOLDS, REVIEW_FOCUS_AREAS, REVIEW_STYLES, diff --git a/apps/mobile/src/lib/hooks/use-code-reviewer.test.ts b/apps/mobile/src/lib/hooks/use-code-reviewer.test.ts new file mode 100644 index 0000000000..637d7de844 --- /dev/null +++ b/apps/mobile/src/lib/hooks/use-code-reviewer.test.ts @@ -0,0 +1,368 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { type ConfigPatch, PERSONAL_SCOPE } from '@/lib/code-reviewer-config'; + +import { useSaveReviewConfig } from './use-code-reviewer'; + +type MutationOptions = { + mutationFn?: (vars: unknown) => Promise; + onError?: (error: unknown) => void; + onSettled?: () => void; + onSuccess?: (data: unknown) => void; +}; + +type PersonalPatch = { + platform: string; + reviewStyle?: string; + focusAreas?: string[]; + customInstructions?: string; + modelSlug?: string; + thinkingEffort?: string | null; + gateThreshold?: string; + repositorySelectionMode?: string; + selectedRepositoryIds?: (number | string)[]; + repositoryModelOverrides?: { + repositoryId: number | string; + repoFullName: string; + modelSlug: string; + thinkingEffort?: string | null; + }[]; + disableReviewMd?: boolean; + autoConfigureWebhooks?: boolean; +}; + +type OrgPatch = PersonalPatch & { organizationId: string }; + +const personalPatchMutateMock = vi.fn(); +const orgPatchMutateMock = vi.fn(); +const personalSaveMutateMock = vi.fn(); +const orgSaveMutateMock = vi.fn(); +const invalidateQueriesMock = vi.fn(); +const cancelQueriesMock = vi.fn(); +const getQueryDataMock = vi.fn(); +const setQueryDataMock = vi.fn(); +const toastErrorMock = vi.fn(); + +let lastCapturedOptions: MutationOptions | null = null; + +vi.mock('@tanstack/react-query', () => ({ + useMutation: (opts: MutationOptions) => { + lastCapturedOptions = opts; + return { mutate: vi.fn() }; + }, + useQuery: () => ({ data: undefined }), + useQueryClient: () => ({ + cancelQueries: cancelQueriesMock, + getQueryData: getQueryDataMock, + setQueryData: setQueryDataMock, + invalidateQueries: invalidateQueriesMock, + }), +})); + +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => ({ + personalReviewAgent: { + getReviewConfig: { queryKey: () => ['personalReviewAgent', 'getReviewConfig'] }, + }, + organizations: { + reviewAgent: { + getReviewConfig: { queryKey: () => ['organizations', 'reviewAgent', 'getReviewConfig'] }, + }, + }, + }), + trpcClient: { + personalReviewAgent: { + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + patchReviewConfig: { mutate: (vars: unknown) => personalPatchMutateMock(vars) }, + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + saveReviewConfig: { mutate: (vars: unknown) => personalSaveMutateMock(vars) }, + }, + organizations: { + reviewAgent: { + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + patchReviewConfig: { mutate: (vars: unknown) => orgPatchMutateMock(vars) }, + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + saveReviewConfig: { mutate: (vars: unknown) => orgSaveMutateMock(vars) }, + }, + }, + }, +})); + +vi.mock('@/lib/a11y/announcing-toast', () => ({ + announcingToast: { error: (msg: string) => toastErrorMock(msg) }, +})); + +// use-code-reviewer.ts re-exports from use-reviewer-permission, which +// imports `useRouter` from expo-router. Loading the real module in node +// blows up on the expo-router source map, so stub just the surface the +// re-export's transitive imports actually reach. +vi.mock('expo-router', () => ({ + useRouter: () => ({ push: vi.fn(), replace: vi.fn(), back: vi.fn() }), +})); + +function getSaveOptions( + scope: string, + platform: 'github' | 'gitlab' | 'bitbucket' +): MutationOptions { + lastCapturedOptions = null; + // eslint-disable-next-line react-hooks/rules-of-hooks + useSaveReviewConfig(scope, platform); + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (!lastCapturedOptions) { + throw new Error('mutation options for useSaveReviewConfig were not captured'); + } + return lastCapturedOptions; +} + +beforeEach(() => { + lastCapturedOptions = null; + personalPatchMutateMock.mockReset(); + orgPatchMutateMock.mockReset(); + personalSaveMutateMock.mockReset(); + orgSaveMutateMock.mockReset(); + invalidateQueriesMock.mockReset(); + cancelQueriesMock.mockReset(); + getQueryDataMock.mockReset(); + setQueryDataMock.mockReset(); + toastErrorMock.mockReset(); + // Default: each patch mutate resolves to a successful payload. Tests + // override per-case when they need a different outcome. + personalPatchMutateMock.mockResolvedValue({ success: true, webhookSync: null }); + orgPatchMutateMock.mockResolvedValue({ success: true, webhookSync: null }); +}); + +afterEach(() => { + vi.clearAllMocks(); +}); + +describe('useSaveReviewConfig mutationFn payload shape', () => { + it('sends ONLY edited fields + platform for a personal github patch (NOT a full document)', async () => { + const opts = getSaveOptions(PERSONAL_SCOPE, 'github'); + + const patch: ConfigPatch = { reviewStyle: 'strict' }; + await opts.mutationFn?.(patch); + + expect(personalPatchMutateMock).toHaveBeenCalledTimes(1); + const sent = personalPatchMutateMock.mock.calls[0]?.[0] as PersonalPatch; + // Platform is always present. + expect(sent.platform).toBe('github'); + // Only the edited key reaches the wire — every other mobile-editable + // field must be absent. (A full-doc save would carry all of these.) + expect(sent).toEqual({ platform: 'github', reviewStyle: 'strict' }); + // Explicit negative assertions, since `toEqual` would pass for a + // full-doc payload that happens to also include the same keys. + expect(sent).not.toHaveProperty('focusAreas'); + expect(sent).not.toHaveProperty('customInstructions'); + expect(sent).not.toHaveProperty('modelSlug'); + expect(sent).not.toHaveProperty('thinkingEffort'); + expect(sent).not.toHaveProperty('gateThreshold'); + expect(sent).not.toHaveProperty('repositorySelectionMode'); + expect(sent).not.toHaveProperty('selectedRepositoryIds'); + expect(sent).not.toHaveProperty('repositoryModelOverrides'); + expect(sent).not.toHaveProperty('disableReviewMd'); + expect(sent).not.toHaveProperty('autoConfigureWebhooks'); + // And the route family must be the PATCH, not the legacy save. + expect(personalSaveMutateMock).not.toHaveBeenCalled(); + }); + + it('sends ONLY edited fields for a personal github multi-field patch', async () => { + const opts = getSaveOptions(PERSONAL_SCOPE, 'github'); + + const patch: ConfigPatch = { + reviewStyle: 'lenient', + focusAreas: ['security', 'performance'], + modelSlug: 'openai/gpt-5', + }; + await opts.mutationFn?.(patch); + + expect(personalPatchMutateMock).toHaveBeenCalledTimes(1); + const sent = personalPatchMutateMock.mock.calls[0]?.[0] as PersonalPatch; + expect(sent).toEqual({ + platform: 'github', + reviewStyle: 'lenient', + focusAreas: ['security', 'performance'], + modelSlug: 'openai/gpt-5', + }); + expect(sent).not.toHaveProperty('customInstructions'); + expect(sent).not.toHaveProperty('thinkingEffort'); + expect(sent).not.toHaveProperty('gateThreshold'); + expect(sent).not.toHaveProperty('repositorySelectionMode'); + expect(sent).not.toHaveProperty('selectedRepositoryIds'); + expect(sent).not.toHaveProperty('repositoryModelOverrides'); + expect(sent).not.toHaveProperty('disableReviewMd'); + expect(sent).not.toHaveProperty('autoConfigureWebhooks'); + }); + + it('narrow personal selectedRepositoryIds / repositoryModelOverrides to numeric ids and does not include them when absent from the patch', async () => { + const opts = getSaveOptions(PERSONAL_SCOPE, 'github'); + + // Patch carries both keys with mixed string/number ids (a defensive + // shape — the production UI only sends numbers, but the type permits + // strings). The personal schema rejects strings, so they must be + // filtered out before going on the wire. + const patch = { + reviewStyle: 'strict' as const, + selectedRepositoryIds: [101, 'bitbucket-uuid', 202] as (number | string)[], + repositoryModelOverrides: [ + { repositoryId: 101, repoFullName: 'a/a', modelSlug: 'm', thinkingEffort: null }, + { + repositoryId: 'bitbucket-uuid', + repoFullName: 'b/b', + modelSlug: 'm', + thinkingEffort: null, + }, + ], + }; + await opts.mutationFn?.(patch); + + const sent = personalPatchMutateMock.mock.calls[0]?.[0] as PersonalPatch; + expect(sent.platform).toBe('github'); + expect(sent.reviewStyle).toBe('strict'); + expect(sent.selectedRepositoryIds).toEqual([101, 202]); + expect(sent.repositoryModelOverrides).toEqual([ + { repositoryId: 101, repoFullName: 'a/a', modelSlug: 'm', thinkingEffort: null }, + ]); + }); + + it('does not inject selectedRepositoryIds / repositoryModelOverrides when the patch omits them', async () => { + const opts = getSaveOptions(PERSONAL_SCOPE, 'github'); + + await opts.mutationFn?.({ focusAreas: ['security'] }); + + const sent = personalPatchMutateMock.mock.calls[0]?.[0] as PersonalPatch; + expect(sent).toEqual({ platform: 'github', focusAreas: ['security'] }); + // An empty array would still be a real edit that could clobber stored + // values — the hook must not silently synthesize one. + expect(sent).not.toHaveProperty('selectedRepositoryIds'); + expect(sent).not.toHaveProperty('repositoryModelOverrides'); + }); + + it('includes autoConfigureWebhooks on a GitLab personal patch only when selectedRepositoryIds is present', async () => { + const opts = getSaveOptions(PERSONAL_SCOPE, 'gitlab'); + + // Repo-selection edit: webhook re-sync must run server-side. + await opts.mutationFn?.({ selectedRepositoryIds: [101] }); + const sentSelection = personalPatchMutateMock.mock.calls[0]?.[0] as PersonalPatch; + expect(sentSelection).toEqual({ + platform: 'gitlab', + selectedRepositoryIds: [101], + autoConfigureWebhooks: true, + }); + + // Unrelated edit: webhook re-sync must NOT run server-side (gated on + // selectedRepositoryIds being present in the patch). + personalPatchMutateMock.mockClear(); + await opts.mutationFn?.({ focusAreas: ['security'] }); + const sentUnrelated = personalPatchMutateMock.mock.calls[0]?.[0] as PersonalPatch; + expect(sentUnrelated).toEqual({ platform: 'gitlab', focusAreas: ['security'] }); + expect(sentUnrelated).not.toHaveProperty('autoConfigureWebhooks'); + }); + + it('sends ONLY edited fields + organizationId + platform for an org github patch', async () => { + const opts = getSaveOptions('org_42', 'github'); + + const patch: ConfigPatch = { gateThreshold: 'critical' }; + await opts.mutationFn?.(patch); + + expect(orgPatchMutateMock).toHaveBeenCalledTimes(1); + const sent = orgPatchMutateMock.mock.calls[0]?.[0] as OrgPatch; + expect(sent).toEqual({ + organizationId: 'org_42', + platform: 'github', + gateThreshold: 'critical', + }); + expect(sent).not.toHaveProperty('reviewStyle'); + expect(sent).not.toHaveProperty('focusAreas'); + expect(sent).not.toHaveProperty('selectedRepositoryIds'); + expect(sent).not.toHaveProperty('repositoryModelOverrides'); + expect(sent).not.toHaveProperty('autoConfigureWebhooks'); + expect(personalPatchMutateMock).not.toHaveBeenCalled(); + expect(personalSaveMutateMock).not.toHaveBeenCalled(); + expect(orgSaveMutateMock).not.toHaveBeenCalled(); + }); + + it('does NOT narrow string-id repository overrides for the org path (org schema accepts both)', async () => { + const opts = getSaveOptions('org_42', 'bitbucket'); + + const patch: ConfigPatch = { + selectedRepositoryIds: ['bitbucket-uuid-1', 'bitbucket-uuid-2'], + repositoryModelOverrides: [ + { repositoryId: 'bitbucket-uuid-1', repoFullName: 'a/a', modelSlug: 'm' }, + ], + }; + await opts.mutationFn?.(patch); + + const sent = orgPatchMutateMock.mock.calls[0]?.[0] as OrgPatch; + // String ids are preserved end-to-end on the org route. + expect(sent.selectedRepositoryIds).toEqual(['bitbucket-uuid-1', 'bitbucket-uuid-2']); + expect(sent.repositoryModelOverrides).toEqual([ + { repositoryId: 'bitbucket-uuid-1', repoFullName: 'a/a', modelSlug: 'm' }, + ]); + }); + + it('includes autoConfigureWebhooks on a GitLab org patch only when selectedRepositoryIds is present', async () => { + const opts = getSaveOptions('org_42', 'gitlab'); + + await opts.mutationFn?.({ selectedRepositoryIds: [202, 303] }); + const sentSelection = orgPatchMutateMock.mock.calls[0]?.[0] as OrgPatch; + expect(sentSelection).toEqual({ + organizationId: 'org_42', + platform: 'gitlab', + selectedRepositoryIds: [202, 303], + autoConfigureWebhooks: true, + }); + + orgPatchMutateMock.mockClear(); + await opts.mutationFn?.({ focusAreas: ['security'] }); + const sentUnrelated = orgPatchMutateMock.mock.calls[0]?.[0] as OrgPatch; + expect(sentUnrelated).toEqual({ + organizationId: 'org_42', + platform: 'gitlab', + focusAreas: ['security'], + }); + expect(sentUnrelated).not.toHaveProperty('autoConfigureWebhooks'); + }); +}); + +describe('useSaveReviewConfig onError', () => { + it('toasts the thrown error message and does not call invalidateQueries from onError', async () => { + personalPatchMutateMock.mockReset(); + personalPatchMutateMock.mockResolvedValue({ + success: false, + webhookSync: null, + }); + const opts = getSaveOptions(PERSONAL_SCOPE, 'github'); + + let thrown: unknown = null; + try { + await opts.mutationFn?.({ reviewStyle: 'strict' }); + } catch (error) { + thrown = error; + opts.onError?.(error); + } + + expect(thrown).toBeInstanceOf(Error); + expect((thrown as Error).message).toBe('Failed to save review config'); + expect(toastErrorMock).toHaveBeenCalledWith('Failed to save review config'); + // onSettled is the only place invalidateQueries should fire; onError + // must not also invalidate (would clobber a follow-up optimistic save). + expect(invalidateQueriesMock).not.toHaveBeenCalled(); + }); + + it('propagates a transport-level rejection from patchReviewConfig verbatim and still toasts it', async () => { + personalPatchMutateMock.mockReset(); + personalPatchMutateMock.mockRejectedValue(new Error('Network unreachable')); + const opts = getSaveOptions(PERSONAL_SCOPE, 'github'); + + let thrown: unknown = null; + try { + await opts.mutationFn?.({ reviewStyle: 'strict' }); + } catch (error) { + thrown = error; + opts.onError?.(error); + } + + expect((thrown as Error).message).toBe('Network unreachable'); + expect(toastErrorMock).toHaveBeenCalledWith('Network unreachable'); + }); +}); diff --git a/apps/mobile/src/lib/hooks/use-code-reviewer.ts b/apps/mobile/src/lib/hooks/use-code-reviewer.ts index b56280e2cf..ac02657698 100644 --- a/apps/mobile/src/lib/hooks/use-code-reviewer.ts +++ b/apps/mobile/src/lib/hooks/use-code-reviewer.ts @@ -2,7 +2,6 @@ import { useMutation, useQuery, useQueryClient, type UseQueryResult } from '@tan import { announcingToast } from '@/lib/a11y/announcing-toast'; import { - buildSaveConfigInput, type ConfigPatch, PERSONAL_SCOPE, type ReviewConfigData, @@ -220,37 +219,80 @@ export function useSaveReviewConfig(scope: string, platform: ReviewerPlatform) { // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule mutationFn: (patch: ConfigPatch) => // Rapid taps (e.g. toggling several focus areas in a row) each send a - // full-config snapshot; without serializing them, two in-flight saves - // for the same scope+platform can resolve out of order and the - // earlier response can stomp the later one's result. Chaining onto - // the prior in-flight save for this key keeps them in order — simple - // FIFO, no dedupe/coalescing. + // PATCH; without serializing them, two in-flight saves for the same + // scope+platform can resolve out of order and the earlier response can + // stomp the later one's optimistic state. Chaining onto the prior + // in-flight save for this key keeps them in order — simple FIFO, no + // dedupe/coalescing. chainSave(saveChainKey, async () => { - const config = queryClient.getQueryData(queryKey); - if (!config) { - throw new Error('Config not loaded yet'); - } - const input = buildSaveConfigInput(platform, config, patch); - // The personal schema only accepts numeric repository IDs (bitbucket, - // the only string-ID platform, is org-only). Filtering keeps this a - // type-safe narrowing rather than a cast; the personal branch is only - // ever reached with platform !== 'bitbucket' in practice. - const result = isPersonal(scope) - ? await trpcClient.personalReviewAgent.saveReviewConfig.mutate({ - ...input, - platform: toPersonalPlatform(platform), - selectedRepositoryIds: input.selectedRepositoryIds.filter( - (id): id is number => typeof id === 'number' - ), - // Same numeric-only narrowing as selectedRepositoryIds above. - repositoryModelOverrides: input.repositoryModelOverrides.filter( + // The PATCH only carries edited fields. Server-side field-merge + // preserves every key absent from the patch, so the mobile client + // does not need to read back the full config (or any of the + // org-only/council/manuallyAddedRepositories fields it never loaded) + // just to send a partial update. + // + // Pull each optional field off the patch individually (rather than + // spreading `patch` first) so the personal tRPC option type can see + // the already-narrowed numeric arrays — `ConfigPatch` permits + // string-id repository overrides (bitbucket is org-only by UI + // construction, but the shared type still allows them), and the + // personal PATCH schema only accepts numbers. + const { + selectedRepositoryIds: rawSelectedRepositoryIds, + repositoryModelOverrides: rawRepositoryModelOverrides, + ...restPatch + } = patch; + // The personal schema only accepts numeric repository IDs + // (bitbucket, the only string-ID platform, is org-only). Filtering + // keeps this a type-safe narrowing rather than a cast; the + // personal branch is only ever reached with platform !== + // 'bitbucket' in practice. Only include these keys when the + // incoming patch actually carries them — an empty array would + // still be a real edit and could clobber stored values. + const narrowedSelectedRepositoryIds = + rawSelectedRepositoryIds !== undefined + ? rawSelectedRepositoryIds.filter((id): id is number => typeof id === 'number') + : undefined; + const narrowedRepositoryModelOverrides = + rawRepositoryModelOverrides !== undefined + ? rawRepositoryModelOverrides.filter( (override): override is typeof override & { repositoryId: number } => typeof override.repositoryId === 'number' - ), + ) + : undefined; + // GitLab webhook re-sync is gated server-side on + // `selectedRepositoryIds` being present in the patch; we send + // `autoConfigureWebhooks: true` (mobile has no toggle) to match + // the prior always-true save behavior when a repo selection edit + // is part of the patch. + const gitlabAutoConfigure = + platform === 'gitlab' && rawSelectedRepositoryIds !== undefined + ? ({ autoConfigureWebhooks: true } as const) + : ({} as const); + + const result = isPersonal(scope) + ? await trpcClient.personalReviewAgent.patchReviewConfig.mutate({ + platform: toPersonalPlatform(platform), + ...restPatch, + ...(narrowedSelectedRepositoryIds !== undefined + ? { selectedRepositoryIds: narrowedSelectedRepositoryIds } + : {}), + ...(narrowedRepositoryModelOverrides !== undefined + ? { repositoryModelOverrides: narrowedRepositoryModelOverrides } + : {}), + ...gitlabAutoConfigure, }) - : await trpcClient.organizations.reviewAgent.saveReviewConfig.mutate({ - ...input, + : await trpcClient.organizations.reviewAgent.patchReviewConfig.mutate({ organizationId: scope, + platform, + ...restPatch, + ...(rawSelectedRepositoryIds !== undefined + ? { selectedRepositoryIds: rawSelectedRepositoryIds } + : {}), + ...(rawRepositoryModelOverrides !== undefined + ? { repositoryModelOverrides: rawRepositoryModelOverrides } + : {}), + ...gitlabAutoConfigure, }); // Same reasoning as useToggleReviewer: `success` is typed as `boolean`, // not a `true` literal, so a domain failure must throw rather than diff --git a/apps/mobile/src/lib/hooks/use-code-reviews.test.ts b/apps/mobile/src/lib/hooks/use-code-reviews.test.ts new file mode 100644 index 0000000000..aeb9b1b9e2 --- /dev/null +++ b/apps/mobile/src/lib/hooks/use-code-reviews.test.ts @@ -0,0 +1,293 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { + cancelReviewMutationFn, + createManualReviewMutationFn, + retriggerReviewMutationFn, + useCancelReview, + useCreateManualReview, + useRetriggerReview, +} from './use-code-reviews'; + +type MutationOptions = { + mutationFn?: (vars: unknown) => Promise; + onSuccess?: (data: unknown, vars: unknown) => void; + onError?: (error: unknown) => void; +}; + +const cancelMutateMock = vi.fn(); +const retriggerMutateMock = vi.fn(); +const personalCreateMutateMock = vi.fn(); +const orgCreateMutateMock = vi.fn(); +const invalidateQueriesMock = vi.fn(); +const cancelQueriesMock = vi.fn(); +const getQueryDataMock = vi.fn(); +const setQueryDataMock = vi.fn(); +const toastErrorMock = vi.fn(); + +let lastCapturedOptions: MutationOptions | null = null; + +vi.mock('@tanstack/react-query', () => ({ + useMutation: (opts: MutationOptions) => { + lastCapturedOptions = opts; + return { mutate: vi.fn() }; + }, + useQuery: () => ({ data: undefined }), + useQueryClient: () => ({ + cancelQueries: cancelQueriesMock, + getQueryData: getQueryDataMock, + setQueryData: setQueryDataMock, + invalidateQueries: invalidateQueriesMock, + }), +})); + +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => ({ + codeReviews: { + listForUser: { queryKey: () => ['codeReviews', 'listForUser'] }, + listForOrganization: { queryKey: () => ['codeReviews', 'listForOrganization'] }, + get: { queryKey: () => ['codeReviews', 'get'] }, + }, + }), + trpcClient: { + codeReviews: { + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + cancel: { mutate: (vars: unknown) => cancelMutateMock(vars) }, + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + retrigger: { mutate: (vars: unknown) => retriggerMutateMock(vars) }, + }, + personalReviewAgent: { + createManualReviewJob: { + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + mutate: (vars: unknown) => personalCreateMutateMock(vars), + }, + }, + organizations: { + reviewAgent: { + createManualReviewJob: { + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + mutate: (vars: unknown) => orgCreateMutateMock(vars), + }, + }, + }, + }, +})); + +vi.mock('@/lib/hooks/use-code-reviewer', () => ({ + PERSONAL_SCOPE: 'personal', +})); + +vi.mock('@kilocode/app-shared/code-review', () => ({ + hasInFlightReview: () => false, + isInFlightReviewStatus: () => false, +})); + +vi.mock('@/lib/a11y/announcing-toast', () => ({ + announcingToast: { error: (msg: string) => toastErrorMock(msg) }, +})); + +const CREATE_VARS = { + platform: 'github', + url: 'https://github.com/foo/bar/pull/1', + modelSlug: 'claude-opus-4-7', +} as const; + +function getOptions(hook: 'cancel' | 'retrigger' | 'create', scope = 'personal'): MutationOptions { + lastCapturedOptions = null; + if (hook === 'cancel') { + // eslint-disable-next-line react-hooks/rules-of-hooks + useCancelReview(scope); + } else if (hook === 'retrigger') { + // eslint-disable-next-line react-hooks/rules-of-hooks + useRetriggerReview(scope); + } else { + // eslint-disable-next-line react-hooks/rules-of-hooks + useCreateManualReview(scope); + } + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (!lastCapturedOptions) { + throw new Error(`mutation options for ${hook} were not captured`); + } + return lastCapturedOptions; +} + +beforeEach(() => { + lastCapturedOptions = null; + cancelMutateMock.mockReset(); + retriggerMutateMock.mockReset(); + personalCreateMutateMock.mockReset(); + orgCreateMutateMock.mockReset(); + invalidateQueriesMock.mockReset(); + cancelQueriesMock.mockReset(); + getQueryDataMock.mockReset(); + setQueryDataMock.mockReset(); + toastErrorMock.mockReset(); +}); + +afterEach(() => { + vi.clearAllMocks(); +}); + +describe('cancelReviewMutationFn', () => { + it('throws a typed error carrying the server error message on {success:false}', async () => { + cancelMutateMock.mockResolvedValue({ + success: false, + error: 'Review cannot be cancelled in its current state.', + }); + + await expect(cancelReviewMutationFn({ reviewId: 'r1' })).rejects.toThrow( + 'Review cannot be cancelled in its current state.' + ); + }); + + it('resolves with the full success payload so the mutation lifecycle continues normally', async () => { + const successPayload = { success: true, review: { id: 'r1', status: 'cancelled' } }; + cancelMutateMock.mockResolvedValueOnce(successPayload); + + await expect(cancelReviewMutationFn({ reviewId: 'r1' })).resolves.toEqual(successPayload); + }); +}); + +describe('useCancelReview wiring', () => { + it('toasts the thrown error message via onError and does NOT invalidate queries', async () => { + cancelMutateMock.mockResolvedValue({ + success: false, + error: 'Already completed', + }); + const opts = getOptions('cancel'); + + let thrown: unknown = null; + try { + await opts.mutationFn?.({ reviewId: 'r1' }); + } catch (error) { + thrown = error; + opts.onError?.(error); + } + + expect(thrown).toBeInstanceOf(Error); + expect((thrown as Error).message).toBe('Already completed'); + expect(toastErrorMock).toHaveBeenCalledWith('Already completed'); + expect(invalidateQueriesMock).not.toHaveBeenCalled(); + }); + + it('invalidates the review list and detail on real success', () => { + const opts = getOptions('cancel'); + opts.onSuccess?.({ success: true, review: { id: 'r1' } }, { reviewId: 'r1' }); + + expect(invalidateQueriesMock).toHaveBeenCalledTimes(2); + expect(toastErrorMock).not.toHaveBeenCalled(); + }); +}); + +describe('retriggerReviewMutationFn', () => { + it('throws a typed error carrying the server error message on {success:false}', async () => { + retriggerMutateMock.mockResolvedValueOnce({ + success: false, + error: 'Repository not connected', + }); + + await expect(retriggerReviewMutationFn({ reviewId: 'r2' })).rejects.toThrow( + 'Repository not connected' + ); + }); + + it('resolves with the full success payload on success', async () => { + const successPayload = { success: true, review: { id: 'r2', status: 'queued' } }; + retriggerMutateMock.mockResolvedValueOnce(successPayload); + + await expect(retriggerReviewMutationFn({ reviewId: 'r2' })).resolves.toEqual(successPayload); + }); +}); + +describe('useRetriggerReview wiring', () => { + it('toasts the thrown error message via onError and does NOT invalidate queries', async () => { + retriggerMutateMock.mockResolvedValueOnce({ + success: false, + error: 'Provider rate limit hit', + }); + const opts = getOptions('retrigger'); + + try { + await opts.mutationFn?.({ reviewId: 'r2' }); + } catch (error) { + opts.onError?.(error); + } + + expect(toastErrorMock).toHaveBeenCalledWith('Provider rate limit hit'); + expect(invalidateQueriesMock).not.toHaveBeenCalled(); + }); + + it('invalidates the review list and detail on real success', () => { + const opts = getOptions('retrigger'); + opts.onSuccess?.({ success: true }, { reviewId: 'r2' }); + + expect(invalidateQueriesMock).toHaveBeenCalledTimes(2); + expect(toastErrorMock).not.toHaveBeenCalled(); + }); +}); + +describe('createManualReviewMutationFn', () => { + it('throws a typed error carrying the server error message on {success:false} (personal scope)', async () => { + personalCreateMutateMock.mockResolvedValue({ + success: false, + error: 'Invalid pull request URL', + }); + + await expect(createManualReviewMutationFn('personal', CREATE_VARS)).rejects.toThrow( + 'Invalid pull request URL' + ); + }); + + it('throws a typed error carrying the server error message on {success:false} (org scope)', async () => { + orgCreateMutateMock.mockResolvedValue({ + success: false, + error: 'Provider not connected for organization', + }); + + await expect(createManualReviewMutationFn('org_42', CREATE_VARS)).rejects.toThrow( + 'Provider not connected for organization' + ); + expect(orgCreateMutateMock).toHaveBeenCalledWith( + expect.objectContaining({ organizationId: 'org_42' }) + ); + }); + + it('resolves with the full success payload (including reviewId) so caller navigation works', async () => { + const successPayload = { reviewId: 'rev_abc123', outputMode: 'provider' }; + personalCreateMutateMock.mockResolvedValue(successPayload); + + await expect(createManualReviewMutationFn('personal', CREATE_VARS)).resolves.toEqual( + successPayload + ); + }); +}); + +describe('useCreateManualReview wiring', () => { + it('toasts the thrown error message via onError and does NOT invalidate queries', async () => { + const opts = getOptions('create', 'personal'); + personalCreateMutateMock.mockResolvedValue({ + success: false, + error: 'Insufficient balance', + }); + + let thrown: unknown = null; + try { + await opts.mutationFn?.(CREATE_VARS); + } catch (error) { + thrown = error; + opts.onError?.(error); + } + + expect((thrown as Error).message).toBe('Insufficient balance'); + expect(toastErrorMock).toHaveBeenCalledWith('Insufficient balance'); + expect(invalidateQueriesMock).not.toHaveBeenCalled(); + }); + + it('invalidates the list (no detail) on real success', () => { + const opts = getOptions('create', 'personal'); + opts.onSuccess?.({ reviewId: 'rev_abc123', outputMode: 'provider' }, undefined); + + expect(invalidateQueriesMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/mobile/src/lib/hooks/use-code-reviews.ts b/apps/mobile/src/lib/hooks/use-code-reviews.ts index 5fae5d2691..fc587187e2 100644 --- a/apps/mobile/src/lib/hooks/use-code-reviews.ts +++ b/apps/mobile/src/lib/hooks/use-code-reviews.ts @@ -64,18 +64,26 @@ function useInvalidateReviews(scope: string) { }; } +export async function cancelReviewMutationFn(vars: { reviewId: string }) { + // `success` is typed as `boolean` (not a `true` literal), so a domain + // failure here must not be treated as a resolved mutation — throwing + // routes it to onError (toast) instead of letting callers' onSuccess + // fire haptics/navigation as if it worked. The error carries the + // server's `data.error` verbatim so toast.error(error.message) shows + // the domain reason instead of a generic literal. + const result = await trpcClient.codeReviews.cancel.mutate({ reviewId: vars.reviewId }); + if (!result.success) { + throw new Error(result.error); + } + return result; +} + export function useCancelReview(scope: string) { const invalidateReviews = useInvalidateReviews(scope); return useMutation({ - // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule - mutationFn: (vars: { reviewId: string }) => - trpcClient.codeReviews.cancel.mutate({ reviewId: vars.reviewId }), - onSuccess: (data, vars) => { - if (!data.success) { - announcingToast.error(data.error); - return; - } + mutationFn: cancelReviewMutationFn, + onSuccess: (_data, vars) => { invalidateReviews(vars.reviewId); }, onError: error => { @@ -84,18 +92,22 @@ export function useCancelReview(scope: string) { }); } +export async function retriggerReviewMutationFn(vars: { reviewId: string }) { + // Same typed-error pattern as cancelReviewMutationFn: a domain failure throws + // so React Query runs onError (toast) rather than onSuccess (haptic). + const result = await trpcClient.codeReviews.retrigger.mutate({ reviewId: vars.reviewId }); + if (!result.success) { + throw new Error(result.error); + } + return result; +} + export function useRetriggerReview(scope: string) { const invalidateReviews = useInvalidateReviews(scope); return useMutation({ - // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule - mutationFn: (vars: { reviewId: string }) => - trpcClient.codeReviews.retrigger.mutate({ reviewId: vars.reviewId }), - onSuccess: (data, vars) => { - if (!data.success) { - announcingToast.error(data.error); - return; - } + mutationFn: retriggerReviewMutationFn, + onSuccess: (_data, vars) => { invalidateReviews(vars.reviewId); }, onError: error => { @@ -104,24 +116,42 @@ export function useRetriggerReview(scope: string) { }); } +type CreateManualReviewInput = { + platform: 'github' | 'gitlab'; + url: string; + modelSlug: string; + thinkingEffort?: string | null; + instructions?: string; +}; + +export async function createManualReviewMutationFn(scope: string, vars: CreateManualReviewInput) { + // Same typed-error pattern: a domain failure throws so the screen's + // per-call onSuccess (haptic + router.replace to the new review) + // does not run with `reviewId` undefined. The full success payload + // (including `reviewId`) still resolves on real success so caller + // navigation keeps working. + const result = isPersonal(scope) + ? await trpcClient.personalReviewAgent.createManualReviewJob.mutate(vars) + : await trpcClient.organizations.reviewAgent.createManualReviewJob.mutate({ + ...vars, + organizationId: scope, + }); + // The create router resolves with the job result directly (no + // `{success, error}` envelope) or throws. Keep a narrow defensive guard + // in case the mutation ever returns the `{success: false, error}` shape + // used by other code-reviews mutations, so a domain failure still routes + // to onError without treating a real success payload as a failure. + if ((result as { success?: boolean }).success === false) { + throw new Error((result as { error?: string }).error ?? 'Unknown error'); + } + return result; +} + export function useCreateManualReview(scope: string) { const invalidateReviews = useInvalidateReviews(scope); return useMutation({ - // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule - mutationFn: (vars: { - platform: 'github' | 'gitlab'; - url: string; - modelSlug: string; - thinkingEffort?: string | null; - instructions?: string; - }) => - isPersonal(scope) - ? trpcClient.personalReviewAgent.createManualReviewJob.mutate(vars) - : trpcClient.organizations.reviewAgent.createManualReviewJob.mutate({ - ...vars, - organizationId: scope, - }), + mutationFn: createManualReviewMutationFn.bind(null, scope), onSuccess: () => { invalidateReviews(); }, diff --git a/apps/mobile/src/lib/pr-review/merge/merge-result-banner-store.test.ts b/apps/mobile/src/lib/pr-review/merge/merge-result-banner-store.test.ts new file mode 100644 index 0000000000..2ae0be365d --- /dev/null +++ b/apps/mobile/src/lib/pr-review/merge/merge-result-banner-store.test.ts @@ -0,0 +1,64 @@ +import { afterEach, describe, expect, it } from 'vitest'; + +import { + __resetMergePartialSuccessStoreForTests, + clearMergePartialSuccess, + consumeMergePartialSuccess, + type PrRef, + setMergePartialSuccess, +} from './merge-result-banner-store'; + +const ref: PrRef = { owner: 'octocat', repo: 'hello', number: 1 }; + +afterEach(() => { + __resetMergePartialSuccessStoreForTests(); +}); + +describe('merge partial-success banner store', () => { + it('returns null when no entry has been set', () => { + expect(consumeMergePartialSuccess(ref)).toBeNull(); + }); + + it('returns the stored value once and then clears it', () => { + setMergePartialSuccess(ref, { reason: 'Reference does not exist' }); + + expect(consumeMergePartialSuccess(ref)).toEqual({ reason: 'Reference does not exist' }); + // Consume is destructive — a second read returns null. + expect(consumeMergePartialSuccess(ref)).toBeNull(); + }); + + it('keeps entries isolated per PR (owner/repo/number)', () => { + const a: PrRef = { owner: 'octocat', repo: 'hello', number: 1 }; + const b: PrRef = { owner: 'octocat', repo: 'hello', number: 2 }; + const c: PrRef = { owner: 'octocat', repo: 'world', number: 1 }; + + setMergePartialSuccess(a, { reason: 'for a' }); + setMergePartialSuccess(b, { reason: 'for b' }); + setMergePartialSuccess(c, { reason: 'for c' }); + + expect(consumeMergePartialSuccess(a)).toEqual({ reason: 'for a' }); + expect(consumeMergePartialSuccess(b)).toEqual({ reason: 'for b' }); + expect(consumeMergePartialSuccess(c)).toEqual({ reason: 'for c' }); + expect(consumeMergePartialSuccess(a)).toBeNull(); + }); + + it('treats owner and repo as case-insensitive keys', () => { + setMergePartialSuccess({ owner: 'OctoCat', repo: 'Hello', number: 1 }, { reason: 'x' }); + + expect(consumeMergePartialSuccess({ owner: 'octocat', repo: 'hello', number: 1 })).toEqual({ + reason: 'x', + }); + }); + + it('clearMergePartialSuccess removes a specific entry without affecting siblings', () => { + const a: PrRef = { owner: 'octocat', repo: 'hello', number: 1 }; + const b: PrRef = { owner: 'octocat', repo: 'hello', number: 2 }; + + setMergePartialSuccess(a, { reason: 'a' }); + setMergePartialSuccess(b, { reason: 'b' }); + + clearMergePartialSuccess(a); + expect(consumeMergePartialSuccess(a)).toBeNull(); + expect(consumeMergePartialSuccess(b)).toEqual({ reason: 'b' }); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/merge/merge-result-banner-store.ts b/apps/mobile/src/lib/pr-review/merge/merge-result-banner-store.ts new file mode 100644 index 0000000000..d1a55cf948 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/merge/merge-result-banner-store.ts @@ -0,0 +1,50 @@ +// In-memory cross-route store for the post-merge "branch delete failed" +// partial-success banner. The merge sheet writes the reason here right +// before auto-dismissing; the PR review screen reads on focus and clears +// the entry so the banner does not reappear after the user navigates +// away and back. +// +// NOT persisted to AsyncStorage / SecureStore: the banner is ephemeral +// "you just did this thing and one part of it did not complete" +// feedback, not durable state. A cold reload can safely drop it. + +export type PrRef = { + owner: string; + repo: string; + number: number; +}; + +type PartialMergeSuccess = { + /** Human-readable branch-delete failure reason from the server. */ + reason: string; +}; + +const store = new Map(); + +function key(ref: PrRef): string { + return `${ref.owner.toLowerCase()}/${ref.repo.toLowerCase()}#${ref.number}`; +} + +export function setMergePartialSuccess(ref: PrRef, value: PartialMergeSuccess): void { + store.set(key(ref), value); +} + +export function consumeMergePartialSuccess(ref: PrRef): PartialMergeSuccess | null { + const k = key(ref); + const value = store.get(k) ?? null; + // Consume-on-read: the screen MUST render exactly once and then clear + // so the banner does not flash on every focus. + if (value) { + store.delete(k); + } + return value; +} + +export function clearMergePartialSuccess(ref: PrRef): void { + store.delete(key(ref)); +} + +/** Test-only: drop every entry. Never call from production code. */ +export function __resetMergePartialSuccessStoreForTests(): void { + store.clear(); +} diff --git a/apps/mobile/src/lib/pr-review/merge/merge-result-error.test.ts b/apps/mobile/src/lib/pr-review/merge/merge-result-error.test.ts new file mode 100644 index 0000000000..de5410c660 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/merge/merge-result-error.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, it } from 'vitest'; + +import { classifyPrReviewMutationError } from '@/lib/pr-review/classify-pr-review-query-state'; + +import { MergeNotCompletedError } from './merge-result-error'; + +describe('MergeNotCompletedError', () => { + it('defaults the message to "GitHub did not complete the merge."', () => { + const error = new MergeNotCompletedError(); + expect(error.message).toBe('GitHub did not complete the merge.'); + expect(error.name).toBe('MergeNotCompletedError'); + }); + + it('preserves the optional sha and reason', () => { + const error = new MergeNotCompletedError({ + sha: 'mergedsha', + reason: 'not mergeable', + }); + expect(error.sha).toBe('mergedsha'); + expect(error.reason).toBe('not mergeable'); + }); + + it('is classified as RETRYABLE by classifyPrReviewMutationError (NOT terminal bad-request)', () => { + // The whole point of this typed error: it must not be treated as a + // tRPC BAD_REQUEST, because the submit button is enabled in the + // retryable branch and disabled in the bad-request branch. If a + // future refactor attaches a tRPC code, this test will catch it. + const error = new MergeNotCompletedError({ reason: 'not mergeable' }); + + const classification = classifyPrReviewMutationError(error); + expect(classification).toEqual({ kind: 'retryable' }); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/merge/merge-result-error.ts b/apps/mobile/src/lib/pr-review/merge/merge-result-error.ts new file mode 100644 index 0000000000..ce0cb16d1c --- /dev/null +++ b/apps/mobile/src/lib/pr-review/merge/merge-result-error.ts @@ -0,0 +1,25 @@ +// Typed error thrown by the PR merge sheet when the server's +// `mergePullRequest` procedure returns a non-success result. The server +// only treats `merged: true` as a real merge — anything else (e.g. a 405 +// "not mergeable" reply where `response.data.merged === false`) must NOT +// be celebrated as a success. +// +// This class intentionally does NOT expose a tRPC `data.code` so the +// existing `classifyPrReviewMutationError` falls through to the +// retryable branch (sheet stays open, submit re-enabled, inline error). +// Routing it through the BAD_REQUEST branch would disable the submit +// button and lock the user out of retrying. + +export class MergeNotCompletedError extends Error { + /** The sha GitHub reported in the merge response, when available. */ + readonly sha?: string; + /** Optional human-readable reason from the server (e.g. "not mergeable"). */ + readonly reason?: string; + + constructor(args: { message?: string; sha?: string; reason?: string } = {}) { + super(args.message ?? 'GitHub did not complete the merge.'); + this.name = 'MergeNotCompletedError'; + this.sha = args.sha; + this.reason = args.reason; + } +} diff --git a/apps/mobile/src/lib/pr-review/merge/merge-result-gate.test.ts b/apps/mobile/src/lib/pr-review/merge/merge-result-gate.test.ts new file mode 100644 index 0000000000..69628c859d --- /dev/null +++ b/apps/mobile/src/lib/pr-review/merge/merge-result-gate.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, it } from 'vitest'; + +import { classifyPrReviewMutationError } from '@/lib/pr-review/classify-pr-review-query-state'; + +import { + assertMergeResult, + gateMergeResult, + type MergePullRequestResult, +} from './merge-result-gate'; +import { MergeNotCompletedError } from './merge-result-error'; + +describe('gateMergeResult', () => { + it('returns clean for merged:true, branchDeleted:true', () => { + const result: MergePullRequestResult = { + merged: true, + sha: 's1', + branchDeleted: true, + }; + expect(gateMergeResult(result)).toEqual({ kind: 'clean' }); + }); + + it('returns clean for merged:true when branch delete was not requested (no branchDeleteError key)', () => { + // The server omits `branchDeleteError` when the user did not ask for + // a delete (deleteBranch: false) or when the head is cross-repo. The + // sheet should NOT show the partial-success banner in those cases. + const result: MergePullRequestResult = { + merged: true, + sha: 's1', + branchDeleted: false, + }; + expect(gateMergeResult(result)).toEqual({ kind: 'clean' }); + }); + + it('returns partial for merged:true + branchDeleteError (banner required)', () => { + const result: MergePullRequestResult = { + merged: true, + sha: 's1', + branchDeleted: false, + branchDeleteError: 'Reference does not exist', + }; + expect(gateMergeResult(result)).toEqual({ + kind: 'partial', + reason: 'Reference does not exist', + }); + }); + + it('returns incomplete for merged:false (the bug the slice fixes)', () => { + // The first branch of the server returns `merged: boolean`, so the + // narrowest object literal we can write that matches is the + // merged:false variant. The gate must not let this through. + const result: MergePullRequestResult = { + merged: false, + sha: 's1', + branchDeleted: false, + }; + expect(gateMergeResult(result)).toEqual({ kind: 'incomplete' }); + }); +}); + +describe('assertMergeResult', () => { + it('returns the clean gate for a clean merge', () => { + const result: MergePullRequestResult = { + merged: true, + sha: 's1', + branchDeleted: true, + }; + expect(assertMergeResult(result)).toEqual({ kind: 'clean' }); + }); + + it('returns the partial gate (no throw) for a merged + branch-delete-failed result', () => { + const result: MergePullRequestResult = { + merged: true, + sha: 's1', + branchDeleted: false, + branchDeleteError: '422 Reference does not exist', + }; + expect(assertMergeResult(result)).toEqual({ + kind: 'partial', + reason: '422 Reference does not exist', + }); + }); + + it('throws MergeNotCompletedError for merged:false and the error classifies as RETRYABLE', () => { + const result: MergePullRequestResult = { + merged: false, + sha: 's1', + branchDeleted: false, + }; + + let thrown: unknown = null; + try { + assertMergeResult(result); + } catch (error) { + thrown = error; + } + + expect(thrown).toBeInstanceOf(MergeNotCompletedError); + expect((thrown as MergeNotCompletedError).sha).toBe('s1'); + // The whole point of the typed error: it must NOT be classified as + // a terminal bad-request, because the submit button is enabled in + // the retryable branch and disabled in the bad-request branch. + expect(classifyPrReviewMutationError(thrown)).toEqual({ kind: 'retryable' }); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/merge/merge-result-gate.ts b/apps/mobile/src/lib/pr-review/merge/merge-result-gate.ts new file mode 100644 index 0000000000..91d42e97c0 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/merge/merge-result-gate.ts @@ -0,0 +1,89 @@ +// Pure gate between the `mergePullRequest` server result and the +// sheet's success handling. The server returns a discriminated union +// of three shapes: +// +// 1. { merged: ; sha; branchDeleted: false } +// — GitHub declined the merge (merged=false), or +// the merge succeeded but the user did not ask for a branch +// delete (cross-repo OR deleteBranch=false). +// 2. { merged: true; sha; branchDeleted: true } +// — merged AND the best-effort branch delete succeeded. +// 3. { merged: true; sha; branchDeleted: false; branchDeleteError } +// — merged but the best-effort branch delete failed. +// +// The sheet MUST celebrate (haptic + dismiss) only when the merge +// actually happened (`merged === true`). A `merged: false` response +// means GitHub did not perform the merge and the sheet must throw +// `MergeNotCompletedError` so React Query's `onError` and the +// sheet's classification effect can treat it as RETRYABLE (NOT +// terminal bad-request). +// +// A `branchDeleteError` on a merged result means the partial-success +// path: the user should see the success animation, the sheet should +// dismiss, and the PR review screen should surface a persistent +// "merged but branch delete failed" banner afterwards. +// +// NOTE: the type is intentionally permissive on the first variant +// (`merged: boolean`) to match the inferred tRPC return type — the +// server widens `merged = Boolean(response.data.merged)` and the +// subsequent branches only narrow the other fields. + +import { MergeNotCompletedError } from './merge-result-error'; + +export type MergePullRequestResult = + | { merged: boolean; sha: string; branchDeleted: false } + | { merged: true; sha: string; branchDeleted: true } + | { merged: true; sha: string; branchDeleted: false; branchDeleteError: string }; + +type MergeResultGate = + | { kind: 'clean' } + | { kind: 'partial'; reason: string } + | { kind: 'incomplete' }; + +/** + * Decide how the sheet should react to a `mergePullRequest` result. + * + * Returns `{ kind: 'clean' }` for an uneventful success, + * `{ kind: 'partial', reason }` for a merged-but-branch-delete-failed + * outcome, and `{ kind: 'incomplete' }` when GitHub did not perform + * the merge. + */ +export function gateMergeResult(result: MergePullRequestResult): MergeResultGate { + // `result.merged` is the union of `boolean | true` — a falsy value here + // can only be `false` (GitHub declined the merge) since tRPC resolves + // server-side booleans, so `!result.merged` is the right gate without + // the eslint-flagged `!== true` literal compare. + if (!result.merged) { + return { kind: 'incomplete' }; + } + // Same narrowing: `result.branchDeleted` is `true | false` after the + // `merged` gate; truthy means the best-effort branch delete succeeded. + if (result.branchDeleted) { + return { kind: 'clean' }; + } + // branchDeleted is false here. The server only sets `branchDeleteError` + // when the user actually requested a delete AND it failed. The + // cross-repo / not-requested paths leave it absent. The sheet + // collapses both to "clean" (no banner) because the user did not + // ask for a delete. + if ('branchDeleteError' in result && typeof result.branchDeleteError === 'string') { + return { kind: 'partial', reason: result.branchDeleteError }; + } + return { kind: 'clean' }; +} + +/** + * Convenience wrapper: inspects `gateMergeResult` and either throws + * `MergeNotCompletedError` (incomplete) or returns the gate decision. + * The sheet uses this so the throw is co-located with the decision + * logic that causes it. + */ +export function assertMergeResult(result: MergePullRequestResult): MergeResultGate { + const gate = gateMergeResult(result); + if (gate.kind === 'incomplete') { + throw new MergeNotCompletedError({ + sha: result.sha, + }); + } + return gate; +} diff --git a/apps/mobile/src/lib/pr-review/merge/merge-success-effects.test.ts b/apps/mobile/src/lib/pr-review/merge/merge-success-effects.test.ts new file mode 100644 index 0000000000..a0957c1a98 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/merge/merge-success-effects.test.ts @@ -0,0 +1,47 @@ +import { beforeEach, describe, expect, it } from 'vitest'; + +import { applyMergeSuccessEffects } from './merge-success-effects'; +import { + __resetMergePartialSuccessStoreForTests, + consumeMergePartialSuccess, +} from './merge-result-banner-store'; + +const REF = { owner: 'octocat', repo: 'hello', number: 1 }; + +beforeEach(() => { + __resetMergePartialSuccessStoreForTests(); +}); + +describe('applyMergeSuccessEffects', () => { + it('partial success writes the banner and celebrates', () => { + const result = { + merged: true, + sha: 'sha', + branchDeleted: false, + branchDeleteError: 'Reference does not exist', + } as const; + + const { celebrate } = applyMergeSuccessEffects(result, REF); + + expect(celebrate).toBe(true); + expect(consumeMergePartialSuccess(REF)).toEqual({ reason: 'Reference does not exist' }); + }); + + it('clean success celebrates without writing a banner', () => { + const result = { merged: true, sha: 'sha', branchDeleted: true } as const; + + const { celebrate } = applyMergeSuccessEffects(result, REF); + + expect(celebrate).toBe(true); + expect(consumeMergePartialSuccess(REF)).toBeNull(); + }); + + it('incomplete result is unreachable in the sheet because the hook throws first; defensively it does not celebrate or write a banner', () => { + const result = { merged: false, sha: 'sha', branchDeleted: false } as const; + + const { celebrate } = applyMergeSuccessEffects(result, REF); + + expect(celebrate).toBe(false); + expect(consumeMergePartialSuccess(REF)).toBeNull(); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/merge/merge-success-effects.ts b/apps/mobile/src/lib/pr-review/merge/merge-success-effects.ts new file mode 100644 index 0000000000..d9b1c65fd9 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/merge/merge-success-effects.ts @@ -0,0 +1,31 @@ +import { type PrRef, setMergePartialSuccess } from './merge-result-banner-store'; +import { gateMergeResult, type MergePullRequestResult } from './merge-result-gate'; + +type MergeSuccessEffects = { + /** Whether the sheet should celebrate (haptic + refetch + dismiss). */ + celebrate: boolean; +}; + +/** + * Pure helper that wires the post-merge success side effects. + * + * - `clean` and `partial` both return `{ celebrate: true }`. + * - `partial` also writes the persistent banner to `merge-result-banner-store`. + * - `incomplete` returns `{ celebrate: false }` and writes nothing. + * + * In production the mutation hook throws on `incomplete` before the sheet + * reaches this helper, so the `incomplete` branch is defensive. + */ +export function applyMergeSuccessEffects( + result: MergePullRequestResult, + ref: PrRef +): MergeSuccessEffects { + const gate = gateMergeResult(result); + if (gate.kind === 'incomplete') { + return { celebrate: false }; + } + if (gate.kind === 'partial') { + setMergePartialSuccess(ref, { reason: gate.reason }); + } + return { celebrate: true }; +} diff --git a/apps/mobile/src/lib/pr-review/merge/use-pr-merge-mutations.test.ts b/apps/mobile/src/lib/pr-review/merge/use-pr-merge-mutations.test.ts new file mode 100644 index 0000000000..2f2a7eacc1 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/merge/use-pr-merge-mutations.test.ts @@ -0,0 +1,149 @@ +// P0-B-08 wiring tests for `useMergePullRequestMutation`. +// +// The pure gate / store / error class are covered by their own unit +// tests. These tests assert the WIRING: the hook's `mutationFn` +// delegates to `trpcClient.githubPrReview.mergePullRequest.mutate` +// and then routes the result through `assertMergeResult`, so a +// `merged: false` reply throws `MergeNotCompletedError` and lands in +// React Query's `onError` (NOT `onSuccess`). + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useMergePullRequestMutation } from './use-pr-merge-mutations'; +import { MergeNotCompletedError } from './merge-result-error'; +import { classifyPrReviewMutationError } from '@/lib/pr-review/classify-pr-review-query-state'; + +type MutationOptions = { + mutationFn?: (vars: unknown) => Promise; + onError?: (error: unknown) => void; + onSettled?: (data: unknown, error: unknown, vars: unknown) => Promise | void; +}; + +let lastCapturedOptions: MutationOptions | null = null; +const mutateMock = vi.fn(); +const invalidateQueriesMock = vi.fn(); +const toastErrorMock = vi.fn(); + +vi.mock('@tanstack/react-query', () => ({ + useMutation: (opts: MutationOptions) => { + lastCapturedOptions = opts; + return { mutateAsync: vi.fn(), mutate: vi.fn() }; + }, + useQueryClient: () => ({ + invalidateQueries: (...args: unknown[]) => { + invalidateQueriesMock(...args); + }, + }), +})); + +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => ({ + githubPrReview: { + getPullRequest: { queryKey: () => ['githubPrReview', 'getPullRequest'] }, + listChecks: { pathFilter: () => ['githubPrReview', 'listChecks'] }, + listFiles: { pathFilter: () => ['githubPrReview', 'listFiles'] }, + }, + }), + trpcClient: { + githubPrReview: { + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + mergePullRequest: { mutate: (vars: unknown) => mutateMock(vars) }, + }, + }, +})); + +vi.mock('sonner-native', () => ({ + toast: { error: (msg: string) => toastErrorMock(msg) }, +})); + +const REF = { owner: 'octocat', repo: 'hello', number: 1 }; +const INPUT = { + owner: 'octocat', + repo: 'hello', + number: 1, + method: 'squash' as const, + deleteBranch: true, + expectedHeadSha: 'a'.repeat(40), +}; + +describe('useMergePullRequestMutation (P0-B-08 wiring)', () => { + beforeEach(() => { + lastCapturedOptions = null; + mutateMock.mockReset(); + invalidateQueriesMock.mockReset(); + toastErrorMock.mockReset(); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('mounts a useMutation with a custom mutationFn (so the gate can throw on merged:false)', () => { + useMergePullRequestMutation(REF); + expect(lastCapturedOptions?.mutationFn).toBeDefined(); + }); + + it('throws MergeNotCompletedError on a merged:false result, classifying as RETRYABLE (not bad-request)', async () => { + // The whole point of the slice: when GitHub returns `merged: false` + // (e.g. 405 "not mergeable"), the hook must reject so React Query + // routes it to `onError` (toast) and the sheet's effect treats it + // as RETRYABLE — the submit button stays enabled. If the mutation + // resolved instead, the sheet would fire a success haptic and + // dismiss even though GitHub did not perform the merge. + mutateMock.mockResolvedValueOnce({ merged: false, sha: 's1', branchDeleted: false }); + useMergePullRequestMutation(REF); + + let thrown: unknown = null; + try { + await lastCapturedOptions?.mutationFn?.(INPUT); + } catch (error) { + thrown = error; + } + + expect(thrown).toBeInstanceOf(MergeNotCompletedError); + expect((thrown as MergeNotCompletedError).sha).toBe('s1'); + // Default message is the user-visible "GitHub did not complete the merge." + expect((thrown as Error).message).toBe('GitHub did not complete the merge.'); + // classifyPrReviewMutationError is what the sheet uses; the typed + // error must fall through to RETRYABLE so the submit button stays + // enabled. Routing it through BAD_REQUEST would lock the user out. + expect(classifyPrReviewMutationError(thrown)).toEqual({ kind: 'retryable' }); + }); + + it('RESOLVES on a clean merged:true (does not celebrate nothing, does not throw)', async () => { + mutateMock.mockResolvedValueOnce({ merged: true, sha: 's1', branchDeleted: true }); + useMergePullRequestMutation(REF); + + await expect(lastCapturedOptions?.mutationFn?.(INPUT)).resolves.toEqual({ + merged: true, + sha: 's1', + branchDeleted: true, + }); + }); + + it('RESOLVES on a partial merged:true + branchDeleteError (so performSubmit can write the banner)', async () => { + // The partial case MUST resolve (not throw) so the sheet can read + // the result, run `gateMergeResult`, and write the banner store + // before dismissing. + mutateMock.mockResolvedValueOnce({ + merged: true, + sha: 's1', + branchDeleted: false, + branchDeleteError: 'Reference does not exist', + }); + useMergePullRequestMutation(REF); + + await expect(lastCapturedOptions?.mutationFn?.(INPUT)).resolves.toEqual({ + merged: true, + sha: 's1', + branchDeleted: false, + branchDeleteError: 'Reference does not exist', + }); + }); + + it('onError still toasts the message (so the retryable inline error surfaces)', () => { + useMergePullRequestMutation(REF); + lastCapturedOptions?.onError?.(new Error('boom')); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/merge/use-pr-merge-mutations.ts b/apps/mobile/src/lib/pr-review/merge/use-pr-merge-mutations.ts index 3f29bcaa24..a0c0fbe571 100644 --- a/apps/mobile/src/lib/pr-review/merge/use-pr-merge-mutations.ts +++ b/apps/mobile/src/lib/pr-review/merge/use-pr-merge-mutations.ts @@ -15,10 +15,25 @@ import { useMutation, useQueryClient } from '@tanstack/react-query'; import { toast } from 'sonner-native'; -import { useTRPC } from '@/lib/trpc'; +import { trpcClient, useTRPC } from '@/lib/trpc'; +import { + assertMergeResult, + type MergePullRequestResult, +} from '@/lib/pr-review/merge/merge-result-gate'; type PrRef = { owner: string; repo: string; number: number }; +type MergePullRequestInput = { + owner: string; + repo: string; + number: number; + method: 'merge' | 'squash' | 'rebase'; + commitTitle?: string; + commitMessage?: string; + deleteBranch: boolean; + expectedHeadSha: string; +}; + function usePrRefKeys(ref: PrRef) { const trpc = useTRPC(); return { @@ -40,20 +55,33 @@ async function invalidatePrCaches( } export function useMergePullRequestMutation(ref: PrRef) { - const trpc = useTRPC(); const queryClient = useQueryClient(); const keys = usePrRefKeys(ref); - return useMutation( - trpc.githubPrReview.mergePullRequest.mutationOptions({ - onError: (error: { message: string }) => { - toast.error(error.message); - }, - onSettled: async () => { - await invalidatePrCaches(queryClient, keys); - }, - }) - ); + // P0-B-08: gate success on the authoritative `merged: true` result + // BEFORE React Query resolves the mutation. The server only treats + // `merged: true` as a real merge — a `merged: false` reply (e.g. a 405 + // "not mergeable" where GitHub refuses) must NOT be celebrated as a + // success. `assertMergeResult` throws `MergeNotCompletedError` on + // `merged !== true`; that throw lands in `onError` and the sheet's + // existing classification effect treats it as RETRYABLE (NOT terminal + // bad-request), so the submit button stays enabled and the user can + // retry. The typed return is preserved so `performSubmit` can read + // the sha / branchDeleted / branchDeleteError off the resolved value. + return useMutation({ + mutationFn: async input => { + const result = await trpcClient.githubPrReview.mergePullRequest.mutate(input); + // Throws on `merged: false`; returns the gate on clean / partial. + assertMergeResult(result); + return result; + }, + onError: (error: { message: string }) => { + toast.error(error.message); + }, + onSettled: async () => { + await invalidatePrCaches(queryClient, keys); + }, + }); } export function useUpdateBranchMutation(ref: PrRef) { diff --git a/apps/mobile/vitest.pure.config.ts b/apps/mobile/vitest.pure.config.ts index 2d74336de0..05a476f3c4 100644 --- a/apps/mobile/vitest.pure.config.ts +++ b/apps/mobile/vitest.pure.config.ts @@ -28,6 +28,7 @@ export default defineProject({ 'src/lib/pr-review/**/*.test.ts', 'src/lib/voice-input/**/*.test.ts', 'src/components/**/*.test.ts', + 'src/components/pr-review/**/*.test.tsx', ], }, }); diff --git a/apps/web/package.json b/apps/web/package.json index 53d41780a3..8ac18cb744 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -191,8 +191,10 @@ "@types/react-dom": "19.2.3", "@typescript/native-preview": "catalog:", "babel-plugin-react-compiler": "1.0.0", + "@octokit/graphql-schema": "15.26.1", "dependency-cruiser": "17.3.10", "dotenv": "17.3.1", + "graphql": "16.14.2", "ink": "6.8.0", "jest": "30.3.0", "knip": "5.86.0", diff --git a/apps/web/src/app/(app)/code-reviews/ReviewAgentPageClient.tsx b/apps/web/src/app/(app)/code-reviews/ReviewAgentPageClient.tsx index f9d7b63323..4e6136e745 100644 --- a/apps/web/src/app/(app)/code-reviews/ReviewAgentPageClient.tsx +++ b/apps/web/src/app/(app)/code-reviews/ReviewAgentPageClient.tsx @@ -275,23 +275,7 @@ export function ReviewAgentPageClient({ - + diff --git a/apps/web/src/app/(app)/organizations/[id]/code-reviews/ReviewAgentPageClient.tsx b/apps/web/src/app/(app)/organizations/[id]/code-reviews/ReviewAgentPageClient.tsx index acf8a2ba9a..a6ac3a20a0 100644 --- a/apps/web/src/app/(app)/organizations/[id]/code-reviews/ReviewAgentPageClient.tsx +++ b/apps/web/src/app/(app)/organizations/[id]/code-reviews/ReviewAgentPageClient.tsx @@ -405,20 +405,6 @@ export function ReviewAgentPageClient({ organizationId={organizationId} platform="gitlab" councilUiEnabled={councilUiEnabled} - gitlabStatusData={ - gitlabStatusData - ? { - connected: gitlabStatusData.connected, - integration: gitlabStatusData.integration - ? { - isValid: gitlabStatusData.integration.isValid, - webhookSecret: gitlabStatusData.integration.webhookSecret, - instanceUrl: gitlabStatusData.integration.instanceUrl, - } - : undefined, - } - : undefined - } /> diff --git a/apps/web/src/components/code-reviews/ReviewConfigForm.tsx b/apps/web/src/components/code-reviews/ReviewConfigForm.tsx index 2dca22273f..7c606b7842 100644 --- a/apps/web/src/components/code-reviews/ReviewConfigForm.tsx +++ b/apps/web/src/components/code-reviews/ReviewConfigForm.tsx @@ -77,19 +77,9 @@ import { type Platform = 'github' | 'gitlab'; -export type GitLabStatusData = { - connected: boolean; - integration?: { - isValid: boolean; - webhookSecret?: string; - instanceUrl?: string; - }; -}; - export type ReviewConfigFormProps = { organizationId?: string; platform?: Platform; - gitlabStatusData?: GitLabStatusData; /** Same gate as the manual council UI: local dev, or an entitled org behind the rollout flag. */ councilUiEnabled?: boolean; }; @@ -137,7 +127,6 @@ export const REVIEW_STYLES = REVIEW_STYLE_VALUES.map(value => ({ export function ReviewConfigForm({ organizationId, platform = 'github', - gitlabStatusData, councilUiEnabled = false, }: ReviewConfigFormProps) { const trpc = useTRPC(); @@ -326,13 +315,45 @@ export function ReviewConfigForm({ } }, [availableVariants, thinkingEffort]); - // Mutation for regenerating webhook secret - const regenerateSecretMutation = useMutation( + // Mutation for regenerating webhook secret. The org path is billing-gated + // (owner/billing_manager only); the personal path is self-gated. The + // secret is returned ONCE on success and never re-fetched from status — + // status no longer carries it. + const orgRegenerateSecretMutation = useMutation( + trpc.organizations.reviewAgent.rotateGitLabWebhookSecret.mutationOptions({ + onSuccess: data => { + setRegeneratedSecret(data.webhookSecret); + const updated = data.webhookSync.updated; + const errors = data.webhookSync.errors.length; + toast.success( + errors > 0 + ? `Webhook secret rotated. ${updated} webhooks updated, ${errors} error(s) — check audit log.` + : `Webhook secret rotated. ${updated} webhook(s) re-synced.` + ); + void queryClient.invalidateQueries({ + queryKey: trpc.organizations.reviewAgent.getGitLabStatus.queryKey({ + organizationId: organizationId ?? '', + }), + }); + }, + onError: error => { + toast.error('Failed to rotate webhook secret', { + description: error.message, + }); + }, + }) + ); + const personalRegenerateSecretMutation = useMutation( trpc.gitlab.regenerateWebhookSecret.mutationOptions({ onSuccess: data => { setRegeneratedSecret(data.webhookSecret); - toast.success('Webhook secret regenerated successfully'); - // Invalidate the GitLab status query to refresh the data + const updated = data.webhookSync.updated; + const errors = data.webhookSync.errors.length; + toast.success( + errors > 0 + ? `Webhook secret regenerated. ${updated} webhooks updated, ${errors} error(s) — check audit log.` + : `Webhook secret regenerated. ${updated} webhook(s) re-synced.` + ); void queryClient.invalidateQueries({ queryKey: trpc.personalReviewAgent.getGitLabStatus.queryKey(), }); @@ -347,9 +368,17 @@ export function ReviewConfigForm({ const handleRegenerateSecret = () => { setRegeneratedSecret(null); // Clear any previously shown secret - regenerateSecretMutation.mutate({}); + if (organizationId) { + orgRegenerateSecretMutation.mutate({ organizationId }); + } else { + personalRegenerateSecretMutation.mutate(); + } }; + const regenerateSecretMutation = organizationId + ? orgRegenerateSecretMutation + : personalRegenerateSecretMutation; + const handleCopyWebhookUrl = async () => { await navigator.clipboard.writeText(webhookUrl); setCopiedWebhookUrl(true); @@ -357,16 +386,10 @@ export function ReviewConfigForm({ setTimeout(() => setCopiedWebhookUrl(false), 2000); }; - const handleCopyWebhookSecret = async () => { - const secret = gitlabStatusData?.integration?.webhookSecret; - if (secret) { - await navigator.clipboard.writeText(secret); - setCopiedWebhookSecret(true); - toast.success('Webhook secret copied to clipboard'); - setTimeout(() => setCopiedWebhookSecret(false), 2000); - } - }; - + // The status endpoint no longer returns the webhook secret, so the old + // `handleCopyWebhookSecret` (which copied the status-provided secret) and + // its markup are removed. The only path to view/copy the secret is the + // rotate mutation, which returns it once into `regeneratedSecret` below. const handleCopyRegeneratedSecret = async () => { if (regeneratedSecret) { await navigator.clipboard.writeText(regeneratedSecret); @@ -1346,30 +1369,6 @@ export function ReviewConfigForm({

- ) : gitlabStatusData?.integration?.webhookSecret ? ( - <> -
- - •••••••••••••••• - - -
-

- Use this secret token in your GitLab webhook configuration for - security. -

- ) : (

No webhook secret configured. Click regenerate to create one. diff --git a/apps/web/src/lib/auto-fix/application/webhook/review-comment-webhook-processor.test.ts b/apps/web/src/lib/auto-fix/application/webhook/review-comment-webhook-processor.test.ts new file mode 100644 index 0000000000..654a2d6bd8 --- /dev/null +++ b/apps/web/src/lib/auto-fix/application/webhook/review-comment-webhook-processor.test.ts @@ -0,0 +1,155 @@ +/** + * Unit tests for the auto-fix review-comment webhook processor's + * admission branch. Focuses on the previously-buggy local-regex check + * (which rejected the product-advertised "@kilocode-bot fix it" command) + * and the new shared `parseFixCommand` admission. + * + * The downstream dispatch path (permission, agent config, ticket + * creation, dispatch) is intentionally NOT exercised here — it has its + * own coverage and would require many more mocks. The single side + * effect we observe is the first function called after admission + * (`getAgentConfigForOwner`), so when admission is rejected the + * function returns silently with no further calls; when admission is + * granted, the mock throws and the test catches the throw. + */ +import type { PlatformIntegration } from '@kilocode/db/schema'; +import type { PullRequestReviewCommentPayload } from '@/lib/integrations/platforms/github/webhook-schemas'; + +const mockGetAgentConfigForOwner = jest.fn(); +const mockFindExistingReviewCommentFixTicket = jest.fn(); +const mockParseFixCommand = jest.fn(); + +jest.mock('@/lib/agent-config/db/agent-configs', () => ({ + getAgentConfigForOwner: (...args: unknown[]) => mockGetAgentConfigForOwner(...args), +})); + +jest.mock('../../db/fix-tickets', () => ({ + createFixTicket: jest.fn(), + findExistingReviewCommentFixTicket: (...args: unknown[]) => + mockFindExistingReviewCommentFixTicket(...args), + resetFixTicketForRetry: jest.fn(), +})); + +jest.mock('../../dispatch/dispatch-pending-fixes', () => ({ + tryDispatchPendingFixes: jest.fn().mockResolvedValue(undefined), +})); + +jest.mock('@/lib/bot-users/bot-user-service', () => ({ + getBotUserId: jest.fn(), +})); + +jest.mock('@/lib/integrations/platforms/github/adapter', () => ({ + addReactionToPRReviewComment: jest.fn().mockResolvedValue(undefined), + getCollaboratorPermissionLevel: jest.fn(), +})); + +jest.mock('@sentry/nextjs', () => ({ + captureException: jest.fn(), +})); + +jest.mock('@kilocode/app-shared/code-review', () => ({ + parseFixCommand: (text: string) => mockParseFixCommand(text), +})); + +import { ReviewCommentWebhookProcessor } from './review-comment-webhook-processor'; + +function buildPayload(body: string): PullRequestReviewCommentPayload { + return { + action: 'created', + comment: { + id: 1, + body, + user: { login: 'maintainer' }, + in_reply_to_id: null, + created_at: '2026-07-23T00:00:00.000Z', + html_url: 'https://github.com/acme/widgets/pull/42#discussion_r1', + path: 'src/widget.ts', + line: 10, + diff_hunk: '@@', + // MEMBER is in WRITE_ACCESS_ASSOCIATIONS so the permission API + // fallback (also mocked) is skipped. + author_association: 'MEMBER', + }, + pull_request: { + number: 42, + title: 'Test PR', + html_url: 'https://github.com/acme/widgets/pull/42', + user: { login: 'contributor' }, + head: { sha: 'abc123', ref: 'feature' }, + base: { ref: 'main' }, + }, + repository: { + id: 1, + name: 'widgets', + full_name: 'acme/widgets', + private: true, + owner: { login: 'acme' }, + }, + installation: { id: 123 }, + sender: { login: 'maintainer' }, + }; +} + +const integration = { + id: 'integration-1', + owned_by_user_id: 'user-1', + owned_by_organization_id: null, + github_app_type: 'standard', +} as unknown as PlatformIntegration; + +describe('ReviewCommentWebhookProcessor admission', () => { + let processor: ReviewCommentWebhookProcessor; + + beforeEach(() => { + jest.clearAllMocks(); + processor = new ReviewCommentWebhookProcessor(); + }); + + it('admits the product-advertised @kilocode-bot fix it command (regression evidence)', async () => { + // Real shared parser behavior is asserted in the shared package's + // mention-command.test.ts. Here we verify the processor delegates + // admission to the shared parser and proceeds to the next step. + const body = '@kilocode-bot fix it'; + mockParseFixCommand.mockReturnValue(true); + // First call after admission — when this throws we know admission + // was granted. + mockGetAgentConfigForOwner.mockRejectedValue(new Error('admitted')); + + await expect(processor.process(buildPayload(body), integration)).rejects.toThrow('admitted'); + + expect(mockParseFixCommand).toHaveBeenCalledWith(body); + expect(mockGetAgentConfigForOwner).toHaveBeenCalledTimes(1); + }); + + it('admits the existing shorthand @kilo fix', async () => { + const body = '@kilo fix this'; + mockParseFixCommand.mockReturnValue(true); + mockGetAgentConfigForOwner.mockRejectedValue(new Error('admitted')); + + await expect(processor.process(buildPayload(body), integration)).rejects.toThrow('admitted'); + + expect(mockParseFixCommand).toHaveBeenCalledWith(body); + expect(mockGetAgentConfigForOwner).toHaveBeenCalledTimes(1); + }); + + it('rejects a non-matching body without calling getAgentConfigForOwner', async () => { + const body = 'please fix this'; + mockParseFixCommand.mockReturnValue(false); + + await processor.process(buildPayload(body), integration); + + expect(mockParseFixCommand).toHaveBeenCalledWith(body); + expect(mockGetAgentConfigForOwner).not.toHaveBeenCalled(); + expect(mockFindExistingReviewCommentFixTicket).not.toHaveBeenCalled(); + }); + + it('rejects a mention-only body (no fix keyword) without further processing', async () => { + const body = '@kilocode-bot ship it'; + mockParseFixCommand.mockReturnValue(false); + + await processor.process(buildPayload(body), integration); + + expect(mockParseFixCommand).toHaveBeenCalledWith(body); + expect(mockGetAgentConfigForOwner).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/src/lib/auto-fix/application/webhook/review-comment-webhook-processor.ts b/apps/web/src/lib/auto-fix/application/webhook/review-comment-webhook-processor.ts index 2d6f384808..03c969e5ff 100644 --- a/apps/web/src/lib/auto-fix/application/webhook/review-comment-webhook-processor.ts +++ b/apps/web/src/lib/auto-fix/application/webhook/review-comment-webhook-processor.ts @@ -30,9 +30,7 @@ import type { PullRequestReviewCommentPayload, GitHubAuthorAssociation, } from '@/lib/integrations/platforms/github/webhook-schemas'; - -const KILO_MENTION_PATTERN = /@kilo\b/i; -const FIX_KEYWORD_PATTERN = /\b(fix|patch)\b/i; +import { parseFixCommand } from '@kilocode/app-shared/code-review'; /** * author_association values that imply write access. @@ -71,8 +69,13 @@ export class ReviewCommentWebhookProcessor { commentId: comment.id, }); - // 1. Check if comment body contains @kilo and a fix keyword - if (!KILO_MENTION_PATTERN.test(comment.body) || !FIX_KEYWORD_PATTERN.test(comment.body)) { + // 1. Check if comment body contains @kilo and a fix keyword. + // Admission is delegated to the shared parseFixCommand so the + // product-advertised "@kilocode-bot fix it" footer command and + // the existing "@kilo … fix" shorthand both admit (and a + // mention-only or fix-only body still rejects). See + // @kilocode/app-shared/code-review/mention-command.ts. + if (!parseFixCommand(comment.body)) { logExceptInTest('[ReviewCommentWebhookProcessor] No @kilo fix mention found', { commentId: comment.id, }); diff --git a/apps/web/src/lib/code-reviews/prompts/default-prompt-template.drift-guard.test.ts b/apps/web/src/lib/code-reviews/prompts/default-prompt-template.drift-guard.test.ts new file mode 100644 index 0000000000..0ff34c0862 --- /dev/null +++ b/apps/web/src/lib/code-reviews/prompts/default-prompt-template.drift-guard.test.ts @@ -0,0 +1,42 @@ +/** + * Drift-guard test: the prompt-template footer literally tells users + * "Reply with `@kilocode-bot fix it` to have Kilo Code address this + * issue", and the auto-fix webhook processor is supposed to admit + * exactly that command. This test reads the JSON footer literal + * verbatim, asserts the footer still contains the advertised command + * string, and asserts the shared `parseFixCommand` parser admits it. + * + * Placement: this test lives in apps/web (not in @kilocode/app-shared) + * because the shared package cannot import a JSON file that lives in + * apps/web, while apps/web can import both the template JSON and the + * shared parser. Co-locating the footer literal and the parser + * assertion in the same test makes any future divergence — a footer + * wording change, a parser narrowing, or a mention-pattern + * simplification that re-breaks the advertised command — fail + * immediately. + */ +import defaultPromptTemplate from './default-prompt-template.json'; +import { parseFixCommand } from '@kilocode/app-shared/code-review'; + +const ADVERTISED_COMMAND = '@kilocode-bot fix it'; + +describe('default-prompt-template inlineCommentFooter drift guard', () => { + const footer = defaultPromptTemplate.inlineCommentFooter; + + it('still advertises the exact @kilocode-bot fix it command', () => { + expect(footer).toContain(ADVERTISED_COMMAND); + }); + + it('the shared parseFixCommand admits the exact advertised command', () => { + expect(parseFixCommand(ADVERTISED_COMMAND)).toBe(true); + }); + + it('parseFixCommand also admits a representative command embedded in the footer text', () => { + // Sanity check: extracting a representative admit-command from the + // footer literal and running it through the parser must still admit. + // If both the footer wording and the parser ever drift, this fails. + const sample = footer.split('\n').find(line => line.includes('@kilocode-bot')); + expect(sample).toBeDefined(); + expect(parseFixCommand(sample!)).toBe(true); + }); +}); diff --git a/apps/web/src/lib/github-pr-review/normalize-reactions.test.ts b/apps/web/src/lib/github-pr-review/normalize-reactions.test.ts new file mode 100644 index 0000000000..a366ce4563 --- /dev/null +++ b/apps/web/src/lib/github-pr-review/normalize-reactions.test.ts @@ -0,0 +1,123 @@ +/** + * @jest-environment node + * + * Pins the reaction DTO invariant for `normalizeReactions` / `normalizeComment` + * (P0-C-14). The router selects GitHub's `reactionGroups` field, which + * returns a flat `ReactionGroup[]` (one entry per `ReactionContent`) with + * `reactors.totalCount`. The output DTO consumed by + * `apps/web/src/lib/github-pr-review/mappers.ts` and the mobile reactions + * row keeps the shape `Array<{ content: string; count: number; + * viewerHasReacted: boolean }>`, preserving source order and filtering + * zero-count groups (shipped behavior; the mobile reactions row renders a + * fixed set of 8 pills and hides zero counts, so dropped zero-count groups + * are invisible to the consumer). + */ +import { + normalizeComment_FOR_TEST, + normalizeReactions_FOR_TEST, +} from '@/routers/github-pr-review-router'; + +describe('normalizeReactions (reactionGroups shape)', () => { + it('maps a group with reactors.totalCount to { content, count, viewerHasReacted }', () => { + const out = normalizeReactions_FOR_TEST([ + { + content: '+1', + viewerHasReacted: true, + reactors: { totalCount: 3 }, + }, + ]); + expect(out).toEqual([{ content: '+1', count: 3, viewerHasReacted: true }]); + }); + + it('treats absent/null reactors as count: 0 — filtered out, never throws', () => { + expect( + normalizeReactions_FOR_TEST([ + { content: 'THUMBS_UP', viewerHasReacted: false, reactors: null }, + ]) + ).toEqual([]); + + // `reactors` omitted entirely — same behavior. + expect(normalizeReactions_FOR_TEST([{ content: 'HEART', viewerHasReacted: false }])).toEqual( + [] + ); + }); + + it('preserves source order of surviving entries and drops zero-count groups', () => { + const out = normalizeReactions_FOR_TEST([ + { content: '+1', viewerHasReacted: true, reactors: { totalCount: 2 } }, + { + content: 'LAUGH', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'HEART', + viewerHasReacted: false, + reactors: { totalCount: 7 }, + }, + ]); + expect(out).toEqual([ + { content: '+1', count: 2, viewerHasReacted: true }, + // Zero-count groups are dropped — the shipped contract; the mobile + // reactions row renders a fixed 8-pill set and hides zero counts, so + // dropping them does not change the rendered output. + { content: 'HEART', count: 7, viewerHasReacted: false }, + ]); + expect(out.map(r => r.content)).toEqual(['+1', 'HEART']); + }); + + it('coerces a truthy non-boolean viewerHasReacted to true (legacy GitHub quirk)', () => { + // The legacy normalizeReactions wrapper called `Boolean(...)`; preserve + // that contract even when GitHub occasionally returns truthy non-booleans. + const out = normalizeReactions_FOR_TEST([ + { + content: 'ROCKET', + viewerHasReacted: 1 as unknown as boolean, + reactors: { totalCount: 1 }, + }, + ]); + expect(out[0]?.viewerHasReacted).toBe(true); + }); + + it('returns an empty array for an empty input (no spurious entries)', () => { + expect(normalizeReactions_FOR_TEST([])).toEqual([]); + }); +}); + +describe('normalizeComment (reactionGroups shape)', () => { + it('reads node.reactionGroups and forwards the same DTO shape', () => { + const out = normalizeComment_FOR_TEST({ + databaseId: 42, + id: 'node_42', + body: 'hello', + createdAt: '2024-01-01T00:00:00Z', + author: { login: 'octocat', avatarUrl: 'https://x/y.png' }, + reactionGroups: [ + { content: '+1', viewerHasReacted: false, reactors: { totalCount: 1 } }, + { + content: 'EYES', + viewerHasReacted: true, + reactors: { totalCount: 4 }, + }, + ], + }); + expect(out.databaseId).toBe(42); + expect(out.reactions).toEqual([ + { content: '+1', count: 1, viewerHasReacted: false }, + { content: 'EYES', count: 4, viewerHasReacted: true }, + ]); + }); + + it('defaults reactionGroups to [] when the field is absent or null', () => { + const out = normalizeComment_FOR_TEST({ + databaseId: 1, + id: 'node_1', + body: '', + createdAt: '2024-01-01T00:00:00Z', + author: null, + // `reactionGroups` omitted on purpose. + } as unknown as Parameters[0]); + expect(out.reactions).toEqual([]); + expect(out.author).toBeNull(); + }); +}); diff --git a/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts b/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts index 4de2045083..a85bb251e1 100644 --- a/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts +++ b/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts @@ -23,13 +23,41 @@ function commentNode( author: { login: 'octocat', avatarUrl: 'https://x/y.png' }, // Live GitHub shape: unpaginated reactionGroups list (all group types). reactionGroups: reactionGroups ?? [ - { content: 'THUMBS_UP', viewerHasReacted: false, reactors: { totalCount: 0 } }, - { content: 'THUMBS_DOWN', viewerHasReacted: false, reactors: { totalCount: 0 } }, - { content: 'LAUGH', viewerHasReacted: false, reactors: { totalCount: 0 } }, - { content: 'HOORAY', viewerHasReacted: false, reactors: { totalCount: 0 } }, - { content: 'CONFUSED', viewerHasReacted: false, reactors: { totalCount: 0 } }, - { content: 'HEART', viewerHasReacted: false, reactors: { totalCount: 0 } }, - { content: 'ROCKET', viewerHasReacted: false, reactors: { totalCount: 0 } }, + { + content: 'THUMBS_UP', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'THUMBS_DOWN', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'LAUGH', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'HOORAY', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'CONFUSED', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'HEART', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'ROCKET', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, { content: 'EYES', viewerHasReacted: false, reactors: { totalCount: 0 } }, ], }; @@ -123,14 +151,46 @@ describe('fetchAllThreadComments', () => { pageInfo: { hasNextPage: false, endCursor: null }, nodes: [ commentNode(1, [ - { content: 'THUMBS_UP', viewerHasReacted: true, reactors: { totalCount: 3 } }, - { content: 'THUMBS_DOWN', viewerHasReacted: false, reactors: { totalCount: 0 } }, - { content: 'LAUGH', viewerHasReacted: false, reactors: { totalCount: 0 } }, - { content: 'HOORAY', viewerHasReacted: false, reactors: { totalCount: 0 } }, - { content: 'CONFUSED', viewerHasReacted: false, reactors: { totalCount: 0 } }, - { content: 'HEART', viewerHasReacted: false, reactors: { totalCount: 1 } }, - { content: 'ROCKET', viewerHasReacted: false, reactors: { totalCount: 0 } }, - { content: 'EYES', viewerHasReacted: false, reactors: { totalCount: 0 } }, + { + content: 'THUMBS_UP', + viewerHasReacted: true, + reactors: { totalCount: 3 }, + }, + { + content: 'THUMBS_DOWN', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'LAUGH', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'HOORAY', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'CONFUSED', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'HEART', + viewerHasReacted: false, + reactors: { totalCount: 1 }, + }, + { + content: 'ROCKET', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'EYES', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, ]), ], }, diff --git a/apps/web/src/routers/code-reviews-router.test.ts b/apps/web/src/routers/code-reviews-router.test.ts index 7cf4d14821..3c7469fbc1 100644 --- a/apps/web/src/routers/code-reviews-router.test.ts +++ b/apps/web/src/routers/code-reviews-router.test.ts @@ -2251,3 +2251,620 @@ describe('review agent config repository model overrides', () => { ).rejects.toMatchObject({ code: 'BAD_REQUEST' }); }); }); + +describe('personalReviewAgent.patchReviewConfig', () => { + let testUser: User; + + beforeAll(async () => { + testUser = await insertTestUser(); + }); + + afterEach(async () => { + await db + .delete(agent_configs) + .where( + and( + eq(agent_configs.agent_type, 'code_review'), + eq(agent_configs.owned_by_user_id, testUser.id) + ) + ); + await db + .delete(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, testUser.id)); + mockSyncWebhooksForRepositories.mockReset(); + }); + + afterAll(async () => { + await db.delete(kilocode_users).where(eq(kilocode_users.id, testUser.id)); + }); + + beforeEach(() => { + mockGetValidGitLabToken.mockReset(); + mockSyncWebhooksForRepositories.mockReset(); + mockSyncWebhooksForRepositories.mockResolvedValue({ + result: { created: [], updated: [], deleted: [], errors: [] }, + updatedWebhooks: {}, + }); + }); + + // Seeds a GitHub personal config with values that the patch should NOT + // touch: manuallyAddedRepositories, repositoryModelOverrides, and the + // review_memory_enabled / review_analytics_enabled feature flags. Each + // round-trip test asserts all of these survive a mobile-shaped patch. + async function seedPersonalGithubConfig() { + await db.insert(agent_configs).values({ + owned_by_user_id: testUser.id, + agent_type: 'code_review', + platform: 'github', + config: { + review_style: 'balanced', + focus_areas: ['bugs'], + custom_instructions: 'be terse', + model_slug: 'anthropic/claude-sonnet-5', + thinking_effort: null, + gate_threshold: 'off', + repository_selection_mode: 'all', + selected_repository_ids: [101, 202], + manually_added_repositories: [ + { id: 9, name: 'manual', full_name: 'manual/repo', private: true }, + ], + repository_model_overrides: [ + { + repository_id: 101, + repo_full_name: 'acme/api', + model_slug: 'openai/gpt-5', + thinking_effort: 'high', + }, + ], + disable_review_md: true, + // Web-only setting the patch schema never carries; `false` proves + // the patch passes it through instead of resetting to the default. + skip_bot_pull_requests: false, + review_memory_enabled: true, + review_analytics_enabled: true, + }, + is_enabled: false, + created_by: testUser.id, + }); + } + + it('returns NOT_FOUND when no stored personal config exists', async () => { + const caller = await createCallerForUser(testUser.id); + + await expect( + caller.personalReviewAgent.patchReviewConfig({ + platform: 'github', + reviewStyle: 'strict', + }) + ).rejects.toMatchObject({ code: 'NOT_FOUND' }); + + const stored = await db.query.agent_configs.findFirst({ + where: and( + eq(agent_configs.agent_type, 'code_review'), + eq(agent_configs.owned_by_user_id, testUser.id) + ), + }); + // PATCH must not have created a row. + expect(stored).toBeUndefined(); + }); + + it('preserves manuallyAddedRepositories and repositoryModelOverrides when the patch omits them', async () => { + await seedPersonalGithubConfig(); + const caller = await createCallerForUser(testUser.id); + + await caller.personalReviewAgent.patchReviewConfig({ + platform: 'github', + reviewStyle: 'strict', + focusAreas: ['security'], + modelSlug: 'openai/gpt-5', + }); + + const stored = await db.query.agent_configs.findFirst({ + where: and( + eq(agent_configs.agent_type, 'code_review'), + eq(agent_configs.owned_by_user_id, testUser.id) + ), + }); + + expect(stored?.config).toEqual( + expect.objectContaining({ + review_style: 'strict', + focus_areas: ['security'], + model_slug: 'openai/gpt-5', + // Preserved by the field-merge: + manually_added_repositories: [ + { id: 9, name: 'manual', full_name: 'manual/repo', private: true }, + ], + repository_model_overrides: [ + { + repository_id: 101, + repo_full_name: 'acme/api', + model_slug: 'openai/gpt-5', + thinking_effort: 'high', + }, + ], + selected_repository_ids: [101, 202], + repository_selection_mode: 'all', + gate_threshold: 'off', + disable_review_md: true, + // Web-only setting preserved by the patch pass-through: + skip_bot_pull_requests: false, + // Feature flags preserved by `preserveCodeReviewFeatureSettings`: + review_memory_enabled: true, + review_analytics_enabled: true, + }) + ); + }); + + it('forces GitLab repository_selection_mode to selected when the patch omits it', async () => { + await db.insert(agent_configs).values({ + owned_by_user_id: testUser.id, + agent_type: 'code_review', + platform: 'gitlab', + config: { + review_style: 'balanced', + focus_areas: [], + model_slug: 'test-model', + repository_selection_mode: 'all', + selected_repository_ids: [101], + }, + is_enabled: false, + created_by: testUser.id, + }); + const caller = await createCallerForUser(testUser.id); + + await caller.personalReviewAgent.patchReviewConfig({ + platform: 'gitlab', + // `repositorySelectionMode` deliberately omitted — GitLab forcing + // must still clamp to 'selected' post-merge. + modelSlug: 'openai/gpt-5', + }); + + const stored = await db.query.agent_configs.findFirst({ + where: and( + eq(agent_configs.agent_type, 'code_review'), + eq(agent_configs.owned_by_user_id, testUser.id) + ), + }); + expect(stored?.config).toEqual( + expect.objectContaining({ + repository_selection_mode: 'selected', + model_slug: 'openai/gpt-5', + }) + ); + }); + + it('does not run GitLab webhook sync when selectedRepositoryIds is absent from the patch', async () => { + await seedPersonalGithubConfig(); + // Switch the stored row to gitlab so the proc's gitlab branch is + // reachable. The patch only sends focusAreas — selection is untouched, + // so webhook sync must NOT run. + await db + .update(agent_configs) + .set({ platform: 'gitlab' }) + .where( + and( + eq(agent_configs.agent_type, 'code_review'), + eq(agent_configs.owned_by_user_id, testUser.id) + ) + ); + const caller = await createCallerForUser(testUser.id); + + const result = await caller.personalReviewAgent.patchReviewConfig({ + platform: 'gitlab', + focusAreas: ['performance'], + }); + + expect(result.success).toBe(true); + expect(result.webhookSync).toBeNull(); + expect(mockSyncWebhooksForRepositories).not.toHaveBeenCalled(); + }); + + it('runs GitLab webhook sync only when selectedRepositoryIds is present in the patch', async () => { + await db.insert(agent_configs).values({ + owned_by_user_id: testUser.id, + agent_type: 'code_review', + platform: 'gitlab', + config: { + review_style: 'balanced', + focus_areas: [], + model_slug: 'test-model', + repository_selection_mode: 'selected', + selected_repository_ids: [101, 202], + review_memory_enabled: true, + review_analytics_enabled: true, + }, + is_enabled: false, + created_by: testUser.id, + }); + await db.insert(platform_integrations).values({ + owned_by_user_id: testUser.id, + platform: 'gitlab', + integration_type: 'oauth', + integration_status: 'active', + metadata: { + webhook_secret: 'webhook-secret', + gitlab_instance_url: 'https://gitlab.example.com', + configured_webhooks: {}, + }, + }); + mockGetValidGitLabToken.mockResolvedValue('gitlab-token'); + const caller = await createCallerForUser(testUser.id); + + await caller.personalReviewAgent.patchReviewConfig({ + platform: 'gitlab', + selectedRepositoryIds: [202, 303], + }); + + expect(mockSyncWebhooksForRepositories).toHaveBeenCalledWith( + 'gitlab-token', + 'webhook-secret', + [202, 303], + [101, 202], + {}, + 'https://gitlab.example.com' + ); + }); + + // P0-B-13b: real mobile→server contract guard. The mobile + // useSaveReviewConfig hook now sends a partial patch whose shape is + // exactly { platform, ...editedFields } — no manuallyAddedRepositories, + // no repositoryModelOverrides, no autoConfigureWebhooks. The server + // PATCH must field-merge those absent keys from the stored config. + it('preserves a config seeded with manuallyAddedRepositories + overrides when a mobile-shaped patch is applied', async () => { + await seedPersonalGithubConfig(); + const caller = await createCallerForUser(testUser.id); + + // Mobile-shaped patch: ONLY the keys the mobile UI lets the user edit. + // The personal PATCH schema does not even accept + // manuallyAddedRepositories / repositoryModelOverrides / + // autoConfigureWebhooks here — the contract guard is that the server + // never asks for them. + await caller.personalReviewAgent.patchReviewConfig({ + platform: 'github', + reviewStyle: 'strict', + focusAreas: ['security'], + }); + + const stored = await db.query.agent_configs.findFirst({ + where: and( + eq(agent_configs.agent_type, 'code_review'), + eq(agent_configs.owned_by_user_id, testUser.id) + ), + }); + + // Patched fields applied. + expect(stored?.config).toEqual( + expect.objectContaining({ + review_style: 'strict', + focus_areas: ['security'], + }) + ); + // Stored fields NOT in the mobile-shaped patch must round-trip + // unchanged. The personal schema has no council/councilEnabled, so + // the mobile contract is specifically about manuallyAddedRepositories + // and repositoryModelOverrides. + expect(stored?.config).toEqual( + expect.objectContaining({ + manually_added_repositories: [ + { id: 9, name: 'manual', full_name: 'manual/repo', private: true }, + ], + repository_model_overrides: [ + { + repository_id: 101, + repo_full_name: 'acme/api', + model_slug: 'openai/gpt-5', + thinking_effort: 'high', + }, + ], + }) + ); + // Other stored fields that the mobile client never read or sent must + // also be preserved. + expect(stored?.config).toEqual( + expect.objectContaining({ + selected_repository_ids: [101, 202], + repository_selection_mode: 'all', + gate_threshold: 'off', + disable_review_md: true, + }) + ); + }); +}); + +// ============================================================================ +// P1-D-32: GitLab webhook secret handling on the personal surface. +// +// Regression guards for two security fixes: +// 1. `personalReviewAgent.getGitLabStatus` MUST NOT return the webhook +// secret. (Lower risk than the org path because the caller is the +// secret owner, but still a status-read leak that this slice removes.) +// 2. `gitlab.regenerateWebhookSecret` MUST re-sync the Kilo-managed +// webhooks so the integration keeps working with the new secret. The +// previous shape persisted a new secret and never re-synced, so live +// webhooks kept carrying the old secret and stopped validating. +// ============================================================================ + +async function seedPersonalGitLabIntegration(userId: string, metadata: Record) { + await db.insert(platform_integrations).values({ + owned_by_user_id: userId, + platform: 'gitlab', + integration_type: 'oauth', + integration_status: 'active', + platform_installation_id: `inst-${crypto.randomUUID()}`, + metadata: { ...metadata, webhook_secret: 'old-secret-do-not-leak' }, + }); +} + +async function readPersonalWebhookSecret(userId: string): Promise { + const row = await db.query.platform_integrations.findFirst({ + where: and( + eq(platform_integrations.owned_by_user_id, userId), + eq(platform_integrations.platform, 'gitlab') + ), + }); + return (row?.metadata as Record | null)?.webhook_secret as string | undefined; +} + +async function readPersonalConfiguredWebhooks( + userId: string +): Promise> { + const row = await db.query.platform_integrations.findFirst({ + where: and( + eq(platform_integrations.owned_by_user_id, userId), + eq(platform_integrations.platform, 'gitlab') + ), + }); + return ( + ((row?.metadata as Record | null)?.configured_webhooks as + | Record + | undefined) ?? {} + ); +} + +describe('personalReviewAgent.getGitLabStatus P1-D-32 (omits webhook secret)', () => { + let testUser: User; + + beforeAll(async () => { + testUser = await insertTestUser(); + }); + + beforeEach(() => { + mockGetValidGitLabToken.mockReset(); + mockSyncWebhooksForRepositories.mockReset(); + }); + + afterEach(async () => { + await db + .delete(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, testUser.id)); + }); + + afterAll(async () => { + await db.delete(kilocode_users).where(eq(kilocode_users.id, testUser.id)); + }); + + it('returns the integration shape WITHOUT webhookSecret for the self caller', async () => { + await seedPersonalGitLabIntegration(testUser.id, { + gitlab_instance_url: 'https://gitlab.example.com', + configured_webhooks: { '101': { hook_id: 9001, created_at: '2026-01-01T00:00:00Z' } }, + }); + + const caller = await createCallerForUser(testUser.id); + const status = await caller.personalReviewAgent.getGitLabStatus(); + + expect(status.connected).toBe(true); + expect(status.integration).toBeDefined(); + // Regression guard: the secret must NEVER appear in the status + // payload, even for the secret's owner. If this assertion fails, + // the leak has been re-introduced. + expect(status.integration).not.toHaveProperty('webhookSecret'); + expect((status.integration as Record).webhookSecret).toBeUndefined(); + // The rest of the shape is preserved (non-secret fields still ship; + // account/repositorySelection/installedAt pass through verbatim from the + // stored integration row regardless of their concrete values). + expect(status.integration).toEqual( + expect.objectContaining({ + isValid: true, + instanceUrl: 'https://gitlab.example.com', + }) + ); + expect(status.integration).toHaveProperty('accountLogin'); + expect(status.integration).toHaveProperty('repositorySelection'); + expect(status.integration).toHaveProperty('installedAt'); + }); +}); + +describe('gitlab.regenerateWebhookSecret P1-D-32 (self-only, re-syncs)', () => { + let testUser: User; + let otherUser: User; + + beforeAll(async () => { + testUser = await insertTestUser(); + otherUser = await insertTestUser(); + }); + + beforeEach(() => { + mockGetValidGitLabToken.mockReset(); + mockSyncWebhooksForRepositories.mockReset(); + // Default sync outcome: every currently-configured repo was "updated" + // (mirrors the `previous=[]` "treat all as added" path used by rotate). + mockSyncWebhooksForRepositories.mockImplementation( + async (_token, _secret, selectedIds, _previous, configuredWebhooks) => { + const updatedWebhooks: Record< + string, + { hook_id: number; created_at: string; updated_at?: string } + > = {}; + for (const id of selectedIds) { + const existing = configuredWebhooks[String(id)]; + updatedWebhooks[String(id)] = { + hook_id: existing?.hook_id ?? 1000 + Number(id), + created_at: existing?.created_at ?? new Date().toISOString(), + updated_at: new Date().toISOString(), + }; + } + return { + result: { + created: [], + updated: selectedIds.map((id: number) => ({ projectId: id, hookId: 1000 + id })), + deleted: [], + errors: [], + }, + updatedWebhooks, + }; + } + ); + mockGetValidGitLabToken.mockResolvedValue('gitlab-access-token'); + }); + + afterEach(async () => { + await db + .delete(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, testUser.id)); + await db + .delete(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, otherUser.id)); + }); + + afterAll(async () => { + await db.delete(kilocode_users).where(inArray(kilocode_users.id, [testUser.id, otherUser.id])); + }); + + it('persists a NEW secret, re-syncs webhooks with the new secret and previous=[]', async () => { + const configured = { + '101': { hook_id: 9001, created_at: '2026-01-01T00:00:00Z' }, + '202': { hook_id: 9002, created_at: '2026-01-02T00:00:00Z' }, + }; + await seedPersonalGitLabIntegration(testUser.id, { + gitlab_instance_url: 'https://gitlab.example.com', + configured_webhooks: configured, + }); + + const caller = await createCallerForUser(testUser.id); + const result = await caller.gitlab.regenerateWebhookSecret(); + + expect(typeof result.webhookSecret).toBe('string'); + expect(result.webhookSecret).toMatch(/^[0-9a-f]{64}$/); + expect(result.webhookSecret).not.toBe('old-secret-do-not-leak'); + + expect(mockSyncWebhooksForRepositories).toHaveBeenCalledTimes(1); + expect(mockSyncWebhooksForRepositories).toHaveBeenCalledWith( + 'gitlab-access-token', + result.webhookSecret, + [101, 202], + [], + configured, + 'https://gitlab.example.com' + ); + expect(result.webhookSync.updated).toBe(2); + expect(result.webhookSync.created).toBe(0); + expect(result.webhookSync.deleted).toBe(0); + expect(result.webhookSync.errors).toEqual([]); + expect(result.configuredWebhookCount).toBe(2); + + // Persistence: metadata.webhook_secret is the NEW secret and + // metadata.configured_webhooks was updated with the sync output. + expect(await readPersonalWebhookSecret(testUser.id)).toBe(result.webhookSecret); + const stored = await readPersonalConfiguredWebhooks(testUser.id); + expect(Object.keys(stored).sort()).toEqual(['101', '202']); + expect(stored['101']?.updated_at).toBeDefined(); + expect(stored['202']?.updated_at).toBeDefined(); + }); + + it('with empty configured_webhooks returns the new secret and does NOT call sync', async () => { + await seedPersonalGitLabIntegration(testUser.id, { + gitlab_instance_url: 'https://gitlab.example.com', + configured_webhooks: {}, + }); + + const caller = await createCallerForUser(testUser.id); + const result = await caller.gitlab.regenerateWebhookSecret(); + + expect(result.webhookSecret).toMatch(/^[0-9a-f]{64}$/); + expect(result.webhookSync).toEqual({ + created: 0, + updated: 0, + deleted: 0, + errors: [], + }); + expect(result.configuredWebhookCount).toBe(0); + expect(mockSyncWebhooksForRepositories).not.toHaveBeenCalled(); + // No token lookup needed when there are no webhooks to re-sync. + expect(mockGetValidGitLabToken).not.toHaveBeenCalled(); + expect(await readPersonalWebhookSecret(testUser.id)).toBe(result.webhookSecret); + }); + + it('is self-only: the callers own integration is rotated, never another users', async () => { + const configuredSelf = { '1': { hook_id: 1, created_at: '2026-01-01T00:00:00Z' } }; + const configuredOther = { '2': { hook_id: 2, created_at: '2026-01-01T00:00:00Z' } }; + await seedPersonalGitLabIntegration(testUser.id, { + gitlab_instance_url: 'https://gitlab.com', + configured_webhooks: configuredSelf, + }); + await seedPersonalGitLabIntegration(otherUser.id, { + gitlab_instance_url: 'https://gitlab.com', + configured_webhooks: configuredOther, + }); + + const caller = await createCallerForUser(testUser.id); + const result = await caller.gitlab.regenerateWebhookSecret(); + + // Self was rotated; other user is untouched. + expect(await readPersonalWebhookSecret(testUser.id)).toBe(result.webhookSecret); + expect(await readPersonalWebhookSecret(otherUser.id)).toBe('old-secret-do-not-leak'); + + // Sync was called only once, for the caller's configured repo id. + expect(mockSyncWebhooksForRepositories).toHaveBeenCalledTimes(1); + expect(mockSyncWebhooksForRepositories).toHaveBeenCalledWith( + 'gitlab-access-token', + result.webhookSecret, + [1], + [], + configuredSelf, + 'https://gitlab.com' + ); + }); + + it('persists the NEW secret even when the webhook re-sync throws (no lost-secret state)', async () => { + const configured = { + '404': { hook_id: 9040, created_at: '2026-03-01T00:00:00Z' }, + }; + await seedPersonalGitLabIntegration(testUser.id, { + gitlab_instance_url: 'https://gitlab.example.com', + configured_webhooks: configured, + }); + mockSyncWebhooksForRepositories.mockRejectedValueOnce(new Error('gitlab responded 500')); + + const caller = await createCallerForUser(testUser.id); + // Must not throw: losing the just-rotated secret while GitLab may + // already carry it would strand the caller's integration. + const result = await caller.gitlab.regenerateWebhookSecret(); + + expect(result.webhookSecret).toMatch(/^[0-9a-f]{64}$/); + expect(result.webhookSecret).not.toBe('old-secret-do-not-leak'); + expect(result.webhookSync.errors).toHaveLength(1); + expect(result.webhookSync.updated).toBe(0); + // New secret persisted for manual recovery; surfaced error omits it. + expect(await readPersonalWebhookSecret(testUser.id)).toBe(result.webhookSecret); + expect(JSON.stringify(result.webhookSync.errors)).not.toContain(result.webhookSecret); + }); + + it('persists the NEW secret even when the access-token lookup throws', async () => { + const configured = { + '505': { hook_id: 9050, created_at: '2026-03-02T00:00:00Z' }, + }; + await seedPersonalGitLabIntegration(testUser.id, { + gitlab_instance_url: 'https://gitlab.example.com', + configured_webhooks: configured, + }); + mockGetValidGitLabToken.mockRejectedValueOnce(new Error('token expired')); + + const caller = await createCallerForUser(testUser.id); + const result = await caller.gitlab.regenerateWebhookSecret(); + + expect(result.webhookSecret).toMatch(/^[0-9a-f]{64}$/); + expect(result.webhookSync.errors).toHaveLength(1); + expect(mockSyncWebhooksForRepositories).not.toHaveBeenCalled(); + expect(await readPersonalWebhookSecret(testUser.id)).toBe(result.webhookSecret); + }); +}); diff --git a/apps/web/src/routers/code-reviews-router.ts b/apps/web/src/routers/code-reviews-router.ts index 8c5a48ca4d..ef5bf161cf 100644 --- a/apps/web/src/routers/code-reviews-router.ts +++ b/apps/web/src/routers/code-reviews-router.ts @@ -30,6 +30,11 @@ import { createManualCodeReviewJob, ManualCodeReviewJobInputSchema, } from '@/lib/code-reviews/manual-code-review-jobs'; +import { + applyCodeReviewConfigPatch, + type CodeReviewFieldMergePatch, + type CodeReviewStoredConfig, +} from '@kilocode/app-shared/code-review'; const PlatformSchema = z.enum(['github', 'gitlab']).default('github'); @@ -108,6 +113,40 @@ const SaveReviewConfigInputSchema = z.object({ autoConfigureWebhooks: z.boolean().optional().default(true), }); +// Field-merge PATCH schema for personal users. Strict subset of +// SaveReviewConfigInputSchema: every field is optional (omission = preserve +// stored value), no defaults. Mobile uses this for partial updates; web forms +// stay on the full save. Reuses the same per-field bounds as +// SaveReviewConfigInputSchema so a partial update can't smuggle in values +// that the full save would reject. +const PatchReviewConfigInputSchema = z.object({ + platform: PlatformSchema, + reviewStyle: z.enum(['strict', 'balanced', 'lenient', 'roast']).optional(), + focusAreas: z.array(z.string()).optional(), + customInstructions: z.string().optional(), + modelSlug: z.string().optional(), + thinkingEffort: z + .string() + .max(50) + .regex(/^[a-zA-Z]+$/) + .nullable() + .optional(), + repositorySelectionMode: z.enum(['all', 'selected']).optional(), + selectedRepositoryIds: z.array(z.number()).optional(), + manuallyAddedRepositories: z.array(ManuallyAddedRepositoryInputSchema).optional(), + repositoryModelOverrides: z + .array(RepositoryModelOverrideInputSchema) + .max(MAX_REPOSITORY_MODEL_OVERRIDES) + .superRefine(rejectDuplicateRepositoryModelOverrides) + .optional(), + disableReviewMd: z.boolean().optional(), + gateThreshold: z.enum(['off', 'all', 'warning', 'critical']).optional(), + // GitLab-specific: auto-configure webhooks. Only consulted when + // `selectedRepositoryIds` is also present in the patch (we only re-sync + // webhooks in that case, matching the full-save gating). + autoConfigureWebhooks: z.boolean().optional(), +}); + export const personalReviewAgentRouter = createTRPCRouter({ createManualReviewJob: baseProcedure .input(ManualCodeReviewJobInputSchema) @@ -164,9 +203,12 @@ export const personalReviewAgentRouter = createTRPCRouter({ }; } - // Extract webhook secret from metadata for display + // NOTE: The webhook secret is intentionally NOT returned here. The + // previous shape leaked it on every status read (self-only, but still + // a status-read leak). The secret is now surfaced only via the + // self-gated `gitlab.regenerateWebhookSecret` mutation (returned + // once, on demand). See P1-D-32. const metadata = integration.metadata as Record | null; - const webhookSecret = metadata?.webhook_secret as string | undefined; return { connected: true, @@ -175,7 +217,6 @@ export const personalReviewAgentRouter = createTRPCRouter({ repositorySelection: integration.repository_access, installedAt: integration.installed_at, isValid: true, // GitLab OAuth doesn't have suspension concept - webhookSecret, // Include webhook secret for user to configure in GitLab instanceUrl: (metadata?.gitlab_instance_url as string) || 'https://gitlab.com', }, }; @@ -400,6 +441,220 @@ export const personalReviewAgentRouter = createTRPCRouter({ } }), + /** + * Field-merge PATCH for the personal user's review agent configuration. + * + * Unlike `saveReviewConfig` (full-document overwrite), this procedure READS + * the current stored config and applies ONLY the fields present in the + * patch — every unlisted field is preserved verbatim. Designed for the + * mobile app, which edits one or two settings at a time and must not + * clobber `manuallyAddedRepositories`, `repositoryModelOverrides`, or + * feature flags the mobile UI doesn't surface. + * + * Platform forcing from the full save is re-applied post-merge (GitLab + * forces `repository_selection_mode = 'selected'`). GitLab webhook sync + * runs ONLY when `selectedRepositoryIds` is present in the patch, so an + * unrelated edit (e.g. `modelSlug` only) never touches integration + * metadata. + * + * `preserveCodeReviewFeatureSettings: true` keeps `review_memory_enabled` + * and `review_analytics_enabled` on the row even when the patch supplies + * neither, matching the full-save contract. + * + * Throws `NOT_FOUND` if no stored config exists — a PATCH cannot + * bootstrap a row (use `saveReviewConfig` for that). + */ + patchReviewConfig: baseProcedure + .input(PatchReviewConfigInputSchema) + .mutation(async ({ input, ctx }) => { + try { + const owner = { type: 'user' as const, id: ctx.user.id, userId: ctx.user.id }; + const platform = input.platform; + + const previousConfig = await getAgentConfigForOwner(owner, 'code_review', platform); + if (!previousConfig) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: + 'No existing review agent configuration to patch. Save one with saveReviewConfig first.', + }); + } + const previousRepoIds = + ((previousConfig.config as CodeReviewAgentConfig | undefined)?.selected_repository_ids as + | Array + | undefined) || []; + + // Convert the stored snake_case config to a camelCase snapshot for + // the merge helper. Mirrors the field mapping used in + // `getReviewConfig` so a round-trip PATCH is a no-op on the read + // shape. + const prevCfg = previousConfig.config as CodeReviewAgentConfig; + const stored: CodeReviewStoredConfig = { + reviewStyle: prevCfg.review_style || 'balanced', + focusAreas: prevCfg.focus_areas || [], + customInstructions: prevCfg.custom_instructions ?? null, + modelSlug: prevCfg.model_slug || PRIMARY_DEFAULT_MODEL, + thinkingEffort: prevCfg.thinking_effort ?? null, + gateThreshold: prevCfg.gate_threshold ?? 'off', + repositorySelectionMode: prevCfg.repository_selection_mode || 'all', + selectedRepositoryIds: prevCfg.selected_repository_ids ?? [], + repositoryModelOverrides: (prevCfg.repository_model_overrides ?? []).map(o => ({ + repositoryId: o.repository_id, + repoFullName: o.repo_full_name, + modelSlug: o.model_slug, + thinkingEffort: o.thinking_effort ?? null, + })), + disableReviewMd: prevCfg.disable_review_md ?? true, + manuallyAddedRepositories: prevCfg.manually_added_repositories || [], + }; + + // Field-merge: every key absent from `input` is preserved from + // `stored`. `null` is an explicit "clear" (e.g. customInstructions). + // Council-related keys aren't accepted by the personal input schema + // so they can never reach this handler. + const { platform: _ignored, ...rest } = input; + const patch: CodeReviewFieldMergePatch = rest; + const merged = applyCodeReviewConfigPatch(stored, patch); + + // Re-apply platform forcing post-merge. GitLab only supports + // 'selected' repo mode server-side, so an omitted or 'all' + // repositorySelectionMode is clamped to 'selected' here. The full + // save applies the same clamp; the PATCH must too or a save→patch + // round-trip could land on an invalid mode. + const isGitLab = platform === PLATFORM.GITLAB; + const repositorySelectionMode: 'all' | 'selected' = isGitLab + ? 'selected' + : (merged.repositorySelectionMode ?? 'all'); + + const repositoryModelOverrides: RepositoryModelOverride[] = ( + merged.repositoryModelOverrides ?? [] + ).map(override => ({ + repository_id: override.repositoryId, + repo_full_name: override.repoFullName, + model_slug: override.modelSlug, + thinking_effort: override.thinkingEffort ?? null, + })); + + await upsertAgentConfigForOwner({ + owner, + agentType: 'code_review', + platform, + config: { + review_style: merged.reviewStyle ?? 'balanced', + focus_areas: merged.focusAreas ?? [], + custom_instructions: merged.customInstructions ?? null, + model_slug: merged.modelSlug ?? PRIMARY_DEFAULT_MODEL, + thinking_effort: merged.thinkingEffort ?? null, + gate_threshold: merged.gateThreshold ?? 'off', + repository_selection_mode: repositorySelectionMode, + selected_repository_ids: (merged.selectedRepositoryIds ?? []).filter( + (repositoryId): repositoryId is number => typeof repositoryId === 'number' + ), + manually_added_repositories: merged.manuallyAddedRepositories ?? [], + repository_model_overrides: repositoryModelOverrides, + disable_review_md: merged.disableReviewMd ?? true, + // The patch schema never carries `skipBotPullRequests` (a + // web-only setting from the full save). Pass the stored value + // through so a PATCH can't silently reset it to the default — + // same field-merge contract as every other omitted field. + skip_bot_pull_requests: prevCfg.skip_bot_pull_requests ?? true, + review_memory_enabled: false, + review_analytics_enabled: false, + }, + preserveCodeReviewFeatureSettings: true, + createdBy: ctx.user.id, + }); + + // GitLab webhook sync runs ONLY when the patch actually carries + // `selectedRepositoryIds`. A patch that doesn't touch selection + // (e.g. mobile updating `focusAreas`) must not mutate integration + // metadata. Auto-configure is honored when present, defaulting to + // true to match the full-save default. + let webhookSyncResult = null; + if ( + isGitLab && + input.selectedRepositoryIds !== undefined && + (input.autoConfigureWebhooks ?? true) && + repositorySelectionMode === 'selected' + ) { + const integration = await getIntegrationForOwner(owner, PLATFORM.GITLAB); + if (integration) { + const metadata = integration.metadata as Record | null; + const webhookSecret = metadata?.webhook_secret as string | undefined; + const instanceUrl = + (metadata?.gitlab_instance_url as string | undefined) || 'https://gitlab.com'; + const configuredWebhooks = + (metadata?.configured_webhooks as Record) || {}; + + if (webhookSecret) { + try { + const accessToken = await getValidGitLabToken(integration, { + userId: ctx.user.id, + }); + + const selectedRepositoryIds = (input.selectedRepositoryIds ?? []).filter( + (repositoryId): repositoryId is number => typeof repositoryId === 'number' + ); + const previousSelectedRepositoryIds = previousRepoIds.filter( + (repositoryId): repositoryId is number => typeof repositoryId === 'number' + ); + const { result, updatedWebhooks } = await syncWebhooksForRepositories( + accessToken, + webhookSecret, + selectedRepositoryIds, + previousSelectedRepositoryIds, + configuredWebhooks, + instanceUrl + ); + + await updateIntegrationMetadataForOwner(owner, PLATFORM.GITLAB, { + configured_webhooks: updatedWebhooks, + }); + + webhookSyncResult = { + created: result.created.length, + updated: result.updated.length, + deleted: result.deleted.length, + errors: result.errors, + }; + + logExceptInTest('[patchReviewConfig] Webhook sync completed', webhookSyncResult); + } catch (webhookError) { + logExceptInTest('[patchReviewConfig] Webhook sync failed', { + error: + webhookError instanceof Error ? webhookError.message : String(webhookError), + }); + webhookSyncResult = { + created: 0, + updated: 0, + deleted: 0, + errors: [ + { + projectId: 0, + error: webhookError instanceof Error ? webhookError.message : 'Unknown error', + operation: 'sync' as const, + }, + ], + }; + } + } + } + } + + return { + success: true, + webhookSync: webhookSyncResult, + }; + } catch (error) { + if (error instanceof TRPCError) throw error; + console.error('Error patching review config:', error); + throw new TRPCError({ + code: 'INTERNAL_SERVER_ERROR', + message: 'Failed to patch review configuration', + }); + } + }), + /** * Toggles the review agent on/off for personal user */ diff --git a/apps/web/src/routers/github-pr-review-graphql-schema.test.ts b/apps/web/src/routers/github-pr-review-graphql-schema.test.ts new file mode 100644 index 0000000000..2337282bbe --- /dev/null +++ b/apps/web/src/routers/github-pr-review-graphql-schema.test.ts @@ -0,0 +1,62 @@ +// Schema-validity test for every raw PR-Review GraphQL document in +// `github-pr-review-router.ts`. Pinned against GitHub's official GraphQL SDL +// (bundled in `@octokit/graphql-schema`, auto-updated by that package) using +// `graphql`'s `parse` + `validate` so a future GitHub schema change or a +// hand-edited typo is caught at test time rather than at runtime against +// `octokit.request('POST /graphql', …)`. + +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; + +import { buildClientSchema, parse, validate } from 'graphql'; + +import { describe, test, expect } from '@jest/globals'; + +import { PR_REVIEW_GRAPHQL_DOCUMENTS } from '@/routers/github-pr-review-router'; + +// `@octokit/graphql-schema` is published as an ESM-only package (`"type": +// "module"`), which Jest's CJS test runner cannot `import` directly without +// enabling `--experimental-vm-modules`. We still depend on the package — it +// auto-updates `schema.json` (GitHub's authoritative GraphQL introspection +// result) and `schema.graphql` (the matching SDL) on every GitHub schema +// change — and read the introspection JSON off disk so the dependency is +// exercised. `buildClientSchema` is the recommended way to materialize a +// schema from an introspection result and is what `@octokit/graphql-schema`'s +// own `validate` helper uses internally; the SDL cannot be passed to +// `buildSchema` directly because it contains extension types that the strict +// SDL builder rejects. If the dependency is silently dropped, this read +// fails and the test errors loudly. +const introspectionPath = join(__dirname, '../../node_modules/@octokit/graphql-schema/schema.json'); +const introspection = JSON.parse(readFileSync(introspectionPath, 'utf8')); +const githubSchema = buildClientSchema(introspection); + +describe('github-pr-review-router GraphQL documents', () => { + test('exports exactly 10 documents (sanity guard for the export record)', () => { + expect(Object.keys(PR_REVIEW_GRAPHQL_DOCUMENTS)).toHaveLength(10); + }); + + test.each(Object.entries(PR_REVIEW_GRAPHQL_DOCUMENTS))( + '%s is valid against the GitHub GraphQL schema', + (_name, doc) => { + const parsed = parse(doc); + const errors = validate(githubSchema, parsed); + expect(errors).toEqual([]); + } + ); + + test('validate() flags a deliberately broken document (teeth guard)', () => { + // Reference a field that does not exist on the GitHub `Repository` type + // (`definitelyNotAFieldOnRepository`). If validate() ever stops being + // strict, this test will start passing-on-bad-docs and the guard fails. + const broken = /* GraphQL */ ` + query BrokenTeethGuard { + repository(owner: "x", name: "y") { + definitelyNotAFieldOnRepository + } + } + `; + const parsed = parse(broken); + const errors = validate(githubSchema, parsed); + expect(errors.length).toBeGreaterThan(0); + }); +}); diff --git a/apps/web/src/routers/github-pr-review-router.test.ts b/apps/web/src/routers/github-pr-review-router.test.ts index 2997465f9d..ace5abbbea 100644 --- a/apps/web/src/routers/github-pr-review-router.test.ts +++ b/apps/web/src/routers/github-pr-review-router.test.ts @@ -24,6 +24,7 @@ type OctokitMock = { createReplyForReviewComment: jest.Mock; updateBranch: jest.Mock; listFiles: jest.Mock; + get: jest.Mock; }; git: { deleteRef: jest.Mock }; request: jest.Mock; @@ -43,6 +44,7 @@ function buildOctokit(token: string): OctokitMock { createReplyForReviewComment: jest.fn(), updateBranch: jest.fn(), listFiles: jest.fn(), + get: jest.fn(), }, git: { deleteRef: jest.fn() }, request: jest.fn(), @@ -51,6 +53,25 @@ function buildOctokit(token: string): OctokitMock { return octokit; } +const SAME_REPO_ID = 101; +const OTHER_REPO_ID = 202; + +type PrGetFixture = { + headRef: string; + headSha: string; + headRepoId: number; + baseRepoId: number; +}; + +function mockPrGet(octokit: OctokitMock, fixture: PrGetFixture) { + octokit.pulls.get.mockResolvedValueOnce({ + data: { + head: { ref: fixture.headRef, sha: fixture.headSha, repo: { id: fixture.headRepoId } }, + base: { ref: 'main', repo: { id: fixture.baseRepoId } }, + }, + }); +} + jest.mock('@/lib/github-pr-review/client', () => ({ createGitHubPrReviewOctokit: (token: string) => buildOctokit(token), GITHUB_API_BASE_URL: 'https://api.github.com', @@ -99,6 +120,18 @@ describe('githubPrReviewRouter.mergePullRequest', () => { const caller = createCaller({ user: { id: 'user-1' } as User }); const firstOctokit = buildOctokit('t1'); + // The server now derives same-repo from the fetched PR; this test + // exercises the legacy `isCrossRepo: true` path which the server must + // ignore in favor of its derived value. We make the fetched PR same-repo + // and the `isCrossRepo: true` claim must NOT prevent the delete (see + // the "legacy fields are accepted and ignored" test). To keep this + // test's intent intact, we make the fetched PR cross-repo here. + mockPrGet(firstOctokit, { + headRef: 'feature/x', + headSha: 'a'.repeat(40), + headRepoId: OTHER_REPO_ID, + baseRepoId: SAME_REPO_ID, + }); firstOctokit.pulls.merge.mockResolvedValueOnce({ data: { merged: true, sha: 'mergedsha', message: 'PR merged' }, }); @@ -106,6 +139,7 @@ describe('githubPrReviewRouter.mergePullRequest', () => { const result = await caller.mergePullRequest({ ...baseMergeInput, isCrossRepo: true }); expect(result).toEqual({ merged: true, sha: 'mergedsha', branchDeleted: false }); + expect(firstOctokit.pulls.get).toHaveBeenCalledTimes(1); expect(firstOctokit.pulls.merge).toHaveBeenCalledTimes(1); expect(firstOctokit.git.deleteRef).not.toHaveBeenCalled(); }); @@ -115,6 +149,12 @@ describe('githubPrReviewRouter.mergePullRequest', () => { const caller = createCaller({ user: { id: 'user-1' } as User }); const firstOctokit = buildOctokit('t1'); + mockPrGet(firstOctokit, { + headRef: 'feature/x', + headSha: 'a'.repeat(40), + headRepoId: SAME_REPO_ID, + baseRepoId: SAME_REPO_ID, + }); firstOctokit.pulls.merge.mockResolvedValueOnce({ data: { merged: true, sha: 'mergedsha', message: 'PR merged' }, }); @@ -125,11 +165,38 @@ describe('githubPrReviewRouter.mergePullRequest', () => { expect(firstOctokit.git.deleteRef).not.toHaveBeenCalled(); }); + it('reports merged:false and skips branch delete when GitHub declines the merge', async () => { + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + + const firstOctokit = buildOctokit('t1'); + mockPrGet(firstOctokit, { + headRef: 'feature/x', + headSha: 'a'.repeat(40), + headRepoId: SAME_REPO_ID, + baseRepoId: SAME_REPO_ID, + }); + firstOctokit.pulls.merge.mockResolvedValueOnce({ + data: { merged: false, sha: 'mergedsha', message: 'PR is not mergeable' }, + }); + + const result = await caller.mergePullRequest({ ...baseMergeInput, deleteBranch: true }); + + expect(result).toEqual({ merged: false, sha: 'mergedsha', branchDeleted: false }); + expect(firstOctokit.git.deleteRef).not.toHaveBeenCalled(); + }); + it('reports branchDeleted=true on a successful same-repo delete', async () => { getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); const caller = createCaller({ user: { id: 'user-1' } as User }); const firstOctokit = buildOctokit('t1'); + mockPrGet(firstOctokit, { + headRef: 'feature/x', + headSha: 'a'.repeat(40), + headRepoId: SAME_REPO_ID, + baseRepoId: SAME_REPO_ID, + }); firstOctokit.pulls.merge.mockResolvedValueOnce({ data: { merged: true, sha: 'mergedsha', message: 'PR merged' }, }); @@ -150,6 +217,12 @@ describe('githubPrReviewRouter.mergePullRequest', () => { const caller = createCaller({ user: { id: 'user-1' } as User }); const firstOctokit = buildOctokit('t1'); + mockPrGet(firstOctokit, { + headRef: 'feature/x', + headSha: 'a'.repeat(40), + headRepoId: SAME_REPO_ID, + baseRepoId: SAME_REPO_ID, + }); firstOctokit.pulls.merge.mockResolvedValueOnce({ data: { merged: true, sha: 'mergedsha', message: 'PR merged' }, }); @@ -165,6 +238,167 @@ describe('githubPrReviewRouter.mergePullRequest', () => { }); }); +describe('githubPrReviewRouter.mergePullRequest P0-D-09 (spoofed headRef + cross-repo + sha fence)', () => { + // P0-D-09: a caller can no longer pick which ref gets deleted by sending + // a spoofed `headRef` (e.g. "main"). The server derives the head ref, + // same-repo identity, and head sha from `octokit.pulls.get` and fences + // `git.deleteRef` on those server-derived values. The legacy + // `headRef` / `isCrossRepo` fields are still accepted on the wire for + // backward compatibility with older shipped clients but have no effect. + + it('deletes the server-derived head ref and ignores a spoofed headRef in the payload', async () => { + // Baseline-demonstrating assertion: pre-fix, the `headRef: "main"` we + // send here would have been passed verbatim to `git.deleteRef` and + // the default branch would have been deleted. Post-fix, the only ref + // touched is the server-derived `heads/feature/x`. + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + + const firstOctokit = buildOctokit('t1'); + mockPrGet(firstOctokit, { + headRef: 'feature/x', + headSha: 'a'.repeat(40), + headRepoId: SAME_REPO_ID, + baseRepoId: SAME_REPO_ID, + }); + firstOctokit.pulls.merge.mockResolvedValueOnce({ + data: { merged: true, sha: 'mergedsha', message: 'PR merged' }, + }); + firstOctokit.git.deleteRef.mockResolvedValueOnce({ data: {} }); + + const result = await caller.mergePullRequest({ + ...baseMergeInput, + headRef: 'main', // spoofed — must be ignored + }); + + expect(result).toEqual({ merged: true, sha: 'mergedsha', branchDeleted: true }); + expect(firstOctokit.git.deleteRef).toHaveBeenCalledTimes(1); + expect(firstOctokit.git.deleteRef).toHaveBeenCalledWith({ + owner: 'octocat', + repo: 'hello', + ref: 'heads/feature/x', + }); + // Baseline assertion: the spoofed `main` ref never reaches `deleteRef`. + expect(firstOctokit.git.deleteRef).not.toHaveBeenCalledWith( + expect.objectContaining({ ref: 'heads/main' }) + ); + }); + + it('does not delete any ref when the fetched PR is cross-repo (head.repo.id !== base.repo.id)', async () => { + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + + const firstOctokit = buildOctokit('t1'); + mockPrGet(firstOctokit, { + headRef: 'feature/fork-branch', + headSha: 'a'.repeat(40), + headRepoId: OTHER_REPO_ID, + baseRepoId: SAME_REPO_ID, + }); + firstOctokit.pulls.merge.mockResolvedValueOnce({ + data: { merged: true, sha: 'mergedsha', message: 'PR merged' }, + }); + + const result = await caller.mergePullRequest({ ...baseMergeInput, isCrossRepo: false }); + + expect(result).toEqual({ merged: true, sha: 'mergedsha', branchDeleted: false }); + expect(firstOctokit.git.deleteRef).not.toHaveBeenCalled(); + }); + + it('does not delete when the fetched head sha does not match expectedHeadSha', async () => { + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + + const firstOctokit = buildOctokit('t1'); + // The PR's real head sha moved since the caller rendered the merge + // sheet; the caller's `expectedHeadSha` is stale. + mockPrGet(firstOctokit, { + headRef: 'feature/x', + headSha: 'b'.repeat(40), + headRepoId: SAME_REPO_ID, + baseRepoId: SAME_REPO_ID, + }); + firstOctokit.pulls.merge.mockResolvedValueOnce({ + data: { merged: true, sha: 'mergedsha', message: 'PR merged' }, + }); + + const result = await caller.mergePullRequest({ ...baseMergeInput }); + + expect(result).toEqual({ merged: true, sha: 'mergedsha', branchDeleted: false }); + expect(firstOctokit.git.deleteRef).not.toHaveBeenCalled(); + }); + + it('still accepts a request that includes legacy headRef + isCrossRepo (no rejection)', async () => { + // The schema must TOLERATE the legacy wire fields (older shipped + // clients still send them). Even when the caller sends + // `isCrossRepo: true`, the server derives same-repo from the fetched + // PR and proceeds to delete when the PR is actually same-repo. + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + + const firstOctokit = buildOctokit('t1'); + mockPrGet(firstOctokit, { + headRef: 'feature/x', + headSha: 'a'.repeat(40), + headRepoId: SAME_REPO_ID, + baseRepoId: SAME_REPO_ID, + }); + firstOctokit.pulls.merge.mockResolvedValueOnce({ + data: { merged: true, sha: 'mergedsha', message: 'PR merged' }, + }); + firstOctokit.git.deleteRef.mockResolvedValueOnce({ data: {} }); + + const result = await caller.mergePullRequest({ + ...baseMergeInput, + headRef: 'feature/x', + isCrossRepo: true, // legacy lie — must be ignored + }); + + // The server-derived same-repo identity wins; the delete proceeds on + // the server-derived head ref. + expect(result).toEqual({ merged: true, sha: 'mergedsha', branchDeleted: true }); + expect(firstOctokit.git.deleteRef).toHaveBeenCalledWith({ + owner: 'octocat', + repo: 'hello', + ref: 'heads/feature/x', + }); + }); + + it('accepts a request that OMITS the legacy headRef + isCrossRepo fields entirely (new wire)', async () => { + // The new mobile wire drops `headRef` / `isCrossRepo` entirely. The + // schema must accept the request and derive everything from the + // fetched PR. + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + + const firstOctokit = buildOctokit('t1'); + mockPrGet(firstOctokit, { + headRef: 'feature/x', + headSha: 'a'.repeat(40), + headRepoId: SAME_REPO_ID, + baseRepoId: SAME_REPO_ID, + }); + firstOctokit.pulls.merge.mockResolvedValueOnce({ + data: { merged: true, sha: 'mergedsha', message: 'PR merged' }, + }); + firstOctokit.git.deleteRef.mockResolvedValueOnce({ data: {} }); + + // Strip the legacy fields. + const { headRef: _legacyHeadRef, isCrossRepo: _legacyIsCrossRepo, ...newWire } = baseMergeInput; + void _legacyHeadRef; + void _legacyIsCrossRepo; + + const result = await caller.mergePullRequest(newWire); + + expect(result).toEqual({ merged: true, sha: 'mergedsha', branchDeleted: true }); + expect(firstOctokit.git.deleteRef).toHaveBeenCalledWith({ + owner: 'octocat', + repo: 'hello', + ref: 'heads/feature/x', + }); + }); +}); + describe('githubPrReviewRouter infinite-query inputs accept the tRPC direction field', () => { // tRPC's useInfiniteQuery integration injects `direction: 'forward'|'backward'` // into the procedure input. The inputs are `.strict()`, so without an explicit @@ -627,6 +861,14 @@ describe('githubPrReviewRouter mutations go through withGitHubUserTokenRetry', ( const caller = createCaller({ user: { id: 'user-1' } as User }); const t1Octokit = buildOctokit('t1'); + // The server now fetches the PR first to derive head ref / same-repo / + // head sha; the actual merge call is the one that 409s here. + mockPrGet(t1Octokit, { + headRef: 'feature/x', + headSha: 'a'.repeat(40), + headRepoId: SAME_REPO_ID, + baseRepoId: SAME_REPO_ID, + }); t1Octokit.pulls.merge.mockRejectedValueOnce({ status: 409, message: 'Head branch was modified', diff --git a/apps/web/src/routers/github-pr-review-router.ts b/apps/web/src/routers/github-pr-review-router.ts index 68f0bf973f..8ad6655c72 100644 --- a/apps/web/src/routers/github-pr-review-router.ts +++ b/apps/web/src/routers/github-pr-review-router.ts @@ -128,7 +128,11 @@ const SubmitReviewInput = ownerRepoSchema body: z.string().min(1).max(65_535).optional(), commitSha: z.string().min(40).max(64), comments: z - .array(CommentPositionSchema.extend({ body: z.string().min(1).max(65_535) }).strict()) + .array( + CommentPositionSchema.extend({ + body: z.string().min(1).max(65_535), + }).strict() + ) .max(100) .optional(), }) @@ -143,6 +147,14 @@ const ReactionInput = z }) .strict(); +// `headRef` and `isCrossRepo` were required in an earlier wire version. Older +// shipped mobile clients still send them; newer clients omit them entirely. +// The schema stays `.strict()` (so genuinely unknown fields are still +// rejected) and tolerates these two legacy fields — they are accepted and +// IGNORED. The server derives the authoritative head ref / same-repo +// identity from `octokit.pulls.get` so a caller cannot spoof which ref gets +// deleted. Aligns with the tolerate-not-reject pattern near `direction` +// above. const MergePullRequestInput = ownerRepoSchema .extend({ number: prNumberSchema, @@ -151,8 +163,9 @@ const MergePullRequestInput = ownerRepoSchema commitMessage: z.string().min(1).max(65_535).optional(), deleteBranch: z.boolean(), expectedHeadSha: z.string().min(40).max(64), - headRef: z.string().min(1).max(255), - isCrossRepo: z.boolean(), + // Legacy fields — accepted for backward compat, ignored by the server. + headRef: z.string().min(1).max(255).optional(), + isCrossRepo: z.boolean().optional(), }) .strict(); @@ -441,6 +454,31 @@ function normalizeComment(node: GraphQlCommentNode) { export const REVIEW_THREAD_COMMENTS_FOLLOWUP_QUERY_FOR_TEST = REVIEW_THREAD_COMMENTS_FOLLOWUP_QUERY; export const CONVERSATION_COMMENTS_QUERY_FOR_TEST = CONVERSATION_COMMENTS_QUERY; +// All raw PR-Review GraphQL documents defined in this router, collected as a +// single exported record so the schema-validity test enumerates docs from +// module exports (newly added docs are auto-covered). Keys are the +// operation name / mutation tag; values are the unchanged document strings. +export const PR_REVIEW_GRAPHQL_DOCUMENTS = { + PULL_REQUEST_FRAGMENT_QUERY, + REVIEW_THREADS_QUERY, + REVIEW_THREAD_COMMENTS_FOLLOWUP_QUERY, + CONVERSATION_COMMENTS_QUERY, + ENABLE_AUTO_MERGE_MUTATION, + DISABLE_AUTO_MERGE_MUTATION, + RESOLVE_THREAD_MUTATION, + UNRESOLVE_THREAD_MUTATION, + ADD_REACTION_MUTATION, + REMOVE_REACTION_MUTATION, +} as const; + +// Exported for unit testing the reaction DTO invariant pinned against +// GitHub's actual `reactionGroups` shape. The downstream DTO contract — +// `Array<{ content: string; count: number; viewerHasReacted: boolean }>` — +// is consumed by `mappers.ts` and the mobile reactions row and must NOT +// change shape; see `normalize-reactions.test.ts`. +export const normalizeReactions_FOR_TEST = normalizeReactions; +export const normalizeComment_FOR_TEST = normalizeComment; + export async function fetchAllThreadComments(args: { octokit: ReturnType; threadId: string; @@ -635,14 +673,20 @@ export const githubPrReviewRouter = createTRPCRouter({ const repo = repoResp.data; // GraphQL for reviewDecision + viewer.login type OverviewGraphQl = { - repository: { pullRequest: { reviewDecision: string | null } | null } | null; + repository: { + pullRequest: { reviewDecision: string | null } | null; + } | null; viewer: { login: string } | null; }; let graphQl: OverviewGraphQl | null = null; try { const gqlResp = (await octokit.request('POST /graphql', { query: PULL_REQUEST_FRAGMENT_QUERY, - variables: { owner: input.owner, name: input.repo, number: input.number }, + variables: { + owner: input.owner, + name: input.repo, + number: input.number, + }, })) as { data: { data: OverviewGraphQl | null; errors?: unknown } }; throwTrpcFromGraphQlErrors(gqlResp.data.errors as never); graphQl = gqlResp.data.data ?? null; @@ -894,9 +938,13 @@ export const githubPrReviewRouter = createTRPCRouter({ const result = await withGitHubUserTokenRetry({ kiloUserId: ctx.user.id, call: async octokit => { - const variables = buildResolveThreadVariables({ threadId: input.threadId }); + const variables = buildResolveThreadVariables({ + threadId: input.threadId, + }); const payload = await runGraphQlMutation<{ - resolveReviewThread: { thread: { id: string; isResolved: boolean } } | null; + resolveReviewThread: { + thread: { id: string; isResolved: boolean }; + } | null; }>({ octokit, query: RESOLVE_THREAD_MUTATION, variables }); const thread = requireGraphQlOperation( payload.resolveReviewThread?.thread, @@ -912,9 +960,13 @@ export const githubPrReviewRouter = createTRPCRouter({ const result = await withGitHubUserTokenRetry({ kiloUserId: ctx.user.id, call: async octokit => { - const variables = buildUnresolveThreadVariables({ threadId: input.threadId }); + const variables = buildUnresolveThreadVariables({ + threadId: input.threadId, + }); const payload = await runGraphQlMutation<{ - unresolveReviewThread: { thread: { id: string; isResolved: boolean } } | null; + unresolveReviewThread: { + thread: { id: string; isResolved: boolean }; + } | null; }>({ octokit, query: UNRESOLVE_THREAD_MUTATION, variables }); const thread = requireGraphQlOperation( payload.unresolveReviewThread?.thread, @@ -970,10 +1022,41 @@ export const githubPrReviewRouter = createTRPCRouter({ // returns 409 and the caller should re-fetch. The branch delete after a // successful merge is BEST-EFFORT: failures are reported in the result // (never thrown) so the mobile client can surface a banner. + // + // P0-D-09: the head ref + same-repo identity are derived from + // `octokit.pulls.get` rather than the client input. A caller must not be + // able to merge PR #N with a valid `expectedHeadSha` and then delete an + // arbitrary same-repo ref (e.g. `main`) by spoofing `headRef`. The delete + // is fenced on the server-derived head sha matching `expectedHeadSha`, + // same-repo identity, and the merge actually completing. mergePullRequest: baseProcedure.input(MergePullRequestInput).mutation(async ({ ctx, input }) => { return withGitHubUserTokenRetry({ kiloUserId: ctx.user.id, call: async octokit => { + // Fetch the PR first so we know the authoritative head ref, head sha, + // and whether the head repo is the same as the base repo. A merge + // does not move the head branch, so the ref/sha derived here are + // valid for the post-merge delete decision. + const prResp = await octokit.pulls.get({ + owner: input.owner, + repo: input.repo, + pull_number: input.number, + }); + const pr = prResp.data; + const headRepo = pr.head?.repo ?? null; + const baseRepo = pr.base?.repo ?? null; + // Treat a null/absent head repo (e.g. deleted fork) as not-deletable; + // also bail if base.repo is missing for the same reason. Compare the + // numeric repo id — robust against name/owner changes. + const sameRepo = + headRepo !== null && + baseRepo !== null && + typeof headRepo.id === 'number' && + typeof baseRepo.id === 'number' && + headRepo.id === baseRepo.id; + const fetchedHeadSha = typeof pr.head?.sha === 'string' ? pr.head.sha : null; + const headRefName = typeof pr.head?.ref === 'string' ? pr.head.ref : null; + const params = buildMergePullRequestParams({ owner: input.owner, repo: input.repo, @@ -985,22 +1068,30 @@ export const githubPrReviewRouter = createTRPCRouter({ }); const response = await octokit.pulls.merge(params); const merged = Boolean(response.data.merged); - if (!merged || !input.deleteBranch || input.isCrossRepo) { + if ( + !merged || + !input.deleteBranch || + !sameRepo || + headRefName === null || + fetchedHeadSha === null || + fetchedHeadSha !== input.expectedHeadSha + ) { return { merged, sha: response.data.sha, branchDeleted: false as const, }; } - // Best-effort: only call deleteRef when the head is same-repo. - // Catch every error and surface it in the result instead of - // failing the whole mutation. + // Best-effort: only call deleteRef when the server-derived head is + // same-repo AND the head sha we fetched matches what the caller + // claimed to merge. Catch every error and surface it in the result + // instead of failing the whole mutation. try { await octokit.git.deleteRef( buildDeleteRefParams({ owner: input.owner, repo: input.repo, - headRef: input.headRef, + headRef: headRefName, }) ); return { @@ -1070,7 +1161,9 @@ export const githubPrReviewRouter = createTRPCRouter({ const result = await withGitHubUserTokenRetry({ kiloUserId: ctx.user.id, call: async octokit => { - const variables = buildDisableAutoMergeVariables({ prNodeId: input.prNodeId }); + const variables = buildDisableAutoMergeVariables({ + prNodeId: input.prNodeId, + }); const payload = await runGraphQlMutation<{ disablePullRequestAutoMerge: { pullRequest: { id: string } } | null; }>({ octokit, query: DISABLE_AUTO_MERGE_MUTATION, variables }); diff --git a/apps/web/src/routers/gitlab-router.ts b/apps/web/src/routers/gitlab-router.ts index b7b66f3c1c..912d4fa2c6 100644 --- a/apps/web/src/routers/gitlab-router.ts +++ b/apps/web/src/routers/gitlab-router.ts @@ -1,7 +1,9 @@ import 'server-only'; import { baseProcedure, createTRPCRouter } from '@/lib/trpc/init'; +import { TRPCError } from '@trpc/server'; import * as z from 'zod'; import * as gitlabService from '@/lib/integrations/gitlab-service'; +import { getValidGitLabToken } from '@/lib/integrations/gitlab-service'; import { ensureOrganizationAccess } from '@/routers/organizations/utils'; import { resolveOwner, @@ -12,6 +14,16 @@ import { validateGitLabInstance } from '@/lib/integrations/platforms/gitlab/adap import { validatePersonalAccessToken } from '@/lib/integrations/platforms/gitlab/adapter'; import { isPlatformIntegrationHealthy } from '@/lib/integrations/core/health'; import { requireNumericPlatformRepositories } from '@/lib/integrations/core/types'; +import { + getIntegrationForOwner, + updateIntegrationMetadataForOwner, +} from '@/lib/integrations/db/platform-integrations'; +import { + syncWebhooksForRepositories, + type ConfiguredWebhook, +} from '@/lib/integrations/platforms/gitlab/webhook-sync'; +import { logExceptInTest } from '@/lib/utils.server'; +import { randomBytes } from 'node:crypto'; export const gitlabRouter = createTRPCRouter({ /** @@ -198,17 +210,126 @@ export const gitlabRouter = createTRPCRouter({ ); }), - regenerateWebhookSecret: baseProcedure - .input( - z.object({ - organizationId: z.uuid().optional(), - }) - ) - .mutation(async ({ ctx, input }) => { - if (input.organizationId) { - await ensureOrganizationAccess(ctx, input.organizationId, ['owner', 'billing_manager']); - } - const owner = resolveOwner(ctx, input.organizationId); - return gitlabService.regenerateWebhookSecret(owner); - }), + // Personal/self-only GitLab webhook secret rotation. The secret is + // returned ONCE on success and never re-fetched from status — status + // no longer carries it (see P1-D-32). Org rotation goes through the + // dedicated billing-gated `organizations.reviewAgent.rotateGitLabWebhookSecret` + // mutation; this endpoint is the caller's own personal integration + // only, and re-syncs the Kilo-managed webhooks so the integration + // keeps working after the secret change. + regenerateWebhookSecret: baseProcedure.mutation(async ({ ctx }) => { + // Self-only: resolve the caller's own owner directly. The org + // surface uses `organizations.reviewAgent.rotateGitLabWebhookSecret` + // with `organizationBillingMutationProcedure` gating. + const owner = { type: 'user' as const, id: ctx.user.id }; + + // Generate the new secret here so we can re-sync the Kilo-managed + // webhooks against the SAME secret in a single operation. The + // underlying service-level regen would leave the live webhooks + // carrying the old secret, breaking the integration. + const newSecret = randomBytes(32).toString('hex'); + + const integration = await getIntegrationForOwner(owner, 'gitlab'); + if (!integration) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'GitLab integration not found', + }); + } + + const existingMetadata = (integration.metadata || {}) as Record; + const configuredWebhooks = + (existingMetadata.configured_webhooks as Record | undefined) ?? {}; + const instanceUrl = + (existingMetadata.gitlab_instance_url as string | undefined) || 'https://gitlab.com'; + + // No Kilo-managed webhooks → skip the network round-trip and just + // persist + return the new secret for manual reconfiguration. + if (Object.keys(configuredWebhooks).length === 0) { + await updateIntegrationMetadataForOwner(owner, 'gitlab', { + ...existingMetadata, + webhook_secret: newSecret, + }); + return { + webhookSecret: newSecret, + webhookSync: { + created: 0, + updated: 0, + deleted: 0, + errors: [] as Array<{ projectId: number; error: string; operation: string }>, + }, + configuredWebhookCount: 0, + }; + } + + let webhookSyncResult: { + created: number; + updated: number; + deleted: number; + errors: Array<{ projectId: number; error: string; operation: string }>; + } = { created: 0, updated: 0, deleted: 0, errors: [] }; + let updatedWebhooks: Record = configuredWebhooks; + + try { + const accessToken = await getValidGitLabToken(integration, { userId: ctx.user.id }); + const configuredRepoIds = Object.keys(configuredWebhooks) + .map(id => Number.parseInt(id, 10)) + .filter(id => Number.isFinite(id)); + + // previous=[] → every currently-configured repo is treated as + // "added" by the sync helper, so the existing Kilo webhook is + // UPDATED in place with the new secret (nothing is deleted). + const syncOutcome = await syncWebhooksForRepositories( + accessToken, + newSecret, + configuredRepoIds, + [], + configuredWebhooks, + instanceUrl + ); + updatedWebhooks = syncOutcome.updatedWebhooks; + webhookSyncResult = { + created: syncOutcome.result.created.length, + updated: syncOutcome.result.updated.length, + deleted: syncOutcome.result.deleted.length, + errors: syncOutcome.result.errors, + }; + logExceptInTest('[gitlab.regenerateWebhookSecret] Webhook re-sync completed', { + created: webhookSyncResult.created, + updated: webhookSyncResult.updated, + deleted: webhookSyncResult.deleted, + errorCount: webhookSyncResult.errors.length, + }); + } catch (webhookError) { + // Re-sync failure MUST NOT lose the new secret: persist it + // anyway so the operator can recover via manual reconfiguration. + logExceptInTest('[gitlab.regenerateWebhookSecret] Webhook re-sync failed', { + error: webhookError instanceof Error ? webhookError.message : String(webhookError), + }); + webhookSyncResult = { + created: 0, + updated: 0, + deleted: 0, + errors: [ + { + projectId: 0, + error: webhookError instanceof Error ? webhookError.message : 'Unknown error', + operation: 'create', + }, + ], + }; + } + + await updateIntegrationMetadataForOwner(owner, 'gitlab', { + ...existingMetadata, + webhook_secret: newSecret, + configured_webhooks: updatedWebhooks, + }); + + return { + webhookSecret: newSecret, + webhookSync: webhookSyncResult, + configuredWebhookCount: Object.keys(updatedWebhooks).length, + }; + }), }); diff --git a/apps/web/src/routers/organizations/organization-code-reviews-gitlab-rotate.test.ts b/apps/web/src/routers/organizations/organization-code-reviews-gitlab-rotate.test.ts new file mode 100644 index 0000000000..72d8583d9f --- /dev/null +++ b/apps/web/src/routers/organizations/organization-code-reviews-gitlab-rotate.test.ts @@ -0,0 +1,471 @@ +const mockSyncWebhooksForRepositories = jest.fn(); +const mockGetValidGitLabToken = jest.fn(); + +jest.mock('@/lib/integrations/platforms/gitlab/webhook-sync', () => ({ + syncWebhooksForRepositories: (...args: unknown[]) => mockSyncWebhooksForRepositories(...args), +})); + +jest.mock('@/lib/integrations/gitlab-service', () => ({ + getValidGitLabToken: (...args: unknown[]) => mockGetValidGitLabToken(...args), +})); + +// NOTE: `jest` is intentionally NOT imported from '@jest/globals' here. The +// @swc/jest transform only hoists `jest.mock(...)` above the static imports +// when `jest` is the global binding; importing it as a local binding disables +// that hoist, so the mocks below would register AFTER `createCallerForUser` +// pulls in the real gitlab-service. Using global `jest` keeps the mocks hoisted. +import { afterAll, beforeEach, describe, expect, it } from '@jest/globals'; +import { createCallerForUser } from '@/routers/test-utils'; +import { insertTestUser } from '@/tests/helpers/user.helper'; +import { createTestOrganization } from '@/tests/helpers/organization.helper'; +import { addUserToOrganization } from '@/lib/organizations/organizations'; +import { db } from '@/lib/drizzle'; +import { + kilocode_users, + organization_audit_logs, + organization_memberships, + organizations, + platform_integrations, +} from '@kilocode/db/schema'; +import { and, eq, inArray } from 'drizzle-orm'; + +const CREATED_ORG_IDS: string[] = []; +const SEED_USER_IDS: string[] = []; + +async function makeOrgAndOwner() { + const owner = await insertTestUser(); + SEED_USER_IDS.push(owner.id); + // require_seats=false grants the trial-bypass that + // organizationBillingMutationProcedure needs. + const organization = await createTestOrganization( + `GitLab rotate ${crypto.randomUUID()}`, + owner.id, + 0, + {}, + false + ); + CREATED_ORG_IDS.push(organization.id); + return { owner, organization }; +} + +async function seedGitLabIntegration( + organizationId: string, + metadata: Record +): Promise<{ id: string; secret: string | undefined }> { + const oldSecret = 'old-secret-do-not-leak'; + const [integration] = await db + .insert(platform_integrations) + .values({ + owned_by_organization_id: organizationId, + platform: 'gitlab', + integration_type: 'oauth', + integration_status: 'active', + platform_installation_id: `inst-${crypto.randomUUID()}`, + metadata: { ...metadata, webhook_secret: oldSecret }, + }) + .returning(); + return { id: integration!.id, secret: oldSecret }; +} + +async function readMetadata(organizationId: string) { + const row = await db.query.platform_integrations.findFirst({ + where: and( + eq(platform_integrations.owned_by_organization_id, organizationId), + eq(platform_integrations.platform, 'gitlab') + ), + }); + return (row?.metadata ?? {}) as Record; +} + +async function readWebhookSecret(organizationId: string): Promise { + const md = await readMetadata(organizationId); + return md.webhook_secret as string | undefined; +} + +async function settingsChangeAuditMessages(organizationId: string): Promise { + const rows = await db + .select({ message: organization_audit_logs.message }) + .from(organization_audit_logs) + .where( + and( + eq(organization_audit_logs.organization_id, organizationId), + eq(organization_audit_logs.action, 'organization.settings.change') + ) + ); + return rows.map(r => r.message); +} + +describe('P1-D-32 GitLab webhook secret (rotation + status)', () => { + afterAll(async () => { + for (const organizationId of CREATED_ORG_IDS) { + await db + .delete(organization_audit_logs) + .where(eq(organization_audit_logs.organization_id, organizationId)); + await db + .delete(platform_integrations) + .where(eq(platform_integrations.owned_by_organization_id, organizationId)); + await db + .delete(organization_memberships) + .where(eq(organization_memberships.organization_id, organizationId)); + await db.delete(organizations).where(eq(organizations.id, organizationId)); + } + if (SEED_USER_IDS.length > 0) { + await db.delete(kilocode_users).where(inArray(kilocode_users.id, SEED_USER_IDS)); + } + }); + + beforeEach(() => { + mockSyncWebhooksForRepositories.mockReset(); + mockGetValidGitLabToken.mockReset(); + // Default sync outcome: every currently-configured repo was "updated" + // (mirrors the `previous=[]` "treat all as added" path used by rotate). + mockSyncWebhooksForRepositories.mockImplementation( + async (_token, _secret, selectedIds, _previous, configuredWebhooks) => { + const updatedWebhooks: Record< + string, + { hook_id: number; created_at: string; updated_at?: string } + > = {}; + for (const id of selectedIds) { + const existing = configuredWebhooks[String(id)]; + updatedWebhooks[String(id)] = { + hook_id: existing?.hook_id ?? 1000 + Number(id), + created_at: existing?.created_at ?? new Date().toISOString(), + updated_at: new Date().toISOString(), + }; + } + return { + result: { + created: [], + updated: selectedIds.map((id: number) => ({ projectId: id, hookId: 1000 + id })), + deleted: [], + errors: [], + }, + updatedWebhooks, + }; + } + ); + mockGetValidGitLabToken.mockResolvedValue('gitlab-access-token'); + }); + + describe('organization review agent router: getGitLabStatus P1-D-32 (omits webhook secret)', () => { + it('returns the integration shape WITHOUT webhookSecret for a non-privileged member', async () => { + const { organization } = await makeOrgAndOwner(); + const member = await insertTestUser(); + SEED_USER_IDS.push(member.id); + await addUserToOrganization(organization.id, member.id, 'member'); + + await seedGitLabIntegration(organization.id, { + gitlab_instance_url: 'https://gitlab.example.com', + configured_webhooks: { '101': { hook_id: 9001, created_at: '2026-01-01T00:00:00Z' } }, + }); + + const caller = await createCallerForUser(member.id); + const status = await caller.organizations.reviewAgent.getGitLabStatus({ + organizationId: organization.id, + }); + + expect(status.connected).toBe(true); + expect(status.integration).toBeDefined(); + // Regression guard: the secret must NEVER appear in the status + // payload. If this assertion fails, the leak has been re-introduced. + expect(status.integration).not.toHaveProperty('webhookSecret'); + expect((status.integration as Record).webhookSecret).toBeUndefined(); + // The rest of the shape is preserved (the non-secret fields still ship; + // account/repositorySelection/installedAt are passed through verbatim from + // the stored integration row regardless of their concrete values). + expect(status.integration).toEqual( + expect.objectContaining({ + isValid: true, + instanceUrl: 'https://gitlab.example.com', + }) + ); + expect(status.integration).toHaveProperty('accountLogin'); + expect(status.integration).toHaveProperty('repositorySelection'); + expect(status.integration).toHaveProperty('installedAt'); + }); + + it('still omits webhookSecret when the caller is the owner', async () => { + const { owner, organization } = await makeOrgAndOwner(); + await seedGitLabIntegration(organization.id, { + gitlab_instance_url: 'https://gitlab.com', + configured_webhooks: {}, + }); + + const caller = await createCallerForUser(owner.id); + const status = await caller.organizations.reviewAgent.getGitLabStatus({ + organizationId: organization.id, + }); + + expect(status.connected).toBe(true); + expect(status.integration).not.toHaveProperty('webhookSecret'); + }); + }); + + describe('organization review agent router: rotateGitLabWebhookSecret P1-D-32', () => { + it('is denied for a plain org member (UNAUTHORIZED)', async () => { + const { organization } = await makeOrgAndOwner(); + const member = await insertTestUser(); + SEED_USER_IDS.push(member.id); + await addUserToOrganization(organization.id, member.id, 'member'); + await seedGitLabIntegration(organization.id, { configured_webhooks: {} }); + + const caller = await createCallerForUser(member.id); + await expect( + caller.organizations.reviewAgent.rotateGitLabWebhookSecret({ + organizationId: organization.id, + }) + ).rejects.toMatchObject({ code: 'UNAUTHORIZED' }); + }); + + it('is denied for a non-member (UNAUTHORIZED)', async () => { + const { organization } = await makeOrgAndOwner(); + // No membership row for this user at all — should be FORBIDDEN by + // the billing-mutation procedure before the handler runs. + const stranger = await insertTestUser(); + SEED_USER_IDS.push(stranger.id); + await seedGitLabIntegration(organization.id, { configured_webhooks: {} }); + + const caller = await createCallerForUser(stranger.id); + await expect( + caller.organizations.reviewAgent.rotateGitLabWebhookSecret({ + organizationId: organization.id, + }) + ).rejects.toMatchObject({ code: 'UNAUTHORIZED' }); + }); + + it('is allowed for the owner, persists a NEW secret, re-syncs webhooks, and returns the new secret once', async () => { + const { owner, organization } = await makeOrgAndOwner(); + const configured = { + '101': { hook_id: 9001, created_at: '2026-01-01T00:00:00Z' }, + '202': { hook_id: 9002, created_at: '2026-01-02T00:00:00Z' }, + }; + await seedGitLabIntegration(organization.id, { + gitlab_instance_url: 'https://gitlab.example.com', + configured_webhooks: configured, + }); + + const caller = await createCallerForUser(owner.id); + const result = await caller.organizations.reviewAgent.rotateGitLabWebhookSecret({ + organizationId: organization.id, + }); + + // Returned secret must be a non-empty hex string and distinct from + // the previously stored one. The new secret is returned ONCE here. + expect(typeof result.webhookSecret).toBe('string'); + expect(result.webhookSecret).toMatch(/^[0-9a-f]{64}$/); + expect(result.webhookSecret).not.toBe('old-secret-do-not-leak'); + + // Re-sync must be invoked exactly once, with the new secret, the + // configured repo ids (as numbers), previous=[] (so every currently + // configured repo is treated as "added" and UPDATED in place), the + // existing configured_webhooks map, and the stored instance URL. + expect(mockSyncWebhooksForRepositories).toHaveBeenCalledTimes(1); + expect(mockSyncWebhooksForRepositories).toHaveBeenCalledWith( + 'gitlab-access-token', + result.webhookSecret, + [101, 202], + [], + configured, + 'https://gitlab.example.com' + ); + expect(result.webhookSync.updated).toBe(2); + expect(result.webhookSync.created).toBe(0); + expect(result.webhookSync.deleted).toBe(0); + expect(result.webhookSync.errors).toEqual([]); + expect(result.configuredWebhookCount).toBe(2); + + // Persistence: metadata.webhook_secret is the NEW secret and + // metadata.configured_webhooks was updated with the sync output. + const storedSecret = await readWebhookSecret(organization.id); + expect(storedSecret).toBe(result.webhookSecret); + + const storedMetadata = await readMetadata(organization.id); + const storedConfigured = storedMetadata.configured_webhooks as Record< + string, + { hook_id: number; created_at: string; updated_at?: string } + >; + expect(Object.keys(storedConfigured).sort()).toEqual(['101', '202']); + expect(storedConfigured['101']?.updated_at).toBeDefined(); + expect(storedConfigured['202']?.updated_at).toBeDefined(); + + // The new secret is NEVER logged/returned elsewhere — the only + // exposure point is the one-shot return value above. Audit log + // records the rotation event WITHOUT the secret. + const auditMessages = await settingsChangeAuditMessages(organization.id); + const rotateMessages = auditMessages.filter(m => + m.startsWith('Rotated GitLab webhook secret') + ); + expect(rotateMessages).toHaveLength(1); + expect(rotateMessages[0]).not.toContain(result.webhookSecret); + expect(rotateMessages[0]).toContain('2 updated'); + }); + + it('is allowed for a billing_manager, with the same re-sync behavior', async () => { + const { organization } = await makeOrgAndOwner(); + const billingManager = await insertTestUser(); + SEED_USER_IDS.push(billingManager.id); + await addUserToOrganization(organization.id, billingManager.id, 'billing_manager'); + const configured = { + '303': { hook_id: 9030, created_at: '2026-02-01T00:00:00Z' }, + }; + await seedGitLabIntegration(organization.id, { + gitlab_instance_url: 'https://gitlab.com', + configured_webhooks: configured, + }); + + const caller = await createCallerForUser(billingManager.id); + const result = await caller.organizations.reviewAgent.rotateGitLabWebhookSecret({ + organizationId: organization.id, + }); + + expect(result.webhookSecret).toMatch(/^[0-9a-f]{64}$/); + expect(mockSyncWebhooksForRepositories).toHaveBeenCalledWith( + 'gitlab-access-token', + result.webhookSecret, + [303], + [], + configured, + 'https://gitlab.com' + ); + expect(await readWebhookSecret(organization.id)).toBe(result.webhookSecret); + }); + + it('with empty configured_webhooks returns the new secret and does NOT call sync', async () => { + const { owner, organization } = await makeOrgAndOwner(); + await seedGitLabIntegration(organization.id, { + gitlab_instance_url: 'https://gitlab.example.com', + configured_webhooks: {}, + }); + + const caller = await createCallerForUser(owner.id); + const result = await caller.organizations.reviewAgent.rotateGitLabWebhookSecret({ + organizationId: organization.id, + }); + + expect(result.webhookSecret).toMatch(/^[0-9a-f]{64}$/); + expect(result.webhookSync).toEqual({ + created: 0, + updated: 0, + deleted: 0, + errors: [], + }); + expect(result.configuredWebhookCount).toBe(0); + expect(mockSyncWebhooksForRepositories).not.toHaveBeenCalled(); + // No token lookup needed when there are no webhooks to re-sync. + expect(mockGetValidGitLabToken).not.toHaveBeenCalled(); + // The new secret was still persisted for manual reconfiguration. + expect(await readWebhookSecret(organization.id)).toBe(result.webhookSecret); + + // Audit log mentions the no-rotation branch so operators can tell + // manual-only rotations apart from synced rotations. + const auditMessages = await settingsChangeAuditMessages(organization.id); + expect(auditMessages).toEqual([ + 'Rotated GitLab webhook secret (no Kilo-managed webhooks to re-sync)', + ]); + }); + + it('only touches THIS integration — does not rotate another orgs secret', async () => { + const { owner: ownerA, organization: orgA } = await makeOrgAndOwner(); + const { organization: orgB } = await makeOrgAndOwner(); + + const configuredA = { '1': { hook_id: 1, created_at: '2026-01-01T00:00:00Z' } }; + const configuredB = { '2': { hook_id: 2, created_at: '2026-01-01T00:00:00Z' } }; + await seedGitLabIntegration(orgA.id, { + gitlab_instance_url: 'https://gitlab.com', + configured_webhooks: configuredA, + }); + await seedGitLabIntegration(orgB.id, { + gitlab_instance_url: 'https://gitlab.com', + configured_webhooks: configuredB, + }); + + const oldSecretB = await readWebhookSecret(orgB.id); + expect(oldSecretB).toBe('old-secret-do-not-leak'); + + const callerA = await createCallerForUser(ownerA.id); + const result = await callerA.organizations.reviewAgent.rotateGitLabWebhookSecret({ + organizationId: orgA.id, + }); + + // Org A was rotated; org B is untouched. + expect(await readWebhookSecret(orgA.id)).toBe(result.webhookSecret); + expect(await readWebhookSecret(orgB.id)).toBe('old-secret-do-not-leak'); + + // Sync was called only once, for org A's configured repo id. + expect(mockSyncWebhooksForRepositories).toHaveBeenCalledTimes(1); + expect(mockSyncWebhooksForRepositories).toHaveBeenCalledWith( + 'gitlab-access-token', + result.webhookSecret, + [1], + [], + configuredA, + 'https://gitlab.com' + ); + }); + + it('persists the NEW secret even when the webhook re-sync throws (no lost-secret state)', async () => { + const { owner, organization } = await makeOrgAndOwner(); + const configured = { + '404': { hook_id: 9040, created_at: '2026-03-01T00:00:00Z' }, + }; + await seedGitLabIntegration(organization.id, { + gitlab_instance_url: 'https://gitlab.example.com', + configured_webhooks: configured, + }); + // The GitLab re-sync call fails outright (e.g. a 5xx from GitLab). + const syncError = new Error('gitlab responded 500'); + mockSyncWebhooksForRepositories.mockRejectedValueOnce(syncError); + + const caller = await createCallerForUser(owner.id); + // The mutation MUST NOT throw — losing the just-rotated secret while + // GitLab may already carry it would strand the integration. + const result = await caller.organizations.reviewAgent.rotateGitLabWebhookSecret({ + organizationId: organization.id, + }); + + expect(result.webhookSecret).toMatch(/^[0-9a-f]{64}$/); + expect(result.webhookSecret).not.toBe('old-secret-do-not-leak'); + // The failure is surfaced as a sync error, not swallowed silently. + expect(result.webhookSync.errors).toHaveLength(1); + expect(result.webhookSync.updated).toBe(0); + + // The new secret is persisted regardless, so the operator can recover + // via manual reconfiguration and a retry does not desync further. + expect(await readWebhookSecret(organization.id)).toBe(result.webhookSecret); + + // Neither the surfaced error payload nor the audit log leaks the secret. + expect(JSON.stringify(result.webhookSync.errors)).not.toContain(result.webhookSecret); + const auditMessages = await settingsChangeAuditMessages(organization.id); + const rotateMessages = auditMessages.filter(m => + m.startsWith('Rotated GitLab webhook secret') + ); + expect(rotateMessages).toHaveLength(1); + expect(rotateMessages[0]).not.toContain(result.webhookSecret); + }); + + it('persists the NEW secret even when the access-token lookup throws', async () => { + const { owner, organization } = await makeOrgAndOwner(); + const configured = { + '505': { hook_id: 9050, created_at: '2026-03-02T00:00:00Z' }, + }; + await seedGitLabIntegration(organization.id, { + gitlab_instance_url: 'https://gitlab.example.com', + configured_webhooks: configured, + }); + // Token resolution fails before the sync can run. + mockGetValidGitLabToken.mockRejectedValueOnce(new Error('token expired')); + + const caller = await createCallerForUser(owner.id); + const result = await caller.organizations.reviewAgent.rotateGitLabWebhookSecret({ + organizationId: organization.id, + }); + + expect(result.webhookSecret).toMatch(/^[0-9a-f]{64}$/); + expect(result.webhookSync.errors).toHaveLength(1); + // Sync is never reached when the token lookup fails. + expect(mockSyncWebhooksForRepositories).not.toHaveBeenCalled(); + // Secret still persisted for recovery. + expect(await readWebhookSecret(organization.id)).toBe(result.webhookSecret); + }); + }); +}); diff --git a/apps/web/src/routers/organizations/organization-code-reviews-router.test.ts b/apps/web/src/routers/organizations/organization-code-reviews-router.test.ts index ae911c2afd..f1e650f280 100644 --- a/apps/web/src/routers/organizations/organization-code-reviews-router.test.ts +++ b/apps/web/src/routers/organizations/organization-code-reviews-router.test.ts @@ -6,7 +6,6 @@ import { getAgentConfig } from '@/lib/agent-config/db/agent-configs'; import { db } from '@/lib/drizzle'; import { agent_configs, organization_audit_logs, organizations } from '@kilocode/db/schema'; import { and, eq } from 'drizzle-orm'; - const createdOrganizationIds: string[] = []; async function createFixtureOrganization() { @@ -200,6 +199,261 @@ describe('organization review agent router: council config', () => { }); }); +describe('organization review agent router: patchReviewConfig', () => { + const activeCouncil = { + enabled: true as const, + aggregation_strategy: 'unanimous' as const, + specialists: [ + { + id: 'security', + role: 'security' as const, + name: 'Security', + enabled: true, + required: false, + lens: 'x', + }, + { + id: 'performance', + role: 'performance' as const, + name: 'Performance', + enabled: true, + required: false, + lens: 'y', + }, + ], + }; + + // A fully-populated org config that the field-merge PATCH must NOT + // touch when the patch omits those fields: council, councilEnabled, + // manuallyAddedRepositories, repositoryModelOverrides, plus the + // review_memory_enabled / review_analytics_enabled feature flags. + async function seedOrgGithubConfig( + organization: { id: string }, + owner: { id: string } + ): Promise { + await db.insert(agent_configs).values({ + owned_by_organization_id: organization.id, + agent_type: 'code_review', + platform: 'github', + config: { + review_style: 'balanced', + focus_areas: ['bugs'], + custom_instructions: 'be terse', + model_slug: 'anthropic/claude-sonnet-5', + thinking_effort: null, + gate_threshold: 'off', + repository_selection_mode: 'all', + selected_repository_ids: [101, 202], + manually_added_repositories: [ + { id: 9, name: 'manual', full_name: 'manual/repo', private: true }, + ], + repository_model_overrides: [ + { + repository_id: 101, + repo_full_name: 'acme/api', + model_slug: 'openai/gpt-5', + thinking_effort: 'high', + }, + ], + council: activeCouncil, + council_enabled_repository_ids: [101, 202], + disable_review_md: true, + // Web-only setting the patch schema never carries; `false` proves + // the patch passes it through instead of resetting to the default. + skip_bot_pull_requests: false, + review_memory_enabled: true, + review_analytics_enabled: true, + }, + is_enabled: false, + created_by: owner.id, + }); + } + + it('returns NOT_FOUND when no stored org config exists', async () => { + const { owner, organization } = await createFixtureOrganization(); + const caller = await createCallerForUser(owner.id); + + await expect( + caller.organizations.reviewAgent.patchReviewConfig({ + organizationId: organization.id, + platform: 'github', + reviewStyle: 'strict', + }) + ).rejects.toMatchObject({ code: 'NOT_FOUND' }); + + const stored = await getAgentConfig(organization.id, 'code_review', 'github'); + expect(stored).toBeNull(); + }); + + it('preserves council, councilEnabled, manuallyAddedRepositories, and overrides when the mobile-shaped patch omits them', async () => { + const { owner, organization } = await createFixtureOrganization(); + await seedOrgGithubConfig(organization, owner); + const caller = await createCallerForUser(owner.id); + + await caller.organizations.reviewAgent.patchReviewConfig({ + organizationId: organization.id, + platform: 'github', + // Only the mobile-shaped fields. Council / councilEnabled / + // manuallyAddedRepositories / repositoryModelOverrides are absent + // and must round-trip through the patch unchanged. + reviewStyle: 'strict', + focusAreas: ['security'], + modelSlug: 'openai/gpt-5', + }); + + const stored = await getAgentConfig(organization.id, 'code_review', 'github'); + expect(stored?.config).toEqual( + expect.objectContaining({ + review_style: 'strict', + focus_areas: ['security'], + model_slug: 'openai/gpt-5', + council: expect.objectContaining({ enabled: true, aggregation_strategy: 'unanimous' }), + council_enabled_repository_ids: [101, 202], + manually_added_repositories: [ + { id: 9, name: 'manual', full_name: 'manual/repo', private: true }, + ], + repository_model_overrides: [ + { + repository_id: 101, + repo_full_name: 'acme/api', + model_slug: 'openai/gpt-5', + thinking_effort: 'high', + }, + ], + // Feature flags preserved by `preserveCodeReviewFeatureSettings`: + review_memory_enabled: true, + review_analytics_enabled: true, + }) + ); + }); + + it('preserves council when the patch edits an unrelated field (entitlement gate NOT triggered)', async () => { + // The fixture org is entitled, but the assertion here is behavioral: + // an unrelated field edit (reviewStyle) must NOT re-evaluate the + // entitlement gate. We verify by checking the gate would have failed + // had it run against a NON-entitled org: if the helper decides to + // re-trigger the gate on every patch, the assertion would either + // silently pass (entitled) or, in a non-entitled setup, throw — so + // this test also documents the omit-council preservation contract. + const { owner, organization } = await createFixtureOrganization(); + await seedOrgGithubConfig(organization, owner); + const caller = await createCallerForUser(owner.id); + + await caller.organizations.reviewAgent.patchReviewConfig({ + organizationId: organization.id, + platform: 'github', + reviewStyle: 'lenient', + }); + + const stored = await getAgentConfig(organization.id, 'code_review', 'github'); + expect(stored?.config).toEqual( + expect.objectContaining({ + review_style: 'lenient', + // Council preserved unchanged. + council: expect.objectContaining({ + enabled: true, + aggregation_strategy: 'unanimous', + specialists: expect.arrayContaining([ + expect.objectContaining({ id: 'security' }), + expect.objectContaining({ id: 'performance' }), + ]), + }), + council_enabled_repository_ids: [101, 202], + }) + ); + }); + + it('writes a PATCH audit log identifying the patch action', async () => { + const { owner, organization } = await createFixtureOrganization(); + await seedOrgGithubConfig(organization, owner); + const caller = await createCallerForUser(owner.id); + + await caller.organizations.reviewAgent.patchReviewConfig({ + organizationId: organization.id, + platform: 'github', + reviewStyle: 'roast', + }); + + const logs = await settingsChangeAuditLogs(organization.id); + expect(logs).toHaveLength(1); + expect(logs[0]?.message).toMatch(/^Patched Review Agent configuration for github/); + expect(logs[0]?.message).toContain('roast'); + }); + + // P0-B-13b: real mobile→server contract guard. The mobile + // useSaveReviewConfig hook now sends a partial org patch whose shape + // is exactly { organizationId, platform, ...editedFields } — no + // manuallyAddedRepositories, no council, no councilEnabledRepositoryIds, + // no autoConfigureWebhooks. The server PATCH must field-merge those + // absent keys from the stored config so mobile edits do not clobber + // org-only state the mobile UI does not surface. + it('preserves a config seeded with council + manuallyAddedRepositories + councilEnabledRepositoryIds when a mobile-shaped patch is applied', async () => { + const { owner, organization } = await createFixtureOrganization(); + await seedOrgGithubConfig(organization, owner); + const caller = await createCallerForUser(owner.id); + + // Mobile-shaped patch: ONLY the keys the mobile UI lets the user + // edit. council, councilEnabledRepositoryIds, manuallyAddedRepositories, + // and repositoryModelOverrides are ALL absent — the server must + // preserve them. + await caller.organizations.reviewAgent.patchReviewConfig({ + organizationId: organization.id, + platform: 'github', + reviewStyle: 'strict', + focusAreas: ['security'], + }); + + const stored = await getAgentConfig(organization.id, 'code_review', 'github'); + + // Patched fields applied. + expect(stored?.config).toEqual( + expect.objectContaining({ + review_style: 'strict', + focus_areas: ['security'], + }) + ); + // Stored fields NOT in the mobile-shaped patch must round-trip + // unchanged. The org schema accepts council / + // councilEnabledRepositoryIds / manuallyAddedRepositories, but mobile + // never sends them — the field-merge must keep them as-is. + expect(stored?.config).toEqual( + expect.objectContaining({ + council: expect.objectContaining({ + enabled: true, + aggregation_strategy: 'unanimous', + specialists: expect.arrayContaining([ + expect.objectContaining({ id: 'security' }), + expect.objectContaining({ id: 'performance' }), + ]), + }), + council_enabled_repository_ids: [101, 202], + manually_added_repositories: [ + { id: 9, name: 'manual', full_name: 'manual/repo', private: true }, + ], + repository_model_overrides: [ + { + repository_id: 101, + repo_full_name: 'acme/api', + model_slug: 'openai/gpt-5', + thinking_effort: 'high', + }, + ], + }) + ); + // Other stored fields that the mobile client never read or sent must + // also be preserved. + expect(stored?.config).toEqual( + expect.objectContaining({ + selected_repository_ids: [101, 202], + repository_selection_mode: 'all', + gate_threshold: 'off', + disable_review_md: true, + skip_bot_pull_requests: false, + }) + ); + }); +}); + describe('organization review agent router: skip bot pull requests', () => { afterAll(async () => { for (const organizationId of createdOrganizationIds) { diff --git a/apps/web/src/routers/organizations/organization-code-reviews-router.ts b/apps/web/src/routers/organizations/organization-code-reviews-router.ts index e76ddd5c84..1bad7ea799 100644 --- a/apps/web/src/routers/organizations/organization-code-reviews-router.ts +++ b/apps/web/src/routers/organizations/organization-code-reviews-router.ts @@ -29,7 +29,10 @@ import { fetchGitLabRepositoriesForOrganization } from '@/lib/cloud-agent/gitlab import { PRIMARY_DEFAULT_MODEL } from '@/lib/ai-gateway/models'; import { createDefaultCodeReviewConfig } from '@/lib/code-reviews/core/default-config'; import { isCouncilEntitledForOrganization } from '@/lib/code-reviews/core/council-entitlement'; -import { CodeReviewCouncilConfigSchema } from '@kilocode/db/schema-types'; +import { + CodeReviewCouncilConfigSchema, + type CodeReviewCouncilConfig, +} from '@kilocode/db/schema-types'; import { isCouncilActive } from '@kilocode/worker-utils/code-review-council'; import { PLATFORM } from '@/lib/integrations/core/constants'; import { isPlatformIntegrationHealthy } from '@/lib/integrations/core/health'; @@ -44,6 +47,7 @@ import { getCodeReviewActionRequiredState, } from '@/lib/code-reviews/action-required'; import { getReviewMemoryEnabledFromConfig } from '@/lib/code-reviews/review-memory/settings'; +import { randomBytes } from 'node:crypto'; import { createManualCodeReviewJob, ManualCodeReviewJobInputSchema, @@ -59,6 +63,11 @@ import { ManualBitbucketCodeReviewTriggerError, triggerManualBitbucketCodeReview, } from '@/lib/integrations/platforms/bitbucket/manual-code-review-trigger'; +import { + applyCodeReviewConfigPatch, + type CodeReviewFieldMergePatch, + type CodeReviewStoredConfig, +} from '@kilocode/app-shared/code-review'; const PlatformSchema = z.enum(['github', 'gitlab', 'bitbucket']).default('github'); @@ -145,6 +154,40 @@ const SaveReviewConfigInputSchema = OrganizationIdInputSchema.extend({ autoConfigureWebhooks: z.boolean().optional().default(true), }); +// Field-merge PATCH schema for organization review agent configurations. +// Strict subset of SaveReviewConfigInputSchema: every field is optional +// (omission = preserve stored value), no defaults, no Bitbucket-cache +// validation. Reuses the same per-field bounds as the full save so a +// partial update can't smuggle in values the full save would reject. +const PatchReviewConfigInputSchema = OrganizationIdInputSchema.extend({ + platform: PlatformSchema, + reviewStyle: z.enum(['strict', 'balanced', 'lenient', 'roast']).optional(), + focusAreas: z.array(z.string()).optional(), + customInstructions: z.string().optional(), + modelSlug: z.string().optional(), + thinkingEffort: z + .string() + .max(50) + .regex(/^[a-zA-Z]+$/) + .nullable() + .optional(), + repositorySelectionMode: z.enum(['all', 'selected']).optional(), + selectedRepositoryIds: z.array(z.union([z.number(), z.string()])).optional(), + manuallyAddedRepositories: z.array(ManuallyAddedRepositoryInputSchema).optional(), + repositoryModelOverrides: z + .array(RepositoryModelOverrideInputSchema) + .max(MAX_REPOSITORY_MODEL_OVERRIDES) + .superRefine(rejectDuplicateRepositoryModelOverrides) + .optional(), + disableReviewMd: z.boolean().optional(), + gateThreshold: z.enum(['off', 'all', 'warning', 'critical']).optional(), + council: CodeReviewCouncilConfigSchema.nullable().optional(), + councilEnabledRepositoryIds: z.array(z.union([z.number(), z.string()])).optional(), + // GitLab-specific: only consulted when `selectedRepositoryIds` is also + // present in the patch. + autoConfigureWebhooks: z.boolean().optional(), +}); + const CreateManualReviewJobInputSchema = OrganizationIdInputSchema.extend( ManualCodeReviewJobInputSchema.shape ); @@ -440,9 +483,12 @@ export const organizationReviewAgentRouter = createTRPCRouter({ }; } - // Extract webhook secret from metadata for display + // NOTE: The webhook secret is intentionally NOT returned here. The + // previous shape leaked it to every org member. The secret is now + // surfaced only via the billing-gated `rotateGitLabWebhookSecret` + // mutation (returned once, on demand) and the manual webhook setup + // instructions in the UI. See P1-D-32. const metadata = integration.metadata as Record | null; - const webhookSecret = metadata?.webhook_secret as string | undefined; return { connected: true, @@ -451,7 +497,6 @@ export const organizationReviewAgentRouter = createTRPCRouter({ repositorySelection: integration.repository_access, installedAt: integration.installed_at, isValid: true, - webhookSecret, // Include webhook secret for user to configure in GitLab instanceUrl: (metadata?.gitlab_instance_url as string) || 'https://gitlab.com', }, }; @@ -752,6 +797,307 @@ export const organizationReviewAgentRouter = createTRPCRouter({ } }), + /** + * Field-merge PATCH for the organization's review agent configuration. + * + * Unlike `saveReviewConfig` (full-document overwrite), this procedure READS + * the current stored config and applies ONLY the fields present in the + * patch — every unlisted field is preserved verbatim. Designed for the + * mobile app, which edits one or two settings at a time and must not + * clobber `council`, `councilEnabledRepositoryIds`, `manuallyAddedRepositories`, + * `repositoryModelOverrides`, or feature flags the mobile UI doesn't surface. + * + * Platform forcing from the full save is re-applied post-merge: + * - GitLab forces `repository_selection_mode = 'selected'` + * - Bitbucket forces 'selected' + `gate_threshold = 'off'` + + * `disable_review_md = true` + `manually_added_repositories = []` + * The PATCH intentionally does NOT re-validate Bitbucket selections + * against the workspace cache (that's a save-level concern handled by + * the full save) and does NOT ensure the Bitbucket workspace webhook + * (also save-level). Callers that change `selectedRepositoryIds` for + * Bitbucket via PATCH are expected to have already saved a valid + * configuration. + * + * GitLab webhook sync runs ONLY when `selectedRepositoryIds` is present + * in the patch, so an unrelated edit (e.g. `focusAreas` only) never + * touches integration metadata. + * + * The council entitlement gate (`isCouncilActive + isCouncilEntitledForOrganization`) + * fires ONLY when the patch actually carries a `council` key. An omitted + * council must not re-trigger the gate, matching the full-save + * `council ?? undefined` behavior — a non-entitled org can keep its + * existing council config untouched. + * + * Throws `NOT_FOUND` if no stored config exists — a PATCH cannot + * bootstrap a row (use `saveReviewConfig` for that). + */ + patchReviewConfig: organizationBillingMutationProcedure + .input(PatchReviewConfigInputSchema) + .mutation(async ({ input, ctx }) => { + try { + const platform = input.platform; + const isBitbucket = platform === PLATFORM.BITBUCKET; + const isGitLab = platform === PLATFORM.GITLAB; + + const previousConfig = await getAgentConfig(input.organizationId, 'code_review', platform); + if (!previousConfig) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: + 'No existing review agent configuration to patch. Save one with saveReviewConfig first.', + }); + } + const previousRepoIds = + ((previousConfig.config as CodeReviewAgentConfig | undefined)?.selected_repository_ids as + | Array + | undefined) || []; + + const prevCfg = previousConfig.config as CodeReviewAgentConfig; + const stored: CodeReviewStoredConfig = { + reviewStyle: prevCfg.review_style || 'balanced', + focusAreas: prevCfg.focus_areas || [], + customInstructions: prevCfg.custom_instructions ?? null, + modelSlug: prevCfg.model_slug || PRIMARY_DEFAULT_MODEL, + thinkingEffort: prevCfg.thinking_effort ?? null, + gateThreshold: isBitbucket ? 'off' : (prevCfg.gate_threshold ?? 'off'), + repositorySelectionMode: isBitbucket + ? 'selected' + : prevCfg.repository_selection_mode || 'all', + selectedRepositoryIds: prevCfg.selected_repository_ids ?? [], + repositoryModelOverrides: (prevCfg.repository_model_overrides ?? []).map(o => ({ + repositoryId: o.repository_id, + repoFullName: o.repo_full_name, + modelSlug: o.model_slug, + thinkingEffort: o.thinking_effort ?? null, + })), + disableReviewMd: isBitbucket ? true : (prevCfg.disable_review_md ?? true), + manuallyAddedRepositories: isBitbucket ? [] : prevCfg.manually_added_repositories || [], + // Council is org-only and Bitbucket never carries one; preserve as-is + // otherwise. The patch input accepts `null` to clear and an object + // to replace — both are handled by the merge helper. + council: isBitbucket + ? null + : ((prevCfg.council ?? null) as CodeReviewCouncilConfig | null), + councilEnabledRepositoryIds: isBitbucket + ? [] + : (prevCfg.council_enabled_repository_ids ?? []), + }; + + // Field-merge: every key absent from `input` is preserved from + // `stored`. `null` is an explicit "clear" (e.g. `council: null`). + const { organizationId: _orgId, platform: _platform, ...rest } = input; + const patch: CodeReviewFieldMergePatch = rest; + const merged = applyCodeReviewConfigPatch(stored, patch); + + // Council entitlement gate: ONLY when the patch actually carries a + // `council` key. An omitted council must not re-trigger the gate — + // a non-entitled org keeps its existing (un)set council untouched. + // `Object.prototype.hasOwnProperty` distinguishes a real patch key + // (including `null`) from a missing one, which `Object.keys` already + // filters but we re-check explicitly for the gate decision. + if ( + Object.prototype.hasOwnProperty.call(patch, 'council') && + patch.council && + isCouncilActive(patch.council as CodeReviewCouncilConfig | null) + ) { + const entitled = await isCouncilEntitledForOrganization(input.organizationId); + if (!entitled) { + throw new TRPCError({ + code: 'FORBIDDEN', + message: 'Council review requires an enterprise plan with an active subscription.', + }); + } + } + + // Re-apply platform forcing post-merge. Mirrors the full save + // exactly so a save→patch round-trip preserves the same invariant: + // - GitLab: 'selected' only + // - Bitbucket: 'selected' + gateThreshold='off' + + // disableReviewMd=true + manuallyAddedRepositories=[] + // repositoryModelOverrides are pruned to the platform's id type, same + // as the full save. + const repositorySelectionMode: 'all' | 'selected' = + isBitbucket || isGitLab ? 'selected' : (merged.repositorySelectionMode ?? 'all'); + const gateThreshold: 'off' | 'all' | 'warning' | 'critical' = isBitbucket + ? 'off' + : (merged.gateThreshold ?? 'off'); + const disableReviewMd = isBitbucket ? true : (merged.disableReviewMd ?? true); + const manuallyAddedRepositories: typeof merged.manuallyAddedRepositories = isBitbucket + ? [] + : (merged.manuallyAddedRepositories ?? []); + // `merged.council` is statically typed as the loose shared-helper + // shape (`CodeReviewCouncilConfigInput | null | undefined`). At + // runtime it is always the canonical `CodeReviewCouncilConfig` — + // the stored config is validated by `CodeReviewAgentConfigSchema` + // on read, and the patch input is validated by + // `CodeReviewCouncilConfigSchema` at the route boundary, so the + // merge helper can only ever produce the strict shape. Cast to + // satisfy `upsertAgentConfig`'s strict config type. + const council = isBitbucket + ? undefined + : Object.prototype.hasOwnProperty.call(patch, 'council') + ? ((patch.council as CodeReviewCouncilConfig | null) ?? undefined) + : ((merged.council as CodeReviewCouncilConfig | null | undefined) ?? undefined); + const councilEnabledRepositoryIds: Array = isBitbucket + ? [] + : (merged.councilEnabledRepositoryIds ?? []); + + const repositoryModelOverrides: RepositoryModelOverride[] = ( + merged.repositoryModelOverrides ?? [] + ) + .filter(override => + isBitbucket + ? typeof override.repositoryId === 'string' + : typeof override.repositoryId === 'number' + ) + .map(override => ({ + repository_id: override.repositoryId, + repo_full_name: override.repoFullName, + model_slug: override.modelSlug, + thinking_effort: override.thinkingEffort ?? null, + })); + + await upsertAgentConfig({ + organizationId: input.organizationId, + agentType: 'code_review', + platform, + config: { + review_style: merged.reviewStyle ?? 'balanced', + focus_areas: merged.focusAreas ?? [], + custom_instructions: merged.customInstructions ?? null, + model_slug: merged.modelSlug ?? PRIMARY_DEFAULT_MODEL, + thinking_effort: merged.thinkingEffort ?? null, + gate_threshold: gateThreshold, + repository_selection_mode: repositorySelectionMode, + selected_repository_ids: (merged.selectedRepositoryIds ?? []) as Array, + manually_added_repositories: manuallyAddedRepositories, + repository_model_overrides: repositoryModelOverrides, + council, + council_enabled_repository_ids: councilEnabledRepositoryIds, + disable_review_md: disableReviewMd, + // The patch schema never carries `skipBotPullRequests` (a + // web-only setting from the full save). Pass the stored value + // through so a mobile PATCH can't silently reset it to the + // default — same field-merge contract as every other omitted + // field. + skip_bot_pull_requests: prevCfg.skip_bot_pull_requests ?? true, + review_memory_enabled: false, + review_analytics_enabled: false, + }, + preserveCodeReviewFeatureSettings: !isBitbucket, + createdBy: ctx.user.id, + }); + + // GitLab webhook sync runs ONLY when the patch actually carries + // `selectedRepositoryIds`. A patch that doesn't touch selection + // (e.g. mobile updating `focusAreas`) must not mutate integration + // metadata. Auto-configure is honored when present, defaulting to + // true to match the full-save default. + let webhookSyncResult = null; + if ( + isGitLab && + input.selectedRepositoryIds !== undefined && + (input.autoConfigureWebhooks ?? true) && + repositorySelectionMode === 'selected' + ) { + const integration = await getIntegrationForOrganization( + input.organizationId, + PLATFORM.GITLAB + ); + if (integration) { + const metadata = integration.metadata as Record | null; + const webhookSecret = metadata?.webhook_secret as string | undefined; + const instanceUrl = + (metadata?.gitlab_instance_url as string | undefined) || 'https://gitlab.com'; + const configuredWebhooks = + (metadata?.configured_webhooks as Record) || {}; + + if (webhookSecret) { + try { + const accessToken = await getValidGitLabToken(integration, { + userId: ctx.user.id, + organizationId: input.organizationId, + }); + + const selectedRepositoryIds = (input.selectedRepositoryIds ?? []).filter( + (repositoryId): repositoryId is number => typeof repositoryId === 'number' + ); + const previousSelectedRepositoryIds = previousRepoIds.filter( + (repositoryId): repositoryId is number => typeof repositoryId === 'number' + ); + const { result, updatedWebhooks } = await syncWebhooksForRepositories( + accessToken, + webhookSecret, + selectedRepositoryIds, + previousSelectedRepositoryIds, + configuredWebhooks, + instanceUrl + ); + + const existingMetadata = (integration.metadata as Record) || {}; + await updateIntegrationMetadata(integration.id, { + ...existingMetadata, + configured_webhooks: updatedWebhooks, + }); + + webhookSyncResult = { + created: result.created.length, + updated: result.updated.length, + deleted: result.deleted.length, + errors: result.errors, + }; + + logExceptInTest( + '[patchReviewConfig] Webhook sync completed for organization', + webhookSyncResult + ); + } catch (webhookError) { + logExceptInTest('[patchReviewConfig] Webhook sync failed for organization', { + error: + webhookError instanceof Error ? webhookError.message : String(webhookError), + }); + webhookSyncResult = { + created: 0, + updated: 0, + deleted: 0, + errors: [ + { + projectId: 0, + error: webhookError instanceof Error ? webhookError.message : 'Unknown error', + operation: 'sync' as const, + }, + ], + }; + } + } + } + } + + // Audit log identifies this as a PATCH action so reviewers can tell + // it apart from the full-save audit message. + await createAuditLog({ + organization_id: input.organizationId, + action: 'organization.settings.change', + actor_id: ctx.user.id, + actor_email: ctx.user.google_user_email, + actor_name: ctx.user.google_user_name, + message: `Patched Review Agent configuration for ${platform} (style: ${merged.reviewStyle ?? 'unknown'})${webhookSyncResult ? `, webhooks: ${webhookSyncResult.created} created, ${webhookSyncResult.deleted} deleted` : ''}`, + }); + + return { + success: true, + webhookSync: webhookSyncResult, + }; + } catch (error) { + if (error instanceof TRPCError) throw error; + console.error('Error patching review config:', error); + throw new TRPCError({ + code: 'INTERNAL_SERVER_ERROR', + message: 'Failed to patch review configuration', + }); + } + }), + /** * Toggles the review agent on/off */ @@ -900,4 +1246,156 @@ export const organizationReviewAgentRouter = createTRPCRouter({ }); } }), + + /** + * Rotate the GitLab webhook secret for the org integration and re-sync + * the Kilo-managed webhooks so they keep validating with the new secret. + * + * Admin-gated (owner or billing_manager) — the previous shape leaked the + * secret to every org member via `getGitLabStatus`. The new secret is + * returned ONCE, only to the caller, for manual reconfiguration. When + * Kilo-managed webhooks exist they are all UPDATED in place (the + * `previous = []` trick makes every currently-configured repo an + * "added" repo, which the sync helper handles as an update of the + * existing webhook). When no webhooks are configured (manual-only + * setup) the sync is skipped and the secret is still returned once. + * + * Scope is per-org: only this integration's metadata is touched. See + * P1-D-32. + */ + rotateGitLabWebhookSecret: organizationBillingMutationProcedure + .input(OrganizationIdInputSchema) + .mutation(async ({ input, ctx }) => { + const organizationId = input.organizationId; + const integration = await getIntegrationForOrganization(organizationId, PLATFORM.GITLAB); + + if (!integration) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'GitLab integration not found for this organization', + }); + } + + const existingMetadata = (integration.metadata || {}) as Record; + const configuredWebhooks = + (existingMetadata.configured_webhooks as Record | undefined) ?? + {}; + const instanceUrl = + (existingMetadata.gitlab_instance_url as string | undefined) || 'https://gitlab.com'; + + // Persist a brand-new secret. We generate it here (rather than via + // gitlab-service.regenerateWebhookSecret) so the re-sync and the + // metadata write happen against a single secret in a single + // operation. Never log the secret or the full metadata. + const newSecret = randomBytes(32).toString('hex'); + + // If no Kilo-managed webhooks are configured, skip the network + // round-trip entirely and just persist + return the new secret. + if (Object.keys(configuredWebhooks).length === 0) { + await updateIntegrationMetadata(integration.id, { + ...existingMetadata, + webhook_secret: newSecret, + }); + await createAuditLog({ + organization_id: organizationId, + action: 'organization.settings.change', + actor_id: ctx.user.id, + actor_email: ctx.user.google_user_email, + actor_name: ctx.user.google_user_name, + message: 'Rotated GitLab webhook secret (no Kilo-managed webhooks to re-sync)', + }); + return { + webhookSecret: newSecret, + webhookSync: { + created: 0, + updated: 0, + deleted: 0, + errors: [] as Array<{ projectId: number; error: string; operation: string }>, + }, + configuredWebhookCount: 0, + }; + } + + let webhookSyncResult: { + created: number; + updated: number; + deleted: number; + errors: Array<{ projectId: number; error: string; operation: string }>; + } = { created: 0, updated: 0, deleted: 0, errors: [] }; + let updatedWebhooks: Record = configuredWebhooks; + + try { + const accessToken = await getValidGitLabToken(integration, { + userId: ctx.user.id, + organizationId, + }); + const configuredRepoIds = Object.keys(configuredWebhooks) + .map(id => Number.parseInt(id, 10)) + .filter(id => Number.isFinite(id)); + + // Pass `previous = []` so the sync helper treats every currently + // configured repo as newly added and UPDATES its existing Kilo + // webhook in place with the new secret — nothing is deleted. + const syncOutcome = await syncWebhooksForRepositories( + accessToken, + newSecret, + configuredRepoIds, + [], + configuredWebhooks, + instanceUrl + ); + updatedWebhooks = syncOutcome.updatedWebhooks; + webhookSyncResult = { + created: syncOutcome.result.created.length, + updated: syncOutcome.result.updated.length, + deleted: syncOutcome.result.deleted.length, + errors: syncOutcome.result.errors, + }; + logExceptInTest('[rotateGitLabWebhookSecret] Webhook re-sync completed for organization', { + created: webhookSyncResult.created, + updated: webhookSyncResult.updated, + deleted: webhookSyncResult.deleted, + errorCount: webhookSyncResult.errors.length, + }); + } catch (webhookError) { + // Re-sync failure MUST NOT lose the new secret: persist it + // anyway so the operator can recover via manual reconfiguration. + logExceptInTest('[rotateGitLabWebhookSecret] Webhook re-sync failed for organization', { + error: webhookError instanceof Error ? webhookError.message : String(webhookError), + }); + webhookSyncResult = { + created: 0, + updated: 0, + deleted: 0, + errors: [ + { + projectId: 0, + error: webhookError instanceof Error ? webhookError.message : 'Unknown error', + operation: 'sync' as const, + }, + ], + }; + } + + await updateIntegrationMetadata(integration.id, { + ...existingMetadata, + webhook_secret: newSecret, + configured_webhooks: updatedWebhooks, + }); + + await createAuditLog({ + organization_id: organizationId, + action: 'organization.settings.change', + actor_id: ctx.user.id, + actor_email: ctx.user.google_user_email, + actor_name: ctx.user.google_user_name, + message: `Rotated GitLab webhook secret (webhooks: ${webhookSyncResult.updated} updated, ${webhookSyncResult.errors.length} errors)`, + }); + + return { + webhookSecret: newSecret, + webhookSync: webhookSyncResult, + configuredWebhookCount: Object.keys(updatedWebhooks).length, + }; + }), }); diff --git a/packages/app-shared/src/code-review/config.test.ts b/packages/app-shared/src/code-review/config.test.ts index 203d4dcca6..83e7fdc310 100644 --- a/packages/app-shared/src/code-review/config.test.ts +++ b/packages/app-shared/src/code-review/config.test.ts @@ -1,89 +1,182 @@ import { describe, expect, it } from 'vitest'; -import { buildSaveConfigInput, type CodeReviewConfigInput } from './config'; +import { applyCodeReviewConfigPatch, type CodeReviewStoredConfig } from './config'; -// Moved from apps/mobile/src/lib/code-reviewer-config.test.ts — assertions -// kept identical, only the imported type name changed (ReviewConfigData -> -// CodeReviewConfigInput, this module's structural equivalent). -const config: CodeReviewConfigInput = { +// Stored config snapshot covering every field the helper knows about, so each +// test can assert "this field is preserved" cleanly. +const stored: CodeReviewStoredConfig = { reviewStyle: 'balanced', - focusAreas: ['bugs', 'security'], - customInstructions: null, + focusAreas: ['bugs'], + customInstructions: 'be terse', modelSlug: 'anthropic/claude-sonnet-5', thinkingEffort: null, gateThreshold: 'off', repositorySelectionMode: 'all', - selectedRepositoryIds: [], - repositoryModelOverrides: [], + selectedRepositoryIds: [101, 202], + repositoryModelOverrides: [ + { + repositoryId: 101, + repoFullName: 'acme/api', + modelSlug: 'openai/gpt-5', + thinkingEffort: 'high', + }, + ], disableReviewMd: true, + manuallyAddedRepositories: [{ id: 9, name: 'manual', full_name: 'manual/repo', private: true }], + council: { + enabled: true, + aggregation_strategy: 'unanimous', + specialists: [ + { + id: 'security', + role: 'security', + name: 'Security', + enabled: true, + required: false, + lens: 'audit', + }, + ], + }, + councilEnabledRepositoryIds: [101, 202], }; -describe('buildSaveConfigInput', () => { - it('carries the full current config for an untouched field', () => { - const input = buildSaveConfigInput('github', config, { reviewStyle: 'strict' }); - expect(input).toEqual({ - platform: 'github', +describe('applyCodeReviewConfigPatch', () => { + it('preserves every field of `stored` when the patch is empty', () => { + const merged = applyCodeReviewConfigPatch(stored, {}); + expect(merged).toEqual(stored); + }); + + it('does not mutate `stored` or the patch', () => { + const storedSnapshot = JSON.parse(JSON.stringify(stored)); + const patch = { + reviewStyle: 'strict' as const, + focusAreas: ['security'], + }; + const patchSnapshot = JSON.parse(JSON.stringify(patch)); + applyCodeReviewConfigPatch(stored, patch); + expect(stored).toEqual(storedSnapshot); + expect(patch).toEqual(patchSnapshot); + }); + + it('preserves council / manuallyAddedRepositories / councilEnabledRepositoryIds when the mobile-style patch omits them', () => { + // Mobile's PATCH only sends the mobile-shaped fields — council-related + // fields must round-trip through untouched. + const merged = applyCodeReviewConfigPatch(stored, { reviewStyle: 'strict', - focusAreas: ['bugs', 'security'], - customInstructions: undefined, - modelSlug: 'anthropic/claude-sonnet-5', - thinkingEffort: null, - gateThreshold: 'off', - repositorySelectionMode: 'all', - selectedRepositoryIds: [], - repositoryModelOverrides: [], - disableReviewMd: true, + focusAreas: ['performance'], + customInstructions: 'be nice', + modelSlug: 'openai/gpt-5', + }); + expect(merged.reviewStyle).toBe('strict'); + expect(merged.focusAreas).toEqual(['performance']); + expect(merged.customInstructions).toBe('be nice'); + expect(merged.modelSlug).toBe('openai/gpt-5'); + expect(merged.council).toEqual(stored.council); + expect(merged.manuallyAddedRepositories).toEqual(stored.manuallyAddedRepositories); + expect(merged.councilEnabledRepositoryIds).toEqual(stored.councilEnabledRepositoryIds); + // Unrelated stored fields stay put. + expect(merged.thinkingEffort).toBe(stored.thinkingEffort); + expect(merged.gateThreshold).toBe(stored.gateThreshold); + expect(merged.repositorySelectionMode).toBe(stored.repositorySelectionMode); + expect(merged.selectedRepositoryIds).toEqual(stored.selectedRepositoryIds); + }); + + it('updates only the keys present in the patch and leaves the rest alone', () => { + const merged = applyCodeReviewConfigPatch(stored, { + gateThreshold: 'critical', + disableReviewMd: false, }); + expect(merged.gateThreshold).toBe('critical'); + expect(merged.disableReviewMd).toBe(false); + expect(merged.reviewStyle).toBe(stored.reviewStyle); + expect(merged.focusAreas).toEqual(stored.focusAreas); + expect(merged.customInstructions).toBe(stored.customInstructions); + expect(merged.modelSlug).toBe(stored.modelSlug); + expect(merged.selectedRepositoryIds).toEqual(stored.selectedRepositoryIds); + expect(merged.repositoryModelOverrides).toEqual(stored.repositoryModelOverrides); + expect(merged.council).toEqual(stored.council); + expect(merged.councilEnabledRepositoryIds).toEqual(stored.councilEnabledRepositoryIds); }); - it('preserves repository model overrides across an unrelated patch', () => { - const overrides = [ + it('replaces repositoryModelOverrides only when the patch supplies it (camelCase stays camelCase)', () => { + const newOverrides = [ { - repositoryId: 123, - repoFullName: 'acme/api', + repositoryId: 303, + repoFullName: 'acme/web', modelSlug: 'anthropic/claude-opus-4.8', thinkingEffort: null, }, ]; - const input = buildSaveConfigInput( - 'github', - { ...config, repositoryModelOverrides: overrides }, - { reviewStyle: 'strict' } - ); - expect(input.repositoryModelOverrides).toEqual(overrides); - }); - - it('applies patches over current values', () => { - const input = buildSaveConfigInput('github', config, { - focusAreas: ['performance'], - customInstructions: 'be nice', + const merged = applyCodeReviewConfigPatch(stored, { + repositoryModelOverrides: newOverrides, }); - expect(input.focusAreas).toEqual(['performance']); - expect(input.customInstructions).toBe('be nice'); - expect(input.reviewStyle).toBe('balanced'); - }); + // Same identity (shallow) — the helper doesn't clone complex values. + expect(merged.repositoryModelOverrides).toBe(newOverrides); + // No snake_case key leaks in: stored still uses `repositoryId`, + // `repoFullName`, etc. + expect(merged.repositoryModelOverrides?.[0]).toEqual({ + repositoryId: 303, + repoFullName: 'acme/web', + modelSlug: 'anthropic/claude-opus-4.8', + thinkingEffort: null, + }); + const firstOverride = (merged.repositoryModelOverrides ?? [])[0] as Record; + expect(firstOverride.repository_id).toBeUndefined(); - it('includes autoConfigureWebhooks for gitlab', () => { - const input = buildSaveConfigInput('gitlab', config, {}); - expect(input.platform).toBe('gitlab'); - expect(input.autoConfigureWebhooks).toBe(true); + // And the omits case keeps the stored array intact. + const untouched = applyCodeReviewConfigPatch(stored, { reviewStyle: 'lenient' }); + expect(untouched.repositoryModelOverrides).toBe(stored.repositoryModelOverrides); }); - it('carries string repository ids for bitbucket', () => { - const input = buildSaveConfigInput('bitbucket', config, { - selectedRepositoryIds: ['uuid-1'], - }); - expect(input.platform).toBe('bitbucket'); - expect(input.selectedRepositoryIds).toEqual(['uuid-1']); + it('treats a `null` patch value as an explicit clear (overrides stored), not an omit', () => { + const merged = applyCodeReviewConfigPatch(stored, { council: null }); + expect(merged.council).toBeNull(); + // Other fields preserved. + expect(merged.reviewStyle).toBe(stored.reviewStyle); + expect(merged.councilEnabledRepositoryIds).toEqual(stored.councilEnabledRepositoryIds); }); - it('forces selected repository mode for gitlab even when config default is all', () => { - const input = buildSaveConfigInput('gitlab', config, {}); - expect(input.repositorySelectionMode).toBe('selected'); + it('ignores explicit `undefined` values in the patch (preserves stored)', () => { + const merged = applyCodeReviewConfigPatch(stored, { + reviewStyle: undefined, + // Cast to satisfy the partial type at the test call site. + council: undefined, + }); + expect(merged.reviewStyle).toBe(stored.reviewStyle); + expect(merged.council).toEqual(stored.council); }); - it('forces selected repository mode for bitbucket even when config default is all', () => { - const input = buildSaveConfigInput('bitbucket', config, {}); - expect(input.repositorySelectionMode).toBe('selected'); + it('updates council + councilEnabledRepositoryIds when the org patch supplies them', () => { + const newCouncil = { + enabled: true, + aggregation_strategy: 'majority' as const, + specialists: [ + { + id: 'security', + role: 'security', + name: 'Security', + enabled: true, + required: false, + lens: 'audit', + }, + { + id: 'perf', + role: 'performance', + name: 'Performance', + enabled: true, + required: false, + lens: 'latency', + }, + ], + }; + const merged = applyCodeReviewConfigPatch(stored, { + council: newCouncil, + councilEnabledRepositoryIds: [303, 404], + }); + expect(merged.council).toBe(newCouncil); + expect(merged.councilEnabledRepositoryIds).toEqual([303, 404]); + // Unrelated fields preserved. + expect(merged.manuallyAddedRepositories).toEqual(stored.manuallyAddedRepositories); + expect(merged.repositoryModelOverrides).toEqual(stored.repositoryModelOverrides); }); }); diff --git a/packages/app-shared/src/code-review/config.ts b/packages/app-shared/src/code-review/config.ts index 8ed7615a26..47bc7c0618 100644 --- a/packages/app-shared/src/code-review/config.ts +++ b/packages/app-shared/src/code-review/config.ts @@ -1,4 +1,4 @@ -import type { CodeReviewPlatform, GateThreshold, ReviewStyle } from './enums'; +import type { GateThreshold, ReviewStyle } from './enums'; // Wire shape of a per-repository model override. Mirrors the tRPC input/output // contract (camelCase); the persisted snake_case shape lives in @@ -7,26 +7,48 @@ export type RepositoryModelOverrideInput = { repositoryId: number | string; repoFullName: string; modelSlug: string; - thinkingEffort: string | null; + // Optional to mirror the tRPC input contract (RepositoryModelOverrideInputSchema + // marks thinkingEffort `.nullable().optional()`); an omitted value means "no + // override effort". Read sites already coalesce with `?? null` when persisting. + thinkingEffort?: string | null; }; -// Structural shape of the review config a save request is built from — -// matches apps/mobile/src/lib/code-reviewer-config.ts's ReviewConfigData -// (mobile keeps that name/type locally, derived from its tRPC query output; -// this is only the subset buildSaveConfigInput actually reads). -export type CodeReviewConfigInput = { - reviewStyle: ReviewStyle; - focusAreas: string[]; - customInstructions: string | null; - modelSlug: string; - thinkingEffort: string | null; - gateThreshold: GateThreshold; - repositorySelectionMode: 'all' | 'selected'; - selectedRepositoryIds: (number | string)[]; - repositoryModelOverrides: RepositoryModelOverrideInput[]; - disableReviewMd: boolean; +// Wire shape of a manually-added repository (GitLab pagination workaround). +// Mirrors the tRPC input contract; the persisted snake_case shape lives in +// ManuallyAddedRepositorySchema in @kilocode/db/schema-types. +export type ManuallyAddedRepositoryInput = { + id: number; + name: string; + full_name: string; + private: boolean; +}; + +// Wire shape of the Code Reviewer council config (camelCase keys mirror the +// tRPC patch input; the persisted snake_case shape is +// CodeReviewCouncilConfigSchema in @kilocode/db/schema-types). Kept loose on +// purpose — callers zod-validate against the canonical schema at the route +// boundary, and this helper only needs to know it's an opaque object that +// should be preserved verbatim when the patch doesn't carry it. +export type CodeReviewCouncilConfigInput = { + enabled?: boolean; + aggregation_strategy?: 'majority' | 'unanimous' | 'advisory'; + specialists: Array<{ + id: string; + role: string; + name: string; + enabled: boolean; + required: boolean; + lens: string; + model_slug?: string; + thinking_effort?: string | null; + }>; }; +// Mobile/personal-org save patch. All keys are optional; omission preserves the +// stored value. The PATCH route handlers spread this into the saved payload, so +// it MUST NOT carry org-only field-merge fields (manuallyAddedRepositories, +// council, councilEnabledRepositoryIds) that the strict saveReviewConfig schemas +// do not accept. export type CodeReviewConfigPatch = Partial<{ reviewStyle: ReviewStyle; focusAreas: string[]; @@ -40,42 +62,64 @@ export type CodeReviewConfigPatch = Partial<{ disableReviewMd: boolean; }>; -// Ported verbatim from apps/mobile/src/lib/code-reviewer-config.ts. +// Field-merge PATCH surface. Extends the save patch with the org-only fields +// that the PATCH procedures preserve but the strict saveReviewConfig schemas do +// not accept. Used by applyCodeReviewConfigPatch and the two PATCH route handlers. +export type CodeReviewFieldMergePatch = CodeReviewConfigPatch & + Partial<{ + manuallyAddedRepositories: ManuallyAddedRepositoryInput[]; + council: CodeReviewCouncilConfigInput | null; + councilEnabledRepositoryIds: (number | string)[]; + }>; + +// Snapshot of a stored Code Reviewer config in camelCase, suitable as the +// `stored` argument to `applyCodeReviewConfigPatch`. Every field is optional +// because callers may have a subset (e.g. personal configs never carry +// `council`); the helper preserves whatever is supplied. +export type CodeReviewStoredConfig = { + reviewStyle?: ReviewStyle; + focusAreas?: string[]; + customInstructions?: string | null; + modelSlug?: string; + thinkingEffort?: string | null; + gateThreshold?: GateThreshold; + repositorySelectionMode?: 'all' | 'selected'; + selectedRepositoryIds?: (number | string)[]; + repositoryModelOverrides?: RepositoryModelOverrideInput[]; + disableReviewMd?: boolean; + // Org-only fields. Personal configs never have these in storage, so the + // stored snapshot can omit them and the helper still works. + manuallyAddedRepositories?: ManuallyAddedRepositoryInput[]; + council?: CodeReviewCouncilConfigInput | null; + councilEnabledRepositoryIds?: (number | string)[]; +}; + +// Field-merge helper for the PATCH endpoints (`personalReviewAgent.patchReviewConfig` +// and `organizations.reviewAgent.patchReviewConfig`). Returns a new object +// containing every field of `stored` plus any field explicitly set in `patch` +// (where "set" means `hasOwnProperty` AND value is not `undefined` — `null` is +// a real "clear" value, e.g. `council: null` disables council). // -// This is mobile-flavored, not a shared web/mobile rule: web's -// ReviewConfigForm.tsx builds its save payload inline and does NOT force -// 'selected' repo mode or a fixed autoConfigureWebhooks for gitlab — it -// exposes autoConfigureWebhooks as a user-toggleable checkbox (default true) -// and only forces repositorySelectionMode to 'selected' in local UI state -// (via a useEffect keyed off isGitLab), and it never sends bitbucket at all -// (ReviewConfigForm's Platform type is 'github' | 'gitlab' only). So this -// function stays mobile's rule, ported unchanged; web is not adapted to it. -export function buildSaveConfigInput( - platform: CodeReviewPlatform, - config: CodeReviewConfigInput, - patch: CodeReviewConfigPatch -) { - return { - platform, - reviewStyle: config.reviewStyle, - focusAreas: config.focusAreas, - customInstructions: config.customInstructions ?? undefined, - modelSlug: config.modelSlug, - thinkingEffort: config.thinkingEffort, - gateThreshold: config.gateThreshold, - // GitLab and Bitbucket only support 'selected' repo mode server-side; the - // mode picker only exists for github, so force it here instead of relying - // on a config default that can still be 'all'. - repositorySelectionMode: - platform === 'gitlab' || platform === 'bitbucket' - ? ('selected' as const) - : config.repositorySelectionMode, - selectedRepositoryIds: config.selectedRepositoryIds, - // Preserve web-created overrides across mobile settings edits (mobile has no - // override editing UI in v1). The server prunes these to the current selection. - repositoryModelOverrides: config.repositoryModelOverrides, - disableReviewMd: config.disableReviewMd, - ...(platform === 'gitlab' ? { autoConfigureWebhooks: true as const } : {}), - ...patch, - }; +// Does not mutate either argument. Keys present on `patch` are copied via a +// shallow assignment, so complex values (arrays, council objects) are +// referenced, not cloned — callers that mutate the returned override arrays +// in place would observe the change on `patch.repositoryModelOverrides` too. +// In practice the route handler passes the result straight to upsert without +// further mutation, so this is fine and matches the spec's "smallest boring +// implementation" rule. +export function applyCodeReviewConfigPatch( + stored: CodeReviewStoredConfig, + patch: CodeReviewFieldMergePatch +): CodeReviewStoredConfig { + const merged: CodeReviewStoredConfig = { ...stored }; + for (const key of Object.keys(patch) as Array) { + if (Object.prototype.hasOwnProperty.call(patch, key)) { + const value = patch[key]; + if (value !== undefined) { + // Safe: every key in the patch is also a key on CodeReviewStoredConfig. + (merged as Record)[key] = value; + } + } + } + return merged; } diff --git a/packages/app-shared/src/code-review/index.ts b/packages/app-shared/src/code-review/index.ts index 989526b042..366d87bdf3 100644 --- a/packages/app-shared/src/code-review/index.ts +++ b/packages/app-shared/src/code-review/index.ts @@ -2,3 +2,4 @@ export * from './enums'; export * from './status'; export * from './links'; export * from './config'; +export * from './mention-command'; diff --git a/packages/app-shared/src/code-review/mention-command.test.ts b/packages/app-shared/src/code-review/mention-command.test.ts new file mode 100644 index 0000000000..b700d5e028 --- /dev/null +++ b/packages/app-shared/src/code-review/mention-command.test.ts @@ -0,0 +1,64 @@ +import { describe, expect, it } from 'vitest'; + +import { parseFixCommand } from './mention-command'; + +describe('parseFixCommand', () => { + describe('admits', () => { + it('admits the product-advertised @kilocode-bot fix it command', () => { + // The exact command string the inlineCommentFooter in + // apps/web/src/lib/code-reviews/prompts/default-prompt-template.json + // asks users to reply with. Regression guard: the previous + // /@kilo\b/i pattern rejected this because \b does not match + // between letters of "kilocode". + expect(parseFixCommand('@kilocode-bot fix it')).toBe(true); + }); + + it('admits the existing shorthand @kilo please fix', () => { + expect(parseFixCommand('@kilo please fix')).toBe(true); + }); + + it('admits the existing shorthand @kilo patch this', () => { + expect(parseFixCommand('@kilo patch this')).toBe(true); + }); + + it('admits @kilocode (no -bot suffix) with a fix keyword', () => { + expect(parseFixCommand('@kilocode can you fix this?')).toBe(true); + }); + + it('is case-insensitive for both mention and fix keyword', () => { + expect(parseFixCommand('@KiloCode-Bot FIX it')).toBe(true); + expect(parseFixCommand('@KILO Patch this')).toBe(true); + }); + + it('admits when the mention and fix keyword appear in either order', () => { + expect(parseFixCommand('Please fix this @kilocode-bot thanks')).toBe(true); + }); + }); + + describe('rejects', () => { + it('rejects a mention without a fix keyword', () => { + expect(parseFixCommand('@kilocode-bot ship it')).toBe(false); + expect(parseFixCommand('@kilo ship it')).toBe(false); + }); + + it('rejects a fix keyword without a mention', () => { + expect(parseFixCommand('please fix this')).toBe(false); + expect(parseFixCommand('patch this thing')).toBe(false); + }); + + it('rejects an empty string', () => { + expect(parseFixCommand('')).toBe(false); + }); + + it('rejects unrelated text', () => { + expect(parseFixCommand('Looks good to me!')).toBe(false); + expect(parseFixCommand('LGTM, merging.')).toBe(false); + }); + + it('rejects unrelated @kilo-prefixed mentions that are not Kilo handles', () => { + expect(parseFixCommand('@kilocorp fix it')).toBe(false); + expect(parseFixCommand('@kilogram patch this')).toBe(false); + expect(parseFixCommand('@kilobyte fix')).toBe(false); + }); + }); +}); diff --git a/packages/app-shared/src/code-review/mention-command.ts b/packages/app-shared/src/code-review/mention-command.ts new file mode 100644 index 0000000000..2fcd2199da --- /dev/null +++ b/packages/app-shared/src/code-review/mention-command.ts @@ -0,0 +1,40 @@ +/** + * Shared parser that decides whether a free-form text body (typically a + * GitHub PR review comment) should be admitted as a request for Kilo to + * auto-fix the issue it discusses. + * + * Why this lives in @kilocode/app-shared and not in the webhook consumer: + * the canonical "what command admits a fix" rule has to stay in lock-step + * with the user-facing footer that the code-review prompt advertises in + * inline review comments (see + * apps/web/src/lib/code-reviews/prompts/default-prompt-template.json, + * field inlineCommentFooter). The drift-guard test in apps/web + * (default-prompt-template.drift-guard.test.ts) reads that footer literal + * and asserts this parser still admits the exact command it advertises, so a + * future change to the footer or the parser that breaks the contract fails + * the test instead of silently regressing the product. + * + * The mention pattern is deliberately broadened from the previous strict + * one (which rejected the product-advertised "@kilocode-bot fix it" + * because the word-boundary assertion did not match between the letters of + * "kilocode"): the new pattern admits @kilo, @kilocode, and + * @kilocode-bot (and any future @kilo… variant) while still requiring a + * fix-or-patch keyword. A bare "fix" or "patch" with no @kilo* mention is + * rejected so unrelated comment text does not trigger Auto Fix. + */ + +/** + * The mention pattern admits the known Kilo handles — @kilo, @kilocode, + * and @kilocode-bot — without matching unrelated tokens that start with the + * "kilo" prefix (e.g. @kilocorp, @kilogram). The first alternative matches a + * standalone @kilo; the second matches @kilocode with an optional suffix. + */ +const MENTION_PATTERN = /@kilo(?:\b|code[\w-]*\b)/i; +const FIX_KEYWORD_PATTERN = /\b(?:fix|patch)\b/i; + +export function parseFixCommand(text: string): boolean { + if (typeof text !== 'string' || text.length === 0) { + return false; + } + return MENTION_PATTERN.test(text) && FIX_KEYWORD_PATTERN.test(text); +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1f46242c20..dc59808704 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -666,7 +666,7 @@ importers: version: 4.27.0 '@chat-adapter/linear': specifier: 4.27.0 - version: 4.27.0 + version: 4.27.0(graphql@16.14.2) '@chat-adapter/slack': specifier: 4.27.0 version: 4.27.0(bufferutil@4.1.0)(utf-8-validate@6.0.6) @@ -732,7 +732,7 @@ importers: version: link:../../packages/worker-utils '@linear/sdk': specifier: 76.0.0 - version: 76.0.0 + version: 76.0.0(graphql@16.14.2) '@lottiefiles/dotlottie-react': specifier: 0.17.15 version: 0.17.15(react@19.2.6) @@ -1076,6 +1076,9 @@ importers: '@jest/globals': specifier: 30.3.0 version: 30.3.0 + '@octokit/graphql-schema': + specifier: 15.26.1 + version: 15.26.1 '@playwright/test': specifier: 1.58.2 version: 1.58.2 @@ -1121,6 +1124,9 @@ importers: dotenv: specifier: 17.3.1 version: 17.3.1 + graphql: + specifier: 16.14.2 + version: 16.14.2 ink: specifier: 6.8.0 version: 6.8.0(@types/react@19.2.14)(bufferutil@4.1.0)(react-devtools-core@6.1.5(bufferutil@4.1.0)(utf-8-validate@6.0.6))(react@19.2.6)(utf-8-validate@6.0.6) @@ -5905,6 +5911,9 @@ packages: resolution: {integrity: sha512-FWFlNxghg4HrXkD3ifYbS/IdL/mDHjh9QcsNyhQjN8dplUoZbejsdpmuqdA76nxj2xoWPs7p8uX2SNr9rYu0Ag==} engines: {node: '>= 20'} + '@octokit/graphql-schema@15.26.1': + resolution: {integrity: sha512-RFDC2MpRBd4AxSRvUeBIVeBU7ojN/SxDfALUd7iVYOSeEK3gZaqR2MGOysj4Zh2xj2RY5fQAUT+Oqq7hWTraMA==} + '@octokit/graphql@9.0.3': resolution: {integrity: sha512-grAEuupr/C1rALFnXTv6ZQhFuL1D8G5y8CN04RgrO4FIPMrtm+mcZzFG7dcBm+nq+1ppNixu+Jd78aeJOYxlGA==} engines: {node: '>= 20'} @@ -13010,6 +13019,16 @@ packages: resolution: {integrity: sha512-7dYm06A945mXuIk/5HUlSjeyIYChW8vCEiU2dkOKKqJJzwAWxTkCc91Eqbz7TgODh2rtFFKWI/fekowWHOkmjQ==} engines: {node: ^12.20.0 || >=14.13.1} + graphql-tag@2.12.7: + resolution: {integrity: sha512-xnE/NFzy+0eIesvAsREJZ284zTl/wYuBAvpsFSDhRGRdRHdnE90M21Q3xAWyYInb0J756c6x0pIQ62+vtvOs1Q==} + engines: {node: '>=10'} + peerDependencies: + graphql: ^0.9.0 || ^0.10.0 || ^0.11.0 || ^0.12.0 || ^0.13.0 || ^14.0.0 || ^15.0.0 || ^16.0.0 || ^17.0.0 + + graphql@16.14.2: + resolution: {integrity: sha512-Chq1s4CY7jmh8gO2qvLIJyfCDIN+EHLFW/9iShnp1z8FjBQMoodWP1kDC36VAMXXIvAjj4ARa7ntfAV2BrjsbA==} + engines: {node: ^12.22.0 || ^14.16.0 || ^16.0.0 || >=17.0.0} + growly@1.3.0: resolution: {integrity: sha512-+xGQY0YyAWCnqy7Cd++hc2JqMYzlm0dG30Jd0beaA64sROr8C4nt8Yc9V5Ro3avlSUDTN0ulqP/VBKi1/lLygw==} @@ -19791,10 +19810,10 @@ snapshots: transitivePeerDependencies: - supports-color - '@chat-adapter/linear@4.27.0': + '@chat-adapter/linear@4.27.0(graphql@16.14.2)': dependencies: '@chat-adapter/shared': 4.27.0 - '@linear/sdk': 76.0.0 + '@linear/sdk': 76.0.0(graphql@16.14.2) chat: 4.27.0 transitivePeerDependencies: - graphql @@ -21072,7 +21091,9 @@ snapshots: '@grammyjs/types@3.27.3': {} - '@graphql-typed-document-node/core@3.2.0': {} + '@graphql-typed-document-node/core@3.2.0(graphql@16.14.2)': + dependencies: + graphql: 16.14.2 '@hapi/hoek@9.3.0': {} @@ -21881,9 +21902,9 @@ snapshots: '@lezer/highlight': 1.2.3 '@lezer/lr': 1.4.10 - '@linear/sdk@76.0.0': + '@linear/sdk@76.0.0(graphql@16.14.2)': dependencies: - '@graphql-typed-document-node/core': 3.2.0 + '@graphql-typed-document-node/core': 3.2.0(graphql@16.14.2) transitivePeerDependencies: - graphql @@ -22296,6 +22317,11 @@ snapshots: '@octokit/types': 16.0.0 universal-user-agent: 7.0.3 + '@octokit/graphql-schema@15.26.1': + dependencies: + graphql: 16.14.2 + graphql-tag: 2.12.7(graphql@16.14.2) + '@octokit/graphql@9.0.3': dependencies: '@octokit/request': 10.0.8 @@ -30004,6 +30030,13 @@ snapshots: - encoding - supports-color + graphql-tag@2.12.7(graphql@16.14.2): + dependencies: + graphql: 16.14.2 + tslib: 2.8.1 + + graphql@16.14.2: {} + growly@1.3.0: {} gzip-size@6.0.0: