From 32fbb00dd26d79ea7664a55d5881803f87a44c6a Mon Sep 17 00:00:00 2001 From: Codex Date: Fri, 29 May 2026 23:40:35 +0900 Subject: [PATCH] fix(paired): cap consecutive reviewer silent-failure retries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/db/base-schema.ts | 1 + src/db/bootstrap.test.ts | 1 + .../migrations/015_reviewer-failure-count.ts | 23 ++++++ src/db/migrations/index.ts | 2 + src/db/paired-state.ts | 13 ++- src/paired-execution-context-owner.ts | 1 + src/paired-execution-context-reviewer.ts | 80 ++++++++++++++++--- src/paired-execution-context-shared.ts | 3 + src/paired-execution-context.test.ts | 30 ++++++- src/paired-execution-context.ts | 3 + src/types.ts | 1 + 11 files changed, 147 insertions(+), 11 deletions(-) create mode 100644 src/db/migrations/015_reviewer-failure-count.ts diff --git a/src/db/base-schema.ts b/src/db/base-schema.ts index 4cae88e..74fe10e 100644 --- a/src/db/base-schema.ts +++ b/src/db/base-schema.ts @@ -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, diff --git a/src/db/bootstrap.test.ts b/src/db/bootstrap.test.ts index b8d3c3c..9425adf 100644 --- a/src/db/bootstrap.test.ts +++ b/src/db/bootstrap.test.ts @@ -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' }, ]; } diff --git a/src/db/migrations/015_reviewer-failure-count.ts b/src/db/migrations/015_reviewer-failure-count.ts new file mode 100644 index 0000000..390f845 --- /dev/null +++ b/src/db/migrations/015_reviewer-failure-count.ts @@ -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 + `); + }, +}; diff --git a/src/db/migrations/index.ts b/src/db/migrations/index.ts index ddd4bb2..0acc3ef 100644 --- a/src/db/migrations/index.ts +++ b/src/db/migrations/index.ts @@ -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 { diff --git a/src/db/paired-state.ts b/src/db/paired-state.ts index 0ebbb20..4b3d004 100644 --- a/src/db/paired-state.ts +++ b/src/db/paired-state.ts @@ -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); diff --git a/src/paired-execution-context-owner.ts b/src/paired-execution-context-owner.ts index f5e622c..4d13106 100644 --- a/src/paired-execution-context-owner.ts +++ b/src/paired-execution-context-owner.ts @@ -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, diff --git a/src/paired-execution-context-reviewer.ts b/src/paired-execution-context-reviewer.ts index af32f46..0f5c59c 100644 --- a/src/paired-execution-context-reviewer.ts +++ b/src/paired-execution-context-reviewer.ts @@ -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 }, diff --git a/src/paired-execution-context-shared.ts b/src/paired-execution-context-shared.ts index 194be9a..b5b51ba 100644 --- a/src/paired-execution-context-shared.ts +++ b/src/paired-execution-context-shared.ts @@ -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; diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index eedbd73..ccaf4fc 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -143,6 +143,7 @@ function buildPairedTask(overrides: Partial = {}): 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({ diff --git a/src/paired-execution-context.ts b/src/paired-execution-context.ts index e8777ea..e331bcb 100644 --- a/src/paired-execution-context.ts +++ b/src/paired-execution-context.ts @@ -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, } diff --git a/src/types.ts b/src/types.ts index cab64be..849652f 100644 --- a/src/types.ts +++ b/src/types.ts @@ -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;