diff --git a/src/queue/processors.ts b/src/queue/processors.ts index ef680f25b4..11430d7ba1 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -363,6 +363,7 @@ import { filterReviewFilesForAi, resolvePullRequestAutoReviewSkipReason, resolveAutoReviewSkipSummary, + isContributorControlledAutoReviewSkipReason, resolveRepoEnrichmentToggles, resolveReviewAutoReviewConfig, resolveReviewPathInstructions, @@ -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>, "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: { @@ -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 && diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index e96b52a971..35a14aeb53 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -2373,6 +2373,10 @@ export const AUTO_REVIEW_SKIP_SUMMARY: Record = { "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]; diff --git a/test/unit/auto-review-wiring.test.ts b/test/unit/auto-review-wiring.test.ts index 1bdd94e3d3..0d0d2c1ea7 100644 --- a/test/unit/auto-review-wiring.test.ts +++ b/test/unit/auto-review-wiring.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from "vitest"; import { auditPullRequestAutoReviewSkip, + maybeAddRequiredAutoReviewSkipHold, resolveAutoReviewSkipForPullRequest, resolveReviewManifestForAiReview, } from "../../src/queue/processors"; @@ -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([]); + }); + }); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 40178044e1..41760bf569 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -23,6 +23,7 @@ import { evaluateAutoReviewSkipReason, resolveAutoReviewSkipSummary, AUTO_REVIEW_SKIP_SUMMARY, + isContributorControlledAutoReviewSkipReason, resolveAutoReviewConfig, resolveReviewPromptOverrides, composeManifestReviewInstructions, @@ -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);