fix(paired): cap consecutive reviewer silent-failure retries
This commit is contained in:
@@ -142,6 +142,7 @@ export function applyBaseSchema(database: Database): void {
|
|||||||
review_requested_at TEXT,
|
review_requested_at TEXT,
|
||||||
round_trip_count INTEGER NOT NULL DEFAULT 0,
|
round_trip_count INTEGER NOT NULL DEFAULT 0,
|
||||||
owner_failure_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,
|
owner_step_done_streak INTEGER NOT NULL DEFAULT 0,
|
||||||
finalize_step_done_count 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,
|
task_done_then_user_reopen_count INTEGER NOT NULL DEFAULT 0,
|
||||||
|
|||||||
@@ -43,6 +43,7 @@ function getExpectedSchemaMigrations(): Array<{
|
|||||||
{ version: 16, name: 'room_skill_overrides' },
|
{ version: 16, name: 'room_skill_overrides' },
|
||||||
{ version: 17, name: 'scheduled_task_room_role' },
|
{ version: 17, name: 'scheduled_task_room_role' },
|
||||||
{ version: 18, name: 'paired_turn_output_attachments' },
|
{ version: 18, name: 'paired_turn_output_attachments' },
|
||||||
|
{ version: 19, name: 'reviewer_failure_count' },
|
||||||
];
|
];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
23
src/db/migrations/019_reviewer-failure-count.ts
Normal file
23
src/db/migrations/019_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: 19,
|
||||||
|
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
|
||||||
|
`);
|
||||||
|
},
|
||||||
|
};
|
||||||
@@ -18,6 +18,7 @@ import { TURN_PROGRESS_TEXT_MIGRATION } from './015_turn-progress-text.js';
|
|||||||
import { ROOM_SKILL_OVERRIDES_MIGRATION } from './016_room-skill-overrides.js';
|
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 { 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 { PAIRED_TURN_OUTPUT_ATTACHMENTS_MIGRATION } from './018_paired-turn-output-attachments.js';
|
||||||
|
import { REVIEWER_FAILURE_COUNT_MIGRATION } from './019_reviewer-failure-count.js';
|
||||||
import type {
|
import type {
|
||||||
SchemaMigrationArgs,
|
SchemaMigrationArgs,
|
||||||
SchemaMigrationDefinition,
|
SchemaMigrationDefinition,
|
||||||
@@ -44,6 +45,7 @@ const ORDERED_SCHEMA_MIGRATIONS: readonly SchemaMigrationDefinition[] = [
|
|||||||
ROOM_SKILL_OVERRIDES_MIGRATION,
|
ROOM_SKILL_OVERRIDES_MIGRATION,
|
||||||
SCHEDULED_TASK_ROOM_ROLE_MIGRATION,
|
SCHEDULED_TASK_ROOM_ROLE_MIGRATION,
|
||||||
PAIRED_TURN_OUTPUT_ATTACHMENTS_MIGRATION,
|
PAIRED_TURN_OUTPUT_ATTACHMENTS_MIGRATION,
|
||||||
|
REVIEWER_FAILURE_COUNT_MIGRATION,
|
||||||
];
|
];
|
||||||
|
|
||||||
function ensureSchemaMigrationsTable(database: Database): void {
|
function ensureSchemaMigrationsTable(database: Database): void {
|
||||||
|
|||||||
@@ -50,6 +50,7 @@ export type PairedTaskUpdates = Partial<
|
|||||||
| 'review_requested_at'
|
| 'review_requested_at'
|
||||||
| 'round_trip_count'
|
| 'round_trip_count'
|
||||||
| 'owner_failure_count'
|
| 'owner_failure_count'
|
||||||
|
| 'reviewer_failure_count'
|
||||||
| 'owner_step_done_streak'
|
| 'owner_step_done_streak'
|
||||||
| 'finalize_step_done_count'
|
| 'finalize_step_done_count'
|
||||||
| 'task_done_then_user_reopen_count'
|
| 'task_done_then_user_reopen_count'
|
||||||
@@ -176,6 +177,7 @@ export function createPairedTaskInDatabase(
|
|||||||
review_requested_at,
|
review_requested_at,
|
||||||
round_trip_count,
|
round_trip_count,
|
||||||
owner_failure_count,
|
owner_failure_count,
|
||||||
|
reviewer_failure_count,
|
||||||
owner_step_done_streak,
|
owner_step_done_streak,
|
||||||
finalize_step_done_count,
|
finalize_step_done_count,
|
||||||
task_done_then_user_reopen_count,
|
task_done_then_user_reopen_count,
|
||||||
@@ -187,7 +189,7 @@ export function createPairedTaskInDatabase(
|
|||||||
created_at,
|
created_at,
|
||||||
updated_at
|
updated_at
|
||||||
)
|
)
|
||||||
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
|
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
|
||||||
`,
|
`,
|
||||||
)
|
)
|
||||||
.run(
|
.run(
|
||||||
@@ -205,6 +207,7 @@ export function createPairedTaskInDatabase(
|
|||||||
task.review_requested_at,
|
task.review_requested_at,
|
||||||
task.round_trip_count,
|
task.round_trip_count,
|
||||||
task.owner_failure_count ?? 0,
|
task.owner_failure_count ?? 0,
|
||||||
|
task.reviewer_failure_count ?? 0,
|
||||||
task.owner_step_done_streak ?? 0,
|
task.owner_step_done_streak ?? 0,
|
||||||
task.finalize_step_done_count ?? 0,
|
task.finalize_step_done_count ?? 0,
|
||||||
task.task_done_then_user_reopen_count ?? 0,
|
task.task_done_then_user_reopen_count ?? 0,
|
||||||
@@ -339,6 +342,10 @@ export function updatePairedTaskInDatabase(
|
|||||||
fields.push('owner_failure_count = ?');
|
fields.push('owner_failure_count = ?');
|
||||||
values.push(updates.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) {
|
if (updates.owner_step_done_streak !== undefined) {
|
||||||
fields.push('owner_step_done_streak = ?');
|
fields.push('owner_step_done_streak = ?');
|
||||||
values.push(updates.owner_step_done_streak);
|
values.push(updates.owner_step_done_streak);
|
||||||
@@ -418,6 +425,10 @@ export function updatePairedTaskIfUnchangedInDatabase(
|
|||||||
fields.push('owner_failure_count = ?');
|
fields.push('owner_failure_count = ?');
|
||||||
values.push(updates.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) {
|
if (updates.owner_step_done_streak !== undefined) {
|
||||||
fields.push('owner_step_done_streak = ?');
|
fields.push('owner_step_done_streak = ?');
|
||||||
values.push(updates.owner_step_done_streak);
|
values.push(updates.owner_step_done_streak);
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ export function requestArbiterOrEscalate(args: {
|
|||||||
review_requested_at?: string | null;
|
review_requested_at?: string | null;
|
||||||
round_trip_count?: number;
|
round_trip_count?: number;
|
||||||
owner_failure_count?: number;
|
owner_failure_count?: number;
|
||||||
|
reviewer_failure_count?: number;
|
||||||
owner_step_done_streak?: number;
|
owner_step_done_streak?: number;
|
||||||
finalize_step_done_count?: number;
|
finalize_step_done_count?: number;
|
||||||
task_done_then_user_reopen_count?: number;
|
task_done_then_user_reopen_count?: number;
|
||||||
|
|||||||
@@ -293,6 +293,7 @@ function maybeAutoTriggerReviewerAfterOwnerCompletion(args: {
|
|||||||
patch: {
|
patch: {
|
||||||
round_trip_count: task.round_trip_count + 1,
|
round_trip_count: task.round_trip_count + 1,
|
||||||
owner_failure_count: 0,
|
owner_failure_count: 0,
|
||||||
|
reviewer_failure_count: 0,
|
||||||
owner_step_done_streak: 0,
|
owner_step_done_streak: 0,
|
||||||
empty_step_done_streak: 0,
|
empty_step_done_streak: 0,
|
||||||
...args.patch,
|
...args.patch,
|
||||||
|
|||||||
@@ -2,7 +2,10 @@ import { ARBITER_DEADLOCK_THRESHOLD } from './config.js';
|
|||||||
import { getPairedWorkspace } from './db.js';
|
import { getPairedWorkspace } from './db.js';
|
||||||
import { logger } from './logger.js';
|
import { logger } from './logger.js';
|
||||||
import { requestArbiterOrEscalate } from './paired-arbiter-request.js';
|
import { requestArbiterOrEscalate } from './paired-arbiter-request.js';
|
||||||
import { transitionPairedTaskStatus } from './paired-task-status.js';
|
import {
|
||||||
|
applyPairedTaskPatch,
|
||||||
|
transitionPairedTaskStatus,
|
||||||
|
} from './paired-task-status.js';
|
||||||
import {
|
import {
|
||||||
resolveReviewerCompletionSignal,
|
resolveReviewerCompletionSignal,
|
||||||
resolveReviewerFailureSignal,
|
resolveReviewerFailureSignal,
|
||||||
@@ -11,6 +14,17 @@ import { resolveCanonicalSourceRef } from './paired-source-ref.js';
|
|||||||
import { parseVisibleVerdict } from './paired-verdict.js';
|
import { parseVisibleVerdict } from './paired-verdict.js';
|
||||||
import type { PairedTask } from './types.js';
|
import type { PairedTask } from './types.js';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Maximum consecutive reviewer executions that may produce no verdict
|
||||||
|
* before we stop the ping-pong loop. Mirrors the owner-side failure cap 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;
|
||||||
|
|
||||||
export function handleFailedReviewerExecution(args: {
|
export function handleFailedReviewerExecution(args: {
|
||||||
task: PairedTask;
|
task: PairedTask;
|
||||||
taskId: string;
|
taskId: string;
|
||||||
@@ -75,6 +89,35 @@ export function handleFailedReviewerExecution(args: {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// 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 =
|
const fallbackStatus =
|
||||||
task.status === 'in_review' || task.status === 'review_ready'
|
task.status === 'in_review' || task.status === 'review_ready'
|
||||||
? 'review_ready'
|
? 'review_ready'
|
||||||
@@ -86,17 +129,30 @@ export function handleFailedReviewerExecution(args: {
|
|||||||
nextStatus: fallbackStatus,
|
nextStatus: fallbackStatus,
|
||||||
expectedUpdatedAt: task.updated_at,
|
expectedUpdatedAt: task.updated_at,
|
||||||
updatedAt: now,
|
updatedAt: now,
|
||||||
});
|
patch: {
|
||||||
logger.warn(
|
reviewer_failure_count: nextFailureCount,
|
||||||
{
|
|
||||||
taskId,
|
|
||||||
role: 'reviewer',
|
|
||||||
previousStatus: task.status,
|
|
||||||
nextStatus: fallbackStatus,
|
|
||||||
},
|
},
|
||||||
'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: {
|
export function handleReviewerCompletion(args: {
|
||||||
@@ -128,6 +184,7 @@ export function handleReviewerCompletion(args: {
|
|||||||
patch: {
|
patch: {
|
||||||
source_ref: approvedSourceRef,
|
source_ref: approvedSourceRef,
|
||||||
owner_failure_count: 0,
|
owner_failure_count: 0,
|
||||||
|
reviewer_failure_count: 0,
|
||||||
owner_step_done_streak: 0,
|
owner_step_done_streak: 0,
|
||||||
finalize_step_done_count: 0,
|
finalize_step_done_count: 0,
|
||||||
empty_step_done_streak: 0,
|
empty_step_done_streak: 0,
|
||||||
@@ -164,6 +221,7 @@ export function handleReviewerCompletion(args: {
|
|||||||
arbiterLogMessage,
|
arbiterLogMessage,
|
||||||
escalateLogMessage,
|
escalateLogMessage,
|
||||||
logContext: { taskId, verdict, summary: summary?.slice(0, 100) },
|
logContext: { taskId, verdict, summary: summary?.slice(0, 100) },
|
||||||
|
patch: { reviewer_failure_count: 0 },
|
||||||
});
|
});
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
@@ -175,6 +233,7 @@ export function handleReviewerCompletion(args: {
|
|||||||
nextStatus: 'active',
|
nextStatus: 'active',
|
||||||
expectedUpdatedAt: task.updated_at,
|
expectedUpdatedAt: task.updated_at,
|
||||||
updatedAt: now,
|
updatedAt: now,
|
||||||
|
patch: { reviewer_failure_count: 0 },
|
||||||
});
|
});
|
||||||
logger.info(
|
logger.info(
|
||||||
{ taskId, verdict },
|
{ taskId, verdict },
|
||||||
|
|||||||
@@ -141,6 +141,7 @@ function createActiveTaskForRoom(args: {
|
|||||||
review_requested_at: null,
|
review_requested_at: null,
|
||||||
round_trip_count: 0,
|
round_trip_count: 0,
|
||||||
owner_failure_count: 0,
|
owner_failure_count: 0,
|
||||||
|
reviewer_failure_count: 0,
|
||||||
owner_step_done_streak: 0,
|
owner_step_done_streak: 0,
|
||||||
finalize_step_done_count: 0,
|
finalize_step_done_count: 0,
|
||||||
task_done_then_user_reopen_count: 0,
|
task_done_then_user_reopen_count: 0,
|
||||||
@@ -474,6 +475,7 @@ export function preparePairedExecutionContext(args: {
|
|||||||
? {
|
? {
|
||||||
round_trip_count: 0,
|
round_trip_count: 0,
|
||||||
owner_failure_count: 0,
|
owner_failure_count: 0,
|
||||||
|
reviewer_failure_count: 0,
|
||||||
owner_step_done_streak: 0,
|
owner_step_done_streak: 0,
|
||||||
empty_step_done_streak: 0,
|
empty_step_done_streak: 0,
|
||||||
}
|
}
|
||||||
@@ -490,6 +492,7 @@ export function preparePairedExecutionContext(args: {
|
|||||||
? {
|
? {
|
||||||
round_trip_count: 0,
|
round_trip_count: 0,
|
||||||
owner_failure_count: 0,
|
owner_failure_count: 0,
|
||||||
|
reviewer_failure_count: 0,
|
||||||
owner_step_done_streak: 0,
|
owner_step_done_streak: 0,
|
||||||
empty_step_done_streak: 0,
|
empty_step_done_streak: 0,
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -62,6 +62,7 @@ export function transitionPairedTaskStatus(args: {
|
|||||||
review_requested_at?: string | null;
|
review_requested_at?: string | null;
|
||||||
round_trip_count?: number;
|
round_trip_count?: number;
|
||||||
owner_failure_count?: number;
|
owner_failure_count?: number;
|
||||||
|
reviewer_failure_count?: number;
|
||||||
owner_step_done_streak?: number;
|
owner_step_done_streak?: number;
|
||||||
finalize_step_done_count?: number;
|
finalize_step_done_count?: number;
|
||||||
task_done_then_user_reopen_count?: number;
|
task_done_then_user_reopen_count?: number;
|
||||||
@@ -110,6 +111,7 @@ export function applyPairedTaskPatch(args: {
|
|||||||
review_requested_at?: string | null;
|
review_requested_at?: string | null;
|
||||||
round_trip_count?: number;
|
round_trip_count?: number;
|
||||||
owner_failure_count?: number;
|
owner_failure_count?: number;
|
||||||
|
reviewer_failure_count?: number;
|
||||||
owner_step_done_streak?: number;
|
owner_step_done_streak?: number;
|
||||||
finalize_step_done_count?: number;
|
finalize_step_done_count?: number;
|
||||||
task_done_then_user_reopen_count?: number;
|
task_done_then_user_reopen_count?: number;
|
||||||
|
|||||||
@@ -128,6 +128,7 @@ export interface PairedTask {
|
|||||||
review_requested_at: string | null;
|
review_requested_at: string | null;
|
||||||
round_trip_count: number;
|
round_trip_count: number;
|
||||||
owner_failure_count?: number | null;
|
owner_failure_count?: number | null;
|
||||||
|
reviewer_failure_count?: number | null;
|
||||||
owner_step_done_streak?: number | null;
|
owner_step_done_streak?: number | null;
|
||||||
finalize_step_done_count?: number | null;
|
finalize_step_done_count?: number | null;
|
||||||
task_done_then_user_reopen_count?: number | null;
|
task_done_then_user_reopen_count?: number | null;
|
||||||
|
|||||||
Reference in New Issue
Block a user