Build the commit that lands: canonical merge as one pull request (T-1502, #719) - #754
Conversation
`dist/` is committed because a git clone is the install (ADR-0011), so every commit on `main` has to match the bundle its own source produces. `.gitattributes` marks `dist/**` as `-diff -merge`, so when two source pull requests land close together the second one carries the bundle the first one left behind, and somebody has to rebuild it by hand. `build:canonical` is a `linux/amd64` Docker build. #720 arrived from a Windows machine with `src/` and `test/` only -- correctly, because that host cannot produce one -- and waited on a maintainer twice. That is the cost this removes: not CI minutes, but which contributors can finish their own change. On a push to `main` that leaves the committed bundle behind, the App now rebuilds and opens a pull request carrying it. Nothing existing moves: `check` still runs `git diff --exit-code -- dist/`, `artifact:verify` still answers whether the committed manifest describes the committed bundle, and pull requests still carry `dist/` as they do today. This is additive. It opens a pull request rather than pushing, and that is ADR-0036 rather than caution. `main` requires eleven contexts and the eleventh is `lint`, which no push to `main` can produce for two independent reasons -- `demo-lint.yml`'s push trigger is scoped to `dev`, and its `lint` job is gated on `github.event_name == 'pull_request'` because a push has no `base_ref`. Every push-shaped design therefore needs a bypass, and a bypass is not the check passing. The assumption underneath that decision was measured on 2026-08-18: an App-opened pull request attaches all eleven required contexts (run 32082467906). Three things it has to not do, each with its own guard. It must not answer its own rebuild: a commit touching only `dist/` and the manifest is skipped. It must not read an empty change list as that case -- no paths is no evidence, not proof of a rebuild, so it falls through to the build rather than skipping. And it must not push as the wrong identity: `persist-credentials: false`, because the checkout otherwise leaves an `extraheader` that beats a push URL, and the committer address carries the bot user id rather than the App id (#741, #743 -- both measured here rather than reasoned). Limit: this keeps `main` consistent after the fact; pull requests still carry `dist/`, so the conflict itself is unchanged. Removing it means pull requests carrying no bundle at all, and that has an unreconciled consequence recorded on #719 -- the clone, audit and install jobs would then validate `main`'s bundle on every pull request Blast: system Undo: easy Certainty: firm Record-Id: r-rebuildpr Provenance: authored Verified: the guard was exercised on three real change shapes -- artifacts only skips, source-plus-dist and source-only do not; both `printf` calls have as many arguments as conversions; 63 tests across action-lint, preserve-workflow-safety and release-publish-prerequisites pass with the new workflow present CommitLore-Version: 2.0.0
CommitLore — record lintTrailers: clean — 10 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 guard that stops the canonical rebuild from answering itself recognises a shape that does not occur in this repository's history. Every pull request currently carries its own `dist/` -- that is #719's complaint -- so a commit touching only the bundle and its manifest is something this feature will create and nothing has yet. A guard whose only subject is its own future output has no observation behind it, and the first time it is wrong is a loop on `main`. Writing the test found the bug immediately. The inline version read `git show --name-only`, and `git show` on a merge prints the *combined* diff -- only paths that differ from every parent. This repository has one such commit, `bd297e1`, a merge that resolved `dist/`: the inline guard reads it as artifacts-only and would have skipped. Against its first parent it changed thirty files. Reading a merge as a rebuild is the direction that loops. So the decision moved out of the workflow into `scripts/canonical-rebuild-guard.mjs` and reads `--first-parent`. The workflow calls that file and the test drives the same file, rather than a copy of its logic -- the shape #691 removed 845 lines to end, after three readers in three days mistook a dead copy for the live one. Five cases, each a real repository with a real commit: a squash-merged rebuild skips; a source change carrying its bundle does not; a source-only change does not; a merge that only resolved the bundle does not; and a commit with no paths does not, because no paths is no evidence rather than proof of a rebuild. Limit: the squash-merge case is built here rather than observed, so it asserts what this repository's merge settings produce today -- a merge_commit or rebase merge of a rebuild pull request has a different shape and is not covered Blast: system Undo: easy Certainty: firm Record-Id: r-guardtest Provenance: authored Verified: removing `--first-parent` restores the original defect and the merge case fails; restoring it passes 5 of 5, and the guard was also run against this repository's real `bd297e1`, `HEAD` and `origin/main` CommitLore-Version: 2.0.0
The first shape of this let a wrong commit reach `main` and opened a second pull request to fix it. That does not meet what T-1502 asks for -- "a source-only pull request merges and the commit that lands on `main` passes `artifact:verify` and `git diff --exit-code -- dist/` without anyone rebuilding by hand" -- because the commit that lands is the wrong one, briefly, every time. It was also building a file T-1502 owns while T-1501 puts "any workflow file" out of scope. So it builds the landing commit instead: `main` plus the pull request's source plus a canonical rebuild, on one branch, opened as one pull request. All eleven required contexts run on the tree that actually merges rather than on one that resembles it, and a contributor who cannot produce a `linux/amd64` Docker build waits for nothing (#720). Three things the review of the first shape found, each fixed here rather than argued with. The concurrency group orders jobs, not merges. My own comment claimed it removed a race and then described that race: a rebuild is of `main` as it stood when the job started, and nothing stops another pull request landing while it runs. There is now a check between the rebuild and the push that refuses when `main` moved. A rebuild branch cannot be updated, only replaced. `main`'s protection is `strict`, and `.gitattributes` marks `dist/**` as `-merge`, so GitHub's "Update branch" conflicts on the one file the job exists to produce. The branch is keyed to the pull request rather than to a commit, and a rerun force-pushes it. And the guard's own reading was wrong on merge commits. `git show --name-only` prints a merge's combined diff -- only paths differing from every parent -- so a clean merge reports nothing at all. `git show --first-parent` reports seven paths for `5dcc02b` where the plain form reports zero. The suggested replacement, `diff-tree --first-parent`, also reports zero; it was measured rather than adopted. The safety property with no precedent here is the order of two steps. The rebuild runs `npm ci` and `build:canonical` on the merged tree, which executes whatever the pull request put in `package.json`. The App token is minted after that, and is not in that step's environment: a rebuild of somebody else's change must not be able to read the credential that lets it push. `test/canonical-merge-workflow.test.ts` asserts the ordering, with comments stripped so the explanation cannot satisfy it. Limit: this leaves pull requests still carrying `dist/` when they choose to -- T-1503 is what stops requiring it, and until then a source-only pull request is a contributor's option rather than the rule Blast: system Undo: easy Certainty: firm Record-Id: r-canonmergewf Provenance: authored Verified: two negative controls fail the intended assertion and pass after restore -- moving the token mint above the rebuild, and deleting the source-only refusal; 79 tests across the five workflow suites pass, and the guard's `--first-parent` reading was checked against this repository's real merge commit rather than a constructed one CommitLore-Version: 2.0.0
The order guard was thinner than it read. The rebuild runs `npm ci` and `build:canonical` on the merged tree, so the pull request's `package.json` and every dependency lifecycle script it pulls in execute before the App token is minted -- that part held. What did not hold is what the mint step then ran: `node scripts/app-installation-token.mjs`, from the merged workspace. `scripts/` is source. A source-only pull request may change that file, and the step that runs it has `COMMITLORE_BOT_KEY` in its environment. So the App private key was reachable by editing one file, with no `postinstall` and nothing clever -- the guard was about *when* contributor code runs and the credential was reachable by *what* ran afterwards. It now comes from `main` rather than from the merge: the script is extracted with `git show <base>:` into a temporary directory and run from there, so the workspace's `node_modules` is not on its resolution path either. The script imports only `node:crypto` and uses global `fetch`, so nothing else has to come with it. The staleness check had the same shape one level down. It compared `main`'s current sha against `git rev-parse origin/main` read *after* the rebuild -- a value the rebuild had the opportunity to choose, since it runs in the same workspace as that `.git`. The base is now captured before anything from the pull request executes. Two assertions cover both, and both fail when reverted. Limit: this contains the credential, not the runner -- a pull request can still execute arbitrary code during `npm ci`, which is inherent to rebuilding somebody else's change, and the remaining exposure is the runner and the network it can reach rather than this repository's App Blast: system Undo: easy Certainty: firm Record-Id: r-mintfrombase Provenance: authored Verified: restoring `node scripts/app-installation-token.mjs` fails the credential-path assertion, and moving the base-sha read back after the rebuild fails the ordering one; 13 of 13 pass with both in place CommitLore-Version: 2.0.0
Taking the credential script off the merged tree fixed what ran in the mint step and not how it was launched. Both stayed in one job, and step order inside a job is not a boundary: `$GITHUB_ENV` and `$GITHUB_PATH` written during `npm ci` persist into every later step, so a dependency's lifecycle script can set `NODE_OPTIONS=--require=...` or put its own `git` on `PATH` and be running inside the step that holds the App key -- whichever file that step chose to execute. The App private key was reachable by a `postinstall`, one refusal list and one careful extraction later. So the credential lives on the other side of a job boundary. The first job merges, rebuilds and commits, then hands the result over as a git bundle. The second runs on a clean runner that has never executed anything from the pull request: it takes the bundle, checks that `main` has not moved, mints the token and pushes. It never runs `npm`. The base sha crosses with the bundle rather than being re-derived, for the same reason it was captured early: a workspace the rebuild ran in is a workspace the rebuild could have edited. Two comments were claiming more than the code does, and both are corrected rather than softened. The source-only filter is a scope filter, not a security control -- it rejects three path prefixes and does nothing about `package.json` scripts, a lockfile, `.npmrc` or a patched dependency, which are exactly what the rebuild executes. And `.github/workflows/` is in that list for tidiness: this workflow is loaded from the default branch, so a pull request cannot change the file that is running, and saying it could made the filter sound load-bearing. Limit: the window between the moved-main check and the push is narrowed rather than closed -- if `main` lands something in it, the opened pull request is red on its own `check` rather than silently wrong, because `build:canonical` there will not match Blast: system Undo: easy Certainty: firm Record-Id: r-jobboundary Provenance: authored Verified: collapsing the two jobs back into one fails three assertions -- the credential job runs contributor code, the bundle handover disappears, and the base sha stops crossing a boundary; 14 of 14 pass restored, and 79 across the four workflow suites CommitLore-Version: 2.0.0
…anual T-1502 asks that "a source-only pull request merges and the commit that lands on `main` passes ... without anyone rebuilding by hand". This job opens a second pull request, so the first half looked unmet -- and it is met by what lands rather than by what is clicked. The branch merges the contributor's head with `--no-ff`, so their commit is an ancestor of it. Merged with a merge commit, that commit lands on `main` and GitHub closes their pull request as merged. Squashed, new bytes land instead and their pull request stays open pointing at nothing. This repository allows both, so the method is part of the contract rather than a preference, and the bot now says so in the body it writes. A test pins both halves: the `--no-ff` and the instruction. The reason this is dispatched by hand also changed, and saying so matters more than the trigger does. It was security -- an automatic trigger would let a fork's push decide when the App token sits in an environment building that fork's code. The job split removed that: the token is never in the job that runs contributor code. What is left is cost, one Docker `npm ci` and a canonical build per dispatch, which a pull-request trigger would run on every push from anybody. Leaving the old reason in place would have been the worse outcome. A guard whose stated reason has been fixed elsewhere is one somebody removes later on the grounds that the reason no longer holds, without noticing it acquired a different one. And `without a maintainer` in PRD-F15's success line means without a maintainer *rebuilding* -- what #720 waited on twice. Nobody rebuilds here. Limit: the merge method is stated and asserted in the body, not enforced -- a squash merge of a canonical pull request still works, it just leaves the contributor's pull request open, and nothing fails when somebody does it Blast: system Undo: easy Certainty: firm Record-Id: r-mergemethod Provenance: authored Verified: `allow_merge_commit` is true on this repository, so the method is a choice rather than a constraint; replacing `--no-ff` with `--squash` fails the new assertion, and 15 of 15 pass restored CommitLore-Version: 2.0.0
|
Holding this out of the 1.1.3 release, on its own ticket's requirement rather than on caution.
This workflow has never run. The tests assert its safety properties from the file with comments stripped, and the guard has an e2e that builds real commits — but nothing here is the real merge that ticket asks for, and shipping it in a release would put an unexercised workflow in the tree that reads as working. That is #691's shape: 845 lines of installer that no install ran, which three readers in three days took as live.
What holding costs is only the status quo — #720 keeps waiting on a maintainer's rebuild — and that cost is already recorded on #719 with countable conditions. What it needs before mergingOne real dispatch against a real source-only pull request, with the run URL and the resulting canonical pull request linked here. Specifically: The last one has never been observed. Its shape does not exist in this repository's history — every pull request currently carries its own |
A blind refutation round on this branch broke three of the four claims I put to it, and verifying one of them found a defect it had not been looking for. `gh pr create` renders "GitHub closes #123 as merged" into the canonical pull request's body. GitHub binds a closing keyword to the number straight after it, and a pull request closed by keyword is recorded closed with `mergedAt` null -- the opposite of the sentence containing it, and the opposite of what T-1502 accepts. Measured on #752 six hours ago: an integration body said "GitHub closes #752, #755, #756 ... as merged", the keyword bound to #752 alone, and that one was recorded closed while the five with no keyword were recorded merged. There is no API to convert it afterwards. This workflow would have reproduced it on every run, and no test read the body. Two ticket statements were also wrong against the file. "The job never checks out or executes a pull request's head" was borrowed from the rule #723 fixed for `preserve`, which only reads a pull request; this one rebuilds it, and rebuilding somebody's change means running it. Unsatisfiable as written, so it would have been dropped rather than met -- what the job split actually holds is that the runner executing that code has no credential. And the negative control the ticket named, skipping `artifact:manifest`, cannot be performed from a pull request: the step is hard-coded in a workflow loaded from the default branch and the source-only filter refuses workflow edits. A negative control nobody can run is the defect it was written to prevent, so it is replaced with one that can be: edit `dist/` on the pushed canonical branch and watch `ci.yml` go red. Limit: the canonical pull request asks for a merge commit and cannot enforce one -- squash and rebase are both enabled and the button remembers the last method used, which is how #760 closed five of six as merged Blast: module Undo: easy Certainty: firm Record-Id: r-t1502body Provenance: authored Verified: restored the keyword and watched the new test fail naming `closes #123`, then restored the fix and saw 21 tests pass across both workflow test files CommitLore-Version: 2.0.0
A blind refutation round broke three of the four claims this branch rested onI put four claims to an independent reviewer with instructions to refute rather than approve, gave it a worktree checked out at this branch's head, and required it to report which files it actually opened. It read 19 files — the 9 it was given plus 10 it went looking for — and broke three. C1 — merging this before T-1502's observations exist is safe: could not refuteFour attacks failed: this branch does not touch C2 — "merge first, then observe" is forced: refutedI had written that merging a source-only pull request would trigger the rebuild. It does not. C3 — the plan produces T-1502's two observations: refutedThree separate errors, and the third is the one that matters. A source-only pull request cannot merge through its own button while C4 — no token leak and no execution of a pull request's head: half refutedThe token half survived four attacks: The execute-head half is false, and the file says so itself: rebuilding somebody's change means running it. That is a statement about the ticket, not about this workflow — see below. Verifying C4 found something none of the four claims was about
A pull request closed by keyword is recorded closed, This was measured six hours ago on #752: #760's body said "GitHub closes #752, #755, #756 … as merged", the keyword bound to #752 alone, and that one was recorded closed while the five with no keyword were recorded merged by reachability. Fixed, and a test now renders the Two ticket statements were wrong against the file"The job never checks out or executes a pull request's head." Borrowed from the rule #723 fixed for The negative control. Replaced with one that can be performed: after the workflow pushes One limitation recorded rather than fixedThe canonical pull request asks for a merge commit and cannot enforce one. Squash and rebase are both enabled and GitHub's button remembers the last method used. Recorded in the ticket with both remedies; choosing one is the owner's call, not this ticket's. |
Rebuilt after review. The first shape let a wrong commit reach
mainand opened a second pull request to fix it. That does not meet T-1502's acceptance — "a source-only pull request merges and the commit that lands onmainpassesartifact:verifyandgit diff --exit-code -- dist/without anyone rebuilding by hand" — because the commit that lands is the wrong one, briefly, every time. It was also building a file T-1502 owns while T-1501 puts "any workflow file" out of scope.What it does now
main+ the pull request's source + a canonical rebuild, on one branch, as one pull request. All eleven required contexts run on the tree that actually merges, rather than on one that resembles it. A contributor who cannot produce alinux/amd64Docker build waits for nothing — which is #720.The safety property with no precedent in this repository
The rebuild runs
npm ciandbuild:canonicalon the merged tree, which executes whatever the pull request put inpackage.json. So:The order of steps 4 and 6 is the whole guard, and
test/canonical-merge-workflow.test.tsasserts it with comment lines stripped, so the workflow's own explanation cannot satisfy the check (#723's shape).Three findings from the review, fixed rather than argued with
The concurrency group orders jobs, not merges. My own comment claimed it removed a race and then described that race. A rebuild is of
mainas it stood when the job started, and nothing stops another pull request landing while it runs. There is now a check between the rebuild and the push.A rebuild branch cannot be updated, only replaced.
mainisstrict, and.gitattributesmarksdist/**as-merge, so "Update branch" conflicts on the one file this job exists to produce. The branch is keyed to the pull request rather than to a commit, and a rerun force-pushes.The guard was blind on merge commits.
git show --name-onlyprints a merge's combined diff — only paths differing from every parent — so a clean merge reports nothing:Negative controls
Plus
test/canonical-rebuild-guard.test.ts: five cases, each a real repository with a real commit, and removing--first-parentfails the merge case. 79 tests across five workflow suites.Run by hand, deliberately
An automatic trigger would let a fork's push decide when this repository's App token sits in an environment building that fork's code. #723 is the record of why a workflow must not take direction from a pull request head.
Limit
Pull requests can still carry
dist/when they choose to. T-1503 is what stops requiring it; until then a source-only pull request is a contributor's option rather than the rule, and #719's success line ("no pull request carriesdist/") is not reached by this alone.