diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts index d1de3fb3..53e237ef 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts @@ -513,10 +513,15 @@ export class GhGitHubReader implements T.GitHubReader { "--limit", "300", "--json", - "number,headRefName,baseRefName", + "number,headRefName,baseRefName,isCrossRepository", ]); return list(value, "open PRs").map((item, index) => { const object = record(item, `open PRs[${index}]`); + if (typeof object.isCrossRepository !== "boolean") + missing( + `open PRs[${index}].isCrossRepository`, + object.isCrossRepository + ); return { number: parsePrNumber(object.number, `open PRs[${index}].number`), headRefName: string( @@ -527,6 +532,7 @@ export class GhGitHubReader implements T.GitHubReader { object.baseRefName, `open PRs[${index}].baseRefName` ), + isCrossRepository: object.isCrossRepository, }; }); } @@ -705,7 +711,11 @@ export function orderStack( open: readonly T.OpenPullRequest[] ): T.NonEmpty { const byNumber = new Map(open.map((pr) => [pr.number, pr])); - const byHead = new Map(open.map((pr) => [pr.headRefName, pr])); + const byHead = new Map( + open + .filter((pr) => pr.isCrossRepository !== true) + .map((pr) => [pr.headRefName, pr]) + ); const children = new Map(); for (const pr of open) children.set(pr.baseRefName, [...(children.get(pr.baseRefName) ?? []), pr]); @@ -717,7 +727,8 @@ export function orderStack( let current = start; while (byHead.has(current.baseRefName)) { const parent = byHead.get(current.baseRefName); - if (parent === undefined) break; + if (parent === undefined || parent === start || down.includes(parent)) + break; down.push(parent); current = parent; } @@ -727,6 +738,7 @@ export function orderStack( ]); const up: T.OpenPullRequest[] = []; const visit = (parent: T.OpenPullRequest): void => { + if (parent.isCrossRepository === true) return; for (const child of children.get(parent.headRefName) ?? []) { if (seen.has(child.number)) continue; seen.add(child.number); diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/stack.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/stack.test.ts new file mode 100644 index 00000000..3b5bc94c --- /dev/null +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/stack.test.ts @@ -0,0 +1,133 @@ +// Stack discovery must end. A PR from a fork's main into main has the head +// branch name main, which is also the base of every PR stacked on main, and +// two PRs whose branches point at each other form a cycle. +import { afterEach, describe, expect, it } from "bun:test"; +import { spawnSync } from "node:child_process"; +import { + chmodSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { orderStack } from "./github.ts"; +import { parsePrNumber } from "./types.ts"; + +const context = (number: number) => ({ + owner: "owner", + repo: "repo", + number: parsePrNumber(number), +}); +const pr = ( + number: number, + headRefName: string, + baseRefName: string, + isCrossRepository = false +) => ({ + number: parsePrNumber(number), + headRefName, + baseRefName, + isCrossRepository, +}); +const numbers = (stack: readonly { readonly number: number }[]) => + stack.map((item) => Number(item.number)); + +describe("stack discovery", () => { + const forkMain = pr(7, "main", "main", true); + + it("does not take a fork PR from main as the parent of a PR on main", () => { + expect( + numbers( + orderStack(context(42), [ + forkMain, + pr(42, "feature", "main"), + pr(43, "upstack", "feature"), + ]) + ) + ).toEqual([42, 43]); + }); + + it("does not stack the PRs on main above a fork PR from main", () => { + expect( + numbers(orderStack(context(7), [forkMain, pr(42, "feature", "main")])) + ).toEqual([7]); + }); + + it("stops at a cycle of branches", () => { + expect( + numbers(orderStack(context(42), [pr(41, "a", "b"), pr(42, "b", "a")])) + ).toEqual([41, 42]); + }); +}); + +describe("the gh reader on a repository with an open PR from a fork's main", () => { + const directories: string[] = []; + afterEach(() => { + for (const dir of directories.splice(0)) + rmSync(dir, { recursive: true, force: true }); + }); + + it("watches the stack without the fork PR", () => { + const dir = mkdtempSync(join(tmpdir(), "watch-fork-main-")); + directories.push(dir); + const bin = join(dir, "bin"); + mkdirSync(bin); + const gh = join(bin, "gh"); + writeFileSync( + gh, + `#!${process.execPath} +const args = process.argv.slice(2); +const fail = (stderr) => { console.error(stderr); process.exit(1); }; +const graphql = (name) => args.some((a) => a.includes('query ' + name)); +const commits = (commit) => ({ data: { repository: { pullRequest: { commits: { nodes: [{ commit }] } } } } }); +let value; +if (args[0] === 'pr' && args[1] === 'list') { + value = [ + { number: 7, headRefName: 'main', baseRefName: 'main', isCrossRepository: true }, + { number: 1, headRefName: 'feature', baseRefName: 'main', isCrossRepository: false }, + ]; +} else if (args[0] === 'pr' && args[1] === 'view') { + value = { mergeable: 'MERGEABLE', mergeStateStatus: 'CLEAN', reviewDecision: '', headRefOid: 'head', headRefName: 'feature', baseRefName: 'main', state: 'OPEN', mergedAt: null, isDraft: false }; +} else if (args[0] === 'pr' && args[1] === 'checks') { + fail("no checks reported on the 'feature' branch"); +} else if (graphql('PrCheckRollup')) { + value = commits({ statusCheckRollup: null }); +} else if (graphql('PrCommitStatuses')) { + value = commits({ oid: 'head', statusCheckRollup: null }); +} else if (graphql('ReviewThreads')) { + value = { data: { repository: { pullRequest: { reviewThreads: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } } } } } }; +} else { + fail('unexpected fixture command: ' + JSON.stringify(args)); +} +console.log(JSON.stringify(value)); +` + ); + chmodSync(gh, 0o755); + const entry = join(dir, "entry.ts"); + writeFileSync( + entry, + `import { main } from ${JSON.stringify(join(import.meta.dir, "cli.ts"))}; process.exitCode = await main(process.argv.slice(2));\n` + ); + const result = spawnSync( + process.execPath, + [ + entry, + ...["--owner", "owner", "--repo", "repo", "--pr", "1"], + ...["--stack", "--status-only", "--settle", "0"], + ], + { + encoding: "utf8", + timeout: 10_000, + env: { PATH: `${bin}:${process.env.PATH}` }, + } + ); + expect(result.signal).toBeNull(); + expect(result.status).toBe(0); + expect(JSON.parse(result.stdout)).toMatchObject({ + kind: "STATUS", + rows: [{ kind: "open", context: context(1) }], + }); + }); +}); diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/types.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/types.ts index 1cc2a895..ff5754ab 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/types.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/types.ts @@ -55,6 +55,8 @@ export interface OpenPullRequest { readonly number: PrNumber; readonly headRefName: string; readonly baseRefName: string; + // A head branch in another repository is never the base of a PR here. + readonly isCrossRepository?: boolean; } export interface ReviewComment { readonly authorLogin: string | null; diff --git a/tools/forks.json b/tools/forks.json index 086fa801..5a8a2841 100644 --- a/tools/forks.json +++ b/tools/forks.json @@ -50,7 +50,7 @@ }, "plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts": { "kind": "policy", - "why": "Gives the gh reader the reviewActivity method that settle.ts uses, and carries two fixes from Michael Denyer's pstack-claude: a completed check that gh buckets as pending, such as STALE, reads as failed instead of pending forever (pstack-claude #97), and a head with no checks configured reads as such when gh reports none and the head has no rollup (pstack-claude #153).", + "why": "Gives the gh reader the reviewActivity method that settle.ts uses, and carries two fixes from Michael Denyer's pstack-claude: a completed check that gh buckets as pending, such as STALE, reads as failed instead of pending forever (pstack-claude #97), and a head with no checks configured reads as such when gh reports none and the head has no rollup (pstack-claude #153); its stack discovery never takes a PR whose head is in another repository as a parent and stops at a branch cycle, so an open PR from a fork's main cannot loop it forever.", "since": "2.0.0-rc.1", "upstream": "not-proposed" }, @@ -74,7 +74,7 @@ }, "plugins/pstack/skills/poteto-mode/scripts/watch-pr/types.ts": { "kind": "policy", - "why": "Adds the settle state, decision, and reader method that settle.ts defines, and the review-required and merge-blocked gates, the no-checks read and CI state, and the confirming-block wait and timeout reason that the fixes from Michael Denyer's pstack-claude add.", + "why": "Adds the settle state, decision, and reader method that settle.ts defines, and the review-required and merge-blocked gates, the no-checks read and CI state, and the confirming-block wait and timeout reason that the fixes from Michael Denyer's pstack-claude add; it also adds the isCrossRepository flag on an open PR that stack discovery reads.", "since": "2.0.0-rc.1", "upstream": "not-proposed" },