diff --git a/review-enrichment/src/analyzers/heavy-dependency.ts b/review-enrichment/src/analyzers/heavy-dependency.ts new file mode 100644 index 0000000000..e43eab45b6 --- /dev/null +++ b/review-enrichment/src/analyzers/heavy-dependency.ts @@ -0,0 +1,189 @@ +// Heavy-dependency-for-trivial-use analyzer (#1505). For each newly-added/upgraded npm dependency, count direct +// import/require usage in the PR's added lines and fetch package weight metadata. Flag only when the package is +// both materially heavy and used trivially, so the review brief can ask whether a local helper/native API would do. +import type { EnrichRequest, HeavyDependencyFinding } from "../types.js"; +import { extractDependencyChanges } from "./dependency-scan.js"; + +const MAX_WEIGHT_LOOKUPS = 20; +const MAX_FINDINGS = 15; +const TRIVIAL_USAGE_MAX = 2; +const MIN_INSTALL_BYTES = 500_000; +const MIN_BUNDLE_BYTES = 80_000; +const MIN_GZIP_BYTES = 25_000; + +const NPM_PACKAGE_RE = + /^(?:@[a-z0-9][a-z0-9._-]*\/[a-z0-9][a-z0-9._-]*|[a-z0-9][a-z0-9._-]*)$/; +const SEMVER_RE = + /^(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/; + +interface AddedLine { + file: string; + line: number; + text: string; +} + +export interface PackageWeight { + installSizeBytes: number | null; + bundleSizeBytes: number | null; + gzipSizeBytes: number | null; + dependencyCount: number | null; +} + +interface ScanOptions { + signal?: AbortSignal; +} + +function isSafeNpmPackageVersion(name: string, version: string): boolean { + return NPM_PACKAGE_RE.test(name) && SEMVER_RE.test(version); +} + +function addedPatchLines( + files: NonNullable, +): AddedLine[] { + const lines: AddedLine[] = []; + for (const file of files) { + if (!file.patch) continue; + let nextLine = 0; + for (const raw of file.patch.split("\n")) { + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw); + if (hunk) { + nextLine = Number(hunk[1]); + continue; + } + if (raw.startsWith("\\ No newline")) continue; + if (raw.startsWith("+") && !raw.startsWith("+++")) { + lines.push({ + file: file.path, + line: nextLine || 1, + text: raw.slice(1), + }); + nextLine += 1; + continue; + } + if (raw.startsWith("-") && !raw.startsWith("---")) continue; + if (nextLine) nextLine += 1; + } + } + return lines; +} + +function moduleSpecifiers(text: string): string[] { + const specs: string[] = []; + const callOrFrom = + /(?:from\s*|require\s*\(\s*|import\s*\(\s*)["']([^"']+)["']/g; + for (const match of text.matchAll(callOrFrom)) { + if (match[1]) specs.push(match[1]); + } + const sideEffect = /^\s*import\s+["']([^"']+)["']/.exec(text); + if (sideEffect?.[1]) specs.push(sideEffect[1]); + return specs; +} + +function specifierMatchesPackage(specifier: string, pkg: string): boolean { + return specifier === pkg || specifier.startsWith(`${pkg}/`); +} + +export function countPackagePatchUsages( + files: NonNullable, + pkg: string, +): Pick { + const locations: HeavyDependencyFinding["usageLocations"] = []; + for (const line of addedPatchLines(files)) { + const matches = moduleSpecifiers(line.text).filter((specifier) => + specifierMatchesPackage(specifier, pkg), + ); + for (let i = 0; i < matches.length; i += 1) { + locations.push({ file: line.file, line: line.line }); + } + } + return { + usageCount: locations.length, + usageLocations: locations.slice(0, TRIVIAL_USAGE_MAX), + }; +} + +function numberOrNull(value: unknown): number | null { + return typeof value === "number" && Number.isFinite(value) ? value : null; +} + +export async function queryPackageWeight( + pkg: string, + version: string, + fetchImpl: typeof fetch = fetch, + signal?: AbortSignal, +): Promise { + if (signal?.aborted) return null; + try { + const packageSpec = encodeURIComponent(`${pkg}@${version}`); + const response = await fetchImpl( + `https://bundlephobia.com/api/size?package=${packageSpec}`, + { signal }, + ); + if (!response.ok) return null; + const data = (await response.json()) as { + installSize?: unknown; + size?: unknown; + gzip?: unknown; + dependencyCount?: unknown; + }; + return { + installSizeBytes: numberOrNull(data.installSize), + bundleSizeBytes: numberOrNull(data.size), + gzipSizeBytes: numberOrNull(data.gzip), + dependencyCount: numberOrNull(data.dependencyCount), + }; + } catch { + return null; + } +} + +export function isHeavyPackageWeight(weight: PackageWeight): boolean { + return ( + (weight.installSizeBytes ?? 0) >= MIN_INSTALL_BYTES || + (weight.bundleSizeBytes ?? 0) >= MIN_BUNDLE_BYTES || + (weight.gzipSizeBytes ?? 0) >= MIN_GZIP_BYTES + ); +} + +export async function scanHeavyDependencies( + req: EnrichRequest, + fetchImpl: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + const findings: HeavyDependencyFinding[] = []; + const changes = extractDependencyChanges(req.files ?? []).filter( + (change) => change.ecosystem === "npm", + ); + let weightLookups = 0; + + for (const change of changes) { + if (options.signal?.aborted || findings.length >= MAX_FINDINGS) break; + if (!isSafeNpmPackageVersion(change.package, change.to)) continue; + + const usage = countPackagePatchUsages(req.files ?? [], change.package); + if (usage.usageCount < 1 || usage.usageCount > TRIVIAL_USAGE_MAX) continue; + if (weightLookups >= MAX_WEIGHT_LOOKUPS) break; + weightLookups += 1; + + const weight = await queryPackageWeight( + change.package, + change.to, + fetchImpl, + options.signal, + ); + if (!weight || !isHeavyPackageWeight(weight)) continue; + + findings.push({ + ecosystem: "npm", + package: change.package, + version: change.to, + from: change.from, + direction: change.from ? "change" : "add", + usageCount: usage.usageCount, + usageLocations: usage.usageLocations, + ...weight, + }); + } + + return findings; +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index d1dcfc8227..3870ceeea7 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -12,6 +12,7 @@ import { scanLockfileDrift } from "./analyzers/lockfile-drift.js"; import { scanSecrets } from "./analyzers/secret-scan.js"; import { scanLicenses } from "./analyzers/license-check.js"; import { scanInstallScripts } from "./analyzers/install-scripts.js"; +import { scanHeavyDependencies } from "./analyzers/heavy-dependency.js"; import { scanActionPins } from "./analyzers/actions-pin.js"; import { scanEol } from "./analyzers/eol-check.js"; import { scanRedos } from "./analyzers/redos.js"; @@ -33,6 +34,8 @@ const ANALYZERS: Record = { secret: (req) => scanSecrets(req), license: (req) => scanLicenses(req), installScript: (req) => scanInstallScripts(req), + heavyDependency: (req, signal) => + scanHeavyDependencies(req, fetch, { signal }), actionPin: (req) => scanActionPins(req), eol: (req) => scanEol(req), redos: (req) => scanRedos(req), diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index e9fc157841..12625c3eb0 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -40,6 +40,13 @@ function formatBytes(n: number): string { return `${n} B`; } +function bytesLabel(value: number | null): string { + if (value === null) return "unknown"; + if (value >= 1_000_000) return `${(value / 1_000_000).toFixed(1)} MB`; + if (value >= 1_000) return `${Math.round(value / 1_000)} KB`; + return `${value} B`; +} + /** Build the `promptSection` (verbatim splice) + a one-line `systemSuffix` from the findings. Empty when nothing found. */ export function renderBrief( findings: BriefFindings, @@ -125,6 +132,26 @@ export function renderBrief( } } + const heavyDependencies = findings.heavyDependency ?? []; + if (heavyDependencies.length) { + lines.push( + "### Heavy dependencies used trivially (consider native code or a small helper)", + ); + for (const dep of heavyDependencies) { + const locations = dep.usageLocations + .map((location) => safeCodeSpan(`${location.file}:${location.line}`)) + .join(", "); + const dependencyCount = + dep.dependencyCount === null + ? "unknown deps" + : `${dep.dependencyCount} deps`; + const sizes = `install ${bytesLabel(dep.installSizeBytes)}, bundle ${bytesLabel(dep.bundleSizeBytes)}, gzip ${bytesLabel(dep.gzipSizeBytes)}`; + lines.push( + `- ${safeCodeSpan(`${dep.package}@${dep.version}`)} (${dep.ecosystem}): used ${dep.usageCount} time${dep.usageCount === 1 ? "" : "s"} at ${locations}; ${sizes}, ${dependencyCount}`, + ); + } + } + const actionPins = findings.actionPin ?? []; if (actionPins.length) { lines.push("### Unpinned GitHub Actions (pin to a commit SHA)"); diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 3e7c0abc5c..ad1ff17804 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -81,6 +81,22 @@ export interface InstallScriptFinding { publishedAt: string | null; } +/** A newly-added/upgraded npm package that is materially heavy but only directly imported/required a few times + * in the changed lines. Size values are package-service bytes and are nullable when that service omits one. */ +export interface HeavyDependencyFinding { + ecosystem: "npm"; + package: string; + version: string; + from: string | null; + direction: "add" | "change"; + usageCount: number; + usageLocations: Array<{ file: string; line: number }>; + installSizeBytes: number | null; + bundleSizeBytes: number | null; + gzipSizeBytes: number | null; + dependencyCount: number | null; +} + /** A third-party GitHub Action referenced by a mutable tag/branch instead of a pinned commit SHA. */ export interface ActionPinFinding { file: string; @@ -170,6 +186,7 @@ export interface BriefFindings { license?: LicenseFinding[]; actionPin?: ActionPinFinding[]; installScript?: InstallScriptFinding[]; + heavyDependency?: HeavyDependencyFinding[]; eol?: EolFinding[]; redos?: RedosFinding[]; provenance?: ProvenanceFinding[]; diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index 7b00ebca58..c5481d618b 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -15,6 +15,12 @@ import { buildBrief } from "../dist/brief.js"; import { scanPatch, scanSecrets } from "../dist/analyzers/secret-scan.js"; import { scanLicenses } from "../dist/analyzers/license-check.js"; import { scanInstallScripts } from "../dist/analyzers/install-scripts.js"; +import { + countPackagePatchUsages, + isHeavyPackageWeight, + queryPackageWeight, + scanHeavyDependencies, +} from "../dist/analyzers/heavy-dependency.js"; import { scanWorkflowPins, scanActionPins, @@ -945,6 +951,255 @@ test("buildBrief: install-script analyzer runs alongside the others", async () = } }); +test("countPackagePatchUsages: line-cites import, require, dynamic import, and subpath usage", () => { + const usage = countPackagePatchUsages( + [ + { + path: "src/app.ts", + patch: [ + "@@ -10,0 +10,4 @@", + '+import get from "lodash/get";', + '+const fp = require("lodash/fp");', + '+const x = await import("lodash");', + '+import other from "left-pad";', + ].join("\n"), + }, + ], + "lodash", + ); + + assert.equal(usage.usageCount, 3); + assert.deepEqual(usage.usageLocations, [ + { file: "src/app.ts", line: 10 }, + { file: "src/app.ts", line: 11 }, + ]); +}); + +test("queryPackageWeight: maps bundlephobia size fields and degrades on non-ok", async () => { + const weight = await queryPackageWeight("lodash", "4.17.21", async () => ({ + ok: true, + json: async () => ({ + installSize: 1_400_000, + size: 72_000, + gzip: 25_500, + dependencyCount: 1, + }), + })); + + assert.deepEqual(weight, { + installSizeBytes: 1_400_000, + bundleSizeBytes: 72_000, + gzipSizeBytes: 25_500, + dependencyCount: 1, + }); + assert.equal( + await queryPackageWeight("x", "1.0.0", async () => ({ + ok: false, + json: async () => ({}), + })), + null, + ); +}); + +test("isHeavyPackageWeight: flags install, bundle, or gzip threshold hits", () => { + assert.equal( + isHeavyPackageWeight({ + installSizeBytes: 500_000, + bundleSizeBytes: null, + gzipSizeBytes: null, + dependencyCount: null, + }), + true, + ); + assert.equal( + isHeavyPackageWeight({ + installSizeBytes: null, + bundleSizeBytes: 80_000, + gzipSizeBytes: null, + dependencyCount: null, + }), + true, + ); + assert.equal( + isHeavyPackageWeight({ + installSizeBytes: null, + bundleSizeBytes: null, + gzipSizeBytes: 25_000, + dependencyCount: null, + }), + true, + ); + assert.equal( + isHeavyPackageWeight({ + installSizeBytes: 100_000, + bundleSizeBytes: 10_000, + gzipSizeBytes: 2_000, + dependencyCount: 0, + }), + false, + ); +}); + +test("scanHeavyDependencies: flags heavy npm deps used trivially and skips non-trivial usage", async () => { + const controller = new AbortController(); + const findings = await scanHeavyDependencies( + { + repoFullName: "o/r", + prNumber: 1, + files: [ + { + path: "package.json", + patch: [ + '+ "lodash": "4.17.21",', + '+ "tiny": "1.0.0",', + '+ "many": "2.0.0",', + ].join("\n"), + }, + { + path: "src/app.ts", + patch: [ + "@@ -1,0 +1,6 @@", + '+import get from "lodash/get";', + '+import tiny from "tiny";', + '+import one from "many/one";', + '+import two from "many/two";', + '+import three from "many/three";', + ].join("\n"), + }, + ], + }, + async (url, init) => { + assert.ok(init?.signal instanceof AbortSignal); + const u = String(url); + if (u.includes("tiny")) + return { ok: true, json: async () => ({ installSize: 10_000 }) }; + return { + ok: true, + json: async () => ({ + installSize: 1_400_000, + size: 72_000, + gzip: 25_500, + dependencyCount: 1, + }), + }; + }, + { signal: controller.signal }, + ); + + assert.equal(findings.length, 1); + assert.equal(findings[0].package, "lodash"); + assert.equal(findings[0].usageCount, 1); + assert.deepEqual(findings[0].usageLocations, [ + { file: "src/app.ts", line: 1 }, + ]); +}); + +test("scanHeavyDependencies: lookup budget ignores unused dependency changes", async () => { + const unusedDeps = Array.from( + { length: 20 }, + (_, i) => `+ "unused-${i}": "1.0.0",`, + ); + let lookups = 0; + const findings = await scanHeavyDependencies( + { + repoFullName: "o/r", + prNumber: 1, + files: [ + { + path: "package.json", + patch: [...unusedDeps, '+ "late-heavy": "1.0.0",'].join("\n"), + }, + { + path: "src/app.ts", + patch: '@@ -1,0 +1,1 @@\n+import heavy from "late-heavy";', + }, + ], + }, + async (url) => { + lookups += 1; + assert.match(String(url), /late-heavy%401\.0\.0/); + return { + ok: true, + json: async () => ({ + installSize: 1_400_000, + size: 90_000, + gzip: 30_000, + dependencyCount: 3, + }), + }; + }, + ); + + assert.equal(lookups, 1); + assert.equal(findings.length, 1); + assert.equal(findings[0].package, "late-heavy"); +}); + +test("renderBrief: renders the heavy-dependency block with size evidence", () => { + const r = renderBrief({ + heavyDependency: [ + { + ecosystem: "npm", + package: "lodash", + version: "4.17.21", + from: null, + direction: "add", + usageCount: 1, + usageLocations: [{ file: "src/app.ts", line: 1 }], + installSizeBytes: 1_400_000, + bundleSizeBytes: 72_000, + gzipSizeBytes: 25_500, + dependencyCount: 1, + }, + ], + }); + + assert.match(r.promptSection, /Heavy dependencies used trivially/); + assert.match(r.promptSection, /`lodash@4\.17\.21` \(npm\)/); + assert.match(r.promptSection, /`src\/app\.ts:1`/); + assert.match(r.promptSection, /install 1\.4 MB, bundle 72 KB, gzip 26 KB/); +}); + +test("buildBrief: heavy-dependency analyzer runs alongside the others", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async (url) => { + const u = String(url); + if (u.includes("bundlephobia")) + return { + ok: true, + json: async () => ({ + installSize: 1_400_000, + size: 72_000, + gzip: 25_500, + dependencyCount: 1, + }), + }; + if (u.includes("deps.dev")) + return { ok: true, json: async () => ({ licenses: ["MIT"] }) }; + if (u.includes("attestations")) + return { ok: true, json: async () => ({ attestations: [{}] }) }; + return { ok: true, json: async () => ({ vulns: [], versions: {} }) }; + }; + try { + const brief = await buildBrief({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { path: "package.json", patch: '+ "lodash": "4.17.21",' }, + { + path: "src/app.ts", + patch: '@@ -1,0 +1,1 @@\n+import get from "lodash/get";', + }, + ], + }); + assert.equal(brief.analyzerStatus.heavyDependency, "ok"); + assert.equal(brief.findings.heavyDependency.length, 1); + assert.match(brief.promptSection, /Heavy dependencies used trivially/); + } finally { + globalThis.fetch = realFetch; + } +}); + test("scanWorkflowPins: flags unpinned third-party actions, skips official + SHA-pinned + local, line-cited", () => { const patch = [ "@@ -1,1 +1,5 @@", @@ -2186,14 +2441,16 @@ test("buildBrief: provenance analyzer runs, flags binary file and missing npm at const brief = await buildBrief({ repoFullName: "o/r", prNumber: 1, + analyzers: ["provenance"], 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.equal(brief.findings.provenance.length, 2); assert.match(brief.promptSection, /provenance/); + assert.match(brief.promptSection, /Binary files committed/); } finally { globalThis.fetch = realFetch; }