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.
This commit is contained in:
jochen
@@ -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
|
||||
|
||||
@@ -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).
|
||||
@@ -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/<base>`, 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/<base>` 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.
|
||||
Reference in New Issue
Block a user