diff --git a/src/paired-execution-context-reviewer.ts b/src/paired-execution-context-reviewer.ts index 0f5c59c..57b64b8 100644 --- a/src/paired-execution-context-reviewer.ts +++ b/src/paired-execution-context-reviewer.ts @@ -209,6 +209,9 @@ export function handleReviewerCompletion(args: { visibleVerdict: verdict, roundTripCount: task.round_trip_count, deadlockThreshold, + // Always finite (independent of arbiter availability) so reviewer STEP_DONE + // ping-pong terminates instead of looping forever when arbiter is disabled. + stepDoneDeadlockThreshold: ARBITER_DEADLOCK_THRESHOLD, }); switch (signal.kind) { diff --git a/src/paired-execution-context-shared.test.ts b/src/paired-execution-context-shared.test.ts index 8f6bb59..9f883cb 100644 --- a/src/paired-execution-context-shared.test.ts +++ b/src/paired-execution-context-shared.test.ts @@ -127,6 +127,7 @@ describe('paired execution context shared verdict helpers', () => { visibleVerdict: parseVisibleVerdict('APPROVE\nverified'), roundTripCount: 2, deadlockThreshold: 2, + stepDoneDeadlockThreshold: 2, }), ).toEqual({ kind: 'request_owner_finalize' }); expect( @@ -134,6 +135,7 @@ describe('paired execution context shared verdict helpers', () => { visibleVerdict: 'task_done', roundTripCount: 0, deadlockThreshold: 3, + stepDoneDeadlockThreshold: 3, }), ).toEqual({ kind: 'request_owner_finalize' }); expect( @@ -141,6 +143,7 @@ describe('paired execution context shared verdict helpers', () => { visibleVerdict: 'step_done', roundTripCount: 0, deadlockThreshold: 3, + stepDoneDeadlockThreshold: 3, }), ).toEqual({ kind: 'request_owner_changes' }); expect( @@ -148,6 +151,7 @@ describe('paired execution context shared verdict helpers', () => { visibleVerdict: 'done', roundTripCount: 0, deadlockThreshold: 3, + stepDoneDeadlockThreshold: 3, }), ).toEqual({ kind: 'request_owner_finalize' }); expect( @@ -155,6 +159,7 @@ describe('paired execution context shared verdict helpers', () => { visibleVerdict: 'continue', roundTripCount: 1, deadlockThreshold: 3, + stepDoneDeadlockThreshold: 3, }), ).toEqual({ kind: 'request_owner_changes' }); expect( @@ -162,26 +167,40 @@ describe('paired execution context shared verdict helpers', () => { visibleVerdict: 'done_with_concerns', roundTripCount: 3, deadlockThreshold: 3, + stepDoneDeadlockThreshold: 3, }), ).toEqual({ kind: 'request_arbiter' }); }); - it('routes reviewer step_done to arbiter once round trips hit the deadlock threshold', () => { - // Below threshold: keep iterating with the owner. + it('routes reviewer step_done to arbiter once round trips hit its own finite threshold', () => { + // Below the step_done threshold: keep iterating with the owner. expect( resolveReviewerCompletionSignal({ visibleVerdict: 'step_done', roundTripCount: 1, deadlockThreshold: 2, + stepDoneDeadlockThreshold: 2, }), ).toEqual({ kind: 'request_owner_changes' }); - // At/above threshold: break the owner⇄reviewer step_done oscillation by - // escalating to the arbiter instead of looping forever. + // At/above the step_done threshold: break the owner⇄reviewer step_done + // oscillation by escalating to the arbiter instead of looping forever. expect( resolveReviewerCompletionSignal({ visibleVerdict: 'step_done', roundTripCount: 2, deadlockThreshold: 2, + stepDoneDeadlockThreshold: 2, + }), + ).toEqual({ kind: 'request_arbiter' }); + // Regression guard: when arbiter is disabled the caller passes + // deadlockThreshold=Infinity, but step_done must STILL terminate via its own + // finite threshold — otherwise the production loop (arbiter off) never ends. + expect( + resolveReviewerCompletionSignal({ + visibleVerdict: 'step_done', + roundTripCount: 2, + deadlockThreshold: Number.POSITIVE_INFINITY, + stepDoneDeadlockThreshold: 2, }), ).toEqual({ kind: 'request_arbiter' }); }); diff --git a/src/paired-execution-context-shared.ts b/src/paired-execution-context-shared.ts index dcd995c..2bb0217 100644 --- a/src/paired-execution-context-shared.ts +++ b/src/paired-execution-context-shared.ts @@ -104,15 +104,28 @@ export function resolveReviewerCompletionSignal(args: { visibleVerdict: VisibleVerdict; roundTripCount: number; deadlockThreshold: number; + stepDoneDeadlockThreshold: number; }): CompletionSignal { - const { visibleVerdict, roundTripCount, deadlockThreshold } = args; + const { + visibleVerdict, + roundTripCount, + deadlockThreshold, + stepDoneDeadlockThreshold, + } = args; switch (visibleVerdict) { case 'task_done': case 'done': return { kind: 'request_owner_finalize' }; case 'step_done': - if (roundTripCount >= deadlockThreshold) { + // Reviewer STEP_DONE while the owner keeps replying TASK_DONE is a + // content-free oscillation, so it must terminate even when no arbiter is + // configured (deadlockThreshold would otherwise be Infinity and never + // trip). Use the always-finite arbiter threshold; once it is hit, + // requestArbiterOrEscalate routes to the arbiter, or escalates to the + // user when no arbiter exists. Productive REVISE/continue disagreement + // keeps using deadlockThreshold below and is intentionally left alone. + if (roundTripCount >= stepDoneDeadlockThreshold) { return { kind: 'request_arbiter' }; } return { kind: 'request_owner_changes' }; diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index ccaf4fc..7628766 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -1444,6 +1444,95 @@ describe('paired execution context', () => { ); }); + it('escalates reviewer STEP_DONE to the user at the threshold when arbiter is disabled', () => { + vi.spyOn(config, 'isArbiterEnabled').mockReturnValue(false); + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'in_review', + round_trip_count: config.ARBITER_DEADLOCK_THRESHOLD, + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'reviewer', + status: 'succeeded', + summary: 'STEP_DONE\n남은 단계가 있어 계속 진행합니다.', + }); + + // Arbiter off: the ping-pong terminates by escalating to the user instead + // of looping back to the owner forever. + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'completed', + completion_reason: 'escalated', + }), + ); + expect(db.updatePairedTask).not.toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'active', + }), + ); + }); + + it('requests arbiter for reviewer STEP_DONE at the threshold when arbiter is enabled', () => { + vi.spyOn(config, 'isArbiterEnabled').mockReturnValue(true); + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'in_review', + round_trip_count: config.ARBITER_DEADLOCK_THRESHOLD, + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'reviewer', + status: 'succeeded', + summary: 'STEP_DONE\n남은 단계가 있어 계속 진행합니다.', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'arbiter_requested', + arbiter_requested_at: expect.any(String), + }), + ); + }); + + it('returns reviewer STEP_DONE to the owner below the threshold when arbiter is disabled', () => { + vi.spyOn(config, 'isArbiterEnabled').mockReturnValue(false); + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'in_review', + round_trip_count: config.ARBITER_DEADLOCK_THRESHOLD - 1, + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'reviewer', + status: 'succeeded', + summary: 'STEP_DONE\n남은 단계가 있어 계속 진행합니다.', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'active', + }), + ); + expect(db.updatePairedTask).not.toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'completed', + completion_reason: 'escalated', + }), + ); + }); + it('keeps reviewer tasks review_ready and increments reviewer_failure_count after the first silent failure', () => { vi.mocked(db.getPairedTaskById).mockReturnValue( buildPairedTask({