From 29053d32738833a8ad7ae9f2d943a9d18537db3b Mon Sep 17 00:00:00 2001 From: Eyejoker Date: Sat, 28 Mar 2026 23:36:46 +0900 Subject: [PATCH] feat: formalize paired review command flow --- src/db.test.ts | 7 +- src/db.ts | 88 +++++++- src/message-agent-executor.test.ts | 9 +- src/paired-execution-context.test.ts | 129 ++++++++++-- src/paired-execution-context.ts | 38 ++-- src/paired-review-command.test.ts | 140 +++++++++++++ src/paired-workspace-manager.test.ts | 288 ++++++++++++++++++++++++++- src/paired-workspace-manager.ts | 176 ++++++++++++++-- src/session-commands.test.ts | 37 +++- src/session-commands.ts | 8 +- src/types.ts | 2 + 11 files changed, 863 insertions(+), 59 deletions(-) create mode 100644 src/paired-review-command.test.ts diff --git a/src/db.test.ts b/src/db.test.ts index 025315b..1c0eaa3 100644 --- a/src/db.test.ts +++ b/src/db.test.ts @@ -588,6 +588,7 @@ describe('paired task state', () => { role: 'owner', workspace_dir: '/tmp/paired-room/owner', snapshot_source_dir: null, + snapshot_source_fingerprint: null, status: 'ready', snapshot_refreshed_at: null, created_at: '2026-03-28T00:00:00.000Z', @@ -654,7 +655,7 @@ describe('paired task state', () => { }); updatePairedTask('paired-task-2', { - status: 'review_ready', + status: 'review_pending', review_requested_at: '2026-03-28T00:10:00.000Z', updated_at: '2026-03-28T00:10:00.000Z', }); @@ -669,6 +670,7 @@ describe('paired task state', () => { role: 'reviewer', workspace_dir: '/tmp/reviewer-v1', snapshot_source_dir: '/tmp/owner', + snapshot_source_fingerprint: 'fingerprint-v1', status: 'ready', snapshot_refreshed_at: '2026-03-28T00:10:00.000Z', created_at: '2026-03-28T00:10:00.000Z', @@ -680,13 +682,14 @@ describe('paired task state', () => { role: 'reviewer', workspace_dir: '/tmp/reviewer-v2', snapshot_source_dir: '/tmp/owner', + snapshot_source_fingerprint: 'fingerprint-v2', status: 'ready', snapshot_refreshed_at: '2026-03-28T00:12:00.000Z', created_at: '2026-03-28T00:10:00.000Z', updated_at: '2026-03-28T00:12:00.000Z', }); - expect(getPairedTaskById('paired-task-2')?.status).toBe('review_ready'); + expect(getPairedTaskById('paired-task-2')?.status).toBe('review_pending'); expect(getPairedExecutionById('paired-exec-2')?.status).toBe('running'); expect( listPairedWorkspacesForTask('paired-task-2').map( diff --git a/src/db.ts b/src/db.ts index e67a658..63664b1 100644 --- a/src/db.ts +++ b/src/db.ts @@ -260,6 +260,7 @@ function createSchema(database: Database.Database): void { CHECK ( status IN ( 'draft', + 'review_pending', 'review_ready', 'in_review', 'changes_requested', @@ -294,6 +295,7 @@ function createSchema(database: Database.Database): void { role TEXT NOT NULL, workspace_dir TEXT NOT NULL, snapshot_source_dir TEXT, + snapshot_source_fingerprint TEXT, status TEXT NOT NULL DEFAULT 'provisioning', snapshot_refreshed_at TEXT, created_at TEXT NOT NULL, @@ -418,6 +420,14 @@ function createSchema(database: Database.Database): void { /* column already exists */ } + try { + database.exec( + `ALTER TABLE paired_workspaces ADD COLUMN snapshot_source_fingerprint TEXT`, + ); + } catch { + /* column already exists */ + } + database.exec(` UPDATE scheduled_tasks SET agent_type = COALESCE( @@ -608,6 +618,79 @@ function createSchema(database: Database.Database): void { ); } + const pairedTasksSqlRow = database + .prepare( + `SELECT sql FROM sqlite_master WHERE type = 'table' AND name = 'paired_tasks'`, + ) + .get() as { sql?: string } | undefined; + const pairedTasksSql = pairedTasksSqlRow?.sql || ''; + if (pairedTasksSql && !pairedTasksSql.includes("'review_pending'")) { + database.exec(` + CREATE TABLE paired_tasks_new ( + id TEXT PRIMARY KEY, + chat_jid TEXT NOT NULL, + group_folder TEXT NOT NULL, + owner_service_id TEXT NOT NULL, + reviewer_service_id TEXT NOT NULL, + title TEXT, + source_ref TEXT, + review_requested_at TEXT, + status TEXT NOT NULL DEFAULT 'draft', + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL, + CHECK ( + status IN ( + 'draft', + 'review_pending', + 'review_ready', + 'in_review', + 'changes_requested', + 'merge_ready', + 'merged', + 'discarded', + 'failed' + ) + ) + ); + `); + database.exec(` + INSERT INTO paired_tasks_new ( + id, + chat_jid, + group_folder, + owner_service_id, + reviewer_service_id, + title, + source_ref, + review_requested_at, + status, + created_at, + updated_at + ) + SELECT + id, + chat_jid, + group_folder, + owner_service_id, + reviewer_service_id, + title, + source_ref, + review_requested_at, + status, + created_at, + updated_at + FROM paired_tasks; + `); + database.exec(` + DROP TABLE paired_tasks; + ALTER TABLE paired_tasks_new RENAME TO paired_tasks; + `); + database.exec(` + CREATE INDEX IF NOT EXISTS idx_paired_tasks_chat_status + ON paired_tasks(chat_jid, status, updated_at); + `); + } + // Migrate sessions table to composite PK (group_folder, agent_type) const sessionCols = database .prepare('PRAGMA table_info(sessions)') @@ -1960,15 +2043,17 @@ export function upsertPairedWorkspace(workspace: PairedWorkspace): void { role, workspace_dir, snapshot_source_dir, + snapshot_source_fingerprint, status, snapshot_refreshed_at, created_at, updated_at ) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(id) DO UPDATE SET workspace_dir = excluded.workspace_dir, snapshot_source_dir = excluded.snapshot_source_dir, + snapshot_source_fingerprint = excluded.snapshot_source_fingerprint, status = excluded.status, snapshot_refreshed_at = excluded.snapshot_refreshed_at, updated_at = excluded.updated_at @@ -1979,6 +2064,7 @@ export function upsertPairedWorkspace(workspace: PairedWorkspace): void { workspace.role, workspace.workspace_dir, workspace.snapshot_source_dir, + workspace.snapshot_source_fingerprint, workspace.status, workspace.snapshot_refreshed_at, workspace.created_at, diff --git a/src/message-agent-executor.test.ts b/src/message-agent-executor.test.ts index 19b0584..2c94c84 100644 --- a/src/message-agent-executor.test.ts +++ b/src/message-agent-executor.test.ts @@ -344,6 +344,7 @@ describe('runAgentForGroup room memory', () => { role: 'owner', workspace_dir: '/tmp/paired/owner', snapshot_source_dir: null, + snapshot_source_fingerprint: null, status: 'ready', snapshot_refreshed_at: null, created_at: '2026-03-28T00:00:00.000Z', @@ -387,7 +388,7 @@ describe('runAgentForGroup room memory', () => { }); }); - it('blocks reviewer execution before review-ready and does not spawn the runner', async () => { + it('blocks reviewer execution when an in-review snapshot became stale and does not spawn the runner', async () => { const group = { ...makeGroup(), folder: 'test-group', @@ -439,7 +440,7 @@ describe('runAgentForGroup room memory', () => { EJCLAW_REVIEWER_RUNTIME: '1', }, blockMessage: - 'Review snapshot is not ready yet. Ask the owner to run /review-ready after preparing changes.', + 'Review snapshot is stale after owner changes. Retry the review once to refresh against the latest owner workspace.', }); const result = await runAgentForGroup(makeDeps(), { @@ -462,7 +463,7 @@ describe('runAgentForGroup room memory', () => { expect(agentRunner.runAgentProcess).not.toHaveBeenCalled(); expect(outputs).toEqual([ { - text: 'Review snapshot is not ready yet. Ask the owner to run /review-ready after preparing changes.', + text: 'Review snapshot is stale after owner changes. Retry the review once to refresh against the latest owner workspace.', result: null, }, ]); @@ -472,7 +473,7 @@ describe('runAgentForGroup room memory', () => { executionId: 'run-blocked-reviewer:claude', status: 'failed', summary: - 'Review snapshot is not ready yet. Ask the owner to run /review-ready after preparing changes.', + 'Review snapshot is stale after owner changes. Retry the review once to refresh against the latest owner workspace.', }); }); }); diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index 8edc7e4..5e0c384 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -14,11 +14,13 @@ vi.mock('./db.js', () => ({ vi.mock('./paired-workspace-manager.js', () => ({ markPairedTaskReviewReady: vi.fn(), + prepareReviewerWorkspaceForExecution: vi.fn(), provisionOwnerWorkspaceForPairedTask: vi.fn(), })); vi.mock('./logger.js', () => ({ logger: { + debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn(), @@ -73,11 +75,18 @@ describe('paired execution context', () => { role: 'owner', workspace_dir: '/tmp/paired/task-1/owner', snapshot_source_dir: null, + snapshot_source_fingerprint: null, status: 'ready', snapshot_refreshed_at: null, created_at: '2026-03-28T00:00:00.000Z', updated_at: '2026-03-28T00:00:00.000Z', }); + vi.mocked( + pairedWorkspaceManager.prepareReviewerWorkspaceForExecution, + ).mockReturnValue({ + workspace: null, + autoRefreshed: false, + }); }); it('creates an owner execution with a worktree override', () => { @@ -103,7 +112,7 @@ describe('paired execution context', () => { }); }); - it('uses the reviewer snapshot only after review_ready and marks the task in_review', () => { + it('uses the reviewer snapshot after lazy auto-refresh and marks the task in_review', () => { vi.mocked(db.getLatestOpenPairedTaskForChat).mockReturnValue({ id: 'task-1', chat_jid: 'dc:test', @@ -117,14 +126,33 @@ describe('paired execution context', () => { created_at: '2026-03-28T00:00:00.000Z', updated_at: '2026-03-28T00:00:00.000Z', }); - vi.mocked(db.getPairedWorkspace).mockReturnValue({ - 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', - status: 'ready', - snapshot_refreshed_at: '2026-03-28T00:00:00.000Z', + vi.mocked( + pairedWorkspaceManager.prepareReviewerWorkspaceForExecution, + ).mockReturnValue({ + workspace: { + 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_source_fingerprint: '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', + }, + autoRefreshed: true, + }); + 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', + 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', }); @@ -147,7 +175,7 @@ describe('paired execution context', () => { }); }); - it('blocks reviewer execution before review_ready instead of falling back to the canonical repo', () => { + it('does not change task state for a general reviewer turn before /review', () => { vi.mocked(db.getLatestOpenPairedTaskForChat).mockReturnValue({ id: 'task-1', chat_jid: 'dc:test', @@ -161,19 +189,65 @@ describe('paired execution context', () => { created_at: '2026-03-28T00:00:00.000Z', updated_at: '2026-03-28T00:00:00.000Z', }); + vi.mocked( + pairedWorkspaceManager.prepareReviewerWorkspaceForExecution, + ).mockReturnValue({ + workspace: null, + autoRefreshed: false, + blockMessage: + 'Review snapshot is not ready yet. Ask the owner to run /review (or /review-ready) after preparing changes.', + }); const result = preparePairedExecutionContext({ group, chatJid: 'dc:test', - runId: 'run-blocked-reviewer', + runId: 'run-general-reviewer', roomRoleContext: reviewerContext, }); expect(result?.blockMessage).toBe( - 'Review snapshot is not ready yet. Ask the owner to run /review-ready after preparing changes.', + 'Review snapshot is not ready yet. Ask the owner to run /review (or /review-ready) after preparing changes.', + ); + expect(db.updatePairedTask).not.toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ status: 'in_review' }), + ); + }); + + it('blocks reviewer execution when an in-review snapshot became stale', () => { + 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', + review_requested_at: '2026-03-28T00:00:00.000Z', + status: 'in_review', + created_at: '2026-03-28T00:00:00.000Z', + updated_at: '2026-03-28T00:00:00.000Z', + }); + vi.mocked( + pairedWorkspaceManager.prepareReviewerWorkspaceForExecution, + ).mockReturnValue({ + workspace: null, + autoRefreshed: false, + blockMessage: + 'Review snapshot is stale after owner changes. Retry the review once to refresh against the latest owner workspace.', + }); + + const result = preparePairedExecutionContext({ + group, + chatJid: 'dc:test', + runId: 'run-stale-reviewer', + roomRoleContext: reviewerContext, + }); + + expect(result?.blockMessage).toBe( + 'Review snapshot is stale after owner changes. Retry the review once to refresh against the latest owner workspace.', ); expect(result?.envOverrides.EJCLAW_WORK_DIR).toBeUndefined(); - expect(db.getPairedWorkspace).not.toHaveBeenCalled(); }); it('marks the active room task review-ready through the workspace manager', () => { @@ -211,6 +285,7 @@ describe('paired execution context', () => { role: 'owner', workspace_dir: '/tmp/paired/task-1/owner', snapshot_source_dir: null, + snapshot_source_fingerprint: null, status: 'ready', snapshot_refreshed_at: null, created_at: '2026-03-28T00:00:00.000Z', @@ -222,6 +297,7 @@ describe('paired execution context', () => { role: 'reviewer', workspace_dir: '/tmp/paired/task-1/reviewer', snapshot_source_dir: '/tmp/paired/task-1/owner', + snapshot_source_fingerprint: 'fingerprint-1', status: 'ready', snapshot_refreshed_at: '2026-03-28T00:00:00.000Z', created_at: '2026-03-28T00:00:00.000Z', @@ -241,4 +317,31 @@ describe('paired execution context', () => { ).toHaveBeenCalledWith('task-1'); expect(result?.task.status).toBe('review_ready'); }); + + it('returns null when review-ready setup cannot resolve an owner workspace', () => { + 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', + review_requested_at: null, + status: 'draft', + created_at: '2026-03-28T00:00:00.000Z', + updated_at: '2026-03-28T00:00:00.000Z', + }); + vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue( + null, + ); + + const result = markRoomReviewReady({ + group, + chatJid: 'dc:test', + roomRoleContext: ownerContext, + }); + + expect(result).toBeNull(); + }); }); diff --git a/src/paired-execution-context.ts b/src/paired-execution-context.ts index a612944..e99ea6a 100644 --- a/src/paired-execution-context.ts +++ b/src/paired-execution-context.ts @@ -1,13 +1,16 @@ import { execFileSync } from 'child_process'; import crypto from 'crypto'; +import { + SERVICE_ID, + normalizeServiceId, +} from './config.js'; import { createPairedExecution, createPairedTask, getLatestOpenPairedTaskForChat, getPairedExecutionById, getPairedTaskById, - getPairedWorkspace, updatePairedExecution, updatePairedTask, upsertPairedProject, @@ -15,6 +18,7 @@ import { import { logger } from './logger.js'; import { markPairedTaskReviewReady, + prepareReviewerWorkspaceForExecution, provisionOwnerWorkspaceForPairedTask, } from './paired-workspace-manager.js'; import type { @@ -170,22 +174,17 @@ export function preparePairedExecutionContext(args: { if (roomRoleContext.role === 'owner') { workspace = provisionOwnerWorkspaceForPairedTask(task.id); - } else if ( - task.status === 'review_ready' || - task.status === 'in_review' || - task.status === 'merge_ready' || - task.status === 'changes_requested' - ) { - workspace = getPairedWorkspace(task.id, 'reviewer') ?? null; - if (workspace && task.status === 'review_ready') { + } else { + const reviewerWorkspace = prepareReviewerWorkspaceForExecution(task); + workspace = reviewerWorkspace.workspace; + blockMessage = reviewerWorkspace.blockMessage; + const latestTask = getPairedTaskById(task.id) ?? task; + if (workspace && latestTask.status === 'review_ready') { updatePairedTask(task.id, { status: 'in_review', updated_at: now, }); } - } else { - blockMessage = - 'Review snapshot is not ready yet. Ask the owner to run /review-ready after preparing changes.'; } const execution = ensureExecutionRecord({ @@ -247,9 +246,18 @@ export function markRoomReviewReady(args: { return null; } - const { ownerWorkspace, reviewerWorkspace } = markPairedTaskReviewReady( - task.id, - ); + const isOwnerService = + normalizeServiceId(SERVICE_ID) === task.owner_service_id; + if (isOwnerService) { + provisionOwnerWorkspaceForPairedTask(task.id); + } + + const reviewReady = markPairedTaskReviewReady(task.id); + if (!reviewReady) { + return null; + } + + const { ownerWorkspace, reviewerWorkspace } = reviewReady; return { task: getPairedTaskById(task.id) ?? task, ownerWorkspace, diff --git a/src/paired-review-command.test.ts b/src/paired-review-command.test.ts new file mode 100644 index 0000000..c920bf2 --- /dev/null +++ b/src/paired-review-command.test.ts @@ -0,0 +1,140 @@ +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('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!.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); + }); +}); diff --git a/src/paired-workspace-manager.test.ts b/src/paired-workspace-manager.test.ts index 8076211..e76ca81 100644 --- a/src/paired-workspace-manager.test.ts +++ b/src/paired-workspace-manager.test.ts @@ -96,8 +96,9 @@ describe('paired workspace manager', () => { 'review me\n', ); - const { reviewerWorkspace } = - manager.markPairedTaskReviewReady('paired-task-1'); + const reviewReady = manager.markPairedTaskReviewReady('paired-task-1'); + expect(reviewReady).not.toBeNull(); + const reviewerWorkspace = reviewReady!.reviewerWorkspace; expect( fs.readFileSync( @@ -121,9 +122,92 @@ describe('paired workspace manager', () => { runGit(['status', '--short'], reviewerWorkspace.workspace_dir), ).toContain('M README.md'); expect(db.getPairedTaskById('paired-task-1')?.status).toBe('review_ready'); + expect(db.getPairedTaskById('paired-task-1')?.review_requested_at).toBeTruthy(); expect( db.getPairedWorkspace('paired-task-1', 'reviewer')?.snapshot_refreshed_at, ).toBeTruthy(); + expect( + db.getPairedWorkspace('paired-task-1', 'reviewer') + ?.snapshot_source_fingerprint, + ).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 }); + 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); + + process.env.EJCLAW_STORE_DIR = path.join(tempRoot, 'shared-store'); + process.env.EJCLAW_GROUPS_DIR = path.join(tempRoot, 'shared-groups'); + + process.env.EJCLAW_DATA_DIR = path.join(tempRoot, 'data-owner'); + vi.resetModules(); + let { db, manager } = await loadModules(); + db.initDatabase(); + + const now = '2026-03-28T00:00:00.000Z'; + db.upsertPairedProject({ + chat_jid: 'dc:test', + group_folder: 'paired-room', + canonical_work_dir: canonicalDir, + workspace_topology: 'shadow-snapshot', + created_at: now, + updated_at: now, + }); + db.createPairedTask({ + id: 'paired-task-cross-service', + chat_jid: 'dc:test', + group_folder: 'paired-room', + owner_service_id: 'codex-main', + reviewer_service_id: 'codex-review', + title: 'cross service review', + source_ref: 'HEAD', + review_requested_at: null, + status: 'draft', + created_at: now, + updated_at: now, + }); + + const ownerWorkspace = + manager.provisionOwnerWorkspaceForPairedTask('paired-task-cross-service'); + fs.writeFileSync( + path.join(ownerWorkspace.workspace_dir, 'README.md'), + 'owner modified\n', + ); + + process.env.EJCLAW_DATA_DIR = path.join(tempRoot, 'data-review'); + vi.resetModules(); + ({ db, manager } = await loadModules()); + db.initDatabase(); + + const reviewerWorkspace = manager.refreshReviewerSnapshotForPairedTask( + 'paired-task-cross-service', + ); + const reviewServiceOwnerDir = path.join( + tempRoot, + 'data-review', + 'workspaces', + 'paired-room', + 'tasks', + 'paired-task-cross-service', + 'owner', + ); + + expect( + fs.readFileSync( + path.join(reviewerWorkspace.workspace_dir, 'README.md'), + 'utf-8', + ), + ).toBe('owner modified\n'); + expect(reviewerWorkspace.snapshot_source_dir).toBe( + ownerWorkspace.workspace_dir, + ); + expect(fs.existsSync(reviewServiceOwnerDir)).toBe(false); }); it('replaces stale reviewer files on snapshot refresh', async () => { @@ -377,6 +461,7 @@ describe('paired workspace manager', () => { updated_at: now, }); + manager.provisionOwnerWorkspaceForPairedTask('paired-task-4'); const reviewerWorkspace = manager.refreshReviewerSnapshotForPairedTask('paired-task-4'); @@ -400,4 +485,203 @@ 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, + workspace_topology: 'shadow-snapshot', + 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', + review_requested_at: null, + status: 'draft', + 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(); + + 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, + workspace_topology: 'shadow-snapshot', + created_at: now, + updated_at: now, + }); + db.createPairedTask({ + id: 'paired-task-5b', + chat_jid: 'dc:test', + group_folder: 'paired-room', + owner_service_id: 'codex-main', + reviewer_service_id: 'codex-review', + title: null, + source_ref: 'HEAD', + review_requested_at: '2026-03-28T00:01:00.000Z', + status: 'review_pending', + created_at: now, + updated_at: '2026-03-28T00:01:00.000Z', + }); + + const ownerWorkspace = + manager.provisionOwnerWorkspaceForPairedTask('paired-task-5b'); + fs.writeFileSync( + path.join(ownerWorkspace.workspace_dir, 'README.md'), + 'owner change\n', + ); + + const result = manager.prepareReviewerWorkspaceForExecution( + db.getPairedTaskById('paired-task-5b')!, + ); + + expect(result.autoRefreshed).toBe(true); + expect(result.blockMessage).toBeUndefined(); + expect( + fs.readFileSync( + path.join(result.workspace!.workspace_dir, 'README.md'), + 'utf-8', + ), + ).toBe('owner change\n'); + expect(db.getPairedTaskById('paired-task-5b')?.status).toBe('review_ready'); + expect(db.getPairedTaskById('paired-task-5b')?.review_requested_at).toBe( + '2026-03-28T00:01:00.000Z', + ); + }); + + it('blocks once when an in-review snapshot becomes stale, then refreshes on retry', 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, + workspace_topology: 'shadow-snapshot', + created_at: now, + updated_at: now, + }); + db.createPairedTask({ + id: 'paired-task-6', + chat_jid: 'dc:test', + group_folder: 'paired-room', + owner_service_id: 'codex-main', + reviewer_service_id: 'codex-review', + title: null, + source_ref: 'HEAD', + review_requested_at: null, + status: 'draft', + created_at: now, + updated_at: now, + }); + + const ownerWorkspace = + manager.provisionOwnerWorkspaceForPairedTask('paired-task-6'); + fs.writeFileSync( + path.join(ownerWorkspace.workspace_dir, 'README.md'), + 'first review\n', + ); + manager.markPairedTaskReviewReady('paired-task-6'); + db.updatePairedTask('paired-task-6', { + status: 'in_review', + updated_at: '2026-03-28T00:05:00.000Z', + }); + + fs.writeFileSync( + path.join(ownerWorkspace.workspace_dir, 'README.md'), + 'owner changed again\n', + ); + + const blocked = manager.prepareReviewerWorkspaceForExecution( + db.getPairedTaskById('paired-task-6')!, + ); + + expect(blocked.workspace).toBeNull(); + expect(blocked.autoRefreshed).toBe(false); + 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')?.review_requested_at).toBeTruthy(); + expect(db.getPairedWorkspace('paired-task-6', 'reviewer')?.status).toBe( + 'stale', + ); + + const refreshed = manager.prepareReviewerWorkspaceForExecution( + db.getPairedTaskById('paired-task-6')!, + ); + + expect(refreshed.autoRefreshed).toBe(true); + expect(refreshed.blockMessage).toBeUndefined(); + expect( + fs.readFileSync( + path.join(refreshed.workspace!.workspace_dir, 'README.md'), + 'utf-8', + ), + ).toBe('owner changed again\n'); + expect(db.getPairedTaskById('paired-task-6')?.review_requested_at).toBeTruthy(); + }); }); diff --git a/src/paired-workspace-manager.ts b/src/paired-workspace-manager.ts index ef76729..ca0c1fb 100644 --- a/src/paired-workspace-manager.ts +++ b/src/paired-workspace-manager.ts @@ -1,4 +1,5 @@ import { execFileSync } from 'child_process'; +import crypto from 'crypto'; import fs from 'fs'; import path from 'path'; @@ -13,6 +14,11 @@ import { resolvePairedTaskWorkspacePath } from './group-folder.js'; import { logger } from './logger.js'; 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.'; + const REVIEWER_SNAPSHOT_DENY_SEGMENTS = new Set([ '.git', '.claude', @@ -115,14 +121,6 @@ function ensureCleanDirectory(targetDir: string): void { fs.mkdirSync(targetDir, { recursive: true }); } -function resetDirectoryExceptGit(targetDir: string): void { - if (!fs.existsSync(targetDir)) return; - for (const entry of fs.readdirSync(targetDir)) { - if (entry === '.git') continue; - fs.rmSync(path.join(targetDir, entry), { recursive: true, force: true }); - } -} - function listGitPaths(repoDir: string, args: string[]): string[] { const output = execFileSync('git', args, { cwd: repoDir, @@ -221,6 +219,34 @@ function removeSnapshotPaths(targetDir: string, relativePaths: string[]): void { } } +function buildReviewerSnapshotFingerprint(args: { + sourceDir: string; + allowedTrackedFiles: string[]; + deletedTrackedFiles: string[]; + allowedUntrackedFiles: string[]; +}): string { + const hash = crypto.createHash('sha256'); + const appendFile = (kind: 'tracked' | 'untracked', relativePath: string) => { + const sourcePath = path.join(args.sourceDir, relativePath); + if (!fs.existsSync(sourcePath)) return; + hash.update(`${kind}\0${relativePath}\0`); + hash.update(fs.readFileSync(sourcePath)); + hash.update('\0'); + }; + + for (const relativePath of [...new Set(args.allowedTrackedFiles)].sort()) { + appendFile('tracked', relativePath); + } + for (const relativePath of [...new Set(args.deletedTrackedFiles)].sort()) { + hash.update(`deleted\0${relativePath}\0`); + } + for (const relativePath of [...new Set(args.allowedUntrackedFiles)].sort()) { + appendFile('untracked', relativePath); + } + + return hash.digest('hex'); +} + function applyReviewerSparseCheckout( reviewerDir: string, allowedTrackedFiles: string[], @@ -275,7 +301,9 @@ function makeWorkspaceRecord(args: { role: PairedWorkspace['role']; workspaceDir: string; snapshotSourceDir?: string | null; + snapshotSourceFingerprint?: string | null; snapshotRefreshedAt?: string | null; + status?: PairedWorkspace['status']; createdAt?: string; }): PairedWorkspace { const existing = getPairedWorkspace(args.taskId, args.role); @@ -287,7 +315,11 @@ function makeWorkspaceRecord(args: { workspace_dir: args.workspaceDir, snapshot_source_dir: args.snapshotSourceDir ?? existing?.snapshot_source_dir ?? null, - status: 'ready', + snapshot_source_fingerprint: + args.snapshotSourceFingerprint ?? + existing?.snapshot_source_fingerprint ?? + null, + status: args.status ?? 'ready', snapshot_refreshed_at: args.snapshotRefreshedAt ?? existing?.snapshot_refreshed_at ?? null, created_at: existing?.created_at || args.createdAt || now, @@ -334,7 +366,12 @@ export function provisionOwnerWorkspaceForPairedTask( export function refreshReviewerSnapshotForPairedTask( taskId: string, ): PairedWorkspace { - const ownerWorkspace = provisionOwnerWorkspaceForPairedTask(taskId); + const ownerWorkspace = getPairedWorkspace(taskId, 'owner'); + if (!ownerWorkspace) { + throw new Error( + `Owner workspace is not available for paired task ${taskId}.`, + ); + } ensureGitRepository(ownerWorkspace.workspace_dir); const { task } = getTaskAndProject(taskId); @@ -359,6 +396,12 @@ export function refreshReviewerSnapshotForPairedTask( const allowedUntrackedFiles = listAllowedUntrackedFiles( ownerWorkspace.workspace_dir, ); + const snapshotFingerprint = buildReviewerSnapshotFingerprint({ + sourceDir: ownerWorkspace.workspace_dir, + allowedTrackedFiles, + deletedTrackedFiles, + allowedUntrackedFiles, + }); runGit(['reset', '--hard', 'HEAD'], reviewerDir); runGit(['clean', '-fdx'], reviewerDir); @@ -382,6 +425,7 @@ export function refreshReviewerSnapshotForPairedTask( role: 'reviewer', workspaceDir: reviewerDir, snapshotSourceDir: ownerWorkspace.workspace_dir, + snapshotSourceFingerprint: snapshotFingerprint, snapshotRefreshedAt: refreshedAt, }); upsertPairedWorkspace(workspace); @@ -395,16 +439,122 @@ export function refreshReviewerSnapshotForPairedTask( export function markPairedTaskReviewReady(taskId: string): { ownerWorkspace: PairedWorkspace; reviewerWorkspace: PairedWorkspace; -} { - const ownerWorkspace = provisionOwnerWorkspaceForPairedTask(taskId); +} | null { + const requestedAt = new Date().toISOString(); + updatePairedTask(taskId, { + status: 'review_pending', + review_requested_at: requestedAt, + updated_at: requestedAt, + }); + + const ownerWorkspace = getPairedWorkspace(taskId, 'owner'); + if (!ownerWorkspace) { + return null; + } const reviewerWorkspace = refreshReviewerSnapshotForPairedTask(taskId); const now = new Date().toISOString(); updatePairedTask(taskId, { status: 'review_ready', - review_requested_at: now, updated_at: now, }); return { ownerWorkspace, reviewerWorkspace }; } + +export interface PreparedReviewerWorkspace { + workspace: PairedWorkspace | null; + blockMessage?: string; + autoRefreshed: boolean; +} + +export function prepareReviewerWorkspaceForExecution( + task: PairedTask, +): PreparedReviewerWorkspace { + const ownerWorkspace = getPairedWorkspace(task.id, 'owner') ?? null; + if (!ownerWorkspace) { + return { + workspace: null, + autoRefreshed: false, + blockMessage: REVIEWER_SNAPSHOT_NOT_READY_BLOCK_MESSAGE, + }; + } + const reviewerWorkspace = getPairedWorkspace(task.id, 'reviewer') ?? null; + const allowedTrackedFiles = listAllowedTrackedFiles( + ownerWorkspace.workspace_dir, + ); + const deletedTrackedFiles = listDeletedTrackedFiles( + ownerWorkspace.workspace_dir, + ); + const allowedUntrackedFiles = listAllowedUntrackedFiles( + ownerWorkspace.workspace_dir, + ); + const currentFingerprint = buildReviewerSnapshotFingerprint({ + sourceDir: ownerWorkspace.workspace_dir, + allowedTrackedFiles, + deletedTrackedFiles, + allowedUntrackedFiles, + }); + const snapshotMissing = + !reviewerWorkspace?.snapshot_refreshed_at || !reviewerWorkspace; + const snapshotStale = + !!reviewerWorkspace && + (reviewerWorkspace.status === 'stale' || + reviewerWorkspace.snapshot_source_fingerprint !== currentFingerprint); + const now = new Date().toISOString(); + + if (snapshotMissing || snapshotStale) { + if ( + task.status === 'review_pending' || + task.status === 'review_ready' + ) { + const refreshedWorkspace = refreshReviewerSnapshotForPairedTask(task.id); + updatePairedTask(task.id, { + status: 'review_ready', + updated_at: now, + }); + return { + workspace: refreshedWorkspace, + autoRefreshed: true, + }; + } + + if (snapshotMissing) { + return { + workspace: null, + autoRefreshed: false, + blockMessage: REVIEWER_SNAPSHOT_NOT_READY_BLOCK_MESSAGE, + }; + } + + if (reviewerWorkspace) { + upsertPairedWorkspace( + makeWorkspaceRecord({ + taskId: task.id, + role: 'reviewer', + workspaceDir: reviewerWorkspace.workspace_dir, + snapshotSourceDir: reviewerWorkspace.snapshot_source_dir, + snapshotSourceFingerprint: + reviewerWorkspace.snapshot_source_fingerprint, + snapshotRefreshedAt: reviewerWorkspace.snapshot_refreshed_at, + status: 'stale', + createdAt: reviewerWorkspace.created_at, + }), + ); + } + updatePairedTask(task.id, { + status: 'review_pending', + updated_at: now, + }); + return { + workspace: null, + autoRefreshed: false, + blockMessage: REVIEWER_SNAPSHOT_STALE_BLOCK_MESSAGE, + }; + } + + return { + workspace: reviewerWorkspace, + autoRefreshed: false, + }; +} diff --git a/src/session-commands.test.ts b/src/session-commands.test.ts index 211f1d0..c0c36d7 100644 --- a/src/session-commands.test.ts +++ b/src/session-commands.test.ts @@ -23,10 +23,12 @@ describe('extractSessionCommand', () => { expect(extractSessionCommand('/clear', trigger)).toBe('/clear'); }); - it('detects bare /review-ready', () => { - expect(extractSessionCommand('/review-ready', trigger)).toBe( - '/review-ready', - ); + it('detects bare /review', () => { + expect(extractSessionCommand('/review', trigger)).toBe('/review'); + }); + + it('normalizes /review-ready to /review', () => { + expect(extractSessionCommand('/review-ready', trigger)).toBe('/review'); }); it('rejects /compact with extra text', () => { @@ -185,7 +187,32 @@ describe('handleSessionCommand', () => { ); }); - it('handles authorized /review-ready without invoking the agent', async () => { + it('handles authorized /review without invoking the agent', async () => { + const deps = makeDeps({ + markReviewReady: vi + .fn() + .mockResolvedValue('Review snapshot updated.\n- Task: paired-task-1'), + }); + + const result = await handleSessionCommand({ + missedMessages: [makeMsg('/review')], + isMainGroup: true, + groupName: 'test', + triggerPattern: trigger, + timezone: 'UTC', + deps, + }); + + expect(result).toEqual({ handled: true, success: true }); + expect(deps.markReviewReady).toHaveBeenCalledTimes(1); + expect(deps.runAgent).not.toHaveBeenCalled(); + expect(deps.advanceCursor).toHaveBeenCalledWith('100'); + expect(deps.sendMessage).toHaveBeenCalledWith( + 'Review snapshot updated.\n- Task: paired-task-1', + ); + }); + + it('handles authorized /review-ready as an alias without invoking the agent', async () => { const deps = makeDeps({ markReviewReady: vi .fn() diff --git a/src/session-commands.ts b/src/session-commands.ts index 1ced58e..6c57b99 100644 --- a/src/session-commands.ts +++ b/src/session-commands.ts @@ -11,7 +11,7 @@ const SESSION_COMMAND_CONTROL_PATTERNS = [ /^\/compact failed\. The session is unchanged\.$/, /^Conversation compacted\.$/, /^Review snapshot updated\.$/, - /^Review-ready is unavailable for this room\./, + /^Review is unavailable for this room\./, ]; /** @@ -26,7 +26,7 @@ export function extractSessionCommand( text = text.replace(triggerPattern, '').trim(); if (text === '/compact') return '/compact'; if (text === '/clear') return '/clear'; - if (text === '/review-ready') return '/review-ready'; + if (text === '/review' || text === '/review-ready') return '/review'; return null; } @@ -153,12 +153,12 @@ export async function handleSessionCommand(opts: { return { handled: true, success: true }; } - if (command === '/review-ready') { + if (command === '/review') { const result = await deps.markReviewReady(); deps.advanceCursor(cmdMsg.timestamp); await deps.sendMessage( result ?? - 'Review-ready is unavailable for this room. Paired workspaces require a configured project workDir.', + 'Review is unavailable for this room. Paired workspaces require a configured project workDir.', ); return { handled: true, success: true }; } diff --git a/src/types.ts b/src/types.ts index 549e507..4f432f9 100644 --- a/src/types.ts +++ b/src/types.ts @@ -29,6 +29,7 @@ export type PairedWorkspaceTopology = 'shadow-snapshot' | 'reviewer-cow'; export type PairedTaskStatus = | 'draft' + | 'review_pending' | 'review_ready' | 'in_review' | 'changes_requested' @@ -107,6 +108,7 @@ export interface PairedWorkspace { role: PairedWorkspaceRole; workspace_dir: string; snapshot_source_dir: string | null; + snapshot_source_fingerprint: string | null; status: PairedWorkspaceStatus; snapshot_refreshed_at: string | null; created_at: string;