Skip to content

Commit b9f049a

Browse files
committed
fix(ai): make prompt drift a cache miss, bound the Ollama context, and attribute votes to the producing model (#9477, #9478)
getCachedAiReview reuses a cacheable row with NO maxAgeMs and replays 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 one -- so a PR closed or held under the pre-#9074 false-consensus rule re-gated at the same head to the SAME stale finding and the corrected logic never ran. The fingerprint now folds in REVIEW_PROMPT_VERSION and a digest of the canonical judge prompt, both pure and available before the call, so drift is an automatic miss rather than something a human must remember. Bumped to v7 to kill rows written under the incomplete fingerprint. Ollama silently left-truncates past num_ctx (typically 4k-8k by default) and then answers confidently over whatever survived -- 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 context budget, so on the fallback provider a review could come from a fraction of the change while carrying the same blocker authority and confidence semantics as the primary. An explicit num_ctx is now sent for the ollama provider only, since other OpenAI-compatible servers may reject an unknown options key, and an explicit caller providerOptions still wins. runWorkersOpinion iterates [primary, fallback] internally and its outcome carried no model identity, so a fallback-produced review was recorded as a PRIMARY vote -- poisoning the reviewer_vote audit events, #8229's routing track records, and scoreJudgmentAgreement's contribution to decision-record confidence. The doc's claim that slot-to-model is unambiguous by construction holds for the tie-break swap, not for in-slot fallback.
1 parent d1c770c commit b9f049a

6 files changed

Lines changed: 170 additions & 6 deletions

File tree

src/review/ai-review-cache-input.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import type {
44
SelfHostAiModelConfig,
55
} from "../signals/focus-manifest";
66
import { sha256Hex } from "../utils/crypto";
7+
import { buildCanonicalJudgePrompt, REVIEW_PROMPT_VERSION } from "../services/ai-review";
78

89
// Bumped v1→v2 (#2995): `features` gained a `cultureProfile` member. Bumped v2→v3 (#2182-#2186): `features`
910
// gained an `impactMap` member. Bumped v3→v4 (#3902): `selfHostAiModelOverride` gained ollamaModel/openaiModel/
@@ -14,7 +15,8 @@ import { sha256Hex } from "../utils/crypto";
1415
// codexFirstOutputTimeoutMs members. Every prior cached review's fingerprint was computed without those keys,
1516
// so bumping the version guarantees a clean cache miss on the first review after upgrade rather than silently
1617
// reusing a hash computed under a different payload shape.
17-
export const AI_REVIEW_CACHE_INPUT_VERSION = "ai-review-input:v6";
18+
// #9477: bumped v6 -> v7 to invalidate every row written under the incomplete fingerprint below.
19+
export const AI_REVIEW_CACHE_INPUT_VERSION = "ai-review-input:v7";
1820

1921
// #regate-churn (root cause, confirmed in production): this fingerprint USED to also hash the PR's live
2022
// `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 = {
130132
export async function aiReviewCacheInputFingerprint(input: AiReviewCacheInput): Promise<string> {
131133
const payload = {
132134
version: AI_REVIEW_CACHE_INPUT_VERSION,
135+
// #9477: the fingerprint must cover what the model is ASKED, not just the change it is asked about.
136+
//
137+
// getCachedAiReview reuses a cacheable=1 row with NO maxAgeMs, replaying its findings verbatim --
138+
// including a severity:"critical" ai_consensus_defect. AI_REVIEW_CACHE_INPUT_VERSION was hand-bumped and
139+
// last moved for #8364, but #8789, #8791, #8833, #8845, #8961, #9035, #9074, #9087, #9114, #9145 and #9445
140+
// all changed prompt text or verdict logic WITHOUT a bump. So a PR closed or held under (say) the pre-#9074
141+
// "false consensus" rule re-gated at the same head to the SAME stale finding, and the corrected logic
142+
// never ran for any already-reviewed head.
143+
//
144+
// Folding in the prompt version AND a digest of the canonical judge prompt makes drift an automatic cache
145+
// MISS instead of something a human has to remember. Both are pure and available before the call.
146+
promptVersion: REVIEW_PROMPT_VERSION,
147+
promptDigest: await sha256Hex(buildCanonicalJudgePrompt()),
133148
title: input.title,
134149
body: input.body ?? null,
135150
mode: input.mode,

src/selfhost/ai.ts

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -378,7 +378,15 @@ export function createOpenAiCompatibleAi(opts: {
378378
messages: toMessages(options).map((message) => ({ role: message.role, content: toOpenAiMessageContent(message.content) })),
379379
max_tokens: options.max_tokens,
380380
temperature: options.temperature,
381-
...(options.providerOptions ? { options: options.providerOptions } : {}),
381+
// #9478: Ollama silently LEFT-TRUNCATES anything past its context window (num_ctx, typically 4k-8k
382+
// by default) and then answers confidently over whatever survived -- which is the TAIL of the prompt,
383+
// i.e. the context sections rather than the diff. The review path sends up to 120k chars of diff plus
384+
// a 240k-char aggregate context budget, so on the fallback provider a review could be produced from a
385+
// fraction of the change while carrying the SAME blocker authority and the same confidence semantics
386+
// as the primary. Nothing surfaced which provider decided. Send an explicit num_ctx sized for the
387+
// review prompt so the server allocates the window instead of quietly dropping the diff; an explicit
388+
// caller-supplied providerOptions still wins.
389+
...ollamaContextOptions(opts.providerName, options),
382390
// #8790: force JSON mode when the caller declared a JSON contract — Ollama's and vLLM's
383391
// OpenAI-compatible layers both honor it; servers that don't get the 400-fallback below.
384392
...(withResponseFormat && options.responseFormat === "json_object" ? { response_format: { type: "json_object" } } : {}),
@@ -711,6 +719,29 @@ function buildAiUsage(fields: {
711719
* rows to a real provider instead of leaving them permanently unattributed. Recognizes the bundled
712720
* docker-compose `ollama` service and OpenAI's own endpoint; anything else (vLLM, LM Studio, a custom
713721
* hostname) is the honest generic "openai-compatible" bucket rather than a guessed, possibly-wrong label. */
722+
/**
723+
* #9478: the `options` bag sent to an Ollama-compatible server, carrying an explicit context window.
724+
*
725+
* Only applied for the `ollama` provider -- other OpenAI-compatible servers may reject an unknown `options`
726+
* key, and only Ollama has the silent-truncation behaviour this guards against. A caller that supplies its own
727+
* providerOptions keeps full control (the vision path already sets num_ctx itself).
728+
*/
729+
export function ollamaContextOptions(
730+
providerName: "ollama" | "openai" | "openai-compatible" | undefined,
731+
options: { providerOptions?: Record<string, unknown> | undefined },
732+
): { options?: Record<string, unknown> } {
733+
if (options.providerOptions) return { options: options.providerOptions };
734+
if (providerName !== "ollama") return {};
735+
return { options: { num_ctx: ollamaNumCtx() } };
736+
}
737+
738+
/** Context window requested from Ollama for review-sized prompts. Overridable because it trades GPU memory
739+
* against how much of a large diff the model can actually see. */
740+
export function ollamaNumCtx(): number {
741+
const raw = Number(process.env["OLLAMA_NUM_CTX"] ?? "");
742+
return Number.isFinite(raw) && raw > 0 ? Math.floor(raw) : 32_768;
743+
}
744+
714745
export function providerNameFromBaseUrl(baseUrl: string | undefined): "ollama" | "openai" | "openai-compatible" {
715746
let hostname = "";
716747
try {

src/services/ai-review.ts

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -573,6 +573,12 @@ export function parsedReviewModelIds(diagnostics: readonly AiReviewDiagnostic[])
573573
type ReviewerOpinionOutcome = {
574574
review: ModelReview | null;
575575
fallbackNote?: string | undefined;
576+
/** #9478: the model that actually PRODUCED this review. runWorkersOpinion iterates [primary, fallback]
577+
* internally, so a fallback-produced opinion was previously recorded as a primary vote -- poisoning the
578+
* reviewer_vote audit events, recordRoutingShadow's evidence-weighted routing track records (#8229), and
579+
* scoreJudgmentAgreement's contribution to decision-record confidence. The doc claimed slot<->model was
580+
* "unambiguous by construction", which holds for the tie-break slot SWAP but not for in-slot fallback. */
581+
producedBy?: string | undefined;
576582
};
577583

578584
type AiGatewayOptions = { gateway?: { id: string } };
@@ -1485,6 +1491,8 @@ async function runWorkersOpinion(
14851491
// ONLY for the case where every attempt across every model comes back this way -- degrades to exactly today's
14861492
// behavior in that (expected to be rare) worst case, never worse.
14871493
let bestIncompleteReview: ModelReview | null = null;
1494+
// #9478: which model produced the last-resort candidate, so its vote is attributed correctly too.
1495+
let bestIncompleteReviewModel: string | undefined;
14881496
const models = fallback && fallback !== primary ? [primary, fallback] : [primary];
14891497
for (const [modelIndex, model] of models.entries()) {
14901498
if (modelIndex > 0) {
@@ -1578,12 +1586,13 @@ async function runWorkersOpinion(
15781586
}
15791587
if (parsed && parsed.assessment.trim() !== "") {
15801588
diagnostics.push({ model, attempt, status: "parsed", responseChars: text.length, hasJsonObject: Boolean(extractLastJsonObject(text)), ...usageFields });
1581-
return { review: parsed };
1589+
return { review: parsed, producedBy: model };
15821590
}
15831591
if (parsed) {
15841592
// Valid JSON, real blockers/nits/suggestions, but the REQUIRED assessment came back empty --
15851593
// keep it as a last-resort candidate and retry for a real one instead of accepting immediately.
15861594
bestIncompleteReview = parsed;
1595+
bestIncompleteReviewModel = model;
15871596
diagnostics.push({ model, attempt, status: "missing_assessment", responseChars: text.length, hasJsonObject: true, ...usageFields });
15881597
console.warn(
15891598
JSON.stringify({
@@ -1701,7 +1710,7 @@ async function runWorkersOpinion(
17011710
nitsCount: bestIncompleteReview.nits.length,
17021711
}),
17031712
);
1704-
return { review: bestIncompleteReview };
1713+
return { review: bestIncompleteReview, ...(bestIncompleteReviewModel ? { producedBy: bestIncompleteReviewModel } : {}) };
17051714
}
17061715
return { review: null };
17071716
}
@@ -2971,8 +2980,10 @@ export async function runLoopOverAiReview(
29712980
if (a.fallbackNote) fallbackNotes.push(a.fallbackNote);
29722981
if (b.fallbackNote) fallbackNotes.push(b.fallbackNote);
29732982
// #8229 stage 0: attach votes HERE, where slot↔model is unambiguous by construction.
2974-
if (a.review) reviewerVotes.push({ reviewer: primary.model, votedFail: a.review.blockers.length > 0 });
2975-
if (b.review) reviewerVotes.push({ reviewer: secondary.model, votedFail: b.review.blockers.length > 0 });
2983+
// #9478: attribute to the model that ACTUALLY produced the review, falling back to the slot's configured
2984+
// model only when the outcome carries no producer (an unparseable/never-ran slot has no vote anyway).
2985+
if (a.review) reviewerVotes.push({ reviewer: a.producedBy ?? primary.model, votedFail: a.review.blockers.length > 0 });
2986+
if (b.review) reviewerVotes.push({ reviewer: b.producedBy ?? secondary.model, votedFail: b.review.blockers.length > 0 });
29762987
secondReview = b.review;
29772988
// Combine per the configured strategy (#dual-ai-combiner). Default `consensus` is byte-identical to the
29782989
// historical logic: block only on agreement, lone blocker → split, a missing opinion → inconclusive

test/unit/ai-review-cache-input.test.ts

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { describe, expect, it, vi } from "vitest";
2+
import * as aiReviewModule from "../../src/services/ai-review";
13
import {
24
AI_REVIEW_CACHE_INPUT_VERSION,
35
aiReviewCacheInputFingerprint,
@@ -351,3 +353,42 @@ describe("aiReviewCacheInputFingerprint", () => {
351353
expect(repeated).toBe(original);
352354
});
353355
});
356+
357+
// #9477: getCachedAiReview reuses a cacheable=1 row with NO maxAgeMs, replaying its findings verbatim --
358+
// including a severity:"critical" ai_consensus_defect. AI_REVIEW_CACHE_INPUT_VERSION was hand-bumped and last
359+
// moved for #8364, but #8789, #8791, #8833, #8845, #8961, #9035, #9074, #9087, #9114, #9145 and #9445 all
360+
// changed prompt text or verdict logic WITHOUT a bump. So a PR closed or held under (say) the pre-#9074 "false
361+
// consensus" rule re-gated at the same head to the SAME stale finding, and the fixed logic never ran for any
362+
// already-reviewed head. The fingerprint must cover what the model is ASKED, not only the change it is asked
363+
// about -- and it must do so structurally, not by relying on a human remembering a constant.
364+
describe("prompt-drift is an automatic cache miss (#9477)", () => {
365+
it("REGRESSION: a change to the canonical judge prompt changes the fingerprint", async () => {
366+
const before = await aiReviewCacheInputFingerprint(baseInput());
367+
const spy = vi.spyOn(aiReviewModule, "buildCanonicalJudgePrompt").mockReturnValue("ENTIRELY DIFFERENT PROMPT TEXT");
368+
try {
369+
const after = await aiReviewCacheInputFingerprint(baseInput());
370+
expect(after).not.toBe(before);
371+
} finally {
372+
spy.mockRestore();
373+
}
374+
});
375+
376+
it("REGRESSION: the prompt VERSION participates too, so a deliberate bump also invalidates", async () => {
377+
const before = await aiReviewCacheInputFingerprint(baseInput());
378+
const spy = vi.spyOn(aiReviewModule, "REVIEW_PROMPT_VERSION", "get").mockReturnValue("review-prompt-v2" as typeof aiReviewModule.REVIEW_PROMPT_VERSION);
379+
try {
380+
expect(await aiReviewCacheInputFingerprint(baseInput())).not.toBe(before);
381+
} finally {
382+
spy.mockRestore();
383+
}
384+
});
385+
386+
it("INVARIANT: an unchanged prompt and unchanged inputs still produce a STABLE fingerprint", async () => {
387+
// The fix must not destroy the cache: identical inputs must keep hitting, or every review re-pays.
388+
expect(await aiReviewCacheInputFingerprint(baseInput())).toBe(await aiReviewCacheInputFingerprint(baseInput()));
389+
});
390+
391+
it("INVARIANT: the version constant was bumped, so rows written under the incomplete fingerprint are dead", () => {
392+
expect(AI_REVIEW_CACHE_INPUT_VERSION).not.toBe("ai-review-input:v6");
393+
});
394+
});

test/unit/ai-review.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5308,3 +5308,33 @@ describe("#8833: enforced boundaries between model judgment and deterministic fa
53085308
expect(parseReviewConfidence(-2)).toBe(0);
53095309
});
53105310
});
5311+
5312+
// #9478: runWorkersOpinion iterates [primary, fallback] internally, and ReviewerOpinionOutcome carried no model
5313+
// identity -- so a fallback-produced opinion was recorded as a PRIMARY vote. Those votes become reviewer_vote
5314+
// audit events and feed recordRoutingShadow's evidence-weighted routing track records (#8229) plus
5315+
// scoreJudgmentAgreement's contribution to decision-record confidence, so the calibration data was quietly
5316+
// wrong whenever the primary failed over. The doc claimed slot<->model was "unambiguous by construction" --
5317+
// true for the tie-break slot SWAP, false for in-slot fallback.
5318+
describe("reviewer vote attribution (#9478)", () => {
5319+
it("REGRESSION: a fallback-produced review is attributed to the FALLBACK model, not the primary", async () => {
5320+
const run = vi.fn(async (model: string) => {
5321+
if (model === "primary") throw new Error("subscription_cli_timeout");
5322+
return { response: reviewJson() };
5323+
});
5324+
const env = createTestEnv({ AI: { run } as unknown as Ai });
5325+
const diagnostics: Array<{ status: string; model: string }> = [];
5326+
const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never);
5327+
5328+
expect(parsed.review).not.toBeNull();
5329+
expect(parsed.producedBy).toBe("fallback"); // NOT "primary"
5330+
});
5331+
5332+
it("INVARIANT: a primary-produced review is still attributed to the primary", async () => {
5333+
const run = vi.fn(async () => ({ response: reviewJson() }));
5334+
const env = createTestEnv({ AI: { run } as unknown as Ai });
5335+
const diagnostics: Array<{ status: string; model: string }> = [];
5336+
const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never);
5337+
5338+
expect(parsed.producedBy).toBe("primary");
5339+
});
5340+
});

test/unit/selfhost-ai.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import { assertNoLegacySharedAiEnv, buildProvider, claudeErrorStatus, codexError
2222
import { labelSelfHostReviewerModel, labelSelfHostReviewerModels } from "../../src/selfhost/ai-config";
2323
import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics";
2424
import { initPostHog, resetPostHogForTest } from "../../src/selfhost/posthog";
25+
import { ollamaContextOptions, ollamaNumCtx } from "../../src/selfhost/ai";
2526

2627
describe("resolveModel (#979 — never leak the Workers-AI default to a self-host backend)", () => {
2728
const WORKERS_DEFAULT = "@cf/meta/llama-3.1-8b-instruct-fp8-fast";
@@ -2485,3 +2486,38 @@ describe("shouldWarnRagEmbedUnavailable (#8765 boot preflight)", () => {
24852486
expect(shouldWarnRagEmbedUnavailable({ LOOPOVER_REVIEW_RAG: "true" })).toBe(false);
24862487
});
24872488
});
2489+
2490+
// #9478: Ollama silently LEFT-TRUNCATES anything past num_ctx (typically 4k-8k by default) and then answers
2491+
// confidently over whatever survived -- which is the TAIL of the prompt, i.e. the context sections rather than
2492+
// the diff. The review path sends up to 120k chars of diff plus a 240k-char aggregate context budget, so on the
2493+
// fallback provider a review could be produced from a fraction of the change while carrying the SAME blocker
2494+
// authority and confidence semantics as the primary, with nothing surfacing which provider decided.
2495+
describe("ollama context window (#9478)", () => {
2496+
afterEach(() => { delete process.env["OLLAMA_NUM_CTX"]; });
2497+
2498+
it("REGRESSION: sends an explicit num_ctx for the ollama provider", () => {
2499+
expect(ollamaContextOptions("ollama", {})).toEqual({ options: { num_ctx: ollamaNumCtx() } });
2500+
});
2501+
2502+
it("INVARIANT: sends nothing for other OpenAI-compatible providers, which may reject an unknown options key", () => {
2503+
expect(ollamaContextOptions("openai", {})).toEqual({});
2504+
expect(ollamaContextOptions("openai-compatible", {})).toEqual({});
2505+
expect(ollamaContextOptions(undefined, {})).toEqual({});
2506+
});
2507+
2508+
it("INVARIANT: an explicit caller providerOptions always wins (the vision path sets its own num_ctx)", () => {
2509+
const explicit = { num_ctx: 4096, temperature: 0 };
2510+
expect(ollamaContextOptions("ollama", { providerOptions: explicit })).toEqual({ options: explicit });
2511+
expect(ollamaContextOptions("openai", { providerOptions: explicit })).toEqual({ options: explicit });
2512+
});
2513+
2514+
it("is overridable, since the window trades GPU memory against how much of a large diff the model can see", () => {
2515+
process.env["OLLAMA_NUM_CTX"] = "65536";
2516+
expect(ollamaNumCtx()).toBe(65_536);
2517+
});
2518+
2519+
it.each([[""], ["not-a-number"], ["0"], ["-1"]])("falls back to a sane default for %s", (value) => {
2520+
process.env["OLLAMA_NUM_CTX"] = value;
2521+
expect(ollamaNumCtx()).toBeGreaterThan(8_192); // must exceed the truncation-prone stock defaults
2522+
});
2523+
});

0 commit comments

Comments
 (0)