From 16f444f867a9064c1d653da6ca335700ba0ed3f8 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 10 Feb 2026 11:38:42 +0100 Subject: [PATCH 01/11] fix(mcp-server): use random available ports in server tests Replace hardcoded port numbers with dynamically assigned ports to avoid EADDRINUSE errors in CI environments where ports may already be in use. Co-Authored-By: Claude Opus 4.6 --- packages/mcp-server/test/server.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/mcp-server/test/server.test.ts b/packages/mcp-server/test/server.test.ts index a3f51a3c9..872396690 100644 --- a/packages/mcp-server/test/server.test.ts +++ b/packages/mcp-server/test/server.test.ts @@ -2,6 +2,7 @@ import type * as http from 'http'; import type * as net from 'net'; import jsonwebtoken from 'jsonwebtoken'; +import * as net from 'net'; import request from 'supertest'; import createMockForestServerClient from './helpers/forest-server-client'; @@ -10,6 +11,17 @@ import MockServer from './test-utils/mock-server'; import ForestMCPServer from '../src/server'; import { clearSchemaCache } from '../src/utils/schema-fetcher'; +function getAvailablePort(): Promise { + return new Promise((resolve, reject) => { + const srv = net.createServer(); + srv.listen(0, () => { + const { port } = srv.address() as net.AddressInfo; + srv.close(() => resolve(port)); + }); + srv.on('error', reject); + }); +} + function shutDownHttpServer(server: http.Server | undefined): Promise { if (!server) return Promise.resolve(); From b1b519486b1936bcc125a025f12e9989cb38fd1f Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 10 Feb 2026 11:51:58 +0100 Subject: [PATCH 02/11] refactor(mcp-server): extract getAvailablePort to reusable test util Co-Authored-By: Claude Opus 4.6 --- packages/mcp-server/test/server.test.ts | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/packages/mcp-server/test/server.test.ts b/packages/mcp-server/test/server.test.ts index 872396690..a7c35fbca 100644 --- a/packages/mcp-server/test/server.test.ts +++ b/packages/mcp-server/test/server.test.ts @@ -11,17 +11,6 @@ import MockServer from './test-utils/mock-server'; import ForestMCPServer from '../src/server'; import { clearSchemaCache } from '../src/utils/schema-fetcher'; -function getAvailablePort(): Promise { - return new Promise((resolve, reject) => { - const srv = net.createServer(); - srv.listen(0, () => { - const { port } = srv.address() as net.AddressInfo; - srv.close(() => resolve(port)); - }); - srv.on('error', reject); - }); -} - function shutDownHttpServer(server: http.Server | undefined): Promise { if (!server) return Promise.resolve(); From 6f280f9c05f98f82a850079f4f3d35f2108c6a0a Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 10 Feb 2026 12:07:50 +0100 Subject: [PATCH 03/11] fix(mcp-server): fix lint import order in server tests Co-Authored-By: Claude Opus 4.6 --- packages/mcp-server/test/server.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/mcp-server/test/server.test.ts b/packages/mcp-server/test/server.test.ts index a7c35fbca..a3f51a3c9 100644 --- a/packages/mcp-server/test/server.test.ts +++ b/packages/mcp-server/test/server.test.ts @@ -2,7 +2,6 @@ import type * as http from 'http'; import type * as net from 'net'; import jsonwebtoken from 'jsonwebtoken'; -import * as net from 'net'; import request from 'supertest'; import createMockForestServerClient from './helpers/forest-server-client'; From 81539e99336c403863abb5bed2c5cf8c823b64c3 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 10 Feb 2026 10:54:16 +0100 Subject: [PATCH 04/11] fix(mcp-server): add diagnostic logging for 500 errors Improve logging to diagnose production 500 errors (0ms response, no useful info): - Replace fragile res.end wrapping with res.on('finish') in logging middleware - Log JSON-RPC method name in handleMcpRequest - Add stack traces to MCP error catch and global error handler - Log request method/path in global error handler - Use Error level for 5xx, Warn for 4xx in response logging Co-Authored-By: Claude Opus 4.6 --- packages/mcp-server/src/server.ts | 34 ++++++++++++++++--------- packages/mcp-server/test/server.test.ts | 6 +++-- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/packages/mcp-server/src/server.ts b/packages/mcp-server/src/server.ts index 000939b27..8f2be1ad7 100644 --- a/packages/mcp-server/src/server.ts +++ b/packages/mcp-server/src/server.ts @@ -234,7 +234,8 @@ export default class ForestMCPServer { * Logs the request, intercepts the response for error logging, and delegates to the transport. */ private async handleMcpRequest(req: express.Request, res: express.Response): Promise { - this.logger('Info', `Incoming ${req.method} ${req.path}`); + const method = req.body?.method || 'unknown'; + this.logger('Info', `Incoming ${req.method} ${req.path} - method: ${method}`); if (!this.mcpTransport) { throw new Error('MCP transport not initialized'); @@ -328,17 +329,16 @@ export default class ForestMCPServer { app.use((req, res, next) => { const startTime = Date.now(); - // Capture the original end method to log after response is sent - const originalEnd = res.end.bind(res); - res.end = ((chunk?: unknown, encoding?: BufferEncoding | (() => void)) => { + res.on('finish', () => { const duration = Date.now() - startTime; + let level: LogLevel = 'Info'; + if (res.statusCode >= 500) level = 'Error'; + else if (res.statusCode >= 400) level = 'Warn'; this.logger( - 'Info', + level, `[${res.statusCode}] ${req.method} ${req.baseUrl || req.path} - ${duration}ms`, ); - - return originalEnd(chunk, encoding as BufferEncoding); - }) as typeof res.end; + }); next(); }); @@ -373,14 +373,20 @@ export default class ForestMCPServer { }), (req, res) => { this.handleMcpRequest(req, res).catch(error => { - this.logger('Error', `MCP Error: ${error}`); + const mcpMethod = req.body?.method || 'unknown'; + const err = error as Error; + this.logger('Error', `MCP Error on method '${mcpMethod}': ${err.message || error}`); + + if (err.stack) { + this.logger('Error', `Stack: ${err.stack}`); + } if (!res.headersSent) { res.status(500).json({ jsonrpc: '2.0', error: { code: -32603, - message: (error as Error)?.message || 'Internal server error', + message: err?.message || 'Internal server error', }, id: null, }); @@ -395,8 +401,12 @@ export default class ForestMCPServer { const { logger } = this; app.use( // eslint-disable-next-line @typescript-eslint/no-unused-vars - (err: Error, _req: express.Request, res: express.Response, _next: express.NextFunction) => { - logger('Error', `Unhandled error: ${err.message}`); + (err: Error, req: express.Request, res: express.Response, _next: express.NextFunction) => { + logger('Error', `Unhandled error on ${req.method} ${req.path}: ${err.message}`); + + if (err.stack) { + logger('Error', `Stack: ${err.stack}`); + } if (!res.headersSent) { res.status(500).json({ diff --git a/packages/mcp-server/test/server.test.ts b/packages/mcp-server/test/server.test.ts index a3f51a3c9..d51ad5a99 100644 --- a/packages/mcp-server/test/server.test.ts +++ b/packages/mcp-server/test/server.test.ts @@ -2056,7 +2056,7 @@ describe('ForestMCPServer Instance', () => { .set('Accept', 'application/json, text/event-stream') .send({ jsonrpc: '2.0', method: 'tools/list', id: 1 }); - expect(mockLogger).toHaveBeenCalledWith('Info', 'Incoming POST /mcp'); + expect(mockLogger).toHaveBeenCalledWith('Info', 'Incoming POST /mcp - method: tools/list'); }); it('should log tool calls with safe parameters', async () => { @@ -2172,7 +2172,9 @@ describe('ForestMCPServer Instance', () => { }); const { calls } = mockLogger.mock; - const incomingIndex = calls.findIndex((c: [string, string]) => c[1] === 'Incoming POST /mcp'); + const incomingIndex = calls.findIndex((c: [string, string]) => + c[1].startsWith('Incoming POST /mcp'), + ); const toolCallIndex = calls.findIndex((c: [string, string]) => c[1].includes('Tool call: list'), ); From 39c832ab9919dbe3ccecc5a1e9099dc19db2152c Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 10 Feb 2026 10:58:47 +0100 Subject: [PATCH 05/11] fix(mcp-server): clarify log format to avoid repeating 'method' Co-Authored-By: Claude Opus 4.6 --- packages/mcp-server/src/server.ts | 4 ++-- packages/mcp-server/test/server.test.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/mcp-server/src/server.ts b/packages/mcp-server/src/server.ts index 8f2be1ad7..e72e974bd 100644 --- a/packages/mcp-server/src/server.ts +++ b/packages/mcp-server/src/server.ts @@ -234,8 +234,8 @@ export default class ForestMCPServer { * Logs the request, intercepts the response for error logging, and delegates to the transport. */ private async handleMcpRequest(req: express.Request, res: express.Response): Promise { - const method = req.body?.method || 'unknown'; - this.logger('Info', `Incoming ${req.method} ${req.path} - method: ${method}`); + const rpcMethod = req.body?.method || 'unknown'; + this.logger('Info', `Incoming ${req.method} ${req.path} [${rpcMethod}]`); if (!this.mcpTransport) { throw new Error('MCP transport not initialized'); diff --git a/packages/mcp-server/test/server.test.ts b/packages/mcp-server/test/server.test.ts index d51ad5a99..cd9186d1b 100644 --- a/packages/mcp-server/test/server.test.ts +++ b/packages/mcp-server/test/server.test.ts @@ -2056,7 +2056,7 @@ describe('ForestMCPServer Instance', () => { .set('Accept', 'application/json, text/event-stream') .send({ jsonrpc: '2.0', method: 'tools/list', id: 1 }); - expect(mockLogger).toHaveBeenCalledWith('Info', 'Incoming POST /mcp - method: tools/list'); + expect(mockLogger).toHaveBeenCalledWith('Info', 'Incoming POST /mcp [tools/list]'); }); it('should log tool calls with safe parameters', async () => { From 2a4d1c9613f29ac395dc26d27b2c5e4a64447623 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 10 Feb 2026 11:05:57 +0100 Subject: [PATCH 06/11] fix(mcp-server): log when headersSent prevents error response, add missing tests - Log a warning when error response cannot be sent because headers were already sent - Add test for Warn log level on 4xx responses - Add test for MCP error handler logging method name and stack trace Co-Authored-By: Claude Opus 4.6 --- packages/mcp-server/src/server.ts | 51 ++++++++++++++++++------- packages/mcp-server/test/server.test.ts | 42 ++++++++++++++++++++ 2 files changed, 79 insertions(+), 14 deletions(-) diff --git a/packages/mcp-server/src/server.ts b/packages/mcp-server/src/server.ts index e72e974bd..98bc691b0 100644 --- a/packages/mcp-server/src/server.ts +++ b/packages/mcp-server/src/server.ts @@ -56,6 +56,21 @@ const defaultLogger: Logger = (level, message) => { getDefaultLogFn(level)(message); }; +function logLevelForStatus(statusCode: number): LogLevel { + if (statusCode >= 500) return 'Error'; + if (statusCode >= 400) return 'Warn'; + + return 'Info'; +} + +function logErrorWithStack(logger: Logger, message: string, err: Error): void { + logger('Error', message); + + if (err.stack) { + logger('Error', `Stack: ${err.stack}`); + } +} + /** Fields that are safe to log for each tool (non-sensitive data) */ const SAFE_ARGUMENTS_FOR_LOGGING: Record = { list: ['collectionName'], @@ -331,9 +346,7 @@ export default class ForestMCPServer { res.on('finish', () => { const duration = Date.now() - startTime; - let level: LogLevel = 'Info'; - if (res.statusCode >= 500) level = 'Error'; - else if (res.statusCode >= 400) level = 'Warn'; + const level = logLevelForStatus(res.statusCode); this.logger( level, `[${res.statusCode}] ${req.method} ${req.baseUrl || req.path} - ${duration}ms`, @@ -375,21 +388,26 @@ export default class ForestMCPServer { this.handleMcpRequest(req, res).catch(error => { const mcpMethod = req.body?.method || 'unknown'; const err = error as Error; - this.logger('Error', `MCP Error on method '${mcpMethod}': ${err.message || error}`); - - if (err.stack) { - this.logger('Error', `Stack: ${err.stack}`); - } + logErrorWithStack( + this.logger, + `MCP Error on method '${mcpMethod}': ${err.message || error}`, + err, + ); if (!res.headersSent) { res.status(500).json({ jsonrpc: '2.0', error: { code: -32603, - message: err?.message || 'Internal server error', + message: err.message || 'Internal server error', }, id: null, }); + } else { + this.logger( + 'Warn', + `Cannot send error response (headers already sent) for method '${mcpMethod}'`, + ); } }); }, @@ -402,17 +420,22 @@ export default class ForestMCPServer { app.use( // eslint-disable-next-line @typescript-eslint/no-unused-vars (err: Error, req: express.Request, res: express.Response, _next: express.NextFunction) => { - logger('Error', `Unhandled error on ${req.method} ${req.path}: ${err.message}`); - - if (err.stack) { - logger('Error', `Stack: ${err.stack}`); - } + logErrorWithStack( + logger, + `Unhandled error on ${req.method} ${req.path}: ${err.message}`, + err, + ); if (!res.headersSent) { res.status(500).json({ error: 'internal_server_error', error_description: err.message, }); + } else { + logger( + 'Warn', + `Cannot send error response (headers already sent) for ${req.method} ${req.path}`, + ); } }, ); diff --git a/packages/mcp-server/test/server.test.ts b/packages/mcp-server/test/server.test.ts index cd9186d1b..c8e69f5e2 100644 --- a/packages/mcp-server/test/server.test.ts +++ b/packages/mcp-server/test/server.test.ts @@ -2145,6 +2145,48 @@ describe('ForestMCPServer Instance', () => { ); }); + it('should log with Warn level for 4xx responses', async () => { + // Send unauthenticated request to trigger 401 + await request(loggingHttpServer) + .post('/mcp') + .set('Content-Type', 'application/json') + .send({ jsonrpc: '2.0', method: 'tools/list', id: 1 }); + + expect(mockLogger).toHaveBeenCalledWith( + 'Warn', + expect.stringMatching(/\[401\] POST \/mcp - \d+ms/), + ); + }); + + it('should log MCP error with method name and stack trace when handleMcpRequest fails', async () => { + const authSecret = process.env.FOREST_AUTH_SECRET || 'test-auth-secret'; + const validToken = jsonwebtoken.sign( + { id: 123, email: 'user@example.com', renderingId: 456 }, + authSecret, + { expiresIn: '1h' }, + ); + + // Break the transport to force an error in handleMcpRequest + const originalTransport = loggingServer.mcpTransport; + loggingServer.mcpTransport = undefined; + + await request(loggingHttpServer) + .post('/mcp') + .set('Authorization', `Bearer ${validToken}`) + .set('Content-Type', 'application/json') + .set('Accept', 'application/json, text/event-stream') + .send({ jsonrpc: '2.0', method: 'tools/list', id: 1 }); + + // Restore transport + loggingServer.mcpTransport = originalTransport; + + expect(mockLogger).toHaveBeenCalledWith( + 'Error', + expect.stringContaining("MCP Error on method 'tools/list'"), + ); + expect(mockLogger).toHaveBeenCalledWith('Error', expect.stringContaining('Stack:')); + }); + it('should log in correct order: incoming, tool call, response', async () => { const authSecret = process.env.FOREST_AUTH_SECRET || 'test-auth-secret'; const validToken = jsonwebtoken.sign( From 40dc34814be7419fb9c6a319544c9322d821030c Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 10 Feb 2026 13:10:56 +0100 Subject: [PATCH 07/11] fix(mcp-server): add coverage tests for headersSent and global error handler Co-Authored-By: Claude Opus 4.6 --- packages/mcp-server/test/server.test.ts | 52 +++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/packages/mcp-server/test/server.test.ts b/packages/mcp-server/test/server.test.ts index c8e69f5e2..8eb57b019 100644 --- a/packages/mcp-server/test/server.test.ts +++ b/packages/mcp-server/test/server.test.ts @@ -2187,6 +2187,58 @@ describe('ForestMCPServer Instance', () => { expect(mockLogger).toHaveBeenCalledWith('Error', expect.stringContaining('Stack:')); }); + it('should log warning when headersSent prevents MCP error response', async () => { + const authSecret = process.env.FOREST_AUTH_SECRET || 'test-auth-secret'; + const validToken = jsonwebtoken.sign( + { id: 123, email: 'user@example.com', renderingId: 456 }, + authSecret, + { expiresIn: '1h' }, + ); + + // Mock handleMcpRequest to send headers, end response, then throw + const serverRecord = loggingServer as unknown as Record; + const originalHandle = serverRecord.handleMcpRequest; + + serverRecord.handleMcpRequest = async ( + _req: unknown, + res: { writeHead: (s: number) => void; end: () => void }, + ) => { + res.writeHead(200); + res.end(); + throw new Error('Error after headers sent'); + }; + + await request(loggingHttpServer) + .post('/mcp') + .set('Authorization', `Bearer ${validToken}`) + .set('Content-Type', 'application/json') + .set('Accept', 'application/json, text/event-stream') + .send({ jsonrpc: '2.0', method: 'tools/list', id: 1 }); + + // Restore + serverRecord.handleMcpRequest = originalHandle; + + expect(mockLogger).toHaveBeenCalledWith( + 'Warn', + expect.stringContaining('Cannot send error response (headers already sent)'), + ); + }); + + it('should log unhandled Express error with stack trace', async () => { + // Send malformed JSON to trigger express.json() parse error, + // which calls next(err) and reaches the global error handler + await request(loggingHttpServer) + .post('/mcp') + .set('Content-Type', 'application/json') + .send('invalid json'); + + expect(mockLogger).toHaveBeenCalledWith( + 'Error', + expect.stringContaining('Unhandled error on POST /mcp'), + ); + expect(mockLogger).toHaveBeenCalledWith('Error', expect.stringContaining('Stack:')); + }); + it('should log in correct order: incoming, tool call, response', async () => { const authSecret = process.env.FOREST_AUTH_SECRET || 'test-auth-secret'; const validToken = jsonwebtoken.sign( From 4c8531a78f94e73fc6dc567e0ddb20d2c7e69c74 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 10 Feb 2026 13:24:12 +0100 Subject: [PATCH 08/11] fix(mcp-server): add coverage for global error handler headersSent branch Co-Authored-By: Claude Opus 4.6 --- packages/mcp-server/test/server.test.ts | 30 +++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/packages/mcp-server/test/server.test.ts b/packages/mcp-server/test/server.test.ts index 8eb57b019..360d3fde7 100644 --- a/packages/mcp-server/test/server.test.ts +++ b/packages/mcp-server/test/server.test.ts @@ -2239,6 +2239,36 @@ describe('ForestMCPServer Instance', () => { expect(mockLogger).toHaveBeenCalledWith('Error', expect.stringContaining('Stack:')); }); + it('should log warning when headersSent prevents global error handler response', () => { + // Extract the global error handler from Express's router stack + // It's the 4-arg function (err, req, res, next) registered via app.use() + // eslint-disable-next-line no-underscore-dangle + const routerStack = ( + (loggingServer as unknown as Record).expressApp as { + _router: { stack: { handle: { length: number } }[] }; + } + )._router.stack; + const errorHandlerLayer = routerStack.find(layer => layer.handle.length === 4); + expect(errorHandlerLayer).toBeDefined(); + + const errorHandler = errorHandlerLayer?.handle as unknown as ( + err: Error, + req: { method: string; path: string }, + res: { headersSent: boolean }, + next: () => void, + ) => void; + + const mockReq = { method: 'POST', path: '/mcp' }; + const mockRes = { headersSent: true }; + + errorHandler(new Error('test error'), mockReq, mockRes, () => {}); + + expect(mockLogger).toHaveBeenCalledWith( + 'Warn', + 'Cannot send error response (headers already sent) for POST /mcp', + ); + }); + it('should log in correct order: incoming, tool call, response', async () => { const authSecret = process.env.FOREST_AUTH_SECRET || 'test-auth-secret'; const validToken = jsonwebtoken.sign( From 4af5f0d90e7f8c1131deed9567f8315b852667c4 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 10 Feb 2026 14:26:22 +0100 Subject: [PATCH 09/11] fix(mcp-server): return real error messages instead of "Unexpected error" - Catch tool handler errors and return them as isError results instead of throwing, because the MCP SDK replaces thrown messages with generic text - Expose real error messages in dev mode (isProduction: false) in the agent error handler, keeping "Unexpected error" in production for security Co-Authored-By: Claude Opus 4.6 --- .../agent/src/routes/system/error-handling.ts | 6 + .../test/routes/system/error-handling.test.ts | 12 ++ .../mcp-server/src/utils/tool-with-logging.ts | 16 +- packages/mcp-server/test/tools/create.test.ts | 26 +++- packages/mcp-server/test/tools/delete.test.ts | 28 +++- .../test/tools/describe-collection.test.ts | 10 +- .../test/tools/execute-action.test.ts | 37 +++-- .../test/tools/get-action-form.test.ts | 42 ++--- .../test/tools/list-related.test.ts | 143 +++++++++++------- packages/mcp-server/test/tools/list.test.ts | 69 ++++++--- packages/mcp-server/test/tools/update.test.ts | 32 ++-- .../test/utils/tool-with-logging.test.ts | 10 +- 12 files changed, 279 insertions(+), 152 deletions(-) diff --git a/packages/agent/src/routes/system/error-handling.ts b/packages/agent/src/routes/system/error-handling.ts index 1b717563e..496473e6e 100644 --- a/packages/agent/src/routes/system/error-handling.ts +++ b/packages/agent/src/routes/system/error-handling.ts @@ -94,6 +94,12 @@ export default class ErrorHandling extends BaseRoute { return error.message; } + // In development mode, expose the real error message to help debugging. + // In production, we keep "Unexpected error" to avoid leaking internal details. + if (!this.options.isProduction && error.message) { + return error.message; + } + return 'Unexpected error'; } diff --git a/packages/agent/test/routes/system/error-handling.test.ts b/packages/agent/test/routes/system/error-handling.test.ts index 41f4f5942..d7ff16e61 100644 --- a/packages/agent/test/routes/system/error-handling.test.ts +++ b/packages/agent/test/routes/system/error-handling.test.ts @@ -240,6 +240,18 @@ describe('ErrorHandling', () => { expect(console.error).toHaveBeenCalled(); }); + + test('it should expose real error message instead of "Unexpected error"', async () => { + const context = createMockContext(); + const next = jest.fn().mockRejectedValue(new Error('Cast to ObjectId failed')); + + await expect(handleError.call(route, context, next)).rejects.toThrow(); + + expect(context.response.status).toStrictEqual(HttpCode.InternalServerError); + expect(context.response.body).toStrictEqual({ + errors: [{ detail: 'Cast to ObjectId failed', name: 'Error', status: 500 }], + }); + }); }); describe('guaranty cross packages instanceof errors workaround', () => { diff --git a/packages/mcp-server/src/utils/tool-with-logging.ts b/packages/mcp-server/src/utils/tool-with-logging.ts index c0f7d3ce5..9ea0c517f 100644 --- a/packages/mcp-server/src/utils/tool-with-logging.ts +++ b/packages/mcp-server/src/utils/tool-with-logging.ts @@ -99,7 +99,21 @@ export default function registerToolWithLogging< (async (args: any, extra: any) => { logValidationErrorsIfAny(args, schema, toolName, logger); - return handler(args as TArgs, extra); + // Return errors as tool results (isError: true) instead of throwing. + // Per MCP spec, tool errors should be reported within the result object, + // not as protocol-level errors, so the LLM can see and handle them. + // The SDK also replaces thrown error messages with generic "Unexpected error". + // See: https://modelcontextprotocol.io/docs/concepts/tools + try { + return await handler(args as TArgs, extra); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + + return { + content: [{ type: 'text', text: message }], + isError: true, + }; + } // eslint-disable-next-line @typescript-eslint/no-explicit-any }) as any, ); diff --git a/packages/mcp-server/test/tools/create.test.ts b/packages/mcp-server/test/tools/create.test.ts index 86d12576f..9be54102e 100644 --- a/packages/mcp-server/test/tools/create.test.ts +++ b/packages/mcp-server/test/tools/create.test.ts @@ -247,7 +247,7 @@ describe('declareCreateTool', () => { }); describe('error handling', () => { - it('should parse error with nested error.text structure in message', async () => { + it('should return error as tool result with nested error.text structure in message', async () => { const errorPayload = { error: { status: 400, @@ -264,12 +264,17 @@ describe('declareCreateTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - await expect( - registeredToolHandler({ collectionName: 'users', attributes: {} }, mockExtra), - ).rejects.toThrow('Name is required'); + const result = await registeredToolHandler( + { collectionName: 'users', attributes: {} }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Name is required') }], + isError: true, + }); }); - it('should rethrow original error when no parsable error found', async () => { + it('should return error as tool result when no parsable error found', async () => { const agentError = { unknownProperty: 'some value' }; const mockCreate = jest.fn().mockRejectedValue(agentError); const mockCollection = jest.fn().mockReturnValue({ create: mockCreate }); @@ -278,9 +283,14 @@ describe('declareCreateTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - await expect( - registeredToolHandler({ collectionName: 'users', attributes: {} }, mockExtra), - ).rejects.toEqual(agentError); + const result = await registeredToolHandler( + { collectionName: 'users', attributes: {} }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.any(String) }], + isError: true, + }); }); }); }); diff --git a/packages/mcp-server/test/tools/delete.test.ts b/packages/mcp-server/test/tools/delete.test.ts index 84ba6d5bf..3c98b39ae 100644 --- a/packages/mcp-server/test/tools/delete.test.ts +++ b/packages/mcp-server/test/tools/delete.test.ts @@ -235,7 +235,7 @@ describe('declareDeleteTool', () => { }); describe('error handling', () => { - it('should parse error with nested error.text structure in message', async () => { + it('should return error as tool result with nested error.text structure in message', async () => { const errorPayload = { error: { status: 403, @@ -252,12 +252,19 @@ describe('declareDeleteTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - await expect( - registeredToolHandler({ collectionName: 'users', recordIds: [1] }, mockExtra), - ).rejects.toThrow('Cannot delete protected records'); + const result = await registeredToolHandler( + { collectionName: 'users', recordIds: [1] }, + mockExtra, + ); + expect(result).toEqual({ + content: [ + { type: 'text', text: expect.stringContaining('Cannot delete protected records') }, + ], + isError: true, + }); }); - it('should rethrow original error when no parsable error found', async () => { + it('should return error as tool result when no parsable error found', async () => { const agentError = { unknownProperty: 'some value' }; const mockDelete = jest.fn().mockRejectedValue(agentError); const mockCollection = jest.fn().mockReturnValue({ delete: mockDelete }); @@ -266,9 +273,14 @@ describe('declareDeleteTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - await expect( - registeredToolHandler({ collectionName: 'users', recordIds: [1] }, mockExtra), - ).rejects.toEqual(agentError); + const result = await registeredToolHandler( + { collectionName: 'users', recordIds: [1] }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.any(String) }], + isError: true, + }); }); }); }); diff --git a/packages/mcp-server/test/tools/describe-collection.test.ts b/packages/mcp-server/test/tools/describe-collection.test.ts index de2c8ce63..2f3c60558 100644 --- a/packages/mcp-server/test/tools/describe-collection.test.ts +++ b/packages/mcp-server/test/tools/describe-collection.test.ts @@ -342,7 +342,7 @@ describe('declareDescribeCollectionTool', () => { ); }); - it('should throw and log Error for non-404 capabilities errors', async () => { + it('should return error as tool result and log Error for non-404 capabilities errors', async () => { const mockCapabilities = jest.fn().mockRejectedValue(new Error('Server error')); const mockCollection = jest.fn().mockReturnValue({ capabilities: mockCapabilities }); mockBuildClient.mockReturnValue({ @@ -353,9 +353,11 @@ describe('declareDescribeCollectionTool', () => { mockFetchForestSchema.mockResolvedValue({ collections: [] }); mockGetFieldsOfCollection.mockReturnValue([]); - await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( - 'Server error', - ); + const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Server error') }], + isError: true, + }); expect(mockLogger).toHaveBeenCalledWith( 'Error', diff --git a/packages/mcp-server/test/tools/execute-action.test.ts b/packages/mcp-server/test/tools/execute-action.test.ts index 7db07fc13..0e17553db 100644 --- a/packages/mcp-server/test/tools/execute-action.test.ts +++ b/packages/mcp-server/test/tools/execute-action.test.ts @@ -398,7 +398,7 @@ describe('declareExecuteActionTool', () => { }); describe('error handling', () => { - it('should parse error with nested error.text structure in message', async () => { + it('should return error as tool result with nested error.text structure in message', async () => { const errorPayload = { error: { status: 403, @@ -417,15 +417,22 @@ describe('declareExecuteActionTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - await expect( - registeredToolHandler( - { collectionName: 'users', actionName: 'sendEmail', recordIds: [1] }, - mockExtra, - ), - ).rejects.toThrow('Cannot execute action on these records'); + const result = await registeredToolHandler( + { collectionName: 'users', actionName: 'sendEmail', recordIds: [1] }, + mockExtra, + ); + expect(result).toEqual({ + content: [ + { + type: 'text', + text: expect.stringContaining('Cannot execute action on these records'), + }, + ], + isError: true, + }); }); - it('should rethrow original error when no parsable error found', async () => { + it('should return error as tool result when no parsable error found', async () => { const agentError = { unknownProperty: 'some value' }; const mockAction = jest.fn().mockRejectedValue(agentError); const mockCollection = jest.fn().mockReturnValue({ action: mockAction }); @@ -434,12 +441,14 @@ describe('declareExecuteActionTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - await expect( - registeredToolHandler( - { collectionName: 'users', actionName: 'sendEmail', recordIds: [1] }, - mockExtra, - ), - ).rejects.toEqual(agentError); + const result = await registeredToolHandler( + { collectionName: 'users', actionName: 'sendEmail', recordIds: [1] }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.any(String) }], + isError: true, + }); }); }); }); diff --git a/packages/mcp-server/test/tools/get-action-form.test.ts b/packages/mcp-server/test/tools/get-action-form.test.ts index 95e8f0ae9..971287cf5 100644 --- a/packages/mcp-server/test/tools/get-action-form.test.ts +++ b/packages/mcp-server/test/tools/get-action-form.test.ts @@ -720,7 +720,7 @@ describe('declareGetActionFormTool', () => { }); describe('error handling', () => { - it('should propagate errors from the action call', async () => { + it('should return error as tool result from the action call', async () => { const agentError = new Error('Action not found'); const mockAction = jest.fn().mockRejectedValue(agentError); const mockCollection = jest.fn().mockReturnValue({ action: mockAction }); @@ -729,15 +729,17 @@ describe('declareGetActionFormTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - await expect( - registeredToolHandler( - { collectionName: 'users', actionName: 'nonExistent', recordIds: [1] }, - mockExtra, - ), - ).rejects.toThrow('Action not found'); + const result = await registeredToolHandler( + { collectionName: 'users', actionName: 'nonExistent', recordIds: [1] }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Action not found') }], + isError: true, + }); }); - it('should propagate errors from tryToSetFields call', async () => { + it('should return error as tool result from tryToSetFields call', async () => { const tryToSetFieldsError = new Error('Invalid field value'); const mockGetFields = jest.fn().mockReturnValue([]); const mockTryToSetFields = jest.fn().mockRejectedValue(tryToSetFieldsError); @@ -751,17 +753,19 @@ describe('declareGetActionFormTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - await expect( - registeredToolHandler( - { - collectionName: 'users', - actionName: 'sendEmail', - recordIds: [1], - values: { invalidField: 'value' }, - }, - mockExtra, - ), - ).rejects.toThrow('Invalid field value'); + const result = await registeredToolHandler( + { + collectionName: 'users', + actionName: 'sendEmail', + recordIds: [1], + values: { invalidField: 'value' }, + }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Invalid field value') }], + isError: true, + }); }); }); }); diff --git a/packages/mcp-server/test/tools/list-related.test.ts b/packages/mcp-server/test/tools/list-related.test.ts index 026abbba6..72a1c4261 100644 --- a/packages/mcp-server/test/tools/list-related.test.ts +++ b/packages/mcp-server/test/tools/list-related.test.ts @@ -595,7 +595,7 @@ describe('declareListRelatedTool', () => { } as unknown as ReturnType); }); - it('should parse error with nested error.text structure in message', async () => { + it('should return error as tool result with nested error.text structure in message', async () => { const errorPayload = { error: { status: 400, @@ -610,15 +610,17 @@ describe('declareListRelatedTool', () => { mockFetchForestSchema.mockResolvedValue({ collections: [] }); mockGetFieldsOfCollection.mockReturnValue([]); - await expect( - registeredToolHandler( - { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, - mockExtra, - ), - ).rejects.toThrow('Invalid filters provided'); + const result = await registeredToolHandler( + { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Invalid filters provided') }], + isError: true, + }); }); - it('should parse error with direct text property in message', async () => { + it('should return error as tool result with direct text property in message', async () => { const errorPayload = { text: JSON.stringify({ errors: [{ name: 'ValidationError', detail: 'Direct text error' }], @@ -630,15 +632,17 @@ describe('declareListRelatedTool', () => { mockFetchForestSchema.mockResolvedValue({ collections: [] }); mockGetFieldsOfCollection.mockReturnValue([]); - await expect( - registeredToolHandler( - { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, - mockExtra, - ), - ).rejects.toThrow('Direct text error'); + const result = await registeredToolHandler( + { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Direct text error') }], + isError: true, + }); }); - it('should provide helpful error message for Invalid sort errors', async () => { + it('should return helpful error message for Invalid sort errors', async () => { const errorPayload = { error: { status: 400, @@ -688,20 +692,27 @@ describe('declareListRelatedTool', () => { mockFetchForestSchema.mockResolvedValue(mockSchema); mockGetFieldsOfCollection.mockReturnValue(mockFields); - await expect( - registeredToolHandler( - { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, - mockExtra, - ), - ).rejects.toThrow( - 'The sort field provided is invalid for this collection. Available fields for the collection users are: id, total.', + const result = await registeredToolHandler( + { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, + mockExtra, ); + expect(result).toEqual({ + content: [ + { + type: 'text', + text: expect.stringContaining( + 'The sort field provided is invalid for this collection. Available fields for the collection users are: id, total.', + ), + }, + ], + isError: true, + }); expect(mockFetchForestSchema).toHaveBeenCalledWith(mockForestServerClient); expect(mockGetFieldsOfCollection).toHaveBeenCalledWith(mockSchema, 'users'); }); - it('should provide helpful error message when relation is not found', async () => { + it('should return helpful error message when relation is not found', async () => { const agentError = new Error('Relation not found'); mockList.mockRejectedValue(agentError); @@ -746,14 +757,21 @@ describe('declareListRelatedTool', () => { mockFetchForestSchema.mockResolvedValue(mockSchema); mockGetFieldsOfCollection.mockReturnValue(mockFields); - await expect( - registeredToolHandler( - { collectionName: 'users', relationName: 'invalidRelation', parentRecordId: 1 }, - mockExtra, - ), - ).rejects.toThrow( - 'The relation name provided is invalid for this collection. Available relations for collection users are: orders, reviews.', + const result = await registeredToolHandler( + { collectionName: 'users', relationName: 'invalidRelation', parentRecordId: 1 }, + mockExtra, ); + expect(result).toEqual({ + content: [ + { + type: 'text', + text: expect.stringContaining( + 'The relation name provided is invalid for this collection. Available relations for collection users are: orders, reviews.', + ), + }, + ], + isError: true, + }); }); it('should include BelongsToMany relations in available relations error message', async () => { @@ -790,14 +808,21 @@ describe('declareListRelatedTool', () => { mockFetchForestSchema.mockResolvedValue(mockSchema); mockGetFieldsOfCollection.mockReturnValue(mockFields); - await expect( - registeredToolHandler( - { collectionName: 'users', relationName: 'invalidRelation', parentRecordId: 1 }, - mockExtra, - ), - ).rejects.toThrow( - 'The relation name provided is invalid for this collection. Available relations for collection users are: orders, tags.', + const result = await registeredToolHandler( + { collectionName: 'users', relationName: 'invalidRelation', parentRecordId: 1 }, + mockExtra, ); + expect(result).toEqual({ + content: [ + { + type: 'text', + text: expect.stringContaining( + 'The relation name provided is invalid for this collection. Available relations for collection users are: orders, tags.', + ), + }, + ], + isError: true, + }); }); it('should not show relation error when relation exists but error is different', async () => { @@ -823,13 +848,15 @@ describe('declareListRelatedTool', () => { mockFetchForestSchema.mockResolvedValue(mockSchema); mockGetFieldsOfCollection.mockReturnValue(mockFields); - // Should throw the original error message since 'orders' relation exists - await expect( - registeredToolHandler( - { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, - mockExtra, - ), - ).rejects.toThrow('Some other error'); + // Should return the original error message since 'orders' relation exists + const result = await registeredToolHandler( + { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Some other error') }], + isError: true, + }); }); it('should fall back to error.message when message is not valid JSON', async () => { @@ -854,12 +881,14 @@ describe('declareListRelatedTool', () => { }); mockGetFieldsOfCollection.mockReturnValue(mockFields); - await expect( - registeredToolHandler( - { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, - mockExtra, - ), - ).rejects.toThrow('Plain error message'); + const result = await registeredToolHandler( + { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Plain error message') }], + isError: true, + }); }); it('should handle string errors thrown directly', async () => { @@ -884,12 +913,14 @@ describe('declareListRelatedTool', () => { }); mockGetFieldsOfCollection.mockReturnValue(mockFields); - await expect( - registeredToolHandler( - { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, - mockExtra, - ), - ).rejects.toThrow('Connection failed'); + const result = await registeredToolHandler( + { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Connection failed') }], + isError: true, + }); }); }); }); diff --git a/packages/mcp-server/test/tools/list.test.ts b/packages/mcp-server/test/tools/list.test.ts index efd0a7f3d..8ef295f7b 100644 --- a/packages/mcp-server/test/tools/list.test.ts +++ b/packages/mcp-server/test/tools/list.test.ts @@ -894,7 +894,7 @@ describe('declareListTool', () => { } as unknown as ReturnType); }); - it('should parse error with nested error.text structure in message', async () => { + it('should return error as tool result with nested error.text structure in message', async () => { // The RPC client throws an Error with message containing JSON: { error: { text: '...' } } const errorPayload = { error: { @@ -907,12 +907,14 @@ describe('declareListTool', () => { const agentError = new Error(JSON.stringify(errorPayload)); mockList.mockRejectedValue(agentError); - await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( - 'Invalid filters provided', - ); + const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Invalid filters provided') }], + isError: true, + }); }); - it('should parse error with direct text property in message', async () => { + it('should return error as tool result with direct text property in message', async () => { // The RPC client throws an Error with message containing JSON: { text: '...' } const errorPayload = { text: JSON.stringify({ @@ -922,12 +924,14 @@ describe('declareListTool', () => { const agentError = new Error(JSON.stringify(errorPayload)); mockList.mockRejectedValue(agentError); - await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( - 'Direct text error', - ); + const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Direct text error') }], + isError: true, + }); }); - it('should use message property from parsed JSON when no text field', async () => { + it('should return error as tool result with message property from parsed JSON when no text field', async () => { // The RPC client throws an Error with message containing JSON: { message: '...' } const errorPayload = { message: 'Error message from JSON payload', @@ -935,31 +939,39 @@ describe('declareListTool', () => { const agentError = new Error(JSON.stringify(errorPayload)); mockList.mockRejectedValue(agentError); - await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( - 'Error message from JSON payload', - ); + const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); + expect(result).toEqual({ + content: [ + { type: 'text', text: expect.stringContaining('Error message from JSON payload') }, + ], + isError: true, + }); }); - it('should fall back to error.message when message is not valid JSON', async () => { + it('should return error as tool result when message is not valid JSON', async () => { // The RPC client throws an Error with a plain string message (not JSON) const agentError = new Error('Plain error message'); mockList.mockRejectedValue(agentError); - await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( - 'Plain error message', - ); + const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Plain error message') }], + isError: true, + }); }); - it('should handle string errors thrown directly', async () => { + it('should return error as tool result for string errors thrown directly', async () => { // Some libraries throw string errors directly mockList.mockRejectedValue('Connection failed'); - await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( - 'Connection failed', - ); + const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Connection failed') }], + isError: true, + }); }); - it('should provide helpful error message for Invalid sort errors', async () => { + it('should return helpful error message for Invalid sort errors', async () => { // The RPC client throws an "Invalid sort" error const errorPayload = { error: { @@ -1021,9 +1033,18 @@ describe('declareListTool', () => { mockFetchForestSchema.mockResolvedValue(mockSchema); mockGetFieldsOfCollection.mockReturnValue(mockFields); - await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( - 'The sort field provided is invalid for this collection. Available fields for the collection users are: id, name, email.', - ); + const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); + expect(result).toEqual({ + content: [ + { + type: 'text', + text: expect.stringContaining( + 'The sort field provided is invalid for this collection. Available fields for the collection users are: id, name, email.', + ), + }, + ], + isError: true, + }); expect(mockFetchForestSchema).toHaveBeenCalledWith(mockForestServerClient); expect(mockGetFieldsOfCollection).toHaveBeenCalledWith(mockSchema, 'users'); diff --git a/packages/mcp-server/test/tools/update.test.ts b/packages/mcp-server/test/tools/update.test.ts index 4e71aae5e..e18d6444b 100644 --- a/packages/mcp-server/test/tools/update.test.ts +++ b/packages/mcp-server/test/tools/update.test.ts @@ -262,7 +262,7 @@ describe('declareUpdateTool', () => { }); describe('error handling', () => { - it('should parse error with nested error.text structure in message', async () => { + it('should return error as tool result with nested error.text structure in message', async () => { const errorPayload = { error: { status: 404, @@ -279,15 +279,17 @@ describe('declareUpdateTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - await expect( - registeredToolHandler( - { collectionName: 'users', recordId: 999, attributes: {} }, - mockExtra, - ), - ).rejects.toThrow('Record not found'); + const result = await registeredToolHandler( + { collectionName: 'users', recordId: 999, attributes: {} }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.stringContaining('Record not found') }], + isError: true, + }); }); - it('should rethrow original error when no parsable error found', async () => { + it('should return error as tool result when no parsable error found', async () => { const agentError = { unknownProperty: 'some value' }; const mockUpdate = jest.fn().mockRejectedValue(agentError); const mockCollection = jest.fn().mockReturnValue({ update: mockUpdate }); @@ -296,12 +298,14 @@ describe('declareUpdateTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - await expect( - registeredToolHandler( - { collectionName: 'users', recordId: 1, attributes: {} }, - mockExtra, - ), - ).rejects.toEqual(agentError); + const result = await registeredToolHandler( + { collectionName: 'users', recordId: 1, attributes: {} }, + mockExtra, + ); + expect(result).toEqual({ + content: [{ type: 'text', text: expect.any(String) }], + isError: true, + }); }); }); }); diff --git a/packages/mcp-server/test/utils/tool-with-logging.test.ts b/packages/mcp-server/test/utils/tool-with-logging.test.ts index bbf140ec5..a804ccad2 100644 --- a/packages/mcp-server/test/utils/tool-with-logging.test.ts +++ b/packages/mcp-server/test/utils/tool-with-logging.test.ts @@ -121,15 +121,17 @@ describe('registerToolWithLogging', () => { expect(result).toBe(expectedResult); }); - it('should propagate handler errors without logging', async () => { + it('should return error as tool result instead of throwing', async () => { const error = new Error('Handler failed'); const handler = jest.fn().mockRejectedValue(error); registerToolWithLogging(mockMcpServer as never, 'test-tool', toolConfig, handler, mockLogger); - await expect(registeredHandler({ name: 'test', count: 42 }, {})).rejects.toThrow( - 'Handler failed', - ); + const result = await registeredHandler({ name: 'test', count: 42 }, {}); + expect(result).toEqual({ + content: [{ type: 'text', text: 'Handler failed' }], + isError: true, + }); expect(mockLogger).not.toHaveBeenCalled(); }); From ffc0dd1e1f7a271e8775fedf9cbd620a42f91b21 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 10 Feb 2026 14:41:37 +0100 Subject: [PATCH 10/11] fix(mcp-server): fix inaccurate comments and harden error handling - Remove incorrect claim that MCP SDK replaces thrown error messages - Normalize non-Error values in catch handler to avoid undefined message - Fix misleading comment about arrow function context - Stop leaking error details in global error handler response Co-Authored-By: Claude Opus 4.6 --- packages/mcp-server/src/server.ts | 6 +++--- packages/mcp-server/src/utils/tool-with-logging.ts | 5 ++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/mcp-server/src/server.ts b/packages/mcp-server/src/server.ts index 98bc691b0..8c9c23038 100644 --- a/packages/mcp-server/src/server.ts +++ b/packages/mcp-server/src/server.ts @@ -387,7 +387,7 @@ export default class ForestMCPServer { (req, res) => { this.handleMcpRequest(req, res).catch(error => { const mcpMethod = req.body?.method || 'unknown'; - const err = error as Error; + const err = error instanceof Error ? error : new Error(String(error)); logErrorWithStack( this.logger, `MCP Error on method '${mcpMethod}': ${err.message || error}`, @@ -415,7 +415,7 @@ export default class ForestMCPServer { // Global error handler to catch any unhandled errors // Express requires all 4 parameters to recognize this as an error handler - // Capture logger for use in error handler (arrow function would lose context) + // Capture logger reference so the error handler closure does not depend on 'this' const { logger } = this; app.use( // eslint-disable-next-line @typescript-eslint/no-unused-vars @@ -429,7 +429,7 @@ export default class ForestMCPServer { if (!res.headersSent) { res.status(500).json({ error: 'internal_server_error', - error_description: err.message, + error_description: 'Internal server error', }); } else { logger( diff --git a/packages/mcp-server/src/utils/tool-with-logging.ts b/packages/mcp-server/src/utils/tool-with-logging.ts index 9ea0c517f..b2a177066 100644 --- a/packages/mcp-server/src/utils/tool-with-logging.ts +++ b/packages/mcp-server/src/utils/tool-with-logging.ts @@ -60,8 +60,8 @@ function logValidationErrorsIfAny( * This wrapper logs validation errors with detailed field information, * which helps debug tool calls when clients send invalid arguments. * - * Note: Execution errors are logged by the SSE response interceptor in server.ts, - * so we don't duplicate that logging here. + * Note: Execution errors are caught and converted to { isError: true } tool results. + * The SSE response interceptor in server.ts additionally logs these errors from the stream. * * @example * registerToolWithLogging( @@ -102,7 +102,6 @@ export default function registerToolWithLogging< // Return errors as tool results (isError: true) instead of throwing. // Per MCP spec, tool errors should be reported within the result object, // not as protocol-level errors, so the LLM can see and handle them. - // The SDK also replaces thrown error messages with generic "Unexpected error". // See: https://modelcontextprotocol.io/docs/concepts/tools try { return await handler(args as TArgs, extra); From a51c795355ee487c1bf7c0807a4e4874a40edd9d Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 10 Feb 2026 15:12:56 +0100 Subject: [PATCH 11/11] fix(mcp-server): scope PR to diagnostic logging only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revert out-of-scope changes (isError tool handling, agent dev mode error messages) — these will be in a separate PR. Remove global error handler headersSent else branch per review feedback. Co-Authored-By: Claude Opus 4.6 --- .../agent/src/routes/system/error-handling.ts | 6 - .../test/routes/system/error-handling.test.ts | 12 -- packages/mcp-server/src/server.ts | 7 +- .../mcp-server/src/utils/tool-with-logging.ts | 19 +-- packages/mcp-server/test/server.test.ts | 30 ---- packages/mcp-server/test/tools/create.test.ts | 26 +--- packages/mcp-server/test/tools/delete.test.ts | 28 +--- .../test/tools/describe-collection.test.ts | 10 +- .../test/tools/execute-action.test.ts | 37 ++--- .../test/tools/get-action-form.test.ts | 42 +++-- .../test/tools/list-related.test.ts | 143 +++++++----------- packages/mcp-server/test/tools/list.test.ts | 69 +++------ packages/mcp-server/test/tools/update.test.ts | 32 ++-- .../test/utils/tool-with-logging.test.ts | 10 +- 14 files changed, 155 insertions(+), 316 deletions(-) diff --git a/packages/agent/src/routes/system/error-handling.ts b/packages/agent/src/routes/system/error-handling.ts index 496473e6e..1b717563e 100644 --- a/packages/agent/src/routes/system/error-handling.ts +++ b/packages/agent/src/routes/system/error-handling.ts @@ -94,12 +94,6 @@ export default class ErrorHandling extends BaseRoute { return error.message; } - // In development mode, expose the real error message to help debugging. - // In production, we keep "Unexpected error" to avoid leaking internal details. - if (!this.options.isProduction && error.message) { - return error.message; - } - return 'Unexpected error'; } diff --git a/packages/agent/test/routes/system/error-handling.test.ts b/packages/agent/test/routes/system/error-handling.test.ts index d7ff16e61..41f4f5942 100644 --- a/packages/agent/test/routes/system/error-handling.test.ts +++ b/packages/agent/test/routes/system/error-handling.test.ts @@ -240,18 +240,6 @@ describe('ErrorHandling', () => { expect(console.error).toHaveBeenCalled(); }); - - test('it should expose real error message instead of "Unexpected error"', async () => { - const context = createMockContext(); - const next = jest.fn().mockRejectedValue(new Error('Cast to ObjectId failed')); - - await expect(handleError.call(route, context, next)).rejects.toThrow(); - - expect(context.response.status).toStrictEqual(HttpCode.InternalServerError); - expect(context.response.body).toStrictEqual({ - errors: [{ detail: 'Cast to ObjectId failed', name: 'Error', status: 500 }], - }); - }); }); describe('guaranty cross packages instanceof errors workaround', () => { diff --git a/packages/mcp-server/src/server.ts b/packages/mcp-server/src/server.ts index 8c9c23038..d8ba6d32e 100644 --- a/packages/mcp-server/src/server.ts +++ b/packages/mcp-server/src/server.ts @@ -429,13 +429,8 @@ export default class ForestMCPServer { if (!res.headersSent) { res.status(500).json({ error: 'internal_server_error', - error_description: 'Internal server error', + error_description: err.message, }); - } else { - logger( - 'Warn', - `Cannot send error response (headers already sent) for ${req.method} ${req.path}`, - ); } }, ); diff --git a/packages/mcp-server/src/utils/tool-with-logging.ts b/packages/mcp-server/src/utils/tool-with-logging.ts index b2a177066..c0f7d3ce5 100644 --- a/packages/mcp-server/src/utils/tool-with-logging.ts +++ b/packages/mcp-server/src/utils/tool-with-logging.ts @@ -60,8 +60,8 @@ function logValidationErrorsIfAny( * This wrapper logs validation errors with detailed field information, * which helps debug tool calls when clients send invalid arguments. * - * Note: Execution errors are caught and converted to { isError: true } tool results. - * The SSE response interceptor in server.ts additionally logs these errors from the stream. + * Note: Execution errors are logged by the SSE response interceptor in server.ts, + * so we don't duplicate that logging here. * * @example * registerToolWithLogging( @@ -99,20 +99,7 @@ export default function registerToolWithLogging< (async (args: any, extra: any) => { logValidationErrorsIfAny(args, schema, toolName, logger); - // Return errors as tool results (isError: true) instead of throwing. - // Per MCP spec, tool errors should be reported within the result object, - // not as protocol-level errors, so the LLM can see and handle them. - // See: https://modelcontextprotocol.io/docs/concepts/tools - try { - return await handler(args as TArgs, extra); - } catch (error) { - const message = error instanceof Error ? error.message : String(error); - - return { - content: [{ type: 'text', text: message }], - isError: true, - }; - } + return handler(args as TArgs, extra); // eslint-disable-next-line @typescript-eslint/no-explicit-any }) as any, ); diff --git a/packages/mcp-server/test/server.test.ts b/packages/mcp-server/test/server.test.ts index 360d3fde7..8eb57b019 100644 --- a/packages/mcp-server/test/server.test.ts +++ b/packages/mcp-server/test/server.test.ts @@ -2239,36 +2239,6 @@ describe('ForestMCPServer Instance', () => { expect(mockLogger).toHaveBeenCalledWith('Error', expect.stringContaining('Stack:')); }); - it('should log warning when headersSent prevents global error handler response', () => { - // Extract the global error handler from Express's router stack - // It's the 4-arg function (err, req, res, next) registered via app.use() - // eslint-disable-next-line no-underscore-dangle - const routerStack = ( - (loggingServer as unknown as Record).expressApp as { - _router: { stack: { handle: { length: number } }[] }; - } - )._router.stack; - const errorHandlerLayer = routerStack.find(layer => layer.handle.length === 4); - expect(errorHandlerLayer).toBeDefined(); - - const errorHandler = errorHandlerLayer?.handle as unknown as ( - err: Error, - req: { method: string; path: string }, - res: { headersSent: boolean }, - next: () => void, - ) => void; - - const mockReq = { method: 'POST', path: '/mcp' }; - const mockRes = { headersSent: true }; - - errorHandler(new Error('test error'), mockReq, mockRes, () => {}); - - expect(mockLogger).toHaveBeenCalledWith( - 'Warn', - 'Cannot send error response (headers already sent) for POST /mcp', - ); - }); - it('should log in correct order: incoming, tool call, response', async () => { const authSecret = process.env.FOREST_AUTH_SECRET || 'test-auth-secret'; const validToken = jsonwebtoken.sign( diff --git a/packages/mcp-server/test/tools/create.test.ts b/packages/mcp-server/test/tools/create.test.ts index 9be54102e..86d12576f 100644 --- a/packages/mcp-server/test/tools/create.test.ts +++ b/packages/mcp-server/test/tools/create.test.ts @@ -247,7 +247,7 @@ describe('declareCreateTool', () => { }); describe('error handling', () => { - it('should return error as tool result with nested error.text structure in message', async () => { + it('should parse error with nested error.text structure in message', async () => { const errorPayload = { error: { status: 400, @@ -264,17 +264,12 @@ describe('declareCreateTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - const result = await registeredToolHandler( - { collectionName: 'users', attributes: {} }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Name is required') }], - isError: true, - }); + await expect( + registeredToolHandler({ collectionName: 'users', attributes: {} }, mockExtra), + ).rejects.toThrow('Name is required'); }); - it('should return error as tool result when no parsable error found', async () => { + it('should rethrow original error when no parsable error found', async () => { const agentError = { unknownProperty: 'some value' }; const mockCreate = jest.fn().mockRejectedValue(agentError); const mockCollection = jest.fn().mockReturnValue({ create: mockCreate }); @@ -283,14 +278,9 @@ describe('declareCreateTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - const result = await registeredToolHandler( - { collectionName: 'users', attributes: {} }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.any(String) }], - isError: true, - }); + await expect( + registeredToolHandler({ collectionName: 'users', attributes: {} }, mockExtra), + ).rejects.toEqual(agentError); }); }); }); diff --git a/packages/mcp-server/test/tools/delete.test.ts b/packages/mcp-server/test/tools/delete.test.ts index 3c98b39ae..84ba6d5bf 100644 --- a/packages/mcp-server/test/tools/delete.test.ts +++ b/packages/mcp-server/test/tools/delete.test.ts @@ -235,7 +235,7 @@ describe('declareDeleteTool', () => { }); describe('error handling', () => { - it('should return error as tool result with nested error.text structure in message', async () => { + it('should parse error with nested error.text structure in message', async () => { const errorPayload = { error: { status: 403, @@ -252,19 +252,12 @@ describe('declareDeleteTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - const result = await registeredToolHandler( - { collectionName: 'users', recordIds: [1] }, - mockExtra, - ); - expect(result).toEqual({ - content: [ - { type: 'text', text: expect.stringContaining('Cannot delete protected records') }, - ], - isError: true, - }); + await expect( + registeredToolHandler({ collectionName: 'users', recordIds: [1] }, mockExtra), + ).rejects.toThrow('Cannot delete protected records'); }); - it('should return error as tool result when no parsable error found', async () => { + it('should rethrow original error when no parsable error found', async () => { const agentError = { unknownProperty: 'some value' }; const mockDelete = jest.fn().mockRejectedValue(agentError); const mockCollection = jest.fn().mockReturnValue({ delete: mockDelete }); @@ -273,14 +266,9 @@ describe('declareDeleteTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - const result = await registeredToolHandler( - { collectionName: 'users', recordIds: [1] }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.any(String) }], - isError: true, - }); + await expect( + registeredToolHandler({ collectionName: 'users', recordIds: [1] }, mockExtra), + ).rejects.toEqual(agentError); }); }); }); diff --git a/packages/mcp-server/test/tools/describe-collection.test.ts b/packages/mcp-server/test/tools/describe-collection.test.ts index 2f3c60558..de2c8ce63 100644 --- a/packages/mcp-server/test/tools/describe-collection.test.ts +++ b/packages/mcp-server/test/tools/describe-collection.test.ts @@ -342,7 +342,7 @@ describe('declareDescribeCollectionTool', () => { ); }); - it('should return error as tool result and log Error for non-404 capabilities errors', async () => { + it('should throw and log Error for non-404 capabilities errors', async () => { const mockCapabilities = jest.fn().mockRejectedValue(new Error('Server error')); const mockCollection = jest.fn().mockReturnValue({ capabilities: mockCapabilities }); mockBuildClient.mockReturnValue({ @@ -353,11 +353,9 @@ describe('declareDescribeCollectionTool', () => { mockFetchForestSchema.mockResolvedValue({ collections: [] }); mockGetFieldsOfCollection.mockReturnValue([]); - const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Server error') }], - isError: true, - }); + await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( + 'Server error', + ); expect(mockLogger).toHaveBeenCalledWith( 'Error', diff --git a/packages/mcp-server/test/tools/execute-action.test.ts b/packages/mcp-server/test/tools/execute-action.test.ts index 0e17553db..7db07fc13 100644 --- a/packages/mcp-server/test/tools/execute-action.test.ts +++ b/packages/mcp-server/test/tools/execute-action.test.ts @@ -398,7 +398,7 @@ describe('declareExecuteActionTool', () => { }); describe('error handling', () => { - it('should return error as tool result with nested error.text structure in message', async () => { + it('should parse error with nested error.text structure in message', async () => { const errorPayload = { error: { status: 403, @@ -417,22 +417,15 @@ describe('declareExecuteActionTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - const result = await registeredToolHandler( - { collectionName: 'users', actionName: 'sendEmail', recordIds: [1] }, - mockExtra, - ); - expect(result).toEqual({ - content: [ - { - type: 'text', - text: expect.stringContaining('Cannot execute action on these records'), - }, - ], - isError: true, - }); + await expect( + registeredToolHandler( + { collectionName: 'users', actionName: 'sendEmail', recordIds: [1] }, + mockExtra, + ), + ).rejects.toThrow('Cannot execute action on these records'); }); - it('should return error as tool result when no parsable error found', async () => { + it('should rethrow original error when no parsable error found', async () => { const agentError = { unknownProperty: 'some value' }; const mockAction = jest.fn().mockRejectedValue(agentError); const mockCollection = jest.fn().mockReturnValue({ action: mockAction }); @@ -441,14 +434,12 @@ describe('declareExecuteActionTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - const result = await registeredToolHandler( - { collectionName: 'users', actionName: 'sendEmail', recordIds: [1] }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.any(String) }], - isError: true, - }); + await expect( + registeredToolHandler( + { collectionName: 'users', actionName: 'sendEmail', recordIds: [1] }, + mockExtra, + ), + ).rejects.toEqual(agentError); }); }); }); diff --git a/packages/mcp-server/test/tools/get-action-form.test.ts b/packages/mcp-server/test/tools/get-action-form.test.ts index 971287cf5..95e8f0ae9 100644 --- a/packages/mcp-server/test/tools/get-action-form.test.ts +++ b/packages/mcp-server/test/tools/get-action-form.test.ts @@ -720,7 +720,7 @@ describe('declareGetActionFormTool', () => { }); describe('error handling', () => { - it('should return error as tool result from the action call', async () => { + it('should propagate errors from the action call', async () => { const agentError = new Error('Action not found'); const mockAction = jest.fn().mockRejectedValue(agentError); const mockCollection = jest.fn().mockReturnValue({ action: mockAction }); @@ -729,17 +729,15 @@ describe('declareGetActionFormTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - const result = await registeredToolHandler( - { collectionName: 'users', actionName: 'nonExistent', recordIds: [1] }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Action not found') }], - isError: true, - }); + await expect( + registeredToolHandler( + { collectionName: 'users', actionName: 'nonExistent', recordIds: [1] }, + mockExtra, + ), + ).rejects.toThrow('Action not found'); }); - it('should return error as tool result from tryToSetFields call', async () => { + it('should propagate errors from tryToSetFields call', async () => { const tryToSetFieldsError = new Error('Invalid field value'); const mockGetFields = jest.fn().mockReturnValue([]); const mockTryToSetFields = jest.fn().mockRejectedValue(tryToSetFieldsError); @@ -753,19 +751,17 @@ describe('declareGetActionFormTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - const result = await registeredToolHandler( - { - collectionName: 'users', - actionName: 'sendEmail', - recordIds: [1], - values: { invalidField: 'value' }, - }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Invalid field value') }], - isError: true, - }); + await expect( + registeredToolHandler( + { + collectionName: 'users', + actionName: 'sendEmail', + recordIds: [1], + values: { invalidField: 'value' }, + }, + mockExtra, + ), + ).rejects.toThrow('Invalid field value'); }); }); }); diff --git a/packages/mcp-server/test/tools/list-related.test.ts b/packages/mcp-server/test/tools/list-related.test.ts index 72a1c4261..026abbba6 100644 --- a/packages/mcp-server/test/tools/list-related.test.ts +++ b/packages/mcp-server/test/tools/list-related.test.ts @@ -595,7 +595,7 @@ describe('declareListRelatedTool', () => { } as unknown as ReturnType); }); - it('should return error as tool result with nested error.text structure in message', async () => { + it('should parse error with nested error.text structure in message', async () => { const errorPayload = { error: { status: 400, @@ -610,17 +610,15 @@ describe('declareListRelatedTool', () => { mockFetchForestSchema.mockResolvedValue({ collections: [] }); mockGetFieldsOfCollection.mockReturnValue([]); - const result = await registeredToolHandler( - { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Invalid filters provided') }], - isError: true, - }); + await expect( + registeredToolHandler( + { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, + mockExtra, + ), + ).rejects.toThrow('Invalid filters provided'); }); - it('should return error as tool result with direct text property in message', async () => { + it('should parse error with direct text property in message', async () => { const errorPayload = { text: JSON.stringify({ errors: [{ name: 'ValidationError', detail: 'Direct text error' }], @@ -632,17 +630,15 @@ describe('declareListRelatedTool', () => { mockFetchForestSchema.mockResolvedValue({ collections: [] }); mockGetFieldsOfCollection.mockReturnValue([]); - const result = await registeredToolHandler( - { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Direct text error') }], - isError: true, - }); + await expect( + registeredToolHandler( + { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, + mockExtra, + ), + ).rejects.toThrow('Direct text error'); }); - it('should return helpful error message for Invalid sort errors', async () => { + it('should provide helpful error message for Invalid sort errors', async () => { const errorPayload = { error: { status: 400, @@ -692,27 +688,20 @@ describe('declareListRelatedTool', () => { mockFetchForestSchema.mockResolvedValue(mockSchema); mockGetFieldsOfCollection.mockReturnValue(mockFields); - const result = await registeredToolHandler( - { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, - mockExtra, + await expect( + registeredToolHandler( + { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, + mockExtra, + ), + ).rejects.toThrow( + 'The sort field provided is invalid for this collection. Available fields for the collection users are: id, total.', ); - expect(result).toEqual({ - content: [ - { - type: 'text', - text: expect.stringContaining( - 'The sort field provided is invalid for this collection. Available fields for the collection users are: id, total.', - ), - }, - ], - isError: true, - }); expect(mockFetchForestSchema).toHaveBeenCalledWith(mockForestServerClient); expect(mockGetFieldsOfCollection).toHaveBeenCalledWith(mockSchema, 'users'); }); - it('should return helpful error message when relation is not found', async () => { + it('should provide helpful error message when relation is not found', async () => { const agentError = new Error('Relation not found'); mockList.mockRejectedValue(agentError); @@ -757,21 +746,14 @@ describe('declareListRelatedTool', () => { mockFetchForestSchema.mockResolvedValue(mockSchema); mockGetFieldsOfCollection.mockReturnValue(mockFields); - const result = await registeredToolHandler( - { collectionName: 'users', relationName: 'invalidRelation', parentRecordId: 1 }, - mockExtra, + await expect( + registeredToolHandler( + { collectionName: 'users', relationName: 'invalidRelation', parentRecordId: 1 }, + mockExtra, + ), + ).rejects.toThrow( + 'The relation name provided is invalid for this collection. Available relations for collection users are: orders, reviews.', ); - expect(result).toEqual({ - content: [ - { - type: 'text', - text: expect.stringContaining( - 'The relation name provided is invalid for this collection. Available relations for collection users are: orders, reviews.', - ), - }, - ], - isError: true, - }); }); it('should include BelongsToMany relations in available relations error message', async () => { @@ -808,21 +790,14 @@ describe('declareListRelatedTool', () => { mockFetchForestSchema.mockResolvedValue(mockSchema); mockGetFieldsOfCollection.mockReturnValue(mockFields); - const result = await registeredToolHandler( - { collectionName: 'users', relationName: 'invalidRelation', parentRecordId: 1 }, - mockExtra, + await expect( + registeredToolHandler( + { collectionName: 'users', relationName: 'invalidRelation', parentRecordId: 1 }, + mockExtra, + ), + ).rejects.toThrow( + 'The relation name provided is invalid for this collection. Available relations for collection users are: orders, tags.', ); - expect(result).toEqual({ - content: [ - { - type: 'text', - text: expect.stringContaining( - 'The relation name provided is invalid for this collection. Available relations for collection users are: orders, tags.', - ), - }, - ], - isError: true, - }); }); it('should not show relation error when relation exists but error is different', async () => { @@ -848,15 +823,13 @@ describe('declareListRelatedTool', () => { mockFetchForestSchema.mockResolvedValue(mockSchema); mockGetFieldsOfCollection.mockReturnValue(mockFields); - // Should return the original error message since 'orders' relation exists - const result = await registeredToolHandler( - { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Some other error') }], - isError: true, - }); + // Should throw the original error message since 'orders' relation exists + await expect( + registeredToolHandler( + { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, + mockExtra, + ), + ).rejects.toThrow('Some other error'); }); it('should fall back to error.message when message is not valid JSON', async () => { @@ -881,14 +854,12 @@ describe('declareListRelatedTool', () => { }); mockGetFieldsOfCollection.mockReturnValue(mockFields); - const result = await registeredToolHandler( - { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Plain error message') }], - isError: true, - }); + await expect( + registeredToolHandler( + { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, + mockExtra, + ), + ).rejects.toThrow('Plain error message'); }); it('should handle string errors thrown directly', async () => { @@ -913,14 +884,12 @@ describe('declareListRelatedTool', () => { }); mockGetFieldsOfCollection.mockReturnValue(mockFields); - const result = await registeredToolHandler( - { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Connection failed') }], - isError: true, - }); + await expect( + registeredToolHandler( + { collectionName: 'users', relationName: 'orders', parentRecordId: 1 }, + mockExtra, + ), + ).rejects.toThrow('Connection failed'); }); }); }); diff --git a/packages/mcp-server/test/tools/list.test.ts b/packages/mcp-server/test/tools/list.test.ts index 8ef295f7b..efd0a7f3d 100644 --- a/packages/mcp-server/test/tools/list.test.ts +++ b/packages/mcp-server/test/tools/list.test.ts @@ -894,7 +894,7 @@ describe('declareListTool', () => { } as unknown as ReturnType); }); - it('should return error as tool result with nested error.text structure in message', async () => { + it('should parse error with nested error.text structure in message', async () => { // The RPC client throws an Error with message containing JSON: { error: { text: '...' } } const errorPayload = { error: { @@ -907,14 +907,12 @@ describe('declareListTool', () => { const agentError = new Error(JSON.stringify(errorPayload)); mockList.mockRejectedValue(agentError); - const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Invalid filters provided') }], - isError: true, - }); + await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( + 'Invalid filters provided', + ); }); - it('should return error as tool result with direct text property in message', async () => { + it('should parse error with direct text property in message', async () => { // The RPC client throws an Error with message containing JSON: { text: '...' } const errorPayload = { text: JSON.stringify({ @@ -924,14 +922,12 @@ describe('declareListTool', () => { const agentError = new Error(JSON.stringify(errorPayload)); mockList.mockRejectedValue(agentError); - const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Direct text error') }], - isError: true, - }); + await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( + 'Direct text error', + ); }); - it('should return error as tool result with message property from parsed JSON when no text field', async () => { + it('should use message property from parsed JSON when no text field', async () => { // The RPC client throws an Error with message containing JSON: { message: '...' } const errorPayload = { message: 'Error message from JSON payload', @@ -939,39 +935,31 @@ describe('declareListTool', () => { const agentError = new Error(JSON.stringify(errorPayload)); mockList.mockRejectedValue(agentError); - const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); - expect(result).toEqual({ - content: [ - { type: 'text', text: expect.stringContaining('Error message from JSON payload') }, - ], - isError: true, - }); + await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( + 'Error message from JSON payload', + ); }); - it('should return error as tool result when message is not valid JSON', async () => { + it('should fall back to error.message when message is not valid JSON', async () => { // The RPC client throws an Error with a plain string message (not JSON) const agentError = new Error('Plain error message'); mockList.mockRejectedValue(agentError); - const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Plain error message') }], - isError: true, - }); + await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( + 'Plain error message', + ); }); - it('should return error as tool result for string errors thrown directly', async () => { + it('should handle string errors thrown directly', async () => { // Some libraries throw string errors directly mockList.mockRejectedValue('Connection failed'); - const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Connection failed') }], - isError: true, - }); + await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( + 'Connection failed', + ); }); - it('should return helpful error message for Invalid sort errors', async () => { + it('should provide helpful error message for Invalid sort errors', async () => { // The RPC client throws an "Invalid sort" error const errorPayload = { error: { @@ -1033,18 +1021,9 @@ describe('declareListTool', () => { mockFetchForestSchema.mockResolvedValue(mockSchema); mockGetFieldsOfCollection.mockReturnValue(mockFields); - const result = await registeredToolHandler({ collectionName: 'users' }, mockExtra); - expect(result).toEqual({ - content: [ - { - type: 'text', - text: expect.stringContaining( - 'The sort field provided is invalid for this collection. Available fields for the collection users are: id, name, email.', - ), - }, - ], - isError: true, - }); + await expect(registeredToolHandler({ collectionName: 'users' }, mockExtra)).rejects.toThrow( + 'The sort field provided is invalid for this collection. Available fields for the collection users are: id, name, email.', + ); expect(mockFetchForestSchema).toHaveBeenCalledWith(mockForestServerClient); expect(mockGetFieldsOfCollection).toHaveBeenCalledWith(mockSchema, 'users'); diff --git a/packages/mcp-server/test/tools/update.test.ts b/packages/mcp-server/test/tools/update.test.ts index e18d6444b..4e71aae5e 100644 --- a/packages/mcp-server/test/tools/update.test.ts +++ b/packages/mcp-server/test/tools/update.test.ts @@ -262,7 +262,7 @@ describe('declareUpdateTool', () => { }); describe('error handling', () => { - it('should return error as tool result with nested error.text structure in message', async () => { + it('should parse error with nested error.text structure in message', async () => { const errorPayload = { error: { status: 404, @@ -279,17 +279,15 @@ describe('declareUpdateTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - const result = await registeredToolHandler( - { collectionName: 'users', recordId: 999, attributes: {} }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('Record not found') }], - isError: true, - }); + await expect( + registeredToolHandler( + { collectionName: 'users', recordId: 999, attributes: {} }, + mockExtra, + ), + ).rejects.toThrow('Record not found'); }); - it('should return error as tool result when no parsable error found', async () => { + it('should rethrow original error when no parsable error found', async () => { const agentError = { unknownProperty: 'some value' }; const mockUpdate = jest.fn().mockRejectedValue(agentError); const mockCollection = jest.fn().mockReturnValue({ update: mockUpdate }); @@ -298,14 +296,12 @@ describe('declareUpdateTool', () => { authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 }, } as unknown as ReturnType); - const result = await registeredToolHandler( - { collectionName: 'users', recordId: 1, attributes: {} }, - mockExtra, - ); - expect(result).toEqual({ - content: [{ type: 'text', text: expect.any(String) }], - isError: true, - }); + await expect( + registeredToolHandler( + { collectionName: 'users', recordId: 1, attributes: {} }, + mockExtra, + ), + ).rejects.toEqual(agentError); }); }); }); diff --git a/packages/mcp-server/test/utils/tool-with-logging.test.ts b/packages/mcp-server/test/utils/tool-with-logging.test.ts index a804ccad2..bbf140ec5 100644 --- a/packages/mcp-server/test/utils/tool-with-logging.test.ts +++ b/packages/mcp-server/test/utils/tool-with-logging.test.ts @@ -121,17 +121,15 @@ describe('registerToolWithLogging', () => { expect(result).toBe(expectedResult); }); - it('should return error as tool result instead of throwing', async () => { + it('should propagate handler errors without logging', async () => { const error = new Error('Handler failed'); const handler = jest.fn().mockRejectedValue(error); registerToolWithLogging(mockMcpServer as never, 'test-tool', toolConfig, handler, mockLogger); - const result = await registeredHandler({ name: 'test', count: 42 }, {}); - expect(result).toEqual({ - content: [{ type: 'text', text: 'Handler failed' }], - isError: true, - }); + await expect(registeredHandler({ name: 'test', count: 42 }, {})).rejects.toThrow( + 'Handler failed', + ); expect(mockLogger).not.toHaveBeenCalled(); });