diff --git a/src/queue/ai-review-orchestration.ts b/src/queue/ai-review-orchestration.ts index 623482d989..90122d51e4 100644 --- a/src/queue/ai-review-orchestration.ts +++ b/src/queue/ai-review-orchestration.ts @@ -363,6 +363,17 @@ export async function runAiReviewForAdvisory( // value-assessment prompt addition. Absent/false ⇒ the prompt is byte-identical (no valueAssessment // requested) -- the only reachable value until this PR started resolving the feature. improvementSignal?: boolean | undefined; + // Screenshot-table-vision's plain-language evidence summary (#screenshot-vision-summary, #4366 follow-up), + // resolved by the caller from the SAME (self-hosted `env.AI_VISION`/BYOK) vision call that already checks + // the PR's before/after screenshot-table for gaming -- see `runScreenshotTableVisionForAdvisory` / + // `parseScreenshotTableVisionSummary`. TEXT ONLY, by design (#cost-architecture): the vision call already + // looked at the actual image bytes on the cheap self-hosted model; only its distilled text summary reaches + // this (frontier-model) review, so the prompt's token cost grows by a small amount of text, never by image + // tokens. Threaded straight through to runLoopOverAiReview's own field of the same name (mirroring + // reviewInstructions/pathGuidance's byte-identical-when-absent contract). Absent/null (no screenshot-table, + // the vision gate declined, or the call failed/returned unparseable output) ⇒ the reviewer prompt is + // byte-identical to before this field existed -- never routed through the images/AiContentBlock parameter. + screenshotEvidenceSummary?: string | null | undefined; // The inbound webhook delivery id that triggered this review (#codex-timeout-fields) — forwarded to a // self-host provider's failure log purely for operator correlation; never read by any review logic. Absent // (e.g. a sweep/repair fan-out with no single originating delivery, or a unit test) ⇒ the log line omits it. @@ -701,6 +712,9 @@ export async function runAiReviewForAdvisory( files.map((file) => file.path), ), repoInstructions: args.reviewInstructions ?? null, + // #screenshot-vision-summary: the caller's already-resolved screenshot-table-vision evidence summary + // (TEXT ONLY -- see this arg's own doc comment above). Absent/null ⇒ byte-identical prompt. + screenshotEvidenceSummary: args.screenshotEvidenceSummary ?? null, changedFiles: files, // improvementSignal (#4744): ask the model for the ordinal value/improvement judgment (#4743) only when // the caller resolved the feature on for this repo. Absent/false ⇒ byte-identical prompt. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 7fff7bd795..b6bafe2e86 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -616,6 +616,7 @@ import { buildScreenshotTableVisionUserPrompt, evaluateScreenshotTableVisionGate, parseScreenshotTableVisionResponse, + parseScreenshotTableVisionSummary, SCREENSHOT_TABLE_VISION_FINDING_CODE, SCREENSHOT_TABLE_VISION_SYSTEM_PROMPT, } from "../review/visual/screenshot-table-vision"; @@ -7702,6 +7703,14 @@ async function recordScreenshotTableVisionUsage( * settings). STRICTLY ADVISORY, mirrors `runVisualVisionForAdvisory`'s exact shape (resolve reputation + BYOK, * gate, call, parse, mutate `args.advisory.findings`) so it can be exercised directly in tests. Never throws: * any failure (a broken image fetch, a provider error, an unparseable response) degrades to "no finding added". + * + * Returns the SAME vision call's plain-language evidence summary (#screenshot-vision-summary), when the call + * actually ran and produced one — `undefined` for every skip/early-exit/failure path (mirrors this summary's + * own "absent means omit" contract, see `parseScreenshotTableVisionSummary`'s doc comment). The caller + * (`maybePublishPrPublicSurface`) threads this into the main AI review's `screenshotEvidenceSummary` prompt + * param as TEXT-ONLY extra context — never the image bytes themselves (#cost-architecture: vision stays on + * the cheap self-hosted `env.AI_VISION`/BYOK call already made here; only its distilled text output reaches + * the separate, expensive frontier-model review call). */ export async function runScreenshotTableVisionForAdvisory( env: Env, @@ -7716,13 +7725,14 @@ export async function runScreenshotTableVisionForAdvisory( settings: RepositorySettings; advisory: { findings: AdvisoryFinding[] }; }, -): Promise { - if (args.mode === "paused" || !args.settings.screenshotTableGate?.enabled) return; +): Promise { + if (args.mode === "paused" || !args.settings.screenshotTableGate?.enabled) return undefined; const rawPairs = extractTableRowImageUrls(args.prBody).filter((pair) => pair.every((url) => isSafeHttpUrl(url))); - if (rawPairs.length === 0) return; + if (rawPairs.length === 0) return undefined; try { const fetchedPairs: Array<{ before: AiContentBlock; after: AiContentBlock }> = []; const findings: AdvisoryFinding[] = []; + let evidenceSummary: string | undefined; for (const [rowIndex, [beforeUrl, afterUrl]] of rawPairs.slice(0, 2).entries()) { /* v8 ignore next -- defensive: rawPairs only contains rows with >=2 urls, so both slots exist here. */ if (!beforeUrl || !afterUrl) continue; @@ -7771,7 +7781,10 @@ export async function runScreenshotTableVisionForAdvisory( let visionText: string | null; let visionUsage: AiReviewActualUsage | undefined; if (providerKey) { - const response = await callAiProvider(providerKey, SCREENSHOT_TABLE_VISION_SYSTEM_PROMPT, userPrompt, 400, images); + // 600 (was 400): the response now carries the findings array PLUS the always-on plain-language + // `summary` field (#screenshot-vision-summary) -- mirrors runSelfHostVisualVision's own 600-token + // cap on the self-host leg below, so BYOK and self-host give the model the same amount of room. + const response = await callAiProvider(providerKey, SCREENSHOT_TABLE_VISION_SYSTEM_PROMPT, userPrompt, 600, images); visionText = response.text; visionUsage = response.usage; await recordScreenshotTableVisionUsage( @@ -7798,10 +7811,15 @@ export async function runScreenshotTableVisionForAdvisory( if (visionText) { const parsed = parseScreenshotTableVisionResponse(visionText, gate.pairCount); findings.push(...buildScreenshotTableVisionFindings(parsed)); + // #screenshot-vision-summary: parsed from the SAME response, never a second vision call (keeps the + // self-hosted GPU cost identical to the gaming-only check alone). undefined for an unparseable/blank + // summary -- the caller's own "absent means omit" contract for the AI review prompt param. + evidenceSummary = parseScreenshotTableVisionSummary(visionText); } } } if (findings.length > 0) args.advisory.findings.push(...findings); + return evidenceSummary; } catch (error) { console.log( JSON.stringify({ @@ -7811,6 +7829,7 @@ export async function runScreenshotTableVisionForAdvisory( message: errorMessage(error).slice(0, 200), }), ); + return undefined; } } @@ -8078,6 +8097,13 @@ async function maybePublishPrPublicSurface( // resolving false this pass, in which case the quadrant degrades to showing nothing extra rather than // fabricating a risk reading (see formatRiskValueQuadrant's own doc comment). let slopBand: SlopBand | null = null; + // #screenshot-vision-summary: the screenshot-table-vision pass's plain-language evidence summary (when it ran + // and produced one) -- resolved BEFORE the AI review below runs (see the `runScreenshotTableVisionForAdvisory` + // call further down, moved earlier in this pass specifically so this value exists in time) and threaded into + // `runAiReviewForAdvisory` as extra TEXT-ONLY context (#cost-architecture: never the image bytes themselves). + // Stays undefined for every skip/failure path -- the AI review prompt is then byte-identical to before this + // field existed. + let screenshotEvidenceSummary: string | undefined; // Resolve the repo's action mode ONCE for the whole publish pass and thread it into every GitHub write below, so // a dry-run / pause / global-freeze publishes NOTHING (check-run, comment, label) — the gate verdict is still // computed + returned for the disposition logic, the writes are just suppressed + audited. (#dry-run-chokepoint) @@ -9383,6 +9409,28 @@ async function maybePublishPrPublicSurface( } } } + // Vision-verify a contributor-pasted screenshot-table (#4366 wiring) — see runScreenshotTableVisionForAdvisory's + // own doc comment. Independent of the bot-capture vision block further down (checked below the gate/panel + // rendering): this checks the CONTRIBUTOR's own pasted table images, not the bot's rendered before/after + // pair, so it never needs the visual-capture pipeline's output. MOVED here (#screenshot-vision-summary), + // ahead of the AI review's own cache-read/run decision just below, so the vision pass's plain-language + // evidence summary exists in time to thread into `runAiReviewForAdvisory` as extra context -- this call's + // OWN gating (mode/screenshotTableGate.enabled/reputation/provider/image-pairs, all internal to + // `runScreenshotTableVisionForAdvisory`) is completely unchanged, and it stays independent of + // `aiReviewWillRun` below exactly as before this move: a repo with the screenshot-table gate on but AI + // review off (or an AI-review-ineligible author) still gets the gaming-detection check, it just has no AI + // review to hand a summary to. + screenshotEvidenceSummary = await runScreenshotTableVisionForAdvisory(env, { + mode, + repoFullName, + pr, + prBody: pr.body, + prTitle: pr.title, + author, + confirmedContributor, + settings, + advisory, + }); if (aiReviewWillRun) { // Per-(repo, PR, head SHA, mode) advisory lock (#regate-dup-prep), claimed HERE — not just inside // runAiReviewForAdvisory — so it covers the cache-read DECISION below too, not only the LLM call itself. @@ -9690,6 +9738,12 @@ async function maybePublishPrPublicSurface( // improvementSignal (#4744): resolved once above, reused here so the LLM tier's value-assessment // prompt addition (#4743) only fires when this repo has actually opted in. improvementSignal: improvementSignalAllowed, + // #screenshot-vision-summary: the screenshot-table-vision pass's plain-language evidence summary, + // resolved earlier in THIS pass (see the `runScreenshotTableVisionForAdvisory` call above, + // before this `if (aiReviewWillRun)` block) -- TEXT ONLY, never the image bytes (#cost-architecture). + // undefined (no screenshot-table, the vision gate declined, or the call failed/returned unparseable + // output) ⇒ this review's prompt is byte-identical to before this field existed. + screenshotEvidenceSummary, // #regate-dup-prep: this call's own advisory lock is already claimed (by aiReviewCacheReadDecideAndRun's // caller, above) — pass it through so runAiReviewForAdvisory trusts it instead of re-claiming (and // losing) against itself, and does not release it before the cache write below runs. @@ -10662,20 +10716,17 @@ async function maybePublishPrPublicSurface( routes: beforeAfter, bugAnalysisEnabled, }); - // Vision-verify a contributor-pasted screenshot-table (#4366 wiring) — see runScreenshotTableVisionForAdvisory's - // own doc comment. Independent of the bot-capture vision block above: this checks the CONTRIBUTOR's own - // pasted table images, not the bot's rendered before/after pair. - await runScreenshotTableVisionForAdvisory(env, { - mode, - repoFullName, - pr, - prBody: pr.body, - prTitle: pr.title, - author, - confirmedContributor, - settings, - advisory, - }); + // Vision-verify a contributor-pasted screenshot-table (#4366 wiring): the actual vision call (and its + // findings) now runs EARLIER in this pass -- see the `runScreenshotTableVisionForAdvisory` call above, + // near the AI review's own cache-read/run decision -- so its plain-language evidence summary is ready in + // time to thread into THIS pass's AI review prompt as extra context (#screenshot-vision-summary / + // #cost-architecture). Moved so `runAiReviewForAdvisory` (which now accepts `screenshotEvidenceSummary`) + // is called AFTER the vision pass, not before it. One deliberate, benign side effect of moving the call + // (and its `advisory.findings` mutation) this much earlier: its STRICTLY ADVISORY findings (never a gate + // blocker, see screenshot-table-vision.ts's header) can now also land in `gateEvaluation`/`commentGate` + // (computed further up, between the two positions) for THIS pass, where before this move they only ever + // reached `advisoryFindings: advisory.findings` below (read live, after the old call site) -- i.e. they + // show up sooner in the SAME rendered comment, never later or not at all. // review.memory (#2181, apply slice of #1964): before the unified comment renders, suppress/demote // advisory (non-blocking) findings a maintainer already dismissed as false positives for this repo. ONLY // ever applied to `commentGate.warnings` -- NEVER `commentGate.blockers` -- so this can never change the diff --git a/src/review/visual/screenshot-table-vision.ts b/src/review/visual/screenshot-table-vision.ts index 59b4469314..64843d606d 100644 --- a/src/review/visual/screenshot-table-vision.ts +++ b/src/review/visual/screenshot-table-vision.ts @@ -15,6 +15,16 @@ // two images still look near-identical (a re-encoded/recompressed duplicate a byte comparison would miss) // OR plausibly unrelated to the PR's stated change (a screenshot from an unrelated app/page/topic). // +// The SAME vision call (#screenshot-vision-summary) ALSO returns a plain-language `summary` describing what the +// before/after images show and whether they plausibly support the PR's stated change -- always on, no separate +// config toggle (unlike visual-findings.ts's `bugAnalysisEnabled`-style dual-prompt precedent, which this module +// deliberately does NOT follow: the maintainer wants this on for every repo that already opted into the +// deterministic gate). This is a SECOND field in the SAME JSON response, never a second vision API call -- +// keeping the (cheap, self-hosted `env.AI_VISION`) GPU cost identical to the gaming-only check alone. The live +// caller threads ONLY this summary's TEXT (never the image bytes/AiContentBlocks themselves) into the main AI +// review's prompt as extra context (#cost-architecture) -- see `runAiReviewForAdvisory` / `runLoopOverAiReview`'s +// `screenshotEvidenceSummary` param. +// // STRICTLY ADVISORY: `SCREENSHOT_TABLE_VISION_FINDING_CODE` is not one of the codes `isConfiguredGateBlocker` // (src/rules/advisory.ts) recognizes, so this finding can NEVER become a gate blocker -- it rides the // identical `advisory.findings` pipeline `visual_regression_finding`/`ai_consensus_defect` already use. @@ -72,16 +82,24 @@ export type ScreenshotTableVisionFinding = { pairIndex: number; body: string }; const MAX_SCREENSHOT_TABLE_VISION_FINDINGS = 2; export const SCREENSHOT_TABLE_VISION_SYSTEM_PROMPT = [ - "You are checking a pull request's before/after screenshot-table evidence for gaming, not for visual regressions.", - "Each pair below is one table row's before image followed by its after image, in that order.", - 'Respond with ONLY a JSON object of this exact shape (no prose, no code fence): {"findings": [{"pairIndex": number, "body": string}]}.', - "Report a finding for a pair ONLY when the two images are effectively the SAME screenshot (a near-identical", - "duplicate, not a genuine before/after difference) OR when either image looks implausible as evidence for the", - "stated change (an unrelated app/website/topic, a blank/broken render, or an obviously irrelevant picture).", - "Do NOT report a pair just because the visual difference is small — a genuine minor style tweak is exactly", - "what real before/after evidence looks like. pairIndex is 1 for the first pair, 2 for the second, and so on.", - "Each body is ONE sentence, specific to what you SEE. Return an empty findings array when every pair looks", - "like genuine, plausible before/after evidence. Never mention rewards, payouts, wallets, hotkeys, coldkeys, or trust scores.", + "You are reviewing a pull request's before/after screenshot-table evidence for TWO separate purposes: gaming", + "detection AND a plain factual summary. Each pair below is one table row's before image followed by its after", + "image, in that order.", + 'Respond with ONLY a JSON object of this exact shape (no prose, no code fence): {"findings": [{"pairIndex": number, "body": string}], "summary": string}.', + "GAMING DETECTION (the findings array): report a finding for a pair ONLY when the two images are effectively the", + "SAME screenshot (a near-identical duplicate, not a genuine before/after difference) OR when either image looks", + "implausible as evidence for the stated change (an unrelated app/website/topic, a blank/broken render, or an", + "obviously irrelevant picture). Do NOT report a pair just because the visual difference is small — a genuine minor", + "style tweak is exactly what real before/after evidence looks like. pairIndex is 1 for the first pair, 2 for the", + "second, and so on. Each finding body is ONE sentence, specific to what you SEE. Return an empty findings array", + "when every pair looks like genuine, plausible before/after evidence — you are checking for gaming here, not for", + "visual regressions.", + "EVIDENCE SUMMARY (the summary field, ALWAYS include this, even when findings is empty): in 1-3 plain-language", + "sentences, describe what the before and after images actually show and whether they plausibly support the pull", + "request's stated change (its title, if given, appears above). Call out any visible UX or visual regression you", + "can see comparing the before image to the after image. This is a neutral, factual description for a human", + "reviewer, a different question from the gaming judgment above — write it even when findings is empty.", + "Never mention rewards, payouts, wallets, hotkeys, coldkeys, or trust scores.", ].join(" "); /** Build the user-turn text naming the PR's stated change ahead of the image content blocks — the caller @@ -122,6 +140,37 @@ export function parseScreenshotTableVisionResponse(text: string, pairCount: numb return out; } +/** Bound on the plain-language evidence summary's length (#screenshot-vision-summary) — mirrors the bounded- + * length convention every other freeform AI-authored prompt-context field in this codebase follows (e.g. + * `review.instructions`'s own manifest-parse-time cap) so a verbose vision response can never blow out the + * main AI review's token budget — the entire point of keeping this addition TEXT-ONLY (see this file's + * header's cost-architecture note). */ +const MAX_SCREENSHOT_TABLE_VISION_SUMMARY_CHARS = 600; + +/** Parse ONLY the new plain-language `summary` field out of the model's structured vision response + * (#screenshot-vision-summary) — a SIBLING parser to {@link parseScreenshotTableVisionResponse}, deliberately + * independent so that function's existing findings-parsing behavior (and its own test suite) stay untouched. + * Returns `undefined` — never an empty string — for a missing/blank/non-string `summary`, an unparseable + * response, or one that trips the public/private boundary (`toPublicSafe`); the same fail-safe convention + * {@link parseScreenshotTableVisionResponse} uses. This "absent means omit" contract matches exactly what the + * eventual `screenshotEvidenceSummary` review-prompt param expects: absent/empty ⇒ the main review's prompt + * stays byte-identical to today. Bounded to {@link MAX_SCREENSHOT_TABLE_VISION_SUMMARY_CHARS}. */ +export function parseScreenshotTableVisionSummary(text: string): string | undefined { + const raw = extractLastJsonObject(text); + if (!raw) return undefined; + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return undefined; + } + const summaryRaw = (parsed as { summary?: unknown } | null)?.summary; + if (typeof summaryRaw !== "string") return undefined; + const safe = toPublicSafe(summaryRaw); + if (!safe) return undefined; + return safe.slice(0, MAX_SCREENSHOT_TABLE_VISION_SUMMARY_CHARS); +} + /** Build the ADVISORY-ONLY findings for the unified comment (#4366) — one per vision observation, feeding the * SAME `advisory.findings` pipeline `visual_regression_finding`/`ai_consensus_defect` already ride. * `severity: "warning"` is required, not incidental — `evaluateGateCheckCore` (src/rules/advisory.ts) only diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 5a0eec447a..9279b316c4 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -305,6 +305,20 @@ export type LoopOverAiReviewInput = { * the reviewer prompt is byte-identical. */ repoInstructions?: string | null | undefined; + /** + * Screenshot-table-vision's plain-language evidence summary (#screenshot-vision-summary, #4366 follow-up), + * resolved by the caller from a SEPARATE, already-completed vision call over the PR's before/after + * screenshot-table (self-hosted `env.AI_VISION`, cheap GPU compute, or BYOK) — see + * `review/visual/screenshot-table-vision.ts`'s `parseScreenshotTableVisionSummary`. TEXT ONLY, by design + * (#cost-architecture): that vision call already looked at the actual image bytes on the cheap self-hosted + * model; only its distilled text summary reaches THIS (frontier-model) review, so this prompt's token cost + * grows by a small amount of text, never by image tokens — deliberately NOT routed through the `images` + * parameter below (see `toContentBlocks`), which is a separate, unrelated, still-inert plumbing path (#4111). + * Absent/null (no screenshot-table, the vision gate declined, or the vision call failed/returned unparseable + * output) ⇒ the reviewer prompt is byte-identical to before this field existed, same convention as + * `repoInstructions`/`pathGuidance` above. + */ + screenshotEvidenceSummary?: string | null | undefined; /** * `.loopover.yml` `review.inline_comments` (#inline-comments) — when true (the caller has already ANDed the * operator flag + cutover allowlist + the per-repo manifest toggle), the reviewer is asked to ALSO emit an @@ -981,12 +995,17 @@ function buildSystemPrompt(input: LoopOverAiReviewInput): string { // review; empty ⇒ nothing appended (byte-identical). const repoInstructionsAppend = buildRepoInstructionsSystemAppend(input.repoInstructions); const repoInstructionsSuffix = repoInstructionsAppend ? ` ${repoInstructionsAppend}` : ""; + // #screenshot-vision-summary: the screenshot-table-vision pass's plain-language TEXT-ONLY summary (never image + // bytes -- see this field's own doc comment on LoopOverAiReviewInput). Absent/blank ⇒ nothing appended + // (byte-identical), same convention as repoInstructions immediately above. + const screenshotEvidenceAppend = buildScreenshotEvidenceSystemAppend(input.screenshotEvidenceSummary); + const screenshotEvidenceSuffix = screenshotEvidenceAppend ? ` ${screenshotEvidenceAppend}` : ""; const inlineSuffix = input.inlineFindings ? INLINE_FINDINGS_SUFFIX : ""; // review.finding_categories (#1958) only makes sense layered on top of inlineFindings itself being requested. const categorySuffix = input.inlineFindings && input.findingCategories ? FINDING_CATEGORY_SUFFIX : ""; // improvementSignal (#4743): caller-resolved, exactly like inlineFindings/findingCategories above. const improvementSignalSuffix = input.improvementSignal ? IMPROVEMENT_SIGNAL_SUFFIX : ""; - return `${REVIEW_SYSTEM_PROMPT}${groundingSuffix}${enrichmentSuffix}${profileSuffix}${securityFocusSuffix}${pathSuffix}${repoInstructionsSuffix}${inlineSuffix}${categorySuffix}${improvementSignalSuffix}`; + return `${REVIEW_SYSTEM_PROMPT}${groundingSuffix}${enrichmentSuffix}${profileSuffix}${securityFocusSuffix}${pathSuffix}${repoInstructionsSuffix}${screenshotEvidenceSuffix}${inlineSuffix}${categorySuffix}${improvementSignalSuffix}`; } function buildRepoInstructionsSystemAppend(repoInstructions: string | null | undefined): string { @@ -996,6 +1015,18 @@ function buildRepoInstructionsSystemAppend(repoInstructions: string | null | und : ""; } +/** #screenshot-vision-summary: mirrors {@link buildRepoInstructionsSystemAppend}'s exact shape -- a labeled + * section header the model can distinguish from other prompt context, empty for a blank/whitespace-only or + * absent summary so the system prompt stays byte-identical. The label calls out that this is a DISTILLED + * vision-model summary (not the reviewer's own observation, and not the raw images) so the reviewer treats it + * as reported evidence to weigh, not ground truth it verified itself. */ +function buildScreenshotEvidenceSystemAppend(screenshotEvidenceSummary: string | null | undefined): string { + const trimmed = screenshotEvidenceSummary?.trim(); + return trimmed + ? `SCREENSHOT EVIDENCE (a separate vision model's summary of this PR's before/after screenshot-table images — text only; weigh it as reported evidence): ${trimmed}` + : ""; +} + /** Correlation + per-repo override context forwarded to `env.AI.run`'s options. `jobId`/`repoFullName`/ * `pullNumber` (#codex-timeout-fields) are purely observational — a self-host provider-failure log, never read * by any provider's own request logic. `claudeModel`/`claudeEffort`/`codexModel`/`codexEffort` and diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 2e4bac5ccb..4cbd3d741c 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -500,6 +500,62 @@ describe("review.profile shapes the reviewer system prompt (#review-profile)", ( expect((await optionsFor("claude-code", " ")).systemAppend).toBeUndefined(); }); + it("screenshotEvidenceSummary (#screenshot-vision-summary) is appended to the system prompt; absent/null/blank leaves it byte-identical", async () => { + const systemPromptOf = (run: ReturnType): string => + (run.mock.calls[0]?.[1] as { messages?: Array<{ content?: string }> }) + ?.messages?.[0]?.content ?? ""; + const runSummary = async (screenshotEvidenceSummary: string | null | undefined) => { + const run = vi.fn(async () => ({ response: reviewJson() })); + const env = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await runLoopOverAiReview(env, { ...baseInput, screenshotEvidenceSummary }); + return systemPromptOf(run); + }; + const withSummary = await runSummary( + "The after screenshot shows the nav bar moved to the right, matching the PR's stated redesign.", + ); + expect(withSummary).toContain("SCREENSHOT EVIDENCE"); + expect(withSummary).toContain("matching the PR's stated redesign"); + // Absent, null, or whitespace-only → no append (byte-identical prompt), same convention as repoInstructions. + const withoutUndefined = await runSummary(undefined); + const withoutNull = await runSummary(null); + const withoutBlank = await runSummary(" "); + expect(withoutUndefined).not.toContain("SCREENSHOT EVIDENCE"); + expect(withoutNull).not.toContain("SCREENSHOT EVIDENCE"); + expect(withoutBlank).not.toContain("SCREENSHOT EVIDENCE"); + expect(withoutUndefined).toBe(withoutNull); + expect(withoutNull).toBe(withoutBlank); + }); + + it("screenshotEvidenceSummary composes correctly alongside repoInstructions and pathGuidance when all three are present", async () => { + const systemPromptOf = (run: ReturnType): string => + (run.mock.calls[0]?.[1] as { messages?: Array<{ content?: string }> }) + ?.messages?.[0]?.content ?? ""; + const run = vi.fn(async () => ({ response: reviewJson() })); + const env = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await runLoopOverAiReview(env, { + ...baseInput, + pathGuidance: "\n\nPath-specific review instructions:\n- `src/**`: Enforce null checks.", + repoInstructions: "Follow our async-error conventions.", + screenshotEvidenceSummary: "The after screenshot shows a visible layout regression in the header.", + }); + const system = systemPromptOf(run); + expect(system).toContain("Enforce null checks."); + expect(system).toContain("REPOSITORY REVIEW INSTRUCTIONS"); + expect(system).toContain("async-error conventions"); + expect(system).toContain("SCREENSHOT EVIDENCE"); + expect(system).toContain("visible layout regression in the header"); + }); + it("the inline-findings instruction is appended to the system prompt ONLY when requested (#inline-comments)", async () => { const systemPromptOf = (run: ReturnType): string => (run.mock.calls[0]?.[1] as { messages?: Array<{ content?: string }> }) diff --git a/test/unit/screenshot-evidence-summary-wiring.test.ts b/test/unit/screenshot-evidence-summary-wiring.test.ts new file mode 100644 index 0000000000..6f03791a0a --- /dev/null +++ b/test/unit/screenshot-evidence-summary-wiring.test.ts @@ -0,0 +1,223 @@ +// #screenshot-vision-summary: end-to-end regression coverage for the reordering that threads +// screenshot-table-vision's plain-language evidence summary into the SAME pass's main AI review prompt. +// Unlike screenshot-table-vision.test.ts (pure parser unit tests) and screenshot-table-vision-wiring.test.ts +// (runScreenshotTableVisionForAdvisory in isolation), this file drives the FULL webhook pipeline +// (processJob -> maybePublishPrPublicSurface) so it actually exercises the reordering inside that +// (unexported) function -- the "bridge" between the two halves, not just each half tested independently. +import { afterEach, describe, expect, it, vi } from "vitest"; +import { upsertInstallation, upsertRepositorySettings } from "../../src/db/repositories"; +import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; +import { processJob } from "../../src/queue/processors"; +import { createTestEnv } from "../helpers/d1"; + +async function generatePrivateKeyPem(): Promise { + const key = (await crypto.subtle.generateKey( + { + name: "RSASSA-PKCS1-v1_5", + modulusLength: 2048, + publicExponent: new Uint8Array([1, 0, 1]), + hash: "SHA-256", + }, + true, + ["sign", "verify"], + )) as CryptoKeyPair; + const exported = await crypto.subtle.exportKey("pkcs8", key.privateKey); + const base64 = Buffer.from(exported as ArrayBuffer).toString("base64").replace(/(.{64})/g, "$1\n"); + return `-----BEGIN PRIVATE KEY-----\n${base64}\n-----END PRIVATE KEY-----\n`; +} + +const REPO_FULL_NAME = "JSONbored/gittensory"; +const BEFORE_URL = "https://user-images.githubusercontent.com/vision-before.png"; +const AFTER_URL = "https://user-images.githubusercontent.com/vision-after.png"; + +function prBodyWithTable(): string { + return `Redesigns the nav bar per the linked issue.\n\n| Before | After |\n| --- | --- |\n| ![before](${BEFORE_URL}) | ![after](${AFTER_URL}) |\n\nCloses #1`; +} + +/** Wires a full webhook pass with the screenshot-table gate ON (a genuine before/after table, so the + * deterministic gate never violates/closes) and AI review ON (advisory, single opinion, so exactly one + * `env.AI.run` call carries the main review's prompt) -- `aiReviewAllAuthors: true` unlocks self-host + * vision without needing a separate confirmed-miner-detection fixture. `visionResponse`/`reviewResponse` + * are the raw JSON text each mocked binding returns; `pull` lets each test use a distinct PR number/head so + * D1 rows never collide across tests in this file. */ +async function runWebhookPass(args: { + pull: number; + headSha: string; + visionResponse: string | null; + reviewResponse: string; +}): Promise<{ visionRun: ReturnType; reviewRun: ReturnType }> { + const visionRun = vi.fn(async () => (args.visionResponse === null ? { response: "" } : { response: args.visionResponse })); + const reviewRun = vi.fn(async () => ({ response: args.reviewResponse })); + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: reviewRun } as unknown as Ai, + AI_VISION: { run: visionRun } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "gittensory", full_name: REPO_FULL_NAME, private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: REPO_FULL_NAME }); + await upsertRepoFocusManifest( + env, + REPO_FULL_NAME, + { + settings: { + commentMode: "all_prs", + publicSurface: "comment_only", + checkRunMode: "off", + reviewCheckMode: "required", + aiReviewMode: "advisory", + aiReviewAllAuthors: true, + screenshotTableGate: { enabled: true, whenLabels: [], whenPaths: [] }, + }, + }, + "repo_file", + ); + const pullPath = `/pulls/${args.pull}`; + vi.stubGlobal( + "fetch", + vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url === BEFORE_URL) return new Response(new Uint8Array([1, 2, 3]), { status: 200, headers: { "content-type": "image/png" } }); + if (url === AFTER_URL) return new Response(new Uint8Array([4, 5, 6]), { status: 200, headers: { "content-type": "image/png" } }); + if (url.includes(`${pullPath}/files`)) return Response.json([{ filename: "apps/ui/src/nav.tsx", status: "modified", additions: 4, deletions: 1, changes: 5, patch: "@@\n+export const Nav = () => null;" }]); + if (url.includes(`${pullPath}/reviews`)) return Response.json([]); + if (url.includes(`${pullPath}/commits`)) return Response.json([]); + if (url.endsWith(pullPath)) return Response.json({ number: args.pull, state: "open", user: { login: "nav-contributor" }, head: { sha: args.headSha }, mergeable_state: "clean" }); + if (url.includes(`/commits/${args.headSha}/check-runs`)) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes(`/commits/${args.headSha}/status`)) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + if (url.includes(`/issues/${args.pull}/labels`)) return Response.json([]); + if (url.includes(`/issues/${args.pull}/comments`)) return Response.json([]); + return Response.json({}); + }), + ); + + await processJob(env, { + type: "github-webhook", + deliveryId: `screenshot-evidence-summary-${args.pull}`, + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: REPO_FULL_NAME, private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: args.pull, + title: "Redesign the nav bar", + state: "open", + user: { login: "nav-contributor" }, + head: { sha: args.headSha }, + labels: [], + body: prBodyWithTable(), + mergeable_state: "clean", + reviewDecision: "APPROVED", + }, + }, + }); + + return { visionRun, reviewRun }; +} + +function reviewPromptOf(reviewRun: ReturnType): { system: string; user: unknown } { + const call = reviewRun.mock.calls[0] as unknown as [string, { messages?: Array<{ role: string; content: unknown }> }] | undefined; + const messages = call?.[1]?.messages ?? []; + const system = (messages.find((m) => m.role === "system")?.content as string | undefined) ?? ""; + const user = messages.find((m) => m.role === "user")?.content; + return { system, user }; +} + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe("screenshot-table-vision summary reaches the main AI review prompt (#screenshot-vision-summary)", () => { + it("threads the vision pass's evidence summary into the SAME pass's AI review system prompt (the actual bridge)", async () => { + const { visionRun, reviewRun } = await runWebhookPass({ + pull: 201, + headSha: "nav-sha-201", + visionResponse: JSON.stringify({ + findings: [], + summary: "The after screenshot shows the nav bar moved to the right, which matches the PR's stated redesign.", + }), + reviewResponse: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [], confidence: 1 }), + }); + + // Exactly one vision call -- the SAME call produces both the (empty) findings and the summary, never a + // second vision API call (#cost-architecture). + expect(visionRun).toHaveBeenCalledTimes(1); + expect(reviewRun).toHaveBeenCalledTimes(1); + + const { system } = reviewPromptOf(reviewRun); + expect(system).toContain("SCREENSHOT EVIDENCE"); + expect(system).toContain("matches the PR's stated redesign"); + }); + + it("reordering did not change any of runAiReviewForAdvisory's other existing inputs: title/diff/repo still reach the prompt untouched", async () => { + const { reviewRun } = await runWebhookPass({ + pull: 202, + headSha: "nav-sha-202", + visionResponse: JSON.stringify({ findings: [], summary: "The after screenshot shows the redesigned nav bar." }), + reviewResponse: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [], confidence: 1 }), + }); + const { user } = reviewPromptOf(reviewRun); + expect(typeof user).toBe("string"); + const userText = user as string; + expect(userText).toContain(REPO_FULL_NAME); + expect(userText).toContain("Redesign the nav bar"); + expect(userText).toContain("export const Nav"); + }); + + it("the main review call NEVER receives image content blocks from the screenshot-table-vision path (text only, #cost-architecture)", async () => { + const { reviewRun } = await runWebhookPass({ + pull: 203, + headSha: "nav-sha-203", + visionResponse: JSON.stringify({ findings: [], summary: "The after screenshot shows the redesigned nav bar." }), + reviewResponse: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [], confidence: 1 }), + }); + const call = reviewRun.mock.calls[0] as unknown as [string, Record]; + const options = call[1]; + // The user message content is a plain string (toContentBlocks only returns an array when images are + // attached) -- a byte-shaped image payload would show up as an array of content blocks instead. + const messages = (options.messages as Array<{ role: string; content: unknown }>) ?? []; + for (const message of messages) { + expect(Array.isArray(message.content)).toBe(false); + } + // Belt-and-suspenders: grep the ENTIRE serialized call args for any image-shaped payload. + const serialized = JSON.stringify(options); + expect(serialized).not.toContain('"type":"image"'); + expect(serialized).not.toContain("base64"); + expect(options.images).toBeUndefined(); + }); + + it("fallback: when the vision pass produces no summary (empty response), the review runs with NO extra context -- byte-identical to no screenshot-table at all", async () => { + const { reviewRun } = await runWebhookPass({ + pull: 204, + headSha: "nav-sha-204", + // A blank self-host response degrades to "no usable output" -- no findings, no summary. + visionResponse: null, + reviewResponse: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [], confidence: 1 }), + }); + const { system } = reviewPromptOf(reviewRun); + expect(system).not.toContain("SCREENSHOT EVIDENCE"); + }); + + it("fallback: the AI review is never blocked or delayed by a vision failure (unparseable response)", async () => { + const { reviewRun } = await runWebhookPass({ + pull: 205, + headSha: "nav-sha-205", + visionResponse: "not json, just prose", + reviewResponse: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [], confidence: 1 }), + }); + expect(reviewRun).toHaveBeenCalledTimes(1); + const { system } = reviewPromptOf(reviewRun); + expect(system).not.toContain("SCREENSHOT EVIDENCE"); + }); +}); diff --git a/test/unit/screenshot-table-vision-wiring.test.ts b/test/unit/screenshot-table-vision-wiring.test.ts index ed1387e922..338d616b36 100644 --- a/test/unit/screenshot-table-vision-wiring.test.ts +++ b/test/unit/screenshot-table-vision-wiring.test.ts @@ -212,6 +212,122 @@ describe("runScreenshotTableVisionForAdvisory (#4366)", () => { ]); }); + it("(#screenshot-vision-summary) returns the vision call's plain-language evidence summary alongside its findings", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + stubShotsAndProvider( + JSON.stringify({ + findings: [{ pairIndex: 1, body: "The after screenshot shows an unrelated login page." }], + summary: "The after screenshot shows a login page, not the redesigned nav bar the PR title describes.", + }), + ); + const adv = findingsHolder(); + const summary = await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(summary).toBe("The after screenshot shows a login page, not the redesigned nav bar the PR title describes."); + // The existing findings-pipeline is unaffected by the new summary field riding in the same response. + expect(adv.findings).toEqual([ + { + code: "screenshot_table_vision_finding", + severity: "warning", + title: "Possible screenshot-table issue: pair 1", + detail: "The after screenshot shows an unrelated login page.", + action: "Advisory only — verify the screenshot-table images against the stated change before deciding.", + }, + ]); + }); + + it("(#screenshot-vision-summary) returns undefined when the provider response has no summary field, even with real findings", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + stubShotsAndProvider(findingsResponse([{ pairIndex: 1, body: "The after screenshot shows an unrelated login page." }])); + const adv = findingsHolder(); + const summary = await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(summary).toBeUndefined(); + expect(adv.findings).toHaveLength(1); + }); + + it("(#screenshot-vision-summary) runs via env.AI_VISION and returns the self-host response's summary too", async () => { + const runMock = vi.fn(async () => ({ + response: JSON.stringify({ findings: [], summary: "Both screenshots show the same redesigned nav bar, matching the PR title." }), + })); + const env = byokEnv(); + (env as unknown as { AI_VISION: unknown }).AI_VISION = { run: runMock }; + stubShotsAndProvider(null); + const adv = findingsHolder(); + const summary = await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings({ aiReviewByok: false }), + advisory: adv, + }); + expect(summary).toBe("Both screenshots show the same redesigned nav bar, matching the PR title."); + expect(adv.findings).toEqual([]); + }); + + it("(#screenshot-vision-summary) returns undefined when the deterministic gate never fires (no AI call at all)", async () => { + const env = byokEnv(); + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + const adv = findingsHolder(); + const summary = await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: "Just a plain description, no table.", + prTitle: "Fix a typo", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(summary).toBeUndefined(); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("(#screenshot-vision-summary) returns undefined for a byte-identical pair (no AI call, so no summary either)", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + stubShotsAndProvider(null, { before: [9, 9, 9], after: [9, 9, 9] }); + const adv = findingsHolder(); + const summary = await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(summary).toBeUndefined(); + }); + it("adds no finding when the provider returns an empty findings array (genuine evidence)", async () => { const env = byokEnv(); await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); diff --git a/test/unit/screenshot-table-vision.test.ts b/test/unit/screenshot-table-vision.test.ts index 3ed9f4c13a..6f0d037e3a 100644 --- a/test/unit/screenshot-table-vision.test.ts +++ b/test/unit/screenshot-table-vision.test.ts @@ -4,6 +4,7 @@ import { buildScreenshotTableVisionUserPrompt, evaluateScreenshotTableVisionGate, parseScreenshotTableVisionResponse, + parseScreenshotTableVisionSummary, SCREENSHOT_TABLE_VISION_FINDING_CODE, } from "../../src/review/visual/screenshot-table-vision"; import type { AiReviewProviderKey } from "../../src/services/ai-review"; @@ -115,6 +116,69 @@ describe("parseScreenshotTableVisionResponse", () => { const findings = Array.from({ length: 5 }, (_, i) => ({ pairIndex: 1, body: `Issue ${i}.` })); expect(parseScreenshotTableVisionResponse(JSON.stringify({ findings }), 2)).toHaveLength(2); }); + + it("(#screenshot-vision-summary) an extra 'summary' field in the same response never affects findings-parsing", () => { + const text = JSON.stringify({ + findings: [{ pairIndex: 1, body: "Both images are the same screenshot." }], + summary: "The after screenshot moves the nav bar to the right, matching the PR's stated redesign.", + }); + expect(parseScreenshotTableVisionResponse(text, 2)).toEqual([{ pairIndex: 1, body: "Both images are the same screenshot." }]); + }); +}); + +describe("parseScreenshotTableVisionSummary (#screenshot-vision-summary)", () => { + it("parses a valid summary string into public-safe text", () => { + const text = JSON.stringify({ + findings: [], + summary: "The after screenshot shows the nav bar moved to the right, matching the PR's stated redesign.", + }); + expect(parseScreenshotTableVisionSummary(text)).toBe( + "The after screenshot shows the nav bar moved to the right, matching the PR's stated redesign.", + ); + }); + + it("is independent of the findings array -- a response with real findings still yields its summary", () => { + const text = JSON.stringify({ + findings: [{ pairIndex: 1, body: "Both images are the same screenshot." }], + summary: "The two screenshots look identical, which does not support the stated change.", + }); + expect(parseScreenshotTableVisionSummary(text)).toBe( + "The two screenshots look identical, which does not support the stated change.", + ); + }); + + it("returns undefined for a missing summary field", () => { + expect(parseScreenshotTableVisionSummary(JSON.stringify({ findings: [] }))).toBeUndefined(); + }); + + it("returns undefined for a non-string summary field", () => { + expect(parseScreenshotTableVisionSummary(JSON.stringify({ findings: [], summary: 42 }))).toBeUndefined(); + }); + + it("returns undefined for a blank/whitespace-only summary (fails toPublicSafe's emptiness guard)", () => { + expect(parseScreenshotTableVisionSummary(JSON.stringify({ findings: [], summary: " " }))).toBeUndefined(); + }); + + it("returns undefined for text with no JSON object at all", () => { + expect(parseScreenshotTableVisionSummary("not json, just prose")).toBeUndefined(); + }); + + it("returns undefined for a balanced-brace object that is still invalid JSON (e.g. a trailing comma)", () => { + expect(parseScreenshotTableVisionSummary('{"summary": "x",}')).toBeUndefined(); + }); + + it("truncates a summary longer than MAX_SCREENSHOT_TABLE_VISION_SUMMARY_CHARS", () => { + const longSummary = "A".repeat(1000); + const result = parseScreenshotTableVisionSummary(JSON.stringify({ findings: [], summary: longSummary })); + expect(result).toBeDefined(); + expect(result?.length).toBe(600); + }); + + it("trims a summary within the bound instead of always slicing to the max", () => { + expect(parseScreenshotTableVisionSummary(JSON.stringify({ findings: [], summary: " A short summary. " }))).toBe( + "A short summary.", + ); + }); }); describe("buildScreenshotTableVisionFindings", () => {