diff --git a/.github/workflows/canonical-merge.yml b/.github/workflows/canonical-merge.yml new file mode 100644 index 00000000..fae66734 --- /dev/null +++ b/.github/workflows/canonical-merge.yml @@ -0,0 +1,263 @@ +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, 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: + 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" + + # 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 + 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 + 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 }}" + 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="${{ 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 + 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: + COMMITLORE_BOT_APP_ID: ${{ secrets.COMMITLORE_BOT_APP_ID }} + COMMITLORE_BOT_KEY: ${{ secrets.COMMITLORE_BOT_KEY }} + run: | + set -euo pipefail + # 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.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" + + - name: Open the canonical pull request + env: + GH_TOKEN: ${{ steps.token.outputs.token }} + PR: ${{ inputs.pull_request_number }} + run: | + set -euo pipefail + 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. + 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\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/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-merge-workflow.test.ts b/test/canonical-merge-workflow.test.ts new file mode 100644 index 00000000..dfcc19ba --- /dev/null +++ b/test/canonical-merge-workflow.test.ts @@ -0,0 +1,196 @@ +/** + * 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('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 + // 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', () => { + 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('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\.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, 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', () => { + 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('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/); + }); + + 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); + }); +}); 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'); + }); +});