diff --git a/review-enrichment/src/analyzers/asset-weight.ts b/review-enrichment/src/analyzers/asset-weight.ts new file mode 100644 index 0000000000..158e535938 --- /dev/null +++ b/review-enrichment/src/analyzers/asset-weight.ts @@ -0,0 +1,268 @@ +// Image/binary asset weight-delta analyzer (#1506). Flags a PR that commits or grows a heavy image/font/binary +// blob — repo + CDN/cold-start bloat the textual diff hides behind "Binary files differ". Binary sizes are not in +// the patch, so this is the one analyzer that needs the GitHub API: the git tree at headSha (and baseSha, for +// modified files) is fetched with the request's short-lived token — one recursive call returns every blob's size, +// which also sidesteps the Contents API's 1 MB cap. Pure size arithmetic after that; no external service. +// Fail-safe: returns [] without a token/headSha or when the head tree fetch is not OK; growth findings require a +// matching base size. +import type { EnrichRequest, AssetWeightFinding } from "../types.js"; + +const MAX_FINDINGS = 50; // keep the brief bounded after evaluating every changed binary candidate +const THRESHOLD_BYTES = 100 * 1024; // flag a newly-added blob >= 100 KB, or growth >= 100 KB +const GITHUB_API = "https://api.github.com"; +const GITHUB_API_VERSION = "2022-11-28"; + +// Extensions that are genuinely binary (text formats like .svg/.json are excluded — their bytes are in the diff). +const BINARY_EXTS = new Set([ + "png", + "jpg", + "jpeg", + "gif", + "bmp", + "tiff", + "tif", + "ico", + "webp", + "avif", + "woff", + "woff2", + "ttf", + "otf", + "eot", + "mp4", + "mov", + "avi", + "webm", + "mkv", + "mp3", + "wav", + "flac", + "ogg", + "zip", + "tar", + "gz", + "tgz", + "bz2", + "7z", + "rar", + "xz", + "pdf", + "psd", + "ai", + "sketch", + "fig", + "xcf", + "exe", + "dll", + "so", + "dylib", + "bin", + "dat", + "wasm", + "node", + "jar", + "class", +]); + +interface ScanOptions { + signal?: AbortSignal; +} + +// A single repo path segment (owner or name): word chars, dot, dash only. Whole-segment `.`/`..` are rejected +// separately so they can't traverse. A commit SHA: hex only — we only ever fetch a real object, never an arbitrary ref. +const REPO_SEGMENT = /^[A-Za-z0-9._-]+$/; +const SHA_RE = /^[0-9a-fA-F]{7,64}$/; + +function isBinaryAsset(path: string): boolean { + const dot = path.lastIndexOf("."); + return dot >= 0 && BINARY_EXTS.has(path.slice(dot + 1).toLowerCase()); +} + +type EnrichFile = NonNullable[number]; + +function basePathForGrowth(file: EnrichFile): string | null { + if (file.status === "modified" || file.status === "changed") return file.path; + if (file.status === "renamed") return file.previousPath || null; + return null; +} + +function githubHeaders(token: string): Record { + return { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": GITHUB_API_VERSION, + "User-Agent": "gittensory-review-enrichment", + }; +} + +function encodeRepoPath(path: string): string | null { + const segments = path.split("/"); + if (!path || segments.some((seg) => !seg || seg === "." || seg === "..")) { + return null; + } + return segments.map(encodeURIComponent).join("/"); +} + +/** Parse `owner/repo`, rejecting anything that isn't exactly two safe segments — no extra `/`, no `.`/`..` + * traversal, no query/fragment characters. This stops a hostile `repoFullName` from redirecting the + * token-bearing request to another repository. Returns null when unsafe. */ +function parseRepo( + repoFullName: string, +): { owner: string; repo: string } | null { + const parts = repoFullName.split("/"); + if (parts.length !== 2) return null; + const [owner, repo] = parts; + for (const seg of [owner, repo]) { + if (!seg || seg === "." || seg === ".." || !REPO_SEGMENT.test(seg)) { + return null; + } + } + return { owner: owner!, repo: repo! }; +} + +/** Fetch every blob's byte size in the repo's git tree at `sha`. One recursive call. Empty map on an invalid SHA + * or a non-OK reply. `owner`/`repo` are validated by the caller; every segment is URL-encoded here (defense in + * depth) so nothing user-derived can break out of the intended API path. */ +async function fetchTreeSizes( + owner: string, + repo: string, + sha: string, + token: string, + fetchImpl: typeof fetch, + signal?: AbortSignal, +): Promise<{ sizes: Map; truncated: boolean }> { + const sizes = new Map(); + if (!SHA_RE.test(sha)) return { sizes, truncated: false }; + const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/git/trees/${encodeURIComponent(sha)}?recursive=1`; + const res = await fetchImpl(url, { + headers: githubHeaders(token), + signal, + }); + if (!res.ok) return { sizes, truncated: false }; + const json = (await res.json()) as { + tree?: Array<{ path?: string; type?: string; size?: number }>; + truncated?: boolean; + }; + for (const entry of json.tree ?? []) { + if (entry.type === "blob" && typeof entry.size === "number" && entry.path) { + sizes.set(entry.path, entry.size); + } + } + return { sizes, truncated: json.truncated === true }; +} + +async function fetchPathSizes( + owner: string, + repo: string, + sha: string, + token: string, + paths: Iterable, + fetchImpl: typeof fetch, + signal?: AbortSignal, +): Promise> { + const sizes = new Map(); + if (!SHA_RE.test(sha)) return sizes; + for (const path of new Set(paths)) { + const encodedPath = encodeRepoPath(path); + if (!encodedPath) continue; + const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encodedPath}?ref=${encodeURIComponent(sha)}`; + const res = await fetchImpl(url, { headers: githubHeaders(token), signal }); + if (!res.ok) continue; + const json = (await res.json()) as { type?: string; size?: number } | unknown[]; + if (!Array.isArray(json) && typeof json.size === "number") { + sizes.set(path, json.size); + } + } + return sizes; +} + +async function fetchRelevantSizes( + owner: string, + repo: string, + sha: string, + token: string, + paths: Iterable, + fetchImpl: typeof fetch, + signal?: AbortSignal, +): Promise> { + const tree = await fetchTreeSizes(owner, repo, sha, token, fetchImpl, signal); + if (!tree.truncated) return tree.sizes; + return fetchPathSizes(owner, repo, sha, token, paths, fetchImpl, signal); +} + +/** Analyzer entrypoint: flag heavy binary assets the PR adds or grows past the threshold. Pure size arithmetic over + * the GitHub git tree; fail-safe (returns [] without a token or on a failed head tree fetch). */ +export async function scanAssetWeight( + req: EnrichRequest, + fetchImpl: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + const token = req.githubToken; + if (!token || !req.headSha) return []; + const repo = parseRepo(req.repoFullName); + if (!repo) return []; + + const binaries = (req.files ?? []).filter( + (f) => f.status !== "removed" && isBinaryAsset(f.path), + ); + if (!binaries.length) return []; + + const headSizes = await fetchRelevantSizes( + repo.owner, + repo.repo, + req.headSha, + token, + binaries.map((file) => file.path), + fetchImpl, + options.signal, + ); + const basePaths = binaries.flatMap((file) => basePathForGrowth(file) ?? []); + const needBase = binaries.some((f) => basePathForGrowth(f) !== null); + const baseSizes = + needBase && req.baseSha + ? await fetchRelevantSizes( + repo.owner, + repo.repo, + req.baseSha, + token, + basePaths, + fetchImpl, + options.signal, + ) + : new Map(); + + const findings: AssetWeightFinding[] = []; + for (const file of binaries) { + const bytes = headSizes.get(file.path); + if (typeof bytes !== "number") continue; + + if (file.status === "added" || file.status === "copied") { + if (bytes >= THRESHOLD_BYTES) { + findings.push({ + path: file.path, + bytes, + deltaBytes: bytes, + status: "added", + }); + } + continue; + } + + const basePath = basePathForGrowth(file); + if (basePath) { + const baseBytes = baseSizes.get(basePath); + if (typeof baseBytes !== "number") continue; + const deltaBytes = bytes - baseBytes; + if (deltaBytes < THRESHOLD_BYTES) continue; + findings.push({ + path: file.path, + bytes, + deltaBytes, + status: "grown", + }); + } + } + return findings + .sort((a, b) => b.deltaBytes - a.deltaBytes) + .slice(0, MAX_FINDINGS); +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index a2ca974bd4..6cbf74b70c 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -17,6 +17,7 @@ import { scanRedos } from "./analyzers/redos.js"; import { scanProvenance } from "./analyzers/provenance.js"; import { scanCodeowners } from "./analyzers/codeowners.js"; import { scanSecretLog } from "./analyzers/secret-log.js"; +import { scanAssetWeight } from "./analyzers/asset-weight.js"; import { renderBrief } from "./render.js"; type AnalyzerFn = (req: EnrichRequest, signal: AbortSignal) => Promise; @@ -33,6 +34,7 @@ const ANALYZERS: Record = { provenance: (req, signal) => scanProvenance(req, fetch, { signal }), codeowners: (req, signal) => scanCodeowners(req, fetch, { signal }), secretLog: (req, signal) => scanSecretLog(req, signal), + assetWeight: (req, signal) => scanAssetWeight(req, fetch, { signal }), }; function runWithTimeout( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 2533c42122..3008b706f0 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -34,6 +34,12 @@ function promptText(value: string): string { .replace(/([*_{}[\]()#+.!|-])/g, "\\$1"); } +function formatBytes(n: number): string { + if (n >= 1048576) return `${(n / 1048576).toFixed(1)} MiB`; + if (n >= 1024) return `${(n / 1024).toFixed(0)} KiB`; + return `${n} B`; +} + /** Build the `promptSection` (verbatim splice) + a one-line `systemSuffix` from the findings. Empty when nothing found. */ export function renderBrief( findings: BriefFindings, @@ -195,6 +201,20 @@ export function renderBrief( } } + const assets = findings.assetWeight ?? []; + if (assets.length) { + lines.push( + "### Heavy binary assets (optimize, or move to a CDN / Git LFS)", + ); + for (const item of assets) { + const detail = + item.status === "added" + ? `adds ${formatBytes(item.bytes)}` + : `grows +${formatBytes(item.deltaBytes)} to ${formatBytes(item.bytes)}`; + lines.push(`- ${safeCodeSpan(item.path)} ${detail}`); + } + } + if (!lines.length) return { promptSection: "", systemSuffix: "" }; const header = diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index f533c9f743..b7de952841 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -13,6 +13,7 @@ export interface EnrichRequest { files?: Array<{ path: string; status?: string; + previousPath?: string; patch?: string; additions?: number; deletions?: number; @@ -123,6 +124,15 @@ export interface SecretLogFinding { category: "secret" | "pii" | "request-object"; } +/** A heavy binary asset the PR adds or grows. `bytes` is the size at headSha; `deltaBytes` is the growth vs base + * (equal to `bytes` for a newly-added file). */ +export interface AssetWeightFinding { + path: string; + bytes: number; + deltaBytes: number; + status: "added" | "grown"; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -135,6 +145,7 @@ export interface BriefFindings { provenance?: ProvenanceFinding[]; codeowners?: CodeownersFinding[]; secretLog?: SecretLogFinding[]; + assetWeight?: AssetWeightFinding[]; } export type AnalyzerStatus = "ok" | "degraded" | "skipped"; diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index 743983a95d..4d9d65db87 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -21,6 +21,7 @@ import { scanPatchForRedos, scanRedos, } from "../dist/analyzers/redos.js"; +import { scanAssetWeight } from "../dist/analyzers/asset-weight.js"; import { classifyAddedFile, isSafeToCheck, @@ -29,6 +30,7 @@ import { matchesPypiVersion, scanProvenance, } from "../dist/analyzers/provenance.js"; +import { findOwners, parseCodeowners, patternToRegex, @@ -1504,6 +1506,221 @@ test("scanProvenance: handles undefined files gracefully", async () => { assert.deepEqual(findings, []); }); +const treeReply = (tree) => ({ ok: true, json: async () => ({ tree }) }); +const HEAD_SHA = "1111111111111111111111111111111111111111"; +const BASE_SHA = "2222222222222222222222222222222222222222"; + +test("scanAssetWeight: flags a large newly-added binary, ignores small + non-binary files", async () => { + const findings = await scanAssetWeight( + { + repoFullName: "o/r", + prNumber: 1, + headSha: HEAD_SHA, + githubToken: "t", + files: [ + { path: "img/logo.png", status: "added" }, + { path: "icon.svg", status: "added" }, + { path: "src/x.ts", status: "added" }, + { path: "tiny.gif", status: "added" }, + ], + }, + async () => + treeReply([ + { path: "img/logo.png", type: "blob", size: 250000 }, + { path: "tiny.gif", type: "blob", size: 2000 }, + ]), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].path, "img/logo.png"); + assert.equal(findings[0].status, "added"); + assert.equal(findings[0].bytes, 250000); + assert.equal(findings[0].deltaBytes, 250000); +}); + +test("scanAssetWeight: evaluates large binaries after the first 50 candidate paths", async () => { + const smallFiles = Array.from({ length: 50 }, (_, i) => ({ + path: `small-${i}.png`, + status: "added", + })); + const files = [...smallFiles, { path: "late-large.png", status: "added" }]; + const findings = await scanAssetWeight( + { + repoFullName: "o/r", + prNumber: 1, + headSha: HEAD_SHA, + githubToken: "t", + files, + }, + async () => + treeReply([ + ...smallFiles.map((file) => ({ + path: file.path, + type: "blob", + size: 2000, + })), + { path: "late-large.png", type: "blob", size: 10_000_000 }, + ]), + ); + assert.deepEqual(findings, [ + { + path: "late-large.png", + bytes: 10_000_000, + deltaBytes: 10_000_000, + status: "added", + }, + ]); +}); + +test("scanAssetWeight: caps findings after ranking by size, not by PR file order", async () => { + const files = Array.from({ length: 51 }, (_, i) => ({ + path: `asset-${i}.png`, + status: "added", + })); + const findings = await scanAssetWeight( + { + repoFullName: "o/r", + prNumber: 1, + headSha: HEAD_SHA, + githubToken: "t", + files, + }, + async () => + treeReply( + files.map((file, i) => ({ + path: file.path, + type: "blob", + size: i === 50 ? 10_000_000 : 150000, + })), + ), + ); + assert.equal(findings.length, 50); + assert.equal(findings[0].path, "asset-50.png"); + assert.equal(findings[0].bytes, 10_000_000); +}); + +test("scanAssetWeight: flags a binary that GREW past the threshold (base vs head)", async () => { + const fetchImpl = async (url) => + String(url).includes(BASE_SHA) + ? treeReply([{ path: "video.mp4", type: "blob", size: 50000 }]) + : treeReply([{ path: "video.mp4", type: "blob", size: 250000 }]); + const findings = await scanAssetWeight( + { + repoFullName: "o/r", + prNumber: 1, + headSha: HEAD_SHA, + baseSha: BASE_SHA, + githubToken: "t", + files: [{ path: "video.mp4", status: "modified" }], + }, + fetchImpl, + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].status, "grown"); + assert.equal(findings[0].deltaBytes, 200000); + assert.equal(findings[0].bytes, 250000); +}); + +test("scanAssetWeight: flags a renamed binary that grew using its previous path", async () => { + const fetchImpl = async (url) => + String(url).includes(BASE_SHA) + ? treeReply([{ path: "old/video.mp4", type: "blob", size: 50000 }]) + : treeReply([{ path: "new/video.mp4", type: "blob", size: 250000 }]); + const findings = await scanAssetWeight( + { + repoFullName: "o/r", + prNumber: 1, + headSha: HEAD_SHA, + baseSha: BASE_SHA, + githubToken: "t", + files: [ + { + path: "new/video.mp4", + status: "renamed", + previousPath: "old/video.mp4", + }, + ], + }, + fetchImpl, + ); + assert.deepEqual(findings, [ + { + path: "new/video.mp4", + bytes: 250000, + deltaBytes: 200000, + status: "grown", + }, + ]); +}); + +test("scanAssetWeight: flags a copied binary as an added heavy path", async () => { + const fetchImpl = async (url) => { + assert.doesNotMatch(String(url), new RegExp(BASE_SHA)); + return treeReply([{ path: "copy/data.bin", type: "blob", size: 10485760 }]); + }; + const findings = await scanAssetWeight( + { + repoFullName: "o/r", + prNumber: 1, + headSha: HEAD_SHA, + baseSha: BASE_SHA, + githubToken: "t", + files: [ + { + path: "copy/data.bin", + status: "copied", + previousPath: "old/data.bin", + }, + ], + }, + fetchImpl, + ); + assert.deepEqual(findings, [ + { + path: "copy/data.bin", + bytes: 10485760, + deltaBytes: 10485760, + status: "added", + }, + ]); +}); + +test("scanAssetWeight: renamed binaries need a previous path before reporting growth", async () => { + const findings = await scanAssetWeight( + { + repoFullName: "o/r", + prNumber: 1, + headSha: HEAD_SHA, + baseSha: BASE_SHA, + githubToken: "t", + files: [{ path: "video.mp4", status: "renamed" }], + }, + async (url) => + String(url).includes(BASE_SHA) + ? treeReply([{ path: "video.mp4", type: "blob", size: 50000 }]) + : treeReply([{ path: "video.mp4", type: "blob", size: 250000 }]), + ); + assert.deepEqual(findings, []); +}); + +test("scanAssetWeight: small growth is not flagged", async () => { + const fetchImpl = async (url) => + String(url).includes(BASE_SHA) + ? treeReply([{ path: "a.png", type: "blob", size: 300000 }]) + : treeReply([{ path: "a.png", type: "blob", size: 310000 }]); + const findings = await scanAssetWeight( + { + repoFullName: "o/r", + prNumber: 1, + headSha: HEAD_SHA, + baseSha: BASE_SHA, + githubToken: "t", + files: [{ path: "a.png", status: "modified" }], + }, + fetchImpl, + ); + assert.deepEqual(findings, []); +}); + // --------------------------------------------------------------------------- // renderBrief: provenance block // --------------------------------------------------------------------------- @@ -1562,6 +1779,204 @@ test("buildBrief: provenance analyzer runs, flags binary file and missing npm at return { ok: false, status: 404, json: async () => ({}) }; return { ok: true, json: async () => ({}) }; }; + try { + const brief = await buildBrief({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { path: "native/tool.exe", status: "added" }, + { path: "package.json", patch: '+ "no-attest": "1.0.0",' }, + ], + }); + assert.equal(brief.analyzerStatus.provenance, "ok"); + assert.ok(brief.findings.provenance.length >= 2); + assert.match(brief.promptSection, /provenance/); + } finally { + globalThis.fetch = realFetch; + } +}); + +test("scanAssetWeight: failed base fetch does not reclassify modified binaries as added", async () => { + const findings = await scanAssetWeight( + { + repoFullName: "o/r", + prNumber: 1, + headSha: HEAD_SHA, + baseSha: BASE_SHA, + githubToken: "t", + files: [ + { path: "video.mp4", status: "modified" }, + { path: "clip.mov", status: "changed" }, + { path: "poster.png", status: "added" }, + ], + }, + async (url) => + String(url).includes(BASE_SHA) + ? { ok: false, json: async () => ({}) } + : treeReply([ + { path: "video.mp4", type: "blob", size: 250000 }, + { path: "clip.mov", type: "blob", size: 260000 }, + { path: "poster.png", type: "blob", size: 270000 }, + ]), + ); + assert.deepEqual(findings, [ + { + path: "poster.png", + bytes: 270000, + deltaBytes: 270000, + status: "added", + }, + ]); +}); + +test("scanAssetWeight: missing baseSha does not reclassify modified binaries as added", async () => { + const findings = await scanAssetWeight( + { + repoFullName: "o/r", + prNumber: 1, + headSha: HEAD_SHA, + githubToken: "t", + files: [{ path: "video.mp4", status: "modified" }], + }, + async () => treeReply([{ path: "video.mp4", type: "blob", size: 250000 }]), + ); + assert.deepEqual(findings, []); +}); + +test("buildBrief: asset-weight falls back to candidate paths for truncated tree responses", async () => { + const realFetch = globalThis.fetch; + const apiVersions: Array = []; + globalThis.fetch = async (url, init) => { + apiVersions.push( + (init?.headers as Record | undefined)?.[ + "X-GitHub-Api-Version" + ], + ); + const href = String(url); + if (href.includes("git/trees")) { + return { ok: true, json: async () => ({ truncated: true, tree: [] }) }; + } + if (href.includes("contents/big.png") && href.includes(HEAD_SHA)) { + return { ok: true, json: async () => ({ type: "file", size: 300000 }) }; + } + if (href.includes("contents/big.png") && href.includes(BASE_SHA)) { + return { ok: true, json: async () => ({ type: "file", size: 50000 }) }; + } + return { ok: true, json: async () => ({}) }; + }; + try { + const brief = await buildBrief({ + repoFullName: "o/r", + prNumber: 1, + headSha: HEAD_SHA, + baseSha: BASE_SHA, + githubToken: "t", + files: [{ path: "big.png", status: "modified" }], + analyzers: ["assetWeight"], + }); + assert.equal(brief.partial, false); + assert.equal(brief.analyzerStatus.assetWeight, "ok"); + assert.equal(brief.findings.assetWeight?.[0]?.status, "grown"); + assert.equal(brief.findings.assetWeight?.[0]?.deltaBytes, 250000); + assert.match(brief.promptSection, /Heavy binary assets/); + assert.ok(apiVersions.every((version) => version === "2022-11-28")); + } finally { + globalThis.fetch = realFetch; + } +}); + +test("scanAssetWeight: fail-safe — no token, no binaries, or failed fetch returns []", async () => { + const tree = async () => + treeReply([{ path: "a.png", type: "blob", size: 999999 }]); + assert.deepEqual( + await scanAssetWeight( + { repoFullName: "o/r", prNumber: 1, headSha: HEAD_SHA, files: [{ path: "a.png", status: "added" }] }, + tree, + ), + [], + ); // no token + assert.deepEqual( + await scanAssetWeight( + { repoFullName: "o/r", prNumber: 1, headSha: HEAD_SHA, githubToken: "t", files: [{ path: "readme.md", status: "added" }] }, + tree, + ), + [], + ); // no binary files + assert.deepEqual( + await scanAssetWeight( + { repoFullName: "o/r", prNumber: 1, headSha: HEAD_SHA, githubToken: "t", files: [{ path: "a.png", status: "added" }] }, + async () => ({ ok: false, json: async () => ({}) }), + ), + [], + ); // tree fetch not OK +}); + +test("scanAssetWeight: rejects path-traversal repoFullName + non-SHA refs (no token-bearing fetch)", async () => { + let fetched = false; + const spy = async () => { + fetched = true; + return treeReply([{ path: "a.png", type: "blob", size: 999999 }]); + }; + const file = { path: "a.png", status: "added" }; + for (const repoFullName of ["a/b/../../x/y", "../evil", "owner/repo/extra", "o/.."]) { + assert.deepEqual( + await scanAssetWeight( + { repoFullName, prNumber: 1, headSha: HEAD_SHA, githubToken: "t", files: [file] }, + spy, + ), + [], + ); + } + assert.deepEqual( + await scanAssetWeight( + { repoFullName: "o/r", prNumber: 1, headSha: "main", githubToken: "t", files: [file] }, + spy, + ), + [], + ); + assert.equal(fetched, false, "the token-bearing fetch never runs for unsafe input"); +}); + +test("renderBrief: renders the asset-weight block with human-readable sizes", () => { + const r = renderBrief({ + assetWeight: [ + { path: "img/logo.png", bytes: 2500000, deltaBytes: 2500000, status: "added" }, + { path: "v.mp4", bytes: 300000, deltaBytes: 200000, status: "grown" }, + ], + }); + assert.match(r.promptSection, /Heavy binary assets/); + assert.match(r.promptSection, /`img\/logo\.png` adds 2\.4 MiB/); + assert.match(r.promptSection, /`v\.mp4` grows \+195 KiB to 293 KiB/); +}); + +test("buildBrief: asset-weight analyzer reports grown binaries from request file status", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async (url) => + String(url).includes("git/trees") + ? String(url).includes(BASE_SHA) + ? treeReply([{ path: "big.png", type: "blob", size: 50000 }]) + : treeReply([{ path: "big.png", type: "blob", size: 300000 }]) + : { ok: true, json: async () => ({}) }; + try { + const brief = await buildBrief({ + repoFullName: "o/r", + prNumber: 1, + headSha: HEAD_SHA, + baseSha: BASE_SHA, + githubToken: "t", + files: [{ path: "big.png", status: "modified" }], + }); + assert.equal(brief.analyzerStatus.assetWeight, "ok"); + assert.equal(brief.findings.assetWeight.length, 1); + assert.equal(brief.findings.assetWeight[0].status, "grown"); + assert.equal(brief.findings.assetWeight[0].deltaBytes, 250000); + assert.match(brief.promptSection, /Heavy binary assets/); + assert.match(brief.promptSection, /grows/); + } finally { + globalThis.fetch = realFetch; + } +}); + test("codeOnly: blanks string messages, keeps ${...} interpolation bodies", () => { assert.equal(codeOnly('"a secret here"'), " "); assert.equal(codeOnly("'plain'"), " "); @@ -1717,21 +2132,22 @@ test("buildBrief: secret-log analyzer runs (pure, no network)", async () => { repoFullName: "o/r", prNumber: 1, files: [ - { path: "native/tool.exe", status: "added" }, - { path: "package.json", patch: '+ "no-attest": "1.0.0",' }, + { + path: "src/a.ts", + patch: "@@ -1,0 +1,1 @@\n+console.log(req.headers.authorization);", + }, ], }); - assert.equal(brief.analyzerStatus.provenance, "ok"); - assert.ok(brief.findings.provenance.length >= 2); - assert.match(brief.promptSection, /provenance/); + assert.equal(brief.analyzerStatus.secretLog, "ok"); + assert.equal(brief.findings.secretLog.length, 1); + assert.match(brief.promptSection, /Secrets \/ PII reaching a log/); } finally { globalThis.fetch = realFetch; } }); -test("buildBrief: provenance analyzer throw → degraded + partial", async () => { +test("buildBrief: provenance analyzer fetch failure fails safe", async () => { const realFetch = globalThis.fetch; - // Cause all fetches to fail (provenance uses fetch for attestation checks) globalThis.fetch = async () => { throw new Error("network down"); }; try { const brief = await buildBrief({ @@ -1740,20 +2156,9 @@ test("buildBrief: provenance analyzer throw → degraded + partial", async () => analyzers: ["provenance"], files: [{ path: "package.json", patch: '+ "pkg": "1.0.0",' }], }); - // provenance fetch throws → degraded; binary scan still ran but that's pure - // The analyzer as a whole may succeed (binary scan is pure) or degrade on fetch. - // Because hasNpmAttestation catches fetch errors (fail-safe), the analyzer succeeds. assert.equal(brief.analyzerStatus.provenance, "ok"); assert.equal(brief.partial, false); - { - path: "src/a.ts", - patch: "@@ -1,0 +1,1 @@\n+console.log(req.headers.authorization);", - }, - ], - }); - assert.equal(brief.analyzerStatus.secretLog, "ok"); - assert.equal(brief.findings.secretLog.length, 1); - assert.match(brief.promptSection, /Secrets \/ PII reaching a log/); + assert.deepEqual(brief.findings.provenance, []); } finally { globalThis.fetch = realFetch; } diff --git a/src/review/enrichment-wire.ts b/src/review/enrichment-wire.ts index 4ec00e5624..3a976dab2f 100644 --- a/src/review/enrichment-wire.ts +++ b/src/review/enrichment-wire.ts @@ -83,6 +83,8 @@ export async function buildReviewEnrichment( title: input.title, files: input.files.map((file) => ({ path: file.path, + status: file.status ?? undefined, + previousPath: file.previousFilename ?? undefined, patch: typeof file.payload?.patch === "string" ? file.payload.patch diff --git a/test/unit/enrichment-wire.test.ts b/test/unit/enrichment-wire.test.ts index 65e7680428..db6cd8d511 100644 --- a/test/unit/enrichment-wire.test.ts +++ b/test/unit/enrichment-wire.test.ts @@ -11,7 +11,13 @@ const input = { headSha: "abc", title: "t", files: [ - { path: "a.ts", payload: { patch: "@@ +1 @@" } }, + { path: "a.ts", status: "modified", payload: { patch: "@@ +1 @@" } }, + { + path: "renamed.png", + status: "renamed", + previousFilename: "old.png", + payload: { patch: "@@ +2 @@" }, + }, { path: "b.ts" }, ] as never, diff: "the diff", @@ -81,8 +87,14 @@ describe("buildReviewEnrichment", () => { const body = JSON.parse(calls[0]!.init.body as string); expect(body.repoFullName).toBe("o/r"); expect(body.files).toEqual([ - { path: "a.ts", patch: "@@ +1 @@" }, - { path: "b.ts", patch: undefined }, + { path: "a.ts", status: "modified", previousPath: undefined, patch: "@@ +1 @@" }, + { + path: "renamed.png", + status: "renamed", + previousPath: "old.png", + patch: "@@ +2 @@", + }, + { path: "b.ts", status: undefined, patch: undefined }, ]); });