Serialize merge-ready owner finalize handoff

This commit is contained in:
ejclaw
2026-04-05 20:11:14 +09:00
parent b6c8546f25
commit 4b7618896e
4 changed files with 132 additions and 1 deletions

View File

@@ -1030,6 +1030,92 @@ describe('runAgentForGroup room memory', () => {
undefined,
);
});
it('does not enqueue a second generic follow-up when reviewer approval already moved the task to merge_ready', async () => {
const group = { ...makeGroup(), folder: 'test-group' };
const deps = makeDeps();
vi.mocked(serviceRouting.getEffectiveChannelLease).mockReturnValue({
chat_jid: 'group@test',
owner_agent_type: 'claude-code',
reviewer_agent_type: 'claude-code',
arbiter_agent_type: null,
owner_service_id: 'claude',
reviewer_service_id: 'claude',
arbiter_service_id: null,
activated_at: null,
reason: null,
explicit: false,
});
vi.mocked(
pairedExecutionContext.preparePairedExecutionContext,
).mockReturnValue({
task: {
id: 'paired-task-merge-ready',
chat_jid: 'group@test',
group_folder: 'test-group',
owner_service_id: 'claude',
reviewer_service_id: 'claude',
title: null,
source_ref: 'HEAD',
plan_notes: null,
round_trip_count: 1,
review_requested_at: '2026-03-31T00:00:00.000Z',
status: 'in_review',
arbiter_verdict: null,
arbiter_requested_at: null,
completion_reason: null,
created_at: '2026-03-31T00:00:00.000Z',
updated_at: '2026-03-31T00:00:00.000Z',
},
workspace: null,
envOverrides: {},
});
vi.mocked(db.getPairedTaskById).mockReturnValue({
id: 'paired-task-merge-ready',
chat_jid: 'group@test',
group_folder: 'test-group',
owner_service_id: 'claude',
reviewer_service_id: 'claude',
title: null,
source_ref: 'HEAD',
plan_notes: null,
round_trip_count: 1,
review_requested_at: '2026-03-31T00:00:00.000Z',
status: 'merge_ready',
arbiter_verdict: null,
arbiter_requested_at: null,
completion_reason: null,
created_at: '2026-03-31T00:00:00.000Z',
updated_at: '2026-03-31T00:00:00.000Z',
});
vi.mocked(agentRunner.runAgentProcess).mockImplementation(
async (_group, _input, _onProcess, onOutput) => {
await onOutput?.({
status: 'success',
result: 'DONE\nreview complete',
output: { visibility: 'public', text: 'DONE\nreview complete' },
phase: 'final',
});
return {
status: 'success',
result: 'DONE\nreview complete',
};
},
);
const result = await runAgentForGroup(deps, {
group,
prompt: 'please review',
chatJid: 'group@test',
runId: 'run-review-merge-ready',
forcedRole: 'reviewer',
onOutput: async () => {},
});
expect(result).toBe('success');
expect(deps.queue.enqueueMessageCheck).not.toHaveBeenCalled();
});
});
describe('runAgentForGroup Claude rotation', () => {

View File

@@ -1124,8 +1124,20 @@ export async function runAgentForGroup(
pairedExecutionStatus === 'succeeded' &&
pairedSawOutput
) {
const completedRole = roomRoleContext?.role ?? 'owner';
const finishedCheck = getPairedTaskById(pairedExecutionContext.task.id);
if (finishedCheck?.status !== 'completed') {
const skipGenericFollowUpForInlineFinalize =
completedRole === 'reviewer' && finishedCheck?.status === 'merge_ready';
if (skipGenericFollowUpForInlineFinalize) {
log.info(
{
taskId: pairedExecutionContext.task.id,
role: completedRole,
taskStatus: finishedCheck?.status ?? null,
},
'Skipping generic follow-up after reviewer approval because inline finalize will handle the owner handoff',
);
} else if (finishedCheck?.status !== 'completed') {
deps.queue.enqueueMessageCheck(chatJid);
}
}

View File

@@ -391,6 +391,27 @@ describe('paired execution context', () => {
// Should not throw; just logs.
});
it('ignores late completions after the paired task is already completed', () => {
vi.mocked(db.getPairedTaskById).mockReturnValue(
buildPairedTask({
status: 'completed',
completion_reason: 'done',
}),
);
completePairedExecutionContext({
taskId: 'task-1',
role: 'owner',
status: 'succeeded',
summary: 'DONE',
});
expect(db.updatePairedTask).not.toHaveBeenCalled();
expect(
pairedWorkspaceManager.markPairedTaskReviewReady,
).not.toHaveBeenCalled();
});
it('completes owner finalize when only the commit object changed after approval', () => {
const repoDir = createCanonicalRepoWithCommit('reviewed');
const approvedSourceRef = resolveTreeRef(repoDir);

View File

@@ -429,6 +429,18 @@ export function completePairedExecutionContext(args: {
const task = getPairedTaskById(taskId);
if (!task) return;
if (task.status === 'completed') {
logger.info(
{
taskId,
role,
status,
completionReason: task.completion_reason ?? null,
},
'Ignoring late paired execution completion for an already completed task',
);
return;
}
// On failure: for reviewers, still check verdict from summary — output may
// have been delivered even though the executor classified it as failed