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; }, },