diff --git a/services/reviewer/src/github-client.test.ts b/services/reviewer/src/github-client.test.ts index 632262bd6..4341230d6 100644 --- a/services/reviewer/src/github-client.test.ts +++ b/services/reviewer/src/github-client.test.ts @@ -18,7 +18,7 @@ import { describe, test, expect, mock, spyOn } from "bun:test"; import type { Octokit } from "@octokit/rest"; import { CHINESE_WALL_MARKER, MINSKY_REVIEWER_BOT_LOGIN } from "./prior-review-summary"; -import { fetchPriorReviews } from "./github-client"; +import { fetchPriorReviews, fetchListFiles, MAX_FILES_FETCHED } from "./github-client"; // --------------------------------------------------------------------------- // Fake Octokit builder @@ -243,3 +243,121 @@ describe("fetchPriorReviews", () => { } }); }); + +// --------------------------------------------------------------------------- +// fetchListFiles tests +// --------------------------------------------------------------------------- + +/** + * Build a minimal fake Octokit for fetchListFiles tests. + * fetchListFiles calls octokit.paginate(octokit.rest.pulls.listFiles, ...). + */ +function buildListFilesOctokit( + paginateImpl: (endpoint: unknown, options: unknown) => Promise> +): Octokit { + return { + paginate: mock(paginateImpl), + rest: { + pulls: { + listFiles: mock(async () => ({ data: [] })), + }, + }, + } as unknown as Octokit; +} + +describe("fetchListFiles", () => { + test("calls octokit.paginate (not listFiles directly) to follow Link headers", async () => { + const octokit = buildListFilesOctokit(async () => [{ filename: "src/foo.ts" }]); + + await fetchListFiles(octokit, "owner", "repo", 42); + + expect((octokit.paginate as ReturnType).mock.calls).toHaveLength(1); + // listFiles itself must NOT be called directly + expect((octokit.rest.pulls.listFiles as ReturnType).mock.calls).toHaveLength(0); + }); + + test("returns filenames from all pages on success", async () => { + const octokit = buildListFilesOctokit(async () => [ + { filename: "src/foo.ts" }, + { filename: "src/bar.ts" }, + { filename: "README.md" }, + ]); + + const result = await fetchListFiles(octokit, "owner", "repo", 1); + expect(result).toEqual(["src/foo.ts", "src/bar.ts", "README.md"]); + }); + + test("returns [] and emits pr_scope_listfiles_error structured log on paginate error", async () => { + const octokit = buildListFilesOctokit(async () => { + throw new Error("API rate limit exceeded"); + }); + + const logSpy = spyOn(console, "log").mockImplementation(() => {}); + try { + const result = await fetchListFiles(octokit, "owner", "repo", 7); + + expect(result).toEqual([]); + // Must emit a structured JSON log with event: pr_scope_listfiles_error + const logCalls = logSpy.mock.calls; + const errorLog = logCalls + .map((args) => { + try { + return JSON.parse(args[0] as string); + } catch { + return null; + } + }) + .find((obj) => obj?.event === "pr_scope_listfiles_error"); + expect(errorLog).not.toBeNull(); + expect(errorLog?.pr).toBe(7); + expect(errorLog?.error).toContain("rate limit"); + } finally { + logSpy.mockRestore(); + } + }); + + test("returns [] and emits pr_scope_files_cap_exceeded when file count exceeds MAX_FILES_FETCHED", async () => { + // Create MAX_FILES_FETCHED + 1 files to exceed the cap. + const tooManyFiles = Array.from({ length: MAX_FILES_FETCHED + 1 }, (_, i) => ({ + filename: `src/file${i}.ts`, + })); + const octokit = buildListFilesOctokit(async () => tooManyFiles); + + const logSpy = spyOn(console, "log").mockImplementation(() => {}); + try { + const result = await fetchListFiles(octokit, "owner", "repo", 99); + + expect(result).toEqual([]); + // Must emit pr_scope_files_cap_exceeded structured log + const logCalls = logSpy.mock.calls; + const capLog = logCalls + .map((args) => { + try { + return JSON.parse(args[0] as string); + } catch { + return null; + } + }) + .find((obj) => obj?.event === "pr_scope_files_cap_exceeded"); + expect(capLog).not.toBeNull(); + expect(capLog?.pr).toBe(99); + expect(capLog?.fileCount).toBe(MAX_FILES_FETCHED + 1); + expect(capLog?.cap).toBe(MAX_FILES_FETCHED); + } finally { + logSpy.mockRestore(); + } + }); + + test("returns filenames (not []) when file count is exactly at the cap boundary (not exceeded)", async () => { + // MAX_FILES_FETCHED files — exactly at the limit, not over it. + const exactlyAtCap = Array.from({ length: MAX_FILES_FETCHED }, (_, i) => ({ + filename: `src/file${i}.ts`, + })); + const octokit = buildListFilesOctokit(async () => exactlyAtCap); + + const result = await fetchListFiles(octokit, "owner", "repo", 1); + // Should return filenames, not fall back to [] + expect(result).toHaveLength(MAX_FILES_FETCHED); + expect(result[0]).toBe("src/file0.ts"); + }); +}); diff --git a/services/reviewer/src/github-client.ts b/services/reviewer/src/github-client.ts index 2dbc4ac94..73c6d7a4a 100644 --- a/services/reviewer/src/github-client.ts +++ b/services/reviewer/src/github-client.ts @@ -56,13 +56,78 @@ export interface PullRequestContext { filesChanged: string[]; } +/** + * Hard limit on the number of changed files fetched per PR to avoid runaway + * pagination on PRs that touch thousands of files (GitHub caps at 3000 files + * per PR but the classifier's heuristics work on far fewer). When the cap is + * hit we return [] so the scope classifier falls through to conservative + * `normal` scope rather than classifying on partial data. + */ +export const MAX_FILES_FETCHED = 1000; + +/** + * Fetch the list of files changed by a PR, following Link headers via + * octokit.paginate. Returns an array of filename strings. + * + * Safety cap: if more than MAX_FILES_FETCHED files are returned the cap is + * exceeded and [] is returned (scope classifier falls through to normal). + * On any error an empty array is also returned; both cases emit a structured + * JSON log so the failure is observable in the service logs. + * + * Exported for tests. + */ +export async function fetchListFiles( + octokit: Octokit, + owner: string, + repo: string, + prNumber: number +): Promise { + let allFiles: Array<{ filename: string }>; + try { + allFiles = await octokit.paginate(octokit.rest.pulls.listFiles, { + owner, + repo, + pull_number: prNumber, + per_page: 100, + }); + } catch (err: unknown) { + const message = err instanceof Error ? err.message : String(err); + console.log( + JSON.stringify({ + event: "pr_scope_listfiles_error", + owner, + repo, + pr: prNumber, + error: message, + }) + ); + return []; + } + + if (allFiles.length > MAX_FILES_FETCHED) { + console.log( + JSON.stringify({ + event: "pr_scope_files_cap_exceeded", + owner, + repo, + pr: prNumber, + fileCount: allFiles.length, + cap: MAX_FILES_FETCHED, + }) + ); + return []; + } + + return allFiles.map((f) => f.filename); +} + export async function fetchPullRequestContext( octokit: Octokit, owner: string, repo: string, prNumber: number ): Promise { - const [prResponse, diffResponse, filesResponse] = await Promise.all([ + const [prResponse, diffResponse, filesChanged] = await Promise.all([ octokit.rest.pulls.get({ owner, repo, pull_number: prNumber }), octokit.request("GET /repos/{owner}/{repo}/pulls/{pull_number}", { owner, @@ -70,25 +135,7 @@ export async function fetchPullRequestContext( pull_number: prNumber, mediaType: { format: "diff" }, }), - // Fetch changed file paths for the scope classifier (mt#1188). - // per_page=300 covers the vast majority of PRs; GitHub caps at 3000 files - // per PR, but the classifier's heuristics work well enough on the first - // 300 — the scope check is advisory, not security-critical. - // - // Fall back to an empty list on failure (rare 422 on >3000-file PRs, - // transient 5xx). The classifier then produces conservative `normal` - // scope and the review still runs — matching the "advisory, not - // security-critical" framing above. The failure is observable via the - // warning log. - octokit.rest.pulls - .listFiles({ owner, repo, pull_number: prNumber, per_page: 300 }) - .catch((err: unknown) => { - const message = err instanceof Error ? err.message : String(err); - console.warn( - `[mt#1188] pulls.listFiles failed for ${owner}/${repo}#${prNumber}; falling back to empty filesChanged (scope will default to normal): ${message}` - ); - return { data: [] as Array<{ filename: string }> }; - }), + fetchListFiles(octokit, owner, repo, prNumber), ]); const pr = prResponse.data; @@ -96,7 +143,6 @@ export async function fetchPullRequestContext( // string at runtime even though the typed response is PullRequest. String() // safely coerces the runtime value without the as-unknown double cast. const diff = String(diffResponse.data); - const filesChanged = filesResponse.data.map((f) => f.filename); // Head repository coords may differ from base coords for forked PRs. // pr.head.repo is null in rare cases (deleted fork); fall back to base. diff --git a/services/reviewer/src/pr-scope.test.ts b/services/reviewer/src/pr-scope.test.ts index 45939cf14..881264d21 100644 --- a/services/reviewer/src/pr-scope.test.ts +++ b/services/reviewer/src/pr-scope.test.ts @@ -104,6 +104,65 @@ describe("classifyPRScope — test-only", () => { }); }); +describe("classifyPRScope — test-only (expanded TEST_FILE_PATTERN, mt#1188 BLOCKING 2)", () => { + test("__tests__/ directory anywhere in path is test-only", () => { + expect( + classifyPRScope({ + diff: NORMAL_DIFF, + filesChanged: ["src/__tests__/foo.ts", "src/__tests__/bar.ts"], + }) + ).toBe("test-only"); + }); + + test("nested __tests__/ (e.g. packages/core/__tests__/util.ts) is test-only", () => { + expect( + classifyPRScope({ + diff: NORMAL_DIFF, + filesChanged: ["packages/core/__tests__/util.ts"], + }) + ).toBe("test-only"); + }); + + test("test/ at root (not nested) is test-only", () => { + expect( + classifyPRScope({ + diff: NORMAL_DIFF, + filesChanged: ["test/integration/foo.ts"], + }) + ).toBe("test-only"); + }); + + test("case-insensitive: .TEST.ts extension is test-only", () => { + // Unlikely in practice but the pattern now carries the i flag. + expect( + classifyPRScope({ + diff: NORMAL_DIFF, + filesChanged: ["src/foo.TEST.ts"], + }) + ).toBe("test-only"); + }); + + test(".test.mjs file is test-only", () => { + expect(classifyPRScope({ diff: NORMAL_DIFF, filesChanged: ["scripts/util.test.mjs"] })).toBe( + "test-only" + ); + }); + + test(".spec.cjs file is test-only", () => { + expect(classifyPRScope({ diff: NORMAL_DIFF, filesChanged: ["scripts/util.spec.cjs"] })).toBe( + "test-only" + ); + }); + + test("__tests__/ file mixed with code file is NOT test-only", () => { + const result = classifyPRScope({ + diff: NORMAL_DIFF, + filesChanged: ["src/__tests__/foo.ts", "src/foo.ts"], + }); + expect(result).not.toBe("test-only"); + }); +}); + describe("classifyPRScope — trivial", () => { test("2 changed lines, 1 file, non-docs/test → trivial", () => { expect(classifyPRScope({ diff: TRIVIAL_DIFF, filesChanged: ["src/foo.ts"] })).toBe("trivial"); diff --git a/services/reviewer/src/pr-scope.ts b/services/reviewer/src/pr-scope.ts index e16c2bc81..d53a5744a 100644 --- a/services/reviewer/src/pr-scope.ts +++ b/services/reviewer/src/pr-scope.ts @@ -33,7 +33,8 @@ export type ScopeBucket = "trivial-or-docs" | "test-only" | "normal"; const DOCS_FILE_PATTERN = /^(docs\/|.*\.md$|.*\.mdx$|README(\.[a-z]+)?$|CHANGELOG(\.[a-z]+)?$|LICENSE(\.[a-z]+)?$)/i; -const TEST_FILE_PATTERN = /^(tests\/|.*\.test\.(ts|tsx|js|jsx)$|.*\.spec\.(ts|tsx|js|jsx)$)/; +const TEST_FILE_PATTERN = + /^(tests\/|test\/|.*__tests__\/|.*\.(test|spec)\.(ts|tsx|js|jsx|mjs|cjs)$)/i; /** Opt-out marker in PR body: force the result to `trivial`. */ const TRIVIAL_MARKER = ""; diff --git a/services/reviewer/src/review-worker.ts b/services/reviewer/src/review-worker.ts index 71d143c42..89ce035a0 100644 --- a/services/reviewer/src/review-worker.ts +++ b/services/reviewer/src/review-worker.ts @@ -420,6 +420,21 @@ export async function runReview( filesChanged: pr.filesChanged, prBody: pr.body, }); + + // Emit a structured log when the minsky:trivial marker overrides the scope. + // Makes marker usage visible in metrics so we can track opt-out frequency. + if (prScope === "trivial" && pr.body.includes("")) { + console.log( + JSON.stringify({ + event: "pr_scope_marker_override", + owner, + repo, + pr: prNumber, + sha: pr.headSha, + }) + ); + } + const scopeBucket = scopeBucketFor(prScope); const routing = decideRouting(tier, config);