From a394dda64069459166ed475d2d4f7f36bd9aeb4d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:00:35 -0700 Subject: [PATCH] =?UTF-8?q?feat(review):=20structured=20verifiable=20absen?= =?UTF-8?q?ce-claim=20schema=20=E2=80=94=20hallucinated=20absence=20can=20?= =?UTF-8?q?no=20longer=20block=20(#8833)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The flagship remaining piece of #8833. The rubric's DIFF SCOPE / TRACE-BEFORE-ASSERTING-ABSENCE rules have always REQUESTED that an absence blocker point at the visible line that breaks; this makes the discipline checkable and then ENFORCES it: - Blocker entries may now be OBJECTS {claim, kind, evidence}; the object form is REQUIRED for the ABSENCE-claim family (missing_symbol, missing_import, missing_guard, missing_handling, missing_registration -- a CLOSED set, so a novel kind string cannot smuggle a claim past verification). parseModelReview normalizes every usable entry into the existing blockers: string[] surface -- nothing downstream changes shape -- and collects absence claims into ModelReview.absenceClaims. - demoteUnverifiedAbsenceBlockers checks each claim's quoted evidence VERBATIM against the exact user prompt the call sent (leading diff +/- marker stripped; sub-8-char quotes never verify vacuously). An unverifiable claim demotes to the same verification nit the rubric prescribes; a verified quote leaves the blocker standing untouched -- the check can only remove claims the model provably could not ground. - Enforced on BOTH the Workers and BYOK parse paths, logged as ai_review_unverified_absence_demoted for prompt-regression tracking. REVIEW_PROMPT_VERSION bumps to v3 (the schema shapes the verdict surface), moving the #9477 cache fingerprint and #8222 replay keys. Closes #8833 --- src/services/ai-review.ts | 104 ++++++++++++++++- test/unit/ai-review.test.ts | 111 ++++++++++++++++++- test/unit/counterfactual-replay-core.test.ts | 2 +- 3 files changed, 211 insertions(+), 6 deletions(-) diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 5db00f067..61e4011ee 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -106,6 +106,7 @@ export const REVIEW_SYSTEM_PROMPT = [ "PERFORMANCE SEVERITY — a performance concern is a blocker ONLY when the diff introduces a genuine, visible regression with a concrete trigger (a DB query or network call moved inside a loop, a loop/fanout over input whose size the diff removed a bound on). A stylistic or micro-optimization preference ('could use a Map instead of an array', 'this could be slightly faster') is a NIT, not a blocker, even if real.", "DIFF SCOPE — the diff shows only CHANGED lines, NOT whole files. A function, variable, import, type, or symbol you do not SEE may already be defined or imported elsewhere in the same file/module. NEVER report a 'missing import', 'undefined/not-imported symbol', or 'X is not defined -> ReferenceError' as a blocker unless the diff ITSELF removes the definition or introduces the symbol without defining it anywhere shown. When you cannot confirm a symbol is missing from the visible diff, it is NOT a blocker — at most a nit ('verify X is imported/defined').", "TRACE BEFORE ASSERTING ABSENCE — this rule extends to ANY 'X is missing' blocker (a missing schema/annotation/field, a missing null/array/type guard, a missing await/error-handler, an unregistered route/tool/handler): a backfill loop, a default, an early guard, or a registration ELSEWHERE may already supply it. Before calling absence a blocker, find the line in the visible context that WOULD break and reference it; if you cannot SEE the breaking code, downgrade to a nit phrased as a verification ('confirm X is registered/guarded'), never a blocker.", + 'ABSENCE CLAIMS ARE VERIFIED MECHANICALLY — when a blocker asserts something is MISSING, you MUST use the object form in the blockers array: {"claim": "", "kind": "", "evidence": ""}. The evidence quote is checked mechanically against the material you were shown: an absence blocker whose quote is not found there is automatically demoted to a verification nit. Non-absence blockers remain plain strings.', // #8789: the bail is for MECHANICAL breakage only. The previous wording ("does not cohere with the PR // title/description") made a model bail on a VALID diff whose title merely undersold it — confirmed live // (2026-07-26, PR #8735: a "trivial test-fixture fix" title over a diff also carrying real production @@ -541,6 +542,17 @@ export type ModelReview = { * fabricates a value or a fallback band. */ valueAssessment?: { magnitude: ImprovementMagnitude; rationale: string } | undefined; + /** + * #8833 (structured verifiable blockers): the ABSENCE-family claims the model emitted in object form, + * carrying the exact visible line the model says would break (`evidence`). Populated by parseModelReview + * from object-form blocker entries; ALWAYS present (defaults []). The claims also appear as plain strings + * in `blockers` (the canonical decision surface — nothing downstream changes shape); this parallel field + * exists so demoteUnverifiedAbsenceBlockers can machine-check each claim's evidence against the material + * the model was actually shown, which is what makes the DIFF SCOPE / TRACE-BEFORE-ASSERTING-ABSENCE rules + * enforceable instead of requested. Optional (absent === []) so the many existing ModelReview literals in + * combiners/tests keep compiling — only parseModelReview writes it and only the demotion reads it. + */ + absenceClaims?: { claim: string; evidence: string }[] | undefined; }; export type AiReviewDiagnostic = { @@ -1025,6 +1037,47 @@ export function demoteStaleBaseClaimBlockers(review: ModelReview): { review: Mod }; } +/** #8833 (structured verifiable blockers): the ABSENCE-claim kinds whose object form is REQUIRED by the + * rubric, because each asserts that something is NOT there — the one claim family the confirmed + * hallucination pattern lives in (a symbol/import/guard/handler the model merely could not SEE). Every + * other kind remains free-form judgment; this list is deliberately closed so a novel kind string can never + * smuggle a claim past verification (unknown kinds are treated as plain judgment, not as absence). */ +export const ABSENCE_CLAIM_KINDS = new Set(["missing_symbol", "missing_import", "missing_guard", "missing_handling", "missing_registration"]); + +/** #8833: the minimum usable evidence quote. Shorter fragments ("x", ") {") appear in virtually any diff, so + * they would verify vacuously — a quote must be long enough to plausibly identify ONE breaking line. */ +export const ABSENCE_EVIDENCE_MIN_CHARS = 8; + +/** #8833 (structured verifiable blockers): machine-check every ABSENCE claim's quoted evidence against the + * material the model was actually shown, and demote the unverifiable ones to verification nits. + * + * The rubric's DIFF SCOPE / TRACE-BEFORE-ASSERTING-ABSENCE rules have always REQUESTED this discipline + * ("find the line that WOULD break ... if you cannot SEE the breaking code, downgrade to a nit"); the + * object-form blocker schema makes it checkable and this function makes it ENFORCED: an absence claim + * whose `evidence` does not appear verbatim in the prompt (after stripping a leading diff +/- marker) is + * exactly the "cannot see the breaking code" case, downgraded to the same verification nit the rubric + * prescribes. A verified quote leaves the blocker standing untouched — this can only ever remove claims + * the model provably could not ground, never real judgment. PURE. */ +export function demoteUnverifiedAbsenceBlockers(review: ModelReview, promptText: string): { review: ModelReview; demoted: string[] } { + const claims = review.absenceClaims ?? []; + if (claims.length === 0) return { review, demoted: [] }; + const unverified = new Set(); + for (const { claim, evidence } of claims) { + const quote = evidence.trim().replace(/^[+-]\s?/, ""); + if (quote.length < ABSENCE_EVIDENCE_MIN_CHARS || !promptText.includes(quote)) unverified.add(claim); + } + const demoted = review.blockers.filter((blocker) => unverified.has(blocker)); + if (demoted.length === 0) return { review, demoted }; + return { + review: { + ...review, + blockers: review.blockers.filter((blocker) => !unverified.has(blocker)), + nits: [...review.nits, ...demoted.map((claim) => `Verify: ${claim} (demoted: absence claim whose quoted evidence was not found in the reviewed material — the breaking line must be visible to block)`)], + }, + 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); @@ -1099,7 +1152,38 @@ export function parseModelReview(text: string): ModelReview | null { }; const assessment = typeof obj.assessment === "string" ? obj.assessment.trim() : ""; - const blockers = toList(obj.blockers); + // #8833 (structured verifiable blockers): a blocker entry may be the OBJECT form + // {claim, kind, evidence}. Every usable entry lands in `blockers` as its plain claim string — the + // canonical decision surface is unchanged — and entries whose kind is an ABSENCE claim additionally + // land in `absenceClaims` for evidence verification. Fail-safe per entry: a non-string/non-object + // entry, or an object without a usable `claim`, is dropped, exactly like toList drops non-strings. + // An unknown `kind` is kept as plain judgment but NEVER as an absence claim (closed kind set — a + // novel kind string cannot smuggle a claim past verification). + const toBlockers = (value: unknown): { blockers: string[]; absenceClaims: { claim: string; evidence: string }[] } => { + if (!Array.isArray(value)) return { blockers: [], absenceClaims: [] }; + const flat: string[] = []; + const absence: { claim: string; evidence: string }[] = []; + for (const entry of value) { + if (typeof entry === "string") { + const claim = entry.trim(); + if (claim) flat.push(claim); + continue; + } + if (entry && typeof entry === "object") { + const o = entry as Record; + const claim = typeof o.claim === "string" ? o.claim.trim() : ""; + if (!claim) continue; + flat.push(claim); + if (typeof o.kind === "string" && ABSENCE_CLAIM_KINDS.has(o.kind)) { + absence.push({ claim, evidence: typeof o.evidence === "string" ? o.evidence : "" }); + } + } + } + const kept = flat.slice(0, 6); + const keptSet = new Set(kept); + return { blockers: kept, absenceClaims: absence.filter((a) => keptSet.has(a.claim)) }; + }; + const { blockers, absenceClaims } = toBlockers(obj.blockers); const nits = toList(obj.nits); const suggestions = toList(obj.suggestions); const inlineFindings = toInlineFindings(obj.inlineFindings); @@ -1149,6 +1233,7 @@ export function parseModelReview(text: string): ModelReview | null { suggestions, inlineFindings, confidence, + absenceClaims, ...(valueAssessment ? { valueAssessment } : {}), }; } catch { @@ -1365,7 +1450,7 @@ const IMPROVEMENT_SIGNAL_SUFFIX = * that shapes the judge's verdict surface — REVIEW_SYSTEM_PROMPT, buildSystemPrompt's suffix composition, * or parseModelReview's accepted output shape. The CI replay compares base vs head canonical prompts and * only spends when the version (or the canonical text) actually changed. */ -export const REVIEW_PROMPT_VERSION = "review-prompt-v2"; // v2 (#8833): size/stale-base adjudication ban + explicit "unknown" confidence abstention +export const REVIEW_PROMPT_VERSION = "review-prompt-v3"; // v3 (#8833): structured verifiable absence-claim schema; v2: size/stale-base adjudication ban + explicit "unknown" confidence abstention /** #8222: the CANONICAL judge prompt — buildSystemPrompt with every optional suffix absent, which is the * exact base-model system prompt a default-configured repo's review runs under. The replay harness diffs @@ -1655,7 +1740,10 @@ async function runWorkersOpinion( const sizeDemotion = testEvidenceDemotion ? demoteSizeClaimBlockers(testEvidenceDemotion.review) : null; // ... and for base-staleness/rebase/conflict claims (owned by stale_base_ref + mergeable_state). const staleBaseDemotion = sizeDemotion ? demoteStaleBaseClaimBlockers(sizeDemotion.review) : null; - const parsed = staleBaseDemotion?.review ?? null; + // #8833 (structured verifiable blockers): absence claims must quote a breaking line that actually + // appears in what the model was shown -- checked against the SAME user prompt this call sent. + const absenceDemotion = staleBaseDemotion ? demoteUnverifiedAbsenceBlockers(staleBaseDemotion.review, user) : null; + const parsed = absenceDemotion?.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 })); } @@ -1671,6 +1759,9 @@ async function runWorkersOpinion( if (staleBaseDemotion && staleBaseDemotion.demoted.length > 0) { console.warn(JSON.stringify({ level: "warn", event: "ai_review_stale_base_claim_demoted", model, count: staleBaseDemotion.demoted.length })); } + if (absenceDemotion && absenceDemotion.demoted.length > 0) { + console.warn(JSON.stringify({ level: "warn", event: "ai_review_unverified_absence_demoted", model, count: absenceDemotion.demoted.length })); + } if (parsed && parsed.assessment.trim() !== "") { diagnostics.push({ model, attempt, status: "parsed", responseChars: text.length, hasJsonObject: Boolean(extractLastJsonObject(text)), ...usageFields }); return { review: parsed, producedBy: model }; @@ -2071,7 +2162,12 @@ async function runProviderReview( if (providerStaleBaseDemotion && providerStaleBaseDemotion.demoted.length > 0) { console.warn(JSON.stringify({ level: "warn", event: "ai_review_stale_base_claim_demoted", provider: providerKey.provider, count: providerStaleBaseDemotion.demoted.length })); } - const review = providerStaleBaseDemotion?.review ?? null; + // #8833 (structured verifiable blockers): same evidence verification as the Workers path. + const providerAbsenceDemotion = providerStaleBaseDemotion ? demoteUnverifiedAbsenceBlockers(providerStaleBaseDemotion.review, user) : null; + if (providerAbsenceDemotion && providerAbsenceDemotion.demoted.length > 0) { + console.warn(JSON.stringify({ level: "warn", event: "ai_review_unverified_absence_demoted", provider: providerKey.provider, count: providerAbsenceDemotion.demoted.length })); + } + const review = providerAbsenceDemotion?.review ?? null; return { review, diagnostic: { diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index eae6ac7cb..f9e31bff8 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -317,7 +317,7 @@ describe("runLoopOverAiReview gating", () => { content: [ { type: "text", - text: '{"assessment":"provider view","blockers":["The tests are failing on CI","No before/after screenshots provided for this visual change","This PR is too large and should be split into smaller PRs","The branch has merge conflicts with the base","Race in src/lock.ts"],"nits":[],"suggestions":[]}', + text: '{"assessment":"provider view","blockers":["The tests are failing on CI","No before/after screenshots provided for this visual change","This PR is too large and should be split into smaller PRs","The branch has merge conflicts with the base",{"claim":"Missing guard before items[0] in src/list.ts","kind":"missing_guard","evidence":"+ const first = items[0].id;"},"Race in src/lock.ts"],"nits":[],"suggestions":[]}', }, ], }), @@ -353,6 +353,9 @@ describe("runLoopOverAiReview gating", () => { // #8833: the size and stale-base bans hold on the provider path too. expect(warn.mock.calls.some(([line]) => String(line).includes("ai_review_size_claim_demoted"))).toBe(true); expect(warn.mock.calls.some(([line]) => String(line).includes("ai_review_stale_base_claim_demoted"))).toBe(true); + // #8833: absence-evidence verification holds on the provider path too — the quoted line is nowhere in + // this PR's prompt, so the object-form claim demotes instead of blocking. + expect(warn.mock.calls.some(([line]) => String(line).includes("ai_review_unverified_absence_demoted"))).toBe(true); warn.mockRestore(); }); @@ -3538,6 +3541,27 @@ describe("pure helpers", () => { warn.mockRestore(); }); + it("#8833 REGRESSION (verifiable blockers): runWorkersOpinion checks absence evidence against the ACTUAL user prompt it sent", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const run = vi.fn(async () => ({ + response: JSON.stringify({ + assessment: "looks off", + blockers: [ + { claim: "Missing import of parseId breaks the new call", kind: "missing_import", evidence: "+ const id = parseId(raw);" }, + { claim: "Missing null guard before items[0]", kind: "missing_guard", evidence: "+ const first = items[0].id;" }, + ], + nits: [], suggestions: [], + }), + })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + // The user prompt CONTAINS the parseId line but NOT the items[0] line — one claim verifies, one cannot. + const parsed = await runWorkersOpinion(env, "@cf/x/model", "@cf/x/model", "sys", "diff:\n+ const id = parseId(raw);", 256); + expect(parsed.review?.blockers).toEqual(["Missing import of parseId breaks the new call"]); + expect(parsed.review?.nits.some((nit) => nit.startsWith("Verify: Missing null guard"))).toBe(true); + expect(warn.mock.calls.some(([line]) => String(line).includes("ai_review_unverified_absence_demoted"))).toBe(true); + warn.mockRestore(); + }); + it("#8961: runWorkersOpinion demotes an evidence-absence blocker ONLY when the body was truncated", async () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); const run = vi.fn(async () => ({ @@ -5456,6 +5480,91 @@ describe("#8833: enforced boundaries between model judgment and deterministic fa expect(demoteStaleBaseClaimBlockers(review(kept)).demoted).toEqual([]); }); + it("#8833 (verifiable blockers): parseModelReview accepts the object form — claims land in blockers, absence kinds in absenceClaims", async () => { + const { parseModelReview } = await import("../../src/services/ai-review"); + const parsed = parseModelReview(JSON.stringify({ + assessment: "Mixed forms.", + blockers: [ + "Plain string logic defect in src/a.ts", + " ", // whitespace-only string entry: dropped, exactly like toList + { claim: "Missing import of parseId breaks src/b.ts", kind: "missing_import", evidence: "+ const id = parseId(raw);" }, + { claim: "Free-form object claim with unknown kind", kind: "novel_kind", evidence: "whatever" }, + { claim: "Absence claim with non-string evidence", kind: "missing_guard", evidence: 42 }, + { kind: "missing_guard", evidence: "no claim, dropped" }, + 42, + ], + nits: [], suggestions: [], + })); + expect(parsed?.blockers).toEqual([ + "Plain string logic defect in src/a.ts", + "Missing import of parseId breaks src/b.ts", + "Free-form object claim with unknown kind", + "Absence claim with non-string evidence", + ]); + // Only the CLOSED absence-kind set collects evidence — a novel kind cannot smuggle a claim past + // verification — and non-string evidence normalizes to "" (which can never verify, so it demotes). + expect(parsed?.absenceClaims).toEqual([ + { claim: "Missing import of parseId breaks src/b.ts", evidence: "+ const id = parseId(raw);" }, + { claim: "Absence claim with non-string evidence", evidence: "" }, + ]); + }); + + it("#8833 (verifiable blockers): the 6-blocker cap drops the overflow's absence claims too — no orphaned verification entries", async () => { + const { parseModelReview } = await import("../../src/services/ai-review"); + const many = [1, 2, 3, 4, 5, 6].map((i) => `Defect number ${i}`); + const parsed = parseModelReview(JSON.stringify({ + assessment: "Overflowing.", + blockers: [...many, { claim: "Missing guard, seventh entry", kind: "missing_guard", evidence: "+ return items[0].id;" }], + nits: [], suggestions: [], + })); + expect(parsed?.blockers).toHaveLength(6); + expect(parsed?.absenceClaims).toEqual([]); + }); + + it("#8833 REGRESSION (verifiable blockers): an absence claim whose quoted evidence is NOT in the prompt demotes to a verification nit", async () => { + const { demoteUnverifiedAbsenceBlockers } = await import("../../src/services/ai-review"); + const prompt = "diff:\n+ const id = parseId(raw);\n+ return id;"; + const base = { + assessment: "a", nits: [], suggestions: [], confidence: 0.97, inlineFindings: [], + blockers: ["Missing import of parseId breaks src/b.ts", "Hallucinated missing guard in src/c.ts", "Plain judgment blocker stays"], + absenceClaims: [ + { claim: "Missing import of parseId breaks src/b.ts", evidence: "+ const id = parseId(raw);" }, // verifiable — the quote IS in the prompt + { claim: "Hallucinated missing guard in src/c.ts", evidence: "+ items.forEach((x) => sink(x.id));" }, // NOT in the prompt + ], + } as never; + const { review: out, demoted } = demoteUnverifiedAbsenceBlockers(base, prompt); + expect(demoted).toEqual(["Hallucinated missing guard in src/c.ts"]); + // The verified absence claim and the plain judgment blocker both keep blocker authority. + expect(out.blockers).toEqual(["Missing import of parseId breaks src/b.ts", "Plain judgment blocker stays"]); + expect(out.nits.some((nit: string) => nit.startsWith("Verify: Hallucinated missing guard"))).toBe(true); + }); + + it("#8833 (verifiable blockers): the leading diff marker is stripped before matching, and vacuous quotes never verify", async () => { + const { demoteUnverifiedAbsenceBlockers, ABSENCE_EVIDENCE_MIN_CHARS } = await import("../../src/services/ai-review"); + const reviewWith = (evidence: string) => ({ + assessment: "a", nits: [], suggestions: [], confidence: 1, inlineFindings: [], + blockers: ["Absence claim under test"], + absenceClaims: [{ claim: "Absence claim under test", evidence }], + }) as never; + // The prompt shows the line WITHOUT the +; a model that copied it WITH the marker still verifies. + const prompt = " const id = parseId(raw);"; + expect(demoteUnverifiedAbsenceBlockers(reviewWith("+ const id = parseId(raw);"), prompt).demoted).toEqual([]); + // Sub-minimum quotes (\") {\", \"x\") appear in virtually any diff — they must never verify vacuously. + expect(ABSENCE_EVIDENCE_MIN_CHARS).toBeGreaterThan(4); + expect(demoteUnverifiedAbsenceBlockers(reviewWith(") {"), `x) {x`).demoted).toEqual(["Absence claim under test"]); + expect(demoteUnverifiedAbsenceBlockers(reviewWith(""), prompt).demoted).toEqual(["Absence claim under test"]); + // No absence claims at all: same object back, zero-allocation, exactly like every sibling demotion. + const plain = { assessment: "a", nits: [], suggestions: [], confidence: 1, inlineFindings: [], blockers: ["b"] } as never; + expect(demoteUnverifiedAbsenceBlockers(plain, prompt).review).toBe(plain); + }); + + it("#8833 (verifiable blockers): the rubric demands the object form for absence claims and names the mechanical check", async () => { + const { REVIEW_SYSTEM_PROMPT } = await import("../../src/services/ai-review"); + expect(REVIEW_SYSTEM_PROMPT).toContain("ABSENCE CLAIMS ARE VERIFIED MECHANICALLY"); + expect(REVIEW_SYSTEM_PROMPT).toContain("missing_symbol | missing_import | missing_guard | missing_handling | missing_registration"); + expect(REVIEW_SYSTEM_PROMPT).toContain("copied VERBATIM"); + }); + it("#8833: the rubric names the full deterministic-fact family — CI, size, staleness, conflicts — as never-blockers", async () => { const { REVIEW_SYSTEM_PROMPT } = await import("../../src/services/ai-review"); expect(REVIEW_SYSTEM_PROMPT).toContain("decided deterministically by the gate"); diff --git a/test/unit/counterfactual-replay-core.test.ts b/test/unit/counterfactual-replay-core.test.ts index 62cf54933..78a8e2d89 100644 --- a/test/unit/counterfactual-replay-core.test.ts +++ b/test/unit/counterfactual-replay-core.test.ts @@ -126,7 +126,7 @@ describe("budget + artifacts helpers (#8221)", () => { describe("#8222: prompt version + CI comment/persist helpers", () => { it("the canonical judge prompt is a stable pure function of source and the version constant is pinned", () => { - expect(REVIEW_PROMPT_VERSION).toBe("review-prompt-v2"); + expect(REVIEW_PROMPT_VERSION).toBe("review-prompt-v3"); const prompt = buildCanonicalJudgePrompt(); expect(prompt.length).toBeGreaterThan(100); expect(prompt).toBe(buildCanonicalJudgePrompt()); // no clock, no env, no input