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
11 changes: 11 additions & 0 deletions src/queue/ai-review-orchestration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,9 @@ export function aiReviewAttemptFailedResult(
export async function shouldStartAiReviewForAdvisory(
env: Env,
args: {
// Threaded straight into shouldRequirePublicAiReviewForAdvisory's hard gate below -- a "paused" repo must
// never be told a review will run, matching runAiReviewForAdvisory's own mode==="paused" stop.
mode: AgentActionMode;
settings: RepositorySettings;
advisory: Pick<Awaited<ReturnType<typeof buildPullRequestAdvisory>>, "headSha">;
repoFullName: string;
Expand Down Expand Up @@ -352,6 +355,7 @@ export function resolveAiReviewableAuthor(
* exact reason whenever `aiReviewWillRun` ends up false for a cause none of the OTHER (already-audited) paths
* — author blacklist, manual-review freeze, auto-review skip, one-shot reuse, reputation skip — cover. */
export type PublicAiReviewGateSkipReason =
| "paused"
| "skip_ai_review_requested"
| "ai_review_mode_off"
| "author_not_reviewable"
Expand All @@ -363,6 +367,11 @@ export type PublicAiReviewGateSkipReason =
export function resolvePublicAiReviewGateSkipReason(
env: Env,
args: {
// #9692: evaluated FIRST, ahead of every other reason -- a paused repo's publish pass still computes the
// gate verdict (only GitHub writes are suppressed), so this predicate must independently know about the
// same stop runAiReviewForAdvisory applies as its own first check, or the "expected a review" flag built
// from this function drifts out of sync with reality on every paused pass.
mode: AgentActionMode;
settings: RepositorySettings;
advisory: Pick<Awaited<ReturnType<typeof buildPullRequestAdvisory>>, "headSha">;
repoFullName: string;
Expand All @@ -372,6 +381,7 @@ export function resolvePublicAiReviewGateSkipReason(
},
): PublicAiReviewGateSkipReason | null {
const reviewableAuthor = resolveAiReviewableAuthor(args.settings, args.confirmedContributor);
if (args.mode === "paused") return "paused";
if (args.skipAiReview) return "skip_ai_review_requested";
if (args.settings.aiReviewMode === "off") return "ai_review_mode_off";
if (!reviewableAuthor) return "author_not_reviewable";
Expand All @@ -385,6 +395,7 @@ export function resolvePublicAiReviewGateSkipReason(
export function shouldRequirePublicAiReviewForAdvisory(
env: Env,
args: {
mode: AgentActionMode;
settings: RepositorySettings;
advisory: Pick<Awaited<ReturnType<typeof buildPullRequestAdvisory>>, "headSha">;
repoFullName: string;
Expand Down
8 changes: 8 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8364,6 +8364,7 @@ async function resolvePullRequestFilesForReview(
export function maybeAddRequiredAutoReviewSkipHold(
env: Env,
args: {
mode: AgentActionMode;
settings: RepositorySettings;
advisory: Pick<Awaited<ReturnType<typeof buildPullRequestAdvisory>>, "headSha" | "findings">;
repoFullName: string;
Expand Down Expand Up @@ -8410,6 +8411,7 @@ export function maybeAddRequiredAutoReviewSkipHold(
export function maybeAddPromptInjectionHold(
env: Env,
args: {
mode: AgentActionMode;
settings: RepositorySettings;
advisory: Pick<Awaited<ReturnType<typeof buildPullRequestAdvisory>>, "headSha" | "findings">;
repoFullName: string;
Expand Down Expand Up @@ -8443,6 +8445,7 @@ export function maybeAddPromptInjectionHold(
export function maybeAddReputationSkipHold(
env: Env,
args: {
mode: AgentActionMode;
settings: RepositorySettings;
advisory: Pick<Awaited<ReturnType<typeof buildPullRequestAdvisory>>, "headSha" | "findings">;
repoFullName: string;
Expand Down Expand Up @@ -11204,6 +11207,7 @@ async function maybePublishPrPublicSurface(
// inline findings at the publish site below, mirroring findingCategories.
fixHandoffEnabledForReview = shouldEmitFixHandoff(env, repoFullName, reviewManifestForAutoReview?.review.fixHandoff ?? undefined);
maybeAddRequiredAutoReviewSkipHold(env, {
mode,
settings,
advisory,
repoFullName,
Expand All @@ -11227,6 +11231,7 @@ async function maybePublishPrPublicSurface(
// auto-merge, i.e. suspicion would BUY less scrutiny. Exact sibling of the contributor-controlled skip
// hold above; a no-op when the repo does not require blocking AI review, or when no skip fired.
maybeAddReputationSkipHold(env, {
mode,
settings,
advisory,
repoFullName,
Expand All @@ -11239,6 +11244,7 @@ async function maybePublishPrPublicSurface(
// #9035: a caught reviewer-manipulation attempt is evidence of intent, not a code-quality observation, and
// must not buy an automated decision. Same fail-closed shape as the two holds above.
maybeAddPromptInjectionHold(env, {
mode,
settings,
advisory,
repoFullName,
Expand Down Expand Up @@ -11268,6 +11274,7 @@ async function maybePublishPrPublicSurface(
!autoReviewSkipReason &&
!oneShotPriorReview &&
(await shouldStartAiReviewForAdvisory(env, {
mode,
settings,
advisory,
repoFullName,
Expand Down Expand Up @@ -11383,6 +11390,7 @@ async function maybePublishPrPublicSurface(
// the remaining silent branch: a forced pass that ends with no fresh review always now has exactly one of
// a reuse event, a reuse-unavailable event, a reputation-skip hold, or this event in the audit trail.
const publicGateSkipReason = resolvePublicAiReviewGateSkipReason(env, {
mode,
settings,
advisory,
repoFullName,
Expand Down
48 changes: 46 additions & 2 deletions test/unit/ai-review-advisory.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { afterEach, describe, expect, it, vi } from "vitest";
import { buildAiReviewDiff, claimAiReviewLock, runAiReviewForAdvisory, shouldStartAiReviewForAdvisory } from "../../src/queue/processors";
import { resolveAiReviewableAuthor } from "../../src/queue/ai-review-orchestration";
import { resolveAiReviewableAuthor, resolvePublicAiReviewGateSkipReason } from "../../src/queue/ai-review-orchestration";
import { BEST_REVIEW_MODELS, INCOHERENT_DIFF_ASSESSMENT } from "../../src/services/ai-review";
import * as posthogModule from "../../src/selfhost/posthog";
import { recordAiUsageEvent, upsertRepositoryAiKey } from "../../src/db/repositories";
Expand Down Expand Up @@ -144,7 +144,7 @@ describe("resolveAiReviewableAuthor invariant (#orb-ai-review-always-review)", (

describe("shouldStartAiReviewForAdvisory", () => {
const enabledEnv = () => aiEnv(async () => ({ response: notesOnlyJson() }));
const base = { settings: { aiReviewMode: "advisory", gatePack: "gittensor" } as RepositorySettings, advisory: advisory(), repoFullName: "acme/widgets", author: "alice", confirmedContributor: true };
const base = { mode: "live" as const, settings: { aiReviewMode: "advisory", gatePack: "gittensor" } as RepositorySettings, advisory: advisory(), repoFullName: "acme/widgets", author: "alice", confirmedContributor: true };

it("matches the AI review entry gates before the reviewing placeholder is posted", async () => {
await expect(shouldStartAiReviewForAdvisory(enabledEnv(), base)).resolves.toBe(true);
Expand Down Expand Up @@ -222,6 +222,50 @@ describe("shouldStartAiReviewForAdvisory", () => {
).resolves.toBe(false);
await expect(shouldStartAiReviewForAdvisory(env, { ...base, forceAiReview: true, skipAiReview: true })).resolves.toBe(false);
});

// #9692: a paused repo must never be told a review will run -- runAiReviewForAdvisory already refuses to
// spend on mode: "paused" as its own first check, and this predicate must agree, or the caller's "was a
// review expected" flag drifts out of sync with what actually happened (the root cause of the
// ai_review_public_summary_missing false alarm this issue fixes).
it("#9692: mode 'paused' never starts a review, even on an otherwise fully-eligible PR", async () => {
await expect(shouldStartAiReviewForAdvisory(enabledEnv(), { ...base, mode: "paused" })).resolves.toBe(false);
// forceAiReview bypasses only the reputation skip, never this hard gate.
await expect(shouldStartAiReviewForAdvisory(enabledEnv(), { ...base, mode: "paused", forceAiReview: true })).resolves.toBe(false);
});
});

describe("resolvePublicAiReviewGateSkipReason (#9692)", () => {
const gateEnv = () => aiEnv(async () => ({ response: notesOnlyJson() }));
const gateArgs = {
mode: "live" as const,
settings: { aiReviewMode: "advisory", gatePack: "gittensor" } as RepositorySettings,
advisory: advisory(),
repoFullName: "acme/widgets",
author: "alice",
confirmedContributor: true,
};

it("returns 'paused' for mode: 'paused' on an otherwise fully-eligible PR", () => {
expect(resolvePublicAiReviewGateSkipReason(gateEnv(), { ...gateArgs, mode: "paused" })).toBe("paused");
});

it("PRECEDENCE: 'paused' wins even when a different skip reason would also apply (skipAiReview / mode off)", () => {
expect(resolvePublicAiReviewGateSkipReason(gateEnv(), { ...gateArgs, mode: "paused", skipAiReview: true })).toBe("paused");
expect(
resolvePublicAiReviewGateSkipReason(gateEnv(), {
...gateArgs,
mode: "paused",
settings: { aiReviewMode: "off" } as RepositorySettings,
}),
).toBe("paused");
});

it("mode: 'live' or 'dry_run' falls through to the pre-existing reasons, never 'paused'", () => {
expect(resolvePublicAiReviewGateSkipReason(gateEnv(), { ...gateArgs, skipAiReview: true })).toBe("skip_ai_review_requested");
expect(resolvePublicAiReviewGateSkipReason(gateEnv(), { ...gateArgs, mode: "dry_run", skipAiReview: true })).toBe("skip_ai_review_requested");
expect(resolvePublicAiReviewGateSkipReason(gateEnv(), gateArgs)).toBeNull();
expect(resolvePublicAiReviewGateSkipReason(gateEnv(), { ...gateArgs, mode: "dry_run" })).toBeNull();
});
});

describe("runAiReviewForAdvisory", () => {
Expand Down
60 changes: 60 additions & 0 deletions test/unit/auto-review-wiring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ describe("review.auto_review wiring (#1954)", () => {
const advisory = { headSha: "sha", findings: [] as unknown[] };

const added = maybeAddPromptInjectionHold(blockingEnv, {
mode: "live",
settings,
advisory: advisory as never,
repoFullName: "acme/widgets",
Expand All @@ -434,6 +435,7 @@ describe("review.auto_review wiring (#1954)", () => {
const untouched = { headSha: "sha", findings: [] as unknown[] };
expect(
maybeAddPromptInjectionHold(blockingEnv, {
mode: "live",
settings,
advisory: untouched as never,
repoFullName: "acme/widgets",
Expand All @@ -449,6 +451,7 @@ describe("review.auto_review wiring (#1954)", () => {
const offAdvisory = { headSha: "sha", findings: [] as unknown[] };
expect(
maybeAddPromptInjectionHold(offEnv, {
mode: "live",
settings: { gatePack: "oss-anti-slop", aiReviewMode: "off" } as never,
advisory: offAdvisory as never,
repoFullName: "acme/widgets",
Expand All @@ -468,6 +471,7 @@ describe("review.auto_review wiring (#1954)", () => {
const settings = { gatePack: "oss-anti-slop", aiReviewMode: "block", aiReviewAllAuthors: false } as never;
const advisory = { headSha: "sha", findings: [] as unknown[] };
const added = maybeAddReputationSkipHold(blockingEnv, {
mode: "live",
settings,
advisory: advisory as never,
repoFullName: "acme/widgets",
Expand All @@ -486,6 +490,7 @@ describe("review.auto_review wiring (#1954)", () => {
const advisory = { headSha: "sha", findings: [] as unknown[] };
expect(
maybeAddReputationSkipHold(blockingEnv, {
mode: "live",
settings,
advisory: advisory as never,
repoFullName: "acme/widgets",
Expand All @@ -502,6 +507,7 @@ describe("review.auto_review wiring (#1954)", () => {
const settings = { gatePack: "oss-anti-slop", aiReviewMode: "block", aiReviewAllAuthors: false } as never;
const advisory = { headSha: "sha", findings: [] as unknown[] };
const added = maybeAddReputationSkipHold(blockingEnv, {
mode: "live",
settings,
advisory: advisory as never,
repoFullName: "acme/widgets",
Expand All @@ -517,6 +523,7 @@ describe("review.auto_review wiring (#1954)", () => {
const untouched = { headSha: "sha", findings: [] as unknown[] };
expect(
maybeAddReputationSkipHold(blockingEnv, {
mode: "live",
settings,
advisory: untouched as never,
repoFullName: "acme/widgets",
Expand All @@ -532,6 +539,7 @@ describe("review.auto_review wiring (#1954)", () => {
const reviewOff = { headSha: "sha", findings: [] as unknown[] };
expect(
maybeAddReputationSkipHold(blockingEnv, {
mode: "live",
settings: { gatePack: "oss-anti-slop", aiReviewMode: "off", aiReviewAllAuthors: false } as never,
advisory: reviewOff as never,
repoFullName: "acme/widgets",
Expand All @@ -548,6 +556,7 @@ describe("review.auto_review wiring (#1954)", () => {
const added = maybeAddRequiredAutoReviewSkipHold(
{ AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI: {} } as Env,
{
mode: "live",
settings: { gatePack: "oss-anti-slop", aiReviewMode: "block", aiReviewAllAuthors: false } as any,
advisory,
repoFullName: "acme/widgets",
Expand All @@ -569,6 +578,7 @@ describe("review.auto_review wiring (#1954)", () => {
const trustedSkip = { headSha: "sha", findings: [] };
expect(
maybeAddRequiredAutoReviewSkipHold({ AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI: {} } as Env, {
mode: "live",
settings: { gatePack: "oss-anti-slop", aiReviewMode: "block", aiReviewAllAuthors: false } as any,
advisory: trustedSkip,
repoFullName: "acme/widgets",
Expand All @@ -582,6 +592,7 @@ describe("review.auto_review wiring (#1954)", () => {
const disabledAi = { headSha: "sha", findings: [] };
expect(
maybeAddRequiredAutoReviewSkipHold({ AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI: {} } as Env, {
mode: "live",
settings: { gatePack: "oss-anti-slop", aiReviewMode: "off", aiReviewAllAuthors: false } as any,
advisory: disabledAi,
repoFullName: "acme/widgets",
Expand All @@ -593,4 +604,53 @@ describe("review.auto_review wiring (#1954)", () => {
expect(disabledAi.findings).toEqual([]);
});

// #9692: a paused repo suppresses these holds the same way "AI review off" already does -- nothing was
// expected to run this pass, so a skip is not a suppression worth holding for.
it("#9692: mode 'paused' suppresses every sibling hold, even when its own skip condition fired", () => {
const blockingEnv = { AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI: {} } as Env;
const settings = { gatePack: "oss-anti-slop", aiReviewMode: "block", aiReviewAllAuthors: false } as never;

const reputationAdvisory = { headSha: "sha", findings: [] as unknown[] };
expect(
maybeAddReputationSkipHold(blockingEnv, {
mode: "paused",
settings,
advisory: reputationAdvisory as never,
repoFullName: "acme/widgets",
author: "burst-farmer",
confirmedContributor: false,
reputationSkipped: true,
}),
).toBe(false);
expect(reputationAdvisory.findings).toEqual([]);

const injectionAdvisory = { headSha: "sha", findings: [] as unknown[] };
expect(
maybeAddPromptInjectionHold(blockingEnv, {
mode: "paused",
settings,
advisory: injectionAdvisory as never,
repoFullName: "acme/widgets",
author: "attacker",
confirmedContributor: false,
injectionDetected: true,
}),
).toBe(false);
expect(injectionAdvisory.findings).toEqual([]);

const autoReviewAdvisory = { headSha: "sha", findings: [] as unknown[] };
expect(
maybeAddRequiredAutoReviewSkipHold(blockingEnv, {
mode: "paused",
settings: settings as any,
advisory: autoReviewAdvisory as never,
repoFullName: "acme/widgets",
author: "alice",
confirmedContributor: false,
autoReviewSkipReason: "review skipped (WIP title)",
}),
).toBe(false);
expect(autoReviewAdvisory.findings).toEqual([]);
});

});
4 changes: 4 additions & 0 deletions test/unit/gate-integrity-invariants.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ describe("reputation-triggered and contributor-controlled AI-review skips hold i

const contributorAdvisory = advisoryStub();
const contributorHeld = maybeAddRequiredAutoReviewSkipHold(env, {
mode: "live",
settings,
advisory: contributorAdvisory as never,
repoFullName: "acme/widgets",
Expand All @@ -125,6 +126,7 @@ describe("reputation-triggered and contributor-controlled AI-review skips hold i

const reputationAdvisory = advisoryStub();
const reputationHeld = maybeAddReputationSkipHold(env, {
mode: "live",
settings,
advisory: reputationAdvisory as never,
repoFullName: "acme/widgets",
Expand All @@ -150,6 +152,7 @@ describe("reputation-triggered and contributor-controlled AI-review skips hold i
const contributorAdvisory = advisoryStub();
expect(
maybeAddRequiredAutoReviewSkipHold(env, {
mode: "live",
settings,
advisory: contributorAdvisory as never,
repoFullName: "acme/widgets",
Expand All @@ -163,6 +166,7 @@ describe("reputation-triggered and contributor-controlled AI-review skips hold i
const reputationAdvisory = advisoryStub();
expect(
maybeAddReputationSkipHold(env, {
mode: "live",
settings,
advisory: reputationAdvisory as never,
repoFullName: "acme/widgets",
Expand Down
Loading