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
40 changes: 40 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ import {
filterReviewFilesForAi,
resolvePullRequestAutoReviewSkipReason,
resolveAutoReviewSkipSummary,
isContributorControlledAutoReviewSkipReason,
resolveRepoEnrichmentToggles,
resolveReviewAutoReviewConfig,
resolveReviewPathInstructions,
Expand Down Expand Up @@ -6339,6 +6340,36 @@ export async function shouldStartAiReviewForAdvisory(
return !(isReputationEnabled(env) && isConvergenceRepoAllowed(env, args.repoFullName) && (await shouldSkipAiForReputation(env, { project: args.repoFullName, submitter: args.author })));
}

export function maybeAddRequiredAutoReviewSkipHold(
env: Env,
args: {
settings: RepositorySettings;
advisory: Pick<Awaited<ReturnType<typeof buildPullRequestAdvisory>>, "headSha" | "findings">;
repoFullName: string;
author: string | null;
confirmedContributor: boolean;
skipAiReview?: boolean | undefined;
autoReviewSkipReason: string | null;
},
): boolean {
if (
args.autoReviewSkipReason === null ||
!isContributorControlledAutoReviewSkipReason(args.autoReviewSkipReason) ||
!shouldRequirePublicAiReviewForAdvisory(env, args)
) {
return false;
}
args.advisory.findings.push({
code: "ai_review_inconclusive",
severity: "warning",
title: "Required AI review was skipped by contributor-controlled metadata",
detail:
"The repository requires blocking AI review, but review.auto_review matched the PR title or base branch. The gate is held for human review instead of passing automatically.",
action: "Run AI review with a trusted override or remove the contributor-controlled auto_review match before merging.",
});
return true;
}

export function shouldRequirePublicAiReviewForAdvisory(
env: Env,
args: {
Expand Down Expand Up @@ -8265,6 +8296,15 @@ async function maybePublishPrPublicSurface(
changedFilesSummaryEnabledForReview = deterministicReviewOverrides.changedFilesSummary;
effortScoreEnabledForReview = deterministicReviewOverrides.effortScore;
minFindingSeverityForReview = deterministicReviewOverrides.minFindingSeverity;
maybeAddRequiredAutoReviewSkipHold(env, {
settings,
advisory,
repoFullName,
author,
confirmedContributor,
skipAiReview: webhook.skipAiReview,
autoReviewSkipReason,
});
const aiReviewWillRun =
!authorBlacklisted &&
!isFrozenForManualReview &&
Expand Down
4 changes: 4 additions & 0 deletions src/signals/focus-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2373,6 +2373,10 @@ export const AUTO_REVIEW_SKIP_SUMMARY: Record<AutoReviewSkipReason, string> = {
"review paused (commit threshold)": "Published AI review count reached review.auto_review.auto_pause_after_reviewed_commits, so further AI review is paused.",
};

export function isContributorControlledAutoReviewSkipReason(skipReason: string): boolean {
return skipReason === "review skipped (WIP title)" || skipReason === "review skipped (base branch out of scope)";
}

export function resolveAutoReviewSkipSummary(skipReason: string): string {
if (Object.prototype.hasOwnProperty.call(AUTO_REVIEW_SKIP_SUMMARY, skipReason)) {
return AUTO_REVIEW_SKIP_SUMMARY[skipReason as AutoReviewSkipReason];
Expand Down
51 changes: 51 additions & 0 deletions test/unit/auto-review-wiring.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { describe, expect, it, vi } from "vitest";
import {
auditPullRequestAutoReviewSkip,
maybeAddRequiredAutoReviewSkipHold,
resolveAutoReviewSkipForPullRequest,
resolveReviewManifestForAiReview,
} from "../../src/queue/processors";
Expand Down Expand Up @@ -404,4 +405,54 @@ describe("review.auto_review wiring (#1954)", () => {

loadSpy.mockRestore();
});
it("holds instead of quietly skipping when contributor-controlled metadata suppresses required AI review", () => {
const advisory = { headSha: "sha", findings: [] };
const added = maybeAddRequiredAutoReviewSkipHold(
{ AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI: {} } as Env,
{
settings: { gatePack: "oss-anti-slop", aiReviewMode: "block", aiReviewAllAuthors: false } as any,
advisory,
repoFullName: "acme/widgets",
author: "alice",
confirmedContributor: false,
autoReviewSkipReason: "review skipped (WIP title)",
},
);

expect(added).toBe(true);
expect(advisory.findings).toEqual([
expect.objectContaining({
code: "ai_review_inconclusive",
severity: "warning",
title: "Required AI review was skipped by contributor-controlled metadata",
}),
]);

const trustedSkip = { headSha: "sha", findings: [] };
expect(
maybeAddRequiredAutoReviewSkipHold({ AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI: {} } as Env, {
settings: { gatePack: "oss-anti-slop", aiReviewMode: "block", aiReviewAllAuthors: false } as any,
advisory: trustedSkip,
repoFullName: "acme/widgets",
author: "alice",
confirmedContributor: false,
autoReviewSkipReason: "review skipped (label)",
}),
).toBe(false);
expect(trustedSkip.findings).toEqual([]);

const disabledAi = { headSha: "sha", findings: [] };
expect(
maybeAddRequiredAutoReviewSkipHold({ AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI: {} } as Env, {
settings: { gatePack: "oss-anti-slop", aiReviewMode: "off", aiReviewAllAuthors: false } as any,
advisory: disabledAi,
repoFullName: "acme/widgets",
author: "alice",
confirmedContributor: false,
autoReviewSkipReason: "review skipped (base branch out of scope)",
}),
).toBe(false);
expect(disabledAi.findings).toEqual([]);
});

});
10 changes: 10 additions & 0 deletions test/unit/focus-manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
evaluateAutoReviewSkipReason,
resolveAutoReviewSkipSummary,
AUTO_REVIEW_SKIP_SUMMARY,
isContributorControlledAutoReviewSkipReason,
resolveAutoReviewConfig,
resolveReviewPromptOverrides,
composeManifestReviewInstructions,
Expand Down Expand Up @@ -3191,6 +3192,15 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => {
expect(resolveAutoReviewSkipSummary("review skipped (unknown)")).toBe("review skipped (unknown)");
});

it("marks only contributor-controlled auto_review skip reasons as requiring a hold", () => {
expect(isContributorControlledAutoReviewSkipReason("review skipped (WIP title)")).toBe(true);
expect(isContributorControlledAutoReviewSkipReason("review skipped (base branch out of scope)")).toBe(true);
expect(isContributorControlledAutoReviewSkipReason("review skipped (draft)")).toBe(false);
expect(isContributorControlledAutoReviewSkipReason("review skipped (ignored author)")).toBe(false);
expect(isContributorControlledAutoReviewSkipReason("review skipped (label)")).toBe(false);
expect(isContributorControlledAutoReviewSkipReason("review skipped (unknown)")).toBe(false);
});

it("parses auto_pause_after_reviewed_commits with bounds validation (#2042)", () => {
const ok = parseFocusManifest({ review: { auto_review: { auto_pause_after_reviewed_commits: 3 } } });
expect(ok.review.autoReview.autoPauseAfterReviewedCommits).toBe(3);
Expand Down