diff --git a/src/paired-execution-context-owner.ts b/src/paired-execution-context-owner.ts index fe9b8a1..2694a1f 100644 --- a/src/paired-execution-context-owner.ts +++ b/src/paired-execution-context-owner.ts @@ -15,6 +15,8 @@ import { } from './paired-execution-context-shared.js'; import type { PairedTask } from './types.js'; +type OwnerFinalizeOutcome = 'stop' | 're_review'; + export function handleFailedOwnerExecution(args: { task: PairedTask; taskId: string; @@ -40,7 +42,7 @@ function handleOwnerFinalizeCompletion(args: { taskId: string; summary?: string | null; now: string; -}): boolean { +}): OwnerFinalizeOutcome { const { task, taskId, summary, now } = args; const ownerVerdict = classifyVerdict(summary); @@ -57,7 +59,7 @@ function handleOwnerFinalizeCompletion(args: { summary: summary?.slice(0, 100), }, }); - return true; + return 'stop'; } if (ownerVerdict === 'done_with_concerns') { @@ -74,7 +76,7 @@ function handleOwnerFinalizeCompletion(args: { roundTrips: task.round_trip_count, }, }); - return true; + return 'stop'; } transitionPairedTaskStatus({ taskId, @@ -90,7 +92,7 @@ function handleOwnerFinalizeCompletion(args: { }, 'Owner raised concerns during finalize — task set back to active', ); - return false; + return 're_review'; } const workspace = getPairedWorkspace(task.id, 'owner'); @@ -114,17 +116,23 @@ function handleOwnerFinalizeCompletion(args: { hasNewChanges, }, }); - return true; + return 'stop'; } + transitionPairedTaskStatus({ + taskId, + currentStatus: task.status, + nextStatus: 'active', + updatedAt: now, + }); logger.info( { taskId, sourceRef: task.source_ref, hasNewChanges, }, - 'Owner made changes after reviewer approval — re-triggering review', + 'Owner made changes after reviewer approval — task set back to active before re-review', ); - return false; + return 're_review'; } transitionPairedTaskStatus({ @@ -140,28 +148,16 @@ function handleOwnerFinalizeCompletion(args: { { taskId, hasNewChanges, summary: summary?.slice(0, 100) }, 'Owner finalized after reviewer approval — task completed', ); - return true; + return 'stop'; } -export function handleOwnerCompletion(args: { +function maybeAutoTriggerReviewerAfterOwnerCompletion(args: { task: PairedTask; taskId: string; - summary?: string | null; + now: string; + logMessage: string; }): void { - const { task, taskId, summary } = args; - const now = new Date().toISOString(); - - if (task.status === 'merge_ready') { - const shouldStop = handleOwnerFinalizeCompletion({ - task, - taskId, - summary, - now, - }); - if (shouldStop) { - return; - } - } + const { task, taskId, now, logMessage } = args; if (hasActiveCiWatcherForChat(task.chat_jid)) { logger.info( @@ -171,25 +167,6 @@ export function handleOwnerCompletion(args: { return; } - if (task.status !== 'merge_ready') { - const ownerVerdict = classifyVerdict(summary); - if (ownerVerdict === 'blocked' || ownerVerdict === 'needs_context') { - requestArbiterOrEscalate({ - taskId, - currentStatus: task.status, - now, - arbiterLogMessage: 'Owner blocked/needs_context — requesting arbiter', - escalateLogMessage: 'Owner blocked/needs_context — escalating to user', - logContext: { - taskId, - ownerVerdict, - summary: summary?.slice(0, 100), - }, - }); - return; - } - } - if (task.round_trip_count >= PAIRED_MAX_ROUND_TRIPS) { logger.info( { @@ -211,9 +188,58 @@ export function handleOwnerCompletion(args: { round_trip_count: task.round_trip_count + 1, }, }); - logger.info( - { taskId, roundTrip: task.round_trip_count + 1 }, - 'Auto-triggered reviewer after owner completion', - ); + logger.info({ taskId, roundTrip: task.round_trip_count + 1 }, logMessage); } } + +export function handleOwnerCompletion(args: { + task: PairedTask; + taskId: string; + summary?: string | null; +}): void { + const { task, taskId, summary } = args; + const now = new Date().toISOString(); + + if (task.status === 'merge_ready') { + const finalizeOutcome = handleOwnerFinalizeCompletion({ + task, + taskId, + summary, + now, + }); + if (finalizeOutcome === 're_review') { + maybeAutoTriggerReviewerAfterOwnerCompletion({ + task, + taskId, + now, + logMessage: + 'Auto-triggered reviewer after owner finalize required re-review', + }); + } + return; + } + + const ownerVerdict = classifyVerdict(summary); + if (ownerVerdict === 'blocked' || ownerVerdict === 'needs_context') { + requestArbiterOrEscalate({ + taskId, + currentStatus: task.status, + now, + arbiterLogMessage: 'Owner blocked/needs_context — requesting arbiter', + escalateLogMessage: 'Owner blocked/needs_context — escalating to user', + logContext: { + taskId, + ownerVerdict, + summary: summary?.slice(0, 100), + }, + }); + return; + } + + maybeAutoTriggerReviewerAfterOwnerCompletion({ + task, + taskId, + now, + logMessage: 'Auto-triggered reviewer after owner completion', + }); +} diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index 1f57b57..6014fa9 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -518,6 +518,64 @@ describe('paired execution context', () => { ).not.toHaveBeenCalled(); }); + it('re-triggers review once when owner reports DONE_WITH_CONCERNS during finalize', () => { + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'merge_ready', + 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', + role: 'owner', + status: 'succeeded', + summary: 'DONE_WITH_CONCERNS\n\nneeds another reviewer pass', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'active', + }), + ); + expect( + pairedWorkspaceManager.markPairedTaskReviewReady, + ).toHaveBeenCalledTimes(1); + expect( + pairedWorkspaceManager.markPairedTaskReviewReady, + ).toHaveBeenCalledWith('task-1'); + const returnToActiveOrder = vi + .mocked(db.updatePairedTask) + .mock.calls.findIndex( + ([id, patch]) => id === 'task-1' && patch.status === 'active', + ); + const reopenReviewOrder = vi.mocked( + pairedWorkspaceManager.markPairedTaskReviewReady, + ).mock.invocationCallOrder[0]; + expect(returnToActiveOrder).toBeGreaterThanOrEqual(0); + expect( + vi.mocked(db.updatePairedTask).mock.invocationCallOrder[ + returnToActiveOrder + ], + ).toBeLessThan(reopenReviewOrder); + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + round_trip_count: 2, + }), + ); + }); + it('re-triggers review when owner changed code after approval', () => { const repoDir = createCanonicalRepoWithCommit('reviewed'); const approvedSourceRef = resolveTreeRef(repoDir); @@ -561,6 +619,29 @@ describe('paired execution context', () => { expect( pairedWorkspaceManager.markPairedTaskReviewReady, ).toHaveBeenCalledWith('task-1'); + expect( + pairedWorkspaceManager.markPairedTaskReviewReady, + ).toHaveBeenCalledTimes(1); + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'active', + }), + ); + const returnToActiveOrder = vi + .mocked(db.updatePairedTask) + .mock.calls.findIndex( + ([id, patch]) => id === 'task-1' && patch.status === 'active', + ); + const reopenReviewOrder = vi.mocked( + pairedWorkspaceManager.markPairedTaskReviewReady, + ).mock.invocationCallOrder[0]; + expect(returnToActiveOrder).toBeGreaterThanOrEqual(0); + expect( + vi.mocked(db.updatePairedTask).mock.invocationCallOrder[ + returnToActiveOrder + ], + ).toBeLessThan(reopenReviewOrder); expect(db.updatePairedTask).toHaveBeenCalledWith( 'task-1', expect.objectContaining({