Skip to content

Commit ce1aba9

Browse files
os-zhuangclaude
andauthored
feat(service-ai): turn idempotency (turnId) — safe Retry without re-planning (ADR-0013 D1) (#1900)
A user turn becomes an idempotent unit. The client supplies a stable `turnId` per turn (constant across Retry); the server dedups the inbound user message by (conversationId, turnId) and short-circuits the stored reply when the turn already completed, instead of writing a duplicate user row and re-running the tool loop (which re-plans → renamed objects, orphan drafts). - ai_messages: + turn_id column + (conversation_id, turn_id) index (auto-reconciled by the SQL driver; no hand-written migration). - contract: turnId on ToolExecutionContext; addMessage(…, turnId) + new getTurnState(conversationId, turnId) on IAIConversationService. - conversation services (objectql + in-memory): persist turn_id on every message of a turn; getTurnState reports userExists + the final reply (an assistant message with no pending tool calls). - ai-service: dedup + short-circuit in BOTH chatWithToolsImpl and streamChatWithTools; every turn message tagged with turnId. - agent-routes: read body.turnId into toolExecutionContext. Tests: +6 dedup/short-circuit (chat + stream) and +4 getTurnState cases; full service-ai suite green (385). Cross-repo: pairs with objectui chat client (turnId in request body) and a cloud SHA bump, per the ADR-0012 cross-repo recipe. Refs cloud#334. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent f0d544c commit ce1aba9

8 files changed

Lines changed: 425 additions & 9 deletions

File tree

packages/services/service-ai/src/__tests__/ai-service.test.ts

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -518,6 +518,165 @@ describe('AIService', () => {
518518
);
519519
expect(result.content).toBe('still ok');
520520
});
521+
522+
// ─── Turn idempotency (ADR-0013 D1) ─────────────────────────────
523+
524+
it('chatWithTools re-sending the same turnId returns the stored reply without re-running the model', async () => {
525+
const conversationService = new InMemoryConversationService();
526+
let calls = 0;
527+
const adapter: LLMAdapter = {
528+
name: 'turn-idem',
529+
chat: async () => {
530+
calls += 1;
531+
return { content: `reply #${calls}`, model: 'test' };
532+
},
533+
complete: async () => ({ content: '' }),
534+
};
535+
const service = new AIService({ adapter, conversationService, logger: silentLogger });
536+
const conv = await conversationService.create();
537+
const ctx = { toolExecutionContext: { conversationId: conv.id, turnId: 'turn-1' } };
538+
539+
const first = await service.chatWithTools([{ role: 'user', content: 'hi' }], ctx);
540+
const second = await service.chatWithTools([{ role: 'user', content: 'hi' }], ctx);
541+
542+
// Adapter ran exactly once; the retry short-circuited the stored reply.
543+
expect(calls).toBe(1);
544+
expect(first.content).toBe('reply #1');
545+
expect(second.content).toBe('reply #1');
546+
547+
// No duplicate user/assistant rows — still just the one turn.
548+
const after = await conversationService.get(conv.id);
549+
expect(after?.messages.map((m) => m.role)).toEqual(['user', 'assistant']);
550+
});
551+
552+
it('chatWithTools re-sending the same turnId does NOT re-execute tools', async () => {
553+
const conversationService = new InMemoryConversationService();
554+
const toolRegistry = new ToolRegistry();
555+
let toolCalls = 0;
556+
toolRegistry.register(
557+
{ name: 'echo', description: 'echo', parameters: {} as any },
558+
async (input: any) => {
559+
toolCalls += 1;
560+
return JSON.stringify({ ok: true, input });
561+
},
562+
);
563+
let modelCalls = 0;
564+
const adapter: LLMAdapter = {
565+
name: 'turn-idem-tools',
566+
chat: async () => {
567+
modelCalls += 1;
568+
if (modelCalls === 1) {
569+
return {
570+
content: '',
571+
model: 'test',
572+
toolCalls: [{ type: 'tool-call' as const, toolCallId: 'tc1', toolName: 'echo', input: { x: 1 } }],
573+
};
574+
}
575+
return { content: 'all done', model: 'test' };
576+
},
577+
complete: async () => ({ content: '' }),
578+
};
579+
const service = new AIService({ adapter, conversationService, toolRegistry, logger: silentLogger });
580+
const conv = await conversationService.create();
581+
const ctx = { toolExecutionContext: { conversationId: conv.id, turnId: 'turn-1' } };
582+
583+
await service.chatWithTools([{ role: 'user', content: 'use echo' }], ctx);
584+
expect(toolCalls).toBe(1);
585+
586+
// Retry the SAME completed turn → no further tool execution / planning.
587+
const retry = await service.chatWithTools([{ role: 'user', content: 'use echo' }], ctx);
588+
expect(retry.content).toBe('all done');
589+
expect(toolCalls).toBe(1);
590+
expect(modelCalls).toBe(2); // unchanged from the first turn's two rounds
591+
});
592+
593+
it('chatWithTools dedups the inbound user message when re-running an INCOMPLETE turn', async () => {
594+
const conversationService = new InMemoryConversationService();
595+
// Seed an incomplete turn: a user message exists, but no assistant reply.
596+
const conv = await conversationService.create();
597+
await conversationService.addMessage(conv.id, { role: 'user', content: 'hi' }, undefined, 'turn-1');
598+
599+
let calls = 0;
600+
const adapter: LLMAdapter = {
601+
name: 'turn-idem-incomplete',
602+
chat: async () => {
603+
calls += 1;
604+
return { content: 'recovered reply', model: 'test' };
605+
},
606+
complete: async () => ({ content: '' }),
607+
};
608+
const service = new AIService({ adapter, conversationService, logger: silentLogger });
609+
610+
const result = await service.chatWithTools(
611+
[{ role: 'user', content: 'hi' }],
612+
{ toolExecutionContext: { conversationId: conv.id, turnId: 'turn-1' } },
613+
);
614+
615+
// Re-ran (turn was incomplete) but did NOT write a second user row.
616+
expect(calls).toBe(1);
617+
expect(result.content).toBe('recovered reply');
618+
const after = await conversationService.get(conv.id);
619+
expect(after?.messages.map((m) => m.role)).toEqual(['user', 'assistant']);
620+
});
621+
622+
it('chatWithTools without a turnId keeps the legacy behaviour (each call is a fresh turn)', async () => {
623+
const conversationService = new InMemoryConversationService();
624+
let calls = 0;
625+
const adapter: LLMAdapter = {
626+
name: 'no-turn',
627+
chat: async () => {
628+
calls += 1;
629+
return { content: `reply #${calls}`, model: 'test' };
630+
},
631+
complete: async () => ({ content: '' }),
632+
};
633+
const service = new AIService({ adapter, conversationService, logger: silentLogger });
634+
const conv = await conversationService.create();
635+
const ctx = { toolExecutionContext: { conversationId: conv.id } };
636+
637+
await service.chatWithTools([{ role: 'user', content: 'hi' }], ctx);
638+
await service.chatWithTools([{ role: 'user', content: 'hi' }], ctx);
639+
640+
// No idempotency key → both calls run and persist independently.
641+
expect(calls).toBe(2);
642+
const after = await conversationService.get(conv.id);
643+
expect(after?.messages.map((m) => m.role)).toEqual(['user', 'assistant', 'user', 'assistant']);
644+
});
645+
646+
it('streamChatWithTools re-sending the same turnId replays the stored reply without re-running the model', async () => {
647+
const conversationService = new InMemoryConversationService();
648+
let calls = 0;
649+
const adapter: LLMAdapter = {
650+
name: 'turn-idem-stream',
651+
chat: async () => {
652+
calls += 1;
653+
return { content: 'streamed reply', model: 'test' };
654+
},
655+
complete: async () => ({ content: '' }),
656+
};
657+
const service = new AIService({ adapter, conversationService, logger: silentLogger });
658+
const conv = await conversationService.create();
659+
const ctx = { toolExecutionContext: { conversationId: conv.id, turnId: 'turn-1' } };
660+
661+
const drain = async () => {
662+
const events: TextStreamPart<ToolSet>[] = [];
663+
for await (const ev of service.streamChatWithTools([{ role: 'user', content: 'hi' }], ctx)) {
664+
events.push(ev);
665+
}
666+
return events;
667+
};
668+
669+
await drain();
670+
const second = await drain();
671+
672+
expect(calls).toBe(1); // retry short-circuited; adapter not called again
673+
const text = second.find((e) => (e as { type: string }).type === 'text-delta');
674+
expect(text && (text as any).text).toBe('streamed reply');
675+
expect(second[second.length - 1].type).toBe('finish');
676+
677+
const after = await conversationService.get(conv.id);
678+
expect(after?.messages.map((m) => m.role)).toEqual(['user', 'assistant']);
679+
});
521680
});
522681

523682
// ─────────────────────────────────────────────────────────────────

packages/services/service-ai/src/__tests__/objectql-conversation-service.test.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,60 @@ describe('ObjectQLConversationService', () => {
415415
expect(fetched!.metadata).toBeUndefined();
416416
});
417417

418+
// ── Turn idempotency (ADR-0013 D1) ──────────────────────────────
419+
420+
it('persists turn_id on every message of a turn', async () => {
421+
const conv = await service.create();
422+
await service.addMessage(conv.id, { role: 'user', content: 'hi' }, undefined, 'turn-1');
423+
await service.addMessage(conv.id, { role: 'assistant', content: 'reply' }, undefined, 'turn-1');
424+
425+
const rows = await engine.find('ai_messages', { where: { conversation_id: conv.id } });
426+
expect(rows).toHaveLength(2);
427+
expect(rows.every((r: any) => r.turn_id === 'turn-1')).toBe(true);
428+
});
429+
430+
it('getTurnState reports a completed turn (user + final assistant reply)', async () => {
431+
const conv = await service.create();
432+
await service.addMessage(conv.id, { role: 'user', content: 'hi' }, undefined, 'turn-1');
433+
await service.addMessage(
434+
conv.id,
435+
{ role: 'assistant', content: 'final answer' },
436+
{ model: 'test', promptTokens: 3, completionTokens: 5, totalTokens: 8 },
437+
'turn-1',
438+
);
439+
440+
const state = await service.getTurnState(conv.id, 'turn-1');
441+
expect(state.userExists).toBe(true);
442+
expect(state.reply?.content).toBe('final answer');
443+
expect(state.reply?.model).toBe('test');
444+
expect(state.reply?.usage?.totalTokens).toBe(8);
445+
});
446+
447+
it('getTurnState treats a tool-call-only assistant turn as NOT a final reply', async () => {
448+
const conv = await service.create();
449+
await service.addMessage(conv.id, { role: 'user', content: 'use a tool' }, undefined, 'turn-1');
450+
// Assistant turn that only requested tools — carries tool_calls, no final text.
451+
await service.addMessage(
452+
conv.id,
453+
{
454+
role: 'assistant',
455+
content: [{ type: 'tool-call', toolCallId: 'tc1', toolName: 'echo', input: {} }],
456+
} as ModelMessage,
457+
undefined,
458+
'turn-1',
459+
);
460+
461+
const state = await service.getTurnState(conv.id, 'turn-1');
462+
expect(state.userExists).toBe(true);
463+
expect(state.reply).toBeNull(); // turn never produced a final reply
464+
});
465+
466+
it('getTurnState returns no reply / no user for an unknown turn', async () => {
467+
const conv = await service.create();
468+
const state = await service.getTurnState(conv.id, 'never-seen');
469+
expect(state).toEqual({ userExists: false, reply: null });
470+
});
471+
418472
it('should handle invalid JSON in tool_calls gracefully', async () => {
419473
const conv = await service.create();
420474
await service.addMessage(conv.id, { role: 'assistant', content: 'checking tools' });

packages/services/service-ai/src/ai-service.ts

Lines changed: 53 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -352,9 +352,10 @@ export class AIService implements IAIService {
352352
conversationId: string,
353353
message: ModelMessage,
354354
extras?: MessageObservability,
355+
turnId?: string,
355356
): Promise<void> {
356357
try {
357-
await this.conversationService.addMessage(conversationId, message, extras);
358+
await this.conversationService.addMessage(conversationId, message, extras, turnId);
358359
} catch (err) {
359360
this.logger.warn('[AI] persist message failed', {
360361
conversationId,
@@ -565,13 +566,34 @@ export class AIService implements IAIService {
565566
// Working copy of the conversation
566567
const conversation = [...messages];
567568

569+
// Turn idempotency (ADR-0013 D1). When the client supplies a stable
570+
// turnId, dedup the inbound user message and short-circuit a turn that
571+
// already completed instead of re-running the tool loop / re-planning.
572+
const turnId = toolExecutionContext?.turnId;
573+
568574
// Persist the inbound user turn when a conversationId is supplied.
569575
// Only the last message is written — callers are assumed to pass
570576
// prior history alongside the new turn; we don't diff.
571577
if (conversationId && messages.length > 0) {
572578
const last = messages[messages.length - 1];
573579
if (last && (last as { role?: string }).role === 'user') {
574-
await this.persistMessage(conversationId, last);
580+
if (turnId) {
581+
const state = await this.conversationService.getTurnState(conversationId, turnId);
582+
// Completed turn re-requested → return the stored reply, no tools.
583+
if (state.reply) {
584+
this.logger.debug('[AI] chatWithTools short-circuit completed turn', { turnId });
585+
return autoCreatedConversationId
586+
? { ...state.reply, conversationId: autoCreatedConversationId }
587+
: state.reply;
588+
}
589+
// Incomplete/failed turn re-requested → don't write a duplicate
590+
// user row; fall through and re-run (D2 handles "already succeeded").
591+
if (!state.userExists) {
592+
await this.persistMessage(conversationId, last, undefined, turnId);
593+
}
594+
} else {
595+
await this.persistMessage(conversationId, last);
596+
}
575597
}
576598
}
577599

@@ -602,6 +624,7 @@ export class AIService implements IAIService {
602624
content: result.content,
603625
} as ModelMessage,
604626
turnObservability,
627+
turnId,
605628
);
606629
void this.summarizeConversation(conversationId);
607630
}
@@ -627,8 +650,9 @@ export class AIService implements IAIService {
627650
if (conversationId) {
628651
// Attribute usage / latency to the assistant turn that triggered
629652
// the tool calls — the subsequent role:'tool' messages have no
630-
// LLM cost of their own.
631-
await this.persistMessage(conversationId, assistantTurn, turnObservability);
653+
// LLM cost of their own. Tagged with turnId but keeps tool_calls,
654+
// so getTurnState never mistakes it for the turn's final reply.
655+
await this.persistMessage(conversationId, assistantTurn, turnObservability, turnId);
632656
}
633657

634658
// Execute all tool calls in parallel, threading the per-request
@@ -666,7 +690,7 @@ export class AIService implements IAIService {
666690
} as ModelMessage;
667691
conversation.push(toolTurn);
668692
if (conversationId) {
669-
await this.persistMessage(conversationId, toolTurn);
693+
await this.persistMessage(conversationId, toolTurn, undefined, turnId);
670694
}
671695
}
672696

@@ -700,6 +724,7 @@ export class AIService implements IAIService {
700724
content: finalResult.content,
701725
} as ModelMessage,
702726
finalObservability,
727+
turnId,
703728
);
704729
void this.summarizeConversation(conversationId);
705730
}
@@ -760,10 +785,28 @@ export class AIService implements IAIService {
760785
} as TextStreamPart<ToolSet>;
761786
}
762787

788+
// Turn idempotency (ADR-0013 D1) — see chatWithToolsImpl for the rationale.
789+
const turnId = toolExecutionContext?.turnId;
790+
763791
if (conversationId && messages.length > 0) {
764792
const last = messages[messages.length - 1];
765793
if (last && (last as { role?: string }).role === 'user') {
766-
await this.persistMessage(conversationId, last);
794+
if (turnId) {
795+
const state = await this.conversationService.getTurnState(conversationId, turnId);
796+
// Completed turn re-requested → replay the stored reply, no tools.
797+
if (state.reply) {
798+
this.logger.debug('[AI] streamChatWithTools short-circuit completed turn', { turnId });
799+
yield textDeltaPart('stream', state.reply.content);
800+
yield finishPart(state.reply);
801+
return;
802+
}
803+
// Incomplete/failed turn → don't duplicate the user row; re-run.
804+
if (!state.userExists) {
805+
await this.persistMessage(conversationId, last, undefined, turnId);
806+
}
807+
} else {
808+
await this.persistMessage(conversationId, last);
809+
}
767810
}
768811
}
769812

@@ -783,6 +826,7 @@ export class AIService implements IAIService {
783826
content: result.content,
784827
} as ModelMessage,
785828
turnObservability,
829+
turnId,
786830
);
787831
void this.summarizeConversation(conversationId);
788832
}
@@ -805,7 +849,7 @@ export class AIService implements IAIService {
805849
} as ModelMessage;
806850
conversation.push(assistantTurn);
807851
if (conversationId) {
808-
await this.persistMessage(conversationId, assistantTurn, turnObservability);
852+
await this.persistMessage(conversationId, assistantTurn, turnObservability, turnId);
809853
}
810854

811855
// Drain tool PROGRESS events (emitted via `ctx.onProgress` during a
@@ -872,7 +916,7 @@ export class AIService implements IAIService {
872916
} as ModelMessage;
873917
conversation.push(toolTurn);
874918
if (conversationId) {
875-
await this.persistMessage(conversationId, toolTurn);
919+
await this.persistMessage(conversationId, toolTurn, undefined, turnId);
876920
}
877921
}
878922

@@ -899,6 +943,7 @@ export class AIService implements IAIService {
899943
content: result.content,
900944
} as ModelMessage,
901945
finalObservability,
946+
turnId,
902947
);
903948
void this.summarizeConversation(conversationId);
904949
}

0 commit comments

Comments
 (0)