Skip to content

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

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 analyzers off hand-rolled bounded-fetch onto boundedFetchText #4821

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
69 changes: 27 additions & 42 deletions review-enrichment/src/analyzers/complexity-delta.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
// that file, never a crash -- checked via plain truthiness, NEVER a strict `=== null` compare (see
// reconstruct-old-content.ts's own doc comment: an empty string is falsy but `!== null`, so a strict-null check
// would wrongly treat a brand-new file's "" as valid before-content).
import type { EnrichRequest, ComplexityDeltaFinding } from "../types.js";
import type { AnalyzerDiagnostics, EnrichRequest, ComplexityDeltaFinding } from "../types.js";
import type { AnalysisContext } from "../analysis-context.js";
import { boundedFetchText } from "../external-fetch.js";
import { githubHeaders } from "../github-headers.js";
import { reconstructOldContent } from "./reconstruct-old-content.js";
import { isJsTsPath, scanContentForComplexity } from "./complexity.js";
Expand All @@ -41,56 +43,39 @@ const MAX_FETCH_BYTES = 1_000_000;

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

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 (#4759) — 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 fetchFileAtHeadSha(
owner: string,
repo: string,
path: string,
headSha: string,
token: string,
fetchFn: typeof fetch,
signal: AbortSignal | undefined,
options: ScanOptions,
): Promise<string | null> {
try {
const encoded = path.split("/").map(encodeURIComponent).join("/");
const resp = await fetchFn(
`${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: fetchFn,
diagnostics: options.diagnostics,
phase: "complexityDelta",
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;
}

/** Full-file-scan the reconstructed OLD content and the NEW (head) content of one file with
Expand Down Expand Up @@ -155,7 +140,7 @@ export async function scanComplexityDelta(
headSha,
githubToken,
fetchFn,
options.signal,
options,
);
if (!headContent) continue;
if (options.signal?.aborted) break; // an abort during the fetch should suppress this file's findings too
Expand Down
80 changes: 37 additions & 43 deletions review-enrichment/src/analyzers/doc-comment-drift.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,13 @@
// non-parameter signature edit (return type, name, modifier, parameter type) over PRE-EXISTING stale docs a
// non-finding. Deliberately conservative: only NAMED `function` declarations whose parameters are confidently
// enumerable (any destructuring / non-identifier param → skip the function). Reports symbol + stale params + line.
import type { EnrichRequest, DocCommentDriftFinding } from "../types.js";
import type { AnalyzerDiagnostics, EnrichRequest, DocCommentDriftFinding } from "../types.js";
import type { AnalysisContext } from "../analysis-context.js";
import { boundedFetchText } from "../external-fetch.js";
import { githubHeaders } from "../github-headers.js";
import { reconstructOldContent } from "./reconstruct-old-content.js";

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

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

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 (#4759) — 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: "docCommentDrift",
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;
}

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

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

let content: string | null = null;
try {
const path = file.path.split("/").map(encodeURIComponent).join("/");
const resp = await fetchFn(
`https://github.com/ghapi/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
68 changes: 27 additions & 41 deletions review-enrichment/src/analyzers/exhaustiveness-drift.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
// files and other changed consumer files at headSha (injected fetch), reverse-applies the patch to recover the
// pre-PR member set, and only reports high-confidence misses (explicit enum/union cases, no default branch). Bounded
// file-fetch caps; fail-safe on missing token/headSha, bad slug, or fetch errors.
import type { EnrichRequest, ExhaustivenessFinding } from "../types.js";
import type { AnalyzerDiagnostics, EnrichRequest, ExhaustivenessFinding } from "../types.js";
import type { AnalysisContext } from "../analysis-context.js";
import { boundedFetchText } from "../external-fetch.js";
import { githubHeaders } from "../github-headers.js";
import { reconstructOldContent } from "./reconstruct-old-content.js";
import { isDiffFileHeaderLine } from "./diff-lines.js";
Expand All @@ -28,6 +30,8 @@ const DEFAULT_CASE_RE = /^\s*default\s*:/;

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

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

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 (#4759) — 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,
signal: AbortSignal | undefined,
options: ScanOptions,
): Promise<string | null> {
try {
const encoded = path.split("/").map(encodeURIComponent).join("/");
const resp = await fetchFn(
`${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: fetchFn,
diagnostics: options.diagnostics,
phase: "exhaustiveness",
subcall: "github-contents",
maxBytes: MAX_FETCH_BYTES,
maxCallsPerCategory: MAX_FETCHES,
};
const response = options.analysis
? await options.analysis.fetchText(url, fetchOptions)
: await boundedFetchText(url, fetchOptions);
return response.ok ? response.data : null;
}

/** Walk a unified diff and collect newly added enum/union members with their declaring type name and new-file line. */
Expand Down Expand Up @@ -296,7 +282,7 @@ export async function scanExhaustivenessDrift(
return null;
}
fetches += 1;
const content = await fetchFileAtHead(owner, repo, path, headSha, githubToken, fetchFn, options.signal);
const content = await fetchFileAtHead(owner, repo, path, headSha, githubToken, fetchFn, options);
contentCache.set(path, content);
return content;
};
Expand Down
42 changes: 38 additions & 4 deletions review-enrichment/test/complexity-delta.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,10 @@ test("scanComplexityDelta: stops on an already-aborted signal", async () => {

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

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

test("scanComplexityDelta: uses the analysis-context fetchText when supplied, instead of the bare fetch path", async () => {
// #4759: 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.
let analysisCalls = 0;
const analysis = {
fetchText: async (_url, _opts) => {
analysisCalls += 1;
return {
ok: true,
status: 200,
data: HEAD_CONTENT,
bytes: HEAD_CONTENT.length,
elapsedMs: 0,
endpointCategory: "github-contents",
};
},
};
const findings = await scanComplexityDelta(
baseReq([{ path: "src/calc.ts", patch: CALC_PATCH }]),
async () => {
throw new Error("bare fetch should not be used when analysis.fetchText is supplied");
},
{ analysis },
);
assert.equal(analysisCalls, 1);
assert.equal(findings.length, 1);
assert.deepEqual(findings[0], { file: "src/calc.ts", line: 1, name: "calc", before: 5, after: 2, delta: -3 });
});

test("renderBrief emits a public-safe complexity-delta block", () => {
const { promptSection } = renderBrief({
complexityDelta: [{ file: "src/calc.ts", line: 1, name: "calc", before: 5, after: 2, delta: -3 }],
Expand Down
Loading