From 21c8e4cdb48b884cc66e9b68c1b989bc388e34d5 Mon Sep 17 00:00:00 2001 From: MongLong0214 Date: Mon, 17 Aug 2026 23:01:12 +0900 Subject: [PATCH 1/2] A bot merge goes through a pull request, because a push cannot be checked (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 --- ...a-bot-merge-goes-through-a-pull-request.md | 104 ++++++++++++++++++ .../PRD-F15-canonical-artifact-provenance.md | 43 ++++---- .../F15-canonical-artifact-provenance.md | 43 ++++++-- 3 files changed, 157 insertions(+), 33 deletions(-) create mode 100644 docs/adr/ADR-0036-a-bot-merge-goes-through-a-pull-request.md diff --git a/docs/adr/ADR-0036-a-bot-merge-goes-through-a-pull-request.md b/docs/adr/ADR-0036-a-bot-merge-goes-through-a-pull-request.md new file mode 100644 index 00000000..5bf682d5 --- /dev/null +++ b/docs/adr/ADR-0036-a-bot-merge-goes-through-a-pull-request.md @@ -0,0 +1,104 @@ +# ADR-0036: a bot merge goes through a pull request, because a push cannot be checked + +- Status: Accepted (2026-08-17) +- Owner: CTO +- Issue: [#719](https://github.com/MongLong0214/commitlore/issues/719) +- PRD: [F15](../prd/PRD-F15-canonical-artifact-provenance.md) — closes its "unsolved question" +- Ticket: T-1501 +- Relates to: [ADR-0011](ADR-0011-plugin-first-distribution.md) (distribution is a git clone), and #723's + record that `preserve` reports rather than blocks + +## Context + +F15 wants pull requests to stop carrying `dist/`. The direction is a privileged merge: a +GitHub App merges the source, runs `build:canonical` and `artifact:manifest`, and publishes +that tree — so the commit that lands matches its own source from the start. + +The question T-1501 exists to answer is what verifies that commit. **A tree the App +assembles has never been checked.** That the source pull request was green is a statement +about a different tree, and shipping the naive form moves the gate from *blocking before* to +*reporting after* — the limitation recorded against `preserve` in #723, adopted voluntarily. + +## The finding that decides it + +`main`'s branch protection requires **eleven** status contexts. Ten are the release gate's +`REQUIRED_CHECKS`. The eleventh is `lint`, and `demo-lint.yml` says why it cannot appear +anywhere else: + +```yaml +lint: + # The action's own range derivation works for PRs. On push events there is + # no base_ref, so this job only runs for pull requests. + if: github.event_name == 'pull_request' +``` + +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** — +not the App's, not anyone's. Every push-shaped option therefore needs a bypass, and a +bypass is not "the check ran and passed"; it is "the check did not have to". + +That is not a fact about this App. It is a fact about this repository's gate: `main` is +reachable only through a pull request by construction. + +## Decision + +**The App does not push to `main`. It opens a pull request.** + +The App assembles a branch — the source change plus a canonical rebuild — and opens a pull +request for it. All eleven contexts run on that branch, including `lint`. It merges the way +every other change merges. + +No bypass is requested, and none should be granted for this purpose. + +## Consequences + +**What is preserved.** Every commit reaching `main` has passed the same eleven checks as +any other. `git diff --exit-code -- dist/` keeps its meaning, and the two claims F15 refuses +to separate — *this source is deterministic* and *this checkout is the plugin* — stay +joined at the point the commit is admitted rather than reported on afterwards. + +**What it costs.** One additional pull request and one additional CI cycle per change. That +is worse than today for a change that would not have conflicted, and better for one that +would. It also spends CI to buy a property rather than to buy speed, which is the trade F15 +was reaching for in the other direction. + +**What it fixes, and this is the point.** A contributor never needs `build:canonical`. #720 +arrived from a Windows machine with `src/` and `test/` only — correctly, since a Windows +host cannot produce a `linux/amd64` Docker build — and waited on a maintainer twice. Under +this decision that wait does not exist, on any platform. + +**What stays open.** Whether the saving is worth the second cycle. This ADR answers *how a +bot merge is verified*, not *whether to build one*. #719's reopening conditions are +unchanged, and T-1502 remains unscheduled. + +## Rejected + +**Bypass plus a required push-event check.** Cheapest, and the honest description is that +`main` may hold an unverified tree until the push job finishes. #723 records exactly this +shape as a limitation forced by an event; choosing it here would be adopting it by +preference. If the cost of the extra cycle ever makes this necessary, it should arrive as +its own ADR that says plainly which property is being traded. + +**Staging ref, then fast-forward `main`.** Appealing because the staging commit could carry +its own green checks, so the push looks like it stands on its merits. It does not: `lint` +never runs on a branch push either, so the staging commit is missing the same context and +the fast-forward needs the same bypass. The finding above kills this one specifically, and +it is recorded because it is the option that looks safest before the `lint` detail is +noticed. + +**Push the rebuild to the contributor's own branch.** Removes the contributor's dependency +on Docker without an extra pull request, and does not remove the conflict — every branch +still carries `dist/`. It also cannot reach a fork: the App is installed on this repository, +not on anyone's fork, so the case that actually hurt is the case it misses. + +## What must be verified before T-1502 + +1. **That protection evaluates required contexts on the pushed commit**, as assumed above. + The decision does not depend on it — going through a pull request is correct either way — + but the *rejection* of the staging-ref option does. +2. **That a pull request opened by an App triggers the same eleven checks.** A workflow + whose `on: pull_request` does not fire for App-opened pull requests would leave the + rebuild pull request green with nothing having run, which is the empty-runner shape + (#722) in a new place. + +Neither has been measured. Recorded as assumptions rather than facts. diff --git a/docs/prd/PRD-F15-canonical-artifact-provenance.md b/docs/prd/PRD-F15-canonical-artifact-provenance.md index 4b4649a9..24bf9a94 100644 --- a/docs/prd/PRD-F15-canonical-artifact-provenance.md +++ b/docs/prd/PRD-F15-canonical-artifact-provenance.md @@ -95,27 +95,28 @@ A weaker variant — rebuild and push on `push` to `main` — keeps `HEAD` consi leaves one stale parent in history, which must never be tagged. It is recorded so that it is rejected deliberately rather than rediscovered. -## The unsolved question - -**A commit the App pushes has not been checked.** - -For the App to push to `main` it must be in the branch protection bypass list. The commit -it pushes is the merge result plus a rebuild — a tree no required check ran against. That -the pull request was green is a statement about a different tree. - -So the naive implementation moves the gate from *blocking before* to *reporting after*, -which is the limitation recorded against `preserve` in #723 — adopted voluntarily this -time. Any design that ships must answer this. Three shapes are worth evaluating and none -has been: - -1. **Push to a staging ref, let checks run there, fast-forward `main` only on green.** - Keeps blocking semantics; costs a second CI cycle, which is the cost being removed. -2. **Make the rebuild verifiable without re-running checks** — the App pushes, and a - required push-event check re-derives the bundle and compares. Still after the fact, but - the window is bounded and the failure is loud rather than silent. -3. **Do not bypass at all**: the App pushes to a branch, opens a pull request containing - only the rebuilt artifact, and that pull request passes normally. Preserves every - property; adds a second merge per change. +## How a bot merge is verified — answered (ADR-0036) + +`main`'s protection requires **eleven** contexts, and the eleventh is `lint`, which +`demo-lint.yml` runs only on `pull_request` because a push event has no `base_ref`. So a +direct push produces no `lint` context, ever, and **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 does not push. **It opens a pull request** carrying 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. + +Cost: 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. The staging-ref option +was rejected for the same `lint` reason it initially seemed to solve; ADR-0036 records why, +because it is the shape that looks safest before that detail is noticed. + +Two assumptions behind it are unmeasured and named there: that protection evaluates +required contexts on the pushed commit, and that a pull request opened by an App triggers +the same checks. The second matters most — a workflow that does not fire for App-opened +pull requests would leave the rebuild green with nothing having run, which is #722's empty +runner in a new place. ## Success diff --git a/docs/tickets/F15-canonical-artifact-provenance.md b/docs/tickets/F15-canonical-artifact-provenance.md index 38f6f00c..3faf8b9c 100644 --- a/docs/tickets/F15-canonical-artifact-provenance.md +++ b/docs/tickets/F15-canonical-artifact-provenance.md @@ -5,16 +5,20 @@ > Issue: [#719](https://github.com/MongLong0214/commitlore/issues/719) > Baseline head: `ad6fee3` (1.1.2). -**T-1501 is a decision, not an implementation, and nothing after it may start until it -lands.** The credentials are in place; the reason this is unscheduled is that a commit the -App pushes has not been checked, and no ticket here pretends otherwise. +**T-1501 is closed by [ADR-0036](../adr/ADR-0036-a-bot-merge-goes-through-a-pull-request.md).** +The App does not push to `main`; it opens a pull request. `main`'s protection requires +`lint`, which runs only on `pull_request`, so no push can satisfy protection on its merits +and every push-shaped option needs a bypass. Going through a pull request needs none. + +T-1502 onward remain **unscheduled**: the decision answers how a bot merge is verified, not +whether to build one, and #719's reopening conditions are unchanged. **Ordering is strict.** T-1501 → T-1502 → T-1503 → T-1504. Each removes something the next depends on not existing. --- -## T-1501 Decide how a bot-pushed commit is verified (S) — decision only +## T-1501 Decide how a bot-pushed commit is verified (S) — **done**, ADR-0036 **Owns** @@ -45,18 +49,23 @@ limitation recorded against `preserve` in #723, adopted voluntarily. pull request for it, which passes normally. Preserves every property. Costs a second merge per change and needs a rule for what happens when that second pull request fails. -**Acceptance** +**Acceptance** — met by ADR-0036. It names the chosen shape (pull request, no bypass), +what it costs (one extra pull request and CI cycle per change), and the three rejected +shapes with their reasons. The staging-ref option is rejected by the same `lint` finding +that looked like it solved the problem, which is recorded because that is the shape a +reader reaches for first. -- An ADR that names the chosen shape, the property it gives up if any, and the two - rejected shapes with their costs. -- If the answer is that none is acceptable, that is a valid outcome: the ADR says so and - #719 stays open with the reason sharpened. **Do not ship a design to close a ticket.** +Two assumptions are named there as unmeasured, and **T-1502 verifies them before anything +else**: that protection evaluates required contexts on the pushed commit, and that an +App-opened pull request triggers the same checks. The second is the dangerous one — a +workflow that does not fire would leave the rebuild pull request green with nothing having +run, which is #722's empty runner in a new place. **Not in scope** — any workflow file, any permission change. --- -## T-1502 Rebuild-and-push, behind whatever T-1501 chose (M) +## T-1502 The rebuild arrives as a pull request (M) **Owns** @@ -64,8 +73,18 @@ limitation recorded against `preserve` in #723, adopted voluntarily. - `scripts/check-exact-head-ci.mjs` — `EXPECTED_CI_WORKFLOW_SHA256` if `ci.yml` moves - `test/canonical-merge-workflow.test.ts` (new) -**Depends on** — T-1501 merged. Branch protection updated by the repository owner, which -is not a step any agent performs. +**Depends on** — ADR-0036. **No branch protection change is required, and none should be +requested** — that is the point of the decision. + +**Verify first, before writing the workflow.** ADR-0036 names two assumptions it did not +measure, and the second can make everything downstream vacuous: + +- protection evaluates required contexts on the pushed commit (this only affects why the + staging-ref option was rejected, not the chosen shape); +- **a pull request opened by the App triggers all eleven contexts.** If `on: pull_request` + does not fire for App-opened pull requests, the rebuild pull request is green with + nothing having run — #722's empty runner in a new place. Open one throwaway pull request + from the App and read the check list before building on it. **Owns the assertions**, in the shape `test/preserve-workflow-safety.test.ts` established: the workflow's safety properties are read from the file with comment lines stripped, so a From aec23ef3b0b651deaefe09acdbd05c958803c385 Mon Sep 17 00:00:00 2001 From: MongLong0214 Date: Mon, 17 Aug 2026 23:08:51 +0900 Subject: [PATCH 2/2] State both reasons a push cannot produce the lint context 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 --- ...a-bot-merge-goes-through-a-pull-request.md | 29 +++++++++++++------ .../PRD-F15-canonical-artifact-provenance.md | 6 ++-- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/docs/adr/ADR-0036-a-bot-merge-goes-through-a-pull-request.md b/docs/adr/ADR-0036-a-bot-merge-goes-through-a-pull-request.md index 5bf682d5..acf15ce0 100644 --- a/docs/adr/ADR-0036-a-bot-merge-goes-through-a-pull-request.md +++ b/docs/adr/ADR-0036-a-bot-merge-goes-through-a-pull-request.md @@ -22,16 +22,26 @@ about a different tree, and shipping the naive form moves the gate from *blockin ## The finding that decides it `main`'s branch protection requires **eleven** status contexts. Ten are the release gate's -`REQUIRED_CHECKS`. The eleventh is `lint`, and `demo-lint.yml` says why it cannot appear -anywhere else: +`REQUIRED_CHECKS`. The eleventh is `lint`, and a push to `main` cannot produce it for two +independent reasons in the same file: ```yaml -lint: - # The action's own range derivation works for PRs. On push events there is - # no base_ref, so this job only runs for pull requests. - if: github.event_name == 'pull_request' +on: + pull_request: + push: + branches: [dev] # a push to main does not start this workflow at all + +jobs: + lint: + # The action's own range derivation works for PRs. On push events there is + # no base_ref, so this job only runs for pull requests. + if: github.event_name == 'pull_request' ``` +Either one is sufficient. Both are there because the job needs a base to compare against +and a push does not carry one — the trigger scope and the condition are the same fact +stated twice, not a belt-and-braces accident. + 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** — not the App's, not anyone's. Every push-shaped option therefore needs a bypass, and a @@ -80,9 +90,10 @@ preference. If the cost of the extra cycle ever makes this necessary, it should its own ADR that says plainly which property is being traded. **Staging ref, then fast-forward `main`.** Appealing because the staging commit could carry -its own green checks, so the push looks like it stands on its merits. It does not: `lint` -never runs on a branch push either, so the staging commit is missing the same context and -the fast-forward needs the same bypass. The finding above kills this one specifically, and +its own green checks, so the push looks like it stands on its merits. It does not, and more +plainly than for `main`: the push trigger is scoped to `dev`, so a push to a staging branch +starts no lint workflow at all. The staging commit is missing the same context and the +fast-forward needs the same bypass. The finding above kills this one specifically, and it is recorded because it is the option that looks safest before the `lint` detail is noticed. diff --git a/docs/prd/PRD-F15-canonical-artifact-provenance.md b/docs/prd/PRD-F15-canonical-artifact-provenance.md index 24bf9a94..36a2d207 100644 --- a/docs/prd/PRD-F15-canonical-artifact-provenance.md +++ b/docs/prd/PRD-F15-canonical-artifact-provenance.md @@ -97,8 +97,10 @@ is rejected deliberately rather than rediscovered. ## How a bot merge is verified — answered (ADR-0036) -`main`'s protection requires **eleven** contexts, and the eleventh is `lint`, which -`demo-lint.yml` runs only on `pull_request` because a push event has no `base_ref`. So a +`main`'s protection requires **eleven** contexts, and the eleventh is `lint`, which a push +to `main` cannot produce for two independent reasons: `demo-lint.yml`'s push trigger is +scoped to `dev`, so the workflow does not run at all, and the `lint` job is additionally +gated on `github.event_name == 'pull_request'` because a push event has no `base_ref`. So a direct push produces no `lint` context, ever, and **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".