Merge branch 'codex/owner/ejclaw'

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',
+  });
+}

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