From 06e273e728118a163d36072d19fd50cf3f04320f Mon Sep 17 00:00:00 2001 From: aaight Date: Fri, 8 May 2026 16:53:18 +0200 Subject: [PATCH 1/5] refactor(triggers): extract agent work item linking (#1268) Co-authored-by: Cascade Bot --- src/triggers/shared/agent-execution.ts | 130 +------- src/triggers/shared/agent-work-items.ts | 137 +++++++++ .../triggers/shared/agent-work-items.test.ts | 283 ++++++++++++++++++ 3 files changed, 428 insertions(+), 122 deletions(-) create mode 100644 src/triggers/shared/agent-work-items.ts create mode 100644 tests/unit/triggers/shared/agent-work-items.test.ts diff --git a/src/triggers/shared/agent-execution.ts b/src/triggers/shared/agent-execution.ts index 1f09a7d89..0f52b6fd9 100644 --- a/src/triggers/shared/agent-execution.ts +++ b/src/triggers/shared/agent-execution.ts @@ -1,8 +1,6 @@ import { getAgentProfile } from '../../agents/definitions/profiles.js'; import type { LifecycleHooks } from '../../agents/definitions/schema.js'; import { runAgent } from '../../agents/registry.js'; -import { createWorkItem, linkPRToWorkItem } from '../../db/repositories/prWorkItemsRepository.js'; -import { updateRunPRNumber } from '../../db/repositories/runsRepository.js'; import { getPMProvider } from '../../pm/context.js'; import { createPMProvider, @@ -22,6 +20,12 @@ import { parseRepoFullName } from '../../utils/repo.js'; import type { TriggerResult } from '../types.js'; import type { AgentExecutionConfig, AgentExecutionContext } from './agent-execution-types.js'; import { handleAgentResultArtifacts } from './agent-result-handler.js'; +import { + linkPRPostExecution, + persistPreRunWorkItems, + prepareAgentWorkItem, + resolveWorkItemId, +} from './agent-work-items.js'; import { isPipelineAtCapacity } from './backlog-check.js'; import { checkBudgetExceeded } from './budget.js'; import { triggerDebugAnalysis } from './debug-runner.js'; @@ -136,64 +140,6 @@ async function notifyValidationFailure( } } -/** - * Link a PR to a work item and backfill the run's prNumber after agent execution. - * Extracted to reduce cognitive complexity in the main pipeline function. - */ -async function linkPRPostExecution( - agentResult: AgentResult & { prUrl: string }, - project: ProjectConfig & { repo: string }, - result: TriggerResult, - workItemId: string | undefined, -): Promise { - const prNumber = extractPRNumber(agentResult.prUrl); - if (!prNumber) return; - - // Fetch PR title from GitHub API (best-effort, resolves the prTitle gap) - let prTitle: string | undefined; - try { - const { githubClient } = await import('../../github/client.js'); - const { parseRepoFullName } = await import('../../utils/repo.js'); - const { owner, repo } = parseRepoFullName(project.repo); - const pr = await githubClient.getPR(owner, repo, prNumber); - prTitle = pr.title; - } catch (err) { - logger.warn('Failed to fetch PR title from GitHub', { - projectId: project.id, - prNumber, - error: String(err), - }); - } - - try { - await linkPRToWorkItem(project.id, project.repo, prNumber, workItemId ?? null, { - prUrl: agentResult.prUrl, - prTitle, - workItemUrl: result.workItemUrl, - workItemTitle: result.workItemTitle, - }); - } catch (err) { - logger.warn('Failed to link PR to work item post-execution', { - projectId: project.id, - prNumber, - workItemId, - error: String(err), - }); - } - - if (agentResult.runId) { - try { - await updateRunPRNumber(agentResult.runId, prNumber); - } catch (err) { - logger.warn('Failed to backfill prNumber on run', { - runId: agentResult.runId, - prNumber, - error: String(err), - }); - } - } -} - /** * Dispatch a review agent after a successful implementation run, if the PR's * CI is green and no review has been dispatched yet. @@ -366,24 +312,6 @@ async function postAgentSummaryToPM( } } -/** - * Resolve a work item ID: prefer the one from TriggerResult, fall back to DB lookup by PR number. - */ -async function resolveWorkItemId( - workItemId: string | undefined, - projectId: string, - prNumber: number | undefined, -): Promise { - if (workItemId) return workItemId; - if (!prNumber || !projectId) return undefined; - try { - const { lookupWorkItemForPR } = await import('../../db/repositories/prWorkItemsRepository.js'); - return (await lookupWorkItemForPR(projectId, prNumber)) ?? undefined; - } catch { - return undefined; // best-effort - } -} - /** * Shared agent execution pipeline. * @@ -456,7 +384,7 @@ export async function runAgentExecutionPipeline( // caught up — preferring the live value avoids carrying a stale `undefined` // into runAgent (and therefore agent_runs.work_item_id) and into the // post-execution linkPRToWorkItem write. - const workItemId = await resolveWorkItemId(result.workItemId, project.id, result.prNumber); + const { workItemId, agentInput } = await prepareAgentWorkItem(result, project.id); // Patch agentInput.workItemId whenever it diverges from the resolved value. // Two cases this catches: @@ -471,11 +399,6 @@ export async function runAgentExecutionPipeline( // catches this at write-time; this runtime patch is the safety net. // tryCreateRun (src/agents/shared/runTracking.ts) reads workItemId from // agentInput when persisting agent_runs.work_item_id. - const agentInput = - workItemId && (result.agentInput.workItemId as string | undefined) !== workItemId - ? { ...result.agentInput, workItemId } - : result.agentInput; - const executionContext: AgentExecutionContext = { result, project, @@ -500,44 +423,7 @@ export async function runAgentExecutionPipeline( remainingBudgetUsd = budgetResult.remainingBudgetUsd; } - // Insert a work-item-only row so PM-triggered runs show up in the dashboard - // even before a PR is created. This is idempotent — if a row already exists - // it is updated with the latest display fields. - if (executionContext.workItemId) { - try { - await createWorkItem(executionContext.project.id, executionContext.workItemId, { - workItemUrl: executionContext.result.workItemUrl, - workItemTitle: executionContext.result.workItemTitle, - }); - } catch (err) { - logger.warn('Failed to persist work-item row for PM-triggered run', { - projectId: executionContext.project.id, - workItemId: executionContext.workItemId, - error: String(err), - }); - } - } - - // Ensure a pr_work_items entry exists for PR-triggered runs (SCM webhooks). - // This covers cases where no workItemId exists (orphan PRs) — linkPRToWorkItem - // handles upsert, promotion, and orphan creation. - if (result.prNumber && project.repo) { - try { - await linkPRToWorkItem(project.id, project.repo, result.prNumber, workItemId ?? null, { - workItemUrl: result.workItemUrl, - workItemTitle: result.workItemTitle, - prUrl: result.prUrl, - prTitle: result.prTitle, - }); - } catch (err) { - logger.warn('Failed to ensure pr_work_items entry for PR-triggered run', { - projectId: project.id, - prNumber: result.prNumber, - workItemId, - error: String(err), - }); - } - } + await persistPreRunWorkItems(result, project, workItemId); if (executionContext.workItemId && !skipPrepareForAgent) { await executionContext.lifecycle.prepareForAgent( diff --git a/src/triggers/shared/agent-work-items.ts b/src/triggers/shared/agent-work-items.ts new file mode 100644 index 000000000..c14b6209c --- /dev/null +++ b/src/triggers/shared/agent-work-items.ts @@ -0,0 +1,137 @@ +import { createWorkItem, linkPRToWorkItem } from '../../db/repositories/prWorkItemsRepository.js'; +import { updateRunPRNumber } from '../../db/repositories/runsRepository.js'; +import type { AgentInput, AgentResult, ProjectConfig } from '../../types/index.js'; +import { logger } from '../../utils/logging.js'; +import { extractPRNumber } from '../../utils/prUrl.js'; +import { parseRepoFullName } from '../../utils/repo.js'; +import type { TriggerResult } from '../types.js'; + +export interface ResolvedAgentWorkItem { + workItemId: string | undefined; + agentInput: AgentInput; +} + +export async function resolveWorkItemId( + workItemId: string | undefined, + projectId: string, + prNumber: number | undefined, +): Promise { + if (workItemId) return workItemId; + if (!prNumber) return undefined; + + try { + const { lookupWorkItemForPR } = await import('../../db/repositories/prWorkItemsRepository.js'); + return (await lookupWorkItemForPR(projectId, prNumber)) ?? undefined; + } catch (err) { + logger.warn('Failed to resolve workItemId for PR', { + projectId, + prNumber, + error: String(err), + }); + return undefined; + } +} + +export async function prepareAgentWorkItem( + result: TriggerResult, + projectId: string, +): Promise { + const workItemId = await resolveWorkItemId(result.workItemId, projectId, result.prNumber); + const agentInput = + workItemId && result.agentInput.workItemId !== workItemId + ? { ...result.agentInput, workItemId } + : result.agentInput; + + return { workItemId, agentInput }; +} + +export async function persistPreRunWorkItems( + result: TriggerResult, + project: ProjectConfig, + workItemId: string | undefined, +): Promise { + if (workItemId) { + try { + await createWorkItem(project.id, workItemId, { + workItemUrl: result.workItemUrl, + workItemTitle: result.workItemTitle, + }); + } catch (err) { + logger.warn('Failed to persist work-item row for PM-triggered run', { + projectId: project.id, + workItemId, + error: String(err), + }); + } + } + + if (result.prNumber && project.repo) { + try { + await linkPRToWorkItem(project.id, project.repo, result.prNumber, workItemId ?? null, { + workItemUrl: result.workItemUrl, + workItemTitle: result.workItemTitle, + prUrl: result.prUrl, + prTitle: result.prTitle, + }); + } catch (err) { + logger.warn('Failed to ensure pr_work_items entry for PR-triggered run', { + projectId: project.id, + prNumber: result.prNumber, + workItemId, + error: String(err), + }); + } + } +} + +export async function linkPRPostExecution( + agentResult: AgentResult & { prUrl: string }, + project: ProjectConfig & { repo: string }, + result: TriggerResult, + workItemId: string | undefined, +): Promise { + const prNumber = extractPRNumber(agentResult.prUrl); + if (!prNumber) return; + + let prTitle: string | undefined; + try { + const { githubClient } = await import('../../github/client.js'); + const { owner, repo } = parseRepoFullName(project.repo); + const pr = await githubClient.getPR(owner, repo, prNumber); + prTitle = pr.title; + } catch (err) { + logger.warn('Failed to fetch PR title from GitHub', { + projectId: project.id, + prNumber, + error: String(err), + }); + } + + try { + await linkPRToWorkItem(project.id, project.repo, prNumber, workItemId ?? null, { + prUrl: agentResult.prUrl, + prTitle, + workItemUrl: result.workItemUrl, + workItemTitle: result.workItemTitle, + }); + } catch (err) { + logger.warn('Failed to link PR to work item post-execution', { + projectId: project.id, + prNumber, + workItemId, + error: String(err), + }); + } + + if (agentResult.runId) { + try { + await updateRunPRNumber(agentResult.runId, prNumber); + } catch (err) { + logger.warn('Failed to backfill prNumber on run', { + runId: agentResult.runId, + prNumber, + error: String(err), + }); + } + } +} diff --git a/tests/unit/triggers/shared/agent-work-items.test.ts b/tests/unit/triggers/shared/agent-work-items.test.ts new file mode 100644 index 000000000..40bc2dbe1 --- /dev/null +++ b/tests/unit/triggers/shared/agent-work-items.test.ts @@ -0,0 +1,283 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const { + mockCreateWorkItem, + mockLinkPRToWorkItem, + mockLookupWorkItemForPR, + mockUpdateRunPRNumber, + mockGithubClient, + mockParseRepoFullName, + mockLogger, +} = vi.hoisted(() => ({ + mockCreateWorkItem: vi.fn().mockResolvedValue(undefined), + mockLinkPRToWorkItem: vi.fn().mockResolvedValue(undefined), + mockLookupWorkItemForPR: vi.fn().mockResolvedValue(null), + mockUpdateRunPRNumber: vi.fn().mockResolvedValue(undefined), + mockGithubClient: { + getPR: vi.fn().mockResolvedValue({ title: 'feat: linked PR' }), + }, + mockParseRepoFullName: vi.fn().mockReturnValue({ owner: 'acme', repo: 'myapp' }), + mockLogger: { + warn: vi.fn(), + info: vi.fn(), + debug: vi.fn(), + error: vi.fn(), + }, +})); + +vi.mock('../../../../src/db/repositories/prWorkItemsRepository.js', () => ({ + createWorkItem: mockCreateWorkItem, + linkPRToWorkItem: mockLinkPRToWorkItem, + lookupWorkItemForPR: mockLookupWorkItemForPR, +})); + +vi.mock('../../../../src/db/repositories/runsRepository.js', () => ({ + updateRunPRNumber: mockUpdateRunPRNumber, +})); + +vi.mock('../../../../src/github/client.js', () => ({ + githubClient: mockGithubClient, +})); + +vi.mock('../../../../src/utils/repo.js', () => ({ + parseRepoFullName: mockParseRepoFullName, +})); + +vi.mock('../../../../src/utils/logging.js', () => ({ + logger: mockLogger, +})); + +import { + linkPRPostExecution, + persistPreRunWorkItems, + prepareAgentWorkItem, + resolveWorkItemId, +} from '../../../../src/triggers/shared/agent-work-items.js'; +import type { TriggerResult } from '../../../../src/triggers/types.js'; +import type { ProjectConfig } from '../../../../src/types/index.js'; + +const PROJECT = { + id: 'project-1', + repo: 'acme/myapp', + pm: { type: 'trello' }, +} as unknown as ProjectConfig; + +describe('agent-work-items', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockCreateWorkItem.mockResolvedValue(undefined); + mockLinkPRToWorkItem.mockResolvedValue(undefined); + mockLookupWorkItemForPR.mockResolvedValue(null); + mockUpdateRunPRNumber.mockResolvedValue(undefined); + mockGithubClient.getPR.mockResolvedValue({ title: 'feat: linked PR' }); + mockParseRepoFullName.mockReturnValue({ owner: 'acme', repo: 'myapp' }); + }); + + describe('resolveWorkItemId', () => { + it('prefers the trigger-supplied workItemId', async () => { + const result = await resolveWorkItemId('card-from-trigger', 'project-1', 42); + + expect(result).toBe('card-from-trigger'); + expect(mockLookupWorkItemForPR).not.toHaveBeenCalled(); + }); + + it('falls back to PR lookup when no trigger workItemId is present', async () => { + mockLookupWorkItemForPR.mockResolvedValueOnce('card-from-db'); + + const result = await resolveWorkItemId(undefined, 'project-1', 42); + + expect(result).toBe('card-from-db'); + expect(mockLookupWorkItemForPR).toHaveBeenCalledWith('project-1', 42); + }); + + it('warns and returns undefined when PR lookup fails', async () => { + mockLookupWorkItemForPR.mockRejectedValueOnce(new Error('db unavailable')); + + const result = await resolveWorkItemId(undefined, 'project-1', 42); + + expect(result).toBeUndefined(); + expect(mockLogger.warn).toHaveBeenCalledWith( + 'Failed to resolve workItemId for PR', + expect.objectContaining({ projectId: 'project-1', prNumber: 42 }), + ); + }); + }); + + describe('prepareAgentWorkItem', () => { + it('patches agentInput.workItemId only when a work item is resolved', async () => { + mockLookupWorkItemForPR.mockResolvedValueOnce('card-from-db'); + const result = await prepareAgentWorkItem( + { + agentType: 'review', + agentInput: { prNumber: 42 }, + prNumber: 42, + }, + 'project-1', + ); + + expect(result).toEqual({ + workItemId: 'card-from-db', + agentInput: { prNumber: 42, workItemId: 'card-from-db' }, + }); + }); + + it('leaves agentInput untouched when no work item is resolved', async () => { + const agentInput = { prNumber: 42 }; + const result = await prepareAgentWorkItem( + { agentType: 'review', agentInput, prNumber: 42 }, + 'project-1', + ); + + expect(result.workItemId).toBeUndefined(); + expect(result.agentInput).toBe(agentInput); + }); + }); + + describe('persistPreRunWorkItems', () => { + it('persists PM work-item display data and links the PR before the run', async () => { + const result: TriggerResult = { + agentType: 'review', + agentInput: {}, + workItemId: 'card-1', + workItemUrl: 'https://trello.com/c/card-1', + workItemTitle: 'Build the thing', + prNumber: 42, + prUrl: 'https://github.com/acme/myapp/pull/42', + prTitle: 'Test PR', + }; + + await persistPreRunWorkItems(result, PROJECT, 'card-1'); + + expect(mockCreateWorkItem).toHaveBeenCalledWith('project-1', 'card-1', { + workItemUrl: 'https://trello.com/c/card-1', + workItemTitle: 'Build the thing', + }); + expect(mockLinkPRToWorkItem).toHaveBeenCalledWith( + 'project-1', + 'acme/myapp', + 42, + 'card-1', + expect.objectContaining({ + prUrl: 'https://github.com/acme/myapp/pull/42', + prTitle: 'Test PR', + }), + ); + }); + + it('creates an orphan PR link when no workItemId is resolved', async () => { + await persistPreRunWorkItems( + { + agentType: 'review', + agentInput: {}, + prNumber: 42, + prUrl: 'https://github.com/acme/myapp/pull/42', + }, + PROJECT, + undefined, + ); + + expect(mockCreateWorkItem).not.toHaveBeenCalled(); + expect(mockLinkPRToWorkItem).toHaveBeenCalledWith( + 'project-1', + 'acme/myapp', + 42, + null, + expect.objectContaining({ prUrl: 'https://github.com/acme/myapp/pull/42' }), + ); + }); + + it('logs persistence and link failures without throwing', async () => { + mockCreateWorkItem.mockRejectedValueOnce(new Error('create failed')); + mockLinkPRToWorkItem.mockRejectedValueOnce(new Error('link failed')); + + await expect( + persistPreRunWorkItems( + { agentType: 'review', agentInput: {}, prNumber: 42 }, + PROJECT, + 'card-1', + ), + ).resolves.toBeUndefined(); + + expect(mockLogger.warn).toHaveBeenCalledWith( + 'Failed to persist work-item row for PM-triggered run', + expect.objectContaining({ workItemId: 'card-1' }), + ); + expect(mockLogger.warn).toHaveBeenCalledWith( + 'Failed to ensure pr_work_items entry for PR-triggered run', + expect.objectContaining({ prNumber: 42, workItemId: 'card-1' }), + ); + }); + }); + + describe('linkPRPostExecution', () => { + const agentResult = { + success: true, + output: '', + runId: 'run-1', + prUrl: 'https://github.com/acme/myapp/pull/42', + }; + + it('fetches the PR title, links the PR, and backfills run prNumber', async () => { + await linkPRPostExecution( + agentResult, + PROJECT as ProjectConfig & { repo: string }, + { + agentType: 'implementation', + agentInput: {}, + workItemUrl: 'https://trello.com/c/card-1', + workItemTitle: 'Build the thing', + }, + 'card-1', + ); + + expect(mockGithubClient.getPR).toHaveBeenCalledWith('acme', 'myapp', 42); + expect(mockLinkPRToWorkItem).toHaveBeenCalledWith( + 'project-1', + 'acme/myapp', + 42, + 'card-1', + expect.objectContaining({ + prUrl: 'https://github.com/acme/myapp/pull/42', + prTitle: 'feat: linked PR', + workItemTitle: 'Build the thing', + }), + ); + expect(mockUpdateRunPRNumber).toHaveBeenCalledWith('run-1', 42); + }); + + it('continues when PR title fetch, link, and run backfill fail', async () => { + mockGithubClient.getPR.mockRejectedValueOnce(new Error('github failed')); + mockLinkPRToWorkItem.mockRejectedValueOnce(new Error('link failed')); + mockUpdateRunPRNumber.mockRejectedValueOnce(new Error('run failed')); + + await expect( + linkPRPostExecution( + agentResult, + PROJECT as ProjectConfig & { repo: string }, + { agentType: 'implementation', agentInput: {} }, + 'card-1', + ), + ).resolves.toBeUndefined(); + + expect(mockLinkPRToWorkItem).toHaveBeenCalledWith( + 'project-1', + 'acme/myapp', + 42, + 'card-1', + expect.objectContaining({ prTitle: undefined }), + ); + expect(mockLogger.warn).toHaveBeenCalledWith( + 'Failed to fetch PR title from GitHub', + expect.objectContaining({ prNumber: 42 }), + ); + expect(mockLogger.warn).toHaveBeenCalledWith( + 'Failed to link PR to work item post-execution', + expect.objectContaining({ prNumber: 42, workItemId: 'card-1' }), + ); + expect(mockLogger.warn).toHaveBeenCalledWith( + 'Failed to backfill prNumber on run', + expect.objectContaining({ runId: 'run-1', prNumber: 42 }), + ); + }); + }); +}); From 717d00f14854a76795a74c97e06eadf7c805ee72 Mon Sep 17 00:00:00 2001 From: aaight Date: Fri, 8 May 2026 21:33:41 +0200 Subject: [PATCH 2/5] refactor(triggers): extract agent execution lifecycle (#1271) * refactor(triggers): extract agent execution lifecycle * fix(ci): resolve production audit failure --------- Co-authored-by: Cascade Bot --- package-lock.json | 106 +++---- .../shared/agent-execution-lifecycle.ts | 159 ++++++++++ src/triggers/shared/agent-execution.ts | 146 ++------- .../shared/agent-execution-lifecycle.test.ts | 283 ++++++++++++++++++ 4 files changed, 507 insertions(+), 187 deletions(-) create mode 100644 src/triggers/shared/agent-execution-lifecycle.ts create mode 100644 tests/unit/triggers/shared/agent-execution-lifecycle.test.ts diff --git a/package-lock.json b/package-lock.json index 726327567..b2caac049 100644 --- a/package-lock.json +++ b/package-lock.json @@ -98,9 +98,9 @@ } }, "node_modules/@anthropic-ai/claude-agent-sdk": { - "version": "0.2.131", - "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk/-/claude-agent-sdk-0.2.131.tgz", - "integrity": "sha512-4Xak+BlcxXuni5BvNeb0tnSapIoCBxE7cFnXvkUs0EwbY88FkmdJEtBXZbF7NRuN8bUwDeNxvy0Fs0dWnzpU+g==", + "version": "0.2.136", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk/-/claude-agent-sdk-0.2.136.tgz", + "integrity": "sha512-43zwEEyv39+jPglzSDSBu90cZvtuKB8QsVJL/PuDt6XghhPubjhXaBJ0qdFcyHSJXrKuyKms1aHXqykRwwzTFQ==", "license": "SEE LICENSE IN README.md", "dependencies": { "@anthropic-ai/sdk": "^0.81.0", @@ -110,23 +110,23 @@ "node": ">=18.0.0" }, "optionalDependencies": { - "@anthropic-ai/claude-agent-sdk-darwin-arm64": "0.2.131", - "@anthropic-ai/claude-agent-sdk-darwin-x64": "0.2.131", - "@anthropic-ai/claude-agent-sdk-linux-arm64": "0.2.131", - "@anthropic-ai/claude-agent-sdk-linux-arm64-musl": "0.2.131", - "@anthropic-ai/claude-agent-sdk-linux-x64": "0.2.131", - "@anthropic-ai/claude-agent-sdk-linux-x64-musl": "0.2.131", - "@anthropic-ai/claude-agent-sdk-win32-arm64": "0.2.131", - "@anthropic-ai/claude-agent-sdk-win32-x64": "0.2.131" + "@anthropic-ai/claude-agent-sdk-darwin-arm64": "0.2.136", + "@anthropic-ai/claude-agent-sdk-darwin-x64": "0.2.136", + "@anthropic-ai/claude-agent-sdk-linux-arm64": "0.2.136", + "@anthropic-ai/claude-agent-sdk-linux-arm64-musl": "0.2.136", + "@anthropic-ai/claude-agent-sdk-linux-x64": "0.2.136", + "@anthropic-ai/claude-agent-sdk-linux-x64-musl": "0.2.136", + "@anthropic-ai/claude-agent-sdk-win32-arm64": "0.2.136", + "@anthropic-ai/claude-agent-sdk-win32-x64": "0.2.136" }, "peerDependencies": { "zod": "^4.0.0" } }, "node_modules/@anthropic-ai/claude-agent-sdk-darwin-arm64": { - "version": "0.2.131", - "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-darwin-arm64/-/claude-agent-sdk-darwin-arm64-0.2.131.tgz", - "integrity": "sha512-jOGq8lAi6bakqX0MBVkJDOddC2xSYnP1XHzps2cBF696dQlHoXs4hqU+69Wt4oKScyw4tM4Pe+Mmeut9LJqbEg==", + "version": "0.2.136", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-darwin-arm64/-/claude-agent-sdk-darwin-arm64-0.2.136.tgz", + "integrity": "sha512-EmUkFbUhKEu51Nc01dS3Mwn4YTcwjUORAydoolGY0ZGPJt04WL9ske0QO5L4MU9M43L/5FAByfn9GnDEjX3ONg==", "cpu": [ "arm64" ], @@ -137,9 +137,9 @@ ] }, "node_modules/@anthropic-ai/claude-agent-sdk-darwin-x64": { - "version": "0.2.131", - "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-darwin-x64/-/claude-agent-sdk-darwin-x64-0.2.131.tgz", - "integrity": "sha512-IxewhApb20ucAxnpUCAwETLjO5PsQRAJIBBlDlNqPsd20LIZVVQuQ5orFf6CGEs6MfYRnWz2FYwfHhguGNPIyQ==", + "version": "0.2.136", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-darwin-x64/-/claude-agent-sdk-darwin-x64-0.2.136.tgz", + "integrity": "sha512-uSh7OGZVl7vTd+3ybCRfCqUdAMS8E8TnqCFzC7XtqHoUxbyQpeavTaW22dMP7b0a1RsDDQ1NsvEGcQIue8XYkg==", "cpu": [ "x64" ], @@ -150,15 +150,12 @@ ] }, "node_modules/@anthropic-ai/claude-agent-sdk-linux-arm64": { - "version": "0.2.131", - "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-linux-arm64/-/claude-agent-sdk-linux-arm64-0.2.131.tgz", - "integrity": "sha512-GDwaga8aadtVeYq1wJM2BSWp5l/Srel7L5WRbEvkEWXeGP463S7VLJyiNVcbjbi/HLmyQigEkzFoHfZdeqKOvw==", + "version": "0.2.136", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-linux-arm64/-/claude-agent-sdk-linux-arm64-0.2.136.tgz", + "integrity": "sha512-u3m5O35xBDX5zkAyt0hYl0E6hPOr6oviFD00LzHtVvyu7yXFBP/Aaq1u5CGgf/dmsSYqXg6dWJhNcVepIcphUQ==", "cpu": [ "arm64" ], - "libc": [ - "glibc" - ], "license": "SEE LICENSE IN LICENSE.md", "optional": true, "os": [ @@ -166,15 +163,12 @@ ] }, "node_modules/@anthropic-ai/claude-agent-sdk-linux-arm64-musl": { - "version": "0.2.131", - "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-linux-arm64-musl/-/claude-agent-sdk-linux-arm64-musl-0.2.131.tgz", - "integrity": "sha512-7efL5otHqTKMeNxIztEjEGs8ktlR3hfMmVbo1HaEbs+tkJ6fvMwS3k4xnUP7Bqy+GsM+U9r9kRdNz4MVdc80hg==", + "version": "0.2.136", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-linux-arm64-musl/-/claude-agent-sdk-linux-arm64-musl-0.2.136.tgz", + "integrity": "sha512-djd8U2UfUVzCXRqLzd3YQFnCNe+csXPGznUjlQpR3lbLkc+M9apZmTkFANFgFhK9dG/AZdHSSyLM1GVS96dm0g==", "cpu": [ "arm64" ], - "libc": [ - "musl" - ], "license": "SEE LICENSE IN LICENSE.md", "optional": true, "os": [ @@ -182,15 +176,12 @@ ] }, "node_modules/@anthropic-ai/claude-agent-sdk-linux-x64": { - "version": "0.2.131", - "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-linux-x64/-/claude-agent-sdk-linux-x64-0.2.131.tgz", - "integrity": "sha512-tJJggvCGtkK876CowajF/42AdUy0TTJk0gHeCKuDCMJF3hMs70EtYnwyM81nb10tKUFb6zYdvn6iPn6iGx7iFQ==", + "version": "0.2.136", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-linux-x64/-/claude-agent-sdk-linux-x64-0.2.136.tgz", + "integrity": "sha512-jSjVsZT+o96pR7R2JPRMWHp3NSBmHsVwTLtjibPS8LFSLLLGfbcSWfX6Q5m+4H78fsKYnLTTYk/tXI6r8Q1GgQ==", "cpu": [ "x64" ], - "libc": [ - "glibc" - ], "license": "SEE LICENSE IN LICENSE.md", "optional": true, "os": [ @@ -198,15 +189,12 @@ ] }, "node_modules/@anthropic-ai/claude-agent-sdk-linux-x64-musl": { - "version": "0.2.131", - "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-linux-x64-musl/-/claude-agent-sdk-linux-x64-musl-0.2.131.tgz", - "integrity": "sha512-WNqUJscB1F86Igbnw5zXpndT89I7l3aIvPJQEOrSA5JaIDmfJft8QA1rrJPwf2tcxP8nNS0H3MbEFBAxq92bNw==", + "version": "0.2.136", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-linux-x64-musl/-/claude-agent-sdk-linux-x64-musl-0.2.136.tgz", + "integrity": "sha512-2yb+jc8OVmmMvIuBnvfu5JcnkbfuBG+Se0ePLRr0JaNPJ5DJf0ZR86drpTnjANPcAryk5cm9xtzfOliNqLTIcA==", "cpu": [ "x64" ], - "libc": [ - "musl" - ], "license": "SEE LICENSE IN LICENSE.md", "optional": true, "os": [ @@ -214,9 +202,9 @@ ] }, "node_modules/@anthropic-ai/claude-agent-sdk-win32-arm64": { - "version": "0.2.131", - "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-win32-arm64/-/claude-agent-sdk-win32-arm64-0.2.131.tgz", - "integrity": "sha512-LDXYMqR3T1JtaIusmVDr6e539IhE+IULKYBiLC7+v7VvLG6niP1cC+4W/zYZRnUcbzUgcfoIi1FvrWhtF6/M+A==", + "version": "0.2.136", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-win32-arm64/-/claude-agent-sdk-win32-arm64-0.2.136.tgz", + "integrity": "sha512-TWSeel2tr+VxA3S8PlBrwN/MYU93VZM+G/qu+mriloOFXJv3fzbycDAbUf8WnvGPrAoAbKbej11vo3mmtPiDyw==", "cpu": [ "arm64" ], @@ -227,9 +215,9 @@ ] }, "node_modules/@anthropic-ai/claude-agent-sdk-win32-x64": { - "version": "0.2.131", - "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-win32-x64/-/claude-agent-sdk-win32-x64-0.2.131.tgz", - "integrity": "sha512-gwLUkQWtK9Un2i9mWWQgoaEk+2rzamiH3r4j7aoTyVzB4ZQgxdBBOP9ac5o9pIwQE+vflr0HvKk1O54Z320Vng==", + "version": "0.2.136", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-win32-x64/-/claude-agent-sdk-win32-x64-0.2.136.tgz", + "integrity": "sha512-2xs5a7T58bEMxcCu3cDO2yKYQdhNJDjqJc479PXjL7CqQb3g9VnDf+SXsnWTbeLwCGqxSOVulhCBkC7T9nYTrg==", "cpu": [ "x64" ], @@ -6888,12 +6876,12 @@ } }, "node_modules/express-rate-limit": { - "version": "8.5.0", - "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-8.5.0.tgz", - "integrity": "sha512-XKhFohWaSBdVJNTi5TaHziqnPkv04I9UQV6q1Wy7Ui6GGQZVW12ojDFwqer14EvCXxjvPG0CyWXx7cAXpALB4Q==", + "version": "8.5.1", + "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-8.5.1.tgz", + "integrity": "sha512-5O6KYmyJEpuPJV5hNTXKbAHWRqrzyu+OI3vUnSd2kXFubIVpG7ezpgxQy76Zo5GQZtrQBg86hF+CM/NX+cioiQ==", "license": "MIT", "dependencies": { - "ip-address": "10.1.0" + "ip-address": "^10.2.0" }, "engines": { "node": ">= 16" @@ -6957,7 +6945,9 @@ "license": "MIT" }, "node_modules/fast-uri": { - "version": "3.1.0", + "version": "3.1.2", + "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.2.tgz", + "integrity": "sha512-rVjf7ArG3LTk+FS6Yw81V1DLuZl1bRbNrev6Tmd/9RaroeeRRJhAt7jg/6YFxbvAQXUCavSoZhPPj6oOx+5KjQ==", "funding": [ { "type": "github", @@ -7435,9 +7425,9 @@ } }, "node_modules/hono": { - "version": "4.12.14", - "resolved": "https://registry.npmjs.org/hono/-/hono-4.12.14.tgz", - "integrity": "sha512-am5zfg3yu6sqn5yjKBNqhnTX7Cv+m00ox+7jbaKkrLMRJ4rAdldd1xPd/JzbBWspqaQv6RSTrgFN95EsfhC+7w==", + "version": "4.12.18", + "resolved": "https://registry.npmjs.org/hono/-/hono-4.12.18.tgz", + "integrity": "sha512-RWzP96k/yv0PQfyXnWjs6zot20TqfpfsNXhOnev8d1InAxubW93L11/oNUc3tQqn2G0bSdAOBpX+2uDFHV7kdQ==", "license": "MIT", "engines": { "node": ">=16.9.0" @@ -7650,9 +7640,9 @@ } }, "node_modules/ip-address": { - "version": "10.1.0", - "resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.1.0.tgz", - "integrity": "sha512-XXADHxXmvT9+CRxhXg56LJovE+bmWnEWB78LB83VZTprKTmaC5QfruXocxzTZ2Kl0DNwKuBdlIhjL8LeY8Sf8Q==", + "version": "10.2.0", + "resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.2.0.tgz", + "integrity": "sha512-/+S6j4E9AHvW9SWMSEY9Xfy66O5PWvVEJ08O0y5JGyEKQpojb0K0GKpz/v5HJ/G0vi3D2sjGK78119oXZeE0qA==", "license": "MIT", "engines": { "node": ">= 12" diff --git a/src/triggers/shared/agent-execution-lifecycle.ts b/src/triggers/shared/agent-execution-lifecycle.ts new file mode 100644 index 000000000..930742115 --- /dev/null +++ b/src/triggers/shared/agent-execution-lifecycle.ts @@ -0,0 +1,159 @@ +import type { LifecycleHooks } from '../../agents/definitions/schema.js'; +import type { PMLifecycleManager } from '../../pm/index.js'; +import type { AgentResult, ProjectConfig } from '../../types/index.js'; +import { logger } from '../../utils/logging.js'; +import type { TriggerResult } from '../types.js'; +import type { AgentExecutionConfig, AgentExecutionContext } from './agent-execution-types.js'; +import { handleAgentResultArtifacts } from './agent-result-handler.js'; +import { checkBudgetExceeded } from './budget.js'; +import { + formatValidationErrors, + type ValidationResult, + validateIntegrations, +} from './integration-validation.js'; + +interface ValidationLifecycleParams { + result: TriggerResult; + project: ProjectConfig; + executionConfig: AgentExecutionConfig; + agentType: string; + lifecycle: PMLifecycleManager; +} + +/** + * Notify PM and source-specific callbacks when integration validation fails before the agent runs. + */ +async function notifyValidationFailure( + result: TriggerResult, + validation: ValidationResult, + lifecycle: PMLifecycleManager, + executionConfig: AgentExecutionConfig, + agentType: string, + projectId: string, +): Promise { + const errorMessage = formatValidationErrors(validation); + logger.error('Integration validation failed', { + agentType, + projectId, + errors: validation.errors, + }); + + // Only notify via PM if PM validation passed (otherwise PM isn't configured) + const pmFailed = validation.errors.some((e) => e.category === 'pm'); + if (result.workItemId && !pmFailed) { + await lifecycle.handleFailure(result.workItemId, errorMessage); + } + + // Call onFailure callback (for GitHub PR updates) + if (executionConfig.onFailure) { + await executionConfig.onFailure(result, { success: false, output: '', error: errorMessage }); + } +} + +/** + * Run pre-flight integration validation and apply the existing failure notification semantics. + * Returns false when execution should stop before preparing or running the agent. + */ +export async function validateAgentExecutionLifecycle({ + result, + project, + executionConfig, + agentType, + lifecycle, +}: ValidationLifecycleParams): Promise { + const validation = await validateIntegrations(project.id, agentType, project); + if (validation.valid) return true; + + await notifyValidationFailure( + result, + validation, + lifecycle, + executionConfig, + agentType, + project.id, + ); + return false; +} + +/** + * Check the budget before running an agent. + * Returns the remaining budget if not exceeded, or abort=true when the agent + * must not start and the lifecycle manager has been notified. + */ +export async function checkPreRunBudget( + workItemId: string, + project: ProjectConfig, + lifecycle: PMLifecycleManager, +): Promise<{ remainingBudgetUsd: number | undefined; abort: boolean }> { + const budgetCheck = await checkBudgetExceeded(workItemId, project); + if (budgetCheck?.exceeded) { + logger.warn('Budget exceeded, agent not started', { + workItemId, + currentCost: budgetCheck.currentCost, + budget: budgetCheck.budget, + }); + await lifecycle.handleBudgetExceeded(workItemId, budgetCheck.currentCost, budgetCheck.budget); + return { remainingBudgetUsd: undefined, abort: true }; + } + return { remainingBudgetUsd: budgetCheck?.remaining, abort: false }; +} + +/** + * Run pre-agent lifecycle steps owned by the PM lifecycle manager. + */ +export async function prepareAgentExecutionLifecycle( + context: AgentExecutionContext, +): Promise { + if (context.workItemId && !context.executionConfig.skipPrepareForAgent) { + await context.lifecycle.prepareForAgent(context.workItemId, context.lifecycleHooks); + } +} + +/** + * Run post-agent lifecycle steps: artifact handling, budget warning, cleanup, success/failure. + */ +export async function runPostAgentExecutionLifecycle( + workItemId: string, + agentType: string, + agentResult: AgentResult, + project: ProjectConfig, + lifecycle: PMLifecycleManager, + lifecycleHooks: LifecycleHooks, + executionConfig: AgentExecutionConfig, +): Promise { + const { + skipPrepareForAgent = false, + skipHandleFailure = false, + handleSuccessOnlyForAgentType, + } = executionConfig; + + await handleAgentResultArtifacts(workItemId, agentType, agentResult, project); + + const postBudgetCheck = await checkBudgetExceeded(workItemId, project); + if (postBudgetCheck?.exceeded) { + await lifecycle.handleBudgetWarning( + workItemId, + postBudgetCheck.currentCost, + postBudgetCheck.budget, + ); + } + + if (!skipPrepareForAgent) { + await lifecycle.cleanupProcessing(workItemId); + } + + const shouldCallHandleSuccess = + agentResult.success && + (!handleSuccessOnlyForAgentType || agentType === handleSuccessOnlyForAgentType); + + if (shouldCallHandleSuccess) { + await lifecycle.handleSuccess( + workItemId, + lifecycleHooks, + agentResult.prUrl, + agentResult.progressCommentId, + ); + } else if (!agentResult.success && !skipHandleFailure) { + await lifecycle.handleFailure(workItemId, agentResult.error); + } +} diff --git a/src/triggers/shared/agent-execution.ts b/src/triggers/shared/agent-execution.ts index 0f52b6fd9..d853c6f84 100644 --- a/src/triggers/shared/agent-execution.ts +++ b/src/triggers/shared/agent-execution.ts @@ -18,8 +18,13 @@ import { logger } from '../../utils/logging.js'; import { extractPRNumber } from '../../utils/prUrl.js'; import { parseRepoFullName } from '../../utils/repo.js'; import type { TriggerResult } from '../types.js'; +import { + checkPreRunBudget, + prepareAgentExecutionLifecycle, + runPostAgentExecutionLifecycle, + validateAgentExecutionLifecycle, +} from './agent-execution-lifecycle.js'; import type { AgentExecutionConfig, AgentExecutionContext } from './agent-execution-types.js'; -import { handleAgentResultArtifacts } from './agent-result-handler.js'; import { linkPRPostExecution, persistPreRunWorkItems, @@ -27,119 +32,11 @@ import { resolveWorkItemId, } from './agent-work-items.js'; import { isPipelineAtCapacity } from './backlog-check.js'; -import { checkBudgetExceeded } from './budget.js'; import { triggerDebugAnalysis } from './debug-runner.js'; import { shouldTriggerDebug } from './debug-trigger.js'; -import { - formatValidationErrors, - type ValidationResult, - validateIntegrations, -} from './integration-validation.js'; export type { AgentExecutionConfig } from './agent-execution-types.js'; -/** - * Check the budget before running an agent. - * Returns the remaining budget if not exceeded, or null to signal the caller - * should abort (budget exceeded and lifecycle notified). - */ -async function checkPreRunBudget( - workItemId: string, - project: ProjectConfig, - lifecycle: PMLifecycleManager, -): Promise<{ remainingBudgetUsd: number | undefined; abort: boolean }> { - const budgetCheck = await checkBudgetExceeded(workItemId, project); - if (budgetCheck?.exceeded) { - logger.warn('Budget exceeded, agent not started', { - workItemId, - currentCost: budgetCheck.currentCost, - budget: budgetCheck.budget, - }); - await lifecycle.handleBudgetExceeded(workItemId, budgetCheck.currentCost, budgetCheck.budget); - return { remainingBudgetUsd: undefined, abort: true }; - } - return { remainingBudgetUsd: budgetCheck?.remaining, abort: false }; -} - -/** - * Run post-agent lifecycle steps: artifact handling, budget warning, cleanup, success/failure. - */ -async function runPostAgentLifecycle( - workItemId: string, - agentType: string, - agentResult: AgentResult, - project: ProjectConfig, - lifecycle: PMLifecycleManager, - lifecycleHooks: LifecycleHooks, - executionConfig: AgentExecutionConfig, -): Promise { - const { - skipPrepareForAgent = false, - skipHandleFailure = false, - handleSuccessOnlyForAgentType, - } = executionConfig; - - await handleAgentResultArtifacts(workItemId, agentType, agentResult, project); - - const postBudgetCheck = await checkBudgetExceeded(workItemId, project); - if (postBudgetCheck?.exceeded) { - await lifecycle.handleBudgetWarning( - workItemId, - postBudgetCheck.currentCost, - postBudgetCheck.budget, - ); - } - - if (!skipPrepareForAgent) { - await lifecycle.cleanupProcessing(workItemId); - } - - const shouldCallHandleSuccess = - agentResult.success && - (!handleSuccessOnlyForAgentType || agentType === handleSuccessOnlyForAgentType); - - if (shouldCallHandleSuccess) { - await lifecycle.handleSuccess( - workItemId, - lifecycleHooks, - agentResult.prUrl, - agentResult.progressCommentId, - ); - } else if (!agentResult.success && !skipHandleFailure) { - await lifecycle.handleFailure(workItemId, agentResult.error); - } -} - -/** - * Notify PM and GitHub when integration validation fails before the agent runs. - */ -async function notifyValidationFailure( - result: TriggerResult, - validation: ValidationResult, - lifecycle: PMLifecycleManager, - executionConfig: AgentExecutionConfig, - agentType: string, - projectId: string, -): Promise { - const errorMessage = formatValidationErrors(validation); - logger.error('Integration validation failed', { - agentType, - projectId, - errors: validation.errors, - }); - - // Only notify via PM if PM validation passed (otherwise PM isn't configured) - const pmFailed = validation.errors.some((e) => e.category === 'pm'); - if (result.workItemId && !pmFailed) { - await lifecycle.handleFailure(result.workItemId, errorMessage); - } - - // Call onFailure callback (for GitHub PR updates) - if (executionConfig.onFailure) { - await executionConfig.onFailure(result, { success: false, output: '', error: errorMessage }); - } -} - /** * Dispatch a review agent after a successful implementation run, if the PR's * CI is green and no review has been dispatched yet. @@ -362,21 +259,18 @@ export async function runAgentExecutionPipeline( }); } - // Pre-flight integration validation - const validation = await validateIntegrations(project.id, agentType, project); - if (!validation.valid) { - await notifyValidationFailure( - result, - validation, - lifecycle, - executionConfig, - agentType, - project.id, - ); + const canExecute = await validateAgentExecutionLifecycle({ + result, + project, + agentType, + lifecycle, + executionConfig, + }); + if (!canExecute) { return; } - const { skipPrepareForAgent = false, onSuccess, onFailure, logLabel = 'Agent' } = executionConfig; + const { onSuccess, onFailure, logLabel = 'Agent' } = executionConfig; // Re-resolve workItemId at run time. The trigger handler (e.g. PROpenedTrigger) // captures workItemId synchronously at webhook arrival, before any other @@ -424,13 +318,7 @@ export async function runAgentExecutionPipeline( } await persistPreRunWorkItems(result, project, workItemId); - - if (executionContext.workItemId && !skipPrepareForAgent) { - await executionContext.lifecycle.prepareForAgent( - executionContext.workItemId, - executionContext.lifecycleHooks, - ); - } + await prepareAgentExecutionLifecycle(executionContext); const agentResult = await runAgent(executionContext.agentType, { ...executionContext.agentInput, @@ -453,7 +341,7 @@ export async function runAgentExecutionPipeline( await postAgentSummaryToPM(agentType, agentResult, workItemId, project.id, result.prNumber); if (workItemId) { - await runPostAgentLifecycle( + await runPostAgentExecutionLifecycle( workItemId, agentType, agentResult, diff --git a/tests/unit/triggers/shared/agent-execution-lifecycle.test.ts b/tests/unit/triggers/shared/agent-execution-lifecycle.test.ts new file mode 100644 index 000000000..f2dd7278b --- /dev/null +++ b/tests/unit/triggers/shared/agent-execution-lifecycle.test.ts @@ -0,0 +1,283 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const { + mockValidateIntegrations, + mockFormatValidationErrors, + mockCheckBudgetExceeded, + mockHandleAgentResultArtifacts, + mockLogger, +} = vi.hoisted(() => ({ + mockValidateIntegrations: vi.fn(), + mockFormatValidationErrors: vi.fn(), + mockCheckBudgetExceeded: vi.fn(), + mockHandleAgentResultArtifacts: vi.fn(), + mockLogger: { + error: vi.fn(), + warn: vi.fn(), + info: vi.fn(), + debug: vi.fn(), + }, +})); + +vi.mock('../../../../src/triggers/shared/integration-validation.js', () => ({ + validateIntegrations: mockValidateIntegrations, + formatValidationErrors: mockFormatValidationErrors, +})); + +vi.mock('../../../../src/triggers/shared/budget.js', () => ({ + checkBudgetExceeded: mockCheckBudgetExceeded, +})); + +vi.mock('../../../../src/triggers/shared/agent-result-handler.js', () => ({ + handleAgentResultArtifacts: mockHandleAgentResultArtifacts, +})); + +vi.mock('../../../../src/utils/logging.js', () => ({ + logger: mockLogger, +})); + +import type { PMLifecycleManager } from '../../../../src/pm/index.js'; +import { + checkPreRunBudget, + prepareAgentExecutionLifecycle, + runPostAgentExecutionLifecycle, + validateAgentExecutionLifecycle, +} from '../../../../src/triggers/shared/agent-execution-lifecycle.js'; +import type { AgentExecutionContext } from '../../../../src/triggers/shared/agent-execution-types.js'; +import type { TriggerResult } from '../../../../src/triggers/types.js'; +import type { AgentResult, CascadeConfig, ProjectConfig } from '../../../../src/types/index.js'; + +const PROJECT = { + id: 'project-1', + pm: { type: 'trello' }, +} as ProjectConfig; + +const CONFIG = { projects: [PROJECT] } as CascadeConfig; + +const RESULT: TriggerResult = { + agentType: 'implementation', + workItemId: 'card-1', + agentInput: {}, +}; + +function createLifecycle() { + return { + prepareForAgent: vi.fn().mockResolvedValue(undefined), + handleSuccess: vi.fn().mockResolvedValue(undefined), + handleFailure: vi.fn().mockResolvedValue(undefined), + handleBudgetExceeded: vi.fn().mockResolvedValue(undefined), + handleBudgetWarning: vi.fn().mockResolvedValue(undefined), + cleanupProcessing: vi.fn().mockResolvedValue(undefined), + } as unknown as PMLifecycleManager & Record>; +} + +function createContext( + lifecycle: PMLifecycleManager, + overrides: Partial = {}, +): AgentExecutionContext { + return { + result: RESULT, + project: PROJECT, + config: CONFIG, + executionConfig: {}, + agentType: 'implementation', + logLabel: 'Agent', + lifecycle, + lifecycleHooks: {}, + workItemId: 'card-1', + agentInput: {}, + ...overrides, + }; +} + +describe('agent execution lifecycle helper', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockValidateIntegrations.mockResolvedValue({ valid: true, errors: [] }); + mockFormatValidationErrors.mockReturnValue('validation error'); + mockCheckBudgetExceeded.mockResolvedValue(null); + mockHandleAgentResultArtifacts.mockResolvedValue(undefined); + }); + + it('allows execution when integration validation succeeds', async () => { + const lifecycle = createLifecycle(); + + await expect( + validateAgentExecutionLifecycle({ + result: RESULT, + project: PROJECT, + executionConfig: {}, + agentType: 'implementation', + lifecycle, + }), + ).resolves.toBe(true); + + expect(mockValidateIntegrations).toHaveBeenCalledWith('project-1', 'implementation', PROJECT); + expect(lifecycle.handleFailure).not.toHaveBeenCalled(); + }); + + it('formats, logs, notifies PM, and invokes onFailure when non-PM validation fails', async () => { + const lifecycle = createLifecycle(); + const onFailure = vi.fn().mockResolvedValue(undefined); + const validation = { + valid: false, + errors: [{ category: 'scm' as const, message: 'GitHub token missing' }], + }; + mockValidateIntegrations.mockResolvedValueOnce(validation); + + await expect( + validateAgentExecutionLifecycle({ + result: RESULT, + project: PROJECT, + executionConfig: { onFailure }, + agentType: 'implementation', + lifecycle, + }), + ).resolves.toBe(false); + + expect(mockFormatValidationErrors).toHaveBeenCalledWith(validation); + expect(mockLogger.error).toHaveBeenCalledWith('Integration validation failed', { + agentType: 'implementation', + projectId: 'project-1', + errors: validation.errors, + }); + expect(lifecycle.handleFailure).toHaveBeenCalledWith('card-1', 'validation error'); + expect(onFailure).toHaveBeenCalledWith(RESULT, { + success: false, + output: '', + error: 'validation error', + }); + }); + + it('does not notify PM when PM validation failed', async () => { + const lifecycle = createLifecycle(); + mockValidateIntegrations.mockResolvedValueOnce({ + valid: false, + errors: [{ category: 'pm' as const, message: 'Trello missing' }], + }); + + await validateAgentExecutionLifecycle({ + result: RESULT, + project: PROJECT, + executionConfig: {}, + agentType: 'implementation', + lifecycle, + }); + + expect(lifecycle.handleFailure).not.toHaveBeenCalled(); + }); + + it('handles pre-run budget exceedance and tells callers to abort', async () => { + const lifecycle = createLifecycle(); + mockCheckBudgetExceeded.mockResolvedValueOnce({ + exceeded: true, + currentCost: 6, + budget: 5, + remaining: 0, + }); + + const result = await checkPreRunBudget('card-1', PROJECT, lifecycle); + + expect(result).toEqual({ remainingBudgetUsd: undefined, abort: true }); + expect(lifecycle.handleBudgetExceeded).toHaveBeenCalledWith('card-1', 6, 5); + }); + + it('returns remaining budget when pre-run budget is not exceeded', async () => { + const lifecycle = createLifecycle(); + mockCheckBudgetExceeded.mockResolvedValueOnce({ + exceeded: false, + currentCost: 2, + budget: 5, + remaining: 3, + }); + + await expect(checkPreRunBudget('card-1', PROJECT, lifecycle)).resolves.toEqual({ + remainingBudgetUsd: 3, + abort: false, + }); + expect(lifecycle.handleBudgetExceeded).not.toHaveBeenCalled(); + }); + + it('runs prepareForAgent unless skipPrepareForAgent is set', async () => { + const lifecycle = createLifecycle(); + await prepareAgentExecutionLifecycle(createContext(lifecycle)); + await prepareAgentExecutionLifecycle( + createContext(lifecycle, { executionConfig: { skipPrepareForAgent: true } }), + ); + + expect(lifecycle.prepareForAgent).toHaveBeenCalledTimes(1); + expect(lifecycle.prepareForAgent).toHaveBeenCalledWith('card-1', {}); + }); + + it('runs post-agent artifacts, budget warning, cleanup, and success in order', async () => { + const lifecycle = createLifecycle(); + const agentResult: AgentResult = { + success: true, + output: '', + prUrl: 'https://github.com/acme/myapp/pull/1', + progressCommentId: 'progress-1', + }; + mockCheckBudgetExceeded.mockResolvedValueOnce({ + exceeded: true, + currentCost: 5.5, + budget: 5, + remaining: 0, + }); + + await runPostAgentExecutionLifecycle( + 'card-1', + 'implementation', + agentResult, + PROJECT, + lifecycle, + {}, + {}, + ); + + expect(mockHandleAgentResultArtifacts).toHaveBeenCalledWith( + 'card-1', + 'implementation', + agentResult, + PROJECT, + ); + expect(lifecycle.handleBudgetWarning).toHaveBeenCalledWith('card-1', 5.5, 5); + expect(lifecycle.cleanupProcessing).toHaveBeenCalledWith('card-1'); + expect(lifecycle.handleSuccess).toHaveBeenCalledWith( + 'card-1', + {}, + 'https://github.com/acme/myapp/pull/1', + 'progress-1', + ); + expect(mockHandleAgentResultArtifacts.mock.invocationCallOrder[0]).toBeLessThan( + lifecycle.handleBudgetWarning.mock.invocationCallOrder[0], + ); + expect(lifecycle.handleBudgetWarning.mock.invocationCallOrder[0]).toBeLessThan( + lifecycle.cleanupProcessing.mock.invocationCallOrder[0], + ); + expect(lifecycle.cleanupProcessing.mock.invocationCallOrder[0]).toBeLessThan( + lifecycle.handleSuccess.mock.invocationCallOrder[0], + ); + }); + + it('preserves GitHub-style post-run skip options', async () => { + const lifecycle = createLifecycle(); + + await runPostAgentExecutionLifecycle( + 'card-1', + 'review', + { success: false, output: '', error: 'review failed' }, + PROJECT, + lifecycle, + {}, + { + skipPrepareForAgent: true, + skipHandleFailure: true, + handleSuccessOnlyForAgentType: 'implementation', + }, + ); + + expect(lifecycle.cleanupProcessing).not.toHaveBeenCalled(); + expect(lifecycle.handleSuccess).not.toHaveBeenCalled(); + expect(lifecycle.handleFailure).not.toHaveBeenCalled(); + }); +}); From 43c6b6c573a96200d8b512d469c71a1ae87e1fc0 Mon Sep 17 00:00:00 2001 From: aaight Date: Fri, 8 May 2026 22:05:02 +0200 Subject: [PATCH 3/5] test(triggers): pin trigger contract regressions (#1272) * test(triggers): pin trigger contract regressions * fix(triggers): replace mocked uniqueness check with real-implementation test Move the built-in trigger handler name uniqueness regression out of builtins.test.ts (which mocks listPMProviders() and every trigger module, making it incapable of catching real production duplicates) into a new builtins-uniqueness.test.ts that side-effect-imports the real Trello, JIRA, and Linear manifests and calls registerBuiltInTriggers with real handler instances. A duplicate name in any src/triggers/ handler or PM manifest will now fail CI. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Cascade Bot Co-authored-by: Claude Sonnet 4.6 --- .../unit/triggers/builtins-uniqueness.test.ts | 57 +++++++++++++++++++ .../trigger-event-consistency.test.ts | 36 ++++++++++++ .../trigger-work-item-id-consistency.test.ts | 36 ++++++++++++ 3 files changed, 129 insertions(+) create mode 100644 tests/unit/triggers/builtins-uniqueness.test.ts diff --git a/tests/unit/triggers/builtins-uniqueness.test.ts b/tests/unit/triggers/builtins-uniqueness.test.ts new file mode 100644 index 000000000..8015187ed --- /dev/null +++ b/tests/unit/triggers/builtins-uniqueness.test.ts @@ -0,0 +1,57 @@ +/** + * Regression: built-in trigger handler name uniqueness with real implementations. + * + * Unlike builtins.test.ts — which mocks every trigger module and + * listPMProviders() so it can assert on ordering/counts — this file uses the + * real production PM manifests (Trello, JIRA, Linear) and real GitHub/Sentry + * trigger classes. That means a duplicate name introduced anywhere in + * src/triggers/ or in a PM manifest's triggerHandlers array will fail this + * test at CI time, not just when the mock is also updated. + */ + +import { describe, expect, it } from 'vitest'; + +// Side-effect imports: register the real production PM providers so +// listPMProviders() (called inside registerBuiltInTriggers) returns the +// actual Trello, JIRA, and Linear manifests with their real handler names. +import '../../../src/integrations/pm/trello/index.js'; +import '../../../src/integrations/pm/jira/index.js'; +import '../../../src/integrations/pm/linear/index.js'; + +import { registerBuiltInTriggers } from '../../../src/triggers/builtins.js'; +import type { TriggerRegistry } from '../../../src/triggers/registry.js'; + +function createCollectingRegistry(): { + register: (handler: unknown) => void; + handlers: unknown[]; +} { + const handlers: unknown[] = []; + return { + register: (handler: unknown) => handlers.push(handler), + handlers, + }; +} + +describe('registerBuiltInTriggers — real handler name uniqueness', () => { + it('registers each built-in trigger handler name exactly once across PM, SCM, and alerting', () => { + const registry = createCollectingRegistry(); + + registerBuiltInTriggers(registry as unknown as TriggerRegistry); + + const registeredNames = registry.handlers.map((h) => (h as { name: string }).name); + const countsByName = new Map(); + for (const name of registeredNames) { + countsByName.set(name, (countsByName.get(name) ?? 0) + 1); + } + + const duplicates = [...countsByName.entries()] + .filter(([, count]) => count > 1) + .map(([name, count]) => `${name} (${count} registrations)`); + + expect( + duplicates, + `Built-in trigger handler names must be globally unique across PM, SCM, alerting, and internal registrations.\n` + + `Duplicate names:\n${duplicates.map((d) => ` - ${d}`).join('\n')}`, + ).toEqual([]); + }); +}); diff --git a/tests/unit/triggers/trigger-event-consistency.test.ts b/tests/unit/triggers/trigger-event-consistency.test.ts index d078fb0f0..ed1c925da 100644 --- a/tests/unit/triggers/trigger-event-consistency.test.ts +++ b/tests/unit/triggers/trigger-event-consistency.test.ts @@ -28,6 +28,29 @@ import { describe, expect, it } from 'vitest'; const TRIGGERS_ROOT = join(__dirname, '..', '..', '..', 'src', 'triggers'); +const BUILT_IN_TRIGGER_FILES = [ + 'github/check-suite-failure.ts', + 'github/check-suite-success.ts', + 'github/pr-comment-mention.ts', + 'github/pr-conflict-detected.ts', + 'github/pr-merged.ts', + 'github/pr-opened.ts', + 'github/pr-ready-to-merge.ts', + 'github/pr-review-submitted.ts', + 'github/review-requested.ts', + 'jira/comment-mention.ts', + 'jira/label-added.ts', + 'jira/status-changed.ts', + 'linear/comment-mention.ts', + 'linear/label-added.ts', + 'linear/status-changed.ts', + 'sentry/alerting-issue.ts', + 'sentry/alerting-metric.ts', + 'trello/comment-mention.ts', + 'trello/label-added.ts', + 'trello/status-changed.ts', +].map((path) => join(TRIGGERS_ROOT, path)); + // Handlers that legitimately gate on one event without emitting it as a // `triggerEvent`. Add an entry ONLY when there's a real reason — every // exemption silently weakens the guard. @@ -101,6 +124,19 @@ describe('trigger-event-string consistency (static guard)', () => { expect(scans.length).toBeGreaterThan(10); }); + it('covers every current built-in trigger source file', () => { + const scannedFiles = new Set(allFiles); + const missingFiles = BUILT_IN_TRIGGER_FILES.filter((file) => !scannedFiles.has(file)); + + expect( + missingFiles.map((file) => file.replace(`${TRIGGERS_ROOT}/`, 'src/triggers/')), + `trigger-event-string consistency must scan every built-in trigger file registered by PM manifests, SCM, and alerting. ` + + `Missing files:\n${missingFiles + .map((file) => ` - ${file.replace(`${TRIGGERS_ROOT}/`, 'src/triggers/')}`) + .join('\n')}`, + ).toEqual([]); + }); + for (const scan of scans) { const relPath = scan.file.replace(`${TRIGGERS_ROOT}/`, 'src/triggers/'); diff --git a/tests/unit/triggers/trigger-work-item-id-consistency.test.ts b/tests/unit/triggers/trigger-work-item-id-consistency.test.ts index 178185713..a8f166376 100644 --- a/tests/unit/triggers/trigger-work-item-id-consistency.test.ts +++ b/tests/unit/triggers/trigger-work-item-id-consistency.test.ts @@ -34,6 +34,29 @@ import { describe, expect, it } from 'vitest'; const TRIGGERS_ROOT = join(__dirname, '..', '..', '..', 'src', 'triggers'); +const BUILT_IN_TRIGGER_FILES = [ + 'github/check-suite-failure.ts', + 'github/check-suite-success.ts', + 'github/pr-comment-mention.ts', + 'github/pr-conflict-detected.ts', + 'github/pr-merged.ts', + 'github/pr-opened.ts', + 'github/pr-ready-to-merge.ts', + 'github/pr-review-submitted.ts', + 'github/review-requested.ts', + 'jira/comment-mention.ts', + 'jira/label-added.ts', + 'jira/status-changed.ts', + 'linear/comment-mention.ts', + 'linear/label-added.ts', + 'linear/status-changed.ts', + 'sentry/alerting-issue.ts', + 'sentry/alerting-metric.ts', + 'trello/comment-mention.ts', + 'trello/label-added.ts', + 'trello/status-changed.ts', +].map((path) => join(TRIGGERS_ROOT, path)); + interface Violation { file: string; line: number; @@ -137,6 +160,19 @@ describe('trigger workItemId consistency (static guard)', () => { expect(allFiles.length).toBeGreaterThan(10); }); + it('covers every current built-in trigger source file', () => { + const scannedFiles = new Set(allFiles); + const missingFiles = BUILT_IN_TRIGGER_FILES.filter((file) => !scannedFiles.has(file)); + + expect( + missingFiles.map((file) => file.replace(`${TRIGGERS_ROOT}/`, 'src/triggers/')), + `trigger workItemId consistency must scan every built-in trigger file registered by PM manifests, SCM, and alerting. ` + + `Missing files:\n${missingFiles + .map((file) => ` - ${file.replace(`${TRIGGERS_ROOT}/`, 'src/triggers/')}`) + .join('\n')}`, + ).toEqual([]); + }); + const allViolations = allFiles.flatMap(scanFile); it('every TriggerResult with a top-level workItemId also carries it inside agentInput', () => { From 0ba0524364d217e2d9e228d935786d618ba32aa8 Mon Sep 17 00:00:00 2001 From: aaight Date: Fri, 8 May 2026 22:31:35 +0200 Subject: [PATCH 4/5] feat(triggers): add canonical events and result builders (#1273) * feat(triggers): add canonical events and result builders * fix(triggers): mirror prNumber into agentInput in buildGitHubPRDispatchResult The builder required `prNumber` but only wrote it to the top-level TriggerResult. The worker runs agents from `executionContext.agentInput`, and the existing safety net only mirrors `workItemId`, not `prNumber`. A migrated handler relying on the required option could therefore enqueue a PR agent with no `input.prNumber`, breaking PR context assembly and refs/pull checkout. Extend `buildAgentInput` to accept an optional `prNumber` and pass the option through so it is set in `agentInput` with the option value winning over any stale caller-supplied value (matching the work-item mirroring behaviour). Add tests covering both the injection-when-absent and the stale-value-override cases. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Cascade Bot Co-authored-by: Claude Sonnet 4.6 --- src/triggers/README.md | 2 + src/triggers/shared/events.ts | 45 ++++++ src/triggers/shared/result-builders.ts | 144 +++++++++++++++++ src/triggers/shared/skip.ts | 7 +- src/types/index.ts | 3 +- tests/unit/triggers/shared/events.test.ts | 50 ++++++ .../triggers/shared/result-builders.test.ts | 146 ++++++++++++++++++ 7 files changed, 391 insertions(+), 6 deletions(-) create mode 100644 src/triggers/shared/events.ts create mode 100644 src/triggers/shared/result-builders.ts create mode 100644 tests/unit/triggers/shared/events.test.ts create mode 100644 tests/unit/triggers/shared/result-builders.test.ts diff --git a/src/triggers/README.md b/src/triggers/README.md index 78a6813bc..ed377826f 100644 --- a/src/triggers/README.md +++ b/src/triggers/README.md @@ -77,6 +77,8 @@ To reduce duplication across the three worker-side handlers, shared utilities ar | `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 | GitHub worker handler | +| `events.ts` | `TRIGGER_EVENTS` — typed catalog of canonical trigger event names | Trigger handlers and tests | +| `result-builders.ts` | Builders for dispatch, skip, no-agent, and deferred re-check `TriggerResult` shapes | Trigger handlers and tests | | `agent-execution.ts` | `runAgentExecutionPipeline()` — full agent lifecycle | All handlers (via `webhook-execution.ts`) | | `webhook-execution.ts` | `runAgentWithCredentials()` — LLM keys + credentials + pipeline | GitHub, PM | diff --git a/src/triggers/shared/events.ts b/src/triggers/shared/events.ts new file mode 100644 index 000000000..f2d421ac3 --- /dev/null +++ b/src/triggers/shared/events.ts @@ -0,0 +1,45 @@ +export const TRIGGER_EVENTS = { + PM: { + STATUS_CHANGED: 'pm:status-changed', + LABEL_ADDED: 'pm:label-added', + COMMENT_MENTION: 'pm:comment-mention', + }, + SCM: { + CHECK_SUITE_SUCCESS: 'scm:check-suite-success', + CHECK_SUITE_FAILURE: 'scm:check-suite-failure', + PR_REVIEW_SUBMITTED: 'scm:pr-review-submitted', + REVIEW_REQUESTED: 'scm:review-requested', + PR_OPENED: 'scm:pr-opened', + PR_COMMENT_MENTION: 'scm:pr-comment-mention', + PR_MERGED: 'scm:pr-merged', + PR_READY_TO_MERGE: 'scm:pr-ready-to-merge', + PR_CONFLICT_DETECTED: 'scm:pr-conflict-detected', + }, + ALERTING: { + ISSUE_ALERT: 'alerting:issue-alert', + METRIC_ALERT: 'alerting:metric-alert', + }, + INTERNAL: { + AUTO_CHAIN: 'internal:auto-chain', + }, +} as const; + +type ObjectValues = T[keyof T]; + +export type PMTriggerEvent = ObjectValues; +export type SCMTriggerEvent = ObjectValues; +export type AlertingTriggerEvent = ObjectValues; +export type InternalTriggerEvent = ObjectValues; + +export type CanonicalTriggerEvent = + | PMTriggerEvent + | SCMTriggerEvent + | AlertingTriggerEvent + | InternalTriggerEvent; + +export const ALL_TRIGGER_EVENTS = [ + ...Object.values(TRIGGER_EVENTS.PM), + ...Object.values(TRIGGER_EVENTS.SCM), + ...Object.values(TRIGGER_EVENTS.ALERTING), + ...Object.values(TRIGGER_EVENTS.INTERNAL), +] as CanonicalTriggerEvent[]; diff --git a/src/triggers/shared/result-builders.ts b/src/triggers/shared/result-builders.ts new file mode 100644 index 000000000..2ded64be9 --- /dev/null +++ b/src/triggers/shared/result-builders.ts @@ -0,0 +1,144 @@ +import type { AgentInput, TriggerResult } from '../../types/index.js'; +import type { CanonicalTriggerEvent, PMTriggerEvent, SCMTriggerEvent } from './events.js'; + +type AgentType = Exclude; + +interface BaseDispatchOptions { + agentType: AgentType; + agentInput?: AgentInput; + workItemId?: string; + workItemUrl?: string; + workItemTitle?: string; + onBlocked?: TriggerResult['onBlocked']; + coalesceKey?: string; +} + +interface PMDispatchOptions extends BaseDispatchOptions { + triggerEvent: PMTriggerEvent; + workItemId: string; +} + +interface GitHubPRDispatchOptions extends BaseDispatchOptions { + triggerEvent: SCMTriggerEvent; + prNumber: number; + prUrl?: string; + prTitle?: string; +} + +interface NoAgentOptions { + agentInput?: AgentInput; + workItemId?: string; + workItemUrl?: string; + workItemTitle?: string; + prNumber?: number; + prUrl?: string; + prTitle?: string; + lockKey?: string; + coalesceKey?: string; +} + +interface DeferredRecheckOptions { + delayMs: number; + coalesceKey: string; + agentInput?: AgentInput; +} + +function buildAgentInput( + agentInput: AgentInput | undefined, + triggerEvent: CanonicalTriggerEvent, + workItemId: string | undefined, + prNumber?: number, +): AgentInput { + return { + ...agentInput, + triggerEvent, + ...(workItemId ? { workItemId } : {}), + ...(prNumber !== undefined ? { prNumber } : {}), + }; +} + +export function buildPMDispatchResult(options: PMDispatchOptions): TriggerResult { + const { + agentType, + agentInput, + triggerEvent, + workItemId, + workItemUrl, + workItemTitle, + onBlocked, + coalesceKey, + } = options; + + return { + agentType, + agentInput: buildAgentInput(agentInput, triggerEvent, workItemId), + workItemId, + workItemUrl, + workItemTitle, + onBlocked, + coalesceKey, + }; +} + +export function buildGitHubPRDispatchResult(options: GitHubPRDispatchOptions): TriggerResult { + const { + agentType, + agentInput, + triggerEvent, + workItemId, + workItemUrl, + workItemTitle, + prNumber, + prUrl, + prTitle, + onBlocked, + coalesceKey, + } = options; + + return { + agentType, + agentInput: buildAgentInput(agentInput, triggerEvent, workItemId, prNumber), + prNumber, + prUrl, + prTitle, + workItemId, + workItemUrl, + workItemTitle, + onBlocked, + coalesceKey, + }; +} + +export function buildSkipResult(handler: string, message: string): TriggerResult { + return { + agentType: null, + agentInput: {}, + skipReason: { handler, message }, + }; +} + +export function buildNoAgentResult(options: NoAgentOptions = {}): TriggerResult { + return { + agentType: null, + agentInput: options.agentInput ?? {}, + ...(options.workItemId ? { workItemId: options.workItemId } : {}), + ...(options.workItemUrl ? { workItemUrl: options.workItemUrl } : {}), + ...(options.workItemTitle ? { workItemTitle: options.workItemTitle } : {}), + ...(options.prNumber ? { prNumber: options.prNumber } : {}), + ...(options.prUrl ? { prUrl: options.prUrl } : {}), + ...(options.prTitle ? { prTitle: options.prTitle } : {}), + ...(options.lockKey ? { lockKey: options.lockKey } : {}), + ...(options.coalesceKey ? { coalesceKey: options.coalesceKey } : {}), + }; +} + +export function buildDeferredRecheckResult(options: DeferredRecheckOptions): TriggerResult { + return { + agentType: null, + agentInput: options.agentInput ?? {}, + deferredRecheck: { + delayMs: options.delayMs, + coalesceKey: options.coalesceKey, + }, + }; +} diff --git a/src/triggers/shared/skip.ts b/src/triggers/shared/skip.ts index 34f2442ef..46205eb82 100644 --- a/src/triggers/shared/skip.ts +++ b/src/triggers/shared/skip.ts @@ -1,4 +1,5 @@ import type { TriggerResult } from '../../types/index.js'; +import { buildSkipResult } from './result-builders.js'; /** * Build a structured self-skip result so the router's webhook log @@ -13,9 +14,5 @@ import type { TriggerResult } from '../../types/index.js'; * `import { skip } from '../shared/skip.js'` and one call. */ export function skip(handler: string, message: string): TriggerResult { - return { - agentType: null, - agentInput: {}, - skipReason: { handler, message }, - }; + return buildSkipResult(handler, message); } diff --git a/src/types/index.ts b/src/types/index.ts index b3e0ddcdf..2b9c07537 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -1,6 +1,7 @@ import type { z } from 'zod'; import type { CascadeConfigSchema, ProjectConfigSchema } from '../config/schema.js'; import type { PersonaIdentities } from '../github/personas.js'; +import type { CanonicalTriggerEvent } from '../triggers/shared/events.js'; export type ProjectConfig = z.infer; export type CascadeConfig = z.infer; @@ -29,7 +30,7 @@ export interface AgentInput { | 'manual'; /** YAML-format trigger event name for context pipeline resolution (e.g. 'scm:check-suite-success') */ - triggerEvent?: string; + triggerEvent?: CanonicalTriggerEvent | (string & {}); // Debug agent fields logDir?: string; diff --git a/tests/unit/triggers/shared/events.test.ts b/tests/unit/triggers/shared/events.test.ts new file mode 100644 index 000000000..2c5723e1e --- /dev/null +++ b/tests/unit/triggers/shared/events.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from 'vitest'; +import { + ALL_TRIGGER_EVENTS, + type AlertingTriggerEvent, + type InternalTriggerEvent, + type PMTriggerEvent, + type SCMTriggerEvent, + TRIGGER_EVENTS, +} from '../../../../src/triggers/shared/events.js'; + +describe('TRIGGER_EVENTS', () => { + it('catalogs every current trigger event', () => { + expect(ALL_TRIGGER_EVENTS).toEqual([ + 'pm:status-changed', + 'pm:label-added', + 'pm:comment-mention', + 'scm:check-suite-success', + 'scm:check-suite-failure', + 'scm:pr-review-submitted', + 'scm:review-requested', + 'scm:pr-opened', + 'scm:pr-comment-mention', + 'scm:pr-merged', + 'scm:pr-ready-to-merge', + 'scm:pr-conflict-detected', + 'alerting:issue-alert', + 'alerting:metric-alert', + 'internal:auto-chain', + ]); + }); + + it('keeps event values available as TypeScript literals', () => { + const pmEvent = TRIGGER_EVENTS.PM.STATUS_CHANGED satisfies 'pm:status-changed'; + const scmEvent = TRIGGER_EVENTS.SCM.PR_OPENED satisfies 'scm:pr-opened'; + const alertingEvent = TRIGGER_EVENTS.ALERTING.ISSUE_ALERT satisfies 'alerting:issue-alert'; + const internalEvent = TRIGGER_EVENTS.INTERNAL.AUTO_CHAIN satisfies 'internal:auto-chain'; + + const pmTyped: PMTriggerEvent = pmEvent; + const scmTyped: SCMTriggerEvent = scmEvent; + const alertingTyped: AlertingTriggerEvent = alertingEvent; + const internalTyped: InternalTriggerEvent = internalEvent; + + expect([pmTyped, scmTyped, alertingTyped, internalTyped]).toEqual([ + 'pm:status-changed', + 'scm:pr-opened', + 'alerting:issue-alert', + 'internal:auto-chain', + ]); + }); +}); diff --git a/tests/unit/triggers/shared/result-builders.test.ts b/tests/unit/triggers/shared/result-builders.test.ts new file mode 100644 index 000000000..196d4a420 --- /dev/null +++ b/tests/unit/triggers/shared/result-builders.test.ts @@ -0,0 +1,146 @@ +import { describe, expect, it, vi } from 'vitest'; +import { TRIGGER_EVENTS } from '../../../../src/triggers/shared/events.js'; +import { + buildDeferredRecheckResult, + buildGitHubPRDispatchResult, + buildNoAgentResult, + buildPMDispatchResult, + buildSkipResult, +} from '../../../../src/triggers/shared/result-builders.js'; + +describe('trigger result builders', () => { + it('builds PM dispatch results and mirrors workItemId into agentInput', () => { + const result = buildPMDispatchResult({ + agentType: 'implementation', + triggerEvent: TRIGGER_EVENTS.PM.STATUS_CHANGED, + workItemId: 'card-123', + workItemUrl: 'https://trello.com/c/abc', + workItemTitle: 'Implement feature', + agentInput: { + workItemId: 'stale-id', + modelOverride: 'gpt-test', + }, + }); + + expect(result).toEqual({ + agentType: 'implementation', + agentInput: { + workItemId: 'card-123', + modelOverride: 'gpt-test', + triggerEvent: 'pm:status-changed', + }, + workItemId: 'card-123', + workItemUrl: 'https://trello.com/c/abc', + workItemTitle: 'Implement feature', + onBlocked: undefined, + coalesceKey: undefined, + }); + }); + + it('builds GitHub PR dispatch results and mirrors a linked work item into agentInput', () => { + const onBlocked = vi.fn(); + + const result = buildGitHubPRDispatchResult({ + agentType: 'review', + triggerEvent: TRIGGER_EVENTS.SCM.PR_OPENED, + prNumber: 42, + prUrl: 'https://github.com/acme/repo/pull/42', + prTitle: 'feat: add thing', + workItemId: 'card-456', + agentInput: { + prNumber: 42, + repoFullName: 'acme/repo', + }, + onBlocked, + }); + + expect(result).toEqual({ + agentType: 'review', + agentInput: { + prNumber: 42, + repoFullName: 'acme/repo', + triggerEvent: 'scm:pr-opened', + workItemId: 'card-456', + }, + prNumber: 42, + prUrl: 'https://github.com/acme/repo/pull/42', + prTitle: 'feat: add thing', + workItemId: 'card-456', + workItemUrl: undefined, + workItemTitle: undefined, + onBlocked, + coalesceKey: undefined, + }); + }); + + it('mirrors prNumber into agentInput from options even when not present in agentInput', () => { + const result = buildGitHubPRDispatchResult({ + agentType: 'review', + triggerEvent: TRIGGER_EVENTS.SCM.CHECK_SUITE_SUCCESS, + prNumber: 42, + agentInput: { + repoFullName: 'acme/repo', + }, + }); + + expect(result.agentInput).toEqual({ + prNumber: 42, + repoFullName: 'acme/repo', + triggerEvent: 'scm:check-suite-success', + }); + expect(result.workItemId).toBeUndefined(); + }); + + it('mirrors prNumber into agentInput with the option value winning over a stale agentInput.prNumber', () => { + const result = buildGitHubPRDispatchResult({ + agentType: 'review', + triggerEvent: TRIGGER_EVENTS.SCM.PR_REVIEW_SUBMITTED, + prNumber: 42, + agentInput: { + prNumber: 99, // stale value — should be overridden + repoFullName: 'acme/repo', + }, + }); + + expect(result.agentInput?.prNumber).toBe(42); + expect(result.prNumber).toBe(42); + }); + + it('builds structured skip results with the existing skip payload shape', () => { + expect(buildSkipResult('check-suite-success', 'not all checks complete')).toEqual({ + agentType: null, + agentInput: {}, + skipReason: { + handler: 'check-suite-success', + message: 'not all checks complete', + }, + }); + }); + + it('builds no-agent operation results', () => { + expect( + buildNoAgentResult({ lockKey: 'sentry:issue-1', coalesceKey: 'p1:sentry:issue-1' }), + ).toEqual({ + agentType: null, + agentInput: {}, + lockKey: 'sentry:issue-1', + coalesceKey: 'p1:sentry:issue-1', + }); + }); + + it('builds deferred re-check results with scheduler metadata', () => { + expect( + buildDeferredRecheckResult({ + delayMs: 45_000, + coalesceKey: 'p1:pr-conflict-recheck:42', + }), + ).toEqual({ + agentType: null, + agentInput: {}, + deferredRecheck: { + delayMs: 45_000, + coalesceKey: 'p1:pr-conflict-recheck:42', + }, + }); + }); +}); From 6d80fd0df11bfa22694e5a185d6e3c90165c216b Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Sat, 9 May 2026 09:18:59 +0200 Subject: [PATCH 5/5] fix(session-finish): scope GitHub client so PR-lookup fallback works (#1275) The cascade-tools session finish CLI extended raw oclif Command instead of CredentialScopedCommand, so the withGitHubToken AsyncLocalStorage scope was never established. When the primary PR sidecar was missing for any reason, validateFinish's secondary fallback findPRForCurrentBranch would call getClient() against an empty scope, throw "No GitHub client in scope", get silently swallowed by the catch in finish.ts, and the run would fail with "Cannot finish session without creating a PR" even when the PR existed on GitHub. Caught live on cascade run 06ec8a0a (PR #1274 was created but the run was marked failed). Switch Finish to extend CredentialScopedCommand so it inherits the same credential-scope wiring every other agent-facing CLI command uses. Add a regression test that exercises the real src/github/client.js module (only mocking the @octokit/rest constructor) so any future regression of the credential scope re-surfaces. The existing finish.test.ts gets the PM and GitHub credential scopes mocked as no-ops since the now-active credential chain would otherwise need the manifest registry bootstrapped. Co-authored-by: Claude Opus 4.7 (1M context) --- src/cli/session/finish.ts | 7 +- .../session/finish-credential-scope.test.ts | 126 ++++++++++++++++++ tests/unit/cli/session/finish.test.ts | 8 ++ 3 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 tests/unit/cli/session/finish-credential-scope.test.ts diff --git a/src/cli/session/finish.ts b/src/cli/session/finish.ts index ac90131a2..47de552ee 100644 --- a/src/cli/session/finish.ts +++ b/src/cli/session/finish.ts @@ -1,4 +1,4 @@ -import { Command, Flags } from '@oclif/core'; +import { Flags } from '@oclif/core'; import { readCompletionEvidence } from '../../backends/completion.js'; import { validateFinish, writePushedChangesSidecar } from '../../gadgets/session/core/finish.js'; import { finishDef } from '../../gadgets/session/definitions.js'; @@ -8,6 +8,7 @@ import { PUSHED_CHANGES_SIDECAR_ENV_VAR, REVIEW_SIDECAR_ENV_VAR, } from '../../gadgets/sessionState.js'; +import { CredentialScopedCommand } from '../base.js'; function readFinishHooksFromEnv(): SessionHooks { const raw = process.env.CASCADE_FINISH_HOOKS; @@ -20,7 +21,7 @@ function readFinishHooksFromEnv(): SessionHooks { } } -export default class Finish extends Command { +export default class Finish extends CredentialScopedCommand { static override description = finishDef.description; static override flags = { @@ -39,7 +40,7 @@ export default class Finish extends Command { }), }; - async run(): Promise { + async execute(): Promise { const { flags } = await this.parse(Finish); const hooks = readFinishHooksFromEnv(); const evidence = readCompletionEvidence({ diff --git a/tests/unit/cli/session/finish-credential-scope.test.ts b/tests/unit/cli/session/finish-credential-scope.test.ts new file mode 100644 index 000000000..c464ff230 --- /dev/null +++ b/tests/unit/cli/session/finish-credential-scope.test.ts @@ -0,0 +1,126 @@ +/** + * Regression net for cascade run `06ec8a0a` (Trello d1KVMufl, PR #1274): the agent + * created the PR but the run was marked failed because `cascade-tools session finish` + * extended raw oclif `Command` and skipped `CredentialScopedCommand.run()`'s + * `withGitHubToken` scope. The fallback `findPRForCurrentBranch()` then hit + * `getClient()` against empty AsyncLocalStorage, threw, got silently swallowed, + * and reported "Cannot finish session without creating a PR" despite the PR existing. + * + * Critical: do NOT mock `src/github/client.js` here — that would short-circuit the + * exact code path the bug lived in. Only mock at the `@octokit/rest` boundary. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const pullsList = vi.fn(); +const octokitFactory = vi.fn(() => ({ pulls: { list: pullsList } })); +vi.mock('@octokit/rest', () => ({ + Octokit: vi.fn().mockImplementation((...args: unknown[]) => octokitFactory(...args)), +})); + +const mockExecSync = vi.fn(); +const mockExecFileSync = vi.fn(); +vi.mock('node:child_process', () => ({ + execSync: (...args: unknown[]) => mockExecSync(...args), + execFileSync: (...args: unknown[]) => mockExecFileSync(...args), +})); + +// PM scope is irrelevant to this test — short-circuit it so we don't need to +// bootstrap the manifest registry. Other PM credential scopes are similarly +// unused: only GITHUB_TOKEN is set in env, so withTrello/Jira/Linear are no-ops. +vi.mock('../../../../src/pm/index.js', () => ({ + createPMProvider: vi.fn().mockReturnValue({}), + withPMProvider: vi.fn((_p: unknown, fn: () => Promise) => fn()), +})); + +import FinishCommand from '../../../../src/cli/session/finish.js'; + +describe('session finish CLI — GitHub credential scope', () => { + let originalEnv: NodeJS.ProcessEnv; + + beforeEach(() => { + originalEnv = { ...process.env }; + process.env.CASCADE_FINISH_HOOKS = JSON.stringify({ requiresPR: true }); + process.env.GITHUB_TOKEN = 'ghp_test_token'; + process.env.CASCADE_AGENT_TYPE = 'implementation'; + delete process.env.CASCADE_PR_SIDECAR_PATH; + delete process.env.CASCADE_REVIEW_SIDECAR_PATH; + delete process.env.CASCADE_PUSHED_CHANGES_SIDECAR_PATH; + delete process.env.CASCADE_PR_BRANCH; + delete process.env.CASCADE_INITIAL_HEAD_SHA; + + mockExecSync.mockImplementation((cmd: string) => { + if (cmd === 'git rev-parse --abbrev-ref HEAD') return 'feature/trigger-enable-gate-helper\n'; + if (cmd === 'git remote get-url origin') + return 'https://github.com/mongrel-intelligence/cascade.git\n'; + return ''; + }); + pullsList.mockReset(); + }); + + afterEach(() => { + process.env = originalEnv; + // Note: do NOT call vi.restoreAllMocks() — it would also restore the + // Octokit factory's mockImplementation set at module load, breaking the + // next test in this file. mockReset() above on the per-test spies (and + // resetting via beforeEach) is sufficient. + }); + + function buildCommand() { + const cmd = new FinishCommand([], {} as never); + cmd.log = vi.fn(); + cmd.exit = vi.fn((code?: number) => { + throw new Error(`exit:${code}`); + }) as never; + cmd.parse = vi.fn().mockResolvedValue({ + flags: { + comment: 'shipped', + 'pr-created': false, + 'review-submitted': false, + }, + args: {}, + argv: [], + raw: [], + metadata: {}, + nonExistentFlags: {}, + } as never); + return cmd; + } + + it('succeeds when sidecar is absent but findPRForCurrentBranch resolves a PR via the scoped GitHub client', async () => { + // Mocked Octokit returns one open PR — i.e. the agent did create the PR. + pullsList.mockResolvedValue({ + data: [{ html_url: 'https://github.com/mongrel-intelligence/cascade/pull/1274' }], + }); + + const cmd = buildCommand(); + await cmd.run(); + + expect(pullsList).toHaveBeenCalledWith({ + owner: 'mongrel-intelligence', + repo: 'cascade', + head: 'mongrel-intelligence:feature/trigger-enable-gate-helper', + state: 'open', + per_page: 1, + }); + expect(cmd.log).toHaveBeenCalledWith( + JSON.stringify({ success: true, data: 'Session ended: shipped' }), + ); + }); + + it('still fails (with the legitimate error) when sidecar is absent and the GitHub API returns no PR', async () => { + pullsList.mockResolvedValue({ data: [] }); + + const cmd = buildCommand(); + await expect(cmd.run()).rejects.toThrow('exit:1'); + + expect(pullsList).toHaveBeenCalled(); + expect(cmd.log).toHaveBeenCalledWith( + JSON.stringify({ + success: false, + error: + 'Cannot finish session without creating a PR. ' + + 'You must call CreatePR to submit your changes before calling Finish.', + }), + ); + }); +}); diff --git a/tests/unit/cli/session/finish.test.ts b/tests/unit/cli/session/finish.test.ts index d010038ed..b886de370 100644 --- a/tests/unit/cli/session/finish.test.ts +++ b/tests/unit/cli/session/finish.test.ts @@ -15,6 +15,14 @@ vi.mock('../../../../src/github/client.js', () => ({ githubClient: { getOpenPRByBranch: vi.fn(), }, + withGitHubToken: vi.fn((_token: string, fn: () => Promise) => fn()), +})); + +// PM and other-credential scopes are irrelevant to these tests (only pushed-changes +// validation is exercised). Short-circuit them so we don't need the manifest registry. +vi.mock('../../../../src/pm/index.js', () => ({ + createPMProvider: vi.fn().mockReturnValue({}), + withPMProvider: vi.fn((_p: unknown, fn: () => Promise) => fn()), })); import FinishCommand from '../../../../src/cli/session/finish.js';