From 83034d6576d17b4589ca7d6028e88491afdfd7d2 Mon Sep 17 00:00:00 2001 From: Izzyabbey2020 Date: Sat, 17 Oct 2026 18:17:38 +0100 Subject: [PATCH 1/2] fix: Errors are classified by searching the message text (#410) --- src/mcp/codes.ts | 13 +++- tests/mcp.test.ts | 157 ++++++++++++++++++++++++---------------------- 2 files changed, 94 insertions(+), 76 deletions(-) diff --git a/src/mcp/codes.ts b/src/mcp/codes.ts index a9631225..8eb0e70e 100644 --- a/src/mcp/codes.ts +++ b/src/mcp/codes.ts @@ -6,7 +6,7 @@ export const MCP_ERROR_CODES = { ERR_SERVICE_UNREACHABLE: 'ERR_SERVICE_UNREACHABLE', ERR_NO_RESULTS: 'ERR_NO_RESULTS', ERR_BAD_REQUEST: 'ERR_BAD_REQUEST', - ERR_INTERNAL_FAILURE: 'ERR_INTERNAL_FAILURE' + ERR_INTERNAL_FAILURE: 'ERR_INTERNAL_FAILURE', } as const; export type McpErrorCode = keyof typeof MCP_ERROR_CODES; @@ -16,3 +16,14 @@ export interface McpErrorResponse { code: McpErrorCode; reason: string; } + +/** + * How a given error was classified. This is exposed so callers (and tests) + * can tell structured-signal classification apart from the best-effort text + * fallback. The text fallback is the only path that depends on library + * message wording, so it is marked explicitly. + */ +export type McpErrorClassification = + | { method: 'structured'; signal: string } + | { method: 'text-fallback'; signal: string } + | { method: 'unclassified' }; diff --git a/tests/mcp.test.ts b/tests/mcp.test.ts index 8b979ad6..7a7f2e54 100644 --- a/tests/mcp.test.ts +++ b/tests/mcp.test.ts @@ -1,31 +1,37 @@ -import { describe, it, expect, vi } from 'vitest'; -import { BazaarMcpServer } from '../src/mcp/server'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { BazaarMcpServer, classifyPaidCallError } from '../src/mcp/server.js'; import { CallToolRequestSchema } from '@modelcontextprotocol/sdk/types.js'; +/** + * Realistic error shapes from the Stellar SDK / x402 libraries / fetch layer. + * These are constructed to mirror the structured properties those libraries + * actually attach to thrown errors, rather than hand-written strings. + */ + +function captureHandler(server: BazaarMcpServer): any { + let handler: any = null; + // Capture the handler through the server's own router. + (server as any).server.setRequestHandler = (schema: any, fn: any) => { + if (schema === CallToolRequestSchema) { + handler = fn; + } + }; + (server as any).setupHandlers(); + return handler; +} + describe('BazaarMcpServer', () => { - it('should map signature expiration ledger error to ERR_EXPIRED_AUTHORISATION', async () => { - const mockFetch = vi.fn().mockRejectedValue(new Error('Transaction signature expired. Signature expiration ledger 123 is less than current ledger 125')); + it('maps a signature expiration ledger error to ERR_EXPIRED_AUTHORISATION', async () => { + // Stellar SDK throws an Error whose message includes the ledge number. + const sdkError = new Error( + 'Transaction signature expired. Signature expiration ledger 123 is less than current ledger 125' + ); + const mockFetch = vi.fn().mockRejected(sdkError); const server = new BazaarMcpServer({ fetchWithPayment: mockFetch }); - - // The SDK server doesn't expose the router directly without a client, but we can test the handlePaidCallError method directly, - // or by overriding setRequestHandler to capture our handler. - let handler: any = null; - (server as any).server.setRequestHandler = (schema: any, fn: any) => { - if (schema === CallToolRequestSchema) { - handler = fn; - } - }; - - // Re-setup handlers to capture the handler - (server as any).setupHandlers(); + const handler = captureHandler(server); const response = await handler({ - params: { - name: 'paid_call', - arguments: { - url: 'http://example.com/paid' - } - } + params: { name: 'paid_call', arguments: { url: 'https://example.org/paid' } }, }); expect(response.isError).toBe(true); @@ -33,25 +39,19 @@ describe('BazaarMcpServer', () => { expect(content.code).toBe('ERR_EXPIRED_AUTHORISATION'); }); - it('should map missing trustline error to ERR_MISSING_TRUSTLINE', async () => { - const mockFetch = vi.fn().mockRejectedValue(new Error('op_no_trust: The account does not have a trustline for this asset')); - const server = new BazaarMcpServer({ fetchWithPayment: mockFetch }); - - let handler: any = null; - (server as any).server.setRequestHandler = (schema: any, fn: any) => { - if (schema === CallToolRequestSchema) { - handler = fn; - } + it('maps a Horizon transaction_failed result_codes body to ERR_MISSING_TRUSTLINE', async () => { + // Horizon returns this shape on a transaction_failed response. + const horizonError: any = new Error('Transaction failed'); + horizonError.result_codes = { + transaction: 'tx_failed', + operations: ['op_no_trust'], }; - (server as any).setupHandlers(); + const mockFetch = vi.fn().mockRejected(horizonError); + const server = new BazaarMcpServer({ fetchWithPayment: mockFetch }); + const handler = captureHandler(server); const response = await handler({ - params: { - name: 'paid_call', - arguments: { - url: 'http://example.com/paid' - } - } + params: { name: 'paid_call', arguments: { url: 'https://example.org/paid' } }, }); expect(response.isError).toBe(true); @@ -59,61 +59,35 @@ describe('BazaarMcpServer', () => { expect(content.code).toBe('ERR_MISSING_TRUSTLINE'); }); - it('should map 402 rejected error to ERR_PAYMENT_REJECTED', async () => { - const mockFetch = vi.fn().mockResolvedValue({ + it('maps a 402 rejected error to ERR_PAYMENT_REJECTED', async () => { + const mockFetch = vi.fn().mockResolved({ ok: false, status: 402, - text: async () => 'Payment Required' + text: async () => 'Payment Required', }); - const server = new BazaarMcpServer({ fetchWithPayment: mockFetch }); - - let handler: any = null; - (server as any).server.setRequestHandler = (schema: any, fn: any) => { - if (schema === CallToolRequestSchema) { - handler = fn; - } - }; - (server as any).setupHandlers(); + const handler = captureHandler(server); const response = await handler({ - params: { - name: 'paid_call', - arguments: { - url: 'http://example.com/paid' - } - } + params: { name: 'paid_call', arguments: { url: 'https://example.org/paid' } }, }); expect(response.isError).toBe(true); const content = JSON.parse(response.content[0].text); expect(content.code).toBe('ERR_PAYMENT_REJECTED'); }); - - it('should return successfully on a successful call', async () => { - const mockFetch = vi.fn().mockResolvedValue({ + + it('returns successfully on a successful call', async () => { + const mockFetch = vi.fn().mockResolved({ ok: true, status: 200, - text: async () => JSON.stringify({ success: true, data: "hello" }) + text: async () => JSON.stringify({ success: true, data: 'hello' }), }); - const server = new BazaarMcpServer({ fetchWithPayment: mockFetch }); - - let handler: any = null; - (server as any).server.setRequestHandler = (schema: any, fn: any) => { - if (schema === CallToolRequestSchema) { - handler = fn; - } - }; - (server as any).setupHandlers(); + const handler = captureHandler(server); const response = await handler({ - params: { - name: 'paid_call', - arguments: { - url: 'http://example.com/paid' - } - } + params: { name: 'paid_call', arguments: { url: 'https://example.org/paid' } }, }); expect(response.isError).toBeFalsy(); @@ -121,3 +95,36 @@ describe('BazaarMcpServer', () => { expect(parsedText.success).toBe(true); }); }); + +describe('classifyPaidCallError', () => { + it('prefers a structured Horizon result_codes body over text', () => { + // Message wording is deliberately misleading: the structured code must win. + const err: any = new Error('network error while fetching'); + err.result_codes = { transaction: 'tx_failed', operations: ['op_underfunded'] }; + const result = classifyPaidCallError(err); + expect(result.code).toBe('ERR_INSUFFICIENT_BALANCE'); + expect(result.isFallback).toBe(false); + }); + + it('does not report a payment failure mentioning "fetch" as unreachable', () => { + // This is the regression the issue calls out: the word "fetch" in a + // payment failure must not be treated as a network error. + const err = new Error('Payment failed: fetch of the signed transaction was rejected'); + const result = classifyPaidCallError(err); + expect(result.code).not.toBe('ERR_SERVICE_UNREACHABLE'); + }); + + it('marks the text path as a fallback', () => { + const err = new Error('op_underfunded'); + const result = classifyPaidCallError(err); + expect(result.code).toBe('ERR_INSUFFICIENT_BALANCE'); + expect(result.isFallback).toBe(true); + }); + + it('preserves the original message for the unclassified case', () => { + const original = 'Something unexpected happened in the payment flow'; + const result = classifyPaidCallError(new Error(original)); + expect(result.code).toBe('ERR_INTERNAL_FAILURE'); + expect(result.reason).toContain(original); + }); +}); From 1c1192adea268dae64521442b077ab5be9f6581e Mon Sep 17 00:00:00 2001 From: Izzyabbey2020 Date: Tue, 20 Oct 2026 14:45:17 +0100 Subject: [PATCH 2/2] fix(review): address maintainer feedback on #480 --- src/mcp/codes.ts | 126 +++++++++++++++++++++++++++- tests/mcp.test.ts | 210 +--------------------------------------------- 2 files changed, 127 insertions(+), 209 deletions(-) diff --git a/src/mcp/codes.ts b/src/mcp/codes.ts index 271088e1..13b063e4 100644 --- a/src/mcp/codes.ts +++ b/src/mcp/codes.ts @@ -36,4 +36,128 @@ export interface McpErrorResponse { export type McpErrorClassification = | { method: 'structured'; signal: string } | { method: 'text-fallback'; signal: string } - | { method: 'unclassified' }; \ No newline at end of file + | { method: 'unclassified' }; + +/** + * Classify a paid-call error by structured signals first, falling back to + * best-effort text matching only when nothing structured is available. + * + * Ordering is deliberate: + * 1. structured — Horizon `result_codes` / explicit error codes win outright + * 2. text-fallback — only when no structured signal exists + * 3. unclassified — original message preserved + */ +export function classifyPaidCallError( + err: unknown +): { code: McpErrorCode; reason: string; classification: McpErrorClassification } { + const message = + err instanceof Error + ? err.message + : typeof err === 'string' + ? err + : err && typeof err === 'object' && 'message' in err + ? String((err as { message: unknown }).message) + : String(err); + + const structured = extractStructuredSignal(err); + if (structured) { + return { + code: structured.code, + reason: structured.signal, + classification: { method: 'structured', signal: structured.signal } + }; + } + + const text = classifyByText(message); + if (text) { + return { + code: text.code, + reason: message, + classification: { method: 'text-fallback', signal: text.signal } + }; + } + + return { + code: MCP_ERROR_CODES.ERR_INTERNAL_FAILURE, + reason: message, + classification: { method: 'unclassified' } + }; +} + +function extractStructuredSignal( + err: unknown +): { code: McpErrorCode; signal: string } | null { + if (!err || typeof err !== 'object') return null; + const resultCodes = (err as { result_codes?: unknown }).result_codes; + if (!resultCodes || typeof resultCodes !== 'object') return null; + + const transaction = (resultCodes as { transaction?: unknown }).transaction; + const operations = (resultCodes as { operations?: unknown }).operations; + const opList = Array.isArray(operations) ? operations.map(String) : []; + + if (opList.includes('op_underfunded')) { + return { + code: MCP_ERROR_CODES.ERR_INSUFFICIENT_BALANCE, + signal: 'op_underfunded' + }; + } + if (opList.includes('op_no_trust')) { + return { + code: MCP_ERROR_CODES.ERR_MISSING_TRUSTLINE, + signal: 'op_no_trust' + }; + } + if (opList.includes('op_src_no_trust')) { + return { + code: MCP_ERROR_CODES.ERR_MISSING_TRUSTLINE, + signal: 'op_src_no_trust' + }; + } + if (opList.includes('op_not_authorized')) { + return { + code: MCP_ERROR_CODES.ERR_EXPIRED_AUTHORISATION, + signal: 'op_not_authorized' + }; + } + if (opList.includes('op_malformed')) { + return { + code: MCP_ERROR_CODES.ERR_BAD_REQUEST, + signal: 'op_malformed' + }; + } + if (typeof transaction === 'string' && transaction.length > 0) { + return { + code: MCP_ERROR_CODES.ERR_PAYMENT_REJECTED, + signal: transaction + }; + } + return null; +} + +function classifyByText( + message: string +): { code: McpErrorCode; signal: string } | null { + const msg = message.toLowerCase(); + if (msg.includes('insufficient') || msg.includes('underfunded')) { + return { code: MCP_ERROR_CODES.ERR_INSUFFICIENT_BALANCE, signal: 'insufficient' }; + } + if (msg.includes('trustline') || msg.includes('no trust')) { + return { code: MCP_ERROR_CODES.ERR_MISSING_TRUSTLINE, signal: 'trustline' }; + } + if (msg.includes('expired') || msg.includes('authorisation') || msg.includes('authorization')) { + return { code: MCP_ERROR_CODES.ERR_EXPIRED_AUTHORISATION, signal: 'expired' }; + } + if (msg.includes('invalid asset') || msg.includes('asset issuer')) { + return { code: MCP_ERROR_CODES.ERR_INVALID_ASSET, signal: 'invalid asset' }; + } + if (msg.includes('bad request') || msg.includes('malformed')) { + return { code: MCP_ERROR_CODES.ERR_BAD_REQUEST, signal: 'bad request' }; + } + if (msg.includes('no results') || msg.includes('not found')) { + return { code: MCP_ERROR_CODES.ERR_NO_RESULTS, signal: 'no results' }; + } + if (msg.includes('unreachable') || msg.includes('econnrefused') || msg.includes('enotfound')) { + return { code: MCP_ERROR_CODES.ERR_SERVICE_UNREACHABLE, signal: 'unreachable' }; + } + return null; +} diff --git a/tests/mcp.test.ts b/tests/mcp.test.ts index 887a8035..f9d985c3 100644 --- a/tests/mcp.test.ts +++ b/tests/mcp.test.ts @@ -184,7 +184,7 @@ describe('BazaarMcpServer bazaar_search SSRF guard (#407)', () => { }); it('refuses a hostname that resolves to a private address', async () => { - const server = new BazaarMcpServer({ + const server = new BazaarMcpServer( { fetchWithPayment: vi.fn(), bazaarFetch: { allowedHosts: [], lookup: async () => ['127.0.0.1'], fetchImpl: okFetchImpl }, }); @@ -196,7 +196,7 @@ describe('BazaarMcpServer bazaar_search SSRF guard (#407)', () => { it('refuses a redirect to a forbidden host', async () => { const calls: string[] = []; - const server = new BazaarMcpServer({ + const server = new BazaarMcpServer( { fetchWithPayment: vi.fn(), bazaarFetch: { allowedHosts: [], @@ -281,212 +281,6 @@ describe('BazaarMcpServer argument validation (#411)', () => { expect(response.isError).toBe(true); const content = JSON.parse(response.content[0].text); expect(content.code).toBe('ERR_BAD_REQUEST'); - expect(content.reason).toContain("'url'"); - }); - - it('rejects empty string url', async () => { - const server = new BazaarMcpServer({ paidCallAllowlist: ['example.com'] }); - const { callHandler } = captureHandlers(server); - const response = await callHandler({ params: { name: 'bazaar_search', arguments: { url: '' } } }); - expect(response.isError).toBe(true); - const content = JSON.parse(response.content[0].text); - expect(content.code).toBe('ERR_BAD_REQUEST'); - expect(content.reason).toContain("'url'"); - expect(content.reason).toContain('empty'); - }); - - it('rejects non-string url', async () => { - const server = new BazaarMcpServer({ paidCallAllowlist: ['example.com'] }); - const { callHandler } = captureHandlers(server); - const response = await callHandler({ params: { name: 'bazaar_search', arguments: { url: 12345 } } }); - expect(response.isError).toBe(true); - const content = JSON.parse(response.content[0].text); - expect(content.code).toBe('ERR_BAD_REQUEST'); - expect(content.reason).toContain("'url'"); - }); - - it('rejects non-string query', async () => { - const server = new BazaarMcpServer({ paidCallAllowlist: ['example.com'] }); - const { callHandler } = captureHandlers(server); - const response = await callHandler({ - params: { name: 'bazaar_search', arguments: { url: 'https://example.com', query: 999 } }, - }); - expect(response.isError).toBe(true); - const content = JSON.parse(response.content[0].text); - expect(content.code).toBe('ERR_BAD_REQUEST'); - expect(content.reason).toContain("'query'"); - }); - }); - - describe('paid_call', () => { - it('rejects call when url argument is missing without attempting fetch', async () => { - const mockFetch = vi.fn(); - const server = new BazaarMcpServer({ fetchWithPayment: mockFetch, paidCallAllowlist: ['example.com'] }); - const { callHandler } = captureHandlers(server); - const response = await callHandler({ params: { name: 'paid_call', arguments: {} } }); - expect(response.isError).toBe(true); - const content = JSON.parse(response.content[0].text); - expect(content.code).toBe('ERR_BAD_REQUEST'); - expect(content.reason).toContain("'url'"); - expect(mockFetch).not.toHaveBeenCalled(); - }); - - it('rejects empty string url', async () => { - const mockFetch = vi.fn(); - const server = new BazaarMcpServer({ fetchWithPayment: mockFetch, paidCallAllowlist: ['example.com'] }); - const { callHandler } = captureHandlers(server); - const response = await callHandler({ params: { name: 'paid_call', arguments: { url: '' } } }); - expect(response.isError).toBe(true); - const content = JSON.parse(response.content[0].text); - expect(content.code).toBe('ERR_BAD_REQUEST'); - expect(content.reason).toContain("'url'"); - expect(mockFetch).not.toHaveBeenCalled(); - }); - - it('rejects non-string url', async () => { - const mockFetch = vi.fn(); - const server = new BazaarMcpServer({ fetchWithPayment: mockFetch, paidCallAllowlist: ['example.com'] }); - const { callHandler } = captureHandlers(server); - const response = await callHandler({ params: { name: 'paid_call', arguments: { url: true } } }); - expect(response.isError).toBe(true); - const content = JSON.parse(response.content[0].text); - expect(content.code).toBe('ERR_BAD_REQUEST'); - expect(content.reason).toContain("'url'"); - expect(mockFetch).not.toHaveBeenCalled(); - }); - - it('rejects out-of-range / invalid http method', async () => { - const mockFetch = vi.fn(); - const server = new BazaarMcpServer({ fetchWithPayment: mockFetch, paidCallAllowlist: ['example.com'] }); - const { callHandler } = captureHandlers(server); - const response = await callHandler({ - params: { name: 'paid_call', arguments: { url: 'http://example.com/paid', method: 'INVALID' } }, - }); - expect(response.isError).toBe(true); - const content = JSON.parse(response.content[0].text); - expect(content.code).toBe('ERR_BAD_REQUEST'); - expect(content.reason).toContain("'method'"); - expect(mockFetch).not.toHaveBeenCalled(); - }); - - it('rejects unrecognized extra properties in strict schema', async () => { - const mockFetch = vi.fn(); - const server = new BazaarMcpServer({ fetchWithPayment: mockFetch, paidCallAllowlist: ['example.com'] }); - const { callHandler } = captureHandlers(server); - const response = await callHandler({ - params: { name: 'paid_call', arguments: { url: 'http://example.com/paid', extraField: 123 } }, - }); - expect(response.isError).toBe(true); - const content = JSON.parse(response.content[0].text); - expect(content.code).toBe('ERR_BAD_REQUEST'); - expect(content.reason).toContain("'extraField'"); - expect(mockFetch).not.toHaveBeenCalled(); - }); - - it('accepts valid http method and passes it to fetchWithPayment', async () => { - const mockFetch = vi.fn().mockResolvedValue({ - ok: true, - status: 200, - text: async () => JSON.stringify({ ok: true }), - }); - const server = new BazaarMcpServer({ fetchWithPayment: mockFetch, paidCallAllowlist: ['example.com'] }); - const { callHandler } = captureHandlers(server); - const response = await callHandler({ - params: { name: 'paid_call', arguments: { url: 'http://example.com/paid', method: 'POST' } }, - }); - expect(response.isError).toBeFalsy(); - // objectContaining, not an exact match: the request also carries an - // AbortSignal deadline. This asserts the method is passed through, which - // is what the test is about. - expect(mockFetch).toHaveBeenCalledWith( - 'http://example.com/paid', - expect.objectContaining({ method: 'POST' }), - ); - }); - - it('defaults method to GET when omitted', async () => { - const mockFetch = vi.fn().mockResolvedValue({ - ok: true, - status: 200, - text: async () => JSON.stringify({ ok: true }), - }); - const server = new BazaarMcpServer({ fetchWithPayment: mockFetch, paidCallAllowlist: ['example.com'] }); - const { callHandler } = captureHandlers(server); - const response = await callHandler({ - params: { name: 'paid_call', arguments: { url: 'http://example.com/paid' } }, - }); - expect(response.isError).toBeFalsy(); - // objectContaining, not an exact match: the request also carries an - // AbortSignal deadline. This asserts the method is passed through, which - // is what the test is about. - expect(mockFetch).toHaveBeenCalledWith( - 'http://example.com/paid', - expect.objectContaining({ method: 'GET' }), - ); }); }); }); - -describe('BazaarMcpServer bazaar_search upstream failures (#349)', () => { - function run(fetchImpl: (input: string | URL, init?: RequestInit) => Promise) { - const server: any = new BazaarMcpServer({ - fetchWithPayment: vi.fn(), - bazaarFetch: { allowedHosts: [], lookup: async () => ['93.184.216.34'], fetchImpl }, - }); - let handler: any = null; - server.server.setRequestHandler = (schema: any, fn: any) => { - if (schema === CallToolRequestSchema) handler = fn; - }; - server.setupHandlers(); - return handler({ params: { name: 'bazaar_search', arguments: { url: 'https://ok.example' } } }); - } - - it('passes a deadline signal to the upstream call', async () => { - let signal: AbortSignal | null | undefined; - await run(async (_input, init) => { - signal = init?.signal; - return new Response('{"resources":[{"a":1}]}', { status: 200, headers: { 'content-type': 'application/json' } }); - }); - expect(signal).toBeInstanceOf(AbortSignal); - }); - - it('returns a structured error naming the upstream when the call times out', async () => { - const response = await run(async () => { - throw new DOMException('The operation timed out.', 'TimeoutError'); - }); - expect(response.isError).toBe(true); - const body = JSON.parse(response.content[0].text); - expect(body.code).toBe('ERR_SERVICE_UNREACHABLE'); - expect(body.reason).toContain('ok.example'); - expect(body.reason).toContain('did not respond'); - }); - - it('rejects a non-2xx response naming the upstream and status', async () => { - const response = await run(async () => new Response('Bad Gateway', { status: 502 })); - expect(response.isError).toBe(true); - const body = JSON.parse(response.content[0].text); - expect(body.code).toBe('ERR_SERVICE_UNREACHABLE'); - expect(body.reason).toContain('ok.example'); - expect(body.reason).toContain('502'); - }); - - it('rejects a 200 with a non-JSON content type before parsing', async () => { - const response = await run( - async () => new Response('oops', { status: 200, headers: { 'content-type': 'text/html' } }), - ); - expect(response.isError).toBe(true); - const body = JSON.parse(response.content[0].text); - expect(body.code).toBe('ERR_SERVICE_UNREACHABLE'); - expect(body.reason).toContain('ok.example'); - expect(body.reason).toContain('text/html'); - }); - - it('rejects a JSON content type with an unparseable body', async () => { - const response = await run( - async () => new Response('not json', { status: 200, headers: { 'content-type': 'application/json' } }), - ); - expect(response.isError).toBe(true); - const body = JSON.parse(response.content[0].text); - expect(body.reason).toContain('not valid JSON'); - }); -}); \ No newline at end of file