From 8c2fea26afb0ec742ec85984ca235ebb06d68a2f Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 29 Apr 2025 16:42:27 +0530 Subject: [PATCH 1/9] fix: Add the no parameter check back again. We will remove this once we actually implement the `strict` flag and centralize this functionality by moving this check to the tool's constructor in a future PR. --- packages/toolbox-core/src/toolbox_core/tool.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/toolbox-core/src/toolbox_core/tool.py b/packages/toolbox-core/src/toolbox_core/tool.py index 8029d396..c2654784 100644 --- a/packages/toolbox-core/src/toolbox_core/tool.py +++ b/packages/toolbox-core/src/toolbox_core/tool.py @@ -316,6 +316,8 @@ def bind_parameters( """ param_names = set(p.name for p in self.__params) for name in bound_params.keys(): + if name not in param_names: + raise Exception(f"unable to bind parameters: no parameter named {name}") if name in self.__bound_parameters: raise ValueError( f"cannot re-bind parameter: parameter '{name}' is already bound" From 433eb906a86c95bd0ae9796c5048533a0770eefd Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 29 Apr 2025 17:11:25 +0530 Subject: [PATCH 2/9] fix: Reverse the error conditions to avoid masking of the second error. --- packages/toolbox-core/src/toolbox_core/tool.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/tool.py b/packages/toolbox-core/src/toolbox_core/tool.py index c2654784..8029d396 100644 --- a/packages/toolbox-core/src/toolbox_core/tool.py +++ b/packages/toolbox-core/src/toolbox_core/tool.py @@ -316,8 +316,6 @@ def bind_parameters( """ param_names = set(p.name for p in self.__params) for name in bound_params.keys(): - if name not in param_names: - raise Exception(f"unable to bind parameters: no parameter named {name}") if name in self.__bound_parameters: raise ValueError( f"cannot re-bind parameter: parameter '{name}' is already bound" From 7ef1a7a24994ec0c3816fbb77df1b10b4e35a59b Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Wed, 30 Apr 2025 17:00:56 +0530 Subject: [PATCH 3/9] feat: Implement strict validation for client while loading tools --- .../toolbox-core/src/toolbox_core/client.py | 101 ++++++++++++++++-- 1 file changed, 91 insertions(+), 10 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/client.py b/packages/toolbox-core/src/toolbox_core/client.py index 14dba866..d114b127 100644 --- a/packages/toolbox-core/src/toolbox_core/client.py +++ b/packages/toolbox-core/src/toolbox_core/client.py @@ -157,6 +157,9 @@ async def load_tool( for execution. The specific arguments and behavior of the callable depend on the tool itself. + Raises: + ValueError: If the loaded tool instance fails to utilize at least + one provided parameter or auth token (if any provided). """ # Resolve client headers resolved_headers = { @@ -174,7 +177,7 @@ async def load_tool( if name not in manifest.tools: # TODO: Better exception raise Exception(f"Tool '{name}' not found!") - tool, _, _ = self.__parse_tool( + tool, used_auth_keys, used_bound_keys = self.__parse_tool( name, manifest.tools[name], auth_token_getters, @@ -182,6 +185,26 @@ async def load_tool( self.__client_headers, ) + provided_auth_keys = set(auth_token_getters.keys()) + provided_bound_keys = set(bound_params.keys()) + + unused_auth = provided_auth_keys - used_auth_keys + unused_bound = provided_bound_keys - used_bound_keys + + if unused_auth or unused_bound: + error_messages = [] + if unused_auth: + error_messages.append( + f"unused auth tokens: {', '.join(unused_auth)}" + ) + if unused_bound: + error_messages.append( + f"unused bound parameters: {', '.join(unused_bound)}" + ) + raise ValueError( + f"Validation failed for tool '{name}': { '; '.join(error_messages) }." + ) + return tool async def load_toolset( @@ -189,40 +212,98 @@ async def load_toolset( name: Optional[str] = None, auth_token_getters: dict[str, Callable[[], str]] = {}, bound_params: Mapping[str, Union[Callable[[], Any], Any]] = {}, + strict: bool = False, ) -> list[ToolboxTool]: """ Asynchronously fetches a toolset and loads all tools defined within it. Args: - name: Name of the toolset to load tools. + name: Name of the toolset to load. If None, loads the default toolset. auth_token_getters: A mapping of authentication service names to callables that return the corresponding authentication token. bound_params: A mapping of parameter names to bind to specific values or callables that are called to produce values as needed. + strict: If True, raises an error if *any* loaded tool instance fails + to utilize at least one provided parameter or auth token (if any + provided). If False (default), raises an error only if a + user-provided parameter or auth token cannot be applied to *any* + loaded tool across the set. Returns: list[ToolboxTool]: A list of callables, one for each tool defined in the toolset. + + Raises: + ValueError: If validation fails based on the `strict` flag. """ + # Resolve client headers original_headers = self.__client_headers resolved_headers = { header_name: await resolve_value(original_headers[header_name]) for header_name in original_headers } - # Request the definition of the tool from the server + # Request the definition of the toolset from the server url = f"{self.__base_url}/api/toolset/{name or ''}" async with self.__session.get(url, headers=resolved_headers) as response: json = await response.json() manifest: ManifestSchema = ManifestSchema(**json) - # parse each tools name and schema into a list of ToolboxTools - tools = [ - self.__parse_tool( - n, s, auth_token_getters, bound_params, self.__client_headers - )[0] - for n, s in manifest.tools.items() - ] + tools: list[ToolboxTool] = [] + overall_used_auth_keys: set[str] = set() + overall_used_bound_params: set[str] = set() + provided_auth_keys = set(auth_token_getters.keys()) + provided_bound_keys = set(bound_params.keys()) + + # parse each tool's name and schema into a list of ToolboxTools + for tool_name, schema in manifest.tools.items(): + tool, used_auth_keys, used_bound_keys = self.__parse_tool( + tool_name, + schema, + auth_token_getters, + bound_params, + self.__client_headers, + ) + tools.append(tool) + + if strict: + unused_auth = provided_auth_keys - used_auth_keys + unused_bound = provided_bound_keys - used_bound_keys + if unused_auth or unused_bound: + error_messages = [] + if unused_auth: + error_messages.append( + f"unused auth tokens: {', '.join(unused_auth)}" + ) + if unused_bound: + error_messages.append( + f"unused bound parameters: {', '.join(unused_bound)}" + ) + raise ValueError( + f"Validation failed for tool '{name}': { '; '.join(error_messages) }." + ) + else: + overall_used_auth_keys.update(used_auth_keys) + overall_used_bound_params.update(used_bound_keys) + + if not strict: + unused_auth = provided_auth_keys - overall_used_auth_keys + unused_bound = provided_bound_keys - overall_used_bound_params + + if unused_auth or unused_bound: + error_messages = [] + if unused_auth: + error_messages.append( + f"unused auth tokens could not be applied to any tool: {', '.join(unused_auth)}" + ) + if unused_bound: + error_messages.append( + f"unused bound parameters could not be applied to any tool: {', '.join(unused_bound)}" + ) + raise ValueError( + f"Validation failed for toolset '{name or 'default'}': { '; '.join(error_messages) }." + ) + return tools async def add_headers( From d6f13b3feabad66dc4d497db4f362d0b4c9744ef Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Fri, 2 May 2025 19:02:13 +0530 Subject: [PATCH 4/9] fix: Fix adding tool's name for strict while loading toolset. --- packages/toolbox-core/src/toolbox_core/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/toolbox-core/src/toolbox_core/client.py b/packages/toolbox-core/src/toolbox_core/client.py index d114b127..a6acbce1 100644 --- a/packages/toolbox-core/src/toolbox_core/client.py +++ b/packages/toolbox-core/src/toolbox_core/client.py @@ -280,7 +280,7 @@ async def load_toolset( f"unused bound parameters: {', '.join(unused_bound)}" ) raise ValueError( - f"Validation failed for tool '{name}': { '; '.join(error_messages) }." + f"Validation failed for tool '{tool_name}': { '; '.join(error_messages) }." ) else: overall_used_auth_keys.update(used_auth_keys) From bd7e004b38a6602690552e47e9e329adefbf245d Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Fri, 2 May 2025 19:02:22 +0530 Subject: [PATCH 5/9] chore: Add unit test cases. --- packages/toolbox-core/tests/test_client.py | 519 ++++++++++++++++++++- 1 file changed, 511 insertions(+), 8 deletions(-) diff --git a/packages/toolbox-core/tests/test_client.py b/packages/toolbox-core/tests/test_client.py index e05445f8..971f69e0 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -66,6 +66,40 @@ def test_tool_auth(): ], ) +@pytest.fixture +def tool_schema_minimal(): + """A tool with no parameters, no auth.""" + return ToolSchema( + description="Minimal Test Tool", + parameters=[], + ) + +@pytest.fixture +def tool_schema_requires_auth_X(): + """A tool requiring 'auth_service_X'.""" + return ToolSchema( + description="Tool Requiring Auth X", + parameters=[ + ParameterSchema( + name="auth_param_X", # + type="string", + description="Auth X Token", + authSources=["auth_service_X"], + ), + ParameterSchema(name="data", type="string", description="Some data"), + ], + ) + + +@pytest.fixture +def tool_schema_with_param_P(): + """A tool with a specific parameter 'param_P'.""" + return ToolSchema( + description="Tool with Parameter P", + parameters=[ + ParameterSchema(name="param_P", type="string", description="Parameter P"), + ], + ) # --- Helper Functions for Mocking --- @@ -211,8 +245,8 @@ async def test_invoke_tool_server_error(aioresponses, test_tool_str): @pytest.mark.asyncio async def test_load_tool_not_found_in_manifest(aioresponses, test_tool_str): """ - Tests that load_tool raises an Exception when the requested tool name - is not found in the manifest returned by the server, using existing fixtures. + Tests that load_tool raises an Exception when the requested tool name is not + found in the manifest returned by the server, using existing fixtures. """ ACTUAL_TOOL_IN_MANIFEST = "actual_tool_abc" REQUESTED_TOOL_NAME = "non_existent_tool_xyz" @@ -529,6 +563,471 @@ async def test_bind_param_async_callable_value_success(self, tool_name, client): bound_async_callable.assert_awaited_once() +class TestUnusedParameterValidation: + """ + Tests for validation errors related to unused auth tokens or bound + parameters during tool loading. + """ + + # --- Tests for load_tool --- + + @pytest.mark.asyncio + async def test_load_tool_with_unused_auth_token_raises_error( + self, aioresponses, tool_schema_minimal + ): + """ + Tests load_tool raises ValueError if an unused auth token is provided. + The tool (tool_schema_minimal) does not declare any authSources. + """ + tool_name = "minimal_tool_for_unused_auth" + mock_tool_load(aioresponses, tool_name, tool_schema_minimal, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for tool '{tool_name}': unused auth tokens: unused_auth_service", + ): + await client.load_tool( + tool_name, + auth_token_getters={"unused_auth_service": lambda: "token"}, + ) + + @pytest.mark.asyncio + async def test_load_tool_with_unused_bound_parameter_raises_error( + self, aioresponses, tool_schema_minimal + ): + """ + Tests load_tool raises ValueError if an unused bound parameter is + provided. The tool (tool_schema_minimal) has no parameters to bind. + """ + tool_name = "minimal_tool_for_unused_bound" + mock_tool_load(aioresponses, tool_name, tool_schema_minimal, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for tool '{tool_name}': unused bound parameters: unused_bound_param", + ): + await client.load_tool( + tool_name, bound_params={"unused_bound_param": "value"} + ) + + @pytest.mark.asyncio + async def test_load_tool_with_unused_auth_and_bound_raises_error( + self, aioresponses, tool_schema_minimal + ): + """ + Tests load_tool raises ValueError if both unused auth and bound params + are provided. + """ + tool_name = "minimal_tool_for_unused_both" + mock_tool_load(aioresponses, tool_name, tool_schema_minimal, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for tool '{tool_name}': unused auth tokens: unused_auth_service; unused bound parameters: unused_bound_param", + ): + await client.load_tool( + tool_name, + auth_token_getters={"unused_auth_service": lambda: "token"}, + bound_params={"unused_bound_param": "value"}, + ) + + # --- Tests for load_toolset (strict=True) --- + + @pytest.mark.asyncio + async def test_load_toolset_strict_one_tool_unused_auth_raises( + self, aioresponses, tool_schema_minimal + ): + """ + Tests load_toolset(strict=True) with one tool. If that tool doesn't use + a provided auth token, it raises an error. + """ + toolset_name = "strict_set_single_tool_unused_auth" + tool_A_name = "tool_A_minimal" + tools_dict = {tool_A_name: tool_schema_minimal} + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for tool '{tool_A_name}': unused auth tokens: auth_Y", + ): + await client.load_toolset( + toolset_name, + auth_token_getters={"auth_Y": lambda: "tokenY"}, + strict=True, + ) + + @pytest.mark.asyncio + async def test_load_toolset_strict_first_tool_fails_auth_raises( + self, aioresponses, tool_schema_minimal, tool_schema_requires_auth_X + ): + """ + Tests load_toolset(strict=True). If the first tool processed doesn't use + a provided auth token (even if other tokens are used by other tools, or + itself), it raises an error. + """ + toolset_name = "strict_set_first_tool_fails_auth" + tool_minimal_name = "minimal_first" + tool_auth_X_name = "auth_tool_x_later" + + tools_dict = { + tool_minimal_name: tool_schema_minimal, + tool_auth_X_name: tool_schema_requires_auth_X, + } + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for tool '{tool_minimal_name}': unused auth tokens: auth_service_X", + ): + await client.load_toolset( + toolset_name, + auth_token_getters={ + "auth_service_X": lambda: "tokenX", + }, + strict=True, + ) + + @pytest.mark.asyncio + async def test_load_toolset_strict_second_tool_fails_auth_raises( + self, aioresponses, tool_schema_minimal, tool_schema_requires_auth_X + ): + """ + Tests load_toolset(strict=True). If the first tool is fine, but a + subsequent tool doesn't use a provided auth token, it raises an error. + """ + toolset_name = "strict_set_second_tool_fails_auth" + tool_auth_X_name = "auth_tool_x_first" + tool_minimal_name = "minimal_later" + + tools_dict = { + tool_auth_X_name: tool_schema_requires_auth_X, + tool_minimal_name: tool_schema_minimal, + } + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for tool '{tool_minimal_name}': unused auth tokens: auth_service_X", + ): + await client.load_toolset( + toolset_name, + auth_token_getters={ + "auth_service_X": lambda: "tokenX", + }, + strict=True, + ) + @pytest.mark.asyncio + async def test_load_toolset_strict_one_tool_unused_bound_raises( + self, aioresponses, tool_schema_minimal + ): + """ + Tests load_toolset(strict=True) with one tool. If that tool doesn't use + a provided bound parameter, it raises an error. + """ + toolset_name = "strict_set_single_tool_unused_bound" + tool_A_name = "tool_A_minimal_for_bound" + tools_dict = {tool_A_name: tool_schema_minimal} + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for tool '{tool_A_name}': unused bound parameters: param_Q", + ): + await client.load_toolset( + toolset_name, + bound_params={"param_Q": "valueQ"}, + strict=True, + ) + + @pytest.mark.asyncio + async def test_load_toolset_strict_first_tool_fails_bound_raises( + self, aioresponses, tool_schema_minimal, tool_schema_with_param_P + ): + """ + Tests load_toolset(strict=True). If the first tool processed doesn't use + a provided bound parameter, it raises an error. + """ + toolset_name = "strict_set_first_tool_fails_bound" + tool_minimal_name = "minimal_first_bound" + tool_param_P_name = "param_p_tool_later" + + tools_dict = { + tool_minimal_name: tool_schema_minimal, + tool_param_P_name: tool_schema_with_param_P, + } + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for tool '{tool_minimal_name}': unused bound parameters: param_P", + ): + await client.load_toolset( + toolset_name, + bound_params={ + "param_P": "valueP", + }, + strict=True, + ) + + @pytest.mark.asyncio + async def test_load_toolset_strict_second_tool_fails_bound_raises( + self, aioresponses, tool_schema_minimal, tool_schema_with_param_P + ): + """ + Tests load_toolset(strict=True). If the first tool is fine, but a + subsequent tool doesn't use a provided bound parameter, it raises an error. + """ + toolset_name = "strict_set_second_tool_fails_bound" + tool_param_P_name = "param_p_tool_first" + tool_minimal_name = "minimal_later" + + tools_dict = { + tool_param_P_name: tool_schema_with_param_P, + tool_minimal_name: tool_schema_minimal, + } + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for tool '{tool_minimal_name}': unused bound parameters: param_P", + ): + await client.load_toolset( + toolset_name, + bound_params={ + "param_P": "valueP", + }, + strict=True, + ) + + + + @pytest.mark.asyncio + async def test_load_toolset_strict_with_unused_auth_and_bound_raises_error( + self, aioresponses, tool_schema_minimal + ): + """ + Tests load_toolset(strict=True) raises ValueError if both unused auth + and bound params exist for a tool. Uses a single minimal tool in the + set. + """ + toolset_name = "strict_set_unused_both_minimal_tool" + tool_minimal_name = "minimal_for_both_strict" + tools_dict = {tool_minimal_name: tool_schema_minimal} + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for tool '{tool_minimal_name}': unused auth tokens: stray_auth; unused bound parameters: stray_param", + ): + await client.load_toolset( + toolset_name, + auth_token_getters={"stray_auth": lambda: "stray_token"}, + bound_params={"stray_param": "stray_value"}, + strict=True, + ) + + # --- Tests for load_toolset (strict=False) --- + + @pytest.mark.asyncio + async def test_load_toolset_non_strict_globally_unused_auth_raises_error( + self, aioresponses, tool_schema_minimal, tool_schema_requires_auth_X + ): + """ + Tests load_toolset(strict=False) raises ValueError if an auth token is + unused by ALL tools. + """ + toolset_name = "non_strict_globally_unused_auth" + tools_dict = { + "auth_tool": tool_schema_requires_auth_X, + "minimal_tool_in_set": tool_schema_minimal, + } + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for toolset '{toolset_name}': unused auth tokens could not be applied to any tool: globally_unused_auth", + ): + await client.load_toolset( + toolset_name, + auth_token_getters={ + "auth_service_X": lambda: "tokenX", + "globally_unused_auth": lambda: "tokenG", + }, + strict=False, + ) + + @pytest.mark.asyncio + async def test_load_toolset_non_strict_globally_unused_bound_raises_error( + self, aioresponses, tool_schema_minimal, tool_schema_with_param_P + ): + """ + Tests load_toolset(strict=False) raises ValueError if a bound param is + unused by ALL tools. + """ + toolset_name = "non_strict_globally_unused_bound" + tools_dict = { + "param_P_tool_in_set": tool_schema_with_param_P, + "minimal_tool_in_set_bound": tool_schema_minimal, + } + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for toolset '{toolset_name}': unused bound parameters could not be applied to any tool: globally_unused_param", + ): + await client.load_toolset( + toolset_name, + bound_params={ + "param_P": "valueP", + "globally_unused_param": "valueG", + }, + strict=False, + ) + + @pytest.mark.asyncio + async def test_load_toolset_non_strict_globally_unused_auth_and_bound_raises_error( + self, aioresponses, tool_schema_requires_auth_X, tool_schema_with_param_P + ): + """ + Tests load_toolset(strict=False) raises ValueError if globally unused + auth and bound params exist. + """ + toolset_name = "non_strict_globally_unused_both" + tools_dict = { + "auth_tool_for_both": tool_schema_requires_auth_X, + "param_P_tool_for_both": tool_schema_with_param_P, + } + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for toolset '{toolset_name}': unused auth tokens could not be applied to any tool: globally_unused_auth; unused bound parameters could not be applied to any tool: globally_unused_param", + ): + await client.load_toolset( + toolset_name, + auth_token_getters={ + "auth_service_X": lambda: "tokenX", + "globally_unused_auth": lambda: "tokenG", + }, + bound_params={ + "param_P": "valueP", + "globally_unused_param": "valueG", + }, + strict=False, + ) + + @pytest.mark.asyncio + async def test_load_toolset_non_strict_default_name_globally_unused_auth( + self, aioresponses, tool_schema_minimal + ): + """ + Tests load_toolset(strict=False, name=None) raises ValueError for + globally unused auth, checking the 'default' toolset name in the error. + """ + toolset_name = None + expected_error_toolset_name = "default" + tools_dict = {"some_minimal_tool": tool_schema_minimal} + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + with pytest.raises( + ValueError, + match=rf"Validation failed for toolset '{expected_error_toolset_name}': unused auth tokens could not be applied to any tool: globally_unused_auth_default", + ): + await client.load_toolset( + name=toolset_name, + auth_token_getters={"globally_unused_auth_default": lambda: "token"}, + strict=False, + ) + + + @pytest.mark.asyncio + async def test_load_toolset_non_strict_partially_used_auth_succeeds( + self, aioresponses, tool_schema_minimal, tool_schema_requires_auth_X + ): + """ + Tests load_toolset(strict=False) succeeds if an auth token is used by at + least one tool, even if not by all. + """ + toolset_name = "non_strict_partially_used_auth" + tools_dict = { + "auth_tool_partial": tool_schema_requires_auth_X, + "minimal_tool_partial": tool_schema_minimal, + } + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + await client.load_toolset( + toolset_name, + auth_token_getters={"auth_service_X": lambda: "tokenX"}, + strict=False, + ) + + + @pytest.mark.asyncio + async def test_load_toolset_non_strict_partially_used_bound_succeeds( + self, aioresponses, tool_schema_minimal, tool_schema_with_param_P + ): + """ + Tests load_toolset(strict=False) succeeds if a bound param is used by at + least one tool, even if not by all. + """ + toolset_name = "non_strict_partially_used_bound" + tools_dict = { + "param_P_tool_partial": tool_schema_with_param_P, + "minimal_tool_partial_bound": tool_schema_minimal, + } + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + await client.load_toolset( + toolset_name, + bound_params={"param_P": "valueP"}, + strict=False, + ) + + + @pytest.mark.asyncio + async def test_load_toolset_non_strict_partially_used_auth_and_bound_succeeds( + self, aioresponses, tool_schema_requires_auth_X, tool_schema_with_param_P + ): + """ + Tests load_toolset(strict=False) succeeds if both an auth token and a + bound param is used by at least one tool, even if not by all. + """ + toolset_name = "non_strict_partially_used_both" + tools_dict = { + "auth_tool_for_both": tool_schema_requires_auth_X, + "param_P_tool_for_both": tool_schema_with_param_P, + } + mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + + async with ToolboxClient(TEST_BASE_URL) as client: + await client.load_toolset( + toolset_name, + auth_token_getters={ + "auth_service_X": lambda: "tokenX", + }, + bound_params={ + "param_P": "valueP", + }, + strict=False, + ) + + class TestClientHeaders: @pytest.fixture def static_header(self): @@ -561,8 +1060,8 @@ def create_callback_factory( callback_status: int = 200, ) -> Callable: """ - Factory that RETURNS a callback function for aioresponses. - The returned callback will check headers and return the specified payload/status. + Factory that RETURNS a callback function for aioresponses. The returned + callback will check headers and return the specified payload/status. """ def actual_callback(url, **kwargs): @@ -616,7 +1115,8 @@ async def test_load_tool_with_sync_callable_headers( sync_callable_header, sync_callable_header_value, ): - """Tests loading and invoking a tool with sync callable client headers.""" + """Tests loading and invoking a tool with sync callable client + headers.""" tool_name = "tool_with_sync_callable_headers" manifest = ManifestSchema( serverVersion="0.0.0", tools={tool_name: test_tool_str} @@ -660,7 +1160,8 @@ async def test_load_tool_with_async_callable_headers( async_callable_header, async_callable_header_value, ): - """Tests loading and invoking a tool with async callable client headers.""" + """Tests loading and invoking a tool with async callable client + headers.""" tool_name = "tool_with_async_callable_headers" manifest = ManifestSchema( serverVersion="0.0.0", tools={tool_name: test_tool_str} @@ -757,7 +1258,8 @@ async def test_add_headers_success( @pytest.mark.asyncio async def test_add_headers_duplicate_fail(self, static_header): - """Tests that adding a duplicate header via add_headers raises ValueError.""" + """Tests that adding a duplicate header via add_headers raises + ValueError.""" async with ToolboxClient(TEST_BASE_URL, client_headers=static_header) as client: with pytest.raises( ValueError, @@ -770,7 +1272,8 @@ async def test_client_header_auth_token_conflict_fail( self, aioresponses, test_tool_auth ): """ - Tests that loading a tool fails if a client header conflicts with an auth token name. + Tests that loading a tool fails if a client header conflicts with an + auth token name. """ tool_name = "auth_conflict_tool" conflict_key = "my-auth-service_token" From d37cb569ed402808adba4e941c9daa01de707f28 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Fri, 2 May 2025 19:03:25 +0530 Subject: [PATCH 6/9] chore: Delint --- .../toolbox-core/src/toolbox_core/client.py | 4 +- packages/toolbox-core/tests/test_client.py | 85 +++++++++++++------ 2 files changed, 61 insertions(+), 28 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/client.py b/packages/toolbox-core/src/toolbox_core/client.py index a6acbce1..3fe0977d 100644 --- a/packages/toolbox-core/src/toolbox_core/client.py +++ b/packages/toolbox-core/src/toolbox_core/client.py @@ -194,9 +194,7 @@ async def load_tool( if unused_auth or unused_bound: error_messages = [] if unused_auth: - error_messages.append( - f"unused auth tokens: {', '.join(unused_auth)}" - ) + error_messages.append(f"unused auth tokens: {', '.join(unused_auth)}") if unused_bound: error_messages.append( f"unused bound parameters: {', '.join(unused_bound)}" diff --git a/packages/toolbox-core/tests/test_client.py b/packages/toolbox-core/tests/test_client.py index 971f69e0..a6561d06 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -66,6 +66,7 @@ def test_tool_auth(): ], ) + @pytest.fixture def tool_schema_minimal(): """A tool with no parameters, no auth.""" @@ -74,6 +75,7 @@ def tool_schema_minimal(): parameters=[], ) + @pytest.fixture def tool_schema_requires_auth_X(): """A tool requiring 'auth_service_X'.""" @@ -81,7 +83,7 @@ def tool_schema_requires_auth_X(): description="Tool Requiring Auth X", parameters=[ ParameterSchema( - name="auth_param_X", # + name="auth_param_X", # type="string", description="Auth X Token", authSources=["auth_service_X"], @@ -101,6 +103,7 @@ def tool_schema_with_param_P(): ], ) + # --- Helper Functions for Mocking --- @@ -580,7 +583,9 @@ async def test_load_tool_with_unused_auth_token_raises_error( The tool (tool_schema_minimal) does not declare any authSources. """ tool_name = "minimal_tool_for_unused_auth" - mock_tool_load(aioresponses, tool_name, tool_schema_minimal, base_url=TEST_BASE_URL) + mock_tool_load( + aioresponses, tool_name, tool_schema_minimal, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -601,7 +606,9 @@ async def test_load_tool_with_unused_bound_parameter_raises_error( provided. The tool (tool_schema_minimal) has no parameters to bind. """ tool_name = "minimal_tool_for_unused_bound" - mock_tool_load(aioresponses, tool_name, tool_schema_minimal, base_url=TEST_BASE_URL) + mock_tool_load( + aioresponses, tool_name, tool_schema_minimal, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -621,7 +628,9 @@ async def test_load_tool_with_unused_auth_and_bound_raises_error( are provided. """ tool_name = "minimal_tool_for_unused_both" - mock_tool_load(aioresponses, tool_name, tool_schema_minimal, base_url=TEST_BASE_URL) + mock_tool_load( + aioresponses, tool_name, tool_schema_minimal, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -647,7 +656,9 @@ async def test_load_toolset_strict_one_tool_unused_auth_raises( toolset_name = "strict_set_single_tool_unused_auth" tool_A_name = "tool_A_minimal" tools_dict = {tool_A_name: tool_schema_minimal} - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -677,7 +688,9 @@ async def test_load_toolset_strict_first_tool_fails_auth_raises( tool_minimal_name: tool_schema_minimal, tool_auth_X_name: tool_schema_requires_auth_X, } - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -708,7 +721,9 @@ async def test_load_toolset_strict_second_tool_fails_auth_raises( tool_auth_X_name: tool_schema_requires_auth_X, tool_minimal_name: tool_schema_minimal, } - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -722,6 +737,7 @@ async def test_load_toolset_strict_second_tool_fails_auth_raises( }, strict=True, ) + @pytest.mark.asyncio async def test_load_toolset_strict_one_tool_unused_bound_raises( self, aioresponses, tool_schema_minimal @@ -733,7 +749,9 @@ async def test_load_toolset_strict_one_tool_unused_bound_raises( toolset_name = "strict_set_single_tool_unused_bound" tool_A_name = "tool_A_minimal_for_bound" tools_dict = {tool_A_name: tool_schema_minimal} - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -762,7 +780,9 @@ async def test_load_toolset_strict_first_tool_fails_bound_raises( tool_minimal_name: tool_schema_minimal, tool_param_P_name: tool_schema_with_param_P, } - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -793,7 +813,9 @@ async def test_load_toolset_strict_second_tool_fails_bound_raises( tool_param_P_name: tool_schema_with_param_P, tool_minimal_name: tool_schema_minimal, } - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -808,8 +830,6 @@ async def test_load_toolset_strict_second_tool_fails_bound_raises( strict=True, ) - - @pytest.mark.asyncio async def test_load_toolset_strict_with_unused_auth_and_bound_raises_error( self, aioresponses, tool_schema_minimal @@ -822,7 +842,9 @@ async def test_load_toolset_strict_with_unused_auth_and_bound_raises_error( toolset_name = "strict_set_unused_both_minimal_tool" tool_minimal_name = "minimal_for_both_strict" tools_dict = {tool_minimal_name: tool_schema_minimal} - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -851,7 +873,9 @@ async def test_load_toolset_non_strict_globally_unused_auth_raises_error( "auth_tool": tool_schema_requires_auth_X, "minimal_tool_in_set": tool_schema_minimal, } - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -880,7 +904,9 @@ async def test_load_toolset_non_strict_globally_unused_bound_raises_error( "param_P_tool_in_set": tool_schema_with_param_P, "minimal_tool_in_set_bound": tool_schema_minimal, } - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -909,7 +935,9 @@ async def test_load_toolset_non_strict_globally_unused_auth_and_bound_raises_err "auth_tool_for_both": tool_schema_requires_auth_X, "param_P_tool_for_both": tool_schema_with_param_P, } - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -928,7 +956,7 @@ async def test_load_toolset_non_strict_globally_unused_auth_and_bound_raises_err }, strict=False, ) - + @pytest.mark.asyncio async def test_load_toolset_non_strict_default_name_globally_unused_auth( self, aioresponses, tool_schema_minimal @@ -940,7 +968,9 @@ async def test_load_toolset_non_strict_default_name_globally_unused_auth( toolset_name = None expected_error_toolset_name = "default" tools_dict = {"some_minimal_tool": tool_schema_minimal} - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: with pytest.raises( @@ -949,11 +979,12 @@ async def test_load_toolset_non_strict_default_name_globally_unused_auth( ): await client.load_toolset( name=toolset_name, - auth_token_getters={"globally_unused_auth_default": lambda: "token"}, + auth_token_getters={ + "globally_unused_auth_default": lambda: "token" + }, strict=False, ) - @pytest.mark.asyncio async def test_load_toolset_non_strict_partially_used_auth_succeeds( self, aioresponses, tool_schema_minimal, tool_schema_requires_auth_X @@ -967,7 +998,9 @@ async def test_load_toolset_non_strict_partially_used_auth_succeeds( "auth_tool_partial": tool_schema_requires_auth_X, "minimal_tool_partial": tool_schema_minimal, } - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: await client.load_toolset( @@ -976,7 +1009,6 @@ async def test_load_toolset_non_strict_partially_used_auth_succeeds( strict=False, ) - @pytest.mark.asyncio async def test_load_toolset_non_strict_partially_used_bound_succeeds( self, aioresponses, tool_schema_minimal, tool_schema_with_param_P @@ -990,7 +1022,9 @@ async def test_load_toolset_non_strict_partially_used_bound_succeeds( "param_P_tool_partial": tool_schema_with_param_P, "minimal_tool_partial_bound": tool_schema_minimal, } - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: await client.load_toolset( @@ -999,7 +1033,6 @@ async def test_load_toolset_non_strict_partially_used_bound_succeeds( strict=False, ) - @pytest.mark.asyncio async def test_load_toolset_non_strict_partially_used_auth_and_bound_succeeds( self, aioresponses, tool_schema_requires_auth_X, tool_schema_with_param_P @@ -1013,7 +1046,9 @@ async def test_load_toolset_non_strict_partially_used_auth_and_bound_succeeds( "auth_tool_for_both": tool_schema_requires_auth_X, "param_P_tool_for_both": tool_schema_with_param_P, } - mock_toolset_load(aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL) + mock_toolset_load( + aioresponses, toolset_name, tools_dict, base_url=TEST_BASE_URL + ) async with ToolboxClient(TEST_BASE_URL) as client: await client.load_toolset( From 69094773d01ed96ea642748c659ce507296ae211 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Fri, 2 May 2025 20:03:37 +0530 Subject: [PATCH 7/9] chore: Fix integration tests. --- packages/toolbox-core/tests/test_e2e.py | 13 ++++++++----- packages/toolbox-core/tests/test_sync_e2e.py | 13 ++++++++----- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/toolbox-core/tests/test_e2e.py b/packages/toolbox-core/tests/test_e2e.py index cf7b21d1..717f70f0 100644 --- a/packages/toolbox-core/tests/test_e2e.py +++ b/packages/toolbox-core/tests/test_e2e.py @@ -133,11 +133,14 @@ async def test_run_tool_unauth_with_auth( self, toolbox: ToolboxClient, auth_token2: str ): """Tests running a tool that doesn't require auth, with auth provided.""" - tool = await toolbox.load_tool( - "get-row-by-id", auth_token_getters={"my-test-auth": lambda: auth_token2} - ) - response = await tool(id="2") - assert "row2" in response + + with pytest.raises( + ValueError, + match=rf"Validation failed for tool 'get-row-by-id': unused auth tokens: my-test-auth", + ): + await toolbox.load_tool( + "get-row-by-id", auth_token_getters={"my-test-auth": lambda: auth_token2} + ) async def test_run_tool_no_auth(self, toolbox: ToolboxClient): """Tests running a tool requiring auth without providing auth.""" diff --git a/packages/toolbox-core/tests/test_sync_e2e.py b/packages/toolbox-core/tests/test_sync_e2e.py index 055ca977..6ee80af8 100644 --- a/packages/toolbox-core/tests/test_sync_e2e.py +++ b/packages/toolbox-core/tests/test_sync_e2e.py @@ -115,11 +115,14 @@ def test_run_tool_unauth_with_auth( self, toolbox: ToolboxSyncClient, auth_token2: str ): """Tests running a tool that doesn't require auth, with auth provided.""" - tool = toolbox.load_tool( - "get-row-by-id", auth_token_getters={"my-test-auth": lambda: auth_token2} - ) - response = tool(id="2") - assert "row2" in response + + with pytest.raises( + ValueError, + match=rf"Validation failed for tool 'get-row-by-id': unused auth tokens: my-test-auth", + ): + toolbox.load_tool( + "get-row-by-id", auth_token_getters={"my-test-auth": lambda: auth_token2} + ) def test_run_tool_no_auth(self, toolbox: ToolboxSyncClient): """Tests running a tool requiring auth without providing auth.""" From ce5ee7ea299b2964eee693f284cfc916ab35e39f Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Fri, 2 May 2025 20:06:44 +0530 Subject: [PATCH 8/9] chore: Delint --- packages/toolbox-core/tests/test_e2e.py | 3 ++- packages/toolbox-core/tests/test_sync_e2e.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/toolbox-core/tests/test_e2e.py b/packages/toolbox-core/tests/test_e2e.py index 717f70f0..88dbd4be 100644 --- a/packages/toolbox-core/tests/test_e2e.py +++ b/packages/toolbox-core/tests/test_e2e.py @@ -139,7 +139,8 @@ async def test_run_tool_unauth_with_auth( match=rf"Validation failed for tool 'get-row-by-id': unused auth tokens: my-test-auth", ): await toolbox.load_tool( - "get-row-by-id", auth_token_getters={"my-test-auth": lambda: auth_token2} + "get-row-by-id", + auth_token_getters={"my-test-auth": lambda: auth_token2}, ) async def test_run_tool_no_auth(self, toolbox: ToolboxClient): diff --git a/packages/toolbox-core/tests/test_sync_e2e.py b/packages/toolbox-core/tests/test_sync_e2e.py index 6ee80af8..3429645b 100644 --- a/packages/toolbox-core/tests/test_sync_e2e.py +++ b/packages/toolbox-core/tests/test_sync_e2e.py @@ -121,7 +121,8 @@ def test_run_tool_unauth_with_auth( match=rf"Validation failed for tool 'get-row-by-id': unused auth tokens: my-test-auth", ): toolbox.load_tool( - "get-row-by-id", auth_token_getters={"my-test-auth": lambda: auth_token2} + "get-row-by-id", + auth_token_getters={"my-test-auth": lambda: auth_token2}, ) def test_run_tool_no_auth(self, toolbox: ToolboxSyncClient): From e7683f4178348dde5105ba32b819845f611e048b Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Mon, 12 May 2025 10:30:27 +0530 Subject: [PATCH 9/9] chore: Remove unnecessary if statement --- .../toolbox-core/src/toolbox_core/client.py | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/client.py b/packages/toolbox-core/src/toolbox_core/client.py index 3fe0977d..6f9b3eab 100644 --- a/packages/toolbox-core/src/toolbox_core/client.py +++ b/packages/toolbox-core/src/toolbox_core/client.py @@ -284,23 +284,22 @@ async def load_toolset( overall_used_auth_keys.update(used_auth_keys) overall_used_bound_params.update(used_bound_keys) - if not strict: - unused_auth = provided_auth_keys - overall_used_auth_keys - unused_bound = provided_bound_keys - overall_used_bound_params - - if unused_auth or unused_bound: - error_messages = [] - if unused_auth: - error_messages.append( - f"unused auth tokens could not be applied to any tool: {', '.join(unused_auth)}" - ) - if unused_bound: - error_messages.append( - f"unused bound parameters could not be applied to any tool: {', '.join(unused_bound)}" - ) - raise ValueError( - f"Validation failed for toolset '{name or 'default'}': { '; '.join(error_messages) }." + unused_auth = provided_auth_keys - overall_used_auth_keys + unused_bound = provided_bound_keys - overall_used_bound_params + + if unused_auth or unused_bound: + error_messages = [] + if unused_auth: + error_messages.append( + f"unused auth tokens could not be applied to any tool: {', '.join(unused_auth)}" + ) + if unused_bound: + error_messages.append( + f"unused bound parameters could not be applied to any tool: {', '.join(unused_bound)}" ) + raise ValueError( + f"Validation failed for toolset '{name or 'default'}': { '; '.join(error_messages) }." + ) return tools