diff --git a/.changeset/fix-mcp-error-forwarding.md b/.changeset/fix-mcp-error-forwarding.md new file mode 100644 index 0000000000..fa12e224a9 --- /dev/null +++ b/.changeset/fix-mcp-error-forwarding.md @@ -0,0 +1,7 @@ +--- +'@backstage/backend-defaults': patch +'@backstage/backend-test-utils': patch +'@backstage/plugin-mcp-actions-backend': patch +--- + +Fixed error forwarding in the actions registry so that known errors like `InputError` and `NotFoundError` thrown by actions preserve their original status codes and messages instead of being wrapped in `ForwardedError` and coerced to 500. diff --git a/packages/backend-defaults/src/alpha/entrypoints/actionsRegistry/DefaultActionsRegistryService.ts b/packages/backend-defaults/src/alpha/entrypoints/actionsRegistry/DefaultActionsRegistryService.ts index 7348e0a185..ffe74f81d9 100644 --- a/packages/backend-defaults/src/alpha/entrypoints/actionsRegistry/DefaultActionsRegistryService.ts +++ b/packages/backend-defaults/src/alpha/entrypoints/actionsRegistry/DefaultActionsRegistryService.ts @@ -28,12 +28,7 @@ import { ActionsRegistryActionOptions, ActionsRegistryService, } from '@backstage/backend-plugin-api/alpha'; -import { - ForwardedError, - InputError, - NotAllowedError, - NotFoundError, -} from '@backstage/errors'; +import { InputError, NotAllowedError, NotFoundError } from '@backstage/errors'; export class DefaultActionsRegistryService implements ActionsRegistryService { private actions: Map> = @@ -131,31 +126,24 @@ export class DefaultActionsRegistryService implements ActionsRegistryService { ); } - try { - const result = await action.action({ - input: input.data, - credentials, - logger: this.logger, - }); + const result = await action.action({ + input: input.data, + credentials, + logger: this.logger, + }); - const output = action.schema?.output - ? action.schema.output(z).safeParse(result?.output) - : ({ success: true, data: result?.output } as const); + const output = action.schema?.output + ? action.schema.output(z).safeParse(result?.output) + : ({ success: true, data: result?.output } as const); - if (!output.success) { - throw new InputError( - `Invalid output from action "${req.params.actionId}"`, - output.error, - ); - } - - res.json({ output: output.data }); - } catch (error) { - throw new ForwardedError( - `Failed execution of action "${req.params.actionId}"`, - error, + if (!output.success) { + throw new InputError( + `Invalid output from action "${req.params.actionId}"`, + output.error, ); } + + res.json({ output: output.data }); }, ); return router; diff --git a/packages/backend-defaults/src/alpha/entrypoints/actionsRegistry/actionsRegistryServiceFactory.test.ts b/packages/backend-defaults/src/alpha/entrypoints/actionsRegistry/actionsRegistryServiceFactory.test.ts index 79c103c284..1e7c27960e 100644 --- a/packages/backend-defaults/src/alpha/entrypoints/actionsRegistry/actionsRegistryServiceFactory.test.ts +++ b/packages/backend-defaults/src/alpha/entrypoints/actionsRegistry/actionsRegistryServiceFactory.test.ts @@ -22,7 +22,7 @@ import { import { httpRouterServiceFactory } from '../../../entrypoints/httpRouter'; import request from 'supertest'; import { actionsRegistryServiceFactory } from './actionsRegistryServiceFactory'; -import { InputError } from '@backstage/errors'; +import { InputError, NotFoundError } from '@backstage/errors'; import { actionsRegistryServiceRef } from '@backstage/backend-plugin-api/alpha'; describe('actionsRegistryServiceFactory', () => { @@ -510,7 +510,7 @@ describe('actionsRegistryServiceFactory', () => { expect(body).toMatchObject({ output: { ok: true } }); }); - it('should return the error from the action if it throws', async () => { + it('should forward the original error when the action throws a known error', async () => { const { server } = await startTestBackend({ features: [pluginSubject, ...defaultServices], }); @@ -528,9 +528,32 @@ describe('actionsRegistryServiceFactory', () => { expect(status).toBe(400); expect(body).toMatchObject({ error: { - message: expect.stringContaining( - 'Failed execution of action "my-plugin:test"', - ), + name: 'InputError', + message: 'test', + }, + }); + }); + + it('should forward a NotFoundError from the action with 404 status', async () => { + const { server } = await startTestBackend({ + features: [pluginSubject, ...defaultServices], + }); + + mockAction.mockRejectedValue(new NotFoundError('entity not found')); + + const { body, status } = await request(server) + .post( + '/api/my-plugin/.backstage/actions/v1/actions/my-plugin:test/invoke', + ) + .send({ + name: 'test', + }); + + expect(status).toBe(404); + expect(body).toMatchObject({ + error: { + name: 'NotFoundError', + message: 'entity not found', }, }); }); diff --git a/packages/backend-test-utils/src/alpha/services/MockActionsRegistry.ts b/packages/backend-test-utils/src/alpha/services/MockActionsRegistry.ts index 94e5987afd..384a5e8e95 100644 --- a/packages/backend-test-utils/src/alpha/services/MockActionsRegistry.ts +++ b/packages/backend-test-utils/src/alpha/services/MockActionsRegistry.ts @@ -17,7 +17,7 @@ import { BackstageCredentials, LoggerService, } from '@backstage/backend-plugin-api'; -import { ForwardedError, InputError, NotFoundError } from '@backstage/errors'; +import { InputError, NotFoundError } from '@backstage/errors'; import { JsonObject, JsonValue } from '@backstage/types'; import { z, AnyZodObject } from 'zod'; import zodToJsonSchema from 'zod-to-json-schema'; @@ -126,31 +126,24 @@ export class MockActionsRegistry throw new InputError(`Invalid input to action "${opts.id}"`, input.error); } - try { - const result = await action.action({ - input: input.data, - credentials: opts.credentials ?? mockCredentials.none(), - logger: this.logger, - }); + const result = await action.action({ + input: input.data, + credentials: opts.credentials ?? mockCredentials.none(), + logger: this.logger, + }); - const output = action.schema?.output - ? action.schema.output(z).safeParse(result?.output) - : ({ success: true, data: result?.output } as const); + const output = action.schema?.output + ? action.schema.output(z).safeParse(result?.output) + : ({ success: true, data: result?.output } as const); - if (!output.success) { - throw new InputError( - `Invalid output from action "${opts.id}"`, - output.error, - ); - } - - return { output: output.data }; - } catch (error) { - throw new ForwardedError( - `Failed execution of action "${opts.id}"`, - error, + if (!output.success) { + throw new InputError( + `Invalid output from action "${opts.id}"`, + output.error, ); } + + return { output: output.data }; } register< diff --git a/plugins/catalog-backend/src/actions/createRegisterCatalogEntitiesAction.test.ts b/plugins/catalog-backend/src/actions/createRegisterCatalogEntitiesAction.test.ts index 31a89a31f3..a6926a2fd9 100644 --- a/plugins/catalog-backend/src/actions/createRegisterCatalogEntitiesAction.test.ts +++ b/plugins/catalog-backend/src/actions/createRegisterCatalogEntitiesAction.test.ts @@ -16,7 +16,6 @@ import { createRegisterCatalogEntitiesAction } from './createRegisterCatalogEntitiesAction'; import { catalogServiceMock } from '@backstage/plugin-catalog-node/testUtils'; import { actionsRegistryServiceMock } from '@backstage/backend-test-utils/alpha'; -import { ForwardedError } from '@backstage/errors'; describe('createRegisterCatalogEntitiesAction', () => { it('should successfully register a catalog location with a valid URL', async () => { @@ -78,14 +77,13 @@ describe('createRegisterCatalogEntitiesAction', () => { ).rejects.toThrow('not-a-valid-url is an invalid URL'); }); - it('should throw a ForwardedError if catalog.addLocation throws an error', async () => { + it('should throw the original error if catalog.addLocation fails', async () => { const mockActionsRegistry = actionsRegistryServiceMock(); const mockCatalog = catalogServiceMock(); - const errorMessage = 'Failed to add location'; mockCatalog.addLocation = jest .fn() - .mockRejectedValue(new Error(errorMessage)); + .mockRejectedValue(new Error('Failed to add location')); createRegisterCatalogEntitiesAction({ catalog: mockCatalog, @@ -100,6 +98,9 @@ describe('createRegisterCatalogEntitiesAction', () => { 'https://github.com/example/repo/blob/main/catalog-info.yaml', }, }), - ).rejects.toThrow(ForwardedError); + ).rejects.toMatchObject({ + name: 'Error', + message: 'Failed to add location', + }); }); }); diff --git a/plugins/catalog-backend/src/actions/createUnregisterCatalogEntitiesAction.test.ts b/plugins/catalog-backend/src/actions/createUnregisterCatalogEntitiesAction.test.ts index 492a427630..bd886e37ba 100644 --- a/plugins/catalog-backend/src/actions/createUnregisterCatalogEntitiesAction.test.ts +++ b/plugins/catalog-backend/src/actions/createUnregisterCatalogEntitiesAction.test.ts @@ -192,10 +192,13 @@ describe('createUnregisterCatalogEntitiesAction', () => { ); }); - it('should throw NotFoundError if no location matches the URL', async () => { + it('should throw NotFoundError with the original message if no location matches the URL', async () => { const mockActionsRegistry = actionsRegistryServiceMock(); const mockCatalog = catalogServiceMock(); + const locationUrl = + 'https://github.com/backstage/demo/blob/master/catalog-info.yaml'; + mockCatalog.getLocations = jest.fn().mockResolvedValue({ items: [ { id: 'location-id-1', target: 'https://other-url.com/catalog.yaml' }, @@ -207,51 +210,24 @@ describe('createUnregisterCatalogEntitiesAction', () => { actionsRegistry: mockActionsRegistry, }); - await expect( - mockActionsRegistry.invoke({ - id: 'test:unregister-entity', - input: { - type: { - locationUrl: - 'https://github.com/backstage/demo/blob/master/catalog-info.yaml', - }, - }, - }), - ).rejects.toThrow(/NotFoundError/); - }); - - it('should throw NotFoundError with descriptive message when location not found', async () => { - const mockActionsRegistry = actionsRegistryServiceMock(); - const mockCatalog = catalogServiceMock(); - - const locationUrl = - 'https://github.com/backstage/demo/blob/master/catalog-info.yaml'; - - mockCatalog.getLocations = jest.fn().mockResolvedValue({ - items: [], - }); - - createUnregisterCatalogEntitiesAction({ - catalog: mockCatalog, - actionsRegistry: mockActionsRegistry, - }); - await expect( mockActionsRegistry.invoke({ id: 'test:unregister-entity', input: { type: { locationUrl } }, }), - ).rejects.toThrow(`Location with URL ${locationUrl} not found`); + ).rejects.toMatchObject({ + name: 'NotFoundError', + message: `Location with URL ${locationUrl} not found`, + }); }); - it('should throw an error if catalog.getLocations throws an error', async () => { + it('should throw the original error if catalog.getLocations fails', async () => { const mockActionsRegistry = actionsRegistryServiceMock(); const mockCatalog = catalogServiceMock(); - const errorMessage = 'Failed to get locations'; mockCatalog.getLocations = jest .fn() - .mockRejectedValue(new Error(errorMessage)); + .mockRejectedValue(new Error('Failed to get locations')); createUnregisterCatalogEntitiesAction({ catalog: mockCatalog, @@ -268,7 +244,10 @@ describe('createUnregisterCatalogEntitiesAction', () => { }, }, }), - ).rejects.toThrow(errorMessage); + ).rejects.toMatchObject({ + name: 'Error', + message: 'Failed to get locations', + }); }); }); }); diff --git a/plugins/mcp-actions-backend/src/services/McpService.test.ts b/plugins/mcp-actions-backend/src/services/McpService.test.ts index 038700c4a5..c75572bf20 100644 --- a/plugins/mcp-actions-backend/src/services/McpService.test.ts +++ b/plugins/mcp-actions-backend/src/services/McpService.test.ts @@ -26,6 +26,7 @@ import { CallToolResultSchema, ListToolsResultSchema, } from '@modelcontextprotocol/sdk/types.js'; +import { InputError, NotFoundError } from '@backstage/errors'; describe('McpService', () => { it('should list the available actions as tools in the mcp backend', async () => { @@ -343,4 +344,116 @@ describe('McpService', () => { }), ); }); + + it('should forward the original InputError when an action throws one', async () => { + const mockActionsRegistry = actionsRegistryServiceMock(); + mockActionsRegistry.register({ + name: 'failing-action', + title: 'Failing', + description: 'An action that throws InputError', + schema: { + input: z => z.object({ value: z.string() }), + output: z => z.object({}), + }, + action: async () => { + throw new InputError('the value was invalid'); + }, + }); + + const mcpService = await McpService.create({ + actions: mockActionsRegistry, + metrics: metricsServiceMock.mock(), + }); + + const server = mcpService.getServer({ + credentials: mockCredentials.user(), + }); + + const client = new Client({ + name: 'test client', + version: '1.0', + }); + + const [clientTransport, serverTransport] = + InMemoryTransport.createLinkedPair(); + + await Promise.all([ + client.connect(clientTransport), + server.connect(serverTransport), + ]); + + const result = await client.request( + { + method: 'tools/call', + params: { name: 'failing-action', arguments: { value: 'test' } }, + }, + CallToolResultSchema, + ); + + expect(result).toEqual({ + content: [ + { + type: 'text', + text: 'InputError: the value was invalid', + }, + ], + isError: true, + }); + }); + + it('should forward the original NotFoundError when an action throws one', async () => { + const mockActionsRegistry = actionsRegistryServiceMock(); + mockActionsRegistry.register({ + name: 'not-found-action', + title: 'Not Found', + description: 'An action that throws NotFoundError', + schema: { + input: z => z.object({ id: z.string() }), + output: z => z.object({}), + }, + action: async () => { + throw new NotFoundError('entity does not exist'); + }, + }); + + const mcpService = await McpService.create({ + actions: mockActionsRegistry, + metrics: metricsServiceMock.mock(), + }); + + const server = mcpService.getServer({ + credentials: mockCredentials.user(), + }); + + const client = new Client({ + name: 'test client', + version: '1.0', + }); + + const [clientTransport, serverTransport] = + InMemoryTransport.createLinkedPair(); + + await Promise.all([ + client.connect(clientTransport), + server.connect(serverTransport), + ]); + + const result = await client.request( + { + method: 'tools/call', + params: { name: 'not-found-action', arguments: { id: 'abc' } }, + }, + CallToolResultSchema, + ); + + expect(result).toEqual({ + content: [ + { + type: 'text', + text: 'NotFoundError: entity does not exist', + }, + ], + isError: true, + }); + }); }); diff --git a/plugins/mcp-actions-backend/src/services/handleErrors.test.ts b/plugins/mcp-actions-backend/src/services/handleErrors.test.ts new file mode 100644 index 0000000000..17fe6de0a6 --- /dev/null +++ b/plugins/mcp-actions-backend/src/services/handleErrors.test.ts @@ -0,0 +1,144 @@ +/* + * Copyright 2025 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { + InputError, + NotFoundError, + NotAllowedError, + ForwardedError, + ResponseError, +} from '@backstage/errors'; +import { handleErrors } from './handleErrors'; + +describe('handleErrors', () => { + it('should return the error description for a known error', async () => { + const result = await handleErrors(async () => { + throw new InputError('bad input'); + }); + + expect(result).toEqual({ + content: [{ type: 'text', text: 'InputError: bad input' }], + isError: true, + }); + }); + + it('should return the error description for a NotFoundError', async () => { + const result = await handleErrors(async () => { + throw new NotFoundError('entity not found'); + }); + + expect(result).toEqual({ + content: [{ type: 'text', text: 'NotFoundError: entity not found' }], + isError: true, + }); + }); + + it('should return the error description for a NotAllowedError', async () => { + const result = await handleErrors(async () => { + throw new NotAllowedError('forbidden'); + }); + + expect(result).toEqual({ + content: [{ type: 'text', text: 'NotAllowedError: forbidden' }], + isError: true, + }); + }); + + it('should rethrow an unknown error', async () => { + await expect( + handleErrors(async () => { + throw new Error('unknown problem'); + }), + ).rejects.toThrow('unknown problem'); + }); + + it('should handle a ForwardedError that inherits the cause name', async () => { + const result = await handleErrors(async () => { + throw new ForwardedError( + 'wrapper message', + new InputError('original error'), + ); + }); + + // ForwardedError inherits name from cause and CustomErrorBase + // concatenates the cause message into the full message + expect(result).toEqual({ + content: [ + { + type: 'text', + text: 'InputError: wrapper message; caused by InputError: original error', + }, + ], + isError: true, + }); + }); + + it('should extract the cause from a ResponseError', async () => { + const response = { + ok: false, + status: 400, + statusText: 'Bad Request', + headers: new Headers({ 'content-type': 'application/json' }), + text: async () => + JSON.stringify({ + error: { name: 'InputError', message: 'bad value' }, + response: { statusCode: 400 }, + }), + }; + + const responseError = await ResponseError.fromResponse(response as any); + + const result = await handleErrors(async () => { + throw responseError; + }); + + expect(result).toEqual({ + content: [{ type: 'text', text: 'InputError: bad value' }], + isError: true, + }); + }); + + it('should recursively extract through nested ResponseErrors', async () => { + const innerResponse = { + ok: false, + status: 400, + statusText: 'Bad Request', + headers: new Headers({ 'content-type': 'application/json' }), + text: async () => + JSON.stringify({ + error: { + name: 'ResponseError', + message: 'Request failed with 400 Bad Request', + cause: { name: 'InputError', message: 'deeply nested error' }, + }, + response: { statusCode: 400 }, + }), + }; + + const responseError = await ResponseError.fromResponse( + innerResponse as any, + ); + + const result = await handleErrors(async () => { + throw responseError; + }); + + expect(result).toEqual({ + content: [{ type: 'text', text: 'InputError: deeply nested error' }], + isError: true, + }); + }); +}); diff --git a/plugins/mcp-actions-backend/src/services/handleErrors.ts b/plugins/mcp-actions-backend/src/services/handleErrors.ts index 5266c80e75..97070bb723 100644 --- a/plugins/mcp-actions-backend/src/services/handleErrors.ts +++ b/plugins/mcp-actions-backend/src/services/handleErrors.ts @@ -35,14 +35,14 @@ const knownErrors = new Set([ 'ServiceUnavailableError', ]); -// Extracts the cause error, if the provided error is `ResponseError` or -// `ForwardedError` with a cause. +// Recursively extracts the innermost cause from ResponseError or +// ForwardedError wrappers to surface the original error. function extractCause(err: ErrorLike): ErrorLike { if ( (err.name === 'ResponseError' || err instanceof ForwardedError) && isError(err.cause) ) { - return err.cause; + return extractCause(err.cause); } return err; }