From 728fd4fdf1fea11dca40e64c2902659646c39fa6 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Wed, 18 Feb 2026 18:19:35 +0000 Subject: [PATCH 1/7] refactor(agents): extract shared lifecycle utilities to eliminate code duplication --- src/agents/base.ts | 68 +------ src/agents/review.ts | 48 +---- src/agents/shared/cleanup.ts | 38 ++++ src/agents/shared/lifecycle.ts | 70 +------ src/agents/shared/prFormatting.ts | 51 ++++++ src/agents/shared/promptContext.ts | 56 ++++++ src/agents/shared/runTracking.ts | 94 ++++++++++ src/backends/adapter.ts | 161 ++++------------ src/backends/agent-profiles.ts | 114 ++++++++---- src/backends/claude-code/index.ts | 9 +- src/utils/prUrl.ts | 17 ++ tests/unit/agents/shared/cleanup.test.ts | 111 +++++++++++ tests/unit/agents/shared/runTracking.test.ts | 182 +++++++++++++++++++ tests/unit/backends/adapter.test.ts | 34 ++++ tests/unit/utils/prUrl.test.ts | 46 +++++ 15 files changed, 759 insertions(+), 340 deletions(-) create mode 100644 src/agents/shared/cleanup.ts create mode 100644 src/agents/shared/promptContext.ts create mode 100644 src/agents/shared/runTracking.ts create mode 100644 src/utils/prUrl.ts create mode 100644 tests/unit/agents/shared/cleanup.test.ts create mode 100644 tests/unit/agents/shared/runTracking.test.ts create mode 100644 tests/unit/utils/prUrl.test.ts diff --git a/src/agents/base.ts b/src/agents/base.ts index e8047bb0..48e37507 100644 --- a/src/agents/base.ts +++ b/src/agents/base.ts @@ -30,10 +30,11 @@ 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 { 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 +58,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 +94,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 }, @@ -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( 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/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/lifecycle.ts b/src/agents/shared/lifecycle.ts index 19c2cdc5..ddbc66b0 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") */ @@ -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, @@ -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..49620535 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, @@ -674,6 +581,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..25f06196 100644 --- a/src/backends/agent-profiles.ts +++ b/src/backends/agent-profiles.ts @@ -1,6 +1,4 @@ import { execFileSync } from 'node:child_process'; -import { readFile } from 'node:fs/promises'; -import { join } from 'node:path'; import { formatPRComments, @@ -8,13 +6,13 @@ import { 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'; @@ -62,6 +60,28 @@ const SESSION_TOOL = 'Finish'; const ALL_SDK_TOOLS = ['Read', 'Write', 'Edit', 'Bash', 'Glob', 'Grep']; const READ_ONLY_SDK_TOOLS = ['Read', 'Bash', 'Glob', 'Grep']; +// ============================================================================ +// AgentCapabilities +// ============================================================================ + +/** + * Describes what a particular agent type is allowed to do. + * Used to gate both llmist gadget inclusion (base.ts) and + * Claude Code tool filtering (agent-profiles.ts) from a single source. + */ +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; + /** Does the agent need a GitHub token for API calls? */ + needsGitHubToken: boolean; + /** True for agents that only interact with the PM system (no repo changes) */ + isReadOnly: boolean; +} + // ============================================================================ // AgentProfile Interface // ============================================================================ @@ -95,6 +115,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 +198,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 +482,13 @@ const briefingProfile: AgentProfile = { needsGitHubToken: false, fetchContext: fetchWorkItemContext, buildTaskPrompt: buildWorkItemTaskPrompt, + capabilities: { + canEditFiles: true, + canCreatePR: false, + canUpdateChecklists: true, + needsGitHubToken: false, + isReadOnly: false, + }, }; const planningProfile: AgentProfile = { @@ -500,6 +498,13 @@ const planningProfile: AgentProfile = { needsGitHubToken: false, fetchContext: fetchWorkItemContext, buildTaskPrompt: buildWorkItemTaskPrompt, + capabilities: { + canEditFiles: false, + canCreatePR: false, + canUpdateChecklists: false, + needsGitHubToken: false, + isReadOnly: true, + }, }; const reviewProfile: AgentProfile = { @@ -509,6 +514,13 @@ const reviewProfile: AgentProfile = { needsGitHubToken: true, fetchContext: fetchReviewContext, buildTaskPrompt: buildReviewTaskPrompt, + capabilities: { + canEditFiles: false, + canCreatePR: false, + canUpdateChecklists: false, + needsGitHubToken: true, + isReadOnly: true, + }, async preExecute({ input, logWriter }: PreExecuteParams): Promise { const repoFullName = input.repoFullName as string; @@ -528,6 +540,13 @@ const respondToPlanningCommentProfile: AgentProfile = { needsGitHubToken: false, fetchContext: fetchWorkItemContext, buildTaskPrompt: buildCommentResponseTaskPrompt, + capabilities: { + canEditFiles: false, + canCreatePR: false, + canUpdateChecklists: true, + needsGitHubToken: false, + isReadOnly: true, + }, }; const respondToCIProfile: AgentProfile = { @@ -544,6 +563,13 @@ const respondToCIProfile: AgentProfile = { blockGitPush: false, fetchContext: fetchCIContext, buildTaskPrompt: buildCITaskPrompt, + capabilities: { + canEditFiles: true, + canCreatePR: false, + canUpdateChecklists: true, + needsGitHubToken: true, + isReadOnly: false, + }, async preExecute({ input, logWriter }: PreExecuteParams): Promise { const repoFullName = input.repoFullName as string; @@ -568,6 +594,13 @@ const respondToPRCommentProfile: AgentProfile = { blockGitPush: false, fetchContext: fetchPRCommentResponseContext, buildTaskPrompt: buildPRCommentResponseTaskPrompt, + capabilities: { + canEditFiles: true, + canCreatePR: false, + canUpdateChecklists: false, + needsGitHubToken: true, + isReadOnly: false, + }, }; const defaultProfile: AgentProfile = { @@ -577,11 +610,22 @@ const defaultProfile: AgentProfile = { needsGitHubToken: false, fetchContext: fetchWorkItemContext, buildTaskPrompt: buildWorkItemTaskPrompt, + capabilities: { + canEditFiles: true, + canCreatePR: true, + canUpdateChecklists: true, + needsGitHubToken: false, + isReadOnly: false, + }, }; const implementationProfile: AgentProfile = { ...defaultProfile, needsGitHubToken: true, + capabilities: { + ...defaultProfile.capabilities, + needsGitHubToken: true, + }, }; // ============================================================================ 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/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..2cf6d547 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,13 @@ function makeMockProfile(overrides?: Partial): AgentProfile { needsGitHubToken: false, fetchContext: vi.fn().mockResolvedValue([]), buildTaskPrompt: () => 'Process the work item', + capabilities: { + canEditFiles: true, + canCreatePR: true, + canUpdateChecklists: true, + needsGitHubToken: false, + isReadOnly: false, + }, ...overrides, }; } 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'); + }); +}); From 5df9dc7b5bc80c82efbbde6dea4811a6581fcbf7 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Wed, 18 Feb 2026 18:20:27 +0000 Subject: [PATCH 2/7] feat(progress): replace initial progress comment with final summary via state file --- src/backends/progress.ts | 2 + src/backends/progressMonitor.ts | 92 +++++++----- src/backends/progressState.ts | 77 ++++++++++ src/gadgets/pm/core/postComment.ts | 18 ++- tests/unit/backends/progress.test.ts | 131 ++++++++++++++++++ tests/unit/backends/progressState.test.ts | 114 +++++++++++++++ .../unit/gadgets/pm/core/postComment.test.ts | 83 +++++++++++ 7 files changed, 479 insertions(+), 38 deletions(-) create mode 100644 src/backends/progressState.ts create mode 100644 tests/unit/backends/progressState.test.ts 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..a86b7ec3 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,8 @@ export class ProgressMonitor implements ProgressReporter { clearInterval(this.timer); this.timer = null; } + // Clean up state file on stop + clearProgressCommentId(this.config.repoDir); } // ── Internal ── @@ -155,6 +159,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 +230,52 @@ 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 + 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.progressCommentId = await provider.addComment(cardId, summary); + this.config.logWriter('INFO', 'Posted progress update to work item', { + cardId, + commentId: 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..47f861cf --- /dev/null +++ b/src/backends/progressState.ts @@ -0,0 +1,77 @@ +/** + * 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, rmSync, writeFileSync } from 'node:fs'; +import { readFileSync } 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 in the current + * working directory (process.cwd()). + * + * @returns `{ workItemId, commentId }` if the state file exists and is valid, + * or `null` if not found or malformed. + */ +export function readProgressCommentId(): { workItemId: string; commentId: string } | null { + const filePath = join(process.cwd(), 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/tests/unit/backends/progress.test.ts b/tests/unit/backends/progress.test.ts index b02eb407..10692126 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,123 @@ 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('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..116226e4 --- /dev/null +++ b/tests/unit/backends/progressState.test.ts @@ -0,0 +1,114 @@ +import { existsSync, mkdirSync, rmSync } 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 = require('node:fs').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)', () => { + require('node:fs').writeFileSync(join(tmpDir, STATE_FILE_NAME), 'no-colon-here', 'utf-8'); + + const result = readProgressCommentId(); + expect(result).toBeNull(); + }); + + it('returns null for empty state file', () => { + require('node:fs').writeFileSync(join(tmpDir, STATE_FILE_NAME), '', 'utf-8'); + + const result = readProgressCommentId(); + expect(result).toBeNull(); + }); + + 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); + }); + }); }); From d77f5ce4371364b54f91a1781b11212f452deb7b Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Wed, 18 Feb 2026 18:59:32 +0000 Subject: [PATCH 3/7] refactor(agents): wire up AgentCapabilities and remove duplication - Extract AgentCapabilities interface and registry to agents/shared/capabilities.ts - Replace hardcoded agentType checks in base.ts with capability lookups - Remove needsGitHubToken from AgentCapabilities (top-level AgentProfile field is single source of truth) - Update adapter test to match simplified capabilities Co-Authored-By: Claude Opus 4.6 --- src/agents/base.ts | 22 +++--- src/agents/shared/capabilities.ts | 105 ++++++++++++++++++++++++++++ src/backends/agent-profiles.ts | 35 +--------- tests/unit/backends/adapter.test.ts | 1 - 4 files changed, 118 insertions(+), 45 deletions(-) create mode 100644 src/agents/shared/capabilities.ts diff --git a/src/agents/base.ts b/src/agents/base.ts index 48e37507..682100af 100644 --- a/src/agents/base.ts +++ b/src/agents/base.ts @@ -32,6 +32,7 @@ import type { AgentInput, AgentResult, CascadeConfig, ProjectConfig } from '../t import { logger } from '../utils/logging.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'; @@ -262,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(), @@ -281,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(), @@ -290,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(), ]; diff --git a/src/agents/shared/capabilities.ts b/src/agents/shared/capabilities.ts new file mode 100644 index 00000000..986956c6 --- /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. Must stay in sync with the AgentProfile + * definitions in backends/agent-profiles.ts. + */ +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/backends/agent-profiles.ts b/src/backends/agent-profiles.ts index 25f06196..a9520364 100644 --- a/src/backends/agent-profiles.ts +++ b/src/backends/agent-profiles.ts @@ -1,5 +1,7 @@ import { execFileSync } from 'node:child_process'; +import type { AgentCapabilities } from '../agents/shared/capabilities.js'; +export type { AgentCapabilities } from '../agents/shared/capabilities.js'; import { formatPRComments, formatPRDetails, @@ -60,28 +62,6 @@ const SESSION_TOOL = 'Finish'; const ALL_SDK_TOOLS = ['Read', 'Write', 'Edit', 'Bash', 'Glob', 'Grep']; const READ_ONLY_SDK_TOOLS = ['Read', 'Bash', 'Glob', 'Grep']; -// ============================================================================ -// AgentCapabilities -// ============================================================================ - -/** - * Describes what a particular agent type is allowed to do. - * Used to gate both llmist gadget inclusion (base.ts) and - * Claude Code tool filtering (agent-profiles.ts) from a single source. - */ -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; - /** Does the agent need a GitHub token for API calls? */ - needsGitHubToken: boolean; - /** True for agents that only interact with the PM system (no repo changes) */ - isReadOnly: boolean; -} - // ============================================================================ // AgentProfile Interface // ============================================================================ @@ -486,7 +466,6 @@ const briefingProfile: AgentProfile = { canEditFiles: true, canCreatePR: false, canUpdateChecklists: true, - needsGitHubToken: false, isReadOnly: false, }, }; @@ -502,7 +481,6 @@ const planningProfile: AgentProfile = { canEditFiles: false, canCreatePR: false, canUpdateChecklists: false, - needsGitHubToken: false, isReadOnly: true, }, }; @@ -518,7 +496,6 @@ const reviewProfile: AgentProfile = { canEditFiles: false, canCreatePR: false, canUpdateChecklists: false, - needsGitHubToken: true, isReadOnly: true, }, @@ -544,7 +521,6 @@ const respondToPlanningCommentProfile: AgentProfile = { canEditFiles: false, canCreatePR: false, canUpdateChecklists: true, - needsGitHubToken: false, isReadOnly: true, }, }; @@ -567,7 +543,6 @@ const respondToCIProfile: AgentProfile = { canEditFiles: true, canCreatePR: false, canUpdateChecklists: true, - needsGitHubToken: true, isReadOnly: false, }, @@ -598,7 +573,6 @@ const respondToPRCommentProfile: AgentProfile = { canEditFiles: true, canCreatePR: false, canUpdateChecklists: false, - needsGitHubToken: true, isReadOnly: false, }, }; @@ -614,7 +588,6 @@ const defaultProfile: AgentProfile = { canEditFiles: true, canCreatePR: true, canUpdateChecklists: true, - needsGitHubToken: false, isReadOnly: false, }, }; @@ -622,10 +595,6 @@ const defaultProfile: AgentProfile = { const implementationProfile: AgentProfile = { ...defaultProfile, needsGitHubToken: true, - capabilities: { - ...defaultProfile.capabilities, - needsGitHubToken: true, - }, }; // ============================================================================ diff --git a/tests/unit/backends/adapter.test.ts b/tests/unit/backends/adapter.test.ts index 2cf6d547..788bbe80 100644 --- a/tests/unit/backends/adapter.test.ts +++ b/tests/unit/backends/adapter.test.ts @@ -196,7 +196,6 @@ function makeMockProfile(overrides?: Partial): AgentProfile { canEditFiles: true, canCreatePR: true, canUpdateChecklists: true, - needsGitHubToken: false, isReadOnly: false, }, ...overrides, From e91c930ba6f0a544007f5098049e3f061e2ad3a3 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Wed, 18 Feb 2026 19:08:40 +0000 Subject: [PATCH 4/7] fix(progress): wire repoDir to call sites and fix API asymmetry - Pass repoDir to createProgressMonitor in adapter.ts and base.ts so the state file is actually written (previously dead code) - Update lifecycle.ts callback signature to forward repoDir - Add optional repoDir param to readProgressCommentId() for API symmetry with writeProgressCommentId() and clearProgressCommentId() - Add .cascade-progress-comment-id to .gitignore - Merge split fs imports in progressState.ts - Replace require('node:fs') with ESM imports in progressState tests - Add clarifying comment on maybeWriteStateFile call path Co-Authored-By: Claude Opus 4.6 --- .gitignore | 3 +++ src/agents/base.ts | 3 ++- src/agents/shared/githubAgent.ts | 2 +- src/agents/shared/lifecycle.ts | 4 ++-- src/backends/adapter.ts | 1 + src/backends/progressMonitor.ts | 4 +++- src/backends/progressState.ts | 18 ++++++++++++------ tests/unit/backends/progressState.test.ts | 15 +++++++++++---- 8 files changed, 35 insertions(+), 15 deletions(-) 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..a654afad 100644 --- a/src/agents/base.ts +++ b/src/agents/base.ts @@ -613,7 +613,7 @@ export async function executeAgent( ctx.implementationSteps, ), - createProgressMonitor: (fileLogger) => + createProgressMonitor: (fileLogger, repoDir) => createProgressMonitor({ logWriter: fileLogger.write.bind(fileLogger), agentType, @@ -621,6 +621,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/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..87576d42 100644 --- a/src/agents/shared/lifecycle.ts +++ b/src/agents/shared/lifecycle.ts @@ -80,7 +80,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; @@ -344,7 +344,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, diff --git a/src/backends/adapter.ts b/src/backends/adapter.ts index 2dc8c3ab..3779c0e3 100644 --- a/src/backends/adapter.ts +++ b/src/backends/adapter.ts @@ -597,6 +597,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, }); diff --git a/src/backends/progressMonitor.ts b/src/backends/progressMonitor.ts index a86b7ec3..81dbedb8 100644 --- a/src/backends/progressMonitor.ts +++ b/src/backends/progressMonitor.ts @@ -241,7 +241,9 @@ export class ProgressMonitor implements ProgressReporter { if (!provider) return; if (this.progressCommentId) { - // Subsequent ticks: update the existing comment + // 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', { diff --git a/src/backends/progressState.ts b/src/backends/progressState.ts index 47f861cf..41215d1f 100644 --- a/src/backends/progressState.ts +++ b/src/backends/progressState.ts @@ -11,8 +11,7 @@ * File format: `:` */ -import { existsSync, rmSync, writeFileSync } from 'node:fs'; -import { readFileSync } from 'node:fs'; +import { existsSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; const STATE_FILE_NAME = '.cascade-progress-comment-id'; @@ -34,14 +33,21 @@ export function writeProgressCommentId( } /** - * Reads the progress comment state from the state file in the current - * working directory (process.cwd()). + * 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(): { workItemId: string; commentId: string } | null { - const filePath = join(process.cwd(), STATE_FILE_NAME); +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; diff --git a/tests/unit/backends/progressState.test.ts b/tests/unit/backends/progressState.test.ts index 116226e4..db1861bf 100644 --- a/tests/unit/backends/progressState.test.ts +++ b/tests/unit/backends/progressState.test.ts @@ -1,4 +1,4 @@ -import { existsSync, mkdirSync, rmSync } from 'node:fs'; +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'; @@ -34,7 +34,7 @@ describe('progressState utilities', () => { const stateFile = join(tmpDir, STATE_FILE_NAME); expect(existsSync(stateFile)).toBe(true); - const content = require('node:fs').readFileSync(stateFile, 'utf-8'); + const content = readFileSync(stateFile, 'utf-8'); expect(content).toBe('card123:comment456'); }); @@ -61,19 +61,26 @@ describe('progressState utilities', () => { }); it('returns null for malformed state file (no colon)', () => { - require('node:fs').writeFileSync(join(tmpDir, STATE_FILE_NAME), 'no-colon-here', 'utf-8'); + writeFileSync(join(tmpDir, STATE_FILE_NAME), 'no-colon-here', 'utf-8'); const result = readProgressCommentId(); expect(result).toBeNull(); }); it('returns null for empty state file', () => { - require('node:fs').writeFileSync(join(tmpDir, STATE_FILE_NAME), '', 'utf-8'); + 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'); From 02d5e8ea2456b61d14673160621d6a5e87ff8299 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Wed, 18 Feb 2026 19:18:10 +0000 Subject: [PATCH 5/7] refactor(agents): make capabilities.ts single source of truth for AgentCapabilities Replace inline capability objects in agent-profiles.ts with getAgentCapabilities() lookups, eliminating the dual-registry sync risk flagged in code review. Co-Authored-By: Claude Opus 4.6 --- src/agents/shared/capabilities.ts | 4 +-- src/backends/agent-profiles.ts | 52 ++++++------------------------- 2 files changed, 11 insertions(+), 45 deletions(-) diff --git a/src/agents/shared/capabilities.ts b/src/agents/shared/capabilities.ts index 986956c6..336ecae7 100644 --- a/src/agents/shared/capabilities.ts +++ b/src/agents/shared/capabilities.ts @@ -36,8 +36,8 @@ const DEFAULT_CAPABILITIES: AgentCapabilities = { }; /** - * Capabilities per agent type. Must stay in sync with the AgentProfile - * definitions in backends/agent-profiles.ts. + * Capabilities per agent type — single source of truth. + * AgentProfile in backends/agent-profiles.ts consumes these via getAgentCapabilities(). */ const CAPABILITIES_REGISTRY: Record = { briefing: { diff --git a/src/backends/agent-profiles.ts b/src/backends/agent-profiles.ts index a9520364..6262fb78 100644 --- a/src/backends/agent-profiles.ts +++ b/src/backends/agent-profiles.ts @@ -1,6 +1,6 @@ import { execFileSync } from 'node:child_process'; -import type { AgentCapabilities } from '../agents/shared/capabilities.js'; +import { type AgentCapabilities, getAgentCapabilities } from '../agents/shared/capabilities.js'; export type { AgentCapabilities } from '../agents/shared/capabilities.js'; import { formatPRComments, @@ -462,12 +462,7 @@ const briefingProfile: AgentProfile = { needsGitHubToken: false, fetchContext: fetchWorkItemContext, buildTaskPrompt: buildWorkItemTaskPrompt, - capabilities: { - canEditFiles: true, - canCreatePR: false, - canUpdateChecklists: true, - isReadOnly: false, - }, + capabilities: getAgentCapabilities('briefing'), }; const planningProfile: AgentProfile = { @@ -477,12 +472,7 @@ const planningProfile: AgentProfile = { needsGitHubToken: false, fetchContext: fetchWorkItemContext, buildTaskPrompt: buildWorkItemTaskPrompt, - capabilities: { - canEditFiles: false, - canCreatePR: false, - canUpdateChecklists: false, - isReadOnly: true, - }, + capabilities: getAgentCapabilities('planning'), }; const reviewProfile: AgentProfile = { @@ -492,12 +482,7 @@ const reviewProfile: AgentProfile = { needsGitHubToken: true, fetchContext: fetchReviewContext, buildTaskPrompt: buildReviewTaskPrompt, - capabilities: { - canEditFiles: false, - canCreatePR: false, - canUpdateChecklists: false, - isReadOnly: true, - }, + capabilities: getAgentCapabilities('review'), async preExecute({ input, logWriter }: PreExecuteParams): Promise { const repoFullName = input.repoFullName as string; @@ -517,12 +502,7 @@ const respondToPlanningCommentProfile: AgentProfile = { needsGitHubToken: false, fetchContext: fetchWorkItemContext, buildTaskPrompt: buildCommentResponseTaskPrompt, - capabilities: { - canEditFiles: false, - canCreatePR: false, - canUpdateChecklists: true, - isReadOnly: true, - }, + capabilities: getAgentCapabilities('respond-to-planning-comment'), }; const respondToCIProfile: AgentProfile = { @@ -539,12 +519,7 @@ const respondToCIProfile: AgentProfile = { blockGitPush: false, fetchContext: fetchCIContext, buildTaskPrompt: buildCITaskPrompt, - capabilities: { - canEditFiles: true, - canCreatePR: false, - canUpdateChecklists: true, - isReadOnly: false, - }, + capabilities: getAgentCapabilities('respond-to-ci'), async preExecute({ input, logWriter }: PreExecuteParams): Promise { const repoFullName = input.repoFullName as string; @@ -569,12 +544,7 @@ const respondToPRCommentProfile: AgentProfile = { blockGitPush: false, fetchContext: fetchPRCommentResponseContext, buildTaskPrompt: buildPRCommentResponseTaskPrompt, - capabilities: { - canEditFiles: true, - canCreatePR: false, - canUpdateChecklists: false, - isReadOnly: false, - }, + capabilities: getAgentCapabilities('respond-to-pr-comment'), }; const defaultProfile: AgentProfile = { @@ -584,17 +554,13 @@ const defaultProfile: AgentProfile = { needsGitHubToken: false, fetchContext: fetchWorkItemContext, buildTaskPrompt: buildWorkItemTaskPrompt, - capabilities: { - canEditFiles: true, - canCreatePR: true, - canUpdateChecklists: true, - isReadOnly: false, - }, + capabilities: getAgentCapabilities('debug'), }; const implementationProfile: AgentProfile = { ...defaultProfile, needsGitHubToken: true, + capabilities: getAgentCapabilities('implementation'), }; // ============================================================================ From 597fce5b03e0b011f988e10f56e40d2c3068be0a Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Wed, 18 Feb 2026 19:31:53 +0000 Subject: [PATCH 6/7] fix(progress): write state file from first tick when initial comment fails The `else` branch in `postProgressToPM` (entered when `postInitialComment()` fails and the first tick creates the comment) was not calling `maybeWriteStateFile()`, leaving the state file unwritten. This meant the PostComment gadget would not find the progress comment ID and would create a duplicate instead of replacing it. Co-Authored-By: Claude Opus 4.6 --- src/backends/progressMonitor.ts | 6 ++++- tests/unit/backends/progress.test.ts | 40 ++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/backends/progressMonitor.ts b/src/backends/progressMonitor.ts index 81dbedb8..87d341ba 100644 --- a/src/backends/progressMonitor.ts +++ b/src/backends/progressMonitor.ts @@ -264,12 +264,16 @@ export class ProgressMonitor implements ProgressReporter { this.maybeWriteStateFile(cardId, this.progressCommentId); } } else { - // First tick: create the comment and store its ID + // 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); } } diff --git a/tests/unit/backends/progress.test.ts b/tests/unit/backends/progress.test.ts index 10692126..61cc3685 100644 --- a/tests/unit/backends/progress.test.ts +++ b/tests/unit/backends/progress.test.ts @@ -725,6 +725,46 @@ describe('ProgressMonitor — state file integration', () => { 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({ From 16b9325290b8c202c25d3ac647b639af7e04022a Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Wed, 18 Feb 2026 19:41:27 +0000 Subject: [PATCH 7/7] fix(progress): wrap state file cleanup in try-catch in stop() clearProgressCommentId() uses rmSync which can throw on permissions errors. Since stop() is called from finally blocks in lifecycle.ts and adapter.ts, an uncaught exception would mask the actual agent execution result. Wrap in try-catch for best-effort cleanup. Co-Authored-By: Claude Opus 4.6 --- src/backends/progressMonitor.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/backends/progressMonitor.ts b/src/backends/progressMonitor.ts index 87d341ba..9aef81d9 100644 --- a/src/backends/progressMonitor.ts +++ b/src/backends/progressMonitor.ts @@ -137,8 +137,13 @@ export class ProgressMonitor implements ProgressReporter { clearInterval(this.timer); this.timer = null; } - // Clean up state file on stop - clearProgressCommentId(this.config.repoDir); + // 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 ──