From 5f28550551d0f57916d428a7d9780b4297053a7e Mon Sep 17 00:00:00 2001 From: Alex Newman Date: Mon, 23 Feb 2026 16:41:28 -0500 Subject: [PATCH] MAESTRO: fix MCP type coercion for batch endpoints, add defensive observation error handling Add string-to-array coercion for ids and memorySessionIds in DataRoutes.ts batch endpoints so MCP clients sending "[1,2,3]" or "1,2,3" instead of native arrays no longer get 400 errors. Wrap observation storage path in SessionRoutes.ts with try/catch returning 200 on recoverable errors instead of 500, preventing hook breakage. Co-Authored-By: Claude Opus 4.6 --- src/services/worker/http/routes/DataRoutes.ts | 14 +- .../worker/http/routes/SessionRoutes.ts | 102 ++++----- .../http/routes/data-routes-coercion.test.ts | 195 ++++++++++++++++++ 3 files changed, 261 insertions(+), 50 deletions(-) create mode 100644 tests/worker/http/routes/data-routes-coercion.test.ts diff --git a/src/services/worker/http/routes/DataRoutes.ts b/src/services/worker/http/routes/DataRoutes.ts index 54853379..ab1a4ec3 100644 --- a/src/services/worker/http/routes/DataRoutes.ts +++ b/src/services/worker/http/routes/DataRoutes.ts @@ -114,7 +114,12 @@ export class DataRoutes extends BaseRouteHandler { * Body: { ids: number[], orderBy?: 'date_desc' | 'date_asc', limit?: number, project?: string } */ private handleGetObservationsByIds = this.wrapHandler((req: Request, res: Response): void => { - const { ids, orderBy, limit, project } = req.body; + let { ids, orderBy, limit, project } = req.body; + + // Coerce string-encoded arrays from MCP clients (e.g. "[1,2,3]" or "1,2,3") + if (typeof ids === 'string') { + try { ids = JSON.parse(ids); } catch { ids = ids.split(',').map(Number); } + } if (!ids || !Array.isArray(ids)) { this.badRequest(res, 'ids must be an array of numbers'); @@ -163,7 +168,12 @@ export class DataRoutes extends BaseRouteHandler { * Body: { memorySessionIds: string[] } */ private handleGetSdkSessionsByIds = this.wrapHandler((req: Request, res: Response): void => { - const { memorySessionIds } = req.body; + let { memorySessionIds } = req.body; + + // Coerce string-encoded arrays from MCP clients (e.g. '["a","b"]' or "a,b") + if (typeof memorySessionIds === 'string') { + try { memorySessionIds = JSON.parse(memorySessionIds); } catch { memorySessionIds = memorySessionIds.split(',').map((s: string) => s.trim()); } + } if (!Array.isArray(memorySessionIds)) { this.badRequest(res, 'memorySessionIds must be an array'); diff --git a/src/services/worker/http/routes/SessionRoutes.ts b/src/services/worker/http/routes/SessionRoutes.ts index cd252075..8311fae5 100644 --- a/src/services/worker/http/routes/SessionRoutes.ts +++ b/src/services/worker/http/routes/SessionRoutes.ts @@ -504,57 +504,63 @@ export class SessionRoutes extends BaseRouteHandler { } } - const store = this.dbManager.getSessionStore(); + try { + const store = this.dbManager.getSessionStore(); - // Get or create session - const sessionDbId = store.createSDKSession(contentSessionId, '', ''); - const promptNumber = store.getPromptNumberFromUserPrompts(contentSessionId); + // Get or create session + const sessionDbId = store.createSDKSession(contentSessionId, '', ''); + const promptNumber = store.getPromptNumberFromUserPrompts(contentSessionId); - // Privacy check: skip if user prompt was entirely private - const userPrompt = PrivacyCheckValidator.checkUserPromptPrivacy( - store, - contentSessionId, - promptNumber, - 'observation', - sessionDbId, - { tool_name } - ); - if (!userPrompt) { - res.json({ status: 'skipped', reason: 'private' }); - return; + // Privacy check: skip if user prompt was entirely private + const userPrompt = PrivacyCheckValidator.checkUserPromptPrivacy( + store, + contentSessionId, + promptNumber, + 'observation', + sessionDbId, + { tool_name } + ); + if (!userPrompt) { + res.json({ status: 'skipped', reason: 'private' }); + return; + } + + // Strip memory tags from tool_input and tool_response + const cleanedToolInput = tool_input !== undefined + ? stripMemoryTagsFromJson(JSON.stringify(tool_input)) + : '{}'; + + const cleanedToolResponse = tool_response !== undefined + ? stripMemoryTagsFromJson(JSON.stringify(tool_response)) + : '{}'; + + // Queue observation + this.sessionManager.queueObservation(sessionDbId, { + tool_name, + tool_input: cleanedToolInput, + tool_response: cleanedToolResponse, + prompt_number: promptNumber, + cwd: cwd || (() => { + logger.error('SESSION', 'Missing cwd when queueing observation in SessionRoutes', { + sessionId: sessionDbId, + tool_name + }); + return ''; + })() + }); + + // Ensure SDK agent is running + this.ensureGeneratorRunning(sessionDbId, 'observation'); + + // Broadcast observation queued event + this.eventBroadcaster.broadcastObservationQueued(sessionDbId); + + res.json({ status: 'queued' }); + } catch (error) { + // Return 200 on recoverable errors so the hook doesn't break + logger.error('SESSION', 'Observation storage failed', { contentSessionId, tool_name }, error as Error); + res.json({ stored: false, reason: (error as Error).message }); } - - // Strip memory tags from tool_input and tool_response - const cleanedToolInput = tool_input !== undefined - ? stripMemoryTagsFromJson(JSON.stringify(tool_input)) - : '{}'; - - const cleanedToolResponse = tool_response !== undefined - ? stripMemoryTagsFromJson(JSON.stringify(tool_response)) - : '{}'; - - // Queue observation - this.sessionManager.queueObservation(sessionDbId, { - tool_name, - tool_input: cleanedToolInput, - tool_response: cleanedToolResponse, - prompt_number: promptNumber, - cwd: cwd || (() => { - logger.error('SESSION', 'Missing cwd when queueing observation in SessionRoutes', { - sessionId: sessionDbId, - tool_name - }); - return ''; - })() - }); - - // Ensure SDK agent is running - this.ensureGeneratorRunning(sessionDbId, 'observation'); - - // Broadcast observation queued event - this.eventBroadcaster.broadcastObservationQueued(sessionDbId); - - res.json({ status: 'queued' }); }); /** diff --git a/tests/worker/http/routes/data-routes-coercion.test.ts b/tests/worker/http/routes/data-routes-coercion.test.ts new file mode 100644 index 00000000..fa8eadc3 --- /dev/null +++ b/tests/worker/http/routes/data-routes-coercion.test.ts @@ -0,0 +1,195 @@ +/** + * DataRoutes Type Coercion Tests + * + * Tests that MCP clients sending string-encoded arrays for `ids` and + * `memorySessionIds` are properly coerced before validation. + * + * Mock Justification: + * - Express req/res mocks: Required because route handlers expect Express objects + * - DatabaseManager/SessionStore: Avoids database setup; we test coercion logic, not queries + * - Logger spies: Suppress console output during tests + */ + +import { describe, it, expect, mock, beforeEach, afterEach, spyOn } from 'bun:test'; +import type { Request, Response } from 'express'; +import { logger } from '../../../../src/utils/logger.js'; + +// Mock dependencies before importing DataRoutes +mock.module('../../../../src/shared/paths.js', () => ({ + getPackageRoot: () => '/tmp/test', +})); +mock.module('../../../../src/shared/worker-utils.js', () => ({ + getWorkerPort: () => 37777, +})); + +import { DataRoutes } from '../../../../src/services/worker/http/routes/DataRoutes.js'; + +let loggerSpies: ReturnType[] = []; + +// Helper to create mock req/res +function createMockReqRes(body: any): { req: Partial; res: Partial; jsonSpy: ReturnType; statusSpy: ReturnType } { + const jsonSpy = mock(() => {}); + const statusSpy = mock(() => ({ json: jsonSpy })); + return { + req: { body, path: '/test', query: {} } as Partial, + res: { json: jsonSpy, status: statusSpy } as unknown as Partial, + jsonSpy, + statusSpy, + }; +} + +describe('DataRoutes Type Coercion', () => { + let routes: DataRoutes; + let mockGetObservationsByIds: ReturnType; + let mockGetSdkSessionsBySessionIds: ReturnType; + + beforeEach(() => { + loggerSpies = [ + spyOn(logger, 'info').mockImplementation(() => {}), + spyOn(logger, 'debug').mockImplementation(() => {}), + spyOn(logger, 'warn').mockImplementation(() => {}), + spyOn(logger, 'error').mockImplementation(() => {}), + spyOn(logger, 'failure').mockImplementation(() => {}), + ]; + + mockGetObservationsByIds = mock(() => [{ id: 1 }, { id: 2 }]); + mockGetSdkSessionsBySessionIds = mock(() => [{ id: 'abc' }]); + + const mockDbManager = { + getSessionStore: () => ({ + getObservationsByIds: mockGetObservationsByIds, + getSdkSessionsBySessionIds: mockGetSdkSessionsBySessionIds, + }), + }; + + routes = new DataRoutes( + {} as any, // paginationHelper + mockDbManager as any, + {} as any, // sessionManager + {} as any, // sseBroadcaster + {} as any, // workerService + Date.now() + ); + }); + + afterEach(() => { + loggerSpies.forEach(spy => spy.mockRestore()); + mock.restore(); + }); + + describe('handleGetObservationsByIds — ids coercion', () => { + // Access the handler via setupRoutes + let handler: (req: Request, res: Response) => void; + + beforeEach(() => { + const mockApp = { + get: mock(() => {}), + post: mock((path: string, fn: any) => { + if (path === '/api/observations/batch') handler = fn; + }), + delete: mock(() => {}), + }; + routes.setupRoutes(mockApp as any); + }); + + it('should accept a native array of numbers', () => { + const { req, res, jsonSpy } = createMockReqRes({ ids: [1, 2, 3] }); + handler(req as Request, res as Response); + + expect(mockGetObservationsByIds).toHaveBeenCalledWith([1, 2, 3], expect.anything()); + expect(jsonSpy).toHaveBeenCalled(); + }); + + it('should coerce a JSON-encoded string array "[1,2,3]" to native array', () => { + const { req, res, jsonSpy } = createMockReqRes({ ids: '[1,2,3]' }); + handler(req as Request, res as Response); + + expect(mockGetObservationsByIds).toHaveBeenCalledWith([1, 2, 3], expect.anything()); + expect(jsonSpy).toHaveBeenCalled(); + }); + + it('should coerce a comma-separated string "1,2,3" to native array', () => { + const { req, res, jsonSpy } = createMockReqRes({ ids: '1,2,3' }); + handler(req as Request, res as Response); + + expect(mockGetObservationsByIds).toHaveBeenCalledWith([1, 2, 3], expect.anything()); + expect(jsonSpy).toHaveBeenCalled(); + }); + + it('should reject non-integer values after coercion', () => { + const { req, res, statusSpy } = createMockReqRes({ ids: 'foo,bar' }); + handler(req as Request, res as Response); + + // NaN values should fail the Number.isInteger check + expect(statusSpy).toHaveBeenCalledWith(400); + }); + + it('should reject missing ids', () => { + const { req, res, statusSpy } = createMockReqRes({}); + handler(req as Request, res as Response); + + expect(statusSpy).toHaveBeenCalledWith(400); + }); + + it('should return empty array for empty ids array', () => { + const { req, res, jsonSpy } = createMockReqRes({ ids: [] }); + handler(req as Request, res as Response); + + expect(jsonSpy).toHaveBeenCalledWith([]); + }); + }); + + describe('handleGetSdkSessionsByIds — memorySessionIds coercion', () => { + let handler: (req: Request, res: Response) => void; + + beforeEach(() => { + const mockApp = { + get: mock(() => {}), + post: mock((path: string, fn: any) => { + if (path === '/api/sdk-sessions/batch') handler = fn; + }), + delete: mock(() => {}), + }; + routes.setupRoutes(mockApp as any); + }); + + it('should accept a native array of strings', () => { + const { req, res, jsonSpy } = createMockReqRes({ memorySessionIds: ['abc', 'def'] }); + handler(req as Request, res as Response); + + expect(mockGetSdkSessionsBySessionIds).toHaveBeenCalledWith(['abc', 'def']); + expect(jsonSpy).toHaveBeenCalled(); + }); + + it('should coerce a JSON-encoded string array to native array', () => { + const { req, res, jsonSpy } = createMockReqRes({ memorySessionIds: '["abc","def"]' }); + handler(req as Request, res as Response); + + expect(mockGetSdkSessionsBySessionIds).toHaveBeenCalledWith(['abc', 'def']); + expect(jsonSpy).toHaveBeenCalled(); + }); + + it('should coerce a comma-separated string to native array', () => { + const { req, res, jsonSpy } = createMockReqRes({ memorySessionIds: 'abc,def' }); + handler(req as Request, res as Response); + + expect(mockGetSdkSessionsBySessionIds).toHaveBeenCalledWith(['abc', 'def']); + expect(jsonSpy).toHaveBeenCalled(); + }); + + it('should trim whitespace from comma-separated values', () => { + const { req, res, jsonSpy } = createMockReqRes({ memorySessionIds: 'abc, def , ghi' }); + handler(req as Request, res as Response); + + expect(mockGetSdkSessionsBySessionIds).toHaveBeenCalledWith(['abc', 'def', 'ghi']); + expect(jsonSpy).toHaveBeenCalled(); + }); + + it('should reject non-array, non-string values', () => { + const { req, res, statusSpy } = createMockReqRes({ memorySessionIds: 42 }); + handler(req as Request, res as Response); + + expect(statusSpy).toHaveBeenCalledWith(400); + }); + }); +});