From 447a6c5093ab62b04cd38c454cd9cce4684773a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Hou=C5=A1ka?= Date: Fri, 10 Jul 2026 13:10:26 +0200 Subject: [PATCH] fix: target intended users in lgtm approvals Maintainers frequently direct lgtm at tagged users who volunteer on an issue, but approval previously applied to the issue or PR author. Mentioned users are now approval targets, with the author as the fallback. Prose and examples can also contain lgtm without intending approval. Only treat lgtm or lgtmi at the start, after leading mentions, or at the end as commands. Process PR comments too, where approvals previously had no effect. --- .github/workflows/approve-contributor.yml | 132 ++++++++++++++-------- .github/workflows/issue-gate.yml | 2 +- .github/workflows/pr-gate.yml | 4 +- CONTRIBUTING.md | 6 +- 4 files changed, 94 insertions(+), 50 deletions(-) diff --git a/.github/workflows/approve-contributor.yml b/.github/workflows/approve-contributor.yml index d2d8a58d770..409a4c32e67 100644 --- a/.github/workflows/approve-contributor.yml +++ b/.github/workflows/approve-contributor.yml @@ -6,11 +6,11 @@ on: jobs: approve: - if: ${{ !github.event.issue.pull_request }} runs-on: ubuntu-latest permissions: contents: write issues: write + pull-requests: write steps: - name: Checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -30,17 +30,18 @@ jobs: const commenter = context.payload.comment.user.login; const commentBody = (context.payload.comment.body || '').trim(); - let targetCapability; - if (/\blgtmi\b/i.test(commentBody)) { - targetCapability = 'issue'; - } else if (/\blgtm\b/i.test(commentBody)) { - targetCapability = 'pr'; - } else { - console.log('Comment does not match lgtm or lgtmi'); + const approvalAtStartPattern = /^[\s.]*(?:@[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?(?:\s*,\s*|[.:]\s*|\s+))*(lgtmi|lgtm)(?=$|[\s.])/i; + const approvalAtEndPattern = /(?:^|[\s.])(lgtmi|lgtm)[\s.]*$/i; + const approvalMatch = commentBody.match(approvalAtStartPattern) ?? commentBody.match(approvalAtEndPattern); + + if (!approvalMatch) { + console.log('Comment does not start or end with lgtm or lgtmi'); core.setOutput('status', 'skipped'); return; } + const targetCapability = approvalMatch[1].toLowerCase() === 'lgtmi' ? 'issue' : 'pr'; + try { const { data: permissionLevel } = await github.rest.repos.getCollaboratorPermissionLevel({ owner: context.repo.owner, @@ -59,6 +60,24 @@ jobs: return; } + function parseMentionedUsers(body) { + const users = []; + const seenUsers = new Set(); + const mentionPattern = /(^|[^A-Za-z0-9_])@([A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?)(?![A-Za-z0-9-]|\/)/g; + + for (const match of body.matchAll(mentionPattern)) { + const username = match[2]; + const normalizedUser = username.toLowerCase(); + if (seenUsers.has(normalizedUser)) { + continue; + } + seenUsers.add(normalizedUser); + users.push(username); + } + + return users; + } + function parseApprovedUsers(content) { const lines = content.split('\n'); const entries = []; @@ -113,63 +132,88 @@ jobs: const content = fs.readFileSync(APPROVED_FILE, 'utf8'); const { entries, users } = parseApprovedUsers(content); - const normalizedAuthor = issueAuthor.toLowerCase(); - const existingEntry = users.get(normalizedAuthor); - const existingCapability = existingEntry?.capability ?? null; + const mentionedUsers = parseMentionedUsers(commentBody); + const approvalTargets = mentionedUsers.length > 0 ? mentionedUsers : [issueAuthor]; + const changedTargets = []; + const alreadyTargets = []; + + for (const username of approvalTargets) { + const normalizedUser = username.toLowerCase(); + const existingEntry = users.get(normalizedUser); + const existingCapability = existingEntry?.capability ?? null; + + if (existingCapability === 'pr' || existingCapability === targetCapability) { + alreadyTargets.push(existingEntry?.username ?? username); + console.log(`${username} is already approved for ${existingCapability}`); + continue; + } - if (existingCapability === 'pr' || existingCapability === targetCapability) { - core.setOutput('status', 'already'); - core.setOutput('capability', existingCapability); - console.log(`${issueAuthor} is already approved for ${existingCapability}`); - return; + if (existingEntry) { + existingEntry.capability = targetCapability; + changedTargets.push(existingEntry.username); + } else { + const entry = { type: 'user', username, normalizedUser, capability: targetCapability }; + entries.push(entry); + users.set(normalizedUser, entry); + changedTargets.push(username); + } + + console.log(`Set ${username} capability to ${targetCapability}`); } - if (existingEntry) { - existingEntry.capability = targetCapability; - } else { - entries.push({ type: 'user', username: issueAuthor, normalizedUser: normalizedAuthor, capability: targetCapability }); + core.setOutput('capability', targetCapability); + core.setOutput('changed_targets', JSON.stringify(changedTargets)); + core.setOutput('already_targets', JSON.stringify(alreadyTargets)); + + if (changedTargets.length === 0) { + core.setOutput('status', 'already'); + return; } fs.writeFileSync(APPROVED_FILE, stringifyApprovedUsers(entries)); - core.setOutput('status', existingCapability ? 'updated' : 'added'); - core.setOutput('capability', targetCapability); - console.log(`Set ${issueAuthor} capability to ${targetCapability}`); + core.setOutput('status', 'changed'); - name: Commit and push - if: steps.update.outputs.status == 'added' || steps.update.outputs.status == 'updated' + if: steps.update.outputs.status == 'changed' run: | git config user.name "github-actions[bot]" git config user.email "github-actions[bot]@users.noreply.github.com" git add .github/APPROVED_CONTRIBUTORS - git diff --staged --quiet || git commit -m "chore: approve contributor ${{ github.event.issue.user.login }}" + git diff --staged --quiet || git commit -m "chore: approve contributors from issue #${{ github.event.issue.number }}" git push - name: Comment on issue - if: steps.update.outputs.status == 'added' || steps.update.outputs.status == 'updated' || steps.update.outputs.status == 'already' + if: steps.update.outputs.status == 'changed' || steps.update.outputs.status == 'already' uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + CAPABILITY: ${{ steps.update.outputs.capability }} + CHANGED_TARGETS: ${{ steps.update.outputs.changed_targets }} + ALREADY_TARGETS: ${{ steps.update.outputs.already_targets }} with: script: | - const issueAuthor = context.payload.issue.user.login; - const capability = '${{ steps.update.outputs.capability }}'; + const capability = process.env.CAPABILITY; + const changedTargets = JSON.parse(process.env.CHANGED_TARGETS || '[]'); + const alreadyTargets = JSON.parse(process.env.ALREADY_TARGETS || '[]'); const defaultBranch = context.payload.repository.default_branch; - let body; - - if ('${{ steps.update.outputs.status }}' === 'already') { - body = `@${issueAuthor} is already approved.`; - } else if (capability === 'issue') { - body = [ - `@${issueAuthor} approved for issues. Your future issues will not be auto-closed. PRs still require \`lgtm\`.`, - '', - `See [CONTRIBUTING.md](https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${defaultBranch}/CONTRIBUTING.md).`, - ].join('\n'); - } else { - body = [ - `@${issueAuthor} approved for issues and PRs. Your future issues and PRs will not be auto-closed.`, - '', - `See [CONTRIBUTING.md](https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${defaultBranch}/CONTRIBUTING.md).`, - ].join('\n'); + const formatTargets = (targets) => targets.map((target) => `@${target}`).join(', '); + const bodyLines = []; + + if (changedTargets.length > 0) { + if (capability === 'issue') { + bodyLines.push(`${formatTargets(changedTargets)} approved for issues. Future issues will not be auto-closed. PRs still require \`lgtm\` at the start of a maintainer reply (optionally after one or more \`@username\` mentions) or at the end.`); + } else { + bodyLines.push(`${formatTargets(changedTargets)} approved for issues and PRs. Future issues and PRs will not be auto-closed.`); + } } + if (alreadyTargets.length > 0) { + const verb = alreadyTargets.length === 1 ? 'is' : 'are'; + bodyLines.push(`${formatTargets(alreadyTargets)} ${verb} already approved.`); + } + + bodyLines.push('', `See [CONTRIBUTING.md](https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${defaultBranch}/CONTRIBUTING.md).`); + const body = bodyLines.join('\n'); + await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, diff --git a/.github/workflows/issue-gate.yml b/.github/workflows/issue-gate.yml index 02d4000d1a7..03372acaead 100644 --- a/.github/workflows/issue-gate.yml +++ b/.github/workflows/issue-gate.yml @@ -101,7 +101,7 @@ jobs: '', `Maintainers review auto-closed issues daily and reopen worthwhile ones. Issues that do not meet the quality bar in [CONTRIBUTING.md](https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${defaultBranch}/CONTRIBUTING.md) will not be reopened or receive a reply.`, '', - 'If a maintainer replies `lgtmi` on one of your issues, your future issues will stay open. If a maintainer replies `lgtm`, your future issues and PRs will stay open.', + 'If a maintainer replies `lgtmi` on one of your issues, your future issues will stay open. If a maintainer replies `lgtm`, your future issues and PRs will stay open. The command must be at the start of the reply (optionally after one or more `@username` mentions) or at the end.', '', `See [CONTRIBUTING.md](https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${defaultBranch}/CONTRIBUTING.md).`, ].join('\n'); diff --git a/.github/workflows/pr-gate.yml b/.github/workflows/pr-gate.yml index 48e0745fb0d..bc6f7eef11d 100644 --- a/.github/workflows/pr-gate.yml +++ b/.github/workflows/pr-gate.yml @@ -116,11 +116,11 @@ jobs: console.log(`${prAuthor} is not approved, closing PR`); const message = [ - 'This PR was auto-closed. Only contributors approved with `lgtm` can open PRs. Open an issue first.', + 'This PR was auto-closed. Only contributors approved with `lgtm` can open PRs. Open an issue first and ask a maintainer for approval.', '', `Maintainers review auto-closed issues daily. Issues that do not meet the quality bar in [CONTRIBUTING.md](https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${defaultBranch}/CONTRIBUTING.md) will not be reopened or receive a reply.`, '', - 'If a maintainer replies `lgtmi`, your future issues will stay open. If a maintainer replies `lgtm`, your future issues and PRs will stay open.', + 'If a maintainer replies `lgtmi`, your future issues will stay open. If a maintainer replies `lgtm`, your future issues and PRs will stay open. The command must be at the start of the reply (optionally after one or more `@username` mentions) or at the end.', '', `See [CONTRIBUTING.md](https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${defaultBranch}/CONTRIBUTING.md).`, ].join('\n'); diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b0c38322dc0..c6d76f1c574 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -31,7 +31,7 @@ Approval happens through maintainer replies on issues: - `lgtmi`: your future issues will not be auto-closed - `lgtm`: your future issues and PRs will not be auto-closed -`lgtmi` does not grant rights to submit PRs. Only `lgtm` grants rights to submit PRs. +The command must be at the start of the reply (optionally after one or more `@username` mentions) or at the end. `lgtmi` does not grant rights to submit PRs. Only `lgtm` grants rights to submit PRs. ## Quality Bar For Issues @@ -45,7 +45,7 @@ If you open an issue, keep it short, concrete, and worth reading. - Explain why it matters. - If you want to implement the change yourself, say so. -If the issue is real and written well, a maintainer may reopen it, reply `lgtmi`, or reply `lgtm`. +If the issue is real and written well, a maintainer may reopen it or reply with `lgtmi` or `lgtm` in the command position described above. ## Blocking @@ -55,7 +55,7 @@ If you send a large volume of issues through automation, your GitHub account wil ## Before Submitting a PR -Do not open a PR unless you have already been approved with `lgtm`. +Do not open a PR unless you have already been approved by a maintainer using `lgtm` in the command position described above. Before submitting a PR: