From 59c77a7a843bd24c9789b8fea5cca50e4e086b12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Mon, 31 Aug 2026 20:49:17 +0000 Subject: [PATCH] ci: revoke stale merge approval after untrusted push (#3224) --- ...e-ready-to-merge-workflow-contract_test.py | 188 +++++++++++++ .github/workflows/lint-action-pinning.yml | 3 + .github/workflows/revoke-ready-to-merge.yml | 263 ++++++++++++++++++ ...oke-merge-approval-after-untrusted-push.md | 62 +++++ docs/decisions/INDEX.md | 1 + .../plan.md | 110 ++++++++ .../task-01-add-revocation-workflow.md | 107 +++++++ docs/specs/ci/README.md | 2 + .../contributor-merge-approval-revocation.md | 71 +++++ .../contributor-merge-approval-revocation.md | 182 ++++++++++++ 10 files changed, 989 insertions(+) create mode 100644 .github/scripts/revoke-ready-to-merge-workflow-contract_test.py create mode 100644 .github/workflows/revoke-ready-to-merge.yml create mode 100644 docs/decisions/2026-08-31-revoke-merge-approval-after-untrusted-push.md create mode 100644 docs/plans/contributor-merge-approval-revocation/plan.md create mode 100644 docs/plans/contributor-merge-approval-revocation/task-01-add-revocation-workflow.md create mode 100644 docs/specs/ci/requirements/contributor-merge-approval-revocation.md create mode 100644 docs/specs/ci/system-design/contributor-merge-approval-revocation.md diff --git a/.github/scripts/revoke-ready-to-merge-workflow-contract_test.py b/.github/scripts/revoke-ready-to-merge-workflow-contract_test.py new file mode 100644 index 0000000000..334fb22875 --- /dev/null +++ b/.github/scripts/revoke-ready-to-merge-workflow-contract_test.py @@ -0,0 +1,188 @@ +#!/usr/bin/env python3 +"""Contract tests for the stale merge-approval revocation workflow.""" + +from pathlib import Path +import unittest + + +REPO_ROOT = Path(__file__).resolve().parents[2] +WORKFLOW = REPO_ROOT / ".github" / "workflows" / "revoke-ready-to-merge.yml" +LINT_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "lint-action-pinning.yml" + + +class RevokeReadyToMergeWorkflowContractTest(unittest.TestCase): + def setUp(self) -> None: + self.assertTrue(WORKFLOW.is_file(), "Revocation workflow is missing") + self.workflow = WORKFLOW.read_text(encoding="utf-8") + + # @covers AC-CI-MERGE-APPROVAL-001.1 + def test_workflow_only_handles_pull_request_synchronize_events(self) -> None: + self.assertIn( + "on:\n pull_request_target:\n types: [synchronize]\n", + self.workflow, + ) + trigger = self.workflow.partition("on:\n")[2].partition("\nconcurrency:")[0] + self.assertEqual( + trigger, + " pull_request_target:\n types: [synchronize]\n", + ) + + # @covers AC-CI-MERGE-APPROVAL-001.1 + def test_removes_only_the_exact_merge_approval_label(self) -> None: + self.assertIn("const approvalLabel = 'ready-to-merge';", self.workflow) + self.assertIn("eventPullRequest?.labels", self.workflow) + self.assertIn( + "label => label?.name === approvalLabel", + self.workflow, + ) + self.assertIn("github.rest.issues.removeLabel", self.workflow) + self.assertIn("name: approvalLabel", self.workflow) + self.assertNotIn("safe-to-review", self.workflow) + + # @covers AC-CI-MERGE-APPROVAL-001.2 + def test_reads_and_independently_cleans_active_merge_states(self) -> None: + self.assertIn("autoMergeRequest { id }", self.workflow) + self.assertIn("mergeQueueEntry { id }", self.workflow) + self.assertIn("disablePullRequestAutoMerge", self.workflow) + self.assertIn("dequeuePullRequest", self.workflow) + self.assertIn("pullRequestId: $id", self.workflow) + self.assertIn("input: { id: $id }", self.workflow) + self.assertIn("currentPullRequest.autoMergeRequest?.id", self.workflow) + self.assertIn("currentPullRequest.mergeQueueEntry?.id", self.workflow) + self.assertIn("attemptCleanup(\n 'auto-merge',", self.workflow) + self.assertIn("attemptCleanup(\n 'merge queue',", self.workflow) + + attempt_cleanup_start = self.workflow.index("async function attemptCleanup") + attempt_cleanup_end = self.workflow.index("async function finish") + attempt_cleanup = self.workflow[attempt_cleanup_start:attempt_cleanup_end] + self.assertIn("catch (error)", attempt_cleanup) + self.assertIn("recordFailure(`${operation} cleanup`, error)", attempt_cleanup) + self.assertNotIn("throw ", attempt_cleanup) + + auto_cleanup = self.workflow.index( + "await attemptCleanup(\n 'auto-merge'," + ) + queue_cleanup = self.workflow.index( + "await attemptCleanup(\n 'merge queue'," + ) + self.assertLess(auto_cleanup, queue_cleanup) + + auto_merge_mutation = self.workflow.index("disablePullRequestAutoMerge") + auto_merge_block = self.workflow[auto_merge_mutation - 200 : auto_merge_mutation + 300] + self.assertIn("{ id: currentPullRequest.id }", auto_merge_block) + + queue_mutation = self.workflow.index("dequeuePullRequest") + queue_block = self.workflow[queue_mutation - 200 : queue_mutation + 300] + self.assertIn("{ id: currentPullRequest.id }", queue_block) + + # @covers AC-CI-MERGE-APPROVAL-001.3 + def test_event_label_snapshot_gates_all_cleanup(self) -> None: + gate = self.workflow.index("if (!hasApprovalLabel)") + write_gate = self.workflow.index( + "if (permission === 'write' || permission === 'admin')" + ) + + for cleanup in ( + "github.rest.issues.removeLabel", + "disablePullRequestAutoMerge", + "dequeuePullRequest", + ): + cleanup_index = self.workflow.index(cleanup) + self.assertLess(write_gate, cleanup_index) + self.assertLess(gate, cleanup_index) + + remove_label = self.workflow.index("github.rest.issues.removeLabel") + self.assertIn("return;", self.workflow[gate:remove_label]) + self.assertIn("hasApprovalLabel", self.workflow) + + # @covers AC-CI-MERGE-APPROVAL-001.4 + def test_write_and_admin_pushers_return_before_cleanup(self) -> None: + self.assertIn("context.payload.sender?.login", self.workflow) + self.assertIn( + "github.rest.repos.getCollaboratorPermissionLevel", + self.workflow, + ) + self.assertIn( + "permission === 'write' || permission === 'admin'", + self.workflow, + ) + trusted_gate = self.workflow.index( + "permission === 'write' || permission === 'admin'" + ) + permission_error = self.workflow.index("if (permissionError)") + remove_label = self.workflow.index("github.rest.issues.removeLabel") + permission_failure_branch = self.workflow[permission_error:trusted_gate] + + self.assertLess(permission_error, trusted_gate) + self.assertNotIn("return;", permission_failure_branch) + self.assertIn("return;", self.workflow[trusted_gate:remove_label]) + + # @covers AC-CI-MERGE-APPROVAL-001.5 + def test_permission_lookup_fails_closed_and_reports_failures(self) -> None: + self.assertIn("permissionError", self.workflow) + self.assertIn("permission = response.data.permission", self.workflow) + self.assertIn("permissionError = new Error", self.workflow) + self.assertIn("core.warning", self.workflow) + self.assertIn("core.setFailed", self.workflow) + self.assertIn("!senderLogin", self.workflow) + + # @covers AC-CI-MERGE-APPROVAL-001.6 + def test_cleanup_is_idempotent_but_unexpected_errors_are_failures(self) -> None: + self.assertIn("function isExpectedAlreadyCleanError", self.workflow) + self.assertIn("status === 404", self.workflow) + self.assertIn("Label does not exist", self.workflow) + self.assertIn("Pull request is not set to auto-merge", self.workflow) + self.assertIn("Pull request is not in the merge queue", self.workflow) + self.assertIn("if (isExpectedAlreadyCleanError", self.workflow) + self.assertIn("failures.push", self.workflow) + + expected_branch = self.workflow.index( + "if (isExpectedAlreadyCleanError(operation, error))" + ) + expected_outcome = self.workflow.index( + "recordResult(operation, 'already clean; concurrent cleanup won')", + expected_branch, + ) + expected_return = self.workflow.index("return;", expected_outcome) + unexpected_failure = self.workflow.index( + "recordFailure(`${operation} cleanup`, error)", + expected_branch, + ) + + self.assertLess(expected_branch, expected_outcome) + self.assertLess(expected_outcome, expected_return) + self.assertLess(expected_return, unexpected_failure) + + # @covers AC-CI-MERGE-APPROVAL-001.6 + def test_concurrency_serializes_each_pull_request_without_cancellation(self) -> None: + self.assertRegex( + self.workflow, + r"group: revoke-ready-to-merge-\$\{\{ github\.event\.pull_request\.number \}\}", + ) + self.assertIn("cancel-in-progress: false", self.workflow) + + # @covers AC-CI-MERGE-APPROVAL-001.7 + def test_workflow_has_trusted_least_privilege_and_pinned_execution(self) -> None: + permissions = self.workflow.partition("permissions:\n")[2].partition("\njobs:")[0] + self.assertEqual("pull-requests: write", permissions.strip()) + self.assertNotIn("actions/checkout", self.workflow) + self.assertNotIn("\n - run:", self.workflow) + self.assertNotIn("child_process", self.workflow) + self.assertNotIn("eval(", self.workflow) + self.assertRegex( + self.workflow, + r"uses: actions/github-script@[0-9a-f]{40} # v9\.[0-9]+\.[0-9]+", + ) + + def test_contract_is_registered_in_the_required_lint_workflow(self) -> None: + lint_workflow = LINT_WORKFLOW.read_text(encoding="utf-8") + + self.assertRegex( + lint_workflow, + r"(?m)^ - name: Test stale merge approval revocation workflow contract$\n" + r"^ run: python3 .github/scripts/revoke-ready-to-merge-workflow-contract_test.py$", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/lint-action-pinning.yml b/.github/workflows/lint-action-pinning.yml index 0c82206dbf..a374e5f71b 100644 --- a/.github/workflows/lint-action-pinning.yml +++ b/.github/workflows/lint-action-pinning.yml @@ -72,6 +72,9 @@ jobs: - name: Test E2E workflow contract run: python3 .github/scripts/e2e-tests-workflow-contract_test.py + - name: Test stale merge approval revocation workflow contract + run: python3 .github/scripts/revoke-ready-to-merge-workflow-contract_test.py + - name: Test managed runtime pin update workflow contract run: python3 .github/scripts/update-agent-runtime-pins-workflow-contract_test.py diff --git a/.github/workflows/revoke-ready-to-merge.yml b/.github/workflows/revoke-ready-to-merge.yml new file mode 100644 index 0000000000..51e9b4d37b --- /dev/null +++ b/.github/workflows/revoke-ready-to-merge.yml @@ -0,0 +1,263 @@ +name: Revoke stale merge approval + +on: + pull_request_target: + types: [synchronize] + +concurrency: + group: revoke-ready-to-merge-${{ github.event.pull_request.number }} + cancel-in-progress: false + +permissions: + pull-requests: write + +jobs: + revoke: + name: Revoke stale merge approval + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - name: Revoke stale merge approval + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const approvalLabel = 'ready-to-merge'; + const eventPullRequest = context.payload.pull_request; + const pullNumber = eventPullRequest?.number; + const senderLogin = context.payload.sender?.login; + const failures = []; + const results = []; + + function errorMessages(error) { + const messages = []; + if (typeof error?.message === 'string') { + messages.push(error.message); + } + if (Array.isArray(error?.errors)) { + for (const entry of error.errors) { + if (typeof entry?.message === 'string') { + messages.push(entry.message); + } + } + } + if (typeof error?.response?.data?.message === 'string') { + messages.push(error.response.data.message); + } + return [...new Set(messages)]; + } + + function errorText(error) { + return errorMessages(error).join('; ') || 'unknown error'; + } + + function recordResult(operation, outcome) { + results.push([operation, outcome]); + } + + function recordFailure(operation, error) { + const message = errorText(error); + core.error(`${operation}: ${message}`); + failures.push(`${operation}: ${message}`); + recordResult(operation, 'failed'); + } + + function isExpectedAlreadyCleanError(operation, error) { + const messages = errorMessages(error); + if ( + operation === 'label' && + (error?.status === 404 || error?.response?.status === 404) + ) { + return messages.includes('Label does not exist'); + } + + const expectedMessages = { + 'auto-merge': ['Pull request is not set to auto-merge'], + 'merge queue': ['Pull request is not in the merge queue'], + }; + return (expectedMessages[operation] ?? []).some(message => + messages.includes(message), + ); + } + + async function attemptCleanup(operation, id, mutation) { + if (!id) { + core.info(`${operation}: no active state`); + recordResult(operation, 'already clean; no mutation required'); + return; + } + + try { + await mutation(id); + core.info(`${operation}: cleanup completed`); + recordResult(operation, 'cleanup completed'); + } catch (error) { + if (isExpectedAlreadyCleanError(operation, error)) { + core.info(`${operation}: already clean`); + recordResult(operation, 'already clean; concurrent cleanup won'); + return; + } + recordFailure(`${operation} cleanup`, error); + } + } + + async function finish() { + core.summary.addHeading(`Merge approval cleanup for PR #${pullNumber}`); + core.summary.addTable([ + [ + { data: 'Operation', header: true }, + { data: 'Outcome', header: true }, + ], + ...results, + ]); + if (failures.length > 0) { + core.summary.addQuote(`Failures: ${failures.join('; ')}`); + } + await core.summary.write(); + if (failures.length > 0) { + core.setFailed(failures.join('; ')); + } + } + + if (!Number.isInteger(pullNumber)) { + recordFailure( + 'event validation', + new Error('synchronize event has no pull-request number'), + ); + await finish(); + return; + } + + let permission; + let permissionError; + if (!senderLogin) { + permissionError = new Error('synchronize event has no sender login'); + } else { + try { + const response = + await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: senderLogin, + }); + permission = response.data.permission; + if (!['admin', 'write', 'read', 'none'].includes(permission)) { + permissionError = new Error( + `permission response was unexpected: ${permission ?? 'missing'}`, + ); + } + } catch (error) { + permissionError = error; + } + } + + if (permissionError) { + const message = errorText(permissionError); + core.warning(`Could not resolve pusher permission: ${message}`); + failures.push(`permission lookup: ${message}`); + recordResult('pusher permission', 'lookup failed; treated as untrusted'); + } else { + core.info(`Pusher permission: ${permission}`); + recordResult('pusher permission', permission); + } + + if (permission === 'write' || permission === 'admin') { + core.info('Write-capable pusher; leaving merge approval state unchanged'); + recordResult('cleanup', 'skipped for write-capable pusher'); + await finish(); + return; + } + + const eventLabels = Array.isArray(eventPullRequest?.labels) + ? eventPullRequest.labels + : []; + const hasApprovalLabel = eventLabels.some( + label => label?.name === approvalLabel, + ); + if (!hasApprovalLabel) { + core.info('Event snapshot has no ready-to-merge label; skipping cleanup'); + recordResult('cleanup', 'skipped; approval label absent from event snapshot'); + await finish(); + return; + } + + try { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: pullNumber, + name: approvalLabel, + }); + core.info('Removed ready-to-merge label'); + recordResult('label', 'removed'); + } catch (error) { + if (isExpectedAlreadyCleanError('label', error)) { + core.info('ready-to-merge label was already removed'); + recordResult('label', 'already clean; concurrent removal won'); + } else { + recordFailure('label removal', error); + } + } + + let currentPullRequest; + try { + const state = await github.graphql( + ` + query($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + id + autoMergeRequest { id } + mergeQueueEntry { id } + } + } + } + `, + { + owner: context.repo.owner, + repo: context.repo.repo, + number: pullNumber, + }, + ); + currentPullRequest = state.repository?.pullRequest; + if (!currentPullRequest) { + throw new Error(`pull request #${pullNumber} was not found`); + } + } catch (error) { + recordFailure('merge state read', error); + } + + if (currentPullRequest) { + await attemptCleanup( + 'auto-merge', + currentPullRequest.autoMergeRequest?.id, + id => + github.graphql( + ` + mutation($id: ID!) { + disablePullRequestAutoMerge(input: { pullRequestId: $id }) { + pullRequest { id } + } + } + `, + { id: currentPullRequest.id }, + ), + ); + + await attemptCleanup( + 'merge queue', + currentPullRequest.mergeQueueEntry?.id, + id => + github.graphql( + ` + mutation($id: ID!) { + dequeuePullRequest(input: { id: $id }) { + mergeQueueEntry { id } + } + } + `, + { id: currentPullRequest.id }, + ), + ); + } + + await finish(); diff --git a/docs/decisions/2026-08-31-revoke-merge-approval-after-untrusted-push.md b/docs/decisions/2026-08-31-revoke-merge-approval-after-untrusted-push.md new file mode 100644 index 0000000000..f2c28d6092 --- /dev/null +++ b/docs/decisions/2026-08-31-revoke-merge-approval-after-untrusted-push.md @@ -0,0 +1,62 @@ +# ADR-2026-08-31-revoke-merge-approval-after-untrusted-push: Revoke merge approval after an untrusted pull-request push + +**Status:** accepted +**Date:** 2026-08-31 +**Area:** workflow, security + +## Context + +The `ready-to-merge` label is used by a coordinator as the maintainer's +approval to repair CI, enqueue, or automatically merge a pull request. If a +user without write access pushes a new commit after that approval, the label +and any active merge operation describe an older revision. Leaving them active +can allow a changed external contribution to proceed without a fresh +maintainer review. + +The workflow must also handle an external pull request that a maintainer +updates. That push is trusted for this purpose, even though the pull request +author may not have repository access. + +## Decision + +Add a base-controlled `pull_request_target` workflow for `synchronize` events. +Use `github.event.sender` as the pusher identity and resolve the sender's +current repository permission through GitHub's collaborator permission API. +Only `write` and `admin` are trusted. Treat missing, unknown, or failed +permission results as untrusted. + +For an untrusted event whose label snapshot contains the exact +`ready-to-merge` label, remove that label. Then read current GitHub pull-request +state and independently disable an active auto-merge request and dequeue an +active merge-queue entry. Do not check out or execute pull-request content. +Give the job only `pull-requests: write`, pin all external actions by commit +SHA, and serialize runs per pull request without canceling older runs. + +## Consequences + +- A contributor push requires a fresh `ready-to-merge` decision before the + coordinator can continue. +- A maintainer can update an external contributor's branch without resetting + the approval. +- Existing auto-merge and merge-queue state is actively withdrawn instead of + relying on the coordinator to notice the label change. +- A transient permission lookup failure can conservatively revoke approval from + a maintainer push. The workflow exposes that failure so the result can be + investigated. +- The workflow has a pull-request write token and must remain free of checkout, + arbitrary scripts, and contributor-controlled inputs that could execute. + +## Alternatives Considered + +- **Use the pull-request author's `author_association` or head repository:** + Rejected because the requested exemption follows the person who pushed the + new commit, and those fields do not provide the current effective repository + permission for that person. +- **Remove only the label:** Rejected because a coordinator may already have + enabled auto-merge or placed the pull request in the merge queue. +- **Use a normal `pull_request` workflow:** Rejected because fork events do not + provide the trusted write-capable token needed to mutate the base pull + request. +- **Maintain a static maintainer allowlist:** Rejected because repository, + team, organization, and enterprise grants can change independently of a + workflow file. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index 9bf23d08cb..ec18011763 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -223,5 +223,6 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-08-30-e2e-browser-cache | [Cache host-runner E2E browser provisioning](2026-08-30-e2e-browser-cache.md) | accepted | infra, workflow | 2026-08-30 | | 2026-08-30-environment-owned-git-status | [Project Current Git Status From the Task Environment](2026-08-30-environment-owned-git-status.md) | proposed | backend, frontend, protocol | 2026-08-30 | | 2026-08-31-local-worktree-refresh-best-effort | [Local Worktree Refresh Is Best Effort](2026-08-31-local-worktree-refresh-best-effort.md) | accepted | backend, security, operations | 2026-08-31 | +| 2026-08-31-revoke-merge-approval-after-untrusted-push | [Revoke merge approval after an untrusted pull-request push](2026-08-31-revoke-merge-approval-after-untrusted-push.md) | accepted | workflow, security | 2026-08-31 | | 2026-08-31-passthrough-running-publication | [Defer Passthrough Running Publication Until Guard Release](2026-08-31-passthrough-running-publication.md) | accepted | backend | 2026-08-31 | | 2026-08-31-agent-aware-mcp-tool-names | [Preserve Canonical MCP Tool Names Through Agent Namespacing](2026-08-31-agent-aware-mcp-tool-names.md) | accepted | backend, agentctl, protocol | 2026-08-31 | diff --git a/docs/plans/contributor-merge-approval-revocation/plan.md b/docs/plans/contributor-merge-approval-revocation/plan.md new file mode 100644 index 0000000000..5bf4496b57 --- /dev/null +++ b/docs/plans/contributor-merge-approval-revocation/plan.md @@ -0,0 +1,110 @@ +--- +created: 2026-08-31 +status: done +requirements: + - REQ-CI-MERGE-APPROVAL-001 +system_design: + - ../../specs/ci/system-design/contributor-merge-approval-revocation.md +legacy_specs: [] +--- + +# Implementation Plan: Contributor merge approval revocation + +## Overview + +Add one trusted GitHub Actions workflow that removes stale `ready-to-merge` +approval after a non-write pusher updates a pull request. The workflow will +also disable active auto-merge and dequeue the pull request, while exempting +pushes from current write-capable users. Contract tests and the existing action +pinning and workflow security checks will verify the implementation. + +## Scope + +### In scope + +- A `pull_request_target` synchronize workflow from the trusted base branch. +- Current permission lookup for the synchronize event sender. +- Exact `ready-to-merge` label removal for untrusted pushes. +- Active auto-merge disable and merge-queue dequeue operations. +- Idempotent cleanup and per-pull-request concurrency. +- Static workflow contract coverage and CI registration. + +### Out of scope + +- Changes to `safe-to-review` or other contributor review and preview labels. +- Changes to Kandev application code or merge-queue integration. +- Code review, CI re-evaluation, or automatic re-approval. +- UI, application code, or non-GitHub providers. + +## Technical approach + +Implement `.github/workflows/revoke-ready-to-merge.yml` with only the +`pull_request_target` `synchronize` activity. Run the pinned +`actions/github-script` action without checkout. Resolve +`github.event.sender.login` through the collaborator permission endpoint and +return for `write` or `admin`. For non-write, unknown, or failed permission +results, require the event label snapshot to contain `ready-to-merge`, remove +that exact label, read fresh pull-request GraphQL state, and independently +disable auto-merge and dequeue an active queue entry. + +Add `.github/scripts/revoke-ready-to-merge-workflow-contract_test.py` to assert +the trigger, pusher permission gate, exact label, GraphQL mutation names, +idempotent cleanup intent, no checkout or unsafe execution, least-privilege +permission block, pinned action, and per-PR non-canceling concurrency. Register +the test in `.github/workflows/lint-action-pinning.yml`. + +## Tests + +- `AC-CI-MERGE-APPROVAL-001.1` and `.2`: contract assertions for the exact + label removal and both merge-state mutations. +- `AC-CI-MERGE-APPROVAL-001.3` and `.4`: contract assertions for event-label + gating and write/admin pusher exemption. +- `AC-CI-MERGE-APPROVAL-001.5`: contract assertion for sender permission + lookup and fail-closed handling. +- `AC-CI-MERGE-APPROVAL-001.6`: contract assertions for idempotent, independent + cleanup and non-canceling per-PR concurrency. +- `AC-CI-MERGE-APPROVAL-001.7`: contract assertions for trusted workflow + provenance, no checkout, pinned action, and `pull-requests: write` only. + +## Operational validation + +After deployment, verify a disposable external-contributor PR in these states: + +1. With `ready-to-merge` and no active merge operation, a contributor push + removes only the label. +2. With `ready-to-merge` and auto-merge enabled, a contributor push removes + the label and disables auto-merge. +3. With `ready-to-merge` and an active queue entry, a contributor push removes + the label and dequeues the PR. +4. A maintainer push to the same external PR leaves all three states unchanged. +5. A push without the label does not alter queue or auto-merge state. + +## Work orders + +- [x] [Task 01: Add merge approval revocation workflow](task-01-add-revocation-workflow.md) + +## Verification results + +- RED: the new workflow contract test failed because the revocation workflow + did not exist yet. +- GREEN: `python3 + .github/scripts/revoke-ready-to-merge-workflow-contract_test.py` passed, 10 + tests. +- `python3 .github/scripts/lint-action-pinning_test.py` passed, 9 tests, and + `python3 .github/scripts/lint-action-pinning.py` passed for 21 workflows. +- `python3 scripts/lint-spec-files.test.py` passed, 20 tests, and + `python3 scripts/lint-spec-files.py --all` passed. +- `git diff --check` passed. +- `zizmor .github/workflows` completed with the repository's existing findings + plus the required workflow's generic `dangerous-triggers` finding for + `pull_request_target`; the targeted audit reported no other finding. + +## Risks + +- A permission API outage is fail-closed and can revoke approval for a + write-capable pusher; the workflow must make the lookup failure visible. +- GitHub may change GraphQL error wording for already-clean queue or + auto-merge state; tests and cleanup handling must classify only known + idempotent outcomes as successful no-ops. +- A delayed workflow must not remove a label added after an unlabelled push; + the event label snapshot is part of the cleanup gate. diff --git a/docs/plans/contributor-merge-approval-revocation/task-01-add-revocation-workflow.md b/docs/plans/contributor-merge-approval-revocation/task-01-add-revocation-workflow.md new file mode 100644 index 0000000000..c5c8c9dbff --- /dev/null +++ b/docs/plans/contributor-merge-approval-revocation/task-01-add-revocation-workflow.md @@ -0,0 +1,107 @@ +--- +id: "01-add-revocation-workflow" +title: "Add merge approval revocation workflow" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-CI-MERGE-APPROVAL-001 +acceptance_criteria: + - AC-CI-MERGE-APPROVAL-001.1 + - AC-CI-MERGE-APPROVAL-001.2 + - AC-CI-MERGE-APPROVAL-001.3 + - AC-CI-MERGE-APPROVAL-001.4 + - AC-CI-MERGE-APPROVAL-001.5 + - AC-CI-MERGE-APPROVAL-001.6 + - AC-CI-MERGE-APPROVAL-001.7 +system_design: + - ../../specs/ci/system-design/contributor-merge-approval-revocation.md +--- + +# Task 01: Add merge approval revocation workflow + +## Summary + +Create the trusted base-controlled workflow that revokes `ready-to-merge` and +withdraws active merge automation after an untrusted contributor push. Add +static contract coverage and register it with the existing workflow lint job. + +## In scope + +- Add `.github/workflows/revoke-ready-to-merge.yml`. +- Add the workflow contract test. +- Register the contract test with action-pinning CI. +- Preserve the pusher-based permission exemption and event-label race guard. + +## Out of scope + +- Changes to Kandev application code or merge-queue integration. +- Changes to existing contributor review and preview workflows. +- Automatic re-approval or validation of the new pull-request revision. + +## Acceptance + +- A non-write, event-labeled synchronize path removes the exact label and + independently attempts auto-merge disable and queue dequeue; write/admin + pushers and unlabelled event snapshots are no-ops. +- The workflow uses only trusted base content, has `pull-requests: write` as + its mutation permission, and pins the external action. +- The contract test runs in the existing lint workflow and passes with the + action-pinning and workflow security checks. + +## Verification + +```bash +python3 .github/scripts/revoke-ready-to-merge-workflow-contract_test.py +python3 .github/scripts/lint-action-pinning_test.py +python3 .github/scripts/lint-action-pinning.py +zizmor .github/workflows +git diff --check +``` + +## Files likely touched + +- `.github/workflows/revoke-ready-to-merge.yml` +- `.github/scripts/revoke-ready-to-merge-workflow-contract_test.py` +- `.github/workflows/lint-action-pinning.yml` + +## Dependencies + +None. + +## Risks + +- The implementation must preserve cleanup if one GitHub mutation is already + satisfied or another mutation fails. +- The permission lookup must use the event sender, not the pull-request author. + +## Parallelism + +`sequential` + +## Inputs + +- `docs/specs/ci/requirements/contributor-merge-approval-revocation.md` +- `docs/specs/ci/system-design/contributor-merge-approval-revocation.md` +- `docs/decisions/2026-08-31-revoke-merge-approval-after-untrusted-push.md` +- `.github/AGENTS.md` and existing workflow contract tests. + +## Results + +- Added the base-controlled `pull_request_target` synchronize workflow with + current sender permission lookup, exact event-label gating, and write/admin + exemption. +- Added independent label, auto-merge, and merge-queue cleanup with explicit + idempotent outcomes and failure reporting. +- Added 10 workflow contract tests and registered them in the always-on action + pinning workflow. +- RED failed against the missing workflow, then GREEN passed all 10 contract + tests. +- `lint-action-pinning_test.py` passed, 9 tests, and `lint-action-pinning.py` + passed for 21 workflows. +- Specification tests and all-file specification lint passed. +- `git diff --check` passed. +- `zizmor .github/workflows` completed with existing repository findings and + the intentional `pull_request_target` dangerous-trigger finding for this + required workflow; no other finding was reported by its targeted audit. diff --git a/docs/specs/ci/README.md b/docs/specs/ci/README.md index 66bc900085..c38e4544a1 100644 --- a/docs/specs/ci/README.md +++ b/docs/specs/ci/README.md @@ -36,10 +36,12 @@ deployment, and pull request walkthrough generation. ### Requirements - [Unified contributor PR automation](requirements/unified-contributor-pr-automation.md) +- [Contributor merge approval revocation](requirements/contributor-merge-approval-revocation.md) ### System design - [Unified contributor PR automation](system-design/unified-contributor-pr-automation.md) +- [Contributor merge approval revocation](system-design/contributor-merge-approval-revocation.md) ## Migration diff --git a/docs/specs/ci/requirements/contributor-merge-approval-revocation.md b/docs/specs/ci/requirements/contributor-merge-approval-revocation.md new file mode 100644 index 0000000000..693974a287 --- /dev/null +++ b/docs/specs/ci/requirements/contributor-merge-approval-revocation.md @@ -0,0 +1,71 @@ +--- +status: draft +system: ci +created: 2026-08-31 +owners: + - kandev +--- + +# Contributor merge approval revocation requirements + +## Overview + +The `ready-to-merge` label is a maintainer approval for a specific pull-request +revision. A new commit from a user without write access makes that approval +stale. The CI automation system owns the base-controlled workflow that revokes +the stale approval and prevents an already-started merge operation from using +it. + +## Terminology + +- **Merge approval label:** The exact `ready-to-merge` pull-request label. +- **Pusher:** The user identified by the `synchronize` event sender. +- **Write-capable pusher:** A pusher whose current repository permission is + `write` or `admin`. GitHub's `maintain` role is included in the `write` + permission value. +- **Untrusted push:** A `synchronize` event whose pusher is not write-capable, + including an unknown pusher or an unavailable permission result. + +## Requirements + +### REQ-CI-MERGE-APPROVAL-001: Revoke stale merge approval after an untrusted push + +**Intent:** Ensure that a contributor cannot change a pull request after a +maintainer approves it for merge and leave the stale approval connected to an +active merge operation. + +#### Acceptance criteria + +- **AC-CI-MERGE-APPROVAL-001.1:** When an untrusted push synchronizes a pull + request that had the `ready-to-merge` label in the event snapshot, the + system shall remove that label from the pull request. +- **AC-CI-MERGE-APPROVAL-001.2:** After the label revocation for an untrusted + push, when GitHub reports an active auto-merge request, the system shall + disable that auto-merge request, and when GitHub reports an active merge + queue entry, the system shall remove the pull request from the queue. +- **AC-CI-MERGE-APPROVAL-001.3:** When the event snapshot does not contain the + `ready-to-merge` label, the system shall leave the pull request's labels, + auto-merge state, and merge-queue state unchanged. +- **AC-CI-MERGE-APPROVAL-001.4:** When a write-capable pusher synchronizes a + pull request, the system shall leave the pull request's labels, auto-merge + state, and merge-queue state unchanged. +- **AC-CI-MERGE-APPROVAL-001.5:** When the pusher is missing, unknown, or the + permission lookup fails, the system shall classify the push as untrusted and + attempt the same approval and merge-state cleanup, while reporting the + permission lookup failure in the workflow result. +- **AC-CI-MERGE-APPROVAL-001.6:** When cleanup is retried or an already-clean + state is observed, the system shall not remove unrelated labels or report a + successful merge-state change that was not needed. +- **AC-CI-MERGE-APPROVAL-001.7:** The cleanup shall run from trusted + base-controlled workflow content without checking out or executing + pull-request files, and its job shall have only the pull-request write + permission required for the cleanup. + +## Out of scope + +- Removing or changing the existing `safe-to-review` contributor automation + label contract. +- Re-evaluating code changes, reviews, checks, or branch protection rules. +- Automatically reapplying `ready-to-merge` after a maintainer review. +- Changing merge behavior for commits pushed by a write-capable user. +- Supporting providers other than GitHub pull requests. diff --git a/docs/specs/ci/system-design/contributor-merge-approval-revocation.md b/docs/specs/ci/system-design/contributor-merge-approval-revocation.md new file mode 100644 index 0000000000..b9b5b8bf3d --- /dev/null +++ b/docs/specs/ci/system-design/contributor-merge-approval-revocation.md @@ -0,0 +1,182 @@ +--- +status: draft +system: ci +requirements: + - REQ-CI-MERGE-APPROVAL-001 +--- + +# Contributor merge approval revocation system design + +## Purpose and boundaries + +This design defines the base-controlled GitHub Actions workflow that invalidates +the `ready-to-merge` approval after a non-write contributor push. The CI system +owns the workflow event, trust decision, permissions, and contract tests. The +GitHub integration owns the provider's merge queue and auto-merge APIs; this +workflow only invokes those existing GitHub operations to remove state made +unsafe by the push. + +The workflow does not inspect the pull-request diff, run repository code, or +decide whether the new revision is mergeable. A maintainer must apply the label +again after reviewing the new revision. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-CI-MERGE-APPROVAL-001` | [Event and trust contract](#event-and-trust-contract), [Control flow](#control-flow), [Security](#security), [Failure and recovery](#failure-and-recovery) | + +## Components and responsibilities + +| Component | Responsibility | +| --- | --- | +| `.github/workflows/revoke-ready-to-merge.yml` | Runs the cleanup on base-controlled `pull_request_target` synchronize events. | +| `actions/github-script` | Executes the pinned, trusted API orchestration without checking out repository content. | +| GitHub collaborator permission API | Resolves the current repository permission of the synchronize event sender. | +| GitHub label REST API | Removes the exact `ready-to-merge` label from the pull request. | +| GitHub pull-request GraphQL API | Reads active auto-merge and queue state, then disables or dequeues each active state. | +| `.github/scripts/revoke-ready-to-merge-workflow-contract_test.py` | Protects the event, pusher identity, permission, cleanup mutations, and permission boundary. | +| `.github/workflows/lint-action-pinning.yml` | Runs the workflow contract test and action-pinning checks. | + +## Event and trust contract + +The workflow subscribes only to: + +```yaml +on: + pull_request_target: + types: [synchronize] +``` + +`github.event.sender.login` is the pusher identity for the decision. The +workflow asks GitHub for that user's current repository permission through +`GET /repos/{owner}/{repo}/collaborators/{username}/permission`. + +The REST response's legacy `permission` value is the normalized decision: + +- `write` and `admin` are write-capable and exempt from cleanup. +- `read`, `none`, a missing value, a missing sender, a `404`, or another + lookup failure is untrusted and follows the cleanup path. + +The workflow first checks the event payload's label snapshot for the exact +`ready-to-merge` name. This prevents a delayed workflow from removing a label +that a maintainer added after an unlabelled push. A current label removal is +still idempotent when a concurrent maintainer action removed it first. + +## Data and contracts + +The workflow uses only bounded event and API values: + +- repository owner and name from `context.repo`; +- the pull-request number from `context.payload.pull_request.number`; +- the pusher login from `context.payload.sender.login`; +- the exact label name `ready-to-merge`; +- the pull request node ID and optional `autoMergeRequest` and + `mergeQueueEntry` fields from a fresh GraphQL read. + +The GraphQL read is equivalent to: + +```graphql +query($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + id + autoMergeRequest { id } + mergeQueueEntry { id } + } + } +} +``` + +For each non-null state, the workflow invokes the matching mutation: + +```graphql +mutation($id: ID!) { + disablePullRequestAutoMerge(input: { pullRequestId: $id }) { + pullRequest { id } + } +} +``` + +```graphql +mutation($id: ID!) { + dequeuePullRequest(input: { id: $id }) { + mergeQueueEntry { id } + } +} +``` + +The workflow does not change any other label, merge method, branch, review, +check, or queue entry. + +## Control flow + +1. GitHub starts the trusted base workflow for a `synchronize` event. +2. The job resolves the event sender's current repository permission. +3. A `write` or `admin` permission ends the job without any pull-request + mutation. +4. An untrusted path requires `ready-to-merge` in the event label snapshot. If + it is absent, the job ends without changing merge state. +5. The workflow removes only `ready-to-merge`. It records an absent-label + response as an idempotent outcome and continues to merge-state cleanup. +6. The workflow reads the current pull request through GraphQL after label + removal. It disables an active auto-merge request and dequeues an active + queue entry as independent operations. +7. The job reports the label, auto-merge, and queue outcomes. It attempts all + independent cleanup operations before failing the job for an unexpected API + error. + +The concurrency group is keyed by pull-request number and does not cancel an +older run. This ensures that a non-write push cannot be silently skipped when +several synchronize events arrive close together. + +## Failure and recovery + +- A missing or failed permission lookup fails closed into the cleanup path and + marks the workflow result as failed or warning-visible according to the + lookup error, so a maintainer can investigate a false revocation. +- A missing label is a successful no-op when the event snapshot did not contain + it. A concurrent deletion is also idempotent. +- A missing auto-merge request or queue entry is a successful no-op. +- Label, auto-merge, and queue operations are attempted independently. One + failure does not prevent the workflow from attempting the other cleanup + operations. +- An unexpected API failure makes the workflow fail after the attempted + cleanup. The label removal is the first safety action, so a failed later + mutation remains visible to maintainers for manual cleanup. +- Repeated synchronize events converge on the same clean state and never + reapply the merge approval. + +## Persistence + +The workflow has no repository or Kandev persistence. GitHub remains the source +of truth for labels, auto-merge requests, and queue entries. The event payload +and fresh API read provide the state needed for one run. + +## Security + +- `pull_request_target` runs the workflow definition from the trusted base + branch and is required so a fork event can receive the trusted write-capable + base `GITHUB_TOKEN`. +- The job does not use `actions/checkout`, execute a shell command from the + pull request, read pull-request files, or expose repository secrets. +- The job declares `pull-requests: write` and does not declare `issues: write`. + GitHub's label API permits pull-request write permission for labels on pull + requests, and the same permission is used for the pull-request mutations. +- The external action reference is pinned to a full commit SHA. Contract tests + reject a workflow that changes the event, permission identity, cleanup + mutations, or trusted-content boundary. +- Event and API values are passed as API parameters, not interpolated into + executable workflow source. + +## Observability + +The workflow log and step summary identify the pull request, pusher permission +classification, and each cleanup result without printing tokens. The contract +test provides local evidence for the event filter, permission gate, mutation +coverage, action pin, and least-privilege permission block. + +## Related decisions + +- [Revoke merge approval after an untrusted pull-request push](../../../decisions/2026-08-31-revoke-merge-approval-after-untrusted-push.md) +- [Use One Maintainer Approval Label for Contributor PR Automation](../../../decisions/2026-08-24-unified-fork-approval-label.md)