diff --git a/src/__tests__/claude-client-status.test.ts b/src/__tests__/claude-client-status.test.ts new file mode 100644 index 0000000..d6713df --- /dev/null +++ b/src/__tests__/claude-client-status.test.ts @@ -0,0 +1,71 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const { mockExecuteClaudeCli } = vi.hoisted(() => ({ + mockExecuteClaudeCli: vi.fn(), +})); + +vi.mock('../infra/claude/process.js', () => ({ + executeClaudeCli: mockExecuteClaudeCli, +})); + +vi.mock('../shared/utils/index.js', () => ({ + createLogger: vi.fn(() => ({ + error: vi.fn(), + warn: vi.fn(), + info: vi.fn(), + debug: vi.fn(), + })), +})); + +vi.mock('../shared/prompts/index.js', () => ({ + loadTemplate: vi.fn(() => 'system prompt'), +})); + +import { ClaudeClient } from '../infra/claude/client.js'; +import type { ClaudeCallOptions } from '../infra/claude/client.js'; + +describe('ClaudeClient status normalization', () => { + const options: ClaudeCallOptions = { + cwd: '/tmp/takt-test', + }; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should return error status when call() receives an interrupted failure', async () => { + mockExecuteClaudeCli.mockResolvedValue({ + success: false, + interrupted: true, + content: 'Interrupted by signal', + error: 'SIGINT', + sessionId: 'session-1', + }); + + const client = new ClaudeClient(); + + const response = await client.call('coder', 'Implement feature', options); + + expect(response.status).toBe('error'); + expect(response.error).toBe('SIGINT'); + expect(response.content).toBe('Interrupted by signal'); + }); + + it('should return error status when callCustom() receives an interrupted failure', async () => { + mockExecuteClaudeCli.mockResolvedValue({ + success: false, + interrupted: true, + content: 'Interrupted by signal', + error: 'SIGINT', + sessionId: 'session-2', + }); + + const client = new ClaudeClient(); + + const response = await client.callCustom('custom-coder', 'Implement feature', 'system prompt', options); + + expect(response.status).toBe('error'); + expect(response.error).toBe('SIGINT'); + expect(response.content).toBe('Interrupted by signal'); + }); +}); diff --git a/src/__tests__/engine-error.test.ts b/src/__tests__/engine-error.test.ts index 676c207..bc7c0cf 100644 --- a/src/__tests__/engine-error.test.ts +++ b/src/__tests__/engine-error.test.ts @@ -155,8 +155,8 @@ describe('PieceEngine Integration: Error Handling', () => { // ===================================================== // 3. Interrupted status routing // ===================================================== - describe('Interrupted status', () => { - it('should continue with normal rule routing and skip report phase when movement returns interrupted', async () => { + describe('Error status', () => { + it('should abort immediately and skip report phase when movement returns error', async () => { const config = buildDefaultPieceConfig({ initialMovement: 'plan', movements: [ @@ -169,11 +169,12 @@ describe('PieceEngine Integration: Error Handling', () => { const engine = new PieceEngine(config, tmpDir, 'test task', { projectCwd: tmpDir }); mockRunAgentSequence([ - makeResponse({ persona: 'plan', status: 'interrupted', content: 'Partial response' }), - ]); - - mockDetectMatchedRuleSequence([ - { index: 0, method: 'phase1_tag' }, + makeResponse({ + persona: 'plan', + status: 'error', + content: 'Partial response', + error: 'interrupted by signal', + }), ]); const abortFn = vi.fn(); @@ -181,10 +182,107 @@ describe('PieceEngine Integration: Error Handling', () => { const state = await engine.run(); - expect(state.status).toBe('completed'); - expect(abortFn).not.toHaveBeenCalled(); + expect(state.status).toBe('aborted'); + expect(abortFn).toHaveBeenCalledOnce(); expect(runReportPhase).not.toHaveBeenCalled(); }); + + it('should abort when movement returns an unhandled status and skip report phase', async () => { + const config = buildDefaultPieceConfig({ + initialMovement: 'plan', + movements: [ + makeMovement('plan', { + outputContracts: [{ name: '01-plan.md', format: '# Plan' }], + rules: [makeRule('continue', 'COMPLETE')], + }), + ], + }); + const engine = new PieceEngine(config, tmpDir, 'test task', { projectCwd: tmpDir }); + + mockRunAgentSequence([ + makeResponse({ + persona: 'plan', + status: 'pending' as never, + content: 'pending response', + }), + ]); + + const abortFn = vi.fn(); + engine.on('piece:abort', abortFn); + + const state = await engine.run(); + + expect(state.status).toBe('aborted'); + expect(abortFn).toHaveBeenCalledOnce(); + const reason = abortFn.mock.calls[0]![1] as string; + expect(reason).toContain('Unhandled response status: pending'); + expect(runReportPhase).not.toHaveBeenCalled(); + }); + }); + + describe('runSingleIteration status routing', () => { + it('should abort without rule resolution when movement returns blocked', async () => { + const config = buildDefaultPieceConfig({ + initialMovement: 'plan', + movements: [ + makeMovement('plan', { + rules: [makeRule('continue', 'COMPLETE')], + }), + ], + }); + const engine = new PieceEngine(config, tmpDir, 'test task', { projectCwd: tmpDir }); + + mockRunAgentSequence([ + makeResponse({ + persona: 'plan', + status: 'blocked', + content: 'need input', + }), + ]); + + const abortFn = vi.fn(); + engine.on('piece:abort', abortFn); + + const result = await engine.runSingleIteration(); + + expect(result.nextMovement).toBe('ABORT'); + expect(result.isComplete).toBe(true); + expect(engine.getState().status).toBe('aborted'); + expect(abortFn).toHaveBeenCalledOnce(); + }); + + it('should abort without rule resolution when movement returns error', async () => { + const config = buildDefaultPieceConfig({ + initialMovement: 'plan', + movements: [ + makeMovement('plan', { + rules: [makeRule('continue', 'COMPLETE')], + }), + ], + }); + const engine = new PieceEngine(config, tmpDir, 'test task', { projectCwd: tmpDir }); + + mockRunAgentSequence([ + makeResponse({ + persona: 'plan', + status: 'error', + content: 'failed', + error: 'request failed', + }), + ]); + + const abortFn = vi.fn(); + engine.on('piece:abort', abortFn); + + const result = await engine.runSingleIteration(); + + expect(result.nextMovement).toBe('ABORT'); + expect(result.isComplete).toBe(true); + expect(engine.getState().status).toBe('aborted'); + expect(abortFn).toHaveBeenCalledOnce(); + const reason = abortFn.mock.calls[0]![1] as string; + expect(reason).toContain('Movement "plan" failed: request failed'); + }); }); // ===================================================== diff --git a/src/__tests__/engine-loop-monitors.test.ts b/src/__tests__/engine-loop-monitors.test.ts index 31aff5d..2044a0b 100644 --- a/src/__tests__/engine-loop-monitors.test.ts +++ b/src/__tests__/engine-loop-monitors.test.ts @@ -39,6 +39,7 @@ vi.mock('../shared/utils/index.js', async (importOriginal) => ({ import { PieceEngine } from '../core/piece/index.js'; import { runAgent } from '../agents/runner.js'; +import { runReportPhase } from '../core/piece/phase-runner.js'; import { makeResponse, makeMovement, @@ -208,6 +209,40 @@ describe('PieceEngine Integration: Loop Monitors', () => { // 8 iterations: impl + ai_review*3 + ai_fix*2 + judge + reviewers expect(state.iteration).toBe(8); }); + + it('should abort when judge returns non-done status', async () => { + const config = buildConfigWithLoopMonitor(1); + engine = new PieceEngine(config, tmpDir, 'test task', { projectCwd: tmpDir }); + + mockRunAgentSequence([ + makeResponse({ persona: 'implement', content: 'Implementation done' }), + makeResponse({ persona: 'ai_review', content: 'Issues found: X' }), + makeResponse({ persona: 'ai_fix', content: 'Fixed X' }), + makeResponse({ + persona: 'supervisor', + status: 'error', + content: 'judge failed', + error: 'judge interrupted', + }), + ]); + + mockDetectMatchedRuleSequence([ + { index: 0, method: 'phase1_tag' }, + { index: 1, method: 'phase1_tag' }, + { index: 0, method: 'phase1_tag' }, + ]); + + const abortFn = vi.fn(); + engine.on('piece:abort', abortFn); + + const state = await engine.run(); + + expect(state.status).toBe('aborted'); + expect(abortFn).toHaveBeenCalledOnce(); + const reason = abortFn.mock.calls[0]![1] as string; + expect(reason).toContain('Unhandled response status: error'); + expect(runReportPhase).not.toHaveBeenCalled(); + }); }); // ===================================================== diff --git a/src/__tests__/it-mock-scenario.test.ts b/src/__tests__/it-mock-scenario.test.ts index 40c067b..3bce2ae 100644 --- a/src/__tests__/it-mock-scenario.test.ts +++ b/src/__tests__/it-mock-scenario.test.ts @@ -14,6 +14,7 @@ import { resetScenario, type ScenarioEntry, } from '../infra/mock/index.js'; +import { STATUS_VALUES } from '../core/models/status.js'; describe('ScenarioQueue', () => { it('should consume entries in order when no agent specified', () => { @@ -130,6 +131,16 @@ describe('loadScenarioFile', () => { expect(entries[1]).toEqual({ persona: undefined, status: 'blocked', content: 'Blocked' }); }); + it('should accept all statuses from shared status contract', () => { + const scenario = STATUS_VALUES.map((status, i) => ({ status, content: `entry-${i}` })); + const filePath = join(tempDir, 'all-statuses.json'); + writeFileSync(filePath, JSON.stringify(scenario)); + + const entries = loadScenarioFile(filePath); + + expect(entries.map((entry) => entry.status)).toEqual([...STATUS_VALUES]); + }); + it('should default status to "done" if omitted', () => { const scenario = [{ content: 'Simple response' }]; const filePath = join(tempDir, 'scenario.json'); @@ -167,7 +178,21 @@ describe('loadScenarioFile', () => { it('should throw for invalid status', () => { const filePath = join(tempDir, 'bad-status.json'); - writeFileSync(filePath, '[{"content": "test", "status": "invalid"}]'); + writeFileSync(filePath, '[{"content": "test", "status": "approved"}]'); + + expect(() => loadScenarioFile(filePath)).toThrow('invalid status'); + }); + + it('should throw for rejected status', () => { + const filePath = join(tempDir, 'rejected-status.json'); + writeFileSync(filePath, '[{"content": "test", "status": "rejected"}]'); + + expect(() => loadScenarioFile(filePath)).toThrow('invalid status'); + }); + + it('should throw for improve status', () => { + const filePath = join(tempDir, 'improve-status.json'); + writeFileSync(filePath, '[{"content": "test", "status": "improve"}]'); expect(() => loadScenarioFile(filePath)).toThrow('invalid status'); }); diff --git a/src/__tests__/models.test.ts b/src/__tests__/models.test.ts index a2b8cc9..66ce034 100644 --- a/src/__tests__/models.test.ts +++ b/src/__tests__/models.test.ts @@ -14,6 +14,7 @@ import { GlobalConfigSchema, ProjectConfigSchema, } from '../core/models/index.js'; +import { STATUS_VALUES } from '../core/models/status.js'; describe('AgentTypeSchema', () => { it('should accept valid agent types', () => { @@ -30,18 +31,25 @@ describe('AgentTypeSchema', () => { describe('StatusSchema', () => { it('should accept valid statuses', () => { - expect(StatusSchema.parse('pending')).toBe('pending'); expect(StatusSchema.parse('done')).toBe('done'); - expect(StatusSchema.parse('approved')).toBe('approved'); - expect(StatusSchema.parse('rejected')).toBe('rejected'); expect(StatusSchema.parse('blocked')).toBe('blocked'); expect(StatusSchema.parse('error')).toBe('error'); - expect(StatusSchema.parse('answer')).toBe('answer'); + }); + + it('should align with the shared status contract values', () => { + expect(StatusSchema.options).toEqual([...STATUS_VALUES]); }); it('should reject invalid statuses', () => { expect(() => StatusSchema.parse('unknown')).toThrow(); expect(() => StatusSchema.parse('conditional')).toThrow(); + expect(() => StatusSchema.parse('pending')).toThrow(); + expect(() => StatusSchema.parse('approved')).toThrow(); + expect(() => StatusSchema.parse('rejected')).toThrow(); + expect(() => StatusSchema.parse('improve')).toThrow(); + expect(() => StatusSchema.parse('cancelled')).toThrow(); + expect(() => StatusSchema.parse('interrupted')).toThrow(); + expect(() => StatusSchema.parse('answer')).toThrow(); }); }); diff --git a/src/core/models/schemas.ts b/src/core/models/schemas.ts index aef04da..22d4802 100644 --- a/src/core/models/schemas.ts +++ b/src/core/models/schemas.ts @@ -8,6 +8,7 @@ import { z } from 'zod/v4'; import { DEFAULT_LANGUAGE } from '../../shared/constants.js'; import { McpServersSchema } from './mcp-schemas.js'; import { INTERACTIVE_MODES } from './interactive-mode.js'; +import { STATUS_VALUES } from './status.js'; export { McpServerConfigSchema, McpServersSchema } from './mcp-schemas.js'; @@ -44,18 +45,7 @@ export const TaktConfigSchema = z.object({ export const AgentTypeSchema = z.enum(['coder', 'architect', 'supervisor', 'custom']); /** Status schema */ -export const StatusSchema = z.enum([ - 'pending', - 'done', - 'blocked', - 'error', - 'approved', - 'rejected', - 'improve', - 'cancelled', - 'interrupted', - 'answer', -]); +export const StatusSchema = z.enum(STATUS_VALUES); /** Permission mode schema for tool execution */ export const PermissionModeSchema = z.enum(['readonly', 'edit', 'full']); diff --git a/src/core/models/session.ts b/src/core/models/session.ts index d503e5f..dc44a8d 100644 --- a/src/core/models/session.ts +++ b/src/core/models/session.ts @@ -5,6 +5,8 @@ import type { AgentResponse } from './response.js'; import type { Status } from './status.js'; +type SessionAgentStatus = 'pending' | Status; + /** * Session state for piece execution */ @@ -13,9 +15,9 @@ export interface SessionState { projectDir: string; iteration: number; maxMovements: number; - coderStatus: Status; - architectStatus: Status; - supervisorStatus: Status; + coderStatus: SessionAgentStatus; + architectStatus: SessionAgentStatus; + supervisorStatus: SessionAgentStatus; history: AgentResponse[]; context: string; } diff --git a/src/core/models/status.ts b/src/core/models/status.ts index 0dde9f7..9da61a4 100644 --- a/src/core/models/status.ts +++ b/src/core/models/status.ts @@ -6,17 +6,10 @@ export type AgentType = 'coder' | 'architect' | 'supervisor' | 'custom'; /** Execution status for agents and pieces */ -export type Status = - | 'pending' - | 'done' - | 'blocked' - | 'error' - | 'approved' - | 'rejected' - | 'improve' - | 'cancelled' - | 'interrupted' - | 'answer'; +export const STATUS_VALUES = ['done', 'blocked', 'error'] as const; + +/** Execution status for agents and pieces */ +export type Status = typeof STATUS_VALUES[number]; /** How a rule match was detected */ export type RuleMatchMethod = diff --git a/src/core/piece/engine/PieceEngine.ts b/src/core/piece/engine/PieceEngine.ts index b963e18..5c11b00 100644 --- a/src/core/piece/engine/PieceEngine.ts +++ b/src/core/piece/engine/PieceEngine.ts @@ -446,6 +446,14 @@ export class PieceEngine extends EventEmitter { throw new Error(`No matching rule found for movement "${step.name}" (status: ${response.status})`); } + private resolveNextMovementFromDone(step: PieceMovement, response: AgentResponse): string { + if (response.status !== 'done') { + throw new Error(`Unhandled response status: ${response.status}`); + } + + return this.resolveNextMovement(step, response); + } + /** Build instruction (public, used by pieceExecution.ts for logging) */ buildInstruction(step: PieceMovement, movementIteration: number): string { return this.movementExecutor.buildInstruction( @@ -557,7 +565,7 @@ export class PieceEngine extends EventEmitter { this.emit('movement:complete', judgeMovement, response, instruction); // Resolve next movement from the judge's rules - const nextMovement = this.resolveNextMovement(judgeMovement, response); + const nextMovement = this.resolveNextMovementFromDone(judgeMovement, response); log.info('Loop monitor judge decision', { cycle: monitor.cycle, @@ -658,7 +666,7 @@ export class PieceEngine extends EventEmitter { break; } - let nextMovement = this.resolveNextMovement(movement, response); + let nextMovement = this.resolveNextMovementFromDone(movement, response); log.debug('Movement transition', { from: movement.name, status: response.status, @@ -764,7 +772,21 @@ export class PieceEngine extends EventEmitter { this.state.iteration++; const { response } = await this.runMovement(movement); - const nextMovement = this.resolveNextMovement(movement, response); + + if (response.status === 'blocked') { + this.state.status = 'aborted'; + this.emit('piece:abort', this.state, 'Piece blocked and no user input provided'); + return { response, nextMovement: ABORT_MOVEMENT, isComplete: true, loopDetected: loopCheck.isLoop }; + } + + if (response.status === 'error') { + const detail = response.error ?? response.content; + this.state.status = 'aborted'; + this.emit('piece:abort', this.state, `Movement "${movement.name}" failed: ${detail}`); + return { response, nextMovement: ABORT_MOVEMENT, isComplete: true, loopDetected: loopCheck.isLoop }; + } + + const nextMovement = this.resolveNextMovementFromDone(movement, response); const isComplete = nextMovement === COMPLETE_MOVEMENT || nextMovement === ABORT_MOVEMENT; if (response.matchedRuleIndex != null && movement.rules) { diff --git a/src/infra/claude/client.ts b/src/infra/claude/client.ts index 2ca5057..295c19f 100644 --- a/src/infra/claude/client.ts +++ b/src/infra/claude/client.ts @@ -25,9 +25,6 @@ export class ClaudeClient { result: { success: boolean; interrupted?: boolean; content: string; fullContent?: string }, ): Status { if (!result.success) { - if (result.interrupted) { - return 'interrupted'; - } return 'error'; } return 'done'; diff --git a/src/infra/claude/process.ts b/src/infra/claude/process.ts index 75e6e2c..781a66e 100644 --- a/src/infra/claude/process.ts +++ b/src/infra/claude/process.ts @@ -64,7 +64,6 @@ export async function executeClaudeCli( export class ClaudeProcess { private options: ClaudeSpawnOptions; private currentSessionId?: string; - private interrupted = false; constructor(options: ClaudeSpawnOptions) { this.options = options; @@ -72,18 +71,13 @@ export class ClaudeProcess { /** Execute a prompt */ async execute(prompt: string): Promise { - this.interrupted = false; const result = await executeClaudeCli(prompt, this.options); this.currentSessionId = result.sessionId; - if (result.interrupted) { - this.interrupted = true; - } return result; } /** Interrupt the running query */ kill(): void { - this.interrupted = true; interruptCurrentProcess(); } @@ -96,9 +90,4 @@ export class ClaudeProcess { getSessionId(): string | undefined { return this.currentSessionId; } - - /** Check if query was interrupted */ - wasInterrupted(): boolean { - return this.interrupted; - } } diff --git a/src/infra/mock/scenario.ts b/src/infra/mock/scenario.ts index 7ffc0ff..b946b37 100644 --- a/src/infra/mock/scenario.ts +++ b/src/infra/mock/scenario.ts @@ -8,6 +8,7 @@ import { readFileSync, existsSync } from 'node:fs'; import type { ScenarioEntry } from './types.js'; +import { STATUS_VALUES } from '../../core/models/status.js'; export type { ScenarioEntry }; @@ -130,11 +131,10 @@ function validateEntry(entry: unknown, index: number): ScenarioEntry { } // status defaults to 'done' - const validStatuses = ['done', 'blocked', 'error', 'approved', 'rejected', 'improve'] as const; const status = obj.status ?? 'done'; - if (typeof status !== 'string' || !validStatuses.includes(status as typeof validStatuses[number])) { + if (typeof status !== 'string' || !STATUS_VALUES.includes(status as typeof STATUS_VALUES[number])) { throw new Error( - `Scenario entry [${index}] has invalid status "${String(status)}". Valid: ${validStatuses.join(', ')}`, + `Scenario entry [${index}] has invalid status "${String(status)}". Valid: ${STATUS_VALUES.join(', ')}`, ); } diff --git a/src/infra/mock/types.ts b/src/infra/mock/types.ts index 5bb5dae..a36b642 100644 --- a/src/infra/mock/types.ts +++ b/src/infra/mock/types.ts @@ -3,6 +3,7 @@ */ import type { StreamCallback } from '../claude/index.js'; +import type { Status } from '../../core/models/status.js'; /** Options for mock calls */ export interface MockCallOptions { @@ -12,7 +13,7 @@ export interface MockCallOptions { /** Fixed response content (optional, defaults to generic mock response) */ mockResponse?: string; /** Fixed status to return (optional, defaults to 'done') */ - mockStatus?: 'done' | 'blocked' | 'error' | 'approved' | 'rejected' | 'improve'; + mockStatus?: Status; /** Structured output payload returned as-is */ structuredOutput?: Record; } @@ -22,7 +23,7 @@ export interface ScenarioEntry { /** Persona name to match (optional — if omitted, consumed by call order) */ persona?: string; /** Response status */ - status: 'done' | 'blocked' | 'error' | 'approved' | 'rejected' | 'improve'; + status: Status; /** Response content body */ content: string; /** Optional structured output payload (for outputSchema-driven flows) */