From 7e0001e3045b0ff77a2ca924da5ed32d029bc6c7 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 21:23:26 +0200 Subject: [PATCH 1/3] Merge a pull request marked ready at its head once its checks pass (issue 722) The operator wants a reviewed pull request merged without a session calling the merge tool again after every recheck. The reviewer marks it ready (label mesh/ready and a comment "ready at "); the forge's open-pulls read carries the mark, and mesh-delivery merges through gitea_merge_pull_request at that head, in a group's order once its composed check passed. A push after the mark clears it; a failed check is said on the pull request and as a stalled line. --- modules/gitea/client.ts | 42 ++- modules/gitea/test/announcing.test.ts | 49 +++ modules/gitea/tools/index.ts | 2 +- .../cmd/mesh-delivery/fakes_test.go | 25 ++ .../mesh-delivery/cmd/mesh-delivery/holder.go | 9 + .../mesh-delivery/cmd/mesh-delivery/main.go | 5 +- .../cmd/mesh-delivery/manifest_test.go | 4 +- .../mesh-delivery/cmd/mesh-delivery/model.go | 3 + .../mesh-delivery/cmd/mesh-delivery/ports.go | 20 ++ .../mesh-delivery/cmd/mesh-delivery/ready.go | 323 ++++++++++++++++++ .../cmd/mesh-delivery/ready_test.go | 199 +++++++++++ .../mesh-delivery/cmd/mesh-delivery/table.go | 7 +- .../cmd/mesh-delivery/unchecked.go | 4 + .../mesh-delivery/cmd/mesh-delivery/verbs.go | 1 + modules/mesh-delivery/module.json | 5 +- 15 files changed, 688 insertions(+), 10 deletions(-) create mode 100644 modules/mesh-delivery/cmd/mesh-delivery/ready.go create mode 100644 modules/mesh-delivery/cmd/mesh-delivery/ready_test.go diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index 4ac40fb9..ee0572ee 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -101,6 +101,34 @@ export interface OpenPull { /** The head's statuses by context, the newest of each, with its state (novox/hq issue 438): what tells a check * that started and never answered — pending — from one that answered. */ checks: Record; + /** The pull request's labels, by name. */ + labels?: string[]; + /** Its ready mark (novox/hq issue 722): the label `mesh/ready` and the newest comment "ready at ", the + * head a reviewer approved. Absent without the label; `head` empty when the label names no head in a comment. */ + ready?: ReadyMark; +} + +/** The label that marks a pull request ready to merge at the head a comment names (novox/hq issue 722). */ +export const READY_LABEL = "mesh/ready"; + +/** A comment that names the head a pull request is marked ready at: "ready at ", on a line of its own. */ +const READY_AT = /^\s*ready at ([0-9a-f]{40}|[0-9a-f]{64})\b/im; + +/** A pull request's ready mark: the head it names, who said it and when, and the label's id, to take it off. */ +export interface ReadyMark { + head: string; + by?: string; + at?: string; + label_id: number; +} + +/** The ready mark a pull request's comments make, oldest first as the forge lists them: the newest that names a head. */ +export function readyMarkOf(labelID: number, comments: GiteaComment[]): ReadyMark { + for (let i = comments.length - 1; i >= 0; i--) { + const m = READY_AT.exec(comments[i].body); + if (m) return { head: m[1].toLowerCase(), by: comments[i].user, at: comments[i].created_at, label_id: labelID }; + } + return { head: "", label_id: labelID }; } /** One status on a head, the newest of its context: its state, what it said, and when it was set. */ @@ -332,8 +360,9 @@ export class GiteaClient { /** The open pull requests of every repository the mesh's account sees, updated since a moment when one is given, * page by page (novox/hq issue 347): each with its repository and its own update time. */ - async searchOpenPulls(since?: string): Promise<{ owner: string; name: string; number: number; updated_at?: string }[]> { - const out: { owner: string; name: string; number: number; updated_at?: string }[] = []; + async searchOpenPulls(since?: string): Promise<{ owner: string; name: string; number: number; updated_at?: string; + labels?: { id: number; name: string }[] }[]> { + const out: { owner: string; name: string; number: number; updated_at?: string; labels?: { id: number; name: string }[] }[] = []; for (let page = 1; page <= 20; page++) { const qs = new URLSearchParams({ type: "pulls", state: "open", limit: "50", page: String(page) }); if (since) qs.set("since", since); @@ -343,7 +372,8 @@ export class GiteaClient { const [fo, fn] = String(r.full_name ?? "").split("/"); const owner = String(r.owner ?? fo ?? ""); const name = String(r.name ?? fn ?? ""); - if (owner && name && i?.number) out.push({ owner, name, number: Number(i.number), updated_at: i.updated_at }); + const labels = (Array.isArray(i?.labels) ? i.labels : []).map((l: any) => ({ id: Number(l?.id ?? 0), name: String(l?.name ?? "") })); + if (owner && name && i?.number) out.push({ owner, name, number: Number(i.number), updated_at: i.updated_at, labels }); } if (!found || found.length < 50) break; } @@ -364,9 +394,13 @@ export class GiteaClient { if (!rules.has(full)) rules.set(full, await this.branchProtections(found.owner, found.name).catch(() => undefined)); const rule = rules.get(full)?.find((p) => (p.rule_name ?? p.branch_name) === (pull.base || "main")); const checks = await this.headChecks(found.owner, found.name, pull.head_sha); + const labels = found.labels ?? []; + // The ready mark (novox/hq issue 722): its comments are read only for a pull request that carries the label. + const label = labels.find((l) => l.name === READY_LABEL); + const ready = label ? readyMarkOf(label.id, await this.listComments(found.owner, found.name, pull.number)) : undefined; out.push({ owner: found.owner, repo: found.name, number: pull.number, base: pull.base ?? "", head_sha: pull.head_sha, html_url: pull.html_url, required: rule?.enable_status_check ? (rule.status_check_contexts ?? []) : [], - statuses: Object.keys(checks), checks }); + statuses: Object.keys(checks), checks, labels: labels.map((l) => l.name), ...(ready ? { ready } : {}) }); } return out; } diff --git a/modules/gitea/test/announcing.test.ts b/modules/gitea/test/announcing.test.ts index b24c7fef..54daea78 100644 --- a/modules/gitea/test/announcing.test.ts +++ b/modules/gitea/test/announcing.test.ts @@ -137,3 +137,52 @@ test("a merged pull request whose branch was deleted is announced with its branc assert.equal(headBranchOf({ ref: "refs/pull/7/head" }), "refs/pull/7/head"); assert.equal(headBranchOf(undefined), undefined); }); + +// **A pull request marked ready says the head it is marked at** (novox/hq issue 722): the label mesh/ready and the +// newest comment "ready at ". mesh-delivery merges on it; without the label no comment is read. +test("a pull request's ready mark is read: the label and the head the newest ready comment names", async () => { + const { GiteaClient } = await import("../client.ts"); + const head = "c".repeat(40), older = "d".repeat(40); + const asked: string[] = []; + const server = createServer((req, res) => { + const url = req.url ?? ""; + asked.push(url); + const send = (v: unknown) => res.end(JSON.stringify(v)); + if (url.startsWith("/api/v1/repos/issues/search")) { + return send(url.includes("page=1") ? [ + { number: 7, repository: { owner: "novox", name: "hq", full_name: "novox/hq" }, + labels: [{ id: 31, name: "mesh/ready" }, { id: 2, name: "bug" }] }, + { number: 8, repository: { owner: "novox", name: "hq", full_name: "novox/hq" }, labels: [] }, + ] : []); + } + if (url.startsWith("/api/v1/repos/novox/hq/pulls/7")) { + return send({ number: 7, state: "open", base: { ref: "main" }, head: { ref: "x", sha: head }, html_url: "h" }); + } + if (url.startsWith("/api/v1/repos/novox/hq/pulls/8")) { + return send({ number: 8, state: "open", base: { ref: "main" }, head: { ref: "y", sha: older }, html_url: "h" }); + } + if (url.startsWith("/api/v1/repos/novox/hq/issues/7/comments")) { + return send([ + { id: 1, user: { login: "mesh" }, body: `ready at ${older}`, created_at: "2026-10-14T10:00:00Z" }, + { id: 2, user: { login: "mesh" }, body: `Reviewed: approve.\nready at ${head}\n`, created_at: "2026-10-14T11:00:00Z" }, + { id: 3, user: { login: "jochen" }, body: "thanks", created_at: "2026-10-14T12:00:00Z" }, + ]); + } + if (url.includes("/branch_protections")) return send([]); + if (url.includes("/status")) return send({ statuses: [] }); + res.statusCode = 404; + res.end("{}"); + }); + await new Promise((r) => server.listen(0, r)); + const client = new GiteaClient(`http://127.0.0.1:${(server.address() as any).port}`, "t"); + try { + const open = await client.openPulls(); + assert.deepEqual(open[0].labels, ["mesh/ready", "bug"]); + assert.deepEqual(open[0].ready, { head, by: "mesh", at: "2026-10-14T11:00:00Z", label_id: 31 }, + "the newest comment that names a head"); + assert.equal(open[1].ready, undefined, "no label, no mark"); + assert.ok(!asked.some((u) => u.includes("/issues/8/comments")), "no comment read without the label"); + } finally { + server.close(); + } +}); diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index 201b08d0..99803c6c 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -514,7 +514,7 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { }, { name: "gitea_open_pulls", - description: "Every open pull request of every repository, from the forge itself: its head, its base, the statuses the base's protection requires and the ones its head has, each with its state, description and when it was set. What mesh-delivery reads to say a pull request the mesh was never asked to check (novox/hq issue 347), or whose check started and never answered (issue 438). Reads only.", + description: "Every open pull request of every repository, from the forge itself: its head, its base, the statuses the base's protection requires and the ones its head has, each with its state, description and when it was set. What mesh-delivery reads to say a pull request the mesh was never asked to check (novox/hq issue 347), or whose check started and never answered (issue 438), and each pull request's labels and ready mark — the label mesh/ready and the head the newest comment \"ready at \" names — which mesh-delivery merges on (issue 722). Reads only.", input: {}, run: async () => ({ pulls: await gitea.openPulls() }), }, diff --git a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go index a9f0768a..d6572f79 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go @@ -396,6 +396,9 @@ type fakeForge struct { // open is what OpenPulls answers; openDown a forge that cannot say. open []OpenPull openDown bool + // merges, comments and unlabels are what was asked of the forge for ready marks (novox/hq issue 722). + merges, comments, unlabels []string + refuse error } func (f *fakeForge) OpenPulls() ([]OpenPull, error) { @@ -649,3 +652,25 @@ func aWalk(id, repo, commit string, waits bool) Walk { } return w } + +// Merge, Comment and Unlabel record what was asked (novox/hq issue 722); Merge answers refuse when set. +func (f *fakeForge) Merge(owner, repo string, number int, head string) error { + f.mu.Lock() + defer f.mu.Unlock() + f.merges = append(f.merges, fmt.Sprintf("%s/%s#%d@%s", owner, repo, number, head)) + return f.refuse +} + +func (f *fakeForge) Comment(owner, repo string, number int, body string) error { + f.mu.Lock() + defer f.mu.Unlock() + f.comments = append(f.comments, fmt.Sprintf("%s/%s#%d: %s", owner, repo, number, body)) + return nil +} + +func (f *fakeForge) Unlabel(owner, repo string, number, label int) error { + f.mu.Lock() + defer f.mu.Unlock() + f.unlabels = append(f.unlabels, fmt.Sprintf("%s/%s#%d-%d", owner, repo, number, label)) + return nil +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 45dcaae3..da430dc2 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -69,6 +69,8 @@ type Holder struct { leaveRefused map[string]string // tried counts the owed effects Flush tried since this owner started, by how each ended (novox/hq issue 407). tried owedTries + // marks are the open pull requests read with a ready mark, by owner/repo#number (novox/hq issue 722, ready.go). + marks map[string]*marked } // owedTries is how many owed effects were tried since this owner started, and when the last was: three numbers @@ -120,6 +122,7 @@ func (h *Holder) init() { h.askTried = map[string]time.Time{} h.walkAsked = map[string]time.Time{} h.unchecked = map[string]unchecked{} + h.marks = map[string]*marked{} h.walks = map[string]seenWalk{} h.orphans = map[string]time.Time{} h.asking = map[string]bool{} @@ -996,6 +999,12 @@ func (h *Holder) Tick() { for _, ask := range h.leaveStopped() { ask() } + h.mu.Lock() + asks := h.readyAsks(h.Now()) + h.mu.Unlock() + for _, ask := range asks { + ask() + } } // plan works out, under the lock, what to ask; each ask applies its own answer. diff --git a/modules/mesh-delivery/cmd/mesh-delivery/main.go b/modules/mesh-delivery/cmd/mesh-delivery/main.go index be753778..cd24e09b 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/main.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/main.go @@ -76,8 +76,9 @@ func run(h *Holder, l *listening) { logf("[mesh-delivery] the controller's walks cannot be read: %v", err) } } - // Every open pull request, from the forge itself, every five minutes (novox/hq issue 347). - if now.Sub(looked) >= 5*time.Minute { + // Every open pull request, from the forge itself, every minute (novox/hq issues 347, 722): what says a + // head nobody checks, and the ready marks a merge is asked on. + if now.Sub(looked) >= time.Minute { looked = now h.SeeOpenPulls() } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/manifest_test.go b/modules/mesh-delivery/cmd/mesh-delivery/manifest_test.go index 6b4800f7..ade98c14 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/manifest_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/manifest_test.go @@ -65,7 +65,9 @@ func TestItClaimsTheDeliverySeatAndFollowsWhatItHandles(t *testing.T) { want = append(want, "seat:"+ControllerSeat+"."+v) } want = append(want, "gitea.gitea_note_append", "gitea.gitea_delivery_view", "gitea.gitea_commit_status", - "gitea.gitea_commit_statuses", "gitea.gitea_contains", "gitea.gitea_open_pulls") + "gitea.gitea_commit_statuses", "gitea.gitea_contains", "gitea.gitea_open_pulls", + // The merge on a ready mark (novox/hq issue 722): the merge tool, a comment, a label taken off. + "gitea.gitea_merge_pull_request", "gitea.gitea_add_comment", "gitea.gitea_api") if !reflect.DeepEqual(m.Invokes, want) { t.Fatalf("invokes %v, wanted %v", m.Invokes, want) } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/model.go b/modules/mesh-delivery/cmd/mesh-delivery/model.go index 1f25263b..ec3e2e1d 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/model.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/model.go @@ -110,6 +110,9 @@ type Delivery struct { LetGo time.Time `json:"let_go"` LetGoWhy string `json:"let_go_why,omitempty"` + // Ready is what this owner did with its pull request's ready mark (novox/hq issue 722, ready.go). + Ready *ReadyRun `json:"ready,omitempty"` + Transitions []Transition `json:"transitions,omitempty"` // Owed are the forge's and the bus's effects of its transitions not yet done: said only after the // delivery is kept, retried until done. diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ports.go b/modules/mesh-delivery/cmd/mesh-delivery/ports.go index a4727928..63fb608f 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ports.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ports.go @@ -91,6 +91,13 @@ type Forge interface { // OpenPulls is every open pull request, with what its base requires and what its head has (novox/hq issue // 347). Reads only. OpenPulls() ([]OpenPull, error) + // Merge asks the forge's merge tool to merge a pull request at the head named (gitea_merge_pull_request, novox/hq + // issue 722): the one path every merge takes. Its refusal is the error, in its words. + Merge(owner, repo string, number int, head string) error + // Comment adds a comment to a pull request. + Comment(owner, repo string, number int, body string) error + // Unlabel takes a label off a pull request, by the label's id. + Unlabel(owner, repo string, number, label int) error } // ForgeChecks is what the forge says of a commit's checks (the gitea module's gitea_commit_statuses). @@ -392,6 +399,19 @@ func (f toolForge) Statuses(owner, repo string, number int, commit, base string) return &c, nil } +func (f toolForge) Merge(owner, repo string, number int, head string) error { + return f.tool("gitea_merge_pull_request", map[string]any{"owner": owner, "repo": repo, "number": number, "head": head}) +} + +func (f toolForge) Comment(owner, repo string, number int, body string) error { + return f.tool("gitea_add_comment", map[string]any{"owner": owner, "repo": repo, "number": number, "body": body}) +} + +func (f toolForge) Unlabel(owner, repo string, number, label int) error { + return f.tool("gitea_api", map[string]any{"method": "DELETE", + "path": fmt.Sprintf("/repos/%s/%s/issues/%d/labels/%d", owner, repo, number, label)}) +} + func (f toolForge) OpenPulls() ([]OpenPull, error) { raw, err := f.ask("gitea.gitea_open_pulls", map[string]any{}) if err != nil { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready.go b/modules/mesh-delivery/cmd/mesh-delivery/ready.go new file mode 100644 index 00000000..f5488f25 --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready.go @@ -0,0 +1,323 @@ +package main + +import ( + "fmt" + "strings" + "time" +) + +// Merge on ready (novox/hq issue 722). +// +// The operator, 2026-10-14: "the reviewer task should merge the request or mark the MR as ready to merge in any case". +// Before, a reviewed pull request with green checks waited until a session called the forge's merge tool, and again +// after every check the tool asked because the trunk had moved. Now the independent reviewer marks the pull request +// ready at the head it approved — the forge label `mesh/ready` and a comment "ready at " (approving +// through the forge is the operator's) — and this owner merges it, through the forge's own merge tool +// (gitea_merge_pull_request), the one path every merge takes: +// +// - only at the head the mark names, and only while that is the pull request's head: a push after the mark clears +// it — the label taken off, and said on the pull request — and the new head needs a mark of its own; +// - only when its delivery is ready here: its check passed against its trunk's head. A verdict out of date is judged +// again by the merge tool, which asks the check against the trunk's head itself (novox/hq ADR 0323 §5); the merge +// is asked again once a minute, and lands on the passing verdict; +// - a group member only at its turn in the group's order, and only once the group's composed check passed for its +// members' heads as they are; +// - a check that failed merges nothing: it is said once on the pull request, and as a stalled line, which the +// controller raises as the condition `delivery..stalled`. So is a mark the merge tool refused for an hour. +// +// What this owner does with a mark is kept on the delivery (Ready), said in its log, and the merge's transitions name +// it as their author. + +// ReadyMark is a pull request's ready mark as the forge reads it (the gitea module's gitea_open_pulls): the head the +// newest "ready at" comment names, who wrote it and when, and the label's id. Head is empty when the label is on and +// no comment names a head. +type ReadyMark struct { + Head string `json:"head"` + By string `json:"by,omitempty"` + At string `json:"at,omitempty"` + LabelID int `json:"label_id"` +} + +// ReadyLabel is the label that marks a pull request ready. +const ReadyLabel = "mesh/ready" + +// readyTryEvery is how often a marked head's merge is asked while the merge tool refuses it: a check it asked takes +// minutes, and a refusal costs the forge a few reads. +const readyTryEvery = time.Minute + +// readyRefusedFor is how long the merge tool may refuse a marked head before it is said as stalled: longer than a +// check takes (the controller's checkTimeout is 45 minutes), so a check on its way is never said. +const readyRefusedFor = time.Hour + +// ReadyRun is what this owner did with a delivery's ready mark: kept on the delivery and shown whole by `show`. +type ReadyRun struct { + // Head, By and MarkedAt are the mark: the head it names, who marked it, when. + Head string `json:"head"` + By string `json:"by,omitempty"` + MarkedAt string `json:"marked_at,omitempty"` + // Seen is when this owner first read the mark. + Seen time.Time `json:"seen"` + // Waits is what the merge waits on, when it waits: a verdict, the group's turn or its composed check. + Waits string `json:"waits,omitempty"` + // Tries is how many times the merge was asked; LastTry when, and Answer what the merge tool last answered. + Tries int `json:"tries,omitempty"` + LastTry time.Time `json:"last_try"` + Answer string `json:"answer,omitempty"` + // RefusedSince is when the merge tool began refusing it without a break. + RefusedSince *time.Time `json:"refused_since,omitempty"` + // Merged is when the merge tool merged it. + Merged *time.Time `json:"merged,omitempty"` + // Failed is why it is not merged and will not be on this head: its check failed. Said once on the pull request. + Failed string `json:"failed,omitempty"` +} + +// mergedBy is who a merge this owner made on a ready mark is said to be by, in its transitions. +func (r *ReadyRun) mergedBy() string { + if r == nil || r.Merged == nil { + return "" + } + by := byOwner + ", on the ready mark at " + shortOf(r.Head, 12) + if r.By != "" { + by += " by " + r.By + } + return by +} + +// marked is a pull request read with a ready mark. +type marked struct { + pull OpenPull + // cleared is when the mark was cleared, the label taken off: not cleared again within readyTryEvery. + cleared time.Time +} + +// pullKey is a pull request as a key: owner/repo#number. +func pullKey(p OpenPull) string { + return strings.ToLower(p.Owner+"/"+p.Repo) + fmt.Sprintf("#%d", p.Number) +} + +// seeMarks keeps the open pull requests that carry a ready mark; under the lock. A pull request no longer open or +// no longer marked drops out. +func (h *Holder) seeMarks(open []OpenPull) { + seen := map[string]*marked{} + for _, p := range open { + if p.Ready == nil || p.HeadSHA == "" { + continue + } + k := pullKey(p) + m := &marked{pull: p} + if was := h.marks[k]; was != nil && was.pull.Ready != nil && was.pull.Ready.Head == p.Ready.Head && + was.pull.HeadSHA == p.HeadSHA { + m.cleared = was.cleared + } + seen[k] = m + } + h.marks = seen +} + +// sameFull is whether a mark names a head: the whole sha, or — read from an older forge module — at least twelve +// characters of it. +func sameFull(mark, head string) bool { + mark, head = strings.ToLower(mark), strings.ToLower(head) + return len(mark) >= 12 && strings.HasPrefix(head, mark) +} + +// readyAsks works out, under the lock, what each ready mark needs: a mark cleared, a merge asked, a failure said. +// The asking happens outside the lock. +func (h *Holder) readyAsks(now time.Time) []func() { + var asks []func() + for k, m := range h.marks { + p, mark := m.pull, *m.pull.Ready + repository := strings.ToLower(p.Owner + "/" + p.Repo) + if !sameFull(mark.Head, p.HeadSHA) { + if !m.cleared.IsZero() && now.Sub(m.cleared) < readyTryEvery { + continue + } + m.cleared = now + asks = append(asks, h.clearMark(k, p, mark)) + continue + } + d := h.deliveries[IDOf(repository, p.HeadSHA)] + if d == nil || d.State.Final() || d.MergedAs != "" { + continue // not announced yet, or past its merge + } + if d.Ready == nil || d.Ready.Head != mark.Head { + d.Ready = &ReadyRun{Head: mark.Head, By: mark.By, MarkedAt: mark.At, Seen: now} + h.Logf("[mesh-delivery] %s is marked ready at %s by %s: merged once its check passes against its trunk's head", + d.ID, shortOf(mark.Head, 12), orNone(mark.By)) + h.keep(d) + } + r := d.Ready + if r.Merged != nil { + continue + } + if d.State == Rejected { + if r.Failed == "" { + r.Failed = "its check failed" + if d.Check != nil { + r.Failed += ": " + oneLine(d.Check.Summary) + if j := d.Check.Judged(); j != "" { + r.Failed += " (" + j + ")" + } + } + r.Waits = "" + h.Logf("[mesh-delivery] %s is marked ready and not merged: %s", d.ID, r.Failed) + h.keep(d) + asks = append(asks, h.sayOn(d, fmt.Sprintf("mesh-delivery: **not merged.** This pull request is marked "+ + "ready at %s, and %s. A new head, marked ready once reviewed, or a recheck that passes, merges it.", + mark.Head, r.Failed))) + } + continue + } + r.Failed = "" + waits := h.readyWaits(d) + if waits != r.Waits { + r.Waits = waits + if waits != "" { + h.Logf("[mesh-delivery] %s is marked ready and waits: %s", d.ID, waits) + } + h.keep(d) + } + if waits != "" || (!r.LastTry.IsZero() && now.Sub(r.LastTry) < readyTryEvery) { + continue + } + r.Tries++ + r.LastTry = now + h.keep(d) + asks = append(asks, h.askMerge(d.ID, p, mark.Head)) + } + return asks +} + +// readyWaits is what a marked delivery's merge waits on; empty when it is asked now. A ready delivery is asked; so is +// a proposed one whose verdict went out of date and whose check nobody asked again — the merge tool asks it. +func (h *Holder) readyWaits(d *Delivery) string { + switch { + case d.State == Ready: + case d.State == Proposed && d.outOfDate() != "" && d.AskedAgain == nil: + case d.State == Proposed && d.AskedAgain != nil: + return "its check against the trunk's head is asked and has not answered" + default: + return "its check has not passed yet (" + string(d.State) + ")" + } + if d.Group == "" { + return "" + } + return h.groupTurnToMerge(d) +} + +// groupTurnToMerge is why a group member may not merge yet; empty at its turn: the group's composed check passed for +// its members' heads as they are and is not out of date, and every member before it in the group's order merged. +func (h *Holder) groupTurnToMerge(d *Delivery) string { + g := h.groups[d.Group] + if g == nil { + return "" + } + members := h.membersOf(g) + if state, why := GroupState(g, members); state != "ready" && state != "delivering" { + return fmt.Sprintf("its group %s is %s: %s", g.ID, state, why) + } + if c := g.Check; c == nil || (c.Verdict != "pass" && c.Verdict != "warning") || c.OutOfDate != "" || + !sameMembers(c.Commits, commitsOf(members)) { + return "its group " + g.ID + "'s composed check has not passed for its members' heads" + } + if !containsString(g.Order, d.ID) { + return "its group " + g.ID + "'s order is not worked out for it yet" + } + for _, id := range g.Order { + if id == d.ID { + return "" + } + if o := h.deliveries[id]; o != nil && o.MergedAs == "" && !o.State.Final() { + return "its group " + g.ID + "'s order merges " + id + " first" + } + } + return "" +} + +// askMerge asks the forge's merge tool to merge a marked head, and keeps its answer. +func (h *Holder) askMerge(id string, p OpenPull, head string) func() { + return func() { + err := h.Forge.Merge(p.Owner, p.Repo, p.Number, head) + h.mu.Lock() + defer h.mu.Unlock() + d := h.deliveries[id] + if d == nil || d.Ready == nil { + return + } + r, now := d.Ready, h.Now() + if err == nil { + r.Merged, r.Answer, r.RefusedSince = &now, "merged", nil + h.Logf("[mesh-delivery] %s merged on its ready mark at %s, its check passed against its trunk's head", id, + shortOf(head, 12)) + h.keep(d) + return + } + answer := oneLine(err.Error()) + if r.RefusedSince == nil { + r.RefusedSince = &now + } + if answer != r.Answer { + h.Logf("[mesh-delivery] %s is marked ready, and the merge tool did not merge it (asked again in %s): %s", id, + readyTryEvery, answer) + } + r.Answer = answer + h.keep(d) + } +} + +// clearMark takes a mark off a pull request whose head moved after it, and says so on it. +func (h *Holder) clearMark(k string, p OpenPull, mark ReadyMark) func() { + return func() { + err := h.Forge.Unlabel(p.Owner, p.Repo, p.Number, mark.LabelID) + if err == nil { + err = h.Forge.Comment(p.Owner, p.Repo, p.Number, fmt.Sprintf("mesh-delivery: the ready mark at %s is cleared: "+ + "the head is now %s, pushed after it. Mark the new head ready once it is reviewed.", mark.Head, p.HeadSHA)) + } + if err != nil { + h.Logf("[mesh-delivery] %s's ready mark at %s names an older head than %s, and could not be cleared yet: %v", + k, shortOf(mark.Head, 12), shortOf(p.HeadSHA, 12), err) + return + } + h.Logf("[mesh-delivery] %s's ready mark at %s cleared: its head is now %s", k, shortOf(mark.Head, 12), + shortOf(p.HeadSHA, 12)) + h.mu.Lock() + delete(h.marks, k) + h.mu.Unlock() + } +} + +// sayOn comments on a delivery's pull request. +func (h *Holder) sayOn(d *Delivery, body string) func() { + owner, repo, number, id := d.Owner(), d.Repo(), d.Number, d.ID + return func() { + if err := h.Forge.Comment(owner, repo, number, body); err != nil { + h.Logf("[mesh-delivery] %s: could not say on its pull request: %v", id, err) + } + } +} + +// readyLines are the marked heads that will not merge: a check that failed at once, a merge refused past its bound; +// as stalled lines, which the controller raises as conditions. Under the lock. +func (h *Holder) readyLines(now time.Time) []StalledLine { + var out []StalledLine + for _, d := range h.deliveries { + r := d.Ready + if r == nil || r.Merged != nil || d.MergedAs != "" || d.State.Final() { + continue + } + switch { + case r.Failed != "" && d.State == Rejected: + out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(), + Bound: "0s", H2: "none: a new head marked ready, or a recheck that passes — the operator's", + Says: fmt.Sprintf("%s#%d is marked ready at %s and not merged: %s", d.Repository, d.Number, + shortOf(r.Head, 12), r.Failed)}) + case r.RefusedSince != nil && now.Sub(*r.RefusedSince) >= readyRefusedFor: + out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, + For: now.Sub(*r.RefusedSince).Round(time.Second).String(), Bound: readyRefusedFor.String(), + H2: "none: the merge is asked again each minute — the operator's", + Says: fmt.Sprintf("%s#%d is marked ready at %s, and the merge tool has refused it since %s: %s", d.Repository, + d.Number, shortOf(r.Head, 12), r.RefusedSince.UTC().Format(time.RFC3339), r.Answer)}) + } + } + return out +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go b/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go new file mode 100644 index 00000000..e818e9b1 --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go @@ -0,0 +1,199 @@ +package main + +import ( + "errors" + "strings" + "testing" + "time" +) + +// **A pull request marked ready is merged by this owner** (novox/hq issue 722): before, a reviewed pull request with +// green checks waited until a session called the merge tool, and again after every check the tool asked because the +// trunk had moved. Each test below fails on a holder without the ready mark: nothing merged it. + +const ( + readyHead = "1111111111111111111111111111111111111111" + pushed = "2222222222222222222222222222222222222222" +) + +// markedPull is an open pull request as gitea_open_pulls reads it, labelled mesh/ready at the head named. +func markedPull(repo string, number int, head, markedAt string) OpenPull { + owner, name, _ := strings.Cut(repo, "/") + return OpenPull{Owner: owner, Repo: name, Number: number, Base: "main", HeadSHA: head, Labels: []string{ReadyLabel}, + Ready: &ReadyMark{Head: markedAt, By: "reviewer", At: "2026-10-14T19:00:00Z", LabelID: 31}} +} + +func judgedAt(repo string, number int, sha, gate, trunk string) CheckedEvent { + c := verdict(repo, number, sha, gate) + c.Trunk = trunk + return c +} + +func TestAReadyPullRequestMarkedAtItsHeadIsMergedThroughTheMergeTool(t *testing.T) { + w := newWorld(t) + id := IDOf("novox/app", readyHead) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + w.settleAll() + if len(w.forge.merges) != 0 { + t.Fatalf("an unmarked pull request was merged: %v", w.forge.merges) + } + w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} + w.h.SeeOpenPulls() + w.settleAll() + if len(w.forge.merges) != 1 || w.forge.merges[0] != "novox/app#7@"+readyHead { + t.Fatalf("a ready, marked pull request was merged %v; wanted once, at the marked head", w.forge.merges) + } + d := w.delivery(id) + if d.Ready == nil || d.Ready.Merged == nil || d.Ready.By != "reviewer" || d.Ready.Tries != 1 { + t.Fatalf("what was done with the mark is not kept: %+v", d.Ready) + } + // Its merge's transition names who merged it. + w.h.PullMerged(merged("novox/app", 7, readyHead, "m7m7m7m7m7m7")) + w.h.WalkMoved(w.ctl.put(aWalk("plan-7", "novox/app", "m7m7m7m7m7m7", true))) + w.settleAll() + d = w.delivery(id) + var last Transition + for _, tr := range d.Transitions { + if tr.Event == EvMerged { + last = tr + } + } + if !strings.Contains(last.By, "ready mark at "+readyHead[:12]+" by reviewer") { + t.Fatalf("the merge's transition does not name the mark: %+v", last) + } + if len(w.forge.merges) != 1 { + t.Fatalf("merged again: %v", w.forge.merges) + } +} + +func TestAPushAfterTheMarkClearsItAndNothingIsMerged(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.h.PullUpdated(pr("novox/app", 7, pushed, "feat/x", "src/a.go")) + w.h.Checked(judgedAt("novox/app", 7, pushed, "pass", "t1t1t1t1t1t1")) + w.forge.open = []OpenPull{markedPull("novox/app", 7, pushed, readyHead)} + w.h.SeeOpenPulls() + w.settleAll() + if len(w.forge.merges) != 0 { + t.Fatalf("a head pushed after the mark was merged: %v", w.forge.merges) + } + if len(w.forge.unlabels) != 1 || w.forge.unlabels[0] != "novox/app#7-31" { + t.Fatalf("the stale mark's label was not taken off: %v", w.forge.unlabels) + } + if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "cleared") || + !strings.Contains(w.forge.comments[0], pushed) { + t.Fatalf("the cleared mark is not said on the pull request: %v", w.forge.comments) + } +} + +func TestAMovedTrunkIsJudgedAgainByTheMergeToolAndMergedOnItsVerdict(t *testing.T) { + w := newWorld(t) + id := IDOf("novox/app", readyHead) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + // Another merge moves the trunk: its verdict is out of date, and it goes back to proposed. + w.h.PullMerged(merged("novox/app", 9, "9999999999999999", "t2t2t2t2t2t2")) + if w.state(id) != Proposed { + t.Fatalf("out of date, it is %s", w.state(id)) + } + // The merge tool asks the check against the trunk's head and refuses: nothing merged. + w.forge.refuse = errors.New("the forge refused gitea_merge_pull_request: The check of head merged into trunk was asked") + w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} + w.h.SeeOpenPulls() + w.settleAll() + if len(w.forge.merges) != 1 { + t.Fatalf("the merge tool was not asked once to judge it again: %v", w.forge.merges) + } + // The tool's ask is the head announced again; it waits for that verdict, asking nothing. + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.later(5 * time.Minute) + w.settleAll() + if len(w.forge.merges) != 1 || !strings.Contains(w.delivery(id).Ready.Waits, "asked") { + t.Fatalf("asked again while its check was on its way: %v (%+v)", w.forge.merges, w.delivery(id).Ready) + } + // It passes against the trunk's head: merged, without anyone asking. + w.forge.refuse = nil + w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t2t2t2t2t2t2")) + w.settleAll() + if w.state(id) != Ready || len(w.forge.merges) != 2 || w.delivery(id).Ready.Merged == nil { + t.Fatalf("on its passing verdict it was not merged: %s %v %+v", w.state(id), w.forge.merges, w.delivery(id).Ready) + } +} + +func TestAMarkedHeadWhoseCheckFailedIsNotMergedAndIsSaid(t *testing.T) { + w := newWorld(t) + id := IDOf("novox/app", readyHead) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.h.Checked(judgedAt("novox/app", 7, readyHead, "fail", "t1t1t1t1t1t1")) + w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} + w.h.SeeOpenPulls() + w.settleAll() + w.later(10 * time.Minute) + w.h.SeeOpenPulls() + w.settleAll() + if len(w.forge.merges) != 0 { + t.Fatalf("a head whose check failed was merged: %v", w.forge.merges) + } + if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "not merged") { + t.Fatalf("the failure is not said once on the pull request: %v", w.forge.comments) + } + lines := w.h.Stalled() + if len(lines) != 1 || lines[0].ID != id || lines[0].State != Rejected || !strings.Contains(lines[0].Says, "marked ready") { + t.Fatalf("the failure is not a stalled line, which the controller raises as a condition: %+v", lines) + } +} + +func TestAMarkRefusedForAnHourIsSaid(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + w.forge.refuse = errors.New("the forge refused gitea_merge_pull_request: mesh/repo-check is pending") + w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} + w.h.SeeOpenPulls() + w.settleAll() + w.later(30 * time.Second) + w.settleAll() + if len(w.forge.merges) != 1 { + t.Fatalf("asked again within the minute: %v", w.forge.merges) + } + w.later(readyRefusedFor) + w.settleAll() + if len(w.forge.merges) != 2 { + t.Fatalf("not asked again after a minute: %v", w.forge.merges) + } + if lines := w.h.Stalled(); len(lines) != 1 || !strings.Contains(lines[0].Says, "refused it since") { + t.Fatalf("a mark refused for an hour is not said: %+v", lines) + } +} + +func TestAGroupMergesItsMarkedMembersInOrderOnlyOnceComposed(t *testing.T) { + w := newWorld(t) + const ctlHead, catHead = "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1", "d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2" + ctl, cat := IDOf("novox/mesh-controller", ctlHead), IDOf("novox/mesh-catalog", catHead) + w.ctl.order = func(ms []Member) Order { return Order{Order: []string{ctl, cat}} } + w.h.PullUpdated(pr("novox/mesh-controller", 1, ctlHead, "feat/x", "internal/a.go")) + w.h.PullUpdated(pr("novox/mesh-catalog", 2, catHead, "feat/x", "modules/app/module.json")) + w.h.Checked(judgedAt("novox/mesh-controller", 1, ctlHead, "pass", "t1t1t1t1t1t1")) + w.h.Checked(judgedAt("novox/mesh-catalog", 2, catHead, "pass", "t3t3t3t3t3t3")) + w.forge.open = []OpenPull{markedPull("novox/mesh-controller", 1, ctlHead, ctlHead), + markedPull("novox/mesh-catalog", 2, catHead, catHead)} + w.h.SeeOpenPulls() + w.settleAll() + if len(w.forge.merges) != 0 { + t.Fatalf("a member merged before its group's composed check passed: %v", w.forge.merges) + } + w.h.Checked(CheckedEvent{Group: "feat/x", ID: "check-1", Verdict: "pass", Summary: "both compose"}) + w.settleAll() + if len(w.forge.merges) != 1 || w.forge.merges[0] != "novox/mesh-controller#1@"+ctlHead { + t.Fatalf("composed, the first member in the group's order was not merged alone: %v", w.forge.merges) + } + if waits := w.delivery(cat).Ready.Waits; !strings.Contains(waits, "merges "+ctl+" first") { + t.Fatalf("the second member does not say it waits its turn: %q", waits) + } + w.h.PullMerged(merged("novox/mesh-controller", 1, ctlHead, "m1m1m1m1m1m1")) + w.settleAll() + if len(w.forge.merges) != 2 || w.forge.merges[1] != "novox/mesh-catalog#2@"+catHead { + t.Fatalf("after the first member merged, the next was not: %v", w.forge.merges) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/table.go b/modules/mesh-delivery/cmd/mesh-delivery/table.go index 687f1b2c..9f9a6749 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/table.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/table.go @@ -498,7 +498,12 @@ func Apply(d *Delivery, ev Event, act bool, f Facts) (Transition, error) { if why == "" { why = r.Guard } - t := Transition{At: f.Now, From: d.State, To: r.To, Event: ev, Why: why, By: f.By} + by := f.By + if by == "" && (ev == EvMerged || ev == EvDone) && d.MergedAs != "" { + // A merge this owner made on a ready mark names it as its author (novox/hq issue 722). + by = d.Ready.mergedBy() + } + t := Transition{At: f.Now, From: d.State, To: r.To, Event: ev, Why: why, By: by} d.State, d.Since = r.To, f.Now d.Transitions = append(d.Transitions, t) if len(d.Transitions) > keptTransitions { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go b/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go index 2947b823..14cc5d20 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go @@ -72,6 +72,9 @@ type OpenPull struct { // than that says none: the head is then read by Statuses alone, as before, and a pending check is not told // from an answered one. Checks map[string]HeadCheck `json:"checks,omitempty"` + // Labels are its labels by name, and Ready its ready mark, when it carries one (novox/hq issue 722). + Labels []string `json:"labels,omitempty"` + Ready *ReadyMark `json:"ready,omitempty"` } // HeadCheck is one status on a head as the forge keeps it: pending, success, failure, error or warning. @@ -201,6 +204,7 @@ func (h *Holder) SeeOpenPulls() { seen[id] = u } h.unchecked = seen + h.seeMarks(open) } // started is whether the head's check started, rather than was never asked: it has any merge check, or a delivery diff --git a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go index bcaa8e57..b95aacd2 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go @@ -249,6 +249,7 @@ func (h *Holder) Stalled() []StalledLine { } out = append(out, h.withChecks(h.uncheckedLines(now))...) out = append(out, h.orphanLines(now)...) + out = append(out, h.readyLines(now)...) sort.Slice(out, func(i, j int) bool { return out[i].ID < out[j].ID }) return out } diff --git a/modules/mesh-delivery/module.json b/modules/mesh-delivery/module.json index 681165fe..4c486145 100644 --- a/modules/mesh-delivery/module.json +++ b/modules/mesh-delivery/module.json @@ -47,7 +47,10 @@ "gitea.gitea_commit_status", "gitea.gitea_commit_statuses", "gitea.gitea_contains", - "gitea.gitea_open_pulls" + "gitea.gitea_open_pulls", + "gitea.gitea_merge_pull_request", + "gitea.gitea_add_comment", + "gitea.gitea_api" ], "state": [ "deliveries", -- 2.54.0 From 5b20e419ec1f5b3c465dc82f65fa70646529371e Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 21:45:39 +0200 Subject: [PATCH 2/3] Count a ready mark only from a listed account, after the last push and the label (issue 722) Review of #303: anyone who can comment could turn a stale label into a mark, and a long thread hid the newest comment. The forge now reads the pull request's whole timeline and takes a "ready at" comment only from the accounts in the trusted setting ready-authors (default the mesh's own forge account and the operator's), written after the label was last added and after the last push; any other author is ignored and said in the log. --- modules/gitea/client.ts | 95 +++++++++++++++++++++--- modules/gitea/module.json | 18 ++++- modules/gitea/test/announcing.test.ts | 102 +++++++++++++++++++++----- modules/gitea/tools/index.ts | 6 +- 4 files changed, 191 insertions(+), 30 deletions(-) diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index ee0572ee..55bba9dc 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -122,13 +122,71 @@ export interface ReadyMark { label_id: number; } -/** The ready mark a pull request's comments make, oldest first as the forge lists them: the newest that names a head. */ -export function readyMarkOf(labelID: number, comments: GiteaComment[]): ReadyMark { - for (let i = comments.length - 1; i >= 0; i--) { - const m = READY_AT.exec(comments[i].body); - if (m) return { head: m[1].toLowerCase(), by: comments[i].user, at: comments[i].created_at, label_id: labelID }; +/** **The accounts whose "ready at" comment counts** (novox/hq issue 722): the trusted setting ready-authors, which + * the mesh writes to the file MESH_GITEA_READY_AUTHORS_FILE names as a line `authors= …`. A file + * that cannot be read leaves the mesh's own forge account alone, said; never an empty list read as everyone. */ +export function readyAuthors(file = process.env.MESH_GITEA_READY_AUTHORS_FILE, own = process.env.MESH_GITEA_ADMIN_USER, + read: (f: string) => string = (f) => readFileSync(f, "utf8")): string[] { + const fallback = own ? [own] : []; + if (!file) return fallback; + try { + const line = read(file).split("\n").find((l) => l.startsWith("authors=")); + const authors = (line ?? "").slice("authors=".length).split(/[\s,]+/).filter(Boolean); + if (authors.length) return authors; + console.error(`[gitea] ${file} names no account that may mark a pull request ready; only ${fallback.join(", ") || "none"} may`); + } catch (err) { + console.error(`[gitea] ${file} cannot be read (${err instanceof Error ? err.message : err}); only ` + + `${fallback.join(", ") || "none"} may mark a pull request ready`); } - return { head: "", label_id: labelID }; + return fallback; +} + +/** One event of an issue's or pull request's timeline, as the forge keeps it, oldest first: a comment, a push to a + * pull request's branch (`pull_push`), a label added (body "1") or taken off (body ""), and the rest. */ +export interface TimelineEvent { + id: number; + type: string; + body: string; + created_at?: string; + user?: string; + label?: { id: number; name: string }; +} + +/** The comments that wrote "ready at" without counting, by id: each said once in the log (novox/hq issue 722). */ +const ignoredSaid = new Set(); + +/** **The ready mark a pull request's timeline makes** (novox/hq issue 722), read in the forge's order — every event is + * one row of one table, numbered as it happened, so the order is exact where times share a second. A "ready at" + * comment counts only when its author is one of the accounts given, and only when it was written after the label + * mesh/ready was last added and after the last push to the pull request's branch: a push or the label taken off + * undoes every mark before it, so a mark older than the latest push never counts, even with the label still on. A + * comment by any other account is ignored, and said once. */ +export function readyMarkOf(labelID: number, events: TimelineEvent[], authors: string[], + say: (line: string) => void = (l) => console.error(l)): ReadyMark { + const listed = new Set(authors.map((a) => a.toLowerCase())); + let labelled = false; + let mark: ReadyMark | undefined; + for (const e of [...events].sort((x, y) => x.id - y.id)) { + if (e.type === "pull_push") { + mark = undefined; + } else if (e.type === "label" && e.label?.name === READY_LABEL) { + labelled = e.body === "1"; + mark = undefined; + } else if (e.type === "comment") { + const m = READY_AT.exec(e.body); + if (!m) continue; + if (!listed.has(String(e.user ?? "").toLowerCase())) { + if (!ignoredSaid.has(e.id)) { + ignoredSaid.add(e.id); + say(`[gitea] a "ready at ${m[1].slice(0, 12)}" comment (${e.id}) by ${e.user ?? "an unknown account"} is ignored: ` + + `only ${authors.join(", ") || "no account"} may mark a pull request ready (novox/hq issue 722)`); + } + continue; + } + if (labelled) mark = { head: m[1].toLowerCase(), by: e.user, at: e.created_at, label_id: labelID }; + } + } + return mark ?? { head: "", label_id: labelID }; } /** One status on a head, the newest of its context: its state, what it said, and when it was set. */ @@ -384,7 +442,7 @@ export class GiteaClient { * requires and the contexts its head has a status of, in any state. What mesh-delivery reads to say a pull * request the mesh was never asked to check; asked of the forge, never of what the announcer remembers, so a * fault in the announcer cannot hide itself. Reads only. */ - async openPulls(): Promise { + async openPulls(readyAuthors: string[] = []): Promise { const out: OpenPull[] = []; const rules = new Map(); for (const found of await this.searchOpenPulls()) { @@ -395,9 +453,10 @@ export class GiteaClient { const rule = rules.get(full)?.find((p) => (p.rule_name ?? p.branch_name) === (pull.base || "main")); const checks = await this.headChecks(found.owner, found.name, pull.head_sha); const labels = found.labels ?? []; - // The ready mark (novox/hq issue 722): its comments are read only for a pull request that carries the label. + // The ready mark (novox/hq issue 722): its timeline is read, whole, only for a pull request that carries the label. const label = labels.find((l) => l.name === READY_LABEL); - const ready = label ? readyMarkOf(label.id, await this.listComments(found.owner, found.name, pull.number)) : undefined; + const ready = label ? readyMarkOf(label.id, await this.timeline(found.owner, found.name, pull.number), readyAuthors) + : undefined; out.push({ owner: found.owner, repo: found.name, number: pull.number, base: pull.base ?? "", head_sha: pull.head_sha, html_url: pull.html_url, required: rule?.enable_status_check ? (rule.status_check_contexts ?? []) : [], statuses: Object.keys(checks), checks, labels: labels.map((l) => l.name), ...(ready ? { ready } : {}) }); @@ -673,6 +732,24 @@ export class GiteaClient { return this.requestText(`${repoPath(owner, repo)}/pulls/${index}.diff`); } + /** **Every event of an issue's or pull request's timeline**, oldest first, page by page (novox/hq issue 722): a long + * thread hides nothing. Bounded at 100 pages of 50; a longer one is said, and its first 5000 events read. */ + async timeline(owner: string, repo: string, index: number): Promise { + indexOf(index); + const out: TimelineEvent[] = []; + for (let page = 1; page <= 100; page++) { + const raw = await this.request(`${repoPath(owner, repo)}/issues/${index}/timeline?page=${page}&limit=50`); + for (const e of raw ?? []) { + out.push({ id: Number(e?.id ?? 0), type: String(e?.type ?? ""), body: String(e?.body ?? ""), + created_at: e?.created_at, user: e?.user?.login, + ...(e?.label ? { label: { id: Number(e.label.id ?? 0), name: String(e.label.name ?? "") } } : {}) }); + } + if (!raw || raw.length < 50) return out; + } + console.error(`[gitea] ${owner}/${repo}#${index} has more than 5000 timeline events; only the first 5000 are read`); + return out; + } + /** Every comment on an issue or pull request, oldest first. */ async listComments(owner: string, repo: string, index: number): Promise { indexOf(index); diff --git a/modules/gitea/module.json b/modules/gitea/module.json index c97134a5..c3290a80 100644 --- a/modules/gitea/module.json +++ b/modules/gitea/module.json @@ -109,6 +109,13 @@ } } }, + "settings": { + "ready-authors": { + "kind": "preference", + "default": "mesh-admin jschoubben", + "why": "the forge accounts whose comment \"ready at \" marks a pull request ready for mesh-delivery to merge, separated by spaces: the mesh's own forge account and the operator's (novox/hq issue 722). A comment by any other account is ignored and said in the forge's log. Trusted: who may have a change merged is the operator's word" + } + }, "resources": [ { "id": "mesh-state", @@ -122,6 +129,14 @@ "path": "${dir:mesh-state}/state", "mode": "0700" }, + { + "id": "ready-authors", + "type": "file", + "path": "${dir:mesh-state}/ready-authors", + "mode": "0600", + "trusted": true, + "content": "# The accounts whose \"ready at \" comment marks a pull request ready (novox/hq issue 722), from the setting\n# ready-authors, written by the mesh. Replaced on every push; change the setting, never this file.\nauthors=${setting:ready-authors}\n" + }, { "id": "state", "type": "directory", @@ -246,7 +261,8 @@ "MESH_GITEA_ADMIN_USER": "mesh-admin", "MESH_GITEA_ADMIN_PASSWORD_FILE": "${dir:state}/admin.secret", "MESH_GITEA_STATE_DIR": "${dir:runtime-state}", - "MESH_RECEIVES": "${dir:grants}/npm.json" + "MESH_RECEIVES": "${dir:grants}/npm.json", + "MESH_GITEA_READY_AUTHORS_FILE": "${dir:mesh-state}/ready-authors" } }, { diff --git a/modules/gitea/test/announcing.test.ts b/modules/gitea/test/announcing.test.ts index 54daea78..01ce7a05 100644 --- a/modules/gitea/test/announcing.test.ts +++ b/modules/gitea/test/announcing.test.ts @@ -139,10 +139,15 @@ test("a merged pull request whose branch was deleted is announced with its branc }); // **A pull request marked ready says the head it is marked at** (novox/hq issue 722): the label mesh/ready and the -// newest comment "ready at ". mesh-delivery merges on it; without the label no comment is read. -test("a pull request's ready mark is read: the label and the head the newest ready comment names", async () => { - const { GiteaClient } = await import("../client.ts"); - const head = "c".repeat(40), older = "d".repeat(40); +// newest comment "ready at " by a listed account, written after the label was added and after the last +// push. Read from the pull request's whole timeline, page by page; without the label nothing is read. +const head = "c".repeat(40), older = "d".repeat(40); +const ev = (id: number, type: string, body: string, user = "mesh-admin", label?: { id: number; name: string }) => + ({ id, type, body, user: { login: user }, created_at: `2026-10-14T10:${String(id % 60).padStart(2, "0")}:00Z`, + ...(label ? { label } : {}) }); +const readyLabel = { id: 31, name: "mesh/ready" }; + +function forgeWith(timeline: unknown[]) { const asked: string[] = []; const server = createServer((req, res) => { const url = req.url ?? ""; @@ -161,28 +166,91 @@ test("a pull request's ready mark is read: the label and the head the newest rea if (url.startsWith("/api/v1/repos/novox/hq/pulls/8")) { return send({ number: 8, state: "open", base: { ref: "main" }, head: { ref: "y", sha: older }, html_url: "h" }); } - if (url.startsWith("/api/v1/repos/novox/hq/issues/7/comments")) { - return send([ - { id: 1, user: { login: "mesh" }, body: `ready at ${older}`, created_at: "2026-10-14T10:00:00Z" }, - { id: 2, user: { login: "mesh" }, body: `Reviewed: approve.\nready at ${head}\n`, created_at: "2026-10-14T11:00:00Z" }, - { id: 3, user: { login: "jochen" }, body: "thanks", created_at: "2026-10-14T12:00:00Z" }, - ]); + const t = /\/issues\/7\/timeline\?page=(\d+)&limit=50/.exec(url); + if (t) { + const page = Number(t[1]); + return send(timeline.slice((page - 1) * 50, page * 50)); } if (url.includes("/branch_protections")) return send([]); if (url.includes("/status")) return send({ statuses: [] }); res.statusCode = 404; res.end("{}"); }); + return { server, asked }; +} + +async function readOpen(timeline: unknown[], authors = ["mesh-admin", "jschoubben"]) { + const { GiteaClient } = await import("../client.ts"); + const { server, asked } = forgeWith(timeline); await new Promise((r) => server.listen(0, r)); - const client = new GiteaClient(`http://127.0.0.1:${(server.address() as any).port}`, "t"); try { - const open = await client.openPulls(); - assert.deepEqual(open[0].labels, ["mesh/ready", "bug"]); - assert.deepEqual(open[0].ready, { head, by: "mesh", at: "2026-10-14T11:00:00Z", label_id: 31 }, - "the newest comment that names a head"); - assert.equal(open[1].ready, undefined, "no label, no mark"); - assert.ok(!asked.some((u) => u.includes("/issues/8/comments")), "no comment read without the label"); + const client = new GiteaClient(`http://127.0.0.1:${(server.address() as any).port}`, "t"); + return { open: await client.openPulls(authors), asked }; } finally { server.close(); } +} + +test("a pull request's ready mark is read: the label and the head the newest listed ready comment names", async () => { + const { open, asked } = await readOpen([ + ev(1, "pull_push", JSON.stringify({ commit_ids: [head] })), + ev(2, "label", "1", "mesh-admin", readyLabel), + ev(3, "comment", `ready at ${older}`), + ev(4, "comment", `Reviewed: approve.\nready at ${head}\n`), + ev(5, "comment", "thanks", "jschoubben"), + ]); + assert.deepEqual(open[0].labels, ["mesh/ready", "bug"]); + assert.deepEqual(open[0].ready, { head, by: "mesh-admin", at: "2026-10-14T10:04:00Z", label_id: 31 }); + assert.equal(open[1].ready, undefined, "no label, no mark"); + assert.ok(!asked.some((u) => u.includes("/issues/8/")), "no timeline read without the label"); +}); + +test("a ready comment by an account not listed is ignored, and said", async () => { + const { readyMarkOf } = await import("../client.ts"); + const said: string[] = []; + const mark = readyMarkOf(31, [ + ev(1, "pull_push", "{}") as any, ev(2, "label", "1", "mesh-admin", readyLabel) as any, + { ...ev(3, "comment", `ready at ${head}`, "outsider"), user: "outsider" }, + ].map((e: any) => ({ ...e, user: typeof e.user === "string" ? e.user : e.user.login })), ["mesh-admin"], (l) => said.push(l)); + assert.equal(mark.head, "", "an outsider's mark counts"); + assert.equal(said.length, 1); + assert.match(said[0], /by outsider is ignored/); +}); + +test("a ready comment older than the latest push, or than the label, never counts", async () => { + const pushedAfter = await readOpen([ + ev(1, "label", "1", "mesh-admin", readyLabel), + ev(2, "comment", `ready at ${head}`), + ev(3, "pull_push", JSON.stringify({ commit_ids: [head] })), + ]); + assert.equal(pushedAfter.open[0].ready?.head, "", "a mark written before the latest push counts, the label still on"); + const beforeLabel = await readOpen([ + ev(1, "pull_push", "{}"), + ev(2, "comment", `ready at ${head}`), + ev(3, "label", "1", "mesh-admin", readyLabel), + ]); + assert.equal(beforeLabel.open[0].ready?.head, "", "a mark written before the label was added counts"); + const takenOff = await readOpen([ + ev(1, "label", "1", "mesh-admin", readyLabel), + ev(2, "comment", `ready at ${head}`), + ev(3, "label", "", "mesh-admin", readyLabel), + ]); + assert.equal(takenOff.open[0].ready?.head, "", "a mark whose label was taken off since counts"); +}); + +test("a long thread hides nothing: every page of the timeline is read", async () => { + const events: unknown[] = [ev(1, "pull_push", "{}"), ev(2, "label", "1", "mesh-admin", readyLabel), + ev(3, "comment", `ready at ${older}`)]; + for (let id = 4; id < 130; id++) events.push(ev(id, "comment", "a long discussion", "jschoubben")); + events.push(ev(130, "comment", `ready at ${head}`, "jschoubben")); + const { open, asked } = await readOpen(events); + assert.equal(open[0].ready?.head, head, "the newest mark, on the third page, is not read"); + assert.equal(asked.filter((u) => u.includes("/issues/7/timeline")).length, 3); +}); + +test("the accounts that may mark are the setting's, and only the mesh's own when it cannot be read", async () => { + const { readyAuthors } = await import("../client.ts"); + assert.deepEqual(readyAuthors("f", "mesh-admin", () => "# x\nauthors=mesh-admin jschoubben\n"), ["mesh-admin", "jschoubben"]); + assert.deepEqual(readyAuthors("f", "mesh-admin", () => { throw new Error("gone"); }), ["mesh-admin"]); + assert.deepEqual(readyAuthors("f", "mesh-admin", () => "authors=\n"), ["mesh-admin"]); }); diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index 99803c6c..b8c46151 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -10,7 +10,7 @@ import { registerModuleTools, type ToolDefinition } from "@novox/mesh-sdk/tools"; import { emit } from "@novox/mesh-sdk/events"; -import { GiteaClient, type BranchProtection } from "../client.js"; +import { GiteaClient, readyAuthors, type BranchProtection } from "../client.js"; import { appendNote, run, viewBody, viewComment } from "../delivery.js"; import { containedFrom, isCommit, mergeableUnder, newestByContext } from "../checks.js"; import { @@ -514,9 +514,9 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { }, { name: "gitea_open_pulls", - description: "Every open pull request of every repository, from the forge itself: its head, its base, the statuses the base's protection requires and the ones its head has, each with its state, description and when it was set. What mesh-delivery reads to say a pull request the mesh was never asked to check (novox/hq issue 347), or whose check started and never answered (issue 438), and each pull request's labels and ready mark — the label mesh/ready and the head the newest comment \"ready at \" names — which mesh-delivery merges on (issue 722). Reads only.", + description: "Every open pull request of every repository, from the forge itself: its head, its base, the statuses the base's protection requires and the ones its head has, each with its state, description and when it was set. What mesh-delivery reads to say a pull request the mesh was never asked to check (novox/hq issue 347), or whose check started and never answered (issue 438), and each pull request's labels and ready mark — the label mesh/ready and the head named by the newest comment \"ready at \" that a listed account (the setting ready-authors) wrote after the label was added and after the last push — which mesh-delivery merges on (issue 722). Reads only.", input: {}, - run: async () => ({ pulls: await gitea.openPulls() }), + run: async () => ({ pulls: await gitea.openPulls(readyAuthors()) }), }, { name: "gitea_contains", -- 2.54.0 From baa5ef802e8220f6664f3de99e78c0f78383e879 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 21:50:05 +0200 Subject: [PATCH 3/3] Name the keys gitea's mergeable config takes: none (ADR 0326) gitea now declares a setting, so every mergeable file of it must name its keys; the runtime config reads no setting, and ready-authors reaches its own file. --- modules/gitea/module.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/gitea/module.json b/modules/gitea/module.json index c3290a80..069d7163 100644 --- a/modules/gitea/module.json +++ b/modules/gitea/module.json @@ -222,7 +222,8 @@ "path": "${dir:mesh-state}/config.json", "mode": "0600", "content": "{}\n", - "merge": "json" + "merge": "json", + "takes-settings": [] } ], "provides": [ -- 2.54.0