From 8e4114abbfab39e29d17872574acd25e48c10408 Mon Sep 17 00:00:00 2001 From: Thomas Kaul <4159106+dtslvr@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:25:17 +0200 Subject: [PATCH] Task/improve MCP (#7786) * Improve MCP server * Update changelog --- CHANGELOG.md | 1 + .../app/endpoints/mcp/mcp.controller.spec.ts | 154 ++++++++------- .../src/app/endpoints/mcp/mcp.controller.ts | 175 +++--------------- apps/api/src/app/endpoints/mcp/mcp.module.ts | 2 + apps/api/src/app/endpoints/mcp/mcp.schemas.ts | 115 ++++++++++++ apps/api/src/app/endpoints/mcp/mcp.service.ts | 8 + .../import/errors/import-validation.error.ts | 4 +- apps/api/src/errors/caller-facing.error.ts | 12 ++ .../filters/mcp-tool-exception.filter.spec.ts | 83 +++++++++ .../src/filters/mcp-tool-exception.filter.ts | 30 +-- 10 files changed, 357 insertions(+), 227 deletions(-) create mode 100644 apps/api/src/app/endpoints/mcp/mcp.schemas.ts create mode 100644 apps/api/src/app/endpoints/mcp/mcp.service.ts create mode 100644 apps/api/src/errors/caller-facing.error.ts create mode 100644 apps/api/src/filters/mcp-tool-exception.filter.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 9913c0e29..23891b0d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Improved the server of the Model Context Protocol (MCP) (experimental) - Introduced a maximum length for the comment in the API endpoints - Introduced a maximum length for the search query and the symbol in the API endpoints - Hardened the validation of the query parameters (`accounts`, `assetClasses`, `dataSource` and `tags`) in the API endpoints with filters diff --git a/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts b/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts index b07441e73..2c84df6d0 100644 --- a/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts +++ b/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts @@ -2,25 +2,29 @@ import { ImportValidationError } from '@ghostfolio/api/app/import/errors/import- import { ImportService } from '@ghostfolio/api/app/import/import.service'; import { UserService } from '@ghostfolio/api/app/user/user.service'; import { REQUIRES_SCOPE_KEY } from '@ghostfolio/api/decorators/requires-scope.decorator'; +import { McpToolExceptionFilter } from '@ghostfolio/api/filters/mcp-tool-exception.filter'; +import { AccessGuard } from '@ghostfolio/api/guards/access.guard'; import { ConfigurationService } from '@ghostfolio/api/services/configuration/configuration.service'; import { MCP_MAX_ACTIVITIES } from '@ghostfolio/common/config'; import { Activity } from '@ghostfolio/common/interfaces'; import { permissions } from '@ghostfolio/common/permissions'; -import { scopes } from '@ghostfolio/common/scopes'; +import { Scope, scopes } from '@ghostfolio/common/scopes'; import type { ImpersonationContext, UserWithSettings } from '@ghostfolio/common/types'; -import { HttpException, Logger } from '@nestjs/common'; -import { RpcException } from '@nestjs/microservices'; +import { HttpException } from '@nestjs/common'; +import { + EXCEPTION_FILTERS_METADATA, + GUARDS_METADATA +} from '@nestjs/common/constants'; import { DataSource, Type as ActivityType } from '@prisma/client'; -import { getReasonPhrase, StatusCodes } from 'http-status-codes'; +import { MCP_TOOL_METADATA_KEY, ToolMetadata } from '@rekog/mcp-nest'; -import { - GhostfolioMcpController, - IMPORT_ACTIVITIES_PARAMETERS -} from './mcp.controller'; +import { GhostfolioMcpController } from './mcp.controller'; +import { IMPORT_ACTIVITIES_PARAMETERS } from './mcp.schemas'; +import { McpService } from './mcp.service'; // The controller reads the columns of the tables from the AiService, which // imports two packages which ship as an ECMAScript module only, which Jest @@ -34,6 +38,30 @@ jest.mock('ai', () => { return { generateText: jest.fn() }; }); +/** + * Gives the metadata which a decorator sets on the method of a tool. The + * prototype is read by the name of the method, hence the type of the metadata + * is given by the caller. + */ +function getMetadataOfMethod(metadataKey: string, methodName: string) { + const methodsByName = GhostfolioMcpController.prototype as unknown as Record< + string, + object + >; + + return Reflect.getMetadata(metadataKey, methodsByName[methodName]) as T; +} + +function getToolMethodNames() { + return Object.getOwnPropertyNames(GhostfolioMcpController.prototype).filter( + (methodName) => { + return Boolean( + getMetadataOfMethod(MCP_TOOL_METADATA_KEY, methodName) + ); + } + ); +} + function createActivity(overrides: Record = {}) { return { currency: 'USD', @@ -85,6 +113,7 @@ describe('GhostfolioMcpController', () => { undefined, configurationService, importService, + new McpService(), userService ); }); @@ -93,13 +122,57 @@ describe('GhostfolioMcpController', () => { jest.restoreAllMocks(); }); + describe('Tools', () => { + // A tool without the decorator of the scope would be open to every access, + // hence a new tool has to declare its scope + it('Requires a scope of access for each tool', () => { + const toolMethodNames = getToolMethodNames(); + + expect(toolMethodNames.length).toBeGreaterThan(0); + + const toolMethodNamesWithoutScope = toolMethodNames.filter( + (methodName) => { + return !getMetadataOfMethod(REQUIRES_SCOPE_KEY, methodName) + ?.length; + } + ); + + expect(toolMethodNamesWithoutScope).toEqual([]); + }); + + // The decorator RequiresScope sets the same metadata as the decorator + // RequiresScopeOfAccess, but applies AuthGuard('jwt'), which a request of + // an access cannot pass, hence the guards tell the two decorators apart + it('Applies the guard of the access to each tool', () => { + const toolMethodNames = getToolMethodNames(); + + expect(toolMethodNames.length).toBeGreaterThan(0); + + const toolMethodNamesWithoutGuardOfAccess = toolMethodNames.filter( + (methodName) => { + return !getMetadataOfMethod( + GUARDS_METADATA, + methodName + )?.includes(AccessGuard); + } + ); + + expect(toolMethodNamesWithoutGuardOfAccess).toEqual([]); + }); + + // The tools have no try and catch, hence the filter is the only guarantee + // that an unexpected exception does not expose internals + it('Applies the filter of the exceptions of the tools', () => { + expect( + Reflect.getMetadata(EXCEPTION_FILTERS_METADATA, GhostfolioMcpController) + ).toEqual([McpToolExceptionFilter]); + }); + }); + describe('Import activities', () => { it('Requires the scope to create an activity', () => { expect( - Reflect.getMetadata( - REQUIRES_SCOPE_KEY, - GhostfolioMcpController.prototype.importActivities - ) + getMetadataOfMethod(REQUIRES_SCOPE_KEY, 'importActivities') ).toEqual([scopes.activityCreate]); }); @@ -174,68 +247,21 @@ describe('GhostfolioMcpController', () => { ); }); - it('Passes on the message of a validation only', async () => { - setupUser([permissions.createActivity]); - - jest - .spyOn(importService, 'import') - .mockRejectedValue( - new ImportValidationError('activities.0.symbol ("X") is not valid') - ); - - await expect( - controller.importActivities(impersonation, { - activities: [createActivity()] - }) - ).rejects.toThrow( - new RpcException('activities.0.symbol ("X") is not valid') - ); - }); - - it('Hides the message of an unexpected error and writes it to the log', async () => { + // The McpToolExceptionFilter maps the error, hence the tool passes it on + it('Passes on an error of the import', async () => { setupUser([permissions.createActivity]); - const error = new Error( - 'Unique constraint failed on the fields: (dataSource)' + const error = new ImportValidationError( + 'activities.0.symbol ("X") is not valid' ); - const logError = jest - .spyOn(Logger.prototype, 'error') - .mockImplementation(); - jest.spyOn(importService, 'import').mockRejectedValue(error); await expect( controller.importActivities(impersonation, { activities: [createActivity()] }) - ).rejects.toThrow( - new RpcException(getReasonPhrase(StatusCodes.INTERNAL_SERVER_ERROR)) - ); - - expect(logError).toHaveBeenCalledWith(error); - }); - - it('Does not write the message of a validation to the log', async () => { - setupUser([permissions.createActivity]); - - const logError = jest - .spyOn(Logger.prototype, 'error') - .mockImplementation(); - - jest - .spyOn(importService, 'import') - .mockRejectedValue( - new ImportValidationError('activities.0.accountId ("X") is not valid') - ); - - await expect( - controller.importActivities(impersonation, { - activities: [createActivity({ accountId: 'X' })] - }) - ).rejects.toThrow(RpcException); - - expect(logError).not.toHaveBeenCalled(); + ).rejects.toBe(error); }); }); diff --git a/apps/api/src/app/endpoints/mcp/mcp.controller.ts b/apps/api/src/app/endpoints/mcp/mcp.controller.ts index 307724ea1..420acab3e 100644 --- a/apps/api/src/app/endpoints/mcp/mcp.controller.ts +++ b/apps/api/src/app/endpoints/mcp/mcp.controller.ts @@ -1,154 +1,43 @@ import { AiService } from '@ghostfolio/api/app/endpoints/ai/ai.service'; -import { ImportValidationError } from '@ghostfolio/api/app/import/errors/import-validation.error'; import { ImportService } from '@ghostfolio/api/app/import/import.service'; import { UserService } from '@ghostfolio/api/app/user/user.service'; import { Impersonation } from '@ghostfolio/api/decorators/impersonation.decorator'; import { RequiresScopeOfAccess } from '@ghostfolio/api/decorators/requires-scope-of-access.decorator'; -import { DATE_RANGE_PATTERN } from '@ghostfolio/api/dtos/date-range-filter.dto'; import { McpToolExceptionFilter } from '@ghostfolio/api/filters/mcp-tool-exception.filter'; import { getUnmaskedGhostfolioDataSource } from '@ghostfolio/api/helper/data-source.helper'; import { ApiService } from '@ghostfolio/api/services/api/api.service'; import { ConfigurationService } from '@ghostfolio/api/services/configuration/configuration.service'; import { getIntervalFromDateRange } from '@ghostfolio/common/calculation-helper'; import { - DATE_RANGES, DEFAULT_LANGUAGE_CODE, - MCP_MAX_ACCOUNTS, MCP_MAX_ACTIVITIES } from '@ghostfolio/common/config'; -import { - isValidCurrencyCode, - isValidDateAfter1970 -} from '@ghostfolio/common/helper'; -import { Activity } from '@ghostfolio/common/interfaces'; import { hasPermission, permissions } from '@ghostfolio/common/permissions'; import { scopes } from '@ghostfolio/common/scopes'; import type { ImpersonationContext } from '@ghostfolio/common/types'; -import { HttpException, Logger, UseFilters } from '@nestjs/common'; -import { Payload, RpcException } from '@nestjs/microservices'; -import { AssetClass, DataSource, Type as ActivityType } from '@prisma/client'; +import { HttpException, UseFilters } from '@nestjs/common'; +import { Payload } from '@nestjs/microservices'; import { McpController, Tool } from '@rekog/mcp-nest'; import { getReasonPhrase, StatusCodes } from 'http-status-codes'; import { z } from 'zod'; -const GET_ACCOUNTS_PARAMETERS = z.object({ - accountIds: z - .array(z.string().min(1)) - .min(1) - .max(MCP_MAX_ACCOUNTS) - .optional() - .describe( - `The identifiers of the accounts to get, at most ${MCP_MAX_ACCOUNTS}` - ), - assetClasses: z - .array(z.enum(AssetClass)) - .min(1) - .optional() - .describe('The asset classes of the accounts to get'), - holding: z - .object({ - dataSource: z - .enum(DataSource) - .describe('The data source of the asset profile'), - symbol: z.string().describe('The symbol of the asset profile') - }) - .optional() - .describe('The asset profile of the accounts to get') -}); - -const GET_ACTIVITIES_PARAMETERS = z.object({ - activityTypes: z - .array(z.enum(ActivityType)) - .min(1) - .optional() - .describe('The types of the activities to get'), - assetClasses: z - .array(z.enum(AssetClass)) - .min(1) - .optional() - .describe('The asset classes of the activities to get'), - holding: z - .object({ - dataSource: z - .enum(DataSource) - .describe('The data source of the asset profile'), - symbol: z.string().describe('The symbol of the asset profile') - }) - .optional() - .describe('The asset profile of the activities to get'), - range: z - .string() - .regex(DATE_RANGE_PATTERN) - .optional() - .describe( - `The date range of the activities to get, either ${DATE_RANGES.join( - ', ' - )} or a calendar year like 2024` - ), - skip: z - .number() - .int() - .min(0) - .optional() - .describe('The number of activities to skip'), - take: z - .number() - .int() - .min(1) - .max(MCP_MAX_ACTIVITIES) - .optional() - .describe(`The number of activities to get, at most ${MCP_MAX_ACTIVITIES}`) -}); - -export const IMPORT_ACTIVITIES_PARAMETERS = z.object({ - activities: z - .array( - z.object({ - accountId: z - .string() - .min(1) - .optional() - .describe('The identifier of the account of the activity'), - comment: z.string().optional().describe('The comment of the activity'), - currency: z - .string() - .refine(isValidCurrencyCode) - .describe( - 'The currency of the fee and of the unit price, as an ISO 4217 code in upper case' - ), - dataSource: z - .enum(DataSource) - .optional() - .describe('The data source of the asset profile'), - date: z - .string() - .refine(isValidDateAfter1970) - .describe( - 'The date of the activity, as an ISO 8601 date or date and time' - ), - fee: z.number().min(0).describe('The fee of the activity'), - quantity: z.number().min(0).describe('The quantity of the activity'), - symbol: z.string().min(1).describe('The symbol of the asset profile'), - type: z.enum(ActivityType).describe('The type of the activity'), - unitPrice: z.number().min(0).describe('The unit price of the activity') - }) - ) - .min(1) - .max(MCP_MAX_ACTIVITIES) - .describe(`The activities to import, at most ${MCP_MAX_ACTIVITIES}`) -}); +import { + GET_ACCOUNTS_PARAMETERS, + GET_ACTIVITIES_PARAMETERS, + IMPORT_ACTIVITIES_PARAMETERS +} from './mcp.schemas'; +import { McpService } from './mcp.service'; @McpController() @UseFilters(McpToolExceptionFilter) export class GhostfolioMcpController { - private readonly logger = new Logger(GhostfolioMcpController.name); - public constructor( private readonly aiService: AiService, private readonly apiService: ApiService, private readonly configurationService: ConfigurationService, private readonly importService: ImportService, + private readonly mcpService: McpService, private readonly userService: UserService ) {} @@ -183,7 +72,7 @@ export class GhostfolioMcpController { const table = await this.aiService.getAccountsTable({ filters, userId }); - return { content: [{ text: table, type: 'text' as const }] }; + return this.mcpService.getTextResult(table); } @RequiresScopeOfAccess(scopes.activityRead) @@ -232,13 +121,13 @@ export class GhostfolioMcpController { filters, skip, startDate, + take, userId, - take: take ?? MCP_MAX_ACTIVITIES, types: activityTypes, userCurrency: userSettings.baseCurrency }); - return { content: [{ text: table, type: 'text' as const }] }; + return this.mcpService.getTextResult(table); } @RequiresScopeOfAccess(scopes.portfolioRead) @@ -263,7 +152,7 @@ export class GhostfolioMcpController { userCurrency: userSettings.baseCurrency }); - return { content: [{ text: prompt, type: 'text' as const }] }; + return this.mcpService.getTextResult(prompt); } /** @@ -313,32 +202,16 @@ export class GhostfolioMcpController { }; }); - let importedActivities: Activity[]; - - try { - importedActivities = await this.importService.import({ - activitiesDto, - user, - accountsWithBalancesDto: [], - assetProfilesWithMarketDataDto: [], - platformsDto: [], - tagsDto: [] - }); - } catch (error) { - // The message of a validation names the activity which is not valid and - // is written for the caller, hence it is passed on - if (error instanceof ImportValidationError) { - throw new RpcException(error.message); - } - - // Every other message can carry internals of the application, hence it - // is written to the log and the reason phrase is passed on instead - this.logger.error(error); - - throw new RpcException( - getReasonPhrase(StatusCodes.INTERNAL_SERVER_ERROR) - ); - } + // The filter passes on the message of a CallerFacingError, which is + // written for the caller, and hides the message of every other error + const importedActivities = await this.importService.import({ + activitiesDto, + user, + accountsWithBalancesDto: [], + assetProfilesWithMarketDataDto: [], + platformsDto: [], + tagsDto: [] + }); const text = [ `Imported activities: ${importedActivities.length}`, @@ -347,6 +220,6 @@ export class GhostfolioMcpController { }` ].join('\n'); - return { content: [{ text, type: 'text' as const }] }; + return this.mcpService.getTextResult(text); } } diff --git a/apps/api/src/app/endpoints/mcp/mcp.module.ts b/apps/api/src/app/endpoints/mcp/mcp.module.ts index e2fbce79f..0c054c7fb 100644 --- a/apps/api/src/app/endpoints/mcp/mcp.module.ts +++ b/apps/api/src/app/endpoints/mcp/mcp.module.ts @@ -15,11 +15,13 @@ import { } from '@rekog/mcp-nest'; import { GhostfolioMcpController } from './mcp.controller'; +import { McpService } from './mcp.service'; @Module({ controllers: [GhostfolioMcpController], imports: [AiModule, ApiModule, ConfigurationModule, ImportModule, UserModule], providers: [ + McpService, { inject: [ConfigurationService], provide: MCP_STRATEGY, diff --git a/apps/api/src/app/endpoints/mcp/mcp.schemas.ts b/apps/api/src/app/endpoints/mcp/mcp.schemas.ts new file mode 100644 index 000000000..44568890a --- /dev/null +++ b/apps/api/src/app/endpoints/mcp/mcp.schemas.ts @@ -0,0 +1,115 @@ +import { DATE_RANGE_PATTERN } from '@ghostfolio/api/dtos/date-range-filter.dto'; +import { + DATE_RANGES, + MCP_MAX_ACCOUNTS, + MCP_MAX_ACTIVITIES +} from '@ghostfolio/common/config'; +import { + isValidCurrencyCode, + isValidDateAfter1970 +} from '@ghostfolio/common/helper'; + +import { AssetClass, DataSource, Type as ActivityType } from '@prisma/client'; +import { z } from 'zod'; + +const HOLDING_PARAMETER = z.object({ + dataSource: z + .enum(DataSource) + .describe('The data source of the asset profile'), + symbol: z.string().describe('The symbol of the asset profile') +}); + +export const GET_ACCOUNTS_PARAMETERS = z.object({ + accountIds: z + .array(z.string().min(1)) + .min(1) + .max(MCP_MAX_ACCOUNTS) + .optional() + .describe( + `The identifiers of the accounts to get, at most ${MCP_MAX_ACCOUNTS}` + ), + assetClasses: z + .array(z.enum(AssetClass)) + .min(1) + .optional() + .describe('The asset classes of the accounts to get'), + holding: HOLDING_PARAMETER.optional().describe( + 'The asset profile of the accounts to get' + ) +}); + +export const GET_ACTIVITIES_PARAMETERS = z.object({ + activityTypes: z + .array(z.enum(ActivityType)) + .min(1) + .optional() + .describe('The types of the activities to get'), + assetClasses: z + .array(z.enum(AssetClass)) + .min(1) + .optional() + .describe('The asset classes of the activities to get'), + holding: HOLDING_PARAMETER.optional().describe( + 'The asset profile of the activities to get' + ), + range: z + .string() + .regex(DATE_RANGE_PATTERN) + .optional() + .describe( + `The date range of the activities to get, either ${DATE_RANGES.join( + ', ' + )} or a calendar year like 2024` + ), + skip: z + .number() + .int() + .min(0) + .optional() + .describe('The number of activities to skip'), + take: z + .number() + .int() + .min(1) + .max(MCP_MAX_ACTIVITIES) + .default(MCP_MAX_ACTIVITIES) + .describe(`The number of activities to get, at most ${MCP_MAX_ACTIVITIES}`) +}); + +export const IMPORT_ACTIVITIES_PARAMETERS = z.object({ + activities: z + .array( + z.object({ + accountId: z + .string() + .min(1) + .optional() + .describe('The identifier of the account of the activity'), + comment: z.string().optional().describe('The comment of the activity'), + currency: z + .string() + .refine(isValidCurrencyCode) + .describe( + 'The currency of the fee and of the unit price, as an ISO 4217 code in upper case' + ), + dataSource: z + .enum(DataSource) + .optional() + .describe('The data source of the asset profile'), + date: z + .string() + .refine(isValidDateAfter1970) + .describe( + 'The date of the activity, as an ISO 8601 date or date and time' + ), + fee: z.number().min(0).describe('The fee of the activity'), + quantity: z.number().min(0).describe('The quantity of the activity'), + symbol: z.string().min(1).describe('The symbol of the asset profile'), + type: z.enum(ActivityType).describe('The type of the activity'), + unitPrice: z.number().min(0).describe('The unit price of the activity') + }) + ) + .min(1) + .max(MCP_MAX_ACTIVITIES) + .describe(`The activities to import, at most ${MCP_MAX_ACTIVITIES}`) +}); diff --git a/apps/api/src/app/endpoints/mcp/mcp.service.ts b/apps/api/src/app/endpoints/mcp/mcp.service.ts new file mode 100644 index 000000000..2da6003b4 --- /dev/null +++ b/apps/api/src/app/endpoints/mcp/mcp.service.ts @@ -0,0 +1,8 @@ +import { Injectable } from '@nestjs/common'; + +@Injectable() +export class McpService { + public getTextResult(text: string) { + return { content: [{ text, type: 'text' as const }] }; + } +} diff --git a/apps/api/src/app/import/errors/import-validation.error.ts b/apps/api/src/app/import/errors/import-validation.error.ts index a228c9933..4e96ce233 100644 --- a/apps/api/src/app/import/errors/import-validation.error.ts +++ b/apps/api/src/app/import/errors/import-validation.error.ts @@ -1,4 +1,6 @@ -export class ImportValidationError extends Error { +import { CallerFacingError } from '@ghostfolio/api/errors/caller-facing.error'; + +export class ImportValidationError extends CallerFacingError { public constructor(message: string) { super(message); diff --git a/apps/api/src/errors/caller-facing.error.ts b/apps/api/src/errors/caller-facing.error.ts new file mode 100644 index 000000000..5b7687a19 --- /dev/null +++ b/apps/api/src/errors/caller-facing.error.ts @@ -0,0 +1,12 @@ +/** + * An error whose message is written for the caller. A filter passes such a + * message on, while it hides the message of every other error, because that + * message can carry internals of the application. + */ +export class CallerFacingError extends Error { + public constructor(message: string) { + super(message); + + this.name = 'CallerFacingError'; + } +} diff --git a/apps/api/src/filters/mcp-tool-exception.filter.spec.ts b/apps/api/src/filters/mcp-tool-exception.filter.spec.ts new file mode 100644 index 000000000..23eb6ff92 --- /dev/null +++ b/apps/api/src/filters/mcp-tool-exception.filter.spec.ts @@ -0,0 +1,83 @@ +import { ImportValidationError } from '@ghostfolio/api/app/import/errors/import-validation.error'; +import { PortfolioSnapshotComputationError } from '@ghostfolio/api/app/portfolio/errors/portfolio-snapshot-computation.error'; + +import { ForbiddenException, Logger } from '@nestjs/common'; +import { getReasonPhrase, StatusCodes } from 'http-status-codes'; +import { firstValueFrom } from 'rxjs'; + +import { McpToolExceptionFilter } from './mcp-tool-exception.filter'; + +describe('McpToolExceptionFilter', () => { + let filter: McpToolExceptionFilter; + let logError: jest.SpyInstance; + + async function getErrorOfException(exception: unknown) { + try { + await firstValueFrom(filter.catch(exception)); + } catch (error) { + return error; + } + + throw new Error('The filter gave no error'); + } + + beforeEach(() => { + filter = new McpToolExceptionFilter(); + + logError = jest.spyOn(Logger.prototype, 'error').mockImplementation(); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('Passes on the message of an error which is written for the caller', async () => { + const exception = new ImportValidationError( + 'activities.0.symbol ("X") is not valid' + ); + + expect(await getErrorOfException(exception)).toEqual({ + message: 'activities.0.symbol ("X") is not valid', + status: 'error' + }); + + expect(logError).not.toHaveBeenCalled(); + }); + + it('Hides the message of an unexpected error and writes it to the log', async () => { + const exception = new Error( + 'Unique constraint failed on the fields: (dataSource)' + ); + + expect(await getErrorOfException(exception)).toEqual({ + message: getReasonPhrase(StatusCodes.INTERNAL_SERVER_ERROR), + status: 'error' + }); + + expect(logError).toHaveBeenCalledWith(exception); + }); + + // An access without the scope of a tool causes a refused call at each + // attempt, which would fill the log + it('Gives the reason phrase of the status of an HttpException and writes no log', async () => { + expect(await getErrorOfException(new ForbiddenException())).toEqual({ + message: getReasonPhrase(StatusCodes.FORBIDDEN), + status: 'error' + }); + + expect(logError).not.toHaveBeenCalled(); + }); + + it('Gives the reason phrase of a service which is not available if a snapshot cannot be computed', async () => { + const exception = new PortfolioSnapshotComputationError( + 'The snapshot cannot be computed' + ); + + expect(await getErrorOfException(exception)).toEqual({ + message: getReasonPhrase(StatusCodes.SERVICE_UNAVAILABLE), + status: 'error' + }); + + expect(logError).toHaveBeenCalledWith(exception); + }); +}); diff --git a/apps/api/src/filters/mcp-tool-exception.filter.ts b/apps/api/src/filters/mcp-tool-exception.filter.ts index 7525c6559..6630c1c1c 100644 --- a/apps/api/src/filters/mcp-tool-exception.filter.ts +++ b/apps/api/src/filters/mcp-tool-exception.filter.ts @@ -1,4 +1,5 @@ import { PortfolioSnapshotComputationError } from '@ghostfolio/api/app/portfolio/errors/portfolio-snapshot-computation.error'; +import { CallerFacingError } from '@ghostfolio/api/errors/caller-facing.error'; import { Catch, @@ -6,7 +7,6 @@ import { Logger, RpcExceptionFilter } from '@nestjs/common'; -import { RpcException } from '@nestjs/microservices'; import { getReasonPhrase, StatusCodes } from 'http-status-codes'; import { Observable, throwError } from 'rxjs'; @@ -20,24 +20,32 @@ export class McpToolExceptionFilter implements RpcExceptionFilter { private readonly logger = new Logger(McpToolExceptionFilter.name); public catch(exception: unknown): Observable { - this.logger.error(exception); - - if (exception instanceof RpcException) { + // The message of this exception is written for the caller, hence it is + // passed on and is not written to the log + if (exception instanceof CallerFacingError) { return throwError(() => { - return exception.getError(); + return { message: exception.message, status: 'error' }; }); } - return throwError(() => { - return { message: this.getMessage(exception), status: 'error' }; - }); - } + const statusCode = this.getStatus(exception); + + // An exception which the caller causes, for example a refused call, is + // expected, hence only an exception of the application is written to the + // log + if (statusCode >= StatusCodes.INTERNAL_SERVER_ERROR) { + this.logger.error(exception); + } - private getMessage(exception: unknown) { // The message of an exception can carry internals, for example the // property names of a data transfer object of a failed validation, hence // the reason phrase of the status is passed on instead - return this.getReasonPhraseOfStatus(this.getStatus(exception)); + return throwError(() => { + return { + message: this.getReasonPhraseOfStatus(statusCode), + status: 'error' + }; + }); } private getReasonPhraseOfStatus(statusCode: number) {