diff --git a/.env.example b/.env.example index 759964fc6a..6e2f8b3691 100644 --- a/.env.example +++ b/.env.example @@ -64,18 +64,18 @@ GITTENSORY_REVIEW_ENRICHMENT=false # BEGIN GENERATED REES ANALYZERS # Current analyzer names: # dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol,redos -# provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild -# history,docCommentDrift +# provenance,codeowners,callerImpact,secretLog,assetWeight,typosquat,commitSignature +# iacMisconfig,nativeBuild,history,docCommentDrift # # Profile defaults: # fast: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol # redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild # balanced (default): dependency,lockfileDrift,secret,license,installScript,heavyDependency -# actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature -# iacMisconfig,nativeBuild,history,docCommentDrift +# actionPin,eol,redos,provenance,codeowners,callerImpact,secretLog,assetWeight,typosquat +# commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift # deep: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol -# redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig -# nativeBuild,history,docCommentDrift +# redos,provenance,codeowners,callerImpact,secretLog,assetWeight,typosquat,commitSignature +# iacMisconfig,nativeBuild,history,docCommentDrift # END GENERATED REES ANALYZERS # Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index a73ff5eec9..5c9a37902e 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -327,6 +327,31 @@ export const REES_ANALYZERS = [ "Leave REES_FORWARD_GITHUB_TOKEN unset/false to disable token forwarding; this analyzer will then skip when it cannot read CODEOWNERS.", }, }, + { + name: "callerImpact", + title: "Cross-file caller impact + dead exports", + category: "quality", + cost: "github-light", + defaultEnabled: true, + profiles: ["balanced", "deep"], + requires: ["files", "github-token", "head-sha"], + limits: { + maxFindings: 18, + maxSearchResults: 30, + }, + docs: { + summary: + "Detects export-surface changes with live callsites and new exports with no callsites.", + looksAt: + "Top-level exported symbols in the PR diff plus a bounded symbol search in unchanged files at headSha.", + reports: + "Changed/removed/renamed exports with unchanged file callsites, and dead exports with zero external callsites.", + network: + "Calls GitHub code search and per-file contents APIs. Requires headSha plus GitHub token forwarding for private repos.", + notes: + "The analyzer is additive and fail-safe: network failures yield fewer findings, never errors in brief assembly.", + }, + }, { name: "secretLog", title: "Secrets or PII in logs", diff --git a/review-enrichment/README.md b/review-enrichment/README.md index 7859bb96c0..309108acea 100644 --- a/review-enrichment/README.md +++ b/review-enrichment/README.md @@ -36,6 +36,7 @@ inside the operator's trust boundary. The engine prefers a short-lived installat | `redos` | Regex literals with catastrophic-backtracking structure. | Pure local. | | `provenance` | Missing package attestations plus binary/vendored/minified additions. | Calls npm/PyPI for attestations; path checks are local. | | `codeowners` | Changed files owned by CODEOWNERS entries that do not include the PR author. | Calls GitHub API; needs author and token for private repos. | +| `callerImpact` | Export-surface changes with live cross-file callsites or dead new exports. | Calls GitHub code search and contents APIs; needs token/private-read access. | | `secretLog` | Secrets, PII, or request/session objects written to logs/stdout. | Pure local. | | `assetWeight` | Heavy binary assets added or grown. | Calls GitHub API; needs headSha, baseSha for growth, and token for private repos. | | `typosquat` | New dependency names that look squatted or publicly claimable. | Uses bundled popular-package lists plus npm/PyPI lookups. | diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index 9ad7e58ad8..db39ae638a 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -369,6 +369,33 @@ "notes": "Leave REES_FORWARD_GITHUB_TOKEN unset/false to disable token forwarding; this analyzer will then skip when it cannot read CODEOWNERS." } }, + { + "name": "callerImpact", + "title": "Cross-file caller impact + dead exports", + "category": "quality", + "cost": "github-light", + "defaultEnabled": true, + "profiles": [ + "balanced", + "deep" + ], + "requires": [ + "files", + "github-token", + "head-sha" + ], + "limits": { + "maxFindings": 18, + "maxSearchResults": 30 + }, + "docs": { + "summary": "Detects export-surface changes with live callsites and new exports with no callsites.", + "looksAt": "Top-level exported symbols in the PR diff plus a bounded symbol search in unchanged files at headSha.", + "reports": "Changed/removed/renamed exports with unchanged file callsites, and dead exports with zero external callsites.", + "network": "Calls GitHub code search and per-file contents APIs. Requires headSha plus GitHub token forwarding for private repos.", + "notes": "The analyzer is additive and fail-safe: network failures yield fewer findings, never errors in brief assembly." + } + }, { "name": "secretLog", "title": "Secrets or PII in logs", diff --git a/review-enrichment/src/analyzers/caller-impact.ts b/review-enrichment/src/analyzers/caller-impact.ts new file mode 100644 index 0000000000..a08dd6ea40 --- /dev/null +++ b/review-enrichment/src/analyzers/caller-impact.ts @@ -0,0 +1,533 @@ +// Cross-file caller-impact + dead-symbol analyzer (#1509). Detects top-level export declarations +// removed from the PR diff that are still called in unchanged files, changed exports with still-live callsites, +// renamed export surfaces (alias migration), and added exports that are currently unreferenced. +import type { + AnalyzerDiagnostics, + CallerImpactFinding, + EnrichRequest, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchJson, boundedFetchText } from "../external-fetch.js"; + +const GITHUB_API = "https://api.github.com"; +const GITHUB_API_VERSION = "2022-11-28"; +const MAX_SYMBOLS = 18; // cap PR-derived symbols so one analyzer run stays bounded +const MAX_SEARCH_RESULTS = 30; // per-symbol cap +const MAX_CALLERS_PER_SYMBOL = 8; // keep findings short enough for prompt budget +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; +const IDENT_RE = /^[A-Za-z_$][A-Za-z0-9_$]*$/; +const IDENT_RE_STR = "[A-Za-z_$][A-Za-z0-9_$]*"; +const IDENT_BOUNDARY = `${IDENT_RE_STR}(?=[^A-Za-z0-9_$]|$)`; + +interface ParsedExport { + file: string; + line: number; + symbol: string; + localSymbol: string; + side: "added" | "removed"; + usesAlias: boolean; +} + +interface SearchPayload { + path: string; +} + +interface ScanOptions { + signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; +} + +function parseRepo( + repoFullName: string, +): { owner: string; repo: string } | null { + const parts = repoFullName.split("/"); + if (parts.length !== 2) return null; + const [owner, repo] = parts; + if (!owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return null; + return { owner, 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 encodePath(path: string): string | null { + if (!path) return null; + const segments = path.split("/"); + if (segments.some((segment) => !segment || segment === "." || segment === "..")) + return null; + return segments.map(encodeURIComponent).join("/"); +} + +function safeSymbol(symbol: string): boolean { + return IDENT_RE.test(symbol) && symbol.length <= 80; +} + +function symbolBoundaryRegex(symbol: string): RegExp { + const escaped = symbol.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + return new RegExp( + `(?:^|[^A-Za-z0-9_$])(${escaped})(?:$|[^A-Za-z0-9_$])`, + ); +} + +function patchEntries(patch: string): Array<{ + side: "added" | "removed"; + line: number; + content: string; +}> { + const entries: Array<{ + side: "added" | "removed"; + line: number; + content: string; + }> = []; + let oldLine = 0; + let newLine = 0; + let inHunk = false; + + for (const rawLine of patch.split(/\r?\n/)) { + const hunk = /^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(rawLine); + if (hunk) { + oldLine = Number.parseInt(hunk[1]!, 10); + newLine = Number.parseInt(hunk[2]!, 10); + inHunk = true; + continue; + } + if (!inHunk) continue; + if (rawLine.startsWith("+++") || rawLine.startsWith("---")) continue; + if (rawLine.startsWith(" ")) { + oldLine += 1; + newLine += 1; + continue; + } + if (rawLine.startsWith("+")) { + entries.push({ side: "added", line: newLine, content: rawLine.slice(1) }); + newLine += 1; + continue; + } + if (rawLine.startsWith("-")) { + entries.push({ side: "removed", line: oldLine, content: rawLine.slice(1) }); + oldLine += 1; + } + } + return entries; +} + +function parseExportLines(line: string): Array<{ + local: string; + symbol: string; + usesAlias: boolean; +}> { + const declaration = new RegExp( + `^\\s*export\\s+(?:default\\s+)?(?:(?:async\\s+)?function|class|interface|type|enum|const|let|var|namespace)\\s+(${IDENT_BOUNDARY})`, + ).exec(line); + if (declaration) { + const symbol = declaration[1]; + if (!symbol || !IDENT_RE.test(symbol)) return []; + return [{ local: symbol, symbol, usesAlias: false }]; + } + + const namespaceAs = new RegExp(`^\\s*export\\s*\\*\\s+as\\s+(${IDENT_BOUNDARY})`).exec( + line, + ); + if (namespaceAs) { + const symbol = namespaceAs[1]; + if (!symbol || !IDENT_RE.test(symbol)) return []; + return [{ local: "*", symbol, usesAlias: false }]; + } + + const exports = /^\s*export\s*\{([^}]*)\}\s*(?:from\s+["'][^"']+["'])?\s*;?/.exec(line); + if (!exports) return []; + + return (exports[1] ?? "") + .split(",") + .map((item) => item.trim()) + .filter(Boolean) + .map((item) => { + const alias = new RegExp( + `^((?:default)|${IDENT_RE_STR})\\s+as\\s+(${IDENT_RE_STR})$`, + ).exec(item); + if (alias && IDENT_RE.test(alias[1]!) && IDENT_RE.test(alias[2]!)) { + return { + local: alias[1]!, + symbol: alias[2]!, + usesAlias: true, + }; + } + if (IDENT_RE.test(item)) return { local: item, symbol: item, usesAlias: false }; + return null; + }) + .filter((entry): entry is NonNullable => entry !== null); +} + +function extractExportsFromPatch(filePath: string, patch: string): ParsedExport[] { + const parsed: ParsedExport[] = []; + for (const entry of patchEntries(patch)) { + for (const decl of parseExportLines(entry.content)) { + parsed.push({ + file: filePath, + line: entry.line, + symbol: decl.symbol, + localSymbol: decl.local, + side: entry.side, + usesAlias: decl.usesAlias, + }); + } + } + return parsed; +} + +function collectFromPatch( + files: NonNullable, +): { + added: ParsedExport[]; + removed: ParsedExport[]; + changedPaths: Set; +} { + const added: ParsedExport[] = []; + const removed: ParsedExport[] = []; + const changedPaths = new Set(); + + for (const file of files) { + changedPaths.add(file.path); + if (!file.patch) continue; + const entries = extractExportsFromPatch(file.path, file.patch); + for (const entry of entries) { + if (entry.side === "added") added.push(entry); + else removed.push(entry); + } + } + + return { added, removed, changedPaths }; +} + +async function searchUsages( + owner: string, + repo: string, + symbol: string, + token: string, + fetchImpl: typeof fetch, + options: ScanOptions = {}, +): Promise { + if (!safeSymbol(symbol)) return []; + const query = `\"${symbol}\" repo:${owner}/${repo} in:file`; + const url = `${GITHUB_API}/search/code?q=${encodeURIComponent(query)}&per_page=${MAX_SEARCH_RESULTS}`; + const headers = githubHeaders(token); + + try { + const response = options.analysis + ? await options.analysis.fetchJson<{ items?: SearchPayload[] }>(url, { + endpointCategory: "github-search-code", + headers, + method: "GET", + signal: options.signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "callerImpact", + subcall: "github-search-code", + maxBytes: 256 * 1024, + }) + : await boundedFetchJson<{ items?: SearchPayload[] }>(url, { + endpointCategory: "github-search-code", + headers, + method: "GET", + signal: options.signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "callerImpact", + subcall: "github-search-code", + maxBytes: 256 * 1024, + }); + if (!response.ok) return []; + const payload = response.data; + const paths = (payload.items ?? []) + .map((item) => item.path) + .filter((path): path is string => Boolean(path)) + .filter((path, index, list) => list.indexOf(path) === index); + + return paths; + } catch { + return []; + } +} + +async function readFileContainsSymbol( + owner: string, + repo: string, + path: string, + symbol: string, + headSha: string, + token: string, + fetchImpl: typeof fetch, + skipLineNumbers: ReadonlySet = new Set(), + options: ScanOptions = {}, +): Promise { + const encodedPath = encodePath(path); + if (!encodedPath) return false; + const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encodedPath}?ref=${encodeURIComponent(headSha)}`; + + try { + const response = options.analysis + ? await options.analysis.fetchText(url, { + endpointCategory: "github-contents-raw", + headers: { ...githubHeaders(token), Accept: "application/vnd.github.raw" }, + method: "GET", + signal: options.signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "callerImpact", + subcall: "github-contents-raw", + maxBytes: 768 * 1024, + }) + : await boundedFetchText(url, { + endpointCategory: "github-contents-raw", + headers: { ...githubHeaders(token), Accept: "application/vnd.github.raw" }, + method: "GET", + signal: options.signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "callerImpact", + subcall: "github-contents-raw", + maxBytes: 768 * 1024, + }); + if (!response.ok) return false; + const text = response.data; + const lineRegex = symbolBoundaryRegex(symbol); + for (const [index, line] of text.split("\n").entries()) { + const lineNumber = index + 1; + if (skipLineNumbers.has(lineNumber)) continue; + if (lineRegex.test(line)) { + return true; + } + } + return false; + } catch { + return false; + } +} + +async function resolveCallers( + owner: string, + repo: string, + symbol: string, + headSha: string, + token: string, + skipPaths: Set | null, + additionalPaths: Iterable = [], + skipLineNumbersByPath: ReadonlyMap> = new Map(), + fetchImpl: typeof fetch, + options: ScanOptions = {}, +): Promise { + const hitPaths = await searchUsages( + owner, + repo, + symbol, + token, + fetchImpl, + options, + ); + const candidatePaths = new Set(hitPaths); + for (const path of additionalPaths) candidatePaths.add(path); + const callers: string[] = []; + for (const path of candidatePaths) { + if (skipPaths?.has(path)) continue; + if (callers.length >= MAX_CALLERS_PER_SYMBOL) break; + if ( + await readFileContainsSymbol( + owner, + repo, + path, + symbol, + headSha, + token, + fetchImpl, + skipLineNumbersByPath.get(path) ?? new Set(), + options, + ) + ) { + callers.push(path); + } + } + return callers; +} + +interface Candidate { + kind: CallerImpactFinding["kind"]; + file: string; + line: number; + symbol: string; + searchSymbol: string; + previousSymbol?: string; +} + +function collectCandidates(added: ParsedExport[], removed: ParsedExport[]): Candidate[] { + const addedBySymbol = new Map(); + const removedBySymbol = new Map(); + const addedAliasByLocal = new Map(); + const removedAliasByLocal = new Map(); + + for (const entry of added) { + const list = addedBySymbol.get(entry.symbol) ?? []; + list.push(entry); + addedBySymbol.set(entry.symbol, list); + if (entry.usesAlias) { + const aliasList = addedAliasByLocal.get(entry.localSymbol) ?? []; + aliasList.push(entry); + addedAliasByLocal.set(entry.localSymbol, aliasList); + } + } + for (const entry of removed) { + const list = removedBySymbol.get(entry.symbol) ?? []; + list.push(entry); + removedBySymbol.set(entry.symbol, list); + if (entry.usesAlias) { + const aliasList = removedAliasByLocal.get(entry.localSymbol) ?? []; + aliasList.push(entry); + removedAliasByLocal.set(entry.localSymbol, aliasList); + } + } + + const candidates: Candidate[] = []; + const renameFrom = new Set(); + const renameTo = new Set(); + + for (const [local, removedAliasList] of removedAliasByLocal) { + const addedAliasList = addedAliasByLocal.get(local) ?? []; + if (removedAliasList.length !== 1 || addedAliasList.length !== 1) continue; + const removedAlias = removedAliasList[0]; + const addedAlias = addedAliasList[0]; + if (!removedAlias || !addedAlias) continue; + if (removedAlias.symbol === addedAlias.symbol) continue; + + candidates.push({ + kind: "renamed", + file: removedAlias.file, + line: removedAlias.line, + symbol: addedAlias.symbol, + searchSymbol: removedAlias.symbol, + previousSymbol: removedAlias.symbol, + }); + renameFrom.add(removedAlias.symbol); + renameTo.add(addedAlias.symbol); + } + + for (const [symbol, entries] of removedBySymbol) { + if (!entries.length || renameFrom.has(symbol) || removedBySymbol.get(symbol)?.length === 0) + continue; + if (addedBySymbol.has(symbol)) continue; + const first = entries[0]; + if (!first) continue; + candidates.push({ + kind: "removed", + file: first.file, + line: first.line, + symbol, + searchSymbol: symbol, + }); + } + + for (const [symbol, entries] of addedBySymbol) { + if (!entries.length || renameTo.has(symbol)) continue; + if (removedBySymbol.has(symbol)) { + const first = entries[0]; + if (!first) continue; + candidates.push({ + kind: "changed", + file: first.file, + line: first.line, + symbol, + searchSymbol: symbol, + }); + continue; + } + const first = entries[0]; + if (!first) continue; + candidates.push({ + kind: "dead", + file: first.file, + line: first.line, + symbol, + searchSymbol: symbol, + }); + } + + const deduped = new Map(); + for (const candidate of candidates) { + const key = `${candidate.kind}:${candidate.symbol}`; + if (!deduped.has(key)) deduped.set(key, candidate); + } + return Array.from(deduped.values()).slice(0, MAX_SYMBOLS); +} + +/** Analyzer entrypoint: return caller-impact and dead-symbol findings from diff exports + search-backed usage checks. */ +export async function scanCallerImpact( + req: EnrichRequest, + fetchImpl: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + if (!req.githubToken || !req.headSha) return []; + const repo = parseRepo(req.repoFullName); + if (!repo) return []; + + const files = req.files ?? []; + const { added, removed, changedPaths } = collectFromPatch(files); + if (!added.length && !removed.length) return []; + + const candidates = collectCandidates(added, removed); + if (!candidates.length) return []; + + const findings: CallerImpactFinding[] = []; + for (const candidate of candidates) { + if (options.signal?.aborted) throw new Error("analyzer_aborted"); + const skipLineNumbersByPath = + candidate.kind === "dead" + ? new Map([[candidate.file, new Set([candidate.line])]]) + : new Map>(); + const isDead = candidate.kind === "dead"; + const skipPaths = isDead ? null : changedPaths; + const additionalPaths = isDead ? Array.from(changedPaths) : []; + const callers = await resolveCallers( + repo.owner, + repo.repo, + candidate.searchSymbol, + req.headSha, + req.githubToken, + skipPaths, + additionalPaths, + skipLineNumbersByPath, + fetchImpl, + options, + ); + + if (candidate.kind === "dead") { + if (callers.length === 0) { + findings.push({ + kind: "dead", + file: candidate.file, + line: candidate.line, + symbol: candidate.symbol, + callers: [], + }); + } + continue; + } + + if (callers.length > 0) { + findings.push({ + kind: candidate.kind, + file: candidate.file, + line: candidate.line, + symbol: candidate.symbol, + callers, + previousSymbol: candidate.previousSymbol, + }); + } + } + + return findings; +} diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 9d89670555..600d042f1d 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -2,6 +2,7 @@ import { scanActionPins } from "./actions-pin.js"; import { scanAssetWeight } from "./asset-weight.js"; import { scanCodeowners } from "./codeowners.js"; import { scanCommitSignature } from "./commit-signature.js"; +import { scanCallerImpact } from "./caller-impact.js"; import { dependencyAnalyzer } from "./dependency/descriptor.js"; import { scanDocCommentDrift } from "./doc-comment-drift.js"; import { scanEol } from "./eol-check.js"; @@ -216,6 +217,51 @@ export const ANALYZER_DESCRIPTORS = [ run: (req, { signal, analysis, diagnostics }) => scanCodeowners(req, fetch, { signal, analysis, diagnostics }), }), + descriptor({ + name: "callerImpact", + title: "Cross-file caller impact + dead exports", + category: "quality", + cost: "github-light", + defaultEnabled: true, + requires: ["files", "github-token", "head-sha"], + limits: { maxFindings: 18, maxSearchResults: 30 }, + docs: { + summary: + "Detects export-surface changes with live callsites and new exports with no callsites.", + looksAt: + "Top-level exported symbols in the PR diff plus a bounded symbol search in unchanged files at headSha.", + reports: + "Changed/removed/renamed exports with unchanged file callsites, and dead exports with zero external callsites.", + network: + "Calls GitHub code search and per-file contents APIs. Requires headSha plus GitHub token forwarding for private repos.", + notes: + "The analyzer is additive and fail-safe: network failures yield fewer findings, never errors in brief assembly.", + }, + render: (findings, { safeCodeSpan }) => { + if (!findings.length) return []; + const lines: string[] = []; + lines.push("### Export caller-impact and dead-symbol candidates"); + for (const item of findings) { + const fileLine = safeCodeSpan(`${item.file}:${item.line}`); + if (item.kind === "dead") { + lines.push( + `- ${fileLine} — added export \`${safeCodeSpan(item.symbol)}\` currently has no callsites outside changed files`, + ); + continue; + } + + const title = + item.kind === "renamed" + ? `renamed ${safeCodeSpan(item.previousSymbol ?? item.symbol)} -> ${safeCodeSpan(item.symbol)}` + : `${item.kind} export \`${safeCodeSpan(item.symbol)}\``; + const callers = item.callers.map((path) => safeCodeSpan(path)).join(", "); + lines.push(`- ${fileLine} — ${title} still called in: ${callers}`); + } + return lines; + }, + run: (req, { signal, analysis, diagnostics }) => + scanCallerImpact(req, fetch, { signal, analysis, diagnostics }), + }), descriptor({ name: "secretLog", title: "Secrets or PII in logs", diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index c90461c434..5ee31dc2fc 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -178,6 +178,8 @@ export function renderBrief( } } + lines.push(...renderDescriptorSection("callerImpact", findings.callerImpact)); + const secretLogs = findings.secretLog ?? []; if (secretLogs.length) { lines.push( diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index d3e4bd61e7..dab5bba089 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -176,6 +176,17 @@ export interface AssetWeightFinding { status: "added" | "grown"; } +/** A changed/removed/renamed exported symbol with live callers in unchanged files, and newly-added dead exports. */ +export interface CallerImpactFinding { + symbol: string; + file: string; + line: number; + kind: "changed" | "removed" | "renamed" | "dead"; + previousSymbol?: string; + /** Paths to outside-PR caller files (best-effort, bounded). */ + callers: string[]; +} + /** A newly-added dependency whose name is a near-miss of a popular package (typosquat) or an unscoped name that * is not published on the public registry and is therefore publicly claimable (dependency-confusion). Reports * the package name + the reason only — never the manifest contents. (#1501) */ @@ -285,6 +296,7 @@ export interface BriefFindings { codeowners?: CodeownersFinding[]; secretLog?: SecretLogFinding[]; assetWeight?: AssetWeightFinding[]; + callerImpact?: CallerImpactFinding[]; typosquat?: TyposquatFinding[]; commitSignature?: CommitSignatureFinding[]; iacMisconfig?: IacMisconfigFinding[]; diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 6ec7364335..386b687000 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -21,6 +21,7 @@ const EXPECTED_ANALYZERS = [ "redos", "provenance", "codeowners", + "callerImpact", "secretLog", "assetWeight", "typosquat", diff --git a/review-enrichment/test/caller-impact.test.ts b/review-enrichment/test/caller-impact.test.ts new file mode 100644 index 0000000000..ac24030bd6 --- /dev/null +++ b/review-enrichment/test/caller-impact.test.ts @@ -0,0 +1,332 @@ +// Units for cross-file caller-impact / dead-symbol analyzer (#1509). Kept standalone so it can evolve +// without bloating enrichment.test.ts. All HTTP is mocked; no external network dependency. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { scanCallerImpact } from "../dist/analyzers/caller-impact.js"; +import { renderBrief } from "../dist/render.js"; + +const jsonResponse = (body, status = 200) => ({ + ok: status >= 200 && status < 300, + status, + text: async () => JSON.stringify(body), +}); + +const textResponse = (text, status = 200) => ({ + ok: status >= 200 && status < 300, + status, + text: async () => text, +}); + +const req = (patches) => ({ + repoFullName: "octo/repo", + prNumber: 1, + headSha: "abc123", + githubToken: "ghp_testtoken", + files: patches, +}); + +const expectSearch = (items) => + async (url) => { + if (url.includes("/search/code")) { + return jsonResponse({ items }); + } + throw new Error(`unexpected request: ${url}`); + }; + +const fileContentsRouter = (contents) => + async (url) => { + for (const [path, text] of contents) { + if (url.includes(path)) return textResponse(text); + } + return undefined; + }; + +const fetchFor = (...handlers) => { + const [searchHandler, ...contentHandlers] = handlers; + return async (url) => { + if (url.includes("/search/code")) { + return searchHandler(url); + } + for (const handler of contentHandlers) { + const out = await handler(url); + if (out) return out; + } + throw new Error(`unhandled request: ${url}`); + }; +}; + +test("scanCallerImpact: changed export flags unchanged callers", async () => { + const findings = await scanCallerImpact( + req([ + { + path: "src/api.ts", + patch: + "@@ -1,2 +1,2 @@\n-export function doThing(a: number) {}\n+export function doThing(a: string) {}", + }, + ]), + fetchFor( + expectSearch([ + { path: "src/consumer.ts" }, + { path: "src/api.ts" }, + ]), + fileContentsRouter([ + ["/contents/src/api.ts?ref=abc123", "export function doThing(a: string) {}"], + ["/contents/src/consumer.ts?ref=abc123", "import { doThing } from './api';\ndoThing('x');"], + ]), + ), + ); + + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "changed"); + assert.equal(findings[0].symbol, "doThing"); + assert.deepEqual(findings[0].callers, ["src/consumer.ts"]); +}); + +test("scanCallerImpact: handles symbol boundaries for dollar-prefixed identifiers", async () => { + const findings = await scanCallerImpact( + req([ + { + path: "src/api.ts", + patch: + "@@ -1,2 +1,2 @@\n-export function $api(a: number) {}\n+export function $api(a: string) {}", + }, + ]), + fetchFor( + expectSearch([{ path: "src/consumer.ts" }]), + fileContentsRouter([ + ["/contents/src/api.ts?ref=abc123", "export function $api(a: string) {}"], + [ + "/contents/src/consumer.ts?ref=abc123", + "import { $api } from './api';\n$api('x');\n", + ], + ]), + ), + ); + + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "changed"); + assert.equal(findings[0].symbol, "$api"); + assert.deepEqual(findings[0].callers, ["src/consumer.ts"]); +}); + +test("scanCallerImpact: handles symbol boundaries for dollar-suffixed identifiers", async () => { + const findings = await scanCallerImpact( + req([ + { + path: "src/api.ts", + patch: + "@@ -1,1 +1,1 @@\n-export function api$(a: string) {}\n+export function api$(a: number) {}", + }, + ]), + fetchFor( + expectSearch([{ path: "src/consumer.ts" }]), + fileContentsRouter([ + ["/contents/src/api.ts?ref=abc123", "export function api$(a: number) {}"], + [ + "/contents/src/consumer.ts?ref=abc123", + "import { api$ } from './api';\napi$('x');\n", + ], + ]), + ), + ); + + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "changed"); + assert.equal(findings[0].symbol, "api$"); + assert.deepEqual(findings[0].callers, ["src/consumer.ts"]); +}); + +test("scanCallerImpact: removed export flags live callsites", async () => { + const findings = await scanCallerImpact( + req([ + { + path: "src/api.ts", + patch: "@@ -1,1 +1,0 @@\n-export function removedFeature() {}", + }, + ]), + fetchFor( + expectSearch([{ path: "src/consumer.ts" }]), + fileContentsRouter([ + ["/contents/src/consumer.ts?ref=abc123", "removedFeature();"], + ]), + ), + ); + + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "removed"); + assert.equal(findings[0].symbol, "removedFeature"); + assert.deepEqual(findings[0].callers, ["src/consumer.ts"]); +}); + +test("scanCallerImpact: renamed export reports old symbol still referenced", async () => { + const findings = await scanCallerImpact( + req([ + { + path: "src/api.ts", + patch: + "@@ -1,1 +1,1 @@\n-export { oldName as OldFeature };\n+export { oldName as NewFeature };", + }, + ]), + fetchFor( + expectSearch([{ path: "src/consumer.ts" }]), + fileContentsRouter([ + ["/contents/src/consumer.ts?ref=abc123", "import { OldFeature } from './api';\nOldFeature();"], + ]), + ), + ); + + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "renamed"); + assert.equal(findings[0].previousSymbol, "OldFeature"); + assert.equal(findings[0].symbol, "NewFeature"); + assert.deepEqual(findings[0].callers, ["src/consumer.ts"]); +}); + +test("scanCallerImpact: dead new export is reported without unchanged callers", async () => { + const findings = await scanCallerImpact( + req([ + { + path: "src/api.ts", + patch: + "@@ -1,0 +1,1 @@\n+export function deadApi() { return 7; }", + }, + ]), + fetchFor( + expectSearch([{ path: "src/consumer.ts" }]), + fileContentsRouter([ + ["/contents/src/consumer.ts?ref=abc123", "export const x = 1;"], + ]), + ), + ); + + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "dead"); + assert.equal(findings[0].symbol, "deadApi"); + assert.deepEqual(findings[0].callers, []); +}); + +test("scanCallerImpact: import-only references are still live callers", async () => { + const findings = await scanCallerImpact( + req([ + { + path: "src/api.ts", + patch: + "@@ -1,1 +1,1 @@\n-export function doThing(a: number) {}\n+export function doThing(a: string) {}", + }, + ]), + fetchFor( + expectSearch([{ path: "src/consumer.ts" }]), + fileContentsRouter([ + ["/contents/src/api.ts?ref=abc123", "export function doThing(a: string) {}"], + [ + "/contents/src/consumer.ts?ref=abc123", + "import { doThing } from './api';", + ], + ]), + ), + ); + + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "changed"); + assert.equal(findings[0].symbol, "doThing"); + assert.deepEqual(findings[0].callers, ["src/consumer.ts"]); +}); + +test("scanCallerImpact: dead detection includes same-file callsites", async () => { + const findings = await scanCallerImpact( + req([ + { + path: "src/api.ts", + patch: + "@@ -1,0 +1,2 @@\n+export function localEntry() { return 1; }\n+localEntry();", + }, + ]), + fetchFor( + expectSearch([{ path: "src/api.ts" }]), + fileContentsRouter([ + ["/contents/src/api.ts?ref=abc123", "export function localEntry() { return 1; }\nlocalEntry();"], + ]), + ), + ); + + assert.equal(findings.length, 0); +}); + +test("scanCallerImpact: dead detection ignores declaration file but not other changed files", async () => { + const findings = await scanCallerImpact( + req([ + { + path: "src/api.ts", + patch: + "@@ -1,0 +1,2 @@\n+export function localEntry() { return 1; }\n+localEntry();", + }, + { + path: "src/consumer.ts", + patch: + "@@ -1,0 +1,2 @@\n+import { localEntry } from './api';\n+localEntry();", + }, + ]), + fetchFor( + expectSearch([{ path: "src/api.ts" }, { path: "src/consumer.ts" }]), + fileContentsRouter([ + [ + "/contents/src/api.ts?ref=abc123", + "export function localEntry() { return 1; }\nlocalEntry();", + ], + [ + "/contents/src/consumer.ts?ref=abc123", + "import { localEntry } from './api';\nlocalEntry();", + ], + ]), + ), + ); + + assert.equal(findings.length, 0); +}); + +test("scanCallerImpact: missing token/head-sha skips analysis", async () => { + const outNoToken = await scanCallerImpact({ + repoFullName: "octo/repo", + prNumber: 1, + files: [{ path: "src/api.ts", patch: "+export function x() {}" }], + headSha: "abc123", + } as never); + assert.deepEqual(outNoToken, []); + + const outNoHead = await scanCallerImpact({ + repoFullName: "octo/repo", + prNumber: 1, + githubToken: "ghp_testtoken", + files: [{ path: "src/api.ts", patch: "+export function x() {}" }], + } as never); + assert.deepEqual(outNoHead, []); +}); + +test("renderBrief includes caller-impact findings", () => { + const { promptSection } = renderBrief({ + callerImpact: [ + { + kind: "renamed", + file: "src/api.ts", + line: 12, + symbol: "NewFeature", + searchSymbol: "OldFeature", + previousSymbol: "OldFeature", + callers: ["src/consumer.ts", "src/widgets.ts"], + }, + { + kind: "dead", + file: "src/api.ts", + line: 27, + symbol: "deadApi", + searchSymbol: "deadApi", + callers: [], + }, + ], + } as never); + + assert.match(promptSection, /Export caller-impact and dead-symbol candidates/); + assert.match(promptSection, /OldFeature/); + assert.match(promptSection, /NewFeature/); + assert.match(promptSection, /deadApi/); +});