Skip to content

Commit 9808635

Browse files
authored
refactor(code-review): remove feature flags (#2764)
* fix(code-review): skip merge commits before cancelling in-flight reviews When a pull_request.synchronize / merge_request.update event arrives with a merge-commit head (e.g. clicking GitHub's 'Update branch' button), the review was being cancelled before the merge-commit skip check ran, leaving the PR with no active review. Reorder so the merge-commit check runs first and bails out without touching existing reviews. Extract the decision into small pure helpers for unit testing. * chore(code-review): drop review feature flags * fix(review): set initial agent version
1 parent b56b478 commit 9808635

14 files changed

Lines changed: 384 additions & 259 deletions

File tree

apps/web/src/components/code-reviews/ReviewConfigForm.tsx

Lines changed: 22 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -205,8 +205,6 @@ export function ReviewConfigForm({
205205
const [selectedModel, setSelectedModel] = useState(PRIMARY_DEFAULT_MODEL);
206206
const [thinkingEffort, setThinkingEffort] = useState<string | null>(null);
207207
const [gateThreshold, setGateThreshold] = useState<'off' | 'all' | 'warning' | 'critical'>('off');
208-
const isCloudAgentNextEnabled = configData?.isCloudAgentNextEnabled ?? false;
209-
const isPrGateEnabled = configData?.isPrGateEnabled ?? false;
210208
const [repositorySelectionMode, setRepositorySelectionMode] = useState<'all' | 'selected'>('all');
211209
const [selectedRepositoryIds, setSelectedRepositoryIds] = useState<number[]>([]);
212210
// Repositories added from search results (for GitLab where pagination limits initial results)
@@ -551,8 +549,8 @@ export function ReviewConfigForm({
551549
helperText="Choose the AI model to use for code reviews"
552550
/>
553551

554-
{/* Thinking Effort — only shown when cloud-agent-next is available and the model supports variants */}
555-
{availableVariants.length > 0 && isCloudAgentNextEnabled && (
552+
{/* Thinking Effort — only shown when the model supports variants */}
553+
{availableVariants.length > 0 && (
556554
<div className="space-y-2">
557555
<Label>Thinking Effort</Label>
558556
<Select
@@ -577,29 +575,26 @@ export function ReviewConfigForm({
577575
</div>
578576
)}
579577

580-
{/* PR Gate Threshold — only shown when PR gate feature flag is enabled */}
581-
{isPrGateEnabled && (
582-
<div className="space-y-2">
583-
<Label>PR Gate Threshold</Label>
584-
<Select
585-
value={gateThreshold}
586-
onValueChange={v => setGateThreshold(v as 'off' | 'all' | 'warning' | 'critical')}
587-
>
588-
<SelectTrigger className="w-full">
589-
<SelectValue />
590-
</SelectTrigger>
591-
<SelectContent>
592-
<SelectItem value="off">Off</SelectItem>
593-
<SelectItem value="all">All findings</SelectItem>
594-
<SelectItem value="warning">Warnings and above</SelectItem>
595-
<SelectItem value="critical">Critical issues only</SelectItem>
596-
</SelectContent>
597-
</Select>
598-
<p className="text-muted-foreground text-sm">
599-
Controls when the PR status check reports a failure based on review findings
600-
</p>
601-
</div>
602-
)}
578+
<div className="space-y-2">
579+
<Label>PR Gate Threshold</Label>
580+
<Select
581+
value={gateThreshold}
582+
onValueChange={v => setGateThreshold(v as 'off' | 'all' | 'warning' | 'critical')}
583+
>
584+
<SelectTrigger className="w-full">
585+
<SelectValue />
586+
</SelectTrigger>
587+
<SelectContent>
588+
<SelectItem value="off">Off</SelectItem>
589+
<SelectItem value="all">All findings</SelectItem>
590+
<SelectItem value="warning">Warnings and above</SelectItem>
591+
<SelectItem value="critical">Critical issues only</SelectItem>
592+
</SelectContent>
593+
</Select>
594+
<p className="text-muted-foreground text-sm">
595+
Controls when the PR status check reports a failure based on review findings
596+
</p>
597+
</div>
603598

604599
{/* Review Style */}
605600
<div className="space-y-3">

apps/web/src/lib/code-reviews/core/constants.ts

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,6 @@ export const DEFAULT_CODE_REVIEW_MODEL = 'anthropic/claude-sonnet-4.6';
1616
*/
1717
export const DEFAULT_CODE_REVIEW_MODE = 'code' as const;
1818

19-
// ============================================================================
20-
// Feature Flags
21-
// ============================================================================
22-
23-
/** PostHog flag that gates incremental (diff-only) reviews on follow-up pushes */
24-
export const FEATURE_FLAG_INCREMENTAL_REVIEW = 'code-review-incremental';
25-
2619
// ============================================================================
2720
// Pagination
2821
// ============================================================================

apps/web/src/lib/code-reviews/db/code-reviews.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,4 +129,16 @@ describe('findPreviousCompletedReview', () => {
129129

130130
expect(review?.terminalReason).toBe('billing');
131131
});
132+
133+
it('creates new reviews with agent_version set to v2', async () => {
134+
const id = await createReview('sha-v2-default');
135+
136+
const [review] = await db
137+
.select({ agentVersion: cloud_agent_code_reviews.agent_version })
138+
.from(cloud_agent_code_reviews)
139+
.where(eq(cloud_agent_code_reviews.id, id))
140+
.limit(1);
141+
142+
expect(review?.agentVersion).toBe('v2');
143+
});
132144
});

apps/web/src/lib/code-reviews/db/code-reviews.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ export async function createCodeReview(params: CreateReviewParams): Promise<stri
4040
head_sha: params.headSha,
4141
platform: params.platform ?? 'github',
4242
platform_project_id: params.platformProjectId ?? null,
43+
agent_version: 'v2',
4344
status: 'pending',
4445
})
4546
.returning({ id: cloud_agent_code_reviews.id });

apps/web/src/lib/code-reviews/dispatch/dispatch-pending-reviews.ts

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import { getAgentConfigForOwner } from '@/lib/agent-config/db/agent-configs';
1818
import { updateCodeReviewStatus } from '../db/code-reviews';
1919
import { captureException } from '@sentry/nextjs';
2020
import { errorExceptInTest, logExceptInTest } from '@/lib/utils.server';
21-
import { isFeatureFlagEnabled } from '@/lib/posthog-feature-flags';
2221
import { codeReviewWorkerClient } from '../client/code-review-worker-client';
2322
import type { CodeReviewPlatform } from '../core/schemas';
2423

@@ -201,26 +200,15 @@ async function dispatchReview(
201200
);
202201
}
203202

204-
// 2. Evaluate feature flag: use cloud-agent-next?
205-
const useCloudAgentNext =
206-
process.env.NODE_ENV === 'development' ||
207-
(await isFeatureFlagEnabled('code-review-cloud-agent-next', owner.userId));
208-
209-
logExceptInTest('[dispatchReview] Feature flag evaluated', {
210-
reviewId: review.id,
211-
userId: owner.userId,
212-
useCloudAgentNext,
213-
});
214-
215-
// 3. Prepare complete payload for cloud agent
203+
// 2. Prepare complete payload for cloud agent
216204
const payload = await prepareReviewPayload({
217205
reviewId: review.id,
218206
owner,
219207
agentConfig,
220208
platform,
221209
});
222210

223-
// 4. Atomically claim the review to prevent concurrent dispatchers from
211+
// 3. Atomically claim the review to prevent concurrent dispatchers from
224212
// picking the same review. Done as late as possible (after all prep work)
225213
// to minimise the crash window between claim and dispatch.
226214
// Accepts 'pending' (normal) or stale 'queued' (abandoned claim recovery).
@@ -248,11 +236,11 @@ async function dispatchReview(
248236
return false;
249237
}
250238

251-
// 5. Dispatch to Cloudflare Worker to create CodeReviewOrchestrator DO.
239+
// 4. Dispatch to Cloudflare Worker to create CodeReviewOrchestrator DO.
252240
// If this fails, keep the claim in `queued` and rely on stale-claim
253241
// recovery. A transport failure is ambiguous: the worker may have
254242
// created the DO even if this request did not observe the response.
255-
const agentVersion = useCloudAgentNext ? 'v2' : 'v1';
243+
const agentVersion = 'v2';
256244
try {
257245
await codeReviewWorkerClient.dispatchReview({
258246
...payload,
@@ -271,7 +259,7 @@ async function dispatchReview(
271259
return false;
272260
}
273261

274-
// 6. Record which agent version was dispatched without rewriting status.
262+
// 5. Record which agent version was dispatched without rewriting status.
275263
// The worker may already have advanced the review to running/completed.
276264
try {
277265
await db

apps/web/src/lib/code-reviews/triggers/prepare-review-payload.ts

Lines changed: 31 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,7 @@ import type {
3737
} from '../prompts/generate-prompt';
3838
import { getIntegrationById } from '@/lib/integrations/db/platform-integrations';
3939
import { getCodeReviewById, findPreviousCompletedReview } from '../db/code-reviews';
40-
import { isFeatureFlagEnabled } from '@/lib/posthog-feature-flags';
41-
import {
42-
DEFAULT_CODE_REVIEW_MODEL,
43-
DEFAULT_CODE_REVIEW_MODE,
44-
FEATURE_FLAG_INCREMENTAL_REVIEW,
45-
} from '../core/constants';
40+
import { DEFAULT_CODE_REVIEW_MODEL, DEFAULT_CODE_REVIEW_MODE } from '../core/constants';
4641
import type { Owner } from '../core';
4742
import { generateReviewPrompt } from '../prompts/generate-prompt';
4843
import type { CodeReviewAgentConfig } from '@/lib/agent-config/core/types';
@@ -319,54 +314,41 @@ export async function prepareReviewPayload(
319314
// continuation) are derived from the same review row to avoid mismatches.
320315
let previousHeadSha: string | null = null;
321316
let previousCloudAgentSessionId: string | undefined;
322-
const incrementalEnabled = await isFeatureFlagEnabled(
323-
FEATURE_FLAG_INCREMENTAL_REVIEW,
324-
owner.userId
325-
);
326-
327-
logExceptInTest('[prepareReviewPayload] Incremental review flag evaluated', {
328-
reviewId,
329-
incrementalEnabled,
330-
ownerId: owner.userId,
331-
});
332-
333-
if (incrementalEnabled) {
334-
try {
335-
const previousReview = await findPreviousCompletedReview(
336-
review.repo_full_name,
337-
review.pr_number,
338-
existingReviewState?.headCommitSha ?? review.head_sha,
339-
platform
340-
);
341-
previousHeadSha = previousReview?.head_sha ?? null;
342-
previousCloudAgentSessionId = previousReview?.session_id ?? undefined;
343-
344-
if (previousHeadSha) {
345-
logExceptInTest(
346-
'[prepareReviewPayload] Found previous completed review for incremental mode',
347-
{
348-
reviewId,
349-
previousHeadSha: previousHeadSha.substring(0, 8),
350-
currentHeadSha: review.head_sha.substring(0, 8),
351-
previousCloudAgentSessionId,
352-
}
353-
);
354-
} else {
355-
logExceptInTest(
356-
'[prepareReviewPayload] No previous completed review found, using full review',
357-
{ reviewId }
358-
);
359-
}
360-
} catch (error) {
361-
// Non-critical - fall back to full review
317+
try {
318+
const previousReview = await findPreviousCompletedReview(
319+
review.repo_full_name,
320+
review.pr_number,
321+
existingReviewState?.headCommitSha ?? review.head_sha,
322+
platform
323+
);
324+
previousHeadSha = previousReview?.head_sha ?? null;
325+
previousCloudAgentSessionId = previousReview?.session_id ?? undefined;
326+
327+
if (previousHeadSha) {
362328
logExceptInTest(
363-
'[prepareReviewPayload] Failed to fetch previous review, falling back to full review:',
329+
'[prepareReviewPayload] Found previous completed review for incremental mode',
364330
{
365331
reviewId,
366-
error,
332+
previousHeadSha: previousHeadSha.substring(0, 8),
333+
currentHeadSha: review.head_sha.substring(0, 8),
334+
previousCloudAgentSessionId,
367335
}
368336
);
337+
} else {
338+
logExceptInTest(
339+
'[prepareReviewPayload] No previous completed review found, using full review',
340+
{ reviewId }
341+
);
369342
}
343+
} catch (error) {
344+
// Non-critical - fall back to full review
345+
logExceptInTest(
346+
'[prepareReviewPayload] Failed to fetch previous review, falling back to full review:',
347+
{
348+
reviewId,
349+
error,
350+
}
351+
);
370352
}
371353

372354
// 5. Generate auth token for cloud agent with bot identifier
@@ -402,12 +384,7 @@ export async function prepareReviewPayload(
402384
// GitHub: uses githubRepo (owner/repo format) + githubToken
403385
// GitLab: uses gitUrl (full HTTPS URL) + gitToken
404386
const variant = config.thinking_effort ?? undefined;
405-
// Defense-in-depth: only send gateThreshold to the agent when the PR gate flag is enabled.
406-
// This prevents a stale non-'off' config from activating gating after the flag is turned off.
407-
const isPrGateEnabled =
408-
process.env.NODE_ENV === 'development' ||
409-
(await isFeatureFlagEnabled('code-review-pr-gate', owner.userId));
410-
const gateThreshold = isPrGateEnabled ? (config.gate_threshold ?? 'off') : 'off';
387+
const gateThreshold = config.gate_threshold ?? 'off';
411388
const sessionInput: SessionInput =
412389
platform === PLATFORM.GITLAB
413390
? {

apps/web/src/lib/integrations/platforms/github/webhook-handlers/pull-request-handler.test.ts

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { resolvePullRequestCheckoutRef } from '@/lib/integrations/platforms/github/webhook-handlers/pull-request-checkout-ref';
2+
import { shouldSkipSynchronizeForMergeCommit } from '@/lib/integrations/platforms/github/webhook-handlers/pull-request-handler';
23

34
describe('resolvePullRequestCheckoutRef', () => {
45
it('uses head.ref for same-repo PRs', () => {
@@ -63,3 +64,64 @@ describe('resolvePullRequestCheckoutRef', () => {
6364
});
6465
});
6566
});
67+
68+
describe('shouldSkipSynchronizeForMergeCommit', () => {
69+
const baseArgs = {
70+
installationId: 'inst-1',
71+
headOwner: 'acme',
72+
headRepoName: 'widgets',
73+
headSha: 'deadbeef',
74+
appType: 'standard' as const,
75+
};
76+
77+
it('returns false for non-synchronize actions without calling the check', async () => {
78+
for (const action of ['opened', 'reopened', 'ready_for_review']) {
79+
let called = false;
80+
const result = await shouldSkipSynchronizeForMergeCommit({
81+
...baseArgs,
82+
action,
83+
isMergeCommitFn: async () => {
84+
called = true;
85+
return true;
86+
},
87+
});
88+
89+
expect(result).toBe(false);
90+
expect(called).toBe(false);
91+
}
92+
});
93+
94+
it('returns true when synchronize head is a merge commit', async () => {
95+
const result = await shouldSkipSynchronizeForMergeCommit({
96+
...baseArgs,
97+
action: 'synchronize',
98+
isMergeCommitFn: async () => true,
99+
});
100+
101+
expect(result).toBe(true);
102+
});
103+
104+
it('returns false when synchronize head is not a merge commit', async () => {
105+
const result = await shouldSkipSynchronizeForMergeCommit({
106+
...baseArgs,
107+
action: 'synchronize',
108+
isMergeCommitFn: async () => false,
109+
});
110+
111+
expect(result).toBe(false);
112+
});
113+
114+
it('passes the expected arguments to the check function', async () => {
115+
const calls: Array<[string, string, string, string, string]> = [];
116+
await shouldSkipSynchronizeForMergeCommit({
117+
...baseArgs,
118+
action: 'synchronize',
119+
isMergeCommitFn: async (installationId, owner, repo, sha, appType) => {
120+
calls.push([installationId, owner, repo, sha, appType]);
121+
return false;
122+
},
123+
});
124+
125+
expect(calls).toEqual([['inst-1', 'acme', 'widgets', 'deadbeef', 'standard']]);
126+
});
127+
});

0 commit comments

Comments
 (0)