From c531bffde9c6a5809587577b8a43a195a065e04f Mon Sep 17 00:00:00 2001 From: Maurya Date: Mon, 27 Apr 2026 12:30:07 +0530 Subject: [PATCH 1/6] fix: make get_minimal_context_tool async via asyncio.to_thread Prevents event-loop blocking on large repos by offloading the blocking git-diff + BFS work to a thread. Updates TestLongRunningToolsAreAsync to include get_minimal_context_tool in HEAVY_TOOLS coverage. --- code_review_graph/main.py | 114 +++++++++++++++++++++++++++----------- tests/test_main.py | 1 + 2 files changed, 83 insertions(+), 32 deletions(-) diff --git a/code_review_graph/main.py b/code_review_graph/main.py index a36f68a9..b61482f9 100644 --- a/code_review_graph/main.py +++ b/code_review_graph/main.py @@ -147,13 +147,15 @@ 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), ) @mcp.tool() -def get_minimal_context_tool( +async def get_minimal_context_tool( task: str = "", changed_files: Optional[list[str]] = None, repo_root: Optional[str] = None, @@ -165,15 +167,21 @@ def get_minimal_context_tool( next tools in a single compact response. Use this as the entry point before any other graph tool to minimize token usage. + Offloaded to a thread via ``asyncio.to_thread`` so git diff / analyze_changes + don't block the stdio event loop. See: #46, #136. + Args: task: What you are doing (e.g. "review PR #42", "debug login timeout"). changed_files: Explicit list of changed files. Auto-detected if omitted. repo_root: Repository root path. Auto-detected if omitted. base: Git ref for diff comparison. Default: HEAD~1. """ - return get_minimal_context( - task=task, changed_files=changed_files, - repo_root=_resolve_repo_root(repo_root), base=base, + return await asyncio.to_thread( + get_minimal_context, + task=task, + changed_files=changed_files, + repo_root=_resolve_repo_root(repo_root), + base=base, ) @@ -198,8 +206,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, ) @@ -229,7 +240,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, ) @@ -260,9 +273,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, ) @@ -291,7 +308,11 @@ 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, + query=query, + kind=kind, + limit=limit, + repo_root=_resolve_repo_root(repo_root), + model=model, detail_level=detail_level, ) @@ -382,8 +403,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), ) @@ -410,7 +434,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, ) @@ -436,8 +463,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), ) @@ -459,7 +488,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), ) @@ -485,7 +516,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, ) @@ -512,8 +545,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), ) @@ -563,9 +598,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, ) @@ -600,8 +638,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), ) @@ -630,7 +672,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, ) @@ -676,7 +719,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), ) @@ -695,7 +739,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, ) @@ -715,7 +760,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, ) @@ -753,7 +799,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, ) @@ -799,7 +846,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 "", ) @@ -903,6 +952,7 @@ def main(repo_root: str | None = None) -> None: _default_repo_root = repo_root if sys.platform == "win32": import asyncio + asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy()) mcp.run(transport="stdio") diff --git a/tests/test_main.py b/tests/test_main.py index 07ee7162..0684f3a8 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -62,6 +62,7 @@ class TestLongRunningToolsAreAsync: "embed_graph_tool", "detect_changes_tool", "generate_wiki_tool", + "get_minimal_context_tool", } @pytest.mark.asyncio From f1f7333916240fce3fb2855b19af1b33de9c0d17 Mon Sep 17 00:00:00 2001 From: Maurya Date: Mon, 27 Apr 2026 12:47:06 +0530 Subject: [PATCH 2/6] fix: sort imports in main.py to resolve ruff I001 --- code_review_graph/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code_review_graph/main.py b/code_review_graph/main.py index 8674f995..79f7ec79 100644 --- a/code_review_graph/main.py +++ b/code_review_graph/main.py @@ -43,7 +43,6 @@ get_suggested_questions_func, get_surprising_connections_func, get_wiki_page_func, - traverse_graph_func, list_communities_func, list_flows, list_graph_stats, @@ -52,6 +51,7 @@ refactor_func, run_postprocess, semantic_search_nodes, + traverse_graph_func, ) # NOTE: Thread-safe for stdio MCP (single-threaded). If adding HTTP/SSE From d416de38f62a6e82e94c8d897f8319218fc11c7c Mon Sep 17 00:00:00 2001 From: Maurya Date: Mon, 27 Apr 2026 12:47:43 +0530 Subject: [PATCH 3/6] test: fix stdio assertion to include show_banner=False --- tests/test_main.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/test_main.py b/tests/test_main.py index 7f542395..a3018c61 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -59,7 +59,7 @@ def fake_run(**kwargs): monkeypatch.setattr(crg_main.mcp, "run", fake_run) crg_main.main(repo_root=None) - assert calls == [{"transport": "stdio"}] + assert calls == [{"transport": "stdio", "show_banner": False}] def test_http_calls_mcp_run_with_host_port(self, monkeypatch): calls: list[dict] = [] @@ -189,7 +189,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, @@ -214,6 +217,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``). @@ -285,4 +289,3 @@ async def test_whitespace_handling(self): crg_main._apply_tool_filter(" query_graph_tool , semantic_search_nodes_tool ") remaining = set((await crg_main.mcp.get_tools()).keys()) assert remaining == {"query_graph_tool", "semantic_search_nodes_tool"} - From 325149265787423bb9d4434afcfced9d7d3b73f2 Mon Sep 17 00:00:00 2001 From: Maurya Date: Mon, 27 Apr 2026 12:55:44 +0530 Subject: [PATCH 4/6] chore: remove cosmetic-only formatting changes from PR Revert all parameter reflow, blank line churn, and non-semantic formatting introduced by earlier merge/fix commits. Keep only: - import sort (ruff I001 fix) - async def get_minimal_context_tool - await asyncio.to_thread wrapper + docstring - show_banner=False assertion - get_minimal_context_tool in heavy-tools set --- 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 79f7ec79..5671244f 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), ) @@ -208,11 +206,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, ) @@ -242,9 +237,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, ) @@ -275,13 +268,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, ) @@ -316,13 +305,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, ) @@ -424,11 +408,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), ) @@ -455,10 +436,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, ) @@ -484,10 +462,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), ) @@ -509,9 +485,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), ) @@ -537,9 +511,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, ) @@ -566,10 +538,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), ) @@ -619,12 +589,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, ) @@ -659,12 +626,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), ) @@ -693,8 +656,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, ) @@ -740,8 +702,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), ) @@ -760,8 +721,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, ) @@ -781,8 +741,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, ) @@ -820,8 +779,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, ) @@ -867,9 +825,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 "", ) @@ -998,6 +954,7 @@ def _apply_tool_filter(tools: str | None = None) -> None: mcp.remove_tool(name) + def main( repo_root: str | None = None, tools: str | None = None, @@ -1030,7 +987,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 a3018c61..62bbe171 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -189,10 +189,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, @@ -217,7 +214,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``). @@ -289,3 +285,4 @@ async def test_whitespace_handling(self): crg_main._apply_tool_filter(" query_graph_tool , semantic_search_nodes_tool ") remaining = set((await crg_main.mcp.get_tools()).keys()) assert remaining == {"query_graph_tool", "semantic_search_nodes_tool"} + From 5a6bdec20d22e6072a943ee256e9d743ab4e648f Mon Sep 17 00:00:00 2001 From: Maurya Date: Mon, 27 Apr 2026 13:56:48 +0530 Subject: [PATCH 5/6] 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 5671244f..173fcaab 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), ) @@ -206,8 +208,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, ) @@ -237,7 +242,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, ) @@ -268,9 +275,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, ) @@ -305,8 +316,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, ) @@ -408,8 +424,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), ) @@ -436,7 +455,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, ) @@ -462,8 +484,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), ) @@ -485,7 +509,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), ) @@ -511,7 +537,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, ) @@ -538,8 +566,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), ) @@ -589,9 +619,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, ) @@ -626,8 +659,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), ) @@ -656,7 +693,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, ) @@ -702,7 +740,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), ) @@ -721,7 +760,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, ) @@ -741,7 +781,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, ) @@ -779,7 +820,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, ) @@ -825,7 +867,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 "", ) @@ -948,11 +992,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( @@ -987,6 +1034,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 62bbe171..2f3880b3 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -189,7 +189,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, @@ -214,6 +217,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``). @@ -226,63 +230,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 bacca6a14c1feb4ee9252ca5820144cc4c7f29e4 Mon Sep 17 00:00:00 2001 From: Maurya Date: Mon, 27 Apr 2026 14:16:46 +0530 Subject: [PATCH 6/6] Revert "fix: replace _tool_manager with local_provider for FastMCP 3.x tool-filter compat" This reverts commit 5a6bdec20d22e6072a943ee256e9d743ab4e648f. --- code_review_graph/main.py | 112 +++++++++++--------------------------- tests/test_main.py | 57 +++++++++---------- 2 files changed, 58 insertions(+), 111 deletions(-) diff --git a/code_review_graph/main.py b/code_review_graph/main.py index 173fcaab..5671244f 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), ) @@ -208,11 +206,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, ) @@ -242,9 +237,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, ) @@ -275,13 +268,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, ) @@ -316,13 +305,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, ) @@ -424,11 +408,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), ) @@ -455,10 +436,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, ) @@ -484,10 +462,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), ) @@ -509,9 +485,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), ) @@ -537,9 +511,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, ) @@ -566,10 +538,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), ) @@ -619,12 +589,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, ) @@ -659,12 +626,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), ) @@ -693,8 +656,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, ) @@ -740,8 +702,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), ) @@ -760,8 +721,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, ) @@ -781,8 +741,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, ) @@ -820,8 +779,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, ) @@ -867,9 +825,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 "", ) @@ -992,14 +948,11 @@ 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 = [ - k.split(":")[1].split("@")[0] - for k in mcp.local_provider._components - if k.startswith("tool:") - ] + registered = list(mcp._tool_manager._tools.keys()) for name in registered: if name not in allowed: - mcp.local_provider.remove_tool(name) + mcp.remove_tool(name) + def main( @@ -1034,7 +987,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 2f3880b3..62bbe171 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -189,10 +189,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, @@ -217,7 +214,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``). @@ -230,64 +226,63 @@ class TestApplyToolFilter: def _restore_tools(self): """Snapshot registered tools before test, restore after. - _apply_tool_filter calls ``local_provider.remove_tool()`` which is + _apply_tool_filter calls ``mcp.remove_tool()`` which is permanent. We restore by re-adding from the saved snapshot. """ - original = dict(crg_main.mcp.local_provider._components) + original = dict(crg_main.mcp._tool_manager._tools) yield - crg_main.mcp.local_provider._components.clear() - crg_main.mcp.local_provider._components.update(original) + crg_main.mcp._tool_manager._tools.clear() + crg_main.mcp._tool_manager._tools.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) - 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): + @pytest.mark.asyncio + async def test_no_filter_keeps_all_tools(self): """When neither --tools nor CRG_TOOLS is set, all tools remain.""" - before = self._tool_names() + before = set((await crg_main.mcp.get_tools()).keys()) crg_main._apply_tool_filter(None) - after = self._tool_names() + after = set((await crg_main.mcp.get_tools()).keys()) assert before == after - def test_filter_via_argument(self): + @pytest.mark.asyncio + async 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 = self._tool_names() + remaining = set((await crg_main.mcp.get_tools()).keys()) assert remaining == {"query_graph_tool", "semantic_search_nodes_tool"} - def test_filter_via_env_var(self, monkeypatch): + @pytest.mark.asyncio + async 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 = self._tool_names() + remaining = set((await crg_main.mcp.get_tools()).keys()) assert remaining == {"query_graph_tool"} - def test_argument_takes_precedence_over_env(self, monkeypatch): + @pytest.mark.asyncio + async 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 = self._tool_names() + remaining = set((await crg_main.mcp.get_tools()).keys()) assert remaining == {"query_graph_tool"} - def test_empty_string_is_noop(self): + @pytest.mark.asyncio + async def test_empty_string_is_noop(self): """An empty string should not remove all tools.""" - before = self._tool_names() + before = set((await crg_main.mcp.get_tools()).keys()) crg_main._apply_tool_filter("") - after = self._tool_names() + after = set((await crg_main.mcp.get_tools()).keys()) assert before == after - def test_whitespace_handling(self): + @pytest.mark.asyncio + async def test_whitespace_handling(self): """Spaces around tool names are stripped.""" crg_main._apply_tool_filter(" query_graph_tool , semantic_search_nodes_tool ") - remaining = self._tool_names() + remaining = set((await crg_main.mcp.get_tools()).keys()) assert remaining == {"query_graph_tool", "semantic_search_nodes_tool"} +