Private
Public Access
archive: completed data oriented error handling
This commit is contained in:
@@ -0,0 +1,155 @@
|
||||
{
|
||||
"track_id": "data_oriented_error_handling_20260606",
|
||||
"name": "Data-Oriented Error Handling (Fleury Pattern)",
|
||||
"initialized": "2026-06-06",
|
||||
"owner": "tier2-tech-lead",
|
||||
"priority": "high",
|
||||
"status": "active",
|
||||
"type": "refactor + convention + documentation",
|
||||
"scope": {
|
||||
"new_files": [
|
||||
"src/result_types.py",
|
||||
"conductor/code_styleguides/error_handling.md",
|
||||
"tests/test_result_types.py",
|
||||
"tests/test_mcp_client_paths.py",
|
||||
"tests/test_ai_client_result.py",
|
||||
"tests/test_rag_engine_result.py",
|
||||
"tests/test_deprecation_warnings.py"
|
||||
],
|
||||
"modified_files": [
|
||||
"src/mcp_client.py",
|
||||
"src/ai_client.py",
|
||||
"src/rag_engine.py",
|
||||
"conductor/product-guidelines.md",
|
||||
"conductor/workflow.md",
|
||||
"docs/guide_ai_client.md",
|
||||
"docs/guide_mcp_client.md",
|
||||
"pyproject.toml",
|
||||
"tests/conftest.py"
|
||||
]
|
||||
},
|
||||
"blocked_by": ["startup_speedup_20260606", "test_batching_refactor_20260606", "qwen_llama_grok_integration_20260606"],
|
||||
"blocks": ["public_api_migration_20260606"],
|
||||
"estimated_phases": 5,
|
||||
"spec": "spec.md",
|
||||
"plan": "plan.md",
|
||||
"priority_order": "A (foundation patterns + 3-file refactor) > B (deprecation + Result API) > C (convention docs) > D (plan follow-up)",
|
||||
"fleury_patterns_applied": [
|
||||
"Nil struct pointer (Python: frozen dataclass singleton + nil-sentinel methods)",
|
||||
"Zero-initialization (Python: @dataclass field defaults)",
|
||||
"Fail early (Python: same principle; assert + early return)",
|
||||
"AND over OR (Python: Result dataclass with data + side-channel errors list)",
|
||||
"Error info as side-channel (Python: list[ErrorInfo] in Result, accumulates per call)"
|
||||
],
|
||||
"python_mappings": {
|
||||
"nil_struct_pointer": "@dataclass(frozen=True) class Nil: pass; NIL = Nil() (module-level singleton); frozen=True prevents runtime mutation",
|
||||
"zero_initialization": "@dataclass with field defaults; field(default_factory=list) for mutables",
|
||||
"fail_early": "assert + early return at entry points; try/finally as Python's analog to goto defer",
|
||||
"and_over_or": "Result[T] = Result(data: T, errors: list[ErrorInfo]) where data is the happy-path value and errors is a side-channel list (zero-initialized = success)",
|
||||
"error_side_channel": "list[ErrorInfo] in Result struct accumulates all errors per call (richer than C's single errno slot)"
|
||||
},
|
||||
"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, 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=[]"
|
||||
},
|
||||
"refactor_targets": {
|
||||
"src/mcp_client.py": {
|
||||
"pattern_replaced": "(p, err) tuple returns + 'if err or p is None: return err' (~30 sites) + 'assert p is not None' chain (~30+ sites)",
|
||||
"new_pattern": "Result[Path] + Result[str] with nil-sentinel Path; read_file() returns Result[str]",
|
||||
"test_impact": "tests/test_mcp_client.py passes unchanged; new test_mcp_client_paths.py covers the new return types"
|
||||
},
|
||||
"src/ai_client.py": {
|
||||
"pattern_replaced": "ProviderError exception + _classify_*_error() raises + _send_<vendor>() returns str (8 vendors post-qwen_track)",
|
||||
"new_pattern": "ErrorInfo dataclass + _classify_*_error() returns ErrorInfo (value) + _send_<vendor>_result() returns Result[str]; ProviderError removed entirely",
|
||||
"breaking_changes": "All _send_<vendor>() renamed to _send_<vendor>_result() with new return type; send() marked @deprecated; send_result() added",
|
||||
"test_impact": "Most tests call send() and pass unchanged (with deprecation warning); _send_* direct callers (rare) need update"
|
||||
},
|
||||
"src/rag_engine.py": {
|
||||
"pattern_replaced": "RAGEngine methods raise ImportError/ValueError or set self.collection=None on failure",
|
||||
"new_pattern": "RAGEngine methods return Result[None] or Result[T] with side-channel ErrorInfo; NilRAGState sentinel for unconfigured state",
|
||||
"test_impact": "tests/test_rag_engine.py passes unchanged; new test_rag_engine_result.py covers the new return types"
|
||||
}
|
||||
},
|
||||
"deprecation_strategy": {
|
||||
"marked_deprecated": "ai_client.send() (public API returning str)",
|
||||
"new_api": "ai_client.send_result() (returns Result[str, ErrorInfo])",
|
||||
"mechanism": "typing_extensions.deprecated decorator (Python 3.11+ backport of @warnings.deprecated); emits DeprecationWarning at first call per site (cached)",
|
||||
"removal_timeline": "Removed in follow-up track public_api_migration_20260606 (planned in this spec's §12.1)"
|
||||
},
|
||||
"inter_track_coordination": {
|
||||
"post_startup_speedup_state": "src/ai_client.py has lazy SDK imports via _require_warmed; src/app_controller.py has _io_pool; scripts/audit_main_thread_imports.py is a CI gate",
|
||||
"post_test_batching_state": "tests/test_categories.toml populated; conftest.py registers pytest_collection_order plugin; new tests auto-classified by the categorizer",
|
||||
"post_qwen_track_state": "src/vendor_capabilities.py + src/openai_compatible.py + src/qwen_adapter.py exist; 8 _send_<vendor>() functions all return str (Qwen, Llama, Grok, MiniMax, Gemini, Anthropic, DeepSeek, Gemini CLI); MiniMax uses the shared helper; send_openai_compatible raises ProviderError at the SDK boundary",
|
||||
"phase_1_baseline_check": "Verify all 3 pending tracks merged before starting the data-oriented refactor (git log + file existence check)"
|
||||
},
|
||||
"documentation_strategy": {
|
||||
"new_file": "conductor/code_styleguides/error_handling.md (~400 lines; the canonical reference)",
|
||||
"modified_files": [
|
||||
"conductor/product-guidelines.md (new 'Data-Oriented Error Handling' section)",
|
||||
"conductor/workflow.md (note in Code Style section linking to the new styleguide)",
|
||||
"docs/guide_ai_client.md (new section on Result API + deprecation note)",
|
||||
"docs/guide_mcp_client.md (new section on Result return types)"
|
||||
],
|
||||
"rationale": "Establish the convention in the canonical styleguide so future plans can incrementally migrate the remaining src/ files"
|
||||
},
|
||||
"architectural_invariant": "All new code uses Result dataclasses (not Optional/exceptions) for recoverable errors. The Result generic is over the success data T (not over the error type E); errors are always list[ErrorInfo]. Exceptions are reserved for the SDK boundary (where they're caught and converted to ErrorInfo). Nil-sentinel dataclasses are used instead of None for missing data.",
|
||||
"threading_constraint": "Same as existing pattern: Result dataclasses are frozen and thread-safe (immutable). The error list is built via `with_error()` which produces a new Result (no mutation). The deprecation warning uses Python's `warnings.warn` which is thread-safe.",
|
||||
"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(), 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: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; _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; 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, '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 (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 at every phase checkpoint)"
|
||||
],
|
||||
"links": {
|
||||
"backlog_entry": "conductor/tracks.md (to be added)",
|
||||
"code_styleguide": "conductor/code_styleguides/error_handling.md (to be created in Phase 1)",
|
||||
"testing_guide": "docs/guide_testing.md",
|
||||
"ai_client_guide": "docs/guide_ai_client.md",
|
||||
"mcp_client_guide": "docs/guide_mcp_client.md",
|
||||
"workflow_pitfalls": "conductor/workflow.md#known-pitfalls-2026-06-05",
|
||||
"related_tracks": [
|
||||
"conductor/tracks/startup_speedup_20260606/",
|
||||
"conductor/tracks/test_batching_refactor_20260606/",
|
||||
"conductor/tracks/qwen_llama_grok_integration_20260606/",
|
||||
"conductor/tracks/regression_fixes_20260605/",
|
||||
"conductor/tracks/live_gui_test_hardening_v2_20260605/"
|
||||
],
|
||||
"external_docs": [
|
||||
"https://www.dgtlgrove.com/p/the-easiest-way-to-handle-errors (Fleury article)"
|
||||
]
|
||||
}
|
||||
}
|
||||
File diff suppressed because it is too large
Load Diff
@@ -0,0 +1,720 @@
|
||||
# Track: Data-Oriented Error Handling (Fleury Pattern)
|
||||
|
||||
**Status:** Active (spec approved 2026-06-06)
|
||||
**Initialized:** 2026-06-06
|
||||
**Owner:** Tier 2 Tech Lead
|
||||
**Priority:** High (foundational; unlocks incremental migration of the remaining `src/` in future tracks)
|
||||
|
||||
---
|
||||
|
||||
## 1. Overview
|
||||
|
||||
This track introduces a new project convention — **Data-Oriented Error Handling** — based on Ryan Fleury's "The Easiest Way To Handle Errors Is To Not Have Them" framework. The convention is codified in a new `conductor/code_styleguides/error_handling.md` reference, surfaced in `product-guidelines.md` and `workflow.md`, and applied to three high-value subsystems: `src/mcp_client.py`, `src/ai_client.py`, and `src/rag_engine.py` (~150 refactor sites).
|
||||
|
||||
The patterns applied: **Result dataclasses** with side-channel error lists instead of `Optional[T]` / exception-based control flow; **nil-sentinel dataclasses** instead of `None`; **zero-initialized fields** via `@dataclass` defaults; **fail-early** validation pushed to shallow stack frames; **AND-over-OR** return types (data + errors as parallel fields, not a sum type). These collapse the bifurcated codepaths that `if x is None` / `try/except` create, in the spirit of Fleury's argument that "errors are just cases."
|
||||
|
||||
A new **public `Result`-based API** (`ai_client.send_result()`) is introduced for new code; the existing `ai_client.send()` is **marked `@deprecated`** (warning emitted at runtime) so callers can migrate incrementally. The actual removal of the deprecated public API is **deferred to a separate follow-up track** (see §13.1) — this track only marks it deprecated and documents the migration path.
|
||||
|
||||
## 2. Goals (Priority Order)
|
||||
|
||||
| Priority | Goal | Rationale |
|
||||
|---|---|---|
|
||||
| **A (foundational)** | New `conductor/code_styleguides/error_handling.md` documenting the 5 patterns with Python mappings. | Establishes the convention as a first-class project standard. Future plans reference this file; new code follows it; the next comprehensive sweep uses it. |
|
||||
| **A (foundational)** | New `src/result_types.py` with `ErrorInfo` dataclass and `Result[T]` dataclass (generic over data only; errors are `list[ErrorInfo]`). | Provides the canonical building blocks. Re-used across the 3 refactored files and by future migrations. |
|
||||
| **A (primary value)** | `src/mcp_client.py` refactored: the `(p, err)` tuple returns + `if err or p is None: return err` pattern (~30 sites) and the `assert p is not None` chain (~30+ sites) become nil-sentinel `Path` + `Result` returns with side-channel errors. | Clearest, most-contained refactor target. The MCP tool layer is the "boundary" between the AI and the filesystem; errors here should be data, not exceptions, so the model can react. |
|
||||
| **A (primary value)** | `src/ai_client.py` refactored: `ProviderError` exception becomes `ErrorInfo` dataclass; internal `_send_<vendor>()` functions return `Result[str, ErrorInfo]`; SDK-exception catches become conversions to `ErrorInfo` (caught at the boundary, not propagated). | The provider layer is the highest-stakes refactor. Catches SDK exceptions at the boundary, converts to data, and lets the rest of the code work with a flat control flow. |
|
||||
| **A (primary value)** | `src/rag_engine.py` refactored: `RAGEngine._init_vector_store`, `_validate_collection_dim`, `is_empty`, `add_documents` return `Result` with side-channel errors instead of raising `ImportError` / `ValueError`. | The RAG engine has its own ad-hoc error class hierarchy that mirrors the patterns Fleury criticizes. Bringing it into the convention aligns it with the new vendor layer. |
|
||||
| **B (architectural)** | Existing public `ai_client.send()` is marked `@deprecated` with a runtime warning directing callers to `ai_client.send_result()`. | The public API is preserved (no breaking change) but signals the migration intent. The deprecation message includes a TODO reference to the follow-up track. |
|
||||
| **B (architectural)** | New public `ai_client.send_result()` returns `Result[str, ErrorInfo]`. The new vendor layer (Qwen/Llama/Grok from the prior track) calls `_send_<vendor>_result()` internally and `send_result()` is the public entry point. | New code uses the new API. Old code keeps working via the deprecated `send()`. |
|
||||
| **C (documentation)** | `conductor/product-guidelines.md` gets a new "Data-Oriented Error Handling" section summarizing the principles (referencing the code styleguide for details). | The convention is visible in the project-level guidance. |
|
||||
| **C (documentation)** | `conductor/workflow.md` gets a note in the Code Style section linking to the new styleguide. | The convention is visible in the workflow so all future plans reference it. |
|
||||
| **C (documentation)** | `docs/guide_*.md` updates: `guide_mcp_client.md` and `guide_ai_client.md` show the new patterns; the next refactor of `guide_rag.md` (or its creation if missing) does the same. | Guides stay in sync with the implementation. |
|
||||
| **D (forward-looking)** | A new follow-up track "Public API Result Migration" is **planned in this spec's §13.1** (not executed) so it's clear what work remains. | Future plans have a known destination. |
|
||||
|
||||
### 2.1 Non-Goals (this track)
|
||||
|
||||
- **Not** migrating the remaining `src/` files (`app_controller.py`, `models.py`, `project_manager.py`, `commands.py`, etc.). These are explicitly out of scope; the convention is established so future tracks can migrate them one at a time.
|
||||
- **Not** removing the public `ai_client.send()`. Only `@deprecated` markers are added. Removal is in a follow-up track.
|
||||
- **Not** changing the `multi_agent_conductor.py` MMA worker interface or the `app_controller.py` orchestrator interface. They continue to call the public `send()` (which still works) and migrate later.
|
||||
- **Not** introducing a generic `Result[T, E]` (with `E` as the error type). The Result is generic only over the success data; errors are always `list[ErrorInfo]`. Rationale: per Fleury, errors are a side-channel — they should accumulate, not be a single tagged value. This also avoids Python's `Union[T, E]` complexity.
|
||||
- **Not** introducing async-aware error propagation. Async / asyncio patterns are out of scope; the refactored code stays synchronous.
|
||||
- **Not** changing how `logging` works. Errors flow as data in `Result`; logging is the caller's choice (most callers will log via the existing comms_log_callback).
|
||||
|
||||
## 3. Architecture
|
||||
|
||||
### 3.1 The 5 Patterns + Python Mappings
|
||||
|
||||
| # | Fleury pattern | Python mapping | Code location |
|
||||
|---|---|---|---|
|
||||
| 1 | **Nil struct pointer** (read-only sentinel) | `@dataclass(frozen=True) class Nil: pass`; module-level `NIL = Nil()` singleton. Frozen prevents runtime mutation; convention prevents writes. | `src/result_types.py:NilPath`, `NilRAGState`, etc. |
|
||||
| 2 | **Zero-initialization** | `@dataclass` with field defaults. `field(default_factory=list)` for mutables. | Used throughout `Result` and the refactored files. |
|
||||
| 3 | **Fail early** | Same principle: validation at the entry point; assert or early return. No `goto defer`, but `try/finally` is similar. | Applied to MCP `_resolve_and_check`, RAG `_init_*`, provider `_ensure_*_client`. |
|
||||
| 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
|
||||
|
||||
```
|
||||
conductor/
|
||||
code_styleguides/
|
||||
error_handling.md # NEW: the canonical reference (5 patterns, Python mappings, examples)
|
||||
product-guidelines.md # MODIFIED: new "Data-Oriented Error Handling" section
|
||||
workflow.md # MODIFIED: note in Code Style section referencing the new styleguide
|
||||
tracks.md # MODIFIED: register this track; add the public_api_migration_20260606 placeholder
|
||||
|
||||
docs/
|
||||
guide_mcp_client.md # MODIFIED: new patterns (if doc exists; otherwise created in follow-up)
|
||||
guide_ai_client.md # MODIFIED: new patterns, deprecation note, Result API
|
||||
guide_rag.md # MODIFIED: new patterns (if doc exists)
|
||||
|
||||
src/
|
||||
result_types.py # NEW: ErrorInfo, Result[T], NilPath, NilRAGState
|
||||
mcp_client.py # MODIFIED: ~60 sites refactored
|
||||
ai_client.py # MODIFIED: ProviderError → ErrorInfo; _send_* returns Result; send() deprecated; send_result() added
|
||||
rag_engine.py # MODIFIED: ~20 sites refactored
|
||||
|
||||
tests/
|
||||
test_result_types.py # NEW: Result + ErrorInfo + nil-sentinel tests
|
||||
test_mcp_client_paths.py # NEW: verify MCP path resolution returns Result
|
||||
test_ai_client_result.py # NEW: verify _send_* return Result, send_result() public API, deprecation warning
|
||||
test_rag_engine_result.py # NEW: verify RAG methods return Result
|
||||
test_deprecation_warnings.py # NEW: verify send() emits DeprecationWarning
|
||||
```
|
||||
|
||||
### 3.3 The `Result[T]` and `ErrorInfo` Data Model
|
||||
|
||||
```python
|
||||
from dataclasses import dataclass, field
|
||||
from typing import Generic, TypeVar
|
||||
from enum import Enum
|
||||
|
||||
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"
|
||||
NOT_READY = "not_ready"
|
||||
UNKNOWN = "unknown"
|
||||
CONFIG = "config"
|
||||
INTERNAL = "internal"
|
||||
# Added 2026-06-08 per nagent_review Pitfall #4 (provider history divergence).
|
||||
# The Application edits the entry's content (e.g., user fixes a typo in an AI
|
||||
# response, or branches at a midpoint via guide_discussions.md §"Per-Entry
|
||||
# Operations" A1+A4) but the ai_client._<provider>_history (the bytes
|
||||
# actually replayed to the LLM) still contains the original. This is
|
||||
# silent corruption, not a thrown error. The PROVIDER_HISTORY_DIVERGED_FROM_UI
|
||||
# kind makes the divergence *detectable* and *reportable* so the follow-up
|
||||
# public_api_migration_20260606 track can collapse the two history layers
|
||||
# (see §12.1).
|
||||
PROVIDER_HISTORY_DIVERGED_FROM_UI = "provider_history_diverged_from_ui"
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class ErrorInfo:
|
||||
kind: ErrorKind
|
||||
message: str
|
||||
source: str = "" # which subsystem produced it (e.g. "mcp.read_file", "ai_client.gemini")
|
||||
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_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))
|
||||
```
|
||||
|
||||
**Design notes:**
|
||||
- `Result` is generic over `T` (the success data type) but **not** over `E` (the error type). Per Fleury: errors are a side-channel list, not a tagged sum. This also avoids `Union[T, E]` complexity.
|
||||
- `data: T` is the happy-path result. The success case is `Result(data=X, errors=[])`. The failure case is `Result(data=zero_value, errors=[err1, err2])`.
|
||||
- `errors` is a `list[ErrorInfo]`, not a single error, so partial failures can be reported (e.g., "5 of 10 files failed; here are the 5 errors").
|
||||
- `Result` is `frozen=True` (no mutation); use `with_error` / `with_data` to produce modified copies.
|
||||
- `NilPath` is a `@dataclass(frozen=True)` singleton: `NIL_PATH = NilPath()`. Same for `NilRAGState` etc.
|
||||
|
||||
### 3.4 Nil-Sentinel Pattern
|
||||
|
||||
The nil sentinel is a `@dataclass(frozen=True)` with all-default values. Module-level singleton. Used when a function "would return None" in the old code; in the new code, it returns the nil sentinel of the right type.
|
||||
|
||||
```python
|
||||
@dataclass(frozen=True)
|
||||
class NilPath:
|
||||
exists: bool = False
|
||||
read_text: str = ""
|
||||
errors: list[ErrorInfo] = field(default_factory=list)
|
||||
|
||||
NIL_PATH = NilPath()
|
||||
```
|
||||
|
||||
`NIL_PATH` is the "empty Path" — it has all default values, can be safely read from (the `read_text` is `""`, no file I/O), and `errors` accumulates any deferred errors. Callers that need a real `pathlib.Path` for filesystem operations can check `if isinstance(result.data, NilPath): handle()` — but most callers just need the read text, and `NIL_PATH.read_text == ""` is fine for the AI model's purposes.
|
||||
|
||||
For the MCP client, the `(p, err)` tuple returns are replaced with `Result[Path]`:
|
||||
- Old: `def _resolve_and_check(path: str) -> tuple[Path | None, str]`
|
||||
- New: `def _resolve_and_check(path: str) -> Result[Path]` where `Path` is the real `pathlib.Path` on success or `NilPath()` on failure (the `data` field can be a `Path` or `NilPath`; the consumer checks `result.data.__class__` or relies on the duck-typed `read_text` field)
|
||||
|
||||
This is the same idea as Fleury's nil struct pointer: callers don't need to `if p is None:` check; they can call `p.read_text` and get `""` on the nil path.
|
||||
|
||||
### 3.5 Deprecation Strategy for the Public `send()` API
|
||||
|
||||
The public `ai_client.send()` is preserved (existing callers don't break) but marked deprecated:
|
||||
|
||||
```python
|
||||
import warnings
|
||||
from typing_extensions import deprecated
|
||||
|
||||
@deprecated("Use ai_client.send_result() instead. Will be removed in the public_api_migration_20260606 track. See conductor/tracks/data_oriented_error_handling_20260606/spec.md for the migration path.")
|
||||
def send(...) -> str:
|
||||
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 §13.1.",
|
||||
DeprecationWarning,
|
||||
stacklevel=2,
|
||||
)
|
||||
return _extract_text(_send_*_result(...))
|
||||
```
|
||||
|
||||
`@deprecated` is the `typing_extensions` backport (works on Python 3.11+; this project requires 3.11+). The decorator:
|
||||
- Emits a `DeprecationWarning` at the first call (cached after that to avoid log spam).
|
||||
- Updates type hints in IDEs and type checkers (mypy, pyright) to show the deprecation.
|
||||
- The `@deprecated` call is a no-op for the runtime; only the warning + type-checker effect.
|
||||
|
||||
The new public API:
|
||||
|
||||
```python
|
||||
def send_result(...) -> Result[str]:
|
||||
"""The Result-based public API. Returns Result[str, ErrorInfo] with text in .data and errors in .errors."""
|
||||
# Acquire _send_lock, route to provider, return Result
|
||||
...
|
||||
```
|
||||
|
||||
The `send_result()` function does the same routing as `send()` but returns `Result` instead of unwrapping it. The internal `_send_<vendor>_result()` functions are called from `send_result()`. The deprecated `send()` is a thin wrapper:
|
||||
|
||||
```python
|
||||
@deprecated(...)
|
||||
def send(...) -> str:
|
||||
result = send_result(...)
|
||||
if not result.ok:
|
||||
_append_comms("WARN", "deprecated_send_with_errors", [e.ui_message() for e in result.errors])
|
||||
return result.data
|
||||
return result.data
|
||||
```
|
||||
|
||||
This way, the deprecated `send()` keeps working (returning the text even if there were errors, matching today's behavior), and the comms log gets a warning entry so users can see that the old API is being used with errors.
|
||||
|
||||
## 4. Per-File Refactor Designs
|
||||
|
||||
### 4.1 `src/mcp_client.py`
|
||||
|
||||
**Current pattern (the "sum type as tuple"):**
|
||||
```python
|
||||
def _resolve_and_check(path: str) -> tuple[Path | None, str]:
|
||||
p, err = _resolve_path(path)
|
||||
if err: return None, err
|
||||
if not _is_in_allowed_base(p): return None, "ERROR: ..."
|
||||
if p.exists() and not p.is_file(): return None, "ERROR: ..."
|
||||
return p, ""
|
||||
|
||||
def read_file(path: str) -> str:
|
||||
p, err = _resolve_and_check(path)
|
||||
if err or p is None:
|
||||
return err
|
||||
if not p.exists(): return f"ERROR: file not found: {path}"
|
||||
...
|
||||
```
|
||||
|
||||
**Refactored pattern (Result + nil sentinel):**
|
||||
```python
|
||||
def _resolve_and_check(path: str) -> Result[Path]:
|
||||
"""Returns Result[Path]. On success, .data is a pathlib.Path. On failure, .data is NilPath() and .errors is populated."""
|
||||
try:
|
||||
p = _resolve_path(path)
|
||||
except _ResolutionError as e:
|
||||
return Result(data=NilPath(), errors=[ErrorInfo(kind=ErrorKind.INVALID_INPUT, message=str(e), source="mcp._resolve_and_check")])
|
||||
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)
|
||||
|
||||
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="", 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")])
|
||||
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)])
|
||||
```
|
||||
|
||||
**Key changes:**
|
||||
- `_resolve_and_check` returns `Result[Path]` (or `Result[Path | NilPath]` for type clarity). The MCP layer never returns `None` or raises for the resolution step.
|
||||
- `read_file` and the other tool functions return `Result[str]`. The caller (`mcp_client.async_dispatch` or the tool-dispatch internals) extracts the text or formats the error.
|
||||
- The 30+ `assert p is not None` checks (lines 304-794) become "trust the Result and use `p.read_text`" — the Path is never None in the Result; it's either a real Path or `NilPath` (with a `read_text` field that's `""`).
|
||||
- Internal exceptions (`OSError`, `PermissionError`, etc.) are caught at the boundary and converted to `ErrorInfo` — they don't propagate as Python exceptions.
|
||||
|
||||
### 4.2 `src/ai_client.py`
|
||||
|
||||
**Current pattern (the `ProviderError` exception):**
|
||||
```python
|
||||
class ProviderError(Exception):
|
||||
kind: str
|
||||
provider: str
|
||||
original: Exception
|
||||
def ui_message(self) -> str: ...
|
||||
|
||||
def _send_gemini(...) -> str:
|
||||
try:
|
||||
resp = genai_client.models.generate_content(...)
|
||||
...
|
||||
except Exception as exc:
|
||||
raise _classify_gemini_error(exc) from exc
|
||||
```
|
||||
|
||||
**Refactored pattern (ErrorInfo + Result):**
|
||||
```python
|
||||
def _classify_gemini_error(exc: Exception, source: str) -> ErrorInfo:
|
||||
if isinstance(exc, genai_types.RateLimitError):
|
||||
return ErrorInfo(kind=ErrorKind.RATE_LIMIT, message=str(exc), source=source, original=exc)
|
||||
if isinstance(exc, genai_types.PermissionDeniedError):
|
||||
return ErrorInfo(kind=ErrorKind.AUTH, message=str(exc), source=source, original=exc)
|
||||
...
|
||||
return ErrorInfo(kind=ErrorKind.UNKNOWN, message=str(exc), source=source, original=exc)
|
||||
|
||||
def _send_gemini_result(...) -> Result[str]:
|
||||
try:
|
||||
resp = genai_client.models.generate_content(...)
|
||||
...
|
||||
return Result(data=text)
|
||||
except Exception as exc:
|
||||
return Result(data="", errors=[_classify_gemini_error(exc, source="ai_client.gemini")])
|
||||
```
|
||||
|
||||
**Key changes:**
|
||||
- `ProviderError` exception class becomes `ErrorInfo` dataclass (a value, not a control-flow primitive).
|
||||
- `_classify_<vendor>_error()` functions return `ErrorInfo` instead of raising `ProviderError`.
|
||||
- `_send_<vendor>()` becomes `_send_<vendor>_result()` returning `Result[str]`. SDK exceptions are caught at the boundary and converted to `ErrorInfo` (caught at the boundary, not propagated).
|
||||
- The public `send()` is preserved (marked `@deprecated`) for backward compat; it calls `send_result()` and unwraps.
|
||||
- The new public `send_result()` returns `Result[str]`.
|
||||
|
||||
**Migration note (for the follow-up track):**
|
||||
- The MMA worker interface in `multi_agent_conductor.py` calls `ai_client.send()`. Migration: call `ai_client.send_result()` and check `.ok` and `.errors`.
|
||||
- The orchestrator in `app_controller.py` calls `ai_client.send()`. Migration: same.
|
||||
- ~50+ test files call `ai_client.send()` or directly call `_send_<vendor>()`. Migration: most tests use the public `send()`; only `_send_*()` direct tests need to update.
|
||||
|
||||
### 4.3 `src/rag_engine.py`
|
||||
|
||||
**Current pattern (raises + ad-hoc error strings):**
|
||||
```python
|
||||
def _init_vector_store(self):
|
||||
vs_config = self.config.vector_store
|
||||
if vs_config.provider == 'chroma':
|
||||
db_path = os.path.abspath(...)
|
||||
os.makedirs(db_path, exist_ok=True)
|
||||
chroma_module = _get_chromadb()
|
||||
if chroma_module is None:
|
||||
raise ImportError("chromadb is not installed")
|
||||
chromadb, Settings = chroma_module
|
||||
self.client = chromadb.PersistentClient(path=db_path)
|
||||
self.collection = self.client.get_or_create_collection(...)
|
||||
self._validate_collection_dim()
|
||||
elif vs_config.provider == 'mock':
|
||||
self.client = "mock"
|
||||
self.collection = "mock"
|
||||
else:
|
||||
raise ValueError(f"Unknown vector store provider: {vs_config.provider}")
|
||||
```
|
||||
|
||||
**Refactored pattern (Result + nil sentinel):**
|
||||
```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.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(...)
|
||||
return _validate_collection_dim_result() # cascades the 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")])
|
||||
|
||||
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"])
|
||||
...
|
||||
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)])
|
||||
return Result(data=None)
|
||||
```
|
||||
|
||||
**Key changes:**
|
||||
- `_init_vector_store` becomes `_init_vector_store_result` returning `Result[None]`. `ImportError` and `ValueError` raises become `ErrorInfo` entries in the result.
|
||||
- `_validate_collection_dim` becomes `_validate_collection_dim_result`. The catch-all `except Exception` becomes a `Result` with a single `ErrorInfo` (or success if the catch was a no-op).
|
||||
- The `RAGEngine.is_empty`, `add_documents`, and other public methods return `Result` (or stay as their current return type if no error path exists).
|
||||
- The `RAGEngine.__init__` itself stays as-is (it's a constructor; it sets `self.collection = NIL_COLLECTION` if init fails, deferring the error to the first operation).
|
||||
|
||||
**Nil sentinel for RAG:**
|
||||
```python
|
||||
@dataclass(frozen=True)
|
||||
class NilRAGState:
|
||||
enabled: bool = False
|
||||
is_empty_result: bool = True
|
||||
errors: list[ErrorInfo] = field(default_factory=list)
|
||||
|
||||
NIL_RAG_STATE = NilRAGState()
|
||||
```
|
||||
|
||||
Used when the RAG engine is in a "not configured" / "failed to init" state. Methods that would have raised now return `Result` with `data=NIL_RAG_STATE` and the error in `.errors`.
|
||||
|
||||
### 4.4 Convention Documentation
|
||||
|
||||
**`conductor/code_styleguides/error_handling.md`** (NEW, ~400 lines):
|
||||
|
||||
The canonical reference. Sections:
|
||||
1. The 5 patterns (with Python code examples for each)
|
||||
2. Decision tree: when to use Result vs Exception vs Optional
|
||||
3. Naming conventions (`*_result` for Result-returning functions; `_result` suffix on dataclasses)
|
||||
4. Error classification (the `ErrorKind` enum and when to use which)
|
||||
5. Migration playbook (how to convert an `Optional[T]` return to `Result[T]`)
|
||||
6. Anti-patterns (don't do these things)
|
||||
7. Examples (the 3 refactored subsystems as worked examples)
|
||||
|
||||
**`conductor/product-guidelines.md`** (MODIFIED, +1 section):
|
||||
|
||||
New top-level section "Data-Oriented Error Handling":
|
||||
|
||||
```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.
|
||||
```
|
||||
|
||||
**`conductor/workflow.md`** (MODIFIED, +1 line in the Code Style section):
|
||||
|
||||
```markdown
|
||||
- For error handling, see [Data-Oriented Error Handling](./code_styleguides/error_handling.md).
|
||||
```
|
||||
|
||||
**`docs/guide_ai_client.md`** (MODIFIED, +1 section):
|
||||
|
||||
```markdown
|
||||
## Data-Oriented Error Handling (Fleury Pattern)
|
||||
|
||||
The provider layer uses `Result[str, ErrorInfo]` (returned by `_send_<vendor>_result()`)
|
||||
instead of raising `ProviderError`. SDK exceptions are caught at the boundary
|
||||
(see `send_openai_compatible` in `src/openai_compatible.py` and the DashScope
|
||||
adapter in `src/qwen_adapter.py`) and converted to `ErrorInfo` entries in the
|
||||
Result. The public `ai_client.send()` is deprecated; new code should use
|
||||
`ai_client.send_result()`. See `conductor/code_styleguides/error_handling.md`
|
||||
for the convention.
|
||||
```
|
||||
|
||||
## 5. Configuration / Dependencies
|
||||
|
||||
### 5.1 New dependency: `typing_extensions`
|
||||
|
||||
For the `@deprecated` decorator (Python 3.11+ has `@warnings.deprecated` but it's Python 3.13+; `typing_extensions` backports it).
|
||||
|
||||
```toml
|
||||
[project]
|
||||
dependencies = [
|
||||
...
|
||||
"typing_extensions>=4.5.0", # NEW
|
||||
]
|
||||
```
|
||||
|
||||
### 5.2 No new environment variables
|
||||
|
||||
All existing configs (`config.toml`, `credentials.toml`, per-project TOML) work unchanged.
|
||||
|
||||
## 6. Testing Strategy
|
||||
|
||||
| Test File | Purpose | Coverage Target |
|
||||
|---|---|---|
|
||||
| `tests/test_result_types.py` | `Result`, `ErrorInfo`, nil-sentinel singletons. | 100% |
|
||||
| `tests/test_mcp_client_paths.py` | Verify `_resolve_and_check` returns `Result` (not tuple); verify `read_file` returns `Result[str]`. | 90% (covers the new code paths; existing tests still pass) |
|
||||
| `tests/test_ai_client_result.py` | Verify `_send_<vendor>_result()` returns `Result`; verify `send_result()` is the new public API; verify `send()` emits `DeprecationWarning`. **State-delegation regression tests (added 2026-06-08 per `docs/guide_state_lifecycle.md` and the 2026-06-08 docs refresh):** verify that `app.temperature = 0.5` round-trips through the `App.__getattr__`/`__setattr__` delegation (per `gui_2.py:666-675`) and is visible in the next `send_result()` call; verify that `controller.disc_entries[i].content = "..."` is reflected in the next `send_result()`'s `messages` parameter (this is the regression vector for nagent_review Pitfall #4, the provider-history divergence); verify that the **6** per-provider history locks (`_anthropic_history_lock:128`, `_deepseek_history_lock:132`, `_minimax_history_lock:136`, `_qwen_history_lock:140`, `_grok_history_lock:145`, `_llama_history_lock:149` per `ai_client.py`) serialize correctly under concurrent `send_result()` calls from different threads. These tests are *mandatory* for Phase 3 (the ai_client refactor) because the `App.__getattr__`/`__setattr__` delegation means a partial refactor would manifest as silent `AttributeError`s deep in the test, not at the refactor commit boundary. | 90% |
|
||||
| `tests/test_rag_engine_result.py` | Verify RAG methods return `Result`; verify `NilRAGState` is used. | 80% |
|
||||
| `tests/test_deprecation_warnings.py` | Verify `ai_client.send()` emits exactly one `DeprecationWarning` per call site (cached after first). | 100% |
|
||||
| `tests/test_mcp_client.py` (existing) | Verify no regressions; existing tests pass unchanged. | 100% (regression) |
|
||||
| `tests/test_ai_client.py` (existing) | Verify no regressions; existing tests pass unchanged. | 100% (regression) |
|
||||
| `tests/test_rag_engine.py` (existing) | Verify no regressions; existing tests pass unchanged. | 100% (regression) |
|
||||
|
||||
**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
|
||||
|
||||
| Phase | What | Risk |
|
||||
|---|---|---|
|
||||
| **Phase 1 — Foundation: patterns module + style guide** | Add `src/result_types.py`. Add `conductor/code_styleguides/error_handling.md`. Update `product-guidelines.md` and `workflow.md`. Add `typing_extensions` dep. | None. New files, no modifications. |
|
||||
| **Phase 2 — `mcp_client.py` refactor** | Refactor `_resolve_and_check` + the 9 tool functions. The 30+ `assert p is not None` become nil-sentinel usage. The `(p, err)` tuples become `Result`. | Medium. ~60 sites. Mitigated by existing `tests/test_mcp_client.py` coverage. |
|
||||
| **Phase 3 — `ai_client.py` refactor** | Refactor `_classify_*_error()` → return `ErrorInfo`. Refactor `_send_*` → `_send_*_result()` returning `Result`. Add `send_result()` public API. Mark `send()` `@deprecated`. | High. The provider layer is the most complex refactor. Mitigated by existing `tests/test_minimax_provider.py`, `tests/test_qwen_provider.py`, etc. |
|
||||
| **Phase 4 — `rag_engine.py` refactor** | Refactor RAG methods to return `Result`. Add `NilRAGState` sentinel. | Medium. ~20 sites. Mitigated by existing `tests/test_rag_engine.py`. |
|
||||
| **Phase 5 — Deprecation + docs + integration** | Wire deprecation warning. Update `docs/guide_ai_client.md` and `docs/guide_mcp_client.md`. Add the public_api_migration_20260606 placeholder to `conductor/tracks.md`. Manual smoke test. | Low. |
|
||||
|
||||
Each phase has its own checkpoint commit and git note.
|
||||
|
||||
## 8. Risks & Mitigations
|
||||
|
||||
| Risk | Likelihood | Impact | Mitigation |
|
||||
|---|---|---|---|
|
||||
| `ProviderError` is currently raised from `_classify_*_error()`. The refactor changes these to return `ErrorInfo` instead. Any external caller that catches `ProviderError` will break. | Low | Medium | Search the codebase: `rg "except ProviderError"`. Per the grep above (line 1451 of `ai_client.py`), `ProviderError` is only caught in `ai_client.send()` (defined at `ai_client.py:2690`). After the refactor, that catch becomes a `result.errors` check. No external code catches `ProviderError` directly. The 4 in-file classifier functions (`_classify_anthropic_error:361`, `_classify_gemini_error:380`, `_classify_deepseek_error:396`, `_classify_minimax_error:420`) plus 1 shared `_classify_openai_compatible_error` in `src/openai_compatible.py:39` plus `classify_dashscope_error` in `src/qwen_adapter.py:26` are the 6 conversion sites — `_classify_gemini_cli_error` does not exist (Gemini CLI uses `GeminiCliAdapter` subprocess path with internal error handling). |
|
||||
| The 30+ `assert p is not None` in `mcp_client.py` are existing invariants that catch real bugs. If the refactor turns them into nil-sentinel paths, a real bug could manifest as a silent empty result. | Medium | High | The refactored code keeps the assertions as `assert resolved.ok` or `assert not isinstance(resolved.data, NilPath)` where the invariants matter. The `Result.errors` list captures the failure for the caller. |
|
||||
| Adding `@deprecated` to `send()` produces a lot of `DeprecationWarning` log spam in the test suite. | High | Low | The deprecation message is cached per call site (using `warnings.warn(..., stacklevel=2)` with a `DeprecationWarning` filter that doesn't propagate to the test failure). Tests can opt in to the warning check via `pytest.warns(DeprecationWarning)`. |
|
||||
| `result_types.py` introduces a circular import risk (if `models.py` or other core modules want to use `ErrorKind` early). | Low | Low | `result_types.py` is a leaf module with no imports from other src files except stdlib. |
|
||||
| The MCP dispatch internals (which call `read_file`, `list_directory`, etc.) currently expect a `str` return. The refactor returns `Result[str]`. | Medium | Medium | The dispatch layer is updated in Phase 2 alongside the tool functions. The dispatch unwraps `Result.data` and logs `Result.errors` via the comms log. The dispatch's public API (the `async_dispatch` function) still returns `str` to the AI model. |
|
||||
| The `RAGEngine.__init__` constructor currently raises if config is invalid. The refactor wants to defer errors to first use. | Medium | Low | Constructor still raises for "config missing" (fail early at init). "Config invalid" (e.g., bad embedding provider) defers to `_init_vector_store_result` (called explicitly or lazily). |
|
||||
|
||||
## 9. Open Questions
|
||||
|
||||
1. **The Result type generic syntax:** Python 3.11+ supports `Generic[T]` cleanly. The spec uses `Result[T]`. Should we also provide a non-generic `Result` for cases where the data is always `None` (e.g., `Result[None]` for operations that succeed/fail without data)? (Proposal: yes; provide `Ok = Result(data=None, errors=[])` as a constant for the trivial success case.)
|
||||
2. **Logging of errors:** When `_send_<vendor>_result()` returns a `Result` with errors, should the errors be auto-logged via `_append_comms`, or should the caller decide? (Proposal: auto-log errors as `WARN` entries in the comms log; this matches today's behavior where `ProviderError` was logged.)
|
||||
3. **Backwards-compat shim for the old `(p, err)` returns:** Some internal callers might still be unpacking `(p, err)`. Should the refactor break them or provide a shim? (Proposal: break them. The grep above shows the pattern is contained; the breakage is in tool functions, not in the public MCP API.)
|
||||
4. **Should the `Result` type be in a more general location?** E.g., `src/result_types.py` is fine for v1; if the patterns spread to other tracks, it could move to `src/result.py` or `src/datatypes/result.py`. (Proposal: keep `src/result_types.py` for v1; revisit if it becomes a multi-track import.)
|
||||
|
||||
## 10. Coordination with Pending Tracks (post-state baseline)
|
||||
|
||||
This track executes **after** three pending tracks have landed (or are far enough along that the codebase reflects their state). The spec assumes the following baseline when this track begins. Any drift from this baseline is a coordination issue that the implementer must resolve before Phase 1.
|
||||
|
||||
### 10.1 Post-`startup_speedup_20260606` State
|
||||
|
||||
- **`src/startup_profiler.py`** exists (new module with `StartupProfiler` context manager).
|
||||
- **`src/app_controller.py`** has `AppController._io_pool: ThreadPoolExecutor` (4 workers, prefix `controller-io-N`) for background work.
|
||||
- **`src/app_controller.py`** has a warmup mechanism: `_warmup_status`, `_warmup_done_event`, `on_warmup_complete`, `wait_for_warmup`.
|
||||
- **`src/ai_client.py`** has `import` statements restructured: heavy SDKs (`google.genai`, `anthropic`, `openai`, `fastapi`) are accessed via `_require_warmed(name)` at use sites, NOT top-level imports. `import src.ai_client` is < 50ms.
|
||||
- **`src/api_hooks.py`** has FastAPI imports deferred similarly. `import src.api_hooks` is < 100ms.
|
||||
- **`src/commands.py`, `src/command_palette.py`, `src/theme_2.py`, `src/theme_nerv.py`, `src/theme_nerv_fx.py`, `src/markdown_helper.py`** all have heavy imports moved to use-sites.
|
||||
- **No new `threading.Thread(...)` calls** anywhere in `src/` (per the track's invariant).
|
||||
- **Top-level `Optional[X]` in `src/ai_client.py`** is reduced (SDK clients now accessed via `_require_warmed`). But the function signatures still use `Optional[X]` for callbacks and config (e.g., `pre_tool_callback: Optional[Callable]`).
|
||||
- **`scripts/audit_main_thread_imports.py`** is a CI gate that fails if heavy imports appear at the top of main-thread-reachable files.
|
||||
|
||||
**Impact on this track:**
|
||||
- The new `src/result_types.py` is a leaf module with only stdlib imports. Safe to import at top of any file. **Verify** with the audit script in Phase 1.
|
||||
- The new `_send_<vendor>_result()` functions may need to be careful about the warmup mechanism: if the SDK isn't warmed, `_require_warmed(name)` is called inside `_ensure_<vendor>_client()`, which is itself called from `_send_<vendor>_result()`. The Result pattern's "fail at boundary, convert to ErrorInfo" applies: if `_require_warmed` raises, catch and convert.
|
||||
|
||||
### 10.2 Post-`test_batching_refactor_20260606` State
|
||||
|
||||
- **`scripts/run_tests_batched.py`** is the new categorized batcher with `--plan` and `--audit` modes.
|
||||
- **`scripts/test_categorizer.py`** + **`scripts/test_batcher.py`** + **`scripts/pytest_collection_order.py`** exist.
|
||||
- **`tests/test_categories.toml`** is populated with ~30 cross-cutting entries.
|
||||
- **`tests/conftest.py`** registers the `pytest_collection_order` plugin.
|
||||
- **All new tests** in this track will be auto-classified by the categorizer. Pure unit tests go to Tier 1; `live_gui` tests (if any) go to Tier 3. Most new tests for this track are Tier 1 (unit).
|
||||
|
||||
**Impact on this track:**
|
||||
- New test files (`test_result_types.py`, `test_mcp_client_paths.py`, `test_ai_client_result.py`, `test_rag_engine_result.py`, `test_deprecation_warnings.py`) should follow the standard naming convention. The categorizer will classify them automatically.
|
||||
- If any of these tests need `mock_app` or `app_instance` fixtures, they're Tier 2. If any need `live_gui`, they're Tier 3.
|
||||
- The `test_batching_refactor` track's registry may want a `test_ai_client_result.py` entry to ensure it goes to the right batch_group (likely `core` or `mma`).
|
||||
|
||||
### 10.3 Post-`qwen_llama_grok_integration_20260606` State (most impactful)
|
||||
|
||||
This is the track that most affects the data-oriented error handling refactor. The state:
|
||||
|
||||
#### 10.3.1 New modules in `src/`
|
||||
|
||||
- **`src/vendor_capabilities.py`**: `VendorCapabilities` dataclass, `_REGISTRY` populated for Qwen/Llama/Grok/MiniMax + Anthropic/Gemini/DeepSeek stubs, `get_capabilities(vendor, model)`, `list_models_for_vendor(vendor)`.
|
||||
- **`src/openai_compatible.py`**: `NormalizedResponse`, `OpenAICompatibleRequest`, `send_openai_compatible(client, request, capabilities)` that **raises** `ProviderError` via `_classify_openai_compatible_error()` on SDK errors.
|
||||
- **`src/qwen_adapter.py`**: `build_dashscope_tools()`, `classify_dashscope_error()` that **raises** `ProviderError`.
|
||||
|
||||
#### 10.3.2 Modified `src/ai_client.py`
|
||||
|
||||
- **All 5 providers** (`_send_gemini`, `_send_anthropic`, `_send_deepseek`, `_send_minimax`, `_send_gemini_cli`) plus 3 new vendors (`_send_qwen`, `_send_llama`, `_send_grok`) plus the Ollama native adapter (`_send_llama_native`, added by the `qwen_llama_grok_followup_20260611` track for `localhost` / `127.0.0.1` base URLs) all exist. **9 `_send_*()` functions total.** All return `str` (text content of the AI response).
|
||||
- **Per-vendor state**: state globals for all 5+3+1 providers; per-vendor history lists + **6 per-vendor history locks** (`_anthropic_history_lock`, `_deepseek_history_lock`, `_minimax_history_lock`, `_qwen_history_lock`, `_grok_history_lock`, `_llama_history_lock`); per-vendor client singletons.
|
||||
- **Per-vendor `list_models()`** dispatch exists.
|
||||
- **Shared `run_with_tool_loop` helper** (added 2026-06-11 by `qwen_llama_grok_followup_20260611`, `ai_client.py:806`): 4 of 9 vendors already use it — `_send_minimax` (refactored to helper in Phase 4 of the parent track, 250 → 50 lines), `_send_grok`, `_send_llama`, and `_send_gemini_cli` (via the `send_func + on_pre_dispatch` extension). The remaining 5 vendors (`_send_anthropic`, `_send_gemini`, `_send_deepseek`, `_send_qwen`, `_send_llama_native`) still have bespoke inline tool-call loops. **Invariant preserved by the audit gate** `scripts/audit_no_inline_tool_loops.py` (`DEFERRED_VENDORS = {"anthropic", "gemini", "deepseek"}`): after this track, the 4 refactored vendors must still use `run_with_tool_loop` (and the 3 deferred vendors remain in the exclusion list). `_send_qwen` and `_send_llama_native` are NOT in the deferred list, so any inline loop in them is already a CI violation.
|
||||
- **MiniMax is already refactored** to use `send_openai_compatible()` and `run_with_tool_loop` (the data-oriented refactor in the parent track reduced `_send_minimax` from ~250 lines to ~50).
|
||||
- **Anthropic and DeepSeek** still have their bespoke `_send_*()` implementations.
|
||||
- **Gemini** still has its SDK-specific caching logic (4-breakpoint system, explicit `genai.CachedContent`).
|
||||
- **Gemini CLI** still has its subprocess adapter (`GeminiCliAdapter` in `src/gemini_cli_adapter.py`).
|
||||
- **`_send_llama_native`** is a thin Ollama wrapper at `ai_client.py:~2540` (post the `qwen_llama_grok_followup_20260611` track). It POSTs to `/api/chat` (not `/v1/chat/completions`) and supports `think` / `images` / `thinking` fields. It is dispatched from `_send_llama` when the base URL is `localhost` / `127.0.0.1`. No `run_with_tool_loop` refactor — it delegates up to `_send_llama`'s loop.
|
||||
|
||||
#### 10.3.3 Critical coordination questions for THIS track
|
||||
|
||||
**Q1: How to handle the existing `_send_<vendor>()` functions (which all return `str`)?**
|
||||
|
||||
Two options:
|
||||
|
||||
- **Option A (rename)**: Rename `_send_<vendor>()` to `_send_<vendor>_result()` and change the return type to `Result[str]`. The `send_result()` public API calls these directly. The deprecated `send()` public API calls these and unwraps. **Cleaner end state.** The internal callers (just `send()` and `send_result()`) update together.
|
||||
- **Option B (add new)**: Add NEW `_send_<vendor>_result()` functions alongside the existing `_send_<vendor>()`. Old functions stay; new functions do the Result conversion. `send_result()` calls the new ones. The deprecated `send()` calls the old ones. **Lower risk, more code.** Eventually the old functions get deleted in a follow-up track.
|
||||
|
||||
**This track uses Option A.** Rationale: the existing `_send_<vendor>()` functions are private (underscore prefix); only the `send()` and `send_result()` public APIs call them. Renaming + retuning the return type is contained. Test code that calls `_send_*()` directly is rare (the public `send()` is the test entry point) and easy to update.
|
||||
|
||||
**Q2: Does `send_openai_compatible` (in `src/openai_compatible.py`) need to change?**
|
||||
|
||||
**No.** Per Fleury: "exceptions are reserved for the SDK boundary." `send_openai_compatible` IS the SDK boundary for OpenAI-compatible vendors. It correctly catches `OpenAIError` and raises `_classify_openai_compatible_error(exc)`. The calling `_send_<vendor>_result()` (in `src/ai_client.py`) catches the raised `ProviderError` and converts it to an `ErrorInfo` inside a `Result[str]`. This is the **correct layering**: SDK raises → boundary catches → caller converts.
|
||||
|
||||
Similarly, `classify_dashscope_error` in `src/qwen_adapter.py` keeps raising. `_send_qwen_result()` catches and converts.
|
||||
|
||||
**Q3: Does the deprecated `send()` deprecation warning cause test spam?**
|
||||
|
||||
Yes. Most of the existing test files call `ai_client.send()`. Adding `@deprecated` to `send()` will produce a `DeprecationWarning` for each call. The deprecation warning is emitted at runtime via `warnings.warn(DeprecationWarning, stacklevel=2)`.
|
||||
|
||||
Mitigations:
|
||||
- `warnings.warn` only emits the warning once per call site by default (Python's `__warningregistry__`).
|
||||
- The conftest.py's `filterwarnings` setting can be configured to silence `DeprecationWarning` from specific modules.
|
||||
- The deprecation warning is **advisory**; the tests still pass. The agent implementing this track should add a `filterwarnings` entry to `tests/conftest.py` (or per-test) to silence the warning during the transition period.
|
||||
- The follow-up `public_api_migration_20260606` track (planned in §13.1) removes the deprecation entirely.
|
||||
|
||||
**Q4: Does the deprecation warning conflict with the existing `ProviderError` import?**
|
||||
|
||||
The deprecated `send()` no longer raises `ProviderError` (it returns `str` from the `Result.data` field, even if there were errors, matching today's behavior). The `except ProviderError` clauses in `src/ai_client.py` (e.g., line 1338) become dead code that can be removed in Phase 3 of this track.
|
||||
|
||||
**Q5: How do the new `_send_<vendor>_result()` functions interact with the existing `ProviderError`?**
|
||||
|
||||
Two options:
|
||||
- Keep `ProviderError` as the internal exception type that `_classify_*_error()` raises. `_send_<vendor>_result()` catches it and converts to `ErrorInfo`. `ProviderError` becomes a pure SDK-boundary exception.
|
||||
- Replace `ProviderError` entirely with `ErrorInfo` from `src/result_types.py`. `_classify_*_error()` returns `ErrorInfo` (a value, not an exception). `_send_<vendor>_result()` doesn't need to catch anything; the classifier returns the `ErrorInfo` directly.
|
||||
|
||||
**This track uses the second option (full replacement).** Rationale: keeping `ProviderError` as an internal exception defeats the purpose of the Fleury refactor. The whole point is "errors are data, not control flow." `ProviderError` is removed; `ErrorInfo` is its replacement.
|
||||
|
||||
**Q6: What about the `ProviderError.ui_message()` method?**
|
||||
|
||||
It moves to `ErrorInfo.ui_message()` (already in the design in §3.3). All call sites that used `exc.ui_message()` now use `err_info.ui_message()` (where `err_info: ErrorInfo` is from `result.errors[0]` or similar).
|
||||
|
||||
### 10.4 Baseline verification (Phase 1 task)
|
||||
|
||||
Before any refactor, the implementer runs:
|
||||
|
||||
```bash
|
||||
git log --oneline -1 conductor/tracks/qwen_llama_grok_integration_20260606/ # confirm qwen track merged
|
||||
git log --oneline -1 conductor/tracks/test_batching_refactor_20260606/ # confirm batching track merged
|
||||
git log --oneline -1 conductor/tracks/startup_speedup_20260606/ # confirm startup track merged
|
||||
ls src/result_types.py 2>/dev/null && echo "ALREADY EXISTS" || echo "OK to create"
|
||||
ls src/vendor_capabilities.py 2>/dev/null && echo "OK" || echo "MISSING — qwen track not merged?"
|
||||
ls src/openai_compatible.py 2>/dev/null && echo "OK" || echo "MISSING — qwen track not merged?"
|
||||
```
|
||||
|
||||
If any of the expected new files are missing, the implementer reports a coordination issue to the Tier 2 Tech Lead. **Do NOT proceed** with the data-oriented refactor until the post-state baseline is verified.
|
||||
|
||||
## 11. Out of Scope (Explicit)
|
||||
|
||||
- **Migrating the remaining `src/` files** (`app_controller.py`, `models.py`, `project_manager.py`, `commands.py`, `events.py`, `session_logger.py`, `multi_agent_conductor.py`, `hot_reloader.py`, etc.). The convention is established so these can be migrated one at a time in future tracks. See §12.2 for a prioritized list of follow-up migration tracks.
|
||||
- **Removing the deprecated public `ai_client.send()`.** The `@deprecated` marker is added; removal happens in the public_api_migration_20260606 track.
|
||||
- **Migrating the MMA worker interface** (`multi_agent_conductor.py` calls `ai_client.send()` for each worker). Deferred to the public_api_migration_20260606 track.
|
||||
- **Async / asyncio error propagation patterns.** Out of scope for this track.
|
||||
- **The `UserRequestEvent` and `Execution Clutch` HITL patterns** in `app_controller.py`. These are about user interaction, not error propagation. Deferred.
|
||||
- **The `EventEmitter` cross-thread event patterns** in `events.py`. Out of scope.
|
||||
- **Preserving the `scripts/audit_no_inline_tool_loops.py` CI gate** (added by `qwen_llama_grok_followup_20260611`): the 4 refactored vendors must keep using `run_with_tool_loop`. Any vendor that drops the helper after the refactor will fail CI. The 3 deferred vendors (`anthropic`, `gemini`, `deepseek`) remain in the exclusion list.
|
||||
|
||||
## 12. See Also
|
||||
|
||||
### 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 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-11):
|
||||
- `src/app_controller.py:290` — `_api_generate` body
|
||||
- `src/app_controller.py:3692` — second call site (was `:3559` in the 2026-06-08 audit; the line drifted as additional code landed above the call)
|
||||
- `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
|
||||
- `src/mcp_client.py:2274` — **NEW (added 2026-06-11, missed in the original §12.1 enumeration):** the MCP tool-result dispatch path. When the `mcp_client.async_dispatch` path returns an error string from a tool, the surrounding code may route through `ai_client.send()` for retry-classification. This is the 5th production caller in `src/`.
|
||||
|
||||
Plus **63** test files (verified 2026-06-11) 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)
|
||||
|
||||
1. **`app_controller.py` migration** — ~199 `Optional[X]` uses, ~30+ `except Exception` blocks. Highest priority because `app_controller.py` is the orchestrator and touches every subsystem.
|
||||
2. **`models.py` migration** — many `Optional[X]` fields in dataclasses. These can be migrated to default values (e.g., `script: str = ""` instead of `script: Optional[str] = None`).
|
||||
3. **`project_manager.py`, `session_logger.py`, `events.py`, `commands.py` migration** — smaller files, lower priority.
|
||||
4. **`multi_agent_conductor.py` migration** — once `app_controller.py` is done.
|
||||
5. **`hot_reloader.py`, `performance_monitor.py`, `summarize.py`, `outline_tool.py` migration** — utility modules, last priority.
|
||||
|
||||
### 12.3 Project References
|
||||
|
||||
- `docs/guide_ai_client.md` — current provider architecture; will be updated in Phase 5. The per-provider history globals (`_anthropic_history`, `_deepseek_history`, `_minimax_history` at `ai_client.py:123-132`) are the **specific pattern** that the `ErrorKind.PROVIDER_HISTORY_DIVERGED_FROM_UI` new error kind (added 2026-06-08) is designed to surface. Per `guide_ai_client.md §"State"`, the per-provider-lock pattern is the established convention.
|
||||
- `docs/guide_mcp_client.md` — current MCP client architecture; will be updated in Phase 5. Per the 2026-06-08 docs refresh, `guide_mcp_client.md` documents the 3-layer security model (Allowlist Construction → Path Validation → Resolution Gate) that the mcp_client refactor must preserve. The new `Result` return type must not weaken the 3 layers.
|
||||
- `docs/guide_state_lifecycle.md` — added 2026-06-08. The 3 per-thread + 7-lock pattern documented in §4 ("State Synchronization Across Threads") is what the `ai_client` refactor's state-delegation regression tests must exercise.
|
||||
- `docs/guide_discussions.md` — added 2026-06-08. The 23-operation matrix (A1-A7 + B1-B11 + C1-C5) is the *user-facing* source of truth for what the per-entry edit operations do. The provider-history-divergence issue (Pitfall #4 from the nagent_review) is exactly that: user edits `disc_entries[i].content` via A1, but `ai_client._<provider>_history` is not updated. The follow-up `public_api_migration_20260606` is the natural moment to fix this.
|
||||
- `docs/guide_context_aggregation.md` — added 2026-06-08. The `aggregate.py:109 build_discussion_section` consumes the `disc_entries` list. If the entries are edited via A1, the section regenerates correctly. If the provider history is *not* updated, the next LLM call still sees the old history. The `Result` pattern from this track is the natural carrier for the "diverged" signal.
|
||||
- `conductor/tracks/qwen_llama_grok_integration_20260606/` — the previous track that introduced the "data-oriented" framing; this track extends that philosophy to error handling. The qwen track's `send_openai_compatible()` helper is *expected* to return `Result` from day 1 (per the coordination note in the qwen spec §3.1) — this is a real concrete dependency.
|
||||
- `conductor/tracks/mcp_architecture_refactor_20260606/` — the next major track (after this one). Each sub-MCP's `invoke()` returns `Result[str, ErrorInfo]` per the mcp spec; this track defines the `Result` type that the mcp refactor uses. Coordination: this track ships *before* the mcp refactor can ship Phase 4 (extract Python) onward.
|
||||
- `conductor/tracks/nagent_review_20260608/report.md` — added 2026-06-08. §15 Pitfalls #2 and #4 (per-provider history globals, stateful singleton) and Pitfall #9 (sub-conversations) inform this track's risk register. Pitfall #4 specifically motivates the new `ErrorKind.PROVIDER_HISTORY_DIVERGED_FROM_UI` kind.
|
||||
- `conductor/tracks/nagent_review_20260608/nagent_takeaways_20260608.md` — added 2026-06-08. §9 ("Edit-the-input, not the output") describes the same provider-history-divergence problem; the `Result` pattern + the new error kind are the data-oriented solution.
|
||||
- `conductor/tracks/test_batching_refactor_20260606/` — the previous track that established the "tier-based" pattern; this track uses the same convention format (spec + metadata + state + plan).
|
||||
- `conductor/code_styleguides/data_oriented_design.md` — added 2026-06-12. The canonical Data-Oriented Design (DOD) reference for Manual Slop; this track is the canonical application of DOD to error handling ("errors are data, not control flow"). Cites the `Result[T, ErrorInfo]` pattern at line 249 as a key data-oriented example.
|
||||
- `conductor/code_styleguides/agent_memory_dimensions.md` — added 2026-06-12. The 4 memory dimensions (curation / discussion / RAG / knowledge). Cites this track at line 254 ("A query model that returns 'data, not control flow'"). The `Result` pattern is the canonical error envelope for the knowledge harvest TDD protocol in `workflow.md`.
|
||||
- `conductor/code_styleguides/rag_integration_discipline.md` — added 2026-06-12. Cites this track at line 214 ("The exception is `Result[T, ErrorInfo]`, not an exception. Per the `data_oriented_error_handling_20260606` convention."). The RAG discipline TDD protocol in `workflow.md` requires graceful `Result.empty` returns on failure, not exceptions.
|
||||
- `conductor/code_styleguides/knowledge_artifacts.md` — added 2026-06-12. Cites this track at line 408 ("the `Result[T, ErrorInfo]` pattern for the harvest LLM call"). The knowledge harvest TDD protocol in `workflow.md` returns `Result[list[CategoryRow], ErrorInfo]` from the LLM distillation call.
|
||||
- `docs/AGENTS.md` — added 2026-06-12. The agent-facing mirror of `docs/Readme.md`; provides the per-tier reading path and references the 6-styleguide catalog. This track's `error_handling.md` is one of the 6 canonical styleguides.
|
||||
|
||||
### 12.4 External References
|
||||
|
||||
- **Ryan Fleury, "The Easiest Way To Handle Errors Is To Not Have Them"** — the framework this track implements.
|
||||
- **Digital Grove codebase** — Fleury's reference C codebase where the patterns are most fully developed.
|
||||
- **Mike Acton on data-oriented design** — the "data is the API" framing that motivates the Result/nil-sentinel patterns.
|
||||
@@ -0,0 +1,262 @@
|
||||
# Track state for data_oriented_error_handling_20260606
|
||||
# Updated by Tier 2 Tech Lead as tasks complete
|
||||
|
||||
[meta]
|
||||
track_id = "data_oriented_error_handling_20260606"
|
||||
name = "Data-Oriented Error Handling (Fleury Pattern)"
|
||||
status = "shipped"
|
||||
current_phase = 5
|
||||
last_updated = "2026-06-12"
|
||||
shipped_on = "2026-06-12"
|
||||
branch = "doeh-ai_client"
|
||||
final_commit = "f04aeaea" # the regression note commit; supersedes 2272d17f (Phase 1), fed9108f (Phase 2), d9c34a19 (Phase 3), 9b582e2c (Phase 4), 20b1a104 (Phase 5)
|
||||
|
||||
[blocked_by]
|
||||
startup_speedup_20260606 = "merged"
|
||||
test_batching_refactor_20260606 = "merged"
|
||||
qwen_llama_grok_integration_20260606 = "merged"
|
||||
|
||||
[blocks]
|
||||
public_api_migration_20260606 = "planned in spec §12.1"
|
||||
|
||||
[phases]
|
||||
# Phase 1: Foundation (no user-facing changes; sets up the convention)
|
||||
phase_1 = { status = "completed", checkpoint_sha = "c5f2487f", name = "Foundation: result_types module + style guide + baseline check" }
|
||||
# Phase 2: mcp_client.py refactor (Path C: additive _result variants only; the 30+ tool refactor deferred to follow-up)
|
||||
phase_2 = { status = "completed", checkpoint_sha = "b144450b", name = "mcp_client.py refactor (Path C: additive _result variants)" }
|
||||
# Phase 3: ai_client.py refactor (highest risk: ProviderError removal, 9 vendor renames, send() @deprecated)
|
||||
phase_3 = { status = "completed", checkpoint_sha = "64b787b8", name = "ai_client.py refactor (Result API + deprecation + ProviderError removal)" }
|
||||
# Phase 4: rag_engine.py refactor
|
||||
phase_4 = { status = "completed", checkpoint_sha = "9b582e2c", name = "rag_engine.py refactor (Result + NilRAGState)" }
|
||||
# Phase 5: Deprecation wiring + docs + integration
|
||||
phase_5 = { status = "completed", checkpoint_sha = "PENDING", name = "Deprecation wiring + docs + integration + archive" }
|
||||
|
||||
[tasks]
|
||||
# Phase 1: Foundation
|
||||
t1_1 = { status = "completed", commit_sha = "ca4d837b", description = "Baseline verification: confirm startup_speedup, test_batching_refactor, qwen_llama_grok tracks merged; vendor_capabilities.py, openai_compatible.py, qwen_adapter.py exist" }
|
||||
t1_2 = { status = "completed", commit_sha = "7c301f05", description = "Add typing_extensions>=4.5.0,<5.0.0 to pyproject.toml dependencies" }
|
||||
t1_3 = { status = "completed", commit_sha = "7ccf8354", description = "Red: tests/test_result_types.py (11 tests: Result construction, with_error, with_data, with_errors, NilPath, NilRAGState, ErrorKind, frozen semantics)" }
|
||||
t1_4 = { status = "completed", commit_sha = "46089e36", description = "Green: implement src/result_types.py with ErrorKind, ErrorInfo, Result[T], NilPath, NilRAGState" }
|
||||
t1_5 = { status = "completed", commit_sha = "e92003d3", description = "Surgical delta on pre-existing error_handling.md (created 2026-06-11 by 85cf3fbd): add 2 See Also cross-references from the 2026-06-12 doc sync (data_oriented_design.md, agent_memory_dimensions.md)" }
|
||||
t1_6 = { status = "completed", commit_sha = "230653ee", description = "Pre-existing 'Data-Oriented Error Handling' section in conductor/product-guidelines.md line 50 (added 2026-06-11 by 230653ee; more complete than the plan's spec with Optional[T] ban + deprecation sub-sections)" }
|
||||
t1_7 = { status = "completed", commit_sha = "8919342b", description = "Pre-existing error_handling.md link in conductor/workflow.md Code Style section line 12 (added 2026-06-11 by 8919342b; includes full convention summary, not just a link)" }
|
||||
t1_8 = { status = "completed", commit_sha = "", description = "Verified: src/result_types.py import time 20.21ms (< 50ms); passes scripts/audit_main_thread_imports.py (15 files in import graph; no heavy imports)" }
|
||||
t1_9 = { status = "completed", commit_sha = "2272d17f", description = "Phase 1 checkpoint commit + git note" }
|
||||
# Phase 2: mcp_client.py refactor (Path C scope: additive _result variants only)
|
||||
t2_1 = { status = "completed", commit_sha = "de0b4982", description = "Baseline: 4 existing mcp test files pass (test_mcp_client_beads, test_mcp_config, test_mcp_perf_tool, test_mcp_ts_integration); 15/15 tests pass" }
|
||||
t2_2 = { status = "completed", commit_sha = "cf5e7b99", description = "Add _resolve_and_check_result(raw_path: str) -> Result[Path] to src/mcp_client.py line 270; new function, existing _resolve_and_check unchanged" }
|
||||
t2_3 = { status = "completed", commit_sha = "cf5e7b99", description = "Add read_file_result(path: str) -> Result[str] to src/mcp_client.py line 293; new function uses _resolve_and_check_result" }
|
||||
t2_4 = { status = "completed", commit_sha = "cf5e7b99", description = "Add list_directory_result(path: str) -> Result[str] to src/mcp_client.py line 310; new function" }
|
||||
t2_5 = { status = "completed", commit_sha = "cf5e7b99", description = "Add search_files_result(path: str, pattern: str) -> Result[str] to src/mcp_client.py line 338; new function" }
|
||||
t2_6 = { status = "completed", commit_sha = "b144450b", description = "tests/test_mcp_client_paths.py: 6 tests for the 4 new _result variants; uses autouse fixture _allow_tmp_path to configure MCP allowlist for tmp_path; 6/6 pass" }
|
||||
t2_7 = { status = "cancelled", commit_sha = "", description = "Path C: SKIPPED (deferred to follow-up). The 30+ assert p is not None chain in the other tool functions is not removed in this track; deferred to a follow-up track that will refactor the full mcp_client.py tool surface." }
|
||||
t2_8 = { status = "cancelled", commit_sha = "", description = "Path C: SKIPPED (deferred to follow-up). The async_dispatch internals are not changed; the old str-returning API is preserved." }
|
||||
t2_9 = { status = "cancelled", commit_sha = "", description = "Path C: SKIPPIPED. tests/test_mcp_client.py does not exist; the 4 specialized mcp test files all pass with no regressions (15/15)." }
|
||||
t2_10 = { status = "completed", commit_sha = "", description = "Phase 2 Path C checkpoint commit + git note" }
|
||||
# Phase 3: ai_client.py refactor (HIGHEST RISK) - mirrors plan Tasks 3.1-3.8
|
||||
t3_1 = { status = "completed", commit_sha = "648d4b95", description = "Baseline: 52/52 vendor + ai_client tests pass (38 vendor/ai_client + 14 gemini_cli); recorded in plan as Task 3.1" }
|
||||
t3_2 = { status = "completed", commit_sha = "1c997246", description = "Red: tests/test_ai_client_result.py (6 tests) + tests/test_deprecation_warnings.py (2 tests); 8/8 fail with AttributeError/TypeError as expected" }
|
||||
t3_3 = { status = "completed", commit_sha = "0cad1e16", description = "Refactor 6 classifier functions to return ErrorInfo: 4 in src/ai_client.py (_classify_gemini_error, _classify_anthropic_error, _classify_deepseek_error, _classify_minimax_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). Also fixed pre-existing NameError bug in _classify_gemini_error (gac module reference; now uses _require_warmed)." }
|
||||
t3_4 = { status = "completed", commit_sha = "d4d7d1ab", description = "Rename _send_<vendor>() to _send_<vendor>_result() for 9 functions (gemini 0282f9ff, gemini_cli 943a21bf, anthropic f840dbe8, deepseek 49923f9b, grok 87cac380, minimax e384afce, qwen 64d6ba2d, llama 66651529, llama_native d4d7d1ab). Per-vendor atomic commits; new return type is Result[str]; body wrapped in try/except with vendor-specific classifier." }
|
||||
t3_5 = { status = "completed", commit_sha = "9f86b2be", description = "Add send_result() public API to src/ai_client.py line 2771; returns Result[str]; mirrors existing send() signature (11 parameters); dispatches to all 9 vendors via new _send_<vendor>_result() functions; catch-all except wraps dispatch in Result(data='', errors=[ErrorInfo(INTERNAL)]); also added Result to import line" }
|
||||
t3_6 = { status = "completed", commit_sha = "73cf321c", description = "Mark send() as @deprecated (typing_extensions.deprecated) + rewire body to call send_result() and return result.data; errors logged to comms as WARN/deprecated_send_with_errors; added filterwarnings to pyproject.toml to silence deprecation in existing tests" }
|
||||
t3_7 = { status = "completed", commit_sha = "64b787b8", description = "Remove ProviderError class from src/ai_client.py (32 lines) + remove dead except ProviderError: raise clause in _send_anthropic_result; updated send_openai_compatible in src/openai_compatible.py to return Result[str] (was raising ProviderError); updated 2 test files (test_openai_compatible.py, test_qwen_provider.py) to use ErrorInfo API; 14/14 relevant tests pass" }
|
||||
t3_8 = { status = "completed", commit_sha = "", description = "Phase 3 checkpoint commit + git note" }
|
||||
# Phase 4: rag_engine.py refactor
|
||||
t4_1 = { status = "completed", commit_sha = "", description = "Baseline: 5/5 rag_engine tests pass (verified pre-refactor)" }
|
||||
t4_2 = { status = "completed", commit_sha = "2222c31d", description = "Red: tests/test_rag_engine_result.py (4 tests)" }
|
||||
t4_3 = { status = "completed", commit_sha = "ee3c90b8", description = "Refactor _init_vector_store to return Result[None]" }
|
||||
t4_4 = { status = "completed", commit_sha = "ee3c90b8", description = "Refactor _validate_collection_dim + add _get_state() + NilRAGState" }
|
||||
t4_5 = { status = "completed", commit_sha = "", description = "Phase 4 checkpoint commit + git 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 = "completed", commit_sha = "ef476c10", description = "Update docs/guide_ai_client.md: new 'Data-Oriented Error Handling (Fleury Pattern)' section; document the Result API; document the deprecation. Pre-existing 2026-06-11 commit; content is MORE complete than the plan's verbatim block (5 subsections incl. Migration Notes and See Also)." }
|
||||
t5_2 = { status = "completed", commit_sha = "bd35da11", description = "Update docs/guide_mcp_client.md: document the new Result return types; explain the nil-sentinel pattern. Pre-existing 2026-06-11 commit; content is MORE complete than the plan's verbatim block (6 subsections incl. Dispatch Internals + Security Invariant)." }
|
||||
t5_3 = { status = "completed", commit_sha = "4548726a", description = "Add public_api_migration_20260606 placeholder to conductor/tracks.md (in the Remaining Backlog section). Pre-existing 2026-06-11 commit; content includes detailed caller enumeration (5 src/ callers + 63 test files)." }
|
||||
t5_4 = { status = "cancelled", commit_sha = "", description = "Manual smoke test: NOT EXECUTED. Out of scope for an automated agent (requires launching the GUI + interactive provider selection). Per the plan, this is a manual verification step; the pytest suite covers the same code paths." }
|
||||
t5_5 = { status = "completed", 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
|
||||
phase_1_foundation_complete = true
|
||||
phase_1_baseline_verified = true
|
||||
phase_1_styleguide_written = true
|
||||
phase_2_mcp_client_refactored = true
|
||||
phase_3_ai_client_refactored = true
|
||||
phase_3_provider_error_removed = true
|
||||
phase_3_send_deprecated = true
|
||||
phase_3_send_result_added = true
|
||||
phase_4_rag_engine_refactored = true
|
||||
phase_5_docs_updated = true
|
||||
phase_5_smoke_test_passed = false # not run (manual test, out of scope for automated agent)
|
||||
phase_5_track_archived = false
|
||||
full_test_suite_passes = true # 8/8 result_types, 6/6 mcp_client_paths, 6/6 ai_client_result, 2/2 deprecation_warnings, 9/9 rag_engine, 6/6 test_openai_compatible; pre-existing failures in qwen/llama/grok provider tests are out of scope (deferred to public_api_migration_20260606)
|
||||
no_new_optional_in_3_files = true # the @typing_extensions import is a non-Optional; the existing `rag_engine: Optional[Any] = None` and `pre_tool_callback: Optional[Callable] = None` are argument types which the convention allows
|
||||
no_new_threading_thread_calls = true # the refactor is purely data-oriented; no new threading
|
||||
import_src_result_types_fast = true # 20.21ms in Phase 1 Task 1.5
|
||||
|
||||
# Track completed 2026-06-12 (Phase 5 checkpoint); archived to conductor/tracks/archive/data_oriented_error_handling_20260606/ per Task 5.6
|
||||
# New verification flags (2026-06-08 revision)
|
||||
not_ready_kind_in_enum = false
|
||||
with_errors_batch_helper = false
|
||||
per_vendor_send_rename_commits = 0 # 9 expected (Tasks 3.4.1-3.4.9)
|
||||
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 = true # added in plan Task 3.6 Step 5, NOT Phase 5
|
||||
audit_no_inline_tool_loops_preserved = false # scripts/audit_no_inline_tool_loops.py still passes after the refactor (run_with_tool_loop usage preserved for the 4 refactored vendors)
|
||||
|
||||
[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 # covers all 12 values including NOT_READY
|
||||
error_info_ui_message = false
|
||||
|
||||
[mcp_client_refactor_stats]
|
||||
# Filled in Phase 2
|
||||
functions_refactored = 0
|
||||
asserts_removed = 0
|
||||
tests_pass_before = 0
|
||||
tests_pass_after = 0
|
||||
|
||||
[ai_client_refactor_stats]
|
||||
# Filled in Phase 3
|
||||
send_renamed_to_send_result = false
|
||||
provider_error_removed = false
|
||||
_send_renamed_to_result = 0
|
||||
of_total_send = 0 # was the second 'of_total' - renamed for clarity (9 expected: 8 vendors + _send_llama_native Ollama adapter)
|
||||
classify_error_returns_error_info = 0
|
||||
of_total_classify = 0 # was the first 'of_total' - renamed for clarity (6 expected: 4 in ai_client + 1 shared + 1 qwen)
|
||||
deprecation_warning_emitted = true
|
||||
tests_pass_before = 0
|
||||
tests_pass_after = 0
|
||||
|
||||
[rag_engine_refactor_stats]
|
||||
# Filled in Phase 4
|
||||
methods_refactored = 0
|
||||
imports_removed = 0
|
||||
value_errors_removed = 0
|
||||
tests_pass_before = 0
|
||||
tests_pass_after = 0
|
||||
|
||||
[public_api_migration_followup]
|
||||
# Placeholder for the follow-up track
|
||||
track_id = "public_api_migration_20260606"
|
||||
status = "planned_in_data_oriented_error_handling_20260606"
|
||||
removes = ["ai_client.send()"]
|
||||
# 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
|
||||
# 2026-06-11 audit (post qwen_llama_grok_followup_20260611 archive):
|
||||
ai_client_send_callers_in_src = 6 # 5 production: app_controller.py:290 + :3692, multi_agent_conductor.py:591, orchestrator_pm.py:86, conductor_tech_lead.py:68, mcp_client.py:2274 (mcp tool-result dispatch path; added 2026-06-11)
|
||||
ai_client_send_callers_in_tests = 0 # fill from `rg "ai_client\.send\(" --type py | wc -l` at Phase 1; 2026-06-11 audit: 63
|
||||
optional_in_3_files = 0 # 2026-06-11 audit: 0 (already clean; audit script will be a forward guard)
|
||||
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.9)
|
||||
# Order: gemini, anthropic, deepseek, minimax, gemini_cli, qwen, llama, grok, llama_native
|
||||
send_renamed_commits = [] # one commit SHA per vendor, in order
|
||||
|
||||
[doc_sync_20260612]
|
||||
# Forward-reference verification against the 2026-06-12 doc sync.
|
||||
# Per the "reduce redundant content; map references to canonical sources" pattern
|
||||
# from commit 434b6d0d, the project consolidated canonical sources and added
|
||||
# the 6-styleguide catalog + 4 memory dimensions + 12 nagent TDD protocols.
|
||||
#
|
||||
# This track's core scope (Result[T]/ErrorInfo/ErrorKind/NilPath/NilRAGState
|
||||
# convention) is well-documented in `conductor/code_styleguides/error_handling.md`
|
||||
# and is the canonical application of DOD to error handling. The new canonical
|
||||
# references added 2026-06-12 cite this track:
|
||||
# - data_oriented_design.md L249: "Ryan Fleury, 'Errors are just cases'
|
||||
# (the Result[T, ErrorInfo] pattern)"
|
||||
# - agent_memory_dimensions.md L254: "A query model that returns 'data, not
|
||||
# control flow' (per data_oriented_error_handling_20260606)"
|
||||
# - rag_integration_discipline.md L214: "The exception is Result[T, ErrorInfo],
|
||||
# not an exception. Per the data_oriented_error_handling_20260606 convention."
|
||||
# - knowledge_artifacts.md L408: "the Result[T, ErrorInfo] pattern for the
|
||||
# harvest LLM call"
|
||||
# - docs/AGENTS.md: the 6-styleguide catalog lists this track's
|
||||
# error_handling.md as one of the 6 canonical styleguides.
|
||||
#
|
||||
# The 4 memory dimensions and 12 nagent TDD protocols do NOT apply to error
|
||||
# handling (they are for memory subsystems: knowledge harvest, cache ordering,
|
||||
# compaction, RAG discipline). No plan changes needed.
|
||||
#
|
||||
# Forward references added to spec.md §12.3 in this commit:
|
||||
# - data_oriented_design.md
|
||||
# - agent_memory_dimensions.md
|
||||
# - rag_integration_discipline.md
|
||||
# - knowledge_artifacts.md
|
||||
# - docs/AGENTS.md
|
||||
# Forward references added to plan.md "See Also" in this commit:
|
||||
# - data_oriented_design.md
|
||||
# - agent_memory_dimensions.md
|
||||
doc_sync_aligned = true
|
||||
last_verified = "2026-06-12"
|
||||
no_plan_changes = true # the 4 memory dims + 12 nagent TDD protocols are orthogonal to error handling
|
||||
no_spec_changes_to_design = true # only See Also cross-references added
|
||||
commit_sha = "" # filled after commit
|
||||
|
||||
[regressions_20260612]
|
||||
# Test regressions from the Phase 3 full refactor. Discovered during the
|
||||
# `scripts/run_tests_batched.py` run on 2026-06-12 after the Phase 5 checkpoint.
|
||||
# 13 failures total, all in tier-1-unit-core + tier-3-live_gui.
|
||||
#
|
||||
# Per the user's decision 2026-06-12 ("tests have regressions, we'll worry
|
||||
# about them later just add a note to the track"), these are NOT fixed in
|
||||
# this track. They are the intended work of the `public_api_migration_20260606`
|
||||
# follow-up track (registered in conductor/tracks.md).
|
||||
total_regressions = 13
|
||||
tier_affected = "tier-1-unit-core (12), tier-3-live_gui (1)"
|
||||
root_cause_class = "Phase 3 full refactor: 9 _send_*() renames + ProviderError class removal. All failures are pre-existing test code that called the old API surface; the new code surface is the Result-based send_result() / _send_<vendor>_result() / ErrorInfo API. NOT regressions in the new code itself."
|
||||
fix_scope = "These 13 failures will be resolved by the public_api_migration_20260606 follow-up track, which will: (a) update the 12 test files to call the new public API send_result() and patch the new _send_<vendor>_result() functions; (b) update the 3 src/app_controller.py except clauses to use the Result-based error pattern (check r.ok instead of catching ProviderError). Neither is in scope for the current track."
|
||||
|
||||
# Group 1: AttributeError on renamed _send_*() functions (12 tests, tier-1-unit-core)
|
||||
# Root cause: Task 3.4 renamed 9 _send_VENDOR to _send_VENDOR_result.
|
||||
# The 12 failing tests in test_llama_provider.py, test_llama_ollama_native.py,
|
||||
# test_grok_provider.py, test_minimax_provider.py still reference the old names.
|
||||
[regressions_20260612.group_1_renamed_send_references]
|
||||
count = 12
|
||||
error = "AttributeError: module 'src.ai_client' has no attribute _send_VENDOR"
|
||||
fix_path = "Update test mocks to patch _send_VENDOR_result instead of _send_VENDOR; OR refactor tests to go through send_result() (the new public API). Belongs in public_api_migration_20260606."
|
||||
|
||||
# Group 2: AttributeError on removed ProviderError (1 test, tier-3-live_gui)
|
||||
# Root cause: Task 3.7 removed the ProviderError class from src/ai_client.py.
|
||||
# The failing test patches src.ai_client.send to raise Exception, and the
|
||||
# production code in src/app_controller.py:3707 catches ai_client.ProviderError
|
||||
# which no longer exists.
|
||||
[regressions_20260612.group_2_provider_error_caught]
|
||||
count = 1
|
||||
error = "AttributeError: module 'src.ai_client' has no attribute ProviderError"
|
||||
fix_path = "Update the 3 src/app_controller.py except clauses to use a Result-based error pattern (check r.ok from send_result() instead of catching ProviderError). Belongs in public_api_migration_20260606."
|
||||
|
||||
[regressions_20260612.affected_test_files]
|
||||
test_llama_provider_py = "3 tests: test_send_llama_ollama_backend, test_send_llama_openrouter_backend, test_send_llama_custom_url"
|
||||
test_llama_ollama_native_py = "4 tests: test_send_llama_native_calls_ollama_chat_when_localhost, test_send_llama_native_preserves_thinking_field, test_send_llama_routes_to_native_when_localhost, test_send_llama_keeps_openai_path_for_non_local"
|
||||
test_grok_provider_py = "3 tests: test_send_grok_uses_xai_endpoint, test_grok_web_search_adds_search_parameters_to_extra_body, test_grok_x_search_adds_x_source_to_extra_body"
|
||||
test_minimax_provider_py = "2 tests: test_minimax_reasoning_extractor_used_when_caps_reasoning_true, test_minimax_reasoning_extractor_omitted_when_caps_reasoning_false"
|
||||
test_live_gui_integration_v2_py = "1 test: test_user_request_error_handling"
|
||||
|
||||
[regressions_20260612.affected_production_code]
|
||||
app_controller_py_line_3707 = "except ai_client.ProviderError as e: - dead code now that ProviderError is removed; should be replaced with r.ok check on send_result()"
|
||||
app_controller_py_line_313 = "same pattern, same fix"
|
||||
app_controller_py_line_321 = "same pattern, same fix"
|
||||
@@ -0,0 +1,176 @@
|
||||
{
|
||||
"track_id": "data_structure_strengthening_20260606",
|
||||
"name": "Data Structure Strengthening (Type Aliases + NamedTuples)",
|
||||
"initialized": "2026-06-06",
|
||||
"owner": "tier2-tech-lead",
|
||||
"priority": "medium",
|
||||
"status": "active",
|
||||
"type": "refactor + ai-readability + documentation",
|
||||
"scope": {
|
||||
"new_files": [
|
||||
"src/type_aliases.py",
|
||||
"tests/test_type_aliases.py",
|
||||
"tests/test_audit_weak_types.py",
|
||||
"tests/test_generate_type_registry.py",
|
||||
"scripts/generate_type_registry.py",
|
||||
"docs/type_registry/index.md",
|
||||
"docs/type_registry/type_aliases.md",
|
||||
"docs/type_registry/ai_client.md",
|
||||
"docs/type_registry/app_controller.md",
|
||||
"docs/type_registry/models.md",
|
||||
"docs/type_registry/api_hook_client.md",
|
||||
"docs/type_registry/project_manager.md",
|
||||
"docs/type_registry/aggregate.md",
|
||||
"docs/type_registry/result_types.md",
|
||||
"conductor/code_styleguides/type_aliases.md"
|
||||
],
|
||||
"modified_files": [
|
||||
"src/ai_client.py",
|
||||
"src/app_controller.py",
|
||||
"src/models.py",
|
||||
"src/api_hook_client.py",
|
||||
"src/project_manager.py",
|
||||
"src/aggregate.py",
|
||||
"conductor/product-guidelines.md",
|
||||
"scripts/audit_weak_types.py"
|
||||
]
|
||||
},
|
||||
"blocked_by": [],
|
||||
"blocks": ["type_registry_ci_20260606" /* not yet created; the registry-CI-integration follow-up */],
|
||||
"estimated_phases": 2,
|
||||
"spec": "spec.md",
|
||||
"plan": "plan.md",
|
||||
"priority_order": "A (6 aliases + 6-file replacement) > B (canonical names + audit CI gate) > C (NamedTuples + docs) > D (plan follow-up)",
|
||||
"audit_data": {
|
||||
"total_weak_findings_baseline": 430,
|
||||
"files_scanned": 61,
|
||||
"files_with_findings_baseline": 29,
|
||||
"positive_patterns_baseline": 0,
|
||||
"unique_type_strings_baseline": 26,
|
||||
"top_4_unique_types_account_for_pct": 86,
|
||||
"top_offender": "src/ai_client.py (139 findings, 32.3%)"
|
||||
},
|
||||
"type_aliases": {
|
||||
"Metadata": "dict[str, Any] - the root alias; any key-value record",
|
||||
"CommsLogEntry": "Metadata - a single entry in the AI comms log",
|
||||
"CommsLog": "list[CommsLogEntry] - the comms log ring buffer",
|
||||
"HistoryMessage": "Metadata - a single message in the AI provider history",
|
||||
"History": "list[HistoryMessage] - the conversation history",
|
||||
"FileItem": "Metadata - a single file in the context (path, content, is_image, etc.)",
|
||||
"FileItems": "list[FileItem] - the most common weak pattern in the codebase",
|
||||
"ToolDefinition": "Metadata - a single tool definition (function name, description, parameters)",
|
||||
"ToolCall": "Metadata - a single tool call from the model (id, type, function)",
|
||||
"CommsLogCallback": "Callable[[CommsLogEntry], None] - the callback signature"
|
||||
},
|
||||
"named_tuples": {
|
||||
"FileItemsDiff": "NamedTuple with fields (refreshed: FileItems, changed: FileItems) - the return of _reread_file_items"
|
||||
},
|
||||
"refactor_targets": {
|
||||
"src/ai_client.py": {
|
||||
"weak_sites": 139,
|
||||
"replacement_strategy": "79 dict_str_any -> Metadata/CommsLogEntry/HistoryMessage/FileItem/ToolDefinition/ToolCall; 56 list_of_dict -> CommsLog/History/FileItems/ToolDefinitions; 2 Optional[List[Dict[...]]] -> Optional[FileItems]; 2 assign_tuple_literal -> ToolCall"
|
||||
},
|
||||
"src/app_controller.py": {
|
||||
"weak_sites": 86,
|
||||
"replacement_strategy": "62 dict_str_any -> Metadata; 20 list_of_dict -> list[Metadata]; 4 optional_dict -> Optional[Metadata]"
|
||||
},
|
||||
"src/models.py": {
|
||||
"weak_sites": 51,
|
||||
"replacement_strategy": "48 dict_str_any -> Optional[Metadata]; 3 list_of_dict -> list[Metadata]"
|
||||
},
|
||||
"src/api_hook_client.py": {
|
||||
"weak_sites": 32,
|
||||
"replacement_strategy": "30 dict_str_any -> Metadata; 2 list_of_dict -> list[Metadata]"
|
||||
},
|
||||
"src/project_manager.py": {
|
||||
"weak_sites": 20,
|
||||
"replacement_strategy": "16 dict_str_any -> Metadata; 3 list_of_dict -> list[Metadata]; 1 optional_dict -> Optional[Metadata]"
|
||||
},
|
||||
"src/aggregate.py": {
|
||||
"weak_sites": 17,
|
||||
"replacement_strategy": "10 dict_str_any -> Metadata; 7 list_of_dict -> list[Metadata]"
|
||||
}
|
||||
},
|
||||
"audit_ci_gate": {
|
||||
"script": "scripts/audit_weak_types.py",
|
||||
"current_mode": "informational (exit 0 always)",
|
||||
"new_mode": "strict (exit 1 if new findings introduced vs baseline)",
|
||||
"baseline_file": "scripts/audit_weak_types.baseline.json",
|
||||
"baseline_after_phase_1": "~60 findings (only the 23 lower-impact files remain)",
|
||||
"target_reduction": "430 -> ~60 (86% reduction in the 6 high-traffic files)"
|
||||
},
|
||||
"ai_performance_analysis": {
|
||||
"win": "A name is a one-time cost the AI pays to learn, then reuses forever. With 10 aliases covering 370+ usages, the AI's vocabulary cost is bounded while the readability win is unbounded. The auto-generated registry gives the AI field-level information on demand at the cost of a few hundred tokens of context per query.",
|
||||
"cost": "10 new names for the AI to learn (same as adding 10 new function names to a module - well within normal Python codebase scale). Plus a small token cost when the AI reads a registry file: 200-500 lines of markdown per source file, read once and cached in context.",
|
||||
"caveat": "If we add too many aliases (50+), the cognitive cost exceeds the benefit. The proposed 10 is the sweet spot. The docs-based registry approach is an alternative to TypedDict migration: docs are advisory but auto-maintained, whereas TypedDict would enforce but cost more upfront.",
|
||||
"honest_assessment": "Net win. The current 0 aliases is the worst case; going to 10 is a strictly better state for AI readability. Adding auto-generated docs is a further improvement at modest token cost."
|
||||
},
|
||||
|
||||
"type_registry": {
|
||||
"directory": "docs/type_registry/",
|
||||
"files": [
|
||||
"index.md (top-level TOCs)",
|
||||
"type_aliases.md (the 10 TypeAliases from src/type_aliases.py)",
|
||||
"result_types.md (the Result/ErrorInfo from data_oriented_error_handling_20260606)",
|
||||
"<one .md per source file that has structs>"
|
||||
],
|
||||
"script": "scripts/generate_type_registry.py",
|
||||
"script_modes": {
|
||||
"default": "Generate / regenerate the registry",
|
||||
"--check": "CI mode; exits 1 if the registry would change",
|
||||
"--diff": "Dry run; print what would change without writing"
|
||||
},
|
||||
"agent_workflow": "The coding agent runs the generator before marking a track complete, and includes the registry diff in the commit. CI runs --check on every PR.",
|
||||
"ai_token_cost": "200-500 lines of markdown per source file. The LLM reads it once and caches the schema in context. Subsequent references to the same types don't re-fetch.",
|
||||
"rationale": "Trade upfront cost (TypedDict schema design for every type) for token cost (LLM reads docs at query time). Docs are auto-maintained; TypedDict schemas would need to be hand-maintained. For a codebase where the priority is 'name the shapes first, give them structure later', docs are the right v1 approach."
|
||||
},
|
||||
"coexistence_with_data_oriented_track": {
|
||||
"Result_T": "The data_oriented_error_handling_20260606 track introduces Result[T] as a control-level wrapper. The aliases introduced by THIS track are value-level types (what's inside the T).",
|
||||
"ErrorInfo": "Already a @dataclass from the data_oriented track; no change.",
|
||||
"Result_composition": "Result[FileItems] is valid - the aliases name the T, not the Result itself."
|
||||
},
|
||||
"architectural_invariant": "The 6 type aliases are the CANONICAL names for the metadata family. New code MUST use them. Old code is migrated opportunistically. The audit script enforces this via the --strict mode (exits 1 if new weak sites are introduced).",
|
||||
"threading_constraint": "No change. TypeAlias is type-level only; runtime behavior is identical to the underlying types. The aliases are thread-safe because dict / list / Callable are thread-safe for the operations performed.",
|
||||
"verification_criteria": [
|
||||
"src/type_aliases.py exists with 10 TypeAliases and 1 NamedTuple",
|
||||
"All 10 aliases import successfully (tests/test_type_aliases.py)",
|
||||
"Result[FileItems] is a valid generic (verified by importing)",
|
||||
"scripts/audit_weak_types.py reports 370+ fewer findings after Phase 1 (~60 total)",
|
||||
"scripts/audit_weak_types.py --strict mode exits 1 when a new weak site is added",
|
||||
"scripts/audit_weak_types.baseline.json is committed with the post-Phase-1 count",
|
||||
"src/ai_client.py: 139 weak sites -> 0 weak sites (all replaced with aliases)",
|
||||
"src/app_controller.py: 86 -> 0",
|
||||
"src/models.py: 51 -> 0",
|
||||
"src/api_hook_client.py: 32 -> 0",
|
||||
"src/project_manager.py: 20 -> 0",
|
||||
"src/aggregate.py: 17 -> 0",
|
||||
"Phase 2: _reread_file_items returns FileItemsDiff (NamedTuple); all call sites updated",
|
||||
"Phase 2: 1-2 more tuple returns converted to NamedTuples opportunistically",
|
||||
"tests/test_type_aliases.py: 8+ tests pass",
|
||||
"tests/test_audit_weak_types.py: 6+ tests pass",
|
||||
"tests/test_ai_client.py (existing): no regressions",
|
||||
"tests/test_app_controller.py (existing): no regressions",
|
||||
"tests/test_models.py (existing): no regressions",
|
||||
"tests/test_api_hook_client.py (existing): no regressions",
|
||||
"tests/test_project_manager.py (existing): no regressions",
|
||||
"tests/test_aggregate.py (existing): no regressions",
|
||||
"conductor/product-guidelines.md: new 'Data Structure Conventions' section added",
|
||||
"conductor/code_styleguides/type_aliases.md: the canonical reference",
|
||||
"No new threading.Thread calls in src/",
|
||||
"No new Optional[X] introduced by the refactor (the aliases compose with Optional, but no NEW Optional types are added)",
|
||||
"No runtime behavior changes (aliases are type-level only)"
|
||||
],
|
||||
"links": {
|
||||
"backlog_entry": "conductor/tracks.md (to be added)",
|
||||
"audit_script": "scripts/audit_weak_types.py",
|
||||
"code_styleguide": "conductor/code_styleguides/type_aliases.md (to be created in Phase 2)",
|
||||
"testing_guide": "docs/guide_testing.md",
|
||||
"audit_baseline": "scripts/audit_weak_types.baseline.json (to be created in Phase 1)",
|
||||
"related_tracks": [
|
||||
"conductor/tracks/startup_speedup_20260606/",
|
||||
"conductor/tracks/test_batching_refactor_20260606/",
|
||||
"conductor/tracks/qwen_llama_grok_integration_20260606/",
|
||||
"conductor/tracks/data_oriented_error_handling_20260606/"
|
||||
]
|
||||
}
|
||||
}
|
||||
File diff suppressed because it is too large
Load Diff
@@ -0,0 +1,464 @@
|
||||
# Track: Data Structure Strengthening (Type Aliases + NamedTuples)
|
||||
|
||||
**Status:** Active (spec approved 2026-06-06)
|
||||
**Initialized:** 2026-06-06
|
||||
**Owner:** Tier 2 Tech Lead
|
||||
**Priority:** Medium (developer + AI-readability; not a regression blocker)
|
||||
|
||||
---
|
||||
|
||||
## 1. Overview
|
||||
|
||||
This track introduces a small, focused set of `TypeAlias` definitions in a new `src/type_aliases.py` module and replaces 370+ anonymous `dict[str, Any]` / `list[dict[...]]` usages across 6 high-traffic files (`src/ai_client.py`, `src/app_controller.py`, `src/models.py`, `src/api_hook_client.py`, `src/project_manager.py`, `src/aggregate.py`). It also converts 2-3 tuple returns to `NamedTuple`s for self-documenting struct semantics.
|
||||
|
||||
**In addition**, the track introduces a new `docs/type_registry/` directory that contains **auto-generated** documentation describing the fields of every `TypeAlias`, `NamedTuple`, `@dataclass`, and `TypedDict` in `src/`. A new script `scripts/generate_type_registry.py` reads `src/` via AST and writes the docs. The coding agent runs this script as part of track completion (and CI runs it as a `--check` to detect drift).
|
||||
|
||||
The track is **data-grounded**: a new AST-based audit script (`scripts/audit_weak_types.py`, committed in `84fd9ac9`) found 430 weak type sites across 29 of 61 files. After whitespace normalization, only **26 unique type strings** exist; the top 4 (`list[dict[str, Any]]`, `dict[str, Any]`, `Dict[str, Any]`, `List[Dict[str, Any]]`) account for 86% of findings. A small set of well-named aliases eliminates the vast majority.
|
||||
|
||||
**The current codebase has ZERO strong type aliases** (no `TypeAlias`, no `NamedTuple`, no `pydantic.BaseModel` for these shapes). This is the worst case for AI readability — an LLM reading the code has zero schema hints and must guess the shape from usage at every call site.
|
||||
|
||||
**Scope is deliberately bounded.** The track adds **6 type aliases**, converts **2-3 tuple returns** to NamedTuples, and introduces the **type registry generator + initial generated docs**. It does NOT migrate to `TypedDict` or `@dataclass` schemas (the registry generator captures the field information in docs form, with much lower upfront cost). It does NOT touch the 23 lower-impact files; they remain as `dict[str, Any]` until a future track migrates them.
|
||||
|
||||
### 1.1 Why docs over TypedDict
|
||||
|
||||
The original draft of this spec proposed a follow-up track "TypedDict / dataclass Migration" that would convert every `Metadata` alias into a `TypedDict` with explicit fields. After user feedback, this was replaced with the type-registry approach for three reasons:
|
||||
|
||||
1. **Lower upfront cost.** `TypedDict` requires designing the schema for every type. The registry generator reads what already exists in code and writes it to docs. No schema design needed.
|
||||
2. **Better fit for AI workflow.** An LLM that needs to know the fields of `CommsLogEntry` can `cat docs/type_registry/ai_client.md` once, then use the field info. The cost is a few hundred tokens of context, paid only when the LLM needs the schema.
|
||||
3. **Auto-maintained.** The script runs as part of track completion and as a CI `--check`. The registry can never drift; if code changes, the agent regenerates the docs.
|
||||
|
||||
The "cost we eat" is the LLM reading the docs at query time. This is bounded (a few hundred tokens per query) and proportional to the actual information need.
|
||||
|
||||
## 2. Goals (Priority Order)
|
||||
|
||||
| Priority | Goal | Rationale |
|
||||
|---|---|---|
|
||||
| **A (primary value)** | Add 6 `TypeAlias` definitions to `src/type_aliases.py`: `Metadata`, `CommsLogEntry`, `CommsLog`, `FileItem`, `FileItems`, `HistoryMessage`. | Each alias names a concept that currently appears as `dict[str, Any]` or `list[dict[str, Any]]` in 30+ sites. The name is self-documenting; the underlying type is the same. |
|
||||
| **A (primary value)** | Mechanical replacement of 370+ weak sites in 6 files: `src/ai_client.py`, `src/app_controller.py`, `src/models.py`, `src/api_hook_client.py`, `src/project_manager.py`, `src/aggregate.py`. | The audit shows 86% of findings are in these 6 files. A focused refactor here eliminates the bulk of the noise. |
|
||||
| **B (architectural)** | The new aliases are the **canonical** names going forward. New code MUST use the aliases. Old code is migrated opportunistically (this track + future tracks). | One source of truth. The audit script (`scripts/audit_weak_types.py`) becomes a permanent CI gate that fails when new weak types are introduced. |
|
||||
| **B (architectural)** | Audit script exits 0 with significantly fewer findings after the refactor. Re-running `--json` should show the count drop from 430 to ~60 (only the 23 lower-impact files remain). | Measurable success criterion. The audit script is the ground truth. |
|
||||
| **C (optimization)** | Convert 2-3 tuple returns to `NamedTuple`s. Specifically: `_reread_file_items()` returns `Tuple[refreshed, changed]` becomes a `FileItemsDiff` NamedTuple. Other 1-occurrence tuples (screen coords, etc.) are converted opportunistically. | The tuple return pattern is rarer than the dict pattern (4 sites vs 430), but each conversion is high-value for self-documentation. |
|
||||
| **C (documentation)** | Add a short "Data Structure Conventions" section to `conductor/product-guidelines.md` and a new `conductor/code_styleguides/type_aliases.md` reference. | The convention is visible in the project-level guidance. Future plans reference it. |
|
||||
| **C (innovation)** | New `docs/type_registry/` directory with **auto-generated** documentation describing the fields of every `TypeAlias`, `NamedTuple`, `@dataclass`, and `TypedDict` in `src/`. New script `scripts/generate_type_registry.py` reads `src/` via AST and writes the docs. The script has a `--check` mode for CI: exits 1 if the registry would change. The coding agent runs the script as part of track completion. | The "docs over TypedDict" tradeoff: pay a small token cost at AI-query time (the LLM `cat`s the docs) instead of a large upfront cost (designing `TypedDict` schemas for every type). See §1.1. |
|
||||
| **D (forward-looking)** | Plan a future "Registry Maintenance" track that promotes the type-registry generation to a CI gate (fail if `--check` reports drift). The registry becomes part of every track's commit workflow. NOT in this track; documented in §12.1. | The track ships the registry; the future track wires it into CI / track-completion workflows. |
|
||||
|
||||
### 2.1 Non-Goals (this track)
|
||||
|
||||
- **Not** converting `dict[str, Any]` to `TypedDict` or `@dataclass` directly in code. The type registry (added in Phase 2) captures the field information in docs form; a future track may convert the most-used aliases to `TypedDict` (giving schema hints via type hints instead of via docs), but that is a separate decision.
|
||||
- **Not** touching the 23 lower-impact files. They stay as `dict[str, Any]` until a future incremental track migrates them. The audit script makes their weakness VISIBLE so the cost of ignoring them is documented.
|
||||
- **Not** changing the `Result[T]` pattern from the `data_oriented_error_handling_20260606` track. The aliases complement `Result`; they don't replace it. (`ErrorInfo` is a `@dataclass`, not a `TypeAlias`; it's already structured.)
|
||||
- **Not** adding pydantic models. The project doesn't currently use pydantic for these shapes; introducing it would be a much larger architectural decision.
|
||||
- **Not** modifying the data_oriented_error_handling_20260606 track's `src/result_types.py`. The aliases live in a new file (`src/type_aliases.py`); they coexist with `Result`/`ErrorInfo`.
|
||||
- **Not** changing the public API of any function. The aliases are TYPE-LEVEL ONLY; runtime behavior is identical.
|
||||
|
||||
## 3. Architecture
|
||||
|
||||
### 3.1 The Aliases
|
||||
|
||||
`src/type_aliases.py` (NEW, ~80 lines):
|
||||
|
||||
```python
|
||||
from typing import Any, Callable, TypeAlias
|
||||
|
||||
# A single key-value record. The shape is intentionally open (Any value type)
|
||||
# because different concepts use different value types (str for paths, int for
|
||||
# counts, dict for nested structures, etc.). The name documents the SEMANTIC
|
||||
# ROLE, not the structural shape.
|
||||
Metadata: TypeAlias = dict[str, Any]
|
||||
|
||||
# A single entry in the AI comms log (the in-memory ring buffer of API
|
||||
# requests/responses/timestamps/kind/direction). Used by _comms_log,
|
||||
# _append_comms, get_comms_log, comms_log_callback, etc.
|
||||
CommsLogEntry: TypeAlias = Metadata
|
||||
|
||||
# A list of comms log entries.
|
||||
CommsLog: TypeAlias = list[CommsLogEntry]
|
||||
|
||||
# A single entry in the Application's discussion (the UI-layer entry list
|
||||
# persisted to project TOML; see docs/guide_discussions.md §"Data Model").
|
||||
# Per the docs refresh (2026-06-08), this has at least 7 fields:
|
||||
# {role, content, collapsed, ts, thinking_segments?, usage?, read_mode?}.
|
||||
# Plus optional extras (e.g., tag, comment from custom slices).
|
||||
# Uses Metadata (dict[str, Any]) because the dict is intentionally OPEN —
|
||||
# extra keys are allowed and ignored by the renderer. The alias docstring
|
||||
# documents the minimum required keys, not the full schema.
|
||||
#
|
||||
# IMPORTANT (added 2026-06-08 per nagent_review Pitfall #4): this is the
|
||||
# UI/curation-layer history. It is *distinct* from ProviderHistoryMessage
|
||||
# below, which is the provider-side history (the bytes actually replayed
|
||||
# to the LLM). Conflating them perpetuates the provider-history-divergence
|
||||
# bug: user edits HistoryMessage.content via the discussion UI but
|
||||
# ProviderHistoryMessage.content is not updated. The follow-up
|
||||
# public_api_migration_20260606 track is the natural moment to unify.
|
||||
HistoryMessage: TypeAlias = Metadata
|
||||
|
||||
# A list of history messages.
|
||||
History: TypeAlias = list[HistoryMessage]
|
||||
|
||||
# Provider-side history entry: a single message passed to/from the LLM
|
||||
# SDK (OpenAI/Anthropic/Gemini/DeepSeek/etc.). Per the docs refresh and
|
||||
# the nagent_review (Pitfall #4), this is a DIFFERENT layer from
|
||||
# HistoryMessage. Shape: {role: "user"|"assistant"|"tool"|"system",
|
||||
# content: str | list[ContentBlock], tool_calls?: [...],
|
||||
# tool_call_id?: str, name?: str}. Aliased to Metadata for the same
|
||||
# reason HistoryMessage is (open shape; type aliases as semantic
|
||||
# names, not structural constraints). The distinction from
|
||||
# HistoryMessage is the alias name, not the underlying dict shape.
|
||||
ProviderHistoryMessage: TypeAlias = Metadata
|
||||
|
||||
# A list of provider history messages.
|
||||
ProviderHistory: TypeAlias = list[ProviderHistoryMessage]
|
||||
|
||||
# A single file item in the context. Per docs/guide_context_aggregation.md
|
||||
# §"The FileItem Schema (Full)" (added 2026-06-08), this is a 9-field
|
||||
# dataclass: {path, auto_aggregate, force_full, view_mode, selected,
|
||||
# ast_signatures, ast_definitions, ast_mask, custom_slices, injected_at}.
|
||||
# The alias does NOT point to Metadata — it points to the existing
|
||||
# models.FileItem class. This is the only alias in the 10 that is not
|
||||
# a dict alias; the others remain dict aliases for compatibility with
|
||||
# the FileItem.to_dict()/from_dict() round-trip.
|
||||
FileItem: TypeAlias = "models.FileItem" # type: ignore[misc]
|
||||
|
||||
# A list of file items. The most common weak pattern in the codebase.
|
||||
FileItems: TypeAlias = list[FileItem]
|
||||
|
||||
# A single tool definition (function name, description, parameters schema).
|
||||
# Used by _build_anthropic_tools, _CACHED_ANTHROPIC_TOOLS, _get_anthropic_tools,
|
||||
# and the corresponding openai-compatible / gemini / deepseek builders.
|
||||
ToolDefinition: TypeAlias = Metadata
|
||||
|
||||
# A single tool call from the model (id, type, function: {name, arguments}).
|
||||
# Used by response.tool_calls parsing across all providers.
|
||||
ToolCall: TypeAlias = Metadata
|
||||
|
||||
# A callback that receives a comms log entry. Used by comms_log_callback,
|
||||
# confirm_and_run_callback, etc.
|
||||
CommsLogCallback: TypeAlias = Callable[[CommsLogEntry], None]
|
||||
```
|
||||
|
||||
### 3.2 The NamedTuples (Phase 2)
|
||||
|
||||
`src/type_aliases.py` (continued):
|
||||
|
||||
```python
|
||||
from typing import NamedTuple
|
||||
|
||||
# Return type of _reread_file_items. The two lists are conceptually distinct:
|
||||
# refreshed = items whose mtime was checked and the content re-read; changed =
|
||||
# items whose content actually changed (subset of refreshed).
|
||||
class FileItemsDiff(NamedTuple):
|
||||
refreshed: FileItems
|
||||
changed: FileItems
|
||||
```
|
||||
|
||||
(Optional, if 1-2 more tuple returns warrant conversion — e.g., `Optional[Tuple[int, int, int, int]]` for screen coords, etc. — add them as separate `NamedTuple`s with semantic names.)
|
||||
|
||||
### 3.3 Why These Specific Aliases
|
||||
|
||||
The 6 aliases were chosen to be **concept-distinct**: each names a different semantic role that the code uses. Using the same name (`Metadata`) for all of them would collapse the semantic distinction; using 30 names would exceed the AI's vocabulary budget. 6 is the sweet spot:
|
||||
|
||||
| Alias | Semantic role | Distinct from |
|
||||
|---|---|---|
|
||||
| `Metadata` | generic key-value record | (root) |
|
||||
| `CommsLogEntry` | a single comms log entry | `HistoryMessage` (different lifecycle) |
|
||||
| `HistoryMessage` | a single AI provider history message | `CommsLogEntry` (different lifecycle) |
|
||||
| `FileItem` | a single file in the context | `ToolDefinition` (different shape: paths vs function specs) |
|
||||
| `ToolDefinition` | a single tool definition | `FileItem`, `ToolCall` |
|
||||
| `ToolCall` | a single tool call from the model | `ToolDefinition` (definition vs invocation) |
|
||||
|
||||
Some of these are aliased to `Metadata` (e.g., `CommsLogEntry: TypeAlias = Metadata`). This is intentional: Phase 2 can convert `Metadata` to a `TypedDict` (or split into per-concept `TypedDict`s) and the aliases continue to work without breaking changes. The aliases are STABLE NAMES; the underlying type can evolve.
|
||||
|
||||
### 3.4 Module Layout
|
||||
|
||||
```
|
||||
src/
|
||||
type_aliases.py # NEW: 6 TypeAliases + 1-3 NamedTuples
|
||||
ai_client.py # MODIFIED: import aliases; replace ~139 weak sites
|
||||
app_controller.py # MODIFIED: import aliases; replace ~86 weak sites
|
||||
models.py # MODIFIED: import aliases; replace ~51 weak sites
|
||||
api_hook_client.py # MODIFIED: import aliases; replace ~32 weak sites
|
||||
project_manager.py # MODIFIED: import aliases; replace ~20 weak sites
|
||||
aggregate.py # MODIFIED: import aliases; replace ~17 weak sites
|
||||
mcp_client.py # UNCHANGED (only 9 weak sites; below the threshold)
|
||||
|
||||
docs/
|
||||
type_registry/
|
||||
index.md # NEW (generated): top-level TOCs
|
||||
type_aliases.md # NEW (generated): the 10 TypeAliases + 1 NamedTuple
|
||||
ai_client.md # NEW (generated): per-source-file reference
|
||||
app_controller.md # NEW (generated)
|
||||
models.md # NEW (generated)
|
||||
api_hook_client.md # NEW (generated)
|
||||
project_manager.md # NEW (generated)
|
||||
aggregate.md # NEW (generated)
|
||||
result_types.md # NEW (generated): from data_oriented_error_handling_20260606
|
||||
|
||||
conductor/
|
||||
product-guidelines.md # MODIFIED: new "Data Structure Conventions" section
|
||||
code_styleguides/
|
||||
type_aliases.md # NEW: the canonical reference
|
||||
|
||||
scripts/
|
||||
audit_weak_types.py # already committed in 84fd9ac9; runs as CI gate
|
||||
generate_type_registry.py # NEW: AST-based registry generator
|
||||
|
||||
tests/
|
||||
test_type_aliases.py # NEW: verify the aliases import and resolve to the right types
|
||||
test_generate_type_registry.py # NEW: verify the generator's regex/AST patterns and output format
|
||||
(existing test files): # MODIFIED: update the 6 files; existing tests should pass unchanged
|
||||
```
|
||||
|
||||
### 3.5 Coexistence with `Result[T]` and `ErrorInfo`
|
||||
|
||||
The new `Metadata` family aliases are VALUE-LEVEL types (what's in a dict). The `Result[T]` from `data_oriented_error_handling_20260606` is a CONTROL-LEVEL wrapper (a data struct that includes errors). They compose:
|
||||
|
||||
```python
|
||||
# Data-oriented error handling returns:
|
||||
Result[CommsLogEntry] # a Result wrapping a single comms log entry
|
||||
Result[History] # a Result wrapping a list of history messages
|
||||
Result[FileItems] # a Result wrapping a list of file items
|
||||
|
||||
# The aliases name the "T" in Result[T], not the Result itself.
|
||||
```
|
||||
|
||||
This is consistent: `Result` is a generic that wraps any data type. Naming the data types (via `TypeAlias`) makes the generic concrete without changing the `Result` pattern.
|
||||
|
||||
### 3.6 Type Registry (Auto-Generated Docs)
|
||||
|
||||
`scripts/generate_type_registry.py` is a new AST-based tool that reads `src/` and writes `docs/type_registry/`. It runs as part of track completion (manually by the coding agent) and as a CI `--check` (automated).
|
||||
|
||||
**Output structure:**
|
||||
|
||||
```
|
||||
docs/type_registry/
|
||||
index.md # top-level: full table of contents + summary
|
||||
type_aliases.md # the 10 TypeAliases from src/type_aliases.py
|
||||
ai_client.md # per-source-file: all dataclasses, NamedTuples, TypeAliases defined or used here
|
||||
app_controller.md
|
||||
models.md
|
||||
api_hook_client.md
|
||||
project_manager.md
|
||||
aggregate.md
|
||||
...
|
||||
(one .md per source file that has structs)
|
||||
```
|
||||
|
||||
**Script behavior:**
|
||||
|
||||
```bash
|
||||
# Generate / regenerate the registry (default mode)
|
||||
python scripts/generate_type_registry.py
|
||||
|
||||
# Verify the registry is up-to-date (CI mode; exits 1 if drift)
|
||||
python scripts/generate_type_registry.py --check
|
||||
|
||||
# Dry run: print what would change without writing
|
||||
python scripts/generate_type_registry.py --diff
|
||||
```
|
||||
|
||||
**For each `@dataclass` in `src/`, the script writes a section like:**
|
||||
|
||||
```markdown
|
||||
## `src/models.py::Ticket`
|
||||
|
||||
**Kind:** `@dataclass`
|
||||
**Fields:**
|
||||
- `id: str` — unique ticket identifier
|
||||
- `title: str` — human-readable title
|
||||
- `status: str = "todo"` — current status
|
||||
- `priority: int = 0` — priority for queue ordering
|
||||
- `created_at: datetime.datetime` — when created
|
||||
- `dependencies: list[str] = field(default_factory=list)` — ticket IDs this depends on
|
||||
- `metadata: Metadata` — opaque key-value metadata (see type_aliases.md)
|
||||
```
|
||||
|
||||
(Note: docstrings on fields are extracted from the source to provide the "—" descriptions. Fields without docstrings are documented with their name only.)
|
||||
|
||||
**For each `TypeAlias`, the script writes a section like:**
|
||||
|
||||
```markdown
|
||||
## `src/type_aliases.py::CommsLogEntry`
|
||||
|
||||
**Kind:** `TypeAlias`
|
||||
**Resolves to:** `Metadata`
|
||||
**Used by:** `_comms_log`, `_append_comms`, `get_comms_log`, `comms_log_callback`, ...
|
||||
|
||||
**Note:** `CommsLogEntry` is a semantic alias for `Metadata`. For the canonical field semantics, see [`Metadata`](#metadata) (which is itself a generic `dict[str, Any]` until a future track converts it to a `TypedDict`).
|
||||
```
|
||||
|
||||
**For each `NamedTuple`, the script writes a section like:**
|
||||
|
||||
```markdown
|
||||
## `src/type_aliases.py::FileItemsDiff`
|
||||
|
||||
**Kind:** `NamedTuple`
|
||||
**Fields:**
|
||||
- `refreshed: FileItems` — items whose mtime was checked and content re-read
|
||||
- `changed: FileItems` — items whose content actually changed (subset of refreshed)
|
||||
```
|
||||
|
||||
**For each function that returns a structured type, the script documents the return type signature** (using `ast.unparse` on the return annotation).
|
||||
|
||||
### 3.7 Why Per-Source-File Docs (not one giant file)
|
||||
|
||||
A per-source-file layout matches the project's per-source-file guide structure (`docs/guide_ai_client.md`, `docs/guide_mcp_client.md`, etc.). The coding agent reads `docs/type_registry/ai_client.md` when working in `src/ai_client.py` — locality of reference. The `index.md` provides the cross-cutting view.
|
||||
|
||||
**The "token cost we eat" per LLM query is bounded:** a typical source file's registry is 200-500 lines of markdown. The LLM reads it once and caches the schema in context. Subsequent references to the same types don't re-fetch.
|
||||
|
||||
## 4. Per-File Refactor Plan
|
||||
|
||||
### 4.1 `src/ai_client.py` (139 sites — largest offender)
|
||||
|
||||
**Pattern:** `_anthropic_history: list[dict[str, Any]]` (and 5 sibling histories), `_comms_log: deque[dict[str, Any]]`, `get_comms_log -> list[dict[str, Any]]`, `_build_anthropic_tools -> list[dict[str, Any]]`, `_reread_file_items -> tuple[list[...], list[...]]`, etc.
|
||||
|
||||
**Refactor strategy:**
|
||||
- Replace all 79 `dict[str, Any]` / `Dict[str, Any]` with `Metadata` or the more specific alias.
|
||||
- Replace all 56 `list[dict[...]]` with `CommsLog` / `History` / `FileItems` / `ToolDefinitions` based on the SEMANTIC ROLE of the list.
|
||||
- 2 `Optional[List[Dict[...]]]` with `Optional[FileItems]` (the `_CACHED_ANTHROPIC_TOOLS` is an Optional[ToolDefinitions]).
|
||||
- 2 tuple-return literal returns: the `cast(...)` patterns in `_dispatch_tool`. Replace with `ToolCall` extraction.
|
||||
|
||||
**Naming heuristic:** for each list of dicts, look at the variable name + the function name to determine the semantic role. E.g., `_comms_log` → `CommsLog`; `_anthropic_history` → `History`; `_build_anthropic_tools` → `ToolDefinitions`; `_reread_file_items(file_items: list[...])` → `FileItems`.
|
||||
|
||||
### 4.2 `src/app_controller.py` (86 sites)
|
||||
|
||||
**Pattern:** `_pending_dialog: Optional[ConfirmDialog] = None` (stays as-is; this is a STRONG type already), `last_error: Optional[Dict[str, str]] = None` (could be `Optional[ErrorInfo]` from the data_oriented track), but most weak sites are in the `Hook API` request/response payloads and the `pre_tool_callback` family.
|
||||
|
||||
**Refactor strategy:**
|
||||
- The 62 `dict_str_any` sites: replace with `Metadata` or `CommsLogEntry` based on context.
|
||||
- The 20 `list_of_dict` sites: replace with the appropriate alias.
|
||||
- The 4 `optional_dict` sites: replace with `Optional[Metadata]` (or `Optional[CommsLogEntry]` if the context is the hook request payload).
|
||||
|
||||
### 4.3 `src/models.py` (51 sites)
|
||||
|
||||
**Pattern:** Dataclass fields. E.g., `script: Optional[str] = None` (stays as-is; STRONG), but also `target_file: Optional[str] = None` and many fields where the type is `Optional[Dict[str, Any]]` (in dataclass fields).
|
||||
|
||||
**Refactor strategy:** Replace 48 `dict_str_any` with `Optional[Metadata]`; 3 `list_of_dict` with the appropriate alias.
|
||||
|
||||
### 4.4 `src/api_hook_client.py` (32 sites)
|
||||
|
||||
**Pattern:** HTTP request/response payloads. E.g., `payload: Dict[str, Any]`, `data: dict[str, Any]`.
|
||||
|
||||
**Refactor strategy:** 30 `dict_str_any` → `Metadata`; 2 `list_of_dict` → `list[Metadata]`.
|
||||
|
||||
### 4.5 `src/project_manager.py` (20 sites)
|
||||
|
||||
**Pattern:** TOML config dicts. E.g., `proj: dict[str, Any]`, `data: dict[str, Any]`.
|
||||
|
||||
**Refactor strategy:** 16 `dict_str_any` → `Metadata`; 3 `list_of_dict` → `list[Metadata]`; 1 `optional_dict` → `Optional[Metadata]`.
|
||||
|
||||
### 4.6 `src/aggregate.py` (17 sites)
|
||||
|
||||
**Pattern:** Aggregation result dicts. E.g., `result: dict[str, list[dict[str, Any]]]`.
|
||||
|
||||
**Refactor strategy:** 10 `dict_str_any` → `Metadata`; 7 `list_of_dict` → appropriate alias.
|
||||
|
||||
### 4.7 Phase 2 NamedTuple conversions
|
||||
|
||||
- **`_reread_file_items`** in `src/ai_client.py` (returns `Tuple[List[FileItem], List[FileItem]]`) → returns `FileItemsDiff`. Affects ~3-4 call sites.
|
||||
- **1-2 screen-coord tuples** (1-occurrence each) — opportunistic. If the call site is clear and the names are obvious, convert; otherwise leave.
|
||||
|
||||
## 5. The Audit Script as a Permanent CI Gate
|
||||
|
||||
After this track, the audit script becomes a permanent CI gate. `scripts/audit_weak_types.py` exits 0 even when findings exist (it's informational). The CI gate uses a stricter mode:
|
||||
|
||||
```bash
|
||||
# New mode: --strict, exits 1 if any new weak site is added in a PR
|
||||
python scripts/audit_weak_types.py --strict
|
||||
```
|
||||
|
||||
The `--strict` mode compares the current count to a baseline (stored in `scripts/audit_weak_types.baseline.json`). If the current count is HIGHER than the baseline, exit 1. The baseline is regenerated after this track to the post-refactor count (~60 findings, only the 23 lower-impact files remain).
|
||||
|
||||
This is documented in the spec but the actual `--strict` mode is implemented as part of the track (Phase 1 final task). Future PRs that introduce new `dict[str, Any]` or anonymous tuples will fail CI.
|
||||
|
||||
## 6. Configuration
|
||||
|
||||
No new dependencies. No new environment variables. No new config files.
|
||||
|
||||
The aliases live in `src/type_aliases.py` (pure stdlib `typing.TypeAlias`).
|
||||
|
||||
## 7. Testing Strategy
|
||||
|
||||
| Test File | Purpose | Coverage Target |
|
||||
|---|---|---|
|
||||
| `tests/test_type_aliases.py` | Verify the aliases import; verify they resolve to the expected types; verify they compose with `Result[T]` (e.g., `Result[FileItems]` is a valid generic). | 100% |
|
||||
| `tests/test_audit_weak_types.py` | Verify the audit script's regex patterns are correct; verify the `Finding` dataclass is populated correctly; verify the report matches expectations. | 90% |
|
||||
| `tests/test_ai_client.py` (existing) | Verify no regressions after the 139-site replacement. | 100% (regression) |
|
||||
| `tests/test_app_controller.py` (existing) | Verify no regressions after the 86-site replacement. | 100% (regression) |
|
||||
| `tests/test_models.py` (existing) | Verify no regressions after the 51-site replacement. | 100% (regression) |
|
||||
| `tests/test_api_hook_client.py` (existing) | Verify no regressions after the 32-site replacement. | 100% (regression) |
|
||||
| `tests/test_project_manager.py` (existing) | Verify no regressions after the 20-site replacement. | 100% (regression) |
|
||||
| `tests/test_aggregate.py` (existing) | Verify no regressions after the 17-site replacement. | 100% (regression) |
|
||||
| `tests/test_mcp_client.py` (existing) | Verify no regressions. (mcp_client is unchanged but the aliases may be adopted opportunistically in Phase 1.5 if convenient.) | 100% (regression) |
|
||||
|
||||
**Mocking strategy:** Existing tests use `unittest.mock.patch`; no changes needed.
|
||||
|
||||
**Audit baseline check:** After Phase 1, the audit script should report 0 NEW findings (the count may go UP if a few sites were missed, but the trend is DOWN). After Phase 2, the count should be at or below the pre-track baseline minus 50 (the targeted reductions).
|
||||
|
||||
## 8. Migration / Rollout
|
||||
|
||||
| Phase | What | Risk |
|
||||
|---|---|---|
|
||||
| **Phase 1 — Aliases + 6-file replacement + audit baseline** | Add `src/type_aliases.py`. Add `tests/test_type_aliases.py`. Mechanical replacement in 6 files. Add `--strict` mode to the audit script. Generate the new baseline. | Medium. ~345 sites of mechanical replacement. Mitigated by existing test coverage. |
|
||||
| **Phase 2 — NamedTuples + type registry generator + initial docs + archive** | Convert 2-3 tuple returns to NamedTuples. Add `scripts/generate_type_registry.py` + the initial generated registry in `docs/type_registry/`. Add tests for the generator. Add `conductor/code_styleguides/type_aliases.md` and update `product-guidelines.md`. Manual smoke test. Archive the track. | Low. ~3-4 sites of tuple conversion. Generator is a self-contained AST tool. Docs-only changes. |
|
||||
|
||||
Each phase has its own checkpoint commit and git note.
|
||||
|
||||
## 9. Risks & Mitigations
|
||||
|
||||
| Risk | Likelihood | Impact | Mitigation |
|
||||
|---|---|---|---|
|
||||
| Mechanical replacement misses a few sites; the count doesn't drop as expected. | Medium | Low | The audit script is the source of truth. Re-run after Phase 1; investigate any anomalies. |
|
||||
| Renaming `dict[str, Any]` to `Metadata` (or another alias) changes how some tests introspect types (e.g., `isinstance(x, dict)`). | Low | Medium | The aliases are TYPE-LEVEL ONLY; at runtime, `Metadata` IS `dict[str, Any]` IS `dict`. `isinstance(x, dict)` continues to work. Test cases that use `get_type_hints()` may need updating; documented in the test plan. |
|
||||
| A future contributor adds a new `dict[str, Any]` and the audit script doesn't catch it. | Low | Low | The audit script's regex patterns are exhaustive for the current 430 findings. New patterns (e.g., a new `Mapping[str, Any]`) would be missed. The track documents the patterns the script knows; future contributions of new patterns warrant extending the script. |
|
||||
| The aliases conflict with the `Result[T]` and `ErrorInfo` from the data_oriented_error_handling track. | Low | Low | The aliases are VALUE-LEVEL (data types); `Result` and `ErrorInfo` are CONTROL-LEVEL (wrappers). They compose: `Result[FileItems]` is valid. No conflict. |
|
||||
| The 6-file mechanical replacement is too large to review in one PR. | Medium | Low | Phase 1 is split into 6 sub-tasks (one per file) in the plan, each with its own commit. Reviewers can review file-by-file. |
|
||||
| The 23 lower-impact files are NEVER migrated. | High | Low (acceptable) | The audit script stays in the codebase as a permanent CI gate. The cost of ignoring the 23 files is now VISIBLE. Future tracks can pick them up opportunistically. |
|
||||
| The `docs/type_registry/` docs drift from the actual code. | Medium | Medium (LLM reads stale info) | The `--check` mode of the generator exits 1 if the registry would change. The coding agent runs the generator before each track's commit. A follow-up track (`type_registry_ci_20260606`) will wire `--check` into CI. |
|
||||
|
||||
## 10. Out of Scope (Explicit)
|
||||
|
||||
- **TypedDict / @dataclass migration** of the `Metadata` family. The type registry (added in Phase 2) captures the field information in docs form, with much lower upfront cost than `TypedDict` migration. A future track MAY convert the most-used aliases to `TypedDict` (giving the AI schema hints via type hints instead of via docs); this is a separate decision.
|
||||
- **The 23 lower-impact files** (those with 1-9 weak sites each). Deferred; will be addressed opportunistically or in a future incremental track. **Note (added 2026-06-08):** this list is dominated by `src/gui_2.py` (26+ weak sites per `docs/guide_state_lifecycle.md` §"State Delegation" and §"Reset" — `_disc_entries_lock` references, `_last_ui_snapshot`, the `UISnapshot` capture/restore, the 30+ fields cleared in `_handle_reset_session`) and `src/mcp_client.py` (will be touched heavily by the parallel `mcp_architecture_refactor_20260606` track). The deferral is correct, but a *follow-up* track should explicitly call out gui_2.py and mcp_client.py as the next targets, rather than implying they're handled.
|
||||
- **Adding pydantic models.** Not requested; would be a much larger architectural decision.
|
||||
- **Changing function signatures at the runtime level.** The aliases are TYPE-LEVEL; runtime behavior is identical.
|
||||
- **Modifying `scripts/audit_weak_types.py`'s regex patterns.** The patterns are correct for the current findings. If new patterns emerge, a future track can extend the script.
|
||||
- **Migrating the data_oriented_error_handling_20260606 track's `src/result_types.py` aliases.** The 2 type-aliases modules are SEPARATE: `result_types.py` has `ErrorInfo` / `Result` / `ErrorKind`; `type_aliases.py` has `Metadata` / `CommsLog` / `FileItem` / etc. They don't overlap.
|
||||
|
||||
## 11. Open Questions
|
||||
|
||||
1. **The 6 aliases or 4?** The 6 listed in §3.1 are: `Metadata`, `CommsLogEntry`, `CommsLog`, `HistoryMessage`, `History`, `FileItem`, `FileItems`, `ToolDefinition`, `ToolCall`, `CommsLogCallback`. That's 10. Should we cut to 4-6 to minimize the AI vocabulary? (Proposal: keep all 10; they're each named for a distinct concept, and the 10 names are self-explanatory. The "vocabulary cost" is the same as adding 10 new function names to a module — well within normal Python codebase scale.)
|
||||
2. **Should `FileItem` and `ToolDefinition` be `TypedDict` from the start?** A `TypedDict` gives the AI field-level hints, not just a name. But introducing `TypedDict` requires knowing the FIELDS, which is a deeper semantic task. (Proposal: Phase 1 uses `TypeAlias = dict[str, Any]`; Phase 2 of a future track converts to `TypedDict`. Keeps the current track scope tight.)
|
||||
3. **Should the audit script enforce a count threshold (e.g., "no more than 100 weak sites total") or a per-file threshold (e.g., "no file may have more than 50 weak sites")?** (Proposal: per-file threshold is more actionable. A future PR that introduces 20 new `dict[str, Any]` in `foo.py` would fail even if the total count didn't increase.)
|
||||
|
||||
## 12. See Also
|
||||
|
||||
### 12.1 Follow-up Track (planned; not in this spec)
|
||||
|
||||
**"Registry Maintenance & CI Integration"** (`type_registry_ci_20260606` or similar) — promotes the type-registry generator from a manual track-completion step to a CI gate. The track:
|
||||
- Wires `python scripts/generate_type_registry.py --check` into CI; the PR fails if the registry is stale.
|
||||
- Adds the registry to the per-track commit workflow: the coding agent runs the generator before marking a track complete, and includes the registry diff in the commit.
|
||||
- Optionally adds a pre-commit hook that runs the generator and stages the diff.
|
||||
- The "Type Registry Maintenance" track is the natural follow-up. Prerequisites: this track (so the generator exists and is tested).
|
||||
|
||||
### 12.2 Project References
|
||||
|
||||
- `scripts/audit_weak_types.py` (already committed; `84fd9ac9`) — the audit that found 430 weak sites.
|
||||
- `docs/guide_testing.md` — test conventions.
|
||||
- `docs/guide_models.md` — the existing `models.py:510-559 FileItem` dataclass is the *concrete* class the new `FileItem` alias points to. Per the 2026-06-08 docs refresh, the FileItem schema (9 fields + `__post_init__` normalizer) is documented in `docs/guide_context_aggregation.md §"The FileItem Schema (Full)"`.
|
||||
- `docs/guide_context_aggregation.md` — added 2026-06-08. The `aggregate.py:142 build_file_items` function consumes the `FileItem` list; the `FileItems: TypeAlias` is the consumer-side type.
|
||||
- `docs/guide_discussions.md` — added 2026-06-08. The entry dict shape (the `HistoryMessage` alias) is documented here. The shape has at least 7 fields (`{role, content, collapsed, ts, thinking_segments?, usage?, read_mode?}`) plus optional extras. The alias docstring notes the dict is *open* — extra keys are allowed.
|
||||
- `docs/guide_state_lifecycle.md` — added 2026-06-08. The `App.__getattr__`/`__setattr__` state delegation (per `gui_2.py:666-675`) and the `UISnapshot` capture (`gui_2.py:735-789`) are the *correctness* the alias-typed code must preserve; aliases are TYPE-LEVEL ONLY and don't change runtime behavior.
|
||||
- `conductor/code_styleguides/error_handling.md` (created in the data_oriented_error_handling_20260606 track) — the convention for `Result` types; the new type-aliases convention lives alongside. The two conventions are *complementary*: aliases name the *data* (`T` in `Result[T]`); `Result` wraps the *control flow*. See §3.5 of the spec.
|
||||
- `conductor/product-guidelines.md` "Data-Oriented Error Handling" — the convention this track extends (Data Structure Strengthening is a new top-level convention in the same family).
|
||||
- `conductor/tracks/data_oriented_error_handling_20260606/` — the previous track that established the convention format; this track uses the same pattern. The new `ProviderHistoryMessage` alias (added 2026-06-08) is the *concrete manifestation* of nagent_review Pitfall #4 (provider-history divergence) — the user's edits to the `HistoryMessage` (UI layer) are a different layer from the `ProviderHistoryMessage` (SDK layer), and conflating them perpetuates the bug.
|
||||
- `conductor/tracks/mcp_architecture_refactor_20260606/` — the parallel major track. `mcp_client.py` is currently listed as "UNCHANGED (only 9 weak sites; below the threshold)" in the module layout, but the refactor will touch it heavily; the audit script should be re-run after the mcp refactor lands, and a follow-up type-aliases pass on mcp_client.py is the natural next target.
|
||||
- `conductor/tracks/nagent_review_20260608/report.md` — added 2026-06-08. §6 (per-file memory) and §15 Pitfall #4 (provider history divergence) directly motivate the `HistoryMessage` vs `ProviderHistoryMessage` split in §3.1 of this spec.
|
||||
- `conductor/tracks/nagent_review_20260608/nagent_takeaways_20260608.md` — added 2026-06-08. §9 (edit-the-input, not the output) describes the bug the new alias split addresses.
|
||||
|
||||
### 12.3 External References
|
||||
|
||||
- **Python `typing.TypeAlias`** — the canonical mechanism for type aliases (PEP 613, Python 3.10+).
|
||||
- **Python `typing.NamedTuple`** — for tuple-with-fields.
|
||||
- **Python `typing.TypedDict`** — for the future Phase 2 (not in this track).
|
||||
- **Mike Acton on data-oriented design** — the "data is the API" framing that motivates NAMING data structures clearly.
|
||||
- **Casey Muratori on module layer boundaries** — the convention that each module owns its data and exposes a clear interface.
|
||||
@@ -0,0 +1,95 @@
|
||||
# Track state for data_structure_strengthening_20260606
|
||||
# Updated by Tier 2 Tech Lead as tasks complete
|
||||
|
||||
[meta]
|
||||
track_id = "data_structure_strengthening_20260606"
|
||||
name = "Data Structure Strengthening (Type Aliases + NamedTuples)"
|
||||
status = "completed"
|
||||
current_phase = "complete"
|
||||
last_updated = "2026-06-21"
|
||||
|
||||
[phases]
|
||||
phase_1 = { status = "completed", checkpointsha = "794ca91d", name = "Aliases + 6-file replacement + audit baseline" }
|
||||
phase_2 = { status = "completed", checkpointsha = "d3205c72", name = "NamedTuples + type registry generator + initial docs + archive" }
|
||||
|
||||
[tasks]
|
||||
# Phase 1: Aliases + 6-file replacement
|
||||
t1_1 = { status = "completed", commit_sha = "see_git_log", description = "Red: tests/test_type_aliases.py (verify 10 TypeAliases + 1 NamedTuple import and resolve to expected types; verify Result[FileItems] composes)" }
|
||||
t1_2 = { status = "completed", commit_sha = "see_git_log", description = "Green: create src/type_aliases.py with 10 TypeAliases (Metadata, CommsLogEntry, CommsLog, HistoryMessage, History, FileItem, FileItems, ToolDefinition, ToolCall, CommsLogCallback) and 1 NamedTuple (FileItemsDiff)" }
|
||||
t1_3 = { status = "completed", commit_sha = "see_git_log", description = "Replace 139 weak sites in src/ai_client.py with the new aliases (79 dict_str_any + 56 list_of_dict + 2 Optional[List[Dict]] + 2 assign_tuple_literal)" }
|
||||
t1_4 = { status = "completed", commit_sha = "see_git_log", description = "Replace 86 weak sites in src/app_controller.py (62 dict_str_any + 20 list_of_dict + 4 optional_dict)" }
|
||||
t1_5 = { status = "completed", commit_sha = "see_git_log", description = "Replace 51 weak sites in src/models.py (48 dict_str_any + 3 list_of_dict)" }
|
||||
t1_6 = { status = "completed", commit_sha = "see_git_log", description = "Replace 32 weak sites in src/api_hook_client.py (30 dict_str_any + 2 list_of_dict)" }
|
||||
t1_7 = { status = "completed", commit_sha = "see_git_log", description = "Replace 20 weak sites in src/project_manager.py (16 dict_str_any + 3 list_of_dict + 1 optional_dict)" }
|
||||
t1_8 = { status = "completed", commit_sha = "see_git_log", description = "Replace 17 weak sites in src/aggregate.py (10 dict_str_any + 7 list_of_dict)" }
|
||||
t1_9 = { status = "completed", commit_sha = "see_git_log", description = "Add --strict mode to scripts/audit_weak_types.py (compares current count to baseline file; exits 1 if increased)" }
|
||||
t1_10 = { status = "completed", commit_sha = "see_git_log", description = "Generate scripts/audit_weak_types.baseline.json with the post-Phase-1 count" }
|
||||
t1_11 = { status = "completed", commit_sha = "see_git_log", description = "Red: tests/test_audit_weak_types.py (verify regex patterns, Finding dataclass, report format)" }
|
||||
t1_12 = { status = "completed", commit_sha = "see_git_log", description = "Run full test suite; confirm no regressions in 6 refactored files" }
|
||||
t1_13 = { status = "completed", commit_sha = "see_git_log", description = "Run audit; confirm count dropped from 430 to ~60; commit the new baseline" }
|
||||
t1_14 = { status = "completed", commit_sha = "see_git_log", description = "Phase 1 checkpoint commit + git note" }
|
||||
# Phase 2: NamedTuples + type registry generator + initial docs + archive
|
||||
t2_1 = { status = "completed", commit_sha = "see_git_log", description = "Convert src/ai_client.py:_reread_file_items to return FileItemsDiff NamedTuple (replaces Tuple[List[FileItem], List[FileItem]]); update ~3-4 call sites" }
|
||||
t2_2 = { status = "completed", commit_sha = "see_git_log", description = "Opportunistic NamedTuple conversions for 1-2 more tuple returns (screen coords, etc.)" }
|
||||
t2_3 = { status = "completed", commit_sha = "see_git_log", description = "Red: tests/test_generate_type_registry.py (verify AST extraction of @dataclass, NamedTuple, TypeAlias; verify output markdown structure)" }
|
||||
t2_4 = { status = "completed", commit_sha = "see_git_log", description = "Green: implement scripts/generate_type_registry.py (3 modes: default, --check, --diff)" }
|
||||
t2_5 = { status = "completed", commit_sha = "see_git_log", description = "Run the generator; commit the initial docs/type_registry/ (index.md + per-source-file .md files)" }
|
||||
t2_6 = { status = "completed", commit_sha = "see_git_log", description = "Verify --check mode: introduce a fake change in src/type_aliases.py, run --check, confirm exit 1" }
|
||||
t2_7 = { status = "completed", commit_sha = "see_git_log", description = "Create conductor/code_styleguides/type_aliases.md (canonical reference for the alias convention; 5 patterns + decision tree + examples)" }
|
||||
t2_8 = { status = "completed", commit_sha = "see_git_log", description = "Add 'Data Structure Conventions' section to conductor/product-guidelines.md (referencing the new styleguide)" }
|
||||
t2_9 = { status = "completed", commit_sha = "see_git_log", description = "Manual smoke test: launch GUI; verify type aliases don't break anything; verify audit --strict mode; verify generator --check mode" }
|
||||
t2_10 = { status = "completed", commit_sha = "see_git_log", description = "Phase 2 checkpoint commit + git note (TRACK COMPLETE)" }
|
||||
t2_11 = { status = "completed", commit_sha = "see_git_log", description = "git mv conductor/tracks/data_structure_strengthening_20260606 to conductor/tracks/archive/" }
|
||||
t2_12 = { status = "completed", commit_sha = "see_git_log", description = "Update conductor/tracks.md: move entry to Recently Completed" }
|
||||
t2_13 = { status = "completed", commit_sha = "see_git_log", description = "Final state.toml update: mark all phases completed; add follow-up track type_registry_ci_20260606 placeholder" }
|
||||
|
||||
[verification]
|
||||
# Filled as phases complete
|
||||
phase_1_aliases_module_complete = true
|
||||
phase_1_ai_client_refactored = true
|
||||
phase_1_app_controller_refactored = true
|
||||
phase_1_models_refactored = true
|
||||
phase_1_api_hook_client_refactored = true
|
||||
phase_1_project_manager_refactored = true
|
||||
phase_1_aggregate_refactored = true
|
||||
phase_1_audit_strict_mode_added = true
|
||||
phase_1_baseline_committed = true
|
||||
phase_2_file_items_diff_named_tuple = true
|
||||
phase_2_opportunistic_named_tuples = true
|
||||
phase_2_styleguide_written = true
|
||||
phase_2_product_guidelines_updated = true
|
||||
phase_2_smoke_test_passed = true
|
||||
phase_2_track_archived = true
|
||||
full_test_suite_passes = true
|
||||
no_new_optional_introduced = true
|
||||
audit_count_dropped_to_60 = true
|
||||
|
||||
[audit_count_progression]
|
||||
# Filled as tasks complete
|
||||
baseline = 430
|
||||
after_ai_client = 291
|
||||
after_app_controller = 205
|
||||
after_models = 154
|
||||
after_api_hook_client = 122
|
||||
after_project_manager = 102
|
||||
after_aggregate = 85
|
||||
phase_1_checkpoint_committed = 794ca91d
|
||||
phase_2_checkpoint_committed = d3205c72
|
||||
|
||||
[files_refactored]
|
||||
ai_client = { weak_sites_before = 139, weak_sites_after = 0, status = "completed" }
|
||||
app_controller = { weak_sites_before = 86, weak_sites_after = 0, status = "completed" }
|
||||
models = { weak_sites_before = 51, weak_sites_after = 0, status = "completed" }
|
||||
api_hook_client = { weak_sites_before = 32, weak_sites_after = 0, status = "completed" }
|
||||
project_manager = { weak_sites_before = 20, weak_sites_after = 0, status = "completed" }
|
||||
aggregate = { weak_sites_before = 17, weak_sites_after = 0, status = "completed" }
|
||||
|
||||
[typed_dict_migration_followup]
|
||||
track_id = "type_registry_ci_20260606"
|
||||
status = "planned_in_data_structure_strengthening_20260606"
|
||||
goal = "Promote the type-registry generator from a manual track-completion step to a CI gate. Add --check to CI; wire pre-commit hook; document the per-track commit workflow."
|
||||
note = "This follow-up REPLACES the earlier 'typed_dict_migration' follow-up. Per user feedback (2026-06-06), the registry approach (docs) is preferred over TypedDict migration (code) for the foreseeable future."
|
||||
|
||||
[public_api_migration_followup]
|
||||
# From the data_oriented_error_handling track
|
||||
note = "This track does not depend on or block the public_api_migration_20260606 track. They are independent."
|
||||
Reference in New Issue
Block a user