feat: task scheduling, CLI commands, and task resumption (v0.4.0)

[dean0x]

Summary

• Task Scheduling: Cron and one-time schedule support with full lifecycle management (create, list, get, cancel, pause, resume), execution history, missed run policies, and timezone support
• CLI Commands: 6 schedule subcommands + pipeline for chained sequential tasks + resume for failed task resumption
• Task Resumption: Auto-checkpoints on task completion/failure, git state capture, enriched prompt construction for resumed tasks with retry chain tracking
• Schedule Service Extraction: Business logic extracted from MCP adapter into ScheduleManagerService for CLI reuse
• FK Cascade Fix: Replaced INSERT OR REPLACE with proper INSERT OR IGNORE + UPDATE to prevent child row destruction during task/schedule updates
• Comprehensive Tests: 11 new test files, 844+ tests passing across 36 files

New MCP Tools

• ScheduleTask, ListSchedules, GetSchedule, CancelSchedule, PauseSchedule, ResumeSchedule
• ResumeTask

New CLI Commands

• claudine schedule create|list|get|cancel|pause|resume
• claudine pipeline
• claudine resume

Database Migrations

• v3: schedules table
• v4: schedule_executions table
• v5: task_checkpoints table

Test plan

• npm run test:core (273 tests)
• npm run test:handlers (76 tests)
• npm run test:repositories (105 tests)
• npm run test:implementations (218 tests)
• npm run test:adapters (38 tests)
• npm run test:cli (83 tests)
• npm run test:integration (51 tests)
• npm run build clean
• Release notes at docs/releases/RELEASE_NOTES_v0.4.0.md

[qodo-free-for-open-source-projects]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Unvalidated datetime parsing Description: User-provided ISO 8601 datetime strings are parsed using Date.parse() without sanitization, which could allow malformed input to cause unexpected behavior or denial of service through invalid date parsing. mcp-adapter.ts [868-881] Referred Code if (data.scheduledAt) { scheduledAtMs = Date.parse(data.scheduledAt); if (isNaN(scheduledAtMs)) { return { content: [{ type: 'text', text: `Invalid scheduledAt datetime: ${data.scheduledAt}` }], isError: true, }; } if (scheduledAtMs <= Date.now()) { return { content: [{ type: 'text', text: 'scheduledAt must be in the future' }], isError: true, }; }
	Unsafe JSON deserialization Description: JSON parsing of task_template from database without validation could allow execution of malicious task configurations if database is compromised or contains corrupted data. schedule-repository.ts [401-406] Referred Code let taskTemplate: DelegateRequest; try { taskTemplate = JSON.parse(data.task_template) as DelegateRequest; } catch (e) { throw new Error(`Invalid task_template JSON for schedule ${data.id}: ${e}`); }
Ticket Compliance
🟡	🎫 #45 🟢 Support standard 5-field cron expression parsing for recurring schedules Support one-time scheduling via ISO 8601 timestamps Implement timezone support using IANA format with UTC as default Implement three missed run policies: skip (default), catchup, and fail Create new MCP tools: ScheduleTask, ListSchedules, CancelSchedule (and additional pause/resume tools) Track schedule execution history for all executions Implement timer-based execution checking every 60 seconds Ensure schedules persist across server restarts Create database tables for scheduled_tasks and schedule_history Emit schedule lifecycle events (TaskScheduleCreated, TaskScheduleTriggered, etc.) Create ScheduleHandler event handler with timer Create ScheduleRepository for SQLite persistence Add cron-parser dependency Invalid cron expressions should return INVALID_INPUT error Past timestamps for one-time schedules should return error Invalid timezones should return error with valid options Missed runs must be handled per configured policy ⚪ Valid cron expressions should create schedules with correct next_run_time One-time schedules should execute at specified time and not repeat Tasks should execute within 60 seconds of scheduled time Schedules must persist across server restarts Schedule history must track all executions with status Concurrent execution must be prevented (skip if previous still running) Server restart must load all active schedules correctly
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Potential Sensitive Data Logging: The taskTemplate.prompt field is logged in responses and may contain user-provided sensitive information that should be sanitized or truncated. Referred Code prompt: schedule.taskTemplate.prompt.substring(0, 100) + (schedule.taskTemplate.prompt.length > 100 ? '...' : ''), priority: schedule.taskTemplate.priority, Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-free-for-open-source-projects]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Prevent data loss on schedule update Suggestion Impact: Added a prepared updateStmt using an UPDATE schedules ... WHERE id = @id query and changed the update() method to execute this UPDATE instead of calling save() (which used INSERT OR REPLACE), explicitly preventing ON DELETE CASCADE from deleting child execution rows. code diff: + private readonly updateStmt: SQLite.Statement; private readonly recordExecutionStmt: SQLite.Statement; private readonly getExecutionByIdStmt: SQLite.Statement; private readonly getExecutionHistoryStmt: SQLite.Statement; @@ -125,6 +152,26 @@ ) `); + // UPDATE preserves the row (unlike INSERT OR REPLACE which deletes + inserts, + // triggering ON DELETE CASCADE on child tables like schedule_executions) + this.updateStmt = this.db.prepare(` + UPDATE schedules SET + task_template = @taskTemplate, + schedule_type = @scheduleType, + cron_expression = @cronExpression, + scheduled_at = @scheduledAt, + timezone = @timezone, + missed_run_policy = @missedRunPolicy, + status = @status, + max_runs = @maxRuns, + run_count = @runCount, + last_run_at = @lastRunAt, + next_run_at = @nextRunAt, + expires_at = @expiresAt, + updated_at = @updatedAt + WHERE id = @id + `); + this.findByIdStmt = this.db.prepare(` SELECT * FROM schedules WHERE id = ? `); @@ -134,7 +181,7 @@ `); this.findByStatusStmt = this.db.prepare(` - SELECT * FROM schedules WHERE status = ? ORDER BY created_at DESC + SELECT * FROM schedules WHERE status = ? ORDER BY created_at DESC LIMIT ? OFFSET ? `); // ARCHITECTURE: Critical query for scheduler tick - finds schedules ready to trigger @@ -233,8 +280,29 @@ updatedAt: Date.now(), }; - // Save the updated schedule - return this.save(updatedSchedule); + // Use UPDATE (not INSERT OR REPLACE) to preserve child rows + // INSERT OR REPLACE deletes the old row first, triggering ON DELETE CASCADE + return tryCatchAsync( + async () => { + this.updateStmt.run({ + id: updatedSchedule.id, + taskTemplate: JSON.stringify(updatedSchedule.taskTemplate), + scheduleType: updatedSchedule.scheduleType, + cronExpression: updatedSchedule.cronExpression ?? null, + scheduledAt: updatedSchedule.scheduledAt ?? null, + timezone: updatedSchedule.timezone, + missedRunPolicy: updatedSchedule.missedRunPolicy, + status: updatedSchedule.status, + maxRuns: updatedSchedule.maxRuns ?? null, + runCount: updatedSchedule.runCount, + lastRunAt: updatedSchedule.lastRunAt ?? null, + nextRunAt: updatedSchedule.nextRunAt ?? null, + expiresAt: updatedSchedule.expiresAt ?? null, + updatedAt: updatedSchedule.updatedAt, + }); + }, + operationErrorHandler('update schedule', { scheduleId: id }) + ); } Replace the INSERT OR REPLACE logic in the update method with a dedicated UPDATE statement to prevent the cascading deletion of a schedule's execution history. src/implementations/schedule-repository.ts [214-238] -this.saveStmt = this.db.prepare(` - INSERT OR REPLACE INTO schedules ( - id, task_template, schedule_type, cron_expression, scheduled_at, - timezone, missed_run_policy, status, max_runs, run_count, - last_run_at, next_run_at, expires_at, created_at, updated_at - ) VALUES ( - @id, @taskTemplate, @scheduleType, @cronExpression, @scheduledAt, - @timezone, @missedRunPolicy, @status, @maxRuns, @runCount, - @lastRunAt, @nextRunAt, @expiresAt, @createdAt, @updatedAt - ) -`); +private readonly updateStmt: SQLite.Statement; +... +constructor(database: Database) { + this.db = database.getDatabase(); + + // Prepare statements for better performance + this.saveStmt = this.db.prepare(` + INSERT INTO schedules ( + id, task_template, schedule_type, cron_expression, scheduled_at, + timezone, missed_run_policy, status, max_runs, run_count, + last_run_at, next_run_at, expires_at, created_at, updated_at + ) VALUES ( + @id, @taskTemplate, @scheduleType, @cronExpression, @scheduledAt, + @timezone, @missedRunPolicy, @status, @maxRuns, @runCount, + @lastRunAt, @nextRunAt, @expiresAt, @createdAt, @updatedAt + ) + `); + + this.updateStmt = this.db.prepare(` + UPDATE schedules + SET + task_template = @taskTemplate, + schedule_type = @scheduleType, + cron_expression = @cronExpression, + scheduled_at = @scheduledAt, + timezone = @timezone, + missed_run_policy = @missedRunPolicy, + status = @status, + max_runs = @maxRuns, + run_count = @runCount, + last_run_at = @lastRunAt, + next_run_at = @nextRunAt, + expires_at = @expiresAt, + updated_at = @updatedAt + WHERE id = @id + `); ... async update(id: ScheduleId, update: Partial<Schedule>): Promise<Result<void>> { - // First get the existing schedule - const existingResult = await this.findById(id); + return tryCatchAsync( + async () => { + const existingResult = await this.findById(id); + if (!existingResult.ok) { + throw existingResult.error; + } + if (!existingResult.value) { + throw new ClaudineError(ErrorCode.TASK_NOT_FOUND, `Schedule ${id} not found`); + } - if (!existingResult.ok) { - return existingResult; - } + const schedule: Schedule = { ...existingResult.value, ...update, updatedAt: Date.now() }; - if (!existingResult.value) { - return err(new ClaudineError( - ErrorCode.TASK_NOT_FOUND, - `Schedule ${id} not found` - )); - } - - // Merge updates with existing schedule - const updatedSchedule: Schedule = { - ...existingResult.value, - ...update, - updatedAt: Date.now(), - }; - - // Save the updated schedule - return this.save(updatedSchedule); + this.updateStmt.run({ + id: schedule.id, + taskTemplate: JSON.stringify(schedule.taskTemplate), + scheduleType: schedule.scheduleType, + cronExpression: schedule.cronExpression ?? null, + scheduledAt: schedule.scheduledAt ?? null, + timezone: schedule.timezone, + missedRunPolicy: schedule.missedRunPolicy, + status: schedule.status, + maxRuns: schedule.maxRuns ?? null, + runCount: schedule.runCount, + lastRunAt: schedule.lastRunAt ?? null, + nextRunAt: schedule.nextRunAt ?? null, + expiresAt: schedule.expiresAt ?? null, + updatedAt: schedule.updatedAt, + }); + }, + operationErrorHandler('update schedule', { scheduleId: id }) + ); } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 10 __ Why: This suggestion correctly identifies a critical data loss bug where updating a schedule via INSERT OR REPLACE triggers a cascading delete on its execution history, and it provides the correct fix.	High
	Correctly handle missed one-time schedules In updateNextRun, change the status of a missed one-time schedule from COMPLETED to EXPIRED to more accurately represent that its execution window has passed without it running. src/services/schedule-executor.ts [284-293] private async updateNextRun(schedule: Schedule): Promise<void> { if (schedule.scheduleType === ScheduleType.ONE_TIME) { - // One-time schedules don't repeat - mark as completed + // One-time schedules don't repeat - mark as expired if missed await this.scheduleRepo.update(schedule.id, { - status: ScheduleStatus.COMPLETED, + status: ScheduleStatus.EXPIRED, nextRunAt: undefined, }); - this.logger.info('One-time schedule completed', { scheduleId: schedule.id }); + this.logger.info('One-time schedule expired after missed run', { scheduleId: schedule.id }); return; } ... [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that marking a missed one-time schedule as COMPLETED is semantically incorrect. Changing the status to EXPIRED more accurately reflects the schedule's state, improving clarity.	Low
High-level	✅ Simplify by centralizing schedule logic Suggestion Impact: The MCP adapter’s schedule endpoints were refactored into thin wrappers: extensive schedule validation, nextRunAt calculation, and direct repository/event-bus usage were removed and replaced with calls into a centralized ScheduleService (create/list/get/cancel/pause/resume). This effectively centralizes scheduling business logic outside the adapter, aligning with the intent of the suggestion (though via a service rather than “only emit ScheduleCreated”). code diff: # File: src/adapters/mcp-adapter.ts @@ -5,22 +5,20 @@ import { Server } from '@modelcontextprotocol/sdk/server/index.js'; import { z } from 'zod'; -import { TaskManager, Logger, ScheduleRepository } from '../core/interfaces.js'; +import { TaskManager, Logger, ScheduleService } from '../core/interfaces.js'; import { DelegateRequest, + ResumeTaskRequest, Priority, TaskId, - Schedule, ScheduleId, ScheduleStatus, ScheduleType, - MissedRunPolicy, - createSchedule, + ScheduleCreateRequest, } from '../core/domain.js'; import { match } from '../core/result.js'; import { validatePath } from '../utils/validation.js'; -import { validateCronExpression, isValidTimezone, getNextRunTime } from '../utils/cron.js'; -import { EventBus } from '../core/events/event-bus.js'; +import { toMissedRunPolicy } from '../services/schedule-manager.js'; import pkg from '../../package.json' with { type: 'json' }; // Zod schemas for MCP protocol validation @@ -60,6 +58,11 @@ taskId: z.string(), }); +const ResumeTaskSchema = z.object({ + taskId: z.string().describe('Task ID to resume (must be in terminal state)'), + additionalContext: z.string().max(4000).optional().describe('Additional instructions for the resumed task'), +}); + // Schedule-related Zod schemas (v0.4.0 Task Scheduling) const ScheduleTaskSchema = z.object({ prompt: z.string().min(1).max(4000).describe('Task prompt to execute'), @@ -72,6 +75,7 @@ workingDirectory: z.string().optional(), maxRuns: z.number().min(1).optional().describe('Maximum number of runs for cron schedules'), expiresAt: z.string().optional().describe('ISO 8601 datetime when schedule expires'), + afterSchedule: z.string().optional().describe('Schedule ID to chain after (new tasks depend on this schedule\'s latest task)'), }); const ListSchedulesSchema = z.object({ @@ -99,14 +103,19 @@ scheduleId: z.string().describe('Schedule ID to resume'), }); +/** Standard MCP tool response shape */ +interface MCPToolResponse { + content: { type: string; text: string }[]; + isError?: boolean; +} + export class MCPAdapter { private server: Server; constructor( private readonly taskManager: TaskManager, private readonly logger: Logger, - private readonly scheduleRepository?: ScheduleRepository, - private readonly eventBus?: EventBus + private readonly scheduleService: ScheduleService ) { this.server = new Server( { @@ -160,6 +169,8 @@ return await this.handleCancelTask(args); case 'RetryTask': return await this.handleRetryTask(args); + case 'ResumeTask': + return await this.handleResumeTask(args); // Schedule tools (v0.4.0 Task Scheduling) case 'ScheduleTask': return await this.handleScheduleTask(args); @@ -221,8 +232,8 @@ }, useWorktree: { type: 'boolean', - description: 'Create a git worktree for isolated execution', - default: true, + description: 'Create a git worktree for isolated execution (opt-in)', + default: false, }, worktreeCleanup: { type: 'string', @@ -357,6 +368,26 @@ required: ['taskId'], }, }, + { + name: 'ResumeTask', + description: 'Resume a failed/completed task with enriched context from its checkpoint (smart retry with previous output, errors, and git state)', + inputSchema: { + type: 'object', + properties: { + taskId: { + type: 'string', + description: 'Task ID to resume (must be in terminal state: completed, failed, or cancelled)', + pattern: '^task-[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$', + }, + additionalContext: { + type: 'string', + description: 'Additional instructions or context for the resumed task', + maxLength: 4000, + }, + }, + required: ['taskId'], + }, + }, // Schedule tools (v0.4.0 Task Scheduling) { name: 'ScheduleTask', @@ -404,6 +435,10 @@ expiresAt: { type: 'string', description: 'ISO 8601 expiration datetime', + }, + afterSchedule: { + type: 'string', + description: 'Schedule ID to chain after (new tasks depend on this schedule\'s latest task)', }, }, required: ['prompt', 'scheduleType'], @@ -810,16 +845,55 @@ }); } + private async handleResumeTask(args: unknown): Promise<MCPToolResponse> { + const parseResult = ResumeTaskSchema.safeParse(args); + + if (!parseResult.success) { + return { + content: [{ type: 'text', text: `Validation error: ${parseResult.error.message}` }], + isError: true, + }; + } + + const { taskId, additionalContext } = parseResult.data; + + const request: ResumeTaskRequest = { + taskId: TaskId(taskId), + additionalContext, + }; + + const result = await this.taskManager.resume(request); + + return match(result, { + ok: (newTask) => ({ + content: [{ + type: 'text', + text: JSON.stringify({ + success: true, + message: `Task ${taskId} resumed successfully`, + newTaskId: newTask.id, + retryCount: newTask.retryCount || 1, + parentTaskId: newTask.parentTaskId, + }, null, 2), + }], + }), + err: (error) => ({ + content: [{ type: 'text', text: JSON.stringify({ success: false, error: error.message }, null, 2) }], + isError: true, + }), + }); + } + // ============================================================================ // SCHEDULE HANDLERS (v0.4.0 Task Scheduling) + // Thin wrappers: Zod parse -> service call -> format MCP response // ============================================================================ /** * Handle ScheduleTask tool call * Creates a new schedule for recurring or one-time task execution */ - private async handleScheduleTask(args: unknown): Promise<{ content: { type: string; text: string }[]; isError?: boolean }> { - // Validate input at boundary + private async handleScheduleTask(args: unknown): Promise<MCPToolResponse> { const parseResult = ScheduleTaskSchema.safeParse(args); if (!parseResult.success) { return { @@ -829,143 +903,48 @@ } const data = parseResult.data; - // Validate schedule type requirements - if (data.scheduleType === 'cron' && !data.cronExpression) { - return { - content: [{ type: 'text', text: 'cronExpression is required for cron schedules' }], - isError: true, - }; - } - if (data.scheduleType === 'one_time' && !data.scheduledAt) { - return { - content: [{ type: 'text', text: 'scheduledAt is required for one-time schedules' }], - isError: true, - }; - } - - // Validate cron expression - if (data.cronExpression) { - const cronResult = validateCronExpression(data.cronExpression); - if (!cronResult.ok) { - return { - content: [{ type: 'text', text: cronResult.error.message }], - isError: true, - }; - } - } - - // Validate timezone - const tz = data.timezone ?? 'UTC'; - if (!isValidTimezone(tz)) { - return { - content: [{ type: 'text', text: `Invalid timezone: ${data.timezone}` }], - isError: true, - }; - } - - // Parse scheduledAt if provided - let scheduledAtMs: number | undefined; - if (data.scheduledAt) { - scheduledAtMs = Date.parse(data.scheduledAt); - if (isNaN(scheduledAtMs)) { - return { - content: [{ type: 'text', text: `Invalid scheduledAt datetime: ${data.scheduledAt}` }], - isError: true, - }; - } - if (scheduledAtMs <= Date.now()) { - return { - content: [{ type: 'text', text: 'scheduledAt must be in the future' }], - isError: true, - }; - } - } - - // Parse expiresAt if provided - let expiresAtMs: number | undefined; - if (data.expiresAt) { - expiresAtMs = Date.parse(data.expiresAt); - if (isNaN(expiresAtMs)) { - return { - content: [{ type: 'text', text: `Invalid expiresAt datetime: ${data.expiresAt}` }], - isError: true, - }; - } - } - - // Calculate nextRunAt - let nextRunAt: number; - if (data.scheduleType === 'cron' && data.cronExpression) { - const nextResult = getNextRunTime(data.cronExpression, tz); - if (!nextResult.ok) { - return { - content: [{ type: 'text', text: nextResult.error.message }], - isError: true, - }; - } - nextRunAt = nextResult.value; - } else { - nextRunAt = scheduledAtMs!; - } - - // Create schedule - const schedule = createSchedule({ - taskTemplate: { - prompt: data.prompt, - priority: data.priority as Priority | undefined, - workingDirectory: data.workingDirectory, - }, + const request: ScheduleCreateRequest = { + prompt: data.prompt, scheduleType: data.scheduleType === 'cron' ? ScheduleType.CRON : ScheduleType.ONE_TIME, cronExpression: data.cronExpression, - scheduledAt: scheduledAtMs, - timezone: tz, - missedRunPolicy: data.missedRunPolicy === 'catchup' ? MissedRunPolicy.CATCHUP - : data.missedRunPolicy === 'fail' ? MissedRunPolicy.FAIL - : MissedRunPolicy.SKIP, + scheduledAt: data.scheduledAt, + timezone: data.timezone, + missedRunPolicy: toMissedRunPolicy(data.missedRunPolicy), + priority: data.priority as Priority | undefined, + workingDirectory: data.workingDirectory, maxRuns: data.maxRuns, - expiresAt: expiresAtMs, + expiresAt: data.expiresAt, + afterScheduleId: data.afterSchedule ? ScheduleId(data.afterSchedule) : undefined, + }; + + const result = await this.scheduleService.createSchedule(request); + + return match(result, { + ok: (schedule) => ({ + content: [{ + type: 'text', + text: JSON.stringify({ + success: true, + scheduleId: schedule.id, + scheduleType: schedule.scheduleType, + nextRunAt: schedule.nextRunAt ? new Date(schedule.nextRunAt).toISOString() : null, + timezone: schedule.timezone, + status: schedule.status, + }, null, 2), + }], + }), + err: (error) => ({ + content: [{ type: 'text', text: JSON.stringify({ success: false, error: error.message }, null, 2) }], + isError: true, + }), }); - - // Check dependencies are available - if (!this.scheduleRepository || !this.eventBus) { - return { - content: [{ type: 'text', text: 'Schedule repository not available' }], - isError: true, - }; - } - - // Save schedule - const saveResult = await this.scheduleRepository.save(schedule); - if (!saveResult.ok) { - return { - content: [{ type: 'text', text: `Failed to save schedule: ${saveResult.error.message}` }], - isError: true, - }; - } - - // Emit event - await this.eventBus.emit('ScheduleCreated', { schedule }); - - return { - content: [{ - type: 'text', - text: JSON.stringify({ - success: true, - scheduleId: schedule.id, - scheduleType: schedule.scheduleType, - nextRunAt: new Date(nextRunAt).toISOString(), - timezone: schedule.timezone, - status: schedule.status, - }, null, 2), - }], - }; } /** * Handle ListSchedules tool call * Lists schedules with optional status filter and pagination */ - private async handleListSchedules(args: unknown): Promise<{ content: { type: string; text: string }[]; isError?: boolean }> { + private async handleListSchedules(args: unknown): Promise<MCPToolResponse> { const parseResult = ListSchedulesSchema.safeParse(args); if (!parseResult.success) { return { @@ -974,64 +953,49 @@ }; } - if (!this.scheduleRepository) { - return { - content: [{ type: 'text', text: 'Schedule repository not available' }], - isError: true, - }; - } - const { status, limit, offset } = parseResult.data; - let schedules: readonly Schedule[]; - if (status) { - const statusResult = await this.scheduleRepository.findByStatus(status as ScheduleStatus); - if (!statusResult.ok) { + const result = await this.scheduleService.listSchedules( + status as ScheduleStatus | undefined, + limit, + offset + ); + + return match(result, { + ok: (schedules) => { + const simplifiedSchedules = schedules.map(s => ({ + id: s.id, + status: s.status, + scheduleType: s.scheduleType, + cronExpression: s.cronExpression, + nextRunAt: s.nextRunAt ? new Date(s.nextRunAt).toISOString() : null, + runCount: s.runCount, + maxRuns: s.maxRuns, + })); + return { - content: [{ type: 'text', text: `Failed to list schedules: ${statusResult.error.message}` }], - isError: true, + content: [{ + type: 'text', + text: JSON.stringify({ + success: true, + schedules: simplifiedSchedules, + count: simplifiedSchedules.length, + }, null, 2), + }], }; - } - // Apply pagination manually for findByStatus - schedules = statusResult.value.slice(offset, offset + limit); - } else { - const result = await this.scheduleRepository.findAll(limit, offset); - if (!result.ok) { - return { - content: [{ type: 'text', text: `Failed to list schedules: ${result.error.message}` }], - isError: true, - }; - } - schedules = result.value; - } - - const simplifiedSchedules = schedules.map(s => ({ - id: s.id, - status: s.status, - scheduleType: s.scheduleType, - cronExpression: s.cronExpression, - nextRunAt: s.nextRunAt ? new Date(s.nextRunAt).toISOString() : null, - runCount: s.runCount, - maxRuns: s.maxRuns, - })); - - return { - content: [{ - type: 'text', - text: JSON.stringify({ - success: true, - schedules: simplifiedSchedules, - count: simplifiedSchedules.length, - }, null, 2), - }], - }; + }, + err: (error) => ({ + content: [{ type: 'text', text: JSON.stringify({ success: false, error: error.message }, null, 2) }], + isError: true, + }), + }); } /** * Handle GetSchedule tool call * Gets details of a specific schedule with optional execution history */ - private async handleGetSchedule(args: unknown): Promise<{ content: { type: string; text: string }[]; isError?: boolean }> { + private async handleGetSchedule(args: unknown): Promise<MCPToolResponse> { const parseResult = GetScheduleSchema.safeParse(args); if (!parseResult.success) { return { @@ -1040,86 +1004,70 @@ }; } - if (!this.scheduleRepository) { - return { - content: [{ type: 'text', text: 'Schedule repository not available' }], - isError: true, - }; - } - const { scheduleId, includeHistory, historyLimit } = parseResult.data; - const scheduleResult = await this.scheduleRepository.findById(ScheduleId(scheduleId)); - if (!scheduleResult.ok) { - return { - content: [{ type: 'text', text: `Failed to get schedule: ${scheduleResult.error.message}` }], - isError: true, - }; - } - - if (!scheduleResult.value) { - return { - content: [{ type: 'text', text: `Schedule ${scheduleId} not found` }], - isError: true, - }; - } - - const schedule = scheduleResult.value; - const response: Record<string, unknown> = { - success: true, - schedule: { - id: schedule.id, - status: schedule.status, - scheduleType: schedule.scheduleType, - cronExpression: schedule.cronExpression, - scheduledAt: schedule.scheduledAt ? new Date(schedule.scheduledAt).toISOString() : null, - timezone: schedule.timezone, - missedRunPolicy: schedule.missedRunPolicy, - maxRuns: schedule.maxRuns, - runCount: schedule.runCount, - lastRunAt: schedule.lastRunAt ? new Date(schedule.lastRunAt).toISOString() : null, - nextRunAt: schedule.nextRunAt ? new Date(schedule.nextRunAt).toISOString() : null, - expiresAt: schedule.expiresAt ? new Date(schedule.expiresAt).toISOString() : null, - createdAt: new Date(schedule.createdAt).toISOString(), - updatedAt: new Date(schedule.updatedAt).toISOString(), - taskTemplate: { - prompt: schedule.taskTemplate.prompt.substring(0, 100) + (schedule.taskTemplate.prompt.length > 100 ? '...' : ''), - priority: schedule.taskTemplate.priority, - workingDirectory: schedule.taskTemplate.workingDirectory, - }, + const result = await this.scheduleService.getSchedule( + ScheduleId(scheduleId), + includeHistory, + historyLimit + ); + + return match(result, { + ok: ({ schedule, history }) => { + const response: Record<string, unknown> = { + success: true, + schedule: { + id: schedule.id, + status: schedule.status, + scheduleType: schedule.scheduleType, + cronExpression: schedule.cronExpression, + scheduledAt: schedule.scheduledAt ? new Date(schedule.scheduledAt).toISOString() : null, + timezone: schedule.timezone, + missedRunPolicy: schedule.missedRunPolicy, + maxRuns: schedule.maxRuns, + runCount: schedule.runCount, + lastRunAt: schedule.lastRunAt ? new Date(schedule.lastRunAt).toISOString() : null, + nextRunAt: schedule.nextRunAt ? new Date(schedule.nextRunAt).toISOString() : null, + expiresAt: schedule.expiresAt ? new Date(schedule.expiresAt).toISOString() : null, + createdAt: new Date(schedule.createdAt).toISOString(), + updatedAt: new Date(schedule.updatedAt).toISOString(), + taskTemplate: { + prompt: schedule.taskTemplate.prompt.substring(0, 100) + (schedule.taskTemplate.prompt.length > 100 ? '...' : ''), + priority: schedule.taskTemplate.priority, + workingDirectory: schedule.taskTemplate.workingDirectory, + }, + }, + }; + + if (history) { + response.history = history.map(h => ({ + scheduledFor: new Date(h.scheduledFor).toISOString(), + executedAt: h.executedAt ? new Date(h.executedAt).toISOString() : null, + status: h.status, + taskId: h.taskId, + errorMessage: h.errorMessage, + })); + } + + return { + content: [{ + type: 'text', + text: JSON.stringify(response, null, 2), + }], + }; }, - }; - - // Include execution history if requested - if (includeHistory) { - const historyResult = await this.scheduleRepository.getExecutionHistory( - ScheduleId(scheduleId), - historyLimit - ); - if (historyResult.ok) { - response.history = historyResult.value.map(h => ({ - scheduledFor: new Date(h.scheduledFor).toISOString(), - executedAt: h.executedAt ? new Date(h.executedAt).toISOString() : null, - status: h.status, - taskId: h.taskId, - errorMessage: h.errorMessage, - })); - } - } - - return { - content: [{ - type: 'text', - text: JSON.stringify(response, null, 2), - }], - }; + err: (error) => ({ + content: [{ type: 'text', text: JSON.stringify({ success: false, error: error.message }, null, 2) }], + isError: true, + }), + }); } /** * Handle CancelSchedule tool call * Cancels an active schedule */ - private async handleCancelSchedule(args: unknown): Promise<{ content: { type: string; text: string }[]; isError?: boolean }> { + private async handleCancelSchedule(args: unknown): Promise<MCPToolResponse> { const parseResult = CancelScheduleSchema.safeParse(args); if (!parseResult.success) { return { @@ -1128,54 +1076,33 @@ }; } - if (!this.scheduleRepository || !this.eventBus) { - return { - content: [{ type: 'text', text: 'Schedule repository not available' }], - isError: true, - }; - } - const { scheduleId, reason } = parseResult.data; - // Verify schedule exists - const scheduleResult = await this.scheduleRepository.findById(ScheduleId(scheduleId)); - if (!scheduleResult.ok) { - return { - content: [{ type: 'text', text: `Failed to get schedule: ${scheduleResult.error.message}` }], - isError: true, - }; - } - - if (!scheduleResult.value) { - return { - content: [{ type: 'text', text: `Schedule ${scheduleId} not found` }], - isError: true, - }; - } - - // Emit cancel event - await this.eventBus.emit('ScheduleCancelled', { - scheduleId: ScheduleId(scheduleId), - reason, + const result = await this.scheduleService.cancelSchedule(ScheduleId(scheduleId), reason); + + return match(result, { + ok: () => ({ + content: [{ + type: 'text', + text: JSON.stringify({ + success: true, + message: `Schedule ${scheduleId} cancelled`, + reason, + }, null, 2), + }], + }), + err: (error) => ({ + content: [{ type: 'text', text: JSON.stringify({ success: false, error: error.message }, null, 2) }], + isError: true, + }), }); - - return { - content: [{ - type: 'text', - text: JSON.stringify({ - success: true, - message: `Schedule ${scheduleId} cancelled`, - reason, - }, null, 2), - }], - }; } /** * Handle PauseSchedule tool call * Pauses an active schedule (can be resumed later) */ - private async handlePauseSchedule(args: unknown): Promise<{ content: { type: string; text: string }[]; isError?: boolean }> { + private async handlePauseSchedule(args: unknown): Promise<MCPToolResponse> { const parseResult = PauseScheduleSchema.safeParse(args); if (!parseResult.success) { return { @@ -1184,59 +1111,32 @@ }; } - if (!this.scheduleRepository || !this.eventBus) { - return { - content: [{ type: 'text', text: 'Schedule repository not available' }], - isError: true, - }; - } - const { scheduleId } = parseResult.data; - // Verify schedule exists and is active - const scheduleResult = await this.scheduleRepository.findById(ScheduleId(scheduleId)); - if (!scheduleResult.ok) { - return { - content: [{ type: 'text', text: `Failed to get schedule: ${scheduleResult.error.message}` }], - isError: true, - }; - } - - if (!scheduleResult.value) { - return { - content: [{ type: 'text', text: `Schedule ${scheduleId} not found` }], - isError: true, - }; - } - - if (scheduleResult.value.status !== ScheduleStatus.ACTIVE) { - return { - content: [{ type: 'text', text: `Schedule ${scheduleId} is not active (status: ${scheduleResult.value.status})` }], - isError: true, - }; - } - - // Emit pause event - await this.eventBus.emit('SchedulePaused', { - scheduleId: ScheduleId(scheduleId), + const result = await this.scheduleService.pauseSchedule(ScheduleId(scheduleId)); + + return match(result, { + ok: () => ({ + content: [{ + type: 'text', + text: JSON.stringify({ + success: true, + message: `Schedule ${scheduleId} paused`, + }, null, 2), + }], + }), + err: (error) => ({ + content: [{ type: 'text', text: JSON.stringify({ success: false, error: error.message }, null, 2) }], + isError: true, + }), }); - - return { - content: [{ - type: 'text', - text: JSON.stringify({ - success: true, - message: `Schedule ${scheduleId} paused`, - }, null, 2), - }], - }; } /** * Handle ResumeSchedule tool call * Resumes a paused schedule */ - private async handleResumeSchedule(args: unknown): Promise<{ content: { type: string; text: string }[]; isError?: boolean }> { + private async handleResumeSchedule(args: unknown): Promise<MCPToolResponse> { const parseResult = ResumeScheduleSchema.safeParse(args); if (!parseResult.success) { return { @@ -1245,51 +1145,25 @@ }; } - if (!this.scheduleRepository || !this.eventBus) { - return { - content: [{ type: 'text', text: 'Schedule repository not available' }], - isError: true, - }; - } - const { scheduleId } = parseResult.data; - // Verify schedule exists and is paused - const scheduleResult = await this.scheduleRepository.findById(ScheduleId(scheduleId)); - if (!scheduleResult.ok) { - return { - content: [{ type: 'text', text: `Failed to get schedule: ${scheduleResult.error.message}` }], - isError: true, - }; - } - - if (!scheduleResult.value) { - return { - content: [{ type: 'text', text: `Schedule ${scheduleId} not found` }], - isError: true, - }; - } - - if (scheduleResult.value.status !== ScheduleStatus.PAUSED) { - return { - content: [{ type: 'text', text: `Schedule ${scheduleId} is not paused (status: ${scheduleResult.value.status})` }], - isError: true, - }; - } - - // Emit resume event - await this.eventBus.emit('ScheduleResumed', { - scheduleId: ScheduleId(scheduleId), + const result = await this.scheduleService.resumeSchedule(ScheduleId(scheduleId)); + + return match(result, { + ok: () => ({ + content: [{ + type: 'text', + text: JSON.stringify({ + success: true, + message: `Schedule ${scheduleId} resumed`, + }, null, 2), + }], + }), + err: (error) => ({ + content: [{ type: 'text', text: JSON.stringify({ success: false, error: error.message }, null, 2) }], + isError: true, + }), }); - - return { - content: [{ - type: 'text', - text: JSON.stringify({ - success: true, - message: `Schedule ${scheduleId} resumed`, - }, null, 2), - }], - }; } } Centralize schedule creation logic by removing duplicated validation and nextRunAt calculation from mcp-adapter.ts. The adapter should only emit a ScheduleCreated event, leaving all business logic and persistence to the ScheduleHandler. Examples: src/adapters/mcp-adapter.ts [821-962] private async handleScheduleTask(args: unknown): Promise<{ content: { type: string; text: string }[]; isError?: boolean }> { // Validate input at boundary const parseResult = ScheduleTaskSchema.safeParse(args); if (!parseResult.success) { return { content: [{ type: 'text', text: `Validation error: ${parseResult.error.message}` }], isError: true, }; } const data = parseResult.data; ... (clipped 132 lines) src/services/handlers/schedule-handler.ts [145-229] private async handleScheduleCreated(event: ScheduleCreatedEvent): Promise<void> { await this.handleEvent(event, async (e) => { const schedule = e.schedule; this.logger.info('Processing new schedule', { scheduleId: schedule.id, type: schedule.scheduleType, timezone: schedule.timezone }); ... (clipped 75 lines) Solution Walkthrough: Before: // in mcp-adapter.ts async function handleScheduleTask(args) { // ... validation ... const nextRunAt = getNextRunTime(cronExpr, tz); const schedule = createSchedule({ ...data }); // The adapter saves the schedule await scheduleRepository.save(schedule); // The adapter emits an event await eventBus.emit('ScheduleCreated', { schedule }); } // in schedule-handler.ts async function handleScheduleCreated(event) { const schedule = event.schedule; // ... re-validation ... const nextRunAt = getNextRunTime(schedule.cronExpression, ...); const updatedSchedule = updateSchedule(schedule, { nextRunAt }); // The handler saves the schedule AGAIN await scheduleRepo.save(updatedSchedule); } After: // in mcp-adapter.ts async function handleScheduleTask(args) { // ... basic input validation ... const schedule = createSchedule({ ...data }); // The adapter ONLY emits an event with the raw request await eventBus.emit('ScheduleCreated', { schedule }); } // in schedule-handler.ts async function handleScheduleCreated(event) { const schedule = event.schedule; // ... full validation ... const nextRunAt = getNextRunTime(schedule.cronExpression, ...); const updatedSchedule = updateSchedule(schedule, { nextRunAt }); // The handler is the ONLY component that saves the schedule await scheduleRepo.save(updatedSchedule); } Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a critical architectural flaw where business logic is duplicated in mcp-adapter.ts and schedule-handler.ts, leading to inefficiency and poor maintainability.	High
General	✅ Improve performance of schedule filtering Suggestion Impact: handleListSchedules no longer fetches all schedules for a status and slices in memory; it now delegates to scheduleService.listSchedules(status, limit, offset), which applies pagination in the underlying layer. code diff: - private async handleListSchedules(args: unknown): Promise<{ content: { type: string; text: string }[]; isError?: boolean }> { + private async handleListSchedules(args: unknown): Promise<MCPToolResponse> { const parseResult = ListSchedulesSchema.safeParse(args); if (!parseResult.success) { return { @@ -974,64 +953,49 @@ }; } - if (!this.scheduleRepository) { - return { - content: [{ type: 'text', text: 'Schedule repository not available' }], - isError: true, - }; - } - const { status, limit, offset } = parseResult.data; - let schedules: readonly Schedule[]; - if (status) { - const statusResult = await this.scheduleRepository.findByStatus(status as ScheduleStatus); - if (!statusResult.ok) { + const result = await this.scheduleService.listSchedules( + status as ScheduleStatus | undefined, + limit, + offset + ); + + return match(result, { + ok: (schedules) => { + const simplifiedSchedules = schedules.map(s => ({ + id: s.id, + status: s.status, + scheduleType: s.scheduleType, + cronExpression: s.cronExpression, + nextRunAt: s.nextRunAt ? new Date(s.nextRunAt).toISOString() : null, + runCount: s.runCount, + maxRuns: s.maxRuns, + })); + return { - content: [{ type: 'text', text: `Failed to list schedules: ${statusResult.error.message}` }], - isError: true, + content: [{ + type: 'text', + text: JSON.stringify({ + success: true, + schedules: simplifiedSchedules, + count: simplifiedSchedules.length, + }, null, 2), + }], }; - } - // Apply pagination manually for findByStatus - schedules = statusResult.value.slice(offset, offset + limit); - } else { - const result = await this.scheduleRepository.findAll(limit, offset); - if (!result.ok) { - return { - content: [{ type: 'text', text: `Failed to list schedules: ${result.error.message}` }], - isError: true, - }; - } - schedules = result.value; - } - - const simplifiedSchedules = schedules.map(s => ({ - id: s.id, - status: s.status, - scheduleType: s.scheduleType, - cronExpression: s.cronExpression, - nextRunAt: s.nextRunAt ? new Date(s.nextRunAt).toISOString() : null, - runCount: s.runCount, - maxRuns: s.maxRuns, - })); - - return { - content: [{ - type: 'text', - text: JSON.stringify({ - success: true, - schedules: simplifiedSchedules, - count: simplifiedSchedules.length, - }, null, 2), - }], - }; + }, + err: (error) => ({ + content: [{ type: 'text', text: JSON.stringify({ success: false, error: error.message }, null, 2) }], + isError: true, + }), + }); } Improve performance in handleListSchedules by applying pagination at the database level. Modify the findByStatus repository method to accept limit and offset to avoid fetching all matching schedules into memory. src/adapters/mcp-adapter.ts [986-1006] let schedules: readonly Schedule[]; -if (status) { - const statusResult = await this.scheduleRepository.findByStatus(status as ScheduleStatus); - if (!statusResult.ok) { - return { - content: [{ type: 'text', text: `Failed to list schedules: ${statusResult.error.message}` }], - isError: true, - }; - } - // Apply pagination manually for findByStatus - schedules = statusResult.value.slice(offset, offset + limit); -} else { - const result = await this.scheduleRepository.findAll(limit, offset); - if (!result.ok) { - return { - content: [{ type: 'text', text: `Failed to list schedules: ${result.error.message}` }], - isError: true, - }; - } - schedules = result.value; +const result = status + ? await this.scheduleRepository.findByStatus(status as ScheduleStatus, limit, offset) + : await this.scheduleRepository.findAll(limit, offset); + +if (!result.ok) { + return { + content: [{ type: 'text', text: `Failed to list schedules: ${result.error.message}` }], + isError: true, + }; } +schedules = result.value; [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a significant performance issue where pagination is applied in-memory instead of at the database level, and it proposes the correct architectural fix.	Medium
	✅ Ensure next run time is cleared Suggestion Impact: The update object construction was changed to always include `nextRunAt: newNextRunAt` instead of conditionally spreading it, ensuring nextRunAt is explicitly cleared when newNextRunAt is undefined (with added comments noting this prevents repeated retriggers). code diff: // Build update object immutably + // IMPORTANT: Always include nextRunAt to prevent infinite retrigger when getNextRunTime fails. + // If newNextRunAt is undefined (e.g., cron parse failure), this clears the old past nextRunAt + // so the schedule is not returned by findDue on every tick. const updates: Partial<Schedule> = { runCount: newRunCount, lastRunAt: triggeredAt, - ...(newNextRunAt !== undefined ? { nextRunAt: newNextRunAt } : {}), + nextRunAt: newNextRunAt, ...(newStatus !== undefined ? { status: newStatus } : {}), }; In handleScheduleTriggered, ensure the nextRunAt field is explicitly set to undefined when a schedule's status becomes terminal (e.g., COMPLETED or EXPIRED) to avoid misleading state. src/services/handlers/schedule-handler.ts [335-349] // Build update object immutably const updates: Partial<Schedule> = { runCount: newRunCount, lastRunAt: triggeredAt, - ...(newNextRunAt !== undefined ? { nextRunAt: newNextRunAt } : {}), + nextRunAt: newNextRunAt, ...(newStatus !== undefined ? { status: newStatus } : {}), }; // Persist updates const updateResult = await this.scheduleRepo.update(scheduleId, updates); [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a logic flaw where a schedule's nextRunAt is not cleared when it enters a terminal state, which could be misleading. The proposed fix ensures data consistency.	Low
Update

[dean0x]

Code Review: Scheduling Feature Implementation

Review Summary: 8 perspectives (Security, Architecture, Performance, Quality, TypeScript, Database, Dependencies, Documentation)
Date: 2026-02-18
Branch: task-2025-01-25_2210 → main
Recommendation: CHANGES_REQUESTED

---

Overall Assessment

This PR implements a comprehensive scheduling feature (cron and one-time schedules) with solid code quality overall: proper Zod validation at boundaries, immutable types, Result types consistently used, event-driven architecture alignment. However, multiple reviewers independently identified 4 critical architectural issues that must be resolved before merge.

Key Consensus Issues (found by 3+ reviewers):

1. Dual-save on schedule creation (architecture, performance, quality)
2. Unsafe type casts in ScheduleHandler (architecture, typescript, quality)
3. Silent enum defaults masking data corruption (security, database, typescript, quality)
4. Non-null assertion on undefined value (typescript, quality)
5. Read-modify-write TOCTOU race in update() (database, architecture, performance)
6. Infinite retrigger on cron parse failure (quality, with architecture + performance ripple effects)

---

BLOCKING Findings (Must Fix Before Merge)

CRITICAL / HIGH SEVERITY (9 total)

1. Dual-save on schedule creation — /Users/dean/Sandbox/claudine/src/adapters/mcp-adapter.ts:938 and /Users/dean/Sandbox/claudine/src/services/handlers/schedule-handler.ts:209-210

Status: BLOCKING | Consensus: 4 reviewers independently

• Problem: MCPAdapter saves schedule directly at line 938, then emits ScheduleCreated. ScheduleHandler receives event and saves again at line 210. Schedule is persisted twice per creation—once without nextRunAt, second time with it computed.
• Impact: Double database round-trips, inconsistent intermediate state, violates event-driven architecture pattern.
• Fix: Remove scheduleRepository.save() from MCPAdapter. Only emit ScheduleCreated and let handler be sole persistence owner, consistent with how DelegateTask → PersistenceHandler works.
• Priority: CRITICAL—architecture foundation for entire feature

---

2. Non-null assertion on potentially undefined scheduledAtMs — /Users/dean/Sandbox/claudine/src/adapters/mcp-adapter.ts:908

Status: BLOCKING | Consensus: TypeScript + Quality reviewers

• Problem: nextRunAt = scheduledAtMs!; uses non-null assertion that can fail at runtime. When scheduleType is 'one_time' and scheduledAt is validated, logically safe—but code structure doesn't let TypeScript prove it. Future refactoring could break this.
• Impact: Runtime undefined assigned where number is expected, causing Invalid Date at line 956.
• Fix: Restructure control flow for type narrowing:

  } else {
    if (scheduledAtMs === undefined) {
      return {
        content: [{ type: 'text', text: 'scheduledAt must be provided for one-time schedules' }],
        isError: true,
      };
    }
    nextRunAt = scheduledAtMs;
  }

---

3. Infinite retrigger when getNextRunTime fails — /Users/dean/Sandbox/claudine/src/services/handlers/schedule-handler.ts:299-306

Status: BLOCKING | Consensus: Quality + Architecture + Performance

• Problem: When getNextRunTime() fails (e.g., invalid cron), newNextRunAt remains undefined. Conditional spread ...(newNextRunAt !== undefined ? { nextRunAt: newNextRunAt } : {}) means nextRunAt is NOT updated. Old (already-past) nextRunAt persists, causing executor to re-trigger schedule on every 60-second tick indefinitely.
• Impact: Unbounded resource consumption. Each retrigger: reads due schedules, emits ScheduleTriggered, creates task, records execution, updates schedule, emits TaskDelegated/ScheduleExecuted. Repeats every minute forever.
• Fix: Explicitly handle error path:

  } else {
    const nextRunTimeResult = getNextRunTime(schedule);
    if (!nextRunTimeResult.ok) {
      // Pause schedule and explicitly clear nextRunAt
      newNextRunAt = undefined;
      newStatus = ScheduleStatus.PAUSED;
      const pauseEvent = new SchedulePausedEvent({
        scheduleId: schedule.id,
        reason: `Failed to compute next run time: ${nextRunTimeResult.error.message}`
      });
      // Emit pause event and include explicit nextRunAt: null in update
    } else {
      newNextRunAt = nextRunTimeResult.value;
    }
  }
  // Always include nextRunAt in update:
  const update: Partial<Schedule> = { 
    nextRunAt: newNextRunAt,
    // ... other fields
  };

---

4. Unsafe type casts in handleScheduleQuery — /Users/dean/Sandbox/claudine/src/services/handlers/schedule-handler.ts:503-555

Status: BLOCKING | Consensus: TypeScript + Architecture + Quality

• Problem: Multiple unsafe casts bypass type system:

  • (e as unknown as { __correlationId?: string }).__correlationId (lines 503, 514)
  • (this.eventBus as { respondError?: ... }).respondError?.(...) (lines 526-527, 539-540, 552-553)

  These access hypothetical internal properties not part of EventBus interface. If EventBus changes, casts silently break at runtime.

• Impact: Loss of type safety. Request-response pattern is untyped and forces casts throughout code.

• Fix: Extend EventBus interface properly:

  // In core/events/event-bus.ts:
  export interface EventBus {
    // ... existing methods ...
    respond?(correlationId: string, data: unknown): void;
    respondError?(correlationId: string, error: ClaudineError): void;
  }
  
  // Extend BaseEvent or create RequestEvent:
  export interface RequestEvent extends BaseEvent {
    __correlationId?: string;
  }
  
  // Then in handler, use typed event:
  const correlationId = (e as RequestEvent).__correlationId;
  this.eventBus.respond?.(correlationId, responseData);

---

5. Unsafe type assertions for enums — /Users/dean/Sandbox/claudine/src/adapters/mcp-adapter.ts:915,988

Status: BLOCKING | Severity: HIGH

• Problem:

  • priority: data.priority as Priority | undefined (line 915)
  • status as ScheduleStatus (line 988)

  Both cast Zod-validated strings to enums. If enum values ever diverge from string literals, casts silently break.

• Impact: Type safety violation. Invalid enum values silently pass through.

• Fix: Use explicit mapping:

  priority: data.priority ? Priority[data.priority as keyof typeof Priority] : undefined,

---

6. Read-Modify-Write TOCTOU race in update() — /Users/dean/Sandbox/claudine/src/implementations/schedule-repository.ts:214-237

Status: BLOCKING | Consensus: Database + Architecture + Performance (3 reviewers)

• Problem: update() does findById() then save() without transaction. Under concurrent execution, one update can silently overwrite the other. E.g., ScheduleHandler increments runCount while another handler updates status—one write loses the other's changes.
• Impact: Lost updates. This contradicts project's own CLAUDE.md requirement for synchronous transaction protection (documented for dependency management).
• Fix: Use targeted SQL UPDATE (superior to transaction-only approach):

  async update(id: ScheduleId, update: Partial<Schedule>): Promise<Result<void>> {
    return tryCatchAsync(
      async () => {
        const setClauses: string[] = [];
        const params: unknown[] = [];
  
        if (update.status !== undefined) {
          setClauses.push('status = ?');
          params.push(update.status);
        }
        if (update.nextRunAt !== undefined) {
          setClauses.push('next_run_at = ?');
          params.push(update.nextRunAt);
        }
        if (update.runCount !== undefined) {
          setClauses.push('run_count = ?');
          params.push(update.runCount);
        }
        // ... other fields as needed ...
        
        if (setClauses.length === 0) return; // No updates
        
        setClauses.push('updated_at = ?');
        params.push(Date.now());
        params.push(id);
  
        this.db.prepare(
          `UPDATE schedules SET ${setClauses.join(', ')} WHERE id = ?`
        ).run(...params);
      },
      operationErrorHandler('update schedule', { scheduleId: id })
    );
  }

  Benefit: Solves both TOCTOU race AND performance concern (eliminates read + double serialization).

---

7. Missing validatePath() on workingDirectory — /Users/dean/Sandbox/claudine/src/adapters/mcp-adapter.ts:913

Status: BLOCKING | Severity: HIGH (Security)

• Problem: handleScheduleTask passes data.workingDirectory directly into task template without validation. handleDelegateTask (line 521-537) correctly validates using validatePath(). Schedule skips validation entirely, enabling path traversal attacks.
• Impact: OWASP A01 (Broken Access Control). Scheduled tasks could execute in arbitrary filesystem locations.
• Fix: Apply same validation as DelegateTask:

  let validatedWorkingDirectory: string | undefined;
  if (data.workingDirectory) {
    const pathValidation = validatePath(data.workingDirectory);
    if (!pathValidation.ok) {
      return {
        content: [{ type: 'text', text: `Invalid working directory: ${pathValidation.error.message}` }],
        isError: true,
      };
    }
    validatedWorkingDirectory = pathValidation.value;
  }
  
  const schedule = createSchedule({
    taskTemplate: {
      prompt: data.prompt,
      priority: data.priority as Priority | undefined,
      workingDirectory: validatedWorkingDirectory,  // Use validated value
    },
    // ...
  });

---

8. Unsafe JSON deserialization of task_template — /Users/dean/Sandbox/claudine/src/implementations/schedule-repository.ts:402-405

Status: BLOCKING | Severity: HIGH (Security)

• Problem: JSON.parse() deserializes task_template from database, then bare as DelegateRequest type assertion without Zod validation. Inconsistent with "parse, don't validate" principle. Corrupted or tampered JSON could cause unexpected task parameters.
• Impact: OWASP A08 (Unsafe Deserialization).
• Fix: Add Zod schema validation:

  const DelegateRequestSchema = z.object({
    prompt: z.string().min(1).max(4000),
    priority: z.enum(['P0', 'P1', 'P2']).optional(),
    workingDirectory: z.string().optional(),
  });
  
  let taskTemplate: DelegateRequest;
  try {
    const parsed = JSON.parse(data.task_template);
    taskTemplate = DelegateRequestSchema.parse(parsed);
  } catch (e) {
    throw new Error(`Invalid task_template for schedule ${data.id}: ${e}`);
  }

---

9. Silent enum defaults masking data corruption — /Users/dean/Sandbox/claudine/src/implementations/schedule-repository.ts:449-479

Status: BLOCKING | Consensus: Security + Database + TypeScript + Quality (4 reviewers)

• Problem: toScheduleStatus() and toMissedRunPolicy() have default cases returning ACTIVE and SKIP for unknown values. Since Zod validates rows, reaching default indicates schema drift/corruption. Silently defaulting to ACTIVE re-activates a cancelled/expired schedule.
• Impact: Data corruption goes unnoticed. Defense-in-depth violation.
• Fix: Throw error on unknown values:

  private toScheduleStatus(value: string): ScheduleStatus {
    switch (value) {
      case 'active': return ScheduleStatus.ACTIVE;
      case 'paused': return ScheduleStatus.PAUSED;
      case 'completed': return ScheduleStatus.COMPLETED;
      case 'cancelled': return ScheduleStatus.CANCELLED;
      case 'expired': return ScheduleStatus.EXPIRED;
      default:
        throw new Error(`Unknown schedule status: ${value} - possible data corruption`);
    }
  }

---

SHOULD-FIX Findings (Strongly Recommended)

HIGH SEVERITY (5 total)

S1. ScheduleExecutor not stopped during graceful shutdown — /Users/dean/Sandbox/claudine/src/index.ts:75-94 and /Users/dean/Sandbox/claudine/src/core/container.ts:202-240

• Problem: ScheduleExecutor.start() called in bootstrap but neither shutdown() nor container.dispose() calls stop(). Timer is .unref()'d so won't block exit, but during graceful shutdown ticks can fire against closed resources.
• Fix: Add scheduleExecutor.stop() to container.dispose() before killing workers.

---

S2. Event subscription leak in ScheduleExecutor — /Users/dean/Sandbox/claudine/src/services/schedule-executor.ts:82-113

• Problem: subscribeToTaskEvents() subscribes to 5 events in constructor but stop() (line 187) only clears timer. Subscriptions persist and accumulate on repeated start/stop.
• Impact: Resource leak. Repeated stop/start cycles hit EventBus maxListenersPerEvent limit.
• Fix: Store subscription IDs and clean up in stop():

  private subscriptionIds: string[] = [];
  
  private subscribeToTaskEvents(): void {
    const sub1 = this.eventBus.subscribe('ScheduleExecuted', ...);
    if (sub1.ok) this.subscriptionIds.push(sub1.value);
    // ... etc
  }
  
  stop(): Result<void, ClaudineError> {
    // ... timer cleanup ...
    for (const id of this.subscriptionIds) {
      this.eventBus.unsubscribe(id);
    }
    this.subscriptionIds = [];
    // ...
  }

---

S3. Dead event subscriptions in ScheduleHandler — /Users/dean/Sandbox/claudine/src/services/handlers/schedule-handler.ts:117-118,572-592

• Problem: ScheduleHandler subscribes to TaskCompleted and TaskFailed for ALL tasks but handlers are no-ops (just log). Combined with ScheduleExecutor also subscribing to same events, each task completion fires 4+ redundant handlers.
• Fix: Remove no-op subscriptions until feature is actually implemented (YAGNI). ScheduleExecutor already handles task tracking.

---

S4. Missing ScheduleUpdate type enforcement — /Users/dean/Sandbox/claudine/src/core/domain.ts:302 vs /Users/dean/Sandbox/claudine/src/core/interfaces.ts:250

• Problem: ScheduleUpdate type (domain.ts:302) correctly restricts updatable fields, but ScheduleRepository.update() uses Partial<Schedule> (interfaces.ts:250), allowing id, createdAt, taskTemplate mutations.
• Fix: Use ScheduleUpdate consistently in repository interface and ScheduleUpdatedEvent.

---

S5. findByStatus() has no result limit — /Users/dean/Sandbox/claudine/src/implementations/schedule-repository.ts:136-138

• Problem: findByStatusStmt has no LIMIT clause. Thousands of completed/cancelled schedules fetch into memory at once, then .slice() throws away most.
• Fix: Add LIMIT/OFFSET pagination:

  SELECT * FROM schedules WHERE status = ? ORDER BY created_at DESC LIMIT ? OFFSET ?

---

S6. MCPAdapter optional parameters should be required — /Users/dean/Sandbox/claudine/src/adapters/mcp-adapter.ts:108-109

• Problem: scheduleRepository?: ScheduleRepository and eventBus?: EventBus are optional. Every handler performs runtime null checks. Bootstrap always provides both. Optionality hides wiring bugs.
• Fix: Make both required. If scheduling is optional as feature, gate it at bootstrap level, not inside adapter.

---

MEDIUM SEVERITY (5 total)

S7. Unsafe as Priority type assertion — /Users/dean/Sandbox/claudine/src/adapters/mcp-adapter.ts:915

• Problem: Zod validates string enum, then cast to Priority enum bypasses further checks.
• Fix: Use explicit mapping or validated lookup.

---

S8. CancelSchedule doesn't validate status — /Users/dean/Sandbox/claudine/src/adapters/mcp-adapter.ts:1048-1090

• Problem: Can cancel completed/cancelled schedules. Inconsistent with Pause/Resume validation.
• Fix: Check status before emitting cancel event.

---

S9. Sequential schedule processing could be parallelized — /Users/dean/Sandbox/claudine/src/services/schedule-executor.ts:243-245

• Problem: Due schedules processed with for...await sequentially. Independent schedules could run in parallel with Promise.allSettled.
• Note: Architecture reviewer argues sequential prevents races; pragmatically sound but debatable. Consider this an optimization, not critical.

---

S10. Concurrent execution prevention not persisted — /Users/dean/Sandbox/claudine/src/services/schedule-executor.ts:58

• Problem: runningSchedules Map is in-memory. Process restart loses running state, enabling duplicate executions.
• Fix: At minimum, document limitation. Ideally, check execution history on startup.

---

S11. High cyclomatic complexity in handleScheduleTask — /Users/dean/Sandbox/claudine/src/adapters/mcp-adapter.ts:839-935

• Problem: ~100 lines, 9 error paths. Duplicates cron/timezone validation that handler also does.
• Fix: Adapter does Zod boundary validation only; delegate business validation to handler.

---

PRE-EXISTING Issues (Not Blocking)

These were found by reviewers but already exist in the codebase:

• Security PE1: MCP tool handler uses z.any() for arguments (permissive outer schema)
• Security PE2: Error messages may leak internal details (file paths, database errors)
• Dependencies: cron-parser v4.9.0 is one major version behind (v5.5.0 latest). Consider upgrade or evaluate zero-dependency alternatives like croner.
• Dependencies: Outdated major versions of @modelcontextprotocol/sdk have known CVEs (pre-existing)
• Documentation: README testing section contradicts CLAUDE.md about npm test (pre-existing)

---

Documentation (Critical Gaps)

The implementation code quality is solid, but documentation layer not updated:

1. FEATURES.md still claims "Scheduled Tasks: No cron-like scheduling" (actively contradicts implemented feature)
2. CLAUDE.md missing scheduling handlers, tools, file locations
3. README.md missing all 6 scheduling tools from MCP table
4. ROADMAP.md shows scheduling as "Research" (already implemented)
5. No dedicated docs/SCHEDULING.md (equivalent to existing TASK-DEPENDENCIES.md)
6. No release notes for this feature

These must be updated for release. See documentation review report for detailed fixes.

---

Summary Table

Severity	Blocking	Should-Fix	Pre-existing
CRITICAL	1	0	0
HIGH	8	5	3
MEDIUM	0	5	5
LOW	0	1	5

Total: 9 BLOCKING findings (must resolve before merge)

---

Merge Recommendation

Status: ❌ CHANGES_REQUESTED

Blocking Issues: 9 (all HIGH or CRITICAL severity)

Key Priority Fixes (in order):

1. Remove dual-save on schedule creation (CRITICAL architectural)
2. Fix infinite retrigger on cron parse failure (causes unbounded resource consumption)
3. Add path validation on workingDirectory (security)
4. Replace unsafe type casts with proper EventBus interface extension
5. Add transaction/targeted UPDATE to prevent TOCTOU race
6. Add JSON schema validation for task_template deserialization
7. Fix silent enum defaults to throw on corruption
8. Remove non-null assertion, restructure for type safety
9. Update all documentation files

All 9 must be resolved before this PR is merge-ready.

---

Review compiled by Claude Code reviewing scheduling feature implementation across 8 perspectives. Consensus findings (3+ reviewers) marked with indicator.