Skip to content

Commit 6db1530

Browse files
JSONboredJSONbored
committed
refactor(rees): migrate three analyzers off hand-rolled bounded-fetch onto boundedFetchText (#4759)
doc-comment-drift.ts, exhaustiveness-drift.ts, and complexity-delta.ts each carried their own private "fetch a file at headSha, bounded/streamed read capped at 1MB" helper -- near-byte-identical copies of the same logic, none with a timeout or circuit breaker. duplication-delta.ts (#4741/#4760) already migrated onto the more mature boundedFetchText (external-fetch.ts): a typed ok/failure result, a per-endpoint-category circuit breaker, a configurable timeout, and byte-size capping. Mirror that same fetchFileAtHead call pattern -- including the options.analysis.fetchText fallback for when an AnalysisContext is available -- in all three instead of leaving three more hand-rolled copies to drift further. doc-comment-drift.ts also switches to the shared githubHeaders() helper in place of its own inline auth headers, closing the one pre-existing inconsistency among the three (the other two already used it). Pure fetch-mechanism migration -- verified byte-faithful via each analyzer's existing test suite passing unchanged, plus one new test per file exercising the added options.analysis branch (mirroring duplication-delta.test.ts's own coverage of that path).
1 parent 20c412b commit 6db1530

6 files changed

Lines changed: 183 additions & 130 deletions

File tree

‎review-enrichment/src/analyzers/complexity-delta.ts‎

Lines changed: 27 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@
2727
// that file, never a crash -- checked via plain truthiness, NEVER a strict `=== null` compare (see
2828
// reconstruct-old-content.ts's own doc comment: an empty string is falsy but `!== null`, so a strict-null check
2929
// would wrongly treat a brand-new file's "" as valid before-content).
30-
import type { EnrichRequest, ComplexityDeltaFinding } from "../types.js";
30+
import type { AnalyzerDiagnostics, EnrichRequest, ComplexityDeltaFinding } from "../types.js";
31+
import type { AnalysisContext } from "../analysis-context.js";
32+
import { boundedFetchText } from "../external-fetch.js";
3133
import { githubHeaders } from "../github-headers.js";
3234
import { reconstructOldContent } from "./reconstruct-old-content.js";
3335
import { isJsTsPath, scanContentForComplexity } from "./complexity.js";
@@ -41,56 +43,39 @@ const MAX_FETCH_BYTES = 1_000_000;
4143

4244
interface ScanOptions {
4345
signal?: AbortSignal;
46+
analysis?: Pick<AnalysisContext, "fetchText">;
47+
diagnostics?: AnalyzerDiagnostics;
4448
}
4549

46-
async function readBoundedText(resp: Response, signal?: AbortSignal): Promise<string | null> {
47-
const length = Number(resp.headers.get("content-length"));
48-
if (Number.isFinite(length) && length > MAX_FETCH_BYTES) return null;
49-
if (!resp.body) return null;
50-
51-
const reader = resp.body.getReader();
52-
const decoder = new TextDecoder();
53-
let size = 0;
54-
let text = "";
55-
try {
56-
while (true) {
57-
if (signal?.aborted) return null;
58-
const { done, value } = await reader.read();
59-
if (done) break;
60-
size += value.byteLength;
61-
if (size > MAX_FETCH_BYTES) {
62-
await reader.cancel();
63-
return null;
64-
}
65-
text += decoder.decode(value, { stream: true });
66-
}
67-
text += decoder.decode();
68-
return text;
69-
} finally {
70-
reader.releaseLock();
71-
}
72-
}
73-
50+
/** Fetch a changed file's raw content at `headSha` through the shared bounded-text helper (#4759) — with the
51+
* analysis context's caching/metering when supplied, mirroring `duplication-delta.ts`'s own `fetchFileAtHead`.
52+
* Returns null on any non-OK / oversized / network outcome so the caller fails safe. */
7453
async function fetchFileAtHeadSha(
7554
owner: string,
7655
repo: string,
7756
path: string,
7857
headSha: string,
7958
token: string,
8059
fetchFn: typeof fetch,
81-
signal: AbortSignal | undefined,
60+
options: ScanOptions,
8261
): Promise<string | null> {
83-
try {
84-
const encoded = path.split("/").map(encodeURIComponent).join("/");
85-
const resp = await fetchFn(
86-
`${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encoded}?ref=${encodeURIComponent(headSha)}`,
87-
{ headers: githubHeaders(token, { raw: true }), signal },
88-
);
89-
if (!resp.ok) return null;
90-
return await readBoundedText(resp, signal);
91-
} catch {
92-
return null;
93-
}
62+
const encoded = path.split("/").map(encodeURIComponent).join("/");
63+
const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encoded}?ref=${encodeURIComponent(headSha)}`;
64+
const fetchOptions = {
65+
endpointCategory: "github-contents",
66+
headers: githubHeaders(token, { raw: true }),
67+
signal: options.signal,
68+
fetchImpl: fetchFn,
69+
diagnostics: options.diagnostics,
70+
phase: "complexityDelta",
71+
subcall: "github-contents",
72+
maxBytes: MAX_FETCH_BYTES,
73+
maxCallsPerCategory: MAX_FILES,
74+
};
75+
const response = options.analysis
76+
? await options.analysis.fetchText(url, fetchOptions)
77+
: await boundedFetchText(url, fetchOptions);
78+
return response.ok ? response.data : null;
9479
}
9580

9681
/** Full-file-scan the reconstructed OLD content and the NEW (head) content of one file with
@@ -155,7 +140,7 @@ export async function scanComplexityDelta(
155140
headSha,
156141
githubToken,
157142
fetchFn,
158-
options.signal,
143+
options,
159144
);
160145
if (!headContent) continue;
161146
if (options.signal?.aborted) break; // an abort during the fetch should suppress this file's findings too

‎review-enrichment/src/analyzers/doc-comment-drift.ts‎

Lines changed: 37 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,13 @@
55
// non-parameter signature edit (return type, name, modifier, parameter type) over PRE-EXISTING stale docs a
66
// non-finding. Deliberately conservative: only NAMED `function` declarations whose parameters are confidently
77
// enumerable (any destructuring / non-identifier param → skip the function). Reports symbol + stale params + line.
8-
import type { EnrichRequest, DocCommentDriftFinding } from "../types.js";
8+
import type { AnalyzerDiagnostics, EnrichRequest, DocCommentDriftFinding } from "../types.js";
9+
import type { AnalysisContext } from "../analysis-context.js";
10+
import { boundedFetchText } from "../external-fetch.js";
11+
import { githubHeaders } from "../github-headers.js";
912
import { reconstructOldContent } from "./reconstruct-old-content.js";
1013

14+
const GITHUB_API = "https://api.github.com";
1115
const MAX_FILES = 20;
1216
const MAX_FINDINGS = 50;
1317
const MAX_SIGNATURE_LINES = 40;
@@ -22,34 +26,39 @@ const FUNC_DECL_RE = /^\s*(?:export\s+)?(?:default\s+)?(?:async\s+)?function\s*\
2226

2327
interface ScanOptions {
2428
signal?: AbortSignal;
29+
analysis?: Pick<AnalysisContext, "fetchText">;
30+
diagnostics?: AnalyzerDiagnostics;
2531
}
2632

27-
async function readBoundedText(resp: Response, signal?: AbortSignal): Promise<string | null> {
28-
const length = Number(resp.headers.get("content-length"));
29-
if (Number.isFinite(length) && length > MAX_FETCH_BYTES) return null;
30-
if (!resp.body) return null;
31-
32-
const reader = resp.body.getReader();
33-
const decoder = new TextDecoder();
34-
let size = 0;
35-
let text = "";
36-
try {
37-
while (true) {
38-
if (signal?.aborted) return null;
39-
const { done, value } = await reader.read();
40-
if (done) break;
41-
size += value.byteLength;
42-
if (size > MAX_FETCH_BYTES) {
43-
await reader.cancel();
44-
return null;
45-
}
46-
text += decoder.decode(value, { stream: true });
47-
}
48-
text += decoder.decode();
49-
return text;
50-
} finally {
51-
reader.releaseLock();
52-
}
33+
/** Fetch a changed file's raw content at `headSha` through the shared bounded-text helper (#4759) — with the
34+
* analysis context's caching/metering when supplied, mirroring `duplication-delta.ts`'s own `fetchFileAtHead`.
35+
* Returns null on any non-OK / oversized / network outcome so the caller fails safe. */
36+
async function fetchFileAtHead(
37+
owner: string,
38+
repo: string,
39+
path: string,
40+
headSha: string,
41+
token: string,
42+
fetchFn: typeof fetch,
43+
options: ScanOptions,
44+
): Promise<string | null> {
45+
const encoded = path.split("/").map(encodeURIComponent).join("/");
46+
const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encoded}?ref=${encodeURIComponent(headSha)}`;
47+
const fetchOptions = {
48+
endpointCategory: "github-contents",
49+
headers: githubHeaders(token, { raw: true }),
50+
signal: options.signal,
51+
fetchImpl: fetchFn,
52+
diagnostics: options.diagnostics,
53+
phase: "docCommentDrift",
54+
subcall: "github-contents",
55+
maxBytes: MAX_FETCH_BYTES,
56+
maxCallsPerCategory: MAX_FILES,
57+
};
58+
const response = options.analysis
59+
? await options.analysis.fetchText(url, fetchOptions)
60+
: await boundedFetchText(url, fetchOptions);
61+
return response.ok ? response.data : null;
5362
}
5463

5564
/** Map every named `function NAME` declaration in `content` to its enumerable parameter-name set. A function whose
@@ -312,11 +321,6 @@ export async function scanDocCommentDrift(
312321
const repo = parts[1];
313322
if (parts.length !== 2 || !owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return [];
314323

315-
const headers: Record<string, string> = {
316-
Authorization: `Bearer ${githubToken}`,
317-
Accept: "application/vnd.github.raw",
318-
"X-GitHub-Api-Version": "2022-11-28",
319-
};
320324
const sources = files
321325
.filter((file) => file.patch && SOURCE_RE.test(file.path) && !SKIP_RE.test(file.path))
322326
.slice(0, MAX_FILES);
@@ -325,17 +329,7 @@ export async function scanDocCommentDrift(
325329
for (const file of sources) {
326330
if (options.signal?.aborted) break;
327331

328-
let content: string | null = null;
329-
try {
330-
const path = file.path.split("/").map(encodeURIComponent).join("/");
331-
const resp = await fetchFn(
332-
`https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${path}?ref=${encodeURIComponent(headSha)}`,
333-
{ headers, signal: options.signal },
334-
);
335-
if (resp.ok) content = await readBoundedText(resp, options.signal);
336-
} catch {
337-
content = null;
338-
}
332+
const content = await fetchFileAtHead(owner, repo, file.path, headSha, githubToken, fetchFn, options);
339333
if (!content) continue;
340334
if (options.signal?.aborted) break; // an abort during the fetch should suppress this file's findings too
341335

‎review-enrichment/src/analyzers/exhaustiveness-drift.ts‎

Lines changed: 27 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33
// files and other changed consumer files at headSha (injected fetch), reverse-applies the patch to recover the
44
// pre-PR member set, and only reports high-confidence misses (explicit enum/union cases, no default branch). Bounded
55
// file-fetch caps; fail-safe on missing token/headSha, bad slug, or fetch errors.
6-
import type { EnrichRequest, ExhaustivenessFinding } from "../types.js";
6+
import type { AnalyzerDiagnostics, EnrichRequest, ExhaustivenessFinding } from "../types.js";
7+
import type { AnalysisContext } from "../analysis-context.js";
8+
import { boundedFetchText } from "../external-fetch.js";
79
import { githubHeaders } from "../github-headers.js";
810
import { reconstructOldContent } from "./reconstruct-old-content.js";
911
import { isDiffFileHeaderLine } from "./diff-lines.js";
@@ -28,6 +30,8 @@ const DEFAULT_CASE_RE = /^\s*default\s*:/;
2830

2931
interface ScanOptions {
3032
signal?: AbortSignal;
33+
analysis?: Pick<AnalysisContext, "fetchText">;
34+
diagnostics?: AnalyzerDiagnostics;
3135
}
3236

3337
interface AddedMemberCandidate {
@@ -46,53 +50,35 @@ function isScannablePath(path: string): boolean {
4650
return SOURCE_RE.test(path) && !SKIP_RE.test(path) && !isTestPath(path);
4751
}
4852

49-
async function readBoundedText(resp: Response, signal?: AbortSignal): Promise<string | null> {
50-
const length = Number(resp.headers.get("content-length"));
51-
if (Number.isFinite(length) && length > MAX_FETCH_BYTES) return null;
52-
if (!resp.body) return null;
53-
const reader = resp.body.getReader();
54-
const decoder = new TextDecoder();
55-
let size = 0;
56-
let text = "";
57-
try {
58-
while (true) {
59-
if (signal?.aborted) return null;
60-
const { done, value } = await reader.read();
61-
if (done) break;
62-
size += value.byteLength;
63-
if (size > MAX_FETCH_BYTES) {
64-
await reader.cancel();
65-
return null;
66-
}
67-
text += decoder.decode(value, { stream: true });
68-
}
69-
text += decoder.decode();
70-
return text;
71-
} finally {
72-
reader.releaseLock();
73-
}
74-
}
75-
53+
/** Fetch a changed file's raw content at `headSha` through the shared bounded-text helper (#4759) — with the
54+
* analysis context's caching/metering when supplied, mirroring `duplication-delta.ts`'s own `fetchFileAtHead`.
55+
* Returns null on any non-OK / oversized / network outcome so the caller fails safe. */
7656
async function fetchFileAtHead(
7757
owner: string,
7858
repo: string,
7959
path: string,
8060
headSha: string,
8161
token: string,
8262
fetchFn: typeof fetch,
83-
signal: AbortSignal | undefined,
63+
options: ScanOptions,
8464
): Promise<string | null> {
85-
try {
86-
const encoded = path.split("/").map(encodeURIComponent).join("/");
87-
const resp = await fetchFn(
88-
`${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encoded}?ref=${encodeURIComponent(headSha)}`,
89-
{ headers: githubHeaders(token, { raw: true }), signal },
90-
);
91-
if (!resp.ok) return null;
92-
return await readBoundedText(resp, signal);
93-
} catch {
94-
return null;
95-
}
65+
const encoded = path.split("/").map(encodeURIComponent).join("/");
66+
const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encoded}?ref=${encodeURIComponent(headSha)}`;
67+
const fetchOptions = {
68+
endpointCategory: "github-contents",
69+
headers: githubHeaders(token, { raw: true }),
70+
signal: options.signal,
71+
fetchImpl: fetchFn,
72+
diagnostics: options.diagnostics,
73+
phase: "exhaustiveness",
74+
subcall: "github-contents",
75+
maxBytes: MAX_FETCH_BYTES,
76+
maxCallsPerCategory: MAX_FETCHES,
77+
};
78+
const response = options.analysis
79+
? await options.analysis.fetchText(url, fetchOptions)
80+
: await boundedFetchText(url, fetchOptions);
81+
return response.ok ? response.data : null;
9682
}
9783

9884
/** Walk a unified diff and collect newly added enum/union members with their declaring type name and new-file line. */
@@ -296,7 +282,7 @@ export async function scanExhaustivenessDrift(
296282
return null;
297283
}
298284
fetches += 1;
299-
const content = await fetchFileAtHead(owner, repo, path, headSha, githubToken, fetchFn, options.signal);
285+
const content = await fetchFileAtHead(owner, repo, path, headSha, githubToken, fetchFn, options);
300286
contentCache.set(path, content);
301287
return content;
302288
};

‎review-enrichment/test/complexity-delta.test.ts‎

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,10 @@ test("scanComplexityDelta: stops on an already-aborted signal", async () => {
204204

205205
test("scanComplexityDelta: an abort that becomes true before the body read begins yields no findings for that file", async () => {
206206
// The signal is still false when the per-file loop's pre-fetch check runs, but flips true INSIDE the fetch
207-
// itself -- readBoundedText's own first internal check (not the outer one) must catch this.
207+
// itself, before the Response is even returned. The shared boundedFetchText helper (#4759) has no signal-polling
208+
// of its own inside its read loop -- it just reads whatever Response the mocked fetchImpl hands back, which
209+
// succeeds here regardless of the signal's state -- so it's the loop's OWN post-fetch check that must catch the
210+
// now-true signal and discard this file's content.
208211
const abortController = new AbortController();
209212
const out = await scanComplexityDelta(
210213
baseReq([{ path: "src/a.ts", patch: CALC_PATCH }]),
@@ -218,9 +221,10 @@ test("scanComplexityDelta: an abort that becomes true before the body read begin
218221
});
219222

220223
test("scanComplexityDelta: an abort that fires only after a file's content is fully read stops further files", async () => {
221-
// The signal flips true DURING the body read's final chunk (after readBoundedText's last internal check already
222-
// passed), so readBoundedText itself returns the content successfully -- the OUTER post-fetch check must still
223-
// catch it and stop before a second file is ever fetched.
224+
// The signal flips true DURING the body read's final chunk. The shared boundedFetchText helper (#4759) has no
225+
// signal-polling of its own inside its read loop, so it finishes reading this (mocked, in-memory) stream and
226+
// returns the content successfully -- the loop's OWN post-fetch check must still catch it and stop before a
227+
// second file is ever fetched.
224228
const abortController = new AbortController();
225229
let fetchCalls = 0;
226230
const out = await scanComplexityDelta(
@@ -297,6 +301,36 @@ test("scanComplexityDelta: respects the findings cap across files and stops fetc
297301
assert.equal(fetchCalls, 1); // the cap was hit mid-file-1, so file 2 is never fetched
298302
});
299303

304+
test("scanComplexityDelta: uses the analysis-context fetchText when supplied, instead of the bare fetch path", async () => {
305+
// #4759: the file-content fetch now goes through the shared boundedFetchText helper, which prefers
306+
// options.analysis.fetchText (mirrors duplication-delta.ts's own fetchFileAtHead) when an AnalysisContext is
307+
// supplied — the raw fetchFn passed as the second positional arg must never be invoked in that case.
308+
let analysisCalls = 0;
309+
const analysis = {
310+
fetchText: async (_url, _opts) => {
311+
analysisCalls += 1;
312+
return {
313+
ok: true,
314+
status: 200,
315+
data: HEAD_CONTENT,
316+
bytes: HEAD_CONTENT.length,
317+
elapsedMs: 0,
318+
endpointCategory: "github-contents",
319+
};
320+
},
321+
};
322+
const findings = await scanComplexityDelta(
323+
baseReq([{ path: "src/calc.ts", patch: CALC_PATCH }]),
324+
async () => {
325+
throw new Error("bare fetch should not be used when analysis.fetchText is supplied");
326+
},
327+
{ analysis },
328+
);
329+
assert.equal(analysisCalls, 1);
330+
assert.equal(findings.length, 1);
331+
assert.deepEqual(findings[0], { file: "src/calc.ts", line: 1, name: "calc", before: 5, after: 2, delta: -3 });
332+
});
333+
300334
test("renderBrief emits a public-safe complexity-delta block", () => {
301335
const { promptSection } = renderBrief({
302336
complexityDelta: [{ file: "src/calc.ts", line: 1, name: "calc", before: 5, after: 2, delta: -3 }],

0 commit comments

Comments
 (0)