ed/manual_slop
Private
Public Access
0
0
Fork 0
Code Activity

docs: fix 6 contradictions from CONTRADICTIONS_REPORT_20260627 (C5/C6/C17/C19/C2)

Browse Source 
Six fixes for the c11_python doc sync (chronology row 3):

- C5 (Result notation): Result[str, ErrorInfo] -> Result[str] at
  docs/guide_ai_client.md lines 452 + 469; also error_handling.md
  line 801 (historical deprecation section).
- C6 (RAGChunk schema): docs/guide_models.md lines 343-349 corrected
  to match src/rag_engine.py:19-25 (id, document, path, score, metadata).
- C17 (type_aliases.md table): rewrote alias table to reflect post-2026-06-25
  reality (Metadata is @dataclass(frozen=True, slots=True) with 36 fields;
  11 per-aggregate dataclasses listed with source locations; removed
  stale 'underlying type is dict[str, Any]' claim at line 73 + the
  'keep Metadata as dict[str, Any]' claim at line 81).
- C19 (OBLITERATE principle): added 'OBLITERATE Principle' section to
  error_handling.md after Migration Playbook; clarified in Hard Rules
  that argument types that may be None (caller choice) are NOT banned.
- C2 (audit script name): docs/AGENTS.md references updated to point
  to scripts/audit_optional_returns.py (the all-src/ successor to
  scripts/audit_optional_in_3_files.py).

Also: docs/reports/CONTRADICTIONS_REPORT_20260627.md — the contradictions
index that drives these fixes. Kept for reference.

C16 + C18 were already addressed in commit 770c2fdb (python.md §10
Documented Exceptions table + §17.10 audit inventory).
This commit is contained in:
ed
2026-06-26 09:24:38 -04:00
parent 770c2fdb32
commit a101d34656
6 changed files with 426 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
conductor/code_styleguides/error_handling.md
+74 -12
View File
@@ -209,16 +209,23 @@ The 3 refactored subsystems demonstrate each pattern in context:
---
## Hard Rules (enforced in the 3 refactored files)
## Hard Rules (enforced in all `src/*.py` as of 2026-06-27)
These are non-negotiable in `src/mcp_client.py`, `src/ai_client.py`, and
`src/rag_engine.py`:
These are non-negotiable in all `src/*.py` files. The migration-target
files (14 of them) were historically not enforced; as of 2026-06-27 the
`scripts/audit_optional_in_baseline_files.py --strict` audit (renamed
from `_in_3_files.py` per the contradictions report) covers all
`src/*.py`, and the `cruft_elimination_20260627` track documents the
remaining work to bring the 14 migration-target files into compliance.
- **`Optional[T]` return types are FORBIDDEN** in the 3 refactored files. Use
- **`Optional[T]` return types are FORBIDDEN** in all `src/*.py`. Use
`Result[T]` (with `NIL_T` singleton if needed) instead. Rationale:
`Optional[T]` is the sum type `Union[T, None]` that Fleury's framework
replaces. Mixing the two patterns reintroduces the bifurcation the
convention is designed to remove.
- Argument types that may be `None` (e.g., `rag_engine: Optional[Any] = None`)
remain allowed; they describe a caller choice, not a runtime failure
of this function. Only `Optional[T]` *return* types are banned.
- **Function return types must be `Result[T]` for any function that can fail
at runtime.** A function that can't fail (e.g., `get_name() -> str`)
doesn't need a `Result`. The classification is "can this return a different
@@ -230,9 +237,12 @@ These are non-negotiable in `src/mcp_client.py`, `src/ai_client.py`, and
`try/except` is reserved for converting `OSError`, `PermissionError`, and
similar I/O exceptions to `ErrorInfo` at the mcp_client tool boundary.
The verification script `scripts/audit_optional_in_3_files.py` enforces the
`Optional[X]` rule by failing CI if any new `Optional[X]` appears in the 3
refactored files.
The verification script `scripts/audit_optional_returns.py` enforces the
`Optional[X]` rule by failing CI if any new `Optional[X]` return type
appears in any `src/*.py` file. (As of 2026-06-27 this is the successor to
`scripts/audit_optional_in_3_files.py`, which covered only 4 baseline files;
the new script scans all `src/*.py` per the cruft_elimination_20260627
expansion of the ban.)
### `Optional[X]` in argument types
@@ -790,6 +800,58 @@ When converting existing code:
---
## The OBLITERATE Principle (Result Migration Anti-Pattern)
**Added 2026-06-27** (from `result_migration_cruft_removal_20260620`).
When a function is migrated from `Optional[T]` / `raise` to `Result[T]`:
- **NO pass-throughs.** Do NOT keep a legacy wrapper like `def _x(): return _x_result(...).data`. The wrapper is dead code the moment the migration lands.
- **NO backward compat.** Do NOT keep the old return type alongside the new one. Pick one (the new `Result[T]`), and delete the other.
- **In-site callers rewritten in the same atomic commit.** Every caller of the migrated function must be updated to use `result.ok` / `result.errors` / `result.data` directly. No deprecation period. No "we'll fix it later."
- **The dead code dies.** Legacy `def _x_result_to_x(...)` shims, `_x_result()` passthrough helpers, and conditional return-type guards must be deleted in the same commit that introduces `Result[T]`. Leaving them creates two equivalent APIs that future agents must disambiguate.
### The wrong pattern (pass-through that should be obliterated)
```python
# BEFORE (the legacy):
def do_thing() -> Optional[str]:
result = do_thing_result()
if not result.ok: return None
return result.data
# AFTER (the new):
def do_thing_result() -> Result[str]:
...
```
The `do_thing` function must be **deleted**, not kept as a wrapper. Keep only one entry point: `do_thing_result()`.
### The right pattern (single canonical entry point)
```python
# After OBLITERATE: only do_thing_result exists
def do_thing_result() -> Result[str]:
...
```
Callers are rewritten:
```python
# BEFORE:
result = do_thing()
if result is None: handle_failure()
# AFTER:
result = do_thing_result()
if not result.ok: handle_failure(result.errors)
```
### Why this rule
The `result_migration_cruft_removal_20260620` track ended with 9 legacy wrappers across 4 files (`mcp_client`, `ai_client`, `rag_engine`, `gui_2`). The wrappers were dead code that added visual noise, broke `mypy --strict`, and required every new caller to decide which path to use. Removing them required `Phase 9: LEGACY_WRAPPER_OBLITERATION` as an explicit step — that step should never have been necessary. **Don't ship pass-through wrappers in the first place.**
---
## Historical deprecation (added 2026-06-15, reverted 2026-06-16)
The public `ai_client.send()` was briefly marked `@deprecated` in favor of
@@ -798,7 +860,7 @@ The public `ai_client.send()` was briefly marked `@deprecated` in favor of
reverted on 2026-06-16 by `send_result_to_send_20260616` after the
Tier 2 autonomous sandbox proved capable of doing the rename safely.
`ai_client.send(...) -> Result[str, ErrorInfo]` is the canonical public API.
`ai_client.send(...) -> Result[str]` (with `errors: list[ErrorInfo]` as a side-channel field) is the canonical public API.
No deprecation is in effect. For the historical record of the brief
deprecation cycle, see
`conductor/tracks/public_api_migration_and_ui_polish_20260615/spec.md`
@@ -881,10 +943,10 @@ When writing NEW code, you MUST:
When writing NEW code, you MUST NOT:
1. **DO NOT use `Optional[T]` as a return type** (in any file in
`src/mcp_client.py`, `src/ai_client.py`, `src/rag_engine.py` —
the 3 refactored files). Use `Result[T]` instead. CI fails if
you add a new `Optional[T]` to those files (enforced by
`scripts/audit_optional_in_3_files.py`).
`src/`). Use `Result[T]` instead. CI fails if you add a new
`Optional[T]` return type to any `src/*.py` (enforced by
`scripts/audit_optional_in_baseline_files.py --strict`,
which scans all `src/*.py` as of 2026-06-27).
2. **DO NOT use `Optional[T]` as a return type** (anywhere else in
`src/`). The convention is migrating to `Result[T]`; new code
conductor/code_styleguides/type_aliases.md
+30 -16
View File
@@ -12,20 +12,34 @@ Reference: the audit script `scripts/audit_weak_types.py` is the ground truth. T
## The 10 Aliases (the canonical set)
`src/type_aliases.py` defines 10 `TypeAlias`es + 1 `NamedTuple`:
**Updated 2026-06-27** to reflect the post-`metadata_promotion_20260624` / `cruft_elimination_20260627` reality:
`Metadata` is no longer `dict[str, Any]`; it is now `@dataclass(frozen=True, slots=True)` with explicit fields.
The per-aggregate aliases (`CommsLogEntry`, `HistoryMessage`, `ToolDefinition`, `SessionInsights`, `DiscussionSettings`, `CustomSlice`, `MMAUsageStats`, `ProviderPayload`, `UIPanelConfig`, `PathInfo`) are `@dataclass(frozen=True)` types defined in `src/type_aliases.py`.
`FileItem` and `ToolCall` are forward-reference `TypeAlias` strings pointing to types defined in `src/models.py` and `src/openai_schemas.py` respectively (avoids circular imports).
`RAGChunk` is the 11th dataclass — it lives in `src/rag_engine.py` (not in `type_aliases.py`) because it's tightly coupled to the RAG engine's chunking logic.
| Alias | Resolves to | Semantic role |
`src/type_aliases.py` defines 10 `TypeAlias`es + 11 dataclasses + 1 `NamedTuple` (12 total aggregate types):
| Alias / Dataclass | Source | Semantic role |
|---|---|---|
| `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 (UI-layer) |
| `History` | `list[HistoryMessage]` | The conversation history |
| `FileItem` | `Metadata` | A single file in the context (path, content, view_mode, etc.) |
| `FileItems` | `list[FileItem]` | The most common weak pattern in the codebase |
| `ToolDefinition` | `Metadata` | A single tool definition (name, description, parameters schema) |
| `ToolCall` | `Metadata` | A single tool call from the model (id, type, function) |
| `CommsLogCallback` | `Callable[[CommsLogEntry], None]` | The callback signature for comms log updates |
| `Metadata` | `@dataclass(frozen=True, slots=True)` in `type_aliases.py` (36 fields) | The boundary type at the wire (TOML/JSON parse). Dict-compat methods (`__getitem__`, `get`, etc.) keep legacy call sites working. |
| `CommsLogEntry` | `@dataclass(frozen=True)` in `type_aliases.py` (8 fields) | A single entry in the AI comms log |
| `CommsLog` | `TypeAlias = list[CommsLogEntry]` | The comms log ring buffer |
| `HistoryMessage` | `@dataclass(frozen=True)` in `type_aliases.py` (6 fields) | A single message in the AI provider history (UI-layer) |
| `History` | `TypeAlias = list[HistoryMessage]` | The conversation history |
| `FileItem` | `TypeAlias = "models.FileItem"` | A single file in the context (path, content, view_mode, etc.) — defined in `src/models.py` |
| `FileItems` | `TypeAlias = list[FileItem]` | The most common weak pattern in the codebase |
| `ToolDefinition` | `@dataclass(frozen=True)` in `type_aliases.py` (4 fields) | A single tool definition (name, description, parameters schema) |
| `ToolCall` | `TypeAlias = "openai_schemas.ToolCall"` | A single tool call from the model (id, type, function) — defined in `src/openai_schemas.py` |
| `SessionInsights` | `@dataclass(frozen=True)` in `type_aliases.py` (6 fields) | Session-level token/cost metrics |
| `DiscussionSettings` | `@dataclass(frozen=True)` in `type_aliases.py` (3 fields) | Per-discussion generation params |
| `CustomSlice` | `@dataclass(frozen=True)` in `type_aliases.py` (4 fields) | A Fuzzy Anchor slice definition |
| `MMAUsageStats` | `@dataclass(frozen=True)` in `type_aliases.py` (3 fields) | Per-tier input/output token counter |
| `ProviderPayload` | `@dataclass(frozen=True)` in `type_aliases.py` (4 fields) | The payload sent to a provider (script, args, output, source_tier) |
| `UIPanelConfig` | `@dataclass(frozen=True)` in `type_aliases.py` (3 fields) | Per-window separator flags |
| `PathInfo` | `@dataclass(frozen=True)` in `type_aliases.py` (3 fields) | Paths config (logs_dir, scripts_dir, project_root) |
| `RAGChunk` | `@dataclass(frozen=True)` in `rag_engine.py` (5 fields: id, document, path, score, metadata) | A single RAG result chunk |
| `CommsLogCallback` | `TypeAlias = Callable[[CommsLogEntry], None]` | The callback signature for comms log updates |
Plus the NamedTuple:
@@ -70,17 +84,17 @@ def append_comms(entry: CommsLogEntry) -> None: ...
def get_history() -> History: ...
```
The underlying type is still `dict[str, Any]`; the alias name is the documentation.
**Updated 2026-06-27** — `Metadata` is itself a `@dataclass(frozen=True, slots=True)` with 36 explicit fields covering the wire schema. It is NOT a `TypeAlias = dict[str, Any]` anymore. The aliases below (e.g., `CommsLogEntry`, `HistoryMessage`) point to their own per-aggregate dataclasses, not to `Metadata`. The original "names for shapes" pattern has been promoted to the structural level (per §2.5).
### 2.5. When the role has stable distinct fields, promote it to its OWN dataclass
**Added 2026-06-25 (correction to `metadata_promotion_20260624`).** When a sub-aggregate has a known set of stable, distinct fields (e.g., `CommsLogEntry` has `ts, role, kind, direction, model, source_tier, content, error`; `FileItem` has `path, view_mode, custom_slices`; `RAGChunk` has `document, path, score`), promote it to its OWN `@dataclass(frozen=True, slots=True)` with its OWN fields. Do **NOT** share one mega-dataclass across multiple concepts.
**Added 2026-06-25 (correction to `metadata_promotion_20260624`).** When a sub-aggregate has a known set of stable, distinct fields (e.g., `CommsLogEntry` has `ts, role, kind, direction, model, source_tier, content, error`; `FileItem` has `path, view_mode, custom_slices`; `RAGChunk` has `id, document, path, score, metadata`), promote it to its OWN `@dataclass(frozen=True, slots=True)` with its OWN fields. Do **NOT** share one mega-dataclass across multiple concepts.
**Why:** the per-aggregate dataclass is the "names for shapes" pattern extended to the structural level. Each concept gets its own type, its own fields, its own `to_dict()` / `from_dict()` round-trip. Consumers use direct field access (`entry.ts`, `t.depends_on`, `chunk.document`) which compiles to a single C-level field read with 0 branches.
**When NOT to promote:** when the shape is genuinely unknown at type level (TOML project config, generic JSON parsing at a wire boundary, polymorphic log dumping). These are **collapsed codepaths** and they keep `Metadata: TypeAlias = dict[str, Any]` as the catch-all.
**When NOT to promote:** when the shape is genuinely unknown at type level and the fields are heterogeneous (e.g., log entries from 5 different vendors with mutually-exclusive keys). Use `Metadata: Metadata` (the dataclass) as the catch-all — its 36 explicit fields cover the common wire schema, and its dict-compat methods allow ad-hoc keys for vendor-specific extensions. Do NOT use `dict[str, Any]` directly anywhere; `Metadata` is the typed replacement.
**Canonical pattern (from `src/openai_schemas.py` and `src/models.py:533`):**
**Canonical pattern (from `src/openai_schemas.py` and `src/type_aliases.py`):**
```python
@dataclass(frozen=True, slots=True)