diff --git a/conductor/tracks/result_migration_app_controller_20260618/metadata.json b/conductor/tracks/result_migration_app_controller_20260618/metadata.json index 6e0bdbea..36f8bf09 100644 --- a/conductor/tracks/result_migration_app_controller_20260618/metadata.json +++ b/conductor/tracks/result_migration_app_controller_20260618/metadata.json @@ -28,7 +28,9 @@ "conductor/tracks/result_migration_app_controller_20260618/metadata.json", "conductor/tracks/result_migration_app_controller_20260618/plan.md", "conductor/tracks/result_migration_app_controller_20260618/spec.md", - "conductor/tracks/result_migration_20260616/spec.md" + "conductor/tracks/result_migration_20260616/spec.md", + "scripts/audit_exception_handling.py", + "tests/test_audit_heuristics.py" ], "deleted_files": [] }, @@ -51,7 +53,10 @@ "Tier 2 batched suite (35 tests) ALL 5 batches PASS", "Tier 3 batched suite (56 tests): 1 known environmental live_gui flake (test_context_sim_live - 2s eventual consistency timeout under load); not caused by Phase 6 migration", "Every migrated except body contains Result(data=..., errors=[ErrorInfo(original=e)]) (verified by grep - no debug-log-only except bodies)", - "docs/reports/TRACK_COMPLETION_result_migration_app_controller_20260618.md rewritten with full Phase 1-6 coverage; the misleading '8 silent swallow migrated' claim from Phase 5 is superseded" + "docs/reports/TRACK_COMPLETION_result_migration_app_controller_20260618.md rewritten with full Phase 1-6 coverage; the misleading '8 silent swallow migrated' claim from Phase 5 is superseded", + "src/app_controller.py has 0 strict-violation sites after Phase 7 (L242, L256, L5064, L5093 migrated to Result[T] or no longer over-classified by audit heuristic)", + "scripts/audit_exception_handling.py _is_api_handler heuristic tightened: BOUNDARY_FASTAPI only applies when except body raises HTTPException or returns Result", + "tests/test_audit_heuristics.py has 3 unit tests verifying the tightened heuristic does not regress the 15 existing BOUNDARY_FASTAPI sites" ], "regressions_and_pre_existing_failures": [ { @@ -82,7 +87,7 @@ ], "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." + "scope": "1 source file (src/app_controller.py) + 1 audit script (scripts/audit_exception_handling.py) modified across 7 phases; 49 migration sites (45 in Phases 1-5 + 4 strict-violation sites in Phase 7); 1 new test file (test_app_controller_result.py) extended + 1 new test file (tests/test_audit_heuristics.py); 4 metadata/plan/state files; 1 end-of-track report. 25+ atomic commits (18 in Phases 1-6 + 7+ in Phase 7)." }, "risk_register": [ { @@ -129,6 +134,21 @@ "risk": "Phase 6: Scope (28 sites) is large; Phase 6 may itself need a follow-up Phase 7 if any site resists migration", "likelihood": "low", "mitigation": "Phase 6 is bounded by 8 sub-phases with concrete drain-point patterns. If a site resists migration (e.g., a function with side effects that cannot return Result), the user explicitly carves it out; no Tier 2-initiated 'follow-up' deferrals are allowed." + }, + { + "risk": "Phase 7: Heuristic tightening may regress other files' _api_* boundary sites that do not raise HTTPException", + "likelihood": "medium", + "mitigation": "FR7's 3 unit tests in tests/test_audit_heuristics.py lock the 15 existing BOUNDARY_FASTAPI sites; manual verification of src/api_hooks.py during implementation" + }, + { + "risk": "Phase 7: Legacy wrapper for _push_mma_state_update preserves fire-and-forget semantics that may mask future failures", + "likelihood": "low", + "mitigation": "Docstring deprecation note in _push_mma_state_update; follow-up track migrates callers to the _result variant" + }, + { + "risk": "Phase 7: _last_request_errors field may grow unbounded if not reset per-request", + "likelihood": "low", + "mitigation": "Verify Phase 6 added the per-request reset; add reset in _api_generate entry point if missing" } ] } diff --git a/conductor/tracks/result_migration_app_controller_20260618/plan.md b/conductor/tracks/result_migration_app_controller_20260618/plan.md index a8e48eae..28d92496 100644 --- a/conductor/tracks/result_migration_app_controller_20260618/plan.md +++ b/conductor/tracks/result_migration_app_controller_20260618/plan.md @@ -461,3 +461,84 @@ Focus: replace every `except ...: logging.debug(...); ` body ## End-of-Track Report (added 2026-06-17 convention; rewritten per Phase 6) On Phase 6 completion, rewrite `docs/reports/TRACK_COMPLETION_result_migration_app_controller_20260618.md` to cover all 6 phases. Update `conductor/tracks/result_migration_app_controller_20260618/state.toml` to `status = "completed"`, `current_phase = 6`. + +--- + +## Phase 7: Strict Enforcement Cleanup (added 2026-06-19) +Focus: 4-site migration + audit heuristic tightening (1 source file + 1 audit script + 1 new test file + 7+ atomic commits). + +**Task 7.1: Confirm the heuristic over-application** +- **WHERE:** `scripts/audit_exception_handling.py:300-410` +- **WHAT:** Read the `_is_api_handler()` definition and the classification call site at line 393-397. Confirm that the heuristic over-applies BOUNDARY_FASTAPI to ALL try/except inside `_api_*` handlers, including nested ones that only log. +- **VERIFY:** A short written summary of the bug (1-2 sentences) committed to the git note for task 7.6. +- **COMMIT:** No commit (verification only). + +**Task 7.2: Migrate L242 (RAG augmentation in `_api_generate`)** +- **WHERE:** `src/app_controller.py:232-244` +- **WHAT:** Replace the inline `try/except Exception: sys.stderr.write(...)` with a call to `_rag_search_result(user_msg)` returning `Result[str]`. On error, append to `self._last_request_errors`. +- **VERIFY:** New unit test in `tests/test_app_controller_result.py` passes (covers success path + RAG-error path); `audit_exception_handling.py` no longer classifies L242 as BOUNDARY_FASTAPI. +- **COMMIT:** `refactor(app_controller): migrate L242 RAG augmentation to _rag_search_result (Phase 7)` + +**Task 7.3: Migrate L256 (symbol resolution in `_api_generate`)** +- **WHERE:** `src/app_controller.py:246-258` +- **WHAT:** Same pattern as task 7.2 using `_symbol_resolution_result(user_msg, file_items) -> Result[str]` (Phase 6 helper). +- **VERIFY:** New unit test in `tests/test_app_controller_result.py`; `audit_exception_handling.py` no longer classifies L256 as BOUNDARY_FASTAPI. +- **COMMIT:** `refactor(app_controller): migrate L256 symbol resolution to _symbol_resolution_result (Phase 7)` + +**Task 7.4: Migrate `_push_mma_state_update`** +- **WHERE:** `src/app_controller.py:_push_mma_state_update` (the function body preceding L5064). +- **WHAT:** Extract `_push_mma_state_update_result() -> Result[None]` helper. Legacy wrapper calls `self._report_worker_error` on failure. +- **VERIFY:** New unit test in `tests/test_app_controller_result.py`; `audit_exception_handling.py` no longer classifies L5064 as INTERNAL_COMPLIANT (now BOUNDARY_CONVERSION or compliant with Result). +- **COMMIT:** `refactor(app_controller): migrate _push_mma_state_update to Result helper (Phase 7)` + +**Task 7.5: Migrate `_load_active_tickets.beads` inner** +- **WHERE:** `src/app_controller.py:5093` (inner try of `_load_active_tickets`). +- **WHAT:** Extract `_load_beads_from_path_result(beads_path) -> Result[List[Ticket]]`. Outer merges via `.with_errors()` and routes through `self._report_worker_error`. +- **VERIFY:** New unit test in `tests/test_app_controller_result.py`; `audit_exception_handling.py` no longer classifies L5093 as INTERNAL_COMPLIANT. +- **COMMIT:** `refactor(app_controller): migrate _load_active_tickets.beads to Result helper (Phase 7)` + +**Task 7.6: Tighten the audit heuristic** +- **WHERE:** `scripts/audit_exception_handling.py:319-321` AND the classification at line 393-397. +- **WHAT:** Add AST check on except body: require `ast.Raise` with `exc.func.id == "HTTPException"` OR a `return` of `Result(...)` for BOUNDARY_FASTAPI. Otherwise re-classify as INTERNAL_SILENT_SWALLOW (logging body) or INTERNAL_COMPLIANT (try/finally cleanup). +- **VERIFY:** 3 new unit tests in `tests/test_audit_heuristics.py` pass; the 15 existing BOUNDARY_FASTAPI sites remain classified. +- **COMMIT:** `fix(audit): tighten _is_api_handler BOUNDARY_FASTAPI heuristic (Phase 7)` + +**Task 7.7: Add 4 unit tests for migrated sites** +- **WHERE:** `tests/test_app_controller_result.py` (extend existing). +- **WHAT:** Add `test_l242_rag_search_returns_result`, `test_l256_symbol_resolution_returns_result`, `test_push_mma_state_update_returns_result`, `test_load_beads_from_path_returns_result`. +- **VERIFY:** All 4 tests pass; coverage for the migrated sites is locked. +- **COMMIT:** `test(app_controller_result): add Phase 7 migration tests (4 sites)` + +**Task 7.8: Add 3 regression-guard tests for the heuristic** +- **WHERE:** `tests/test_audit_heuristics.py` (new file). +- **WHAT:** Add `test_15_existing_fastapi_sites_remain_classified`, `test_4_strict_violation_sites_flagged_when_heuristic_reverted`, `test_is_api_handler_requires_http_exception_in_body`. +- **VERIFY:** All 3 tests pass; the heuristic does not regress existing BOUNDARY_FASTAPI sites. +- **COMMIT:** `test(audit_heuristics): add regression-guard tests for Phase 7 heuristic tightening` + +**Task 7.9: Run `--strict` audit and verify gate** +- **COMMAND:** `uv run python scripts/audit_exception_handling.py --src src/app_controller.py --strict` +- **VERIFY:** Exit code 0; output shows 0 INTERNAL_SILENT_SWALLOW AND 0 strict-violation sites (L242, L256, L5064, L5093). +- **COMMIT:** No commit (verification only). + +**Task 7.10: Run full 11-tier batched suite** +- **COMMAND:** `uv run python scripts/run_tests_batched.py` +- **VERIFY:** Pass count matches post-Phase-6 baseline; no new regressions. +- **NOTE:** If new failures appear, fix forward (do not loop; read code, predict, fix once, report). + +**Task 7.11: Update state.toml and metadata.json** +- **WHERE:** `conductor/tracks/result_migration_app_controller_20260618/state.toml` and `metadata.json`. +- **WHAT:** Mark all t7_* tasks complete; set `phase_7_complete = true`; add 3 risk_register entries and 3 verification_criteria entries. +- **COMMIT:** `conductor(plan): mark Phase 7 complete (4 silent-swallow sites + audit heuristic tightened)` + +**Task 7.12: Phase 7 checkpoint commit with git note** +- **COMMIT:** `conductor(checkpoint): Phase 7 strict enforcement cleanup complete` +- **GIT NOTE:** 4 silent-swallow sites migrated to proper Result[T]; audit heuristic tightened so BOUNDARY_FASTAPI only applies when except body raises HTTPException; 7+ atomic commits; `--strict` audit exits 0. + +**Task 7.13: Conductor - User Manual Verification** +- Per workflow.md "Phase Completion Verification and Checkpointing Protocol": present the audit before/after metrics and await explicit confirmation before marking the track fully complete. + +--- + +## End-of-Track Report (Phase 7 addendum) + +Append a "Phase 7 Addendum" section to `docs/reports/TRACK_COMPLETION_result_migration_app_controller_20260618.md` documenting the 4-site cleanup and the audit heuristic tightening. diff --git a/conductor/tracks/result_migration_app_controller_20260618/spec.md b/conductor/tracks/result_migration_app_controller_20260618/spec.md index 19afbff2..d25c3733 100644 --- a/conductor/tracks/result_migration_app_controller_20260618/spec.md +++ b/conductor/tracks/result_migration_app_controller_20260618/spec.md @@ -476,3 +476,114 @@ Unlike Phase 3's deferral pattern (which left 20 nested sites as "follow-up"), P - **R8 (Phase 6):** The 20 nested sites introduced by Phase 2 may have been overwritten by Phase 3's `logging.debug` add. The migration must remove the `logging.debug` AND replace with `Result` return (not add a Result on top of the logging). - **R9 (Phase 6):** Scope (28 sites) is large but bounded. Mitigation: 8 groups with clear drain patterns; each group is a sub-batch (3-5 commits per group). If a group takes too many commits, the group can be split further. +## 22. Phase 7 - Strict Enforcement Cleanup (added 2026-06-19) + +### 22.1 Background + +Phase 6 reduced INTERNAL_SILENT_SWALLOW from 30 to 0 per `audit_exception_handling.py`. However, 4 sites are classified as compliant by the audit via heuristic over-application, not by satisfying the user's principle (`error_handling.md:530`: "logging is NOT a drain"): + +| Line | Function | Audit class | Strict status | +|---|---|---|---| +| L242 | `_api_generate` (RAG) | BOUNDARY_FASTAPI | violation - sys.stderr.write only | +| L256 | `_api_generate` (symbols) | BOUNDARY_FASTAPI | violation - sys.stderr.write only | +| L5064 | `_push_mma_state_update` | INTERNAL_COMPLIANT | violation - logging + print, no Result | +| L5093 | `_load_active_tickets.beads` inner | INTERNAL_COMPLIANT | violation - logging + print, no Result | + +The audit heuristic at `scripts/audit_exception_handling.py:319-321` (`_is_api_handler()`) plus the classification at line 393-397 over-applies BOUNDARY_FASTAPI to ALL try/except inside `_api_*` handlers regardless of whether the except body raises HTTPException. Per `error_handling.md:534`, BOUNDARY_FASTAPI only applies to `raise HTTPException(...)` sites. This is the same laundering pattern that sub-track 2 Phase 10 to 11 redo addressed. + +### 22.2 Goals + +1. Migrate the 4 strict-violation sites to proper Result[T] propagation using the Phase 6 helpers already in the file. +2. Tighten the audit heuristic so future sites are not over-classified. +3. Add regression tests that lock in the correct behavior. + +### 22.3 Functional Requirements + +- **FR1** `src/app_controller.py:232-244` (RAG augmentation in `_api_generate`) calls the existing `_rag_search_result(user_msg)` helper (Phase 6 Group 6.5/6.6) returning `Result[str]`. On error, append to `self._last_request_errors`. The outer `_api_generate` raises `HTTPException` with accumulated errors on subsequent API failure. +- **FR2** `src/app_controller.py:246-258` (symbol resolution in `_api_generate`) calls the existing `_symbol_resolution_result(user_msg, file_items)` helper. Same accumulation pattern. +- **FR3** `src/app_controller.py:_push_mma_state_update` is split: new `_push_mma_state_update_result()` returning `Result[None]`; legacy wrapper preserves fire-and-forget but routes errors through `self._report_worker_error`. +- **FR4** `src/app_controller.py:_load_active_tickets` inner-beads try/except is extracted to `_load_beads_from_path_result()` returning `Result[List[Ticket]]`; outer merges errors via `.with_errors()` and routes through `self._report_worker_error`. + +- **FR5** `scripts/audit_exception_handling.py:319-321` (`_is_api_handler`) and line 393-397 (classification): BOUNDARY_FASTAPI applies ONLY when the except body actually contains `ast.Raise(exc=HTTPException(...))` OR returns a Result propagated to the caller. Otherwise re-classify as INTERNAL_SILENT_SWALLOW if the body has logging, or INTERNAL_COMPLIANT if it is `try/finally` cleanup. +- **FR6** 4 unit tests in `tests/test_app_controller_result.py` verify each migrated site returns Result[T] with proper error propagation. +- **FR7** 3 unit tests in a new `tests/test_audit_heuristics.py` verify (a) the 15 existing BOUNDARY_FASTAPI sites in `src/api_hooks.py` and `src/app_controller.py` remain classified correctly, (b) the 4 strict-violation sites ARE flagged when the heuristic is reverted to old behavior (regression-guard), (c) `_is_api_handler` requires HTTPException raise in except body. + +### 22.4 Non-Functional Requirements + +- **NFR1** `audit_exception_handling.py --src src/app_controller.py --strict` exits 0. +- **NFR2** Without `--strict`, 0 INTERNAL_SILENT_SWALLOW AND 0 strict-violation sites (L242, L256, L5064, L5093) reported. +- **NFR3** Full 11-tier batched suite passes; no new regressions vs post-Phase-6 baseline. +- **NFR4** 1-space indentation per `product-guidelines.md`. +- **NFR5** Per-file atomic commits; no batching. + +### 22.5 Per-Site Migration Patterns + +#### 22.5.1 L242 - RAG search in `_api_generate` + +**WHERE:** `src/app_controller.py:232-244` + +**HOW:** Replace the inline `try/except Exception: sys.stderr.write(...)` with a call to `_rag_search_result(user_msg)` (Phase 6 helper) returning `Result[str]`. On error, append to `self._last_request_errors`. The user sees degraded context (no RAG) but the failure is visible. + +**SAFETY:** `_last_request_errors` is the field added in Phase 6 Group 6.6. If Phase 6 did not add a lock, add `self._last_request_errors_lock = threading.Lock()` and acquire it on every append and on reset. + +#### 22.5.2 L256 - Symbol resolution in `_api_generate` + +**WHERE:** `src/app_controller.py:246-258` + +**HOW:** Same pattern as 22.5.1 using `_symbol_resolution_result(user_msg, file_items) -> Result[str]` (Phase 6 helper). + +**SAFETY:** Same as 22.5.1. + +#### 22.5.3 L5064 - `_push_mma_state_update` + +**WHERE:** `src/app_controller.py:_push_mma_state_update` (function body preceding L5064). + +**HOW:** Extract a `_push_mma_state_update_result() -> Result[None]` helper; legacy wrapper calls `self._report_worker_error` on failure. + +**SAFETY:** Called from MMA worker thread per `docs/guide_multi_agent_conductor.md`. Legacy wrapper preserves fire-and-forget semantics for existing callers; new code should use the `_result` variant. + +#### 22.5.4 L5093 - `_load_active_tickets.beads` inner + +**WHERE:** `src/app_controller.py:5093` (inside the outer try of `_load_active_tickets`). + +**HOW:** Extract `_load_beads_from_path_result(beads_path) -> Result[List[Ticket]]`; outer `_load_active_tickets` merges errors via `.with_errors()` and routes through `self._report_worker_error`. + +**SAFETY:** Main-thread only per existing callers; no thread-safety concerns. + +#### 22.5.5 FR5 - Audit heuristic tightening + +**WHERE:** `scripts/audit_exception_handling.py:319-321` (`_is_api_handler`) AND the classification call site at line 393-397. + +**HOW:** Add an AST check on the `ast.ExceptHandler.body`: require either an `ast.Raise` node where `exc.func.id == "HTTPException"` OR a `return` statement returning a `Result` constructor call. If neither, re-classify as INTERNAL_SILENT_SWALLOW (if body has logging) or INTERNAL_COMPLIANT (if body is `try/finally` cleanup only). + +**SAFETY:** The classification tightening affects all 65 src/ files. The 3 unit tests in FR7 lock the regression boundary; the 15 existing BOUNDARY_FASTAPI sites must remain classified. + +### 22.6 Architecture Reference + +- `conductor/code_styleguides/error_handling.md:462-476` - "What is NOT a drain point" (the rule being enforced). +- `conductor/code_styleguides/error_handling.md:496-516` - Heuristic D (the legitimate drain-point heuristic Phase 7 must not regress). +- `conductor/code_styleguides/error_handling.md:530` - the "logging is NOT a drain" rule. +- `docs/guide_app_controller.md` "Modular Controller Pattern" - the helper-extraction pattern. +- `conductor/tracks/result_migration_app_controller_20260618/spec.md` Phase 6 addendum sections 12-21 - the addendum pattern this phase follows. + +### 22.7 Verification Criteria + +- **VC1** `audit_exception_handling.py --src src/app_controller.py --strict` exits 0. +- **VC2** 4 unit tests in `tests/test_app_controller_result.py` pass (one per migrated site). +- **VC3** 3 unit tests in `tests/test_audit_heuristics.py` pass (heuristic regression-guard). +- **VC4** Full 11-tier batched suite passes; no new regressions. +- **VC5** Git history shows 7+ atomic commits (4 site migrations + 1 heuristic fix + 1 tests + 1 state updates). +- **VC6** Phase 7 checkpoint commit with git note documenting audit before/after metrics. + +### 22.8 Out of Scope + +- Other `_api_*` handlers in `src/api_hooks.py` (verified compliant; tests in FR7 guard against regression). +- 38 INTERNAL_BROAD_CATCH sites in `src/gui_2.py` (sub-track 4 territory). +- 77 violations in the 3 refactored baseline files (sub-track 5 territory per completion report section 7.2). + +### 22.9 Risks + +- **R7-1** Heuristic tightening may regress other files' `_api_*` boundary sites. Mitigation: FR7's 3 unit tests lock the 15 existing BOUNDARY_FASTAPI sites; manual verification of `src/api_hooks.py` during implementation. +- **R7-2** Legacy wrapper for `_push_mma_state_update` preserves fire-and-forget. Mitigation: docstring deprecation note; follow-up track migrates callers. +- **R7-3** `_last_request_errors` may grow unbounded. Mitigation: verify Phase 6 reset the field per-request; add reset if missing. + diff --git a/conductor/tracks/result_migration_app_controller_20260618/state.toml b/conductor/tracks/result_migration_app_controller_20260618/state.toml index ad6b9d88..cddc3ae3 100644 --- a/conductor/tracks/result_migration_app_controller_20260618/state.toml +++ b/conductor/tracks/result_migration_app_controller_20260618/state.toml @@ -114,3 +114,28 @@ regression_1_fixed = true regression_2_fixed = true batched_suite_no_new_regressions = true audit_silent_swallow_zero = true + +phase_7 = { status = "pending", checkpointsha = "", name = "Strict Enforcement Cleanup: 4 silent-swallow sites + audit heuristic tightening" } + +# Phase 7: Strict Enforcement Cleanup +# Audit gate: uv run python scripts/audit_exception_handling.py --src src/app_controller.py --strict exits 0 +# AND 0 strict-violation sites (L242, L256, L5064, L5093) reported + +t7_1 = { status = "pending", commit_sha = "", description = "Confirm heuristic over-application at scripts/audit_exception_handling.py:319-321 + 393-397" } +t7_2 = { status = "pending", commit_sha = "", description = "Migrate src/app_controller.py:242 (RAG) to _rag_search_result + _last_request_errors" } +t7_3 = { status = "pending", commit_sha = "", description = "Migrate src/app_controller.py:256 (symbols) to _symbol_resolution_result + _last_request_errors" } +t7_4 = { status = "pending", commit_sha = "", description = "Migrate _push_mma_state_update: split into _push_mma_state_update_result + legacy wrapper" } +t7_5 = { status = "pending", commit_sha = "", description = "Migrate _load_active_tickets.beads inner: _load_beads_from_path_result helper" } +t7_6 = { status = "pending", commit_sha = "", description = "Tighten audit heuristic: BOUNDARY_FASTAPI only when except body raises HTTPException or returns Result" } +t7_7 = { status = "pending", commit_sha = "", description = "Add 4 unit tests in tests/test_app_controller_result.py for migrated sites" } +t7_8 = { status = "pending", commit_sha = "", description = "Add 3 unit tests in new tests/test_audit_heuristics.py for heuristic regression-guard" } +t7_9 = { status = "pending", commit_sha = "", description = "Run audit --strict; verify 0 violations + FR7 tests pass" } +t7_10 = { status = "pending", commit_sha = "", description = "Run 11-tier batched suite; verify no new regressions" } +t7_11 = { status = "pending", commit_sha = "", description = "Update state.toml Phase 7 tasks complete; update metadata.json; conductor(plan) commit" } +t7_12 = { status = "pending", commit_sha = "", description = "Phase 7 checkpoint commit with git note (audit before/after metrics)" } +t7_13 = { status = "pending", commit_sha = "", description = "Conductor - User Manual Verification (per workflow.md)" } + +[verification.phase_7] +phase_7_complete = false +audit_strict_exits_0 = false +fr7_regression_guard_tests_pass = false