diff --git a/review-enrichment/src/analyzers/dependency-scan.ts b/review-enrichment/src/analyzers/dependency-scan.ts index 7188a12fd6..f4dc1777e2 100644 --- a/review-enrichment/src/analyzers/dependency-scan.ts +++ b/review-enrichment/src/analyzers/dependency-scan.ts @@ -9,6 +9,7 @@ import type { } from "../types.js"; import type { AnalysisContext } from "../analysis-context.js"; import { boundedFetchJson } from "../external-fetch.js"; +import { isDiffFileHeaderLine } from "./diff-lines.js"; export interface DepChange { ecosystem: string; @@ -107,11 +108,7 @@ export function extractDependencyChanges( if (manifestFiles > maxManifestFiles) break; for (const line of file.patch.split("\n", maxPatchLinesPerFile)) { const sign = line[0]; - if ( - (sign !== "+" && sign !== "-") || - line.startsWith("+++ ") || - line.startsWith("---") - ) + if ((sign !== "+" && sign !== "-") || isDiffFileHeaderLine(line)) continue; const parsed = parseLine(manifest, line.slice(1).trim()); if (!parsed) continue; diff --git a/review-enrichment/src/analyzers/lockfile-drift.ts b/review-enrichment/src/analyzers/lockfile-drift.ts index f9e492b12c..d50cfc1a9b 100644 --- a/review-enrichment/src/analyzers/lockfile-drift.ts +++ b/review-enrichment/src/analyzers/lockfile-drift.ts @@ -10,6 +10,7 @@ import type { import type { AnalysisContext } from "../analysis-context.js"; import { extractDependencyChanges } from "./dependency-scan.js"; import { boundedFetchJson } from "../external-fetch.js"; +import { isDiffFileHeaderLine } from "./diff-lines.js"; import { isParseableLockfile, lockfileBasename } from "../lockfile-path.js"; interface LockfileChange { @@ -126,7 +127,7 @@ function* patchLines( for (const raw of patch.split("\n")) { seen += 1; if (seen > maxLines) break; - if (raw.startsWith("+++ ") || raw.startsWith("---")) continue; + if (isDiffFileHeaderLine(raw)) continue; const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw); if (hunk) { newLine = Number(hunk[1]); diff --git a/review-enrichment/test/dependency-scan.test.ts b/review-enrichment/test/dependency-scan.test.ts new file mode 100644 index 0000000000..3240ec7d98 --- /dev/null +++ b/review-enrichment/test/dependency-scan.test.ts @@ -0,0 +1,28 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { extractDependencyChanges } from "../dist/analyzers/dependency-scan.js"; + +test("extractDependencyChanges skips real file headers via the shared discriminator, not spurious deps", () => { + // The patch carries the unified-diff file headers (`--- a/…`, `+++ b/…`) ahead of the hunk. They must be + // skipped as headers — never parsed as dependency lines — while the real version bump is extracted. This + // pins the behavior after swapping the anchored `startsWith("+++ ")`/`startsWith("---")` guard for the + // shared isDiffFileHeaderLine helper (which only matches `+++ a/`/`b/`/`/dev/null` headers). + const changes = extractDependencyChanges([ + { + path: "package.json", + patch: [ + "--- a/package.json", + "+++ b/package.json", + "@@ -5,3 +5,3 @@", + ' "dependencies": {', + '- "lodash": "4.17.20",', + '+ "lodash": "4.17.21",', + ].join("\n"), + }, + ]); + + assert.deepEqual(changes, [ + { ecosystem: "npm", package: "lodash", from: "4.17.20", to: "4.17.21" }, + ]); +}); diff --git a/review-enrichment/test/lockfile-drift.test.ts b/review-enrichment/test/lockfile-drift.test.ts index 81ec5cd69d..99ac713dce 100644 --- a/review-enrichment/test/lockfile-drift.test.ts +++ b/review-enrichment/test/lockfile-drift.test.ts @@ -27,6 +27,35 @@ test("extractLockfileChanges matches lockfile basenames case-insensitively", () ]); }); +test("extractLockfileChanges keeps new-file line numbers correct across ++-content added lines", () => { + // An added line whose CONTENT begins with `++ ` renders in the diff as `+++ …`. The old anchored + // `startsWith("+++ ")` guard mistook it for a `+++ b/file` header and `continue`d WITHOUT advancing the + // new-file line counter, so every finding AFTER it was reported one line too low. The shared + // isDiffFileHeaderLine helper only skips real `+++ a/`/`b/`/`/dev/null` headers, so the counter stays true. + const changes = extractLockfileChanges([ + { + path: "package-lock.json", + patch: [ + "@@ -9,0 +10,3 @@", + '+ "node_modules/lodash": {', // new-file line 10 + "+++ not a header — added content whose text begins with ++", // new-file line 11 (must be counted) + '+ "version": "4.17.21"', // new-file line 12 + ].join("\n"), + }, + ]); + + assert.deepEqual(changes, [ + { + file: "package-lock.json", + line: 12, // 12, not 11 — the intervening ++-content line is counted, not swallowed as a header + ecosystem: "npm", + package: "lodash", + from: null, + to: "4.17.21", + }, + ]); +}); + test("extractLockfileChanges does not let unparsed lockfiles consume the scan budget", () => { const yarnPatch = [ "@@ -1,0 +1,2 @@",