Skip to content

Commit e8dde36

Browse files
committed
fix(enrichment): blame-link deleted-file + old-line clarity fixes (#2034)
Address review nits: - Handle removed files without requiring a patch: status === "removed" now blames via the file path alone (anchored to line 1), so a deleted file with a binary/truncated diff is no longer silently skipped despite the analyzer claiming to cover deletes. - Render the coordinate explicitly as `(old line N)` instead of `file:line`, which read like a current line — the value is the OLD-file line. - Give the commit→PR association call its own diagnostics subcall ("github-commit-pulls") instead of sharing "github-commits", for precise per-subcall diagnostics (shared endpoint budget unchanged). - Tests: a shifted-hunk case pinning the old-line semantics (+ its render) and a patchless removed-file case. review-enrichment: 402/402 pass.
1 parent 8e35fb9 commit e8dde36

3 files changed

Lines changed: 31 additions & 7 deletions

File tree

review-enrichment/src/analyzers/blame-link.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ async function fetchGithubJson<T>(
7575
fetchFn: typeof fetch,
7676
signal: AbortSignal | undefined,
7777
options: Pick<ScanOptions, "analysis" | "diagnostics">,
78+
subcall: string,
7879
): Promise<T | null> {
7980
const fetchOptions = {
8081
endpointCategory: "github-commits",
@@ -83,7 +84,7 @@ async function fetchGithubJson<T>(
8384
fetchImpl: fetchFn,
8485
diagnostics: options.diagnostics,
8586
phase: "blame-link",
86-
subcall: "github-commits",
87+
subcall,
8788
maxBytes: 256 * 1024,
8889
maxCallsPerCategory: MAX_LOOKUPS,
8990
};
@@ -109,7 +110,7 @@ export async function fetchLatestCommitSha(
109110
const url =
110111
`${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits` +
111112
`?path=${encodeURIComponent(path)}&per_page=1${shaQuery}`;
112-
const commits = await fetchGithubJson<CommitListItem[]>(url, headers, fetchFn, signal, options);
113+
const commits = await fetchGithubJson<CommitListItem[]>(url, headers, fetchFn, signal, options, "github-commits");
113114
const sha = Array.isArray(commits) ? commits[0]?.sha : undefined;
114115
return typeof sha === "string" && sha ? sha : null;
115116
}
@@ -125,7 +126,7 @@ export async function fetchPrForCommit(
125126
options: Pick<ScanOptions, "analysis" | "diagnostics">,
126127
): Promise<number | null> {
127128
const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(sha)}/pulls`;
128-
const pulls = await fetchGithubJson<AssociatedPr[]>(url, headers, fetchFn, signal, options);
129+
const pulls = await fetchGithubJson<AssociatedPr[]>(url, headers, fetchFn, signal, options, "github-commit-pulls");
129130
const number = Array.isArray(pulls) ? pulls[0]?.number : undefined;
130131
return typeof number === "number" ? number : null;
131132
}
@@ -150,9 +151,12 @@ export async function scanBlameLink(
150151
// additions (a patch with no deletion line has no prior author to attribute).
151152
const candidates: Array<{ path: string; line: number }> = [];
152153
for (const file of files) {
153-
if (file.status === "added" || SKIP_RE.test(file.path) || !file.patch) continue;
154-
const line = firstTouchedOldLine(file.patch);
155-
if (line === null) continue;
154+
if (file.status === "added" || SKIP_RE.test(file.path)) continue;
155+
let line = file.patch ? firstTouchedOldLine(file.patch) : null;
156+
// A removed file is entirely a deletion: its path alone drives the history lookup even when the diff carries no
157+
// usable patch (binary/truncated). Anchor to line 1 as the representative point.
158+
if (line === null && file.status === "removed") line = 1;
159+
if (line === null) continue; // a modified file with only additions / no usable patch → nothing to blame
156160
candidates.push({ path: file.path, line });
157161
if (candidates.length >= MAX_FILES_PROBED) break;
158162
}

review-enrichment/src/analyzers/registry.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -470,7 +470,7 @@ export const ANALYZER_DESCRIPTORS = [
470470
? `commit ${helpers.safeCodeSpan(item.introducedByShaPrefix)}`
471471
: "an unknown prior change";
472472
lines.push(
473-
`- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} was most recently introduced by ${origin}`,
473+
`- ${helpers.safeCodeSpan(item.file)} (old line ${item.line}) was most recently introduced by ${origin}`,
474474
);
475475
}
476476
return lines;

review-enrichment/test/blame-link.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,26 @@ test("scanBlameLink: resolves the originating PR for a modified line", async ()
5151
assert.match(brief, /#42/);
5252
});
5353

54+
test("scanBlameLink: reports the OLD-file line on a shifted hunk, and renders it as an old line", async () => {
55+
const findings = await scanBlameLink(
56+
// hunk shifted: old side starts at 20, new side at 25 — the blamed coordinate is the OLD line, not the new one
57+
req([{ path: "src/app.ts", status: "modified", patch: "@@ -20,3 +25,3 @@\n keep\n-old\n+new\n" }], { baseSha: "b" }),
58+
routedFetch({ commitSha: "abcdef1234567890", prNumber: 5 }),
59+
);
60+
assert.equal(findings[0].line, 21); // old 20 (header) + 1 context; NOT the new-side 26
61+
assert.match(renderBrief({ blameLink: findings }).promptSection, /old line 21/);
62+
});
63+
64+
test("scanBlameLink: a removed file is blamed via its path even without a patch", async () => {
65+
const findings = await scanBlameLink(
66+
req([{ path: "src/gone.ts", status: "removed" }], { baseSha: "b" }),
67+
routedFetch({ commitSha: "abcdef1234567890", prNumber: 8 }),
68+
);
69+
assert.deepEqual(findings, [
70+
{ file: "src/gone.ts", line: 1, introducedByShaPrefix: "abcdef123456", introducedByPr: 8 },
71+
]);
72+
});
73+
5474
test("scanBlameLink: a commit with no associated PR still surfaces the SHA prefix", async () => {
5575
const findings = await scanBlameLink(
5676
req([{ path: "src/app.ts", status: "modified", patch: modifyPatch(1) }]),

0 commit comments

Comments
 (0)