From 879e84c8fa554aea1508b60a79c410ed9d7f6838 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 21 Jul 2026 01:43:04 -0700 Subject: [PATCH] feat(review): thread screenshot-table vision evidence into the main AI review The main AI code review (the narrative assessment/blockers/nits) never saw any images, contributor-submitted or otherwise. Contributor-submitted before/after screenshot-tables already got a real vision-AI pass, but it was scoped strictly to gaming detection (duplicate/irrelevant images), never correctness or UX judgment, and its output only ever fed a separate advisory list the main review never read. Extends the SAME single vision call (self-hosted env.AI_VISION or BYOK, no extra GPU cost) to also return a plain-language evidence summary alongside its existing gaming-detection findings, and reorders the pass to run before the main AI review so that summary exists in time. The summary is threaded into the main review's prompt as plain TEXT ONLY, appended the same way repoInstructions/pathGuidance already are (absent/blank leaves the prompt byte-identical) -- never routed through the images/content-block parameter, so the frontier-model review call's token cost grows by a small amount of text and never by image tokens. The existing gaming-detection behavior, its gating (reputation/provider/ image-pair presence), and the deterministic missing-screenshot-table close gate are all unchanged. --- src/queue/ai-review-orchestration.ts | 14 ++ src/queue/processors.ts | 87 +++++-- src/review/visual/screenshot-table-vision.ts | 69 +++++- src/services/ai-review.ts | 33 ++- test/unit/ai-review.test.ts | 56 +++++ ...screenshot-evidence-summary-wiring.test.ts | 223 ++++++++++++++++++ .../screenshot-table-vision-wiring.test.ts | 116 +++++++++ test/unit/screenshot-table-vision.test.ts | 64 +++++ 8 files changed, 633 insertions(+), 29 deletions(-) create mode 100644 test/unit/screenshot-evidence-summary-wiring.test.ts 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", () => {