diff --git a/README.md b/README.md index 9021ff8..972cbff 100644 --- a/README.md +++ b/README.md @@ -8,3 +8,4 @@ GitHub Actions relevant to the management of MCP repositories. |---|---| | [`cloudflare-pages-preview/deploy`](cloudflare-pages-preview/README.md) | Deploy a static HTML directory to Cloudflare Pages under a per-PR branch alias, inject noindex headers, and post a sticky PR comment with preview URLs | | [`cloudflare-pages-preview/cleanup`](cloudflare-pages-preview/README.md) | Delete Cloudflare Pages deployments for a closed PR's branch alias and update the sticky comment | +| [`slash-commands`](slash-commands/README.md) | Handle `/lgtm` and `/hold` PR slash commands gated by team membership + CODEOWNERS; invalidate approval on push | diff --git a/slash-commands/README.md b/slash-commands/README.md new file mode 100644 index 0000000..1e10107 --- /dev/null +++ b/slash-commands/README.md @@ -0,0 +1,181 @@ +# slash-commands + +Reusable composite action that implements Prow-style `/lgtm` and `/hold` slash +commands on pull requests, gated by team membership and CODEOWNERS. + +## What it does + +Listens for PR comments and push events, then: + +- **`/lgtm`** — if the commenter is a core maintainer _or_ a CODEOWNER of the + PR's changed files, adds an `approved` label and submits a bot **APPROVE** + review with body "Approved on behalf of @user via `/lgtm`." +- **`/lgtm cancel`** — removes the `approved` label and dismisses the bot + review. +- **`/hold`** — adds a `do-not-merge/hold` label (same authorization rule). +- **`/hold cancel`** or **`/unhold`** — removes the hold label. +- **New commits pushed** — removes the `approved` label, dismisses the bot + review, and posts a brief comment asking for re-approval. + +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` +their own PR, receive a 👎 reaction with no further noise. Successful commands +receive a 👍 reaction. + +The caller workflow adds a separate `status` job that reports a required check +which passes only when `approved` is present and `do-not-merge/hold` is absent +— making the labels an actual merge gate via branch protection. + +## Prerequisites + +- The `approved` and `do-not-merge/hold` labels **must already exist** in the + caller repo (the action does not auto-create them). Suggested colors: + `approved` → `#0e8a16`, `do-not-merge/hold` → `#d93f0b`. +- A CODEOWNERS file at `.github/CODEOWNERS` (or pass a different path via + `codeowners-path`). Without one, only `always-allow-teams` members can act. + +## Required secrets + +Create this secret in the caller repo: GitHub → repo → **Settings → Secrets +and variables → Actions → New repository secret**. + +| Repo secret name | Action input | Value | +|---|---|---| +| `SLASH_COMMANDS_TOKEN` | `github-token` | Fine-grained PAT (or GitHub App installation token) with **Organization → Members: read** plus **Repository → Contents: read, Pull requests: write, Issues: write** | + +> **The default `${{ github.token }}` will NOT work.** Team-membership checks +> (`GET /orgs/{org}/teams/{team}/memberships/{user}`) require `read:org` scope, +> which the automatic `GITHUB_TOKEN` never has — even with a `permissions:` +> block. You _must_ provide a PAT or App token. + +### `SLASH_COMMANDS_TOKEN` + +**Option A — fine-grained PAT (recommended for single-repo setups):** + +GitHub → **Settings → Developer settings → Personal access tokens → Fine-grained +tokens → Generate new token**. Configure: + +| Section | Setting | +|---|---| +| Resource owner | `modelcontextprotocol` (or your org) | +| Repository access | **Only select repositories** → pick the caller repo | +| Repository permissions → Contents | Read-only | +| Repository permissions → Pull requests | Read and write | +| Repository permissions → Issues | Read and write | +| Organization permissions → Members | Read-only | + +Do not grant any write permissions beyond Pull requests + Issues. **Contents** +must remain read-only — the action never writes code. + +**Option B — GitHub App installation token:** + +If your org already uses a bot App, generate an installation token with the +same scopes and store it as `SLASH_COMMANDS_TOKEN`. The App must be installed +on the caller repo _and_ have org-level **Members: read**. + +## Caller workflow requirements + +| | | +|---|---| +| **Triggers** | `issue_comment` (types: `created`) + `pull_request_target` (types: `synchronize`, `opened`, `reopened`, `labeled`, `unlabeled`) | +| **Permissions** | `pull-requests: write`, `issues: write`, `contents: read` | +| **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. | + +## Usage + +```yaml +name: Slash Commands + +on: + issue_comment: + types: [created] + pull_request_target: + types: [synchronize, opened, reopened, labeled, unlabeled] + +permissions: + pull-requests: write + issues: write + contents: read + +jobs: + handle: + if: >- + (github.event_name == 'issue_comment' && github.event.issue.pull_request) || + (github.event_name == 'pull_request_target' && github.event.action == 'synchronize') + runs-on: ubuntu-latest + steps: + - uses: modelcontextprotocol/actions/slash-commands@main + with: + github-token: ${{ secrets.SLASH_COMMANDS_TOKEN }} + always-allow-teams: core-maintainers,lead-maintainers + + # Merge-gate status check — make this a required check in branch protection. + status: + if: github.event_name == 'pull_request_target' + runs-on: ubuntu-latest + steps: + - name: Require approval, block on hold + env: + LABELS: ${{ toJson(github.event.pull_request.labels.*.name) }} + run: | + echo "Labels: $LABELS" + if ! jq -e 'contains(["approved"])' <<<"$LABELS" > /dev/null; then + echo "::error::Missing 'approved' label — comment /lgtm" + exit 1 + fi + if jq -e 'contains(["do-not-merge/hold"])' <<<"$LABELS" > /dev/null; then + echo "::error::'do-not-merge/hold' present — resolve, then /unhold" + exit 1 + fi + echo "approved and not held" +``` + +## Inputs + +| Input | Required | Default | Description | +|---|---|---|---| +| `github-token` | ✅ | — | PAT or App token with `read:org` + repo `pull-requests: write` + `issues: write` + `contents: read`. **Default `GITHUB_TOKEN` will not work** for team checks. | +| `approved-label` | | `approved` | Label added by `/lgtm` | +| `hold-label` | | `do-not-merge/hold` | Label added by `/hold` | +| `always-allow-teams` | | `core-maintainers` | Comma-separated team slugs (in the repo's org) whose members can `/lgtm` or `/hold` any PR regardless of CODEOWNERS | +| `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. | + +## Outputs + +| Output | Description | +|---|---| +| `result` | One of: `lgtm-added`, `lgtm-removed`, `hold-added`, `hold-removed`, `invalidated`, `unauthorized`, `self-lgtm-blocked`, `noop` | +| `actor` | Login of the commenter (empty for non-comment triggers) | + +## CODEOWNERS pattern support + +The inline CODEOWNERS parser handles the patterns currently used in MCP repos: + +| Pattern | Supported | Meaning | +|---|---|---| +| `/path/` | ✅ | Anchored directory prefix (matches anything under `path/`) | +| `/path/to/file.ext` | ✅ | Exact anchored path | +| `*.ext` | ✅ | Any file with extension `.ext` at any depth | +| `/path/**/*.ext` | ✅ | Glob at any depth under `/path/` | +| `**/file` | ✅ | File named `file` at any depth | +| `@user` / `@org/team` | ✅ | Owner: individual user or team (team org must match repo org) | +| `!negation` | ❌ | Not supported (not in use in MCP repos) | +| Escaped spaces (`\ `) | ❌ | Not supported | + +Last-match-wins per file, consistent with GitHub's native CODEOWNERS semantics. + +## Security notes + +- **Comment body injection** — command parsing is a fixed regex on the + first line in JS; comment text is never interpolated into a shell. +- **CODEOWNERS tampering** — fetched from `pr.base.ref`, never the PR head. + An attacker cannot add themselves as CODEOWNER in the PR under review. +- **Workflow modification** — `issue_comment` and `pull_request_target` run + from the default-branch workflow definition. The action does not check out + PR code. +- **Self-approval** — explicitly blocked before the auth check. +- **Review provenance** — the bot review body always names the real approver + ("on behalf of @login"), even though GitHub shows the review as bot-authored. diff --git a/slash-commands/action.yml b/slash-commands/action.yml new file mode 100644 index 0000000..274bd69 --- /dev/null +++ b/slash-commands/action.yml @@ -0,0 +1,331 @@ +name: PR Slash Commands +description: >- + Handles /lgtm and /hold slash commands on PRs. Authorized users + (core maintainers or CODEOWNERS of the PR's changed files) can approve via + /lgtm (adds a label + bot APPROVE review) or block via /hold. Approval is + invalidated when new commits are pushed. + +inputs: + github-token: + description: >- + Token for API calls. MUST have read:org scope to check team membership — + the default GITHUB_TOKEN cannot do this. Use a fine-grained PAT with + Organization Members:read + Repository Contents:read + Pull requests:write + + Issues:write, or a GitHub App installation token with equivalent perms. + required: true + approved-label: + description: Label added by /lgtm and used by the status job as the merge gate + required: false + default: approved + hold-label: + description: Label added by /hold to block merging + required: false + default: do-not-merge/hold + always-allow-teams: + description: >- + Comma-separated team slugs (within the repo's org) whose members may + /lgtm or /hold any PR regardless of CODEOWNERS + required: false + default: core-maintainers + codeowners-path: + description: Path to the CODEOWNERS file (fetched from the PR base branch) + required: false + default: .github/CODEOWNERS + 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. + required: false + default: 'true' + submit-review: + description: >- + Whether to submit a bot APPROVE review alongside the label on /lgtm. + Set to 'false' to rely on the label alone. + required: false + default: 'true' + +outputs: + result: + description: >- + One of: lgtm-added, lgtm-removed, hold-added, hold-removed, invalidated, + unauthorized, self-lgtm-blocked, noop + value: ${{ steps.handle.outputs.result }} + actor: + description: Login of the user who triggered the action (commenter) + value: ${{ steps.handle.outputs.actor }} + +runs: + using: composite + steps: + - name: Handle slash command + id: handle + uses: actions/github-script@v8 + env: + APPROVED_LABEL: ${{ inputs.approved-label }} + HOLD_LABEL: ${{ inputs.hold-label }} + ALWAYS_ALLOW_TEAMS: ${{ inputs.always-allow-teams }} + CODEOWNERS_PATH: ${{ inputs.codeowners-path }} + INVALIDATE_ON_PUSH: ${{ inputs.invalidate-on-push }} + SUBMIT_REVIEW: ${{ inputs.submit-review }} + with: + github-token: ${{ inputs.github-token }} + script: | + const { owner, repo } = context.repo; + const approvedLabel = process.env.APPROVED_LABEL; + const holdLabel = process.env.HOLD_LABEL; + const alwaysAllowTeams = process.env.ALWAYS_ALLOW_TEAMS + .split(',').map(s => s.trim()).filter(Boolean); + const codeownersPath = process.env.CODEOWNERS_PATH; + const invalidateOnPush = process.env.INVALIDATE_ON_PUSH === 'true'; + const submitReview = process.env.SUBMIT_REVIEW === 'true'; + const reviewMarker = ''; + + function setResult(result, actor) { + core.setOutput('result', result); + core.setOutput('actor', actor || ''); + core.info(`Result: ${result}${actor ? ` (actor: ${actor})` : ''}`); + } + + // --- CODEOWNERS pattern → regex --------------------------------- + // Supports: leading-/ anchoring, trailing-/ directory match, + // `**` (any depth), `*` (single segment), `?`. No `!` negation. + function patternToRegex(pattern) { + let p = pattern; + const anchored = p.startsWith('/'); + if (anchored) p = p.slice(1); + const dirMatch = p.endsWith('/'); + if (dirMatch) p = p.slice(0, -1); + // Escape regex meta chars except our glob tokens * and ? + let re = p.replace(/[.+^${}()|[\]\\]/g, '\\$&'); + // Handle globs. Use placeholders so single-* replacement doesn't + // catch ** sequences. `**/` = zero or more directories. + re = re.replace(/\*\*\//g, '\u0001'); + re = re.replace(/\*\*/g, '\u0000'); + re = re.replace(/\*/g, '[^/]*'); + re = re.replace(/\?/g, '[^/]'); + re = re.replace(/\u0001/g, '(?:.*/)?'); + re = re.replace(/\u0000/g, '.*'); + // Anchoring: leading `/` → match from repo root; otherwise + // match at any path depth (start or after a `/`). + const prefix = anchored ? '^' : '(^|/)'; + // Trailing `/` → directory prefix (match anything under it). + // Otherwise must match to end of string. + const suffix = dirMatch ? '/.*$' : '$'; + return new RegExp(prefix + re + suffix); + } + + function parseCodeowners(content) { + const rules = []; + for (const raw of content.split('\n')) { + const line = raw.replace(/#.*$/, '').trim(); + if (!line) continue; + const parts = line.split(/\s+/); + const pattern = parts[0]; + const owners = parts.slice(1); + if (!owners.length) continue; + rules.push({ regex: patternToRegex(pattern), owners }); + } + return rules; + } + + function ownersForFile(rules, file) { + // Last match wins. + let owners = null; + for (const r of rules) { + if (r.regex.test(file)) owners = r.owners; + } + return owners || []; + } + + // --- Auth helpers ----------------------------------------------- + const teamMembershipCache = new Map(); + async function isTeamMember(user, teamSlug) { + const key = `${teamSlug}:${user}`; + if (teamMembershipCache.has(key)) return teamMembershipCache.get(key); + try { + const { data } = await github.rest.teams.getMembershipForUserInOrg({ + org: owner, team_slug: teamSlug, username: user, + }); + const ok = data.state === 'active'; + teamMembershipCache.set(key, ok); + return ok; + } catch (e) { + if (e.status === 404) { + teamMembershipCache.set(key, false); + return false; + } + // 403 here almost always means the token lacks read:org. + if (e.status === 403) { + core.warning(`Team membership check for @${owner}/${teamSlug} returned 403 — the provided github-token likely lacks read:org (Organization Members:read) scope. Treating as not-a-member.`); + teamMembershipCache.set(key, false); + return false; + } + throw e; + } + } + + async function isAuthorized(user, pr) { + // 1. always-allow teams + for (const team of alwaysAllowTeams) { + if (await isTeamMember(user, team)) { + core.info(`@${user} authorized via always-allow team @${owner}/${team}`); + return true; + } + } + // 2. CODEOWNERS of the PR's changed files (from BASE ref) + let content; + try { + const { data } = await github.rest.repos.getContent({ + owner, repo, path: codeownersPath, ref: pr.base.ref, + }); + content = Buffer.from(data.content, data.encoding).toString('utf8'); + } catch (e) { + if (e.status === 404) { + core.info(`No CODEOWNERS found at ${codeownersPath} on ${pr.base.ref}`); + return false; + } + throw e; + } + const rules = parseCodeowners(content); + const files = await github.paginate(github.rest.pulls.listFiles, { + owner, repo, pull_number: pr.number, per_page: 100, + }); + const ownerSet = new Set(); + for (const f of files) { + for (const o of ownersForFile(rules, f.filename)) ownerSet.add(o); + } + core.info(`CODEOWNERS for this PR's files: ${[...ownerSet].join(', ') || '(none)'}`); + for (const o of ownerSet) { + if (o.includes('/')) { + // @org/team — we require the org to match our repo's org + const [oOrg, oTeam] = o.replace(/^@/, '').split('/'); + if (oOrg.toLowerCase() !== owner.toLowerCase()) continue; + if (await isTeamMember(user, oTeam)) { + core.info(`@${user} authorized via CODEOWNER team ${o}`); + return true; + } + } else { + // @user + if (o.replace(/^@/, '').toLowerCase() === user.toLowerCase()) { + core.info(`@${user} authorized via CODEOWNER user entry`); + return true; + } + } + } + return false; + } + + // --- Label & review helpers ------------------------------------- + async function addLabel(prNumber, label) { + await github.rest.issues.addLabels({ + owner, repo, issue_number: prNumber, labels: [label], + }); + } + async function removeLabel(prNumber, label) { + try { + await github.rest.issues.removeLabel({ + owner, repo, issue_number: prNumber, name: label, + }); + } catch (e) { + if (e.status !== 404) throw e; + } + } + async function react(commentId, content) { + await github.rest.reactions.createForIssueComment({ + owner, repo, comment_id: commentId, content, + }); + } + async function submitApproval(prNumber, onBehalfOf) { + await github.rest.pulls.createReview({ + owner, repo, pull_number: prNumber, event: 'APPROVE', + body: `${reviewMarker}\nApproved on behalf of @${onBehalfOf} via \`/lgtm\`.`, + }); + } + async function dismissBotApprovals(prNumber, reason) { + const reviews = await github.paginate(github.rest.pulls.listReviews, { + owner, repo, pull_number: prNumber, per_page: 100, + }); + for (const r of reviews) { + if (r.state !== 'APPROVED') continue; + if (!r.body || !r.body.includes(reviewMarker)) continue; + try { + await github.rest.pulls.dismissReview({ + owner, repo, pull_number: prNumber, review_id: r.id, + message: reason, + }); + } catch (e) { + core.warning(`Failed to dismiss review ${r.id}: ${e.message}`); + } + } + } + + // --- Dispatch --------------------------------------------------- + const ev = context.eventName; + + // Invalidation on push + if (ev === 'pull_request_target' && context.payload.action === 'synchronize') { + if (!invalidateOnPush) return setResult('noop'); + const pr = context.payload.pull_request; + const hasApproved = pr.labels.some(l => l.name === approvedLabel); + if (!hasApproved) return setResult('noop'); + await removeLabel(pr.number, approvedLabel); + await dismissBotApprovals(pr.number, 'New commits pushed — approval invalidated.'); + await github.rest.issues.createComment({ + owner, repo, issue_number: pr.number, + body: `New commits were pushed — removed the \`${approvedLabel}\` label. Re-approve with \`/lgtm\`.`, + }); + return setResult('invalidated'); + } + + // Comment commands + if (ev !== 'issue_comment') return setResult('noop'); + if (!context.payload.issue.pull_request) return setResult('noop'); + + const commentBody = context.payload.comment.body || ''; + const commentId = context.payload.comment.id; + const actor = context.payload.comment.user.login; + const firstLine = commentBody.split('\n')[0].trim(); + const m = firstLine.match(/^\/(lgtm|hold|unhold)\b\s*(cancel)?\s*$/i); + if (!m) return setResult('noop', actor); + + const cmd = m[1].toLowerCase(); + const cancel = !!m[2] || cmd === 'unhold'; + + const prNumber = context.payload.issue.number; + const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: prNumber }); + if (pr.state !== 'open') return setResult('noop', actor); + + // Self-approval guard (only for /lgtm, not /hold) + if (cmd === 'lgtm' && !cancel && actor.toLowerCase() === pr.user.login.toLowerCase()) { + await react(commentId, '-1'); + return setResult('self-lgtm-blocked', actor); + } + + // Auth check (same rule for all commands) + if (!(await isAuthorized(actor, pr))) { + await react(commentId, '-1'); + return setResult('unauthorized', actor); + } + + // Execute + if (cmd === 'lgtm' && !cancel) { + await addLabel(prNumber, approvedLabel); + if (submitReview) await submitApproval(prNumber, actor); + await react(commentId, '+1'); + 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'); + return setResult('lgtm-removed', actor); + } + if ((cmd === 'hold' && !cancel)) { + await addLabel(prNumber, holdLabel); + await react(commentId, '+1'); + return setResult('hold-added', actor); + } + // /hold cancel or /unhold + await removeLabel(prNumber, holdLabel); + await react(commentId, '+1'); + return setResult('hold-removed', actor);