novox/mesh-catalog
6
0
Fork 0
Code Issues Pull Requests 5 Actions Packages Projects Releases Wiki Activity

Merge pull request 'gitea: a merge names the head it was reviewed at (issue 381)' (#193) from fix/381-a-merge-names-the-head-it-was-reviewed-at into main

Browse Source
This commit was merged in pull request #193.
This commit is contained in:
mesh-admin
2026-10-10 23:56:30 +00:00
parent fa6360c1d3 c87a4421b9
commit d856892ced
3 changed files with 206 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
modules/gitea/client.ts
+11
View File
@@ -335,6 +335,17 @@ 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) => {
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.
*
* `limit` is what is asked for, and a merge that changed more says so rather than being read
modules/gitea/test/guard.test.ts
+127 -10
View File
@@ -21,7 +21,18 @@ const state = {
statuses: [] as { context: string; state: string }[],
rules: [] as any[],
lastCommit: "2026-01-01T00:00:00Z",
// 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: "",
// 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,
status_check_contexts: ["mesh/merge-gate", "mesh/repo-check"], block_admin_merge_override: true });
@@ -30,6 +41,12 @@ 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;
state.mergeCommit = "feedface00000000000000000000000000000000";
state.parents = null;
state.commitsFail = 0;
});
const json = (res: ServerResponse, status: number, body: unknown): void => {
@@ -59,13 +76,22 @@ 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" });
return json(res, 200, { number: 7, title: "x", state: state.merged ? "closed" : "open", merged: state.merged,
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}/commits/abc1234/status`) {
return json(res, 200, { sha: "abc1234", state: "success", statuses: state.statuses });
if (method === "GET" && path === `${repo}/git/commits/${state.mergeCommit}`) {
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 });
}
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 +159,111 @@ 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 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"), /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, 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 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, 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, REVIEWED);
assert.equal(done.read_back, undefined);
});
// 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";
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");
});
// 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<string, unknown>;
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<string, unknown>;
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<string, unknown>;
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<string, unknown>;
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;
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 () => {
@@ -172,10 +289,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
+68 -6
View File
@@ -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";
@@ -262,14 +265,18 @@ 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" },
method: { type: "string", description: "merge | rebase | squash (default merge)" },
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 (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) => {
@@ -282,9 +289,29 @@ 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`);
}
// 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. 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
// branch nothing guards would merge past every check (novox/hq ADR 0266).
@@ -301,8 +328,43 @@ 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);
return { merged: true, number, method, deleted_branch: deleteBranch, merge_commit_sha: merged.merge_commit_sha };
// 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<string, unknown> = { merged: true, number, method, head: pull.head_sha, deleted_branch: deleteBranch };
let landed: string | undefined;
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 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 (${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)`);
}
if (landed !== undefined) answer.landed = landed;
return answer;
},
},