gitea: a merge names the head it was reviewed at (issue 381)

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

@@ -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<void>((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<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);
+});
+
+// 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<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/);
 });
 
 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(), []);
 });

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<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)`;
+        }
+        return answer;
       },
     },