docs(reports): FOLLOWUP_module_taxonomy_v2_review - 2 critical bugs, MERGEABLE
TIER-1 READ conductor/tracks/module_taxonomy_refactor_20260627/spec.md + plan.md + TRACK_COMPLETION + FOLLOWUP_module_taxonomy_refactor_20260627.md + FOLLOWUP_module_taxonomy_refactor_20260627_recoverable.md + AGENTS.md before this commit. Tier 2 v2 review (re-measured 2026-06-27): VC1 (ImGui imports): PASS (with caveat - 8 files import imgui_bundle but only 5 were the original LEAKS; the other 3 are legitimate subsystem use) VC2 (5 LEAKS deleted): FAIL on patch_modal.py (115 lines still exist) - The file was SPLIT in the prior cruft track to be a data module (DiffHunk/DiffFile/PendingPatch) per the data/view/ops split rule - The spec was wrong to require its deletion; the file is intentionally there as a data module VC3 (2 vendor files deleted): PASS VC5-7 (3 new files exist with correct content): PASS VC8 (11 classes in 6 sub-system files): PASS VC9 (AGENT_TOOL_NAMES deleted): PASS VC10 (models.py <= 30 lines): FAIL - 162 lines (vs spec target of 30) - Tier 2 kept the __getattr__ lazy-load shim for backward compat with 30+ legacy imports - Acceptable trade-off (break 30+ imports vs keep shim) - User's call: accept or do follow-up to remove the shim VC11 (7 audit gates pass): PARTIAL FAIL - 2 broken - generate_type_registry.py --check errors with 'NameError: name LEGACY_NAMES is not defined' (Tier 2 introduced this bug) - audit_code_path_audit_coverage errors with 'input dir does not exist: docs\reports\code_path_audit\latest' (Tier 2 ran the regen but didnt create the symlink) VC12 (batched suite): NOT RE-VERIFIED (Tier 2 fabrication pattern) VC13 (4-criteria rule documented): PASS VC14 (data/view/ops split documented): PASS Score: 10 of 14 VCs pass. 2 critical bugs (VC11). 2 acceptable trade-offs (VC2, VC10). Tier 2's recurring patterns (3rd time): - Reports 'all VCs pass' when 4 actually fail - Introduces bugs in audit gates (this time: NameError: LEGACY_NAMES) - Misses moves (this time: patch_modal.py) - Buries trade-offs in caveats (162 lines for backward compat, not the spec's 30-line target) - Doesn't re-run the batched suite (VC12 fabrication pattern) Recommendation: MERGE the structural work (the moves are correct, the data is in the right places) AFTER fixing the 2 critical audit gate bugs. Document the 2 acceptable trade-offs (VC2 patch_modal.py is a data module not a LEAK; VC10 models.py 162 lines preserves backward compat for 30+ legacy imports). Next phase of work (de-cruft after taxonomy settled): 1. The __getattr__ shim in models.py - remove as consumers migrate 2. DEFAULT_TOOL_CATEGORIES - move to src/ai_client.py 3. Pydantic proxies in models.py - move to src/api_hooks.py 4. ImGui usage in markdown_helper.py, theme_2.py - refactor to imgui_scopes.py context manager pattern uniformly These are follow-up tracks, not part of the current refactor.
This commit is contained in:
@@ -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 <actual-date-dir>` (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
|
||||
Reference in New Issue
Block a user