refactor(mcp_client): obliterate legacy _resolve_and_check wrapper; migrate 5 callers to _resolve_and_check_result (Phase 3)

Phase 3 (1 of 9 cruft sites obliterated): The legacy wrapper _resolve_and_check(raw_path) returned tuple[Path|None, str], dropping the structured ErrorInfo from _resolve_and_check_result. Callers in dispatch_tool_call (py_remove_def, py_add_def, py_move_def, py_region_wrap) used the pattern 'p, err = _resolve_and_check(path); if err: return err' which is exactly the false drain the user wants obliterated. Migration: - DELETED: _resolve_and_check wrapper (lines 175-188 in src/mcp_client.py) - UPDATED: 5 callers in dispatch_tool_call now call _resolve_and_check_result directly with .ok check + NilPath check + structured error routing - UPDATED: 4 test files that monkey-patched _resolve_and_check to mock the Result helper instead: - test_mcp_ts_integration.py (1 mock) - test_ts_c_tools.py (2 mocks) - test_ts_cpp_tools.py (8 mocks) - test_cruft_removal.py (NEW; 4 tests including the wrapper-obliterated invariant + the audit-script-finds-zero invariant + 2 dispatch tests) Test result: 51/51 pass (31 baseline + 16 heuristic + 4 cruft). Audit gate: src/mcp_client.py --strict exits 0 (no new violations introduced). Baseline audit: --include-baseline --strict exits 1 only due to 4 pre-existing non-baseline INTERNAL_RETHROW sites in outline_tool.py / warmup.py / vendor_capabilities.py (out of scope per spec). The wrapper IS DELETED. No pass-through. No backward compat. The dead code dies.
2026-06-20 19:48:00 -04:00
parent 3967a42071
commit 5c871dacac
5 changed files with 136 additions and 55 deletions
@@ -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", ""))
@@ -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}"
+    )
@@ -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
@@ -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
@@ -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