Skip to content

fix: markProcessed on chatops auth-fail / exception paths (Bug #6) #51

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
avrabe wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: markProcessed on chatops auth-fail / exception paths (Bug #6) #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions __tests__/integration/app.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});

// =========================================================================
Expand Down Expand Up @@ -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 });
Expand Down Expand Up @@ -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 () => {
Expand Down
152 changes: 82 additions & 70 deletions src/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:<command>` 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:<command>` 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) => {
Expand Down Expand Up @@ -509,6 +509,7 @@ function registerApp(app, options = {}) {

if (cmd === '/configure-repo') {
if (!(await requireOrgMember()) || !(await requireAllowedUser())) {
if (deliveryId) markProcessed(deliveryId);
return;
}

Expand All @@ -531,6 +532,7 @@ function registerApp(app, options = {}) {

if (cmd === '/sync-all-repos') {
if (!(await requireOrgMember()) || !(await requireAllowedUser())) {
if (deliveryId) markProcessed(deliveryId);
return;
}

Expand All @@ -551,6 +553,7 @@ function registerApp(app, options = {}) {

if (cmd === '/check-config') {
if (!(await requireOrgMember()) || !(await requireAllowedUser())) {
if (deliveryId) markProcessed(deliveryId);
return;
}

Expand All @@ -567,6 +570,7 @@ function registerApp(app, options = {}) {

if (cmd === '/check-dependabot') {
if (!(await requireOrgMember()) || !(await requireAllowedUser())) {
if (deliveryId) markProcessed(deliveryId);
return;
}

Expand All @@ -583,6 +587,7 @@ function registerApp(app, options = {}) {

if (cmd === '/fix-dependabot-labels') {
if (!(await requireOrgMember()) || !(await requireAllowedUser())) {
if (deliveryId) markProcessed(deliveryId);
return;
}

Expand Down Expand Up @@ -615,6 +620,7 @@ function registerApp(app, options = {}) {

if (cmd === '/generate-dependabot') {
if (!(await requireOrgMember()) || !(await requireAllowedUser())) {
if (deliveryId) markProcessed(deliveryId);
return;
}

Expand Down Expand Up @@ -661,6 +667,7 @@ function registerApp(app, options = {}) {

if (cmd === '/analyze-org') {
if (!(await requireOrgMember()) || !(await requireAllowedUser())) {
if (deliveryId) markProcessed(deliveryId);
return;
}

Expand All @@ -687,6 +694,7 @@ function registerApp(app, options = {}) {

if (cmd === '/check-merge-strategy') {
if (!(await requireOrgMember()) || !(await requireAllowedUser())) {
if (deliveryId) markProcessed(deliveryId);
return;
}

Expand Down Expand Up @@ -738,6 +746,7 @@ function registerApp(app, options = {}) {

if (cmd === '/allow-merge-commit') {
if (!(await requireOrgMember()) || !(await requireAllowedUser())) {
if (deliveryId) markProcessed(deliveryId);
return;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -792,6 +802,7 @@ function registerApp(app, options = {}) {

if (cmd === '/review-pr') {
if (!(await requireOrgMember()) || !(await requireAllowedUser())) {
if (deliveryId) markProcessed(deliveryId);
return;
}

Expand All @@ -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;
}

Expand Down
Loading