From 59b13d091d7716119d97074dd063615b38998b40 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 21:57:32 +0200 Subject: [PATCH] Ask no check of a group member that waits on its composed check, and stop asking a merge refused the same way with no trunk move (issue 1106) --- modules/gitea/merge.ts | 10 ++++- modules/gitea/pulls.ts | 6 ++- modules/gitea/test/guard.test.ts | 24 +++++++++++ modules/gitea/trunk.ts | 7 +++- .../cmd/mesh-delivery-gate/approval.go | 1 + .../cmd/mesh-delivery-gate/issue1348_test.go | 40 +++++++++++++++++++ .../cmd/mesh-delivery-gate/merges.go | 11 +++++ .../cmd/mesh-delivery-gate/ready.go | 38 ++++++++++++++++++ 8 files changed, 134 insertions(+), 3 deletions(-) diff --git a/modules/gitea/merge.ts b/modules/gitea/merge.ts index 09bdcee6..1bac4620 100644 --- a/modules/gitea/merge.ts +++ b/modules/gitea/merge.ts @@ -12,7 +12,7 @@ import { mergeableUnder, newestByContext } from "./checks.js"; import { MERGE_METHODS, Refused, checkName } from "./guard.js"; import { CHECK_CONTEXT, REPO_CHECK_CONTEXT } from "./pulls.js"; import { approvedHeadsOf, groupRefusal, memberKey, namesSiblings, notApproved, readSiblings, siblingOutOfDate, siblingsFile } from "./siblings.js"; -import { announcementOf, askedWords, outOfDateWords, sameCommit, tookFile, tookWords, twelve, verdictAtTrunk } from "./trunk.js"; +import { announcementOf, askedWords, outOfDateWords, sameCommit, tookFile, tookWords, twelve, verdictAtTrunk, waitsForComposed } from "./trunk.js"; /** A head named for a merge: at least 12 hex characters of its sha (novox/hq issue 381), up to a sha-256 commit's 64. */ const REVIEWED_HEAD = /^[0-9a-f]{12,64}$/; @@ -117,6 +117,14 @@ export function mergeTool(gitea: GiteaClient, given: MergeDeps): ToolDefinition : at.judged ? `was judged against trunk ${twelve(at.judged)}` : "names no trunk commit it was judged against"; const took = tookWords(tookFile(deps.stateDir), `${owner}/${repo}`); const head = twelve(pull.head_sha), now = twelve(trunkHead); + // **A member that waits for its group's composed check is not announced again** (novox/hq issue 1106): its own + // check can only say "composed" once more, so an ask here brought a verdict naming no trunk and the next merge was + // refused the same way for ever. The composed check is mesh-delivery's to ask; this says whose verdict is awaited. + if (waitsForComposed(checks[CHECK_CONTEXT])) { + throw new Refused(`${owner}/${repo}#${number} is not merged: its merge gate ${checks[CHECK_CONTEXT]?.description}; the ` + + `verdict that names its trunk commit is its group's composed check's, and mesh-delivery asks it, this tool asks no check ` + + `(main is now ${now}). Ask again once the group's composed check has passed. Nothing was merged`); + } if (at.onItsWay) { throw new Refused(`${owner}/${repo}#${number} is not merged: its verdict ${judged}, and ${pull.base} is now ` + `${now}. Its check is already asked and not yet answered (${checks[CHECK_CONTEXT]?.description}), so it is not ` + diff --git a/modules/gitea/pulls.ts b/modules/gitea/pulls.ts index 4e01f415..cdd6a7ff 100644 --- a/modules/gitea/pulls.ts +++ b/modules/gitea/pulls.ts @@ -226,6 +226,10 @@ function repoWrong(repo: Layer | undefined, facts: RepoCheckFacts): boolean { return !!repo && repoStatus(repo, facts).state !== "success"; } +/** What a group member's gate says while the group's composed check is the one that judges it: the member's own check can say + * nothing more of it, so asking that check again brings no verdict that names a trunk (novox/hq issue 1106). */ +export const WAITS_FOR_COMPOSED = "waits for its group's composed check"; + /** The forge's status for the gate. */ export function statusFor(c: Checked): CommitStatus { const gate = c.gate ?? { verdict: c.verdict, summary: c.summary }; @@ -260,7 +264,7 @@ export function statusesFor(c: Checked, facts: RepoCheckFacts = {}, gateNow?: st // set. The gate stays pending, saying it waits for the group, unless a composed verdict already stands on it. const out: CommitStatus[] = []; if (!(gateNow ?? "").includes(" composed: ")) { - out.push({ state: "pending", context: CHECK_CONTEXT, description: "waits for its group's composed check" }); + out.push({ state: "pending", context: CHECK_CONTEXT, description: WAITS_FOR_COMPOSED }); } const repo = c["repo-check"]; if (repo) out.push(judgedFirst(c, repoStatus(repo, facts))); diff --git a/modules/gitea/test/guard.test.ts b/modules/gitea/test/guard.test.ts index a654393a..c7605f3c 100644 --- a/modules/gitea/test/guard.test.ts +++ b/modules/gitea/test/guard.test.ts @@ -339,6 +339,30 @@ test("gitea_merge_pull_request refuses a verdict judged against an earlier trunk assert.equal(state.emitted.length, 4, "a merge asks no check"); }); +// **A group member whose gate waits for its group's composed check is not announced again by the merge tool** (novox/hq issue +// 1106): the member's own check can only say "composed" again, so each refusal's ask brought a verdict that names no trunk and +// the next merge was refused the same way, for ever. The tool says whose verdict is awaited and asks nothing. +test("gitea_merge_pull_request asks no check of a member that waits for its group's composed check", async () => { + const merge = () => tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, head: REVIEWED }); + state.trunk = TRUNK; + const before = state.emitted.length; + state.statuses = [{ context: "mesh/merge-gate", state: "pending", description: "waits for its group's composed check" }, + { context: "mesh/repo-check", state: "success", description: "pass: y" }]; + for (let i = 0; i < 3; i++) { + await assert.rejects(merge(), (err: Error) => { + assert.match(err.message, /waits for its group's composed check/); + assert.match(err.message, /mesh-delivery asks it, this tool asks no check/); + assert.match(err.message, /Nothing was merged/); + return true; + }); + } + assert.equal(state.emitted.length, before, "no check is announced again and again"); + assert.equal(state.merged, false); + // Once the composed verdict names the member's unmoved trunk, it merges. + state.statuses = judgedAt(TRUNK); + assert.equal(((await merge()) as Record).merged, true); +}); + // A merge lands only the head that was reviewed (novox/hq issue 381): mesh-controller#197 was reviewed at one head, // force-pushed, checked green at the new one, and merged there, because the tool merged whatever the head was. test("gitea_merge_pull_request merges only the head it is named, and refuses one that is not the head", async () => { diff --git a/modules/gitea/trunk.ts b/modules/gitea/trunk.ts index 1555fab1..e2ed8572 100644 --- a/modules/gitea/trunk.ts +++ b/modules/gitea/trunk.ts @@ -17,7 +17,7 @@ import { existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs"; import { dirname } from "node:path"; import type { CommitStatus, GiteaClient, GiteaPull, HeadCheck } from "./client.js"; -import { CHECK_CONTEXT, REPO_CHECK_CONTEXT } from "./pulls.js"; +import { CHECK_CONTEXT, REPO_CHECK_CONTEXT, WAITS_FOR_COMPOSED } from "./pulls.js"; /** A commit as a verdict names it: its first twelve characters. */ export function twelve(sha: string | undefined): string { @@ -58,6 +58,11 @@ export function checkOnItsWay(gate: HeadCheck | undefined): boolean { return d.startsWith("waiting in the build queue") || d.startsWith("building on ") || d.startsWith("checking again"); } +/** Whether a gate says it waits for its group's composed check. */ +export function waitsForComposed(gate: HeadCheck | undefined): boolean { + return !!gate && gate.state === "pending" && (gate.description ?? "").startsWith(WAITS_FOR_COMPOSED); +} + /** * The statuses a trunk's move puts back to pending on one open pull request's head, from what the forge holds on it * now: none when it has no merge check yet, when a check is pending (on its way, or already out of date — its diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go index 71c3fc5b..da3befe5 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go @@ -634,6 +634,7 @@ func (h *Holder) answered(key string, ans sdk.Answer) []func() { for _, d := range covered { r := d.Ready r.Warrant = ans.Words + r.Repeats = 0 // the operator chose: whatever the tool refused before is asked afresh (issue 1106) h.mergeAgainNow(d) // the operator chose: the merge is asked at once switch ans.Option { case OptionApprove: diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1348_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1348_test.go index 07bb1c26..4555603b 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1348_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1348_test.go @@ -173,3 +173,43 @@ func TestAGroupMergedWholeMovesSourceMainOnceAndAnnouncesWithTheTeller(t *testin t.Fatalf("source.main moved again: %d", len(w.source.moves)) } } + +// 1106: the same refusal again and again with the trunk not moved is not asked on without end. Three refusals in a row with +// the same cause end the asking by a verdict, a status or a composed check, and say it urgent; a trunk's move, or the +// operator's Approve, starts it again. +func TestARefusalThatRepeatsWithNoTrunkMoveIsNotAskedAgainWithoutEnd(t *testing.T) { + w := answeredFirst(t) + d := w.h.deliveries[IDOf(gCat, gHk)] + refuse := func(took string) { + w.h.ForgeRefused(sdk.Forge{Kind: sdk.ForgeRefused, Repository: gCat, Number: 2, Head: gHk, Act: "merge", + Words: "novox/mesh-catalog#2 is not merged: its verdict names no trunk commit it was judged against, and main is now f70a99dbf485. " + + "The check of head d2d2d2d2d2d2 merged into trunk f70a99dbf485 was asked; the last check took " + took + ". Ask again once both statuses pass"}) + } + tried := w.now + for i, took := range []string{"4m1s", "4m9s"} { + refuse(took) + d.Ready.LastTry = tried + w.h.mergeAgainNow(d) + if !d.Ready.LastTry.IsZero() { + t.Fatalf("refusal %d: the merge was not asked again at once on a verdict, as after a first refusal", i+1) + } + } + refuse("4m5s") // the third in a row, with only the check's duration changed + if d.Ready.Repeats != 3 { + t.Fatalf("%d refusals counted for the same cause, wanted 3", d.Ready.Repeats) + } + d.Ready.LastTry = tried + w.h.mergeAgainNow(d) // the composed check or a verdict answers: nothing is asked + if d.Ready.LastTry.IsZero() { + t.Fatal("the merge was asked again after the same refusal three times with the trunk not moved") + } + if lines := urgentLines(w); len(lines) != 1 || !strings.Contains(lines[0].Says, "names no trunk commit") { + t.Fatalf("the repeated refusal is not said urgent: %+v", lines) + } + // The trunk moves: the merge is asked again. + d.TrunkNow = "a1a1a1a1a1a1a1a1" + w.h.mergeAgainNow(d) + if !d.Ready.LastTry.IsZero() { + t.Fatal("a trunk's move did not let the merge be asked again") + } +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go index 0ec30658..4070027b 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go @@ -287,9 +287,20 @@ func (h *Holder) ForgeRefused(f sdk.Forge) { if r.RefusedSince == nil { r.RefusedSince = &now } + r.noteRefusal(d, answer) if !outOfDateRefusal(answer) { r.Urgent = true // never a warning again for this head } + if r.Repeats >= maxRepeatedRefusals { + // Loud, not endless (novox/hq issue 1106): the same refusal again with the trunk not moved is no trunk's move, so + // the merge is not asked again by a verdict or a status; the operator is told, urgent, and decides. + r.Urgent = true + if r.Repeats == maxRepeatedRefusals { + h.Logf("[mesh-delivery] %s: the merge tool refused it %d times in a row with the same words and the trunk not moved, so "+ + "the merge is asked no more until the trunk moves, a new head is announced or the operator approves again: %s", + d.ID, r.Repeats, answer) + } + } if answer != r.Answer { h.Logf("[mesh-delivery] %s is approved, and the merge tool did not merge it (asked again at the next verdict, Approve or composed check): %s", d.ID, answer) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go index ffccdb18..f2809277 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -85,6 +85,11 @@ type ReadyRun struct { // Urgent is set once a refusal of this head was one that is urgent at once: its severity never steps down to a // warning after (issue 1275, AC6). Urgent bool `json:"urgent,omitempty"` + // Repeats is how many times in a row the merge tool refused with the same words while the trunk stayed where RepeatTrunk + // says: past maxRepeatedRefusals the merge is asked no more by a verdict, a status or a composed check, and the refusal + // is urgent (novox/hq issue 1106). A trunk's move, a new head or the operator's Approve starts the count again. + Repeats int `json:"repeats,omitempty"` + RepeatTrunk string `json:"repeat_trunk,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. @@ -322,10 +327,43 @@ func (h *Holder) askFresh(d *Delivery, now time.Time) func() { // verdict, an Approve, a group's composed check), so the wait after a refusal does not hold it. Under the lock. func (h *Holder) mergeAgainNow(d *Delivery) { if d != nil && d.Ready != nil && d.Ready.Merged == nil { + if d.Ready.repeatedRefusal(d) { + return // the same refusal again and again with the trunk where it was: asking more changes nothing (issue 1106) + } d.Ready.LastTry = time.Time{} } } +// maxRepeatedRefusals is how many refusals in a row, with the same words and the trunk not moved, end the asking again: a +// refusal that repeats with no trunk move is not a trunk's move, and the check asked after each only brings the same words. +const maxRepeatedRefusals = 3 + +// repeatedRefusal is whether the merge was refused maxRepeatedRefusals times with these words and the trunk where it is +// now: it is then asked no more until something changes (a trunk's move, a new head, the operator's Approve). +func (r *ReadyRun) repeatedRefusal(d *Delivery) bool { + return r.Repeats >= maxRepeatedRefusals && r.RepeatTrunk == d.TrunkNow +} + +// refusalKey is a refusal's first sentence, up to the trunk it names ("... and main is now "): what follows says how long the +// last check took and the like, which differs each time and is no change of cause. +func refusalKey(answer string) string { + if i := strings.Index(answer, " is now "); i >= 0 { + if j := strings.Index(answer[i:], ". "); j >= 0 { + return answer[:i+j] + } + } + return answer +} + +// noteRefusal counts a refusal: the same words with the trunk where it was count on, anything else starts the count at one. +func (r *ReadyRun) noteRefusal(d *Delivery, answer string) { + if refusalKey(answer) == refusalKey(r.Answer) && r.RepeatTrunk == d.TrunkNow && r.Repeats > 0 { + r.Repeats++ + return + } + r.Repeats, r.RepeatTrunk = 1, d.TrunkNow +} + // readyWaits is what a delivery's merge waits on; empty when the Approve may be asked and the merge asked. A ready // delivery is asked. So is a proposed one whose verdict went out of date, when an Approve was asked for it already (it // stays through a trunk move, ADR 0421 §3) and its check against the trunk's head is not on its way; one without an