From 6edf5aebbd3c4cccc1b791d3325a64aeb2fc183a Mon Sep 17 00:00:00 2001 From: Eyejoker Date: Mon, 30 Mar 2026 01:27:58 +0900 Subject: [PATCH] refactor: remove explicit paired review command path --- src/message-agent-executor.test.ts | 14 +- src/message-runtime.test.ts | 186 +----------------- src/paired-execution-context.test.ts | 284 +-------------------------- src/paired-execution-context.ts | 109 ---------- src/paired-review-command.test.ts | 240 ---------------------- src/paired-workspace-manager.test.ts | 220 +-------------------- src/paired-workspace-manager.ts | 2 +- src/service-routing.test.ts | 4 +- src/session-commands.test.ts | 55 ------ src/session-commands.ts | 3 - 10 files changed, 16 insertions(+), 1101 deletions(-) delete mode 100644 src/paired-review-command.test.ts diff --git a/src/message-agent-executor.test.ts b/src/message-agent-executor.test.ts index 2018595..1367bb0 100644 --- a/src/message-agent-executor.test.ts +++ b/src/message-agent-executor.test.ts @@ -14,7 +14,9 @@ vi.mock('./config.js', () => ({ CODEX_MAIN_SERVICE_ID: 'codex-main', CODEX_REVIEW_SERVICE_ID: 'codex-review', DATA_DIR: '/tmp/ejclaw-test-data', + REVIEWER_AGENT_TYPE: 'claude-code', SERVICE_SESSION_SCOPE: 'claude', + isClaudeService: vi.fn(() => true), normalizeServiceId: vi.fn((serviceId: string) => serviceId === 'codex' ? 'codex-main' : serviceId, ), @@ -23,6 +25,7 @@ vi.mock('./config.js', () => ({ vi.mock('./db.js', () => ({ createServiceHandoff: vi.fn(), getAllTasks: vi.fn(() => []), + getLatestOpenPairedTaskForChat: vi.fn(() => undefined), })); vi.mock('./service-routing.js', () => ({ @@ -110,7 +113,6 @@ vi.mock('./memento-client.js', () => ({ vi.mock('./paired-execution-context.js', () => ({ completePairedExecutionContext: vi.fn(), - markRoomReviewReady: vi.fn(), preparePairedExecutionContext: vi.fn(() => undefined), })); @@ -236,9 +238,7 @@ describe('runAgentForGroup room memory', () => { expect(agentRunner.runAgentProcess).toHaveBeenCalledWith( group, expect.objectContaining({ - prompt: expect.stringMatching( - /If you have no user-visible content to send for this turn, output exactly this JSON and nothing else: \{"ejclaw":\{"visibility":"silent"\}\}[\s\S]*If you have already emitted any visible progress, status update, or partial answer earlier in this turn, do not end with the JSON object\. Finish with a short visible final conclusion for the user instead\./, - ), + prompt: 'hello', }), expect.any(Function), undefined, @@ -295,9 +295,7 @@ describe('runAgentForGroup room memory', () => { expect(agentRunner.runAgentProcess).toHaveBeenCalledWith( group, expect.objectContaining({ - prompt: expect.stringMatching( - /If you have already emitted any visible progress, status update, or partial answer earlier in this turn, do not end with the JSON object\. Finish with a short visible final conclusion for the user instead\.[\s\S]*If you have not already emitted any visible progress, status update, or partial answer in this turn and you are only agreeing, mirroring, or restating without adding a concrete correction, risk, missing prerequisite, test gap, or code change, output only the JSON object\./, - ), + prompt: 'hello', }), expect.any(Function), undefined, @@ -438,6 +436,7 @@ describe('runAgentForGroup room memory', () => { pairedExecutionContext.completePairedExecutionContext, ).toHaveBeenCalledWith({ taskId: 'paired-task-1', + role: 'owner', status: 'succeeded', summary: 'ok', }); @@ -515,6 +514,7 @@ describe('runAgentForGroup room memory', () => { pairedExecutionContext.completePairedExecutionContext, ).toHaveBeenCalledWith({ taskId: 'paired-task-1', + role: 'owner', status: 'failed', summary: 'Review snapshot is stale after owner changes. Retry the review once to refresh against the latest owner workspace.', diff --git a/src/message-runtime.test.ts b/src/message-runtime.test.ts index c3a3b4c..823f044 100644 --- a/src/message-runtime.test.ts +++ b/src/message-runtime.test.ts @@ -27,6 +27,7 @@ vi.mock('./config.js', () => ({ SERVICE_SESSION_SCOPE: 'claude', CODEX_MAIN_SERVICE_ID: 'codex-main', CODEX_REVIEW_SERVICE_ID: 'codex-review', + REVIEWER_AGENT_TYPE: 'claude-code', normalizeServiceId: vi.fn((serviceId: string) => serviceId), isClaudeService: vi.fn(() => true), isReviewService: vi.fn(() => false), @@ -36,8 +37,6 @@ vi.mock('./config.js', () => ({ vi.mock('./paired-execution-context.js', () => ({ preparePairedExecutionContext: vi.fn(() => undefined), completePairedExecutionContext: vi.fn(), - markRoomReviewReady: vi.fn(() => null), - formatRoomReviewReadyMessage: vi.fn(() => null), })); vi.mock('./db.js', () => { @@ -118,6 +117,7 @@ vi.mock('./db.js', () => { ), getOpenWorkItem: vi.fn(() => undefined), getPendingServiceHandoffs: vi.fn(() => []), + getLatestOpenPairedTaskForChat: vi.fn(() => undefined), createProducedWorkItem: vi.fn((input) => ({ id: 1, group_folder: input.group_folder, @@ -181,8 +181,6 @@ import { resolveGroupIpcPath } from './group-folder.js'; import { createMessageRuntime } from './message-runtime.js'; import * as outputSuppression from './output-suppression.js'; import * as config from './config.js'; -import * as pairedExecutionContext from './paired-execution-context.js'; -import * as sessionCommands from './session-commands.js'; import type { Channel, RegisteredGroup } from './types.js'; function makeGroup(agentType: 'claude-code' | 'codex'): RegisteredGroup { @@ -219,186 +217,6 @@ describe('createMessageRuntime', () => { vi.mocked(config.isReviewService).mockReturnValue(false); }); - it('surfaces the pending review message through the message-runtime /review path', async () => { - const chatJid = 'group@test'; - const group = { - ...makeGroup('codex'), - workDir: '/repo/canonical', - }; - const channel = makeChannel(chatJid); - const saveState = vi.fn(); - const lastAgentTimestamps: Record = {}; - const pendingMessage = [ - 'Review request recorded, but the owner workspace is not ready yet.', - '- Task: paired-task-1', - 'The task stays review_pending until the owner workspace is prepared.', - ].join('\n'); - - vi.mocked(db.isPairedRoomJid).mockReturnValue(true); - vi.mocked(db.getMessagesSince).mockReturnValue([ - { - id: 'msg-review', - chat_jid: chatJid, - sender: 'me@test', - sender_name: 'Me', - content: '/review', - timestamp: '2026-03-29T00:00:00.000Z', - is_from_me: true, - }, - ]); - const actualSessionCommands = await vi.importActual< - typeof import('./session-commands.js') - >('./session-commands.js'); - vi.mocked(sessionCommands.handleSessionCommand).mockImplementation((opts) => - actualSessionCommands.handleSessionCommand(opts), - ); - vi.mocked(pairedExecutionContext.markRoomReviewReady).mockReturnValue({ - status: 'pending', - task: { - id: 'paired-task-1', - chat_jid: chatJid, - group_folder: group.folder, - owner_service_id: 'claude', - reviewer_service_id: 'codex-main', - title: null, - source_ref: 'HEAD', - plan_notes: null, - round_trip_count: 0, - review_requested_at: '2026-03-29T00:00:00.000Z', - status: 'review_ready', - created_at: '2026-03-29T00:00:00.000Z', - updated_at: '2026-03-29T00:00:00.000Z', - }, - pendingReason: 'owner-workspace-not-ready', - }); - vi.mocked( - pairedExecutionContext.formatRoomReviewReadyMessage, - ).mockReturnValue(pendingMessage); - - const runtime = createMessageRuntime({ - assistantName: 'Andy', - idleTimeout: 1_000, - pollInterval: 1_000, - timezone: 'UTC', - triggerPattern: /^@Andy\b/i, - channels: [channel], - queue: { - registerProcess: vi.fn(), - closeStdin: vi.fn(), - notifyIdle: vi.fn(), - } as any, - getRegisteredGroups: () => ({ [chatJid]: group }), - getSessions: () => ({}), - getLastTimestamp: () => '', - setLastTimestamp: vi.fn(), - getLastAgentTimestamps: () => lastAgentTimestamps, - saveState, - persistSession: vi.fn(), - clearSession: vi.fn(), - }); - - const result = await runtime.processGroupMessages(chatJid, { - runId: 'run-review-pending', - reason: 'messages', - }); - - expect(result).toBe(true); - expect(sessionCommands.handleSessionCommand).toHaveBeenCalled(); - expect(pairedExecutionContext.markRoomReviewReady).toHaveBeenCalled(); - expect( - pairedExecutionContext.formatRoomReviewReadyMessage, - ).toHaveBeenCalled(); - expect(channel.sendMessage).toHaveBeenCalledWith(chatJid, pendingMessage); - }); - - it('surfaces the high-risk plan gate message through the message-runtime /review path', async () => { - const chatJid = 'group@test'; - const group = { - ...makeGroup('codex'), - workDir: '/repo/canonical', - }; - const channel = makeChannel(chatJid); - const saveState = vi.fn(); - const lastAgentTimestamps: Record = {}; - const blockedMessage = [ - 'Plan review is required before formal review for this high-risk task.', - '- Task: paired-task-1', - '- Plan status: pending', - 'Ask the owner to record a plan and have the reviewer approve it before /review.', - ].join('\n'); - - vi.mocked(db.isPairedRoomJid).mockReturnValue(true); - vi.mocked(db.getMessagesSince).mockReturnValue([ - { - id: 'msg-review', - chat_jid: chatJid, - sender: 'me@test', - sender_name: 'Me', - content: '/review', - timestamp: '2026-03-29T00:00:00.000Z', - is_from_me: true, - }, - ]); - const actualSessionCommands = await vi.importActual< - typeof import('./session-commands.js') - >('./session-commands.js'); - vi.mocked(sessionCommands.handleSessionCommand).mockImplementation((opts) => - actualSessionCommands.handleSessionCommand(opts), - ); - vi.mocked(pairedExecutionContext.markRoomReviewReady).mockReturnValue({ - status: 'pending', - task: { - id: 'paired-task-1', - chat_jid: chatJid, - group_folder: group.folder, - owner_service_id: 'claude', - reviewer_service_id: 'codex-main', - title: null, - source_ref: 'HEAD', - plan_notes: null, - round_trip_count: 0, - review_requested_at: null, - status: 'active', - created_at: '2026-03-29T00:00:00.000Z', - updated_at: '2026-03-29T00:00:00.000Z', - }, - pendingReason: 'owner-workspace-not-ready', - }); - vi.mocked( - pairedExecutionContext.formatRoomReviewReadyMessage, - ).mockReturnValue(blockedMessage); - - const runtime = createMessageRuntime({ - assistantName: 'Andy', - idleTimeout: 1_000, - pollInterval: 1_000, - timezone: 'UTC', - triggerPattern: /^@Andy\b/i, - channels: [channel], - queue: { - registerProcess: vi.fn(), - closeStdin: vi.fn(), - notifyIdle: vi.fn(), - } as any, - getRegisteredGroups: () => ({ [chatJid]: group }), - getSessions: () => ({}), - getLastTimestamp: () => '', - setLastTimestamp: vi.fn(), - getLastAgentTimestamps: () => lastAgentTimestamps, - saveState, - persistSession: vi.fn(), - clearSession: vi.fn(), - }); - - const result = await runtime.processGroupMessages(chatJid, { - runId: 'run-review-blocked', - reason: 'messages', - }); - - expect(result).toBe(true); - expect(channel.sendMessage).toHaveBeenCalledWith(chatJid, blockedMessage); - }); - it('ignores generic failure bot messages in paired rooms', async () => { const chatJid = 'group@test'; const group = makeGroup('codex'); diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index 3d24804..1163588 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -33,8 +33,6 @@ vi.mock('./logger.js', () => ({ import * as db from './db.js'; import { completePairedExecutionContext, - formatRoomReviewReadyMessage, - markRoomReviewReady, preparePairedExecutionContext, } from './paired-execution-context.js'; import * as pairedWorkspaceManager from './paired-workspace-manager.js'; @@ -137,78 +135,6 @@ function buildWorkspace( }; } -async function importExecutionContextForService(serviceId: string) { - vi.resetModules(); - - const dbModule = { - createPairedTask: vi.fn(), - getLatestPairedTaskForChat: vi.fn(), - getLatestOpenPairedTaskForChat: vi.fn(), - getPairedTaskById: vi.fn(), - getPairedWorkspace: vi.fn(), - updatePairedTask: vi.fn(), - upsertPairedProject: vi.fn(), - }; - dbModule.getLatestOpenPairedTaskForChat.mockReturnValue(undefined); - dbModule.getPairedTaskById.mockReturnValue(undefined); - dbModule.getPairedWorkspace.mockReturnValue(undefined); - - const pairedWorkspaceManagerModule = { - markPairedTaskReviewReady: vi.fn(), - prepareReviewerWorkspaceForExecution: vi.fn(), - provisionOwnerWorkspaceForPairedTask: vi.fn(), - }; - pairedWorkspaceManagerModule.provisionOwnerWorkspaceForPairedTask.mockReturnValue( - { - id: 'task-1:owner', - task_id: 'task-1', - role: 'owner', - workspace_dir: '/tmp/paired/task-1/owner', - snapshot_source_dir: null, - snapshot_ref: null, - status: 'ready', - snapshot_refreshed_at: null, - created_at: '2026-03-28T00:00:00.000Z', - updated_at: '2026-03-28T00:00:00.000Z', - }, - ); - pairedWorkspaceManagerModule.prepareReviewerWorkspaceForExecution.mockReturnValue( - { - workspace: null, - autoRefreshed: false, - }, - ); - - vi.doMock('./db.js', () => dbModule); - vi.doMock( - './paired-workspace-manager.js', - () => pairedWorkspaceManagerModule, - ); - vi.doMock('./logger.js', () => ({ - logger: { - debug: vi.fn(), - info: vi.fn(), - warn: vi.fn(), - error: vi.fn(), - }, - })); - vi.doMock('./config.js', async () => { - const actual = - await vi.importActual('./config.js'); - return { - ...actual, - SERVICE_ID: serviceId, - }; - }); - - const module = await import('./paired-execution-context.js'); - return { - dbModule, - pairedWorkspaceManagerModule, - markRoomReviewReady: module.markRoomReviewReady, - }; -} - describe('paired execution context', () => { beforeEach(() => { vi.resetAllMocks(); @@ -325,7 +251,7 @@ describe('paired execution context', () => { }); }); - it('does not change task state for a general reviewer turn before /review', () => { + it('does not change task state when the reviewer snapshot is not ready', () => { vi.mocked(db.getLatestOpenPairedTaskForChat).mockReturnValue({ id: 'task-1', chat_jid: 'dc:test', @@ -347,7 +273,7 @@ describe('paired execution context', () => { workspace: null, autoRefreshed: false, blockMessage: - 'Review snapshot is not ready yet. Ask the owner to run /review (or /review-ready) after preparing changes.', + 'Review snapshot is not ready yet. Wait for the owner to complete a turn so the reviewer snapshot can be prepared.', }); const result = preparePairedExecutionContext({ @@ -358,7 +284,7 @@ describe('paired execution context', () => { }); expect(result?.blockMessage).toBe( - 'Review snapshot is not ready yet. Ask the owner to run /review (or /review-ready) after preparing changes.', + 'Review snapshot is not ready yet. Wait for the owner to complete a turn so the reviewer snapshot can be prepared.', ); expect(db.updatePairedTask).not.toHaveBeenCalledWith( 'task-1', @@ -531,208 +457,4 @@ describe('paired execution context', () => { ); }); - it('returns ready for /review on the owner service', async () => { - const { - dbModule, - pairedWorkspaceManagerModule, - markRoomReviewReady: isolatedMarkRoomReviewReady, - } = await importExecutionContextForService('codex-main'); - - dbModule.getLatestOpenPairedTaskForChat.mockReturnValue({ - id: 'task-1', - chat_jid: 'dc:test', - group_folder: group.folder, - owner_service_id: 'codex-main', - reviewer_service_id: 'codex-review', - title: null, - source_ref: 'HEAD', - plan_notes: null, - round_trip_count: 0, - review_requested_at: null, - status: 'active', - created_at: '2026-03-28T00:00:00.000Z', - updated_at: '2026-03-28T00:00:00.000Z', - }); - dbModule.getPairedTaskById.mockReturnValue({ - id: 'task-1', - chat_jid: 'dc:test', - group_folder: group.folder, - owner_service_id: 'codex-main', - reviewer_service_id: 'codex-review', - title: null, - source_ref: 'HEAD', - plan_notes: null, - round_trip_count: 0, - review_requested_at: '2026-03-28T00:00:00.000Z', - status: 'review_ready', - created_at: '2026-03-28T00:00:00.000Z', - updated_at: '2026-03-28T00:00:00.000Z', - }); - pairedWorkspaceManagerModule.markPairedTaskReviewReady.mockReturnValue({ - ownerWorkspace: { - id: 'task-1:owner', - task_id: 'task-1', - role: 'owner', - workspace_dir: '/tmp/paired/task-1/owner', - snapshot_source_dir: null, - snapshot_ref: null, - status: 'ready', - snapshot_refreshed_at: null, - created_at: '2026-03-28T00:00:00.000Z', - updated_at: '2026-03-28T00:00:00.000Z', - }, - reviewerWorkspace: { - id: 'task-1:reviewer', - task_id: 'task-1', - role: 'reviewer', - workspace_dir: '/tmp/paired/task-1/reviewer', - snapshot_source_dir: '/tmp/paired/task-1/owner', - snapshot_ref: 'fingerprint-1', - status: 'ready', - snapshot_refreshed_at: '2026-03-28T00:00:00.000Z', - created_at: '2026-03-28T00:00:00.000Z', - updated_at: '2026-03-28T00:00:00.000Z', - }, - }); - - const result = isolatedMarkRoomReviewReady({ - group, - chatJid: 'dc:test', - roomRoleContext: ownerContext, - }); - - expect( - pairedWorkspaceManagerModule.provisionOwnerWorkspaceForPairedTask, - ).toHaveBeenCalledWith('task-1'); - expect( - pairedWorkspaceManagerModule.markPairedTaskReviewReady, - ).toHaveBeenCalledWith('task-1'); - expect(result?.status).toBe('ready'); - if (!result || result.status !== 'ready') { - throw new Error('expected ready review result'); - } - expect(result.task.status).toBe('review_ready'); - }); - - it('returns pending for /review on the reviewer service before the owner workspace is visible', async () => { - const { - dbModule, - pairedWorkspaceManagerModule, - markRoomReviewReady: isolatedMarkRoomReviewReady, - } = await importExecutionContextForService('codex-review'); - - dbModule.getLatestOpenPairedTaskForChat.mockReturnValue({ - id: 'task-1', - chat_jid: 'dc:test', - group_folder: group.folder, - owner_service_id: 'codex-main', - reviewer_service_id: 'codex-review', - title: null, - source_ref: 'HEAD', - plan_notes: null, - round_trip_count: 0, - review_requested_at: null, - status: 'active', - created_at: '2026-03-28T00:00:00.000Z', - updated_at: '2026-03-28T00:00:00.000Z', - }); - dbModule.getPairedTaskById.mockReturnValue({ - id: 'task-1', - chat_jid: 'dc:test', - group_folder: group.folder, - owner_service_id: 'codex-main', - reviewer_service_id: 'codex-review', - title: null, - source_ref: 'HEAD', - plan_notes: null, - round_trip_count: 0, - review_requested_at: '2026-03-28T00:00:00.000Z', - status: 'review_ready', - created_at: '2026-03-28T00:00:00.000Z', - updated_at: '2026-03-28T00:00:00.000Z', - }); - pairedWorkspaceManagerModule.markPairedTaskReviewReady.mockReturnValue( - null, - ); - - const result = isolatedMarkRoomReviewReady({ - group, - chatJid: 'dc:test', - roomRoleContext: ownerContext, - }); - - expect( - pairedWorkspaceManagerModule.provisionOwnerWorkspaceForPairedTask, - ).not.toHaveBeenCalled(); - expect(dbModule.getPairedWorkspace).toHaveBeenCalledWith('task-1', 'owner'); - expect( - pairedWorkspaceManagerModule.markPairedTaskReviewReady, - ).toHaveBeenCalledWith('task-1'); - expect(result).toEqual({ - status: 'pending', - task: expect.objectContaining({ - id: 'task-1', - status: 'review_ready', - }), - pendingReason: 'owner-workspace-not-ready', - }); - }); - - it('keeps review_ready and returns a pending result when owner workspace is not ready', () => { - vi.mocked(db.getLatestOpenPairedTaskForChat).mockReturnValue({ - id: 'task-1', - chat_jid: 'dc:test', - group_folder: group.folder, - owner_service_id: 'codex-main', - reviewer_service_id: 'codex-review', - title: null, - source_ref: 'HEAD', - plan_notes: null, - round_trip_count: 0, - review_requested_at: null, - status: 'active', - created_at: '2026-03-28T00:00:00.000Z', - updated_at: '2026-03-28T00:00:00.000Z', - }); - vi.mocked(db.getPairedTaskById).mockReturnValue({ - id: 'task-1', - chat_jid: 'dc:test', - group_folder: group.folder, - owner_service_id: 'codex-main', - reviewer_service_id: 'codex-review', - title: null, - source_ref: 'HEAD', - plan_notes: null, - round_trip_count: 0, - review_requested_at: '2026-03-29T00:00:00.000Z', - status: 'review_ready', - created_at: '2026-03-28T00:00:00.000Z', - updated_at: '2026-03-29T00:00:00.000Z', - }); - vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue( - null, - ); - - const result = markRoomReviewReady({ - group, - chatJid: 'dc:test', - roomRoleContext: ownerContext, - }); - - expect(result).toEqual({ - status: 'pending', - task: expect.objectContaining({ - id: 'task-1', - status: 'review_ready', - }), - pendingReason: 'owner-workspace-not-ready', - }); - expect(formatRoomReviewReadyMessage(result)).toBe( - [ - 'Review request recorded, but the owner workspace is not ready yet.', - '- Task: task-1', - 'The task stays review_pending until the owner workspace is prepared.', - ].join('\n'), - ); - }); }); diff --git a/src/paired-execution-context.ts b/src/paired-execution-context.ts index a1192d1..c6ffeba 100644 --- a/src/paired-execution-context.ts +++ b/src/paired-execution-context.ts @@ -5,8 +5,6 @@ import path from 'path'; import { DATA_DIR, - SERVICE_ID, - normalizeServiceId, PAIRED_MAX_ROUND_TRIPS, } from './config.js'; import { @@ -477,110 +475,3 @@ export function completePairedExecutionContext(args: { } } } - -// --------------------------------------------------------------------------- -// markRoomReviewReady -// --------------------------------------------------------------------------- - -export type MarkRoomReviewReadyResult = - | { - status: 'ready'; - task: PairedTask; - ownerWorkspace: PairedWorkspace; - reviewerWorkspace: PairedWorkspace; - } - | { - status: 'pending'; - task: PairedTask; - pendingReason: 'owner-workspace-not-ready'; - } - | null; - -export function markRoomReviewReady(args: { - group: RegisteredGroup; - chatJid: string; - roomRoleContext?: RoomRoleContext; -}): MarkRoomReviewReadyResult { - const { group, chatJid, roomRoleContext } = args; - if (!roomRoleContext || !group.workDir) { - return null; - } - - const task = ensureActiveTask(group, chatJid, roomRoleContext); - if (!task) { - return null; - } - - const isOwnerService = - normalizeServiceId(SERVICE_ID) === task.owner_service_id; - const ownerWorkspace = - (isOwnerService - ? provisionOwnerWorkspaceForPairedTask(task.id) - : getPairedWorkspace(task.id, 'owner')) ?? null; - - if (!ownerWorkspace) { - markPairedTaskReviewReady(task.id); - return { - status: 'pending', - task: getPairedTaskById(task.id) ?? task, - pendingReason: 'owner-workspace-not-ready', - }; - } - - // Update task status and refresh reviewer snapshot - const reviewResult = markPairedTaskReviewReady(task.id); - const latestTask = getPairedTaskById(task.id) ?? task; - - if (reviewResult) { - return { - status: 'ready', - task: latestTask, - ownerWorkspace: reviewResult.ownerWorkspace, - reviewerWorkspace: reviewResult.reviewerWorkspace, - }; - } - - // Snapshot refresh succeeded but result was null — try reading persisted state - const reviewerWorkspace = getPairedWorkspace(task.id, 'reviewer'); - if (!reviewerWorkspace) { - return { - status: 'pending', - task: latestTask, - pendingReason: 'owner-workspace-not-ready', - }; - } - - return { - status: 'ready', - task: latestTask, - ownerWorkspace, - reviewerWorkspace, - }; -} - -// --------------------------------------------------------------------------- -// formatRoomReviewReadyMessage -// --------------------------------------------------------------------------- - -export function formatRoomReviewReadyMessage( - result: MarkRoomReviewReadyResult, -): string | null { - if (!result) { - return null; - } - - if (result.status === 'pending') { - return [ - 'Review request recorded, but the owner workspace is not ready yet.', - `- Task: ${result.task.id}`, - 'The task stays review_pending until the owner workspace is prepared.', - ].join('\n'); - } - - return [ - 'Review snapshot updated.', - `- Task: ${result.task.id}`, - `- Owner workspace: ${result.ownerWorkspace.workspace_dir}`, - `- Reviewer snapshot: ${result.reviewerWorkspace.workspace_dir}`, - ].join('\n'); -} diff --git a/src/paired-review-command.test.ts b/src/paired-review-command.test.ts deleted file mode 100644 index 4eed8a9..0000000 --- a/src/paired-review-command.test.ts +++ /dev/null @@ -1,240 +0,0 @@ -import { execFileSync } from 'child_process'; -import fs from 'fs'; -import path from 'path'; - -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; - -import type { RegisteredGroup, RoomRoleContext } from './types.js'; - -function runGit(args: string[], cwd: string): string { - return execFileSync('git', args, { - cwd, - encoding: 'utf-8', - stdio: ['ignore', 'pipe', 'pipe'], - }).trim(); -} - -async function loadModules() { - const db = await import('./db.js'); - const executionContext = await import('./paired-execution-context.js'); - return { db, executionContext }; -} - -const ownerContext: RoomRoleContext = { - serviceId: 'codex-main', - role: 'owner', - ownerServiceId: 'codex-main', - reviewerServiceId: 'codex-review', - failoverOwner: false, -}; - -const reviewerContext: RoomRoleContext = { - serviceId: 'codex-review', - role: 'reviewer', - ownerServiceId: 'codex-main', - reviewerServiceId: 'codex-review', - failoverOwner: false, -}; - -describe('paired /review command path', () => { - let tempRoot: string; - let previousDataDir: string | undefined; - let previousGroupsDir: string | undefined; - let previousStoreDir: string | undefined; - let previousServiceId: string | undefined; - - beforeEach(() => { - tempRoot = fs.mkdtempSync(path.join('/tmp', 'ejclaw-review-command-')); - previousDataDir = process.env.EJCLAW_DATA_DIR; - previousGroupsDir = process.env.EJCLAW_GROUPS_DIR; - previousStoreDir = process.env.EJCLAW_STORE_DIR; - previousServiceId = process.env.SERVICE_ID; - process.env.EJCLAW_STORE_DIR = path.join(tempRoot, 'shared-store'); - process.env.EJCLAW_GROUPS_DIR = path.join(tempRoot, 'shared-groups'); - vi.resetModules(); - }); - - afterEach(() => { - if (previousDataDir === undefined) delete process.env.EJCLAW_DATA_DIR; - else process.env.EJCLAW_DATA_DIR = previousDataDir; - if (previousGroupsDir === undefined) delete process.env.EJCLAW_GROUPS_DIR; - else process.env.EJCLAW_GROUPS_DIR = previousGroupsDir; - if (previousStoreDir === undefined) delete process.env.EJCLAW_STORE_DIR; - else process.env.EJCLAW_STORE_DIR = previousStoreDir; - if (previousServiceId === undefined) delete process.env.SERVICE_ID; - else process.env.SERVICE_ID = previousServiceId; - fs.rmSync(tempRoot, { recursive: true, force: true }); - }); - - it('records the first owner /review event with the provisioned owner workspace fingerprint', async () => { - const canonicalDir = path.join(tempRoot, 'canonical'); - fs.mkdirSync(canonicalDir, { recursive: true }); - runGit(['init'], canonicalDir); - runGit(['config', 'user.email', 'test@example.com'], canonicalDir); - runGit(['config', 'user.name', 'EJClaw Test'], canonicalDir); - fs.writeFileSync(path.join(canonicalDir, 'README.md'), 'original\n'); - runGit(['add', 'README.md'], canonicalDir); - runGit(['commit', '-m', 'initial'], canonicalDir); - - const group: RegisteredGroup = { - name: 'Paired Room', - folder: 'paired-room', - trigger: '@codex', - added_at: '2026-03-28T00:00:00.000Z', - agentType: 'codex', - workDir: canonicalDir, - }; - - process.env.EJCLAW_DATA_DIR = path.join(tempRoot, 'data-owner'); - process.env.SERVICE_ID = 'codex-main'; - vi.resetModules(); - const { db, executionContext } = await loadModules(); - db.initDatabase(); - - const result = executionContext.markRoomReviewReady({ - group, - chatJid: 'dc:test', - roomRoleContext: ownerContext, - }); - - expect(result?.status).toBe('ready'); - if (!result || result.status !== 'ready') { - throw new Error('expected owner /review to prepare a ready snapshot'); - } - expect(result.reviewerWorkspace.snapshot_ref).toBeTruthy(); - }); - - it('reuses the db owner workspace when reviewer service handles /review', async () => { - const canonicalDir = path.join(tempRoot, 'canonical'); - fs.mkdirSync(canonicalDir, { recursive: true }); - runGit(['init'], canonicalDir); - runGit(['config', 'user.email', 'test@example.com'], canonicalDir); - runGit(['config', 'user.name', 'EJClaw Test'], canonicalDir); - fs.writeFileSync(path.join(canonicalDir, 'README.md'), 'original\n'); - runGit(['add', 'README.md'], canonicalDir); - runGit(['commit', '-m', 'initial'], canonicalDir); - - const group: RegisteredGroup = { - name: 'Paired Room', - folder: 'paired-room', - trigger: '@codex', - added_at: '2026-03-28T00:00:00.000Z', - agentType: 'codex', - workDir: canonicalDir, - }; - - process.env.EJCLAW_DATA_DIR = path.join(tempRoot, 'data-owner'); - process.env.SERVICE_ID = 'codex-main'; - vi.resetModules(); - let { db, executionContext } = await loadModules(); - db.initDatabase(); - - const ownerResult = executionContext.preparePairedExecutionContext({ - group, - chatJid: 'dc:test', - runId: 'run-owner', - roomRoleContext: ownerContext, - }); - - expect(ownerResult?.workspace?.workspace_dir).toBeTruthy(); - const ownerWorkspaceDir = ownerResult!.workspace!.workspace_dir; - fs.writeFileSync( - path.join(ownerWorkspaceDir, 'README.md'), - 'owner change\n', - ); - - process.env.EJCLAW_DATA_DIR = path.join(tempRoot, 'data-review'); - process.env.SERVICE_ID = 'codex-review'; - vi.resetModules(); - ({ db, executionContext } = await loadModules()); - db.initDatabase(); - - const result = executionContext.markRoomReviewReady({ - group, - chatJid: 'dc:test', - roomRoleContext: reviewerContext, - }); - - const reviewerLocalOwnerDir = path.join( - tempRoot, - 'data-review', - 'workspaces', - 'paired-room', - 'tasks', - ownerResult!.task.id, - 'owner', - ); - - expect(result).not.toBeNull(); - expect(result?.status).toBe('ready'); - if (!result || result.status !== 'ready') { - throw new Error('expected reviewer /review to prepare a ready snapshot'); - } - expect(result.ownerWorkspace.workspace_dir).toBe(ownerWorkspaceDir); - expect(result.reviewerWorkspace.snapshot_source_dir).toBe( - ownerWorkspaceDir, - ); - expect( - fs.readFileSync( - path.join(result.reviewerWorkspace.workspace_dir, 'README.md'), - 'utf-8', - ), - ).toBe('owner change\n'); - expect(fs.existsSync(reviewerLocalOwnerDir)).toBe(false); - }); - - it('keeps review_pending when reviewer service handles /review before owner workspace exists', async () => { - const canonicalDir = path.join(tempRoot, 'canonical'); - fs.mkdirSync(canonicalDir, { recursive: true }); - runGit(['init'], canonicalDir); - runGit(['config', 'user.email', 'test@example.com'], canonicalDir); - runGit(['config', 'user.name', 'EJClaw Test'], canonicalDir); - fs.writeFileSync(path.join(canonicalDir, 'README.md'), 'original\n'); - runGit(['add', 'README.md'], canonicalDir); - runGit(['commit', '-m', 'initial'], canonicalDir); - - const group: RegisteredGroup = { - name: 'Paired Room', - folder: 'paired-room', - trigger: '@codex', - added_at: '2026-03-28T00:00:00.000Z', - agentType: 'codex', - workDir: canonicalDir, - }; - - process.env.EJCLAW_DATA_DIR = path.join(tempRoot, 'data-review'); - process.env.SERVICE_ID = 'codex-review'; - vi.resetModules(); - const { db, executionContext } = await loadModules(); - db.initDatabase(); - - const result = executionContext.markRoomReviewReady({ - group, - chatJid: 'dc:test', - roomRoleContext: reviewerContext, - }); - - const task = db.getLatestOpenPairedTaskForChat('dc:test'); - const reviewerLocalOwnerDir = path.join( - tempRoot, - 'data-review', - 'workspaces', - 'paired-room', - 'tasks', - task!.id, - 'owner', - ); - - expect(result).toEqual({ - status: 'pending', - task: expect.objectContaining({ - id: task!.id, - status: 'review_ready', - }), - pendingReason: 'owner-workspace-not-ready', - }); - expect(task?.status).toBe('review_ready'); - expect(task?.review_requested_at).toBeTruthy(); - expect(fs.existsSync(reviewerLocalOwnerDir)).toBe(false); - }); -}); diff --git a/src/paired-workspace-manager.test.ts b/src/paired-workspace-manager.test.ts index 0bdb289..771abbd 100644 --- a/src/paired-workspace-manager.test.ts +++ b/src/paired-workspace-manager.test.ts @@ -134,54 +134,6 @@ describe('paired workspace manager', () => { ).toBeTruthy(); }); - it('keeps review_pending when /review is requested before an owner workspace exists', async () => { - const { db, manager } = await loadModules(); - db._initTestDatabase(); - - const canonicalDir = path.join(tempRoot, 'canonical'); - fs.mkdirSync(canonicalDir, { recursive: true }); - runGit(['init'], canonicalDir); - runGit(['config', 'user.email', 'test@example.com'], canonicalDir); - runGit(['config', 'user.name', 'EJClaw Test'], canonicalDir); - fs.writeFileSync(path.join(canonicalDir, 'README.md'), 'original\n'); - runGit(['add', 'README.md'], canonicalDir); - runGit(['commit', '-m', 'initial'], canonicalDir); - - const now = '2026-03-28T00:00:00.000Z'; - db.upsertPairedProject({ - chat_jid: 'dc:test', - group_folder: 'paired-room', - canonical_work_dir: canonicalDir, - created_at: now, - updated_at: now, - }); - db.createPairedTask({ - id: 'paired-task-pending', - chat_jid: 'dc:test', - group_folder: 'paired-room', - owner_service_id: 'codex-main', - reviewer_service_id: 'codex-review', - title: null, - source_ref: 'HEAD', - plan_notes: null, - round_trip_count: 0, - review_requested_at: null, - status: 'active', - created_at: now, - updated_at: now, - }); - - const result = manager.markPairedTaskReviewReady('paired-task-pending'); - - expect(result).toBeNull(); - expect(db.getPairedTaskById('paired-task-pending')?.status).toBe( - 'review_pending', - ); - expect( - db.getPairedTaskById('paired-task-pending')?.review_requested_at, - ).toBeTruthy(); - }); - it('uses the shared DB owner workspace across service-local data dirs', async () => { const canonicalDir = path.join(tempRoot, 'canonical'); fs.mkdirSync(canonicalDir, { recursive: true }); @@ -541,62 +493,6 @@ describe('paired workspace manager', () => { ).toBe('EXAMPLE=1\n'); }); - it('does not auto-refresh a missing reviewer snapshot while the task is still draft', async () => { - const { db, manager } = await loadModules(); - db._initTestDatabase(); - - const canonicalDir = path.join(tempRoot, 'canonical'); - fs.mkdirSync(canonicalDir, { recursive: true }); - runGit(['init'], canonicalDir); - runGit(['config', 'user.email', 'test@example.com'], canonicalDir); - runGit(['config', 'user.name', 'EJClaw Test'], canonicalDir); - fs.writeFileSync(path.join(canonicalDir, 'README.md'), 'base\n'); - runGit(['add', 'README.md'], canonicalDir); - runGit(['commit', '-m', 'initial'], canonicalDir); - - const now = '2026-03-28T00:00:00.000Z'; - db.upsertPairedProject({ - chat_jid: 'dc:test', - group_folder: 'paired-room', - canonical_work_dir: canonicalDir, - created_at: now, - updated_at: now, - }); - db.createPairedTask({ - id: 'paired-task-5', - chat_jid: 'dc:test', - group_folder: 'paired-room', - owner_service_id: 'codex-main', - reviewer_service_id: 'codex-review', - title: null, - source_ref: 'HEAD', - plan_notes: null, - round_trip_count: 0, - review_requested_at: null, - status: 'active', - created_at: now, - updated_at: now, - }); - - const ownerWorkspace = - manager.provisionOwnerWorkspaceForPairedTask('paired-task-5'); - fs.writeFileSync( - path.join(ownerWorkspace.workspace_dir, 'README.md'), - 'owner change\n', - ); - - const result = manager.prepareReviewerWorkspaceForExecution( - db.getPairedTaskById('paired-task-5')!, - ); - - expect(result.workspace).toBeNull(); - expect(result.autoRefreshed).toBe(false); - expect(result.blockMessage).toBe( - 'Review snapshot is not ready yet. Ask the owner to run /review (or /review-ready) after preparing changes.', - ); - expect(db.getPairedTaskById('paired-task-5')?.status).toBe('draft'); - }); - it('auto-refreshes a missing reviewer snapshot after an explicit review request', async () => { const { db, manager } = await loadModules(); db._initTestDatabase(); @@ -659,70 +555,6 @@ describe('paired workspace manager', () => { ); }); - it('auto-refreshes once the owner workspace appears after review_pending was already recorded', async () => { - const { db, manager } = await loadModules(); - db._initTestDatabase(); - - const canonicalDir = path.join(tempRoot, 'canonical'); - fs.mkdirSync(canonicalDir, { recursive: true }); - runGit(['init'], canonicalDir); - runGit(['config', 'user.email', 'test@example.com'], canonicalDir); - runGit(['config', 'user.name', 'EJClaw Test'], canonicalDir); - fs.writeFileSync(path.join(canonicalDir, 'README.md'), 'base\n'); - runGit(['add', 'README.md'], canonicalDir); - runGit(['commit', '-m', 'initial'], canonicalDir); - - const now = '2026-03-28T00:00:00.000Z'; - db.upsertPairedProject({ - chat_jid: 'dc:test', - group_folder: 'paired-room', - canonical_work_dir: canonicalDir, - created_at: now, - updated_at: now, - }); - db.createPairedTask({ - id: 'paired-task-5c', - chat_jid: 'dc:test', - group_folder: 'paired-room', - owner_service_id: 'codex-main', - reviewer_service_id: 'codex-review', - title: null, - source_ref: 'HEAD', - plan_notes: null, - round_trip_count: 0, - review_requested_at: null, - status: 'active', - created_at: now, - updated_at: now, - }); - - expect(manager.markPairedTaskReviewReady('paired-task-5c')).toBeNull(); - expect(db.getPairedTaskById('paired-task-5c')?.status).toBe( - 'review_pending', - ); - - const ownerWorkspace = - manager.provisionOwnerWorkspaceForPairedTask('paired-task-5c'); - fs.writeFileSync( - path.join(ownerWorkspace.workspace_dir, 'README.md'), - 'owner ready now\n', - ); - - const result = manager.prepareReviewerWorkspaceForExecution( - db.getPairedTaskById('paired-task-5c')!, - ); - - expect(result.autoRefreshed).toBe(true); - expect(result.blockMessage).toBeUndefined(); - expect( - fs.readFileSync( - path.join(result.workspace!.workspace_dir, 'README.md'), - 'utf-8', - ), - ).toBe('owner ready now\n'); - expect(db.getPairedTaskById('paired-task-5c')?.status).toBe('review_ready'); - }); - it('blocks once when an in-review snapshot becomes stale, then refreshes on retry', async () => { const { db, manager } = await loadModules(); db._initTestDatabase(); @@ -786,9 +618,7 @@ describe('paired workspace manager', () => { expect(blocked.blockMessage).toBe( 'Review snapshot is stale after owner changes. Retry the review once to refresh against the latest owner workspace.', ); - expect(db.getPairedTaskById('paired-task-6')?.status).toBe( - 'review_pending', - ); + expect(db.getPairedTaskById('paired-task-6')?.status).toBe('review_ready'); expect( db.getPairedTaskById('paired-task-6')?.review_requested_at, ).toBeTruthy(); @@ -812,52 +642,4 @@ describe('paired workspace manager', () => { db.getPairedTaskById('paired-task-6')?.review_requested_at, ).toBeTruthy(); }); - it('blocks reviewer execution for a high-risk task until the plan is approved', async () => { - const { db, manager } = await loadModules(); - db._initTestDatabase(); - - const canonicalDir = path.join(tempRoot, 'canonical'); - fs.mkdirSync(canonicalDir, { recursive: true }); - runGit(['init'], canonicalDir); - runGit(['config', 'user.email', 'test@example.com'], canonicalDir); - runGit(['config', 'user.name', 'EJClaw Test'], canonicalDir); - fs.writeFileSync(path.join(canonicalDir, 'README.md'), 'base\n'); - runGit(['add', 'README.md'], canonicalDir); - runGit(['commit', '-m', 'initial'], canonicalDir); - - const now = '2026-03-28T00:00:00.000Z'; - db.upsertPairedProject({ - chat_jid: 'dc:test', - group_folder: 'paired-room', - canonical_work_dir: canonicalDir, - created_at: now, - updated_at: now, - }); - db.createPairedTask({ - id: 'paired-task-plan-gate', - chat_jid: 'dc:test', - group_folder: 'paired-room', - owner_service_id: 'codex-main', - reviewer_service_id: 'codex-review', - title: null, - source_ref: 'HEAD', - plan_notes: null, - round_trip_count: 0, - review_requested_at: null, - status: 'active', - created_at: now, - updated_at: now, - }); - - const result = manager.prepareReviewerWorkspaceForExecution( - db.getPairedTaskById('paired-task-plan-gate')!, - ); - - expect(result).toEqual({ - workspace: null, - autoRefreshed: false, - blockMessage: - 'Plan review is required before formal review for this high-risk task.', - }); - }); }); diff --git a/src/paired-workspace-manager.ts b/src/paired-workspace-manager.ts index 26b1673..dd75cd8 100644 --- a/src/paired-workspace-manager.ts +++ b/src/paired-workspace-manager.ts @@ -18,7 +18,7 @@ import type { PairedTask, PairedWorkspace } from './types.js'; const REVIEWER_SNAPSHOT_STALE_BLOCK_MESSAGE = 'Review snapshot is stale after owner changes. Retry the review once to refresh against the latest owner workspace.'; const REVIEWER_SNAPSHOT_NOT_READY_BLOCK_MESSAGE = - 'Review snapshot is not ready yet. Ask the owner to run /review (or /review-ready) after preparing changes.'; + 'Review snapshot is not ready yet. Wait for the owner to complete a turn so the reviewer snapshot can be prepared.'; const REVIEWER_SNAPSHOT_DENY_SEGMENTS = new Set([ '.git', '.claude', diff --git a/src/service-routing.test.ts b/src/service-routing.test.ts index 6d384e4..97da7fd 100644 --- a/src/service-routing.test.ts +++ b/src/service-routing.test.ts @@ -69,8 +69,8 @@ describe('service-routing failover leases', () => { expect(getEffectiveChannelLease('dc:paired')).toMatchObject({ chat_jid: 'dc:paired', - owner_service_id: 'claude', - reviewer_service_id: 'codex-main', + owner_service_id: 'codex-main', + reviewer_service_id: 'claude', explicit: false, }); }); diff --git a/src/session-commands.test.ts b/src/session-commands.test.ts index 8db23df..b3a69d8 100644 --- a/src/session-commands.test.ts +++ b/src/session-commands.test.ts @@ -23,20 +23,6 @@ describe('extractSessionCommand', () => { expect(extractSessionCommand('/clear', trigger)).toBe('/clear'); }); - it('detects bare /review', () => { - expect(extractSessionCommand('/review', trigger)).toBe('/review'); - }); - - it('detects bare /deploy-complete', () => { - expect(extractSessionCommand('/deploy-complete', trigger)).toBe( - '/deploy-complete', - ); - }); - - it('normalizes /review-ready to /review', () => { - expect(extractSessionCommand('/review-ready', trigger)).toBe('/review'); - }); - it('rejects /compact with extra text', () => { expect(extractSessionCommand('/compact now please', trigger)).toBeNull(); }); @@ -97,47 +83,6 @@ describe('isSessionCommandControlMessage', () => { ).toBe(true); }); - it('matches multiline review snapshot output', () => { - expect( - isSessionCommandControlMessage( - 'Review snapshot updated.\n- Task: task-1', - ), - ).toBe(true); - }); - - it('matches pending review request output', () => { - expect( - isSessionCommandControlMessage( - [ - 'Review request recorded, but the owner workspace is not ready yet.', - '- Task: task-1', - ].join('\n'), - ), - ).toBe(true); - }); - - it('matches plan-gate control output', () => { - expect( - isSessionCommandControlMessage( - 'Plan review is required before formal review for this high-risk task.\n- Task: task-1', - ), - ).toBe(true); - }); - - it('matches plan artifact control output', () => { - expect( - isSessionCommandControlMessage('Plan recorded.\n- Task: task-1'), - ).toBe(true); - }); - - it('matches deployment finalized output', () => { - expect( - isSessionCommandControlMessage( - 'Deployment finalized.\n- Task: task-1\n- Checkpoint: abc123', - ), - ).toBe(true); - }); - it('does not match regular bot conversation', () => { expect( isSessionCommandControlMessage( diff --git a/src/session-commands.ts b/src/session-commands.ts index 944184e..a6d05c5 100644 --- a/src/session-commands.ts +++ b/src/session-commands.ts @@ -10,9 +10,6 @@ const SESSION_COMMAND_CONTROL_PATTERNS = [ /^Failed to process messages before \/compact\. Try again\.$/, /^\/compact failed\. The session is unchanged\.$/, /^Conversation compacted\.$/, - /^Review snapshot updated\.(?:\n|$)/, - /^Review request recorded, but the owner workspace is not ready yet\.(?:\n|$)/, - /^Review is unavailable for this room\./, ]; function normalizeSessionCommandText(