fix: target intended users in lgtm approvals (#7023)

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

@@ -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 = [];
 
-            if (existingCapability === 'pr' || existingCapability === targetCapability) {
+            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 (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}`);
+            }
+
+            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');
-              core.setOutput('capability', existingCapability);
-              console.log(`${issueAuthor} is already approved for ${existingCapability}`);
               return;
             }
 
-            if (existingEntry) {
-              existingEntry.capability = targetCapability;
-            } else {
-              entries.push({ type: 'user', username: issueAuthor, normalizedUser: normalizedAuthor, capability: targetCapability });
-            }
-
             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;
+            const formatTargets = (targets) => targets.map((target) => `@${target}`).join(', ');
+            const bodyLines = [];
 
-            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');
+            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,

.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');

.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');

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: