# 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 <SpecificException> as e:
      return Result(data=default, errors=[ErrorInfo(
          kind=ErrorKind.INTERNAL,
          message=str(e),
          source="<ctx>",
          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] <sha>`)
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.
