From 4d3ab2037817e01f8e9304594bc575cbc6bc139f Mon Sep 17 00:00:00 2001 From: Codex Date: Mon, 22 Jun 2026 19:30:22 +0900 Subject: [PATCH] fix(paired): stop silent halt on reviewer PROCEED + reviewer-unavailable Two causes of the paired room "keeps stopping" symptom: - Reviewer approvals worded as "PROCEED" were parsed as 'continue' (a change request), causing an owner TASK_DONE <-> reviewer PROCEED ping-pong until the deadlock cap. parseReviewerVerdict() now treats a leading PROCEED as approval so the turn finalizes after one round. - When the Codex reviewer was unavailable, the owner's answer was held for review and the user saw nothing. Now the held owner answer is emitted with a "review skipped" notice on reviewer_codex_unavailable. Verified: tsc --noEmit clean; 27 related vitest tests pass. Co-Authored-By: Claude Opus 4.7 --- src/db/bootstrap.test.ts | 4 +- src/message-agent-executor-paired.test.ts | 91 ++++++++++++++++++++ src/message-agent-executor-paired.ts | 33 ++++++- src/paired-execution-context-reviewer.ts | 6 +- src/paired-execution-context-routing.test.ts | 28 ++++++ src/paired-verdict.test.ts | 41 +++++++++ src/paired-verdict.ts | 58 +++++++++++++ 7 files changed, 256 insertions(+), 5 deletions(-) diff --git a/src/db/bootstrap.test.ts b/src/db/bootstrap.test.ts index fb85777..1fd849f 100644 --- a/src/db/bootstrap.test.ts +++ b/src/db/bootstrap.test.ts @@ -149,7 +149,9 @@ describe('initializeDatabaseSchema', () => { }; for (let version = 1; version <= 15; version += 1) { database - .prepare('INSERT INTO schema_migrations (version, name) VALUES (?, ?)') + .prepare( + 'INSERT INTO schema_migrations (version, name) VALUES (?, ?)', + ) .run(version, collidedNames[version] ?? `legacy_${version}`); } // Precondition: the collided database is missing the progress columns. diff --git a/src/message-agent-executor-paired.test.ts b/src/message-agent-executor-paired.test.ts index 8d9c00a..b823051 100644 --- a/src/message-agent-executor-paired.test.ts +++ b/src/message-agent-executor-paired.test.ts @@ -6,6 +6,7 @@ vi.mock('./db.js', () => ({ getLastHumanMessageSender: vi.fn(() => '216851709744513024'), getLatestTurnNumber: vi.fn(() => 0), getPairedTaskById: vi.fn(), + getPairedTurnOutputs: vi.fn(() => []), insertPairedTurnOutput: vi.fn(), refreshPairedTaskExecutionLease: vi.fn(() => true), releasePairedTaskExecutionLease: vi.fn(), @@ -239,6 +240,96 @@ describe('createPairedExecutionLifecycle completion handling', () => { expect(outputs).toEqual([]); }); + it('delivers the held owner answer and a notice when the reviewer Codex is unavailable', async () => { + const outputs: AgentOutput[] = []; + + vi.mocked(db.getPairedTurnOutputs).mockReturnValue([ + { + id: 1, + task_id: 'paired-task-reviewer-down', + turn_number: 1, + role: 'owner', + output_text: 'TASK_DONE\n\n원하시던 .env 위치는 여기에 있습니다.', + created_at: '2026-04-09T00:00:00.000Z', + }, + ]); + + vi.mocked(db.getPairedTaskById).mockReturnValue({ + id: 'paired-task-reviewer-down', + chat_jid: 'group@test', + group_folder: 'test-group', + owner_service_id: 'claude', + reviewer_service_id: 'codex-main', + title: null, + source_ref: 'HEAD', + plan_notes: null, + round_trip_count: 1, + review_requested_at: '2026-04-09T00:00:00.000Z', + status: 'completed', + arbiter_verdict: 'escalate', + arbiter_requested_at: null, + completion_reason: 'reviewer_codex_unavailable', + created_at: '2026-04-09T00:00:00.000Z', + updated_at: '2026-04-09T00:00:01.000Z', + }); + + const lifecycle = createPairedExecutionLifecycle({ + pairedExecutionContext: { + task: { + id: 'paired-task-reviewer-down', + chat_jid: 'group@test', + group_folder: 'test-group', + owner_service_id: 'claude', + reviewer_service_id: 'codex-main', + title: null, + source_ref: 'HEAD', + plan_notes: null, + round_trip_count: 1, + review_requested_at: '2026-04-09T00:00:00.000Z', + status: 'in_review', + arbiter_verdict: null, + arbiter_requested_at: null, + completion_reason: null, + created_at: '2026-04-09T00:00:00.000Z', + updated_at: '2026-04-09T00:00:00.000Z', + }, + workspace: null, + envOverrides: {}, + }, + pairedTurnIdentity: { + turnId: + 'paired-task-reviewer-down:2026-04-09T00:00:00.000Z:reviewer-turn', + taskId: 'paired-task-reviewer-down', + taskUpdatedAt: '2026-04-09T00:00:00.000Z', + intentKind: 'reviewer-turn', + role: 'reviewer', + }, + completedRole: 'reviewer', + chatJid: 'group@test', + runId: 'run-reviewer-down', + enqueueMessageCheck: vi.fn(), + onOutput: async (output) => { + outputs.push(output); + }, + log, + }); + + lifecycle.markStatus('failed'); + lifecycle.updateSummary({ + errorText: + 'auth-expired: All Codex rotation accounts unavailable; re-auth required before launching Codex', + }); + await lifecycle.asyncFinalize(); + + const texts = outputs.map((o) => + o.output?.visibility === 'public' ? o.output.text : o.result, + ); + expect(texts).toEqual([ + '원하시던 .env 위치는 여기에 있습니다.', + 'ℹ️ 리뷰어 일시 사용 불가 — 리뷰 없이 owner 답변으로 진행했습니다.', + ]); + }); + it('releases an owner turn interrupted by a human message without counting an owner failure', async () => { const enqueueMessageCheck = vi.fn(); vi.mocked(db.getPairedTaskById).mockReturnValue({ diff --git a/src/message-agent-executor-paired.ts b/src/message-agent-executor-paired.ts index f508cae..49d4ea0 100644 --- a/src/message-agent-executor-paired.ts +++ b/src/message-agent-executor-paired.ts @@ -5,6 +5,7 @@ import { getLastHumanMessageSender, getLatestTurnNumber, getPairedTaskById, + getPairedTurnOutputs, insertPairedTurnOutput, refreshPairedTaskExecutionLease, releasePairedTaskExecutionLease, @@ -14,7 +15,10 @@ import { completePairedExecutionContext, type PreparedPairedExecutionContext, } from './paired-execution-context.js'; -import { parseVisibleVerdict } from './paired-verdict.js'; +import { + parseVisibleVerdict, + stripLeadingStatusLine, +} from './paired-verdict.js'; import { resolvePairedFollowUpQueueAction } from './message-agent-executor-rules.js'; import { enqueuePairedFollowUpAfterEvent } from './message-runtime-follow-up.js'; import type { PairedTurnIdentity } from './paired-turn-identity.js'; @@ -91,6 +95,17 @@ export function resolveOwnerNextStepNotice( } } +function getLatestOwnerAnswer(taskId: string): string | null { + const outputs = getPairedTurnOutputs(taskId); + for (let i = outputs.length - 1; i >= 0; i -= 1) { + const output = outputs[i]; + if (output.role !== 'owner') continue; + const text = stripLeadingStatusLine(output.output_text ?? '').trim(); + if (text.length > 0) return text; + } + return null; +} + async function notifyPairedCompletionIfNeeded(args: { task: PairedTaskRecord | null | undefined; chatJid: string; @@ -116,6 +131,22 @@ async function notifyPairedCompletionIfNeeded(args: { return; } + // Reviewer (Codex) was unavailable, so the turn ended before the reviewer + // could approve. Instead of stopping silently, deliver the owner's already + // produced answer and tell the user the review was skipped — otherwise the + // user sees nothing at all (the owner answer is held until review completes). + if ( + task.status === 'completed' && + task.completion_reason === 'reviewer_codex_unavailable' + ) { + const ownerAnswer = getLatestOwnerAnswer(task.id); + if (ownerAnswer) { + await emit(ownerAnswer); + } + await emit('ℹ️ 리뷰어 일시 사용 불가 — 리뷰 없이 owner 답변으로 진행했습니다.'); + return; + } + // Owner turn: surface the next routing step so the ending is unambiguous. if (args.completedRole !== 'owner') return; const notice = resolveOwnerNextStepNotice(task); diff --git a/src/paired-execution-context-reviewer.ts b/src/paired-execution-context-reviewer.ts index de93e5f..4a72889 100644 --- a/src/paired-execution-context-reviewer.ts +++ b/src/paired-execution-context-reviewer.ts @@ -12,7 +12,7 @@ import { resolveReviewerFailureSignal, } from './paired-completion-signals.js'; import { resolveCanonicalSourceRef } from './paired-source-ref.js'; -import { parseVisibleVerdict } from './paired-verdict.js'; +import { parseReviewerVerdict } from './paired-verdict.js'; import type { PairedTask } from './types.js'; /** @@ -60,7 +60,7 @@ export function handleFailedReviewerExecution(args: { } if (summary) { - const verdict = parseVisibleVerdict(summary); + const verdict = parseReviewerVerdict(summary); const signal = resolveReviewerFailureSignal({ visibleVerdict: verdict, }); @@ -191,7 +191,7 @@ export function handleReviewerCompletion(args: { }): void { const { task, taskId, summary } = args; const now = new Date().toISOString(); - const verdict = parseVisibleVerdict(summary); + const verdict = parseReviewerVerdict(summary); const signal = resolveReviewerCompletionSignal({ visibleVerdict: verdict, roundTripCount: task.round_trip_count, diff --git a/src/paired-execution-context-routing.test.ts b/src/paired-execution-context-routing.test.ts index c0575e7..1e56722 100644 --- a/src/paired-execution-context-routing.test.ts +++ b/src/paired-execution-context-routing.test.ts @@ -104,6 +104,34 @@ describe('paired execution routing loop guards', () => { ); }); + it('treats a reviewer PROCEED as approval and routes to owner finalize', () => { + // Regression: Codex reviewers approve with an arbiter-style "PROCEED" line. + // It used to parse as 'continue' (a change request), so the reviewer's + // approval bounced the task back to active and owner↔reviewer ping-ponged + // (owner TASK_DONE ↔ reviewer PROCEED) until the deadlock cap. A PROCEED + // approval must move the task to merge_ready so the owner can finalize. + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'in_review', + source_ref: 'approved-ref', + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'reviewer', + status: 'succeeded', + summary: 'PROCEED\n위치와 첨부는 맞습니다.', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'merge_ready', + }), + ); + }); + it('clears stale owner loop state when reviewer approval is recovered from a failed run', () => { vi.mocked(db.getPairedTaskById).mockReturnValue( buildPairedTask({ diff --git a/src/paired-verdict.test.ts b/src/paired-verdict.test.ts index 210c9dd..d05fd30 100644 --- a/src/paired-verdict.test.ts +++ b/src/paired-verdict.test.ts @@ -2,7 +2,9 @@ import { describe, expect, it } from 'vitest'; import { classifyArbiterVerdict, + parseReviewerVerdict, parseVisibleVerdict, + stripLeadingStatusLine, } from './paired-verdict.js'; describe('paired verdict parser', () => { @@ -80,6 +82,45 @@ describe('paired verdict parser', () => { ).toBe('continue'); }); + it('treats a reviewer PROCEED line as an approval (DONE)', () => { + expect(parseReviewerVerdict('PROCEED\n위치와 첨부는 맞습니다.')).toBe('done'); + expect(parseReviewerVerdict('**PROCEED**\n최종 답변 내용 맞습니다.')).toBe( + 'done', + ); + expect( + parseReviewerVerdict( + ['검토 결과를 정리합니다.', 'PROCEED', '확정합니다.'].join('\n'), + ), + ).toBe('done'); + }); + + it('does not upgrade non-approval reviewer verdicts', () => { + // REVISE / RESET / plain prose remain a change request ('continue'). + expect(parseReviewerVerdict('REVISE\n이 부분을 고쳐주세요')).toBe('continue'); + expect(parseReviewerVerdict('RESET\n방향을 다시 잡읍시다')).toBe('continue'); + expect(parseReviewerVerdict('추가 수정이 필요합니다')).toBe('continue'); + // Documented reviewer tokens still parse to their own verdicts. + expect(parseReviewerVerdict('TASK_DONE\n승인합니다')).toBe('task_done'); + expect(parseReviewerVerdict('DONE_WITH_CONCERNS\n우려가 있습니다')).toBe( + 'done_with_concerns', + ); + // "PROCEED" mentioned in prose, not as the leading verdict, is ignored. + expect( + parseReviewerVerdict('리뷰어가 PROCEED 했는지 확인이 필요합니다'), + ).toBe('continue'); + }); + + it('strips a leading status line for direct delivery', () => { + expect(stripLeadingStatusLine('TASK_DONE\n\n실제 답변입니다.')).toBe( + '실제 답변입니다.', + ); + expect(stripLeadingStatusLine('STEP_DONE\n다음 내용')).toBe('다음 내용'); + // No leading status token → text is returned untouched. + expect(stripLeadingStatusLine('그냥 본문입니다.\n둘째 줄')).toBe( + '그냥 본문입니다.\n둘째 줄', + ); + }); + it('classifies arbiter verdicts from leading visible lines', () => { expect(classifyArbiterVerdict('PROCEED\ncontinue')).toBe('proceed'); expect(classifyArbiterVerdict('**VERDICT: REVISE**\nfix it')).toBe( diff --git a/src/paired-verdict.ts b/src/paired-verdict.ts index 6cac2f0..3d4e17b 100644 --- a/src/paired-verdict.ts +++ b/src/paired-verdict.ts @@ -69,6 +69,64 @@ export function parseVisibleVerdict( return 'continue'; } +/** + * Remove a leading status/verdict marker line (e.g. "TASK_DONE", "STEP_DONE") + * from owner output so a held answer can be shown to the user directly without + * the internal status token. Only strips when the very first non-empty line is + * a recognised marker; otherwise returns the text untouched. + */ +export function stripLeadingStatusLine(text: string): string { + const lines = text.split('\n'); + let firstNonEmpty = 0; + while (firstNonEmpty < lines.length && lines[firstNonEmpty].trim() === '') { + firstNonEmpty += 1; + } + if ( + firstNonEmpty >= lines.length || + !parseVisibleVerdictLine(lines[firstNonEmpty].trim()) + ) { + return text; + } + const remainder = lines.slice(firstNonEmpty + 1); + while (remainder.length > 0 && remainder[0].trim() === '') { + remainder.shift(); + } + return remainder.join('\n').trim(); +} + +const REVIEWER_APPROVAL_LINE = + /^\*{0,2}(?:VERDICT\s*[:—-]\s*)?PROCEED(?:\*{0,2})?\b/i; + +/** + * Reviewers — especially the Codex reviewer — approve a turn with an + * arbiter-style "PROCEED" line instead of the documented TASK_DONE. Plain + * parseVisibleVerdict() does not know "PROCEED" and maps it to 'continue', + * which resolveReviewerCompletionSignal() treats as a change request. So an + * *approving* reviewer silently re-opens the owner turn and the two roles + * ping-pong (owner TASK_DONE ↔ reviewer PROCEED) until the deadlock cap fires — + * the user sees a burst of near-identical messages that then just stops. + * + * Treat a leading PROCEED as an approval (equivalent to DONE) so the state + * machine routes to owner finalize and the task actually completes after one + * round instead of looping. Only PROCEED is upgraded; REVISE/RESET/plain text + * stay 'continue' (a genuine change request). + */ +export function parseReviewerVerdict( + summary: string | null | undefined, +): VisibleVerdict { + const verdict = parseVisibleVerdict(summary); + if (verdict !== 'continue' || !summary) return verdict; + const cleaned = stripInternalBlocks(summary).trim(); + if (!cleaned) return verdict; + for (const line of leadingVisibleLines( + cleaned, + VISIBLE_VERDICT_SCAN_LINE_LIMIT, + )) { + if (REVIEWER_APPROVAL_LINE.test(line)) return 'done'; + } + return verdict; +} + export function classifyArbiterVerdict( summary: string | null | undefined, ): ArbiterVerdictResult {