fix: preserve handoff roles during failover

2026-04-01 02:54:35 +09:00
parent a955964522
commit 60f360b5ea
8 changed files with 235 additions and 11 deletions
--- a/src/db.test.ts
+++ b/src/db.test.ts
@@ -1227,6 +1227,28 @@ describe('service handoff completion', () => {
      'dc:handoff': '5',
    });
  });
+
+  it('stores the intended handoff role when provided', () => {
+    const handoff = createServiceHandoff({
+      chat_jid: 'dc:handoff-role',
+      group_folder: 'test-group',
+      source_service_id: 'claude',
+      target_service_id: 'codex-review',
+      target_agent_type: 'codex',
+      prompt: 'please review',
+      reason: 'reviewer-claude-429',
+      intended_role: 'reviewer',
+    });
+
+    expect(handoff.intended_role).toBe('reviewer');
+    expect(getPendingServiceHandoffs('codex-review')).toEqual([
+      expect.objectContaining({
+        id: handoff.id,
+        intended_role: 'reviewer',
+        reason: 'reviewer-claude-429',
+      }),
+    ]);
+  });
 });

 describe('message seq cursors', () => {
--- a/src/db.ts
+++ b/src/db.ts
@@ -159,6 +159,7 @@ export interface ServiceHandoff {
  start_seq: number | null;
  end_seq: number | null;
  reason: string | null;
+  intended_role: PairedRoomRole | null;
  created_at: string;
  claimed_at: string | null;
  completed_at: string | null;
@@ -408,11 +409,13 @@ function createSchema(database: Database): void {
      start_seq INTEGER,
      end_seq INTEGER,
      reason TEXT,
+      intended_role TEXT,
      created_at TEXT NOT NULL DEFAULT (datetime('now')),
      claimed_at TEXT,
      completed_at TEXT,
      last_error TEXT,
-      CHECK (status IN ('pending', 'claimed', 'completed', 'failed'))
+      CHECK (status IN ('pending', 'claimed', 'completed', 'failed')),
+      CHECK (intended_role IN ('owner', 'reviewer', 'arbiter') OR intended_role IS NULL)
    );
    CREATE INDEX IF NOT EXISTS idx_service_handoffs_target
      ON service_handoffs(target_service_id, status, created_at);
@@ -568,6 +571,12 @@ function createSchema(database: Database): void {
    /* column already exists */
  }

+  try {
+    database.exec(`ALTER TABLE service_handoffs ADD COLUMN intended_role TEXT`);
+  } catch {
+    /* column already exists */
+  }
+
  database.exec(
    `UPDATE room_settings
     SET mode_source = 'explicit'
@@ -3518,6 +3527,7 @@ export function createServiceHandoff(input: {
  start_seq?: number | null;
  end_seq?: number | null;
  reason?: string | null;
+  intended_role?: PairedRoomRole | null;
 }): ServiceHandoff {
  db.prepare(
    `INSERT INTO service_handoffs (
@@ -3529,8 +3539,9 @@ export function createServiceHandoff(input: {
        prompt,
        start_seq,
        end_seq,
-        reason
-      ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`,
+        reason,
+        intended_role
+      ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`,
  ).run(
    input.chat_jid,
    input.group_folder,
@@ -3541,6 +3552,7 @@ export function createServiceHandoff(input: {
    input.start_seq ?? null,
    input.end_seq ?? null,
    input.reason ?? null,
+    input.intended_role ?? null,
  );

  const lastId = (
--- a/src/message-agent-executor.test.ts
+++ b/src/message-agent-executor.test.ts
@@ -397,6 +397,61 @@ describe('runAgentForGroup room memory', () => {
    );
  });

+  it('honors a forced reviewer role even when the paired task status is active', async () => {
+    const group = { ...makeGroup(), folder: 'test-group' };
+    vi.mocked(serviceRouting.getEffectiveChannelLease).mockReturnValue({
+      chat_jid: 'group@test',
+      owner_service_id: 'codex-main',
+      reviewer_service_id: 'claude',
+      arbiter_service_id: null,
+      activated_at: null,
+      reason: null,
+      explicit: false,
+    });
+    vi.mocked(db.getLatestOpenPairedTaskForChat).mockReturnValue({
+      id: 'paired-task-active',
+      chat_jid: 'group@test',
+      group_folder: 'test-group',
+      owner_service_id: 'codex-main',
+      reviewer_service_id: 'claude',
+      title: null,
+      source_ref: 'HEAD',
+      plan_notes: null,
+      round_trip_count: 0,
+      review_requested_at: '2026-03-31T00:00:00.000Z',
+      status: 'active',
+      arbiter_verdict: null,
+      arbiter_requested_at: null,
+      completion_reason: null,
+      created_at: '2026-03-31T00:00:00.000Z',
+      updated_at: '2026-03-31T00:00:00.000Z',
+    });
+
+    await runAgentForGroup(makeDeps(), {
+      group,
+      prompt: 'please retry review',
+      chatJid: 'group@test',
+      runId: 'run-forced-reviewer',
+      forcedRole: 'reviewer',
+    });
+
+    expect(agentRunner.runAgentProcess).toHaveBeenCalledWith(
+      group,
+      expect.objectContaining({
+        roomRoleContext: {
+          serviceId: 'claude',
+          role: 'reviewer',
+          ownerServiceId: 'codex-main',
+          reviewerServiceId: 'claude',
+          failoverOwner: false,
+        },
+      }),
+      expect.any(Function),
+      undefined,
+      undefined,
+    );
+  });
+
  it('allows silent reviewer outputs', async () => {
    const group = { ...makeGroup(), folder: 'test-group', workDir: '/repo' };
    vi.mocked(serviceRouting.getEffectiveChannelLease).mockReturnValue({
@@ -1005,6 +1060,7 @@ describe('runAgentForGroup Claude rotation', () => {
        start_seq: 10,
        end_seq: 12,
        reason: 'claude-usage-exhausted',
+        intended_role: 'owner',
      }),
    );
  });
@@ -1207,6 +1263,7 @@ describe('runAgentForGroup Claude rotation', () => {
        target_service_id: 'codex-review',
        target_agent_type: 'codex',
        reason: 'claude-org-access-denied',
+        intended_role: 'owner',
      }),
    );
  });
--- a/src/message-agent-executor.ts
+++ b/src/message-agent-executor.ts
@@ -69,7 +69,7 @@ import {
 } from './codex-token-rotation.js';
 import type { CodexRotationReason } from './agent-error-detection.js';
 import { getTokenCount } from './token-rotation.js';
-import type { RegisteredGroup } from './types.js';
+import type { PairedRoomRole, RegisteredGroup } from './types.js';

 // ── Main executor ─────────────────────────────────────────────────

@@ -92,6 +92,7 @@ export async function runAgentForGroup(
    startSeq?: number | null;
    endSeq?: number | null;
    hasHumanMessage?: boolean;
+    forcedRole?: PairedRoomRole;
    onOutput?: (output: AgentOutput) => Promise<void>;
  },
 ): Promise<'success' | 'error'> {
@@ -107,7 +108,13 @@ export async function runAgentForGroup(
  const pairedTask = currentLease.reviewer_service_id
    ? getLatestOpenPairedTaskForChat(chatJid)
    : null;
-  const activeRole = resolveActiveRole(pairedTask?.status);
+  const inferredRole = resolveActiveRole(pairedTask?.status);
+  const canHonorForcedRole = Boolean(
+    args.forcedRole === 'owner' ||
+      (args.forcedRole === 'reviewer' && currentLease.reviewer_service_id) ||
+      (args.forcedRole === 'arbiter' && currentLease.arbiter_service_id),
+  );
+  const activeRole = canHonorForcedRole ? args.forcedRole! : inferredRole;
  const effectiveServiceId =
    activeRole === 'arbiter'
      ? currentLease.arbiter_service_id!
@@ -293,6 +300,7 @@ export async function runAgentForGroup(
        start_seq: startSeq ?? null,
        end_seq: endSeq ?? null,
        reason: `arbiter-claude-${reason}`,
+        intended_role: 'arbiter',
      });
      log.warn(
        { reason },
@@ -314,6 +322,7 @@ export async function runAgentForGroup(
        start_seq: startSeq ?? null,
        end_seq: endSeq ?? null,
        reason: `reviewer-claude-${reason}`,
+        intended_role: 'reviewer',
      });
      log.warn(
        { reason },
@@ -333,6 +342,7 @@ export async function runAgentForGroup(
      start_seq: startSeq ?? null,
      end_seq: endSeq ?? null,
      reason: `claude-${reason}`,
+      intended_role: activeRole,
    });
    log.warn(
      { reason },
--- a/src/message-runtime.test.ts
+++ b/src/message-runtime.test.ts
@@ -183,7 +183,10 @@ vi.mock('./session-commands.js', () => ({
 import * as agentRunner from './agent-runner.js';
 import * as db from './db.js';
 import { resolveGroupIpcPath } from './group-folder.js';
-import { createMessageRuntime } from './message-runtime.js';
+import {
+  createMessageRuntime,
+  resolveHandoffRoleOverride,
+} from './message-runtime.js';
 import * as config from './config.js';
 import * as serviceRouting from './service-routing.js';
 import type { Channel, RegisteredGroup } from './types.js';
@@ -223,6 +226,33 @@ describe('createMessageRuntime', () => {
    vi.mocked(config.isReviewService).mockReturnValue(false);
  });

+  it('prefers intended_role over reason prefixes for handoff role resolution', () => {
+    expect(
+      resolveHandoffRoleOverride({
+        intended_role: 'reviewer',
+        reason: 'claude-429',
+      }),
+    ).toBe('reviewer');
+    expect(
+      resolveHandoffRoleOverride({
+        intended_role: null,
+        reason: 'arbiter-claude-429',
+      }),
+    ).toBe('arbiter');
+    expect(
+      resolveHandoffRoleOverride({
+        intended_role: null,
+        reason: 'reviewer-claude-usage-exhausted',
+      }),
+    ).toBe('reviewer');
+    expect(
+      resolveHandoffRoleOverride({
+        intended_role: null,
+        reason: 'claude-usage-exhausted',
+      }),
+    ).toBeUndefined();
+  });
+
  it('ignores generic failure bot messages in paired rooms', async () => {
    const chatJid = 'group@test';
    const group = makeGroup('codex');
--- a/src/message-runtime.ts
+++ b/src/message-runtime.ts
@@ -58,6 +58,7 @@ import {
 import {
  Channel,
  NewMessage,
+  type PairedRoomRole,
  RegisteredGroup,
  type PairedTask,
  type PairedTurnOutput,
@@ -94,6 +95,21 @@ export function isDuplicateOfLastBotFinal(
  return normalizedLast === normalizedCurrent && normalizedLast.length > 0;
 }

+export function resolveHandoffRoleOverride(
+  handoff: Pick<ServiceHandoff, 'intended_role' | 'reason'>,
+): PairedRoomRole | undefined {
+  if (handoff.intended_role) {
+    return handoff.intended_role;
+  }
+  if (handoff.reason?.startsWith('reviewer-')) {
+    return 'reviewer';
+  }
+  if (handoff.reason?.startsWith('arbiter-')) {
+    return 'arbiter';
+  }
+  return undefined;
+}
+
 export interface MessageRuntimeDeps {
  assistantName: string;
  idleTimeout: number;
@@ -415,6 +431,7 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): {
      startSeq?: number | null;
      endSeq?: number | null;
      hasHumanMessage?: boolean;
+      forcedRole?: PairedRoomRole;
    },
  ): Promise<'success' | 'error'> =>
    runAgentForGroup(
@@ -434,6 +451,7 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): {
        startSeq: options?.startSeq,
        endSeq: options?.endSeq,
        hasHumanMessage: options?.hasHumanMessage,
+        forcedRole: options?.forcedRole,
        onOutput,
      },
    );
@@ -447,6 +465,7 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): {
    startSeq: number | null;
    endSeq: number | null;
    hasHumanMessage?: boolean;
+    forcedRole?: PairedRoomRole;
  }): Promise<{
    outputStatus: 'success' | 'error';
    deliverySucceeded: boolean;
@@ -503,7 +522,12 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): {
        chatJid,
        runId,
        (result) => turnController.handleOutput(result),
-        { startSeq, endSeq, hasHumanMessage: args.hasHumanMessage },
+        {
+          startSeq,
+          endSeq,
+          hasHumanMessage: args.hasHumanMessage,
+          forcedRole: args.forcedRole,
+        },
      );

      const { deliverySucceeded, visiblePhase } =
@@ -574,14 +598,13 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): {

    // Reviewer/arbiter failover handoffs should run via the appropriate
    // channel so they execute in the correct role mode.
-    const isReviewerHandoff = handoff.reason?.startsWith('reviewer-');
-    const isArbiterHandoff = handoff.reason?.startsWith('arbiter-');
+    const handoffRole = resolveHandoffRoleOverride(handoff);
    let handoffChannel = channel;
-    if (isReviewerHandoff) {
+    if (handoffRole === 'reviewer') {
      const revChName =
        REVIEWER_AGENT_TYPE === 'claude-code' ? 'discord' : 'discord-review';
      handoffChannel = findChannelByName(deps.channels, revChName) || channel;
-    } else if (isArbiterHandoff) {
+    } else if (handoffRole === 'arbiter') {
      handoffChannel =
        findChannelByName(deps.channels, 'discord-review') || channel;
    }
@@ -596,6 +619,7 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): {
        channel: handoffChannel,
        startSeq: handoff.start_seq,
        endSeq: handoff.end_seq,
+        forcedRole: handoffRole,
      });

      if (!result.deliverySucceeded) {
--- a/src/paired-execution-context.test.ts
+++ b/src/paired-execution-context.test.ts
@@ -445,6 +445,51 @@ describe('paired execution context', () => {
    );
  });

+  it('requests arbiter instead of re-reviewing when repeated DONE finalize loops exceed the threshold', () => {
+    vi.spyOn(config, 'isArbiterEnabled').mockReturnValue(true);
+
+    const repoDir = createCanonicalRepoWithCommit('reviewed');
+    const approvedSourceRef = resolveTreeRef(repoDir);
+    fs.writeFileSync(path.join(repoDir, 'README.md'), 'changed again\n');
+    execFileSync('git', ['add', 'README.md'], {
+      cwd: repoDir,
+      stdio: 'ignore',
+    });
+    execFileSync('git', ['commit', '-m', 'code change'], {
+      cwd: repoDir,
+      stdio: 'ignore',
+    });
+
+    vi.mocked(db.getPairedTaskById).mockReturnValue(
+      buildPairedTask({
+        status: 'merge_ready',
+        source_ref: approvedSourceRef,
+        round_trip_count: config.ARBITER_DEADLOCK_THRESHOLD,
+      }),
+    );
+    vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) =>
+      role === 'owner' ? buildWorkspace('owner', repoDir) : undefined,
+    );
+
+    completePairedExecutionContext({
+      taskId: 'task-1',
+      role: 'owner',
+      status: 'succeeded',
+      summary: 'DONE',
+    });
+
+    expect(db.updatePairedTask).toHaveBeenCalledWith(
+      'task-1',
+      expect.objectContaining({
+        status: 'arbiter_requested',
+        arbiter_requested_at: expect.any(String),
+      }),
+    );
+    expect(
+      pairedWorkspaceManager.markPairedTaskReviewReady,
+    ).not.toHaveBeenCalled();
+  });
+
  it.each(['BLOCKED', 'NEEDS_CONTEXT'])(
    'escalates immediately when owner reports %s during finalize without arbiter',
    (summary) => {
--- a/src/paired-execution-context.ts
+++ b/src/paired-execution-context.ts
@@ -525,6 +525,30 @@ export function completePairedExecutionContext(args: {
          : null;

        if (hasNewChanges === true) {
+          if (task.round_trip_count >= ARBITER_DEADLOCK_THRESHOLD) {
+            if (isArbiterEnabled()) {
+              updatePairedTask(taskId, {
+                status: 'arbiter_requested',
+                arbiter_requested_at: now,
+                updated_at: now,
+              });
+              logger.info(
+                { taskId, roundTrips: task.round_trip_count, hasNewChanges },
+                'Owner finalize DONE loop detected — requesting arbiter',
+              );
+            } else {
+              updatePairedTask(taskId, {
+                status: 'completed',
+                completion_reason: 'escalated',
+                updated_at: now,
+              });
+              logger.info(
+                { taskId, roundTrips: task.round_trip_count, hasNewChanges },
+                'Owner finalize DONE loop detected — escalating to user',
+              );
+            }
+            return;
+          }
          // Owner made changes after approval → needs re-review
          logger.info(
            {