From 60f360b5eac7a5bf549ebb3272299fc395de4d65 Mon Sep 17 00:00:00 2001 From: Eyejoker Date: Wed, 1 Apr 2026 02:54:35 +0900 Subject: [PATCH] fix: preserve handoff roles during failover --- src/db.test.ts | 22 +++++++++++ src/db.ts | 18 +++++++-- src/message-agent-executor.test.ts | 57 ++++++++++++++++++++++++++++ src/message-agent-executor.ts | 14 ++++++- src/message-runtime.test.ts | 32 +++++++++++++++- src/message-runtime.ts | 34 ++++++++++++++--- src/paired-execution-context.test.ts | 45 ++++++++++++++++++++++ src/paired-execution-context.ts | 24 ++++++++++++ 8 files changed, 235 insertions(+), 11 deletions(-) diff --git a/src/db.test.ts b/src/db.test.ts index 4e01adc..c8c0ef8 100644 --- 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', () => { diff --git a/src/db.ts b/src/db.ts index 644a3df..0360656 100644 --- 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 = ( diff --git a/src/message-agent-executor.test.ts b/src/message-agent-executor.test.ts index 201cda5..e729c8c 100644 --- 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', }), ); }); diff --git a/src/message-agent-executor.ts b/src/message-agent-executor.ts index bb2bce5..6214694 100644 --- 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; }, ): 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 }, diff --git a/src/message-runtime.test.ts b/src/message-runtime.test.ts index bd17e04..fb3188f 100644 --- 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'); diff --git a/src/message-runtime.ts b/src/message-runtime.ts index d76595d..747c4c9 100644 --- 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, +): 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) { diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index d82475f..004090d 100644 --- 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) => { diff --git a/src/paired-execution-context.ts b/src/paired-execution-context.ts index d476457..5bcc6d1 100644 --- 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( {