From 23af232837cd82e0431c26ea548cc12608046c4e Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Thu, 10 Sep 2026 18:29:22 -0700 Subject: [PATCH] fix(tools): reject malformed tool parameter schemas at registration Port from earendil-works/pi#9300 fix (acaa253cc): a plugin registering a tool whose schema["parameters"] is not a dict (a list, string, etc.) previously registered fine and the malformed schema was serialized into every provider request, 400-ing turns far from the offending plugin. Live probe on main confirmed the bad schema flows into _fn_def() and the OpenAI wire unchanged. Fail at registry.register() with the tool name in the error instead. The plugin loader already catches registration exceptions and marks the plugin errored, so a broken plugin degrades gracefully rather than breaking every session. Schemas that omit "parameters" stay valid (no-argument tools); MCP tools are unaffected (their schemas pass through _normalize_mcp_input_schema first, which always returns a dict). --- tests/tools/test_registry.py | 19 +++++++++++++++++++ tools/registry.py | 11 +++++++++++ 2 files changed, 30 insertions(+) diff --git a/tests/tools/test_registry.py b/tests/tools/test_registry.py index 6573941fda..8b964dd949 100644 --- a/tests/tools/test_registry.py +++ b/tests/tools/test_registry.py @@ -6,6 +6,8 @@ import threading from pathlib import Path from unittest.mock import patch +import pytest + from tools.registry import ( ToolRegistry, _MAX_LOGGED_ERROR_CHARS, @@ -40,6 +42,23 @@ class TestRegisterAndDispatch: result = json.loads(reg.dispatch("alpha", {})) assert result == {"ok": True} + def test_register_rejects_non_dict_parameters(self): + """A list/str ``parameters`` fails at registration, not in a provider request (pi acaa253cc).""" + reg = ToolRegistry() + bad = {"name": "bad", "description": "x", "parameters": ["not", "an", "object"]} + with pytest.raises(ValueError, match="parameters"): + reg.register(name="bad", toolset="core", schema=bad, handler=_dummy_handler) + assert reg.get_entry("bad") is None + + def test_register_rejects_non_dict_schema(self): + reg = ToolRegistry() + with pytest.raises(ValueError, match="schema must be a dict"): + reg.register(name="bad2", toolset="core", schema=None, handler=_dummy_handler) + # Omitted parameters stays allowed (some tools take no arguments). + reg.register(name="noargs", toolset="core", + schema={"name": "noargs", "description": "x"}, handler=_dummy_handler) + assert reg.get_entry("noargs") is not None + def test_cross_mcp_toolsets_do_not_overwrite_atomically(self, caplog): """Parallel MCP registrations with one name leave exactly one owner.""" diff --git a/tools/registry.py b/tools/registry.py index b82ae4aeea..8544c76af5 100644 --- a/tools/registry.py +++ b/tools/registry.py @@ -602,6 +602,17 @@ class ToolRegistry: """Register a tool (called at import time by each tool file). ``override=True`` is an explicit opt-in for plugins replacing a built-in implementation (e.g. a headed-Chrome browser backend); without it, cross-toolset shadowing is rejected.""" + # Reject malformed schemas at registration, not at request time: a non-dict + # ``parameters`` (e.g. a list) serializes into every provider request and 400s the + # whole turn far from the offending plugin. Failing here names the culprit instead. + if not isinstance(schema, dict): + raise ValueError( + f"Tool {name!r}: schema must be a dict, got {type(schema).__name__}") + params = schema.get("parameters") + if params is not None and not isinstance(params, dict): + raise ValueError( + f"Tool {name!r}: schema['parameters'] must be an object (JSON Schema dict), " + f"got {type(params).__name__}") handler_owner = self._plugin_owner_of(handler) caller_owner = self._plugin_namespace_of_module(self._caller_module()) owner = caller_owner or handler_owner