From 93d906fb7babf52afe6186de845015ae95cb9147 Mon Sep 17 00:00:00 2001 From: Ed_ Date: Thu, 18 Jun 2026 18:50:48 -0400 Subject: [PATCH] move tracks to archive --- .../ai_client_docs_20260613/plan.md | 0 .../ai_client_docs_20260613/spec.md | 0 .../ai_client_docs_20260613/state.toml | 0 .../index.md | 0 .../metadata.json | 0 .../plan.md | 0 .../spec.md | 0 .../test_harness_hardening_20260310/index.md | 0 .../metadata.json | 0 .../test_harness_hardening_20260310/plan.md | 0 .../test_harness_hardening_20260310/spec.md | 0 .../metadata.json | 0 .../tier2_autonomous_sandbox_20260616/plan.md | 0 .../tier2_autonomous_sandbox_20260616/spec.md | 0 .../state.toml | 0 .../tier2_no_appdata_20260618/metadata.json | 0 .../tier2_no_appdata_20260618/plan.md | 0 .../tier2_no_appdata_20260618/spec.md | 0 .../tier2_no_appdata_20260618/state.toml | 0 conductor/tracks.md | 1 + .../metadata.json | 108 ++++++ .../plan.md | 266 +++++++++++++++ .../spec.md | 310 ++++++++++++++++++ .../state.toml | 70 ++++ 24 files changed, 755 insertions(+) rename conductor/{tracks => archive}/ai_client_docs_20260613/plan.md (100%) rename conductor/{tracks => archive}/ai_client_docs_20260613/spec.md (100%) rename conductor/{tracks => archive}/ai_client_docs_20260613/state.toml (100%) rename conductor/{tracks => archive}/conductor_path_configurable_20260306/index.md (100%) rename conductor/{tracks => archive}/conductor_path_configurable_20260306/metadata.json (100%) rename conductor/{tracks => archive}/conductor_path_configurable_20260306/plan.md (100%) rename conductor/{tracks => archive}/conductor_path_configurable_20260306/spec.md (100%) rename conductor/{tracks => archive}/test_harness_hardening_20260310/index.md (100%) rename conductor/{tracks => archive}/test_harness_hardening_20260310/metadata.json (100%) rename conductor/{tracks => archive}/test_harness_hardening_20260310/plan.md (100%) rename conductor/{tracks => archive}/test_harness_hardening_20260310/spec.md (100%) rename conductor/{tracks => archive}/tier2_autonomous_sandbox_20260616/metadata.json (100%) rename conductor/{tracks => archive}/tier2_autonomous_sandbox_20260616/plan.md (100%) rename conductor/{tracks => archive}/tier2_autonomous_sandbox_20260616/spec.md (100%) rename conductor/{tracks => archive}/tier2_autonomous_sandbox_20260616/state.toml (100%) rename conductor/{tracks => archive}/tier2_no_appdata_20260618/metadata.json (100%) rename conductor/{tracks => archive}/tier2_no_appdata_20260618/plan.md (100%) rename conductor/{tracks => archive}/tier2_no_appdata_20260618/spec.md (100%) rename conductor/{tracks => archive}/tier2_no_appdata_20260618/state.toml (100%) create mode 100644 conductor/tracks/result_migration_app_controller_20260618/metadata.json create mode 100644 conductor/tracks/result_migration_app_controller_20260618/plan.md create mode 100644 conductor/tracks/result_migration_app_controller_20260618/spec.md create mode 100644 conductor/tracks/result_migration_app_controller_20260618/state.toml diff --git a/conductor/tracks/ai_client_docs_20260613/plan.md b/conductor/archive/ai_client_docs_20260613/plan.md similarity index 100% rename from conductor/tracks/ai_client_docs_20260613/plan.md rename to conductor/archive/ai_client_docs_20260613/plan.md diff --git a/conductor/tracks/ai_client_docs_20260613/spec.md b/conductor/archive/ai_client_docs_20260613/spec.md similarity index 100% rename from conductor/tracks/ai_client_docs_20260613/spec.md rename to conductor/archive/ai_client_docs_20260613/spec.md diff --git a/conductor/tracks/ai_client_docs_20260613/state.toml b/conductor/archive/ai_client_docs_20260613/state.toml similarity index 100% rename from conductor/tracks/ai_client_docs_20260613/state.toml rename to conductor/archive/ai_client_docs_20260613/state.toml diff --git a/conductor/tracks/conductor_path_configurable_20260306/index.md b/conductor/archive/conductor_path_configurable_20260306/index.md similarity index 100% rename from conductor/tracks/conductor_path_configurable_20260306/index.md rename to conductor/archive/conductor_path_configurable_20260306/index.md diff --git a/conductor/tracks/conductor_path_configurable_20260306/metadata.json b/conductor/archive/conductor_path_configurable_20260306/metadata.json similarity index 100% rename from conductor/tracks/conductor_path_configurable_20260306/metadata.json rename to conductor/archive/conductor_path_configurable_20260306/metadata.json diff --git a/conductor/tracks/conductor_path_configurable_20260306/plan.md b/conductor/archive/conductor_path_configurable_20260306/plan.md similarity index 100% rename from conductor/tracks/conductor_path_configurable_20260306/plan.md rename to conductor/archive/conductor_path_configurable_20260306/plan.md diff --git a/conductor/tracks/conductor_path_configurable_20260306/spec.md b/conductor/archive/conductor_path_configurable_20260306/spec.md similarity index 100% rename from conductor/tracks/conductor_path_configurable_20260306/spec.md rename to conductor/archive/conductor_path_configurable_20260306/spec.md diff --git a/conductor/tracks/test_harness_hardening_20260310/index.md b/conductor/archive/test_harness_hardening_20260310/index.md similarity index 100% rename from conductor/tracks/test_harness_hardening_20260310/index.md rename to conductor/archive/test_harness_hardening_20260310/index.md diff --git a/conductor/tracks/test_harness_hardening_20260310/metadata.json b/conductor/archive/test_harness_hardening_20260310/metadata.json similarity index 100% rename from conductor/tracks/test_harness_hardening_20260310/metadata.json rename to conductor/archive/test_harness_hardening_20260310/metadata.json diff --git a/conductor/tracks/test_harness_hardening_20260310/plan.md b/conductor/archive/test_harness_hardening_20260310/plan.md similarity index 100% rename from conductor/tracks/test_harness_hardening_20260310/plan.md rename to conductor/archive/test_harness_hardening_20260310/plan.md diff --git a/conductor/tracks/test_harness_hardening_20260310/spec.md b/conductor/archive/test_harness_hardening_20260310/spec.md similarity index 100% rename from conductor/tracks/test_harness_hardening_20260310/spec.md rename to conductor/archive/test_harness_hardening_20260310/spec.md diff --git a/conductor/tracks/tier2_autonomous_sandbox_20260616/metadata.json b/conductor/archive/tier2_autonomous_sandbox_20260616/metadata.json similarity index 100% rename from conductor/tracks/tier2_autonomous_sandbox_20260616/metadata.json rename to conductor/archive/tier2_autonomous_sandbox_20260616/metadata.json diff --git a/conductor/tracks/tier2_autonomous_sandbox_20260616/plan.md b/conductor/archive/tier2_autonomous_sandbox_20260616/plan.md similarity index 100% rename from conductor/tracks/tier2_autonomous_sandbox_20260616/plan.md rename to conductor/archive/tier2_autonomous_sandbox_20260616/plan.md diff --git a/conductor/tracks/tier2_autonomous_sandbox_20260616/spec.md b/conductor/archive/tier2_autonomous_sandbox_20260616/spec.md similarity index 100% rename from conductor/tracks/tier2_autonomous_sandbox_20260616/spec.md rename to conductor/archive/tier2_autonomous_sandbox_20260616/spec.md diff --git a/conductor/tracks/tier2_autonomous_sandbox_20260616/state.toml b/conductor/archive/tier2_autonomous_sandbox_20260616/state.toml similarity index 100% rename from conductor/tracks/tier2_autonomous_sandbox_20260616/state.toml rename to conductor/archive/tier2_autonomous_sandbox_20260616/state.toml diff --git a/conductor/tracks/tier2_no_appdata_20260618/metadata.json b/conductor/archive/tier2_no_appdata_20260618/metadata.json similarity index 100% rename from conductor/tracks/tier2_no_appdata_20260618/metadata.json rename to conductor/archive/tier2_no_appdata_20260618/metadata.json diff --git a/conductor/tracks/tier2_no_appdata_20260618/plan.md b/conductor/archive/tier2_no_appdata_20260618/plan.md similarity index 100% rename from conductor/tracks/tier2_no_appdata_20260618/plan.md rename to conductor/archive/tier2_no_appdata_20260618/plan.md diff --git a/conductor/tracks/tier2_no_appdata_20260618/spec.md b/conductor/archive/tier2_no_appdata_20260618/spec.md similarity index 100% rename from conductor/tracks/tier2_no_appdata_20260618/spec.md rename to conductor/archive/tier2_no_appdata_20260618/spec.md diff --git a/conductor/tracks/tier2_no_appdata_20260618/state.toml b/conductor/archive/tier2_no_appdata_20260618/state.toml similarity index 100% rename from conductor/tracks/tier2_no_appdata_20260618/state.toml rename to conductor/archive/tier2_no_appdata_20260618/state.toml diff --git a/conductor/tracks.md b/conductor/tracks.md index 4edf29c1..ba5cc939 100644 --- a/conductor/tracks.md +++ b/conductor/tracks.md @@ -27,6 +27,7 @@ Tracks that are unblocked and ready to start. Ordered by **dependency** (blocked | 6d | A | [Result Migration (5 sub-tracks)](#track-result-migration-5-sub-tracks-new-2026-06-16) | umbrella spec ✓; sub-tracks 1+2 initialized (sub-track 1: `result_migration_review_pass_20260617` **shipped 2026-06-17**; sub-track 2: `result_migration_small_files_20260617` initialized; 3 remaining) | `exception_handling_audit_20260616`; identifies the migration target | (none — independent; **NEW 2026-06-16**; refactor phase; 5 sub-tracks eliminate the 268 "bad" sites per the audit; sub-tracks use the consistent `result_migration_*` prefix; **post-review pass 2026-06-17**: sub-track 4 gains 1 site `src/gui_2.py:1349`) | | 6d-1 | A | [Result Migration Sub-Track 1: Review Pass](#track-result-migration-sub-track-1-review-pass-2026-06-17) | spec ✓, plan ✓, metadata ✓, state ✓; **shipped 2026-06-17** (43 sites classified: 23 compliant + 1 migration-target + 8 PATTERN_1/2 + 9 compliant + 1 audit-script-bug; 10 new heuristics added; 3 audit-script bugs documented) | `result_migration_20260616` (umbrella); `exception_handling_audit_20260616` (shipped 2026-06-16) | (**NEW 2026-06-17**; sub-track 1 of 5; 43 sites classified; no production code change; T-shirt S; per-site decisions feed sub-tracks 2-4; 3 audit-script bugs documented for sub-track 2 Phase 1) | | 6d-2 | A | [Result Migration Sub-Track 2: Small Files + Audit-Script Bug Fixes](#track-result-migration-sub-track-2-small-files--audit-script-bug-fixes-2026-06-17) | spec ✓, plan ✓, metadata ✓, state ✓, **shipped 2026-06-18** (Phase 10 REJECTED for sliming 21 sites via 5 laundering heuristics; Phase 11 REDOES the 21 sites: 5 full Result migrations in warmup.py + 2 helper extracts + 14 documented; Phase 12 = ACTUAL full Result[T] migration: 16 sites in api_hooks.py + 27 sites in 16 small files; Heuristic #19 REMOVED; visit_Try bug FIXED; Heuristic D ADDED; Drain Points section in styleguide; **Phase 12 REJECTED for false test claim**; **Phase 13 = script crash fixed (UTF-8 reconfigure in run_tests_batched.py) + 3 failures investigated on parent commit (0 regressions) + 4 pre-existing Gemini 503 tests documented with @pytest.mark.skip + test_execution_sim_live switched from gemini_cli to gemini per user directive (STILL FAILS, reported for diff track); 11/11 tiers actually run; 9 PASS clean + 2 PASS with documented issues) | `result_migration_20260616` (umbrella); `result_migration_review_pass_20260617` (shipped 2026-06-17) | (**NEW 2026-06-17**; sub-track 2 of 5; 37 files (35 SMALL + 2 MEDIUM) with 76 sites; Phase 1 = 3 audit-script bugs fixed; Phases 3-8 = 49 sites migrated; Phase 10 = 26 SILENT_SWALLOW + 14 new UNCLEAR sites via full Result + 5 new heuristics; **Phase 10 REJECTED; Phase 11 = 5 full Result + 2 helper extracts + 14 documented; 5 laundering heuristics REVERTED; Heuristic A ADDED; Phase 12 = ACTUAL migration of all sites + styleguide Drain Points; Phase 13 = test count verification; 2 reported issues for diff tracks**) | +| 6d-3 | A | [Result Migration Sub-Track 3: App Controller](#track-result-migration-sub-track-3-app-controller-2026-06-18) | spec ✓, plan ✓, metadata ✓, state ✓, **active**; migrates 45 sites in `src/app_controller.py` to `Result[T]` (32 INTERNAL_BROAD_CATCH + 8 INTERNAL_SILENT_SWALLOW + 4 INTERNAL_RETHROW + 1 INTERNAL_OPTIONAL_RETURN); 22 sites stay as-is (15 BOUNDARY_FASTAPI + 2 BOUNDARY_SDK + 4 INTERNAL_COMPLIANT + 1 INTERNAL_PROGRAMMER_RAISE). **Phase 1 = fix the 2 known regressions** (test_tool_presets_execution::test_tool_ask_approval + test_extended_sims::test_execution_sim_live) caused by the half-migrated `session_logger.log_tool_call` call site in `_offload_entry_payload` (lines 3715, 3721). 5-file-commit pattern from `doeh_test_thinking_cleanup_20260615` (1 source + 1 test + 1 plan + 1 metadata + 1 state per task). 6 phases: (1) Setup + fix regressions; (2) 32 broad-catch → 4 bulk batches; (3) 8 silent-swallow → 2 batches with logging.debug per Heuristic #19; (4) 4 rethrow classified + 1 optional migrated; (5) Verify + audit + end-of-track report. | `result_migration_20260616` (umbrella); `result_migration_small_files_20260617` (shipped 2026-06-18) | (**NEW 2026-06-18**; sub-track 3 of 5; scope: 1 source file (src/app_controller.py) modified across 6 phases; 45 migration sites organized into 4 bulk batches + 3 single-site tasks; 1 new test file (test_app_controller_result.py) + 2 test files updated; 4 metadata/plan/state files; 1 end-of-track report; 18 atomic commits. **Scope larger than umbrella's T-shirt estimate** (45 migration + 22 stay = 67 total, not the estimated 22 + 34 = 56); the audit's per-category output is the source of truth, not the umbrella's T-shirt estimate**) | | 6e | A (meta-tooling) | [Tier 2 Autonomous Sandbox (unattended track execution)](#track-tier-2-autonomous-sandbox-new-2026-06-16) | spec ✓, plan ✓, **shipped 2026-06-16** (9 phases, 24 default-on tests + 4 opt-in tests + 1 smoke e2e) | (none — independent; **NEW 2026-06-16**; meta-tooling; eliminates the `permission: ask` bottleneck for well-regularized tracks via a 3-layer enforcement stack: OpenCode permission system + Windows restricted token + git hooks) | | 7 | — | [UI Polish (Five Issues)](#track-ui-polish-five-issues) | spec ✓, plan ✓, ready to start (Phases 1/4/5 shipped; Phases 2/3 code shipped but tests broken — fixed by track 6a) | (none — independent) | | 7a | B | [SQLite-Granularity Inline Docs for gui_2.py](#track-sqlite-granularity-inline-docs-for-gui_2py) | spec ✓, plan ✓, complete | (none — independent) | diff --git a/conductor/tracks/result_migration_app_controller_20260618/metadata.json b/conductor/tracks/result_migration_app_controller_20260618/metadata.json new file mode 100644 index 00000000..8a008f3f --- /dev/null +++ b/conductor/tracks/result_migration_app_controller_20260618/metadata.json @@ -0,0 +1,108 @@ +{ + "id": "result_migration_app_controller_20260618", + "name": "Result Migration - Sub-Track 3 (App Controller)", + "date": "2026-06-18", + "type": "refactor", + "priority": "A", + "spec": "conductor/tracks/result_migration_app_controller_20260618/spec.md", + "plan": "conductor/tracks/result_migration_app_controller_20260618/plan.md", + "status": "active", + "umbrella": "result_migration_20260616", + "sub_track_index": 3, + "blocked_by": { + "result_migration_small_files_20260617": "shipped 2026-06-17" + }, + "blocks": {}, + "scope": { + "new_files": [ + "tests/test_app_controller_result.py", + "docs/reports/TRACK_COMPLETION_result_migration_app_controller_20260618.md" + ], + "modified_files": [ + "src/app_controller.py", + "tests/test_app_controller_offloading.py", + "tests/test_audit_exception_handling_heuristics.py", + "conductor/tracks.md", + "conductor/tracks/result_migration_app_controller_20260618/state.toml", + "conductor/tracks/result_migration_app_controller_20260618/metadata.json", + "conductor/tracks/result_migration_app_controller_20260618/plan.md", + "conductor/tracks/result_migration_20260616/spec.md" + ], + "deleted_files": [] + }, + "verification_criteria": [ + "src/app_controller.py has zero INTERNAL_BROAD_CATCH sites (32 migrated)", + "src/app_controller.py has zero INTERNAL_SILENT_SWALLOW sites (8 migrated with logging.debug per Heuristic #19)", + "src/app_controller.py has zero INTERNAL_RETHROW sites (4 classified: legitimate patterns stay; SUSPICIOUS migrated)", + "src/app_controller.py has zero INTERNAL_OPTIONAL_RETURN sites (1 migrated to Result[int] or nil-sentinel)", + "src/app_controller.py preserves 15 BOUNDARY_FASTAPI sites (unchanged, per styleguide Boundary Types section)", + "src/app_controller.py preserves 2 BOUNDARY_SDK sites (unchanged, per styleguide Boundary Types section)", + "src/app_controller.py preserves 4 INTERNAL_COMPLIANT sites (unchanged, already compliant)", + "src/app_controller.py preserves 1 INTERNAL_PROGRAMMER_RAISE site (unchanged, per Fail Early pattern)", + "tests/test_app_controller_result.py exists with 5+ tests, all pass", + "tests/test_app_controller_offloading.py has 2 new unwrap-path tests, all pass", + "tests/test_tool_presets_execution::test_tool_ask_approval passes (Regression 1 fixed)", + "tests/test_extended_sims::test_execution_sim_live passes (Regression 2 fixed)", + "uv run python scripts/audit_exception_handling.py --by-size shows 0 INTERNAL_BROAD_CATCH and 0 INTERNAL_SILENT_SWALLOW for src/app_controller.py", + "uv run python scripts/run_tests_batched.py shows no new regressions (882 passed / 17 skipped / 2 xfailed, with the 2 previously-failing tests now passing)", + "docs/reports/TRACK_COMPLETION_result_migration_app_controller_20260618.md exists with the 7 standard sections" + ], + "regressions_and_pre_existing_failures": [ + { + "name": "test_tool_presets_execution::test_tool_ask_approval", + "cause": "session_logger.log_tool_call was partially migrated to return Result but the call site in _offload_entry_payload was not updated", + "fix_phase": 1, + "fix_task": 1.3 + }, + { + "name": "test_extended_sims::test_execution_sim_live", + "cause": "downstream effect of test_tool_ask_approval failure; the live GUI runs the same _offload_entry_payload path", + "fix_phase": 1, + "fix_task": 1.3 + } + ], + "pre_existing_failures_remaining": [], + "deferred_to_followup_tracks": [ + { + "title": "Sub-track 4: result_migration_gui_2", + "description": "Migrate src/gui_2.py (260KB) to the Result convention. The umbrella's sub-track 4 plan (line 276 of conductor/tracks/result_migration_20260616/spec.md) covers the 55 sites in gui_2.py.", + "track_status": "planned (per umbrella)" + }, + { + "title": "Sub-track 5: result_migration_baseline_cleanup", + "description": "Close the remaining 77 violations in the 3 refactored baseline files (mcp_client.py, ai_client.py, rag_engine.py). Per umbrella sub-track 5 (line 296-309 of result_migration_20260616/spec.md).", + "track_status": "planned (per umbrella)" + } + ], + "estimated_effort": { + "method": "scope (per workflow.md Tier 1 Track Initialization Rules). NO day estimates.", + "scope": "1 source file (src/app_controller.py) modified across 6 phases; 45 migration sites organized into 4 bulk batches + 3 single-site tasks; 1 new test file (test_app_controller_result.py) + 2 test files updated; 4 metadata/plan/state files; 1 end-of-track report. 18 atomic commits." + }, + "risk_register": [ + { + "risk": "Migrating __getattr__ may break Python's attribute lookup protocol (e.g., hasattr)", + "likelihood": "medium", + "mitigation": "Phase 4 task 4.1 explicitly tests test_app_getattr_hasattr_bug.py and test_app_controller_getattr_ui_bug.py; SUSPICIOUS rethrows are migrated; Pattern 1/2/3 legitimate rethrows stay" + }, + { + "risk": "Migrating 32 broad-catch sites changes error reporting semantics that downstream code may depend on", + "likelihood": "medium", + "mitigation": "Each batch is committed separately; the 2 new Result tests verify the contract; the batched suite is re-run at the end of Phase 5 to catch downstream breakage" + }, + { + "risk": "The audit's per-category count may shift as the migration proceeds (the script may reclassify sites based on context)", + "likelihood": "low", + "mitigation": "The audit is run after each phase; if a site moves from INTERNAL_BROAD_CATCH to BOUNDARY_FASTAPI mid-migration, the plan task description is updated to reflect the new category" + }, + { + "risk": "Scope is larger than the umbrella estimated (45 vs 22 migration sites); the XL T-shirt size may understate the work", + "likelihood": "medium", + "mitigation": "The umbrella spec is updated post-track (Phase 5 task 5.6) to reflect the actual count; the audit's per-category output is the source of truth" + }, + { + "risk": "The 2 known regressions (test_tool_ask_approval, test_execution_sim_live) may have additional root causes beyond the log_tool_call half-migration", + "likelihood": "low", + "mitigation": "Phase 1 task 1.3 is the regression fix; if the tests still fail after the fix, the implementation investigates before Phase 2 begins (do not loop; read code, predict, fix once, report)" + } + ] +} diff --git a/conductor/tracks/result_migration_app_controller_20260618/plan.md b/conductor/tracks/result_migration_app_controller_20260618/plan.md new file mode 100644 index 00000000..245beeb5 --- /dev/null +++ b/conductor/tracks/result_migration_app_controller_20260618/plan.md @@ -0,0 +1,266 @@ +# Plan: Result Migration — Sub-Track 3 (App Controller) + +**Sub-track:** `result_migration_app_controller_20260618` (3rd of 5 sub-tracks) +**Umbrella:** `result_migration_20260616` +**Date:** 2026-06-18 +**Owner:** Tier 2 Tech Lead +**Base commit:** `5107f3ca` (merge of `tier2/live_gui_test_fixes_20260618` into `tier2/result_migration_small_files_20260617`) + +--- + +## Phase 1: Setup + Fix the regression (highest priority) + +Focus: register the sub-track, then immediately fix the 2 known regressions (test_tool_ask_approval + test_execution_sim_live) so subsequent phases can run against a green tier-3-live_gui. + +### Task 1.1: Create sub-track folder +- **WHERE:** `conductor/tracks/result_migration_app_controller_20260618/` +- **WHAT:** spec.md (exists), plan.md (this file), metadata.json, state.toml +- **HOW:** Write the 3 new files following the umbrella spec pattern. The spec.md is already written by Tier 1. +- **SAFETY:** None (new files only). +- **COMMIT:** `conductor(track): spec/plan/metadata/state for result_migration_app_controller_20260618` +- **GIT NOTE:** Summary of sub-track 3 scope; references the 2 known regressions. + +### Task 1.2: Update `conductor/tracks.md` +- **WHERE:** `conductor/tracks.md` (after the umbrella row, before sub-track 4) +- **WHAT:** Add a row for the new sub-track +- **HOW:** Same pattern as the umbrella and the existing sub-tracks +- **SAFETY:** None (documentation only). +- **COMMIT:** `conductor: register result_migration_app_controller_20260618 in tracks.md` +- **GIT NOTE:** 1-sentence note + +### Task 1.3: Fix `_offload_entry_payload` call site (Regression 1) +- **WHERE:** `src/app_controller.py:3709-3725` (`_offload_entry_payload` method) +- **WHAT:** Unwrap the `Result` returned by `session_logger.log_tool_output` and `session_logger.log_tool_call`. The current code does `Path(ref_path).name` where `ref_path` is a `Result` object — `Path()` expects a string. +- **HOW:** Per FR5 in spec.md: + ```python + def _offload_entry_payload(self, entry: Dict[str, Any]) -> Dict[str, Any]: + optimized = copy.deepcopy(entry) + kind = optimized.get("kind") + payload = optimized.get("payload", {}) + if kind == "tool_result" and "output" in payload: + output = payload["output"] + ref_result = session_logger.log_tool_output(output) + if ref_result.ok and ref_result.data: + filename = Path(ref_result.data).name + payload["output"] = f"[REF:{filename}]" + elif ref_result.errors: + logging.debug("offload tool_output failed: %s", ref_result.errors[0].ui_message()) + if kind == "tool_call" and "script" in payload: + script = payload["script"] + ref_result = session_logger.log_tool_call(script, "LOG_ONLY", None) + if ref_result.ok and ref_result.data: + filename = Path(ref_result.data).name + payload["script"] = f"[REF:{filename}]" + elif ref_result.errors: + logging.debug("offload tool_call failed: %s", ref_result.errors[0].ui_message()) + return optimized + ``` +- **SAFETY:** The function signature is unchanged. The optimization (small payload via `[REF:filename]`) is preserved for both success and failure paths. The error path now logs at `logging.debug` (per Heuristic #19); on success the file content is referenced. +- **VERIFY:** `uv run python -m pytest tests/test_app_controller_offloading.py tests/test_tool_presets_execution.py -v` — `test_tool_ask_approval` passes; `test_on_comms_entry_tool_result_offloading` still passes. +- **COMMIT:** `fix(app_controller): _offload_entry_payload unwraps Result from session_logger (regression fix)` +- **GIT NOTE:** Closes the regression in `test_tool_ask_approval`; the `session_logger.log_tool_call` was partially migrated to return `Result` but the call site was not updated. The convention's "AND over OR" pattern handles it here. + +### Task 1.4: Add test for the unwrap path +- **WHERE:** `tests/test_app_controller_offloading.py` (existing file; add 2 new tests) +- **WHAT:** Add 2 tests: + 1. `test_offload_unwraps_result_success` — verify that when `log_tool_output` returns a successful `Result[data=path]`, the payload gets `[REF:filename]`. + 2. `test_offload_logs_debug_on_result_errors` — verify that when `log_tool_output` returns a `Result` with errors, a `logging.debug` is emitted and the payload is unchanged. +- **HOW:** Mock `session_logger.log_tool_output` and `log_tool_call` to return `Result` objects; assert the payload and the log call. +- **SAFETY:** Test-only changes; no production risk. +- **VERIFY:** The 2 new tests pass; existing 2 offloading tests still pass. +- **COMMIT:** `test(app_controller): offloading - verify Result unwrap in success and error paths` +- **GIT NOTE:** Tests for FR5; covers the regression from task 1.3. + +### Task 1.5: Run the regression test and confirm both fixes +- **COMMAND:** `uv run python -m pytest tests/test_tool_presets_execution.py::test_tool_ask_approval tests/test_extended_sims.py::test_execution_sim_live -v` +- **EXPECT:** Both pass. +- **COMMIT:** No commit (verification only). +- **NOTE:** If `test_execution_sim_live` still fails, investigate the failure mode (may be a separate issue from Regression 1). + +### Task 1.6: Phase 1 checkpoint commit +- **COMMIT:** `conductor(plan): mark Phase 1 complete (regression fix)` +- **GIT NOTE:** Phase 1 = 2 known regressions fixed; verified by `test_tool_ask_approval` + `test_execution_sim_live`. Now safe to proceed with the bulk migration. + +--- + +## Phase 2: Migrate the 32 INTERNAL_BROAD_CATCH sites (bulk) + +Focus: the main migration work. 32 sites, organized into 4 sub-batches by context (callback handlers, project ops, conductor ops, GUI tasks). Each sub-batch is 6-10 sites touching the same file; one commit per sub-batch. + +### Task 2.1: Create `tests/test_app_controller_result.py` (the new test file) +- **WHERE:** `tests/test_app_controller_result.py` (NEW) +- **WHAT:** 5+ tests verifying Result return types for the migrated methods (placeholder tests that will be filled in as the migrations land). Initial tests can be: + 1. `test_offload_entry_payload_returns_dict` — sanity check. + 2. `test_migrated_method_returns_result_when_no_error` — pattern template. + 3. `test_migrated_method_returns_result_with_error_on_failure` — pattern template. + 4. `test_migrated_method_never_raises_exception` — verifies the broad-catch is gone. + 5. `test_offload_entry_payload_preserves_unchanged_payload` — verifies the no-op path. +- **HOW:** Import `Result`, `ErrorInfo`, `ErrorKind` from `src.result_types`. Model on `tests/test_ai_client_result.py`. +- **SAFETY:** Test-only changes; no production risk. +- **COMMIT:** `test(app_controller): scaffold tests/test_app_controller_result.py with 5 Result-pattern tests` +- **GIT NOTE:** The 5 tests use generic placeholders that become specific per migration in subsequent tasks. The scaffolding defines the pattern. + +### Task 2.2: Migrate batch 1 — callback handlers (4 sites) +- **WHERE:** `src/app_controller.py:537 (_handle_custom_callback)`, `:579 (_handle_click)`, `:2046 (cb_load_prior_log)`, `:2068 (cb_load_prior_log)`, `:2081 (cb_load_prior_log)` +- **WHAT:** Convert `except Exception as e: pass` (or `print(...)`) to `except as e: return Result(data=None, errors=[...])`. The callback may need to return a `Result`; if the caller doesn't use the return value, wrap the body in a `try/except` that returns a result and is logged. +- **HOW:** For each site: + 1. Read the snippet + 3 lines of context with `get_file_slice`. + 2. Identify the specific exception (KeyError? AttributeError? OSError?). + 3. Add `from src.result_types import Result, ErrorInfo, ErrorKind` if not imported. + 4. Replace the broad `except Exception` with the specific one. + 5. Return a `Result` with the appropriate data and errors. +- **SAFETY:** The callback's caller may not be Result-aware; the migration may need to update the caller's signature. Track this in the plan task description. +- **VERIFY:** The 4 migrated sites + the 2 new tests in `test_app_controller_result.py` pass. +- **COMMIT:** `refactor(app_controller): migrate 5 callback sites to Result (batch 1)` +- **GIT NOTE:** Specific exceptions caught per site; Result return type. + +### Task 2.3: Migrate batch 2 — project ops (5 sites) +- **WHERE:** `src/app_controller.py:2129 (run_manual_prune)`, `:2140 (_load_active_project)`, `:2154 (_load_active_project)`, `:2195 (run_prune)`, `:2890 (_refresh_from_project)`, `:2944 (_save_active_project)` +- **WHAT:** Same pattern as 2.2 +- **SAFETY:** Project ops have side effects (file I/O). The migration must preserve the side-effect semantics while changing the error reporting. +- **VERIFY:** Project-op tests + the 2 new Result tests pass. +- **COMMIT:** `refactor(app_controller): migrate 6 project-op sites to Result (batch 2)` +- **GIT NOTE:** Project ops side effects preserved; Result error reporting added. + +### Task 2.4: Migrate batch 3 — conductor / track ops (8 sites) +- **WHERE:** `src/app_controller.py:3057 (_run)`, `:3084 (do_fetch)`, `:3094 (do_fetch)`, `:4237 (_start_track_logic)`, `:4349 (_cb_run_conductor_setup)`, `:4446 (_cb_load_track)`, `:4475 (_push_mma_state_update)`, `:4504 (_load_active_tickets)` +- **WHAT:** Same pattern as 2.2 +- **SAFETY:** Conductor ops interact with the MMA state. The migration must NOT change the state-mutation order; only the error reporting. +- **VERIFY:** MMA tests + the 2 new Result tests pass. +- **COMMIT:** `refactor(app_controller): migrate 8 conductor/track sites to Result (batch 3)` +- **GIT NOTE:** Conductor ops state order preserved; Result error reporting added. + +### Task 2.5: Migrate batch 4 — worker / task ops (8 sites) +- **WHERE:** `src/app_controller.py:3434 (worker)`, `:3471 (worker)`, `:3542 (worker)`, `:3635 (_handle_request_event)`, `:3648 (_handle_request_event)`, `:4070 (_bg_task)`, `:4100 (_bg_task)`, `:1669 (_process_pending_gui_tasks)`, `:1420 (_update_inject_preview)`, `:1480 (_do_rag_sync)`, `:1947 (replace_ref)` +- **WHAT:** Same pattern as 2.2 +- **SAFETY:** Worker / task ops run on background threads. The migration must be thread-safe (no shared mutable state changes that aren't already locked). +- **VERIFY:** Worker tests + the 2 new Result tests pass. +- **COMMIT:** `refactor(app_controller): migrate 11 worker/task sites to Result (batch 4)` +- **GIT NOTE:** Worker ops thread safety preserved; Result error reporting added. + +### Task 2.6: Phase 2 checkpoint commit +- **COMMIT:** `conductor(plan): mark Phase 2 complete (32 INTERNAL_BROAD_CATCH sites migrated)` +- **GIT NOTE:** Phase 2 = 32 broad-catch sites migrated; the audit's `INTERNAL_BROAD_CATCH` count for `app_controller.py` is now 0. + +--- + +## Phase 3: Migrate the 8 INTERNAL_SILENT_SWALLOW sites + +Focus: add `logging.debug` per Heuristic #19; convert return to `Result[T]`. + +### Task 3.1: Migrate SIGINT and timeline sites (3 sites) +- **WHERE:** `src/app_controller.py:751 (_on_sigint)`, `:756 (_install_sigint_exit_handler)`, `:1294 (mark_first_frame_rendered)`, `:1376 (_on_warmup_complete_for_timeline)` +- **WHAT:** Add `logging.debug("swallowed exception: %s", e, extra={"source": ""})`; convert return to `Result[None]` (`OK` on success, `Result(data=None, errors=[...])` on swallow). +- **VERIFY:** The 4 sites + the 2 new Result tests pass. +- **COMMIT:** `refactor(app_controller): migrate 4 SIGINT/timeline sites to Result with debug logging (silent swallow batch 1)` +- **GIT NOTE:** Heuristic #19 satisfied; Result error side-channel. + +### Task 3.2: Migrate MCP and worker sites (4 sites) +- **WHERE:** `src/app_controller.py:1566 (mcp_config_json)`, `:2389 (queue_fallback)`, `:4098 (_bg_task)`, `:4192 (_start_track_logic)` +- **WHAT:** Same pattern as 3.1 +- **VERIFY:** The 4 sites + the 2 new Result tests pass. +- **COMMIT:** `refactor(app_controller): migrate 4 MCP/worker sites to Result with debug logging (silent swallow batch 2)` +- **GIT NOTE:** Heuristic #19 satisfied; Result error side-channel. + +### Task 3.3: Phase 3 checkpoint commit +- **COMMIT:** `conductor(plan): mark Phase 3 complete (8 INTERNAL_SILENT_SWALLOW sites migrated)` +- **GIT NOTE:** Phase 3 = 8 silent-swallow sites migrated; the audit's `INTERNAL_SILENT_SWALLOW` count for `app_controller.py` is now 0. + +--- + +## Phase 4: Classify 4 INTERNAL_RETHROW + migrate 1 INTERNAL_OPTIONAL_RETURN + +Focus: the smaller, judgment-required categories. Each is a per-site decision. + +### Task 4.1: Classify the 2 `__getattr__` rethrow sites +- **WHERE:** `src/app_controller.py:1225 (__getattr__)`, `:1251 (__getattr__)` +- **WHAT:** Read the snippet + 3 lines of context. Determine pattern: + - If catching + re-raising the SAME exception: SUSPICIOUS, migrate to Result. + - If catching + re-raising as a different type (e.g., AttributeError → KeyError): legitimate, stay. + - If catching + adding context (logging) + re-raising: legitimate, stay; add `logging.debug` per Heuristic #19. +- **SAFETY:** `__getattr__` is part of Python's attribute lookup protocol. Removing the try/except changes the behavior for `hasattr` and other introspection. The migration must preserve the lookup semantics. +- **VERIFY:** `tests/test_app_getattr_hasattr_bug.py` and `tests/test_app_controller_getattr_ui_bug.py` pass. +- **COMMIT:** `refactor(app_controller): classify __getattr__ rethrow sites (Pattern 1/2/3 or migrate)` +- **GIT NOTE:** Per-site rationale documented in the commit body. + +### Task 4.2: Classify the 2 `load_context_preset` rethrow sites +- **WHERE:** `src/app_controller.py:2983 (load_context_preset)`, `:2986 (load_context_preset)` +- **WHAT:** Same pattern analysis as 4.1 +- **VERIFY:** Context preset tests pass. +- **COMMIT:** `refactor(app_controller): classify load_context_preset rethrow sites (Pattern 1/2/3 or migrate)` +- **GIT NOTE:** Per-site rationale documented in the commit body. + +### Task 4.3: Migrate the `cold_start_ts` Optional site +- **WHERE:** `src/app_controller.py:1358 (cold_start_ts)` +- **WHAT:** Read the call sites to determine the right shape (nil-sentinel vs `Result[int]`). Then implement per FR4. +- **HOW:** + 1. Grep for `cold_start_ts` call sites (expect 1-3). + 2. For each call site, determine if it uses `if x is not None:` or has separate "set" vs "missing" semantics. + 3. If "set vs missing" matters: use `Result[int]`. + 4. If "zero is a valid value": use a frozen `@dataclass ColdStartTs: value: int = 0; set: bool = False; NIL_COLD_START_TS = ColdStartTs()`. + 5. If neither: use `Optional[int]` → `Result[int]` (the convention says `Optional[T]` for "might fail" is an anti-pattern). +- **VERIFY:** Warmup tests pass. +- **COMMIT:** `refactor(app_controller): migrate cold_start_ts from Optional[int] to Result[int] (per call-site shape)` +- **GIT NOTE:** Shape chosen based on call-site semantics. + +### Task 4.4: Phase 4 checkpoint commit +- **COMMIT:** `conductor(plan): mark Phase 4 complete (4 INTERNAL_RETHROW classified, 1 INTERNAL_OPTIONAL_RETURN migrated)` +- **GIT NOTE:** Phase 4 = 5 sites (4 rethrow + 1 optional) resolved; the audit's `INTERNAL_RETHROW` and `INTERNAL_OPTIONAL_RETURN` counts for `app_controller.py` are now 0. + +--- + +## Phase 5: Verify, document, end-of-track report + +Focus: confirm all 45 migration-target sites are migrated; re-run batched suite; write the end-of-track report. + +### Task 5.1: Re-run audit and confirm zero migration sites +- **COMMAND:** `uv run python scripts/audit_exception_handling.py --by-size` +- **EXPECT:** `src/app_controller.py (V=15, S=0, ?=0, C=4, total=19)` — the 15 BOUNDARY_FASTAPI + 2 BOUNDARY_SDK + 4 INTERNAL_COMPLIANT + 1 INTERNAL_PROGRAMMER_RAISE = 22 stay (the audit may bucket BOUNDARY_FASTAPI and BOUNDARY_SDK differently — verify the actual count structure). +- **COMMIT:** No commit (verification only). + +### Task 5.2: Run targeted tests +- **COMMAND:** `uv run python -m pytest tests/test_app_controller_result.py tests/test_app_controller_offloading.py tests/test_tool_presets_execution.py tests/test_extended_sims.py tests/test_audit_exception_handling_heuristics.py -v` +- **EXPECT:** All pass. +- **COMMIT:** No commit (verification only). + +### Task 5.3: Run the full batched suite +- **COMMAND:** `uv run python scripts/run_tests_batched.py` +- **EXPECT:** 882 passed / 17 skipped / 2 xfailed (same as before this track, except the 2 previously-failing tests now pass). +- **COMMIT:** No commit (verification only). +- **NOTE:** If new failures appear, fix forward or skip with documented reason (per the "Report-Instead-of-Fix" anti-pattern rule: do not commit a fix that has only been verified in isolation). + +### Task 5.4: Add audit-heuristics tests for the 2 new app_controller categories +- **WHERE:** `tests/test_audit_exception_handling_heuristics.py` (existing file) +- **WHAT:** Add 2 tests: + 1. `test_app_controller_post_migration_has_zero_broad_catch` — runs the audit and asserts that the 32 INTERNAL_BROAD_CATCH sites are gone (or re-classified to COMPLIANT). + 2. `test_app_controller_post_migration_has_zero_silent_swallow` — same for the 8 INTERNAL_SILENT_SWALLOW sites. +- **SAFETY:** The audit script may emit transient counts during the migration; these tests are run only at the end of Phase 5 (after all migrations land). +- **COMMIT:** `test(audit): add post-migration assertions for app_controller categories` +- **GIT NOTE:** Locks in the post-migration invariant. + +### Task 5.5: Write the end-of-track report +- **WHERE:** `docs/reports/TRACK_COMPLETION_result_migration_app_controller_20260618.md` (NEW) +- **WHAT:** 7-section markdown report (per the 2026-06-17 convention): + 1. Header (track, branch, dates, scope, commit count) + 2. Tasks completed (per phase) + 3. Audit results (pre vs post) + 4. Last 3 failures (Regression 1 + Regression 2 details) + 5. Files modified (1 source + 2 tests + 4 metadata/plan/state) + 6. Git state (`git log` summary) + 7. Recommendation (next sub-track — sub-track 4 `gui_2`) +- **COMMIT:** `docs(reports): TRACK_COMPLETION_result_migration_app_controller_20260618` +- **GIT NOTE:** End-of-track report for the user to review. + +### Task 5.6: Mark state.toml complete + update umbrella +- **WHERE:** `conductor/tracks/result_migration_app_controller_20260618/state.toml`, `conductor/tracks/result_migration_20260616/spec.md` (line 256) +- **WHAT:** + 1. `state.toml` — set `status = "completed"`, `current_phase = "complete"`. + 2. `spec.md` (umbrella) — update line 256 to reflect the actual count (45 migration + 22 stay = 67 total, NOT the estimated 22 + 34 = 56). Add a note that the audit's per-category output is the source of truth, not the T-shirt-size estimate. +- **COMMIT:** `conductor(plan): mark result_migration_app_controller_20260618 as complete; update umbrella count` +- **GIT NOTE:** Sub-track 3 complete; the umbrella's count is updated to reflect the actual scope. + +--- + +## End-of-Track Report (added 2026-06-17 convention) + +On Phase 5 completion, write `docs/reports/TRACK_COMPLETION_result_migration_app_controller_20260618.md` and update `conductor/tracks/result_migration_app_controller_20260618/state.toml` to `status = "completed"`. diff --git a/conductor/tracks/result_migration_app_controller_20260618/spec.md b/conductor/tracks/result_migration_app_controller_20260618/spec.md new file mode 100644 index 00000000..ecf77df6 --- /dev/null +++ b/conductor/tracks/result_migration_app_controller_20260618/spec.md @@ -0,0 +1,310 @@ +# Track Specification: Result Migration — Sub-Track 3 (App Controller) + +**Track ID:** `result_migration_app_controller_20260618` +**Date:** 2026-06-18 +**Priority:** A (resolves the 2 known tier-1-unit-core + tier-3-live_gui regressions; completes the app_controller arm of the umbrella `result_migration_20260616`) +**Type:** refactor (data-oriented error handling convention; no behavior change visible to users) +**Umbrella:** `result_migration_20260616` (sub-track 3 of 5) + +## Overview + +Migrate the 45 migration-target exception-handling sites in `src/app_controller.py` to the data-oriented error handling convention (Result[T] dataclasses). 22 sites stay as-is (15 FastAPI boundary handlers, 2 SDK-boundary catches in `do_post`, 4 already-compliant, 1 programmer-error raise). The migration fixes the 2 known regressions: `test_tool_presets_execution::test_tool_ask_approval` (TypeError from a half-migrated `session_logger.log_tool_call` call site) and the downstream `test_extended_sims::test_execution_sim_live` failure. + +After this track, the audit's `INTERNAL_BROAD_CATCH` / `INTERNAL_SILENT_SWALLOW` / `INTERNAL_RETHROW` / `INTERNAL_OPTIONAL_RETURN` counts for `src/app_controller.py` drop to zero. The FastAPI and SDK boundary counts (15 + 2) stay at their current values (per the "Boundary Types" section in `conductor/code_styleguides/error_handling.md`). + +## Current State Audit (as of 2026-06-18, commit 5107f3ca post-merge) + +### App controller site breakdown (via `scripts/audit_exception_handling.py`) + +``` +src\app_controller.py (V=41, S=4, ?=0, C=22, total=67) +``` + +The umbrella spec at `conductor/tracks/result_migration_20260616/spec.md:256` estimated 56 sites (35 V + 3 S + 2 ? + 16 C). The actual count is 67 because the audit script improved since the umbrella was written: + +- **Heuristic A** (added in Phase 11 of `result_migration_small_files_20260617`) re-classified 8 previously-UNCLEAR sites as `INTERNAL_SILENT_SWALLOW` (the original heuristics under-counted this category). +- **Heuristic D** (Phase 12) re-classified 1 site as `INTERNAL_OPTIONAL_RETURN` (the new line was not anticipated in the umbrella). +- The 2 UNCLEAR sites at `app_controller.py:1842` and `:1668` (from sub-track 1) are now both COMPLIANT — no migration needed. + +### Migration scope (45 sites) + +| Category | Count | Treatment | +|---|---|---| +| `INTERNAL_BROAD_CATCH` | 32 | Catch specific exception + return `Result[T]` (or nil-sentinel for void) per Pattern 3 ("Fail early") | +| `INTERNAL_SILENT_SWALLOW` | 8 | Add `logging.debug(..., extra={"source": "ctx"})` per Heuristic #19; convert return to `Result[T]` | +| `INTERNAL_RETHROW` | 4 | Classify as Pattern 1/2/3; if SUSPICIOUS, convert to `Result[T]` propagation | +| `INTERNAL_OPTIONAL_RETURN` | 1 | Replace `Optional[T]` with `Result[T]` or nil-sentinel dataclass | +| **Total migration** | **45** | | + +### Migration-target site list (line numbers + ctx) + +The 32 `INTERNAL_BROAD_CATCH` sites: + +``` +L 537 _handle_custom_callback +L 579 _handle_click +L 1420 _update_inject_preview +L 1480 _do_rag_sync +L 1669 _process_pending_gui_tasks +L 1947 replace_ref +L 2046 cb_load_prior_log +L 2068 cb_load_prior_log +L 2081 cb_load_prior_log +L 2129 run_manual_prune +L 2140 _load_active_project +L 2154 _load_active_project +L 2195 run_prune +L 2767 _do_project_switch +L 2779 _do_project_switch +L 2890 _refresh_from_project +L 2944 _save_active_project +L 3057 _run +L 3084 do_fetch +L 3094 do_fetch +L 3434 worker +L 3471 worker +L 3542 worker +L 3635 _handle_request_event +L 3648 _handle_request_event +L 4070 _bg_task +L 4100 _bg_task +L 4237 _start_track_logic +L 4349 _cb_run_conductor_setup +L 4446 _cb_load_track +L 4475 _push_mma_state_update +L 4504 _load_active_tickets +``` + +The 8 `INTERNAL_SILENT_SWALLOW` sites: + +``` +L 751 _on_sigint +L 756 _install_sigint_exit_handler +L 1294 mark_first_frame_rendered +L 1376 _on_warmup_complete_for_timeline +L 1566 mcp_config_json +L 2389 queue_fallback +L 4098 _bg_task +L 4192 _start_track_logic +``` + +The 4 `INTERNAL_RETHROW` sites: + +``` +L 1225 __getattr__ +L 1251 __getattr__ +L 2983 load_context_preset +L 2986 load_context_preset +``` + +The 1 `INTERNAL_OPTIONAL_RETURN` site: + +``` +L 1358 cold_start_ts +``` + +### Sites that stay as-is (22) + +| Category | Count | Lines | Why | +|---|---|---|---| +| `BOUNDARY_FASTAPI` | 15 | 96, 99, 213, 215, 239, 253, 285, 309, 312, 320, 341, 369, 380, 401, 402 | FastAPI exception handlers; per the "Boundary Types" section in `conductor/code_styleguides/error_handling.md`, HTTP-layer exceptions stay as exceptions because FastAPI's exception-handler middleware is the SDK boundary. | +| `BOUNDARY_SDK` | 2 | 3291, 3313 (`do_post`) | SDK-boundary catches; per the same styleguide section, these are converted to `ErrorInfo` only if a Result return is feasible. `do_post` does not return Result (it's an internal helper), so the catch stays. | +| `INTERNAL_COMPLIANT` | 4 | 1843, 2066, 2763, 3744 | Already compliant per the audit's heuristics. | +| `INTERNAL_PROGRAMMER_RAISE` | 1 | 3124 | `raise ValueError` on a known-bad code path; per the styleguide, programmer errors stay as exceptions. | +| **Total stay** | **22** | | | + +### Known regressions this track fixes + +The `INTERNAL_RETHROW` and `INTERNAL_SILENT_SWALLOW` migrations surface 2 test failures that block the batched suite: + +**Regression 1: `tests/test_tool_presets_execution.py::test_tool_ask_approval` (tier-1-unit-core)** + +``` +src/app_controller.py:3723: in _offload_entry_payload + filename = Path(ref_path).name +TypeError: expected str, bytes or os.PathLike object, not Result +``` + +`session_logger.log_tool_call` (in `src/session_logger.py:205`) was partially migrated to return `Result[data=str(...)]` but the call site at `app_controller.py:3715, 3721` still does `Path(ref_path).name` expecting a string. The migration in this track updates the call site to unwrap the Result (per the convention's "AND over OR" pattern): + +```python +# Before (broken): +ref_path = session_logger.log_tool_call(script, "LOG_ONLY", None) +if ref_path: + filename = Path(ref_path).name + payload["script"] = f"[REF:{filename}]" + +# After: +ref_result = session_logger.log_tool_call(script, "LOG_ONLY", None) +if ref_result.ok and ref_result.data: + filename = Path(ref_result.data).name + payload["script"] = f"[REF:{filename}]" +elif ref_result.errors: + logging.debug("offload failed: %s", ref_result.errors[0].ui_message()) +``` + +**Regression 2: `tests/test_extended_sims.py::test_execution_sim_live` (tier-3-live_gui)** + +``` +[ABORT] Execution simulation aborted due to persistent GUI error: error +``` + +This is a downstream effect of Regression 1: the live GUI runs the same `_offload_entry_payload` path during script execution; the offload crashes, the AI status flips to "error", the simulation aborts. Fixes itself once Regression 1 is fixed. + +### Already Implemented (DO NOT re-implement) + +- The data-oriented error handling convention: `src/result_types.py` defines `Result[T]`, `ErrorInfo`, `ErrorKind`, nil-sentences (`NIL_PATH`, `NIL_RAG_STATE`, `OK`). +- The audit script: `scripts/audit_exception_handling.py` (the canonical migration site detector with 10 categories). +- The 3 refactored baseline files (already migrated to Result[T]): `src/mcp_client.py`, `src/ai_client.py`, `src/rag_engine.py`. +- Sub-track 2 (`result_migration_small_files_20260617`, shipped 2026-06-17 with Phase 13 complete) — the 16 small files (`outline_tool.py`, `summarize.py`, `shell_runner.py`, `log_registry.py`, `summary_cache.py`, `warmup.py`, `api_hooks.py`, `models.py`, `project_manager.py`, `orchestrator_pm.py`, `hot_reloader.py`, `file_cache.py`, `markdown_helper.py`, `theme_models.py`, `conductor_tech_lead.py`, `log_pruner.py`) were migrated. Their `__pycache__/` and `artifacts/` audit data is the reference for the migration patterns. +- The 5-file-commit pattern from `doeh_test_thinking_cleanup_20260615`: 1 source + 1 test + 1 plan + 1 metadata + 1 state per task. Not 11 separate test mocks for 11 sites. + +## Goals + +1. **Zero migration-target sites in `src/app_controller.py` after this track.** Audit re-run shows `INTERNAL_BROAD_CATCH` + `INTERNAL_SILENT_SWALLOW` + `INTERNAL_RETHROW` + `INTERNAL_OPTIONAL_RETURN` all = 0 for `app_controller.py`. +2. **22 stay-as-is sites stay as-is.** The boundary classification (15 FastAPI + 2 SDK + 4 compliant + 1 programmer-raise) is preserved. +3. **The 2 known test regressions are fixed.** `test_tool_ask_approval` and `test_execution_sim_live` pass. +4. **No new regressions.** The batched suite shows the same 882 passed / 17 skipped / 2 xfailed as before this track (the 1 currently-failing test_tool_ask_approval + 1 currently-failing test_execution_sim_live turn green, no new failures). +5. **The migration uses the 5 conventions** from `conductor/code_styleguides/error_handling.md`: nil-sentinel dataclasses, zero-init, fail early, AND over OR, error-info side-channel. + +## Functional Requirements + +**FR1. Migrate 32 INTERNAL_BROAD_CATCH sites to `Result[T]` propagation.** + +For each site: +- Read the snippet + 2-3 lines of context (`get_file_slice`). +- Replace `try: ... except Exception as e: pass # broad swallow` with: + ```python + try: + ... + except as e: + return Result(data=default, errors=[ErrorInfo( + kind=ErrorKind.INTERNAL, + message=str(e), + source="", + original=e, + )]) + ``` +- The return type may change from `None` to `Result[None]` (use `OK` for the success case), or from a specific type to `Result[T]`. +- Add `from src.result_types import Result, ErrorInfo, ErrorKind` at the top of `app_controller.py` if not already present. + +**FR2. Migrate 8 INTERNAL_SILENT_SWALLOW sites with logging per Heuristic #19.** + +For each site: +- Add `logging.debug("swallowed exception: %s", e, extra={"source": "ctx"})` before the `pass` or `return None`. +- Convert the return to `Result[T]` per FR1's pattern (the `errors=[ErrorInfo(...)]` side-channel carries the swallowed exception). + +**FR3. Classify 4 INTERNAL_RETHROW sites.** + +For each site, determine the pattern: +- **Pattern 1** (catch + convert + raise as different type): legitimate. Stay as-is. +- **Pattern 2** (catch + log + re-raise): legitimate. Add `logging.debug` for visibility, but the raise stays. +- **Pattern 3** (catch + cleanup + re-raise): legitimate. Stay as-is. +- **SUSPICIOUS** (catch + re-raise the same exception): migration-target. Convert to Result-based; remove the try/except. + +The 4 sites (lines 1225, 1251, 2983, 2986) are in `__getattr__` and `load_context_preset`. Tier 2 reads each and classifies per the pattern. The Phase 1 plan task walks through this. + +**FR4. Migrate 1 INTERNAL_OPTIONAL_RETURN site (L1358 `cold_start_ts`).** + +Replace `Optional[int]` with a nil-sentinel dataclass or `Result[int]`: +- If the return value is consumed by code that uses `if x is not None:`, use a frozen `@dataclass` (e.g., `class ColdStartTs: value: int = 0; set: bool = False; NIL_COLD_START_TS = ColdStartTs()`). +- If the return value is consumed by code that needs to distinguish "missing" from "zero", use `Result[int]`. +- Tier 2 picks the right shape based on the 1-2 call sites. + +**FR5. Fix the half-migrated `session_logger.log_tool_call` call site (Regression 1).** + +In `src/app_controller.py:_offload_entry_payload`: +- Update the 2 `ref_path = session_logger.log_tool_output(...)` / `log_tool_call(...)` calls to unwrap the `Result`: + ```python + ref_result = session_logger.log_tool_output(output) + if ref_result.ok and ref_result.data: + filename = Path(ref_result.data).name + payload["output"] = f"[REF:{filename}]" + elif ref_result.errors: + logging.debug("offload failed: %s", ref_result.errors[0].ui_message()) + ``` +- Do NOT change `src/session_logger.py` (the migration is at the call site per convention). + +**FR6. Add tests for the new Result-based API (1 new test file + selective updates).** + +Create `tests/test_app_controller_result.py` (modeled on `tests/test_ai_client_result.py`): +- 5+ tests verifying Result return types and error side-channels for the migrated methods +- 3+ tests verifying the `log_tool_call` / `log_tool_output` unwrapping in `_offload_entry_payload` +- 1 test verifying Regression 2 (`test_execution_sim_live`) end-to-end behavior + +Update `tests/test_app_controller_offloading.py`: +- 1 test verifying the unwrapped path stores a `[REF:filename]` correctly when offload succeeds +- 1 test verifying a debug log is emitted when offload fails + +**FR7. Preserve the 22 stay-as-is sites.** + +Do NOT touch any of the 22 sites listed above. The FastAPI handlers, SDK-boundary catches, compliant sites, and programmer-raise must remain exception-based. Add a comment at the top of each handler citing the styleguide section ("Per `conductor/code_styleguides/error_handling.md` §'Boundary Types'"). + +**FR8. Per-task atomic commits with the 5-file pattern.** + +Each task touches 5 files (per `doeh_test_thinking_cleanup_20260615`): +1. `src/app_controller.py` (the source change) +2. `tests/test_app_controller_result.py` (new test) or `tests/test_app_controller_offloading.py` (update) +3. `conductor/tracks/result_migration_app_controller_20260618/plan.md` (mark task `[x] `) +4. `conductor/tracks/result_migration_app_controller_20260618/metadata.json` (update scope counters) +5. `conductor/tracks/result_migration_app_controller_20260618/state.toml` (mark task `completed`) + +Not 11 separate test mocks for 11 sites. One combined test for each Result-returning method (e.g., `_offload_entry_payload` returns Result, test the unwrap path). + +## Non-Functional Requirements + +- **No new dependencies.** `Result`, `ErrorInfo`, `ErrorKind` are in `src/result_types.py` (already imported by other modules). +- **No changes to the public API.** The `_predefined_callbacks` and `_gettable_fields` Hook API registries stay identical (no callback signature changes; the internal Result types are hidden from the API surface). +- **Thread safety preserved.** `app_controller.py` uses `threading.Lock` for several state dicts (`_pending_gui_tasks_lock`, `_api_event_queue_lock`, etc.). The migration does not change lock semantics. +- **Hot reload compatibility.** Per the umbrella spec, the `src/app_controller.py` changes are exercised through the hot-reload mechanism (`Ctrl+Alt+R`). The user can verify each batch visually if desired. + +## Architecture Reference + +- **`conductor/code_styleguides/error_handling.md`** — the 5 patterns (Nil-Sentinel, Zero-Init, Fail Early, AND over OR, Error Info as Side-Channel), the data model (`Result[T]`, `ErrorInfo`, `ErrorKind`), the decision tree, and the "Boundary Types" section that determines which sites stay as exceptions. +- **`conductor/tracks/result_migration_20260616/spec.md:254-274`** — the umbrella's sub-track 3 description. The current scope (45 migration + 22 stay) is BIGGER than the umbrella estimated (22 + 34) because the audit script improved. +- **`conductor/tracks/result_migration_20260616/plan.md:101-200`** — sub-track 2's plan (the small-files migration that this sub-track parallels). The phase structure (Setup → Migrate → Test → Document → Verify) is the template. +- **`conductor/tracks/result_migration_small_files_20260617/spec.md`** — the shipped sub-track 2. Look at the actual commits to see the 5-file pattern in action. +- **`docs/guide_architecture.md`** — the threading model (background threads, `_pending_gui_tasks` queue, `_pending_tool_calls_lock`). +- **`docs/guide_app_controller.md`** — the app_controller architecture (Hook API, MMA conductor, RAG integration). +- **`docs/guide_testing.md`** — the test patterns (Result-based assertions, mock patterns, live_gui fixture). + +## Out of Scope + +- The 3 refactored baseline files (`mcp_client.py`, `ai_client.py`, `rag_engine.py`) — already done. +- The 16 small files (sub-track 2) — already done. +- `src/gui_2.py` (260KB; 55 sites) — sub-track 4. **Not** part of this track. +- The 5 baseline files' remaining 77 violations (sub-track 5) — not part of this track. +- Migration of `session_logger.log_tool_call` to a fully Result-based signature — the half-migrated state is intentional; the convention is that call sites unwrap, not that every function returns Result. The migration at the call site in `_offload_entry_payload` (FR5) is the canonical fix. +- The MMA conductor and RAG engine's Result propagation (the upstream of `app_controller`) — they're already Result-based; the work in this track is downstream consumption. +- Tier 4 QA hooks — the QA callback in `app_controller:_on_comms_entry` is already Result-aware; no change needed. + +## Test Inventory (after this track) + +| Test file | Type | Status | Tests | +|---|---|---|---| +| `tests/test_app_controller_result.py` (NEW) | unit | default-on | 5+ Result return type tests | +| `tests/test_app_controller_offloading.py` | unit | default-on | +2 unwrap path tests | +| `tests/test_tool_presets_execution.py` | unit | default-on | `test_tool_ask_approval` (currently FAILING → fixed) | +| `tests/test_extended_sims.py` | integration | default-on, opt-in `tier-3-live_gui` | `test_execution_sim_live` (currently FAILING → fixed) | +| `tests/test_audit_exception_handling_heuristics.py` | unit | default-on | +2 new heuristics (INTERNAL_OPTIONAL_RETURN for app_controller; INTERNAL_RETHROW Pattern 3) | +| `scripts/audit_exception_handling.py` | static analyzer | default-on | re-classified counts | + +The post-track batched suite: same 882 passed / 17 skipped / 2 xfailed (the 1 currently-failing + 1 currently-failing both turn green; no new failures introduced). + +## Verification Criteria + +- `uv run python scripts/audit_exception_handling.py --by-size` shows `src/app_controller.py (V=0, S=0, ?=0, C=37, total=37)` after the track (the new total = 15 BOUNDARY_FASTAPI + 2 BOUNDARY_SDK + 4 INTERNAL_COMPLIANT + 1 INTERNAL_PROGRAMMER_RAISE = 22 stay + 15 stay = ... let me recompute: 22 stay + 0 migration = 22 total? no, the audit's `C` count includes both `INTERNAL_COMPLIANT` AND the `BOUNDARY_*` classes are NOT counted as violations; they show up as C. +- Actually the audit's `compliant_sites` count includes only `INTERNAL_COMPLIANT` (4). The `BOUNDARY_FASTAPI` (15) and `BOUNDARY_SDK` (2) are in `violations`? Let me re-check the audit. If the post-track count is `V=15, S=0, ?=0, C=4, total=19` (just the FastAPI + SDK + INTERNAL_COMPLIANT + PROGRAMMER_RAISE = 19 + 2 SDK + 4 COMPLIANT + 1 PROGRAMMER_RAISE = 26), that's the target. Wait I need to verify the actual count structure. +- The user's regression check (post-track): `uv run python scripts/run_tests_batched.py` shows 882 passed / 17 skipped / 2 xfailed (1 new from this track or maintained from before). +- `tests/test_app_controller_result.py` exists and all 5+ tests pass. +- `tests/test_app_controller_offloading.py` has the 2 new unwrap tests and all pass. +- The `_offload_entry_payload` test path is exercised end-to-end (via `test_tool_ask_approval`). +- The 22 stay-as-is sites are not modified (verified by `git diff src/app_controller.py | grep -E "L 96|L 99|L 213|..."` showing no changes at those line ranges; the line numbers may shift slightly as code is added/removed, so the verification is by `context` name not line number). + +## Risk Register + +- **R1:** The migration may break the 17 currently-skipped live_gui tests (the ones that require the GUI to be running). Mitigation: re-run live_gui suite at the end of Phase 5; if new failures appear, fix forward or skip with documented reason. +- **R2:** The `INTERNAL_RETHROW` classification for `__getattr__` (L1225, L1251) is unusual — `__getattr__` should re-raise to support Python's attribute lookup protocol. Mitigation: the convention's "Fail early" pattern says programmer errors stay as exceptions; Tier 2 documents the rationale per site. +- **R3:** The 1 `INTERNAL_OPTIONAL_RETURN` site (L1358 `cold_start_ts`) has multiple call sites. The shape (nil-sentinel vs Result) depends on how the call sites use the value. Tier 2 reads the call sites and picks the right shape. +- **R4:** The `log_tool_call` call site in `_offload_entry_payload` (FR5) is the regression that's blocking the batched suite. It's also the FIRST thing Tier 2 should fix (in Phase 1 Task 1.x) to unblock the regression check. +- **R5:** Scope is larger than the umbrella estimated (45 vs 22 migration). Mitigation: the umbrella spec is updated post-track to reflect the actual count; the audit's per-category output is the source of truth, not the umbrella's T-shirt-size estimate. diff --git a/conductor/tracks/result_migration_app_controller_20260618/state.toml b/conductor/tracks/result_migration_app_controller_20260618/state.toml new file mode 100644 index 00000000..9acfce92 --- /dev/null +++ b/conductor/tracks/result_migration_app_controller_20260618/state.toml @@ -0,0 +1,70 @@ +# Track state for result_migration_app_controller_20260618 +# Updated by Tier 2 Tech Lead as tasks complete + +[meta] +track_id = "result_migration_app_controller_20260618" +name = "Result Migration - Sub-Track 3 (App Controller)" +status = "active" +current_phase = 0 +last_updated = "2026-06-18" +umbrella = "result_migration_20260616" +sub_track_index = 3 + +[blocked_by] +result_migration_small_files_20260617 = "shipped 2026-06-17" + +[blocks] +result_migration_gui_2_ = "blocked by this track; will be planned after Phase 5 completion" + +[phases] +phase_1 = { status = "pending", checkpointsha = "", name = "Setup + Fix the regression (test_tool_ask_approval + test_execution_sim_live)" } +phase_2 = { status = "pending", checkpointsha = "", name = "Migrate the 32 INTERNAL_BROAD_CATCH sites (4 bulk batches)" } +phase_3 = { status = "pending", checkpointsha = "", name = "Migrate the 8 INTERNAL_SILENT_SWALLOW sites (with logging.debug per Heuristic #19)" } +phase_4 = { status = "pending", checkpointsha = "", name = "Classify 4 INTERNAL_RETHROW + migrate 1 INTERNAL_OPTIONAL_RETURN" } +phase_5 = { status = "pending", checkpointsha = "", name = "Verify, document, end-of-track report" } + +[tasks] +# Phase 1: Setup + Fix the regression +t1_1 = { status = "pending", commit_sha = "", description = "Create sub-track folder (spec.md exists; plan.md, metadata.json, state.toml)" } +t1_2 = { status = "pending", commit_sha = "", description = "Update conductor/tracks.md with the new sub-track row" } +t1_3 = { status = "pending", commit_sha = "", description = "Fix _offload_entry_payload call site in src/app_controller.py:3709-3725 (unwrap Result from log_tool_output/log_tool_call)" } +t1_4 = { status = "pending", commit_sha = "", description = "Add 2 unwrap-path tests in tests/test_app_controller_offloading.py" } +t1_5 = { status = "pending", commit_sha = "", description = "Run targeted regression test (test_tool_ask_approval + test_execution_sim_live); verify both pass" } +t1_6 = { status = "pending", commit_sha = "", description = "Phase 1 checkpoint commit" } + +# Phase 2: Migrate 32 INTERNAL_BROAD_CATCH sites +t2_1 = { status = "pending", commit_sha = "", description = "Create tests/test_app_controller_result.py with 5 scaffolding tests" } +t2_2 = { status = "pending", commit_sha = "", description = "Migrate batch 1: 5 callback-handler sites (L537, L579, L2046, L2068, L2081)" } +t2_3 = { status = "pending", commit_sha = "", description = "Migrate batch 2: 6 project-op sites (L2129, L2140, L2154, L2195, L2890, L2944)" } +t2_4 = { status = "pending", commit_sha = "", description = "Migrate batch 3: 8 conductor/track sites (L3057, L3084, L3094, L4237, L4349, L4446, L4475, L4504)" } +t2_5 = { status = "pending", commit_sha = "", description = "Migrate batch 4: 11 worker/task sites (L3434, L3471, L3542, L3635, L3648, L4070, L4100, L1669, L1420, L1480, L1947)" } +t2_6 = { status = "pending", commit_sha = "", description = "Phase 2 checkpoint commit" } + +# Phase 3: Migrate 8 INTERNAL_SILENT_SWALLOW sites +t3_1 = { status = "pending", commit_sha = "", description = "Migrate batch 1: 4 SIGINT/timeline sites (L751, L756, L1294, L1376)" } +t3_2 = { status = "pending", commit_sha = "", description = "Migrate batch 2: 4 MCP/worker sites (L1566, L2389, L4098, L4192)" } +t3_3 = { status = "pending", commit_sha = "", description = "Phase 3 checkpoint commit" } + +# Phase 4: Classify 4 INTERNAL_RETHROW + migrate 1 INTERNAL_OPTIONAL_RETURN +t4_1 = { status = "pending", commit_sha = "", description = "Classify the 2 __getattr__ rethrow sites (L1225, L1251) per Pattern 1/2/3 or migrate" } +t4_2 = { status = "pending", commit_sha = "", description = "Classify the 2 load_context_preset rethrow sites (L2983, L2986) per Pattern 1/2/3 or migrate" } +t4_3 = { status = "pending", commit_sha = "", description = "Migrate cold_start_ts from Optional[int] to Result[int] or nil-sentinel (L1358)" } +t4_4 = { status = "pending", commit_sha = "", description = "Phase 4 checkpoint commit" } + +# Phase 5: Verify, document, end-of-track report +t5_1 = { status = "pending", commit_sha = "", description = "Re-run audit_exception_handling.py; confirm 0 migration sites in src/app_controller.py" } +t5_2 = { status = "pending", commit_sha = "", description = "Run targeted tests (test_app_controller_result, test_app_controller_offloading, test_tool_presets_execution, test_extended_sims, test_audit_exception_handling_heuristics)" } +t5_3 = { status = "pending", commit_sha = "", description = "Run the full batched suite; confirm no new regressions" } +t5_4 = { status = "pending", commit_sha = "", description = "Add 2 post-migration invariant tests in test_audit_exception_handling_heuristics.py" } +t5_5 = { status = "pending", commit_sha = "", description = "Write docs/reports/TRACK_COMPLETION_result_migration_app_controller_20260618.md" } +t5_6 = { status = "pending", commit_sha = "", description = "Mark state.toml complete; update umbrella spec count to reflect actual scope (45 migration + 22 stay = 67 total)" } + +[verification] +phase_1_complete = false +phase_2_complete = false +phase_3_complete = false +phase_4_complete = false +phase_5_complete = false +regression_1_fixed = false +regression_2_fixed = false +batched_suite_no_new_regressions = false