From 0d918425769a2a06e8db0fe7acea1269f108c378 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 19:21:38 -0700 Subject: [PATCH] refactor(rees): migrate three more analyzers off hand-rolled bounded-fetch onto boundedFetchText undocumented-export.ts, unused-export.ts, and caller-impact.ts each still hand-rolled their own private readBoundedText + fetchFileAtHead wrapper for fetching a changed file's content at headSha, the same pattern doc-comment-drift.ts, exhaustiveness-drift.ts, and complexity-delta.ts had before #4759/PR #4821 migrated them onto the shared boundedFetchText helper. undocumented-export.ts also hand-built its own GitHub auth headers inline instead of the shared githubHeaders() helper, the same inconsistency doc-comment-drift.ts had before that PR. Deletes each file's private readBoundedText outright (never relocated), replaces the fetch path with boundedFetchText / options.analysis.fetchText (mirroring duplication-delta.ts's own fetchFileAtHead), and switches undocumented-export.ts onto githubHeaders(). unused-export.ts and caller-impact.ts already used githubHeaders() and only needed the boundedFetchText migration. No new shared module. Closes #4824 --- .../src/analyzers/caller-impact.ts | 66 ++++++--------- .../src/analyzers/undocumented-export.ts | 80 +++++++++---------- .../src/analyzers/unused-export.ts | 66 ++++++--------- review-enrichment/test/caller-impact.test.ts | 39 +++++++++ .../test/undocumented-export.test.ts | 22 +++++ review-enrichment/test/unused-export.test.ts | 29 +++++++ 6 files changed, 174 insertions(+), 128 deletions(-) diff --git a/review-enrichment/src/analyzers/caller-impact.ts b/review-enrichment/src/analyzers/caller-impact.ts index d25617668f..442cb1adc3 100644 --- a/review-enrichment/src/analyzers/caller-impact.ts +++ b/review-enrichment/src/analyzers/caller-impact.ts @@ -26,7 +26,7 @@ import type { EnrichRequest, } from "../types.js"; import type { AnalysisContext } from "../analysis-context.js"; -import { boundedFetchJson } from "../external-fetch.js"; +import { boundedFetchJson, boundedFetchText } from "../external-fetch.js"; import { githubHeaders } from "../github-headers.js"; import { exportedNames, isPublicEntrypoint } from "./api-break.js"; import { isTestPath } from "./test-ratio.js"; @@ -49,7 +49,7 @@ const SKIP_RE = /(?:\.d\.ts$|\.min\.|(?:^|\/)(?:dist|build|vendor|node_modules)\ interface ScanOptions { signal?: AbortSignal; - analysis?: Pick; + analysis?: Pick; diagnostics?: AnalyzerDiagnostics; } @@ -238,33 +238,9 @@ export function candidateCallerPaths( 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(); - } -} - +/** Fetch a changed file's raw content at `headSha` through the shared bounded-text helper (with the analysis + * context's caching/metering when supplied, mirroring `duplication-delta.ts`'s own `fetchFileAtHead`). Returns + * null on any non-OK / oversized / network outcome so the caller fails safe. */ async function fetchFileAtHead( owner: string, repo: string, @@ -272,19 +248,25 @@ async function fetchFileAtHead( headSha: string, token: string, fetchImpl: typeof fetch, - signal: AbortSignal | undefined, + options: ScanOptions, ): 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, { raw: true }), signal }, - ); - if (!resp.ok) return null; - return await readBoundedText(resp, signal); - } catch { - return null; - } + const encoded = path.split("/").map(encodeURIComponent).join("/"); + const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encoded}?ref=${encodeURIComponent(headSha)}`; + const fetchOptions = { + endpointCategory: "github-contents", + headers: githubHeaders(token, { raw: true }), + signal: options.signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "caller-impact", + subcall: "github-contents", + maxBytes: MAX_FETCH_BYTES, + maxCallsPerCategory: MAX_FILE_FETCHES, + }; + const response = options.analysis + ? await options.analysis.fetchText(url, fetchOptions) + : await boundedFetchText(url, fetchOptions); + return response.ok ? response.data : null; } async function searchSymbolReferences( @@ -344,7 +326,7 @@ export async function scanCallerImpact( return null; } fileFetches += 1; - const content = await fetchFileAtHead(owner, repo, path, headSha, githubToken, fetchFn, options.signal); + const content = await fetchFileAtHead(owner, repo, path, headSha, githubToken, fetchFn, options); fileCache.set(path, content); return content; }; diff --git a/review-enrichment/src/analyzers/undocumented-export.ts b/review-enrichment/src/analyzers/undocumented-export.ts index 30b8846307..251ff3f81d 100644 --- a/review-enrichment/src/analyzers/undocumented-export.ts +++ b/review-enrichment/src/analyzers/undocumented-export.ts @@ -6,7 +6,10 @@ // interface|type|enum NAME` declarations in `index.*` files (re-export lists and `export *` are ignored, since they // aggregate symbols documented at their definition); a missing token/head-sha, an unresolvable repo slug, or any // fetch error yields no finding rather than an error. -import type { EnrichRequest, UndocumentedExportFinding } from "../types.js"; +import type { AnalyzerDiagnostics, EnrichRequest, UndocumentedExportFinding } from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchText } from "../external-fetch.js"; +import { githubHeaders } from "../github-headers.js"; import { isDiffFileHeaderLine } from "./diff-lines.js"; const GITHUB_API = "https://api.github.com"; @@ -30,6 +33,8 @@ const DIRECTIVE_COMMENT_RE = interface ScanOptions { signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; } /** Split a `const`/`let`/`var` declarator list on TOP-LEVEL commas, tracking ()/{}/[] depth and string literals so a @@ -148,31 +153,35 @@ export function hasPrecedingDocComment(lines: string[], lineIndex: number): bool return j >= 0 && lines[j]!.trimStart().startsWith("/**"); } -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(); - } +/** Fetch a changed entrypoint's raw content at `headSha` through the shared bounded-text helper (with the analysis + * context's caching/metering when supplied, mirroring `duplication-delta.ts`'s own `fetchFileAtHead`). Returns + * null on any non-OK / oversized / network outcome so the caller fails safe. */ +async function fetchFileAtHead( + owner: string, + repo: string, + path: string, + headSha: string, + token: string, + fetchFn: typeof fetch, + options: ScanOptions, +): Promise { + const encoded = path.split("/").map(encodeURIComponent).join("/"); + const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encoded}?ref=${encodeURIComponent(headSha)}`; + const fetchOptions = { + endpointCategory: "github-contents", + headers: githubHeaders(token, { raw: true }), + signal: options.signal, + fetchImpl: fetchFn, + diagnostics: options.diagnostics, + phase: "undocumented-export", + subcall: "github-contents", + maxBytes: MAX_FETCH_BYTES, + maxCallsPerCategory: MAX_FILES, + }; + const response = options.analysis + ? await options.analysis.fetchText(url, fetchOptions) + : await boundedFetchText(url, fetchOptions); + return response.ok ? response.data : null; } /** Analyzer entrypoint: for each changed `index.*` entrypoint, fetch it at headSha and flag added exports with no @@ -189,13 +198,6 @@ export async function scanUndocumentedExport( const [owner, repo] = parts; if (parts.length !== 2 || !owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return []; - const headers: Record = { - Authorization: `Bearer ${githubToken}`, - // `vnd.github.raw` returns the file's raw bytes from the Contents API — the same media type the sibling - // github-light analyzer doc-comment-drift.ts uses to fetch a file at headSha. - Accept: "application/vnd.github.raw", - "X-GitHub-Api-Version": "2022-11-28", - }; // Parse added exports FIRST (cheap, pure), then spend the MAX_FILES fetch budget only on entrypoints that actually // have added exports — so index files with no relevant additions can't consume the budget and hide later ones. const candidates: Array<{ file: (typeof files)[number]; added: Array<{ symbol: string; newLine: number }> }> = []; @@ -211,17 +213,7 @@ export async function scanUndocumentedExport( for (const { file, added } of candidates) { if (options.signal?.aborted) break; - let content: string | null = null; - try { - const path = file.path.split("/").map(encodeURIComponent).join("/"); - const resp = await fetchFn( - `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${path}?ref=${encodeURIComponent(headSha)}`, - { headers, signal: options.signal }, - ); - if (resp.ok) content = await readBoundedText(resp, options.signal); - } catch { - content = null; - } + const content = await fetchFileAtHead(owner, repo, file.path, headSha, githubToken, fetchFn, options); if (!content) continue; if (options.signal?.aborted) break; // an abort during the fetch should suppress this file's findings too diff --git a/review-enrichment/src/analyzers/unused-export.ts b/review-enrichment/src/analyzers/unused-export.ts index a8407017ba..e32327fa99 100644 --- a/review-enrichment/src/analyzers/unused-export.ts +++ b/review-enrichment/src/analyzers/unused-export.ts @@ -11,7 +11,7 @@ import type { UnusedExportFinding, } from "../types.js"; import type { AnalysisContext } from "../analysis-context.js"; -import { boundedFetchJson } from "../external-fetch.js"; +import { boundedFetchJson, boundedFetchText } from "../external-fetch.js"; import { githubHeaders } from "../github-headers.js"; import { exportedSymbols, parseAddedExports } from "./undocumented-export.js"; import { isTestPath } from "./test-ratio.js"; @@ -32,7 +32,7 @@ const SKIP_RE = /(?:\.d\.ts$|\.min\.|(?:^|\/)(?:dist|build|vendor)\/)/; interface ScanOptions { signal?: AbortSignal; - analysis?: Pick; + analysis?: Pick; diagnostics?: AnalyzerDiagnostics; } @@ -84,33 +84,9 @@ export function isDeadOnArrivalFromSearch( return total === 1; } -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(); - } -} - +/** Fetch a changed file's raw content at `headSha` through the shared bounded-text helper (with the analysis + * context's caching/metering when supplied, mirroring `duplication-delta.ts`'s own `fetchFileAtHead`). Returns + * null on any non-OK / oversized / network outcome so the caller fails safe. */ async function fetchFileAtHead( owner: string, repo: string, @@ -118,19 +94,25 @@ async function fetchFileAtHead( headSha: string, token: string, fetchImpl: typeof fetch, - signal: AbortSignal | undefined, + options: ScanOptions, ): 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, { raw: true }), signal }, - ); - if (!resp.ok) return null; - return await readBoundedText(resp, signal); - } catch { - return null; - } + const encoded = path.split("/").map(encodeURIComponent).join("/"); + const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encoded}?ref=${encodeURIComponent(headSha)}`; + const fetchOptions = { + endpointCategory: "github-contents", + headers: githubHeaders(token, { raw: true }), + signal: options.signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "unused-export", + subcall: "github-contents", + maxBytes: MAX_FETCH_BYTES, + maxCallsPerCategory: MAX_FILE_FETCHES, + }; + const response = options.analysis + ? await options.analysis.fetchText(url, fetchOptions) + : await boundedFetchText(url, fetchOptions); + return response.ok ? response.data : null; } async function searchSymbolReferences( @@ -194,7 +176,7 @@ export async function scanUnusedExport( return null; } fileFetches += 1; - const content = await fetchFileAtHead(owner, repo, path, headSha, githubToken, fetchFn, options.signal); + const content = await fetchFileAtHead(owner, repo, path, headSha, githubToken, fetchFn, options); fileCache.set(path, content); return content; }; diff --git a/review-enrichment/test/caller-impact.test.ts b/review-enrichment/test/caller-impact.test.ts index 34fa152175..bfb7b818d7 100644 --- a/review-enrichment/test/caller-impact.test.ts +++ b/review-enrichment/test/caller-impact.test.ts @@ -190,6 +190,45 @@ test("scanCallerImpact: flags a removed export confirmed to be imported by an un assert.match(brief, /src\/consumer\.ts/); }); +test("scanCallerImpact: uses the analysis-context fetchJson/fetchText when supplied, instead of the bare fetch path", async () => { + // #4824: the search AND the caller-file-content fetch now go through the shared boundedFetch* helpers, which + // prefer options.analysis.fetchJson/fetchText (mirrors duplication-delta.ts's own fetchFileAtHead) when an + // AnalysisContext is supplied — the raw fetchFn passed as the second positional arg must never be invoked. + resetExternalFetchCircuitBreakerForTest(); + let fetchJsonCalls = 0; + let fetchTextCalls = 0; + const analysis = { + fetchJson: async (_url, _opts) => { + fetchJsonCalls += 1; + return { + ok: true, + status: 200, + data: JSON.parse(searchJson([{ path: "src/consumer.ts" }, { path: "src/utils.ts" }])), + bytes: 0, + elapsedMs: 0, + endpointCategory: "github-code-search-callers", + }; + }, + fetchText: async (_url, _opts) => { + fetchTextCalls += 1; + const data = `import { removedHelper } from "./utils";\nremovedHelper();`; + return { ok: true, status: 200, data, bytes: data.length, elapsedMs: 0, endpointCategory: "github-contents" }; + }, + }; + const findings = await scanCallerImpact( + req([{ path: "src/utils.ts", patch: REMOVED_PATCH }]), + async () => { + throw new Error("bare fetch should not be used when analysis is supplied"); + }, + { analysis }, + ); + assert.equal(fetchJsonCalls, 1); + assert.equal(fetchTextCalls, 1); + assert.deepEqual(findings, [ + { file: "src/utils.ts", line: 2, symbol: "removedHelper", callers: ["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` })); diff --git a/review-enrichment/test/undocumented-export.test.ts b/review-enrichment/test/undocumented-export.test.ts index 8923e5ba2f..eaf82e3032 100644 --- a/review-enrichment/test/undocumented-export.test.ts +++ b/review-enrichment/test/undocumented-export.test.ts @@ -96,6 +96,28 @@ test("scanUndocumentedExport: flags the undocumented export, not the documented assert.match(brief, /undoc/); }); +test("scanUndocumentedExport: uses the analysis-context fetchText when supplied, instead of the bare fetch path", async () => { + // #4824: the entrypoint-content fetch now goes through the shared boundedFetchText helper, which prefers + // options.analysis.fetchText (mirrors duplication-delta.ts's own fetchFileAtHead) when an AnalysisContext is + // supplied — the raw fetchFn passed as the second positional arg must never be invoked in that case. + let analysisCalls = 0; + const analysis = { + fetchText: async (_url, _opts) => { + analysisCalls += 1; + return { ok: true, status: 200, data: HEAD, bytes: HEAD.length, elapsedMs: 0, endpointCategory: "github-contents" }; + }, + }; + const findings = await scanUndocumentedExport( + req([{ path: "src/index.ts", status: "modified", patch: PATCH }]), + async () => { + throw new Error("bare fetch should not be used when analysis.fetchText is supplied"); + }, + { analysis }, + ); + assert.equal(analysisCalls, 1); + assert.deepEqual(findings, [{ file: "src/index.ts", line: 4, symbol: "undoc" }]); +}); + test("scanUndocumentedExport: fetches the entrypoint at the head ref with a per-segment-encoded path", async () => { let calledUrl = ""; const recording = async (url) => { diff --git a/review-enrichment/test/unused-export.test.ts b/review-enrichment/test/unused-export.test.ts index 636f6871b9..573fa3a7e5 100644 --- a/review-enrichment/test/unused-export.test.ts +++ b/review-enrichment/test/unused-export.test.ts @@ -103,6 +103,35 @@ test("scanUnusedExport: does not flag when the head file uses the export locally assert.deepEqual(findings, []); }); +test("scanUnusedExport: uses the analysis-context fetchText for file content when supplied, instead of the bare fetch path", async () => { + // #4824: the file-content fetch now goes through the shared boundedFetchText helper, which prefers + // options.analysis.fetchText (mirrors duplication-delta.ts's own fetchFileAtHead) when an AnalysisContext is + // supplied — the raw fetchFn passed as the second positional arg must never be invoked in that case. The head + // content references the export locally, so referencesSymbolInSource short-circuits before any search is + // attempted — analysis.fetchJson is never called either. + const patch = ["@@ -0,0 +1,2 @@", "+export function helper() {}", "+helper();"].join("\n"); + const head = "export function helper() {}\nhelper();"; + let fetchTextCalls = 0; + const analysis = { + fetchText: async (_url, _opts) => { + fetchTextCalls += 1; + return { ok: true, status: 200, data: head, bytes: head.length, elapsedMs: 0, endpointCategory: "github-contents" }; + }, + fetchJson: async () => { + throw new Error("fetchJson should not be called: the local reference short-circuits before any search"); + }, + }; + const findings = await scanUnusedExport( + req([{ path: "src/util.ts", status: "added", patch }]), + async () => { + throw new Error("bare fetch should not be used when analysis.fetchText is supplied"); + }, + { analysis }, + ); + assert.equal(fetchTextCalls, 1); + assert.deepEqual(findings, []); +}); + test("scanUnusedExport: enforces the maxSearches cap", async () => { const patch = ["@@ -0,0 +1,1 @@", "+export function fn() {}"].join("\n"); const files = Array.from({ length: 12 }, (_, i) => ({