From a0765c0b13735ea4f97f7ab69290c734e3dabd56 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 11:00:44 +0200 Subject: [PATCH] Issue 310: a pull request ran its own copy of its repository's check The build seat ran the merge-check.sh in the change's head, so a branch older than the script, or one that gutted it, merged untested; and the media catalogue did not require mesh/repo-check at all. Located, fixed on branches, replayed as R310; the protection change awaits the operator. --- .../45-a-core-that-cannot-fail-silently.md | 7 +- .../00-report.md | 86 +++++++++++++++++++ .../01-diagnosis.md | 56 ++++++++++++ 3 files changed, 148 insertions(+), 1 deletion(-) create mode 100644 04-ISSUES/310-a-pull-request-ran-its-own-copy-of-its-check/00-report.md create mode 100644 04-ISSUES/310-a-pull-request-ran-its-own-copy-of-its-check/01-diagnosis.md diff --git a/03-DESIGN/01-to-be/45-a-core-that-cannot-fail-silently.md b/03-DESIGN/01-to-be/45-a-core-that-cannot-fail-silently.md index 23673f7a..9f1c4187 100644 --- a/03-DESIGN/01-to-be/45-a-core-that-cannot-fail-silently.md +++ b/03-DESIGN/01-to-be/45-a-core-that-cannot-fail-silently.md @@ -549,7 +549,12 @@ the definitions of the modules the plan moves or adds; the replays. Every reposi own** `merge-check.sh` (`mesh/repo-check`) in the toolchain it declares. A repository in two languages declares each further part of its check and the toolchain that part runs in, and the build seat runs each part in that toolchain's own container: the layer passes only when every part does (issue 302; checked by -the builder's `TestARepositoryInTwoLanguagesIsCheckedInBoth`). A repository with none has "no repository +the builder's `TestARepositoryInTwoLanguagesIsCheckedInBoth`). **The check that judges a change is the base +branch's**: where the branch a pull request merges into holds a `merge-check.sh`, its script, its parts and +its toolchain run against the change's tree, and a head that lacks or alters any of them is said so in the +summary. A change to the check is judged by the one it replaces (issue 310; checked by the builder's +`TestAHeadWithoutTheScriptIsJudgedByMains` and `TestAHeadThatGutsTheScriptIsStillJudgedByMains`, replayed as +R310). A repository with none has "no repository check defined", a success where the branch does not require `mesh/repo-check` and a failure for a person where it does. **A note is never a warning on a required status**: the forge combines `warning` as a failure, so a note โ€” a wide rebuild, a problem already so on the base โ€” is a success that says it, and only diff --git a/04-ISSUES/310-a-pull-request-ran-its-own-copy-of-its-check/00-report.md b/04-ISSUES/310-a-pull-request-ran-its-own-copy-of-its-check/00-report.md new file mode 100644 index 00000000..42d2df2d --- /dev/null +++ b/04-ISSUES/310-a-pull-request-ran-its-own-copy-of-its-check/00-report.md @@ -0,0 +1,86 @@ +--- +status: located +opened: 2026-10-08 +located-in: [mesh-controller internal/builder (check.go, the build agent's merge check), the forge's branch protection of mesh-media-catalog] +fixed-by: mesh-controller#135, mesh-lab#65 +replay: R310 +amended-design: 03-DESIGN/01-to-be/45-a-core-that-cannot-fail-silently.md +--- + +# 310. A pull request ran its own copy of its repository's check + +## What was observed + +In the media catalogue, a pull request for the music manager was cut from a commit that came before +the repository had a `merge-check.sh`. The build seat checked it and posted `mesh/repo-check` as +**success**, with a summary saying no repository check was defined. The pull request merged. None of the +catalogue's tests had run on it. + +Two holes let it through, and either one alone would have been enough: + +1. **The status was not required there.** The media catalogue's branch protection required + `mesh/merge-gate` only. A `mesh/repo-check` of any colour could not stop a merge. Reading the rules of + every mesh repository with `gitea_branch_protection_get` on 2026-10-08 found that the catalogue is the + only repository that holds a `merge-check.sh` on main and does not require `mesh/repo-check`. The Go + tool runner's repository requires only `mesh/merge-gate` as well, but it has no script to require. +2. **The check ran the script the change brought.** The build agent cloned the pull request's head and + ran the `merge-check.sh` it found there, with the toolchain and parts declared in that copy. A branch + older than the script had none, so it got the warning above. A change could also delete the script, or + reduce it to `exit 0`, and its own check would pass. The same was true of a declared part + (`# mesh-check-also:`) and of the toolchain marker. + +## Why it is an issue + +To-be 45 ยง9 and ADR 0237 say every repository runs its own `merge-check.sh` before it merges, so that +nothing merges untested. The rule was kept in form and not in substance. The thing under judgement chose +its own judge. Where the status was not required, nobody had to read the judge's verdict at all. Nothing +said this was happening. The warning read like a fact about the repository, but it was really a fact +about how old the branch was. + +## Fix + +**The base branch's check judges the change** (mesh-controller#135). Where the branch a pull request +merges into holds a `merge-check.sh`, the build seat runs that script against the change's tree: the +base branch's script, its declared parts, and its toolchain marker. The change's own copies are not run. +The base branch's files go into the throwaway clone after the gate has composed the change as it stands. + +- If the head lacks the script, the base's runs and the summary says so: "the change holds no + merge-check.sh: main's judged it". +- If the head changes the script or any of its declared parts, the base's version still runs. The + summary leads with "THE CHANGE ALTERS ITS OWN CHECK", names the files, and says the change's version + judges the pull requests after it merges. The verdict stays that of the base's script. It is not turned + into a warning, because the forge counts a warning as a failure (issue 293), and a warning would block + every legitimate change to a check. +- If the base branch holds no script, nothing changes: the head's script runs, or the head is warned that + it has none. +- If the base branch cannot be read, the check answers an error. It never falls back to the change's + copy. + +**A change to the check is judged by the check it replaces.** This is deliberate. The first pull request +after it merges is the first one judged by the new version. A change that alters the check and also needs +the new check to pass is split into two pull requests. + +**Every repository with a script requires its status.** The forge's rule is a policy, so the operator +changes it rather than the agent. The proposed change is for the media catalogue to require +`mesh/merge-gate` and `mesh/repo-check`, with administrator override still blocked. Until that is done, +the first hole stays open in that one repository. + +## How it is checked + +**The check:** `mesh/repo-check` on every pull request. A pull request that removes or alters +`merge-check.sh` or a declared part shows it in the summary, and the result is the base branch's verdict. + +- `TestAHeadWithoutTheScriptIsJudgedByMains` and `TestAHeadThatGutsTheScriptIsStillJudgedByMains` + (mesh-controller `internal/builder`) show that a head with no script, a gutted script, a gutted declared + part, or no `mesh-check-also` line is still judged by main's script, parts and toolchain. Beside them: + a legitimate change is judged by the check it replaces, a main with no script keeps the old behaviour, + and an unreadable base is an error. A test gated on the container runtime runs the whole `Check` with a + head that has no script. +- **The replay, R310**, in mesh-lab's register (mesh-lab#65), is `TestReplay310`. It is written with only + what the build agent had before the fix. On the commit before, it fails because the layer answered the + warning. On the fix, it passes. The lab's prover reports it as PROVED. +- **The protection** is checked by reading each repository's rule with `gitea_branch_protection_get`. + Nothing checks it automatically yet. A repository can still hold a script without requiring its + status, and nothing would say so. That is recorded here as the open half of this issue. + +The trail is in [01-diagnosis.md](01-diagnosis.md). diff --git a/04-ISSUES/310-a-pull-request-ran-its-own-copy-of-its-check/01-diagnosis.md b/04-ISSUES/310-a-pull-request-ran-its-own-copy-of-its-check/01-diagnosis.md new file mode 100644 index 00000000..e622e989 --- /dev/null +++ b/04-ISSUES/310-a-pull-request-ran-its-own-copy-of-its-check/01-diagnosis.md @@ -0,0 +1,56 @@ +# 310 โ€” diagnosis + +## 2026-10-08: why a merged pull request ran no tests + +The music manager's pull request in the media catalogue had `mesh/repo-check` as success, with the +summary "no repository check defined" carried as a warning. The repository holds a `merge-check.sh` on +main. Two questions follow: why was the script not run, and why did the merge go ahead? + +**Why it was not run.** The build agent's merge check is `Check` in mesh-controller `internal/builder`. +It clones the repository at the pull request's head and reads `merge-check.sh` from that tree. When there +is none and no module of the graph is touched, it answers the repository layer as a warning and runs +nothing. The branch had been cut before the script existed, so the tree had none. The base branch was +already known to the check, because the gate compares manifests against `origin/`, but the +repository layer never looked at it. The same reading means a head that holds a different script runs +that copy, with the toolchain and `mesh-check-also` parts declared in it. So a change can choose what +judges it. + +**Why it merged.** `gitea_branch_protection_get` on the catalogue's main returned only +`mesh/merge-gate` as required. Even a red `mesh/repo-check` would not have stopped the merge. + +## The rules read, 2026-10-08 + +| Repository | `merge-check.sh` on main | Required statuses | Gap | +|---|---|---|---| +| `hq` | yes | `mesh/repo-check` | none (no module of the graph is built from it) | +| `mesh-catalog` | yes | `mesh/merge-gate`, `mesh/repo-check` | none | +| `mesh-controller` | yes | `mesh/merge-gate`, `mesh/repo-check` | none | +| `mesh-host` | yes | `mesh/merge-gate`, `mesh/repo-check` | none | +| `mesh-lab` | yes | `mesh/repo-check` | none | +| `mesh-media-catalog` | yes | `mesh/merge-gate` | **does not require `mesh/repo-check`** | +| `mesh-sdk` | yes | `mesh/merge-gate`, `mesh/repo-check` | none | +| `mesh-tools` | yes | `mesh/merge-gate`, `mesh/repo-check` | none | +| `mesh-tools-go` | no | `mesh/merge-gate` | none to require until it has a script | + +The organisation's other repositories hold no `merge-check.sh`. + +## Ruled out + +- **Failing a head that lacks the script.** This closes the old-branch case but not the gutted-script + case, and it makes a person rebase only to pick up a file the check could have taken from main itself. +- **Running both the head's and the base's scripts.** The head's copy adds nothing that can be trusted. + If it passes while the base's fails, the pull request still fails. If it fails while the base's passes, + that is the change judging itself. +- **Making an altered check a warning.** The forge counts a warning on a required status as a failure + (issue 293). A legitimate change to a check could then never merge. The alteration is said in the + summary, and the verdict stays the base's. +- **Running the base's script from outside the tree.** Scripts change into their own directories and + name their parts by relative path. So the base's files are written into the throwaway clone in place + of the change's, after the gate has composed the change. + +## The fix + +The fix is described in the report. The base branch is read as `origin/` in the clone. If it +cannot be read, the check answers an error rather than falling back to the head. A path the base branch +does not hold is told apart from a read that failed: `git ls-tree` answers nothing for an absent path +and fails only when it cannot read.