diff --git a/src/review/visual/visual-findings.ts b/src/review/visual/visual-findings.ts index 403c6e6ac1..cae519c877 100644 --- a/src/review/visual/visual-findings.ts +++ b/src/review/visual/visual-findings.ts @@ -134,6 +134,10 @@ export type VisualVisionFinding = { path: string; body: string; category?: "regr /** Cap on findings kept from a single vision response — mirrors `composeAdvisoryNotes`'s selectivity so a * verbose model can't pad the comment with a long list of minor observations. */ const MAX_VISUAL_FINDINGS = 3; +/** Upper bound on a model-authored finding `path` after public-safe filtering, so one finding's title cannot + * flood a public comment. A real route path is far shorter; a value past this is truncated for display and + * simply won't match any captured route in findVisualEvidence (#10062). */ +const MAX_VISUAL_PATH_CHARS = 256; export const VISUAL_VISION_SYSTEM_PROMPT = [ "You are reviewing a BEFORE (production) vs AFTER (this pull request's preview deploy) screenshot pair for the same route.", @@ -217,10 +221,14 @@ export function parseVisualVisionResponse(text: string): VisualVisionFinding[] { if (out.length >= MAX_VISUAL_FINDINGS) break; if (!entry || typeof entry !== "object") continue; const record = entry as Record; - const path = typeof record.path === "string" ? record.path.trim() : ""; + // path is model-authored free text off the vision JSON, interpolated into a public finding title — filter + // it through toPublicSafe exactly as body is, dropping the entry when it returns null (#10062). + const rawPath = typeof record.path === "string" ? record.path : ""; + const filteredPath = toPublicSafe(rawPath); const rawBody = typeof record.body === "string" ? record.body : ""; const body = toPublicSafe(rawBody); - if (!path || !body) continue; + if (!filteredPath || !body) continue; + const path = filteredPath.slice(0, MAX_VISUAL_PATH_CHARS); const category = record.category === "regression" || record.category === "unrelated" ? record.category : undefined; out.push(category ? { path, body, category } : { path, body }); } diff --git a/test/unit/visual-findings.test.ts b/test/unit/visual-findings.test.ts index f950c5fd12..506ea6f1cd 100644 --- a/test/unit/visual-findings.test.ts +++ b/test/unit/visual-findings.test.ts @@ -200,6 +200,28 @@ describe("parseVisualVisionResponse", () => { expect(parseVisualVisionResponse(text)).toEqual([]); }); + it("REGRESSION (#10062): filters public-unsafe markup out of a model-authored path", () => { + const text = JSON.stringify({ findings: [{ path: "/app [pwn](http://evil.example) @maintainer", body: "Broke." }] }); + const [finding] = parseVisualVisionResponse(text); + expect(finding).toBeDefined(); + // The live link-injection markup no longer survives into the public finding path. + expect(finding!.path).not.toContain("](http://evil.example)"); + expect(finding!.path).not.toContain("[pwn]"); + }); + + it("REGRESSION (#10062): bounds a path to the MAX_VISUAL_PATH_CHARS constant", () => { + const text = JSON.stringify({ findings: [{ path: `/${"a".repeat(500)}`, body: "Broke." }] }); + const [finding] = parseVisualVisionResponse(text); + expect(finding!.path.length).toBe(256); + }); + + it("REGRESSION (#10062): an ordinary route path round-trips byte-identically and still attaches visualEvidence", () => { + const findings = parseVisualVisionResponse(JSON.stringify({ findings: [{ path: "/app", body: "Broke." }] })); + expect(findings).toEqual([{ path: "/app", body: "Broke." }]); // survives toPublicSafe unchanged + const built = buildVisualRegressionFindings(findings, [changedRoute("/app")]); + expect(built[0]?.visualEvidence?.path).toBe("/app"); // route lookup still matches on the scrubbed path + }); + it("drops an entry with a blank/empty body (fails toPublicSafe's emptiness guard)", () => { const text = JSON.stringify({ findings: [{ path: "/pricing", body: "" }] }); expect(parseVisualVisionResponse(text)).toEqual([]); diff --git a/test/unit/visual-followup.test.ts b/test/unit/visual-followup.test.ts index 163b29323d..817303e092 100644 --- a/test/unit/visual-followup.test.ts +++ b/test/unit/visual-followup.test.ts @@ -5,7 +5,7 @@ import { selectUnrelatedVisualFindings, } from "../../src/review/visual/visual-followup"; import { VISUAL_FOLLOWUP_COMMENT_MARKER } from "../../src/github/comments"; -import { VISUAL_REGRESSION_FINDING_CODE, VISUAL_UNRELATED_ISSUE_FINDING_CODE } from "../../src/review/visual/visual-findings"; +import { buildVisualRegressionFindings, parseVisualVisionResponse, VISUAL_REGRESSION_FINDING_CODE, VISUAL_UNRELATED_ISSUE_FINDING_CODE } from "../../src/review/visual/visual-findings"; import type { AdvisoryFinding } from "../../src/types"; const unrelatedFinding = (overrides: Partial = {}): AdvisoryFinding => ({ @@ -135,4 +135,14 @@ describe("buildVisualFollowupComment", () => { expect(body).not.toContain("Possible visual regression"); expect(body).toContain("Possible unrelated visual issue"); }); + + it("REGRESSION (#10062): a hostile model path is scrubbed at parse time and never reaches the follow-up body as live markup", () => { + const hostile = JSON.stringify({ + findings: [{ path: "/app [pwn](http://evil.example) @maintainer", body: "Layout looks off, unrelated to this change.", category: "unrelated" }], + }); + const built = buildVisualRegressionFindings(parseVisualVisionResponse(hostile)); + const body = buildVisualFollowupComment(built, ["jsonbored"]); + expect(body).not.toBeNull(); + expect(body!).not.toContain("](http://evil.example)"); // the injected link never renders live + }); });