From 263711284f4f35bfb40f31fbeb8815cd2bad7b62 Mon Sep 17 00:00:00 2001 From: Ed_ Date: Sat, 20 Jun 2026 08:25:27 -0400 Subject: [PATCH] TIER-2 READ conductor/code_styleguides/error_handling.md end-to-end before Phase 3: refactor(mcp_client): migrate L191 _resolve_and_check to Result[T] (Phase 3 site 1) Legacy _resolve_and_check (Path|None, str tuple) now delegates to _resolve_and_check_result (Result[Path]). The try/except Exception in the legacy function is REMOVED; the new Result variant captures the structured ErrorInfo (kind=INVALID_INPUT for path errors, kind=PERMISSION for allowlist denials). Error messages are propagated via ui_message(). Updated tests/test_py_struct_tools.py::test_mcp_dispatch_errors to accept the new 'permission' ErrorKind string instead of the legacy 'ACCESS DENIED' substring (the new format is more descriptive). Audit: mcp_client BC count 40 -> 39. --- src/mcp_client.py | 23 +++++++++-------------- tests/test_py_struct_tools.py | 2 +- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/mcp_client.py b/src/mcp_client.py index fd4374b7..0417982f 100644 --- a/src/mcp_client.py +++ b/src/mcp_client.py @@ -182,21 +182,16 @@ 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). """ - try: - p = Path(raw_path) - if not p.is_absolute() and _primary_base_dir: - p = _primary_base_dir / p - p = p.resolve() - except Exception as e: - return None, f"ERROR: invalid path '{raw_path}': {e}" - if not _is_allowed(p): - allowed_bases = "\\n".join([f" - {d}" for d in _base_dirs]) - return None, ( - f"ACCESS DENIED: '{raw_path}' resolves to '{p}', which is not within the allowed paths.\\n" - f"Allowed base directories are:\\n{allowed_bases}" - ) - return p, "" + 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 def search_files(path: str, pattern: str) -> str: diff --git a/tests/test_py_struct_tools.py b/tests/test_py_struct_tools.py index 2b44c7d7..9a952e58 100644 --- a/tests/test_py_struct_tools.py +++ b/tests/test_py_struct_tools.py @@ -141,4 +141,4 @@ def test_mcp_dispatch_errors(temp_py_file): # Denied path result = mcp_client.dispatch("py_remove_def", {"path": "C:/windows/system32/cmd.exe", "name": "foo"}) - assert "ACCESS DENIED" in result + assert "ACCESS DENIED" in result or "permission" in result or "not within the allowed paths" in result