From 50239616c06d738f33c4c3126976f8cf8a25388f Mon Sep 17 00:00:00 2001 From: Maurya Date: Mon, 27 Apr 2026 13:56:48 +0530 Subject: [PATCH 1/2] fix: replace _tool_manager with local_provider for FastMCP 3.x tool-filter compat --- code_review_graph/main.py | 112 +++++++++++++++++++++++++++----------- tests/test_main.py | 57 ++++++++++--------- 2 files changed, 111 insertions(+), 58 deletions(-) diff --git a/code_review_graph/main.py b/code_review_graph/main.py index 733336e1..3ca328f6 100644 --- a/code_review_graph/main.py +++ b/code_review_graph/main.py @@ -149,7 +149,9 @@ async def run_postprocess_tool( """ return await asyncio.to_thread( run_postprocess, - flows=flows, communities=communities, fts=fts, + flows=flows, + communities=communities, + fts=fts, repo_root=_resolve_repo_root(repo_root), ) @@ -200,8 +202,11 @@ def get_impact_radius_tool( detail_level: "standard" for full output, "minimal" for compact summary. Default: standard. """ return get_impact_radius( - changed_files=changed_files, max_depth=max_depth, - repo_root=_resolve_repo_root(repo_root), base=base, detail_level=detail_level, + changed_files=changed_files, + max_depth=max_depth, + repo_root=_resolve_repo_root(repo_root), + base=base, + detail_level=detail_level, ) @@ -231,7 +236,9 @@ def query_graph_tool( detail_level: "standard" for full output, "minimal" for compact summary. Default: standard. """ return query_graph( - pattern=pattern, target=target, repo_root=_resolve_repo_root(repo_root), + pattern=pattern, + target=target, + repo_root=_resolve_repo_root(repo_root), detail_level=detail_level, ) @@ -262,9 +269,13 @@ def get_review_context_tool( token-efficient summary. Default: standard. """ return get_review_context( - changed_files=changed_files, max_depth=max_depth, - include_source=include_source, max_lines_per_file=max_lines_per_file, - repo_root=_resolve_repo_root(repo_root), base=base, detail_level=detail_level, + changed_files=changed_files, + max_depth=max_depth, + include_source=include_source, + max_lines_per_file=max_lines_per_file, + repo_root=_resolve_repo_root(repo_root), + base=base, + detail_level=detail_level, ) @@ -299,8 +310,13 @@ def semantic_search_nodes_tool( detail_level: "standard" for full output, "minimal" for compact summary. Default: standard. """ return semantic_search_nodes( - query=query, kind=kind, limit=limit, repo_root=_resolve_repo_root(repo_root), - model=model, provider=provider, detail_level=detail_level, + query=query, + kind=kind, + limit=limit, + repo_root=_resolve_repo_root(repo_root), + model=model, + provider=provider, + detail_level=detail_level, ) @@ -402,8 +418,11 @@ def find_large_functions_tool( repo_root: Repository root path. Auto-detected if omitted. """ return find_large_functions( - min_lines=min_lines, kind=kind, file_path_pattern=file_path_pattern, - limit=limit, repo_root=_resolve_repo_root(repo_root), + min_lines=min_lines, + kind=kind, + file_path_pattern=file_path_pattern, + limit=limit, + repo_root=_resolve_repo_root(repo_root), ) @@ -430,7 +449,10 @@ def list_flows_tool( repo_root: Repository root path. Auto-detected if omitted. """ return list_flows( - repo_root=_resolve_repo_root(repo_root), sort_by=sort_by, limit=limit, kind=kind, + repo_root=_resolve_repo_root(repo_root), + sort_by=sort_by, + limit=limit, + kind=kind, detail_level=detail_level, ) @@ -456,8 +478,10 @@ def get_flow_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_flow( - flow_id=flow_id, flow_name=flow_name, - include_source=include_source, repo_root=_resolve_repo_root(repo_root), + flow_id=flow_id, + flow_name=flow_name, + include_source=include_source, + repo_root=_resolve_repo_root(repo_root), ) @@ -479,7 +503,9 @@ def get_affected_flows_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_affected_flows_func( - changed_files=changed_files, base=base, repo_root=_resolve_repo_root(repo_root), + changed_files=changed_files, + base=base, + repo_root=_resolve_repo_root(repo_root), ) @@ -505,7 +531,9 @@ def list_communities_tool( repo_root: Repository root path. Auto-detected if omitted. """ return list_communities_func( - repo_root=_resolve_repo_root(repo_root), sort_by=sort_by, min_size=min_size, + repo_root=_resolve_repo_root(repo_root), + sort_by=sort_by, + min_size=min_size, detail_level=detail_level, ) @@ -532,8 +560,10 @@ def get_community_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_community_func( - community_name=community_name, community_id=community_id, - include_members=include_members, repo_root=_resolve_repo_root(repo_root), + community_name=community_name, + community_id=community_id, + include_members=include_members, + repo_root=_resolve_repo_root(repo_root), ) @@ -583,9 +613,12 @@ async def detect_changes_tool( """ return await asyncio.to_thread( detect_changes_func, - base=base, changed_files=changed_files, - include_source=include_source, max_depth=max_depth, - repo_root=_resolve_repo_root(repo_root), detail_level=detail_level, + base=base, + changed_files=changed_files, + include_source=include_source, + max_depth=max_depth, + repo_root=_resolve_repo_root(repo_root), + detail_level=detail_level, ) @@ -620,8 +653,12 @@ def refactor_tool( repo_root: Repository root path. Auto-detected if omitted. """ return refactor_func( - mode=mode, old_name=old_name, new_name=new_name, - kind=kind, file_pattern=file_pattern, repo_root=_resolve_repo_root(repo_root), + mode=mode, + old_name=old_name, + new_name=new_name, + kind=kind, + file_pattern=file_pattern, + repo_root=_resolve_repo_root(repo_root), ) @@ -650,7 +687,8 @@ def apply_refactor_tool( committing changes to disk. See: #176 """ return apply_refactor_func( - refactor_id=refactor_id, repo_root=_resolve_repo_root(repo_root), + refactor_id=refactor_id, + repo_root=_resolve_repo_root(repo_root), dry_run=dry_run, ) @@ -696,7 +734,8 @@ def get_wiki_page_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_wiki_page_func( - community_name=community_name, repo_root=_resolve_repo_root(repo_root), + community_name=community_name, + repo_root=_resolve_repo_root(repo_root), ) @@ -715,7 +754,8 @@ def get_hub_nodes_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_hub_nodes_func( - repo_root=_resolve_repo_root(repo_root), top_n=top_n, + repo_root=_resolve_repo_root(repo_root), + top_n=top_n, ) @@ -735,7 +775,8 @@ def get_bridge_nodes_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_bridge_nodes_func( - repo_root=_resolve_repo_root(repo_root), top_n=top_n, + repo_root=_resolve_repo_root(repo_root), + top_n=top_n, ) @@ -773,7 +814,8 @@ def get_surprising_connections_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_surprising_connections_func( - repo_root=_resolve_repo_root(repo_root), top_n=top_n, + repo_root=_resolve_repo_root(repo_root), + top_n=top_n, ) @@ -819,7 +861,9 @@ def traverse_graph_tool( repo_root: Repository root path. Auto-detected if omitted. """ return traverse_graph_func( - query=query, mode=mode, depth=depth, + query=query, + mode=mode, + depth=depth, token_budget=token_budget, repo_root=_resolve_repo_root(repo_root) or "", ) @@ -942,11 +986,14 @@ def _apply_tool_filter(tools: str | None = None) -> None: allowed = {t.strip() for t in raw.split(",") if t.strip()} if not allowed: return - registered = list(mcp._tool_manager._tools.keys()) + registered = [ + k.split(":")[1].split("@")[0] + for k in mcp.local_provider._components + if k.startswith("tool:") + ] for name in registered: if name not in allowed: - mcp.remove_tool(name) - + mcp.local_provider.remove_tool(name) def main( @@ -981,6 +1028,7 @@ def main( _apply_tool_filter(tools) if sys.platform == "win32": import asyncio + asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy()) if transport == "stdio": # Stdio MCP must keep stdout strictly JSON-RPC. FastMCP's banner/update diff --git a/tests/test_main.py b/tests/test_main.py index 61fbbf49..59740bfa 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -188,7 +188,10 @@ def test_regression_guard_does_not_depend_on_fastmcp_internals(self): # function bodies (not the docstrings) so an explanatory comment # mentioning an old API name doesn't trip this guard. forbidden_mcp_attrs = { - "get_tools", "_tools", "tool_manager", "_tool_manager", + "get_tools", + "_tools", + "tool_manager", + "_tool_manager", } for guard_fn in ( self.test_heavy_tools_are_coroutines, @@ -213,6 +216,7 @@ def test_regression_guard_does_not_depend_on_fastmcp_internals(self): f"getattr(crg_main, tool_name) instead." ) + class TestApplyToolFilter: """Tests for _apply_tool_filter (``serve --tools`` / ``CRG_TOOLS``). @@ -225,63 +229,64 @@ class TestApplyToolFilter: def _restore_tools(self): """Snapshot registered tools before test, restore after. - _apply_tool_filter calls ``mcp.remove_tool()`` which is + _apply_tool_filter calls ``local_provider.remove_tool()`` which is permanent. We restore by re-adding from the saved snapshot. """ - original = dict(crg_main.mcp._tool_manager._tools) + original = dict(crg_main.mcp.local_provider._components) yield - crg_main.mcp._tool_manager._tools.clear() - crg_main.mcp._tool_manager._tools.update(original) + crg_main.mcp.local_provider._components.clear() + crg_main.mcp.local_provider._components.update(original) @pytest.fixture(autouse=True) def _clean_env(self, monkeypatch): """Ensure CRG_TOOLS is not set from the outer environment.""" monkeypatch.delenv("CRG_TOOLS", raising=False) - @pytest.mark.asyncio - async def test_no_filter_keeps_all_tools(self): + def _tool_names(self) -> set[str]: + """Return current registered tool names without FastMCP internals.""" + return { + k.split(":")[1].split("@")[0] + for k in crg_main.mcp.local_provider._components + if k.startswith("tool:") + } + + def test_no_filter_keeps_all_tools(self): """When neither --tools nor CRG_TOOLS is set, all tools remain.""" - before = set((await crg_main.mcp.get_tools()).keys()) + before = self._tool_names() crg_main._apply_tool_filter(None) - after = set((await crg_main.mcp.get_tools()).keys()) + after = self._tool_names() assert before == after - @pytest.mark.asyncio - async def test_filter_via_argument(self): + def test_filter_via_argument(self): """The ``tools`` argument keeps only the listed tools.""" keep = "query_graph_tool,semantic_search_nodes_tool" crg_main._apply_tool_filter(keep) - remaining = set((await crg_main.mcp.get_tools()).keys()) + remaining = self._tool_names() assert remaining == {"query_graph_tool", "semantic_search_nodes_tool"} - @pytest.mark.asyncio - async def test_filter_via_env_var(self, monkeypatch): + def test_filter_via_env_var(self, monkeypatch): """The ``CRG_TOOLS`` env var works as fallback.""" monkeypatch.setenv("CRG_TOOLS", "query_graph_tool") crg_main._apply_tool_filter(None) - remaining = set((await crg_main.mcp.get_tools()).keys()) + remaining = self._tool_names() assert remaining == {"query_graph_tool"} - @pytest.mark.asyncio - async def test_argument_takes_precedence_over_env(self, monkeypatch): + def test_argument_takes_precedence_over_env(self, monkeypatch): """CLI --tools wins over CRG_TOOLS env var.""" monkeypatch.setenv("CRG_TOOLS", "list_repos_tool") crg_main._apply_tool_filter("query_graph_tool") - remaining = set((await crg_main.mcp.get_tools()).keys()) + remaining = self._tool_names() assert remaining == {"query_graph_tool"} - @pytest.mark.asyncio - async def test_empty_string_is_noop(self): + def test_empty_string_is_noop(self): """An empty string should not remove all tools.""" - before = set((await crg_main.mcp.get_tools()).keys()) + before = self._tool_names() crg_main._apply_tool_filter("") - after = set((await crg_main.mcp.get_tools()).keys()) + after = self._tool_names() assert before == after - @pytest.mark.asyncio - async def test_whitespace_handling(self): + def test_whitespace_handling(self): """Spaces around tool names are stripped.""" crg_main._apply_tool_filter(" query_graph_tool , semantic_search_nodes_tool ") - remaining = set((await crg_main.mcp.get_tools()).keys()) + remaining = self._tool_names() assert remaining == {"query_graph_tool", "semantic_search_nodes_tool"} - From d041021e3d86f6f5a67c21d06d8bea1452ad2d63 Mon Sep 17 00:00:00 2001 From: Maurya Date: Mon, 27 Apr 2026 14:55:05 +0530 Subject: [PATCH 2/2] chore: remove cosmetic-only formatting changes from PR #396 Keep only the semantic FastMCP 3.x compatibility edits: - main.py: enumerate tools via mcp.local_provider._components instead of _tool_manager._tools - main.py: remove via mcp.local_provider.remove_tool() instead of mcp.remove_tool() - tests/test_main.py: fixture snapshot/restore via local_provider._components - tests/test_main.py: sync _tool_names() helper replacing async get_tools() path - tests/test_main.py: docstring update to local_provider.remove_tool() Reverts: one-arg-per-line call reflows, blank line near asyncio import, set-literal reformat, extra blank line before class, EOF whitespace churn. --- code_review_graph/main.py | 104 +++++++++++--------------------------- tests/test_main.py | 7 +-- 2 files changed, 32 insertions(+), 79 deletions(-) diff --git a/code_review_graph/main.py b/code_review_graph/main.py index 3ca328f6..94c5f434 100644 --- a/code_review_graph/main.py +++ b/code_review_graph/main.py @@ -149,9 +149,7 @@ async def run_postprocess_tool( """ return await asyncio.to_thread( run_postprocess, - flows=flows, - communities=communities, - fts=fts, + flows=flows, communities=communities, fts=fts, repo_root=_resolve_repo_root(repo_root), ) @@ -202,11 +200,8 @@ def get_impact_radius_tool( detail_level: "standard" for full output, "minimal" for compact summary. Default: standard. """ return get_impact_radius( - changed_files=changed_files, - max_depth=max_depth, - repo_root=_resolve_repo_root(repo_root), - base=base, - detail_level=detail_level, + changed_files=changed_files, max_depth=max_depth, + repo_root=_resolve_repo_root(repo_root), base=base, detail_level=detail_level, ) @@ -236,9 +231,7 @@ def query_graph_tool( detail_level: "standard" for full output, "minimal" for compact summary. Default: standard. """ return query_graph( - pattern=pattern, - target=target, - repo_root=_resolve_repo_root(repo_root), + pattern=pattern, target=target, repo_root=_resolve_repo_root(repo_root), detail_level=detail_level, ) @@ -269,13 +262,9 @@ def get_review_context_tool( token-efficient summary. Default: standard. """ return get_review_context( - changed_files=changed_files, - max_depth=max_depth, - include_source=include_source, - max_lines_per_file=max_lines_per_file, - repo_root=_resolve_repo_root(repo_root), - base=base, - detail_level=detail_level, + changed_files=changed_files, max_depth=max_depth, + include_source=include_source, max_lines_per_file=max_lines_per_file, + repo_root=_resolve_repo_root(repo_root), base=base, detail_level=detail_level, ) @@ -310,13 +299,8 @@ def semantic_search_nodes_tool( detail_level: "standard" for full output, "minimal" for compact summary. Default: standard. """ return semantic_search_nodes( - query=query, - kind=kind, - limit=limit, - repo_root=_resolve_repo_root(repo_root), - model=model, - provider=provider, - detail_level=detail_level, + query=query, kind=kind, limit=limit, repo_root=_resolve_repo_root(repo_root), + model=model, provider=provider, detail_level=detail_level, ) @@ -418,11 +402,8 @@ def find_large_functions_tool( repo_root: Repository root path. Auto-detected if omitted. """ return find_large_functions( - min_lines=min_lines, - kind=kind, - file_path_pattern=file_path_pattern, - limit=limit, - repo_root=_resolve_repo_root(repo_root), + min_lines=min_lines, kind=kind, file_path_pattern=file_path_pattern, + limit=limit, repo_root=_resolve_repo_root(repo_root), ) @@ -449,10 +430,7 @@ def list_flows_tool( repo_root: Repository root path. Auto-detected if omitted. """ return list_flows( - repo_root=_resolve_repo_root(repo_root), - sort_by=sort_by, - limit=limit, - kind=kind, + repo_root=_resolve_repo_root(repo_root), sort_by=sort_by, limit=limit, kind=kind, detail_level=detail_level, ) @@ -478,10 +456,8 @@ def get_flow_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_flow( - flow_id=flow_id, - flow_name=flow_name, - include_source=include_source, - repo_root=_resolve_repo_root(repo_root), + flow_id=flow_id, flow_name=flow_name, + include_source=include_source, repo_root=_resolve_repo_root(repo_root), ) @@ -503,9 +479,7 @@ def get_affected_flows_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_affected_flows_func( - changed_files=changed_files, - base=base, - repo_root=_resolve_repo_root(repo_root), + changed_files=changed_files, base=base, repo_root=_resolve_repo_root(repo_root), ) @@ -531,9 +505,7 @@ def list_communities_tool( repo_root: Repository root path. Auto-detected if omitted. """ return list_communities_func( - repo_root=_resolve_repo_root(repo_root), - sort_by=sort_by, - min_size=min_size, + repo_root=_resolve_repo_root(repo_root), sort_by=sort_by, min_size=min_size, detail_level=detail_level, ) @@ -560,10 +532,8 @@ def get_community_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_community_func( - community_name=community_name, - community_id=community_id, - include_members=include_members, - repo_root=_resolve_repo_root(repo_root), + community_name=community_name, community_id=community_id, + include_members=include_members, repo_root=_resolve_repo_root(repo_root), ) @@ -613,12 +583,9 @@ async def detect_changes_tool( """ return await asyncio.to_thread( detect_changes_func, - base=base, - changed_files=changed_files, - include_source=include_source, - max_depth=max_depth, - repo_root=_resolve_repo_root(repo_root), - detail_level=detail_level, + base=base, changed_files=changed_files, + include_source=include_source, max_depth=max_depth, + repo_root=_resolve_repo_root(repo_root), detail_level=detail_level, ) @@ -653,12 +620,8 @@ def refactor_tool( repo_root: Repository root path. Auto-detected if omitted. """ return refactor_func( - mode=mode, - old_name=old_name, - new_name=new_name, - kind=kind, - file_pattern=file_pattern, - repo_root=_resolve_repo_root(repo_root), + mode=mode, old_name=old_name, new_name=new_name, + kind=kind, file_pattern=file_pattern, repo_root=_resolve_repo_root(repo_root), ) @@ -687,8 +650,7 @@ def apply_refactor_tool( committing changes to disk. See: #176 """ return apply_refactor_func( - refactor_id=refactor_id, - repo_root=_resolve_repo_root(repo_root), + refactor_id=refactor_id, repo_root=_resolve_repo_root(repo_root), dry_run=dry_run, ) @@ -734,8 +696,7 @@ def get_wiki_page_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_wiki_page_func( - community_name=community_name, - repo_root=_resolve_repo_root(repo_root), + community_name=community_name, repo_root=_resolve_repo_root(repo_root), ) @@ -754,8 +715,7 @@ def get_hub_nodes_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_hub_nodes_func( - repo_root=_resolve_repo_root(repo_root), - top_n=top_n, + repo_root=_resolve_repo_root(repo_root), top_n=top_n, ) @@ -775,8 +735,7 @@ def get_bridge_nodes_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_bridge_nodes_func( - repo_root=_resolve_repo_root(repo_root), - top_n=top_n, + repo_root=_resolve_repo_root(repo_root), top_n=top_n, ) @@ -814,8 +773,7 @@ def get_surprising_connections_tool( repo_root: Repository root path. Auto-detected if omitted. """ return get_surprising_connections_func( - repo_root=_resolve_repo_root(repo_root), - top_n=top_n, + repo_root=_resolve_repo_root(repo_root), top_n=top_n, ) @@ -861,9 +819,7 @@ def traverse_graph_tool( repo_root: Repository root path. Auto-detected if omitted. """ return traverse_graph_func( - query=query, - mode=mode, - depth=depth, + query=query, mode=mode, depth=depth, token_budget=token_budget, repo_root=_resolve_repo_root(repo_root) or "", ) @@ -996,6 +952,7 @@ def _apply_tool_filter(tools: str | None = None) -> None: mcp.local_provider.remove_tool(name) + def main( repo_root: str | None = None, tools: str | None = None, @@ -1028,7 +985,6 @@ def main( _apply_tool_filter(tools) if sys.platform == "win32": import asyncio - asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy()) if transport == "stdio": # Stdio MCP must keep stdout strictly JSON-RPC. FastMCP's banner/update diff --git a/tests/test_main.py b/tests/test_main.py index 59740bfa..2d174883 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -188,10 +188,7 @@ def test_regression_guard_does_not_depend_on_fastmcp_internals(self): # function bodies (not the docstrings) so an explanatory comment # mentioning an old API name doesn't trip this guard. forbidden_mcp_attrs = { - "get_tools", - "_tools", - "tool_manager", - "_tool_manager", + "get_tools", "_tools", "tool_manager", "_tool_manager", } for guard_fn in ( self.test_heavy_tools_are_coroutines, @@ -216,7 +213,6 @@ def test_regression_guard_does_not_depend_on_fastmcp_internals(self): f"getattr(crg_main, tool_name) instead." ) - class TestApplyToolFilter: """Tests for _apply_tool_filter (``serve --tools`` / ``CRG_TOOLS``). @@ -290,3 +286,4 @@ def test_whitespace_handling(self): crg_main._apply_tool_filter(" query_graph_tool , semantic_search_nodes_tool ") remaining = self._tool_names() assert remaining == {"query_graph_tool", "semantic_search_nodes_tool"} +