diff --git a/review-enrichment/src/analyzers/doc-comment-drift.ts b/review-enrichment/src/analyzers/doc-comment-drift.ts index b6715709e1..f8225f8d53 100644 --- a/review-enrichment/src/analyzers/doc-comment-drift.ts +++ b/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 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. */ diff --git a/review-enrichment/src/analyzers/exhaustiveness-drift.ts b/review-enrichment/src/analyzers/exhaustiveness-drift.ts index e04a36c018..2bf64252eb 100644 --- a/review-enrichment/src/analyzers/exhaustiveness-drift.ts +++ b/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"; diff --git a/review-enrichment/src/analyzers/reconstruct-old-content.ts b/review-enrichment/src/analyzers/reconstruct-old-content.ts new file mode 100644 index 0000000000..ce3da7723e --- /dev/null +++ b/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"); +} diff --git a/review-enrichment/test/doc-comment-drift.test.ts b/review-enrichment/test/doc-comment-drift.test.ts index 4a94d8c469..05800d3c7c 100644 --- a/review-enrichment/test/doc-comment-drift.test.ts +++ b/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`; diff --git a/review-enrichment/test/reconstruct-old-content.test.ts b/review-enrichment/test/reconstruct-old-content.test.ts new file mode 100644 index 0000000000..55df2782c5 --- /dev/null +++ b/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 +});