slash-commands: write commit status directly from /lgtm handler
/lgtm was adding the approved label and posting the bot review, but the merge-gate commit status stayed pending forever. Root cause: the status sub-action was designed to react to the labeled/unlabeled events emitted when main.js mutates labels, but main.js writes via GITHUB_TOKEN and GitHub suppresses workflow runs for GITHUB_TOKEN-triggered events. The event cascade never happened. Fix: main.js now calls createCommitStatus directly after each of the four label mutations (/lgtm, /lgtm cancel, /hold, /unhold), computing the new state from the pre-mutation label snapshot so cross-label cases (lgtm while held, unhold while approved) resolve correctly. The status sub-action is reduced to setting pending on open and on push. It no longer reads the approved label at all — that branch is removed, and the approved-label input with it. This closes a bypass: previously anyone with triage permissions could manually add the approved label, and on the resulting labeled event the sub-action would set success. Now the only success writer is the auth-gated /lgtm path. Caller workflows should trim pull_request_target types to [opened, synchronize]. The labeled/unlabeled/reopened triggers are dead weight: labeled/unlabeled never fired for bot writes, and reopened would overwrite a valid success with pending (commit statuses persist on the SHA across close/reopen).
This commit is contained in:
Den Delimarsky
parent
f3ad04958f
commit
332da5ae1a
4 changed files with 85 additions and 53 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 }}
|
||||
|
|
|
|||
|
|
@ -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 = '<!-- slash-commands-lgtm -->';
|
||||
// `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);
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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({
|
||||
|
|
|
|||
Loading…
Reference in a new issue