Fix paired finalize re-review reentry

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<string, unknown>;
 

src/message-agent-executor.test.ts

@@ -1410,9 +1410,7 @@ describe('runAgentForGroup room memory', () => {
     );
     expect(
       vi.mocked(db.insertPairedTurnOutput).mock.invocationCallOrder[0],
-    ).toBeLessThan(
+    ).toBeLessThan(onOutput.mock.invocationCallOrder[0]);
-      onOutput.mock.invocationCallOrder[0],
-    );
     expect(
       vi.mocked(db.insertPairedTurnOutput).mock.invocationCallOrder[0],
     ).toBeLessThan(

src/message-runtime.ts

@@ -1156,10 +1156,7 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): {
         await channel
           .setTyping?.(chatJid, true)
           ?.catch((err) =>
-            logger.warn(
+            logger.warn({ chatJid, err }, 'Failed to set typing indicator'),
-              { chatJid, err },
-              'Failed to set typing indicator',
-            ),
           );
       }
       return;

src/paired-execution-context-owner.ts

@@ -2,10 +2,7 @@ import {
   ARBITER_DEADLOCK_THRESHOLD,
   PAIRED_MAX_ROUND_TRIPS,
 } from './config.js';
-import {
+import { getPairedWorkspace, hasActiveCiWatcherForChat } from './db.js';
-  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 { task, taskId, now, logMessage } = args;
-  const now = new Date().toISOString();
-
-  if (task.status === 'merge_ready') {
-    const shouldStop = handleOwnerFinalizeCompletion({
-      task,
-      taskId,
-      summary,
-      now,
-    });
-    if (shouldStop) {
-      return;
-    }
-  }
 
   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(
+    logger.info({ taskId, roundTrip: task.round_trip_count + 1 }, logMessage);
-      { taskId, roundTrip: task.round_trip_count + 1 },
-      'Auto-triggered reviewer after owner completion',
-    );
   }
 }
+
+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',
+  });
+}

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({

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(
+    expect(db.getPairedTaskById('paired-task-no-owner-workspace')?.status).toBe(
-      db.getPairedTaskById('paired-task-no-owner-workspace')?.status,
+      'active',
-    ).toBe('active');
+    );
   });
 
   it('uses the shared DB owner workspace across service-local data dirs', async () => {