From c8de317bdab417901e10f56aff8362f81cad531b Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Sun, 22 Feb 2026 13:18:41 +0000 Subject: [PATCH] fix(jira): fall back to transition when subtask delete returns 403 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `deleteIssue()` fails with 403 (missing "Delete Issues" permission), fall back to transitioning the subtask to a terminal status (Cancelled, Won't Do, Rejected, Closed, or Done — in priority order). Also refactor webhook handlers in router/index.ts to extract helper functions, resolving all 3 pre-existing cognitive complexity warnings. Co-Authored-By: Claude Opus 4.6 --- src/pm/jira/adapter.ts | 31 ++- src/router/index.ts | 321 ++++++++++++++++------------- tests/unit/pm/jira/adapter.test.ts | 48 +++++ 3 files changed, 251 insertions(+), 149 deletions(-) diff --git a/src/pm/jira/adapter.ts b/src/pm/jira/adapter.ts index 6b1751b6..3bb983a4 100644 --- a/src/pm/jira/adapter.ts +++ b/src/pm/jira/adapter.ts @@ -279,7 +279,36 @@ export class JiraPMProvider implements PMProvider { async deleteChecklistItem(_workItemId: string, checkItemId: string): Promise { // checkItemId is a JIRA issue key (subtask) - await jiraClient.deleteIssue(checkItemId); + try { + await jiraClient.deleteIssue(checkItemId); + } catch (error) { + const is403 = + error instanceof Error && + (error.message.includes('403') || error.message.includes('Forbidden')); + if (!is403) throw error; + + // Deletion not permitted — transition to a terminal status instead + logger.info('Delete not permitted, transitioning subtask to terminal status', { + issueKey: checkItemId, + }); + const transitions = await jiraClient.getTransitions(checkItemId); + const terminalNames = ['cancelled', "won't do", 'rejected', 'closed', 'done']; + let match: JiraTransition | undefined; + for (const name of terminalNames) { + match = transitions.find((t: JiraTransition) => { + const toName = (t.to?.name ?? '').toLowerCase(); + const tName = (t.name ?? '').toLowerCase(); + return toName === name || tName === name; + }); + if (match) break; + } + if (!match?.id) { + throw new Error( + `Cannot delete subtask ${checkItemId}: deletion returned 403 and no terminal transition found (available: ${transitions.map((t: JiraTransition) => t.name).join(', ')})`, + ); + } + await jiraClient.transitionIssue(checkItemId, match.id); + } } async getAttachments(workItemId: string): Promise { diff --git a/src/router/index.ts b/src/router/index.ts index 26a2a3ee..e9f8d0fc 100644 --- a/src/router/index.ts +++ b/src/router/index.ts @@ -183,6 +183,66 @@ async function tryPostTrelloAck( return commentId ?? undefined; } +async function isSelfAuthoredTrelloComment(payload: unknown, projectId: string): Promise { + const action = (payload as Record).action as Record | undefined; + const commentAuthorId = action?.idMemberCreator as string | undefined; + if (!commentAuthorId) return false; + try { + const botId = await resolveTrelloBotMemberId(projectId); + return !!botId && commentAuthorId === botId; + } catch { + return false; // Identity resolution failed — proceed normally + } +} + +async function processTrelloWebhookEvent( + project: RouterProjectConfig, + cardId: string, + actionType: string, + payload: unknown, +): Promise { + if (actionType === 'commentCard' && (await isSelfAuthoredTrelloComment(payload, project.id))) { + console.log('[Router] Ignoring self-authored Trello comment'); + return; + } + + console.log('[Router] Queueing Trello job:', { actionType, cardId, projectId: project.id }); + + // Fire-and-forget acknowledgment reaction — only for comment actions + if (actionType === 'commentCard') { + void sendAcknowledgeReaction('trello', project.id, payload).catch((err) => + console.error('[Router] Trello reaction error:', err), + ); + } + + // Try to post an ack comment via trigger matching (non-blocking best-effort) + let ackCommentId: string | undefined; + try { + ackCommentId = await tryPostTrelloAck(project.id, cardId, payload); + } catch (err) { + console.warn('[Router] Trello ack comment failed (non-fatal):', String(err)); + } + + const job: CascadeJob = { + type: 'trello', + source: 'trello', + payload, + projectId: project.id, + cardId, + actionType: actionType || 'unknown', + receivedAt: new Date().toISOString(), + ackCommentId, + }; + + try { + const jobId = await addJob(job); + console.log('[Router] Trello job queued:', { jobId, actionType, ackCommentId }); + } catch (err) { + console.error('[Router] Failed to queue Trello job:', err); + // Still return to caller — Trello gets 200 to avoid retries + } +} + /** * Try to match a trigger and post an ack comment for a GitHub webhook. * Returns the ack comment ID if posted, undefined otherwise. @@ -226,6 +286,94 @@ async function tryPostGitHubAck( return commentId ?? undefined; } +async function isSelfAuthoredGitHubComment( + payload: unknown, + repoFullName: string, +): Promise { + const p = payload as Record; + const commentUser = (p.comment as Record | undefined)?.user as + | Record + | undefined; + const login = commentUser?.login as string | undefined; + if (!login) return false; + try { + const project = await findProjectByRepo(repoFullName); + if (!project) return false; + const personas = await resolvePersonaIdentities(project.id); + return isCascadeBot(login, personas); + } catch { + return false; // Persona resolution failed — proceed normally + } +} + +function fireGitHubAckReaction(repoFullName: string, payload: unknown): void { + void (async () => { + try { + const project = await findProjectByRepo(repoFullName); + if (!project) { + console.warn('[Router] No project found for repo, skipping GitHub reaction', { + repoFullName, + }); + return; + } + const personaIdentities = await resolvePersonaIdentities(project.id); + await sendAcknowledgeReaction('github', repoFullName, payload, personaIdentities, project); + } catch (err) { + console.warn('[Router] GitHub reaction error:', String(err)); + } + })(); +} + +async function processGitHubWebhookEvent( + eventType: string, + repoFullName: string, + payload: unknown, +): Promise { + const isCommentEvent = + eventType === 'issue_comment' || eventType === 'pull_request_review_comment'; + + if (isCommentEvent && (await isSelfAuthoredGitHubComment(payload, repoFullName))) { + console.log('[Router] Ignoring self-authored GitHub comment'); + return; + } + + console.log('[Router] Queueing GitHub job:', { eventType, repoFullName }); + + // Fire-and-forget acknowledgment reaction — only for comment events that @mention the bot + if (isCommentEvent) { + fireGitHubAckReaction(repoFullName, payload); + } + + // Try to post an ack comment via trigger matching (non-blocking best-effort) + let ackCommentId: number | undefined; + try { + ackCommentId = await tryPostGitHubAck(eventType, repoFullName, payload); + } catch (err) { + console.warn('[Router] GitHub ack comment failed (non-fatal):', String(err)); + } + + const job: CascadeJob = { + type: 'github', + source: 'github', + payload, + eventType, + repoFullName, + receivedAt: new Date().toISOString(), + ackCommentId, + }; + + // Fire pre-actions (non-blocking) before queueing + const p = payload as Record; + firePreActions(job as GitHubJob, p); + + try { + const jobId = await addJob(job); + console.log('[Router] GitHub job queued:', { jobId, eventType, ackCommentId }); + } catch (err) { + console.error('[Router] Failed to queue GitHub job:', err); + } +} + /** * Try to match a trigger and post an ack comment for a JIRA webhook. * Returns the ack comment ID if posted, undefined otherwise. @@ -250,6 +398,25 @@ async function tryPostJiraAck( return commentId ?? undefined; } +async function isSelfAuthoredJiraComment( + webhookEvent: string, + payload: unknown, + projectId: string, +): Promise { + if (!webhookEvent.startsWith('comment_')) return false; + const p = payload as Record; + const comment = p.comment as Record | undefined; + const author = comment?.author as Record | undefined; + const commentAuthorId = author?.accountId as string | undefined; + if (!commentAuthorId) return false; + try { + const botId = await resolveJiraBotAccountId(projectId); + return !!botId && commentAuthorId === botId; + } catch { + return false; // Identity resolution failed — proceed normally + } +} + /** * Fire non-blocking pre-actions for a GitHub job before it is queued. * Currently adds a 👀 reaction for first-time check_suite success events. @@ -364,60 +531,7 @@ app.post('/trello/webhook', async (c) => { }); if (shouldProcess && project && cardId) { - // Skip self-authored Trello comments to prevent infinite loops - if (actionType === 'commentCard') { - const action = (payload as Record).action as - | Record - | undefined; - const commentAuthorId = action?.idMemberCreator as string | undefined; - if (commentAuthorId) { - try { - const botId = await resolveTrelloBotMemberId(project.id); - if (botId && commentAuthorId === botId) { - console.log('[Router] Ignoring self-authored Trello comment'); - return c.text('OK', 200); - } - } catch { - // Identity resolution failed — proceed normally - } - } - } - - console.log('[Router] Queueing Trello job:', { actionType, cardId, projectId: project.id }); - - // Fire-and-forget acknowledgment reaction — only for comment actions - if (actionType === 'commentCard') { - void sendAcknowledgeReaction('trello', project.id, payload).catch((err) => - console.error('[Router] Trello reaction error:', err), - ); - } - - // Try to post an ack comment via trigger matching (non-blocking best-effort) - let ackCommentId: string | undefined; - try { - ackCommentId = await tryPostTrelloAck(project.id, cardId, payload); - } catch (err) { - console.warn('[Router] Trello ack comment failed (non-fatal):', String(err)); - } - - const job: CascadeJob = { - type: 'trello', - source: 'trello', - payload, - projectId: project.id, - cardId, - actionType: actionType || 'unknown', - receivedAt: new Date().toISOString(), - ackCommentId, - }; - - try { - const jobId = await addJob(job); - console.log('[Router] Trello job queued:', { jobId, actionType, ackCommentId }); - } catch (err) { - console.error('[Router] Failed to queue Trello job:', err); - // Still return 200 to Trello to avoid retries - } + await processTrelloWebhookEvent(project, cardId, actionType || 'unknown', payload); } else { console.log(`[Router] Ignoring Trello: ${actionType || 'unknown'}`); } @@ -507,82 +621,7 @@ app.post('/github/webhook', async (c) => { }); if (shouldProcess) { - // Skip self-authored GitHub comments to prevent infinite loops - if (eventType === 'issue_comment' || eventType === 'pull_request_review_comment') { - const commentLogin = (p.comment as Record | undefined)?.user as - | Record - | undefined; - const login = commentLogin?.login as string | undefined; - if (login) { - try { - const project = await findProjectByRepo(repoFullName); - if (project) { - const personas = await resolvePersonaIdentities(project.id); - if (isCascadeBot(login, personas)) { - console.log('[Router] Ignoring self-authored GitHub comment'); - return c.text('OK', 200); - } - } - } catch { - // Persona resolution failed — proceed normally - } - } - } - - console.log('[Router] Queueing GitHub job:', { eventType, repoFullName }); - - // Fire-and-forget acknowledgment reaction — only for comment events that @mention the bot - if (eventType === 'issue_comment' || eventType === 'pull_request_review_comment') { - void (async () => { - try { - const project = await findProjectByRepo(repoFullName); - if (!project) { - console.warn('[Router] No project found for repo, skipping GitHub reaction', { - repoFullName, - }); - return; - } - const personaIdentities = await resolvePersonaIdentities(project.id); - await sendAcknowledgeReaction( - 'github', - repoFullName, - payload, - personaIdentities, - project, - ); - } catch (err) { - console.warn('[Router] GitHub reaction error:', String(err)); - } - })(); - } - - // Try to post an ack comment via trigger matching (non-blocking best-effort) - let ackCommentId: number | undefined; - try { - ackCommentId = await tryPostGitHubAck(eventType, repoFullName, payload); - } catch (err) { - console.warn('[Router] GitHub ack comment failed (non-fatal):', String(err)); - } - - const job: CascadeJob = { - type: 'github', - source: 'github', - payload, - eventType, - repoFullName, - receivedAt: new Date().toISOString(), - ackCommentId, - }; - - // Fire pre-actions (non-blocking) before queueing - firePreActions(job as GitHubJob, p); - - try { - const jobId = await addJob(job); - console.log('[Router] GitHub job queued:', { jobId, eventType, ackCommentId }); - } catch (err) { - console.error('[Router] Failed to queue GitHub job:', err); - } + await processGitHubWebhookEvent(eventType, repoFullName, payload); } else { console.log('[Router] Ignoring GitHub event:', eventType); } @@ -651,25 +690,11 @@ app.post('/jira/webhook', async (c) => { }); if (shouldProcess && project) { - // Skip self-authored JIRA comments to prevent infinite loops - if (webhookEvent.startsWith('comment_')) { - const comment = p.comment as Record | undefined; - const author = comment?.author as Record | undefined; - const commentAuthorId = author?.accountId as string | undefined; - if (commentAuthorId) { - try { - const botId = await resolveJiraBotAccountId(project.id); - if (botId && commentAuthorId === botId) { - console.log('[Router] Ignoring self-authored JIRA comment'); - return c.text('OK', 200); - } - } catch { - // Identity resolution failed — proceed normally - } - } + if (await isSelfAuthoredJiraComment(webhookEvent, payload, project.id)) { + console.log('[Router] Ignoring self-authored JIRA comment'); + } else { + await queueJiraJob(project, issueKey, webhookEvent, payload, config.fullProjects); } - - await queueJiraJob(project, issueKey, webhookEvent, payload, config.fullProjects); } else { console.log(`[Router] Ignoring JIRA: ${webhookEvent}`); } diff --git a/tests/unit/pm/jira/adapter.test.ts b/tests/unit/pm/jira/adapter.test.ts index 4f4ad511..d6a93135 100644 --- a/tests/unit/pm/jira/adapter.test.ts +++ b/tests/unit/pm/jira/adapter.test.ts @@ -580,6 +580,54 @@ describe('JiraPMProvider', () => { expect(mockJiraClient.deleteIssue).toHaveBeenCalledWith('PROJ-5'); }); + + it('falls back to transition when deleteIssue returns 403', async () => { + mockJiraClient.deleteIssue.mockRejectedValue(new Error('Request failed with status 403')); + mockJiraClient.getTransitions.mockResolvedValue([ + { id: 't-1', name: 'In Progress', to: { name: 'In Progress' } }, + { id: 't-2', name: 'Cancelled', to: { name: 'Cancelled' } }, + ]); + mockJiraClient.transitionIssue.mockResolvedValue(undefined); + + await provider.deleteChecklistItem('PROJ-1', 'PROJ-5'); + + expect(mockJiraClient.transitionIssue).toHaveBeenCalledWith('PROJ-5', 't-2'); + }); + + it('tries terminal statuses in priority order (cancelled preferred over done)', async () => { + mockJiraClient.deleteIssue.mockRejectedValue(new Error('403 Forbidden')); + mockJiraClient.getTransitions.mockResolvedValue([ + { id: 't-done', name: 'Done', to: { name: 'Done' } }, + { id: 't-cancel', name: 'Cancel', to: { name: 'Cancelled' } }, + ]); + mockJiraClient.transitionIssue.mockResolvedValue(undefined); + + await provider.deleteChecklistItem('PROJ-1', 'PROJ-5'); + + // Should pick "Cancelled" (higher priority) over "Done" + expect(mockJiraClient.transitionIssue).toHaveBeenCalledWith('PROJ-5', 't-cancel'); + }); + + it('throws when no terminal transition available after 403', async () => { + mockJiraClient.deleteIssue.mockRejectedValue(new Error('403 Forbidden')); + mockJiraClient.getTransitions.mockResolvedValue([ + { id: 't-1', name: 'In Progress', to: { name: 'In Progress' } }, + { id: 't-2', name: 'In Review', to: { name: 'In Review' } }, + ]); + + await expect(provider.deleteChecklistItem('PROJ-1', 'PROJ-5')).rejects.toThrow( + 'Cannot delete subtask PROJ-5: deletion returned 403 and no terminal transition found', + ); + }); + + it('re-throws non-403 errors without fallback', async () => { + mockJiraClient.deleteIssue.mockRejectedValue(new Error('500 Internal Server Error')); + + await expect(provider.deleteChecklistItem('PROJ-1', 'PROJ-5')).rejects.toThrow( + '500 Internal Server Error', + ); + expect(mockJiraClient.getTransitions).not.toHaveBeenCalled(); + }); }); describe('getAttachments', () => {