refactor(rees): promote reconstructOldContent into a shared analyzer helper (#4752)

reconstructOldContent (unified-diff reverse-patch reconstruction) was
private to doc-comment-drift.ts and imported cross-file from there by
exhaustiveness-drift.ts -- a one-off trick, not shared infrastructure.
Move it into its own co-located module (matching the existing
diff-lines.ts / binary-extensions.ts / github-headers.ts precedent for
shared analyzer helpers) so any analyzer can recover a changed file's
pre-PR text without re-deriving this.

Pure code motion: every executable line is byte-identical (verified via
diff + matching MD5 after stripping whole-line comments); only the
function's own doc comment and one inline comment were reworded to
drop doc-comment-drift-specific framing now that the helper is shared.
Both existing callers are migrated with a one-line import-path change
each and no other modifications.

Adds a dedicated test file for the promoted helper: the four existing
reconstructOldContent unit tests move over verbatim, plus five new
tests closing every previously-untested branch (a non-hunk preamble
line, out-of-order/overlapping hunks, a "no newline at end of file"
marker, the trailing-flush no-op case, and the wholly-new-file case) --
100% line/branch/function coverage confirmed via node's built-in
coverage instrumentation.

Author: JSONbored

review-enrichment/src/analyzers/doc-comment-drift.ts

@@ -6,6 +6,7 @@
 // non-finding. Deliberately conservative: only NAMED `function` declarations whose parameters are confidently
 // enumerable (any destructuring / non-identifier param → skip the function). Reports symbol + stale params + line.
 import type { EnrichRequest, DocCommentDriftFinding } from "../types.js";
+import { reconstructOldContent } from "./reconstruct-old-content.js";
 
 const MAX_FILES = 20;
 const MAX_FINDINGS = 50;
@@ -51,47 +52,6 @@ async function readBoundedText(resp: Response, signal?: AbortSignal): Promise<st
   }
 }
 
-/** Reconstruct the pre-PR content of a file by reverse-applying its unified `patch` to the post-PR `newContent`:
- *  context and removed (`-`) lines rebuild the old text; added (`+`) lines are dropped. Returns null if a hunk's
- *  position runs past the content (so the caller falls back to "no old parameters" and reports nothing). Pure. */
-export function reconstructOldContent(newContent: string, patch: string): string | null {
-  const newLines = newContent.split("\n");
-  const patchLines = patch.split("\n");
-  const out: string[] = [];
-  let cursor = 0; // next unconsumed index into newLines
-  let i = 0;
-  while (i < patchLines.length) {
-    const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(patchLines[i]!);
-    if (!header) {
-      i += 1;
-      continue;
-    }
-    const hunkStart = Number(header[1]) - 1; // 0-based new-file line the hunk begins at
-    if (hunkStart < cursor || hunkStart > newLines.length) return null;
-    while (cursor < hunkStart) out.push(newLines[cursor++]!); // unchanged lines before the hunk
-    i += 1;
-    while (i < patchLines.length && !patchLines[i]!.startsWith("@@")) {
-      const l = patchLines[i]!;
-      if (!l.startsWith("\\")) {
-        const sign = l[0];
-        const body = l.slice(1);
-        if (sign === "-") {
-          out.push(body); // removed: present in old only
-        } else {
-          // added or context lines must match the fetched head content at the cursor; a mismatch means the patch
-          // doesn't align with `newContent` (malformed/truncated input) → bail so we never trust a bad old signature.
-          if (newLines[cursor] !== body) return null;
-          if (sign !== "+") out.push(body); // context is present in old too; an added line is not
-          cursor += 1;
-        }
-      }
-      i += 1;
-    }
-  }
-  while (cursor < newLines.length) out.push(newLines[cursor++]!);
-  return out.join("\n");
-}
-
 /** Map every named `function NAME` declaration in `content` to its enumerable parameter-name set. A function whose
  *  parameters aren't confidently enumerable is omitted; a name DECLARED MORE THAN ONCE (overload/duplicate) is
  *  excluded entirely, so a lookup can never return a sibling declaration's parameters. Used to compare OLD vs NEW. */

review-enrichment/src/analyzers/exhaustiveness-drift.ts

@@ -5,7 +5,7 @@
 // file-fetch caps; fail-safe on missing token/headSha, bad slug, or fetch errors.
 import type { EnrichRequest, ExhaustivenessFinding } from "../types.js";
 import { githubHeaders } from "../github-headers.js";
-import { reconstructOldContent } from "./doc-comment-drift.js";
+import { reconstructOldContent } from "./reconstruct-old-content.js";
 import { isDiffFileHeaderLine } from "./diff-lines.js";
 import { isTestPath } from "./test-ratio.js";
 import { DEFAULT_MAX_FINDINGS } from "./limits.js";

review-enrichment/src/analyzers/reconstruct-old-content.ts

@@ -0,0 +1,71 @@
+// Shared unified-diff reverse-patch reconstruction (#4739, part of epic #4737). Originally private to
+// doc-comment-drift.ts (#1519) and imported cross-file from there by exhaustiveness-drift.ts (#2028) — a
+// one-off trick living in the wrong place rather than shared infrastructure. Promoted here, unchanged in
+// behavior, so any analyzer can recover a changed file's pre-PR text without re-deriving this.
+//
+// Cost note: this function does no I/O. The caller must already have fetched the file's post-change
+// (`headSha`) content — the same authed GitHub contents-API fetch every current caller already performs
+// for its own purposes — and pass it in as `newContent`. Promoting the reverse-patch algorithm out of
+// doc-comment-drift.ts does not add a new network call.
+//
+// Binary files: this function only ever sees two text blobs (`newContent`, `patch`) and has no file path
+// or extension to inspect, so it cannot itself detect a binary file. That filtering happens one layer up:
+// every current caller only invokes this after confirming the file's patch is present and its path
+// matches a known source extension (GitHub omits `.patch` entirely for binary/oversized files, so a
+// binary path never reaches here in practice). A future caller must keep doing that same source/extension
+// filtering before calling this — it is not this function's job to guess from content alone.
+
+/** Reconstruct the pre-PR content of a file by reverse-applying its unified `patch` to the post-PR
+ *  `newContent`: context and removed (`-`) lines rebuild the old text; added (`+`) lines are dropped.
+ *
+ *  Returns `null` when the patch cannot be reverse-applied against the given `newContent` — a hunk starts
+ *  before the cursor or past the end of the content, or an added/context line doesn't match `newContent`
+ *  at the expected position (a malformed/truncated patch, or a `newContent` that doesn't correspond to
+ *  the same ref the patch was computed against).
+ *
+ *  Returns an empty string when the patch reverse-applies cleanly but yields zero pre-PR lines — the case
+ *  for a file that did not exist before this PR (a "wholly added" patch has no old-side content to
+ *  rebuild). An empty string and `null` are both falsy; every caller should treat either as "no usable
+ *  before-content for this file" via a plain truthiness check (`if (!beforeContent) …`), not a strict
+ *  `=== null` comparison — the two are operationally the same "nothing to compare against" outcome, and
+ *  patch data alone cannot (and need not) distinguish a brand-new file from a pre-existing 0-byte one.
+ *
+ *  Pure — no I/O, no dependency on `path` or any other file metadata. */
+export function reconstructOldContent(newContent: string, patch: string): string | null {
+  const newLines = newContent.split("\n");
+  const patchLines = patch.split("\n");
+  const out: string[] = [];
+  let cursor = 0; // next unconsumed index into newLines
+  let i = 0;
+  while (i < patchLines.length) {
+    const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(patchLines[i]!);
+    if (!header) {
+      i += 1;
+      continue;
+    }
+    const hunkStart = Number(header[1]) - 1; // 0-based new-file line the hunk begins at
+    if (hunkStart < cursor || hunkStart > newLines.length) return null;
+    while (cursor < hunkStart) out.push(newLines[cursor++]!); // unchanged lines before the hunk
+    i += 1;
+    while (i < patchLines.length && !patchLines[i]!.startsWith("@@")) {
+      const l = patchLines[i]!;
+      if (!l.startsWith("\\")) {
+        const sign = l[0];
+        const body = l.slice(1);
+        if (sign === "-") {
+          out.push(body); // removed: present in old only
+        } else {
+          // added or context lines must match the fetched head content at the cursor; a mismatch means the patch
+          // doesn't align with `newContent` (malformed/truncated input, or a different ref) → bail so we never
+          // trust a reconstructed result that isn't provably faithful to the real pre-PR file.
+          if (newLines[cursor] !== body) return null;
+          if (sign !== "+") out.push(body); // context is present in old too; an added line is not
+          cursor += 1;
+        }
+      }
+      i += 1;
+    }
+  }
+  while (cursor < newLines.length) out.push(newLines[cursor++]!);
+  return out.join("\n");
+}

review-enrichment/test/doc-comment-drift.test.ts

@@ -3,7 +3,6 @@
 import { test } from "node:test";
 import assert from "node:assert/strict";
 import {
-  reconstructOldContent,
   extractFunctionParams,
   parseDocParams,
   parseFunctionParams,
@@ -26,12 +25,6 @@ const oldParams = (entries) => new Map(entries.map(([name, ids]) => [name, new S
 const DRIFTED = `/**\n * @param oldName the old one\n */\nexport function doThing(newName) {\n  return newName;\n}\n`;
 const DRIFT_PATCH = `@@ -1,6 +1,6 @@\n /**\n  * @param oldName the old one\n  */\n-export function doThing(oldName) {\n+export function doThing(newName) {\n   return newName;\n }`;
 
-test("reconstructOldContent: reverse-applies a patch to rebuild the pre-PR file", () => {
-  const old = reconstructOldContent(DRIFTED, DRIFT_PATCH);
-  assert.match(old, /function doThing\(oldName\)/); // the old parameter name is restored
-  assert.doesNotMatch(old, /newName\) \{/); // the added signature line is dropped
-});
-
 test("extractFunctionParams: maps each enumerable named function to its parameter set", () => {
   const map = extractFunctionParams(`export function f(a, b) {}\nfunction g({ x }) {}\nfunction h(c) {}\n`);
   assert.deepEqual([...map.get("f")], ["a", "b"]);
@@ -65,25 +58,6 @@ test("extractFunctionParams: skips a TS `this` pseudo-parameter, keeping the rea
   assert.deepEqual([...map.get("qux")], ["a"]);
 });
 
-test("reconstructOldContent: bails (null) when the patch context does not match the head content", () => {
-  // The context line ` other` doesn't exist in newContent → misaligned patch → fail closed.
-  assert.equal(reconstructOldContent(`a\nb\n`, `@@ -1,2 +1,2 @@\n-x\n+a\n other`), null);
-});
-
-test("reconstructOldContent: rebuilds across MULTIPLE hunks, filling the unchanged gap between them", () => {
-  // new file: a / X / c / d. Hunk 1 changed Y→X (line 2); hunk 2 changed D→d (line 4); `c` is the untouched gap.
-  const old = reconstructOldContent(
-    `a\nX\nc\nd\n`,
-    `@@ -1,2 +1,2 @@\n a\n-Y\n+X\n@@ -4,1 +4,1 @@\n-D\n+d`,
-  );
-  assert.equal(old, `a\nY\nc\nD\n`);
-});
-
-test("reconstructOldContent: bails (null) when a hunk starts beyond the head content's length", () => {
-  // A hunk anchored at line 99 of a 2-line file can't align → fail closed rather than fabricate old content.
-  assert.equal(reconstructOldContent(`a\nb\n`, `@@ -99,1 +99,1 @@\n a`), null);
-});
-
 test("findDocCommentDrift: a duplicate-named function is skipped (no cross-declaration false positive)", () => {
   // Two `dup` declarations; a stale @param on the first must not borrow the other's old params.
   const content = `/**\n * @param gone\n */\nexport function dup(a) {}\nfunction dup(b) {}\n`;

review-enrichment/test/reconstruct-old-content.test.ts

@@ -0,0 +1,83 @@
+// Units for the shared reverse-patch reconstruction helper (#4739, part of epic #4737). Own file (not
+// doc-comment-drift.test.ts / exhaustiveness-drift.test.ts) now that the function lives in its own
+// module and is consumed by both of those analyzers. Runs against the compiled dist/.
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { reconstructOldContent } from "../dist/analyzers/reconstruct-old-content.js";
+
+// The four tests below are relocated verbatim from doc-comment-drift.test.ts (this function's prior home)
+// as part of #4739's extraction — same fixtures, same assertions, zero behavior change.
+
+test("reconstructOldContent: reverse-applies a patch to rebuild the pre-PR file", () => {
+  const DRIFTED = `/**\n * @param oldName the old one\n */\nexport function doThing(newName) {\n  return newName;\n}\n`;
+  const DRIFT_PATCH = `@@ -1,6 +1,6 @@\n /**\n  * @param oldName the old one\n  */\n-export function doThing(oldName) {\n+export function doThing(newName) {\n   return newName;\n }`;
+  const old = reconstructOldContent(DRIFTED, DRIFT_PATCH);
+  assert.match(old, /function doThing\(oldName\)/); // the old parameter name is restored
+  assert.doesNotMatch(old, /newName\) \{/); // the added signature line is dropped
+});
+
+test("reconstructOldContent: bails (null) when the patch context does not match the head content", () => {
+  // The context line ` other` doesn't exist in newContent → misaligned patch → fail closed.
+  assert.equal(reconstructOldContent(`a\nb\n`, `@@ -1,2 +1,2 @@\n-x\n+a\n other`), null);
+});
+
+test("reconstructOldContent: rebuilds across MULTIPLE hunks, filling the unchanged gap between them", () => {
+  // new file: a / X / c / d. Hunk 1 changed Y→X (line 2); hunk 2 changed D→d (line 4); `c` is the untouched gap.
+  const old = reconstructOldContent(
+    `a\nX\nc\nd\n`,
+    `@@ -1,2 +1,2 @@\n a\n-Y\n+X\n@@ -4,1 +4,1 @@\n-D\n+d`,
+  );
+  assert.equal(old, `a\nY\nc\nD\n`);
+});
+
+test("reconstructOldContent: bails (null) when a hunk starts beyond the head content's length", () => {
+  // A hunk anchored at line 99 of a 2-line file can't align → fail closed rather than fabricate old content.
+  assert.equal(reconstructOldContent(`a\nb\n`, `@@ -99,1 +99,1 @@\n a`), null);
+});
+
+// The tests below are new, added for #4739's full-branch-coverage requirement on the promoted shared
+// helper — each pins a branch the four relocated tests above don't already exercise.
+
+test("reconstructOldContent: a non-hunk preamble line before the first @@ header is skipped, not fatal", () => {
+  // A raw `diff --git a/x b/x` style line (never present in GitHub's per-file `.patch`, but the loop
+  // defensively tolerates it) must be skipped over, not mistaken for hunk content or a parse failure.
+  const old = reconstructOldContent("b", "diff --git a/x b/x\n@@ -1,1 +1,1 @@\n-a\n+b");
+  assert.equal(old, "a");
+});
+
+test("reconstructOldContent: bails (null) when a later hunk starts before the previous hunk's cursor (out of order/overlap)", () => {
+  // Hunk 1 consumes new-file lines 1-2 (cursor ends at 2); hunk 2 claims to start at new-file line 2
+  // (0-based index 1), which is BEHIND the cursor — an out-of-order or overlapping hunk pair that must
+  // fail closed rather than reconstruct a nonsensical result.
+  assert.equal(
+    reconstructOldContent("a\nb\nc\nd", "@@ -1,2 +1,2 @@\n a\n b\n@@ -2,1 +2,1 @@\n c"),
+    null,
+  );
+});
+
+test("reconstructOldContent: a `\\ No newline at end of file` marker line is skipped, not treated as content", () => {
+  // The marker starts with `\` (never `+`/`-`/` `); it must be ignored entirely rather than parsed as a
+  // sign+body pair (which would read a bogus sign and desync the cursor, or falsely fail closed).
+  const old = reconstructOldContent(
+    "a\nb",
+    "@@ -1,2 +1,2 @@\n a\n-x\n+b\n\\ No newline at end of file",
+  );
+  assert.equal(old, "a\nx");
+});
+
+test("reconstructOldContent: the trailing unchanged-lines flush is a no-op when the last hunk already reaches EOF", () => {
+  // The final `while (cursor < newLines.length)` flush must correctly do NOTHING when the last hunk's
+  // context/added lines already consumed every remaining new-file line.
+  const old = reconstructOldContent("a\nb", "@@ -1,2 +1,2 @@\n-x\n+a\n b");
+  assert.equal(old, "x\nb");
+});
+
+test("reconstructOldContent: a wholly new file reconstructs to an empty string, not null — both are falsy", () => {
+  // A patch that is 100% additions (old range `-0,0`) has no old-side content to rebuild: the correct
+  // reconstruction of "the file did not exist before this PR" is an empty string, not null. Every caller
+  // must treat this the same as null via a plain truthiness check (see the module's own doc comment) —
+  // patch data alone cannot (and need not) distinguish a brand-new file from a pre-existing 0-byte one.
+  const old = reconstructOldContent("a\nb\nc", "@@ -0,0 +1,3 @@\n+a\n+b\n+c");
+  assert.equal(old, "");
+  assert.ok(!old); // falsy, exactly like null — this is the contract every caller relies on
+});