fix(mt#1338): salvage mt#1188 BLOCKING fixes (listFiles pagination + TEST_FILE_PATTERN coverage)

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<Array<{ filename: string }>>
+): 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<typeof mock>).mock.calls).toHaveLength(1);
+    // listFiles itself must NOT be called directly
+    expect((octokit.rest.pulls.listFiles as ReturnType<typeof mock>).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");
+  });
+});

services/reviewer/src/github-client.ts

@@ -56,47 +56,93 @@ 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<string[]> {
+  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<PullRequestContext> {
-  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,
       repo,
       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;
   // mediaType: { format: "diff" } makes Octokit return the body as a raw
   // 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.

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");

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 = "<!-- minsky:trivial -->";

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("<!-- minsky:trivial -->")) {
+    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);