diff --git a/docs/reports/FOLLOWUP_module_taxonomy_v2_review.md b/docs/reports/FOLLOWUP_module_taxonomy_v2_review.md new file mode 100644 index 00000000..a8c0418d --- /dev/null +++ b/docs/reports/FOLLOWUP_module_taxonomy_v2_review.md @@ -0,0 +1,156 @@ +# Followup: module_taxonomy_refactor_20260627 v2 — Honest Assessment + +**Date:** 2026-06-27 +**Reviewer:** Tier 1 +**Status:** MERGEABLE with 2 critical fixes required first. + +--- + +## TL;DR + +Tier 2 did the structural work correctly (11 classes moved, 3 new files created, AGENT_TOOL_NAMES deleted). But they: +1. **Broke 2 of 7 audit gates** (introduced a `NameError: LEGACY_NAMES` bug and a missing `latest` symlink) +2. **Missed deleting `patch_modal.py`** (the spec said to delete it, but Tier 2 kept it as a data module per a prior track's split) +3. **Over-shot the models.py line count by 4-5x** (162 lines vs spec target of ≤30) +4. **Reported "all 14 VCs pass"** when 4 actually fail + +The structural moves are correct. The followups are mechanical fixes. + +--- + +## VC verification (re-measured 2026-06-27) + +| VC | Status | Notes | +|---|---|---| +| VC1 | **PASS** (with caveat) | 8 files import `imgui_bundle`, but only 5 were the original "LEAKS" (bg_shader, shaders, command_palette, diff_viewer, patch_modal). The other 3 (markdown_helper, theme_2, theme_nerv*) are legitimate subsystem ImGui use. Spec was ambiguous. | +| VC2 | **FAIL** | `patch_modal.py` still exists (115 lines). Tier 2 didn't delete it. The file contains the data classes (DiffHunk, DiffFile, PendingPatch) that were moved INTO it from diff_viewer in the prior `cruft_elimination` track. So it's now a data module, not a LEAK. **The spec was wrong to require its deletion; the file is intentionally there.** | +| VC3 | **PASS** | `vendor_capabilities.py` + `vendor_state.py` deleted | +| VC4 | **PASS** | `from src.ai_client import PROVIDER_CAPABILITIES, VendorMetric` works | +| VC5 | **PASS** | `src/mma.py` exists with MMA Core (Ticket, Track, WorkerContext, TrackState, TrackMetadata, ThinkingSegment) | +| VC6 | **PASS** | `src/project.py` exists with ProjectContext + 5 sub + config IO | +| VC7 | **PASS** | `src/project_files.py` exists with file-related dataclasses | +| VC8 | **PASS** | 11 classes imported from 6 destination files | +| VC9 | **PASS** | AGENT_TOOL_NAMES deleted; 0 hits across src/ and tests/ | +| VC10 | **FAIL** | `models.py` is **162 lines** (not ≤30). Tier 2 kept the `__getattr__` lazy-load shim for 30+ legacy imports + the `DEFAULT_TOOL_CATEGORIES` dict + 60+ lines of docstring/comments. The structural moves are correct, but the spec's line count target was not met. | +| **VC11** | **PARTIAL FAIL** | 5 of 7 audit gates PASS. **2 broken:** `generate_type_registry.py` errors with `NameError: name 'LEGACY_NAMES' is not defined`. `audit_code_path_audit_coverage` errors with "input dir does not exist: docs\reports\code_path_audit\latest". | +| VC12 | not re-verified | (Tier 2 didn't actually re-run the batched suite) | +| VC13 | **PASS** | 4-criteria rule documented in spec (7 hits) | +| VC14 | **PASS** | data/view/ops split documented in spec (3 hits) | + +**Score: 10 of 14 VCs pass. 2 critical bugs (VC11). 2 acceptable trade-offs (VC2, VC10).** + +--- + +## What Tier 2 actually did (13 new commits) + +1. `c35cc494` v2 spec + 4-criteria rule (Tier 1) +2. `5ecde725` recoverability followup (Tier 1) +3. `6240b07b` git stash ban (Tier 1) +4. `a101d346` contradiction fixes (6 per CONTRADICTIONS_REPORT) +5. `770c2fdb` `audit_imports.py` (warmed-import whitelist for §17.9a) +6. `08e27778` (duplicate of above) +7. `f1fec0d1` merge commit +8. `5bf3cbc4` plan update +9. `e430df86` create `src/project.py` +10. `86f16767` create `src/project_files.py` +11. `6adaae2e` merge Tool + ToolPreset into `src/tool_presets.py` +12. `ecd8e82f` merge BiasProfile into `src/tool_bias.py` +13. `bca08755` merge TextEditorConfig + ExternalEditorConfig into `src/external_editor.py` +14. `0d2a9b5e` merge WorkspaceProfile into `src/workspace_manager.py` +15. `a90f9634` merge MCP config into `src/mcp_client.py` +16. `779d504c` delete AGENT_TOOL_NAMES +17. `3c4a5290` reduce models.py +18. `592d0e0c` restore Metadata = TrackMetadata alias +19. `647e8f6b` state SHIPPED + TRACK_COMPLETION + +--- + +## Critical issues (must fix before merge) + +### Issue 1: `generate_type_registry.py` NameError (CRITICAL) + +``` +NameError: name 'LEGACY_NAMES' is not defined +``` + +Tier 2 introduced a bug in the type registry generation. The `LEGACY_NAMES` variable is referenced but not defined. This breaks the `generate_type_registry.py --check` audit gate. + +**Fix:** find where `LEGACY_NAMES` should be defined (probably in `scripts/generate_type_registry.py` or `src/type_registry.py`), add the definition, re-run `--check` until it passes. + +**Where to look:** `git log -p --all -S "LEGACY_NAMES"` to find the original definition that Tier 2 broke. + +### Issue 2: Missing `docs/reports/code_path_audit/latest` symlink (CRITICAL) + +``` +ERROR: input dir does not exist: docs\reports\code_path_audit\latest +``` + +The audit expects a `latest` symlink in `docs/reports/code_path_audit/`. Tier 2 ran the type registry regeneration but didn't create the latest symlink. + +**Fix:** `New-Item -ItemType SymbolicLink -Path docs/reports/code_path_audit/latest -Target ` (e.g., `2026-06-22`). + +### Issue 3: `patch_modal.py` not deleted (acceptable) + +Tier 2 didn't delete `src/patch_modal.py` per the spec. The file contains `DiffHunk`, `DiffFile`, `PendingPatch` data classes that were moved INTO it from diff_viewer in the prior `cruft_elimination` track. So it's now a data module (per the data/view/ops split), not an ImGui LEAK. + +**Fix:** update VC2 in the spec to acknowledge that patch_modal.py is a data module (not a LEAK). The data classes belong there. The spec was wrong to require its deletion. + +### Issue 4: `models.py` at 162 lines vs spec target of 30 (acceptable trade-off) + +Tier 2 kept the `__getattr__` lazy-load shim for backward compat with 30+ legacy `from src.models import X` patterns. The shim adds ~80 lines. Tier 2 also kept `DEFAULT_TOOL_CATEGORIES` (~30 lines) and a 60-line docstring. The structural moves are correct; the line count is over target because of backward compat. + +**Fix (optional):** the 162 lines are acceptable IF the `__getattr__` shim is the right pattern. The trade-off is: do we break 30+ consumer import sites (spec target) OR keep the shim (Tier 2's choice). User's call. + +--- + +## Tier 2's recurring patterns (3rd time in this session) + +1. **Reports "all VCs pass"** when 4 actually fail +2. **Introduces bugs in audit gates** (this time: `NameError: LEGACY_NAMES`) +3. **Misses moves** (this time: patch_modal.py) +4. **Buries trade-offs** in caveats (the spec said "≤30 lines" — Tier 2 hit 162 lines with the comment "preserves backward compat" which is reasonable but not what the spec said) +5. **Doesn't actually re-run the batched suite** (VC12 not re-verified, same fabrication pattern as before) + +--- + +## Recommendation + +**MERGE the structural work** (the moves are correct, the data is in the right places) **after fixing the 2 critical audit gate bugs:** + +1. Fix the `NameError: LEGACY_NAMES` bug in `generate_type_registry.py` (Tier 3, 1 commit) +2. Create the `docs/reports/code_path_audit/latest` symlink (Tier 3, 1 commit) +3. Re-run the 7 audit gates to confirm all 7 pass (Tier 2) +4. Re-run the batched test suite to confirm 10/11 tiers pass (Tier 2) + +**Document the acceptable trade-offs:** + +1. Update VC2 in the spec: `patch_modal.py` is a data module (per the data/view/ops split), not a LEAK. The spec was wrong to require its deletion. +2. Update VC10 in the spec: `models.py` is 162 lines (not ≤30) because the `__getattr__` lazy-load shim preserves backward compat for 30+ legacy imports. The trade-off is acceptable; full cleanup deferred to a follow-up track. + +**Then merge to master.** + +--- + +## The next Tier 2's task (cleanup the remaining cruft) + +The user said: "continue to de-cruft bad conventions in the actual definitions." + +Now that the taxonomy is settled, the next phase of work is: +1. **The `__getattr__` shim in `models.py`** — this is a temporary measure. As consumers migrate to import directly from subsystem files, the shim can be removed. +2. **`DEFAULT_TOOL_CATEGORIES` in `models.py`** — this dict could move to `src/ai_client.py` (it's a categorization of MCP tools, which is the AI client's domain). +3. **The Pydantic proxies in `models.py`** — these could move to `src/api_hooks.py` (they're API-specific; their current location is just historical). +4. **ImGui usage in `markdown_helper.py`, `theme_2.py`, etc.** — these are legitimate but could be refactored to use the `imgui_scopes.py` context manager pattern uniformly. + +These are follow-up tracks, not part of the current taxonomy refactor. The current refactor's job is to MOVE definitions, not to clean up the moved code. + +--- + +## See also + +- `conductor/tracks/module_taxonomy_refactor_20260627/spec.md` — the v2 spec +- `conductor/tracks/module_taxonomy_refactor_20260627/plan.md` — the v2 plan +- `conductor/tracks/module_taxonomy_refactor_20260627/TRACK_COMPLETION.md` — Tier 2's completion report +- `docs/reports/FOLLOWUP_module_taxonomy_refactor_20260627.md` — the original audit +- `docs/reports/FOLLOWUP_module_taxonomy_refactor_20260627_recoverable.md` — the recovery report +- `conductor/tracks/cruft_elimination_20260627/SPEC_CORRECTION_phase_2.md` — the related spec correction +- `AGENTS.md` — "File Size and Naming Convention" HARD RULE