diff --git a/slash-commands/README.md b/slash-commands/README.md index 7cda899..d75de24 100644 --- a/slash-commands/README.md +++ b/slash-commands/README.md @@ -29,10 +29,13 @@ must match `^/cmd\b`). Unauthorized attempts, or a PR author trying to `/lgtm` their own PR, receive a 👎 reaction with no further noise. Successful commands receive a 👍 reaction. -The caller workflow pairs this with the `slash-commands/status` sub-action, -which sets a commit status that stays **pending** until `approved` is present -and goes **failure** when `do-not-merge/hold` is present — making the labels -an actual merge gate via branch protection without a red ❌ on every fresh PR. +The action writes a commit status (`slash-commands/approval` by default) after +each label mutation — this is the merge gate. Mark it as a required status +check in branch protection. The commit status can **only** be set to `success` +by an authorized `/lgtm`; manually adding the `approved` label through the +GitHub UI has no effect on the gate. The caller workflow pairs this with the +`slash-commands/status` sub-action, which sets the status to `pending` on open +and on every push (fresh SHA = fresh approval needed). ## Prerequisites @@ -87,7 +90,7 @@ on the caller repo _and_ have org-level **Members: read**. | | | |---|---| -| **Triggers** | `issue_comment` (types: `created`) + `pull_request_target` (types: `synchronize`, `opened`, `reopened`, `labeled`, `unlabeled`) | +| **Triggers** | `issue_comment` (types: `created`) + `pull_request_target` (types: `opened`, `synchronize`) | | **Permissions** | `pull-requests: write`, `issues: write`, `contents: read`, `statuses: write`. If `/stageblog` is enabled, also `actions: write`. | | **Fork-PR safety** | No special guard needed — `issue_comment` and `pull_request_target` both run in the **base** repo's context with the base workflow definition, so fork authors cannot modify the logic. CODEOWNERS is also fetched from the PR's **base** ref, never the head. | | **No checkout** | The action calls GitHub API only. Do not `actions/checkout` PR code. | @@ -101,7 +104,7 @@ on: issue_comment: types: [created] pull_request_target: - types: [synchronize, opened, reopened, labeled, unlabeled] + types: [opened, synchronize] permissions: pull-requests: write @@ -189,6 +192,7 @@ jobs: | `codeowners-path` | | `.github/CODEOWNERS` | Path to CODEOWNERS, fetched from the PR's **base** ref (never head — tamper-proof) | | `invalidate-on-push` | | `'true'` | Remove `approved` label + dismiss bot review + comment when new commits are pushed. Set `'false'` to keep approval across pushes. | | `submit-review` | | `'true'` | Submit a bot APPROVE review alongside the label on `/lgtm`. Set `'false'` to use label only. | +| `status-context` | | `slash-commands/approval` | Commit-status context written after each label mutation. Must match the `status` sub-action's `status-context`. Mark this as a required status check in branch protection. | | `stageblog-workflow` | | _(empty)_ | Workflow file name (e.g. `stage-blog.yml`) to dispatch when `/stageblog` is invoked. Empty = command disabled. The workflow must accept `pr_number` and `head_sha` string inputs. | | `stageblog-paths` | | `blog/**` | Comma-separated CODEOWNERS-style glob patterns. `/stageblog` is refused if no changed file matches. | @@ -201,10 +205,16 @@ jobs: ## `slash-commands/status` sub-action -Sets a commit status on the PR head SHA derived from the approval/hold labels. -Use in a `status` job that runs on every `pull_request_target` event. The job -itself always succeeds — the commit status it creates is what branch protection -enforces. Make the `status-context` value a required status check. +Sets the **initial** `pending` commit status when a PR opens, and forces it +back to `pending` on every push (new SHA = new approval required). Run in a +`status` job on `pull_request_target` events. Mark the `status-context` value +as a required status check in branch protection. + +This sub-action **never sets `success`** — that would let anyone with `triage` +permissions bypass the auth gate by adding the `approved` label manually. +Only the main action's `/lgtm` handler (which runs the team/CODEOWNERS check) +writes `success`. The sub-action also cannot react to the main action's label +writes: `GITHUB_TOKEN`-triggered events do not create workflow runs. **States:** @@ -212,29 +222,22 @@ enforces. Make the `status-context` value a required status check. |---|---|---| | `synchronize` event + `invalidate-on-push: true` | `pending` (🟡) | New commits — re-approve | | `do-not-merge/hold` present | `failure` (❌) | Blocked by hold | -| `approved` present, no hold | `success` (✅) | Approved via /lgtm | | otherwise | `pending` (🟡) | Awaiting /lgtm | -The `synchronize` special-case is a race guard: the event payload's labels -reflect the *pre-push* state, so without it this action would read a stale -`approved` and set `success` on the new SHA while the `handle` job is still -removing the label in parallel — letting auto-merge slip through. - **Inputs:** | Input | Required | Default | Description | |---|---|---|---| | `github-token` | | `${{ github.token }}` | Token for the commit-status API. Workflow needs `statuses: write`. | -| `approved-label` | | `approved` | Must match the main action's `approved-label` | | `hold-label` | | `do-not-merge/hold` | Must match the main action's `hold-label` | -| `status-context` | | `slash-commands/approval` | Name shown in the merge box. Mark this as a required status check. | -| `invalidate-on-push` | | `'true'` | On `synchronize`, force pending regardless of labels. **Must match** the main action's `invalidate-on-push`. Disabling this opens an auto-merge race. | +| `status-context` | | `slash-commands/approval` | Name shown in the merge box. **Must match** the main action's `status-context`. Mark this as a required status check. | +| `invalidate-on-push` | | `'true'` | On `synchronize`, force pending regardless of labels. **Must match** the main action's `invalidate-on-push`. | **Outputs:** | Output | Description | |---|---| -| `state` | The commit status state set: `pending`, `success`, or `failure` | +| `state` | The commit status state set: `pending` or `failure` | ## CODEOWNERS pattern support @@ -263,6 +266,11 @@ Last-match-wins per file, consistent with GitHub's native CODEOWNERS semantics. from the default-branch workflow definition. The action does not check out PR code. - **Self-approval** — explicitly blocked before the auth check. +- **Manual label bypass** — the commit status (the actual merge gate) is only + ever set to `success` by the `/lgtm` handler after the auth check passes. + The `status` sub-action sets `pending`/`failure` but never `success`, and the + caller workflow does not trigger on `labeled` events. Adding the `approved` + label via the GitHub UI is cosmetic. - **`/stageblog` SHA pinning** — the dispatch passes `pr.head.sha` captured at the moment the maintainer comments. The companion workflow **must** check out `inputs.head_sha` (not `refs/pull/N/head` or `pr.head.ref`) so a fork diff --git a/slash-commands/action.yml b/slash-commands/action.yml index aab97c4..6ea2cce 100644 --- a/slash-commands/action.yml +++ b/slash-commands/action.yml @@ -54,6 +54,13 @@ inputs: Set to 'false' to rely on the label alone. required: false default: 'true' + status-context: + description: >- + Commit-status context name written after each label mutation. Must match + the status sub-action's status-context so both jobs write to the same + check. Mark this context as a required status check in branch protection. + required: false + default: slash-commands/approval stageblog-workflow: description: >- Workflow file name (e.g. stage-blog.yml) to dispatch when /stageblog is @@ -93,6 +100,7 @@ runs: CODEOWNERS_PATH: ${{ inputs.codeowners-path }} INVALIDATE_ON_PUSH: ${{ inputs.invalidate-on-push }} SUBMIT_REVIEW: ${{ inputs.submit-review }} + STATUS_CONTEXT: ${{ inputs.status-context }} STAGEBLOG_WORKFLOW: ${{ inputs.stageblog-workflow }} STAGEBLOG_PATHS: ${{ inputs.stageblog-paths }} ORG_TOKEN: ${{ inputs.github-token }} diff --git a/slash-commands/main.js b/slash-commands/main.js index a0d5e5c..4ba3237 100644 --- a/slash-commands/main.js +++ b/slash-commands/main.js @@ -11,6 +11,7 @@ module.exports = async ({ github, context, core }) => { const codeownersPath = process.env.CODEOWNERS_PATH; const invalidateOnPush = process.env.INVALIDATE_ON_PUSH === 'true'; const submitReview = process.env.SUBMIT_REVIEW === 'true'; + const statusContext = process.env.STATUS_CONTEXT; const reviewMarker = ''; // `github` is authed with bot-token (defaults to GITHUB_TOKEN) so comments, // reactions, labels, and reviews show as github-actions[bot]. The PAT is @@ -207,6 +208,28 @@ module.exports = async ({ github, context, core }) => { } } } + // GITHUB_TOKEN-triggered label events do NOT create workflow runs, so the + // status sub-action cannot react to our addLabel/removeLabel calls. Set the + // commit status directly here. This is also the ONLY path that may set + // state=success — the status sub-action deliberately never does, so that a + // manually-added approved label (triage+ perms suffice) cannot bypass auth. + async function setCommitStatus(sha, approved, held) { + let state, description; + if (held) { + state = 'failure'; + description = `Blocked by ${holdLabel} — comment /unhold to clear`; + } else if (approved) { + state = 'success'; + description = `Approved via /lgtm`; + } else { + state = 'pending'; + description = `Awaiting /lgtm from a maintainer or CODEOWNER`; + } + await github.rest.repos.createCommitStatus({ + owner, repo, sha, state, description, context: statusContext, + }); + core.info(`Set commit status ${state} on ${sha.slice(0,7)}: ${description}`); + } // --- Dispatch --------------------------------------------------- const ev = context.eventName; @@ -244,6 +267,10 @@ module.exports = async ({ github, context, core }) => { const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: prNumber }); if (pr.state !== 'open') return setResult('noop', actor); + const preLabels = new Set(pr.labels.map(l => l.name)); + const hadApproved = preLabels.has(approvedLabel); + const hadHold = preLabels.has(holdLabel); + // Self-approval guard (only for /lgtm, not /hold) if (cmd === 'lgtm' && !cancel && actor.toLowerCase() === pr.user.login.toLowerCase()) { await react(commentId, '-1'); @@ -305,21 +332,25 @@ module.exports = async ({ github, context, core }) => { await addLabel(prNumber, approvedLabel); if (submitReview) await submitApproval(prNumber, actor); await react(commentId, '+1'); + await setCommitStatus(pr.head.sha, true, hadHold); return setResult('lgtm-added', actor); } if (cmd === 'lgtm' && cancel) { await removeLabel(prNumber, approvedLabel); await dismissBotApprovals(prNumber, `@${actor} cancelled approval via /lgtm cancel.`); await react(commentId, '+1'); + await setCommitStatus(pr.head.sha, false, hadHold); return setResult('lgtm-removed', actor); } if ((cmd === 'hold' && !cancel)) { await addLabel(prNumber, holdLabel); await react(commentId, '+1'); + await setCommitStatus(pr.head.sha, hadApproved, true); return setResult('hold-added', actor); } // /hold cancel or /unhold await removeLabel(prNumber, holdLabel); await react(commentId, '+1'); + await setCommitStatus(pr.head.sha, hadApproved, false); return setResult('hold-removed', actor); }; diff --git a/slash-commands/status/action.yml b/slash-commands/status/action.yml index 8c460cb..bf31891 100644 --- a/slash-commands/status/action.yml +++ b/slash-commands/status/action.yml @@ -1,10 +1,10 @@ name: PR Slash Commands — Merge-Gate Status description: >- - Sets a commit status on the PR head SHA based on approval/hold labels. - Stays `pending` until the approved label is present, goes `failure` when the - hold label is present, and `success` when approved-and-not-held. Use this - instead of an exit-code-based status job so PRs show a yellow pending check - (not a red failure) while awaiting /lgtm. + Sets the initial pending commit status on the PR head SHA at open time and + forces it back to pending on push (new SHA, approval invalidated). Never + sets success — only the main slash-commands action's auth-gated /lgtm + handler may do that, so a manually-added approved label cannot bypass the + merge gate. Sets failure if the hold label is present. inputs: github-token: @@ -13,10 +13,6 @@ inputs: this token `statuses: write`. required: false default: ${{ github.token }} - approved-label: - description: Label that marks the PR as approved (set by /lgtm) - required: false - default: approved hold-label: description: Label that blocks merging (set by /hold) required: false @@ -30,17 +26,14 @@ inputs: invalidate-on-push: description: >- When 'true', a push (synchronize event) forces the status to pending - regardless of labels. This must match the main slash-commands action's - invalidate-on-push setting. Without this, on a push the event payload - still contains the pre-push label state, so this action would set - success on the new SHA (racing with the handle job removing the label) - and auto-merge can slip through. + regardless of labels. Must match the main slash-commands action's + invalidate-on-push setting. required: false default: 'true' outputs: state: - description: The commit status state that was set (pending, success, failure) + description: The commit status state that was set (pending or failure) value: ${{ steps.set.outputs.state }} runs: @@ -50,7 +43,6 @@ runs: id: set uses: actions/github-script@v8 env: - APPROVED_LABEL: ${{ inputs.approved-label }} HOLD_LABEL: ${{ inputs.hold-label }} STATUS_CONTEXT: ${{ inputs.status-context }} INVALIDATE_ON_PUSH: ${{ inputs.invalidate-on-push }} @@ -64,32 +56,25 @@ runs: return; } const sha = pr.head.sha; - const approvedLabel = process.env.APPROVED_LABEL; const holdLabel = process.env.HOLD_LABEL; const statusContext = process.env.STATUS_CONTEXT; const invalidateOnPush = process.env.INVALIDATE_ON_PUSH === 'true'; - // On push, the event payload's labels reflect the PRE-push state. - // If invalidation is on, the handle job is about to (or already did) - // remove the approved label in parallel — but we can't see that here. - // Force pending so auto-merge can't slip through on a stale success. - // The subsequent unlabeled event will re-evaluate (and stay pending). + // Never set success here. The only success writer is the main + // action's /lgtm handler, which is auth-gated. Reading the approved + // label would let anyone with triage+ perms bypass auth by labeling + // manually. This sub-action's job is: pending on open, pending on + // push (new SHA needs fresh approval), failure on hold. let state, description; if (invalidateOnPush && context.payload.action === 'synchronize') { state = 'pending'; description = `New commits pushed — awaiting re-approval via /lgtm`; + } else if (pr.labels.some(l => l.name === holdLabel)) { + state = 'failure'; + description = `Blocked by ${holdLabel} — comment /unhold to clear`; } else { - const labels = pr.labels.map(l => l.name); - if (labels.includes(holdLabel)) { - state = 'failure'; - description = `Blocked by ${holdLabel} — comment /unhold to clear`; - } else if (labels.includes(approvedLabel)) { - state = 'success'; - description = `Approved via /lgtm`; - } else { - state = 'pending'; - description = `Awaiting /lgtm from a maintainer or CODEOWNER`; - } + state = 'pending'; + description = `Awaiting /lgtm from a maintainer or CODEOWNER`; } await github.rest.repos.createCommitStatus({