Private
Public Access
0
0

conductor(track): plan for result_migration_small_files_20260617

This commit is contained in:
2026-06-17 18:20:06 -04:00
parent 0aa00e394d
commit 9f9fcf93e1
@@ -0,0 +1,346 @@
# Plan: Result Migration — Sub-Track 2 (Small Files + Audit-Script Bug Fixes)
**Sub-track:** `result_migration_small_files_20260617`
**Umbrella:** [`result_migration_20260616`](../../result_migration_20260616/spec.md)
**Owner:** Tier 2 Tech Lead
**Base commit:** origin/master (post-`result_migration_review_pass_20260617` merge)
**Audit-data commit:** origin/master HEAD (the 10 new heuristics are in `scripts/audit_exception_handling.py`; 3 documented bugs in §4.4 of `docs/reports/RESULT_MIGRATION_REVIEW_PASS_20260617.md`)
---
## Phase 1: Audit-Script Bug Fixes (3 bugs, TDD)
### 1.1 Fix `visit_Try` walker bug
- [ ] **Task 1.1.1: Write failing test for the `visit_Try` walker**
- WHERE: `tests/test_audit_exception_handling_bug_fixes.py` (new file)
- WHAT: A test fixture with a `try/except/except/raise` pattern where the FIRST `except` handler has a `raise` statement. The test asserts the audit classifies the first handler's raise as `INTERNAL_RETHROW` (or whatever the correct category is).
- HOW: Use the `subprocess` pattern from `tests/test_audit_exception_handling_heuristics.py` (write a fixture to a temp dir, invoke the audit, parse the JSON)
- COMMIT: not yet (this is the RED step; commit follows the GREEN step)
- [ ] **Task 1.1.2: Fix the `visit_Try` walker**
- WHERE: `scripts/audit_exception_handling.py:759-784`
- WHAT: The `for handler in node.handlers` loop at L771 leaves `handler` bound to the last handler. The `for child in handler.body if node.handlers else []` at L774 only walks the last handler's body. Fix: move the `for child in ...` loop INSIDE the `for handler in node.handlers` loop so each handler's body is walked.
- HOW: Surgical edit. The current code is roughly:
```python
for handler in node.handlers:
self._record_finding(handler, ...)
for child in handler.body if node.handlers else []: # uses LAST handler
...
```
The fix is to walk each handler's body inside the loop:
```python
for handler in node.handlers:
self._record_finding(handler, ...)
for child in handler.body:
...
```
- SAFETY: The audit script is a static analyzer; the change doesn't affect runtime behavior. Run the audit before and after to verify the new findings are correct.
- COMMIT: `fix(scripts): visit_Try walker now visits ALL except handlers (bug from review pass §4.4 #1)`
- GIT NOTE: Per-site count delta (the `src/rag_engine.py:31` raise is now in the findings; expected 5-15 new INTERNAL_RETHROW findings across the codebase)
- [ ] **Task 1.1.3: Verify the fix doesn't break existing tests**
- WHERE: `tests/test_audit_exception_handling_heuristics.py` + the 11 test tiers
- WHAT: Run the existing 10 heuristic tests + the full test suite to verify no regression
- HOW: `uv run pytest tests/test_audit_exception_handling_heuristics.py -v` + `uv run python scripts/run_tests_batched.py`
- SAFETY: If a regression occurs, the fix is wrong; revert and re-investigate
- COMMIT: rolled into 1.1.2 (the test is part of the same atomic commit)
### 1.2 Fix `render_json` compliant-finding filter
- [ ] **Task 1.2.1: Write failing test for the per-file findings list**
- WHERE: `tests/test_audit_exception_handling_bug_fixes.py`
- WHAT: A test fixture with an `INTERNAL_COMPLIANT` site (e.g., the list.index+ValueError pattern). Run the audit with `--json` (non-verbose). Assert the `INTERNAL_COMPLIANT` finding is in the per-file list.
- HOW: Same `subprocess` pattern; parse the JSON; check the findings list
- [ ] **Task 1.2.2: Fix the filter**
- WHERE: `scripts/audit_exception_handling.py:884, 889, 958`
- WHAT: The filter `if f.category in VIOLATION_CATEGORIES or f.category in ("UNCLEAR", "INTERNAL_RETHROW")` excludes `INTERNAL_COMPLIANT`. The fix depends on intent:
- If the goal is "show only non-compliant findings" (current behavior), the filter is correct; the fix is to add a `--verbose` flag that includes compliant findings
- If the goal is "show all findings, categorized", the filter should be removed entirely
- The intended fix per the review-pass report: show all findings in non-verbose mode (the totals are right; the per-file list should match the totals). Change the filter to `True` (include all) or to a per-category opt-in via `--verbose`.
- HOW: The simplest fix is to change the filter to `True` (include all findings) for the per-file list. This may make the JSON output larger; verify the change doesn't break the existing `--json` consumers.
- SAFETY: The audit's JSON output is consumed by the 10 existing heuristic tests + the tier-2 agent. If the output shape changes, the tests catch it.
- COMMIT: `fix(scripts): render_json per-file list now includes all findings (bug from review pass §4.4 #2)`
- GIT NOTE: Per-file list now shows all findings (not just violations); INTERNAL_COMPLIANT sites are visible
- [ ] **Task 1.2.3: Verify the fix doesn't break existing tests**
- WHERE: same as 1.1.3
- COMMIT: rolled into 1.2.2
### 1.3 Fix `render_json` truncation
- [ ] **Task 1.3.1: Write failing test for the no-truncation behavior**
- WHERE: `tests/test_audit_exception_handling_bug_fixes.py`
- WHAT: A test fixture in a file with very few violations (e.g., 1 violation) that creates a low-violation file with an UNCLEAR site. Run the audit with `--json` and assert the UNCLEAR site is in the per-file findings list (not truncated to top 15).
- HOW: Use the audit's `--src` flag to point to a temp dir with the fixture; parse the JSON
- [ ] **Task 1.3.2: Fix the truncation**
- WHERE: `scripts/audit_exception_handling.py:1058` (CLI default `--top 15`) and `scripts/audit_exception_handling.py:958` (the slice `[r for r in sorted_reports[:top]]`)
- WHAT: The per-file list is truncated to top 15 by violation count. The fix is either:
- Change the default `--top` to a much higher value (e.g., 200, which is the total file count)
- Or remove the truncation entirely
- HOW: The simplest fix is to change the default to a value >= the total file count. 200 is a safe default (the project has 65 src/ files; 200 covers future growth).
- SAFETY: The `--top` flag is a CLI option; users can still pass `--top 15` if they want a truncated view.
- COMMIT: `fix(scripts): render_json no longer truncates per-file list to top 15 (bug from review pass §4.4 #3)`
- GIT NOTE: Default `--top` changed to 200; per-file list shows all files with findings
- [ ] **Task 1.3.3: Verify the fix doesn't break existing tests**
- WHERE: same as 1.1.3
- COMMIT: rolled into 1.3.2
### 1.4 Phase 1 Verification
- [ ] **Task 1.4.1: Run the full audit post-Phase-1**
- WHERE: `scripts/audit_exception_handling.py`
- WHAT: `uv run python scripts/audit_exception_handling.py --json`; verify:
- `src/rag_engine.py:31` is in the findings (the bug 1 fix)
- The per-file list includes INTERNAL_COMPLIANT findings (the bug 2 fix)
- The per-file list is not truncated to top 15 (the bug 3 fix)
- HOW: parse the JSON; assert each condition
- COMMIT: rolled into the last fix commit (or a separate `docs(track): verify audit bug fixes` commit)
- [ ] **Task 1.4.2: Run the full test suite**
- WHERE: `tests/`
- WHAT: `uv run python scripts/run_tests_batched.py`; verify all 11 tiers PASS
- COMMIT: rolled into 1.4.1
---
## Phase 2: Classify the 4 UNCLEAR Sites in SMALL
### 2.1 Per-site classification
- [ ] **Task 2.1.1: Classify `src/outline_tool.py` UNCLEAR site**
- WHERE: `src/outline_tool.py:49` (per the audit JSON)
- WHAT: Read the snippet + 2-3 lines of context; classify compliant-or-migration
- HOW: `manual-slop_get_file_slice` on the file at L48-52; read the function context
- DECISION: record in the per-site report
- COMMIT: `docs(track): result_migration_small_files decisions for src/outline_tool.py UNCLEAR`
- GIT NOTE: Classification + rationale
- [ ] **Task 2.1.2: Classify `src/summarize.py` UNCLEAR site**
- WHERE: `src/summarize.py:36`
- WHAT: Same as 2.1.1
- COMMIT: `docs(track): result_migration_small_files decisions for src/summarize.py UNCLEAR`
- [ ] **Task 2.1.3: Classify `src/conductor_tech_lead.py` UNCLEAR site**
- WHERE: `src/conductor_tech_lead.py:1` (or wherever the audit reports)
- WHAT: Same as 2.1.1
- COMMIT: `docs(track): result_migration_small_files decisions for src/conductor_tech_lead.py UNCLEAR`
- [ ] **Task 2.1.4: Classify `src/openai_compatible.py` UNCLEAR site**
- WHERE: `src/openai_compatible.py:1` (or wherever the audit reports)
- WHAT: Same as 2.1.1
- COMMIT: `docs(track): result_migration_small_files decisions for src/openai_compatible.py UNCLEAR`
- [ ] **Task 2.1.5: Update audit heuristics if patterns emerge**
- WHERE: `scripts/audit_exception_handling.py` (the `_classify_except` / `_classify_raise` functions)
- WHAT: If 2+ of the 4 UNCLEAR sites are compliant and share a common pattern, add a heuristic to the audit script (similar to the review pass)
- HOW: Same TDD pattern as the review pass (test first, then heuristic)
- COMMIT: `feat(scripts): add heuristics for the 4 SMALL UNCLEAR patterns` (conditional on pattern emergence)
---
## Phase 3-7: Migrate the 37 Files in Batches
The 35 SMALL files are batched 5-7 per commit (per the umbrella spec). The 2 MEDIUM files get dedicated commits. The batches are grouped by topic for coherence:
### Phase 3: Logging + Tracking batch (7 files)
- [ ] **Task 3.1: Migrate `src/summary_cache.py` (4 sites)**
- WHERE: `src/summary_cache.py:39, 48, 91, 100`
- WHAT: Each `try/except` becomes a `Result[T]` return
- HOW: `manual-slop_get_file_slice` for each site; convert
- COMMIT: `refactor(src): migrate src/summary_cache.py to Result[T] error handling`
- GIT NOTE: 4 sites migrated; per-site mapping
- [ ] **Task 3.2: Migrate `src/log_pruner.py` (2 sites)**
- WHERE: `src/log_pruner.py:0+` (2 compliant sites — no migration needed)
- DECISION: per the audit, the 2 sites are `INTERNAL_COMPLIANT`; no migration; document the decision
- COMMIT: `docs(track): result_migration_small_files decisions for src/log_pruner.py (2 compliant; 0 migration)`
- GIT NOTE: Audit classification confirmed
- [ ] **Task 3.3: Migrate `src/log_registry.py` (2 sites)**
- WHERE: `src/log_registry.py:2+` (per the audit)
- COMMIT: `refactor(src): migrate src/log_registry.py to Result[T] error handling`
- GIT NOTE: 2 sites migrated
- [ ] **Task 3.4: Migrate `src/performance_monitor.py` (1 site)**
- WHERE: `src/performance_monitor.py:1+` (per the audit; 1 compliant site)
- DECISION: 1 compliant; no migration
- COMMIT: `docs(track): result_migration_small_files decisions for src/performance_monitor.py (1 compliant; 0 migration)`
- GIT NOTE: Audit classification confirmed
- [ ] **Task 3.5: Migrate `src/startup_profiler.py` (1 site)**
- WHERE: `src/startup_profiler.py:1+` (per the audit; 1 migration-target)
- COMMIT: `refactor(src): migrate src/startup_profiler.py to Result[T] error handling`
- GIT NOTE: 1 site migrated
- [ ] **Task 3.6: Migrate `src/project_manager.py` (5 sites)**
- WHERE: `src/project_manager.py:32, 98, 363, 375, 390` (per the audit JSON)
- COMMIT: `refactor(src): migrate src/project_manager.py to Result[T] error handling`
- GIT NOTE: 5 sites migrated
- [ ] **Task 3.7: Migrate `src/paths.py` (3 sites)**
- WHERE: `src/paths.py:3+` (per the audit; 3 compliant sites)
- DECISION: 3 compliant; no migration
- COMMIT: `docs(track): result_migration_small_files decisions for src/paths.py (3 compliant; 0 migration)`
- GIT NOTE: Audit classification confirmed
### Phase 4: Config + Preset batch (6 files)
- [ ] **Task 4.1: Migrate `src/presets.py` (2 sites)**
- WHERE: `src/presets.py:2+` (per the audit; 2 migration-target)
- COMMIT: `refactor(src): migrate src/presets.py to Result[T] error handling`
- [ ] **Task 4.2: Migrate `src/personas.py` (3 sites)**
- WHERE: `src/personas.py:3+` (per the audit; 3 compliant sites)
- DECISION: 3 compliant; no migration
- COMMIT: `docs(track): result_migration_small_files decisions for src/personas.py (3 compliant; 0 migration)`
- [ ] **Task 4.3: Migrate `src/tool_presets.py` (3 sites)**
- WHERE: `src/tool_presets.py:3+` (per the audit; 3 compliant sites)
- DECISION: 3 compliant; no migration
- COMMIT: `docs(track): result_migration_small_files decisions for src/tool_presets.py (3 compliant; 0 migration)`
- [ ] **Task 4.4: Migrate `src/context_presets.py` (1 site)**
- WHERE: `src/context_presets.py:1+` (per the audit; 1 migration-target)
- COMMIT: `refactor(src): migrate src/context_presets.py to Result[T] error handling`
- [ ] **Task 4.5: Migrate `src/vendor_capabilities.py` (1 site)**
- WHERE: `src/vendor_capabilities.py:1+` (per the audit; 1 migration-target — actually suspicious category)
- COMMIT: `refactor(src): migrate src/vendor_capabilities.py to Result[T] error handling`
- [ ] **Task 4.6: Migrate `src/workspace_manager.py` (3 sites)**
- WHERE: `src/workspace_manager.py:3+` (per the audit; 3 compliant sites)
- DECISION: 3 compliant; no migration
- COMMIT: `docs(track): result_migration_small_files decisions for src/workspace_manager.py (3 compliant; 0 migration)`
### Phase 5: UI + Theme + Tooling batch (7 files)
- [ ] **Task 5.1: Migrate `src/command_palette.py` (1 site)**
- [ ] **Task 5.2: Migrate `src/commands.py` (3 sites)**
- [ ] **Task 5.3: Migrate `src/diff_viewer.py` (1 site)**
- [ ] **Task 5.4: Migrate `src/external_editor.py` (3 sites, 2 OPTIONAL_RETURN)**
- [ ] **Task 5.5: Migrate `src/theme_2.py` (1 site)**
- [ ] **Task 5.6: Migrate `src/theme_models.py` (1 migration-target + 9 compliant)**
- [ ] **Task 5.7: Migrate `src/markdown_helper.py` (2 sites)**
(Each task = 1 commit; pattern as in Phase 3-4)
### Phase 6: Provider + Adapter + Orchestration batch (7 files)
- [ ] **Task 6.1: Migrate `src/gemini_cli_adapter.py` (2 sites)**
- [ ] **Task 6.2: Migrate `src/openai_compatible.py` (1 UNCLEAR — already classified in Phase 2)**
- [ ] **Task 6.3: Migrate `src/aggregate.py` (4 sites)**
- [ ] **Task 6.4: Migrate `src/conductor_tech_lead.py` (1 UNCLEAR — already classified in Phase 2)**
- [ ] **Task 6.5: Migrate `src/dag_engine.py` (1 site)**
- [ ] **Task 6.6: Migrate `src/multi_agent_conductor.py` (4 sites)**
- [ ] **Task 6.7: Migrate `src/models.py` (3 sites: 2 V + 1 S; 2 compliant sites stay as-is)**
(Each task = 1 commit; pattern as in Phase 3-4)
### Phase 7: Infrastructure + Hook + Utility batch (8 files)
- [ ] **Task 7.1: Migrate `src/api_hook_client.py` (2 sites)**
- [ ] **Task 7.2: Migrate `src/api_hooks.py` (5 sites: 3 V + 2 S)**
- [ ] **Task 7.3: Migrate `src/file_cache.py` (2 sites)**
- [ ] **Task 7.4: Migrate `src/hot_reloader.py` (1 site)**
- [ ] **Task 7.5: Migrate `src/orchestrator_pm.py` (2 sites)**
- [ ] **Task 7.6: Migrate `src/outline_tool.py` (3 sites, including the 1 UNCLEAR from Phase 2)**
- [ ] **Task 7.7: Migrate `src/shell_runner.py` (2 sites)**
- [ ] **Task 7.8: Migrate `src/summarize.py` (2 sites, including the 1 UNCLEAR from Phase 2)**
(Each task = 1 commit; pattern as in Phase 3-4)
### Phase 8: MEDIUM files (2 files, dedicated commits per umbrella)
- [ ] **Task 8.1: Migrate `src/session_logger.py` (8 sites)**
- WHERE: `src/session_logger.py:99, 131, 147, 160, 188, 201, 226, 245`
- WHAT: All 8 sites are migration-target; convert each to `Result[T]`
- HOW: `manual-slop_get_file_slice` for each site; convert
- COMMIT: `refactor(src): migrate src/session_logger.py to Result[T] error handling (8 sites)`
- GIT NOTE: 8 sites migrated; per-site mapping
- [ ] **Task 8.2: Migrate `src/warmup.py` (7 sites)**
- WHERE: `src/warmup.py:85, 139, 175, 215, 249, 276, 300` (per the audit JSON; L85 is the validation raise — already classified as compliant by the review pass)
- WHAT: 6 migration-target + 1 compliant; convert the 6
- HOW: `manual-slop_get_file_slice` for each migration-target site; convert
- COMMIT: `refactor(src): migrate src/warmup.py to Result[T] error handling (6 sites; L85 validation raise stays as-is)`
- GIT NOTE: 6 sites migrated; L85 stays (validation raise is legitimate per styleguide)
---
## Phase 9: Verification
- [ ] **Task 9.1: Run the audit post-migration**
- WHERE: `scripts/audit_exception_handling.py`
- WHAT: `uv run python scripts/audit_exception_handling.py --json`; verify:
- 0 migration-target sites in the 37-file scope (all are `INTERNAL_COMPLIANT`, `BOUNDARY_*`, or `INTERNAL_PROGRAMMER_RAISE`)
- The per-file list shows all findings (not truncated)
- HOW: parse the JSON; assert the 37 files have 0 V+S sites
- COMMIT: `docs(track): verify result_migration_small_files_20260617 migration complete (0 migration-target sites in scope)`
- [ ] **Task 9.2: Run the full test suite**
- WHERE: `tests/`
- WHAT: `uv run python scripts/run_tests_batched.py`; verify all 11 tiers PASS
- HOW: the batched runner
- COMMIT: rolled into 9.1
- [ ] **Task 9.3: Write the per-site report**
- WHERE: `docs/reports/RESULT_MIGRATION_SMALL_FILES_20260617.md`
- WHAT: Per-site decision table for the 4 UNCLEAR sites + per-file migration summary (76 sites across 37 files) + audit-script bug-fix summary + verification
- HOW: follow the format of the review-pass report
- COMMIT: `docs(report): add result_migration_small_files_20260617 report`
- GIT NOTE: Summary of the migration + audit-script fixes
- [ ] **Task 9.4: Update the umbrella spec**
- WHERE: `conductor/tracks/result_migration_20260616/spec.md`
- WHAT: Update the "Post-Review Pass Update" callout to note sub-track 2 shipped; update the sub-track 2 row to "shipped"; update the recommended sequence
- HOW: edit the spec
- COMMIT: `docs(track): update umbrella with sub-track 2 shipped`
- GIT NOTE: 1-sentence note
- [ ] **Task 9.5: Mark the track as completed**
- WHERE: `conductor/tracks/result_migration_small_files_20260617/metadata.json` + `state.toml`, `conductor/tracks.md`
- WHAT: Update `status: active → completed`; `current_phase: "complete"`
- HOW: edit the files
- COMMIT: `conductor(track): mark result_migration_small_files_20260617 as completed`
- GIT NOTE: 1-sentence note
- [ ] **Task 9.6: Write the track completion report**
- WHERE: `docs/reports/TRACK_COMPLETION_result_migration_small_files_20260617.md`
- WHAT: End-of-track report (per the precedent set by the review-pass completion report)
- HOW: follow the format of `docs/reports/TRACK_COMPLETION_result_migration_review_pass_20260617.md`
- COMMIT: `docs(reports): TRACK_COMPLETION_result_migration_small_files_20260617`
- GIT NOTE: End-of-track report
---
## Risks at the Plan Level
| Risk | Mitigation |
|---|---|
| Phase 1 fixes break the existing 10 TDD tests | Task 1.1.3 / 1.2.3 / 1.3.3 verify each fix in isolation before the next |
| The 4 UNCLEAR sites are non-trivial migrations (more than 5 lines each) | Phase 2 classifies first; if any are >10 lines, they get their own commit in Phase 7 (not bundled) |
| The migration breaks behavior in a way the test suite doesn't catch | Task 9.2 catches regressions; for files that aren't tier-tested, manual smoke-testing is added |
| The batched-commit pattern (5-7 files per commit) is too coarse | The batch plan can be adjusted per-file; the umbrella's spec is guidance, not rigid |
| New migration-target sites surface after the `visit_Try` fix | Task 1.4.1 verifies the count delta; the per-batch scope adjusts |
| The audit-script fix commit is too large (>500 lines) | Each bug gets its own commit (1.1.2, 1.2.2, 1.3.2 are separate) |
| The MEDIUM files (session_logger, warmup) have complex migrations that don't fit the Result pattern | Per the styleguide, some sites are legitimately `BOUNDARY_*`; those stay as-is. The decision is documented in the report |
---
## Verification Snapshot (capture in the report)
After Phase 9, capture in `docs/reports/RESULT_MIGRATION_SMALL_FILES_20260617.md`:
- Audit pre-Phase-1: 76 sites (62V + 10S + 4?); 3 audit-script bugs documented
- Audit post-Phase-1: 0 audit-script bugs (the 3 bugs are fixed)
- Audit post-Phase-2: 4 UNCLEAR sites classified (decision count by category)
- Audit post-Phase-9: 0 migration-target sites in the 37-file scope
- Per-file migration summary (76 sites → 0; per-file counts)
- Per-site decisions for the 4 UNCLEAR sites
- Audit-script bug-fix summary (3 bugs; per-bug description + fix)
- Test pass count: all 11 tiers PASS; new tests added (6-9 for the audit-script fixes + N for the migrations)