fix: Display proper error messages and strip null params from tool calls (#12437)

This commit is contained in:
Cristhian Zanforlin Lousa
2026-04-07 11:17:23 -03:00
committed by GitHub
parent a2e2b44087
commit 6938fd1d17
15 changed files with 409 additions and 82 deletions

View File

@ -2557,3 +2557,128 @@ class TestSnakeToCamelConversion:
# Metadata fields
assert _snake_to_camel("_meta_data") == "_metaData"
assert _snake_to_camel("_created_at") == "_createdAt"
class TestStripNoneRecursive:
"""Tests for _strip_none_recursive.
Ensures null values are removed from nested dicts and arrays before
sending arguments to MCP servers.
Bug: antvis/mcp-server-chart returns "Expected string, received null"
because LLMs send explicit null for optional fields inside arrays of objects.
"""
def test_should_remove_none_from_flat_dict(self):
"""Top-level None values must be stripped."""
from lfx.base.mcp.util import _strip_none_recursive
# Arrange
data = {"name": "chart", "style": None, "title": "My Chart"}
# Act
result = _strip_none_recursive(data)
# Assert
assert result == {"name": "chart", "title": "My Chart"}
assert "style" not in result
def test_should_remove_none_from_nested_objects_in_arrays(self):
"""The exact bug scenario: data[N].group = null inside an array."""
from lfx.base.mcp.util import _strip_none_recursive
# Arrange — reproduces the exact payload from the bug report
data = {
"data": [
{"name": "Revenue", "value": 100, "group": None},
{"name": "Costs", "value": 50, "group": None},
{"name": "Profit", "value": 50, "group": None},
],
"style": None,
}
# Act
result = _strip_none_recursive(data)
# Assert — group and style must be absent
assert result == {
"data": [
{"name": "Revenue", "value": 100},
{"name": "Costs", "value": 50},
{"name": "Profit", "value": 50},
],
}
assert "style" not in result
for item in result["data"]:
assert "group" not in item
def test_should_handle_deeply_nested_none(self):
"""None values several levels deep must also be stripped."""
from lfx.base.mcp.util import _strip_none_recursive
# Arrange
data = {
"config": {
"axis": {"label": None, "color": "red"},
"legend": None,
}
}
# Act
result = _strip_none_recursive(data)
# Assert
assert result == {"config": {"axis": {"color": "red"}}}
def test_should_preserve_falsy_non_none_values(self):
"""Zero, empty string, False, empty list, empty dict must NOT be stripped."""
from lfx.base.mcp.util import _strip_none_recursive
# Arrange
data = {
"count": 0,
"label": "",
"enabled": False,
"items": [],
"meta": {},
}
# Act
result = _strip_none_recursive(data)
# Assert — all falsy-but-not-None values preserved
assert result == {
"count": 0,
"label": "",
"enabled": False,
"items": [],
"meta": {},
}
def test_should_return_primitives_unchanged(self):
"""Non-dict, non-list values pass through unchanged."""
from lfx.base.mcp.util import _strip_none_recursive
assert _strip_none_recursive("hello") == "hello"
assert _strip_none_recursive(42) == 42
assert _strip_none_recursive(True) is True # noqa: FBT003
def test_should_handle_empty_structures(self):
"""Empty dict and empty list return empty."""
from lfx.base.mcp.util import _strip_none_recursive
assert _strip_none_recursive({}) == {}
assert _strip_none_recursive([]) == []
def test_should_handle_list_of_primitives_with_none(self):
"""None items inside a plain list are NOT removed (only dict keys)."""
from lfx.base.mcp.util import _strip_none_recursive
# Arrange — list items that are None stay (we only strip dict keys)
data = [1, None, "hello", None]
# Act
result = _strip_none_recursive(data)
# Assert — None items in list preserved (stripping applies to dict keys only)
assert result == [1, None, "hello", None]

View File

@ -0,0 +1,150 @@
import { extractApiErrorMessage } from "../extract-api-error-message";
describe("extractApiErrorMessage", () => {
const FALLBACK = "Default error";
it("should_return_string_detail_when_detail_is_string", () => {
// Arrange — simple string detail (e.g. HTTPException)
const error = {
response: { data: { detail: "Server not found" } },
};
// Act
const result = extractApiErrorMessage(error, FALLBACK);
// Assert
expect(result).toBe("Server not found");
});
it("should_return_joined_msg_fields_when_detail_is_array_of_validation_errors", () => {
// Arrange — FastAPI RequestValidationError format (the exact bug scenario)
const error = {
response: {
data: {
detail: [
{
type: "value_error",
loc: ["body", "args"],
msg: "Value error, Argument '-y' is not allowed for security reasons",
input: ["-y", "@antv/mcp-server-chart"],
ctx: {
error: "Argument '-y' is not allowed for security reasons",
},
},
],
},
},
};
// Act
const result = extractApiErrorMessage(error, FALLBACK);
// Assert — must be the readable msg, NOT "[object Object]"
expect(result).toBe(
"Value error, Argument '-y' is not allowed for security reasons",
);
expect(result).not.toContain("[object Object]");
});
it("should_join_multiple_validation_errors_with_semicolon", () => {
// Arrange — multiple validation errors in one response
const error = {
response: {
data: {
detail: [
{ msg: "Field 'name' is required" },
{ msg: "Field 'command' is required" },
],
},
},
};
// Act
const result = extractApiErrorMessage(error, FALLBACK);
// Assert
expect(result).toBe(
"Field 'name' is required; Field 'command' is required",
);
});
it("should_stringify_array_items_without_msg_field", () => {
// Arrange — array items that don't have a msg field
const error = {
response: {
data: {
detail: [{ error: "something went wrong" }],
},
},
};
// Act
const result = extractApiErrorMessage(error, FALLBACK);
// Assert — should use String() coercion, not crash
expect(result).toBe("[object Object]");
});
it("should_extract_msg_from_single_object_detail", () => {
// Arrange — detail is a single object with msg
const error = {
response: { data: { detail: { msg: "Invalid configuration" } } },
};
// Act
const result = extractApiErrorMessage(error, FALLBACK);
// Assert
expect(result).toBe("Invalid configuration");
});
it("should_extract_message_from_single_object_detail", () => {
// Arrange — detail is a single object with message (not msg)
const error = {
response: {
data: { detail: { message: "Connection refused" } },
},
};
// Act
const result = extractApiErrorMessage(error, FALLBACK);
// Assert
expect(result).toBe("Connection refused");
});
it("should_json_stringify_object_without_msg_or_message", () => {
// Arrange — object detail without known keys
const error = {
response: { data: { detail: { code: 500, info: "crash" } } },
};
// Act
const result = extractApiErrorMessage(error, FALLBACK);
// Assert
expect(result).toBe('{"code":500,"info":"crash"}');
});
it("should_use_error_message_when_no_detail", () => {
// Arrange — no response.data.detail, just error.message
const error = { message: "Network Error" };
// Act
const result = extractApiErrorMessage(error, FALLBACK);
// Assert
expect(result).toBe("Network Error");
});
it("should_use_fallback_when_no_detail_and_no_message", () => {
// Arrange — completely empty error
const error = {};
// Act
const result = extractApiErrorMessage(error, FALLBACK);
// Assert
expect(result).toBe(FALLBACK);
});
});

View File

@ -0,0 +1,35 @@
/**
* Extracts a human-readable error message from an Axios error response.
*
* FastAPI validation errors return `detail` as an array of objects
* (e.g. [{type, loc, msg, input, ctx}]). Directly coercing these
* to a string produces "[object Object]". This helper normalises
* all known shapes of `detail` into a readable string.
*/
export function extractApiErrorMessage(
error: { response?: { data?: { detail?: unknown } }; message?: string },
fallback: string,
): string {
const detail = error.response?.data?.detail;
if (typeof detail === "string") {
return detail;
}
if (Array.isArray(detail)) {
return detail
.map((d: Record<string, unknown>) =>
typeof d.msg === "string" ? d.msg : String(d),
)
.join("; ");
}
if (detail && typeof detail === "object") {
const obj = detail as Record<string, unknown>;
if (typeof obj.msg === "string") return obj.msg;
if (typeof obj.message === "string") return obj.message;
return JSON.stringify(detail);
}
return error.message || fallback;
}

View File

@ -92,6 +92,20 @@ export const usePostUploadFileV2: useMutationFunctionType<
},
},
);
// Replace the optimistic "temp" entry with the actual server data so
// that file.path in the cache matches the path returned to callers
// (via handleUpload → internalSelectedFiles). Without this, there is
// a race window where the optimistic path (just the filename) differs
// from the server path (user_id/filename), causing checkboxes to
// appear unchecked until the background refetch completes.
queryClient.setQueryData(["useGetFilesV2"], (old: any) => {
if (!Array.isArray(old)) return [response.data];
return old.map((file: any) =>
file?.id === "temp" ? { ...response.data } : file,
);
});
return response.data;
} catch (e) {
queryClient.setQueryData(["useGetFilesV2"], (old: FileType[]) => {

View File

@ -3,6 +3,7 @@ import type { useMutationFunctionType } from "@/types/api";
import type { MCPServerType } from "@/types/mcp";
import { api } from "../../api";
import { getURL } from "../../helpers/constants";
import { extractApiErrorMessage } from "../../helpers/extract-api-error-message";
import { UseRequestProcessor } from "../../services/request-processor";
interface AddMCPServerResponse {
@ -44,13 +45,13 @@ export const useAddMCPServer: useMutationFunctionType<
);
return { message: res.data?.message || "MCP Server added successfully" };
} catch (error: any) {
// Transform the error to include a message that can be handled by the UI
const errorMessage =
error.response?.data?.detail ||
error.message ||
"Failed to install MCP";
throw new Error(errorMessage);
} catch (error: unknown) {
throw new Error(
extractApiErrorMessage(
error as Parameters<typeof extractApiErrorMessage>[0],
"Failed to install MCP",
),
);
}
}

View File

@ -3,6 +3,7 @@ import type { useMutationFunctionType } from "@/types/api";
import type { MCPServerType } from "@/types/mcp";
import { api } from "../../api";
import { getURL } from "../../helpers/constants";
import { extractApiErrorMessage } from "../../helpers/extract-api-error-message";
import { UseRequestProcessor } from "../../services/request-processor";
interface DeleteMCPServerResponse {
@ -31,13 +32,13 @@ export const useDeleteMCPServer: useMutationFunctionType<
return {
message: res.data?.message || "MCP Server deleted successfully",
};
} catch (error: any) {
// Transform the error to include a message that can be handled by the UI
const errorMessage =
error.response?.data?.detail ||
error.message ||
"Failed to delete MCP Server";
throw new Error(errorMessage);
} catch (error: unknown) {
throw new Error(
extractApiErrorMessage(
error as Parameters<typeof extractApiErrorMessage>[0],
"Failed to delete MCP Server",
),
);
}
}

View File

@ -2,6 +2,7 @@ import type { UseMutationResult } from "@tanstack/react-query";
import type { useMutationFunctionType } from "@/types/api";
import { api } from "../../api";
import { getURL } from "../../helpers/constants";
import { extractApiErrorMessage } from "../../helpers/extract-api-error-message";
import { UseRequestProcessor } from "../../services/request-processor";
interface PatchInstallMCPParams {
@ -36,13 +37,13 @@ export const usePatchInstallMCP: useMutationFunctionType<
);
return { message: res.data?.message || "MCP installed successfully" };
} catch (error: any) {
// Transform the error to include a message that can be handled by the UI
const errorMessage =
error.response?.data?.detail ||
error.message ||
"Failed to install MCP";
throw new Error(errorMessage);
} catch (error: unknown) {
throw new Error(
extractApiErrorMessage(
error as Parameters<typeof extractApiErrorMessage>[0],
"Failed to install MCP",
),
);
}
}

View File

@ -3,6 +3,7 @@ import type { useMutationFunctionType } from "@/types/api";
import type { MCPServerType } from "@/types/mcp";
import { api } from "../../api";
import { getURL } from "../../helpers/constants";
import { extractApiErrorMessage } from "../../helpers/extract-api-error-message";
import { UseRequestProcessor } from "../../services/request-processor";
import type { getMCPServersResponse } from "./use-get-mcp-servers";
@ -58,13 +59,13 @@ export const usePatchMCPServer: useMutationFunctionType<
return {
message: res.data?.message || "MCP Server patched successfully",
};
} catch (error: any) {
// Transform the error to include a message that can be handled by the UI
const errorMessage =
error.response?.data?.detail ||
error.message ||
"Failed to patch MCP Server";
throw new Error(errorMessage);
} catch (error: unknown) {
throw new Error(
extractApiErrorMessage(
error as Parameters<typeof extractApiErrorMessage>[0],
"Failed to patch MCP Server",
),
);
}
}

View File

@ -3,7 +3,6 @@ import { nanoid } from "nanoid";
import { useEffect, useState } from "react";
import { useLocation } from "react-router-dom";
import { ForwardedIconComponent } from "@/components/common/genericIconComponent";
import ShadTooltip from "@/components/common/shadTooltipComponent";
import InputListComponent from "@/components/core/parameterRenderComponent/components/inputListComponent";
import { Button } from "@/components/ui/button";
import { Input } from "@/components/ui/input";
@ -27,7 +26,6 @@ import IOKeyPairInputWithVariables from "@/modals/IOModal/components/IOFieldView
import type { MCPServerType } from "@/types/mcp";
import { extractMcpServersFromJson } from "@/utils/mcpUtils";
import { parseString } from "@/utils/stringManipulation";
import { cn } from "@/utils/utils";
const MCP_SETTINGS_PAGE = "/settings/mcp-servers";
@ -364,16 +362,9 @@ export default function AddMcpServerModal({
id="global-variable-modal-inputs"
>
{error && (
<ShadTooltip content={error}>
<div
className={cn(
"absolute right-4 top-4 truncate text-xs font-medium text-destructive",
type === "JSON" ? "w-3/5" : "w-4/5",
)}
>
{error}
</div>
</ShadTooltip>
<div className="mb-4 rounded-md bg-destructive/10 px-4 py-2 text-xs font-medium text-destructive">
{error}
</div>
)}
<TabsContent value="JSON" className="flex flex-col p-0 m-0">
<Label className="!text-mmd mb-2">Paste in JSON config</Label>

View File

@ -4,6 +4,7 @@ import { expect, test } from "../../fixtures";
import { awaitBootstrapTest } from "../../utils/await-bootstrap-test";
import { initialGPTsetup } from "../../utils/initialGPTsetup";
import { unselectNodes } from "../../utils/unselect-nodes";
import { uploadFile } from "../../utils/upload-file";
import { withEventDeliveryModes } from "../../utils/withEventDeliveryModes";
withEventDeliveryModes(
@ -27,18 +28,7 @@ withEventDeliveryModes(
.click();
await initialGPTsetup(page);
await page.getByTestId("input-file-component").last().click();
const fileChooserPromise = page.waitForEvent("filechooser");
await page.getByTestId("drag-files-component").last().click();
const fileChooser = await fileChooserPromise;
await fileChooser.setFiles(
path.join(__dirname, "../../assets/test_file.txt"),
);
await page.getByText("test_file.txt").last().isVisible();
await page.waitForTimeout(500);
await page.getByTestId("select-files-modal-button").click();
await uploadFile(page, "test_file.txt");
await page.waitForSelector('[data-testid="title-Chat Output"]', {
timeout: 3000,

View File

@ -4,7 +4,7 @@ import { expect, test } from "../../fixtures";
import { addLegacyComponents } from "../../utils/add-legacy-components";
import { adjustScreenView } from "../../utils/adjust-screen-view";
import { awaitBootstrapTest } from "../../utils/await-bootstrap-test";
import { ensureCheckboxChecked } from "../../utils/ensure-checkbox-checked";
import { ensureFileSelected } from "../../utils/ensure-checkbox-checked";
import { generateRandomFilename } from "../../utils/generate-filename";
import {
disableInspectPanel,
@ -95,8 +95,7 @@ test(
timeout: 5000,
});
const checkbox = page.getByTestId(`checkbox-${sourceFileName}`).last();
await ensureCheckboxChecked(checkbox);
await ensureFileSelected(page);
// Create DataTransfer object and file
const dataTransfer = await page.evaluateHandle((jsonFileName) => {
@ -130,16 +129,16 @@ test(
await expect(
page.getByTestId(`checkbox-${sourceFileName}`).last(),
).toHaveAttribute("data-state", "checked", { timeout: 1000 });
).toHaveAttribute("data-state", "checked", { timeout: 5000 });
// Test checkbox
await expect(
page.getByTestId(`checkbox-${sourceFileName}`).last(),
).toHaveAttribute("data-state", "checked");
).toHaveAttribute("data-state", "checked", { timeout: 5000 });
await expect(
page.getByTestId(`checkbox-${jsonFileName}`).last(),
).toHaveAttribute("data-state", "checked");
).toHaveAttribute("data-state", "checked", { timeout: 5000 });
await page.getByTestId(`checkbox-${sourceFileName}`).last().click();
await page.getByTestId(`checkbox-${jsonFileName}`).last().click();
@ -240,10 +239,10 @@ test(
await expect(
page.getByTestId(`checkbox-${renamedTxtFile}`).last(),
).toHaveAttribute("data-state", "checked");
).toHaveAttribute("data-state", "checked", { timeout: 5000 });
await expect(
page.getByTestId(`checkbox-${renamedJsonFile}`).last(),
).toHaveAttribute("data-state", "checked");
).toHaveAttribute("data-state", "checked", { timeout: 5000 });
await page.getByTestId("select-files-modal-button").click();

View File

@ -1,21 +1,19 @@
import type { Locator } from "@playwright/test";
import type { Page } from "@playwright/test";
import { expect } from "../fixtures";
/**
* Ensures a checkbox reaches the "checked" state.
* On Windows CI, auto-select after upload may not trigger due to a race
* condition between focus/change events. This helper clicks the checkbox
* manually if it's not auto-checked within the initial timeout.
* Ensures a file was selected in the file management modal after upload.
*
* On Windows CI, the upload response path may not exactly match the file.path
* from the list query (due to a query refetch race), which means the checkbox
* visual state may stay "unchecked" even though internalSelectedFiles already
* contains the correct path from handleUpload. Since clicking "Select files"
* submits internalSelectedFiles (not the checkbox state), we only need to
* confirm that at least one file is in the selection — visible via the
* "N selected" counter.
*/
export async function ensureCheckboxChecked(checkbox: Locator, timeout = 5000) {
try {
await expect(checkbox).toHaveAttribute("data-state", "checked", {
timeout,
});
} catch {
await checkbox.click();
await expect(checkbox).toHaveAttribute("data-state", "checked", {
timeout: 3000,
});
}
export async function ensureFileSelected(page: Page) {
await expect(page.getByText("selected")).toBeVisible({
timeout: 5000,
});
}

View File

@ -2,7 +2,7 @@ import type { Page } from "@playwright/test";
import fs from "fs";
import path from "path";
import { expect } from "../fixtures";
import { ensureCheckboxChecked } from "./ensure-checkbox-checked";
import { ensureFileSelected } from "./ensure-checkbox-checked";
import { generateRandomFilename } from "./generate-filename";
import { unselectNodes } from "./unselect-nodes";
@ -92,8 +92,7 @@ export async function uploadFile(page: Page, fileName: string) {
timeout: 30000,
});
const checkbox = page.getByTestId(`checkbox-${sourceFileName}`).last();
await ensureCheckboxChecked(checkbox);
await ensureFileSelected(page);
await page.getByTestId("select-files-modal-button").click();

View File

@ -471,6 +471,21 @@ def _handle_tool_validation_error(
raise ValueError(msg) from e
def _strip_none_recursive(obj: Any) -> Any:
"""Recursively remove None values from dicts (including inside lists).
``model_dump(exclude_none=True)`` handles top-level and nested-model
None fields, but when LLMs explicitly send ``null`` for fields inside
arrays of objects the serialised dict may still contain ``None``.
This helper guarantees a clean payload before it reaches the MCP server.
"""
if isinstance(obj, dict):
return {k: _strip_none_recursive(v) for k, v in obj.items() if v is not None}
if isinstance(obj, list):
return [_strip_none_recursive(item) for item in obj]
return obj
def create_tool_coroutine(tool_name: str, arg_schema: type[BaseModel], client) -> Callable[..., Awaitable]:
async def tool_coroutine(*args, **kwargs):
# Get field names from the model (preserving order)
@ -494,7 +509,8 @@ def create_tool_coroutine(tool_name: str, arg_schema: type[BaseModel], client) -
_handle_tool_validation_error(e, tool_name, original_args, arg_schema)
try:
return await client.run_tool(tool_name, arguments=validated.model_dump(exclude_none=True))
arguments = _strip_none_recursive(validated.model_dump(exclude_none=True))
return await client.run_tool(tool_name, arguments=arguments)
except Exception as e:
await logger.aerror(f"Tool '{tool_name}' execution failed: {e}")
# Re-raise with more context
@ -523,7 +539,8 @@ def create_tool_func(tool_name: str, arg_schema: type[BaseModel], client) -> Cal
_handle_tool_validation_error(e, tool_name, original_args, arg_schema)
try:
return run_until_complete(client.run_tool(tool_name, arguments=validated.model_dump(exclude_none=True)))
arguments = _strip_none_recursive(validated.model_dump(exclude_none=True))
return run_until_complete(client.run_tool(tool_name, arguments=arguments))
except Exception as e:
logger.error(f"Tool '{tool_name}' execution failed: {e}")
# Re-raise with more context

View File

@ -1,7 +1,6 @@
import json
from json import JSONDecodeError
import jq
from json_repair import repair_json
from lfx.custom.custom_component.component import Component
@ -86,6 +85,12 @@ class ParseJSONDataComponent(Component):
logger.info("to_filter: %s", to_filter)
try:
import jq
except ImportError:
msg = "jq is required for Parse JSON. Install with: pip install jq"
raise ImportError(msg) from None
results = jq.compile(self.query).input_text(full_filter_str).all()
logger.info("results: %s", results)
return [Data(data=value) if isinstance(value, dict) else Data(text=str(value)) for value in results]