diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index 4a930d12a..ebe6e972a 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -28,7 +28,9 @@ authenticated `gh` CLI. Use Left/Right arrows or Home/End to switch detail tabs. - **GitHub actions** — submit feedback, request changes, close a PR, or approve it and enable auto-merge with a merge commit. Each action needs two clicks to confirm and - is also recorded in local board history. + is also recorded in local board history. Approval is pinned to the board's commit + SHA, and auto-merge requires that same head commit. If the head changes, refresh + and review the new commit before approving again. Only one GitHub action can run per PR at a time. - **Reset board** — after a second click to confirm, clear every local AI review, queued review, manual move, recorded action, and history entry, then reload open PRs. diff --git a/.github/extensions/open-pr-board/lib/decisions.mjs b/.github/extensions/open-pr-board/lib/decisions.mjs index dab2202d1..6bbae16fc 100644 --- a/.github/extensions/open-pr-board/lib/decisions.mjs +++ b/.github/extensions/open-pr-board/lib/decisions.mjs @@ -21,7 +21,7 @@ export async function applyDecisionOnGitHub({ store, number, kind, comment, refr let result; if (kind === "approve") { - result = await github.approveAndEnableAutoMerge(store.repo, number, { body }); + result = await github.approveAndEnableAutoMerge(store.repo, number, { body, headRefOid: item.headRefOid }); } else if (kind === "close") { result = await github.closePullRequest(store.repo, number, { body }); } else { diff --git a/.github/extensions/open-pr-board/lib/github.mjs b/.github/extensions/open-pr-board/lib/github.mjs index 72f955b45..1ed8a533a 100644 --- a/.github/extensions/open-pr-board/lib/github.mjs +++ b/.github/extensions/open-pr-board/lib/github.mjs @@ -66,11 +66,18 @@ export async function closePullRequest(repo, number, { body } = {}, runner = run } } -export async function approveAndEnableAutoMerge(repo, number, { body } = {}, runner = runGh) { - await submitPullRequestReview(repo, number, { kind: "approve", body }, runner); +export async function approveAndEnableAutoMerge(repo, number, { body, headRefOid } = {}, runner = runGh) { + if (!headRefOid) throw new Error("The board has no commit SHA for this pull request. Refresh before approving."); + const current = JSON.parse(await runner(["api", `repos/${repo}/pulls/${number}`])); + if (current.state !== "open" || current.head?.sha !== headRefOid) { + throw new Error("The pull request changed or is no longer open. Refresh and review it again before approving."); + } + await runner(["api", `repos/${repo}/pulls/${number}/reviews`, "--method", "POST", "--input", "-"], { + input: JSON.stringify({ event: "APPROVE", commit_id: headRefOid, body: String(body ?? "").trim() }), + }); try { - await runner(["pr", "merge", String(number), "-R", repo, "--auto", "--merge"]); + await runner(["pr", "merge", String(number), "-R", repo, "--auto", "--merge", "--match-head-commit", headRefOid]); } catch (error) { return { status: "approval-submitted", diff --git a/eng/open-pr-board.test.mjs b/eng/open-pr-board.test.mjs index b9a51a5c5..b6d5e4f8e 100644 --- a/eng/open-pr-board.test.mjs +++ b/eng/open-pr-board.test.mjs @@ -6,6 +6,7 @@ import vm from "node:vm"; import { test } from "node:test"; import { BoardStore } from "../.github/extensions/open-pr-board/lib/state.mjs"; import { applyDecisionOnGitHub } from "../.github/extensions/open-pr-board/lib/decisions.mjs"; +import { approveAndEnableAutoMerge } from "../.github/extensions/open-pr-board/lib/github.mjs"; import { REVIEW_FIELDS_SCHEMA } from "../.github/extensions/open-pr-board/lib/review-prompt.mjs"; const pr = (headRefOid = "first") => ({ @@ -27,6 +28,65 @@ function deferred() { return { promise, resolve, reject }; } +test("approval passes the board's commit SHA to the GitHub helper", async (t) => { + const store = await createStore(t); + let options; + await applyDecisionOnGitHub({ + store, number: 1, kind: "approve", comment: "Reviewed.", refresh: async () => {}, + github: { + approveAndEnableAutoMerge: async (repo, number, input) => { + options = input; + return { status: "auto-merge-enabled" }; + }, + }, + }); + assert.deepEqual(options, { body: "Reviewed.", headRefOid: "first" }); +}); + +test("approval and auto-merge are pinned to the reviewed commit", async () => { + const headRefOid = "a".repeat(40); + const calls = []; + const result = await approveAndEnableAutoMerge("example/repo", 1, { body: "Reviewed.", headRefOid }, + async (args, options) => { + calls.push({ args, options }); + if (calls.length === 1) return JSON.stringify({ state: "open", head: { sha: headRefOid } }); + if (args[1] === "view") return JSON.stringify({ state: "OPEN", autoMergeRequest: {} }); + return ""; + }); + assert.equal(result.status, "auto-merge-enabled"); + assert.deepEqual(calls[1].args, ["api", "repos/example/repo/pulls/1/reviews", "--method", "POST", "--input", "-"]); + assert.deepEqual(JSON.parse(calls[1].options.input), { event: "APPROVE", commit_id: headRefOid, body: "Reviewed." }); + assert.deepEqual(calls[2].args, ["pr", "merge", "1", "-R", "example/repo", "--auto", "--merge", "--match-head-commit", headRefOid]); +}); + +test("approval refuses a missing SHA, changed head, or closed pull request without writing", async () => { + let calls = 0; + const runner = async () => { calls++; throw new Error("must not run"); }; + await assert.rejects(approveAndEnableAutoMerge("example/repo", 1, {}, runner), /no commit SHA/); + assert.equal(calls, 0); + for (const current of [{ state: "open", head: { sha: "second" } }, { state: "closed", head: { sha: "first" } }]) { + calls = 0; + await assert.rejects(approveAndEnableAutoMerge("example/repo", 1, { headRefOid: "first" }, async () => { + calls++; + return JSON.stringify(current); + }), /Refresh and review/); + assert.equal(calls, 1); + } +}); + +test("a head change after approval reports partial success instead of retrying the merge", async () => { + let calls = 0; + const result = await approveAndEnableAutoMerge("example/repo", 1, { headRefOid: "first" }, async () => { + calls++; + if (calls === 1) return JSON.stringify({ state: "open", head: { sha: "first" } }); + if (calls === 3) throw new Error("Head commit changed"); + return ""; + }); + assert.equal(calls, 3); + assert.equal(result.status, "approval-submitted"); + assert.match(result.warning, /Head commit changed/); +}); + test("review schema requires a nonempty queue token and a one-sentence rationale", () => { assert.ok(REVIEW_FIELDS_SCHEMA.required.includes("queueId")); assert.equal(REVIEW_FIELDS_SCHEMA.properties.queueId.minLength, 1);