diff --git a/src/mcp_client.py b/src/mcp_client.py index 174a020f..e691fcb7 100644 --- a/src/mcp_client.py +++ b/src/mcp_client.py @@ -172,21 +172,7 @@ def _is_allowed(path: Path) -> bool: return True return False -def _resolve_and_check(raw_path: str) -> tuple[Path | None, str]: - """ - Resolve raw_path and verify it passes the allowlist check. - Returns (resolved_path, error_string). error_string is empty on success. - - Thin wrapper over _resolve_and_check_result; the legacy (Path|None, str) - tuple shape is preserved for backward compatibility, but the try/except - handling now lives in the Result variant (where it can return ErrorInfo - with the structured kind/message/original fields). - """ - resolved = _resolve_and_check_result(raw_path) - if resolved.ok and not isinstance(resolved.data, NilPath): - return resolved.data, "" - return None, "; ".join(e.ui_message() for e in resolved.errors) - # ------------------------------------------------------------------ tool implementations +# ------------------------------------------------------------------ tool implementations def search_files(path: str, pattern: str) -> str: """ @@ -1823,12 +1809,20 @@ def dispatch(tool_name: str, tool_input: dict[str, Any]) -> str: if tool_name == "ts_cpp_update_definition": return ts_cpp_update_definition(path, str(tool_input.get("name", "")), str(tool_input.get("new_content", ""))) if tool_name == "py_remove_def": - p, err = _resolve_and_check(path) - if err: return err + resolved = _resolve_and_check_result(path) + if not resolved.ok: + return "; ".join(e.ui_message() for e in resolved.errors) + if isinstance(resolved.data, NilPath): + return "; ".join(e.ui_message() for e in resolved.errors) + p = resolved.data return py_struct_tools.py_remove_def(str(p), str(tool_input.get("name", ""))) if tool_name == "py_add_def": - p, err = _resolve_and_check(path) - if err: return err + resolved = _resolve_and_check_result(path) + if not resolved.ok: + return "; ".join(e.ui_message() for e in resolved.errors) + if isinstance(resolved.data, NilPath): + return "; ".join(e.ui_message() for e in resolved.errors) + p = resolved.data return py_struct_tools.py_add_def( str(p), str(tool_input.get("name", "")), @@ -1837,23 +1831,26 @@ def dispatch(tool_name: str, tool_input: dict[str, Any]) -> str: tool_input.get("anchor_symbol") ) if tool_name == "py_move_def": - p_src, err = _resolve_and_check(str(tool_input.get("src_path", ""))) - if err: return err - p_dest, err = _resolve_and_check(str(tool_input.get("dest_path", ""))) - if err: return err + resolved_src = _resolve_and_check_result(str(tool_input.get("src_path", ""))) + if not resolved_src.ok or isinstance(resolved_src.data, NilPath): + return "; ".join(e.ui_message() for e in resolved_src.errors) + resolved_dest = _resolve_and_check_result(str(tool_input.get("dest_path", ""))) + if not resolved_dest.ok or isinstance(resolved_dest.data, NilPath): + return "; ".join(e.ui_message() for e in resolved_dest.errors) return py_struct_tools.py_move_def( - str(p_src), - str(p_dest), + str(resolved_src.data), + str(resolved_dest.data), str(tool_input.get("name", "")), str(tool_input.get("dest_name", "")), str(tool_input.get("anchor_type", "")), tool_input.get("anchor_symbol") ) if tool_name == "py_region_wrap": - p, err = _resolve_and_check(path) - if err: return err + resolved = _resolve_and_check_result(path) + if not resolved.ok or isinstance(resolved.data, NilPath): + return "; ".join(e.ui_message() for e in resolved.errors) return py_struct_tools.py_region_wrap( - str(p), + str(resolved.data), int(tool_input.get("start_line", 1)), int(tool_input.get("end_line", 1)), str(tool_input.get("region_name", "")) diff --git a/tests/test_cruft_removal.py b/tests/test_cruft_removal.py new file mode 100644 index 00000000..d80a56aa --- /dev/null +++ b/tests/test_cruft_removal.py @@ -0,0 +1,84 @@ +"""Wrapper-obliteration tests for result_migration_cruft_removal_20260620. + +Phase 3 (mcp_client._resolve_and_check): the legacy wrapper is DELETED; +callers in dispatch_tool_call use _resolve_and_check_result(...).ok directly. +The test mocks _resolve_and_check_result (the proper Result helper). +""" +import subprocess +from pathlib import Path +from unittest.mock import patch + +import pytest + +from src.mcp_client import dispatch + + +@pytest.fixture +def mock_resolve_result(): + """Mock the proper Result helper to always succeed.""" + from src import mcp_client + original = mcp_client._resolve_and_check_result + mcp_client._resolve_and_check_result = lambda path: __import__("src").mcp_client.Result( + data=Path(path), + errors=[], + ) + yield + mcp_client._resolve_and_check_result = original + + +def test_resolve_and_check_wrapper_obliterated(): + """Phase 3 invariant: the legacy _resolve_and_check wrapper is DELETED.""" + from src import mcp_client + assert not hasattr(mcp_client, "_resolve_and_check"), ( + "_resolve_and_check legacy wrapper must be OBLITERATED (deleted). " + "Callers must use _resolve_and_check_result(...).ok directly." + ) + + +def test_dispatch_py_remove_def_uses_result_helper(mock_resolve_result, tmp_path): + """dispatch_tool_call for py_remove_def uses _resolve_and_check_result(...).ok.""" + from src import mcp_client + + c_file = tmp_path / "test.py" + c_file.write_text("def foo(): pass\n") + + with patch("src.mcp_client.py_struct_tools") as mock_struct: + mock_struct.py_remove_def.return_value = "removed" + # dispatch routes through _resolve_and_check_result now + result = dispatch("py_remove_def", {"path": str(c_file), "name": "foo"}) + assert result == "removed", f"expected 'removed', got {result!r}" + + +def test_dispatch_py_move_def_uses_result_helper(mock_resolve_result, tmp_path): + """dispatch_tool_call for py_move_def (multi-path) uses _resolve_and_check_result.""" + from src import mcp_client + + src_file = tmp_path / "src.py" + src_file.write_text("def foo(): pass\n") + dest_file = tmp_path / "dest.py" + dest_file.write_text("# dest\n") + + with patch("src.mcp_client.py_struct_tools") as mock_struct: + mock_struct.py_move_def.return_value = "moved" + result = dispatch( + "py_move_def", + { + "src_path": str(src_file), + "dest_path": str(dest_file), + "name": "foo", + "dest_name": "App", + "anchor_type": "before", + }, + ) + assert result == "moved", f"expected 'moved', got {result!r}" + + +def test_audit_script_finds_zero_mcp_client_wrappers(): + """Phase 3 invariant: scripts/audit_legacy_wrappers.py reports 0 wrappers in src/mcp_client.py.""" + r = subprocess.run( + ["uv", "run", "python", "scripts/audit_legacy_wrappers.py"], + capture_output=True, text=True, + ) + assert "src\\mcp_client.py" not in r.stdout, ( + f"expected 0 wrappers in src/mcp_client.py, but found:\n{r.stdout}" + ) \ No newline at end of file diff --git a/tests/test_mcp_ts_integration.py b/tests/test_mcp_ts_integration.py index 477a09df..307d3df1 100644 --- a/tests/test_mcp_ts_integration.py +++ b/tests/test_mcp_ts_integration.py @@ -12,10 +12,10 @@ from src.mcp_client import dispatch @pytest.fixture def mock_resolve(): from src import mcp_client - original_resolve = mcp_client._resolve_and_check - mcp_client._resolve_and_check = lambda path: (Path(path), None) + original_resolve = mcp_client._resolve_and_check_result + mcp_client._resolve_and_check_result = lambda path: __import__("src").mcp_client.Result(data=Path(path), errors=[]) yield - mcp_client._resolve_and_check = original_resolve + mcp_client._resolve_and_check_result = original_resolve def test_ts_c_get_skeleton_dispatch(tmp_path, mock_resolve): # Verify ts_c_get_skeleton via dispatch diff --git a/tests/test_ts_c_tools.py b/tests/test_ts_c_tools.py index fe024208..294cadfc 100644 --- a/tests/test_ts_c_tools.py +++ b/tests/test_ts_c_tools.py @@ -29,8 +29,8 @@ struct Point { # Mock _resolve_and_check to allow tmp_path from src import mcp_client - original_resolve = mcp_client._resolve_and_check - mcp_client._resolve_and_check = lambda path: (Path(path), None) + original_resolve = mcp_client._resolve_and_check_result + mcp_client._resolve_and_check_result = lambda path: __import__("src").mcp_client.Result(data=Path(path), errors=[]) try: skeleton = ts_c_get_skeleton(str(c_file)) @@ -39,7 +39,7 @@ struct Point { assert "struct Point" in skeleton assert "printf" not in skeleton finally: - mcp_client._resolve_and_check = original_resolve + mcp_client._resolve_and_check_result = original_resolve def test_ts_c_get_code_outline(tmp_path): c_code = """ @@ -54,12 +54,12 @@ int func2(int x) { c_file.write_text(c_code) from src import mcp_client - original_resolve = mcp_client._resolve_and_check - mcp_client._resolve_and_check = lambda path: (Path(path), None) + original_resolve = mcp_client._resolve_and_check_result + mcp_client._resolve_and_check_result = lambda path: __import__("src").mcp_client.Result(data=Path(path), errors=[]) try: outline = ts_c_get_code_outline(str(c_file)) assert "[Func] func1 (Lines 2-3)" in outline assert "[Func] func2 (Lines 5-7)" in outline finally: - mcp_client._resolve_and_check = original_resolve + mcp_client._resolve_and_check_result = original_resolve diff --git a/tests/test_ts_cpp_tools.py b/tests/test_ts_cpp_tools.py index 4c162881..3cfcc9bc 100644 --- a/tests/test_ts_cpp_tools.py +++ b/tests/test_ts_cpp_tools.py @@ -31,8 +31,8 @@ void globalFunc() { cpp_file.write_text(cpp_code) from src import mcp_client - original_resolve = mcp_client._resolve_and_check - mcp_client._resolve_and_check = lambda path: (Path(path), None) + original_resolve = mcp_client._resolve_and_check_result + mcp_client._resolve_and_check_result = lambda path: __import__("src").mcp_client.Result(data=Path(path), errors=[]) try: skeleton = ts_cpp_get_skeleton(str(cpp_file)) @@ -42,7 +42,7 @@ void globalFunc() { assert "void globalFunc() { ... }" in skeleton assert "std::cout" not in skeleton finally: - mcp_client._resolve_and_check = original_resolve + mcp_client._resolve_and_check_result = original_resolve def test_ts_cpp_get_code_outline(tmp_path): cpp_code = """ @@ -59,8 +59,8 @@ void templateFunc(T t) { cpp_file.write_text(cpp_code) from src import mcp_client - original_resolve = mcp_client._resolve_and_check - mcp_client._resolve_and_check = lambda path: (Path(path), None) + original_resolve = mcp_client._resolve_and_check_result + mcp_client._resolve_and_check_result = lambda path: __import__("src").mcp_client.Result(data=Path(path), errors=[]) try: outline = ts_cpp_get_code_outline(str(cpp_file)) @@ -68,7 +68,7 @@ void templateFunc(T t) { assert "[Method] method1 (Lines 3-4)" in outline assert "[Func] templateFunc (Lines 8-9)" in outline finally: - mcp_client._resolve_and_check = original_resolve + mcp_client._resolve_and_check_result = original_resolve def test_exhaustive_cpp_samples(): base_dir = Path(__file__).parent / "assets" / "cpp_samples" @@ -99,8 +99,8 @@ def test_exhaustive_cpp_samples(): } from src import mcp_client - original_resolve = mcp_client._resolve_and_check - mcp_client._resolve_and_check = lambda path: (Path(path), None) + original_resolve = mcp_client._resolve_and_check_result + mcp_client._resolve_and_check_result = lambda path: __import__("src").mcp_client.Result(data=Path(path), errors=[]) try: for filename, symbols in files_to_test.items(): @@ -123,7 +123,7 @@ def test_exhaustive_cpp_samples(): simple_name = symbol.split("::")[-1] assert simple_name in definition finally: - mcp_client._resolve_and_check = original_resolve + mcp_client._resolve_and_check_result = original_resolve def test_ts_cpp_update_definition(tmp_path): asset_path = Path(__file__).parent / "assets" / "cpp_samples" / "component_registry.cpp" @@ -132,8 +132,8 @@ def test_ts_cpp_update_definition(tmp_path): cpp_file.write_text(cpp_content, encoding="utf-8") from src import mcp_client - original_resolve = mcp_client._resolve_and_check - mcp_client._resolve_and_check = lambda path: (Path(path), None) + original_resolve = mcp_client._resolve_and_check_result + mcp_client._resolve_and_check_result = lambda path: __import__("src").mcp_client.Result(data=Path(path), errors=[]) try: new_register_body = """void ComponentRegistry::Register(const std::string& type, ComponentCreator creator) { @@ -156,7 +156,7 @@ def test_ts_cpp_update_definition(tmp_path): assert "// Updated body" in definition assert "DEBUG: Registering" in definition finally: - mcp_client._resolve_and_check = original_resolve + mcp_client._resolve_and_check_result = original_resolve def test_exhaustive_gencpp_samples(): base_dir = Path(__file__).parent / "assets" / "gencpp_samples" @@ -184,8 +184,8 @@ def test_exhaustive_gencpp_samples(): } from src import mcp_client - original_resolve = mcp_client._resolve_and_check - mcp_client._resolve_and_check = lambda path: (Path(path), None) + original_resolve = mcp_client._resolve_and_check_result + mcp_client._resolve_and_check_result = lambda path: __import__("src").mcp_client.Result(data=Path(path), errors=[]) try: for filename, symbols in files_to_test.items(): @@ -207,7 +207,7 @@ def test_exhaustive_gencpp_samples(): simple_name = symbol.split("::")[-1] assert simple_name in definition finally: - mcp_client._resolve_and_check = original_resolve + mcp_client._resolve_and_check_result = original_resolve def test_ts_cpp_update_definition_gencpp(tmp_path): asset_path = Path(__file__).parent / "assets" / "gencpp_samples" / "parser.cpp" @@ -216,8 +216,8 @@ def test_ts_cpp_update_definition_gencpp(tmp_path): cpp_file.write_text(cpp_content, encoding="utf-8") from src import mcp_client - original_resolve = mcp_client._resolve_and_check - mcp_client._resolve_and_check = lambda path: (Path(path), None) + original_resolve = mcp_client._resolve_and_check_result + mcp_client._resolve_and_check_result = lambda path: __import__("src").mcp_client.Result(data=Path(path), errors=[]) try: # Update parser_pop @@ -239,4 +239,4 @@ def test_ts_cpp_update_definition_gencpp(tmp_path): definition = ts_cpp_get_definition(str(cpp_file), "parser_pop") assert "// HELLO FROM TEST" in definition finally: - mcp_client._resolve_and_check = original_resolve + mcp_client._resolve_and_check_result = original_resolve