diff --git a/review-enrichment/src/analyzers/complexity-delta.ts b/review-enrichment/src/analyzers/complexity-delta.ts index a8536d54ea..3431e62766 100644 --- a/review-enrichment/src/analyzers/complexity-delta.ts +++ b/review-enrichment/src/analyzers/complexity-delta.ts @@ -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"; @@ -41,36 +43,13 @@ const MAX_FETCH_BYTES = 1_000_000; interface ScanOptions { signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; } -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 (#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, @@ -78,19 +57,25 @@ async function fetchFileAtHeadSha( headSha: string, token: string, fetchFn: typeof fetch, - signal: AbortSignal | undefined, + options: ScanOptions, ): Promise { - 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 @@ -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 diff --git a/review-enrichment/src/analyzers/doc-comment-drift.ts b/review-enrichment/src/analyzers/doc-comment-drift.ts index f8225f8d53..59cc937075 100644 --- a/review-enrichment/src/analyzers/doc-comment-drift.ts +++ b/review-enrichment/src/analyzers/doc-comment-drift.ts @@ -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://api.github.com"; const MAX_FILES = 20; const MAX_FINDINGS = 50; const MAX_SIGNATURE_LINES = 40; @@ -22,34 +26,39 @@ const FUNC_DECL_RE = /^\s*(?:export\s+)?(?:default\s+)?(?:async\s+)?function\s*\ interface ScanOptions { signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; } -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 (#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 { + 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 @@ -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 = { - 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); @@ -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://api.github.com/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/exhaustiveness-drift.ts b/review-enrichment/src/analyzers/exhaustiveness-drift.ts index 2bf64252eb..26800ed311 100644 --- a/review-enrichment/src/analyzers/exhaustiveness-drift.ts +++ b/review-enrichment/src/analyzers/exhaustiveness-drift.ts @@ -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"; @@ -28,6 +30,8 @@ const DEFAULT_CASE_RE = /^\s*default\s*:/; interface ScanOptions { signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; } interface AddedMemberCandidate { @@ -46,33 +50,9 @@ function isScannablePath(path: string): boolean { return SOURCE_RE.test(path) && !SKIP_RE.test(path) && !isTestPath(path); } -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 (#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, @@ -80,19 +60,25 @@ async function fetchFileAtHead( headSha: string, token: string, fetchFn: typeof fetch, - signal: AbortSignal | undefined, + options: ScanOptions, ): Promise { - 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. */ @@ -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; }; diff --git a/review-enrichment/test/complexity-delta.test.ts b/review-enrichment/test/complexity-delta.test.ts index f195ba52b3..b80b8b011f 100644 --- a/review-enrichment/test/complexity-delta.test.ts +++ b/review-enrichment/test/complexity-delta.test.ts @@ -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 }]), @@ -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( @@ -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 }], diff --git a/review-enrichment/test/doc-comment-drift.test.ts b/review-enrichment/test/doc-comment-drift.test.ts index 05800d3c7c..384cd9fc86 100644 --- a/review-enrichment/test/doc-comment-drift.test.ts +++ b/review-enrichment/test/doc-comment-drift.test.ts @@ -341,6 +341,29 @@ test("scanDocCommentDrift: stops on an already-aborted signal", async () => { assert.deepEqual(out, []); }); +test("scanDocCommentDrift: 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: DRIFTED, bytes: DRIFTED.length, elapsedMs: 0, endpointCategory: "github-contents" }; + }, + }; + const findings = await scanDocCommentDrift( + baseReq([{ path: "src/a.ts", patch: DRIFT_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].staleParams, ["oldName"]); +}); + test("renderBrief emits a public-safe doc-comment-drift block", () => { const { promptSection } = renderBrief({ docCommentDrift: [{ file: "src/a.ts", line: 4, symbol: "doThing", staleParams: ["oldName"] }], diff --git a/review-enrichment/test/exhaustiveness-drift.test.ts b/review-enrichment/test/exhaustiveness-drift.test.ts index a3b1884ee6..afdb0a9980 100644 --- a/review-enrichment/test/exhaustiveness-drift.test.ts +++ b/review-enrichment/test/exhaustiveness-drift.test.ts @@ -152,6 +152,37 @@ test("scanExhaustivenessDrift: enforces the maxFetches cap", async () => { assert.equal(fetches, 10); }); +test("scanExhaustivenessDrift: 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_UNCOVERED, + bytes: HEAD_UNCOVERED.length, + elapsedMs: 0, + endpointCategory: "github-contents", + }; + }, + }; + const findings = await scanExhaustivenessDrift( + req([{ path: "src/status.ts", status: "modified", patch: PATCH_ADD_ARCHIVED }]), + 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/status.ts", line: 4, unionName: "Status", addedMember: "Archived" }, + ]); +}); + test("scanExhaustivenessDrift: returns no findings without a GitHub token", async () => { const findings = await scanExhaustivenessDrift( req([{ path: "src/status.ts", status: "modified", patch: PATCH_ADD_ARCHIVED }], {