From 39bea115fdcef019c2f84866f1bd6cac0b2b903b Mon Sep 17 00:00:00 2001 From: dev-miro26 Date: Mon, 29 Jun 2026 15:33:40 -0700 Subject: [PATCH 1/7] feat(enrichment): cross-file caller-impact / dead-symbol analyzer (#1509) --- .../src/analyzers/caller-impact.ts | 234 +++++++++++++++++ review-enrichment/src/brief.ts | 2 + review-enrichment/src/render.ts | 24 ++ review-enrichment/src/types.ts | 11 + review-enrichment/test/caller-impact.test.ts | 247 ++++++++++++++++++ 5 files changed, 518 insertions(+) create mode 100644 review-enrichment/src/analyzers/caller-impact.ts create mode 100644 review-enrichment/test/caller-impact.test.ts diff --git a/review-enrichment/src/analyzers/caller-impact.ts b/review-enrichment/src/analyzers/caller-impact.ts new file mode 100644 index 0000000000..6ea95b566f --- /dev/null +++ b/review-enrichment/src/analyzers/caller-impact.ts @@ -0,0 +1,234 @@ +// Cross-file caller-impact / dead-symbol analyzer (#1509). Surfaces two cross-file hazards the no-checkout +// `claude --print` reviewer (which only sees the diff) is blind to: +// 1. An exported top-level symbol the PR removes / renames / changes the signature of that STILL has live callers +// in files the PR did NOT touch — a hidden compile/runtime break. Callers are resolved on the repo's default +// branch via the GitHub Code Search API, which is exactly where the pre-existing (about-to-break) callers live. +// 2. A newly-exported symbol referenced nowhere in the PR — dead-on-arrival. Code Search indexes the DEFAULT +// branch only, so a brand-new symbol is invisible to it; this case is therefore judged from the diff (the new +// export is dead if no added line outside its own declaration references it), and entrypoint files (index.*, +// *.d.ts) are skipped because public API is intentionally unused internally. +// +// Reports symbol names + unchanged caller file paths only — never source. Uses the request's short-lived githubToken +// for Code Search; fail-safe: returns [] without a token, and a failed/rate-limited search drops that symbol only. +import type { EnrichRequest, CallerImpactFinding } from "../types.js"; + +const GITHUB_API = "https://api.github.com"; +const GITHUB_API_VERSION = "2022-11-28"; +const MAX_SYMBOLS_SEARCHED = 8; // Code Search is rate-limited (~10/min); bound the per-PR symbol fan-out +const MAX_DEAD_REPORTED = 10; // cap dead-on-arrival findings (diff-only, no network) +const MAX_CALLER_FILES = 10; // cap caller files listed per symbol +const CODE_SEARCH_PER_PAGE = 20; + +const REPO_SEGMENT = /^[A-Za-z0-9._-]+$/; +const ENTRYPOINT_RE = /(^|\/)index\.[cm]?[jt]sx?$|\.d\.ts$/; // public-API files: skip dead-on-arrival here + +interface ScanOptions { + signal?: AbortSignal; +} + +/** Parse `owner/repo`, rejecting anything that isn't exactly two safe segments (no traversal / extra slashes) so a + * hostile `repoFullName` cannot redirect the token-bearing request elsewhere. Returns null when unsafe. */ +export function parseRepo( + repoFullName: string, +): { owner: string; repo: string } | null { + const parts = repoFullName.split("/"); + if (parts.length !== 2) return null; + const [owner, repo] = parts; + for (const seg of [owner, repo]) { + if (!seg || seg === "." || seg === ".." || !REPO_SEGMENT.test(seg)) { + return null; + } + } + return { owner: owner!, repo: repo! }; +} + +function githubHeaders(token: string): Record { + return { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": GITHUB_API_VERSION, + "User-Agent": "gittensory-review-enrichment", + }; +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +/** Exported top-level identifier(s) declared on a single source line. Handles `export function|class|const|let|var| + * interface|type|enum|namespace NAME`, `export default function|class NAME`, and `export { a, b as c }` (the public + * name is the alias after `as`). Returns [] for `export * from …`, anonymous default exports, and non-export lines. */ +export function parseExportedNames(line: string): string[] { + const s = line.trim(); + if (!s.startsWith("export")) return []; + + const brace = s.match(/^export\s+(?:type\s+)?\{([^}]*)\}/); + if (brace) { + return brace[1]! + .split(",") + .map((part) => { + const seg = part.trim(); + const asMatch = seg.match(/\bas\s+([A-Za-z_$][\w$]*)/); + if (asMatch) return asMatch[1]!; + const id = seg.match(/^([A-Za-z_$][\w$]*)/); + return id ? id[1]! : ""; + }) + .filter((name): name is string => name.length > 0 && name !== "default"); + } + + const def = s.match( + /^export\s+default\s+(?:async\s+)?(?:function\*?|class)\s+([A-Za-z_$][\w$]*)/, + ); + if (def) return [def[1]!]; + + const decl = s.match( + /^export\s+(?:declare\s+)?(?:abstract\s+)?(?:async\s+)?(?:function\*?|class|const|let|var|interface|type|enum|namespace)\s+([A-Za-z_$][\w$]*)/, + ); + if (decl) return [decl[1]!]; + + return []; +} + +/** Added ('+') and removed ('-') source lines of a unified-diff patch (markers stripped, hunk headers excluded). */ +function splitPatch(patch: string): { added: string[]; removed: string[] } { + const added: string[] = []; + const removed: string[] = []; + for (const line of patch.split("\n")) { + if (line.startsWith("+") && !line.startsWith("+++")) added.push(line.slice(1)); + else if (line.startsWith("-") && !line.startsWith("---")) removed.push(line.slice(1)); + } + return { added, removed }; +} + +interface DiffExports { + /** name → normalized removed export-declaration text */ + removed: Map; + /** name → normalized added export-declaration text */ + added: Map; + /** every added source line across the PR (for the dead-on-arrival reference scan) */ + addedLines: string[]; + /** newly-exported name → the file it was added in (first seen) */ + addedExportFile: Map; +} + +const norm = (line: string): string => line.trim().replace(/\s+/g, " "); + +/** Collect the PR's exported-symbol churn from every file patch. */ +export function collectDiffExports(files: NonNullable): DiffExports { + const removed = new Map(); + const added = new Map(); + const addedLines: string[] = []; + const addedExportFile = new Map(); + + for (const file of files) { + if (!file.patch) continue; + const { added: addedSrc, removed: removedSrc } = splitPatch(file.patch); + for (const src of removedSrc) { + for (const name of parseExportedNames(src)) removed.set(name, norm(src)); + } + for (const src of addedSrc) { + addedLines.push(src); + for (const name of parseExportedNames(src)) { + added.set(name, norm(src)); + if (!addedExportFile.has(name)) addedExportFile.set(name, file.path); + } + } + } + return { removed, added, addedLines, addedExportFile }; +} + +/** True when the symbol is used in an added line OTHER than its own export declaration (so it is NOT dead). The + * boundaries exclude identifier characters (incl. `$`) so a name is matched whole, never as a substring. */ +export function isReferencedInDiff(symbol: string, addedLines: string[]): boolean { + const re = new RegExp(`(?, + token: string, + fetchImpl: typeof fetch, + signal?: AbortSignal, +): Promise { + try { + const query = `"${symbol}" repo:${owner}/${repo}`; + const url = `${GITHUB_API}/search/code?q=${encodeURIComponent(query)}&per_page=${CODE_SEARCH_PER_PAGE}`; + const res = await fetchImpl(url, { headers: githubHeaders(token), signal }); + if (!res.ok) return null; + const json = (await res.json()) as { items?: Array<{ path?: string }> }; + const files = new Set(); + for (const item of json.items ?? []) { + if (typeof item.path === "string" && !changed.has(item.path)) { + files.add(item.path); + } + } + return [...files].slice(0, MAX_CALLER_FILES); + } catch { + return null; + } +} + +/** Analyzer entrypoint. Flags removed/renamed/changed exports that still have external callers, plus dead-on-arrival + * new exports. Fail-safe: returns [] without a token or changed exports; a failed search drops that symbol only. */ +export async function scanCallerImpact( + req: EnrichRequest, + fetchImpl: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + const token = req.githubToken; + const repo = parseRepo(req.repoFullName); + const files = req.files ?? []; + if (!token || !repo || files.length === 0) return []; + + const { removed, added, addedLines, addedExportFile } = collectDiffExports(files); + if (removed.size === 0 && added.size === 0) return []; + + const changed = new Set(); + for (const file of files) { + changed.add(file.path); + if (file.previousPath) changed.add(file.previousPath); + } + + const findings: CallerImpactFinding[] = []; + + // Removed / renamed / signature-changed exports → look for callers in unchanged files (bounded Code Search budget). + let searched = 0; + for (const [symbol, removedText] of removed) { + if (searched >= MAX_SYMBOLS_SEARCHED) break; + const addedText = added.get(symbol); + // Present on both sides with an IDENTICAL declaration ⇒ moved/reformatted, not a real change ⇒ skip. + if (addedText !== undefined && addedText === removedText) continue; + searched++; + const callerFiles = await searchExternalCallers(symbol, repo.owner, repo.repo, changed, token, fetchImpl, options.signal); + if (!callerFiles || callerFiles.length === 0) continue; + findings.push({ + symbol, + kind: addedText === undefined ? "removed-with-callers" : "changed-with-callers", + callerFiles: callerFiles.sort(), + }); + } + + // Dead-on-arrival: newly-exported symbols (not also removed) referenced nowhere in the diff. Diff-only — Code + // Search can't see a brand-new symbol. Skip public-entrypoint files, whose exports are meant for external use. + let deadReported = 0; + for (const [symbol] of added) { + if (deadReported >= MAX_DEAD_REPORTED) break; + if (removed.has(symbol)) continue; // changed, not new — handled above + const file = addedExportFile.get(symbol) ?? ""; + if (ENTRYPOINT_RE.test(file)) continue; // likely public API + if (isReferencedInDiff(symbol, addedLines)) continue; + deadReported++; + findings.push({ symbol, kind: "dead-on-arrival", callerFiles: [] }); + } + + return findings; +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index ed19645260..2139837e48 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -22,6 +22,7 @@ import { scanSecretLog } from "./analyzers/secret-log.js"; import { scanAssetWeight } from "./analyzers/asset-weight.js"; import { scanTyposquat } from "./analyzers/typosquat.js"; import { scanNativeBuild } from "./analyzers/native-build.js"; +import { scanCallerImpact } from "./analyzers/caller-impact.js"; import { renderBrief } from "./render.js"; import { captureAnalyzerDegradation } from "./sentry.js"; @@ -46,6 +47,7 @@ const ANALYZERS: Record = { assetWeight: (req, signal) => scanAssetWeight(req, fetch, { signal }), typosquat: (req, signal) => scanTyposquat(req, fetch, { signal }), nativeBuild: (req, signal) => scanNativeBuild(req, fetch, { signal }), + callerImpact: (req, signal) => scanCallerImpact(req, fetch, { signal }), }; function runWithTimeout( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 7edcebbdbf..7f90a43e13 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -293,6 +293,30 @@ export function renderBrief( } } + const callerImpact = findings.callerImpact ?? []; + if (callerImpact.length) { + lines.push( + "### Cross-file API impact (callers in unchanged files / dead exports)", + ); + for (const item of callerImpact) { + if (item.kind === "dead-on-arrival") { + lines.push( + `- ${safeCodeSpan(item.symbol)} is exported but referenced nowhere in this PR (dead-on-arrival) — wire it up, or drop the export`, + ); + continue; + } + const verb = + item.kind === "removed-with-callers" + ? "removed/renamed but still referenced in" + : "signature-changed but still referenced in"; + const files = item.callerFiles.map((f) => safeCodeSpan(f)).join(", "); + const count = item.callerFiles.length; + lines.push( + `- ${safeCodeSpan(item.symbol)} ${verb} ${count} unchanged file${count === 1 ? "" : "s"}: ${files} — update the callers or keep a compatibility shim`, + ); + } + } + if (!lines.length) return { promptSection: "", systemSuffix: "" }; const header = diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 639b7d1970..c17a527926 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -193,6 +193,16 @@ export interface NativeBuildFinding { reason: string; } +/** An exported top-level symbol the PR removes / renames / changes the signature of while it still has live callers + * in files the PR did NOT touch (a hidden cross-file compile/runtime break), or a newly-exported symbol referenced + * nowhere in the PR (dead-on-arrival). Reports the symbol name + the unchanged caller files only — never source. (#1509) */ +export interface CallerImpactFinding { + symbol: string; + kind: "removed-with-callers" | "changed-with-callers" | "dead-on-arrival"; + /** Unchanged files (outside the PR's diff) that still reference the symbol. Empty for `dead-on-arrival`. */ + callerFiles: string[]; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -210,6 +220,7 @@ export interface BriefFindings { assetWeight?: AssetWeightFinding[]; typosquat?: TyposquatFinding[]; nativeBuild?: NativeBuildFinding[]; + callerImpact?: CallerImpactFinding[]; } export type AnalyzerStatus = "ok" | "degraded" | "skipped"; diff --git a/review-enrichment/test/caller-impact.test.ts b/review-enrichment/test/caller-impact.test.ts new file mode 100644 index 0000000000..8790b4c513 --- /dev/null +++ b/review-enrichment/test/caller-impact.test.ts @@ -0,0 +1,247 @@ +// Units for the cross-file caller-impact / dead-symbol analyzer (#1509). Kept in its own file (not +// enrichment.test.ts) so concurrent analyzer PRs don't collide on a shared test file. Runs against the compiled dist/. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + parseRepo, + parseExportedNames, + collectDiffExports, + isReferencedInDiff, + scanCallerImpact, +} from "../dist/analyzers/caller-impact.js"; +import { renderBrief } from "../dist/render.js"; + +const res = (body, { ok = true, status = 200 } = {}) => ({ + ok, + status, + json: async () => body, + text: async () => JSON.stringify(body), +}); +const throwingFetch = async () => { + throw new Error("network down"); +}; + +// String substring / RegExp router so each test declares only the endpoints it exercises. +function router(routes) { + return async (url) => { + for (const [match, handler] of routes) { + const hit = typeof match === "string" ? url.includes(match) : match.test(url); + if (hit) return handler; + } + return res({ items: [] }); + }; +} + +// ── pure helpers ──────────────────────────────────────────────────────────────── + +test("parseExportedNames covers the common export forms", () => { + assert.deepEqual(parseExportedNames("export function foo() {}"), ["foo"]); + assert.deepEqual(parseExportedNames("export async function bar() {}"), ["bar"]); + assert.deepEqual(parseExportedNames("export const baz = 1;"), ["baz"]); + assert.deepEqual(parseExportedNames("export class Qux {}"), ["Qux"]); + assert.deepEqual(parseExportedNames("export interface IThing {}"), ["IThing"]); + assert.deepEqual(parseExportedNames("export type TThing = string;"), ["TThing"]); + assert.deepEqual(parseExportedNames("export enum E {}"), ["E"]); + assert.deepEqual(parseExportedNames("export namespace NS {}"), ["NS"]); + assert.deepEqual(parseExportedNames("export declare function dfn(): void;"), ["dfn"]); + assert.deepEqual(parseExportedNames("export default function main() {}"), ["main"]); + assert.deepEqual(parseExportedNames("export default class App {}"), ["App"]); + assert.deepEqual(parseExportedNames(" export function indented() {}"), ["indented"]); +}); + +test("parseExportedNames handles named export lists with aliases", () => { + assert.deepEqual(parseExportedNames("export { a, b as c, d };"), ["a", "c", "d"]); + assert.deepEqual(parseExportedNames("export type { T1, T2 };"), ["T1", "T2"]); + assert.deepEqual(parseExportedNames('export { x } from "./x";'), ["x"]); +}); + +test("parseExportedNames returns [] for re-export-all, anonymous default, and non-exports", () => { + assert.deepEqual(parseExportedNames('export * from "./x";'), []); + assert.deepEqual(parseExportedNames("export default 42;"), []); + assert.deepEqual(parseExportedNames("const x = 1;"), []); + assert.deepEqual(parseExportedNames("import { foo } from './a';"), []); +}); + +test("collectDiffExports splits removed/added exports and added lines", () => { + const out = collectDiffExports([ + { + path: "f.ts", + patch: "@@ -1,1 +1,2 @@\n-export const removed = 1;\n+export const added = 1;\n+useSomething();", + }, + ]); + assert.ok(out.removed.has("removed")); + assert.ok(out.added.has("added")); + assert.equal(out.addedExportFile.get("added"), "f.ts"); + assert.ok(out.addedLines.includes("useSomething();")); +}); + +test("isReferencedInDiff ignores the export declaration and escapes regex metachars", () => { + assert.equal(isReferencedInDiff("added", ["export const added = 1;", "const y = added + 1;"]), true); + assert.equal(isReferencedInDiff("lonely", ["export const lonely = 1;"]), false); + assert.equal(isReferencedInDiff("x$", ["foo(x$);"]), true); // `$` must be escaped, not treated as anchor +}); + +test("parseRepo rejects unsafe names", () => { + assert.deepEqual(parseRepo("o/r"), { owner: "o", repo: "r" }); + assert.equal(parseRepo("o"), null); + assert.equal(parseRepo("o/r/x"), null); + assert.equal(parseRepo("../x"), null); +}); + +// ── scanCallerImpact ────────────────────────────────────────────────────────── + +test("scanCallerImpact: a removed export with external callers is flagged; changed files are excluded", async () => { + const fetchImpl = router([ + ["%22foo%22", res({ items: [{ path: "src/caller.ts" }, { path: "src/lib.ts" }] })], + ]); + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(a: string): void;" }], + }, + fetchImpl, + ); + assert.equal(out.length, 1); + assert.equal(out[0].symbol, "foo"); + assert.equal(out[0].kind, "removed-with-callers"); + assert.deepEqual(out[0].callerFiles, ["src/caller.ts"]); // src/lib.ts (changed) filtered out +}); + +test("scanCallerImpact: a signature change with external callers is flagged as changed-with-callers", async () => { + const fetchImpl = router([ + ["%22bar%22", res({ items: [{ path: "src/caller.ts" }] })], + ]); + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [ + { + path: "src/lib.ts", + patch: "@@ -1,1 +1,1 @@\n-export function bar(a: string): void;\n+export function bar(a: number): void;", + }, + ], + }, + fetchImpl, + ); + assert.equal(out.length, 1); + assert.equal(out[0].kind, "changed-with-callers"); +}); + +test("scanCallerImpact: an identical export on both sides (moved) is not searched or flagged", async () => { + let searched = false; + const tracking = async (url) => { + if (url.includes("/search/code")) searched = true; + return res({ items: [{ path: "src/caller.ts" }] }); + }; + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/lib.ts", patch: "@@ -1,1 +1,1 @@\n-export function baz(): void;\n+export function baz(): void;" }], + }, + tracking, + ); + assert.deepEqual(out, []); + assert.equal(searched, false); +}); + +test("scanCallerImpact: a new export referenced nowhere is dead-on-arrival (no network call)", async () => { + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,1 @@\n+export const newThing = 1;" }], + }, + throwingFetch, // must NOT be called for the dead-on-arrival (diff-only) path + ); + assert.equal(out.length, 1); + assert.equal(out[0].symbol, "newThing"); + assert.equal(out[0].kind, "dead-on-arrival"); + assert.deepEqual(out[0].callerFiles, []); +}); + +test("scanCallerImpact: a new export used elsewhere in the diff is not dead", async () => { + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,2 @@\n+export const used = 1;\n+const x = used + 1;" }], + }, + throwingFetch, + ); + assert.deepEqual(out, []); +}); + +test("scanCallerImpact: a new export from a public entrypoint is not flagged dead", async () => { + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/index.ts", patch: "@@ -0,0 +1,1 @@\n+export const apiThing = 1;" }], + }, + throwingFetch, + ); + assert.deepEqual(out, []); +}); + +test("scanCallerImpact: no token returns [] without any fetch", async () => { + const out = await scanCallerImpact( + { repoFullName: "o/r", prNumber: 1, files: [{ path: "src/lib.ts", patch: "@@ @@\n-export function foo(): void;" }] }, + throwingFetch, + ); + assert.deepEqual(out, []); +}); + +test("scanCallerImpact: a diff with no export churn returns []", async () => { + const out = await scanCallerImpact( + { repoFullName: "o/r", prNumber: 1, githubToken: "t", files: [{ path: "src/lib.ts", patch: "@@ -0,0 +1,1 @@\n+const local = 1;" }] }, + throwingFetch, + ); + assert.deepEqual(out, []); +}); + +test("scanCallerImpact: a rate-limited Code Search drops that symbol without throwing", async () => { + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], + }, + router([["/search/code", res({}, { ok: false, status: 403 })]]), + ); + assert.deepEqual(out, []); +}); + +test("scanCallerImpact: an unsafe repoFullName is rejected before any fetch", async () => { + const out = await scanCallerImpact( + { repoFullName: "o/r/../x", prNumber: 1, githubToken: "t", files: [{ path: "a.ts", patch: "@@ @@\n-export const z = 1;" }] }, + throwingFetch, + ); + assert.deepEqual(out, []); +}); + +// ── render ────────────────────────────────────────────────────────────────────── + +test("renderBrief emits a public-safe caller-impact block", () => { + const { promptSection } = renderBrief({ + callerImpact: [ + { symbol: "foo", kind: "removed-with-callers", callerFiles: ["src/a.ts", "src/b.ts"] }, + { symbol: "bar", kind: "changed-with-callers", callerFiles: ["src/c.ts"] }, + { symbol: "baz", kind: "dead-on-arrival", callerFiles: [] }, + ], + }); + assert.match(promptSection, /Cross-file API impact/); + assert.match(promptSection, /`foo` removed\/renamed but still referenced in 2 unchanged files/); + assert.match(promptSection, /`bar` signature-changed but still referenced in 1 unchanged file\b/); + assert.match(promptSection, /`baz` is exported but referenced nowhere in this PR \(dead-on-arrival\)/); + assert.match(promptSection, /`src\/a\.ts`, `src\/b\.ts`/); +}); From 87a5582553506150d6c0826a103cb0f18175ffd9 Mon Sep 17 00:00:00 2001 From: dev-miro26 Date: Mon, 29 Jun 2026 16:13:01 -0700 Subject: [PATCH 2/7] fix(enrichment): require a real code reference for caller-impact findings (#1509) --- .../src/analyzers/caller-impact.ts | 146 ++++++++++++++---- review-enrichment/test/caller-impact.test.ts | 85 +++++++++- 2 files changed, 196 insertions(+), 35 deletions(-) diff --git a/review-enrichment/src/analyzers/caller-impact.ts b/review-enrichment/src/analyzers/caller-impact.ts index 6ea95b566f..d6bdb30a90 100644 --- a/review-enrichment/src/analyzers/caller-impact.ts +++ b/review-enrichment/src/analyzers/caller-impact.ts @@ -2,14 +2,17 @@ // `claude --print` reviewer (which only sees the diff) is blind to: // 1. An exported top-level symbol the PR removes / renames / changes the signature of that STILL has live callers // in files the PR did NOT touch — a hidden compile/runtime break. Callers are resolved on the repo's default -// branch via the GitHub Code Search API, which is exactly where the pre-existing (about-to-break) callers live. +// branch via the GitHub Code Search API (text-match), which is exactly where the pre-existing (about-to-break) +// callers live. A hit only counts when it is in a CODE file AND the matched fragment uses the symbol as a real +// reference (not a doc/markdown match, comment, or string mention) — Code Search alone is a plain text search. // 2. A newly-exported symbol referenced nowhere in the PR — dead-on-arrival. Code Search indexes the DEFAULT // branch only, so a brand-new symbol is invisible to it; this case is therefore judged from the diff (the new -// export is dead if no added line outside its own declaration references it), and entrypoint files (index.*, -// *.d.ts) are skipped because public API is intentionally unused internally. +// export is dead if no added CODE line — comments/strings excluded — outside its own declaration references it), +// and entrypoint files (index.*, *.d.ts) are skipped because public API is intentionally unused internally. // -// Reports symbol names + unchanged caller file paths only — never source. Uses the request's short-lived githubToken -// for Code Search; fail-safe: returns [] without a token, and a failed/rate-limited search drops that symbol only. +// Reports symbol names + unchanged caller file paths only — never source. The Code-Search path uses the request's +// short-lived githubToken; the diff-only dead-on-arrival path needs neither token nor network. Fail-safe: a failed / +// rate-limited search drops that symbol only. import type { EnrichRequest, CallerImpactFinding } from "../types.js"; const GITHUB_API = "https://api.github.com"; @@ -21,6 +24,8 @@ const CODE_SEARCH_PER_PAGE = 20; const REPO_SEGMENT = /^[A-Za-z0-9._-]+$/; const ENTRYPOINT_RE = /(^|\/)index\.[cm]?[jt]sx?$|\.d\.ts$/; // public-API files: skip dead-on-arrival here +// Only a code file can be a real "caller"; a match in a doc/markdown/text/config file is never a compile/runtime dep. +const CODE_FILE_RE = /\.(?:m?[jt]sx?|cts|mts|vue|svelte)$/i; interface ScanOptions { signal?: AbortSignal; @@ -55,6 +60,36 @@ function escapeRegExp(value: string): string { return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); } +/** Strip line/block comments and string/template-literal CONTENT from a single line, so a symbol that appears only in + * a comment or string is not mistaken for a real code reference. Best-effort single-line scrub (this analyzer is + * advisory): a comment-only line (`//…`, JSDoc `*…`, `/*…`) is dropped entirely. */ +export function stripCommentsAndStrings(line: string): string { + const trimmed = line.trim(); + if ( + trimmed.startsWith("//") || + trimmed.startsWith("*") || + trimmed.startsWith("/*") + ) { + return ""; + } + return line + .replace(/\/\*.*?\*\//g, " ") // inline block comment + .replace(/\/\/.*$/, " ") // trailing line comment + .replace(/"(?:[^"\\]|\\.)*"/g, '""') // double-quoted string content + .replace(/'(?:[^'\\]|\\.)*'/g, "''") // single-quoted string content + .replace(/`(?:[^`\\]|\\.)*`/g, "``"); // template-literal content +} + +/** True when `symbol` appears as a real code reference (a whole identifier in non-comment, non-string code) somewhere + * in `code` — used to confirm a Code Search hit / a diff line is an actual usage, not a doc/comment/string mention. */ +export function referencesSymbol(code: string, symbol: string): boolean { + const re = new RegExp(`(? line.trim().replace(/\s+/g, " "); +/** Parse exported symbol names from a sequence of source lines (diff markers already stripped), joining a multiline + * `export { … }` block that spans several lines into one statement. Returns one entry per export statement. */ +export function extractExports( + lines: string[], +): Array<{ names: string[]; declText: string }> { + const out: Array<{ names: string[]; declText: string }> = []; + for (let i = 0; i < lines.length; i++) { + const line = lines[i]!; + const trimmed = line.trim(); + // A `export {` / `export type {` that does not close its brace on the same line — accumulate until `}` (bounded). + if (/^export\s+(?:type\s+)?\{/.test(trimmed) && !trimmed.includes("}")) { + const parts = [line]; + let j = i + 1; + while (j < lines.length && j - i <= 50) { + parts.push(lines[j]!); + if (lines[j]!.includes("}")) break; + j++; + } + const joined = norm(parts.join(" ")); + const names = parseExportedNames(joined); + if (names.length) out.push({ names, declText: joined }); + i = j; + continue; + } + const names = parseExportedNames(line); + if (names.length) out.push({ names, declText: norm(line) }); + } + return out; +} + /** Collect the PR's exported-symbol churn from every file patch. */ export function collectDiffExports(files: NonNullable): DiffExports { const removed = new Map(); @@ -123,13 +188,13 @@ export function collectDiffExports(files: NonNullable): for (const file of files) { if (!file.patch) continue; const { added: addedSrc, removed: removedSrc } = splitPatch(file.patch); - for (const src of removedSrc) { - for (const name of parseExportedNames(src)) removed.set(name, norm(src)); + for (const { names, declText } of extractExports(removedSrc)) { + for (const name of names) removed.set(name, declText); } - for (const src of addedSrc) { - addedLines.push(src); - for (const name of parseExportedNames(src)) { - added.set(name, norm(src)); + for (const src of addedSrc) addedLines.push(src); + for (const { names, declText } of extractExports(addedSrc)) { + for (const name of names) { + added.set(name, declText); if (!addedExportFile.has(name)) addedExportFile.set(name, file.path); } } @@ -142,9 +207,9 @@ export function collectDiffExports(files: NonNullable): export function isReferencedInDiff(symbol: string, addedLines: string[]): boolean { const re = new RegExp(`(? }; + const json = (await res.json()) as { + items?: Array<{ path?: string; text_matches?: Array<{ fragment?: string }> }>; + }; const files = new Set(); for (const item of json.items ?? []) { - if (typeof item.path === "string" && !changed.has(item.path)) { - files.add(item.path); + const path = item.path; + // A caller must be an UNCHANGED CODE file whose matched fragment uses the symbol as a real reference. + if (typeof path !== "string" || changed.has(path) || !CODE_FILE_RE.test(path)) { + continue; + } + if ((item.text_matches ?? []).some((m) => referencesSymbol(m.fragment ?? "", symbol))) { + files.add(path); } } return [...files].slice(0, MAX_CALLER_FILES); @@ -187,7 +264,7 @@ export async function scanCallerImpact( const token = req.githubToken; const repo = parseRepo(req.repoFullName); const files = req.files ?? []; - if (!token || !repo || files.length === 0) return []; + if (!repo || files.length === 0) return []; const { removed, added, addedLines, addedExportFile } = collectDiffExports(files); if (removed.size === 0 && added.size === 0) return []; @@ -200,21 +277,24 @@ export async function scanCallerImpact( const findings: CallerImpactFinding[] = []; - // Removed / renamed / signature-changed exports → look for callers in unchanged files (bounded Code Search budget). - let searched = 0; - for (const [symbol, removedText] of removed) { - if (searched >= MAX_SYMBOLS_SEARCHED) break; - const addedText = added.get(symbol); - // Present on both sides with an IDENTICAL declaration ⇒ moved/reformatted, not a real change ⇒ skip. - if (addedText !== undefined && addedText === removedText) continue; - searched++; - const callerFiles = await searchExternalCallers(symbol, repo.owner, repo.repo, changed, token, fetchImpl, options.signal); - if (!callerFiles || callerFiles.length === 0) continue; - findings.push({ - symbol, - kind: addedText === undefined ? "removed-with-callers" : "changed-with-callers", - callerFiles: callerFiles.sort(), - }); + // Removed / renamed / signature-changed exports → callers in unchanged files. Needs the token for Code Search; + // skipped without one. Bounded by the Code Search rate budget. + if (token) { + let searched = 0; + for (const [symbol, removedText] of removed) { + if (searched >= MAX_SYMBOLS_SEARCHED) break; + const addedText = added.get(symbol); + // Present on both sides with an IDENTICAL declaration ⇒ moved/reformatted, not a real change ⇒ skip. + if (addedText !== undefined && addedText === removedText) continue; + searched++; + const callerFiles = await searchExternalCallers(symbol, repo.owner, repo.repo, changed, token, fetchImpl, options.signal); + if (!callerFiles || callerFiles.length === 0) continue; + findings.push({ + symbol, + kind: addedText === undefined ? "removed-with-callers" : "changed-with-callers", + callerFiles: callerFiles.sort(), + }); + } } // Dead-on-arrival: newly-exported symbols (not also removed) referenced nowhere in the diff. Diff-only — Code diff --git a/review-enrichment/test/caller-impact.test.ts b/review-enrichment/test/caller-impact.test.ts index 8790b4c513..0783d1d7dc 100644 --- a/review-enrichment/test/caller-impact.test.ts +++ b/review-enrichment/test/caller-impact.test.ts @@ -5,8 +5,10 @@ import assert from "node:assert/strict"; import { parseRepo, parseExportedNames, + extractExports, collectDiffExports, isReferencedInDiff, + referencesSymbol, scanCallerImpact, } from "../dist/analyzers/caller-impact.js"; import { renderBrief } from "../dist/render.js"; @@ -88,11 +90,40 @@ test("parseRepo rejects unsafe names", () => { assert.equal(parseRepo("../x"), null); }); +test("referencesSymbol counts real code references, not comments or strings", () => { + assert.equal(referencesSymbol("import { foo } from './x';", "foo"), true); + assert.equal(referencesSymbol("bar(foo);", "foo"), true); + assert.equal(referencesSymbol("// uses foo here", "foo"), false); // line comment + assert.equal(referencesSymbol(" * @param foo the thing", "foo"), false); // JSDoc continuation + assert.equal(referencesSymbol("const s = 'foo';", "foo"), false); // string literal + assert.equal(referencesSymbol("const t = `foo`;", "foo"), false); // template literal + assert.equal(referencesSymbol("notfoo + foobar", "foo"), false); // substring only +}); + +test("extractExports joins a multiline export { } block", () => { + assert.deepEqual( + extractExports(["export {", " alpha,", " beta,", "};"]).flatMap((e) => e.names), + ["alpha", "beta"], + ); + assert.deepEqual( + extractExports(["export const single = 1;"]).flatMap((e) => e.names), + ["single"], + ); +}); + // ── scanCallerImpact ────────────────────────────────────────────────────────── test("scanCallerImpact: a removed export with external callers is flagged; changed files are excluded", async () => { const fetchImpl = router([ - ["%22foo%22", res({ items: [{ path: "src/caller.ts" }, { path: "src/lib.ts" }] })], + [ + "%22foo%22", + res({ + items: [ + { path: "src/caller.ts", text_matches: [{ fragment: "import { foo } from './lib';\nfoo();" }] }, + { path: "src/lib.ts", text_matches: [{ fragment: "export function foo() {}" }] }, + ], + }), + ], ]); const out = await scanCallerImpact( { @@ -111,7 +142,7 @@ test("scanCallerImpact: a removed export with external callers is flagged; chang test("scanCallerImpact: a signature change with external callers is flagged as changed-with-callers", async () => { const fetchImpl = router([ - ["%22bar%22", res({ items: [{ path: "src/caller.ts" }] })], + ["%22bar%22", res({ items: [{ path: "src/caller.ts", text_matches: [{ fragment: "bar(1);" }] }] })], ]); const out = await scanCallerImpact( { @@ -229,6 +260,56 @@ test("scanCallerImpact: an unsafe repoFullName is rejected before any fetch", as assert.deepEqual(out, []); }); +test("scanCallerImpact: a hit only in a comment, string, or markdown is NOT counted as a caller", async () => { + const fetchImpl = router([ + [ + "%22foo%22", + res({ + items: [ + { path: "src/comment.ts", text_matches: [{ fragment: "// foo is documented here" }] }, + { path: "docs/readme.md", text_matches: [{ fragment: "the foo helper is great" }] }, + { path: "src/strings.ts", text_matches: [{ fragment: "const label = 'foo';" }] }, + ], + }), + ], + ]); + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], + }, + fetchImpl, + ); + assert.deepEqual(out, []); // comment-only, markdown, and string-only matches are not real callers +}); + +test("scanCallerImpact: a comment mention does not suppress dead-on-arrival", async () => { + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,2 @@\n+export const newThing = 1;\n+// TODO wire newThing later" }], + }, + throwingFetch, + ); + assert.equal(out.length, 1); + assert.equal(out[0].kind, "dead-on-arrival"); + assert.equal(out[0].symbol, "newThing"); +}); + +test("scanCallerImpact: dead-on-arrival runs without a token (diff-only, no network)", async () => { + const out = await scanCallerImpact( + { repoFullName: "o/r", prNumber: 1, files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,1 @@\n+export const orphan = 1;" }] }, + throwingFetch, // no token ⇒ no network; must not be called + ); + assert.equal(out.length, 1); + assert.equal(out[0].kind, "dead-on-arrival"); + assert.equal(out[0].symbol, "orphan"); +}); + // ── render ────────────────────────────────────────────────────────────────────── test("renderBrief emits a public-safe caller-impact block", () => { From 61be8c17f122ae3a10a1dbb0e29a07b802d46e9b Mon Sep 17 00:00:00 2001 From: dev-miro26 Date: Mon, 29 Jun 2026 20:52:58 -0700 Subject: [PATCH 3/7] fix(enrichment): paginate caller-impact Code Search past a filtered first page (#1509) --- .../src/analyzers/caller-impact.ts | 62 ++++++++++++------- review-enrichment/test/caller-impact.test.ts | 33 ++++++++++ 2 files changed, 74 insertions(+), 21 deletions(-) diff --git a/review-enrichment/src/analyzers/caller-impact.ts b/review-enrichment/src/analyzers/caller-impact.ts index d6bdb30a90..bd9fb5897e 100644 --- a/review-enrichment/src/analyzers/caller-impact.ts +++ b/review-enrichment/src/analyzers/caller-impact.ts @@ -21,6 +21,8 @@ const MAX_SYMBOLS_SEARCHED = 8; // Code Search is rate-limited (~10/min); bound const MAX_DEAD_REPORTED = 10; // cap dead-on-arrival findings (diff-only, no network) const MAX_CALLER_FILES = 10; // cap caller files listed per symbol const CODE_SEARCH_PER_PAGE = 20; +const MAX_SEARCH_PAGES = 5; // pages one symbol may walk (≤100 hits) to see past filtered noise on page 1 +const MAX_TOTAL_SEARCH_REQUESTS = 10; // global Code Search request budget per review (respects the ~10/min secondary limit) const REPO_SEGMENT = /^[A-Za-z0-9._-]+$/; const ENTRYPOINT_RE = /(^|\/)index\.[cm]?[jt]sx?$|\.d\.ts$/; // public-API files: skip dead-on-arrival here @@ -214,7 +216,11 @@ export function isReferencedInDiff(symbol: string, addedLines: string[]): boolea return false; } -/** Unchanged files (outside `changed`) that reference `symbol` on the default branch, or null on error/non-OK. */ +/** Unchanged CODE files (outside `changed`) whose matched fragment uses `symbol` as a real reference. Walks Code + * Search pages until MAX_CALLER_FILES confirmed callers are found, GitHub reports no more items, the per-symbol page + * cap is hit, or the shared request `budget` is spent — because page 1 can be filled with filtered-out noise (changed + * files, comments, docs) while a real caller sits on a later page. Returns null on a non-OK reply / network error + * (drops this symbol only). A hit with no `text_matches` can't be confirmed, so it is conservatively NOT counted. */ async function searchExternalCallers( symbol: string, owner: string, @@ -222,40 +228,51 @@ async function searchExternalCallers( changed: Set, token: string, fetchImpl: typeof fetch, + budget: { remaining: number }, signal?: AbortSignal, ): Promise { - try { - const query = `"${symbol}" repo:${owner}/${repo}`; - const url = `${GITHUB_API}/search/code?q=${encodeURIComponent(query)}&per_page=${CODE_SEARCH_PER_PAGE}`; - // text-match media type returns the matched fragments, so a hit can be confirmed as a real reference rather than - // a doc/comment/string mention (Code Search itself is a plain text search). - const res = await fetchImpl(url, { - headers: { ...githubHeaders(token), Accept: "application/vnd.github.text-match+json" }, - signal, - }); - if (!res.ok) return null; - const json = (await res.json()) as { + const files = new Set(); + const query = `"${symbol}" repo:${owner}/${repo}`; + for (let page = 1; page <= MAX_SEARCH_PAGES && budget.remaining > 0; page++) { + budget.remaining--; + let json: { + total_count?: number; items?: Array<{ path?: string; text_matches?: Array<{ fragment?: string }> }>; }; - const files = new Set(); - for (const item of json.items ?? []) { + try { + const url = `${GITHUB_API}/search/code?q=${encodeURIComponent(query)}&per_page=${CODE_SEARCH_PER_PAGE}&page=${page}`; + // text-match media type returns the matched fragments, so a hit can be confirmed as a real reference rather + // than a doc/comment/string mention (Code Search itself is a plain text search). + const res = await fetchImpl(url, { + headers: { ...githubHeaders(token), Accept: "application/vnd.github.text-match+json" }, + signal, + }); + if (!res.ok) return null; + json = (await res.json()) as typeof json; + } catch { + return null; + } + const items = json.items ?? []; + for (const item of items) { const path = item.path; - // A caller must be an UNCHANGED CODE file whose matched fragment uses the symbol as a real reference. if (typeof path !== "string" || changed.has(path) || !CODE_FILE_RE.test(path)) { continue; } if ((item.text_matches ?? []).some((m) => referencesSymbol(m.fragment ?? "", symbol))) { files.add(path); + if (files.size >= MAX_CALLER_FILES) return [...files]; } } - return [...files].slice(0, MAX_CALLER_FILES); - } catch { - return null; + // No more results to page through: a short page, or we've covered the reported total. + if (items.length < CODE_SEARCH_PER_PAGE) break; + if (typeof json.total_count === "number" && page * CODE_SEARCH_PER_PAGE >= json.total_count) break; } + return [...files].slice(0, MAX_CALLER_FILES); } /** Analyzer entrypoint. Flags removed/renamed/changed exports that still have external callers, plus dead-on-arrival - * new exports. Fail-safe: returns [] without a token or changed exports; a failed search drops that symbol only. */ + * new exports. The Code-Search caller path needs a token (skipped without one); the diff-only dead-on-arrival path + * runs regardless. Fail-safe: returns [] without a repo or changed exports; a failed search drops that symbol only. */ export async function scanCallerImpact( req: EnrichRequest, fetchImpl: typeof fetch = fetch, @@ -280,14 +297,17 @@ export async function scanCallerImpact( // Removed / renamed / signature-changed exports → callers in unchanged files. Needs the token for Code Search; // skipped without one. Bounded by the Code Search rate budget. if (token) { + // Shared Code Search request budget across all symbols (each may walk several pages), so a common symbol can't + // exhaust the rate budget for the rest. + const searchBudget = { remaining: MAX_TOTAL_SEARCH_REQUESTS }; let searched = 0; for (const [symbol, removedText] of removed) { - if (searched >= MAX_SYMBOLS_SEARCHED) break; + if (searched >= MAX_SYMBOLS_SEARCHED || searchBudget.remaining <= 0) break; const addedText = added.get(symbol); // Present on both sides with an IDENTICAL declaration ⇒ moved/reformatted, not a real change ⇒ skip. if (addedText !== undefined && addedText === removedText) continue; searched++; - const callerFiles = await searchExternalCallers(symbol, repo.owner, repo.repo, changed, token, fetchImpl, options.signal); + const callerFiles = await searchExternalCallers(symbol, repo.owner, repo.repo, changed, token, fetchImpl, searchBudget, options.signal); if (!callerFiles || callerFiles.length === 0) continue; findings.push({ symbol, diff --git a/review-enrichment/test/caller-impact.test.ts b/review-enrichment/test/caller-impact.test.ts index 0783d1d7dc..5181acf52e 100644 --- a/review-enrichment/test/caller-impact.test.ts +++ b/review-enrichment/test/caller-impact.test.ts @@ -252,6 +252,39 @@ test("scanCallerImpact: a rate-limited Code Search drops that symbol without thr assert.deepEqual(out, []); }); +test("scanCallerImpact: pages past a noisy first page to find a real caller on page 2", async () => { + // Page 1 is a FULL page of non-callers (the changed file, a comment-only hit, and markdown docs); the real + // unchanged caller is only on page 2, so the analyzer must paginate to find it. + const page1 = [ + { path: "src/lib.ts", text_matches: [{ fragment: "export function foo() {}" }] }, // changed file → excluded + { path: "src/note.ts", text_matches: [{ fragment: "// foo is nice" }] }, // comment-only → not a reference + { path: "docs/x.md", text_matches: [{ fragment: "the foo helper" }] }, // markdown → excluded + ]; + while (page1.length < 20) page1.push({ path: "docs/pad.md", text_matches: [{ fragment: "foo" }] }); + const fetchImpl = async (url) => { + if (url.includes("&page=1")) return res({ total_count: 21, items: page1 }); + if (url.includes("&page=2")) { + return res({ + total_count: 21, + items: [{ path: "src/real-caller.ts", text_matches: [{ fragment: "import { foo } from './lib';\nfoo();" }] }], + }); + } + return res({ items: [] }); + }; + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], + }, + fetchImpl, + ); + assert.equal(out.length, 1); + assert.equal(out[0].kind, "removed-with-callers"); + assert.deepEqual(out[0].callerFiles, ["src/real-caller.ts"]); +}); + test("scanCallerImpact: an unsafe repoFullName is rejected before any fetch", async () => { const out = await scanCallerImpact( { repoFullName: "o/r/../x", prNumber: 1, githubToken: "t", files: [{ path: "a.ts", patch: "@@ @@\n-export const z = 1;" }] }, From c8410a75f5b8479251d26cd7c7b42b6d3a2fa9f3 Mon Sep 17 00:00:00 2001 From: dev-miro26 Date: Mon, 29 Jun 2026 21:08:27 -0700 Subject: [PATCH 4/7] fix(enrichment): detect multiline export signature changes in caller-impact (#1509) --- .../src/analyzers/caller-impact.ts | 51 +++++++++++-------- review-enrichment/test/caller-impact.test.ts | 51 +++++++++++++++++++ 2 files changed, 80 insertions(+), 22 deletions(-) diff --git a/review-enrichment/src/analyzers/caller-impact.ts b/review-enrichment/src/analyzers/caller-impact.ts index bd9fb5897e..9b32af7b0b 100644 --- a/review-enrichment/src/analyzers/caller-impact.ts +++ b/review-enrichment/src/analyzers/caller-impact.ts @@ -23,6 +23,7 @@ const MAX_CALLER_FILES = 10; // cap caller files listed per symbol const CODE_SEARCH_PER_PAGE = 20; const MAX_SEARCH_PAGES = 5; // pages one symbol may walk (≤100 hits) to see past filtered noise on page 1 const MAX_TOTAL_SEARCH_REQUESTS = 10; // global Code Search request budget per review (respects the ~10/min secondary limit) +const MAX_DECL_LINES = 40; // bound the contiguous multiline export declaration accumulated for signature comparison const REPO_SEGMENT = /^[A-Za-z0-9._-]+$/; const ENTRYPOINT_RE = /(^|\/)index\.[cm]?[jt]sx?$|\.d\.ts$/; // public-API files: skip dead-on-arrival here @@ -150,32 +151,37 @@ interface DiffExports { const norm = (line: string): string => line.trim().replace(/\s+/g, " "); -/** Parse exported symbol names from a sequence of source lines (diff markers already stripped), joining a multiline - * `export { … }` block that spans several lines into one statement. Returns one entry per export statement. */ +/** Inclusive index where the export declaration starting at `start` ends: walk lines until the bracket depth + * (parens/braces/brackets) returns to 0 and the line is not a continuation. This captures the FULL contiguous + * multiline declaration — function parameter lists, interface/object/type bodies, and `export { … }` blocks — so a + * signature change on a LATER line is not mistaken for an identical move/reformat. Bounded by MAX_DECL_LINES. */ +function declarationEnd(lines: string[], start: number): number { + let depth = 0; + for (let i = start; i < lines.length && i - start < MAX_DECL_LINES; i++) { + for (const ch of lines[i]!) { + if (ch === "(" || ch === "{" || ch === "[") depth++; + else if (ch === ")" || ch === "}" || ch === "]") depth = Math.max(0, depth - 1); + } + // Complete when nothing is left open and the line does not end on a continuation operator (`=`, `|`, `&`, `,`, `(`, `<`). + if (depth === 0 && !/[=|&,(<]\s*$/.test(lines[i]!)) return i; + } + return Math.min(start + MAX_DECL_LINES - 1, lines.length - 1); +} + +/** Parse exported symbol names from a sequence of source lines (diff markers already stripped). Each multiline export + * declaration is joined into one statement so the FULL declaration text (not just its first line) is compared for + * signature changes. Returns one entry per export statement. */ export function extractExports( lines: string[], ): Array<{ names: string[]; declText: string }> { const out: Array<{ names: string[]; declText: string }> = []; for (let i = 0; i < lines.length; i++) { - const line = lines[i]!; - const trimmed = line.trim(); - // A `export {` / `export type {` that does not close its brace on the same line — accumulate until `}` (bounded). - if (/^export\s+(?:type\s+)?\{/.test(trimmed) && !trimmed.includes("}")) { - const parts = [line]; - let j = i + 1; - while (j < lines.length && j - i <= 50) { - parts.push(lines[j]!); - if (lines[j]!.includes("}")) break; - j++; - } - const joined = norm(parts.join(" ")); - const names = parseExportedNames(joined); - if (names.length) out.push({ names, declText: joined }); - i = j; - continue; - } - const names = parseExportedNames(line); - if (names.length) out.push({ names, declText: norm(line) }); + if (!lines[i]!.trim().startsWith("export")) continue; + const end = declarationEnd(lines, i); + const joined = norm(lines.slice(i, end + 1).join(" ")); + const names = parseExportedNames(joined); + if (names.length) out.push({ names, declText: joined }); + i = end; } return out; } @@ -330,5 +336,6 @@ export async function scanCallerImpact( findings.push({ symbol, kind: "dead-on-arrival", callerFiles: [] }); } - return findings; + // Stable order (by kind, then symbol) so the rendered brief is deterministic regardless of Code Search result order. + return findings.sort((a, b) => a.kind.localeCompare(b.kind) || a.symbol.localeCompare(b.symbol)); } diff --git a/review-enrichment/test/caller-impact.test.ts b/review-enrichment/test/caller-impact.test.ts index 5181acf52e..bfa9f9a4cb 100644 --- a/review-enrichment/test/caller-impact.test.ts +++ b/review-enrichment/test/caller-impact.test.ts @@ -111,6 +111,15 @@ test("extractExports joins a multiline export { } block", () => { ); }); +test("extractExports captures a multiline function signature for change comparison", () => { + const before = extractExports(["export function foo(", " a: string,", "): void;"]); + const after = extractExports(["export function foo(", " a: number,", "): void;"]); + assert.deepEqual(before.flatMap((e) => e.names), ["foo"]); + assert.deepEqual(after.flatMap((e) => e.names), ["foo"]); + // Same name, but the FULL declaration text differs ⇒ a real signature change, not an identical move/reformat. + assert.notEqual(before[0].declText, after[0].declText); +}); + // ── scanCallerImpact ────────────────────────────────────────────────────────── test("scanCallerImpact: a removed export with external callers is flagged; changed files are excluded", async () => { @@ -285,6 +294,48 @@ test("scanCallerImpact: pages past a noisy first page to find a real caller on p assert.deepEqual(out[0].callerFiles, ["src/real-caller.ts"]); }); +test("scanCallerImpact: a multiline signature change is detected as changed-with-callers", async () => { + const fetchImpl = router([ + ["%22foo%22", res({ items: [{ path: "src/caller.ts", text_matches: [{ fragment: "foo(1);" }] }] })], + ]); + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [ + { + path: "src/lib.ts", + // Whole declaration block removed + re-added; only a parameter line (not the first line) changes. + patch: + "@@ -1,3 +1,3 @@\n-export function foo(\n- a: string,\n-): void;\n+export function foo(\n+ a: number,\n+): void;", + }, + ], + }, + fetchImpl, + ); + assert.equal(out.length, 1); + assert.equal(out[0].symbol, "foo"); + assert.equal(out[0].kind, "changed-with-callers"); +}); + +test("scanCallerImpact: a re-export does not suppress dead-on-arrival", async () => { + // fileA adds the export; a barrel only re-exports it (API forwarding) — that is not real implementation use. + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [ + { path: "src/a.ts", patch: "@@ -0,0 +1,1 @@\n+export const orphan = 1;" }, + { path: "src/barrel.ts", patch: "@@ -0,0 +1,1 @@\n+export { orphan } from './a';" }, + ], + }, + throwingFetch, // dead-on-arrival is diff-only + ); + assert.ok(out.some((f) => f.symbol === "orphan" && f.kind === "dead-on-arrival")); +}); + test("scanCallerImpact: an unsafe repoFullName is rejected before any fetch", async () => { const out = await scanCallerImpact( { repoFullName: "o/r/../x", prNumber: 1, githubToken: "t", files: [{ path: "a.ts", patch: "@@ @@\n-export const z = 1;" }] }, From f1424741f45c23993b3b87e745e6778ed76939de Mon Sep 17 00:00:00 2001 From: dev-miro26 Date: Mon, 29 Jun 2026 21:26:45 -0700 Subject: [PATCH 5/7] fix(enrichment): bind caller-impact to real imports and full-patch export churn (#1509) --- .../src/analyzers/caller-impact.ts | 342 ++++++++++++----- review-enrichment/test/caller-impact.test.ts | 355 +++++++++++------- 2 files changed, 472 insertions(+), 225 deletions(-) diff --git a/review-enrichment/src/analyzers/caller-impact.ts b/review-enrichment/src/analyzers/caller-impact.ts index 9b32af7b0b..901e09be26 100644 --- a/review-enrichment/src/analyzers/caller-impact.ts +++ b/review-enrichment/src/analyzers/caller-impact.ts @@ -1,18 +1,19 @@ // Cross-file caller-impact / dead-symbol analyzer (#1509). Surfaces two cross-file hazards the no-checkout // `claude --print` reviewer (which only sees the diff) is blind to: // 1. An exported top-level symbol the PR removes / renames / changes the signature of that STILL has live callers -// in files the PR did NOT touch — a hidden compile/runtime break. Callers are resolved on the repo's default -// branch via the GitHub Code Search API (text-match), which is exactly where the pre-existing (about-to-break) -// callers live. A hit only counts when it is in a CODE file AND the matched fragment uses the symbol as a real -// reference (not a doc/markdown match, comment, or string mention) — Code Search alone is a plain text search. -// 2. A newly-exported symbol referenced nowhere in the PR — dead-on-arrival. Code Search indexes the DEFAULT -// branch only, so a brand-new symbol is invisible to it; this case is therefore judged from the diff (the new -// export is dead if no added CODE line — comments/strings excluded — outside its own declaration references it), -// and entrypoint files (index.*, *.d.ts) are skipped because public API is intentionally unused internally. +// in files the PR did NOT touch — a hidden compile/runtime break. Candidate callers come from the GitHub Code +// Search API (text-match) on the default branch; each candidate's CONTENT is then fetched and a finding is only +// reported when that file actually IMPORTS the symbol FROM the changed module — so a file that merely defines or +// uses its own identically-named symbol is never falsely flagged. +// 2. A newly-exported symbol referenced nowhere in the PR — dead-on-arrival. Code Search indexes the DEFAULT branch +// only, so a brand-new symbol is invisible to it; this is judged from the diff (the new export is dead if no +// added CODE line — comments/strings excluded — outside its own declaration references it), and entrypoint files +// (index.*, *.d.ts) are skipped because public API is intentionally unused internally. // -// Reports symbol names + unchanged caller file paths only — never source. The Code-Search path uses the request's -// short-lived githubToken; the diff-only dead-on-arrival path needs neither token nor network. Fail-safe: a failed / -// rate-limited search drops that symbol only. +// Export churn is read from the FULL patch (context + removed → pre-image; context + added → post-image) so a change +// confined to a parameter line of a multiline signature is still seen even when the `export …` line is unchanged +// context. Reports symbol names + unchanged caller file paths only — never source. Fail-safe: a failed / rate-limited +// lookup drops that symbol only; a candidate whose content can't be verified is dropped. import type { EnrichRequest, CallerImpactFinding } from "../types.js"; const GITHUB_API = "https://api.github.com"; @@ -20,15 +21,18 @@ const GITHUB_API_VERSION = "2022-11-28"; const MAX_SYMBOLS_SEARCHED = 8; // Code Search is rate-limited (~10/min); bound the per-PR symbol fan-out const MAX_DEAD_REPORTED = 10; // cap dead-on-arrival findings (diff-only, no network) const MAX_CALLER_FILES = 10; // cap caller files listed per symbol +const MAX_CALLER_CANDIDATES = 20; // candidate code files (per symbol) to import-verify before stopping const CODE_SEARCH_PER_PAGE = 20; const MAX_SEARCH_PAGES = 5; // pages one symbol may walk (≤100 hits) to see past filtered noise on page 1 -const MAX_TOTAL_SEARCH_REQUESTS = 10; // global Code Search request budget per review (respects the ~10/min secondary limit) +const MAX_TOTAL_SEARCH_REQUESTS = 10; // global Code Search request budget (respects the ~10/min secondary limit) +const MAX_TOTAL_CONTENT_FETCHES = 30; // global Contents-API budget for import verification across all symbols const MAX_DECL_LINES = 40; // bound the contiguous multiline export declaration accumulated for signature comparison const REPO_SEGMENT = /^[A-Za-z0-9._-]+$/; const ENTRYPOINT_RE = /(^|\/)index\.[cm]?[jt]sx?$|\.d\.ts$/; // public-API files: skip dead-on-arrival here // Only a code file can be a real "caller"; a match in a doc/markdown/text/config file is never a compile/runtime dep. const CODE_FILE_RE = /\.(?:m?[jt]sx?|cts|mts|vue|svelte)$/i; +const MODULE_EXT_RE = /\.(?:d\.ts|[cm]?[jt]sx?|cts|mts|vue|svelte)$/i; interface ScanOptions { signal?: AbortSignal; @@ -63,6 +67,36 @@ function escapeRegExp(value: string): string { return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); } +/** A whole-identifier matcher for `symbol` — boundaries exclude identifier characters (incl. `$`). */ +function identifier(symbol: string): RegExp { + return new RegExp(`(?; - /** name → normalized added export-declaration text */ - added: Map; - /** every added source line across the PR (for the dead-on-arrival reference scan) */ - addedLines: string[]; - /** newly-exported name → the file it was added in (first seen) */ - addedExportFile: Map; -} - const norm = (line: string): string => line.trim().replace(/\s+/g, " "); /** Inclusive index where the export declaration starting at `start` ends: walk lines until the bracket depth @@ -168,9 +218,9 @@ function declarationEnd(lines: string[], start: number): number { return Math.min(start + MAX_DECL_LINES - 1, lines.length - 1); } -/** Parse exported symbol names from a sequence of source lines (diff markers already stripped). Each multiline export - * declaration is joined into one statement so the FULL declaration text (not just its first line) is compared for - * signature changes. Returns one entry per export statement. */ +/** Parse exported symbol names from a sequence of source lines. Each multiline export declaration is joined into one + * statement so the FULL declaration text (not just its first line) is compared for signature changes. Returns one + * entry per export statement. */ export function extractExports( lines: string[], ): Array<{ names: string[]; declText: string }> { @@ -186,48 +236,119 @@ export function extractExports( return out; } -/** Collect the PR's exported-symbol churn from every file patch. */ -export function collectDiffExports(files: NonNullable): DiffExports { - const removed = new Map(); - const added = new Map(); +/** A unified-diff patch split into its pre-image (context + removed) and post-image (context + added), plus the + * purely-added lines. Comparing exports parsed from the pre- vs post-image catches a change confined to an inner line + * of a multiline declaration whose `export …` line is unchanged context. */ +function splitPatchImages(patch: string): { + pre: string[]; + post: string[]; + added: string[]; +} { + const pre: string[] = []; + const post: string[] = []; + const added: string[] = []; + for (const line of patch.split("\n")) { + if (line.startsWith("@@") || line.startsWith("+++") || line.startsWith("---")) { + continue; + } + if (line.startsWith("+")) { + const text = line.slice(1); + post.push(text); + added.push(text); + } else if (line.startsWith("-")) { + pre.push(line.slice(1)); + } else { + const ctx = line.startsWith(" ") ? line.slice(1) : line; + pre.push(ctx); + post.push(ctx); + } + } + return { pre, post, added }; +} + +interface DiffExports { + /** exported name → pre-image declaration text (context + removed lines) */ + oldExports: Map; + /** exported name → post-image declaration text (context + added lines) */ + newExports: Map; + /** old exported name → the file it was declared in */ + oldExportFile: Map; + /** new exported name → the file it was declared in */ + newExportFile: Map; + /** purely-added source lines across the PR (for the dead-on-arrival reference scan) */ + addedLines: string[]; +} + +/** Collect the PR's exported-symbol churn from every file patch, reconstructing each file's pre- and post-image. */ +export function collectDiffExports( + files: NonNullable, +): DiffExports { + const oldExports = new Map(); + const newExports = new Map(); + const oldExportFile = new Map(); + const newExportFile = new Map(); const addedLines: string[] = []; - const addedExportFile = new Map(); for (const file of files) { if (!file.patch) continue; - const { added: addedSrc, removed: removedSrc } = splitPatch(file.patch); - for (const { names, declText } of extractExports(removedSrc)) { - for (const name of names) removed.set(name, declText); + const { pre, post, added } = splitPatchImages(file.patch); + for (const src of added) addedLines.push(src); + for (const { names, declText } of extractExports(pre)) { + for (const name of names) { + oldExports.set(name, declText); + if (!oldExportFile.has(name)) oldExportFile.set(name, file.path); + } } - for (const src of addedSrc) addedLines.push(src); - for (const { names, declText } of extractExports(addedSrc)) { + for (const { names, declText } of extractExports(post)) { for (const name of names) { - added.set(name, declText); - if (!addedExportFile.has(name)) addedExportFile.set(name, file.path); + newExports.set(name, declText); + if (!newExportFile.has(name)) newExportFile.set(name, file.path); } } } - return { removed, added, addedLines, addedExportFile }; + return { oldExports, newExports, oldExportFile, newExportFile, addedLines }; } -/** True when the symbol is used in an added line OTHER than its own export declaration (so it is NOT dead). The - * boundaries exclude identifier characters (incl. `$`) so a name is matched whole, never as a substring. */ +/** True when the symbol is used in an added line OTHER than its own export declaration (so it is NOT dead). A mention + * only in a comment or string is not a real reference. */ export function isReferencedInDiff(symbol: string, addedLines: string[]): boolean { - const re = new RegExp(`(? { + try { + const encodedPath = path + .split("/") + .map((seg) => encodeURIComponent(seg)) + .join("/"); + const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encodedPath}`; + const res = await fetchImpl(url, { + headers: { ...githubHeaders(token), Accept: "application/vnd.github.raw" }, + signal, + }); + if (!res.ok) return null; + return await res.text(); + } catch { + return null; + } +} + +/** Candidate unchanged CODE files (outside `changed`) whose matched fragment references `symbol`, walking Code Search + * pages past filtered noise. Returns null on a non-OK reply / network error (drops this symbol only). */ +async function searchCallerCandidates( symbol: string, owner: string, repo: string, @@ -237,7 +358,8 @@ async function searchExternalCallers( budget: { remaining: number }, signal?: AbortSignal, ): Promise { - const files = new Set(); + const candidates: string[] = []; + const seen = new Set(); const query = `"${symbol}" repo:${owner}/${repo}`; for (let page = 1; page <= MAX_SEARCH_PAGES && budget.remaining > 0; page++) { budget.remaining--; @@ -247,8 +369,8 @@ async function searchExternalCallers( }; try { const url = `${GITHUB_API}/search/code?q=${encodeURIComponent(query)}&per_page=${CODE_SEARCH_PER_PAGE}&page=${page}`; - // text-match media type returns the matched fragments, so a hit can be confirmed as a real reference rather - // than a doc/comment/string mention (Code Search itself is a plain text search). + // text-match media type returns the matched fragments, so a hit can be pre-filtered to real references rather + // than doc/comment/string mentions (Code Search itself is a plain text search). const res = await fetchImpl(url, { headers: { ...githubHeaders(token), Accept: "application/vnd.github.text-match+json" }, signal, @@ -261,24 +383,52 @@ async function searchExternalCallers( const items = json.items ?? []; for (const item of items) { const path = item.path; - if (typeof path !== "string" || changed.has(path) || !CODE_FILE_RE.test(path)) { + if (typeof path !== "string" || changed.has(path) || !CODE_FILE_RE.test(path) || seen.has(path)) { continue; } if ((item.text_matches ?? []).some((m) => referencesSymbol(m.fragment ?? "", symbol))) { - files.add(path); - if (files.size >= MAX_CALLER_FILES) return [...files]; + seen.add(path); + candidates.push(path); + if (candidates.length >= MAX_CALLER_CANDIDATES) return candidates; } } - // No more results to page through: a short page, or we've covered the reported total. - if (items.length < CODE_SEARCH_PER_PAGE) break; + if (items.length < CODE_SEARCH_PER_PAGE) break; // last page if (typeof json.total_count === "number" && page * CODE_SEARCH_PER_PAGE >= json.total_count) break; } - return [...files].slice(0, MAX_CALLER_FILES); + return candidates; +} + +/** Unchanged files that IMPORT `symbol` from `moduleName` (the changed module). Code Search surfaces candidates by + * text; each candidate's content is then fetched and import-verified so a same-named symbol in an unrelated module is + * never reported. Returns null only when the Code Search itself failed (drops this symbol). */ +async function findExternalCallers( + symbol: string, + owner: string, + repo: string, + changed: Set, + moduleName: string, + token: string, + fetchImpl: typeof fetch, + searchBudget: { remaining: number }, + contentBudget: { remaining: number }, + signal?: AbortSignal, +): Promise { + const candidates = await searchCallerCandidates(symbol, owner, repo, changed, token, fetchImpl, searchBudget, signal); + if (candidates === null) return null; + const callers: string[] = []; + for (const path of candidates) { + if (callers.length >= MAX_CALLER_FILES || contentBudget.remaining <= 0) break; + contentBudget.remaining--; + const content = await fetchFileContent(owner, repo, path, token, fetchImpl, signal); + if (content === null) continue; // can't verify the import → drop (conservative) + if (importsSymbolFromModule(content, symbol, moduleName)) callers.push(path); + } + return callers.sort(); } -/** Analyzer entrypoint. Flags removed/renamed/changed exports that still have external callers, plus dead-on-arrival - * new exports. The Code-Search caller path needs a token (skipped without one); the diff-only dead-on-arrival path - * runs regardless. Fail-safe: returns [] without a repo or changed exports; a failed search drops that symbol only. */ +/** Analyzer entrypoint. Flags removed/renamed/changed exports that still have importing callers in unchanged files, + * plus dead-on-arrival new exports. The caller path needs a token (skipped without one); the diff-only dead-on-arrival + * path runs regardless. Fail-safe: returns [] without a repo or export churn; a failed lookup drops that symbol only. */ export async function scanCallerImpact( req: EnrichRequest, fetchImpl: typeof fetch = fetch, @@ -289,8 +439,8 @@ export async function scanCallerImpact( const files = req.files ?? []; if (!repo || files.length === 0) return []; - const { removed, added, addedLines, addedExportFile } = collectDiffExports(files); - if (removed.size === 0 && added.size === 0) return []; + const { oldExports, newExports, oldExportFile, newExportFile, addedLines } = collectDiffExports(files); + if (oldExports.size === 0 && newExports.size === 0) return []; const changed = new Set(); for (const file of files) { @@ -300,36 +450,36 @@ export async function scanCallerImpact( const findings: CallerImpactFinding[] = []; - // Removed / renamed / signature-changed exports → callers in unchanged files. Needs the token for Code Search; - // skipped without one. Bounded by the Code Search rate budget. + // Removed / renamed / signature-changed exports → importing callers in unchanged files. Needs the token; skipped + // without one. Bounded by the shared Code Search + Contents budgets. if (token) { - // Shared Code Search request budget across all symbols (each may walk several pages), so a common symbol can't - // exhaust the rate budget for the rest. const searchBudget = { remaining: MAX_TOTAL_SEARCH_REQUESTS }; + const contentBudget = { remaining: MAX_TOTAL_CONTENT_FETCHES }; let searched = 0; - for (const [symbol, removedText] of removed) { + for (const [symbol, oldText] of oldExports) { if (searched >= MAX_SYMBOLS_SEARCHED || searchBudget.remaining <= 0) break; - const addedText = added.get(symbol); - // Present on both sides with an IDENTICAL declaration ⇒ moved/reformatted, not a real change ⇒ skip. - if (addedText !== undefined && addedText === removedText) continue; + const newText = newExports.get(symbol); + if (newText !== undefined && newText === oldText) continue; // unchanged ⇒ skip + const moduleName = moduleBasename(oldExportFile.get(symbol) ?? ""); + if (!moduleName) continue; searched++; - const callerFiles = await searchExternalCallers(symbol, repo.owner, repo.repo, changed, token, fetchImpl, searchBudget, options.signal); + const callerFiles = await findExternalCallers(symbol, repo.owner, repo.repo, changed, moduleName, token, fetchImpl, searchBudget, contentBudget, options.signal); if (!callerFiles || callerFiles.length === 0) continue; findings.push({ symbol, - kind: addedText === undefined ? "removed-with-callers" : "changed-with-callers", - callerFiles: callerFiles.sort(), + kind: newText === undefined ? "removed-with-callers" : "changed-with-callers", + callerFiles, }); } } - // Dead-on-arrival: newly-exported symbols (not also removed) referenced nowhere in the diff. Diff-only — Code + // Dead-on-arrival: newly-exported symbols (not also present before) referenced nowhere in the diff. Diff-only — Code // Search can't see a brand-new symbol. Skip public-entrypoint files, whose exports are meant for external use. let deadReported = 0; - for (const [symbol] of added) { + for (const [symbol] of newExports) { if (deadReported >= MAX_DEAD_REPORTED) break; - if (removed.has(symbol)) continue; // changed, not new — handled above - const file = addedExportFile.get(symbol) ?? ""; + if (oldExports.has(symbol)) continue; // changed, not new — handled above + const file = newExportFile.get(symbol) ?? ""; if (ENTRYPOINT_RE.test(file)) continue; // likely public API if (isReferencedInDiff(symbol, addedLines)) continue; deadReported++; diff --git a/review-enrichment/test/caller-impact.test.ts b/review-enrichment/test/caller-impact.test.ts index bfa9f9a4cb..722a27fbf3 100644 --- a/review-enrichment/test/caller-impact.test.ts +++ b/review-enrichment/test/caller-impact.test.ts @@ -9,6 +9,8 @@ import { collectDiffExports, isReferencedInDiff, referencesSymbol, + moduleBasename, + importsSymbolFromModule, scanCallerImpact, } from "../dist/analyzers/caller-impact.js"; import { renderBrief } from "../dist/render.js"; @@ -19,16 +21,30 @@ const res = (body, { ok = true, status = 200 } = {}) => ({ json: async () => body, text: async () => JSON.stringify(body), }); +const raw = (text, { ok = true, status = 200 } = {}) => ({ + ok, + status, + json: async () => ({}), + text: async () => text, +}); const throwingFetch = async () => { throw new Error("network down"); }; -// String substring / RegExp router so each test declares only the endpoints it exercises. -function router(routes) { +// A fetch stub routing /search/code (by encoded-symbol substring → items) and /contents/ (→ raw file content). +function ghStub({ search = {}, contents = {} }) { return async (url) => { - for (const [match, handler] of routes) { - const hit = typeof match === "string" ? url.includes(match) : match.test(url); - if (hit) return handler; + if (url.includes("/search/code")) { + for (const [enc, items] of Object.entries(search)) { + if (url.includes(enc)) return res({ items }); + } + return res({ items: [] }); + } + if (url.includes("/contents/")) { + for (const [path, content] of Object.entries(contents)) { + if (url.includes("/contents/" + path)) return raw(content); + } + return raw("", { ok: false, status: 404 }); } return res({ items: [] }); }; @@ -51,6 +67,12 @@ test("parseExportedNames covers the common export forms", () => { assert.deepEqual(parseExportedNames(" export function indented() {}"), ["indented"]); }); +test("parseExportedNames enumerates every declarator and handles const enum", () => { + assert.deepEqual(parseExportedNames("export const a = 1, b = 2;"), ["a", "b"]); + assert.deepEqual(parseExportedNames("export let x = f(1, 2), y = 3;"), ["x", "y"]); // comma inside () is not a split + assert.deepEqual(parseExportedNames("export const enum Color { Red }"), ["Color"]); +}); + test("parseExportedNames handles named export lists with aliases", () => { assert.deepEqual(parseExportedNames("export { a, b as c, d };"), ["a", "c", "d"]); assert.deepEqual(parseExportedNames("export type { T1, T2 };"), ["T1", "T2"]); @@ -64,16 +86,31 @@ test("parseExportedNames returns [] for re-export-all, anonymous default, and no assert.deepEqual(parseExportedNames("import { foo } from './a';"), []); }); -test("collectDiffExports splits removed/added exports and added lines", () => { +test("moduleBasename strips directories and extensions", () => { + assert.equal(moduleBasename("src/lib.ts"), "lib"); + assert.equal(moduleBasename("./lib"), "lib"); + assert.equal(moduleBasename("../utils/lib.js"), "lib"); + assert.equal(moduleBasename("@scope/pkg/feature.d.ts"), "feature"); +}); + +test("importsSymbolFromModule binds to a named import from the matching module only", () => { + assert.equal(importsSymbolFromModule("import { foo } from './lib';", "foo", "lib"), true); + assert.equal(importsSymbolFromModule("import { foo as bar } from '../lib.js';", "foo", "lib"), true); + assert.equal(importsSymbolFromModule("import { other } from './lib';", "foo", "lib"), false); // different symbol + assert.equal(importsSymbolFromModule("import { foo } from './other';", "foo", "lib"), false); // different module + assert.equal(importsSymbolFromModule("function foo() {}\nfoo();", "foo", "lib"), false); // own symbol, no import +}); + +test("collectDiffExports reconstructs pre/post export images and added lines", () => { const out = collectDiffExports([ { path: "f.ts", patch: "@@ -1,1 +1,2 @@\n-export const removed = 1;\n+export const added = 1;\n+useSomething();", }, ]); - assert.ok(out.removed.has("removed")); - assert.ok(out.added.has("added")); - assert.equal(out.addedExportFile.get("added"), "f.ts"); + assert.ok(out.oldExports.has("removed")); + assert.ok(out.newExports.has("added")); + assert.equal(out.newExportFile.get("added"), "f.ts"); assert.ok(out.addedLines.includes("useSomething();")); }); @@ -122,18 +159,16 @@ test("extractExports captures a multiline function signature for change comparis // ── scanCallerImpact ────────────────────────────────────────────────────────── -test("scanCallerImpact: a removed export with external callers is flagged; changed files are excluded", async () => { - const fetchImpl = router([ - [ - "%22foo%22", - res({ - items: [ - { path: "src/caller.ts", text_matches: [{ fragment: "import { foo } from './lib';\nfoo();" }] }, - { path: "src/lib.ts", text_matches: [{ fragment: "export function foo() {}" }] }, - ], - }), - ], - ]); +test("scanCallerImpact: a removed export with an importing caller is flagged; changed files are excluded", async () => { + const fetchImpl = ghStub({ + search: { + "%22foo%22": [ + { path: "src/caller.ts", text_matches: [{ fragment: "import { foo } from './lib';\nfoo();" }] }, + { path: "src/lib.ts", text_matches: [{ fragment: "export function foo() {}" }] }, // changed file → excluded + ], + }, + contents: { "src/caller.ts": "import { foo } from './lib';\nfoo();" }, + }); const out = await scanCallerImpact( { repoFullName: "o/r", @@ -146,13 +181,40 @@ test("scanCallerImpact: a removed export with external callers is flagged; chang assert.equal(out.length, 1); assert.equal(out[0].symbol, "foo"); assert.equal(out[0].kind, "removed-with-callers"); - assert.deepEqual(out[0].callerFiles, ["src/caller.ts"]); // src/lib.ts (changed) filtered out + assert.deepEqual(out[0].callerFiles, ["src/caller.ts"]); }); -test("scanCallerImpact: a signature change with external callers is flagged as changed-with-callers", async () => { - const fetchImpl = router([ - ["%22bar%22", res({ items: [{ path: "src/caller.ts", text_matches: [{ fragment: "bar(1);" }] }] })], - ]); +test("scanCallerImpact: an unrelated file defining its OWN same-named symbol is not a caller", async () => { + const fetchImpl = ghStub({ + search: { + "%22foo%22": [ + { path: "src/real.ts", text_matches: [{ fragment: "foo();" }] }, + { path: "src/unrelated.ts", text_matches: [{ fragment: "foo();" }] }, + ], + }, + contents: { + "src/real.ts": "import { foo } from './lib';\nfoo();", // real consumer of the removed module + "src/unrelated.ts": "function foo() {}\nfoo();", // its OWN foo — not the removed one + }, + }); + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], + }, + fetchImpl, + ); + assert.equal(out.length, 1); + assert.deepEqual(out[0].callerFiles, ["src/real.ts"]); // unrelated.ts excluded by import binding +}); + +test("scanCallerImpact: a signature change with an importing caller is changed-with-callers", async () => { + const fetchImpl = ghStub({ + search: { "%22bar%22": [{ path: "src/caller.ts", text_matches: [{ fragment: "bar(1);" }] }] }, + contents: { "src/caller.ts": "import { bar } from './lib';\nbar(1);" }, + }); const out = await scanCallerImpact( { repoFullName: "o/r", @@ -171,11 +233,60 @@ test("scanCallerImpact: a signature change with external callers is flagged as c assert.equal(out[0].kind, "changed-with-callers"); }); +test("scanCallerImpact: a change confined to a parameter line (export line is context) is detected", async () => { + const fetchImpl = ghStub({ + search: { "%22foo%22": [{ path: "src/caller.ts", text_matches: [{ fragment: "foo(1);" }] }] }, + contents: { "src/caller.ts": "import { foo } from './lib';\nfoo(1);" }, + }); + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [ + { + path: "src/lib.ts", + // `export function foo(` and `): void;` are unchanged CONTEXT; only the parameter line is -/+. + patch: "@@ -1,3 +1,3 @@\n export function foo(\n- a: string,\n+ a: number,\n ): void;", + }, + ], + }, + fetchImpl, + ); + assert.equal(out.length, 1); + assert.equal(out[0].symbol, "foo"); + assert.equal(out[0].kind, "changed-with-callers"); +}); + +test("scanCallerImpact: a multiline signature change (whole block re-added) is changed-with-callers", async () => { + const fetchImpl = ghStub({ + search: { "%22foo%22": [{ path: "src/caller.ts", text_matches: [{ fragment: "foo(1);" }] }] }, + contents: { "src/caller.ts": "import { foo } from './lib';\nfoo(1);" }, + }); + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [ + { + path: "src/lib.ts", + patch: + "@@ -1,3 +1,3 @@\n-export function foo(\n- a: string,\n-): void;\n+export function foo(\n+ a: number,\n+): void;", + }, + ], + }, + fetchImpl, + ); + assert.equal(out.length, 1); + assert.equal(out[0].kind, "changed-with-callers"); +}); + test("scanCallerImpact: an identical export on both sides (moved) is not searched or flagged", async () => { let searched = false; const tracking = async (url) => { if (url.includes("/search/code")) searched = true; - return res({ items: [{ path: "src/caller.ts" }] }); + return res({ items: [] }); }; const out = await scanCallerImpact( { @@ -190,137 +301,134 @@ test("scanCallerImpact: an identical export on both sides (moved) is not searche assert.equal(searched, false); }); -test("scanCallerImpact: a new export referenced nowhere is dead-on-arrival (no network call)", async () => { +test("scanCallerImpact: pages past a noisy first page to find an importing caller on page 2", async () => { + const page1 = [ + { path: "src/lib.ts", text_matches: [{ fragment: "export function foo() {}" }] }, // changed file → excluded + { path: "src/note.ts", text_matches: [{ fragment: "// foo is nice" }] }, // comment-only → not a reference + { path: "docs/x.md", text_matches: [{ fragment: "the foo helper" }] }, // markdown → excluded + ]; + while (page1.length < 20) page1.push({ path: "docs/pad.md", text_matches: [{ fragment: "foo" }] }); + const fetchImpl = async (url) => { + if (url.includes("/contents/src/real-caller.ts")) return raw("import { foo } from './lib';\nfoo();"); + if (url.includes("/search/code")) { + if (url.includes("&page=1")) return res({ total_count: 21, items: page1 }); + if (url.includes("&page=2")) { + return res({ + total_count: 21, + items: [{ path: "src/real-caller.ts", text_matches: [{ fragment: "import { foo } from './lib';\nfoo();" }] }], + }); + } + } + return res({ items: [] }); + }; const out = await scanCallerImpact( { repoFullName: "o/r", prNumber: 1, githubToken: "t", - files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,1 @@\n+export const newThing = 1;" }], + files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], }, - throwingFetch, // must NOT be called for the dead-on-arrival (diff-only) path + fetchImpl, ); assert.equal(out.length, 1); - assert.equal(out[0].symbol, "newThing"); - assert.equal(out[0].kind, "dead-on-arrival"); - assert.deepEqual(out[0].callerFiles, []); + assert.equal(out[0].kind, "removed-with-callers"); + assert.deepEqual(out[0].callerFiles, ["src/real-caller.ts"]); }); -test("scanCallerImpact: a new export used elsewhere in the diff is not dead", async () => { +test("scanCallerImpact: a hit only in a comment, string, or markdown is not a caller", async () => { + const fetchImpl = ghStub({ + search: { + "%22foo%22": [ + { path: "src/comment.ts", text_matches: [{ fragment: "// foo is documented here" }] }, + { path: "docs/readme.md", text_matches: [{ fragment: "the foo helper is great" }] }, + { path: "src/strings.ts", text_matches: [{ fragment: "const label = 'foo';" }] }, + ], + }, + }); const out = await scanCallerImpact( { repoFullName: "o/r", prNumber: 1, githubToken: "t", - files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,2 @@\n+export const used = 1;\n+const x = used + 1;" }], + files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], }, - throwingFetch, + fetchImpl, ); assert.deepEqual(out, []); }); -test("scanCallerImpact: a new export from a public entrypoint is not flagged dead", async () => { +test("scanCallerImpact: every declarator in a multi-declarator export is tracked (dead-on-arrival)", async () => { const out = await scanCallerImpact( { repoFullName: "o/r", prNumber: 1, githubToken: "t", - files: [{ path: "src/index.ts", patch: "@@ -0,0 +1,1 @@\n+export const apiThing = 1;" }], + files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,1 @@\n+export const a = 1, b = 2;" }], }, throwingFetch, ); - assert.deepEqual(out, []); -}); - -test("scanCallerImpact: no token returns [] without any fetch", async () => { - const out = await scanCallerImpact( - { repoFullName: "o/r", prNumber: 1, files: [{ path: "src/lib.ts", patch: "@@ @@\n-export function foo(): void;" }] }, - throwingFetch, - ); - assert.deepEqual(out, []); + const dead = out.filter((f) => f.kind === "dead-on-arrival").map((f) => f.symbol).sort(); + assert.deepEqual(dead, ["a", "b"]); }); -test("scanCallerImpact: a diff with no export churn returns []", async () => { +test("scanCallerImpact: a new export referenced nowhere is dead-on-arrival (no network call)", async () => { const out = await scanCallerImpact( - { repoFullName: "o/r", prNumber: 1, githubToken: "t", files: [{ path: "src/lib.ts", patch: "@@ -0,0 +1,1 @@\n+const local = 1;" }] }, + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,1 @@\n+export const newThing = 1;" }], + }, throwingFetch, ); - assert.deepEqual(out, []); + assert.equal(out.length, 1); + assert.equal(out[0].symbol, "newThing"); + assert.equal(out[0].kind, "dead-on-arrival"); + assert.deepEqual(out[0].callerFiles, []); }); -test("scanCallerImpact: a rate-limited Code Search drops that symbol without throwing", async () => { +test("scanCallerImpact: a new export used elsewhere in the diff is not dead", async () => { const out = await scanCallerImpact( { repoFullName: "o/r", prNumber: 1, githubToken: "t", - files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], + files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,2 @@\n+export const used = 1;\n+const x = used + 1;" }], }, - router([["/search/code", res({}, { ok: false, status: 403 })]]), + throwingFetch, ); assert.deepEqual(out, []); }); -test("scanCallerImpact: pages past a noisy first page to find a real caller on page 2", async () => { - // Page 1 is a FULL page of non-callers (the changed file, a comment-only hit, and markdown docs); the real - // unchanged caller is only on page 2, so the analyzer must paginate to find it. - const page1 = [ - { path: "src/lib.ts", text_matches: [{ fragment: "export function foo() {}" }] }, // changed file → excluded - { path: "src/note.ts", text_matches: [{ fragment: "// foo is nice" }] }, // comment-only → not a reference - { path: "docs/x.md", text_matches: [{ fragment: "the foo helper" }] }, // markdown → excluded - ]; - while (page1.length < 20) page1.push({ path: "docs/pad.md", text_matches: [{ fragment: "foo" }] }); - const fetchImpl = async (url) => { - if (url.includes("&page=1")) return res({ total_count: 21, items: page1 }); - if (url.includes("&page=2")) { - return res({ - total_count: 21, - items: [{ path: "src/real-caller.ts", text_matches: [{ fragment: "import { foo } from './lib';\nfoo();" }] }], - }); - } - return res({ items: [] }); - }; +test("scanCallerImpact: a comment mention does not suppress dead-on-arrival", async () => { const out = await scanCallerImpact( { repoFullName: "o/r", prNumber: 1, githubToken: "t", - files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], + files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,2 @@\n+export const newThing = 1;\n+// TODO wire newThing later" }], }, - fetchImpl, + throwingFetch, ); assert.equal(out.length, 1); - assert.equal(out[0].kind, "removed-with-callers"); - assert.deepEqual(out[0].callerFiles, ["src/real-caller.ts"]); + assert.equal(out[0].kind, "dead-on-arrival"); + assert.equal(out[0].symbol, "newThing"); }); -test("scanCallerImpact: a multiline signature change is detected as changed-with-callers", async () => { - const fetchImpl = router([ - ["%22foo%22", res({ items: [{ path: "src/caller.ts", text_matches: [{ fragment: "foo(1);" }] }] })], - ]); +test("scanCallerImpact: a new export from a public entrypoint is not flagged dead", async () => { const out = await scanCallerImpact( { repoFullName: "o/r", prNumber: 1, githubToken: "t", - files: [ - { - path: "src/lib.ts", - // Whole declaration block removed + re-added; only a parameter line (not the first line) changes. - patch: - "@@ -1,3 +1,3 @@\n-export function foo(\n- a: string,\n-): void;\n+export function foo(\n+ a: number,\n+): void;", - }, - ], + files: [{ path: "src/index.ts", patch: "@@ -0,0 +1,1 @@\n+export const apiThing = 1;" }], }, - fetchImpl, + throwingFetch, ); - assert.equal(out.length, 1); - assert.equal(out[0].symbol, "foo"); - assert.equal(out[0].kind, "changed-with-callers"); + assert.deepEqual(out, []); }); test("scanCallerImpact: a re-export does not suppress dead-on-arrival", async () => { - // fileA adds the export; a barrel only re-exports it (API forwarding) — that is not real implementation use. const out = await scanCallerImpact( { repoFullName: "o/r", @@ -331,67 +439,56 @@ test("scanCallerImpact: a re-export does not suppress dead-on-arrival", async () { path: "src/barrel.ts", patch: "@@ -0,0 +1,1 @@\n+export { orphan } from './a';" }, ], }, - throwingFetch, // dead-on-arrival is diff-only + throwingFetch, ); assert.ok(out.some((f) => f.symbol === "orphan" && f.kind === "dead-on-arrival")); }); -test("scanCallerImpact: an unsafe repoFullName is rejected before any fetch", async () => { +test("scanCallerImpact: dead-on-arrival runs without a token (diff-only, no network)", async () => { const out = await scanCallerImpact( - { repoFullName: "o/r/../x", prNumber: 1, githubToken: "t", files: [{ path: "a.ts", patch: "@@ @@\n-export const z = 1;" }] }, + { repoFullName: "o/r", prNumber: 1, files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,1 @@\n+export const orphan = 1;" }] }, + throwingFetch, // no token ⇒ no network; must not be called + ); + assert.equal(out.length, 1); + assert.equal(out[0].kind, "dead-on-arrival"); + assert.equal(out[0].symbol, "orphan"); +}); + +test("scanCallerImpact: no token returns [] without any fetch", async () => { + const out = await scanCallerImpact( + { repoFullName: "o/r", prNumber: 1, files: [{ path: "src/lib.ts", patch: "@@ @@\n-export function foo(): void;" }] }, throwingFetch, ); assert.deepEqual(out, []); }); -test("scanCallerImpact: a hit only in a comment, string, or markdown is NOT counted as a caller", async () => { - const fetchImpl = router([ - [ - "%22foo%22", - res({ - items: [ - { path: "src/comment.ts", text_matches: [{ fragment: "// foo is documented here" }] }, - { path: "docs/readme.md", text_matches: [{ fragment: "the foo helper is great" }] }, - { path: "src/strings.ts", text_matches: [{ fragment: "const label = 'foo';" }] }, - ], - }), - ], - ]); +test("scanCallerImpact: a diff with no export churn returns []", async () => { const out = await scanCallerImpact( - { - repoFullName: "o/r", - prNumber: 1, - githubToken: "t", - files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], - }, - fetchImpl, + { repoFullName: "o/r", prNumber: 1, githubToken: "t", files: [{ path: "src/lib.ts", patch: "@@ -0,0 +1,1 @@\n+const local = 1;" }] }, + throwingFetch, ); - assert.deepEqual(out, []); // comment-only, markdown, and string-only matches are not real callers + assert.deepEqual(out, []); }); -test("scanCallerImpact: a comment mention does not suppress dead-on-arrival", async () => { +test("scanCallerImpact: a rate-limited Code Search drops that symbol without throwing", async () => { const out = await scanCallerImpact( { repoFullName: "o/r", prNumber: 1, githubToken: "t", - files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,2 @@\n+export const newThing = 1;\n+// TODO wire newThing later" }], + files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], }, - throwingFetch, + async (url) => (url.includes("/search/code") ? res({}, { ok: false, status: 403 }) : res({ items: [] })), ); - assert.equal(out.length, 1); - assert.equal(out[0].kind, "dead-on-arrival"); - assert.equal(out[0].symbol, "newThing"); + assert.deepEqual(out, []); }); -test("scanCallerImpact: dead-on-arrival runs without a token (diff-only, no network)", async () => { +test("scanCallerImpact: an unsafe repoFullName is rejected before any fetch", async () => { const out = await scanCallerImpact( - { repoFullName: "o/r", prNumber: 1, files: [{ path: "src/util.ts", patch: "@@ -0,0 +1,1 @@\n+export const orphan = 1;" }] }, - throwingFetch, // no token ⇒ no network; must not be called + { repoFullName: "o/r/../x", prNumber: 1, githubToken: "t", files: [{ path: "a.ts", patch: "@@ @@\n-export const z = 1;" }] }, + throwingFetch, ); - assert.equal(out.length, 1); - assert.equal(out[0].kind, "dead-on-arrival"); - assert.equal(out[0].symbol, "orphan"); + assert.deepEqual(out, []); }); // ── render ────────────────────────────────────────────────────────────────────── From 4165c2e68800c962e75e64a6ddc5c138a8a4560f Mon Sep 17 00:00:00 2001 From: dev-miro26 Date: Mon, 29 Jun 2026 21:42:53 -0700 Subject: [PATCH 6/7] fix(enrichment): per-file export keying and resolved-import caller binding (#1509) --- .../src/analyzers/caller-impact.ts | 181 +++++++++++------- review-enrichment/test/caller-impact.test.ts | 128 +++++++++++-- 2 files changed, 223 insertions(+), 86 deletions(-) diff --git a/review-enrichment/src/analyzers/caller-impact.ts b/review-enrichment/src/analyzers/caller-impact.ts index 901e09be26..7962a154a9 100644 --- a/review-enrichment/src/analyzers/caller-impact.ts +++ b/review-enrichment/src/analyzers/caller-impact.ts @@ -1,19 +1,20 @@ // Cross-file caller-impact / dead-symbol analyzer (#1509). Surfaces two cross-file hazards the no-checkout // `claude --print` reviewer (which only sees the diff) is blind to: -// 1. An exported top-level symbol the PR removes / renames / changes the signature of that STILL has live callers -// in files the PR did NOT touch — a hidden compile/runtime break. Candidate callers come from the GitHub Code +// 1. An exported top-level symbol the PR removes / renames / changes the signature of that STILL has dependents in +// files the PR did NOT touch — a hidden compile/runtime break. Candidate dependents come from the GitHub Code // Search API (text-match) on the default branch; each candidate's CONTENT is then fetched and a finding is only -// reported when that file actually IMPORTS the symbol FROM the changed module — so a file that merely defines or -// uses its own identically-named symbol is never falsely flagged. +// reported when that file actually DEPENDS ON the symbol FROM the changed module — a named import, a namespace +// import used as `ns.symbol`, an `export { symbol } from`, or an `export * from` barrel — where the import +// specifier RESOLVES (relative to the candidate file) to the changed file's path. So a same-named export in an +// unrelated module, or an import of a different `./lib`, is never falsely flagged. Export churn is keyed per +// (file, name), so two changed files exporting the same name never overwrite each other. // 2. A newly-exported symbol referenced nowhere in the PR — dead-on-arrival. Code Search indexes the DEFAULT branch -// only, so a brand-new symbol is invisible to it; this is judged from the diff (the new export is dead if no -// added CODE line — comments/strings excluded — outside its own declaration references it), and entrypoint files +// only, so a brand-new symbol is invisible to it; this is judged from the diff, and entrypoint files // (index.*, *.d.ts) are skipped because public API is intentionally unused internally. // -// Export churn is read from the FULL patch (context + removed → pre-image; context + added → post-image) so a change -// confined to a parameter line of a multiline signature is still seen even when the `export …` line is unchanged -// context. Reports symbol names + unchanged caller file paths only — never source. Fail-safe: a failed / rate-limited -// lookup drops that symbol only; a candidate whose content can't be verified is dropped. +// Reports symbol names + unchanged dependent file paths only — never source. The caller path uses the request's +// short-lived githubToken; the diff-only dead-on-arrival path needs neither token nor network. Fail-safe: a failed / +// rate-limited lookup drops that symbol only; a candidate whose content/import can't be verified is dropped. import type { EnrichRequest, CallerImpactFinding } from "../types.js"; const GITHUB_API = "https://api.github.com"; @@ -30,7 +31,7 @@ const MAX_DECL_LINES = 40; // bound the contiguous multiline export declaration const REPO_SEGMENT = /^[A-Za-z0-9._-]+$/; const ENTRYPOINT_RE = /(^|\/)index\.[cm]?[jt]sx?$|\.d\.ts$/; // public-API files: skip dead-on-arrival here -// Only a code file can be a real "caller"; a match in a doc/markdown/text/config file is never a compile/runtime dep. +// Only a code file can be a real dependent; a match in a doc/markdown/text/config file is never a compile/runtime dep. const CODE_FILE_RE = /\.(?:m?[jt]sx?|cts|mts|vue|svelte)$/i; const MODULE_EXT_RE = /\.(?:d\.ts|[cm]?[jt]sx?|cts|mts|vue|svelte)$/i; @@ -72,10 +73,25 @@ function identifier(symbol: string): RegExp { return new RegExp(`(?` is a dependent only if it actually uses `ns.symbol`. + const usage = new RegExp( + `(?` barrel re-exports every symbol, incl. this one + } } return false; } @@ -202,9 +235,8 @@ export function parseExportedNames(line: string): string[] { const norm = (line: string): string => line.trim().replace(/\s+/g, " "); /** Inclusive index where the export declaration starting at `start` ends: walk lines until the bracket depth - * (parens/braces/brackets) returns to 0 and the line is not a continuation. This captures the FULL contiguous - * multiline declaration — function parameter lists, interface/object/type bodies, and `export { … }` blocks — so a - * signature change on a LATER line is not mistaken for an identical move/reformat. Bounded by MAX_DECL_LINES. */ + * (parens/braces/brackets) returns to 0 and the line is not a continuation. Captures the FULL contiguous multiline + * declaration so a signature change on a LATER line is not mistaken for an identical move. Bounded by MAX_DECL_LINES. */ function declarationEnd(lines: string[], start: number): number { let depth = 0; for (let i = start; i < lines.length && i - start < MAX_DECL_LINES; i++) { @@ -212,15 +244,13 @@ function declarationEnd(lines: string[], start: number): number { if (ch === "(" || ch === "{" || ch === "[") depth++; else if (ch === ")" || ch === "}" || ch === "]") depth = Math.max(0, depth - 1); } - // Complete when nothing is left open and the line does not end on a continuation operator (`=`, `|`, `&`, `,`, `(`, `<`). if (depth === 0 && !/[=|&,(<]\s*$/.test(lines[i]!)) return i; } return Math.min(start + MAX_DECL_LINES - 1, lines.length - 1); } /** Parse exported symbol names from a sequence of source lines. Each multiline export declaration is joined into one - * statement so the FULL declaration text (not just its first line) is compared for signature changes. Returns one - * entry per export statement. */ + * statement so the FULL declaration text (not just its first line) is compared for signature changes. */ export function extractExports( lines: string[], ): Array<{ names: string[]; declText: string }> { @@ -266,26 +296,33 @@ function splitPatchImages(patch: string): { return { pre, post, added }; } +/** One exported symbol declared in one file's pre- or post-image. */ +interface ExportEntry { + file: string; + name: string; + declText: string; +} + interface DiffExports { - /** exported name → pre-image declaration text (context + removed lines) */ - oldExports: Map; - /** exported name → post-image declaration text (context + added lines) */ - newExports: Map; - /** old exported name → the file it was declared in */ - oldExportFile: Map; - /** new exported name → the file it was declared in */ + /** every export in the pre-image, keyed per (file, name) */ + oldEntries: ExportEntry[]; + /** every export in the post-image, keyed per (file, name) */ + newEntries: ExportEntry[]; + /** the set of names exported anywhere before the PR (so a genuinely-new name can be told from a move) */ + oldNames: Set; + /** newly-exported name → the file it was first declared in (for the dead-on-arrival entrypoint check) */ newExportFile: Map; /** purely-added source lines across the PR (for the dead-on-arrival reference scan) */ addedLines: string[]; } -/** Collect the PR's exported-symbol churn from every file patch, reconstructing each file's pre- and post-image. */ +/** Collect the PR's exported-symbol churn PER FILE, reconstructing each file's pre- and post-image. */ export function collectDiffExports( files: NonNullable, ): DiffExports { - const oldExports = new Map(); - const newExports = new Map(); - const oldExportFile = new Map(); + const oldEntries: ExportEntry[] = []; + const newEntries: ExportEntry[] = []; + const oldNames = new Set(); const newExportFile = new Map(); const addedLines: string[] = []; @@ -295,18 +332,18 @@ export function collectDiffExports( for (const src of added) addedLines.push(src); for (const { names, declText } of extractExports(pre)) { for (const name of names) { - oldExports.set(name, declText); - if (!oldExportFile.has(name)) oldExportFile.set(name, file.path); + oldEntries.push({ file: file.path, name, declText }); + oldNames.add(name); } } for (const { names, declText } of extractExports(post)) { for (const name of names) { - newExports.set(name, declText); + newEntries.push({ file: file.path, name, declText }); if (!newExportFile.has(name)) newExportFile.set(name, file.path); } } } - return { oldExports, newExports, oldExportFile, newExportFile, addedLines }; + return { oldEntries, newEntries, oldNames, newExportFile, addedLines }; } /** True when the symbol is used in an added line OTHER than its own export declaration (so it is NOT dead). A mention @@ -369,8 +406,6 @@ async function searchCallerCandidates( }; try { const url = `${GITHUB_API}/search/code?q=${encodeURIComponent(query)}&per_page=${CODE_SEARCH_PER_PAGE}&page=${page}`; - // text-match media type returns the matched fragments, so a hit can be pre-filtered to real references rather - // than doc/comment/string mentions (Code Search itself is a plain text search). const res = await fetchImpl(url, { headers: { ...githubHeaders(token), Accept: "application/vnd.github.text-match+json" }, signal, @@ -398,15 +433,15 @@ async function searchCallerCandidates( return candidates; } -/** Unchanged files that IMPORT `symbol` from `moduleName` (the changed module). Code Search surfaces candidates by - * text; each candidate's content is then fetched and import-verified so a same-named symbol in an unrelated module is - * never reported. Returns null only when the Code Search itself failed (drops this symbol). */ +/** Unchanged files that DEPEND ON `symbol` from the changed module at `changedPath`. Code Search surfaces candidates + * by text; each candidate's content is then fetched and import-verified (relative-path resolved) so a same-named + * symbol in an unrelated module is never reported. Returns null only when the Code Search itself failed. */ async function findExternalCallers( symbol: string, owner: string, repo: string, changed: Set, - moduleName: string, + changedPath: string, token: string, fetchImpl: typeof fetch, searchBudget: { remaining: number }, @@ -421,12 +456,12 @@ async function findExternalCallers( contentBudget.remaining--; const content = await fetchFileContent(owner, repo, path, token, fetchImpl, signal); if (content === null) continue; // can't verify the import → drop (conservative) - if (importsSymbolFromModule(content, symbol, moduleName)) callers.push(path); + if (importsSymbolFromModule(content, symbol, path, changedPath)) callers.push(path); } return callers.sort(); } -/** Analyzer entrypoint. Flags removed/renamed/changed exports that still have importing callers in unchanged files, +/** Analyzer entrypoint. Flags removed/renamed/changed exports that still have importing dependents in unchanged files, * plus dead-on-arrival new exports. The caller path needs a token (skipped without one); the diff-only dead-on-arrival * path runs regardless. Fail-safe: returns [] without a repo or export churn; a failed lookup drops that symbol only. */ export async function scanCallerImpact( @@ -439,8 +474,8 @@ export async function scanCallerImpact( const files = req.files ?? []; if (!repo || files.length === 0) return []; - const { oldExports, newExports, oldExportFile, newExportFile, addedLines } = collectDiffExports(files); - if (oldExports.size === 0 && newExports.size === 0) return []; + const { oldEntries, newEntries, oldNames, newExportFile, addedLines } = collectDiffExports(files); + if (oldEntries.length === 0 && newEntries.length === 0) return []; const changed = new Set(); for (const file of files) { @@ -448,42 +483,46 @@ export async function scanCallerImpact( if (file.previousPath) changed.add(file.previousPath); } + // Per-(file, name) post-image declaration text, so a same-named export in another file can't be conflated. + const newByKey = new Map(); + for (const entry of newEntries) newByKey.set(`${entry.file}${entry.name}`, entry.declText); + const findings: CallerImpactFinding[] = []; - // Removed / renamed / signature-changed exports → importing callers in unchanged files. Needs the token; skipped + // Removed / renamed / signature-changed exports → importing dependents in unchanged files. Needs the token; skipped // without one. Bounded by the shared Code Search + Contents budgets. if (token) { const searchBudget = { remaining: MAX_TOTAL_SEARCH_REQUESTS }; const contentBudget = { remaining: MAX_TOTAL_CONTENT_FETCHES }; let searched = 0; - for (const [symbol, oldText] of oldExports) { + for (const entry of oldEntries) { if (searched >= MAX_SYMBOLS_SEARCHED || searchBudget.remaining <= 0) break; - const newText = newExports.get(symbol); - if (newText !== undefined && newText === oldText) continue; // unchanged ⇒ skip - const moduleName = moduleBasename(oldExportFile.get(symbol) ?? ""); - if (!moduleName) continue; + const newText = newByKey.get(`${entry.file}${entry.name}`); + if (newText !== undefined && newText === entry.declText) continue; // unchanged in this file ⇒ skip searched++; - const callerFiles = await findExternalCallers(symbol, repo.owner, repo.repo, changed, moduleName, token, fetchImpl, searchBudget, contentBudget, options.signal); + const callerFiles = await findExternalCallers(entry.name, repo.owner, repo.repo, changed, entry.file, token, fetchImpl, searchBudget, contentBudget, options.signal); if (!callerFiles || callerFiles.length === 0) continue; findings.push({ - symbol, + symbol: entry.name, kind: newText === undefined ? "removed-with-callers" : "changed-with-callers", callerFiles, }); } } - // Dead-on-arrival: newly-exported symbols (not also present before) referenced nowhere in the diff. Diff-only — Code - // Search can't see a brand-new symbol. Skip public-entrypoint files, whose exports are meant for external use. + // Dead-on-arrival: genuinely-new exported symbols (not exported anywhere before) referenced nowhere in the diff. + // Diff-only — Code Search can't see a brand-new symbol. Skip public-entrypoint files (exports meant for external use). let deadReported = 0; - for (const [symbol] of newExports) { + const deadSeen = new Set(); + for (const entry of newEntries) { if (deadReported >= MAX_DEAD_REPORTED) break; - if (oldExports.has(symbol)) continue; // changed, not new — handled above - const file = newExportFile.get(symbol) ?? ""; + if (oldNames.has(entry.name) || deadSeen.has(entry.name)) continue; // existed before, or already reported + const file = newExportFile.get(entry.name) ?? entry.file; if (ENTRYPOINT_RE.test(file)) continue; // likely public API - if (isReferencedInDiff(symbol, addedLines)) continue; + if (isReferencedInDiff(entry.name, addedLines)) continue; + deadSeen.add(entry.name); deadReported++; - findings.push({ symbol, kind: "dead-on-arrival", callerFiles: [] }); + findings.push({ symbol: entry.name, kind: "dead-on-arrival", callerFiles: [] }); } // Stable order (by kind, then symbol) so the rendered brief is deterministic regardless of Code Search result order. diff --git a/review-enrichment/test/caller-impact.test.ts b/review-enrichment/test/caller-impact.test.ts index 722a27fbf3..a484902834 100644 --- a/review-enrichment/test/caller-impact.test.ts +++ b/review-enrichment/test/caller-impact.test.ts @@ -9,7 +9,8 @@ import { collectDiffExports, isReferencedInDiff, referencesSymbol, - moduleBasename, + normalizeModulePath, + resolveImport, importsSymbolFromModule, scanCallerImpact, } from "../dist/analyzers/caller-impact.js"; @@ -86,30 +87,41 @@ test("parseExportedNames returns [] for re-export-all, anonymous default, and no assert.deepEqual(parseExportedNames("import { foo } from './a';"), []); }); -test("moduleBasename strips directories and extensions", () => { - assert.equal(moduleBasename("src/lib.ts"), "lib"); - assert.equal(moduleBasename("./lib"), "lib"); - assert.equal(moduleBasename("../utils/lib.js"), "lib"); - assert.equal(moduleBasename("@scope/pkg/feature.d.ts"), "feature"); +test("normalizeModulePath strips extension and trailing index", () => { + assert.equal(normalizeModulePath("src/lib.ts"), "src/lib"); + assert.equal(normalizeModulePath("src/lib/index.ts"), "src/lib"); + assert.equal(normalizeModulePath("a/b/c.d.ts"), "a/b/c"); }); -test("importsSymbolFromModule binds to a named import from the matching module only", () => { - assert.equal(importsSymbolFromModule("import { foo } from './lib';", "foo", "lib"), true); - assert.equal(importsSymbolFromModule("import { foo as bar } from '../lib.js';", "foo", "lib"), true); - assert.equal(importsSymbolFromModule("import { other } from './lib';", "foo", "lib"), false); // different symbol - assert.equal(importsSymbolFromModule("import { foo } from './other';", "foo", "lib"), false); // different module - assert.equal(importsSymbolFromModule("function foo() {}\nfoo();", "foo", "lib"), false); // own symbol, no import +test("resolveImport resolves relative specifiers and rejects bare ones", () => { + assert.equal(resolveImport("src/a/c.ts", "../lib"), "src/lib"); + assert.equal(resolveImport("src/c.ts", "./lib"), "src/lib"); + assert.equal(resolveImport("src/c.ts", "./lib/index"), "src/lib"); + assert.equal(resolveImport("src/c.ts", "lodash"), null); // bare package — not resolvable here }); -test("collectDiffExports reconstructs pre/post export images and added lines", () => { +test("importsSymbolFromModule resolves relative imports and covers named/namespace/re-export shapes", () => { + const lib = "src/lib.ts"; + assert.equal(importsSymbolFromModule("import { foo } from './lib';", "foo", "src/c.ts", lib), true); + assert.equal(importsSymbolFromModule("import { foo as bar } from '../lib';", "foo", "src/a/c.ts", lib), true); + assert.equal(importsSymbolFromModule("import { other } from './lib';", "foo", "src/c.ts", lib), false); // different symbol + assert.equal(importsSymbolFromModule("import { foo } from './lib';", "foo", "src/a/c.ts", lib), false); // ./lib here = src/a/lib + assert.equal(importsSymbolFromModule("import * as lib from './lib';\nlib.foo();", "foo", "src/c.ts", lib), true); // namespace use + assert.equal(importsSymbolFromModule("import * as lib from './lib';\nlib.bar();", "foo", "src/c.ts", lib), false); // namespace, not foo + assert.equal(importsSymbolFromModule("export { foo } from './lib';", "foo", "src/c.ts", lib), true); // re-export barrel + assert.equal(importsSymbolFromModule("export * from './lib';", "foo", "src/c.ts", lib), true); // star re-export + assert.equal(importsSymbolFromModule("function foo() {}", "foo", "src/c.ts", lib), false); // own def, no import +}); + +test("collectDiffExports reconstructs pre/post export images per file", () => { const out = collectDiffExports([ { path: "f.ts", patch: "@@ -1,1 +1,2 @@\n-export const removed = 1;\n+export const added = 1;\n+useSomething();", }, ]); - assert.ok(out.oldExports.has("removed")); - assert.ok(out.newExports.has("added")); + assert.ok(out.oldEntries.some((e) => e.name === "removed" && e.file === "f.ts")); + assert.ok(out.newEntries.some((e) => e.name === "added" && e.file === "f.ts")); assert.equal(out.newExportFile.get("added"), "f.ts"); assert.ok(out.addedLines.includes("useSomething();")); }); @@ -210,6 +222,92 @@ test("scanCallerImpact: an unrelated file defining its OWN same-named symbol is assert.deepEqual(out[0].callerFiles, ["src/real.ts"]); // unrelated.ts excluded by import binding }); +test("scanCallerImpact: an import of the same name from a DIFFERENT ./lib is not a caller", async () => { + const fetchImpl = ghStub({ + search: { "%22foo%22": [{ path: "src/feature/x.ts", text_matches: [{ fragment: "foo();" }] }] }, + contents: { "src/feature/x.ts": "import { foo } from './lib';\nfoo();" }, // ./lib here = src/feature/lib + }); + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], + }, + fetchImpl, + ); + assert.deepEqual(out, []); // imports a different ./lib (src/feature/lib), not the changed src/lib +}); + +test("scanCallerImpact: a namespace import that uses ns.symbol is a caller", async () => { + const fetchImpl = ghStub({ + search: { "%22foo%22": [{ path: "src/ns.ts", text_matches: [{ fragment: "lib.foo();" }] }] }, + contents: { "src/ns.ts": "import * as lib from './lib';\nlib.foo();" }, + }); + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], + }, + fetchImpl, + ); + assert.equal(out.length, 1); + assert.deepEqual(out[0].callerFiles, ["src/ns.ts"]); +}); + +test("scanCallerImpact: a re-export barrel forwarding the symbol is a caller", async () => { + const fetchImpl = ghStub({ + search: { "%22foo%22": [{ path: "src/barrel.ts", text_matches: [{ fragment: "export { foo } from './lib';" }] }] }, + contents: { "src/barrel.ts": "export { foo } from './lib';" }, + }); + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [{ path: "src/lib.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }], + }, + fetchImpl, + ); + assert.equal(out.length, 1); + assert.deepEqual(out[0].callerFiles, ["src/barrel.ts"]); +}); + +test("scanCallerImpact: same-named exports in two changed files are classified per file", async () => { + const fetchImpl = ghStub({ + search: { + "%22foo%22": [ + { path: "src/ca.ts", text_matches: [{ fragment: "foo();" }] }, + { path: "src/cb.ts", text_matches: [{ fragment: "foo(1);" }] }, + ], + }, + contents: { + "src/ca.ts": "import { foo } from './a';\nfoo();", // depends on a's foo + "src/cb.ts": "import { foo } from './b';\nfoo(1);", // depends on b's foo + }, + }); + const out = await scanCallerImpact( + { + repoFullName: "o/r", + prNumber: 1, + githubToken: "t", + files: [ + { path: "src/a.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }, // removed from a + { + path: "src/b.ts", + patch: "@@ -1,1 +1,1 @@\n-export function foo(x: string): void;\n+export function foo(x: number): void;", // changed in b + }, + ], + }, + fetchImpl, + ); + const byKind = Object.fromEntries(out.map((f) => [f.kind, f.callerFiles])); + assert.deepEqual(byKind["removed-with-callers"], ["src/ca.ts"]); // a's foo → caller ca (imports ./a) + assert.deepEqual(byKind["changed-with-callers"], ["src/cb.ts"]); // b's foo → caller cb (imports ./b) +}); + test("scanCallerImpact: a signature change with an importing caller is changed-with-callers", async () => { const fetchImpl = ghStub({ search: { "%22bar%22": [{ path: "src/caller.ts", text_matches: [{ fragment: "bar(1);" }] }] }, From dbb7641f05e3a3d480c5bdf4b7bc6f65b3e677dc Mon Sep 17 00:00:00 2001 From: dev-miro26 Date: Mon, 29 Jun 2026 22:04:07 -0700 Subject: [PATCH 7/7] fix(enrichment): scope caller-impact to removals; handle renames and skip default exports (#1509) --- .../src/analyzers/caller-impact.ts | 161 +++++++++--------- review-enrichment/src/render.ts | 6 +- review-enrichment/src/types.ts | 6 +- review-enrichment/test/caller-impact.test.ts | 155 ++++++----------- 4 files changed, 130 insertions(+), 198 deletions(-) diff --git a/review-enrichment/src/analyzers/caller-impact.ts b/review-enrichment/src/analyzers/caller-impact.ts index 7962a154a9..b60a448f5e 100644 --- a/review-enrichment/src/analyzers/caller-impact.ts +++ b/review-enrichment/src/analyzers/caller-impact.ts @@ -1,13 +1,16 @@ // Cross-file caller-impact / dead-symbol analyzer (#1509). Surfaces two cross-file hazards the no-checkout // `claude --print` reviewer (which only sees the diff) is blind to: -// 1. An exported top-level symbol the PR removes / renames / changes the signature of that STILL has dependents in -// files the PR did NOT touch — a hidden compile/runtime break. Candidate dependents come from the GitHub Code -// Search API (text-match) on the default branch; each candidate's CONTENT is then fetched and a finding is only -// reported when that file actually DEPENDS ON the symbol FROM the changed module — a named import, a namespace -// import used as `ns.symbol`, an `export { symbol } from`, or an `export * from` barrel — where the import -// specifier RESOLVES (relative to the candidate file) to the changed file's path. So a same-named export in an -// unrelated module, or an import of a different `./lib`, is never falsely flagged. Export churn is keyed per -// (file, name), so two changed files exporting the same name never overwrite each other. +// 1. An exported top-level symbol the PR REMOVES or RENAMES AWAY from a module while it still has importing +// dependents in files the PR did NOT touch — a hidden compile/runtime break. Candidate dependents come from the +// GitHub Code Search API (text-match) on the default branch; each candidate's CONTENT is then fetched and a +// finding is only reported when that file actually DEPENDS ON the symbol FROM the changed module — a named +// import, a namespace import used as `ns.symbol`, an `export { symbol } from`, or an `export * from` barrel — +// where the import specifier RESOLVES (relative to the candidate file) to the changed file's path. So a +// same-named export in an unrelated module, or an import of a different `./lib`, is never falsely flagged. +// Churn is keyed per (file, name) using each file's OLD path (so a rename is seen), and only the PRESENCE of an +// export is considered — an in-place signature/body edit is deliberately NOT flagged (a body change doesn't +// break importers and signature-vs-body can't be told apart reliably from a diff). Default exports are not +// modeled (a consumer imports a default with any local name, which name-based Code Search can't resolve). // 2. A newly-exported symbol referenced nowhere in the PR — dead-on-arrival. Code Search indexes the DEFAULT branch // only, so a brand-new symbol is invisible to it; this is judged from the diff, and entrypoint files // (index.*, *.d.ts) are skipped because public API is intentionally unused internally. @@ -27,7 +30,7 @@ const CODE_SEARCH_PER_PAGE = 20; const MAX_SEARCH_PAGES = 5; // pages one symbol may walk (≤100 hits) to see past filtered noise on page 1 const MAX_TOTAL_SEARCH_REQUESTS = 10; // global Code Search request budget (respects the ~10/min secondary limit) const MAX_TOTAL_CONTENT_FETCHES = 30; // global Contents-API budget for import verification across all symbols -const MAX_DECL_LINES = 40; // bound the contiguous multiline export declaration accumulated for signature comparison +const MAX_DECL_LINES = 40; // bound the contiguous multiline export declaration accumulated for name extraction const REPO_SEGMENT = /^[A-Za-z0-9._-]+$/; const ENTRYPOINT_RE = /(^|\/)index\.[cm]?[jt]sx?$|\.d\.ts$/; // public-API files: skip dead-on-arrival here @@ -81,7 +84,7 @@ export function normalizeModulePath(path: string): string { .replace(/\/index$/, ""); } -/** Resolve a relative import specifier against the importing file's path → a normalized module path. Returns null for +/** Resolve a relative import specifier against the importing file's path -> a normalized module path. Returns null for * a non-relative (bare package / tsconfig-alias) specifier that can't be resolved without build config. */ export function resolveImport(fromFile: string, specifier: string): string | null { if (!specifier.startsWith(".")) return null; @@ -115,7 +118,7 @@ function splitTopLevelCommas(value: string): string[] { /** Strip line/block comments and string/template-literal CONTENT from a single line, so a symbol that appears only in * a comment or string is not mistaken for a real code reference. Best-effort single-line scrub (advisory): a - * comment-only line (`//…`, JSDoc `*…`, `/*…`) is dropped entirely. */ + * comment-only line (`//...`, JSDoc `*...`, `/*...`) is dropped entirely. */ export function stripCommentsAndStrings(line: string): string { const trimmed = line.trim(); if ( @@ -145,10 +148,10 @@ export function referencesSymbol(code: string, symbol: string): boolean { } /** True when the candidate file `content` (at `candidatePath`) depends on `symbol` from the changed module at - * `changedPath`. A dependent is: a named import / re-export `{ … symbol … } from`, a namespace import + * `changedPath`. A dependent is: a named import / re-export `{ ... symbol ... } from`, a namespace import * `* as ns from` used as `ns.symbol`, or an `export * from` barrel — in every case the specifier must RESOLVE - * (relative to the candidate file) to the changed file's path, so a same-named export in an unrelated module is not - * matched. */ + * (relative to the candidate file) to the changed file's path, so a same-named export in an unrelated module, or an + * import of a different `./lib`, is not matched. */ export function importsSymbolFromModule( content: string, symbol: string, @@ -181,8 +184,8 @@ export function importsSymbolFromModule( /** Exported top-level identifier(s) declared by a single (possibly multiline-joined) export statement. Handles * `export function|class|interface|type|enum|namespace NAME`, `export const enum NAME`, multi-declarator - * `export const a = 1, b = 2`, `export default function|class NAME`, and `export { a, b as c }` (the public name is - * the alias after `as`). Returns [] for `export * from …`, anonymous default exports, and non-export lines. */ + * `export const a = 1, b = 2`, and `export { a, b as c }` (the public name is the alias after `as`). Returns [] for + * `export default ...` (defaults aren't modeled), `export * from ...`, and non-export lines. */ export function parseExportedNames(line: string): string[] { const s = line.trim(); if (!s.startsWith("export")) return []; @@ -201,10 +204,10 @@ export function parseExportedNames(line: string): string[] { .filter((name): name is string => name.length > 0 && name !== "default"); } - const def = s.match( - /^export\s+default\s+(?:async\s+)?(?:function\*?|class)\s+([A-Za-z_$][\w$]*)/, - ); - if (def) return [def[1]!]; + // Default exports are intentionally NOT modeled: an unchanged consumer imports a default with any local name + // (`import anything from './lib'`), which name-based Code Search can't resolve, so treating the declared name + // (`export default function main`) as the public symbol would be wrong. (#1509) + if (/^export\s+default\b/.test(s)) return []; const constEnum = s.match( /^export\s+(?:declare\s+)?const\s+enum\s+([A-Za-z_$][\w$]*)/, @@ -236,7 +239,7 @@ const norm = (line: string): string => line.trim().replace(/\s+/g, " "); /** Inclusive index where the export declaration starting at `start` ends: walk lines until the bracket depth * (parens/braces/brackets) returns to 0 and the line is not a continuation. Captures the FULL contiguous multiline - * declaration so a signature change on a LATER line is not mistaken for an identical move. Bounded by MAX_DECL_LINES. */ + * declaration so a name on a later line is still parsed. Bounded by MAX_DECL_LINES. */ function declarationEnd(lines: string[], start: number): number { let depth = 0; for (let i = start; i < lines.length && i - start < MAX_DECL_LINES; i++) { @@ -250,25 +253,22 @@ function declarationEnd(lines: string[], start: number): number { } /** Parse exported symbol names from a sequence of source lines. Each multiline export declaration is joined into one - * statement so the FULL declaration text (not just its first line) is compared for signature changes. */ -export function extractExports( - lines: string[], -): Array<{ names: string[]; declText: string }> { - const out: Array<{ names: string[]; declText: string }> = []; + * statement so a name spread across several lines is still found. */ +export function extractExports(lines: string[]): string[] { + const out: string[] = []; for (let i = 0; i < lines.length; i++) { if (!lines[i]!.trim().startsWith("export")) continue; const end = declarationEnd(lines, i); const joined = norm(lines.slice(i, end + 1).join(" ")); - const names = parseExportedNames(joined); - if (names.length) out.push({ names, declText: joined }); + out.push(...parseExportedNames(joined)); i = end; } return out; } /** A unified-diff patch split into its pre-image (context + removed) and post-image (context + added), plus the - * purely-added lines. Comparing exports parsed from the pre- vs post-image catches a change confined to an inner line - * of a multiline declaration whose `export …` line is unchanged context. */ + * purely-added lines. Reconstructing each image catches a churn confined to an inner line of a declaration whose + * `export ...` line is unchanged context. */ function splitPatchImages(patch: string): { pre: string[]; post: string[]; @@ -296,22 +296,19 @@ function splitPatchImages(patch: string): { return { pre, post, added }; } -/** One exported symbol declared in one file's pre- or post-image. */ -interface ExportEntry { - file: string; - name: string; - declText: string; -} - interface DiffExports { - /** every export in the pre-image, keyed per (file, name) */ - oldEntries: ExportEntry[]; - /** every export in the post-image, keyed per (file, name) */ - newEntries: ExportEntry[]; + /** set of `${file} ${name}` exported in some file's pre-image (file = the file's OLD path, so a rename is seen) */ + oldExports: Set; + /** set of `${file} ${name}` exported in some file's post-image */ + newExports: Set; + /** old export `${file} ${name}` -> { file (old path), name }, for caller resolution against the old module */ + oldExportInfo: Map; /** the set of names exported anywhere before the PR (so a genuinely-new name can be told from a move) */ oldNames: Set; - /** newly-exported name → the file it was first declared in (for the dead-on-arrival entrypoint check) */ + /** newly-exported name -> the file it was first declared in (for the dead-on-arrival entrypoint check) */ newExportFile: Map; + /** the set of names exported anywhere in the post-image (dead-on-arrival iterates these) */ + newNames: Set; /** purely-added source lines across the PR (for the dead-on-arrival reference scan) */ addedLines: string[]; } @@ -320,30 +317,34 @@ interface DiffExports { export function collectDiffExports( files: NonNullable, ): DiffExports { - const oldEntries: ExportEntry[] = []; - const newEntries: ExportEntry[] = []; + const oldExports = new Set(); + const newExports = new Set(); + const oldExportInfo = new Map(); const oldNames = new Set(); const newExportFile = new Map(); + const newNames = new Set(); const addedLines: string[] = []; for (const file of files) { if (!file.patch) continue; const { pre, post, added } = splitPatchImages(file.patch); for (const src of added) addedLines.push(src); - for (const { names, declText } of extractExports(pre)) { - for (const name of names) { - oldEntries.push({ file: file.path, name, declText }); - oldNames.add(name); - } + // Pre-image exports belong to the file's OLD path, so a pure rename (src/old.ts -> src/new.ts) makes the export + // "removed" from src/old.ts and importers of `./old` are correctly flagged. (#1509) + const oldPath = file.previousPath ?? file.path; + for (const name of extractExports(pre)) { + const key = `${oldPath} ${name}`; + oldExports.add(key); + oldExportInfo.set(key, { file: oldPath, name }); + oldNames.add(name); } - for (const { names, declText } of extractExports(post)) { - for (const name of names) { - newEntries.push({ file: file.path, name, declText }); - if (!newExportFile.has(name)) newExportFile.set(name, file.path); - } + for (const name of extractExports(post)) { + newExports.add(`${file.path} ${name}`); + newNames.add(name); + if (!newExportFile.has(name)) newExportFile.set(name, file.path); } } - return { oldEntries, newEntries, oldNames, newExportFile, addedLines }; + return { oldExports, newExports, oldExportInfo, oldNames, newExportFile, newNames, addedLines }; } /** True when the symbol is used in an added line OTHER than its own export declaration (so it is NOT dead). A mention @@ -455,15 +456,16 @@ async function findExternalCallers( if (callers.length >= MAX_CALLER_FILES || contentBudget.remaining <= 0) break; contentBudget.remaining--; const content = await fetchFileContent(owner, repo, path, token, fetchImpl, signal); - if (content === null) continue; // can't verify the import → drop (conservative) + if (content === null) continue; // can't verify the import -> drop (conservative) if (importsSymbolFromModule(content, symbol, path, changedPath)) callers.push(path); } return callers.sort(); } -/** Analyzer entrypoint. Flags removed/renamed/changed exports that still have importing dependents in unchanged files, - * plus dead-on-arrival new exports. The caller path needs a token (skipped without one); the diff-only dead-on-arrival - * path runs regardless. Fail-safe: returns [] without a repo or export churn; a failed lookup drops that symbol only. */ +/** Analyzer entrypoint. Flags exports the PR removes / renames away that still have importing dependents in unchanged + * files, plus dead-on-arrival new exports. The caller path needs a token (skipped without one); the diff-only + * dead-on-arrival path runs regardless. Fail-safe: returns [] without a repo or export churn; a failed lookup drops + * that symbol only. */ export async function scanCallerImpact( req: EnrichRequest, fetchImpl: typeof fetch = fetch, @@ -474,8 +476,9 @@ export async function scanCallerImpact( const files = req.files ?? []; if (!repo || files.length === 0) return []; - const { oldEntries, newEntries, oldNames, newExportFile, addedLines } = collectDiffExports(files); - if (oldEntries.length === 0 && newEntries.length === 0) return []; + const { oldExports, newExports, oldExportInfo, oldNames, newExportFile, newNames, addedLines } = + collectDiffExports(files); + if (oldExports.size === 0 && newExports.size === 0) return []; const changed = new Set(); for (const file of files) { @@ -483,46 +486,38 @@ export async function scanCallerImpact( if (file.previousPath) changed.add(file.previousPath); } - // Per-(file, name) post-image declaration text, so a same-named export in another file can't be conflated. - const newByKey = new Map(); - for (const entry of newEntries) newByKey.set(`${entry.file}${entry.name}`, entry.declText); - const findings: CallerImpactFinding[] = []; - // Removed / renamed / signature-changed exports → importing dependents in unchanged files. Needs the token; skipped - // without one. Bounded by the shared Code Search + Contents budgets. + // Removed / renamed-away exports -> importing dependents in unchanged files. An export is "removed from its module" + // when its `${oldPath} ${name}` key is absent from the post-image set; an in-place signature/body change keeps the + // key present and is intentionally NOT flagged. Needs the token; bounded by the shared Code Search + Contents budgets. if (token) { const searchBudget = { remaining: MAX_TOTAL_SEARCH_REQUESTS }; const contentBudget = { remaining: MAX_TOTAL_CONTENT_FETCHES }; let searched = 0; - for (const entry of oldEntries) { + for (const key of oldExports) { if (searched >= MAX_SYMBOLS_SEARCHED || searchBudget.remaining <= 0) break; - const newText = newByKey.get(`${entry.file}${entry.name}`); - if (newText !== undefined && newText === entry.declText) continue; // unchanged in this file ⇒ skip + if (newExports.has(key)) continue; // still exported from this module + const info = oldExportInfo.get(key); + if (!info) continue; searched++; - const callerFiles = await findExternalCallers(entry.name, repo.owner, repo.repo, changed, entry.file, token, fetchImpl, searchBudget, contentBudget, options.signal); + const callerFiles = await findExternalCallers(info.name, repo.owner, repo.repo, changed, info.file, token, fetchImpl, searchBudget, contentBudget, options.signal); if (!callerFiles || callerFiles.length === 0) continue; - findings.push({ - symbol: entry.name, - kind: newText === undefined ? "removed-with-callers" : "changed-with-callers", - callerFiles, - }); + findings.push({ symbol: info.name, kind: "removed-with-callers", callerFiles }); } } // Dead-on-arrival: genuinely-new exported symbols (not exported anywhere before) referenced nowhere in the diff. // Diff-only — Code Search can't see a brand-new symbol. Skip public-entrypoint files (exports meant for external use). let deadReported = 0; - const deadSeen = new Set(); - for (const entry of newEntries) { + for (const name of newNames) { if (deadReported >= MAX_DEAD_REPORTED) break; - if (oldNames.has(entry.name) || deadSeen.has(entry.name)) continue; // existed before, or already reported - const file = newExportFile.get(entry.name) ?? entry.file; + if (oldNames.has(name)) continue; // existed before — a move, not a new export + const file = newExportFile.get(name) ?? ""; if (ENTRYPOINT_RE.test(file)) continue; // likely public API - if (isReferencedInDiff(entry.name, addedLines)) continue; - deadSeen.add(entry.name); + if (isReferencedInDiff(name, addedLines)) continue; deadReported++; - findings.push({ symbol: entry.name, kind: "dead-on-arrival", callerFiles: [] }); + findings.push({ symbol: name, kind: "dead-on-arrival", callerFiles: [] }); } // Stable order (by kind, then symbol) so the rendered brief is deterministic regardless of Code Search result order. diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 9c53d1664e..4de23c1bd8 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -362,14 +362,10 @@ export function renderBrief( ); continue; } - const verb = - item.kind === "removed-with-callers" - ? "removed/renamed but still referenced in" - : "signature-changed but still referenced in"; const files = item.callerFiles.map((f) => safeCodeSpan(f)).join(", "); const count = item.callerFiles.length; lines.push( - `- ${safeCodeSpan(item.symbol)} ${verb} ${count} unchanged file${count === 1 ? "" : "s"}: ${files} — update the callers or keep a compatibility shim`, + `- ${safeCodeSpan(item.symbol)} removed/renamed but still imported by ${count} unchanged file${count === 1 ? "" : "s"}: ${files} — update the importers or keep a compatibility shim`, ); } } diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index f145a4c595..0e0511cf71 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -236,13 +236,13 @@ export interface NativeBuildFinding { reason: string; } -/** An exported top-level symbol the PR removes / renames / changes the signature of while it still has live callers +/** An exported top-level symbol the PR removes / renames away from a module while it still has importing dependents * in files the PR did NOT touch (a hidden cross-file compile/runtime break), or a newly-exported symbol referenced * nowhere in the PR (dead-on-arrival). Reports the symbol name + the unchanged caller files only — never source. (#1509) */ export interface CallerImpactFinding { symbol: string; - kind: "removed-with-callers" | "changed-with-callers" | "dead-on-arrival"; - /** Unchanged files (outside the PR's diff) that still reference the symbol. Empty for `dead-on-arrival`. */ + kind: "removed-with-callers" | "dead-on-arrival"; + /** Unchanged files that still import the symbol from the changed module. Empty for `dead-on-arrival`. */ callerFiles: string[]; } diff --git a/review-enrichment/test/caller-impact.test.ts b/review-enrichment/test/caller-impact.test.ts index a484902834..c977abee49 100644 --- a/review-enrichment/test/caller-impact.test.ts +++ b/review-enrichment/test/caller-impact.test.ts @@ -32,7 +32,7 @@ const throwingFetch = async () => { throw new Error("network down"); }; -// A fetch stub routing /search/code (by encoded-symbol substring → items) and /contents/ (→ raw file content). +// A fetch stub routing /search/code (by encoded-symbol substring -> items) and /contents/ (-> raw file content). function ghStub({ search = {}, contents = {} }) { return async (url) => { if (url.includes("/search/code")) { @@ -63,8 +63,6 @@ test("parseExportedNames covers the common export forms", () => { assert.deepEqual(parseExportedNames("export enum E {}"), ["E"]); assert.deepEqual(parseExportedNames("export namespace NS {}"), ["NS"]); assert.deepEqual(parseExportedNames("export declare function dfn(): void;"), ["dfn"]); - assert.deepEqual(parseExportedNames("export default function main() {}"), ["main"]); - assert.deepEqual(parseExportedNames("export default class App {}"), ["App"]); assert.deepEqual(parseExportedNames(" export function indented() {}"), ["indented"]); }); @@ -80,9 +78,11 @@ test("parseExportedNames handles named export lists with aliases", () => { assert.deepEqual(parseExportedNames('export { x } from "./x";'), ["x"]); }); -test("parseExportedNames returns [] for re-export-all, anonymous default, and non-exports", () => { - assert.deepEqual(parseExportedNames('export * from "./x";'), []); +test("parseExportedNames returns [] for default exports, re-export-all, and non-exports", () => { + assert.deepEqual(parseExportedNames("export default function main() {}"), []); // defaults are not modeled + assert.deepEqual(parseExportedNames("export default class App {}"), []); assert.deepEqual(parseExportedNames("export default 42;"), []); + assert.deepEqual(parseExportedNames('export * from "./x";'), []); assert.deepEqual(parseExportedNames("const x = 1;"), []); assert.deepEqual(parseExportedNames("import { foo } from './a';"), []); }); @@ -113,15 +113,15 @@ test("importsSymbolFromModule resolves relative imports and covers named/namespa assert.equal(importsSymbolFromModule("function foo() {}", "foo", "src/c.ts", lib), false); // own def, no import }); -test("collectDiffExports reconstructs pre/post export images per file", () => { +test("collectDiffExports reconstructs pre/post export sets per file", () => { const out = collectDiffExports([ { path: "f.ts", patch: "@@ -1,1 +1,2 @@\n-export const removed = 1;\n+export const added = 1;\n+useSomething();", }, ]); - assert.ok(out.oldEntries.some((e) => e.name === "removed" && e.file === "f.ts")); - assert.ok(out.newEntries.some((e) => e.name === "added" && e.file === "f.ts")); + assert.ok(out.oldExports.has("f.ts removed")); + assert.ok(out.newExports.has("f.ts added")); assert.equal(out.newExportFile.get("added"), "f.ts"); assert.ok(out.addedLines.includes("useSomething();")); }); @@ -149,24 +149,10 @@ test("referencesSymbol counts real code references, not comments or strings", () assert.equal(referencesSymbol("notfoo + foobar", "foo"), false); // substring only }); -test("extractExports joins a multiline export { } block", () => { - assert.deepEqual( - extractExports(["export {", " alpha,", " beta,", "};"]).flatMap((e) => e.names), - ["alpha", "beta"], - ); - assert.deepEqual( - extractExports(["export const single = 1;"]).flatMap((e) => e.names), - ["single"], - ); -}); - -test("extractExports captures a multiline function signature for change comparison", () => { - const before = extractExports(["export function foo(", " a: string,", "): void;"]); - const after = extractExports(["export function foo(", " a: number,", "): void;"]); - assert.deepEqual(before.flatMap((e) => e.names), ["foo"]); - assert.deepEqual(after.flatMap((e) => e.names), ["foo"]); - // Same name, but the FULL declaration text differs ⇒ a real signature change, not an identical move/reformat. - assert.notEqual(before[0].declText, after[0].declText); +test("extractExports joins a multiline export { } block and a multiline declaration", () => { + assert.deepEqual(extractExports(["export {", " alpha,", " beta,", "};"]), ["alpha", "beta"]); + assert.deepEqual(extractExports(["export const single = 1;"]), ["single"]); + assert.deepEqual(extractExports(["export function foo(", " a: string,", "): void;"]), ["foo"]); }); // ── scanCallerImpact ────────────────────────────────────────────────────────── @@ -275,43 +261,35 @@ test("scanCallerImpact: a re-export barrel forwarding the symbol is a caller", a assert.deepEqual(out[0].callerFiles, ["src/barrel.ts"]); }); -test("scanCallerImpact: same-named exports in two changed files are classified per file", async () => { - const fetchImpl = ghStub({ - search: { - "%22foo%22": [ - { path: "src/ca.ts", text_matches: [{ fragment: "foo();" }] }, - { path: "src/cb.ts", text_matches: [{ fragment: "foo(1);" }] }, - ], - }, - contents: { - "src/ca.ts": "import { foo } from './a';\nfoo();", // depends on a's foo - "src/cb.ts": "import { foo } from './b';\nfoo(1);", // depends on b's foo - }, - }); +test("scanCallerImpact: a body-only change to an exported function is NOT flagged", async () => { + let searched = false; + const fetchImpl = async (url) => { + if (url.includes("/search/code")) searched = true; + return res({ items: [] }); + }; const out = await scanCallerImpact( { repoFullName: "o/r", prNumber: 1, githubToken: "t", files: [ - { path: "src/a.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }, // removed from a { - path: "src/b.ts", - patch: "@@ -1,1 +1,1 @@\n-export function foo(x: string): void;\n+export function foo(x: number): void;", // changed in b + path: "src/lib.ts", + // `export function foo(): void {` and `}` are unchanged context; only the body line changes. + patch: "@@ -1,3 +1,3 @@\n export function foo(): void {\n- return doOld();\n+ return doNew();\n }", }, ], }, fetchImpl, ); - const byKind = Object.fromEntries(out.map((f) => [f.kind, f.callerFiles])); - assert.deepEqual(byKind["removed-with-callers"], ["src/ca.ts"]); // a's foo → caller ca (imports ./a) - assert.deepEqual(byKind["changed-with-callers"], ["src/cb.ts"]); // b's foo → caller cb (imports ./b) + assert.deepEqual(out, []); // foo is still exported from src/lib (present in pre and post) → not a removal + assert.equal(searched, false); }); -test("scanCallerImpact: a signature change with an importing caller is changed-with-callers", async () => { +test("scanCallerImpact: a renamed file's importers of the OLD path are flagged", async () => { const fetchImpl = ghStub({ - search: { "%22bar%22": [{ path: "src/caller.ts", text_matches: [{ fragment: "bar(1);" }] }] }, - contents: { "src/caller.ts": "import { bar } from './lib';\nbar(1);" }, + search: { "%22foo%22": [{ path: "src/caller.ts", text_matches: [{ fragment: "foo();" }] }] }, + contents: { "src/caller.ts": "import { foo } from './old';\nfoo();" }, }); const out = await scanCallerImpact( { @@ -320,46 +298,32 @@ test("scanCallerImpact: a signature change with an importing caller is changed-w githubToken: "t", files: [ { - path: "src/lib.ts", - patch: "@@ -1,1 +1,1 @@\n-export function bar(a: string): void;\n+export function bar(a: number): void;", + path: "src/new.ts", + previousPath: "src/old.ts", + status: "renamed", + patch: "@@ -1,1 +1,1 @@\n-export function foo(): void;\n+export function foo(): void;", }, ], }, fetchImpl, ); assert.equal(out.length, 1); - assert.equal(out[0].kind, "changed-with-callers"); + assert.equal(out[0].kind, "removed-with-callers"); + assert.deepEqual(out[0].callerFiles, ["src/caller.ts"]); // importer of ./old breaks after the rename }); -test("scanCallerImpact: a change confined to a parameter line (export line is context) is detected", async () => { +test("scanCallerImpact: same-named exports removed from two changed files are classified per file", async () => { const fetchImpl = ghStub({ - search: { "%22foo%22": [{ path: "src/caller.ts", text_matches: [{ fragment: "foo(1);" }] }] }, - contents: { "src/caller.ts": "import { foo } from './lib';\nfoo(1);" }, - }); - const out = await scanCallerImpact( - { - repoFullName: "o/r", - prNumber: 1, - githubToken: "t", - files: [ - { - path: "src/lib.ts", - // `export function foo(` and `): void;` are unchanged CONTEXT; only the parameter line is -/+. - patch: "@@ -1,3 +1,3 @@\n export function foo(\n- a: string,\n+ a: number,\n ): void;", - }, + search: { + "%22foo%22": [ + { path: "src/ca.ts", text_matches: [{ fragment: "foo();" }] }, + { path: "src/cb.ts", text_matches: [{ fragment: "foo();" }] }, ], }, - fetchImpl, - ); - assert.equal(out.length, 1); - assert.equal(out[0].symbol, "foo"); - assert.equal(out[0].kind, "changed-with-callers"); -}); - -test("scanCallerImpact: a multiline signature change (whole block re-added) is changed-with-callers", async () => { - const fetchImpl = ghStub({ - search: { "%22foo%22": [{ path: "src/caller.ts", text_matches: [{ fragment: "foo(1);" }] }] }, - contents: { "src/caller.ts": "import { foo } from './lib';\nfoo(1);" }, + contents: { + "src/ca.ts": "import { foo } from './a';\nfoo();", // depends on a's foo + "src/cb.ts": "import { foo } from './b';\nfoo();", // depends on b's foo + }, }); const out = await scanCallerImpact( { @@ -367,36 +331,15 @@ test("scanCallerImpact: a multiline signature change (whole block re-added) is c prNumber: 1, githubToken: "t", files: [ - { - path: "src/lib.ts", - patch: - "@@ -1,3 +1,3 @@\n-export function foo(\n- a: string,\n-): void;\n+export function foo(\n+ a: number,\n+): void;", - }, + { path: "src/a.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }, + { path: "src/b.ts", patch: "@@ -1,1 +0,0 @@\n-export function foo(): void;" }, ], }, fetchImpl, ); - assert.equal(out.length, 1); - assert.equal(out[0].kind, "changed-with-callers"); -}); - -test("scanCallerImpact: an identical export on both sides (moved) is not searched or flagged", async () => { - let searched = false; - const tracking = async (url) => { - if (url.includes("/search/code")) searched = true; - return res({ items: [] }); - }; - const out = await scanCallerImpact( - { - repoFullName: "o/r", - prNumber: 1, - githubToken: "t", - files: [{ path: "src/lib.ts", patch: "@@ -1,1 +1,1 @@\n-export function baz(): void;\n+export function baz(): void;" }], - }, - tracking, - ); - assert.deepEqual(out, []); - assert.equal(searched, false); + // foo removed from BOTH modules → two findings, each bound to its own importer. + const callerSets = out.map((f) => f.callerFiles).sort(); + assert.deepEqual(callerSets, [["src/ca.ts"], ["src/cb.ts"]]); }); test("scanCallerImpact: pages past a noisy first page to find an importing caller on page 2", async () => { @@ -552,7 +495,7 @@ test("scanCallerImpact: dead-on-arrival runs without a token (diff-only, no netw assert.equal(out[0].symbol, "orphan"); }); -test("scanCallerImpact: no token returns [] without any fetch", async () => { +test("scanCallerImpact: no token returns [] without any caller fetch", async () => { const out = await scanCallerImpact( { repoFullName: "o/r", prNumber: 1, files: [{ path: "src/lib.ts", patch: "@@ @@\n-export function foo(): void;" }] }, throwingFetch, @@ -595,13 +538,11 @@ test("renderBrief emits a public-safe caller-impact block", () => { const { promptSection } = renderBrief({ callerImpact: [ { symbol: "foo", kind: "removed-with-callers", callerFiles: ["src/a.ts", "src/b.ts"] }, - { symbol: "bar", kind: "changed-with-callers", callerFiles: ["src/c.ts"] }, { symbol: "baz", kind: "dead-on-arrival", callerFiles: [] }, ], }); assert.match(promptSection, /Cross-file API impact/); - assert.match(promptSection, /`foo` removed\/renamed but still referenced in 2 unchanged files/); - assert.match(promptSection, /`bar` signature-changed but still referenced in 1 unchanged file\b/); + assert.match(promptSection, /`foo` removed\/renamed but still imported by 2 unchanged files/); assert.match(promptSection, /`baz` is exported but referenced nowhere in this PR \(dead-on-arrival\)/); assert.match(promptSection, /`src\/a\.ts`, `src\/b\.ts`/); });