diff --git a/apps/loopover-ui/src/lib/selfhost-env-reference.ts b/apps/loopover-ui/src/lib/selfhost-env-reference.ts index e7c0067ed3..e5c0776eb2 100644 --- a/apps/loopover-ui/src/lib/selfhost-env-reference.ts +++ b/apps/loopover-ui/src/lib/selfhost-env-reference.ts @@ -373,6 +373,10 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [ name: "OLLAMA_AI_MODEL", firstReference: "src/selfhost/ai.ts", }, + { + name: "OLLAMA_NUM_CTX", + firstReference: "src/selfhost/ai.ts", + }, { name: "OPENAI_AI_BASE_URL", firstReference: "src/selfhost/ai.ts", @@ -754,6 +758,7 @@ export const SELFHOST_ENV_REFERENCE_MARKDOWN = [ "| `OLLAMA_AI_API_KEY` | `src/selfhost/ai.ts` |", "| `OLLAMA_AI_BASE_URL` | `src/selfhost/ai.ts` |", "| `OLLAMA_AI_MODEL` | `src/selfhost/ai.ts` |", + "| `OLLAMA_NUM_CTX` | `src/selfhost/ai.ts` |", "| `OPENAI_AI_BASE_URL` | `src/selfhost/ai.ts` |", "| `OPENAI_AI_MODEL` | `src/selfhost/ai.ts` |", "| `OPENAI_API_KEY` | `src/selfhost/ai-config.ts` |", diff --git a/src/review/ai-review-cache-input.ts b/src/review/ai-review-cache-input.ts index 153f25bbdd..f64e117f37 100644 --- a/src/review/ai-review-cache-input.ts +++ b/src/review/ai-review-cache-input.ts @@ -4,6 +4,7 @@ import type { SelfHostAiModelConfig, } from "../signals/focus-manifest"; import { sha256Hex } from "../utils/crypto"; +import { buildCanonicalJudgePrompt, REVIEW_PROMPT_VERSION } from "../services/ai-review"; // Bumped v1→v2 (#2995): `features` gained a `cultureProfile` member. Bumped v2→v3 (#2182-#2186): `features` // gained an `impactMap` member. Bumped v3→v4 (#3902): `selfHostAiModelOverride` gained ollamaModel/openaiModel/ @@ -14,7 +15,8 @@ import { sha256Hex } from "../utils/crypto"; // codexFirstOutputTimeoutMs members. Every prior cached review's fingerprint was computed without those keys, // so bumping the version guarantees a clean cache miss on the first review after upgrade rather than silently // reusing a hash computed under a different payload shape. -export const AI_REVIEW_CACHE_INPUT_VERSION = "ai-review-input:v6"; +// #9477: bumped v6 -> v7 to invalidate every row written under the incomplete fingerprint below. +export const AI_REVIEW_CACHE_INPUT_VERSION = "ai-review-input:v7"; // #regate-churn (root cause, confirmed in production): this fingerprint USED to also hash the PR's live // `baseSha`, on the theory that a rebase/retarget can change the diff GitHub reports for an otherwise-unchanged @@ -130,6 +132,19 @@ export type AiReviewCacheInput = { export async function aiReviewCacheInputFingerprint(input: AiReviewCacheInput): Promise { const payload = { version: AI_REVIEW_CACHE_INPUT_VERSION, + // #9477: the fingerprint must cover what the model is ASKED, not just the change it is asked about. + // + // getCachedAiReview reuses a cacheable=1 row with NO maxAgeMs, replaying its findings verbatim -- + // including a severity:"critical" ai_consensus_defect. AI_REVIEW_CACHE_INPUT_VERSION was hand-bumped and + // last moved for #8364, but #8789, #8791, #8833, #8845, #8961, #9035, #9074, #9087, #9114, #9145 and #9445 + // all changed prompt text or verdict logic WITHOUT a bump. So a PR closed or held under (say) the pre-#9074 + // "false consensus" rule re-gated at the same head to the SAME stale finding, and the corrected logic + // never ran for any already-reviewed head. + // + // Folding in the prompt version AND a digest of the canonical judge prompt makes drift an automatic cache + // MISS instead of something a human has to remember. Both are pure and available before the call. + promptVersion: REVIEW_PROMPT_VERSION, + promptDigest: await sha256Hex(buildCanonicalJudgePrompt()), title: input.title, body: input.body ?? null, mode: input.mode, diff --git a/src/selfhost/ai.ts b/src/selfhost/ai.ts index ee672a8cdb..b826e344d6 100644 --- a/src/selfhost/ai.ts +++ b/src/selfhost/ai.ts @@ -378,7 +378,15 @@ export function createOpenAiCompatibleAi(opts: { messages: toMessages(options).map((message) => ({ role: message.role, content: toOpenAiMessageContent(message.content) })), max_tokens: options.max_tokens, temperature: options.temperature, - ...(options.providerOptions ? { options: options.providerOptions } : {}), + // #9478: Ollama silently LEFT-TRUNCATES anything past its context window (num_ctx, typically 4k-8k + // by default) and then answers confidently over whatever survived -- which is the TAIL of the prompt, + // i.e. the context sections rather than the diff. The review path sends up to 120k chars of diff plus + // a 240k-char aggregate context budget, so on the fallback provider a review could be produced from a + // fraction of the change while carrying the SAME blocker authority and the same confidence semantics + // as the primary. Nothing surfaced which provider decided. Send an explicit num_ctx sized for the + // review prompt so the server allocates the window instead of quietly dropping the diff; an explicit + // caller-supplied providerOptions still wins. + ...ollamaContextOptions(opts.providerName, options), // #8790: force JSON mode when the caller declared a JSON contract — Ollama's and vLLM's // OpenAI-compatible layers both honor it; servers that don't get the 400-fallback below. ...(withResponseFormat && options.responseFormat === "json_object" ? { response_format: { type: "json_object" } } : {}), @@ -711,6 +719,29 @@ function buildAiUsage(fields: { * rows to a real provider instead of leaving them permanently unattributed. Recognizes the bundled * docker-compose `ollama` service and OpenAI's own endpoint; anything else (vLLM, LM Studio, a custom * hostname) is the honest generic "openai-compatible" bucket rather than a guessed, possibly-wrong label. */ +/** + * #9478: the `options` bag sent to an Ollama-compatible server, carrying an explicit context window. + * + * Only applied for the `ollama` provider -- other OpenAI-compatible servers may reject an unknown `options` + * key, and only Ollama has the silent-truncation behaviour this guards against. A caller that supplies its own + * providerOptions keeps full control (the vision path already sets num_ctx itself). + */ +export function ollamaContextOptions( + providerName: "ollama" | "openai" | "openai-compatible" | undefined, + options: { providerOptions?: Record | undefined }, +): { options?: Record } { + if (options.providerOptions) return { options: options.providerOptions }; + if (providerName !== "ollama") return {}; + return { options: { num_ctx: ollamaNumCtx() } }; +} + +/** Context window requested from Ollama for review-sized prompts. Overridable because it trades GPU memory + * against how much of a large diff the model can actually see. */ +export function ollamaNumCtx(): number { + const raw = Number(process.env["OLLAMA_NUM_CTX"] ?? ""); + return Number.isFinite(raw) && raw > 0 ? Math.floor(raw) : 32_768; +} + export function providerNameFromBaseUrl(baseUrl: string | undefined): "ollama" | "openai" | "openai-compatible" { let hostname = ""; try { diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index ed9eaca6ee..2fb19a0f76 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -573,6 +573,12 @@ export function parsedReviewModelIds(diagnostics: readonly AiReviewDiagnostic[]) type ReviewerOpinionOutcome = { review: ModelReview | null; fallbackNote?: string | undefined; + /** #9478: the model that actually PRODUCED this review. runWorkersOpinion iterates [primary, fallback] + * internally, so a fallback-produced opinion was previously recorded as a primary vote -- poisoning the + * reviewer_vote audit events, recordRoutingShadow's evidence-weighted routing track records (#8229), and + * scoreJudgmentAgreement's contribution to decision-record confidence. The doc claimed slot<->model was + * "unambiguous by construction", which holds for the tie-break slot SWAP but not for in-slot fallback. */ + producedBy?: string | undefined; }; type AiGatewayOptions = { gateway?: { id: string } }; @@ -1485,6 +1491,8 @@ async function runWorkersOpinion( // ONLY for the case where every attempt across every model comes back this way -- degrades to exactly today's // behavior in that (expected to be rare) worst case, never worse. let bestIncompleteReview: ModelReview | null = null; + // #9478: which model produced the last-resort candidate, so its vote is attributed correctly too. + let bestIncompleteReviewModel: string | undefined; const models = fallback && fallback !== primary ? [primary, fallback] : [primary]; for (const [modelIndex, model] of models.entries()) { if (modelIndex > 0) { @@ -1578,12 +1586,13 @@ async function runWorkersOpinion( } if (parsed && parsed.assessment.trim() !== "") { diagnostics.push({ model, attempt, status: "parsed", responseChars: text.length, hasJsonObject: Boolean(extractLastJsonObject(text)), ...usageFields }); - return { review: parsed }; + return { review: parsed, producedBy: model }; } if (parsed) { // Valid JSON, real blockers/nits/suggestions, but the REQUIRED assessment came back empty -- // keep it as a last-resort candidate and retry for a real one instead of accepting immediately. bestIncompleteReview = parsed; + bestIncompleteReviewModel = model; diagnostics.push({ model, attempt, status: "missing_assessment", responseChars: text.length, hasJsonObject: true, ...usageFields }); console.warn( JSON.stringify({ @@ -1701,7 +1710,7 @@ async function runWorkersOpinion( nitsCount: bestIncompleteReview.nits.length, }), ); - return { review: bestIncompleteReview }; + return { review: bestIncompleteReview, ...(bestIncompleteReviewModel ? { producedBy: bestIncompleteReviewModel } : {}) }; } return { review: null }; } @@ -2971,8 +2980,10 @@ export async function runLoopOverAiReview( if (a.fallbackNote) fallbackNotes.push(a.fallbackNote); if (b.fallbackNote) fallbackNotes.push(b.fallbackNote); // #8229 stage 0: attach votes HERE, where slot↔model is unambiguous by construction. - if (a.review) reviewerVotes.push({ reviewer: primary.model, votedFail: a.review.blockers.length > 0 }); - if (b.review) reviewerVotes.push({ reviewer: secondary.model, votedFail: b.review.blockers.length > 0 }); + // #9478: attribute to the model that ACTUALLY produced the review, falling back to the slot's configured + // model only when the outcome carries no producer (an unparseable/never-ran slot has no vote anyway). + if (a.review) reviewerVotes.push({ reviewer: a.producedBy ?? primary.model, votedFail: a.review.blockers.length > 0 }); + if (b.review) reviewerVotes.push({ reviewer: b.producedBy ?? secondary.model, votedFail: b.review.blockers.length > 0 }); secondReview = b.review; // Combine per the configured strategy (#dual-ai-combiner). Default `consensus` is byte-identical to the // historical logic: block only on agreement, lone blocker → split, a missing opinion → inconclusive diff --git a/test/unit/ai-review-cache-input.test.ts b/test/unit/ai-review-cache-input.test.ts index 9328fb8687..08c65ba902 100644 --- a/test/unit/ai-review-cache-input.test.ts +++ b/test/unit/ai-review-cache-input.test.ts @@ -1,3 +1,5 @@ +import { describe, expect, it, vi } from "vitest"; +import * as aiReviewModule from "../../src/services/ai-review"; import { AI_REVIEW_CACHE_INPUT_VERSION, aiReviewCacheInputFingerprint, @@ -351,3 +353,42 @@ describe("aiReviewCacheInputFingerprint", () => { expect(repeated).toBe(original); }); }); + +// #9477: getCachedAiReview reuses a cacheable=1 row with NO maxAgeMs, replaying its findings verbatim -- +// including a severity:"critical" ai_consensus_defect. AI_REVIEW_CACHE_INPUT_VERSION was hand-bumped and last +// moved for #8364, but #8789, #8791, #8833, #8845, #8961, #9035, #9074, #9087, #9114, #9145 and #9445 all +// changed prompt text or verdict logic WITHOUT a bump. So a PR closed or held under (say) the pre-#9074 "false +// consensus" rule re-gated at the same head to the SAME stale finding, and the fixed logic never ran for any +// already-reviewed head. The fingerprint must cover what the model is ASKED, not only the change it is asked +// about -- and it must do so structurally, not by relying on a human remembering a constant. +describe("prompt-drift is an automatic cache miss (#9477)", () => { + it("REGRESSION: a change to the canonical judge prompt changes the fingerprint", async () => { + const before = await aiReviewCacheInputFingerprint(baseInput()); + const spy = vi.spyOn(aiReviewModule, "buildCanonicalJudgePrompt").mockReturnValue("ENTIRELY DIFFERENT PROMPT TEXT"); + try { + const after = await aiReviewCacheInputFingerprint(baseInput()); + expect(after).not.toBe(before); + } finally { + spy.mockRestore(); + } + }); + + it("REGRESSION: the prompt VERSION participates too, so a deliberate bump also invalidates", async () => { + const before = await aiReviewCacheInputFingerprint(baseInput()); + const spy = vi.spyOn(aiReviewModule, "REVIEW_PROMPT_VERSION", "get").mockReturnValue("review-prompt-v2" as typeof aiReviewModule.REVIEW_PROMPT_VERSION); + try { + expect(await aiReviewCacheInputFingerprint(baseInput())).not.toBe(before); + } finally { + spy.mockRestore(); + } + }); + + it("INVARIANT: an unchanged prompt and unchanged inputs still produce a STABLE fingerprint", async () => { + // The fix must not destroy the cache: identical inputs must keep hitting, or every review re-pays. + expect(await aiReviewCacheInputFingerprint(baseInput())).toBe(await aiReviewCacheInputFingerprint(baseInput())); + }); + + it("INVARIANT: the version constant was bumped, so rows written under the incomplete fingerprint are dead", () => { + expect(AI_REVIEW_CACHE_INPUT_VERSION).not.toBe("ai-review-input:v6"); + }); +}); diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 68856c9a6d..763a0aca3f 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -5308,3 +5308,33 @@ describe("#8833: enforced boundaries between model judgment and deterministic fa expect(parseReviewConfidence(-2)).toBe(0); }); }); + +// #9478: runWorkersOpinion iterates [primary, fallback] internally, and ReviewerOpinionOutcome carried no model +// identity -- so a fallback-produced opinion was recorded as a PRIMARY vote. Those votes become reviewer_vote +// audit events and feed recordRoutingShadow's evidence-weighted routing track records (#8229) plus +// scoreJudgmentAgreement's contribution to decision-record confidence, so the calibration data was quietly +// wrong whenever the primary failed over. The doc claimed slot<->model was "unambiguous by construction" -- +// true for the tie-break slot SWAP, false for in-slot fallback. +describe("reviewer vote attribution (#9478)", () => { + it("REGRESSION: a fallback-produced review is attributed to the FALLBACK model, not the primary", async () => { + const run = vi.fn(async (model: string) => { + if (model === "primary") throw new Error("subscription_cli_timeout"); + return { response: reviewJson() }; + }); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const diagnostics: Array<{ status: string; model: string }> = []; + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + + expect(parsed.review).not.toBeNull(); + expect(parsed.producedBy).toBe("fallback"); // NOT "primary" + }); + + it("INVARIANT: a primary-produced review is still attributed to the primary", async () => { + const run = vi.fn(async () => ({ response: reviewJson() })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const diagnostics: Array<{ status: string; model: string }> = []; + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + + expect(parsed.producedBy).toBe("primary"); + }); +}); diff --git a/test/unit/selfhost-ai.test.ts b/test/unit/selfhost-ai.test.ts index 422e801d54..3e0198b191 100644 --- a/test/unit/selfhost-ai.test.ts +++ b/test/unit/selfhost-ai.test.ts @@ -22,6 +22,7 @@ import { assertNoLegacySharedAiEnv, buildProvider, claudeErrorStatus, codexError import { labelSelfHostReviewerModel, labelSelfHostReviewerModels } from "../../src/selfhost/ai-config"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import { initPostHog, resetPostHogForTest } from "../../src/selfhost/posthog"; +import { ollamaContextOptions, ollamaNumCtx } from "../../src/selfhost/ai"; describe("resolveModel (#979 — never leak the Workers-AI default to a self-host backend)", () => { const WORKERS_DEFAULT = "@cf/meta/llama-3.1-8b-instruct-fp8-fast"; @@ -2485,3 +2486,38 @@ describe("shouldWarnRagEmbedUnavailable (#8765 boot preflight)", () => { expect(shouldWarnRagEmbedUnavailable({ LOOPOVER_REVIEW_RAG: "true" })).toBe(false); }); }); + +// #9478: Ollama silently LEFT-TRUNCATES anything past num_ctx (typically 4k-8k by default) and then answers +// confidently over whatever survived -- which is the TAIL of the prompt, i.e. the context sections rather than +// the diff. The review path sends up to 120k chars of diff plus a 240k-char aggregate context budget, so on the +// fallback provider a review could be produced from a fraction of the change while carrying the SAME blocker +// authority and confidence semantics as the primary, with nothing surfacing which provider decided. +describe("ollama context window (#9478)", () => { + afterEach(() => { delete process.env["OLLAMA_NUM_CTX"]; }); + + it("REGRESSION: sends an explicit num_ctx for the ollama provider", () => { + expect(ollamaContextOptions("ollama", {})).toEqual({ options: { num_ctx: ollamaNumCtx() } }); + }); + + it("INVARIANT: sends nothing for other OpenAI-compatible providers, which may reject an unknown options key", () => { + expect(ollamaContextOptions("openai", {})).toEqual({}); + expect(ollamaContextOptions("openai-compatible", {})).toEqual({}); + expect(ollamaContextOptions(undefined, {})).toEqual({}); + }); + + it("INVARIANT: an explicit caller providerOptions always wins (the vision path sets its own num_ctx)", () => { + const explicit = { num_ctx: 4096, temperature: 0 }; + expect(ollamaContextOptions("ollama", { providerOptions: explicit })).toEqual({ options: explicit }); + expect(ollamaContextOptions("openai", { providerOptions: explicit })).toEqual({ options: explicit }); + }); + + it("is overridable, since the window trades GPU memory against how much of a large diff the model can see", () => { + process.env["OLLAMA_NUM_CTX"] = "65536"; + expect(ollamaNumCtx()).toBe(65_536); + }); + + it.each([[""], ["not-a-number"], ["0"], ["-1"]])("falls back to a sane default for %s", (value) => { + process.env["OLLAMA_NUM_CTX"] = value; + expect(ollamaNumCtx()).toBeGreaterThan(8_192); // must exceed the truncation-prone stock defaults + }); +});