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

@@ -335,6 +335,14 @@ export class GiteaClient {
     return GiteaClient.mapPull(await this.request<any>(`${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<string[]> {
+    const c = await this.request<any>(`${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

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<string, unknown>;
+  const done = (await merge(REVIEWED.slice(0, 12).toUpperCase())) as Record<string, unknown>;
   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<string, unknown>;
-  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<string, unknown>;
+  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<string, unknown>;
+    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 () => {

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<string, unknown> = { 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;
       },
     },