Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions packages/mcp-server/src/utils/tool-with-logging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -99,7 +99,26 @@ 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.
// See: https://modelcontextprotocol.io/docs/concepts/tools
try {
return await handler(args as TArgs, extra);
} catch (error) {
let message: string;

try {
message = error instanceof Error ? error.message : JSON.stringify(error) ?? String(error);
} catch {
message = String(error);
}

return {
content: [{ type: 'text', text: message }],
isError: true,
};
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
}) as any,
);
Expand Down
24 changes: 17 additions & 7 deletions packages/mcp-server/test/tools/create.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -264,12 +264,17 @@ describe('declareCreateTool', () => {
authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 },
} as unknown as ReturnType<typeof buildClient>);

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 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 });
Expand All @@ -278,9 +283,14 @@ describe('declareCreateTool', () => {
authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 },
} as unknown as ReturnType<typeof buildClient>);

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: '{"unknownProperty":"some value"}' }],
isError: true,
});
});
});
});
Expand Down
26 changes: 19 additions & 7 deletions packages/mcp-server/test/tools/delete.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,12 +252,19 @@ describe('declareDeleteTool', () => {
authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 },
} as unknown as ReturnType<typeof buildClient>);

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 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 });
Expand All @@ -266,9 +273,14 @@ describe('declareDeleteTool', () => {
authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 },
} as unknown as ReturnType<typeof buildClient>);

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: '{"unknownProperty":"some value"}' }],
isError: true,
});
});
});
});
Expand Down
10 changes: 6 additions & 4 deletions packages/mcp-server/test/tools/describe-collection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ describe('declareDescribeCollectionTool', () => {
);
});

it('should throw and log Error for non-404 capabilities errors', async () => {
it('should return error 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({
Expand All @@ -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',
Expand Down
35 changes: 22 additions & 13 deletions packages/mcp-server/test/tools/execute-action.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -417,15 +417,22 @@ describe('declareExecuteActionTool', () => {
authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 },
} as unknown as ReturnType<typeof buildClientWithActions>);

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 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 });
Expand All @@ -434,12 +441,14 @@ describe('declareExecuteActionTool', () => {
authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 },
} as unknown as ReturnType<typeof buildClientWithActions>);

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: '{"unknownProperty":"some value"}' }],
isError: true,
});
});
});
});
Expand Down
38 changes: 21 additions & 17 deletions packages/mcp-server/test/tools/get-action-form.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -729,12 +729,14 @@ describe('declareGetActionFormTool', () => {
authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 },
} as unknown as ReturnType<typeof buildClientWithActions>);

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 () => {
Expand All @@ -751,17 +753,19 @@ describe('declareGetActionFormTool', () => {
authData: { userId: 1, renderingId: '123', environmentId: 1, projectId: 1 },
} as unknown as ReturnType<typeof buildClientWithActions>);

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,
});
});
});
});
Expand Down
Loading
Loading