diff --git a/docs/reports/PLANNING_CORRECTION_metadata_promotion_20260625.md b/docs/reports/PLANNING_CORRECTION_metadata_promotion_20260625.md new file mode 100644 index 00000000..daa17878 --- /dev/null +++ b/docs/reports/PLANNING_CORRECTION_metadata_promotion_20260625.md @@ -0,0 +1,328 @@ +# Planning Correction: metadata_promotion_20260624 + +**Date:** 2026-06-25 +**Author:** Tier 1 (post-audit correction) +**Status:** SPEC + PLAN + METADATA.JSON corrected; styleguide clarified; awaiting commit +**Scope:** Removes the bad inference from the `metadata_promotion_20260624` track (the proposal to share one mega-dataclass across all 5 sub-aggregates) and replaces it with the per-aggregate dataclass design that the 2026-06-06 `data_structure_strengthening` spec originally anticipated. + +## TL;DR + +The original `metadata_promotion_20260624` track (committed `e50bebdd` on 2026-06-25) proposed: + +```python +@dataclass(frozen=True, slots=True) +class Metadata: + role: str = "" + content: Any = None + tool_calls: Any = None + tool_call_id: str = "" + name: str = "" + args: Any = None + source_tier: str = "main" + model: str = "unknown" + id: str = "" + ts: str = "" + role_: str = "" # For dicts that used 'role' as a key + description: str = "" + depends_on: tuple[str, ...] = () + status: str = "" + manual_block: bool = False + completed_tickets: int = 0 + auto_start: bool = False + command: str = "" + script: str = "" + output: Any = None + error: str = "" + tier: str = "" + path: str = "" + full_path: str = "" + filename: str = "" + mtime: float = 0.0 + size: int = 0 + # ... ~200 fields total, all Optional or with sensible defaults ... + +CommsLogEntry: TypeAlias = Metadata # BAD +CommsLog: TypeAlias = list[CommsLogEntry] +HistoryMessage: TypeAlias = Metadata # BAD +History: TypeAlias = list[HistoryMessage] +FileItem: TypeAlias = Metadata # BAD +FileItems: TypeAlias = list[FileItem] +ToolDefinition: TypeAlias = Metadata # BAD +ToolCall: TypeAlias = Metadata # BAD +``` + +This is **wrong**. The 5 sub-aggregates (`CommsLogEntry`, `HistoryMessage`, `FileItem`, `ToolDefinition`, `ToolCall`) are distinct concepts with distinct field sets. Lifting them into one mega-dataclass: + +1. **Hides the type information that direct field access is supposed to reveal.** A consumer that has a `Ticket` can read `.source_tier` (a `CommsLogEntry` field) and silently get the empty default. +2. **Is "less defined" than the current `dict[str, Any]` state.** Today, reading `.source_tier` on a `Ticket` raises `AttributeError` immediately. After the mega-dataclass, it silently returns `""`. +3. **Reverses the original 2026-06-06 design intent.** The `data_structure_strengthening_20260606` spec §3.3 explicitly anticipated per-concept promotion: *"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."* + +The corrected design promotes each known sub-aggregate to its OWN dataclass with its OWN fields. `Metadata: TypeAlias = dict[str, Any]` is preserved as the catch-all for **truly collapsed codepaths** (TOML project config, generic JSON parsing, polymorphic log dumping) only. + +## What was bad about the original inference + +### 1. The original spec proposed a single mega-dataclass with ~200 fields + +The original `metadata_promotion_20260624/spec.md` §FR1 defined: + +```python +@dataclass(frozen=True, slots=True) +class Metadata: + role: str = "" + content: Any = None + tool_calls: Any = None + tool_call_id: str = "" + name: str = "" + args: Any = None + source_tier: str = "main" + model: str = "unknown" + id: str = "" + ts: str = "" + role_: str = "" # For dicts that used 'role' as a key + description: str = "" + depends_on: tuple[str, ...] = () + status: str = "" + manual_block: bool = False + completed_tickets: int = 0 + auto_start: bool = False + command: str = "" + script: str = "" + output: Any = None + error: str = "" + tier: str = "" + path: str = "" + full_path: str = "" + filename: str = "" + mtime: float = 0.0 + size: int = 0 + # ... ~200 fields total, all Optional or with sensible defaults ... + +CommsLogEntry: TypeAlias = Metadata +CommsLog: TypeAlias = list[CommsLogEntry] +HistoryMessage: TypeAlias = Metadata +History: TypeAlias = list[HistoryMessage] +FileItem: TypeAlias = Metadata +FileItems: TypeAlias = list[FileItem] +ToolDefinition: TypeAlias = Metadata +ToolCall: TypeAlias = Metadata +``` + +This is the bad inference. The user complaint: + +> "If we have known sub-types they should be their own data class if they're not already, this doesn't make sense to lift them into a less defined moshpit, even with the data-oriented setup." + +The 200-field mega-dataclass IS the "less defined moshpit." It mashes 12+ distinct aggregates into one polymorphic type. + +### 2. The original spec's G3 explicitly mandated the bad pattern + +The original `metadata_promotion_20260624/spec.md` Goal G3: + +> "**G3**: All 5 sub-aggregates share the same dataclass (per type_aliases.py chain)." + +And the Out of Scope: + +> "The 5 sub-aggregates (CommsLogEntry, HistoryMessage, FileItem, ToolDefinition, ToolCall) becoming separate dataclasses each (overkill; they share the same Metadata base)" + +The user complaint: + +> "All 5 sub-aggregates share the same dataclass (per type_aliases.py chain) Is not a good thing todo." + +The original spec's G3 + Out of Scope are direct contradictions of the user's intent. Both are rewritten in the corrected spec. + +### 3. The original spec's 213 access sites actually span 12+ distinct aggregates + +A sampling of the actual access patterns in `src/` (from `git grep -E "\.get\('[a-z_]+',"`): + +| Access pattern | Aggregate it actually represents | +|---|---| +| `item.get('custom_slices', [])`, `item.get('content', '')` | **FileItem** | +| `fi.get('path', 'attachment')` | **FileItem** | +| `chunk.get('document', '')` | **RAGChunk** | +| `entry.get('source_tier', 'main')`, `entry.get('model', 'unknown')` | **CommsLogEntry** | +| `u.get('input_tokens', 0)`, `u.get('output_tokens', 0)` | **UsageStats** | +| `t.get('id', '')`, `t.get('depends_on', [])`, `t.get('manual_block', False)`, `t.get('status')` | **Ticket** | +| `stats.get('model', 'unknown')`, `stats.get('input', 0)`, `stats.get('output', 0)` | **MMAUsageStats** | +| `insights.get('total_tokens', 0)`, `insights.get('call_count', 0)`, `insights.get('burn_rate', 0)`, `insights.get('session_cost', 0)`, `insights.get('completed_tickets', 0)`, `insights.get('efficiency', 0)` | **SessionInsights** | +| `entry.get('temperature', 0.7)`, `entry.get('top_p', 1.0)`, `entry.get('max_output_tokens', 0)` | **DiscussionSettings** | +| `slc.get('tag', '')`, `slc.get('comment', '')` | **CustomSlice** | +| `preset.get('files', [])`, `preset.get('screenshots', [])` | **ContextPreset** | +| `payload.get('script')`, `payload.get('args', {})`, `payload.get('output', '')`, `payload.get('content', '')` | **ProviderPayload** | +| `self.project.get('paths', {})`, `self.project.get('conductor', {})`, `self.project.get('context_presets', {})` | **ProjectConfig** (TRULY collapsed codepath) | +| `gui_cfg.get('separate_message_panel', False)`, `gui_cfg.get('separate_response_panel', False)`, `gui_cfg.get('separate_tool_calls_panel', False)` | **UIPanelConfig** | +| `self.project.get('discussion', {}).get('discussions', {})` | **DiscussionStore** | +| `path_info['logs_dir']['path']` | **PathInfo** (nested) | + +There is no single "Metadata" shape. The 107 `.get()` sites access ~12 distinct aggregates. The original spec's mega-dataclass tried to force them all into one type — that IS the "less defined moshpit." + +### 4. The corrected design follows the canonical pattern already in production + +`src/openai_schemas.py` defines **5 separate frozen dataclasses**: + +- `ToolCallFunction` (2 fields: `name, arguments`) +- `ToolCall` (3 fields: `id, function, type`) +- `ChatMessage` (5 fields: `role, content, tool_calls, tool_call_id, name`) +- `UsageStats` (4 fields: `input_tokens, output_tokens, cache_read_tokens, cache_creation_tokens`) +- `NormalizedResponse` (4 fields: `text, tool_calls, usage, raw_response`) + +`src/models.py` defines **4 more separate frozen dataclasses**: + +- `Ticket` (15 fields: `id, description, target_symbols, context_requirements, depends_on, status, assigned_to, priority, target_file, blocked_reason, step_mode, retry_count, manual_block, model_override, persona_id`) +- `FileItem` (10 fields: `path, auto_aggregate, force_full, view_mode, selected, ast_signatures, ast_definitions, ast_mask, custom_slices, injected_at`) with paired `to_dict()` / `from_dict()` +- `Track` (3 fields: `id, description, tickets`) +- `TrackState` (3 fields: `metadata, discussion, tasks`) + +These are the **canonical reference pattern**. They are not shared mega-dataclasses; they are per-aggregate frozen dataclasses with their own fields. The corrected `metadata_promotion_20260624` spec continues in this direction. + +## What the corrected design is + +### Per-aggregate dataclasses (each its own type with its own fields) + +| Class | Module | Fields | Reused vs NEW | +|---|---|---:|---| +| `Ticket` | `src/models.py:302` | 15 | REUSED | +| `FileItem` | `src/models.py:533` | 10 | REUSED | +| `ContextPreset` | `src/models.py:932` (extended) | 3+ | REUSED + EXTENDED | +| `ToolCall` | `src/openai_schemas.py:32` | 3 | REUSED | +| `ToolCallFunction` | `src/openai_schemas.py:26` | 2 | REUSED | +| `ChatMessage` | `src/openai_schemas.py:48` | 5 | REUSED | +| `UsageStats` | `src/openai_schemas.py:68` | 4 | REUSED | +| `NormalizedResponse` | `src/openai_schemas.py:78` | 4 | REUSED | +| `CommsLogEntry` | `src/type_aliases.py` (NEW) | 8 | NEW | +| `HistoryMessage` | `src/type_aliases.py` (NEW) | 6 | NEW | +| `ToolDefinition` | `src/type_aliases.py` (NEW) | 4 | NEW | +| `SessionInsights` | `src/type_aliases.py` (NEW) | 6 | NEW | +| `DiscussionSettings` | `src/type_aliases.py` (NEW) | 3 | NEW | +| `CustomSlice` | `src/type_aliases.py` (NEW) | 4 | NEW | +| `MMAUsageStats` | `src/type_aliases.py` (NEW) | 3 | NEW | +| `ProviderPayload` | `src/type_aliases.py` (NEW) | 4 | NEW | +| `UIPanelConfig` | `src/type_aliases.py` (NEW) | 3 | NEW | +| `PathInfo` | `src/type_aliases.py` (NEW) | 3 | NEW | +| `RAGChunk` | `src/rag_engine.py` (NEW) | 4 | NEW | + +Each new dataclass has a paired `to_dict()` / `from_dict()` round-trip (the canonical pattern from `src/openai_schemas.py` and `src/models.py:533`). + +### `Metadata: TypeAlias = dict[str, Any]` — preserved as the catch-all + +`Metadata` is **unchanged**. It is the catch-all for the truly collapsed codepaths: + +- `manual_slop.toml` project config loading (`self.project.get('paths', {})`, `self.project.get('conductor', {})`, `self.project.get('context_presets', {})`, `self.project.get('discussion', {})`) +- Generic JSON parsing at the wire boundary (REST API payloads, WebSocket messages) +- Polymorphic log dumping (a function that serializes a list of mixed-aggregate entries to JSON without caring about their individual types) + +These sites keep `Metadata` and `.get('key', default)` because there is no per-aggregate type to promote to. The classification (per-site: "promoted" or "collapsed-codepath with justification") is auditable in the Phase 11 commit message. + +### 13 phases (1 per aggregate + audit + verification) + +The corrected plan has 13 phases: + +- Phase 0: Design the new dataclasses + add regression-guard tests (5 tasks) +- Phase 1: Migrate `Ticket` consumers (3 tasks; remove legacy `get()` method) +- Phase 2: Migrate `FileItem` consumers (2 tasks) +- Phase 3: Migrate `CommsLogEntry` consumers (4 tasks; new dataclass) +- Phase 4: Migrate `HistoryMessage` consumers (2 tasks; new dataclass) +- Phase 5: Wire `ChatMessage` into per-vendor send paths (4 tasks) +- Phase 6: Wire `UsageStats` into per-call usage aggregation (1 task) +- Phase 7: Wire `ToolCall` into tool loop section (2 tasks) +- Phase 8: Migrate `ToolDefinition` consumers (2 tasks; new dataclass) +- Phase 9: Migrate `RAGChunk` consumers (1 task; new dataclass) +- Phase 10: Migrate small-batch aggregates (2 tasks; 8 small aggregates) +- Phase 11: `Metadata` collapsed-codepath audit (1 task; classification per FR6) +- Phase 12: Verification + end-of-track (1 task; 3 commits) + +Estimated 29+ atomic commits. + +## What was changed in the corrected artifacts + +### `conductor/tracks/metadata_promotion_20260624/spec.md` + +Rewrote: + +- **Overview**: rewrote to emphasize per-aggregate dataclasses (not a shared mega-dataclass) and added the "CORRECTED 2026-06-25" status banner +- **Current State Audit**: added a 16-row table mapping each access pattern to its actual aggregate (the evidence that 12+ aggregates exist) +- **Goals**: rewrote G3 from "All 5 sub-aggregates share the same dataclass" to "Each known sub-aggregate is its OWN `@dataclass(frozen=True, slots=True)`" +- **Goals**: added G2 explicitly: "`Metadata: TypeAlias = dict[str, Any]` is preserved as the catch-all; NOT promoted to a shared mega-dataclass" +- **Goals**: added G8: classification rule for the remaining `.get()` sites +- **Functional Requirements**: rewrote FR1 with per-aggregate dataclass tables (existing reused + NEW dataclasses) and a "Why per-aggregate, not mega-dataclass" section +- **Out of Scope**: removed the "5 sub-aggregates becoming separate dataclasses each is overkill" line; added an explicit "Promoting `Metadata` to a shared mega-dataclass is the original spec's bad inference; rejected 2026-06-25" line +- **Non-Goals**: rewrote to reference the per-aggregate design +- **Risks**: rewrote R1 to reference the canonical pattern from `src/openai_schemas.py` / `src/models.py:533`; added R7 for name collisions + +### `conductor/tracks/metadata_promotion_20260624/plan.md` + +Rewrote: + +- **Header**: added "CORRECTED 2026-06-25" status banner +- **Phase 0**: expanded to 5 tasks (was 2); now includes RAGChunk (in `src/rag_engine.py`), ContextPreset schema completion (in `src/models.py`), per-aggregate test files (split into 12 files, not 1), and the styleguide clarification +- **Phases 1-10**: renamed to per-aggregate phases (Ticket, FileItem, CommsLogEntry, HistoryMessage, ChatMessage, UsageStats, ToolCall, ToolDefinition, RAGChunk, small-batch aggregates) +- **Phase 11**: NEW — the `Metadata` collapsed-codepath classification audit +- **Phase 12**: renamed from "Phase 6" — verification + end-of-track +- **Commit log**: expanded from 19-21 commits to 29+ commits +- **Verification commands**: updated to reflect the per-aggregate design (VC1: Metadata unchanged; VC2: each new dataclass exists; VC6: 60+ tests across 12 test files) + +### `conductor/tracks/metadata_promotion_20260624/metadata.json` + +Rewrote: + +- **`name`**: changed from "Metadata Promotion: dict[str, Any] -> @dataclass(frozen=True, slots=True)" to "Metadata Promotion: per-aggregate dataclasses + direct field access (NOT a shared mega-dataclass)" +- **`corrected`**: added field with date and correction note +- **`blocked_by`**: updated to reflect `code_path_audit_phase_3_provider_state_20260624` SHIPPED status +- **`scope.new_files`**: replaced single `tests/test_metadata_dataclass.py` with 12 per-aggregate test files +- **`scope.modified_files`**: replaced `src/type_aliases.py` alone with the 12 modified files (the type_aliases.py + the 9 consumer files + the styleguide + ContextPreset in models.py + RAGChunk in rag_engine.py) +- **`scope.new_dataclasses`**: NEW field — the 11 new dataclasses to add +- **`scope.reused_existing_dataclasses`**: NEW field — the 8 existing dataclasses to reuse unchanged +- **`scope.deprecated`**: NEW field — the 4 things this track removes (the alias chain, the legacy `Ticket.get()` method) +- **`verification_criteria`**: replaced "All 5 sub-aggregate TypeAliases (CommsLogEntry, HistoryMessage, FileItem, ToolDefinition, ToolCall) point to the new Metadata" with the per-aggregate criteria; added "Planning correction report exists" +- **`estimated_effort.scope`**: updated to reflect 29+ commits across 13 phases +- **`risk_register`**: rewrote R1-R7 to reference the per-aggregate design; added R7 (name collisions) and R8 (legacy `Ticket.get()` removal) +- **`out_of_scope`**: added "Promoting Metadata: TypeAlias = dict[str, Any] itself to a shared mega-dataclass (the original spec's bad inference; rejected 2026-06-25)" + +### `conductor/code_styleguides/type_aliases.md` + +Added §2.5 (after §2) — "When the role has stable distinct fields, promote it to its OWN dataclass": + +- The rule (per-aggregate dataclasses, not mega-dataclass) +- The when-NOT-to-promote rule (collapsed codepaths keep `Metadata`) +- A worked example from `src/openai_schemas.py` and `src/models.py:533` +- A reference back to the 2026-06-06 `data_structure_strengthening_20260606` spec §3.3 design intent +- A note that the `metadata_promotion_20260624` track was corrected on 2026-06-25 to continue in the per-concept promotion direction + +## Why this happened (the Tier 1 failure pattern) + +The original `metadata_promotion_20260624` author (me, on 2026-06-25) cited the `data_structure_strengthening_20260606` spec §3.3 design intent as evidence that the aliases could be promoted: + +> "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." + +But then the author chose the wrong direction: instead of splitting into per-concept TypedDicts/dataclasses (the "(or split into per-concept `TypedDict`s)" option), the author consolidated all 5 sub-aggregates into one mega-dataclass. The author treated the 5 sub-aggregates as "all the same thing, just labeled differently" — the exact opposite of what the 2026-06-06 spec anticipated. + +The user feedback (2026-06-25): + +> "I don't know where the previous tier 1 got the idea that this would be ok. It just makes a mess for no reason. Downstream codepaths that are going to utilize a specific data class should just... fucking use them." + +The Tier 1 failure pattern: + +1. **Cited the spec without reading the actual code.** The author should have run `git grep -E "\.get\('[a-z_]+',"` to see the actual access patterns. The 12+ distinct aggregates are evident from the access patterns. +2. **Did not check the existing per-aggregate dataclasses.** `src/openai_schemas.py` and `src/models.py` already define 9 separate frozen dataclasses — each with its own fields. The pattern was already in production; the author should have followed it. +3. **Conflated "names for shapes" with "same shape."** The `data_structure_strengthening_20260606` convention is "names for shapes" (the aliases document semantic role), but the underlying types were all `dict[str, Any]` because the codebase didn't have per-aggregate dataclasses yet. The promotion step is to GIVE each aggregate its OWN dataclass, not to MERGE them into one mega-dataclass. + +## Lessons learned (for future Tier 1s) + +1. **Read the actual code before designing.** The 12+ aggregates are evident from a `git grep` of the access patterns. Don't infer from type aliases alone. +2. **Check for existing per-aggregate dataclasses.** `src/openai_schemas.py` and `src/models.py` already define 9 separate frozen dataclasses. The pattern is canonical; follow it. +3. **Read the original spec's design intent.** `data_structure_strengthening_20260606` §3.3 anticipated per-concept promotion. The corrected design continues in that direction. +4. **"Names for shapes" ≠ "same shape."** Aliases document semantic role, but the underlying types can (and should) diverge into per-aggregate dataclasses as the codebase matures. +5. **The user said: "If we have known sub-types they should be their own data class if they're not already."** This is the rule. The original spec violated it; the corrected spec follows it. + +## See also + +- `conductor/tracks/metadata_promotion_20260624/spec.md` (corrected 2026-06-25) +- `conductor/tracks/metadata_promotion_20260624/plan.md` (corrected 2026-06-25) +- `conductor/tracks/metadata_promotion_20260624/metadata.json` (corrected 2026-06-25) +- `conductor/code_styleguides/type_aliases.md` §2.5 (added 2026-06-25) +- `conductor/code_styleguides/data_oriented_design.md` — canonical DOD reference +- `conductor/code_styleguides/error_handling.md` — `Result[T]` convention +- `conductor/tracks/data_structure_strengthening_20260606/spec.md` §3.3 — original 2026-06-06 design intent +- `conductor/tracks/any_type_componentization_20260621/spec.md` — grandparent track (89 sites promoted to dataclasses) +- `src/openai_schemas.py` — canonical per-aggregate dataclass pattern +- `src/models.py:533` — `FileItem` with `to_dict()` / `from_dict()` round-trip +- `src/models.py:302` — `Ticket` with 15 typed fields +- `docs/reports/SSDL_CAMPAIGN_ABORTED_20260624.md` — the post-mortem that established the type-dispatch-as-bug thesis \ No newline at end of file