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