Merge pull request #12 from modelcontextprotocol/den/lgtm-status-direct-write

slash-commands: write commit status directly from /lgtm handler

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,
+The action writes a commit status (`slash-commands/approval` by default) after
-which sets a commit status that stays **pending** until `approved` is present
+each label mutation — this is the merge gate. Mark it as a required status
-and goes **failure** when `do-not-merge/hold` is present — making the labels
+check in branch protection. The commit status can **only** be set to `success`
-an actual merge gate via branch protection without a red ❌ on every fresh PR.
+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.
+Sets the **initial** `pending` commit status when a PR opens, and forces it
-Use in a `status` job that runs on every `pull_request_target` event. The job
+back to `pending` on every push (new SHA = new approval required). Run in a
-itself always succeeds — the commit status it creates is what branch protection
+`status` job on `pull_request_target` events. Mark the `status-context` value
-enforces. Make the `status-context` value a required status check.
+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. |
+| `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`. Disabling this opens an auto-merge race. |
+| `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

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 }}

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 = '<!-- 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);
 };

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.
+  Sets the initial pending commit status on the PR head SHA at open time and
-  Stays `pending` until the approved label is present, goes `failure` when the
+  forces it back to pending on push (new SHA, approval invalidated). Never
-  hold label is present, and `success` when approved-and-not-held. Use this
+  sets success — only the main slash-commands action's auth-gated /lgtm
-  instead of an exit-code-based status job so PRs show a yellow pending check
+  handler may do that, so a manually-added approved label cannot bypass the
-  (not a red failure) while awaiting /lgtm.
+  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
+      regardless of labels. Must match the main slash-commands action's
-      invalidate-on-push setting. Without this, on a push the event payload
+      invalidate-on-push setting.
-      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:
-    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.
+          // Never set success here. The only success writer is the main
-          // If invalidation is on, the handle job is about to (or already did)
+          // action's /lgtm handler, which is auth-gated. Reading the approved
-          // remove the approved label in parallel — but we can't see that here.
+          // label would let anyone with triage+ perms bypass auth by labeling
-          // Force pending so auto-merge can't slip through on a stale success.
+          // manually. This sub-action's job is: pending on open, pending on
-          // The subsequent unlabeled event will re-evaluate (and stay pending).
+          // 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);
+            state = 'pending';
-            if (labels.includes(holdLabel)) {
+            description = `Awaiting /lgtm from a maintainer or CODEOWNER`;
-              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({