diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 964396231e..e6cfeb02c3 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -745,15 +745,48 @@ export function extractLastJsonObject(text: string): string | null { * parser and the combiners so the fallback is identical everywhere. */ export const DEFAULT_REVIEW_CONFIDENCE = 1; -/** Coerce a model's `confidence` field to a calibrated value in [0,1] (#8). A finite number is clamped into range; - * anything else (absent, NaN/±Infinity — which JSON can't even encode — string, etc.) falls back to 1.0 so the gate - * degrades to today's always-block behavior rather than silently un-blocking a real defect. PURE. */ +/** #8833: the fallback when a model states NO usable confidence at all. The old fallback was 1.0 — "the + * model said nothing" read as MAXIMUM certainty, so a review missing the field skipped every low-confidence + * safeguard (#4603's disposition, the close-confidence floor) and drove a straight close. 0.5 sits below + * every sane close floor (default 0.93), so an unstated confidence routes to the low-confidence disposition + * (default hold_for_review — still blocks, but a human decides the close) instead of asserting certainty + * the model never claimed. A STATED confidence is untouched. */ +export const CONFIDENCE_WHEN_UNSTATED = 0.5; + +/** Coerce a model's `confidence` field to a calibrated value in [0,1] (#8). A finite number is clamped into + * range; anything else (absent, NaN/±Infinity — which JSON can't even encode — string, etc.) falls back to + * {@link CONFIDENCE_WHEN_UNSTATED} — silence is not certainty (#8833). PURE. */ export function parseReviewConfidence(value: unknown): number { if (typeof value !== "number" || !Number.isFinite(value)) - return DEFAULT_REVIEW_CONFIDENCE; + return CONFIDENCE_WHEN_UNSTATED; return Math.min(1, Math.max(0, value)); } +/** #8833: vocabulary of claims the model is FORBIDDEN to adjudicate because a deterministic owner already + * decides them — CI/build/test-run state comes from buildCheckAggregate, never from a model's reading of + * the CI table the prompt shows it for context. The prompt has always SAID this (twice); this makes it + * enforced instead of requested. Deliberately narrow: matches run/state phrasing ("CI is failing", "build + * failed", "tests are failing/red") — never code-content phrasing ("this breaks the build" as a prediction + * about the DIFF is judgment, "the build is failing" as a report about CI is not. The pattern requires the + * run-state verb shape). */ +export const CI_CLAIM_PATTERN = /\b(ci|pipeline|workflow|checks?|builds?|type-?checks?|tests?(\s+(run|suite))?)\b[^.]{0,40}\b(is|are|was|were|still)?\s*(failing|failed|red|broken|not\s+passing|pending|in\s+progress)\b/i; + +/** #8833: deterministically demote CI-state blockers to nits. Returns the demoted claims so callers can + * audit how often the model attempts the forbidden adjudication (a rising rate is a prompt-regression + * signal). PURE; never touches non-CI claims; never touches nits/suggestions. */ +export function demoteCiClaimBlockers(review: ModelReview): { review: ModelReview; demoted: string[] } { + const demoted = review.blockers.filter((blocker) => CI_CLAIM_PATTERN.test(blocker)); + if (demoted.length === 0) return { review, demoted }; + return { + review: { + ...review, + blockers: review.blockers.filter((blocker) => !CI_CLAIM_PATTERN.test(blocker)), + nits: [...review.nits, ...demoted.map((claim) => `${claim} (demoted: CI state is decided deterministically, not by review)`)], + }, + demoted, + }; +} + /** Parse a model's JSON review into a normalized {@link ModelReview}, or null when unparseable. */ export function parseModelReview(text: string): ModelReview | null { const jsonText = extractLastJsonObject(text); @@ -1317,7 +1350,13 @@ async function runWorkersOpinion( break; } lastRawText = text; - const parsed = parseModelReview(text); + const parsedRaw = parseModelReview(text); + // #8833: enforce the CI-adjudication ban at parse time — the prompt REQUESTS it, this guarantees it. + const demotion = parsedRaw ? demoteCiClaimBlockers(parsedRaw) : null; + const parsed = demotion?.review ?? null; + if (demotion && demotion.demoted.length > 0) { + console.warn(JSON.stringify({ level: "warn", event: "ai_review_ci_claim_demoted", model, count: demotion.demoted.length })); + } if (parsed && parsed.assessment.trim() !== "") { diagnostics.push({ model, attempt, status: "parsed", responseChars: text.length, hasJsonObject: Boolean(extractLastJsonObject(text)), ...usageFields }); return { review: parsed }; @@ -1646,7 +1685,13 @@ async function runProviderReview( if (failure) return { review: null, failure, diagnostic: { model, attempt: 0, status: "provider_error", error: failure } }; /* v8 ignore next -- callAiProvider returns a string for every non-failure response; null is a type-level guard. */ const textValue = text ?? ""; - const review = textValue ? parseModelReview(textValue) : null; + const parsedProviderReview = textValue ? parseModelReview(textValue) : null; + // #8833: same CI-adjudication enforcement as the Workers path — no parse route escapes it. + const providerDemotion = parsedProviderReview ? demoteCiClaimBlockers(parsedProviderReview) : null; + if (providerDemotion && providerDemotion.demoted.length > 0) { + console.warn(JSON.stringify({ level: "warn", event: "ai_review_ci_claim_demoted", provider: providerKey.provider, count: providerDemotion.demoted.length })); + } + const review = providerDemotion?.review ?? null; return { review, diagnostic: { diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index f104824406..d32ca4386a 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { + CONFIDENCE_WHEN_UNSTATED, __aiReviewInternals, BEST_REVIEW_MODELS, buildTestEvidencePromptSection, @@ -252,6 +253,47 @@ describe("runLoopOverAiReview gating", () => { expect(run).not.toHaveBeenCalled(); }); + it("#8833: the BYOK provider path demotes a CI-state blocker too — no parse route escapes the enforcement", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const fetchMock = vi.fn( + async () => + new Response( + JSON.stringify({ + content: [ + { + type: "text", + text: '{"assessment":"provider view","blockers":["The tests are failing on CI","Race in src/lock.ts"],"nits":[],"suggestions":[]}', + }, + ], + }), + { status: 200 }, + ), + ); + vi.stubGlobal("fetch", fetchMock); + const env = createTestEnv({ + AI: { run: vi.fn() } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "1", + AI_BYOK_DAILY_REPO_LIMIT: "5", + }); + const result = await runLoopOverAiReview(env, { ...baseInput, providerKey: { provider: "anthropic", key: "sk-ant-secret" } }); + expect(result.status).toBe("ok"); + if (result.status === "ok") { + // Single-provider BYOK is advisory-only (no consensus defect) — the observable contract is the + // rendered advisory: the CI claim appears ONLY in the Nits section, annotated, never under Blockers. + const notes = result.advisoryNotes ?? ""; + const blockersSection = notes.split("**Nits")[0] ?? notes; + expect(blockersSection).toContain("Race in src/lock.ts"); + expect(blockersSection).not.toContain("tests are failing"); + expect(notes).toContain("decided deterministically"); + } + // The demotion arm executed on the provider path (its log fired) — the strong behavioral assertions live + // in the pure demoteCiClaimBlockers tests and the workers-path test above. + expect(warn.mock.calls.some(([line]) => String(line).includes("ai_review_ci_claim_demoted"))).toBe(true); + warn.mockRestore(); + }); + it("enforces a separate per-repo daily quota before BYOK provider calls", async () => { const fetchMock = vi.fn( async () => @@ -2209,17 +2251,20 @@ describe("pure helpers", () => { expect(parsed?.blockers).toEqual(["X in src/a.ts"]); }); - it("parseReviewConfidence uses a present value, falls back to 1.0 when absent/garbage, and clamps to [0,1] (#8)", () => { + it("parseReviewConfidence uses a present value, falls back to CONFIDENCE_WHEN_UNSTATED when absent/garbage, and clamps to [0,1] (#8, #8833)", () => { expect(parseReviewConfidence(0.75)).toBe(0.75); // present, in range → used verbatim expect(parseReviewConfidence(0)).toBe(0); // explicit zero is honored (not treated as falsy/absent) - expect(parseReviewConfidence(undefined)).toBe(1); // absent → fallback 1.0 - expect(parseReviewConfidence("0.5")).toBe(1); // non-number → fallback 1.0 - expect(parseReviewConfidence(Number.NaN)).toBe(1); // non-finite → fallback 1.0 + // #8833: silence is not certainty — the old 1.0 fallback made a review that STATED no confidence skip + // every low-confidence safeguard. Absent/garbage now reads as sub-floor, routing to the low-confidence + // disposition (default hold_for_review — still blocks, a human decides the close). + expect(parseReviewConfidence(undefined)).toBe(CONFIDENCE_WHEN_UNSTATED); + expect(parseReviewConfidence("0.5")).toBe(CONFIDENCE_WHEN_UNSTATED); + expect(parseReviewConfidence(Number.NaN)).toBe(CONFIDENCE_WHEN_UNSTATED); expect(parseReviewConfidence(1.7)).toBe(1); // above range → clamped to 1 expect(parseReviewConfidence(-0.3)).toBe(0); // below range → clamped to 0 }); - it("parseModelReview threads a calibrated confidence and defaults it to 1.0 when absent/unparseable (#8)", () => { + it("parseModelReview threads a calibrated confidence and defaults it to CONFIDENCE_WHEN_UNSTATED when absent/unparseable (#8, #8833)", () => { const withConfidence = parseModelReview( '{"assessment":"leak in b.ts","blockers":["Unclosed handle in src/b.ts"],"nits":[],"suggestions":[],"confidence":0.4}', ); @@ -2227,11 +2272,11 @@ describe("pure helpers", () => { const noConfidence = parseModelReview( reviewJson({ present: true, title: "Null deref in src/a.ts" }), ); - expect(noConfidence?.confidence).toBe(1); // absent → fallback 1.0 + expect(noConfidence?.confidence).toBe(CONFIDENCE_WHEN_UNSTATED); // absent → sub-floor, never certainty const garbageConfidence = parseModelReview( '{"assessment":"ok","blockers":["X in src/a.ts"],"nits":[],"suggestions":[],"confidence":"high"}', ); - expect(garbageConfidence?.confidence).toBe(1); // unparseable → fallback 1.0 + expect(garbageConfidence?.confidence).toBe(CONFIDENCE_WHEN_UNSTATED); // unparseable → sub-floor }); describe("combineReviews (#dual-ai-combiner)", () => { @@ -3191,6 +3236,19 @@ describe("pure helpers", () => { expect(run).toHaveBeenCalledTimes(1); }); + it("#8833: runWorkersOpinion demotes a CI-state blocker in the parsed review and logs the attempt", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const run = vi.fn(async () => ({ + response: '{"assessment":"looks off","blockers":["CI is failing (validate, validate-tests)","Null deref in src/a.ts"],"nits":[],"suggestions":[]}', + })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const parsed = await runWorkersOpinion(env, "@cf/x/model", "@cf/x/model", "sys", "user", 256); + expect(parsed.review?.blockers).toEqual(["Null deref in src/a.ts"]); + expect(parsed.review?.nits.some((nit) => nit.includes("decided deterministically"))).toBe(true); + expect(warn.mock.calls.some(([line]) => String(line).includes("ai_review_ci_claim_demoted"))).toBe(true); + warn.mockRestore(); + }); + it("REGRESSION (#4111): runWorkersOpinion attaches supplied images to the user message; omits them (plain string) when absent", async () => { const seenContents: unknown[] = []; const run = vi.fn(async (_model: string, options: Record) => { @@ -4755,3 +4813,44 @@ describe("REVIEW_SYSTEM_PROMPT performance-regression instruction (#2559)", () = expect(system).toContain("micro-optimization preference"); }); }); + +describe("#8833: enforced boundaries between model judgment and deterministic fact", async () => { + const { demoteCiClaimBlockers, CI_CLAIM_PATTERN, parseReviewConfidence, CONFIDENCE_WHEN_UNSTATED } = await import("../../src/services/ai-review"); + + const review = (blockers: string[], nits: string[] = []) => + ({ assessment: "a", blockers, nits, suggestions: [], confidence: 0.97, inlineFindings: [] }) as never; + + it("demotes CI-STATE claims to nits — the model reports on runs it was told not to adjudicate", () => { + const { review: out, demoted } = demoteCiClaimBlockers(review(["CI is failing (validate, validate-tests)", "The function drops the error branch"])); + expect(demoted).toEqual(["CI is failing (validate, validate-tests)"]); + expect(out.blockers).toEqual(["The function drops the error branch"]); + expect(out.nits.some((nit: string) => nit.includes("decided deterministically"))).toBe(true); + for (const claim of ["Tests are failing on main", "the build failed twice", "typecheck is still red", "workflow run is pending"]) { + expect(CI_CLAIM_PATTERN.test(claim)).toBe(true); + } + }); + + it("NEVER touches code-content judgment — predictions about the diff are the model's job", () => { + const kept = [ + "This change breaks the build contract for downstream consumers", // prediction about the DIFF, no run-state verb shape + "Missing test coverage for the error branch", + "The added check silently swallows the failure", + ]; + const { review: out, demoted } = demoteCiClaimBlockers(review(kept)); + expect(demoted).toEqual([]); + expect(out.blockers).toEqual(kept); + // Zero-demotion returns the SAME object (no pointless reallocation on the hot path). + const untouched = review(kept); + expect(demoteCiClaimBlockers(untouched).review).toBe(untouched); + }); + + it("silence is not certainty: an unstated/garbage confidence parses to CONFIDENCE_WHEN_UNSTATED, a stated one is honored", () => { + expect(parseReviewConfidence(undefined)).toBe(CONFIDENCE_WHEN_UNSTATED); + expect(parseReviewConfidence("very sure")).toBe(CONFIDENCE_WHEN_UNSTATED); + expect(parseReviewConfidence(Number.NaN)).toBe(CONFIDENCE_WHEN_UNSTATED); + expect(CONFIDENCE_WHEN_UNSTATED).toBeLessThan(0.93); // must sit under the default close floor + expect(parseReviewConfidence(0.97)).toBe(0.97); + expect(parseReviewConfidence(1.7)).toBe(1); + expect(parseReviewConfidence(-2)).toBe(0); + }); +});