From d77a8dc65eae0558023aae07f3b5d640095f7e4e Mon Sep 17 00:00:00 2001 From: MongLong0214 Date: Tue, 18 Aug 2026 11:23:53 +0900 Subject: [PATCH 1/7] Let the App open the rebuild a contributor cannot produce `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 --- .github/workflows/canonical-rebuild.yml | 155 ++++++++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 .github/workflows/canonical-rebuild.yml diff --git a/.github/workflows/canonical-rebuild.yml b/.github/workflows/canonical-rebuild.yml new file mode 100644 index 00000000..17302b3e --- /dev/null +++ b/.github/workflows/canonical-rebuild.yml @@ -0,0 +1,155 @@ +name: Canonical rebuild + +# T-1501 (#719). `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. Today a contributor carries that by hand, and `build:canonical` is a +# `linux/amd64` Docker build — which is why #720 arrived from a Windows machine +# with `src/` and `test/` only, correctly, and waited on a maintainer twice. +# +# This closes that gap without moving any existing check. When a push to `main` +# leaves the committed bundle behind its source, the App opens a pull request +# carrying the rebuild. Every required context runs on it and it merges like +# anything else. +# +# It does not push to `main`. ADR-0036: `main` requires eleven contexts and the +# eleventh is `lint`, which no push can produce — `demo-lint.yml`'s push trigger +# is scoped to `dev` and its `lint` job is additionally gated on +# `github.event_name == 'pull_request'`. So a push-shaped design needs a bypass, +# and a bypass is not "the check passed"; it is "the check did not have to". +# Measured 2026-08-18: an App-opened pull request attaches all eleven +# (run 32082467906). +on: + push: + branches: [main] + +# The App does everything with its own token. This is only to read the push. +permissions: + contents: read + +# One at a time. Two pushes close together would each compute a rebuild of a +# tree the other invalidated, and the second would open a pull request against +# a `main` that had already moved. +concurrency: + group: canonical-rebuild + cancel-in-progress: false + +jobs: + rebuild: + runs-on: ubuntu-latest + timeout-minutes: 30 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + # Without this the checkout leaves an `http..extraheader` holding + # the Actions token, and that header beats credentials in a push URL — + # the App would be carrying a valid token and pushing as somebody else. + # Measured on this repository before #741 fixed the same line. + persist-credentials: false + + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: '22.23.2' + cache: npm + + - run: npm ci + + # The loop this has to not enter. A rebuild pull request touches exactly + # these two paths, so its own merge would otherwise ask for another + # rebuild of a tree that is already correct. + - name: Stop if this push is itself a rebuild + id: guard + run: | + set -euo pipefail + changed="$(git show --name-only --format= HEAD | sed '/^$/d' | sort -u)" + if [ -z "$changed" ]; then + # No paths at all. That is not "only artifacts" -- it is no evidence, + # and reading it as a skip would silently pass over a push that does + # need a rebuild. Fall through and let the build decide. + echo "--- this commit reports no changed paths; letting the build decide" + echo "skip=0" >> "$GITHUB_OUTPUT" + exit 0 + fi + only_artifacts=1 + while IFS= read -r path; do + case "$path" in + dist/*|installer/canonical-artifact.json) ;; + *) only_artifacts=0 ;; + esac + done <<< "$changed" + if [ "$only_artifacts" = "1" ]; then + echo "--- this commit touches only dist/ and the manifest; nothing to do" + echo "skip=1" >> "$GITHUB_OUTPUT" + else + echo "skip=0" >> "$GITHUB_OUTPUT" + fi + + - name: Rebuild and see whether the committed bundle is behind + id: build + if: steps.guard.outputs.skip == '0' + run: | + set -euo pipefail + npm run build:canonical + npm run artifact:manifest + npm run artifact:verify + if git diff --quiet -- dist/ installer/canonical-artifact.json; then + echo "--- the committed bundle already matches its source" + echo "needed=0" >> "$GITHUB_OUTPUT" + else + echo "--- the committed bundle is behind its source:" + git diff --stat -- dist/ installer/canonical-artifact.json + echo "needed=1" >> "$GITHUB_OUTPUT" + fi + + - name: Mint an installation token + id: token + if: steps.build.outputs.needed == '1' + env: + COMMITLORE_BOT_APP_ID: ${{ secrets.COMMITLORE_BOT_APP_ID }} + COMMITLORE_BOT_KEY: ${{ secrets.COMMITLORE_BOT_KEY }} + run: | + set -euo pipefail + token="$(node scripts/app-installation-token.mjs)" + echo "::add-mask::$token" + echo "token=$token" >> "$GITHUB_OUTPUT" + + - name: Open the rebuild as a pull request + if: steps.build.outputs.needed == '1' + env: + GH_TOKEN: ${{ steps.token.outputs.token }} + HEAD_SHA: ${{ github.sha }} + run: | + set -euo pipefail + short="$(git rev-parse --short "$HEAD_SHA")" + branch="canonical-rebuild/$short" + + # The bot user id, not the App id. A noreply address carrying the App + # id matches no account, and the commit is attributed to nobody (#743). + git config user.name 'commitlore-canonical-build[bot]' + git config user.email '317873099+commitlore-canonical-build[bot]@users.noreply.github.com' + git checkout -b "$branch" + git add dist/ installer/canonical-artifact.json + git commit --quiet -m "$(printf 'Rebuild the canonical bundle for %s\n\nThe committed bundle was behind its source after that commit merged, which is what happens when two source pull requests land close together: `.gitattributes` marks `dist/**` as `-diff -merge`, so the second one carries the bundle the first one left behind.\n\nThis is the rebuild, opened as a pull request rather than pushed. Every required context runs on it -- `main` requires eleven and the eleventh is `lint`, which no push to `main` can produce (ADR-0036), so a push-shaped rebuild would need a bypass and a bypass is not the check passing.\n\nLimit: this proves the bundle matches the source of the commit it was built from; whether that commit is the one a reader wants is what the pull request review is for\nBlast: system\nUndo: easy\nCertainty: firm\nRecord-Id: r-rebuild%s\nProvenance: authored\nVerified: `artifact:verify` passed against the regenerated manifest in the same job, and the two `build:canonical` runs in `check` compare this tree to itself on the pull request\nCommitLore-Version: 2.0.0' "$short" "$short")" + + git push --quiet "https://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" "$branch" + + # Who the server says committed it, before anything is opened on top. + # A push that silently used the wrong identity is the failure this + # read-back exists to catch, and it caught one (#743). + pusher="$(gh api "repos/${GITHUB_REPOSITORY}/commits/$(git rev-parse HEAD)" --jq '.committer.login // "unknown"')" + echo "--- pushed $branch; committer on the server: $pusher" + + gh pr create --base main --head "$branch" \ + --title "Rebuild the canonical bundle for $short" \ + --body "$(printf 'The committed bundle fell behind its source when [`%s`](https://github.com/%s/commit/%s) merged.\n\n`.gitattributes` marks `dist/**` as `-diff -merge`, so two source pull requests landing close together leave the second one carrying the bundle the first one left behind. This is the rebuild.\n\nOpened as a pull request rather than pushed: `main` requires eleven contexts and the eleventh is `lint`, which no push to `main` can produce (ADR-0036). A push-shaped rebuild would need a bypass, and a bypass is not the check passing.\n\nOpened by `canonical-rebuild.yml` for #719.' "$short" "$GITHUB_REPOSITORY" "$HEAD_SHA")" + + - name: Report + if: always() + run: | + if [ "${{ steps.guard.outputs.skip }}" = "1" ]; then + echo "skipped: the push was itself a rebuild" + elif [ "${{ steps.build.outputs.needed }}" = "0" ]; then + echo "nothing to do: the committed bundle matches its source" + else + echo "a rebuild pull request was opened" + fi From 1ef646ed541c8073520e0dfe989d954ab0f60bf6 Mon Sep 17 00:00:00 2001 From: MongLong0214 Date: Tue, 18 Aug 2026 11:30:43 +0900 Subject: [PATCH 2/7] Drive the rebuild guard from a test, and find out it was wrong 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 --- .github/workflows/canonical-rebuild.yml | 36 ++---- scripts/canonical-rebuild-guard.mjs | 76 ++++++++++++ test/canonical-rebuild-guard.test.ts | 146 ++++++++++++++++++++++++ 3 files changed, 234 insertions(+), 24 deletions(-) create mode 100644 scripts/canonical-rebuild-guard.mjs create mode 100644 test/canonical-rebuild-guard.test.ts diff --git a/.github/workflows/canonical-rebuild.yml b/.github/workflows/canonical-rebuild.yml index 17302b3e..c605039c 100644 --- a/.github/workflows/canonical-rebuild.yml +++ b/.github/workflows/canonical-rebuild.yml @@ -55,34 +55,22 @@ jobs: - run: npm ci # The loop this has to not enter. A rebuild pull request touches exactly - # these two paths, so its own merge would otherwise ask for another - # rebuild of a tree that is already correct. + # `dist/` and the manifest, so its own merge would otherwise ask for + # another rebuild of a tree that is already correct. + # + # The decision lives in a script rather than here because the shape it + # recognises does not exist in this repository yet -- every pull request + # currently carries its own `dist/`, which is #719's complaint. A guard + # with no observation behind it needs a test that drives the same code, + # and an inline `case` in YAML cannot be driven by one. The first draft + # was inline and read `bd297e1` as artifacts-only: it is a merge, and + # `git show` on a merge prints the combined diff, which lists only files + # that differ from *every* parent. Against its first parent it changed 30. - name: Stop if this push is itself a rebuild id: guard run: | set -euo pipefail - changed="$(git show --name-only --format= HEAD | sed '/^$/d' | sort -u)" - if [ -z "$changed" ]; then - # No paths at all. That is not "only artifacts" -- it is no evidence, - # and reading it as a skip would silently pass over a push that does - # need a rebuild. Fall through and let the build decide. - echo "--- this commit reports no changed paths; letting the build decide" - echo "skip=0" >> "$GITHUB_OUTPUT" - exit 0 - fi - only_artifacts=1 - while IFS= read -r path; do - case "$path" in - dist/*|installer/canonical-artifact.json) ;; - *) only_artifacts=0 ;; - esac - done <<< "$changed" - if [ "$only_artifacts" = "1" ]; then - echo "--- this commit touches only dist/ and the manifest; nothing to do" - echo "skip=1" >> "$GITHUB_OUTPUT" - else - echo "skip=0" >> "$GITHUB_OUTPUT" - fi + node scripts/canonical-rebuild-guard.mjs "$GITHUB_SHA" >> "$GITHUB_OUTPUT" - name: Rebuild and see whether the committed bundle is behind id: build diff --git a/scripts/canonical-rebuild-guard.mjs b/scripts/canonical-rebuild-guard.mjs new file mode 100644 index 00000000..c02fb060 --- /dev/null +++ b/scripts/canonical-rebuild-guard.mjs @@ -0,0 +1,76 @@ +/** + * Does this commit ask for a canonical rebuild, or is it one? (T-1501, #719) + * + * `canonical-rebuild.yml` opens a pull request carrying a rebuilt `dist/` when a + * push to `main` leaves the committed bundle behind its source. That pull + * request's own merge is a push to `main` touching exactly `dist/` and the + * manifest — so without this the feature answers itself forever. + * + * It lives here rather than inline in the workflow because the shape it has to + * recognise **does not exist in this repository's history**. Every pull request + * currently carries its own `dist/`, which is #719's complaint, so a + * squash-merged commit touching only the bundle is something this feature will + * create and nothing has yet. A guard whose only subject is its own output has + * no observation behind it, and #691 is the record of what that costs: "the + * test was measuring a copy of the logic rather than the copy that runs." + * The workflow calls this file; `test/canonical-rebuild-guard.test.ts` drives + * the same file against real commits it builds. + * + * Usage: node scripts/canonical-rebuild-guard.mjs [] [--cwd ] + * Prints `skip=1` or `skip=0` and exits 0. Exits 2 when it cannot decide. + */ + +import { spawnSync } from 'node:child_process'; + +const ARTIFACT_PATHS = [/^dist\//, /^installer\/canonical-artifact\.json$/]; + +/** + * The paths a commit changed, first-parent only. + * + * `--first-parent` matters: a merge commit's default diff is against every + * parent at once, and a merge that resolved only `dist/` — this repository has + * one, `bd297e1` — would report exactly the artifact paths while being nothing + * like a rebuild. Reading a merge as a rebuild is the direction that loops. + */ +export const changedPaths = (ref = 'HEAD', cwd = process.cwd()) => { + const run = spawnSync( + 'git', + ['show', '--first-parent', '--name-only', '--format=', ref], + { cwd, encoding: 'utf8', shell: false }, + ); + if (run.status !== 0) return null; + return [...new Set(run.stdout.split('\n').map((l) => l.trim()).filter(Boolean))].sort(); +}; + +/** + * Whether the workflow should stand down for this commit. + * + * An empty path list is deliberately **not** a skip. No paths is no evidence, + * and reading it as proof of a rebuild would silently pass over a push that + * needs one — the failure would be a `main` whose bundle never catches up, + * reported as a workflow that ran and found nothing to do. + */ +export const isRebuildCommit = (paths) => + paths !== null && paths.length > 0 && paths.every((p) => ARTIFACT_PATHS.some((r) => r.test(p))); + +if (import.meta.url === `file://${process.argv[1]}`) { + const args = process.argv.slice(2); + const cwdAt = args.indexOf('--cwd'); + const cwd = cwdAt >= 0 ? args[cwdAt + 1] : process.cwd(); + // The index to skip only exists when --cwd was given. Deriving it from -1 + // makes it 0, which silently drops a ref passed as the first argument -- the + // ordinary way to call this. + const skipIndex = cwdAt >= 0 ? cwdAt + 1 : -1; + const ref = args.find((a, i) => !a.startsWith('--') && i !== skipIndex) ?? 'HEAD'; + + const paths = changedPaths(ref, cwd); + if (paths === null) { + process.stderr.write(`canonical-rebuild-guard: cannot read ${ref}\n`); + process.exit(2); + } + const skip = isRebuildCommit(paths); + process.stderr.write( + `canonical-rebuild-guard: ${ref} touched ${paths.length} path(s)${paths.length ? `: ${paths.join(', ')}` : ''}\n`, + ); + process.stdout.write(`skip=${skip ? '1' : '0'}\n`); +} diff --git a/test/canonical-rebuild-guard.test.ts b/test/canonical-rebuild-guard.test.ts new file mode 100644 index 00000000..ca40498a --- /dev/null +++ b/test/canonical-rebuild-guard.test.ts @@ -0,0 +1,146 @@ +/** + * T-1501 (#719): the guard that stops the canonical rebuild from answering + * itself, driven against real commits rather than reasoned about. + * + * The subject it exists for **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`. + * + * So the shape is built here. Each case makes a real repository, produces the + * commit, and runs the same script the workflow runs — not a copy of its logic, + * which is the defect #691 removed 845 lines to end. + * + * One case is a regression rather than a hypothesis. The first draft of this + * guard was an inline `case` in the workflow reading `git show --name-only`, + * and against `bd297e1` — a merge in this repository that resolved `dist/` — + * it reported artifacts only and would have skipped. `git show` on a merge + * prints the *combined* diff, which lists only paths differing from every + * parent. Against its first parent that commit changed thirty files. + */ + +import { execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { afterAll, describe, expect, it } from 'vitest'; + +const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..'); +const GUARD = join(REPO_ROOT, 'scripts', 'canonical-rebuild-guard.mjs'); + +const scratch: string[] = []; +afterAll(() => { + for (const dir of scratch) rmSync(dir, { recursive: true, force: true }); +}); + +const git = (cwd: string, args: string[]): string => + execFileSync('git', args, { cwd, encoding: 'utf8', shell: false }); + +/** A repository with one ordinary source commit already on `main`. */ +const repo = (label: string): string => { + const dir = mkdtempSync(join(tmpdir(), `commitlore-${label}-`)); + scratch.push(dir); + git(dir, ['init', '--quiet', '--initial-branch=main']); + git(dir, ['config', 'user.email', 'guard@example.invalid']); + git(dir, ['config', 'user.name', 'guard']); + write(dir, 'src/cli.ts', 'export const a = 1;\n'); + write(dir, 'dist/commitlore.mjs', 'bundle v1\n'); + write(dir, 'installer/canonical-artifact.json', '{"sha256":"v1"}\n'); + git(dir, ['add', '-A']); + git(dir, ['commit', '--quiet', '-m', 'base']); + return dir; +}; + +const write = (dir: string, path: string, body: string): void => { + mkdirSync(dirname(join(dir, path)), { recursive: true }); + writeFileSync(join(dir, path), body); +}; + +const guard = (dir: string, ref = 'HEAD'): string => + execFileSync(process.execPath, [GUARD, ref, '--cwd', dir], { + cwd: dir, + encoding: 'utf8', + shell: false, + }).trim(); + +describe('#719 the rebuild guard, against commits it actually has to read', () => { + it('skips a squash-merged rebuild — the shape this feature creates', () => { + const dir = repo('guard-rebuild'); + // Exactly what `canonical-rebuild.yml` opens, merged the way this + // repository merges: squashed onto main as a single-parent commit. + git(dir, ['checkout', '--quiet', '-b', 'canonical-rebuild/abc1234']); + write(dir, 'dist/commitlore.mjs', 'bundle v2\n'); + write(dir, 'installer/canonical-artifact.json', '{"sha256":"v2"}\n'); + git(dir, ['add', 'dist/', 'installer/canonical-artifact.json']); + git(dir, ['commit', '--quiet', '-m', 'Rebuild the canonical bundle for abc1234']); + git(dir, ['checkout', '--quiet', 'main']); + git(dir, ['merge', '--squash', 'canonical-rebuild/abc1234']); + git(dir, ['commit', '--quiet', '-m', 'Rebuild the canonical bundle for abc1234 (#999)']); + + expect(guard(dir)).toBe('skip=1'); + }); + + it('does not skip a source change that carries its bundle — today\'s ordinary commit', () => { + const dir = repo('guard-source'); + write(dir, 'src/cli.ts', 'export const a = 2;\n'); + write(dir, 'dist/commitlore.mjs', 'bundle v2\n'); + write(dir, 'installer/canonical-artifact.json', '{"sha256":"v2"}\n'); + git(dir, ['add', '-A']); + git(dir, ['commit', '--quiet', '-m', 'a source change with its rebuild']); + + expect(guard(dir)).toBe('skip=0'); + }); + + it('does not skip a source-only change — the case the feature exists for', () => { + const dir = repo('guard-srconly'); + write(dir, 'src/cli.ts', 'export const a = 3;\n'); + git(dir, ['add', '-A']); + git(dir, ['commit', '--quiet', '-m', 'source only, bundle now behind']); + + expect(guard(dir)).toBe('skip=0'); + }); + + it('does not skip a merge that only resolved the bundle — bd297e1\'s shape', () => { + // The regression. `git show` without `--first-parent` prints a merge's + // combined diff, which lists only paths differing from every parent — so a + // merge whose conflict resolution touched `dist/` reads as artifacts-only. + // Reading that as a rebuild is the direction that loops. + const dir = repo('guard-merge'); + git(dir, ['checkout', '--quiet', '-b', 'feature']); + write(dir, 'src/cli.ts', 'export const a = 4;\n'); + write(dir, 'dist/commitlore.mjs', 'bundle feature\n'); + git(dir, ['add', '-A']); + git(dir, ['commit', '--quiet', '-m', 'feature work']); + + git(dir, ['checkout', '--quiet', 'main']); + write(dir, 'dist/commitlore.mjs', 'bundle main\n'); + git(dir, ['add', '-A']); + git(dir, ['commit', '--quiet', '-m', 'main moved']); + + // Merge with a conflict in dist/, resolved to one side. + try { + git(dir, ['merge', '--no-commit', '--no-ff', 'feature']); + } catch { + // expected: the bundle conflicts + } + write(dir, 'dist/commitlore.mjs', 'bundle merged\n'); + git(dir, ['add', '-A']); + git(dir, ['commit', '--quiet', '--no-edit', '-m', 'Merge feature']); + + expect(guard(dir)).toBe('skip=0'); + }); + + it('does not skip a commit with no paths at all', () => { + // No paths is no evidence, not proof of a rebuild. Reading it as a skip + // would pass over a push that needs one, and the failure would be a `main` + // whose bundle never catches up, reported as "nothing to do". + const dir = repo('guard-empty'); + git(dir, ['commit', '--quiet', '--allow-empty', '-m', 'an empty commit']); + + expect(guard(dir)).toBe('skip=0'); + }); +}); From 47c8ed1e933afc3a5a55955b833b2912472117e0 Mon Sep 17 00:00:00 2001 From: MongLong0214 Date: Tue, 18 Aug 2026 11:43:07 +0900 Subject: [PATCH 3/7] Build the commit that lands, instead of repairing it afterwards 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 --- .github/workflows/canonical-merge.yml | 168 ++++++++++++++++++++++++ .github/workflows/canonical-rebuild.yml | 143 -------------------- test/canonical-merge-workflow.test.ts | 120 +++++++++++++++++ 3 files changed, 288 insertions(+), 143 deletions(-) create mode 100644 .github/workflows/canonical-merge.yml delete mode 100644 .github/workflows/canonical-rebuild.yml create mode 100644 test/canonical-merge-workflow.test.ts diff --git a/.github/workflows/canonical-merge.yml b/.github/workflows/canonical-merge.yml new file mode 100644 index 00000000..79ea5524 --- /dev/null +++ b/.github/workflows/canonical-merge.yml @@ -0,0 +1,168 @@ +name: Canonical merge + +# T-1502 (#719). A contributor opens a pull request carrying source only. This +# builds the commit that will land: main + that source + a canonical rebuild, +# as its own pull request, so all eleven required contexts run on the tree that +# actually merges rather than on one that resembles it. +# +# #720 is what this is for. `build:canonical` is a `linux/amd64` Docker build, +# and that pull request arrived from a Windows machine with `src/` and `test/` +# only -- correctly -- and waited on a maintainer twice. +# +# It does not push to `main`. ADR-0036: `main` requires eleven contexts and the +# eleventh is `lint`, which no push can produce -- `demo-lint.yml`'s push +# trigger is scoped to `dev` and its `lint` job is gated on +# `github.event_name == 'pull_request'`. Every push-shaped design needs a +# bypass, and a bypass is not the check passing. Measured 2026-08-18: an +# App-opened pull request attaches all eleven (run 32082467906). +# +# Run by hand. An automatic trigger on pull-request events would make a fork's +# first push decide when this repository's App token is in an environment +# building that fork's code, and #723 is the record of why a workflow must not +# take direction from a pull request head. +on: + workflow_dispatch: + inputs: + pull_request_number: + description: The source-only pull request to canonicalise + required: true + type: string + +permissions: + contents: read + +# One at a time. Each rebuild is of `main` as it stands when the job starts, so +# two running together would each produce a tree the other invalidates. This +# orders the jobs; it does not order the merges, and the staleness check below +# is what covers the gap. +concurrency: + group: canonical-merge + cancel-in-progress: false + +jobs: + canonicalise: + runs-on: ubuntu-latest + timeout-minutes: 40 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: main + fetch-depth: 0 + # The checkout otherwise leaves an `http..extraheader` holding the + # Actions token, and that header beats credentials in a push URL -- a + # valid App token then pushes as `github-actions[bot]` and is refused. + # Measured on this repository (#741). + persist-credentials: false + + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: '22.23.2' + cache: npm + + # Fetched, not checked out. Nothing from the pull request runs until the + # source-only check below has passed. + - name: Fetch the pull request without running it + id: fetch + env: + GH_TOKEN: ${{ github.token }} + PR: ${{ inputs.pull_request_number }} + run: | + set -euo pipefail + head="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR}" --jq .head.sha)" + base="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR}" --jq .base.ref)" + [ "$base" = "main" ] || { echo "::error::#${PR} targets $base, not main"; exit 1; } + git fetch --quiet origin "pull/${PR}/head:pr-${PR}" + echo "head=$head" >> "$GITHUB_OUTPUT" + echo "--- #${PR} head $head" + + # The one refusal that has to happen before anything from the pull request + # is executed. A pull request that carries `dist/` is not source-only and + # canonicalising it would rebuild on top of somebody else's bundle; one + # that carries a workflow is asking this job to run a file it wrote. + - name: Refuse anything that is not a source-only pull request + run: | + set -euo pipefail + changed="$(git diff --name-only main..."pr-${{ inputs.pull_request_number }}")" + echo "--- changed paths:"; echo "$changed" | sed 's/^/ /' + bad="$(echo "$changed" | grep -E '^(dist/|installer/canonical-artifact\.json$|\.github/workflows/)' || true)" + if [ -n "$bad" ]; then + echo "::error::not source-only -- refusing:"; echo "$bad" | sed 's/^/ /' + exit 1 + fi + + - name: Merge the source onto main + run: | + set -euo pipefail + git config user.name 'commitlore-canonical-build[bot]' + git config user.email '317873099+commitlore-canonical-build[bot]@users.noreply.github.com' + git checkout -q -b "canonical/pr-${{ inputs.pull_request_number }}" + git merge --no-ff --no-edit "pr-${{ inputs.pull_request_number }}" + + # This step executes the pull request's own `package.json` and build + # scripts. The App token is deliberately not minted yet and is not in this + # environment: a rebuild of somebody else's change must not be able to + # read the credential that would let it push. + - name: Rebuild the bundle from the merged tree + run: | + set -euo pipefail + npm ci + npm run build:canonical + npm run artifact:manifest + npm run artifact:verify + git add dist/ installer/canonical-artifact.json + if git diff --cached --quiet; then + echo "--- the merged source produces the committed bundle; nothing to add" + else + git commit --quiet -m "$(printf 'Rebuild the canonical bundle for #%s\n\n`build:canonical` on the merged tree, so the commit that lands matches the source it lands with. The pull request carried source only, which is what a contributor on a host that cannot run a linux/amd64 Docker build can produce (#720).\n\nLimit: this proves the bundle matches this tree; whether this tree is what a reviewer wants is what the pull request is for\nBlast: system\nUndo: easy\nCertainty: firm\nRecord-Id: r-canonmerge%s\nProvenance: authored\nVerified: artifact:verify passed against the regenerated manifest in the same job, before any credential was available to it\nCommitLore-Version: 2.0.0' "${{ inputs.pull_request_number }}" "${{ inputs.pull_request_number }}")" + fi + + # `main` may have moved while the rebuild ran, and a rebuild of a tree that + # is no longer main's is exactly what the concurrency group cannot prevent + # -- it orders jobs, not merges. Checked here rather than hoped for. + - name: Refuse if main moved while this ran + env: + GH_TOKEN: ${{ github.token }} + run: | + set -euo pipefail + now="$(gh api "repos/${GITHUB_REPOSITORY}/commits/main" --jq .sha)" + started="$(git rev-parse "origin/main")" + if [ "$now" != "$started" ]; then + echo "::error::main moved from $started to $now while this rebuilt -- rerun after it settles" + exit 1 + fi + echo "--- main is still $now" + + - name: Mint an installation token + id: token + env: + COMMITLORE_BOT_APP_ID: ${{ secrets.COMMITLORE_BOT_APP_ID }} + COMMITLORE_BOT_KEY: ${{ secrets.COMMITLORE_BOT_KEY }} + run: | + set -euo pipefail + token="$(node scripts/app-installation-token.mjs)" + echo "::add-mask::$token" + echo "token=$token" >> "$GITHUB_OUTPUT" + + - name: Open the canonical pull request + env: + GH_TOKEN: ${{ steps.token.outputs.token }} + PR: ${{ inputs.pull_request_number }} + run: | + set -euo pipefail + branch="canonical/pr-${PR}" + # Force, because a rerun after `main` moved has to replace the branch + # rather than update it: `.gitattributes` marks `dist/**` as `-merge`, + # so an update would conflict on the one file this exists to produce. + git push --quiet --force "https://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" "$branch" + + pusher="$(gh api "repos/${GITHUB_REPOSITORY}/commits/$(git rev-parse HEAD)" --jq '.committer.login // "unknown"')" + echo "--- pushed $branch; committer on the server: $pusher" + + existing="$(gh pr list --head "$branch" --state open --json number --jq '.[0].number // ""')" + if [ -n "$existing" ]; then + echo "--- #$existing already open for $branch; the force-push updated it" + else + gh pr create --base main --head "$branch" \ + --title "Canonical merge of #${PR}" \ + --body "$(printf 'The commit that will land for #%s: `main` plus that source plus a canonical rebuild, built together so all eleven required contexts run on the tree that merges rather than on one that resembles it.\n\n#%s carries source only, which is what a contributor on a host that cannot run a `linux/amd64` Docker build can produce (#720). Nothing was rebuilt by hand.\n\nOpened by `canonical-merge.yml` for #719. Close #%s when this merges.' "$PR" "$PR" "$PR")" + fi diff --git a/.github/workflows/canonical-rebuild.yml b/.github/workflows/canonical-rebuild.yml deleted file mode 100644 index c605039c..00000000 --- a/.github/workflows/canonical-rebuild.yml +++ /dev/null @@ -1,143 +0,0 @@ -name: Canonical rebuild - -# T-1501 (#719). `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. Today a contributor carries that by hand, and `build:canonical` is a -# `linux/amd64` Docker build — which is why #720 arrived from a Windows machine -# with `src/` and `test/` only, correctly, and waited on a maintainer twice. -# -# This closes that gap without moving any existing check. When a push to `main` -# leaves the committed bundle behind its source, the App opens a pull request -# carrying the rebuild. Every required context runs on it and it merges like -# anything else. -# -# It does not push to `main`. ADR-0036: `main` requires eleven contexts and the -# eleventh is `lint`, which no push can produce — `demo-lint.yml`'s push trigger -# is scoped to `dev` and its `lint` job is additionally gated on -# `github.event_name == 'pull_request'`. So a push-shaped design needs a bypass, -# and a bypass is not "the check passed"; it is "the check did not have to". -# Measured 2026-08-18: an App-opened pull request attaches all eleven -# (run 32082467906). -on: - push: - branches: [main] - -# The App does everything with its own token. This is only to read the push. -permissions: - contents: read - -# One at a time. Two pushes close together would each compute a rebuild of a -# tree the other invalidated, and the second would open a pull request against -# a `main` that had already moved. -concurrency: - group: canonical-rebuild - cancel-in-progress: false - -jobs: - rebuild: - runs-on: ubuntu-latest - timeout-minutes: 30 - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - fetch-depth: 0 - # Without this the checkout leaves an `http..extraheader` holding - # the Actions token, and that header beats credentials in a push URL — - # the App would be carrying a valid token and pushing as somebody else. - # Measured on this repository before #741 fixed the same line. - persist-credentials: false - - - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 - with: - node-version: '22.23.2' - cache: npm - - - run: npm ci - - # The loop this has to not enter. A rebuild pull request touches exactly - # `dist/` and the manifest, so its own merge would otherwise ask for - # another rebuild of a tree that is already correct. - # - # The decision lives in a script rather than here because the shape it - # recognises does not exist in this repository yet -- every pull request - # currently carries its own `dist/`, which is #719's complaint. A guard - # with no observation behind it needs a test that drives the same code, - # and an inline `case` in YAML cannot be driven by one. The first draft - # was inline and read `bd297e1` as artifacts-only: it is a merge, and - # `git show` on a merge prints the combined diff, which lists only files - # that differ from *every* parent. Against its first parent it changed 30. - - name: Stop if this push is itself a rebuild - id: guard - run: | - set -euo pipefail - node scripts/canonical-rebuild-guard.mjs "$GITHUB_SHA" >> "$GITHUB_OUTPUT" - - - name: Rebuild and see whether the committed bundle is behind - id: build - if: steps.guard.outputs.skip == '0' - run: | - set -euo pipefail - npm run build:canonical - npm run artifact:manifest - npm run artifact:verify - if git diff --quiet -- dist/ installer/canonical-artifact.json; then - echo "--- the committed bundle already matches its source" - echo "needed=0" >> "$GITHUB_OUTPUT" - else - echo "--- the committed bundle is behind its source:" - git diff --stat -- dist/ installer/canonical-artifact.json - echo "needed=1" >> "$GITHUB_OUTPUT" - fi - - - name: Mint an installation token - id: token - if: steps.build.outputs.needed == '1' - env: - COMMITLORE_BOT_APP_ID: ${{ secrets.COMMITLORE_BOT_APP_ID }} - COMMITLORE_BOT_KEY: ${{ secrets.COMMITLORE_BOT_KEY }} - run: | - set -euo pipefail - token="$(node scripts/app-installation-token.mjs)" - echo "::add-mask::$token" - echo "token=$token" >> "$GITHUB_OUTPUT" - - - name: Open the rebuild as a pull request - if: steps.build.outputs.needed == '1' - env: - GH_TOKEN: ${{ steps.token.outputs.token }} - HEAD_SHA: ${{ github.sha }} - run: | - set -euo pipefail - short="$(git rev-parse --short "$HEAD_SHA")" - branch="canonical-rebuild/$short" - - # The bot user id, not the App id. A noreply address carrying the App - # id matches no account, and the commit is attributed to nobody (#743). - git config user.name 'commitlore-canonical-build[bot]' - git config user.email '317873099+commitlore-canonical-build[bot]@users.noreply.github.com' - git checkout -b "$branch" - git add dist/ installer/canonical-artifact.json - git commit --quiet -m "$(printf 'Rebuild the canonical bundle for %s\n\nThe committed bundle was behind its source after that commit merged, which is what happens when two source pull requests land close together: `.gitattributes` marks `dist/**` as `-diff -merge`, so the second one carries the bundle the first one left behind.\n\nThis is the rebuild, opened as a pull request rather than pushed. Every required context runs on it -- `main` requires eleven and the eleventh is `lint`, which no push to `main` can produce (ADR-0036), so a push-shaped rebuild would need a bypass and a bypass is not the check passing.\n\nLimit: this proves the bundle matches the source of the commit it was built from; whether that commit is the one a reader wants is what the pull request review is for\nBlast: system\nUndo: easy\nCertainty: firm\nRecord-Id: r-rebuild%s\nProvenance: authored\nVerified: `artifact:verify` passed against the regenerated manifest in the same job, and the two `build:canonical` runs in `check` compare this tree to itself on the pull request\nCommitLore-Version: 2.0.0' "$short" "$short")" - - git push --quiet "https://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" "$branch" - - # Who the server says committed it, before anything is opened on top. - # A push that silently used the wrong identity is the failure this - # read-back exists to catch, and it caught one (#743). - pusher="$(gh api "repos/${GITHUB_REPOSITORY}/commits/$(git rev-parse HEAD)" --jq '.committer.login // "unknown"')" - echo "--- pushed $branch; committer on the server: $pusher" - - gh pr create --base main --head "$branch" \ - --title "Rebuild the canonical bundle for $short" \ - --body "$(printf 'The committed bundle fell behind its source when [`%s`](https://github.com/%s/commit/%s) merged.\n\n`.gitattributes` marks `dist/**` as `-diff -merge`, so two source pull requests landing close together leave the second one carrying the bundle the first one left behind. This is the rebuild.\n\nOpened as a pull request rather than pushed: `main` requires eleven contexts and the eleventh is `lint`, which no push to `main` can produce (ADR-0036). A push-shaped rebuild would need a bypass, and a bypass is not the check passing.\n\nOpened by `canonical-rebuild.yml` for #719.' "$short" "$GITHUB_REPOSITORY" "$HEAD_SHA")" - - - name: Report - if: always() - run: | - if [ "${{ steps.guard.outputs.skip }}" = "1" ]; then - echo "skipped: the push was itself a rebuild" - elif [ "${{ steps.build.outputs.needed }}" = "0" ]; then - echo "nothing to do: the committed bundle matches its source" - else - echo "a rebuild pull request was opened" - fi diff --git a/test/canonical-merge-workflow.test.ts b/test/canonical-merge-workflow.test.ts new file mode 100644 index 00000000..fa23e35f --- /dev/null +++ b/test/canonical-merge-workflow.test.ts @@ -0,0 +1,120 @@ +/** + * T-1502 (#719): the safety properties of `canonical-merge.yml`, read from the + * file rather than trusted. + * + * Comment lines are stripped before every assertion. The workflow explains + * itself at length and a property satisfied by its own explanation is not + * satisfied — that is the shape `test/preserve-workflow-safety.test.ts` + * established for the same reason (#723). + * + * The property that matters most here has no equivalent in that file. This job + * rebuilds a contributor's change, which means it executes their `package.json` + * and their build scripts. If the App token were minted before that step, a + * pull request could read the credential that lets it push to this repository. + * The order of two steps is the whole guard. + */ + +import { readFileSync } from 'node:fs'; +import { join, resolve, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { describe, expect, it } from 'vitest'; + +const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..'); +const PATH = join(REPO_ROOT, '.github', 'workflows', 'canonical-merge.yml'); + +/** The workflow with every comment line removed, so prose cannot satisfy a check. */ +const code = (): string => + readFileSync(PATH, 'utf8') + .split('\n') + .filter((line) => !/^\s*#/.test(line)) + .join('\n'); + +const lineOf = (needle: string): number => { + const index = code().split('\n').findIndex((l) => l.includes(needle)); + expect(index, `not found in the workflow: ${needle}`).toBeGreaterThanOrEqual(0); + return index; +}; + +describe('T-1502 canonical-merge.yml safety', () => { + it('mints the App token only after the contributor code has already run', () => { + // The rebuild runs `npm ci` and `build:canonical` on the merged tree, which + // executes whatever the pull request put in `package.json`. Minting first + // would put a credential that can push to this repository into that + // environment. + expect(lineOf('build:canonical')).toBeLessThan(lineOf('app-installation-token.mjs')); + }); + + it('does not put the App token in the rebuild step', () => { + const body = code(); + const rebuild = body.slice(body.indexOf('Rebuild the bundle'), body.indexOf('Refuse if main moved')); + expect(rebuild).not.toContain('COMMITLORE_BOT_KEY'); + expect(rebuild).not.toContain('steps.token.outputs.token'); + }); + + it('never echoes the token', () => { + const body = code(); + expect(body).toContain('::add-mask::'); + // The only permitted use is the masked capture and the push URL. + for (const line of body.split('\n')) { + if (!line.includes('steps.token.outputs.token')) continue; + expect(line).toMatch(/GH_TOKEN:|x-access-token/); + } + }); + + it('refuses a pull request that is not source-only, before executing it', () => { + const body = code(); + expect(body).toMatch(/\^\(dist\/\|installer\/canonical-artifact/); + expect(body).toContain('.github/workflows/'); + expect(lineOf('Refuse anything that is not a source-only')).toBeLessThan( + lineOf('Rebuild the bundle'), + ); + }); + + it('rebuilds with build:canonical rather than a local build', () => { + // A local `npm run build` produces bytes from whatever platform the runner + // is, and the committed bundle is defined by the Docker build. + expect(code()).toContain('npm run build:canonical'); + expect(code()).toContain('npm run artifact:verify'); + }); + + it('checks out main, never the pull request head', () => { + const body = code(); + expect(body).toMatch(/ref:\s*main/); + // Fetched into a local ref and merged; never checked out and run directly. + expect(body).toContain('git fetch --quiet origin "pull/${PR}/head:pr-${PR}"'); + expect(body).not.toMatch(/ref:\s*\$\{\{\s*github\.event\.pull_request\.head/); + }); + + it('does not persist the Actions credential over the App token', () => { + expect(code()).toContain('persist-credentials: false'); + }); + + it('refuses when main moved while the rebuild ran', () => { + // The concurrency group orders jobs, not merges: a rebuild is of `main` as + // it stood when the job started, and nothing stops another pull request + // landing in between. Without this the job would open a pull request whose + // bundle is of a tree that is no longer anybody's. + const body = code(); + expect(body).toContain('Refuse if main moved'); + expect(lineOf('Refuse if main moved')).toBeLessThan(lineOf('app-installation-token.mjs')); + }); + + it('replaces its branch rather than updating it', () => { + // `.gitattributes` marks `dist/**` as `-merge`, so an update would conflict + // on the one file this job exists to produce. A rerun force-pushes. + expect(code()).toMatch(/git push --quiet --force/); + }); + + it('requests no permission beyond reading', () => { + expect(code()).toMatch(/permissions:\s*\n\s*contents:\s*read/); + }); + + it('is run by hand, not by a pull request event', () => { + // An automatic trigger would let a fork's push decide when this + // repository's App token is in an environment building that fork's code. + const body = code(); + expect(body).toMatch(/on:\s*\n\s*workflow_dispatch:/); + expect(body).not.toMatch(/^\s*pull_request(_target)?:/m); + }); +}); From d7239185ec5dcdfccba642d1e5e52e8a9b981add Mon Sep 17 00:00:00 2001 From: MongLong0214 Date: Tue, 18 Aug 2026 11:48:23 +0900 Subject: [PATCH 4/7] Take the credential path off the tree the pull request can edit 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 :` 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 --- .github/workflows/canonical-merge.yml | 20 ++++++++++++++++++-- test/canonical-merge-workflow.test.ts | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/.github/workflows/canonical-merge.yml b/.github/workflows/canonical-merge.yml index 79ea5524..237773bc 100644 --- a/.github/workflows/canonical-merge.yml +++ b/.github/workflows/canonical-merge.yml @@ -91,8 +91,12 @@ jobs: fi - name: Merge the source onto main + id: merge run: | set -euo pipefail + # Captured before the rebuild runs anything. Reading it afterwards + # would be reading a value the rebuild had the opportunity to choose. + echo "base=$(git rev-parse origin/main)" >> "$GITHUB_OUTPUT" git config user.name 'commitlore-canonical-build[bot]' git config user.email '317873099+commitlore-canonical-build[bot]@users.noreply.github.com' git checkout -q -b "canonical/pr-${{ inputs.pull_request_number }}" @@ -125,7 +129,7 @@ jobs: run: | set -euo pipefail now="$(gh api "repos/${GITHUB_REPOSITORY}/commits/main" --jq .sha)" - started="$(git rev-parse "origin/main")" + started="${{ steps.merge.outputs.base }}" if [ "$now" != "$started" ]; then echo "::error::main moved from $started to $now while this rebuilt -- rerun after it settles" exit 1 @@ -139,7 +143,19 @@ jobs: COMMITLORE_BOT_KEY: ${{ secrets.COMMITLORE_BOT_KEY }} run: | set -euo pipefail - token="$(node scripts/app-installation-token.mjs)" + # From `main`, never from the merged tree. The rebuild above executed + # the pull request's `package.json` and every dependency lifecycle + # script it pulled in, and the merged tree also contains the pull + # request's version of this very file -- `scripts/` is source, and a + # source-only pull request may change it. Running the merged copy here + # would hand the App private key to whatever that copy is. + # + # Extracted to a path outside the workspace and run from there, so the + # workspace's `node_modules` cannot be resolved by it either. The + # script itself imports only `node:crypto` and uses global `fetch`. + mint="$(mktemp -d)/mint.mjs" + git show "${{ steps.merge.outputs.base }}:scripts/app-installation-token.mjs" > "$mint" + token="$(cd "$(dirname "$mint")" && NODE_OPTIONS= node "$mint")" echo "::add-mask::$token" echo "token=$token" >> "$GITHUB_OUTPUT" diff --git a/test/canonical-merge-workflow.test.ts b/test/canonical-merge-workflow.test.ts index fa23e35f..13a59f2f 100644 --- a/test/canonical-merge-workflow.test.ts +++ b/test/canonical-merge-workflow.test.ts @@ -86,6 +86,24 @@ describe('T-1502 canonical-merge.yml safety', () => { expect(body).not.toMatch(/ref:\s*\$\{\{\s*github\.event\.pull_request\.head/); }); + it('runs the credential script from main, never from the merged tree', () => { + // The sharpest hole the first version had. `scripts/` is source, so a + // source-only pull request may change `app-installation-token.mjs` — and + // the step that runs it has `COMMITLORE_BOT_KEY` in its environment. + // Running the merged copy would hand the App private key to whatever the + // pull request made that file into, without needing a `postinstall` at all. + const body = code(); + expect(body).toMatch(/git show "\$\{\{ steps\.merge\.outputs\.base \}\}:scripts\/app-installation-token\.mjs"/); + // And not from the workspace. + expect(body).not.toMatch(/node scripts\/app-installation-token\.mjs/); + }); + + it('reads main\'s sha before the rebuild, not after', () => { + // The rebuild executes contributor code in this workspace, so a value read + // from `.git` afterwards is a value that code had the chance to choose. + expect(lineOf('base=$(git rev-parse origin/main)')).toBeLessThan(lineOf('build:canonical')); + }); + it('does not persist the Actions credential over the App token', () => { expect(code()).toContain('persist-credentials: false'); }); From 38e20309b07faeb729fdee484e05d1d65b7d7733 Mon Sep 17 00:00:00 2001 From: MongLong0214 Date: Tue, 18 Aug 2026 11:53:45 +0900 Subject: [PATCH 5/7] Put a job boundary where the step order was pretending to be one 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 --- .github/workflows/canonical-merge.yml | 86 +++++++++++++++++++++++++-- test/canonical-merge-workflow.test.ts | 37 +++++++++--- 2 files changed, 109 insertions(+), 14 deletions(-) diff --git a/.github/workflows/canonical-merge.yml b/.github/workflows/canonical-merge.yml index 237773bc..5a25f934 100644 --- a/.github/workflows/canonical-merge.yml +++ b/.github/workflows/canonical-merge.yml @@ -75,10 +75,16 @@ jobs: echo "head=$head" >> "$GITHUB_OUTPUT" echo "--- #${PR} head $head" - # The one refusal that has to happen before anything from the pull request - # is executed. A pull request that carries `dist/` is not source-only and - # canonicalising it would rebuild on top of somebody else's bundle; one - # that carries a workflow is asking this job to run a file it wrote. + # A scope filter, not a security control, and the difference matters. + # It rejects three path prefixes. It does not stop a pull request from + # running code during the rebuild -- `package.json` scripts, a lockfile + # pointing anywhere, `.npmrc`, a patched dependency -- and nothing here + # can, because rebuilding somebody's change means running it. What keeps + # that contained is the job split below, not this list. + # + # `.github/workflows/` is in it for tidiness rather than safety: this + # workflow is loaded from the default branch, so a pull request cannot + # change the file that is running. - name: Refuse anything that is not a source-only pull request run: | set -euo pipefail @@ -136,6 +142,74 @@ jobs: fi echo "--- main is still $now" + - name: Hand the result over as a bundle + run: | + set -euo pipefail + git bundle create /tmp/canonical.bundle main.."canonical/pr-${{ inputs.pull_request_number }}" + echo "${{ steps.merge.outputs.base }}" > /tmp/canonical.base + + - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: canonical-tree + path: | + /tmp/canonical.bundle + /tmp/canonical.base + retention-days: 1 + + # A second runner that has never executed anything from the pull request. + # + # The order of steps inside one job is not a boundary. `$GITHUB_ENV` and + # `$GITHUB_PATH` written during `npm ci` persist into every later step of the + # same job, so a dependency's lifecycle script can set `NODE_OPTIONS` or put a + # `git` of its own on `PATH` and be running inside the step that holds the App + # key -- whichever file that step chose to execute. Extracting the credential + # script from `main` fixed *what* ran and not *how it was launched*. + # + # So the credential lives on the other side of a job boundary. This job never + # runs `npm`, never runs contributor code, and only moves bytes it verifies. + publish: + needs: canonicalise + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: main + fetch-depth: 0 + persist-credentials: false + + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: '22.23.2' + + - uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 + with: + name: canonical-tree + path: /tmp/canonical + + - name: Take the branch from the bundle + id: take + run: | + set -euo pipefail + base="$(cat /tmp/canonical/canonical.base)" + branch="canonical/pr-${{ inputs.pull_request_number }}" + git fetch --quiet /tmp/canonical/canonical.bundle "$branch:$branch" + echo "base=$base" >> "$GITHUB_OUTPUT" + echo "branch=$branch" >> "$GITHUB_OUTPUT" + echo "--- took $branch from the bundle, built on $base" + + - name: Refuse if main moved while the rebuild ran + env: + GH_TOKEN: ${{ github.token }} + run: | + set -euo pipefail + now="$(gh api "repos/${GITHUB_REPOSITORY}/commits/main" --jq .sha)" + if [ "$now" != "${{ steps.take.outputs.base }}" ]; then + echo "::error::main moved from ${{ steps.take.outputs.base }} to $now while this rebuilt -- rerun after it settles" + exit 1 + fi + echo "--- main is still $now" + - name: Mint an installation token id: token env: @@ -154,7 +228,7 @@ jobs: # workspace's `node_modules` cannot be resolved by it either. The # script itself imports only `node:crypto` and uses global `fetch`. mint="$(mktemp -d)/mint.mjs" - git show "${{ steps.merge.outputs.base }}:scripts/app-installation-token.mjs" > "$mint" + git show "${{ steps.take.outputs.base }}:scripts/app-installation-token.mjs" > "$mint" token="$(cd "$(dirname "$mint")" && NODE_OPTIONS= node "$mint")" echo "::add-mask::$token" echo "token=$token" >> "$GITHUB_OUTPUT" @@ -165,7 +239,7 @@ jobs: PR: ${{ inputs.pull_request_number }} run: | set -euo pipefail - branch="canonical/pr-${PR}" + branch="${{ steps.take.outputs.branch }}" # Force, because a rerun after `main` moved has to replace the branch # rather than update it: `.gitattributes` marks `dist/**` as `-merge`, # so an update would conflict on the one file this exists to produce. diff --git a/test/canonical-merge-workflow.test.ts b/test/canonical-merge-workflow.test.ts index 13a59f2f..d7ca5c0a 100644 --- a/test/canonical-merge-workflow.test.ts +++ b/test/canonical-merge-workflow.test.ts @@ -37,12 +37,30 @@ const lineOf = (needle: string): number => { }; describe('T-1502 canonical-merge.yml safety', () => { - it('mints the App token only after the contributor code has already run', () => { - // The rebuild runs `npm ci` and `build:canonical` on the merged tree, which - // executes whatever the pull request put in `package.json`. Minting first - // would put a credential that can push to this repository into that - // environment. - expect(lineOf('build:canonical')).toBeLessThan(lineOf('app-installation-token.mjs')); + it('mints the App token in a job that never ran contributor code', () => { + // Step order inside one job is not a boundary. `$GITHUB_ENV` and + // `$GITHUB_PATH` written during `npm ci` persist into every later step of + // the same job, so a dependency lifecycle script can set `NODE_OPTIONS` 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. Extracting the + // credential script from `main` fixed *what* ran, not *how it was launched*. + const body = code(); + const rebuildJob = body.slice(body.indexOf(' canonicalise:'), body.indexOf(' publish:')); + const publishJob = body.slice(body.indexOf(' publish:')); + + expect(rebuildJob, 'the rebuild job can reach the App key').not.toContain('COMMITLORE_BOT_KEY'); + expect(rebuildJob).not.toContain('app-installation-token.mjs'); + expect(publishJob, 'the publishing job runs npm').not.toMatch(/\bnpm (ci|run)\b/); + expect(publishJob).toContain('app-installation-token.mjs'); + }); + + it('hands the tree over as bytes rather than by rerunning the build', () => { + // A bundle moves the commits the first job produced. Rebuilding in the + // second job would put contributor code back on the runner that holds the + // credential, which is the boundary this exists to keep. + const body = code(); + expect(body).toContain('git bundle create'); + expect(body).toMatch(/needs:\s*canonicalise/); }); it('does not put the App token in the rebuild step', () => { @@ -93,15 +111,18 @@ describe('T-1502 canonical-merge.yml safety', () => { // Running the merged copy would hand the App private key to whatever the // pull request made that file into, without needing a `postinstall` at all. const body = code(); - expect(body).toMatch(/git show "\$\{\{ steps\.merge\.outputs\.base \}\}:scripts\/app-installation-token\.mjs"/); + expect(body).toMatch(/git show "\$\{\{ steps\.take\.outputs\.base \}\}:scripts\/app-installation-token\.mjs"/); // And not from the workspace. expect(body).not.toMatch(/node scripts\/app-installation-token\.mjs/); }); - it('reads main\'s sha before the rebuild, not after', () => { + it('reads main\'s sha before the rebuild, and carries it across the job boundary', () => { // The rebuild executes contributor code in this workspace, so a value read // from `.git` afterwards is a value that code had the chance to choose. expect(lineOf('base=$(git rev-parse origin/main)')).toBeLessThan(lineOf('build:canonical')); + // And the second job takes it from the artifact rather than re-deriving it + // in a workspace the first job could have edited. + expect(code()).toContain('cat /tmp/canonical/canonical.base'); }); it('does not persist the Actions credential over the App token', () => { From 36a301e21068f22570d9e6328edad62def6de66b Mon Sep 17 00:00:00 2001 From: MongLong0214 Date: Tue, 18 Aug 2026 12:32:38 +0900 Subject: [PATCH 6/7] Make the merge method part of the contract, and correct why this is manual 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 --- .github/workflows/canonical-merge.yml | 15 ++++++++++----- test/canonical-merge-workflow.test.ts | 13 +++++++++++++ 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/.github/workflows/canonical-merge.yml b/.github/workflows/canonical-merge.yml index 5a25f934..92a2e47f 100644 --- a/.github/workflows/canonical-merge.yml +++ b/.github/workflows/canonical-merge.yml @@ -16,10 +16,15 @@ name: Canonical merge # bypass, and a bypass is not the check passing. Measured 2026-08-18: an # App-opened pull request attaches all eleven (run 32082467906). # -# Run by hand. An automatic trigger on pull-request events would make a fork's -# first push decide when this repository's App token is in an environment -# building that fork's code, and #723 is the record of why a workflow must not -# take direction from a pull request head. +# Run by hand, and the reason changed. It was security: 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. The job split below removed that -- +# the token is never in the job that runs contributor code -- so what is left +# is cost. Every dispatch is a Docker `npm ci` plus a canonical build, and a +# trigger on pull-request events would run one per push from anybody. +# +# `without a maintainer` in PRD-F15's success line means without a maintainer +# *rebuilding*, which is what #720 waited on twice. Nobody rebuilds here. on: workflow_dispatch: inputs: @@ -254,5 +259,5 @@ jobs: else gh pr create --base main --head "$branch" \ --title "Canonical merge of #${PR}" \ - --body "$(printf 'The commit that will land for #%s: `main` plus that source plus a canonical rebuild, built together so all eleven required contexts run on the tree that merges rather than on one that resembles it.\n\n#%s carries source only, which is what a contributor on a host that cannot run a `linux/amd64` Docker build can produce (#720). Nothing was rebuilt by hand.\n\nOpened by `canonical-merge.yml` for #719. Close #%s when this merges.' "$PR" "$PR" "$PR")" + --body "$(printf 'The commit that will land for #%s: `main` plus that source plus a canonical rebuild, built together so all eleven required contexts run on the tree that merges rather than on one that resembles it.\n\n#%s carries source only, which is what a contributor on a host that cannot run a `linux/amd64` Docker build can produce (#720). Nothing was rebuilt by hand.\n\n**Merge this with a merge commit, not a squash.** This branch merged #%s with `--no-ff`, so its head commit is an ancestor here: a merge commit lands that commit on `main` and GitHub closes #%s as merged, which is what T-1502 asks for. A squash lands new bytes instead, and #%s stays open with nothing to point at.\n\nOpened by `canonical-merge.yml` for #719.' "$PR" "$PR" "$PR" "$PR" "$PR")" fi diff --git a/test/canonical-merge-workflow.test.ts b/test/canonical-merge-workflow.test.ts index d7ca5c0a..a1302aff 100644 --- a/test/canonical-merge-workflow.test.ts +++ b/test/canonical-merge-workflow.test.ts @@ -145,6 +145,19 @@ describe('T-1502 canonical-merge.yml safety', () => { expect(code()).toMatch(/git push --quiet --force/); }); + it('merges the pull request head in, so a merge commit closes it', () => { + // T-1502 asks that "a source-only pull request merges". This job does not + // merge it -- it opens a second pull request -- so the wording is satisfied + // by what lands rather than by what is clicked: the branch merges the + // contributor's head with `--no-ff`, so that commit is an ancestor here. + // A merge commit lands it on `main` and GitHub closes their pull request as + // merged. A squash lands new bytes and leaves it open with nothing to point + // at, which is why the instruction is in the body the bot writes. + const body = code(); + expect(body).toMatch(/git merge --no-ff/); + expect(body).toMatch(/merge commit, not a squash/); + }); + it('requests no permission beyond reading', () => { expect(code()).toMatch(/permissions:\s*\n\s*contents:\s*read/); }); From 1cdb0aef6c02d4f9c49c7691ef496b9ad33666a1 Mon Sep 17 00:00:00 2001 From: MongLong0214 Date: Tue, 18 Aug 2026 15:03:41 +0900 Subject: [PATCH 7/7] The body that says it closes as merged is what stops it 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 --- .github/workflows/canonical-merge.yml | 2 +- .../F15-canonical-artifact-provenance.md | 40 +++++++++++++++++-- test/canonical-merge-workflow.test.ts | 24 +++++++++++ 3 files changed, 61 insertions(+), 5 deletions(-) diff --git a/.github/workflows/canonical-merge.yml b/.github/workflows/canonical-merge.yml index 92a2e47f..fae66734 100644 --- a/.github/workflows/canonical-merge.yml +++ b/.github/workflows/canonical-merge.yml @@ -259,5 +259,5 @@ jobs: else gh pr create --base main --head "$branch" \ --title "Canonical merge of #${PR}" \ - --body "$(printf 'The commit that will land for #%s: `main` plus that source plus a canonical rebuild, built together so all eleven required contexts run on the tree that merges rather than on one that resembles it.\n\n#%s carries source only, which is what a contributor on a host that cannot run a `linux/amd64` Docker build can produce (#720). Nothing was rebuilt by hand.\n\n**Merge this with a merge commit, not a squash.** This branch merged #%s with `--no-ff`, so its head commit is an ancestor here: a merge commit lands that commit on `main` and GitHub closes #%s as merged, which is what T-1502 asks for. A squash lands new bytes instead, and #%s stays open with nothing to point at.\n\nOpened by `canonical-merge.yml` for #719.' "$PR" "$PR" "$PR" "$PR" "$PR")" + --body "$(printf 'The commit that will land for #%s: `main` plus that source plus a canonical rebuild, built together so all eleven required contexts run on the tree that merges rather than on one that resembles it.\n\n#%s carries source only, which is what a contributor on a host that cannot run a `linux/amd64` Docker build can produce (#720). Nothing was rebuilt by hand.\n\n**Merge this with a merge commit, not a squash.** This branch merged #%s with `--no-ff`, so its head commit is an ancestor here: a merge commit lands that commit on `main`, and GitHub then records #%s as merged because its head is reachable -- which is what T-1502 asks for. A squash lands new bytes instead, and #%s stays open with nothing to point at.\n\nThis body deliberately carries no closing keyword. GitHub binds one only to the number straight after it, and a pull request closed by keyword is recorded closed rather than merged -- the opposite of the line above. Reachability does the closing here.\n\nOpened by `canonical-merge.yml` for #719.' "$PR" "$PR" "$PR" "$PR" "$PR")" fi diff --git a/docs/tickets/F15-canonical-artifact-provenance.md b/docs/tickets/F15-canonical-artifact-provenance.md index 3faf8b9c..d80f91c0 100644 --- a/docs/tickets/F15-canonical-artifact-provenance.md +++ b/docs/tickets/F15-canonical-artifact-provenance.md @@ -92,8 +92,16 @@ comment cannot satisfy them. At minimum: - the App token is minted from `COMMITLORE_BOT_APP_ID` / `COMMITLORE_BOT_KEY` and never echoed; -- the job never checks out or executes a pull request's head — the same rule #723 fixed for - `preserve`, and the same reason; +- the job that holds the App credential never checks out or executes a pull request's + head. **Amended 2026-08-18** — it read "the job never …", borrowed wholesale from the + rule #723 fixed for `preserve`. `preserve` only reads a pull request; this one *rebuilds* + it, and rebuilding somebody's change means running it: their `package.json`, their + lockfile, every dependency lifecycle script it pulls in. Written the old way the + assertion is unsatisfiable by any implementation of this feature, so it would have been + quietly dropped rather than met. What is achievable, and what the split into + `canonicalise` → `publish` exists to hold, is that the runner executing that code holds + no credential — the token is minted in a second job, from `main`'s copy of the mint + script, outside the workspace; - the rebuild is `build:canonical`, not a local `npm run build`, or the pushed bytes are not the canonical ones. @@ -102,8 +110,32 @@ comment cannot satisfy them. At minimum: - 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**. -- Deliberately breaking the rebuild — e.g. skipping `artifact:manifest` — produces a red - check, not a green merge. **A fixture that cannot fail is not evidence** (#722). +- Deliberately breaking the rebuild produces a red check, not a green merge. **A fixture + that cannot fail is not evidence** (#722) — and the example this ticket first gave was + one. **Amended 2026-08-18**: "skipping `artifact:manifest`" cannot be produced from a + pull request. That step is hard-coded in the workflow, which is loaded from the default + branch, and the source-only filter refuses any change under `.github/workflows/`, + `dist/` or `installer/canonical-artifact.json`. A negative control nobody can perform is + the same defect it was written to prevent. + + The producible one: after the workflow pushes `canonical/pr-N`, add a commit to that + branch that edits `dist/` without rebuilding. `ci.yml`'s `git diff --exit-code -- dist/ + installer/canonical-artifact.json` must go red on the canonical pull request. That + falsifies the property this ticket actually claims — *the bytes that land match the + source that landed with them* — rather than the workflow's internal step list. + +**Known limitation — the merge method is not enforceable from here.** The canonical pull +request's body asks for a merge commit, because a squash lands new bytes and leaves the +source pull request open with nothing to point at. That request is a check somebody has to +read. The repository allows squash, merge and rebase, and GitHub's merge button remembers +whichever was used last, so the wrong one is a click away — measured on #760, where five +of six pull requests closed as merged and the sixth did not. + +Two ways to close it, and both are the owner's call rather than this ticket's: +`gh api -X PUT .../pulls/N/merge -f merge_method=merge` names the method per merge, and +turning off `allow_squash_merge` / `allow_rebase_merge` names it once for the repository. +The second is the one that fits a repository that commits `dist/`, since a squash breaks +the ancestry every integration pull request here depends on. **Not in scope** — removing `dist/` from pull request requirements. That is T-1503, and doing it here means a failure in this ticket has no fallback. diff --git a/test/canonical-merge-workflow.test.ts b/test/canonical-merge-workflow.test.ts index a1302aff..dfcc19ba 100644 --- a/test/canonical-merge-workflow.test.ts +++ b/test/canonical-merge-workflow.test.ts @@ -37,6 +37,30 @@ const lineOf = (needle: string): number => { }; describe('T-1502 canonical-merge.yml safety', () => { + it('opens the canonical pull request with no closing keyword in its body', () => { + // T-1502's acceptance is that the source pull request ends up *merged*. + // A closing keyword produces the opposite: GitHub records a pull request + // closed by keyword as closed, with `mergedAt` null, and there is no API + // to convert that afterwards. Reachability is what closes it as merged, + // and reachability needs no keyword. + // + // Measured on #752 during the 1.1.3 release: one integration pull request + // body said "GitHub closes #752, #755, #756 ... as merged". The keyword + // bound to #752 alone, so that one was recorded closed while the five with + // no keyword were recorded merged -- the sentence describing the outcome + // is what denied it. This workflow would have reproduced that every run. + const body = code(); + const printf = /--body "\$\(printf '([\s\S]*?)'\s/.exec(body); + expect(printf, 'the gh pr create body is not a printf literal any more').not.toBeNull(); + + const rendered = (printf as RegExpExecArray)[1].replace(/\\n/g, '\n').replace(/%s/g, '123'); + const keyword = /\b(close[sd]?|fix(e[sd])?|resolve[sd]?)\s+#\d+/i.exec(rendered); + expect( + keyword, + `the generated body carries a closing keyword (${keyword?.[0]}); it would close the source pull request as closed rather than merged`, + ).toBeNull(); + }); + it('mints the App token in a job that never ran contributor code', () => { // Step order inside one job is not a boundary. `$GITHUB_ENV` and // `$GITHUB_PATH` written during `npm ci` persist into every later step of