claude-bot/EJClaw
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Route STEP_DONE through reviewer

Browse Source
This commit is contained in:
ejclaw
2026-04-24 12:03:12 +09:00
parent 6d28a4ae19
commit f84dccfa10
8 changed files with 105 additions and 156 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
src/message-runtime-follow-up.test.ts
View File

@@ -83,7 +83,7 @@ describe('message-runtime-follow-up', () => {
expect(enqueue).toHaveBeenCalledTimes(1);
});
it('uses the fallback STEP_DONE verdict to schedule owner follow-ups when the owner keeps the task active', () => {
it('does not use fallback owner STEP_DONE verdict to schedule owner follow-ups while active', () => {
const enqueue = vi.fn();
const result = dispatchPairedFollowUpForEvent({
chatJid: 'group@test',
@@ -102,14 +102,12 @@ describe('message-runtime-follow-up', () => {
});
expect(result).toMatchObject({
kind: 'paired-follow-up',
intentKind: 'owner-follow-up',
scheduled: true,
kind: 'none',
taskStatus: 'active',
lastTurnOutputRole: 'owner',
lastTurnOutputVerdict: 'step_done',
});
expect(enqueue).toHaveBeenCalledTimes(1);
expect(enqueue).not.toHaveBeenCalled();
});
it('prefers the latest persisted turn output over the fallback delivery role when both are present', () => {

8
src/message-runtime-rules.test.ts
View File

@@ -120,7 +120,7 @@ describe('message-runtime-rules', () => {
lastTurnOutputVerdict: 'step_done',
intentKind: 'owner-follow-up',
}),
).toBe(true);
).toBe(false);
});
it('maps active tasks with reviewer output to an owner follow-up', () => {
@@ -132,14 +132,14 @@ describe('message-runtime-rules', () => {
).toEqual({ kind: 'owner-follow-up' });
});
it('maps active tasks with owner STEP_DONE output to an owner follow-up', () => {
it('does not map active owner STEP_DONE output to an owner follow-up', () => {
expect(
resolveNextTurnAction({
taskStatus: 'active',
lastTurnOutputRole: 'owner',
lastTurnOutputVerdict: 'step_done',
}),
).toEqual({ kind: 'owner-follow-up' });
).toEqual({ kind: 'none' });
});
it('returns none when an active task has no reviewer or arbiter handoff output', () => {
@@ -329,7 +329,7 @@ describe('message-runtime-rules', () => {
lastTurnOutputRole: 'owner',
lastTurnOutputVerdict: 'step_done',
}),
).toBe('owner');
).toBeNull();
});
it('resolves reviewer execution target from review_ready task status', () => {

3
src/message-runtime-rules.ts
View File

@@ -146,8 +146,7 @@ export function resolveNextTurnAction(args: {
? { kind: 'none' }
: { kind: 'finalize-owner-turn' };
case 'active':
return args.lastTurnOutputVerdict === 'step_done' ||
args.lastTurnOutputRole === 'reviewer' ||
return args.lastTurnOutputRole === 'reviewer' ||
args.lastTurnOutputRole === 'arbiter'
? { kind: 'owner-follow-up' }
: { kind: 'none' };

20
src/message-runtime.test.ts
View File

@@ -2126,7 +2126,7 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead
expect(pending?.prompt).toContain('이전 reviewer 피드백');
});
it('builds an owner follow-up pending turn when the latest owner output is STEP_DONE on an active task', () => {
it('does not build an owner follow-up pending turn from owner STEP_DONE on an active task', () => {
const chatJid = 'group@test';
const group = makeGroup('claude-code');
const task = {
@@ -2170,12 +2170,7 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead
resolveChannel: () => makeChannel(chatJid),
});
expect(pending).not.toBeNull();
expect(pending).toMatchObject({
role: 'owner',
intentKind: 'owner-follow-up',
taskId: task.id,
});
expect(pending).toBeNull();
});
it('builds a reviewer pending turn when a review_ready task follows owner TASK_DONE output', () => {
@@ -2335,7 +2330,7 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead
);
});
it('re-enqueues owner follow-up after a successful owner STEP_DONE delivery keeps the task active', async () => {
it('does not re-enqueue owner follow-up after a successful owner STEP_DONE delivery keeps the task active', async () => {
const chatJid = 'group@test';
const group = makeGroup('codex');
const ownerChannel: Channel = {
@@ -2445,17 +2440,12 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead
chatJid,
'STEP_DONE\n리팩토링 1단계 완료, 다음 단계 진행',
);
expect(enqueueMessageCheck).toHaveBeenCalledWith(chatJid);
expect(enqueueMessageCheck).not.toHaveBeenCalled();
expect(pairedTask.status).toBe('active');
expect(pairedTask.round_trip_count).toBe(1);
expect(pairedTask.owner_failure_count).toBe(0);
expect(logger.info).toHaveBeenCalledWith(
expect(logger.info).not.toHaveBeenCalledWith(
expect.objectContaining({
chatJid,
runId: 'run-owner-step-done-follow-up',
completedRole: 'owner',
taskId: 'task-owner-step-done-follow-up',
taskStatus: 'active',
intentKind: 'owner-follow-up',
}),
'Queued paired follow-up after successful owner delivery',

132
src/paired-execution-context-owner.ts
View File

@@ -20,10 +20,9 @@ import {
} from './paired-execution-context-shared.js';
import type { PairedTask } from './types.js';
type OwnerFinalizeOutcome = 'stop' | 're_review' | 'continue_owner';
type OwnerFinalizeOutcome = 'stop' | 're_review';
const OWNER_FAILURE_ESCALATION_THRESHOLD = 2;
const EMPTY_STEP_DONE_THRESHOLD = 2;
const OWNER_STEP_DONE_LOOP_THRESHOLD = 3;
export function handleFailedOwnerExecution(args: {
task: PairedTask;
@@ -157,7 +156,7 @@ function handleOwnerFinalizeCompletion(args: {
}
if (
signal.kind === 'request_owner_continue' &&
ownerVerdict === 'step_done' &&
hasNewChanges === false &&
nextEmptyStepDoneStreak >= EMPTY_STEP_DONE_THRESHOLD
) {
@@ -200,7 +199,8 @@ function handleOwnerFinalizeCompletion(args: {
owner_failure_count: 0,
owner_step_done_streak: 0,
finalize_step_done_count: nextFinalizeStepDoneCount,
empty_step_done_streak: 0,
empty_step_done_streak:
ownerVerdict === 'step_done' ? nextEmptyStepDoneStreak : 0,
},
});
}
@@ -213,36 +213,27 @@ function handleOwnerFinalizeCompletion(args: {
},
ownerVerdict === 'done_with_concerns'
? 'Owner raised concerns during finalize — task set back to active'
: 'Owner made changes after reviewer approval — task set back to active before re-review',
: ownerVerdict === 'step_done'
? 'Owner reported STEP_DONE during finalize — task set back to active before review'
: 'Owner made changes after reviewer approval — task set back to active before re-review',
);
return 're_review';
}
if (signal.kind === 'request_owner_continue') {
transitionPairedTaskStatus({
maybeAutoTriggerReviewerAfterOwnerCompletion({
task,
taskId,
currentStatus: task.status,
nextStatus: 'active',
expectedUpdatedAt: task.updated_at,
updatedAt: now,
patch: {
owner_failure_count: 0,
owner_step_done_streak: 0,
finalize_step_done_count: nextFinalizeStepDoneCount,
empty_step_done_streak: nextEmptyStepDoneStreak,
},
now,
logMessage:
ownerVerdict === 'step_done'
? 'Auto-triggered reviewer after owner finalize STEP_DONE'
: 'Auto-triggered reviewer after owner finalize required re-review',
patch:
ownerVerdict === 'step_done'
? {
finalize_step_done_count: nextFinalizeStepDoneCount,
empty_step_done_streak: nextEmptyStepDoneStreak,
}
: undefined,
});
logger.info(
{
taskId,
ownerVerdict,
emptyStepDoneStreak: nextEmptyStepDoneStreak,
finalizeStepDoneCount: nextFinalizeStepDoneCount,
summary: summary?.slice(0, 100),
},
'Owner marked finalize output as an intermediate step — task returned to active without re-review',
);
return 'continue_owner';
return 'stop';
}
transitionPairedTaskStatus({
@@ -271,6 +262,11 @@ function maybeAutoTriggerReviewerAfterOwnerCompletion(args: {
taskId: string;
now: string;
logMessage: string;
patch?: {
owner_step_done_streak?: number;
finalize_step_done_count?: number;
empty_step_done_streak?: number;
};
}): void {
const { task, taskId, now, logMessage } = args;
if (task.round_trip_count >= PAIRED_MAX_ROUND_TRIPS) {
@@ -300,6 +296,7 @@ function maybeAutoTriggerReviewerAfterOwnerCompletion(args: {
owner_failure_count: 0,
owner_step_done_streak: 0,
empty_step_done_streak: 0,
...args.patch,
},
});
if (hasActiveCiWatcherForChat(task.chat_jid)) {
@@ -345,16 +342,8 @@ export function handleOwnerCompletion(args: {
}
const ownerVerdict = parseVisibleVerdict(summary);
const workspace = getPairedWorkspace(task.id, 'owner');
const hasNewChanges = workspace?.workspace_dir
? hasCodeChangesSinceRef(workspace.workspace_dir, task.source_ref)
: null;
const nextOwnerStepDoneStreak =
ownerVerdict === 'step_done' ? (task.owner_step_done_streak ?? 0) + 1 : 0;
const nextEmptyStepDoneStreak =
ownerVerdict === 'step_done' && hasNewChanges === false
? (task.empty_step_done_streak ?? 0) + 1
: 0;
const signal = resolveOwnerCompletionSignal({
phase: 'normal',
visibleVerdict: ownerVerdict,
@@ -372,86 +361,23 @@ export function handleOwnerCompletion(args: {
taskId,
ownerVerdict,
ownerStepDoneStreak: nextOwnerStepDoneStreak,
emptyStepDoneStreak: nextEmptyStepDoneStreak,
summary: summary?.slice(0, 100),
},
patch: {
owner_failure_count: 0,
owner_step_done_streak: nextOwnerStepDoneStreak,
empty_step_done_streak: nextEmptyStepDoneStreak,
},
});
return;
}
if (
signal.kind === 'request_owner_continue' &&
hasNewChanges === false &&
nextOwnerStepDoneStreak >= OWNER_STEP_DONE_LOOP_THRESHOLD &&
nextEmptyStepDoneStreak >= OWNER_STEP_DONE_LOOP_THRESHOLD
) {
requestArbiterOrEscalate({
taskId,
currentStatus: task.status,
expectedUpdatedAt: task.updated_at,
now,
arbiterLogMessage:
'Owner repeated STEP_DONE in active mode without code changes — requesting arbiter',
escalateLogMessage:
'Owner repeated STEP_DONE in active mode without code changes — escalating to user',
logContext: {
taskId,
ownerVerdict,
hasNewChanges,
ownerStepDoneStreak: nextOwnerStepDoneStreak,
emptyStepDoneStreak: nextEmptyStepDoneStreak,
summary: summary?.slice(0, 100),
},
patch: {
owner_failure_count: 0,
owner_step_done_streak: nextOwnerStepDoneStreak,
empty_step_done_streak: nextEmptyStepDoneStreak,
},
});
return;
}
if (signal.kind === 'request_owner_continue') {
applyPairedTaskPatch({
taskId,
expectedUpdatedAt: task.updated_at,
updatedAt: now,
patch: {
owner_failure_count: 0,
owner_step_done_streak: nextOwnerStepDoneStreak,
empty_step_done_streak: nextEmptyStepDoneStreak,
},
});
logger.info(
{
taskId,
ownerVerdict,
hasNewChanges,
ownerStepDoneStreak: nextOwnerStepDoneStreak,
emptyStepDoneStreak: nextEmptyStepDoneStreak,
summary: summary?.slice(0, 100),
},
'Owner marked the current output as an intermediate completed step — keeping task active for follow-up',
);
return;
}
if (
nextOwnerStepDoneStreak !== (task.owner_step_done_streak ?? 0) ||
nextEmptyStepDoneStreak !== (task.empty_step_done_streak ?? 0)
) {
if (nextOwnerStepDoneStreak !== (task.owner_step_done_streak ?? 0)) {
applyPairedTaskPatch({
taskId,
expectedUpdatedAt: task.updated_at,
updatedAt: now,
patch: {
owner_step_done_streak: nextOwnerStepDoneStreak,
empty_step_done_streak: nextEmptyStepDoneStreak,
},
});
}

4
src/paired-execution-context-shared.test.ts
View File

@@ -27,7 +27,7 @@ describe('paired execution context shared verdict helpers', () => {
visibleVerdict: 'step_done',
}),
).toEqual({
kind: 'request_owner_continue',
kind: 'request_reviewer',
resetStatusToActive: false,
});
expect(
@@ -57,7 +57,7 @@ describe('paired execution context shared verdict helpers', () => {
deadlockThreshold: 2,
}),
).toEqual({
kind: 'request_owner_continue',
kind: 'request_reviewer',
resetStatusToActive: true,
});
expect(

9
src/paired-execution-context-shared.ts
View File

@@ -9,7 +9,6 @@ import type { PairedTaskStatus } from './types.js';
export type CompletionSignal =
| { kind: 'request_reviewer'; resetStatusToActive: boolean }
| { kind: 'request_owner_finalize' }
| { kind: 'request_owner_continue'; resetStatusToActive: boolean }
| { kind: 'request_owner_changes' }
| { kind: 'request_arbiter' }
| { kind: 'complete'; completionReason: 'done' | 'escalated' }
@@ -38,12 +37,6 @@ export function resolveOwnerCompletionSignal(args: {
}
if (phase === 'normal') {
if (visibleVerdict === 'step_done') {
return {
kind: 'request_owner_continue',
resetStatusToActive: false,
};
}
return {
kind: 'request_reviewer',
resetStatusToActive: false,
@@ -52,7 +45,7 @@ export function resolveOwnerCompletionSignal(args: {
if (visibleVerdict === 'step_done') {
return {
kind: 'request_owner_continue',
kind: 'request_reviewer',
resetStatusToActive: true,
};
}

77
src/paired-execution-context.test.ts
View File

@@ -802,13 +802,22 @@ describe('paired execution context', () => {
).not.toHaveBeenCalled();
});
it('keeps an active task in owner follow-up mode when the owner reports STEP_DONE', () => {
it('requests review when the owner reports STEP_DONE in active mode', () => {
vi.mocked(db.getPairedTaskById).mockReturnValue(
buildPairedTask({
status: 'active',
round_trip_count: 1,
}),
);
vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue(
{
ownerWorkspace: buildWorkspace('owner', '/tmp/paired/task-1/owner'),
reviewerWorkspace: buildWorkspace(
'reviewer',
'/tmp/paired/task-1/reviewer',
),
},
);
completePairedExecutionContext({
taskId: 'task-1',
@@ -820,22 +829,17 @@ describe('paired execution context', () => {
expect(db.updatePairedTask).toHaveBeenCalledWith(
'task-1',
expect.objectContaining({
owner_failure_count: 0,
owner_step_done_streak: 1,
round_trip_count: 2,
owner_step_done_streak: 0,
empty_step_done_streak: 0,
}),
);
expect(
pairedWorkspaceManager.markPairedTaskReviewReady,
).not.toHaveBeenCalled();
expect(db.updatePairedTask).not.toHaveBeenCalledWith(
'task-1',
expect.objectContaining({
status: 'active',
}),
);
).toHaveBeenCalledWith('task-1');
});
it('requests arbiter when active STEP_DONE repeats without code changes', () => {
it('requests review instead of arbiter when active STEP_DONE repeats without code changes', () => {
vi.spyOn(config, 'isArbiterEnabled').mockReturnValue(true);
const repoDir = createCanonicalRepoWithCommit('active step done loop');
@@ -851,6 +855,15 @@ describe('paired execution context', () => {
vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) =>
role === 'owner' ? buildWorkspace('owner', repoDir) : undefined,
);
vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue(
{
ownerWorkspace: buildWorkspace('owner', repoDir),
reviewerWorkspace: buildWorkspace(
'reviewer',
'/tmp/paired/task-1/reviewer',
),
},
);
completePairedExecutionContext({
taskId: 'task-1',
@@ -862,23 +875,44 @@ describe('paired execution context', () => {
expect(db.updatePairedTask).toHaveBeenCalledWith(
'task-1',
expect.objectContaining({
status: 'arbiter_requested',
owner_step_done_streak: 3,
empty_step_done_streak: 3,
round_trip_count: 1,
owner_step_done_streak: 0,
empty_step_done_streak: 0,
}),
);
expect(
pairedWorkspaceManager.markPairedTaskReviewReady,
).not.toHaveBeenCalled();
).toHaveBeenCalledWith('task-1');
expect(db.updatePairedTask).not.toHaveBeenCalledWith(
'task-1',
expect.objectContaining({
status: 'arbiter_requested',
}),
);
});
it('returns merge_ready owner finalize output to active when the owner reports STEP_DONE', () => {
it('re-triggers review when the owner reports STEP_DONE during finalize', () => {
const repoDir = createCanonicalRepoWithCommit('reviewed');
const approvedSourceRef = resolveTreeRef(repoDir);
vi.mocked(db.getPairedTaskById).mockReturnValue(
buildPairedTask({
status: 'merge_ready',
source_ref: approvedSourceRef,
round_trip_count: 1,
}),
);
vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) =>
role === 'owner' ? buildWorkspace('owner', repoDir) : undefined,
);
vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue(
{
ownerWorkspace: buildWorkspace('owner', repoDir),
reviewerWorkspace: buildWorkspace(
'reviewer',
'/tmp/paired/task-1/reviewer',
),
},
);
completePairedExecutionContext({
taskId: 'task-1',
@@ -893,11 +927,20 @@ describe('paired execution context', () => {
status: 'active',
owner_failure_count: 0,
finalize_step_done_count: 1,
empty_step_done_streak: 1,
}),
);
expect(db.updatePairedTask).toHaveBeenCalledWith(
'task-1',
expect.objectContaining({
round_trip_count: 2,
finalize_step_done_count: 1,
empty_step_done_streak: 1,
}),
);
expect(
pairedWorkspaceManager.markPairedTaskReviewReady,
).not.toHaveBeenCalled();
).toHaveBeenCalledWith('task-1');
});
it('requests arbiter when finalize STEP_DONE repeats without code changes', () => {