fix: keep review requests pending without owner workspace
This commit is contained in:
@@ -41,7 +41,10 @@ import {
|
||||
import { runAgentForGroup } from './message-agent-executor.js';
|
||||
import { MessageTurnController } from './message-turn-controller.js';
|
||||
import { createSuppressToken } from './output-suppression.js';
|
||||
import { markRoomReviewReady } from './paired-execution-context.js';
|
||||
import {
|
||||
formatRoomReviewReadyMessage,
|
||||
markRoomReviewReady,
|
||||
} from './paired-execution-context.js';
|
||||
import { buildRoomRoleContext } from './room-role-context.js';
|
||||
import {
|
||||
extractSessionCommand,
|
||||
@@ -573,15 +576,7 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): {
|
||||
chatJid,
|
||||
roomRoleContext,
|
||||
});
|
||||
if (!result) {
|
||||
return null;
|
||||
}
|
||||
return [
|
||||
'Review snapshot updated.',
|
||||
`- Task: ${result.task.id}`,
|
||||
`- Owner workspace: ${result.ownerWorkspace.workspace_dir}`,
|
||||
`- Reviewer snapshot: ${result.reviewerWorkspace.workspace_dir}`,
|
||||
].join('\n');
|
||||
return formatRoomReviewReadyMessage(result);
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
@@ -29,6 +29,7 @@ vi.mock('./logger.js', () => ({
|
||||
|
||||
import * as db from './db.js';
|
||||
import {
|
||||
formatRoomReviewReadyMessage,
|
||||
markRoomReviewReady,
|
||||
preparePairedExecutionContext,
|
||||
} from './paired-execution-context.js';
|
||||
@@ -315,10 +316,14 @@ describe('paired execution context', () => {
|
||||
expect(
|
||||
pairedWorkspaceManager.markPairedTaskReviewReady,
|
||||
).toHaveBeenCalledWith('task-1');
|
||||
expect(result?.task.status).toBe('review_ready');
|
||||
expect(result?.status).toBe('ready');
|
||||
if (result?.status !== 'ready') {
|
||||
throw new Error('expected ready review result');
|
||||
}
|
||||
expect(result.task.status).toBe('review_ready');
|
||||
});
|
||||
|
||||
it('returns null when review-ready setup cannot resolve an owner workspace', () => {
|
||||
it('returns pending when review-ready setup cannot resolve an owner workspace', () => {
|
||||
vi.mocked(db.getLatestOpenPairedTaskForChat).mockReturnValue({
|
||||
id: 'task-1',
|
||||
chat_jid: 'dc:test',
|
||||
@@ -332,6 +337,19 @@ describe('paired execution context', () => {
|
||||
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',
|
||||
review_requested_at: '2026-03-28T00:01:00.000Z',
|
||||
status: 'review_pending',
|
||||
created_at: '2026-03-28T00:00:00.000Z',
|
||||
updated_at: '2026-03-28T00:01:00.000Z',
|
||||
});
|
||||
vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue(
|
||||
null,
|
||||
);
|
||||
@@ -342,6 +360,39 @@ describe('paired execution context', () => {
|
||||
roomRoleContext: ownerContext,
|
||||
});
|
||||
|
||||
expect(result).toBeNull();
|
||||
expect(result).toEqual(
|
||||
expect.objectContaining({
|
||||
status: 'pending',
|
||||
pendingReason: 'owner-workspace-not-ready',
|
||||
task: expect.objectContaining({
|
||||
id: 'task-1',
|
||||
status: 'review_pending',
|
||||
}),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('formats a pending review-ready result into a user-facing pending message', () => {
|
||||
const message = formatRoomReviewReadyMessage({
|
||||
status: 'pending',
|
||||
pendingReason: 'owner-workspace-not-ready',
|
||||
task: {
|
||||
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:01:00.000Z',
|
||||
status: 'review_pending',
|
||||
created_at: '2026-03-28T00:00:00.000Z',
|
||||
updated_at: '2026-03-28T00:01:00.000Z',
|
||||
},
|
||||
});
|
||||
|
||||
expect(message).toBe(
|
||||
'Review request recorded, but the owner workspace is not ready yet.\n- Task: task-1\nThe task stays review_pending until the owner workspace is prepared.',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -152,6 +152,19 @@ export interface PreparedPairedExecutionContext {
|
||||
blockMessage?: string;
|
||||
}
|
||||
|
||||
export type MarkRoomReviewReadyResult =
|
||||
| {
|
||||
status: 'ready';
|
||||
task: PairedTask;
|
||||
ownerWorkspace: PairedWorkspace;
|
||||
reviewerWorkspace: PairedWorkspace;
|
||||
}
|
||||
| {
|
||||
status: 'pending';
|
||||
task: PairedTask;
|
||||
pendingReason: 'owner-workspace-not-ready';
|
||||
};
|
||||
|
||||
export function preparePairedExecutionContext(args: {
|
||||
group: RegisteredGroup;
|
||||
chatJid: string;
|
||||
@@ -231,11 +244,7 @@ export function markRoomReviewReady(args: {
|
||||
group: RegisteredGroup;
|
||||
chatJid: string;
|
||||
roomRoleContext?: RoomRoleContext;
|
||||
}): {
|
||||
task: PairedTask;
|
||||
ownerWorkspace: PairedWorkspace;
|
||||
reviewerWorkspace: PairedWorkspace;
|
||||
} | null {
|
||||
}): MarkRoomReviewReadyResult | null {
|
||||
const { group, chatJid, roomRoleContext } = args;
|
||||
if (!roomRoleContext || !group.workDir) {
|
||||
return null;
|
||||
@@ -253,14 +262,43 @@ export function markRoomReviewReady(args: {
|
||||
}
|
||||
|
||||
const reviewReady = markPairedTaskReviewReady(task.id);
|
||||
const latestTask = getPairedTaskById(task.id) ?? task;
|
||||
if (!reviewReady) {
|
||||
return null;
|
||||
return {
|
||||
status: 'pending',
|
||||
task: latestTask,
|
||||
pendingReason: 'owner-workspace-not-ready',
|
||||
};
|
||||
}
|
||||
|
||||
const { ownerWorkspace, reviewerWorkspace } = reviewReady;
|
||||
return {
|
||||
task: getPairedTaskById(task.id) ?? task,
|
||||
status: 'ready',
|
||||
task: latestTask,
|
||||
ownerWorkspace,
|
||||
reviewerWorkspace,
|
||||
};
|
||||
}
|
||||
|
||||
export function formatRoomReviewReadyMessage(
|
||||
result: MarkRoomReviewReadyResult | null,
|
||||
): 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');
|
||||
}
|
||||
|
||||
@@ -124,17 +124,74 @@ describe('paired /review command path', () => {
|
||||
'owner',
|
||||
);
|
||||
|
||||
expect(result).not.toBeNull();
|
||||
expect(result!.ownerWorkspace.workspace_dir).toBe(ownerWorkspaceDir);
|
||||
expect(result!.reviewerWorkspace.snapshot_source_dir).toBe(
|
||||
expect(result?.status).toBe('ready');
|
||||
if (result?.status !== 'ready') {
|
||||
throw new Error('expected ready review result');
|
||||
}
|
||||
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'),
|
||||
path.join(result.reviewerWorkspace.workspace_dir, 'README.md'),
|
||||
'utf-8',
|
||||
),
|
||||
).toBe('owner change\n');
|
||||
expect(fs.existsSync(reviewerLocalOwnerDir)).toBe(false);
|
||||
});
|
||||
|
||||
it('returns pending and keeps review_pending when reviewer handles /review before any 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 reviewerLocalOwnerDir = path.join(
|
||||
tempRoot,
|
||||
'data-review',
|
||||
'workspaces',
|
||||
'paired-room',
|
||||
'tasks',
|
||||
result?.task.id ?? 'missing-task',
|
||||
'owner',
|
||||
);
|
||||
|
||||
expect(result).toEqual(
|
||||
expect.objectContaining({
|
||||
status: 'pending',
|
||||
pendingReason: 'owner-workspace-not-ready',
|
||||
task: expect.objectContaining({
|
||||
status: 'review_pending',
|
||||
}),
|
||||
}),
|
||||
);
|
||||
expect(result?.task.review_requested_at).toBeTruthy();
|
||||
expect(fs.existsSync(reviewerLocalOwnerDir)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -132,6 +132,53 @@ 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,
|
||||
workspace_topology: 'shadow-snapshot',
|
||||
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',
|
||||
review_requested_at: null,
|
||||
status: 'draft',
|
||||
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 });
|
||||
@@ -602,6 +649,69 @@ 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,
|
||||
workspace_topology: 'shadow-snapshot',
|
||||
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',
|
||||
review_requested_at: null,
|
||||
status: 'draft',
|
||||
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();
|
||||
|
||||
@@ -91,6 +91,22 @@ describe('isSessionCommandControlMessage', () => {
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('matches multiline review-updated output', () => {
|
||||
expect(
|
||||
isSessionCommandControlMessage(
|
||||
'Review snapshot updated.\n- Task: paired-task-1',
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('matches pending review output', () => {
|
||||
expect(
|
||||
isSessionCommandControlMessage(
|
||||
'Review request recorded, but the owner workspace is not ready yet.\n- Task: paired-task-1\nThe task stays review_pending until the owner workspace is prepared.',
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('does not match regular bot conversation', () => {
|
||||
expect(
|
||||
isSessionCommandControlMessage(
|
||||
@@ -237,6 +253,31 @@ describe('handleSessionCommand', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('sends a pending review message when owner workspace is not ready yet', async () => {
|
||||
const deps = makeDeps({
|
||||
markReviewReady: vi
|
||||
.fn()
|
||||
.mockResolvedValue(
|
||||
'Review request recorded, but the owner workspace is not ready yet.\n- Task: paired-task-1\nThe task stays review_pending until the owner workspace is prepared.',
|
||||
),
|
||||
});
|
||||
|
||||
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.sendMessage).toHaveBeenCalledWith(
|
||||
'Review request recorded, but the owner workspace is not ready yet.\n- Task: paired-task-1\nThe task stays review_pending until the owner workspace is prepared.',
|
||||
);
|
||||
});
|
||||
|
||||
it('sends denial to interactable sender in non-main group', async () => {
|
||||
const deps = makeDeps();
|
||||
const result = await handleSessionCommand({
|
||||
|
||||
@@ -10,7 +10,8 @@ const SESSION_COMMAND_CONTROL_PATTERNS = [
|
||||
/^Failed to process messages before \/compact\. Try again\.$/,
|
||||
/^\/compact failed\. The session is unchanged\.$/,
|
||||
/^Conversation compacted\.$/,
|
||||
/^Review snapshot updated\.$/,
|
||||
/^Review snapshot updated\.(?:\n|$)/,
|
||||
/^Review request recorded, but the owner workspace is not ready yet\.(?:\n|$)/,
|
||||
/^Review is unavailable for this room\./,
|
||||
];
|
||||
|
||||
|
||||
Reference in New Issue
Block a user