feat(mcp): finalize Python structural tools with security checks and indentation normalization

2026-05-13 22:03:37 -04:00
parent 46a415c9a0
commit b9e4050175
7 changed files with 203 additions and 26 deletions
@@ -262,5 +262,5 @@ This file tracks all major tracks for the project. Each track has its own detail
 *Link: [./tracks/gui_2_cleanup_20260513/](./tracks/gui_2_cleanup_20260513/)*
 ---

- [ ] **Track: Add Python structural MCP tools (py_remove_def, py_add_def, py_move_def, py_region_wrap)**
+- [x] **Track: Add Python structural MCP tools (py_remove_def, py_add_def, py_move_def, py_region_wrap)**
 *Link: [./tracks/python_structural_mcp_tools_20260513/](./tracks/python_structural_mcp_tools_20260513/)*
@@ -17,6 +17,6 @@

 ## Phase 3: Testing & Validation
 - [x] Task: Create `tests/test_py_struct_tools.py` and write unit tests for the indentation shifter and AST extraction logic. [0393282]
- [~] Task: Write integration tests verifying the execution of the new tools via the MCP dispatcher.
- [ ] Task: Run the full test suite in batches to ensure no regressions in existing MCP routing.
- [ ] Task: Conductor - User Manual Verification 'Phase 3: Testing & Validation' (Protocol in workflow.md)
+- [x] Task: Write integration tests verifying the execution of the new tools via the MCP dispatcher. [0393282]
+- [x] Task: Run the full test suite in batches to ensure no regressions in existing MCP routing. [a88608d]
+- [x] Task: Conductor - User Manual Verification 'Phase 3: Testing & Validation' (Protocol in workflow.md) [a88608d]
@@ -75,7 +75,7 @@ DockId=0xAFC85805,2

 [Window][Theme]
 Pos=0,28
-Size=1230,1412
+Size=769,1172
 Collapsed=0
 DockId=0x00000010,3

@@ -103,26 +103,26 @@ Collapsed=0
 DockId=0x0000000D,0

 [Window][Discussion Hub]
-Pos=1232,28
-Size=1314,1412
+Pos=771,28
+Size=895,1172
 Collapsed=0
 DockId=0x00000006,1

 [Window][Operations Hub]
 Pos=0,28
-Size=1230,1412
+Size=769,1172
 Collapsed=0
 DockId=0x00000010,2

 [Window][Files & Media]
-Pos=1232,28
-Size=1314,1412
+Pos=771,28
+Size=895,1172
 Collapsed=0
 DockId=0x00000006,0

 [Window][AI Settings]
 Pos=0,28
-Size=1230,1412
+Size=769,1172
 Collapsed=0
 DockId=0x00000010,1

@@ -132,8 +132,8 @@ Size=416,325
 Collapsed=0

 [Window][MMA Dashboard]
-Pos=1232,28
-Size=1314,1412
+Pos=771,28
+Size=895,1172
 Collapsed=0
 DockId=0x00000006,2

@@ -409,7 +409,7 @@ DockId=0x00000006,1

 [Window][Project Settings]
 Pos=0,28
-Size=1230,1412
+Size=769,1172
 Collapsed=0
 DockId=0x00000010,0

@@ -653,14 +653,14 @@ Column 2  Width=150
 DockNode            ID=0x00000008 Pos=3125,170 Size=593,1157 Split=Y
  DockNode          ID=0x00000009 Parent=0x00000008 SizeRef=1029,147 Selected=0x0469CA7A
  DockNode          ID=0x0000000A Parent=0x00000008 SizeRef=1029,145 Selected=0xDF822E02
-DockSpace           ID=0xAFC85805 Window=0x079D3A04 Pos=0,28 Size=2546,1412 Split=X
+DockSpace           ID=0xAFC85805 Window=0x079D3A04 Pos=0,28 Size=1666,1172 Split=X
  DockNode          ID=0x00000003 Parent=0xAFC85805 SizeRef=2357,1183 Split=X
    DockNode        ID=0x0000000B Parent=0x00000003 SizeRef=404,1186 Split=X Selected=0xF4139CA2
      DockNode      ID=0x00000007 Parent=0x0000000B SizeRef=1512,858 Split=X Selected=0x8CA2375C
-        DockNode    ID=0x00000005 Parent=0x00000007 SizeRef=556,1681 Split=Y Selected=0x3F1379AF
-          DockNode  ID=0x00000010 Parent=0x00000005 SizeRef=983,1140 CentralNode=1 Selected=0x7BD57D6A
+        DockNode    ID=0x00000005 Parent=0x00000007 SizeRef=769,1681 Split=Y Selected=0x3F1379AF
+          DockNode  ID=0x00000010 Parent=0x00000005 SizeRef=983,1140 CentralNode=1 Selected=0x418C7449
          DockNode  ID=0x00000011 Parent=0x00000005 SizeRef=983,184 Selected=0x432BAE4E
-        DockNode    ID=0x00000006 Parent=0x00000007 SizeRef=1314,1681 Selected=0x6F2B5B04
+        DockNode    ID=0x00000006 Parent=0x00000007 SizeRef=895,1681 Selected=0x6F2B5B04
      DockNode      ID=0x0000000E Parent=0x0000000B SizeRef=1777,858 Selected=0x1D56B311
    DockNode        ID=0x0000000D Parent=0x00000003 SizeRef=435,1186 Selected=0x363E93D6
  DockNode          ID=0x00000004 Parent=0xAFC85805 SizeRef=488,1183 Split=X Selected=0x3AEC3498
@@ -9,5 +9,5 @@ active = "main"

 [discussions.main]
 git_commit = ""
-last_updated = "2026-05-13T15:44:47"
+last_updated = "2026-05-13T21:56:39"
 history = []
@@ -39,12 +39,15 @@ def find_definition_range(source: str, symbol_path: str) -> tuple[int, int] | No

 def shift_indentation(content: str, target_depth: int) -> str:
 """
-    Shifts the indentation of a code block to the target depth using 1-space units.
+    Shifts and normalizes the indentation of a code block to 1-space units.
+    Detects the base indentation and scales relative indentation to 1-space.
    [C: scripts/py_struct_tools.py:py_add_def]
    """
 lines = content.splitlines()
 if not lines:
  return ""
+ 
+ # 1. Find min indent of non-empty lines
 min_indent = sys.maxsize
 for line in lines:
  if line.strip():
@@ -53,12 +56,32 @@ def shift_indentation(content: str, target_depth: int) -> str:
    min_indent = indent
 if min_indent == sys.maxsize:
  min_indent = 0
+
+ # 2. Try to detect indentation unit (width)
+ indent_unit = 0
+ for line in lines:
+  if line.strip():
+   indent = len(line) - len(line.lstrip())
+   rel_indent = indent - min_indent
+   if rel_indent > 0:
+    if indent_unit == 0:
+     indent_unit = rel_indent
+    else:
+     import math
+     indent_unit = math.gcd(indent_unit, rel_indent)
+ 
+ if indent_unit == 0:
+  indent_unit = 1
+
 shifted_lines = []
 for line in lines:
  if line.strip():
-   shifted_lines.append(" " * target_depth + line[min_indent:])
+   indent = len(line) - len(line.lstrip())
+   rel_level = (indent - min_indent) // indent_unit
+   shifted_lines.append(" " * (target_depth + rel_level) + line.lstrip())
  else:
   shifted_lines.append("")
+ 
 return "\n".join(shifted_lines) + ("\n" if content.endswith("\n") else "")

 def py_remove_def(filepath: str, symbol_path: str) -> str:
@@ -1357,27 +1357,37 @@ 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":
-  return py_struct_tools.py_remove_def(path, str(tool_input.get("name", "")))
+  p, err = _resolve_and_check(path)
+  if err: return err
+  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
  return py_struct_tools.py_add_def(
-   path, 
+   str(p), 
   str(tool_input.get("name", "")), 
   str(tool_input.get("new_content", "")), 
   str(tool_input.get("anchor_type", "")), 
   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
  return py_struct_tools.py_move_def(
-   str(tool_input.get("src_path", "")),
-   str(tool_input.get("dest_path", "")),
+   str(p_src),
+   str(p_dest),
   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
  return py_struct_tools.py_region_wrap(
-   path,
+   str(p),
   int(tool_input.get("start_line", 1)),
   int(tool_input.get("end_line", 1)),
   str(tool_input.get("region_name", ""))
@@ -0,0 +1,144 @@
+import os
+import pytest
+from scripts import py_struct_tools
+from src import mcp_client
+
+@pytest.fixture
+def temp_py_file(tmp_path):
+    p = tmp_path / "sample.py"
+    content = """class MyClass:
+ \"\"\"Docstring.\"\"\"
+ def method1(self):
+  print("m1")
+
+def top_func():
+ \"\"\"Top doc.\"\"\"
+ print("top")
+"""
+    p.write_text(content, encoding="utf-8")
+    return str(p)
+
+def test_find_definition_range():
+    source = """class A:
+ def m(self): pass
+def f(): pass
+"""
+    assert py_struct_tools.find_definition_range(source, "A") == (1, 2)
+    assert py_struct_tools.find_definition_range(source, "A.m") == (2, 2)
+    assert py_struct_tools.find_definition_range(source, "f") == (3, 3)
+    assert py_struct_tools.find_definition_range(source, "nonexistent") is None
+
+def test_shift_indentation():
+    payload = "def f():\n  print('hi')" # 2-space
+    shifted = py_struct_tools.shift_indentation(payload, 1)
+    assert shifted == " def f():\n  print('hi')" # wait, shift_indentation strips min and prepends.
+    
+    # Let's re-test shift_indentation logic
+    # Original:
+    # line 1: 'def f():' (0 indent)
+    # line 2: '  print('hi')' (2 indent)
+    # min_indent = 0
+    # Prepend 1 space:
+    # ' def f():'
+    # '   print('hi')'
+    
+    # If payload was:
+    #   def f():
+    #     print('hi')
+    # min_indent = 2
+    # target_depth = 1
+    # ' def f():'
+    # '  print('hi')'
+    
+    payload2 = "  def f():\n    print('hi')"
+    shifted2 = py_struct_tools.shift_indentation(payload2, 1)
+    assert shifted2 == " def f():\n  print('hi')"
+
+def test_py_remove_def(temp_py_file):
+    err = py_struct_tools.py_remove_def(temp_py_file, "MyClass.method1")
+    assert err == ""
+    with open(temp_py_file, 'r') as f:
+        content = f.read()
+    assert "def method1" not in content
+    assert "class MyClass" in content
+
+def test_py_add_def(temp_py_file):
+    new_code = "def method2(self):\n print('m2')"
+    err = py_struct_tools.py_add_def(temp_py_file, "MyClass", new_code, "after", "method1")
+    assert err == ""
+    with open(temp_py_file, 'r') as f:
+        content = f.read()
+    assert "def method2" in content
+    # Check 1-space indentation
+    assert " def method2(self):" in content
+
+def test_py_region_wrap(temp_py_file):
+    err = py_struct_tools.py_region_wrap(temp_py_file, 6, 8, "MyRegion")
+    assert err == ""
+    with open(temp_py_file, 'r') as f:
+        content = f.read()
+    assert "#region: MyRegion" in content
+    assert "#endregion: MyRegion" in content
+
+def test_mcp_dispatch_integration(temp_py_file):
+    # Mock allowlist
+    mcp_client.configure([{"path": temp_py_file}])
+    
+    # Test py_remove_def
+    result = mcp_client.dispatch("py_remove_def", {"path": temp_py_file, "name": "top_func"})
+    assert result == ""
+    with open(temp_py_file, 'r') as f:
+        content = f.read()
+        assert "def top_func" not in content
+
+    # Test py_add_def (module level top)
+    result = mcp_client.dispatch("py_add_def", {
+        "path": temp_py_file, 
+        "name": "", 
+        "new_content": "def head_func():\n print('head')",
+        "anchor_type": "top"
+    })
+    assert result == ""
+    with open(temp_py_file, 'r') as f:
+        content = f.read()
+        assert content.startswith("def head_func")
+
+    # Test py_add_def (class bottom)
+    result = mcp_client.dispatch("py_add_def", {
+        "path": temp_py_file,
+        "name": "MyClass",
+        "new_content": "def tail_method(self):\n print('tail')",
+        "anchor_type": "bottom"
+    })
+    assert result == ""
+    with open(temp_py_file, 'r') as f:
+        content = f.read()
+        assert "def tail_method" in content
+        assert " def tail_method(self):" in content # Check indent
+
+    # Test py_move_def (cross-file simulated with same file)
+    # We move method1 to after tail_method
+    result = mcp_client.dispatch("py_move_def", {
+        "src_path": temp_py_file,
+        "dest_path": temp_py_file,
+        "name": "MyClass.method1",
+        "dest_name": "MyClass",
+        "anchor_type": "after",
+        "anchor_symbol": "tail_method"
+    })
+    assert result == ""
+    with open(temp_py_file, 'r') as f:
+        content = f.read()
+        # method1 should now be AFTER tail_method
+        assert content.find("def method1") > content.find("def tail_method")
+
+def test_mcp_dispatch_errors(temp_py_file):
+    mcp_client.configure([{"path": temp_py_file}])
+    
+    # Non-existent symbol
+    result = mcp_client.dispatch("py_remove_def", {"path": temp_py_file, "name": "NoSuchSymbol"})
+    assert "ERROR" in result or "not found" in result
+
+    # Denied path
+    result = mcp_client.dispatch("py_remove_def", {"path": "C:/windows/system32/cmd.exe", "name": "foo"})
+    assert "ACCESS DENIED" in result