diff --git a/.env.example b/.env.example index 38ee163ad2..b231263faf 100644 --- a/.env.example +++ b/.env.example @@ -70,7 +70,7 @@ GITTENSORY_REVIEW_ENRICHMENT=false # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber # conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y # i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep,revertRecurrence -# coverageDelta +# coverageDelta,callerImpact # # Profile defaults: # fast: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency @@ -85,7 +85,7 @@ GITTENSORY_REVIEW_ENRICHMENT=false # commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology # todoMarker,magicNumber,conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting # errorSwallow,unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak -# deprecatedDep,revertRecurrence,coverageDelta +# deprecatedDep,revertRecurrence,coverageDelta,callerImpact # deep: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency # hardcodedUrl,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat # commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot @@ -93,7 +93,7 @@ GITTENSORY_REVIEW_ENRICHMENT=false # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber # conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y # i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep,revertRecurrence -# coverageDelta +# coverageDelta,callerImpact # 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 e5bd1f7589..feca564a90 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -1321,6 +1321,34 @@ export const REES_ANALYZERS = [ "Conservative and fail-safe: only an added line the report explicitly marks zero-hit is flagged, so a missing token, absent artifact, unparseable report, or fetch error yields no finding rather than a false one. Bounded by run, file, and line caps.", }, }, + { + name: "callerImpact", + title: "Caller impact of removed exports", + category: "quality", + cost: "github-heavy", + defaultEnabled: true, + profiles: ["balanced", "deep"], + requires: ["files", "github-token", "head-sha"], + limits: { + maxSymbols: 6, + maxSearches: 6, + maxFileFetches: 12, + maxCallersPerFinding: 5, + maxFindings: 25, + }, + docs: { + summary: + "Flags an exported symbol the PR removes or renames away from an internal source file that unchanged in-repo files still import — a hidden cross-file compile/runtime break the diff-only reviewer cannot see.", + looksAt: + "Exported declarations dropped on removed (-) diff lines of changed non-entrypoint TS/JS source files, cross-referenced against callers resolved by repo-scoped GitHub Code Search on the default branch and confirmed by fetching each candidate file at headSha.", + reports: + "The removed symbol, its old-file line, and the unchanged caller file paths that still import it — never file contents.", + network: + "One bounded GitHub Code Search query per removed symbol plus bounded contents fetches at headSha to confirm each candidate caller. Requires headSha and GitHub token forwarding for private repos.", + notes: + "Distinct from api-break (entrypoint→downstream, no network) and unused-export (added→dead): this is a removed export that still has callers. A symbol re-added anywhere in the PR is never flagged; only a candidate confirmed to import the symbol from an internal module path counts (a comment, property, or third-party same-named import does not). Fail-safe: any search/fetch error, rate-limit, incomplete or malformed response, or aborted signal yields no finding rather than a fabricated one.", + }, + }, ] as const satisfies readonly ReesAnalyzerDoc[]; export const REES_ANALYZER_NAMES = REES_ANALYZERS.map((analyzer) => analyzer.name); diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index 08a1bc0cbe..6482e449c9 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -1483,6 +1483,36 @@ "network": "Calls the GitHub Actions runs and artifacts APIs and downloads one coverage artifact zip, each bounded by fixed fanout and byte caps. Requires GitHub token forwarding.", "notes": "Conservative and fail-safe: only an added line the report explicitly marks zero-hit is flagged, so a missing token, absent artifact, unparseable report, or fetch error yields no finding rather than a false one. Bounded by run, file, and line caps." } + }, + { + "name": "callerImpact", + "title": "Caller impact of removed exports", + "category": "quality", + "cost": "github-heavy", + "defaultEnabled": true, + "profiles": [ + "balanced", + "deep" + ], + "requires": [ + "files", + "github-token", + "head-sha" + ], + "limits": { + "maxSymbols": 6, + "maxSearches": 6, + "maxFileFetches": 12, + "maxCallersPerFinding": 5, + "maxFindings": 25 + }, + "docs": { + "summary": "Flags an exported symbol the PR removes or renames away from an internal source file that unchanged in-repo files still import — a hidden cross-file compile/runtime break the diff-only reviewer cannot see.", + "looksAt": "Exported declarations dropped on removed (-) diff lines of changed non-entrypoint TS/JS source files, cross-referenced against callers resolved by repo-scoped GitHub Code Search on the default branch and confirmed by fetching each candidate file at headSha.", + "reports": "The removed symbol, its old-file line, and the unchanged caller file paths that still import it — never file contents.", + "network": "One bounded GitHub Code Search query per removed symbol plus bounded contents fetches at headSha to confirm each candidate caller. Requires headSha and GitHub token forwarding for private repos.", + "notes": "Distinct from api-break (entrypoint→downstream, no network) and unused-export (added→dead): this is a removed export that still has callers. A symbol re-added anywhere in the PR is never flagged; only a candidate confirmed to import the symbol from an internal module path counts (a comment, property, or third-party same-named import does not). Fail-safe: any search/fetch error, rate-limit, incomplete or malformed response, or aborted signal yields no finding rather than a fabricated one." + } } ] } diff --git a/review-enrichment/src/analyzers/caller-impact.ts b/review-enrichment/src/analyzers/caller-impact.ts new file mode 100644 index 0000000000..3764312036 --- /dev/null +++ b/review-enrichment/src/analyzers/caller-impact.ts @@ -0,0 +1,363 @@ +// Caller-impact analyzer (#1509, part of #1499). A no-checkout headless reviewer sees only the diff, so it cannot +// tell that a PR REMOVES (or renames away) an exported symbol that OTHER, UNCHANGED files in the same repo still +// IMPORT — a hidden cross-file compile/runtime break. This fills that gap: it parses exported top-level +// declarations dropped on removed (`-`) diff lines of changed NON-entrypoint source files, resolves the symbol's +// callers on the repo's default branch via repo-scoped GitHub Code Search, keeps only files the PR did NOT touch, +// then CONFIRMS each candidate genuinely IMPORTS the symbol (a real named / default / namespace import from an +// INTERNAL module path — never a bare-text hit in a comment, a property access, or a same-named import from a +// third-party package) by fetching the file at headSha. A symbol re-added anywhere in the PR (an in-place edit, +// a move, or a re-export) is never flagged. Reports the removed symbol + the unchanged caller file paths only — +// never source. +// +// DISTINCT from the two already-shipped export analyzers, by design: +// - api-break (#1510): removed exports from a package PUBLIC ENTRYPOINT (barrel) — a DOWNSTREAM/external break; +// deterministic, no network, no caller resolution. Caller-impact owns the NON-entrypoint (internal) files and +// resolves the actual IN-REPO callers over the network. +// - unused-export (#2025): a newly ADDED export with NO callers (dead-on-arrival). Caller-impact is the inverse: +// a REMOVED export that STILL HAS callers. +// +// Fail-closed: a finding requires POSITIVE, verified evidence of a surviving caller. A missing token/headSha, an +// invalid repo slug, a failed / rate-limited / incomplete Code Search, a malformed response, an unreadable +// candidate file, or an aborted signal all resolve to NO finding for that symbol (never a fabricated one) — an +// error in the search or fetch is NEVER surfaced as a caller. Bounded symbol, search, and file-fetch caps. +import type { + AnalyzerDiagnostics, + CallerImpactFinding, + EnrichRequest, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchJson } from "../external-fetch.js"; +import { exportedNames, isPublicEntrypoint } from "./api-break.js"; +import { isTestPath } from "./test-ratio.js"; + +const GITHUB_API = "https://api.github.com"; +const GITHUB_API_VERSION = "2022-11-28"; +const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const MAX_SYMBOLS = 6; // removed symbols searched per PR (Code Search rate budget) +const MAX_SEARCHES = 6; // bounded Code Search queries per PR +const MAX_FILE_FETCHES = 12; // bounded candidate-caller content fetches per PR +const MAX_CALLERS_PER_FINDING = 5; // caller paths listed per finding (keeps the brief bounded) +const MAX_FINDINGS = 25; +const MIN_SYMBOL_LEN = 3; // skip 1-2 char names — too generic to search reliably +const MAX_FETCH_BYTES = 1_000_000; +const MAX_SEARCH_JSON_BYTES = 256 * 1024; +const SEARCH_PER_PAGE = 50; + +const SOURCE_EXTS = new Set(["ts", "tsx", "js", "jsx", "mjs", "cjs", "mts", "cts"]); +const SKIP_RE = /(?:\.d\.ts$|\.min\.|(?:^|\/)(?:dist|build|vendor|node_modules)\/)/; + +interface ScanOptions { + signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; +} + +interface CodeSearchItem { + path?: string; +} + +interface CodeSearchResponse { + total_count?: number; + incomplete_results?: boolean; + items?: CodeSearchItem[]; +} + +interface RemovedExport { + file: string; + symbol: string; + line: number; +} + +function githubHeaders(token: string, raw = false): Record { + return { + Authorization: `Bearer ${token}`, + Accept: raw ? "application/vnd.github.raw" : "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, "\\$&"); +} + +/** A repo-relative source path caller-impact will scan: a real TS/JS source ext, not a declaration/min/build + * artifact, not a test file. Mirrors the sibling unused-export analyzer's scope. Pure. */ +export function isScannablePath(path: string): boolean { + const ext = /\.([^.]+)$/.exec(path)?.[1]?.toLowerCase(); + return Boolean(ext && SOURCE_EXTS.has(ext) && !SKIP_RE.test(path) && !isTestPath(path)); +} + +/** True when `modulePath` is an INTERNAL (in-repo) import specifier — a relative path or a common repo path alias + * (`@/…`, `~/…`, `#…`) — as opposed to a bare npm/scoped-package or `node:` builtin. Restricting caller + * confirmation to internal imports is what rejects a coincidental same-named import from a third-party package. + * Pure. */ +export function isInternalModulePath(modulePath: string): boolean { + return /^(?:\.\.?(?:\/|$)|@\/|~\/|#)/.test(modulePath); +} + +/** True when an import/re-export statement `body` (the text between the `import`/`export` keyword and its `from`) + * BINDS `symbol` — a named specifier `{ symbol }` / `{ symbol as x }` (imported name compared, so an alias still + * matches), a default binding `symbol`, or a namespace `* as symbol`. Pure. */ +export function importBindsSymbol(body: string, symbol: string): boolean { + const brace = /\{([^{}]*)\}/.exec(body); + if (brace) { + for (const raw of brace[1]!.split(",")) { + const spec = raw.trim().replace(/^type\s+/, ""); + if (!spec) continue; + const importedName = spec.split(/\s+as\s+/)[0]!.trim(); + if (importedName === symbol) return true; + } + } + const beforeBrace = body.replace(/\{[^{}]*\}/g, "").replace(/^\s*type\s+/, ""); + for (const raw of beforeBrace.split(",")) { + const tok = raw.trim(); + if (!tok) continue; + const ns = /^\*\s+as\s+([A-Za-z_$][\w$]*)$/.exec(tok); + if (ns) { + if (ns[1] === symbol) return true; + continue; + } + if (/^[A-Za-z_$][\w$]*$/.test(tok) && tok === symbol) return true; + } + return false; +} + +/** True when `source` genuinely IMPORTS `symbol` from an INTERNAL module — the strong signal that this unchanged + * file is a real caller a removal breaks, versus a coincidental text hit (comment, property access, or a + * same-named import from a third-party package). Conservative by design: a caller reaching the symbol only + * through a barrel re-export, a namespace member access, or a bare package path is not matched — a false + * NEGATIVE only suppresses a finding, which is always fail-safe. Pure. */ +export function fileImportsSymbol(source: string, symbol: string): boolean { + if (!source.includes(symbol)) return false; + const stmtRe = /(?:^|[\n;])[ \t]*(?:import|export)\b([\s\S]*?)\bfrom[ \t]*(['"])([^'"]+)\2/g; + let match: RegExpExecArray | null; + while ((match = stmtRe.exec(source)) !== null) { + const body = match[1] ?? ""; + const modulePath = match[3] ?? ""; + if (!isInternalModulePath(modulePath)) continue; + if (importBindsSymbol(body, symbol)) return true; + } + return false; +} + +/** Exported symbols DROPPED on removed (`-`) lines of changed NON-entrypoint source files, keyed to their pre-PR + * (old-file) line, EXCLUDING any name re-exported on an added (`+`) line ANYWHERE in the PR (an in-place edit, + * move, or re-export re-adds the public name, so callers are not broken). The old-file line counter advances + * over removed + context lines (never added lines), mirroring api-break. Deterministic and pure. */ +export function collectRemovedExports( + files: NonNullable, +): RemovedExport[] { + const added = new Set(); + for (const file of files) { + if (!file.patch) continue; + for (const raw of file.patch.split("\n")) { + if (raw.startsWith("+") && !raw.startsWith("+++")) { + for (const name of exportedNames(raw.slice(1))) added.add(name); + } + } + } + + const removed: RemovedExport[] = []; + for (const file of files) { + if (!file.patch || !isScannablePath(file.path) || isPublicEntrypoint(file.path)) continue; + const seen = new Set(); + let oldLine = 0; + let inHunk = false; + for (const raw of file.patch.split("\n")) { + const hunk = /^@@ -(\d+)(?:,\d+)? \+\d+(?:,\d+)? @@/.exec(raw); + if (hunk) { + oldLine = Number(hunk[1]); + inHunk = true; + continue; + } + if (!inHunk) continue; + if (raw.startsWith("+")) continue; // added line: does not advance the old-file counter + if (raw.startsWith("-")) { + if (raw.startsWith("---")) continue; + for (const name of exportedNames(raw.slice(1))) { + if (name === "default" || name.length < MIN_SYMBOL_LEN || seen.has(name)) continue; + seen.add(name); + removed.push({ file: file.path, symbol: name, line: oldLine }); + } + oldLine++; + } else if (!raw.startsWith("\\")) { + oldLine++; + } + } + } + return removed.filter((entry) => !added.has(entry.symbol)); +} + +/** Unchanged in-repo candidate caller paths from a Code Search response for a removed symbol: scannable source + * files that are NOT the declaring file and NOT touched by the PR. Returns `null` when the response is UNUSABLE + * (missing, incomplete, or malformed) so the caller SUPPRESSES the finding — a failed search is an explicit + * unknown state, never "no callers". An empty array means the search succeeded but found no external caller. */ +export function candidateCallerPaths( + response: CodeSearchResponse | null, + declaringFile: string, + changedPaths: ReadonlySet, +): string[] | null { + if (!response || response.incomplete_results) return null; + if (!Array.isArray(response.items)) return null; + const out: string[] = []; + for (const item of response.items) { + const path = item?.path; + if (typeof path !== "string" || !path) continue; + if (path === declaringFile || changedPaths.has(path) || !isScannablePath(path)) continue; + if (!out.includes(path)) out.push(path); + } + return out; +} + +async function readBoundedText(resp: Response, signal?: AbortSignal): Promise { + const length = Number(resp.headers.get("content-length")); + if (Number.isFinite(length) && length > MAX_FETCH_BYTES) return null; + if (!resp.body) return null; + const reader = resp.body.getReader(); + const decoder = new TextDecoder(); + let size = 0; + let text = ""; + try { + while (true) { + if (signal?.aborted) return null; + const { done, value } = await reader.read(); + if (done) break; + size += value.byteLength; + if (size > MAX_FETCH_BYTES) { + await reader.cancel(); + return null; + } + text += decoder.decode(value, { stream: true }); + } + text += decoder.decode(); + return text; + } finally { + reader.releaseLock(); + } +} + +async function fetchFileAtHead( + owner: string, + repo: string, + path: string, + headSha: string, + token: string, + fetchImpl: typeof fetch, + signal: AbortSignal | undefined, +): Promise { + try { + const encoded = path.split("/").map(encodeURIComponent).join("/"); + const resp = await fetchImpl( + `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encoded}?ref=${encodeURIComponent(headSha)}`, + { headers: githubHeaders(token, true), signal }, + ); + if (!resp.ok) return null; + return await readBoundedText(resp, signal); + } catch { + return null; + } +} + +async function searchSymbolReferences( + owner: string, + repo: string, + symbol: string, + token: string, + fetchImpl: typeof fetch, + options: ScanOptions, +): Promise { + const q = `"${symbol}" repo:${owner}/${repo}`; + const url = `${GITHUB_API}/search/code?q=${encodeURIComponent(q)}&per_page=${SEARCH_PER_PAGE}`; + const fetchOptions = { + endpointCategory: "github-code-search-callers", + headers: githubHeaders(token), + signal: options.signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "caller-impact", + subcall: "code-search", + maxBytes: MAX_SEARCH_JSON_BYTES, + maxCallsPerCategory: MAX_SEARCHES, + }; + const response = options.analysis + ? await options.analysis.fetchJson(url, fetchOptions) + : await boundedFetchJson(url, fetchOptions); + return response.ok ? response.data : null; +} + +/** Analyzer entrypoint: flag exported symbols the PR REMOVES from an internal source file that unchanged in-repo + * files still import (a hidden cross-file break). Fail-safe — returns [] on missing token/headSha, invalid slug, + * no removed exports, or when no caller can be POSITIVELY confirmed; every search/fetch error degrades to no + * finding. Bounded by symbol, search, file-fetch, caller, and finding caps. */ +export async function scanCallerImpact( + req: EnrichRequest, + fetchFn: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + if (options.signal?.aborted) return []; + const { repoFullName, githubToken, headSha, files = [] } = req; + if (!githubToken || !headSha) return []; + const parts = repoFullName.split("/"); + const [owner, repo] = parts; + if (parts.length !== 2 || !owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return []; + + const removed = collectRemovedExports(files).slice(0, MAX_SYMBOLS); + if (!removed.length) return []; + + const changedPaths = new Set(files.map((file) => file.path)); + + const fileCache = new Map(); + let fileFetches = 0; + const loadFile = async (path: string): Promise => { + if (fileCache.has(path)) return fileCache.get(path) ?? null; + if (fileFetches >= MAX_FILE_FETCHES) { + fileCache.set(path, null); + return null; + } + fileFetches += 1; + const content = await fetchFileAtHead(owner, repo, path, headSha, githubToken, fetchFn, options.signal); + fileCache.set(path, content); + return content; + }; + + const findings: CallerImpactFinding[] = []; + let searches = 0; + for (const candidate of removed) { + if (options.signal?.aborted) break; + if (searches >= MAX_SEARCHES) break; + + let response: CodeSearchResponse | null = null; + try { + response = await searchSymbolReferences(owner, repo, candidate.symbol, githubToken, fetchFn, options); + } catch { + response = null; + } + searches += 1; + + // A `null` result is an EXPLICIT unknown (search failed / rate-limited / malformed / incomplete); an empty + // list is a successful "no external caller". Both suppress the finding — an error is never a caller. + const candidatePaths = candidateCallerPaths(response, candidate.file, changedPaths); + if (candidatePaths === null || candidatePaths.length === 0) continue; + + const callers: string[] = []; + for (const path of candidatePaths) { + if (options.signal?.aborted) break; + if (callers.length >= MAX_CALLERS_PER_FINDING) break; + const content = await loadFile(path); + if (content === null) continue; // unreadable candidate → cannot confirm → skip (fail-safe) + if (fileImportsSymbol(content, candidate.symbol)) callers.push(path); + } + + // Emit ONLY on a positively-verified surviving caller. Zero confirmed callers ⇒ no finding. + if (!callers.length) continue; + findings.push({ + file: candidate.file, + line: candidate.line, + symbol: candidate.symbol, + callers, + }); + if (findings.length >= MAX_FINDINGS) break; + } + return findings; +} diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 330a8ea2d1..152b6fd202 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -51,6 +51,7 @@ import { scanApiBreak } from "./api-break.js"; import { scanDeprecatedDependencies } from "./deprecated-dep.js"; import { scanRevertRecurrence } from "./revert-recurrence.js"; import { scanCoverageDelta } from "./coverage-delta.js"; +import { scanCallerImpact } from "./caller-impact.js"; import type { AnalyzerDescriptor, AnalyzerFn, @@ -1535,6 +1536,50 @@ export const ANALYZER_DESCRIPTORS = [ run: (req, { signal, analysis, diagnostics }) => scanCoverageDelta(req, fetch, { signal, analysis, diagnostics }), }), + descriptor({ + name: "callerImpact", + title: "Caller impact of removed exports", + category: "quality", + cost: "github-heavy", + defaultEnabled: true, + requires: ["files", "github-token", "head-sha"], + limits: { + maxSymbols: 6, + maxSearches: 6, + maxFileFetches: 12, + maxCallersPerFinding: 5, + maxFindings: 25, + }, + docs: { + summary: + "Flags an exported symbol the PR removes or renames away from an internal source file that unchanged in-repo files still import — a hidden cross-file compile/runtime break the diff-only reviewer cannot see.", + looksAt: + "Exported declarations dropped on removed (-) diff lines of changed non-entrypoint TS/JS source files, cross-referenced against callers resolved by repo-scoped GitHub Code Search on the default branch and confirmed by fetching each candidate file at headSha.", + reports: + "The removed symbol, its old-file line, and the unchanged caller file paths that still import it — never file contents.", + network: + "One bounded GitHub Code Search query per removed symbol plus bounded contents fetches at headSha to confirm each candidate caller. Requires headSha and GitHub token forwarding for private repos.", + notes: + "Distinct from api-break (entrypoint→downstream, no network) and unused-export (added→dead): this is a removed export that still has callers. A symbol re-added anywhere in the PR is never flagged; only a candidate confirmed to import the symbol from an internal module path counts (a comment, property, or third-party same-named import does not). Fail-safe: any search/fetch error, rate-limit, incomplete or malformed response, or aborted signal yields no finding rather than a fabricated one.", + }, + render: (findings, helpers) => { + if (!findings.length) return []; + const lines = [ + "### Removed exports with live callers (hidden cross-file break in unchanged files)", + ]; + for (const item of findings) { + const callers = item.callers + .map((caller) => helpers.safeCodeSpan(caller)) + .join(", "); + lines.push( + `- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} removes ${helpers.safeCodeSpan(item.symbol)}, still imported by ${callers}`, + ); + } + return lines; + }, + run: (req, { signal, analysis, diagnostics }) => + scanCallerImpact(req, fetch, { signal, analysis, diagnostics }), + }), ] as const satisfies readonly AnyAnalyzerDescriptor[]; export const ANALYZER_NAMES = ANALYZER_DESCRIPTORS.map( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 022b0fd3cb..5cd5c6c97d 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -502,6 +502,7 @@ export function renderBrief( lines.push(...renderDescriptorSection("deprecatedDep", findings.deprecatedDep)); lines.push(...renderDescriptorSection("revertRecurrence", findings.revertRecurrence)); lines.push(...renderDescriptorSection("coverageDelta", findings.coverageDelta)); + lines.push(...renderDescriptorSection("callerImpact", findings.callerImpact)); if (!lines.length) return { promptSection: "", systemSuffix: "" }; diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 9a877404da..5891e4ce13 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -640,6 +640,21 @@ export interface CoverageDeltaFinding { uncoveredLines: number[]; } +/** An exported symbol the PR REMOVES (or renames away) from an INTERNAL source file that UNCHANGED in-repo files + * still import — a hidden cross-file compile/runtime break the no-checkout reviewer cannot see (#1509, part of + * #1499). Callers are resolved on the default branch via repo-scoped GitHub Code Search, filtered to files the PR + * did not touch, and each is CONFIRMED to genuinely import the symbol from an internal module (never a text/ + * comment/property hit). Distinct from api-break (#1510, entrypoint→downstream) and unused-export (#2025, added→ + * dead). Reports the removed symbol, its old-file line, and the unchanged caller file paths only — never code. */ +export interface CallerImpactFinding { + file: string; + /** Old-file line of the removed export declaration in the changed file. */ + line: number; + symbol: string; + /** Unchanged in-repo files confirmed to still import the removed symbol (capped). */ + callers: string[]; +} + export interface BriefFindings { dependency?: DependencyFinding[]; dependencyDiff?: DependencyDiffFinding[]; @@ -694,6 +709,7 @@ export interface BriefFindings { deprecatedDep?: DeprecatedDependencyFinding[]; revertRecurrence?: RevertRecurrenceFinding[]; coverageDelta?: CoverageDeltaFinding[]; + callerImpact?: CallerImpactFinding[]; } /** A JSDoc/TSDoc block whose `@param` tags name parameters the adjacent function no longer declares — a diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 9edf4b1408..1042b62161 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -63,6 +63,7 @@ const EXPECTED_ANALYZERS = [ "deprecatedDep", "revertRecurrence", "coverageDelta", + "callerImpact", ]; test("analyzer descriptors cover the runtime registry in stable order", () => { diff --git a/review-enrichment/test/caller-impact.test.ts b/review-enrichment/test/caller-impact.test.ts new file mode 100644 index 0000000000..3984742b10 --- /dev/null +++ b/review-enrichment/test/caller-impact.test.ts @@ -0,0 +1,347 @@ +// Units for the caller-impact analyzer (#1509). Own file (not enrichment.test.ts) so concurrent analyzer PRs do +// not collide. All network is mocked; runs against the compiled dist/. The external-fetch circuit breaker is +// module-global, so every test that performs a search resets it first for isolation. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + candidateCallerPaths, + collectRemovedExports, + fileImportsSymbol, + importBindsSymbol, + isInternalModulePath, + isScannablePath, + scanCallerImpact, +} from "../dist/analyzers/caller-impact.js"; +import { renderBrief } from "../dist/render.js"; +import { resetExternalFetchCircuitBreakerForTest } from "../dist/external-fetch.js"; + +const REMOVED_PATCH = [ + "@@ -1,3 +1,2 @@", + " const keep = 1;", + "-export function removedHelper() {}", + " const tail = 2;", +].join("\n"); + +const searchJson = (items, { total, incomplete = false } = {}) => + JSON.stringify({ + total_count: total ?? items.length, + incomplete_results: incomplete, + items, + }); + +const req = (files, extra = {}) => ({ + repoFullName: "octo/repo", + prNumber: 1, + githubToken: "ghp_test", + headSha: "abc123", + files, + ...extra, +}); + +// A fetch stub: `search` is the JSON body (or a Response) for /search/code; `contents` maps a path substring to +// a body (or a Response). Unmapped contents requests 404. +const stubFetch = ({ search, contents = {} }) => + async (url) => { + if (url.includes("/search/code")) { + return search instanceof Response ? search : new Response(search, { status: 200 }); + } + if (url.includes("/contents/")) { + for (const [needle, body] of Object.entries(contents)) { + if (url.includes(needle)) { + return body instanceof Response ? body : new Response(body, { status: 200 }); + } + } + return new Response("", { status: 404 }); + } + return new Response("", { status: 404 }); + }; + +// --------------------------------------------------------------------------------------------------------------- +// Pure helpers +// --------------------------------------------------------------------------------------------------------------- + +test("isScannablePath: real source only, excludes decl/build/test", () => { + assert.equal(isScannablePath("src/utils.ts"), true); + assert.equal(isScannablePath("src/a.tsx"), true); + assert.equal(isScannablePath("src/types.d.ts"), false); + assert.equal(isScannablePath("dist/utils.js"), false); + assert.equal(isScannablePath("src/utils.test.ts"), false); + assert.equal(isScannablePath("README.md"), false); +}); + +test("isInternalModulePath: relative and alias are internal; bare/builtin are not", () => { + for (const p of ["./x", "../x", ".", "..", "@/x", "~/x", "#internal"]) { + assert.equal(isInternalModulePath(p), true, p); + } + for (const p of ["react", "@scope/pkg", "node:fs", "lodash/merge"]) { + assert.equal(isInternalModulePath(p), false, p); + } +}); + +test("importBindsSymbol: named (incl. alias), default, and namespace bindings", () => { + assert.equal(importBindsSymbol(" { foo, bar } ", "foo"), true); + assert.equal(importBindsSymbol(" { foo as local } ", "foo"), true); // imported name matched, not the alias + assert.equal(importBindsSymbol(" { other as foo } ", "foo"), false); // foo is only a local alias here + assert.equal(importBindsSymbol(" type { foo } ", "foo"), true); + assert.equal(importBindsSymbol(" * as foo ", "foo"), true); + assert.equal(importBindsSymbol(" foo ", "foo"), true); + assert.equal(importBindsSymbol(" foo, { bar } ", "foo"), true); + assert.equal(importBindsSymbol(" { bar } ", "foo"), false); +}); + +test("fileImportsSymbol: only a real internal import counts; text/property/bare-pkg do not", () => { + assert.equal(fileImportsSymbol(`import { foo } from "./m";\nfoo();`, "foo"), true); + assert.equal(fileImportsSymbol(`import { foo as f } from "../m";`, "foo"), true); + assert.equal(fileImportsSymbol(`export { foo } from "@/m";`, "foo"), true); + assert.equal(fileImportsSymbol(`import foo from "#internal";`, "foo"), true); + assert.equal(fileImportsSymbol(`import { foo } from "third-party";`, "foo"), false); // bare package + assert.equal(fileImportsSymbol(`// foo is used elsewhere\nconst x = obj.foo;`, "foo"), false); // comment/property + assert.equal(fileImportsSymbol(`const foo = 1;`, "foo"), false); // local declaration + assert.equal(fileImportsSymbol(`import { bar } from "./m";`, "foo"), false); +}); + +test("collectRemovedExports: removed export keyed to old-file line", () => { + const removed = collectRemovedExports([{ path: "src/utils.ts", patch: REMOVED_PATCH }]); + assert.deepEqual(removed, [{ file: "src/utils.ts", symbol: "removedHelper", line: 2 }]); +}); + +test("collectRemovedExports: a symbol re-added anywhere in the PR (move/edit) is not removed", () => { + const removed = collectRemovedExports([ + { path: "src/utils.ts", patch: REMOVED_PATCH.replace("removedHelper", "movedHelper") }, + { path: "src/moved.ts", patch: ["@@ -0,0 +1,1 @@", "+export function movedHelper() {}"].join("\n") }, + ]); + assert.deepEqual(removed, []); +}); + +test("collectRemovedExports: skips entrypoint barrels, tests, short names, and default", () => { + const barrel = collectRemovedExports([ + { path: "src/index.ts", patch: REMOVED_PATCH.replace("removedHelper", "barrelFn") }, + ]); + assert.deepEqual(barrel, []); + const testFile = collectRemovedExports([ + { path: "src/utils.test.ts", patch: REMOVED_PATCH.replace("removedHelper", "specFn") }, + ]); + assert.deepEqual(testFile, []); + const shortAndDefault = collectRemovedExports([ + { path: "src/utils.ts", patch: ["@@ -1,2 +1,1 @@", "-export const ab = 1;", "-export default x;"].join("\n") }, + ]); + assert.deepEqual(shortAndDefault, []); +}); + +test("collectRemovedExports: nothing removed yields no candidates", () => { + assert.deepEqual( + collectRemovedExports([ + { path: "src/utils.ts", patch: ["@@ -0,0 +1,1 @@", "+export function added() {}"].join("\n") }, + ]), + [], + ); +}); + +test("candidateCallerPaths: null (unknown) on missing/incomplete/malformed; filters declaring/changed/non-source", () => { + const changed = new Set(["src/utils.ts", "src/also-changed.ts"]); + assert.equal(candidateCallerPaths(null, "src/utils.ts", changed), null); + assert.equal( + candidateCallerPaths({ total_count: 1, incomplete_results: true, items: [{ path: "src/a.ts" }] }, "src/utils.ts", changed), + null, + ); + assert.equal(candidateCallerPaths({ total_count: 1 }, "src/utils.ts", changed), null); // items not an array + assert.deepEqual( + candidateCallerPaths( + { + items: [ + { path: "src/utils.ts" }, // declaring file + { path: "src/also-changed.ts" }, // changed by the PR + { path: "src/gen.d.ts" }, // not scannable + { path: "src/consumer.ts" }, + { path: "src/consumer.ts" }, // duplicate + { path: null }, + ], + }, + "src/utils.ts", + changed, + ), + ["src/consumer.ts"], + ); +}); + +// --------------------------------------------------------------------------------------------------------------- +// scanCallerImpact — happy path + render +// --------------------------------------------------------------------------------------------------------------- + +test("scanCallerImpact: flags a removed export confirmed to be imported by an unchanged file", async () => { + resetExternalFetchCircuitBreakerForTest(); + const fetchFn = stubFetch({ + search: searchJson([{ path: "src/consumer.ts" }, { path: "src/utils.ts" }]), + contents: { "consumer.ts": `import { removedHelper } from "./utils";\nremovedHelper();` }, + }); + const findings = await scanCallerImpact(req([{ path: "src/utils.ts", patch: REMOVED_PATCH }]), fetchFn); + assert.deepEqual(findings, [ + { file: "src/utils.ts", line: 2, symbol: "removedHelper", callers: ["src/consumer.ts"] }, + ]); + const brief = renderBrief({ callerImpact: findings }).promptSection; + assert.match(brief, /Removed exports? with live callers/i); + assert.match(brief, /removedHelper/); + assert.match(brief, /src\/consumer\.ts/); +}); + +test("scanCallerImpact: caller list is capped at MAX_CALLERS_PER_FINDING (5)", async () => { + resetExternalFetchCircuitBreakerForTest(); + const items = Array.from({ length: 7 }, (_, i) => ({ path: `src/c${i}.ts` })); + const contents = Object.fromEntries( + items.map((it) => [it.path.split("/").pop(), `import { removedHelper } from "../utils";`]), + ); + const findings = await scanCallerImpact( + req([{ path: "src/utils.ts", patch: REMOVED_PATCH }]), + stubFetch({ search: searchJson(items), contents }), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].callers.length, 5); +}); + +// --------------------------------------------------------------------------------------------------------------- +// scanCallerImpact — fail-closed / no-finding branches +// --------------------------------------------------------------------------------------------------------------- + +test("scanCallerImpact: a text-only / property / comment match is NOT a caller", async () => { + resetExternalFetchCircuitBreakerForTest(); + const findings = await scanCallerImpact( + req([{ path: "src/utils.ts", patch: REMOVED_PATCH }]), + stubFetch({ + search: searchJson([{ path: "src/consumer.ts" }]), + contents: { "consumer.ts": `// removedHelper was here\nconst v = ns.removedHelper;` }, + }), + ); + assert.deepEqual(findings, []); +}); + +test("scanCallerImpact: a same-named import from a THIRD-PARTY package is NOT a caller", async () => { + resetExternalFetchCircuitBreakerForTest(); + const findings = await scanCallerImpact( + req([{ path: "src/utils.ts", patch: REMOVED_PATCH }]), + stubFetch({ + search: searchJson([{ path: "src/consumer.ts" }]), + contents: { "consumer.ts": `import { removedHelper } from "some-pkg";\nremovedHelper();` }, + }), + ); + assert.deepEqual(findings, []); +}); + +test("scanCallerImpact: a FAILED search (HTTP 500) degrades to no finding — never a fabricated caller", async () => { + resetExternalFetchCircuitBreakerForTest(); + const fetchFn = async (url) => { + if (url.includes("/search/code")) return new Response("upstream error", { status: 500 }); + return new Response("", { status: 404 }); + }; + const findings = await scanCallerImpact(req([{ path: "src/utils.ts", patch: REMOVED_PATCH }]), fetchFn); + assert.deepEqual(findings, []); +}); + +test("scanCallerImpact: a MALFORMED search body (invalid JSON) degrades to no finding", async () => { + resetExternalFetchCircuitBreakerForTest(); + const findings = await scanCallerImpact( + req([{ path: "src/utils.ts", patch: REMOVED_PATCH }]), + stubFetch({ search: "}{ not json" }), + ); + assert.deepEqual(findings, []); +}); + +test("scanCallerImpact: incomplete_results (partial index) degrades to no finding", async () => { + resetExternalFetchCircuitBreakerForTest(); + const findings = await scanCallerImpact( + req([{ path: "src/utils.ts", patch: REMOVED_PATCH }]), + stubFetch({ + search: searchJson([{ path: "src/consumer.ts" }], { total: 9, incomplete: true }), + contents: { "consumer.ts": `import { removedHelper } from "./utils";` }, + }), + ); + assert.deepEqual(findings, []); +}); + +test("scanCallerImpact: a thrown fetch (network error) degrades to no finding", async () => { + resetExternalFetchCircuitBreakerForTest(); + const fetchFn = async (url) => { + if (url.includes("/search/code")) throw new Error("boom"); + return new Response("", { status: 404 }); + }; + const findings = await scanCallerImpact(req([{ path: "src/utils.ts", patch: REMOVED_PATCH }]), fetchFn); + assert.deepEqual(findings, []); +}); + +test("scanCallerImpact: an UNREADABLE candidate file (404 contents) is not counted as a caller", async () => { + resetExternalFetchCircuitBreakerForTest(); + const fetchFn = async (url) => { + if (url.includes("/search/code")) return new Response(searchJson([{ path: "src/consumer.ts" }]), { status: 200 }); + return new Response("", { status: 404 }); // contents 404 + }; + const findings = await scanCallerImpact(req([{ path: "src/utils.ts", patch: REMOVED_PATCH }]), fetchFn); + assert.deepEqual(findings, []); +}); + +test("scanCallerImpact: search with only the declaring + changed files yields no external caller", async () => { + resetExternalFetchCircuitBreakerForTest(); + const findings = await scanCallerImpact( + req([ + { path: "src/utils.ts", patch: REMOVED_PATCH }, + { path: "src/also-changed.ts", patch: ["@@ -0,0 +1,1 @@", "+const q = 1;"].join("\n") }, + ]), + stubFetch({ search: searchJson([{ path: "src/utils.ts" }, { path: "src/also-changed.ts" }]) }), + ); + assert.deepEqual(findings, []); +}); + +test("scanCallerImpact: aborted signal returns [] without any finding", async () => { + resetExternalFetchCircuitBreakerForTest(); + let searched = false; + const fetchFn = async (url) => { + if (url.includes("/search/code")) searched = true; + return new Response(searchJson([{ path: "src/consumer.ts" }]), { status: 200 }); + }; + const findings = await scanCallerImpact( + req([{ path: "src/utils.ts", patch: REMOVED_PATCH }]), + fetchFn, + { signal: AbortSignal.abort() }, + ); + assert.deepEqual(findings, []); + assert.equal(searched, false); +}); + +test("scanCallerImpact: no token / no headSha / invalid slug / no removed exports all return []", async () => { + const failFetch = async () => new Response("", { status: 500 }); + const files = [{ path: "src/utils.ts", patch: REMOVED_PATCH }]; + assert.deepEqual(await scanCallerImpact(req(files, { githubToken: undefined }), failFetch), []); + assert.deepEqual(await scanCallerImpact(req(files, { headSha: undefined }), failFetch), []); + assert.deepEqual(await scanCallerImpact(req(files, { repoFullName: "not-a-slug" }), failFetch), []); + assert.deepEqual( + await scanCallerImpact(req(files, { repoFullName: "octo/re po" }), failFetch), + [], + ); + assert.deepEqual( + await scanCallerImpact( + req([{ path: "src/utils.ts", patch: ["@@ -0,0 +1,1 @@", "+export function added() {}"].join("\n") }]), + failFetch, + ), + [], + ); +}); + +test("scanCallerImpact: enforces the search cap at MAX_SEARCHES (6)", async () => { + resetExternalFetchCircuitBreakerForTest(); + const files = Array.from({ length: 9 }, (_, i) => ({ + path: `src/f${i}.ts`, + patch: REMOVED_PATCH.replace("removedHelper", `removedHelper${i}`), + })); + let searches = 0; + const fetchFn = async (url) => { + if (url.includes("/search/code")) { + searches += 1; + return new Response(searchJson([]), { status: 200 }); + } + return new Response("", { status: 404 }); + }; + await scanCallerImpact(req(files), fetchFn); + assert.equal(searches, 6); +}); + +test("renderBrief: omits the caller-impact section when there are no findings", () => { + assert.equal(renderBrief({ callerImpact: [] }).promptSection, ""); +}); diff --git a/src/review/enrichment-analyzer-names.ts b/src/review/enrichment-analyzer-names.ts index 0760f1b218..3a8b7d47a1 100644 --- a/src/review/enrichment-analyzer-names.ts +++ b/src/review/enrichment-analyzer-names.ts @@ -57,6 +57,7 @@ export const REES_ANALYZER_NAMES = [ "deprecatedDep", "revertRecurrence", "coverageDelta", + "callerImpact", ] as const; export type ReesAnalyzerName = (typeof REES_ANALYZER_NAMES)[number];