From ead3eedb10ce01b2bdfacdf1bad428faee339e3a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 05:23:33 -0700 Subject: [PATCH] =?UTF-8?q?fix(orb):=20enforce=20the=20model/fact=20bounda?= =?UTF-8?q?ry=20=E2=80=94=20CI=20claims=20demoted=20at=20parse,=20silence?= =?UTF-8?q?=20is=20not=20certainty?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A full inventory of the AI-review prompt (posted on #8833) found the two highest-risk gaps between what the prompt REQUESTS and what the parser ENFORCES: 1. CI adjudication. The system prompt forbids the model from judging CI state twice in prose, then hands it the CI table for context -- with no parser enforcement. A blocker string naming CI still became the gate's public 'why blocked' text and drove willClose, even though buildCheckAggregate already owns that fact deterministically (the exact contradiction class behind the 59 corrected verdicts on #8825). demoteCiClaimBlockers now demotes run-state claims to annotated nits at BOTH parse sites (Workers + BYOK provider), with a bounded-vocabulary pattern that never touches code-content judgment ('this breaks the build contract' is a prediction about the diff and stays a blocker; 'the build is failing' is a report about CI and does not). Demotions log ai_review_ci_claim_demoted -- a rising rate is a prompt-regression signal. 2. Unstated confidence read as maximum certainty. parseReviewConfidence defaulted absent/garbage to 1.0, so a review that STATED no confidence skipped every low-confidence safeguard and drove a straight close. It now defaults to CONFIDENCE_WHEN_UNSTATED (0.5, under every sane close floor), routing to the low-confidence disposition -- still blocks, a human decides the close. Advances #8833. Remaining extraction items from the inventory (structured verifiable blocker schema, moving test-evidence/size/ staleness out of the prompt, non-negotiable enrichment facts, explicit unknown in the finding schema) are recorded on the issue. --- src/services/ai-review.ts | 57 ++++++++++++++++-- test/unit/ai-review.test.ts | 113 +++++++++++++++++++++++++++++++++--- 2 files changed, 157 insertions(+), 13 deletions(-) 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); + }); +});