diff --git a/docs/guide_mcp_client.md b/docs/guide_mcp_client.md index 3ea11155..418a39a9 100644 --- a/docs/guide_mcp_client.md +++ b/docs/guide_mcp_client.md @@ -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