From cca9b6858f8d11e2a5e15cafb178bcd23b46d643 Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Wed, 14 Oct 2026 14:09:30 +0530 Subject: [PATCH 1/4] fix(testmanagement): return the upstream message from updateTestCase failures The fallback branch interpolated the CallToolResult returned by formatAxiosError into a string, so every non-404/403 failure reached the caller as "[object Object]" with a credentials hint. The response now carries the HTTP status and the Test Management message, with the credentials hint only on 401. errorText() serialises object bodies so the helper cannot produce the same output for any caller. Co-Authored-By: Claude Fable 5.1 --- src/lib/error.ts | 38 ++++++---- .../testmanagement-utils/update-testcase.ts | 9 ++- tests/tools/testmanagement.test.ts | 71 +++++++++++++++++++ 3 files changed, 101 insertions(+), 17 deletions(-) diff --git a/src/lib/error.ts b/src/lib/error.ts index e64f8631..e5b172a7 100644 --- a/src/lib/error.ts +++ b/src/lib/error.ts @@ -1,6 +1,29 @@ import { AxiosError } from "axios"; import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; +/** + * Returns the most specific human-readable text for an error raised by an + * API call. For an AxiosError with a response body it prefers the body's + * `message`, `error` or `errors` field, serialising it when the API sent an + * object instead of a string. + * @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 && err.response?.data !== undefined) { + const data = err.response.data; + const detail = + typeof data === "string" + ? data + : (data?.message ?? data?.error ?? data?.errors); + if (typeof detail === "string" && detail.trim()) return detail; + if (detail !== undefined && detail !== null) return JSON.stringify(detail); + return err.message || defaultText; + } + if (err instanceof Error) return err.message || defaultText; + return defaultText; +} + /** * Formats an AxiosError into a CallToolResult with an appropriate message. * @param err - The error object to format @@ -10,21 +33,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/tools/testmanagement.test.ts b/tests/tools/testmanagement.test.ts index 3277921a..60c0b200 100644 --- a/tests/tools/testmanagement.test.ts +++ b/tests/tools/testmanagement.test.ts @@ -1604,6 +1604,77 @@ 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('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; From eea590f96195cb831b6bf44363af59a97bfb7f11 Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Wed, 14 Oct 2026 14:48:26 +0530 Subject: [PATCH 2/4] fix(lib): surface the whole response body when it has no message field Co-Authored-By: Claude Fable 5.1 --- src/lib/error.ts | 5 ++++- tests/tools/testmanagement.test.ts | 13 +++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/lib/error.ts b/src/lib/error.ts index e5b172a7..1ce8a5bd 100644 --- a/src/lib/error.ts +++ b/src/lib/error.ts @@ -5,7 +5,8 @@ import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; * Returns the most specific human-readable text for an error raised by an * API call. For an AxiosError with a response body it prefers the body's * `message`, `error` or `errors` field, serialising it when the API sent an - * object instead of a string. + * object instead of a string, and falls back to the whole body (capped at 500 + * chars) when none of those fields exist. * @param err - The error object to describe * @param defaultText - The fallback error message */ @@ -18,6 +19,8 @@ export function errorText(err: unknown, defaultText: string): string { : (data?.message ?? data?.error ?? data?.errors); if (typeof detail === "string" && detail.trim()) return detail; if (detail !== undefined && detail !== null) return JSON.stringify(detail); + const body = JSON.stringify(data); + if (body && body !== "{}" && body !== '""') return body.slice(0, 500); return err.message || defaultText; } if (err instanceof Error) return err.message || defaultText; diff --git a/tests/tools/testmanagement.test.ts b/tests/tools/testmanagement.test.ts index 60c0b200..59e3cc43 100644 --- a/tests/tools/testmanagement.test.ts +++ b/tests/tools/testmanagement.test.ts @@ -1657,6 +1657,19 @@ describe('updateTestCase — upstream error messages', () => { 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')); From c4c95679d25e12590b8597113a37fc51ebae1293 Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Wed, 14 Oct 2026 14:54:08 +0530 Subject: [PATCH 3/4] fix(lib): trim, cap and fall through empty bodies in errorText An empty or whitespace-only body, or a whitespace-only message field, now falls back to the error's own message instead of surfacing a literal "". Surfaced text is capped at 500 characters. errorText has direct unit tests covering field precedence, object serialisation, unknown body shapes, empty bodies, the cap, and non-axios errors. Co-Authored-By: Claude Fable 5.1 --- src/lib/error.ts | 66 ++++++++++++++++++++------- tests/lib/error.test.ts | 98 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 16 deletions(-) create mode 100644 tests/lib/error.test.ts diff --git a/src/lib/error.ts b/src/lib/error.ts index 1ce8a5bd..064143c1 100644 --- a/src/lib/error.ts +++ b/src/lib/error.ts @@ -1,29 +1,63 @@ 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; +} + +function stringify(value: unknown): string { + if (typeof value === "string") return 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. For an AxiosError with a response body it prefers the body's - * `message`, `error` or `errors` field, serialising it when the API sent an - * object instead of a string, and falls back to the whole body (capped at 500 - * chars) when none of those fields exist. + * 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 + * or whitespace-only 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 && err.response?.data !== undefined) { - const data = err.response.data; - const detail = - typeof data === "string" - ? data - : (data?.message ?? data?.error ?? data?.errors); - if (typeof detail === "string" && detail.trim()) return detail; - if (detail !== undefined && detail !== null) return JSON.stringify(detail); - const body = JSON.stringify(data); - if (body && body !== "{}" && body !== '""') return body.slice(0, 500); - return err.message || defaultText; + if (err instanceof AxiosError) { + const text = bodyText(err.response?.data); + if (text) return text; + return clip(err.message || defaultText); } - if (err instanceof Error) return err.message || defaultText; + if (err instanceof Error) return clip(err.message || defaultText); return defaultText; } diff --git a/tests/lib/error.test.ts b/tests/lib/error.test.ts new file mode 100644 index 00000000..9046f5bf --- /dev/null +++ b/tests/lib/error.test.ts @@ -0,0 +1,98 @@ +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("caps the surfaced text at 500 characters", () => { + expect(errorText(axiosError(504, "".repeat(2000)), "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, + }); + }); +}); From 8cf9496b048c94e404f6556d70d849a27f27a57c Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Wed, 14 Oct 2026 15:12:38 +0530 Subject: [PATCH 4/4] fix(lib): report the status instead of an HTML error page body Co-Authored-By: Claude Fable 5.1 --- src/lib/error.ts | 10 +++++++--- tests/lib/error.test.ts | 13 ++++++++++++- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/lib/error.ts b/src/lib/error.ts index 064143c1..f08b2d77 100644 --- a/src/lib/error.ts +++ b/src/lib/error.ts @@ -8,8 +8,12 @@ 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 value.trim(); + if (typeof value === "string") { + return HTML_DOCUMENT.test(value) ? "" : value.trim(); + } if (value === undefined || value === null) return ""; try { const json = JSON.stringify(value); @@ -46,8 +50,8 @@ function bodyText(data: unknown): string | 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 - * or whitespace-only body falls back to the error's own message. + * 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 */ diff --git a/tests/lib/error.test.ts b/tests/lib/error.test.ts index 9046f5bf..2a586922 100644 --- a/tests/lib/error.test.ts +++ b/tests/lib/error.test.ts @@ -68,8 +68,19 @@ describe("errorText", () => { ); }); + 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, "".repeat(2000)), "d")).toHaveLength( + expect(errorText(axiosError(504, "x".repeat(10000)), "d")).toHaveLength( 500, ); const big = {