From 853a18d740ca41fb3007ed88721e9fd6849c59ad Mon Sep 17 00:00:00 2001 From: Nick M <274344962+nickmopen@users.noreply.github.com> Date: Thu, 2 Jul 2026 11:23:00 -0500 Subject: [PATCH 1/7] feat(enrichment): blame-to-PR regression linker analyzer (#2034) New REES github-light analyzer: for files a PR modifies/deletes, resolves which prior PR most recently introduced that region and surfaces it, giving the reviewer instant context on what the change is altering. Bounded + fail-safe. - types.ts: BlameLinkFinding ({ file, line, introducedByPr?, introducedByShaPrefix? }) + blameLink? key on BriefFindings (sha prefix only, never contents). - analyzers/blame-link.ts: firstTouchedOldLine() parses the patch for a representative modified/deleted old line; per file, reads the path's most recent base-branch commit and maps it to its PR via the commit->PR association API. Strict maxFilesProbed (6) + maxLookups (12) caps; emits partial on cap; skips added files and pure additions; no-token/bad-slug/fetch-error -> no finding. - registry.ts: descriptor (category history, cost github-light, requires files+github-token) with an inline render(). - render.ts: dispatch the blameLink section via renderDescriptorSection. - Regenerated analyzer-metadata.json, .env.example, and the UI metadata; added blameLink to the registry meta-test's expected list. - test/blame-link.test.ts: resolved PR, sha-only fallback, unresolvable (no finding), added/pure-addition skip, file-probe cap, token-absent skip. review-enrichment test suite: 400/400 pass. --- .env.example | 6 +- apps/gittensory-ui/src/lib/rees-analyzers.ts | 25 +++ review-enrichment/analyzer-metadata.json | 26 +++ review-enrichment/src/analyzers/blame-link.ts | 180 ++++++++++++++++++ review-enrichment/src/analyzers/registry.ts | 38 ++++ review-enrichment/src/render.ts | 2 + review-enrichment/src/types.ts | 15 ++ .../test/analyzer-registry.test.ts | 1 + review-enrichment/test/blame-link.test.ts | 97 ++++++++++ 9 files changed, 387 insertions(+), 3 deletions(-) create mode 100644 review-enrichment/src/analyzers/blame-link.ts create mode 100644 review-enrichment/test/blame-link.test.ts diff --git a/.env.example b/.env.example index 8e53a488b1..8f38bf80fa 100644 --- a/.env.example +++ b/.env.example @@ -65,17 +65,17 @@ GITTENSORY_REVIEW_ENRICHMENT=false # Current analyzer names: # dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol,redos # provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild -# history,docCommentDrift,duplication,churnHotspot +# history,docCommentDrift,duplication,churnHotspot,blameLink # # Profile defaults: # fast: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol # redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild # balanced (default): dependency,lockfileDrift,secret,license,installScript,heavyDependency # actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature -# iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot +# iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink # deep: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol # redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig -# nativeBuild,history,docCommentDrift,duplication,churnHotspot +# nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink # END GENERATED REES ANALYZERS # Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index 6a2f1a18a3..d8fd231edd 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -559,6 +559,31 @@ export const REES_ANALYZERS = [ "Distinct from the history analyzer's author track record; this scores the change AREA's defect density.", }, }, + { + name: "blameLink", + title: "Blame → originating PR", + category: "history", + cost: "github-light", + defaultEnabled: true, + profiles: ["balanced", "deep"], + requires: ["files", "github-token"], + limits: { + maxFilesProbed: 6, + maxLookups: 12, + }, + docs: { + summary: + "For files this PR modifies or deletes, links the region to the prior PR that most recently introduced it.", + looksAt: + "The first modified/deleted line of each changed file, then that path's most recent base-branch commit and its associated PR.", + reports: + "File, a representative old line, the originating PR number, and a short commit-SHA prefix — never file contents.", + network: + "Calls the GitHub commits API and the commit→PR association API, both bounded by a total lookup cap.", + notes: + "Not per-line blame: it attributes each touched file's most recent prior commit. Fail-safe and partial on cap.", + }, + }, ] as const satisfies readonly ReesAnalyzerDoc[]; export const REES_ANALYZER_NAMES = REES_ANALYZERS.map((analyzer) => analyzer.name); diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index 6b4b3c3248..0c56471d74 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -636,6 +636,32 @@ "network": "Calls the GitHub commits API once per probed file. Requires GitHub token forwarding for private repos.", "notes": "Distinct from the history analyzer's author track record; this scores the change AREA's defect density." } + }, + { + "name": "blameLink", + "title": "Blame → originating PR", + "category": "history", + "cost": "github-light", + "defaultEnabled": true, + "profiles": [ + "balanced", + "deep" + ], + "requires": [ + "files", + "github-token" + ], + "limits": { + "maxFilesProbed": 6, + "maxLookups": 12 + }, + "docs": { + "summary": "For files this PR modifies or deletes, links the region to the prior PR that most recently introduced it.", + "looksAt": "The first modified/deleted line of each changed file, then that path's most recent base-branch commit and its associated PR.", + "reports": "File, a representative old line, the originating PR number, and a short commit-SHA prefix — never file contents.", + "network": "Calls the GitHub commits API and the commit→PR association API, both bounded by a total lookup cap.", + "notes": "Not per-line blame: it attributes each touched file's most recent prior commit. Fail-safe and partial on cap." + } } ] } diff --git a/review-enrichment/src/analyzers/blame-link.ts b/review-enrichment/src/analyzers/blame-link.ts new file mode 100644 index 0000000000..6de7caadc6 --- /dev/null +++ b/review-enrichment/src/analyzers/blame-link.ts @@ -0,0 +1,180 @@ +// Blame-to-PR regression linker (#2034, part of #1499). For files this PR MODIFIES or DELETES, resolves which +// prior PR most recently introduced that region and surfaces it, so the reviewer sees at a glance what history the +// change is altering. It does not run true per-line blame (no checkout, no blame API): for each touched file it +// reads the path's most recent commit on the base branch and maps that commit to its PR via the commit→PR +// association API. Bounded (maxFilesProbed + maxLookups) and fail-safe — any missing token, bad slug, or fetch +// error yields no finding rather than an error. Surfaces only a PR number and a short SHA prefix, never contents. +import type { + AnalyzerDiagnostics, + EnrichRequest, + BlameLinkFinding, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchJson } from "../external-fetch.js"; + +const GITHUB_API = "https://api.github.com"; +const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const MAX_FILES_PROBED = 6; // bound the files we probe, matching the other history-class analyzers +const MAX_LOOKUPS = 12; // hard cap on total GitHub round-trips (each file costs up to 2: commits + pulls) +const SHA_PREFIX_LEN = 12; +// Files whose commit history is not a useful "who introduced this" signal — lockfiles, generated output, binaries. +const SKIP_RE = + /(?:^|\/)(?:package-lock\.json|yarn\.lock|pnpm-lock\.yaml|poetry\.lock|go\.sum)$|\.(?:lock|min\.js|map|snap|png|jpe?g|gif|svg|ico|pdf|zip|gz|woff2?)$|(?:^|\/)(?:dist|build|vendor)\//i; + +interface ScanOptions { + signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; +} + +/** The slice of a GitHub commit-list item this analyzer reads. */ +interface CommitListItem { + sha?: string; +} +/** The slice of a commit→PR association item this analyzer reads. */ +interface AssociatedPr { + number?: number; +} + +/** + * The old-file line number of the FIRST line this patch modifies or deletes, or null when the patch only ADDS + * lines (nothing pre-existing is being altered, so there is no prior author to attribute). Walks unified-diff + * hunks: a `@@ -old,+new @@` header resets the old-line cursor; context lines advance it; a deletion line reports + * the cursor; addition lines do not advance it (they exist only in the new file). Pure. */ +export function firstTouchedOldLine(patch: string): number | null { + let oldLine = 0; + let inHunk = false; + for (const raw of patch.split("\n")) { + const header = raw.match(/^@@ -(\d+)(?:,\d+)? \+\d+(?:,\d+)? @@/); + if (header) { + oldLine = Number(header[1]); + inHunk = true; + continue; + } + if (!inHunk) continue; + if (raw.startsWith("---") || raw.startsWith("+++")) continue; // stray file headers inside the fragment + if (raw.startsWith("-")) return oldLine; // first modified/deleted old-file line + if (raw.startsWith("+")) continue; // added line: present only in the new file, no old-line advance + oldLine += 1; // context line + } + return null; +} + +function githubHeaders(token: string): Record { + return { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }; +} + +async function fetchGithubJson( + url: string, + headers: Record, + fetchFn: typeof fetch, + signal: AbortSignal | undefined, + options: Pick, +): Promise { + const fetchOptions = { + endpointCategory: "github-commits", + headers, + signal, + fetchImpl: fetchFn, + diagnostics: options.diagnostics, + phase: "blame-link", + subcall: "github-commits", + maxBytes: 256 * 1024, + maxCallsPerCategory: MAX_LOOKUPS, + }; + const response = options.analysis + ? await options.analysis.fetchJson(url, fetchOptions) + : await boundedFetchJson(url, fetchOptions); + return response.ok ? response.data : null; +} + +/** The SHA of the most recent commit touching `path` on the base branch, or null. Anchoring to `baseSha` keeps + * the PR's own commits out of the answer so we attribute PRIOR authorship, not this change. */ +export async function fetchLatestCommitSha( + owner: string, + repo: string, + path: string, + baseSha: string | undefined, + headers: Record, + fetchFn: typeof fetch, + signal: AbortSignal | undefined, + options: Pick, +): Promise { + const shaQuery = baseSha ? `&sha=${encodeURIComponent(baseSha)}` : ""; + const url = + `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits` + + `?path=${encodeURIComponent(path)}&per_page=1${shaQuery}`; + const commits = await fetchGithubJson(url, headers, fetchFn, signal, options); + const sha = Array.isArray(commits) ? commits[0]?.sha : undefined; + return typeof sha === "string" && sha ? sha : null; +} + +/** The number of the PR that a commit belongs to, via the commit→PR association API, or null when unassociated. */ +export async function fetchPrForCommit( + owner: string, + repo: string, + sha: string, + headers: Record, + fetchFn: typeof fetch, + signal: AbortSignal | undefined, + options: Pick, +): Promise { + const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(sha)}/pulls`; + const pulls = await fetchGithubJson(url, headers, fetchFn, signal, options); + const number = Array.isArray(pulls) ? pulls[0]?.number : undefined; + return typeof number === "number" ? number : null; +} + +/** Analyzer entrypoint: changed files that alter existing lines → the prior PR/commit that introduced them. + * Fail-safe — no token, bad slug, or fetch error yields no finding. Stops and returns partial results at the + * lookup cap. */ +export async function scanBlameLink( + req: EnrichRequest, + fetchFn: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + const { repoFullName, githubToken, baseSha, files = [] } = req; + if (!githubToken) return []; + const parts = repoFullName.split("/"); + const owner = parts[0]; + const repo = parts[1]; + if (parts.length !== 2 || !owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return []; + + const headers = githubHeaders(githubToken); + // Only files that alter pre-existing lines can be blamed: skip added files, generated/binary paths, and pure + // additions (a patch with no deletion line has no prior author to attribute). + const candidates: Array<{ path: string; line: number }> = []; + for (const file of files) { + if (file.status === "added" || SKIP_RE.test(file.path) || !file.patch) continue; + const line = firstTouchedOldLine(file.patch); + if (line === null) continue; + candidates.push({ path: file.path, line }); + if (candidates.length >= MAX_FILES_PROBED) break; + } + + const findings: BlameLinkFinding[] = []; + let lookups = 0; + for (const { path, line } of candidates) { + if (options.signal?.aborted) break; + if (lookups >= MAX_LOOKUPS) break; // cap reached → emit partial + lookups += 1; + const sha = await fetchLatestCommitSha(owner, repo, path, baseSha, headers, fetchFn, options.signal, options); + if (!sha) continue; // unresolvable line → no finding + let introducedByPr: number | null = null; + if (lookups < MAX_LOOKUPS && !options.signal?.aborted) { + lookups += 1; + introducedByPr = await fetchPrForCommit(owner, repo, sha, headers, fetchFn, options.signal, options); + } + findings.push({ + file: path, + line, + introducedByShaPrefix: sha.slice(0, SHA_PREFIX_LEN), + ...(introducedByPr !== null ? { introducedByPr } : {}), + }); + } + return findings; +} diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index b21f28f10f..6369251729 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -1,6 +1,7 @@ import { scanActionPins } from "./actions-pin.js"; import { scanAssetWeight } from "./asset-weight.js"; import { scanChurnHotspot } from "./churn-hotspot.js"; +import { scanBlameLink } from "./blame-link.js"; import { scanCodeowners } from "./codeowners.js"; import { scanCommitSignature } from "./commit-signature.js"; import { dependencyAnalyzer } from "./dependency/descriptor.js"; @@ -440,6 +441,43 @@ export const ANALYZER_DESCRIPTORS = [ run: (req, { signal, analysis, diagnostics }) => scanChurnHotspot(req, fetch, { signal, analysis, diagnostics }), }), + descriptor({ + name: "blameLink", + title: "Blame → originating PR", + category: "history", + cost: "github-light", + defaultEnabled: true, + requires: ["files", "github-token"], + limits: { maxFilesProbed: 6, maxLookups: 12 }, + docs: { + summary: + "For files this PR modifies or deletes, links the region to the prior PR that most recently introduced it.", + looksAt: + "The first modified/deleted line of each changed file, then that path's most recent base-branch commit and its associated PR.", + reports: "File, a representative old line, the originating PR number, and a short commit-SHA prefix — never file contents.", + network: "Calls the GitHub commits API and the commit→PR association API, both bounded by a total lookup cap.", + notes: + "Not per-line blame: it attributes each touched file's most recent prior commit. Fail-safe and partial on cap.", + }, + render: (findings, helpers) => { + if (!findings.length) return []; + const lines = ["### Prior PRs this change alters (blame → originating PR)"]; + for (const item of findings) { + const origin = + item.introducedByPr !== undefined + ? `#${item.introducedByPr}` + : item.introducedByShaPrefix + ? `commit ${helpers.safeCodeSpan(item.introducedByShaPrefix)}` + : "an unknown prior change"; + lines.push( + `- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} was most recently introduced by ${origin}`, + ); + } + return lines; + }, + run: (req, { signal, analysis, diagnostics }) => + scanBlameLink(req, fetch, { signal, analysis, diagnostics }), + }), ] as const satisfies readonly AnyAnalyzerDescriptor[]; export const ANALYZER_NAMES = ANALYZER_DESCRIPTORS.map( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 8c6193127c..40f6489698 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -379,6 +379,8 @@ export function renderBrief( } } + lines.push(...renderDescriptorSection("blameLink", findings.blameLink)); + if (!lines.length) return { promptSection: "", systemSuffix: "" }; const header = diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 07b7ec54bb..cddeab6df0 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -285,6 +285,20 @@ export interface ChurnHotspotFinding { capped: boolean; } +/** For a changed file that MODIFIES or DELETES existing lines, the prior PR (or commit) that most recently + * introduced that region — resolved from the path's public commit history + the commit→PR association API. Gives + * the reviewer instant context on what the change is altering. Surfaces only a PR number and a short SHA prefix, + * never file contents. (#2034, part of #1499) */ +export interface BlameLinkFinding { + file: string; + /** A representative old-file line number from the touched range (the first modified/deleted line). */ + line: number; + /** The PR that introduced the region, when the commit maps to one via the commit/PR-association API. */ + introducedByPr?: number; + /** Short prefix of the introducing commit's SHA (prefix only — never the full SHA). */ + introducedByShaPrefix?: string; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -308,6 +322,7 @@ export interface BriefFindings { docCommentDrift?: DocCommentDriftFinding[]; duplication?: DuplicationFinding[]; churnHotspot?: ChurnHotspotFinding[]; + blameLink?: BlameLinkFinding[]; } /** A JSDoc/TSDoc block whose `@param` tags name parameters the adjacent function no longer declares — a diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index a6a03dfbc8..483cfb23d0 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -31,6 +31,7 @@ const EXPECTED_ANALYZERS = [ "docCommentDrift", "duplication", "churnHotspot", + "blameLink", ]; test("analyzer descriptors cover the runtime registry in stable order", () => { diff --git a/review-enrichment/test/blame-link.test.ts b/review-enrichment/test/blame-link.test.ts new file mode 100644 index 0000000000..d2959e61c8 --- /dev/null +++ b/review-enrichment/test/blame-link.test.ts @@ -0,0 +1,97 @@ +// Units for the blame-to-PR regression linker (#2034). Own file (not enrichment.test.ts) so concurrent analyzer +// PRs don't collide. All network is mocked. Runs against the compiled dist/. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + firstTouchedOldLine, + scanBlameLink, +} from "../dist/analyzers/blame-link.js"; +import { renderBrief } from "../dist/render.js"; + +const jsonResponse = (body, code = 200) => new Response(JSON.stringify(body), { status: code }); + +// A unified-diff patch that modifies an existing line: header at `oldStart`, one context line, one deletion. +const modifyPatch = (oldStart) => `@@ -${oldStart},3 +${oldStart},3 @@\n unchanged\n-old code\n+new code\n`; + +const req = (files, extra = {}) => ({ + repoFullName: "octo/repo", + prNumber: 1, + githubToken: "ghp_test", + files, + ...extra, +}); + +// A fetch stub that routes by URL: the commit→PR association endpoint (…/pulls) vs the path-history endpoint. +const routedFetch = ({ commitSha, prNumber }) => async (url) => { + if (url.includes("/pulls")) return jsonResponse(prNumber === null ? [] : [{ number: prNumber }]); + if (url.includes("/commits?")) return jsonResponse(commitSha === null ? [] : [{ sha: commitSha }]); + return jsonResponse([], 404); +}; + +test("firstTouchedOldLine: reports the first modified/deleted old-file line, null for pure additions", () => { + assert.equal(firstTouchedOldLine("@@ -10,3 +10,4 @@\n keep\n-drop\n+add\n"), 11); // context 10, deletion 11 + assert.equal(firstTouchedOldLine("@@ -5,2 +5,2 @@\n-first\n+repl\n"), 5); // deletion is the first hunk line + assert.equal(firstTouchedOldLine("@@ -0,0 +1,3 @@\n+a\n+b\n+c\n"), null); // pure addition → nothing to blame + assert.equal(firstTouchedOldLine("no hunk header here"), null); +}); + +test("scanBlameLink: resolves the originating PR for a modified line", async () => { + const findings = await scanBlameLink( + req([{ path: "src/app.ts", status: "modified", patch: modifyPatch(40) }], { baseSha: "base123" }), + routedFetch({ commitSha: "abcdef1234567890", prNumber: 42 }), + ); + assert.deepEqual(findings, [ + { file: "src/app.ts", line: 41, introducedByShaPrefix: "abcdef123456", introducedByPr: 42 }, + ]); + // and it renders into the brief with the PR reference + const brief = renderBrief({ blameLink: findings }).promptSection; + assert.match(brief, /blame → originating PR/i); + assert.match(brief, /#42/); +}); + +test("scanBlameLink: a commit with no associated PR still surfaces the SHA prefix", async () => { + const findings = await scanBlameLink( + req([{ path: "src/app.ts", status: "modified", patch: modifyPatch(1) }]), + routedFetch({ commitSha: "deadbeefcafebabe", prNumber: null }), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].introducedByShaPrefix, "deadbeefcafe"); + assert.equal(findings[0].introducedByPr, undefined); +}); + +test("scanBlameLink: an unresolvable line (no prior commit) yields no finding", async () => { + const findings = await scanBlameLink( + req([{ path: "src/app.ts", status: "modified", patch: modifyPatch(3) }]), + routedFetch({ commitSha: null, prNumber: null }), + ); + assert.deepEqual(findings, []); +}); + +test("scanBlameLink: pure-addition and added files are skipped (nothing to blame)", async () => { + const findings = await scanBlameLink( + req([ + { path: "new.ts", status: "added", patch: "@@ -0,0 +1,2 @@\n+a\n+b\n" }, + { path: "onlyadds.ts", status: "modified", patch: "@@ -3,0 +4,2 @@\n+x\n+y\n" }, + ]), + routedFetch({ commitSha: "abcdef1234567890", prNumber: 7 }), + ); + assert.deepEqual(findings, []); +}); + +test("scanBlameLink: caps the number of files probed", async () => { + const files = Array.from({ length: 10 }, (_, i) => ({ + path: `src/f${i}.ts`, + status: "modified", + patch: modifyPatch(i + 1), + })); + const findings = await scanBlameLink(req(files), routedFetch({ commitSha: "abcdef1234567890", prNumber: 9 })); + assert.equal(findings.length, 6); // MAX_FILES_PROBED +}); + +test("scanBlameLink: no GitHub token → skipped (no finding, no throw)", async () => { + const findings = await scanBlameLink( + req([{ path: "src/app.ts", status: "modified", patch: modifyPatch(2) }], { githubToken: undefined }), + routedFetch({ commitSha: "abcdef1234567890", prNumber: 1 }), + ); + assert.deepEqual(findings, []); +}); From 8e35fb9dd1c6fec75d7debf0a532c908fa79e1bd Mon Sep 17 00:00:00 2001 From: Nick M <274344962+nickmopen@users.noreply.github.com> Date: Thu, 2 Jul 2026 11:34:04 -0500 Subject: [PATCH 2/7] fix(enrichment): skip the no-newline diff marker in blame-link patch parser (#2034) firstTouchedOldLine treated a `\ No newline at end of file` marker line as a context line, wrongly advancing the old-file line counter. Skip it as metadata (like the stray ---/+++ headers). Added a test covering the marker. review-enrichment: 400/400 pass. --- review-enrichment/src/analyzers/blame-link.ts | 1 + review-enrichment/test/blame-link.test.ts | 2 ++ 2 files changed, 3 insertions(+) diff --git a/review-enrichment/src/analyzers/blame-link.ts b/review-enrichment/src/analyzers/blame-link.ts index 6de7caadc6..19f69f7cb0 100644 --- a/review-enrichment/src/analyzers/blame-link.ts +++ b/review-enrichment/src/analyzers/blame-link.ts @@ -53,6 +53,7 @@ export function firstTouchedOldLine(patch: string): number | null { } if (!inHunk) continue; if (raw.startsWith("---") || raw.startsWith("+++")) continue; // stray file headers inside the fragment + if (raw.startsWith("\\")) continue; // `\ No newline at end of file` marker — metadata, not a real line if (raw.startsWith("-")) return oldLine; // first modified/deleted old-file line if (raw.startsWith("+")) continue; // added line: present only in the new file, no old-line advance oldLine += 1; // context line diff --git a/review-enrichment/test/blame-link.test.ts b/review-enrichment/test/blame-link.test.ts index d2959e61c8..8e702dd3b6 100644 --- a/review-enrichment/test/blame-link.test.ts +++ b/review-enrichment/test/blame-link.test.ts @@ -33,6 +33,8 @@ test("firstTouchedOldLine: reports the first modified/deleted old-file line, nul assert.equal(firstTouchedOldLine("@@ -5,2 +5,2 @@\n-first\n+repl\n"), 5); // deletion is the first hunk line assert.equal(firstTouchedOldLine("@@ -0,0 +1,3 @@\n+a\n+b\n+c\n"), null); // pure addition → nothing to blame assert.equal(firstTouchedOldLine("no hunk header here"), null); + // The `\ No newline at end of file` marker is metadata — it must not advance the old-line counter. + assert.equal(firstTouchedOldLine("@@ -7,2 +7,1 @@\n keep\n-gone\n\\ No newline at end of file\n"), 8); }); test("scanBlameLink: resolves the originating PR for a modified line", async () => { From e8dde367a25e3039086b97902ea9b171781468dd Mon Sep 17 00:00:00 2001 From: Nick M <274344962+nickmopen@users.noreply.github.com> Date: Thu, 2 Jul 2026 11:41:34 -0500 Subject: [PATCH 3/7] fix(enrichment): blame-link deleted-file + old-line clarity fixes (#2034) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- review-enrichment/src/analyzers/blame-link.ts | 16 +++++++++------ review-enrichment/src/analyzers/registry.ts | 2 +- review-enrichment/test/blame-link.test.ts | 20 +++++++++++++++++++ 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/review-enrichment/src/analyzers/blame-link.ts b/review-enrichment/src/analyzers/blame-link.ts index 19f69f7cb0..dc42c980e6 100644 --- a/review-enrichment/src/analyzers/blame-link.ts +++ b/review-enrichment/src/analyzers/blame-link.ts @@ -75,6 +75,7 @@ async function fetchGithubJson( fetchFn: typeof fetch, signal: AbortSignal | undefined, options: Pick, + subcall: string, ): Promise { const fetchOptions = { endpointCategory: "github-commits", @@ -83,7 +84,7 @@ async function fetchGithubJson( fetchImpl: fetchFn, diagnostics: options.diagnostics, phase: "blame-link", - subcall: "github-commits", + subcall, maxBytes: 256 * 1024, maxCallsPerCategory: MAX_LOOKUPS, }; @@ -109,7 +110,7 @@ export async function fetchLatestCommitSha( const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits` + `?path=${encodeURIComponent(path)}&per_page=1${shaQuery}`; - const commits = await fetchGithubJson(url, headers, fetchFn, signal, options); + const commits = await fetchGithubJson(url, headers, fetchFn, signal, options, "github-commits"); const sha = Array.isArray(commits) ? commits[0]?.sha : undefined; return typeof sha === "string" && sha ? sha : null; } @@ -125,7 +126,7 @@ export async function fetchPrForCommit( options: Pick, ): Promise { const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(sha)}/pulls`; - const pulls = await fetchGithubJson(url, headers, fetchFn, signal, options); + const pulls = await fetchGithubJson(url, headers, fetchFn, signal, options, "github-commit-pulls"); const number = Array.isArray(pulls) ? pulls[0]?.number : undefined; return typeof number === "number" ? number : null; } @@ -150,9 +151,12 @@ export async function scanBlameLink( // additions (a patch with no deletion line has no prior author to attribute). const candidates: Array<{ path: string; line: number }> = []; for (const file of files) { - if (file.status === "added" || SKIP_RE.test(file.path) || !file.patch) continue; - const line = firstTouchedOldLine(file.patch); - if (line === null) continue; + if (file.status === "added" || SKIP_RE.test(file.path)) continue; + let line = file.patch ? firstTouchedOldLine(file.patch) : null; + // A removed file is entirely a deletion: its path alone drives the history lookup even when the diff carries no + // usable patch (binary/truncated). Anchor to line 1 as the representative point. + if (line === null && file.status === "removed") line = 1; + if (line === null) continue; // a modified file with only additions / no usable patch → nothing to blame candidates.push({ path: file.path, line }); if (candidates.length >= MAX_FILES_PROBED) break; } diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 6369251729..80615468b3 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -470,7 +470,7 @@ export const ANALYZER_DESCRIPTORS = [ ? `commit ${helpers.safeCodeSpan(item.introducedByShaPrefix)}` : "an unknown prior change"; lines.push( - `- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} was most recently introduced by ${origin}`, + `- ${helpers.safeCodeSpan(item.file)} (old line ${item.line}) was most recently introduced by ${origin}`, ); } return lines; diff --git a/review-enrichment/test/blame-link.test.ts b/review-enrichment/test/blame-link.test.ts index 8e702dd3b6..094ac32e2a 100644 --- a/review-enrichment/test/blame-link.test.ts +++ b/review-enrichment/test/blame-link.test.ts @@ -51,6 +51,26 @@ test("scanBlameLink: resolves the originating PR for a modified line", async () assert.match(brief, /#42/); }); +test("scanBlameLink: reports the OLD-file line on a shifted hunk, and renders it as an old line", async () => { + const findings = await scanBlameLink( + // hunk shifted: old side starts at 20, new side at 25 — the blamed coordinate is the OLD line, not the new one + req([{ path: "src/app.ts", status: "modified", patch: "@@ -20,3 +25,3 @@\n keep\n-old\n+new\n" }], { baseSha: "b" }), + routedFetch({ commitSha: "abcdef1234567890", prNumber: 5 }), + ); + assert.equal(findings[0].line, 21); // old 20 (header) + 1 context; NOT the new-side 26 + assert.match(renderBrief({ blameLink: findings }).promptSection, /old line 21/); +}); + +test("scanBlameLink: a removed file is blamed via its path even without a patch", async () => { + const findings = await scanBlameLink( + req([{ path: "src/gone.ts", status: "removed" }], { baseSha: "b" }), + routedFetch({ commitSha: "abcdef1234567890", prNumber: 8 }), + ); + assert.deepEqual(findings, [ + { file: "src/gone.ts", line: 1, introducedByShaPrefix: "abcdef123456", introducedByPr: 8 }, + ]); +}); + test("scanBlameLink: a commit with no associated PR still surfaces the SHA prefix", async () => { const findings = await scanBlameLink( req([{ path: "src/app.ts", status: "modified", patch: modifyPatch(1) }]), From b3a3873687e424fcdb32229d624be2e1d9a35bf7 Mon Sep 17 00:00:00 2001 From: Nick M <274344962+nickmopen@users.noreply.github.com> Date: Thu, 2 Jul 2026 11:59:28 -0500 Subject: [PATCH 4/7] refactor(enrichment): soften blame-link wording + precise category + tighter cap test (#2034) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final review nits (all non-blocking): - Render says the file "was most recently touched by" instead of "introduced by" a specific line — accurate to the file-level path-history lookup this does. - Give the commit→PR association call its own endpointCategory ("github-commit-pulls") for precise rate attribution (loop-level MAX_LOOKUPS still bounds the total). - Cap test now asserts exactly 12 fetch calls and that files past the probe cap (f6..f9) are never queried, pinning the bound. review-enrichment: 402/402 pass. --- review-enrichment/src/analyzers/blame-link.ts | 11 ++++++++--- review-enrichment/src/analyzers/registry.ts | 2 +- review-enrichment/test/blame-link.test.ts | 16 ++++++++++++++-- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/review-enrichment/src/analyzers/blame-link.ts b/review-enrichment/src/analyzers/blame-link.ts index dc42c980e6..b016956b1a 100644 --- a/review-enrichment/src/analyzers/blame-link.ts +++ b/review-enrichment/src/analyzers/blame-link.ts @@ -76,9 +76,10 @@ async function fetchGithubJson( signal: AbortSignal | undefined, options: Pick, subcall: string, + endpointCategory: string, ): Promise { const fetchOptions = { - endpointCategory: "github-commits", + endpointCategory, headers, signal, fetchImpl: fetchFn, @@ -110,7 +111,9 @@ export async function fetchLatestCommitSha( const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits` + `?path=${encodeURIComponent(path)}&per_page=1${shaQuery}`; - const commits = await fetchGithubJson(url, headers, fetchFn, signal, options, "github-commits"); + const commits = await fetchGithubJson( + url, headers, fetchFn, signal, options, "github-commits", "github-commits", + ); const sha = Array.isArray(commits) ? commits[0]?.sha : undefined; return typeof sha === "string" && sha ? sha : null; } @@ -126,7 +129,9 @@ export async function fetchPrForCommit( options: Pick, ): Promise { const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(sha)}/pulls`; - const pulls = await fetchGithubJson(url, headers, fetchFn, signal, options, "github-commit-pulls"); + const pulls = await fetchGithubJson( + url, headers, fetchFn, signal, options, "github-commit-pulls", "github-commit-pulls", + ); const number = Array.isArray(pulls) ? pulls[0]?.number : undefined; return typeof number === "number" ? number : null; } diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 80615468b3..cf6116f600 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -470,7 +470,7 @@ export const ANALYZER_DESCRIPTORS = [ ? `commit ${helpers.safeCodeSpan(item.introducedByShaPrefix)}` : "an unknown prior change"; lines.push( - `- ${helpers.safeCodeSpan(item.file)} (old line ${item.line}) was most recently introduced by ${origin}`, + `- ${helpers.safeCodeSpan(item.file)} (around old line ${item.line}) was most recently touched by ${origin}`, ); } return lines; diff --git a/review-enrichment/test/blame-link.test.ts b/review-enrichment/test/blame-link.test.ts index 094ac32e2a..28ac9d9737 100644 --- a/review-enrichment/test/blame-link.test.ts +++ b/review-enrichment/test/blame-link.test.ts @@ -100,14 +100,26 @@ test("scanBlameLink: pure-addition and added files are skipped (nothing to blame assert.deepEqual(findings, []); }); -test("scanBlameLink: caps the number of files probed", async () => { +test("scanBlameLink: caps probed files and total lookups, leaving later files untouched", async () => { const files = Array.from({ length: 10 }, (_, i) => ({ path: `src/f${i}.ts`, status: "modified", patch: modifyPatch(i + 1), })); - const findings = await scanBlameLink(req(files), routedFetch({ commitSha: "abcdef1234567890", prNumber: 9 })); + let calls = 0; + const probedPaths = new Set(); + const countingFetch = async (url) => { + calls += 1; + const path = new URL(url).searchParams.get("path"); + if (path) probedPaths.add(path); + if (url.includes("/pulls")) return jsonResponse([{ number: 9 }]); + if (url.includes("/commits?")) return jsonResponse([{ sha: "abcdef1234567890" }]); + return jsonResponse([], 404); + }; + const findings = await scanBlameLink(req(files), countingFetch); assert.equal(findings.length, 6); // MAX_FILES_PROBED + assert.equal(calls, 12); // 6 files × (1 commit-list + 1 pulls) = MAX_LOOKUPS; not one more + assert.deepEqual([...probedPaths].sort(), ["src/f0.ts", "src/f1.ts", "src/f2.ts", "src/f3.ts", "src/f4.ts", "src/f5.ts"]); // f6..f9 never probed }); test("scanBlameLink: no GitHub token → skipped (no finding, no throw)", async () => { From e8fb6ac0d97e3a8502f6c910aac07345f9128c80 Mon Sep 17 00:00:00 2001 From: Nick M <274344962+nickmopen@users.noreply.github.com> Date: Thu, 2 Jul 2026 12:57:29 -0500 Subject: [PATCH 5/7] refactor(enrichment): reframe blame-link as file-level last-touch, not line origin (#2034) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the reviewer's false-attribution concern: the analyzer only does a file-level latest-commit lookup, so it must not word its result as introducing a specific line. Make the claim match the behavior (no line-aware blame added). - Rename BlameLinkFinding.introducedBy{Pr,ShaPrefix} → lastTouchedBy{Pr,ShaPrefix}; the `line` field is now documented as a pointer to where THIS PR changes the file, not a line attributed to the PR. - Reframe type doc, analyzer docstring, descriptor title/docs, and the rendered section as "last PR to touch each file — file-level context, bounded scan". - Harden firstTouchedOldLine: only a space-prefixed context line advances the old cursor; additions, the `\` marker, and malformed/extended text no longer count, so a garbled patch fails closed. - Tests: rename fields; add a false-attribution regression (latest file commit touched a different region than the changed line → reported file-level, never attributed to the line) and a malformed-patch fail-closed case. review-enrichment: 403/403 pass. --- apps/gittensory-ui/src/lib/rees-analyzers.ts | 10 ++--- review-enrichment/analyzer-metadata.json | 10 ++--- review-enrichment/src/analyzers/blame-link.ts | 37 ++++++++++--------- review-enrichment/src/analyzers/registry.ts | 26 +++++++------ review-enrichment/src/types.ts | 20 +++++----- review-enrichment/test/blame-link.test.ts | 32 ++++++++++++---- 6 files changed, 79 insertions(+), 56 deletions(-) diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index d8fd231edd..867dfd2607 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -561,7 +561,7 @@ export const REES_ANALYZERS = [ }, { name: "blameLink", - title: "Blame → originating PR", + title: "Recent file history (last PR to touch)", category: "history", cost: "github-light", defaultEnabled: true, @@ -573,15 +573,15 @@ export const REES_ANALYZERS = [ }, docs: { summary: - "For files this PR modifies or deletes, links the region to the prior PR that most recently introduced it.", + "For files this PR modifies or deletes, surfaces the last PR to touch each file — file-level history context, not per-line blame.", looksAt: - "The first modified/deleted line of each changed file, then that path's most recent base-branch commit and its associated PR.", + "Each changed file's most recent base-branch commit (bounded to the first few files) and that commit's associated PR.", reports: - "File, a representative old line, the originating PR number, and a short commit-SHA prefix — never file contents.", + "File, a pointer to where this PR changes it, the last-touching PR number, and a short commit-SHA prefix — never file contents.", network: "Calls the GitHub commits API and the commit→PR association API, both bounded by a total lookup cap.", notes: - "Not per-line blame: it attributes each touched file's most recent prior commit. Fail-safe and partial on cap.", + "File-level, not per-line: it reports each file's most recent prior toucher, never claiming a specific line's origin. Fail-safe and partial on cap.", }, }, ] as const satisfies readonly ReesAnalyzerDoc[]; diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index 0c56471d74..dc5ca73fc9 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -639,7 +639,7 @@ }, { "name": "blameLink", - "title": "Blame → originating PR", + "title": "Recent file history (last PR to touch)", "category": "history", "cost": "github-light", "defaultEnabled": true, @@ -656,11 +656,11 @@ "maxLookups": 12 }, "docs": { - "summary": "For files this PR modifies or deletes, links the region to the prior PR that most recently introduced it.", - "looksAt": "The first modified/deleted line of each changed file, then that path's most recent base-branch commit and its associated PR.", - "reports": "File, a representative old line, the originating PR number, and a short commit-SHA prefix — never file contents.", + "summary": "For files this PR modifies or deletes, surfaces the last PR to touch each file — file-level history context, not per-line blame.", + "looksAt": "Each changed file's most recent base-branch commit (bounded to the first few files) and that commit's associated PR.", + "reports": "File, a pointer to where this PR changes it, the last-touching PR number, and a short commit-SHA prefix — never file contents.", "network": "Calls the GitHub commits API and the commit→PR association API, both bounded by a total lookup cap.", - "notes": "Not per-line blame: it attributes each touched file's most recent prior commit. Fail-safe and partial on cap." + "notes": "File-level, not per-line: it reports each file's most recent prior toucher, never claiming a specific line's origin. Fail-safe and partial on cap." } } ] diff --git a/review-enrichment/src/analyzers/blame-link.ts b/review-enrichment/src/analyzers/blame-link.ts index b016956b1a..23c0663205 100644 --- a/review-enrichment/src/analyzers/blame-link.ts +++ b/review-enrichment/src/analyzers/blame-link.ts @@ -1,9 +1,10 @@ -// Blame-to-PR regression linker (#2034, part of #1499). For files this PR MODIFIES or DELETES, resolves which -// prior PR most recently introduced that region and surfaces it, so the reviewer sees at a glance what history the -// change is altering. It does not run true per-line blame (no checkout, no blame API): for each touched file it -// reads the path's most recent commit on the base branch and maps that commit to its PR via the commit→PR -// association API. Bounded (maxFilesProbed + maxLookups) and fail-safe — any missing token, bad slug, or fetch -// error yields no finding rather than an error. Surfaces only a PR number and a short SHA prefix, never contents. +// Blame-to-PR regression linker (#2034, part of #1499). For files this PR MODIFIES or DELETES, surfaces the prior +// PR that most recently touched that FILE, so the reviewer sees at a glance what recent history the change sits on +// top of. It is deliberately NOT per-line blame (no checkout, no blame API): for each touched file it reads the +// path's most recent commit on the base branch and maps that commit to its PR via the commit→PR association API — +// so the result is file-level "last touched by", never a claim that the surfaced PR introduced a specific line. +// Bounded (maxFilesProbed + maxLookups) and fail-safe — any missing token, bad slug, or fetch error yields no +// finding rather than an error. Surfaces only a PR number and a short SHA prefix, never contents. import type { AnalyzerDiagnostics, EnrichRequest, @@ -39,8 +40,10 @@ interface AssociatedPr { /** * The old-file line number of the FIRST line this patch modifies or deletes, or null when the patch only ADDS * lines (nothing pre-existing is being altered, so there is no prior author to attribute). Walks unified-diff - * hunks: a `@@ -old,+new @@` header resets the old-line cursor; context lines advance it; a deletion line reports - * the cursor; addition lines do not advance it (they exist only in the new file). Pure. */ + * hunks: a `@@ -old,+new @@` header resets the old-line cursor; a deletion line reports the cursor; only a real + * space-prefixed CONTEXT line advances it. Additions, the `\ No newline` marker, and any malformed/extended patch + * text are NOT counted as old-file lines, so a garbled patch fails closed (returns null) rather than reporting a + * drifted line. Pure. */ export function firstTouchedOldLine(patch: string): number | null { let oldLine = 0; let inHunk = false; @@ -52,11 +55,9 @@ export function firstTouchedOldLine(patch: string): number | null { continue; } if (!inHunk) continue; - if (raw.startsWith("---") || raw.startsWith("+++")) continue; // stray file headers inside the fragment - if (raw.startsWith("\\")) continue; // `\ No newline at end of file` marker — metadata, not a real line - if (raw.startsWith("-")) return oldLine; // first modified/deleted old-file line - if (raw.startsWith("+")) continue; // added line: present only in the new file, no old-line advance - oldLine += 1; // context line + if (raw.startsWith("-") && !raw.startsWith("---")) return oldLine; // first modified/deleted old-file line + if (raw.startsWith(" ")) oldLine += 1; // a real context line — the ONLY thing that advances the old cursor + // Everything else (additions, `\`/`+++` markers, malformed text) is not an old-file line: do not advance. } return null; } @@ -173,17 +174,17 @@ export async function scanBlameLink( if (lookups >= MAX_LOOKUPS) break; // cap reached → emit partial lookups += 1; const sha = await fetchLatestCommitSha(owner, repo, path, baseSha, headers, fetchFn, options.signal, options); - if (!sha) continue; // unresolvable line → no finding - let introducedByPr: number | null = null; + if (!sha) continue; // no prior commit on the path → no finding + let lastTouchedByPr: number | null = null; if (lookups < MAX_LOOKUPS && !options.signal?.aborted) { lookups += 1; - introducedByPr = await fetchPrForCommit(owner, repo, sha, headers, fetchFn, options.signal, options); + lastTouchedByPr = await fetchPrForCommit(owner, repo, sha, headers, fetchFn, options.signal, options); } findings.push({ file: path, line, - introducedByShaPrefix: sha.slice(0, SHA_PREFIX_LEN), - ...(introducedByPr !== null ? { introducedByPr } : {}), + lastTouchedByShaPrefix: sha.slice(0, SHA_PREFIX_LEN), + ...(lastTouchedByPr !== null ? { lastTouchedByPr } : {}), }); } return findings; diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index cf6116f600..8efd1bcd90 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -443,7 +443,7 @@ export const ANALYZER_DESCRIPTORS = [ }), descriptor({ name: "blameLink", - title: "Blame → originating PR", + title: "Recent file history (last PR to touch)", category: "history", cost: "github-light", defaultEnabled: true, @@ -451,26 +451,28 @@ export const ANALYZER_DESCRIPTORS = [ limits: { maxFilesProbed: 6, maxLookups: 12 }, docs: { summary: - "For files this PR modifies or deletes, links the region to the prior PR that most recently introduced it.", + "For files this PR modifies or deletes, surfaces the last PR to touch each file — file-level history context, not per-line blame.", looksAt: - "The first modified/deleted line of each changed file, then that path's most recent base-branch commit and its associated PR.", - reports: "File, a representative old line, the originating PR number, and a short commit-SHA prefix — never file contents.", + "Each changed file's most recent base-branch commit (bounded to the first few files) and that commit's associated PR.", + reports: "File, a pointer to where this PR changes it, the last-touching PR number, and a short commit-SHA prefix — never file contents.", network: "Calls the GitHub commits API and the commit→PR association API, both bounded by a total lookup cap.", notes: - "Not per-line blame: it attributes each touched file's most recent prior commit. Fail-safe and partial on cap.", + "File-level, not per-line: it reports each file's most recent prior toucher, never claiming a specific line's origin. Fail-safe and partial on cap.", }, render: (findings, helpers) => { if (!findings.length) return []; - const lines = ["### Prior PRs this change alters (blame → originating PR)"]; + const lines = [ + "### Recent history of changed files (last PR to touch each — file-level context, bounded scan)", + ]; for (const item of findings) { - const origin = - item.introducedByPr !== undefined - ? `#${item.introducedByPr}` - : item.introducedByShaPrefix - ? `commit ${helpers.safeCodeSpan(item.introducedByShaPrefix)}` + const toucher = + item.lastTouchedByPr !== undefined + ? `#${item.lastTouchedByPr}` + : item.lastTouchedByShaPrefix + ? `commit ${helpers.safeCodeSpan(item.lastTouchedByShaPrefix)}` : "an unknown prior change"; lines.push( - `- ${helpers.safeCodeSpan(item.file)} (around old line ${item.line}) was most recently touched by ${origin}`, + `- ${helpers.safeCodeSpan(item.file)} (this PR changes it around old line ${item.line}) was last touched by ${toucher}`, ); } return lines; diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index cddeab6df0..7221ce3e61 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -285,18 +285,20 @@ export interface ChurnHotspotFinding { capped: boolean; } -/** For a changed file that MODIFIES or DELETES existing lines, the prior PR (or commit) that most recently - * introduced that region — resolved from the path's public commit history + the commit→PR association API. Gives - * the reviewer instant context on what the change is altering. Surfaces only a PR number and a short SHA prefix, - * never file contents. (#2034, part of #1499) */ +/** For a changed file that MODIFIES or DELETES existing lines, the prior PR (or commit) that most recently touched + * that FILE — resolved from the path's latest base-branch commit + the commit→PR association API. This is + * FILE-LEVEL context (the last change to land on the file before this PR), not per-line blame: it does not claim + * the surfaced PR introduced any specific line. Surfaces only a PR number and a short SHA prefix, never file + * contents. (#2034, part of #1499) */ export interface BlameLinkFinding { file: string; - /** A representative old-file line number from the touched range (the first modified/deleted line). */ + /** A representative old-file line from THIS PR's change (its first modified/deleted line) — a pointer to where + * the change lands, NOT a line attributed to `lastTouchedByPr`. */ line: number; - /** The PR that introduced the region, when the commit maps to one via the commit/PR-association API. */ - introducedByPr?: number; - /** Short prefix of the introducing commit's SHA (prefix only — never the full SHA). */ - introducedByShaPrefix?: string; + /** The last PR to touch this file before the change, when its commit maps to one via the commit/PR-association API. */ + lastTouchedByPr?: number; + /** Short prefix of that most-recent commit's SHA (prefix only — never the full SHA). */ + lastTouchedByShaPrefix?: string; } /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ diff --git a/review-enrichment/test/blame-link.test.ts b/review-enrichment/test/blame-link.test.ts index 28ac9d9737..35c6b01283 100644 --- a/review-enrichment/test/blame-link.test.ts +++ b/review-enrichment/test/blame-link.test.ts @@ -35,22 +35,40 @@ test("firstTouchedOldLine: reports the first modified/deleted old-file line, nul assert.equal(firstTouchedOldLine("no hunk header here"), null); // The `\ No newline at end of file` marker is metadata — it must not advance the old-line counter. assert.equal(firstTouchedOldLine("@@ -7,2 +7,1 @@\n keep\n-gone\n\\ No newline at end of file\n"), 8); + // Only space-prefixed context advances: a malformed/extended line must NOT be counted as an old-file line. + assert.equal(firstTouchedOldLine("@@ -5,2 +5,2 @@\nmalformed no-prefix line\n-x\n"), 5); // not 6 }); -test("scanBlameLink: resolves the originating PR for a modified line", async () => { +test("scanBlameLink: resolves the last PR to touch a modified file", async () => { const findings = await scanBlameLink( req([{ path: "src/app.ts", status: "modified", patch: modifyPatch(40) }], { baseSha: "base123" }), routedFetch({ commitSha: "abcdef1234567890", prNumber: 42 }), ); assert.deepEqual(findings, [ - { file: "src/app.ts", line: 41, introducedByShaPrefix: "abcdef123456", introducedByPr: 42 }, + { file: "src/app.ts", line: 41, lastTouchedByShaPrefix: "abcdef123456", lastTouchedByPr: 42 }, ]); - // and it renders into the brief with the PR reference + // and it renders into the brief as file-level "last touched", not a line-origin claim const brief = renderBrief({ blameLink: findings }).promptSection; - assert.match(brief, /blame → originating PR/i); + assert.match(brief, /last touched by/i); assert.match(brief, /#42/); }); +test("scanBlameLink: file-level only — it does NOT attribute the last-touch PR to the changed line's origin", async () => { + // The file's latest base commit (PR #99) touched a DIFFERENT region than the line this PR changes (old line 51). + // The finding must report #99 as the file's last toucher and line 51 only as a change POINTER — never a claim + // that #99 introduced line 51. (Reviewer's false-attribution regression case.) + const findings = await scanBlameLink( + req([{ path: "src/app.ts", status: "modified", patch: "@@ -50,3 +50,3 @@\n keep\n-line51\n+new\n" }], { baseSha: "b" }), + routedFetch({ commitSha: "aaaaaaaaaaaabbbb", prNumber: 99 }), + ); + assert.deepEqual(findings, [ + { file: "src/app.ts", line: 51, lastTouchedByShaPrefix: "aaaaaaaaaaaa", lastTouchedByPr: 99 }, + ]); + // The finding carries no "introducedBy"/origin field for the line — attribution is file-level by construction. + assert.equal("introducedByPr" in findings[0], false); + assert.match(renderBrief({ blameLink: findings }).promptSection, /file-level context/i); +}); + test("scanBlameLink: reports the OLD-file line on a shifted hunk, and renders it as an old line", async () => { const findings = await scanBlameLink( // hunk shifted: old side starts at 20, new side at 25 — the blamed coordinate is the OLD line, not the new one @@ -67,7 +85,7 @@ test("scanBlameLink: a removed file is blamed via its path even without a patch" routedFetch({ commitSha: "abcdef1234567890", prNumber: 8 }), ); assert.deepEqual(findings, [ - { file: "src/gone.ts", line: 1, introducedByShaPrefix: "abcdef123456", introducedByPr: 8 }, + { file: "src/gone.ts", line: 1, lastTouchedByShaPrefix: "abcdef123456", lastTouchedByPr: 8 }, ]); }); @@ -77,8 +95,8 @@ test("scanBlameLink: a commit with no associated PR still surfaces the SHA prefi routedFetch({ commitSha: "deadbeefcafebabe", prNumber: null }), ); assert.equal(findings.length, 1); - assert.equal(findings[0].introducedByShaPrefix, "deadbeefcafe"); - assert.equal(findings[0].introducedByPr, undefined); + assert.equal(findings[0].lastTouchedByShaPrefix, "deadbeefcafe"); + assert.equal(findings[0].lastTouchedByPr, undefined); }); test("scanBlameLink: an unresolvable line (no prior commit) yields no finding", async () => { From 83985ffc7aa8f7c01198baf1c47fdb0ee121adc5 Mon Sep 17 00:00:00 2001 From: Nick M <274344962+nickmopen@users.noreply.github.com> Date: Thu, 2 Jul 2026 17:44:14 -0500 Subject: [PATCH 6/7] feat(enrichment): resolve renamed-file history against the old path (#2034) A renamed file lives under its OLD path in the base tree, so blame-link now looks up commit history via `previousPath` for status === "renamed" (falling back to line 1 when a pure rename carries no usable patch), while still displaying the new path to the reviewer. Also trims the rendered section title (was em-dash-heavy and longer than neighboring headings). - Test: a renamed file is probed under its old path and rendered as the new path. review-enrichment: 404/404 pass. --- review-enrichment/src/analyzers/blame-link.ts | 19 ++++++++------ review-enrichment/src/analyzers/registry.ts | 4 +-- review-enrichment/test/blame-link.test.ts | 26 ++++++++++++++++++- 3 files changed, 37 insertions(+), 12 deletions(-) diff --git a/review-enrichment/src/analyzers/blame-link.ts b/review-enrichment/src/analyzers/blame-link.ts index 23c0663205..4adb218d2b 100644 --- a/review-enrichment/src/analyzers/blame-link.ts +++ b/review-enrichment/src/analyzers/blame-link.ts @@ -155,25 +155,28 @@ export async function scanBlameLink( const headers = githubHeaders(githubToken); // Only files that alter pre-existing lines can be blamed: skip added files, generated/binary paths, and pure // additions (a patch with no deletion line has no prior author to attribute). - const candidates: Array<{ path: string; line: number }> = []; + const candidates: Array<{ lookupPath: string; displayPath: string; line: number }> = []; for (const file of files) { if (file.status === "added" || SKIP_RE.test(file.path)) continue; let line = file.patch ? firstTouchedOldLine(file.patch) : null; - // A removed file is entirely a deletion: its path alone drives the history lookup even when the diff carries no - // usable patch (binary/truncated). Anchor to line 1 as the representative point. - if (line === null && file.status === "removed") line = 1; + // A removed OR renamed file resolves against the base tree even without a usable patch (binary/truncated, or a + // pure rename with no content change). Anchor to line 1 as the representative point. + if (line === null && (file.status === "removed" || file.status === "renamed")) line = 1; if (line === null) continue; // a modified file with only additions / no usable patch → nothing to blame - candidates.push({ path: file.path, line }); + // The base tree holds a renamed file under its OLD path, so resolve history against `previousPath` while still + // showing the reviewer the new (display) path. + const lookupPath = file.status === "renamed" && file.previousPath ? file.previousPath : file.path; + candidates.push({ lookupPath, displayPath: file.path, line }); if (candidates.length >= MAX_FILES_PROBED) break; } const findings: BlameLinkFinding[] = []; let lookups = 0; - for (const { path, line } of candidates) { + for (const { lookupPath, displayPath, line } of candidates) { if (options.signal?.aborted) break; if (lookups >= MAX_LOOKUPS) break; // cap reached → emit partial lookups += 1; - const sha = await fetchLatestCommitSha(owner, repo, path, baseSha, headers, fetchFn, options.signal, options); + const sha = await fetchLatestCommitSha(owner, repo, lookupPath, baseSha, headers, fetchFn, options.signal, options); if (!sha) continue; // no prior commit on the path → no finding let lastTouchedByPr: number | null = null; if (lookups < MAX_LOOKUPS && !options.signal?.aborted) { @@ -181,7 +184,7 @@ export async function scanBlameLink( lastTouchedByPr = await fetchPrForCommit(owner, repo, sha, headers, fetchFn, options.signal, options); } findings.push({ - file: path, + file: displayPath, line, lastTouchedByShaPrefix: sha.slice(0, SHA_PREFIX_LEN), ...(lastTouchedByPr !== null ? { lastTouchedByPr } : {}), diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 8efd1bcd90..47a272b5d8 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -461,9 +461,7 @@ export const ANALYZER_DESCRIPTORS = [ }, render: (findings, helpers) => { if (!findings.length) return []; - const lines = [ - "### Recent history of changed files (last PR to touch each — file-level context, bounded scan)", - ]; + const lines = ["### Recent history of changed files (last PR to touch each, file-level)"]; for (const item of findings) { const toucher = item.lastTouchedByPr !== undefined diff --git a/review-enrichment/test/blame-link.test.ts b/review-enrichment/test/blame-link.test.ts index 35c6b01283..885214d160 100644 --- a/review-enrichment/test/blame-link.test.ts +++ b/review-enrichment/test/blame-link.test.ts @@ -66,7 +66,31 @@ test("scanBlameLink: file-level only — it does NOT attribute the last-touch PR ]); // The finding carries no "introducedBy"/origin field for the line — attribution is file-level by construction. assert.equal("introducedByPr" in findings[0], false); - assert.match(renderBrief({ blameLink: findings }).promptSection, /file-level context/i); + const brief = renderBrief({ blameLink: findings }).promptSection; + assert.match(brief, /file-level/i); + assert.doesNotMatch(brief, /introduced/i); // never claims the PR introduced the line +}); + +test("scanBlameLink: a renamed file resolves history against its OLD path, displays the new path", async () => { + let probedPath; + const captureFetch = async (url) => { + if (url.includes("/pulls")) return jsonResponse([{ number: 12 }]); + if (url.includes("/commits?")) { + probedPath = new URL(url).searchParams.get("path"); + return jsonResponse([{ sha: "abcdef1234567890" }]); + } + return jsonResponse([], 404); + }; + const findings = await scanBlameLink( + req( + [{ path: "src/new-name.ts", previousPath: "src/old-name.ts", status: "renamed", patch: "@@ -3,2 +3,2 @@\n keep\n-old\n+new\n" }], + { baseSha: "b" }, + ), + captureFetch, + ); + assert.equal(probedPath, "src/old-name.ts"); // history is looked up under the OLD path (base tree) + assert.equal(findings[0].file, "src/new-name.ts"); // but the reviewer sees the NEW path + assert.equal(findings[0].lastTouchedByPr, 12); }); test("scanBlameLink: reports the OLD-file line on a shifted hunk, and renders it as an old line", async () => { From 170f31f4eac7dd2bb5324aff886266e989de59df Mon Sep 17 00:00:00 2001 From: Nick M <274344962+nickmopen@users.noreply.github.com> Date: Thu, 2 Jul 2026 18:15:50 -0500 Subject: [PATCH 7/7] fix(enrichment): treat every in-hunk `-` line as a deletion, incl. `--`/`---` content (#2034) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous hardening added a `!startsWith("---")` guard to firstTouchedOldLine, but inside a hunk that wrongly skips a real deletion whose CONTENT begins with two hyphens (git renders a deleted `--x` line as `---x`). File headers only appear before a hunk, which the inHunk flag already excludes, so the guard was both wrong and unnecessary — dropped it. Added assertions for `---`/`--`-content deletions. review-enrichment: 404/404 pass. --- review-enrichment/src/analyzers/blame-link.ts | 8 +++++--- review-enrichment/test/blame-link.test.ts | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/review-enrichment/src/analyzers/blame-link.ts b/review-enrichment/src/analyzers/blame-link.ts index 4adb218d2b..c59e34497c 100644 --- a/review-enrichment/src/analyzers/blame-link.ts +++ b/review-enrichment/src/analyzers/blame-link.ts @@ -54,10 +54,12 @@ export function firstTouchedOldLine(patch: string): number | null { inHunk = true; continue; } - if (!inHunk) continue; - if (raw.startsWith("-") && !raw.startsWith("---")) return oldLine; // first modified/deleted old-file line + if (!inHunk) continue; // file headers (`---`/`+++`) only appear before a hunk; the flag skips them + // Inside a hunk EVERY `-`-prefixed line is an old-file deletion — including content that itself starts with + // `--`/`---` (git renders a deleted `--x` line as `---x`). The marker is the first char; the rest is content. + if (raw.startsWith("-")) return oldLine; // first modified/deleted old-file line if (raw.startsWith(" ")) oldLine += 1; // a real context line — the ONLY thing that advances the old cursor - // Everything else (additions, `\`/`+++` markers, malformed text) is not an old-file line: do not advance. + // Everything else (additions, `\`/`+` markers, malformed text) is not an old-file line: do not advance. } return null; } diff --git a/review-enrichment/test/blame-link.test.ts b/review-enrichment/test/blame-link.test.ts index 885214d160..251f553aab 100644 --- a/review-enrichment/test/blame-link.test.ts +++ b/review-enrichment/test/blame-link.test.ts @@ -37,6 +37,10 @@ test("firstTouchedOldLine: reports the first modified/deleted old-file line, nul assert.equal(firstTouchedOldLine("@@ -7,2 +7,1 @@\n keep\n-gone\n\\ No newline at end of file\n"), 8); // Only space-prefixed context advances: a malformed/extended line must NOT be counted as an old-file line. assert.equal(firstTouchedOldLine("@@ -5,2 +5,2 @@\nmalformed no-prefix line\n-x\n"), 5); // not 6 + // Inside a hunk, a deletion whose CONTENT starts with dashes (rendered as `---…`) is still a deletion, not a + // file header — it must be reported, not skipped. + assert.equal(firstTouchedOldLine("@@ -4,2 +4,1 @@\n keep\n---dashes\n"), 5); + assert.equal(firstTouchedOldLine("@@ -8,1 +8,0 @@\n--dash-first\n"), 8); }); test("scanBlameLink: resolves the last PR to touch a modified file", async () => {