From f1b9b3bf3db7138981412d4de5b076bf45f9ecdf Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 22 Apr 2025 14:10:24 +0000 Subject: [PATCH 1/6] Fix parsing of tools with missing docstring descriptions --- src/serena/mcp.py | 5 +++- test/serena/test_mcp.py | 62 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/src/serena/mcp.py b/src/serena/mcp.py index ef8b04c..6bbfc5f 100644 --- a/src/serena/mcp.py +++ b/src/serena/mcp.py @@ -61,7 +61,10 @@ def make_tool( # Mount the tool description as a combination of the docstring description and # the return value description, if it exists. - func_doc = f"{docstring.description.strip().strip('.')}." + if docstring.description: + func_doc = f"{docstring.description.strip().strip('.')}." + else: + func_doc = "No description available." if (docstring.returns) and (docstring_returns := docstring.returns.description): func_doc = f"{func_doc} Returns {docstring_returns.strip().strip('.')}." diff --git a/test/serena/test_mcp.py b/test/serena/test_mcp.py index 6e6a8ae..0e90e76 100644 --- a/test/serena/test_mcp.py +++ b/test/serena/test_mcp.py @@ -198,3 +198,65 @@ def test_make_tool_missing_apply() -> None: with pytest.raises(AttributeError): make_tool(tool) + + +def test_make_tool_missing_description() -> None: + """Test make_tool with a function that has no description in the docstring.""" + + class NoDescriptionTool(BaseMockTool): + def apply(self, param: str) -> str: + """ + :param param: The parameter + :return: A result + """ + return f"Result: {param}" + + def apply_ex(self, *args, **kwargs) -> str: + return self.apply(**kwargs) + + tool = NoDescriptionTool() + mcp_tool = make_tool(tool) + + assert mcp_tool.name == "no_description" + assert mcp_tool.description == "No description available. Returns A result." + assert mcp_tool.parameters["properties"]["param"]["description"] == "The parameter." + + +def test_make_tool_all_tools() -> None: + """Test that make_tool works for all tools in the codebase.""" + from serena.agent import iter_tool_classes, Tool + + # Create a mock agent for tool initialization + class MockAgent: + def __init__(self): + self.project_config = None + self.serena_config = None + + # Get all tool classes + tool_classes = list(iter_tool_classes()) + + # Make sure we found some tools + assert len(tool_classes) > 0 + + # Test each tool class + for tool_class in tool_classes: + try: + # Skip abstract base classes that can't be instantiated + if tool_class.__name__ == "Tool" or getattr(tool_class, "__abstractmethods__", set()): + continue + + # Create an instance of the tool + tool_instance = tool_class(MockAgent()) + + # Try to create an MCP tool from it + mcp_tool = make_tool(tool_instance) + + # Basic validation + assert isinstance(mcp_tool, MCPTool) + assert mcp_tool.name == tool_class.get_name() + + # The description should be a string (either from docstring or default) + assert isinstance(mcp_tool.description, str) + + except Exception as e: + pytest.fail(f"Failed to create MCP tool for {tool_class.__name__}: {e}") From 6bb00745879b9b01d49d12963231bbb23c96bd93 Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 22 Apr 2025 14:12:11 +0000 Subject: [PATCH 2/6] Fix test_make_tool_all_tools to skip test mock classes --- test/serena/test_mcp.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/serena/test_mcp.py b/test/serena/test_mcp.py index 0e90e76..31182f0 100644 --- a/test/serena/test_mcp.py +++ b/test/serena/test_mcp.py @@ -245,6 +245,11 @@ def test_make_tool_all_tools() -> None: if tool_class.__name__ == "Tool" or getattr(tool_class, "__abstractmethods__", set()): continue + # Skip test mock classes that don't properly implement __init__ + if tool_class.__name__ in ["BaseMockTool", "BadTool", "NoParamsTool", "NoReturnTool", + "MissingParamTool", "ComplexDocTool", "FormatTool", "NoDescriptionTool"]: + continue + # Create an instance of the tool tool_instance = tool_class(MockAgent()) From 5cc4086c4ff3add09bd071c3f473b4c892c65d3b Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 22 Apr 2025 14:14:11 +0000 Subject: [PATCH 3/6] Add BasicTool to the list of classes to skip in test_make_tool_all_tools --- test/serena/test_mcp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/serena/test_mcp.py b/test/serena/test_mcp.py index 31182f0..5998d23 100644 --- a/test/serena/test_mcp.py +++ b/test/serena/test_mcp.py @@ -246,7 +246,7 @@ def test_make_tool_all_tools() -> None: continue # Skip test mock classes that don't properly implement __init__ - if tool_class.__name__ in ["BaseMockTool", "BadTool", "NoParamsTool", "NoReturnTool", + if tool_class.__name__ in ["BaseMockTool", "BasicTool", "BadTool", "NoParamsTool", "NoReturnTool", "MissingParamTool", "ComplexDocTool", "FormatTool", "NoDescriptionTool"]: continue From 2737dd2b3f6253c0921ee69391b9bf3e6c9590d0 Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 22 Apr 2025 14:18:30 +0000 Subject: [PATCH 4/6] Use empty string instead of 'No description available' for missing descriptions --- src/serena/mcp.py | 6 +++-- test/serena/test_mcp.py | 2 +- test_make_tool.py | 59 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 test_make_tool.py diff --git a/src/serena/mcp.py b/src/serena/mcp.py index 6bbfc5f..f52e529 100644 --- a/src/serena/mcp.py +++ b/src/serena/mcp.py @@ -64,9 +64,11 @@ def make_tool( if docstring.description: func_doc = f"{docstring.description.strip().strip('.')}." else: - func_doc = "No description available." + func_doc = "" if (docstring.returns) and (docstring_returns := docstring.returns.description): - func_doc = f"{func_doc} Returns {docstring_returns.strip().strip('.')}." + # Only add a space before "Returns" if func_doc is not empty + prefix = " " if func_doc else "" + func_doc = f"{func_doc}{prefix}Returns {docstring_returns.strip().strip('.')}." # Parse the parameter descriptions from the docstring and add pass its description # to the parameters schema. diff --git a/test/serena/test_mcp.py b/test/serena/test_mcp.py index 5998d23..454b7b8 100644 --- a/test/serena/test_mcp.py +++ b/test/serena/test_mcp.py @@ -218,7 +218,7 @@ def test_make_tool_missing_description() -> None: mcp_tool = make_tool(tool) assert mcp_tool.name == "no_description" - assert mcp_tool.description == "No description available. Returns A result." + assert mcp_tool.description == "Returns A result." assert mcp_tool.parameters["properties"]["param"]["description"] == "The parameter." diff --git a/test_make_tool.py b/test_make_tool.py new file mode 100644 index 0000000..2933bd5 --- /dev/null +++ b/test_make_tool.py @@ -0,0 +1,59 @@ +"""Simple test for the make_tool function.""" + +import docstring_parser +from typing import Any, Callable + + +def make_tool_simplified(func_doc: str) -> str: + """Simplified version of make_tool function to test our fix.""" + docstring = docstring_parser.parse(func_doc) + + # Mount the tool description as a combination of the docstring description and + # the return value description, if it exists. + if docstring.description: + func_doc = f"{docstring.description.strip().strip('.')}." + else: + func_doc = "" + if (docstring.returns) and (docstring_returns := docstring.returns.description): + # Only add a space before "Returns" if func_doc is not empty + prefix = " " if func_doc else "" + func_doc = f"{func_doc}{prefix}Returns {docstring_returns.strip().strip('.')}." + + return func_doc + + +def test_with_description(): + """Test with a normal docstring that has a description.""" + doc = """This is a test function. + + :param name: The person's name + :param age: The person's age + :return: A greeting message + """ + result = make_tool_simplified(doc) + assert result == "This is a test function. Returns A greeting message." + + +def test_without_description(): + """Test with a docstring that has no description.""" + doc = """ + :param name: The person's name + :param age: The person's age + :return: A greeting message + """ + result = make_tool_simplified(doc) + assert result == "Returns A greeting message." + + +def test_empty_docstring(): + """Test with an empty docstring.""" + doc = "" + result = make_tool_simplified(doc) + assert result == "" + + +if __name__ == "__main__": + test_with_description() + test_without_description() + test_empty_docstring() + print("All tests passed!") \ No newline at end of file From d9b90f26af1ce385a3ec1d87f1592e242a2f8bd1 Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 22 Apr 2025 15:03:16 +0000 Subject: [PATCH 5/6] Improve tests for make_tool function --- test/serena/test_mcp.py | 128 +++++++++++++++++++---------- test_make_tool.py => verify_fix.py | 20 ++++- 2 files changed, 103 insertions(+), 45 deletions(-) rename test_make_tool.py => verify_fix.py (74%) diff --git a/test/serena/test_mcp.py b/test/serena/test_mcp.py index 454b7b8..0a940d3 100644 --- a/test/serena/test_mcp.py +++ b/test/serena/test_mcp.py @@ -200,31 +200,94 @@ def test_make_tool_missing_apply() -> None: make_tool(tool) -def test_make_tool_missing_description() -> None: - """Test make_tool with a function that has no description in the docstring.""" +@pytest.mark.parametrize( + "docstring, expected_description", + [ + ( + """This is a test function. - class NoDescriptionTool(BaseMockTool): - def apply(self, param: str) -> str: + :param param: The parameter + :return: A result + """, + "This is a test function. Returns A result.", + ), + ( """ :param param: The parameter :return: A result + """, + "Returns A result.", + ), + ( """ + :param param: The parameter + """, + "", + ), + ("", ""), + ], +) +def test_make_tool_descriptions(docstring, expected_description) -> None: + """Test make_tool with various docstring formats.""" + + class TestTool(BaseMockTool): + def apply(self, param: str) -> str: return f"Result: {param}" def apply_ex(self, *args, **kwargs) -> str: return self.apply(**kwargs) - tool = NoDescriptionTool() + # Dynamically set the docstring + TestTool.apply.__doc__ = docstring + + tool = TestTool() mcp_tool = make_tool(tool) - assert mcp_tool.name == "no_description" - assert mcp_tool.description == "Returns A result." - assert mcp_tool.parameters["properties"]["param"]["description"] == "The parameter." + assert mcp_tool.name == "test" + assert mcp_tool.description == expected_description -def test_make_tool_all_tools() -> None: +def is_test_mock_class(tool_class: type) -> bool: + """Check if a class is a test mock class.""" + # Check if the class is defined in a test module + module_name = tool_class.__module__ + return ( + module_name.startswith(("test.", "tests.")) + or "test_" in module_name + or tool_class.__name__ + in [ + "BaseMockTool", + "BasicTool", + "BadTool", + "NoParamsTool", + "NoReturnTool", + "MissingParamTool", + "ComplexDocTool", + "FormatTool", + "NoDescriptionTool", + ] + ) + + +def get_real_tool_classes(): + """Get all non-test, non-abstract tool classes.""" + from serena.agent import iter_tool_classes + + for tool_class in iter_tool_classes(): + # Skip abstract base classes that can't be instantiated + if tool_class.__name__ == "Tool" or getattr(tool_class, "__abstractmethods__", set()): + continue + + # Skip test mock classes + if is_test_mock_class(tool_class): + continue + + yield tool_class + + +@pytest.mark.parametrize("tool_class", list(get_real_tool_classes())) +def test_make_tool_all_tools(tool_class) -> None: """Test that make_tool works for all tools in the codebase.""" - from serena.agent import iter_tool_classes, Tool # Create a mock agent for tool initialization class MockAgent: @@ -232,36 +295,15 @@ def test_make_tool_all_tools() -> None: self.project_config = None self.serena_config = None - # Get all tool classes - tool_classes = list(iter_tool_classes()) - - # Make sure we found some tools - assert len(tool_classes) > 0 - - # Test each tool class - for tool_class in tool_classes: - try: - # Skip abstract base classes that can't be instantiated - if tool_class.__name__ == "Tool" or getattr(tool_class, "__abstractmethods__", set()): - continue - - # Skip test mock classes that don't properly implement __init__ - if tool_class.__name__ in ["BaseMockTool", "BasicTool", "BadTool", "NoParamsTool", "NoReturnTool", - "MissingParamTool", "ComplexDocTool", "FormatTool", "NoDescriptionTool"]: - continue - - # Create an instance of the tool - tool_instance = tool_class(MockAgent()) - - # Try to create an MCP tool from it - mcp_tool = make_tool(tool_instance) - - # Basic validation - assert isinstance(mcp_tool, MCPTool) - assert mcp_tool.name == tool_class.get_name() - - # The description should be a string (either from docstring or default) - assert isinstance(mcp_tool.description, str) - - except Exception as e: - pytest.fail(f"Failed to create MCP tool for {tool_class.__name__}: {e}") + # Create an instance of the tool + tool_instance = tool_class(MockAgent()) + + # Try to create an MCP tool from it + mcp_tool = make_tool(tool_instance) + + # Basic validation + assert isinstance(mcp_tool, MCPTool) + assert mcp_tool.name == tool_class.get_name() + + # The description should be a string (either from docstring or default) + assert isinstance(mcp_tool.description, str) diff --git a/test_make_tool.py b/verify_fix.py similarity index 74% rename from test_make_tool.py rename to verify_fix.py index 2933bd5..d0a2833 100644 --- a/test_make_tool.py +++ b/verify_fix.py @@ -1,7 +1,7 @@ -"""Simple test for the make_tool function.""" +"""Simple script to verify our fix for the make_tool function.""" import docstring_parser -from typing import Any, Callable +from typing import Any, Optional def make_tool_simplified(func_doc: str) -> str: @@ -32,6 +32,7 @@ def test_with_description(): """ result = make_tool_simplified(doc) assert result == "This is a test function. Returns A greeting message." + print("✅ Test with description passed") def test_without_description(): @@ -43,6 +44,7 @@ def test_without_description(): """ result = make_tool_simplified(doc) assert result == "Returns A greeting message." + print("✅ Test without description passed") def test_empty_docstring(): @@ -50,10 +52,24 @@ def test_empty_docstring(): doc = "" result = make_tool_simplified(doc) assert result == "" + print("✅ Test with empty docstring passed") + + +def test_no_return(): + """Test with a docstring that has no return description.""" + doc = """This is a test function. + + :param name: The person's name + :param age: The person's age + """ + result = make_tool_simplified(doc) + assert result == "This is a test function." + print("✅ Test with no return passed") if __name__ == "__main__": test_with_description() test_without_description() test_empty_docstring() + test_no_return() print("All tests passed!") \ No newline at end of file From 4d969844c88c1893b263478ce5ef627c69068272 Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 22 Apr 2025 15:13:20 +0000 Subject: [PATCH 6/6] Update iter_tool_classes to only iterate over tools in the same module by default --- src/serena/agent.py | 14 ++++++-- test/serena/test_mcp.py | 2 +- verify_fix.py | 75 ----------------------------------------- 3 files changed, 13 insertions(+), 78 deletions(-) delete mode 100644 verify_fix.py diff --git a/src/serena/agent.py b/src/serena/agent.py index 80e8090..86e93a9 100644 --- a/src/serena/agent.py +++ b/src/serena/agent.py @@ -1370,8 +1370,18 @@ class InitialInstructionsTool(Tool): return self.agent.prompt_factory.create_system_prompt() -def iter_tool_classes() -> Generator[type[Tool], None, None]: - return iter_subclasses(Tool) +def iter_tool_classes(same_module_only: bool = True) -> Generator[type[Tool], None, None]: + """ + Iterate over all Tool subclasses. + + Args: + same_module_only: If True, only iterate over tools defined in the same module as the Tool class. + If False, iterate over all Tool subclasses. + """ + for tool_class in iter_subclasses(Tool): + if same_module_only and tool_class.__module__ != Tool.__module__: + continue + yield tool_class def print_tool_overview() -> None: diff --git a/test/serena/test_mcp.py b/test/serena/test_mcp.py index 0a940d3..bac38da 100644 --- a/test/serena/test_mcp.py +++ b/test/serena/test_mcp.py @@ -273,7 +273,7 @@ def get_real_tool_classes(): """Get all non-test, non-abstract tool classes.""" from serena.agent import iter_tool_classes - for tool_class in iter_tool_classes(): + for tool_class in iter_tool_classes(same_module_only=False): # Skip abstract base classes that can't be instantiated if tool_class.__name__ == "Tool" or getattr(tool_class, "__abstractmethods__", set()): continue diff --git a/verify_fix.py b/verify_fix.py deleted file mode 100644 index d0a2833..0000000 --- a/verify_fix.py +++ /dev/null @@ -1,75 +0,0 @@ -"""Simple script to verify our fix for the make_tool function.""" - -import docstring_parser -from typing import Any, Optional - - -def make_tool_simplified(func_doc: str) -> str: - """Simplified version of make_tool function to test our fix.""" - docstring = docstring_parser.parse(func_doc) - - # Mount the tool description as a combination of the docstring description and - # the return value description, if it exists. - if docstring.description: - func_doc = f"{docstring.description.strip().strip('.')}." - else: - func_doc = "" - if (docstring.returns) and (docstring_returns := docstring.returns.description): - # Only add a space before "Returns" if func_doc is not empty - prefix = " " if func_doc else "" - func_doc = f"{func_doc}{prefix}Returns {docstring_returns.strip().strip('.')}." - - return func_doc - - -def test_with_description(): - """Test with a normal docstring that has a description.""" - doc = """This is a test function. - - :param name: The person's name - :param age: The person's age - :return: A greeting message - """ - result = make_tool_simplified(doc) - assert result == "This is a test function. Returns A greeting message." - print("✅ Test with description passed") - - -def test_without_description(): - """Test with a docstring that has no description.""" - doc = """ - :param name: The person's name - :param age: The person's age - :return: A greeting message - """ - result = make_tool_simplified(doc) - assert result == "Returns A greeting message." - print("✅ Test without description passed") - - -def test_empty_docstring(): - """Test with an empty docstring.""" - doc = "" - result = make_tool_simplified(doc) - assert result == "" - print("✅ Test with empty docstring passed") - - -def test_no_return(): - """Test with a docstring that has no return description.""" - doc = """This is a test function. - - :param name: The person's name - :param age: The person's age - """ - result = make_tool_simplified(doc) - assert result == "This is a test function." - print("✅ Test with no return passed") - - -if __name__ == "__main__": - test_with_description() - test_without_description() - test_empty_docstring() - test_no_return() - print("All tests passed!") \ No newline at end of file