From 5b74d990d2b49ef6d61838140c3c9f71c0857bab Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 19:21:14 -0700 Subject: [PATCH] refactor(queue): extract AI-slop-advisory gating/orchestration into its own module Part of #4013's module-split sequence (step 4, after transient-locks.ts and signal-snapshot.ts): shouldCollectSlopEvidence, shouldRunSlopAiAdvisory, and runAiSlopForAdvisory move to src/queue/slop-detection.ts. Pure mechanical move, no behavior change -- a re-export shim keeps this file's own internal callers and existing test imports working unchanged. buildAiReviewDiff and buildSecretScanDiff move to src/review/review-diff.ts (their natural existing home -- both already wrapped buildUnifiedReviewDiff there) rather than staying in processors.ts, which would have made the new slop-detection.ts circularly import this file. Closed a genuine, previously-invisible coverage gap surfaced by isolating this code in its own small file: the BYOK declared-provider-matches-stored-key branch had no test exercising it either way, so two tests were added covering both the match and mismatch outcomes. --- src/queue/processors.ts | 232 +++--------------------------------- src/queue/slop-detection.ts | 183 ++++++++++++++++++++++++++++ src/review/review-diff.ts | 48 ++++++++ test/unit/ai-slop.test.ts | 63 ++++++++++ 4 files changed, 309 insertions(+), 217 deletions(-) create mode 100644 src/queue/slop-detection.ts diff --git a/src/queue/processors.ts b/src/queue/processors.ts index c670e966ed..51cc6d1bf1 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -44,8 +44,6 @@ import { countPublishedAiReviewHeads, putCachedAiReview, markAiReviewPublished, - getCachedAiSlopAdvisory, - putCachedAiSlopAdvisory, hasPublishedAiSlopAdvisory, getCachedLinkedIssueSatisfaction, getLatestPublishedLinkedIssueSatisfaction, @@ -285,7 +283,6 @@ import { queueSnapshotFromBinding, } from "../selfhost/queue-common"; import { aiReviewCacheInputFingerprint } from "../review/ai-review-cache-input"; -import { aiSlopCacheInputFingerprint } from "../review/ai-slop-cache-input"; import { linkedIssueSatisfactionCacheInputFingerprint } from "../review/linked-issue-satisfaction-cache-input"; import { AGENT_LABEL_NEEDS_REVIEW, @@ -352,7 +349,15 @@ import { type ContributorProfile, } from "../signals/engine"; import { isDuplicateClusterWinnerByClaim, resolveDuplicateClusterWinnerNumber } from "../signals/duplicate-winner"; -import { buildUnifiedReviewDiff, totalAddedLineCount } from "../review/review-diff"; +import { buildAiReviewDiff, buildSecretScanDiff, buildUnifiedReviewDiff, totalAddedLineCount } from "../review/review-diff"; +// #4013 step 4 (prep): buildAiReviewDiff/buildSecretScanDiff moved to review-diff.ts (a natural existing +// home -- both already wrapped buildUnifiedReviewDiff there) rather than staying here, since keeping them +// in this file would have made the new slop-detection.ts below circularly import this file just for +// buildAiReviewDiff. Re-exported so test/unit/linked-issue-satisfaction-run.test.ts, +// test/unit/ai-review-advisory.test.ts, test/unit/ai-slop.test.ts, and +// test/unit/patchless-secret-scan.test.ts's existing `import { ... } from "../../src/queue/processors"` +// keep working unchanged. +export { buildAiReviewDiff, buildSecretScanDiff } from "../review/review-diff"; import { estimateReviewEffort } from "../review/review-effort"; import { buildUnifiedCommentBody } from "../review/unified-comment-bridge"; import { isRetryableJobError, RetryableJobError } from "./retryable"; @@ -378,6 +383,12 @@ export { claimPrActuationLock, releasePrActuationLock } from "./transient-locks" // two files circularly dependent. import { generateSignalSnapshots, loadOpenQueueCounts } from "./signal-snapshot"; export { generateSignalSnapshots } from "./signal-snapshot"; +// #4013 step 4: same shim shape for the AI-slop-advisory gating/orchestration functions -- imported here +// for this file's own internal callers, and re-exported so test/unit/advisory-ai-routing-call-sites.test.ts, +// test/unit/ai-slop.test.ts, and test/unit/gate-check-policy.test.ts's existing +// `import { ... } from "../../src/queue/processors"` keeps working unchanged. +import { runAiSlopForAdvisory, shouldCollectSlopEvidence, shouldRunSlopAiAdvisory } from "./slop-detection"; +export { runAiSlopForAdvisory, shouldCollectSlopEvidence, shouldRunSlopAiAdvisory } from "./slop-detection"; import { isVisualPath } from "../review/visual/paths"; import { buildCapture, fetchShotContentBlock, hasSuccessfulBotCapture, resolveVisualRoutes, type CaptureRoute } from "../review/visual/capture"; import { @@ -408,7 +419,6 @@ import { buildSlopAssessment, type SlopBand, } from "../signals/slop"; -import { runGittensoryAiSlopAdvisory } from "../services/ai-slop"; import { runGittensoryLinkedIssueSatisfaction } from "../services/linked-issue-satisfaction-run"; import { decidePublicSurface } from "../signals/settings-preview"; import { @@ -6918,12 +6928,6 @@ export async function resolveLinkedIssueAdvisoryContext( return { linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue: !hasOpenReference }; } -export function shouldCollectSlopEvidence( - settings: Pick, -): boolean { - return settings.slopGateMode !== "off" || mergeReadinessGateEnabled(settings); -} - export async function shouldRefreshFilesForPreMergeChecks( env: Env, repoFullName: string, @@ -6934,12 +6938,6 @@ export async function shouldRefreshFilesForPreMergeChecks( return checks.some((check) => check.whenPaths.length > 0); } -export function shouldRunSlopAiAdvisory( - settings: Pick, -): boolean { - return settings.slopAiAdvisory && settings.slopGateMode !== "off"; -} - /** #one-shot-review-cadence: resolve the effective AI review re-trigger cadence. The per-repo * `review.auto_review.cadence` manifest field (`configuredCadence`, already resolved by * resolveReviewAutoReviewConfig) always wins when set; otherwise falls back to the operator's fleet-wide @@ -7118,53 +7116,6 @@ async function resolvePullRequestFilesForReview( return stored; } -/** Build a bounded unified-diff string from cached PR files for the AI reviewer. Caps total size so a - * huge PR cannot blow the model context or the neuron budget; each file's patch is taken from the raw - * GitHub file payload when present. */ -export function buildAiReviewDiff( - files: Awaited>, -): string { - // Source-first + hunk-aware + always-list-dropped-files (ported from reviewbot). The old blind 60k - // head-slice `break`-dropped whole files in stored order, so the file DEFINING a symbol could vanish - // while another referenced it → the model hallucinated "missing import / undefined symbol" (the #1528 - // class, which survived even with grounding on). (#accuracy-gap-1) - return buildUnifiedReviewDiff( - files.map((file) => ({ - path: file.path, - patch: - typeof file.payload?.patch === "string" - ? file.payload.patch - : undefined, - status: file.status, - additions: file.additions, - deletions: file.deletions, - })), - ); -} - -/** - * Build the complete inline patch corpus for deterministic secret scanning. Unlike {@link buildAiReviewDiff}, - * this is intentionally unbudgeted and does not reorder files or drop hunks: security controls must inspect - * every raw patch GitHub returned instead of the lossy AI-review prompt view. - * - * GitHub omits inline `patch` for binary/large files; {@link enrichSecretScanFilesWithPatchFallback} recovers - * scannable `+` lines for those files before this runs (see {@link maybeAddSecretLeakFinding}). - */ -export function buildSecretScanDiff( - files: Awaited>, -): string { - return files - .map((file) => { - const status = file.status ?? "modified"; - const header = `### ${file.path} (${status}) +${file.additions ?? 0}/-${file.deletions ?? 0}`; - const patch = - typeof file.payload?.patch === "string" ? file.payload.patch : ""; - return patch ? `${header}\n${patch}` : header; - }) - .join("\n\n") - .trim(); -} - /** * Run the opt-in AI maintainer review and fold it into the gate + panel. Mutates `advisory.findings` * with a dual-model consensus defect (when `aiReviewMode: block` and the free Workers-AI pair agrees with @@ -8073,159 +8024,6 @@ export async function maybeAddLockfileTamperFinding( } } -/** - * AI-assisted slop advisory (opt-in `slopAiAdvisory`). Appends at most one ADVISORY-only `ai_slop_advisory` - * finding to the advisory; NEVER touches slopRisk or the gate (only the deterministic core can block). The - * caller gates on `settings.slopAiAdvisory` and reuses the already-fetched changed files. Like the AI review - * path, it runs ONLY for confirmed contributors so an unconfirmed/untrusted PR author cannot spend either the - * shared Workers AI budget or the maintainer-paid BYOK quota. Fail-safe: any AI error is swallowed so the - * gate still finalizes. - * - * `commitThresholdReached` (#ai-slop-repeat-spend): mirrors `ai_review`'s OWN `auto_pause_after_reviewed_commits` - * cap (`isAutoReviewCommitThresholdReached`) — a PR that's already been reviewed this many times at essentially - * its current state stops getting a fresh slop advisory too. Before this, every sweep pass re-ran the FULL - * advisory regardless of how many times the SAME head had already been checked (only a headSha+prompt-fingerprint - * cache guarded re-spend, so a stale PR the sweep kept re-visiting paid for a fresh attempt on every pass). - */ -export async function runAiSlopForAdvisory( - env: Env, - args: { - // See runAiReviewForAdvisory's doc comment on this same field (#token-bleed-spend-gate) -- a paused repo - // must never reach the LLM call below, independent of settings.slopAiAdvisory. - mode: AgentActionMode; - settings: RepositorySettings; - advisory: Awaited>; - repoFullName: string; - pr: { number: number; title: string; body?: string | null | undefined }; - author: string | null; - files: Awaited>; - deterministicBand: SlopBand; - confirmedContributor: boolean; - commitThresholdReached: boolean; - }, -): Promise { - // Confirmed-contributor gate (matches runAiReviewForAdvisory): no AI spend — free OR BYOK — on a PR from - // an unconfirmed author. The deterministic slop core still ran for everyone; only the AI layer is gated. - if (args.mode === "paused" || !args.confirmedContributor || !args.advisory.headSha) return; - if (args.commitThresholdReached) { - await recordAuditEvent(env, { - eventType: "github_app.ai_slop_auto_review_skipped", - actor: args.author, - targetKey: `${args.repoFullName}#${args.pr.number}`, - outcome: "completed", - detail: "slop advisory paused (commit threshold); this head has already been reviewed enough times", - metadata: { repoFullName: args.repoFullName, headSha: args.advisory.headSha }, - }).catch( - /* v8 ignore next -- fail-safe: an audit write failure never blocks the handler */ - () => undefined, - ); - return; - } - try { - // BYOK (opt-in): reuse the repo's encrypted key + aiReviewByok flag — one BYOK key serves both AI - // features. A declared provider must match the stored key's provider, else skip BYOK (Workers-AI - // fallback). The contributor is already confirmed (early return above), so BYOK billing is authorized. - // The slop advisory stays advisory-only regardless of which model writes it. - const storedKey = args.settings.aiReviewByok - ? await getDecryptedRepositoryAiKey(env, args.repoFullName) - : null; - const providerKey = - storedKey && - (!args.settings.aiReviewProvider || - args.settings.aiReviewProvider === storedKey.provider) - ? { - provider: storedKey.provider, - key: storedKey.key, - model: args.settings.aiReviewModel ?? storedKey.model, - } - : null; - // #ai-slop-cache: repeated scheduled sweeps at an unchanged prompt reuse the stored result instead of - // re-spending up to 6 free-tier attempts (or a BYOK call) on every tick. The fingerprint includes the - // provider identity plus the prompt-shaping inputs that can drift for the same head SHA (PR edits, - // retarget/base-diff changes, or deterministic-band setting changes). - const aiSlopDiff = buildAiReviewDiff(args.files); - const inputFingerprint = await aiSlopCacheInputFingerprint({ - title: args.pr.title, - body: args.pr.body ?? null, - diff: aiSlopDiff, - deterministicBand: args.deterministicBand, - byok: Boolean(providerKey), - provider: providerKey?.provider, - model: providerKey?.model, - }); - const cachedSlop = await getCachedAiSlopAdvisory(env, args.repoFullName, args.pr.number, args.advisory.headSha, inputFingerprint).catch(() => null); - let result: Awaited>; - if (cachedSlop) { - result = { status: "ok", finding: cachedSlop.finding, band: cachedSlop.band as SlopBand | null, estimatedNeurons: cachedSlop.estimatedNeurons }; - incr("gittensory_ai_slop_cache_hit_total"); - await recordAuditEvent(env, { - eventType: "github_app.ai_slop_cache_hit", - actor: args.author, - targetKey: `${args.repoFullName}#${args.pr.number}`, - outcome: "completed", - detail: "reused a stored AI slop advisory instead of re-spending an LLM call", - /* v8 ignore next -- reached only past this function's own `!args.advisory.headSha` early return, so headSha is always truthy here; the `?? null` is a type-level fallback for an unreachable branch. */ - metadata: { repoFullName: args.repoFullName, headSha: args.advisory.headSha ?? null }, - }).catch(() => undefined); - } else { - incr("gittensory_ai_slop_cache_miss_total"); - await recordAuditEvent(env, { - eventType: "github_app.ai_slop_cache_miss", - actor: args.author, - targetKey: `${args.repoFullName}#${args.pr.number}`, - outcome: "completed", - detail: "no reusable stored AI slop advisory for this head+fingerprint; running a fresh advisory", - /* v8 ignore next -- reached only past this function's own `!args.advisory.headSha` early return, so headSha is always truthy here; the `?? null` is a type-level fallback for an unreachable branch. */ - metadata: { repoFullName: args.repoFullName, headSha: args.advisory.headSha ?? null }, - }).catch(() => undefined); - result = await runGittensoryAiSlopAdvisory(withAdvisoryAiEnv(env, args.settings.advisoryAiRouting?.slop === true), { - repoFullName: args.repoFullName, - prNumber: args.pr.number, - title: args.pr.title, - body: args.pr.body ?? undefined, - diff: aiSlopDiff, - actor: args.author, - deterministicBand: args.deterministicBand, - providerKey, - }); - // Only "ok" actually spent the LLM call (free-tier attempts or a BYOK call) — disabled/unavailable/ - // quota_exceeded all short-circuit BEFORE any provider call, so caching them would suppress a legitimate - // retry once the condition clears without having saved anything. - if (result.status === "ok") { - await putCachedAiSlopAdvisory(env, args.repoFullName, args.pr.number, args.advisory.headSha, inputFingerprint, { - status: result.status, - band: result.band, - finding: result.finding, - estimatedNeurons: result.estimatedNeurons, - }).catch((error) => { - incr("gittensory_ai_slop_cache_write_error_total"); - return recordAuditEvent(env, { - eventType: "github_app.ai_slop_cache_write_error", - actor: args.author, - targetKey: `${args.repoFullName}#${args.pr.number}`, - outcome: "error", - detail: errorMessage(error), - /* v8 ignore next -- reached only past this function's own `!args.advisory.headSha` early return, so headSha is always truthy here; the `?? null` is a type-level fallback for an unreachable branch. */ - metadata: { repoFullName: args.repoFullName, headSha: args.advisory.headSha ?? null }, - }).catch(() => undefined); - }); - } - } - if (result.status === "ok" && result.finding) - args.advisory.findings.push(result.finding); - } catch (error) { - console.error( - JSON.stringify({ - level: "warn", - event: "ai_slop_failed", - repository: args.repoFullName, - pullNumber: args.pr.number, - error: errorMessage(error), - }), - ); - } -} - /** * Run the linked-issue satisfaction assessment for advisory purposes (#1961/#3906) — opt-in via * `linkedIssueSatisfactionGateMode != "off"`. Assesses only the PR's PRIMARY (first) linked issue: v1 chooses diff --git a/src/queue/slop-detection.ts b/src/queue/slop-detection.ts new file mode 100644 index 0000000000..90fa863705 --- /dev/null +++ b/src/queue/slop-detection.ts @@ -0,0 +1,183 @@ +// AI-assisted slop advisory gating and orchestration (#4013 step 4 -- extracted from processors.ts, fourth +// step of the file's own module-split sequence, after transient-locks.ts, signal-snapshot.ts, and +// duplicate-detection.ts). Pure move. mergeReadinessGateEnabled (a trivial one-line predicate, also used by +// processors.ts's own shouldCollectLinkedIssueEvidence there) is inlined directly rather than imported back +// from processors.ts, for the same reason githubAdmissionKeyForToken was inlined in duplicate-detection.ts +// -- it would otherwise make the two files circularly dependent on each other for one line of logic. + +import { getCachedAiSlopAdvisory, getDecryptedRepositoryAiKey, type listPullRequestFiles, putCachedAiSlopAdvisory, recordAuditEvent } from "../db/repositories"; +import { buildPullRequestAdvisory } from "../rules/advisory"; +import { buildAiReviewDiff } from "../review/review-diff"; +import { aiSlopCacheInputFingerprint } from "../review/ai-slop-cache-input"; +import { withAdvisoryAiEnv } from "../selfhost/ai"; +import { incr } from "../selfhost/metrics"; +import { runGittensoryAiSlopAdvisory } from "../services/ai-slop"; +import type { AgentActionMode } from "../settings/agent-execution"; +import type { SlopBand } from "../signals/slop"; +import type { RepositorySettings } from "../types"; +import { errorMessage } from "../utils/json"; + +export function shouldCollectSlopEvidence( + settings: Pick, +): boolean { + return settings.slopGateMode !== "off" || settings.mergeReadinessGateMode !== "off"; +} + +export function shouldRunSlopAiAdvisory( + settings: Pick, +): boolean { + return settings.slopAiAdvisory && settings.slopGateMode !== "off"; +} + +/** + * AI-assisted slop advisory (opt-in `slopAiAdvisory`). Appends at most one ADVISORY-only `ai_slop_advisory` + * finding to the advisory; NEVER touches slopRisk or the gate (only the deterministic core can block). The + * caller gates on `settings.slopAiAdvisory` and reuses the already-fetched changed files. Like the AI review + * path, it runs ONLY for confirmed contributors so an unconfirmed/untrusted PR author cannot spend either the + * shared Workers AI budget or the maintainer-paid BYOK quota. Fail-safe: any AI error is swallowed so the + * gate still finalizes. + * + * `commitThresholdReached` (#ai-slop-repeat-spend): mirrors `ai_review`'s OWN `auto_pause_after_reviewed_commits` + * cap (`isAutoReviewCommitThresholdReached`) — a PR that's already been reviewed this many times at essentially + * its current state stops getting a fresh slop advisory too. Before this, every sweep pass re-ran the FULL + * advisory regardless of how many times the SAME head had already been checked (only a headSha+prompt-fingerprint + * cache guarded re-spend, so a stale PR the sweep kept re-visiting paid for a fresh attempt on every pass). + */ +export async function runAiSlopForAdvisory( + env: Env, + args: { + // See runAiReviewForAdvisory's doc comment on this same field (#token-bleed-spend-gate) -- a paused repo + // must never reach the LLM call below, independent of settings.slopAiAdvisory. + mode: AgentActionMode; + settings: RepositorySettings; + advisory: Awaited>; + repoFullName: string; + pr: { number: number; title: string; body?: string | null | undefined }; + author: string | null; + files: Awaited>; + deterministicBand: SlopBand; + confirmedContributor: boolean; + commitThresholdReached: boolean; + }, +): Promise { + // Confirmed-contributor gate (matches runAiReviewForAdvisory): no AI spend — free OR BYOK — on a PR from + // an unconfirmed author. The deterministic slop core still ran for everyone; only the AI layer is gated. + if (args.mode === "paused" || !args.confirmedContributor || !args.advisory.headSha) return; + if (args.commitThresholdReached) { + await recordAuditEvent(env, { + eventType: "github_app.ai_slop_auto_review_skipped", + actor: args.author, + targetKey: `${args.repoFullName}#${args.pr.number}`, + outcome: "completed", + detail: "slop advisory paused (commit threshold); this head has already been reviewed enough times", + metadata: { repoFullName: args.repoFullName, headSha: args.advisory.headSha }, + }).catch( + /* v8 ignore next -- fail-safe: an audit write failure never blocks the handler */ + () => undefined, + ); + return; + } + try { + // BYOK (opt-in): reuse the repo's encrypted key + aiReviewByok flag — one BYOK key serves both AI + // features. A declared provider must match the stored key's provider, else skip BYOK (Workers-AI + // fallback). The contributor is already confirmed (early return above), so BYOK billing is authorized. + // The slop advisory stays advisory-only regardless of which model writes it. + const storedKey = args.settings.aiReviewByok + ? await getDecryptedRepositoryAiKey(env, args.repoFullName) + : null; + const providerKey = + storedKey && + (!args.settings.aiReviewProvider || + args.settings.aiReviewProvider === storedKey.provider) + ? { + provider: storedKey.provider, + key: storedKey.key, + model: args.settings.aiReviewModel ?? storedKey.model, + } + : null; + // #ai-slop-cache: repeated scheduled sweeps at an unchanged prompt reuse the stored result instead of + // re-spending up to 6 free-tier attempts (or a BYOK call) on every tick. The fingerprint includes the + // provider identity plus the prompt-shaping inputs that can drift for the same head SHA (PR edits, + // retarget/base-diff changes, or deterministic-band setting changes). + const aiSlopDiff = buildAiReviewDiff(args.files); + const inputFingerprint = await aiSlopCacheInputFingerprint({ + title: args.pr.title, + body: args.pr.body ?? null, + diff: aiSlopDiff, + deterministicBand: args.deterministicBand, + byok: Boolean(providerKey), + provider: providerKey?.provider, + model: providerKey?.model, + }); + const cachedSlop = await getCachedAiSlopAdvisory(env, args.repoFullName, args.pr.number, args.advisory.headSha, inputFingerprint).catch(() => null); + let result: Awaited>; + if (cachedSlop) { + result = { status: "ok", finding: cachedSlop.finding, band: cachedSlop.band as SlopBand | null, estimatedNeurons: cachedSlop.estimatedNeurons }; + incr("gittensory_ai_slop_cache_hit_total"); + await recordAuditEvent(env, { + eventType: "github_app.ai_slop_cache_hit", + actor: args.author, + targetKey: `${args.repoFullName}#${args.pr.number}`, + outcome: "completed", + detail: "reused a stored AI slop advisory instead of re-spending an LLM call", + /* v8 ignore next -- reached only past this function's own `!args.advisory.headSha` early return, so headSha is always truthy here; the `?? null` is a type-level fallback for an unreachable branch. */ + metadata: { repoFullName: args.repoFullName, headSha: args.advisory.headSha ?? null }, + }).catch(() => undefined); + } else { + incr("gittensory_ai_slop_cache_miss_total"); + await recordAuditEvent(env, { + eventType: "github_app.ai_slop_cache_miss", + actor: args.author, + targetKey: `${args.repoFullName}#${args.pr.number}`, + outcome: "completed", + detail: "no reusable stored AI slop advisory for this head+fingerprint; running a fresh advisory", + /* v8 ignore next -- reached only past this function's own `!args.advisory.headSha` early return, so headSha is always truthy here; the `?? null` is a type-level fallback for an unreachable branch. */ + metadata: { repoFullName: args.repoFullName, headSha: args.advisory.headSha ?? null }, + }).catch(() => undefined); + result = await runGittensoryAiSlopAdvisory(withAdvisoryAiEnv(env, args.settings.advisoryAiRouting?.slop === true), { + repoFullName: args.repoFullName, + prNumber: args.pr.number, + title: args.pr.title, + body: args.pr.body ?? undefined, + diff: aiSlopDiff, + actor: args.author, + deterministicBand: args.deterministicBand, + providerKey, + }); + // Only "ok" actually spent the LLM call (free-tier attempts or a BYOK call) — disabled/unavailable/ + // quota_exceeded all short-circuit BEFORE any provider call, so caching them would suppress a legitimate + // retry once the condition clears without having saved anything. + if (result.status === "ok") { + await putCachedAiSlopAdvisory(env, args.repoFullName, args.pr.number, args.advisory.headSha, inputFingerprint, { + status: result.status, + band: result.band, + finding: result.finding, + estimatedNeurons: result.estimatedNeurons, + }).catch((error) => { + incr("gittensory_ai_slop_cache_write_error_total"); + return recordAuditEvent(env, { + eventType: "github_app.ai_slop_cache_write_error", + actor: args.author, + targetKey: `${args.repoFullName}#${args.pr.number}`, + outcome: "error", + detail: errorMessage(error), + /* v8 ignore next -- reached only past this function's own `!args.advisory.headSha` early return, so headSha is always truthy here; the `?? null` is a type-level fallback for an unreachable branch. */ + metadata: { repoFullName: args.repoFullName, headSha: args.advisory.headSha ?? null }, + }).catch(() => undefined); + }); + } + } + if (result.status === "ok" && result.finding) + args.advisory.findings.push(result.finding); + } catch (error) { + console.error( + JSON.stringify({ + level: "warn", + event: "ai_slop_failed", + repository: args.repoFullName, + pullNumber: args.pr.number, + error: errorMessage(error), + }), + ); + } +} diff --git a/src/review/review-diff.ts b/src/review/review-diff.ts index de965d541f..1c20e22dc5 100644 --- a/src/review/review-diff.ts +++ b/src/review/review-diff.ts @@ -7,6 +7,7 @@ // patches hunk-aware instead of dropping them, and always lists patch-less/over-budget files. (#accuracy-gap-1) import { isTestPath } from "../signals/test-evidence"; +import type { listPullRequestFiles } from "../db/repositories"; /** Char budget of the diff fed to the review models. The 120B review models have ~128k-token context, so * even a large PR fits in ONE coherent pass (accuracy over speed). Only a genuinely huge PR truncates — @@ -148,3 +149,50 @@ export function buildUnifiedReviewDiff(files: ReviewDiffFile[], budget: number = } return diff.trim(); } + +/** Build a bounded unified-diff string from cached PR files for the AI reviewer. Caps total size so a + * huge PR cannot blow the model context or the neuron budget; each file's patch is taken from the raw + * GitHub file payload when present. */ +export function buildAiReviewDiff( + files: Awaited>, +): string { + // Source-first + hunk-aware + always-list-dropped-files (ported from reviewbot). The old blind 60k + // head-slice `break`-dropped whole files in stored order, so the file DEFINING a symbol could vanish + // while another referenced it → the model hallucinated "missing import / undefined symbol" (the #1528 + // class, which survived even with grounding on). (#accuracy-gap-1) + return buildUnifiedReviewDiff( + files.map((file) => ({ + path: file.path, + patch: + typeof file.payload?.patch === "string" + ? file.payload.patch + : undefined, + status: file.status, + additions: file.additions, + deletions: file.deletions, + })), + ); +} + +/** + * Build the complete inline patch corpus for deterministic secret scanning. Unlike {@link buildAiReviewDiff}, + * this is intentionally unbudgeted and does not reorder files or drop hunks: security controls must inspect + * every raw patch GitHub returned instead of the lossy AI-review prompt view. + * + * GitHub omits inline `patch` for binary/large files; {@link enrichSecretScanFilesWithPatchFallback} recovers + * scannable `+` lines for those files before this runs (see {@link maybeAddSecretLeakFinding}). + */ +export function buildSecretScanDiff( + files: Awaited>, +): string { + return files + .map((file) => { + const status = file.status ?? "modified"; + const header = `### ${file.path} (${status}) +${file.additions ?? 0}/-${file.deletions ?? 0}`; + const patch = + typeof file.payload?.patch === "string" ? file.payload.patch : ""; + return patch ? `${header}\n${patch}` : header; + }) + .join("\n\n") + .trim(); +} diff --git a/test/unit/ai-slop.test.ts b/test/unit/ai-slop.test.ts index bbb33085fa..1ef4df9e7a 100644 --- a/test/unit/ai-slop.test.ts +++ b/test/unit/ai-slop.test.ts @@ -511,6 +511,69 @@ describe("runAiSlopForAdvisory (processor wiring)", () => { expect(run).not.toHaveBeenCalled(); }); + it("uses BYOK when settings.aiReviewProvider is explicitly set and matches the stored key's provider", async () => { + const run = vi.fn(async () => ({ response: slopJson({ band: "clean" }) })); // Workers AI must NOT be used + const env = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + TOKEN_ENCRYPTION_SECRET: "ai-slop-byok-test-encryption-secret-32b", + }); + await upsertRepositoryAiKey(env, { repoFullName: "acme/widgets", provider: "anthropic", key: "sk-ant-byok-slop-9999", model: null }); + const fetchMock = vi.fn(async (_url: RequestInfo | URL, _init?: RequestInit) => new Response(JSON.stringify({ content: [{ type: "text", text: slopJson({ band: "high" }) }] }), { status: 200 })); + vi.stubGlobal("fetch", fetchMock); + const adv = advisory(); + await runAiSlopForAdvisory(env, { + mode: "live", + settings: { aiReviewByok: true, aiReviewProvider: "anthropic" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + files, + deterministicBand: "elevated", + confirmedContributor: true, + commitThresholdReached: false, + }); + // A declared provider matching the stored key's provider still authorizes BYOK (same outcome as the + // no-declared-provider case above) — Workers AI is never called. + expect(adv.findings.map((f) => f.code)).toEqual([AI_SLOP_FINDING_CODE]); + expect(fetchMock.mock.calls[0]?.[0]).toBe("https://api.anthropic.com/v1/messages"); + expect(run).not.toHaveBeenCalled(); + }); + + it("falls back to Workers AI when settings.aiReviewProvider is set but does NOT match the stored key's provider", async () => { + const run = vi.fn(async () => ({ response: slopJson({ band: "clean" }) })); + const env = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + TOKEN_ENCRYPTION_SECRET: "ai-slop-byok-test-encryption-secret-32b", + }); + await upsertRepositoryAiKey(env, { repoFullName: "acme/widgets", provider: "anthropic", key: "sk-ant-byok-slop-9999", model: null }); + const fetchMock = vi.fn(async () => new Response("should never be called", { status: 500 })); + vi.stubGlobal("fetch", fetchMock); + const adv = advisory(); + await runAiSlopForAdvisory(env, { + mode: "live", + settings: { aiReviewByok: true, aiReviewProvider: "openai" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + files, + deterministicBand: "elevated", + confirmedContributor: true, + commitThresholdReached: false, + }); + // Declared provider ("openai") mismatches the stored key's provider ("anthropic") ⇒ BYOK is skipped and + // the free Workers AI path runs instead (the BYOK fetch endpoint is never hit). + expect(run).toHaveBeenCalled(); + expect(fetchMock).not.toHaveBeenCalled(); + }); + it("no-ops entirely for unconfirmed contributors — neither the maintainer BYOK key nor free Workers AI is spent", async () => { const run = vi.fn(async () => ({ response: slopJson({ band: "high" }) })); const env = createTestEnv({