diff --git a/.changeset/brave-goats-march.md b/.changeset/brave-goats-march.md new file mode 100644 index 000000000..f11fb8cdd --- /dev/null +++ b/.changeset/brave-goats-march.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3847 +--- +**Stage the emitted-drift-ack sweep around open PRs** — sweeping an all-spent fragment used to delete it unconditionally, handing any open PR that still touched the same file a modify/delete conflict it did not cause (#3330, #3774, #3648). The guard now holds a fragment back when an open PR still touches it, deferring the sweep until that PR merges or closes. (#3842) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b6d732cbf..b23f2e3f5 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -867,6 +867,14 @@ jobs: if: github.event_name == 'push' && github.ref == 'refs/heads/next' runs-on: ubuntu-latest timeout-minutes: 1 + # `contents: read` restates the top-level default because job-level `permissions` + # REPLACES it rather than adds to it. `pull-requests: read` is new (#3842): the sweep + # step now runs `gh pr list` to defer any all-spent fragment an OPEN PR still touches, + # rather than deleting it out from under that PR (#3330, #3774, #3648 all hit this the + # first time the sweep ran, each with the swept fragment as its only conflicting path). + permissions: + contents: read + pull-requests: read steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: @@ -894,11 +902,15 @@ jobs: - name: Assert no spent ack survives on next (legacy file and fragments) env: BEFORE: ${{ github.event.before }} + # `gh pr list` needs auth; the default GITHUB_TOKEN plus this job's + # `pull-requests: read` permission above is sufficient to read PR file lists in + # this same repo (#3842). + GH_TOKEN: ${{ github.token }} # The sha goes through the environment rather than `${{ }}` interpolation into the # script body, so nothing from the event can ever be parsed as shell. run: | if [ "$BEFORE" = "0000000000000000000000000000000000000000" ]; then - node scripts/lint-emitted-drift-ack.cjs --guard-next + node scripts/lint-emitted-drift-ack.cjs --guard-next --defer-to-open-prs else - node scripts/lint-emitted-drift-ack.cjs --guard-next --base-ref "$BEFORE" + node scripts/lint-emitted-drift-ack.cjs --guard-next --base-ref "$BEFORE" --defer-to-open-prs fi diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0004e2727..61144b2dc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1120,6 +1120,22 @@ too. If you ever see the legacy file present on `next`, delete it; do not try to well-formed. If the job names a spent fragment, run the `git rm` it prints — that is the whole remedy, and there is nothing to regenerate. +**The sweep is staged around open PRs, not unconditional (#3842).** A fragment being +all-spent is necessary but not sufficient to sweep it: deleting a fragment that an OPEN +pull request still modifies hands that PR a `modify/delete` conflict on its very next +merge attempt — precisely the shared-file conflict fragments were adopted to end, just +reintroduced by the sweep itself. This actually happened the first time the sweep ran: +#3330, #3774, and #3648 all conflicted simultaneously, each with the swept fragment as its +*only* conflicting path, all three outside contributors. So `--guard-next` now also takes +`--defer-to-open-prs` (passed by the `guard-no-ack-on-next` job): it runs one `gh pr list +--json number,files` call, and any all-spent fragment an open PR's file list still names +is *held* rather than swept — reported informationally in the job output, never as a +failure — until that PR merges or closes. If the open-PR lookup itself fails (auth, +network, rate limit), every otherwise-sweepable fragment is held for that run rather than +swept blind; the next push to `next` tries again. A fragment fully spent AND untouched by +any open PR sweeps exactly as before — this changes *when* a spent fragment is removed, +never what "spent" means. + `npm run regen:derived` still exists for the artifacts that ARE committed and derived — `sync-manifest-versions`, the ADR index, the capability matrix, the inventory manifest, the registry, and `tests/fixtures/install-tree/*.json` (`npm run gen:install-tree`, the diff --git a/scripts/lint-emitted-drift-ack.cjs b/scripts/lint-emitted-drift-ack.cjs index 88bc20d97..dd0aadfd7 100644 --- a/scripts/lint-emitted-drift-ack.cjs +++ b/scripts/lint-emitted-drift-ack.cjs @@ -326,11 +326,29 @@ function ackEntries(raw) { * * Pure — no fs, no git, no clock. `main()` does the reading. * + * #3842: an all-spent fragment is not automatically safe to sweep. #3078's sweep deletes + * the fragment outright, and when an OPEN PR still modifies that same file, git reports a + * `modify/delete` conflict on the very next merge attempt — exactly the shared-file + * conflict fragments were adopted (#2914) to end, reintroduced by the sweep itself. Three + * outside-contributor PRs (#3330, #3774, #3648) hit this simultaneously the first time the + * sweep ran, each with the swept fragment as its ONLY conflicting path. `openPrTouchedPaths` + * lets a caller defer sweeping any fragment an open PR still touches, without changing the + * inertness rule itself: a held fragment is still reported (informationally, never as a + * failure) so it is not silently forgotten once the touching PR merges or closes. + * * @param {Array<{name: string, currentRaw: string|null, baseRaw: string|null}>} fragments + * @param {object} [opts] + * @param {Set|'unknown'} [opts.openPrTouchedPaths] repo-relative fragment paths + * (`${ACK_DIR_REPO_PATH}/`) that at least one OPEN pull request currently modifies. + * Omit entirely to skip the open-PR distinction altogether (every all-spent fragment is + * reported as sweepable, unchanged pre-#3842 behavior — the shape every existing caller + * and test relies on). Pass the literal string `'unknown'` when the open-PR set could not + * be determined (e.g. the `gh` lookup failed): every otherwise-sweepable fragment is held + * rather than swept, since "we could not check" must never collapse to "assume it is safe". * @returns {{ ok: boolean, message: string, sweepable: string[] }} */ -function assertNoAllSpentFragments(fragments) { - const sweepable = []; +function assertNoAllSpentFragments(fragments, { openPrTouchedPaths } = {}) { + const allSpentFragments = []; for (const { name, currentRaw, baseRaw } of fragments) { const current = ackEntries(currentRaw); @@ -339,27 +357,54 @@ function assertNoAllSpentFragments(fragments) { if (base === null) continue; const allSpent = [...current].every(([rel, prose]) => base.get(rel) === prose); - if (allSpent) sweepable.push({ name, entries: current.size }); + if (allSpent) allSpentFragments.push({ name, entries: current.size }); } - if (sweepable.length === 0) { + const holdAll = openPrTouchedPaths === 'unknown'; + const touched = openPrTouchedPaths instanceof Set ? openPrTouchedPaths : new Set(); + const toSweep = []; + const held = []; + for (const frag of allSpentFragments) { + (holdAll || touched.has(`${ACK_DIR_REPO_PATH}/${frag.name}`) ? held : toSweep).push(frag); + } + + const heldLines = held.length === 0 ? [] : [ + '', + holdAll + ? 'deferred (open-PR check unavailable): whether an open PR still touches the following ' + + 'all-spent fragment(s) could not be determined this run, so none of them were swept ' + + '(#3842) — assuming "safe to sweep" on a failed check would risk the exact conflict ' + + 'this deferral exists to avoid. They will be reconsidered on a later run.' + : `deferred: ${held.length} all-spent fragment(s) are held back because an open pull ` + + 'request still touches them (#3842). Sweeping one now would hand that PR a ' + + 'modify/delete conflict it did not cause — the same failure #2914 adopted fragments ' + + 'to end. They will be swept once the touching PR merges or closes.', + ...held.map( + ({ name, entries }) => ` - ${ACK_DIR_REPO_PATH}/${name} (${entries} entr${entries === 1 ? 'y' : 'ies'}, all spent, held)`, + ), + ]; + + if (toSweep.length === 0) { return { ok: true, - message: `ok guard-no-ack-on-next: no all-spent fragment survives in ${ACK_DIR_REPO_PATH}/`, + message: [ + `ok guard-no-ack-on-next: no all-spent fragment survives in ${ACK_DIR_REPO_PATH}/`, + ...heldLines, + ].join('\n'), sweepable: [], }; } - const lines = sweepable.map( + const lines = toSweep.map( ({ name, entries }) => ` - ${ACK_DIR_REPO_PATH}/${name} (${entries} entr${entries === 1 ? 'y' : 'ies'}, all spent)` + `\n remedy: git rm ${ACK_DIR_REPO_PATH}/${name}`, ); return { ok: false, - sweepable: sweepable.map(({ name }) => name), + sweepable: toSweep.map(({ name }) => name), message: [ - `guard-no-ack-on-next: ${sweepable.length} fully-spent ack fragment(s) survive on next.`, + `guard-no-ack-on-next: ${toSweep.length} fully-spent ack fragment(s) survive on next.`, '', ...lines, '', @@ -371,6 +416,7 @@ function assertNoAllSpentFragments(fragments) { '', 'A partially spent fragment is deliberately NOT reported: only an entirely inert one ' + 'is swept, so appending prose to a live entry to re-arm it keeps working.', + ...heldLines, ].join('\n'), }; } @@ -513,40 +559,164 @@ function assertUsableBaseRef(ref) { return ref; } +/** + * Upper bound on how many OPEN pull requests `fetchOpenPrTouchedAckPaths` may reason + * about in one run. Mirrors the shape of `MAX_ACK_FRAGMENTS` above: exceeding it throws + * rather than silently reasoning about a truncated list — a truncated open-PR set would + * make an actually-touched fragment look untouched and sweep it anyway, which is the + * exact failure #3842 exists to prevent. 200 is comfortably above this repo's open-PR + * count at any point observed to date. + */ +const MAX_OPEN_PRS = 200; + +/** Bound on the `gh pr list` call `fetchOpenPrTouchedAckPaths` makes (#3842). */ +const GH_TIMEOUT_MS = 20_000; + +/** + * Default `execGh` for `fetchOpenPrTouchedAckPaths` — a real `gh` invocation. Kept as a + * separate, swappable function (rather than inlined) so tests can inject a stub instead + * of shelling out to a real, authenticated `gh` — which is unavailable, and would be + * flaky and network-dependent, in the test sandbox. + */ +function execGhDefault(args, { cwd = REPO_ROOT } = {}) { + return execFileSync('gh', args, { + cwd, + encoding: 'utf8', + timeout: GH_TIMEOUT_MS, + maxBuffer: 16 * 1024 * 1024, + stdio: ['ignore', 'pipe', 'pipe'], + }); +} + +/** + * The set of `tests/emitted-drift-acks/*.json` repo-relative paths touched by at least + * one currently-OPEN pull request (#3842). + * + * One `gh` call — `gh pr list --json number,files` returns every open PR's changed-file + * 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. + * + * 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` + * cap — a truncated or unreadable answer must never be silently read as "no open PR + * touches anything", which would defeat the entire deferral this function exists to + * support. The caller (`main()`) decides what "we could not check" means for the sweep; + * this function's only job is to never fabricate an empty answer. + * + * @param {object} [opts] + * @param {string} [opts.cwd] + * @param {(args: string[], opts: {cwd: string}) => string} [opts.execGh] injectable `gh` + * runner, defaulting to a real bounded `execFileSync` call. Tests inject a stub here + * rather than exec'ing a real, authenticated `gh` binary. + * @param {number} [opts.limit] + * @returns {Set} + */ +function fetchOpenPrTouchedAckPaths({ cwd = REPO_ROOT, execGh = execGhDefault, limit = MAX_OPEN_PRS } = {}) { + const stdout = execGh(['pr', 'list', '--state', 'open', '--json', 'number,files', '--limit', String(limit)], { cwd }); + + let prs; + try { + prs = JSON.parse(stdout); + } catch (err) { + throw new Error(`fetchOpenPrTouchedAckPaths: "gh pr list" did not return valid JSON: ${err.message}`); + } + if (!Array.isArray(prs)) { + throw new Error(`fetchOpenPrTouchedAckPaths: expected a JSON array from "gh pr list", got ${typeof prs}`); + } + if (prs.length >= limit) { + throw new Error( + `fetchOpenPrTouchedAckPaths: "gh pr list" returned ${prs.length} open PRs, at or above ` + + `the cap of ${limit}. Refusing to reason about a possibly-truncated list — a fragment ` + + 'touched only by a PR past the cap would look untouched and be swept anyway. Raise ' + + '`limit` or investigate the open-PR count.', + ); + } + + 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; + if (typeof filePath === 'string' && filePath.startsWith(`${ACK_DIR_REPO_PATH}/`)) { + touched.add(filePath); + } + } + } + return touched; +} + +/** + * The full `--guard-next` behavior, factored out of `main()` so it is callable directly + * from a test with injected deps — never through a real, network-dependent `gh` (or a + * real filesystem/git checkout) the way `main()` itself is only exercisable via + * subprocess. Returns data rather than performing I/O; `main()` does the printing and + * exit-code setting. + * + * @param {object} [opts] + * @param {string[]} [opts.argv] defaults to `process.argv` + * @param {string} [opts.cwd] defaults to `REPO_ROOT` + * @param {() => Set} [opts.fetchOpenPrPaths] defaults to `fetchOpenPrTouchedAckPaths`. + * Injectable so a test can supply canned open-PR data (or a throwing stub, to exercise + * the fail-closed "hold everything" path) without shelling out to a real `gh`. + * @returns {{ ok: boolean, lines: string[] }} + */ +function runGuardNext({ argv = process.argv, cwd = REPO_ROOT, fetchOpenPrPaths = fetchOpenPrTouchedAckPaths } = {}) { + const legacyFile = path.join(cwd, ...ACK_REPO_PATH.split('/')); + const legacy = assertAbsentOnNext(fs.existsSync(legacyFile)); + const lines = [legacy.message]; + + // The fragment half (#3078). CI always passes `--base-ref` (the pre-push tip of `next`, + // `github.event.before`), because the default branch allows REBASE merges and one push + // can carry N commits — `HEAD^` alone is not "the state of next before this push" in + // that case. `resolveBaseRef()`'s `HEAD^` is only the fallback for a manual run or a + // single-commit push, where the two agree. NOTE the job's checkout must fetch at least + // depth 2 for the `HEAD^` fallback to resolve at all, and must separately fetch the + // `--base-ref` commit itself, or every fragment reads as brand-new. + const baseFlag = argv.indexOf('--base-ref'); + const baseRef = baseFlag === -1 + ? resolveBaseRef({ cwd }) + : assertUsableBaseRef(argv[baseFlag + 1]); + + const dir = path.join(cwd, ...ACK_DIR_REPO_PATH.split('/')); + const fragments = listFragmentFiles(dir).map((name) => ({ + name, + currentRaw: readIfPresent(path.join(dir, name)), + baseRaw: baseRef === null ? null : readFragmentAtRef(baseRef, name, { cwd }), + })); + + // #3842: opt-in (never on by default — a caller that omits this flag gets the exact + // pre-#3842 behavior, which is what every existing test and any manual/local run relies + // on). CI passes it so the sweep never hands an open PR a modify/delete conflict it did + // not cause. A failed lookup holds EVERYTHING back rather than sweeping blind (see + // fetchOpenPrTouchedAckPaths's own doc comment). + let openPrTouchedPaths; + if (argv.includes('--defer-to-open-prs')) { + try { + openPrTouchedPaths = fetchOpenPrPaths(); + } catch (err) { + lines.push(`lint-emitted-drift-ack: open-PR check unavailable — ${err.message}`); + openPrTouchedPaths = 'unknown'; + } + } + + const sweep = assertNoAllSpentFragments(fragments, { openPrTouchedPaths }); + lines.push(sweep.message); + + return { ok: legacy.ok && sweep.ok, lines }; +} + function main() { - const legacyFile = path.join(REPO_ROOT, ...ACK_REPO_PATH.split('/')); - if (process.argv.includes('--guard-next')) { - const legacy = assertAbsentOnNext(fs.existsSync(legacyFile)); - console.log(legacy.message); - - // The fragment half (#3078). CI always passes `--base-ref` (the pre-push tip of - // `next`, `github.event.before`), because the default branch allows REBASE merges - // and one push can carry N commits — `HEAD^` alone is not "the state of next before - // this push" in that case. `resolveBaseRef()`'s `HEAD^` is only the fallback for a - // manual run or a single-commit push, where the two agree. NOTE the job's checkout - // must fetch at least depth 2 for the `HEAD^` fallback to resolve at all, and must - // separately fetch the `--base-ref` commit itself, or every fragment reads as - // brand-new. - const baseFlag = process.argv.indexOf('--base-ref'); - const baseRef = baseFlag === -1 - ? resolveBaseRef() - : assertUsableBaseRef(process.argv[baseFlag + 1]); - - const dir = path.join(REPO_ROOT, ...ACK_DIR_REPO_PATH.split('/')); - const fragments = listFragmentFiles(dir).map((name) => ({ - name, - currentRaw: readIfPresent(path.join(dir, name)), - baseRaw: baseRef === null ? null : readFragmentAtRef(baseRef, name), - })); - - const sweep = assertNoAllSpentFragments(fragments); - console.log(sweep.message); - - if (!legacy.ok || !sweep.ok) process.exitCode = 1; + const result = runGuardNext(); + for (const line of result.lines) console.log(line); + if (!result.ok) process.exitCode = 1; return; } + const legacyFile = path.join(REPO_ROOT, ...ACK_REPO_PATH.split('/')); + const fragmentsDir = path.join(REPO_ROOT, ...ACK_DIR_REPO_PATH.split('/')); const sources = [ { label: ACK_REPO_PATH, raw: readIfPresent(legacyFile) }, @@ -632,4 +802,8 @@ module.exports = { readFragmentAtRef, assertUsableBaseRef, GIT_TIMEOUT_MS, + fetchOpenPrTouchedAckPaths, + MAX_OPEN_PRS, + GH_TIMEOUT_MS, + runGuardNext, }; diff --git a/tests/emitted-attribution.test.cjs b/tests/emitted-attribution.test.cjs index 97a327ea4..e4eb51ef5 100644 --- a/tests/emitted-attribution.test.cjs +++ b/tests/emitted-attribution.test.cjs @@ -86,6 +86,9 @@ const { resolveBaseRef, readFragmentAtRef, assertUsableBaseRef, + fetchOpenPrTouchedAckPaths, + MAX_OPEN_PRS, + runGuardNext, } = require('../scripts/lint-emitted-drift-ack.cjs'); const { ACK_VERSION, @@ -4299,3 +4302,259 @@ describe('#3271: emitted-runtime-bounds', () => { assert.match(thrown.message, /bounds: worktree 60000ms, build:lib 180000ms, generator 360000ms/); }); }); + +// ─── E. #3842: stage the sweep — hold a fragment an open PR still touches ─── +// +// The sweep landed by #3078 deletes every all-spent fragment unconditionally. When an +// OPEN pull request still modifies that same file, deleting it hands that PR a +// `modify/delete` conflict on its very next merge attempt — the exact shared-file +// conflict fragments were adopted (#2914) to end, reintroduced by the sweep itself. +// #3330, #3774, and #3648 all conflicted the first time the sweep ran, each with the +// swept fragment as its ONLY conflicting path. `assertNoAllSpentFragments` now takes an +// optional `openPrTouchedPaths` to defer sweeping those; `fetchOpenPrTouchedAckPaths` +// computes that set with one `gh pr list` call; `runGuardNext` wires the two together +// behind an opt-in `--defer-to-open-prs` flag so every pre-#3842 caller (including every +// test above, and section C's real-git-repo tests) is completely unaffected. + +describe('#3842: assertNoAllSpentFragments defers to open PRs', () => { + test('omitting openPrTouchedPaths entirely is byte-identical to pre-#3842 behavior', () => { + const spent = doc({ a: { reason: 'a' } }); + const withoutOpt = assertNoAllSpentFragments([frag('a.json', spent, spent)]); + const withEmptyOpt = assertNoAllSpentFragments([frag('a.json', spent, spent)], {}); + assert.deepEqual(withoutOpt, withEmptyOpt); + assert.ok(!withoutOpt.ok); + assert.deepEqual(withoutOpt.sweepable, ['a.json']); + }); + + test('an all-spent fragment named by an open PR is held, not reported as sweepable', () => { + const spent = doc({ a: { reason: 'a' } }); + const r = assertNoAllSpentFragments( + [frag('a.json', spent, spent)], + { openPrTouchedPaths: new Set([`${ACK_DIR_REPO_PATH}/a.json`]) }, + ); + assert.ok(r.ok, 'nothing left safe to sweep, so the guard must pass'); + assert.deepEqual(r.sweepable, []); + assert.match(r.message, /deferred:.*held back because an open pull request/s); + assert.match(r.message, new RegExp(`${ACK_DIR_REPO_PATH}/a\\.json.*held`)); + assert.doesNotMatch(r.message, /git rm/, 'a held fragment must never carry a git rm remedy'); + }); + + test('a mix of held and safe-to-sweep fragments fails only on the safe one', () => { + const spentHeld = doc({ a: { reason: 'a' } }); + const spentSafe = doc({ b: { reason: 'b' } }); + const r = assertNoAllSpentFragments( + [frag('held.json', spentHeld, spentHeld), frag('safe.json', spentSafe, spentSafe)], + { openPrTouchedPaths: new Set([`${ACK_DIR_REPO_PATH}/held.json`]) }, + ); + assert.ok(!r.ok, 'one fragment is still safe to sweep, so the guard must still fail'); + assert.deepEqual(r.sweepable, ['safe.json']); + assert.match(r.message, /safe\.json/); + assert.match(r.message, /git rm[^\n]*safe\.json/); + assert.match(r.message, /held\.json.*held/s); + assert.doesNotMatch(r.message, /git rm[^\n]*held\.json/); + }); + + test('the "unknown" sentinel holds every otherwise-sweepable fragment (a failed open-PR lookup must never sweep blind)', () => { + const spentA = doc({ a: { reason: 'a' } }); + const spentB = doc({ b: { reason: 'b' } }); + const r = assertNoAllSpentFragments( + [frag('a.json', spentA, spentA), frag('b.json', spentB, spentB)], + { openPrTouchedPaths: 'unknown' }, + ); + assert.ok(r.ok); + assert.deepEqual(r.sweepable, []); + assert.match(r.message, /open-PR check unavailable/); + assert.match(r.message, /a\.json/); + assert.match(r.message, /b\.json/); + }); + + test('a PARTIALLY spent fragment is left alone regardless of open-PR touch — the open-PR set only ever narrows an already-sweepable list', () => { + const live = doc({ c: { reason: 'c-new' } }); + const liveBase = doc({ c: { reason: 'c-old' } }); + const r = assertNoAllSpentFragments( + [frag('live.json', live, liveBase)], + { openPrTouchedPaths: new Set([`${ACK_DIR_REPO_PATH}/live.json`]) }, + ); + assert.ok(r.ok); + assert.deepEqual(r.sweepable, []); + assert.doesNotMatch(r.message, /live\.json/, 'a partially-spent fragment is never mentioned by either rule'); + }); +}); + +describe('#3842: fetchOpenPrTouchedAckPaths', () => { + const prsWithFile = (relPath) => JSON.stringify([{ number: 1, files: [{ path: relPath }] }]); + + test('returns only paths under the fragment directory, ignoring unrelated changed files', () => { + const stdout = JSON.stringify([ + { number: 1, files: [{ path: `${ACK_DIR_REPO_PATH}/a.json` }, { path: 'README.md' }] }, + { number: 2, files: [{ path: 'src/foo.cts' }] }, + ]); + const paths = fetchOpenPrTouchedAckPaths({ execGh: () => stdout }); + assert.deepEqual([...paths], [`${ACK_DIR_REPO_PATH}/a.json`]); + }); + + test('accepts a bare-string file entry, not only { path }-shaped ones', () => { + const stdout = JSON.stringify([{ number: 1, files: [`${ACK_DIR_REPO_PATH}/a.json`] }]); + const paths = fetchOpenPrTouchedAckPaths({ execGh: () => stdout }); + assert.deepEqual([...paths], [`${ACK_DIR_REPO_PATH}/a.json`]); + }); + + test('a PR with no files array, or an empty one, contributes nothing and does not throw', () => { + const stdout = JSON.stringify([{ number: 1 }, { number: 2, files: [] }]); + const paths = fetchOpenPrTouchedAckPaths({ execGh: () => stdout }); + assert.deepEqual([...paths], []); + }); + + test('zero open PRs is an empty set, not an error', () => { + const paths = fetchOpenPrTouchedAckPaths({ execGh: () => '[]' }); + assert.deepEqual([...paths], []); + }); + + test('invokes gh with the expected argv: pr list, open state, number+files json, and a --limit', () => { + let capturedArgs; + fetchOpenPrTouchedAckPaths({ + execGh: (args) => { capturedArgs = args; return '[]'; }, + limit: 42, + }); + assert.deepEqual(capturedArgs, ['pr', 'list', '--state', 'open', '--json', 'number,files', '--limit', '42']); + }); + + test('malformed JSON from gh throws, rather than degrading to an empty (falsely "nothing touched") set', () => { + assert.throws( + () => fetchOpenPrTouchedAckPaths({ execGh: () => '{ not json' }), + /did not return valid JSON/, + ); + }); + + test('a non-array JSON value from gh throws', () => { + assert.throws( + () => fetchOpenPrTouchedAckPaths({ execGh: () => '{"unexpected":"shape"}' }), + /expected a JSON array/, + ); + }); + + test('a real execGh failure (gh missing, unauthenticated, rate-limited) propagates rather than being swallowed here', () => { + assert.throws( + () => fetchOpenPrTouchedAckPaths({ + execGh: () => { throw new Error('gh: command not found'); }, + }), + /command not found/, + ); + }); + + // Boundary coverage at the MAX_OPEN_PRS cap: limit-1, limit, limit+1. + test(`boundary: ${MAX_OPEN_PRS - 1} open PRs (limit-1) is accepted without truncation risk`, () => { + const n = MAX_OPEN_PRS - 1; + const stdout = JSON.stringify(Array.from({ length: n }, (_, i) => ({ number: i, files: [] }))); + assert.doesNotThrow(() => fetchOpenPrTouchedAckPaths({ execGh: () => stdout })); + }); + + test(`boundary: exactly ${MAX_OPEN_PRS} open PRs (limit) throws — a list this long might be truncated by --limit itself`, () => { + const n = MAX_OPEN_PRS; + const stdout = JSON.stringify(Array.from({ length: n }, (_, i) => ({ number: i, files: [] }))); + assert.throws( + () => fetchOpenPrTouchedAckPaths({ execGh: () => stdout }), + /at or above the cap/, + ); + }); + + test(`boundary: ${MAX_OPEN_PRS + 1} open PRs (limit+1) also throws`, () => { + const n = MAX_OPEN_PRS + 1; + const stdout = JSON.stringify(Array.from({ length: n }, (_, i) => ({ number: i, files: [] }))); + assert.throws( + () => fetchOpenPrTouchedAckPaths({ execGh: () => stdout }), + /at or above the cap/, + ); + }); + + test('sanity: a stub returning a single PR that touches one fragment is detected (guards against a vacuously-passing stub)', () => { + const paths = fetchOpenPrTouchedAckPaths({ execGh: () => prsWithFile(`${ACK_DIR_REPO_PATH}/x.json`) }); + assert.equal(paths.has(`${ACK_DIR_REPO_PATH}/x.json`), true); + assert.equal(paths.size, 1); + }); +}); + +describe('#3842: runGuardNext wires --defer-to-open-prs end to end against a real repo', () => { + test('without --defer-to-open-prs, the open-PR fetcher is never called and behavior is unchanged', () => { + 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'); + + let calls = 0; + const result = runGuardNext({ + argv: ['node', 'script', '--guard-next', '--base-ref', c2], + cwd: repo.dir, + fetchOpenPrPaths: () => { calls += 1; return new Set(); }, + }); + assert.equal(calls, 0, 'the fetcher must not be invoked without the opt-in flag'); + assert.ok(!result.ok, 'the fully-spent fragment must still fail the guard'); + assert.ok(result.lines.some((l) => l.includes('a.json'))); + } finally { + cleanup(repo.dir); + } + }); + + test('with --defer-to-open-prs, a fragment an "open PR" touches is held instead of failing the guard', () => { + 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: () => new Set([`${ACK_DIR_REPO_PATH}/a.json`]), + }); + assert.ok(result.ok, 'the only sweepable fragment is held, so the guard must pass'); + assert.ok(result.lines.some((l) => l.includes('a.json') && l.includes('held'))); + } finally { + cleanup(repo.dir); + } + }); + + test('with --defer-to-open-prs, a failing fetcher holds everything rather than sweeping blind, and still reports why', () => { + 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('gh: rate limited'); }, + }); + assert.ok(result.ok, 'an unverifiable open-PR set must hold rather than sweep'); + assert.ok(result.lines.some((l) => l.includes('gh: rate limited'))); + 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 { + 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: () => new Set(['tests/emitted-drift-acks/unrelated-other.json']), + }); + assert.ok(!result.ok, 'a.json is untouched by any open PR, so it must still be reported as sweepable'); + assert.ok(result.lines.some((l) => l.includes('git rm') && l.includes('a.json'))); + } finally { + cleanup(repo.dir); + } + }); +});