fix(toolbox-core)!: Standardize on ValueError for improved error consistency (#219)

* chore: Add unit test coverage.

* chore: Consolidate auth header creation logic

Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places.

This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.

* chore(toolbox-core): Standardize on ValueError for improved error consistency

This PR updates instances where generic `Exception` objects were thrown in `toolbox-core` to use `ValueError` instead.

This aligns with the prevalent use of `ValueError` for argument-related issues elsewhere in the package, leading to more specific, predictable, and robust error handling for consumers of `toolbox-core`.

* chore: Delint

* chore: Remove duplicate test

Author: anubhav756

packages/toolbox-core/src/toolbox_core/client.py

@@ -176,7 +176,7 @@ async def load_tool(
         # parse the provided definition to a tool
         if name not in manifest.tools:
             # TODO: Better exception
-            raise Exception(f"Tool '{name}' not found!")
+            raise ValueError(f"Tool '{name}' not found!")
         tool, used_auth_keys, used_bound_keys = self.__parse_tool(
             name,
             manifest.tools[name],

packages/toolbox-core/src/toolbox_core/tool.py

@@ -212,7 +212,7 @@ async def __call__(self, *args: Any, **kwargs: Any) -> str:
             req_auth_services = set()
             for s in self.__required_authn_params.values():
                 req_auth_services.update(s)
-            raise Exception(
+            raise ValueError(
                 f"One or more of the following authn services are required to invoke this tool"
                 f": {','.join(req_auth_services)}"
             )
@@ -329,7 +329,9 @@ def bind_params(
                 )
 
             if name not in param_names:
-                raise Exception(f"unable to bind parameters: no parameter named {name}")
+                raise ValueError(
+                    f"unable to bind parameters: no parameter named {name}"
+                )
 
         new_params = []
         for p in self.__params:

packages/toolbox-core/tests/test_client.py

@@ -266,7 +266,9 @@ async def test_load_tool_not_found_in_manifest(aioresponses, test_tool_str):
     )
 
     async with ToolboxClient(TEST_BASE_URL) as client:
-        with pytest.raises(Exception, match=f"Tool '{REQUESTED_TOOL_NAME}' not found!"):
+        with pytest.raises(
+            ValueError, match=f"Tool '{REQUESTED_TOOL_NAME}' not found!"
+        ):
             await client.load_tool(REQUESTED_TOOL_NAME)
 
     aioresponses.assert_called_once_with(
@@ -474,7 +476,7 @@ async def test_bind_params_fail(self, tool_name, client):
         assert len(tool.__signature__.parameters) == 2
         assert "argA" in tool.__signature__.parameters
 
-        with pytest.raises(Exception) as e:
+        with pytest.raises(ValueError) as e:
             tool.bind_params({"argC": lambda: 5})
         assert "unable to bind parameters: no parameter named argC" in str(e.value)
 
@@ -605,138 +607,7 @@ async def test_bind_param_fail(self, tool_name, client):
         assert len(tool.__signature__.parameters) == 2
         assert "argA" in tool.__signature__.parameters
 
-        with pytest.raises(Exception) as e:
-            tool.bind_param("argC", lambda: 5)
-        assert "unable to bind parameters: no parameter named argC" in str(e.value)
-
-    @pytest.mark.asyncio
-    async def test_rebind_param_fail(self, tool_name, client):
-        """
-        Tests that 'bind_param' fails when attempting to re-bind a
-        parameter that has already been bound.
-        """
-        tool = await client.load_tool(tool_name)
-
-        assert len(tool.__signature__.parameters) == 2
-        assert "argA" in tool.__signature__.parameters
-
-        tool_with_bound_param = tool.bind_param("argA", lambda: 10)
-
-        assert len(tool_with_bound_param.__signature__.parameters) == 1
-        assert "argA" not in tool_with_bound_param.__signature__.parameters
-
         with pytest.raises(ValueError) as e:
-            tool_with_bound_param.bind_param("argA", lambda: 20)
-
-        assert "cannot re-bind parameter: parameter 'argA' is already bound" in str(
-            e.value
-        )
-
-    @pytest.mark.asyncio
-    async def test_bind_param_static_value_success(self, tool_name, client):
-        """
-        Tests bind_param method with a static value.
-        """
-
-        bound_value = "Test value"
-
-        tool = await client.load_tool(tool_name)
-        bound_tool = tool.bind_param("argB", bound_value)
-
-        assert bound_tool is not tool
-        assert "argB" not in bound_tool.__signature__.parameters
-        assert "argA" in bound_tool.__signature__.parameters
-
-        passed_value_a = 42
-        res_payload = await bound_tool(argA=passed_value_a)
-
-        assert res_payload == {"argA": passed_value_a, "argB": bound_value}
-
-    @pytest.mark.asyncio
-    async def test_bind_param_sync_callable_value_success(self, tool_name, client):
-        """
-        Tests bind_param method with a sync callable value.
-        """
-
-        bound_value_result = True
-        bound_sync_callable = Mock(return_value=bound_value_result)
-
-        tool = await client.load_tool(tool_name)
-        bound_tool = tool.bind_param("argB", bound_sync_callable)
-
-        assert bound_tool is not tool
-        assert "argB" not in bound_tool.__signature__.parameters
-        assert "argA" in bound_tool.__signature__.parameters
-
-        passed_value_a = 42
-        res_payload = await bound_tool(argA=passed_value_a)
-
-        assert res_payload == {"argA": passed_value_a, "argB": bound_value_result}
-        bound_sync_callable.assert_called_once()
-
-    @pytest.mark.asyncio
-    async def test_bind_param_async_callable_value_success(self, tool_name, client):
-        """
-        Tests bind_param method with an async callable value.
-        """
-
-        bound_value_result = True
-        bound_async_callable = AsyncMock(return_value=bound_value_result)
-
-        tool = await client.load_tool(tool_name)
-        bound_tool = tool.bind_param("argB", bound_async_callable)
-
-        assert bound_tool is not tool
-        assert "argB" not in bound_tool.__signature__.parameters
-        assert "argA" in bound_tool.__signature__.parameters
-
-        passed_value_a = 42
-        res_payload = await bound_tool(argA=passed_value_a)
-
-        assert res_payload == {"argA": passed_value_a, "argB": bound_value_result}
-        bound_async_callable.assert_awaited_once()
-
-    @pytest.mark.asyncio
-    async def test_bind_param_success(self, tool_name, client):
-        """Tests 'bind_param' with a bound parameter specified."""
-        tool = await client.load_tool(tool_name)
-
-        assert len(tool.__signature__.parameters) == 2
-        assert "argA" in tool.__signature__.parameters
-
-        tool = tool.bind_param("argA", 5)
-
-        assert len(tool.__signature__.parameters) == 1
-        assert "argA" not in tool.__signature__.parameters
-
-        res = await tool(True)
-        assert "argA" in res
-
-    @pytest.mark.asyncio
-    async def test_bind_callable_param_success(self, tool_name, client):
-        """Tests 'bind_param' with a bound parameter specified."""
-        tool = await client.load_tool(tool_name)
-
-        assert len(tool.__signature__.parameters) == 2
-        assert "argA" in tool.__signature__.parameters
-
-        tool = tool.bind_param("argA", lambda: 5)
-
-        assert len(tool.__signature__.parameters) == 1
-        assert "argA" not in tool.__signature__.parameters
-
-        res = await tool(True)
-        assert "argA" in res
-
-    @pytest.mark.asyncio
-    async def test_bind_param_fail(self, tool_name, client):
-        """Tests 'bind_param' with a bound parameter that doesn't exist."""
-        tool = await client.load_tool(tool_name)
-
-        assert len(tool.__signature__.parameters) == 2
-        assert "argA" in tool.__signature__.parameters
-
-        with pytest.raises(Exception) as e:
             tool.bind_param("argC", lambda: 5)
         assert "unable to bind parameters: no parameter named argC" in str(e.value)
 

packages/toolbox-core/tests/test_e2e.py

@@ -188,7 +188,7 @@ async def test_run_tool_param_auth_no_auth(self, toolbox: ToolboxClient):
         """Tests running a tool with a param requiring auth, without auth."""
         tool = await toolbox.load_tool("get-row-by-email-auth")
         with pytest.raises(
-            Exception,
+            ValueError,
             match="One or more of the following authn services are required to invoke this tool: my-test-auth",
         ):
             await tool()

packages/toolbox-core/tests/test_sync_e2e.py

@@ -156,7 +156,7 @@ def test_run_tool_param_auth_no_auth(self, toolbox: ToolboxSyncClient):
         """Tests running a tool with a param requiring auth, without auth."""
         tool = toolbox.load_tool("get-row-by-email-auth")
         with pytest.raises(
-            Exception,
+            ValueError,
             match="One or more of the following authn services are required to invoke this tool: my-test-auth",
         ):
             tool()