From d0ca76f776022108700cd84466236ba5852d0baa Mon Sep 17 00:00:00 2001 From: Eyejoker Date: Fri, 29 May 2026 23:19:42 +0900 Subject: [PATCH] fix: route arbiter resolutions through reviewer (#195) --- src/message-runtime-prompts.ts | 3 +- src/message-runtime.test.ts | 5 +- src/paired-execution-context-arbiter.ts | 44 +++- src/paired-execution-context-reviewer.ts | 16 +- src/paired-execution-context-routing.test.ts | 232 +++++++++++++++++++ src/paired-execution-context.test.ts | 4 + src/paired-task-status.ts | 7 +- src/paired-verdict.test.ts | 1 + src/paired-verdict.ts | 7 +- 9 files changed, 306 insertions(+), 13 deletions(-) create mode 100644 src/paired-execution-context-routing.test.ts diff --git a/src/message-runtime-prompts.ts b/src/message-runtime-prompts.ts index abe836e..1c8ae42 100644 --- a/src/message-runtime-prompts.ts +++ b/src/message-runtime-prompts.ts @@ -212,6 +212,7 @@ export function buildFinalizePendingPrompt(_args: { }): string { return `The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result. If you intend to close this paired turn now, your first line must be TASK_DONE. -If the original request still has remaining work and the owner flow should continue, your first line may be STEP_DONE. +Do not use STEP_DONE only because a broader roadmap still has remaining work; close the approved slice and continue the next slice in a new owner turn. +Use STEP_DONE only when this same approved scope still needs additional owner changes and another review pass. If your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.`; } diff --git a/src/message-runtime.test.ts b/src/message-runtime.test.ts index 2aca497..7f5922d 100644 --- a/src/message-runtime.test.ts +++ b/src/message-runtime.test.ts @@ -1702,7 +1702,8 @@ describe('createMessageRuntime', () => { expect(input.prompt).toBe( 'The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result.\n' + 'If you intend to close this paired turn now, your first line must be TASK_DONE.\n' + - 'If the original request still has remaining work and the owner flow should continue, your first line may be STEP_DONE.\n' + + 'Do not use STEP_DONE only because a broader roadmap still has remaining work; close the approved slice and continue the next slice in a new owner turn.\n' + + 'Use STEP_DONE only when this same approved scope still needs additional owner changes and another review pass.\n' + 'If your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.', ); expect(input.prompt).not.toContain("Reviewer's final assessment:"); @@ -1811,7 +1812,7 @@ describe('createMessageRuntime', () => { vi.mocked(agentRunner.runAgentProcess).mockImplementation( async (_group, input, _onProcess, onOutput) => { expect(input.prompt).toBe( - 'The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result.\nIf you intend to close this paired turn now, your first line must be TASK_DONE.\nIf the original request still has remaining work and the owner flow should continue, your first line may be STEP_DONE.\nIf your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.', + 'The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result.\nIf you intend to close this paired turn now, your first line must be TASK_DONE.\nDo not use STEP_DONE only because a broader roadmap still has remaining work; close the approved slice and continue the next slice in a new owner turn.\nUse STEP_DONE only when this same approved scope still needs additional owner changes and another review pass.\nIf your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.', ); expect(input.prompt).not.toContain('리뷰 승인 요약'); expect(input.prompt).not.toContain('DONE\n승인합니다.'); diff --git a/src/paired-execution-context-arbiter.ts b/src/paired-execution-context-arbiter.ts index 519228e..863b23e 100644 --- a/src/paired-execution-context-arbiter.ts +++ b/src/paired-execution-context-arbiter.ts @@ -1,9 +1,10 @@ -import { ARBITER_DEADLOCK_THRESHOLD } from './config.js'; import { logger } from './logger.js'; import { transitionPairedTaskStatus } from './paired-task-status.js'; import { classifyArbiterVerdict } from './paired-verdict.js'; import type { PairedTask } from './types.js'; +const ARBITER_RESOLUTION_ROUND_TRIP_COUNT = 0; + export function handleFailedArbiterExecution(args: { task: PairedTask; taskId: string; @@ -50,6 +51,27 @@ export function handleArbiterCompletion(args: { switch (arbiterVerdict) { case 'proceed': + transitionPairedTaskStatus({ + taskId, + currentStatus: 'in_arbitration', + nextStatus: 'review_ready', + expectedUpdatedAt: task.updated_at, + updatedAt: now, + patch: { + round_trip_count: ARBITER_RESOLUTION_ROUND_TRIP_COUNT, + owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: 0, + empty_step_done_streak: 0, + arbiter_verdict: arbiterVerdict, + arbiter_requested_at: null, + }, + }); + logger.info( + { taskId, arbiterVerdict }, + 'Arbiter proceeded — returning task to reviewer for approval', + ); + return; case 'revise': case 'reset': transitionPairedTaskStatus({ @@ -59,13 +81,18 @@ export function handleArbiterCompletion(args: { expectedUpdatedAt: task.updated_at, updatedAt: now, patch: { - round_trip_count: Math.max(0, ARBITER_DEADLOCK_THRESHOLD - 1), + round_trip_count: ARBITER_RESOLUTION_ROUND_TRIP_COUNT, + owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: 0, + empty_step_done_streak: 0, arbiter_verdict: arbiterVerdict, + arbiter_requested_at: null, }, }); logger.info( { taskId, arbiterVerdict }, - 'Arbiter resolved deadlock — resuming ping-pong', + 'Arbiter requested owner changes — resuming owner flow', ); return; case 'escalate': @@ -86,17 +113,22 @@ export function handleArbiterCompletion(args: { transitionPairedTaskStatus({ taskId, currentStatus: 'in_arbitration', - nextStatus: 'active', + nextStatus: 'review_ready', expectedUpdatedAt: task.updated_at, updatedAt: now, patch: { - round_trip_count: Math.max(0, ARBITER_DEADLOCK_THRESHOLD - 1), + round_trip_count: ARBITER_RESOLUTION_ROUND_TRIP_COUNT, + owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: 0, + empty_step_done_streak: 0, arbiter_verdict: 'unknown', + arbiter_requested_at: null, }, }); logger.warn( { taskId, summary: summary?.slice(0, 200) }, - 'Arbiter verdict unrecognized — falling back to proceed', + 'Arbiter verdict unrecognized — returning task to reviewer as proceed fallback', ); return; } diff --git a/src/paired-execution-context-reviewer.ts b/src/paired-execution-context-reviewer.ts index ab996bc..8dc0d58 100644 --- a/src/paired-execution-context-reviewer.ts +++ b/src/paired-execution-context-reviewer.ts @@ -47,7 +47,15 @@ export function handleFailedReviewerExecution(args: { updatedAt: now, patch: { ...(signal.kind === 'request_owner_finalize' - ? { source_ref: approvedSourceRef } + ? { + source_ref: approvedSourceRef, + owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: 0, + empty_step_done_streak: 0, + arbiter_verdict: null, + arbiter_requested_at: null, + } : {}), ...(signal.kind === 'complete' ? { completion_reason: signal.completionReason } @@ -119,6 +127,12 @@ export function handleReviewerCompletion(args: { updatedAt: now, patch: { source_ref: approvedSourceRef, + owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: 0, + empty_step_done_streak: 0, + arbiter_verdict: null, + arbiter_requested_at: null, }, }); logger.info( diff --git a/src/paired-execution-context-routing.test.ts b/src/paired-execution-context-routing.test.ts new file mode 100644 index 0000000..8f53ca8 --- /dev/null +++ b/src/paired-execution-context-routing.test.ts @@ -0,0 +1,232 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('./db.js', () => { + const updatePairedTask = vi.fn(); + return { + getPairedTaskById: vi.fn(), + getPairedWorkspace: vi.fn(), + updatePairedTask, + updatePairedTaskIfUnchanged: vi.fn((id, _expectedUpdatedAt, updates) => { + updatePairedTask(id, updates); + return true; + }), + releasePairedTaskExecutionLease: vi.fn(), + }; +}); + +vi.mock('./paired-workspace-manager.js', () => ({ + isOwnerWorkspaceRepairNeededError: vi.fn(() => false), + markPairedTaskReviewReady: vi.fn(), + prepareReviewerWorkspaceForExecution: vi.fn(), + provisionOwnerWorkspaceForPairedTask: vi.fn(), +})); + +vi.mock('./logger.js', () => ({ + logger: { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }, +})); + +import * as config from './config.js'; +import * as db from './db.js'; +import { completePairedExecutionContext } from './paired-execution-context.js'; +import type { PairedTask } from './types.js'; + +function buildPairedTask(overrides: Partial = {}): PairedTask { + return { + id: 'task-1', + chat_jid: 'dc:test', + group_folder: 'ejclaw', + owner_service_id: config.CODEX_MAIN_SERVICE_ID, + reviewer_service_id: config.REVIEWER_SERVICE_ID_FOR_TYPE, + title: null, + source_ref: 'HEAD', + plan_notes: null, + review_requested_at: null, + round_trip_count: 0, + owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: 0, + task_done_then_user_reopen_count: 0, + empty_step_done_streak: 0, + status: 'active', + arbiter_verdict: null, + arbiter_requested_at: null, + completion_reason: null, + created_at: '2026-03-28T00:00:00.000Z', + updated_at: '2026-03-28T00:00:00.000Z', + ...overrides, + }; +} + +describe('paired execution routing loop guards', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(db.getPairedWorkspace).mockReturnValue(undefined); + }); + + it('clears stale owner loop state when reviewer approves normally', () => { + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'in_review', + source_ref: 'approved-ref', + owner_failure_count: 1, + owner_step_done_streak: 3, + finalize_step_done_count: 1, + empty_step_done_streak: 2, + arbiter_verdict: 'proceed', + arbiter_requested_at: '2026-03-28T00:00:01.000Z', + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'reviewer', + status: 'succeeded', + summary: 'TASK_DONE\n승인', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'merge_ready', + source_ref: 'approved-ref', + owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: 0, + empty_step_done_streak: 0, + arbiter_verdict: null, + arbiter_requested_at: null, + }), + ); + }); + + it('clears stale owner loop state when reviewer approval is recovered from a failed run', () => { + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'in_review', + source_ref: 'approved-ref', + owner_step_done_streak: 2, + empty_step_done_streak: 2, + arbiter_verdict: 'proceed', + arbiter_requested_at: '2026-03-28T00:00:01.000Z', + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'reviewer', + status: 'failed', + summary: 'DONE\n승인', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'merge_ready', + owner_step_done_streak: 0, + empty_step_done_streak: 0, + arbiter_verdict: null, + arbiter_requested_at: null, + }), + ); + }); + + it('routes arbiter PROCEED back to reviewer instead of owner ping-pong', () => { + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'in_arbitration', + round_trip_count: config.ARBITER_DEADLOCK_THRESHOLD, + owner_step_done_streak: 3, + finalize_step_done_count: 1, + empty_step_done_streak: 4, + arbiter_requested_at: '2026-03-28T00:00:01.000Z', + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'arbiter', + status: 'succeeded', + summary: 'PROCEED\nReviewer should approve.', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'review_ready', + round_trip_count: 0, + owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: 0, + empty_step_done_streak: 0, + arbiter_verdict: 'proceed', + arbiter_requested_at: null, + }), + ); + }); + + it('routes unknown arbiter verdicts to reviewer as a proceed fallback', () => { + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'in_arbitration', + round_trip_count: config.ARBITER_DEADLOCK_THRESHOLD, + owner_step_done_streak: 3, + empty_step_done_streak: 4, + arbiter_requested_at: '2026-03-28T00:00:01.000Z', + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'arbiter', + status: 'succeeded', + summary: 'No formal verdict, but this should not re-run owner.', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'review_ready', + round_trip_count: 0, + owner_step_done_streak: 0, + empty_step_done_streak: 0, + arbiter_verdict: 'unknown', + arbiter_requested_at: null, + }), + ); + }); + + it('keeps arbiter REVISE on owner flow while clearing stale loop counters', () => { + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'in_arbitration', + round_trip_count: config.ARBITER_DEADLOCK_THRESHOLD, + owner_step_done_streak: 3, + empty_step_done_streak: 4, + arbiter_requested_at: '2026-03-28T00:00:01.000Z', + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'arbiter', + status: 'succeeded', + summary: 'REVISE\nOwner must fix this.', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'active', + owner_step_done_streak: 0, + empty_step_done_streak: 0, + arbiter_verdict: 'revise', + arbiter_requested_at: null, + }), + ); + }); +}); diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index df28dcb..918b167 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -1296,6 +1296,8 @@ describe('paired execution context', () => { buildPairedTask({ status: 'in_review', source_ref: 'stale-ref', + owner_step_done_streak: 2, + empty_step_done_streak: 2, }), ); vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) => @@ -1314,6 +1316,8 @@ describe('paired execution context', () => { expect.objectContaining({ status: 'merge_ready', source_ref: approvedSourceRef, + owner_step_done_streak: 0, + empty_step_done_streak: 0, }), ); }); diff --git a/src/paired-task-status.ts b/src/paired-task-status.ts index c9a03dd..1325552 100644 --- a/src/paired-task-status.ts +++ b/src/paired-task-status.ts @@ -23,7 +23,12 @@ export const ALLOWED_PAIRED_STATUS_TRANSITIONS: Record< merge_ready: new Set(['active', 'arbiter_requested', 'completed']), completed: new Set(), arbiter_requested: new Set(['in_arbitration', 'completed']), - in_arbitration: new Set(['active', 'arbiter_requested', 'completed']), + in_arbitration: new Set([ + 'active', + 'review_ready', + 'arbiter_requested', + 'completed', + ]), }; export function assertPairedTaskStatusTransition(args: { diff --git a/src/paired-verdict.test.ts b/src/paired-verdict.test.ts index 9bd3e52..aa2f221 100644 --- a/src/paired-verdict.test.ts +++ b/src/paired-verdict.test.ts @@ -69,6 +69,7 @@ describe('paired verdict parser', () => { ), ).toBe('reset'); expect(classifyArbiterVerdict('ESCALATE\nblocked')).toBe('escalate'); + expect(classifyArbiterVerdict('CONTINUE\nsame as proceed')).toBe('proceed'); expect(classifyArbiterVerdict('no verdict here')).toBe('unknown'); }); }); diff --git a/src/paired-verdict.ts b/src/paired-verdict.ts index c95eeb3..0b247f0 100644 --- a/src/paired-verdict.ts +++ b/src/paired-verdict.ts @@ -75,10 +75,13 @@ export function classifyArbiterVerdict( if (!cleaned) return 'unknown'; const firstLine = cleaned.split('\n')[0].trim(); const verdictMatch = firstLine.match( - /\*{0,2}(?:VERDICT\s*[:—-]\s*)?(PROCEED|REVISE|RESET|ESCALATE)\*{0,2}/i, + /\*{0,2}(?:VERDICT\s*[:—-]\s*)?(PROCEED|REVISE|RESET|ESCALATE|CONTINUE)\*{0,2}/i, ); if (verdictMatch) { - return verdictMatch[1].toLowerCase() as ArbiterVerdictResult; + const normalized = verdictMatch[1].toLowerCase(); + return normalized === 'continue' + ? 'proceed' + : (normalized as ArbiterVerdictResult); } return 'unknown'; }