From bfce387494a656b09eedb68fa45553d280db2b9b Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Fri, 3 Apr 2026 17:06:38 +0000 Subject: [PATCH 1/3] refactor(triggers): extract shared webhook utilities and add Sentry concurrency --- src/triggers/README.md | 131 ++++++++++++++++ src/triggers/github/webhook-handler.ts | 99 ++++-------- src/triggers/sentry/webhook-handler.ts | 41 +++-- src/triggers/shared/concurrency.ts | 64 ++++++++ src/triggers/shared/credential-scope.ts | 37 +++++ src/triggers/shared/pm-ack.ts | 47 ++++++ src/triggers/shared/trigger-resolution.ts | 48 ++++++ .../triggers/github-webhook-handler.test.ts | 46 ++---- .../triggers/sentry-webhook-handler.test.ts | 101 ++++++++---- .../unit/triggers/shared/concurrency.test.ts | 145 ++++++++++++++++++ .../triggers/shared/credential-scope.test.ts | 122 +++++++++++++++ .../shared/trigger-resolution.test.ts | 130 ++++++++++++++++ 12 files changed, 868 insertions(+), 143 deletions(-) create mode 100644 src/triggers/README.md create mode 100644 src/triggers/shared/concurrency.ts create mode 100644 src/triggers/shared/credential-scope.ts create mode 100644 src/triggers/shared/pm-ack.ts create mode 100644 src/triggers/shared/trigger-resolution.ts create mode 100644 tests/unit/triggers/shared/concurrency.test.ts create mode 100644 tests/unit/triggers/shared/credential-scope.test.ts create mode 100644 tests/unit/triggers/shared/trigger-resolution.test.ts diff --git a/src/triggers/README.md b/src/triggers/README.md new file mode 100644 index 00000000..8d691c55 --- /dev/null +++ b/src/triggers/README.md @@ -0,0 +1,131 @@ +# Trigger System + +This directory contains the trigger handlers and registry that route webhook events to agents. + +## Architecture Overview + +``` +Webhook → Router → Redis/BullMQ → Worker → TriggerRegistry → Agent +``` + +### Two-tier webhook handling + +Webhook processing is split into two distinct tiers: + +| Tier | Where | Purpose | +|------|-------|---------| +| **Router** | `src/router/` | Receive, validate, acknowledge, enqueue | +| **Worker** | `src/triggers/` | Resolve trigger, establish credentials, run agent | + +**Router side is fully unified** — all four providers (Trello, JIRA, GitHub, Sentry) share `processRouterWebhook()` + `RouterPlatformAdapter`. No provider-specific branching in the router. + +**Worker side has intentional divergence** — see below. + +--- + +## Worker-Side Handler Comparison + +| Feature | PM (`processPMWebhook`) | GitHub (`processGitHubWebhook`) | Sentry (`processSentryWebhook`) | +|---------|------------------------|--------------------------------|--------------------------------| +| Trigger dispatch | ✅ Registry | ✅ Registry or pre-resolved | ✅ Registry or pre-resolved | +| Ack comment (PR) | ❌ N/A | ✅ Posts to PR | ❌ N/A | +| Ack comment (PM) | ✅ Via PM lifecycle | ✅ For PM-focused agents | ❌ N/A | +| CI check polling | ❌ N/A | ✅ `pollWaitForChecks()` | ❌ N/A | +| PM credential scope | ✅ `integration.withCredentials` | ✅ `withPMCredentials` | ✅ `withPMCredentials` | +| PM lifecycle ops | ✅ prepareForAgent / handleFailure | ✅ For PM-focused agents | ❌ Skipped | +| Persona token mgmt | ❌ N/A | ✅ Implementer / reviewer | ❌ N/A | +| Agent concurrency | ✅ `checkAgentTypeConcurrency` | ✅ `checkAgentTypeConcurrency` | ✅ `withAgentTypeConcurrency` | + +--- + +## Why GitHub and Sentry Cannot Use `processPMWebhook()` + +`processPMWebhook()` assumes **PM semantics**: +- It calls `integration.parseWebhookPayload()` expecting a PM event (card ID, board identifier) +- It drives `PMLifecycleManager` (prepareForAgent → handleFailure / handleSuccess) +- The `PMIntegration` interface provides card parsing, ack cleanup, and credential scoping + +Forcing GitHub or Sentry into this pipeline would require: +- Provider-specific `if` branches inside `processPMWebhook()` — worse than current design +- Mocking PM lifecycle ops (they don't apply to Sentry alerts or GitHub PRs) + +### GitHub-specific features (cannot be generalized) + +1. **CI check polling** (`pollWaitForChecks`) — GitHub is the only provider with CI. No other source polls build status before running an agent. +2. **PR acknowledgment comments** — GitHub PRs get a comment like "👀 Reviewing…" immediately. No other source has this flow. +3. **Dual-persona token management** — The implementer vs. reviewer persona selection is GitHub-specific. No Trello/JIRA/Sentry equivalent. +4. **PM-focused agent routing** — When a PM-focused agent (e.g. `backlog-manager`) fires from a GitHub PR event, it posts the ack to Trello/JIRA instead of the PR, and uses PM-appropriate lifecycle config. + +### Sentry-specific simplicity (intentional) + +Sentry is an alerting source. There are no: +- Work item cards to manage lifecycle on +- PR comments to post +- CI checks to poll + +Sentry's handler is intentionally minimal: load project, resolve trigger, run agent in PM scope. + +--- + +## Shared Utilities (`src/triggers/shared/`) + +To reduce duplication across the three worker-side handlers, shared utilities are extracted to `src/triggers/shared/`: + +| File | Purpose | Used By | +|------|---------|---------| +| `concurrency.ts` | `withAgentTypeConcurrency()` — wraps check→mark→execute→clear | GitHub, PM, Sentry | +| `trigger-resolution.ts` | `resolveTriggerResult()` — pre-resolved or dispatch | GitHub, Sentry (PM has inline logic) | +| `credential-scope.ts` | `withPMScope()` — `withPMCredentials` + `withPMProvider` | GitHub, Sentry | +| `pm-ack.ts` | `postPMAckComment()` — posts ack to Trello/JIRA | Router GitHub adapter, GitHub worker handler | +| `agent-execution.ts` | `runAgentExecutionPipeline()` — full agent lifecycle | All handlers (via `webhook-execution.ts`) | +| `webhook-execution.ts` | `runAgentWithCredentials()` — LLM keys + credentials + pipeline | GitHub, PM | + +--- + +## Flow Diagrams + +### PM webhook (Trello / JIRA) + +``` +processPMWebhook(integration, payload, registry) + └─ integration.parseWebhookPayload(payload) → event + └─ integration.lookupProject(event.identifier) → project + └─ integration.withCredentials(projectId) + └─ withPMProvider(pmProvider) + └─ resolveTriggerResult(registry, ctx, preResolved) + └─ handleMatchedTrigger(...) + └─ withAgentTypeConcurrency(projectId, agentType) + └─ startWatchdog() + └─ executeAgent() → runAgentWithCredentials() + └─ injectLlmApiKeys() + └─ withGitHubToken(personaToken) + └─ runAgentExecutionPipeline(...) +``` + +### GitHub webhook + +``` +processGitHubWebhook(payload, eventType, registry, ackCommentId, triggerResult) + └─ integration.parseWebhookPayload(payload) → event + └─ integration.lookupProject(event.repo) → project + └─ resolveTriggerResult(registry, ctx, preResolved) + └─ [optional] pollWaitForChecks(result, repo) → checksOk + └─ maybePostAckComment(result, ...) → PR or PM ack + └─ runGitHubAgent(result, project, config) + └─ withAgentTypeConcurrency(projectId, agentType) + └─ startWatchdog() + └─ withPMScope(project) + └─ runAgentWithCredentials(integration, result, ...) +``` + +### Sentry webhook + +``` +processSentryWebhook(payload, projectId, registry, triggerResult) + └─ loadProjectConfigById(projectId) → project + └─ resolveTriggerResult(registry, ctx, preResolved) + └─ withAgentTypeConcurrency(projectId, agentType) + └─ startWatchdog() + └─ withPMScope(project) + └─ runAgentExecutionPipeline(result, ...) +``` diff --git a/src/triggers/github/webhook-handler.ts b/src/triggers/github/webhook-handler.ts index 41c79317..4b62b8f0 100644 --- a/src/triggers/github/webhook-handler.ts +++ b/src/triggers/github/webhook-handler.ts @@ -6,26 +6,23 @@ * - CI check polling → ./check-polling.ts * - Credential scoping + agent execution → ../shared/webhook-execution.ts * - GitHub-specific AgentExecutionConfig → ./integration.ts + * - Agent-type concurrency → ../shared/concurrency.ts + * - PM credential scope → ../shared/credential-scope.ts + * - PM ack posting → ../shared/pm-ack.ts */ import { isPMFocusedAgent } from '../../agents/definitions/loader.js'; import { githubClient, withGitHubToken } from '../../github/client.js'; import { getPersonaToken, resolvePersonaIdentities } from '../../github/personas.js'; -import { withPMCredentials, withPMProvider } from '../../pm/context.js'; -import { createPMProvider, pmRegistry } from '../../pm/index.js'; import { extractGitHubContext, generateAckMessage } from '../../router/ackMessageGenerator.js'; -import { postJiraAck, postTrelloAck } from '../../router/acknowledgments.js'; -import { - checkAgentTypeConcurrency, - clearAgentTypeEnqueued, - markAgentTypeEnqueued, - markRecentlyDispatched, -} from '../../router/agent-type-lock.js'; import type { CascadeConfig, ProjectConfig, TriggerContext } from '../../types/index.js'; import { logger, startWatchdog } from '../../utils/index.js'; import { parseRepoFullName } from '../../utils/repo.js'; import { safeOperation } from '../../utils/safeOperation.js'; import type { TriggerRegistry } from '../registry.js'; +import { withAgentTypeConcurrency } from '../shared/concurrency.js'; +import { withPMScope } from '../shared/credential-scope.js'; +import { postPMAckComment } from '../shared/pm-ack.js'; import { runAgentWithCredentials } from '../shared/webhook-execution.js'; import type { TriggerResult } from '../types.js'; import { postAcknowledgmentComment, updateInitialCommentWithError } from './ack-comments.js'; @@ -53,10 +50,6 @@ function requireProjectId(project: ProjectConfig): string { return project.id; } -function isValidPmType(pmType: string | undefined): pmType is 'trello' | 'jira' { - return pmType === 'trello' || pmType === 'jira'; -} - async function maybePostPmAckComment( result: TriggerResult, payload: unknown, @@ -73,18 +66,13 @@ async function maybePostPmAckComment( ); const pmType = project.pm?.type; - if (!isValidPmType(pmType)) { - logger.warn('Unknown PM type for PM-focused agent ack (worker-side)', { - agentType: result.agentType, - pmType, - }); - return; - } - - const commentId = - pmType === 'trello' - ? await postTrelloAck(projectId, workItemId, message) - : await postJiraAck(projectId, workItemId, message); + const commentId = await postPMAckComment( + projectId, + workItemId, + pmType, + message, + result.agentType ?? undefined, + ); if (commentId) { result.agentInput.ackCommentId = commentId; @@ -102,14 +90,7 @@ async function dispatchTrigger( const personaIdentities = await resolvePersonaIdentities(projectId); const githubToken = await getPersonaToken(projectId, 'implementation'); const ctx: TriggerContext = { project, source: 'github', payload, personaIdentities }; - const pmProvider = createPMProvider(project); - return withPMCredentials( - projectId, - project.pm?.type, - (t) => pmRegistry.getOrNull(t), - () => - withPMProvider(pmProvider, () => withGitHubToken(githubToken, () => registry.dispatch(ctx))), - ); + return withPMScope(project, () => withGitHubToken(githubToken, () => registry.dispatch(ctx))); } /** Post ack comment on the PR using the agent-specific persona token. */ @@ -167,43 +148,35 @@ async function runGitHubAgent( project: ProjectConfig, config: CascadeConfig, ): Promise { - // Agent-type concurrency limit - let agentTypeMaxConcurrency: number | null = null; - if (result.agentType) { - const concurrencyCheck = await checkAgentTypeConcurrency(project.id, result.agentType); - agentTypeMaxConcurrency = concurrencyCheck.maxConcurrency; - if (concurrencyCheck.blocked) return; - if (agentTypeMaxConcurrency !== null) { - markRecentlyDispatched(project.id, result.agentType); - markAgentTypeEnqueued(project.id, result.agentType); - } - } - startWatchdog(project.watchdogTimeoutMs); // PM-focused agents (e.g. backlog-manager) triggered from GitHub should use // PM-appropriate lifecycle config: no GitHub PR comment callbacks, allow PM lifecycle ops. const pmFocused = result.agentType ? await isPMFocusedAgent(result.agentType) : false; - try { + const agentType = result.agentType; + + const execute = async () => { // Establish PM credential + provider scope for agents with workItemId // (needed for PM lifecycle operations: labels, status moves, PR links) - const pmProvider = createPMProvider(project); - await withPMCredentials( - project.id, - project.pm?.type, - (t) => pmRegistry.getOrNull(t), - () => - withPMProvider(pmProvider, () => - runAgentWithCredentials( - integration, - result, - project, - config, - resolveGitHubExecutionConfig(pmFocused), - ), - ), + await withPMScope(project, () => + runAgentWithCredentials( + integration, + result, + project, + config, + resolveGitHubExecutionConfig(pmFocused), + ), ); + }; + + // Agent-type concurrency limit wraps the entire execution + try { + if (agentType) { + await withAgentTypeConcurrency(project.id, agentType, execute, 'GitHub agent'); + } else { + await execute(); + } } catch (err) { logger.error('Failed to process GitHub webhook', { error: String(err) }); if (!pmFocused) { @@ -216,10 +189,6 @@ async function runGitHubAgent( updateInitialCommentWithError(result, { success: false, error: String(err) }), ); } - } finally { - if (result.agentType && agentTypeMaxConcurrency !== null) { - clearAgentTypeEnqueued(project.id, result.agentType); - } } } diff --git a/src/triggers/sentry/webhook-handler.ts b/src/triggers/sentry/webhook-handler.ts index 9bb3ec98..946aa1b3 100644 --- a/src/triggers/sentry/webhook-handler.ts +++ b/src/triggers/sentry/webhook-handler.ts @@ -5,15 +5,21 @@ * falling back to dispatching through the trigger registry if not. * After resolving the trigger result, runs the matched agent via the * shared execution pipeline. + * + * Shared utilities used: + * - Trigger resolution → ../shared/trigger-resolution.ts + * - Agent-type concurrency → ../shared/concurrency.ts + * - PM credential scope → ../shared/credential-scope.ts */ -import { withPMCredentials, withPMProvider } from '../../pm/context.js'; -import { createPMProvider, pmRegistry } from '../../pm/index.js'; import type { TriggerResult } from '../../types/index.js'; import { startWatchdog } from '../../utils/lifecycle.js'; import { logger } from '../../utils/logging.js'; import type { TriggerRegistry } from '../registry.js'; import { runAgentExecutionPipeline } from '../shared/agent-execution.js'; +import { withAgentTypeConcurrency } from '../shared/concurrency.js'; +import { withPMScope } from '../shared/credential-scope.js'; +import { resolveTriggerResult } from '../shared/trigger-resolution.js'; export async function processSentryWebhook( payload: unknown, @@ -29,22 +35,14 @@ export async function processSentryWebhook( return; } + const ctx = { + project: pc.project, + source: 'sentry' as const, + payload, + }; + // Resolve trigger result — use pre-computed from router or dispatch via registry - let result: TriggerResult | null; - if (triggerResult) { - logger.info('processSentryWebhook: using pre-computed trigger result', { - projectId, - agentType: triggerResult.agentType, - }); - result = triggerResult; - } else { - const ctx = { - project: pc.project, - source: 'sentry' as const, - payload, - }; - result = await registry.dispatch(ctx); - } + const result = await resolveTriggerResult(registry, ctx, triggerResult, 'processSentryWebhook'); if (!result) { logger.info('processSentryWebhook: no trigger matched', { projectId }); @@ -65,18 +63,17 @@ export async function processSentryWebhook( startWatchdog(pc.project.watchdogTimeoutMs); - const pmProvider = createPMProvider(pc.project); - await withPMCredentials( + await withAgentTypeConcurrency( pc.project.id, - pc.project.pm?.type, - (t) => pmRegistry.getOrNull(t), + result.agentType, () => - withPMProvider(pmProvider, () => + withPMScope(pc.project, () => runAgentExecutionPipeline(result, pc.project, pc.config, { logLabel: 'Sentry agent', skipPrepareForAgent: true, skipHandleFailure: true, }), ), + 'processSentryWebhook', ); } diff --git a/src/triggers/shared/concurrency.ts b/src/triggers/shared/concurrency.ts new file mode 100644 index 00000000..b2414f69 --- /dev/null +++ b/src/triggers/shared/concurrency.ts @@ -0,0 +1,64 @@ +/** + * Shared concurrency management utility for webhook handlers. + * + * Wraps the duplicated check→mark→execute→clear pattern used by both + * `handleMatchedTrigger()` (pm/webhook-handler) and `runGitHubAgent()` + * (github/webhook-handler) into a single reusable function. + * + * Usage: + * await withAgentTypeConcurrency(projectId, agentType, () => runTheAgent()); + */ + +import { + checkAgentTypeConcurrency, + clearAgentTypeEnqueued, + markAgentTypeEnqueued, + markRecentlyDispatched, +} from '../../router/agent-type-lock.js'; +import { logger } from '../../utils/logging.js'; + +/** + * Execute `fn` within agent-type concurrency limits. + * + * 1. Checks whether the agent-type is at its concurrency limit. + * 2. If not blocked, marks the slot as enqueued and runs `fn`. + * 3. Clears the enqueued slot in a `finally` block. + * + * Returns `false` if the concurrency check was blocked (fn was not called), + * `true` if fn was called (regardless of whether it succeeded). + * + * @param projectId The project ID to scope concurrency to. + * @param agentType The agent type being dispatched. + * @param fn The async function to run if not blocked. + * @param logLabel Optional label for log messages (default: 'Agent'). + */ +export async function withAgentTypeConcurrency( + projectId: string, + agentType: string, + fn: () => Promise, + logLabel?: string, +): Promise { + const concurrencyCheck = await checkAgentTypeConcurrency(projectId, agentType, logLabel); + if (concurrencyCheck.blocked) { + logger.info(`${logLabel ?? 'Agent'} type concurrency blocked, skipping`, { + projectId, + agentType, + }); + return false; + } + + const hasLimit = concurrencyCheck.maxConcurrency !== null; + if (hasLimit) { + markRecentlyDispatched(projectId, agentType); + markAgentTypeEnqueued(projectId, agentType); + } + + try { + await fn(); + return true; + } finally { + if (hasLimit) { + clearAgentTypeEnqueued(projectId, agentType); + } + } +} diff --git a/src/triggers/shared/credential-scope.ts b/src/triggers/shared/credential-scope.ts new file mode 100644 index 00000000..db78bb81 --- /dev/null +++ b/src/triggers/shared/credential-scope.ts @@ -0,0 +1,37 @@ +/** + * Shared PM credential scoping utility for webhook handlers. + * + * Wraps the `withPMCredentials(…, () => withPMProvider(…, fn))` pattern + * used by GitHub (webhook-handler.ts L190-206) and Sentry (webhook-handler.ts L68-81) + * into a single reusable function. + * + * Usage: + * await withPMScope(project, () => runTheAgent()); + */ + +import { withPMCredentials, withPMProvider } from '../../pm/context.js'; +import { createPMProvider, pmRegistry } from '../../pm/index.js'; +import type { ProjectConfig } from '../../types/index.js'; + +/** + * Execute `fn` within the PM credential and PM provider scope for a project. + * + * Sets up: + * withPMCredentials → withPMProvider → fn() + * + * This is the standard PM scope needed for agents that perform PM lifecycle + * operations (labels, status moves, PR links). Falls through gracefully if + * no PM type is configured on the project. + * + * @param project Project config (used to determine PM type and credentials). + * @param fn Async function to run inside the PM scope. + */ +export async function withPMScope(project: ProjectConfig, fn: () => Promise): Promise { + const pmProvider = createPMProvider(project); + return withPMCredentials( + project.id, + project.pm?.type, + (t) => pmRegistry.getOrNull(t), + () => withPMProvider(pmProvider, fn), + ); +} diff --git a/src/triggers/shared/pm-ack.ts b/src/triggers/shared/pm-ack.ts new file mode 100644 index 00000000..216b1d7c --- /dev/null +++ b/src/triggers/shared/pm-ack.ts @@ -0,0 +1,47 @@ +/** + * Shared PM acknowledgment posting utility for webhook handlers. + * + * Centralises the logic for posting acknowledgment comments to PM tools + * (Trello/JIRA) for PM-focused agents triggered from GitHub or other + * non-PM sources. This is shared between: + * + * - Router-side: `router/adapters/github.ts` (postPMAck) + * - Worker-side: `triggers/github/webhook-handler.ts` (maybePostPmAckComment) + */ + +import { postJiraAck, postTrelloAck } from '../../router/acknowledgments.js'; +import { logger } from '../../utils/logging.js'; + +/** + * Post a PM acknowledgment comment to Trello or JIRA. + * + * Returns the comment ID if successfully posted, or null if the PM type + * is not supported or posting failed. + * + * @param projectId The project ID for credential resolution. + * @param workItemId The work item ID to post the comment on (card ID / issue key). + * @param pmType The PM provider type ('trello' or 'jira'). + * @param message The acknowledgment message to post. + * @param agentType Used only for warning log context when pmType is unknown. + */ +export async function postPMAckComment( + projectId: string, + workItemId: string, + pmType: string | undefined, + message: string, + agentType?: string, +): Promise { + if (pmType === 'trello') { + return postTrelloAck(projectId, workItemId, message); + } + + if (pmType === 'jira') { + return postJiraAck(projectId, workItemId, message); + } + + logger.warn('Unknown PM type for PM-focused agent ack, skipping', { + agentType, + pmType, + }); + return null; +} diff --git a/src/triggers/shared/trigger-resolution.ts b/src/triggers/shared/trigger-resolution.ts new file mode 100644 index 00000000..07ec264e --- /dev/null +++ b/src/triggers/shared/trigger-resolution.ts @@ -0,0 +1,48 @@ +/** + * Shared trigger resolution utility for webhook handlers. + * + * Extracts the "if pre-resolved result use it, else dispatch" pattern + * duplicated across GitHub (webhook-handler.ts L253-261), + * Sentry (webhook-handler.ts L33-47), and PM (webhook-handler.ts L64-87). + * + * Usage: + * const result = await resolveTriggerResult(registry, ctx, preResolvedResult); + */ + +import type { TriggerContext, TriggerResult } from '../../types/index.js'; +import { logger } from '../../utils/logging.js'; +import type { TriggerRegistry } from '../registry.js'; + +/** + * Resolve a trigger result from either a pre-computed result or registry dispatch. + * + * If `preResolvedResult` is provided, it is returned immediately (skipping dispatch). + * Otherwise, `registry.dispatch(ctx)` is called to compute the result. + * + * @param registry Trigger registry to dispatch against (when no pre-resolved result). + * @param ctx Trigger context passed to registry dispatch. + * @param preResolvedResult Optional pre-computed result from the router (skips dispatch). + * @param logLabel Optional label for log messages (default: uses ctx.source). + * @returns The resolved TriggerResult, or null if no trigger matched. + */ +export async function resolveTriggerResult( + registry: TriggerRegistry, + ctx: TriggerContext, + preResolvedResult?: TriggerResult, + logLabel?: string, +): Promise { + const label = logLabel ?? ctx.source; + + if (preResolvedResult) { + logger.info(`${label}: using pre-resolved trigger result`, { + agentType: preResolvedResult.agentType, + }); + return preResolvedResult; + } + + const result = await registry.dispatch(ctx); + if (!result) { + logger.info(`${label}: no trigger matched`); + } + return result; +} diff --git a/tests/unit/triggers/github-webhook-handler.test.ts b/tests/unit/triggers/github-webhook-handler.test.ts index 3cfecf4b..32651de4 100644 --- a/tests/unit/triggers/github-webhook-handler.test.ts +++ b/tests/unit/triggers/github-webhook-handler.test.ts @@ -59,23 +59,21 @@ vi.mock('../../../src/router/ackMessageGenerator.js', () => ({ generateAckMessage: vi.fn().mockResolvedValue('Starting...'), })); -vi.mock('../../../src/router/acknowledgments.js', () => ({ - postTrelloAck: vi.fn().mockResolvedValue('comment-id'), - postJiraAck: vi.fn().mockResolvedValue('comment-id'), -})); - vi.mock('../../../src/utils/safeOperation.js', () => ({ safeOperation: vi.fn().mockImplementation((fn) => fn()), })); -vi.mock('../../../src/pm/context.js', () => ({ - withPMCredentials: vi.fn().mockImplementation((_id, _type, _get, fn) => fn()), - withPMProvider: vi.fn().mockImplementation((_provider, fn) => fn()), +// Mock shared utilities used by processGitHubWebhook +vi.mock('../../../src/triggers/shared/concurrency.js', () => ({ + withAgentTypeConcurrency: vi.fn().mockImplementation((_id, _type, fn) => fn()), +})); + +vi.mock('../../../src/triggers/shared/credential-scope.js', () => ({ + withPMScope: vi.fn().mockImplementation((_project, fn) => fn()), })); -vi.mock('../../../src/pm/index.js', () => ({ - createPMProvider: vi.fn().mockReturnValue({}), - pmRegistry: { getOrNull: vi.fn().mockReturnValue(null) }, +vi.mock('../../../src/triggers/shared/pm-ack.js', () => ({ + postPMAckComment: vi.fn().mockResolvedValue('pm-comment-id'), })); vi.mock('../../../src/triggers/shared/webhook-execution.js', () => ({ @@ -91,13 +89,6 @@ vi.mock('../../../src/triggers/github/check-polling.js', () => ({ pollWaitForChecks: vi.fn().mockResolvedValue(true), })); -vi.mock('../../../src/router/agent-type-lock.js', () => ({ - checkAgentTypeConcurrency: vi.fn().mockResolvedValue({ maxConcurrency: null, blocked: false }), - markAgentTypeEnqueued: vi.fn(), - clearAgentTypeEnqueued: vi.fn(), - markRecentlyDispatched: vi.fn(), -})); - vi.mock('../../../src/utils/index.js', () => ({ logger: { debug: vi.fn(), @@ -110,14 +101,14 @@ vi.mock('../../../src/utils/index.js', () => ({ import { isPMFocusedAgent } from '../../../src/agents/definitions/loader.js'; import { githubClient } from '../../../src/github/client.js'; -import { postJiraAck, postTrelloAck } from '../../../src/router/acknowledgments.js'; -import { checkAgentTypeConcurrency } from '../../../src/router/agent-type-lock.js'; import { postAcknowledgmentComment, updateInitialCommentWithError, } from '../../../src/triggers/github/ack-comments.js'; import { pollWaitForChecks } from '../../../src/triggers/github/check-polling.js'; import { processGitHubWebhook } from '../../../src/triggers/github/webhook-handler.js'; +import { withAgentTypeConcurrency } from '../../../src/triggers/shared/concurrency.js'; +import { postPMAckComment } from '../../../src/triggers/shared/pm-ack.js'; import { runAgentWithCredentials } from '../../../src/triggers/shared/webhook-execution.js'; import { startWatchdog } from '../../../src/utils/index.js'; @@ -144,6 +135,7 @@ const validPayload = { beforeEach(() => { mockRunAgentWithCredentials.mockResolvedValue(undefined); + vi.mocked(withAgentTypeConcurrency).mockImplementation((_id, _type, fn) => fn()); }); describe('processGitHubWebhook', () => { @@ -235,10 +227,7 @@ describe('processGitHubWebhook', () => { }); it('skips agent execution when agent-type concurrency is blocked', async () => { - vi.mocked(checkAgentTypeConcurrency).mockResolvedValueOnce({ - maxConcurrency: 1, - blocked: true, - }); + vi.mocked(withAgentTypeConcurrency).mockResolvedValueOnce(false); const registry = createMockRegistry(); await processGitHubWebhook(validPayload, 'pull_request', registry as never); expect(mockRunAgentWithCredentials).not.toHaveBeenCalled(); @@ -322,9 +311,9 @@ describe('processGitHubWebhook', () => { expect(mockRunAgentWithCredentials).not.toHaveBeenCalled(); }); - it('posts PM ack to Trello when PM-focused agent triggered from GitHub (trello PM)', async () => { + it('posts PM ack to PM tool when PM-focused agent triggered from GitHub', async () => { vi.mocked(isPMFocusedAgent).mockResolvedValue(true); - vi.mocked(postTrelloAck).mockResolvedValue('trello-ack-id'); + vi.mocked(postPMAckComment).mockResolvedValue('pm-ack-id'); // Override lookupProject to return a project with trello PM const { GitHubWebhookIntegration } = await import( @@ -354,7 +343,7 @@ describe('processGitHubWebhook', () => { await processGitHubWebhook(validPayload, 'pull_request', registry as never); - // PM ack should be posted to Trello (or attempt was made); GitHub PR comment not used + // PM ack should be posted to PM tool; GitHub PR comment not used expect(postAcknowledgmentComment).not.toHaveBeenCalled(); }); @@ -371,8 +360,7 @@ describe('processGitHubWebhook', () => { await processGitHubWebhook(validPayload, 'pull_request', registry as never); - expect(postTrelloAck).not.toHaveBeenCalled(); - expect(postJiraAck).not.toHaveBeenCalled(); + expect(postPMAckComment).not.toHaveBeenCalled(); expect(postAcknowledgmentComment).not.toHaveBeenCalled(); }); diff --git a/tests/unit/triggers/sentry-webhook-handler.test.ts b/tests/unit/triggers/sentry-webhook-handler.test.ts index 1fb9d121..e7c91c22 100644 --- a/tests/unit/triggers/sentry-webhook-handler.test.ts +++ b/tests/unit/triggers/sentry-webhook-handler.test.ts @@ -7,16 +7,6 @@ vi.mock('../../../src/config/provider.js', () => ({ loadProjectConfigById: vi.fn(), })); -vi.mock('../../../src/pm/context.js', () => ({ - withPMCredentials: vi.fn().mockImplementation((_id, _type, _getter, fn) => fn()), - withPMProvider: vi.fn().mockImplementation((_provider, fn) => fn()), -})); - -vi.mock('../../../src/pm/index.js', () => ({ - createPMProvider: vi.fn().mockReturnValue({}), - pmRegistry: { getOrNull: vi.fn().mockReturnValue(null) }, -})); - vi.mock('../../../src/utils/lifecycle.js', () => ({ startWatchdog: vi.fn(), })); @@ -25,10 +15,25 @@ vi.mock('../../../src/triggers/shared/agent-execution.js', () => ({ runAgentExecutionPipeline: vi.fn().mockResolvedValue(undefined), })); +// Mock shared utilities used by processSentryWebhook +vi.mock('../../../src/triggers/shared/concurrency.js', () => ({ + withAgentTypeConcurrency: vi.fn().mockImplementation((_projectId, _agentType, fn) => fn()), +})); + +vi.mock('../../../src/triggers/shared/credential-scope.js', () => ({ + withPMScope: vi.fn().mockImplementation((_project, fn) => fn()), +})); + +vi.mock('../../../src/triggers/shared/trigger-resolution.js', () => ({ + resolveTriggerResult: vi.fn(), +})); + import { loadProjectConfigById } from '../../../src/config/provider.js'; -import { withPMCredentials, withPMProvider } from '../../../src/pm/context.js'; import { processSentryWebhook } from '../../../src/triggers/sentry/webhook-handler.js'; import { runAgentExecutionPipeline } from '../../../src/triggers/shared/agent-execution.js'; +import { withAgentTypeConcurrency } from '../../../src/triggers/shared/concurrency.js'; +import { withPMScope } from '../../../src/triggers/shared/credential-scope.js'; +import { resolveTriggerResult } from '../../../src/triggers/shared/trigger-resolution.js'; import { createMockProject } from '../../helpers/factories.js'; const mockProject = createMockProject({ id: 'proj-sentry' }); @@ -45,22 +50,29 @@ describe('processSentryWebhook', () => { }); vi.mocked(runAgentExecutionPipeline).mockResolvedValue(undefined); // Re-apply pass-through implementations after resetAllMocks clears them - vi.mocked(withPMCredentials).mockImplementation((_id, _type, _getter, fn) => fn()); - vi.mocked(withPMProvider).mockImplementation((_provider, fn) => fn()); + vi.mocked(withAgentTypeConcurrency).mockImplementation((_projectId, _agentType, fn) => + fn().then(() => true), + ); + vi.mocked(withPMScope).mockImplementation((_project, fn) => fn()); + // resolveTriggerResult defaults to null (no trigger matched) + vi.mocked(resolveTriggerResult).mockResolvedValue(null); }); - it('loads project config by projectId and dispatches with sentry source when no triggerResult', async () => { + it('loads project config by projectId and calls resolveTriggerResult with sentry source', async () => { const payload = { resource: 'event_alert', cascadeProjectId: 'proj-sentry' }; await processSentryWebhook(payload, 'proj-sentry', mockRegistry as never, undefined); expect(loadProjectConfigById).toHaveBeenCalledWith('proj-sentry'); - expect(mockRegistry.dispatch).toHaveBeenCalledWith( + expect(resolveTriggerResult).toHaveBeenCalledWith( + mockRegistry, expect.objectContaining({ source: 'sentry', payload, project: mockProject, }), + undefined, + 'processSentryWebhook', ); }); @@ -69,13 +81,14 @@ describe('processSentryWebhook', () => { await processSentryWebhook(payload, 'proj-sentry', mockRegistry as never); - const dispatchCall = mockRegistry.dispatch.mock.calls[0][0]; - expect(dispatchCall.source).toBe('sentry'); - expect(dispatchCall.payload).toBe(payload); - expect(dispatchCall.project).toBe(mockProject); + const resolveCall = vi.mocked(resolveTriggerResult).mock.calls[0]; + const ctx = resolveCall[1]; + expect(ctx.source).toBe('sentry'); + expect(ctx.payload).toBe(payload); + expect(ctx.project).toBe(mockProject); }); - it('logs a warning and returns without dispatching when project is not found', async () => { + it('logs a warning and returns without calling resolveTriggerResult when project is not found', async () => { vi.mocked(loadProjectConfigById).mockResolvedValue(undefined); const payload = { resource: 'event_alert' }; @@ -85,26 +98,33 @@ describe('processSentryWebhook', () => { expect.stringContaining('project not found'), expect.objectContaining({ projectId: 'unknown-proj' }), ); - expect(mockRegistry.dispatch).not.toHaveBeenCalled(); + expect(resolveTriggerResult).not.toHaveBeenCalled(); }); - it('does NOT call registry.dispatch when triggerResult is provided', async () => { + it('passes triggerResult to resolveTriggerResult when provided', async () => { const payload = { resource: 'event_alert', cascadeProjectId: 'proj-sentry' }; const triggerResult = { agentType: 'alerting', agentInput: {} } as never; await processSentryWebhook(payload, 'proj-sentry', mockRegistry as never, triggerResult); - expect(mockRegistry.dispatch).not.toHaveBeenCalled(); + expect(resolveTriggerResult).toHaveBeenCalledWith( + mockRegistry, + expect.any(Object), + triggerResult, + 'processSentryWebhook', + ); }); - it('logs info message when triggerResult is provided', async () => { + it('logs info message when triggerResult is provided (via resolveTriggerResult)', async () => { const payload = { resource: 'event_alert' }; const triggerResult = { agentType: 'alerting', agentInput: {} } as never; + vi.mocked(resolveTriggerResult).mockResolvedValue(triggerResult); await processSentryWebhook(payload, 'proj-sentry', mockRegistry as never, triggerResult); + // processSentryWebhook logs "running agent" when it proceeds after resolution expect(mockLogger.info).toHaveBeenCalledWith( - expect.stringContaining('pre-computed trigger result'), + expect.stringContaining('running agent'), expect.objectContaining({ projectId: 'proj-sentry', agentType: 'alerting' }), ); }); @@ -112,6 +132,7 @@ describe('processSentryWebhook', () => { it('runs the agent execution pipeline when triggerResult has an agentType', async () => { const payload = { resource: 'event_alert', cascadeProjectId: 'proj-sentry' }; const triggerResult = { agentType: 'alerting', agentInput: {} } as never; + vi.mocked(resolveTriggerResult).mockResolvedValue(triggerResult); await processSentryWebhook(payload, 'proj-sentry', mockRegistry as never, triggerResult); @@ -123,12 +144,38 @@ describe('processSentryWebhook', () => { ); }); - it('does not run the agent when registry dispatch returns null', async () => { + it('does not run the agent when resolveTriggerResult returns null', async () => { const payload = { resource: 'event_alert', cascadeProjectId: 'proj-sentry' }; - mockRegistry.dispatch.mockResolvedValue(null); + vi.mocked(resolveTriggerResult).mockResolvedValue(null); await processSentryWebhook(payload, 'proj-sentry', mockRegistry as never); expect(runAgentExecutionPipeline).not.toHaveBeenCalled(); }); + + it('applies agent-type concurrency when running the agent', async () => { + const payload = { resource: 'event_alert' }; + const triggerResult = { agentType: 'alerting', agentInput: {} } as never; + vi.mocked(resolveTriggerResult).mockResolvedValue(triggerResult); + + await processSentryWebhook(payload, 'proj-sentry', mockRegistry as never, triggerResult); + + expect(withAgentTypeConcurrency).toHaveBeenCalledWith( + 'proj-sentry', + 'alerting', + expect.any(Function), + 'processSentryWebhook', + ); + }); + + it('skips execution when concurrency is blocked', async () => { + const payload = { resource: 'event_alert' }; + const triggerResult = { agentType: 'alerting', agentInput: {} } as never; + vi.mocked(resolveTriggerResult).mockResolvedValue(triggerResult); + vi.mocked(withAgentTypeConcurrency).mockResolvedValue(false); + + await processSentryWebhook(payload, 'proj-sentry', mockRegistry as never, triggerResult); + + expect(runAgentExecutionPipeline).not.toHaveBeenCalled(); + }); }); diff --git a/tests/unit/triggers/shared/concurrency.test.ts b/tests/unit/triggers/shared/concurrency.test.ts new file mode 100644 index 00000000..63a095c6 --- /dev/null +++ b/tests/unit/triggers/shared/concurrency.test.ts @@ -0,0 +1,145 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +// --------------------------------------------------------------------------- +// Hoisted mocks — declared before imports so module-level singletons are mocked +// --------------------------------------------------------------------------- + +const { + mockCheckAgentTypeConcurrency, + mockMarkAgentTypeEnqueued, + mockClearAgentTypeEnqueued, + mockMarkRecentlyDispatched, +} = vi.hoisted(() => ({ + mockCheckAgentTypeConcurrency: vi.fn(), + mockMarkAgentTypeEnqueued: vi.fn(), + mockClearAgentTypeEnqueued: vi.fn(), + mockMarkRecentlyDispatched: vi.fn(), +})); + +vi.mock('../../../../src/router/agent-type-lock.js', () => ({ + checkAgentTypeConcurrency: mockCheckAgentTypeConcurrency, + markAgentTypeEnqueued: mockMarkAgentTypeEnqueued, + clearAgentTypeEnqueued: mockClearAgentTypeEnqueued, + markRecentlyDispatched: mockMarkRecentlyDispatched, +})); + +vi.mock('../../../../src/utils/logging.js', () => ({ + logger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }, +})); + +import { withAgentTypeConcurrency } from '../../../../src/triggers/shared/concurrency.js'; + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +const PROJECT_ID = 'project-1'; +const AGENT_TYPE = 'implementation'; + +beforeEach(() => { + vi.clearAllMocks(); + // Default: no limit, not blocked + mockCheckAgentTypeConcurrency.mockResolvedValue({ maxConcurrency: null, blocked: false }); +}); + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe('withAgentTypeConcurrency', () => { + it('calls fn and returns true when not blocked and no limit', async () => { + const fn = vi.fn().mockResolvedValue(undefined); + mockCheckAgentTypeConcurrency.mockResolvedValue({ maxConcurrency: null, blocked: false }); + + const result = await withAgentTypeConcurrency(PROJECT_ID, AGENT_TYPE, fn); + + expect(result).toBe(true); + expect(fn).toHaveBeenCalledOnce(); + }); + + it('returns false and does not call fn when blocked', async () => { + const fn = vi.fn().mockResolvedValue(undefined); + mockCheckAgentTypeConcurrency.mockResolvedValue({ maxConcurrency: 1, blocked: true }); + + const result = await withAgentTypeConcurrency(PROJECT_ID, AGENT_TYPE, fn); + + expect(result).toBe(false); + expect(fn).not.toHaveBeenCalled(); + }); + + it('marks enqueued and dispatched when limit is set (not null)', async () => { + const fn = vi.fn().mockResolvedValue(undefined); + mockCheckAgentTypeConcurrency.mockResolvedValue({ maxConcurrency: 2, blocked: false }); + + await withAgentTypeConcurrency(PROJECT_ID, AGENT_TYPE, fn); + + expect(mockMarkRecentlyDispatched).toHaveBeenCalledWith(PROJECT_ID, AGENT_TYPE); + expect(mockMarkAgentTypeEnqueued).toHaveBeenCalledWith(PROJECT_ID, AGENT_TYPE); + }); + + it('does not mark enqueued when no limit (maxConcurrency null)', async () => { + const fn = vi.fn().mockResolvedValue(undefined); + mockCheckAgentTypeConcurrency.mockResolvedValue({ maxConcurrency: null, blocked: false }); + + await withAgentTypeConcurrency(PROJECT_ID, AGENT_TYPE, fn); + + expect(mockMarkRecentlyDispatched).not.toHaveBeenCalled(); + expect(mockMarkAgentTypeEnqueued).not.toHaveBeenCalled(); + }); + + it('clears enqueued slot in finally block when limit is set', async () => { + const fn = vi.fn().mockResolvedValue(undefined); + mockCheckAgentTypeConcurrency.mockResolvedValue({ maxConcurrency: 1, blocked: false }); + + await withAgentTypeConcurrency(PROJECT_ID, AGENT_TYPE, fn); + + expect(mockClearAgentTypeEnqueued).toHaveBeenCalledWith(PROJECT_ID, AGENT_TYPE); + }); + + it('does not clear enqueued slot when no limit', async () => { + const fn = vi.fn().mockResolvedValue(undefined); + mockCheckAgentTypeConcurrency.mockResolvedValue({ maxConcurrency: null, blocked: false }); + + await withAgentTypeConcurrency(PROJECT_ID, AGENT_TYPE, fn); + + expect(mockClearAgentTypeEnqueued).not.toHaveBeenCalled(); + }); + + it('clears enqueued slot even when fn throws', async () => { + const fn = vi.fn().mockRejectedValue(new Error('agent crashed')); + mockCheckAgentTypeConcurrency.mockResolvedValue({ maxConcurrency: 1, blocked: false }); + + await expect(withAgentTypeConcurrency(PROJECT_ID, AGENT_TYPE, fn)).rejects.toThrow( + 'agent crashed', + ); + + expect(mockClearAgentTypeEnqueued).toHaveBeenCalledWith(PROJECT_ID, AGENT_TYPE); + }); + + it('does not clear enqueued slot when blocked (fn never ran)', async () => { + const fn = vi.fn(); + mockCheckAgentTypeConcurrency.mockResolvedValue({ maxConcurrency: 1, blocked: true }); + + await withAgentTypeConcurrency(PROJECT_ID, AGENT_TYPE, fn); + + expect(mockClearAgentTypeEnqueued).not.toHaveBeenCalled(); + }); + + it('passes logLabel to checkAgentTypeConcurrency', async () => { + const fn = vi.fn().mockResolvedValue(undefined); + mockCheckAgentTypeConcurrency.mockResolvedValue({ maxConcurrency: null, blocked: false }); + + await withAgentTypeConcurrency(PROJECT_ID, AGENT_TYPE, fn, 'My handler'); + + expect(mockCheckAgentTypeConcurrency).toHaveBeenCalledWith( + PROJECT_ID, + AGENT_TYPE, + 'My handler', + ); + }); +}); diff --git a/tests/unit/triggers/shared/credential-scope.test.ts b/tests/unit/triggers/shared/credential-scope.test.ts new file mode 100644 index 00000000..d09ef825 --- /dev/null +++ b/tests/unit/triggers/shared/credential-scope.test.ts @@ -0,0 +1,122 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +// --------------------------------------------------------------------------- +// Hoisted mocks +// --------------------------------------------------------------------------- + +const { mockWithPMCredentials, mockWithPMProvider, mockCreatePMProvider, mockGetOrNull } = + vi.hoisted(() => { + return { + mockWithPMCredentials: vi.fn().mockImplementation((_id, _type, _getter, fn) => fn()), + mockWithPMProvider: vi.fn().mockImplementation((_provider, fn) => fn()), + mockCreatePMProvider: vi.fn().mockReturnValue({ type: 'trello' }), + mockGetOrNull: vi.fn().mockReturnValue(null), + }; + }); + +vi.mock('../../../../src/pm/context.js', () => ({ + withPMCredentials: mockWithPMCredentials, + withPMProvider: mockWithPMProvider, +})); + +vi.mock('../../../../src/pm/index.js', () => ({ + createPMProvider: mockCreatePMProvider, + pmRegistry: { getOrNull: mockGetOrNull }, +})); + +import { withPMScope } from '../../../../src/triggers/shared/credential-scope.js'; +import type { ProjectConfig } from '../../../../src/types/index.js'; + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +const mockProject = { + id: 'project-1', + name: 'Test', + repo: 'owner/repo', + baseBranch: 'main', + pm: { type: 'trello' }, +} as ProjectConfig; + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe('withPMScope', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockWithPMCredentials.mockImplementation( + (_id: unknown, _type: unknown, _getter: unknown, fn: () => Promise) => fn(), + ); + mockWithPMProvider.mockImplementation((_provider: unknown, fn: () => Promise) => fn()); + mockCreatePMProvider.mockReturnValue({ type: 'trello' }); + }); + + it('calls fn and returns its result', async () => { + const fn = vi.fn().mockResolvedValue('result'); + + const result = await withPMScope(mockProject, fn); + + expect(result).toBe('result'); + expect(fn).toHaveBeenCalledOnce(); + }); + + it('creates a PM provider using the project', async () => { + const fn = vi.fn().mockResolvedValue(undefined); + + await withPMScope(mockProject, fn); + + expect(mockCreatePMProvider).toHaveBeenCalledWith(mockProject); + }); + + it('calls withPMCredentials with project id and pm type', async () => { + const fn = vi.fn().mockResolvedValue(undefined); + + await withPMScope(mockProject, fn); + + expect(mockWithPMCredentials).toHaveBeenCalledWith( + 'project-1', + 'trello', + expect.any(Function), + expect.any(Function), + ); + }); + + it('calls withPMProvider with the created PM provider', async () => { + const fn = vi.fn().mockResolvedValue(undefined); + const mockProvider = { type: 'trello' }; + mockCreatePMProvider.mockReturnValue(mockProvider); + + await withPMScope(mockProject, fn); + + expect(mockWithPMProvider).toHaveBeenCalledWith(mockProvider, expect.any(Function)); + }); + + it('works when project has no PM type', async () => { + const projectWithoutPM = { ...mockProject, pm: undefined } as ProjectConfig; + const fn = vi.fn().mockResolvedValue('value'); + + const result = await withPMScope(projectWithoutPM, fn); + + expect(result).toBe('value'); + // withPMCredentials is still called (falls through to fn() when pmType is undefined) + expect(mockWithPMCredentials).toHaveBeenCalledWith( + 'project-1', + undefined, + expect.any(Function), + expect.any(Function), + ); + }); + + it('uses pmRegistry.getOrNull as the integration getter', async () => { + const fn = vi.fn().mockResolvedValue(undefined); + + await withPMScope(mockProject, fn); + + // The getter function passed to withPMCredentials calls pmRegistry.getOrNull + const getter = mockWithPMCredentials.mock.calls[0][2]; + getter('trello'); + expect(mockGetOrNull).toHaveBeenCalledWith('trello'); + }); +}); diff --git a/tests/unit/triggers/shared/trigger-resolution.test.ts b/tests/unit/triggers/shared/trigger-resolution.test.ts new file mode 100644 index 00000000..ffa5ff6b --- /dev/null +++ b/tests/unit/triggers/shared/trigger-resolution.test.ts @@ -0,0 +1,130 @@ +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(), + }, +})); + +import { resolveTriggerResult } from '../../../../src/triggers/shared/trigger-resolution.js'; +import type { TriggerContext, TriggerResult } from '../../../../src/types/index.js'; +import { logger } from '../../../../src/utils/logging.js'; + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +const mockProject = { + id: 'project-1', + name: 'Test', + repo: 'owner/repo', + baseBranch: 'main', +} as Parameters[1]['project']; + +const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: { some: 'data' }, +}; + +const triggerResult: TriggerResult = { + agentType: 'implementation', + agentInput: { repoFullName: 'owner/repo' }, +}; + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe('resolveTriggerResult', () => { + let mockRegistry: { dispatch: ReturnType }; + + beforeEach(() => { + vi.clearAllMocks(); + mockRegistry = { dispatch: vi.fn().mockResolvedValue(null) }; + }); + + it('returns preResolvedResult without dispatching when provided', async () => { + const result = await resolveTriggerResult(mockRegistry as never, ctx, triggerResult); + + expect(result).toBe(triggerResult); + expect(mockRegistry.dispatch).not.toHaveBeenCalled(); + }); + + it('logs info message with agentType when preResolvedResult is provided', async () => { + await resolveTriggerResult(mockRegistry as never, ctx, triggerResult, 'MyHandler'); + + expect(vi.mocked(logger.info)).toHaveBeenCalledWith( + expect.stringContaining('pre-resolved trigger result'), + expect.objectContaining({ agentType: 'implementation' }), + ); + }); + + it('includes logLabel in log message when provided', async () => { + await resolveTriggerResult(mockRegistry as never, ctx, triggerResult, 'MyCustomLabel'); + + expect(vi.mocked(logger.info)).toHaveBeenCalledWith( + expect.stringContaining('MyCustomLabel'), + expect.any(Object), + ); + }); + + it('falls back to ctx.source in log message when no logLabel', async () => { + await resolveTriggerResult(mockRegistry as never, ctx, triggerResult); + + expect(vi.mocked(logger.info)).toHaveBeenCalledWith( + expect.stringContaining('github'), + expect.any(Object), + ); + }); + + it('dispatches to registry when no preResolvedResult', async () => { + mockRegistry.dispatch.mockResolvedValue(triggerResult); + + const result = await resolveTriggerResult(mockRegistry as never, ctx); + + expect(mockRegistry.dispatch).toHaveBeenCalledWith(ctx); + expect(result).toBe(triggerResult); + }); + + it('returns null when registry dispatch returns null', async () => { + mockRegistry.dispatch.mockResolvedValue(null); + + const result = await resolveTriggerResult(mockRegistry as never, ctx); + + expect(result).toBeNull(); + }); + + it('logs info when no trigger matched (dispatch returns null)', async () => { + mockRegistry.dispatch.mockResolvedValue(null); + + await resolveTriggerResult(mockRegistry as never, ctx, undefined, 'TestHandler'); + + expect(vi.mocked(logger.info)).toHaveBeenCalledWith( + expect.stringContaining('no trigger matched'), + ); + }); + + it('does not log "no trigger matched" when dispatch returns a result', async () => { + mockRegistry.dispatch.mockResolvedValue(triggerResult); + + await resolveTriggerResult(mockRegistry as never, ctx); + + const infoCall = vi + .mocked(logger.info) + .mock.calls.find((call) => String(call[0]).includes('no trigger matched')); + expect(infoCall).toBeUndefined(); + }); + + it('passes undefined preResolvedResult and dispatches', async () => { + mockRegistry.dispatch.mockResolvedValue(triggerResult); + + const result = await resolveTriggerResult(mockRegistry as never, ctx, undefined); + + expect(mockRegistry.dispatch).toHaveBeenCalledOnce(); + expect(result).toBe(triggerResult); + }); +}); From b7ffdaf5bf91f783e9c1761e49f6d3a9808a8e6d Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Fri, 3 Apr 2026 17:16:45 +0000 Subject: [PATCH 2/3] fix(triggers): move startWatchdog inside concurrency callback and fix README - Move startWatchdog() inside the execute closure (GitHub) and inside the concurrency callback (Sentry) so it only fires when an agent actually runs. Previously the watchdog timer started before withAgentTypeConcurrency, so a concurrency-blocked job would tick a timer that could fire process.exit(1) after the container finished its work. - Fix Worker-Side Handler Comparison table: GitHub now uses withAgentTypeConcurrency (not checkAgentTypeConcurrency directly). - Fix Shared Utilities table: remove PM from the "Used By" column for concurrency.ts since src/pm/webhook-handler.ts still uses raw checkAgentTypeConcurrency. - Update flow diagrams to show startWatchdog nested inside withAgentTypeConcurrency. Co-Authored-By: Claude Opus 4.6 --- src/triggers/README.md | 16 ++++++++-------- src/triggers/github/webhook-handler.ts | 7 +++++-- src/triggers/sentry/webhook-handler.ts | 13 ++++++++----- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/triggers/README.md b/src/triggers/README.md index 8d691c55..bfef5618 100644 --- a/src/triggers/README.md +++ b/src/triggers/README.md @@ -34,7 +34,7 @@ Webhook processing is split into two distinct tiers: | PM credential scope | ✅ `integration.withCredentials` | ✅ `withPMCredentials` | ✅ `withPMCredentials` | | PM lifecycle ops | ✅ prepareForAgent / handleFailure | ✅ For PM-focused agents | ❌ Skipped | | Persona token mgmt | ❌ N/A | ✅ Implementer / reviewer | ❌ N/A | -| Agent concurrency | ✅ `checkAgentTypeConcurrency` | ✅ `checkAgentTypeConcurrency` | ✅ `withAgentTypeConcurrency` | +| Agent concurrency | ✅ `checkAgentTypeConcurrency` | ✅ `withAgentTypeConcurrency` | ✅ `withAgentTypeConcurrency` | --- @@ -73,7 +73,7 @@ To reduce duplication across the three worker-side handlers, shared utilities ar | File | Purpose | Used By | |------|---------|---------| -| `concurrency.ts` | `withAgentTypeConcurrency()` — wraps check→mark→execute→clear | GitHub, PM, Sentry | +| `concurrency.ts` | `withAgentTypeConcurrency()` — wraps check→mark→execute→clear | GitHub, Sentry | | `trigger-resolution.ts` | `resolveTriggerResult()` — pre-resolved or dispatch | GitHub, Sentry (PM has inline logic) | | `credential-scope.ts` | `withPMScope()` — `withPMCredentials` + `withPMProvider` | GitHub, Sentry | | `pm-ack.ts` | `postPMAckComment()` — posts ack to Trello/JIRA | Router GitHub adapter, GitHub worker handler | @@ -113,9 +113,9 @@ processGitHubWebhook(payload, eventType, registry, ackCommentId, triggerResult) └─ maybePostAckComment(result, ...) → PR or PM ack └─ runGitHubAgent(result, project, config) └─ withAgentTypeConcurrency(projectId, agentType) - └─ startWatchdog() - └─ withPMScope(project) - └─ runAgentWithCredentials(integration, result, ...) + └─ startWatchdog() + └─ withPMScope(project) + └─ runAgentWithCredentials(integration, result, ...) ``` ### Sentry webhook @@ -125,7 +125,7 @@ processSentryWebhook(payload, projectId, registry, triggerResult) └─ loadProjectConfigById(projectId) → project └─ resolveTriggerResult(registry, ctx, preResolved) └─ withAgentTypeConcurrency(projectId, agentType) - └─ startWatchdog() - └─ withPMScope(project) - └─ runAgentExecutionPipeline(result, ...) + └─ startWatchdog() + └─ withPMScope(project) + └─ runAgentExecutionPipeline(result, ...) ``` diff --git a/src/triggers/github/webhook-handler.ts b/src/triggers/github/webhook-handler.ts index 4b62b8f0..762ab7db 100644 --- a/src/triggers/github/webhook-handler.ts +++ b/src/triggers/github/webhook-handler.ts @@ -148,8 +148,6 @@ async function runGitHubAgent( project: ProjectConfig, config: CascadeConfig, ): Promise { - startWatchdog(project.watchdogTimeoutMs); - // PM-focused agents (e.g. backlog-manager) triggered from GitHub should use // PM-appropriate lifecycle config: no GitHub PR comment callbacks, allow PM lifecycle ops. const pmFocused = result.agentType ? await isPMFocusedAgent(result.agentType) : false; @@ -157,6 +155,11 @@ async function runGitHubAgent( const agentType = result.agentType; const execute = async () => { + // Only start the watchdog when the agent actually runs (after concurrency check passes). + // Starting it before the check risks a spurious process.exit(1) if the container + // is still alive after a concurrency-blocked job finishes. + startWatchdog(project.watchdogTimeoutMs); + // Establish PM credential + provider scope for agents with workItemId // (needed for PM lifecycle operations: labels, status moves, PR links) await withPMScope(project, () => diff --git a/src/triggers/sentry/webhook-handler.ts b/src/triggers/sentry/webhook-handler.ts index 946aa1b3..67b29bf8 100644 --- a/src/triggers/sentry/webhook-handler.ts +++ b/src/triggers/sentry/webhook-handler.ts @@ -61,19 +61,22 @@ export async function processSentryWebhook( agentType: result.agentType, }); - startWatchdog(pc.project.watchdogTimeoutMs); - await withAgentTypeConcurrency( pc.project.id, result.agentType, - () => - withPMScope(pc.project, () => + () => { + // Only start the watchdog when the agent actually runs (after concurrency check passes). + // Starting it before the check risks a spurious process.exit(1) if the container + // is still alive after a concurrency-blocked job finishes. + startWatchdog(pc.project.watchdogTimeoutMs); + return withPMScope(pc.project, () => runAgentExecutionPipeline(result, pc.project, pc.config, { logLabel: 'Sentry agent', skipPrepareForAgent: true, skipHandleFailure: true, }), - ), + ); + }, 'processSentryWebhook', ); } From 3e84e3992954cb10a2c17b59fd231bdba7d61eb9 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Fri, 3 Apr 2026 17:28:22 +0000 Subject: [PATCH 3/3] docs(triggers): fix inaccurate "Used By" entries and module comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - trigger-resolution.ts: correct "Used By" to "Sentry (GitHub and PM use inline logic)" — GitHub handler uses inline if/else, not resolveTriggerResult - pm-ack.ts: correct "Used By" to "GitHub worker handler" — the router adapter has its own local postPMAck and does not import the shared utility - pm-ack.ts module comment: remove false claim about router-side usage; add note clarifying the router adapter has its own local function - GitHub webhook flow diagram: replace resolveTriggerResult with accurate inline dispatch description Co-Authored-By: Claude Opus 4.6 --- src/triggers/README.md | 6 +++--- src/triggers/shared/pm-ack.ts | 7 +++++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/triggers/README.md b/src/triggers/README.md index bfef5618..78a6813b 100644 --- a/src/triggers/README.md +++ b/src/triggers/README.md @@ -74,9 +74,9 @@ To reduce duplication across the three worker-side handlers, shared utilities ar | File | Purpose | Used By | |------|---------|---------| | `concurrency.ts` | `withAgentTypeConcurrency()` — wraps check→mark→execute→clear | GitHub, Sentry | -| `trigger-resolution.ts` | `resolveTriggerResult()` — pre-resolved or dispatch | GitHub, Sentry (PM has inline logic) | +| `trigger-resolution.ts` | `resolveTriggerResult()` — pre-resolved or dispatch | Sentry (GitHub and PM use inline logic) | | `credential-scope.ts` | `withPMScope()` — `withPMCredentials` + `withPMProvider` | GitHub, Sentry | -| `pm-ack.ts` | `postPMAckComment()` — posts ack to Trello/JIRA | Router GitHub adapter, GitHub worker handler | +| `pm-ack.ts` | `postPMAckComment()` — posts ack to Trello/JIRA | GitHub worker handler | | `agent-execution.ts` | `runAgentExecutionPipeline()` — full agent lifecycle | All handlers (via `webhook-execution.ts`) | | `webhook-execution.ts` | `runAgentWithCredentials()` — LLM keys + credentials + pipeline | GitHub, PM | @@ -108,7 +108,7 @@ processPMWebhook(integration, payload, registry) processGitHubWebhook(payload, eventType, registry, ackCommentId, triggerResult) └─ integration.parseWebhookPayload(payload) → event └─ integration.lookupProject(event.repo) → project - └─ resolveTriggerResult(registry, ctx, preResolved) + └─ [inline] if triggerResult → use it, else dispatchTrigger(registry, payload, project) └─ [optional] pollWaitForChecks(result, repo) → checksOk └─ maybePostAckComment(result, ...) → PR or PM ack └─ runGitHubAgent(result, project, config) diff --git a/src/triggers/shared/pm-ack.ts b/src/triggers/shared/pm-ack.ts index 216b1d7c..72b8c3ad 100644 --- a/src/triggers/shared/pm-ack.ts +++ b/src/triggers/shared/pm-ack.ts @@ -3,10 +3,13 @@ * * Centralises the logic for posting acknowledgment comments to PM tools * (Trello/JIRA) for PM-focused agents triggered from GitHub or other - * non-PM sources. This is shared between: + * non-PM sources. * - * - Router-side: `router/adapters/github.ts` (postPMAck) + * Used by: * - Worker-side: `triggers/github/webhook-handler.ts` (maybePostPmAckComment) + * + * Note: `router/adapters/github.ts` has its own local `postPMAck` function + * and does not use this shared utility. */ import { postJiraAck, postTrelloAck } from '../../router/acknowledgments.js';