move tracks to archive
This commit is contained in:
@@ -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) |
|
||||
|
||||
@@ -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)"
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -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 <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"`.
|
||||
@@ -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 <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.
|
||||
@@ -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_<YYYYMMDD> = "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
|
||||
Reference in New Issue
Block a user