From 8a62e5581d2a184d948009604f1602ef693c7f4a Mon Sep 17 00:00:00 2001 From: openhands Date: Fri, 24 Jan 2025 22:11:12 +0000 Subject: [PATCH 1/5] Improve function call validation with better error handling --- .../codeact_agent/function_calling.py | 58 ++++++++++++++++++- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/openhands/agenthub/codeact_agent/function_calling.py b/openhands/agenthub/codeact_agent/function_calling.py index 6232e0300b73..4ec33a59f83f 100644 --- a/openhands/agenthub/codeact_agent/function_calling.py +++ b/openhands/agenthub/codeact_agent/function_calling.py @@ -12,7 +12,10 @@ ModelResponse, ) -from openhands.core.exceptions import FunctionCallNotExistsError +from openhands.core.exceptions import ( + FunctionCallNotExistsError, + FunctionCallValidationError, +) from openhands.core.logger import openhands_logger as logger from openhands.events.action import ( Action, @@ -494,14 +497,19 @@ def response_to_actions(response: ModelResponse) -> list[Action]: f'Failed to parse tool call arguments: {tool_call.function.arguments}' ) from e if tool_call.function.name == 'execute_bash': - # this is an LLM error: add empty command to avoid breaking the tool call if 'command' not in arguments: - arguments['command'] = '' + raise FunctionCallValidationError( + f'Missing required argument "command" in tool call {tool_call.function.name}' + ) # convert is_input to boolean if 'is_input' in arguments: arguments['is_input'] = arguments['is_input'] == 'true' action = CmdRunAction(**arguments) elif tool_call.function.name == 'execute_ipython_cell': + if 'code' not in arguments: + raise FunctionCallValidationError( + f'Missing required argument "code" in tool call {tool_call.function.name}' + ) action = IPythonRunCellAction(**arguments) elif tool_call.function.name == 'delegate_to_browsing_agent': action = AgentDelegateAction( @@ -511,8 +519,44 @@ def response_to_actions(response: ModelResponse) -> list[Action]: elif tool_call.function.name == 'finish': action = AgentFinishAction() elif tool_call.function.name == 'edit_file': + if 'path' not in arguments: + raise FunctionCallValidationError( + f'Missing required argument "path" in tool call {tool_call.function.name}' + ) + if 'content' not in arguments: + raise FunctionCallValidationError( + f'Missing required argument "content" in tool call {tool_call.function.name}' + ) action = FileEditAction(**arguments) elif tool_call.function.name == 'str_replace_editor': + if 'command' not in arguments: + raise FunctionCallValidationError( + f'Missing required argument "command" in tool call {tool_call.function.name}' + ) + if 'path' not in arguments: + raise FunctionCallValidationError( + f'Missing required argument "path" in tool call {tool_call.function.name}' + ) + # Check additional required arguments based on command + if arguments['command'] == 'create' and 'file_text' not in arguments: + raise FunctionCallValidationError( + f'Missing required argument "file_text" for command "create" in tool call {tool_call.function.name}' + ) + if arguments['command'] == 'str_replace': + if 'old_str' not in arguments: + raise FunctionCallValidationError( + f'Missing required argument "old_str" for command "str_replace" in tool call {tool_call.function.name}' + ) + if arguments['command'] == 'insert': + if 'new_str' not in arguments: + raise FunctionCallValidationError( + f'Missing required argument "new_str" for command "insert" in tool call {tool_call.function.name}' + ) + if 'insert_line' not in arguments: + raise FunctionCallValidationError( + f'Missing required argument "insert_line" for command "insert" in tool call {tool_call.function.name}' + ) + # We implement this in agent_skills, which can be used via Jupyter # convert tool_call.function.arguments to kwargs that can be passed to file_editor code = f'print(file_editor(**{arguments}))' @@ -534,8 +578,16 @@ def response_to_actions(response: ModelResponse) -> list[Action]: impl_source=FileEditSource.OH_ACI, ) elif tool_call.function.name == 'browser': + if 'code' not in arguments: + raise FunctionCallValidationError( + f'Missing required argument "code" in tool call {tool_call.function.name}' + ) action = BrowseInteractiveAction(browser_actions=arguments['code']) elif tool_call.function.name == 'web_read': + if 'url' not in arguments: + raise FunctionCallValidationError( + f'Missing required argument "url" in tool call {tool_call.function.name}' + ) action = BrowseURLAction(url=arguments['url']) else: raise FunctionCallNotExistsError( From 663138650128792cfd9e896452b2c93949b8d135 Mon Sep 17 00:00:00 2001 From: openhands Date: Fri, 24 Jan 2025 22:13:45 +0000 Subject: [PATCH 2/5] Use explicit kwargs instead of **arguments for better type safety --- .../agenthub/codeact_agent/function_calling.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/openhands/agenthub/codeact_agent/function_calling.py b/openhands/agenthub/codeact_agent/function_calling.py index 4ec33a59f83f..f3564d82200f 100644 --- a/openhands/agenthub/codeact_agent/function_calling.py +++ b/openhands/agenthub/codeact_agent/function_calling.py @@ -502,15 +502,14 @@ def response_to_actions(response: ModelResponse) -> list[Action]: f'Missing required argument "command" in tool call {tool_call.function.name}' ) # convert is_input to boolean - if 'is_input' in arguments: - arguments['is_input'] = arguments['is_input'] == 'true' - action = CmdRunAction(**arguments) + is_input = arguments.get('is_input', 'false') == 'true' + action = CmdRunAction(command=arguments['command'], is_input=is_input) elif tool_call.function.name == 'execute_ipython_cell': if 'code' not in arguments: raise FunctionCallValidationError( f'Missing required argument "code" in tool call {tool_call.function.name}' ) - action = IPythonRunCellAction(**arguments) + action = IPythonRunCellAction(code=arguments['code']) elif tool_call.function.name == 'delegate_to_browsing_agent': action = AgentDelegateAction( agent='BrowsingAgent', @@ -527,7 +526,12 @@ def response_to_actions(response: ModelResponse) -> list[Action]: raise FunctionCallValidationError( f'Missing required argument "content" in tool call {tool_call.function.name}' ) - action = FileEditAction(**arguments) + action = FileEditAction( + path=arguments['path'], + content=arguments['content'], + start=arguments.get('start', 1), + end=arguments.get('end', -1), + ) elif tool_call.function.name == 'str_replace_editor': if 'command' not in arguments: raise FunctionCallValidationError( From 85c45901770b88ee1e790eeaf584a8be8fb5cba1 Mon Sep 17 00:00:00 2001 From: openhands Date: Fri, 24 Jan 2025 22:19:26 +0000 Subject: [PATCH 3/5] Simplify file editor validation to only check required arguments --- .../codeact_agent/function_calling.py | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/openhands/agenthub/codeact_agent/function_calling.py b/openhands/agenthub/codeact_agent/function_calling.py index f3564d82200f..196457db0f6e 100644 --- a/openhands/agenthub/codeact_agent/function_calling.py +++ b/openhands/agenthub/codeact_agent/function_calling.py @@ -541,25 +541,6 @@ def response_to_actions(response: ModelResponse) -> list[Action]: raise FunctionCallValidationError( f'Missing required argument "path" in tool call {tool_call.function.name}' ) - # Check additional required arguments based on command - if arguments['command'] == 'create' and 'file_text' not in arguments: - raise FunctionCallValidationError( - f'Missing required argument "file_text" for command "create" in tool call {tool_call.function.name}' - ) - if arguments['command'] == 'str_replace': - if 'old_str' not in arguments: - raise FunctionCallValidationError( - f'Missing required argument "old_str" for command "str_replace" in tool call {tool_call.function.name}' - ) - if arguments['command'] == 'insert': - if 'new_str' not in arguments: - raise FunctionCallValidationError( - f'Missing required argument "new_str" for command "insert" in tool call {tool_call.function.name}' - ) - if 'insert_line' not in arguments: - raise FunctionCallValidationError( - f'Missing required argument "insert_line" for command "insert" in tool call {tool_call.function.name}' - ) # We implement this in agent_skills, which can be used via Jupyter # convert tool_call.function.arguments to kwargs that can be passed to file_editor From 7907d763b7eeae04375ba2c5a9761293279827c7 Mon Sep 17 00:00:00 2001 From: openhands Date: Fri, 24 Jan 2025 22:23:44 +0000 Subject: [PATCH 4/5] Add unit tests for function calling error handling --- tests/unit/test_function_calling.py | 226 ++++++++++++++++++++++++++++ 1 file changed, 226 insertions(+) create mode 100644 tests/unit/test_function_calling.py diff --git a/tests/unit/test_function_calling.py b/tests/unit/test_function_calling.py new file mode 100644 index 000000000000..2e00d6c75ee5 --- /dev/null +++ b/tests/unit/test_function_calling.py @@ -0,0 +1,226 @@ +"""Test function calling module.""" + +import json +import pytest +from unittest.mock import Mock + +from litellm import ModelResponse + +from openhands.agenthub.codeact_agent.function_calling import response_to_actions +from openhands.core.exceptions import FunctionCallValidationError +from openhands.events.action import ( + CmdRunAction, + IPythonRunCellAction, + FileEditAction, + FileReadAction, + BrowseInteractiveAction, + BrowseURLAction, +) +from openhands.events.event import FileEditSource, FileReadSource + + +def create_mock_response(function_name: str, arguments: dict) -> ModelResponse: + """Helper function to create a mock response with a tool call.""" + return ModelResponse( + id="mock-id", + choices=[ + Mock( + message=Mock( + tool_calls=[ + Mock( + function=Mock( + name=function_name, + arguments=json.dumps(arguments) + ), + id="mock-tool-call-id" + ) + ], + content=None + ) + ) + ] + ) + + +def test_execute_bash_valid(): + """Test execute_bash with valid arguments.""" + response = create_mock_response("execute_bash", {"command": "ls", "is_input": "false"}) + actions = response_to_actions(response) + assert len(actions) == 1 + assert isinstance(actions[0], CmdRunAction) + assert actions[0].command == "ls" + assert actions[0].is_input is False + + +def test_execute_bash_missing_command(): + """Test execute_bash with missing command argument.""" + response = create_mock_response("execute_bash", {"is_input": "false"}) + with pytest.raises(FunctionCallValidationError) as exc_info: + response_to_actions(response) + assert 'Missing required argument "command"' in str(exc_info.value) + + +def test_execute_ipython_cell_valid(): + """Test execute_ipython_cell with valid arguments.""" + response = create_mock_response("execute_ipython_cell", {"code": "print('hello')"}) + actions = response_to_actions(response) + assert len(actions) == 1 + assert isinstance(actions[0], IPythonRunCellAction) + assert actions[0].code == "print('hello')" + + +def test_execute_ipython_cell_missing_code(): + """Test execute_ipython_cell with missing code argument.""" + response = create_mock_response("execute_ipython_cell", {}) + with pytest.raises(FunctionCallValidationError) as exc_info: + response_to_actions(response) + assert 'Missing required argument "code"' in str(exc_info.value) + + +def test_edit_file_valid(): + """Test edit_file with valid arguments.""" + response = create_mock_response( + "edit_file", + { + "path": "/path/to/file", + "content": "file content", + "start": 1, + "end": 10 + } + ) + actions = response_to_actions(response) + assert len(actions) == 1 + assert isinstance(actions[0], FileEditAction) + assert actions[0].path == "/path/to/file" + assert actions[0].content == "file content" + assert actions[0].start == 1 + assert actions[0].end == 10 + + +def test_edit_file_missing_required(): + """Test edit_file with missing required arguments.""" + # Missing path + response = create_mock_response("edit_file", {"content": "content"}) + with pytest.raises(FunctionCallValidationError) as exc_info: + response_to_actions(response) + assert 'Missing required argument "path"' in str(exc_info.value) + + # Missing content + response = create_mock_response("edit_file", {"path": "/path/to/file"}) + with pytest.raises(FunctionCallValidationError) as exc_info: + response_to_actions(response) + assert 'Missing required argument "content"' in str(exc_info.value) + + +def test_str_replace_editor_valid(): + """Test str_replace_editor with valid arguments.""" + # Test view command + response = create_mock_response( + "str_replace_editor", + { + "command": "view", + "path": "/path/to/file" + } + ) + actions = response_to_actions(response) + assert len(actions) == 1 + assert isinstance(actions[0], FileReadAction) + assert actions[0].path == "/path/to/file" + assert actions[0].impl_source == FileReadSource.OH_ACI + + # Test other commands + response = create_mock_response( + "str_replace_editor", + { + "command": "str_replace", + "path": "/path/to/file", + "old_str": "old", + "new_str": "new" + } + ) + actions = response_to_actions(response) + assert len(actions) == 1 + assert isinstance(actions[0], FileEditAction) + assert actions[0].path == "/path/to/file" + assert actions[0].impl_source == FileEditSource.OH_ACI + + +def test_str_replace_editor_missing_required(): + """Test str_replace_editor with missing required arguments.""" + # Missing command + response = create_mock_response("str_replace_editor", {"path": "/path/to/file"}) + with pytest.raises(FunctionCallValidationError) as exc_info: + response_to_actions(response) + assert 'Missing required argument "command"' in str(exc_info.value) + + # Missing path + response = create_mock_response("str_replace_editor", {"command": "view"}) + with pytest.raises(FunctionCallValidationError) as exc_info: + response_to_actions(response) + assert 'Missing required argument "path"' in str(exc_info.value) + + +def test_browser_valid(): + """Test browser with valid arguments.""" + response = create_mock_response( + "browser", + {"code": "click('button-1')"} + ) + actions = response_to_actions(response) + assert len(actions) == 1 + assert isinstance(actions[0], BrowseInteractiveAction) + assert actions[0].browser_actions == "click('button-1')" + + +def test_browser_missing_code(): + """Test browser with missing code argument.""" + response = create_mock_response("browser", {}) + with pytest.raises(FunctionCallValidationError) as exc_info: + response_to_actions(response) + assert 'Missing required argument "code"' in str(exc_info.value) + + +def test_web_read_valid(): + """Test web_read with valid arguments.""" + response = create_mock_response( + "web_read", + {"url": "https://example.com"} + ) + actions = response_to_actions(response) + assert len(actions) == 1 + assert isinstance(actions[0], BrowseURLAction) + assert actions[0].url == "https://example.com" + + +def test_web_read_missing_url(): + """Test web_read with missing url argument.""" + response = create_mock_response("web_read", {}) + with pytest.raises(FunctionCallValidationError) as exc_info: + response_to_actions(response) + assert 'Missing required argument "url"' in str(exc_info.value) + + +def test_invalid_json_arguments(): + """Test handling of invalid JSON in arguments.""" + response = ModelResponse( + id="mock-id", + choices=[ + Mock( + message=Mock( + tool_calls=[ + Mock( + function=Mock( + name="execute_bash", + arguments="invalid json" + ), + id="mock-tool-call-id" + ) + ], + content=None + ) + ) + ] + ) + with pytest.raises(RuntimeError) as exc_info: + response_to_actions(response) + assert "Failed to parse tool call arguments" in str(exc_info.value) \ No newline at end of file From 9a90439163c6d8720d8ae77ebfa60f1aa2d5293f Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 27 Jan 2025 17:18:43 +0000 Subject: [PATCH 5/5] Fix test_function_calling.py to use correct litellm types --- tests/unit/test_function_calling.py | 150 +++++++++++++--------------- 1 file changed, 72 insertions(+), 78 deletions(-) diff --git a/tests/unit/test_function_calling.py b/tests/unit/test_function_calling.py index 2e00d6c75ee5..57170bd08f93 100644 --- a/tests/unit/test_function_calling.py +++ b/tests/unit/test_function_calling.py @@ -1,20 +1,19 @@ """Test function calling module.""" import json -import pytest -from unittest.mock import Mock +import pytest from litellm import ModelResponse from openhands.agenthub.codeact_agent.function_calling import response_to_actions from openhands.core.exceptions import FunctionCallValidationError from openhands.events.action import ( + BrowseInteractiveAction, + BrowseURLAction, CmdRunAction, - IPythonRunCellAction, FileEditAction, FileReadAction, - BrowseInteractiveAction, - BrowseURLAction, + IPythonRunCellAction, ) from openhands.events.event import FileEditSource, FileReadSource @@ -22,39 +21,45 @@ def create_mock_response(function_name: str, arguments: dict) -> ModelResponse: """Helper function to create a mock response with a tool call.""" return ModelResponse( - id="mock-id", + id='mock-id', choices=[ - Mock( - message=Mock( - tool_calls=[ - Mock( - function=Mock( - name=function_name, - arguments=json.dumps(arguments) - ), - id="mock-tool-call-id" - ) + { + 'message': { + 'tool_calls': [ + { + 'function': { + 'name': function_name, + 'arguments': json.dumps(arguments), + }, + 'id': 'mock-tool-call-id', + 'type': 'function', + } ], - content=None - ) - ) - ] + 'content': None, + 'role': 'assistant', + }, + 'index': 0, + 'finish_reason': 'tool_calls', + } + ], ) def test_execute_bash_valid(): """Test execute_bash with valid arguments.""" - response = create_mock_response("execute_bash", {"command": "ls", "is_input": "false"}) + response = create_mock_response( + 'execute_bash', {'command': 'ls', 'is_input': 'false'} + ) actions = response_to_actions(response) assert len(actions) == 1 assert isinstance(actions[0], CmdRunAction) - assert actions[0].command == "ls" + assert actions[0].command == 'ls' assert actions[0].is_input is False def test_execute_bash_missing_command(): """Test execute_bash with missing command argument.""" - response = create_mock_response("execute_bash", {"is_input": "false"}) + response = create_mock_response('execute_bash', {'is_input': 'false'}) with pytest.raises(FunctionCallValidationError) as exc_info: response_to_actions(response) assert 'Missing required argument "command"' in str(exc_info.value) @@ -62,7 +67,7 @@ def test_execute_bash_missing_command(): def test_execute_ipython_cell_valid(): """Test execute_ipython_cell with valid arguments.""" - response = create_mock_response("execute_ipython_cell", {"code": "print('hello')"}) + response = create_mock_response('execute_ipython_cell', {'code': "print('hello')"}) actions = response_to_actions(response) assert len(actions) == 1 assert isinstance(actions[0], IPythonRunCellAction) @@ -71,7 +76,7 @@ def test_execute_ipython_cell_valid(): def test_execute_ipython_cell_missing_code(): """Test execute_ipython_cell with missing code argument.""" - response = create_mock_response("execute_ipython_cell", {}) + response = create_mock_response('execute_ipython_cell', {}) with pytest.raises(FunctionCallValidationError) as exc_info: response_to_actions(response) assert 'Missing required argument "code"' in str(exc_info.value) @@ -80,19 +85,14 @@ def test_execute_ipython_cell_missing_code(): def test_edit_file_valid(): """Test edit_file with valid arguments.""" response = create_mock_response( - "edit_file", - { - "path": "/path/to/file", - "content": "file content", - "start": 1, - "end": 10 - } + 'edit_file', + {'path': '/path/to/file', 'content': 'file content', 'start': 1, 'end': 10}, ) actions = response_to_actions(response) assert len(actions) == 1 assert isinstance(actions[0], FileEditAction) - assert actions[0].path == "/path/to/file" - assert actions[0].content == "file content" + assert actions[0].path == '/path/to/file' + assert actions[0].content == 'file content' assert actions[0].start == 1 assert actions[0].end == 10 @@ -100,13 +100,13 @@ def test_edit_file_valid(): def test_edit_file_missing_required(): """Test edit_file with missing required arguments.""" # Missing path - response = create_mock_response("edit_file", {"content": "content"}) + response = create_mock_response('edit_file', {'content': 'content'}) with pytest.raises(FunctionCallValidationError) as exc_info: response_to_actions(response) assert 'Missing required argument "path"' in str(exc_info.value) # Missing content - response = create_mock_response("edit_file", {"path": "/path/to/file"}) + response = create_mock_response('edit_file', {'path': '/path/to/file'}) with pytest.raises(FunctionCallValidationError) as exc_info: response_to_actions(response) assert 'Missing required argument "content"' in str(exc_info.value) @@ -116,45 +116,41 @@ def test_str_replace_editor_valid(): """Test str_replace_editor with valid arguments.""" # Test view command response = create_mock_response( - "str_replace_editor", - { - "command": "view", - "path": "/path/to/file" - } + 'str_replace_editor', {'command': 'view', 'path': '/path/to/file'} ) actions = response_to_actions(response) assert len(actions) == 1 assert isinstance(actions[0], FileReadAction) - assert actions[0].path == "/path/to/file" + assert actions[0].path == '/path/to/file' assert actions[0].impl_source == FileReadSource.OH_ACI # Test other commands response = create_mock_response( - "str_replace_editor", + 'str_replace_editor', { - "command": "str_replace", - "path": "/path/to/file", - "old_str": "old", - "new_str": "new" - } + 'command': 'str_replace', + 'path': '/path/to/file', + 'old_str': 'old', + 'new_str': 'new', + }, ) actions = response_to_actions(response) assert len(actions) == 1 assert isinstance(actions[0], FileEditAction) - assert actions[0].path == "/path/to/file" + assert actions[0].path == '/path/to/file' assert actions[0].impl_source == FileEditSource.OH_ACI def test_str_replace_editor_missing_required(): """Test str_replace_editor with missing required arguments.""" # Missing command - response = create_mock_response("str_replace_editor", {"path": "/path/to/file"}) + response = create_mock_response('str_replace_editor', {'path': '/path/to/file'}) with pytest.raises(FunctionCallValidationError) as exc_info: response_to_actions(response) assert 'Missing required argument "command"' in str(exc_info.value) # Missing path - response = create_mock_response("str_replace_editor", {"command": "view"}) + response = create_mock_response('str_replace_editor', {'command': 'view'}) with pytest.raises(FunctionCallValidationError) as exc_info: response_to_actions(response) assert 'Missing required argument "path"' in str(exc_info.value) @@ -162,10 +158,7 @@ def test_str_replace_editor_missing_required(): def test_browser_valid(): """Test browser with valid arguments.""" - response = create_mock_response( - "browser", - {"code": "click('button-1')"} - ) + response = create_mock_response('browser', {'code': "click('button-1')"}) actions = response_to_actions(response) assert len(actions) == 1 assert isinstance(actions[0], BrowseInteractiveAction) @@ -174,7 +167,7 @@ def test_browser_valid(): def test_browser_missing_code(): """Test browser with missing code argument.""" - response = create_mock_response("browser", {}) + response = create_mock_response('browser', {}) with pytest.raises(FunctionCallValidationError) as exc_info: response_to_actions(response) assert 'Missing required argument "code"' in str(exc_info.value) @@ -182,19 +175,16 @@ def test_browser_missing_code(): def test_web_read_valid(): """Test web_read with valid arguments.""" - response = create_mock_response( - "web_read", - {"url": "https://example.com"} - ) + response = create_mock_response('web_read', {'url': 'https://example.com'}) actions = response_to_actions(response) assert len(actions) == 1 assert isinstance(actions[0], BrowseURLAction) - assert actions[0].url == "https://example.com" + assert actions[0].url == 'https://example.com' def test_web_read_missing_url(): """Test web_read with missing url argument.""" - response = create_mock_response("web_read", {}) + response = create_mock_response('web_read', {}) with pytest.raises(FunctionCallValidationError) as exc_info: response_to_actions(response) assert 'Missing required argument "url"' in str(exc_info.value) @@ -203,24 +193,28 @@ def test_web_read_missing_url(): def test_invalid_json_arguments(): """Test handling of invalid JSON in arguments.""" response = ModelResponse( - id="mock-id", + id='mock-id', choices=[ - Mock( - message=Mock( - tool_calls=[ - Mock( - function=Mock( - name="execute_bash", - arguments="invalid json" - ), - id="mock-tool-call-id" - ) + { + 'message': { + 'tool_calls': [ + { + 'function': { + 'name': 'execute_bash', + 'arguments': 'invalid json', + }, + 'id': 'mock-tool-call-id', + 'type': 'function', + } ], - content=None - ) - ) - ] + 'content': None, + 'role': 'assistant', + }, + 'index': 0, + 'finish_reason': 'tool_calls', + } + ], ) with pytest.raises(RuntimeError) as exc_info: response_to_actions(response) - assert "Failed to parse tool call arguments" in str(exc_info.value) \ No newline at end of file + assert 'Failed to parse tool call arguments' in str(exc_info.value)