From 331e42197d53b0baeb42e599a2bceaa4bb85854b Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Wed, 28 Oct 2026 15:01:40 -0300 Subject: [PATCH 1/3] feat(merge-gate): gate and merge a GitHub native stack with --stack A PR above position 1 in a native stack was refused base-not-trunk, so a stack could not go through the gate. `merge-gate --stack [--merge]` reads the stack (paginated), plans positions 1..P (merged entries below skipped, a closed one or a non-open target refused), and gates every open PR with the same per-PR decide(). decide() now takes an ExpectedBase: trunk, or the head of the open PR directly below it. A stack that targets another branch is refused by the bottom PR's trunk check. Reasons are tagged "#N: ". One merge-async call on the top PR, pinned to its gated head, lands the range, and every PR is read back (merge-unverified unless all read MERGED; merge-failed only when the job failed and none merged). Plain mode runs through the same plan path (a one-PR plan on trunk) with unchanged verdict text, plus a `stack` field (null) on every verdict and a --stack hint on base-not-trunk when the PR query reports a stack. facts, pre-merge head checks, and read-backs now run in parallel, so the fake gh claims response slots with mkdir instead of a shared counts file. Tested: bun test orch watch-pr merge-gate check-plan.test.ts (216 pass); bun run typecheck (clean); bun scripts/check-port.ts (0 findings); dry runs without --merge on nagringa-dev/monorepo #2009 --stack (gated #2005 and #2009, refused on their pending checks), #2003 --stack (merged target refused), and plain #2009 (hint printed). Reviewer notes: merge_action stays direct_merge, as in the single-PR fallback, so a merge-queue branch fails instead of enqueueing; an `enqueued` status reads back as merge-unverified. The merge method is the top PR's (policy mergeCommitBranches), applied to the whole range. --- .../sync-upstream/references/translation.md | 2 +- README.md | 2 +- skills/poteto-mode/playbooks/opening-a-pr.md | 2 +- skills/poteto-mode/playbooks/shipping.md | 2 +- .../scripts/merge-gate/cli.test.ts | 323 +++++++++++++++--- skills/poteto-mode/scripts/merge-gate/cli.ts | 89 +++-- .../scripts/merge-gate/decide.test.ts | 204 ++++++++++- .../poteto-mode/scripts/merge-gate/decide.ts | 109 +++++- .../scripts/merge-gate/fake-gh.test-helper.ts | 26 +- .../poteto-mode/scripts/merge-gate/github.ts | 161 ++++++++- 10 files changed, 792 insertions(+), 128 deletions(-) diff --git a/.claude/skills/sync-upstream/references/translation.md b/.claude/skills/sync-upstream/references/translation.md index aaa69f9..4ec7680 100644 --- a/.claude/skills/sync-upstream/references/translation.md +++ b/.claude/skills/sync-upstream/references/translation.md @@ -76,7 +76,7 @@ A sync must keep these even when upstream's line differs. - orchestrate and swarm isolation: remote by default for orchestrate workers, and a worktree for any swarm worker that checks out, builds, or runs. - `.claude-plugin/plugin.json` carries no `version`, where upstream's `.cursor-plugin/plugin.json` does. Claude Code then versions a GitHub-hosted install by commit SHA, so every merge to `main` reaches `claude plugin update`. A version string that does not change blocks updates. The `plugin-version` rule in `scripts/check-port.ts` keeps it out. `claude plugin validate` warns about the missing version. That warning is expected. - interrogate adds the blind-spot lenses (`references/blind-spots.md`, Step 3 item 5), a PR number or URL as scope, the lead checking each Act on and Consider finding against source, and the someone-else's-PR flow (`references/review-comments.md`), all folded in from Lucas's retired pr-review-swarm skill. When a playbook runs it as the pre-PR gate, it hands the verdict back instead of ending the turn. The gate's rules live in Opening a PR step 2. It runs no tool-based analyzers: a CodeScene change-set step was tried and removed after an eval showed the reviewers already covered everything it found (PR #2). -- `skills/poteto-mode/scripts/merge-gate/` is port-only. It reuses watch-pr's readiness verdict without editing watch-pr, and adds the policy: allowed repos, human-only paths needing a human approval on the head, and head pinning with `--match-head-commit`. A PR in a GitHub native stack refuses `gh pr merge`, so the gate falls back to the merge-async REST endpoint with the same head SHA. Every playbook merges only through it, and a refusal is final: human-only changes are the user's to merge. The policy lives outside git, and editing it is on poteto-mode's always-pause list. +- `skills/poteto-mode/scripts/merge-gate/` is port-only. It reuses watch-pr's readiness verdict without editing watch-pr, and adds the policy: allowed repos, human-only paths needing a human approval on the head, and head pinning with `--match-head-commit`. A PR in a GitHub native stack refuses `gh pr merge`, so the gate falls back to the merge-async REST endpoint with the same head SHA. `--stack` gates native stack positions 1 to the target as one unit: each open PR runs the same per-PR decision, with its expected base being the head of the open PR below it instead of trunk, and one merge-async call on the target, pinned to its head, lands the range atomically. Upstream has no native stacks. Every playbook merges only through it, and a refusal is final: human-only changes are the user's to merge. The policy lives outside git, and editing it is on poteto-mode's always-pause list. - `principle-never-block-on-the-human` narrows its force-push example to "force-push to a shared branch", matching poteto-mode's Autonomy. Autopilot owners force-push their own branches with a lease on every rebase. - models.md uses only `opus` and `sonnet`. `fable` is dropped for cost: every role that upstream maps to it, and every panel slot, goes to `opus` or `sonnet`. interrogate runs two reviewers (`opus`, `sonnet`), accepting that both share one family and give less independent review than a cross-family pair. - `orch frontier set` walks the PR base chain with `gh` only. Upstream's `--forge` option and its Origin branch are removed, so the command takes no forge. diff --git a/README.md b/README.md index 5040058..65b46f1 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ mkdir -p ~/.claude/pstack && [ -e ~/.claude/pstack/merge-policy.json ] || curl - Edit it for your repos. Both `defaults.humanOnly` and `repos` are required, and a repo with no entry is refused. `humanOnly` globs mark paths that need a human's approval on the exact head commit before the agent merges. The approver must be a collaborator other than the PR author, so on a solo repo those paths mean you merge them yourself. `requireHumanApprovalOnHead` asks for that approval on every PR in the repo. `mergeCommitBranches` lists branch globs that merge with a merge commit instead of a squash. -`merge-gate` also refuses unless `gh` is signed in as the PR author, the PR targets the default branch (so upper PRs in a stack wait for their turn), and the head has passing checks. A repo with no CI is refused with `checks-missing`. +`merge-gate` also refuses unless `gh` is signed in as the PR author, the PR targets the default branch (so upper PRs in a stack wait for their turn), and the head has passing checks. A repo with no CI is refused with `checks-missing`. For a GitHub native stack, `merge-gate --stack --merge` gates every open PR from the bottom up to ``, each based on the one below it, and merges them together. ## Differences from upstream diff --git a/skills/poteto-mode/playbooks/opening-a-pr.md b/skills/poteto-mode/playbooks/opening-a-pr.md index 8931a1f..749661c 100644 --- a/skills/poteto-mode/playbooks/opening-a-pr.md +++ b/skills/poteto-mode/playbooks/opening-a-pr.md @@ -33,7 +33,7 @@ After the sections, attach videos or screenshots when they prove a claim. Full S 2. Gate. Run the **interrogate** skill against trunk. Apply each Act on finding that fits the PR's intent without changing a public API or deleting data. An Act on finding you won't fix here is a blocker, unless the lead re-buckets it to Consider or Dismissed with a reason. Commit the fixes after `/simplify` over them, rerun the checks they affect, and push. When a fix changed behavior beyond the lines its finding cited, run the gate once more on the new head and apply its findings the same way. Never run a third round. 3. No blockers: `gh pr create` without `--draft`, or `gh pr ready ` for your own draft that you have not readied before. A PR that went back to draft after you readied it is a hold: report it and stop. In a stack, ready only your own PR. `gh stack submit --auto --open` is for one agent that gated every PR in the stack, followed by `gh pr edit` for each title and body. With blockers: the PR stays or opens as a draft. 4. Merge, when this PR is the whole task rather than a layer mid-stack or mid-phase, and never for a revert PR. Run the **Babysit** playbook in `drive` mode until the watcher reports `READY`, then `scripts/merge-gate/merge-gate --merge`. Never run `gh pr merge` yourself. A final refusal is never overridden that way: the user merges it. - - Final (report the reason and any listed files, and stop): `needs-human-approval`, `repo-not-allowed`, `not-author`, `changes-requested`, `base-not-trunk`, `policy-invalid`, `threads-unverified`, `reviews-unverified`, `files-incomplete`, `draft` (someone drafted it after you readied it, which means hold). A verdict with any final reason is final. + - Final (report the reason and any listed files, and stop): `needs-human-approval`, `repo-not-allowed`, `not-author`, `changes-requested`, `base-not-trunk`, `policy-invalid`, `threads-unverified`, `reviews-unverified`, `files-incomplete`, `draft` (someone drafted it after you readied it, which means hold). A verdict with any final reason is final. A `base-not-trunk` refusal on a PR in a GitHub native stack means it lands with its stack, through Shipping's `--stack`, and only on an explicit request to land. - Back to Babysit, then re-gate: `checks-failed`, `threads-unresolved`, and `not-mergeable` when the detail says `DIRTY` or `BEHIND` (rebase first). - Transient (wait for the watcher, rerun once, then report): `checks-pending`, `head-moved`, `github-error`, and any other `not-mergeable`. - `merge-unverified`: run `gh pr view --json state,mergeCommit`. If it merged, continue with the post-merge watch on that commit. If not, a stacked PR's async merge may still be queued, so poll `gh pr view` for a few minutes before you rerun with `--merge`. If it is still open after that, treat it as transient. diff --git a/skills/poteto-mode/playbooks/shipping.md b/skills/poteto-mode/playbooks/shipping.md index c7eadd2..ce972f8 100644 --- a/skills/poteto-mode/playbooks/shipping.md +++ b/skills/poteto-mode/playbooks/shipping.md @@ -7,7 +7,7 @@ This is the half after `playbooks/babysit.md`. Run it only on an explicit reques 1. **Verify each PR independently before it lands.** Every PR operation goes through `gh`. Never require Graphite (`gt`). One verifier per PR, each a background subagent with `isolation: "worktree"`, runs the change the way a user would with the matching control skill (such as the `claude-in-chrome` browser tools or the built-in `run` skill), parent against head, and posts `PASS`, `PASS+NOTES`, or `FAIL` on that PR. Only a verdict from an agent that did not write the code counts, because an author checks what it meant to build. Green CI and an approving bot review are not verdicts, because neither runs the change as a user would. 2. **Land only the contiguous verified run rooted at the bottom.** Walk up from the lowest unmerged PR and stop at the first one without `PASS` or `PASS+NOTES`. The PRs below it are the run, and the top one is the ceiling. A verified PR above an unverified one waits, because it cannot land without the code below it. Moving one out of a `gh stack` takes the interactive `gh stack modify`, so leave that to the user. Once its branch neither holds nor needs that code and it targets trunk, it can land on its own after a fresh verdict, because its old one ran with that code beneath it and trunk does not have it. 3. **Re-check that the verdicts still describe the code.** A verdict covers a patch, and a chain rebase moves every PR above it to a new SHA without failing any check. Compare `git patch-id` at the verdict SHA with the current head before trusting an older verdict. When the patches differ only in tests, docs, or lint config, a lane's result stands if what it ran builds the same at both SHAs apart from noise. Noise is a difference that also shows up between two builds of the verdict SHA, or an embedded commit SHA. Report each kind of noise with its files, so a reader can check the call. The change itself still gets fresh checks and a review, because the old verdict never saw it. A lane with no build output, such as a dev server, cannot show this, so rerun it, and re-verify anything else that drifted. -4. **Land bottom-up, one PR at a time.** Always merge the run's lowest unmerged PR. Bring its branch onto the exact trunk tip when needed, with `gh stack rebase --downstack` and `gh stack push` from the bottom branch, and re-run step 3 after the push. Merge with `scripts/merge-gate/merge-gate --merge`, which refuses a PR whose base is not trunk, because one whose base is still its parent's branch would merge into that branch. A refusal stops the landing. For `needs-human-approval`, report the PR and the human-only files the gate listed, and leave the merge to the user. Read the PR back (`gh pr view --json state,mergedAt,baseRefName`), because a merge can report success when a merge queue only queued the PR. A landed PR reads merged with trunk as its base. Never arm `--auto`, because without required checks GitHub merges it at once. After a merge lands, GitHub restacks the rest of a `gh stack` server-side. Run `gh stack sync`, confirm the next PR now targets trunk, and let step 3's patch-id rule decide whether its verdict survives. Merge it once its checks pass and its verdict survives. +4. **Land bottom-up, one PR at a time.** Always merge the run's lowest unmerged PR. Bring its branch onto the exact trunk tip when needed, with `gh stack rebase --downstack` and `gh stack push` from the bottom branch, and re-run step 3 after the push. Merge with `scripts/merge-gate/merge-gate --merge`, which refuses a PR whose base is not trunk, because one whose base is still its parent's branch would merge into that branch. In a GitHub native stack, land the whole run at once with `scripts/merge-gate/merge-gate --stack --merge` instead. It gates every open PR from position 1 to the ceiling, each against the head of the PR below it, and lands them in one atomic merge-async call. A refusal stops the landing. For `needs-human-approval`, report the PR and the human-only files the gate listed, and leave the merge to the user. Read the PR back (`gh pr view --json state,mergedAt,baseRefName`), because a merge can report success when a merge queue only queued the PR. A landed PR reads merged with trunk as its base. Never arm `--auto`, because without required checks GitHub merges it at once. After a merge lands, GitHub restacks the rest of a `gh stack` server-side. Run `gh stack sync`, confirm the next PR now targets trunk, and let step 3's patch-id rule decide whether its verdict survives. Merge it once its checks pass and its verdict survives. 5. **Leave the stack alone while it lands.** Step 4's rebase, push, and sync are the only changes to a branch or base, because a merge lands whatever its branch and base hold at that moment, verdict or not. Use `scripts/watch-pr/watch-pr --queued-stack --stack-prs ` only as an event wake under `/loop` in dynamic mode, and after each wake read `gh pr view --json state,mergedAt,mergeStateStatus,statusCheckRollup` for the current PR until `mergedAt` is set. A base that moves to trunk when its parent merges is expected, not damage to repair. If a merge stalls, find out why before you change anything. 6. **Stop at the ceiling.** Extending the run takes new verdicts, so it is a new pass through step 1. diff --git a/skills/poteto-mode/scripts/merge-gate/cli.test.ts b/skills/poteto-mode/scripts/merge-gate/cli.test.ts index 4ad401b..a05a154 100644 --- a/skills/poteto-mode/scripts/merge-gate/cli.test.ts +++ b/skills/poteto-mode/scripts/merge-gate/cli.test.ts @@ -25,17 +25,23 @@ afterEach(async () => { for (const cleanup of cleanups.splice(0)) await cleanup(); }); -interface Scenario { +interface PrFixture { + readonly head?: string; readonly headRefName?: string; + readonly baseRefName?: string; readonly isDraft?: boolean; readonly viewHeads?: readonly string[]; - readonly checks?: "passing" | "none"; + readonly checks?: "passing" | "pending" | "none"; + readonly outcome?: FakeRule[1]; + readonly defaultBranch?: string | null; + readonly stack?: number | null; +} + +interface Scenario extends PrFixture { readonly user?: FakeRule[1]; readonly merge?: FakeRule[1]; - readonly outcome?: FakeRule[1]; readonly asyncMerge?: FakeRule[1]; readonly asyncPolls?: FakeRule[1]; - readonly defaultBranch?: string | null; /** Contents of $HOME/.claude/pstack/merge-policy.json; null leaves it absent. */ readonly policy?: string | null; } @@ -54,38 +60,45 @@ const outcome = (state: string, mergeCommit: string | null): FakeResponse => ({ }, }); -function rules(scenario: Scenario = {}): readonly FakeRule[] { - const view = (head: string) => ({ +const CHECKS = { + passing: { name: "test", state: "SUCCESS", bucket: "pass", rollup: "SUCCESS" }, + pending: { name: "e2e", state: "PENDING", bucket: "pending", rollup: "PENDING" }, +} as const; + +function prRules(number: number, pr: PrFixture): FakeRule[] { + const head = pr.head ?? HEAD; + const view = (viewHead: string) => ({ stdout: { mergeable: "MERGEABLE", mergeStateStatus: "CLEAN", reviewDecision: "", - headRefOid: head, - headRefName: scenario.headRefName ?? "feature", - baseRefName: "main", + headRefOid: viewHead, + headRefName: pr.headRefName ?? "feature", + baseRefName: pr.baseRefName ?? "main", state: "OPEN", mergedAt: null, - isDraft: scenario.isDraft ?? false, + isDraft: pr.isDraft ?? false, }, }); const pullRequest = (value: unknown) => ({ stdout: { data: { repository: { pullRequest: value } } }, }); + const graphql = (query: string): readonly string[] => [`query ${query}`, `pr=${number}`]; + const checks = pr.checks === "none" ? null : CHECKS[pr.checks ?? "passing"]; return [ - ["api user", scenario.user ?? [{ stdout: { login: "agent" } }]], [ - "query MergeGatePr", + graphql("MergeGatePr"), [ { stdout: { data: { repository: { nameWithOwner: "acme/app", - defaultBranchRef: - scenario.defaultBranch === null ? null : { name: scenario.defaultBranch ?? "main" }, + defaultBranchRef: pr.defaultBranch === null ? null : { name: pr.defaultBranch ?? "main" }, pullRequest: { - headRefOid: HEAD, - baseRefName: "main", + headRefOid: head, + baseRefName: pr.baseRefName ?? "main", + stack: pr.stack === undefined || pr.stack === null ? null : { number: pr.stack }, changedFiles: 1, author: { __typename: "User", login: "agent" }, reviewThreads: { totalCount: 0 }, @@ -97,51 +110,68 @@ function rules(scenario: Scenario = {}): readonly FakeRule[] { }, ], ], - ["repos/acme/app/pulls/7/files", [{ stdout: [{ filename: "src/index.ts" }] }]], - ["pr view 7 --repo acme/app", (scenario.viewHeads ?? [HEAD]).map(view)], + [`repos/acme/app/pulls/${number}/files`, [{ stdout: [{ filename: "src/index.ts" }] }]], + [`pr view ${number} --repo acme/app`, (pr.viewHeads ?? [head]).map(view)], [ - "pr checks 7", - scenario.checks === "none" + `pr checks ${number}`, + checks === null ? [{ stderr: "no checks reported on the 'feature' branch\n", exit: 1 }] : [ { stdout: [ - { - name: "test", - state: "SUCCESS", - description: "", - link: "", - workflow: "ci", - bucket: "pass", - }, + { name: checks.name, state: checks.state, description: "", link: "", workflow: "ci", bucket: checks.bucket }, ], }, ], ], + [graphql("PrCheckRollup"), [pullRequest({ commits: { nodes: [{ commit: { statusCheckRollup: null } }] } })]], + [graphql("ReviewThreads"), [pullRequest({ reviewThreads: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } } })]], [ - "query PrCheckRollup", - [pullRequest({ commits: { nodes: [{ commit: { statusCheckRollup: null } }] } })], + graphql("PrCommitStatuses"), + [pullRequest({ commits: { nodes: [{ commit: { oid: head, statusCheckRollup: { state: checks?.rollup ?? "SUCCESS" } } }] } })], ], - ["query ReviewThreads", [pullRequest({ reviewThreads: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } } })]], - [ - "query PrCommitStatuses", - [ - pullRequest({ - commits: { - nodes: [{ commit: { oid: HEAD, statusCheckRollup: { state: "SUCCESS" } } }], + [graphql("MergeGateOutcome"), pr.outcome ?? [outcome("MERGED", MERGE_COMMIT)]], + ]; +} + +/** Stack fixtures put PR #(position + 4) at each position, on branch feature-(number). */ +const prAt = (position: number): number => position + 4; +type EntryState = "OPEN" | "CLOSED" | "MERGED"; + +const stackPage = (states: readonly EntryState[], { from = 1, cursor = null as string | null } = {}): FakeResponse => ({ + stdout: { + data: { + repository: { + pullRequest: { + stack: { + number: 42, + entries: { + pageInfo: { hasNextPage: cursor !== null, endCursor: cursor }, + nodes: states.map((state, index) => ({ + position: from + index, + pullRequest: { number: prAt(from + index), state, headRefName: `feature-${prAt(from + index)}` }, + })), + }, }, - }), - ], - ], + }, + }, + }, + }, +}); +const NOT_STACKED: FakeResponse = { stdout: { data: { repository: { pullRequest: { stack: null } } } } }; + +function rules(scenario: Scenario = {}): readonly FakeRule[] { + return [ + ["api user", scenario.user ?? [{ stdout: { login: "agent" } }]], + ...prRules(7, scenario), ["pr merge 7", scenario.merge ?? [{ stdout: "" }]], [`pulls/7/merge-async/${JOB}`, scenario.asyncPolls ?? [{ stdout: { status: "merged", details: { uuid: JOB } } }]], ["pulls/7/merge-async", scenario.asyncMerge ?? [{ stdout: { status: "pending", details: { uuid: JOB } } }]], - ["query MergeGateOutcome", scenario.outcome ?? [outcome("MERGED", MERGE_COMMIT)]], ]; } -async function setup(scenario: Scenario = {}): Promise { - const gh = await fakeGh(rules(scenario)); +async function setup(scenario: Scenario = {}, fixture: readonly FakeRule[] = rules(scenario)): Promise { + const gh = await fakeGh(fixture); cleanups.push(gh.cleanup); const policy = scenario.policy === undefined ? JSON.stringify(POLICY) : scenario.policy; if (policy !== null) @@ -273,7 +303,7 @@ describe("merge-gate CLI against a fake gh", () => { expect(await merges(gh)).toEqual([...expectedMerges]); }); - const subject = { repo: "acme/app", pr: 7, head: HEAD }; + const subject = { repo: "acme/app", pr: 7, head: HEAD, stack: null }; it.each<[string, Scenario, number, Verdict]>([ [ "reports a merge that landed despite a non-zero exit", @@ -466,6 +496,7 @@ describe("merge-gate CLI against a fake gh", () => { repo: "acme/app", pr: 7, head: null, + stack: null, decision: "refuse", merged: false, mergeCommit: null, @@ -500,6 +531,207 @@ describe("merge-gate CLI against a fake gh", () => { }); }); +const HEAD_5 = "5555555555555555555555555555555555555555"; +const HEAD_6 = "6666666666666666666666666666666666666666"; +const STACK_PRS: Readonly> = { + [prAt(1)]: { head: HEAD_5, headRefName: "feature-5", baseRefName: "main", stack: 42 }, + [prAt(2)]: { head: HEAD_6, headRefName: "feature-6", baseRefName: "feature-5", stack: 42 }, + [prAt(3)]: { head: HEAD, headRefName: "feature-7", baseRefName: "feature-6", stack: 42 }, +}; + +interface StackScenario { + readonly stack?: FakeRule[1]; + readonly prs?: Readonly>; + readonly asyncPolls?: FakeRule[1]; +} + +function stackRules(scenario: StackScenario): readonly FakeRule[] { + return [ + ["api user", [{ stdout: { login: "agent" } }]], + ["query MergeGateStack", scenario.stack ?? [stackPage(["OPEN", "OPEN", "OPEN"])]], + ...[1, 2, 3].map(prAt).flatMap((number) => prRules(number, { ...STACK_PRS[number], ...scenario.prs?.[number] })), + ["pr merge", [{ stderr: "merge-gate must not call gh pr merge on a stack\n", exit: 1 }]], + [`pulls/7/merge-async/${JOB}`, scenario.asyncPolls ?? [{ stdout: { status: "merged", details: { uuid: JOB } } }]], + ["pulls/7/merge-async", [{ stdout: { status: "pending", details: { uuid: JOB } } }]], + ]; +} + +describe("merge-gate --stack against a fake gh", () => { + const gatedPrs = async (gh: FakeGh) => + (await gh.calls()).flatMap((argv) => { + const match = /^repos\/acme\/app\/pulls\/(\d+)\/files/.exec(argv[1] ?? ""); + return match === null ? [] : [Number(match[1])]; + }); + const subject = (prs: readonly number[] | null, head: string | null = HEAD) => ({ + repo: "acme/app", + pr: 7, + head, + stack: prs === null ? null : { number: 42, prs }, + }); + const merged = (prs: readonly number[]): Verdict => ({ + ...subject(prs), + decision: "merge", + merged: true, + mergeCommit: MERGE_COMMIT, + reasons: [], + }); + const mergeable = (prs: readonly number[]): Verdict => ({ ...merged(prs), merged: false, mergeCommit: null }); + const refused = (base: ReturnType, code: ReasonCode, detail: string): Verdict => ({ + ...base, + decision: "refuse", + merged: false, + mergeCommit: null, + reasons: [{ code, detail }], + }); + const ALL = [5, 6, 7]; + const UNSETTLED = + "so the stack may still merge. Wait for gh pr view to show every PR settled before rerunning merge-gate --stack --merge."; + const open = { outcome: [outcome("OPEN", null)] }; + it.each<{ + name: string; + scenario: StackScenario; + merge: boolean; + exit: number; + verdict: Verdict; + gated: readonly number[]; + calls: readonly (readonly string[])[]; + }>([ + { + name: "merges the whole range with one merge-async call pinned to the top head", + scenario: {}, + merge: true, + exit: 0, + verdict: merged(ALL), + gated: ALL, + calls: [asyncPut, asyncPoll], + }, + { name: "reports the range as mergeable without merging", scenario: {}, merge: false, exit: 0, verdict: mergeable(ALL), gated: ALL, calls: [] }, + { + name: "reads stack entries across pages", + scenario: { stack: [stackPage(["OPEN", "OPEN"], { cursor: "Mg" }), stackPage(["OPEN"], { from: 3 })] }, + merge: false, + exit: 0, + verdict: mergeable(ALL), + gated: ALL, + calls: [], + }, + { + name: "refuses a stack whose positions skip one", + scenario: { stack: [stackPage(["OPEN"], { cursor: "MQ" }), stackPage(["OPEN"], { from: 3 })] }, + merge: true, + exit: 2, + verdict: refused(subject(null, null), "github-error", "stack #42 lists positions 1, 3"), + gated: [], + calls: [], + }, + { + name: "refuses a middle PR with pending checks and names it", + scenario: { prs: { 6: { checks: "pending" } } }, + merge: true, + exit: 2, + verdict: refused(subject(ALL), "checks-pending", "#6: pending: e2e"), + gated: ALL, + calls: [], + }, + { + name: "refuses a PR whose base is not the head of the PR below it", + scenario: { prs: { 7: { baseRefName: "feature-5" } } }, + merge: true, + exit: 2, + verdict: refused(subject(ALL), "base-not-trunk", "#7: base is feature-5, the head of #6 below it is feature-6"), + gated: ALL, + calls: [], + }, + { + name: "refuses a stack that targets a branch other than the default", + scenario: { prs: { 5: { baseRefName: "release" } } }, + merge: true, + exit: 2, + verdict: refused(subject(ALL), "base-not-trunk", "#5: base is release, the default branch is main"), + gated: ALL, + calls: [], + }, + { + name: "skips a merged lower entry and gates the rest against trunk", + scenario: { stack: [stackPage(["MERGED", "OPEN", "OPEN"])], prs: { 6: { baseRefName: "main" } } }, + merge: true, + exit: 0, + verdict: merged([6, 7]), + gated: [6, 7], + calls: [asyncPut, asyncPoll], + }, + { + name: "refuses a closed lower entry", + scenario: { stack: [stackPage(["CLOSED", "OPEN", "OPEN"])] }, + merge: true, + exit: 2, + verdict: refused(subject([6, 7], null), "not-open", "#5: PR is closed"), + gated: [], + calls: [], + }, + { + name: "refuses a PR that is not in a native stack", + scenario: { stack: [NOT_STACKED] }, + merge: true, + exit: 2, + verdict: refused(subject(null, null), "invalid-arguments", "#7 is not in a native stack; rerun without --stack"), + gated: [], + calls: [], + }, + { + name: "refuses when a lower head moves before merging", + scenario: { prs: { 5: { viewHeads: [HEAD_5, MOVED] } } }, + merge: true, + exit: 2, + verdict: refused(subject(ALL), "head-moved", `#5: head was ${HEAD_5}, then ${MOVED} before merging`), + gated: ALL, + calls: [], + }, + { + name: "reports an unverified merge when one PR in the range does not read merged", + scenario: { prs: { 6: open } }, + merge: true, + exit: 1, + verdict: refused(subject(ALL), "merge-unverified", `merge-async job ${JOB} reported merged; #6 is OPEN, ${UNSETTLED}`), + gated: ALL, + calls: [asyncPut, asyncPoll], + }, + { + name: "refuses a failed merge when no PR in the range merged", + scenario: { + asyncPolls: [{ stdout: { status: "failed", details: { message: "Merge conflict" } } }], + prs: { 5: open, 6: open, 7: open }, + }, + merge: true, + exit: 2, + verdict: refused(subject(ALL), "merge-failed", `merge-async job ${JOB} failed: Merge conflict; #5 is OPEN, #6 is OPEN, #7 is OPEN`), + gated: ALL, + calls: [asyncPut, asyncPoll], + }, + ])("$name", async ({ scenario, merge, exit, verdict, gated, calls }) => { + const gh = await setup({}, stackRules(scenario)); + const result = runCli(gh, ["7", "--repo", "acme/app", "--stack", ...(merge ? ["--merge"] : [])]); + expect(result).toEqual({ exit, verdict }); + expect((await gatedPrs(gh)).sort()).toEqual([...gated]); + expect([...(await merges(gh)), ...(await asyncMerges(gh))]).toEqual([...calls]); + }); + + it("points a plain run at --stack when the PR is in a native stack", async () => { + const gh = await setup({ baseRefName: "feature-6", stack: 42 }); + const result = runCli(gh, ["7", "--repo", "acme/app", "--merge"]); + expect(result).toEqual({ + exit: 2, + verdict: refused( + subject(null), + "base-not-trunk", + "base is feature-6, the default branch is main; PR is in native stack #42, so rerun with --stack to gate it with the PRs below it" + ), + }); + expect(await gh.calls()).not.toContainEqual(expect.arrayContaining([expect.stringContaining("query MergeGateStack")])); + expect(await merges(gh)).toEqual([]); + }); +}); + describe("main", () => { it("exits 1 and refuses on an unexpected error", async () => { const directory = await mkdtemp(join(tmpdir(), "merge-gate-test-")); @@ -518,6 +750,8 @@ describe("main", () => { facts: unexpected, head: unexpected, merge: unexpected, + stack: unexpected, + mergeStack: unexpected, }, stdout: (text) => stdout.push(text), stderr: () => {}, @@ -527,6 +761,7 @@ describe("main", () => { repo: "acme/app", pr: 7, head: null, + stack: null, decision: "refuse", merged: false, mergeCommit: null, diff --git a/skills/poteto-mode/scripts/merge-gate/cli.ts b/skills/poteto-mode/scripts/merge-gate/cli.ts index 76bc7a0..acb146a 100644 --- a/skills/poteto-mode/scripts/merge-gate/cli.ts +++ b/skills/poteto-mode/scripts/merge-gate/cli.ts @@ -3,15 +3,17 @@ import { join } from "node:path"; import { Command, CommanderError, InvalidArgumentError } from "commander"; import { WatcherQueryError } from "../watch-pr/github.ts"; import type * as W from "../watch-pr/types.ts"; -import { parsePrNumber } from "../watch-pr/types.ts"; -import { type Reason, type ReasonCode, decide } from "./decide.ts"; -import { type GateGitHub, GhGateGitHub, GitHubError } from "./github.ts"; +import { nonEmpty, parsePrNumber } from "../watch-pr/types.ts"; +import { type Reason, type ReasonCode, decidePlan, plainPlan, planStack } from "./decide.ts"; +import { type GateGitHub, GhGateGitHub, GitHubError, type MergeResult } from "./github.ts"; import { PolicyError, loadPolicy } from "./policy.ts"; interface Subject { readonly repo: string | null; readonly pr: number | null; readonly head: string | null; + /** With --stack, the native stack and the PRs gated, bottom first; null without --stack or before the stack is read. */ + readonly stack: { readonly number: number; readonly prs: readonly number[] } | null; } export type Verdict = Subject & @@ -40,6 +42,7 @@ interface Request { readonly number: W.PrNumber; readonly repo: W.Repository | null; readonly merge: boolean; + readonly stack: boolean; } export interface Runtime { @@ -77,17 +80,19 @@ function target(value: string): { readonly number: W.PrNumber; readonly repo: W. function parseArgs(argv: readonly string[], runtime: Runtime): Request { const program = new Command("merge-gate") .description( - "Decide whether an agent may merge one PR under the merge policy, and with --merge, merge it.\nPrints one JSON verdict. Exit 0: mergeable or merged, 2: refused, 1: internal error." + "Decide whether an agent may merge one PR under the merge policy, and with --merge, merge it.\nWith --stack, gate every open PR from the bottom of its GitHub native stack up to this one, and merge them together.\nPrints one JSON verdict. Exit 0: mergeable or merged, 2: refused, 1: internal error." ) .configureOutput({ writeOut: runtime.stdout, writeErr: runtime.stderr }) .exitOverride() .argument("", "PR number or GitHub PR URL", target) .option("--repo ", "repository; defaults to the checkout's origin", repository) - .option("--merge", "merge when the verdict is merge", false); + .option("--merge", "merge when the verdict is merge", false) + .option("--stack", "gate and merge native stack positions 1 to this PR as one unit", false); program.parse(argv, { from: "user" }); const options = program.opts<{ readonly repo?: W.Repository; readonly merge: boolean; + readonly stack: boolean; }>(); const pr: ReturnType = program.processedArgs[0]; if (pr.repo !== null && options.repo !== undefined && slug(pr.repo).toLowerCase() !== slug(options.repo).toLowerCase()) @@ -96,50 +101,78 @@ function parseArgs(argv: readonly string[], runtime: Runtime): Request { number: pr.number, repo: pr.repo ?? options.repo ?? null, merge: options.merge, + stack: options.stack, }; } -const refuse = (subject: Subject, code: ReasonCode, detail: string): Verdict => ({ +const refuse = (subject: Subject, reasons: W.NonEmpty): Verdict => ({ ...subject, decision: "refuse", merged: false, mergeCommit: null, - reasons: [{ code, detail }], + reasons, }); +const refuseOne = (subject: Subject, code: ReasonCode, detail: string): Verdict => refuse(subject, [{ code, detail }]); + +/** A merge decision, after the merge when `merge` is given. */ +function approve(subject: Subject, merge: MergeResult | null): Verdict { + if (merge === null) return { ...subject, decision: "merge", merged: false, mergeCommit: null, reasons: [] }; + return merge.kind === "merged" + ? { ...subject, decision: "merge", merged: true, mergeCommit: merge.commit, reasons: [] } + : refuseOne(subject, merge.kind === "unverified" ? "merge-unverified" : "merge-failed", merge.detail); +} + async function evaluate(request: Request, runtime: Runtime): Promise { const { github } = runtime; let subject: Subject = { repo: request.repo === null ? null : slug(request.repo), pr: request.number, head: null, + stack: null, }; try { const policy = await loadPolicy(runtime.policyPath); const repo = request.repo ?? (await github.originRepo()); if (repo === null) - return refuse(subject, "invalid-arguments", "cannot infer the repository from the origin remote; pass --repo owner/name"); + return refuseOne(subject, "invalid-arguments", "cannot infer the repository from the origin remote; pass --repo owner/name"); subject = { ...subject, repo: slug(repo) }; - const viewer = await github.viewer(); - const facts = await github.facts({ ...repo, number: request.number }); - subject = { repo: slug(facts.pr), pr: facts.pr.number, head: facts.head }; - const decision = decide(policy, facts, viewer); - if (decision.kind === "refuse") - return { ...subject, decision: "refuse", merged: false, mergeCommit: null, reasons: decision.reasons }; - if (!request.merge) - return { ...subject, decision: "merge", merged: false, mergeCommit: null, reasons: [] }; - const head = await github.head(facts.pr); - if (head !== facts.head) - return refuse(subject, "head-moved", `head was ${facts.head}, then ${head ?? "unknown"} before merging`); - const merge = await github.merge(facts.pr, facts.head, decision.method); - return merge.kind === "merged" - ? { ...subject, decision: "merge", merged: true, mergeCommit: merge.commit, reasons: [] } - : refuse(subject, merge.kind === "unverified" ? "merge-unverified" : "merge-failed", merge.detail); + const at = (number: W.PrNumber): W.PrContext => ({ ...repo, number }); + const [viewer, stack] = await Promise.all([github.viewer(), request.stack ? github.stack(at(request.number)) : undefined]); + if (stack === null) + return refuseOne(subject, "invalid-arguments", `#${request.number} is not in a native stack; rerun without --stack`); + const { plan, reasons: blocked } = + stack === undefined ? { plan: plainPlan(request.number), reasons: [] } : planStack(stack); + if (plan.stack !== null) + subject = { ...subject, stack: { number: plan.stack, prs: [...plan.below, plan.top].map((pr) => pr.number) } }; + const unplanned = nonEmpty(blocked); + if (unplanned !== null) return refuse(subject, unplanned); + const [top, ...below] = await Promise.all( + [plan.top, ...plan.below].map(async ({ number, base }) => ({ facts: await github.facts(at(number)), base })) + ); + subject = { ...subject, repo: slug(top.facts.pr), pr: top.facts.pr.number, head: top.facts.head }; + const decision = decidePlan(policy, viewer, plan.stack, below, top); + if (decision.kind === "refuse") return refuse(subject, decision.reasons); + if (!request.merge) return approve(subject, null); + const gated = [...below, top].map(({ facts }) => facts); + const heads = await Promise.all(gated.map((facts) => github.head(facts.pr))); + const moved = gated.findIndex((facts, index) => heads[index] !== facts.head); + if (moved >= 0) { + const { pr, head } = gated[moved]; + const tag = plan.stack === null ? "" : `#${pr.number}: `; + return refuseOne(subject, "head-moved", `${tag}head was ${head}, then ${heads[moved] ?? "unknown"} before merging`); + } + return approve( + subject, + plan.stack === null + ? await github.merge(top.facts.pr, top.facts.head, decision.method) + : await github.mergeStack(top.facts.pr, top.facts.head, decision.method, below.map(({ facts }) => facts.pr)) + ); } catch (error) { - if (error instanceof PolicyError) return refuse(subject, "policy-invalid", error.message); + if (error instanceof PolicyError) return refuseOne(subject, "policy-invalid", error.message); if (error instanceof GitHubError || error instanceof WatcherQueryError) - return refuse(subject, "github-error", error.message); - return refuse(subject, "internal-error", String(error)); + return refuseOne(subject, "github-error", error.message); + return refuseOne(subject, "internal-error", String(error)); } } @@ -162,8 +195,8 @@ export async function main( verdict = await evaluate(parseArgs(argv, runtime), runtime); } catch (error) { if (error instanceof CommanderError && error.exitCode === 0) return 0; - verdict = refuse( - { repo: null, pr: null, head: null }, + verdict = refuseOne( + { repo: null, pr: null, head: null, stack: null }, error instanceof CommanderError ? "invalid-arguments" : "internal-error", String(error) ); diff --git a/skills/poteto-mode/scripts/merge-gate/decide.test.ts b/skills/poteto-mode/scripts/merge-gate/decide.test.ts index f0323be..3ce25a5 100644 --- a/skills/poteto-mode/scripts/merge-gate/decide.test.ts +++ b/skills/poteto-mode/scripts/merge-gate/decide.test.ts @@ -11,10 +11,17 @@ import type { import { parsePrNumber } from "../watch-pr/types.ts"; import { type Decision, + type ExpectedBase, + type NativeStack, type PrFacts, type Readiness, + type ReasonCode, type Review, + type StackEntry, + TRUNK, decide, + decidePlan, + planStack, } from "./decide.ts"; import { parsePolicy } from "./policy.ts"; @@ -111,6 +118,7 @@ interface Scenario { readonly ci?: CiState; readonly threads?: readonly ReviewThread[]; readonly paths?: readonly string[]; + readonly base?: ExpectedBase; } function run(scenario: Scenario = {}): Decision { @@ -148,24 +156,22 @@ function run(scenario: Scenario = {}): Decision { path, previousPath: null, })); - return decide( - policy, - { - pr: context, - head: HEAD, - base: "main", - defaultBranch: "main", - author: { login: "agent", type: "User" }, - reviews: [], - reviewThreadCount: threads.length, - reviewCount: 0, - changedFileCount: files.length, - files, - readiness, - ...scenario.facts, - }, - scenario.viewer ?? "agent" - ); + const prFacts: PrFacts = { + pr: context, + head: HEAD, + base: "main", + defaultBranch: "main", + stack: null, + author: { login: "agent", type: "User" }, + reviews: [], + reviewThreadCount: threads.length, + reviewCount: 0, + changedFileCount: files.length, + files, + readiness, + ...scenario.facts, + }; + return decide(policy, prFacts, scenario.viewer ?? "agent", scenario.base ?? TRUNK); } const codes = (decision: Decision): readonly string[] => @@ -354,3 +360,165 @@ describe("merge method", () => { expect(codes(run({ repo, pr: { headRefName } }))).toEqual(expected); }); }); + +describe("expected base", () => { + const below: ExpectedBase = { kind: "below", number: parsePrNumber(6), branch: "feature-6" }; + it.each<[string, Scenario, Decision]>([ + ["accepts a stacked PR whose base is the head below it", { facts: { base: "feature-6" }, base: below }, { kind: "merge", method: "squash" }], + [ + "refuses a stacked PR whose base is not the head below it", + { facts: { base: "feature-5" }, base: below }, + { kind: "refuse", reasons: [{ code: "base-not-trunk", detail: "base is feature-5, the head of #6 below it is feature-6" }] }, + ], + [ + "refuses trunk when the head below it is expected", + { base: below }, + { kind: "refuse", reasons: [{ code: "base-not-trunk", detail: "base is main, the head of #6 below it is feature-6" }] }, + ], + [ + "names the default branch for a plain PR", + { facts: { base: "release" } }, + { kind: "refuse", reasons: [{ code: "base-not-trunk", detail: "base is release, the default branch is main" }] }, + ], + ])("%s", (_name, scenario, expected) => { + expect(run(scenario)).toEqual(expected); + }); +}); + +/** Stack fixtures put PR #(position + 4) at each position, on branch feature-(number). */ +const prAt = (position: number) => parsePrNumber(position + 4); +const entry = (position: number, state: StackEntry["state"] = "OPEN"): StackEntry => ({ + position, + number: prAt(position), + state, + headRefName: `feature-${prAt(position)}`, +}); +const onTop = (position: number): ExpectedBase => ({ kind: "below", number: prAt(position), branch: `feature-${prAt(position)}` }); + +describe("planStack", () => { + const stack = (states: readonly StackEntry["state"][]): NativeStack => ({ + number: 42, + below: states.slice(0, -1).map((state, index) => entry(index + 1, state)), + target: entry(states.length, states.at(-1)), + }); + it.each<{ name: string; states: readonly StackEntry["state"][]; below: readonly [number, ExpectedBase][]; top: ExpectedBase; reasons: readonly string[] }>([ + { + name: "gates every PR below the target, each against the head below it", + states: ["OPEN", "OPEN", "OPEN"], + below: [[1, TRUNK], [2, onTop(1)]], + top: onTop(2), + reasons: [], + }, + { + name: "skips a merged lower entry and expects trunk under the lowest open one", + states: ["MERGED", "OPEN", "OPEN"], + below: [[2, TRUNK]], + top: onTop(2), + reasons: [], + }, + { name: "gates a target at position 1 alone", states: ["OPEN"], below: [], top: TRUNK, reasons: [] }, + { + name: "refuses a closed lower entry", + states: ["CLOSED", "OPEN", "OPEN"], + below: [[2, TRUNK]], + top: onTop(2), + reasons: ["#5: PR is closed"], + }, + { name: "refuses a merged target", states: ["MERGED", "MERGED"], below: [], top: TRUNK, reasons: ["#6: PR is merged"] }, + ])("$name", ({ states, below, top, reasons }) => { + expect(planStack(stack(states))).toEqual({ + plan: { + stack: 42, + below: below.map(([position, base]) => ({ number: prAt(position), base })), + top: { number: prAt(states.length), base: top }, + }, + reasons: reasons.map((detail) => ({ code: "not-open", detail })), + }); + }); +}); + +describe("decidePlan", () => { + const gated = (position: number, base: ExpectedBase, facts: Partial = {}) => ({ + base, + facts: { + pr: { owner: "acme", repo: "app", number: prAt(position) }, + head: HEAD, + base: base.kind === "trunk" ? "main" : base.branch, + defaultBranch: "main", + stack: 42, + author: { login: "agent", type: "User" }, + reviews: [], + reviewThreadCount: 0, + reviewCount: 0, + changedFileCount: 1, + files: [{ path: "src/index.ts", previousPath: null }], + readiness: { + kind: "open", + context: { owner: "acme", repo: "app", number: prAt(position) }, + facts: { + context: { owner: "acme", repo: "app", number: prAt(position) }, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: null, + headRefOid: HEAD, + headRefName: `feature-${prAt(position)}`, + baseRefName: base.kind === "trunk" ? "main" : base.branch, + state: "OPEN", + mergedAt: null, + isDraft: false, + }, + threads: [], + ci: clean([passingCheck()]), + reviewAutomationRunning: false, + }, + ...facts, + } satisfies PrFacts, + }); + it.each<{ name: string; stack: number | null; below: readonly ReturnType[]; top: ReturnType; expected: Decision }>([ + { + name: "merges a stack whose every PR passes", + stack: 42, + below: [gated(1, TRUNK), gated(2, onTop(1))], + top: gated(3, onTop(2)), + expected: { kind: "merge", method: "squash" }, + }, + { + name: "tags each refusal with its PR in a stack", + stack: 42, + below: [gated(1, TRUNK, { base: "release" }), gated(2, onTop(1))], + top: gated(3, onTop(2), { base: "feature-5" }), + expected: { + kind: "refuse", + reasons: [ + { code: "base-not-trunk", detail: "#5: base is release, the default branch is main" }, + { code: "base-not-trunk", detail: "#7: base is feature-5, the head of #6 below it is feature-6" }, + ], + }, + }, + { + name: "points a plain run on a stacked PR at --stack", + stack: null, + below: [], + top: gated(3, TRUNK, { base: "feature-6" }), + expected: { + kind: "refuse", + reasons: [ + { + code: "base-not-trunk", + detail: + "base is feature-6, the default branch is main; PR is in native stack #42, so rerun with --stack to gate it with the PRs below it", + }, + ], + }, + }, + { + name: "gives no hint for a plain PR outside a stack", + stack: null, + below: [], + top: gated(3, TRUNK, { base: "release", stack: null }), + expected: { kind: "refuse", reasons: [{ code: "base-not-trunk", detail: "base is release, the default branch is main" }] }, + }, + ])("$name", ({ stack, below, top, expected }) => { + expect(decidePlan(policy, "agent", stack, below, top)).toEqual(expected); + }); +}); diff --git a/skills/poteto-mode/scripts/merge-gate/decide.ts b/skills/poteto-mode/scripts/merge-gate/decide.ts index e8fc273..7c53a4a 100644 --- a/skills/poteto-mode/scripts/merge-gate/decide.ts +++ b/skills/poteto-mode/scripts/merge-gate/decide.ts @@ -74,6 +74,8 @@ export interface PrFacts { /** The PR's base branch and the repo's default branch; null when GitHub reports none. */ readonly base: string | null; readonly defaultBranch: string | null; + /** The number of the native stack the PR is in, or null. */ + readonly stack: number | null; readonly author: Actor | null; readonly reviews: readonly Review[]; readonly reviewThreadCount: number; @@ -83,6 +85,44 @@ export interface PrFacts { readonly readiness: Readiness; } +/** The branch a PR must target: trunk, or in a native stack the head of the open PR below it. */ +export type ExpectedBase = + | { readonly kind: "trunk" } + | { readonly kind: "below"; readonly number: W.PrNumber; readonly branch: string }; + +export const TRUNK: ExpectedBase = { kind: "trunk" }; + +export interface StackEntry { + readonly position: number; + readonly number: W.PrNumber; + readonly state: "OPEN" | "CLOSED" | "MERGED"; + readonly headRefName: string; +} + +/** A GitHub native stack read for its PR `target`; `below` holds every entry under it, bottom first. */ +export interface NativeStack { + readonly number: number; + readonly below: readonly StackEntry[]; + readonly target: StackEntry; +} + +export interface PlannedPr { + readonly number: W.PrNumber; + readonly base: ExpectedBase; +} + +/** The PRs one run gates and merges: `top` is the requested PR, `below` the open stack PRs under it. */ +export interface Plan { + readonly stack: number | null; + readonly below: readonly PlannedPr[]; + readonly top: PlannedPr; +} + +export interface Gated { + readonly facts: PrFacts; + readonly base: ExpectedBase; +} + export type MergeMethod = "squash" | "merge"; export type Decision = @@ -118,7 +158,7 @@ const CHECK_OUTCOME: Record = { const reason = (code: ReasonCode, detail: string): Reason => ({ code, detail }); -export function decide(policy: Policy, facts: PrFacts, viewer: string): Decision { +export function decide(policy: Policy, facts: PrFacts, viewer: string, base: ExpectedBase): Decision { const repo = repoPolicy(policy, facts.pr); const stances = latestStances(facts.reviews); if (repo === null) @@ -129,11 +169,11 @@ export function decide(policy: Policy, facts: PrFacts, viewer: string): Decision "repo-not-allowed", `${facts.pr.owner}/${facts.pr.repo} matches no repos entry in the merge policy` ), - ...pullRequestReasons(facts, viewer, stances), + ...pullRequestReasons(facts, viewer, base, stances), ], }; const reasons = nonEmpty([ - ...pullRequestReasons(facts, viewer, stances), + ...pullRequestReasons(facts, viewer, base, stances), ...approvalReasons(repo, facts, stances), ]); if (reasons !== null) return { kind: "refuse", reasons }; @@ -160,6 +200,7 @@ function latestStances(reviews: readonly Review[]): readonly Stance[] { function pullRequestReasons( facts: PrFacts, viewer: string, + base: ExpectedBase, stances: readonly Stance[] ): Reason[] { const { readiness } = facts; @@ -175,13 +216,12 @@ function pullRequestReasons( `PR author is ${facts.author?.login ?? "unknown"}, gh is authenticated as ${viewer}` ) ); - if (facts.base === null || facts.base !== facts.defaultBranch) - reasons.push( - reason( - "base-not-trunk", - `base is ${facts.base ?? "unknown"}, the default branch is ${facts.defaultBranch ?? "unknown"}` - ) - ); + const [branch, name] = + base.kind === "trunk" + ? [facts.defaultBranch, "the default branch"] + : [base.branch, `the head of #${base.number} below it`]; + if (facts.base === null || facts.base !== branch) + reasons.push(reason("base-not-trunk", `base is ${facts.base ?? "unknown"}, ${name} is ${branch ?? "unknown"}`)); reasons.push(...changesRequestedReasons(pr.reviewDecision, stances)); if (readiness.kind === "open" || readiness.kind === "no-checks") reasons.push( @@ -310,3 +350,52 @@ function humanApprovedHead(facts: PrFacts, stances: readonly Stance[]): boolean WRITE_ACCESS.has(review.authorAssociation) ); } + +export const plainPlan = (number: W.PrNumber): Plan => ({ stack: null, below: [], top: { number, base: TRUNK } }); + +// Merged entries below the target are already in trunk. Each open PR must sit +// on the open PR directly below it, and the lowest on trunk, because +// merge-async lands the whole range into the stack's base in one go. +export function planStack(stack: NativeStack): { readonly plan: Plan; readonly reasons: readonly Reason[] } { + const open = stack.below.filter((entry) => entry.state === "OPEN"); + const base = (under: StackEntry | undefined): ExpectedBase => + under === undefined ? TRUNK : { kind: "below", number: under.number, branch: under.headRefName }; + return { + plan: { + stack: stack.number, + below: open.map((entry, index) => ({ number: entry.number, base: base(open[index - 1]) })), + top: { number: stack.target.number, base: base(open.at(-1)) }, + }, + reasons: [...stack.below.filter((entry) => entry.state === "CLOSED"), ...(stack.target.state === "OPEN" ? [] : [stack.target])].map( + (entry) => reason("not-open", `#${entry.number}: PR is ${entry.state.toLowerCase()}`) + ), + }; +} + +/** Gates every PR of a plan; it merges only when all pass, by the method the top PR's branch calls for. */ +export function decidePlan( + policy: Policy, + viewer: string, + stack: number | null, + below: readonly Gated[], + top: Gated +): Decision { + const decision = decide(policy, top.facts, viewer, top.base); + const refusals = (facts: PrFacts, verdict: Decision): readonly Reason[] => + verdict.kind === "merge" + ? [] + : verdict.reasons.map((refusal) => + stack === null ? withStackHint(refusal, facts) : { ...refusal, detail: `#${facts.pr.number}: ${refusal.detail}` } + ); + const reasons = nonEmpty([ + ...below.flatMap(({ facts, base }) => refusals(facts, decide(policy, facts, viewer, base))), + ...refusals(top.facts, decision), + ]); + return reasons === null ? decision : { kind: "refuse", reasons }; +} + +function withStackHint(refusal: Reason, facts: PrFacts): Reason { + return refusal.code === "base-not-trunk" && facts.stack !== null + ? { ...refusal, detail: `${refusal.detail}; PR is in native stack #${facts.stack}, so rerun with --stack to gate it with the PRs below it` } + : refusal; +} diff --git a/skills/poteto-mode/scripts/merge-gate/fake-gh.test-helper.ts b/skills/poteto-mode/scripts/merge-gate/fake-gh.test-helper.ts index 2ca8902..972d747 100644 --- a/skills/poteto-mode/scripts/merge-gate/fake-gh.test-helper.ts +++ b/skills/poteto-mode/scripts/merge-gate/fake-gh.test-helper.ts @@ -10,11 +10,12 @@ export interface FakeResponse { } /** - * Each rule answers the gh invocations whose joined argv contains `match`. + * Each rule answers the gh invocations whose joined argv contains `match`, + * or every string in it. * Responses are served in order and the last one repeats, which is how a * scenario makes the head move between the read and the merge. */ -export type FakeRule = readonly [match: string, responses: readonly FakeResponse[]]; +export type FakeRule = readonly [match: string | readonly string[], responses: readonly FakeResponse[]]; export interface FakeGh { readonly directory: string; @@ -27,25 +28,32 @@ export async function fakeGh(rules: readonly FakeRule[]): Promise { const directory = await mkdtemp(join(tmpdir(), "merge-gate-test-")); const bin = join(directory, "bin"); const log = join(directory, "calls.ndjson"); - const counts = join(directory, "counts.json"); + const slots = join(directory, "slots"); await Bun.write(join(directory, "rules.json"), JSON.stringify(rules)); await Bun.write( join(bin, "gh"), `#!${process.execPath} -import { appendFileSync, existsSync, readFileSync, writeFileSync } from "node:fs"; +import { appendFileSync, mkdirSync, readFileSync } from "node:fs"; const args = process.argv.slice(2); appendFileSync(${JSON.stringify(log)}, JSON.stringify(args) + "\\n"); const rules = JSON.parse(readFileSync(${JSON.stringify(join(directory, "rules.json"))}, "utf8")); const line = args.join(" "); -const index = rules.findIndex(([match]) => line.includes(match)); +const index = rules.findIndex(([match]) => [match].flat().every((part) => line.includes(part))); if (index < 0) { process.stderr.write("unexpected gh arguments: " + line + "\\n"); process.exit(97); } -const counts = existsSync(${JSON.stringify(counts)}) ? JSON.parse(readFileSync(${JSON.stringify(counts)}, "utf8")) : {}; -const seen = counts[index] ?? 0; -counts[index] = seen + 1; -writeFileSync(${JSON.stringify(counts)}, JSON.stringify(counts)); +// merge-gate runs gh calls concurrently; mkdir claims each call's slot atomically. +mkdirSync(${JSON.stringify(slots)}, { recursive: true }); +let seen = 0; +for (;;) { + try { + mkdirSync(${JSON.stringify(slots)} + "/" + index + "-" + seen); + break; + } catch { + seen += 1; + } +} const responses = rules[index][1]; const response = responses[Math.min(seen, responses.length - 1)]; if (response.stdout !== undefined) diff --git a/skills/poteto-mode/scripts/merge-gate/github.ts b/skills/poteto-mode/scripts/merge-gate/github.ts index ff70520..02da64a 100644 --- a/skills/poteto-mode/scripts/merge-gate/github.ts +++ b/skills/poteto-mode/scripts/merge-gate/github.ts @@ -1,14 +1,17 @@ import { ChecksUnavailable, GhGitHubReader, resolveReviewThreads } from "../watch-pr/github.ts"; import { readSnapshot } from "../watch-pr/policy.ts"; import type * as W from "../watch-pr/types.ts"; +import { parsePrNumber } from "../watch-pr/types.ts"; import type { Actor, ChangedFile, MergeMethod, + NativeStack, PrFacts, Readiness, Review, ReviewState, + StackEntry, } from "./decide.ts"; /** What GitHub reports after the merge request, whatever the request itself said. */ @@ -23,6 +26,10 @@ export interface GateGitHub { facts(target: W.PrContext): Promise; head(pr: W.PrContext): Promise; merge(pr: W.PrContext, head: string, method: MergeMethod): Promise; + /** The native stack `pr` belongs to, or null when it is in none. */ + stack(pr: W.PrContext): Promise; + /** One merge-async call on `top`, which lands every PR below it too; `below` is read back to confirm. */ + mergeStack(top: W.PrContext, head: string, method: MergeMethod, below: readonly W.PrContext[]): Promise; } /** A gh call failed or answered with a shape merge-gate does not recognize. */ @@ -41,6 +48,7 @@ query MergeGatePr($owner: String!, $repo: String!, $pr: Int!) { pullRequest(number: $pr) { headRefOid baseRefName + stack { number } changedFiles author { __typename login } reviewThreads { totalCount } @@ -68,6 +76,23 @@ query MergeGateOutcome($owner: String!, $repo: String!, $pr: Int!) { } } `; +const STACK_QUERY = ` +query MergeGateStack($owner: String!, $repo: String!, $pr: Int!, $after: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr) { + stack { + number + entries(first: 100, after: $after) { + pageInfo { hasNextPage endCursor } + nodes { position pullRequest { number state headRefName } } + } + } + } + } +} +`; +const MAX_STACK_PAGES = 10; +const ENTRY_STATES = ["OPEN", "CLOSED", "MERGED"] as const satisfies readonly StackEntry["state"][]; /** GitHub refuses `gh pr merge` for a PR in a native stack and wants its merge-async endpoint. */ const STACKED = /part of a stack/; /** Keeps a stuck job well inside an agent's two-minute command timeout. */ @@ -114,7 +139,7 @@ async function gh(args: readonly string[]): Promise { const firstLine = (text: string): string => (text.trim().split(/\r?\n/, 1)[0] ?? "").slice(0, 240); -function graphql(query: string, pr: W.PrContext): Promise { +function graphql(query: string, pr: W.PrContext, cursor: string | null = null): Promise { return ghJson([ "api", "graphql", @@ -126,6 +151,7 @@ function graphql(query: string, pr: W.PrContext): Promise { `repo=${pr.repo}`, "-F", `pr=${pr.number}`, + ...(cursor === null ? [] : ["-f", `after=${cursor}`]), ]); } @@ -166,6 +192,13 @@ function count(value: unknown, path: string): number { ? value : invalid(path, value); } +function prNumber(value: unknown, path: string): W.PrNumber { + try { + return parsePrNumber(value); + } catch { + return invalid(path, value); + } +} function actor(value: unknown, path: string): Actor | null { if (value === null) return null; const object = record(value, path); @@ -214,6 +247,7 @@ export function parsePullRequest( repo.defaultBranchRef === null ? null : text(record(repo.defaultBranchRef, "defaultBranchRef").name, "defaultBranchRef.name"), + stack: pr.stack === null ? null : count(record(pr.stack, "stack").number, "stack.number"), author: actor(pr.author, "author"), reviews: list(record(pr.reviews, "reviews").nodes, "reviews.nodes").map(parseReview), reviewCount: count(record(pr.reviews, "reviews").totalCount, "reviews.totalCount"), @@ -236,6 +270,45 @@ function parseFile(value: unknown, index: number): ChangedFile { }; } +function parseStackEntry(value: unknown, index: number): StackEntry { + const path = `stack.entries[${index}]`; + const object = record(value, path); + const pr = record(object.pullRequest, `${path}.pullRequest`); + return { + position: count(object.position, `${path}.position`), + number: prNumber(pr.number, `${path}.pullRequest.number`), + state: ENTRY_STATES.find((state) => state === pr.state) ?? invalid(`${path}.pullRequest.state`, pr.state), + headRefName: text(pr.headRefName, `${path}.pullRequest.headRefName`), + }; +} + +/** A merged PR carries its merge commit; any other state is reported as GitHub names it. */ +type Outcome = { readonly merged: true; readonly commit: string | null } | { readonly merged: false; readonly state: string }; + +/** How a read-back names what it read, so a single PR and a stack each get their own wording. */ +interface Scope { + /** Bottom first; the last is the PR the merge was requested on. */ + readonly prs: readonly W.PrContext[]; + readonly name: (pr: W.PrContext) => string; + readonly reread: (pr: W.PrContext) => string; + readonly what: string; + readonly every: string; + readonly command: string; +} + +// Entries arrive in pages; positions must run 1..n with no gap, and the target +// must be among them, or the read is incomplete. +function nativeStack(number: number, target: W.PrNumber, entries: readonly StackEntry[]): NativeStack { + const sorted = [...entries].sort((a, b) => a.position - b.position); + const index = sorted.findIndex((entry) => entry.number === target); + const top = sorted[index]; + if (top === undefined || sorted.some((entry, position) => entry.position !== position + 1)) + throw new GitHubError( + `stack #${number} lists positions ${sorted.map((entry) => entry.position).join(", ")}${top === undefined ? ` without #${target}` : ""}` + ); + return { number, below: sorted.slice(0, index), target: top }; +} + interface MergeJob { readonly status: string; readonly uuid: string | null; @@ -323,24 +396,82 @@ export class GhGateGitHub implements GateGitHub { result.code !== 0 && STACKED.test(result.stderr) ? await this.mergeAsync(pr, head, method) : { kind: "failed", detail: `gh pr merge exited ${result.code}: ${firstLine(result.stderr) || "no stderr"}` }; - let state: string; - try { - const after = record(repository(await graphql(OUTCOME_QUERY, pr)).pullRequest, "pullRequest"); - state = text(after.state, "state"); - if (state === "MERGED" && optionalText(after.mergedAt, "mergedAt") !== null) - return { kind: "merged", commit: oid(after.mergeCommit, "mergeCommit") }; - } catch { + return this.settle(attempt, { + prs: [pr], + name: () => "PR", + reread: () => "the PR", + what: "it", + every: "it", + command: "merge-gate", + }); + } + + async mergeStack( + top: W.PrContext, + head: string, + method: MergeMethod, + below: readonly W.PrContext[] + ): Promise { + return this.settle(await this.mergeAsync(top, head, method), { + prs: [...below, top], + name: (pr) => `#${pr.number}`, + reread: (pr) => `#${pr.number}`, + what: "the stack", + every: "every PR", + command: "merge-gate --stack", + }); + } + + // A merge request can report success it did not have, or fail after merging, + // so the verdict comes from reading every PR back. + private async settle(attempt: Attempt, scope: Scope): Promise { + const reads = await Promise.allSettled(scope.prs.map((pr) => this.outcome(pr))); + const unread = reads.findIndex((read) => read.status === "rejected"); + if (unread >= 0) return { kind: "unverified", - detail: `${attempt.detail}, then reading the PR back failed, so it may have merged. Rerun merge-gate without --merge.`, + detail: `${attempt.detail}, then reading ${scope.reread(scope.prs[unread])} back failed, so ${scope.what} may have merged. Rerun ${scope.command} without --merge.`, }; - } - return attempt.kind === "pending" - ? { + const outcomes = reads.map((read) => (read as PromiseFulfilledResult).value); + const top = outcomes[outcomes.length - 1]; + const unmerged = outcomes.flatMap((outcome, index) => + outcome.merged ? [] : [`${scope.name(scope.prs[index])} is ${outcome.state}`] + ); + if (top.merged && unmerged.length === 0) return { kind: "merged", commit: top.commit }; + const detail = `${attempt.detail}; ${unmerged.join(", ")}`; + // merge-async is atomic, so a failed job that left every PR open merged nothing. + return attempt.kind === "failed" && unmerged.length === scope.prs.length + ? { kind: "failed", detail } + : { kind: "unverified", - detail: `${attempt.detail}; PR is ${state}, so it may still merge. Wait for gh pr view to show it settled before rerunning merge-gate --merge.`, - } - : { kind: "failed", detail: `${attempt.detail}; PR is ${state}` }; + detail: `${detail}, so ${scope.what} may still merge. Wait for gh pr view to show ${scope.every} settled before rerunning ${scope.command} --merge.`, + }; + } + + private async outcome(pr: W.PrContext): Promise { + const after = record(repository(await graphql(OUTCOME_QUERY, pr)).pullRequest, "pullRequest"); + const state = text(after.state, "state"); + return state === "MERGED" && optionalText(after.mergedAt, "mergedAt") !== null + ? { merged: true, commit: oid(after.mergeCommit, "mergeCommit") } + : { merged: false, state }; + } + + async stack(pr: W.PrContext): Promise { + const entries: StackEntry[] = []; + let cursor: string | null = null; + for (let page = 0; page < MAX_STACK_PAGES; page += 1) { + const pull = record(repository(await graphql(STACK_QUERY, pr, cursor)).pullRequest, "pullRequest"); + if (pull.stack === null) return null; + const stack = record(pull.stack, "stack"); + const connection = record(stack.entries, "stack.entries"); + entries.push( + ...list(connection.nodes, "stack.entries.nodes").map((entry, index) => parseStackEntry(entry, entries.length + index)) + ); + const pageInfo = record(connection.pageInfo, "stack.entries.pageInfo"); + if (pageInfo.hasNextPage !== true) return nativeStack(count(stack.number, "stack.number"), pr.number, entries); + cursor = text(pageInfo.endCursor, "stack.entries.pageInfo.endCursor"); + } + throw new GitHubError(`stack entries for #${pr.number} exceed ${MAX_STACK_PAGES} pages`); } private async mergeAsync(pr: W.PrContext, head: string, method: MergeMethod): Promise { From 5c783a10fb296dbfb481ff70dabbe9ddf9bfe051 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Wed, 28 Oct 2026 15:14:12 -0300 Subject: [PATCH 2/3] fix(merge-gate): check lower heads after a stack merge, re-read the stack, treat 409 as pending merge-async pins only the top PR's head, so a push to a lower PR after the pre-merge check landed ungated commits and still read back as merged. - The read-back now includes headRefOid. A merged PR whose head differs from its gated head returns the new final code merged-ungated-head (exit 1) and names each PR's gated and merged SHA. The window that remains is documented in the code, shipping.md, opening-a-pr.md and translation.md. The gh-stack v0.1.0 merge API docs say nothing about lower heads. - Just before the PUT, the gate re-reads the stack, in parallel with the head re-check. The stack read now includes baseRefName. A different number, a different ordered list of open entries, a different headRefName, or a base other than the gated one returns stack-changed. - Every PR's merge method is collected. If they differ, the gate returns method-mismatch instead of using the top PR's method for the whole range. - A merge-async 409 means a merge request already exists. Its body is parsed. A different expected_head_sha is not polled and ends merge-unverified, or merged-ungated-head if it landed. Otherwise the gate polls that uuid. A 409 never reports merge-failed. The single-PR fallback shares this path. - verdict.stack.prs is now [{ number, head }]. Tested: bun test orch watch-pr merge-gate check-plan.test.ts (225 pass); merge-gate alone 3 runs (159 pass each); bun run typecheck (clean); bun scripts/check-port.ts (0 findings); read-only dry run merge-gate 2009 --stack --repo nagringa-dev/monorepo (stack read with baseRefName parsed, refused on #2009's failing checks, exit 2). --- .../sync-upstream/references/translation.md | 2 +- skills/poteto-mode/playbooks/opening-a-pr.md | 2 +- skills/poteto-mode/playbooks/shipping.md | 2 +- .../scripts/merge-gate/cli.test.ts | 163 ++++++++++++++++-- skills/poteto-mode/scripts/merge-gate/cli.ts | 58 +++++-- .../scripts/merge-gate/decide.test.ts | 27 ++- .../poteto-mode/scripts/merge-gate/decide.ts | 54 ++++-- .../poteto-mode/scripts/merge-gate/github.ts | 130 +++++++++----- 8 files changed, 346 insertions(+), 92 deletions(-) diff --git a/.claude/skills/sync-upstream/references/translation.md b/.claude/skills/sync-upstream/references/translation.md index 4ec7680..c9b5a92 100644 --- a/.claude/skills/sync-upstream/references/translation.md +++ b/.claude/skills/sync-upstream/references/translation.md @@ -76,7 +76,7 @@ A sync must keep these even when upstream's line differs. - orchestrate and swarm isolation: remote by default for orchestrate workers, and a worktree for any swarm worker that checks out, builds, or runs. - `.claude-plugin/plugin.json` carries no `version`, where upstream's `.cursor-plugin/plugin.json` does. Claude Code then versions a GitHub-hosted install by commit SHA, so every merge to `main` reaches `claude plugin update`. A version string that does not change blocks updates. The `plugin-version` rule in `scripts/check-port.ts` keeps it out. `claude plugin validate` warns about the missing version. That warning is expected. - interrogate adds the blind-spot lenses (`references/blind-spots.md`, Step 3 item 5), a PR number or URL as scope, the lead checking each Act on and Consider finding against source, and the someone-else's-PR flow (`references/review-comments.md`), all folded in from Lucas's retired pr-review-swarm skill. When a playbook runs it as the pre-PR gate, it hands the verdict back instead of ending the turn. The gate's rules live in Opening a PR step 2. It runs no tool-based analyzers: a CodeScene change-set step was tried and removed after an eval showed the reviewers already covered everything it found (PR #2). -- `skills/poteto-mode/scripts/merge-gate/` is port-only. It reuses watch-pr's readiness verdict without editing watch-pr, and adds the policy: allowed repos, human-only paths needing a human approval on the head, and head pinning with `--match-head-commit`. A PR in a GitHub native stack refuses `gh pr merge`, so the gate falls back to the merge-async REST endpoint with the same head SHA. `--stack` gates native stack positions 1 to the target as one unit: each open PR runs the same per-PR decision, with its expected base being the head of the open PR below it instead of trunk, and one merge-async call on the target, pinned to its head, lands the range atomically. Upstream has no native stacks. Every playbook merges only through it, and a refusal is final: human-only changes are the user's to merge. The policy lives outside git, and editing it is on poteto-mode's always-pause list. +- `skills/poteto-mode/scripts/merge-gate/` is port-only. It reuses watch-pr's readiness verdict without editing watch-pr, and adds the policy: allowed repos, human-only paths needing a human approval on the head, and head pinning with `--match-head-commit`. A PR in a GitHub native stack refuses `gh pr merge`, so the gate falls back to the merge-async REST endpoint with the same head SHA. `--stack` gates native stack positions 1 to the target as one unit: each open PR runs the same per-PR decision, with its expected base being the head of the open PR below it instead of trunk, and one merge-async call on the target, pinned to its head, lands the range atomically. merge-async pins only that head, so the gate re-reads every head and the stack just before the call and refuses a merged head it did not gate (`merged-ungated-head`). Upstream has no native stacks. Every playbook merges only through it, and a refusal is final: human-only changes are the user's to merge. The policy lives outside git, and editing it is on poteto-mode's always-pause list. - `principle-never-block-on-the-human` narrows its force-push example to "force-push to a shared branch", matching poteto-mode's Autonomy. Autopilot owners force-push their own branches with a lease on every rebase. - models.md uses only `opus` and `sonnet`. `fable` is dropped for cost: every role that upstream maps to it, and every panel slot, goes to `opus` or `sonnet`. interrogate runs two reviewers (`opus`, `sonnet`), accepting that both share one family and give less independent review than a cross-family pair. - `orch frontier set` walks the PR base chain with `gh` only. Upstream's `--forge` option and its Origin branch are removed, so the command takes no forge. diff --git a/skills/poteto-mode/playbooks/opening-a-pr.md b/skills/poteto-mode/playbooks/opening-a-pr.md index 749661c..7ec2bcb 100644 --- a/skills/poteto-mode/playbooks/opening-a-pr.md +++ b/skills/poteto-mode/playbooks/opening-a-pr.md @@ -33,7 +33,7 @@ After the sections, attach videos or screenshots when they prove a claim. Full S 2. Gate. Run the **interrogate** skill against trunk. Apply each Act on finding that fits the PR's intent without changing a public API or deleting data. An Act on finding you won't fix here is a blocker, unless the lead re-buckets it to Consider or Dismissed with a reason. Commit the fixes after `/simplify` over them, rerun the checks they affect, and push. When a fix changed behavior beyond the lines its finding cited, run the gate once more on the new head and apply its findings the same way. Never run a third round. 3. No blockers: `gh pr create` without `--draft`, or `gh pr ready ` for your own draft that you have not readied before. A PR that went back to draft after you readied it is a hold: report it and stop. In a stack, ready only your own PR. `gh stack submit --auto --open` is for one agent that gated every PR in the stack, followed by `gh pr edit` for each title and body. With blockers: the PR stays or opens as a draft. 4. Merge, when this PR is the whole task rather than a layer mid-stack or mid-phase, and never for a revert PR. Run the **Babysit** playbook in `drive` mode until the watcher reports `READY`, then `scripts/merge-gate/merge-gate --merge`. Never run `gh pr merge` yourself. A final refusal is never overridden that way: the user merges it. - - Final (report the reason and any listed files, and stop): `needs-human-approval`, `repo-not-allowed`, `not-author`, `changes-requested`, `base-not-trunk`, `policy-invalid`, `threads-unverified`, `reviews-unverified`, `files-incomplete`, `draft` (someone drafted it after you readied it, which means hold). A verdict with any final reason is final. A `base-not-trunk` refusal on a PR in a GitHub native stack means it lands with its stack, through Shipping's `--stack`, and only on an explicit request to land. + - Final (report the reason and any listed files, and stop): `needs-human-approval`, `repo-not-allowed`, `not-author`, `changes-requested`, `base-not-trunk`, `policy-invalid`, `threads-unverified`, `reviews-unverified`, `files-incomplete`, `method-mismatch`, `draft` (someone drafted it after you readied it, which means hold), and `merged-ungated-head`. A verdict with any final reason is final. `merged-ungated-head` means the merge landed a head the gate never gated: tell the user at once with each PR's gated and merged SHA from the detail, and merge nothing else until they have reviewed what landed. A `base-not-trunk` refusal on a PR in a GitHub native stack means it lands with its stack, through Shipping's `--stack`, and only on an explicit request to land. - Back to Babysit, then re-gate: `checks-failed`, `threads-unresolved`, and `not-mergeable` when the detail says `DIRTY` or `BEHIND` (rebase first). - Transient (wait for the watcher, rerun once, then report): `checks-pending`, `head-moved`, `github-error`, and any other `not-mergeable`. - `merge-unverified`: run `gh pr view --json state,mergeCommit`. If it merged, continue with the post-merge watch on that commit. If not, a stacked PR's async merge may still be queued, so poll `gh pr view` for a few minutes before you rerun with `--merge`. If it is still open after that, treat it as transient. diff --git a/skills/poteto-mode/playbooks/shipping.md b/skills/poteto-mode/playbooks/shipping.md index ce972f8..e0531a9 100644 --- a/skills/poteto-mode/playbooks/shipping.md +++ b/skills/poteto-mode/playbooks/shipping.md @@ -7,7 +7,7 @@ This is the half after `playbooks/babysit.md`. Run it only on an explicit reques 1. **Verify each PR independently before it lands.** Every PR operation goes through `gh`. Never require Graphite (`gt`). One verifier per PR, each a background subagent with `isolation: "worktree"`, runs the change the way a user would with the matching control skill (such as the `claude-in-chrome` browser tools or the built-in `run` skill), parent against head, and posts `PASS`, `PASS+NOTES`, or `FAIL` on that PR. Only a verdict from an agent that did not write the code counts, because an author checks what it meant to build. Green CI and an approving bot review are not verdicts, because neither runs the change as a user would. 2. **Land only the contiguous verified run rooted at the bottom.** Walk up from the lowest unmerged PR and stop at the first one without `PASS` or `PASS+NOTES`. The PRs below it are the run, and the top one is the ceiling. A verified PR above an unverified one waits, because it cannot land without the code below it. Moving one out of a `gh stack` takes the interactive `gh stack modify`, so leave that to the user. Once its branch neither holds nor needs that code and it targets trunk, it can land on its own after a fresh verdict, because its old one ran with that code beneath it and trunk does not have it. 3. **Re-check that the verdicts still describe the code.** A verdict covers a patch, and a chain rebase moves every PR above it to a new SHA without failing any check. Compare `git patch-id` at the verdict SHA with the current head before trusting an older verdict. When the patches differ only in tests, docs, or lint config, a lane's result stands if what it ran builds the same at both SHAs apart from noise. Noise is a difference that also shows up between two builds of the verdict SHA, or an embedded commit SHA. Report each kind of noise with its files, so a reader can check the call. The change itself still gets fresh checks and a review, because the old verdict never saw it. A lane with no build output, such as a dev server, cannot show this, so rerun it, and re-verify anything else that drifted. -4. **Land bottom-up, one PR at a time.** Always merge the run's lowest unmerged PR. Bring its branch onto the exact trunk tip when needed, with `gh stack rebase --downstack` and `gh stack push` from the bottom branch, and re-run step 3 after the push. Merge with `scripts/merge-gate/merge-gate --merge`, which refuses a PR whose base is not trunk, because one whose base is still its parent's branch would merge into that branch. In a GitHub native stack, land the whole run at once with `scripts/merge-gate/merge-gate --stack --merge` instead. It gates every open PR from position 1 to the ceiling, each against the head of the PR below it, and lands them in one atomic merge-async call. A refusal stops the landing. For `needs-human-approval`, report the PR and the human-only files the gate listed, and leave the merge to the user. Read the PR back (`gh pr view --json state,mergedAt,baseRefName`), because a merge can report success when a merge queue only queued the PR. A landed PR reads merged with trunk as its base. Never arm `--auto`, because without required checks GitHub merges it at once. After a merge lands, GitHub restacks the rest of a `gh stack` server-side. Run `gh stack sync`, confirm the next PR now targets trunk, and let step 3's patch-id rule decide whether its verdict survives. Merge it once its checks pass and its verdict survives. +4. **Land bottom-up, one PR at a time.** Always merge the run's lowest unmerged PR. Bring its branch onto the exact trunk tip when needed, with `gh stack rebase --downstack` and `gh stack push` from the bottom branch, and re-run step 3 after the push. Merge with `scripts/merge-gate/merge-gate --merge`, which refuses a PR whose base is not trunk, because one whose base is still its parent's branch would merge into that branch. In a GitHub native stack, land the whole run at once with `scripts/merge-gate/merge-gate --stack --merge` instead. It gates every open PR from position 1 to the ceiling, each against the head of the PR below it, and lands them in one atomic merge-async call. GitHub pins only the ceiling's head, so the gate re-reads every head and the stack just before that call and checks each merged head after it. A push to a lower PR between that re-read and the merge still lands. The gate then reports `merged-ungated-head`, which is final, so step 5 keeps everyone off the stack while it lands. `stack-changed` means a PR was added, merged, closed, or retargeted after gating: find out why, re-verify any PR it touched, and rerun. `method-mismatch` means the run's PRs call for different merge methods, which one call cannot honor: report it and leave the landing to the user. A refusal stops the landing. For `needs-human-approval`, report the PR and the human-only files the gate listed, and leave the merge to the user. Read the PR back (`gh pr view --json state,mergedAt,baseRefName`), because a merge can report success when a merge queue only queued the PR. A landed PR reads merged with trunk as its base. Never arm `--auto`, because without required checks GitHub merges it at once. After a merge lands, GitHub restacks the rest of a `gh stack` server-side. Run `gh stack sync`, confirm the next PR now targets trunk, and let step 3's patch-id rule decide whether its verdict survives. Merge it once its checks pass and its verdict survives. 5. **Leave the stack alone while it lands.** Step 4's rebase, push, and sync are the only changes to a branch or base, because a merge lands whatever its branch and base hold at that moment, verdict or not. Use `scripts/watch-pr/watch-pr --queued-stack --stack-prs ` only as an event wake under `/loop` in dynamic mode, and after each wake read `gh pr view --json state,mergedAt,mergeStateStatus,statusCheckRollup` for the current PR until `mergedAt` is set. A base that moves to trunk when its parent merges is expected, not damage to repair. If a merge stalls, find out why before you change anything. 6. **Stop at the ceiling.** Extending the run takes new verdicts, so it is a new pass through step 1. diff --git a/skills/poteto-mode/scripts/merge-gate/cli.test.ts b/skills/poteto-mode/scripts/merge-gate/cli.test.ts index a05a154..7bb3177 100644 --- a/skills/poteto-mode/scripts/merge-gate/cli.test.ts +++ b/skills/poteto-mode/scripts/merge-gate/cli.test.ts @@ -46,13 +46,14 @@ interface Scenario extends PrFixture { readonly policy?: string | null; } -const outcome = (state: string, mergeCommit: string | null): FakeResponse => ({ +const outcome = (state: string, mergeCommit: string | null, head = HEAD): FakeResponse => ({ stdout: { data: { repository: { pullRequest: { state, mergedAt: state === "MERGED" ? "2026-09-24T00:00:00Z" : null, + headRefOid: head, mergeCommit: mergeCommit === null ? null : { oid: mergeCommit }, }, }, @@ -130,7 +131,7 @@ function prRules(number: number, pr: PrFixture): FakeRule[] { graphql("PrCommitStatuses"), [pullRequest({ commits: { nodes: [{ commit: { oid: head, statusCheckRollup: { state: checks?.rollup ?? "SUCCESS" } } }] } })], ], - [graphql("MergeGateOutcome"), pr.outcome ?? [outcome("MERGED", MERGE_COMMIT)]], + [graphql("MergeGateOutcome"), pr.outcome ?? [outcome("MERGED", MERGE_COMMIT, head)]], ]; } @@ -138,7 +139,13 @@ function prRules(number: number, pr: PrFixture): FakeRule[] { const prAt = (position: number): number => position + 4; type EntryState = "OPEN" | "CLOSED" | "MERGED"; -const stackPage = (states: readonly EntryState[], { from = 1, cursor = null as string | null } = {}): FakeResponse => ({ +interface StackNode { + readonly number: number; + readonly state: EntryState; + readonly base: string; +} + +const stackRead = (nodes: readonly StackNode[], { from = 1, cursor = null as string | null } = {}): FakeResponse => ({ stdout: { data: { repository: { @@ -147,9 +154,9 @@ const stackPage = (states: readonly EntryState[], { from = 1, cursor = null as s number: 42, entries: { pageInfo: { hasNextPage: cursor !== null, endCursor: cursor }, - nodes: states.map((state, index) => ({ + nodes: nodes.map(({ number, state, base }, index) => ({ position: from + index, - pullRequest: { number: prAt(from + index), state, headRefName: `feature-${prAt(from + index)}` }, + pullRequest: { number, state, headRefName: `feature-${number}`, baseRefName: base }, })), }, }, @@ -158,6 +165,15 @@ const stackPage = (states: readonly EntryState[], { from = 1, cursor = null as s }, }, }); +/** Each entry sits on the one below it, and position 1 on main. */ +const stackPage = (states: readonly EntryState[], { from = 1, cursor = null as string | null } = {}): FakeResponse => + stackRead( + states.map((state, index) => { + const position = from + index; + return { number: prAt(position), state, base: position === 1 ? "main" : `feature-${prAt(position - 1)}` }; + }), + { from, cursor } + ); const NOT_STACKED: FakeResponse = { stdout: { data: { repository: { pullRequest: { stack: null } } } } }; function rules(scenario: Scenario = {}): readonly FakeRule[] { @@ -220,6 +236,12 @@ const asyncPut = [ "merge_action=direct_merge", ]; const asyncPoll = ["api", `repos/acme/app/pulls/7/merge-async/${JOB}`]; +const existingRequest = (head: string): FakeResponse => ({ + stdout: { + status: "pending", + details: { message: "Merge request already exists.", uuid: JOB, merge_method: head === HEAD ? "squash" : "merge", expected_head_sha: head }, + }, +}); describe("merge-gate CLI against a fake gh", () => { it.each<[string, Scenario, readonly string[], number, object, readonly (readonly string[])[]]>([ @@ -413,19 +435,55 @@ describe("merge-gate CLI against a fake gh", () => { [squash, asyncPut], ], [ - "refuses when GitHub rejects the pinned head", + "reports an unverified merge on a 409 without a merge request in its body", { merge: [STACKED], asyncMerge: [{ stderr: "gh: Head branch was modified. Review and try the merge again. (HTTP 409)\n", exit: 1 }], ...open, }, - 2, + 1, refused( - "merge-failed", - "merge-async request failed: gh api repos/acme/app/pulls/7/merge-async exited 1: gh: Head branch was modified. Review and try the merge again. (HTTP 409); PR is OPEN" + "merge-unverified", + `merge-async reported an existing merge request (HTTP 409): gh api repos/acme/app/pulls/7/merge-async printed invalid JSON${UNSETTLED}` ), [squash, asyncPut], ], + [ + "polls the existing merge request a 409 returns when it pins the gated head", + { + merge: [STACKED], + asyncMerge: [{ ...existingRequest(HEAD), stderr: "gh: HTTP 409 (HTTP 409)\n", exit: 1 }], + }, + 0, + { ...subject, decision: "merge", merged: true, mergeCommit: MERGE_COMMIT, reasons: [] }, + [squash, asyncPut, asyncPoll], + ], + [ + "reports an unverified merge when a 409 returns a request for another head", + { + merge: [STACKED], + asyncMerge: [{ ...existingRequest(MOVED), stderr: "gh: HTTP 409 (HTTP 409)\n", exit: 1 }], + ...open, + }, + 1, + refused( + "merge-unverified", + `another merge request, merge-async job ${JOB}, already exists with different options (head ${MOVED}, method merge), so merge-gate did not poll it${UNSETTLED}` + ), + [squash, asyncPut], + ], + [ + "never reports a failure for the existing merge request a 409 returns", + { + merge: [STACKED], + asyncMerge: [{ ...existingRequest(HEAD), stderr: "gh: HTTP 409 (HTTP 409)\n", exit: 1 }], + asyncPolls: [{ stdout: { status: "failed", details: { message: "Merge conflict" } } }], + ...open, + }, + 1, + refused("merge-unverified", `merge-async job ${JOB} failed: Merge conflict${UNSETTLED}`), + [squash, asyncPut, asyncPoll], + ], [ "reports an unverified merge when the merge-async request errors server-side", { merge: [STACKED], asyncMerge: [{ stderr: "gh: Bad Gateway (HTTP 502)\n", exit: 1 }], ...open }, @@ -562,11 +620,13 @@ describe("merge-gate --stack against a fake gh", () => { const match = /^repos\/acme\/app\/pulls\/(\d+)\/files/.exec(argv[1] ?? ""); return match === null ? [] : [Number(match[1])]; }); + const HEADS: Readonly> = { 5: HEAD_5, 6: HEAD_6, 7: HEAD }; + /** `head` null is a refusal before the gate read any PR, so no PR carries a head yet. */ const subject = (prs: readonly number[] | null, head: string | null = HEAD) => ({ repo: "acme/app", pr: 7, head, - stack: prs === null ? null : { number: 42, prs }, + stack: prs === null ? null : { number: 42, prs: prs.map((number) => ({ number, head: head && HEADS[number] })) }, }); const merged = (prs: readonly number[]): Verdict => ({ ...subject(prs), @@ -584,6 +644,7 @@ describe("merge-gate --stack against a fake gh", () => { reasons: [{ code, detail }], }); const ALL = [5, 6, 7]; + const GATED_STACK = "#5 (open, feature-5) on main, #6 (open, feature-6) on feature-5, #7 (open, feature-7) on feature-6"; const UNSETTLED = "so the stack may still merge. Wait for gh pr view to show every PR settled before rerunning merge-gate --stack --merge."; const open = { outcome: [outcome("OPEN", null)] }; @@ -653,7 +714,16 @@ describe("merge-gate --stack against a fake gh", () => { }, { name: "skips a merged lower entry and gates the rest against trunk", - scenario: { stack: [stackPage(["MERGED", "OPEN", "OPEN"])], prs: { 6: { baseRefName: "main" } } }, + scenario: { + stack: [ + stackRead([ + { number: 5, state: "MERGED", base: "main" }, + { number: 6, state: "OPEN", base: "main" }, + { number: 7, state: "OPEN", base: "feature-6" }, + ]), + ], + prs: { 6: { baseRefName: "main" } }, + }, merge: true, exit: 0, verdict: merged([6, 7]), @@ -687,6 +757,77 @@ describe("merge-gate --stack against a fake gh", () => { gated: ALL, calls: [], }, + { + name: "refuses a range whose stack gained a PR below the target after gating", + scenario: { + stack: [ + stackPage(["OPEN", "OPEN", "OPEN"]), + stackRead([ + { number: 5, state: "OPEN", base: "main" }, + { number: 8, state: "OPEN", base: "feature-5" }, + { number: 6, state: "OPEN", base: "feature-8" }, + { number: 7, state: "OPEN", base: "feature-6" }, + ]), + ], + }, + merge: true, + exit: 2, + verdict: refused( + subject(ALL), + "stack-changed", + `stack #42 changed after gating: gated ${GATED_STACK}; now stack #42 lists #5 (open, feature-5) on main, #8 (open, feature-8) on feature-5, #6 (open, feature-6) on feature-8, #7 (open, feature-7) on feature-6` + ), + gated: ALL, + calls: [], + }, + { + name: "refuses a range whose PR was retargeted after gating", + scenario: { + stack: [ + stackPage(["OPEN", "OPEN", "OPEN"]), + stackRead([ + { number: 5, state: "OPEN", base: "main" }, + { number: 6, state: "OPEN", base: "main" }, + { number: 7, state: "OPEN", base: "feature-6" }, + ]), + ], + }, + merge: true, + exit: 2, + verdict: refused( + subject(ALL), + "stack-changed", + `stack #42 changed after gating: gated ${GATED_STACK}; now stack #42 lists #5 (open, feature-5) on main, #6 (open, feature-6) on main, #7 (open, feature-7) on feature-6` + ), + gated: ALL, + calls: [], + }, + { + name: "refuses a merge that landed a lower head other than the one gated", + scenario: { prs: { 5: { outcome: [outcome("MERGED", MERGE_COMMIT, MOVED)] } } }, + merge: true, + exit: 1, + verdict: refused( + subject(ALL), + "merged-ungated-head", + `merge-async job ${JOB} reported merged; #5 merged head ${MOVED}, but merge-gate gated ${HEAD_5}. Commits merge-gate never gated landed in ${MERGE_COMMIT}, so a human must review what merged before anything else lands.` + ), + gated: ALL, + calls: [asyncPut, asyncPoll], + }, + { + name: "refuses a range whose PRs call for different merge methods", + scenario: { prs: { 6: { headRefName: "sync/upstream" } } }, + merge: true, + exit: 2, + verdict: refused( + subject(ALL), + "method-mismatch", + "one call merges the range with one method, but #5 uses squash, #6 uses merge, #7 uses squash" + ), + gated: ALL, + calls: [], + }, { name: "reports an unverified merge when one PR in the range does not read merged", scenario: { prs: { 6: open } }, diff --git a/skills/poteto-mode/scripts/merge-gate/cli.ts b/skills/poteto-mode/scripts/merge-gate/cli.ts index acb146a..0bb2cc5 100644 --- a/skills/poteto-mode/scripts/merge-gate/cli.ts +++ b/skills/poteto-mode/scripts/merge-gate/cli.ts @@ -4,7 +4,7 @@ import { Command, CommanderError, InvalidArgumentError } from "commander"; import { WatcherQueryError } from "../watch-pr/github.ts"; import type * as W from "../watch-pr/types.ts"; import { nonEmpty, parsePrNumber } from "../watch-pr/types.ts"; -import { type Reason, type ReasonCode, decidePlan, plainPlan, planStack } from "./decide.ts"; +import { type Reason, type ReasonCode, decidePlan, plainPlan, planStack, stackChange } from "./decide.ts"; import { type GateGitHub, GhGateGitHub, GitHubError, type MergeResult } from "./github.ts"; import { PolicyError, loadPolicy } from "./policy.ts"; @@ -12,8 +12,14 @@ interface Subject { readonly repo: string | null; readonly pr: number | null; readonly head: string | null; - /** With --stack, the native stack and the PRs gated, bottom first; null without --stack or before the stack is read. */ - readonly stack: { readonly number: number; readonly prs: readonly number[] } | null; + /** + * With --stack, the native stack and its PRs, bottom first, each with the head the gate read (null before it read them); + * null without --stack or before the stack is read. + */ + readonly stack: { + readonly number: number; + readonly prs: readonly { readonly number: number; readonly head: string | null }[]; + } | null; } export type Verdict = Subject & @@ -80,7 +86,7 @@ function target(value: string): { readonly number: W.PrNumber; readonly repo: W. function parseArgs(argv: readonly string[], runtime: Runtime): Request { const program = new Command("merge-gate") .description( - "Decide whether an agent may merge one PR under the merge policy, and with --merge, merge it.\nWith --stack, gate every open PR from the bottom of its GitHub native stack up to this one, and merge them together.\nPrints one JSON verdict. Exit 0: mergeable or merged, 2: refused, 1: internal error." + "Decide whether an agent may merge one PR under the merge policy, and with --merge, merge it.\nWith --stack, gate every open PR from the bottom of its GitHub native stack up to this one, and merge them together.\nPrints one JSON verdict. Exit 0: mergeable or merged, 2: refused, 1: internal error, or a merge that may have landed or landed ungated commits." ) .configureOutput({ writeOut: runtime.stdout, writeErr: runtime.stderr }) .exitOverride() @@ -115,12 +121,18 @@ const refuse = (subject: Subject, reasons: W.NonEmpty): Verdict => ({ const refuseOne = (subject: Subject, code: ReasonCode, detail: string): Verdict => refuse(subject, [{ code, detail }]); +const MERGE_REFUSAL: Record, ReasonCode> = { + failed: "merge-failed", + unverified: "merge-unverified", + ungated: "merged-ungated-head", +}; + /** A merge decision, after the merge when `merge` is given. */ function approve(subject: Subject, merge: MergeResult | null): Verdict { if (merge === null) return { ...subject, decision: "merge", merged: false, mergeCommit: null, reasons: [] }; return merge.kind === "merged" ? { ...subject, decision: "merge", merged: true, mergeCommit: merge.commit, reasons: [] } - : refuseOne(subject, merge.kind === "unverified" ? "merge-unverified" : "merge-failed", merge.detail); + : refuseOne(subject, MERGE_REFUSAL[merge.kind], merge.detail); } async function evaluate(request: Request, runtime: Runtime): Promise { @@ -144,30 +156,39 @@ async function evaluate(request: Request, runtime: Runtime): Promise { const { plan, reasons: blocked } = stack === undefined ? { plan: plainPlan(request.number), reasons: [] } : planStack(stack); if (plan.stack !== null) - subject = { ...subject, stack: { number: plan.stack, prs: [...plan.below, plan.top].map((pr) => pr.number) } }; + subject = { ...subject, stack: { number: plan.stack, prs: [...plan.below, plan.top].map(({ number }) => ({ number, head: null })) } }; const unplanned = nonEmpty(blocked); if (unplanned !== null) return refuse(subject, unplanned); const [top, ...below] = await Promise.all( [plan.top, ...plan.below].map(async ({ number, base }) => ({ facts: await github.facts(at(number)), base })) ); - subject = { ...subject, repo: slug(top.facts.pr), pr: top.facts.pr.number, head: top.facts.head }; + const gated = [...below, top].map(({ facts }) => ({ pr: facts.pr, head: facts.head })); + subject = { + ...subject, + repo: slug(top.facts.pr), + pr: top.facts.pr.number, + head: top.facts.head, + stack: subject.stack && { ...subject.stack, prs: gated.map(({ pr, head }) => ({ number: pr.number, head })) }, + }; const decision = decidePlan(policy, viewer, plan.stack, below, top); if (decision.kind === "refuse") return refuse(subject, decision.reasons); if (!request.merge) return approve(subject, null); - const gated = [...below, top].map(({ facts }) => facts); - const heads = await Promise.all(gated.map((facts) => github.head(facts.pr))); - const moved = gated.findIndex((facts, index) => heads[index] !== facts.head); + // As late as possible before the merge: GitHub pins only the top head, so + // these reads are what keep the lower PRs and the stack's shape as gated. + const [heads, now] = await Promise.all([ + Promise.all(gated.map(({ pr }) => github.head(pr))), + stack === undefined ? undefined : github.stack(top.facts.pr), + ]); + const moved = gated.findIndex(({ head }, index) => heads[index] !== head); if (moved >= 0) { const { pr, head } = gated[moved]; const tag = plan.stack === null ? "" : `#${pr.number}: `; return refuseOne(subject, "head-moved", `${tag}head was ${head}, then ${heads[moved] ?? "unknown"} before merging`); } - return approve( - subject, - plan.stack === null - ? await github.merge(top.facts.pr, top.facts.head, decision.method) - : await github.mergeStack(top.facts.pr, top.facts.head, decision.method, below.map(({ facts }) => facts.pr)) - ); + if (stack === undefined) return approve(subject, await github.merge(top.facts.pr, top.facts.head, decision.method)); + const changed = stackChange(stack, plan, top.facts.defaultBranch, now ?? null); + if (changed !== null) return refuse(subject, [changed]); + return approve(subject, await github.mergeStack(gated, decision.method)); } catch (error) { if (error instanceof PolicyError) return refuseOne(subject, "policy-invalid", error.message); if (error instanceof GitHubError || error instanceof WatcherQueryError) @@ -176,9 +197,12 @@ async function evaluate(request: Request, runtime: Runtime): Promise { } } +/** Not a plain refusal: merge-gate failed, or a merge may have landed or landed ungated commits. */ +const NEEDS_A_HUMAN: ReadonlySet = new Set(["internal-error", "merge-unverified", "merged-ungated-head"]); + function exitCode(verdict: Verdict): number { if (verdict.decision === "merge") return 0; - return verdict.reasons.some((reason) => reason.code === "internal-error" || reason.code === "merge-unverified") ? 1 : 2; + return verdict.reasons.some((reason) => NEEDS_A_HUMAN.has(reason.code)) ? 1 : 2; } export async function main( diff --git a/skills/poteto-mode/scripts/merge-gate/decide.test.ts b/skills/poteto-mode/scripts/merge-gate/decide.test.ts index 3ce25a5..f759665 100644 --- a/skills/poteto-mode/scripts/merge-gate/decide.test.ts +++ b/skills/poteto-mode/scripts/merge-gate/decide.test.ts @@ -392,6 +392,7 @@ const entry = (position: number, state: StackEntry["state"] = "OPEN"): StackEntr number: prAt(position), state, headRefName: `feature-${prAt(position)}`, + baseRefName: position === 1 ? "main" : `feature-${prAt(position - 1)}`, }); const onTop = (position: number): ExpectedBase => ({ kind: "below", number: prAt(position), branch: `feature-${prAt(position)}` }); @@ -438,7 +439,7 @@ describe("planStack", () => { }); describe("decidePlan", () => { - const gated = (position: number, base: ExpectedBase, facts: Partial = {}) => ({ + const gated = (position: number, base: ExpectedBase, facts: Partial = {}, branch = `feature-${prAt(position)}`) => ({ base, facts: { pr: { owner: "acme", repo: "app", number: prAt(position) }, @@ -461,7 +462,7 @@ describe("decidePlan", () => { mergeStateStatus: "CLEAN", reviewDecision: null, headRefOid: HEAD, - headRefName: `feature-${prAt(position)}`, + headRefName: branch, baseRefName: base.kind === "trunk" ? "main" : base.branch, state: "OPEN", mergedAt: null, @@ -482,6 +483,28 @@ describe("decidePlan", () => { top: gated(3, onTop(2)), expected: { kind: "merge", method: "squash" }, }, + { + name: "refuses a stack whose PRs call for different merge methods", + stack: 42, + below: [gated(1, TRUNK), gated(2, onTop(1), {}, "sync/upstream")], + top: gated(3, onTop(2)), + expected: { + kind: "refuse", + reasons: [ + { + code: "method-mismatch", + detail: "one call merges the range with one method, but #5 uses squash, #6 uses merge, #7 uses squash", + }, + ], + }, + }, + { + name: "merges a stack with a merge commit when every PR calls for one", + stack: 42, + below: [gated(1, TRUNK, {}, "sync/a")], + top: gated(2, onTop(1), {}, "sync/b"), + expected: { kind: "merge", method: "merge" }, + }, { name: "tags each refusal with its PR in a stack", stack: 42, diff --git a/skills/poteto-mode/scripts/merge-gate/decide.ts b/skills/poteto-mode/scripts/merge-gate/decide.ts index 7c53a4a..714fdcc 100644 --- a/skills/poteto-mode/scripts/merge-gate/decide.ts +++ b/skills/poteto-mode/scripts/merge-gate/decide.ts @@ -24,6 +24,9 @@ export type ReasonCode = | "github-error" | "merge-failed" | "merge-unverified" + | "merged-ungated-head" + | "stack-changed" + | "method-mismatch" | "internal-error"; export interface Reason { @@ -97,6 +100,7 @@ export interface StackEntry { readonly number: W.PrNumber; readonly state: "OPEN" | "CLOSED" | "MERGED"; readonly headRefName: string; + readonly baseRefName: string; } /** A GitHub native stack read for its PR `target`; `below` holds every entry under it, bottom first. */ @@ -372,7 +376,7 @@ export function planStack(stack: NativeStack): { readonly plan: Plan; readonly r }; } -/** Gates every PR of a plan; it merges only when all pass, by the method the top PR's branch calls for. */ +/** Gates every PR of a plan; it merges only when all pass and agree on the method. */ export function decidePlan( policy: Policy, viewer: string, @@ -380,18 +384,42 @@ export function decidePlan( below: readonly Gated[], top: Gated ): Decision { - const decision = decide(policy, top.facts, viewer, top.base); - const refusals = (facts: PrFacts, verdict: Decision): readonly Reason[] => - verdict.kind === "merge" - ? [] - : verdict.reasons.map((refusal) => - stack === null ? withStackHint(refusal, facts) : { ...refusal, detail: `#${facts.pr.number}: ${refusal.detail}` } - ); - const reasons = nonEmpty([ - ...below.flatMap(({ facts, base }) => refusals(facts, decide(policy, facts, viewer, base))), - ...refusals(top.facts, decision), - ]); - return reasons === null ? decision : { kind: "refuse", reasons }; + const verdicts = [...below, top].map(({ facts, base }) => ({ facts, decision: decide(policy, facts, viewer, base) })); + const reasons = nonEmpty( + verdicts.flatMap(({ facts, decision }) => + decision.kind === "merge" + ? [] + : decision.reasons.map((refusal) => + stack === null ? withStackHint(refusal, facts) : { ...refusal, detail: `#${facts.pr.number}: ${refusal.detail}` } + ) + ) + ); + if (reasons !== null) return { kind: "refuse", reasons }; + const methods = verdicts.flatMap(({ facts, decision }) => + decision.kind === "merge" ? [{ number: facts.pr.number, method: decision.method }] : [] + ); + // One merge-async call lands the whole range with a single method. + if (new Set(methods.map(({ method }) => method)).size === 1) return verdicts[verdicts.length - 1].decision; + const each = methods.map(({ number, method }) => `#${number} uses ${method}`).join(", "); + return { kind: "refuse", reasons: [reason("method-mismatch", `one call merges the range with one method, but ${each}`)] }; +} + +// A PR inserted, merged, closed or retargeted after gating changes what one +// merge-async call lands, so the stack read again just before merging must +// list the gated PRs, open and in order, each on the base it was gated on. +export function stackChange(gated: NativeStack, plan: Plan, trunk: string | null, now: NativeStack | null): Reason | null { + const expected = new Map([...plan.below, plan.top].map(({ number, base }) => [number, base.kind === "trunk" ? trunk : base.branch])); + const describe = (stack: NativeStack, base: (entry: StackEntry) => string | null | undefined): string => + [...stack.below, stack.target] + .filter((entry) => entry.state !== "MERGED") + .map((entry) => `#${entry.number} (${entry.state.toLowerCase()}, ${entry.headRefName}) on ${base(entry) ?? "unknown"}`) + .join(", "); + const planned = describe(gated, (entry) => expected.get(entry.number)); + if (now?.number === gated.number && describe(now, (entry) => entry.baseRefName) === planned) return null; + return reason( + "stack-changed", + `stack #${gated.number} changed after gating: gated ${planned}; now ${now === null ? "the PR is in no stack" : `stack #${now.number} lists ${describe(now, (entry) => entry.baseRefName)}`}` + ); } function withStackHint(refusal: Reason, facts: PrFacts): Reason { diff --git a/skills/poteto-mode/scripts/merge-gate/github.ts b/skills/poteto-mode/scripts/merge-gate/github.ts index 02da64a..cce4832 100644 --- a/skills/poteto-mode/scripts/merge-gate/github.ts +++ b/skills/poteto-mode/scripts/merge-gate/github.ts @@ -18,7 +18,15 @@ import type { export type MergeResult = | { readonly kind: "merged"; readonly commit: string | null } | { readonly kind: "failed"; readonly detail: string } - | { readonly kind: "unverified"; readonly detail: string }; + | { readonly kind: "unverified"; readonly detail: string } + /** A PR merged with a head other than the one gated. */ + | { readonly kind: "ungated"; readonly detail: string }; + +/** A PR and the head merge-gate gated on it. */ +export interface Pinned { + readonly pr: W.PrContext; + readonly head: string; +} export interface GateGitHub { originRepo(): Promise; @@ -28,8 +36,11 @@ export interface GateGitHub { merge(pr: W.PrContext, head: string, method: MergeMethod): Promise; /** The native stack `pr` belongs to, or null when it is in none. */ stack(pr: W.PrContext): Promise; - /** One merge-async call on `top`, which lands every PR below it too; `below` is read back to confirm. */ - mergeStack(top: W.PrContext, head: string, method: MergeMethod, below: readonly W.PrContext[]): Promise; + /** + * One merge-async call on the last PR, pinned to its head, lands every PR below it too. + * GitHub pins only that head, so every PR is read back and its merged head checked against its gated one. + */ + mergeStack(range: readonly Pinned[], method: MergeMethod): Promise; } /** A gh call failed or answered with a shape merge-gate does not recognize. */ @@ -71,6 +82,7 @@ query MergeGateOutcome($owner: String!, $repo: String!, $pr: Int!) { pullRequest(number: $pr) { state mergedAt + headRefOid mergeCommit { oid } } } @@ -84,7 +96,7 @@ query MergeGateStack($owner: String!, $repo: String!, $pr: Int!, $after: String) number entries(first: 100, after: $after) { pageInfo { hasNextPage endCursor } - nodes { position pullRequest { number state headRefName } } + nodes { position pullRequest { number state headRefName baseRefName } } } } } @@ -100,6 +112,8 @@ const MERGE_POLL_MS = 2_000; const MAX_MERGE_POLLS = 30; /** Only a 4xx proves GitHub queued nothing; a 5xx, a timeout or a garbled body may hide a queued job. */ const REJECTED = /\(HTTP 4\d\d\)/; +/** merge-async answers 409 when a merge request for the PR already exists, and returns that request. */ +const CONFLICT = /\(HTTP 409\)/; const FILES_PER_PAGE = 100; /** GitHub's pull request files endpoint stops at 3000 files. */ const MAX_FILE_PAGES = 30; @@ -155,17 +169,22 @@ function graphql(query: string, pr: W.PrContext, cursor: string | null = null): ]); } +const describeCommand = (args: readonly string[]): string => `gh ${args.slice(0, 2).join(" ")}`; + async function ghJson(args: readonly string[]): Promise { const result = await gh(args); - const command = `gh ${args.slice(0, 2).join(" ")}`; if (result.code !== 0) throw new GitHubError( - `${command} exited ${result.code}: ${firstLine(result.stderr) || "no stderr"}` + `${describeCommand(args)} exited ${result.code}: ${firstLine(result.stderr) || "no stderr"}` ); + return body(args, result); +} + +function body(args: readonly string[], result: CommandResult): unknown { try { return JSON.parse(result.stdout); } catch { - throw new GitHubError(`${command} printed invalid JSON`); + throw new GitHubError(`${describeCommand(args)} printed invalid JSON`); } } @@ -279,16 +298,19 @@ function parseStackEntry(value: unknown, index: number): StackEntry { number: prNumber(pr.number, `${path}.pullRequest.number`), state: ENTRY_STATES.find((state) => state === pr.state) ?? invalid(`${path}.pullRequest.state`, pr.state), headRefName: text(pr.headRefName, `${path}.pullRequest.headRefName`), + baseRefName: text(pr.baseRefName, `${path}.pullRequest.baseRefName`), }; } -/** A merged PR carries its merge commit; any other state is reported as GitHub names it. */ -type Outcome = { readonly merged: true; readonly commit: string | null } | { readonly merged: false; readonly state: string }; +/** A merged PR carries its merge commit and the head it merged; any other state is reported as GitHub names it. */ +type Outcome = + | { readonly merged: true; readonly commit: string | null; readonly head: string } + | { readonly merged: false; readonly state: string }; /** How a read-back names what it read, so a single PR and a stack each get their own wording. */ interface Scope { /** Bottom first; the last is the PR the merge was requested on. */ - readonly prs: readonly W.PrContext[]; + readonly prs: readonly Pinned[]; readonly name: (pr: W.PrContext) => string; readonly reread: (pr: W.PrContext) => string; readonly what: string; @@ -313,15 +335,22 @@ interface MergeJob { readonly status: string; readonly uuid: string | null; readonly message: string; + /** The head the job will merge and its method; GitHub reports them on a pending job. */ + readonly head: string | null; + readonly method: string | null; } function parseMergeJob(value: unknown): MergeJob { const job = record(value, "merge-async"); const details = job.details === undefined ? {} : record(job.details, "merge-async.details"); + const detail = (field: string): string | null => + details[field] === undefined ? null : text(details[field], `merge-async.details.${field}`); return { status: text(job.status, "merge-async.status"), - uuid: details.uuid === undefined ? null : text(details.uuid, "merge-async.details.uuid"), + uuid: detail("uuid"), message: typeof details.message === "string" ? details.message : JSON.stringify(details), + head: detail("expected_head_sha"), + method: detail("merge_method"), }; } @@ -397,7 +426,7 @@ export class GhGateGitHub implements GateGitHub { ? await this.mergeAsync(pr, head, method) : { kind: "failed", detail: `gh pr merge exited ${result.code}: ${firstLine(result.stderr) || "no stderr"}` }; return this.settle(attempt, { - prs: [pr], + prs: [{ pr, head }], name: () => "PR", reread: () => "the PR", what: "it", @@ -406,16 +435,12 @@ export class GhGateGitHub implements GateGitHub { }); } - async mergeStack( - top: W.PrContext, - head: string, - method: MergeMethod, - below: readonly W.PrContext[] - ): Promise { - return this.settle(await this.mergeAsync(top, head, method), { - prs: [...below, top], - name: (pr) => `#${pr.number}`, - reread: (pr) => `#${pr.number}`, + async mergeStack(range: readonly Pinned[], method: MergeMethod): Promise { + const top = range[range.length - 1]; + return this.settle(await this.mergeAsync(top.pr, top.head, method), { + prs: range, + name: ({ number }) => `#${number}`, + reread: ({ number }) => `#${number}`, what: "the stack", every: "every PR", command: "merge-gate --stack", @@ -423,19 +448,32 @@ export class GhGateGitHub implements GateGitHub { } // A merge request can report success it did not have, or fail after merging, - // so the verdict comes from reading every PR back. + // so the verdict comes from reading every PR back. merge-async pins only the + // top head, so a lower PR pushed after the pre-merge check lands unpinned; + // the read-back catches it after the fact but cannot stop it. private async settle(attempt: Attempt, scope: Scope): Promise { - const reads = await Promise.allSettled(scope.prs.map((pr) => this.outcome(pr))); + const reads = await Promise.allSettled(scope.prs.map(({ pr }) => this.outcome(pr))); const unread = reads.findIndex((read) => read.status === "rejected"); if (unread >= 0) return { kind: "unverified", - detail: `${attempt.detail}, then reading ${scope.reread(scope.prs[unread])} back failed, so ${scope.what} may have merged. Rerun ${scope.command} without --merge.`, + detail: `${attempt.detail}, then reading ${scope.reread(scope.prs[unread].pr)} back failed, so ${scope.what} may have merged. Rerun ${scope.command} without --merge.`, }; const outcomes = reads.map((read) => (read as PromiseFulfilledResult).value); const top = outcomes[outcomes.length - 1]; + const ungated = outcomes.flatMap((outcome, index) => { + const { pr, head } = scope.prs[index]; + return outcome.merged && outcome.head !== head + ? [`${scope.name(pr)} merged head ${outcome.head}, but merge-gate gated ${head}`] + : []; + }); + if (ungated.length > 0) + return { + kind: "ungated", + detail: `${attempt.detail}; ${ungated.join("; ")}. Commits merge-gate never gated landed${top.merged && top.commit !== null ? ` in ${top.commit}` : ""}, so a human must review what merged before anything else lands.`, + }; const unmerged = outcomes.flatMap((outcome, index) => - outcome.merged ? [] : [`${scope.name(scope.prs[index])} is ${outcome.state}`] + outcome.merged ? [] : [`${scope.name(scope.prs[index].pr)} is ${outcome.state}`] ); if (top.merged && unmerged.length === 0) return { kind: "merged", commit: top.commit }; const detail = `${attempt.detail}; ${unmerged.join(", ")}`; @@ -452,7 +490,7 @@ export class GhGateGitHub implements GateGitHub { const after = record(repository(await graphql(OUTCOME_QUERY, pr)).pullRequest, "pullRequest"); const state = text(after.state, "state"); return state === "MERGED" && optionalText(after.mergedAt, "mergedAt") !== null - ? { merged: true, commit: oid(after.mergeCommit, "mergeCommit") } + ? { merged: true, commit: oid(after.mergeCommit, "mergeCommit"), head: text(after.headRefOid, "headRefOid") } : { merged: false, state }; } @@ -476,34 +514,34 @@ export class GhGateGitHub implements GateGitHub { private async mergeAsync(pr: W.PrContext, head: string, method: MergeMethod): Promise { const endpoint = `repos/${pr.owner}/${pr.repo}/pulls/${pr.number}/merge-async`; + const put = ["api", endpoint, "-X", "PUT", "-f", `sha=${head}`, "-f", `merge_method=${method}`, "-f", "merge_action=direct_merge"]; + const result = await gh(put); + const existing = result.code !== 0 && CONFLICT.test(result.stderr); + if (result.code !== 0 && !existing) { + const detail = `merge-async request failed: ${describeCommand(put)} exited ${result.code}: ${firstLine(result.stderr) || "no stderr"}`; + return { kind: REJECTED.test(result.stderr) ? "failed" : "pending", detail }; + } let job: MergeJob; try { - job = parseMergeJob( - await ghJson([ - "api", - endpoint, - "-X", - "PUT", - "-f", - `sha=${head}`, - "-f", - `merge_method=${method}`, - "-f", - "merge_action=direct_merge", - ]) - ); + job = parseMergeJob(body(put, result)); } catch (error) { if (!(error instanceof GitHubError)) throw error; - return { - kind: REJECTED.test(error.message) ? "failed" : "pending", - detail: `merge-async request failed: ${error.message}`, - }; + const request = existing ? "merge-async reported an existing merge request (HTTP 409)" : "merge-async request failed"; + return { kind: "pending", detail: `${request}: ${error.message}` }; } const { uuid } = job; const name = uuid === null ? "merge-async" : `merge-async job ${uuid}`; + // An existing request for another head would merge what merge-gate did not gate. + if (existing && job.head !== null && job.head !== head) + return { + kind: "pending", + detail: `another merge request, ${name}, already exists with different options (head ${job.head}, method ${job.method ?? "unknown"}), so merge-gate did not poll it`, + }; for (let poll = 0; ; poll += 1) { if (job.status === "merged") return { kind: "pending", detail: `${name} reported merged` }; - if (job.status === "failed") return { kind: "failed", detail: `${name} failed: ${firstLine(job.message)}` }; + // A 409 returns a job merge-gate did not submit, so its failure is never reported as this merge's. + if (job.status === "failed") + return { kind: existing ? "pending" : "failed", detail: `${name} failed: ${firstLine(job.message)}` }; if (job.status !== "pending") return { kind: "pending", detail: `${name} reported unknown status ${job.status}` }; if (uuid === null) return { kind: "pending", detail: "merge-async queued a job without a uuid" }; From ff0bcde7882a49bb393167606a5395d3beed3e22 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Wed, 28 Oct 2026 15:16:20 -0300 Subject: [PATCH 3/3] fix(merge-gate): an existing merge request with another method is not polled A 409 that pins the gated head but a different merge_method would land the PR with a method the policy did not choose; report merge-unverified instead of polling it, like a request for another head. Tested: bun test merge-gate 160 pass; typecheck clean; check-port 0. --- .../poteto-mode/scripts/merge-gate/cli.test.ts | 18 ++++++++++++++++-- .../poteto-mode/scripts/merge-gate/github.ts | 7 +++++-- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/skills/poteto-mode/scripts/merge-gate/cli.test.ts b/skills/poteto-mode/scripts/merge-gate/cli.test.ts index 7bb3177..0b8eea2 100644 --- a/skills/poteto-mode/scripts/merge-gate/cli.test.ts +++ b/skills/poteto-mode/scripts/merge-gate/cli.test.ts @@ -236,10 +236,10 @@ const asyncPut = [ "merge_action=direct_merge", ]; const asyncPoll = ["api", `repos/acme/app/pulls/7/merge-async/${JOB}`]; -const existingRequest = (head: string): FakeResponse => ({ +const existingRequest = (head: string, method = head === HEAD ? "squash" : "merge"): FakeResponse => ({ stdout: { status: "pending", - details: { message: "Merge request already exists.", uuid: JOB, merge_method: head === HEAD ? "squash" : "merge", expected_head_sha: head }, + details: { message: "Merge request already exists.", uuid: JOB, merge_method: method, expected_head_sha: head }, }, }); @@ -472,6 +472,20 @@ describe("merge-gate CLI against a fake gh", () => { ), [squash, asyncPut], ], + [ + "reports an unverified merge when a 409 returns a request for the gated head with another method", + { + merge: [STACKED], + asyncMerge: [{ ...existingRequest(HEAD, "merge"), stderr: "gh: HTTP 409 (HTTP 409)\n", exit: 1 }], + ...open, + }, + 1, + refused( + "merge-unverified", + `another merge request, merge-async job ${JOB}, already exists with different options (head ${HEAD}, method merge), so merge-gate did not poll it${UNSETTLED}` + ), + [squash, asyncPut], + ], [ "never reports a failure for the existing merge request a 409 returns", { diff --git a/skills/poteto-mode/scripts/merge-gate/github.ts b/skills/poteto-mode/scripts/merge-gate/github.ts index cce4832..9cf0a21 100644 --- a/skills/poteto-mode/scripts/merge-gate/github.ts +++ b/skills/poteto-mode/scripts/merge-gate/github.ts @@ -531,8 +531,11 @@ export class GhGateGitHub implements GateGitHub { } const { uuid } = job; const name = uuid === null ? "merge-async" : `merge-async job ${uuid}`; - // An existing request for another head would merge what merge-gate did not gate. - if (existing && job.head !== null && job.head !== head) + // An existing request for another head would merge what merge-gate did not gate, and one with + // another method would land it in a way the policy did not choose. + const foreignHead = job.head !== null && job.head !== head; + const foreignMethod = job.method !== null && job.method !== method; + if (existing && (foreignHead || foreignMethod)) return { kind: "pending", detail: `another merge request, ${name}, already exists with different options (head ${job.head}, method ${job.method ?? "unknown"}), so merge-gate did not poll it`,