diff --git a/src/queue/processors.ts b/src/queue/processors.ts index cf3751108..4b6b9d745 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -8284,9 +8284,18 @@ export function maybeAddReputationSkipHold( confirmedContributor: boolean; skipAiReview?: boolean | undefined; reputationSkipped: boolean; + /** True when this pass is a forced re-run, which bypasses the reputation skip -- see body. */ + forceAiReview?: boolean | undefined; }, ): boolean { if (!args.reputationSkipped || !shouldRequirePublicAiReviewForAdvisory(env, args)) return false; + // The hold must describe what HAPPENED, not what the heuristic wanted: aiReviewAllAuthors and a forced + // re-run (the panel checkbox / `@loopover review`) both bypass the reputation skip in + // shouldStartAiReviewForAdvisory, so on those passes the AI review genuinely runs. Pushing the "was + // skipped" finding anyway produced a published surface that asserted the review was skipped DIRECTLY + // ABOVE the review's own summary (observed live on JSONbored/loopover#9764) -- and, worse, held the gate + // for a review that succeeded. + if (args.settings.aiReviewAllAuthors || args.forceAiReview) return false; args.advisory.findings.push({ code: "ai_review_inconclusive", severity: "warning", @@ -11042,6 +11051,7 @@ async function maybePublishPrPublicSurface( confirmedContributor, skipAiReview: webhook.skipAiReview, reputationSkipped: preComputedReputationSkip === true, + forceAiReview: webhook.forceAiReview, }); // #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. diff --git a/src/review/reputation-wire.ts b/src/review/reputation-wire.ts index ed48387fb..772a757a2 100644 --- a/src/review/reputation-wire.ts +++ b/src/review/reputation-wire.ts @@ -127,6 +127,15 @@ export async function shouldSkipAiForReputation( // upgrade-only AMS bridge inside getEffectiveSubmitterReputation, then cadence (#4514) independently. const stats = await getEffectiveSubmitterReputation(env, { repoFullName: args.project, submitter: args.submitter }); if (shouldDowngradeToDeterministic(stats)) return true; + // A TRUSTED submitter is never cadence-downgraded. The machine-paced leg is a spend control against + // UNPROVEN accounts flooding paid review; "trusted" is this same module's own definition of proven (5+ + // recent merged successes at a low fail rate, recency-windowed -- see signalFromCounts). Without this, a + // maintainer running a legitimate high-velocity campaign (observed live: the repo OWNER, 50 submissions in + // 24h at a 7.2-minute median gap, 145 recent merges) had EVERY PR downgraded to deterministic-only and + // held for manual review -- the gate taxing exactly the contributor with the strongest earned record. An + // abuser cannot reach "trusted" cheaply: it requires real merged PRs on this repo inside the window, and + // any prompt-injection row hard-locks the signal to "low" regardless. + if (stats.signal === "trusted") return false; const cadence = await getSubmitterCadence(env, args.project, args.submitter ?? undefined); return isMachinePacedCadence(cadence); } diff --git a/test/unit/auto-review-wiring.test.ts b/test/unit/auto-review-wiring.test.ts index 3574d0002..ab99d2975 100644 --- a/test/unit/auto-review-wiring.test.ts +++ b/test/unit/auto-review-wiring.test.ts @@ -459,6 +459,44 @@ describe("review.auto_review wiring (#1954)", () => { ).toBe(false); }); + it("REGRESSION: no 'was skipped' hold on a FORCED re-run — the bypass means the review actually ran", () => { + // Observed live on JSONbored/loopover#9764: the published panel asserted "Required AI review was skipped + // by a submitter-reputation downgrade" DIRECTLY ABOVE the very AI summary that forced re-run produced, + // and held the gate for a review that succeeded. forceAiReview bypasses the reputation skip in + // shouldStartAiReviewForAdvisory, so the hold must describe what happened, not what the heuristic wanted. + 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 advisory = { headSha: "sha", findings: [] as unknown[] }; + const added = maybeAddReputationSkipHold(blockingEnv, { + settings, + advisory: advisory as never, + repoFullName: "acme/widgets", + author: "the-maintainer", + confirmedContributor: false, + reputationSkipped: true, + forceAiReview: true, + }); + expect(added).toBe(false); + expect(advisory.findings).toEqual([]); + }); + + it("REGRESSION: no hold under aiReviewAllAuthors either — that setting bypasses the skip the same way", () => { + const blockingEnv = { AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI: {} } as Env; + const settings = { gatePack: "oss-anti-slop", aiReviewMode: "block", aiReviewAllAuthors: true } as never; + const advisory = { headSha: "sha", findings: [] as unknown[] }; + expect( + maybeAddReputationSkipHold(blockingEnv, { + settings, + advisory: advisory as never, + repoFullName: "acme/widgets", + author: "anyone", + confirmedContributor: false, + reputationSkipped: true, + }), + ).toBe(false); + expect(advisory.findings).toEqual([]); + }); + it("#9015: a reputation skip HOLDS where blocking AI review is required — suspicion must never buy less scrutiny", () => { 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; diff --git a/test/unit/reputation-wiring.test.ts b/test/unit/reputation-wiring.test.ts index c30b49c11..a5882922d 100644 --- a/test/unit/reputation-wiring.test.ts +++ b/test/unit/reputation-wiring.test.ts @@ -339,6 +339,36 @@ describe("shouldSkipAiForReputation (helper)", () => { expect(await shouldSkipAiForReputation(env, { project: "acme/widgets", submitter: "speedster" })).toBe(true); }); + it("REGRESSION: a TRUSTED submitter is never cadence-downgraded, no matter how fast they pace", async () => { + // Observed live on the ORB: the repo OWNER, mid remediation campaign (50 PRs in 24h, 7.2-minute median + // gap, 145 recent merges), had EVERY PR downgraded to deterministic-only and held for manual review. + // The machine-paced leg is a spend control against UNPROVEN accounts; "trusted" is this module's own + // definition of proven (5+ recent merged successes, low fail rate), so it must short-circuit cadence. + const env = createTestEnv({ LOOPOVER_REVIEW_REPUTATION: "true" }); + // 6 merged outcomes in the window -> signal "trusted" (trustedMinSuccess=5, failRate 0). + for (let i = 0; i < 6; i++) { + await seedReviewTarget(env, { project: "acme/widgets", repo: "acme/widgets", number: 700 + i, installationId: 1, submitter: "the-maintainer", status: "merged", reasonCode: "success" }); + } + // ...and a blatantly machine-paced burst: 8 PRs at a 4-minute gap, all inside the cadence window. + const t0 = Date.now() - 60 * 60_000; + for (let i = 0; i < 8; i++) { + await seedCadencePullRequest(env, { number: 800 + i, submitter: "the-maintainer", createdAt: new Date(t0 + i * 4 * 60_000).toISOString() }); + } + expect(await shouldSkipAiForReputation(env, { project: "acme/widgets", submitter: "the-maintainer" })).toBe(false); + }); + + it("INVARIANT: the trusted exemption cannot mask genuine abuse — a prompt-injection row still downgrades", async () => { + // "trusted" is unreachable with a prompt-injection row in the window: signalFromCounts hard-locks the + // signal to "low" before any success counting. This pins that ordering, so the exemption above can + // never become a loophole where an abuser banks merges and then injects at machine pace. + const env = createTestEnv({ LOOPOVER_REVIEW_REPUTATION: "true" }); + for (let i = 0; i < 6; i++) { + await seedReviewTarget(env, { project: "acme/widgets", repo: "acme/widgets", number: 900 + i, installationId: 1, submitter: "sleeper", status: "merged", reasonCode: "success" }); + } + await seedReviewTarget(env, { project: "acme/widgets", repo: "acme/widgets", number: 990, installationId: 1, submitter: "sleeper", status: "closed", reasonCode: "source_prompt_injection" }); + expect(await shouldSkipAiForReputation(env, { project: "acme/widgets", submitter: "sleeper" })).toBe(true); + }); + it("FLAG-ON: false for the same number of submissions spread naturally over hours (comfortably human pace)", async () => { const env = createTestEnv({ LOOPOVER_REVIEW_REPUTATION: "true" }); const t0 = Date.now() - 20 * 60 * 60_000;