diff --git a/src/message-agent-executor.test.ts b/src/message-agent-executor.test.ts index 9972ea7..fe92ff7 100644 --- a/src/message-agent-executor.test.ts +++ b/src/message-agent-executor.test.ts @@ -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', () => { diff --git a/src/message-agent-executor.ts b/src/message-agent-executor.ts index 66220b6..44661f1 100644 --- a/src/message-agent-executor.ts +++ b/src/message-agent-executor.ts @@ -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); } } diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index c441299..af7b99c 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -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); diff --git a/src/paired-execution-context.ts b/src/paired-execution-context.ts index 86dd418..966702d 100644 --- a/src/paired-execution-context.ts +++ b/src/paired-execution-context.ts @@ -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