feat: verdict-based ping-pong control with merge_ready finalization

- DONE → merge_ready → owner final turn (commit/push) → completed - DONE_WITH_CONCERNS → active → owner addresses → ping-pong continues - BLOCKED/NEEDS_CONTEXT → completed (escalate to user, stop ping-pong) - Regular feedback → active → ping-pong continues - Owner detects merge_ready status and finalizes autonomously
2026-03-29 20:59:08 +09:00
parent 16d20fe627
commit d8cb4b691e
5 changed files with 125 additions and 69 deletions
--- a/src/config.ts
+++ b/src/config.ts
@@ -106,7 +106,9 @@ export const REVIEWER_AGENT_TYPE: AgentType =

 /** Service ID for the reviewer based on agent type. */
 export const REVIEWER_SERVICE_ID_FOR_TYPE =
-  REVIEWER_AGENT_TYPE === 'claude-code' ? CLAUDE_SERVICE_ID : CODEX_REVIEW_SERVICE_ID;
+  REVIEWER_AGENT_TYPE === 'claude-code'
+    ? CLAUDE_SERVICE_ID
+    : CODEX_REVIEW_SERVICE_ID;

 // Max owner↔reviewer round trips per task. 0 = unlimited.
 const rawMaxRoundTrips = getEnv('PAIRED_MAX_ROUND_TRIPS') || '10';
--- a/src/index.ts
+++ b/src/index.ts
@@ -511,44 +511,44 @@ async function main(): Promise<void> {
  });

  leaseRecoveryTimer = setInterval(() => {
-      if (!hasAvailableClaudeToken()) {
-        return;
+    if (!hasAvailableClaudeToken()) {
+      return;
+    }
+    const now = Date.now();
+    for (const lease of getActiveCodexFailoverLeases()) {
+      const activatedMs = lease.activatedAt
+        ? new Date(lease.activatedAt).getTime()
+        : NaN;
+      if (Number.isNaN(activatedMs)) {
+        logger.warn(
+          { chatJid: lease.chatJid, activatedAt: lease.activatedAt },
+          'Failover lease has unparseable activated_at, skipping auto-restore',
+        );
+        continue;
      }
-      const now = Date.now();
-      for (const lease of getActiveCodexFailoverLeases()) {
-        const activatedMs = lease.activatedAt
-          ? new Date(lease.activatedAt).getTime()
-          : NaN;
-        if (Number.isNaN(activatedMs)) {
-          logger.warn(
-            { chatJid: lease.chatJid, activatedAt: lease.activatedAt },
-            'Failover lease has unparseable activated_at, skipping auto-restore',
-          );
-          continue;
-        }
-        const elapsed = now - activatedMs;
-        if (elapsed < FAILOVER_MIN_DURATION_MS) {
-          logger.debug(
-            {
-              chatJid: lease.chatJid,
-              elapsedMin: Math.round(elapsed / 60_000),
-              minDurationMin: Math.round(FAILOVER_MIN_DURATION_MS / 60_000),
-            },
-            'Failover lease still within minimum hold period, skipping restore',
-          );
-          continue;
-        }
-        restoreDefaultChannelLease(lease.chatJid);
-        logger.info(
+      const elapsed = now - activatedMs;
+      if (elapsed < FAILOVER_MIN_DURATION_MS) {
+        logger.debug(
          {
            chatJid: lease.chatJid,
-            serviceId: SERVICE_ID,
            elapsedMin: Math.round(elapsed / 60_000),
+            minDurationMin: Math.round(FAILOVER_MIN_DURATION_MS / 60_000),
          },
-          'Claude token available and failover hold period elapsed, restored default channel lease',
+          'Failover lease still within minimum hold period, skipping restore',
        );
+        continue;
      }
-    }, 5_000);
+      restoreDefaultChannelLease(lease.chatJid);
+      logger.info(
+        {
+          chatJid: lease.chatJid,
+          serviceId: SERVICE_ID,
+          elapsedMin: Math.round(elapsed / 60_000),
+        },
+        'Claude token available and failover hold period elapsed, restored default channel lease',
+      );
+    }
+  }, 5_000);
  runtime.startMessageLoop().catch((err) => {
    logger.fatal({ err }, 'Message loop crashed unexpectedly');
    process.exit(1);
--- a/src/message-runtime.ts
+++ b/src/message-runtime.ts
@@ -491,7 +491,10 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): {
        (pendingTask.status === 'review_ready' ||
          pendingTask.status === 'in_review');
      const deliveryChannel = isReviewerWorkItem ? reviewerChannel : channel;
-      const delivered = await deliverOpenWorkItem(deliveryChannel, openWorkItem);
+      const delivered = await deliverOpenWorkItem(
+        deliveryChannel,
+        openWorkItem,
+      );
      if (!delivered) return false;
    }

@@ -532,9 +535,10 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): {
          if (ownerContent) {
            parts.push(`Owner response:\n---\n${ownerContent}\n---`);
          }
-          const reviewPrompt = parts.length > 0
-            ? `${parts.join('\n\n')}\n\nReview the owner's response above. Provide feedback or approve.`
-            : 'Review the latest owner changes in the workspace. Provide feedback or approve.';
+          const reviewPrompt =
+            parts.length > 0
+              ? `${parts.join('\n\n')}\n\nReview the owner's response above. Provide feedback or approve.`
+              : 'Review the latest owner changes in the workspace. Provide feedback or approve.';

          // Advance cursor past the owner's messages so they aren't re-processed
          const lastRaw = rawMissedMessages[rawMissedMessages.length - 1];
@@ -559,6 +563,31 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): {
          return deliverySucceeded;
        }

+        // merge_ready: reviewer approved, owner gets final turn to finalize
+        if (pendingReviewTask && pendingReviewTask.status === 'merge_ready') {
+          const lastRaw = rawMissedMessages[rawMissedMessages.length - 1];
+          if (lastRaw?.seq != null) {
+            advanceLastAgentCursor(
+              deps.getLastAgentTimestamps(),
+              deps.saveState,
+              chatJid,
+              lastRaw.seq,
+            );
+          }
+          const finalizePrompt =
+            'The reviewer approved your work (DONE). Finalize: commit changes, push if appropriate, and report the result.';
+          const { deliverySucceeded } = await executeTurn({
+            group,
+            prompt: finalizePrompt,
+            chatJid,
+            runId,
+            channel,
+            startSeq: null,
+            endSeq: null,
+          });
+          return deliverySucceeded;
+        }
+
        const lastIgnored = rawMissedMessages[rawMissedMessages.length - 1];
        if (lastIgnored) {
          advanceLastAgentCursor(
--- a/src/output-suppression.ts
+++ b/src/output-suppression.ts
@@ -28,8 +28,6 @@ export function buildStructuredOutputPrompt(
  return prompt;
 }

-export function parseStructuredOutputEnvelope(
-  _rawText: string,
-): null {
+export function parseStructuredOutputEnvelope(_rawText: string): null {
  return null;
 }
--- a/src/paired-execution-context.ts
+++ b/src/paired-execution-context.ts
@@ -29,17 +29,19 @@ import type {
 } from './types.js';

 // ---------------------------------------------------------------------------
-// Reviewer approval detection
+// Reviewer verdict detection
 // ---------------------------------------------------------------------------

-// Only check the first line for explicit status markers from the prompt's
-// completion status protocol (DONE, DONE_WITH_CONCERNS, BLOCKED, NEEDS_CONTEXT).
-// DONE = approved → stop ping-pong. Everything else = continue.
-function isReviewerApproval(summary: string | null | undefined): boolean {
-  if (!summary) return false;
+type ReviewerVerdict = 'done' | 'done_with_concerns' | 'blocked' | 'needs_context' | 'continue';
+
+function classifyReviewerVerdict(summary: string | null | undefined): ReviewerVerdict {
+  if (!summary) return 'continue';
  const firstLine = summary.trimStart().split('\n')[0].trim();
-  // Match DONE but not DONE_WITH_CONCERNS
-  return /\bDONE\b/.test(firstLine) && !/\bDONE_WITH_CONCERNS\b/.test(firstLine);
+  if (/\bBLOCKED\b/.test(firstLine)) return 'blocked';
+  if (/\bNEEDS_CONTEXT\b/.test(firstLine)) return 'needs_context';
+  if (/\bDONE_WITH_CONCERNS\b/.test(firstLine)) return 'done_with_concerns';
+  if (/\bDONE\b/.test(firstLine)) return 'done';
+  return 'continue';
 }

 // ---------------------------------------------------------------------------
@@ -261,8 +263,21 @@ export function completePairedExecutionContext(args: {
    return;
  }

-  // Owner finished → auto-trigger reviewer (if within round trip limit)
+  // Owner finished
  if (role === 'owner') {
+    const now = new Date().toISOString();
+
+    // merge_ready → owner finalized after reviewer approval → completed
+    if (task.status === 'merge_ready') {
+      updatePairedTask(taskId, { status: 'completed', updated_at: now });
+      logger.info(
+        { taskId, summary: args.summary?.slice(0, 100) },
+        'Owner finalized after reviewer approval — task completed',
+      );
+      return;
+    }
+
+    // Normal turn → auto-trigger reviewer (if within round trip limit)
    if (task.round_trip_count >= PAIRED_MAX_ROUND_TRIPS) {
      logger.info(
        {
@@ -275,7 +290,6 @@ export function completePairedExecutionContext(args: {
      return;
    }

-    const now = new Date().toISOString();
    const result = markPairedTaskReviewReady(taskId);
    if (result) {
      updatePairedTask(taskId, {
@@ -290,28 +304,41 @@ export function completePairedExecutionContext(args: {
    }
  }

-  // Reviewer finished → check if approved or needs more work
+  // Reviewer finished → classify verdict and route accordingly
  if (role === 'reviewer') {
    const now = new Date().toISOString();
-    const approved = isReviewerApproval(args.summary);
-    if (approved) {
-      updatePairedTask(taskId, {
-        status: 'completed',
-        updated_at: now,
-      });
-      logger.info(
-        { taskId, summary: args.summary?.slice(0, 100) },
-        'Reviewer approved, task completed — ping-pong stopped',
-      );
-    } else {
-      updatePairedTask(taskId, {
-        status: 'active',
-        updated_at: now,
-      });
-      logger.info(
-        { taskId },
-        'Reviewer requested changes, task set back to active for owner',
-      );
+    const verdict = classifyReviewerVerdict(args.summary);
+
+    switch (verdict) {
+      case 'done':
+        // Approved → owner gets final turn to commit/push
+        updatePairedTask(taskId, { status: 'merge_ready', updated_at: now });
+        logger.info(
+          { taskId, verdict, summary: args.summary?.slice(0, 100) },
+          'Reviewer approved — owner gets final turn to finalize',
+        );
+        break;
+
+      case 'blocked':
+      case 'needs_context':
+        // Needs user decision — stop ping-pong, leave task active
+        updatePairedTask(taskId, { status: 'completed', updated_at: now });
+        logger.info(
+          { taskId, verdict, summary: args.summary?.slice(0, 100) },
+          'Reviewer escalated to user — ping-pong stopped',
+        );
+        break;
+
+      case 'done_with_concerns':
+      case 'continue':
+      default:
+        // Owner needs to address feedback — ping-pong continues
+        updatePairedTask(taskId, { status: 'active', updated_at: now });
+        logger.info(
+          { taskId, verdict },
+          'Reviewer has feedback, task set back to active for owner',
+        );
+        break;
    }
  }
 }