fix(#3842): recover ack paths from pull requests past the per-PR file cap (#3857)

.changeset/tidy-hawks-roar.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 3857
+---
+**A fully-spent ack fragment is no longer swept out from under an open pull request that changes more than 100 files** — `gh pr list --json files` truncates each PR file list at 100, so the fragment read as untouched and deleting it handed that PR the modify/delete conflict the staged sweep exists to prevent. (#3842)

scripts/lint-emitted-drift-ack.cjs

@@ -572,6 +572,33 @@ const MAX_OPEN_PRS = 200;
 /** Bound on the `gh pr list` call `fetchOpenPrTouchedAckPaths` makes (#3842). */
 const GH_TIMEOUT_MS = 20_000;
 
+/**
+ * Upper bound on how many files `gh pr list --json files` reports for ANY ONE
+ * pull request. gh requests a single page of GitHub's file connection, so a PR
+ * touching more than this comes back SILENTLY TRUNCATED — the response carries
+ * no "there is more" flag.
+ *
+ * Measured against PR #3848 (2026-08-25): 124 files changed, 100 returned, and
+ * ZERO of the returned paths under `tests/`, because the list stops mid
+ * `gsd-core/workflows/` which sorts before it. Every ack fragment in that PR was
+ * invisible to the filter below — so the sweep would have deleted it and handed
+ * the PR a modify/delete conflict, the exact failure #3842 exists to prevent,
+ * one level down from the MAX_OPEN_PRS guard that already states this reasoning.
+ *
+ * Reachable rather than theoretical: the launcher preamble is inlined into 113
+ * shipped files, so every preamble change is a >100-file PR, and those are among
+ * the likeliest to carry an ack fragment.
+ */
+const MAX_PR_FILES = 100;
+
+/**
+ * GitHub will not enumerate more than this many files for one pull request on
+ * ANY route, paginated included. A PR past it cannot be answered completely, so
+ * it throws rather than returning a set quietly missing paths — the same rule,
+ * and the same reason, as MAX_OPEN_PRS.
+ */
+const GITHUB_MAX_PR_FILES = 3000;
+
 /**
  * Default `execGh` for `fetchOpenPrTouchedAckPaths` — a real `gh` invocation. Kept as a
  * separate, swappable function (rather than inlined) so tests can inject a stub instead
@@ -588,6 +615,38 @@ function execGhDefault(args, { cwd = REPO_ROOT } = {}) {
   });
 }
 
+/**
+ * Every changed path for ONE pull request, via the paginated REST endpoint.
+ *
+ * `gh pr list --json files` and `gh pr view --json files` both cap at
+ * MAX_PR_FILES; only `gh api … --paginate` walks the whole set. Verified against
+ * PR #3848: 124 paths, including the two under `tests/` that the capped route
+ * dropped. `{owner}` and `{repo}` are gh's own placeholders, resolved from the
+ * repository in `cwd`, so this needs no nwo plumbing.
+ *
+ * Throws on anything it cannot answer completely — the caller turns that into
+ * the `'unknown'` sentinel that holds every fragment, which is the whole
+ * fail-closed contract of this seam.
+ */
+function fetchPrFiles(number, { cwd = REPO_ROOT, execGh = execGhDefault } = {}) {
+  const stdout = execGh(
+    ['api', `repos/{owner}/{repo}/pulls/${number}/files`, '--paginate', '--jq', '.[].filename'],
+    { cwd },
+  );
+  const files = String(stdout)
+    .split('\n')
+    .map((line) => line.trim())
+    .filter((line) => line !== '');
+  if (files.length >= GITHUB_MAX_PR_FILES) {
+    throw new Error(
+      `fetchPrFiles: pull request #${number} reported ${files.length} files, at or above `
+      + `GitHub's own per-PR ceiling of ${GITHUB_MAX_PR_FILES}. Refusing to reason about a list `
+      + 'that may still be incomplete.',
+    );
+  }
+  return files;
+}
+
 /**
  * The set of `tests/emitted-drift-acks/*.json` repo-relative paths touched by at least
  * one currently-OPEN pull request (#3842).
@@ -596,7 +655,10 @@ function execGhDefault(args, { cwd = REPO_ROOT } = {}) {
  * list in a single round trip, never one call per PR — intersected against the fragment
  * directory prefix. This is the "one `gh` API call" the issue itself proposes: cheap
  * enough to run on every push to `next` without meaningfully growing the guard job's
- * budget.
+ * budget. A PR whose file list lands AT `MAX_PR_FILES` earns exactly one additional
+ * paginated `gh api` call to re-fetch its full list (#3842) — because `gh pr list`
+ * silently truncates there, "at the cap" and "truncated" are indistinguishable from
+ * this response alone, so every such PR must be re-checked rather than trusted.
  *
  * Throws (never degrades to an empty set) when: `gh` itself fails (auth, network, rate
  * limit), the output is not parseable JSON, is not an array, or reaches the `MAX_OPEN_PRS`
@@ -637,8 +699,18 @@ function fetchOpenPrTouchedAckPaths({ cwd = REPO_ROOT, execGh = execGhDefault, l
   const touched = new Set();
   for (const pr of prs) {
     const files = Array.isArray(pr?.files) ? pr.files : [];
-    for (const file of files) {
-      const filePath = isPlainObject(file) ? file.path : file;
+    // `>=`, never `>`: a PR with exactly MAX_PR_FILES files is byte-identical,
+    // in this response, to one with four thousand truncated to MAX_PR_FILES.
+    // The only safe reading of "at the cap" is "possibly incomplete".
+    let paths;
+    if (files.length >= MAX_PR_FILES) {
+      // Second round trip, and the only one in this function. Deliberately not
+      // taken for the common case -- see this function's doc comment.
+      paths = fetchPrFiles(pr.number, { cwd, execGh });
+    } else {
+      paths = files.map((file) => (isPlainObject(file) ? file.path : file));
+    }
+    for (const filePath of paths) {
       if (typeof filePath === 'string' && filePath.startsWith(`${ACK_DIR_REPO_PATH}/`)) {
         touched.add(filePath);
       }
@@ -803,7 +875,10 @@ module.exports = {
   assertUsableBaseRef,
   GIT_TIMEOUT_MS,
   fetchOpenPrTouchedAckPaths,
+  fetchPrFiles,
   MAX_OPEN_PRS,
+  MAX_PR_FILES,
+  GITHUB_MAX_PR_FILES,
   GH_TIMEOUT_MS,
   runGuardNext,
 };

tests/emitted-attribution.test.cjs

@@ -88,6 +88,8 @@ const {
   assertUsableBaseRef,
   fetchOpenPrTouchedAckPaths,
   MAX_OPEN_PRS,
+  MAX_PR_FILES,
+  GITHUB_MAX_PR_FILES,
   runGuardNext,
 } = require('../scripts/lint-emitted-drift-ack.cjs');
 const {
@@ -4472,6 +4474,145 @@ describe('#3842: fetchOpenPrTouchedAckPaths', () => {
     assert.equal(paths.has(`${ACK_DIR_REPO_PATH}/x.json`), true);
     assert.equal(paths.size, 1);
   });
+
+  // `gh pr list --json files` truncates each PR's own file list at MAX_PR_FILES, silently
+  // (#3842, PR #3848: 124 files changed, 100 returned, none of the two ack paths among
+  // them because the list stops mid `gsd-core/workflows/`, which sorts before `tests/`).
+  // A PR AT the cap is answered with one additional paginated `gh api` call; below the
+  // cap, the single `gh pr list` call is trusted as-is.
+  const filler = (n) => Array.from(
+    { length: n },
+    (_, i) => ({ path: `gsd-core/workflows/w${String(i).padStart(4, '0')}.md` }),
+  );
+
+  test('a sub-cap PR is answered by the single list call', () => {
+    const files = filler(MAX_PR_FILES - 2).concat([{ path: `${ACK_DIR_REPO_PATH}/a.json` }]);
+    const stdout = JSON.stringify([{ number: 1, files }]);
+    let calls = 0;
+    const paths = fetchOpenPrTouchedAckPaths({ execGh: () => { calls += 1; return stdout; } });
+    assert.equal(calls, 1);
+    assert.equal(paths.has(`${ACK_DIR_REPO_PATH}/a.json`), true);
+  });
+
+  test('a PR at the file cap is re-fetched, because full and truncated look identical', () => {
+    const files = filler(MAX_PR_FILES);
+    const stdout = JSON.stringify([{ number: 77, files }]);
+    const paths = fetchOpenPrTouchedAckPaths({
+      execGh: (args) => (args[0] === 'pr' ? stdout : `${ACK_DIR_REPO_PATH}/a.json\n`),
+    });
+    assert.equal(paths.has(`${ACK_DIR_REPO_PATH}/a.json`), true);
+  });
+
+  test('a PR past the file cap has its ack path recovered by the re-fetch', () => {
+    const files = filler(MAX_PR_FILES + 1);
+    const stdout = JSON.stringify([{ number: 77, files }]);
+    const paths = fetchOpenPrTouchedAckPaths({
+      execGh: (args) => (args[0] === 'pr' ? stdout : `${ACK_DIR_REPO_PATH}/a.json\n`),
+    });
+    assert.equal(paths.has(`${ACK_DIR_REPO_PATH}/a.json`), true);
+  });
+
+  test('the re-fetch asks the paginated REST endpoint for that PR', () => {
+    const files = filler(MAX_PR_FILES);
+    const stdout = JSON.stringify([{ number: 77, files }]);
+    let capturedArgs;
+    fetchOpenPrTouchedAckPaths({
+      execGh: (args) => {
+        if (args[0] === 'pr') return stdout;
+        capturedArgs = args;
+        return '';
+      },
+    });
+    assert.ok(capturedArgs.includes('api'));
+    assert.ok(capturedArgs.includes('repos/{owner}/{repo}/pulls/77/files'));
+    assert.ok(capturedArgs.includes('--paginate'));
+  });
+
+  test('a failed re-fetch throws rather than trusting the truncated list', () => {
+    const files = filler(MAX_PR_FILES);
+    const stdout = JSON.stringify([{ number: 77, files }]);
+    assert.throws(() => fetchOpenPrTouchedAckPaths({
+      execGh: (args) => {
+        if (args[0] === 'pr') return stdout;
+        throw new Error('gh: rate limited');
+      },
+    }));
+  });
+
+  test("a PR beyond GitHub's own file ceiling throws rather than returning a partial set", () => {
+    const files = filler(MAX_PR_FILES);
+    const stdout = JSON.stringify([{ number: 77, files }]);
+    const apiOut = Array.from(
+      { length: GITHUB_MAX_PR_FILES },
+      (_, i) => `gsd-core/workflows/w${i}.md`,
+    ).join('\n');
+    assert.throws(() => fetchOpenPrTouchedAckPaths({
+      execGh: (args) => (args[0] === 'pr' ? stdout : apiOut),
+    }));
+  });
+
+  // Boundary coverage at GitHub's own file ceiling: limit-1, limit, limit+1.
+  // (limit is the test immediately above.)
+  test("boundary: a re-fetch just under GitHub's file ceiling is accepted", () => {
+    const files = filler(MAX_PR_FILES);
+    const stdout = JSON.stringify([{ number: 77, files }]);
+    const paths = Array.from(
+      { length: GITHUB_MAX_PR_FILES - 1 },
+      (_, i) => `gsd-core/workflows/w${i}.md`,
+    );
+    paths[0] = `${ACK_DIR_REPO_PATH}/a.json`;
+    const apiOut = paths.join('\n');
+    let result;
+    assert.doesNotThrow(() => {
+      result = fetchOpenPrTouchedAckPaths({
+        execGh: (args) => (args[0] === 'pr' ? stdout : apiOut),
+      });
+    });
+    assert.equal(result.has(`${ACK_DIR_REPO_PATH}/a.json`), true);
+  });
+
+  test('boundary: a re-fetch one past GitHub\'s file ceiling also throws', () => {
+    const files = filler(MAX_PR_FILES);
+    const stdout = JSON.stringify([{ number: 77, files }]);
+    const apiOut = Array.from(
+      { length: GITHUB_MAX_PR_FILES + 1 },
+      (_, i) => `gsd-core/workflows/w${i}.md`,
+    ).join('\n');
+    assert.throws(() => fetchOpenPrTouchedAckPaths({
+      execGh: (args) => (args[0] === 'pr' ? stdout : apiOut),
+    }));
+  });
+
+  test('a re-fetched PR that touches no fragment contributes nothing', () => {
+    const files = filler(MAX_PR_FILES);
+    const stdout = JSON.stringify([{ number: 77, files }]);
+    const apiOut = filler(MAX_PR_FILES + 5).map((f) => f.path).join('\n');
+    const paths = fetchOpenPrTouchedAckPaths({
+      execGh: (args) => (args[0] === 'pr' ? stdout : apiOut),
+    });
+    assert.equal(paths.size, 0);
+  });
+
+  test('each capped PR is re-fetched independently', () => {
+    const cappedFiles = filler(MAX_PR_FILES);
+    const subCapFiles = filler(MAX_PR_FILES - 1);
+    const stdout = JSON.stringify([
+      { number: 1, files: cappedFiles },
+      { number: 2, files: cappedFiles },
+      { number: 3, files: subCapFiles },
+    ]);
+    let apiCalls = 0;
+    const paths = fetchOpenPrTouchedAckPaths({
+      execGh: (args) => {
+        if (args[0] === 'pr') return stdout;
+        apiCalls += 1;
+        const prNumber = args[1].match(/pulls\/(\d+)\/files/)[1];
+        return prNumber === '2' ? `${ACK_DIR_REPO_PATH}/a.json\n` : 'gsd-core/workflows/w0000.md\n';
+      },
+    });
+    assert.equal(apiCalls, 2);
+    assert.deepEqual([...paths], [`${ACK_DIR_REPO_PATH}/a.json`]);
+  });
 });
 
 describe('#3842: runGuardNext wires --defer-to-open-prs end to end against a real repo', () => {
@@ -4538,6 +4679,27 @@ describe('#3842: runGuardNext wires --defer-to-open-prs end to end against a rea
     }
   });
 
+  test('a re-fetch failure degrades to the unknown sentinel, not to an empty set', () => {
+    const repo = makeGuardNextRepo();
+    try {
+      repo.writeFrag('a.json', { version: ACK_VERSION, paths: { 'x.md': { reason: 'why' } } });
+      const c2 = repo.commit('add fragment');
+      fs.writeFileSync(path.join(repo.dir, 'README.md'), 'unrelated\n');
+      repo.commit('unrelated change');
+
+      const result = runGuardNext({
+        argv: ['node', 'script', '--guard-next', '--base-ref', c2, '--defer-to-open-prs'],
+        cwd: repo.dir,
+        fetchOpenPrPaths: () => { throw new Error('fetchPrFiles: PR #77 re-fetch failed'); },
+      });
+      assert.ok(result.ok, 'an unverifiable open-PR set must hold rather than sweep');
+      assert.ok(result.lines.some((l) => l.includes('a.json') && l.includes('held')));
+      assert.ok(result.lines.some((l) => l.includes('open-PR check unavailable')));
+    } finally {
+      cleanup(repo.dir);
+    }
+  });
+
   test('with --defer-to-open-prs, a fragment untouched by any open PR still sweeps (the flag only narrows, never widens, the safe set)', () => {
     const repo = makeGuardNextRepo();
     try {