Serialize merge-ready owner finalize handoff
This commit is contained in:
@@ -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', () => {
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user