fix(orb): enforce the model/fact boundary — CI claims demoted at parse, silence is not certainty

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: {

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,29 +2251,32 @@ 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}',
     );
     expect(withConfidence?.confidence).toBe(0.4); // present value used
     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<string, unknown>) => {
@@ -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);
+  });
+});