fix(paired): cap consecutive reviewer silent-failure retries
The reviewer (codex) sometimes emits a final result with no visible text. The runtime marks the run failed → handleFailedReviewerExecution preserves status as review_ready → the follow-up scheduler re-queues another reviewer-turn → loop. The existing round_trip / arbiter caps never fire because round_trip_count only advances on owner-side submissions; bot-side reviewer flakes never increment it. Mirror the owner_failure_count pattern: add reviewer_failure_count to paired_tasks, increment on each silent failure, and escalate via requestArbiterOrEscalate once it reaches 2. Reset to 0 on every successful reviewer completion path (PROCEED/REVISE/arbiter/wait-for-user) and when the owner re-submits a fresh review cycle. Repro chat: 1507762222724546560 (stock-adiviser). Task 33968d31-0da2-480c-85d3-7a3999822ab4 logged 11 consecutive reviewer-turn entries with no work_items and round_trip_count stuck at 1.
This commit is contained in:
@@ -121,6 +121,7 @@ export function applyBaseSchema(database: Database): void {
|
||||
review_requested_at TEXT,
|
||||
round_trip_count INTEGER NOT NULL DEFAULT 0,
|
||||
owner_failure_count INTEGER NOT NULL DEFAULT 0,
|
||||
reviewer_failure_count INTEGER NOT NULL DEFAULT 0,
|
||||
owner_step_done_streak INTEGER NOT NULL DEFAULT 0,
|
||||
finalize_step_done_count INTEGER NOT NULL DEFAULT 0,
|
||||
task_done_then_user_reopen_count INTEGER NOT NULL DEFAULT 0,
|
||||
|
||||
@@ -39,6 +39,7 @@ function getExpectedSchemaMigrations(): Array<{
|
||||
{ version: 12, name: 'paired_verdict_and_step_telemetry' },
|
||||
{ version: 13, name: 'message_source_kind' },
|
||||
{ version: 14, name: 'work_item_attachments' },
|
||||
{ version: 15, name: 'reviewer_failure_count' },
|
||||
];
|
||||
}
|
||||
|
||||
|
||||
23
src/db/migrations/015_reviewer-failure-count.ts
Normal file
23
src/db/migrations/015_reviewer-failure-count.ts
Normal file
@@ -0,0 +1,23 @@
|
||||
import type { Database } from 'bun:sqlite';
|
||||
|
||||
import { tableHasColumn } from './helpers.js';
|
||||
import type { SchemaMigrationDefinition } from './types.js';
|
||||
|
||||
export const REVIEWER_FAILURE_COUNT_MIGRATION: SchemaMigrationDefinition = {
|
||||
version: 15,
|
||||
name: 'reviewer_failure_count',
|
||||
apply(database: Database) {
|
||||
if (!tableHasColumn(database, 'paired_tasks', 'reviewer_failure_count')) {
|
||||
database.exec(`
|
||||
ALTER TABLE paired_tasks
|
||||
ADD COLUMN reviewer_failure_count INTEGER NOT NULL DEFAULT 0
|
||||
`);
|
||||
}
|
||||
|
||||
database.exec(`
|
||||
UPDATE paired_tasks
|
||||
SET reviewer_failure_count = 0
|
||||
WHERE reviewer_failure_count IS NULL
|
||||
`);
|
||||
},
|
||||
};
|
||||
@@ -14,6 +14,7 @@ import { OWNER_FAILURE_COUNT_MIGRATION } from './011_owner-failure-count.js';
|
||||
import { PAIRED_VERDICT_AND_STEP_TELEMETRY_MIGRATION } from './012_paired-verdict-and-step-telemetry.js';
|
||||
import { MESSAGE_SOURCE_KIND_MIGRATION } from './013_message-source-kind.js';
|
||||
import { WORK_ITEM_ATTACHMENTS_MIGRATION } from './014_work-item-attachments.js';
|
||||
import { REVIEWER_FAILURE_COUNT_MIGRATION } from './015_reviewer-failure-count.js';
|
||||
import type {
|
||||
SchemaMigrationArgs,
|
||||
SchemaMigrationDefinition,
|
||||
@@ -36,6 +37,7 @@ const ORDERED_SCHEMA_MIGRATIONS: readonly SchemaMigrationDefinition[] = [
|
||||
PAIRED_VERDICT_AND_STEP_TELEMETRY_MIGRATION,
|
||||
MESSAGE_SOURCE_KIND_MIGRATION,
|
||||
WORK_ITEM_ATTACHMENTS_MIGRATION,
|
||||
REVIEWER_FAILURE_COUNT_MIGRATION,
|
||||
];
|
||||
|
||||
function ensureSchemaMigrationsTable(database: Database): void {
|
||||
|
||||
@@ -51,6 +51,7 @@ export type PairedTaskUpdates = Partial<
|
||||
| 'review_requested_at'
|
||||
| 'round_trip_count'
|
||||
| 'owner_failure_count'
|
||||
| 'reviewer_failure_count'
|
||||
| 'owner_step_done_streak'
|
||||
| 'finalize_step_done_count'
|
||||
| 'task_done_then_user_reopen_count'
|
||||
@@ -177,6 +178,7 @@ export function createPairedTaskInDatabase(
|
||||
review_requested_at,
|
||||
round_trip_count,
|
||||
owner_failure_count,
|
||||
reviewer_failure_count,
|
||||
owner_step_done_streak,
|
||||
finalize_step_done_count,
|
||||
task_done_then_user_reopen_count,
|
||||
@@ -188,7 +190,7 @@ export function createPairedTaskInDatabase(
|
||||
created_at,
|
||||
updated_at
|
||||
)
|
||||
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
|
||||
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
|
||||
`,
|
||||
)
|
||||
.run(
|
||||
@@ -206,6 +208,7 @@ export function createPairedTaskInDatabase(
|
||||
task.review_requested_at,
|
||||
task.round_trip_count,
|
||||
task.owner_failure_count ?? 0,
|
||||
task.reviewer_failure_count ?? 0,
|
||||
task.owner_step_done_streak ?? 0,
|
||||
task.finalize_step_done_count ?? 0,
|
||||
task.task_done_then_user_reopen_count ?? 0,
|
||||
@@ -318,6 +321,10 @@ export function updatePairedTaskInDatabase(
|
||||
fields.push('owner_failure_count = ?');
|
||||
values.push(updates.owner_failure_count);
|
||||
}
|
||||
if (updates.reviewer_failure_count !== undefined) {
|
||||
fields.push('reviewer_failure_count = ?');
|
||||
values.push(updates.reviewer_failure_count);
|
||||
}
|
||||
if (updates.owner_step_done_streak !== undefined) {
|
||||
fields.push('owner_step_done_streak = ?');
|
||||
values.push(updates.owner_step_done_streak);
|
||||
@@ -397,6 +404,10 @@ export function updatePairedTaskIfUnchangedInDatabase(
|
||||
fields.push('owner_failure_count = ?');
|
||||
values.push(updates.owner_failure_count);
|
||||
}
|
||||
if (updates.reviewer_failure_count !== undefined) {
|
||||
fields.push('reviewer_failure_count = ?');
|
||||
values.push(updates.reviewer_failure_count);
|
||||
}
|
||||
if (updates.owner_step_done_streak !== undefined) {
|
||||
fields.push('owner_step_done_streak = ?');
|
||||
values.push(updates.owner_step_done_streak);
|
||||
|
||||
@@ -319,6 +319,7 @@ function maybeAutoTriggerReviewerAfterOwnerCompletion(args: {
|
||||
patch: {
|
||||
round_trip_count: task.round_trip_count + 1,
|
||||
owner_failure_count: 0,
|
||||
reviewer_failure_count: 0,
|
||||
owner_step_done_streak: 0,
|
||||
empty_step_done_streak: 0,
|
||||
...args.patch,
|
||||
|
||||
@@ -2,6 +2,7 @@ import { ARBITER_DEADLOCK_THRESHOLD, isArbiterEnabled } from './config.js';
|
||||
import { getPairedWorkspace } from './db.js';
|
||||
import { logger } from './logger.js';
|
||||
import {
|
||||
applyPairedTaskPatch,
|
||||
parseReviewerVisibleVerdict,
|
||||
isUserInputWaitVerdict,
|
||||
requestArbiterOrEscalate,
|
||||
@@ -12,6 +13,17 @@ import {
|
||||
} from './paired-execution-context-shared.js';
|
||||
import type { PairedTask } from './types.js';
|
||||
|
||||
/**
|
||||
* Maximum consecutive reviewer executions that may produce no verdict
|
||||
* before we stop the ping-pong loop. Mirrors `OWNER_FAILURE_ESCALATION_THRESHOLD`
|
||||
* on the owner side and closes the "reviewer silently fails forever" hole:
|
||||
* codex sometimes emits a final result with no visible text, the runtime
|
||||
* marks it failed, the next-turn scheduler re-queues another reviewer-turn,
|
||||
* and the original round_trip_count never increments — so the existing
|
||||
* round-trip / arbiter caps never fire.
|
||||
*/
|
||||
const REVIEWER_FAILURE_ESCALATION_THRESHOLD = 2;
|
||||
|
||||
function isReviewerUnavailableFailure(summary: string | null | undefined) {
|
||||
return /(?:\b429\b|usage limit|rate limit|try again at|purchase more credits)/i.test(
|
||||
summary ?? '',
|
||||
@@ -96,6 +108,35 @@ export function handleFailedReviewerExecution(args: {
|
||||
return;
|
||||
}
|
||||
|
||||
// Reviewer produced no usable verdict and no recognised infrastructure
|
||||
// failure. Track consecutive flakes — after the threshold, stop the
|
||||
// ping-pong loop instead of preserving review_ready forever.
|
||||
const nextFailureCount = (task.reviewer_failure_count ?? 0) + 1;
|
||||
|
||||
if (nextFailureCount >= REVIEWER_FAILURE_ESCALATION_THRESHOLD) {
|
||||
requestArbiterOrEscalate({
|
||||
taskId,
|
||||
currentStatus: task.status,
|
||||
expectedUpdatedAt: task.updated_at,
|
||||
now,
|
||||
arbiterLogMessage:
|
||||
'Reviewer failed repeatedly without a visible verdict — requesting arbiter',
|
||||
escalateLogMessage:
|
||||
'Reviewer failed repeatedly without a visible verdict — escalating to user',
|
||||
logContext: {
|
||||
taskId,
|
||||
role: 'reviewer',
|
||||
previousStatus: task.status,
|
||||
reviewerFailureCount: nextFailureCount,
|
||||
summary: summary?.slice(0, 160),
|
||||
},
|
||||
patch: {
|
||||
reviewer_failure_count: nextFailureCount,
|
||||
},
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
const fallbackStatus =
|
||||
task.status === 'in_review' || task.status === 'review_ready'
|
||||
? 'review_ready'
|
||||
@@ -107,17 +148,30 @@ export function handleFailedReviewerExecution(args: {
|
||||
nextStatus: fallbackStatus,
|
||||
expectedUpdatedAt: task.updated_at,
|
||||
updatedAt: now,
|
||||
});
|
||||
logger.warn(
|
||||
{
|
||||
taskId,
|
||||
role: 'reviewer',
|
||||
previousStatus: task.status,
|
||||
nextStatus: fallbackStatus,
|
||||
patch: {
|
||||
reviewer_failure_count: nextFailureCount,
|
||||
},
|
||||
'Preserved reviewer task in review-ready state after failed execution',
|
||||
);
|
||||
});
|
||||
} else {
|
||||
applyPairedTaskPatch({
|
||||
taskId,
|
||||
expectedUpdatedAt: task.updated_at,
|
||||
updatedAt: now,
|
||||
patch: {
|
||||
reviewer_failure_count: nextFailureCount,
|
||||
},
|
||||
});
|
||||
}
|
||||
logger.warn(
|
||||
{
|
||||
taskId,
|
||||
role: 'reviewer',
|
||||
previousStatus: task.status,
|
||||
reviewerFailureCount: nextFailureCount,
|
||||
summary: summary?.slice(0, 160),
|
||||
},
|
||||
'Preserved reviewer task in review-ready state after failed execution',
|
||||
);
|
||||
}
|
||||
|
||||
export function handleReviewerCompletion(args: {
|
||||
@@ -141,6 +195,7 @@ export function handleReviewerCompletion(args: {
|
||||
updatedAt: now,
|
||||
patch: {
|
||||
completion_reason: 'escalated',
|
||||
reviewer_failure_count: 0,
|
||||
},
|
||||
});
|
||||
logger.info(
|
||||
@@ -170,6 +225,7 @@ export function handleReviewerCompletion(args: {
|
||||
updatedAt: now,
|
||||
patch: {
|
||||
source_ref: approvedSourceRef,
|
||||
reviewer_failure_count: 0,
|
||||
},
|
||||
});
|
||||
logger.info(
|
||||
@@ -201,6 +257,9 @@ export function handleReviewerCompletion(args: {
|
||||
arbiterLogMessage,
|
||||
escalateLogMessage,
|
||||
logContext: { taskId, verdict, summary: summary?.slice(0, 100) },
|
||||
patch: {
|
||||
reviewer_failure_count: 0,
|
||||
},
|
||||
});
|
||||
return;
|
||||
|
||||
@@ -211,6 +270,9 @@ export function handleReviewerCompletion(args: {
|
||||
nextStatus: 'active',
|
||||
expectedUpdatedAt: task.updated_at,
|
||||
updatedAt: now,
|
||||
patch: {
|
||||
reviewer_failure_count: 0,
|
||||
},
|
||||
});
|
||||
logger.info(
|
||||
{ taskId, verdict },
|
||||
|
||||
@@ -270,6 +270,7 @@ export function transitionPairedTaskStatus(args: {
|
||||
review_requested_at?: string | null;
|
||||
round_trip_count?: number;
|
||||
owner_failure_count?: number;
|
||||
reviewer_failure_count?: number;
|
||||
owner_step_done_streak?: number;
|
||||
finalize_step_done_count?: number;
|
||||
task_done_then_user_reopen_count?: number;
|
||||
@@ -318,6 +319,7 @@ export function applyPairedTaskPatch(args: {
|
||||
review_requested_at?: string | null;
|
||||
round_trip_count?: number;
|
||||
owner_failure_count?: number;
|
||||
reviewer_failure_count?: number;
|
||||
owner_step_done_streak?: number;
|
||||
finalize_step_done_count?: number;
|
||||
task_done_then_user_reopen_count?: number;
|
||||
@@ -363,6 +365,7 @@ export function requestArbiterOrEscalate(args: {
|
||||
review_requested_at?: string | null;
|
||||
round_trip_count?: number;
|
||||
owner_failure_count?: number;
|
||||
reviewer_failure_count?: number;
|
||||
owner_step_done_streak?: number;
|
||||
finalize_step_done_count?: number;
|
||||
task_done_then_user_reopen_count?: number;
|
||||
|
||||
@@ -143,6 +143,7 @@ function buildPairedTask(overrides: Partial<PairedTask> = {}): PairedTask {
|
||||
review_requested_at: null,
|
||||
round_trip_count: 0,
|
||||
owner_failure_count: 0,
|
||||
reviewer_failure_count: 0,
|
||||
owner_step_done_streak: 0,
|
||||
finalize_step_done_count: 0,
|
||||
task_done_then_user_reopen_count: 0,
|
||||
@@ -1443,7 +1444,7 @@ describe('paired execution context', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('keeps reviewer tasks review_ready when reviewer execution fails without a terminal verdict', () => {
|
||||
it('keeps reviewer tasks review_ready and increments reviewer_failure_count after the first silent failure', () => {
|
||||
vi.mocked(db.getPairedTaskById).mockReturnValue(
|
||||
buildPairedTask({
|
||||
status: 'in_review',
|
||||
@@ -1461,11 +1462,38 @@ describe('paired execution context', () => {
|
||||
'task-1',
|
||||
expect.objectContaining({
|
||||
status: 'review_ready',
|
||||
reviewer_failure_count: 1,
|
||||
updated_at: expect.any(String),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('requests arbiter after repeated reviewer execution failures without a visible verdict', () => {
|
||||
vi.spyOn(config, 'isArbiterEnabled').mockReturnValue(true);
|
||||
vi.mocked(db.getPairedTaskById).mockReturnValue(
|
||||
buildPairedTask({
|
||||
status: 'review_ready',
|
||||
reviewer_failure_count: 1,
|
||||
}),
|
||||
);
|
||||
|
||||
completePairedExecutionContext({
|
||||
taskId: 'task-1',
|
||||
role: 'reviewer',
|
||||
status: 'failed',
|
||||
summary: 'runtime exploded before verdict',
|
||||
});
|
||||
|
||||
expect(db.updatePairedTask).toHaveBeenCalledWith(
|
||||
'task-1',
|
||||
expect.objectContaining({
|
||||
status: 'arbiter_requested',
|
||||
reviewer_failure_count: 2,
|
||||
arbiter_requested_at: expect.any(String),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('escalates reviewer usage-limit failures instead of retrying forever', () => {
|
||||
vi.mocked(db.getPairedTaskById).mockReturnValue(
|
||||
buildPairedTask({
|
||||
|
||||
@@ -147,6 +147,7 @@ function createActiveTaskForRoom(args: {
|
||||
review_requested_at: null,
|
||||
round_trip_count: 0,
|
||||
owner_failure_count: 0,
|
||||
reviewer_failure_count: 0,
|
||||
owner_step_done_streak: 0,
|
||||
finalize_step_done_count: 0,
|
||||
task_done_then_user_reopen_count: 0,
|
||||
@@ -476,6 +477,7 @@ export function preparePairedExecutionContext(args: {
|
||||
? {
|
||||
round_trip_count: 0,
|
||||
owner_failure_count: 0,
|
||||
reviewer_failure_count: 0,
|
||||
owner_step_done_streak: 0,
|
||||
empty_step_done_streak: 0,
|
||||
}
|
||||
@@ -492,6 +494,7 @@ export function preparePairedExecutionContext(args: {
|
||||
? {
|
||||
round_trip_count: 0,
|
||||
owner_failure_count: 0,
|
||||
reviewer_failure_count: 0,
|
||||
owner_step_done_streak: 0,
|
||||
empty_step_done_streak: 0,
|
||||
}
|
||||
|
||||
@@ -124,6 +124,7 @@ export interface PairedTask {
|
||||
review_requested_at: string | null;
|
||||
round_trip_count: number;
|
||||
owner_failure_count?: number | null;
|
||||
reviewer_failure_count?: number | null;
|
||||
owner_step_done_streak?: number | null;
|
||||
finalize_step_done_count?: number | null;
|
||||
task_done_then_user_reopen_count?: number | null;
|
||||
|
||||
Reference in New Issue
Block a user