fix: address all review feedback — unify AIMessage, widen tool support, use contracts, add input validation

- Unify AIMessage type: 'tool' role + toolCalls/toolCallId now on base type
- AIRequestOptions now includes tools/toolChoice for non-streaming tool calling
- AIMessageWithTools/AIRequestOptionsWithTools kept as deprecated aliases
- AIServiceConfig.conversationService typed as IAIConversationService
- buildAIRoutes depends on IAIService + IAIConversationService contracts
- Routes validate message role (system/user/assistant/tool) and content
- GET /conversations parses and validates limit from query string
- POST /conversations/:id/messages validates role before persisting
- 60 service tests + 22 contract tests = 82 total passing

Agent-Logs-Url: https://github.com/objectstack-ai/spec/sessions/be512058-7717-4f5d-b4ba-9861ca48a4e9

Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>

Author: Copilot

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

@@ -365,7 +365,7 @@ describe('AI Routes', () => {
   });
 
   it('should build all expected routes', () => {
-    const routes = buildAIRoutes(service, silentLogger);
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
     expect(routes.length).toBe(8);
 
     const paths = routes.map(r => `${r.method} ${r.path}`);
@@ -380,7 +380,7 @@ describe('AI Routes', () => {
   });
 
   it('POST /api/v1/ai/chat should return chat result', async () => {
-    const routes = buildAIRoutes(service, silentLogger);
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
     const chatRoute = routes.find(r => r.path === '/api/v1/ai/chat')!;
 
     const response = await chatRoute.handler({
@@ -392,15 +392,15 @@ describe('AI Routes', () => {
   });
 
   it('POST /api/v1/ai/chat should return 400 without messages', async () => {
-    const routes = buildAIRoutes(service, silentLogger);
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
     const chatRoute = routes.find(r => r.path === '/api/v1/ai/chat')!;
 
     const response = await chatRoute.handler({ body: {} });
     expect(response.status).toBe(400);
   });
 
   it('POST /api/v1/ai/chat/stream should return streaming response', async () => {
-    const routes = buildAIRoutes(service, silentLogger);
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
     const streamRoute = routes.find(r => r.path === '/api/v1/ai/chat/stream')!;
 
     const response = await streamRoute.handler({
@@ -420,7 +420,7 @@ describe('AI Routes', () => {
   });
 
   it('POST /api/v1/ai/complete should return completion result', async () => {
-    const routes = buildAIRoutes(service, silentLogger);
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
     const completeRoute = routes.find(r => r.path === '/api/v1/ai/complete')!;
 
     const response = await completeRoute.handler({
@@ -432,15 +432,15 @@ describe('AI Routes', () => {
   });
 
   it('POST /api/v1/ai/complete should return 400 without prompt', async () => {
-    const routes = buildAIRoutes(service, silentLogger);
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
     const completeRoute = routes.find(r => r.path === '/api/v1/ai/complete')!;
 
     const response = await completeRoute.handler({ body: {} });
     expect(response.status).toBe(400);
   });
 
   it('GET /api/v1/ai/models should return model list', async () => {
-    const routes = buildAIRoutes(service, silentLogger);
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
     const modelsRoute = routes.find(r => r.path === '/api/v1/ai/models')!;
 
     const response = await modelsRoute.handler({});
@@ -449,7 +449,7 @@ describe('AI Routes', () => {
   });
 
   it('POST /api/v1/ai/conversations should create conversation', async () => {
-    const routes = buildAIRoutes(service, silentLogger);
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
     const createRoute = routes.find(r => r.method === 'POST' && r.path === '/api/v1/ai/conversations')!;
 
     const response = await createRoute.handler({
@@ -461,7 +461,7 @@ describe('AI Routes', () => {
   });
 
   it('GET /api/v1/ai/conversations should list conversations', async () => {
-    const routes = buildAIRoutes(service, silentLogger);
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
     const createRoute = routes.find(r => r.method === 'POST' && r.path === '/api/v1/ai/conversations')!;
     const listRoute = routes.find(r => r.method === 'GET' && r.path === '/api/v1/ai/conversations')!;
 
@@ -474,7 +474,7 @@ describe('AI Routes', () => {
   });
 
   it('POST /api/v1/ai/conversations/:id/messages should add message', async () => {
-    const routes = buildAIRoutes(service, silentLogger);
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
     const createRoute = routes.find(r => r.method === 'POST' && r.path === '/api/v1/ai/conversations')!;
     const addMsgRoute = routes.find(r => r.path === '/api/v1/ai/conversations/:id/messages')!;
 
@@ -491,7 +491,7 @@ describe('AI Routes', () => {
   });
 
   it('POST /api/v1/ai/conversations/:id/messages should return 404 for unknown conversation', async () => {
-    const routes = buildAIRoutes(service, silentLogger);
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
     const addMsgRoute = routes.find(r => r.path === '/api/v1/ai/conversations/:id/messages')!;
 
     const response = await addMsgRoute.handler({
@@ -503,7 +503,7 @@ describe('AI Routes', () => {
   });
 
   it('DELETE /api/v1/ai/conversations/:id should delete conversation', async () => {
-    const routes = buildAIRoutes(service, silentLogger);
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
     const createRoute = routes.find(r => r.method === 'POST' && r.path === '/api/v1/ai/conversations')!;
     const deleteRoute = routes.find(r => r.path === '/api/v1/ai/conversations/:id')!;
 
@@ -513,6 +513,115 @@ describe('AI Routes', () => {
     const response = await deleteRoute.handler({ params: { id: convId } });
     expect(response.status).toBe(204);
   });
+
+  // ── Message validation ───────────────────────────────────────
+
+  it('POST /api/v1/ai/chat should return 400 for messages with invalid role', async () => {
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
+    const chatRoute = routes.find(r => r.path === '/api/v1/ai/chat')!;
+
+    const response = await chatRoute.handler({
+      body: { messages: [{ role: 'invalid', content: 'Hi' }] },
+    });
+
+    expect(response.status).toBe(400);
+    expect((response.body as any).error).toContain('message.role');
+  });
+
+  it('POST /api/v1/ai/chat should return 400 for messages with non-string content', async () => {
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
+    const chatRoute = routes.find(r => r.path === '/api/v1/ai/chat')!;
+
+    const response = await chatRoute.handler({
+      body: { messages: [{ role: 'user', content: 123 }] },
+    });
+
+    expect(response.status).toBe(400);
+    expect((response.body as any).error).toContain('content');
+  });
+
+  it('POST /api/v1/ai/conversations/:id/messages should return 400 for invalid role', async () => {
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
+    const createRoute = routes.find(r => r.method === 'POST' && r.path === '/api/v1/ai/conversations')!;
+    const addMsgRoute = routes.find(r => r.path === '/api/v1/ai/conversations/:id/messages')!;
+
+    const created = await createRoute.handler({ body: {} });
+    const convId = (created.body as any).id;
+
+    const response = await addMsgRoute.handler({
+      params: { id: convId },
+      body: { role: 'invalid_role', content: 'Hi' },
+    });
+
+    expect(response.status).toBe(400);
+    expect((response.body as any).error).toContain('message.role');
+  });
+
+  it('POST /api/v1/ai/conversations/:id/messages should return 400 for missing content', async () => {
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
+    const addMsgRoute = routes.find(r => r.path === '/api/v1/ai/conversations/:id/messages')!;
+
+    const response = await addMsgRoute.handler({
+      params: { id: 'conv_1' },
+      body: { role: 'user' },
+    });
+
+    expect(response.status).toBe(400);
+    expect((response.body as any).error).toContain('content');
+  });
+
+  // ── Limit parsing ───────────────────────────────────────────
+
+  it('GET /api/v1/ai/conversations should parse limit from query string', async () => {
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
+    const createRoute = routes.find(r => r.method === 'POST' && r.path === '/api/v1/ai/conversations')!;
+    const listRoute = routes.find(r => r.method === 'GET' && r.path === '/api/v1/ai/conversations')!;
+
+    await createRoute.handler({ body: { title: 'C1' } });
+    await createRoute.handler({ body: { title: 'C2' } });
+    await createRoute.handler({ body: { title: 'C3' } });
+
+    const response = await listRoute.handler({ query: { limit: '2' } });
+    expect(response.status).toBe(200);
+    expect((response.body as any).conversations).toHaveLength(2);
+  });
+
+  it('GET /api/v1/ai/conversations should return 400 for invalid limit', async () => {
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
+    const listRoute = routes.find(r => r.method === 'GET' && r.path === '/api/v1/ai/conversations')!;
+
+    const response = await listRoute.handler({ query: { limit: 'abc' } });
+    expect(response.status).toBe(400);
+    expect((response.body as any).error).toContain('limit');
+  });
+
+  it('GET /api/v1/ai/conversations should return 400 for negative limit', async () => {
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
+    const listRoute = routes.find(r => r.method === 'GET' && r.path === '/api/v1/ai/conversations')!;
+
+    const response = await listRoute.handler({ query: { limit: '-1' } });
+    expect(response.status).toBe(400);
+    expect((response.body as any).error).toContain('limit');
+  });
+
+  // ── Tool message in chat ────────────────────────────────────
+
+  it('POST /api/v1/ai/chat should accept tool role messages', async () => {
+    const routes = buildAIRoutes(service, service.conversationService, silentLogger);
+    const chatRoute = routes.find(r => r.path === '/api/v1/ai/chat')!;
+
+    const response = await chatRoute.handler({
+      body: {
+        messages: [
+          { role: 'user', content: 'What is the weather?' },
+          { role: 'assistant', content: '' },
+          { role: 'tool', content: '{"temp": 22}', toolCallId: 'call_1' },
+        ],
+      },
+    });
+
+    expect(response.status).toBe(200);
+  });
 });
 
 // ─────────────────────────────────────────────────────────────────

packages/services/service-ai/src/adapters/memory-adapter.ts

@@ -3,7 +3,6 @@
 import type {
   AIMessage,
   AIRequestOptions,
-  AIRequestOptionsWithTools,
   AIResult,
   AIStreamEvent,
 } from '@objectstack/spec/contracts';
@@ -41,7 +40,7 @@ export class MemoryLLMAdapter implements LLMAdapter {
 
   async *streamChat(
     messages: AIMessage[],
-    _options?: AIRequestOptionsWithTools,
+    _options?: AIRequestOptions,
   ): AsyncIterable<AIStreamEvent> {
     const result = await this.chat(messages);
     // Emit word-by-word deltas for realistic streaming simulation

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

@@ -3,7 +3,6 @@
 import type {
   AIMessage,
   AIRequestOptions,
-  AIRequestOptionsWithTools,
   AIResult,
   AIStreamEvent,
 } from '@objectstack/spec/contracts';
@@ -24,7 +23,7 @@ export interface LLMAdapter {
   /**
    * Generate a chat completion.
    * @param messages - Conversation messages
-   * @param options  - Request configuration
+   * @param options  - Request configuration (includes tool definitions)
    */
   chat(messages: AIMessage[], options?: AIRequestOptions): Promise<AIResult>;
 
@@ -39,7 +38,7 @@ export interface LLMAdapter {
    * Stream a chat completion as an async iterable of events.
    * Implementations that do not support streaming may omit this method.
    */
-  streamChat?(messages: AIMessage[], options?: AIRequestOptionsWithTools): AsyncIterable<AIStreamEvent>;
+  streamChat?(messages: AIMessage[], options?: AIRequestOptions): AsyncIterable<AIStreamEvent>;
 
   /**
    * Generate embedding vectors.

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

@@ -3,10 +3,10 @@
 import type {
   AIMessage,
   AIRequestOptions,
-  AIRequestOptionsWithTools,
   AIResult,
   AIStreamEvent,
   IAIService,
+  IAIConversationService,
 } from '@objectstack/spec/contracts';
 import type { Logger } from '@objectstack/spec/contracts';
 import { createLogger } from '@objectstack/core';
@@ -26,7 +26,7 @@ export interface AIServiceConfig {
   /** Pre-registered tools. */
   toolRegistry?: ToolRegistry;
   /** Conversation service (defaults to InMemoryConversationService). */
-  conversationService?: InMemoryConversationService;
+  conversationService?: IAIConversationService;
 }
 
 /**
@@ -39,7 +39,7 @@ export interface AIServiceConfig {
  * |:---|:---|
  * | {@link LLMAdapter} | LLM provider abstraction (chat, complete, stream, embed) |
  * | {@link ToolRegistry} | Tool definition storage & execution |
- * | {@link InMemoryConversationService} | Conversation CRUD & message persistence |
+ * | {@link IAIConversationService} | Conversation CRUD & message persistence |
  *
  * The service is registered as `'ai'` in the kernel service registry by
  * the {@link AIServicePlugin}.
@@ -48,7 +48,7 @@ export class AIService implements IAIService {
   private readonly adapter: LLMAdapter;
   private readonly logger: Logger;
   readonly toolRegistry: ToolRegistry;
-  readonly conversationService: InMemoryConversationService;
+  readonly conversationService: IAIConversationService;
 
   constructor(config: AIServiceConfig = {}) {
     this.adapter = config.adapter ?? new MemoryLLMAdapter();
@@ -81,7 +81,7 @@ export class AIService implements IAIService {
 
   async *streamChat(
     messages: AIMessage[],
-    options?: AIRequestOptionsWithTools,
+    options?: AIRequestOptions,
   ): AsyncIterable<AIStreamEvent> {
     this.logger.debug('[AI] streamChat', { messageCount: messages.length, model: options?.model });
 

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

@@ -96,7 +96,7 @@ export class AIServicePlugin implements Plugin {
     await ctx.trigger('ai:ready', this.service);
 
     // Build and expose route definitions
-    const routes = buildAIRoutes(this.service, ctx.logger);
+    const routes = buildAIRoutes(this.service, this.service.conversationService, ctx.logger);
 
     // Trigger hook so HTTP server plugins can mount these routes
     await ctx.trigger('ai:routes', routes);