Skip to content

refactor(rees): migrate three more analyzers off hand-rolled bounded-fetch onto boundedFetchText #4947

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
loopover-orb merged 1 commit into from
Jul 11, 2026
Merged

refactor(rees): migrate three more analyzers off hand-rolled bounded-fetch onto boundedFetchText #4947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 24 additions & 42 deletions review-enrichment/src/analyzers/caller-impact.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -49,7 +49,7 @@ const SKIP_RE = /(?:\.d\.ts$|\.min\.|(?:^|\/)(?:dist|build|vendor|node_modules)\

interface ScanOptions {
signal?: AbortSignal;
analysis?: Pick<AnalysisContext, "fetchJson">;
analysis?: Pick<AnalysisContext, "fetchJson" | "fetchText">;
diagnostics?: AnalyzerDiagnostics;
}

Expand Down Expand Up @@ -238,53 +238,35 @@ export function candidateCallerPaths(
return out;
}

async function readBoundedText(resp: Response, signal?: AbortSignal): Promise<string | null> {
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,
path: string,
headSha: string,
token: string,
fetchImpl: typeof fetch,
signal: AbortSignal | undefined,
options: ScanOptions,
): Promise<string | null> {
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(
Expand Down Expand Up @@ -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;
};
Expand Down
80 changes: 36 additions & 44 deletions review-enrichment/src/analyzers/undocumented-export.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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://github.com/ghapi";
Expand All @@ -30,6 +33,8 @@ const DIRECTIVE_COMMENT_RE =

interface ScanOptions {
signal?: AbortSignal;
analysis?: Pick<AnalysisContext, "fetchText">;
diagnostics?: AnalyzerDiagnostics;
}

/** Split a `const`/`let`/`var` declarator list on TOP-LEVEL commas, tracking ()/{}/[] depth and string literals so a
Expand Down Expand Up @@ -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<string | null> {
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<string | 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: 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
Expand All @@ -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<string, string> = {
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 }> }> = [];
Expand All @@ -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

Expand Down
66 changes: 24 additions & 42 deletions review-enrichment/src/analyzers/unused-export.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -32,7 +32,7 @@ const SKIP_RE = /(?:\.d\.ts$|\.min\.|(?:^|\/)(?:dist|build|vendor)\/)/;

interface ScanOptions {
signal?: AbortSignal;
analysis?: Pick<AnalysisContext, "fetchJson">;
analysis?: Pick<AnalysisContext, "fetchJson" | "fetchText">;
diagnostics?: AnalyzerDiagnostics;
}

Expand Down Expand Up @@ -84,53 +84,35 @@ export function isDeadOnArrivalFromSearch(
return total === 1;
}

async function readBoundedText(resp: Response, signal?: AbortSignal): Promise<string | null> {
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,
path: string,
headSha: string,
token: string,
fetchImpl: typeof fetch,
signal: AbortSignal | undefined,
options: ScanOptions,
): Promise<string | null> {
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(
Expand Down Expand Up @@ -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;
};
Expand Down
39 changes: 39 additions & 0 deletions review-enrichment/test/caller-impact.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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` }));
Expand Down
22 changes: 22 additions & 0 deletions review-enrichment/test/undocumented-export.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
Loading