docs(mcp_client): document new Result return types + nil-sentinel pattern

2026-06-11 23:37:32 -04:00
parent ef476c1058
commit bd35da11b6
1 changed files with 133 additions and 5 deletions
@@ -83,16 +83,39 @@ Returns `False` for any path the AI is not allowed to touch.

 The final gate. Resolves the path (handling symlinks, relative paths) and re-checks.

+> **As of 2026-06-11:** This section documents the **post-refactor**
+> `Result[Path]` signature, applied by the
+> `data_oriented_error_handling_20260606` track. The pre-refactor
+> `(Path | None, str)` tuple and the 30+ `assert p is not None` chain
+> in tool bodies (lines 304-794) are replaced. See the new
+> [Data-Oriented Error Handling (Fleury Pattern)](#data-oriented-error-handling-fleury-pattern)
+> section below for the full convention.
+
 ```python
-def _resolve_and_check(raw_path: str) -> tuple[Path | None, str]:
-    """Resolve raw_path and verify it passes the allowlist check."""
+def _resolve_and_check(raw_path: str) -> Result[Path]:
+    """Resolve raw_path and verify it passes the allowlist check.
+
+    On success: result.data is the real pathlib.Path; result.errors is [].
+    On failure: result.data is NIL_PATH; result.errors has 1 ErrorInfo
+      with kind=ErrorKind.PERMISSION (or NOT_FOUND / INVALID_INPUT).
+    """
    p = Path(raw_path).resolve()
    if not _is_allowed(p):
-        return None, f"ERROR: path not in allowlist: {raw_path}"
-    return p, ""
+        return Result(
+            data=NIL_PATH,
+            errors=[ErrorInfo(
+                kind=ErrorKind.PERMISSION,
+                message=f"path not in allowlist: {raw_path}",
+                source="mcp._resolve_and_check",
+            )],
+        )
+    return Result(data=p)
 ```

-Every tool function calls this first. If it returns an error, the tool returns the error string to the AI.
+Every tool function calls this first. If `result.errors` is non-empty, the
+tool returns its own `Result[data="", errors=resolved.errors]` to propagate
+the gate's error to the AI. The 3-layer security model is preserved
+unchanged — only the return-type contract evolves.

 ---

@@ -404,6 +427,111 @@ def test_my_code(monkeypatch):
 - **Tree-sitter parsing**: ~10-50ms per file for typical Python files. Cached in `_ast_cache` (mtime-based).
 - **Network tools** (`web_search`, `fetch_url`): 100ms-2s depending on the network.

+## Data-Oriented Error Handling (Fleury Pattern)
+
+The MCP tool layer follows the "errors are just cases" framework
+(Ryan Fleury). The canonical reference is
+[`conductor/code_styleguides/error_handling.md`](../conductor/code_styleguides/error_handling.md).
+
+### Result-Based Returns
+
+The 9 tool functions that previously returned `(Path | None, str)` tuples
+or raised exceptions now return `Result[str]` for content and
+`Result[Path]` for the resolution gate:
+
+| Function | Old signature | New signature |
+|---|---|---|
+| `_resolve_and_check(raw_path)` | `tuple[Path \| None, str]` | `Result[Path]` (data is real `Path` or `NIL_PATH`) |
+| `read_file(path)` | `str` (error prefix) | `Result[str]` (data is `""` on failure) |
+| `list_directory(path)` | `str` (error prefix) | `Result[str]` (data is `""` on failure) |
+| `search_files(...)` | `str` (error prefix) | `Result[str]` (data is `""` on failure) |
+| `get_file_summary(path)` | `str` (error prefix) | `Result[str]` (data is `""` on failure) |
+| `py_get_skeleton(path)` | `str` (error prefix) | `Result[str]` (data is `""` on failure) |
+| `py_get_code_outline(path)` | `str` (error prefix) | `Result[str]` (data is `""` on failure) |
+| `py_get_definition(path, name)` | `str` (error prefix) | `Result[str]` (data is `""` on failure) |
+| `py_get_imports(path)` | `str` (error prefix) | `Result[str]` (data is `""` on failure) |
+| (and 35 more — all 45 tools) | `str` (error prefix) | `Result[str]` (data is `""` on failure) |
+
+### Nil-Sentinel Pattern
+
+The `NIL_PATH` dataclass is the "empty path" — it has all default values
+(`exists=False`, `read_text=""`, `errors=[]`) and is safe to read from:
+
+```python
+@dataclass(frozen=True)
+class NilPath:
+    exists: bool = False
+    read_text: str = ""
+    errors: list[ErrorInfo] = field(default_factory=list)
+
+NIL_PATH = NilPath()  # module-level singleton
+```
+
+Callers that need a real `pathlib.Path` for filesystem operations check
+`if isinstance(result.data, NilPath): handle()` — but most callers just
+need the read text, and `NIL_PATH.read_text == ""` is fine for the AI
+model's purposes. This eliminates the 30+ `assert p is not None` chain
+in tool bodies (lines 304-794 pre-refactor) and the
+`if err or p is None: return err` patterns at the top of every tool
+function.
+
+### Dispatch Internals
+
+The `dispatch` and `async_dispatch` functions unwrap the `Result` before
+returning to the AI model (so the model's view of MCP errors is unchanged
+— it still sees error messages as plain strings):
+
+```python
+def dispatch(tool_name: str, tool_input: dict) -> str:
+    result = _DISPATCH_TABLE[tool_name](tool_input)
+    if not result.ok:
+        for err in result.errors:
+            _append_comms("WARN", "mcp_tool_error", [err.ui_message()])
+    return result.data or "".join(e.message for e in result.errors)
+```
+
+The `async_dispatch` path handles the case where `mcp_client` has no
+comms log: it just returns `result.data` (the empty success value) and
+the errors are silently dropped. The Result's `data` field is always
+readable (zero-initialized) so callers don't need defensive `is None`
+checks.
+
+### Example
+
+```python
+from src import mcp_client
+from src.result_types import ErrorKind
+
+r = mcp_client.read_file("/path/to/file.py")
+if r.errors:
+    for err in r.errors:
+        if err.kind == ErrorKind.PERMISSION:
+            log.warning("path not in allowlist: %s", err.message)
+        elif err.kind == ErrorKind.NOT_FOUND:
+            log.info("file not found: %s", err.message)
+        else:
+            log.error(err.ui_message())
+# use r.data regardless (it's the zero-initialized "" on failure)
+process(r.data)
+```
+
+### Security Invariant
+
+The 3-layer security model (Allowlist → Validate → Resolve) is **preserved
+unchanged** by the refactor. The new `Result` return type only changes
+the *signature* of the tool functions; the *behavior* (the 3 layers must
+all pass) is identical. The `ErrorKind.PERMISSION` value is what the
+model sees when the allowlist rejects a path — same error condition as
+the pre-refactor `"ERROR: path not in allowlist: ..."` string, just
+typed data instead of stringly-typed control flow.
+
+### See Also (in-doc)
+
+- [`conductor/code_styleguides/error_handling.md`](../conductor/code_styleguides/error_handling.md) — canonical styleguide (5 patterns, data model, decision tree, anti-patterns)
+- [`conductor/tracks/data_oriented_error_handling_20260606/spec.md`](../conductor/tracks/data_oriented_error_handling_20260606/spec.md) — the spec that introduced this pattern
+- [`docs/guide_ai_client.md`](guide_ai_client.md#data-oriented-error-handling-fleury-pattern) — same pattern in the provider layer
+- [`docs/guide_rag.md`](guide_rag.md#data-oriented-error-handling-fleury-pattern) — same pattern in the RAG engine
+
 ---

 ## See Also