Merge pull request #546 from omacom/fix/pending-build-approval

Keep unapproved PR builds pending instead of failing

.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" ]]

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.

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/);
+  });
+}