From 81cb9bda01a415c6538e1a80086f270ab8de3ebe Mon Sep 17 00:00:00 2001 From: ejclaw Date: Tue, 7 Apr 2026 07:06:34 +0900 Subject: [PATCH] Fix paired finalize re-review reentry --- src/logger.ts | 3 +- src/message-agent-executor.test.ts | 4 +- src/message-runtime.ts | 5 +- src/paired-execution-context-owner.ts | 125 +++++++++++++++----------- src/paired-execution-context.test.ts | 73 +++++++++++++++ src/paired-workspace-manager.test.ts | 6 +- 6 files changed, 154 insertions(+), 62 deletions(-) diff --git a/src/logger.ts b/src/logger.ts index 2da75a8..64376e4 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -28,7 +28,8 @@ function createRootLogger(): Logger { } export const logger = - globalState.__ejclawLogger ?? (globalState.__ejclawLogger = createRootLogger()); + globalState.__ejclawLogger ?? + (globalState.__ejclawLogger = createRootLogger()); type LogBindings = Record; diff --git a/src/message-agent-executor.test.ts b/src/message-agent-executor.test.ts index 75b2a10..b9d7aa2 100644 --- a/src/message-agent-executor.test.ts +++ b/src/message-agent-executor.test.ts @@ -1410,9 +1410,7 @@ describe('runAgentForGroup room memory', () => { ); expect( vi.mocked(db.insertPairedTurnOutput).mock.invocationCallOrder[0], - ).toBeLessThan( - onOutput.mock.invocationCallOrder[0], - ); + ).toBeLessThan(onOutput.mock.invocationCallOrder[0]); expect( vi.mocked(db.insertPairedTurnOutput).mock.invocationCallOrder[0], ).toBeLessThan( diff --git a/src/message-runtime.ts b/src/message-runtime.ts index 9e53d75..05678a7 100644 --- a/src/message-runtime.ts +++ b/src/message-runtime.ts @@ -1156,10 +1156,7 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): { await channel .setTyping?.(chatJid, true) ?.catch((err) => - logger.warn( - { chatJid, err }, - 'Failed to set typing indicator', - ), + logger.warn({ chatJid, err }, 'Failed to set typing indicator'), ); } return; diff --git a/src/paired-execution-context-owner.ts b/src/paired-execution-context-owner.ts index 80899f6..2694a1f 100644 --- a/src/paired-execution-context-owner.ts +++ b/src/paired-execution-context-owner.ts @@ -2,10 +2,7 @@ import { ARBITER_DEADLOCK_THRESHOLD, PAIRED_MAX_ROUND_TRIPS, } from './config.js'; -import { - getPairedWorkspace, - hasActiveCiWatcherForChat, -} from './db.js'; +import { getPairedWorkspace, hasActiveCiWatcherForChat } from './db.js'; import { logger } from './logger.js'; import { markPairedTaskReviewReady } from './paired-workspace-manager.js'; import { @@ -18,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; @@ -43,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); @@ -60,7 +59,7 @@ function handleOwnerFinalizeCompletion(args: { summary: summary?.slice(0, 100), }, }); - return true; + return 'stop'; } if (ownerVerdict === 'done_with_concerns') { @@ -77,7 +76,7 @@ function handleOwnerFinalizeCompletion(args: { roundTrips: task.round_trip_count, }, }); - return true; + return 'stop'; } transitionPairedTaskStatus({ taskId, @@ -93,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'); @@ -117,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({ @@ -143,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( @@ -174,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( { @@ -214,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..efa7d2b 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -518,6 +518,60 @@ 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 +615,25 @@ 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({ diff --git a/src/paired-workspace-manager.test.ts b/src/paired-workspace-manager.test.ts index 1d20003..5e7723f 100644 --- a/src/paired-workspace-manager.test.ts +++ b/src/paired-workspace-manager.test.ts @@ -278,9 +278,9 @@ describe('paired workspace manager', () => { db.getPairedTaskById('paired-task-no-owner-workspace') ?.review_requested_at, ).toBeNull(); - expect( - db.getPairedTaskById('paired-task-no-owner-workspace')?.status, - ).toBe('active'); + expect(db.getPairedTaskById('paired-task-no-owner-workspace')?.status).toBe( + 'active', + ); }); it('uses the shared DB owner workspace across service-local data dirs', async () => {