Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/agents/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
11 changes: 11 additions & 0 deletions src/agents/prompts/templates/respond-to-planning-comment.eta
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
11 changes: 8 additions & 3 deletions src/triggers/github/webhook-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
);
Expand All @@ -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(),
Expand Down
8 changes: 6 additions & 2 deletions src/triggers/jira/webhook-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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 {
Expand Down
8 changes: 6 additions & 2 deletions src/triggers/shared/webhook-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>,
processWebhook: (
payload: unknown,
eventType?: string,
ackCommentId?: string | number,
) => Promise<void>,
label: string,
getEventType?: (entry: { payload: unknown; eventType?: string }) => string | undefined,
): void {
Expand All @@ -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) });
});
});
Expand Down
12 changes: 8 additions & 4 deletions src/triggers/trello/webhook-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -161,7 +165,7 @@ export async function processTrelloWebhook(
return;
}

if (tryQueueWebhook(payload)) {
if (tryQueueWebhook(payload, ackCommentId)) {
return;
}

Expand Down
8 changes: 7 additions & 1 deletion src/utils/webhookQueue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -22,6 +27,7 @@ export function enqueueWebhook(payload: unknown, eventType?: string): boolean {
queue.push({
payload,
eventType,
ackCommentId,
receivedAt: new Date(),
});

Expand Down
84 changes: 84 additions & 0 deletions tests/unit/triggers/webhook-queue.test.ts
Original file line number Diff line number Diff line change
@@ -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');
});
});
35 changes: 35 additions & 0 deletions tests/unit/utils/webhookQueue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down