diff --git a/conductor/tracks/data_oriented_error_handling_20260606/plan.md b/conductor/tracks/data_oriented_error_handling_20260606/plan.md new file mode 100644 index 00000000..9b70410c --- /dev/null +++ b/conductor/tracks/data_oriented_error_handling_20260606/plan.md @@ -0,0 +1,1956 @@ +# Data-Oriented Error Handling (Fleury Pattern) — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Introduce Ryan Fleury's "errors are just cases" framework as a project convention. Replace `Optional[T]` and exception-based control flow with `Result[T, ErrorInfo]` dataclasses and nil-sentinel dataclasses in 3 high-value subsystems (`src/mcp_client.py`, `src/ai_client.py`, `src/rag_engine.py`). Mark the public `ai_client.send()` as `@deprecated`; introduce `ai_client.send_result()` as the new public API. Codify the patterns in `conductor/code_styleguides/error_handling.md` and `conductor/product-guidelines.md` so future plans can incrementally migrate the remaining `src/` files. + +**Architecture:** Data-oriented. `Result[T]` is a `@dataclass(frozen=True)` with two fields: `data: T` (zero-initialized = success) and `errors: list[ErrorInfo]` (side-channel; empty = success). `ErrorInfo` is a `@dataclass(frozen=True)` with `kind: ErrorKind`, `message: str`, `source: str`, `original: BaseException | None`. Nil-sentinel dataclasses (`NilPath`, `NilRAGState`) replace `None` for missing data. SDK boundaries (`send_openai_compatible` in `src/openai_compatible.py`, `classify_dashscope_error` in `src/qwen_adapter.py`) keep raising — the calling `_send__result()` catches and converts to `ErrorInfo`. Deprecation uses `typing_extensions.deprecated` (Python 3.11+ backport of `@warnings.deprecated`). + +**Tech Stack:** Python 3.11+, pytest, `typing_extensions>=4.5.0,<5.0.0` (new dep), stdlib `dataclasses`, `enum`, `warnings`, `typing`. **1-space indentation mandatory.** No comments in production code (per project style). + +**Reference:** See `conductor/tracks/data_oriented_error_handling_20260606/spec.md` for the full design, post-tracks baseline, per-file refactor details, and coordination with pending tracks. + +--- + +## File Structure + +| File | Action | Responsibility | +|---|---|---| +| `src/result_types.py` | Create | `ErrorKind` enum, `ErrorInfo` dataclass, `Result[T]` dataclass, `NilPath` + `NilRAGState` singletons | +| `conductor/code_styleguides/error_handling.md` | Create | Canonical reference (~400 lines): 5 patterns, Python mappings, decision tree, examples | +| `conductor/product-guidelines.md` | Modify | Add "Data-Oriented Error Handling" section | +| `conductor/workflow.md` | Modify | Add note in Code Style section | +| `docs/guide_ai_client.md` | Modify | New "Data-Oriented Error Handling (Fleury Pattern)" section + deprecation note | +| `docs/guide_mcp_client.md` | Modify | Document new `Result` return types + nil-sentinel pattern | +| `src/mcp_client.py` | Modify | ~60 sites: `(p, err)` tuples → `Result[Path]`; `assert p is not None` chain → nil-sentinel paths; tool functions return `Result[str]` | +| `src/ai_client.py` | Modify | `ProviderError` class removed; `_classify__error()` returns `ErrorInfo`; `_send_()` renamed to `_send__result()` returning `Result[str]` (8 vendors); `send()` marked `@deprecated`; new `send_result()` public API | +| `src/rag_engine.py` | Modify | RAG methods return `Result`; `NilRAGState` sentinel; broad `except Exception` blocks become `ErrorInfo` entries | +| `pyproject.toml` | Modify | Add `typing_extensions>=4.5.0,<5.0.0` dependency | +| `tests/conftest.py` | Modify | Add `filterwarnings` entry to silence `send()` deprecation in existing tests during the transition | +| `tests/test_result_types.py` | Create | 8+ unit tests for `Result`, `ErrorInfo`, `NilPath`, `NilRAGState` | +| `tests/test_mcp_client_paths.py` | Create | 6+ tests for new `Result` return types in `mcp_client.py` | +| `tests/test_ai_client_result.py` | Create | 8+ tests for new `Result` API + `_send__result()` functions | +| `tests/test_rag_engine_result.py` | Create | 4+ tests for new `Result` return types in `rag_engine.py` | +| `tests/test_deprecation_warnings.py` | Create | Tests verifying `send()` emits exactly one `DeprecationWarning` per call site | +| `conductor/tracks.md` | Modify | Add entry for this track (item 0e) + placeholder for `public_api_migration_20260606` follow-up | + +--- + +# Phase 1: Foundation + +> Goal: New `src/result_types.py` exists with passing unit tests. New `conductor/code_styleguides/error_handling.md` written. `product-guidelines.md` and `workflow.md` updated. `typing_extensions` added to `pyproject.toml`. Post-tracks baseline verified. + +--- + +## Task 1.1: Post-tracks baseline verification + +**Files:** none (verification only) + +- [ ] **Step 1: Confirm the 3 pending tracks have merged** + +Run: +```bash +git log --oneline -1 -- conductor/tracks/startup_speedup_20260606/ 2>/dev/null && echo "startup_speedup: merged" || echo "MISSING: startup_speedup" +git log --oneline -1 -- conductor/tracks/test_batching_refactor_20260606/ 2>/dev/null && echo "test_batching: merged" || echo "MISSING: test_batching" +git log --oneline -1 -- conductor/tracks/qwen_llama_grok_integration_20260606/ 2>/dev/null && echo "qwen_llama_grok: merged" || echo "MISSING: qwen_llama_grok" +``` + +Expected: all 3 tracks show merged. + +- [ ] **Step 2: Confirm the new files from the qwen_track exist** + +Run: +```bash +test -f src/vendor_capabilities.py && echo "vendor_capabilities.py: OK" || echo "MISSING" +test -f src/openai_compatible.py && echo "openai_compatible.py: OK" || echo "MISSING" +test -f src/qwen_adapter.py && echo "qwen_adapter.py: OK" || echo "MISSING" +``` + +Expected: all 3 files exist. + +- [ ] **Step 3: Confirm src/ai_client.py has the new vendor functions** + +Run: `uv run python -c "from src import ai_client; print(hasattr(ai_client, '_send_qwen'), hasattr(ai_client, '_send_llama'), hasattr(ai_client, '_send_grok'), hasattr(ai_client, '_send_minimax'), hasattr(ai_client, 'ProviderError'))"` +Expected: `True True True True True` + +- [ ] **Step 4: If any check fails, STOP and report a coordination issue** + +If `startup_speedup`, `test_batching_refactor`, or `qwen_llama_grok` is not merged, the data-oriented refactor cannot proceed safely. Report to the Tier 2 Tech Lead; do not proceed. + +- [ ] **Step 5: Commit nothing (verification only)** + +No commit. This task is pure baseline verification. + +--- + +## Task 1.2: Add typing_extensions dependency + +**Files:** +- Modify: `pyproject.toml` (find the `dependencies = [` block) + +- [ ] **Step 1: Read the dependencies block** + +Run: `manual-slop_get_file_slice path=pyproject.toml start_line=1 end_line=80` (find the `dependencies` list). + +- [ ] **Step 2: Add typing_extensions to the dependencies list** + +Add (1-space indent, near other backport-style deps): +```toml +"typing_extensions>=4.5.0,<5.0.0", +``` + +- [ ] **Step 3: Verify the dependency resolves** + +Run: `uv lock && uv sync 2>&1 | tail -5` +Expected: `typing_extensions` installs successfully. + +- [ ] **Step 4: Verify the deprecation decorator is importable** + +Run: `uv run python -c "from typing_extensions import deprecated; print(deprecated)"` +Expected: prints the `deprecated` function. + +- [ ] **Step 5: Commit** + +```bash +git add pyproject.toml uv.lock +git commit -m "chore(deps): add typing_extensions>=4.5.0 for @deprecated decorator" +``` + +--- + +## Task 1.3: Write red tests for src/result_types.py + +**Files:** +- Create: `tests/test_result_types.py` + +- [ ] **Step 1: Create the test file with 9 tests** + +```python +import pytest +from src.result_types import ( + ErrorKind, + ErrorInfo, + Result, + NilPath, + NilRAGState, +) + +def test_error_kind_enum_has_expected_values() -> None: + assert ErrorKind.NETWORK.value == "network" + assert ErrorKind.AUTH.value == "auth" + assert ErrorKind.RATE_LIMIT.value == "rate_limit" + assert ErrorKind.NOT_FOUND.value == "not_found" + assert ErrorKind.UNKNOWN.value == "unknown" + +def test_error_info_ui_message_with_source() -> None: + e = ErrorInfo(kind=ErrorKind.RATE_LIMIT, message="too many requests", source="mcp.read_file") + assert e.ui_message() == "[mcp.read_file] rate_limit: too many requests" + +def test_error_info_ui_message_without_source() -> None: + e = ErrorInfo(kind=ErrorKind.AUTH, message="bad key") + assert e.ui_message() == "auth: bad key" + +def test_result_ok_when_no_errors() -> None: + r: Result[str] = Result(data="hello") + assert r.ok is True + assert r.data == "hello" + assert r.errors == [] + +def test_result_not_ok_when_errors_present() -> None: + r: Result[str] = Result(data="", errors=[ErrorInfo(kind=ErrorKind.NOT_FOUND, message="nope", source="test")]) + assert r.ok is False + +def test_result_with_error_returns_new_result_with_appended_error() -> None: + r1: Result[str] = Result(data="hello") + err = ErrorInfo(kind=ErrorKind.NETWORK, message="timeout", source="test") + r2 = r1.with_error(err) + assert r1.errors == [] # original is unchanged (frozen) + assert r2.errors == [err] + assert r2.data == "hello" + +def test_result_with_data_replaces_data_keeps_errors() -> None: + r1: Result[str] = Result(data="", errors=[ErrorInfo(kind=ErrorKind.NETWORK, message="x", source="t")]) + r2 = r1.with_data("new value") + assert r2.data == "new value" + assert len(r2.errors) == 1 + +def test_result_is_frozen() -> None: + from dataclasses import FrozenInstanceError + r: Result[str] = Result(data="x") + with pytest.raises(FrozenInstanceError): + r.data = "y" + +def test_nil_path_singleton_has_default_values() -> None: + assert NilPath.exists is False + assert NilPath.read_text == "" + assert NilPath.errors == [] + assert isinstance(NilPath(), NilPath) + +def test_nil_rag_state_singleton_has_default_values() -> None: + assert NilRAGState.enabled is False + assert NilRAGState.is_empty_result is True + assert NilRAGState.errors == [] +``` + +- [ ] **Step 2: Run, confirm all 10 tests fail** + +Run: `uv run pytest tests/test_result_types.py -v` +Expected: 10 tests FAIL with `ImportError: cannot import name 'ErrorKind' from 'src.result_types'`. + +- [ ] **Step 3: Commit (red)** + +```bash +git add tests/test_result_types.py +git commit -m "test(result_types): add red tests for Result, ErrorInfo, NilPath, NilRAGState" +``` + +--- + +## Task 1.4: Implement src/result_types.py + +**Files:** +- Create: `src/result_types.py` + +- [ ] **Step 1: Create the file with the full data model** + +```python +from dataclasses import dataclass, field +from enum import Enum +from typing import Generic, TypeVar + +T = TypeVar("T") + +class ErrorKind(str, Enum): + NETWORK = "network" + AUTH = "auth" + QUOTA = "quota" + RATE_LIMIT = "rate_limit" + BALANCE = "balance" + PERMISSION = "permission" + NOT_FOUND = "not_found" + INVALID_INPUT = "invalid_input" + UNKNOWN = "unknown" + CONFIG = "config" + INTERNAL = "internal" + +@dataclass(frozen=True) +class ErrorInfo: + kind: ErrorKind + message: str + source: str = "" + original: BaseException | None = None + def ui_message(self) -> str: + src = f"[{self.source}] " if self.source else "" + return f"{src}{self.kind.value}: {self.message}" + +@dataclass(frozen=True) +class Result(Generic[T]): + data: T + errors: list[ErrorInfo] = field(default_factory=list) + @property + def ok(self) -> bool: + return not self.errors + def with_error(self, err: ErrorInfo) -> "Result[T]": + return Result(data=self.data, errors=[*self.errors, err]) + def with_data(self, new_data: T) -> "Result[T]": + return Result(data=new_data, errors=list(self.errors)) + +@dataclass(frozen=True) +class NilPath: + exists: bool = False + read_text: str = "" + errors: list[ErrorInfo] = field(default_factory=list) + +NIL_PATH = NilPath() + +@dataclass(frozen=True) +class NilRAGState: + enabled: bool = False + is_empty_result: bool = True + errors: list[ErrorInfo] = field(default_factory=list) + +NIL_RAG_STATE = NilRAGState() + +OK = Result(data=None) +``` + +- [ ] **Step 2: Run, confirm all 10 tests pass** + +Run: `uv run pytest tests/test_result_types.py -v` +Expected: 10 tests PASS. + +- [ ] **Step 3: Verify import time is fast (< 50ms)** + +Run: +```bash +uv run python -c "import time; t=time.perf_counter(); import src.result_types; print(f'result_types: {(time.perf_counter()-t)*1000:.1f}ms')" +``` + +Expected: < 50ms (no heavy SDK imports at the top of this file). + +- [ ] **Step 4: Commit (green)** + +```bash +git add src/result_types.py +git commit -m "feat(result_types): add ErrorKind, ErrorInfo, Result[T], NilPath, NilRAGState" +``` + +--- + +## Task 1.5: Verify src/result_types.py passes the main-thread import audit + +**Files:** none (verification only) + +- [ ] **Step 1: Run the audit script** + +Run: `uv run python scripts/audit_main_thread_imports.py 2>&1 | tail -10` +Expected: exits 0 (or no new violations introduced by `src/result_types.py`). + +If the audit flags `src/result_types.py`, the import is too heavy. Inspect the file for any `import X` of `google.genai`, `anthropic`, `openai`, `fastapi`, `numpy`, etc. Fix by removing or deferring. + +- [ ] **Step 2: Commit nothing (verification only)** + +If a fix was needed, commit it: +```bash +git add src/result_types.py +git commit -m "fix(result_types): pass main-thread import audit" +``` + +--- + +## Task 1.6: Create conductor/code_styleguides/error_handling.md + +**Files:** +- Create: `conductor/code_styleguides/error_handling.md` + +- [ ] **Step 1: Create the canonical reference file with the 5 patterns, Python mappings, decision tree, and examples** + +```markdown +# Code Style: Data-Oriented Error Handling + +> **For agentic workers:** This is the canonical reference for the "errors are just cases" framework, based on Ryan Fleury's [The Easiest Way To Handle Errors Is To Not Have Them](https://www.dgtlgrove.com/p/the-easiest-way-to-handle-errors). New code in this project follows these patterns. Existing code is migrated incrementally in dedicated tracks. + +## The 5 Patterns + +### 1. Nil-Sentinel Dataclasses (replaces `None`) + +When a function would "return None" in conventional Python, return a nil-sentinel dataclass instead. The sentinel has all default values (zero-initialized) and is safe to read from. + +```python +from dataclasses import dataclass, field + +@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 don't need `if x is None:` checks; they can call `x.read_text` and get `""` on the nil path. + +**Convention:** `NIL_*` (uppercase) is the module-level singleton. `Nil*` (PascalCase) is the class. Frozen dataclass prevents runtime mutation. + +### 2. Zero-Initialization (via `@dataclass` defaults) + +Fresh memory from the OS is zero-initialized. In Python, `@dataclass` with field defaults achieves the same: the data is in a valid "empty" state without any explicit constructor logic. + +```python +@dataclass(frozen=True) +class String8: + text: str = "" + size: int = 0 +``` + +Code that consumes `String8` (e.g., a for-loop bounded by `size`) works correctly with the zero-initialized instance. + +**Convention:** Mutable defaults use `field(default_factory=list)` (NOT `= []`, which is shared across instances). + +### 3. Fail Early (push validation to shallow stack frames) + +Don't defer error checks to deep in the call stack. Push them to the entry point so the user knows ASAP if the operation cannot succeed. + +```python +def do_thing(path: Path) -> Result[str]: + resolved = _resolve_path(path) # validation happens HERE, not deeper + if not resolved.ok: + return Result(data="", errors=resolved.errors) + ... +``` + +**Convention:** `assert` at entry points for invariants. Early `return` for user-facing errors. `try/finally` (Python's analog to `goto defer`) for cleanup. + +### 4. AND over OR (Result with side-channel errors; no sum types) + +Instead of `Union[T, E]` or `Result`, return a struct with BOTH data and errors as parallel fields: + +```python +@dataclass(frozen=True) +class Result(Generic[T]): + data: T # the happy-path result (zero-initialized on failure) + errors: list[ErrorInfo] = field(default_factory=list) # side-channel; empty = success +``` + +Callers: +```python +r = do_thing(path) +if r.errors: + for err in r.errors: log(err.ui_message()) +# use r.data regardless (it's the zero-initialized value on failure) +``` + +**Convention:** `Result` is generic over `T` (the success data) but NOT over the error type. Errors are always `list[ErrorInfo]` (a side-channel list, not a tagged sum). This collapses the bifurcated `if r.ok: ... else: ...` codepaths into a single flat codepath. + +### 5. Error Info as Side-Channel (not as exception) + +Errors flow as DATA in the `Result` struct, not as exceptions. SDK boundaries (which must catch vendor exceptions) convert them to `ErrorInfo`: + +```python +@dataclass(frozen=True) +class ErrorInfo: + kind: ErrorKind + message: str + source: str = "" + original: BaseException | None = None + def ui_message(self) -> str: + src = f"[{self.source}] " if self.source else "" + return f"{src}{self.kind.value}: {self.message}" +``` + +**Convention:** `ErrorInfo` is the new error type. `ProviderError` (the old exception) is removed. SDK helpers (`_classify__error()`) RETURN `ErrorInfo` instead of raising. + +## Decision Tree + +``` +Need to represent "missing or failed"? +| ++-- Is the value a "data" value (not a control-flow signal)? +| +-- Use a Result dataclass (data + errors list) +| +-- Use a nil-sentinel dataclass (zero-initialized) +| ++-- Is the value a control-flow signal (e.g., "abort" or "skip")? +| +-- Use a boolean (or enum) +| +-- Use Optional[bool] / Optional[Enum] ONLY if the absence is meaningful +| ++-- Is the failure "unrecoverable" (programmer error, not runtime condition)? +| +-- Use assert (debug builds) +| +-- Use raise (only for programmer errors like KeyError on a known dict) +| ++-- Does the SDK raise an exception you can't avoid? + +-- Catch at the boundary; convert to ErrorInfo inside a Result +``` + +## Anti-Patterns + +**DON'T do these things:** + +1. **DON'T** use `Optional[X]` for "this might fail at runtime". Use `Result[X]` instead. +2. **DON'T** use `None` as a sentinel for "no result". Use a nil-sentinel dataclass. +3. **DON'T** raise a custom exception class for runtime failures. Catch SDK exceptions and return `ErrorInfo`. +4. **DON'T** use `Union[T, E]` (sum type). Use a struct with parallel fields (AND over OR). +5. **DON'T** have `if x is None: handle; else: use_x` patterns in production code. The nil-sentinel makes them unnecessary. +6. **DON'T** catch `except Exception` and silently swallow. Convert to `ErrorInfo` and return in the `Result`. + +## Examples + +The 3 refactored subsystems demonstrate each pattern in context. See: +- `src/mcp_client.py:205-294` — `read_file`, `list_directory`, `search_files` return `Result[str]`; `(p, err)` tuples become `Result[Path]` +- `src/ai_client.py` — `_send__result()` returns `Result[str]`; `send_result()` is the new public API; `send()` is `@deprecated` +- `src/rag_engine.py:100-180` — `_init_vector_store_result`, `_validate_collection_dim_result` return `Result[None]` + +## When to Use This Convention + +**Use it for:** +- New public APIs (any function that can fail at runtime and the caller might care) +- New internal functions where the caller benefits from knowing the failure (vs. just propagating `None`) + +**Don't use it for:** +- Constructors (`__init__`) that fail with programmer errors (use `assert` or `raise` for these) +- Trivial getters that can't fail (`get_name() -> str` doesn't need a Result) +- Performance-critical hot paths where the overhead of the dataclass allocation is measurable (rare; benchmark first) + +## Migration Playbook + +When converting existing code: + +1. Identify the `Optional[X]` return type or the `raise` statement. +2. Define a `Result` dataclass (or use the existing one) with `data: X` and `errors: list[ErrorInfo]`. +3. Replace `None` returns with `Result(data=NIL_X, errors=[...])` or `Result(data=zero_value, errors=[...])`. +4. Replace `raise X` with `return Result(data=zero_value, errors=[ErrorInfo(kind=..., message=...)])`. +5. Update the caller to check `result.errors` instead of `is None` / `try/except`. +6. Add a test that verifies both the success and failure paths return the right `Result`. + +## See Also + +- `conductor/tracks/data_oriented_error_handling_20260606/spec.md` — the spec that established this convention +- `docs/guide_ai_client.md` "Data-Oriented Error Handling (Fleury Pattern)" — the in-context guide for the provider layer +- `docs/guide_mcp_client.md` — the in-context guide for the MCP tool layer +- Ryan Fleury's [original article](https://www.dgtlgrove.com/p/the-easiest-way-to-handle-errors) — the philosophical foundation +``` + +- [ ] **Step 2: Commit** + +```bash +git add conductor/code_styleguides/error_handling.md +git commit -m "docs(styleguide): add canonical reference for Data-Oriented Error Handling" +``` + +--- + +## Task 1.7: Update conductor/product-guidelines.md + +**Files:** +- Modify: `conductor/product-guidelines.md` (add a new top-level section) + +- [ ] **Step 1: Find where to insert the new section** + +Run: `grep -n "^## " conductor/product-guidelines.md | head -20` to see the existing top-level sections. + +- [ ] **Step 2: Add the new section before the last major section (or after "Code Standards & Architecture")** + +Insert the new section: + +```markdown +## Data-Oriented Error Handling + +The codebase follows the "errors are just cases" framework from Ryan Fleury's +[The Easiest Way To Handle Errors](https://www.dgtlgrove.com/p/the-easiest-way-to-handle-errors). +The canonical reference (with code examples) is in +`conductor/code_styleguides/error_handling.md`. Key principles: + +- **Result dataclasses** instead of `Optional[T]` or exception-based control flow. +- **Nil-sentinel dataclasses** instead of `None`. +- **Zero-initialized fields** via `@dataclass` defaults. +- **Fail early**: validation at the entry point, not deep in the call stack. +- **AND over OR**: return a struct with data + side-channel errors, not a sum type. +- **Exceptions reserved for the SDK boundary**: SDK errors are caught and converted + to `ErrorInfo` dataclasses; the rest of the application works with data, not control flow. + +This convention is established incrementally. The 2026-06-06 track applied it to +`mcp_client.py`, `ai_client.py`, and `rag_engine.py`. Future tracks will apply it +to the remaining `src/` files (see `conductor/tracks/data_oriented_error_handling_20260606/spec.md` +§12.2 for the prioritized list). +``` + +- [ ] **Step 3: Commit** + +```bash +git add conductor/product-guidelines.md +git commit -m "docs(product-guidelines): add Data-Oriented Error Handling section" +``` + +--- + +## Task 1.8: Update conductor/workflow.md + +**Files:** +- Modify: `conductor/workflow.md` (add a note in the Code Style section) + +- [ ] **Step 1: Find the Code Style section** + +Run: `grep -n "Code Style\|^## Code\|^- \`1-space" conductor/workflow.md | head -10` to locate the section. + +- [ ] **Step 2: Add a bullet pointing to the new styleguide** + +Add a new bullet in the Code Style section: +```markdown +- For error handling, see [Data-Oriented Error Handling](./code_styleguides/error_handling.md). +``` + +- [ ] **Step 3: Commit** + +```bash +git add conductor/workflow.md +git commit -m "docs(workflow): link to error_handling.md styleguide from Code Style section" +``` + +--- + +## Task 1.9: Phase 1 checkpoint commit and git note + +**Files:** none (commit + note only) + +- [ ] **Step 1: Run all Phase 1 tests** + +Run: `uv run pytest tests/test_result_types.py -v` +Expected: 10 tests PASS. + +- [ ] **Step 2: Verify no regressions in existing tests** + +Run: `uv run pytest tests/ -q --timeout=60 2>&1 | tail -10` +Expected: no NEW failures (pre-existing failures unchanged). + +- [ ] **Step 3: Create the checkpoint commit and git note** + +```bash +git add -A +if ! git diff --cached --quiet; then git commit -m "conductor(checkpoint): Phase 1 complete - foundation + styleguide"; fi +SHA=$(git log -1 --format="%H") +git notes add -m "Phase 1 checkpoint: data_oriented_error_handling_20260606 + +Foundation: +- src/result_types.py: ErrorKind, ErrorInfo, Result[T], NilPath, NilRAGState, OK +- 10 unit tests pass +- conductor/code_styleguides/error_handling.md (~400 lines, canonical ref) +- conductor/product-guidelines.md + workflow.md updated +- typing_extensions>=4.5.0 dep added +- Post-tracks baseline verified (startup_speedup, test_batching, qwen_llama_grok merged) + +Next: Phase 2 (mcp_client.py refactor)." "$SHA" +``` + +- [ ] **Step 4: Update state.toml phase_1 status** + +Edit `conductor/tracks/data_oriented_error_handling_20260606/state.toml` line: +```toml +phase_1 = { status = "pending", checkpointsha = "", name = "Foundation: result_types module + style guide + baseline check" } +``` +Change to: +```toml +phase_1 = { status = "completed", checkpointsha = "", name = "Foundation: result_types module + style guide + baseline check" } +``` + +```bash +git add conductor/tracks/data_oriented_error_handling_20260606/state.toml +git commit -m "conductor(plan): mark Phase 1 complete in data_oriented_error_handling_20260606" +``` + +--- + +# Phase 2: `src/mcp_client.py` Refactor + +> Goal: `(p, err)` tuple returns become `Result[Path]`. Tool functions return `Result[str]`. The 30+ `assert p is not None` chain (lines 304-794) is removed. Existing tests pass unchanged. + +--- + +## Task 2.1: Baseline: verify existing mcp_client tests pass before refactor + +**Files:** none (verification only) + +- [ ] **Step 1: Run tests/test_mcp_client.py and record pass/fail counts** + +Run: `uv run pytest tests/test_mcp_client.py -v 2>&1 | tail -20` +Expected: all existing tests pass. + +- [ ] **Step 2: Record pass count in state.toml under `[mcp_client_refactor_stats]`** + +- [ ] **Step 3: Commit nothing (baseline)** + +--- + +## Task 2.2: Write red tests for mcp_client.py new Result return types + +**Files:** +- Create: `tests/test_mcp_client_paths.py` + +- [ ] **Step 1: Create the test file with 6 tests** + +```python +from unittest.mock import patch, MagicMock +import pytest +from src import mcp_client +from src.result_types import Result, ErrorKind, ErrorInfo, NilPath + +def test_resolve_and_check_returns_result_not_tuple() -> None: + r = mcp_client._resolve_and_check("/tmp/test.py") + assert isinstance(r, Result) + assert hasattr(r, "data") + assert hasattr(r, "errors") + assert hasattr(r, "ok") + +def test_resolve_and_check_success_returns_real_path() -> None: + with patch.object(mcp_client, "_resolve_path", return_value=MagicMock()): + with patch.object(mcp_client, "_is_in_allowed_base", return_value=True): + p = MagicMock() + p.exists.return_value = True + p.is_file.return_value = True + r = mcp_client._resolve_and_check("/tmp/test.py") + assert r.ok + assert r.data is not None + assert not isinstance(r.data, NilPath) + +def test_resolve_and_check_invalid_path_returns_nil_with_error() -> None: + from src import mcp_client + r = mcp_client._resolve_and_check("/nonexistent/path/that/does/not/exist/anywhere") + assert not r.ok + assert len(r.errors) >= 1 + assert isinstance(r.data, NilPath) + +def test_read_file_returns_result_str() -> None: + r = mcp_client.read_file("/tmp/nonexistent_test_file_xyz.py") + assert isinstance(r, Result) + assert hasattr(r, "data") + assert r.data == "" + assert len(r.errors) >= 1 + assert r.errors[0].kind in (ErrorKind.NOT_FOUND, ErrorKind.PERMISSION, ErrorKind.INVALID_INPUT) + +def test_list_directory_returns_result_str() -> None: + r = mcp_client.list_directory("/nonexistent_dir_xyz") + assert isinstance(r, Result) + assert r.data == "" + assert len(r.errors) >= 1 + +def test_search_files_returns_result_str() -> None: + r = mcp_client.search_files("/nonexistent_dir_xyz", "*.py") + assert isinstance(r, Result) + assert r.data == "" + assert len(r.errors) >= 1 +``` + +- [ ] **Step 2: Run, confirm 6 tests fail** + +Run: `uv run pytest tests/test_mcp_client_paths.py -v` +Expected: 6 tests FAIL with `AssertionError` (the current `_resolve_and_check` returns a tuple, not a Result). + +- [ ] **Step 3: Commit (red)** + +```bash +git add tests/test_mcp_client_paths.py +git commit -m "test(mcp_client): add red tests for new Result return types" +``` + +--- + +## Task 2.3: Refactor _resolve_and_check in src/mcp_client.py + +**Files:** +- Modify: `src/mcp_client.py` (replace `_resolve_and_check` function) + +- [ ] **Step 1: Read the current _resolve_and_check to understand the structure** + +Run: `manual-slop_py_get_definition path=src/mcp_client.py name=_resolve_and_check` + +- [ ] **Step 2: Replace the function with the Result-returning version** + +```python +def _resolve_and_check(path: str) -> Result[Path]: + try: + p = _resolve_path(path) + except Exception as e: + return Result(data=NilPath(), errors=[ErrorInfo(kind=ErrorKind.INVALID_INPUT, message=str(e), source="mcp._resolve_and_check", original=e)]) + if not _is_in_allowed_base(p): + return Result(data=NilPath(), errors=[ErrorInfo(kind=ErrorKind.PERMISSION, message=f"path '{path}' not in allowed base", source="mcp._resolve_and_check")]) + return Result(data=p) +``` + +(Add the import `from src.result_types import Result, ErrorInfo, ErrorKind, NilPath` at the top of `src/mcp_client.py` if not already present.) + +- [ ] **Step 3: Add `Result`, `ErrorInfo`, `ErrorKind`, `NilPath` to mcp_client.py's imports** + +Use `manual-slop_edit_file` to add to the top of `src/mcp_client.py`: +```python +from src.result_types import ErrorInfo, ErrorKind, NilPath, Result +``` + +(Place alphabetically with other `from src.*` imports.) + +- [ ] **Step 4: Run the new tests; confirm 3 of 6 pass (the _resolve_and_check ones)** + +Run: `uv run pytest tests/test_mcp_client_paths.py -v` +Expected: 3 tests PASS (`test_resolve_and_check_*`); 3 tests still FAIL (`test_read_file_*`, `test_list_directory_*`, `test_search_files_*`). + +- [ ] **Step 5: Commit** + +```bash +git add src/mcp_client.py +git commit -m "refactor(mcp_client): _resolve_and_check returns Result[Path]" +``` + +--- + +## Task 2.4: Refactor read_file, list_directory, search_files to return Result[str] + +**Files:** +- Modify: `src/mcp_client.py` (replace 3 tool functions) + +- [ ] **Step 1: Refactor read_file to return Result[str]** + +```python +def read_file(path: str) -> Result[str]: + resolved = _resolve_and_check(path) + if not resolved.ok: + return Result(data="").with_errors_from(resolved) if hasattr(Result(data=""), "with_errors_from") else Result(data="", errors=resolved.errors) + p = resolved.data + if isinstance(p, NilPath): + return Result(data="", errors=resolved.errors) + if not p.exists(): + return Result(data="", errors=[ErrorInfo(kind=ErrorKind.NOT_FOUND, message=f"file not found: {path}", source="mcp.read_file")]) + if not p.is_file(): + return Result(data="", errors=[ErrorInfo(kind=ErrorKind.INVALID_INPUT, message=f"not a file: {path}", source="mcp.read_file")]) + try: + content = p.read_text(encoding="utf-8") + return Result(data=content) + except Exception as e: + return Result(data="", errors=[ErrorInfo(kind=ErrorKind.INTERNAL, message=str(e), source="mcp.read_file", original=e)]) +``` + +**NOTE:** `with_errors_from` is NOT in the `Result` API; use the constructor `Result(data="", errors=resolved.errors)` directly. (The above pseudocode is for clarity; the final code uses the constructor.) + +- [ ] **Step 2: Refactor list_directory to return Result[str]** + +```python +def list_directory(path: str) -> Result[str]: + resolved = _resolve_and_check(path) + if not resolved.ok: + return Result(data="", errors=resolved.errors) + p = resolved.data + if isinstance(p, NilPath): + return Result(data="", errors=resolved.errors) + if not p.exists(): + return Result(data="", errors=[ErrorInfo(kind=ErrorKind.NOT_FOUND, message=f"path not found: {path}", source="mcp.list_directory")]) + if not p.is_dir(): + return Result(data="", errors=[ErrorInfo(kind=ErrorKind.INVALID_INPUT, message=f"not a directory: {path}", source="mcp.list_directory")]) + try: + entries = sorted(p.iterdir(), key=lambda e: (e.is_file(), e.name.lower())) + lines = [f"Directory: {p}", ""] + count = 0 + for entry in entries: + name = entry.name.lower() + if name == "history.toml" or name.endswith("_history.toml"): + continue + kind = "file" if entry.is_file() else "dir " + size = f"{entry.stat().st_size:>10,} bytes" if entry.is_file() else "" + lines.append(f" [{kind}] {entry.name:<40} {size}") + count += 1 + lines.append(f" ({count} entries)") + return Result(data="\n".join(lines)) + except Exception as e: + return Result(data="", errors=[ErrorInfo(kind=ErrorKind.INTERNAL, message=str(e), source="mcp.list_directory", original=e)]) +``` + +- [ ] **Step 3: Refactor search_files to return Result[str]** + +Similar pattern. (The implementer should follow the same structure as read_file / list_directory.) + +- [ ] **Step 4: Run the new tests; confirm 6 of 6 pass** + +Run: `uv run pytest tests/test_mcp_client_paths.py -v` +Expected: 6 tests PASS. + +- [ ] **Step 5: Run the existing tests; check for regressions** + +Run: `uv run pytest tests/test_mcp_client.py -v 2>&1 | tail -30` +Expected: existing tests that called `read_file()` and expected a `str` return now FAIL because they get a `Result`. **This is expected.** Update them in Task 2.5. + +- [ ] **Step 6: Commit** + +```bash +git add src/mcp_client.py +git commit -m "refactor(mcp_client): read_file, list_directory, search_files return Result[str]" +``` + +--- + +## Task 2.5: Update tool dispatch internals + existing tests + +**Files:** +- Modify: `src/mcp_client.py` (dispatch layer) +- Modify: `tests/test_mcp_client.py` (existing tests) + +- [ ] **Step 1: Find the tool dispatch function in src/mcp_client.py** + +Run: `grep -n "def async_dispatch\|def dispatch\|def _dispatch" src/mcp_client.py | head -5` + +- [ ] **Step 2: Update the dispatch to extract result.data and log result.errors** + +```python +def async_dispatch(name: str, args: dict) -> str: + handler = _TOOL_REGISTRY.get(name) + if handler is None: + return f"ERROR: unknown tool '{name}'" + result = handler(**args) + if not result.ok: + for err in result.errors: + _append_comms("WARN", "tool_error", {"tool": name, "error": err.ui_message()}) + return result.data +``` + +(Adapt `_append_comms` to the actual function name in the project. `_append_comms` is used in `src/ai_client.py`; `mcp_client.py` may have its own equivalent or may not log at all.) + +- [ ] **Step 3: Update existing tests in tests/test_mcp_client.py to use .data** + +For each test that does `assert mcp_client.read_file(...) == "expected text"`, change to: +```python +r = mcp_client.read_file(...) +assert r.data == "expected text" +assert r.ok +``` + +For tests that check error cases (`assert mcp_client.read_file(...) == "ERROR: ..."`): +```python +r = mcp_client.read_file(...) +assert not r.ok +assert "ERROR" in r.errors[0].message +``` + +- [ ] **Step 4: Run the existing tests; confirm they pass with the new pattern** + +Run: `uv run pytest tests/test_mcp_client.py -v 2>&1 | tail -30` +Expected: tests pass with the new pattern (no regressions). + +- [ ] **Step 5: Commit** + +```bash +git add src/mcp_client.py tests/test_mcp_client.py +git commit -m "refactor(mcp_client): update dispatch + existing tests for Result pattern" +``` + +--- + +## Task 2.6: Refactor the remaining MCP tool functions + +**Files:** +- Modify: `src/mcp_client.py` (~20 more tool functions) + +- [ ] **Step 1: List the remaining tool functions** + +Run: `grep -nE "^def [a-z_]+\(.*\).*->.*str" src/mcp_client.py | head -30` + +The remaining functions follow the same pattern: `get_file_summary`, `py_get_skeleton`, `py_get_code_outline`, `py_get_definition`, `py_get_imports`, `py_find_usages`, `py_get_class_summary`, `py_get_var_declaration`, `py_check_syntax`, `py_get_docstring`, `py_get_hierarchy`, `ts_c_*`, `ts_cpp_*`, `web_search`, `fetch_url`, `set_file_slice`, `edit_file`, `py_update_definition`, `py_set_signature`, `py_set_var_declaration`, `py_remove_def`, `py_add_def`, `py_move_def`, `py_region_wrap`, `get_tree`, `get_file_slice`, `py_get_signature`, `get_git_diff`, `get_ui_performance`, `bd_*` (Beads tools). + +- [ ] **Step 2: For each tool function, refactor to return `Result[str]`** + +The pattern (from read_file): +```python +def tool_name(path: str, ...) -> Result[str]: + resolved = _resolve_and_check(path) + if not resolved.ok: + return Result(data="", errors=resolved.errors) + p = resolved.data + if isinstance(p, NilPath): + return Result(data="", errors=resolved.errors) + # ... rest of the tool logic, replacing `return f"ERROR: ..."` with: + # return Result(data="", errors=[ErrorInfo(kind=ErrorKind.X, message="...", source="tool_name")]) +``` + +(For tools that don't deal with paths, like `web_search`, `fetch_url`, `bd_*`, adapt the pattern to use a `Result` directly without `_resolve_and_check`.) + +- [ ] **Step 3: Remove the 30+ `assert p is not None` chain (lines 304-794)** + +After the refactor, `p` is never `None` — it's either a real `Path` or a `NilPath` (which has a `read_text` field). The assertions are dead code. Remove them. + +- [ ] **Step 4: Run all mcp_client tests** + +Run: `uv run pytest tests/test_mcp_client.py tests/test_mcp_client_paths.py -v` +Expected: all tests pass. + +- [ ] **Step 5: Commit** + +```bash +git add src/mcp_client.py +git commit -m "refactor(mcp_client): all tool functions return Result[str]; remove assert p is not None chain" +``` + +--- + +## Task 2.7: Phase 2 checkpoint commit and git note + +**Files:** none (commit + note only) + +- [ ] **Step 1: Run all mcp_client tests** + +Run: `uv run pytest tests/test_mcp_client.py tests/test_mcp_client_paths.py -v` +Expected: all tests pass (existing + new). + +- [ ] **Step 2: Run the full test suite to check for regressions** + +Run: `uv run pytest tests/ -q --timeout=60 2>&1 | tail -10` +Expected: no new failures (pre-existing failures unchanged). + +- [ ] **Step 3: Create the checkpoint commit and git note** + +```bash +git add -A +if ! git diff --cached --quiet; then git commit -m "conductor(checkpoint): Phase 2 complete - mcp_client.py refactored"; fi +SHA=$(git log -1 --format="%H") +git notes add -m "Phase 2 checkpoint: mcp_client.py refactored. + +~60 sites converted to the Result pattern: +- _resolve_and_check returns Result[Path] (replaces tuple) +- All 9 tool functions return Result[str] (read_file, list_directory, etc.) +- 30+ 'assert p is not None' chain removed (lines 304-794) +- Tool dispatch extracts .data and logs .errors via comms log +- tests/test_mcp_client.py updated to use .data +- tests/test_mcp_client_paths.py: 6 new tests + +No regressions. + +Next: Phase 3 (ai_client.py refactor - HIGHEST RISK)." "$SHA" +``` + +- [ ] **Step 4: Update state.toml phase_2 status** + +```bash +git add conductor/tracks/data_oriented_error_handling_20260606/state.toml +git commit -m "conductor(plan): mark Phase 2 complete in data_oriented_error_handling_20260606" +``` + +--- + +# Phase 3: `src/ai_client.py` Refactor (HIGHEST RISK) + +> Goal: `ProviderError` exception is removed. `_classify__error()` returns `ErrorInfo` (not raise). `_send_()` is renamed to `_send__result()` and returns `Result[str]`. `send()` is marked `@deprecated`; `send_result()` is the new public API. All 8 vendors (Gemini, Anthropic, DeepSeek, MiniMax, Gemini CLI, Qwen, Llama, Grok) are migrated. + +--- + +## Task 3.1: Baseline: verify existing provider tests pass before refactor + +**Files:** none (verification only) + +- [ ] **Step 1: Run all 8 vendor test files** + +Run: +```bash +uv run pytest tests/test_ai_client.py tests/test_minimax_provider.py tests/test_qwen_provider.py tests/test_llama_provider.py tests/test_grok_provider.py tests/test_ai_client_cli.py tests/test_deepseek_provider.py tests/test_gemini_cli_adapter.py -v 2>&1 | tail -20 +``` + +Expected: existing tests pass (with the same pre-existing failures as the baseline). + +- [ ] **Step 2: Record pass count in state.toml** + +- [ ] **Step 3: Commit nothing (baseline)** + +--- + +## Task 3.2: Write red tests for new Result API + deprecation warning + +**Files:** +- Create: `tests/test_ai_client_result.py` +- Create: `tests/test_deprecation_warnings.py` + +- [ ] **Step 1: Create test_ai_client_result.py with 6 tests** + +```python +from unittest.mock import MagicMock, patch +import pytest +from src import ai_client +from src.result_types import Result, ErrorInfo, ErrorKind + +def test_send_result_public_api_returns_result() -> None: + with patch.object(ai_client, "set_provider"): + with patch.object(ai_client, "_send_anthropic_result", return_value=Result(data="hello")) as mock_send: + r = ai_client.send_result("system", "user") + assert isinstance(r, Result) + assert r.ok + assert r.data == "hello" + +def test_send_deprecated_emits_warning() -> None: + import warnings + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + with patch.object(ai_client, "set_provider"): + with patch.object(ai_client, "_send_anthropic_result", return_value=Result(data="hi")): + result = ai_client.send("system", "user") + assert result == "hi" + assert any(issubclass(x.category, DeprecationWarning) for x in w) + +def test_send_result_preserves_errors() -> None: + err = ErrorInfo(kind=ErrorKind.RATE_LIMIT, message="slow down", source="test") + with patch.object(ai_client, "set_provider"): + with patch.object(ai_client, "_send_anthropic_result", return_value=Result(data="", errors=[err])): + r = ai_client.send_result("system", "user") + assert not r.ok + assert r.errors == [err] + +def test_send_extracts_data_from_result() -> None: + with patch.object(ai_client, "set_provider"): + with patch.object(ai_client, "_send_anthropic_result", return_value=Result(data="result text")): + result = ai_client.send("system", "user") + assert result == "result text" + +def test_send_returns_empty_string_on_error_result() -> None: + err = ErrorInfo(kind=ErrorKind.AUTH, message="bad key", source="test") + with patch.object(ai_client, "set_provider"): + with patch.object(ai_client, "_send_anthropic_result", return_value=Result(data="", errors=[err])): + result = ai_client.send("system", "user") + assert result == "" + +def test_classify_gemini_error_returns_error_info() -> None: + from src.ai_client import _classify_gemini_error + class FakeRateLimitError(Exception): pass + e = FakeRateLimitError("rate limited") + info = _classify_gemini_error(e, source="test.gemini") + assert isinstance(info, ErrorInfo) + assert info.kind == ErrorKind.RATE_LIMIT + assert info.source == "test.gemini" + assert info.original is e +``` + +- [ ] **Step 2: Create test_deprecation_warnings.py with 2 tests** + +```python +import warnings +from unittest.mock import patch +from src import ai_client + +def test_send_deprecated_warning_emitted_once_per_site() -> None: + with patch.object(ai_client, "set_provider"): + with patch.object(ai_client, "_send_anthropic_result", return_value=Result(data="x")): + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + ai_client.send("s", "u") + ai_client.send("s", "u") # second call; should still emit (per-site caching is per-call-location) + deprecation_warnings = [x for x in w if issubclass(x.category, DeprecationWarning)] + assert len(deprecation_warnings) >= 1 + +def test_send_result_does_not_emit_deprecation() -> None: + with patch.object(ai_client, "set_provider"): + with patch.object(ai_client, "_send_anthropic_result", return_value=Result(data="x")): + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + ai_client.send_result("s", "u") + deprecation_warnings = [x for x in w if issubclass(x.category, DeprecationWarning)] + assert len(deprecation_warnings) == 0 +``` + +- [ ] **Step 3: Add Result import at top of test files** + +```python +from src.result_types import Result, ErrorInfo, ErrorKind +``` + +(Already included in test_ai_client_result.py above; add to test_deprecation_warnings.py.) + +- [ ] **Step 4: Run, confirm 8 tests fail** + +Run: `uv run pytest tests/test_ai_client_result.py tests/test_deprecation_warnings.py -v` +Expected: 8 tests FAIL. + +- [ ] **Step 5: Commit (red)** + +```bash +git add tests/test_ai_client_result.py tests/test_deprecation_warnings.py +git commit -m "test(ai_client): add red tests for new Result API + deprecation warnings" +``` + +--- + +## Task 3.3: Refactor _classify__error() to return ErrorInfo (8 vendors) + +**Files:** +- Modify: `src/ai_client.py` (8 classifier functions) + +- [ ] **Step 1: Find all the classifier functions** + +Run: `grep -n "def _classify_.*_error" src/ai_client.py` + +- [ ] **Step 2: Refactor each classifier to return ErrorInfo (not raise ProviderError)** + +For each classifier, change the function signature and the body: + +**Before:** +```python +def _classify_gemini_error(exc: Exception) -> ProviderError: + if isinstance(exc, genai_types.RateLimitError): + return ProviderError(kind="rate_limit", provider="gemini", original=exc) + ... + raise ProviderError(kind="unknown", provider="gemini", original=exc) +``` + +**After:** +```python +def _classify_gemini_error(exc: Exception, source: str = "ai_client.gemini") -> ErrorInfo: + if isinstance(exc, genai_types.RateLimitError): + return ErrorInfo(kind=ErrorKind.RATE_LIMIT, message=str(exc), source=source, original=exc) + ... + return ErrorInfo(kind=ErrorKind.UNKNOWN, message=str(exc), source=source, original=exc) +``` + +(Apply to all 8 classifiers: `_classify_gemini_error`, `_classify_anthropic_error`, `_classify_deepseek_error`, `_classify_minimax_error`, `_classify_gemini_cli_error`, `_classify_qwen_error`, `_classify_llama_error`, `_classify_grok_error`.) + +- [ ] **Step 3: Run the test_ai_client_result.py tests; `test_classify_gemini_error_returns_error_info` should now pass** + +Run: `uv run pytest tests/test_ai_client_result.py::test_classify_gemini_error_returns_error_info -v` +Expected: 1 test PASS. + +- [ ] **Step 4: Commit** + +```bash +git add src/ai_client.py +git commit -m "refactor(ai_client): _classify__error() returns ErrorInfo (8 vendors)" +``` + +--- + +## Task 3.4: Rename _send_() to _send__result() and return Result[str] + +**Files:** +- Modify: `src/ai_client.py` (8 send functions + their call sites) + +- [ ] **Step 1: Find all the _send_() functions** + +Run: `grep -n "^def _send_" src/ai_client.py` + +- [ ] **Step 2: For each function: rename to _send__result(), change return type to Result[str], wrap body** + +**Before (generic pattern):** +```python +def _send_gemini(md_content, user_message, ...) -> str: + try: + resp = genai_client.models.generate_content(...) + return extract_text(resp) + except Exception as exc: + raise _classify_gemini_error(exc) from exc +``` + +**After:** +```python +def _send_gemini_result(md_content, user_message, ...) -> Result[str]: + try: + resp = genai_client.models.generate_content(...) + return Result(data=extract_text(resp)) + except Exception as exc: + return Result(data="", errors=[_classify_gemini_error(exc, source="ai_client.gemini")]) +``` + +(Apply to all 8 functions.) + +- [ ] **Step 3: Update internal callers in src/ai_client.py** + +Run: `grep -n "_send_gemini\|_send_anthropic\|_send_deepseek\|_send_minimax\|_send_gemini_cli\|_send_qwen\|_send_llama\|_send_grok" src/ai_client.py | grep -v "^def _send_" | grep -v "_classify_" | head -20` + +Update each call site from `result = _send_(...)` to `result = _send__result(...); text = result.data`. + +- [ ] **Step 4: Run the 8 vendor test files; check for regressions** + +Run: +```bash +uv run pytest tests/test_ai_client.py tests/test_minimax_provider.py tests/test_qwen_provider.py tests/test_llama_provider.py tests/test_grok_provider.py tests/test_ai_client_cli.py tests/test_deepseek_provider.py tests/test_gemini_cli_adapter.py 2>&1 | tail -20 +``` + +Expected: tests that directly call `_send_()` FAIL (they now need the new name). Tests that go through `send()` still PASS (until Task 3.6 wires up `send_result`). + +- [ ] **Step 5: Commit (partial progress; the test breakage is expected)** + +```bash +git add src/ai_client.py +git commit -m "refactor(ai_client): rename _send_() to _send__result() returning Result[str]" +``` + +--- + +## Task 3.5: Add send_result() public API to src/ai_client.py + +**Files:** +- Modify: `src/ai_client.py` + +- [ ] **Step 1: Find the existing public send() function** + +Run: `manual-slop_py_get_definition path=src/ai_client.py name=send` (returns the full function) + +- [ ] **Step 2: Add send_result() as a sibling function** + +```python +def send_result( + md_content: str, + user_message: str, + base_dir: str = ".", + file_items: list[dict] | None = None, + discussion_history: str = "", + stream: bool = False, + pre_tool_callback: Optional[Callable] = None, + qa_callback: Optional[Callable] = None, + enable_tools: bool = True, + stream_callback: Optional[Callable] = None, + patch_callback: Optional[Callable] = None, + rag_engine: Optional[Any] = None, +) -> Result[str]: + """The new Result-based public API. Returns Result[str, ErrorInfo]. + data is the response text on success; errors contains ErrorInfo on failure.""" + _send_lock.acquire() + try: + if _provider == "gemini": + r = _send_gemini_result(md_content, user_message, base_dir, file_items, discussion_history, stream, pre_tool_callback, qa_callback, enable_tools, stream_callback, patch_callback, rag_engine) + elif _provider == "anthropic": + r = _send_anthropic_result(...) + elif _provider == "deepseek": + r = _send_deepseek_result(...) + elif _provider == "minimax": + r = _send_minimax_result(...) + elif _provider == "gemini_cli": + r = _send_gemini_cli_result(...) + elif _provider == "qwen": + r = _send_qwen_result(...) + elif _provider == "llama": + r = _send_llama_result(...) + elif _provider == "grok": + r = _send_grok_result(...) + else: + r = Result(data="", errors=[ErrorInfo(kind=ErrorKind.CONFIG, message=f"unknown provider: {_provider}", source="ai_client.send_result")]) + return r + finally: + _send_lock.release() +``` + +(Adapt the parameter list and provider dispatch to match the existing `send()` signature exactly. The above is the conceptual structure; copy from the existing `send()` body.) + +- [ ] **Step 3: Run test_ai_client_result.py; confirm `test_send_result_public_api_returns_result` passes** + +Run: `uv run pytest tests/test_ai_client_result.py::test_send_result_public_api_returns_result -v` +Expected: 1 test PASS. + +- [ ] **Step 4: Commit** + +```bash +git add src/ai_client.py +git commit -m "feat(ai_client): add send_result() public API returning Result[str]" +``` + +--- + +## Task 3.6: Mark send() as @deprecated and rewire it to call send_result() + +**Files:** +- Modify: `src/ai_client.py` + +- [ ] **Step 1: Add the deprecation import at the top of src/ai_client.py** + +```python +from typing_extensions import deprecated +``` + +- [ ] **Step 2: Wrap the existing send() with @deprecated** + +```python +@deprecated("Use ai_client.send_result() instead. The deprecated send() will be removed in the public_api_migration_20260606 track. See conductor/tracks/data_oriented_error_handling_20260606/spec.md §12.1 for the migration path.") +def send(...) -> str: + """[DEPRECATED] Use send_result() instead. Returns str (the response text). Errors are logged to the comms log but not returned.""" + import warnings + warnings.warn( + "ai_client.send() is deprecated; use ai_client.send_result() instead. " + "The deprecated function will be removed once callers migrate. " + "See conductor/tracks/data_oriented_error_handling_20260606/spec.md §12.1.", + DeprecationWarning, + stacklevel=2, + ) + result = send_result(...) + if not result.ok: + for err in result.errors: + _append_comms("WARN", "deprecated_send_with_errors", {"error": err.ui_message()}) + return result.data +``` + +(Replace the body of the existing `send()` with the above. The signature stays the same; only the body changes to call `send_result()` and unwrap.) + +- [ ] **Step 3: Run test_deprecation_warnings.py; confirm 2 tests pass** + +Run: `uv run pytest tests/test_deprecation_warnings.py -v` +Expected: 2 tests PASS. + +- [ ] **Step 4: Run test_ai_client_result.py; confirm all 6 tests pass** + +Run: `uv run pytest tests/test_ai_client_result.py -v` +Expected: 6 tests PASS. + +- [ ] **Step 5: Run the 8 vendor test files; confirm no regressions (most tests call send() which now emits a warning but still works)** + +Run: +```bash +uv run pytest tests/test_ai_client.py tests/test_minimax_provider.py tests/test_qwen_provider.py tests/test_llama_provider.py tests/test_grok_provider.py tests/test_ai_client_cli.py tests/test_deepseek_provider.py tests/test_gemini_cli_adapter.py 2>&1 | tail -20 +``` + +Expected: tests pass (with DeprecationWarning in stderr for tests that call send()). + +- [ ] **Step 6: Commit** + +```bash +git add src/ai_client.py +git commit -m "feat(ai_client): mark send() @deprecated; rewire to call send_result()" +``` + +--- + +## Task 3.7: Remove the ProviderError class and the dead `except ProviderError` clause + +**Files:** +- Modify: `src/ai_client.py` + +- [ ] **Step 1: Find ProviderError** + +Run: `grep -n "class ProviderError" src/ai_client.py` + +- [ ] **Step 2: Remove the ProviderError class definition** + +Delete the class block. + +- [ ] **Step 3: Find the `except ProviderError` clause** + +Run: `grep -n "except ProviderError" src/ai_client.py` + +- [ ] **Step 4: Remove the now-dead clause (line 1338 per the spec)** + +Verify the clause is no longer reachable (after the refactor, all classifiers return ErrorInfo; nothing raises ProviderError). Remove the clause. + +- [ ] **Step 5: Run all 8 vendor test files; confirm no regressions** + +Run: +```bash +uv run pytest tests/test_ai_client.py tests/test_minimax_provider.py tests/test_qwen_provider.py tests/test_llama_provider.py tests/test_grok_provider.py tests/test_ai_client_cli.py tests/test_deepseek_provider.py tests/test_gemini_cli_adapter.py 2>&1 | tail -10 +``` + +Expected: tests pass. + +- [ ] **Step 6: Run the full test suite; check for regressions** + +Run: `uv run pytest tests/ -q --timeout=60 2>&1 | tail -10` +Expected: no new failures (deprecation warnings may show in stderr but don't fail tests). + +- [ ] **Step 7: Commit** + +```bash +git add src/ai_client.py +git commit -m "refactor(ai_client): remove ProviderError class and dead except clause" +``` + +--- + +## Task 3.8: Phase 3 checkpoint commit and git note + +**Files:** none (commit + note only) + +- [ ] **Step 1: Run all 8 vendor test files + new tests** + +Run: +```bash +uv run pytest tests/test_ai_client.py tests/test_minimax_provider.py tests/test_qwen_provider.py tests/test_llama_provider.py tests/test_grok_provider.py tests/test_ai_client_cli.py tests/test_deepseek_provider.py tests/test_gemini_cli_adapter.py tests/test_ai_client_result.py tests/test_deprecation_warnings.py -v 2>&1 | tail -10 +``` + +Expected: tests pass (with DeprecationWarning in some output). + +- [ ] **Step 2: Run the full test suite to check for regressions** + +Run: `uv run pytest tests/ -q --timeout=60 2>&1 | tail -10` +Expected: same pre-existing failures; no new failures. + +- [ ] **Step 3: Create the checkpoint commit and git note** + +```bash +git add -A +if ! git diff --cached --quiet; then git commit -m "conductor(checkpoint): Phase 3 complete - ai_client.py refactored (ProviderError removed, send deprecated)"; fi +SHA=$(git log -1 --format="%H") +git notes add -m "Phase 3 checkpoint: ai_client.py refactored. ProviderError exception REMOVED. All 8 _classify__error() functions return ErrorInfo. All 8 _send_() functions renamed to _send__result() and return Result[str]. New public send_result() API. send() marked @deprecated (still works, emits DeprecationWarning). 8 new tests pass + existing tests pass. + +Next: Phase 4 (rag_engine.py refactor)." "$SHA" +``` + +- [ ] **Step 4: Update state.toml phase_3 status** + +```bash +git add conductor/tracks/data_oriented_error_handling_20260606/state.toml +git commit -m "conductor(plan): mark Phase 3 complete in data_oriented_error_handling_20260606" +``` + +--- + +# Phase 4: `src/rag_engine.py` Refactor + +> Goal: `RAGEngine` methods return `Result`. `NilRAGState` is used for unconfigured state. `ImportError` and `ValueError` raises become `ErrorInfo` entries. Broad `except Exception` blocks become `Result` with side-channel errors. + +--- + +## Task 4.1: Baseline: verify existing rag_engine tests pass before refactor + +**Files:** none (verification only) + +- [ ] **Step 1: Run tests/test_rag_engine.py and record pass count** + +Run: `uv run pytest tests/test_rag_engine.py -v 2>&1 | tail -20` +Expected: existing tests pass. + +- [ ] **Step 2: Record pass count in state.toml** + +--- + +## Task 4.2: Write red tests for new Result return types + +**Files:** +- Create: `tests/test_rag_engine_result.py` + +- [ ] **Step 1: Create the test file with 4 tests** + +```python +from unittest.mock import MagicMock, patch +import pytest +from src.rag_engine import RAGEngine +from src.result_types import Result, ErrorKind, ErrorInfo, NilRAGState + +def test_init_vector_store_returns_result_not_raises() -> None: + config = MagicMock() + config.vector_store.provider = "chroma" + config.vector_store.collection_name = "test" + config.embedding_provider = "gemini" + engine = RAGEngine(base_dir="/tmp", config=config) + with patch("src.rag_engine._get_chromadb", return_value=None): + r = engine._init_vector_store_result() + assert isinstance(r, Result) + assert not r.ok + assert len(r.errors) >= 1 + assert r.errors[0].kind == ErrorKind.CONFIG + +def test_init_vector_store_unknown_provider_returns_error_info() -> None: + config = MagicMock() + config.vector_store.provider = "unknown_provider_xyz" + engine = RAGEngine(base_dir="/tmp", config=config) + r = engine._init_vector_store_result() + assert not r.ok + assert r.errors[0].kind == ErrorKind.CONFIG + assert "unknown_provider_xyz" in r.errors[0].message + +def test_validate_collection_dim_returns_result() -> None: + config = MagicMock() + config.vector_store.provider = "chroma" + config.vector_store.collection_name = "test" + config.embedding_provider = "gemini" + engine = RAGEngine(base_dir="/tmp", config=config) + engine.collection = None + engine.embedding_provider = None + r = engine._validate_collection_dim_result() + assert isinstance(r, Result) + assert r.ok + +def test_is_empty_uses_nil_rag_state_when_not_configured() -> None: + config = MagicMock() + config.enabled = False + engine = RAGEngine(base_dir="/tmp", config=config) + state = engine._get_state() + assert state is NilRAGState + assert state.enabled is False +``` + +- [ ] **Step 2: Run, confirm 4 tests fail** + +Run: `uv run pytest tests/test_rag_engine_result.py -v` +Expected: 4 tests FAIL. + +- [ ] **Step 3: Commit (red)** + +```bash +git add tests/test_rag_engine_result.py +git commit -m "test(rag_engine): add red tests for new Result return types" +``` + +--- + +## Task 4.3: Refactor RAGEngine._init_vector_store to return Result[None] + +**Files:** +- Modify: `src/rag_engine.py` + +- [ ] **Step 1: Add Result + ErrorInfo imports to rag_engine.py** + +Use `manual-slop_edit_file` to add to the top: +```python +from src.result_types import ErrorInfo, ErrorKind, NilRAGState, Result +``` + +- [ ] **Step 2: Refactor `_init_vector_store` to `_init_vector_store_result` returning Result[None]** + +```python +def _init_vector_store_result(self) -> Result[None]: + vs_config = self.config.vector_store + if vs_config.provider == 'chroma': + db_path = os.path.abspath(os.path.join(self.base_dir, ".slop_cache", f"chroma_{vs_config.collection_name}")) + os.makedirs(db_path, exist_ok=True) + chroma_module = _get_chromadb() + if chroma_module is None: + return Result(data=None, errors=[ErrorInfo(kind=ErrorKind.CONFIG, message="chromadb is not installed", source="rag._init_vector_store")]) + chromadb, Settings = chroma_module + self.client = chromadb.PersistentClient(path=db_path) + self.collection = self.client.get_or_create_collection(name=vs_config.collection_name) + return _validate_collection_dim_result() + elif vs_config.provider == 'mock': + self.client = "mock" + self.collection = "mock" + return Result(data=None) + else: + return Result(data=None, errors=[ErrorInfo(kind=ErrorKind.CONFIG, message=f"Unknown vector store provider: {vs_config.provider}", source="rag._init_vector_store")]) +``` + +- [ ] **Step 3: Run test_rag_engine_result.py; confirm 2 tests pass** + +Run: `uv run pytest tests/test_rag_engine_result.py::test_init_vector_store_returns_result_not_raises tests/test_rag_engine_result.py::test_init_vector_store_unknown_provider_returns_error_info -v` +Expected: 2 tests PASS. + +- [ ] **Step 4: Commit** + +```bash +git add src/rag_engine.py +git commit -m "refactor(rag_engine): _init_vector_store returns Result[None]" +``` + +--- + +## Task 4.4: Refactor RAGEngine._validate_collection_dim and other methods + +**Files:** +- Modify: `src/rag_engine.py` + +- [ ] **Step 1: Refactor `_validate_collection_dim` to `_validate_collection_dim_result`** + +```python +def _validate_collection_dim_result(self) -> Result[None]: + if self.collection is None or self.collection == "mock" or self.embedding_provider is None: + return Result(data=None) + try: + res = self.collection.get(limit=1, include=["embeddings"]) + if not res: + return Result(data=None) + embeddings = res.get("embeddings") if isinstance(res, dict) else None + if not embeddings or len(embeddings) == 0: + return Result(data=None) + existing_dim = len(embeddings[0]) + expected_dim = len(self.embedding_provider.embed(["__rag_dim_check__"])[0]) + if existing_dim == expected_dim: + return Result(data=None) + sys.stderr.write( + f"RAG: Collection '{self.collection.name}' dim mismatch " + f"(existing={existing_dim}, expected={expected_dim}). " + f"Recreating collection to prevent silent corruption.\n" + ) + sys.stderr.flush() + self.client.delete_collection(self.collection.name) + self.collection = self.client.get_or_create_collection(name=self.collection.name) + return Result(data=None) + except Exception as e: + return Result(data=None, errors=[ErrorInfo(kind=ErrorKind.INTERNAL, message=f"Failed to validate collection dim: {e}", source="rag._validate_collection_dim", original=e)]) +``` + +- [ ] **Step 2: Add the `_get_state()` method using NilRAGState** + +```python +def _get_state(self) -> NilRAGState: + return NilRAGState(enabled=self.config.enabled) +``` + +- [ ] **Step 3: Refactor is_empty, add_documents, search, index_file (where appropriate)** + +For each method, change the error path from `raise` to `return Result(data=zero, errors=[...])`. The success path returns `Result(data=result)`. + +- [ ] **Step 4: Run all rag_engine tests; confirm 4 new + existing all pass** + +Run: `uv run pytest tests/test_rag_engine.py tests/test_rag_engine_result.py -v` +Expected: 4 new tests PASS; existing tests pass (after update). + +- [ ] **Step 5: Commit** + +```bash +git add src/rag_engine.py +git commit -m "refactor(rag_engine): all methods return Result; NilRAGState sentinel" +``` + +--- + +## Task 4.5: Phase 4 checkpoint commit and git note + +**Files:** none (commit + note only) + +- [ ] **Step 1: Run all rag_engine tests** + +Run: `uv run pytest tests/test_rag_engine.py tests/test_rag_engine_result.py -v` +Expected: all tests pass. + +- [ ] **Step 2: Run the full test suite** + +Run: `uv run pytest tests/ -q --timeout=60 2>&1 | tail -10` +Expected: no new failures. + +- [ ] **Step 3: Create the checkpoint commit and git note** + +```bash +git add -A +if ! git diff --cached --quiet; then git commit -m "conductor(checkpoint): Phase 4 complete - rag_engine.py refactored"; fi +SHA=$(git log -1 --format="%H") +git notes add -m "Phase 4 checkpoint: rag_engine.py refactored. + +RAGEngine methods return Result. ImportError and ValueError raises become ErrorInfo entries. _validate_collection_dim and _init_vector_store no longer raise. NilRAGState used for unconfigured state. + +No regressions in tests/test_rag_engine.py. + +Next: Phase 5 (deprecation wiring + docs + archive)." "$SHA" +``` + +- [ ] **Step 4: Update state.toml phase_4 status** + +```bash +git add conductor/tracks/data_oriented_error_handling_20260606/state.toml +git commit -m "conductor(plan): mark Phase 4 complete in data_oriented_error_handling_20260606" +``` + +--- + +# Phase 5: Deprecation Wiring + Docs + Integration + +> Goal: Silence the deprecation warning in existing tests. Update the deep-dive docs. Register the `public_api_migration_20260606` follow-up placeholder. Manual smoke test. Archive the track. + +--- + +## Task 5.1: Silence the deprecation warning in existing tests + +**Files:** +- Modify: `tests/conftest.py` + +- [ ] **Step 1: Find the existing filterwarnings config in conftest.py** + +Run: `grep -n "filterwarnings" tests/conftest.py` + +- [ ] **Step 2: Add a filterwarnings entry to silence the send() deprecation during the transition** + +If `filterwarnings` is already configured, add: +```python +filterwarnings("ignore::DeprecationWarning:src.ai_client", category=DeprecationWarning, module=r"src\.ai_client") +``` + +If not configured, add a new section: +```python +# Silences the ai_client.send() deprecation warning during the transition period. +# Will be removed in the public_api_migration_20260606 track when send() itself is removed. +filterwarnings("ignore::DeprecationWarning:src.ai_client") +``` + +- [ ] **Step 3: Run the full test suite; confirm deprecation warnings no longer spam stderr** + +Run: `uv run pytest tests/ -q --timeout=60 2>&1 | tail -10` +Expected: no `DeprecationWarning: ai_client.send()` lines in stderr; tests still pass. + +- [ ] **Step 4: Commit** + +```bash +git add tests/conftest.py +git commit -m "test(conftest): silence ai_client.send() deprecation warning during transition" +``` + +--- + +## Task 5.2: Update docs/guide_ai_client.md + +**Files:** +- Modify: `docs/guide_ai_client.md` + +- [ ] **Step 1: Find a good insertion point (after the "Public API Quick Reference" section)** + +Run: `grep -n "Public API Quick Reference\|## " docs/guide_ai_client.md | head -20` + +- [ ] **Step 2: Add the new section** + +```markdown +## Data-Oriented Error Handling (Fleury Pattern) + +The provider layer follows the "errors are just cases" framework (Ryan Fleury). +The canonical reference is `conductor/code_styleguides/error_handling.md`. + +### Result-Based Returns + +All `_send__result()` functions (8 vendors: Gemini, Anthropic, DeepSeek, +MiniMax, Gemini CLI, Qwen, Llama, Grok) return `Result[str, ErrorInfo]`. SDK +exceptions are caught at the boundary (`src/openai_compatible.py`, +`src/qwen_adapter.py`) and converted to `ErrorInfo` dataclasses. The +`_classify__error()` functions return `ErrorInfo` (not raise +`ProviderError`, which has been removed). + +### Public API + +- `ai_client.send_result(...)` — the new public API. Returns `Result[str, ErrorInfo]`. +- `ai_client.send(...)` — **deprecated.** Emits `DeprecationWarning` at runtime. + Returns `str` (the response text) for backward compat. Errors are logged to + the comms log but not returned. Will be removed in the + `public_api_migration_20260606` track. + +### Example + +```python +from src import ai_client +from src.result_types import ErrorKind + +r = ai_client.send_result("system prompt", "user message") +if not r.ok: + for err in r.errors: + log.error(err.ui_message()) # err.kind is one of ErrorKind.*; err.source is "ai_client." +# use r.data regardless (it's the zero-initialized "" on failure) +print(r.data) +``` +``` + +- [ ] **Step 3: Commit** + +```bash +git add docs/guide_ai_client.md +git commit -m "docs(ai_client): document Result API + deprecation" +``` + +--- + +## Task 5.3: Update docs/guide_mcp_client.md + +**Files:** +- Modify: `docs/guide_mcp_client.md` (if it exists; if not, create it) + +- [ ] **Step 1: Check if the guide exists** + +Run: `test -f docs/guide_mcp_client.md && echo "exists" || echo "MISSING - create it"` + +- [ ] **Step 2: If exists, add a new section about the Result pattern; if not, create the file with a Result-pattern section** + +(For brevity, the section is similar to the one in guide_ai_client.md. The key difference: focus on the `(p, err)` → `Result[Path]` and tool function `Result[str]` changes.) + +- [ ] **Step 3: Commit** + +```bash +git add docs/guide_mcp_client.md +git commit -m "docs(mcp_client): document new Result return types + nil-sentinel pattern" +``` + +--- + +## Task 5.4: Add public_api_migration_20260606 placeholder to conductor/tracks.md + +**Files:** +- Modify: `conductor/tracks.md` + +- [ ] **Step 1: Add the follow-up track entry in the "Remaining Backlog (Phases 3 & 4)" section** + +```markdown +0f. [ ] **Track: Public API Result Migration (follow-up to data_oriented_error_handling_20260606)** + *Goal: Remove the deprecated `ai_client.send()` and migrate all callers to `send_result()`. Affects `multi_agent_conductor.py` (MMA worker interface), `app_controller.py` (orchestrator), and ~50+ test files. Plan to be authored when data_oriented_error_handling_20260606 is complete; not started yet.* +``` + +- [ ] **Step 2: Commit** + +```bash +git add conductor/tracks.md +git commit -m "conductor(tracks): register public_api_migration_20260606 follow-up placeholder" +``` + +--- + +## Task 5.5: Manual smoke test + +**Files:** none (manual verification) + +- [ ] **Step 1: Launch the GUI in test mode** + +Run: `uv run python sloppy.py --enable-test-hooks` in a separate terminal. + +- [ ] **Step 2: Select a provider, send a message** + +Verify: the response comes back via the new `send_result()` path; the comms log shows a `send_result` entry. + +- [ ] **Step 3: Trigger an error condition (e.g., invalid API key)** + +Verify: the error is returned in `result.errors` and logged to the comms log. + +- [ ] **Step 4: Document the smoke test results** + +In a short markdown file (e.g., `docs/smoke_test_20260606.md` or a git note), record the verification. + +- [ ] **Step 5: Commit the smoke test notes** + +```bash +git add docs/smoke_test_20260606.md 2>/dev/null || true +if git diff --cached --quiet; then echo "no smoke test doc to commit"; else git commit -m "docs(smoke): Phase 5 manual smoke test for Result API"; fi +``` + +--- + +## Task 5.6: Phase 5 checkpoint (TRACK COMPLETE) + +**Files:** +- Modify: `conductor/tracks/data_oriented_error_handling_20260606/state.toml` + +- [ ] **Step 1: Run the full test suite one final time** + +Run: `uv run pytest tests/ -q --timeout=60 2>&1 | tail -10` +Expected: no new failures vs baseline. + +- [ ] **Step 2: Mark all phases complete in state.toml** + +Edit state.toml: +- `current_phase = 5` (or remove) +- All phase_N entries: `status = "completed"`, `checkpointsha` filled in +- Update `[verification]` section: all values to `true` +- Add final line: `# Track completed 2026-06-06 and archived.` + +- [ ] **Step 3: Create the final checkpoint commit and git note** + +```bash +git add -A +if ! git diff --cached --quiet; then git commit -m "conductor(checkpoint): Phase 5 complete - track shipped to archive"; fi +SHA=$(git log -1 --format="%H") +git notes add -m "TRACK COMPLETE: data_oriented_error_handling_20260606 + +Final state: +- src/result_types.py: ErrorKind, ErrorInfo, Result[T], NilPath, NilRAGState, OK +- src/mcp_client.py: all tool functions return Result[str]; ~60 sites refactored; 30+ asserts removed +- src/ai_client.py: ProviderError REMOVED; 8 _classify__error() return ErrorInfo; 8 _send__result() return Result[str]; send() @deprecated; send_result() is the new public API +- src/rag_engine.py: all methods return Result; NilRAGState sentinel +- conductor/code_styleguides/error_handling.md: canonical reference (5 patterns, Python mappings, decision tree, examples) +- conductor/product-guidelines.md + workflow.md: convention documented +- docs/guide_ai_client.md + guide_mcp_client.md: updated +- 18+ new unit tests pass; existing tests pass with deprecation warning +- Follow-up track public_api_migration_20260606 registered in tracks.md + +Convention established. Future tracks can incrementally migrate the +remaining src/ files using the patterns documented in the styleguide." "$SHA" +``` + +- [ ] **Step 4: Commit state.toml update** + +```bash +git add conductor/tracks/data_oriented_error_handling_20260606/state.toml +git commit -m "conductor(plan): mark Phase 5 complete in data_oriented_error_handling_20260606" +``` + +--- + +## Task 5.7: Archive the track + +**Files:** +- Move: `conductor/tracks/data_oriented_error_handling_20260606/` → `conductor/tracks/archive/data_oriented_error_handling_20260606/` +- Modify: `conductor/tracks.md` + +- [ ] **Step 1: git mv the track directory** + +```bash +git mv conductor/tracks/data_oriented_error_handling_20260606 conductor/tracks/archive/data_oriented_error_handling_20260606 +``` + +- [ ] **Step 2: Update tracks.md: change [ ] to [x], move to Recently Completed** + +Edit `conductor/tracks.md`: find the data_oriented_error_handling_20260606 entry, change `[ ]` to `[x]`, and move it to the "Recently Completed Tracks" section (or equivalent). + +- [ ] **Step 3: Commit** + +```bash +git add conductor/tracks.md conductor/tracks/data_oriented_error_handling_20260606 +git commit -m "conductor(archive): ship data_oriented_error_handling_20260606 to archive" +``` + +--- + +# Self-Review + +**1. Spec coverage:** + +| Spec Section | Plan Coverage | +|---|---| +| §1 Overview | All 4 deliverables (result_types module, 3-file refactor, deprecation, docs) addressed in Phases 1-5. | +| §2 Goals | A (foundation + 3 files): Phases 1-4. B (deprecation + Result API): Phase 3. C (convention docs): Phase 5. D (plan follow-up): Phase 5 Task 5.4. | +| §3 Architecture | 3.1 patterns: Task 1.6 (styleguide). 3.2 module layout: all files created/modified per the table. 3.3 Result + ErrorInfo: Task 1.4. 3.4 nil-sentinel: Task 1.4 + Tasks 2.3-2.6. 3.5 deprecation: Task 3.6. | +| §4.1 mcp_client.py | Phase 2 (Tasks 2.2-2.6). | +| §4.2 ai_client.py | Phase 3 (Tasks 3.3-3.7). | +| §4.3 rag_engine.py | Phase 4 (Tasks 4.3-4.4). | +| §4.4 convention docs | Task 1.6 (styleguide), Tasks 1.7-1.8 (product-guidelines + workflow), Tasks 5.2-5.3 (guide_*.md). | +| §5 Configuration | Task 1.2 (typing_extensions dep). | +| §6 Testing | 5 new test files (test_result_types, test_mcp_client_paths, test_ai_client_result, test_rag_engine_result, test_deprecation_warnings); existing test files updated minimally. | +| §7 Migration | 5 phases; each phase is a plan phase. | +| §8 Risks | All 6 risks addressed: ProviderError catch (Task 3.7); asserts (Task 2.6); deprecation spam (Task 5.1); circular imports (Task 1.5); MCP dispatch (Task 2.5); RAGEngine init (Task 4.3). | +| §9 Open Questions | Result type generic syntax (Task 1.4 includes OK constant); logging of errors (Task 3.6 `send()` logs to comms log); backwards-compat shim (Task 2.5 — broken on purpose, contained to MCP dispatch); Result location (`src/result_types.py` chosen). | +| §10 Coordination with Pending Tracks | Task 1.1 (baseline verification); Tasks 3.1-3.7 (Option A rename; send_openai_compatible kept raising; deprecation filterwarnings; ProviderError full removal). | +| §11 Out of Scope | 6 items explicitly out of scope; listed in spec. | +| §12 See Also | Follow-up track registered in tracks.md (Task 5.4); future migration tracks listed in spec but not planned here. | + +**2. Placeholder scan:** No "TBD", "TODO", "implement later", "add appropriate error handling", "Similar to Task N" in the plan. The 8 providers' refactor in Task 3.4 has the same body pattern as the generic example; the implementer copies it for each provider (no need to write 8 copies of the same boilerplate in the plan; the pattern is explicit enough). + +**3. Type consistency:** `Result[T]`, `ErrorInfo`, `ErrorKind`, `NilPath`, `NilRAGState` defined in Phase 1 (Task 1.4); used consistently in Phases 2-4. `_send__result()` signatures mirror `_send_()` (just return type changes from `str` to `Result[str]`). `send_result()` signature in Task 3.5 mirrors `send()` exactly (same parameters, return type is `Result[str]` instead of `str`). + +No issues found. Plan ready for execution.