From 5ea0ef31a1b5f597af40168c34481f9d3f3c0e0a Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Mon, 21 Apr 2025 18:05:33 +0530 Subject: [PATCH 1/8] fix(toolbox-core): Prevent rebinding of parameters in ToolboxTool This pull request updates the `bind_parameters` helper function within the `ToolboxTool` of the `toolbox-core` package to enhance parameter binding management. * The helper now throws an explicit error if a user attempts to bind a parameter that has already been bound. This change aims to prevent unexpected behavior and improve the clarity of parameter assignments. * The error that previously occurred when a user tried to bind a parameter not present in the tool's schema has been removed. This validation will be handled by a `strict` mode implementation in a future PR. --- packages/toolbox-core/src/toolbox_core/tool.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/tool.py b/packages/toolbox-core/src/toolbox_core/tool.py index d9c0d0fe..857f3273 100644 --- a/packages/toolbox-core/src/toolbox_core/tool.py +++ b/packages/toolbox-core/src/toolbox_core/tool.py @@ -314,10 +314,11 @@ def bind_parameters( Returns: A new ToolboxTool instance with the specified parameters bound. """ - 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" + ) new_params = [] for p in self.__params: From 32d808143d6c00ee6ab10174f3241f9aa0d9e2e2 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Mon, 21 Apr 2025 18:13:29 +0530 Subject: [PATCH 2/8] chore: Update unit test case --- packages/toolbox-core/tests/test_client.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/toolbox-core/tests/test_client.py b/packages/toolbox-core/tests/test_client.py index e6d12a0c..610532aa 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -430,15 +430,23 @@ async def test_bind_callable_param_success(self, tool_name, client): 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.""" + async def test_bind_param_fail_on_rebind(self, tool_name, client): + """ + Tests that 'bind_parameters' 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 - with pytest.raises(Exception): - tool = tool.bind_parameters({"argC": lambda: 5}) + bound_tool_once = tool.bind_parameters({"argA": 5}) + assert "argA" not in bound_tool_once.__signature__.parameters + assert "argB" in bound_tool_once.__signature__.parameters + + expected_error_msg = "cannot re-bind parameter: parameter 'argA' is already bound" + with pytest.raises(ValueError, match=expected_error_msg): + bound_tool_once.bind_parameters({"argA": 10}) @pytest.mark.asyncio async def test_bind_param_static_value_success(self, tool_name, client): From 16525b7ccd36aceb599646cb126e2b352ac00d70 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Mon, 21 Apr 2025 18:15:34 +0530 Subject: [PATCH 3/8] chore: Delint --- packages/toolbox-core/tests/test_client.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/toolbox-core/tests/test_client.py b/packages/toolbox-core/tests/test_client.py index 610532aa..cd602e95 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -444,7 +444,9 @@ async def test_bind_param_fail_on_rebind(self, tool_name, client): assert "argA" not in bound_tool_once.__signature__.parameters assert "argB" in bound_tool_once.__signature__.parameters - expected_error_msg = "cannot re-bind parameter: parameter 'argA' is already bound" + expected_error_msg = ( + "cannot re-bind parameter: parameter 'argA' is already bound" + ) with pytest.raises(ValueError, match=expected_error_msg): bound_tool_once.bind_parameters({"argA": 10}) From bce93071a25c8cd5e5d2a7732de3bc1f52e47fe3 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 29 Apr 2025 16:42:27 +0530 Subject: [PATCH 4/8] 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 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/toolbox-core/src/toolbox_core/tool.py b/packages/toolbox-core/src/toolbox_core/tool.py index 857f3273..22e4610d 100644 --- a/packages/toolbox-core/src/toolbox_core/tool.py +++ b/packages/toolbox-core/src/toolbox_core/tool.py @@ -314,7 +314,10 @@ def bind_parameters( Returns: A new ToolboxTool instance with the specified parameters bound. """ + 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 0e9f1a153abab25284149d52161612253566df35 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 29 Apr 2025 17:11:25 +0530 Subject: [PATCH 5/8] fix: Reverse the error conditions to avoid masking of the second error. --- packages/toolbox-core/src/toolbox_core/tool.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/tool.py b/packages/toolbox-core/src/toolbox_core/tool.py index 22e4610d..ba006e5d 100644 --- a/packages/toolbox-core/src/toolbox_core/tool.py +++ b/packages/toolbox-core/src/toolbox_core/tool.py @@ -316,13 +316,14 @@ 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" ) + if name not in param_names: + raise Exception(f"unable to bind parameters: no parameter named {name}") + new_params = [] for p in self.__params: if p.name not in bound_params: From 0696467b79ed4facc1f547f4320b1c33b6e7c46e Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 29 Apr 2025 17:11:34 +0530 Subject: [PATCH 6/8] chore: Fix unit test cases. --- packages/toolbox-core/tests/test_client.py | 27 ++++++++++++++-------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/packages/toolbox-core/tests/test_client.py b/packages/toolbox-core/tests/test_client.py index cd602e95..93a47711 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -430,7 +430,19 @@ async def test_bind_callable_param_success(self, tool_name, client): assert "argA" in res @pytest.mark.asyncio - async def test_bind_param_fail_on_rebind(self, tool_name, client): + async def test_bind_param_fail(self, tool_name, client): + """Tests 'bind_parameters' 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_parameters({"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_parameters' fails when attempting to re-bind a parameter that has already been bound. @@ -440,15 +452,12 @@ async def test_bind_param_fail_on_rebind(self, tool_name, client): assert len(tool.__signature__.parameters) == 2 assert "argA" in tool.__signature__.parameters - bound_tool_once = tool.bind_parameters({"argA": 5}) - assert "argA" not in bound_tool_once.__signature__.parameters - assert "argB" in bound_tool_once.__signature__.parameters + tool_with_bound_param = tool.bind_parameters({"argA": lambda: 10}) - expected_error_msg = ( - "cannot re-bind parameter: parameter 'argA' is already bound" - ) - with pytest.raises(ValueError, match=expected_error_msg): - bound_tool_once.bind_parameters({"argA": 10}) + with pytest.raises(ValueError) as e: + tool_with_bound_param.bind_parameters({"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): From 53ff480b05e1c0b9df1a4b38a9d9bb4f8f982607 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 29 Apr 2025 17:14:16 +0530 Subject: [PATCH 7/8] chore: Make unit test more robust. --- packages/toolbox-core/tests/test_client.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/toolbox-core/tests/test_client.py b/packages/toolbox-core/tests/test_client.py index 93a47711..67454a36 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -454,6 +454,9 @@ async def test_rebind_param_fail(self, tool_name, client): tool_with_bound_param = tool.bind_parameters({"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_parameters({"argA": lambda: 20}) From 574e39fd473f87acfe6deebe7571cc814b0f3ef6 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 29 Apr 2025 17:15:37 +0530 Subject: [PATCH 8/8] chore: Delint --- packages/toolbox-core/tests/test_client.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/toolbox-core/tests/test_client.py b/packages/toolbox-core/tests/test_client.py index 67454a36..e05445f8 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -460,7 +460,9 @@ async def test_rebind_param_fail(self, tool_name, client): with pytest.raises(ValueError) as e: tool_with_bound_param.bind_parameters({"argA": lambda: 20}) - assert "cannot re-bind parameter: parameter 'argA' is already bound" in str(e.value) + 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):