diff --git a/src/agents/base.ts b/src/agents/base.ts index 0c38e123..f705f7c5 100644 --- a/src/agents/base.ts +++ b/src/agents/base.ts @@ -434,6 +434,7 @@ export async function executeAgent( customModels: CUSTOM_MODELS as ModelSpec[], repoDir, trello: cardId ? { cardId } : undefined, + preSeededCommentId: input.ackCommentId as string | undefined, }), interactive, diff --git a/src/agents/prompts/templates/respond-to-planning-comment.eta b/src/agents/prompts/templates/respond-to-planning-comment.eta index 50202c89..e5473053 100644 --- a/src/agents/prompts/templates/respond-to-planning-comment.eta +++ b/src/agents/prompts/templates/respond-to-planning-comment.eta @@ -53,6 +53,17 @@ You are running in a cloned copy of the project repository. Before updating the 4. **Make surgical updates** to the <%= it.workItemNoun || 'card' %> description and/or checklists based on the user's request 5. **Post a reply comment** via PostComment explaining what you changed and why +## Updating the Plan and Checklists + +When modifying the plan, **update the existing checklists in place** — do NOT create duplicate checklists. + +- **Adding steps**: Use `AddChecklist` only when there is no existing checklist to add items to. Otherwise use `UpdateChecklistItem` to rename existing items or add to an existing checklist. +- **Renaming/rewriting steps**: Use `UpdateChecklistItem` to change the text of existing checklist items. +- **Removing steps**: Use `DeleteChecklistItem` to permanently remove checklist items / subtasks that are no longer needed. Do NOT mark removed items as "complete" — they were never done, so deleting is the correct action. +- **Reordering**: Delete and re-add items as needed to achieve the desired order. + +When the user asks to narrow scope, focus on a subset, or drop items from the plan, **always delete** the out-of-scope items rather than leaving them in the checklist. + ## Response Format When updating the <%= it.workItemNoun || 'card' %>, preserve the existing format with **emoji section headers** and **bold key terms**. Only modify the sections that need to change. diff --git a/src/triggers/github/webhook-handler.ts b/src/triggers/github/webhook-handler.ts index c540d5f8..7b28e6ce 100644 --- a/src/triggers/github/webhook-handler.ts +++ b/src/triggers/github/webhook-handler.ts @@ -156,8 +156,13 @@ async function runGitHubAgentJob( function processNextQueuedGitHubWebhook(registry: TriggerRegistry): void { processNextQueuedWebhook( - (payload, eventType) => - processGitHubWebhook(payload, eventType ?? 'pull_request_review_comment', registry), + (payload, eventType, ackCommentId) => + processGitHubWebhook( + payload, + eventType ?? 'pull_request_review_comment', + registry, + ackCommentId as number | undefined, + ), 'GitHub', (entry) => entry.eventType ?? 'pull_request_review_comment', ); @@ -181,7 +186,7 @@ export async function processGitHubWebhook( } if (isCurrentlyProcessing()) { - const queued = enqueueWebhook(payload, eventType); + const queued = enqueueWebhook(payload, eventType, ackCommentId); if (queued) { logger.info('Currently processing, GitHub webhook queued', { queueLength: getQueueLength(), diff --git a/src/triggers/jira/webhook-handler.ts b/src/triggers/jira/webhook-handler.ts index 6609baea..44dc8b9a 100644 --- a/src/triggers/jira/webhook-handler.ts +++ b/src/triggers/jira/webhook-handler.ts @@ -155,7 +155,11 @@ async function cleanupOrphanJiraAck( } function processNextQueuedJiraWebhook(registry: TriggerRegistry): void { - processNextQueuedWebhook((payload) => processJiraWebhook(payload, registry), 'JIRA'); + processNextQueuedWebhook( + (payload, _eventType, ackCommentId) => + processJiraWebhook(payload, registry, ackCommentId as string | undefined), + 'JIRA', + ); } export async function processJiraWebhook( @@ -173,7 +177,7 @@ export async function processJiraWebhook( } if (isCurrentlyProcessing()) { - const queued = enqueueWebhook(payload); + const queued = enqueueWebhook(payload, undefined, ackCommentId); if (queued) { logger.info('Currently processing, JIRA webhook queued', { queueLength: getQueueLength() }); } else { diff --git a/src/triggers/shared/webhook-queue.ts b/src/triggers/shared/webhook-queue.ts index 5b90f36f..48892b9f 100644 --- a/src/triggers/shared/webhook-queue.ts +++ b/src/triggers/shared/webhook-queue.ts @@ -9,7 +9,11 @@ import { logger } from '../../utils/logging.js'; * @param getEventType - Optional function to extract event type from the queued entry. */ export function processNextQueuedWebhook( - processWebhook: (payload: unknown, eventType?: string) => Promise, + processWebhook: ( + payload: unknown, + eventType?: string, + ackCommentId?: string | number, + ) => Promise, label: string, getEventType?: (entry: { payload: unknown; eventType?: string }) => string | undefined, ): void { @@ -20,7 +24,7 @@ export function processNextQueuedWebhook( if (eventType) logContext.eventType = eventType; logger.info(`Processing queued ${label} webhook`, logContext); setImmediate(() => { - processWebhook(next.payload, eventType).catch((err) => { + processWebhook(next.payload, eventType, next.ackCommentId).catch((err) => { logger.error(`Failed to process queued ${label} webhook`, { error: String(err) }); }); }); diff --git a/src/triggers/trello/webhook-handler.ts b/src/triggers/trello/webhook-handler.ts index 56fafded..25f6f8bc 100644 --- a/src/triggers/trello/webhook-handler.ts +++ b/src/triggers/trello/webhook-handler.ts @@ -61,13 +61,17 @@ async function executeAgent( // ============================================================================ function processNextQueued(registry: TriggerRegistry): void { - processNextQueuedWebhook((payload) => processTrelloWebhook(payload, registry), 'Trello'); + processNextQueuedWebhook( + (payload, _eventType, ackCommentId) => + processTrelloWebhook(payload, registry, ackCommentId as string | undefined), + 'Trello', + ); } -function tryQueueWebhook(payload: TrelloWebhookPayload): boolean { +function tryQueueWebhook(payload: TrelloWebhookPayload, ackCommentId?: string): boolean { if (!isCurrentlyProcessing()) return false; - const queued = enqueueWebhook(payload); + const queued = enqueueWebhook(payload, undefined, ackCommentId); if (queued) { logger.info('Currently processing, webhook queued', { queueLength: getQueueLength() }); } else { @@ -161,7 +165,7 @@ export async function processTrelloWebhook( return; } - if (tryQueueWebhook(payload)) { + if (tryQueueWebhook(payload, ackCommentId)) { return; } diff --git a/src/utils/webhookQueue.ts b/src/utils/webhookQueue.ts index 9f2b546a..3edda042 100644 --- a/src/utils/webhookQueue.ts +++ b/src/utils/webhookQueue.ts @@ -5,12 +5,17 @@ const MAX_QUEUE_SIZE = 10; interface QueuedWebhook { payload: unknown; eventType?: string; // Optional for backward compatibility (Trello doesn't need it) + ackCommentId?: string | number; receivedAt: Date; } const queue: QueuedWebhook[] = []; -export function enqueueWebhook(payload: unknown, eventType?: string): boolean { +export function enqueueWebhook( + payload: unknown, + eventType?: string, + ackCommentId?: string | number, +): boolean { if (queue.length >= MAX_QUEUE_SIZE) { logger.warn('Webhook queue full, rejecting', { queueLength: queue.length, @@ -22,6 +27,7 @@ export function enqueueWebhook(payload: unknown, eventType?: string): boolean { queue.push({ payload, eventType, + ackCommentId, receivedAt: new Date(), }); diff --git a/tests/unit/triggers/webhook-queue.test.ts b/tests/unit/triggers/webhook-queue.test.ts new file mode 100644 index 00000000..534e9c19 --- /dev/null +++ b/tests/unit/triggers/webhook-queue.test.ts @@ -0,0 +1,84 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { processNextQueuedWebhook } from '../../../src/triggers/shared/webhook-queue.js'; +import { clearQueue, enqueueWebhook } from '../../../src/utils/webhookQueue.js'; + +describe('processNextQueuedWebhook', () => { + afterEach(() => { + clearQueue(); + }); + + it('does nothing when queue is empty', () => { + const processWebhook = vi.fn().mockResolvedValue(undefined); + + processNextQueuedWebhook(processWebhook, 'Test'); + + expect(processWebhook).not.toHaveBeenCalled(); + }); + + it('forwards payload and eventType to processWebhook', async () => { + const processWebhook = vi.fn().mockResolvedValue(undefined); + enqueueWebhook({ action: 'test' }, 'issue_comment'); + + processNextQueuedWebhook(processWebhook, 'Test'); + + // processWebhook is called via setImmediate — wait for it + await new Promise((resolve) => setImmediate(resolve)); + + expect(processWebhook).toHaveBeenCalledWith( + { action: 'test' }, + undefined, // eventType comes from getEventType, not the queued entry + undefined, // no ackCommentId + ); + }); + + it('uses getEventType to extract event type from queued entry', async () => { + const processWebhook = vi.fn().mockResolvedValue(undefined); + enqueueWebhook({ action: 'test' }, 'pull_request'); + + processNextQueuedWebhook(processWebhook, 'Test', (entry) => entry.eventType); + + await new Promise((resolve) => setImmediate(resolve)); + + expect(processWebhook).toHaveBeenCalledWith({ action: 'test' }, 'pull_request', undefined); + }); + + it('forwards ackCommentId through the queue', async () => { + const processWebhook = vi.fn().mockResolvedValue(undefined); + enqueueWebhook({ action: 'test' }, 'issue_comment', 'ack-123'); + + processNextQueuedWebhook(processWebhook, 'Test', (entry) => entry.eventType); + + await new Promise((resolve) => setImmediate(resolve)); + + expect(processWebhook).toHaveBeenCalledWith({ action: 'test' }, 'issue_comment', 'ack-123'); + }); + + it('forwards numeric ackCommentId through the queue', async () => { + const processWebhook = vi.fn().mockResolvedValue(undefined); + enqueueWebhook({ action: 'test' }, undefined, 10646); + + processNextQueuedWebhook(processWebhook, 'Test'); + + await new Promise((resolve) => setImmediate(resolve)); + + expect(processWebhook).toHaveBeenCalledWith({ action: 'test' }, undefined, 10646); + }); + + it('processes items in FIFO order preserving ackCommentId', async () => { + const processWebhook = vi.fn().mockResolvedValue(undefined); + enqueueWebhook({ order: 1 }, undefined, 'first-ack'); + enqueueWebhook({ order: 2 }, undefined, 'second-ack'); + + // Process first item + processNextQueuedWebhook(processWebhook, 'Test'); + await new Promise((resolve) => setImmediate(resolve)); + + expect(processWebhook).toHaveBeenCalledWith({ order: 1 }, undefined, 'first-ack'); + + // Process second item + processNextQueuedWebhook(processWebhook, 'Test'); + await new Promise((resolve) => setImmediate(resolve)); + + expect(processWebhook).toHaveBeenCalledWith({ order: 2 }, undefined, 'second-ack'); + }); +}); diff --git a/tests/unit/utils/webhookQueue.test.ts b/tests/unit/utils/webhookQueue.test.ts index a95072bb..0a830a97 100644 --- a/tests/unit/utils/webhookQueue.test.ts +++ b/tests/unit/utils/webhookQueue.test.ts @@ -128,6 +128,41 @@ describe('webhookQueue', () => { }); }); + describe('ackCommentId', () => { + it('preserves string ackCommentId through enqueue/dequeue', () => { + enqueueWebhook({ action: 'test' }, undefined, 'comment-42'); + + const item = dequeueWebhook(); + + expect(item?.ackCommentId).toBe('comment-42'); + }); + + it('preserves numeric ackCommentId through enqueue/dequeue', () => { + enqueueWebhook({ action: 'test' }, undefined, 10646); + + const item = dequeueWebhook(); + + expect(item?.ackCommentId).toBe(10646); + }); + + it('defaults ackCommentId to undefined when not provided', () => { + enqueueWebhook({ action: 'test' }); + + const item = dequeueWebhook(); + + expect(item?.ackCommentId).toBeUndefined(); + }); + + it('preserves ackCommentId alongside eventType', () => { + enqueueWebhook({ action: 'test' }, 'issue_comment', 'ack-99'); + + const item = dequeueWebhook(); + + expect(item?.eventType).toBe('issue_comment'); + expect(item?.ackCommentId).toBe('ack-99'); + }); + }); + describe('getMaxQueueSize', () => { it('returns the maximum queue size', () => { const maxSize = getMaxQueueSize();