From 77a0b02914be41c8fc9a37ae666418b99eb5064c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 07:53:23 -0700 Subject: [PATCH] fix(review): stop truncation-induced false 'missing evidence' closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review prompt slices PR descriptions at 2,000 chars; when the cut fell before a Screenshots section, reviewers concluded the required visual evidence was absent and the gate closed compliant PRs (confirmed live: metagraphed#4640 closed with 6 attachments beyond the cut). Attachment presence is a deterministic fact, not a judgment: the prompt now carries the full-body attachment count whenever the description is truncated, and a parse-time guard demotes evidence-absence blockers to nits on truncated bodies at both parse sites, mirroring the CI-claim demotion. Untruncated bodies are untouched — there the claim is a legitimate judgment. --- src/services/ai-review.ts | 74 +++++++++++++++++++++++++++++++++++-- test/unit/ai-review.test.ts | 72 ++++++++++++++++++++++++++++++++++-- 2 files changed, 139 insertions(+), 7 deletions(-) diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index d71f7aaed0..9ad6e002eb 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -789,6 +789,43 @@ export function demoteCiClaimBlockers(review: ModelReview): { review: ModelRevie }; } +/** The prompt's PR-description window (buildUserPrompt slices the body here). Exported so the truncation + * FACT and the evidence-absence demotion below stay pinned to the same number the prompt actually uses. */ +export const PR_BODY_PROMPT_LIMIT = 2000; + +/** #8961: whether a PR body carries image/video attachments is a deterministic FACT — count it from the + * FULL body (markdown images, GitHub user-attachments links, tags, bare media URLs), never let a + * model infer it from a truncated window. PURE. */ +export function countBodyAttachments(body: string): number { + const matches = body.match(/!\[[^\]]*\]\([^)]+\)|github\.com\/user-attachments\/| EVIDENCE_ABSENCE_PATTERN.test(blocker)); + if (demoted.length === 0) return { review, demoted }; + return { + review: { + ...review, + blockers: review.blockers.filter((blocker) => !EVIDENCE_ABSENCE_PATTERN.test(blocker)), + nits: [...review.nits, ...demoted.map((claim) => `${claim} (demoted: the PR description was truncated for review — absence of evidence inside the truncated window is not evidence of absence)`)], + }, + 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); @@ -983,8 +1020,13 @@ function buildUserPrompt(input: LoopOverAiReviewInput): string { const lines = [ `Repository: ${input.repoFullName}`, `Pull request #${input.prNumber}: ${input.title}`, + // #8961: when the body exceeds the window, say so and carry the attachment COUNT as a structured fact + // computed from the FULL body — a reviewer must never conclude required visual evidence is absent just + // because the truncation point fell before a Screenshots section (confirmed production failure class). input.body - ? `Description:\n${input.body.slice(0, 2000)}` + ? input.body.length > PR_BODY_PROMPT_LIMIT + ? `Description (TRUNCATED at ${PR_BODY_PROMPT_LIMIT} chars — the FULL body contains ${countBodyAttachments(input.body)} image/video attachment(s) beyond what you can see; NEVER claim screenshots or visual evidence are missing):\n${input.body.slice(0, PR_BODY_PROMPT_LIMIT)}` + : `Description:\n${input.body}` : "Description: (none)", "", "Unified diff (truncated if large):", @@ -1250,6 +1292,8 @@ async function runWorkersOpinion( // wiring a real caller (source images, invoke with them) is a deliberately deferred follow-up; see // review/visual/visual-findings.ts. images?: readonly AiContentBlock[] | undefined, + // #8961: true when the PR description exceeded the prompt window — arms the evidence-absence demotion. + bodyTruncated = false, ): Promise { const ai = env.AI as unknown as AiRunner | undefined; if (!ai || typeof ai.run !== "function") return { review: null }; @@ -1355,10 +1399,15 @@ async function runWorkersOpinion( 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; + // #8961: same guarantee for evidence-absence claims against a truncated description. + const evidenceDemotion = demotion ? demoteEvidenceAbsenceBlockers(demotion.review, bodyTruncated) : null; + const parsed = evidenceDemotion?.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 (evidenceDemotion && evidenceDemotion.demoted.length > 0) { + console.warn(JSON.stringify({ level: "warn", event: "ai_review_evidence_absence_demoted", model, count: evidenceDemotion.demoted.length })); + } if (parsed && parsed.assessment.trim() !== "") { diagnostics.push({ model, attempt, status: "parsed", responseChars: text.length, hasJsonObject: Boolean(extractLastJsonObject(text)), ...usageFields }); return { review: parsed }; @@ -1675,6 +1724,7 @@ async function runProviderReview( user: string, maxTokens: number, images?: readonly AiContentBlock[] | undefined, + bodyTruncated = false, // #8961: arms the evidence-absence demotion, same contract as runWorkersOpinion ): Promise { const { text, usage, failure } = await callAiProvider( providerKey, @@ -1693,7 +1743,12 @@ async function runProviderReview( 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; + // #8961: same evidence-absence enforcement as the Workers path. + const providerEvidenceDemotion = providerDemotion ? demoteEvidenceAbsenceBlockers(providerDemotion.review, bodyTruncated) : null; + if (providerEvidenceDemotion && providerEvidenceDemotion.demoted.length > 0) { + console.warn(JSON.stringify({ level: "warn", event: "ai_review_evidence_absence_demoted", provider: providerKey.provider, count: providerEvidenceDemotion.demoted.length })); + } + const review = providerEvidenceDemotion?.review ?? null; return { review, diagnostic: { @@ -2374,6 +2429,9 @@ export async function runLoopOverAiReview( ? { ...input, ...defangReviewInput(input) } : input; const user = buildUserPrompt(promptInput); + // #8961: pinned to the SAME body buildUserPrompt just sliced, so the demotion can never disagree with + // the prompt about whether the description was cut. + const bodyTruncated = (promptInput.body?.length ?? 0) > PR_BODY_PROMPT_LIMIT; // Grounding-discipline SYSTEM suffix (convergence, flag-gated). When the caller supplied grounding, the // reviewers are told to verify claims against the attached CI/files; otherwise this is REVIEW_SYSTEM_PROMPT // unchanged (byte-identical). Computed from `promptInput` so it travels with the (possibly defanged) input. @@ -2519,6 +2577,8 @@ export async function runLoopOverAiReview( system, user, maxTokens, + undefined, + bodyTruncated, ); advisoryReview = outcome.review; byokFailure = outcome.failure; @@ -2535,6 +2595,8 @@ export async function runLoopOverAiReview( reviewDiagnostics, repoInstructionsSystemAppend, aiRunCorrelation, + undefined, + bodyTruncated, ); advisoryReview = outcome.review; if (outcome.fallbackNote) fallbackNotes.push(outcome.fallbackNote); @@ -2563,6 +2625,8 @@ export async function runLoopOverAiReview( reviewDiagnostics, repoInstructionsSystemAppend, aiRunCorrelation, + undefined, + bodyTruncated, ) : Promise.resolve({ review: advisoryReview }), runWorkersOpinion( @@ -2575,6 +2639,8 @@ export async function runLoopOverAiReview( reviewDiagnostics, repoInstructionsSystemAppend, aiRunCorrelation, + undefined, + bodyTruncated, ), ]); if (a.fallbackNote) fallbackNotes.push(a.fallbackNote); @@ -2639,6 +2705,8 @@ export async function runLoopOverAiReview( reviewDiagnostics, repoInstructionsSystemAppend, aiRunCorrelation, + undefined, + bodyTruncated, ) : ({ review: advisoryReview } as ReviewerOpinionOutcome); if (a.fallbackNote) fallbackNotes.push(a.fallbackNote); diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index d32ca4386a..0b149e2019 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -262,7 +262,7 @@ describe("runLoopOverAiReview gating", () => { content: [ { type: "text", - text: '{"assessment":"provider view","blockers":["The tests are failing on CI","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","Race in src/lock.ts"],"nits":[],"suggestions":[]}', }, ], }), @@ -277,7 +277,8 @@ describe("runLoopOverAiReview gating", () => { AI_DAILY_NEURON_BUDGET: "1", AI_BYOK_DAILY_REPO_LIMIT: "5", }); - const result = await runLoopOverAiReview(env, { ...baseInput, providerKey: { provider: "anthropic", key: "sk-ant-secret" } }); + // #8961: a body past the prompt window arms the evidence-absence demotion on the provider path too. + const result = await runLoopOverAiReview(env, { ...baseInput, body: `${"y".repeat(2100)}\n![after](https://x.io/1.png)`, 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 @@ -286,11 +287,14 @@ describe("runLoopOverAiReview gating", () => { const blockersSection = notes.split("**Nits")[0] ?? notes; expect(blockersSection).toContain("Race in src/lock.ts"); expect(blockersSection).not.toContain("tests are failing"); + expect(blockersSection).not.toContain("screenshots"); // #8961: demoted, never a blocker on a truncated body expect(notes).toContain("decided deterministically"); + expect(notes).toContain("absence of evidence inside the truncated window"); } - // 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. + // The demotion arms executed on the provider path (their logs fired) — the strong behavioral assertions + // live in the pure demotion tests and the workers-path tests above. expect(warn.mock.calls.some(([line]) => String(line).includes("ai_review_ci_claim_demoted"))).toBe(true); + expect(warn.mock.calls.some(([line]) => String(line).includes("ai_review_evidence_absence_demoted"))).toBe(true); warn.mockRestore(); }); @@ -3249,6 +3253,22 @@ describe("pure helpers", () => { 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 () => ({ + response: '{"assessment":"looks off","blockers":["No before/after screenshots provided for this visual change","Null deref in src/a.ts"],"nits":[],"suggestions":[]}', + })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const truncated = await runWorkersOpinion(env, "@cf/x/model", "@cf/x/model", "sys", "user", 256, [], "", undefined, undefined, true); + expect(truncated.review?.blockers).toEqual(["Null deref in src/a.ts"]); + expect(truncated.review?.nits.some((nit) => nit.includes("absence of evidence inside the truncated window"))).toBe(true); + expect(warn.mock.calls.some(([line]) => String(line).includes("ai_review_evidence_absence_demoted"))).toBe(true); + // Untruncated body: the model saw everything — the same claim is a legitimate judgment and stays a blocker. + const full = await runWorkersOpinion(env, "@cf/x/model", "@cf/x/model", "sys", "user", 256); + expect(full.review?.blockers).toContain("No before/after screenshots provided for this visual change"); + 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) => { @@ -4844,6 +4864,50 @@ describe("#8833: enforced boundaries between model judgment and deterministic fa expect(demoteCiClaimBlockers(untouched).review).toBe(untouched); }); + it("#8961: attachment counting is a deterministic fact — markdown images, user-attachments links, img tags, bare media URLs", async () => { + const { countBodyAttachments } = await import("../../src/services/ai-review"); + expect(countBodyAttachments("plain prose, no media")).toBe(0); + expect(countBodyAttachments("![before](https://example.com/a.png)")).toBe(1); + expect(countBodyAttachments("see https://github.com/user-attachments/assets/abc-123")).toBe(1); + expect(countBodyAttachments(' and https://cdn.example.com/demo.mp4')).toBe(2); + expect(countBodyAttachments("![a](u1) ![b](u2)\nhttps://x.io/shot.jpeg")).toBe(3); + }); + + it("#8961: evidence-absence blockers demote ONLY under a truncated body; other blockers and directions both match", async () => { + const { demoteEvidenceAbsenceBlockers, EVIDENCE_ABSENCE_PATTERN } = await import("../../src/services/ai-review"); + const claims = ["No before/after screenshots are provided for this visual change", "Screenshots are missing for the rendered change", "The added check silently swallows the failure"]; + // Untruncated: untouched, same object (the model saw the whole body — its claim is a judgment). + const untouched = review(claims); + expect(demoteEvidenceAbsenceBlockers(untouched, false).review).toBe(untouched); + // Truncated: both phrasing directions demote; the code-content blocker survives. + const { review: out, demoted } = demoteEvidenceAbsenceBlockers(review(claims), true); + expect(demoted).toHaveLength(2); + expect(out.blockers).toEqual(["The added check silently swallows the failure"]); + expect(out.nits.filter((nit: string) => nit.includes("absence of evidence inside the truncated window"))).toHaveLength(2); + // Truncated but no evidence claims: zero-demotion returns the same object. + const clean = review(["Null deref in src/a.ts"]); + expect(demoteEvidenceAbsenceBlockers(clean, true).review).toBe(clean); + for (const positive of ["cannot confirm before/after screenshot evidence", "fails to provide screen recordings", "visual evidence is omitted"]) { + expect(EVIDENCE_ABSENCE_PATTERN.test(positive)).toBe(true); + } + expect(EVIDENCE_ABSENCE_PATTERN.test("Missing test coverage for the error branch")).toBe(false); + }); + + it("#8961: the prompt carries the truncation + attachment FACT for a long body, and stays plain otherwise", async () => { + const { PR_BODY_PROMPT_LIMIT } = await import("../../src/services/ai-review"); + const images = "![a](https://x.io/1.png) ![b](https://x.io/2.png)"; + const longBody = "y".repeat(PR_BODY_PROMPT_LIMIT + 10) + "\n## Screenshots\n" + images; + const long = buildUserPrompt({ repoFullName: "o/r", prNumber: 1, title: "t", body: longBody, diff: "d", actor: "a", mode: "advisory" } as never); + expect(long).toContain(`TRUNCATED at ${PR_BODY_PROMPT_LIMIT}`); + expect(long).toContain("2 image/video attachment(s)"); + expect(long).toContain("NEVER claim screenshots or visual evidence are missing"); + const short = buildUserPrompt({ repoFullName: "o/r", prNumber: 1, title: "t", body: `hello ${images}`, diff: "d", actor: "a", mode: "advisory" } as never); + expect(short).toContain("Description:\nhello"); + expect(short).not.toContain("TRUNCATED"); + const bodiless = buildUserPrompt({ repoFullName: "o/r", prNumber: 1, title: "t", body: "", diff: "d", actor: "a", mode: "advisory" } as never); + expect(bodiless).toContain("Description: (none)"); + }); + 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);