From f69ad7a6486aa7e3ff89113a4c24eb968e514e02 Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Sun, 22 Feb 2026 12:44:24 +0000 Subject: [PATCH] fix: pass preSeededCommentId in llmist backend + strengthen checklist prompt The llmist backend path in executeAgent() was missing the preSeededCommentId parameter when creating ProgressMonitor, causing the ack comment posted by the router to be orphaned while ProgressMonitor created a new comment. The claude-code backend path already passed this correctly. Also strengthens the respond-to-planning-comment prompt with explicit instructions for checklist management (add/rename/delete) so agents use DeleteChecklistItem when narrowing scope instead of creating duplicate checklists. Additionally carries ackCommentId through the in-worker webhook queue so queued webhooks don't lose their ack comment reference. Co-Authored-By: Claude Opus 4.6 --- src/agents/base.ts | 1 + .../templates/respond-to-planning-comment.eta | 11 +++ src/triggers/github/webhook-handler.ts | 11 ++- src/triggers/jira/webhook-handler.ts | 8 +- src/triggers/shared/webhook-queue.ts | 8 +- src/triggers/trello/webhook-handler.ts | 12 ++- src/utils/webhookQueue.ts | 8 +- tests/unit/triggers/webhook-queue.test.ts | 84 +++++++++++++++++++ tests/unit/utils/webhookQueue.test.ts | 35 ++++++++ 9 files changed, 166 insertions(+), 12 deletions(-) create mode 100644 tests/unit/triggers/webhook-queue.test.ts 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();