From ae3da6a2dd916af60f329d485469f7035bc1d84f Mon Sep 17 00:00:00 2001 From: Den Delimarsky Date: Fri, 24 Apr 2026 00:51:19 +0000 Subject: [PATCH] slash-commands: keep maintainer-applied accepted label across pushes When the approved label was most recently applied directly by a member of always-allow-teams (i.e. a core maintainer using the GitHub UI) rather than by the bot via /lgtm, treat it as a sticky governance decision and skip push invalidation entirely. The bot-applied case (per-SHA /lgtm approval) still invalidates as before. Adds an invalidate-skipped result value and updates docs. :house: Remote-Dev: homespace --- slash-commands/README.md | 9 ++++++--- slash-commands/action.yml | 10 +++++++--- slash-commands/main.js | 24 ++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/slash-commands/README.md b/slash-commands/README.md index 31aebf6..4f84314 100644 --- a/slash-commands/README.md +++ b/slash-commands/README.md @@ -32,7 +32,10 @@ Listens for PR comments and push events, then: Disabled unless `stageblog-workflow` is set. - **New commits pushed** — removes the `accepted` label, dismisses the approval, disables auto-merge, and posts a brief comment asking for - re-approval. + re-approval. **Exception:** if the `accepted` label was most recently + applied manually by a member of `always-allow-teams` or `force-allow-teams` + (rather than by the bot via `/lgtm`), it is treated as a sticky governance + decision and left in place — nothing is removed or dismissed. Commands are parsed from the **first line** of the comment (case-insensitive, must match `^/cmd\b`). Unauthorized attempts, or a PR author trying to `/lgtm` @@ -213,7 +216,7 @@ jobs: | `always-allow-teams` | | `core-maintainers` | Comma-separated team slugs (in the repo's org) whose members can `/lgtm` any PR. **Only these teams can `/lgtm`** — CODEOWNERS does not grant it. Members may also `/hold`/`/stageblog` any PR. | | `force-allow-teams` | | `core-maintainers,lead-maintainers` | Comma-separated team slugs whose members may use `/lgtm force` (bypasses self-approval guard). CODEOWNERS does **not** grant this — only team membership. | | `codeowners-path` | | `.github/CODEOWNERS` | Path to CODEOWNERS, fetched from the PR's **base** ref (never head — tamper-proof). Grants `/hold` and `/stageblog` only, not `/lgtm`. | -| `invalidate-on-push` | | `'true'` | Remove `accepted` label + dismiss approval + disable auto-merge + comment when new commits are pushed. Set `'false'` to keep approval across pushes. | +| `invalidate-on-push` | | `'true'` | Remove `accepted` label + dismiss approval + disable auto-merge + comment when new commits are pushed. Skipped if the label was most recently applied manually by an `always-allow-teams` or `force-allow-teams` member (not the bot via `/lgtm`). Set `'false'` to keep approval across pushes unconditionally. | | `submit-review` | | `'true'` | Submit an APPROVE review alongside the label on `/lgtm`. Set `'false'` to use label only. | | `enable-auto-merge` | | `'true'` | Enable auto-merge after `/lgtm` (disabled on `/lgtm cancel` and push invalidation). Requires repo setting "Allow auto-merge". | | `auto-merge-method` | | `squash` | Merge method for auto-merge: `squash`, `merge`, or `rebase` | @@ -229,7 +232,7 @@ jobs: | Output | Description | |---|---| -| `result` | One of: `lgtm-added`, `lgtm-forced`, `lgtm-removed`, `hold-added`, `hold-removed`, `invalidated`, `unauthorized`, `force-unauthorized`, `self-lgtm-blocked`, `stageblog-dispatched`, `stageblog-not-blog`, `stageblog-disabled`, `noop` | +| `result` | One of: `lgtm-added`, `lgtm-forced`, `lgtm-removed`, `hold-added`, `hold-removed`, `invalidated`, `invalidate-skipped`, `unauthorized`, `force-unauthorized`, `self-lgtm-blocked`, `stageblog-dispatched`, `stageblog-not-blog`, `stageblog-disabled`, `noop` | | `actor` | Login of the commenter (empty for non-comment triggers) | ## CODEOWNERS pattern support diff --git a/slash-commands/action.yml b/slash-commands/action.yml index d560e56..1f76e02 100644 --- a/slash-commands/action.yml +++ b/slash-commands/action.yml @@ -45,7 +45,10 @@ inputs: invalidate-on-push: description: >- Remove the approved label and dismiss the bot review when new commits - are pushed to the PR. Set to 'false' to keep approval across pushes. + are pushed to the PR. Skipped if the label was most recently applied + manually by a member of always-allow-teams or force-allow-teams (not + the bot via /lgtm). Set to 'false' to keep approval across pushes + unconditionally. required: false default: 'true' submit-review: @@ -112,8 +115,9 @@ outputs: result: description: >- One of: lgtm-added, lgtm-forced, lgtm-removed, hold-added, hold-removed, - invalidated, unauthorized, force-unauthorized, self-lgtm-blocked, - stageblog-dispatched, stageblog-not-blog, stageblog-disabled, noop + invalidated, invalidate-skipped, unauthorized, force-unauthorized, + self-lgtm-blocked, stageblog-dispatched, stageblog-not-blog, + stageblog-disabled, noop value: ${{ steps.handle.outputs.result }} actor: description: Login of the user who triggered the action (commenter) diff --git a/slash-commands/main.js b/slash-commands/main.js index f2da594..fbbfc42 100644 --- a/slash-commands/main.js +++ b/slash-commands/main.js @@ -115,6 +115,17 @@ module.exports = async ({ github, context, core }) => { return false; } + async function lastLabeledBy(prNumber, labelName) { + const events = await github.paginate(github.rest.issues.listEvents, { + owner, repo, issue_number: prNumber, per_page: 100, + }); + let actor = null; + for (const e of events) { + if (e.event === 'labeled' && e.label?.name === labelName) actor = e.actor; + } + return actor; + } + async function isAuthorized(user, pr) { // 1. always-allow teams if (await isInAlwaysAllowTeam(user)) return true; @@ -336,6 +347,19 @@ module.exports = async ({ github, context, core }) => { const pr = context.payload.pull_request; const hasApproved = pr.labels.some(l => l.name === approvedLabel); if (!hasApproved) return setResult('noop'); + // If a core/lead maintainer applied the label directly (not the bot via + // /lgtm), treat it as a sticky governance decision rather than a per-SHA + // approval. Check the union of always-allow + force-allow teams so lead + // maintainers are covered even when always-allow-teams is narrower. + const labeler = await lastLabeledBy(pr.number, approvedLabel); + if (labeler && labeler.type !== 'Bot') { + for (const team of [...alwaysAllowTeams, ...forceAllowTeams]) { + if (await isTeamMember(labeler.login, team)) { + core.info(`'${approvedLabel}' was applied manually by @${labeler.login} (member of @${owner}/${team}); skipping push invalidation.`); + return setResult('invalidate-skipped', labeler.login); + } + } + } await removeLabel(pr.number, approvedLabel); await dismissBotApprovals(pr.number, 'New commits pushed — approval invalidated.'); await disableAutoMerge(pr.node_id);