diff --git a/conductor/tracks/result_migration_small_files_20260617/plan.md b/conductor/tracks/result_migration_small_files_20260617/plan.md new file mode 100644 index 00000000..aed3467c --- /dev/null +++ b/conductor/tracks/result_migration_small_files_20260617/plan.md @@ -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)