From 3d71532a3ae4bf17018156ebae9daab04a11597c Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 03:13:40 +0200 Subject: [PATCH] Issue 341: a merge check judges the pull request's head, not what merging it makes Node-engine #66 passed both required statuses after #64/#65 had merged, and its merge did not compile: each side added Applied.Declared. The check clones the head alone and nothing marks a verdict stale when the trunk moves; ADR 0237/0238 say the head, so the fix needs a decision. --- .../00-report.md | 102 ++++++++++++++++++ .../01-diagnosis.md | 102 ++++++++++++++++++ 2 files changed, 204 insertions(+) create mode 100644 04-ISSUES/341-a-merge-check-judges-the-head-not-what-merging-it-makes/00-report.md create mode 100644 04-ISSUES/341-a-merge-check-judges-the-head-not-what-merging-it-makes/01-diagnosis.md diff --git a/04-ISSUES/341-a-merge-check-judges-the-head-not-what-merging-it-makes/00-report.md b/04-ISSUES/341-a-merge-check-judges-the-head-not-what-merging-it-makes/00-report.md new file mode 100644 index 00000000..8baf4f30 --- /dev/null +++ b/04-ISSUES/341-a-merge-check-judges-the-head-not-what-merging-it-makes/00-report.md @@ -0,0 +1,102 @@ +--- +status: located +opened: 2026-10-09 +located-in: [mesh-controller cmd/mesh-controller (checks.go, the check request names the pull request's head as the ref), mesh-controller internal/builder (check.go, the check clones and checks out that ref and never merges the base), mesh-catalog modules/mesh-delivery (table.go, a ready delivery is rechecked only when a person asks or its group changes; holder.go, a merge on the trunk leaves the repository's other open deliveries as they were)] +fixed-by: +amended-design: +--- + +# 341. A merge check judges the pull request's head, not what merging it makes + +## Symptom + +On 2026-10-09 a node-engine pull request passed its merge check, and its merge broke the trunk. The +trunk then did not compile, so no build of it could be made. + +What happened, in order (UTC): + +1. **00:15.** Node-engine #64 and #65 (issue 339's follow-ups) merged. #65's merge commit, `d28ca95`, is + the node-engine build the nodes run. Both pull requests added a field `Applied.Declared` to the + node-engine's store record, a string, under the JSON key `declared`. +2. **00:49.** Node-engine #66 (ADR 0269) was checked, at head `dfb1415`. That check was asked after the + trunk had moved, so the check was not out of date by the clock. `mesh/merge-gate` passed ("4 of 4 + compose") and `mesh/repo-check` passed (its `merge-check.sh`, every package `ok`). Read with + `mesh-delivery.checks` for the commit: both statuses `success`, set at 00:49:42, and "mergeable — every + status main requires is a success". +3. **00:54.** #66 merged, as `690966d`. #66 had added its own `Applied.Declared` field, a `*FoundUnit`, + **under the same JSON key `declared`**. The two edits touched different lines, so git merged them + without a conflict. The result does not compile: + `internal/store/store.go:145:2: Declared redeclared`. +4. **00:55.** The walk the merge started (`plan-1791507264215942736`) failed: "`mesh-host` failed to build + in tier 0". `mesh/delivery` on the head reads `failure`. Nothing was sent to a node, and every node kept + `d28ca95`. +5. **01:09.** Node-engine #67 repaired the trunk. It renamed #66's field to `DeclaredUnit`, key + `declared_unit`, and added a test that pins both keys. + +The facts behind the "different lines" claim, read from the node-engine's repository: `dfb1415` does not +contain `d28ca95`. Their common ancestor is `a2c2c35` (#63's merge). `dfb1415` holds one `Declared` +(store.go line 107), `d28ca95` holds the other (line 139), and `690966d` holds both (lines 107 and 145). + +No node was harmed, because a walk builds before it sends. But the merge check said "this may merge" of +a merge that did not build, and both required statuses were green when it merged. + +## The cost before this: checks that went stale + +The same night, the session that merged the pull requests below had to ask for their checks again by +hand, or rebase a branch, before the verdict was right (as that session reported it). Each time, another +merge or the order of merges had changed what the check would see. What each pull request's own text +shows: + +- **Controller #165** (issue 335). Its body says `mesh/repo-check` "fails on + `TestTheInstallersFirstUserListIsWhatTheControllerWouldCompose` until #159 merges". The head held the + trunk of before #159. A check of the head alone stays red until the branch is rebased, even after #159 + is on the trunk. +- **Controller #159** (issue 330). Node-engine #60's body says the controller's repository check "failed + on exactly this test without it": the check reads the node-engine's trunk beside the controller, so #159 + waited for #60 to merge and then for a new check. +- **Controller #148**. It was rebased so that its check would run on the current trunk. +- **Controller #169 and catalogue #145** (ADR 0268), and **controller #171 and catalogue #147** (the zfs + module). Each pair names a merge order in its body ("#169 first", "Do not merge before mesh-controller + #171"). The second of each pair needed its check asked again once the first had merged. + +In every case a person kept track of which verdicts the last merge had made stale. That is the work the +merge check exists to remove. + +## Why it is a design issue + +The merge check is the rule that "a change is judged before it merges" +([ADR 0237](../../02-DECISIONS/0237-a-change-is-judged-against-the-mesh-that-runs-before-it-merges-on-the-build-seat.md)). +The forge requires its two statuses on the trunk +([ADR 0238](../../02-DECISIONS/0238-a-commit-is-the-build-at-hand-one-commit-one-change-plan-checked-off-the-trunk-and-published-only-on-it.md) +decision 5). A person and an agent both read "both green" as "this merge is safe". But the check judges +something else: the pull request's head, alone. Two pull requests that each pass can break the trunk +together, and a recheck after the trunk moved judges the same head and passes again. + +Nothing says that a verdict stops counting when the trunk moves. The delivery's state table moves a ready +delivery back to proposed only when a person asks or its group changes +([ADR 0239](../../02-DECISIONS/0239-a-delivery-is-owned-by-the-mesh-delivery-module-and-runs-from-commit-to-delivered.md)). +A merge of another pull request into the same trunk changes neither. + +## What is expected + +- A merge check judges exactly what merging would make: the pull request's head merged into the current + trunk. The forge keeps no merge ref for a pull request (`git ls-remote` lists only + `refs/pull//head`), so the build seat makes that merge itself in its clone. A head that does not + merge cleanly is a failure that says so. +- A verdict is tied to the trunk commit it was judged against. When the trunk moves, every open pull + request's verdict into it is stale. The check either runs again by itself or is marked out of date (the + required statuses back to pending), so that a merge is never let through on a verdict for a trunk that + no longer exists. +- The two required statuses then mean "this merge is green", which is what everyone already reads them as. + +## The records say the head + +ADR 0237 decision 4 has the build seat check "the head". ADR 0238 decision 1 says "A commit not on the +trunk — a pull request's head, any branch — is only checked", and decision 2 computes the delivery plan +from "a repository, the branch it merges into, the commit at hand". Neither record says the head is merged +with its trunk first. **So the fix needs a decision record** that extends ADR 0237 §4 and ADR 0238 §1–§2 +(what is checked is the merge of the head into the trunk at a named trunk commit) and ADR 0239's state +table (a trunk move makes a ready verdict stale). ADR 0238's publishing rule is not touched: a merge made +to check a pull request is still a commit off the trunk, only checked, never registered. + +The diagnosis is in [01-diagnosis.md](01-diagnosis.md). diff --git a/04-ISSUES/341-a-merge-check-judges-the-head-not-what-merging-it-makes/01-diagnosis.md b/04-ISSUES/341-a-merge-check-judges-the-head-not-what-merging-it-makes/01-diagnosis.md new file mode 100644 index 00000000..93c10495 --- /dev/null +++ b/04-ISSUES/341-a-merge-check-judges-the-head-not-what-merging-it-makes/01-diagnosis.md @@ -0,0 +1,102 @@ +# 341 — Diagnosis + +## 2026-10-09 + +**The records first.** `records_search` for "redeclared", "refs/pull" and "merged into its base" found +nothing. "recheck" found [issue 305](../305-a-recheck-left-the-old-verdict-standing/00-report.md), which +is close but not the same. 305 made a recheck put both statuses back to pending before the new check +runs. It also recorded that "some of the rechecked pull requests turned red: judged against the trunk of +that day, they no longer composed". So stale verdicts were seen on 2026-10-07 too. 305 fixed how a +recheck is asked; it did not change what a check judges or when a verdict goes stale. + +### What the check builds (mesh-controller at `36008e0`) + +- `cmd/mesh-controller/checks.go`, around line 259: the build request for a pull request's check sets + `Ref: p.Commit`, the pull request's head. The trunk goes in as `Check.Base`, a branch name. +- `internal/builder/check.go`, line 52, the check's own description: it "clones the repository at the + pull request's head". +- `internal/builder/check.go`, lines 249–256 and 265: `clone` runs `git clone` and then + `git checkout ` with `spec.Ref`. Nothing merges the base into that tree. +- `spec.Base` (declared at line 83, "what the gate compares the change against") is read only to pick + the check script from the trunk (`TheCheckThatJudges`, line 270, issue 310's fix) and to show the + touched manifests as they are on the trunk (around line 582). Neither builds the trunk with the change. +- A delivery group's other heads (lines 297–310) are cloned beside the head, each at its own head. They + are composed together for the merge gate. The repository's own `merge-check.sh` still runs on the head + alone. + +So `mesh/repo-check` ran `go test` on `dfb1415`, a tree with one `Declared`, and passed. The tree that +merging made, with two, was never built before the merge. + +### When a verdict goes stale (mesh-catalog at `63de373`) + +- `modules/mesh-delivery/cmd/mesh-delivery/table.go`, line 60 and line 167: `EvRecheck` takes a rejected + or ready delivery back to proposed. Its comment says "a person asked for it again, or its group + changed". +- `holder.go`, `PullMerged` (line 347): a merge records the merge commit on that pull request's own + delivery and links its walk. It does not touch the repository's other open deliveries into the same + trunk. +- The forge marks a head pending only when it announces a new head (issue 305). A merge into the trunk is + not a new head of any other pull request. + +So after #64/#65 merged, #66's verdict was not marked stale by anything. The recheck at 00:49 was asked by +a person. It judged the same head and passed again. + +### What the records decided + +- [ADR 0237](../../02-DECISIONS/0237-a-change-is-judged-against-the-mesh-that-runs-before-it-merges-on-the-build-seat.md) + decision 4: the controller "asks the build seat a check: the head, and beside it the controller the mesh + runs and its main, the catalogue, the node-engine it runs, and mesh-lab". +- [ADR 0238](../../02-DECISIONS/0238-a-commit-is-the-build-at-hand-one-commit-one-change-plan-checked-off-the-trunk-and-published-only-on-it.md) + decision 1: "A commit not on the trunk — a pull request's head, any branch — is only checked". + Decision 2: a delivery plan is computed from "a repository, the branch it merges into, the commit at + hand". +- [ADR 0239](../../02-DECISIONS/0239-a-delivery-is-owned-by-the-mesh-delivery-module-and-runs-from-commit-to-delivered.md)'s + state table: `ready → recheck → proposed` when "a person asked, or its group changed". + +The code does what the records say. The records name the head and never the merge, and say nothing of +a verdict going stale when the trunk moves. **This is a design gap, and its fix needs a decision record** +(playbook 02), not only a code change. + +### Ruled out + +- **A flaky test or a toolchain difference.** The failure is a compile error that exists only in the + merge. `dfb1415` and `d28ca95` each compile; `690966d` does not. +- **A check run before the trunk moved.** The check that let #66 merge was set at 00:49:42, 34 minutes + after #64 and #65 merged. +- **The forge merging badly.** git merged the two edits without a conflict because they touch different + lines. That is correct for git. A semantic conflict like this one is found only by building the merge. + +### Owners + +- **mesh-controller**: the check request and the builder's clone. The check builds the merge of the head + into the trunk commit it names, and says that trunk commit with its verdict. +- **mesh-catalog modules/mesh-delivery**: a trunk move makes the ready and rejected verdicts of the + repository's other open deliveries into that trunk stale. They are proposed again, or held out of date, + with the statuses back to pending (issue 305's path). +- **The forge's module** (mesh-catalog modules/gitea), possibly: if the fix chooses the forge's own + "outdated branch" protection instead, the protection is set there. + +### Directions for the decision (not chosen here) + +1. **The build seat merges, the delivery owner rechecks on a trunk move.** The check clones the trunk at + a named commit, merges the head into it (refusing a conflict), and runs both layers on that tree. Every + merge into a trunk proposes the open deliveries into it again. Cost: one check per open pull request + into that trunk per merge, on the build seat. How many that is on a busy night is not measured yet. +2. **The forge refuses an outdated branch.** Gitea's branch protection can block a merge when the pull + request's branch is behind its base. The check then stays a check of the head, and the head always + contains the trunk. Cost: every merge forces every other open pull request to rebase or merge the + trunk, by a person or an agent, and each of those is a new head and a new check. That is the manual + work the report lists, made mandatory. +3. **Both**: the merge is what is checked, and a verdict carries the trunk commit it was judged against. + The merge is refused while that commit is not the trunk's current one, until the automatic recheck + answers. + +A delivery group already composes its members' heads for the merge gate. Whatever is chosen should say +how a group's composed check meets the trunk: merged into it, member by member in the group's order. + +### Replay + +This is a core issue (its `located-in` names the controller). It resolves with a replay in mesh-lab's +register, or a `replay-none:` saying why none is possible. A replay is possible: two branches off one +trunk commit, each adding the same field to one struct, each passing alone, and the second checked after +the first merged. Before the fix the second passes; after it, the second fails with the compile error.