From cdc0d178f8f5488c6705e34446f336d2ea15beb3 Mon Sep 17 00:00:00 2001 From: MischaPanch Date: Sat, 9 Aug 2025 16:15:51 +0200 Subject: [PATCH] Remove union type annotations from tools. Codex can't deal with them, I suspect ChatGPT will also have problems. This leads to slightly less expressive python code, but it isn't a large problem and is worth the extension in usability for serena. --- src/serena/mcp.py | 117 +++++++++++++++++++++++++++++-- src/serena/tools/file_tools.py | 12 ++-- src/serena/tools/symbol_tools.py | 14 ++-- 3 files changed, 127 insertions(+), 16 deletions(-) diff --git a/src/serena/mcp.py b/src/serena/mcp.py index fb42cbe9..5de1e11a 100644 --- a/src/serena/mcp.py +++ b/src/serena/mcp.py @@ -6,6 +6,7 @@ import sys from abc import abstractmethod from collections.abc import AsyncIterator, Iterator, Sequence from contextlib import asynccontextmanager +from copy import deepcopy from dataclasses import dataclass from typing import Any, Literal, cast @@ -57,12 +58,119 @@ class SerenaMCPFactory: self.project = project @staticmethod - def make_mcp_tool(tool: Tool) -> MCPTool: + def _sanitize_for_openai_tools(schema: dict) -> dict: + """ + This method was written by GPT-5, I have not reviewed it in detail. + Only called when `openai_tool_compatible` is True. + + Make a Pydantic/JSON Schema object compatible with OpenAI tool schema. + - 'integer' -> 'number' (+ multipleOf: 1) + - remove 'null' from union type arrays + - coerce integer-only enums to number + - best-effort simplify oneOf/anyOf when they only differ by integer/number + """ + s = deepcopy(schema) + + def walk(node): + if not isinstance(node, dict): + # lists get handled by parent calls + return node + + # ---- handle type ---- + t = node.get("type") + if isinstance(t, str): + if t == "integer": + node["type"] = "number" + # preserve existing multipleOf but ensure it's integer-like + if "multipleOf" not in node: + node["multipleOf"] = 1 + elif isinstance(t, list): + # remove 'null' (OpenAI tools don't support nullables) + t2 = [x if x != "integer" else "number" for x in t if x != "null"] + if not t2: + # fall back to object if it somehow becomes empty + t2 = ["object"] + node["type"] = t2[0] if len(t2) == 1 else t2 + if "integer" in t or "number" in t2: + # if integers were present, keep integer-like restriction + node.setdefault("multipleOf", 1) + + # ---- enums of integers -> number ---- + if "enum" in node and isinstance(node["enum"], list): + vals = node["enum"] + if vals and all(isinstance(v, int) for v in vals): + node.setdefault("type", "number") + # keep them as ints; JSON 'number' covers ints + node.setdefault("multipleOf", 1) + + # ---- simplify anyOf/oneOf if they only differ by integer/number ---- + for key in ("oneOf", "anyOf"): + if key in node and isinstance(node[key], list): + simplified = [] + changed = False + for sub in node[key]: + sub = walk(sub) # recurse + simplified.append(sub) + # If all subs are the same after integer→number, collapse + try: + import json + + canon = [json.dumps(x, sort_keys=True) for x in simplified] + if len(set(canon)) == 1: + # copy the single schema up + only = simplified[0] + node.pop(key, None) + for k, v in only.items(): + if k not in node: + node[k] = v + changed = True + except Exception: + pass + if not changed: + node[key] = simplified + + # ---- recurse into known schema containers ---- + for child_key in ("properties", "patternProperties", "definitions", "$defs"): + if child_key in node and isinstance(node[child_key], dict): + for k, v in list(node[child_key].items()): + node[child_key][k] = walk(v) + + # arrays/items + if "items" in node: + node["items"] = walk(node["items"]) + + # allOf/if/then/else - pass through with integer→number conversions applied inside + for key in ("allOf",): + if key in node and isinstance(node[key], list): + node[key] = [walk(x) for x in node[key]] + + if "if" in node: + node["if"] = walk(node["if"]) + if "then" in node: + node["then"] = walk(node["then"]) + if "else" in node: + node["else"] = walk(node["else"]) + + return node + + return walk(s) + + @staticmethod + def make_mcp_tool(tool: Tool, openai_tool_compatible: bool = True) -> MCPTool: + """ + Create an MCP tool from a Serena Tool instance. + + :param tool: The Serena Tool instance to convert. + :param openai_tool_compatible: whether to process the tool schema to be compatible with OpenAI tools + (doesn't accept integer, needs number instead, etc.). This allows using Serena MCP within codex. + """ func_name = tool.get_name() func_doc = tool.get_apply_docstring() or "" func_arg_metadata = tool.get_apply_fn_metadata() is_async = False parameters = func_arg_metadata.arg_model.model_json_schema() + if openai_tool_compatible: + parameters = SerenaMCPFactory._sanitize_for_openai_tools(parameters) docstring = docstring_parser.parse(func_doc) @@ -113,12 +221,12 @@ class SerenaMCPFactory: pass # noinspection PyProtectedMember - def _set_mcp_tools(self, mcp: FastMCP) -> None: + def _set_mcp_tools(self, mcp: FastMCP, openai_tool_compatible: bool = False) -> None: """Update the tools in the MCP server""" if mcp is not None: mcp._tool_manager._tools = {} for tool in self._iter_tools(): - mcp_tool = self.make_mcp_tool(tool) + mcp_tool = self.make_mcp_tool(tool, openai_tool_compatible=openai_tool_compatible) mcp._tool_manager._tools[tool.get_name()] = mcp_tool log.info(f"Starting MCP server with {len(mcp._tool_manager._tools)} tools: {list(mcp._tool_manager._tools.keys())}") @@ -225,6 +333,7 @@ class SerenaMCPFactorySingleProcess(SerenaMCPFactory): @asynccontextmanager async def server_lifespan(self, mcp_server: FastMCP) -> AsyncIterator[None]: - self._set_mcp_tools(mcp_server) + openai_tool_compatible = self.context.name in ["chatgpt", "codex"] + self._set_mcp_tools(mcp_server, openai_tool_compatible=openai_tool_compatible) log.info("MCP server lifetime setup complete") yield diff --git a/src/serena/tools/file_tools.py b/src/serena/tools/file_tools.py index b5770ab5..6e3944ba 100644 --- a/src/serena/tools/file_tools.py +++ b/src/serena/tools/file_tools.py @@ -289,8 +289,8 @@ class SearchForPatternTool(Tool): substring_pattern: str, context_lines_before: int = 0, context_lines_after: int = 0, - paths_include_glob: str | None = None, - paths_exclude_glob: str | None = None, + paths_include_glob: str = "", + paths_exclude_glob: str = "", relative_path: str = "", restrict_search_to_code_files: bool = False, max_answer_chars: int = TOOL_DEFAULT_MAX_ANSWER_LENGTH, @@ -327,10 +327,10 @@ class SearchForPatternTool(Tool): :param context_lines_after: Number of lines of context to include after each match :param paths_include_glob: optional glob pattern specifying files to include in the search. Matches against relative file paths from the project root (e.g., "*.py", "src/**/*.ts"). - Only matches files, not directories. + Only matches files, not directories. If left empty, all non-ignored files will be included. :param paths_exclude_glob: optional glob pattern specifying files to exclude from the search. Matches against relative file paths from the project root (e.g., "*test*", "**/*_generated.py"). - Takes precedence over paths_include_glob. Only matches files, not directories. + Takes precedence over paths_include_glob. Only matches files, not directories. If left empty, no files are excluded. :param relative_path: only subpaths of this path (relative to the repo root) will be analyzed. If a path to a single file is passed, only that will be searched. The path must exist, otherwise a `FileNotFoundError` is raised. :param max_answer_chars: if the output is longer than this number of characters, @@ -355,8 +355,8 @@ class SearchForPatternTool(Tool): relative_path=relative_path, context_lines_before=context_lines_before, context_lines_after=context_lines_after, - paths_include_glob=paths_include_glob, - paths_exclude_glob=paths_exclude_glob, + paths_include_glob=paths_include_glob.strip(), + paths_exclude_glob=paths_exclude_glob.strip(), ) else: if os.path.isfile(abs_path): diff --git a/src/serena/tools/symbol_tools.py b/src/serena/tools/symbol_tools.py index 199db67b..c57ec123 100644 --- a/src/serena/tools/symbol_tools.py +++ b/src/serena/tools/symbol_tools.py @@ -87,10 +87,10 @@ class FindSymbolTool(Tool, ToolMarkerSymbolicRead): self, name_path: str, depth: int = 0, - relative_path: str | None = None, + relative_path: str = "", include_body: bool = False, - include_kinds: list[int] | None = None, - exclude_kinds: list[int] | None = None, + include_kinds: list[int] = [], # noqa: B006 + exclude_kinds: list[int] = [], # noqa: B006 substring_matching: bool = False, max_answer_chars: int = TOOL_DEFAULT_MAX_ANSWER_LENGTH, ) -> str: @@ -136,8 +136,10 @@ class FindSymbolTool(Tool, ToolMarkerSymbolicRead): :param include_kinds: Optional. List of LSP symbol kind integers to include. (e.g., 5 for Class, 12 for Function). Valid kinds: 1=file, 2=module, 3=namespace, 4=package, 5=class, 6=method, 7=property, 8=field, 9=constructor, 10=enum, 11=interface, 12=function, 13=variable, 14=constant, 15=string, 16=number, 17=boolean, 18=array, 19=object, - 20=key, 21=null, 22=enum member, 23=struct, 24=event, 25=operator, 26=type parameter + 20=key, 21=null, 22=enum member, 23=struct, 24=event, 25=operator, 26=type parameter. + If not provided, all kinds are included. :param exclude_kinds: Optional. List of LSP symbol kind integers to exclude. Takes precedence over `include_kinds`. + If not provided, no kinds are excluded. :param substring_matching: If True, use substring matching for the last segment of `name`. :param max_answer_chars: Max characters for the JSON result. If exceeded, no content is returned. :return: a list of symbols (with locations) matching the name. @@ -167,8 +169,8 @@ class FindReferencingSymbolsTool(Tool, ToolMarkerSymbolicRead): self, name_path: str, relative_path: str, - include_kinds: list[int] | None = None, - exclude_kinds: list[int] | None = None, + include_kinds: list[int] = [], # noqa: B006 + exclude_kinds: list[int] = [], # noqa: B006 max_answer_chars: int = TOOL_DEFAULT_MAX_ANSWER_LENGTH, ) -> str: """