-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: normalize invalid MCP required flags in MCP schemas #6077
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1a2cc04
d57de25
6de21a0
21b571e
b3478d5
26e2002
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| from types import SimpleNamespace | ||
| from unittest.mock import MagicMock | ||
|
|
||
| from astrbot.core.agent.mcp_client import MCPTool, _normalize_mcp_input_schema | ||
|
|
||
|
|
||
| class TestNormalizeMcpInputSchema: | ||
| def test_lifts_property_level_required_booleans_to_parent_required_array(self): | ||
| schema = { | ||
| "type": "object", | ||
| "properties": { | ||
| "stock_code": {"type": "string", "required": True}, | ||
| "market": {"type": "string", "required": False}, | ||
| }, | ||
| } | ||
|
|
||
| normalized = _normalize_mcp_input_schema(schema) | ||
|
|
||
| assert normalized["required"] == ["stock_code"] | ||
| assert "required" not in normalized["properties"]["stock_code"] | ||
| assert "required" not in normalized["properties"]["market"] | ||
| assert schema["properties"]["stock_code"]["required"] is True | ||
|
|
||
| def test_preserves_existing_required_arrays_while_fixing_nested_objects(self): | ||
| schema = { | ||
| "type": "object", | ||
| "required": ["server"], | ||
| "properties": { | ||
| "server": { | ||
| "type": "object", | ||
| "required": ["transport"], | ||
| "properties": { | ||
| "transport": {"type": "string"}, | ||
| "stock_code": {"type": "string", "required": True}, | ||
| "market": {"type": "string", "required": False}, | ||
| }, | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| normalized = _normalize_mcp_input_schema(schema) | ||
|
|
||
| assert normalized["required"] == ["server"] | ||
| assert normalized["properties"]["server"]["required"] == [ | ||
| "transport", | ||
| "stock_code", | ||
| ] | ||
| assert ( | ||
| "required" | ||
| not in normalized["properties"]["server"]["properties"]["stock_code"] | ||
| ) | ||
| assert ( | ||
| "required" not in normalized["properties"]["server"]["properties"]["market"] | ||
| ) | ||
|
|
||
| def test_preserves_parent_required_flag_for_nested_object_properties(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (testing): Consider adding tests for non-boolean or non-dict The normalization guards ( |
||
| schema = { | ||
| "type": "object", | ||
| "properties": { | ||
| "server": { | ||
| "type": "object", | ||
| "required": True, | ||
| "properties": { | ||
| "transport": {"type": "string", "required": True}, | ||
| }, | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| normalized = _normalize_mcp_input_schema(schema) | ||
|
|
||
| assert normalized["required"] == ["server"] | ||
| assert normalized["properties"]["server"]["required"] == ["transport"] | ||
| assert ( | ||
| "required" | ||
| not in normalized["properties"]["server"]["properties"]["transport"] | ||
| ) | ||
|
|
||
| def test_ignores_non_boolean_required_values_and_non_dict_properties(self): | ||
| schema = { | ||
| "type": "object", | ||
| "properties": { | ||
| "server": "invalid-property-schema", | ||
| "market": {"type": "string", "required": "yes"}, | ||
| "stock_code": {"type": "string", "required": True}, | ||
| }, | ||
| } | ||
|
|
||
| normalized = _normalize_mcp_input_schema(schema) | ||
|
|
||
| assert normalized["required"] == ["stock_code"] | ||
| assert normalized["properties"]["server"] == "invalid-property-schema" | ||
| assert normalized["properties"]["market"]["required"] == "yes" | ||
| assert "required" not in normalized["properties"]["stock_code"] | ||
| assert schema["properties"]["server"] == "invalid-property-schema" | ||
| assert schema["properties"]["market"]["required"] == "yes" | ||
|
|
||
|
|
||
| class TestMCPToolSchemaNormalization: | ||
| def test_mcp_tool_accepts_property_level_required_booleans(self): | ||
| mcp_tool = SimpleNamespace( | ||
| name="quote_lookup", | ||
| description="Lookup a quote", | ||
| inputSchema={ | ||
| "type": "object", | ||
| "properties": { | ||
| "stock_code": {"type": "string", "required": True}, | ||
| "market": {"type": "string", "required": False}, | ||
| }, | ||
| }, | ||
| ) | ||
|
|
||
| tool = MCPTool(mcp_tool, MagicMock(), "gf-securities") | ||
|
|
||
| assert tool.parameters["required"] == ["stock_code"] | ||
| assert "required" not in tool.parameters["properties"]["stock_code"] | ||
| assert "required" not in tool.parameters["properties"]["market"] | ||
Uh oh!
There was an error while loading. Please reload this page.