From 7992430229dfb4df294a81c977c364ec5f5f5de1 Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Sat, 2 May 2026 08:16:13 +0200 Subject: [PATCH] fix: markProcessed on chatops auth-fail / exception paths (Bug #6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: QA bug-hunter flagged that auth-failure early returns and exceptions in `handleChatopsIssue` skipped `markProcessed`. Probot retries the webhook ~5 times on 5xx/timeout, so the user got duplicate "❌ unauthorised" comments and duplicate analysis-report issues. What: - issues.opened: wrap the body in try/finally so the delivery is marked even when handleChatopsIssue or any octokit call throws. Logger captures the error so we don't lose visibility, but the handler swallows it — the alternative is Probot re-running a handler that already posted comments / created issues / enqueued work. - issue_comment.created: surgical fix — every auth-failure return (10 sites: requireOrgMember/requireAllowedUser, /allow-merge-commit admin check, /review-pr not-a-PR check) now marks before returning. Free-text comments that don't match any slash command stay unmarked (preserves existing dedup-skip behaviour for common case). Test plan: - 4 new tests: - issues.opened marks even when octokit.request throws - /sync-all-repos marks on non-membership rejection - /sync-all-repos marks on not-allowed-user rejection - /review-pr marks on not-a-PR rejection - npm test → 837 pass (was 834) - npm run lint → clean Risk: low — behaviour change is narrow (paths that already had visible side effects now also mark dedup state). No paths that previously marked stop marking. The handler now swallows exceptions in issues.opened instead of letting them propagate to Probot — but the old behaviour was Probot logging + retrying, which IS the bug. Co-Authored-By: Claude Opus 4.7 (1M context) --- __tests__/integration/app.test.js | 48 ++++++++++ src/app.js | 152 ++++++++++++++++-------------- 2 files changed, 130 insertions(+), 70 deletions(-) diff --git a/__tests__/integration/app.test.js b/__tests__/integration/app.test.js index da1cb5a..e51c46c 100644 --- a/__tests__/integration/app.test.js +++ b/__tests__/integration/app.test.js @@ -409,6 +409,29 @@ describe('app', () => { const body = context.octokit.issues.createComment.mock.calls[0][0].body; expect(body).toMatch(/task store offline/i); }); + + // Bug #6: even when an octokit call inside the handler throws, the + // delivery must be marked processed. Otherwise Probot retries the + // webhook and re-enqueues / re-comments / re-creates state. + it('marks delivery processed even when an octokit call throws', async () => { + _setConfigForTesting({ + controller_repo: { enabled: true, repo: 'pulseengine/repo-requests', label: 'repo-request' } + }); + const { handlers } = setupApp(); + const context = createIssueOpenedContext(); + // Force createComment to fail mid-handler. + context.octokit.request.mockImplementation((route) => { + if (route === 'POST /repos/{owner}/{repo}/issues/{issue_number}/comments') { + return Promise.reject(new Error('GitHub 503')); + } + return Promise.resolve({ status: 200, data: {} }); + }); + + // The handler swallows the error (logged) and marks processed. + await handlers['issues.opened'](context); + + expect(markProcessed).toHaveBeenCalledWith('delivery-issues-1'); + }); }); // ========================================================================= @@ -1189,6 +1212,28 @@ describe('app', () => { expect(synchronizeAllRepositories).not.toHaveBeenCalled(); }); + // Bug #6: auth-failure paths must mark the delivery processed, + // otherwise Probot's webhook retry posts the same "❌ unauthorised" + // comment over and over. + it('marks processed when rejected for non-membership', async () => { + checkOrganizationMembership.mockResolvedValue(false); + const { handlers } = setupApp(); + const context = createIssueCommentContext('/sync-all-repos'); + await handlers['issue_comment.created'](context); + + expect(markProcessed).toHaveBeenCalledWith('delivery-2'); + }); + + it('marks processed when rejected for not being an allowed user', async () => { + _setConfigForTesting({ allowed_command_users: ['someone-else'] }); + const { handlers } = setupApp(); + const context = createIssueCommentContext('/sync-all-repos'); + await handlers['issue_comment.created'](context); + + expect(synchronizeAllRepositories).not.toHaveBeenCalled(); + expect(markProcessed).toHaveBeenCalledWith('delivery-2'); + }); + it('does not mark processed when deliveryId is absent', async () => { const { handlers } = setupApp(); const context = createIssueCommentContext('/sync-all-repos', { id: undefined }); @@ -1693,6 +1738,9 @@ describe('app', () => { const body = context.octokit.issues.createComment.mock.calls[0][0].body; expect(body).toContain('can only be used on pull requests'); expect(reviewPullRequest).not.toHaveBeenCalled(); + // Bug #6: not-a-PR rejection still posts a comment, so it must + // mark the delivery to prevent retried duplicate replies. + expect(markProcessed).toHaveBeenCalledWith('delivery-2'); }); it('posts working comment, reviews PR, and deletes working comment on success', async () => { diff --git a/src/app.js b/src/app.js index db1446d..7b11454 100644 --- a/src/app.js +++ b/src/app.js @@ -329,91 +329,91 @@ function registerApp(app, options = {}) { const deliveryId = context.id; if (deliveryId && isProcessed(deliveryId)) return; - const { issue, repository, sender } = context.payload; - const fullName = repository.full_name; - - // ChatOps issue forms in the admin repo: a `chatops:` label on - // a newly-opened issue triggers the corresponding ChatOps action, - // bot replies on the issue, optionally closes it. - const chatopsCfg = getChatopsRepoConfig(); - if (chatopsCfg?.enabled && fullName === chatopsCfg.repo) { - const chatopsLabel = (issue.labels || []) - .map((l) => l?.name) - .find((n) => typeof n === 'string' && n.startsWith('chatops:')); - if (chatopsLabel) { - await handleChatopsIssue(context, chatopsLabel, sender, issue, repository); - if (deliveryId) markProcessed(deliveryId); - return; + // Bug #6: every code path that *touches* an issue in the chatops/ + // controller repo must mark the delivery processed — including auth- + // failure replies and exceptions. Otherwise Probot's webhook retry + // (~5 attempts over 8 hours on 5xx/timeout) fires the side-effecting + // path again. Symptom: duplicate "❌ unauthorised" comments, duplicate + // analysis-report issues, duplicate provisioning enqueues. + try { + const { issue, repository, sender } = context.payload; + const fullName = repository.full_name; + + // ChatOps issue forms in the admin repo: a `chatops:` label on + // a newly-opened issue triggers the corresponding ChatOps action, + // bot replies on the issue, optionally closes it. + const chatopsCfg = getChatopsRepoConfig(); + if (chatopsCfg?.enabled && fullName === chatopsCfg.repo) { + const chatopsLabel = (issue.labels || []) + .map((l) => l?.name) + .find((n) => typeof n === 'string' && n.startsWith('chatops:')); + if (chatopsLabel) { + await handleChatopsIssue(context, chatopsLabel, sender, issue, repository); + return; + } } - } - const ctrl = getControllerRepoConfig(); - if (!ctrl?.enabled) { - if (deliveryId) markProcessed(deliveryId); - return; - } + const ctrl = getControllerRepoConfig(); + if (!ctrl?.enabled) return; + if (fullName !== ctrl.repo) return; - if (fullName !== ctrl.repo) { - if (deliveryId) markProcessed(deliveryId); - return; - } + const expectedLabel = ctrl.label || 'repo-request'; + const hasLabel = (issue.labels || []).some((l) => l.name === expectedLabel); + if (!hasLabel) return; - const expectedLabel = ctrl.label || 'repo-request'; - const hasLabel = (issue.labels || []).some((l) => l.name === expectedLabel); - if (!hasLabel) { - if (deliveryId) markProcessed(deliveryId); - return; - } + const fields = parseIssueFormBody(issue.body || ''); + const validation = validateProvisionRequest(fields); - const fields = parseIssueFormBody(issue.body || ''); - const validation = validateProvisionRequest(fields); + const owner = repository.owner.login; + const repo = repository.name; - const owner = repository.owner.login; - const repo = repository.name; + if (!validation.valid) { + await issueOps.createComment(context.octokit,{ + owner, + repo, + issue_number: issue.number, + body: `❌ Provisioning request rejected: ${validation.error}` + }); + return; + } - if (!validation.valid) { - await issueOps.createComment(context.octokit,{ - owner, - repo, - issue_number: issue.number, - body: `❌ Provisioning request rejected: ${validation.error}` - }); - if (deliveryId) markProcessed(deliveryId); - return; - } + const enqueueTask = getEnqueueTask(); + if (!enqueueTask) { + getLogger().warn('Task store not initialized — cannot enqueue provision-repo task'); + await issueOps.createComment(context.octokit,{ + owner, + repo, + issue_number: issue.number, + body: '⚠️ Provisioning is not currently available (task store offline). Please retry later.' + }); + return; + } + + enqueueTask( + 'provision-repo', + `provision-repo:${owner}/${repo}#${issue.number}`, + { + sourceOwner: owner, + sourceRepo: repo, + sourceIssue: issue.number, + params: validation.params + } + ); - const enqueueTask = getEnqueueTask(); - if (!enqueueTask) { - getLogger().warn('Task store not initialized — cannot enqueue provision-repo task'); await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issue.number, - body: '⚠️ Provisioning is not currently available (task store offline). Please retry later.' + body: `✅ Request accepted. Provisioning \`${validation.params.name}\` (${validation.params.visibility}) — you'll get a confirmation comment when the repo is ready.` }); + } catch (err) { + // Mark processed in finally even on error: the alternative is Probot + // retrying after we've already created issues / posted comments, + // which is exactly the duplicate-side-effect bug we're fixing. + getLogger().error({ err, deliveryId }, 'issues.opened handler threw'); + } finally { if (deliveryId) markProcessed(deliveryId); - return; } - - enqueueTask( - 'provision-repo', - `provision-repo:${owner}/${repo}#${issue.number}`, - { - sourceOwner: owner, - sourceRepo: repo, - sourceIssue: issue.number, - params: validation.params - } - ); - - await issueOps.createComment(context.octokit,{ - owner, - repo, - issue_number: issue.number, - body: `✅ Request accepted. Provisioning \`${validation.params.name}\` (${validation.params.visibility}) — you'll get a confirmation comment when the repo is ready.` - }); - - if (deliveryId) markProcessed(deliveryId); }); app.on('issue_comment.created', async (context) => { @@ -509,6 +509,7 @@ function registerApp(app, options = {}) { if (cmd === '/configure-repo') { if (!(await requireOrgMember()) || !(await requireAllowedUser())) { + if (deliveryId) markProcessed(deliveryId); return; } @@ -531,6 +532,7 @@ function registerApp(app, options = {}) { if (cmd === '/sync-all-repos') { if (!(await requireOrgMember()) || !(await requireAllowedUser())) { + if (deliveryId) markProcessed(deliveryId); return; } @@ -551,6 +553,7 @@ function registerApp(app, options = {}) { if (cmd === '/check-config') { if (!(await requireOrgMember()) || !(await requireAllowedUser())) { + if (deliveryId) markProcessed(deliveryId); return; } @@ -567,6 +570,7 @@ function registerApp(app, options = {}) { if (cmd === '/check-dependabot') { if (!(await requireOrgMember()) || !(await requireAllowedUser())) { + if (deliveryId) markProcessed(deliveryId); return; } @@ -583,6 +587,7 @@ function registerApp(app, options = {}) { if (cmd === '/fix-dependabot-labels') { if (!(await requireOrgMember()) || !(await requireAllowedUser())) { + if (deliveryId) markProcessed(deliveryId); return; } @@ -615,6 +620,7 @@ function registerApp(app, options = {}) { if (cmd === '/generate-dependabot') { if (!(await requireOrgMember()) || !(await requireAllowedUser())) { + if (deliveryId) markProcessed(deliveryId); return; } @@ -661,6 +667,7 @@ function registerApp(app, options = {}) { if (cmd === '/analyze-org') { if (!(await requireOrgMember()) || !(await requireAllowedUser())) { + if (deliveryId) markProcessed(deliveryId); return; } @@ -687,6 +694,7 @@ function registerApp(app, options = {}) { if (cmd === '/check-merge-strategy') { if (!(await requireOrgMember()) || !(await requireAllowedUser())) { + if (deliveryId) markProcessed(deliveryId); return; } @@ -738,6 +746,7 @@ function registerApp(app, options = {}) { if (cmd === '/allow-merge-commit') { if (!(await requireOrgMember()) || !(await requireAllowedUser())) { + if (deliveryId) markProcessed(deliveryId); return; } @@ -749,6 +758,7 @@ function registerApp(app, options = {}) { issue_number: issueNumber, body: '❌ You are not authorized to run this command.' }); + if (deliveryId) markProcessed(deliveryId); return; } @@ -792,6 +802,7 @@ function registerApp(app, options = {}) { if (cmd === '/review-pr') { if (!(await requireOrgMember()) || !(await requireAllowedUser())) { + if (deliveryId) markProcessed(deliveryId); return; } @@ -803,6 +814,7 @@ function registerApp(app, options = {}) { issue_number: issueNumber, body: '❌ `/review-pr` can only be used on pull requests, not issues.' }); + if (deliveryId) markProcessed(deliveryId); return; }