conductor(plan): review pass — fix cross-references, add NOT_READY + with_errors + Lottes/Valigo, split §3.4 into 8 sub-tasks
This commit is contained in:
@@ -50,8 +50,8 @@
|
||||
},
|
||||
"result_data_model": {
|
||||
"ErrorInfo": "@dataclass(frozen=True) class ErrorInfo: kind: ErrorKind; message: str; source: str; original: BaseException | None",
|
||||
"ErrorKind": "@enum.Enum: NETWORK, AUTH, QUOTA, RATE_LIMIT, BALANCE, PERMISSION, NOT_FOUND, INVALID_INPUT, UNKNOWN, CONFIG, INTERNAL",
|
||||
"Result": "@dataclass(frozen=True) class Result(Generic[T]): data: T; errors: list[ErrorInfo] = field(default_factory=list); @property ok(self) -> bool; with_error(); with_data()",
|
||||
"ErrorKind": "@enum.Enum: NETWORK, AUTH, QUOTA, RATE_LIMIT, BALANCE, PERMISSION, NOT_FOUND, INVALID_INPUT, NOT_READY, UNKNOWN, CONFIG, INTERNAL",
|
||||
"Result": "@dataclass(frozen=True) class Result(Generic[T]): data: T; errors: list[ErrorInfo] = field(default_factory=list); @property ok(self) -> bool; with_error(err); with_errors(errs_batch); with_data(new_data)",
|
||||
"NilPath": "@dataclass(frozen=True) singleton with exists=False, read_text='', errors=[]",
|
||||
"NilRAGState": "@dataclass(frozen=True) singleton with enabled=False, is_empty_result=True, errors=[]"
|
||||
},
|
||||
@@ -100,35 +100,39 @@
|
||||
"verification_criteria": [
|
||||
"src/result_types.py:Result and ErrorInfo exist with the documented fields; NilPath and NilRAGState are module-level singletons",
|
||||
"src/result_types.py:Result is generic over T (Python 3.11+ Generic syntax)",
|
||||
"src/result_types.py:Result.with_error() and with_data() produce modified copies (frozen semantics)",
|
||||
"src/result_types.py:Result.with_error(), with_errors(), and with_data() produce modified copies (frozen semantics)",
|
||||
"src/result_types.py:ErrorKind enum includes NOT_READY (for _require_warmed failures) in addition to the 11 base values",
|
||||
"src/mcp_client.py:_resolve_and_check returns Result[Path] (not tuple); no 'assert p is not None' chain",
|
||||
"src/mcp_client.py:read_file, list_directory, search_files, get_file_summary, etc. return Result[str]",
|
||||
"src/ai_client.py:ProviderError class is removed (no longer raised; ErrorInfo replaces it)",
|
||||
"src/ai_client.py:_classify_*_error() functions return ErrorInfo (not raise)",
|
||||
"src/ai_client.py:_send_<vendor>() functions are renamed to _send_<vendor>_result() and return Result[str]",
|
||||
"src/ai_client.py:send() is decorated with @typing_extensions.deprecated",
|
||||
"src/ai_client.py:send_result() is the new public API returning Result[str, ErrorInfo]",
|
||||
"src/ai_client.py:6 classifier functions return ErrorInfo (not raise): 5 in src/ai_client.py + 1 shared in src/openai_compatible.py + classify_dashscope_error in src/qwen_adapter.py",
|
||||
"src/ai_client.py:8 _send_<vendor>() functions are renamed to _send_<vendor>_result() and return Result[str] (per-vendor atomic commits per plan Tasks 3.4.1-3.4.8)",
|
||||
"src/ai_client.py:send() is decorated with @typing_extensions.deprecated (no double-warn; pick one of decorator or manual warnings.warn)",
|
||||
"src/ai_client.py:send_result() is the new public API returning Result[str]; mirrors send()'s full signature (13+ params including 8 callbacks, read with manual-slop_py_get_definition before implementing)",
|
||||
"src/ai_client.py:_send_<vendor>_result() catches _require_warmed failures and returns Result with ErrorKind.NOT_READY",
|
||||
"src/rag_engine.py:RAGEngine methods return Result (not raise ImportError/ValueError)",
|
||||
"src/rag_engine.py:NilRAGState is used for unconfigured state",
|
||||
"tests/test_result_types.py:8+ tests pass (Result construction, with_error, with_data, NilPath singleton, ErrorKind enum)",
|
||||
"src/rag_engine.py:NilRAGState is used for unconfigured state; _get_state() returns a NilRAGState instance (not the class); tests assert values not identity",
|
||||
"tests/test_result_types.py:11+ tests pass (Result construction, with_error, with_data, with_errors batch, NilPath singleton, ErrorKind enum including NOT_READY, frozen semantics)",
|
||||
"tests/test_mcp_client_paths.py:6+ tests pass (new Result return types)",
|
||||
"tests/test_ai_client_result.py:8+ tests pass (new Result API, deprecation warning)",
|
||||
"tests/test_rag_engine_result.py:4+ tests pass (new Result return types)",
|
||||
"tests/test_deprecation_warnings.py:send() emits exactly one DeprecationWarning per call site (cached)",
|
||||
"tests/test_rag_engine_result.py:4+ tests pass (new Result return types; test_is_empty asserts value, not identity)",
|
||||
"tests/test_deprecation_warnings.py:send() emits DeprecationWarning; send_result() does not",
|
||||
"tests/mcp_dispatch_no_log_when_no_infra: when mcp_client has no comms log, async_dispatch just returns result.data (no error path)",
|
||||
"tests/test_mcp_client.py (existing): no regressions",
|
||||
"tests/test_ai_client.py (existing): no regressions",
|
||||
"tests/test_minimax_provider.py, test_qwen_provider.py, test_llama_provider.py, test_grok_provider.py (existing): no regressions",
|
||||
"tests/test_rag_engine.py (existing): no regressions",
|
||||
"conductor/code_styleguides/error_handling.md: documented with the 5 patterns, Python mappings, decision tree, examples",
|
||||
"conductor/code_styleguides/error_handling.md: documented with the 5 patterns, Python mappings, decision tree, 'Hard Rules' section (Optional[T] forbidden in 3 files), examples",
|
||||
"conductor/product-guidelines.md: new 'Data-Oriented Error Handling' section added",
|
||||
"conductor/workflow.md: new note in Code Style section",
|
||||
"docs/guide_ai_client.md: updated with Result API + deprecation note",
|
||||
"docs/guide_mcp_client.md: updated with Result return types",
|
||||
"conductor/tracks.md: data_oriented_error_handling_20260606 entry added; public_api_migration_20260606 placeholder added",
|
||||
"conductor/tracks.md: data_oriented_error_handling_20260606 entry added; public_api_migration_20260606 placeholder added (separate track, not this one)",
|
||||
"pyproject.toml: typing_extensions>=4.5.0 dependency added",
|
||||
"import src.result_types < 50ms (no heavy imports at top level; verified by scripts/audit_main_thread_imports.py)",
|
||||
"scripts/audit_optional_in_3_files.py: exists; --strict mode fails CI on new Optional[X] in the 3 refactored files",
|
||||
"No new threading.Thread calls in src/ (per project invariant)",
|
||||
"No new Optional[X] in the 3 refactored files (verified by ripgrep)"
|
||||
"No new Optional[X] in the 3 refactored files (verified by ripgrep at every phase checkpoint)"
|
||||
],
|
||||
"links": {
|
||||
"backlog_entry": "conductor/tracks.md (to be added)",
|
||||
|
||||
@@ -140,6 +140,7 @@ def test_error_kind_enum_has_expected_values() -> None:
|
||||
assert ErrorKind.AUTH.value == "auth"
|
||||
assert ErrorKind.RATE_LIMIT.value == "rate_limit"
|
||||
assert ErrorKind.NOT_FOUND.value == "not_found"
|
||||
assert ErrorKind.NOT_READY.value == "not_ready"
|
||||
assert ErrorKind.UNKNOWN.value == "unknown"
|
||||
|
||||
def test_error_info_ui_message_with_source() -> None:
|
||||
@@ -174,6 +175,17 @@ def test_result_with_data_replaces_data_keeps_errors() -> None:
|
||||
assert r2.data == "new value"
|
||||
assert len(r2.errors) == 1
|
||||
|
||||
def test_result_with_errors_appends_batch() -> None:
|
||||
r1: Result[str] = Result(data="hello")
|
||||
errs = [
|
||||
ErrorInfo(kind=ErrorKind.NETWORK, message="a", source="t"),
|
||||
ErrorInfo(kind=ErrorKind.AUTH, message="b", source="t"),
|
||||
]
|
||||
r2 = r1.with_errors(errs)
|
||||
assert r1.errors == [] # original is unchanged (frozen)
|
||||
assert r2.errors == errs
|
||||
assert r2.data == "hello"
|
||||
|
||||
def test_result_is_frozen() -> None:
|
||||
from dataclasses import FrozenInstanceError
|
||||
r: Result[str] = Result(data="x")
|
||||
@@ -229,6 +241,7 @@ class ErrorKind(str, Enum):
|
||||
PERMISSION = "permission"
|
||||
NOT_FOUND = "not_found"
|
||||
INVALID_INPUT = "invalid_input"
|
||||
NOT_READY = "not_ready"
|
||||
UNKNOWN = "unknown"
|
||||
CONFIG = "config"
|
||||
INTERNAL = "internal"
|
||||
@@ -252,6 +265,8 @@ class Result(Generic[T]):
|
||||
return not self.errors
|
||||
def with_error(self, err: ErrorInfo) -> "Result[T]":
|
||||
return Result(data=self.data, errors=[*self.errors, err])
|
||||
def with_errors(self, new_errors: list[ErrorInfo]) -> "Result[T]":
|
||||
return Result(data=self.data, errors=[*self.errors, *new_errors])
|
||||
def with_data(self, new_data: T) -> "Result[T]":
|
||||
return Result(data=new_data, errors=list(self.errors))
|
||||
|
||||
@@ -459,6 +474,16 @@ The 3 refactored subsystems demonstrate each pattern in context. See:
|
||||
- `src/ai_client.py` — `_send_<vendor>_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]`
|
||||
|
||||
## Hard Rules (enforced in the 3 refactored files)
|
||||
|
||||
These are non-negotiable in `src/mcp_client.py`, `src/ai_client.py`, and `src/rag_engine.py`:
|
||||
|
||||
- **`Optional[T]` return types are FORBIDDEN** in the 3 refactored files. Use `Result[T]` (with `NIL_T` singleton if needed) instead. Rationale: `Optional[T]` is the sum type `Union[T, None]` that Fleury's framework replaces. Mixing the two patterns reintroduces the bifurcation the convention is designed to remove.
|
||||
- **Function return types must be `Result[T]` for any function that can fail at runtime.** A function that can't fail (e.g., `get_name() -> str`) doesn't need a `Result`. The classification is "can this return a different value under different runtime conditions?" If yes, `Result`. If no, plain return type.
|
||||
- **Catch SDK exceptions at the boundary only.** Inside the 3 refactored files, the only place an exception is caught is at the SDK call site (e.g., `_send_<vendor>_result()` wrapping the SDK call). Internal `try/except` is reserved for converting `OSError`, `PermissionError`, and similar I/O exceptions to `ErrorInfo` at the mcp_client tool boundary.
|
||||
|
||||
The verification script `scripts/audit_optional_in_3_files.py` (added by this track, see Plan Task 1.6) enforces the `Optional[X]` rule by failing CI if any new `Optional[X]` appears in the 3 refactored files.
|
||||
|
||||
## When to Use This Convention
|
||||
|
||||
**Use it for:**
|
||||
@@ -770,7 +795,7 @@ git commit -m "refactor(mcp_client): _resolve_and_check returns Result[Path]"
|
||||
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)
|
||||
return Result(data="", errors=resolved.errors)
|
||||
p = resolved.data
|
||||
if isinstance(p, NilPath):
|
||||
return Result(data="", errors=resolved.errors)
|
||||
@@ -785,8 +810,6 @@ def read_file(path: str) -> Result[str]:
|
||||
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
|
||||
@@ -852,8 +875,14 @@ git commit -m "refactor(mcp_client): read_file, list_directory, search_files ret
|
||||
|
||||
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**
|
||||
- [ ] **Step 2: Update the dispatch to extract result.data and log result.errors (or just return result.data if mcp_client has no comms log)**
|
||||
|
||||
First verify what logging infrastructure `src/mcp_client.py` has:
|
||||
```bash
|
||||
rg -n "append_comms|comms_log|def log|def _log" src/mcp_client.py
|
||||
```
|
||||
|
||||
**If `src/mcp_client.py` has a comms/log helper (likely a callback registered on app startup):**
|
||||
```python
|
||||
def async_dispatch(name: str, args: dict) -> str:
|
||||
handler = _TOOL_REGISTRY.get(name)
|
||||
@@ -862,11 +891,21 @@ def async_dispatch(name: str, args: dict) -> str:
|
||||
result = handler(**args)
|
||||
if not result.ok:
|
||||
for err in result.errors:
|
||||
_append_comms("WARN", "tool_error", {"tool": name, "error": err.ui_message()})
|
||||
_log_mcp_error(name, err.ui_message()) # adapt to actual function name
|
||||
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.)
|
||||
**If `src/mcp_client.py` has no comms log (simpler case; matches today's behavior where _resolve_and_check returning None just propagated as empty data):**
|
||||
```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)
|
||||
return result.data
|
||||
```
|
||||
|
||||
(The errors are visible in the caller's `result.errors` if they inspect it; for tools that just need the data, returning `""` on failure matches today's behavior. Logging is optional.)
|
||||
|
||||
- [ ] **Step 3: Update existing tests in tests/test_mcp_client.py to use .data**
|
||||
|
||||
@@ -1126,14 +1165,26 @@ git commit -m "test(ai_client): add red tests for new Result API + deprecation w
|
||||
|
||||
---
|
||||
|
||||
## Task 3.3: Refactor _classify_<vendor>_error() to return ErrorInfo (8 vendors)
|
||||
## Task 3.3: Refactor _classify_<vendor>_error() to return ErrorInfo
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ai_client.py` (8 classifier functions)
|
||||
- Modify: `src/ai_client.py` (5 vendor-specific classifiers + call sites in shared helpers)
|
||||
- Modify: `src/qwen_adapter.py` (1 DashScope-specific classifier; different name: `classify_dashscope_error`, no underscore prefix)
|
||||
- Modify: `src/openai_compatible.py` (1 shared classifier for OpenAI-compatible vendors: `_classify_openai_compatible_error`)
|
||||
|
||||
- [ ] **Step 1: Find all the classifier functions**
|
||||
|
||||
Run: `grep -n "def _classify_.*_error" src/ai_client.py`
|
||||
Run:
|
||||
```bash
|
||||
rg -n "def _classify_.*_error|def classify_dashscope" src/ai_client.py src/qwen_adapter.py src/openai_compatible.py
|
||||
```
|
||||
|
||||
Expected (post-qwen-track baseline):
|
||||
- `src/ai_client.py`: 5 functions (`_classify_gemini_error`, `_classify_anthropic_error`, `_classify_deepseek_error`, `_classify_minimax_error`, `_classify_gemini_cli_error`)
|
||||
- `src/qwen_adapter.py`: 1 function (`classify_dashscope_error`, no underscore prefix)
|
||||
- `src/openai_compatible.py`: 1 function (`_classify_openai_compatible_error`, shared by qwen/llama/grok via `send_openai_compatible`)
|
||||
|
||||
**Note on the 8 vendors / 6 classifiers split:** Qwen, Llama, and Grok all route through the shared `send_openai_compatible()` helper (qwen via DashScope-specific adapter, llama and grok via OpenAI-compatible). They share `_classify_openai_compatible_error`. There are 8 `_send_*_result()` functions (one per vendor) but only 6 classifier functions. The 8 → 6 mismatch is intentional, not an oversight.
|
||||
|
||||
- [ ] **Step 2: Refactor each classifier to return ErrorInfo (not raise ProviderError)**
|
||||
|
||||
@@ -1157,7 +1208,7 @@ def _classify_gemini_error(exc: Exception, source: str = "ai_client.gemini") ->
|
||||
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`.)
|
||||
(Apply to all 6 classifiers across 3 files. The 5 in `src/ai_client.py` get the `_result` rename pattern indirectly via their callers in `_send_*_result()`. `classify_dashscope_error` in `src/qwen_adapter.py` keeps its name (no underscore prefix) but its signature changes from `raise ProviderError` to `return ErrorInfo`. `_classify_openai_compatible_error` in `src/openai_compatible.py` becomes a value-returning function but stays as the SDK-boundary classifier per the convention — it never raises after this refactor.)
|
||||
|
||||
- [ ] **Step 3: Run the test_ai_client_result.py tests; `test_classify_gemini_error_returns_error_info` should now pass**
|
||||
|
||||
@@ -1168,7 +1219,7 @@ Expected: 1 test PASS.
|
||||
|
||||
```bash
|
||||
git add src/ai_client.py
|
||||
git commit -m "refactor(ai_client): _classify_<vendor>_error() returns ErrorInfo (8 vendors)"
|
||||
git commit -m "refactor(ai_client): _classify_<vendor>_error() returns ErrorInfo (5 in ai_client + 1 shared + 1 qwen)"
|
||||
```
|
||||
|
||||
---
|
||||
@@ -1221,12 +1272,48 @@ uv run pytest tests/test_ai_client.py tests/test_minimax_provider.py tests/test_
|
||||
|
||||
Expected: tests that directly call `_send_<vendor>()` 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)**
|
||||
**Task 3.4 is split into 8 per-vendor sub-tasks (3.4.1 - 3.4.8) for atomic per-vendor commits. Each sub-task follows the same pattern but operates on one vendor. The implementer does NOT execute Task 3.4 monolithically.**
|
||||
|
||||
```bash
|
||||
git add src/ai_client.py
|
||||
git commit -m "refactor(ai_client): rename _send_<vendor>() to _send_<vendor>_result() returning Result[str]"
|
||||
```
|
||||
---
|
||||
|
||||
### Task 3.4.1: Rename _send_gemini to _send_gemini_result
|
||||
|
||||
- [ ] **Step 1**: Read current `_send_gemini` with `manual-slop_py_get_definition src/ai_client.py _send_gemini`
|
||||
- [ ] **Step 2**: Rename to `_send_gemini_result`, change return type to `Result[str]`, wrap body per the generic pattern in Task 3.4 Step 2 (using `_classify_gemini_error` with `source="ai_client.gemini"`)
|
||||
- [ ] **Step 3**: Update any internal callers of `_send_gemini` in `src/ai_client.py` to use the new name + extract `.data`
|
||||
- [ ] **Step 4**: `uv run pytest tests/test_gemini_cli_adapter.py tests/test_ai_client.py 2>&1 | tail -10` — expect tests calling `send()` still pass; tests calling `_send_gemini` directly now FAIL
|
||||
- [ ] **Step 5**: Commit: `git commit -m "refactor(ai_client): _send_gemini_result() returns Result[str]"`
|
||||
|
||||
### Task 3.4.2: Rename _send_anthropic to _send_anthropic_result
|
||||
|
||||
(Same pattern as 3.4.1; uses `_classify_anthropic_error` with `source="ai_client.anthropic"`.)
|
||||
|
||||
### Task 3.4.3: Rename _send_deepseek to _send_deepseek_result
|
||||
|
||||
(Same pattern; uses `_classify_deepseek_error` with `source="ai_client.deepseek"`.)
|
||||
|
||||
### Task 3.4.4: Rename _send_minimax to _send_minimax_result
|
||||
|
||||
(Same pattern; uses `_classify_minimax_error` with `source="ai_client.minimax"`. Note: `_send_minimax` is already short after the qwen track's refactor to use `send_openai_compatible`; only the outer wrapper needs the rename.)
|
||||
|
||||
### Task 3.4.5: Rename _send_gemini_cli to _send_gemini_cli_result
|
||||
|
||||
(Same pattern; uses `_classify_gemini_cli_error` with `source="ai_client.gemini_cli"`.)
|
||||
|
||||
### Task 3.4.6: Rename _send_qwen to _send_qwen_result
|
||||
|
||||
(Same pattern; uses `classify_dashscope_error` from `src/qwen_adapter.py` with `source="ai_client.qwen"`.)
|
||||
|
||||
### Task 3.4.7: Rename _send_llama to _send_llama_result
|
||||
|
||||
(Same pattern; uses `_classify_openai_compatible_error` from `src/openai_compatible.py` with `source="ai_client.llama"`.)
|
||||
|
||||
### Task 3.4.8: Rename _send_grok to _send_grok_result
|
||||
|
||||
(Same pattern; uses `_classify_openai_compatible_error` from `src/openai_compatible.py` with `source="ai_client.grok"`.)
|
||||
|
||||
- [ ] **Post-sub-task verification** (after 3.4.8): Run the full vendor test set: `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`
|
||||
- [ ] **Post-sub-task commit** (if final cleanup): `git commit -m "refactor(ai_client): all 8 _send_<vendor>_result() functions return Result[str]" --allow-empty`
|
||||
|
||||
---
|
||||
|
||||
@@ -1299,7 +1386,7 @@ git commit -m "feat(ai_client): add send_result() public API returning Result[st
|
||||
|
||||
---
|
||||
|
||||
## Task 3.6: Mark send() as @deprecated and rewire it to call send_result()
|
||||
## Task 3.6: Mark send() as deprecated and rewire it to call send_result()
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ai_client.py`
|
||||
@@ -1307,16 +1394,18 @@ git commit -m "feat(ai_client): add send_result() public API returning Result[st
|
||||
- [ ] **Step 1: Add the deprecation import at the top of src/ai_client.py**
|
||||
|
||||
```python
|
||||
import warnings
|
||||
from typing_extensions import deprecated
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Wrap the existing send() with @deprecated**
|
||||
(`warnings` is already imported at module top in most files; verify with `rg "^import warnings|^from warnings" src/ai_client.py` and add the import only if missing.)
|
||||
|
||||
- [ ] **Step 2: Wrap the existing send() with @deprecated + manual warnings.warn (single warning, cached by Python's warning registry)**
|
||||
|
||||
```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. "
|
||||
@@ -1331,7 +1420,9 @@ def send(...) -> str:
|
||||
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.)
|
||||
(If the manual `warnings.warn` is dropped per the recommendation above, the function body starts with `result = send_result(...)` and the manual warning block is removed. The `@deprecated` decorator handles the warning.)
|
||||
|
||||
(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. The `@deprecated` decorator emits a `DeprecationWarning` at type-checker level (mypy/pyright hint) AND at runtime. The manual `warnings.warn` is suppressed by the `@deprecated` decorator's effect (the decorator's `__init__.subclass__` wrapping calls `warnings.warn` once per call site; the manual call adds a second per-call-site fire). To avoid double-warnings, the implementer may drop the manual `warnings.warn` and rely on the decorator alone, OR drop the decorator and rely on the manual warn + a `# type: ignore[deprecated]` comment for the type checker. **Pick one** — recommended: keep the `@deprecated` decorator and remove the manual `warnings.warn` block. Update this plan task during execution to match whichever is chosen.)
|
||||
|
||||
- [ ] **Step 3: Run test_deprecation_warnings.py; confirm 2 tests pass**
|
||||
|
||||
@@ -1343,19 +1434,28 @@ Expected: 2 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)**
|
||||
- [ ] **Step 5: Silence the deprecation warning in existing tests via filterwarnings (so test output isn't spammed)**
|
||||
|
||||
Add to `tests/conftest.py` (verify with `rg -n "filterwarnings" tests/conftest.py` first; if already present, append the new entry):
|
||||
```python
|
||||
filterwarnings("ignore::DeprecationWarning:src.ai_client", category=DeprecationWarning, module=r"src\.ai_client")
|
||||
```
|
||||
|
||||
This silences the `DeprecationWarning` emitted by `send()` during the transition period. The `test_deprecation_warnings.py` tests use `warnings.catch_warnings(record=True)` to opt in to the warning capture explicitly, so the filter does not affect them.
|
||||
|
||||
- [ ] **Step 6: Run the 8 vendor test files; confirm no regressions (most tests call send() which now emits a warning but the filter silences it)**
|
||||
|
||||
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()).
|
||||
Expected: tests pass (no DeprecationWarning in stderr thanks to the filter; `test_deprecation_warnings.py` opt-in tests still capture the warning).
|
||||
|
||||
- [ ] **Step 6: Commit**
|
||||
- [ ] **Step 7: Commit**
|
||||
|
||||
```bash
|
||||
git add src/ai_client.py
|
||||
git add src/ai_client.py tests/conftest.py
|
||||
git commit -m "feat(ai_client): mark send() @deprecated; rewire to call send_result()"
|
||||
```
|
||||
|
||||
@@ -1429,7 +1529,7 @@ Expected: same pre-existing failures; no new failures.
|
||||
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_<vendor>_error() functions return ErrorInfo. All 8 _send_<vendor>() functions renamed to _send_<vendor>_result() and return Result[str]. New public send_result() API. send() marked @deprecated (still works, emits DeprecationWarning). 8 new tests pass + existing tests pass.
|
||||
git notes add -m "Phase 3 checkpoint: ai_client.py refactored. ProviderError exception REMOVED. 6 _classify_<vendor>_error() functions return ErrorInfo (5 in src/ai_client.py + 1 shared in src/openai_compatible.py + 1 in src/qwen_adapter.py as classify_dashscope_error). All 8 _send_<vendor>() functions renamed to _send_<vendor>_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"
|
||||
```
|
||||
@@ -1514,8 +1614,10 @@ def test_is_empty_uses_nil_rag_state_when_not_configured() -> None:
|
||||
config.enabled = False
|
||||
engine = RAGEngine(base_dir="/tmp", config=config)
|
||||
state = engine._get_state()
|
||||
assert state is NilRAGState
|
||||
assert isinstance(state, NilRAGState)
|
||||
assert state.enabled is False
|
||||
assert state.is_empty_result is True
|
||||
assert state.errors == []
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run, confirm 4 tests fail**
|
||||
@@ -1681,48 +1783,13 @@ git commit -m "conductor(plan): mark Phase 4 complete in data_oriented_error_han
|
||||
|
||||
# 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.
|
||||
> Goal: Update the deep-dive docs. Register the `public_api_migration_20260606` follow-up placeholder. Manual smoke test. Archive the track.
|
||||
|
||||
**Note**: The `filterwarnings` entry that silences the `ai_client.send()` deprecation in existing tests is added in Task 3.6 Step 5 (the same phase that introduces the deprecation), not deferred to Phase 5. This avoids shipping deprecation-warn-spammy test output during the Phase 3-4 window.
|
||||
|
||||
---
|
||||
|
||||
## 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
|
||||
## Task 5.1: Update docs/guide_ai_client.md
|
||||
|
||||
**Files:**
|
||||
- Modify: `docs/guide_ai_client.md`
|
||||
@@ -1780,7 +1847,7 @@ git commit -m "docs(ai_client): document Result API + deprecation"
|
||||
|
||||
---
|
||||
|
||||
## Task 5.3: Update docs/guide_mcp_client.md
|
||||
## Task 5.2: Update docs/guide_mcp_client.md
|
||||
|
||||
**Files:**
|
||||
- Modify: `docs/guide_mcp_client.md` (if it exists; if not, create it)
|
||||
@@ -1802,7 +1869,7 @@ git commit -m "docs(mcp_client): document new Result return types + nil-sentinel
|
||||
|
||||
---
|
||||
|
||||
## Task 5.4: Add public_api_migration_20260606 placeholder to conductor/tracks.md
|
||||
## Task 5.3: Add public_api_migration_20260606 placeholder to conductor/tracks.md
|
||||
|
||||
**Files:**
|
||||
- Modify: `conductor/tracks.md`
|
||||
@@ -1823,7 +1890,7 @@ git commit -m "conductor(tracks): register public_api_migration_20260606 follow-
|
||||
|
||||
---
|
||||
|
||||
## Task 5.5: Manual smoke test
|
||||
## Task 5.4: Manual smoke test
|
||||
|
||||
**Files:** none (manual verification)
|
||||
|
||||
@@ -1852,7 +1919,7 @@ if git diff --cached --quiet; then echo "no smoke test doc to commit"; else git
|
||||
|
||||
---
|
||||
|
||||
## Task 5.6: Phase 5 checkpoint (TRACK COMPLETE)
|
||||
## Task 5.5: Phase 5 checkpoint (TRACK COMPLETE)
|
||||
|
||||
**Files:**
|
||||
- Modify: `conductor/tracks/data_oriented_error_handling_20260606/state.toml`
|
||||
@@ -1881,7 +1948,7 @@ 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_<vendor>_error() return ErrorInfo; 8 _send_<vendor>_result() return Result[str]; send() @deprecated; send_result() is the new public API
|
||||
- src/ai_client.py: ProviderError REMOVED; 6 classifier functions return ErrorInfo (5 _classify_<vendor>_error + 1 shared _classify_openai_compatible_error in src/openai_compatible.py + classify_dashscope_error in src/qwen_adapter.py); 8 _send_<vendor>_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
|
||||
@@ -1902,7 +1969,7 @@ git commit -m "conductor(plan): mark Phase 5 complete in data_oriented_error_han
|
||||
|
||||
---
|
||||
|
||||
## Task 5.7: Archive the track
|
||||
## Task 5.6: Archive the track
|
||||
|
||||
**Files:**
|
||||
- Move: `conductor/tracks/data_oriented_error_handling_20260606/` → `conductor/tracks/archive/data_oriented_error_handling_20260606/`
|
||||
@@ -1934,20 +2001,20 @@ git commit -m "conductor(archive): ship data_oriented_error_handling_20260606 to
|
||||
| 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. |
|
||||
| §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.3. |
|
||||
| §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). |
|
||||
| §4.4 convention docs | Task 1.6 (styleguide), Tasks 1.7-1.8 (product-guidelines + workflow), Tasks 5.1-5.2 (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). |
|
||||
| §8 Risks | All 6 risks addressed: ProviderError catch (Task 3.7); asserts (Task 2.6); deprecation spam (Task 3.6 Step 5 — filterwarnings added in same phase as the deprecation, not deferred to Phase 5); 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). |
|
||||
| §10 Coordination with Pending Tracks | Task 1.1 (baseline verification); Tasks 3.1-3.8 (Option A rename; send_openai_compatible kept raising; deprecation filterwarnings added in Task 3.6 Step 5; 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. |
|
||||
| §12 See Also | Follow-up track registered in tracks.md (Task 5.3); 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).
|
||||
|
||||
|
||||
@@ -52,6 +52,18 @@ A new **public `Result`-based API** (`ai_client.send_result()`) is introduced fo
|
||||
| 4 | **AND over OR (Result struct with side-channel errors)** | `@dataclass(frozen=True) class Result: data: T; errors: list[ErrorInfo]`. Caller: `r = fn(); if r.errors: handle(); else: use(r.data)`. Empty errors list = success. | `src/result_types.py:Result`; used by all 3 refactored files. |
|
||||
| 5 | **Error info as side-channel** | Per-context error list in the Result struct. The list accumulates all errors encountered, not just the first one. Simpler than C's `errno` (which is single-slot); richer than just raising one exception. | `src/result_types.py:ErrorInfo`; populated by error-classification helpers. |
|
||||
|
||||
#### 3.1.1 3rd-Party Validation (independent corroboration)
|
||||
|
||||
The "errors are data, not control flow" thesis is independently supported by two other practitioners in the data-oriented / C-style community:
|
||||
|
||||
- **Timothy Lottes (@NOTimothyLottes), 2026-06-07** — [X thread]. "Error codes, many APIs get these so wrong. For example aliasing the same code with multiple meaning so the user has zero idea what actually went wrong and what needs fixing." Lottes's pattern: a force-no-inline `ERROR[__line__]: _code_` exit point where the exit code IS the source line number. Errors are zero-cost at init time; "all my error checks are init time (low cost) and only fail just results in this common Err() with printed {line, code} exit path." This track's `Result` dataclass is the Python analog: an `ErrorInfo` with a `source` field and an optional `location: int` (future enhancement) carries the same diagnostic information Lottes's exit code does.
|
||||
|
||||
**Lottes's anti-pattern warning, applied to `ErrorKind`:** "aliasing the same code with multiple meaning" — each `ErrorKind` value has exactly one meaning. Adding a new kind for a new failure mode is preferred over overloading an existing one. The 11 enum values (`NETWORK`, `AUTH`, `QUOTA`, `RATE_LIMIT`, `BALANCE`, `PERMISSION`, `NOT_FOUND`, `INVALID_INPUT`, `NOT_READY`, `UNKNOWN`, `CONFIG`, `INTERNAL`) are the canonical set; if a new failure mode doesn't fit, add a new value, don't overload `UNKNOWN`.
|
||||
|
||||
- **Valigo (@valigotech), "Exceptions are horrifying", 2026-06-07** — YouTube, 14 min. Exceptions "mess with control flow in very weird ways"; the caller can no longer read top-to-bottom and predict what happens. TypeScript's failure to express "this throws" is what motivated the Effect library (a Rust-style `Result<T, E>` port). "Modern languages without legacy baggage move away from exceptions — Rust, Jai, Zig, Odin." JavaScript's worst abuse: throwing a `Promise` for Suspense. "Every time you open a website, you see like six different spinners all over the place."
|
||||
|
||||
**Valigo's anti-pattern warning, applied to this codebase:** `ErrorInfo` is a value, never a thrown object. Do not raise it; do not yield it from a generator; do not pass it as a side-effect return; do not use it as a `Promise` rejection value. It is a data value, period. The Hook API's `/api/ask` Remote Confirmation Protocol (a long-running challenge/response) is conceptually similar to Suspense but is **not** an exception mechanism — it returns a JSON object with a `request_id` and a status, not a thrown value. Future code that adds new cross-thread communication patterns must not smuggle exception-like control flow under the guise of a "request."
|
||||
|
||||
### 3.2 Module Layout
|
||||
|
||||
```
|
||||
@@ -99,6 +111,7 @@ class ErrorKind(str, Enum):
|
||||
PERMISSION = "permission"
|
||||
NOT_FOUND = "not_found"
|
||||
INVALID_INPUT = "invalid_input"
|
||||
NOT_READY = "not_ready"
|
||||
UNKNOWN = "unknown"
|
||||
CONFIG = "config"
|
||||
INTERNAL = "internal"
|
||||
@@ -122,6 +135,8 @@ class Result(Generic[T]):
|
||||
return not self.errors
|
||||
def with_error(self, err: ErrorInfo) -> "Result[T]":
|
||||
return Result(data=self.data, errors=[*self.errors, err])
|
||||
def with_errors(self, new_errors: list[ErrorInfo]) -> "Result[T]":
|
||||
return Result(data=self.data, errors=[*self.errors, *new_errors])
|
||||
def with_data(self, new_data: T) -> "Result[T]":
|
||||
return Result(data=new_data, errors=list(self.errors))
|
||||
```
|
||||
@@ -240,7 +255,7 @@ def read_file(path: str) -> Result[str]:
|
||||
"""Returns Result[str]. On success, .data is the file's text. On failure, .data is '' and .errors is populated."""
|
||||
resolved = _resolve_and_check(path)
|
||||
if not resolved.ok:
|
||||
return Result(data="").with_errors_from(resolved)
|
||||
return Result(data="", errors=resolved.errors)
|
||||
p = resolved.data
|
||||
if not p.exists():
|
||||
return Result(data="", errors=[ErrorInfo(kind=ErrorKind.NOT_FOUND, message=f"file not found: {path}", source="mcp.read_file")])
|
||||
@@ -473,6 +488,23 @@ All existing configs (`config.toml`, `credentials.toml`, per-project TOML) work
|
||||
|
||||
**Mocking strategy:** Existing tests use `unittest.mock.patch` on SDK calls; no changes needed. New tests use the same pattern.
|
||||
|
||||
**Baseline verification (Phase 1):** Run a project-wide grep to record the post-tracks baseline:
|
||||
```bash
|
||||
rg "ai_client\.send\(" --type py | wc -l # direct callers of the public send()
|
||||
rg "_send_(gemini|anthropic|deepseek|minimax|gemini_cli|qwen|llama|grok)\(" src/ -n # direct callers of private _send_<vendor>() — should be 0 post-qwen-track
|
||||
rg "Optional\[" src/mcp_client.py src/ai_client.py src/rag_engine.py | wc -l # baseline Optional usage in the 3 refactored files
|
||||
```
|
||||
|
||||
The numbers go in `state.toml [verification]`:
|
||||
```toml
|
||||
[baseline_post_qwen_track]
|
||||
ai_client_send_callers_in_src = 0 # will be 0 — this track is upstream of callers
|
||||
ai_client_send_callers_in_tests = 0 # record actual count from rg
|
||||
optional_in_3_files = 0 # record actual count from rg
|
||||
```
|
||||
|
||||
The follow-up `public_api_migration_20260606` track uses these as its starting baseline. The `no_new_optional_in_3_files` verification criterion is "the count does not grow during this track" — verified by re-running the grep at Phase 2, 3, 4, 5 checkpoints.
|
||||
|
||||
**Integration verification:** Manual smoke test in the GUI: send a message that exercises the new patterns end-to-end. Document the smoke test in the Phase 5 checkpoint git note.
|
||||
|
||||
## 7. Migration / Rollout
|
||||
@@ -629,7 +661,17 @@ If any of the expected new files are missing, the implementer reports a coordina
|
||||
|
||||
### 12.1 Follow-up Track (planned in §12.1 placeholder; detailed in conductor/tracks.md)
|
||||
|
||||
**"Public API Result Migration"** (`public_api_migration_20260606`) — Removes the deprecated `ai_client.send()`. Migrates all callers (`multi_agent_conductor.py`, `app_controller.py`, ~50+ test files) to `send_result()`. Adds any new public API surface needed (e.g., per-ticket `Result` returns in the MMA conductor). This is the **only** follow-up that this spec plans; the other future migrations are listed below for reference but not planned here.
|
||||
**"Public API Result Migration"** (`public_api_migration_20260606`) — Removes the deprecated `ai_client.send()`. Migrates all callers to `send_result()`. Adds any new public API surface needed (e.g., per-ticket `Result` returns in the MMA conductor). This is the **only** follow-up that this spec plans; the other future migrations are listed below for reference but not planned here.
|
||||
|
||||
**Baseline verification (run during the follow-up track's Phase 1):**
|
||||
The complete list of `ai_client.send()` direct callers in `src/` (verified 2026-06-08):
|
||||
- `src/app_controller.py:290` — `_api_generate` body
|
||||
- `src/app_controller.py:3559` — second call site
|
||||
- `src/multi_agent_conductor.py:591` — MMA worker dispatch
|
||||
- `src/orchestrator_pm.py:86` — orchestrator project manager
|
||||
- `src/conductor_tech_lead.py:68` — Tech Lead sub-agent
|
||||
|
||||
Plus ~50+ test files that call `send()` directly. The follow-up track's `rg "ai_client\.send\(" --type py | wc -l` baseline should match these numbers before migration begins. Tests that call `_send_<vendor>()` directly (rather than `send()`) are also affected by the `Task 3.4` rename and need migration to `_send_<vendor>_result()`.
|
||||
|
||||
### 12.2 Future Migration Tracks (prioritized; NOT planned in this spec)
|
||||
|
||||
|
||||
@@ -50,18 +50,15 @@ t2_7 = { status = "pending", commit_sha = "", description = "Remove the 30+ 'ass
|
||||
t2_8 = { status = "pending", commit_sha = "", description = "Update the tool dispatch internals (mcp_client.async_dispatch) to extract result.data and log result.errors via comms log" }
|
||||
t2_9 = { status = "pending", commit_sha = "", description = "Run full test suite; ensure no regressions in tests/test_mcp_client.py" }
|
||||
t2_10 = { status = "pending", commit_sha = "", description = "Phase 2 checkpoint commit + git note" }
|
||||
# Phase 3: ai_client.py refactor (HIGHEST RISK)
|
||||
t3_1 = { status = "pending", commit_sha = "", description = "Red: tests/test_ai_client_result.py (verify _send_<vendor>_result returns Result[str]; verify send_result public API; verify ProviderError is removed)" }
|
||||
t3_2 = { status = "pending", commit_sha = "", description = "Red: tests/test_deprecation_warnings.py (verify send() emits DeprecationWarning)" }
|
||||
t3_3 = { status = "pending", commit_sha = "", description = "Refactor _classify_<vendor>_error() to return ErrorInfo (not raise ProviderError); remove the raise statement" }
|
||||
t3_4 = { status = "pending", commit_sha = "", description = "Refactor _send_<vendor>() -> _send_<vendor>_result() for all 8 vendors (Gemini, Anthropic, DeepSeek, MiniMax, Gemini CLI, Qwen, Llama, Grok); new return type is Result[str]" }
|
||||
t3_5 = { status = "pending", commit_sha = "", description = "Remove the ProviderError class from src/ai_client.py" }
|
||||
t3_6 = { status = "pending", commit_sha = "", description = "Remove the now-dead 'except ProviderError' clause (line 1338)" }
|
||||
t3_7 = { status = "pending", commit_sha = "", description = "Add send_result() public API to src/ai_client.py; returns Result[str]" }
|
||||
t3_8 = { status = "pending", commit_sha = "", description = "Add @typing_extensions.deprecated decorator to send(); verify it emits DeprecationWarning at first call per site" }
|
||||
t3_9 = { status = "pending", commit_sha = "", description = "Run full test suite; check for deprecation warning spam in test output; add filterwarnings to tests/conftest.py if needed" }
|
||||
t3_10 = { status = "pending", commit_sha = "", description = "Run all 8 vendor test files (test_minimax_provider, test_qwen_provider, test_llama_provider, test_grok_provider, test_ai_client_cli, test_deepseek_provider, etc.); ensure no regressions" }
|
||||
t3_11 = { status = "pending", commit_sha = "", description = "Phase 3 checkpoint commit + git note" }
|
||||
# Phase 3: ai_client.py refactor (HIGHEST RISK) - mirrors plan Tasks 3.1-3.8
|
||||
t3_1 = { status = "pending", commit_sha = "", description = "Baseline: verify existing 8 vendor test files pass before refactor" }
|
||||
t3_2 = { status = "pending", commit_sha = "", description = "Red: tests/test_ai_client_result.py + tests/test_deprecation_warnings.py" }
|
||||
t3_3 = { status = "pending", commit_sha = "", description = "Refactor 6 classifier functions to return ErrorInfo: 5 in src/ai_client.py (_classify_gemini_error, _classify_anthropic_error, _classify_deepseek_error, _classify_minimax_error, _classify_gemini_cli_error) + 1 in src/openai_compatible.py (_classify_openai_compatible_error, shared by qwen/llama/grok) + 1 in src/qwen_adapter.py (classify_dashscope_error, no underscore prefix)" }
|
||||
t3_4 = { status = "pending", commit_sha = "", description = "Rename _send_<vendor>() to _send_<vendor>_result() for all 8 vendors (Gemini, Anthropic, DeepSeek, MiniMax, Gemini CLI, Qwen, Llama, Grok); new return type is Result[str]. Per-vendor atomic commits (8 sub-tasks in plan)." }
|
||||
t3_5 = { status = "pending", commit_sha = "", description = "Add send_result() public API to src/ai_client.py; returns Result[str]; mirrors existing send() signature (13+ parameters including 8 callbacks - read with manual-slop_py_get_definition)" }
|
||||
t3_6 = { status = "pending", commit_sha = "", description = "Mark send() as @deprecated + rewire to call send_result() + add filterwarnings to tests/conftest.py to silence deprecation in existing tests" }
|
||||
t3_7 = { status = "pending", commit_sha = "", description = "Remove the ProviderError class from src/ai_client.py + remove dead 'except ProviderError' clause" }
|
||||
t3_8 = { status = "pending", commit_sha = "", description = "Phase 3 checkpoint commit + git note" }
|
||||
# Phase 4: rag_engine.py refactor
|
||||
t4_1 = { status = "pending", commit_sha = "", description = "Red: tests/test_rag_engine_result.py (verify RAG methods return Result; verify NilRAGState used)" }
|
||||
t4_2 = { status = "pending", commit_sha = "", description = "Refactor RAGEngine._init_vector_store to return Result[None] (replaces raise ImportError / ValueError)" }
|
||||
@@ -69,16 +66,15 @@ t4_3 = { status = "pending", commit_sha = "", description = "Refactor RAGEngine.
|
||||
t4_4 = { status = "pending", commit_sha = "", description = "Refactor RAGEngine.is_empty, add_documents, search, index_file to return Result where appropriate" }
|
||||
t4_5 = { status = "pending", commit_sha = "", description = "Verify tests/test_rag_engine.py still passes (no regressions)" }
|
||||
t4_6 = { status = "pending", commit_sha = "", description = "Phase 4 checkpoint commit + git note" }
|
||||
# Phase 5: Deprecation wiring + docs + integration
|
||||
t5_1 = { status = "pending", commit_sha = "", description = "Add filterwarnings('ignore::DeprecationWarning:src.ai_client') to tests/conftest.py to silence the send() deprecation in existing tests" }
|
||||
t5_2 = { status = "pending", commit_sha = "", description = "Update docs/guide_ai_client.md: new 'Data-Oriented Error Handling (Fleury Pattern)' section; document the Result API; document the deprecation" }
|
||||
t5_3 = { status = "pending", commit_sha = "", description = "Update docs/guide_mcp_client.md: document the new Result return types; explain the nil-sentinel pattern" }
|
||||
t5_4 = { status = "pending", commit_sha = "", description = "Add public_api_migration_20260606 placeholder to conductor/tracks.md (in the Remaining Backlog section)" }
|
||||
t5_5 = { status = "pending", commit_sha = "", description = "Manual smoke test: launch GUI; send a message; verify Result path works end-to-end; verify deprecation warning fires once when send() is called" }
|
||||
t5_6 = { status = "pending", commit_sha = "", description = "Phase 5 checkpoint commit + git note (TRACK COMPLETE)" }
|
||||
t5_7 = { status = "pending", commit_sha = "", description = "git mv conductor/tracks/data_oriented_error_handling_20260606 to conductor/tracks/archive/" }
|
||||
t5_8 = { status = "pending", commit_sha = "", description = "Update conductor/tracks.md: move data_oriented_error_handling_20260606 entry to Recently Completed" }
|
||||
t5_9 = { status = "pending", commit_sha = "", description = "Final state.toml update: mark all phases completed; add final note" }
|
||||
# Phase 5: Deprecation wiring + docs + integration - mirrors plan Tasks 5.1-5.6
|
||||
# Note: The filterwarnings entry that silences send() deprecation in existing tests
|
||||
# is added in plan Task 3.6 Step 5 (same phase as the deprecation), not here.
|
||||
t5_1 = { status = "pending", commit_sha = "", description = "Update docs/guide_ai_client.md: new 'Data-Oriented Error Handling (Fleury Pattern)' section; document the Result API; document the deprecation" }
|
||||
t5_2 = { status = "pending", commit_sha = "", description = "Update docs/guide_mcp_client.md: document the new Result return types; explain the nil-sentinel pattern" }
|
||||
t5_3 = { status = "pending", commit_sha = "", description = "Add public_api_migration_20260606 placeholder to conductor/tracks.md (in the Remaining Backlog section)" }
|
||||
t5_4 = { status = "pending", commit_sha = "", description = "Manual smoke test: launch GUI; send a message; verify Result path works end-to-end; verify deprecation warning fires once when send() is called" }
|
||||
t5_5 = { status = "pending", commit_sha = "", description = "Phase 5 checkpoint commit + git note (TRACK COMPLETE)" }
|
||||
t5_6 = { status = "pending", commit_sha = "", description = "Archive the track: git mv conductor/tracks/data_oriented_error_handling_20260606 to conductor/tracks/archive/ + update tracks.md (move entry to Recently Completed) + final state.toml update" }
|
||||
|
||||
[verification]
|
||||
# Filled as phases complete
|
||||
@@ -98,17 +94,27 @@ full_test_suite_passes = false
|
||||
no_new_optional_in_3_files = false
|
||||
no_new_threading_thread_calls = false
|
||||
import_src_result_types_fast = false
|
||||
# New verification flags (2026-06-08 revision)
|
||||
not_ready_kind_in_enum = false
|
||||
with_errors_batch_helper = false
|
||||
per_vendor_send_rename_commits = 0 # 8 expected (Tasks 3.4.1-3.4.8)
|
||||
optional_in_3_files_baseline_recorded = false
|
||||
hard_rules_section_in_styleguide = false
|
||||
external_validation_cited = false # Lottes + Valigo references in spec §3.1.1
|
||||
audit_optional_script_added = false # scripts/audit_optional_in_3_files.py
|
||||
deprecation_filterwarnings_at_phase_3 = false # added in plan Task 3.6 Step 5, NOT Phase 5
|
||||
|
||||
[result_types_coverage]
|
||||
# Filled as tasks complete
|
||||
result_construction = false
|
||||
result_with_error = false
|
||||
result_with_errors_batch = false # NEW: covers the O(n²) -> O(n) optimization
|
||||
result_with_data = false
|
||||
result_ok_property = false
|
||||
result_frozen = false
|
||||
nil_path_singleton = false
|
||||
nil_rag_state_singleton = false
|
||||
error_kind_enum = false
|
||||
error_kind_enum = false # covers all 12 values including NOT_READY
|
||||
error_info_ui_message = false
|
||||
|
||||
[mcp_client_refactor_stats]
|
||||
@@ -123,9 +129,9 @@ tests_pass_after = 0
|
||||
send_renamed_to_send_result = false
|
||||
provider_error_removed = false
|
||||
_send_renamed_to_result = 0
|
||||
of_total = 0
|
||||
of_total_send = 0 # was the second 'of_total' - renamed for clarity (8 expected)
|
||||
classify_error_returns_error_info = 0
|
||||
of_total = 0
|
||||
of_total_classify = 0 # was the first 'of_total' - renamed for clarity (6 expected)
|
||||
deprecation_warning_emitted = false
|
||||
tests_pass_before = 0
|
||||
tests_pass_after = 0
|
||||
@@ -143,4 +149,22 @@ tests_pass_after = 0
|
||||
track_id = "public_api_migration_20260606"
|
||||
status = "planned_in_data_oriented_error_handling_20260606"
|
||||
removes = ["ai_client.send()"]
|
||||
migrates = ["multi_agent_conductor.py", "app_controller.py", "tests/*"]
|
||||
# 4 direct production callers in src/ (verified 2026-06-08 via rg):
|
||||
migrates = [
|
||||
"src/app_controller.py:290",
|
||||
"src/app_controller.py:3559",
|
||||
"src/multi_agent_conductor.py:591",
|
||||
"src/orchestrator_pm.py:86",
|
||||
"src/conductor_tech_lead.py:68",
|
||||
"tests/* (~50+ test files calling ai_client.send() directly)"
|
||||
]
|
||||
|
||||
[baseline_post_qwen_track]
|
||||
# Recorded at Phase 1 Task 1.1; baseline for the follow-up public_api_migration track
|
||||
ai_client_send_callers_in_src = 5 # 4 production + see spec §12.1
|
||||
ai_client_send_callers_in_tests = 0 # fill from `rg "ai_client\.send\(" --type py | wc -l` at Phase 1
|
||||
optional_in_3_files = 0 # fill from `rg "Optional\[" src/mcp_client.py src/ai_client.py src/rag_engine.py | wc -l`
|
||||
send_callsites_to_migrate = 0 # fill at end of Phase 3 = number of test files updated for the new API
|
||||
|
||||
# Per-vendor refactor commits (Task 3.4.1 - 3.4.8)
|
||||
send_renamed_commits = [] # one commit SHA per vendor, in order
|
||||
|
||||
Reference in New Issue
Block a user