modelcontextprotocol/actions
Watch
1
0
Fork
You've already forked actions
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

slash-commands/status: force pending on synchronize to close auto-merge race

Browse source 
When new commits are pushed, the status job and handle job both fire on
the same synchronize event and run in parallel. The event payload's
pr.labels reflect the PRE-push state — so if the PR was approved before
the push, the status job reads 'approved' from the payload and sets
success on the NEW SHA, even though handle is about to (or already did)
remove the label.

Observed in modelcontextprotocol#2375: label removed at 23:06:29, status
set success at 23:06:30 (one second later, from stale payload), merged
at 23:06:34.

Fix: add invalidate-on-push input (default true, mirroring the main
action). On synchronize with invalidation enabled, set pending
unconditionally — don't trust the payload labels. The subsequent
unlabeled event re-evaluates and stays pending.
This commit is contained in:
Den Delimarsky 2026-03-11 23:09:07 +00:00
commit 60ed112c52
No known key found for this signature in database
2 changed files with 40 additions and 11 deletions
Download patch file Download diff file Expand all files Collapse all files

11
slash-commands/README.md
View file

@ -208,11 +208,17 @@ enforces. Make the `status-context` value a required status check.
**States:**
| Labels | Commit status | Description |
| Condition | Commit status | Description |
|---|---|---|
| `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 |
| neither | `pending` (🟡) | Awaiting /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:**
@ -222,6 +228,7 @@ enforces. Make the `status-context` value a required status check.
| `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. |
**Outputs:**

40
slash-commands/status/action.yml
View file

@ -27,6 +27,16 @@ inputs:
and what you mark as a required status check in branch protection.
required: false
default: slash-commands/approval
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.
required: false
default: 'true'
outputs:
state:
@ -43,6 +53,7 @@ runs:
APPROVED_LABEL: ${{ inputs.approved-label }}
HOLD_LABEL: ${{ inputs.hold-label }}
STATUS_CONTEXT: ${{ inputs.status-context }}
INVALIDATE_ON_PUSH: ${{ inputs.invalidate-on-push }}
with:
github-token: ${{ inputs.github-token }}
script: |
@ -53,21 +64,32 @@ runs:
return;
}
const sha = pr.head.sha;
const labels = pr.labels.map(l => l.name);
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).
let state, description;
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 {
if (invalidateOnPush && context.payload.action === 'synchronize') {
state = 'pending';
description = `Awaiting /lgtm from a maintainer or CODEOWNER`;
description = `New commits pushed — awaiting re-approval via /lgtm`;
} 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`;
}
}
await github.rest.repos.createCommitStatus({