From 8bfb5c47c92a16f2cdc18780817c597df22fcc9c Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 25 Aug 2026 11:36:27 -0400 Subject: [PATCH] fix(#3842): recover ack paths from pull requests past the per-PR file cap (#3857) --- .changeset/tidy-hawks-roar.md | 5 + scripts/lint-emitted-drift-ack.cjs | 81 ++++++++++++++- tests/emitted-attribution.test.cjs | 162 +++++++++++++++++++++++++++++ 3 files changed, 245 insertions(+), 3 deletions(-) create mode 100644 .changeset/tidy-hawks-roar.md diff --git a/.changeset/tidy-hawks-roar.md b/.changeset/tidy-hawks-roar.md new file mode 100644 index 000000000..a2f5c3b86 --- /dev/null +++ b/.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) diff --git a/scripts/lint-emitted-drift-ack.cjs b/scripts/lint-emitted-drift-ack.cjs index dd0aadfd7..91d44ba51 100644 --- a/scripts/lint-emitted-drift-ack.cjs +++ b/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, }; diff --git a/tests/emitted-attribution.test.cjs b/tests/emitted-attribution.test.cjs index e4eb51ef5..26a58c8e8 100644 --- a/tests/emitted-attribution.test.cjs +++ b/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 {