diff --git a/plugins/pstack/skills/poteto-mode/playbooks/orchestrate.md b/plugins/pstack/skills/poteto-mode/playbooks/orchestrate.md index aaf02a02..eae04947 100644 --- a/plugins/pstack/skills/poteto-mode/playbooks/orchestrate.md +++ b/plugins/pstack/skills/poteto-mode/playbooks/orchestrate.md @@ -77,7 +77,7 @@ A dependency is a context relay, not just ordering. Undeclared upstream context #### Stack safety -- The frontier is a computed object, never narrative. Recompute `frontier.json` from `gt` after every merge and stack mutation because GitHub base refs drift mid-restack while gt tracking is authoritative: ordered PR list, branch names, head SHAs, a generation number, the lowest unmerged PR. Resolve it where gt knows the stack, normally the stacker's clone. A checkout whose gt metadata never saw the submits reports no PRs and the command errors rather than guessing. +- The frontier is a computed object, never narrative. Recompute `frontier.json` after every merge and stack mutation because GitHub base refs drift mid-restack. `gt` supplies the authoritative stack order and branch mapping; `gh` supplies each PR's live open, merged, or closed state because `gt info` can return cached state while its background refresh is still running. Record the ordered PR list, branch names, head SHAs, a generation number, and the lowest unmerged PR. Resolve it from a branch in the target linear stack, normally in the stacker's clone. Do not resolve from a trunk with multiple tracked child stacks because `gt log --stack` returns a branching graph and `orch` refuses to guess a chain. A checkout whose gt metadata never saw the submits reports no PRs and the command errors rather than guessing. - Exactly one stacker per stack may run `gt`, serialized within its stack. Record the holder in the standing orders. A restack at this scale is slow and blocks whoever runs it, so give it its own unit and keep the coordinator out of it. - Workers never rebase and never run `gt`. Babysitters follow `playbooks/babysit.md`, one per stack, scoped to one immutable frontier generation. They report conflicts to the stacker rather than restacking. - PR closes and retargets go through the stacker only. Closing a base PR orphans every chain above it. Merges and stack surgery are units with briefs like any other. 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 9c234de7..b167dc16 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts @@ -30,6 +30,12 @@ interface RunResult { readonly stderr: string; } +interface FakeCliPaths { + readonly gh: string; + readonly gt: string; + readonly gtOutput: string; +} + async function makeDirectory(): Promise { const directory = await mkdtemp(join(tmpdir(), "orch-test-")); directories.push(directory); @@ -82,6 +88,15 @@ async function makeGitStack(directory: string): Promise<{ git({ repo, args: ["init", "--initial-branch=main"] }); git({ repo, args: ["config", "user.name", "Orch Test"] }); git({ repo, args: ["config", "user.email", "orch@example.com"] }); + git({ + repo, + args: [ + "remote", + "add", + "origin", + "https://github.com/contributor/widgets.git", + ], + }); await writeFile(join(repo, "main.txt"), "main\n"); git({ repo, args: ["add", "."] }); git({ repo, args: ["commit", "-m", "main"] }); @@ -102,13 +117,13 @@ async function makeGitStack(directory: string): Promise<{ }; } -async function withFakeGt({ +async function withFakeGtAndGh({ directory, operation, output, }: { directory: string; - operation: (outputPath: string) => Promise; + operation: (paths: FakeCliPaths) => Promise; output: string; }): Promise { const bin = join(directory, "bin"); @@ -129,13 +144,13 @@ case "$*" in cat "${outputPath}" ;; "--no-interactive info stack/merged") - printf 'stack/merged\\nPR #10 (Merged) merged change\\n' + printf 'stack/merged\\nPR #10 (New Graphite status) merged change\\nhttps://app.graphite.com/github/pr/base-owner/widgets/10\\ncommit body with an unrelated link\\nhttps://app.graphite.com/github/pr/other-owner/other-repo/99\\n' ;; "--no-interactive info stack/closed") - printf 'stack/closed\\nPR #13 (Closed) closed change\\n' + printf 'stack/closed\\nPR #13 (Closed) closed change\\nhttps://app.graphite.com/github/pr/base-owner/widgets/13\\n' ;; "--no-interactive info stack/open") - printf 'stack/open\\nPR #11 (Needs approvals) open change\\n' + printf 'stack/open\\nPR #11 (Needs approvals) open change\\nhttps://app.graphite.com/github/pr/base-owner/widgets/11\\n' ;; *) printf 'unexpected gt arguments: %s\\n' "$*" >&2 @@ -146,10 +161,34 @@ esac ); await chmod(gt, 0o755); + const gh = join(bin, "gh"); + await writeFile( + gh, + `#!/usr/bin/env bash +set -euo pipefail +case "$*" in + "pr view 10 --repo github.com/base-owner/widgets --json state --jq .state") + printf 'MERGED\\n' + ;; + "pr view 13 --repo github.com/base-owner/widgets --json state --jq .state") + printf 'CLOSED\\n' + ;; + "pr view 11 --repo github.com/base-owner/widgets --json state --jq .state") + printf 'OPEN\\n' + ;; + *) + printf 'unexpected gh arguments: %s\\n' "$*" >&2 + exit 2 + ;; +esac +` + ); + await chmod(gh, 0o755); + const originalPath = process.env.PATH; process.env.PATH = `${bin}:${originalPath ?? ""}`; try { - return await operation(outputPath); + return await operation({ gh, gt, gtOutput: outputPath }); } finally { if (originalPath === undefined) { delete process.env.PATH; @@ -406,7 +445,7 @@ describe("Store", () => { ]); }); - it("resolves the ordered Graphite frontier and validates an optional pin", async () => { + it("uses adjacent PR metadata despite an unknown status and later PR link", async () => { const { directory, store } = await initializedStore(); const stack = await makeGitStack(directory); const output = `◯ main @@ -415,7 +454,7 @@ describe("Store", () => { ◉ stack/open (current) `; - await withFakeGt({ + await withFakeGtAndGh({ directory, output, operation: async () => { @@ -478,11 +517,88 @@ describe("Store", () => { }); }); + it("preserves the previous frontier when GitHub state lookup fails", async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + + await withFakeGtAndGh({ + directory, + output: "◯ main\n◉ stack/merged\n", + operation: async ({ gh }) => { + const before = await store.frontier.set({ repo: stack.repo }); + await writeFile(gh, "#!/usr/bin/env bash\nexit 1\n"); + + await expect( + store.frontier.set({ repo: stack.repo }) + ).rejects.toThrow("gh pr view 10 failed for branch stack/merged"); + expect(await store.frontier.show()).toEqual(before); + }, + }); + }); + + it("preserves the previous frontier when GitHub returns an invalid state", async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + + await withFakeGtAndGh({ + directory, + output: "◯ main\n◉ stack/merged\n", + operation: async ({ gh }) => { + const before = await store.frontier.set({ repo: stack.repo }); + await writeFile(gh, "#!/usr/bin/env bash\nprintf 'UNKNOWN\\n'\n"); + + await expect( + store.frontier.set({ repo: stack.repo }) + ).rejects.toThrow( + "gh pr view 10 returned an invalid state for branch stack/merged" + ); + expect(await store.frontier.show()).toEqual(before); + }, + }); + }); + + it("preserves the previous frontier when Graphite PR identity mismatches", async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + + await withFakeGtAndGh({ + directory, + output: "◯ main\n◉ stack/merged\n", + operation: async ({ gt, gtOutput }) => { + const before = await store.frontier.set({ repo: stack.repo }); + await writeFile( + gt, + `#!/usr/bin/env bash +set -euo pipefail +case "$*" in + "--no-interactive log short --stack --reverse") + cat "${gtOutput}" + ;; + "--no-interactive info stack/merged") + printf 'stack/merged\\nPR #10 (Ready to merge) change\\nhttps://app.graphite.com/github/pr/base-owner/widgets/12\\n' + ;; + *) + exit 2 + ;; +esac +` + ); + + await expect( + store.frontier.set({ repo: stack.repo }) + ).rejects.toThrow( + "gt info output PR identity mismatch for branch stack/merged: row 10, URL 12" + ); + expect(await store.frontier.show()).toEqual(before); + }, + }); + }); + it("rejects unparseable Graphite output loudly", async () => { const { directory, store } = await initializedStore(); const stack = await makeGitStack(directory); - await withFakeGt({ + await withFakeGtAndGh({ directory, output: "◯ main\nthis line is not Graphite output\n", operation: async () => { diff --git a/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts b/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts index 5e6c602f..f97ecb8a 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts @@ -967,71 +967,127 @@ function countLine(value: Counts): string { : entries.map(([name, count]) => `${name}=${count}`).join(", "); } -const OPEN_GT_PR_STATUSES = new Set([ - "Trunk branch locked", - "Changes requested", - "Waiting on PRs in this stack to merge", - "Waiting on downstack merge state", - "Draft", - "Required checks failed", - "Undergoing failure detection", - "Merge queue failed on current head commit", - "Handed off to merge queue...", - "Waiting on downstack", - "Merge conflicts", - "Needs reviewers", - "Needs approvals", - "Needs restack", - "Queued to merge...", - "Ready to merge", - "Ready to merge as stack", - "Rebasing...", - "Waiting on CI...", - "Stale, needs rebase onto trunk", - "Unresolved comments", - "Waiting on required CI", - "Waiting to merge...", -]); - interface GtPullRequest { readonly pr: number; readonly state: FrontierPrState; } +interface GtPullRequestIdentity { + readonly githubRepo: string; + readonly pr: number; +} + interface GtFrontierEntry extends GtPullRequest { readonly branches: string; } -function parseGtPullRequest({ +function parseGtPullRequestIdentity({ branch, - detail, + raw, }: { branch: string; - detail: string; -}): GtPullRequest { + raw: string; +}): GtPullRequestIdentity { + const lines = raw.replace(/\r/g, "").split("\n"); + const prRows = lines + .map((line, index) => ({ index, line })) + .filter( + ({ line }) => + line.startsWith("PR #") || line.startsWith("[origin] PR #") + ); + if (prRows.length === 0) { + throw new UserError( + `gt info output branch ${branch} has no pull request; this clone's gt metadata may predate the submit, so resolve the frontier from the stacker's clone or after gt sync` + ); + } + if (prRows.length > 1) { + throw new UserError( + `gt info output contains multiple PRs for branch ${branch}` + ); + } + const prRow = prRows[0]; + if (prRow === undefined) { + throw new UserError(`gt info output branch ${branch} has no pull request`); + } const match = - /^(?:\[origin\] )?PR #([1-9]\d*)(?: \(([^)\r\n]+)\))?(?: .+)?$/.exec( - detail + /^(?:\[origin\] )?PR #([1-9]\d*)(?: \([^)\r\n]+\))?(?: .+)?$/.exec( + prRow.line ); const pr = Number(match?.[1] ?? 0); if (match === null || !Number.isSafeInteger(pr)) { throw new UserError( - `gt info output has an invalid PR row for branch ${branch}: ${detail}` + `gt info output has an invalid PR row for branch ${branch}: ${prRow.line}` + ); + } + const identityPattern = + /^https:\/\/app\.graphite\.com\/github\/pr\/([A-Za-z0-9-]+)\/([A-Za-z0-9._-]+)\/([1-9]\d*)$/; + const identity = identityPattern.exec(lines[prRow.index + 1] ?? ""); + if (identity === null) { + throw new UserError( + `gt info output branch ${branch} has no adjacent canonical Graphite PR URL` ); } - const status = match[2]; - if (status === "Merged") { - return { pr, state: "MERGED" }; + const urlPr = Number(identity[3] ?? 0); + if (!Number.isSafeInteger(urlPr) || urlPr !== pr) { + throw new UserError( + `gt info output PR identity mismatch for branch ${branch}: row ${pr}, URL ${urlPr}` + ); } - if (status === "Closed") { - return { pr, state: "CLOSED" }; + const owner = identity[1] ?? ""; + const name = identity[2] ?? ""; + return { githubRepo: `github.com/${owner}/${name}`, pr }; +} + +function githubPullRequestState({ + branch, + githubRepo, + pr, + repo, +}: { + branch: string; + githubRepo: string; + pr: number; + repo: string; +}): FrontierPrState { + let raw: string; + try { + raw = execFileSync( + "gh", + [ + "pr", + "view", + String(pr), + "--repo", + githubRepo, + "--json", + "state", + "--jq", + ".state", + ], + { + cwd: repo, + encoding: "utf8", + env: process.env, + stdio: ["ignore", "pipe", "pipe"], + } + ); + } catch (error) { + throw new UserError( + `gh pr view ${pr} failed for branch ${branch}: ${errorMessage(error)}` + ); } - if (status === undefined || OPEN_GT_PR_STATUSES.has(status)) { - return { pr, state: "OPEN" }; + switch (raw.trim()) { + case "OPEN": + return "OPEN"; + case "MERGED": + return "MERGED"; + case "CLOSED": + return "CLOSED"; + default: + throw new UserError( + `gh pr view ${pr} returned an invalid state for branch ${branch}` + ); } - throw new UserError( - `gt info output has an unknown PR state for branch ${branch}: ${status}` - ); } function parseGtBranches(raw: string): readonly string[] { @@ -1083,24 +1139,16 @@ function graphitePullRequest({ `gt info ${branch} failed: ${errorMessage(error)}` ); } - const rows = raw - .replace(/\r/g, "") - .split("\n") - .filter( - (line) => - line.startsWith("PR #") || line.startsWith("[origin] PR #") - ); - if (rows.length === 0) { - throw new UserError( - `gt info output branch ${branch} has no pull request; this clone's gt metadata may predate the submit, so resolve the frontier from the stacker's clone or after gt sync` - ); - } - if (rows.length > 1) { - throw new UserError( - `gt info output contains multiple PRs for branch ${branch}` - ); - } - return parseGtPullRequest({ branch, detail: rows[0] ?? "" }); + const identity = parseGtPullRequestIdentity({ branch, raw }); + return { + pr: identity.pr, + state: githubPullRequestState({ + branch, + githubRepo: identity.githubRepo, + pr: identity.pr, + repo, + }), + }; } function graphiteFrontier(repo: string): readonly GtFrontierEntry[] {