fix(paired): bound arbiter interventions to stop infinite arbiter loop
The owner↔reviewer↔arbiter loop could repeat forever: when the arbiter ruled PROCEED/REVISE/RESET it reset round_trip_count to 0, and nothing tracked how many times the arbiter had already intervened. A re-deadlock re-invoked the arbiter without bound. Add a persistent arbiter_intervention_count (new column + migration 020) that survives the round-trip reset, and a configurable cap ARBITER_MAX_INTERVENTIONS (default 1). Once the arbiter has intervened that many times and the loop still deadlocks, requestArbiterOrEscalate escalates straight to the user instead of re-invoking the arbiter. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -95,6 +95,14 @@ export const AGENT_LANGUAGE = CONFIG.paired.agentLanguage;
|
||||
export const ARBITER_DEADLOCK_THRESHOLD =
|
||||
CONFIG.paired.arbiterDeadlockThreshold;
|
||||
|
||||
/**
|
||||
* Maximum number of times the arbiter may intervene on a single task before the
|
||||
* task is escalated to the user instead of re-invoking the arbiter. This bounds
|
||||
* the owner↔reviewer↔arbiter loop: once the arbiter has ruled this many times
|
||||
* and the loop still deadlocks, a human is asked to step in.
|
||||
*/
|
||||
export const ARBITER_MAX_INTERVENTIONS = CONFIG.paired.arbiterMaxInterventions;
|
||||
|
||||
export function isArbiterEnabled(): boolean {
|
||||
return ARBITER_AGENT_TYPE !== undefined;
|
||||
}
|
||||
|
||||
@@ -244,6 +244,7 @@ export function loadConfig(): AppConfig {
|
||||
),
|
||||
agentLanguage: readText('AGENT_LANGUAGE') ?? '',
|
||||
arbiterDeadlockThreshold: readInteger('ARBITER_DEADLOCK_THRESHOLD', 2),
|
||||
arbiterMaxInterventions: readInteger('ARBITER_MAX_INTERVENTIONS', 1),
|
||||
maxRoundTrips,
|
||||
},
|
||||
models: {
|
||||
|
||||
@@ -57,6 +57,7 @@ export interface AppConfig {
|
||||
forceFreshClaudeReviewerSessionInUnsafeHostMode: boolean;
|
||||
agentLanguage: string;
|
||||
arbiterDeadlockThreshold: number;
|
||||
arbiterMaxInterventions: number;
|
||||
maxRoundTrips: number;
|
||||
};
|
||||
models: {
|
||||
|
||||
@@ -147,6 +147,7 @@ export function applyBaseSchema(database: Database): void {
|
||||
finalize_step_done_count INTEGER NOT NULL DEFAULT 0,
|
||||
task_done_then_user_reopen_count INTEGER NOT NULL DEFAULT 0,
|
||||
empty_step_done_streak INTEGER NOT NULL DEFAULT 0,
|
||||
arbiter_intervention_count INTEGER NOT NULL DEFAULT 0,
|
||||
status TEXT NOT NULL DEFAULT 'active',
|
||||
arbiter_verdict TEXT,
|
||||
arbiter_requested_at TEXT,
|
||||
|
||||
25
src/db/migrations/020_arbiter-intervention-count.ts
Normal file
25
src/db/migrations/020_arbiter-intervention-count.ts
Normal file
@@ -0,0 +1,25 @@
|
||||
import type { Database } from 'bun:sqlite';
|
||||
|
||||
import { tableHasColumn } from './helpers.js';
|
||||
import type { SchemaMigrationDefinition } from './types.js';
|
||||
|
||||
export const ARBITER_INTERVENTION_COUNT_MIGRATION: SchemaMigrationDefinition = {
|
||||
version: 20,
|
||||
name: 'arbiter_intervention_count',
|
||||
apply(database: Database) {
|
||||
if (
|
||||
!tableHasColumn(database, 'paired_tasks', 'arbiter_intervention_count')
|
||||
) {
|
||||
database.exec(`
|
||||
ALTER TABLE paired_tasks
|
||||
ADD COLUMN arbiter_intervention_count INTEGER NOT NULL DEFAULT 0
|
||||
`);
|
||||
}
|
||||
|
||||
database.exec(`
|
||||
UPDATE paired_tasks
|
||||
SET arbiter_intervention_count = 0
|
||||
WHERE arbiter_intervention_count IS NULL
|
||||
`);
|
||||
},
|
||||
};
|
||||
@@ -19,6 +19,7 @@ import { ROOM_SKILL_OVERRIDES_MIGRATION } from './016_room-skill-overrides.js';
|
||||
import { SCHEDULED_TASK_ROOM_ROLE_MIGRATION } from './017_scheduled-task-room-role.js';
|
||||
import { PAIRED_TURN_OUTPUT_ATTACHMENTS_MIGRATION } from './018_paired-turn-output-attachments.js';
|
||||
import { TURN_PROGRESS_TEXT_RECOVERY_MIGRATION } from './019_turn-progress-text-recovery.js';
|
||||
import { ARBITER_INTERVENTION_COUNT_MIGRATION } from './020_arbiter-intervention-count.js';
|
||||
import type {
|
||||
SchemaMigrationArgs,
|
||||
SchemaMigrationDefinition,
|
||||
@@ -46,6 +47,7 @@ const ORDERED_SCHEMA_MIGRATIONS: readonly SchemaMigrationDefinition[] = [
|
||||
SCHEDULED_TASK_ROOM_ROLE_MIGRATION,
|
||||
PAIRED_TURN_OUTPUT_ATTACHMENTS_MIGRATION,
|
||||
TURN_PROGRESS_TEXT_RECOVERY_MIGRATION,
|
||||
ARBITER_INTERVENTION_COUNT_MIGRATION,
|
||||
];
|
||||
|
||||
function ensureSchemaMigrationsTable(database: Database): void {
|
||||
|
||||
@@ -54,6 +54,7 @@ export type PairedTaskUpdates = Partial<
|
||||
| 'finalize_step_done_count'
|
||||
| 'task_done_then_user_reopen_count'
|
||||
| 'empty_step_done_streak'
|
||||
| 'arbiter_intervention_count'
|
||||
| 'status'
|
||||
| 'arbiter_verdict'
|
||||
| 'arbiter_requested_at'
|
||||
@@ -180,6 +181,7 @@ export function createPairedTaskInDatabase(
|
||||
finalize_step_done_count,
|
||||
task_done_then_user_reopen_count,
|
||||
empty_step_done_streak,
|
||||
arbiter_intervention_count,
|
||||
status,
|
||||
arbiter_verdict,
|
||||
arbiter_requested_at,
|
||||
@@ -187,7 +189,7 @@ export function createPairedTaskInDatabase(
|
||||
created_at,
|
||||
updated_at
|
||||
)
|
||||
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
|
||||
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
|
||||
`,
|
||||
)
|
||||
.run(
|
||||
@@ -209,6 +211,7 @@ export function createPairedTaskInDatabase(
|
||||
task.finalize_step_done_count ?? 0,
|
||||
task.task_done_then_user_reopen_count ?? 0,
|
||||
task.empty_step_done_streak ?? 0,
|
||||
task.arbiter_intervention_count ?? 0,
|
||||
task.status,
|
||||
task.arbiter_verdict,
|
||||
task.arbiter_requested_at,
|
||||
@@ -355,6 +358,10 @@ export function updatePairedTaskInDatabase(
|
||||
fields.push('empty_step_done_streak = ?');
|
||||
values.push(updates.empty_step_done_streak);
|
||||
}
|
||||
if (updates.arbiter_intervention_count !== undefined) {
|
||||
fields.push('arbiter_intervention_count = ?');
|
||||
values.push(updates.arbiter_intervention_count);
|
||||
}
|
||||
if (updates.status !== undefined) {
|
||||
fields.push('status = ?');
|
||||
values.push(updates.status);
|
||||
@@ -434,6 +441,10 @@ export function updatePairedTaskIfUnchangedInDatabase(
|
||||
fields.push('empty_step_done_streak = ?');
|
||||
values.push(updates.empty_step_done_streak);
|
||||
}
|
||||
if (updates.arbiter_intervention_count !== undefined) {
|
||||
fields.push('arbiter_intervention_count = ?');
|
||||
values.push(updates.arbiter_intervention_count);
|
||||
}
|
||||
if (updates.status !== undefined) {
|
||||
fields.push('status = ?');
|
||||
values.push(updates.status);
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { isArbiterEnabled } from './config.js';
|
||||
import { ARBITER_MAX_INTERVENTIONS, isArbiterEnabled } from './config.js';
|
||||
import { logger } from './logger.js';
|
||||
import { transitionPairedTaskStatus } from './paired-task-status.js';
|
||||
import type { PairedTaskStatus } from './types.js';
|
||||
@@ -11,6 +11,13 @@ export function requestArbiterOrEscalate(args: {
|
||||
arbiterLogMessage: string;
|
||||
escalateLogMessage: string;
|
||||
logContext?: Record<string, unknown>;
|
||||
/**
|
||||
* How many times the arbiter has already intervened on this task. When this
|
||||
* reaches {@link maxArbiterInterventions} the task is escalated to the user
|
||||
* instead of re-invoking the arbiter, bounding the owner↔reviewer↔arbiter loop.
|
||||
*/
|
||||
arbiterInterventionCount?: number;
|
||||
maxArbiterInterventions?: number;
|
||||
patch?: {
|
||||
title?: string | null;
|
||||
source_ref?: string | null;
|
||||
@@ -22,6 +29,7 @@ export function requestArbiterOrEscalate(args: {
|
||||
finalize_step_done_count?: number;
|
||||
task_done_then_user_reopen_count?: number;
|
||||
empty_step_done_streak?: number;
|
||||
arbiter_intervention_count?: number;
|
||||
arbiter_verdict?: string | null;
|
||||
arbiter_requested_at?: string | null;
|
||||
completion_reason?: string | null;
|
||||
@@ -36,7 +44,10 @@ export function requestArbiterOrEscalate(args: {
|
||||
escalateLogMessage,
|
||||
logContext,
|
||||
} = args;
|
||||
if (isArbiterEnabled()) {
|
||||
const interventionCount = args.arbiterInterventionCount ?? 0;
|
||||
const maxInterventions =
|
||||
args.maxArbiterInterventions ?? ARBITER_MAX_INTERVENTIONS;
|
||||
if (isArbiterEnabled() && interventionCount < maxInterventions) {
|
||||
const transitioned = transitionPairedTaskStatus({
|
||||
taskId,
|
||||
currentStatus,
|
||||
@@ -54,6 +65,33 @@ export function requestArbiterOrEscalate(args: {
|
||||
return transitioned;
|
||||
}
|
||||
|
||||
if (isArbiterEnabled() && interventionCount >= maxInterventions) {
|
||||
const transitioned = transitionPairedTaskStatus({
|
||||
taskId,
|
||||
currentStatus,
|
||||
nextStatus: 'completed',
|
||||
expectedUpdatedAt,
|
||||
updatedAt: now,
|
||||
patch: {
|
||||
...args.patch,
|
||||
arbiter_verdict: 'escalate',
|
||||
arbiter_requested_at: null,
|
||||
completion_reason: 'escalated',
|
||||
},
|
||||
});
|
||||
if (transitioned) {
|
||||
logger.info(
|
||||
{
|
||||
...(logContext ?? { taskId }),
|
||||
arbiterInterventionCount: interventionCount,
|
||||
maxArbiterInterventions: maxInterventions,
|
||||
},
|
||||
'Arbiter intervention budget exhausted — escalating to user instead of re-invoking arbiter',
|
||||
);
|
||||
}
|
||||
return transitioned;
|
||||
}
|
||||
|
||||
const transitioned = transitionPairedTaskStatus({
|
||||
taskId,
|
||||
currentStatus,
|
||||
|
||||
@@ -78,9 +78,19 @@ export function handleArbiterCompletion(args: {
|
||||
const { task, taskId, summary } = args;
|
||||
const now = new Date().toISOString();
|
||||
const arbiterVerdict = classifyArbiterVerdict(summary);
|
||||
// Persist a running count of arbiter interventions so the loop can be bounded.
|
||||
// This must NOT be reset by the round_trip_count reset below — otherwise the
|
||||
// owner↔reviewer loop could re-trigger the arbiter without limit.
|
||||
const nextArbiterInterventionCount =
|
||||
(task.arbiter_intervention_count ?? 0) + 1;
|
||||
|
||||
logger.info(
|
||||
{ taskId, arbiterVerdict, summary: summary?.slice(0, 200) },
|
||||
{
|
||||
taskId,
|
||||
arbiterVerdict,
|
||||
arbiterInterventionCount: nextArbiterInterventionCount,
|
||||
summary: summary?.slice(0, 200),
|
||||
},
|
||||
'Arbiter verdict rendered',
|
||||
);
|
||||
|
||||
@@ -98,6 +108,7 @@ export function handleArbiterCompletion(args: {
|
||||
owner_step_done_streak: 0,
|
||||
finalize_step_done_count: 0,
|
||||
empty_step_done_streak: 0,
|
||||
arbiter_intervention_count: nextArbiterInterventionCount,
|
||||
arbiter_verdict: arbiterVerdict,
|
||||
arbiter_requested_at: null,
|
||||
},
|
||||
@@ -121,12 +132,17 @@ export function handleArbiterCompletion(args: {
|
||||
owner_step_done_streak: 0,
|
||||
finalize_step_done_count: 0,
|
||||
empty_step_done_streak: 0,
|
||||
arbiter_intervention_count: nextArbiterInterventionCount,
|
||||
arbiter_verdict: arbiterVerdict,
|
||||
arbiter_requested_at: null,
|
||||
},
|
||||
});
|
||||
logger.info(
|
||||
{ taskId, arbiterVerdict },
|
||||
{
|
||||
taskId,
|
||||
arbiterVerdict,
|
||||
arbiterInterventionCount: nextArbiterInterventionCount,
|
||||
},
|
||||
'Arbiter requested owner changes — resuming owner flow',
|
||||
);
|
||||
return;
|
||||
|
||||
@@ -67,6 +67,7 @@ export function handleFailedOwnerExecution(args: {
|
||||
currentStatus: task.status,
|
||||
expectedUpdatedAt: task.updated_at,
|
||||
now,
|
||||
arbiterInterventionCount: task.arbiter_intervention_count ?? 0,
|
||||
arbiterLogMessage:
|
||||
'Owner Codex unavailable repeatedly — requesting arbiter',
|
||||
escalateLogMessage:
|
||||
@@ -140,6 +141,7 @@ export function handleFailedOwnerExecution(args: {
|
||||
currentStatus: task.status,
|
||||
expectedUpdatedAt: task.updated_at,
|
||||
now,
|
||||
arbiterInterventionCount: task.arbiter_intervention_count ?? 0,
|
||||
arbiterLogMessage:
|
||||
'Owner failed repeatedly without a visible verdict — requesting arbiter',
|
||||
escalateLogMessage:
|
||||
@@ -237,6 +239,7 @@ function handleOwnerFinalizeCompletion(args: {
|
||||
currentStatus: task.status,
|
||||
expectedUpdatedAt: task.updated_at,
|
||||
now,
|
||||
arbiterInterventionCount: task.arbiter_intervention_count ?? 0,
|
||||
arbiterLogMessage,
|
||||
escalateLogMessage,
|
||||
logContext: {
|
||||
@@ -266,6 +269,7 @@ function handleOwnerFinalizeCompletion(args: {
|
||||
currentStatus: task.status,
|
||||
expectedUpdatedAt: task.updated_at,
|
||||
now,
|
||||
arbiterInterventionCount: task.arbiter_intervention_count ?? 0,
|
||||
arbiterLogMessage:
|
||||
'Owner repeated STEP_DONE during finalize without code changes — requesting arbiter',
|
||||
escalateLogMessage:
|
||||
@@ -464,6 +468,7 @@ export function handleOwnerCompletion(args: {
|
||||
currentStatus: task.status,
|
||||
expectedUpdatedAt: task.updated_at,
|
||||
now,
|
||||
arbiterInterventionCount: task.arbiter_intervention_count ?? 0,
|
||||
arbiterLogMessage: 'Owner blocked/needs_context — requesting arbiter',
|
||||
escalateLogMessage: 'Owner blocked/needs_context — escalating to user',
|
||||
logContext: {
|
||||
@@ -490,6 +495,7 @@ export function handleOwnerCompletion(args: {
|
||||
currentStatus: task.status,
|
||||
expectedUpdatedAt: task.updated_at,
|
||||
now,
|
||||
arbiterInterventionCount: task.arbiter_intervention_count ?? 0,
|
||||
arbiterLogMessage:
|
||||
'Owner repeated STEP_DONE without code changes — requesting arbiter',
|
||||
escalateLogMessage:
|
||||
|
||||
@@ -193,6 +193,7 @@ export function handleReviewerCompletion(args: {
|
||||
arbiterLogMessage,
|
||||
escalateLogMessage,
|
||||
logContext: { taskId, verdict, summary: summary?.slice(0, 100) },
|
||||
arbiterInterventionCount: task.arbiter_intervention_count ?? 0,
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -147,6 +147,7 @@ function buildPairedTask(overrides: Partial<PairedTask> = {}): PairedTask {
|
||||
finalize_step_done_count: 0,
|
||||
task_done_then_user_reopen_count: 0,
|
||||
empty_step_done_streak: 0,
|
||||
arbiter_intervention_count: 0,
|
||||
status: 'active',
|
||||
arbiter_verdict: null,
|
||||
arbiter_requested_at: null,
|
||||
@@ -1224,6 +1225,81 @@ describe('paired execution context', () => {
|
||||
).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('escalates to the user instead of re-invoking the arbiter once the intervention budget is exhausted', () => {
|
||||
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,
|
||||
// Arbiter has already intervened the maximum allowed number of times.
|
||||
arbiter_intervention_count: config.ARBITER_MAX_INTERVENTIONS,
|
||||
}),
|
||||
);
|
||||
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: 'completed',
|
||||
completion_reason: 'escalated',
|
||||
}),
|
||||
);
|
||||
expect(db.updatePairedTask).not.toHaveBeenCalledWith(
|
||||
'task-1',
|
||||
expect.objectContaining({ status: 'arbiter_requested' }),
|
||||
);
|
||||
});
|
||||
|
||||
it('increments the arbiter intervention count and keeps it across the round-trip reset on REVISE', () => {
|
||||
vi.mocked(db.getPairedTaskById).mockReturnValue(
|
||||
buildPairedTask({
|
||||
status: 'in_arbitration',
|
||||
round_trip_count: config.ARBITER_DEADLOCK_THRESHOLD,
|
||||
arbiter_intervention_count: 0,
|
||||
}),
|
||||
);
|
||||
|
||||
completePairedExecutionContext({
|
||||
taskId: 'task-1',
|
||||
role: 'arbiter',
|
||||
status: 'succeeded',
|
||||
summary: 'VERDICT: REVISE\nOwner should address the reviewer feedback.',
|
||||
});
|
||||
|
||||
expect(db.updatePairedTask).toHaveBeenCalledWith(
|
||||
'task-1',
|
||||
expect.objectContaining({
|
||||
status: 'active',
|
||||
round_trip_count: 0,
|
||||
arbiter_intervention_count: 1,
|
||||
arbiter_verdict: 'revise',
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it.each(['BLOCKED', 'NEEDS_CONTEXT'])(
|
||||
'escalates immediately when owner reports %s during finalize without arbiter',
|
||||
(summary) => {
|
||||
|
||||
@@ -145,6 +145,7 @@ function createActiveTaskForRoom(args: {
|
||||
finalize_step_done_count: 0,
|
||||
task_done_then_user_reopen_count: 0,
|
||||
empty_step_done_streak: 0,
|
||||
arbiter_intervention_count: 0,
|
||||
status: 'active',
|
||||
arbiter_verdict: null,
|
||||
arbiter_requested_at: null,
|
||||
@@ -494,6 +495,7 @@ export function preparePairedExecutionContext(args: {
|
||||
owner_failure_count: 0,
|
||||
owner_step_done_streak: 0,
|
||||
empty_step_done_streak: 0,
|
||||
arbiter_intervention_count: 0,
|
||||
}
|
||||
: {}),
|
||||
},
|
||||
@@ -510,6 +512,7 @@ export function preparePairedExecutionContext(args: {
|
||||
owner_failure_count: 0,
|
||||
owner_step_done_streak: 0,
|
||||
empty_step_done_streak: 0,
|
||||
arbiter_intervention_count: 0,
|
||||
}
|
||||
: {}),
|
||||
},
|
||||
|
||||
@@ -66,6 +66,7 @@ export function transitionPairedTaskStatus(args: {
|
||||
finalize_step_done_count?: number;
|
||||
task_done_then_user_reopen_count?: number;
|
||||
empty_step_done_streak?: number;
|
||||
arbiter_intervention_count?: number;
|
||||
arbiter_verdict?: string | null;
|
||||
arbiter_requested_at?: string | null;
|
||||
completion_reason?: string | null;
|
||||
@@ -114,6 +115,7 @@ export function applyPairedTaskPatch(args: {
|
||||
finalize_step_done_count?: number;
|
||||
task_done_then_user_reopen_count?: number;
|
||||
empty_step_done_streak?: number;
|
||||
arbiter_intervention_count?: number;
|
||||
status?: PairedTaskStatus;
|
||||
arbiter_verdict?: string | null;
|
||||
arbiter_requested_at?: string | null;
|
||||
|
||||
@@ -132,6 +132,7 @@ export interface PairedTask {
|
||||
finalize_step_done_count?: number | null;
|
||||
task_done_then_user_reopen_count?: number | null;
|
||||
empty_step_done_streak?: number | null;
|
||||
arbiter_intervention_count?: number | null;
|
||||
status: PairedTaskStatus;
|
||||
arbiter_verdict: string | null;
|
||||
arbiter_requested_at: string | null;
|
||||
|
||||
Reference in New Issue
Block a user