Merge pull request #10 from modelcontextprotocol/den/status-sync-race
slash-commands/status: force pending on synchronize (close auto-merge race)
This commit is contained in:
commit
8e0308e4dd
2 changed files with 40 additions and 11 deletions
|
|
@ -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:**
|
||||
|
||||
|
|
|
|||
|
|
@ -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({
|
||||
|
|
|
|||
Loading…
Reference in a new issue