fix(paired): bound reviewer STEP_DONE loop even when arbiter is disabled
The previous fix gated the step_done deadlock guard on the same deadlockThreshold every verdict uses — but handleReviewerCompletion passes Infinity for that threshold when no arbiter is configured (isArbiterEnabled()=false, e.g. production with the ARBITER__AGENT_TYPE env typo). So step_done never tripped the guard and the owner TASK_DONE ⇄ reviewer STEP_DONE loop still ran forever. Give step_done its own always-finite threshold (stepDoneDeadlockThreshold = ARBITER_DEADLOCK_THRESHOLD) independent of arbiter availability. Once hit, requestArbiterOrEscalate routes to the arbiter when enabled, or completes the task with completion_reason 'escalated' (user escalation) when not. Productive REVISE/continue disagreement still uses deadlockThreshold and is intentionally left on the existing policy (loops to owner when arbiter is off), matching the existing "returns reviewer change requests to owner ... deadlock threshold" test. Adds a shared regression unit test (deadlockThreshold=Infinity + step_done still escalates) and three integration tests through completePairedExecutionContext: arbiter-off+threshold -> escalated, arbiter-on+threshold -> arbiter_requested, below-threshold -> active. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
This commit is contained in:
@@ -209,6 +209,9 @@ export function handleReviewerCompletion(args: {
|
|||||||
visibleVerdict: verdict,
|
visibleVerdict: verdict,
|
||||||
roundTripCount: task.round_trip_count,
|
roundTripCount: task.round_trip_count,
|
||||||
deadlockThreshold,
|
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) {
|
switch (signal.kind) {
|
||||||
|
|||||||
@@ -127,6 +127,7 @@ describe('paired execution context shared verdict helpers', () => {
|
|||||||
visibleVerdict: parseVisibleVerdict('APPROVE\nverified'),
|
visibleVerdict: parseVisibleVerdict('APPROVE\nverified'),
|
||||||
roundTripCount: 2,
|
roundTripCount: 2,
|
||||||
deadlockThreshold: 2,
|
deadlockThreshold: 2,
|
||||||
|
stepDoneDeadlockThreshold: 2,
|
||||||
}),
|
}),
|
||||||
).toEqual({ kind: 'request_owner_finalize' });
|
).toEqual({ kind: 'request_owner_finalize' });
|
||||||
expect(
|
expect(
|
||||||
@@ -134,6 +135,7 @@ describe('paired execution context shared verdict helpers', () => {
|
|||||||
visibleVerdict: 'task_done',
|
visibleVerdict: 'task_done',
|
||||||
roundTripCount: 0,
|
roundTripCount: 0,
|
||||||
deadlockThreshold: 3,
|
deadlockThreshold: 3,
|
||||||
|
stepDoneDeadlockThreshold: 3,
|
||||||
}),
|
}),
|
||||||
).toEqual({ kind: 'request_owner_finalize' });
|
).toEqual({ kind: 'request_owner_finalize' });
|
||||||
expect(
|
expect(
|
||||||
@@ -141,6 +143,7 @@ describe('paired execution context shared verdict helpers', () => {
|
|||||||
visibleVerdict: 'step_done',
|
visibleVerdict: 'step_done',
|
||||||
roundTripCount: 0,
|
roundTripCount: 0,
|
||||||
deadlockThreshold: 3,
|
deadlockThreshold: 3,
|
||||||
|
stepDoneDeadlockThreshold: 3,
|
||||||
}),
|
}),
|
||||||
).toEqual({ kind: 'request_owner_changes' });
|
).toEqual({ kind: 'request_owner_changes' });
|
||||||
expect(
|
expect(
|
||||||
@@ -148,6 +151,7 @@ describe('paired execution context shared verdict helpers', () => {
|
|||||||
visibleVerdict: 'done',
|
visibleVerdict: 'done',
|
||||||
roundTripCount: 0,
|
roundTripCount: 0,
|
||||||
deadlockThreshold: 3,
|
deadlockThreshold: 3,
|
||||||
|
stepDoneDeadlockThreshold: 3,
|
||||||
}),
|
}),
|
||||||
).toEqual({ kind: 'request_owner_finalize' });
|
).toEqual({ kind: 'request_owner_finalize' });
|
||||||
expect(
|
expect(
|
||||||
@@ -155,6 +159,7 @@ describe('paired execution context shared verdict helpers', () => {
|
|||||||
visibleVerdict: 'continue',
|
visibleVerdict: 'continue',
|
||||||
roundTripCount: 1,
|
roundTripCount: 1,
|
||||||
deadlockThreshold: 3,
|
deadlockThreshold: 3,
|
||||||
|
stepDoneDeadlockThreshold: 3,
|
||||||
}),
|
}),
|
||||||
).toEqual({ kind: 'request_owner_changes' });
|
).toEqual({ kind: 'request_owner_changes' });
|
||||||
expect(
|
expect(
|
||||||
@@ -162,26 +167,40 @@ describe('paired execution context shared verdict helpers', () => {
|
|||||||
visibleVerdict: 'done_with_concerns',
|
visibleVerdict: 'done_with_concerns',
|
||||||
roundTripCount: 3,
|
roundTripCount: 3,
|
||||||
deadlockThreshold: 3,
|
deadlockThreshold: 3,
|
||||||
|
stepDoneDeadlockThreshold: 3,
|
||||||
}),
|
}),
|
||||||
).toEqual({ kind: 'request_arbiter' });
|
).toEqual({ kind: 'request_arbiter' });
|
||||||
});
|
});
|
||||||
|
|
||||||
it('routes reviewer step_done to arbiter once round trips hit the deadlock threshold', () => {
|
it('routes reviewer step_done to arbiter once round trips hit its own finite threshold', () => {
|
||||||
// Below threshold: keep iterating with the owner.
|
// Below the step_done threshold: keep iterating with the owner.
|
||||||
expect(
|
expect(
|
||||||
resolveReviewerCompletionSignal({
|
resolveReviewerCompletionSignal({
|
||||||
visibleVerdict: 'step_done',
|
visibleVerdict: 'step_done',
|
||||||
roundTripCount: 1,
|
roundTripCount: 1,
|
||||||
deadlockThreshold: 2,
|
deadlockThreshold: 2,
|
||||||
|
stepDoneDeadlockThreshold: 2,
|
||||||
}),
|
}),
|
||||||
).toEqual({ kind: 'request_owner_changes' });
|
).toEqual({ kind: 'request_owner_changes' });
|
||||||
// At/above threshold: break the owner⇄reviewer step_done oscillation by
|
// At/above the step_done threshold: break the owner⇄reviewer step_done
|
||||||
// escalating to the arbiter instead of looping forever.
|
// oscillation by escalating to the arbiter instead of looping forever.
|
||||||
expect(
|
expect(
|
||||||
resolveReviewerCompletionSignal({
|
resolveReviewerCompletionSignal({
|
||||||
visibleVerdict: 'step_done',
|
visibleVerdict: 'step_done',
|
||||||
roundTripCount: 2,
|
roundTripCount: 2,
|
||||||
deadlockThreshold: 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' });
|
).toEqual({ kind: 'request_arbiter' });
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -104,15 +104,28 @@ export function resolveReviewerCompletionSignal(args: {
|
|||||||
visibleVerdict: VisibleVerdict;
|
visibleVerdict: VisibleVerdict;
|
||||||
roundTripCount: number;
|
roundTripCount: number;
|
||||||
deadlockThreshold: number;
|
deadlockThreshold: number;
|
||||||
|
stepDoneDeadlockThreshold: number;
|
||||||
}): CompletionSignal {
|
}): CompletionSignal {
|
||||||
const { visibleVerdict, roundTripCount, deadlockThreshold } = args;
|
const {
|
||||||
|
visibleVerdict,
|
||||||
|
roundTripCount,
|
||||||
|
deadlockThreshold,
|
||||||
|
stepDoneDeadlockThreshold,
|
||||||
|
} = args;
|
||||||
|
|
||||||
switch (visibleVerdict) {
|
switch (visibleVerdict) {
|
||||||
case 'task_done':
|
case 'task_done':
|
||||||
case 'done':
|
case 'done':
|
||||||
return { kind: 'request_owner_finalize' };
|
return { kind: 'request_owner_finalize' };
|
||||||
case 'step_done':
|
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_arbiter' };
|
||||||
}
|
}
|
||||||
return { kind: 'request_owner_changes' };
|
return { kind: 'request_owner_changes' };
|
||||||
|
|||||||
@@ -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', () => {
|
it('keeps reviewer tasks review_ready and increments reviewer_failure_count after the first silent failure', () => {
|
||||||
vi.mocked(db.getPairedTaskById).mockReturnValue(
|
vi.mocked(db.getPairedTaskById).mockReturnValue(
|
||||||
buildPairedTask({
|
buildPairedTask({
|
||||||
|
|||||||
Reference in New Issue
Block a user