From 0ac69ef90c82d93164cccdc6ec284a59eff883e2 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 05:15:46 -0700 Subject: [PATCH] perf(review): downscale before/after screenshots before the local VLM (#4370) Vision-vision screenshots are captured fullPage, so image HEIGHT scales with a page's full scrollable content even at a fixed 1440px viewport width -- Qwen's dynamic-resolution encoder tokenizes proportional to pixel count, inflating vision prefill cost/latency on long pages. Adds a downscale-to-1280px-longest-edge step scoped ONLY to the bytes sent to the vision model (fetchShotContentBlock), via the same Worker-safe-stub / self-host-real-implementation seam already used for pixel-diff.ts and scroll-gif.ts -- sharp is a native binding and can't run on the Cloudflare Workers runtime, so capture.ts (Worker-reachable) imports only a no-op default; the real resize is swapped in by build-selfhost.mjs's esbuild plugin for the self-host bundle only. Stored/displayed screenshots (the same URL embedded in the PR comment) are completely untouched. Verified with a real local docker build + boot on the target architecture: sharp must be a genuine `dependencies` entry (not devDependencies) since NODE_ENV=production in the runtime image silently skips an explicit `npm install` of a devDependency-classified package -- caught only by actually building and booting the image, not by static review, mirroring how #4354 was diagnosed. --- Dockerfile | 8 +++ package-lock.json | 27 +--------- package.json | 1 + scripts/build-selfhost.mjs | 7 ++- src/review/visual/capture.ts | 3 +- src/review/visual/image-downscale.ts | 11 ++++ src/selfhost/stubs/image-downscale.ts | 32 ++++++++++++ test/unit/image-downscale.test.ts | 14 +++++ .../selfhost-image-downscale-stub.test.ts | 51 +++++++++++++++++++ test/unit/worker-entry-boundary.test.ts | 6 +-- 10 files changed, 129 insertions(+), 31 deletions(-) create mode 100644 src/review/visual/image-downscale.ts create mode 100644 src/selfhost/stubs/image-downscale.ts create mode 100644 test/unit/image-downscale.test.ts create mode 100644 test/unit/selfhost-image-downscale-stub.test.ts diff --git a/Dockerfile b/Dockerfile index 5d73b086ff..45322820a0 100644 --- a/Dockerfile +++ b/Dockerfile @@ -68,6 +68,14 @@ USER root ARG INSTALL_VISUAL_REVIEW=false COPY package*.json ./ RUN if [ "$INSTALL_VISUAL_REVIEW" = "true" ]; then npm install puppeteer-core@22.13.1 --ignore-scripts; fi +# sharp (#4370): esbuild marks it `external` in the --all bundle (a native per-platform binary can't be +# bundled into dist/server.mjs, see scripts/build-selfhost.mjs), so it must be installed separately here, +# same reason as puppeteer-core above -- but unconditional (not behind an opt-in build-arg): it's a core +# dependency of the vision-image-downscale path, not an optional external-sidecar feature. --ignore-scripts +# is safe here: sharp's own platform binary ships as an npm `optionalDependencies` entry +# (@img/sharp--) that npm's normal os/cpu-matched install resolves on its own, not via +# sharp's postinstall script. +RUN npm install sharp@0.34.5 --ignore-scripts # Data dir (the SQLite file) — owned by the unprivileged node user; mount a volume here to persist. RUN mkdir -p /data && chown -R node:node /data /app # Expose the optional user-installed CLIs only after all root build steps have completed, so a diff --git a/package-lock.json b/package-lock.json index a84c22e4b7..24b2b04e86 100644 --- a/package-lock.json +++ b/package-lock.json @@ -30,6 +30,7 @@ "hono": "^4.12.27", "ioredis": "^5.11.1", "pg": "^8.22.0", + "sharp": "^0.34.5", "yaml": "^2.9.0", "zod": "^4.4.3" }, @@ -2618,7 +2619,6 @@ "version": "1.1.0", "resolved": "https://registry.npmjs.org/@img/colour/-/colour-1.1.0.tgz", "integrity": "sha512-Td76q7j57o/tLVdgS746cYARfSyxk8iEfRxewL9h4OMzYhbW4TAcppl0mT4eyqXddh6L/jwoM75mo7ixa/pCeQ==", - "dev": true, "license": "MIT", "engines": { "node": ">=18" @@ -2631,7 +2631,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "Apache-2.0", "optional": true, "os": [ @@ -2654,7 +2653,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "Apache-2.0", "optional": true, "os": [ @@ -2677,7 +2675,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -2694,7 +2691,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -2711,7 +2707,6 @@ "cpu": [ "arm" ], - "dev": true, "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -2728,7 +2723,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -2745,7 +2739,6 @@ "cpu": [ "ppc64" ], - "dev": true, "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -2762,7 +2755,6 @@ "cpu": [ "riscv64" ], - "dev": true, "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -2779,7 +2771,6 @@ "cpu": [ "s390x" ], - "dev": true, "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -2796,7 +2787,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -2813,7 +2803,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -2830,7 +2819,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -2847,7 +2835,6 @@ "cpu": [ "arm" ], - "dev": true, "license": "Apache-2.0", "optional": true, "os": [ @@ -2870,7 +2857,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "Apache-2.0", "optional": true, "os": [ @@ -2893,7 +2879,6 @@ "cpu": [ "ppc64" ], - "dev": true, "license": "Apache-2.0", "optional": true, "os": [ @@ -2916,7 +2901,6 @@ "cpu": [ "riscv64" ], - "dev": true, "license": "Apache-2.0", "optional": true, "os": [ @@ -2939,7 +2923,6 @@ "cpu": [ "s390x" ], - "dev": true, "license": "Apache-2.0", "optional": true, "os": [ @@ -2962,7 +2945,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "Apache-2.0", "optional": true, "os": [ @@ -2985,7 +2967,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "Apache-2.0", "optional": true, "os": [ @@ -3008,7 +2989,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "Apache-2.0", "optional": true, "os": [ @@ -3031,7 +3011,6 @@ "cpu": [ "wasm32" ], - "dev": true, "license": "Apache-2.0 AND LGPL-3.0-or-later AND MIT", "optional": true, "dependencies": { @@ -3051,7 +3030,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "Apache-2.0 AND LGPL-3.0-or-later", "optional": true, "os": [ @@ -3071,7 +3049,6 @@ "cpu": [ "ia32" ], - "dev": true, "license": "Apache-2.0 AND LGPL-3.0-or-later", "optional": true, "os": [ @@ -3091,7 +3068,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "Apache-2.0 AND LGPL-3.0-or-later", "optional": true, "os": [ @@ -14452,7 +14428,6 @@ "version": "0.34.5", "resolved": "https://registry.npmjs.org/sharp/-/sharp-0.34.5.tgz", "integrity": "sha512-Ou9I5Ft9WNcCbXrU9cMgPBcCK8LiwLqcbywW3t4oDV37n1pzpuNLsYiAV8eODnjbtQlSDwZ2cUEeQz4E54Hltg==", - "dev": true, "hasInstallScript": true, "license": "Apache-2.0", "dependencies": { diff --git a/package.json b/package.json index 8055b6994f..34874c2b4b 100644 --- a/package.json +++ b/package.json @@ -101,6 +101,7 @@ "hono": "^4.12.27", "ioredis": "^5.11.1", "pg": "^8.22.0", + "sharp": "^0.34.5", "yaml": "^2.9.0", "zod": "^4.4.3" }, diff --git a/scripts/build-selfhost.mjs b/scripts/build-selfhost.mjs index f056faa418..77a536ac06 100644 --- a/scripts/build-selfhost.mjs +++ b/scripts/build-selfhost.mjs @@ -23,7 +23,10 @@ await esbuild.build({ sourcesContent: true, sourceRoot: "/app/dist", // External: nothing (bundle all) vs every package (external). node: builtins are always external on node. - ...(bundleAll ? {} : { packages: "external" }), + // sharp (#4370) is ALWAYS external even in --all mode: it ships a native per-platform binary esbuild + // cannot bundle, unlike sharp's own JS glue code -- the Dockerfile installs it separately into the + // runtime image (see the runtime-base stage). + ...(bundleAll ? { external: ["sharp"] } : { packages: "external" }), // Bundling CJS deps into an ESM output needs require/__dirname/__filename shimmed (some deps call them). ...(bundleAll ? { @@ -54,6 +57,8 @@ await esbuild.build({ build.onResolve({ filter: /^\.\/pixel-diff$/ }, () => ({ path: resolve(root, "src/selfhost/stubs/pixel-diff.ts") })); // Same pattern for scroll-through GIF assembly (#3612) — pngjs decode + gifenc encode need Node Buffer. build.onResolve({ filter: /^\.\/scroll-gif$/ }, () => ({ path: resolve(root, "src/selfhost/stubs/scroll-gif.ts") })); + // Same pattern for vision-image downscaling (#4370) — sharp is a native binding, Worker-unsafe. + build.onResolve({ filter: /^\.\/image-downscale$/ }, () => ({ path: resolve(root, "src/selfhost/stubs/image-downscale.ts") })); }, }, ], diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index b1c7dd5cf6..a998dca496 100644 --- a/src/review/visual/capture.ts +++ b/src/review/visual/capture.ts @@ -16,6 +16,7 @@ // default TanStack route convention; those hooks can return if a per-repo visual config is added. import { base64Encode, sha256Hex } from "../../utils/crypto"; import type { AiContentBlock } from "../../types"; +import { downscaleForVision } from "./image-downscale"; import type { GitHubRateLimitAdmissionKey } from "../../github/client"; import { dispatchVisualCaptureFallback, fallbackShotR2Key, isFallbackDispatchInFlight, markFallbackDispatched } from "./actions-fallback"; import { @@ -116,7 +117,7 @@ export async function fetchShotContentBlock(url: string): Promise { + return png; +} diff --git a/src/selfhost/stubs/image-downscale.ts b/src/selfhost/stubs/image-downscale.ts new file mode 100644 index 0000000000..c323120e20 --- /dev/null +++ b/src/selfhost/stubs/image-downscale.ts @@ -0,0 +1,32 @@ +// Self-host replacement for src/review/visual/image-downscale.ts (#4370). Swapped in by +// scripts/build-selfhost.mjs's esbuild plugin, the same mechanism used for @cloudflare/puppeteer and +// ./pixel-diff — this file is only ever bundled into dist/server.mjs, never the Worker entry, so it's safe +// to depend on sharp (a native binding) here. Unlike puppeteer-core, sharp is marked `external` in the +// --all esbuild bundle (a native binding can't be bundled) and installed separately into the runtime Docker +// image (see the Dockerfile) rather than lazily imported at call time — sharp has no browser-sidecar-style +// opt-in the way puppeteer-core does, so a plain static import is fine. +import sharp from "sharp"; + +/** Longest-edge cap for the bytes sent to the local VLM — NOT the stored/displayed screenshot (the same URL + * is embedded verbatim in the PR comment; see review/visual/capture.ts's unified-comment-bridge caller). + * shot.ts captures `fullPage: true`, so image HEIGHT scales with the page's full scrollable content even + * though the viewport is a fixed 1440px wide — Qwen's dynamic-resolution vision encoder tokenizes + * proportional to pixel count, so an oversized tall page inflates vision prefill cost/latency for no + * quality gain a human reviewer would notice at chat-image resolution. */ +const VISION_MAX_DIMENSION_PX = 1280; + +/** Downscale `png` so its longest edge is at most {@link VISION_MAX_DIMENSION_PX}, preserving aspect ratio + * and never enlarging an already-small image. Any decode/resize failure (a corrupt/unexpected payload) + * degrades to the ORIGINAL bytes rather than dropping the image — a vision call on a full-size image is + * strictly better than no image at all. */ +export async function downscaleForVision(png: Uint8Array): Promise { + try { + const resized = await sharp(png) + .resize({ width: VISION_MAX_DIMENSION_PX, height: VISION_MAX_DIMENSION_PX, fit: "inside", withoutEnlargement: true }) + .png() + .toBuffer(); + return new Uint8Array(resized); + } catch { + return png; + } +} diff --git a/test/unit/image-downscale.test.ts b/test/unit/image-downscale.test.ts new file mode 100644 index 0000000000..93699dd049 --- /dev/null +++ b/test/unit/image-downscale.test.ts @@ -0,0 +1,14 @@ +import { describe, expect, it } from "vitest"; +import { downscaleForVision } from "../../src/review/visual/image-downscale"; + +describe("image-downscale Worker-safe default (#4370)", () => { + it("returns the input bytes unchanged, since the real implementation is self-host only", async () => { + const png = new Uint8Array([137, 80, 78, 71, 1, 2, 3]); + await expect(downscaleForVision(png)).resolves.toBe(png); + }); + + it("returns an empty input unchanged", async () => { + const empty = new Uint8Array([]); + await expect(downscaleForVision(empty)).resolves.toBe(empty); + }); +}); diff --git a/test/unit/selfhost-image-downscale-stub.test.ts b/test/unit/selfhost-image-downscale-stub.test.ts new file mode 100644 index 0000000000..041a260ac4 --- /dev/null +++ b/test/unit/selfhost-image-downscale-stub.test.ts @@ -0,0 +1,51 @@ +// Tests for the self-host vision-image-downscale stub (#4370). This module is never bundled into the +// Worker entry (scripts/build-selfhost.mjs swaps it in only when building src/server.ts — see +// test/unit/worker-entry-boundary.test.ts for the enforced side of that), so it's safe to depend on sharp +// / real PNG fixtures here, mirroring test/unit/selfhost-pixel-diff-stub.test.ts's own fixture style. +import sharp from "sharp"; +import { describe, expect, it } from "vitest"; +import { downscaleForVision } from "../../src/selfhost/stubs/image-downscale"; + +async function solidPng(width: number, height: number): Promise { + const buf = await sharp({ create: { width, height, channels: 3, background: { r: 10, g: 20, b: 30 } } }) + .png() + .toBuffer(); + return new Uint8Array(buf); +} + +async function dimensionsOf(png: Uint8Array): Promise<{ width: number; height: number }> { + const meta = await sharp(Buffer.from(png)).metadata(); + return { width: meta.width ?? 0, height: meta.height ?? 0 }; +} + +describe("selfhost image-downscale stub (#4370)", () => { + it("downscales a wide image so the longest edge (width) is capped, preserving aspect ratio", async () => { + const wide = await solidPng(2000, 500); + const result = await downscaleForVision(wide); + const { width, height } = await dimensionsOf(result); + expect(width).toBe(1280); + expect(height).toBe(320); // 500/2000 * 1280 + }); + + it("downscales a tall image (the real fullPage-capture shape) so the longest edge (height) is capped", async () => { + const tall = await solidPng(500, 2000); + const result = await downscaleForVision(tall); + const { width, height } = await dimensionsOf(result); + expect(height).toBe(1280); + expect(width).toBe(320); // 500/2000 * 1280 + }); + + it("leaves an already-small image's dimensions unchanged (withoutEnlargement)", async () => { + const small = await solidPng(800, 600); + const result = await downscaleForVision(small); + const { width, height } = await dimensionsOf(result); + expect(width).toBe(800); + expect(height).toBe(600); + }); + + it("degrades to the ORIGINAL bytes (never drops the image) when the input isn't a valid image", async () => { + const garbage = new Uint8Array([1, 2, 3, 4, 5]); + const result = await downscaleForVision(garbage); + expect(result).toBe(garbage); + }); +}); diff --git a/test/unit/worker-entry-boundary.test.ts b/test/unit/worker-entry-boundary.test.ts index efc58ad0c0..1a06395737 100644 --- a/test/unit/worker-entry-boundary.test.ts +++ b/test/unit/worker-entry-boundary.test.ts @@ -10,7 +10,7 @@ const WORKER_ENTRY = join(srcRoot, "index.ts"); const MCP_BIN = join(root, "packages/gittensory-mcp/bin/gittensory-mcp.js"); const FORBIDDEN_PATH = /(?:^|\/)visual-agent\//; -const FORBIDDEN_IDENTIFIERS = /\b(?:pixelmatch|pngjs|visual-diff|gifenc)\b/; +const FORBIDDEN_IDENTIFIERS = /\b(?:pixelmatch|pngjs|visual-diff|gifenc|sharp)\b/; function resolveLocalImport(fromFile: string, specifier: string): string | null { if (!specifier.startsWith(".")) return null; @@ -72,14 +72,14 @@ describe("worker entry boundary", () => { expect(forbidden, `worker entry must not reach agent-only modules: ${forbidden.join(", ")}`).toEqual([]); }); - it("does not reference pixelmatch, pngjs, visual-diff, or gifenc in worker-reachable source", () => { + it("does not reference pixelmatch, pngjs, visual-diff, gifenc, or sharp in worker-reachable source", () => { const hits = collectReachableSources(WORKER_ENTRY) .map((file) => { const content = readFileSync(file, "utf8"); return FORBIDDEN_IDENTIFIERS.test(content) ? relativeToRoot(file) : null; }) .filter((entry): entry is string => entry !== null); - expect(hits, `worker-reachable files must not mention Node-only visual diff/GIF deps: ${hits.join(", ")}`).toEqual([]); + expect(hits, `worker-reachable files must not mention Node-only visual diff/GIF/image deps: ${hits.join(", ")}`).toEqual([]); }); it("does not reference visual diff or GIF modules in the published MCP bin bundle", () => {