fix(review): correlate cross-hunk lockfile tamper signals by package (#8351)

src/review/lockfile-tamper.ts

@@ -48,6 +48,15 @@ function npmPackageFromNodeModulesPath(path: string): string | null {
   return rest.split("/")[0] || null;
 }
 
+/** Recover a bare package name from an npm-registry `resolved` URL (e.g. context lines in a hunk whose
+ *  `"node_modules/<pkg>": {` header fell outside git's 3-line window). Used to key unattributed fallback
+ *  buckets so version + resolved/integrity signals for the SAME package can merge across hunk boundaries
+ *  (#8351) instead of each depth-0 `}` minting a fresh `#unattributed-N` sequence key. */
+function packageNameFromResolvedUrl(url: string): string | null {
+  const match = /^https:\/\/registry\.npmjs\.org\/((?:@[^/]+\/)?[^/]+)\//i.exec(url);
+  return match?.[1] ?? null;
+}
+
 /** True when `path`'s basename is `package-lock.json` — the only lockfile format this check parses today
  *  (npm/lockfileVersion 2-3 JSON shape). Matches ANY directory depth (root, `review-enrichment/`,
  *  `apps/loopover-ui/`, or a future workspace) rather than a hardcoded path list, so a new workspace package
@@ -130,8 +139,15 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid
   // a change was silently dropped: `currentEntryKey` stayed null for the whole hunk, so the
   // `!currentEntryKey ... continue` guard below skipped it -- a tampered field could evade detection
   // entirely just by having enough unchanged sibling fields ahead of it in its entry.
+  //
+  // When a package name can be recovered from a nearby `resolved` URL (including unchanged context
+  // lines), the fallback key is `${path}#unattributed:<pkg>` so two hunks that each close with a
+  // depth-0 `}` (which clears `activeUnknownKey`) still merge into ONE candidate for that package
+  // (#8351). Without a recoverable name we keep the per-block `#unattributed-N` sequence — two
+  // genuinely different nameless blocks must never collapse into each other.
   let activeUnknownKey: string | null = null;
   let unknownEntrySeq = 0;
+  let inferredUnattributedPackage: string | null = null;
 
   const entryFor = (entryKey: string, packageName: string): MutableCandidate => {
     const existing = byEntry.get(entryKey);
@@ -149,6 +165,37 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid
     return created;
   };
 
+  const clearBlockTracking = () => {
+    activeEntry = null;
+    insideRejectedBlock = false;
+    activeUnknownKey = null;
+    inferredUnattributedPackage = null;
+  };
+
+  /** When a sequence-keyed unattributed bucket later learns a package name, re-key (and merge into any
+   *  pre-existing package-keyed candidate) so cross-hunk / late-resolved correlation works (#8351). */
+  const promoteSequenceKeyToPackage = (packageName: string) => {
+    if (!activeUnknownKey?.includes("#unattributed-")) return;
+    const seqKey = activeUnknownKey;
+    const pkgKey = `${path}#unattributed:${packageName}`;
+    activeUnknownKey = pkgKey;
+    const seqEntry = byEntry.get(seqKey);
+    // Freshly minted sequence keys promote before their first field is recorded — no map entry yet.
+    if (!seqEntry) return;
+    const prior = byEntry.get(pkgKey);
+    if (!prior) {
+      byEntry.delete(seqKey);
+      byEntry.set(pkgKey, seqEntry);
+      return;
+    }
+    if (seqEntry.removedVersion !== undefined) prior.removedVersion = seqEntry.removedVersion;
+    if (seqEntry.addedVersion !== undefined) prior.addedVersion = seqEntry.addedVersion;
+    prior.versionChanged = versionChanged(prior.removedVersion, prior.addedVersion);
+    prior.resolvedOrIntegrityChanged = prior.resolvedOrIntegrityChanged || seqEntry.resolvedOrIntegrityChanged;
+    prior.offRegistryResolvedUrl = prior.offRegistryResolvedUrl ?? seqEntry.offRegistryResolvedUrl;
+    byEntry.delete(seqKey);
+  };
+
   for (const line of patchLines(patch)) {
     const body = line.content.trim();
     const objectHeader = /^"([^"]+)"\s*:\s*\{/.exec(body);
@@ -161,43 +208,63 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid
         sawPackagesEntry = true;
         insideRejectedBlock = false;
         activeUnknownKey = null;
+        inferredUnattributedPackage = null;
       } else if (activeEntry) {
         innerObjectDepth++;
       } else if (!sawPackagesEntry && !CONTAINER_KEYS.has(key)) {
         activeEntry = { entryKey: key, packageName: key };
         innerObjectDepth = 0;
         insideRejectedBlock = false;
         activeUnknownKey = null;
+        inferredUnattributedPackage = null;
       } else {
         activeEntry = null;
         innerObjectDepth = 0;
         insideRejectedBlock = true;
         activeUnknownKey = null;
+        inferredUnattributedPackage = null;
       }
       continue;
     }
     if (body === "}" || body.startsWith("},")) {
       if (innerObjectDepth > 0) {
         innerObjectDepth--;
       } else {
-        activeEntry = null;
-        insideRejectedBlock = false;
-        activeUnknownKey = null;
+        clearBlockTracking();
       }
     }
 
     const resolvedMatch = /^"resolved"\s*:\s*"([^"]*)"/.exec(body);
     const integrityMatch = /^"integrity"\s*:\s*"([^"]*)"/.exec(body);
     const versionMatch = /^"version"\s*:\s*"([^"]*)"/.exec(body);
 
+    // Learn package identity from ANY resolved URL in this block (context OR changed) before deciding
+    // the unattributed bucket key — a version-only hunk's leading context often still carries the
+    // registry URL even when the entry header itself is out of window (#8351 / #7778).
+    if (!activeEntry && !insideRejectedBlock && resolvedMatch?.[1]) {
+      const fromResolved = packageNameFromResolvedUrl(resolvedMatch[1]);
+      if (fromResolved) {
+        inferredUnattributedPackage = fromResolved;
+        // Context lines never reach the allocation branch below; still promote a live sequence key.
+        promoteSequenceKeyToPackage(fromResolved);
+      }
+    }
+
     let currentEntryKey = activeEntry?.entryKey ?? null;
     let currentPackageName = activeEntry?.packageName ?? null;
     if (!currentEntryKey && !insideRejectedBlock && line.sign !== " " && (resolvedMatch || integrityMatch || versionMatch)) {
+      // Always mint a sequence key first, then promote to `#unattributed:<pkg>` when a name is known
+      // (covers the empty-bucket promote path on first allocation, and late re-key once fields exist).
       if (!activeUnknownKey) {
         unknownEntrySeq += 1;
         activeUnknownKey = `${path}#unattributed-${unknownEntrySeq}`;
       }
+      if (inferredUnattributedPackage) {
+        promoteSequenceKeyToPackage(inferredUnattributedPackage);
+      }
       currentEntryKey = activeUnknownKey;
+      // Keep the anonymous display label even when the Map key is package-qualified — existing #7778
+      // assertions (and the public finding text for header-less hunks) key off this exact string.
       currentPackageName = "(unattributed lockfile entry)";
     }
     if (!currentEntryKey || !currentPackageName || line.sign === " ") continue;

test/unit/lockfile-tamper.test.ts

@@ -524,4 +524,175 @@ describe("lockfileTamperRiskFinding", () => {
     expect(finding).not.toBeNull();
     expect(finding?.detail).toContain("lodash");
   });
+
+  // #8351: git's ~3-line context often splits a single entry's version bump and its resolved/integrity
+  // change into SEPARATE hunks, each closing with a depth-0 `}`. Resetting the unattributed bucket on
+  // every such brace used to mint a fresh `#unattributed-N` per hunk, so the integrity-only bucket
+  // false-flagged even though the version hunk recorded a real bump for the same package.
+  it("REGRESSION (#8351): does NOT flag a legitimate version+integrity bump split across two header-less hunks for the same package", () => {
+    const lockPatch = [
+      "@@ -50,6 +50,6 @@",
+      '       "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",',
+      '       "license": "MIT",',
+      '-      "version": "4.17.20",',
+      '+      "version": "4.17.21",',
+      "     }",
+      "@@ -60,7 +60,7 @@",
+      '       "version": "4.17.21",',
+      '       "license": "MIT",',
+      '-      "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",',
+      '-      "integrity": "sha512-oldoldold=="',
+      '+      "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz",',
+      '+      "integrity": "sha512-newnewnew=="',
+      "     }",
+    ].join("\n");
+    // No `"node_modules/lodash": {` header in either hunk — package identity comes from the registry
+    // resolved URL in context / changed lines, so both hunks share one unattributed bucket.
+    expect(lockfileTamperRiskFinding([lockfilePatch(lockPatch)])).toBeNull();
+  });
+
+  it("REGRESSION (#8351): still flags a genuine cross-hunk tamper (integrity changed, version NOT changed) for the same header-less package", () => {
+    const lockPatch = [
+      "@@ -50,6 +50,6 @@",
+      '       "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",',
+      '       "license": "MIT",',
+      '-      "integrity": "sha512-oldoldold=="',
+      '+      "integrity": "sha512-tamperedtampered=="',
+      "     }",
+      "@@ -70,6 +70,6 @@",
+      '       "version": "4.17.20",',
+      '       "license": "MIT",',
+      '-      "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",',
+      '+      "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20-mirror.tgz",',
+      "     }",
+    ].join("\n");
+    // Integrity in hunk 1 + resolved swap in hunk 2, neither hunk bumps version — correlation must
+    // still produce a single candidate with resolvedOrIntegrityChanged && !versionChanged.
+    const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]);
+    expect(finding).not.toBeNull();
+    expect(finding?.code).toBe("lockfile_tamper_risk");
+    expect(finding?.title).toContain("unattributed lockfile entry");
+  });
+
+  it("REGRESSION (#8351): two different header-less packages in one patch never share an unattributed bucket", () => {
+    const lockPatch = [
+      "@@ -50,6 +50,6 @@",
+      '       "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",',
+      '       "license": "MIT",',
+      '-      "integrity": "sha512-old-lodash=="',
+      '+      "integrity": "sha512-tampered-lodash=="',
+      "     }",
+      "@@ -80,6 +80,6 @@",
+      '       "resolved": "https://registry.npmjs.org/express/-/express-4.18.0.tgz",',
+      '       "license": "MIT",',
+      '-      "integrity": "sha512-old-express=="',
+      '+      "integrity": "sha512-tampered-express=="',
+      "     }",
+    ].join("\n");
+    const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]);
+    expect(finding).not.toBeNull();
+    // Both packages are independently tampered; collapsing them into one bucket would still flag, but
+    // the title/detail must keep the anonymous unattributed label (not a merged bare name) and the
+    // finding must fire — pinning that distinct registry URLs mint distinct correlation keys.
+    expect(finding?.title).toContain("unattributed lockfile entry");
+    expect(finding?.detail).toContain("unattributed lockfile entry");
+  });
+
+  it("REGRESSION (#8351): upgrades a sequence-keyed unattributed bucket when a resolved URL appears later in the same header-less block", () => {
+    // No leading context resolved URL — version lines mint `#unattributed-N` first; the following
+    // resolved URL recovers the package name and must re-key so a PRIOR hunk's package-keyed integrity
+    // signal (and this block's version bump) still correlate into one candidate.
+    const lockPatch = [
+      "@@ -40,6 +40,6 @@",
+      '       "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",',
+      '       "license": "MIT",',
+      '-      "integrity": "sha512-old=="',
+      '+      "integrity": "sha512-new=="',
+      "     }",
+      "@@ -55,8 +55,8 @@",
+      '-      "version": "1.0.0",',
+      '+      "version": "1.0.1",',
+      '-      "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",',
+      '+      "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.1.tgz",',
+      '-      "integrity": "sha512-new=="',
+      '+      "integrity": "sha512-newer=="',
+      "     }",
+    ].join("\n");
+    expect(lockfileTamperRiskFinding([lockfilePatch(lockPatch)])).toBeNull();
+  });
+
+  it("REGRESSION (#8351): in-block sequence→package re-key works even with no prior package-keyed candidate", () => {
+    const lockPatch = [
+      "@@ -55,8 +55,8 @@",
+      '-      "version": "1.0.0",',
+      '+      "version": "1.0.1",',
+      '-      "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",',
+      '+      "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.1.tgz",',
+      '-      "integrity": "sha512-old=="',
+      '+      "integrity": "sha512-new=="',
+      "     }",
+    ].join("\n");
+    expect(lockfileTamperRiskFinding([lockfilePatch(lockPatch)])).toBeNull();
+  });
+
+  it("REGRESSION (#8351): sequence→package upgrade merges an off-registry resolved URL onto the prior package-keyed candidate", () => {
+    const lockPatch = [
+      "@@ -10,5 +10,5 @@",
+      '       "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",',
+      '       "license": "MIT",',
+      '-      "version": "1.0.0",',
+      '+      "version": "1.0.1",',
+      "     }",
+      "@@ -20,6 +20,6 @@",
+      '-      "resolved": "https://evil.example.com/foo-old.tgz",',
+      '+      "resolved": "https://evil.example.com/foo-new.tgz",',
+      // A second resolved line (invalid JSON, but the line scanner is not a JSON parser) supplies the
+      // registry URL that recovers the package name and triggers the sequence→package upgrade merge.
+      '       "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",',
+      "     }",
+    ].join("\n");
+    const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]);
+    expect(finding).not.toBeNull();
+    expect(finding?.detail).toContain("outside registry.npmjs.org");
+  });
+
+  it("REGRESSION (#8351): merge keeps a prior off-registry URL when the sequence bucket has none", () => {
+    // Hunk 1 records an off-registry resolved under the package key. Hunk 2 starts with a version-only
+    // sequence bucket that later promotes and must NOT clear the prior off-registry URL (?? left arm).
+    const lockPatch = [
+      "@@ -10,6 +10,6 @@",
+      '       "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",',
+      '       "license": "MIT",',
+      '-      "resolved": "https://evil.example.com/foo-old.tgz",',
+      '+      "resolved": "https://evil.example.com/foo-new.tgz",',
+      "     }",
+      "@@ -30,6 +30,6 @@",
+      '-      "version": "1.0.0",',
+      '+      "version": "1.0.1",',
+      '-      "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",',
+      '+      "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.1.tgz",',
+      "     }",
+    ].join("\n");
+    const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]);
+    expect(finding).not.toBeNull();
+    expect(finding?.detail).toContain("outside registry.npmjs.org");
+  });
+
+  it("REGRESSION (#8351): correlates a scoped package across hunks via its registry resolved URL", () => {
+    const lockPatch = [
+      "@@ -50,6 +50,6 @@",
+      '       "resolved": "https://registry.npmjs.org/@babel/core/-/core-7.0.0.tgz",',
+      '       "license": "MIT",',
+      '-      "version": "7.0.0",',
+      '+      "version": "7.0.1",',
+      "     }",
+      "@@ -60,6 +60,6 @@",
+      '       "resolved": "https://registry.npmjs.org/@babel/core/-/core-7.0.0.tgz",',
+      '       "license": "MIT",',
+      '-      "integrity": "sha512-old=="',
+      '+      "integrity": "sha512-new=="',
+      "     }",
+    ].join("\n");
+    expect(lockfileTamperRiskFinding([lockfilePatch(lockPatch)])).toBeNull();
+  });
 });