diff --git a/.github/workflows/build-pr.yml b/.github/workflows/build-pr.yml index 764ef90..7adbfcc 100644 --- a/.github/workflows/build-pr.yml +++ b/.github/workflows/build-pr.yml @@ -7,9 +7,9 @@ name: Build changed packages # Tooling runs from the base branch; a PR supplies only pkgbuilds/. The # vouch gate limits who may spend compute; this limits what their PR can run. -# No paths filter: `result` is the required status check, so it has to be -# reported on every PR. A PR that touches no package directory gets an empty -# matrix and a passing result in seconds. +# No paths filter: approved PRs must report the required `result` even when +# no package directory changed. Those PRs get an empty matrix and a passing +# result in seconds; unapproved PRs wait for maintainer approval. on: pull_request: types: [opened, synchronize, reopened, labeled] @@ -28,13 +28,14 @@ jobs: # collaborators, anyone in .github/VOUCHED.td (read from the default # branch, so a PR cannot vouch for itself), or a PR a maintainer has # labelled "build-approved". Everyone else gets this job's plan output - # and a failing `result` until a maintainer approves the build. + # while the required `result` stays pending until a maintainer approves. changes: runs-on: ubuntu-latest outputs: matrix: ${{ steps.list.outputs.matrix }} count: ${{ steps.gate.outputs.count }} trusted: ${{ steps.gate.outputs.trusted }} + vouch_status: ${{ steps.vouch.outputs.status }} empty: ${{ steps.list.outputs.empty }} steps: # Same rule as the build job: bin/build-matrix comes from base, the @@ -178,20 +179,21 @@ jobs: if-no-files-found: error retention-days: 7 - # The one required status check. Matrix job names carry the package name, so - # they cannot be listed in branch protection; this job's name is stable and - # it fails if any package failed. It also runs (and passes) when no package - # changed, so tooling-only PRs are not stuck waiting for a status. + # `result` is required by branch protection. An unvouched author awaiting + # approval gets a differently named informational check, leaving `result` + # unreported (pending). Skipping or passing a job named `result` would count + # as satisfying the requirement even though no build was authorized. + # Actual planning/build failures and denouncements still report `result`. result: + name: ${{ needs.changes.result == 'success' && needs.changes.outputs.trusted == 'false' && needs.changes.outputs.vouch_status == 'unknown' && needs.changes.outputs.empty == 'false' && 'Awaiting build approval' || 'result' }} needs: [changes, build] if: always() runs-on: ubuntu-latest steps: - run: | echo "trusted=${{ needs.changes.outputs.trusted }} build=${{ needs.build.result }}" - # An untrusted author's PR fails until a maintainer vouches or labels it. - if [[ "${{ needs.changes.outputs.trusted }}" != "true" ]]; then - echo "::error::Builds were not run: author is not vouched. Add to .github/VOUCHED.td or apply the 'build-approved' label." + if [[ "${{ needs.changes.result }}" != "success" ]]; then + echo "::error::Build planning or the trust check failed. See the changes job." exit 1 fi # Nothing to merge: the PR's diff against its base is empty. Its @@ -200,4 +202,13 @@ jobs: echo "::error::This PR changes no files relative to its base. Its content is already on the target branch; close it instead of merging." exit 1 fi + if [[ "${{ needs.changes.outputs.trusted }}" == "false" && "${{ needs.changes.outputs.vouch_status }}" == "unknown" && "${{ needs.changes.outputs.empty }}" == "false" ]]; then + echo "::notice::Awaiting maintainer build approval. Apply 'build-approved' to this PR or vouch for the author in .github/VOUCHED.td." + echo "Package builds are waiting for maintainer approval. Apply **build-approved** to this PR to start them. The required **result** check remains pending." >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi + if [[ "${{ needs.changes.outputs.trusted }}" != "true" ]]; then + echo "::error::Builds are blocked: the author is denounced or the trust result is invalid. The build-approved label cannot override this." + exit 1 + fi [[ "${{ needs.build.result }}" == "success" || "${{ needs.build.result }}" == "skipped" ]] diff --git a/README.md b/README.md index 24168ba..c29aae8 100644 --- a/README.md +++ b/README.md @@ -845,8 +845,11 @@ The repository includes GitHub workflows and systemd services for automated rele 2. **sync-rebuilds.yml** (Every 6 hours): Bumps pkgrel for packages whose `rebuild_on` dependencies have moved in the official repositories and opens a PR. To approve builds for an unvouched contributor's PR, apply **`build-approved`**. -This triggers a package build and automatically releases GitHub's pending -build and test workflows for that PR's current commit. The approval workflow +Until approval, the PR shows **Awaiting build approval** and its required +`result` check stays pending, keeping the PR blocked from merging without +reporting a failed build. Actual build failures and denouncements still fail. +Applying the label triggers a package build and automatically releases GitHub's +pending build and test workflows for that PR's current commit. The approval workflow runs only trusted default-branch code; package builds and tests stay in the ordinary PR workflows. It may take a few minutes for GitHub to register and release all the runs. diff --git a/tests/pr-workflow-approval.cjs b/tests/pr-workflow-approval.cjs index d2b74f8..e4fc661 100644 --- a/tests/pr-workflow-approval.cjs +++ b/tests/pr-workflow-approval.cjs @@ -1,8 +1,9 @@ const assert = require('node:assert/strict'); -const { readFileSync } = require('node:fs'); +const { readFileSync, mkdtempSync, rmSync } = require('node:fs'); const { join } = require('node:path'); +const { tmpdir } = require('node:os'); const { test } = require('node:test'); -const { execFileSync } = require('node:child_process'); +const { execFileSync, spawnSync } = require('node:child_process'); const approve = require('../.github/scripts/approve-pr-workflows.cjs'); const BUILD = '.github/workflows/build-pr.yml'; @@ -240,3 +241,74 @@ test('the build gate permits a missing vouch only with approval, never a denounc }), expected); } }); + +// Exercise the actual reporting job, including its GitHub check name: a +// successful/skipped check called "result" would accidentally allow merging +// a PR whose build never ran. GitHub keeps a missing required check pending. +const resultJob = workflow.slice(workflow.indexOf('\n result:\n')); +const resultName = resultJob.match(/^ name: (.+)$/m)[1]; +const resultScript = resultJob.split(' - run: |\n')[1]; +function report({ trusted = 'false', vouch = 'unknown', empty = 'false', changes = 'success', build = 'skipped' } = {}) { + const needs = { + changes: { result: changes, outputs: { trusted, vouch_status: vouch, empty } }, + build: { result: build }, + }; + // The reporting expressions use &&, || and string equality, with the + // same semantics in JavaScript and Actions for these string-only fixtures. + const render = text => text.replace(/\$\{\{(.*?)\}\}/g, (_, expression) => + new Function('needs', `return (${expression})`)(needs)); + const directory = mkdtempSync(join(tmpdir(), 'build-approval-report-')); + const summaryPath = join(directory, 'summary'); + try { + const result = spawnSync('bash', ['-e', '-c', render(resultScript)], { + env: { ...process.env, GITHUB_STEP_SUMMARY: summaryPath }, encoding: 'utf8', + }); + return { name: render(resultName), ...result, + summary: result.stdout.includes('::notice::') ? readFileSync(summaryPath, 'utf8') : '' }; + } finally { + rmSync(directory, { recursive: true, force: true }); + } +} + +test('an unvouched PR waits without publishing a passing or failing required result', () => { + const result = report(); + assert.equal(result.name, 'Awaiting build approval'); + assert.equal(result.status, 0); + assert.match(result.stdout, /::notice::Awaiting maintainer build approval/); + assert.doesNotMatch(result.stdout, /::error::/); + assert.match(result.summary, /required \*\*result\*\* check remains pending/); +}); + +test('applying build-approved transitions the waiting PR to the required build result', () => { + assert.notEqual(report().name, 'result'); + const approved = report({ trusted: 'true', build: 'success' }); + assert.equal(approved.name, 'result'); + assert.equal(approved.status, 0); + const failed = report({ trusted: 'true', build: 'failure' }); + assert.equal(failed.name, 'result'); + assert.notEqual(failed.status, 0); +}); + +test('trusted tooling-only PRs still satisfy the required result without a package build', () => { + const result = report({ trusted: 'true', vouch: 'vouched' }); + assert.equal(result.name, 'result'); + assert.equal(result.status, 0); +}); + +for (const [name, overrides] of [ + ['denounced author', { vouch: 'denounced' }], + ['failed trust lookup', { vouch: '', changes: 'failure' }], + ['missing trust result', { vouch: '' }], + ['missing gate output', { trusted: '' }], + ['failed planning', { changes: 'failure' }], + ['cancelled planning', { changes: 'cancelled' }], + ['empty PR', { empty: 'true' }], + ['cancelled build', { trusted: 'true', build: 'cancelled' }], +]) { + test(`${name} fails the required result instead of masquerading as pending approval`, () => { + const result = report(overrides); + assert.equal(result.name, 'result'); + assert.notEqual(result.status, 0); + assert.doesNotMatch(result.stdout, /::notice::Awaiting maintainer build approval/); + }); +}