From f0fbe193fa3c36afe633588996f25838b13ad513 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 26 Oct 2026 17:59:34 +0100 Subject: [PATCH 1/4] gitea: mesh-tools administers a user's delivered repository and puts main's rule right, permissions read as the account sees them; knowledge-base.progress: minimal answers leave items to the tree, failed calls warn before parse faults (issues 1945, 1947) --- modules/gitea/client.ts | 48 ++++++++++-- modules/gitea/test/tools-account.test.ts | 31 +++++++- modules/gitea/writes.ts | 77 +++++++++++++------ .../cmd/mesh-knowledge/progress.go | 15 ++++ .../cmd/mesh-knowledge/progress_test.go | 11 +++ 5 files changed, 151 insertions(+), 31 deletions(-) diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index a36e113f..f63a4456 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -6,7 +6,7 @@ import { createHash } from "node:crypto"; import { readFileSync } from "node:fs"; import { ConfiguredToken, MintedToken, type TokenSource } from "./token.js"; -import { MAIN_BRANCH, mainRuleBody, protectionBody, type BranchProtection, type ProtectionWanted } from "./protection.js"; +import { MAIN_BRANCH, mainMergers, mainRuleBody, protectionBody, type BranchProtection, type ProtectionWanted } from "./protection.js"; import { Refused, checkSentPath, ruleCovers, pathSegment, pathSegments } from "./guard.js"; import { isCommit } from "./checks.js"; @@ -1047,12 +1047,19 @@ export class GiteaAdmin { return true; } - /** What the forge itself says an account may do on a repository: none, read, write, admin or owner. */ + /** What the forge gives an account on a repository, effective: none, read, write, admin or owner. It is the repository as + * that account sees it (the site administrator acts as the account with `sudo`), so access through an organisation's team + * counts. The collaborator endpoint is not used: it names only a collaborator's own permission, and says "read" of an + * account that writes through a team (novox/hq issue 1945). */ async repositoryPermission(owner: string, repo: string, user: string): Promise { - const path = `${repoPath(owner, repo)}/collaborators/${encodeURIComponent(user)}/permission`; + const path = `${repoPath(owner, repo)}?sudo=${encodeURIComponent(user)}`; const res = await this.request(path); + if (res.status === 404) return "none"; if (res.status !== 200) GiteaAdmin.fail(path, res); - return String(res.body?.permission ?? "none"); + const p = res.body?.permissions; + if (p?.admin === true) return res.body?.owner?.login === user ? "owner" : "admin"; + if (p?.push === true) return "write"; + return p?.pull === true ? "read" : "none"; } /** Make an account a collaborator of a repository that reads only. The forge asks a repository's administrator for @@ -1067,7 +1074,38 @@ export class GiteaAdmin { await this.addCollaborator(owner, repo, user, "write"); } - private async addCollaborator(owner: string, repo: string, user: string, permission: "read" | "write"): Promise { + /** Make an account an administrator collaborator of a repository (writes.ts: a repository of a user that builds a + * registered module, where the tools' account must also put main's rule right). */ + async addAdminCollaborator(owner: string, repo: string, user: string): Promise { + await this.addCollaborator(owner, repo, user, "admin"); + } + + /** Put main's rule right in one repository: no push, the merge whitelist (the tools' account first), no status required, + * and no administrator override. A rule the operator already set that is stronger is kept, never made weaker (ADR 0266): + * a whitelist that holds the tools' account and only logins the wanted one names, more approvals, a push already refused. + * Answers the rule as the forge holds it after, read back. With the administrator's login. */ + async ensureMainRule(owner: string, repo: string, mergers: string[] = mainMergers()): Promise<{ created: boolean; rule: BranchProtection }> { + const rules = await this.branchProtections(owner, repo); + const own = rules.find((r) => (r.rule_name ?? r.branch_name) === MAIN_BRANCH); + const kept = own?.enable_merge_whitelist === true ? (own.merge_whitelist_usernames ?? []) : []; + const whitelist = kept.length > 0 && kept.includes(mergers[0]) && kept.every((u) => mergers.includes(u)) ? kept : mergers; + const body = { ...mainRuleBody(mergers), merge_whitelist_usernames: whitelist, block_admin_merge_override: true }; + const path = `${repoPath(owner, repo)}/branch_protections`; + if (own) { + const res = await this.request(`${path}/${MAIN_BRANCH}`, { method: "PATCH", body: JSON.stringify(body) }); + if (res.status !== 200) GiteaAdmin.fail(`${path}/${MAIN_BRANCH}`, res); + } else { + const covering = rules.filter((r) => ruleCovers(String(r.rule_name ?? r.branch_name ?? ""), MAIN_BRANCH)); + const approvals = Math.max(0, ...covering.map((r) => Number(r.required_approvals ?? 0))); + const res = await this.request(path, { method: "POST", body: JSON.stringify({ rule_name: MAIN_BRANCH, ...(approvals > 0 ? { required_approvals: approvals } : {}), ...body }) }); + if (res.status !== 201) GiteaAdmin.fail(path, res); + } + const after = (await this.branchProtections(owner, repo)).find((r) => (r.rule_name ?? r.branch_name) === MAIN_BRANCH); + if (!after) throw new Error(`main's rule of ${owner}/${repo} was written, but the forge does not list it`); + return { created: !own, rule: after }; + } + + private async addCollaborator(owner: string, repo: string, user: string, permission: "read" | "write" | "admin"): Promise { const path = `${repoPath(owner, repo)}/collaborators/${encodeURIComponent(user)}`; const res = await this.request(path, { method: "PUT", body: JSON.stringify({ permission }) }); if (res.status === 204 || res.status === 200) return; diff --git a/modules/gitea/test/tools-account.test.ts b/modules/gitea/test/tools-account.test.ts index 80519429..23a30d67 100644 --- a/modules/gitea/test/tools-account.test.ts +++ b/modules/gitea/test/tools-account.test.ts @@ -28,6 +28,7 @@ interface Team { id: number; name: string; units: any; members: Set } interface ForgeRepo { owner: string; name: string; full: string; private: boolean; archived?: boolean; ownerVisibility: string; collaborators: Map } const forgeRepos: ForgeRepo[] = []; +const userRules = new Map(); const repoFaults = { refuse: new Set(), permission: new Set() }; function addRepo(full: string, isPrivate: boolean, ownerVisibility = "public"): ForgeRepo { const [owner, name] = full.split("/"); @@ -84,7 +85,7 @@ function fakeForge() { const server = createServer(async (req, res) => { const url = new URL(req.url ?? "/", "http://fake"); const p = url.pathname.replace(/^\/api\/v1/, ""); - const me = who(req); + const me = url.searchParams.has("sudo") && isAdmin(who(req)) ? url.searchParams.get("sudo") : who(req); calls.push(`${req.method} ${p} as ${me}`); if (me === null) return json(res, 401, { message: "unauthorized" }); @@ -168,6 +169,20 @@ function fakeForge() { if (m[2] === "feat/unguarded") return json(res, 200, { name: m[2], protected: false }); return json(res, 404, { message: "branch does not exist" }); } + // A repository of a user: its rules are its administrator's (a site administrator, or a collaborator with admin). + m = p.match(/^\/repos\/(?!novox\/)([^/]+)\/([^/]+)\/branch_protections(?:\/(.*))?$/); + if (m) { + const key = `${m[1]}/${m[2]}`; + const repo = forgeRepos.find((r) => r.full === key); + if (!repo || !["admin", "owner"].includes(permissionOf(me, repo))) return json(res, 403, { message: "user should be an owner or a collaborator with admin write of a repository" }); + if (req.method === "GET" && m[3] === undefined) return json(res, 200, userRules.has(key) ? [userRules.get(key)] : []); + if (req.method === "PATCH" && m[3] === "main") { + if (!userRules.has(key)) return json(res, 404, { message: "branch protection does not exist" }); + Object.assign(userRules.get(key), await read(req)); + return json(res, 200, userRules.get(key)); + } + if (req.method === "POST") { const b = await read(req); userRules.set(key, b); return json(res, 201, b); } + } // gitea's reqAdmin on the whole group: reading the rules is administration too. m = p.match(/^\/repos\/novox\/([^/]+)\/branch_protections(?:\/(.*))?$/); if (m) { @@ -205,6 +220,7 @@ function fakeForge() { m = p.match(/^\/repos\/([^/]+)\/([^/]+)(?:\/collaborators\/([^/]+)(\/permission)?)?$/); if (m) { const repo = forgeRepos.find((r) => r.owner === decodeURIComponent(m![1]) && r.name === decodeURIComponent(m![2])); + if (repo && req.method === "GET" && !m[3] && url.searchParams.has("sudo") && repoFaults.permission.has(repo.full)) return json(res, 500, { message: "the forge is not well" }); if (!repo || permissionOf(me, repo) === "none") return json(res, 404, { message: "repository does not exist" }); if (req.method === "GET" && !m[3]) { const mine = permissionOf(me, repo); @@ -214,7 +230,8 @@ function fakeForge() { if (req.method === "GET" && m[4]) { if (repoFaults.permission.has(repo.full)) return json(res, 500, { message: "the forge is not well" }); if (!isAdmin(me) && me !== user) return json(res, 403, { message: "only the user or an administrator" }); - return json(res, 200, { permission: permissionOf(user, repo) }); + // As gitea answers: the collaborator's own permission, not what a team gives the account. + return json(res, 200, { permission: repo.collaborators.get(user) ?? "read" }); } if (req.method === "PUT" && !m[4]) { // gitea's reqAdmin: the repository's administration, which on a repository of another user the site administrator alone holds. @@ -608,7 +625,9 @@ test("the merging account is given write on each repository the indexer's module await writes.take({ key: "car-hunter-2", op: "put", value: { v: 1, repository: "jschoubben/car-hunter" } }); assert.equal(forge.calls.length, from, "a repository already written asks nothing more"); const repo = (full: string) => forgeRepos.find((x) => x.full === full)!; - assert.equal(repo("jschoubben/car-hunter").collaborators.get(TOOLS_ACCOUNT), "write"); + assert.equal(repo("jschoubben/car-hunter").collaborators.get(TOOLS_ACCOUNT), "admin", "a user's repository is administered, not only written"); + assert.deepEqual(userRules.get("jschoubben/car-hunter"), { rule_name: "main", enable_push: false, enable_merge_whitelist: true, + merge_whitelist_usernames: [TOOLS_ACCOUNT], enable_status_check: false, status_check_contexts: [], block_admin_merge_override: true }); assert.equal(repo("novox/hq").collaborators.size, 0, "the organisation's repository is written through its team, no collaborator"); assert.equal((await client.getRepo("jschoubben", "car-hunter")).writes, true); assert.deepEqual(said, [{ type: WRITABLE_EVENT, body: { owner: "jschoubben", repo: "car-hunter", account: TOOLS_ACCOUNT } }]); @@ -636,8 +655,14 @@ test("the merging account is given write on each repository the indexer's module assert.ok(!forge.calls.slice(puts).some((c) => c.startsWith("PUT ")), "no write is asked on a repository no module builds from"); assert.equal(repo("jschoubben/dotfiles").collaborators.get(TOOLS_ACCOUNT), "read"); repoFaults.refuse.clear(); + // A stronger rule the operator set (a whitelist of the tools' account alone, two approvals) is kept, never weakened. + userRules.set("jschoubben/snake", { rule_name: "main", enable_push: true, enable_merge_whitelist: true, + merge_whitelist_usernames: [TOOLS_ACCOUNT], required_approvals: 2, block_admin_merge_override: false }); const after = await grantTool.run({ repositories: ["jschoubben/snake"] }) as any; assert.deepEqual(after.granted, ["jschoubben/snake"]); + assert.deepEqual(after.main_rule.map((x: any) => [x.repository, x.created, x.rule.enable_push, x.rule.required_approvals, x.rule.block_admin_merge_override]), + [["jschoubben/snake", false, false, 2, true]]); + assert.equal(repo("jschoubben/snake").collaborators.get(TOOLS_ACCOUNT), "admin"); assert.deepEqual(((await unwritableTool.run({})) as any).unwritable, []); // A module gone from the indexer leaves its repository out of the delivered list. await writes.take({ key: "snake", op: "delete" }); diff --git a/modules/gitea/writes.ts b/modules/gitea/writes.ts index ae9d41a6..70ef948c 100644 --- a/modules/gitea/writes.ts +++ b/modules/gitea/writes.ts @@ -18,10 +18,14 @@ // `repo.writable` {owner, repo, account}: mesh-delivery-gate, which does not ask the operator's Approve of a pull request // whose merge the forge would refuse, reads that repository again on it. // +// **A repository of a user is administered, not only written** (the operator, 2026-10-26: "mesh-tools should get all the +// permissions I give him"): the account is an admin collaborator there and puts main's rule right (client.ts ensureMainRule). +// // Not taken away: a repository whose last module goes from the indexer keeps its collaborator until a hand removes it; // gitea_unwritable_repos and gitea_grant_write say the delivered list as it is now. import type { ForgeRepository } from "./client.js"; +import type { BranchProtection } from "./protection.js"; /** The indexer's state this module reads: one key per registered module, its value naming the repository. */ export const INDEXER_MODULES = "mesh-delivery-indexer.modules"; @@ -33,7 +37,8 @@ export interface WriteForge { everyRepository(): Promise; repository(owner: string, repo: string): Promise; repositoryPermission(owner: string, repo: string, user: string): Promise; - addWriteCollaborator(owner: string, repo: string, user: string): Promise; + addAdminCollaborator(owner: string, repo: string, user: string): Promise; + ensureMainRule(owner: string, repo: string): Promise<{ created: boolean; rule: BranchProtection }>; } /** One change of the indexer's modules state, as the SDK's state watch hands it. */ @@ -51,8 +56,12 @@ export interface Written { delivered: string[]; /** Made writable now, each read back from the forge. */ granted: string[]; - /** Written already (the organisation's team, or a collaborator that writes). */ + /** Administered already (a repository of a user), or written through the organisation's team. */ already: string[]; + /** Each repository of a user whose main's rule was put right (no push, the merge whitelist, no status, no administrator + * override), read back: what the forge holds for it now. */ + main_rule: { repository: string; created: boolean; rule: { enable_push?: boolean; merge_whitelist_usernames?: string[] | null; + block_admin_merge_override?: boolean; enable_status_check?: boolean; required_approvals?: number } }[]; /** The forge refused, in its own words; or the repository is the organisation's, whose team should write it. */ refused: { repository: string; why: string }[]; /** Repositories the account cannot write that are left so, and why: they build no registered module. */ @@ -60,6 +69,7 @@ export interface Written { } const WRITES = new Set(["write", "admin", "owner"]); +const ADMINS = new Set(["admin", "owner"]); const said = (err: unknown): string => (err instanceof Error ? err.message : String(err)); function split(full: string): [string, string] { @@ -118,7 +128,7 @@ export class DeliveredWrites { "(the watch is not granted or not answered); nothing is granted"); } const delivered = this.delivered(); - const result: Written = { account: this.account, delivered, granted: [], already: [], refused: [], left_read_only: [] }; + const result: Written = { account: this.account, delivered, granted: [], already: [], main_rule: [], refused: [], left_read_only: [] }; const named = only && only.length ? only.map((x) => String(x).trim()) : null; const targets: string[] = []; for (const full of named ?? delivered) { @@ -140,36 +150,55 @@ export class DeliveredWrites { result.refused.push({ repository: full, why: `its permission could not be read — ${said(err)}` }); continue; } - if (WRITES.has(permission)) { - this.writable.add(full); - result.already.push(full); - continue; - } if (owner === this.org) { + if (WRITES.has(permission)) { + this.writable.add(full); + result.already.push(full); + continue; + } // The organisation's repositories are written through its team (tools-account.ts), never a collaborator. result.refused.push({ repository: full, why: `a repository of ${this.org}, which ${this.account} writes through the ` + `team ${this.org}/${this.account}; the forge gives it "${permission}", so the team is not as it should be (it is ` + "set again at the next mint of the tools' token)" }); continue; } - try { - await this.forge.addWriteCollaborator(owner, name, this.account); - } catch (err) { - result.refused.push({ repository: full, why: said(err) }); - continue; + // A repository of a user: the account administers it (the operator, 2026-10-26: "mesh-tools should get all the + // permissions I give him"), so it also puts main's rule right, which only an administrator may. + let changed = false; + if (!ADMINS.has(permission)) { + try { + await this.forge.addAdminCollaborator(owner, name, this.account); + } catch (err) { + result.refused.push({ repository: full, why: said(err) }); + continue; + } + let after: string; + try { + after = await this.forge.repositoryPermission(owner, name, this.account); + } catch (err) { + result.refused.push({ repository: full, why: `granted, but its permission could not be read back — ${said(err)}` }); + continue; + } + if (!ADMINS.has(after)) { + result.refused.push({ repository: full, why: `the grant was answered, but the forge still gives ${this.account} "${after}"` }); + continue; + } + changed = true; } - let after: string; try { - after = await this.forge.repositoryPermission(owner, name, this.account); + const { created, rule } = await this.forge.ensureMainRule(owner, name); + result.main_rule.push({ repository: full, created, rule: { enable_push: rule.enable_push, + merge_whitelist_usernames: rule.merge_whitelist_usernames, block_admin_merge_override: rule.block_admin_merge_override, + enable_status_check: rule.enable_status_check, required_approvals: rule.required_approvals } }); } catch (err) { - result.refused.push({ repository: full, why: `granted, but its permission could not be read back — ${said(err)}` }); - continue; - } - if (!WRITES.has(after)) { - result.refused.push({ repository: full, why: `the grant was answered, but the forge still gives ${this.account} "${after}"` }); + result.refused.push({ repository: full, why: `${this.account} administers it, but main's rule could not be put right — ${said(err)}` }); continue; } this.writable.add(full); + if (!changed) { + result.already.push(full); + continue; + } result.granted.push(full); await this.emit(WRITABLE_EVENT, { owner, repo: name, account: this.account }).catch((err) => this.log(`${this.account} writes ${full} now, and that could not be said on the bus — ${said(err)}`)); @@ -230,12 +259,14 @@ export function writeTools(writes: () => DeliveredWrites): Tool[] { }, { name: "gitea_grant_write", - description: "Give the mesh's merging forge account (mesh-tools) write, as a collaborator, on every repository that " + - "builds a registered module and that it cannot write now, or on the repositories named (each must build one). " + + description: "Give the mesh's merging forge account (mesh-tools) administration, as a collaborator, on every repository " + + "of a user that builds a registered module, and put main's rule right there (no push, the merge whitelist, no status " + + "required, no administrator override; a stronger rule the operator set is kept), or on the repositories named (each " + + "must build one). The answer's main_rule says each rule as the forge holds it after, read back. " + "The module does this itself at its start and when a module of a new repository is registered; this is the same " + "act by hand, such as when a merge or a commit status was refused 403. A repository of the organisation novox " + "is written through its team and needs nothing. Answers what was granted (each read back from the forge), what " + - "was written already, what the forge refused in its own words, and which other repositories it left read-only " + + "was administered already, what the forge refused in its own words, and which other repositories it left read-only " + "and why (they build no registered module). Needs the site administrator's login, used for this grant alone.", input: { repositories: { type: "array", items: { type: "string" }, description: "owner/name of each repository; empty or absent, every delivered one" }, diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/progress.go b/modules/mesh-knowledge/cmd/mesh-knowledge/progress.go index 5edd5aaf..99afc4ca 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/progress.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/progress.go @@ -167,6 +167,8 @@ func (k *KB) Progress(a ProgressAsk) (*Progress, error) { if err != nil { return nil, err } + // A cut takes the warnings from the last: the calls that failed are said before the answers that did not parse. + callsFirst(p.Warnings) fitProgress(p, boundOf(a.Bound)) return p, nil } @@ -822,8 +824,21 @@ func movedSay(t *time.Time) string { return " · moved " + when(t) } +// callsFirst puts the warnings of calls that failed before those of answers that did not parse, in place. +func callsFirst(w []string) { + sort.SliceStable(w, func(i, j int) bool { + return !strings.Contains(w[i], " answered something else") && strings.Contains(w[j], " answered something else") + }) +} + // fitProgress keeps an answer within its bound: the items go first, then the tree's last lines, saying what was cut. +// A minimal answer never carries the items: its tree says the same, one line per milestone, so leaving them out is how +// the minimal form is made (the items of design 47 alone passed 8192 bytes) and no cut. A detailed answer's items, and +// everything else that goes, are said in cut. func fitProgress(p *Progress, bound int) { + if p.Detail == DetailMinimal && p.Tree != "" { + p.Items = nil + } if size(p) <= bound { return } diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/progress_test.go b/modules/mesh-knowledge/cmd/mesh-knowledge/progress_test.go index b2895846..ad8347af 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/progress_test.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/progress_test.go @@ -306,12 +306,23 @@ func TestProgressOfADesignMinimalAndDetailed(t *testing.T) { if len(raw) > DefaultBound || p.Detail != DetailMinimal || !strings.Contains(p.Source, "issue 1781") { t.Fatalf("%d bytes, %s, %s", len(raw), p.Detail, p.Source) } + if p.Cut != "" || len(p.Items) != 0 { + t.Errorf("a minimal answer leaves the items to its tree and is not a cut: cut %q, %d items", p.Cut, len(p.Items)) + } + warns := []string{"mesh-delivery.show a answered something else: x", "issue-tracker.get 1: refused", "the tracker did not name 2", "issue-tracker.get 3 answered something else: y"} + callsFirst(warns) + if strings.Join(warns, "|") != "issue-tracker.get 1: refused|the tracker did not name 2|mesh-delivery.show a answered something else: x|issue-tracker.get 3 answered something else: y" { + t.Errorf("call failures do not come first: %q", warns) + } if seats.asked["seat:issue-tracker.list"] != 1 || seats.asked["seat:mesh-delivery.deliveries"] != 0 || seats.asked["seat:issue-tracker.get"] != 0 { t.Fatalf("minimal asked %v", seats.asked) } if w := strings.Join(p.Warnings, "\n"); !strings.Contains(w, "the tracker neither answered nor named missing") || !strings.Contains(w, "973, 525") { t.Errorf("a row the tracker did not answer is not said: %v", p.Warnings) } + if big, _ := k.Progress(ProgressAsk{Of: "design 47", Bound: 8192}); big.Cut != "" || big.Tree != p.Tree { + t.Errorf("a minimal answer at bound 8192 is cut: %q", big.Cut) + } lines := strings.Split(p.Tree, "\n") if !strings.HasPrefix(lines[0], "✗ design 47 · Delivery: from a commit to delivered — ") { t.Fatalf("first line %q", lines[0]) From c85472285d7156ee0f90d8613a0409ad507fd7ba Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 26 Oct 2026 18:23:42 +0100 Subject: [PATCH 2/4] gitea: main's rule read back must say what was written, since the forge silently drops a whitelist login that cannot write (issue 1945, review round 1) --- modules/gitea/client.ts | 7 +++++++ modules/gitea/test/tools-account.test.ts | 16 ++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index f63a4456..5e9f102e 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -1102,6 +1102,13 @@ export class GiteaAdmin { } const after = (await this.branchProtections(owner, repo)).find((r) => (r.rule_name ?? r.branch_name) === MAIN_BRANCH); if (!after) throw new Error(`main's rule of ${owner}/${repo} was written, but the forge does not list it`); + // The forge answers a write it did not keep: it drops a whitelist login that cannot write the repository, silently. + const listed = after.merge_whitelist_usernames ?? []; + const wrong: string[] = []; + if (after.enable_push !== false) wrong.push(`push is ${after.enable_push === true ? "allowed" : "not refused"}`); + if (after.enable_merge_whitelist !== true || !listed.includes(mergers[0])) wrong.push(`the merge whitelist [${listed.join(", ")}] leaves out ${mergers[0]}`); + if (after.enable_status_check !== false) wrong.push("a status is still required"); + if (wrong.length > 0) throw new Error(`main's rule of ${owner}/${repo} was written, but the forge holds otherwise: ${wrong.join("; ")}`); return { created: !own, rule: after }; } diff --git a/modules/gitea/test/tools-account.test.ts b/modules/gitea/test/tools-account.test.ts index 23a30d67..2f1cd6aa 100644 --- a/modules/gitea/test/tools-account.test.ts +++ b/modules/gitea/test/tools-account.test.ts @@ -176,12 +176,15 @@ function fakeForge() { const repo = forgeRepos.find((r) => r.full === key); if (!repo || !["admin", "owner"].includes(permissionOf(me, repo))) return json(res, 403, { message: "user should be an owner or a collaborator with admin write of a repository" }); if (req.method === "GET" && m[3] === undefined) return json(res, 200, userRules.has(key) ? [userRules.get(key)] : []); + // As gitea does: a whitelist login that cannot write the repository is dropped, and the write still answered. + const keep = (b: any) => (Array.isArray(b.merge_whitelist_usernames) + ? { ...b, merge_whitelist_usernames: b.merge_whitelist_usernames.filter((u: string) => ["write", "admin", "owner"].includes(permissionOf(u, repo))) } : b); if (req.method === "PATCH" && m[3] === "main") { if (!userRules.has(key)) return json(res, 404, { message: "branch protection does not exist" }); - Object.assign(userRules.get(key), await read(req)); + Object.assign(userRules.get(key), keep(await read(req))); return json(res, 200, userRules.get(key)); } - if (req.method === "POST") { const b = await read(req); userRules.set(key, b); return json(res, 201, b); } + if (req.method === "POST") { const b = keep(await read(req)); userRules.set(key, b); return json(res, 201, b); } } // gitea's reqAdmin on the whole group: reading the rules is administration too. m = p.match(/^\/repos\/novox\/([^/]+)\/branch_protections(?:\/(.*))?$/); @@ -664,6 +667,15 @@ test("the merging account is given write on each repository the indexer's module [["jschoubben/snake", false, false, 2, true]]); assert.equal(repo("jschoubben/snake").collaborators.get(TOOLS_ACCOUNT), "admin"); assert.deepEqual(((await unwritableTool.run({})) as any).unwritable, []); + // The forge drops a whitelist login that cannot write and still answers 200: main's rule is read back and that fails loud. + const admin = GiteaAdmin.fromEnv(env); + await assert.rejects(admin.ensureMainRule("jschoubben", "dotfiles"), + /main's rule of jschoubben\/dotfiles was written, but the forge holds otherwise: the merge whitelist \[\] leaves out mesh-tools/); + userRules.get("jschoubben/snake").enable_status_check = true; + userRules.get("jschoubben/snake").enable_push = true; + const held = await admin.ensureMainRule("jschoubben", "snake"); + assert.deepEqual([held.rule.enable_push, held.rule.enable_status_check, held.rule.merge_whitelist_usernames], [false, false, [TOOLS_ACCOUNT]]); + userRules.delete("jschoubben/dotfiles"); // A module gone from the indexer leaves its repository out of the delivered list. await writes.take({ key: "snake", op: "delete" }); assert.deepEqual(writes.delivered(), ["jschoubben/car-hunter", "novox/hq"]); From e1e5a8ff0dfc97d19a59250bb7efacf5ea116c44 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 26 Oct 2026 18:34:03 +0100 Subject: [PATCH 3/4] gitea: mesh-tools writes a user's delivered repository and main's rule stays the operator's: only its merge whitelist gets mesh-tools, read back; what else blocks the mesh's merge is said (issue 1945, the operator's answer 2026-10-26 18:30) --- modules/gitea/client.ts | 68 +++++++-------- modules/gitea/test/tools-account.test.ts | 40 +++++---- modules/gitea/writes.ts | 101 ++++++++++++++++------- 3 files changed, 124 insertions(+), 85 deletions(-) diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index 5e9f102e..408c93af 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -6,7 +6,7 @@ import { createHash } from "node:crypto"; import { readFileSync } from "node:fs"; import { ConfiguredToken, MintedToken, type TokenSource } from "./token.js"; -import { MAIN_BRANCH, mainMergers, mainRuleBody, protectionBody, type BranchProtection, type ProtectionWanted } from "./protection.js"; +import { MAIN_BRANCH, mainRuleBody, protectionBody, type BranchProtection, type ProtectionWanted } from "./protection.js"; import { Refused, checkSentPath, ruleCovers, pathSegment, pathSegments } from "./guard.js"; import { isCommit } from "./checks.js"; @@ -1074,45 +1074,39 @@ export class GiteaAdmin { await this.addCollaborator(owner, repo, user, "write"); } - /** Make an account an administrator collaborator of a repository (writes.ts: a repository of a user that builds a - * registered module, where the tools' account must also put main's rule right). */ - async addAdminCollaborator(owner: string, repo: string, user: string): Promise { - await this.addCollaborator(owner, repo, user, "admin"); - } - - /** Put main's rule right in one repository: no push, the merge whitelist (the tools' account first), no status required, - * and no administrator override. A rule the operator already set that is stronger is kept, never made weaker (ADR 0266): - * a whitelist that holds the tools' account and only logins the wanted one names, more approvals, a push already refused. - * Answers the rule as the forge holds it after, read back. With the administrator's login. */ - async ensureMainRule(owner: string, repo: string, mergers: string[] = mainMergers()): Promise<{ created: boolean; rule: BranchProtection }> { + /** The rule that applies to main in a repository, as its administrator reads it: a rule named main, else the first glob + * rule covering main; null when none does. With the administrator's login. */ + async mainRule(owner: string, repo: string): Promise { const rules = await this.branchProtections(owner, repo); - const own = rules.find((r) => (r.rule_name ?? r.branch_name) === MAIN_BRANCH); - const kept = own?.enable_merge_whitelist === true ? (own.merge_whitelist_usernames ?? []) : []; - const whitelist = kept.length > 0 && kept.includes(mergers[0]) && kept.every((u) => mergers.includes(u)) ? kept : mergers; - const body = { ...mainRuleBody(mergers), merge_whitelist_usernames: whitelist, block_admin_merge_override: true }; - const path = `${repoPath(owner, repo)}/branch_protections`; - if (own) { - const res = await this.request(`${path}/${MAIN_BRANCH}`, { method: "PATCH", body: JSON.stringify(body) }); - if (res.status !== 200) GiteaAdmin.fail(`${path}/${MAIN_BRANCH}`, res); - } else { - const covering = rules.filter((r) => ruleCovers(String(r.rule_name ?? r.branch_name ?? ""), MAIN_BRANCH)); - const approvals = Math.max(0, ...covering.map((r) => Number(r.required_approvals ?? 0))); - const res = await this.request(path, { method: "POST", body: JSON.stringify({ rule_name: MAIN_BRANCH, ...(approvals > 0 ? { required_approvals: approvals } : {}), ...body }) }); - if (res.status !== 201) GiteaAdmin.fail(path, res); - } - const after = (await this.branchProtections(owner, repo)).find((r) => (r.rule_name ?? r.branch_name) === MAIN_BRANCH); - if (!after) throw new Error(`main's rule of ${owner}/${repo} was written, but the forge does not list it`); - // The forge answers a write it did not keep: it drops a whitelist login that cannot write the repository, silently. - const listed = after.merge_whitelist_usernames ?? []; - const wrong: string[] = []; - if (after.enable_push !== false) wrong.push(`push is ${after.enable_push === true ? "allowed" : "not refused"}`); - if (after.enable_merge_whitelist !== true || !listed.includes(mergers[0])) wrong.push(`the merge whitelist [${listed.join(", ")}] leaves out ${mergers[0]}`); - if (after.enable_status_check !== false) wrong.push("a status is still required"); - if (wrong.length > 0) throw new Error(`main's rule of ${owner}/${repo} was written, but the forge holds otherwise: ${wrong.join("; ")}`); - return { created: !own, rule: after }; + return rules.find((r) => (r.rule_name ?? r.branch_name) === MAIN_BRANCH) + ?? rules.find((r) => ruleCovers(String(r.rule_name ?? r.branch_name ?? ""), MAIN_BRANCH)) ?? null; } - private async addCollaborator(owner: string, repo: string, user: string, permission: "read" | "write" | "admin"): Promise { + /** Put an account on main's merge whitelist in a repository of a user, and nothing else (the operator, 2026-10-26: + * write, the rule untouched). Only an existing rule whose merge whitelist is on is changed, and only its whitelist: the + * account is added to the logins already there. Push, status checks, approvals and the administrator override stay as + * the operator set them; with no rule, or the whitelist off, nothing is written (whoever writes may merge). Read back: + * the forge drops a whitelist login that cannot write the repository and still answers success, so a whitelist read back + * without the account fails loud. Answers the rule as the forge holds it after. With the administrator's login. */ + async ensureMergerListed(owner: string, repo: string, account: string): Promise<{ changed: boolean; rule: BranchProtection | null }> { + const rule = await this.mainRule(owner, repo); + const listed = rule?.merge_whitelist_usernames ?? []; + if (!rule || rule.enable_merge_whitelist !== true || listed.includes(account)) return { changed: false, rule }; + const name = String(rule.rule_name ?? rule.branch_name); + const path = `${repoPath(owner, repo)}/branch_protections/${encodeURIComponent(name)}`; + const res = await this.request(path, { method: "PATCH", body: JSON.stringify({ merge_whitelist_usernames: [...listed, account] }) }); + if (res.status !== 200) GiteaAdmin.fail(path, res); + const after = (await this.branchProtections(owner, repo)).find((r) => (r.rule_name ?? r.branch_name) === name); + if (!after) throw new Error(`main's rule ${name} of ${owner}/${repo} was written, but the forge does not list it`); + const held = after.merge_whitelist_usernames ?? []; + if (after.enable_merge_whitelist === true && !held.includes(account)) { + throw new Error(`${account} was added to the merge whitelist of main's rule ${name} of ${owner}/${repo}, but the forge ` + + `holds [${held.join(", ")}]: it drops a login that cannot write the repository`); + } + return { changed: true, rule: after }; + } + + private async addCollaborator(owner: string, repo: string, user: string, permission: "read" | "write"): Promise { const path = `${repoPath(owner, repo)}/collaborators/${encodeURIComponent(user)}`; const res = await this.request(path, { method: "PUT", body: JSON.stringify({ permission }) }); if (res.status === 204 || res.status === 200) return; diff --git a/modules/gitea/test/tools-account.test.ts b/modules/gitea/test/tools-account.test.ts index 2f1cd6aa..02644863 100644 --- a/modules/gitea/test/tools-account.test.ts +++ b/modules/gitea/test/tools-account.test.ts @@ -628,9 +628,8 @@ test("the merging account is given write on each repository the indexer's module await writes.take({ key: "car-hunter-2", op: "put", value: { v: 1, repository: "jschoubben/car-hunter" } }); assert.equal(forge.calls.length, from, "a repository already written asks nothing more"); const repo = (full: string) => forgeRepos.find((x) => x.full === full)!; - assert.equal(repo("jschoubben/car-hunter").collaborators.get(TOOLS_ACCOUNT), "admin", "a user's repository is administered, not only written"); - assert.deepEqual(userRules.get("jschoubben/car-hunter"), { rule_name: "main", enable_push: false, enable_merge_whitelist: true, - merge_whitelist_usernames: [TOOLS_ACCOUNT], enable_status_check: false, status_check_contexts: [], block_admin_merge_override: true }); + assert.equal(repo("jschoubben/car-hunter").collaborators.get(TOOLS_ACCOUNT), "write", "a user's repository is written, not administered"); + assert.equal(userRules.has("jschoubben/car-hunter"), false, "a repository with no rule for main is given none"); assert.equal(repo("novox/hq").collaborators.size, 0, "the organisation's repository is written through its team, no collaborator"); assert.equal((await client.getRepo("jschoubben", "car-hunter")).writes, true); assert.deepEqual(said, [{ type: WRITABLE_EVENT, body: { owner: "jschoubben", repo: "car-hunter", account: TOOLS_ACCOUNT } }]); @@ -658,23 +657,28 @@ test("the merging account is given write on each repository the indexer's module assert.ok(!forge.calls.slice(puts).some((c) => c.startsWith("PUT ")), "no write is asked on a repository no module builds from"); assert.equal(repo("jschoubben/dotfiles").collaborators.get(TOOLS_ACCOUNT), "read"); repoFaults.refuse.clear(); - // A stronger rule the operator set (a whitelist of the tools' account alone, two approvals) is kept, never weakened. - userRules.set("jschoubben/snake", { rule_name: "main", enable_push: true, enable_merge_whitelist: true, - merge_whitelist_usernames: [TOOLS_ACCOUNT], required_approvals: 2, block_admin_merge_override: false }); + // The operator's rule is left as set (the operator, 2026-10-26: "Write, rule untouched"): only mesh-tools is added to its + // merge whitelist; a status the mesh does not set and required approvals are said as blocking the mesh's merge, unchanged. + const operators = { rule_name: "main", enable_push: true, enable_merge_whitelist: true, merge_whitelist_usernames: ["jschoubben"], + enable_status_check: true, status_check_contexts: ["mesh/merge-gate", "ci/external"], required_approvals: 2, block_admin_merge_override: false }; + userRules.set("jschoubben/snake", structuredClone(operators)); const after = await grantTool.run({ repositories: ["jschoubben/snake"] }) as any; assert.deepEqual(after.granted, ["jschoubben/snake"]); - assert.deepEqual(after.main_rule.map((x: any) => [x.repository, x.created, x.rule.enable_push, x.rule.required_approvals, x.rule.block_admin_merge_override]), - [["jschoubben/snake", false, false, 2, true]]); - assert.equal(repo("jschoubben/snake").collaborators.get(TOOLS_ACCOUNT), "admin"); - assert.deepEqual(((await unwritableTool.run({})) as any).unwritable, []); - // The forge drops a whitelist login that cannot write and still answers 200: main's rule is read back and that fails loud. - const admin = GiteaAdmin.fromEnv(env); - await assert.rejects(admin.ensureMainRule("jschoubben", "dotfiles"), - /main's rule of jschoubben\/dotfiles was written, but the forge holds otherwise: the merge whitelist \[\] leaves out mesh-tools/); - userRules.get("jschoubben/snake").enable_status_check = true; - userRules.get("jschoubben/snake").enable_push = true; - const held = await admin.ensureMainRule("jschoubben", "snake"); - assert.deepEqual([held.rule.enable_push, held.rule.enable_status_check, held.rule.merge_whitelist_usernames], [false, false, [TOOLS_ACCOUNT]]); + assert.equal(repo("jschoubben/snake").collaborators.get(TOOLS_ACCOUNT), "write"); + assert.deepEqual(userRules.get("jschoubben/snake"), { ...operators, merge_whitelist_usernames: ["jschoubben", TOOLS_ACCOUNT] }, "only the whitelist changed"); + assert.deepEqual(after.merge_whitelist, [{ repository: "jschoubben/snake", changed: true, listed: ["jschoubben", TOOLS_ACCOUNT] }]); + const blockedWhy = "writable, but main's rule blocks the mesh's merge: it requires the status ci/external, which the mesh does not set; " + + "it requires 2 approving reviews on the forge, which the gate does not give"; + assert.deepEqual(after.merge_blocked, [{ repository: "jschoubben/snake", why: blockedWhy }]); + const again = await unwritableTool.run({}) as any; + assert.deepEqual([again.unwritable, again.merge_blocked], [[], [{ repository: "jschoubben/snake", why: blockedWhy }]]); + const writesBefore = forge.calls.filter((c) => /^PATCH .*branch_protections/.test(c)).length; + assert.deepEqual(((await grantTool.run({ repositories: ["jschoubben/snake"] })) as any).merge_whitelist[0].changed, false); + assert.equal(forge.calls.filter((c) => /^PATCH .*branch_protections/.test(c)).length, writesBefore, "a listed account is not written again"); + // The forge drops a whitelist login that cannot write and still answers 200: the whitelist is read back and that fails loud. + userRules.set("jschoubben/dotfiles", { rule_name: "main", enable_merge_whitelist: true, merge_whitelist_usernames: ["jschoubben"] }); + await assert.rejects(GiteaAdmin.fromEnv(env).ensureMergerListed("jschoubben", "dotfiles", TOOLS_ACCOUNT), + /mesh-tools was added to the merge whitelist of main's rule main of jschoubben\/dotfiles, but the forge holds \[jschoubben\]: it drops a login that cannot write/); userRules.delete("jschoubben/dotfiles"); // A module gone from the indexer leaves its repository out of the delivered list. await writes.take({ key: "snake", op: "delete" }); diff --git a/modules/gitea/writes.ts b/modules/gitea/writes.ts index 70ef948c..5237625c 100644 --- a/modules/gitea/writes.ts +++ b/modules/gitea/writes.ts @@ -18,14 +18,18 @@ // `repo.writable` {owner, repo, account}: mesh-delivery-gate, which does not ask the operator's Approve of a pull request // whose merge the forge would refuse, reads that repository again on it. // -// **A repository of a user is administered, not only written** (the operator, 2026-10-26: "mesh-tools should get all the -// permissions I give him"): the account is an admin collaborator there and puts main's rule right (client.ts ensureMainRule). +// **A repository of a user is written, and main's rule is left as the operator set it** (the operator, 2026-10-26, asked +// "Write, rule untouched"): the account is a write collaborator there, and only added to main's merge whitelist when that +// rule keeps one (client.ts ensureMergerListed). What else in main's rule stops the mesh's merge (a status the mesh does not +// set, approvals the gate does not give) is changed by nobody but said: both tools list such a repository as writable, but +// blocked, with why (merge_blocked). // // Not taken away: a repository whose last module goes from the indexer keeps its collaborator until a hand removes it; // gitea_unwritable_repos and gitea_grant_write say the delivered list as it is now. import type { ForgeRepository } from "./client.js"; import type { BranchProtection } from "./protection.js"; +import { CHECK_CONTEXT, REPO_CHECK_CONTEXT } from "./pulls.js"; /** The indexer's state this module reads: one key per registered module, its value naming the repository. */ export const INDEXER_MODULES = "mesh-delivery-indexer.modules"; @@ -37,10 +41,33 @@ export interface WriteForge { everyRepository(): Promise; repository(owner: string, repo: string): Promise; repositoryPermission(owner: string, repo: string, user: string): Promise; - addAdminCollaborator(owner: string, repo: string, user: string): Promise; - ensureMainRule(owner: string, repo: string): Promise<{ created: boolean; rule: BranchProtection }>; + addWriteCollaborator(owner: string, repo: string, user: string): Promise; + mainRule(owner: string, repo: string): Promise; + ensureMergerListed(owner: string, repo: string, account: string): Promise<{ changed: boolean; rule: BranchProtection | null }>; } +/** The statuses every pull request the mesh delivers carries (mesh-delivery's checks); a rule requiring another blocks. */ +const MESH_CONTEXTS = new Set([CHECK_CONTEXT, REPO_CHECK_CONTEXT]); + +/** What in main's rule stops the mesh's merge, in words; empty when nothing does. Changed by nobody here: the operator's. */ +export function mergeBlocks(rule: BranchProtection | null, account: string): string[] { + if (!rule) return []; + const blocks: string[] = []; + if (rule.enable_merge_whitelist === true && !(rule.merge_whitelist_usernames ?? []).includes(account)) { + blocks.push(`its merge whitelist [${(rule.merge_whitelist_usernames ?? []).join(", ")}] leaves out ${account} (gitea_grant_write adds it)`); + } + if (rule.enable_status_check === true) { + for (const c of rule.status_check_contexts ?? []) { + if (!MESH_CONTEXTS.has(c)) blocks.push(`it requires the status ${c}, which the mesh does not set`); + } + } + const approvals = Number(rule.required_approvals ?? 0); + if (approvals > 0) blocks.push(`it requires ${approvals} approving review${approvals === 1 ? "" : "s"} on the forge, which the gate does not give`); + return blocks; +} + +const blockedWhy = (blocks: string[]): string => `writable, but main's rule blocks the mesh's merge: ${blocks.join("; ")}`; + /** One change of the indexer's modules state, as the SDK's state watch hands it. */ export interface ModulesChange { key: string; @@ -56,12 +83,13 @@ export interface Written { delivered: string[]; /** Made writable now, each read back from the forge. */ granted: string[]; - /** Administered already (a repository of a user), or written through the organisation's team. */ + /** Written already (a collaborator of a user's repository), or written through the organisation's team. */ already: string[]; - /** Each repository of a user whose main's rule was put right (no push, the merge whitelist, no status, no administrator - * override), read back: what the forge holds for it now. */ - main_rule: { repository: string; created: boolean; rule: { enable_push?: boolean; merge_whitelist_usernames?: string[] | null; - block_admin_merge_override?: boolean; enable_status_check?: boolean; required_approvals?: number } }[]; + /** Each repository of a user: whether the account was added to main's merge whitelist now, and the whitelist read back + * (null: main's rule keeps no whitelist, or there is no rule, and whoever writes may merge). */ + merge_whitelist: { repository: string; changed: boolean; listed: string[] | null }[]; + /** Writable, but main's rule (the operator's, left as set) stops the mesh's merge, and why. */ + merge_blocked: { repository: string; why: string }[]; /** The forge refused, in its own words; or the repository is the organisation's, whose team should write it. */ refused: { repository: string; why: string }[]; /** Repositories the account cannot write that are left so, and why: they build no registered module. */ @@ -69,7 +97,6 @@ export interface Written { } const WRITES = new Set(["write", "admin", "owner"]); -const ADMINS = new Set(["admin", "owner"]); const said = (err: unknown): string => (err instanceof Error ? err.message : String(err)); function split(full: string): [string, string] { @@ -128,7 +155,7 @@ export class DeliveredWrites { "(the watch is not granted or not answered); nothing is granted"); } const delivered = this.delivered(); - const result: Written = { account: this.account, delivered, granted: [], already: [], main_rule: [], refused: [], left_read_only: [] }; + const result: Written = { account: this.account, delivered, granted: [], already: [], merge_whitelist: [], merge_blocked: [], refused: [], left_read_only: [] }; const named = only && only.length ? only.map((x) => String(x).trim()) : null; const targets: string[] = []; for (const full of named ?? delivered) { @@ -162,12 +189,11 @@ export class DeliveredWrites { "set again at the next mint of the tools' token)" }); continue; } - // A repository of a user: the account administers it (the operator, 2026-10-26: "mesh-tools should get all the - // permissions I give him"), so it also puts main's rule right, which only an administrator may. + // A repository of a user: the account writes it, and is put on main's merge whitelist when the rule keeps one. let changed = false; - if (!ADMINS.has(permission)) { + if (!WRITES.has(permission)) { try { - await this.forge.addAdminCollaborator(owner, name, this.account); + await this.forge.addWriteCollaborator(owner, name, this.account); } catch (err) { result.refused.push({ repository: full, why: said(err) }); continue; @@ -179,19 +205,20 @@ export class DeliveredWrites { result.refused.push({ repository: full, why: `granted, but its permission could not be read back — ${said(err)}` }); continue; } - if (!ADMINS.has(after)) { + if (!WRITES.has(after)) { result.refused.push({ repository: full, why: `the grant was answered, but the forge still gives ${this.account} "${after}"` }); continue; } changed = true; } try { - const { created, rule } = await this.forge.ensureMainRule(owner, name); - result.main_rule.push({ repository: full, created, rule: { enable_push: rule.enable_push, - merge_whitelist_usernames: rule.merge_whitelist_usernames, block_admin_merge_override: rule.block_admin_merge_override, - enable_status_check: rule.enable_status_check, required_approvals: rule.required_approvals } }); + const { changed: listedNow, rule } = await this.forge.ensureMergerListed(owner, name, this.account); + result.merge_whitelist.push({ repository: full, changed: listedNow, + listed: rule?.enable_merge_whitelist === true ? (rule.merge_whitelist_usernames ?? []) : null }); + const blocks = mergeBlocks(rule, this.account); + if (blocks.length > 0) result.merge_blocked.push({ repository: full, why: blockedWhy(blocks) }); } catch (err) { - result.refused.push({ repository: full, why: `${this.account} administers it, but main's rule could not be put right — ${said(err)}` }); + result.refused.push({ repository: full, why: `${this.account} writes it, but could not be put on main's merge whitelist — ${said(err)}` }); continue; } this.writable.add(full); @@ -221,10 +248,12 @@ export class DeliveredWrites { } /** Every delivered repository the account cannot write, as the forge says now, and why. */ - async unwritable(): Promise<{ account: string; delivered: number; unwritable: { repository: string; permission: string; why: string }[] }> { + async unwritable(): Promise<{ account: string; delivered: number; unwritable: { repository: string; permission: string; why: string }[]; + merge_blocked: { repository: string; why: string }[] }> { if (!this.heard) throw new Error(`which repositories build a registered module is not known yet: ${INDEXER_MODULES} has not been read`); const delivered = this.delivered(); const out: { repository: string; permission: string; why: string }[] = []; + const blocked: { repository: string; why: string }[] = []; for (const full of delivered) { const [owner, name] = split(full); let permission: string; @@ -234,12 +263,21 @@ export class DeliveredWrites { out.push({ repository: full, permission: "unknown", why: `the forge did not answer ${this.account}'s permission on it — ${said(err)}` }); continue; } - if (WRITES.has(permission)) continue; + if (WRITES.has(permission)) { + if (owner === this.org) continue; // the organisation's main rules are the mesh's own (mainrules.ts) + try { + const blocks = mergeBlocks(await this.forge.mainRule(owner, name), this.account); + if (blocks.length > 0) blocked.push({ repository: full, why: blockedWhy(blocks) }); + } catch (err) { + blocked.push({ repository: full, why: `writable, but main's rule could not be read — ${said(err)}` }); + } + continue; + } const modules = [...this.modules].filter(([, r]) => r === full).map(([m]) => m).sort(); out.push({ repository: full, permission, why: `builds ${modules.join(", ")}, and the forge gives ${this.account} ` + `"${permission}" on it: its commit status and its merge are refused until gitea_grant_write grants it` }); } - return { account: this.account, delivered: delivered.length, unwritable: out }; + return { account: this.account, delivered: delivered.length, unwritable: out, merge_blocked: blocked }; } } @@ -253,20 +291,23 @@ export function writeTools(writes: () => DeliveredWrites): Tool[] { description: "Every repository that builds a registered module (the indexer's modules, read over the bus) on which " + "the mesh's merging forge account (mesh-tools, which sets commit statuses and merges) cannot write, with the " + "permission the forge gives it there and why: such a pull request can be checked but never merged, and the " + - "delivery gate does not ask its Approve. Empty when it writes every one. Read with the site administrator's login.", + "delivery gate does not ask its Approve. Empty when it writes every one. Its merge_blocked lists each repository of " + + "a user it writes whose main's rule (the operator's, left as set) still stops the mesh's merge: a merge whitelist " + + "without it, a required status the mesh does not set, required approving reviews. Read with the site administrator's login.", input: {}, run: async () => writes().unwritable(), }, { name: "gitea_grant_write", - description: "Give the mesh's merging forge account (mesh-tools) administration, as a collaborator, on every repository " + - "of a user that builds a registered module, and put main's rule right there (no push, the merge whitelist, no status " + - "required, no administrator override; a stronger rule the operator set is kept), or on the repositories named (each " + - "must build one). The answer's main_rule says each rule as the forge holds it after, read back. " + + description: "Give the mesh's merging forge account (mesh-tools) write, as a collaborator, on every repository of a " + + "user that builds a registered module, or on the repositories named (each must build one). Main's rule there is left " + + "as the operator set it; only when it keeps a merge whitelist is mesh-tools added to it, read back (merge_whitelist). " + + "What else in that rule stops the mesh's merge (a required status the mesh does not set, required approving reviews) " + + "is changed by nobody and said in merge_blocked. " + "The module does this itself at its start and when a module of a new repository is registered; this is the same " + "act by hand, such as when a merge or a commit status was refused 403. A repository of the organisation novox " + "is written through its team and needs nothing. Answers what was granted (each read back from the forge), what " + - "was administered already, what the forge refused in its own words, and which other repositories it left read-only " + + "was written already, what the forge refused in its own words, and which other repositories it left read-only " + "and why (they build no registered module). Needs the site administrator's login, used for this grant alone.", input: { repositories: { type: "array", items: { type: "string" }, description: "owner/name of each repository; empty or absent, every delivered one" }, From c584dc57b95e52cfa7dc32bdeff20b0362a81de9 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 26 Oct 2026 18:37:02 +0100 Subject: [PATCH 4/4] gitea: mesh/check is the mesh's own status and never blocks; signed commits, an outdated branch and a rejected review are said as blocking the mesh's merge (issue 1945, review round 2) --- modules/gitea/test/tools-account.test.ts | 12 ++++++++++-- modules/gitea/writes.ts | 7 +++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/modules/gitea/test/tools-account.test.ts b/modules/gitea/test/tools-account.test.ts index 02644863..c69edb77 100644 --- a/modules/gitea/test/tools-account.test.ts +++ b/modules/gitea/test/tools-account.test.ts @@ -660,7 +660,8 @@ test("the merging account is given write on each repository the indexer's module // The operator's rule is left as set (the operator, 2026-10-26: "Write, rule untouched"): only mesh-tools is added to its // merge whitelist; a status the mesh does not set and required approvals are said as blocking the mesh's merge, unchanged. const operators = { rule_name: "main", enable_push: true, enable_merge_whitelist: true, merge_whitelist_usernames: ["jschoubben"], - enable_status_check: true, status_check_contexts: ["mesh/merge-gate", "ci/external"], required_approvals: 2, block_admin_merge_override: false }; + enable_status_check: true, status_check_contexts: ["mesh/check", "mesh/merge-gate", "ci/external"], required_approvals: 2, + block_admin_merge_override: false, require_signed_commits: true, block_on_outdated_branch: true, block_on_rejected_reviews: true }; userRules.set("jschoubben/snake", structuredClone(operators)); const after = await grantTool.run({ repositories: ["jschoubben/snake"] }) as any; assert.deepEqual(after.granted, ["jschoubben/snake"]); @@ -668,13 +669,20 @@ test("the merging account is given write on each repository the indexer's module assert.deepEqual(userRules.get("jschoubben/snake"), { ...operators, merge_whitelist_usernames: ["jschoubben", TOOLS_ACCOUNT] }, "only the whitelist changed"); assert.deepEqual(after.merge_whitelist, [{ repository: "jschoubben/snake", changed: true, listed: ["jschoubben", TOOLS_ACCOUNT] }]); const blockedWhy = "writable, but main's rule blocks the mesh's merge: it requires the status ci/external, which the mesh does not set; " + - "it requires 2 approving reviews on the forge, which the gate does not give"; + "it requires 2 approving reviews on the forge, which the gate does not give; it requires signed commits, which the mesh does not make; " + + "it refuses a branch behind main, which the mesh does not bring up to date; " + + "it refuses a merge after a review asked for changes, which the gate does not clear"; assert.deepEqual(after.merge_blocked, [{ repository: "jschoubben/snake", why: blockedWhy }]); const again = await unwritableTool.run({}) as any; assert.deepEqual([again.unwritable, again.merge_blocked], [[], [{ repository: "jschoubben/snake", why: blockedWhy }]]); const writesBefore = forge.calls.filter((c) => /^PATCH .*branch_protections/.test(c)).length; assert.deepEqual(((await grantTool.run({ repositories: ["jschoubben/snake"] })) as any).merge_whitelist[0].changed, false); assert.equal(forge.calls.filter((c) => /^PATCH .*branch_protections/.test(c)).length, writesBefore, "a listed account is not written again"); + // The mesh's own statuses never block: a rule requiring only them, with mesh-tools listed, is not reported. + userRules.set("jschoubben/snake", { rule_name: "main", enable_merge_whitelist: true, merge_whitelist_usernames: [TOOLS_ACCOUNT], + enable_status_check: true, status_check_contexts: ["mesh/check", "mesh/merge-gate", "mesh/repo-check"] }); + assert.deepEqual(((await unwritableTool.run({})) as any).merge_blocked, []); + assert.deepEqual(((await grantTool.run({ repositories: ["jschoubben/snake"] })) as any).merge_blocked, []); // The forge drops a whitelist login that cannot write and still answers 200: the whitelist is read back and that fails loud. userRules.set("jschoubben/dotfiles", { rule_name: "main", enable_merge_whitelist: true, merge_whitelist_usernames: ["jschoubben"] }); await assert.rejects(GiteaAdmin.fromEnv(env).ensureMergerListed("jschoubben", "dotfiles", TOOLS_ACCOUNT), diff --git a/modules/gitea/writes.ts b/modules/gitea/writes.ts index 5237625c..552f7d58 100644 --- a/modules/gitea/writes.ts +++ b/modules/gitea/writes.ts @@ -29,7 +29,7 @@ import type { ForgeRepository } from "./client.js"; import type { BranchProtection } from "./protection.js"; -import { CHECK_CONTEXT, REPO_CHECK_CONTEXT } from "./pulls.js"; +import { CHECK_CONTEXT, CHECK_STATUS, REPO_CHECK_CONTEXT } from "./pulls.js"; /** The indexer's state this module reads: one key per registered module, its value naming the repository. */ export const INDEXER_MODULES = "mesh-delivery-indexer.modules"; @@ -47,7 +47,7 @@ export interface WriteForge { } /** The statuses every pull request the mesh delivers carries (mesh-delivery's checks); a rule requiring another blocks. */ -const MESH_CONTEXTS = new Set([CHECK_CONTEXT, REPO_CHECK_CONTEXT]); +const MESH_CONTEXTS = new Set([CHECK_STATUS, CHECK_CONTEXT, REPO_CHECK_CONTEXT]); /** What in main's rule stops the mesh's merge, in words; empty when nothing does. Changed by nobody here: the operator's. */ export function mergeBlocks(rule: BranchProtection | null, account: string): string[] { @@ -63,6 +63,9 @@ export function mergeBlocks(rule: BranchProtection | null, account: string): str } const approvals = Number(rule.required_approvals ?? 0); if (approvals > 0) blocks.push(`it requires ${approvals} approving review${approvals === 1 ? "" : "s"} on the forge, which the gate does not give`); + if (rule.require_signed_commits === true) blocks.push("it requires signed commits, which the mesh does not make"); + if (rule.block_on_outdated_branch === true) blocks.push("it refuses a branch behind main, which the mesh does not bring up to date"); + if (rule.block_on_rejected_reviews === true) blocks.push("it refuses a merge after a review asked for changes, which the gate does not clear"); return blocks; }