# 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

- [x] **Task 1.3: Fix `_offload_entry_payload` call site (Regression 1)** [26e57577]

### 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.

- [x] **Task 1.4: Add test for the unwrap path** [4b07e934]

### 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.

- [x] **Task 1.5: Run the regression test and confirm both fixes** [7b823fd0]

### 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).

- [x] **Task 1.6: Phase 1 checkpoint commit** [7b823fd0]

### 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 <SpecificException> 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": "<ctx>"})`; 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"`.
