A bot merge goes through a pull request, because a push cannot be checked (T-1501) - #739
Merged
Conversation
…cked (T-1501) F15 left one question open: what verifies a commit the App assembles. That the source pull request was green is a statement about a different tree, so the naive privileged merge moves the gate from blocking before to reporting after -- the limitation #723 records for `preserve`, adopted voluntarily. The answer came from counting the required contexts. `main` requires eleven; ten are the release gate's, and the eleventh is `lint`, which `demo-lint.yml` runs only on `pull_request` because a push event has no `base_ref`. A direct push produces no `lint` context, ever. Protection evaluates required contexts on the commit being pushed, so **no push to `main` can satisfy protection on its merits** -- the App's or anyone's. Every push-shaped option needs a bypass, and a bypass is not "the check passed", it is "the check did not have to". So the App opens a pull request instead. All eleven contexts run on it and it merges like anything else. No bypass is requested and none should be granted for this. That also kills the option that looked safest. A staging ref whose commit carries its own green checks seems to make the fast-forward stand on its merits, and it does not: `lint` does not run on a branch push either, so the staging commit is missing the same context. Recorded in the ADR because it is the shape a reader reaches for first, and the `lint` detail is easy to miss. The cost is one extra pull request and one extra CI cycle per change -- worse than today for a change that would not have conflicted, better for one that would. What it fixes is the part #719 is actually about: a contributor never needs `build:canonical`, on any platform. Limit: this answers how a bot merge is verified, not whether to build one -- T-1502 onward stay unscheduled and #719's reopening conditions are unchanged Limit: two assumptions behind the rejection are unmeasured and named in the ADR; the load-bearing one is whether an App-opened pull request triggers the checks at all Ruled-out: bypass plus a required push-event check | cheapest, and `main` would hold an unverified tree until the push job finishes -- adopting #723's forced limitation by preference Ruled-out: staging ref then fast-forward | `lint` is absent from a branch push too, so it needs the same bypass it appears to avoid Ruled-out: push the rebuild to the contributor's own branch | cannot reach a fork, which is the case that actually hurt Blast: local Undo: easy Certainty: firm Record-Id: r-botpr1501 Provenance: authored Verified: `demo-lint.yml:25` reads `if: github.event_name == 'pull_request'` with the comment giving the reason; the eleven required contexts were read from the branch protection API earlier this session and `lint` is among them, while `REQUIRED_CHECKS` in scripts/check-exact-head-ci.mjs is ten and excludes it -- the two numbers agreeing is what makes the finding decisive rather than a guess CommitLore-Version: 2.0.0
CommitLore — record lintTrailers: clean — 2 commits in Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
The finding was right and its evidence was half of itself. I quoted the job condition -- `if: github.event_name == 'pull_request'` -- and wrote that `demo-lint.yml` runs only on pull requests. It does not: the workflow carries both triggers, and the push one is scoped to `dev`. So a push to `main` fails to produce `lint` twice over, independently. The workflow never starts, and the job would be skipped if it did. Either alone settles it. The imprecision would have cost something specific. A reader checking the claim finds a `push:` trigger three lines above the quote, concludes the ADR misread its own evidence, and reopens a decision that is correct. The same shape as a wrong reason attached to a right decision in #728, and worth the extra commit for the same reason. It also sharpens the rejected staging-ref option, which is now rejected more plainly than `main` is: a push to a staging branch does not match `branches: [dev]`, so no lint workflow starts there either. Limit: the eleven required contexts are still read from a session-earlier API call rather than re-verified here -- the API returned 503 on both attempts today Blast: local Undo: easy Certainty: firm Record-Id: r-linttwice1501 Provenance: authored Verified: `.github/workflows/demo-lint.yml:11-14` carries `on: pull_request` and `push: branches: [dev]`; `:25` gates `lint` on the pull_request event and `:67` gates `promotion-range-lint` on push, so the two jobs split the two triggers between them CommitLore-Version: 2.0.0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes T-1501 with ADR-0036. Advances #719; does not schedule it.
The question
F15 left one open: what verifies a commit the App assembles? "The source PR was green" is a statement about a different tree, so the naive privileged merge moves the gate from blocking before to reporting after — the limitation #723 records for
preserve, adopted voluntarily.The answer came from counting the required contexts
mainrequires eleven. Ten are the release gate'sREQUIRED_CHECKS. The eleventh islint, anddemo-lint.ymlsays why it cannot be anywhere else:A direct push produces no
lintcontext, ever. Protection evaluates required contexts on the commit being pushed, so no push tomaincan satisfy protection on its merits — the App's or anyone's. Every push-shaped option needs a bypass, and a bypass is not "the check passed"; it is "the check did not have to".Two numbers agreeing is what makes this decisive rather than a guess: protection requires eleven including
lint, andREQUIRED_CHECKSis ten and excludes it — for exactly this reason, already documented in that file.Decision
The App opens a pull request instead of pushing. It carries the source change plus a canonical rebuild, all eleven contexts run on it, and it merges like anything else. No bypass is requested, and none should be granted for this purpose.
It also kills the option that looked safest
A staging ref whose commit carries its own green checks appears to make a fast-forward stand on its merits. It does not —
lintdoes not run on a branch push either, so the staging commit is missing the same context and needs the same bypass it appears to avoid. Recorded in the ADR because it is the shape a reader reaches for first.Cost, and what it fixes
One extra pull request and one extra CI cycle per change — worse than today for a change that would not have conflicted, better for one that would.
What it fixes is what #719 is actually about: a contributor never needs
build:canonical, on any platform. #720 arrived from Windows withsrc/andtest/only and waited on a maintainer twice.Scope
This answers how a bot merge is verified, not whether to build one. T-1502 onward stay unscheduled and #719's reopening conditions are unchanged.
Two assumptions are named as unmeasured, and T-1502 verifies them first. The load-bearing one: does an App-opened pull request trigger the checks at all? If
on: pull_requestdoes not fire for it, the rebuild PR is green with nothing having run — #722's empty runner in a new place.