From 80b1fa9d59fd3616863acd741fe2f39b50ff1cd0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 02:46:55 -0700 Subject: [PATCH 1/7] fix(selfhost): repair Postgres backup metrics and reputation writes --- docker-compose.yml | 2 +- prometheus/prometheus.yml | 3 +++ src/review/submitter-reputation.ts | 2 +- test/unit/reputation-wiring.test.ts | 25 ++++++++++++++++++- .../selfhost-observability-config.test.ts | 6 +++++ test/unit/submitter-reputation.test.ts | 9 ++++--- 6 files changed, 41 insertions(+), 6 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 01ae24a282..4fd838ec09 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -578,7 +578,7 @@ services: command: - /bin/sh - -c - - "sh /backup-metrics.sh" + - "apk add --no-cache busybox-extras >/dev/null 2>&1 && sh /backup-metrics.sh" healthcheck: test: ["CMD-SHELL", "wget -qO- http://127.0.0.1:9101/metrics | grep -q '^gittensory_backup_latest_timestamp_seconds'"] interval: 30s diff --git a/prometheus/prometheus.yml b/prometheus/prometheus.yml index 6608cdd059..b2715e8e2f 100644 --- a/prometheus/prometheus.yml +++ b/prometheus/prometheus.yml @@ -33,6 +33,9 @@ scrape_configs: # Backup freshness from the read-only backup-exporter sidecar. The metrics exist only when the backup # profile is active, which keeps backup alerts opt-in with the backup feature. - job_name: gittensory-backup + # The exporter is a tiny BusyBox httpd wrapper around Prometheus text output. Prometheus v3 rejects + # blank Content-Type responses unless the scrape protocol is explicit. + fallback_scrape_protocol: PrometheusText0.0.4 static_configs: - targets: ["backup-exporter:9101"] scrape_interval: 60s diff --git a/src/review/submitter-reputation.ts b/src/review/submitter-reputation.ts index edf373a800..82d4866c8c 100644 --- a/src/review/submitter-reputation.ts +++ b/src/review/submitter-reputation.ts @@ -208,7 +208,7 @@ export async function recordSubmissionOutcome(env: Env, project: string, submitt await storage(env) .prepare( `INSERT INTO submitter_stats (project, submitter, submissions, ${col}, last_seen) VALUES (?, ?, 1, 1, CURRENT_TIMESTAMP) - ON CONFLICT(project, submitter) DO UPDATE SET submissions = submissions + 1, ${col} = ${col} + 1, last_seen = CURRENT_TIMESTAMP`, + ON CONFLICT(project, submitter) DO UPDATE SET submissions = submitter_stats.submissions + 1, ${col} = submitter_stats.${col} + 1, last_seen = CURRENT_TIMESTAMP`, ) .bind(project, submitter) .run(); diff --git a/test/unit/reputation-wiring.test.ts b/test/unit/reputation-wiring.test.ts index efb22f9fc6..eeffa446e5 100644 --- a/test/unit/reputation-wiring.test.ts +++ b/test/unit/reputation-wiring.test.ts @@ -6,7 +6,7 @@ import { shouldDowngradeToDeterministic, shouldSkipAiForReputation, } from "../../src/review/reputation-wire"; -import { getSubmitterReputation } from "../../src/review/submitter-reputation"; +import { getSubmitterReputation, recordSubmissionOutcome } from "../../src/review/submitter-reputation"; import { evaluateGateCheck } from "../../src/rules/advisory"; import type { Advisory, RepositorySettings } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; @@ -303,6 +303,29 @@ describe("recordReputationOutcome + the 0046 submitter_stats migration", () => { expect(stats.closed).toBe(1); expect(stats.closeRate).toBeCloseTo(0.5, 5); }); + + it("REGRESSION: qualifies submitter_stats counters in the upsert update for Postgres", async () => { + let preparedSql = ""; + const env = { + DB: { + prepare: vi.fn((sql: string) => { + preparedSql = sql; + return { + bind: vi.fn(() => ({ + run: vi.fn(async () => ({})), + })), + }; + }), + }, + } as unknown as Env; + + await recordSubmissionOutcome(env, "acme/widgets", "alice", "merged"); + + expect(preparedSql).toContain("submissions = submitter_stats.submissions + 1"); + expect(preparedSql).toContain("merged = submitter_stats.merged + 1"); + expect(preparedSql).not.toContain("submissions = submissions + 1"); + expect(preparedSql).not.toContain("merged = merged + 1"); + }); }); describe("reputationOutcomeFromTerminalState (pure)", () => { diff --git a/test/unit/selfhost-observability-config.test.ts b/test/unit/selfhost-observability-config.test.ts index 1fa78d5d8f..43a7b35ec3 100644 --- a/test/unit/selfhost-observability-config.test.ts +++ b/test/unit/selfhost-observability-config.test.ts @@ -113,6 +113,11 @@ describe("self-host observability trace config", () => { "./scripts/backup-metrics.sh:/backup-metrics.sh:ro", ]), ); + expect(backupExporter.command).toEqual([ + "/bin/sh", + "-c", + "apk add --no-cache busybox-extras >/dev/null 2>&1 && sh /backup-metrics.sh", + ]); expect(backupExporter.healthcheck?.test).toEqual([ "CMD-SHELL", "wget -qO- http://127.0.0.1:9101/metrics | grep -q '^gittensory_backup_latest_timestamp_seconds'", @@ -126,6 +131,7 @@ describe("self-host observability trace config", () => { }), expect.objectContaining({ job_name: "gittensory-backup", + fallback_scrape_protocol: "PrometheusText0.0.4", static_configs: [{ targets: ["backup-exporter:9101"] }], }), ]), diff --git a/test/unit/submitter-reputation.test.ts b/test/unit/submitter-reputation.test.ts index 5383b0155b..80b72aa8f0 100644 --- a/test/unit/submitter-reputation.test.ts +++ b/test/unit/submitter-reputation.test.ts @@ -216,17 +216,20 @@ describe("recordSubmissionOutcome / getSubmitterReputation (D1, fail-safe)", () await recordSubmissionOutcome(mkEnv(), "p", "u", "merged"); expect(seen[0]).toContain(", merged, last_seen)"); - expect(seen[0]).toContain("merged = merged + 1"); + expect(seen[0]).toContain("submissions = submitter_stats.submissions + 1"); + expect(seen[0]).toContain("merged = submitter_stats.merged + 1"); seen.length = 0; await recordSubmissionOutcome(mkEnv(), "p", "u", "closed"); expect(seen[0]).toContain(", closed, last_seen)"); - expect(seen[0]).toContain("closed = closed + 1"); + expect(seen[0]).toContain("submissions = submitter_stats.submissions + 1"); + expect(seen[0]).toContain("closed = submitter_stats.closed + 1"); seen.length = 0; await recordSubmissionOutcome(mkEnv(), "p", "u", "manual"); expect(seen[0]).toContain(", manual, last_seen)"); - expect(seen[0]).toContain("manual = manual + 1"); + expect(seen[0]).toContain("submissions = submitter_stats.submissions + 1"); + expect(seen[0]).toContain("manual = submitter_stats.manual + 1"); }); it("recordSubmissionOutcome swallows a DB error fail-safe (logs, never throws)", async () => { From d9ae778d88e83d98675d7063fe2d309fd96b8aba Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 03:23:50 -0700 Subject: [PATCH 2/7] fix(review): prevent stale review cache and sweep starvation Key AI review cache reuse by the prompt, feature, and reviewer inputs that shape the result, so same-head stale cache entries from older review configuration are treated as misses. Let scheduled sweeps and backfills rely on GitHub REST admission instead of global queue backlog snapshots, so unrelated pending work cannot starve required review checks. Validation: npm run test:coverage; npx tsc --noEmit --pretty false; git diff --check. --- src/index.ts | 29 +---- src/queue/processors.ts | 151 ++++++++++++++---------- src/review/ai-review-cache-input.ts | 98 +++++++++++++++ test/unit/ai-review-cache-input.test.ts | 122 +++++++++++++++++++ test/unit/index.test.ts | 27 +++-- test/unit/queue.test.ts | 67 +++++++++-- 6 files changed, 383 insertions(+), 111 deletions(-) create mode 100644 src/review/ai-review-cache-input.ts create mode 100644 test/unit/ai-review-cache-input.test.ts diff --git a/src/index.ts b/src/index.ts index 2ed6eb1ab2..0e81aa57d3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -9,15 +9,12 @@ import { isRagEnabled } from "./review/rag-wire"; import { isSelfTuneEnabled } from "./review/selftune-wire"; import { isGitHubBudgetBackgroundJob, - queueSnapshotBacklog, - queueSnapshotFromBinding, scheduledEnqueueDelaySeconds, } from "./selfhost/queue-common"; import { isReviewExecutionJob, isSelfHostedReviewRuntime } from "./selfhost/review-runtime"; import type { JobMessage } from "./types"; const app = createApp(); -const REGATE_BACKPRESSURE_TYPES = ["agent-regate-pr", "agent-regate-sweep"] as const; export { RateLimiter }; @@ -105,26 +102,11 @@ async function enqueueScheduledJobs(env: Env, controller: ScheduledController): // tick (~2 min) retries, and after the bucket resets the sweep resumes. Webhooks never pre-yield. const jobs: JobMessage[] = []; const selfHostedReviews = isSelfHostedReviewRuntime(env); - const queueSnapshot = selfHostedReviews - ? await queueSnapshotFromBinding(env.JOBS).catch((error) => { - console.warn( - JSON.stringify({ - level: "warn", - event: "selfhost_queue_snapshot_failed", - error: error instanceof Error ? error.message : "unknown error", - }), - ); - return null; - }) - : null; - const regateBacklog = queueSnapshotBacklog(queueSnapshot, REGATE_BACKPRESSURE_TYPES); let sweepThrottledUntil: string | undefined; if (selfHostedReviews) { sweepThrottledUntil = await shouldWaitForGitHubRateLimit(env, MAINTENANCE_RESERVED_HEADROOM); if (sweepThrottledUntil) { console.log(JSON.stringify({ event: "regate_sweep_throttled", resetAt: sweepThrottledUntil })); - } else if (regateBacklog > 0) { - console.log(JSON.stringify({ event: "regate_sweep_backlog_deferred", backlog: regateBacklog })); } else { jobs.push({ type: "agent-regate-sweep", requestedBy: "schedule" }); } @@ -138,13 +120,12 @@ async function enqueueScheduledJobs(env: Env, controller: ScheduledController): // per-repo segment + per-PR detail sync — a large GitHub-budget consumer second only to the sweep. Gate it // behind the SAME maintenance headroom the sweep yields at, so when the shared REST budget is low the backfill // SKIPS this 30-min tick and hands the remaining budget to webhooks (which drive timely reviews); the next - // 30-min tick retries, and after the bucket resets the backfill resumes. The cheap single-call health jobs - // (repair-data-fidelity, refresh-installation-health) stay unconditional — they cost ~one call and keep - // installation/health state fresh even while the budget is reserved. - if (selfHostedReviews && !sweepThrottledUntil && regateBacklog === 0) { + // 30-min tick retries, and after the bucket resets the backfill resumes. Queue depth is deliberately not a + // suppressor here: unrelated pending work can stay nonzero for long periods, while rate admission on the + // queued jobs is the precise throttle. The cheap single-call health jobs (repair-data-fidelity, + // refresh-installation-health) stay unconditional. + if (selfHostedReviews && !sweepThrottledUntil) { jobs.push({ type: "backfill-registered-repos", requestedBy: "schedule", mode: isFullSyncWindow ? "full" : "light" }); - } else if (selfHostedReviews && regateBacklog > 0) { - console.log(JSON.stringify({ event: "backfill_backlog_deferred", backlog: regateBacklog })); } else if (selfHostedReviews) { console.log(JSON.stringify({ event: "backfill_throttled", resetAt: sweepThrottledUntil })); } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 7f32dfc58b..55a146863b 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -215,9 +215,10 @@ import { shouldWaitForGitHubRateLimit, } from "../github/rate-limit"; import { - queueSnapshotBacklog, - queueSnapshotFromBinding, -} from "../selfhost/queue-common"; + aiReviewCacheInputFingerprint, + aiReviewCacheInputMatches, + cacheMetadataForAiReviewInput, +} from "../review/ai-review-cache-input"; import { downgradeCloseToHold, downgradeMergeToHold, @@ -421,7 +422,6 @@ import { errorMessage, nowIso } from "../utils/json"; const OFFICIAL_MINER_DETECTION_TTL_MS = 5 * 60 * 1000; const OFFICIAL_MINER_DETECTION_UNAVAILABLE_TTL_MS = 60 * 1000; -const PER_PR_REGATE_BACKPRESSURE_TYPES = ["agent-regate-pr"] as const; const PR_PUBLIC_SURFACE_ACTIONS = new Set([ "opened", "reopened", @@ -1060,11 +1060,6 @@ async function fanOutAgentRegateSweepJobs( }); } -async function currentRegateBacklog(env: Env): Promise { - const snapshot = await queueSnapshotFromBinding(env.JOBS).catch(() => null); - return queueSnapshotBacklog(snapshot, PER_PR_REGATE_BACKPRESSURE_TYPES); -} - // Convergence (RAG / codebase index, flag GITTENSORY_REVIEW_RAG). The dispatch for the `rag-index-repo` job. // Caller already gated on isRagEnabled(env). // - No repoFullName → cron fan-out: enqueue one FULL re-index job per registered + cutover-allowlisted repo. @@ -1224,19 +1219,6 @@ async function sweepRepoRegate( }); return; } - const regateBacklog = requestedBy === "schedule" ? await currentRegateBacklog(env) : 0; - if (regateBacklog > 0) { - await recordAuditEvent(env, { - eventType: "agent.sweep.regate", - actor: "gittensory", - targetKey: repoFullName, - outcome: "queued", - detail: - "re-gate sweep deferred: prior scheduled re-gate work is still pending or processing", - metadata: { repoFullName, mode, deferred: true, regateBacklog }, - }); - return; - } const [repo, openPullRequests] = await Promise.all([ getRepository(env, repoFullName), listOpenPullRequests(env, repoFullName), @@ -5232,10 +5214,75 @@ async function maybePublishPrPublicSurface( agent: "dual-ai", }, async () => { - // #1 self-host AI-review cache: the LLM output for a PR changes only when the code (head SHA) or the review - // mode changes, so reuse a prior review for this exact (repo, pr, head SHA, mode) — a re-delivered webhook or - // the block-mode ~2-min re-gate sweep (which re-runs the AI for every open PR) need not re-spend the call. On - // self-host there is no AI gateway, so this is the only AI cache. The deterministic gate below still runs. + const reviewManifest = await loadRepoFocusManifest(env, repoFullName).catch(() => null); + // `.gittensory.yml` review.profile + review.path_instructions + review.exclude_paths (#review-profile / + // #review-path-instructions / #review-exclude-paths): resolve from the manifest (cached from settings + // resolution, so a cheap cache hit — no extra fetch) and thread them into the AI review. Profile shapes + // nitpickiness; path-instructions add per-path guidance; exclude-paths drop files from review. Absent ⇒ + // byte-identical prompt. Fail-safe to defaults on any read error (resolveReviewPromptOverrides). + const { + profile: reviewProfile, + inlineComments: reviewInlineComments, + pathInstructions: reviewPathInstructions, + instructions: manifestReviewInstructions, + excludePaths: reviewExcludePaths, + } = resolveReviewPromptOverrides(reviewManifest); + inlineCommentsEnabledForReview = shouldRequestInlineFindings( + env, + repoFullName, + reviewInlineComments, + ); + const reviewFilesForAi = await getReviewFiles(); + const changedPaths = reviewFilesForAi.map((file) => file.path); + // Per-repo review CONTEXT (#review-skills): fold the container-private review/AGENTS.md (or legacy + // review/CLAUDE.md) guide + the matching review/skills/*.md modules into the SAME review-instructions slot, + // so reviews follow each repo's conventions. + // Glob-gated for cost (only skills matching the changed files are injected); absent config dir ⇒ empty ⇒ + // byte-identical prompt. getReviewFiles() is memoized, so the second call reuses the loaded diff. + const reviewInstructions = + [ + manifestReviewInstructions, + composeRepoReviewContext( + await loadRepoReviewContext(repoFullName), + changedPaths, + ), + ] + .map((part) => part?.trim()) + .filter(Boolean) + .join("\n\n") || null; + const convergedRepoAllowed = isConvergenceRepoAllowed(env, repoFullName); + const inputFingerprint = await aiReviewCacheInputFingerprint({ + mode: settings.aiReviewMode, + byok: settings.aiReviewByok, + provider: settings.aiReviewProvider, + model: settings.aiReviewModel, + reviewerPlan: env.AI_REVIEW_PLAN, + profile: reviewProfile, + inlineComments: inlineCommentsEnabledForReview, + pathInstructions: reviewPathInstructions, + pathGuidance: resolveReviewPathInstructions( + reviewPathInstructions, + changedPaths, + ), + repoInstructions: reviewInstructions, + excludePaths: reviewExcludePaths, + changedPaths, + features: { + grounding: isGroundingEnabled(env) && convergedRepoAllowed, + rag: resolveConvergedFeature(env, reviewManifest, "rag", repoFullName), + enrichment: isEnrichmentEnabled(env) && convergedRepoAllowed, + reputation: resolveConvergedFeature( + env, + reviewManifest, + "reputation", + repoFullName, + ), + }, + }); + // #1 self-host AI-review cache: the LLM output for a PR changes only when the code (head SHA), review + // mode, reviewer plan, feature activation, or prompt-shaping inputs change. A re-delivered webhook or the + // block-mode re-gate sweep can reuse that exact review; stale same-head reviews from older private review + // instructions or feature config are intentionally treated as misses. The deterministic gate still runs. const cachedReview = await getCachedAiReview( env, repoFullName, @@ -5243,46 +5290,14 @@ async function maybePublishPrPublicSurface( advisory.headSha, settings.aiReviewMode, ).catch(() => null); - if (cachedReview && hasPublicReviewAssessment(cachedReview.notes)) { + if ( + cachedReview && + aiReviewCacheInputMatches(cachedReview.metadata, inputFingerprint) && + hasPublicReviewAssessment(cachedReview.notes) + ) { advisory.findings.push(...cachedReview.findings); aiReview = cachedReview; } else { - // `.gittensory.yml` review.profile + review.path_instructions + review.exclude_paths (#review-profile / - // #review-path-instructions / #review-exclude-paths): resolve from the manifest (cached from settings - // resolution, so a cheap cache hit — no extra fetch) and thread them into the AI review. Profile shapes - // nitpickiness; path-instructions add per-path guidance; exclude-paths drop files from review. Absent ⇒ - // byte-identical prompt. Fail-safe to defaults on any read error (resolveReviewPromptOverrides). - const { - profile: reviewProfile, - inlineComments: reviewInlineComments, - pathInstructions: reviewPathInstructions, - instructions: manifestReviewInstructions, - excludePaths: reviewExcludePaths, - } = resolveReviewPromptOverrides( - /* v8 ignore next -- fail-open manifest-read rejection is exercised in runAiReviewForAdvisory; this wrapper preserves the same fallback. */ - await loadRepoFocusManifest(env, repoFullName).catch(() => null), - ); - inlineCommentsEnabledForReview = shouldRequestInlineFindings( - env, - repoFullName, - reviewInlineComments, - ); - // Per-repo review CONTEXT (#review-skills): fold the container-private review/AGENTS.md (or legacy - // review/CLAUDE.md) guide + the matching review/skills/*.md modules into the SAME review-instructions slot, - // so reviews follow each repo's conventions. - // Glob-gated for cost (only skills matching the changed files are injected); absent config dir ⇒ empty ⇒ - // byte-identical prompt. getReviewFiles() is memoized, so the second call reuses the loaded diff. - const reviewInstructions = - [ - manifestReviewInstructions, - composeRepoReviewContext( - await loadRepoReviewContext(repoFullName), - (await getReviewFiles()).map((file) => file.path), - ), - ] - .map((part) => part?.trim()) - .filter(Boolean) - .join("\n\n") || null; aiReview = await runAiReviewForAdvisory(env, { settings, advisory, @@ -5291,7 +5306,7 @@ async function maybePublishPrPublicSurface( pr: { ...pr, baseSha: webhook.baseSha ?? null }, author, confirmedContributor, - files: await getReviewFiles(), + files: reviewFilesForAi, reviewProfile, reviewPathInstructions, reviewInstructions, @@ -5305,7 +5320,13 @@ async function maybePublishPrPublicSurface( pr.number, advisory.headSha, settings.aiReviewMode, - aiReview, + { + ...aiReview, + metadata: cacheMetadataForAiReviewInput( + aiReview.metadata, + inputFingerprint, + ), + }, ).catch(() => undefined); } }, diff --git a/src/review/ai-review-cache-input.ts b/src/review/ai-review-cache-input.ts new file mode 100644 index 0000000000..bf29c444d1 --- /dev/null +++ b/src/review/ai-review-cache-input.ts @@ -0,0 +1,98 @@ +import type { + ReviewPathInstruction, + ReviewProfile, +} from "../signals/focus-manifest"; +import { sha256Hex } from "../utils/crypto"; + +export const AI_REVIEW_CACHE_INPUT_VERSION = "ai-review-input:v1"; + +export type AiReviewCacheInput = { + mode: string; + byok: boolean; + provider: string | null | undefined; + model: string | null | undefined; + reviewerPlan: + | { + combine?: string | null | undefined; + reviewers?: readonly { model?: string | null | undefined }[] | undefined; + } + | null + | undefined; + profile: ReviewProfile | null | undefined; + inlineComments: boolean; + pathInstructions: readonly ReviewPathInstruction[]; + pathGuidance: string; + repoInstructions: string | null | undefined; + excludePaths: readonly string[]; + changedPaths: readonly string[]; + features: { + grounding: boolean; + rag: boolean; + enrichment: boolean; + reputation: boolean; + }; +}; + +export async function aiReviewCacheInputFingerprint(input: AiReviewCacheInput): Promise { + const payload = { + version: AI_REVIEW_CACHE_INPUT_VERSION, + mode: input.mode, + byok: input.byok, + provider: input.provider ?? null, + model: input.model ?? null, + reviewerPlan: input.reviewerPlan + ? { + combine: input.reviewerPlan.combine ?? null, + reviewers: (input.reviewerPlan.reviewers ?? []).map((reviewer) => reviewer.model ?? null), + } + : null, + profile: input.profile ?? null, + inlineComments: input.inlineComments, + pathInstructions: input.pathInstructions.map((instruction) => ({ + path: instruction.path, + instructions: instruction.instructions, + })), + pathGuidance: input.pathGuidance, + repoInstructions: input.repoInstructions?.trim() || null, + excludePaths: normalizeStringList(input.excludePaths), + changedPaths: normalizeStringList(input.changedPaths), + features: input.features, + }; + return `${AI_REVIEW_CACHE_INPUT_VERSION}:${await sha256Hex(stableStringify(payload))}`; +} + +export function aiReviewCacheInputMatches( + metadata: Record | null | undefined, + fingerprint: string, +): boolean { + return ( + metadata?.inputVersion === AI_REVIEW_CACHE_INPUT_VERSION && + metadata.inputFingerprint === fingerprint + ); +} + +export function cacheMetadataForAiReviewInput( + metadata: Record | null | undefined, + fingerprint: string, +): Record { + return { + ...(metadata ?? {}), + inputVersion: AI_REVIEW_CACHE_INPUT_VERSION, + inputFingerprint: fingerprint, + }; +} + +function normalizeStringList(values: readonly string[]): string[] { + return [...new Set(values.map((value) => value.trim()).filter(Boolean))].sort(); +} + +function stableStringify(value: unknown): string { + if (Array.isArray(value)) return `[${value.map(stableStringify).join(",")}]`; + if (value && typeof value === "object") { + return `{${Object.entries(value) + .sort(([left], [right]) => left.localeCompare(right)) + .map(([key, nested]) => `${JSON.stringify(key)}:${stableStringify(nested)}`) + .join(",")}}`; + } + return JSON.stringify(value); +} diff --git a/test/unit/ai-review-cache-input.test.ts b/test/unit/ai-review-cache-input.test.ts new file mode 100644 index 0000000000..589a6616c9 --- /dev/null +++ b/test/unit/ai-review-cache-input.test.ts @@ -0,0 +1,122 @@ +import { + AI_REVIEW_CACHE_INPUT_VERSION, + aiReviewCacheInputFingerprint, + aiReviewCacheInputMatches, + cacheMetadataForAiReviewInput, + type AiReviewCacheInput, +} from "../../src/review/ai-review-cache-input"; + +const baseInput = (): AiReviewCacheInput => ({ + mode: "block", + byok: false, + provider: null, + model: null, + reviewerPlan: null, + profile: null, + inlineComments: false, + pathInstructions: [], + pathGuidance: "", + repoInstructions: null, + excludePaths: [], + changedPaths: ["src/a.ts"], + features: { + grounding: false, + rag: false, + enrichment: false, + reputation: false, + }, +}); + +describe("aiReviewCacheInputFingerprint", () => { + it("is stable across irrelevant path ordering and whitespace normalization", async () => { + const left = await aiReviewCacheInputFingerprint({ + ...baseInput(), + changedPaths: [" src/b.ts ", "src/a.ts", "src/a.ts"], + excludePaths: ["dist/**", " **/*.lock "], + repoInstructions: " Follow the repo guide. ", + }); + const right = await aiReviewCacheInputFingerprint({ + ...baseInput(), + changedPaths: ["src/a.ts", "src/b.ts"], + excludePaths: ["**/*.lock", "dist/**"], + repoInstructions: "Follow the repo guide.", + }); + + expect(left).toBe(right); + expect(left.startsWith(`${AI_REVIEW_CACHE_INPUT_VERSION}:`)).toBe(true); + }); + + it("changes when prompt-affecting review inputs change", async () => { + const original = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewerPlan: { combine: "consensus", reviewers: [{ model: "a" }, { model: "b" }] }, + pathInstructions: [{ path: "src/**", instructions: "Be strict." }], + pathGuidance: "Be strict.", + features: { ...baseInput().features, rag: true }, + }); + const updated = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewerPlan: { combine: "consensus", reviewers: [{ model: "a" }, { model: "c" }] }, + pathInstructions: [{ path: "src/**", instructions: "Be strict." }], + pathGuidance: "Be strict.", + features: { ...baseInput().features, rag: true }, + }); + + expect(updated).not.toBe(original); + }); + + it("normalizes sparse reviewer plan fields deterministically", async () => { + const omittedReviewers = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewerPlan: {}, + }); + const explicitEmpty = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewerPlan: { combine: null, reviewers: [] }, + }); + const sparse = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewerPlan: { reviewers: [{}] }, + }); + const explicit = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewerPlan: { combine: null, reviewers: [{ model: null }] }, + }); + + expect(omittedReviewers).toBe(explicitEmpty); + expect(sparse).toBe(explicit); + }); +}); + +describe("aiReviewCacheInputMatches", () => { + it("requires both the current input version and exact fingerprint", async () => { + const fingerprint = await aiReviewCacheInputFingerprint(baseInput()); + expect( + aiReviewCacheInputMatches( + { inputVersion: AI_REVIEW_CACHE_INPUT_VERSION, inputFingerprint: fingerprint }, + fingerprint, + ), + ).toBe(true); + expect(aiReviewCacheInputMatches(undefined, fingerprint)).toBe(false); + expect(aiReviewCacheInputMatches({ inputFingerprint: fingerprint }, fingerprint)).toBe(false); + expect( + aiReviewCacheInputMatches( + { inputVersion: AI_REVIEW_CACHE_INPUT_VERSION, inputFingerprint: "different" }, + fingerprint, + ), + ).toBe(false); + }); + + it("adds cache input metadata without discarding existing review telemetry", async () => { + const fingerprint = await aiReviewCacheInputFingerprint(baseInput()); + expect(cacheMetadataForAiReviewInput(null, fingerprint)).toEqual({ + inputVersion: AI_REVIEW_CACHE_INPUT_VERSION, + inputFingerprint: fingerprint, + }); + expect(cacheMetadataForAiReviewInput({ rag: { injected: true } }, fingerprint)).toEqual({ + rag: { injected: true }, + inputVersion: AI_REVIEW_CACHE_INPUT_VERSION, + inputFingerprint: fingerprint, + }); + }); +}); diff --git a/test/unit/index.test.ts b/test/unit/index.test.ts index 3bc8c91859..34d29f1ff5 100644 --- a/test/unit/index.test.ts +++ b/test/unit/index.test.ts @@ -277,20 +277,24 @@ describe("worker entrypoint", () => { expect(sent).toEqual([{ type: "agent-regate-sweep", requestedBy: "schedule" }]); }); - it("does not enqueue scheduled sweep/backfill work while prior regate jobs are still queued", async () => { + it("keeps enqueueing scheduled sweeps while prior regate jobs are queued", async () => { const sent: Array = []; + let snapshotCalled = false; const env = createTestEnv({ JOBS: { async send(message: import("../../src/types").JobMessage) { sent.push(message); }, - snapshot: async () => ({ - totals: { pending: 2, processing: 1, dead: 0, due: 2 }, - byType: [ - { type: "agent-regate-pr", status: "pending", count: 2, due: 2 }, - { type: "agent-regate-sweep", status: "processing", count: 1, due: 0 }, - ], - }), + snapshot: async () => { + snapshotCalled = true; + return { + totals: { pending: 2, processing: 1, dead: 0, due: 2 }, + byType: [ + { type: "agent-regate-pr", status: "pending", count: 2, due: 2 }, + { type: "agent-regate-sweep", status: "processing", count: 1, due: 0 }, + ], + }; + }, } as unknown as Queue, }); const waitUntil: Promise[] = []; @@ -299,12 +303,15 @@ describe("worker entrypoint", () => { await Promise.all(waitUntil); expect(sent).toEqual([ + { type: "agent-regate-sweep", requestedBy: "schedule" }, + { type: "backfill-registered-repos", requestedBy: "schedule", mode: "light" }, { type: "repair-data-fidelity", requestedBy: "schedule" }, { type: "refresh-installation-health", requestedBy: "schedule" }, ]); + expect(snapshotCalled).toBe(false); }); - it("fails open when queue introspection is unavailable so scheduled maintenance still runs", async () => { + it("does not require queue introspection for regular review sweep scheduling", async () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); const sent: Array = []; const env = createTestEnv({ @@ -323,7 +330,7 @@ describe("worker entrypoint", () => { await Promise.all(waitUntil); expect(sent).toEqual([{ type: "agent-regate-sweep", requestedBy: "schedule" }]); - expect(warn).toHaveBeenCalledWith(expect.stringContaining("selfhost_queue_snapshot_failed")); + expect(warn).not.toHaveBeenCalledWith(expect.stringContaining("selfhost_queue_snapshot_failed")); }); it("does not enqueue review sweeps from a broker-only Cloudflare runtime", async () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index ec69054c93..0f86a61ee5 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -45,6 +45,10 @@ import { putCachedAiReview, } from "../../src/db/repositories"; import { agentMaintenanceHeadMatchesGate, changedPathsForGuardrail, contributorEvidenceBatchSize, processJob } from "../../src/queue/processors"; +import { + aiReviewCacheInputFingerprint, + cacheMetadataForAiReviewInput, +} from "../../src/review/ai-review-cache-input"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { normalizeRegistryPayload } from "../../src/registry/normalize"; import { persistRegistrySnapshot } from "../../src/registry/sync"; @@ -811,7 +815,7 @@ describe("queue processors", () => { expect(JSON.parse(audit?.metadata_json ?? "{}")).toMatchObject({ repoFullName: "owner/agent-repo", examined: 1 }); }); - it("agent re-gate sweep runs blocking AI review before auto-maintenance (regression)", async () => { + it("agent re-gate sweep ignores stale same-head AI cache inputs before auto-maintenance (regression)", async () => { let aiCalls = 0; const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), @@ -839,6 +843,11 @@ describe("queue processors", () => { }); await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 7, title: "Stale PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [], body: "Closes #1" }); await upsertPullRequestFile(env, { repoFullName: "owner/agent-repo", pullNumber: 7, path: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, payload: { patch: "@@\n+export const ok = value.length;" } }); + await putCachedAiReview(env, "owner/agent-repo", 7, "a7", "block", { + notes: "stale cached review from older review inputs", + reviewerCount: 2, + findings: [{ code: "ai_review_split", severity: "critical", title: "Old cache", detail: "Old prompt inputs." }], + }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { const url = input.toString(); if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); @@ -1541,10 +1550,31 @@ describe("queue processors", () => { await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 7, title: "Stale PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [], body: "Closes #1" }); await upsertPullRequestFile(env, { repoFullName: "owner/agent-repo", pullNumber: 7, path: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, payload: { patch: "@@\n+export const ok = value.length;" } }); // Pre-seed the AI review for this exact head SHA + mode → the sweep's block-mode review must reuse it, not re-run. + const inputFingerprint = await aiReviewCacheInputFingerprint({ + mode: "block", + byok: false, + provider: null, + model: null, + reviewerPlan: env.AI_REVIEW_PLAN, + profile: null, + inlineComments: false, + pathInstructions: [], + pathGuidance: "", + repoInstructions: null, + excludePaths: [], + changedPaths: ["src/a.ts"], + features: { + grounding: false, + rag: false, + enrichment: false, + reputation: false, + }, + }); await putCachedAiReview(env, "owner/agent-repo", 7, "a7", "block", { notes: "cached review", reviewerCount: 2, findings: [{ code: "ai_consensus_defect", severity: "critical", title: "Cached defect", detail: "Cached critical defect." }], + metadata: cacheMetadataForAiReviewInput(null, inputFingerprint), }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { const url = input.toString(); @@ -2378,17 +2408,21 @@ describe("queue processors", () => { expect(JSON.parse(audit?.metadata_json ?? "{}")).toMatchObject({ deferred: true }); }); - it("REGRESSION: a scheduled repo sweep does not fan out more per-PR regates while prior regate work is queued", async () => { + it("REGRESSION: a scheduled repo sweep still fans out when unrelated per-PR regate work is queued", async () => { const sent: import("../../src/types").JobMessage[] = []; + let snapshotCalled = false; const env = createTestEnv({ JOBS: { async send(m: import("../../src/types").JobMessage) { sent.push(m); }, - snapshot: async () => ({ - totals: { pending: 1, processing: 0, dead: 0, due: 1 }, - byType: [{ type: "agent-regate-pr", status: "pending", count: 1, due: 1 }], - }), + snapshot: async () => { + snapshotCalled = true; + return { + totals: { pending: 1, processing: 0, dead: 0, due: 1 }, + byType: [{ type: "agent-regate-pr", status: "pending", count: 1, due: 1 }], + }; + }, } as unknown as Queue, }); await upsertInstallation(env, { action: "created", installation: { id: 9201, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] } }); @@ -2401,12 +2435,21 @@ describe("queue processors", () => { await processJob(env, { type: "agent-regate-sweep", requestedBy: "schedule", repoFullName: "owner/agent-repo" }); - expect(sent.filter((m) => m.type === "agent-regate-pr")).toEqual([]); - expect(getRepo).not.toHaveBeenCalled(); - expect(listOpen).not.toHaveBeenCalled(); + expect(sent.filter((m) => m.type === "agent-regate-pr")).toEqual([ + expect.objectContaining({ + type: "agent-regate-pr", + deliveryId: "regate-sweep:owner/agent-repo#7", + repoFullName: "owner/agent-repo", + prNumber: 7, + installationId: 9201, + }), + ]); + expect(getRepo).toHaveBeenCalled(); + expect(listOpen).toHaveBeenCalled(); + expect(snapshotCalled).toBe(false); const audit = await env.DB.prepare("select outcome, metadata_json from audit_events where event_type = ?").bind("agent.sweep.regate").first<{ outcome: string; metadata_json: string }>(); - expect(audit?.outcome).toBe("queued"); - expect(JSON.parse(audit?.metadata_json ?? "{}")).toMatchObject({ deferred: true, regateBacklog: 1 }); + expect(audit?.outcome).toBe("completed"); + expect(JSON.parse(audit?.metadata_json ?? "{}")).toMatchObject({ examined: 1 }); }); it("REGRESSION: a scheduled repo sweep ignores sweep rows when deciding per-PR regate backlog", async () => { @@ -2441,7 +2484,7 @@ describe("queue processors", () => { ]); }); - it("INVARIANT: a scheduled repo sweep fails open when queue introspection throws", async () => { + it("INVARIANT: a scheduled repo sweep does not require queue introspection", async () => { const sent: import("../../src/types").JobMessage[] = []; const env = createTestEnv({ JOBS: { From 944822f4889557b06e05c7fc4fcc94a7ba5f5439 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 05:57:38 -0700 Subject: [PATCH 3/7] fix(review): fingerprint the self-host provider's own model/effort/config The AI-review cache fingerprint recorded each reviewer plan entry as only reviewer.model (the self-host PROVIDER name, e.g. "claude-code"), never that provider's own underlying model/effort/timeout/base-url (CLAUDE_AI_MODEL, CLAUDE_AI_EFFORT, CODEX_AI_MODEL, OLLAMA_AI_BASE_URL, etc. -- resolved separately at review-call time, not carried by AI_REVIEW_PLAN). Changing those while the provider name/plan stayed the same reused a same-head cached AI review produced against a different configuration. Add selfHostProviderConfig to the fingerprint input, populated from the same env vars each provider's own client construction reads (excludes API keys -- secrets, and irrelevant to review output). Also closes two codecov/patch branch gaps in the same diff: the new env.AI_REVIEW_PLAN ? {...} : null path in processors.ts, and the pre-existing (this PR's own diff) isGroundingEnabled/isEnrichmentEnabled && convergedRepoAllowed checks, neither of which had a test reaching their truthy arm. --- src/queue/processors.ts | 18 ++++++ src/review/ai-review-cache-input.ts | 42 ++++++++++++++ test/unit/ai-review-cache-input.test.ts | 60 +++++++++++++++++++ test/unit/queue.test.ts | 77 +++++++++++++++++++++++++ 4 files changed, 197 insertions(+) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 55a146863b..a6106efd93 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5257,6 +5257,24 @@ async function maybePublishPrPublicSurface( provider: settings.aiReviewProvider, model: settings.aiReviewModel, reviewerPlan: env.AI_REVIEW_PLAN, + selfHostProviderConfig: env.AI_REVIEW_PLAN + ? { + claudeModel: env.CLAUDE_AI_MODEL, + claudeEffort: env.CLAUDE_AI_EFFORT, + claudeTimeoutMs: env.CLAUDE_AI_TIMEOUT_MS, + codexModel: env.CODEX_AI_MODEL, + codexEffort: env.CODEX_AI_EFFORT, + codexTimeoutMs: env.CODEX_AI_TIMEOUT_MS, + ollamaBaseUrl: env.OLLAMA_AI_BASE_URL, + ollamaModel: env.OLLAMA_AI_MODEL, + openaiCompatibleBaseUrl: env.OPENAI_COMPATIBLE_AI_BASE_URL, + openaiCompatibleModel: env.OPENAI_COMPATIBLE_AI_MODEL, + openaiBaseUrl: env.OPENAI_AI_BASE_URL, + openaiModel: env.OPENAI_AI_MODEL, + anthropicBaseUrl: env.ANTHROPIC_AI_BASE_URL, + anthropicModel: env.ANTHROPIC_AI_MODEL, + } + : null, profile: reviewProfile, inlineComments: inlineCommentsEnabledForReview, pathInstructions: reviewPathInstructions, diff --git a/src/review/ai-review-cache-input.ts b/src/review/ai-review-cache-input.ts index bf29c444d1..431505b121 100644 --- a/src/review/ai-review-cache-input.ts +++ b/src/review/ai-review-cache-input.ts @@ -18,6 +18,30 @@ export type AiReviewCacheInput = { } | null | undefined; + // reviewerPlan only names WHICH self-host provider(s) are active (e.g. "claude-code") -- it does not carry that + // provider's own model/effort/timeout/base-url, which are resolved separately at review-call time (see + // src/selfhost/ai.ts's buildProvider). Fingerprint those too so switching a provider's underlying model or + // endpoint (while the provider name/plan stays the same) forces a cache miss instead of reusing a review + // produced against a different configuration. Deliberately excludes API keys (secrets, and irrelevant to output). + selfHostProviderConfig: + | { + claudeModel?: string | null | undefined; + claudeEffort?: string | null | undefined; + claudeTimeoutMs?: string | null | undefined; + codexModel?: string | null | undefined; + codexEffort?: string | null | undefined; + codexTimeoutMs?: string | null | undefined; + ollamaBaseUrl?: string | null | undefined; + ollamaModel?: string | null | undefined; + openaiCompatibleBaseUrl?: string | null | undefined; + openaiCompatibleModel?: string | null | undefined; + openaiBaseUrl?: string | null | undefined; + openaiModel?: string | null | undefined; + anthropicBaseUrl?: string | null | undefined; + anthropicModel?: string | null | undefined; + } + | null + | undefined; profile: ReviewProfile | null | undefined; inlineComments: boolean; pathInstructions: readonly ReviewPathInstruction[]; @@ -46,6 +70,24 @@ export async function aiReviewCacheInputFingerprint(input: AiReviewCacheInput): reviewers: (input.reviewerPlan.reviewers ?? []).map((reviewer) => reviewer.model ?? null), } : null, + selfHostProviderConfig: input.selfHostProviderConfig + ? { + claudeModel: input.selfHostProviderConfig.claudeModel ?? null, + claudeEffort: input.selfHostProviderConfig.claudeEffort ?? null, + claudeTimeoutMs: input.selfHostProviderConfig.claudeTimeoutMs ?? null, + codexModel: input.selfHostProviderConfig.codexModel ?? null, + codexEffort: input.selfHostProviderConfig.codexEffort ?? null, + codexTimeoutMs: input.selfHostProviderConfig.codexTimeoutMs ?? null, + ollamaBaseUrl: input.selfHostProviderConfig.ollamaBaseUrl ?? null, + ollamaModel: input.selfHostProviderConfig.ollamaModel ?? null, + openaiCompatibleBaseUrl: input.selfHostProviderConfig.openaiCompatibleBaseUrl ?? null, + openaiCompatibleModel: input.selfHostProviderConfig.openaiCompatibleModel ?? null, + openaiBaseUrl: input.selfHostProviderConfig.openaiBaseUrl ?? null, + openaiModel: input.selfHostProviderConfig.openaiModel ?? null, + anthropicBaseUrl: input.selfHostProviderConfig.anthropicBaseUrl ?? null, + anthropicModel: input.selfHostProviderConfig.anthropicModel ?? null, + } + : null, profile: input.profile ?? null, inlineComments: input.inlineComments, pathInstructions: input.pathInstructions.map((instruction) => ({ diff --git a/test/unit/ai-review-cache-input.test.ts b/test/unit/ai-review-cache-input.test.ts index 589a6616c9..2b468789a1 100644 --- a/test/unit/ai-review-cache-input.test.ts +++ b/test/unit/ai-review-cache-input.test.ts @@ -12,6 +12,7 @@ const baseInput = (): AiReviewCacheInput => ({ provider: null, model: null, reviewerPlan: null, + selfHostProviderConfig: null, profile: null, inlineComments: false, pathInstructions: [], @@ -86,6 +87,65 @@ describe("aiReviewCacheInputFingerprint", () => { expect(omittedReviewers).toBe(explicitEmpty); expect(sparse).toBe(explicit); }); + + it("changes when a self-host provider's underlying model/effort/timeout changes, even with the same reviewer plan", async () => { + const reviewerPlan = { combine: "single", reviewers: [{ model: "claude-code" }] }; + const fullyConfigured = { + claudeModel: "sonnet", + claudeEffort: "high", + claudeTimeoutMs: "60000", + codexModel: "gpt-5", + codexEffort: "high", + codexTimeoutMs: "240000", + ollamaBaseUrl: "http://localhost:11434/v1", + ollamaModel: "llama-3.1", + openaiCompatibleBaseUrl: "http://localhost:11434/v1", + openaiCompatibleModel: "llama-3.1", + openaiBaseUrl: "https://api.openai.com/v1", + openaiModel: "gpt-5", + anthropicBaseUrl: "https://api.anthropic.com", + anthropicModel: "claude-sonnet-5", + }; + + const original = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewerPlan, + selfHostProviderConfig: fullyConfigured, + }); + const repeated = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewerPlan, + selfHostProviderConfig: { ...fullyConfigured }, + }); + // The reviewer PLAN (provider names) is unchanged -- only the underlying model changed. The prior + // fingerprint (reviewer.model only) would have collided here; this must now miss. + const modelChanged = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewerPlan, + selfHostProviderConfig: { ...fullyConfigured, claudeModel: "opus" }, + }); + const effortChanged = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewerPlan, + selfHostProviderConfig: { ...fullyConfigured, claudeEffort: "low" }, + }); + + expect(repeated).toBe(original); + expect(modelChanged).not.toBe(original); + expect(effortChanged).not.toBe(original); + }); + + it("normalizes an absent self-host provider config the same whether omitted or explicitly empty", async () => { + const nullConfig = await aiReviewCacheInputFingerprint({ ...baseInput(), selfHostProviderConfig: null }); + const emptyConfig = await aiReviewCacheInputFingerprint({ ...baseInput(), selfHostProviderConfig: {} }); + const sparseConfig = await aiReviewCacheInputFingerprint({ + ...baseInput(), + selfHostProviderConfig: { claudeModel: undefined }, + }); + + expect(emptyConfig).toBe(sparseConfig); + expect(emptyConfig).not.toBe(nullConfig); + }); }); describe("aiReviewCacheInputMatches", () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 0f86a61ee5..b79099c987 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1556,6 +1556,7 @@ describe("queue processors", () => { provider: null, model: null, reviewerPlan: env.AI_REVIEW_PLAN, + selfHostProviderConfig: null, profile: null, inlineComments: false, pathInstructions: [], @@ -1684,6 +1685,82 @@ describe("queue processors", () => { expect(stickyComment.current?.body).not.toContain("is reviewing"); }); + it("computes the AI review cache fingerprint with a self-host reviewer plan and converged grounding/enrichment on (#2119)", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { + run: async () => { + aiCalls += 1; + return { response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }; + }, + } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + // A self-host reviewer plan (not just BYOK/cloud provider/model) plus its underlying provider config. + AI_REVIEW_PLAN: { reviewers: [{ model: "claude-code" }], combine: "single" } as never, + CLAUDE_AI_MODEL: "sonnet", + CLAUDE_AI_EFFORT: "high", + // Grounding + enrichment ON, with the repo allowlisted for convergence, so both feature flags + // resolve past their `isXEnabled(env) && convergedRepoAllowed` check into the fingerprint. + GITTENSORY_REVIEW_GROUNDING: "true", + GITTENSORY_REVIEW_ENRICHMENT: "true", + REES_URL: "https://rees.example", + GITTENSORY_REVIEW_REPOS: "JSONbored/gittensory", + }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "all_prs", + publicSurface: "comment_only", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + aiReviewMode: "block", + gatePack: "oss-anti-slop", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/7/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/7")) return Response.json({ number: 7, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [], body: "Closes #1" }); + if (url.includes("/commits/a7/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a7/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/7/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/7/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + // REES enrichment + any other unmatched call degrade fail-open on a generic empty response. + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "self-host-plan-converged-features", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 7, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [], body: "Closes #1" }, + }, + }); + + // The review ran fresh (no pre-seeded cache to reuse), reaching the fingerprint computation with the + // self-host reviewer plan, its provider config, and both converged feature checks evaluated. + expect(aiCalls).toBeGreaterThan(0); + }); + it("continues to final verdict when the reviewing placeholder audit write fails", async () => { const originalRecordAuditEvent = repositoriesModule.recordAuditEvent; const auditSpy = vi.spyOn(repositoriesModule, "recordAuditEvent").mockImplementation(async (auditEnv, event) => { From 839caaed7bf663f9a28a6faa33ad7ef373573fe4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 06:24:35 -0700 Subject: [PATCH 4/7] fix(review): fingerprint base sha and per-file patch content changedPaths only carried the touched file PATHS. A retarget (new base branch, same head commit) or certain rebases can change the diff GitHub reports for an otherwise-unchanged head SHA -- the same files can stay touched against the new base while their actual patch content differs, so aiReviewCacheInputMatches kept reusing the stale review. Add baseSha and a per-file content digest (path/status/patch/additions/ deletions -- exactly the fields buildAiReviewDiff and the AI review path read) to the fingerprint, sorted by path so a re-fetched diff in a different row order still hits the cache. --- src/queue/processors.ts | 8 +++ src/review/ai-review-cache-input.ts | 23 +++++++++ test/unit/ai-review-cache-input.test.ts | 68 +++++++++++++++++++++++++ test/unit/queue.test.ts | 9 +++- 4 files changed, 107 insertions(+), 1 deletion(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index a6106efd93..a6ef9c35de 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5285,6 +5285,14 @@ async function maybePublishPrPublicSurface( repoInstructions: reviewInstructions, excludePaths: reviewExcludePaths, changedPaths, + baseSha: webhook.baseSha, + reviewFiles: reviewFilesForAi.map((file) => ({ + path: file.path, + status: file.status, + patch: typeof file.payload?.patch === "string" ? file.payload.patch : undefined, + additions: file.additions, + deletions: file.deletions, + })), features: { grounding: isGroundingEnabled(env) && convergedRepoAllowed, rag: resolveConvergedFeature(env, reviewManifest, "rag", repoFullName), diff --git a/src/review/ai-review-cache-input.ts b/src/review/ai-review-cache-input.ts index 431505b121..b5a066206c 100644 --- a/src/review/ai-review-cache-input.ts +++ b/src/review/ai-review-cache-input.ts @@ -49,6 +49,19 @@ export type AiReviewCacheInput = { repoInstructions: string | null | undefined; excludePaths: readonly string[]; changedPaths: readonly string[]; + // A rebase or retarget (new base branch, same head commit) can change the diff GitHub reports for an + // otherwise-unchanged head SHA -- changedPaths (just the path list) stays the same when the same files + // are touched against the new base, but the actual patch content reviewed differs. baseSha plus a + // per-file content digest (path/status/patch/additions/deletions -- the fields buildAiReviewDiff and the + // grounding/RAG paths actually read) closes that gap. + baseSha: string | null | undefined; + reviewFiles: readonly { + path: string; + status?: string | null | undefined; + patch?: string | null | undefined; + additions: number; + deletions: number; + }[]; features: { grounding: boolean; rag: boolean; @@ -98,6 +111,16 @@ export async function aiReviewCacheInputFingerprint(input: AiReviewCacheInput): repoInstructions: input.repoInstructions?.trim() || null, excludePaths: normalizeStringList(input.excludePaths), changedPaths: normalizeStringList(input.changedPaths), + baseSha: input.baseSha ?? null, + reviewFiles: [...input.reviewFiles] + .map((file) => ({ + path: file.path, + status: file.status ?? null, + patch: file.patch ?? null, + additions: file.additions, + deletions: file.deletions, + })) + .sort((left, right) => left.path.localeCompare(right.path)), features: input.features, }; return `${AI_REVIEW_CACHE_INPUT_VERSION}:${await sha256Hex(stableStringify(payload))}`; diff --git a/test/unit/ai-review-cache-input.test.ts b/test/unit/ai-review-cache-input.test.ts index 2b468789a1..365a939b08 100644 --- a/test/unit/ai-review-cache-input.test.ts +++ b/test/unit/ai-review-cache-input.test.ts @@ -13,6 +13,8 @@ const baseInput = (): AiReviewCacheInput => ({ model: null, reviewerPlan: null, selfHostProviderConfig: null, + baseSha: null, + reviewFiles: [], profile: null, inlineComments: false, pathInstructions: [], @@ -88,6 +90,72 @@ describe("aiReviewCacheInputFingerprint", () => { expect(sparse).toBe(explicit); }); + it("changes when the patch content or base sha differs even though the same file paths are touched (retarget/rebase)", async () => { + // A retarget (new base branch, same head commit) or certain rebases can change the diff GitHub reports + // for an otherwise-unchanged head SHA -- changedPaths (just the path list) stays identical when the + // same files are touched against the new base, but the actual reviewed content differs. + const original = await aiReviewCacheInputFingerprint({ + ...baseInput(), + baseSha: "base1", + reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+new", additions: 1, deletions: 1 }], + }); + const samePathsDifferentPatch = await aiReviewCacheInputFingerprint({ + ...baseInput(), + baseSha: "base1", + reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+completely different", additions: 1, deletions: 1 }], + }); + const samePatchDifferentBase = await aiReviewCacheInputFingerprint({ + ...baseInput(), + baseSha: "base2", + reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+new", additions: 1, deletions: 1 }], + }); + const repeated = await aiReviewCacheInputFingerprint({ + ...baseInput(), + baseSha: "base1", + reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+new", additions: 1, deletions: 1 }], + }); + // File order must not matter -- only content -- so a re-fetched diff in a different row order still hits. + const reordered = await aiReviewCacheInputFingerprint({ + ...baseInput(), + baseSha: "base1", + reviewFiles: [ + { path: "src/b.ts", status: "added", patch: "@@ -0,0 +1 @@\n+export {}", additions: 1, deletions: 0 }, + { path: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+new", additions: 1, deletions: 1 }, + ], + }); + const reorderedAgain = await aiReviewCacheInputFingerprint({ + ...baseInput(), + baseSha: "base1", + reviewFiles: [ + { path: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+new", additions: 1, deletions: 1 }, + { path: "src/b.ts", status: "added", patch: "@@ -0,0 +1 @@\n+export {}", additions: 1, deletions: 0 }, + ], + }); + + expect(samePathsDifferentPatch).not.toBe(original); + expect(samePatchDifferentBase).not.toBe(original); + expect(repeated).toBe(original); + expect(reordered).toBe(reorderedAgain); + }); + + it("normalizes a file entry with no status/patch (e.g. a rename with no content change) deterministically", async () => { + const omitted = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewFiles: [{ path: "src/a.ts", additions: 0, deletions: 0 }], + }); + const explicitNull = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewFiles: [{ path: "src/a.ts", status: null, patch: null, additions: 0, deletions: 0 }], + }); + const withStatusAndPatch = await aiReviewCacheInputFingerprint({ + ...baseInput(), + reviewFiles: [{ path: "src/a.ts", status: "renamed", patch: "@@ -1 +1 @@", additions: 0, deletions: 0 }], + }); + + expect(omitted).toBe(explicitNull); + expect(omitted).not.toBe(withStatusAndPatch); + }); + it("changes when a self-host provider's underlying model/effort/timeout changes, even with the same reviewer plan", async () => { const reviewerPlan = { combine: "single", reviewers: [{ model: "claude-code" }] }; const fullyConfigured = { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index b79099c987..b94df8bb41 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1557,6 +1557,8 @@ describe("queue processors", () => { model: null, reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, + baseSha: null, + reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = value.length;", additions: 1, deletions: 0 }], profile: null, inlineComments: false, pathInstructions: [], @@ -1732,7 +1734,12 @@ describe("queue processors", () => { const url = input.toString(); const method = init?.method ?? "GET"; if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); - if (url.includes("/pulls/7/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.includes("/pulls/7/files")) + return Response.json([ + { filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }, + // GitHub omits `patch` for binary/oversized files -- the fingerprint must still normalize this case. + { filename: "assets/logo.png", status: "modified", additions: 0, deletions: 0, changes: 0 }, + ]); if (url.endsWith("/pulls/7")) return Response.json({ number: 7, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [], body: "Closes #1" }); if (url.includes("/commits/a7/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); if (url.includes("/commits/a7/status")) return Response.json({ state: "success", statuses: [] }); From 6dbefb2e5d304412e391cc16d7b88a1ab5d00a3e Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 11:56:26 -0700 Subject: [PATCH 5/7] fix(selfhost): stop a second scheduled sweep trigger from queuing behind the first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior removal of ALL regate queue-depth backpressure let every cron tick enqueue another agent-regate-sweep fan-out even while one was still pending/processing, risking duplicate per-repo re-gates once both trigger messages are consumed. Reinstate the check scoped to the trigger job type only (agent-regate-sweep) — NOT per-PR agent-regate-pr backlog, whose normal staggered/rate-deferred drain is what caused the original sweep-starvation bug this PR set out to fix. --- src/index.ts | 27 +++++++++++++++++++++++++ test/unit/index.test.ts | 44 +++++++++++++++++++++++++++++++++-------- 2 files changed, 63 insertions(+), 8 deletions(-) diff --git a/src/index.ts b/src/index.ts index 0e81aa57d3..6b6a9ec63b 100644 --- a/src/index.ts +++ b/src/index.ts @@ -9,12 +9,21 @@ import { isRagEnabled } from "./review/rag-wire"; import { isSelfTuneEnabled } from "./review/selftune-wire"; import { isGitHubBudgetBackgroundJob, + queueSnapshotBacklog, + queueSnapshotFromBinding, scheduledEnqueueDelaySeconds, } from "./selfhost/queue-common"; import { isReviewExecutionJob, isSelfHostedReviewRuntime } from "./selfhost/review-runtime"; import type { JobMessage } from "./types"; const app = createApp(); +// Scoped to the top-level fan-out TRIGGER only (#audit-sweep-fanout) — NOT "agent-regate-pr", whose per-repo +// backlog is normal, expected, and can legitimately stay nonzero for long periods (staggered/rate-deferred +// per-PR re-reviews), which is exactly what caused the prior broad backlog check to starve the scheduled sweep +// entirely. A pending/processing "agent-regate-sweep" message means a fan-out is already in flight; the +// per-repo drain guard (getLatestRegatedAt / isRegateSweepDraining) already protects individual repos once that +// single fan-out runs, so this only needs to stop a SECOND trigger from queuing up behind the first. +const REGATE_SWEEP_TRIGGER_TYPES = ["agent-regate-sweep"] as const; export { RateLimiter }; @@ -102,11 +111,29 @@ async function enqueueScheduledJobs(env: Env, controller: ScheduledController): // tick (~2 min) retries, and after the bucket resets the sweep resumes. Webhooks never pre-yield. const jobs: JobMessage[] = []; const selfHostedReviews = isSelfHostedReviewRuntime(env); + const queueSnapshot = selfHostedReviews + ? await queueSnapshotFromBinding(env.JOBS).catch((error) => { + console.warn( + JSON.stringify({ + level: "warn", + event: "selfhost_queue_snapshot_failed", + error: error instanceof Error ? error.message : "unknown error", + }), + ); + return null; + }) + : null; + const sweepTriggerBacklog = queueSnapshotBacklog(queueSnapshot, REGATE_SWEEP_TRIGGER_TYPES); let sweepThrottledUntil: string | undefined; if (selfHostedReviews) { sweepThrottledUntil = await shouldWaitForGitHubRateLimit(env, MAINTENANCE_RESERVED_HEADROOM); if (sweepThrottledUntil) { console.log(JSON.stringify({ event: "regate_sweep_throttled", resetAt: sweepThrottledUntil })); + } else if (sweepTriggerBacklog > 0) { + // A fan-out trigger is already pending/processing — skip re-arming so the queue never accumulates a + // second identical trigger behind the first (#audit-sweep-fanout). This is scoped to the trigger job + // itself; it does not look at (and is not blocked by) per-repo "agent-regate-pr" backlog. + console.log(JSON.stringify({ event: "regate_sweep_trigger_backlog_deferred", backlog: sweepTriggerBacklog })); } else { jobs.push({ type: "agent-regate-sweep", requestedBy: "schedule" }); } diff --git a/test/unit/index.test.ts b/test/unit/index.test.ts index 34d29f1ff5..5ec79d6ea9 100644 --- a/test/unit/index.test.ts +++ b/test/unit/index.test.ts @@ -277,7 +277,9 @@ describe("worker entrypoint", () => { expect(sent).toEqual([{ type: "agent-regate-sweep", requestedBy: "schedule" }]); }); - it("keeps enqueueing scheduled sweeps while prior regate jobs are queued", async () => { + it("keeps enqueueing scheduled sweeps while prior per-PR regate jobs are queued (#2119)", async () => { + // Per-PR "agent-regate-pr" backlog is normal, expected, ongoing work (staggered/rate-deferred re-reviews) — + // it must NOT block the next scheduled fan-out trigger, or the sweep starves under any sustained load. const sent: Array = []; let snapshotCalled = false; const env = createTestEnv({ @@ -288,11 +290,8 @@ describe("worker entrypoint", () => { snapshot: async () => { snapshotCalled = true; return { - totals: { pending: 2, processing: 1, dead: 0, due: 2 }, - byType: [ - { type: "agent-regate-pr", status: "pending", count: 2, due: 2 }, - { type: "agent-regate-sweep", status: "processing", count: 1, due: 0 }, - ], + totals: { pending: 2, processing: 0, dead: 0, due: 2 }, + byType: [{ type: "agent-regate-pr", status: "pending", count: 2, due: 2 }], }; }, } as unknown as Queue, @@ -308,7 +307,34 @@ describe("worker entrypoint", () => { { type: "repair-data-fidelity", requestedBy: "schedule" }, { type: "refresh-installation-health", requestedBy: "schedule" }, ]); - expect(snapshotCalled).toBe(false); + expect(snapshotCalled).toBe(true); + }); + + it("defers a new sweep trigger while a prior one is still pending or processing (#2119, #audit-sweep-fanout)", async () => { + const sent: Array = []; + const env = createTestEnv({ + JOBS: { + async send(message: import("../../src/types").JobMessage) { + sent.push(message); + }, + snapshot: async () => ({ + totals: { pending: 0, processing: 1, dead: 0, due: 0 }, + byType: [{ type: "agent-regate-sweep", status: "processing", count: 1, due: 0 }], + }), + } as unknown as Queue, + }); + const waitUntil: Promise[] = []; + + await worker.scheduled(controllerFor("2026-05-25T05:30:00.000Z"), env, executionContext(waitUntil)); + await Promise.all(waitUntil); + + // No SECOND "agent-regate-sweep" trigger is enqueued behind the one already in flight; the other :30 jobs + // are unaffected since they never depended on the (removed, broad) backlog check. + expect(sent).toEqual([ + { type: "backfill-registered-repos", requestedBy: "schedule", mode: "light" }, + { type: "repair-data-fidelity", requestedBy: "schedule" }, + { type: "refresh-installation-health", requestedBy: "schedule" }, + ]); }); it("does not require queue introspection for regular review sweep scheduling", async () => { @@ -329,8 +355,10 @@ describe("worker entrypoint", () => { await worker.scheduled(controllerFor("2026-05-25T05:14:00.000Z"), env, executionContext(waitUntil)); await Promise.all(waitUntil); + // Fails OPEN on a broken snapshot binding: the sweep still enqueues, and the failure is surfaced (not + // silently swallowed) so an operator can see the introspection is unavailable. expect(sent).toEqual([{ type: "agent-regate-sweep", requestedBy: "schedule" }]); - expect(warn).not.toHaveBeenCalledWith(expect.stringContaining("selfhost_queue_snapshot_failed")); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("selfhost_queue_snapshot_failed")); }); it("does not enqueue review sweeps from a broker-only Cloudflare runtime", async () => { From 991a94a7b9250f23425da7d97e47ee89ad18a2ed Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 12:22:26 -0700 Subject: [PATCH 6/7] fix(selfhost): surface backup-exporter install failures, bypass AI review cache for dynamic context Security scanner (P2): the backup-exporter's runtime apk add suppressed all output, hiding an install failure that would silently leave the metrics endpoint non-functional. Drop the >/dev/null 2>&1 redirection. Gate review: grounding/RAG/enrichment/reputation each pull time-varying external context (live CI checks, the vector index, REES/CVE data, the submitter's evolving reputation) that can change for an unchanged head SHA without any of the feature-activation booleans flipping, so a fingerprint built from just those booleans can't detect the drift. A repo with any of these active now bypasses the AI review cache entirely (never reads, never writes) instead of fingerprinting a signal that can't prove freshness. --- docker-compose.yml | 2 +- src/queue/processors.ts | 55 +++++++++----- test/unit/queue.test.ts | 74 +++++++++++++++++++ .../selfhost-observability-config.test.ts | 2 +- 4 files changed, 112 insertions(+), 21 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 4fd838ec09..489724610a 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -578,7 +578,7 @@ services: command: - /bin/sh - -c - - "apk add --no-cache busybox-extras >/dev/null 2>&1 && sh /backup-metrics.sh" + - "apk add --no-cache busybox-extras && sh /backup-metrics.sh" healthcheck: test: ["CMD-SHELL", "wget -qO- http://127.0.0.1:9101/metrics | grep -q '^gittensory_backup_latest_timestamp_seconds'"] interval: 30s diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 0ebabbb8b5..b44f67a01a 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5251,6 +5251,28 @@ async function maybePublishPrPublicSurface( .filter(Boolean) .join("\n\n") || null; const convergedRepoAllowed = isConvergenceRepoAllowed(env, repoFullName); + // Resolved ONCE and reused both for the fingerprint AND the cache-bypass decision below: grounding/RAG/ + // enrichment/reputation each pull TIME-VARYING external context (live CI checks, the vector index, + // REES/CVE data, the submitter's evolving reputation) that can change for the SAME head SHA without + // any of these booleans flipping. Fingerprinting only "is the feature on" can't detect that drift + // without fetching the content itself (which would defeat caching), so a repo with ANY of these active + // bypasses the cache entirely rather than fingerprinting a value that can't prove freshness. + const dynamicReviewFeatures = { + grounding: isGroundingEnabled(env) && convergedRepoAllowed, + rag: resolveConvergedFeature(env, reviewManifest, "rag", repoFullName), + enrichment: isEnrichmentEnabled(env) && convergedRepoAllowed, + reputation: resolveConvergedFeature( + env, + reviewManifest, + "reputation", + repoFullName, + ), + }; + const dynamicReviewContextActive = + dynamicReviewFeatures.grounding || + dynamicReviewFeatures.rag || + dynamicReviewFeatures.enrichment || + dynamicReviewFeatures.reputation; const inputFingerprint = await aiReviewCacheInputFingerprint({ mode: settings.aiReviewMode, byok: settings.aiReviewByok, @@ -5293,29 +5315,24 @@ async function maybePublishPrPublicSurface( additions: file.additions, deletions: file.deletions, })), - features: { - grounding: isGroundingEnabled(env) && convergedRepoAllowed, - rag: resolveConvergedFeature(env, reviewManifest, "rag", repoFullName), - enrichment: isEnrichmentEnabled(env) && convergedRepoAllowed, - reputation: resolveConvergedFeature( - env, - reviewManifest, - "reputation", - repoFullName, - ), - }, + features: dynamicReviewFeatures, }); // #1 self-host AI-review cache: the LLM output for a PR changes only when the code (head SHA), review // mode, reviewer plan, feature activation, or prompt-shaping inputs change. A re-delivered webhook or the // block-mode re-gate sweep can reuse that exact review; stale same-head reviews from older private review // instructions or feature config are intentionally treated as misses. The deterministic gate still runs. - const cachedReview = await getCachedAiReview( - env, - repoFullName, - pr.number, - advisory.headSha, - settings.aiReviewMode, - ).catch(() => null); + // A repo with an active dynamic-context feature (grounding/RAG/enrichment/reputation) bypasses the + // cache entirely — see dynamicReviewContextActive above — since a cache hit there could replay a + // review built against now-stale external context for an otherwise-unchanged head. + const cachedReview = dynamicReviewContextActive + ? null + : await getCachedAiReview( + env, + repoFullName, + pr.number, + advisory.headSha, + settings.aiReviewMode, + ).catch(() => null); if ( cachedReview && aiReviewCacheInputMatches(cachedReview.metadata, inputFingerprint) && @@ -5339,7 +5356,7 @@ async function maybePublishPrPublicSurface( reviewExcludePaths, reviewInlineComments, }); - if (aiReview && aiReview.cacheable !== false) + if (aiReview && aiReview.cacheable !== false && !dynamicReviewContextActive) await putCachedAiReview( env, repoFullName, diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 0ed9a13eca..98fd1c6ec0 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1768,6 +1768,80 @@ describe("queue processors", () => { expect(aiCalls).toBeGreaterThan(0); }); + it("bypasses the AI review cache entirely while a dynamic-context feature (grounding) is active (#2119)", async () => { + // Grounding/RAG/enrichment/reputation each pull TIME-VARYING external context (live CI checks, the vector + // index, REES/CVE data, reputation) that can change for the SAME head SHA without the feature flags + // themselves flipping — so a cache hit here could replay a review built against now-stale context. A repo + // with any of these active must re-run AI on EVERY review of the same head, never reuse a prior cache entry. + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { + run: async () => { + aiCalls += 1; + return { response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }; + }, + } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + GITTENSORY_REVIEW_GROUNDING: "true", + GITTENSORY_REVIEW_REPOS: "JSONbored/gittensory", + }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "all_prs", + publicSurface: "comment_only", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + aiReviewMode: "block", + gatePack: "oss-anti-slop", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/7/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/7")) return Response.json({ number: 7, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [], body: "Closes #1" }); + if (url.includes("/commits/a7/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a7/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/7/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/7/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + const webhook = { + type: "github-webhook" as const, + eventName: "pull_request" as const, + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" as const } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 7, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [], body: "Closes #1" }, + }, + }; + await processJob(env, { ...webhook, deliveryId: "dynamic-context-bypass-1" }); + const firstRunAiCalls = aiCalls; + expect(firstRunAiCalls).toBeGreaterThan(0); + // Re-review of the SAME head with the SAME (unchanged) inputs. A plain fingerprint match would reuse the + // first run's cached review here (leaving aiCalls unchanged) — this asserts the AI ran the SAME full set of + // calls again instead, proving the cache was never written (or never read) while grounding stayed active. + await processJob(env, { ...webhook, deliveryId: "dynamic-context-bypass-2" }); + expect(aiCalls).toBe(firstRunAiCalls * 2); + }); + it("continues to final verdict when the reviewing placeholder audit write fails", async () => { const originalRecordAuditEvent = repositoriesModule.recordAuditEvent; const auditSpy = vi.spyOn(repositoriesModule, "recordAuditEvent").mockImplementation(async (auditEnv, event) => { diff --git a/test/unit/selfhost-observability-config.test.ts b/test/unit/selfhost-observability-config.test.ts index 43a7b35ec3..6e79d457ce 100644 --- a/test/unit/selfhost-observability-config.test.ts +++ b/test/unit/selfhost-observability-config.test.ts @@ -116,7 +116,7 @@ describe("self-host observability trace config", () => { expect(backupExporter.command).toEqual([ "/bin/sh", "-c", - "apk add --no-cache busybox-extras >/dev/null 2>&1 && sh /backup-metrics.sh", + "apk add --no-cache busybox-extras && sh /backup-metrics.sh", ]); expect(backupExporter.healthcheck?.test).toEqual([ "CMD-SHELL", From 88abec0296e2958e6acd05786de6df7a535fdc8b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 13:20:07 -0700 Subject: [PATCH 7/7] fix(review): include PR title in the AI review cache fingerprint The AI review prompt threads the PR title in (runAiReviewForAdvisory's pr.title, used to build the reviewer's context), but the new rich fingerprint in AiReviewCacheInput never included it. A pull_request edited event that changes only the title (same head SHA) could therefore reuse a cached review generated under the old title, replaying findings for prompt metadata that no longer matches the PR. Add a required title field to AiReviewCacheInput and pass pr.title from the queue processor's fingerprint call site. Update the three test fixtures that construct AiReviewCacheInput literals, and add a direct regression test asserting the fingerprint changes when only the title changes. --- src/queue/processors.ts | 1 + src/review/ai-review-cache-input.ts | 5 +++++ test/unit/ai-review-cache-input.test.ts | 13 +++++++++++++ test/unit/ai-review-cache.test.ts | 1 + test/unit/queue.test.ts | 1 + 5 files changed, 21 insertions(+) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index b32362dd06..83beac8d8e 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5568,6 +5568,7 @@ async function maybePublishPrPublicSurface( dynamicReviewFeatures.enrichment || dynamicReviewFeatures.reputation; const inputFingerprint = await aiReviewCacheInputFingerprint({ + title: pr.title, mode: settings.aiReviewMode, byok: settings.aiReviewByok, provider: settings.aiReviewProvider, diff --git a/src/review/ai-review-cache-input.ts b/src/review/ai-review-cache-input.ts index 4e92818525..1d2176d831 100644 --- a/src/review/ai-review-cache-input.ts +++ b/src/review/ai-review-cache-input.ts @@ -7,6 +7,10 @@ import { sha256Hex } from "../utils/crypto"; export const AI_REVIEW_CACHE_INPUT_VERSION = "ai-review-input:v1"; export type AiReviewCacheInput = { + // The PR title is threaded into the reviewer prompt (see runAiReviewForAdvisory's pr.title), so a same-head + // `edited` event that changes only the title must miss the cache rather than replay a review generated for + // different prompt metadata. + title: string; mode: string; byok: boolean; provider: string | null | undefined; @@ -85,6 +89,7 @@ export type AiReviewCacheInput = { export async function aiReviewCacheInputFingerprint(input: AiReviewCacheInput): Promise { const payload = { version: AI_REVIEW_CACHE_INPUT_VERSION, + title: input.title, mode: input.mode, byok: input.byok, provider: input.provider ?? null, diff --git a/test/unit/ai-review-cache-input.test.ts b/test/unit/ai-review-cache-input.test.ts index fcb0efb81c..76d6de2f2c 100644 --- a/test/unit/ai-review-cache-input.test.ts +++ b/test/unit/ai-review-cache-input.test.ts @@ -5,6 +5,7 @@ import { } from "../../src/review/ai-review-cache-input"; const baseInput = (): AiReviewCacheInput => ({ + title: "Fix the retry loop", mode: "block", byok: false, provider: null, @@ -216,6 +217,18 @@ describe("aiReviewCacheInputFingerprint", () => { expect(emptyConfig).not.toBe(nullConfig); }); + it("changes when the PR title changes even though nothing else does (#2119)", async () => { + // The title is threaded into the reviewer prompt (runAiReviewForAdvisory's pr.title), so a same-head + // `edited` event that changes only the title must miss the cache rather than replay a review generated + // against different prompt metadata. + const original = await aiReviewCacheInputFingerprint(baseInput()); + const titleChanged = await aiReviewCacheInputFingerprint({ ...baseInput(), title: "Fix the retry loop (v2)" }); + const repeated = await aiReviewCacheInputFingerprint(baseInput()); + + expect(titleChanged).not.toBe(original); + expect(repeated).toBe(original); + }); + it("changes when aiReviewAllAuthors, aiReviewCloseConfidence, or gatePack change", async () => { const original = await aiReviewCacheInputFingerprint(baseInput()); const allAuthorsChanged = await aiReviewCacheInputFingerprint({ ...baseInput(), aiReviewAllAuthors: true }); diff --git a/test/unit/ai-review-cache.test.ts b/test/unit/ai-review-cache.test.ts index 17b3214168..f507ee6cc5 100644 --- a/test/unit/ai-review-cache.test.ts +++ b/test/unit/ai-review-cache.test.ts @@ -4,6 +4,7 @@ import { aiReviewCacheInputFingerprint, type AiReviewCacheInput } from "../../sr import { createTestEnv } from "../helpers/d1"; const baseFingerprintInput = (): AiReviewCacheInput => ({ + title: "Fix the retry loop", mode: "block", byok: false, provider: null, diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index d414ad5acc..e4afc1194a 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1548,6 +1548,7 @@ describe("queue processors", () => { await upsertPullRequestFile(env, { repoFullName: "owner/agent-repo", pullNumber: 7, path: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, payload: { patch: "@@\n+export const ok = value.length;" } }); // Pre-seed the AI review for this exact head SHA + mode → the sweep's block-mode review must reuse it, not re-run. const inputFingerprint = await aiReviewCacheInputFingerprint({ + title: "Stale PR", mode: "block", byok: false, provider: null,