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..acf15ce0 --- /dev/null +++ b/docs/adr/ADR-0036-a-bot-merge-goes-through-a-pull-request.md @@ -0,0 +1,115 @@ +# 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 a push to `main` cannot produce it for two +independent reasons in the same file: + +```yaml +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 +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, 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. + +**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..36a2d207 100644 --- a/docs/prd/PRD-F15-canonical-artifact-provenance.md +++ b/docs/prd/PRD-F15-canonical-artifact-provenance.md @@ -95,27 +95,30 @@ 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 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". + +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