From cb9a97e42f4da83d319b376147aefd4fc114e132 Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Mon, 23 Feb 2026 14:37:28 +0000 Subject: [PATCH] refactor: code review cleanups for trigger system and router MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace 35+ raw console.log/warn/error calls with structured logger across router handlers (github, trello, jira, index, queue) - Create shared JiraWebhookPayload type and STATUS_TO_AGENT constant in src/triggers/jira/types.ts to eliminate duplication across 3 files - Fix Trello card-moved and label-added triggers to return null instead of throwing on missing card ID (consistency with other handlers) - Fix fragile isAgentLogFilename regex that couldn't handle multi-hyphen agent names like respond-to-review - Extract shared withPMCredentials helper to src/pm/context.ts, eliminating duplication between router and worker - Remove dead fallback job queueing for unresolvable GitHub projects - Rename queueJiraJob → processJiraWebhookEvent for naming consistency - Add Sentry captureException for persistent ack reaction failures - Add ackCommentId type-clarifying comments to job interfaces - Refactor processGitHubWebhook to fix cognitive complexity lint warning (extract tryEnqueueIfBusy and resolveTriggerResult helpers) - Add test coverage for unknown agent types, empty trigger config objects, multi-hyphen filenames, and missing card ID edge cases Co-Authored-By: Claude Opus 4.6 --- src/pm/context.ts | 20 ++ src/pm/webhook-handler.ts | 56 ++++- src/router/github.ts | 189 +++++++++------ src/router/index.ts | 9 +- src/router/jira.ts | 114 +++++---- src/router/queue.ts | 14 +- src/router/trello.ts | 115 +++++---- src/triggers/github/check-suite-failure.ts | 4 - src/triggers/github/check-suite-success.ts | 31 +-- src/triggers/github/pr-comment-mention.ts | 4 - src/triggers/github/pr-merged.ts | 4 - src/triggers/github/pr-opened.ts | 4 - src/triggers/github/pr-ready-to-merge.ts | 4 - src/triggers/github/pr-review-submitted.ts | 4 - src/triggers/github/review-requested.ts | 4 - src/triggers/github/webhook-handler.ts | 152 ++++++++---- src/triggers/jira/comment-mention.ts | 22 +- src/triggers/jira/issue-transitioned.ts | 52 +---- src/triggers/jira/label-added.ts | 51 +--- src/triggers/jira/types.ts | 51 ++++ src/triggers/jira/webhook-handler.ts | 4 +- src/triggers/registry.ts | 14 -- src/triggers/trello/card-moved.ts | 10 +- src/triggers/trello/comment-mention.ts | 4 - src/triggers/trello/label-added.ts | 8 +- src/triggers/trello/webhook-handler.ts | 4 +- src/types/index.ts | 9 +- src/worker-entry.ts | 22 +- tests/unit/config/triggerConfig.test.ts | 30 +++ tests/unit/router/github.test.ts | 182 ++++++++------- tests/unit/router/jira.test.ts | 85 +++++-- tests/unit/router/trello.test.ts | 65 +++++- tests/unit/triggers/card-moved.test.ts | 55 +++-- .../unit/triggers/check-suite-failure.test.ts | 11 - .../unit/triggers/check-suite-success.test.ts | 219 +----------------- .../github-pr-comment-mention.test.ts | 7 - .../triggers/jira-comment-mention.test.ts | 7 - .../triggers/jira-issue-transitioned.test.ts | 49 ---- tests/unit/triggers/jira-label-added.test.ts | 28 --- tests/unit/triggers/label-added.test.ts | 29 +-- tests/unit/triggers/pr-merged.test.ts | 11 - tests/unit/triggers/pr-opened.test.ts | 11 - tests/unit/triggers/pr-ready-to-merge.test.ts | 11 - .../unit/triggers/pr-review-submitted.test.ts | 11 - tests/unit/triggers/registry.test.ts | 167 +++---------- tests/unit/triggers/review-requested.test.ts | 11 - .../triggers/trello-comment-mention.test.ts | 7 - 47 files changed, 894 insertions(+), 1081 deletions(-) create mode 100644 src/triggers/jira/types.ts diff --git a/src/pm/context.ts b/src/pm/context.ts index ba08502c..02d02504 100644 --- a/src/pm/context.ts +++ b/src/pm/context.ts @@ -7,6 +7,7 @@ */ import { AsyncLocalStorage } from 'node:async_hooks'; +import type { PMIntegration } from './integration.js'; import type { PMProvider } from './types.js'; const pmProviderStore = new AsyncLocalStorage(); @@ -28,3 +29,22 @@ export function getPMProvider(): PMProvider { export function getPMProviderOrNull(): PMProvider | null { return pmProviderStore.getStore() ?? null; } + +/** + * Establish PM credential scope for a project. + * + * Uses the integration's withCredentials() for the correct PM type. + * Falls through to running fn() directly if no PM type is configured + * or the integration is unknown. + */ +export async function withPMCredentials( + projectId: string, + pmType: string | undefined, + getIntegration: (type: string) => PMIntegration | null, + fn: () => Promise, +): Promise { + if (!pmType) return fn(); + const integration = getIntegration(pmType); + if (!integration) return fn(); + return integration.withCredentials(projectId, fn); +} diff --git a/src/pm/webhook-handler.ts b/src/pm/webhook-handler.ts index ce29950b..6426c03d 100644 --- a/src/pm/webhook-handler.ts +++ b/src/pm/webhook-handler.ts @@ -88,14 +88,20 @@ async function cleanupOrphanAck( } } -async function handleMatchedTrigger( +async function resolveTriggerResult( integration: PMIntegration, registry: TriggerRegistry, payload: unknown, project: ProjectConfig, - config: CascadeConfig, - ackCommentId?: string, -): Promise { + ackCommentId: string | undefined, + preResolvedResult: TriggerResult | undefined, +): Promise { + if (preResolvedResult) { + logger.info(`Using pre-resolved trigger result for ${integration.type} webhook`, { + agentType: preResolvedResult.agentType, + }); + return preResolvedResult; + } const ctx: TriggerContext = { project, source: integration.type as TriggerSource, payload }; const result = await registry.dispatch(ctx); if (!result) { @@ -103,8 +109,28 @@ async function handleMatchedTrigger( if (ackCommentId) { await cleanupOrphanAck(integration, project.id, payload, ackCommentId); } - return; } + return result; +} + +async function handleMatchedTrigger( + integration: PMIntegration, + registry: TriggerRegistry, + payload: unknown, + project: ProjectConfig, + config: CascadeConfig, + ackCommentId?: string, + preResolvedResult?: TriggerResult, +): Promise { + const result = await resolveTriggerResult( + integration, + registry, + payload, + project, + ackCommentId, + preResolvedResult, + ); + if (!result) return; // Pass ack comment ID into agent input for ProgressMonitor pre-seeding if (ackCommentId) { @@ -152,7 +178,8 @@ async function handleMatchedTrigger( * * Validates the payload via the integration's `parseWebhookPayload()`, * looks up the project, establishes credential + PM provider scope, - * dispatches to the trigger registry, and runs the matched agent. + * dispatches to the trigger registry (or uses pre-resolved result), + * and runs the matched agent. * * Used by both Trello and JIRA webhook handlers. */ @@ -161,8 +188,11 @@ export async function processPMWebhook( payload: unknown, registry: TriggerRegistry, ackCommentId?: string, + triggerResult?: TriggerResult, ): Promise { - logger.info(`Processing ${integration.type} webhook`); + logger.info(`Processing ${integration.type} webhook`, { + hasTriggerResult: !!triggerResult, + }); const event = integration.parseWebhookPayload(payload); if (!event) { @@ -201,11 +231,19 @@ export async function processPMWebhook( } const { project, config } = projectConfig; - // Establish credential + PM provider scope for trigger dispatch + // Establish credential + PM provider scope for agent execution const pmProvider = pmRegistry.createProvider(project); await integration.withCredentials(project.id, () => withPMProvider(pmProvider, () => - handleMatchedTrigger(integration, registry, payload, project, config, ackCommentId), + handleMatchedTrigger( + integration, + registry, + payload, + project, + config, + ackCommentId, + triggerResult, + ), ), ); } diff --git a/src/router/github.ts b/src/router/github.ts index 5adb4e62..c7c31ff4 100644 --- a/src/router/github.ts +++ b/src/router/github.ts @@ -1,18 +1,24 @@ /** * GitHub webhook handler for the router (multi-container) deployment mode. * - * Handles webhook parsing, self-comment filtering, ack posting, pre-actions, - * and job queuing for GitHub webhook events. + * Runs full trigger dispatch() to determine if a job should be queued. + * Only posts ack comments and queues jobs when dispatch confirms a match. */ +import { getProjectGitHubToken } from '../config/projects.js'; import { findProjectByRepo } from '../config/provider.js'; +import { withGitHubToken } from '../github/client.js'; import { type PersonaIdentities, isCascadeBot, resolvePersonaIdentities, } from '../github/personas.js'; +import { withPMCredentials, withPMProvider } from '../pm/context.js'; +import { pmRegistry } from '../pm/registry.js'; +import { captureException } from '../sentry.js'; import type { TriggerRegistry } from '../triggers/registry.js'; -import type { TriggerContext } from '../types/index.js'; +import type { TriggerContext, TriggerResult } from '../types/index.js'; +import { logger } from '../utils/logging.js'; import { extractGitHubContext, generateAckMessage } from './ackMessageGenerator.js'; import { postGitHubAck, resolveGitHubTokenForAck } from './acknowledgments.js'; import { loadProjectConfig } from './config.js'; @@ -21,54 +27,18 @@ import { addEyesReactionToPR } from './pre-actions.js'; import { type CascadeJob, type GitHubJob, addJob } from './queue.js'; import { sendAcknowledgeReaction } from './reactions.js'; +// Ensure PM integrations are registered (idempotent — uses the same singleton registry +// that pm/index.ts populates, but we import from sub-modules to avoid pulling in +// the webhook handler's agent-execution transitive deps). +import { JiraIntegration } from '../pm/jira/integration.js'; +import { TrelloIntegration } from '../pm/trello/integration.js'; +if (!pmRegistry.getOrNull('trello')) pmRegistry.register(new TrelloIntegration()); +if (!pmRegistry.getOrNull('jira')) pmRegistry.register(new JiraIntegration()); + // --------------------------------------------------------------------------- // Internal helpers // --------------------------------------------------------------------------- -/** - * Try to match a trigger and post an ack comment for a GitHub webhook. - * Returns the ack comment ID and message text if posted, undefined otherwise. - */ -export async function tryPostGitHubAck( - eventType: string, - repoFullName: string, - payload: unknown, - triggerRegistry: TriggerRegistry, -): Promise<{ commentId: number; message: string } | undefined> { - const config = await loadProjectConfig(); - const fullProject = config.fullProjects.find((fp) => fp.repo === repoFullName); - if (!fullProject) return undefined; - - let personaIdentities: PersonaIdentities | undefined; - try { - personaIdentities = await resolvePersonaIdentities(fullProject.id); - } catch { - // Persona resolution may fail — proceed without ack - } - - const ctx: TriggerContext = { - project: fullProject, - source: 'github', - payload, - personaIdentities, - }; - const match = triggerRegistry.matchTrigger(ctx); - if (!match) return undefined; - - const context = extractGitHubContext(payload, eventType); - const message = await generateAckMessage(match.agentType, context, fullProject.id); - - const resolved = await resolveGitHubTokenForAck(repoFullName); - if (!resolved) return undefined; - - const tempJob = { eventType, repoFullName, payload } as GitHubJob; - const prNumber = extractPRNumber(tempJob); - if (!prNumber) return undefined; - - const commentId = await postGitHubAck(repoFullName, prNumber, message, resolved.token); - return commentId != null ? { commentId, message } : undefined; -} - export async function isSelfAuthoredGitHubComment( payload: unknown, repoFullName: string, @@ -94,7 +64,7 @@ export function fireGitHubAckReaction(repoFullName: string, payload: unknown): v try { const project = await findProjectByRepo(repoFullName); if (!project) { - console.warn('[Router] No project found for repo, skipping GitHub reaction', { + logger.warn('No project found for repo, skipping GitHub reaction', { repoFullName, }); return; @@ -102,7 +72,11 @@ export function fireGitHubAckReaction(repoFullName: string, payload: unknown): v const personaIdentities = await resolvePersonaIdentities(project.id); await sendAcknowledgeReaction('github', repoFullName, payload, personaIdentities, project); } catch (err) { - console.warn('[Router] GitHub reaction error:', String(err)); + logger.warn('GitHub reaction error', { error: String(err), repoFullName }); + captureException(err, { + tags: { source: 'github_ack_reaction' }, + extra: { repoFullName }, + }); } })(); } @@ -119,11 +93,38 @@ export function firePreActions(job: GitHubJob, p: Record): void const prs = suite?.pull_requests as Array | undefined; if (action === 'completed' && conclusion === 'success' && prs && prs.length > 0) { addEyesReactionToPR(job).catch((err) => - console.warn('[Router] Pre-action error (eyes reaction):', String(err)), + logger.warn('Pre-action error (eyes reaction)', { error: String(err) }), ); } } +async function tryPostAck( + agentType: string, + payload: unknown, + eventType: string, + repoFullName: string, + projectId: string, +): Promise<{ ackCommentId?: number; ackMessage?: string }> { + try { + const context = extractGitHubContext(payload, eventType); + const message = await generateAckMessage(agentType, context, projectId); + const resolved = await resolveGitHubTokenForAck(repoFullName); + if (resolved) { + const tempJob = { eventType, repoFullName, payload } as GitHubJob; + const prNumber = extractPRNumber(tempJob); + if (prNumber) { + const commentId = await postGitHubAck(repoFullName, prNumber, message, resolved.token); + if (commentId != null) { + return { ackCommentId: commentId, ackMessage: message }; + } + } + } + } catch (err) { + logger.warn('GitHub ack comment failed (non-fatal)', { error: String(err) }); + } + return {}; +} + export async function processGitHubWebhookEvent( eventType: string, repoFullName: string, @@ -134,30 +135,84 @@ export async function processGitHubWebhookEvent( eventType === 'issue_comment' || eventType === 'pull_request_review_comment'; if (isCommentEvent && (await isSelfAuthoredGitHubComment(payload, repoFullName))) { - console.log('[Router] Ignoring self-authored GitHub comment'); + logger.info('Ignoring self-authored GitHub comment', { repoFullName }); 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; - let ackMessage: string | undefined; + // Resolve project and credentials for authoritative dispatch + const config = await loadProjectConfig(); + const fullProject = config.fullProjects.find((fp) => fp.repo === repoFullName); + if (!fullProject) { + logger.info('No project for GitHub repo, skipping dispatch', { repoFullName }); + return; + } + + let personaIdentities: PersonaIdentities | undefined; try { - const ackResult = await tryPostGitHubAck(eventType, repoFullName, payload, triggerRegistry); - if (ackResult) { - ackCommentId = ackResult.commentId; - ackMessage = ackResult.message; - } + personaIdentities = await resolvePersonaIdentities(fullProject.id); + } catch { + // Persona resolution may fail — proceed without + } + + // Run authoritative trigger dispatch with all credential scopes + let result: TriggerResult | null = null; + try { + const githubToken = await getProjectGitHubToken(fullProject); + const pmProvider = pmRegistry.createProvider(fullProject); + + const ctx: TriggerContext = { + project: fullProject, + source: 'github', + payload, + personaIdentities, + }; + + result = await withPMCredentials( + fullProject.id, + fullProject.pm?.type, + (t) => pmRegistry.getOrNull(t), + () => + withPMProvider(pmProvider, () => + withGitHubToken(githubToken, () => triggerRegistry.dispatch(ctx)), + ), + ); } catch (err) { - console.warn('[Router] GitHub ack comment failed (non-fatal):', String(err)); + logger.warn('GitHub trigger dispatch failed (non-fatal)', { error: String(err), repoFullName }); + } + + if (!result) { + logger.info('No trigger matched for GitHub event', { eventType, repoFullName }); + return; } + logger.info('GitHub trigger matched', { + agentType: result.agentType || '(no agent)', + prNumber: result.prNumber, + repoFullName, + }); + + // For triggers with no agent (pr-merged, pr-ready-to-merge), dispatch already + // performed the PM operations. No job queuing needed. + if (!result.agentType) { + logger.info('Trigger completed without agent (PM operation done)'); + return; + } + + // Post ack comment — we KNOW the trigger matched + const { ackCommentId, ackMessage } = await tryPostAck( + result.agentType, + payload, + eventType, + repoFullName, + fullProject.id, + ); + + // Queue job with confirmed trigger result const job: CascadeJob = { type: 'github', source: 'github', @@ -167,6 +222,7 @@ export async function processGitHubWebhookEvent( receivedAt: new Date().toISOString(), ackCommentId, ackMessage, + triggerResult: result, }; // Fire pre-actions (non-blocking) before queueing @@ -175,9 +231,9 @@ export async function processGitHubWebhookEvent( try { const jobId = await addJob(job); - console.log('[Router] GitHub job queued:', { jobId, eventType, ackCommentId }); + logger.info('GitHub job queued', { jobId, eventType, ackCommentId }); } catch (err) { - console.error('[Router] Failed to queue GitHub job:', err); + logger.error('Failed to queue GitHub job', { error: String(err), eventType, repoFullName }); } } @@ -195,7 +251,8 @@ const PROCESSABLE_EVENTS = [ /** * Handle a POST /github/webhook request. - * Parses the payload, filters irrelevant events, and queues a job. + * Parses the payload, filters irrelevant events, dispatches triggers, + * and queues a job only when a trigger confirms a match. */ export async function handleGitHubWebhook( eventType: string, @@ -211,7 +268,7 @@ export async function handleGitHubWebhook( if (shouldProcess) { await processGitHubWebhookEvent(eventType, repoFullName, payload, triggerRegistry); } else { - console.log('[Router] Ignoring GitHub event:', eventType); + logger.debug('Ignoring GitHub event', { eventType }); } return { shouldProcess, repoFullName }; diff --git a/src/router/index.ts b/src/router/index.ts index d24edd06..65b687ba 100644 --- a/src/router/index.ts +++ b/src/router/index.ts @@ -9,6 +9,7 @@ import { } from '../server/webhookHandlers.js'; import { registerBuiltInTriggers } from '../triggers/builtins.js'; import { createTriggerRegistry } from '../triggers/registry.js'; +import { logger } from '../utils/logging.js'; import { handleGitHubWebhook } from './github.js'; import { handleJiraWebhook } from './jira.js'; import { getQueueStats } from './queue.js'; @@ -22,7 +23,7 @@ import { setTag('role', 'router'); -// Create trigger registry once at router startup for matchTrigger() calls +// Create trigger registry once at router startup for dispatch() calls const triggerRegistry = createTriggerRegistry(); registerBuiltInTriggers(triggerRegistry); @@ -123,7 +124,7 @@ app.post( // Graceful shutdown async function shutdown(signal: string): Promise { - console.log(`[Router] Received ${signal}, shutting down...`); + logger.info('Received shutdown signal', { signal }); await stopWorkerProcessor(); await flush(3000); process.exit(0); @@ -147,12 +148,12 @@ process.on('unhandledRejection', (reason) => { async function startRouter(): Promise { const port = Number(process.env.PORT) || 3000; startWorkerProcessor(); - console.log(`[Router] Starting on port ${port}`); + logger.info('Starting router', { port }); serve({ fetch: app.fetch, port }); } startRouter().catch(async (err) => { - console.error('[Router] Failed to start:', err); + logger.error('Failed to start router', { error: String(err) }); captureException(err, { tags: { source: 'router_startup' }, level: 'fatal' }); await flush(3000); process.exit(1); diff --git a/src/router/jira.ts b/src/router/jira.ts index db596499..7413023c 100644 --- a/src/router/jira.ts +++ b/src/router/jira.ts @@ -1,15 +1,18 @@ /** * JIRA webhook handler for the router (multi-container) deployment mode. * - * Handles webhook parsing, self-comment filtering, ack posting, and job queuing - * for JIRA webhook events. + * Runs full trigger dispatch() to determine if a job should be queued. + * Only posts ack comments and queues jobs when dispatch confirms a match. */ +import { withJiraCredentials } from '../jira/client.js'; import type { TriggerRegistry } from '../triggers/registry.js'; -import type { ProjectConfig, TriggerContext } from '../types/index.js'; +import type { ProjectConfig, TriggerContext, TriggerResult } from '../types/index.js'; +import { logger } from '../utils/logging.js'; import { extractJiraContext, generateAckMessage } from './ackMessageGenerator.js'; import { postJiraAck, resolveJiraBotAccountId } from './acknowledgments.js'; import { type RouterProjectConfig, loadProjectConfig } from './config.js'; +import { resolveJiraCredentials } from './platformClients.js'; import { type CascadeJob, addJob } from './queue.js'; import { sendAcknowledgeReaction } from './reactions.js'; @@ -17,31 +20,6 @@ import { sendAcknowledgeReaction } from './reactions.js'; // Internal helpers // --------------------------------------------------------------------------- -/** - * Try to match a trigger and post an ack comment for a JIRA webhook. - * Returns the ack comment ID if posted, undefined otherwise. - */ -export async function tryPostJiraAck( - projectId: string, - issueKey: string, - payload: unknown, - fullProjects: ProjectConfig[], - triggerRegistry: TriggerRegistry, -): Promise { - const fullProject = fullProjects.find((fp) => fp.id === projectId); - if (!fullProject || !issueKey) return undefined; - - const ctx: TriggerContext = { project: fullProject, source: 'jira', payload }; - const match = triggerRegistry.matchTrigger(ctx); - if (!match) return undefined; - - const context = extractJiraContext(payload); - const message = await generateAckMessage(match.agentType, context, projectId); - - const commentId = await postJiraAck(projectId, issueKey, message); - return commentId ?? undefined; -} - export async function isSelfAuthoredJiraComment( webhookEvent: string, payload: unknown, @@ -61,7 +39,10 @@ export async function isSelfAuthoredJiraComment( } } -export async function queueJiraJob( +/** + * Run authoritative dispatch and, if matched, post ack + queue job. + */ +export async function processJiraWebhookEvent( project: RouterProjectConfig, issueKey: string, webhookEvent: string, @@ -69,29 +50,64 @@ export async function queueJiraJob( fullProjects: ProjectConfig[], triggerRegistry: TriggerRegistry, ): Promise { - console.log('[Router] Queueing JIRA job:', { webhookEvent, issueKey, projectId: project.id }); - // Fire-and-forget acknowledgment reaction — only for comment events if (webhookEvent.startsWith('comment_')) { void sendAcknowledgeReaction('jira', project.id, payload).catch((err) => - console.error('[Router] JIRA reaction error:', err), + logger.error('JIRA reaction error', { error: String(err) }), ); } - // Try to post an ack comment via trigger matching (non-blocking best-effort) - let ackCommentId: string | undefined; + // Run authoritative trigger dispatch with credentials in scope + const fullProject = fullProjects.find((fp) => fp.id === project.id); + if (!fullProject) { + logger.info('No full project config for JIRA webhook, skipping', { projectId: project.id }); + return; + } + + let result: TriggerResult | null = null; try { - ackCommentId = await tryPostJiraAck( - project.id, - issueKey, - payload, - fullProjects, - triggerRegistry, - ); + const jiraCreds = await resolveJiraCredentials(project.id); + if (!jiraCreds) { + logger.warn('Missing JIRA credentials, cannot dispatch triggers', { projectId: project.id }); + } else { + const ctx: TriggerContext = { project: fullProject, source: 'jira', payload }; + result = await withJiraCredentials( + { email: jiraCreds.email, apiToken: jiraCreds.apiToken, baseUrl: jiraCreds.baseUrl }, + () => triggerRegistry.dispatch(ctx), + ); + } } catch (err) { - console.warn('[Router] JIRA ack comment failed (non-fatal):', String(err)); + logger.warn('JIRA trigger dispatch failed (non-fatal)', { + error: String(err), + projectId: project.id, + }); + } + + if (!result) { + logger.info('No trigger matched for JIRA event', { webhookEvent, issueKey }); + return; + } + + logger.info('JIRA trigger matched', { + agentType: result.agentType, + issueKey, + projectId: project.id, + }); + + // Post ack comment — we KNOW the trigger matched + let ackCommentId: string | undefined; + if (result.agentType) { + try { + const context = extractJiraContext(payload); + const message = await generateAckMessage(result.agentType, context, project.id); + const commentId = await postJiraAck(project.id, issueKey, message); + ackCommentId = commentId ?? undefined; + } catch (err) { + logger.warn('JIRA ack comment failed (non-fatal)', { error: String(err), issueKey }); + } } + // Queue job with confirmed trigger result const job: CascadeJob = { type: 'jira', source: 'jira', @@ -101,13 +117,14 @@ export async function queueJiraJob( webhookEvent, receivedAt: new Date().toISOString(), ackCommentId, + triggerResult: result, }; try { const jobId = await addJob(job); - console.log('[Router] JIRA job queued:', { jobId, webhookEvent, ackCommentId }); + logger.info('JIRA job queued', { jobId, webhookEvent, ackCommentId }); } catch (err) { - console.error('[Router] Failed to queue JIRA job:', err); + logger.error('Failed to queue JIRA job', { error: String(err), webhookEvent, issueKey }); } } @@ -124,7 +141,8 @@ const PROCESSABLE_EVENTS = [ /** * Handle a POST /jira/webhook request. - * Parses the payload, filters irrelevant events, and queues a job. + * Parses the payload, filters irrelevant events, dispatches triggers, + * and queues a job only when a trigger confirms a match. */ export async function handleJiraWebhook( payload: unknown, @@ -148,9 +166,9 @@ export async function handleJiraWebhook( if (shouldProcess && project) { if (await isSelfAuthoredJiraComment(webhookEvent, payload, project.id)) { - console.log('[Router] Ignoring self-authored JIRA comment'); + logger.info('Ignoring self-authored JIRA comment', { webhookEvent }); } else { - await queueJiraJob( + await processJiraWebhookEvent( project, issueKey, webhookEvent, @@ -160,7 +178,7 @@ export async function handleJiraWebhook( ); } } else { - console.log(`[Router] Ignoring JIRA: ${webhookEvent}`); + logger.debug('Ignoring JIRA event', { webhookEvent }); } return { shouldProcess, project, webhookEvent }; diff --git a/src/router/queue.ts b/src/router/queue.ts index cb298542..92dbc92a 100644 --- a/src/router/queue.ts +++ b/src/router/queue.ts @@ -1,5 +1,7 @@ import { type ConnectionOptions, Queue } from 'bullmq'; import { captureException } from '../sentry.js'; +import type { TriggerResult } from '../types/index.js'; +import { logger } from '../utils/logging.js'; import { routerConfig } from './config.js'; // Parse Redis URL to connection options @@ -15,6 +17,9 @@ function parseRedisUrl(url: string): ConnectionOptions { const connection = parseRedisUrl(routerConfig.redisUrl); // Job types +// Note: ackCommentId is `string` for Trello/JIRA (string IDs from their APIs) +// and `number` for GitHub (numeric IDs from GitHub API). Downstream consumers +// (ProgressMonitor) normalize to string via the adapter layer. export interface TrelloJob { type: 'trello'; source: 'trello'; @@ -24,6 +29,7 @@ export interface TrelloJob { actionType: string; receivedAt: string; ackCommentId?: string; + triggerResult?: TriggerResult; } export interface GitHubJob { @@ -35,6 +41,7 @@ export interface GitHubJob { receivedAt: string; ackCommentId?: number; ackMessage?: string; + triggerResult?: TriggerResult; } export interface JiraJob { @@ -46,6 +53,7 @@ export interface JiraJob { webhookEvent: string; receivedAt: string; ackCommentId?: string; + triggerResult?: TriggerResult; } export type CascadeJob = TrelloJob | GitHubJob | JiraJob; @@ -67,17 +75,17 @@ export const jobQueue = new Queue('cascade-jobs', { // Queue event logging jobQueue.on('error', (err) => { - console.error('[Queue] Error:', err); + logger.error('Queue error', { error: String(err) }); captureException(err, { tags: { source: 'job_queue' } }); }); -console.log('[Queue] Initialized with Redis at', routerConfig.redisUrl); +logger.info('Queue initialized', { redisUrl: routerConfig.redisUrl }); // Helper to add a job export async function addJob(job: CascadeJob): Promise { const jobId = `${job.type}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; const result = await jobQueue.add(job.type, job, { jobId }); - console.log('[Queue] Job added:', { id: result.id, type: job.type }); + logger.info('Job added to queue', { id: result.id, type: job.type }); return result.id ?? jobId; } diff --git a/src/router/trello.ts b/src/router/trello.ts index 7e8f0aa9..b23f2add 100644 --- a/src/router/trello.ts +++ b/src/router/trello.ts @@ -1,15 +1,18 @@ /** * Trello webhook handler for the router (multi-container) deployment mode. * - * Handles webhook parsing, self-comment filtering, ack posting, and job queuing - * for Trello webhook events. + * Runs full trigger dispatch() to determine if a job should be queued. + * Only posts ack comments and queues jobs when dispatch confirms a match. */ +import { withTrelloCredentials } from '../trello/client.js'; import type { TriggerRegistry } from '../triggers/registry.js'; -import type { TriggerContext } from '../types/index.js'; +import type { TriggerContext, TriggerResult } from '../types/index.js'; +import { logger } from '../utils/logging.js'; import { extractTrelloContext, generateAckMessage } from './ackMessageGenerator.js'; import { postTrelloAck, resolveTrelloBotMemberId } from './acknowledgments.js'; import { type RouterProjectConfig, loadProjectConfig } from './config.js'; +import { resolveTrelloCredentials } from './platformClients.js'; import { type CascadeJob, addJob } from './queue.js'; import { sendAcknowledgeReaction } from './reactions.js'; @@ -20,9 +23,10 @@ import { sendAcknowledgeReaction } from './reactions.js'; /** * Check if filename matches agent log pattern: {agent-type}-{timestamp}.zip * Examples: implementation-2026-01-02T16-30-24-339Z.zip, briefing-timeout-2026-01-02T12-34-56-789Z.zip + * The timestamp follows ISO 8601 format with colons replaced by hyphens: YYYY-MM-DDTHH-MM-SS-mmmZ */ export function isAgentLogFilename(filename: string): boolean { - return /^[a-z]+(?:-timeout)?-[\d-TZ]+\.zip$/i.test(filename); + return /^.+-\d{4}-\d{2}-\d{2}T[\d-]+Z\.zip$/.test(filename); } export function isCardInTriggerList( @@ -42,7 +46,7 @@ export function isCardInTriggerList( const listAfter = data.listAfter as Record; const listId = listAfter.id as string; if (triggerLists.includes(listId)) { - console.log(`[Router] Card moved to trigger list: ${listId}`); + logger.info('Card moved to trigger list', { listId }); return true; } } @@ -52,7 +56,7 @@ export function isCardInTriggerList( const list = data.list as Record; const listId = list.id as string; if (triggerLists.includes(listId)) { - console.log(`[Router] Card created in trigger list: ${listId}`); + logger.info('Card created in trigger list', { listId }); return true; } } @@ -72,7 +76,7 @@ export function isReadyToProcessLabelAdded( const labelId = label.id as string; if (labelId === project.trello.labels.readyToProcess) { - console.log('[Router] Ready-to-process label added'); + logger.info('Ready-to-process label added', { labelId }); return true; } return false; @@ -90,7 +94,7 @@ export function isAgentLogAttachmentUploaded( const name = attachment.name as string | undefined; if (name && isAgentLogFilename(name) && !name.startsWith('debug-')) { - console.log(`[Router] Agent log attachment uploaded: ${name}`); + logger.info('Agent log attachment uploaded', { name }); return true; } return false; @@ -140,31 +144,6 @@ export async function parseTrelloWebhook(payload: unknown): Promise { - const config = await loadProjectConfig(); - const fullProject = config.fullProjects.find((fp) => fp.id === projectId); - if (!fullProject) return undefined; - - const ctx: TriggerContext = { project: fullProject, source: 'trello', payload }; - const match = triggerRegistry.matchTrigger(ctx); - if (!match) return undefined; - - const context = extractTrelloContext(payload); - const message = await generateAckMessage(match.agentType, context, projectId); - - const commentId = await postTrelloAck(projectId, cardId, message); - return commentId ?? undefined; -} - export async function isSelfAuthoredTrelloComment( payload: unknown, projectId: string, @@ -180,6 +159,9 @@ export async function isSelfAuthoredTrelloComment( } } +/** + * Run authoritative dispatch and, if matched, post ack + queue job. + */ export async function processTrelloWebhookEvent( project: RouterProjectConfig, cardId: string, @@ -188,27 +170,68 @@ export async function processTrelloWebhookEvent( triggerRegistry: TriggerRegistry, ): Promise { if (actionType === 'commentCard' && (await isSelfAuthoredTrelloComment(payload, project.id))) { - console.log('[Router] Ignoring self-authored Trello comment'); + logger.info('Ignoring self-authored Trello comment', { projectId: project.id }); 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), + logger.error('Trello reaction error', { error: String(err) }), ); } - // Try to post an ack comment via trigger matching (non-blocking best-effort) - let ackCommentId: string | undefined; + // Run authoritative trigger dispatch with credentials in scope + const config = await loadProjectConfig(); + const fullProject = config.fullProjects.find((fp) => fp.id === project.id); + if (!fullProject) { + logger.info('No full project config for Trello webhook, skipping', { projectId: project.id }); + return; + } + + let result: TriggerResult | null = null; try { - ackCommentId = await tryPostTrelloAck(project.id, cardId, payload, triggerRegistry); + const trelloCreds = await resolveTrelloCredentials(project.id); + if (!trelloCreds) { + logger.warn('Missing Trello credentials, cannot dispatch triggers', { + projectId: project.id, + }); + } else { + const ctx: TriggerContext = { project: fullProject, source: 'trello', payload }; + result = await withTrelloCredentials(trelloCreds, () => triggerRegistry.dispatch(ctx)); + } } catch (err) { - console.warn('[Router] Trello ack comment failed (non-fatal):', String(err)); + logger.warn('Trello trigger dispatch failed (non-fatal)', { + error: String(err), + projectId: project.id, + }); + } + + if (!result) { + logger.info('No trigger matched for Trello event', { actionType, cardId }); + return; + } + + logger.info('Trello trigger matched', { + agentType: result.agentType, + cardId, + projectId: project.id, + }); + + // Post ack comment — we KNOW the trigger matched + let ackCommentId: string | undefined; + if (result.agentType) { + try { + const context = extractTrelloContext(payload); + const message = await generateAckMessage(result.agentType, context, project.id); + const commentId = await postTrelloAck(project.id, cardId, message); + ackCommentId = commentId ?? undefined; + } catch (err) { + logger.warn('Trello ack comment failed (non-fatal)', { error: String(err), cardId }); + } } + // Queue job with confirmed trigger result const job: CascadeJob = { type: 'trello', source: 'trello', @@ -218,13 +241,14 @@ export async function processTrelloWebhookEvent( actionType: actionType || 'unknown', receivedAt: new Date().toISOString(), ackCommentId, + triggerResult: result, }; try { const jobId = await addJob(job); - console.log('[Router] Trello job queued:', { jobId, actionType, ackCommentId }); + logger.info('Trello job queued', { jobId, actionType, ackCommentId }); } catch (err) { - console.error('[Router] Failed to queue Trello job:', err); + logger.error('Failed to queue Trello job', { error: String(err), actionType, cardId }); // Still return to caller — Trello gets 200 to avoid retries } } @@ -235,7 +259,8 @@ export async function processTrelloWebhookEvent( /** * Handle a POST /trello/webhook request. - * Parses the payload, filters irrelevant events, and queues a job. + * Parses the payload, filters irrelevant events, dispatches triggers, + * and queues a job only when a trigger confirms a match. */ export async function handleTrelloWebhook( payload: unknown, @@ -257,7 +282,7 @@ export async function handleTrelloWebhook( triggerRegistry, ); } else { - console.log(`[Router] Ignoring Trello: ${actionType || 'unknown'}`); + logger.debug('Ignoring Trello event', { actionType: actionType || 'unknown' }); } return { shouldProcess, project, actionType, cardId }; diff --git a/src/triggers/github/check-suite-failure.ts b/src/triggers/github/check-suite-failure.ts index 49725f72..89a1dceb 100644 --- a/src/triggers/github/check-suite-failure.ts +++ b/src/triggers/github/check-suite-failure.ts @@ -41,10 +41,6 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { return true; } - resolveAgentType(): string { - return 'respond-to-ci'; - } - async handle(ctx: TriggerContext): Promise { const payload = ctx.payload as GitHubCheckSuitePayload; const { owner, repo } = parseRepoFullName(payload.repository.full_name); diff --git a/src/triggers/github/check-suite-success.ts b/src/triggers/github/check-suite-success.ts index fb989719..0f0c410a 100644 --- a/src/triggers/github/check-suite-success.ts +++ b/src/triggers/github/check-suite-success.ts @@ -12,8 +12,10 @@ const RETRY_DELAY_MS = 10_000; /** * Wait for all check suites to complete, retrying when some are still in-progress. * Returns immediately if all checks have completed (whether passing or failing). + * + * Called by the worker before starting the review agent (not in the trigger handler). */ -async function waitForChecks( +export async function waitForChecks( owner: string, repo: string, headSha: string, @@ -84,10 +86,6 @@ export class CheckSuiteSuccessTrigger implements TriggerHandler { return true; } - resolveAgentType(): string { - return 'review'; - } - async handle(ctx: TriggerContext): Promise { const payload = ctx.payload as GitHubCheckSuitePayload; const { owner, repo } = parseRepoFullName(payload.repository.full_name); @@ -171,29 +169,15 @@ export class CheckSuiteSuccessTrigger implements TriggerHandler { }); } - // Verify all checks are actually passing (double-check) - // Uses the implementer token already in scope (set by webhook-handler), - // which has actions:read permission. The reviewer's fine-grained PAT may not. - // + // The trigger decision is made — the review agent should run. + // Actual check polling (waitForChecks) is deferred to the worker via the flag. // GitHub fires a check_suite webhook per individual suite completion. // When multiple suites exist, the first webhook arrives before other suites finish. - // waitForChecks retries when checks are still in-progress, but bails on genuine failures. - const checkStatus = await waitForChecks(owner, repo, headSha, prNumber); - - if (!checkStatus.allPassing) { - logger.info('Not all checks passing, skipping review trigger', { - prNumber, - totalChecks: checkStatus.totalCount, - failing: checkStatus.checkRuns.filter((c) => c.conclusion !== 'success').map((c) => c.name), - }); - return null; - } - - logger.info('All CI checks passed - triggering review', { + // The worker will poll until all checks pass before starting the agent. + logger.info('Check-suite success trigger matched — deferring check polling to worker', { prNumber, workItemId, headSha, - totalChecks: checkStatus.totalCount, }); return { @@ -208,6 +192,7 @@ export class CheckSuiteSuccessTrigger implements TriggerHandler { }, prNumber, workItemId, + waitForChecks: true, }; } } diff --git a/src/triggers/github/pr-comment-mention.ts b/src/triggers/github/pr-comment-mention.ts index cb20db7b..b6d0aa7e 100644 --- a/src/triggers/github/pr-comment-mention.ts +++ b/src/triggers/github/pr-comment-mention.ts @@ -39,10 +39,6 @@ export class PRCommentMentionTrigger implements TriggerHandler { return false; } - resolveAgentType(): string { - return 'respond-to-pr-comment'; - } - async handle(ctx: TriggerContext): Promise { // Require persona identities for @mention detection if (!ctx.personaIdentities) { diff --git a/src/triggers/github/pr-merged.ts b/src/triggers/github/pr-merged.ts index 63ed247d..2473fee1 100644 --- a/src/triggers/github/pr-merged.ts +++ b/src/triggers/github/pr-merged.ts @@ -24,10 +24,6 @@ export class PRMergedTrigger implements TriggerHandler { return ctx.payload.action === 'closed'; } - resolveAgentType(): string | null { - return null; // No agent — performs card move directly - } - async handle(ctx: TriggerContext): Promise { const payload = ctx.payload as GitHubPullRequestPayload; const { owner, repo } = parseRepoFullName(payload.repository.full_name); diff --git a/src/triggers/github/pr-opened.ts b/src/triggers/github/pr-opened.ts index fe315449..daba972d 100644 --- a/src/triggers/github/pr-opened.ts +++ b/src/triggers/github/pr-opened.ts @@ -30,10 +30,6 @@ export class PROpenedTrigger implements TriggerHandler { return true; } - resolveAgentType(): string { - return 'respond-to-review'; - } - async handle(ctx: TriggerContext): Promise { const payload = ctx.payload as { pull_request: { diff --git a/src/triggers/github/pr-ready-to-merge.ts b/src/triggers/github/pr-ready-to-merge.ts index 78bd5b84..f70c31c8 100644 --- a/src/triggers/github/pr-ready-to-merge.ts +++ b/src/triggers/github/pr-ready-to-merge.ts @@ -46,10 +46,6 @@ export class PRReadyToMergeTrigger implements TriggerHandler { return false; } - resolveAgentType(): string | null { - return null; // No agent — performs card move directly - } - async handle(ctx: TriggerContext): Promise { let prNumber: number; let headSha: string; diff --git a/src/triggers/github/pr-review-submitted.ts b/src/triggers/github/pr-review-submitted.ts index 0ad21712..7a15662e 100644 --- a/src/triggers/github/pr-review-submitted.ts +++ b/src/triggers/github/pr-review-submitted.ts @@ -27,10 +27,6 @@ export class PRReviewSubmittedTrigger implements TriggerHandler { return true; } - resolveAgentType(): string { - return 'respond-to-review'; - } - async handle(ctx: TriggerContext): Promise { // Type assertion since we validated in matches() const reviewPayload = ctx.payload as { diff --git a/src/triggers/github/review-requested.ts b/src/triggers/github/review-requested.ts index af7fc097..2d1eb0a1 100644 --- a/src/triggers/github/review-requested.ts +++ b/src/triggers/github/review-requested.ts @@ -40,10 +40,6 @@ export class ReviewRequestedTrigger implements TriggerHandler { return true; } - resolveAgentType(): string { - return 'review'; - } - async handle(ctx: TriggerContext): Promise { const payload = ctx.payload as GitHubPullRequestPayload; const prNumber = payload.pull_request.number; diff --git a/src/triggers/github/webhook-handler.ts b/src/triggers/github/webhook-handler.ts index 1b0b1e08..e818fa6c 100644 --- a/src/triggers/github/webhook-handler.ts +++ b/src/triggers/github/webhook-handler.ts @@ -3,6 +3,7 @@ import { loadProjectConfigByRepo } from '../../config/provider.js'; import { getSessionState } from '../../gadgets/sessionState.js'; import { githubClient, withGitHubToken } from '../../github/client.js'; import { getPersonaToken, resolvePersonaIdentities } from '../../github/personas.js'; +import { withPMCredentials } from '../../pm/context.js'; import { createPMProvider, pmRegistry, withPMProvider } from '../../pm/index.js'; import { extractGitHubContext, generateAckMessage } from '../../router/ackMessageGenerator.js'; import type { CascadeConfig, ProjectConfig, TriggerContext } from '../../types/index.js'; @@ -90,19 +91,6 @@ async function postAcknowledgmentComment( } } -/** - * Establish PM credential scope for the project. - * Uses the integration's withCredentials() for the correct PM type. - * Falls through to running fn() directly if no PM type is configured. - */ -async function withPMCredentials(project: ProjectConfig, fn: () => Promise): Promise { - const pmType = project.pm?.type; - if (!pmType) return fn(); - const integration = pmRegistry.getOrNull(pmType); - if (!integration) return fn(); - return integration.withCredentials(project.id, fn); -} - async function executeGitHubAgent( result: TriggerResult, project: ProjectConfig, @@ -122,12 +110,16 @@ async function executeGitHubAgent( try { const pmProvider = createPMProvider(project); - await withPMCredentials(project, () => - withPMProvider(pmProvider, () => - withGitHubToken(githubToken, () => - runAgentExecutionPipeline(result, project, config, executionConfig), + await withPMCredentials( + project.id, + project.pm?.type, + (t) => pmRegistry.getOrNull(t), + () => + withPMProvider(pmProvider, () => + withGitHubToken(githubToken, () => + runAgentExecutionPipeline(result, project, config, executionConfig), + ), ), - ), ); } finally { restoreLlmEnv(); @@ -196,14 +188,98 @@ function processNextQueuedGitHubWebhook(registry: TriggerRegistry): void { ); } +/** + * Poll until all CI checks pass before starting the agent. + * Returns false if checks don't pass after polling (agent should be skipped). + */ +async function pollWaitForChecks( + result: TriggerResult, + repoFullName: string, + githubToken: string, +): Promise { + const { waitForChecks } = await import('./check-suite-success.js'); + const { owner, repo } = parseRepoFullName(repoFullName); + const headSha = result.agentInput.headSha as string; + const prNumber = result.prNumber ?? 0; + + logger.info('Waiting for all checks to pass before starting agent', { prNumber, headSha }); + + const checkStatus = await withGitHubToken(githubToken, () => + waitForChecks(owner, repo, headSha, prNumber), + ); + + if (!checkStatus.allPassing) { + logger.info('Not all checks passing after polling, skipping agent', { + prNumber, + headSha, + failedChecks: checkStatus.checkRuns + .filter((c) => c.conclusion !== 'success') + .map((c) => c.name), + }); + return false; + } + + logger.info('All checks passing, proceeding with agent', { prNumber }); + return true; +} + +/** Try to enqueue the webhook if another job is already processing. Returns true if enqueued (caller should return). */ +function tryEnqueueIfBusy( + payload: unknown, + eventType: string, + ackCommentId?: number, + ackMessage?: string, +): boolean { + if (!isCurrentlyProcessing()) return false; + + const queued = enqueueWebhook(payload, eventType, ackCommentId, ackMessage); + if (queued) { + logger.info('Currently processing, GitHub webhook queued', { + queueLength: getQueueLength(), + eventType, + }); + } else { + logger.warn('Queue full, GitHub webhook rejected', { queueLength: getQueueLength() }); + } + return true; +} + +/** Resolve trigger result — use pre-resolved from router or dispatch via registry. */ +async function resolveTriggerResult( + existing: TriggerResult | undefined, + project: ProjectConfig, + payload: unknown, + personaIdentities: Awaited>, + githubToken: string, + registry: TriggerRegistry, +): Promise { + if (existing) { + logger.info('Using pre-resolved trigger result for GitHub webhook', { + agentType: existing.agentType, + }); + return existing; + } + + const ctx: TriggerContext = { project, source: 'github', payload, personaIdentities }; + const pmProvider = createPMProvider(project); + return withPMCredentials( + project.id, + project.pm?.type, + (t) => pmRegistry.getOrNull(t), + () => + withPMProvider(pmProvider, () => withGitHubToken(githubToken, () => registry.dispatch(ctx))), + ); +} + export async function processGitHubWebhook( payload: unknown, eventType: string, registry: TriggerRegistry, ackCommentId?: number, ackMessage?: string, + triggerResult?: TriggerResult, ): Promise { - logger.info('Processing GitHub webhook', { eventType }); + logger.info('Processing GitHub webhook', { eventType, hasTriggerResult: !!triggerResult }); const p = payload as Record; const repository = p.repository as Record | undefined; @@ -214,18 +290,7 @@ export async function processGitHubWebhook( return; } - if (isCurrentlyProcessing()) { - const queued = enqueueWebhook(payload, eventType, ackCommentId, ackMessage); - if (queued) { - logger.info('Currently processing, GitHub webhook queued', { - queueLength: getQueueLength(), - eventType, - }); - } else { - logger.warn('Queue full, GitHub webhook rejected', { queueLength: getQueueLength() }); - } - return; - } + if (tryEnqueueIfBusy(payload, eventType, ackCommentId, ackMessage)) return; const projectConfig = await loadProjectConfigByRepo(repoFullName); if (!projectConfig) { @@ -234,25 +299,20 @@ export async function processGitHubWebhook( } const { project, config } = projectConfig; - // Resolve persona identities and use implementer token for webhook processing const personaIdentities = await resolvePersonaIdentities(project.id); const githubToken = await getPersonaToken(project.id, 'implementation'); - const pmProvider = createPMProvider(project); - // Establish PM credential + provider scope for trigger dispatch - const ctx: TriggerContext = { project, source: 'github', payload, personaIdentities }; - const result = await withPMCredentials(project, () => - withPMProvider(pmProvider, () => withGitHubToken(githubToken, () => registry.dispatch(ctx))), + const result = await resolveTriggerResult( + triggerResult, + project, + payload, + personaIdentities, + githubToken, + registry, ); if (!result) { logger.info('No trigger matched for GitHub webhook', { eventType, repoFullName }); - // Clean up orphan ack if router posted one but no trigger matched - if (ackCommentId) { - logger.info('Cleaning up orphan ack comment', { ackCommentId, repoFullName }); - const { deleteGitHubAck } = await import('../../router/acknowledgments.js'); - await deleteGitHubAck(repoFullName, ackCommentId, githubToken).catch(() => {}); - } return; } @@ -264,6 +324,12 @@ export async function processGitHubWebhook( result.agentInput.ackMessage = ackMessage; } + // Poll until all CI checks pass before starting agent (deferred from trigger) + if (result.waitForChecks) { + const checksOk = await pollWaitForChecks(result, repoFullName, githubToken); + if (!checksOk) return; + } + logger.info('GitHub trigger matched', { agentType: result.agentType || '(no agent)', prNumber: result.prNumber, diff --git a/src/triggers/jira/comment-mention.ts b/src/triggers/jira/comment-mention.ts index a4982984..0ca534b4 100644 --- a/src/triggers/jira/comment-mention.ts +++ b/src/triggers/jira/comment-mention.ts @@ -10,23 +10,7 @@ import { jiraClient } from '../../jira/client.js'; import { getJiraConfig } from '../../pm/config.js'; import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; - -interface JiraWebhookPayload { - webhookEvent: string; - issue?: { - id?: string; - key: string; - fields?: { - project?: { key?: string }; - status?: { name?: string }; - }; - }; - comment?: { - id?: string; - body?: unknown; - author?: { displayName?: string; accountId?: string }; - }; -} +import type { JiraWebhookPayload } from './types.js'; // Cache authenticated user info to avoid repeated API calls let cachedUserInfo: { accountId: string; displayName: string } | null = null; @@ -120,10 +104,6 @@ export class JiraCommentMentionTrigger implements TriggerHandler { return payload.webhookEvent === 'comment_created' || payload.webhookEvent === 'comment_updated'; } - resolveAgentType(): string { - return 'respond-to-planning-comment'; - } - async handle(ctx: TriggerContext): Promise { const payload = ctx.payload as JiraWebhookPayload; const issueKey = payload.issue?.key; diff --git a/src/triggers/jira/issue-transitioned.ts b/src/triggers/jira/issue-transitioned.ts index 7d5fea27..0b8d600e 100644 --- a/src/triggers/jira/issue-transitioned.ts +++ b/src/triggers/jira/issue-transitioned.ts @@ -12,40 +12,7 @@ import { import { getJiraConfig } from '../../pm/config.js'; import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; - -interface JiraWebhookPayload { - webhookEvent: string; - issue?: { - key: string; - fields?: { - project?: { key?: string }; - status?: { name?: string }; - summary?: string; - }; - }; - changelog?: { - items?: Array<{ - field?: string; - fromString?: string; - toString?: string; - }>; - }; -} - -/** - * Maps a JIRA status name to the CASCADE agent type based on project config. - * - * project.jira.statuses maps CASCADE status names to JIRA status names, e.g.: - * { briefing: "Briefing", planning: "Planning", todo: "To Do" } - * - * We invert this mapping: if the issue transitioned to "Briefing", we fire - * the briefing agent. - */ -const STATUS_TO_AGENT: Record = { - briefing: 'briefing', - planning: 'planning', - todo: 'implementation', -}; +import { type JiraWebhookPayload, STATUS_TO_AGENT } from './types.js'; export class JiraIssueTransitionedTrigger implements TriggerHandler { name = 'jira-issue-transitioned'; @@ -67,23 +34,6 @@ export class JiraIssueTransitionedTrigger implements TriggerHandler { return !!statusChange; } - resolveAgentType(ctx: TriggerContext): string | null { - const payload = ctx.payload as JiraWebhookPayload; - const statusChange = payload.changelog?.items?.find((item) => item.field === 'status'); - const newStatus = statusChange?.toString; - if (!newStatus) return null; - - const jiraConfig = getJiraConfig(ctx.project); - if (!jiraConfig?.statuses) return null; - - for (const [cascadeStatus, jiraStatus] of Object.entries(jiraConfig.statuses)) { - if (jiraStatus.toLowerCase() === newStatus.toLowerCase()) { - return STATUS_TO_AGENT[cascadeStatus] ?? null; - } - } - return null; - } - async handle(ctx: TriggerContext): Promise { const payload = ctx.payload as JiraWebhookPayload; const issueKey = payload.issue?.key; diff --git a/src/triggers/jira/label-added.ts b/src/triggers/jira/label-added.ts index 672ac763..a81e2a39 100644 --- a/src/triggers/jira/label-added.ts +++ b/src/triggers/jira/label-added.ts @@ -18,34 +18,7 @@ import { getJiraConfig } from '../../pm/config.js'; import { resolveProjectPMConfig } from '../../pm/lifecycle.js'; import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; - -interface JiraChangelogItem { - field?: string; - fromString?: string; - toString?: string; -} - -interface JiraLabelPayload { - webhookEvent: string; - issue?: { - key: string; - fields?: { - project?: { key?: string }; - status?: { name?: string }; - summary?: string; - }; - }; - changelog?: { - items?: JiraChangelogItem[]; - }; -} - -/** Same status→agent mapping as issue-transitioned.ts */ -const STATUS_TO_AGENT: Record = { - briefing: 'briefing', - planning: 'planning', - todo: 'implementation', -}; +import { type JiraWebhookPayload, STATUS_TO_AGENT } from './types.js'; /** * Parse which labels were added from a JIRA label changelog item. @@ -56,7 +29,7 @@ const STATUS_TO_AGENT: Record = { * * Returns the set of labels present in `toString` but not in `fromString`. */ -function parseAddedLabels(item: JiraChangelogItem): Set { +function parseAddedLabels(item: { fromString?: string; toString?: string }): Set { const before = new Set((item.fromString ?? '').split(/\s+/).filter(Boolean)); const after = (item.toString ?? '').split(/\s+/).filter(Boolean); return new Set(after.filter((label) => !before.has(label))); @@ -74,7 +47,7 @@ export class JiraReadyToProcessLabelTrigger implements TriggerHandler { return false; } - const payload = ctx.payload as JiraLabelPayload; + const payload = ctx.payload as JiraWebhookPayload; if (!payload.webhookEvent?.startsWith('jira:issue_updated')) return false; const items = payload.changelog?.items; @@ -97,24 +70,8 @@ export class JiraReadyToProcessLabelTrigger implements TriggerHandler { return addedLabels.has(readyLabel); } - resolveAgentType(ctx: TriggerContext): string | null { - const payload = ctx.payload as JiraLabelPayload; - const currentStatus = payload.issue?.fields?.status?.name; - if (!currentStatus) return null; - - const jiraConfig = getJiraConfig(ctx.project); - if (!jiraConfig?.statuses) return null; - - for (const [cascadeStatus, jiraStatus] of Object.entries(jiraConfig.statuses)) { - if (jiraStatus.toLowerCase() === currentStatus.toLowerCase()) { - return STATUS_TO_AGENT[cascadeStatus] ?? null; - } - } - return null; - } - async handle(ctx: TriggerContext): Promise { - const payload = ctx.payload as JiraLabelPayload; + const payload = ctx.payload as JiraWebhookPayload; const issueKey = payload.issue?.key; if (!issueKey) { diff --git a/src/triggers/jira/types.ts b/src/triggers/jira/types.ts new file mode 100644 index 00000000..730396ec --- /dev/null +++ b/src/triggers/jira/types.ts @@ -0,0 +1,51 @@ +/** + * Shared JIRA webhook types and constants used across JIRA trigger handlers. + */ + +// --------------------------------------------------------------------------- +// Webhook Payload +// --------------------------------------------------------------------------- + +export interface JiraWebhookPayload { + webhookEvent: string; + issue?: { + id?: string; + key: string; + fields?: { + project?: { key?: string }; + status?: { name?: string }; + summary?: string; + }; + }; + changelog?: { + items?: Array<{ + field?: string; + fromString?: string; + toString?: string; + }>; + }; + comment?: { + id?: string; + body?: unknown; + author?: { displayName?: string; accountId?: string }; + }; +} + +// --------------------------------------------------------------------------- +// Constants +// --------------------------------------------------------------------------- + +/** + * Maps CASCADE status keys to agent types. + * + * Project config maps CASCADE status names to JIRA status names, e.g.: + * { briefing: "Briefing", planning: "Planning", todo: "To Do" } + * + * We invert that mapping at runtime: if the issue transitioned to "Briefing", + * we look up `briefing` → `briefing` agent. + */ +export const STATUS_TO_AGENT: Record = { + briefing: 'briefing', + planning: 'planning', + todo: 'implementation', +}; diff --git a/src/triggers/jira/webhook-handler.ts b/src/triggers/jira/webhook-handler.ts index 9f034b88..d641e297 100644 --- a/src/triggers/jira/webhook-handler.ts +++ b/src/triggers/jira/webhook-handler.ts @@ -7,13 +7,15 @@ import { pmRegistry } from '../../pm/index.js'; import { processPMWebhook } from '../../pm/webhook-handler.js'; +import type { TriggerResult } from '../../types/index.js'; import type { TriggerRegistry } from '../registry.js'; export async function processJiraWebhook( payload: unknown, registry: TriggerRegistry, ackCommentId?: string, + triggerResult?: TriggerResult, ): Promise { const integration = pmRegistry.get('jira'); - await processPMWebhook(integration, payload, registry, ackCommentId); + await processPMWebhook(integration, payload, registry, ackCommentId, triggerResult); } diff --git a/src/triggers/registry.ts b/src/triggers/registry.ts index 88a5649b..dc2aa5f3 100644 --- a/src/triggers/registry.ts +++ b/src/triggers/registry.ts @@ -41,20 +41,6 @@ export class TriggerRegistry { return null; } - /** - * Find the first handler that matches the context and can resolve an agent type. - * Pure logic — no API calls, no side effects. Safe to call in the router. - */ - matchTrigger(ctx: TriggerContext): { name: string; agentType: string } | null { - for (const handler of this.handlers) { - if (handler.matches(ctx)) { - const agentType = handler.resolveAgentType(ctx); - if (agentType) return { name: handler.name, agentType }; - } - } - return null; - } - getHandlers(): TriggerHandler[] { return [...this.handlers]; } diff --git a/src/triggers/trello/card-moved.ts b/src/triggers/trello/card-moved.ts index 3f90e79e..d6a5b009 100644 --- a/src/triggers/trello/card-moved.ts +++ b/src/triggers/trello/card-moved.ts @@ -1,5 +1,6 @@ import { resolveTrelloTriggerEnabled } from '../../config/triggerConfig.js'; import { getTrelloConfig } from '../../pm/config.js'; +import { logger } from '../../utils/logging.js'; import type { TrelloWebhookPayload, TriggerContext, @@ -51,16 +52,13 @@ function createCardMovedTrigger(config: CardMovedConfig): TriggerHandler { return isMove || isCreate; }, - resolveAgentType(): string { - return config.agentType; - }, - - async handle(ctx: TriggerContext): Promise { + async handle(ctx: TriggerContext): Promise { const payload = ctx.payload as TrelloWebhookPayload; const cardId = payload.action.data.card?.id; if (!cardId) { - throw new Error('No card ID in payload'); + logger.warn('No card ID in Trello card-moved payload', { trigger: config.name }); + return null; } return { diff --git a/src/triggers/trello/comment-mention.ts b/src/triggers/trello/comment-mention.ts index 5ecfc741..f49adfe7 100644 --- a/src/triggers/trello/comment-mention.ts +++ b/src/triggers/trello/comment-mention.ts @@ -45,10 +45,6 @@ export class TrelloCommentMentionTrigger implements TriggerHandler { return ctx.payload.action.type === 'commentCard'; } - resolveAgentType(): string { - return 'respond-to-planning-comment'; - } - async handle(ctx: TriggerContext): Promise { const payload = ctx.payload as TrelloWebhookPayload; const cardId = payload.action.data.card?.id; diff --git a/src/triggers/trello/label-added.ts b/src/triggers/trello/label-added.ts index 27e4afbf..f9465ac4 100644 --- a/src/triggers/trello/label-added.ts +++ b/src/triggers/trello/label-added.ts @@ -36,17 +36,13 @@ export class ReadyToProcessLabelTrigger implements TriggerHandler { ); } - resolveAgentType(): string | null { - // Cannot determine agent type without fetching the card to get its current list ID - return null; - } - async handle(ctx: TriggerContext): Promise { const payload = ctx.payload as TrelloWebhookPayload; const cardId = payload.action.data.card?.id; if (!cardId) { - throw new Error('No card ID in payload'); + logger.warn('No card ID in Trello label-added payload'); + return null; } // Fetch card to get current list ID (webhook payload doesn't include it for addLabelToCard) diff --git a/src/triggers/trello/webhook-handler.ts b/src/triggers/trello/webhook-handler.ts index 9b3f2b36..1e64b18a 100644 --- a/src/triggers/trello/webhook-handler.ts +++ b/src/triggers/trello/webhook-handler.ts @@ -7,13 +7,15 @@ import { pmRegistry } from '../../pm/index.js'; import { processPMWebhook } from '../../pm/webhook-handler.js'; +import type { TriggerResult } from '../../types/index.js'; import type { TriggerRegistry } from '../registry.js'; export async function processTrelloWebhook( payload: unknown, registry: TriggerRegistry, ackCommentId?: string, + triggerResult?: TriggerResult, ): Promise { const integration = pmRegistry.get('trello'); - await processPMWebhook(integration, payload, registry, ackCommentId); + await processPMWebhook(integration, payload, registry, ackCommentId, triggerResult); } diff --git a/src/types/index.ts b/src/types/index.ts index 59ef22e8..ddb58c6d 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -74,18 +74,13 @@ export interface TriggerResult { agentInput: AgentInput; workItemId?: string; prNumber?: number; + /** When true, the worker must poll for all CI checks to pass before starting the agent. */ + waitForChecks?: boolean; } export interface TriggerHandler { name: string; description: string; matches: (ctx: TriggerContext) => boolean; - /** - * Resolve the agent type this trigger would run, without side effects. - * Returns null when agent type cannot be determined without API calls - * (e.g. Trello label-added needs card lookup) or when the trigger - * doesn't run an agent (e.g. PR merged). - */ - resolveAgentType: (ctx: TriggerContext) => string | null; handle: (ctx: TriggerContext) => Promise; } diff --git a/src/worker-entry.ts b/src/worker-entry.ts index 17da73ff..cfa1e1f2 100644 --- a/src/worker-entry.ts +++ b/src/worker-entry.ts @@ -25,6 +25,7 @@ import { registerBuiltInTriggers, } from './triggers/index.js'; import { processTrelloWebhook } from './triggers/trello/webhook-handler.js'; +import type { TriggerResult } from './types/index.js'; import { scrubSensitiveEnv } from './utils/envScrub.js'; import { logger, setLogLevel } from './utils/index.js'; @@ -37,6 +38,7 @@ interface TrelloJobData { actionType: string; receivedAt: string; ackCommentId?: string; + triggerResult?: TriggerResult; } interface GitHubJobData { @@ -48,6 +50,7 @@ interface GitHubJobData { receivedAt: string; ackCommentId?: number; ackMessage?: string; + triggerResult?: TriggerResult; } interface JiraJobData { @@ -59,6 +62,7 @@ interface JiraJobData { webhookEvent: string; receivedAt: string; ackCommentId?: string; + triggerResult?: TriggerResult; } interface ManualRunJobData { @@ -147,8 +151,14 @@ async function dispatchJob( cardId: jobData.cardId, actionType: jobData.actionType, ackCommentId: jobData.ackCommentId, + hasTriggerResult: !!jobData.triggerResult, }); - await processTrelloWebhook(jobData.payload, triggerRegistry, jobData.ackCommentId); + await processTrelloWebhook( + jobData.payload, + triggerRegistry, + jobData.ackCommentId, + jobData.triggerResult, + ); break; case 'github': logger.info('[Worker] Processing GitHub job', { @@ -156,6 +166,7 @@ async function dispatchJob( eventType: jobData.eventType, repoFullName: jobData.repoFullName, ackCommentId: jobData.ackCommentId, + hasTriggerResult: !!jobData.triggerResult, }); await processGitHubWebhook( jobData.payload, @@ -163,6 +174,7 @@ async function dispatchJob( triggerRegistry, jobData.ackCommentId, jobData.ackMessage, + jobData.triggerResult, ); break; case 'jira': @@ -171,8 +183,14 @@ async function dispatchJob( issueKey: jobData.issueKey, webhookEvent: jobData.webhookEvent, ackCommentId: jobData.ackCommentId, + hasTriggerResult: !!jobData.triggerResult, }); - await processJiraWebhook(jobData.payload, triggerRegistry, jobData.ackCommentId); + await processJiraWebhook( + jobData.payload, + triggerRegistry, + jobData.ackCommentId, + jobData.triggerResult, + ); break; case 'manual-run': case 'retry-run': diff --git a/tests/unit/config/triggerConfig.test.ts b/tests/unit/config/triggerConfig.test.ts index 78993637..f1f1cb76 100644 --- a/tests/unit/config/triggerConfig.test.ts +++ b/tests/unit/config/triggerConfig.test.ts @@ -261,6 +261,21 @@ describe('resolveReadyToProcessEnabled', () => { }; expect(resolveReadyToProcessEnabled(config, 'unknown-agent')).toBe(true); }); + + it('defaults to true for known non-toggle agents like respond-to-review', () => { + const config = { + readyToProcessLabel: { briefing: false, planning: false, implementation: false }, + }; + expect(resolveReadyToProcessEnabled(config, 'respond-to-review')).toBe(true); + expect(resolveReadyToProcessEnabled(config, 'debug')).toBe(true); + }); + + it('defaults all agents to true when nested object is empty (Zod fills defaults)', () => { + const parsed = TrelloTriggerConfigSchema.parse({ readyToProcessLabel: {} }); + expect(resolveReadyToProcessEnabled(parsed, 'briefing')).toBe(true); + expect(resolveReadyToProcessEnabled(parsed, 'planning')).toBe(true); + expect(resolveReadyToProcessEnabled(parsed, 'implementation')).toBe(true); + }); }); describe('resolveIssueTransitionedEnabled', () => { @@ -303,6 +318,21 @@ describe('resolveIssueTransitionedEnabled', () => { }; expect(resolveIssueTransitionedEnabled(config, 'unknown-agent')).toBe(true); }); + + it('defaults to true for known non-toggle agents like respond-to-review', () => { + const config = { + issueTransitioned: { briefing: false, planning: false, implementation: false }, + }; + expect(resolveIssueTransitionedEnabled(config, 'respond-to-review')).toBe(true); + expect(resolveIssueTransitionedEnabled(config, 'debug')).toBe(true); + }); + + it('defaults all agents to true when nested object is empty (Zod fills defaults)', () => { + const parsed = JiraTriggerConfigSchema.parse({ issueTransitioned: {} }); + expect(resolveIssueTransitionedEnabled(parsed, 'briefing')).toBe(true); + expect(resolveIssueTransitionedEnabled(parsed, 'planning')).toBe(true); + expect(resolveIssueTransitionedEnabled(parsed, 'implementation')).toBe(true); + }); }); describe('resolveReviewTriggerConfig', () => { diff --git a/tests/unit/router/github.test.ts b/tests/unit/router/github.test.ts index f81012da..615923d8 100644 --- a/tests/unit/router/github.test.ts +++ b/tests/unit/router/github.test.ts @@ -1,5 +1,14 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; +vi.mock('../../../src/utils/logging.js', () => ({ + logger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }, +})); + // Mock heavy imports vi.mock('../../../src/router/queue.js', () => ({ addJob: vi.fn(), @@ -24,6 +33,9 @@ vi.mock('../../../src/router/ackMessageGenerator.js', () => ({ extractGitHubContext: vi.fn().mockReturnValue('PR: Test PR'), generateAckMessage: vi.fn().mockResolvedValue('Starting implementation...'), })); +vi.mock('../../../src/config/projects.js', () => ({ + getProjectGitHubToken: vi.fn().mockResolvedValue('ghp_mock'), +})); vi.mock('../../../src/config/provider.js', () => ({ findProjectByRepo: vi.fn(), })); @@ -31,6 +43,31 @@ vi.mock('../../../src/github/personas.js', () => ({ resolvePersonaIdentities: vi.fn(), isCascadeBot: vi.fn(), })); +vi.mock('../../../src/github/client.js', () => ({ + withGitHubToken: vi.fn().mockImplementation((_t: unknown, fn: () => unknown) => fn()), +})); +vi.mock('../../../src/pm/context.js', () => ({ + withPMProvider: vi.fn().mockImplementation((_p: unknown, fn: () => unknown) => fn()), + withPMCredentials: vi + .fn() + .mockImplementation((_id: unknown, _type: unknown, _get: unknown, fn: () => unknown) => fn()), +})); +vi.mock('../../../src/pm/registry.js', () => ({ + pmRegistry: { + getOrNull: vi.fn().mockReturnValue(null), + createProvider: vi.fn().mockReturnValue({}), + register: vi.fn(), + }, +})); +vi.mock('../../../src/pm/jira/integration.js', () => ({ + JiraIntegration: vi.fn(), +})); +vi.mock('../../../src/pm/trello/integration.js', () => ({ + TrelloIntegration: vi.fn(), +})); +vi.mock('../../../src/sentry.js', () => ({ + captureException: vi.fn(), +})); import { findProjectByRepo } from '../../../src/config/provider.js'; import { isCascadeBot, resolvePersonaIdentities } from '../../../src/github/personas.js'; @@ -42,7 +79,6 @@ import { handleGitHubWebhook, isSelfAuthoredGitHubComment, processGitHubWebhookEvent, - tryPostGitHubAck, } from '../../../src/router/github.js'; import { extractPRNumber } from '../../../src/router/notifications.js'; import { addEyesReactionToPR } from '../../../src/router/pre-actions.js'; @@ -52,7 +88,7 @@ import { sendAcknowledgeReaction } from '../../../src/router/reactions.js'; import type { TriggerRegistry } from '../../../src/triggers/registry.js'; const mockTriggerRegistry = { - matchTrigger: vi.fn(), + dispatch: vi.fn().mockResolvedValue(null), } as unknown as TriggerRegistry; beforeEach(() => { @@ -148,8 +184,34 @@ describe('handleGitHubWebhook', () => { expect(addJob).not.toHaveBeenCalled(); }); - it('processes pull_request events', async () => { - vi.mocked(findProjectByRepo).mockResolvedValue(null); + it('does not queue job when no project found', async () => { + vi.mocked(loadProjectConfig).mockResolvedValue({ + projects: [], + fullProjects: [], + } as never); + + const result = await handleGitHubWebhook( + 'pull_request', + { repository: { full_name: 'owner/repo' }, action: 'opened' }, + mockTriggerRegistry, + ); + + expect(result.shouldProcess).toBe(true); + expect(addJob).not.toHaveBeenCalled(); + }); + + it('queues job when dispatch returns a trigger result', async () => { + vi.mocked(loadProjectConfig).mockResolvedValue({ + projects: [], + fullProjects: [{ id: 'proj-1', repo: 'owner/repo' }], + } as never); + vi.mocked(resolvePersonaIdentities).mockResolvedValue({} as never); + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue({ + agentType: 'implementation', + agentInput: { prNumber: 1 }, + prNumber: 1, + }); + vi.mocked(resolveGitHubTokenForAck).mockResolvedValue(null); vi.mocked(addJob).mockResolvedValue('job-1'); const result = await handleGitHubWebhook( @@ -165,6 +227,7 @@ describe('handleGitHubWebhook', () => { type: 'github', eventType: 'pull_request', repoFullName: 'owner/repo', + triggerResult: expect.objectContaining({ agentType: 'implementation' }), }), ); }); @@ -187,9 +250,13 @@ describe('handleGitHubWebhook', () => { expect(addJob).not.toHaveBeenCalled(); // ...but skipped because self-authored }); - it('processes check_suite events', async () => { - vi.mocked(addJob).mockResolvedValue('job-1'); - vi.mocked(addEyesReactionToPR).mockResolvedValue(undefined); + it('does not queue when dispatch returns no match', async () => { + vi.mocked(loadProjectConfig).mockResolvedValue({ + projects: [], + fullProjects: [{ id: 'proj-1', repo: 'owner/repo' }], + } as never); + vi.mocked(resolvePersonaIdentities).mockResolvedValue({} as never); + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue(null); await handleGitHubWebhook( 'check_suite', @@ -201,9 +268,7 @@ describe('handleGitHubWebhook', () => { mockTriggerRegistry, ); - expect(addJob).toHaveBeenCalledWith( - expect.objectContaining({ type: 'github', eventType: 'check_suite' }), - ); + expect(addJob).not.toHaveBeenCalled(); }); }); @@ -211,8 +276,11 @@ describe('processGitHubWebhookEvent', () => { it('sends ack reaction for comment events', async () => { vi.mocked(findProjectByRepo).mockResolvedValue({ id: 'p1' } as never); vi.mocked(resolvePersonaIdentities).mockResolvedValue({} as never); - vi.mocked(addJob).mockResolvedValue('job-1'); vi.mocked(sendAcknowledgeReaction).mockResolvedValue(undefined); + vi.mocked(loadProjectConfig).mockResolvedValue({ + projects: [], + fullProjects: [{ id: 'p1', repo: 'owner/repo' }], + } as never); await processGitHubWebhookEvent('issue_comment', 'owner/repo', {}, mockTriggerRegistry); @@ -222,9 +290,12 @@ describe('processGitHubWebhookEvent', () => { }); it('does not send ack reaction for non-comment events', async () => { - vi.mocked(addJob).mockResolvedValue('job-1'); + vi.mocked(loadProjectConfig).mockResolvedValue({ + projects: [], + fullProjects: [{ id: 'proj-1', repo: 'owner/repo' }], + } as never); + vi.mocked(resolvePersonaIdentities).mockResolvedValue({} as never); vi.mocked(resolveGitHubTokenForAck).mockResolvedValue(null); - vi.mocked(extractPRNumber).mockReturnValue(null); await processGitHubWebhookEvent('pull_request', 'owner/repo', {}, mockTriggerRegistry); @@ -232,14 +303,15 @@ describe('processGitHubWebhookEvent', () => { }); it('stores ackCommentId and ackMessage on the job when ack succeeds', async () => { - // Setup: make tryPostGitHubAck succeed by mocking its dependencies vi.mocked(loadProjectConfig).mockResolvedValue({ projects: [], fullProjects: [{ id: 'proj-1', repo: 'owner/repo' }], } as never); vi.mocked(resolvePersonaIdentities).mockResolvedValue({} as never); - (mockTriggerRegistry.matchTrigger as ReturnType).mockReturnValue({ + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue({ agentType: 'review', + agentInput: { prNumber: 42 }, + prNumber: 42, }); vi.mocked(generateAckMessage).mockResolvedValue('Looking into the PR now...'); vi.mocked(resolveGitHubTokenForAck).mockResolvedValue({ @@ -262,96 +334,48 @@ describe('processGitHubWebhookEvent', () => { type: 'github', ackCommentId: 12345, ackMessage: 'Looking into the PR now...', + triggerResult: expect.objectContaining({ agentType: 'review' }), }), ); }); - it('leaves ackMessage undefined when ack fails', async () => { - vi.mocked(resolveGitHubTokenForAck).mockResolvedValue(null); - vi.mocked(addJob).mockResolvedValue('job-1'); - - await processGitHubWebhookEvent( - 'pull_request', - 'owner/repo', - { repository: { full_name: 'owner/repo' } }, - mockTriggerRegistry, - ); - - expect(addJob).toHaveBeenCalledWith( - expect.objectContaining({ - type: 'github', - ackCommentId: undefined, - ackMessage: undefined, - }), - ); - }); -}); - -describe('tryPostGitHubAck', () => { - it('returns commentId and message on success', async () => { + it('does not queue job when dispatch returns null', async () => { vi.mocked(loadProjectConfig).mockResolvedValue({ projects: [], fullProjects: [{ id: 'proj-1', repo: 'owner/repo' }], } as never); vi.mocked(resolvePersonaIdentities).mockResolvedValue({} as never); - (mockTriggerRegistry.matchTrigger as ReturnType).mockReturnValue({ - agentType: 'review', - }); - vi.mocked(generateAckMessage).mockResolvedValue('Checking it out...'); - vi.mocked(resolveGitHubTokenForAck).mockResolvedValue({ - token: 'ghp_test', - project: { id: 'proj-1' }, - } as never); - vi.mocked(extractPRNumber).mockReturnValue(5); - vi.mocked(postGitHubAck).mockResolvedValue(777); + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue(null); - const result = await tryPostGitHubAck( - 'pull_request_review', + await processGitHubWebhookEvent( + 'pull_request', 'owner/repo', - {}, + { repository: { full_name: 'owner/repo' } }, mockTriggerRegistry, ); - expect(result).toEqual({ commentId: 777, message: 'Checking it out...' }); - }); - - it('returns undefined when no trigger matches', async () => { - vi.mocked(loadProjectConfig).mockResolvedValue({ - projects: [], - fullProjects: [{ id: 'proj-1', repo: 'owner/repo' }], - } as never); - vi.mocked(resolvePersonaIdentities).mockResolvedValue({} as never); - (mockTriggerRegistry.matchTrigger as ReturnType).mockReturnValue(null); - - const result = await tryPostGitHubAck('pull_request', 'owner/repo', {}, mockTriggerRegistry); - - expect(result).toBeUndefined(); + expect(addJob).not.toHaveBeenCalled(); }); - it('returns undefined when postGitHubAck returns null', async () => { + it('does not queue job for no-agent triggers', async () => { vi.mocked(loadProjectConfig).mockResolvedValue({ projects: [], fullProjects: [{ id: 'proj-1', repo: 'owner/repo' }], } as never); vi.mocked(resolvePersonaIdentities).mockResolvedValue({} as never); - (mockTriggerRegistry.matchTrigger as ReturnType).mockReturnValue({ - agentType: 'review', + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue({ + agentType: null, + agentInput: {}, + prNumber: 42, }); - vi.mocked(generateAckMessage).mockResolvedValue('Msg'); - vi.mocked(resolveGitHubTokenForAck).mockResolvedValue({ - token: 'ghp_test', - project: { id: 'proj-1' }, - } as never); - vi.mocked(extractPRNumber).mockReturnValue(5); - vi.mocked(postGitHubAck).mockResolvedValue(null); - const result = await tryPostGitHubAck( - 'pull_request_review', + await processGitHubWebhookEvent( + 'pull_request', 'owner/repo', - {}, + { repository: { full_name: 'owner/repo' } }, mockTriggerRegistry, ); - expect(result).toBeUndefined(); + expect(addJob).not.toHaveBeenCalled(); }); }); diff --git a/tests/unit/router/jira.test.ts b/tests/unit/router/jira.test.ts index 6e0c6248..80ef8fb0 100644 --- a/tests/unit/router/jira.test.ts +++ b/tests/unit/router/jira.test.ts @@ -1,5 +1,14 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; +vi.mock('../../../src/utils/logging.js', () => ({ + logger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }, +})); + // Mock heavy imports vi.mock('../../../src/router/config.js', () => ({ loadProjectConfig: vi.fn(), @@ -18,6 +27,14 @@ vi.mock('../../../src/router/ackMessageGenerator.js', () => ({ extractJiraContext: vi.fn().mockReturnValue('Issue: Test issue'), generateAckMessage: vi.fn().mockResolvedValue('Starting implementation...'), })); +vi.mock('../../../src/router/platformClients.js', () => ({ + resolveJiraCredentials: vi + .fn() + .mockResolvedValue({ email: 'e@x.com', apiToken: 'tok', baseUrl: 'https://x.atlassian.net' }), +})); +vi.mock('../../../src/jira/client.js', () => ({ + withJiraCredentials: vi.fn().mockImplementation((_c: unknown, fn: () => unknown) => fn()), +})); import { resolveJiraBotAccountId } from '../../../src/router/acknowledgments.js'; import { loadProjectConfig } from '../../../src/router/config.js'; @@ -25,7 +42,7 @@ import type { RouterProjectConfig } from '../../../src/router/config.js'; import { handleJiraWebhook, isSelfAuthoredJiraComment, - queueJiraJob, + processJiraWebhookEvent, } from '../../../src/router/jira.js'; import { addJob } from '../../../src/router/queue.js'; import { sendAcknowledgeReaction } from '../../../src/router/reactions.js'; @@ -42,7 +59,7 @@ const mockProject: RouterProjectConfig = { }; const mockTriggerRegistry = { - matchTrigger: vi.fn(), + dispatch: vi.fn().mockResolvedValue(null), } as unknown as TriggerRegistry; beforeEach(() => { @@ -91,15 +108,22 @@ describe('isSelfAuthoredJiraComment', () => { }); }); -describe('queueJiraJob', () => { - it('queues a jira job', async () => { +describe('processJiraWebhookEvent', () => { + const fullProject = { id: 'p1', repo: 'owner/repo' }; + + it('queues a jira job when dispatch matches', async () => { vi.mocked(addJob).mockResolvedValue('job-1'); - await queueJiraJob( + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue({ + agentType: 'implementation', + agentInput: { issueKey: 'MYPROJ-123' }, + }); + + await processJiraWebhookEvent( mockProject, 'MYPROJ-123', 'jira:issue_updated', { issue: { key: 'MYPROJ-123' } }, - [], + [fullProject] as never, mockTriggerRegistry, ); expect(addJob).toHaveBeenCalledWith( @@ -108,20 +132,39 @@ describe('queueJiraJob', () => { projectId: 'p1', issueKey: 'MYPROJ-123', webhookEvent: 'jira:issue_updated', + triggerResult: expect.objectContaining({ agentType: 'implementation' }), }), ); }); + it('does not queue when dispatch returns null', async () => { + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue(null); + + await processJiraWebhookEvent( + mockProject, + 'MYPROJ-123', + 'jira:issue_updated', + { issue: { key: 'MYPROJ-123' } }, + [fullProject] as never, + mockTriggerRegistry, + ); + expect(addJob).not.toHaveBeenCalled(); + }); + it('sends ack reaction for comment events', async () => { vi.mocked(addJob).mockResolvedValue('job-1'); vi.mocked(sendAcknowledgeReaction).mockResolvedValue(undefined); + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue({ + agentType: 'implementation', + agentInput: {}, + }); - await queueJiraJob( + await processJiraWebhookEvent( mockProject, 'MYPROJ-123', 'comment_created', { comment: {} }, - [], + [fullProject] as never, mockTriggerRegistry, ); @@ -132,12 +175,17 @@ describe('queueJiraJob', () => { it('does not send reaction for non-comment events', async () => { vi.mocked(addJob).mockResolvedValue('job-1'); - await queueJiraJob( + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue({ + agentType: 'implementation', + agentInput: {}, + }); + + await processJiraWebhookEvent( mockProject, 'MYPROJ-123', 'jira:issue_updated', {}, - [], + [fullProject] as never, mockTriggerRegistry, ); expect(sendAcknowledgeReaction).not.toHaveBeenCalled(); @@ -177,12 +225,16 @@ describe('handleJiraWebhook', () => { expect(addJob).not.toHaveBeenCalled(); }); - it('processes jira:issue_updated events for known projects', async () => { + it('processes jira:issue_updated events for known projects when dispatch matches', async () => { vi.mocked(loadProjectConfig).mockResolvedValue({ projects: [mockProject], - fullProjects: [], - }); + fullProjects: [{ id: 'p1' }], + } as never); vi.mocked(addJob).mockResolvedValue('job-1'); + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue({ + agentType: 'implementation', + agentInput: {}, + }); const result = await handleJiraWebhook( { @@ -194,7 +246,12 @@ describe('handleJiraWebhook', () => { expect(result.shouldProcess).toBe(true); expect(addJob).toHaveBeenCalledWith( - expect.objectContaining({ type: 'jira', projectId: 'p1', issueKey: 'MYPROJ-1' }), + expect.objectContaining({ + type: 'jira', + projectId: 'p1', + issueKey: 'MYPROJ-1', + triggerResult: expect.objectContaining({ agentType: 'implementation' }), + }), ); }); diff --git a/tests/unit/router/trello.test.ts b/tests/unit/router/trello.test.ts index bde90102..b38caba7 100644 --- a/tests/unit/router/trello.test.ts +++ b/tests/unit/router/trello.test.ts @@ -1,5 +1,14 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; +vi.mock('../../../src/utils/logging.js', () => ({ + logger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }, +})); + // Mock heavy imports vi.mock('../../../src/router/config.js', () => ({ loadProjectConfig: vi.fn(), @@ -18,6 +27,12 @@ vi.mock('../../../src/router/ackMessageGenerator.js', () => ({ extractTrelloContext: vi.fn().mockReturnValue('Card: Test card'), generateAckMessage: vi.fn().mockResolvedValue('Starting implementation...'), })); +vi.mock('../../../src/router/platformClients.js', () => ({ + resolveTrelloCredentials: vi.fn().mockResolvedValue({ apiKey: 'key', token: 'tok' }), +})); +vi.mock('../../../src/trello/client.js', () => ({ + withTrelloCredentials: vi.fn().mockImplementation((_creds: unknown, fn: () => unknown) => fn()), +})); import { postTrelloAck, resolveTrelloBotMemberId } from '../../../src/router/acknowledgments.js'; import { loadProjectConfig } from '../../../src/router/config.js'; @@ -53,7 +68,7 @@ const mockProject: RouterProjectConfig = { }; const mockTriggerRegistry = { - matchTrigger: vi.fn(), + dispatch: vi.fn().mockResolvedValue(null), } as unknown as TriggerRegistry; beforeEach(() => { @@ -66,10 +81,19 @@ describe('isAgentLogFilename', () => { expect(isAgentLogFilename('briefing-timeout-2026-01-02T12-34-56-789Z.zip')).toBe(true); }); + it('matches multi-hyphen agent names (e.g. respond-to-review)', () => { + expect(isAgentLogFilename('respond-to-review-2026-01-02T16-30-24-339Z.zip')).toBe(true); + expect(isAgentLogFilename('respond-to-pr-comment-2026-01-02T16-30-24-339Z.zip')).toBe(true); + }); + it('does not match non-zip filenames', () => { expect(isAgentLogFilename('screenshot.png')).toBe(false); }); + it('does not match filenames without a timestamp', () => { + expect(isAgentLogFilename('implementation.zip')).toBe(false); + }); + it('matches debug-prefixed filenames (caller filters separately)', () => { expect(isAgentLogFilename('debug-2026-01-02T16-30-24-339Z.zip')).toBe(true); }); @@ -249,9 +273,36 @@ describe('processTrelloWebhookEvent', () => { expect(addJob).not.toHaveBeenCalled(); }); - it('queues a job for non-self-authored comment', async () => { + it('does not queue a job when dispatch returns null', async () => { vi.mocked(resolveTrelloBotMemberId).mockResolvedValue('bot-id'); + vi.mocked(loadProjectConfig).mockResolvedValue({ + projects: [mockProject], + fullProjects: [{ id: 'p1' }], + } as never); + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue(null); + + await processTrelloWebhookEvent( + mockProject, + 'card1', + 'commentCard', + { action: { idMemberCreator: 'user-id' } }, + mockTriggerRegistry, + ); + expect(addJob).not.toHaveBeenCalled(); + }); + + it('queues a job when dispatch returns a result', async () => { + vi.mocked(resolveTrelloBotMemberId).mockResolvedValue('bot-id'); + vi.mocked(loadProjectConfig).mockResolvedValue({ + projects: [mockProject], + fullProjects: [{ id: 'p1' }], + } as never); vi.mocked(addJob).mockResolvedValue('job-1'); + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue({ + agentType: 'implementation', + agentInput: { cardId: 'card1' }, + }); + await processTrelloWebhookEvent( mockProject, 'card1', @@ -265,14 +316,24 @@ describe('processTrelloWebhookEvent', () => { projectId: 'p1', cardId: 'card1', actionType: 'commentCard', + triggerResult: expect.objectContaining({ agentType: 'implementation' }), }), ); }); it('sends ack reaction for comment actions', async () => { vi.mocked(resolveTrelloBotMemberId).mockResolvedValue('bot-id'); + vi.mocked(loadProjectConfig).mockResolvedValue({ + projects: [mockProject], + fullProjects: [{ id: 'p1' }], + } as never); vi.mocked(addJob).mockResolvedValue('job-1'); vi.mocked(sendAcknowledgeReaction).mockResolvedValue(undefined); + (mockTriggerRegistry.dispatch as ReturnType).mockResolvedValue({ + agentType: 'implementation', + agentInput: { cardId: 'card1' }, + }); + await processTrelloWebhookEvent( mockProject, 'card1', diff --git a/tests/unit/triggers/card-moved.test.ts b/tests/unit/triggers/card-moved.test.ts index 8af247b4..58297b7f 100644 --- a/tests/unit/triggers/card-moved.test.ts +++ b/tests/unit/triggers/card-moved.test.ts @@ -1,5 +1,14 @@ import { describe, expect, it, vi } from 'vitest'; +vi.mock('../../../src/utils/logging.js', () => ({ + logger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }, +})); + // Mocks required for PM integration registration (pm/index.js side-effect) vi.mock('../../../src/config/provider.js', () => ({ getIntegrationCredential: vi.fn(), @@ -157,16 +166,6 @@ describe('CardMovedToBriefingTrigger', () => { expect(trigger.matches(ctx)).toBe(false); }); - it('resolveAgentType returns briefing', () => { - const ctx: TriggerContext = { - project: mockProject, - source: 'trello', - payload: {}, - }; - - expect(trigger.resolveAgentType(ctx)).toBe('briefing'); - }); - it('handles and returns briefing agent', async () => { const ctx: TriggerContext = { project: mockProject, @@ -189,9 +188,9 @@ describe('CardMovedToBriefingTrigger', () => { const result = await trigger.handle(ctx); - expect(result.agentType).toBe('briefing'); - expect(result.workItemId).toBe('card123'); - expect(result.agentInput.cardId).toBe('card123'); + expect(result?.agentType).toBe('briefing'); + expect(result?.workItemId).toBe('card123'); + expect(result?.agentInput.cardId).toBe('card123'); }); }); @@ -238,17 +237,33 @@ describe('CardMovedToTodoTrigger', () => { expect(trigger.matches(ctx)).toBe(true); }); - it('resolveAgentType returns implementation', () => { + it('handles and returns implementation agent', async () => { const ctx: TriggerContext = { project: mockProject, source: 'trello', - payload: {}, + payload: { + model: { id: 'board123', name: 'Board' }, + action: { + id: 'action1', + idMemberCreator: 'member1', + type: 'updateCard', + date: '2024-01-01', + data: { + card: { id: 'card456', name: 'Test Card', idShort: 1, shortLink: 'abc' }, + listBefore: { id: 'planning-list-id', name: 'Planning' }, + listAfter: { id: 'todo-list-id', name: 'TODO' }, + }, + }, + }, }; - expect(trigger.resolveAgentType(ctx)).toBe('implementation'); + const result = await trigger.handle(ctx); + + expect(result?.agentType).toBe('implementation'); + expect(result?.workItemId).toBe('card456'); }); - it('handles and returns implementation agent', async () => { + it('returns null when card ID is missing from payload', async () => { const ctx: TriggerContext = { project: mockProject, source: 'trello', @@ -260,7 +275,7 @@ describe('CardMovedToTodoTrigger', () => { type: 'updateCard', date: '2024-01-01', data: { - card: { id: 'card456', name: 'Test Card', idShort: 1, shortLink: 'abc' }, + // No card field listBefore: { id: 'planning-list-id', name: 'Planning' }, listAfter: { id: 'todo-list-id', name: 'TODO' }, }, @@ -269,8 +284,6 @@ describe('CardMovedToTodoTrigger', () => { }; const result = await trigger.handle(ctx); - - expect(result.agentType).toBe('implementation'); - expect(result.workItemId).toBe('card456'); + expect(result).toBeNull(); }); }); diff --git a/tests/unit/triggers/check-suite-failure.test.ts b/tests/unit/triggers/check-suite-failure.test.ts index 90c686ba..e1b15ee7 100644 --- a/tests/unit/triggers/check-suite-failure.test.ts +++ b/tests/unit/triggers/check-suite-failure.test.ts @@ -60,17 +60,6 @@ describe('CheckSuiteFailureTrigger', () => { vi.mocked(lookupWorkItemForPR).mockResolvedValue(null); }); - describe('resolveAgentType', () => { - it('returns respond-to-ci', () => { - const ctx: TriggerContext = { - project: mockProject, - source: 'github', - payload: {}, - }; - expect(trigger.resolveAgentType(ctx)).toBe('respond-to-ci'); - }); - }); - describe('matches', () => { it('matches completed check suite with failure conclusion and PRs', () => { const ctx: TriggerContext = { diff --git a/tests/unit/triggers/check-suite-success.test.ts b/tests/unit/triggers/check-suite-success.test.ts index d8c977db..adb31956 100644 --- a/tests/unit/triggers/check-suite-success.test.ts +++ b/tests/unit/triggers/check-suite-success.test.ts @@ -62,17 +62,6 @@ describe('CheckSuiteSuccessTrigger', () => { vi.mocked(lookupWorkItemForPR).mockResolvedValue(null); }); - describe('resolveAgentType', () => { - it('returns review', () => { - const ctx: TriggerContext = { - project: mockProject, - source: 'github', - payload: {}, - }; - expect(trigger.resolveAgentType(ctx)).toBe('review'); - }); - }); - describe('matches', () => { it('matches completed check suite with success conclusion and PRs', () => { const ctx: TriggerContext = { @@ -150,7 +139,7 @@ describe('CheckSuiteSuccessTrigger', () => { }); describe('handle', () => { - it('returns review result when PR has Trello URL and all checks pass', async () => { + it('returns review result with waitForChecks flag when PR matches', async () => { vi.mocked(githubClient.getPR).mockResolvedValue({ number: 42, title: 'Test PR', @@ -164,14 +153,6 @@ describe('CheckSuiteSuccessTrigger', () => { user: { login: 'cascade-impl' }, }); vi.mocked(githubClient.getPRReviews).mockResolvedValue([]); - vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ - allPassing: true, - totalCount: 2, - checkRuns: [ - { name: 'lint', status: 'completed', conclusion: 'success' }, - { name: 'test', status: 'completed', conclusion: 'success' }, - ], - }); const ctx: TriggerContext = { project: mockProject, @@ -183,7 +164,8 @@ describe('CheckSuiteSuccessTrigger', () => { const result = await trigger.handle(ctx); expect(githubClient.getPR).toHaveBeenCalledWith('owner', 'repo', 42); - expect(githubClient.getCheckSuiteStatus).toHaveBeenCalledWith('owner', 'repo', 'sha123'); + // handle() no longer polls checks — it defers to worker via waitForChecks flag + expect(githubClient.getCheckSuiteStatus).not.toHaveBeenCalled(); expect(result).toEqual({ agentType: 'review', agentInput: { @@ -196,6 +178,7 @@ describe('CheckSuiteSuccessTrigger', () => { }, prNumber: 42, workItemId: 'abc123', + waitForChecks: true, }); }); @@ -280,153 +263,6 @@ describe('CheckSuiteSuccessTrigger', () => { expect(githubClient.getCheckSuiteStatus).not.toHaveBeenCalled(); }); - it('returns null immediately when checks have genuine failures (no retry)', async () => { - vi.mocked(githubClient.getPR).mockResolvedValue({ - number: 42, - title: 'Test PR', - body: 'https://trello.com/c/abc123', - state: 'open', - headRef: 'feature/test', - headSha: 'sha123', - baseRef: 'main', - merged: false, - htmlUrl: 'https://github.com/owner/repo/pull/42', - user: { login: 'cascade-impl' }, - }); - vi.mocked(githubClient.getPRReviews).mockResolvedValue([]); - vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ - allPassing: false, - totalCount: 2, - checkRuns: [ - { name: 'lint', status: 'completed', conclusion: 'success' }, - { name: 'test', status: 'completed', conclusion: 'failure' }, - ], - }); - - const ctx: TriggerContext = { - project: mockProject, - source: 'github', - payload: makeCheckSuitePayload(), - personaIdentities: mockPersonaIdentities, - }; - - const result = await trigger.handle(ctx); - - expect(result).toBeNull(); - // Should NOT retry — all checks completed, one genuinely failed - expect(githubClient.getCheckSuiteStatus).toHaveBeenCalledTimes(1); - }); - - it('retries when checks are still in-progress, then succeeds', async () => { - vi.useFakeTimers(); - - vi.mocked(githubClient.getPR).mockResolvedValue({ - number: 42, - title: 'Test PR', - body: 'https://trello.com/c/abc123', - state: 'open', - headRef: 'feature/test', - headSha: 'sha123', - baseRef: 'main', - merged: false, - htmlUrl: 'https://github.com/owner/repo/pull/42', - user: { login: 'cascade-impl' }, - }); - vi.mocked(githubClient.getPRReviews).mockResolvedValue([]); - vi.mocked(githubClient.getCheckSuiteStatus) - .mockResolvedValueOnce({ - allPassing: false, - totalCount: 2, - checkRuns: [ - { name: 'lint', status: 'completed', conclusion: 'success' }, - { name: 'test', status: 'in_progress', conclusion: null }, - ], - }) - .mockResolvedValueOnce({ - allPassing: true, - totalCount: 2, - checkRuns: [ - { name: 'lint', status: 'completed', conclusion: 'success' }, - { name: 'test', status: 'completed', conclusion: 'success' }, - ], - }); - - const ctx: TriggerContext = { - project: mockProject, - source: 'github', - payload: makeCheckSuitePayload(), - personaIdentities: mockPersonaIdentities, - }; - - const handlePromise = trigger.handle(ctx); - - // Advance past the retry delay - await vi.advanceTimersByTimeAsync(10_000); - - const result = await handlePromise; - - expect(result).not.toBeNull(); - expect(result?.agentType).toBe('review'); - expect(githubClient.getCheckSuiteStatus).toHaveBeenCalledTimes(2); - - vi.useRealTimers(); - }); - - it('retries when checks are in-progress but eventually all fail', async () => { - vi.useFakeTimers(); - - vi.mocked(githubClient.getPR).mockResolvedValue({ - number: 42, - title: 'Test PR', - body: 'https://trello.com/c/abc123', - state: 'open', - headRef: 'feature/test', - headSha: 'sha123', - baseRef: 'main', - merged: false, - htmlUrl: 'https://github.com/owner/repo/pull/42', - user: { login: 'cascade-impl' }, - }); - vi.mocked(githubClient.getPRReviews).mockResolvedValue([]); - vi.mocked(githubClient.getCheckSuiteStatus) - .mockResolvedValueOnce({ - allPassing: false, - totalCount: 2, - checkRuns: [ - { name: 'lint', status: 'completed', conclusion: 'success' }, - { name: 'test', status: 'in_progress', conclusion: null }, - ], - }) - .mockResolvedValueOnce({ - allPassing: false, - totalCount: 2, - checkRuns: [ - { name: 'lint', status: 'completed', conclusion: 'success' }, - { name: 'test', status: 'completed', conclusion: 'failure' }, - ], - }); - - const ctx: TriggerContext = { - project: mockProject, - source: 'github', - payload: makeCheckSuitePayload(), - personaIdentities: mockPersonaIdentities, - }; - - const handlePromise = trigger.handle(ctx); - - // Advance past the retry delay - await vi.advanceTimersByTimeAsync(10_000); - - const result = await handlePromise; - - expect(result).toBeNull(); - // 1 initial + 1 retry (then stops because all completed) - expect(githubClient.getCheckSuiteStatus).toHaveBeenCalledTimes(2); - - vi.useRealTimers(); - }); - it('returns null when PR was already reviewed by reviewer persona at current HEAD', async () => { vi.mocked(githubClient.getPR).mockResolvedValue({ number: 42, @@ -487,11 +323,6 @@ describe('CheckSuiteSuccessTrigger', () => { commitId: 'old-sha', }, ]); - vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ - allPassing: true, - totalCount: 1, - checkRuns: [{ name: 'test', status: 'completed', conclusion: 'success' }], - }); const ctx: TriggerContext = { project: mockProject, @@ -504,7 +335,7 @@ describe('CheckSuiteSuccessTrigger', () => { expect(result).not.toBeNull(); expect(result?.agentType).toBe('review'); - expect(githubClient.getCheckSuiteStatus).toHaveBeenCalled(); + expect(result?.waitForChecks).toBe(true); }); it('skips when latest of multiple reviews covers current HEAD', async () => { @@ -591,11 +422,6 @@ describe('CheckSuiteSuccessTrigger', () => { commitId: 'sha123', }, ]); - vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ - allPassing: true, - totalCount: 1, - checkRuns: [{ name: 'test', status: 'completed', conclusion: 'success' }], - }); const ctx: TriggerContext = { project: mockProject, @@ -608,7 +434,7 @@ describe('CheckSuiteSuccessTrigger', () => { expect(result).not.toBeNull(); expect(result?.agentType).toBe('review'); - expect(githubClient.getCheckSuiteStatus).toHaveBeenCalled(); + expect(result?.waitForChecks).toBe(true); }); it('proceeds when PR has reviews from other users only', async () => { @@ -634,11 +460,6 @@ describe('CheckSuiteSuccessTrigger', () => { commitId: 'sha123', }, ]); - vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ - allPassing: true, - totalCount: 1, - checkRuns: [{ name: 'test', status: 'completed', conclusion: 'success' }], - }); const ctx: TriggerContext = { project: mockProject, @@ -651,6 +472,7 @@ describe('CheckSuiteSuccessTrigger', () => { expect(result).not.toBeNull(); expect(result?.agentType).toBe('review'); + expect(result?.waitForChecks).toBe(true); }); it('fires without work item when PR body has no work item reference', async () => { @@ -667,11 +489,6 @@ describe('CheckSuiteSuccessTrigger', () => { user: { login: 'cascade-impl' }, }); vi.mocked(githubClient.getPRReviews).mockResolvedValue([]); - vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ - allPassing: true, - totalCount: 1, - checkRuns: [{ name: 'test', status: 'completed', conclusion: 'success' }], - }); const ctx: TriggerContext = { project: mockProject, @@ -685,6 +502,7 @@ describe('CheckSuiteSuccessTrigger', () => { expect(result).not.toBeNull(); expect(result?.workItemId).toBeUndefined(); expect(result?.agentInput.cardId).toBeUndefined(); + expect(result?.waitForChecks).toBe(true); }); it('uses DB lookup result over PR body extraction', async () => { @@ -702,11 +520,6 @@ describe('CheckSuiteSuccessTrigger', () => { user: { login: 'cascade-impl' }, }); vi.mocked(githubClient.getPRReviews).mockResolvedValue([]); - vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ - allPassing: true, - totalCount: 1, - checkRuns: [{ name: 'test', status: 'completed', conclusion: 'success' }], - }); const ctx: TriggerContext = { project: mockProject, @@ -719,6 +532,7 @@ describe('CheckSuiteSuccessTrigger', () => { expect(result).not.toBeNull(); expect(result?.workItemId).toBe('db-work-item'); + expect(result?.waitForChecks).toBe(true); }); }); @@ -794,11 +608,6 @@ describe('CheckSuiteSuccessTrigger', () => { user: { login: 'external-contributor' }, }); vi.mocked(githubClient.getPRReviews).mockResolvedValue([]); - vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ - allPassing: true, - totalCount: 1, - checkRuns: [{ name: 'test', status: 'completed', conclusion: 'success' }], - }); const ctx: TriggerContext = { project: mockProjectExternalOnly, @@ -854,11 +663,6 @@ describe('CheckSuiteSuccessTrigger', () => { user: { login: authorLogin }, }); vi.mocked(githubClient.getPRReviews).mockResolvedValue([]); - vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ - allPassing: true, - totalCount: 1, - checkRuns: [{ name: 'test', status: 'completed', conclusion: 'success' }], - }); }; // Implementer PR @@ -900,11 +704,6 @@ describe('CheckSuiteSuccessTrigger', () => { user: { login: 'cascade-impl' }, }); vi.mocked(githubClient.getPRReviews).mockResolvedValue([]); - vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ - allPassing: true, - totalCount: 1, - checkRuns: [{ name: 'test', status: 'completed', conclusion: 'success' }], - }); // mockProject has no github triggers — resolves to legacy defaults (ownPrsOnly=true) const ctx: TriggerContext = { diff --git a/tests/unit/triggers/github-pr-comment-mention.test.ts b/tests/unit/triggers/github-pr-comment-mention.test.ts index d04dd523..3ca5dc66 100644 --- a/tests/unit/triggers/github-pr-comment-mention.test.ts +++ b/tests/unit/triggers/github-pr-comment-mention.test.ts @@ -162,13 +162,6 @@ describe('PRCommentMentionTrigger', () => { }); }); - describe('resolveAgentType', () => { - it('returns respond-to-pr-comment', () => { - const ctx = buildCtx(); - expect(trigger.resolveAgentType(ctx)).toBe('respond-to-pr-comment'); - }); - }); - describe('matches', () => { it('matches issue_comment.created on a PR', () => { expect(trigger.matches(buildCtx())).toBe(true); diff --git a/tests/unit/triggers/jira-comment-mention.test.ts b/tests/unit/triggers/jira-comment-mention.test.ts index 7af32e37..3356762c 100644 --- a/tests/unit/triggers/jira-comment-mention.test.ts +++ b/tests/unit/triggers/jira-comment-mention.test.ts @@ -124,13 +124,6 @@ describe('JiraCommentMentionTrigger', () => { vi.restoreAllMocks(); }); - describe('resolveAgentType', () => { - it('returns respond-to-planning-comment', () => { - const ctx = buildCtx(); - expect(trigger.resolveAgentType(ctx)).toBe('respond-to-planning-comment'); - }); - }); - describe('matches', () => { it('matches comment_created from jira source', () => { expect(trigger.matches(buildCtx({ webhookEvent: 'comment_created' }))).toBe(true); diff --git a/tests/unit/triggers/jira-issue-transitioned.test.ts b/tests/unit/triggers/jira-issue-transitioned.test.ts index dc962132..c0e95d0a 100644 --- a/tests/unit/triggers/jira-issue-transitioned.test.ts +++ b/tests/unit/triggers/jira-issue-transitioned.test.ts @@ -104,55 +104,6 @@ describe('JiraIssueTransitionedTrigger', () => { }); }); - describe('resolveAgentType', () => { - it('returns implementation for "To Do" transition', () => { - const ctx = buildCtx({ - statusChangeItems: [{ field: 'status', fromString: 'Planning', toString: 'To Do' }], - }); - expect(trigger.resolveAgentType(ctx)).toBe('implementation'); - }); - - it('returns briefing for "Briefing" transition', () => { - const ctx = buildCtx({ - statusChangeItems: [{ field: 'status', fromString: 'Backlog', toString: 'Briefing' }], - }); - expect(trigger.resolveAgentType(ctx)).toBe('briefing'); - }); - - it('returns planning for "Planning" transition', () => { - const ctx = buildCtx({ - statusChangeItems: [{ field: 'status', fromString: 'Briefing', toString: 'Planning' }], - }); - expect(trigger.resolveAgentType(ctx)).toBe('planning'); - }); - - it('returns null for unmapped status', () => { - const ctx = buildCtx({ - statusChangeItems: [{ field: 'status', fromString: 'To Do', toString: 'Done' }], - }); - expect(trigger.resolveAgentType(ctx)).toBeNull(); - }); - - it('returns null when no status change in changelog', () => { - const ctx = buildCtx({ - statusChangeItems: [{ field: 'assignee' }], - }); - expect(trigger.resolveAgentType(ctx)).toBeNull(); - }); - - it('returns null when JIRA config is missing', () => { - const ctx = buildCtx({ noJiraConfig: true }); - expect(trigger.resolveAgentType(ctx)).toBeNull(); - }); - - it('is case insensitive', () => { - const ctx = buildCtx({ - statusChangeItems: [{ field: 'status', fromString: 'Backlog', toString: 'briefing' }], - }); - expect(trigger.resolveAgentType(ctx)).toBe('briefing'); - }); - }); - describe('handle', () => { it('returns implementation agent for "To Do" transition', async () => { const ctx = buildCtx({ diff --git a/tests/unit/triggers/jira-label-added.test.ts b/tests/unit/triggers/jira-label-added.test.ts index 31691c8e..0abe529b 100644 --- a/tests/unit/triggers/jira-label-added.test.ts +++ b/tests/unit/triggers/jira-label-added.test.ts @@ -94,34 +94,6 @@ function buildCtx(overrides: { } describe('JiraReadyToProcessLabelTrigger', () => { - describe('resolveAgentType()', () => { - it('returns briefing when issue is in Briefing status', () => { - expect(trigger.resolveAgentType(buildCtx({ statusName: 'Briefing' }))).toBe('briefing'); - }); - - it('returns planning when issue is in Planning status', () => { - expect(trigger.resolveAgentType(buildCtx({ statusName: 'Planning' }))).toBe('planning'); - }); - - it('returns implementation when issue is in To Do status', () => { - expect(trigger.resolveAgentType(buildCtx({ statusName: 'To Do' }))).toBe('implementation'); - }); - - it('returns null when issue is in unmapped status', () => { - expect(trigger.resolveAgentType(buildCtx({ statusName: 'Done' }))).toBeNull(); - }); - - it('returns null when issue has no status', () => { - const ctx = buildCtx({ statusName: undefined }); - // Override payload to have no status field - const payload = ctx.payload as Record; - const issue = payload.issue as Record; - const fields = issue.fields as Record; - fields.status = undefined; - expect(trigger.resolveAgentType(ctx)).toBeNull(); - }); - }); - describe('matches()', () => { it('matches when cascade-ready label is added', () => { expect(trigger.matches(buildCtx({}))).toBe(true); diff --git a/tests/unit/triggers/label-added.test.ts b/tests/unit/triggers/label-added.test.ts index 3a1e1578..3c9b83c6 100644 --- a/tests/unit/triggers/label-added.test.ts +++ b/tests/unit/triggers/label-added.test.ts @@ -138,30 +138,6 @@ describe('ReadyToProcessLabelTrigger', () => { }); }); - describe('resolveAgentType', () => { - it('returns null (requires API call to determine list)', () => { - const ctx: TriggerContext = { - project: mockProject, - source: 'trello', - payload: { - model: { id: 'board123', name: 'Board' }, - action: { - id: 'action1', - idMemberCreator: 'member1', - type: 'addLabelToCard', - date: '2024-01-01', - data: { - card: { id: 'card1', name: 'Test Card', idShort: 1, shortLink: 'abc' }, - label: { id: 'ready-label-id', name: 'Ready', color: 'green' }, - }, - }, - }, - }; - - expect(trigger.resolveAgentType(ctx)).toBeNull(); - }); - }); - describe('handle', () => { it('returns briefing agent when card is in briefing list', async () => { mockGetCard.mockResolvedValue({ @@ -303,7 +279,7 @@ describe('ReadyToProcessLabelTrigger', () => { expect(result.agentType).toBe('briefing'); }); - it('throws when card ID is missing', async () => { + it('returns null when card ID is missing', async () => { const ctx: TriggerContext = { project: mockProject, source: 'trello', @@ -321,7 +297,8 @@ describe('ReadyToProcessLabelTrigger', () => { }, }; - await expect(trigger.handle(ctx)).rejects.toThrow('No card ID in payload'); + const result = await trigger.handle(ctx); + expect(result).toBeNull(); }); }); }); diff --git a/tests/unit/triggers/pr-merged.test.ts b/tests/unit/triggers/pr-merged.test.ts index 1fa0ff82..6337f232 100644 --- a/tests/unit/triggers/pr-merged.test.ts +++ b/tests/unit/triggers/pr-merged.test.ts @@ -82,17 +82,6 @@ describe('PRMergedTrigger', () => { vi.mocked(lookupWorkItemForPR).mockResolvedValue(null); }); - describe('resolveAgentType', () => { - it('returns null (no agent)', () => { - const ctx: TriggerContext = { - project: mockProject, - source: 'github', - payload: {}, - }; - expect(trigger.resolveAgentType(ctx)).toBeNull(); - }); - }); - describe('matches', () => { it('matches when PR is closed', () => { const ctx: TriggerContext = { diff --git a/tests/unit/triggers/pr-opened.test.ts b/tests/unit/triggers/pr-opened.test.ts index b44471cc..eb4e9229 100644 --- a/tests/unit/triggers/pr-opened.test.ts +++ b/tests/unit/triggers/pr-opened.test.ts @@ -169,17 +169,6 @@ describe('PROpenedTrigger', () => { }); }); - describe('resolveAgentType', () => { - it('returns respond-to-review', () => { - const ctx: TriggerContext = { - project: mockProject, - source: 'github', - payload: {}, - }; - expect(trigger.resolveAgentType(ctx)).toBe('respond-to-review'); - }); - }); - describe('handle', () => { it('returns result when PR body has Trello URL', async () => { const ctx: TriggerContext = { diff --git a/tests/unit/triggers/pr-ready-to-merge.test.ts b/tests/unit/triggers/pr-ready-to-merge.test.ts index 403ed431..dc5bf582 100644 --- a/tests/unit/triggers/pr-ready-to-merge.test.ts +++ b/tests/unit/triggers/pr-ready-to-merge.test.ts @@ -83,17 +83,6 @@ describe('PRReadyToMergeTrigger', () => { vi.mocked(lookupWorkItemForPR).mockResolvedValue(null); }); - describe('resolveAgentType', () => { - it('returns null (no agent)', () => { - const ctx: TriggerContext = { - project: mockProject, - source: 'github', - payload: {}, - }; - expect(trigger.resolveAgentType(ctx)).toBeNull(); - }); - }); - describe('matches', () => { it('matches check_suite completed with success and PRs', () => { const ctx: TriggerContext = { diff --git a/tests/unit/triggers/pr-review-submitted.test.ts b/tests/unit/triggers/pr-review-submitted.test.ts index c0291091..3b3ab7c7 100644 --- a/tests/unit/triggers/pr-review-submitted.test.ts +++ b/tests/unit/triggers/pr-review-submitted.test.ts @@ -137,17 +137,6 @@ describe('PRReviewSubmittedTrigger', () => { }); }); - describe('resolveAgentType', () => { - it('returns respond-to-review', () => { - const ctx: TriggerContext = { - project: mockProject, - source: 'github', - payload: {}, - }; - expect(trigger.resolveAgentType(ctx)).toBe('respond-to-review'); - }); - }); - describe('handle', () => { it('returns respond-to-review result when reviewer persona posts changes_requested', async () => { const ctx: TriggerContext = { diff --git a/tests/unit/triggers/registry.test.ts b/tests/unit/triggers/registry.test.ts index 04194812..da43cc05 100644 --- a/tests/unit/triggers/registry.test.ts +++ b/tests/unit/triggers/registry.test.ts @@ -23,7 +23,6 @@ describe('TriggerRegistry', () => { name: 'test-handler', description: 'Test handler', matches: (ctx) => ctx.source === 'trello', - resolveAgentType: () => 'briefing', handle: vi.fn().mockResolvedValue({ agentType: 'briefing', agentInput: { cardId: 'card123' }, @@ -52,7 +51,6 @@ describe('TriggerRegistry', () => { name: 'trello-only', description: 'Only matches trello', matches: (ctx) => ctx.source === 'trello', - resolveAgentType: () => 'briefing', handle: vi.fn(), }; @@ -77,7 +75,6 @@ describe('TriggerRegistry', () => { name: 'to-remove', description: 'Will be removed', matches: () => true, - resolveAgentType: () => 'briefing', handle: vi.fn(), }; @@ -88,113 +85,29 @@ describe('TriggerRegistry', () => { expect(removed).toBe(true); expect(registry.getHandlers()).toHaveLength(0); }); -}); - -describe('TriggerRegistry.matchTrigger', () => { - const mockProject = { - id: 'test', - name: 'Test', - repo: 'owner/repo', - baseBranch: 'main', - branchPrefix: 'feature/', - trello: { - boardId: 'board123', - lists: { todo: 'list1' }, - labels: {}, - }, - }; - - it('returns name and agentType for the first matching handler', () => { - const registry = createTriggerRegistry(); - - const handler: TriggerHandler = { - name: 'card-moved-to-todo', - description: 'Card moved to TODO', - matches: (ctx) => ctx.source === 'trello', - resolveAgentType: () => 'implementation', - handle: vi.fn(), - }; - - registry.register(handler); - - const ctx: TriggerContext = { - project: mockProject, - source: 'trello', - payload: {}, - }; - - const result = registry.matchTrigger(ctx); - - expect(result).toEqual({ name: 'card-moved-to-todo', agentType: 'implementation' }); - }); - it('returns null when no handler matches', () => { + it('calls first matching handler and returns its result', async () => { const registry = createTriggerRegistry(); - const handler: TriggerHandler = { - name: 'trello-only', - description: 'Only matches trello', - matches: (ctx) => ctx.source === 'trello', - resolveAgentType: () => 'briefing', - handle: vi.fn(), - }; - - registry.register(handler); - - const ctx: TriggerContext = { - project: mockProject, - source: 'github', - payload: {}, - }; - - expect(registry.matchTrigger(ctx)).toBeNull(); - }); - - it('skips handlers that match but return null from resolveAgentType', () => { - const registry = createTriggerRegistry(); - - const nullHandler: TriggerHandler = { - name: 'label-added', - description: 'Label added (cannot resolve)', - matches: () => true, - resolveAgentType: () => null, - handle: vi.fn(), - }; - - const validHandler: TriggerHandler = { - name: 'card-moved', - description: 'Card moved', + const handler1: TriggerHandler = { + name: 'first', + description: 'First', matches: () => true, - resolveAgentType: () => 'planning', - handle: vi.fn(), - }; - - registry.register(nullHandler); - registry.register(validHandler); - - const ctx: TriggerContext = { - project: mockProject, - source: 'trello', - payload: {}, + handle: vi.fn().mockResolvedValue({ + agentType: 'briefing', + agentInput: {}, + }), }; - const result = registry.matchTrigger(ctx); - - expect(result).toEqual({ name: 'card-moved', agentType: 'planning' }); - }); - - it('returns null when all matching handlers return null from resolveAgentType', () => { - const registry = createTriggerRegistry(); - - const handler: TriggerHandler = { - name: 'label-added', - description: 'Label added (cannot resolve)', + const handler2: TriggerHandler = { + name: 'second', + description: 'Second', matches: () => true, - resolveAgentType: () => null, handle: vi.fn(), }; - registry.register(handler); + registry.register(handler1); + registry.register(handler2); const ctx: TriggerContext = { project: mockProject, @@ -202,51 +115,31 @@ describe('TriggerRegistry.matchTrigger', () => { payload: {}, }; - expect(registry.matchTrigger(ctx)).toBeNull(); - }); - - it('does not call handle() — pure matching only', () => { - const registry = createTriggerRegistry(); - - const handleMock = vi.fn(); - const handler: TriggerHandler = { - name: 'test', - description: 'Test', - matches: () => true, - resolveAgentType: () => 'review', - handle: handleMock, - }; - - registry.register(handler); - - const ctx: TriggerContext = { - project: mockProject, - source: 'github', - payload: {}, - }; - - registry.matchTrigger(ctx); + const result = await registry.dispatch(ctx); - expect(handleMock).not.toHaveBeenCalled(); + expect(result?.agentType).toBe('briefing'); + expect(handler1.handle).toHaveBeenCalledWith(ctx); + expect(handler2.handle).not.toHaveBeenCalled(); }); - it('returns first match when multiple handlers match', () => { + it('skips handler when handle() returns null and tries next', async () => { const registry = createTriggerRegistry(); const handler1: TriggerHandler = { - name: 'first', - description: 'First', + name: 'no-match', + description: 'Returns null from handle', matches: () => true, - resolveAgentType: () => 'briefing', - handle: vi.fn(), + handle: vi.fn().mockResolvedValue(null), }; const handler2: TriggerHandler = { - name: 'second', - description: 'Second', + name: 'real-match', + description: 'Returns a result', matches: () => true, - resolveAgentType: () => 'planning', - handle: vi.fn(), + handle: vi.fn().mockResolvedValue({ + agentType: 'planning', + agentInput: {}, + }), }; registry.register(handler1); @@ -258,8 +151,10 @@ describe('TriggerRegistry.matchTrigger', () => { payload: {}, }; - const result = registry.matchTrigger(ctx); + const result = await registry.dispatch(ctx); - expect(result).toEqual({ name: 'first', agentType: 'briefing' }); + expect(result?.agentType).toBe('planning'); + expect(handler1.handle).toHaveBeenCalled(); + expect(handler2.handle).toHaveBeenCalled(); }); }); diff --git a/tests/unit/triggers/review-requested.test.ts b/tests/unit/triggers/review-requested.test.ts index 2dcbb6e3..6539ca05 100644 --- a/tests/unit/triggers/review-requested.test.ts +++ b/tests/unit/triggers/review-requested.test.ts @@ -75,17 +75,6 @@ describe('ReviewRequestedTrigger', () => { sender: { login: 'author' }, }); - describe('resolveAgentType', () => { - it('returns review', () => { - const ctx: TriggerContext = { - project: mockProject, - source: 'github', - payload: {}, - }; - expect(trigger.resolveAgentType(ctx)).toBe('review'); - }); - }); - describe('matches', () => { it('does not match by default (opt-in trigger, disabled without config)', () => { const ctx: TriggerContext = { diff --git a/tests/unit/triggers/trello-comment-mention.test.ts b/tests/unit/triggers/trello-comment-mention.test.ts index b8a8fb66..1261b223 100644 --- a/tests/unit/triggers/trello-comment-mention.test.ts +++ b/tests/unit/triggers/trello-comment-mention.test.ts @@ -132,13 +132,6 @@ describe('TrelloCommentMentionTrigger', () => { }); }); - describe('resolveAgentType', () => { - it('returns respond-to-planning-comment', () => { - const ctx = buildCtx(); - expect(trigger.resolveAgentType(ctx)).toBe('respond-to-planning-comment'); - }); - }); - describe('handle', () => { it('returns respond-to-planning-comment result when @mention is present on PLANNING card', async () => { const result = await trigger.handle(buildCtx());