stack-walk: stop child-PR file pagination at maxFilesPerPr and a hard page cap

llp/0010-stack-aware-requalification.explainer.md

@@ -25,7 +25,7 @@ The IO and the logic are split on purpose. `walkUpstack` is a pure, exported fun
 
 - **Same-repo, root included** — a child counts only when `head.repo.full_name` equals `owner/repo` [observed] ([github-pr.ts](../src/sources/github-pr.ts) `fetchOpenChildren` sets `sameRepo`). The REST pulls-list endpoint has no `isCrossRepository` field, so this comparison is the fork gate; a fork PR is dropped. The ROOT PR gets the same gate before the walk even starts: a fork PR's `headRefName` names a branch of the FORK, and using it as the pulls-list `base=` filter would match a same-named base-repo branch (a fork head called `main` would pull in every open PR targeting `main`), letting unrelated PRs into the manifest — so a cross-repo root (`isCrossRepository` from `gh pr view`) returns `null` outright [observed] ([github-pr.ts](../src/sources/github-pr.ts) `getStackContextAsync` cross-repo guard).
 - **Same-author** — when `requireSameAuthor` (default true), a child counts only when its `user.login` equals the current PR's author [observed] ([github-pr.ts](../src/sources/github-pr.ts) `walkUpstack` same-author filter; author from `fetchPrTrustAnchors`). This closes cross-author poisoning: a push-access colleague opening a child PR on the victim's branch cannot inject paths. Teams running genuine multi-author stacks set `requireSameAuthor: false` from the trusted base — an explicit, base-committed choice.
-- **Depth / width caps and a cycle guard** — `maxDepth` levels, `maxPrs` children per level, and a visited-branch `Set` so a branch cycle can't loop [observed] ([github-pr.ts](../src/sources/github-pr.ts) `walkUpstack`). Per child, files are fetched with `--paginate` and capped at `maxFilesPerPr`, setting `truncated` when the cap bites [observed] ([github-pr.ts](../src/sources/github-pr.ts) `fetchChildFiles`).
+- **Depth / width caps and a cycle guard** — `maxDepth` levels, `maxPrs` children per level, and a visited-branch `Set` so a branch cycle can't loop [observed] ([github-pr.ts](../src/sources/github-pr.ts) `walkUpstack`). Per child, file pages are fetched one by one and stop as soon as `maxFilesPerPr` is exceeded or a hard page ceiling is hit (never `--paginate` to the end of a huge child PR), setting `truncated` when either bound bites [observed] ([github-pr.ts](../src/sources/github-pr.ts) `collectChildFiles`, `MAX_CHILD_FILE_PAGES`; tests [stack-walk.test.ts](../src/__tests__/stack-walk.test.ts)).
 
 `--method GET` on the pulls list is mandatory (else `gh` POSTs), and a per-element `--jq` yields NDJSON that stays valid when `gh` concatenates pages — unlike a bare `--paginate`, which concatenates JSON arrays into invalid JSON (the same trap the reporter's manual pagination avoids, [LLP 0008](./0008-sources-and-reporters.explainer.md)) [observed] ([github-pr.ts](../src/sources/github-pr.ts) `fetchOpenChildren`).
 

src/__tests__/stack-walk.test.ts

@@ -1,6 +1,12 @@
 import { test, expect } from "bun:test";
 
-import { parseChildFileNdjson, walkUpstack } from "../sources/github-pr.js";
+import {
+  CHILD_FILES_PER_PAGE,
+  MAX_CHILD_FILE_PAGES,
+  collectChildFiles,
+  parseChildFileNdjson,
+  walkUpstack,
+} from "../sources/github-pr.js";
 import type { StackChildFiles, StackChildPr } from "../sources/github-pr.js";
 import type { StackWalkOptions } from "../sources/source.js";
 
@@ -180,3 +186,65 @@ test("parseChildFileNdjson: blank lines and missing filenames are skipped", () =
   const stdout = `\n${JSON.stringify({ filename: "a.ts" })}\n\n${JSON.stringify({})}\n`;
   expect(parseChildFileNdjson(stdout)).toEqual(["a.ts"]);
 });
+
+/** One NDJSON page of `count` filenames, offset so names are unique across pages. */
+function page(count: number, offset = 0): string {
+  return Array.from({ length: count }, (_, i) =>
+    JSON.stringify({ filename: `src/f${offset + i}.ts` }),
+  ).join("\n");
+}
+
+test("collectChildFiles: stops paginating once maxFiles is exceeded", async () => {
+  // 5 full pages exist, but with maxFiles=100 page 2 already proves truncation —
+  // pages 3..5 must never be fetched.
+  const calls: number[] = [];
+  const result = await collectChildFiles(async (n) => {
+    calls.push(n);
+    return page(CHILD_FILES_PER_PAGE, (n - 1) * CHILD_FILES_PER_PAGE);
+  }, 100);
+  expect(calls).toEqual([1, 2]);
+  expect(result.files.length).toBe(100);
+  expect(result.truncated).toBe(true);
+});
+
+test("collectChildFiles: reads to the end of a short list without truncation", async () => {
+  const pages: Record<number, string> = { 1: page(CHILD_FILES_PER_PAGE), 2: page(40, 100) };
+  const calls: number[] = [];
+  const result = await collectChildFiles(async (n) => {
+    calls.push(n);
+    return pages[n] ?? "";
+  }, 500);
+  expect(calls).toEqual([1, 2]);
+  expect(result.files.length).toBe(140);
+  expect(result.truncated).toBe(false);
+});
+
+test("collectChildFiles: never fetches past the hard page ceiling, and marks the subset", async () => {
+  // Endless full pages with a huge maxFiles: the ceiling is the only stop, and the
+  // result must still say truncated (the end of the list was never observed).
+  const calls: number[] = [];
+  const result = await collectChildFiles(async (n) => {
+    calls.push(n);
+    return page(CHILD_FILES_PER_PAGE, (n - 1) * CHILD_FILES_PER_PAGE);
+  }, 1_000_000);
+  expect(calls.length).toBe(MAX_CHILD_FILE_PAGES);
+  expect(result.truncated).toBe(true);
+});
+
+test("collectChildFiles: a dropped control-char name does not end pagination early", async () => {
+  // Page 1 has 100 raw entries but one is dropped by the parser (99 kept). The
+  // "last page?" decision must use the raw count, so page 2 is still fetched.
+  const forged = JSON.stringify({ filename: "docs/a\nforged.ts" });
+  const pages: Record<number, string> = {
+    1: [page(CHILD_FILES_PER_PAGE - 1), forged].join("\n"),
+    2: page(5, 200),
+  };
+  const calls: number[] = [];
+  const result = await collectChildFiles(async (n) => {
+    calls.push(n);
+    return pages[n] ?? "";
+  }, 500);
+  expect(calls).toEqual([1, 2]);
+  expect(result.files.length).toBe(104);
+  expect(result.truncated).toBe(false);
+});

src/sources/github-pr.ts

@@ -116,6 +116,47 @@ export function parseChildFileNdjson(stdout: string): string[] {
     .filter((name) => name.length > 0 && ![...name].some((char) => char.charCodeAt(0) < 0x20));
 }
 
+/** Page size for the child-PR files endpoint (fetchAllComments convention). */
+export const CHILD_FILES_PER_PAGE = 100;
+
+/**
+ * Safety cap on pagination, same convention as the reporter's MAX_COMMENT_PAGES:
+ * 30 pages = 3000 files bounds a pathological child PR; virtually every real
+ * child PR exits far earlier via the maxFiles early stop below.
+ */
+export const MAX_CHILD_FILE_PAGES = 30;
+
+/**
+ * Collect a child PR's file pages, factored pure over an injected per-page fetcher
+ * (same pattern as walkUpstack) so the stop conditions are testable without gh.
+ * Stops at end-of-list, one entry past `maxFiles` (enough to know the list is
+ * truncated — never paginates a huge child PR to the end just to throw the tail
+ * away), or the hard page ceiling. Exported for tests.
+ */
+export async function collectChildFiles(
+  fetchPage: (page: number) => Promise<string>,
+  maxFiles: number,
+): Promise<StackChildFiles> {
+  const files: string[] = [];
+  let sawEnd = false;
+  for (let page = 1; page <= MAX_CHILD_FILE_PAGES && !sawEnd; page++) {
+    const stdout = await fetchPage(page);
+    // "Last page?" is decided on the RAW line count (jq emits exactly one NDJSON
+    // line per array element): parseChildFileNdjson drops control-char names, so
+    // deciding on its output would mistake a page with a dropped entry for the
+    // final page and silently skip the rest of the list.
+    const rawCount = stdout.split("\n").filter((line) => line.trim()).length;
+    files.push(...parseChildFileNdjson(stdout));
+    sawEnd = rawCount < CHILD_FILES_PER_PAGE;
+    if (files.length > maxFiles) {
+      break;
+    }
+  }
+  // Hitting the page ceiling without seeing the end of the list still marks the
+  // manifest as a subset — never claim completeness that wasn't observed.
+  return { files: files.slice(0, maxFiles), truncated: files.length > maxFiles || !sawEnd };
+}
+
 /**
  * Append gh as a git credential helper for a single command. The token comes from
  * GH_TOKEN via the credential-helper protocol — never argv, never `.git/config` —
@@ -453,37 +494,42 @@ export class GitHubPRSource implements ReviewSource {
       });
   }
 
-  /** A child PR's changed paths, capped at `maxFiles` with a truncated marker. */
+  /**
+   * A child PR's changed paths, capped at `maxFiles` with a truncated marker.
+   * Pages are fetched one by one (not `--paginate`, which would walk a large
+   * child PR's whole file list before the client-side cap applies) so pagination
+   * stops as soon as the cap is exceeded, and never past MAX_CHILD_FILE_PAGES.
+   */
   private async fetchChildFiles(
     gh: string,
     repo: string,
     prNumber: number,
     maxFiles: number,
   ): Promise<StackChildFiles> {
-    const { stdout } = await run(
-      gh,
-      [
-        "api",
-        // --method GET is mandatory once a -f field is present (else gh POSTs).
-        "--method",
-        "GET",
-        `repos/${repo}/pulls/${prNumber}/files`,
-        // 100/page (fetchAllComments convention): a 3000-file child PR is ~30
-        // round-trips instead of ~100 at the REST default of 30/page.
-        "-f",
-        "per_page=100",
-        "--paginate",
-        "--jq",
-        // Objects, NOT raw strings (.[].filename): gh prints a raw string result
-        // one value per line, so a git path containing a newline would split into
-        // TWO manifest entries — one of them a forged membership the grounding
-        // check would then accept. NDJSON keeps the newline escaped.
-        ".[] | {filename}",
-      ],
-      { cwd: this.options.cwd },
-    );
-    const all = parseChildFileNdjson(stdout);
-    return { files: all.slice(0, maxFiles), truncated: all.length > maxFiles };
+    return collectChildFiles(async (page) => {
+      const { stdout } = await run(
+        gh,
+        [
+          "api",
+          // --method GET is mandatory once a -f field is present (else gh POSTs).
+          "--method",
+          "GET",
+          `repos/${repo}/pulls/${prNumber}/files`,
+          "-f",
+          `per_page=${CHILD_FILES_PER_PAGE}`,
+          "-f",
+          `page=${page}`,
+          "--jq",
+          // Objects, NOT raw strings (.[].filename): gh prints a raw string result
+          // one value per line, so a git path containing a newline would split into
+          // TWO manifest entries — one of them a forged membership the grounding
+          // check would then accept. NDJSON keeps the newline escaped.
+          ".[] | {filename}",
+        ],
+        { cwd: this.options.cwd },
+      );
+      return stdout;
+    }, maxFiles);
   }
 
   /**