diff --git a/src/message-runtime-follow-up.test.ts b/src/message-runtime-follow-up.test.ts index 4bf4dfc..f8c3c76 100644 --- a/src/message-runtime-follow-up.test.ts +++ b/src/message-runtime-follow-up.test.ts @@ -83,7 +83,7 @@ describe('message-runtime-follow-up', () => { expect(enqueue).toHaveBeenCalledTimes(1); }); - it('uses the fallback STEP_DONE verdict to schedule owner follow-ups when the owner keeps the task active', () => { + it('does not use fallback owner STEP_DONE verdict to schedule owner follow-ups while active', () => { const enqueue = vi.fn(); const result = dispatchPairedFollowUpForEvent({ chatJid: 'group@test', @@ -102,14 +102,12 @@ describe('message-runtime-follow-up', () => { }); expect(result).toMatchObject({ - kind: 'paired-follow-up', - intentKind: 'owner-follow-up', - scheduled: true, + kind: 'none', taskStatus: 'active', lastTurnOutputRole: 'owner', lastTurnOutputVerdict: 'step_done', }); - expect(enqueue).toHaveBeenCalledTimes(1); + expect(enqueue).not.toHaveBeenCalled(); }); it('prefers the latest persisted turn output over the fallback delivery role when both are present', () => { diff --git a/src/message-runtime-rules.test.ts b/src/message-runtime-rules.test.ts index db9b4bd..d8eba00 100644 --- a/src/message-runtime-rules.test.ts +++ b/src/message-runtime-rules.test.ts @@ -120,7 +120,7 @@ describe('message-runtime-rules', () => { lastTurnOutputVerdict: 'step_done', intentKind: 'owner-follow-up', }), - ).toBe(true); + ).toBe(false); }); it('maps active tasks with reviewer output to an owner follow-up', () => { @@ -132,14 +132,14 @@ describe('message-runtime-rules', () => { ).toEqual({ kind: 'owner-follow-up' }); }); - it('maps active tasks with owner STEP_DONE output to an owner follow-up', () => { + it('does not map active owner STEP_DONE output to an owner follow-up', () => { expect( resolveNextTurnAction({ taskStatus: 'active', lastTurnOutputRole: 'owner', lastTurnOutputVerdict: 'step_done', }), - ).toEqual({ kind: 'owner-follow-up' }); + ).toEqual({ kind: 'none' }); }); it('returns none when an active task has no reviewer or arbiter handoff output', () => { @@ -329,7 +329,7 @@ describe('message-runtime-rules', () => { lastTurnOutputRole: 'owner', lastTurnOutputVerdict: 'step_done', }), - ).toBe('owner'); + ).toBeNull(); }); it('resolves reviewer execution target from review_ready task status', () => { diff --git a/src/message-runtime-rules.ts b/src/message-runtime-rules.ts index 033abb1..c7582e2 100644 --- a/src/message-runtime-rules.ts +++ b/src/message-runtime-rules.ts @@ -146,8 +146,7 @@ export function resolveNextTurnAction(args: { ? { kind: 'none' } : { kind: 'finalize-owner-turn' }; case 'active': - return args.lastTurnOutputVerdict === 'step_done' || - args.lastTurnOutputRole === 'reviewer' || + return args.lastTurnOutputRole === 'reviewer' || args.lastTurnOutputRole === 'arbiter' ? { kind: 'owner-follow-up' } : { kind: 'none' }; diff --git a/src/message-runtime.test.ts b/src/message-runtime.test.ts index 1af391c..6bf1b59 100644 --- a/src/message-runtime.test.ts +++ b/src/message-runtime.test.ts @@ -2126,7 +2126,7 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead expect(pending?.prompt).toContain('이전 reviewer 피드백'); }); - it('builds an owner follow-up pending turn when the latest owner output is STEP_DONE on an active task', () => { + it('does not build an owner follow-up pending turn from owner STEP_DONE on an active task', () => { const chatJid = 'group@test'; const group = makeGroup('claude-code'); const task = { @@ -2170,12 +2170,7 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead resolveChannel: () => makeChannel(chatJid), }); - expect(pending).not.toBeNull(); - expect(pending).toMatchObject({ - role: 'owner', - intentKind: 'owner-follow-up', - taskId: task.id, - }); + expect(pending).toBeNull(); }); it('builds a reviewer pending turn when a review_ready task follows owner TASK_DONE output', () => { @@ -2335,7 +2330,7 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead ); }); - it('re-enqueues owner follow-up after a successful owner STEP_DONE delivery keeps the task active', async () => { + it('does not re-enqueue owner follow-up after a successful owner STEP_DONE delivery keeps the task active', async () => { const chatJid = 'group@test'; const group = makeGroup('codex'); const ownerChannel: Channel = { @@ -2445,17 +2440,12 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead chatJid, 'STEP_DONE\n리팩토링 1단계 완료, 다음 단계 진행', ); - expect(enqueueMessageCheck).toHaveBeenCalledWith(chatJid); + expect(enqueueMessageCheck).not.toHaveBeenCalled(); expect(pairedTask.status).toBe('active'); expect(pairedTask.round_trip_count).toBe(1); expect(pairedTask.owner_failure_count).toBe(0); - expect(logger.info).toHaveBeenCalledWith( + expect(logger.info).not.toHaveBeenCalledWith( expect.objectContaining({ - chatJid, - runId: 'run-owner-step-done-follow-up', - completedRole: 'owner', - taskId: 'task-owner-step-done-follow-up', - taskStatus: 'active', intentKind: 'owner-follow-up', }), 'Queued paired follow-up after successful owner delivery', diff --git a/src/paired-execution-context-owner.ts b/src/paired-execution-context-owner.ts index f820007..beb958a 100644 --- a/src/paired-execution-context-owner.ts +++ b/src/paired-execution-context-owner.ts @@ -20,10 +20,9 @@ import { } from './paired-execution-context-shared.js'; import type { PairedTask } from './types.js'; -type OwnerFinalizeOutcome = 'stop' | 're_review' | 'continue_owner'; +type OwnerFinalizeOutcome = 'stop' | 're_review'; const OWNER_FAILURE_ESCALATION_THRESHOLD = 2; const EMPTY_STEP_DONE_THRESHOLD = 2; -const OWNER_STEP_DONE_LOOP_THRESHOLD = 3; export function handleFailedOwnerExecution(args: { task: PairedTask; @@ -157,7 +156,7 @@ function handleOwnerFinalizeCompletion(args: { } if ( - signal.kind === 'request_owner_continue' && + ownerVerdict === 'step_done' && hasNewChanges === false && nextEmptyStepDoneStreak >= EMPTY_STEP_DONE_THRESHOLD ) { @@ -200,7 +199,8 @@ function handleOwnerFinalizeCompletion(args: { owner_failure_count: 0, owner_step_done_streak: 0, finalize_step_done_count: nextFinalizeStepDoneCount, - empty_step_done_streak: 0, + empty_step_done_streak: + ownerVerdict === 'step_done' ? nextEmptyStepDoneStreak : 0, }, }); } @@ -213,36 +213,27 @@ function handleOwnerFinalizeCompletion(args: { }, ownerVerdict === 'done_with_concerns' ? 'Owner raised concerns during finalize — task set back to active' - : 'Owner made changes after reviewer approval — task set back to active before re-review', + : ownerVerdict === 'step_done' + ? 'Owner reported STEP_DONE during finalize — task set back to active before review' + : 'Owner made changes after reviewer approval — task set back to active before re-review', ); - return 're_review'; - } - - if (signal.kind === 'request_owner_continue') { - transitionPairedTaskStatus({ + maybeAutoTriggerReviewerAfterOwnerCompletion({ + task, taskId, - currentStatus: task.status, - nextStatus: 'active', - expectedUpdatedAt: task.updated_at, - updatedAt: now, - patch: { - owner_failure_count: 0, - owner_step_done_streak: 0, - finalize_step_done_count: nextFinalizeStepDoneCount, - empty_step_done_streak: nextEmptyStepDoneStreak, - }, + now, + logMessage: + ownerVerdict === 'step_done' + ? 'Auto-triggered reviewer after owner finalize STEP_DONE' + : 'Auto-triggered reviewer after owner finalize required re-review', + patch: + ownerVerdict === 'step_done' + ? { + finalize_step_done_count: nextFinalizeStepDoneCount, + empty_step_done_streak: nextEmptyStepDoneStreak, + } + : undefined, }); - logger.info( - { - taskId, - ownerVerdict, - emptyStepDoneStreak: nextEmptyStepDoneStreak, - finalizeStepDoneCount: nextFinalizeStepDoneCount, - summary: summary?.slice(0, 100), - }, - 'Owner marked finalize output as an intermediate step — task returned to active without re-review', - ); - return 'continue_owner'; + return 'stop'; } transitionPairedTaskStatus({ @@ -271,6 +262,11 @@ function maybeAutoTriggerReviewerAfterOwnerCompletion(args: { taskId: string; now: string; logMessage: string; + patch?: { + owner_step_done_streak?: number; + finalize_step_done_count?: number; + empty_step_done_streak?: number; + }; }): void { const { task, taskId, now, logMessage } = args; if (task.round_trip_count >= PAIRED_MAX_ROUND_TRIPS) { @@ -300,6 +296,7 @@ function maybeAutoTriggerReviewerAfterOwnerCompletion(args: { owner_failure_count: 0, owner_step_done_streak: 0, empty_step_done_streak: 0, + ...args.patch, }, }); if (hasActiveCiWatcherForChat(task.chat_jid)) { @@ -345,16 +342,8 @@ export function handleOwnerCompletion(args: { } const ownerVerdict = parseVisibleVerdict(summary); - const workspace = getPairedWorkspace(task.id, 'owner'); - const hasNewChanges = workspace?.workspace_dir - ? hasCodeChangesSinceRef(workspace.workspace_dir, task.source_ref) - : null; const nextOwnerStepDoneStreak = ownerVerdict === 'step_done' ? (task.owner_step_done_streak ?? 0) + 1 : 0; - const nextEmptyStepDoneStreak = - ownerVerdict === 'step_done' && hasNewChanges === false - ? (task.empty_step_done_streak ?? 0) + 1 - : 0; const signal = resolveOwnerCompletionSignal({ phase: 'normal', visibleVerdict: ownerVerdict, @@ -372,86 +361,23 @@ export function handleOwnerCompletion(args: { taskId, ownerVerdict, ownerStepDoneStreak: nextOwnerStepDoneStreak, - emptyStepDoneStreak: nextEmptyStepDoneStreak, summary: summary?.slice(0, 100), }, patch: { owner_failure_count: 0, owner_step_done_streak: nextOwnerStepDoneStreak, - empty_step_done_streak: nextEmptyStepDoneStreak, }, }); return; } - if ( - signal.kind === 'request_owner_continue' && - hasNewChanges === false && - nextOwnerStepDoneStreak >= OWNER_STEP_DONE_LOOP_THRESHOLD && - nextEmptyStepDoneStreak >= OWNER_STEP_DONE_LOOP_THRESHOLD - ) { - requestArbiterOrEscalate({ - taskId, - currentStatus: task.status, - expectedUpdatedAt: task.updated_at, - now, - arbiterLogMessage: - 'Owner repeated STEP_DONE in active mode without code changes — requesting arbiter', - escalateLogMessage: - 'Owner repeated STEP_DONE in active mode without code changes — escalating to user', - logContext: { - taskId, - ownerVerdict, - hasNewChanges, - ownerStepDoneStreak: nextOwnerStepDoneStreak, - emptyStepDoneStreak: nextEmptyStepDoneStreak, - summary: summary?.slice(0, 100), - }, - patch: { - owner_failure_count: 0, - owner_step_done_streak: nextOwnerStepDoneStreak, - empty_step_done_streak: nextEmptyStepDoneStreak, - }, - }); - return; - } - - if (signal.kind === 'request_owner_continue') { - applyPairedTaskPatch({ - taskId, - expectedUpdatedAt: task.updated_at, - updatedAt: now, - patch: { - owner_failure_count: 0, - owner_step_done_streak: nextOwnerStepDoneStreak, - empty_step_done_streak: nextEmptyStepDoneStreak, - }, - }); - logger.info( - { - taskId, - ownerVerdict, - hasNewChanges, - ownerStepDoneStreak: nextOwnerStepDoneStreak, - emptyStepDoneStreak: nextEmptyStepDoneStreak, - summary: summary?.slice(0, 100), - }, - 'Owner marked the current output as an intermediate completed step — keeping task active for follow-up', - ); - return; - } - - if ( - nextOwnerStepDoneStreak !== (task.owner_step_done_streak ?? 0) || - nextEmptyStepDoneStreak !== (task.empty_step_done_streak ?? 0) - ) { + if (nextOwnerStepDoneStreak !== (task.owner_step_done_streak ?? 0)) { applyPairedTaskPatch({ taskId, expectedUpdatedAt: task.updated_at, updatedAt: now, patch: { owner_step_done_streak: nextOwnerStepDoneStreak, - empty_step_done_streak: nextEmptyStepDoneStreak, }, }); } diff --git a/src/paired-execution-context-shared.test.ts b/src/paired-execution-context-shared.test.ts index 3cb197a..c6ceddf 100644 --- a/src/paired-execution-context-shared.test.ts +++ b/src/paired-execution-context-shared.test.ts @@ -27,7 +27,7 @@ describe('paired execution context shared verdict helpers', () => { visibleVerdict: 'step_done', }), ).toEqual({ - kind: 'request_owner_continue', + kind: 'request_reviewer', resetStatusToActive: false, }); expect( @@ -57,7 +57,7 @@ describe('paired execution context shared verdict helpers', () => { deadlockThreshold: 2, }), ).toEqual({ - kind: 'request_owner_continue', + kind: 'request_reviewer', resetStatusToActive: true, }); expect( diff --git a/src/paired-execution-context-shared.ts b/src/paired-execution-context-shared.ts index 998e374..1ede0b1 100644 --- a/src/paired-execution-context-shared.ts +++ b/src/paired-execution-context-shared.ts @@ -9,7 +9,6 @@ import type { PairedTaskStatus } from './types.js'; export type CompletionSignal = | { kind: 'request_reviewer'; resetStatusToActive: boolean } | { kind: 'request_owner_finalize' } - | { kind: 'request_owner_continue'; resetStatusToActive: boolean } | { kind: 'request_owner_changes' } | { kind: 'request_arbiter' } | { kind: 'complete'; completionReason: 'done' | 'escalated' } @@ -38,12 +37,6 @@ export function resolveOwnerCompletionSignal(args: { } if (phase === 'normal') { - if (visibleVerdict === 'step_done') { - return { - kind: 'request_owner_continue', - resetStatusToActive: false, - }; - } return { kind: 'request_reviewer', resetStatusToActive: false, @@ -52,7 +45,7 @@ export function resolveOwnerCompletionSignal(args: { if (visibleVerdict === 'step_done') { return { - kind: 'request_owner_continue', + kind: 'request_reviewer', resetStatusToActive: true, }; } diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index c7a6df2..1c4ab42 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -802,13 +802,22 @@ describe('paired execution context', () => { ).not.toHaveBeenCalled(); }); - it('keeps an active task in owner follow-up mode when the owner reports STEP_DONE', () => { + it('requests review when the owner reports STEP_DONE in active mode', () => { vi.mocked(db.getPairedTaskById).mockReturnValue( buildPairedTask({ status: 'active', round_trip_count: 1, }), ); + vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue( + { + ownerWorkspace: buildWorkspace('owner', '/tmp/paired/task-1/owner'), + reviewerWorkspace: buildWorkspace( + 'reviewer', + '/tmp/paired/task-1/reviewer', + ), + }, + ); completePairedExecutionContext({ taskId: 'task-1', @@ -820,22 +829,17 @@ describe('paired execution context', () => { expect(db.updatePairedTask).toHaveBeenCalledWith( 'task-1', expect.objectContaining({ - owner_failure_count: 0, - owner_step_done_streak: 1, + round_trip_count: 2, + owner_step_done_streak: 0, + empty_step_done_streak: 0, }), ); expect( pairedWorkspaceManager.markPairedTaskReviewReady, - ).not.toHaveBeenCalled(); - expect(db.updatePairedTask).not.toHaveBeenCalledWith( - 'task-1', - expect.objectContaining({ - status: 'active', - }), - ); + ).toHaveBeenCalledWith('task-1'); }); - it('requests arbiter when active STEP_DONE repeats without code changes', () => { + it('requests review instead of arbiter when active STEP_DONE repeats without code changes', () => { vi.spyOn(config, 'isArbiterEnabled').mockReturnValue(true); const repoDir = createCanonicalRepoWithCommit('active step done loop'); @@ -851,6 +855,15 @@ describe('paired execution context', () => { vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) => role === 'owner' ? buildWorkspace('owner', repoDir) : undefined, ); + vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue( + { + ownerWorkspace: buildWorkspace('owner', repoDir), + reviewerWorkspace: buildWorkspace( + 'reviewer', + '/tmp/paired/task-1/reviewer', + ), + }, + ); completePairedExecutionContext({ taskId: 'task-1', @@ -862,23 +875,44 @@ describe('paired execution context', () => { expect(db.updatePairedTask).toHaveBeenCalledWith( 'task-1', expect.objectContaining({ - status: 'arbiter_requested', - owner_step_done_streak: 3, - empty_step_done_streak: 3, + round_trip_count: 1, + owner_step_done_streak: 0, + empty_step_done_streak: 0, }), ); expect( pairedWorkspaceManager.markPairedTaskReviewReady, - ).not.toHaveBeenCalled(); + ).toHaveBeenCalledWith('task-1'); + expect(db.updatePairedTask).not.toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'arbiter_requested', + }), + ); }); - it('returns merge_ready owner finalize output to active when the owner reports STEP_DONE', () => { + it('re-triggers review when the owner reports STEP_DONE during finalize', () => { + const repoDir = createCanonicalRepoWithCommit('reviewed'); + const approvedSourceRef = resolveTreeRef(repoDir); vi.mocked(db.getPairedTaskById).mockReturnValue( buildPairedTask({ status: 'merge_ready', + source_ref: approvedSourceRef, round_trip_count: 1, }), ); + vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) => + role === 'owner' ? buildWorkspace('owner', repoDir) : undefined, + ); + vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue( + { + ownerWorkspace: buildWorkspace('owner', repoDir), + reviewerWorkspace: buildWorkspace( + 'reviewer', + '/tmp/paired/task-1/reviewer', + ), + }, + ); completePairedExecutionContext({ taskId: 'task-1', @@ -893,11 +927,20 @@ describe('paired execution context', () => { status: 'active', owner_failure_count: 0, finalize_step_done_count: 1, + empty_step_done_streak: 1, + }), + ); + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + round_trip_count: 2, + finalize_step_done_count: 1, + empty_step_done_streak: 1, }), ); expect( pairedWorkspaceManager.markPairedTaskReviewReady, - ).not.toHaveBeenCalled(); + ).toHaveBeenCalledWith('task-1'); }); it('requests arbiter when finalize STEP_DONE repeats without code changes', () => {