diff --git a/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts b/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts index 6bf25017..708dc07b 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts @@ -10,7 +10,7 @@ import { } from "node:fs/promises"; import { realpathSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { delimiter, join } from "node:path"; import { NotFoundError, UserError, @@ -164,65 +164,87 @@ async function withFakeForge({ forge, operation, currentRows, + responses, }: { directory: string; forge: Forge; operation: () => Promise; currentRows?: string; + responses?: Readonly>; }): Promise { const bin = join(directory, "bin"); const outputPath = join(directory, "forge-output.txt"); + const callsPath = join(directory, "forge-calls"); await mkdir(bin, { recursive: true }); await writeFile( outputPath, `${ currentRows ?? forgeRows(forge, [ + { + number: 10, + state: "merged", + head: "stack/merged", + base: "main", + }, + { + number: 13, + state: "closed", + head: "stack/closed", + base: "stack/merged", + }, { number: 11, state: "open", head: "stack/open", base: "stack/closed", }, + { + number: 14, + state: "draft", + head: "stack/draft", + base: "stack/open", + }, + { + number: 15, + state: "closed", + head: "stack/abandoned", + base: "stack/open", + }, ]) }\n` ); const { fields } = FORGE_FIXTURES[forge]; - const list = (filter: string, branch: string): string => - `pr list ${filter} ${branch} --state all --json ${fields}`; + const expectedArgv = + forge === "gh" + ? `pr list --state all --limit 300 --json ${fields}` + : `pr list --state all --json ${fields}`; + const customCases = Object.entries(responses ?? {}) + .map( + ([pattern, body]) => ` "${pattern}") + printf 'x' >> "${callsPath.replace(/\\/g, "/")}" + printf '%s\\n' '${body.replace(/'/g, "'\\''")}' + ;;` + ) + .join("\n"); const fake = join(bin, forge); + const repoDir = realpathSync(join(directory, "repo")); await writeFile( fake, `#!/usr/bin/env bash set -euo pipefail -if [ "$(pwd -P)" != "${realpathSync(join(directory, "repo"))}" ]; then - printf '${forge} ran outside the fixture repo: %s\\n' "$(pwd -P)" >&2 +current_dir="$(pwd -P)" +expected_dir="${repoDir.replace(/\\/g, "/")}" +if [ "$current_dir" != "$expected_dir" ] && [ "$(pwd -W 2>/dev/null || true)" != "$expected_dir" ]; then + printf '${forge} ran outside the fixture repo: %s\\n' "$current_dir" >&2 exit 2 fi case "$*" in - "${list("--head", "stack/open")}") - cat "${outputPath}" - ;; - "${list("--head", "stack/closed")}") - printf '%s\\n' '${forgeRows(forge, [{ number: 13, state: "closed", head: "stack/closed", base: "stack/merged" }])}' - ;; - "${list("--head", "stack/merged")}") - printf '%s\\n' '${forgeRows(forge, [{ number: 10, state: "merged", head: "stack/merged", base: "main" }])}' - ;; - "${list("--base", "stack/open")}") - printf '%s\\n' '${forgeRows(forge, [ - { number: 14, state: "draft", head: "stack/draft", base: "stack/open" }, - { - number: 15, - state: "closed", - head: "stack/abandoned", - base: "stack/open", - }, - ])}' - ;; - "${list("--base", "stack/draft")}") - printf '[]\\n' + "${expectedArgv}") + printf 'x' >> "${callsPath.replace(/\\/g, "/")}" + cat "${outputPath.replace(/\\/g, "/")}" ;; +${customCases} *) printf 'unexpected ${forge} arguments: %s\\n' "$*" >&2 exit 2 @@ -232,8 +254,16 @@ esac ); await chmod(fake, 0o755); + if (process.platform === "win32") { + const gitBash = "C:\\Program Files\\Git\\bin\\bash.exe"; + await writeFile( + join(bin, `${forge}.cmd`), + `@"${gitBash}" "${fake.replace(/\\/g, "/")}" %*\r\n` + ); + } + const originalPath = process.env.PATH; - process.env.PATH = `${bin}:${originalPath ?? ""}`; + process.env.PATH = `${bin}${delimiter}${originalPath ?? ""}`; try { return await operation(); } finally { @@ -420,7 +450,9 @@ describe("Store", () => { it("replaces a stale lock whose holder pid is dead", async () => { const { directory } = await initializedStore(); - const exited = Bun.spawn(["true"]); + const exited = Bun.spawn( + process.platform === "win32" ? [process.execPath, "-e", ""] : ["true"] + ); await exited.exited; await writeFile(join(directory, ".orch.lock"), `${exited.pid}\n`); @@ -534,6 +566,9 @@ describe("Store", () => { ], lowestUnmerged: 11, }); + expect( + (await readFile(join(directory, "forge-calls"), "utf8")).length + ).toBe(1); expect( ( await store.frontier.set({ @@ -543,6 +578,9 @@ describe("Store", () => { }) ).generation ).toBe(2); + expect( + (await readFile(join(directory, "forge-calls"), "utf8")).length + ).toBe(2); expect((await store.frontier.show()).generation).toBe(2); await expect( store.frontier.set({ @@ -571,7 +609,7 @@ describe("Store", () => { ).rejects.toThrow("--prs must not contain duplicates"); }, }); - }); + }, { timeout: 15000 }); it(`rejects unparseable ${forge} output loudly`, async () => { const { directory, store } = await initializedStore(); @@ -585,7 +623,7 @@ describe("Store", () => { await expect( store.frontier.set({ repo: stack.repo, forge }) ).rejects.toThrow( - `${forge} pr list --head stack/open output is not valid JSON` + `${forge} pr list output is not valid JSON` ); }, }); @@ -608,6 +646,359 @@ describe("Store", () => { }); }); + it("rejects multiple pull requests for a single head branch", async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + + await withFakeForge({ + directory, + forge: "gh", + currentRows: JSON.stringify([ + { + number: 11, + state: "OPEN", + headRefName: "stack/open", + baseRefName: "stack/closed", + }, + { + number: 12, + state: "OPEN", + headRefName: "stack/open", + baseRefName: "stack/closed", + }, + ]), + operation: async () => { + await expect(store.frontier.set({ repo: stack.repo })).rejects.toThrow( + "branch stack/open has multiple pull requests (#11, #12); close the stale ones first" + ); + }, + }); + }); + + it("rejects non-linear stacks when multiple open PRs target the same base", async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + + await withFakeForge({ + directory, + forge: "gh", + currentRows: JSON.stringify([ + { + number: 10, + state: "MERGED", + headRefName: "stack/merged", + baseRefName: "main", + }, + { + number: 13, + state: "CLOSED", + headRefName: "stack/closed", + baseRefName: "stack/merged", + }, + { + number: 11, + state: "OPEN", + headRefName: "stack/open", + baseRefName: "stack/closed", + }, + { + number: 14, + state: "OPEN", + headRefName: "stack/draft", + baseRefName: "stack/open", + }, + { + number: 16, + state: "OPEN", + headRefName: "stack/other", + baseRefName: "stack/open", + }, + { + number: 15, + state: "CLOSED", + headRefName: "stack/abandoned", + baseRefName: "stack/open", + }, + ]), + operation: async () => { + await expect(store.frontier.set({ repo: stack.repo })).rejects.toThrow( + "stack is not linear: branches stack/draft, stack/other all target stack/open" + ); + }, + }); + }); + + it( + "detects loops in the base chain in both walk directions", + async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + + await withFakeForge({ + directory, + forge: "gh", + currentRows: JSON.stringify([ + { + number: 11, + state: "OPEN", + headRefName: "stack/open", + baseRefName: "stack/closed", + }, + { + number: 13, + state: "CLOSED", + headRefName: "stack/closed", + baseRefName: "stack/open", + }, + ]), + operation: async () => { + await expect(store.frontier.set({ repo: stack.repo })).rejects.toThrow( + "pull request base chain loops at branch stack/open" + ); + }, + }); + + await withFakeForge({ + directory, + forge: "gh", + currentRows: JSON.stringify([ + { + number: 11, + state: "OPEN", + headRefName: "stack/open", + baseRefName: "main", + }, + { + number: 14, + state: "OPEN", + headRefName: "stack/draft", + baseRefName: "stack/open", + }, + { + number: 15, + state: "OPEN", + headRefName: "stack/draft", + baseRefName: "stack/draft", + }, + ]), + operation: async () => { + await expect(store.frontier.set({ repo: stack.repo })).rejects.toThrow( + "pull request base chain loops at branch stack/draft" + ); + }, + }); + }, + { timeout: 15000 } + ); + + it("rejects invalid PR rows in forge output", async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + + await withFakeForge({ + directory, + forge: "gh", + currentRows: JSON.stringify([ + { + number: 11, + state: "OPEN", + headRefName: "stack/open", + }, + ]), + operation: async () => { + await expect(store.frontier.set({ repo: stack.repo })).rejects.toThrow( + "gh pr list output has an invalid PR row" + ); + }, + }); + }); + + it("fails closed when a resolved chain branch ref is missing locally", async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + + git({ repo: stack.repo, args: ["branch", "-D", "stack/merged"] }); + + await withFakeForge({ + directory, + forge: "gh", + operation: async () => { + await expect(store.frontier.set({ repo: stack.repo })).rejects.toThrow( + /git rev-parse.*failed|git rev-parse stack\/merged returned an invalid SHA/ + ); + }, + }); + }); + + it("short-circuits when current branch is trunk without calling the forge", async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + + git({ repo: stack.repo, args: ["checkout", "main"] }); + + await withFakeForge({ + directory, + forge: "gh", + operation: async () => { + const result = await store.frontier.set({ repo: stack.repo }); + expect(result.prs).toEqual([]); + await expect( + readFile(join(directory, "forge-calls"), "utf8") + ).rejects.toThrow(); + }, + }); + }); + + it("enforces forge-specific argv contracts between gh and origin", async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + + await withFakeForge({ + directory, + forge: "gh", + operation: async () => { + const res = await store.frontier.set({ repo: stack.repo, forge: "gh" }); + expect(res.prs).toHaveLength(4); + }, + }); + + await withFakeForge({ + directory, + forge: "origin", + operation: async () => { + const res = await store.frontier.set({ + repo: stack.repo, + forge: "origin", + }); + expect(res.prs).toHaveLength(4); + }, + }); + }); + + it( + "discovers older base pull requests outside the repository-wide limit", + async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + const { fields } = FORGE_FIXTURES.gh; + const dummyRows = Array.from({ length: 300 }, (_, index) => ({ + number: 1000 + index, + state: "CLOSED", + headRefName: `dummy/${index}`, + baseRefName: "main", + })); + dummyRows[0] = { + number: 11, + state: "OPEN", + headRefName: "stack/open", + baseRefName: "stack/closed", + }; + dummyRows[1] = { + number: 14, + state: "OPEN", + headRefName: "stack/draft", + baseRefName: "stack/open", + }; + + await withFakeForge({ + directory, + forge: "gh", + currentRows: JSON.stringify(dummyRows), + responses: { + [`pr list --head stack/closed --state all --json ${fields}`]: + forgeRows("gh", [ + { + number: 13, + state: "closed", + head: "stack/closed", + base: "stack/merged", + }, + ]), + [`pr list --head stack/merged --state all --json ${fields}`]: + forgeRows("gh", [ + { + number: 10, + state: "merged", + head: "stack/merged", + base: "main", + }, + ]), + [`pr list --base stack/open --state all --json ${fields}`]: + forgeRows("gh", [ + { + number: 14, + state: "draft", + head: "stack/draft", + base: "stack/open", + }, + ]), + [`pr list --base stack/draft --state all --json ${fields}`]: "[]", + }, + operation: async () => { + const res = await store.frontier.set({ repo: stack.repo }); + expect(res.prs.map((p) => p.pr)).toEqual([10, 13, 11, 14]); + }, + }); + }, + { timeout: 15000 } + ); + + it( + "discovers older open child pull requests outside the repository-wide limit", + async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + const { fields } = FORGE_FIXTURES.gh; + const dummyRows = Array.from({ length: 300 }, (_, index) => ({ + number: 1000 + index, + state: "CLOSED", + headRefName: `dummy/${index}`, + baseRefName: "main", + })); + dummyRows[0] = { + number: 10, + state: "MERGED", + headRefName: "stack/merged", + baseRefName: "main", + }; + dummyRows[1] = { + number: 13, + state: "CLOSED", + headRefName: "stack/closed", + baseRefName: "stack/merged", + }; + dummyRows[2] = { + number: 11, + state: "OPEN", + headRefName: "stack/open", + baseRefName: "stack/closed", + }; + + await withFakeForge({ + directory, + forge: "gh", + currentRows: JSON.stringify(dummyRows), + responses: { + [`pr list --base stack/open --state all --json ${fields}`]: + forgeRows("gh", [ + { + number: 14, + state: "draft", + head: "stack/draft", + base: "stack/open", + }, + ]), + [`pr list --base stack/draft --state all --json ${fields}`]: "[]", + }, + operation: async () => { + const res = await store.frontier.set({ repo: stack.repo }); + expect(res.prs.map((p) => p.pr)).toEqual([10, 13, 11, 14]); + }, + }); + }, + { timeout: 15000 } + ); + it("rejects malformed TSV, verdict, frontier, and inbox data", async () => { const { directory, store } = await initializedStore(); diff --git a/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts b/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts index 41736891..25c49baa 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts @@ -975,6 +975,7 @@ interface ForgePullRequestQuery { readonly headField: string; readonly baseField: string; readonly states: ReadonlyMap; + readonly listLimit?: number; } const FORGE_PULL_REQUEST_QUERIES: Readonly< @@ -989,6 +990,7 @@ const FORGE_PULL_REQUEST_QUERIES: Readonly< ["MERGED", "MERGED"], ["CLOSED", "CLOSED"], ]), + listLimit: 300, }, origin: { stateField: "status", @@ -1040,37 +1042,11 @@ function runTool({ } } -function listPullRequests({ - filter, - branch, - forge, - repo, -}: { - filter: "--head" | "--base"; - branch: string; - forge: Forge; - repo: string; -}): readonly ForgePullRequest[] { - const query = FORGE_PULL_REQUEST_QUERIES[forge]; - const command = `${forge} pr list ${filter} ${branch}`; - // Every state is fetched because a merged or closed PR still anchors the - // chain, and a draft counts as OPEN. - const raw = runTool({ - command: forge, - args: [ - "pr", - "list", - filter, - branch, - "--state", - "all", - "--json", - ["number", query.stateField, query.headField, query.baseField].join( - "," - ), - ], - repo, - }); +function parsePullRequestRows( + raw: string, + command: string, + query: ForgePullRequestQuery +): readonly ForgePullRequest[] { let value: unknown; try { value = JSON.parse(raw); @@ -1103,23 +1079,124 @@ function listPullRequests({ }); } -function branchPullRequest({ +function listAllPullRequests({ + forge, + repo, +}: { + forge: Forge; + repo: string; +}): { + readonly rows: readonly ForgePullRequest[]; + readonly truncated: boolean; +} { + const query = FORGE_PULL_REQUEST_QUERIES[forge]; + const command = `${forge} pr list`; + const argv: string[] = [ + "pr", + "list", + "--state", + "all", + ...(query.listLimit === undefined + ? [] + : ["--limit", String(query.listLimit)]), + "--json", + ["number", query.stateField, query.headField, query.baseField].join(","), + ]; + const raw = runTool({ command: forge, args: argv, repo }); + const rows = parsePullRequestRows(raw, command, query); + const truncated = + query.listLimit !== undefined && rows.length >= query.listLimit; + return { rows, truncated }; +} + +function listBranchPullRequests({ + filter, + branch, + forge, + repo, +}: { + filter: "--head" | "--base"; + branch: string; + forge: Forge; + repo: string; +}): readonly ForgePullRequest[] { + const query = FORGE_PULL_REQUEST_QUERIES[forge]; + const command = `${forge} pr list ${filter} ${branch}`; + const argv: string[] = [ + "pr", + "list", + filter, + branch, + "--state", + "all", + "--json", + ["number", query.stateField, query.headField, query.baseField].join(","), + ]; + const raw = runTool({ command: forge, args: argv, repo }); + return parsePullRequestRows(raw, command, query); +} + +interface PrGraph { + readonly byHead: Map; + readonly openByBase: Map; + readonly checkedBases: Set; + readonly truncated: boolean; +} + +function buildPrGraph( + rows: readonly ForgePullRequest[], + truncated: boolean +): PrGraph { + const byHead = new Map(); + const openByBase = new Map(); + for (const row of rows) { + (byHead.get(row.branch) ?? byHead.set(row.branch, []).get(row.branch)!).push(row); + if (row.state === "OPEN") { + (openByBase.get(row.base) ?? openByBase.set(row.base, []).get(row.base)!).push(row); + } + } + return { byHead, openByBase, checkedBases: new Set(), truncated }; +} + +function headPrFor({ + graph, branch, forge, repo, }: { + graph: PrGraph; branch: string; forge: Forge; repo: string; }): ForgePullRequest { - const rows = listPullRequests({ filter: "--head", branch, forge, repo }); - const row = rows[0]; + let rows = graph.byHead.get(branch); + if (rows === undefined && graph.truncated) { + const fetched = listBranchPullRequests({ + filter: "--head", + branch, + forge, + repo, + }); + rows = [...fetched]; + graph.byHead.set(branch, rows); + for (const row of fetched) { + if (row.state === "OPEN") { + const baseChildren = graph.openByBase.get(row.base); + if (baseChildren === undefined) { + graph.openByBase.set(row.base, [row]); + } else if (!baseChildren.some((item) => item.pr === row.pr)) { + baseChildren.push(row); + } + } + } + } + const row = (rows ?? [])[0]; if (row === undefined) { throw new UserError( `branch ${branch} has no pull request; open one with ${forge} pr create, or resolve the frontier from a branch whose chain is fully submitted` ); } - if (rows.length > 1) { + if (rows !== undefined && rows.length > 1) { throw new UserError( `branch ${branch} has multiple pull requests (${rows.map((item) => `#${item.pr}`).join(", ")}); close the stale ones first` ); @@ -1127,18 +1204,37 @@ function branchPullRequest({ return row; } -function openChildren({ +function openChildrenOf({ + graph, branch, forge, repo, }: { + graph: PrGraph; branch: string; forge: Forge; repo: string; }): readonly ForgePullRequest[] { - return listPullRequests({ filter: "--base", branch, forge, repo }).filter( - (row) => row.state === "OPEN" - ); + if (graph.truncated && !graph.checkedBases.has(branch)) { + graph.checkedBases.add(branch); + const fetched = listBranchPullRequests({ + filter: "--base", + branch, + forge, + repo, + }).filter((row) => row.state === "OPEN"); + graph.openByBase.set(branch, [...fetched]); + for (const row of fetched) { + const headRows = graph.byHead.get(row.branch); + if (headRows === undefined) { + graph.byHead.set(row.branch, [row]); + } else if (!headRows.some((item) => item.pr === row.pr)) { + headRows.push(row); + } + } + return fetched; + } + return graph.openByBase.get(branch) ?? []; } function runGit({ @@ -1151,18 +1247,32 @@ function runGit({ return runTool({ command: "git", args, repo }).trim(); } -function branchSha({ - branch, - repo, -}: { - branch: string; - repo: string; -}): string { - const sha = runGit({ args: ["rev-parse", branch], repo }); - if (!/^[0-9a-f]{40,64}$/i.test(sha)) { - throw new UserError(`git rev-parse ${branch} returned an invalid SHA`); +function parseRevParseLines( + branches: readonly string[], + raw: string +): string[] { + const lines = raw.trim().split(/\r?\n/); + if (lines.length !== branches.length) { + throw new UserError( + `git rev-parse ${branches.join(" ")} returned ${lines.length} SHAs for ${branches.length} branches` + ); } - return sha; + return lines.map((sha, index) => { + if (!/^[0-9a-f]{40,64}$/i.test(sha)) { + throw new UserError( + `git rev-parse ${branches[index]} returned an invalid SHA` + ); + } + return sha; + }); +} + +function branchShas(args: { + readonly branches: readonly string[]; + readonly repo: string; +}): string[] { + const raw = runGit({ args: ["rev-parse", ...args.branches], repo: args.repo }); + return parseRevParseLines(args.branches, raw); } function currentBranch(repo: string): string { @@ -1187,8 +1297,10 @@ function resolveFrontier(repo: string, forge: Forge): readonly FrontierPr[] { if (current === trunk) { return []; } + const { rows, truncated } = listAllPullRequests({ forge, repo }); + const graph = buildPrGraph(rows, truncated); const seen = new Set([current]); - let bottom = branchPullRequest({ branch: current, forge, repo }); + let bottom = headPrFor({ graph, branch: current, forge, repo }); const chain = [bottom]; while (bottom.base !== trunk) { if (seen.has(bottom.base)) { @@ -1197,12 +1309,12 @@ function resolveFrontier(repo: string, forge: Forge): readonly FrontierPr[] { ); } seen.add(bottom.base); - bottom = branchPullRequest({ branch: bottom.base, forge, repo }); + bottom = headPrFor({ graph, branch: bottom.base, forge, repo }); chain.unshift(bottom); } let tip = current; for (;;) { - const children = openChildren({ branch: tip, forge, repo }); + const children = openChildrenOf({ graph, branch: tip, forge, repo }); const child = children[0]; if (child === undefined) { break; @@ -1226,10 +1338,11 @@ function resolveFrontier(repo: string, forge: Forge): readonly FrontierPr[] { "pull request base chain contains duplicate pull requests" ); } - return chain.map((row) => ({ + const shas = branchShas({ branches: chain.map((row) => row.branch), repo }); + return chain.map((row, index) => ({ pr: row.pr, branches: row.branch, - sha: branchSha({ branch: row.branch, repo }), + sha: shas[index]!, state: row.state, })); }