Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 115 additions & 0 deletions docs/adr/ADR-0036-a-bot-merge-goes-through-a-pull-request.md
Original file line number Diff line number Diff line change
@@ -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.
45 changes: 24 additions & 21 deletions docs/prd/PRD-F15-canonical-artifact-provenance.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
43 changes: 31 additions & 12 deletions docs/tickets/F15-canonical-artifact-provenance.md
Original file line number Diff line number Diff line change
Expand Up @@ -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**

Expand Down Expand Up @@ -45,27 +49,42 @@ 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**

- `.github/workflows/canonical-merge.yml` (new)
- `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
Expand Down
Loading