Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const mockCancelReview = jest.fn();
const mockAddReactionToPR = jest.fn();
const mockIsMergeCommit = jest.fn();
const mockIsCouncilEntitledForOwner = jest.fn();
const mockGetRepositoryCustomization = jest.fn();

jest.mock('@/lib/bot-users/bot-user-service', () => ({
getBotUserId: (organizationId: string, botType: string) =>
Expand Down Expand Up @@ -55,6 +56,10 @@ jest.mock('@/lib/integrations/platforms/github/adapter', () => ({
updateCheckRun: (...args: unknown[]) => mockUpdateCheckRun(...args),
}));

jest.mock('@/lib/integrations/db/platform-integrations', () => ({
getRepositoryCustomization: (...args: unknown[]) => mockGetRepositoryCustomization(...args),
}));

import {
getGitHubPullRequestCheckoutRef,
resolvePullRequestCheckoutRef,
Expand Down Expand Up @@ -123,6 +128,7 @@ beforeEach(() => {
mockAddReactionToPR.mockResolvedValue(undefined);
mockIsMergeCommit.mockResolvedValue(false);
mockIsCouncilEntitledForOwner.mockResolvedValue(false);
mockGetRepositoryCustomization.mockResolvedValue(null);
});

describe('resolvePullRequestCheckoutRef', () => {
Expand Down Expand Up @@ -293,6 +299,71 @@ describe('handlePullRequest', () => {
);
});

describe('repository PR review toggle', () => {
it('skips the review when the repository has an explicit pr_review_mode override of off', async () => {
mockGetBotUserId.mockResolvedValue('bot-user-1');
mockGetAgentConfigForOwner.mockResolvedValue({ is_enabled: true, config: {} });
mockGetRepositoryCustomization.mockResolvedValue({
bot_mention_model_slug: null,
pr_review_mode: 'off',
});

const response = await handlePullRequest(pullRequestPayload(), platformIntegration());

expect(response.status).toBe(200);
expect(await response.json()).toEqual({ message: 'PR reviews disabled for this repository' });
expect(mockGetRepositoryCustomization).toHaveBeenCalledWith(
'8b2ff443-8396-4b07-99ae-7015789da7dd',
'123'
);
expect(mockCreateCodeReview).not.toHaveBeenCalled();
expect(mockCreateCheckRun).not.toHaveBeenCalled();
expect(mockTryDispatchPendingReviews).not.toHaveBeenCalled();
});

it('skips the review when the installation default pr_review_mode is off and the repo has no override', async () => {
mockGetBotUserId.mockResolvedValue('bot-user-1');
mockGetAgentConfigForOwner.mockResolvedValue({ is_enabled: true, config: {} });
mockGetRepositoryCustomization.mockResolvedValue(null);

const response = await handlePullRequest(
pullRequestPayload(),
platformIntegration({ metadata: { pr_review_mode: 'off' } })
);

expect(response.status).toBe(200);
expect(await response.json()).toEqual({ message: 'PR reviews disabled for this repository' });
expect(mockCreateCodeReview).not.toHaveBeenCalled();
});

it('reviews the PR when the repository override of on takes precedence over an off installation default', async () => {
mockGetBotUserId.mockResolvedValue('bot-user-1');
mockGetAgentConfigForOwner.mockResolvedValue({ is_enabled: true, config: {} });
mockGetRepositoryCustomization.mockResolvedValue({
bot_mention_model_slug: null,
pr_review_mode: 'on',
});

const response = await handlePullRequest(
pullRequestPayload(),
platformIntegration({ metadata: { pr_review_mode: 'off' } })
);

expect(response.status).toBe(202);
expect(mockCreateCodeReview).toHaveBeenCalled();
});

it('reviews the PR when neither the repository nor the installation has an explicit mode (defaults to on)', async () => {
mockGetBotUserId.mockResolvedValue('bot-user-1');
mockGetAgentConfigForOwner.mockResolvedValue({ is_enabled: true, config: {} });

const response = await handlePullRequest(pullRequestPayload(), platformIntegration());

expect(response.status).toBe(202);
expect(mockCreateCodeReview).toHaveBeenCalled();
});
});

describe('automated council review type', () => {
const councilConfig = {
is_enabled: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import type { Owner } from '@/lib/code-reviews/core';
import { getBotUserId } from '@/lib/bot-users/bot-user-service';
import type { CodeReviewAgentConfig } from '@/lib/agent-config/core/types';
import type { GitHubAppType } from '@/lib/integrations/platforms/github/adapter';
import { resolveRepositorySettings } from '@/lib/integrations/github-repository-settings';
import { getRepositoryCustomization } from '@/lib/integrations/db/platform-integrations';
import {
addReactionToPR,
createCheckRun,
Expand Down Expand Up @@ -141,7 +143,7 @@ export async function handlePullRequestCodeReview(
// 3. Check if repository is in allowed list (when using selected repositories mode)
const config = agentConfig.config as CodeReviewAgentConfig;

// Bot PRs are skipped by default (enforced at step 5b). Compute the decision up front so the
// Bot PRs are skipped by default (enforced at step 5c). Compute the decision up front so the
// merge-commit path (step 4) also defers to it: otherwise a bot PR whose head is a merge commit
// would be re-pointed to a new SHA with a fresh check run, keeping alive a review the guardrail
// is meant to skip. When true, the PR falls through to cancellation + skip instead.
Expand Down Expand Up @@ -297,10 +299,35 @@ export async function handlePullRequestCodeReview(
);
}

// 5b. Feature-level guardrail: by default, skip automated reviews of bot-authored PRs
// 5b. Check the repository-level PR review toggle. This is the same
// `repository_customizations.pr_review_mode` setting (with installation-level
// default fallback) already resolved for the bot-mention model override —
// see `resolveRepositorySettings`/`resolveModelForGitHubRepository` in
// github-repository-settings.ts. Reusing it here keeps "reviews on/off" a
// single per-repo setting rather than a second, code-review-specific flag.
// Runs AFTER supersession (step 5) so an in-flight review is still cancelled and its
// check run resolved even if the setting was toggled off after the review started;
// otherwise the check run would stay open on the PR until the stale-review reaper runs.
const repositoryCustomization = await getRepositoryCustomization(
integration.id,
String(repository.id)
);
const { prReviewMode } = resolveRepositorySettings(integration, repositoryCustomization);

if (prReviewMode === 'off') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: Merge-commit synchronizes skip this toggle and can keep an in-flight review alive

Step 4 returns early for merge-commit synchronize events (e.g. GitHub "Update branch") before this check runs. Bot-authored PRs already compute their skip up front so that path cannot preserve a review the guardrail is meant to drop; prReviewMode === 'off' needs the same treatment.

If reviews are toggled off while a review is in flight and the next webhook is a merge-commit synchronize, migrateInFlightReviewsToMergeCommitHead re-points the review and opens a fresh check run instead of cancelling it.

Resolve prReviewMode before step 4 and treat 'off' like isBotPullRequestSkip so those events fall through to supersession and this skip.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a deliberate trade-off. If you do it the other way, then an old PR review would block any accepts until the reaping later.

TL;DR: If you toggle the setting mid-PR continuing any existing PR's is better than abandoning them

logExceptInTest(
`PR reviews disabled for repository ${repository.full_name} (ID: ${repository.id})`
);
return NextResponse.json(
{ message: 'PR reviews disabled for this repository' },
{ status: 200 }
);
}

// 5c. Feature-level guardrail: by default, skip automated reviews of bot-authored PRs
// (dependabot/renovate/etc.) — high-volume, low-value dependency bumps otherwise consume review
// compute and clutter the PR. Configurable per org via `skip_bot_pull_requests` (see the
// decision computed before step 4). Applies to standard and council reviews; manual reviews
// decision computed before step 3). Applies to standard and council reviews; manual reviews
// never reach this handler. Runs AFTER supersession (step 5) so a bot push still cancels any
// stale in-flight review and resolves its check run, instead of leaving it stuck.
if (isBotPullRequestSkip) {
Expand Down