diff --git a/src/lib/error.ts b/src/lib/error.ts index e64f8631..f08b2d77 100644 --- a/src/lib/error.ts +++ b/src/lib/error.ts @@ -1,6 +1,70 @@ import { AxiosError } from "axios"; import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; +const MAX_ERROR_TEXT = 500; +const BODY_FIELDS = ["message", "error", "errors"] as const; + +function clip(text: string): string { + return text.length > MAX_ERROR_TEXT ? text.slice(0, MAX_ERROR_TEXT) : text; +} + +const HTML_DOCUMENT = /^\s*<(!doctype|html)\b/i; + +function stringify(value: unknown): string { + if (typeof value === "string") { + return HTML_DOCUMENT.test(value) ? "" : value.trim(); + } + if (value === undefined || value === null) return ""; + try { + const json = JSON.stringify(value); + return json === "{}" || json === "[]" ? "" : (json ?? ""); + } catch { + return ""; + } +} + +/** + * Text taken from an API error body: the first non-empty `message`, `error` + * or `errors` field when any of them is present, otherwise the whole body. + * Returns undefined when the body carries no usable text. + */ +function bodyText(data: unknown): string | undefined { + if (typeof data !== "object" || data === null) { + const text = stringify(data); + return text ? clip(text) : undefined; + } + const record = data as Record; + const named = BODY_FIELDS.filter((key) => key in record); + if (named.length > 0) { + for (const key of named) { + const text = stringify(record[key]); + if (text) return clip(text); + } + return undefined; + } + const text = stringify(record); + return text ? clip(text) : undefined; +} + +/** + * Returns the most specific human-readable text for an error raised by an + * API call, capped at 500 characters. For an AxiosError with a response body + * it prefers the body's `message`, `error` or `errors` field, serialising + * objects, and uses the whole body when none of those fields exist. An empty, + * whitespace-only or HTML-document body falls back to the error's own message. + * @param err - The error object to describe + * @param defaultText - The fallback error message + */ +export function errorText(err: unknown, defaultText: string): string { + if (err instanceof AxiosError) { + const text = bodyText(err.response?.data); + if (text) return text; + return clip(err.message || defaultText); + } + if (err instanceof Error) return clip(err.message || defaultText); + return defaultText; +} + /** * Formats an AxiosError into a CallToolResult with an appropriate message. * @param err - The error object to format @@ -10,21 +74,8 @@ export function formatAxiosError( err: unknown, defaultText: string, ): CallToolResult { - let text = defaultText; - - if (err instanceof AxiosError && err.response?.data) { - const message = - err.response.data.message || - err.response.data.error || - err.message || - defaultText; - text = message; - } else if (err instanceof Error) { - text = err.message; - } - return { - content: [{ type: "text", text }], + content: [{ type: "text", text: errorText(err, defaultText) }], isError: true, }; } diff --git a/src/tools/testmanagement-utils/update-testcase.ts b/src/tools/testmanagement-utils/update-testcase.ts index 0b7a1cbf..de5fe339 100644 --- a/src/tools/testmanagement-utils/update-testcase.ts +++ b/src/tools/testmanagement-utils/update-testcase.ts @@ -1,7 +1,7 @@ import { apiClient } from "../../lib/apiClient.js"; import { z } from "zod"; import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; -import { formatAxiosError } from "../../lib/error.js"; +import { errorText } from "../../lib/error.js"; import { fetchFormFields, normalizeDefaultFields as normalizeDefaultFieldsFromForm, @@ -306,12 +306,15 @@ Error details: ${JSON.stringify(err.response?.data || err.message)}`, }; } - const errorMessage = formatAxiosError(err, "Failed to update test case"); + const status = err.response?.status; + const detail = errorText(err, "unknown error"); + const hint = + status === 401 ? " Check the BrowserStack credentials in use." : ""; return { content: [ { type: "text", - text: `Failed to update test case: ${errorMessage}. Please verify your credentials and try again.`, + text: `Failed to update test case${status ? ` (HTTP ${status})` : ""}: ${detail}.${hint}`, }, ], isError: true, diff --git a/tests/lib/error.test.ts b/tests/lib/error.test.ts new file mode 100644 index 00000000..2a586922 --- /dev/null +++ b/tests/lib/error.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, it } from "vitest"; +import { AxiosError } from "axios"; +import { errorText, formatAxiosError } from "../../src/lib/error"; + +const axiosError = ( + status: number, + data: unknown, + message = `Request failed with status code ${status}`, +) => + Object.assign(Object.create(AxiosError.prototype), { + message, + response: { status, data }, + }); + +describe("errorText", () => { + it("prefers message, then error, then errors", () => { + expect(errorText(axiosError(422, { message: "m", error: "e" }), "d")).toBe( + "m", + ); + expect(errorText(axiosError(422, { error: "e", errors: ["x"] }), "d")).toBe( + "e", + ); + expect(errorText(axiosError(422, { errors: ["x"] }), "d")).toBe('["x"]'); + }); + + it("serialises object fields instead of printing [object Object]", () => { + expect(errorText(axiosError(400, { message: { nested: 1 } }), "d")).toBe( + '{"nested":1}', + ); + expect( + errorText(axiosError(400, { errors: { name: ["is too long"] } }), "d"), + ).toBe('{"name":["is too long"]}'); + }); + + it("uses a plain string body", () => { + expect(errorText(axiosError(401, "Unauthorized"), "d")).toBe( + "Unauthorized", + ); + }); + + it("uses the whole body when none of the known fields exist", () => { + expect( + errorText(axiosError(400, { detail: { hint: "try low" } }), "d"), + ).toBe('{"detail":{"hint":"try low"}}'); + }); + + it("falls back to the error message on an empty or whitespace body", () => { + expect(errorText(axiosError(500, ""), "d")).toBe( + "Request failed with status code 500", + ); + expect(errorText(axiosError(500, " \n"), "d")).toBe( + "Request failed with status code 500", + ); + expect(errorText(axiosError(500, {}), "d")).toBe( + "Request failed with status code 500", + ); + expect(errorText(axiosError(500, { message: " " }), "d")).toBe( + "Request failed with status code 500", + ); + expect(errorText(axiosError(500, undefined), "d")).toBe( + "Request failed with status code 500", + ); + }); + + it("falls back to the default text when the error has no message either", () => { + expect(errorText(axiosError(502, "", ""), "Failed to list")).toBe( + "Failed to list", + ); + }); + + it("ignores an HTML error page and reports the status instead", () => { + const page = + "504 Gateway Time-outnginx"; + expect(errorText(axiosError(504, page), "d")).toBe( + "Request failed with status code 504", + ); + expect( + errorText(axiosError(502, ""), "d"), + ).toBe("Request failed with status code 502"); + }); + + it("caps the surfaced text at 500 characters", () => { + expect(errorText(axiosError(504, "x".repeat(10000)), "d")).toHaveLength( + 500, + ); + const big = { + errors: Array.from({ length: 500 }, (_, i) => `field ${i} is invalid`), + }; + expect(errorText(axiosError(422, big), "d")).toHaveLength(500); + }); + + it("handles non-axios errors and non-errors", () => { + expect(errorText(new Error("read ECONNRESET"), "d")).toBe( + "read ECONNRESET", + ); + expect(errorText(new Error(""), "d")).toBe("d"); + expect(errorText("boom", "d")).toBe("d"); + expect(errorText(undefined, "d")).toBe("d"); + }); +}); + +describe("formatAxiosError", () => { + it("wraps errorText in an error result", () => { + expect(formatAxiosError(axiosError(422, { error: "bad" }), "d")).toEqual({ + content: [{ type: "text", text: "bad" }], + isError: true, + }); + }); +}); diff --git a/tests/tools/testmanagement.test.ts b/tests/tools/testmanagement.test.ts index 3277921a..59e3cc43 100644 --- a/tests/tools/testmanagement.test.ts +++ b/tests/tools/testmanagement.test.ts @@ -1604,6 +1604,90 @@ describe('updateTestCase — multi-select custom_fields', () => { }); }); +// Error path of updateTestCase: the upstream message must reach the caller as text. +describe('updateTestCase — upstream error messages', () => { + let updateTestCaseReal: typeof import('../../src/tools/testmanagement-utils/update-testcase').updateTestCase; + let apiClientMock: typeof import('../../src/lib/apiClient').apiClient; + let AxiosErrorCtor: typeof import('axios').AxiosError; + + const axiosError = (status: number, data: unknown) => + Object.assign(Object.create(AxiosErrorCtor.prototype), { + message: `Request failed with status code ${status}`, + response: { status, data }, + }); + + beforeAll(async () => { + const actual = await vi.importActual< + typeof import('../../src/tools/testmanagement-utils/update-testcase') + >('../../src/tools/testmanagement-utils/update-testcase'); + updateTestCaseReal = actual.updateTestCase; + apiClientMock = (await import('../../src/lib/apiClient')).apiClient; + AxiosErrorCtor = (await import('axios')).AxiosError; + }); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + const args = { project_identifier: 'PR-1', test_case_identifier: 'TC-1', description: 'd' }; + + it('surfaces a string error from the response body with the HTTP status', async () => { + (apiClientMock.patch as Mock).mockRejectedValueOnce( + axiosError(422, { success: false, error: 'Priority is not valid for this project' }), + ); + + const result = await updateTestCaseReal(args, mockConfig as any); + + expect(result.isError).toBe(true); + const text = (result.content[0] as any).text as string; + expect(text).toBe('Failed to update test case (HTTP 422): Priority is not valid for this project.'); + expect(text).not.toContain('[object Object]'); + }); + + it('serialises an object error body instead of printing [object Object]', async () => { + (apiClientMock.patch as Mock).mockRejectedValueOnce( + axiosError(400, { errors: { name: ['is too long'] } }), + ); + + const result = await updateTestCaseReal(args, mockConfig as any); + + const text = (result.content[0] as any).text as string; + expect(text).toContain('(HTTP 400)'); + expect(text).toContain('{"name":["is too long"]}'); + expect(text).not.toContain('[object Object]'); + }); + + it('returns the whole body when it has no message, error or errors field', async () => { + (apiClientMock.patch as Mock).mockRejectedValueOnce( + axiosError(400, { detail: { hint: 'priority must be one of: low, medium, high' } }), + ); + + const result = await updateTestCaseReal(args, mockConfig as any); + + const text = (result.content[0] as any).text as string; + expect(text).toBe( + 'Failed to update test case (HTTP 400): {"detail":{"hint":"priority must be one of: low, medium, high"}}.', + ); + }); + + it('adds the credentials hint only on 401', async () => { + (apiClientMock.patch as Mock).mockRejectedValueOnce(axiosError(401, 'Unauthorized')); + + const result = await updateTestCaseReal(args, mockConfig as any); + + const text = (result.content[0] as any).text as string; + expect(text).toBe('Failed to update test case (HTTP 401): Unauthorized. Check the BrowserStack credentials in use.'); + }); + + it('falls back to the error message when there is no response', async () => { + (apiClientMock.patch as Mock).mockRejectedValueOnce(new Error('read ECONNRESET')); + + const result = await updateTestCaseReal(args, mockConfig as any); + + expect((result.content[0] as any).text).toBe('Failed to update test case: read ECONNRESET.'); + }); +}); + // listTestCaseTemplates resolves a template name to a template_id. describe('listTemplates', () => { let listTemplatesReal: typeof import('../../src/tools/testmanagement-utils/list-templates').listTemplates;