diff --git a/.gitignore b/.gitignore index 07b0b866..a4bb4665 100644 --- a/.gitignore +++ b/.gitignore @@ -42,3 +42,6 @@ test-results/ # Claude Code — commit settings.json but exclude local settings (may contain secrets) .claude/settings.local.json +# Progress comment state file (ephemeral, written by ProgressMonitor) +.cascade-progress-comment-id + diff --git a/src/agents/base.ts b/src/agents/base.ts index e8047bb0..259cee41 100644 --- a/src/agents/base.ts +++ b/src/agents/base.ts @@ -30,10 +30,12 @@ import { type Todo, formatTodoList, initTodoSession, saveTodos } from '../gadget import { getPMProvider } from '../pm/index.js'; import type { AgentInput, AgentResult, CascadeConfig, ProjectConfig } from '../types/index.js'; import { logger } from '../utils/logging.js'; -import type { PromptContext } from './prompts/index.js'; +import { extractPRUrl } from '../utils/prUrl.js'; import { type BuilderType, createConfiguredBuilder } from './shared/builderFactory.js'; +import { getAgentCapabilities } from './shared/capabilities.js'; import { type FileLogger, executeAgentLifecycle } from './shared/lifecycle.js'; import { resolveModelConfig } from './shared/modelResolution.js'; +import { buildPromptContext } from './shared/promptContext.js'; import { setupRepository as setupRepo } from './shared/repository.js'; import { injectContextFiles, @@ -57,19 +59,6 @@ export interface AgentRunner { run: (ctx: AgentContext) => Promise; } -// ============================================================================ -// Repository Setup -// ============================================================================ - -async function setupRepository( - project: ProjectConfig, - log: AgentLogger, - agentType: string, - prBranch?: string, -): Promise { - return setupRepo({ project, log, agentType, prBranch, warmTsCache: true }); -} - // ============================================================================ // Agent Context Building // ============================================================================ @@ -106,51 +95,6 @@ async function loadDbPartials(orgId: string): Promise | unde } } -function buildPromptContext( - cardId: string | undefined, - project: ProjectConfig, - triggerType?: string, - prContext?: { prNumber: number; prBranch: string; repoFullName: string; headSha: string }, - debugContext?: { - logDir: string; - originalCardId: string; - originalCardName: string; - originalCardUrl: string; - detectedAgentType: string; - }, -): PromptContext { - const pmProvider = getPMProvider(); - const isJira = pmProvider.type === 'jira'; - return { - cardId, - cardUrl: cardId ? pmProvider.getWorkItemUrl(cardId) : undefined, - projectId: project.id, - storiesListId: project.trello?.lists?.stories, - processedLabelId: project.trello?.labels?.processed, - pmType: pmProvider.type, - workItemNoun: isJira ? 'issue' : 'card', - workItemNounPlural: isJira ? 'issues' : 'cards', - workItemNounCap: isJira ? 'Issue' : 'Card', - workItemNounPluralCap: isJira ? 'Issues' : 'Cards', - pmName: isJira ? 'JIRA' : 'Trello', - ...(prContext && { - prNumber: prContext.prNumber, - prBranch: prContext.prBranch, - repoFullName: prContext.repoFullName, - headSha: prContext.headSha, - triggerType, - }), - ...(debugContext && { - logDir: debugContext.logDir, - originalCardId: debugContext.originalCardId, - originalCardName: debugContext.originalCardName, - originalCardUrl: debugContext.originalCardUrl, - detectedAgentType: debugContext.detectedAgentType, - debugListId: project.trello?.lists?.debug, - }), - }; -} - function selectPrompt( cardId: string | undefined, commentContext?: { text: string; author: string }, @@ -319,18 +263,17 @@ Start by listing the contents of the log directory, then read and analyze the lo // ============================================================================ function getBaseAgentGadgets(agentType: string) { - // Planning agents are read-only - no file editing capabilities - const isReadOnlyAgent = agentType === 'planning' || agentType === 'respond-to-planning-comment'; + const caps = getAgentCapabilities(agentType); return [ - // Filesystem gadgets (read-only for planning) + // Filesystem gadgets (read-only when canEditFiles is false) new ListDirectory(), new ReadFile(), new RipGrep(), new AstGrep(), - ...(isReadOnlyAgent - ? [] - : [new FileSearchAndReplace(), new FileMultiEdit(), new WriteFile(), new VerifyChanges()]), + ...(caps.canEditFiles + ? [new FileSearchAndReplace(), new FileMultiEdit(), new WriteFile(), new VerifyChanges()] + : []), // Shell commands via tmux (no timeout issues) new Tmux(), new Sleep(), @@ -338,8 +281,8 @@ function getBaseAgentGadgets(agentType: string) { new TodoUpsert(), new TodoUpdateStatus(), new TodoDelete(), - // GitHub gadgets (no PR creation for planning) - ...(isReadOnlyAgent ? [] : [new CreatePR()]), + // GitHub gadgets (PR creation gated by capability) + ...(caps.canCreatePR ? [new CreatePR()] : []), // PM gadgets (work items, comments, checklists — PM-agnostic) new ReadWorkItem(), new PostComment(), @@ -347,9 +290,9 @@ function getBaseAgentGadgets(agentType: string) { new CreateWorkItem(), new ListWorkItems(), new AddChecklist(), - // UpdateChecklistItem not available for planning - prevents marking items complete prematurely - // But respond-to-planning-comment CAN update checklist items (user may ask to check/uncheck steps) - ...(agentType === 'planning' ? [] : [new PMUpdateChecklistItem()]), + // UpdateChecklistItem gated by capability — prevents planning from marking items complete + // prematurely, while respond-to-planning-comment CAN update them + ...(caps.canUpdateChecklists ? [new PMUpdateChecklistItem()] : []), // Session control new Finish(), ]; @@ -511,12 +454,7 @@ async function setupWorkingDirectory( return input.logDir; } - return setupRepository(project, log, agentType, prBranch); -} - -function extractPRUrl(output: string): string | undefined { - const match = output.match(/https:\/\/github\.com\/[^\s]+\/pull\/\d+/); - return match ? match[0] : undefined; + return setupRepo({ project, log, agentType, prBranch, warmTsCache: true }); } export async function executeAgent( @@ -613,7 +551,7 @@ export async function executeAgent( ctx.implementationSteps, ), - createProgressMonitor: (fileLogger) => + createProgressMonitor: (fileLogger, repoDir) => createProgressMonitor({ logWriter: fileLogger.write.bind(fileLogger), agentType, @@ -621,6 +559,7 @@ export async function executeAgent( progressModel: config.defaults.progressModel, intervalMinutes: config.defaults.progressIntervalMinutes, customModels: CUSTOM_MODELS as ModelSpec[], + repoDir, trello: cardId ? { cardId } : undefined, }), diff --git a/src/agents/review.ts b/src/agents/review.ts index 09ac5ddf..25eaba9b 100644 --- a/src/agents/review.ts +++ b/src/agents/review.ts @@ -1,7 +1,3 @@ -import { readFile } from 'node:fs/promises'; -import { join } from 'node:path'; - -import { REVIEW_FILE_CONTENT_TOKEN_LIMIT, estimateTokens } from '../config/reviewConfig.js'; import { Finish } from '../gadgets/Finish.js'; import { ListDirectory } from '../gadgets/ListDirectory.js'; import { ReadFile } from '../gadgets/ReadFile.js'; @@ -26,7 +22,12 @@ import { executeGitHubAgent, } from './shared/githubAgent.js'; import { resolveModelConfig } from './shared/modelResolution.js'; -import { type PRDiff, formatPRDetails, formatPRDiff } from './shared/prFormatting.js'; +import { + type PRFileContents, + formatPRDetails, + formatPRDiff, + readPRFileContents, +} from './shared/prFormatting.js'; import { injectContextFiles, injectSquintContext, @@ -41,43 +42,6 @@ interface ReviewAgentInput extends GitHubAgentInput { config: CascadeConfig; } -// ============================================================================ -// PR File Contents Reading -// ============================================================================ - -interface PRFileContents { - included: Array<{ path: string; content: string }>; - skipped: string[]; -} - -async function readPRFileContents(repoDir: string, prDiff: PRDiff): Promise { - const included: Array<{ path: string; content: string }> = []; - const skipped: string[] = []; - let totalTokens = 0; - - for (const file of prDiff) { - // Skip deleted/binary files - if (file.status === 'removed' || !file.patch) continue; - - const filePath = join(repoDir, file.filename); - try { - const content = await readFile(filePath, 'utf-8'); - const tokens = estimateTokens(content); - - if (totalTokens + tokens <= REVIEW_FILE_CONTENT_TOKEN_LIMIT) { - included.push({ path: file.filename, content }); - totalTokens += tokens; - } else { - skipped.push(file.filename); - } - } catch { - // File might not exist (renamed from), skip - } - } - - return { included, skipped }; -} - // ============================================================================ // Context Building // ============================================================================ diff --git a/src/agents/shared/capabilities.ts b/src/agents/shared/capabilities.ts new file mode 100644 index 00000000..336ecae7 --- /dev/null +++ b/src/agents/shared/capabilities.ts @@ -0,0 +1,105 @@ +// ============================================================================ +// AgentCapabilities +// ============================================================================ + +/** + * Describes what a particular agent type is allowed to do. + * + * Consumed by the llmist backend (agents/base.ts) to gate gadget inclusion + * and by the Claude Code backend (backends/agent-profiles.ts) for tool filtering. + * + * Keeping this in agents/shared/ avoids circular imports between agents/ and backends/. + */ +export interface AgentCapabilities { + /** Can the agent read and write files? (false = read-only) */ + canEditFiles: boolean; + /** Can the agent create GitHub pull requests? */ + canCreatePR: boolean; + /** Can the agent update PM checklist items? */ + canUpdateChecklists: boolean; + /** True for agents that only interact with the PM system (no repo changes) */ + isReadOnly: boolean; +} + +// ============================================================================ +// Capabilities Registry +// ============================================================================ + +/** + * Default capabilities for unknown agent types — full access. + */ +const DEFAULT_CAPABILITIES: AgentCapabilities = { + canEditFiles: true, + canCreatePR: true, + canUpdateChecklists: true, + isReadOnly: false, +}; + +/** + * Capabilities per agent type — single source of truth. + * AgentProfile in backends/agent-profiles.ts consumes these via getAgentCapabilities(). + */ +const CAPABILITIES_REGISTRY: Record = { + briefing: { + canEditFiles: true, + canCreatePR: false, + canUpdateChecklists: true, + isReadOnly: false, + }, + planning: { + canEditFiles: false, + canCreatePR: false, + canUpdateChecklists: false, + isReadOnly: true, + }, + implementation: { + canEditFiles: true, + canCreatePR: true, + canUpdateChecklists: true, + isReadOnly: false, + }, + review: { + canEditFiles: false, + canCreatePR: false, + canUpdateChecklists: false, + isReadOnly: true, + }, + 'respond-to-planning-comment': { + canEditFiles: false, + canCreatePR: false, + canUpdateChecklists: true, + isReadOnly: true, + }, + 'respond-to-review': { + canEditFiles: false, + canCreatePR: false, + canUpdateChecklists: false, + isReadOnly: true, + }, + 'respond-to-ci': { + canEditFiles: true, + canCreatePR: false, + canUpdateChecklists: true, + isReadOnly: false, + }, + 'respond-to-pr-comment': { + canEditFiles: true, + canCreatePR: false, + canUpdateChecklists: false, + isReadOnly: false, + }, + debug: { + canEditFiles: true, + canCreatePR: true, + canUpdateChecklists: true, + isReadOnly: false, + }, +}; + +/** + * Look up capabilities for a given agent type. + * Falls back to full-access defaults for unknown types. + */ +export function getAgentCapabilities(agentType: string): AgentCapabilities { + return CAPABILITIES_REGISTRY[agentType] ?? DEFAULT_CAPABILITIES; +} diff --git a/src/agents/shared/cleanup.ts b/src/agents/shared/cleanup.ts new file mode 100644 index 00000000..27a88154 --- /dev/null +++ b/src/agents/shared/cleanup.ts @@ -0,0 +1,38 @@ +import { cleanupLogDirectory, cleanupLogFile } from '../../utils/fileLogger.js'; +import { clearWatchdogCleanup } from '../../utils/lifecycle.js'; +import { logger } from '../../utils/logging.js'; +import { cleanupTempDir } from '../../utils/repo.js'; +import type { FileLogger } from './lifecycle.js'; + +/** + * Clean up temporary resources after agent execution. + * + * Shared by both the llmist lifecycle (agents/shared/lifecycle.ts) and the + * adapter-based backends (backends/adapter.ts). + * + * @param repoDir - The temp repo directory to remove (null if not set up) + * @param fileLogger - The file logger whose log files should be cleaned up + * @param skipRepoDeletion - When true, skip removing the repoDir (e.g., for debug agents using a pre-existing logDir) + */ +export function cleanupAgentResources( + repoDir: string | null, + fileLogger: FileLogger, + skipRepoDeletion = false, +): void { + clearWatchdogCleanup(); + + const isLocalMode = process.env.CASCADE_LOCAL_MODE === 'true'; + + if (repoDir && !isLocalMode && !skipRepoDeletion) { + try { + cleanupTempDir(repoDir); + } catch (err) { + logger.warn('Failed to cleanup temp directory', { repoDir, error: String(err) }); + } + } + if (!isLocalMode) { + cleanupLogFile(fileLogger.logPath); + cleanupLogFile(fileLogger.llmistLogPath); + cleanupLogDirectory(fileLogger.llmCallLogger.logDir); + } +} diff --git a/src/agents/shared/githubAgent.ts b/src/agents/shared/githubAgent.ts index a1101caa..d6b144ea 100644 --- a/src/agents/shared/githubAgent.ts +++ b/src/agents/shared/githubAgent.ts @@ -197,7 +197,7 @@ export async function executeGitHubAgent< }); }, - createProgressMonitor: (fileLogger) => + createProgressMonitor: (fileLogger, _repoDir) => createProgressMonitor({ logWriter: fileLogger.write.bind(fileLogger), agentType: definition.agentType, diff --git a/src/agents/shared/lifecycle.ts b/src/agents/shared/lifecycle.ts index 19c2cdc5..447ca582 100644 --- a/src/agents/shared/lifecycle.ts +++ b/src/agents/shared/lifecycle.ts @@ -5,17 +5,14 @@ import type { ProgressMonitor } from '../../backends/progressMonitor.js'; import { type CompleteRunInput, type LlmCallRecord, - completeRun, - createRun, storeLlmCallsBulk, storeRunLogs, } from '../../db/repositories/runsRepository.js'; import type { AgentResult } from '../../types/index.js'; import { loadCascadeEnv, unloadCascadeEnv } from '../../utils/cascadeEnv.js'; -import { cleanupLogDirectory, cleanupLogFile, createFileLogger } from '../../utils/fileLogger.js'; -import { clearWatchdogCleanup, setWatchdogCleanup } from '../../utils/lifecycle.js'; +import { createFileLogger } from '../../utils/fileLogger.js'; +import { setWatchdogCleanup } from '../../utils/lifecycle.js'; import { logger } from '../../utils/logging.js'; -import { cleanupTempDir } from '../../utils/repo.js'; import { setupRemoteSquintDb } from '../../utils/squintDb.js'; import { runAgentLoop } from '../utils/agentLoop.js'; import type { AccumulatedLlmCall } from '../utils/hooks.js'; @@ -23,6 +20,8 @@ import { getLogLevel } from '../utils/index.js'; import { createAgentLogger } from '../utils/logging.js'; import { type TrackingContext, createTrackingContext } from '../utils/tracking.js'; import type { BuilderType } from './builderFactory.js'; +import { cleanupAgentResources } from './cleanup.js'; +import { type RunTrackingInput, tryCompleteRun, tryCreateRun } from './runTracking.js'; type FileLogger = ReturnType; type AgentLogger = ReturnType; @@ -35,14 +34,8 @@ export interface BaseAgentContext { prompt: string; } -export interface RunTrackingConfig { - projectId: string; - cardId?: string; - prNumber?: number; - agentType: string; - backendName: string; - triggerType?: string; -} +/** @deprecated Use RunTrackingInput from runTracking.ts */ +export type RunTrackingConfig = RunTrackingInput; export interface ExecuteAgentOptions { /** Identifier for log file naming (e.g., "review-42", "ci-42") */ @@ -80,7 +73,7 @@ export interface ExecuteAgentOptions { }) => Promise; /** Create a ProgressMonitor for time-based progress reporting */ - createProgressMonitor?: (fileLogger: FileLogger) => ProgressMonitor | null; + createProgressMonitor?: (fileLogger: FileLogger, repoDir: string) => ProgressMonitor | null; /** Whether to run in interactive mode */ interactive?: boolean; @@ -105,28 +98,6 @@ export interface ExecuteAgentOptions { // Run Tracking Helpers // ============================================================================ -async function tryCreateRun( - config: RunTrackingConfig, - model?: string, - maxIterations?: number, -): Promise { - try { - return await createRun({ - projectId: config.projectId, - cardId: config.cardId, - prNumber: config.prNumber, - agentType: config.agentType, - backend: config.backendName, - triggerType: config.triggerType, - model, - maxIterations, - }); - } catch (err) { - logger.warn('Failed to create run record', { error: String(err) }); - return undefined; - } -} - async function tryStoreLogsAndCalls( runId: string, fileLogger: FileLogger, @@ -200,14 +171,6 @@ async function tryStoreLogsAndCalls( } } -async function tryCompleteRun(runId: string, input: CompleteRunInput): Promise { - try { - await completeRun(runId, input); - } catch (err) { - logger.warn('Failed to complete run record', { runId, error: String(err) }); - } -} - // ============================================================================ // Run Finalization Helper // ============================================================================ @@ -224,25 +187,6 @@ async function finalizeRun( await tryCompleteRun(runId, input); } -function cleanupLifecycleResources(repoDir: string | null, fileLogger: FileLogger): void { - clearWatchdogCleanup(); - - const isLocalMode = process.env.CASCADE_LOCAL_MODE === 'true'; - - if (repoDir && !isLocalMode) { - try { - cleanupTempDir(repoDir); - } catch (err) { - logger.warn('Failed to cleanup temp directory', { repoDir, error: String(err) }); - } - } - if (!isLocalMode) { - cleanupLogFile(fileLogger.logPath); - cleanupLogFile(fileLogger.llmistLogPath); - cleanupLogDirectory(fileLogger.llmCallLogger.logDir); - } -} - function buildAgentResult( result: Awaited>, logBuffer: Buffer, @@ -344,7 +288,7 @@ export async function executeAgentLifecycle( : new LLMist(); const llmistLogger = createLogger({ minLevel: getLogLevel() }); const trackingContext = createTrackingContext(); - const progressMonitor = options.createProgressMonitor?.(fileLogger) ?? null; + const progressMonitor = options.createProgressMonitor?.(fileLogger, repoDir) ?? null; let builder = options.createBuilder({ client, @@ -451,6 +395,6 @@ export async function executeAgentLifecycle( return { success: false, output: '', error: String(err), logBuffer, runId, durationMs }; } finally { - cleanupLifecycleResources(repoDir, fileLogger); + cleanupAgentResources(repoDir, fileLogger); } } diff --git a/src/agents/shared/prFormatting.ts b/src/agents/shared/prFormatting.ts index 85af2346..e0b1053b 100644 --- a/src/agents/shared/prFormatting.ts +++ b/src/agents/shared/prFormatting.ts @@ -1,3 +1,7 @@ +import { readFile } from 'node:fs/promises'; +import { join } from 'node:path'; + +import { REVIEW_FILE_CONTENT_TOKEN_LIMIT, estimateTokens } from '../../config/reviewConfig.js'; import type { githubClient } from '../../github/client.js'; type PRDetails = Awaited>; @@ -99,3 +103,50 @@ export function formatPRIssueComments(prIssueComments: PRIssueComments): string ) .join('\n\n'); } + +// ============================================================================ +// PR File Contents Reading +// ============================================================================ + +export interface PRFileContents { + included: Array<{ path: string; content: string }>; + skipped: string[]; +} + +/** + * Read the full contents of changed PR files up to a token limit. + * + * Shared between the llmist review agent (agents/review.ts) and the + * Claude Code backend (backends/agent-profiles.ts). + * + * @param repoDir - The local repository directory + * @param prDiff - The PR diff file list from GitHub + * @returns Files that fit within the token limit and those that were skipped + */ +export async function readPRFileContents(repoDir: string, prDiff: PRDiff): Promise { + const included: Array<{ path: string; content: string }> = []; + const skipped: string[] = []; + let totalTokens = 0; + + for (const file of prDiff) { + // Skip deleted/binary files + if (file.status === 'removed' || !file.patch) continue; + + const filePath = join(repoDir, file.filename); + try { + const content = await readFile(filePath, 'utf-8'); + const tokens = estimateTokens(content); + + if (totalTokens + tokens <= REVIEW_FILE_CONTENT_TOKEN_LIMIT) { + included.push({ path: file.filename, content }); + totalTokens += tokens; + } else { + skipped.push(file.filename); + } + } catch { + // File might not exist (renamed from), skip + } + } + + return { included, skipped }; +} diff --git a/src/agents/shared/promptContext.ts b/src/agents/shared/promptContext.ts new file mode 100644 index 00000000..2d6beabc --- /dev/null +++ b/src/agents/shared/promptContext.ts @@ -0,0 +1,56 @@ +import { getPMProvider } from '../../pm/index.js'; +import type { ProjectConfig } from '../../types/index.js'; +import type { PromptContext } from '../prompts/index.js'; + +/** + * Build a PromptContext from project config and optional trigger data. + * + * Shared by the llmist agent lifecycle (agents/base.ts) and the adapter + * (backends/adapter.ts) so both backends use consistent prompt context + * building logic including PM-type normalization and work item noun i18n. + */ +export function buildPromptContext( + cardId: string | undefined, + project: ProjectConfig, + triggerType?: string, + prContext?: { prNumber: number; prBranch: string; repoFullName: string; headSha: string }, + debugContext?: { + logDir: string; + originalCardId: string; + originalCardName: string; + originalCardUrl: string; + detectedAgentType: string; + }, +): PromptContext { + const pmProvider = getPMProvider(); + const isJira = pmProvider.type === 'jira'; + return { + cardId, + cardUrl: cardId ? pmProvider.getWorkItemUrl(cardId) : undefined, + projectId: project.id, + baseBranch: project.baseBranch, + storiesListId: project.trello?.lists?.stories, + processedLabelId: project.trello?.labels?.processed, + pmType: pmProvider.type, + workItemNoun: isJira ? 'issue' : 'card', + workItemNounPlural: isJira ? 'issues' : 'cards', + workItemNounCap: isJira ? 'Issue' : 'Card', + workItemNounPluralCap: isJira ? 'Issues' : 'Cards', + pmName: isJira ? 'JIRA' : 'Trello', + ...(prContext && { + prNumber: prContext.prNumber, + prBranch: prContext.prBranch, + repoFullName: prContext.repoFullName, + headSha: prContext.headSha, + triggerType, + }), + ...(debugContext && { + logDir: debugContext.logDir, + originalCardId: debugContext.originalCardId, + originalCardName: debugContext.originalCardName, + originalCardUrl: debugContext.originalCardUrl, + detectedAgentType: debugContext.detectedAgentType, + debugListId: project.trello?.lists?.debug, + }), + }; +} diff --git a/src/agents/shared/runTracking.ts b/src/agents/shared/runTracking.ts new file mode 100644 index 00000000..24c01b60 --- /dev/null +++ b/src/agents/shared/runTracking.ts @@ -0,0 +1,94 @@ +import fs from 'node:fs'; + +import { + type CompleteRunInput, + completeRun, + createRun, + storeRunLogs, +} from '../../db/repositories/runsRepository.js'; +import { logger } from '../../utils/logging.js'; +import type { FileLogger } from './lifecycle.js'; + +// ============================================================================ +// Run Tracking Configuration +// ============================================================================ + +export interface RunTrackingInput { + projectId: string; + cardId?: string; + prNumber?: number; + agentType: string; + backendName: string; + triggerType?: string; +} + +// ============================================================================ +// Shared Run Tracking Helpers +// ============================================================================ + +/** + * Create a DB run record, suppressing errors so agent execution is unaffected. + */ +export async function tryCreateRun( + input: RunTrackingInput, + model?: string, + maxIterations?: number, +): Promise { + try { + return await createRun({ + projectId: input.projectId, + cardId: input.cardId, + prNumber: input.prNumber, + agentType: input.agentType, + backend: input.backendName, + triggerType: input.triggerType, + model, + maxIterations, + }); + } catch (err) { + logger.warn('Failed to create run record', { error: String(err) }); + return undefined; + } +} + +/** + * Store cascade and llmist log files for a run, suppressing errors. + */ +export async function tryStoreRunLogs(runId: string, fileLogger: FileLogger): Promise { + try { + const cascadeLog = fs.existsSync(fileLogger.logPath) + ? fs.readFileSync(fileLogger.logPath, 'utf-8') + : undefined; + const llmistLog = fs.existsSync(fileLogger.llmistLogPath) + ? fs.readFileSync(fileLogger.llmistLogPath, 'utf-8') + : undefined; + await storeRunLogs(runId, cascadeLog, llmistLog); + } catch (err) { + logger.warn('Failed to store run logs', { runId, error: String(err) }); + } +} + +/** + * Mark a run as complete in the DB, suppressing errors. + */ +export async function tryCompleteRun(runId: string, input: CompleteRunInput): Promise { + try { + await completeRun(runId, input); + } catch (err) { + logger.warn('Failed to complete run record', { runId, error: String(err) }); + } +} + +/** + * Finalize a backend run: store logs and mark complete. + * Used by non-llmist backends (claude-code adapter) that don't accumulate LLM calls. + */ +export async function finalizeBackendRun( + runId: string | undefined, + fileLogger: FileLogger, + input: CompleteRunInput, +): Promise { + if (!runId) return; + await tryStoreRunLogs(runId, fileLogger); + await tryCompleteRun(runId, input); +} diff --git a/src/backends/adapter.ts b/src/backends/adapter.ts index 2dc8c3ab..0fce8f26 100644 --- a/src/backends/adapter.ts +++ b/src/backends/adapter.ts @@ -1,27 +1,26 @@ -import fs from 'node:fs'; import type { ModelSpec } from 'llmist'; import type { PromptContext } from '../agents/prompts/index.js'; +import { cleanupAgentResources } from '../agents/shared/cleanup.js'; import { resolveModelConfig } from '../agents/shared/modelResolution.js'; +import { buildPromptContext } from '../agents/shared/promptContext.js'; import { setupRepository } from '../agents/shared/repository.js'; +import { + type RunTrackingInput, + finalizeBackendRun, + tryCreateRun, +} from '../agents/shared/runTracking.js'; import { createAgentLogger } from '../agents/utils/logging.js'; import { CUSTOM_MODELS } from '../config/customModels.js'; import { getProjectSecrets } from '../config/provider.js'; import { loadPartials } from '../db/repositories/partialsRepository.js'; -import { - type CompleteRunInput, - completeRun, - createRun, - storeRunLogs, -} from '../db/repositories/runsRepository.js'; import { withGitHubToken } from '../github/client.js'; import { getPersonaToken } from '../github/personas.js'; import type { AgentInput, AgentResult, CascadeConfig, ProjectConfig } from '../types/index.js'; import { loadCascadeEnv, unloadCascadeEnv } from '../utils/cascadeEnv.js'; -import { cleanupLogDirectory, cleanupLogFile, createFileLogger } from '../utils/fileLogger.js'; -import { clearWatchdogCleanup, setWatchdogCleanup } from '../utils/lifecycle.js'; +import { createFileLogger } from '../utils/fileLogger.js'; +import { setWatchdogCleanup } from '../utils/lifecycle.js'; import { logger } from '../utils/logging.js'; -import { cleanupTempDir } from '../utils/repo.js'; import { setupRemoteSquintDb } from '../utils/squintDb.js'; import { getAgentProfile } from './agent-profiles.js'; import { createProgressMonitor } from './progress.js'; @@ -264,26 +263,23 @@ async function buildBackendInput( ): Promise> { const { project, config, cardId } = input; - const pmType = project.pm?.type ?? 'trello'; - const promptContext: PromptContext = { + // PR context from check-failure trigger + const prContext = + input.prNumber !== undefined + ? { + prNumber: input.prNumber as number, + prBranch: input.prBranch as string, + repoFullName: input.repoFullName as string, + headSha: input.headSha as string, + } + : undefined; + + const promptContext: PromptContext = buildPromptContext( cardId, - cardUrl: cardId - ? pmType === 'jira' && project.jira - ? `${project.jira.baseUrl}/browse/${cardId}` - : `https://trello.com/c/${cardId}` - : undefined, - projectId: project.id, - baseBranch: project.baseBranch, - storiesListId: project.trello?.lists?.stories, - processedLabelId: project.trello?.labels?.processed, - pmType, - // PR context fields (from check-failure trigger) - prNumber: input.prNumber, - prBranch: input.prBranch, - repoFullName: input.repoFullName, - headSha: input.headSha, - triggerType: input.triggerType, - }; + project, + input.triggerType, + prContext, + ); // Load DB partials for template include resolution let dbPartials: Map | undefined; @@ -366,32 +362,6 @@ async function buildBackendInput( }; } -/** - * Clean up temporary resources after execution. - */ -function cleanupResources( - repoDir: string | null, - fileLogger: ReturnType, - hasLogDir: boolean, -): void { - clearWatchdogCleanup(); - - const isLocalMode = process.env.CASCADE_LOCAL_MODE === 'true'; - - if (repoDir && !isLocalMode && !hasLogDir) { - try { - cleanupTempDir(repoDir); - } catch (err) { - logger.warn('Failed to cleanup temp directory', { repoDir, error: String(err) }); - } - } - if (!isLocalMode) { - cleanupLogFile(fileLogger.logPath); - cleanupLogFile(fileLogger.llmistLogPath); - cleanupLogDirectory(fileLogger.llmCallLogger.logDir); - } -} - /** * Post-process a backend result: validate PR creation for implementation agents * and zero out cost for subscription-backed Claude Code sessions. @@ -428,61 +398,6 @@ function postProcessResult( } } -/** - * Execute an agent using the given backend with full lifecycle management. - * This handles repository setup, context fetching, logging, and cleanup. - * - * Used by non-llmist backends. The llmist backend wraps existing code directly. - */ -async function tryCreateBackendRun( - agentType: string, - backendName: string, - input: AgentInput & { project: ProjectConfig }, - model?: string, - maxIterations?: number, -): Promise { - try { - return await createRun({ - projectId: input.project.id, - cardId: input.cardId, - prNumber: input.prNumber, - agentType, - backend: backendName, - triggerType: input.triggerType, - model, - maxIterations, - }); - } catch (err) { - logger.warn('Failed to create run record', { error: String(err) }); - return undefined; - } -} - -async function tryStoreBackendLogs( - runId: string, - fileLogger: ReturnType, -): Promise { - try { - const cascadeLog = fs.existsSync(fileLogger.logPath) - ? fs.readFileSync(fileLogger.logPath, 'utf-8') - : undefined; - const llmistLog = fs.existsSync(fileLogger.llmistLogPath) - ? fs.readFileSync(fileLogger.llmistLogPath, 'utf-8') - : undefined; - await storeRunLogs(runId, cascadeLog, llmistLog); - } catch (err) { - logger.warn('Failed to store backend run logs', { runId, error: String(err) }); - } -} - -async function tryCompleteBackendRun(runId: string, input: CompleteRunInput): Promise { - try { - await completeRun(runId, input); - } catch (err) { - logger.warn('Failed to complete backend run record', { runId, error: String(err) }); - } -} - function warnIfSubscriptionCostMismatch(_backend: AgentBackend, _project: ProjectConfig): void { // No-op: ANTHROPIC_API_KEY is no longer used. Claude Code uses OAuth only. } @@ -507,16 +422,6 @@ async function resolveGitHubToken( } } -async function finalizeBackendRun( - runId: string | undefined, - fileLogger: ReturnType, - input: CompleteRunInput, -): Promise { - if (!runId) return; - await tryStoreBackendLogs(runId, fileLogger); - await tryCompleteBackendRun(runId, input); -} - export async function executeWithBackend( backend: AgentBackend, agentType: string, @@ -582,13 +487,15 @@ export async function executeWithBackend( ? await withGitHubToken(gitHubToken, buildAndPrepare) : await buildAndPrepare(); - runId = await tryCreateBackendRun( + const runTrackingInput: RunTrackingInput = { + projectId: input.project.id, + cardId: input.cardId, + prNumber: input.prNumber, agentType, - backend.name, - input, - partialInput.model, - partialInput.maxIterations, - ); + backendName: backend.name, + triggerType: input.triggerType, + }; + runId = await tryCreateRun(runTrackingInput, partialInput.model, partialInput.maxIterations); const monitor = createProgressMonitor({ logWriter, @@ -597,6 +504,7 @@ export async function executeWithBackend( progressModel: input.config.defaults.progressModel, intervalMinutes: input.config.defaults.progressIntervalMinutes, customModels: CUSTOM_MODELS as ModelSpec[], + repoDir: repoDir ?? undefined, trello: cardId ? { cardId } : undefined, }); @@ -674,6 +582,6 @@ export async function executeWithBackend( return { success: false, output: '', error: String(err), logBuffer, runId, durationMs }; } finally { - cleanupResources(repoDir, fileLogger, Boolean(input.logDir)); + cleanupAgentResources(repoDir, fileLogger, Boolean(input.logDir)); } } diff --git a/src/backends/agent-profiles.ts b/src/backends/agent-profiles.ts index 9a85597e..6262fb78 100644 --- a/src/backends/agent-profiles.ts +++ b/src/backends/agent-profiles.ts @@ -1,20 +1,20 @@ import { execFileSync } from 'node:child_process'; -import { readFile } from 'node:fs/promises'; -import { join } from 'node:path'; +import { type AgentCapabilities, getAgentCapabilities } from '../agents/shared/capabilities.js'; +export type { AgentCapabilities } from '../agents/shared/capabilities.js'; import { formatPRComments, formatPRDetails, formatPRDiff, formatPRIssueComments, formatPRReviews, + readPRFileContents, } from '../agents/shared/prFormatting.js'; import type { ContextFile } from '../agents/utils/setup.js'; -import { REVIEW_FILE_CONTENT_TOKEN_LIMIT, estimateTokens } from '../config/reviewConfig.js'; import { ListDirectory } from '../gadgets/ListDirectory.js'; import { formatCheckStatus } from '../gadgets/github/core/getPRChecks.js'; import { readWorkItem } from '../gadgets/pm/core/readWorkItem.js'; -import { type PRDiffFile, githubClient } from '../github/client.js'; +import { githubClient } from '../github/client.js'; import type { AgentInput } from '../types/index.js'; import { resolveSquintDbPath } from '../utils/squintDb.js'; import type { ContextInjection, LogWriter, ToolManifest } from './types.js'; @@ -95,6 +95,8 @@ export interface AgentProfile { buildTaskPrompt(input: AgentInput): string; /** Optional pre-execute hook (e.g., post initial PR comment) */ preExecute?(params: PreExecuteParams): Promise; + /** Capability summary — used by llmist backend to select gadgets */ + capabilities: AgentCapabilities; } // ============================================================================ @@ -176,37 +178,6 @@ async function fetchWorkItemInjection(cardId: string): Promise; skipped: string[] }> { - const included: Array<{ path: string; content: string }> = []; - const skipped: string[] = []; - let totalTokens = 0; - - for (const file of prDiff) { - if (file.status === 'removed' || !file.patch) continue; - - const filePath = join(repoDir, file.filename); - try { - const content = await readFile(filePath, 'utf-8'); - const tokens = estimateTokens(content); - - if (totalTokens + tokens <= REVIEW_FILE_CONTENT_TOKEN_LIMIT) { - included.push({ path: file.filename, content }); - totalTokens += tokens; - } else { - skipped.push(file.filename); - } - } catch { - // File might not exist (renamed from), skip - } - } - - return { included, skipped }; -} - /** Fetch PR context injections (ported from review.ts:93-144) */ async function fetchPRContextInjections( owner: string, @@ -491,6 +462,7 @@ const briefingProfile: AgentProfile = { needsGitHubToken: false, fetchContext: fetchWorkItemContext, buildTaskPrompt: buildWorkItemTaskPrompt, + capabilities: getAgentCapabilities('briefing'), }; const planningProfile: AgentProfile = { @@ -500,6 +472,7 @@ const planningProfile: AgentProfile = { needsGitHubToken: false, fetchContext: fetchWorkItemContext, buildTaskPrompt: buildWorkItemTaskPrompt, + capabilities: getAgentCapabilities('planning'), }; const reviewProfile: AgentProfile = { @@ -509,6 +482,7 @@ const reviewProfile: AgentProfile = { needsGitHubToken: true, fetchContext: fetchReviewContext, buildTaskPrompt: buildReviewTaskPrompt, + capabilities: getAgentCapabilities('review'), async preExecute({ input, logWriter }: PreExecuteParams): Promise { const repoFullName = input.repoFullName as string; @@ -528,6 +502,7 @@ const respondToPlanningCommentProfile: AgentProfile = { needsGitHubToken: false, fetchContext: fetchWorkItemContext, buildTaskPrompt: buildCommentResponseTaskPrompt, + capabilities: getAgentCapabilities('respond-to-planning-comment'), }; const respondToCIProfile: AgentProfile = { @@ -544,6 +519,7 @@ const respondToCIProfile: AgentProfile = { blockGitPush: false, fetchContext: fetchCIContext, buildTaskPrompt: buildCITaskPrompt, + capabilities: getAgentCapabilities('respond-to-ci'), async preExecute({ input, logWriter }: PreExecuteParams): Promise { const repoFullName = input.repoFullName as string; @@ -568,6 +544,7 @@ const respondToPRCommentProfile: AgentProfile = { blockGitPush: false, fetchContext: fetchPRCommentResponseContext, buildTaskPrompt: buildPRCommentResponseTaskPrompt, + capabilities: getAgentCapabilities('respond-to-pr-comment'), }; const defaultProfile: AgentProfile = { @@ -577,11 +554,13 @@ const defaultProfile: AgentProfile = { needsGitHubToken: false, fetchContext: fetchWorkItemContext, buildTaskPrompt: buildWorkItemTaskPrompt, + capabilities: getAgentCapabilities('debug'), }; const implementationProfile: AgentProfile = { ...defaultProfile, needsGitHubToken: true, + capabilities: getAgentCapabilities('implementation'), }; // ============================================================================ diff --git a/src/backends/claude-code/index.ts b/src/backends/claude-code/index.ts index b410b8b3..8216a10e 100644 --- a/src/backends/claude-code/index.ts +++ b/src/backends/claude-code/index.ts @@ -10,6 +10,7 @@ import type { } from '@anthropic-ai/claude-agent-sdk'; import { storeLlmCall } from '../../db/repositories/runsRepository.js'; import { logger } from '../../utils/logging.js'; +import { extractPRUrl } from '../../utils/prUrl.js'; import type { AgentBackend, AgentBackendInput, @@ -170,14 +171,6 @@ export function buildEnv(projectSecrets?: Record): { return { env }; } -/** - * Extract a GitHub PR URL from text. - */ -function extractPRUrl(text: string): string | undefined { - const match = text.match(/https:\/\/github\.com\/[^\s"')\]]+\/pull\/\d+/); - return match ? match[0] : undefined; -} - /** * Extract a GitHub PR URL from assistant messages (tool results containing create-pr output). */ diff --git a/src/backends/progress.ts b/src/backends/progress.ts index af0cb70a..5f59548f 100644 --- a/src/backends/progress.ts +++ b/src/backends/progress.ts @@ -11,6 +11,7 @@ export interface ProgressMonitorOptions { progressModel: string; intervalMinutes: number; customModels: ModelSpec[]; + repoDir?: string; trello?: { cardId: string }; github?: { owner: string; repo: string; headerMessage: string }; } @@ -34,6 +35,7 @@ export function createProgressMonitor(options: ProgressMonitorOptions): Progress progressModel: options.progressModel, customModels: options.customModels, logWriter: options.logWriter, + repoDir: options.repoDir, trello: options.trello, github: options.github, }; diff --git a/src/backends/progressMonitor.ts b/src/backends/progressMonitor.ts index 1ae4c983..9aef81d9 100644 --- a/src/backends/progressMonitor.ts +++ b/src/backends/progressMonitor.ts @@ -19,6 +19,7 @@ import { loadTodos } from '../gadgets/todo/storage.js'; import { githubClient } from '../github/client.js'; import { getPMProviderOrNull } from '../pm/index.js'; import { type ProgressContext, callProgressModel } from './progressModel.js'; +import { clearProgressCommentId, writeProgressCommentId } from './progressState.js'; import type { LogWriter, ProgressReporter } from './types.js'; export interface ProgressMonitorConfig { @@ -28,6 +29,7 @@ export interface ProgressMonitorConfig { progressModel: string; customModels: ModelSpec[]; logWriter: LogWriter; + repoDir?: string; trello?: { cardId: string }; github?: { owner: string; repo: string; headerMessage: string }; } @@ -135,6 +137,13 @@ export class ProgressMonitor implements ProgressReporter { clearInterval(this.timer); this.timer = null; } + // Clean up state file on stop (best-effort — stop() is called from finally + // blocks, so an rmSync failure must not mask the actual agent result) + try { + clearProgressCommentId(this.config.repoDir); + } catch { + // State file cleanup is best-effort + } } // ── Internal ── @@ -155,6 +164,15 @@ export class ProgressMonitor implements ProgressReporter { cardId: this.config.trello.cardId, commentId: this.progressCommentId, }); + + // Write state file so PostComment gadget can update this comment + if (this.config.repoDir && this.progressCommentId) { + writeProgressCommentId( + this.config.repoDir, + this.config.trello.cardId, + this.progressCommentId, + ); + } } private async tick(): Promise { @@ -217,47 +235,58 @@ export class ProgressMonitor implements ProgressReporter { } } + private maybeWriteStateFile(cardId: string, commentId: string | null): void { + if (this.config.repoDir && commentId) { + writeProgressCommentId(this.config.repoDir, cardId, commentId); + } + } + + private async postProgressToPM(summary: string, cardId: string): Promise { + const provider = getPMProviderOrNull(); + if (!provider) return; + + if (this.progressCommentId) { + // Subsequent ticks: update the existing comment. + // On success, the state file written by postInitialComment() remains + // valid (same comment ID), so no need to rewrite it here. + try { + await provider.updateComment(cardId, this.progressCommentId, summary); + this.config.logWriter('INFO', 'Updated progress comment on work item', { + cardId, + commentId: this.progressCommentId, + }); + } catch (updateErr) { + // Comment may have been deleted — fall back to creating a new one + this.config.logWriter('WARN', 'Failed to update progress comment, creating new one', { + error: String(updateErr), + }); + this.progressCommentId = await provider.addComment(cardId, summary); + this.config.logWriter('INFO', 'Posted new progress comment to work item', { + cardId, + commentId: this.progressCommentId, + }); + // Update state file with new comment ID + this.maybeWriteStateFile(cardId, this.progressCommentId); + } + } else { + // First tick: create the comment and store its ID. + // This branch is reached when postInitialComment() failed (transient API error) + // and the first tick creates the comment instead. + this.progressCommentId = await provider.addComment(cardId, summary); + this.config.logWriter('INFO', 'Posted progress update to work item', { + cardId, + commentId: this.progressCommentId, + }); + // Write state file so PostComment gadget can find this comment + this.maybeWriteStateFile(cardId, this.progressCommentId); + } + } + private async postProgress(summary: string): Promise { // Post to PM provider (Trello/JIRA) — create once, update in place if (this.config.trello) { try { - const provider = getPMProviderOrNull(); - if (provider) { - if (this.progressCommentId) { - // Subsequent ticks: update the existing comment - try { - await provider.updateComment( - this.config.trello.cardId, - this.progressCommentId, - summary, - ); - this.config.logWriter('INFO', 'Updated progress comment on work item', { - cardId: this.config.trello.cardId, - commentId: this.progressCommentId, - }); - } catch (updateErr) { - // Comment may have been deleted — fall back to creating a new one - this.config.logWriter('WARN', 'Failed to update progress comment, creating new one', { - error: String(updateErr), - }); - this.progressCommentId = await provider.addComment( - this.config.trello.cardId, - summary, - ); - this.config.logWriter('INFO', 'Posted new progress comment to work item', { - cardId: this.config.trello.cardId, - commentId: this.progressCommentId, - }); - } - } else { - // First tick: create the comment and store its ID - this.progressCommentId = await provider.addComment(this.config.trello.cardId, summary); - this.config.logWriter('INFO', 'Posted progress update to work item', { - cardId: this.config.trello.cardId, - commentId: this.progressCommentId, - }); - } - } + await this.postProgressToPM(summary, this.config.trello.cardId); } catch (err) { this.config.logWriter('WARN', 'Failed to post progress to work item', { error: String(err), diff --git a/src/backends/progressState.ts b/src/backends/progressState.ts new file mode 100644 index 00000000..41215d1f --- /dev/null +++ b/src/backends/progressState.ts @@ -0,0 +1,83 @@ +/** + * File-based state bridge for sharing the progress comment ID between + * the ProgressMonitor (which creates the initial comment) and the + * PostComment gadget (which posts the final summary). + * + * Uses a state file `.cascade-progress-comment-id` written to the repo + * working directory. This approach works for both the llmist backend + * (same process) and the Claude Code backend (subprocess), since both + * share the same filesystem. + * + * File format: `:` + */ + +import { existsSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; + +const STATE_FILE_NAME = '.cascade-progress-comment-id'; + +/** + * Writes the progress comment ID to the state file in the given repo directory. + * + * @param repoDir - The working directory where the state file will be written. + * @param workItemId - The work item ID (Trello card ID or JIRA issue key). + * @param commentId - The comment ID returned by addComment(). + */ +export function writeProgressCommentId( + repoDir: string, + workItemId: string, + commentId: string, +): void { + const filePath = join(repoDir, STATE_FILE_NAME); + writeFileSync(filePath, `${workItemId}:${commentId}`, 'utf-8'); +} + +/** + * Reads the progress comment state from the state file. + * + * @param repoDir - Optional directory containing the state file. Defaults to + * `process.cwd()` if not provided. For cross-process usage + * (e.g., Claude Code subprocess), the caller should ensure + * `process.chdir(repoDir)` has been called, or pass `repoDir` + * explicitly. + * @returns `{ workItemId, commentId }` if the state file exists and is valid, + * or `null` if not found or malformed. + */ +export function readProgressCommentId( + repoDir?: string, +): { workItemId: string; commentId: string } | null { + const dir = repoDir ?? process.cwd(); + const filePath = join(dir, STATE_FILE_NAME); + + if (!existsSync(filePath)) return null; + + try { + const content = readFileSync(filePath, 'utf-8').trim(); + const colonIndex = content.indexOf(':'); + if (colonIndex === -1) return null; + + const workItemId = content.slice(0, colonIndex); + const commentId = content.slice(colonIndex + 1); + + if (!workItemId || !commentId) return null; + + return { workItemId, commentId }; + } catch { + return null; + } +} + +/** + * Deletes the progress comment state file. + * + * @param repoDir - Optional directory containing the state file. Defaults to + * `process.cwd()` if not provided. + */ +export function clearProgressCommentId(repoDir?: string): void { + const dir = repoDir ?? process.cwd(); + const filePath = join(dir, STATE_FILE_NAME); + + if (existsSync(filePath)) { + rmSync(filePath); + } +} diff --git a/src/gadgets/pm/core/postComment.ts b/src/gadgets/pm/core/postComment.ts index a7348461..467a6f13 100644 --- a/src/gadgets/pm/core/postComment.ts +++ b/src/gadgets/pm/core/postComment.ts @@ -1,8 +1,24 @@ +import { clearProgressCommentId, readProgressCommentId } from '../../../backends/progressState.js'; import { getPMProvider } from '../../../pm/index.js'; export async function postComment(workItemId: string, text: string): Promise { try { - await getPMProvider().addComment(workItemId, text); + const provider = getPMProvider(); + + // Check if there is a progress comment we should update instead of creating new + const progressState = readProgressCommentId(); + if (progressState && progressState.workItemId === workItemId) { + try { + await provider.updateComment(workItemId, progressState.commentId, text); + clearProgressCommentId(); + return 'Comment posted successfully'; + } catch { + // Fall back to creating a new comment if update fails + clearProgressCommentId(); + } + } + + await provider.addComment(workItemId, text); return 'Comment posted successfully'; } catch (error) { const message = error instanceof Error ? error.message : String(error); diff --git a/src/utils/prUrl.ts b/src/utils/prUrl.ts new file mode 100644 index 00000000..a1a5b223 --- /dev/null +++ b/src/utils/prUrl.ts @@ -0,0 +1,17 @@ +/** + * Shared utility for extracting GitHub PR URLs from text. + * Used by both the llmist backend (agents/base.ts) and the + * Claude Code backend (backends/claude-code/index.ts) to avoid duplication. + */ + +/** + * Extract a GitHub PR URL from arbitrary text output. + * Matches the first occurrence of a GitHub pull request URL. + * + * @param text - The text to search for a PR URL + * @returns The PR URL if found, or undefined + */ +export function extractPRUrl(text: string): string | undefined { + const match = text.match(/https:\/\/github\.com\/[^\s"')\]]+\/pull\/\d+/); + return match ? match[0] : undefined; +} diff --git a/tests/unit/agents/shared/cleanup.test.ts b/tests/unit/agents/shared/cleanup.test.ts new file mode 100644 index 00000000..aee13ae8 --- /dev/null +++ b/tests/unit/agents/shared/cleanup.test.ts @@ -0,0 +1,111 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('../../../../src/utils/lifecycle.js', () => ({ + clearWatchdogCleanup: vi.fn(), + setWatchdogCleanup: vi.fn(), +})); + +vi.mock('../../../../src/utils/repo.js', () => ({ + cleanupTempDir: vi.fn(), +})); + +vi.mock('../../../../src/utils/fileLogger.js', () => ({ + cleanupLogFile: vi.fn(), + cleanupLogDirectory: vi.fn(), + createFileLogger: vi.fn(), +})); + +vi.mock('../../../../src/utils/logging.js', () => ({ + logger: { + warn: vi.fn(), + info: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }, +})); + +import { cleanupAgentResources } from '../../../../src/agents/shared/cleanup.js'; +import { cleanupLogDirectory, cleanupLogFile } from '../../../../src/utils/fileLogger.js'; +import { clearWatchdogCleanup } from '../../../../src/utils/lifecycle.js'; +import { logger } from '../../../../src/utils/logging.js'; +import { cleanupTempDir } from '../../../../src/utils/repo.js'; + +const mockClearWatchdogCleanup = vi.mocked(clearWatchdogCleanup); +const mockCleanupTempDir = vi.mocked(cleanupTempDir); +const mockCleanupLogFile = vi.mocked(cleanupLogFile); +const mockCleanupLogDirectory = vi.mocked(cleanupLogDirectory); + +function makeFileLogger() { + return { + logPath: '/tmp/cascade.log', + llmistLogPath: '/tmp/llmist.log', + llmCallLogger: { logDir: '/tmp/llm-calls' }, + } as unknown as Parameters[1]; +} + +describe('cleanupAgentResources', () => { + const originalEnv = process.env.CASCADE_LOCAL_MODE; + + beforeEach(() => { + vi.clearAllMocks(); + process.env.CASCADE_LOCAL_MODE = undefined; + }); + + afterEach(() => { + process.env.CASCADE_LOCAL_MODE = originalEnv; + }); + + it('clears the watchdog cleanup in all cases', () => { + const fileLogger = makeFileLogger(); + cleanupAgentResources(null, fileLogger); + expect(mockClearWatchdogCleanup).toHaveBeenCalled(); + }); + + it('removes the temp directory when repoDir is set and not local mode', () => { + const fileLogger = makeFileLogger(); + cleanupAgentResources('/tmp/repo-123', fileLogger); + expect(mockCleanupTempDir).toHaveBeenCalledWith('/tmp/repo-123'); + }); + + it('does not remove temp directory when repoDir is null', () => { + const fileLogger = makeFileLogger(); + cleanupAgentResources(null, fileLogger); + expect(mockCleanupTempDir).not.toHaveBeenCalled(); + }); + + it('skips repo deletion when skipRepoDeletion is true', () => { + const fileLogger = makeFileLogger(); + cleanupAgentResources('/tmp/repo-123', fileLogger, true); + expect(mockCleanupTempDir).not.toHaveBeenCalled(); + }); + + it('skips all file deletions in local mode', () => { + process.env.CASCADE_LOCAL_MODE = 'true'; + const fileLogger = makeFileLogger(); + cleanupAgentResources('/tmp/repo-123', fileLogger); + expect(mockCleanupTempDir).not.toHaveBeenCalled(); + expect(mockCleanupLogFile).not.toHaveBeenCalled(); + expect(mockCleanupLogDirectory).not.toHaveBeenCalled(); + }); + + it('cleans up log files in non-local mode', () => { + const fileLogger = makeFileLogger(); + cleanupAgentResources(null, fileLogger); + expect(mockCleanupLogFile).toHaveBeenCalledWith('/tmp/cascade.log'); + expect(mockCleanupLogFile).toHaveBeenCalledWith('/tmp/llmist.log'); + expect(mockCleanupLogDirectory).toHaveBeenCalledWith('/tmp/llm-calls'); + }); + + it('logs a warning but does not throw when temp dir cleanup fails', () => { + mockCleanupTempDir.mockImplementation(() => { + throw new Error('permission denied'); + }); + const fileLogger = makeFileLogger(); + + expect(() => cleanupAgentResources('/tmp/repo-123', fileLogger)).not.toThrow(); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to cleanup temp directory', + expect.objectContaining({ error: 'Error: permission denied' }), + ); + }); +}); diff --git a/tests/unit/agents/shared/runTracking.test.ts b/tests/unit/agents/shared/runTracking.test.ts new file mode 100644 index 00000000..226ff7a1 --- /dev/null +++ b/tests/unit/agents/shared/runTracking.test.ts @@ -0,0 +1,182 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('../../../../src/db/repositories/runsRepository.js', () => ({ + createRun: vi.fn(), + completeRun: vi.fn(), + storeRunLogs: vi.fn(), +})); + +vi.mock('../../../../src/utils/logging.js', () => ({ + logger: { + warn: vi.fn(), + info: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }, +})); + +// Mock fs to control log file existence +vi.mock('node:fs', () => ({ + default: { + existsSync: vi.fn().mockReturnValue(false), + readFileSync: vi.fn().mockReturnValue(''), + }, +})); + +import fs from 'node:fs'; +import { + type RunTrackingInput, + finalizeBackendRun, + tryCompleteRun, + tryCreateRun, + tryStoreRunLogs, +} from '../../../../src/agents/shared/runTracking.js'; +import { + completeRun, + createRun, + storeRunLogs, +} from '../../../../src/db/repositories/runsRepository.js'; +import { logger } from '../../../../src/utils/logging.js'; + +const mockCreateRun = vi.mocked(createRun); +const mockCompleteRun = vi.mocked(completeRun); +const mockStoreRunLogs = vi.mocked(storeRunLogs); +const mockExistsSync = vi.mocked(fs.existsSync); +const mockReadFileSync = vi.mocked(fs.readFileSync); + +function makeFileLogger() { + return { + logPath: '/tmp/test.log', + llmistLogPath: '/tmp/test-llmist.log', + llmCallLogger: { logDir: '/tmp/llm-calls' }, + write: vi.fn(), + close: vi.fn(), + getZippedBuffer: vi.fn().mockResolvedValue(Buffer.from('')), + } as unknown as Parameters[1]; +} + +const baseInput: RunTrackingInput = { + projectId: 'proj-1', + cardId: 'card-123', + agentType: 'implementation', + backendName: 'claude-code', +}; + +describe('tryCreateRun', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('creates a run and returns the run ID', async () => { + mockCreateRun.mockResolvedValue('run-abc'); + + const runId = await tryCreateRun(baseInput, 'claude-3-5-sonnet-20241022', 25); + expect(runId).toBe('run-abc'); + expect(mockCreateRun).toHaveBeenCalledWith({ + projectId: 'proj-1', + cardId: 'card-123', + prNumber: undefined, + agentType: 'implementation', + backend: 'claude-code', + triggerType: undefined, + model: 'claude-3-5-sonnet-20241022', + maxIterations: 25, + }); + }); + + it('returns undefined and logs a warning when createRun throws', async () => { + mockCreateRun.mockRejectedValue(new Error('DB error')); + + const runId = await tryCreateRun(baseInput); + expect(runId).toBeUndefined(); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to create run record', + expect.objectContaining({ error: 'Error: DB error' }), + ); + }); +}); + +describe('tryCompleteRun', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('calls completeRun with the given input', async () => { + mockCompleteRun.mockResolvedValue(undefined); + + await tryCompleteRun('run-xyz', { status: 'completed', success: true }); + expect(mockCompleteRun).toHaveBeenCalledWith('run-xyz', { status: 'completed', success: true }); + }); + + it('swallows errors and logs a warning', async () => { + mockCompleteRun.mockRejectedValue(new Error('network error')); + + await expect( + tryCompleteRun('run-xyz', { status: 'completed', success: true }), + ).resolves.not.toThrow(); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to complete run record', + expect.objectContaining({ error: 'Error: network error' }), + ); + }); +}); + +describe('tryStoreRunLogs', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockExistsSync.mockReturnValue(false); + }); + + it('calls storeRunLogs with undefined logs when files do not exist', async () => { + mockStoreRunLogs.mockResolvedValue(undefined); + const fileLogger = makeFileLogger(); + + await tryStoreRunLogs('run-1', fileLogger); + expect(mockStoreRunLogs).toHaveBeenCalledWith('run-1', undefined, undefined); + }); + + it('reads log file content when files exist', async () => { + mockExistsSync.mockImplementation((p) => String(p).endsWith('.log')); + mockReadFileSync.mockReturnValue('log content'); + mockStoreRunLogs.mockResolvedValue(undefined); + const fileLogger = makeFileLogger(); + + await tryStoreRunLogs('run-1', fileLogger); + expect(mockStoreRunLogs).toHaveBeenCalledWith('run-1', 'log content', 'log content'); + }); + + it('swallows errors and logs a warning', async () => { + mockStoreRunLogs.mockRejectedValue(new Error('write error')); + const fileLogger = makeFileLogger(); + + await expect(tryStoreRunLogs('run-1', fileLogger)).resolves.not.toThrow(); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to store run logs', + expect.objectContaining({ error: 'Error: write error' }), + ); + }); +}); + +describe('finalizeBackendRun', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockExistsSync.mockReturnValue(false); + }); + + it('does nothing when runId is undefined', async () => { + const fileLogger = makeFileLogger(); + await finalizeBackendRun(undefined, fileLogger, { status: 'completed', success: true }); + expect(mockStoreRunLogs).not.toHaveBeenCalled(); + expect(mockCompleteRun).not.toHaveBeenCalled(); + }); + + it('stores logs and completes the run when runId is provided', async () => { + mockStoreRunLogs.mockResolvedValue(undefined); + mockCompleteRun.mockResolvedValue(undefined); + const fileLogger = makeFileLogger(); + + await finalizeBackendRun('run-abc', fileLogger, { status: 'failed', success: false }); + expect(mockStoreRunLogs).toHaveBeenCalledWith('run-abc', undefined, undefined); + expect(mockCompleteRun).toHaveBeenCalledWith('run-abc', { status: 'failed', success: false }); + }); +}); diff --git a/tests/unit/backends/adapter.test.ts b/tests/unit/backends/adapter.test.ts index 6a7e1d88..788bbe80 100644 --- a/tests/unit/backends/adapter.test.ts +++ b/tests/unit/backends/adapter.test.ts @@ -64,6 +64,33 @@ vi.mock('../../../src/backends/agent-profiles.js', () => ({ vi.mock('../../../src/agents/prompts/index.js', () => ({})); +vi.mock('../../../src/agents/shared/promptContext.js', () => ({ + buildPromptContext: vi.fn().mockImplementation( + ( + cardId: string | undefined, + project: { id: string }, + triggerType?: string, + prContext?: { + prNumber?: number; + prBranch?: string; + repoFullName?: string; + headSha?: string; + }, + ) => ({ + cardId, + projectId: project.id, + pmType: 'trello', + ...(prContext && { + prNumber: prContext.prNumber, + prBranch: prContext.prBranch, + repoFullName: prContext.repoFullName, + headSha: prContext.headSha, + triggerType, + }), + }), + ), +})); + const mockCreateRun = vi.fn(); const mockCompleteRun = vi.fn(); const mockStoreRunLogs = vi.fn(); @@ -165,6 +192,12 @@ function makeMockProfile(overrides?: Partial): AgentProfile { needsGitHubToken: false, fetchContext: vi.fn().mockResolvedValue([]), buildTaskPrompt: () => 'Process the work item', + capabilities: { + canEditFiles: true, + canCreatePR: true, + canUpdateChecklists: true, + isReadOnly: false, + }, ...overrides, }; } diff --git a/tests/unit/backends/progress.test.ts b/tests/unit/backends/progress.test.ts index b02eb407..61cc3685 100644 --- a/tests/unit/backends/progress.test.ts +++ b/tests/unit/backends/progress.test.ts @@ -32,10 +32,19 @@ vi.mock('../../../src/config/statusUpdateConfig.js', () => ({ formatGitHubProgressComment: vi.fn(), })); +vi.mock('../../../src/backends/progressState.js', () => ({ + writeProgressCommentId: vi.fn(), + clearProgressCommentId: vi.fn(), +})); + import { syncCompletedTodosToChecklist } from '../../../src/agents/utils/checklistSync.js'; import { createProgressMonitor } from '../../../src/backends/progress.js'; import { callProgressModel } from '../../../src/backends/progressModel.js'; import { ProgressMonitor } from '../../../src/backends/progressMonitor.js'; +import { + clearProgressCommentId, + writeProgressCommentId, +} from '../../../src/backends/progressState.js'; import { formatGitHubProgressComment, formatStatusMessage, @@ -48,6 +57,8 @@ import type { PMProvider } from '../../../src/pm/index.js'; import { getPMProviderOrNull } from '../../../src/pm/index.js'; const mockGetPMProvider = vi.mocked(getPMProviderOrNull); +const mockWriteProgressCommentId = vi.mocked(writeProgressCommentId); +const mockClearProgressCommentId = vi.mocked(clearProgressCommentId); const mockPMProvider = { addComment: vi.fn(), updateComment: vi.fn() }; const mockGithub = vi.mocked(githubClient); const mockGetStatusConfig = vi.mocked(getStatusUpdateConfig); @@ -629,3 +640,163 @@ describe('createProgressMonitor', () => { expect(monitor).toBeInstanceOf(ProgressMonitor); }); }); + +describe('ProgressMonitor — state file integration', () => { + it('writes state file on initial comment when repoDir is provided', async () => { + const logWriter = vi.fn(); + const monitor = new ProgressMonitor({ + agentType: 'planning', + taskDescription: 'Test task', + intervalMinutes: 5, + progressModel: 'test-model', + customModels: [], + logWriter, + repoDir: '/tmp/test-repo', + trello: { cardId: 'card1' }, + }); + + mockGetPMProvider.mockReturnValue(mockPMProvider as unknown as PMProvider); + mockPMProvider.addComment.mockResolvedValue('comment-id-initial'); + + monitor.start(); + await vi.advanceTimersByTimeAsync(0); + + expect(mockWriteProgressCommentId).toHaveBeenCalledWith( + '/tmp/test-repo', + 'card1', + 'comment-id-initial', + ); + monitor.stop(); + }); + + it('does not write state file when repoDir is not provided', async () => { + const monitor = new ProgressMonitor({ + agentType: 'planning', + taskDescription: 'Test task', + intervalMinutes: 5, + progressModel: 'test-model', + customModels: [], + logWriter: vi.fn(), + trello: { cardId: 'card1' }, + }); + + mockGetPMProvider.mockReturnValue(mockPMProvider as unknown as PMProvider); + mockPMProvider.addComment.mockResolvedValue('comment-id-initial'); + + monitor.start(); + await vi.advanceTimersByTimeAsync(0); + + expect(mockWriteProgressCommentId).not.toHaveBeenCalled(); + monitor.stop(); + }); + + it('clears state file on stop()', () => { + const monitor = new ProgressMonitor({ + agentType: 'planning', + taskDescription: 'Test task', + intervalMinutes: 5, + progressModel: 'test-model', + customModels: [], + logWriter: vi.fn(), + repoDir: '/tmp/test-repo', + trello: { cardId: 'card1' }, + }); + + monitor.start(); + monitor.stop(); + + expect(mockClearProgressCommentId).toHaveBeenCalledWith('/tmp/test-repo'); + }); + + it('clears state file on stop() even when repoDir not provided', () => { + const monitor = new ProgressMonitor({ + agentType: 'planning', + taskDescription: 'Test task', + intervalMinutes: 5, + progressModel: 'test-model', + customModels: [], + logWriter: vi.fn(), + trello: { cardId: 'card1' }, + }); + + monitor.start(); + monitor.stop(); + + expect(mockClearProgressCommentId).toHaveBeenCalledWith(undefined); + }); + + it('writes state file from first tick when postInitialComment() failed', async () => { + const logWriter = vi.fn(); + const monitor = new ProgressMonitor({ + agentType: 'planning', + taskDescription: 'Test task', + intervalMinutes: 5, + progressModel: 'test-model', + customModels: [], + logWriter, + repoDir: '/tmp/test-repo', + trello: { cardId: 'card1' }, + }); + + mockGetPMProvider.mockReturnValue(mockPMProvider as unknown as PMProvider); + mockCallProgressModel.mockResolvedValue('Progress update'); + // Initial comment fails (transient API error) + mockPMProvider.addComment + .mockRejectedValueOnce(new Error('API error on initial')) + // First tick succeeds + .mockResolvedValueOnce('comment-id-from-tick'); + + monitor.start(); + // Flush initial comment promise (it fails) + await vi.advanceTimersByTimeAsync(0); + + // Reset mock to track only tick writes + mockWriteProgressCommentId.mockClear(); + + // First tick — enters else branch (progressCommentId is null) + await vi.advanceTimersByTimeAsync(5 * 60 * 1000); + monitor.stop(); + + // State file should be written from the else branch in postProgressToPM + expect(mockWriteProgressCommentId).toHaveBeenCalledWith( + '/tmp/test-repo', + 'card1', + 'comment-id-from-tick', + ); + }); + + it('updates state file when new comment is created after update failure', async () => { + const logWriter = vi.fn(); + const monitor = new ProgressMonitor({ + agentType: 'planning', + taskDescription: 'Test task', + intervalMinutes: 5, + progressModel: 'test-model', + customModels: [], + logWriter, + repoDir: '/tmp/test-repo', + trello: { cardId: 'card1' }, + }); + + mockGetPMProvider.mockReturnValue(mockPMProvider as unknown as PMProvider); + mockCallProgressModel.mockResolvedValue('Progress update'); + mockPMProvider.addComment + .mockResolvedValueOnce('comment-id-initial') + .mockResolvedValueOnce('comment-id-fallback'); + mockPMProvider.updateComment.mockRejectedValue(new Error('Comment not found')); + + monitor.start(); + await vi.advanceTimersByTimeAsync(0); + // First tick — update fails, new comment created + await vi.advanceTimersByTimeAsync(5 * 60 * 1000); + monitor.stop(); + + // writeProgressCommentId called for initial comment and for fallback comment + expect(mockWriteProgressCommentId).toHaveBeenCalledTimes(2); + expect(mockWriteProgressCommentId).toHaveBeenLastCalledWith( + '/tmp/test-repo', + 'card1', + 'comment-id-fallback', + ); + }); +}); diff --git a/tests/unit/backends/progressState.test.ts b/tests/unit/backends/progressState.test.ts new file mode 100644 index 00000000..db1861bf --- /dev/null +++ b/tests/unit/backends/progressState.test.ts @@ -0,0 +1,121 @@ +import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import { + clearProgressCommentId, + readProgressCommentId, + writeProgressCommentId, +} from '../../../src/backends/progressState.js'; + +const STATE_FILE_NAME = '.cascade-progress-comment-id'; + +describe('progressState utilities', () => { + let tmpDir: string; + let origCwd: string; + + beforeEach(() => { + tmpDir = join(tmpdir(), `cascade-test-${Date.now()}`); + mkdirSync(tmpDir, { recursive: true }); + origCwd = process.cwd(); + process.chdir(tmpDir); + }); + + afterEach(() => { + process.chdir(origCwd); + rmSync(tmpDir, { recursive: true, force: true }); + }); + + describe('writeProgressCommentId', () => { + it('writes workItemId:commentId to state file in repoDir', () => { + writeProgressCommentId(tmpDir, 'card123', 'comment456'); + + const stateFile = join(tmpDir, STATE_FILE_NAME); + expect(existsSync(stateFile)).toBe(true); + + const content = readFileSync(stateFile, 'utf-8'); + expect(content).toBe('card123:comment456'); + }); + + it('overwrites existing state file', () => { + writeProgressCommentId(tmpDir, 'card1', 'comment1'); + writeProgressCommentId(tmpDir, 'card2', 'comment2'); + + const result = readProgressCommentId(); + expect(result).toEqual({ workItemId: 'card2', commentId: 'comment2' }); + }); + }); + + describe('readProgressCommentId', () => { + it('returns null when state file does not exist', () => { + const result = readProgressCommentId(); + expect(result).toBeNull(); + }); + + it('returns workItemId and commentId from state file in cwd', () => { + writeProgressCommentId(tmpDir, 'my-card', 'my-comment'); + + const result = readProgressCommentId(); + expect(result).toEqual({ workItemId: 'my-card', commentId: 'my-comment' }); + }); + + it('returns null for malformed state file (no colon)', () => { + writeFileSync(join(tmpDir, STATE_FILE_NAME), 'no-colon-here', 'utf-8'); + + const result = readProgressCommentId(); + expect(result).toBeNull(); + }); + + it('returns null for empty state file', () => { + writeFileSync(join(tmpDir, STATE_FILE_NAME), '', 'utf-8'); + + const result = readProgressCommentId(); + expect(result).toBeNull(); + }); + + it('reads from explicit repoDir when provided', () => { + writeProgressCommentId(tmpDir, 'my-card', 'my-comment'); + + const result = readProgressCommentId(tmpDir); + expect(result).toEqual({ workItemId: 'my-card', commentId: 'my-comment' }); + }); + + it('handles commentId that contains colons (e.g. JIRA IDs)', () => { + writeProgressCommentId(tmpDir, 'PROJ-123', 'comment:with:colons'); + + const result = readProgressCommentId(); + expect(result).toEqual({ workItemId: 'PROJ-123', commentId: 'comment:with:colons' }); + }); + }); + + describe('clearProgressCommentId', () => { + it('deletes state file from repoDir', () => { + writeProgressCommentId(tmpDir, 'card1', 'comment1'); + expect(existsSync(join(tmpDir, STATE_FILE_NAME))).toBe(true); + + clearProgressCommentId(tmpDir); + expect(existsSync(join(tmpDir, STATE_FILE_NAME))).toBe(false); + }); + + it('deletes state file from cwd when no repoDir provided', () => { + writeProgressCommentId(tmpDir, 'card1', 'comment1'); + expect(existsSync(join(tmpDir, STATE_FILE_NAME))).toBe(true); + + clearProgressCommentId(); + expect(existsSync(join(tmpDir, STATE_FILE_NAME))).toBe(false); + }); + + it('does not throw when state file does not exist', () => { + expect(() => clearProgressCommentId(tmpDir)).not.toThrow(); + }); + + it('leaves readProgressCommentId returning null after clear', () => { + writeProgressCommentId(tmpDir, 'card1', 'comment1'); + clearProgressCommentId(tmpDir); + + const result = readProgressCommentId(); + expect(result).toBeNull(); + }); + }); +}); diff --git a/tests/unit/gadgets/pm/core/postComment.test.ts b/tests/unit/gadgets/pm/core/postComment.test.ts index af5bebec..7cf2fa62 100644 --- a/tests/unit/gadgets/pm/core/postComment.test.ts +++ b/tests/unit/gadgets/pm/core/postComment.test.ts @@ -8,10 +8,23 @@ vi.mock('../../../../../src/pm/index.js', () => ({ getPMProvider: vi.fn(() => mockProvider), })); +vi.mock('../../../../../src/backends/progressState.js', () => ({ + readProgressCommentId: vi.fn(() => null), + clearProgressCommentId: vi.fn(), +})); + +import { + clearProgressCommentId, + readProgressCommentId, +} from '../../../../../src/backends/progressState.js'; import { postComment } from '../../../../../src/gadgets/pm/core/postComment.js'; +const mockReadProgressCommentId = vi.mocked(readProgressCommentId); +const mockClearProgressCommentId = vi.mocked(clearProgressCommentId); + beforeEach(() => { vi.clearAllMocks(); + mockReadProgressCommentId.mockReturnValue(null); }); describe('postComment', () => { @@ -48,4 +61,74 @@ describe('postComment', () => { expect(result).toBe('Error posting comment: string error'); }); + + describe('progress comment replacement', () => { + it('updates existing progress comment when state matches workItemId', async () => { + mockReadProgressCommentId.mockReturnValue({ workItemId: 'item1', commentId: 'comment-42' }); + mockProvider.updateComment.mockResolvedValue(undefined); + + const result = await postComment('item1', 'Final summary'); + + expect(mockProvider.updateComment).toHaveBeenCalledWith( + 'item1', + 'comment-42', + 'Final summary', + ); + expect(mockProvider.addComment).not.toHaveBeenCalled(); + expect(mockClearProgressCommentId).toHaveBeenCalled(); + expect(result).toBe('Comment posted successfully'); + }); + + it('does not update when workItemId does not match state', async () => { + mockReadProgressCommentId.mockReturnValue({ + workItemId: 'other-item', + commentId: 'comment-42', + }); + mockProvider.addComment.mockResolvedValue(undefined); + + await postComment('item1', 'My comment'); + + expect(mockProvider.updateComment).not.toHaveBeenCalled(); + expect(mockProvider.addComment).toHaveBeenCalledWith('item1', 'My comment'); + }); + + it('falls back to addComment when updateComment fails, and clears state', async () => { + mockReadProgressCommentId.mockReturnValue({ workItemId: 'item1', commentId: 'comment-42' }); + mockProvider.updateComment.mockRejectedValue(new Error('Comment not found')); + mockProvider.addComment.mockResolvedValue(undefined); + + const result = await postComment('item1', 'Final summary'); + + expect(mockProvider.updateComment).toHaveBeenCalledWith( + 'item1', + 'comment-42', + 'Final summary', + ); + expect(mockProvider.addComment).toHaveBeenCalledWith('item1', 'Final summary'); + expect(mockClearProgressCommentId).toHaveBeenCalled(); + expect(result).toBe('Comment posted successfully'); + }); + + it('creates new comment (no state) when no progress comment exists', async () => { + mockReadProgressCommentId.mockReturnValue(null); + mockProvider.addComment.mockResolvedValue(undefined); + + const result = await postComment('item1', 'New comment'); + + expect(mockProvider.updateComment).not.toHaveBeenCalled(); + expect(mockProvider.addComment).toHaveBeenCalledWith('item1', 'New comment'); + expect(result).toBe('Comment posted successfully'); + }); + + it('clears state before fallback so subsequent calls create new comments', async () => { + mockReadProgressCommentId.mockReturnValue({ workItemId: 'item1', commentId: 'comment-42' }); + mockProvider.updateComment.mockRejectedValue(new Error('gone')); + mockProvider.addComment.mockResolvedValue(undefined); + + await postComment('item1', 'text'); + + // State is cleared even when update fails + expect(mockClearProgressCommentId).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/tests/unit/utils/prUrl.test.ts b/tests/unit/utils/prUrl.test.ts new file mode 100644 index 00000000..25e367d1 --- /dev/null +++ b/tests/unit/utils/prUrl.test.ts @@ -0,0 +1,46 @@ +import { describe, expect, it } from 'vitest'; + +import { extractPRUrl } from '../../../src/utils/prUrl.js'; + +describe('extractPRUrl', () => { + it('extracts a GitHub PR URL from plain text', () => { + const text = 'Created PR: https://github.com/owner/repo/pull/42'; + expect(extractPRUrl(text)).toBe('https://github.com/owner/repo/pull/42'); + }); + + it('extracts a PR URL when surrounded by other text', () => { + const text = 'Done! See https://github.com/acme/cascade/pull/123 for details.'; + expect(extractPRUrl(text)).toBe('https://github.com/acme/cascade/pull/123'); + }); + + it('returns undefined when no PR URL is present', () => { + expect(extractPRUrl('No URLs here.')).toBeUndefined(); + }); + + it('returns undefined for non-PR GitHub URLs', () => { + const text = 'Check https://github.com/owner/repo/issues/5 for context'; + expect(extractPRUrl(text)).toBeUndefined(); + }); + + it('extracts the first PR URL when multiple are present', () => { + const text = 'First: https://github.com/a/b/pull/1 and second: https://github.com/c/d/pull/2'; + expect(extractPRUrl(text)).toBe('https://github.com/a/b/pull/1'); + }); + + it('extracts a PR URL when followed by a period (end of sentence)', () => { + // The regex stops at \d+ so the trailing period is not included + const text = 'See https://github.com/owner/repo/pull/42.'; + const url = extractPRUrl(text); + expect(url).toBe('https://github.com/owner/repo/pull/42'); + }); + + it('returns undefined for empty string', () => { + expect(extractPRUrl('')).toBeUndefined(); + }); + + it('handles URLs in JSON output', () => { + const json = JSON.stringify({ prUrl: 'https://github.com/org/repo/pull/99', success: true }); + // In JSON, URL is surrounded by quotes which are excluded by the regex + expect(extractPRUrl(json)).toBe('https://github.com/org/repo/pull/99'); + }); +});