From c32eb6237d38a7b8b23a5e82a58ceb8c2f07fb7c Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 01:28:52 +0200 Subject: [PATCH 1/3] gitea: a merge names the head it was reviewed at (issue 381) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mesh-controller#197 was reviewed at 065cfa0d, force-pushed to 3ced96f at 12:14 UTC, checked green there at 12:22, and merged at 3ced96f at 12:26. The merge tool kept its promise that only a checked head is merged, but it took no argument for the head the review approved, so it merged whatever the head was and a commit nobody reviewed landed. gitea_merge_pull_request now takes `head`, the commit the caller reviewed (a sha, or at least its first 7 characters), and refuses with nothing sent when it is missing, is not a commit, or is not the pull request's head — naming the head it has now. The forge is handed the same head as head_commit_id, so a push between the read and the merge is the forge's to refuse. The answer reads back what was merged and says so loudly if the forge merged another head anyway. `head` is required, not optional: an optional guard is the shape that let this happen. A caller that leaves it out is told what to name. --- modules/gitea/test/guard.test.ts | 75 +++++++++++++++++++++++++++----- modules/gitea/tools/index.ts | 37 ++++++++++++++-- 2 files changed, 98 insertions(+), 14 deletions(-) diff --git a/modules/gitea/test/guard.test.ts b/modules/gitea/test/guard.test.ts index 423c83c1..c3e5aca0 100644 --- a/modules/gitea/test/guard.test.ts +++ b/modules/gitea/test/guard.test.ts @@ -21,7 +21,12 @@ const state = { statuses: [] as { context: string; state: string }[], rules: [] as any[], lastCommit: "2026-01-01T00:00:00Z", + // The pull request's head, and the head the forge reports once merged (the same, unless a test says otherwise). + head: "", + landed: "", + merged: false, }; +const REVIEWED = "abc1234def5678901234567890abcdef12345678"; const protectedMain = () => ({ rule_name: "main", enable_push: false, enable_status_check: true, status_check_contexts: ["mesh/merge-gate", "mesh/repo-check"], block_admin_merge_override: true }); @@ -30,6 +35,9 @@ beforeEach(() => { state.asked = []; state.statuses = [{ context: "mesh/merge-gate", state: "success" }, { context: "mesh/repo-check", state: "success" }]; state.rules = [protectedMain()]; + state.head = REVIEWED; + state.landed = ""; + state.merged = false; }); const json = (res: ServerResponse, status: number, body: unknown): void => { @@ -59,13 +67,18 @@ const server = createServer(async (req, res) => { return json(res, 200, [{ commit: { committer: { date: state.lastCommit } } }]); } if (method === "GET" && path === `${repo}/pulls/7`) { - return json(res, 200, { number: 7, title: "x", state: "open", merged: false, head: { ref: "feat/x", sha: "abc1234" }, - base: { ref: "main" }, html_url: "http://fake/7" }); + const sha = state.merged && state.landed ? state.landed : state.head; + return json(res, 200, { number: 7, title: "x", state: state.merged ? "closed" : "open", merged: state.merged, + head: { ref: "feat/x", sha }, base: { ref: "main" }, html_url: "http://fake/7", + merge_commit_sha: state.merged ? "feedface00000000000000000000000000000000" : null }); } - if (method === "GET" && path === `${repo}/commits/abc1234/status`) { - return json(res, 200, { sha: "abc1234", state: "success", statuses: state.statuses }); + if (method === "GET" && path === `${repo}/commits/${state.head}/status`) { + return json(res, 200, { sha: state.head, state: "success", statuses: state.statuses }); + } + if (method === "POST" && path === `${repo}/pulls/7/merge`) { + state.merged = true; + return json(res, 200, null); } - if (method === "POST" && path === `${repo}/pulls/7/merge`) return json(res, 200, null); return json(res, method === "GET" ? 200 : 201, {}); }); await new Promise((r) => server.listen(0, "127.0.0.1", () => r())); @@ -133,20 +146,62 @@ test("gitea_branch_protection_set only strengthens", async () => { }); test("gitea_merge_pull_request merges only a head whose required statuses all succeeded, and only that head", async () => { - const merge = () => tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7 }); + const merge = () => tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, + head: REVIEWED }); state.statuses = [{ context: "mesh/merge-gate", state: "success" }, { context: "mesh/repo-check", state: "failure" }]; await assert.rejects(merge(), /not merged: .*mesh\/repo-check \(failure\)/); state.statuses = [{ context: "mesh/merge-gate", state: "success" }]; await assert.rejects(merge(), /mesh\/repo-check \(missing\)/); await assert.rejects(tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, - method: "manually-merged" }), /not a merge method/); + head: REVIEWED, method: "manually-merged" }), /not a merge method/); assert.deepEqual(writes(), []); // The operator said yes and the checks passed: the flow that stays. state.statuses = [{ context: "mesh/merge-gate", state: "success" }, { context: "mesh/repo-check", state: "success" }]; await merge(); const sent = state.sent.find((s) => s.path.endsWith("/pulls/7/merge"))!; assert.equal(sent.body.force_merge, false); - assert.equal(sent.body.head_commit_id, "abc1234"); + assert.equal(sent.body.head_commit_id, REVIEWED); +}); + + +// A merge lands only the head that was reviewed (novox/hq issue 381): mesh-controller#197 was reviewed at one head, +// force-pushed, checked green at the new one, and merged there, because the tool merged whatever the head was. +test("gitea_merge_pull_request merges only the head it is named, and refuses one that moved after the review", async () => { + const merge = (head?: string) => tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, + ...(head === undefined ? {} : { head }) }); + + // No head named: there is no review to hold the merge to. + await assert.rejects(merge(), /name the head you reviewed/); + await assert.rejects(merge(" "), /name the head you reviewed/); + await assert.rejects(merge("main"), /is not a commit/); + await assert.rejects(merge("abc12"), /is not a commit/); // fewer than 7 characters names too many commits + + // The head moved after the review, and its checks passed where it is now: the case of mesh-controller#197. + state.head = "3ced96fc6f8ad6cc7b93698912cb08a71635158a"; + await assert.rejects(merge(REVIEWED), /its head is 3ced96fc6f8a, not the abc1234def5678901234567890abcdef12345678 you named/); + await assert.rejects(merge(REVIEWED.slice(0, 7)), /not the abc1234 you named/); + assert.deepEqual(writes(), [], "a refused merge sends nothing"); + + // Named as reviewed, by its first 7 characters, in either case: the forge is handed the whole head. + state.head = REVIEWED; + const done = (await merge(REVIEWED.slice(0, 7).toUpperCase())) as Record; + const sent = state.sent.find((s) => s.path.endsWith("/pulls/7/merge"))!; + assert.equal(sent.body.head_commit_id, REVIEWED); + assert.equal(done.head, REVIEWED); + assert.equal(done.landed, undefined); + assert.equal(done.not_the_head_named, undefined); +}); + +// The forge is handed the head, and the answer reads back what it merged: if it merged another one anyway, the +// answer says so rather than reporting the merge as the one asked for. +test("gitea_merge_pull_request says so when the forge merged a head other than the one it was handed", async () => { + state.landed = "3ced96fc6f8ad6cc7b93698912cb08a71635158a"; + const done = (await tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, + head: REVIEWED })) as Record; + assert.equal(done.merged, true); + assert.equal(done.head, REVIEWED); + assert.equal(done.landed, "3ced96fc6f8ad6cc7b93698912cb08a71635158a"); + assert.match(String(done.not_the_head_named), /merged 3ced96fc6f8a, not the abc1234def56/); }); test("gitea_delete_repo deletes only the tools' own user's repository, unguarded, untouched for 30 days, named in full", async () => { @@ -172,10 +227,10 @@ test("gitea_api sends the path it judged, and refuses moving a pull request's ba test("gitea_merge_pull_request merges only into a branch whose protection requires the checks", async () => { state.rules = []; - await assert.rejects(tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7 }), + await assert.rejects(tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, head: REVIEWED }), /no protection rule requiring the checks guards/); state.rules = [{ rule_name: "main", enable_push: false, enable_status_check: false, status_check_contexts: [] }]; - await assert.rejects(tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7 }), + await assert.rejects(tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, head: REVIEWED }), /no protection rule requiring the checks guards/); assert.deepEqual(writes(), []); }); diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index 1d0f7880..0ab92a3b 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -262,13 +262,16 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { }, { name: "gitea_merge_pull_request", - description: "Merge a pull request whose head has every status its base branch's protection requires as a success " + - "(mesh/merge-gate, mesh/repo-check), optionally deleting the source branch afterwards. Refused otherwise — no " + - "tool merges past a check — and only the head that was checked is merged.", + description: "Merge a pull request at the head you name — the commit you reviewed — when that is still its head " + + "and every status its base branch's protection requires succeeded there (mesh/merge-gate, mesh/repo-check), " + + "optionally deleting the source branch afterwards. Refused otherwise, with nothing merged: no tool merges past " + + "a check, and none merges a head that moved after it was reviewed.", input: { owner: { type: "string", description: "the repository owner" }, repo: { type: "string", description: "the repository name" }, number: { type: "number", description: "the PR number" }, + head: { type: "string", description: "the commit you reviewed, as its sha or at least its first 7 characters: " + + "refused unless it is the pull request's head" }, method: { type: "string", description: "merge | rebase | squash (default merge)" }, delete_branch: { type: "boolean", description: "delete the source branch after merge (default true)" }, }, @@ -282,9 +285,27 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { // **Only a head whose required statuses all succeeded** (novox/hq ADR 0266): read here rather than left // to the forge, whose administrator — the account this tool's token is minted for — may merge past a // rule that lets it. A rule that cannot be read is no rule passed. + // **Only the head that was reviewed** (novox/hq issue 381): mesh-controller#197 was reviewed at 065cfa0d, + // force-pushed to 3ced96f, checked green there, and merged at 3ced96f, because this tool merged whatever the + // head was and nothing told it which head the review had approved. The caller names it; a head that moved + // since is refused, and the forge is handed the same head, so a push between this read and the merge is + // refused by the forge too. + const named = args.head === undefined ? "" : String(args.head).trim().toLowerCase(); + if (!named) { + throw new Refused(`${owner}/${repo}#${number} is not merged: name the head you reviewed (head), so that only ` + + `that commit can land (novox/hq issue 381). Nothing was merged`); + } + if (!isCommit(named)) { + throw new Refused(`${owner}/${repo}#${number} is not merged: head ${named} is not a commit — a sha, or at least ` + + `its first 7 characters. Nothing was merged`); + } const pull = await gitea.getPullRequest(owner, repo, number); if (pull.merged) throw new Refused(`${owner}/${repo}#${number} is already merged`); if (!pull.head_sha || !pull.base) throw new Refused(`${owner}/${repo}#${number} names no head commit or base branch`); + if (!pull.head_sha.toLowerCase().startsWith(named)) { + throw new Refused(`${owner}/${repo}#${number} is not merged: its head is ${pull.head_sha.slice(0, 12)}, not the ` + + `${named} you named — it moved after that was reviewed. Review the head it has now, then name it. Nothing was merged`); + } const rule = await gitea.branchProtection(owner, repo, pull.base); // Only into a branch whose protection requires the checks: a pull request whose base was moved to a // branch nothing guards would merge past every check (novox/hq ADR 0266). @@ -302,7 +323,15 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { // The merge commit only exists now; answered so the caller can follow what is built from it. // pull.merged is the events entrypoint's to announce (index.ts), once, within its poll. const merged = await gitea.getPullRequest(owner, repo, number); - return { merged: true, number, method, deleted_branch: deleteBranch, merge_commit_sha: merged.merge_commit_sha }; + const answer: Record = { merged: true, number, method, head: pull.head_sha, + deleted_branch: deleteBranch, merge_commit_sha: merged.merge_commit_sha }; + // What landed, read back: the forge was handed the head, and this says so if it merged another one anyway. + if (merged.head_sha && merged.head_sha !== pull.head_sha) { + answer.landed = merged.head_sha; + answer.not_the_head_named = `the forge merged ${merged.head_sha.slice(0, 12)}, not the ${pull.head_sha.slice(0, 12)} ` + + `it was handed: a commit nobody reviewed may have landed. Read it before anything is built from it (novox/hq issue 381)`; + } + return answer; }, }, From 9ef810d9c3b52b136c4227133c0485436fdaabe3 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 01:33:17 +0200 Subject: [PATCH 2/3] gitea: twelve characters name a head, and what landed is read from the merge commit (review of issue 381) The review of #193 approved it with five nits, all taken. Seven characters of a sha can be matched by a commit made to match them in seconds, so an author who force-pushes could have a review merge a commit nobody read. A head is now named by at least 12 characters, up to 64 for a sha-256 repository. What landed was read from the forge's record of the pull request, which is the very thing a race leaves behind. It is now read from the merge commit: its second parent for a merge, the commit itself for a fast-forward. A squash or a rebase makes new commits that name no head, and the answer says it cannot be read back instead of staying silent. A merge of another head than the one handed over was reported as a field beside merged: true, which a caller reading only that would miss. It is now an error that says the merge happened and what landed. The refusal no longer asserts that the head moved after the review, which it cannot know; it says the head is not the one named. --- modules/gitea/client.ts | 8 ++++ modules/gitea/test/guard.test.ts | 66 ++++++++++++++++++++++---------- modules/gitea/tools/index.ts | 43 ++++++++++++++++----- 3 files changed, 86 insertions(+), 31 deletions(-) diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index 06a87020..4924a184 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -335,6 +335,14 @@ export class GiteaClient { return GiteaClient.mapPull(await this.request(`${repoPath(owner, repo)}/pulls/${index}`)); } + /** A commit's parents, by sha, in order: what a merge commit says it joined (novox/hq issue 381). An answer + * that names no parents is no answer, never an empty list read as a root commit. */ + async commitParents(owner: string, repo: string, sha: string): Promise { + const c = await this.request(`${repoPath(owner, repo)}/git/commits/${encodeURIComponent(sha)}`); + if (!Array.isArray(c?.parents)) throw new Error(`the forge named no parents of ${sha}`); + return c.parents.map((p: any) => String(p?.sha ?? "")); + } + /** The files a merged pull request changed, as paths from the repository's root. * * `limit` is what is asked for, and a merge that changed more says so rather than being read diff --git a/modules/gitea/test/guard.test.ts b/modules/gitea/test/guard.test.ts index c3e5aca0..978ca435 100644 --- a/modules/gitea/test/guard.test.ts +++ b/modules/gitea/test/guard.test.ts @@ -21,10 +21,13 @@ const state = { statuses: [] as { context: string; state: string }[], rules: [] as any[], lastCommit: "2026-01-01T00:00:00Z", - // The pull request's head, and the head the forge reports once merged (the same, unless a test says otherwise). + // The pull request's head; the head the merge commit joined (the same, unless a test says otherwise) — the forge's + // record of the pull request keeps saying `head` either way, as it would in the race issue 381 is about; and the + // merge commit's own sha. head: "", landed: "", merged: false, + mergeCommit: "", }; const REVIEWED = "abc1234def5678901234567890abcdef12345678"; const protectedMain = () => ({ rule_name: "main", enable_push: false, enable_status_check: true, @@ -38,6 +41,7 @@ beforeEach(() => { state.head = REVIEWED; state.landed = ""; state.merged = false; + state.mergeCommit = "feedface00000000000000000000000000000000"; }); const json = (res: ServerResponse, status: number, body: unknown): void => { @@ -67,10 +71,13 @@ const server = createServer(async (req, res) => { return json(res, 200, [{ commit: { committer: { date: state.lastCommit } } }]); } if (method === "GET" && path === `${repo}/pulls/7`) { - const sha = state.merged && state.landed ? state.landed : state.head; return json(res, 200, { number: 7, title: "x", state: state.merged ? "closed" : "open", merged: state.merged, - head: { ref: "feat/x", sha }, base: { ref: "main" }, html_url: "http://fake/7", - merge_commit_sha: state.merged ? "feedface00000000000000000000000000000000" : null }); + head: { ref: "feat/x", sha: state.head }, base: { ref: "main" }, html_url: "http://fake/7", + merge_commit_sha: state.merged ? state.mergeCommit : null }); + } + if (method === "GET" && path === `${repo}/git/commits/${state.mergeCommit}`) { + return json(res, 200, { sha: state.mergeCommit, + parents: [{ sha: "8a53532d89b20f0fe4fd0fbc814f2e86b61cbc6b" }, { sha: state.landed || state.head }] }); } if (method === "GET" && path === `${repo}/commits/${state.head}/status`) { return json(res, 200, { sha: state.head, state: "success", statuses: state.statuses }); @@ -166,42 +173,59 @@ test("gitea_merge_pull_request merges only a head whose required statuses all su // A merge lands only the head that was reviewed (novox/hq issue 381): mesh-controller#197 was reviewed at one head, // force-pushed, checked green at the new one, and merged there, because the tool merged whatever the head was. -test("gitea_merge_pull_request merges only the head it is named, and refuses one that moved after the review", async () => { +test("gitea_merge_pull_request merges only the head it is named, and refuses one that is not the head", async () => { const merge = (head?: string) => tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, ...(head === undefined ? {} : { head }) }); // No head named: there is no review to hold the merge to. await assert.rejects(merge(), /name the head you reviewed/); await assert.rejects(merge(" "), /name the head you reviewed/); - await assert.rejects(merge("main"), /is not a commit/); - await assert.rejects(merge("abc12"), /is not a commit/); // fewer than 7 characters names too many commits + await assert.rejects(merge("main"), /does not name one commit/); + // Seven characters can be matched by a commit made to match them: twelve are asked for. + await assert.rejects(merge(REVIEWED.slice(0, 7)), /does not name one commit — give its sha, or at least its first 12/); + await assert.rejects(merge(REVIEWED.slice(0, 11)), /does not name one commit/); // The head moved after the review, and its checks passed where it is now: the case of mesh-controller#197. state.head = "3ced96fc6f8ad6cc7b93698912cb08a71635158a"; await assert.rejects(merge(REVIEWED), /its head is 3ced96fc6f8a, not the abc1234def5678901234567890abcdef12345678 you named/); - await assert.rejects(merge(REVIEWED.slice(0, 7)), /not the abc1234 you named/); + await assert.rejects(merge(REVIEWED.slice(0, 12)), /not the abc1234def56 you named\. If you reviewed an earlier head/); assert.deepEqual(writes(), [], "a refused merge sends nothing"); - // Named as reviewed, by its first 7 characters, in either case: the forge is handed the whole head. + // Named as reviewed, by its first 12 characters, in either case: the forge is handed the whole head, and the merge + // commit's second parent says that head landed. state.head = REVIEWED; - const done = (await merge(REVIEWED.slice(0, 7).toUpperCase())) as Record; + const done = (await merge(REVIEWED.slice(0, 12).toUpperCase())) as Record; const sent = state.sent.find((s) => s.path.endsWith("/pulls/7/merge"))!; assert.equal(sent.body.head_commit_id, REVIEWED); assert.equal(done.head, REVIEWED); - assert.equal(done.landed, undefined); - assert.equal(done.not_the_head_named, undefined); + assert.equal(done.landed, REVIEWED); + assert.equal(done.read_back, undefined); }); -// The forge is handed the head, and the answer reads back what it merged: if it merged another one anyway, the -// answer says so rather than reporting the merge as the one asked for. -test("gitea_merge_pull_request says so when the forge merged a head other than the one it was handed", async () => { +// The forge is handed the head, and what landed is read from the merge commit, not from the forge's record of the +// pull request (which here still says the head it was handed). Another head landing is an error that says the merge +// happened, never a field beside merged: true. +test("gitea_merge_pull_request is loud when the forge merged a head other than the one it was handed", async () => { state.landed = "3ced96fc6f8ad6cc7b93698912cb08a71635158a"; - const done = (await tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, - head: REVIEWED })) as Record; - assert.equal(done.merged, true); - assert.equal(done.head, REVIEWED); - assert.equal(done.landed, "3ced96fc6f8ad6cc7b93698912cb08a71635158a"); - assert.match(String(done.not_the_head_named), /merged 3ced96fc6f8a, not the abc1234def56/); + await assert.rejects(tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, + head: REVIEWED }), /WAS merged \(feedface0000.*\), but the forge merged 3ced96fc6f8a, not the abc1234def56 it was handed/); + assert.equal(state.merged, true, "the merge happened: the error is what says so"); +}); + +test("gitea_merge_pull_request reads back a fast-forward, and says it cannot read back a squash or a rebase", async () => { + state.mergeCommit = REVIEWED; // a fast-forward lands the head itself + const ff = (await tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, + head: REVIEWED, method: "fast-forward-only" })) as Record; + assert.equal(ff.landed, REVIEWED); + + for (const method of ["squash", "rebase", "rebase-merge"]) { + state.merged = false; + state.mergeCommit = "feedface00000000000000000000000000000000"; + const done = (await tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, + head: REVIEWED, method })) as Record; + assert.equal(done.landed, undefined, method); + assert.match(String(done.read_back), new RegExp(`a ${method} makes new commits that name no head`)); + } }); test("gitea_delete_repo deletes only the tools' own user's repository, unguarded, untouched for 30 days, named in full", async () => { diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index 0ab92a3b..0b8a30f2 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -18,6 +18,9 @@ import { ruleCovers, } from "../guard.js"; +/** A head named for a merge: at least 12 hex characters of its sha (novox/hq issue 381), up to a sha-256 commit's 64. */ +const REVIEWED_HEAD = /^[0-9a-f]{12,64}$/; + /** The forge's container, where its repositories and git are: the note is written there (novox/hq ADR 0239). */ const forgeContainer = process.env.MESH_GITEA_CONTAINER || "gitea"; @@ -270,7 +273,7 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { owner: { type: "string", description: "the repository owner" }, repo: { type: "string", description: "the repository name" }, number: { type: "number", description: "the PR number" }, - head: { type: "string", description: "the commit you reviewed, as its sha or at least its first 7 characters: " + + head: { type: "string", description: "the commit you reviewed, as its sha or at least its first 12 characters: " + "refused unless it is the pull request's head" }, method: { type: "string", description: "merge | rebase | squash (default merge)" }, delete_branch: { type: "boolean", description: "delete the source branch after merge (default true)" }, @@ -295,16 +298,18 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { throw new Refused(`${owner}/${repo}#${number} is not merged: name the head you reviewed (head), so that only ` + `that commit can land (novox/hq issue 381). Nothing was merged`); } - if (!isCommit(named)) { - throw new Refused(`${owner}/${repo}#${number} is not merged: head ${named} is not a commit — a sha, or at least ` + - `its first 7 characters. Nothing was merged`); + // Twelve characters, not seven: an author who force-pushes can make a commit whose first seven match the + // reviewed one in seconds, and the review would then merge it. Twelve cannot be matched that way. + if (!REVIEWED_HEAD.test(named)) { + throw new Refused(`${owner}/${repo}#${number} is not merged: head ${named} does not name one commit — give its ` + + `sha, or at least its first 12 characters. Nothing was merged`); } const pull = await gitea.getPullRequest(owner, repo, number); if (pull.merged) throw new Refused(`${owner}/${repo}#${number} is already merged`); if (!pull.head_sha || !pull.base) throw new Refused(`${owner}/${repo}#${number} names no head commit or base branch`); if (!pull.head_sha.toLowerCase().startsWith(named)) { throw new Refused(`${owner}/${repo}#${number} is not merged: its head is ${pull.head_sha.slice(0, 12)}, not the ` + - `${named} you named — it moved after that was reviewed. Review the head it has now, then name it. Nothing was merged`); + `${named} you named. If you reviewed an earlier head, review the one it has now, then name it. Nothing was merged`); } const rule = await gitea.branchProtection(owner, repo, pull.base); // Only into a branch whose protection requires the checks: a pull request whose base was moved to a @@ -325,12 +330,30 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { const merged = await gitea.getPullRequest(owner, repo, number); const answer: Record = { merged: true, number, method, head: pull.head_sha, deleted_branch: deleteBranch, merge_commit_sha: merged.merge_commit_sha }; - // What landed, read back: the forge was handed the head, and this says so if it merged another one anyway. - if (merged.head_sha && merged.head_sha !== pull.head_sha) { - answer.landed = merged.head_sha; - answer.not_the_head_named = `the forge merged ${merged.head_sha.slice(0, 12)}, not the ${pull.head_sha.slice(0, 12)} ` + - `it was handed: a commit nobody reviewed may have landed. Read it before anything is built from it (novox/hq issue 381)`; + // What landed, read back from the commit the merge made rather than from the forge's record of the pull + // request, which is the very thing a race would leave behind. A merge commit's second parent is the head it + // joined; a fast-forward lands the head itself. A squash or a rebase makes new commits that name no head, + // and the answer says it cannot be read back rather than staying silent. + let landed: string | undefined; + if (!merged.merge_commit_sha) { + answer.read_back = "the forge named no merge commit, so what landed cannot be read back"; + } else if (method === "merge") { + const parents = await gitea.commitParents(owner, repo, merged.merge_commit_sha); + if (parents.length === 2) landed = parents[1]; + else answer.read_back = `the merge commit has ${parents.length} parent(s), not two: what landed cannot be read back`; + } else if (method === "fast-forward-only") { + landed = merged.merge_commit_sha; + } else { + answer.read_back = `a ${method} makes new commits that name no head: what landed cannot be read back`; } + // Loud, not a field beside merged: true that a caller reading only that would miss. The merge has happened and + // cannot be taken back by this tool; what must not happen is that it reads as the merge that was asked for. + if (landed !== undefined && landed !== pull.head_sha) { + throw new Error(`${owner}/${repo}#${number} WAS merged (${merged.merge_commit_sha}), but the forge merged ` + + `${landed.slice(0, 12)}, not the ${pull.head_sha.slice(0, 12)} it was handed and you reviewed: a commit nobody ` + + `reviewed has landed. Read it before anything is built from it (novox/hq issue 381)`); + } + if (landed !== undefined) answer.landed = landed; return answer; }, }, From c87a4421b9c711d65b161be12e329715d5b9c765 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 01:35:43 +0200 Subject: [PATCH 3/3] gitea: a read-back that fails after the merge says the merge happened (review of issue 381) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second review found the gap it was asked to look for: everything after the forge's merge could throw — a forge 5xx, a timeout, a commit named without parents — and the caller got a bare error that never said the merge had happened. From the merge on, a read-back that fails or cannot be done is said beside merged: true, in read_back; only a proven other head is an error, and that error says the merge happened. A parent named without its sha is a failed read-back, never an empty head said to have landed. A fast-forward's commit is taken as the head only when the forge names it so. That it does is assumed, not seen on this forge, so another sha is said in read_back rather than raised as another head having landed. The method's description lists all five methods and says which two can be read back. The fast-forward test now gives the merge commit a different second parent, so reading it as a merge would fail there. --- modules/gitea/client.ts | 5 ++- modules/gitea/test/guard.test.ts | 42 ++++++++++++++++++++++++-- modules/gitea/tools/index.ts | 52 +++++++++++++++++++------------- 3 files changed, 75 insertions(+), 24 deletions(-) diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index 4924a184..eac04eb6 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -340,7 +340,10 @@ export class GiteaClient { async commitParents(owner: string, repo: string, sha: string): Promise { const c = await this.request(`${repoPath(owner, repo)}/git/commits/${encodeURIComponent(sha)}`); if (!Array.isArray(c?.parents)) throw new Error(`the forge named no parents of ${sha}`); - return c.parents.map((p: any) => String(p?.sha ?? "")); + return c.parents.map((p: any) => { + if (typeof p?.sha !== "string" || !p.sha) throw new Error(`the forge named a parent of ${sha} without its sha`); + return p.sha; + }); } /** The files a merged pull request changed, as paths from the repository's root. diff --git a/modules/gitea/test/guard.test.ts b/modules/gitea/test/guard.test.ts index 978ca435..92aa711b 100644 --- a/modules/gitea/test/guard.test.ts +++ b/modules/gitea/test/guard.test.ts @@ -28,6 +28,9 @@ const state = { landed: "", merged: false, mergeCommit: "", + // When set, the merge commit's parents as the forge reports them, or the status it answers them with. + parents: null as string[] | null, + commitsFail: 0, }; const REVIEWED = "abc1234def5678901234567890abcdef12345678"; const protectedMain = () => ({ rule_name: "main", enable_push: false, enable_status_check: true, @@ -42,6 +45,8 @@ beforeEach(() => { state.landed = ""; state.merged = false; state.mergeCommit = "feedface00000000000000000000000000000000"; + state.parents = null; + state.commitsFail = 0; }); const json = (res: ServerResponse, status: number, body: unknown): void => { @@ -76,8 +81,9 @@ const server = createServer(async (req, res) => { merge_commit_sha: state.merged ? state.mergeCommit : null }); } if (method === "GET" && path === `${repo}/git/commits/${state.mergeCommit}`) { - return json(res, 200, { sha: state.mergeCommit, - parents: [{ sha: "8a53532d89b20f0fe4fd0fbc814f2e86b61cbc6b" }, { sha: state.landed || state.head }] }); + if (state.commitsFail) return json(res, state.commitsFail, { message: "the forge is not well" }); + const parents = state.parents ?? ["8a53532d89b20f0fe4fd0fbc814f2e86b61cbc6b", state.landed || state.head]; + return json(res, 200, { sha: state.mergeCommit, parents: parents.map((sha) => ({ sha })) }); } if (method === "GET" && path === `${repo}/commits/${state.head}/status`) { return json(res, 200, { sha: state.head, state: "success", statuses: state.statuses }); @@ -212,11 +218,43 @@ test("gitea_merge_pull_request is loud when the forge merged a head other than t assert.equal(state.merged, true, "the merge happened: the error is what says so"); }); +// The merge has happened once the forge says so: a read-back that fails after it must not read as nothing merged. +test("gitea_merge_pull_request says the merge happened when reading back what landed fails", async () => { + state.commitsFail = 500; + const done = (await tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, + head: REVIEWED })) as Record; + assert.equal(state.merged, true); + assert.equal(done.merged, true); + assert.equal(done.landed, undefined); + assert.match(String(done.read_back), /the merge happened, but reading back what landed failed/); + + // A parent the forge names without its sha is a failed read-back, never an empty head said to have landed. + state.merged = false; + state.commitsFail = 0; + state.parents = ["8a53532d89b20f0fe4fd0fbc814f2e86b61cbc6b", ""]; + const blank = (await tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, + head: REVIEWED })) as Record; + assert.equal(blank.merged, true); + assert.match(String(blank.read_back), /without its sha/); +}); + test("gitea_merge_pull_request reads back a fast-forward, and says it cannot read back a squash or a rebase", async () => { state.mergeCommit = REVIEWED; // a fast-forward lands the head itself + // Its parents name another commit second, so reading a fast-forward as a merge's second parent would fail here. + state.parents = ["8a53532d89b20f0fe4fd0fbc814f2e86b61cbc6b"]; const ff = (await tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, head: REVIEWED, method: "fast-forward-only" })) as Record; assert.equal(ff.landed, REVIEWED); + assert.equal(ff.read_back, undefined); + + // A fast-forward whose commit the forge names otherwise is said, not raised: that it names the head is assumed. + state.merged = false; + state.mergeCommit = "feedface00000000000000000000000000000000"; + const odd = (await tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, + head: REVIEWED, method: "fast-forward-only" })) as Record; + assert.equal(odd.merged, true); + assert.equal(odd.landed, undefined); + assert.match(String(odd.read_back), /as this fast-forward's commit, not the head/); for (const method of ["squash", "rebase", "rebase-merge"]) { state.merged = false; diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index 0b8a30f2..3f0638af 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -275,7 +275,8 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { number: { type: "number", description: "the PR number" }, head: { type: "string", description: "the commit you reviewed, as its sha or at least its first 12 characters: " + "refused unless it is the pull request's head" }, - method: { type: "string", description: "merge | rebase | squash (default merge)" }, + method: { type: "string", description: "merge (default) | fast-forward-only | squash | rebase | rebase-merge. " + + "Only merge and fast-forward-only can be read back to prove the head you named is what landed" }, delete_branch: { type: "boolean", description: "delete the source branch after merge (default true)" }, }, run: async (args) => { @@ -327,29 +328,38 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { await gitea.mergePullRequest(owner, repo, number, method, deleteBranch, pull.head_sha); // The merge commit only exists now; answered so the caller can follow what is built from it. // pull.merged is the events entrypoint's to announce (index.ts), once, within its poll. - const merged = await gitea.getPullRequest(owner, repo, number); - const answer: Record = { merged: true, number, method, head: pull.head_sha, - deleted_branch: deleteBranch, merge_commit_sha: merged.merge_commit_sha }; - // What landed, read back from the commit the merge made rather than from the forge's record of the pull - // request, which is the very thing a race would leave behind. A merge commit's second parent is the head it - // joined; a fast-forward lands the head itself. A squash or a rebase makes new commits that name no head, - // and the answer says it cannot be read back rather than staying silent. + // From here the merge has happened. Nothing below may read as if it had not: a read-back that fails or cannot + // be done is said beside merged: true, and only a proven other head is an error — one that says the merge happened. + const answer: Record = { merged: true, number, method, head: pull.head_sha, deleted_branch: deleteBranch }; let landed: string | undefined; - if (!merged.merge_commit_sha) { - answer.read_back = "the forge named no merge commit, so what landed cannot be read back"; - } else if (method === "merge") { - const parents = await gitea.commitParents(owner, repo, merged.merge_commit_sha); - if (parents.length === 2) landed = parents[1]; - else answer.read_back = `the merge commit has ${parents.length} parent(s), not two: what landed cannot be read back`; - } else if (method === "fast-forward-only") { - landed = merged.merge_commit_sha; - } else { - answer.read_back = `a ${method} makes new commits that name no head: what landed cannot be read back`; + try { + const merged = await gitea.getPullRequest(owner, repo, number); + answer.merge_commit_sha = merged.merge_commit_sha; + // What landed, read back from the commit the merge made rather than from the forge's record of the pull + // request, which is the very thing a race would leave behind. A merge commit's second parent is the head it + // joined; a fast-forward lands the head itself. A squash or a rebase makes new commits that name no head. + if (!merged.merge_commit_sha) { + answer.read_back = "the forge named no merge commit, so what landed cannot be read back"; + } else if (method === "merge") { + const parents = await gitea.commitParents(owner, repo, merged.merge_commit_sha); + if (parents.length === 2) landed = parents[1]; + else answer.read_back = `the merge commit has ${parents.length} parent(s), not two: what landed cannot be read back`; + } else if (method === "fast-forward-only") { + // That the forge names the head itself as a fast-forward's merge commit is assumed, not seen on this forge: + // a different sha is said, not raised as another head having landed. + if (merged.merge_commit_sha === pull.head_sha) landed = merged.merge_commit_sha; + else answer.read_back = `the forge named ${merged.merge_commit_sha.slice(0, 12)} as this fast-forward's commit, ` + + `not the head: that the head landed could not be confirmed`; + } else { + answer.read_back = `a ${method} makes new commits that name no head: what landed cannot be read back`; + } + } catch (err) { + answer.read_back = `the merge happened, but reading back what landed failed: ${(err as Error).message}`; } - // Loud, not a field beside merged: true that a caller reading only that would miss. The merge has happened and - // cannot be taken back by this tool; what must not happen is that it reads as the merge that was asked for. + // Loud, not a field beside merged: true that a caller reading only that would miss. The merge cannot be taken + // back by this tool; what must not happen is that it reads as the merge that was asked for. if (landed !== undefined && landed !== pull.head_sha) { - throw new Error(`${owner}/${repo}#${number} WAS merged (${merged.merge_commit_sha}), but the forge merged ` + + throw new Error(`${owner}/${repo}#${number} WAS merged (${answer.merge_commit_sha}), but the forge merged ` + `${landed.slice(0, 12)}, not the ${pull.head_sha.slice(0, 12)} it was handed and you reviewed: a commit nobody ` + `reviewed has landed. Read it before anything is built from it (novox/hq issue 381)`); }