ci: revoke stale merge approval after untrusted push (#3224)
This commit is contained in:
Carlos Florêncio
@@ -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()
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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();
|
||||
@@ -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.
|
||||
@@ -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 |
|
||||
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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.
|
||||
@@ -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)
|
||||
Reference in New Issue
Block a user